[PATCH] drm/imagination: Wait for the firmware's interrupts before stopping it

[PATCH] drm/imagination: Wait for the firmware's interrupts before stopping it

[Ryan Brue]

Once the firmware has acknowledged the POW_OFF request, a runtime suspend
disables the IRQ and stops the firmware straight away, even if the
firmware has raised an interrupt the host has not taken yet. On an
MT8173 (GX6250, META firmware), the SIDEKICK_IDLE.GARTEN poll at the end
of pvr_fw_stop() can then time out. The suspend fails with -ETIMEDOUT,
runtime PM latches the error, and every later pvr_power_get() returns
-EINVAL. The recovery reset that follows can fail the same way and lose
the device.

The firmware counts the interrupts it raises in its OS data. Sample that
count in the IRQ thread after clearing the interrupt, and before
disabling the IRQ for a runtime suspend, wait until the host has seen
every interrupt the firmware counted, as Imagination's DDK does. Clear
the host's copy when a hard reset clears the firmware's.

Fixes: 727538a4bbff ("drm/imagination: Implement power management")
Cc: stable@vger.kernel.org
Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>
---
Found on an Amazon Fire HD 10 (2017), an MT8173 tablet that's not upstream
yet. Its GX6250 (4.40.2.51) is not on the driver's list and runs with
exp_hw_support. With a compositor running, about one desktop start in five
hit this before the patch. A debug build that logs timed-out register polls
named the poll, SIDEKICK_IDLE read 0x7a, GARTEN not idle.

Only META firmware on this one GPU was tested. Each failing suspend in the
instrumented runs was off by exactly one interrupt (for example firmware
14320, host 14319). Why the META core then can't go idle is inferred from
the DDK, which waits at this point; a fixed 1s delay in the same place
also stopped the failures here.

The hard-reset part is untested at runtime. A forced hard reset of this GPU
times out before the firmware objects are cleared, so the stale copy it
guards against can't arise here.

It reproduces within a minute with vkcube --present_mode 1 on the
compositor's display and the GPU's power/autosuspend_delay_ms set to 1.
---
This patch was developed with the help of an LLM. I know there's a Mesa
policy against LLM use for the powervr driver there, so if there's a policy
for drm/imagination too, let me know.
---
With continuous rendering and a 1 ms autosuspend delay, a suspend failed
this way within 22 seconds in 7 of 7 unpatched runs. Three of them were
instrumented: between them they made 2711 suspends, and the three that
failed were the only ones with an interrupt still outstanding after
disable_irq(). With this patch, 8 of 8 runs lasted 60 seconds without a
failure, and in a further run the wait was needed in 15 of 6925
suspends, for at most 6.1 ms.
---
 drivers/gpu/drm/imagination/pvr_device.c |  6 ++++++
 drivers/gpu/drm/imagination/pvr_device.h |  6 ++++++
 drivers/gpu/drm/imagination/pvr_fw.c     |  3 +++
 drivers/gpu/drm/imagination/pvr_power.c  | 27 +++++++++++++++++++++++++++
 4 files changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
index 992509d95d28..99a3842c2783 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -215,6 +215,12 @@ static irqreturn_t pvr_device_irq_thread_handler(int irq, void *data)
 		pvr_fw_irq_clear(pvr_dev);
 
 		if (READ_ONCE(pvr_dev->fw_dev.initialised)) {
+			struct rogue_fwif_osdata *osdata = pvr_dev->fw_dev.fwif_osdata;
+
+			for (u32 i = 0; i < MAX_THREAD_NUM; i++)
+				WRITE_ONCE(pvr_dev->fw_irq_count[i],
+					   READ_ONCE(osdata->interrupt_count[i]));
+
 			pvr_fwccb_process(pvr_dev);
 			pvr_kccb_wake_up_waiters(pvr_dev);
 			pvr_device_process_active_queues(pvr_dev);
diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
index c246122b4caf..91cf2bde589f 100644
--- a/drivers/gpu/drm/imagination/pvr_device.h
+++ b/drivers/gpu/drm/imagination/pvr_device.h
@@ -176,6 +176,12 @@ struct pvr_device {
 	/** @irq: IRQ number. */
 	int irq;
 
+	/**
+	 * @fw_irq_count: Firmware interrupt counts, as last sampled by the IRQ
+	 *                thread after clearing the interrupt.
+	 */
+	u32 fw_irq_count[MAX_THREAD_NUM];
+
 	/** @fwccb: Firmware CCB. */
 	struct pvr_ccb fwccb;
 
diff --git a/drivers/gpu/drm/imagination/pvr_fw.c b/drivers/gpu/drm/imagination/pvr_fw.c
index c7988278df3f..7b42a898aa00 100644
--- a/drivers/gpu/drm/imagination/pvr_fw.c
+++ b/drivers/gpu/drm/imagination/pvr_fw.c
@@ -315,6 +315,9 @@ fw_osdata_init(void *cpu_ptr, void *priv)
 	struct pvr_fw_mem *fw_mem = &pvr_dev->fw_dev.mem;
 
 	pvr_fw_object_get_fw_addr(fw_mem->power_sync_obj, &fwif_osdata->power_sync_fw_addr);
+
+	/* The firmware's interrupt counts restart from zero, and so must the host's copy. */
+	memset(pvr_dev->fw_irq_count, 0, sizeof(pvr_dev->fw_irq_count));
 }
 
 static void
diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
index eb4b6ecdf4f4..2d0fbcf457f6 100644
--- a/drivers/gpu/drm/imagination/pvr_power.c
+++ b/drivers/gpu/drm/imagination/pvr_power.c
@@ -89,6 +89,26 @@ pvr_power_request_pwr_off(struct pvr_device *pvr_dev)
 	return pvr_power_send_command(pvr_dev, &pow_cmd);
 }
 
+static bool
+pvr_power_fw_irqs_taken(struct pvr_device *pvr_dev, u32 thread)
+{
+	return READ_ONCE(pvr_dev->fw_dev.fwif_osdata->interrupt_count[thread]) ==
+	       READ_ONCE(pvr_dev->fw_irq_count[thread]);
+}
+
+static void
+pvr_power_wait_for_fw_irqs(struct pvr_device *pvr_dev)
+{
+	bool taken;
+
+	for (u32 i = 0; i < MAX_THREAD_NUM; i++) {
+		if (read_poll_timeout(pvr_power_fw_irqs_taken, taken, taken, 100,
+				      POWER_SYNC_TIMEOUT_US, false, pvr_dev, i))
+			drm_warn(from_pvr_device(pvr_dev),
+				 "FW thread %u has IRQs the host has not taken\n", i);
+	}
+}
+
 static int
 pvr_power_fw_disable(struct pvr_device *pvr_dev, bool hard_reset, bool rpm_suspend)
 {
@@ -107,6 +127,13 @@ pvr_power_fw_disable(struct pvr_device *pvr_dev, bool hard_reset, bool rpm_suspe
 	}
 
 	if (rpm_suspend) {
+		/*
+		 * Let the IRQ thread take every interrupt the firmware has raised
+		 * before stopping it, as the vendor driver does. Without this, the
+		 * META firmware processor can fail to go idle in pvr_fw_stop().
+		 */
+		pvr_power_wait_for_fw_irqs(pvr_dev);
+
 		/* This also waits for late processing of GPU or firmware IRQs in other cores */
 		disable_irq(pvr_dev->irq);
 	}

---
base-commit: 476d658928af91f5ccb455957be60763d8b00fd3
change-id: 20261004-rbrue-suez-upstreaming-pvr-fw-irq-handshake-b80953eb904b

Best regards,
-- 
Ryan Brue <ryanbrue.dev@gmail.com>

Re: [PATCH] drm/imagination: Wait for the firmware's interrupts before stopping it

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/imagination: stale fw_irq_count after pvr_fw_irq_clear()
  in soft reset
- [Low] drm/imagination: inaccurate Fixes trailer in commit message
--

commit dbe0d48bb06b24b93cd3341271e891958e4e6800
Author: Ryan Brue <ryanbrue.dev@gmail.com>

> drm/imagination: Wait for the firmware's interrupts before stopping it
>
> Once the firmware has acknowledged the POW_OFF request, a runtime suspend
> disables the IRQ and stops the firmware straight away, even if the
> firmware has raised an interrupt the host has not taken yet. On an
> MT8173 (GX6250, META firmware), the SIDEKICK_IDLE.GARTEN poll at the end
> of pvr_fw_stop() can then time out. The suspend fails with -ETIMEDOUT,
> runtime PM latches the error, and every later pvr_power_get() returns
> -EINVAL. The recovery reset that follows can fail the same way and lose
> the device.
>
> The firmware counts the interrupts it raises in its OS data. Sample that
> count in the IRQ thread after clearing the interrupt, and before
> disabling the IRQ for a runtime suspend, wait until the host has seen
> every interrupt the firmware counted, as Imagination's DDK does. Clear
> the host's copy when a hard reset clears the firmware's.
>
> Fixes: 727538a4bbff ("drm/imagination: Implement power management")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ryan Brue <ryanbrue.dev@gmail.com>

[Severity: Low]
In 727538a4bbff ("drm/imagination: Implement power management"),
pvr_power_fw_disable() was an unimplemented stub returning -ENODEV and
pvr_power_device_suspend() only disabled clocks.  Should the Fixes: tag
point to cc1aeedb98ad ("drm/imagination: Implement firmware infrastructure
and META FW support"), which introduced pvr_fw_stop() and wired up
pvr_power_fw_disable() in runtime suspend (or 74ef7844dd8c
("drm/imagination: Disable interrupts before suspending the GPU"), which
added the disable_irq() call before pvr_fw_stop())?

> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
> index 992509d95d289..99a3842c27833 100644
> --- a/drivers/gpu/drm/imagination/pvr_device.c
> +++ b/drivers/gpu/drm/imagination/pvr_device.c
> @@ -215,6 +215,12 @@ static irqreturn_t pvr_device_irq_thread_handler(int irq, void *data)
>  		pvr_fw_irq_clear(pvr_dev);
>  
>  		if (READ_ONCE(pvr_dev->fw_dev.initialised)) {
> +			struct rogue_fwif_osdata *osdata = pvr_dev->fw_dev.fwif_osdata;
> +
> +			for (u32 i = 0; i < MAX_THREAD_NUM; i++)
> +				WRITE_ONCE(pvr_dev->fw_irq_count[i],
> +					   READ_ONCE(osdata->interrupt_count[i]));
> +
>  			pvr_fwccb_process(pvr_dev);
>  			pvr_kccb_wake_up_waiters(pvr_dev);
>  			pvr_device_process_active_queues(pvr_dev);

[Severity: Medium]
Can pvr_dev->fw_irq_count fall behind osdata->interrupt_count when
pvr_fw_irq_clear() is called outside pvr_device_irq_thread_handler(), such
as during a soft reset in pvr_power_reset()?

drivers/gpu/drm/imagination/pvr_power.c:pvr_power_reset() {
    ...
		err = pvr_power_fw_disable(pvr_dev, hard_reset, false);
		if (!err) {
			if (hard_reset) {
    ...
			} else {
				/* Clear the FW faulted flags. */
				pvr_dev->fw_dev.fwif_sysdata->hwr_state_flags &=
					~(ROGUE_FWIF_HWR_FW_FAULT |
					  ROGUE_FWIF_HWR_RESTART_REQUESTED);
			}

			pvr_fw_irq_clear(pvr_dev);

			err = pvr_power_fw_enable(pvr_dev, false);
		}
    ...
}

On a soft reset (hard_reset == false), pvr_fw_hard_reset() is not called,
so osdata->interrupt_count keeps its non-zero values while
disable_irq_nosync() is in effect.  If the firmware raises another
interrupt after pvr_device_irq_thread_handler() sampled
osdata->interrupt_count (or while pvr_power_fw_disable() sends the idle
and power-off requests), pvr_fw_irq_clear() in pvr_power_reset() clears
the pending hardware interrupt without updating pvr_dev->fw_irq_count.
Because the hardware status bit is cleared, pvr_device_irq_thread_handler()
will not run for that interrupt, and the next runtime suspend will see
osdata->interrupt_count[i] != pvr_dev->fw_irq_count[i] in
pvr_power_wait_for_fw_irqs(), stalling for the full 1-second
POWER_SYNC_TIMEOUT_US and logging a false warning.  The same can happen at
boot when pvr_device_irq_init() calls pvr_fw_irq_clear() after
pvr_device_gpu_init() has already started the firmware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-rbrue-suez-upstreaming-pvr-fw-irq-handshake-v1-1-8872686d14d0@gmail.com?part=1

Re: [PATCH] drm/imagination: Wait for the firmware's interrupts before stopping it

[Ryan Brue]

On 11/10/26 8:33 PM, sashiko-bot@kernel.org wrote:
> [Severity: Low]
> In 727538a4bbff ("drm/imagination: Implement power management"),
> pvr_power_fw_disable() was an unimplemented stub returning -ENODEV and
> pvr_power_device_suspend() only disabled clocks.  Should the Fixes: tag
> point to cc1aeedb98ad ("drm/imagination: Implement firmware infrastructure
> and META FW support"), which introduced pvr_fw_stop() and wired up
> pvr_power_fw_disable() in runtime suspend (or 74ef7844dd8c
> ("drm/imagination: Disable interrupts before suspending the GPU"), which
> added the disable_irq() call before pvr_fw_stop())?
>
>
> [Severity: Medium]
> Can pvr_dev->fw_irq_count fall behind osdata->interrupt_count when
> pvr_fw_irq_clear() is called outside pvr_device_irq_thread_handler(), such
> as during a soft reset in pvr_power_reset()?
>
> drivers/gpu/drm/imagination/pvr_power.c:pvr_power_reset() {
>      ...
> 		err = pvr_power_fw_disable(pvr_dev, hard_reset, false);
> 		if (!err) {
> 			if (hard_reset) {
>      ...
> 			} else {
> 				/* Clear the FW faulted flags. */
> 				pvr_dev->fw_dev.fwif_sysdata->hwr_state_flags &=
> 					~(ROGUE_FWIF_HWR_FW_FAULT |
> 					  ROGUE_FWIF_HWR_RESTART_REQUESTED);
> 			}
>
> 			pvr_fw_irq_clear(pvr_dev);
>
> 			err = pvr_power_fw_enable(pvr_dev, false);
> 		}
>      ...
> }
>
> On a soft reset (hard_reset == false), pvr_fw_hard_reset() is not called,
> so osdata->interrupt_count keeps its non-zero values while
> disable_irq_nosync() is in effect.  If the firmware raises another
> interrupt after pvr_device_irq_thread_handler() sampled
> osdata->interrupt_count (or while pvr_power_fw_disable() sends the idle
> and power-off requests), pvr_fw_irq_clear() in pvr_power_reset() clears
> the pending hardware interrupt without updating pvr_dev->fw_irq_count.
> Because the hardware status bit is cleared, pvr_device_irq_thread_handler()
> will not run for that interrupt, and the next runtime suspend will see
> osdata->interrupt_count[i] != pvr_dev->fw_irq_count[i] in
> pvr_power_wait_for_fw_irqs(), stalling for the full 1-second
> POWER_SYNC_TIMEOUT_US and logging a false warning.  The same can happen at
> boot when pvr_device_irq_init() calls pvr_fw_irq_clear() after
> pvr_device_gpu_init() has already started the firmware.
>

I always manage to trigger Sashiko bot with drm/imagination changes :D

Both of these are correct. I'll fix in v2 -- the host's copy of the 
firmware interrupt count will be taken inside pvr_fw_irq_clear(), so 
every clear updates it (IRQ thread, probe, pvr_power_reset()).

I'll wait a little bit for human review if there's anything else I missed.

Thanks,
Ryan Brue