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

[sashiko-bot@kernel.org]

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