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

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

[Marek Szyprowski]

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@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;
+
+	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 = {
-- 
2.34.1

Re: [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@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

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

[Marek Szyprowski]

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@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.

>> +
>> +	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

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

[Tomi Valkeinen]

Hi,

On 19/06/2025 01:38, Marek Szyprowski wrote:
> 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@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.

I see. It's a bit confusing call pattern, but not related to this patch.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

 Tomi

Re: [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@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@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
>
>