Message-ID: <sashiko-outbox-208718@kernel.org>
From: sashiko-bot@kernel.org
Subject: Re: [PATCH v2] drm/imagination: re-check firmware idle state before
 runtime suspend
To: "Baihui Liang" <liangbaihui@linux.spacemit.com>
Cc: dri-devel@lists.freedesktop.org
In-Reply-To: <20261111-img-pm-autosuspend-v2-1-427689f6f2f9@linux.spacemit.com>
References: <20261111-img-pm-autosuspend-v2-1-427689f6f2f9@linux.spacemit.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Wed, 11 Nov 2026 09:47:18 +0000
Precedence: list
Reply-To: sashiko-reviews@lists.linux.dev
Sender: "dri-devel" <dri-devel-bounces@lists.freedesktop.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/im=
agination/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;
> =20
> +	if (READ_ONCE(pvr_dev->fw_dev.initialised) && !pvr_power_is_idle(pvr_de=
v)) {
> +		/*
> +		 * 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 exampl=
e,
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 G=
PU
clocks and power enabled.

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20261111-img-pm-aut=
osuspend-v2-1-427689f6f2f9@linux.spacemit.com?part=3D1