Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/panfrost: Unintended global prepare_lock contention in runtime PM --
commit b64fb23518fa629aff18538498a7ba6190344212 Author: Adrián Larumbe <[email protected]> drm/panfrost: Consolidate device clock management and reset This commit gathers clock enables and disables into a single function to avoid repetitions between driver init/fini and device resume/suspend. It also combines clock prepares and enables together, simplifying the management API. > 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 [ ... ] > +static int panfrost_clks_enable(struct panfrost_device *pfdev, bool > on_resume) > { > int err; > + > + err = clk_prepare_enable(pfdev->clock); [Severity: Medium] Will this introduce global lock contention during runtime PM wake-ups? The commit message notes that clk_prepare can simply increase the refcnt of an already prepared clock. However, clk_prepare() and clk_unprepare() always acquire the global prepare_lock mutex to do so. Furthermore, since the GPU driver is the sole consumer of its specific leaf clocks, the refcount might drop to 0, forcing a full hardware unprepare/prepare cycle which can be quite slow. Because panfrost_clks_enable() is called from panfrost_device_runtime_resume(), this replaces the previous clk_enable() calls (which only take a fast spinlock) with clk_prepare_enable(), causing the driver to contend for the global prepare_lock mutex every time the GPU wakes up from runtime suspend. > + if (err) > + return err; > + > + err = clk_prepare_enable(pfdev->bus_clock); > + if (err) > + goto disable_clock; > + > + if (on_resume) { > + err = clk_prepare_enable(pfdev->bus_ace_clock); > + if (err) > + goto disable_bus_clock; > + } > + > + return 0; > + > +disable_bus_clock: > + clk_disable_unprepare(pfdev->bus_clock); > +disable_clock: > + clk_disable_unprepare(pfdev->clock); > + > + return err; > +} > + > +static void panfrost_clks_disable(struct panfrost_device *pfdev, bool > on_suspend) > +{ > + if (on_suspend) > + clk_disable_unprepare(pfdev->bus_ace_clock); > + clk_disable_unprepare(pfdev->bus_clock); > + clk_disable_unprepare(pfdev->clock); [Severity: Medium] Similar to the resume path, since this is called from panfrost_device_runtime_suspend(), will replacing clk_disable() with clk_disable_unprepare() cause unintended global prepare_lock contention every time the GPU idles? [ ... ] > @@ -436,34 +450,17 @@ static int panfrost_device_runtime_resume(struct device > *dev) [ ... ] > - 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); [ ... ] > @@ -480,9 +477,7 @@ static int panfrost_device_runtime_suspend(struct device > *dev) [ ... ] > 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); > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
