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

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