[PATCH v2 0/5] drm/panthor: Misc fixes

[PATCH v2 0/5] 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 two don't have Fixes tag because they fix issues
we can't really hit with the current implementation (no fault
handling, and no shared irq line).

Changelog available in each patch if you're interested.

Regards,

Boris

Boris Brezillon (5):
  drm/panthor: Fix GPU_COHERENCY_ACE[_LITE] definitions
  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
  drm/panthor: Don't update MMU_INT_MASK in panthor_mmu_irq_handler()

 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    | 7 ++++++-
 drivers/gpu/drm/panthor/panthor_regs.h   | 4 ++--
 6 files changed, 16 insertions(+), 9 deletions(-)

-- 
2.49.0

[PATCH v2 1/5] drm/panthor: Fix GPU_COHERENCY_ACE[_LITE] definitions

[Boris Brezillon]

GPU_COHERENCY_ACE and GPU_COHERENCY_ACE_LITE definitions have been
swapped.

Changes in v2:
- New patch

Reported-by: Liviu Dudau <liviu.dudau@arm.com>
Fixes: 546b366600ef ("drm/panthor: Add GPU register definitions")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_regs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_regs.h b/drivers/gpu/drm/panthor/panthor_regs.h
index b7b3b3add166..a7a323dc5cf9 100644
--- a/drivers/gpu/drm/panthor/panthor_regs.h
+++ b/drivers/gpu/drm/panthor/panthor_regs.h
@@ -133,8 +133,8 @@
 #define GPU_COHERENCY_PROT_BIT(name)			BIT(GPU_COHERENCY_  ## name)
 
 #define GPU_COHERENCY_PROTOCOL				0x304
-#define   GPU_COHERENCY_ACE				0
-#define   GPU_COHERENCY_ACE_LITE			1
+#define   GPU_COHERENCY_ACE_LITE			0
+#define   GPU_COHERENCY_ACE				1
 #define   GPU_COHERENCY_NONE				31
 
 #define MCU_CONTROL					0x700
-- 
2.49.0

Re: [PATCH v2 1/5] drm/panthor: Fix GPU_COHERENCY_ACE[_LITE] definitions

[Steven Price]

On 01/04/2025 19:23, Boris Brezillon wrote:
> GPU_COHERENCY_ACE and GPU_COHERENCY_ACE_LITE definitions have been
> swapped.
> 
> Changes in v2:
> - New patch
> 
> Reported-by: Liviu Dudau <liviu.dudau@arm.com>
> Fixes: 546b366600ef ("drm/panthor: Add GPU register definitions")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panthor/panthor_regs.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_regs.h b/drivers/gpu/drm/panthor/panthor_regs.h
> index b7b3b3add166..a7a323dc5cf9 100644
> --- a/drivers/gpu/drm/panthor/panthor_regs.h
> +++ b/drivers/gpu/drm/panthor/panthor_regs.h
> @@ -133,8 +133,8 @@
>  #define GPU_COHERENCY_PROT_BIT(name)			BIT(GPU_COHERENCY_  ## name)
>  
>  #define GPU_COHERENCY_PROTOCOL				0x304
> -#define   GPU_COHERENCY_ACE				0
> -#define   GPU_COHERENCY_ACE_LITE			1
> +#define   GPU_COHERENCY_ACE_LITE			0
> +#define   GPU_COHERENCY_ACE				1
>  #define   GPU_COHERENCY_NONE				31
>  
>  #define MCU_CONTROL					0x700

[PATCH v2 2/5] 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.

Changes in v2:
- Add Liviu's R-b

Fixes: dd7db8d911a1 ("drm/panthor: Explicitly set the coherency mode")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.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.49.0

Re: [PATCH v2 2/5] drm/panthor: Call panthor_gpu_coherency_init() after PM resume()

[Steven Price]

On 01/04/2025 19:23, 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.
> 
> Changes in v2:
> - Add Liviu's R-b
> 
> Fixes: dd7db8d911a1 ("drm/panthor: Explicitly set the coherency mode")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Reviewed-by: Steven Price <steven.price@arm.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;

[PATCH v2 3/5] 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.

Changes in v2:
- Add Liviu's R-b

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>
---
 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.49.0

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

[Steven Price]

On 01/04/2025 19:23, 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.
> 
> Changes in v2:
> - Add Liviu's R-b
> 
> 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>

Reviewed-by: Steven Price <steven.price@arm.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);
>  	}
>  

[PATCH v2 4/5] 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.

Changes in v2:
- Move the MMU_INT_CLEAR around

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    | 5 +++++
 4 files changed, 9 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..4ac95a31907d 100644
--- 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.
 		 */
-- 
2.49.0

Re: [PATCH v2 4/5] drm/panthor: Let IRQ handlers clear the interrupts themselves

[Liviu Dudau]

On Tue, Apr 01, 2025 at 08:23:47PM +0200, Boris Brezillon 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.
> 
> Changes in v2:
> - Move the MMU_INT_CLEAR around
> 
> 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_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    | 5 +++++
>  4 files changed, 9 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..4ac95a31907d 100644
> --- 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.
>  		 */
> -- 
> 2.49.0
> 

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

Re: [PATCH v2 4/5] drm/panthor: Let IRQ handlers clear the interrupts themselves

[Steven Price]

On 01/04/2025 19:23, Boris Brezillon 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.
> 
> Changes in v2:
> - Move the MMU_INT_CLEAR around
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Look at this made me look at panthor_mmu_irq_handler() and I can't
understand this bit of code:

> static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status)
> {
> 	bool has_unhandled_faults = false;
> 
> 	status = panthor_mmu_fault_mask(ptdev, status);
> 	while (status) {
> 		u32 as = ffs(status | (status >> 16)) - 1;

panthor_mmu_fault_mask() masks status with GENMASK(15, 0), so AFAICT
(status >> 16) is always 0.

Which isn't a big issue, other than you are now only clearing IRQs which
are recognised by the loop.

So this changes the behaviour to not clear interrupt bits 16-31 (the
COMMAND_COMPLETED bits). I believe we don't actually care about them and
AFAICT we always mask those interrupt bits in the IRQ mask - so it
should be safe...

TLDR; I think this change is fine, but I'm not sure if it's what you
intended and it would be good to include something in the commit message
about the functional change.

Thanks,
Steve

> ---
>  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    | 5 +++++
>  4 files changed, 9 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..4ac95a31907d 100644
> --- 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 v2 4/5] drm/panthor: Let IRQ handlers clear the interrupts themselves

[Boris Brezillon]

On Wed, 2 Apr 2025 11:20:17 +0100
Steven Price <steven.price@arm.com> wrote:

> On 01/04/2025 19:23, Boris Brezillon 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.
> > 
> > Changes in v2:
> > - Move the MMU_INT_CLEAR around
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> 
> Look at this made me look at panthor_mmu_irq_handler() and I can't
> understand this bit of code:
> 
> > static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status)
> > {
> > 	bool has_unhandled_faults = false;
> > 
> > 	status = panthor_mmu_fault_mask(ptdev, status);
> > 	while (status) {
> > 		u32 as = ffs(status | (status >> 16)) - 1;  
> 
> panthor_mmu_fault_mask() masks status with GENMASK(15, 0), so AFAICT
> (status >> 16) is always 0.
> 
> Which isn't a big issue, other than you are now only clearing IRQs which
> are recognised by the loop.
> 
> So this changes the behaviour to not clear interrupt bits 16-31 (the
> COMMAND_COMPLETED bits). I believe we don't actually care about them and
> AFAICT we always mask those interrupt bits in the IRQ mask - so it
> should be safe...
> 
> TLDR; I think this change is fine, but I'm not sure if it's what you
> intended and it would be good to include something in the commit message
> about the functional change.

Didn't realize it was changing the behavior, but I was aware we were
only unmasking fault interrupts, and I left COMPLETED ones uncleared on
purpose. I'll add a comment about that in the commit message.

> 
> Thanks,
> Steve
> 
> > ---
> >  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    | 5 +++++
> >  4 files changed, 9 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..4ac95a31907d 100644
> > --- 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 v2 4/5] drm/panthor: Let IRQ handlers clear the interrupts themselves

[Boris Brezillon]

On Wed, 2 Apr 2025 11:20:17 +0100
Steven Price <steven.price@arm.com> wrote:

> > static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status)
> > {
> > 	bool has_unhandled_faults = false;
> > 
> > 	status = panthor_mmu_fault_mask(ptdev, status);
> > 	while (status) {
> > 		u32 as = ffs(status | (status >> 16)) - 1;  
> 
> panthor_mmu_fault_mask() masks status with GENMASK(15, 0), so AFAICT
> (status >> 16) is always 0.
> 
> Which isn't a big issue, other than you are now only clearing IRQs which
> are recognised by the loop.

Quick note on that: I think it's been written when I was trying to add
support for CSF in panfrost, and panthor_mmu_fault_mask() (at the time
it was panfrost_mmu_fault_mask()) was a way to have a different
fault_mask for JM and CSF. Now that JM support in panthor is being
considered [1], I'd prefer to keep this logic, even if it seems silly
right now.

[1]https://gitlab.freedesktop.org/panfrost/linux/-/merge_requests/16

Re: [PATCH v2 4/5] drm/panthor: Let IRQ handlers clear the interrupts themselves

[Steven Price]

On 02/04/2025 11:58, Boris Brezillon wrote:
> On Wed, 2 Apr 2025 11:20:17 +0100
> Steven Price <steven.price@arm.com> wrote:
> 
>>> static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status)
>>> {
>>> 	bool has_unhandled_faults = false;
>>>
>>> 	status = panthor_mmu_fault_mask(ptdev, status);
>>> 	while (status) {
>>> 		u32 as = ffs(status | (status >> 16)) - 1;  
>>
>> panthor_mmu_fault_mask() masks status with GENMASK(15, 0), so AFAICT
>> (status >> 16) is always 0.
>>
>> Which isn't a big issue, other than you are now only clearing IRQs which
>> are recognised by the loop.
> 
> Quick note on that: I think it's been written when I was trying to add
> support for CSF in panfrost, and panthor_mmu_fault_mask() (at the time
> it was panfrost_mmu_fault_mask()) was a way to have a different
> fault_mask for JM and CSF. Now that JM support in panthor is being
> considered [1], I'd prefer to keep this logic, even if it seems silly
> right now.
> 
> [1]https://gitlab.freedesktop.org/panfrost/linux/-/merge_requests/16

Yeah no problem - it was mostly that when I looked at the code it
appears to be expecting 'status' to have bits set in the top half, but
we then only clear the bits in the lower half. It took a bit of digging
to satisfy myself that the IRQ mask always masks out the top bits.

With an updated commit message I think this is fine.

JM support in panthor is a whole other discussion ;)

Thanks,
Steve

[PATCH v2 5/5] drm/panthor: Don't update MMU_INT_MASK in panthor_mmu_irq_handler()

[Boris Brezillon]

Interrupts are automatically unmasked in
panthor_mmu_irq_threaded_handler() when the handler returns. Unmasking
prematurely might generate spurious interrupts if the IRQ line is
shared.

Changes in v2:
- New patch

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 4ac95a31907d..7a7993016314 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -1719,7 +1719,6 @@ static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status)
 		 * re-enabled.
 		 */
 		ptdev->mmu->irq.mask = new_int_mask;
-		gpu_write(ptdev, MMU_INT_MASK, new_int_mask);
 
 		if (ptdev->mmu->as.slots[as].vm)
 			ptdev->mmu->as.slots[as].vm->unhandled_fault = true;
-- 
2.49.0

Re: [PATCH v2 5/5] drm/panthor: Don't update MMU_INT_MASK in panthor_mmu_irq_handler()

[Liviu Dudau]

On Tue, Apr 01, 2025 at 08:23:48PM +0200, Boris Brezillon wrote:
> Interrupts are automatically unmasked in
> panthor_mmu_irq_threaded_handler() when the handler returns. Unmasking
> prematurely might generate spurious interrupts if the IRQ line is
> shared.

Makes sense to not do this too early.

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

Best regards,
Liviu

> 
> Changes in v2:
> - New patch
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/gpu/drm/panthor/panthor_mmu.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 4ac95a31907d..7a7993016314 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -1719,7 +1719,6 @@ static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status)
>  		 * re-enabled.
>  		 */
>  		ptdev->mmu->irq.mask = new_int_mask;
> -		gpu_write(ptdev, MMU_INT_MASK, new_int_mask);
>  
>  		if (ptdev->mmu->as.slots[as].vm)
>  			ptdev->mmu->as.slots[as].vm->unhandled_fault = true;
> -- 
> 2.49.0
> 

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

Re: [PATCH v2 5/5] drm/panthor: Don't update MMU_INT_MASK in panthor_mmu_irq_handler()

[Steven Price]

On 01/04/2025 19:23, Boris Brezillon wrote:
> Interrupts are automatically unmasked in
> panthor_mmu_irq_threaded_handler() when the handler returns. Unmasking
> prematurely might generate spurious interrupts if the IRQ line is
> shared.
> 
> Changes in v2:
> - New patch
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panthor/panthor_mmu.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 4ac95a31907d..7a7993016314 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -1719,7 +1719,6 @@ static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status)
>  		 * re-enabled.
>  		 */
>  		ptdev->mmu->irq.mask = new_int_mask;
> -		gpu_write(ptdev, MMU_INT_MASK, new_int_mask);
>  
>  		if (ptdev->mmu->as.slots[as].vm)
>  			ptdev->mmu->as.slots[as].vm->unhandled_fault = true;