Re: [PATCH v2 24/28] drm/panthor: Quiesce scheduler tick during system suspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/panthor: panthor_device_system_resume() wakes idle GPU
  via tick_work
--

commit 4eba454e973401bba31950f161b35fd4637e2ab7
Author: Karunika Choo <karunika.choo@arm.com>

drm/panthor: Quiesce scheduler tick during system suspend

This commit adds system suspend and resume callbacks that disable the
scheduler tick work before forced runtime suspend and re-enable it on
resume or suspend failure.

> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 2ccd8ad6318d1..ba5252c27410f 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -662,3 +662,29 @@ int panthor_device_suspend(struct device *dev)
>  	atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_SUSPENDED);
>  	return 0;
>  }
> +
> +int panthor_device_system_suspend(struct device *dev)
> +{
> +	struct panthor_device *ptdev = dev_get_drvdata(dev);
> +	int ret;
> +
> +	panthor_sched_pre_suspend(ptdev);
> +
> +	ret = pm_runtime_force_suspend(dev);
> +	if (ret)
> +		panthor_sched_post_resume(ptdev);
> +
> +	return ret;
> +}
> +
> +int panthor_device_system_resume(struct device *dev)
> +{
> +	struct panthor_device *ptdev = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pm_runtime_force_resume(dev);
> +
> +	panthor_sched_post_resume(ptdev);
> +
> +	return ret;
> +}

[ ... ]

> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index a343c5180e01e..3b2ed4095f80e 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -4047,6 +4047,21 @@ void panthor_job_update_resvs(struct drm_exec *exec, struct drm_sched_job *sched
>  				DMA_RESV_USAGE_BOOKKEEP, DMA_RESV_USAGE_BOOKKEEP);
>  }
>  
> +void panthor_sched_pre_suspend(struct panthor_device *ptdev)
> +{
> +	struct panthor_scheduler *sched = ptdev->scheduler;
> +
> +	disable_delayed_work_sync(&sched->tick_work);
> +}
> +
> +void panthor_sched_post_resume(struct panthor_device *ptdev)
> +{
> +	struct panthor_scheduler *sched = ptdev->scheduler;
> +
> +	enable_delayed_work(&sched->tick_work);
> +	panthor_sched_resume(ptdev);
> +}

[Severity: Medium]
Does calling panthor_sched_resume() unconditionally in
panthor_sched_post_resume() force a runtime wakeup of an idle GPU on every
system resume?

When the GPU is already runtime-suspended before system suspend,
pm_runtime_force_resume() leaves the device in RPM_SUSPENDED without
calling panthor_device_resume().  Calling panthor_sched_resume() queues
sched->tick_work immediately, and tick_work() then calls
panthor_device_resume_and_get(), powering up the GPU and queuing a second
tick from panthor_device_resume().

In addition, if pm_runtime_force_resume() fails, queuing sched->tick_work
causes tick_work() to immediately retry panthor_device_resume_and_get() and
hit drm_WARN_ON(&ptdev->base, ret).  Since panthor_device_resume() already
calls panthor_sched_resume() when the device actually resumes, only the
pm_runtime_force_suspend() error path needs to kick the tick if the device
remained active.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110153907.3241978-1-karunika.choo@arm.com?part=24