Re: [PATCH v2] drm/imagination: re-check firmware idle state before runtime suspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/imagination: pvr_remove() fails to stop FW on non-idle
  suspend
- [Low] drm/imagination: missing Fixes trailer on runtime PM bug fix
--

commit 819e68458c31e07fc8a39ed92b923866cc2d2009
Author: Baihui Liang <liangbaihui@linux.spacemit.com>

> drm/imagination: re-check firmware idle state before runtime suspend
>
> pvr_power_device_idle() rejects runtime suspend while the firmware
> hasn't reached ROGUE_FWIF_POW_IDLE. The PM core doesn't reschedule
> idle notification after rejection, so no further pm_runtime_put()
> arrives on an idle GPU and the device stays powered indefinitely.
>
> Move the idle check into pvr_power_device_suspend() and return -EBUSY
> after pm_runtime_mark_last_busy() so the PM core reschedules the
> autosuspend timer and retries until the firmware is idle.
>
> Signed-off-by: Baihui Liang <liangbaihui@linux.spacemit.com>

[Severity: Low]
This fixes a bug in runtime PM idle handling.  Should this include:

  Fixes: 727538a4bbff ("drm/imagination: Implement power management")

[ ... ]

> diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
> index eb4b6ecdf4f4d..71267f714c736 100644
> --- a/drivers/gpu/drm/imagination/pvr_power.c
> +++ b/drivers/gpu/drm/imagination/pvr_power.c
> @@ -378,6 +378,18 @@ pvr_power_device_suspend(struct device *dev)
>  	if (!drm_dev_enter(drm_dev, &idx))
>  		return -EIO;
>  
> +	if (READ_ONCE(pvr_dev->fw_dev.initialised) && !pvr_power_is_idle(pvr_dev)) {
> +		/*
> +		 * The firmware has not drained the GPU yet. Refresh last_busy so
> +		 * the PM core reschedules the autosuspend and retries this callback
> +		 * until the firmware becomes idle.
> +		 */
> +		pm_runtime_mark_last_busy(dev);
> +		drm_dev_exit(idx);
> +
> +		return -EBUSY;
> +	}

[Severity: High]
Can this leave the firmware processor running and the GPU powered on during
pvr_remove()?

Before this change, pvr_power_is_idle() was only checked in .runtime_idle,
so synchronous pm_runtime_suspend() in pvr_remove() bypassed that check and
called pvr_power_fw_disable() to force the firmware idle and stop it.

During driver unbind, __device_release_driver() calls pm_runtime_get_sync()
followed by pm_runtime_put_sync() right before pvr_remove():

drivers/gpu/drm/imagination/pvr_drv.c:pvr_remove() {
	...
	pm_runtime_suspend(drm_dev->dev);
	pvr_device_fini(pvr_dev);
	drm_dev_unplug(drm_dev);
	pvr_watchdog_fini(pvr_dev);
}

If the firmware has not yet transitioned to ROGUE_FWIF_POW_IDLE (for example,
right after pm_runtime_get_sync() resumed the GPU or after a KCCB command),
pvr_power_device_suspend() now returns -EBUSY.  Because pm_runtime_suspend()
does not pass RPM_AUTO, rpm_suspend() does not retry and returns -EBUSY to
pvr_remove(), which ignores the error and runs pvr_device_fini() ->
pvr_fw_fini().

That skips pvr_power_fw_disable() and pwr_ops->power_off(), so pvr_fw_fini()
unmaps and frees the firmware structures, CCBs, code/data GEM objects, and
page tables while the firmware processor is still running, and leaves the GPU
clocks and power enabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-img-pm-autosuspend-v2-1-427689f6f2f9@linux.spacemit.com?part=1