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

[Inki Dae]

Hi Marek,

2025년 6월 19일 (목) 오전 7:39, Marek Szyprowski <m.szyprowski at samsung.com>님이 작성:
>
> On 18.06.2025 14:25, Tomi Valkeinen wrote:
> > 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).
>
> No reference counting is needed here, as the clock enable/disable is
> called from runtime resume/suspend of the exynos_dp (analogix_dp_core)
> and there are only single calls to enable or disable. The only problem
> is that the first call is fimd_dp_clock_enable(enable=false), which
> should be skipped from the fimd runtime PM perspective, that is why I
> added the (enable == ctx->dp_clk_enabled) check.

The fimd_dp_clock_enable function is called from
exynos_drm_pipe_clk_enable(), which, as far as I can see, is invoked
by three modules: the analogix DP driver, the exynos5433 DECON driver,
and the exynos mixer driver.

First, both the decon_atomic_enable() function in
exynos5433_drm_fimd.c and the mixer_atomic_enable() function in
exynos_mixer.c explicitly request runtime PM resume before calling
exynos_drm_pipe_clk_enable(). This ensures the device is properly
powered before any register access.

In my opinion, the root cause of this issue is that the analogix DP
driver does not follow the DRM atomic pipeline and attempts to access
the register file without requesting runtime PM resume.

As you pointed out, the main problem is that the analogix_dp driver
calls exynos_drm_pipe_clk_enable() through exynos_dp_poweron() without
requesting runtime PM.

Given that the exynos_dp.c driver, which uses the analogix DP module,
does register the runtime PM interface, we should look for a more
generic solution that ensures exynos DP is included in the DRM atomic
pipeline chain and requests runtime PM at the appropriate point.

Since this is a critical issue, I will merge the current patch without
further modifications. :)

Thanks,
Inki Dae


>
> >> +
> >> +    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
> >
> >
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
>