Re: [PATCH] drm/panthor: Do not change the GPU clock rate while suspended

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] drm/panthor: skipped rates in panthor_devfreq_target() lost
  across resume
- [Medium] drm/panthor: lockless pm.state check in
  panthor_devfreq_can_set_rate() races
- [Low] drm/panthor: missing Fixes trailer on panthor_devfreq_target()
  bug fix
--

commit b3aaf374222f607db90947d06a6fffd9f1ae68b2
Author: Arun Iyengar <aiyengar1990@gmail.com>

> drm/panthor: Do not change the GPU clock rate while suspended
>
> From the start of panthor_device_suspend() until panthor_device_resume()
> has enabled the clocks again, the GPU clocks are off or about to be, and
> the GPU power domain may be down. devfreq can still call ->target() in
> that window: a write to min_freq or max_freq in sysfs, a thermal cooling
> state change or a PM QoS request all end up in update_devfreq() whatever
> the device state.
>
> With the RK3588 GPU core clock switched to the firmware-managed
> SCMI_CLK_GPU (needed to reach the PVTPLL rates of the upper OPPs), such
> a call is fatal. To set a PVTPLL rate TF-A programs registers in the GPU
> GRF, which sit in the GPU power domain, and with the domain off that
> access ends in an asynchronous SError and a kernel panic, for example
> after
>
>   echo 1000000000 > /sys/class/devfreq/fb000000.gpu/min_freq
>
> while the GPU is runtime suspended.
>
> Only apply a rate to the hardware while the device is resuming or
> active, plus the suspend frequency that devfreq_suspend_device() applies
> from the suspend path while the GPU is still powered. Other requests are
> still accepted and recorded by devfreq, which applies them on resume or
> at the next governor update.
>
> The RFC this is based on (Jonas Karlman's panthor SCMI clock change,
> carried in Armbian's linux-rockchip pull request 380) checked the
> runtime PM status instead. That
> status is still RPM_SUSPENDING after panthor_devfreq_suspend() has run,
> which leaves a window where the rate can be raised again right before
> the power domain goes down. The devfreq suspend count cannot be used
> either: panthor's first runtime resume calls devfreq_resume_device()
> without a matching suspend, so the count is -1 while the GPU is active
> and 0 while it is suspended.
>
> Suggested-by: Jonas Karlman <jonas@kwiboo.se>
> Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>

[Severity: Low]
This fixes a crash when panthor_devfreq_target() is called while the device
is suspended.  Should it include:

  Fixes: fac9b22df4b1 ("drm/panthor: Add the devfreq logical block")

> diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
> index 2249b41ca4afe..cab86e9c9d46a 100644
> --- a/drivers/gpu/drm/panthor/panthor_devfreq.c
> +++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
> @@ -60,20 +60,56 @@ static void panthor_devfreq_update_utilization(struct panthor_devfreq *pdevfreq)
>  	pdevfreq->time_last_update = now;
>  }
>  
> +/*
> + * Whether a new rate can be applied to the hardware now.
> + *
> + * From the start of panthor_device_suspend() until panthor_device_resume()
> + * has enabled the clocks again, the GPU clocks are about to be disabled
> + * or are off, and the GPU power domain may be down. devfreq can still
> + * call ->target() in that window: sysfs
> + * min_freq/max_freq writes, thermal cooling and PM QoS requests all end
> + * up in update_devfreq() whatever the device state.
> + *
> + * Changing the rate then is pointless, and harmful where the core clock
> + * is managed by firmware: on RK3588, setting SCMI_CLK_GPU to a PVTPLL
> + * rate with the GPU power domain off makes TF-A access the powered-down
> + * GPU GRF, which ends in an asynchronous SError. So while the device is
> + * suspending only the suspend frequency, applied by
> + * devfreq_suspend_device(), reaches the hardware, and nothing does
> + * while it is suspended. devfreq records the other requests: they are
> + * applied on resume or at the next governor update.
> + */
> +static bool panthor_devfreq_can_set_rate(struct panthor_device *ptdev,
> +					 unsigned long freq)
> +{
> +	struct devfreq *devfreq = ptdev->devfreq ? ptdev->devfreq->devfreq : NULL;
> +
> +	switch (atomic_read(&ptdev->pm.state)) {
> +	case PANTHOR_DEVICE_PM_STATE_SUSPENDED:
> +		return false;
> +	case PANTHOR_DEVICE_PM_STATE_SUSPENDING:
> +		return devfreq && devfreq->suspend_freq &&
> +		       freq == devfreq->suspend_freq;
> +	default:
> +		return true;
> +	}
> +}

[Severity: Medium]
Can panthor_devfreq_can_set_rate() still allow dev_pm_opp_set_rate() to run
while the GPU clocks are disabled or being turned off?

Neither panthor_device_suspend() nor panthor_device_resume() synchronizes
ptdev->pm.state transitions and clock enable/disable with devfreq->lock:

CPU0                                  CPU1
panthor_device_suspend()
  panthor_devfreq_suspend(ptdev)
                                      update_devfreq()
                                        panthor_devfreq_target()
                                          panthor_devfreq_can_set_rate()
                                            /* state is SUSPENDING and
                                             * freq == suspend_freq -> true
                                             */
  clk_disable_unprepare(clks.coregroup)
  clk_disable_unprepare(clks.stacks)
  clk_disable_unprepare(clks.core)
  atomic_set(..., SUSPENDED)
  /* genpd powers down GPU */
                                          dev_pm_opp_set_rate()
                                            <-- accesses powered-down GPU GRF

The same race exists when panthor_devfreq_can_set_rate() sees
PANTHOR_DEVICE_PM_STATE_ACTIVE right before panthor_device_suspend() disables
the clocks (when suspend_freq is 0 and polling is stopped,
devfreq_suspend_device() does not take devfreq->lock).  Likewise,
panthor_device_resume() sets PANTHOR_DEVICE_PM_STATE_RESUMING before
enabling clks.core, clks.stacks and clks.coregroup, and leaves it set across
clk_disable_unprepare() on the err_suspend_devfreq error path, where the
default branch in panthor_devfreq_can_set_rate() returns true while the
clocks are off.

> +
>  static int panthor_devfreq_target(struct device *dev, unsigned long *freq,
>  				  u32 flags)
>  {
> +	struct panthor_device *ptdev = dev_get_drvdata(dev);
>  	struct dev_pm_opp *opp;
> -	int err;
>  
>  	opp = devfreq_recommended_opp(dev, freq, flags);
>  	if (IS_ERR(opp))
>  		return PTR_ERR(opp);
>  	dev_pm_opp_put(opp);
>  
> -	err = dev_pm_opp_set_rate(dev, *freq);
> +	if (!panthor_devfreq_can_set_rate(ptdev, *freq))
> +		return 0;
>  
> -	return err;
> +	return dev_pm_opp_set_rate(dev, *freq);
>  }

[Severity: High]
Does returning 0 from panthor_devfreq_target() without saving *freq cause
frequency updates made while suspended to be lost across resume?

In devfreq_set_target(), devfreq updates devfreq->previous_freq = new_freq,
but only records devfreq->resume_freq when devfreq->suspend_freq is non-zero:

drivers/devfreq/devfreq.c:devfreq_set_target() {
    ...
	devfreq->previous_freq = new_freq;

	if (devfreq->suspend_freq)
		devfreq->resume_freq = new_freq;
    ...
}

When opp-suspend is not defined in DT (devfreq->suspend_freq == 0),
devfreq->resume_freq stays 0 and devfreq_resume_device() does not call
devfreq_set_target() on resume.  With non-polling governors (performance,
powersave, userspace), DEVFREQ_GOV_RESUME is a no-op, so any min_freq,
max_freq, set_freq or governor change made while runtime-suspended updates
devfreq->previous_freq without ever programming the hardware clock on resume.
With simple_ondemand, devfreq_monitor_resume() overwrites
devfreq->previous_freq with clk_get_rate() (the stale pre-suspend clock rate)
and only queues polling work for polling_ms (50 ms) later, so resumed jobs
run at the stale rate for the first 50 ms or across short job bursts.

Even when devfreq->suspend_freq is non-zero, panthor_device_suspend() sets
PANTHOR_DEVICE_PM_STATE_SUSPENDING before panthor_sched_suspend(),
panthor_fw_suspend(), panthor_mmu_suspend(), panthor_gpu_suspend() and
panthor_pwr_suspend().  Any rate change skipped during that window has its
devfreq->resume_freq overwritten when panthor_devfreq_suspend() ->
devfreq_suspend_device() calls devfreq_set_target(devfreq,
devfreq->suspend_freq, 0), so devfreq_resume_device() restores suspend_freq
instead of the requested rate.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-panthor-suspend-clk-v1-1-53457050e51e@gmail.com?part=1