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.szyprow...@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.valkei...@ideasonboard.com>
Tomi
