Hi Philipp,
On 29.07.2026 10:37, Philipp Zabel wrote:
> On Mi, 2026-07-29 at 03:54 +0100, Adrián Larumbe wrote:
> > During device probe(), failure to do a PM get() will leave the usage_count
> > set to 0, which is the value assigned at device creation time. That means
> > when the autosuspend delay expires, runtime suspend callback won't be
> > invoked, so the device will remain powered on forever.
> >
> > On top of that, failure to call PM put() during device unplug means
> > Panfrost device's PM usage_count increases monotonically for every new
> > module reload.
> >
> > The combined outcome of both of the above was that devfreq OPP transition
> > notifications would be printed all the time, even when no jobs are being
> > submitted. This quickly fills the kernel ring buffer with junk.
> >
> > Even direr than that was the fact MMU interrupts are only enabled when
> > the device is reset, so after device probe() the very first job targeting
> > the tiler heap BO would always time out, because the driver's PM runtime
> > resume callback would not be invoked.
> >
> > Fix all that by moving all GPU enabling and disabling into RPM resume and
> > suspend callbacks, and making sure we resume the device right before
> > touching any HW registers. This is done in imitation of the Panthor model.
> >
> > Signed-off-by: Adrián Larumbe <[email protected]>
> > Fixes: 635430797d3f ("drm/panfrost: Rework runtime PM initialization")
> > Fixes: 876b15d2c88d ("drm/panfrost: Fix module unload")
> > ---
> > drivers/gpu/drm/panfrost/panfrost_device.c | 375
> > +++++++++++++++++------------
> > drivers/gpu/drm/panfrost/panfrost_device.h | 9 +
> > drivers/gpu/drm/panfrost/panfrost_drv.c | 10 +-
> > drivers/gpu/drm/panfrost/panfrost_gpu.c | 7 +-
> > drivers/gpu/drm/panfrost/panfrost_job.c | 7 +-
> > 5 files changed, 237 insertions(+), 171 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c
> > b/drivers/gpu/drm/panfrost/panfrost_device.c
> > index 03905495aee2..a9d29d9ee22b 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> > @@ -34,6 +34,40 @@ static void panfrost_reset_fini(struct panfrost_device
> > *pfdev)
> > reset_control_assert(pfdev->rstc);
> > }
> >
> > +static int panfrost_clk_enable(struct panfrost_device *pfdev)
> > +{
> > + int err;
> > +
> > + err = clk_enable(pfdev->clock);
> > + if (err)
> > + return err;
> > +
> > + err = clk_enable(pfdev->bus_clock);
> > + if (err)
> > + goto disable_clock;
> > +
> > + err = clk_enable(pfdev->bus_ace_clock);
> > + if (err)
> > + goto disable_bus_clock;
> > +
> > + return 0;
> > +
> > +disable_bus_clock:
> > + clk_disable(pfdev->bus_clock);
> > +disable_clock:
> > + clk_disable(pfdev->clock);
> > +
> > + return err;
> > +}
> > +
> > +static void panfrost_clk_disable(struct panfrost_device *pfdev)
> > +{
> > + clk_disable(pfdev->bus_ace_clock);
> > + clk_disable(pfdev->bus_clock);
> > + clk_disable(pfdev->clock);
> > + reset_control_assert(pfdev->rstc);
> > +}
>
> This asymmetry is weird. Why assert the reset control in
> panfrost_clock_disable() but not deassert it in panfrost_clk_enable()?
> It's also unexpected to have a function called _clk_disable do
> something else.
You're right about this. In a newer revision, I've made sure to deassert the
reset
control in panfrost_clk_enable() and enable in panfrost_clock_disable().
I've also renamed both functions so that they reflect their handling of the
reset
control, besides managing the clocks.
> > +
> > static int panfrost_clk_init(struct panfrost_device *pfdev)
> > {
> > int err;
> > @@ -48,7 +82,7 @@ static int panfrost_clk_init(struct panfrost_device
> > *pfdev)
> > rate = clk_get_rate(pfdev->clock);
> > dev_info(pfdev->base.dev, "clock rate = %lu\n", rate);
> >
> > - err = clk_prepare_enable(pfdev->clock);
> > + err = clk_prepare(pfdev->clock);
> > if (err)
> > return err;
> >
> > @@ -57,44 +91,55 @@ static int panfrost_clk_init(struct panfrost_device
> > *pfdev)
> [...]
> > static void panfrost_clk_fini(struct panfrost_device *pfdev)
> > {
> > - clk_disable_unprepare(pfdev->bus_ace_clock);
> > - clk_disable_unprepare(pfdev->bus_clock);
> > - clk_disable_unprepare(pfdev->clock);
> > + if (!(pfdev->comp->pm_features & BIT(GPU_PM_RT)))
> > + panfrost_clk_disable(pfdev);
>
> So here you are effectively adding a reset_control_assert() that wasn't
> there before. That seems to be intentional because you are removing the
> call to panfrost_reset_fini() in panfrost_device_fini().
>
> It's confusing though, panfrost_clk_fini() now cleans up things that
> panfrost_clk_init() didn't set up.
Yes, I agree, it looks extremely confusing. As I said above, I've moved all
assert
control into the clock disable and enable functions, because they're always
meant
to happen in tandem.
> > +
> > + clk_unprepare(pfdev->bus_ace_clock);
> > + clk_unprepare(pfdev->bus_clock);
> > + clk_unprepare(pfdev->clock);
> > }
> >
> > static int panfrost_regulator_init(struct panfrost_device *pfdev)
> > @@ -212,6 +257,133 @@ static int panfrost_pm_domain_init(struct
> > panfrost_device *pfdev)
> > return err;
> > }
> >
> > +static int panfrost_device_runtime_resume(struct device *dev)
> > +{
> > + struct panfrost_device *pfdev = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
> > + ret = reset_control_deassert(pfdev->rstc);
> > + if (ret)
> > + return ret;
> > +
> > + ret = panfrost_clk_enable(pfdev);
> > + if (ret)
> > + goto err_clk;
>
> Here we clearly deassert the reset and enable the clocks ...
>
> > + }
> > +
> > + panfrost_device_reset(pfdev, true);
> > + panfrost_devfreq_resume(pfdev);
> > +
> > + return 0;
> > +
> > +err_clk:
> > + if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
> > + reset_control_assert(pfdev->rstc);
> > + return ret;
> > +}
> > +
> > +static int panfrost_device_runtime_suspend(struct device *dev)
> > +{
> > + struct panfrost_device *pfdev = dev_get_drvdata(dev);
> > +
> > + if (!panfrost_jm_is_idle(pfdev))
> > + return -EBUSY;
> > +
> > + panfrost_devfreq_suspend(pfdev);
> > + panfrost_jm_suspend_irq(pfdev);
> > + panfrost_mmu_suspend_irq(pfdev);
> > + panfrost_gpu_suspend_irq(pfdev);
> > + panfrost_gpu_power_off(pfdev);
> > +
> > + if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
> > + panfrost_clk_disable(pfdev);
>
> ... and here we don't seem to assert the reset, but secretly do.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int panfrost_device_resume(struct device *dev)
> > +{
> > + struct panfrost_device *pfdev = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF)) {
> > + unsigned long freq = pfdev->pfdevfreq.fast_rate;
> > + struct dev_pm_opp *opp;
> > +
> > + opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> > + if (IS_ERR(opp))
> > + return PTR_ERR(opp);
> > + dev_pm_opp_set_opp(dev, opp);
> > + dev_pm_opp_put(opp);
> > + }
> > +
> > + if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
> > + ret = clk_enable(pfdev->clock);
> > + if (ret)
> > + goto err_clk;
> > +
> > + if (pfdev->bus_clock) {
>
> Unnecessary check, clk_enable() handles NULL pointers.
This code I had to shuffle around so that I could call the runtime resume and
suspend functions
from panfrost_device_init() and panfrost_device_fini() when the kernel wasn't
built with CONFIG_PM.
That means those unnecessary checks were already there. I've decided to
postpone their removal until
a next patch series to avoid doing too many things here, but I'll take care of
it eventually.
> > + ret = clk_enable(pfdev->bus_clock);
> > + if (ret)
> > + goto err_bus_clk;
> > + }
> > + }
> > +
> > + ret = pm_runtime_force_resume(dev);
> > + if (ret)
> > + goto err_resume;
> > +
> > + return 0;
> > +
> > +err_resume:
> > + if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS) && pfdev->bus_clock)
>
> Unnecessary check.
>
> > + clk_disable(pfdev->bus_clock);
> > +err_bus_clk:
> > + if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS))
> > + clk_disable(pfdev->clock);
> > +err_clk:
> > + if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
> > + dev_pm_opp_set_opp(dev, NULL);
> > + return ret;
> > +}
> > +
> > +static int panfrost_device_suspend(struct device *dev)
> > +{
> > + struct panfrost_device *pfdev = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = pm_runtime_force_suspend(dev);
> > + if (ret)
> > + return ret;
> > +
> > + if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
> > + if (pfdev->bus_clock)
>
> Unnecessary check.
>
> > + clk_disable(pfdev->bus_clock);
> > +
> > + clk_disable(pfdev->clock);
> > + }
> > +
> > + if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
> > + dev_pm_opp_set_opp(dev, NULL);
> > +
> > + return 0;
> > +}
> > +
> > +EXPORT_GPL_DEV_PM_OPS(panfrost_pm_ops) = {
> > + RUNTIME_PM_OPS(panfrost_device_runtime_suspend,
> > panfrost_device_runtime_resume, NULL)
> > + SYSTEM_SLEEP_PM_OPS(panfrost_device_suspend, panfrost_device_resume)
> > +};
> > +
> > +void panfrost_try_suspend_device(struct panfrost_device *pfdev)
> > +{
> > + pm_runtime_put_sync_suspend(pfdev->base.dev);
> > +
> > + /* If PM is disabled, we need to call the suspend handler manually. */
> > + if (!IS_ENABLED(CONFIG_PM))
> > + panfrost_device_runtime_suspend(pfdev->base.dev);
> > +}
> > +
> > int panfrost_device_init(struct panfrost_device *pfdev)
> > {
> > int err;
> > @@ -265,10 +437,25 @@ int panfrost_device_init(struct panfrost_device
> > *pfdev)
> > goto out_regulator;
> > }
> >
> > - err = panfrost_gpu_init(pfdev);
> > + err = devm_pm_runtime_enable(pfdev->base.dev);
> > if (err)
> > goto out_regulator;
> >
> > + err = pm_runtime_resume_and_get(pfdev->base.dev);
> > + if (err)
> > + goto out_regulator;
> > +
> > + /* If PM is disabled, we need to call panfrost_device_runtime_resume()
> > manually. */
> > + if (!IS_ENABLED(CONFIG_PM)) {
> > + err = panfrost_device_runtime_resume(pfdev->base.dev);
> > + if (err)
> > + goto out_regulator;
> > + }
> > +
> > + err = panfrost_gpu_init(pfdev);
> > + if (err)
> > + goto out_rpm_put;
> > +
> > err = panfrost_mmu_init(pfdev);
> > if (err)
> > goto out_gpu;
> > @@ -295,6 +482,8 @@ int panfrost_device_init(struct panfrost_device *pfdev)
> > panfrost_mmu_fini(pfdev);
> > out_gpu:
> > panfrost_gpu_fini(pfdev);
> > +out_rpm_put:
> > + panfrost_try_suspend_device(pfdev);
> > out_regulator:
> > panfrost_regulator_fini(pfdev);
> > out_devfreq:
>
> Directly below this:
>
> panfrost_devfreq_fini(pfdev);
>
> out_clk:
>
> panfrost_clk_fini(pfdev);
>
> out_reset:
>
> panfrost_reset_fini(pfdev);
>
>
> _reset_fini is still called right after _clk_fini asserted the reset
> control.
Acked. Dealt with this in v5.
> > @@ -313,12 +502,10 @@ void panfrost_device_fini(struct panfrost_device
> > *pfdev)
> > panfrost_gem_shrinker_fini(pfdev);
> > panfrost_perfcnt_fini(pfdev);
> > panfrost_jm_fini(pfdev);
> > - panfrost_mmu_fini(pfdev);
> > - panfrost_gpu_fini(pfdev);
> > - panfrost_devfreq_fini(pfdev);
> > + panfrost_try_suspend_device(pfdev);
> > panfrost_regulator_fini(pfdev);
> > + panfrost_devfreq_fini(pfdev);
> > panfrost_clk_fini(pfdev);
> > - panfrost_reset_fini(pfdev);
>
> Here _reset_fini is dropped because _clk_fini already asserted the
> reset.
>
> regards
> Philipp
Thanks a lot for the review,
Adrian Larumbe
