decon_commit() has a condition guard at the beginning:

	if (ctx->suspended)
		return;

But, when it is being called from decon_atomic_enable(), ctx->suspended
is still set to true, which prevents its execution. decon_commit() is
vital for setting up display timing values, without which the display
pipeline fails to function properly. Call the function after
ctx->suspended is set to false as a fix.

Cc: stable@vger.kernel.org
Fixes: 96976c3d9aff ("drm/exynos: Add DECON driver")
Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
---
 drivers/gpu/drm/exynos/exynos7_drm_decon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
index f91daefa9d2bc5e314c279822047e60ee0d7ca99..43bcbe2e2917df43d7c2d27a9771e892628dd682 100644
--- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
@@ -583,9 +583,9 @@ static void decon_atomic_enable(struct exynos_drm_crtc *crtc)
 	if (test_and_clear_bit(0, &ctx->irq_flags))
 		decon_enable_vblank(ctx->crtc);
 
-	decon_commit(ctx->crtc);
-
 	ctx->suspended = false;
+
+	decon_commit(ctx->crtc);
 }
 
 static void decon_atomic_disable(struct exynos_drm_crtc *crtc)

-- 
2.49.0

Hi,

2025년 6월 27일 (금) 오전 4:21, Kaustabh Chakraborty <kauschluss@disroot.org>님이 작성:
>
> decon_commit() has a condition guard at the beginning:
>
>         if (ctx->suspended)
>                 return;
>
> But, when it is being called from decon_atomic_enable(), ctx->suspended
> is still set to true, which prevents its execution. decon_commit() is
> vital for setting up display timing values, without which the display
> pipeline fails to function properly. Call the function after
> ctx->suspended is set to false as a fix.

Good observation. However, I think a more generic solution is needed.

>
> Cc: stable@vger.kernel.org
> Fixes: 96976c3d9aff ("drm/exynos: Add DECON driver")
> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
> ---
>  drivers/gpu/drm/exynos/exynos7_drm_decon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> index f91daefa9d2bc5e314c279822047e60ee0d7ca99..43bcbe2e2917df43d7c2d27a9771e892628dd682 100644
> --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> @@ -583,9 +583,9 @@ static void decon_atomic_enable(struct exynos_drm_crtc *crtc)
>         if (test_and_clear_bit(0, &ctx->irq_flags))
>                 decon_enable_vblank(ctx->crtc);
>
> -       decon_commit(ctx->crtc);
> -
>         ctx->suspended = false;
> +
> +       decon_commit(ctx->crtc);

There seem to be three possible solutions:

1. Remove all code related to ctx->suspended. If the pipeline flow is
properly managed as in the exynos5433_drm_decon.c module, checking the
ctx->suspended state may no longer be necessary.
2. Remove the ctx->suspended check from decon_commit(). Since the
runtime PM resume is already called before decon_commit() in
decon_atomic_enable(), the DECON controller should already be enabled
at the hardware level, and decon_commit() should work correctly.
3. Move the code that updates ctx->suspended from
decon_atomic_enable() and decon_atomic_disable() to
exynos7_decon_resume() and exynos7_decon_suspend(), respectively. The
decon_atomic_enable() function calls pm_runtime_resume_and_get(),
which ultimately triggers exynos7_decon_resume(). It would be more
appropriate to set ctx->suspended = false in the
exynos7_decon_resume() function, as this is the standard place to
handle hardware state changes and resume actions.
decon_atomic_enable() is responsible for requesting enablement of the
DECON controller, but actual hardware state transitions will be
handled within exynos7_decon_resume() and exynos7_decon_suspend().


Unfortunately, I do not have hardware to test this patch myself. Would
it be possible for you to try one of these approaches and verify the
behavior?
Option 1 would be the best solution if feasible.

Thanks,
Inki Dae

>  }
>
>  static void decon_atomic_disable(struct exynos_drm_crtc *crtc)
>
> --
> 2.49.0
>
>

On 2025-06-27 04:06, Inki Dae wrote:
> Hi,
> 
> 2025년 6월 27일 (금) 오전 4:21, Kaustabh Chakraborty 
> <kauschluss@disroot.org>님이 작성:
>> 
>> decon_commit() has a condition guard at the beginning:
>> 
>>         if (ctx->suspended)
>>                 return;
>> 
>> But, when it is being called from decon_atomic_enable(), 
>> ctx->suspended
>> is still set to true, which prevents its execution. decon_commit() is
>> vital for setting up display timing values, without which the display
>> pipeline fails to function properly. Call the function after
>> ctx->suspended is set to false as a fix.
> 
> Good observation. However, I think a more generic solution is needed.
> 
>> 
>> Cc: stable@vger.kernel.org
>> Fixes: 96976c3d9aff ("drm/exynos: Add DECON driver")
>> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
>> ---
>>  drivers/gpu/drm/exynos/exynos7_drm_decon.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c 
>> b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>> index 
>> f91daefa9d2bc5e314c279822047e60ee0d7ca99..43bcbe2e2917df43d7c2d27a9771e892628dd682 
>> 100644
>> --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>> +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>> @@ -583,9 +583,9 @@ static void decon_atomic_enable(struct 
>> exynos_drm_crtc *crtc)
>>         if (test_and_clear_bit(0, &ctx->irq_flags))
>>                 decon_enable_vblank(ctx->crtc);
>> 
>> -       decon_commit(ctx->crtc);
>> -
>>         ctx->suspended = false;
>> +
>> +       decon_commit(ctx->crtc);
> 
> There seem to be three possible solutions:
> 
> 1. Remove all code related to ctx->suspended. If the pipeline flow is
> properly managed as in the exynos5433_drm_decon.c module, checking the
> ctx->suspended state may no longer be necessary.
> 2. Remove the ctx->suspended check from decon_commit(). Since the
> runtime PM resume is already called before decon_commit() in
> decon_atomic_enable(), the DECON controller should already be enabled
> at the hardware level, and decon_commit() should work correctly.
> 3. Move the code that updates ctx->suspended from
> decon_atomic_enable() and decon_atomic_disable() to
> exynos7_decon_resume() and exynos7_decon_suspend(), respectively. The
> decon_atomic_enable() function calls pm_runtime_resume_and_get(),
> which ultimately triggers exynos7_decon_resume(). It would be more
> appropriate to set ctx->suspended = false in the
> exynos7_decon_resume() function, as this is the standard place to
> handle hardware state changes and resume actions.
> decon_atomic_enable() is responsible for requesting enablement of the
> DECON controller, but actual hardware state transitions will be
> handled within exynos7_decon_resume() and exynos7_decon_suspend().
> 
> 
> Unfortunately, I do not have hardware to test this patch myself. Would
> it be possible for you to try one of these approaches and verify the
> behavior?
> Option 1 would be the best solution if feasible.

Yes, it works fine indeed. Thanks!

> 
> Thanks,
> Inki Dae
> 
>>  }
>> 
>>  static void decon_atomic_disable(struct exynos_drm_crtc *crtc)
>> 
>> --
>> 2.49.0
>> 
>>