Re: [PATCH 5/7] drm/exynos/decon5433: fix DECON standalone update

[Andrzej Hajda <a.hajda@samsung.com>]

On 04/12/2016 04:25 AM, Inki Dae wrote:
> Hi Andrzej,
>
> 2016년 03월 23일 22:15에 Andrzej Hajda 이(가) 쓴 글:
>> DECON should be updated after un-protecting windows and after changing
>> output parameters, otherwise image is not displayed in case of HDMI path.
> I'm not sure why STANDALONE_UPDATE_F bit should be updated after un-protecting windows and changing output parameters.
> The fields with _F prefix mean that these will be applied after vsync so we use protection window in case of all registers which affect display output so that they can be updated together.
>
> Wouldn't there be other thing which affects HDMI output?
>
> In addition, we would need another patch which updates TV relevant registers only in case of DECON-TV. DECON_UPDATE is a register for DECON-TV.

DECON_UPDATE is present in both DECON and DECON-TV and in both cases
have the same field STANDALONE_UPDATE_F.

Documentation for 5433 says:
> When you modify the shadow attributed registers, set
> STANDALONE_UPDATE_F.
>
So it should be set after setting registers with _F suffix,
but it has also _F suffix - contradiction. So I guess this _F suffix
should not be treated too strictly in this case. Moreover the suffix
has been removed in Exynos7420 - it is called just STANDALONE_UPDATE.

Anyway I am not sure what is exact purpose of this register and
the changes proposed in the patch are rather results of multiple tests
with hardware - documentation is rather poor on this subject.

I am also not sure why DECON works without it but DECON-TV does not,
anyway I set it in both variants because documentation says so.

Regards
Andrzej

>
> Thanks,
> Inki Dae
>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>> index ab9154e..7fec656 100644
>> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>> @@ -191,6 +191,8 @@ static void decon_commit(struct exynos_drm_crtc *crtc)
>>  
>>  	/* enable output and display signal */
>>  	decon_set_bits(ctx, DECON_VIDCON0, VIDCON0_ENVID | VIDCON0_ENVID_F, ~0);
>> +
>> +	decon_set_bits(ctx, DECON_UPDATE, STANDALONE_UPDATE_F, ~0);
>>  }
>>  
>>  static void decon_win_set_pixfmt(struct decon_context *ctx, unsigned int win,
>> @@ -316,9 +318,6 @@ static void decon_update_plane(struct exynos_drm_crtc *crtc,
>>  
>>  	/* window enable */
>>  	decon_set_bits(ctx, DECON_WINCONx(win), WINCONx_ENWIN_F, ~0);
>> -
>> -	/* standalone update */
>> -	decon_set_bits(ctx, DECON_UPDATE, STANDALONE_UPDATE_F, ~0);
>>  }
>>  
>>  static void decon_disable_plane(struct exynos_drm_crtc *crtc,
>> @@ -336,9 +335,6 @@ static void decon_disable_plane(struct exynos_drm_crtc *crtc,
>>  	decon_set_bits(ctx, DECON_WINCONx(win), WINCONx_ENWIN_F, 0);
>>  
>>  	decon_shadow_protect_win(ctx, win, false);
>> -
>> -	/* standalone update */
>> -	decon_set_bits(ctx, DECON_UPDATE, STANDALONE_UPDATE_F, ~0);
>>  }
>>  
>>  static void decon_atomic_flush(struct exynos_drm_crtc *crtc)
>> @@ -352,6 +348,9 @@ static void decon_atomic_flush(struct exynos_drm_crtc *crtc)
>>  	for (i = ctx->first_win; i < WINDOWS_NR; i++)
>>  		decon_shadow_protect_win(ctx, i, false);
>>  
>> +	/* standalone update */
>> +	decon_set_bits(ctx, DECON_UPDATE, STANDALONE_UPDATE_F, ~0);
>> +
>>  	if (ctx->out_type == IFTYPE_I80)
>>  		set_bit(BIT_WIN_UPDATED, &ctx->flags);
>>  }
>> @@ -463,8 +462,10 @@ static void decon_clear_channels(struct exynos_drm_crtc *crtc)
>>  		decon_shadow_protect_win(ctx, win, true);
>>  		decon_set_bits(ctx, DECON_WINCONx(win), WINCONx_ENWIN_F, 0);
>>  		decon_shadow_protect_win(ctx, win, false);
>> -		decon_set_bits(ctx, DECON_UPDATE, STANDALONE_UPDATE_F, ~0);
>>  	}
>> +
>> +	decon_set_bits(ctx, DECON_UPDATE, STANDALONE_UPDATE_F, ~0);
>> +
>>  	/* TODO: wait for possible vsync */
>>  	msleep(50);
>>  
>>