Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] clk: slow clk_prepare on runtime resume due to unprepare in suspend --
commit 9d5414aa4af3f7dd6f34ed2bea83b5c9b25db769 Author: Adrián Larumbe <[email protected]> Subject: drm/panfrost: Consolidate device clock management and reset This commit gathers clock enables and disables into a single function to avoid repetition between driver init/fini and device resume/suspend. It also combines prepare/unprepare with enable/disable, under the assumption that clock preparation can simply increase the reference count of an already prepared clock. > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c > b/drivers/gpu/drm/panfrost/panfrost_device.c > index b3a53504bd016..9f2b1967a3984 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_device.c > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c [ ... ] > @@ -436,34 +450,17 @@ static int panfrost_device_runtime_resume(struct device > *dev) > if (ret) > return ret; > > - ret = clk_enable(pfdev->clock); > - if (ret) > - goto err_clk; > - > - ret = clk_enable(pfdev->bus_clock); > - if (ret) > - goto err_bus_clk; > - > - ret = clk_enable(pfdev->bus_ace_clock); > - if (ret) > - goto err_bus_ace_clk; > + ret = panfrost_clks_enable(pfdev, true); > + if (ret) { > + reset_control_assert(pfdev->rstc); > + return ret; > + } > } > > panfrost_device_reset(pfdev, true); [ ... ] > @@ -480,9 +477,7 @@ static int panfrost_device_runtime_suspend(struct device > *dev) > panfrost_gpu_power_off(pfdev); > > if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) { > - clk_disable(pfdev->bus_ace_clock); > - clk_disable(pfdev->bus_clock); > - clk_disable(pfdev->clock); > + panfrost_clks_disable(pfdev, true); > reset_control_assert(pfdev->rstc); > } [Severity: Medium] Does this introduce a performance regression by fully unpreparing the clocks on every idle state? Previously, this code only called clk_disable() during runtime suspend, leaving the clock prepared. Now panfrost_clks_disable() uses clk_disable_unprepare(), reducing the prepare refcount to 0. When the device wakes up, panfrost_device_runtime_resume() calls panfrost_clks_enable() which uses clk_prepare_enable(). Because the clock was unprepared here in suspend, the resume path will pay the penalty of executing the full sleep-capable prepare sequence from a cold state instead of just a fast atomic-safe enable. The commit message notes that "the clk_prepare_* family of functions can simply increase the refcnt of an already prepared clock". However, since the clocks are fully unprepared in this runtime suspend path, they won't be already prepared on resume. Was it intended to fully unprepare the clocks on every runtime suspend? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
