Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Fri, 1 Dec 2023 12:14:37 +0100
From:   Boris Brezillon <boris.brezillon@collabora.com>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
Cc:     robh@kernel.org, steven.price@arm.com,
        maarten.lankhorst@linux.intel.com, mripard@kernel.org,
        tzimmermann@suse.de, airlied@gmail.com, daniel@ffwll.ch,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        kernel@collabora.com, m.szyprowski@samsung.com,
        krzysztof.kozlowski@linaro.org
Subject: Re: [PATCH v3 3/3] drm/panfrost: Synchronize and disable interrupts
 before powering off
Message-ID: <20231201121437.7d5cdefb@collabora.com>
In-Reply-To: <20231201104027.35273-4-angelogioacchino.delregno@collabora.com>
References: <20231201104027.35273-1-angelogioacchino.delregno@collabora.com>
        <20231201104027.35273-4-angelogioacchino.delregno@collabora.com>
Organization: Collabora
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri,  1 Dec 2023 11:40:27 +0100
AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
wrote:

> To make sure that we don't unintentionally perform any unclocked and/or
> unpowered R/W operation on GPU registers, before turning off clocks and
> regulators we must make sure that no GPU, JOB or MMU ISR execution is
> pending: doing that required to add a mechanism to synchronize the

                      ^ requires the addition of a mechanism...

> interrupts on suspend.
> 
> Add functions panfrost_{gpu,job,mmu}_suspend_irq() which will perform
> interrupts masking and ISR execution synchronization, and then call
> those in the panfrost_device_runtime_suspend() handler in the exact
> sequence of job (may require mmu!) -> mmu -> gpu.
> 
> As a side note, JOB and MMU suspend_irq functions needed some special
> treatment: as their interrupt handlers will unmask interrupts, it was
> necessary to add a bitmap for `is_suspended` which is used to address

            to add an `is_suspended` bitmap which is used...

> the possible corner case of unintentional IRQ unmasking because of ISR
> execution after a call to synchronize_irq().

Also fixes the case where the interrupt handler is called when the
device is suspended because the IRQ line is shared with another device.
No need to update the commit message for that though.

> 
> At resume, clear each is_suspended bit in the reset path of JOB/MMU
> to allow unmasking the interrupts.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c |  3 +++
>  drivers/gpu/drm/panfrost/panfrost_device.h |  7 +++++++
>  drivers/gpu/drm/panfrost/panfrost_gpu.c    |  6 ++++++
>  drivers/gpu/drm/panfrost/panfrost_gpu.h    |  1 +
>  drivers/gpu/drm/panfrost/panfrost_job.c    | 20 +++++++++++++++++---
>  drivers/gpu/drm/panfrost/panfrost_job.h    |  1 +
>  drivers/gpu/drm/panfrost/panfrost_mmu.c    | 19 ++++++++++++++++---
>  drivers/gpu/drm/panfrost/panfrost_mmu.h    |  1 +
>  8 files changed, 52 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index c90ad5ee34e7..a45e4addcc19 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -421,6 +421,9 @@ static int panfrost_device_runtime_suspend(struct device *dev)
>  		return -EBUSY;
>  
>  	panfrost_devfreq_suspend(pfdev);
> +	panfrost_job_suspend_irq(pfdev);
> +	panfrost_mmu_suspend_irq(pfdev);
> +	panfrost_gpu_suspend_irq(pfdev);
>  	panfrost_gpu_power_off(pfdev);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 54a8aad54259..5c24f01f8904 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -25,6 +25,12 @@ struct panfrost_perfcnt;
>  #define NUM_JOB_SLOTS 3
>  #define MAX_PM_DOMAINS 5
>  
> +enum panfrost_drv_comp_bits {
> +	PANFROST_COMP_BIT_MMU,
> +	PANFROST_COMP_BIT_JOB,

I think we need one for the GPU interrupt too, for the
irq-line-is-shared-with-another-device thing I was mentioning.

> +	PANFROST_COMP_BIT_MAX
> +};
> +
>  /**
>   * enum panfrost_gpu_pm - Supported kernel power management features
>   * @GPU_PM_CLK_DIS:  Allow disabling clocks during system suspend
> @@ -109,6 +115,7 @@ struct panfrost_device {
>  
>  	struct panfrost_features features;
>  	const struct panfrost_compatible *comp;
> +	DECLARE_BITMAP(is_suspended, PANFROST_COMP_BIT_MAX);
>  
>  	spinlock_t as_lock;
>  	unsigned long as_in_use_mask;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> index 7adc4441fa14..3a6a4fe7aca1 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> @@ -452,6 +452,12 @@ void panfrost_gpu_power_off(struct panfrost_device *pfdev)
>  		dev_err(pfdev->dev, "l2 power transition timeout");
>  }
>  
> +void panfrost_gpu_suspend_irq(struct panfrost_device *pfdev)
> +{

        set_bit(PANFROST_COMP_BIT_GPU, pfdev->is_suspended);

here, and an extra check in panfrost_gpu_irq_handler() to bail out
before the register accesses if PANFROST_COMP_BIT_GPU is set.

> +	gpu_write(pfdev, GPU_INT_MASK, 0);
> +	synchronize_irq(pfdev->gpu_irq);
> +}
> +
>  int panfrost_gpu_init(struct panfrost_device *pfdev)
>  {
>  	int err;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.h b/drivers/gpu/drm/panfrost/panfrost_gpu.h
> index 876fdad9f721..d841b86504ea 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.h
> @@ -15,6 +15,7 @@ u32 panfrost_gpu_get_latest_flush_id(struct panfrost_device *pfdev);
>  int panfrost_gpu_soft_reset(struct panfrost_device *pfdev);
>  void panfrost_gpu_power_on(struct panfrost_device *pfdev);
>  void panfrost_gpu_power_off(struct panfrost_device *pfdev);
> +void panfrost_gpu_suspend_irq(struct panfrost_device *pfdev);
>  
>  void panfrost_cycle_counter_get(struct panfrost_device *pfdev);
>  void panfrost_cycle_counter_put(struct panfrost_device *pfdev);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index f9446e197428..7600e7741211 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -405,6 +405,8 @@ void panfrost_job_enable_interrupts(struct panfrost_device *pfdev)
>  	int j;
>  	u32 irq_mask = 0;
>  
> +	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
> +
>  	for (j = 0; j < NUM_JOB_SLOTS; j++) {
>  		irq_mask |= MK_JS_MASK(j);
>  	}
> @@ -413,6 +415,14 @@ void panfrost_job_enable_interrupts(struct panfrost_device *pfdev)
>  	job_write(pfdev, JOB_INT_MASK, irq_mask);
>  }
>  
> +void panfrost_job_suspend_irq(struct panfrost_device *pfdev)
> +{
> +	set_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
> +
> +	job_write(pfdev, JOB_INT_MASK, 0);
> +	synchronize_irq(pfdev->js->irq);
> +}
> +
>  static void panfrost_job_handle_err(struct panfrost_device *pfdev,
>  				    struct panfrost_job *job,
>  				    unsigned int js)
> @@ -792,9 +802,13 @@ static irqreturn_t panfrost_job_irq_handler_thread(int irq, void *data)
>  	struct panfrost_device *pfdev = data;
>  
>  	panfrost_job_handle_irqs(pfdev);
> -	job_write(pfdev, JOB_INT_MASK,
> -		  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
> -		  GENMASK(NUM_JOB_SLOTS - 1, 0));
> +
> +	/* Enable interrupts only if we're not about to get suspended */
> +	if (!test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended))
> +		job_write(pfdev, JOB_INT_MASK,
> +			  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
> +			  GENMASK(NUM_JOB_SLOTS - 1, 0));
> +

Missing if (test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended)) in
panfrost_job_irq_handler(), to make sure you don't access the registers
if the GPU is suspended.

>  	return IRQ_HANDLED;
>  }
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
> index 17ff808dba07..ec581b97852b 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.h
> @@ -47,6 +47,7 @@ int panfrost_job_get_slot(struct panfrost_job *job);
>  int panfrost_job_push(struct panfrost_job *job);
>  void panfrost_job_put(struct panfrost_job *job);
>  void panfrost_job_enable_interrupts(struct panfrost_device *pfdev);
> +void panfrost_job_suspend_irq(struct panfrost_device *pfdev);
>  int panfrost_job_is_idle(struct panfrost_device *pfdev);
>  
>  #endif
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index ac4296c1e54b..d79d41fe22fe 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -231,6 +231,8 @@ void panfrost_mmu_reset(struct panfrost_device *pfdev)
>  {
>  	struct panfrost_mmu *mmu, *mmu_tmp;
>  
> +	clear_bit(PANFROST_COMP_BIT_MMU, pfdev->is_suspended);
> +
>  	spin_lock(&pfdev->as_lock);
>  
>  	pfdev->as_alloc_mask = 0;
> @@ -744,9 +746,12 @@ static irqreturn_t panfrost_mmu_irq_handler_thread(int irq, void *data)
>  			status = mmu_read(pfdev, MMU_INT_RAWSTAT) & ~pfdev->as_faulty_mask;
>  	}
>  
> -	spin_lock(&pfdev->as_lock);
> -	mmu_write(pfdev, MMU_INT_MASK, ~pfdev->as_faulty_mask);
> -	spin_unlock(&pfdev->as_lock);
> +	/* Enable interrupts only if we're not about to get suspended */
> +	if (!test_bit(PANFROST_COMP_BIT_MMU, pfdev->is_suspended)) {
> +		spin_lock(&pfdev->as_lock);
> +		mmu_write(pfdev, MMU_INT_MASK, ~pfdev->as_faulty_mask);
> +		spin_unlock(&pfdev->as_lock);
> +	}

Ditto, missing if (test_bit(PANFROST_COMP_BIT_MMU, pfdev->is_suspended))
in panfrost_job_irq_handler(), to make sure you don't access the
registers if the GPU is suspended.

>  
>  	return IRQ_HANDLED;
>  };
> @@ -777,3 +782,11 @@ void panfrost_mmu_fini(struct panfrost_device *pfdev)
>  {
>  	mmu_write(pfdev, MMU_INT_MASK, 0);
>  }
> +
> +void panfrost_mmu_suspend_irq(struct panfrost_device *pfdev)
> +{
> +	set_bit(PANFROST_COMP_BIT_MMU, pfdev->is_suspended);
> +
> +	mmu_write(pfdev, MMU_INT_MASK, 0);
> +	synchronize_irq(pfdev->mmu_irq);
> +}
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.h b/drivers/gpu/drm/panfrost/panfrost_mmu.h
> index cc2a0d307feb..022a9a74a114 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.h
> @@ -14,6 +14,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping);
>  int panfrost_mmu_init(struct panfrost_device *pfdev);
>  void panfrost_mmu_fini(struct panfrost_device *pfdev);
>  void panfrost_mmu_reset(struct panfrost_device *pfdev);
> +void panfrost_mmu_suspend_irq(struct panfrost_device *pfdev);
>  
>  u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu);
>  void panfrost_mmu_as_put(struct panfrost_device *pfdev, struct panfrost_mmu *mmu);