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

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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