[PATCH] drm/exynos: fimd: Guard display clock control with runtime PM calls

[Tomi Valkeinen]

Hi,

On 18/06/2025 15:06, Marek Szyprowski wrote:
> Commit c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable
> and post-disable") changed the call sequence to the CRTC enable/disable
> and bridge pre_enable/post_disable methods, so those bridge methods are
> now called when CRTC is not yet enabled.
> 
> This causes a lockup observed on Samsung Peach-Pit/Pi Chromebooks. The
> source of this lockup is a call to fimd_dp_clock_enable() function, when
> FIMD device is not yet runtime resumed. It worked before the mentioned
> commit only because the CRTC implemented by the FIMD driver was always
> enabled what guaranteed the FIMD device to be runtime resumed.
> 
> This patch adds runtime PM guards to the fimd_dp_clock_enable() function
> to enable its proper operation also when the CRTC implemented by FIMD is
> not yet enabled.
> 
> Fixes: 196e059a8a6a ("drm/exynos: convert clock_enable crtc callback to pipeline clock")
> Signed-off-by: Marek Szyprowski <m.szyprowski at samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index c394cc702d7d..205c238cc73a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -187,6 +187,7 @@ struct fimd_context {
>  	u32				i80ifcon;
>  	bool				i80_if;
>  	bool				suspended;
> +	bool				dp_clk_enabled;
>  	wait_queue_head_t		wait_vsync_queue;
>  	atomic_t			wait_vsync_event;
>  	atomic_t			win_updated;
> @@ -1047,7 +1048,18 @@ static void fimd_dp_clock_enable(struct exynos_drm_clk *clk, bool enable)
>  	struct fimd_context *ctx = container_of(clk, struct fimd_context,
>  						dp_clk);
>  	u32 val = enable ? DP_MIE_CLK_DP_ENABLE : DP_MIE_CLK_DISABLE;
> +
> +	if (enable == ctx->dp_clk_enabled)
> +		return;

Does this happen, i.e. is this function called multiple times with
enable set? If so, do you rather need ref counting here? Otherwise the
first fimd_dp_clock_enable(enable=false) call with disable the clock,
instead of the last (assuming the enable/disable calls are matched on
the caller side).

> +
> +	if (enable)
> +		pm_runtime_resume_and_get(ctx->dev);
> +
> +	ctx->dp_clk_enabled = enable;
>  	writel(val, ctx->regs + DP_MIE_CLKCON);
> +
> +	if (!enable)
> +		pm_runtime_put(ctx->dev);
>  }
>  
>  static const struct exynos_drm_crtc_ops fimd_crtc_ops = {

 Tomi