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

[daniel@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.


> 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@suse.de>
> Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
> Cc: <stable@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.

> +		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.

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@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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch