[PATCH 1/3] drm/ast: Do full modeset if the primary plane's format changes

[Thomas Zimmermann]

Hi

Am 27.07.20 um 12:40 schrieb daniel at ffwll.ch:
> On Mon, Jul 27, 2020 at 09:37:05AM +0200, Thomas Zimmermann wrote:
>> The atomic modesetting code tried to distinguish format changes from
>> full modesetting operations. In practice, this was buggy and the format
>> registers were often updated even for simple pageflips.
> 
> Nah it's not buggy, it's intentional. Most hw can update formats in a flip
> withouth having to shut down the hw to do so.

Admittedly it was intentional when I write it, but it never really
worked. I think it might have even updated these color registers on each
frame.

> 
> 
>> Instead do a full modeset if the primary plane changes formats. It's
>> just as rare as an actual mode change, so there will be no performance
>> penalty.
>>
>> The patch also replaces a reference to drm_crtc_state.allow_modeset with
>> the correct call to drm_atomic_crtc_needs_modeset().
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>> Cc: Gerd Hoffmann <kraxel at redhat.com>
>> Cc: Dave Airlie <airlied at redhat.com>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Cc: Sam Ravnborg <sam at ravnborg.org>
>> Cc: Emil Velikov <emil.l.velikov at gmail.com>
>> Cc: "Y.C. Chen" <yc_chen at aspeedtech.com>
>> Cc: <stable at vger.kernel.org> # v5.6+
>> ---
>>  drivers/gpu/drm/ast/ast_mode.c | 23 ++++++++++++++++-------
>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index 154cd877d9d1..3680a000b812 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -527,8 +527,8 @@ static const uint32_t ast_primary_plane_formats[] = {
>>  static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>  						 struct drm_plane_state *state)
>>  {
>> -	struct drm_crtc_state *crtc_state;
>> -	struct ast_crtc_state *ast_crtc_state;
>> +	struct drm_crtc_state *crtc_state, *old_crtc_state;
>> +	struct ast_crtc_state *ast_crtc_state, *old_ast_crtc_state;
>>  	int ret;
>>  
>>  	if (!state->crtc)
>> @@ -550,6 +550,15 @@ static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>  
>>  	ast_crtc_state->format = state->fb->format;
>>  
>> +	old_crtc_state = drm_atomic_get_old_crtc_state(state->state, state->crtc);
>> +	if (!old_crtc_state)
>> +		return 0;
>> +	old_ast_crtc_state = to_ast_crtc_state(old_crtc_state);
>> +	if (!old_ast_crtc_state)
> 
> The above two if checks should never fail, I'd wrap them in a WARN_ON.

Really? But what's the old state when the first mode is being programmed?

> 
>> +		return 0;
>> +	if (ast_crtc_state->format != old_ast_crtc_state->format)
>> +		crtc_state->mode_changed = true;
>> +
>>  	return 0;
>>  }
>>  
>> @@ -775,18 +784,18 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
>>  
>>  	ast_state = to_ast_crtc_state(crtc->state);
>>  
>> -	format = ast_state->format;
>> -	if (!format)
>> +	if (!drm_atomic_crtc_needs_modeset(crtc->state))
>>  		return;
>>  
>> +	format = ast_state->format;
>> +	if (drm_WARN_ON_ONCE(dev, !format))
>> +		return; /* BUG: We didn't set format in primary check(). */
> 
> Hm that entire ast_state->format machinery looks kinda strange, can't you
> just look up the primary plane state everywhere and that's it?
> drm_framebuffer are fully invariant and refcounted to the state, so there
> really shouldn't be any need to copy format around.

ast_state->format is the format that has to be programmed in
atomic_flush(). If it's NULL, the current format was used. Updating the
primary plane's format also requires the vbios info, which depends on
CRTC state. So it's collected in the CRTC's atomic_check().

It felt natural to use the various atomic_check() functions to collect
and store and store away these structures, and later use them in
atomic_flush().

I'd prefer to keep the current design. It's the one that worked best
while writing the atomic-modesetting support for ast.

Best regard
Thomas

> 
> But that's maybe for a next patch. With the commit message clarified that
> everything works as designed, and maybe the two WARN_ON added:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> 
>> +
>>  	vbios_mode_info = &ast_state->vbios_mode_info;
>>  
>>  	ast_set_color_reg(ast, format);
>>  	ast_set_vbios_color_reg(ast, format, vbios_mode_info);
>>  
>> -	if (!crtc->state->mode_changed)
>> -		return;
>> -
>>  	adjusted_mode = &crtc->state->adjusted_mode;
>>  
>>  	ast_set_vbios_mode_reg(ast, adjusted_mode, vbios_mode_info);
>> -- 
>> 2.27.0
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 516 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200727/271f4db3/attachment.sig>