Re: [PATCH v11 05/15] drm/panfrost: Consolidate device clock management and reset

[sashiko-bot@kernel.org]

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 <adrian.larumbe@collabora.com>

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/20260925-claude-fixes-v11-0-0dbf5a58e7ce@collabora.com?part=5