[PATCH 0/3] drm/panthor: Misc fixes

[PATCH 0/3] drm/panthor: Misc fixes

[Boris Brezillon]

Hello,

This is a collection of fixes for bugs found while working on adding
JM support to panthor. Those are not particularly tied to JM support
and are worth having regardless.

Note that the last one doesn't have a Fixes tag because all MMU faults
are considered fatal right now, so unblocking the writer/reader early
on a fault will just make it crash, which is eventually want would
happen anyway.

Regards,

Boris

Boris Brezillon (3):
  drm/panthor: Call panthor_gpu_coherency_init() after PM resume()
  drm/panthor: Update panthor_mmu::irq::mask when needed
  drm/panthor: Let IRQ handlers clear the interrupts themselves

 drivers/gpu/drm/panthor/panthor_device.c | 8 ++++----
 drivers/gpu/drm/panthor/panthor_device.h | 2 --
 drivers/gpu/drm/panthor/panthor_fw.c     | 2 ++
 drivers/gpu/drm/panthor/panthor_gpu.c    | 2 ++
 drivers/gpu/drm/panthor/panthor_mmu.c    | 3 +++
 5 files changed, 11 insertions(+), 6 deletions(-)

-- 
2.48.1

[PATCH 1/3] drm/panthor: Call panthor_gpu_coherency_init() after PM resume()

[Boris Brezillon]

When the device is coherent, panthor_gpu_coherency_init() will read
GPU_COHERENCY_FEATURES to make sure the GPU supports the ACE-Lite
coherency protocol, which will fail if the clocks/power-domains are
not enabled when the read is done. Move the
panthor_gpu_coherency_init() call after the device has been resumed
to prevent that.

Fixes: dd7db8d911a1 ("drm/panthor: Explicitly set the coherency mode")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_device.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index a9da1d1eeb70..c73c1608d6e6 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -171,10 +171,6 @@ int panthor_device_init(struct panthor_device *ptdev)
 	struct page *p;
 	int ret;
 
-	ret = panthor_gpu_coherency_init(ptdev);
-	if (ret)
-		return ret;
-
 	init_completion(&ptdev->unplug.done);
 	ret = drmm_mutex_init(&ptdev->base, &ptdev->unplug.lock);
 	if (ret)
@@ -247,6 +243,10 @@ int panthor_device_init(struct panthor_device *ptdev)
 	if (ret)
 		goto err_rpm_put;
 
+	ret = panthor_gpu_coherency_init(ptdev);
+	if (ret)
+		return ret;
+
 	ret = panthor_mmu_init(ptdev);
 	if (ret)
 		goto err_unplug_gpu;
-- 
2.48.1

[PATCH 2/3] drm/panthor: Update panthor_mmu::irq::mask when needed

[Boris Brezillon]

When we clear the faulty bits in the AS mask, we also need to update
the panthor_mmu::irq::mask field otherwise our IRQ handler won't get
called again until the GPU is reset.

Fixes: 647810ec2476 ("drm/panthor: Add the MMU/VM logical block")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 12a02e28f50f..7cca97d298ea 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -781,6 +781,7 @@ int panthor_vm_active(struct panthor_vm *vm)
 	if (ptdev->mmu->as.faulty_mask & panthor_mmu_as_fault_mask(ptdev, as)) {
 		gpu_write(ptdev, MMU_INT_CLEAR, panthor_mmu_as_fault_mask(ptdev, as));
 		ptdev->mmu->as.faulty_mask &= ~panthor_mmu_as_fault_mask(ptdev, as);
+		ptdev->mmu->irq.mask |= panthor_mmu_as_fault_mask(ptdev, as);
 		gpu_write(ptdev, MMU_INT_MASK, ~ptdev->mmu->as.faulty_mask);
 	}
 
-- 
2.48.1

[PATCH 3/3] drm/panthor: Let IRQ handlers clear the interrupts themselves

[Boris Brezillon]

MMU handler needs to be in control of the job interrupt clears because
clearing the interrupt also unblocks the writer/reader that triggered
the fault, and we don't want it to be unblocked until we've had a chance
to process the IRQ.

Since clearing the clearing is just one line, let's make it explicit
instead of doing it in the generic code path.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_device.h | 2 --
 drivers/gpu/drm/panthor/panthor_fw.c     | 2 ++
 drivers/gpu/drm/panthor/panthor_gpu.c    | 2 ++
 drivers/gpu/drm/panthor/panthor_mmu.c    | 2 ++
 4 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index da6574021664..4c27b6d85f46 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -383,8 +383,6 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
 		if (!status)									\
 			break;									\
 												\
-		gpu_write(ptdev, __reg_prefix ## _INT_CLEAR, status);				\
-												\
 		__handler(ptdev, status);							\
 		ret = IRQ_HANDLED;								\
 	}											\
diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index 0f52766a3120..446bb377b953 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -1008,6 +1008,8 @@ static void panthor_fw_init_global_iface(struct panthor_device *ptdev)
 
 static void panthor_job_irq_handler(struct panthor_device *ptdev, u32 status)
 {
+	gpu_write(ptdev, JOB_INT_CLEAR, status);
+
 	if (!ptdev->fw->booted && (status & JOB_INT_GLOBAL_IF))
 		ptdev->fw->booted = true;
 
diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
index 671049020afa..32d678a0114e 100644
--- a/drivers/gpu/drm/panthor/panthor_gpu.c
+++ b/drivers/gpu/drm/panthor/panthor_gpu.c
@@ -150,6 +150,8 @@ static void panthor_gpu_init_info(struct panthor_device *ptdev)
 
 static void panthor_gpu_irq_handler(struct panthor_device *ptdev, u32 status)
 {
+	gpu_write(ptdev, GPU_INT_CLEAR, status);
+
 	if (status & GPU_IRQ_FAULT) {
 		u32 fault_status = gpu_read(ptdev, GPU_FAULT_STATUS);
 		u64 address = ((u64)gpu_read(ptdev, GPU_FAULT_ADDR_HI) << 32) |
diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 7cca97d298ea..1ecb20bfbc92 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -1669,6 +1669,8 @@ static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status)
 {
 	bool has_unhandled_faults = false;
 
+	gpu_write(ptdev, MMU_INT_CLEAR, status);
+
 	status = panthor_mmu_fault_mask(ptdev, status);
 	while (status) {
 		u32 as = ffs(status | (status >> 16)) - 1;
-- 
2.48.1

Re: [PATCH 3/3] drm/panthor: Let IRQ handlers clear the interrupts themselves

[Boris Brezillon]

On Tue,  1 Apr 2025 09:57:10 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> MMU handler needs to be in control of the job interrupt clears because
> clearing the interrupt also unblocks the writer/reader that triggered
> the fault, and we don't want it to be unblocked until we've had a chance
> to process the IRQ.
> 
> Since clearing the clearing is just one line, let's make it explicit
> instead of doing it in the generic code path.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/gpu/drm/panthor/panthor_device.h | 2 --
>  drivers/gpu/drm/panthor/panthor_fw.c     | 2 ++
>  drivers/gpu/drm/panthor/panthor_gpu.c    | 2 ++
>  drivers/gpu/drm/panthor/panthor_mmu.c    | 2 ++
>  4 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index da6574021664..4c27b6d85f46 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -383,8 +383,6 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
>  		if (!status)									\
>  			break;									\
>  												\
> -		gpu_write(ptdev, __reg_prefix ## _INT_CLEAR, status);				\
> -												\
>  		__handler(ptdev, status);							\
>  		ret = IRQ_HANDLED;								\
>  	}											\
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 0f52766a3120..446bb377b953 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -1008,6 +1008,8 @@ static void panthor_fw_init_global_iface(struct panthor_device *ptdev)
>  
>  static void panthor_job_irq_handler(struct panthor_device *ptdev, u32 status)
>  {
> +	gpu_write(ptdev, JOB_INT_CLEAR, status);
> +
>  	if (!ptdev->fw->booted && (status & JOB_INT_GLOBAL_IF))
>  		ptdev->fw->booted = true;
>  
> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
> index 671049020afa..32d678a0114e 100644
> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> @@ -150,6 +150,8 @@ static void panthor_gpu_init_info(struct panthor_device *ptdev)
>  
>  static void panthor_gpu_irq_handler(struct panthor_device *ptdev, u32 status)
>  {
> +	gpu_write(ptdev, GPU_INT_CLEAR, status);
> +
>  	if (status & GPU_IRQ_FAULT) {
>  		u32 fault_status = gpu_read(ptdev, GPU_FAULT_STATUS);
>  		u64 address = ((u64)gpu_read(ptdev, GPU_FAULT_ADDR_HI) << 32) |
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 7cca97d298ea..1ecb20bfbc92 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -1669,6 +1669,8 @@ static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status)
>  {
>  	bool has_unhandled_faults = false;
>  
> +	gpu_write(ptdev, MMU_INT_CLEAR, status);

Actually, this one was meant to be moved inside the loop, and be a
per-AS clear. I'll fix that in v2.

--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -1710,6 +1710,11 @@ static void panthor_mmu_irq_handler(struct
panthor_device *ptdev, u32 status) access_type, access_type_name(ptdev,
fault_status), source_id);
 
+               /* We don't handle VM faults at the moment, so let's just clear the
+                * interrupt and let the writer/reader crash.
+                */
+               gpu_write(ptdev, MMU_INT_CLEAR, mask);
+
                /* Ignore MMU interrupts on this AS until it's been
                 * re-enabled.
                 */

Re: [PATCH 1/3] drm/panthor: Call panthor_gpu_coherency_init() after PM resume()

[Liviu Dudau]

On Tue, Apr 01, 2025 at 09:57:08AM +0200, Boris Brezillon wrote:
> When the device is coherent, panthor_gpu_coherency_init() will read
> GPU_COHERENCY_FEATURES to make sure the GPU supports the ACE-Lite
> coherency protocol, which will fail if the clocks/power-domains are
> not enabled when the read is done. Move the
> panthor_gpu_coherency_init() call after the device has been resumed
> to prevent that.

While doing this, do you mind fixing the definitions for GPU_COHERENCY_ACE
and GPU_COHERENCY_ACE_LITE? They are swapped, according to my arch spec.

Otherwise, patch looks good to me.

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> 
> Fixes: dd7db8d911a1 ("drm/panthor: Explicitly set the coherency mode")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/gpu/drm/panthor/panthor_device.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index a9da1d1eeb70..c73c1608d6e6 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -171,10 +171,6 @@ int panthor_device_init(struct panthor_device *ptdev)
>  	struct page *p;
>  	int ret;
>  
> -	ret = panthor_gpu_coherency_init(ptdev);
> -	if (ret)
> -		return ret;
> -
>  	init_completion(&ptdev->unplug.done);
>  	ret = drmm_mutex_init(&ptdev->base, &ptdev->unplug.lock);
>  	if (ret)
> @@ -247,6 +243,10 @@ int panthor_device_init(struct panthor_device *ptdev)
>  	if (ret)
>  		goto err_rpm_put;
>  
> +	ret = panthor_gpu_coherency_init(ptdev);
> +	if (ret)
> +		return ret;
> +
>  	ret = panthor_mmu_init(ptdev);
>  	if (ret)
>  		goto err_unplug_gpu;
> -- 
> 2.48.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH 2/3] drm/panthor: Update panthor_mmu::irq::mask when needed

[Liviu Dudau]

On Tue, Apr 01, 2025 at 09:57:09AM +0200, Boris Brezillon wrote:
> When we clear the faulty bits in the AS mask, we also need to update
> the panthor_mmu::irq::mask field otherwise our IRQ handler won't get
> called again until the GPU is reset.
> 
> Fixes: 647810ec2476 ("drm/panthor: Add the MMU/VM logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
>  drivers/gpu/drm/panthor/panthor_mmu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 12a02e28f50f..7cca97d298ea 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -781,6 +781,7 @@ int panthor_vm_active(struct panthor_vm *vm)
>  	if (ptdev->mmu->as.faulty_mask & panthor_mmu_as_fault_mask(ptdev, as)) {
>  		gpu_write(ptdev, MMU_INT_CLEAR, panthor_mmu_as_fault_mask(ptdev, as));
>  		ptdev->mmu->as.faulty_mask &= ~panthor_mmu_as_fault_mask(ptdev, as);
> +		ptdev->mmu->irq.mask |= panthor_mmu_as_fault_mask(ptdev, as);
>  		gpu_write(ptdev, MMU_INT_MASK, ~ptdev->mmu->as.faulty_mask);
>  	}
>  
> -- 
> 2.48.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH 1/3] drm/panthor: Call panthor_gpu_coherency_init() after PM resume()

[Boris Brezillon]

On Tue, 1 Apr 2025 17:17:40 +0100
Liviu Dudau <liviu.dudau@arm.com> wrote:

> On Tue, Apr 01, 2025 at 09:57:08AM +0200, Boris Brezillon wrote:
> > When the device is coherent, panthor_gpu_coherency_init() will read
> > GPU_COHERENCY_FEATURES to make sure the GPU supports the ACE-Lite
> > coherency protocol, which will fail if the clocks/power-domains are
> > not enabled when the read is done. Move the
> > panthor_gpu_coherency_init() call after the device has been resumed
> > to prevent that.  
> 
> While doing this, do you mind fixing the definitions for GPU_COHERENCY_ACE
> and GPU_COHERENCY_ACE_LITE? They are swapped, according to my arch spec.

Oh, nice catch! I'll add a patch fixing that in v2.

> 
> Otherwise, patch looks good to me.
> 
> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> 
> Best regards,
> Liviu
> 
> > 
> > Fixes: dd7db8d911a1 ("drm/panthor: Explicitly set the coherency mode")
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/gpu/drm/panthor/panthor_device.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> > index a9da1d1eeb70..c73c1608d6e6 100644
> > --- a/drivers/gpu/drm/panthor/panthor_device.c
> > +++ b/drivers/gpu/drm/panthor/panthor_device.c
> > @@ -171,10 +171,6 @@ int panthor_device_init(struct panthor_device *ptdev)
> >  	struct page *p;
> >  	int ret;
> >  
> > -	ret = panthor_gpu_coherency_init(ptdev);
> > -	if (ret)
> > -		return ret;
> > -
> >  	init_completion(&ptdev->unplug.done);
> >  	ret = drmm_mutex_init(&ptdev->base, &ptdev->unplug.lock);
> >  	if (ret)
> > @@ -247,6 +243,10 @@ int panthor_device_init(struct panthor_device *ptdev)
> >  	if (ret)
> >  		goto err_rpm_put;
> >  
> > +	ret = panthor_gpu_coherency_init(ptdev);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = panthor_mmu_init(ptdev);
> >  	if (ret)
> >  		goto err_unplug_gpu;
> > -- 
> > 2.48.1
> >   
>