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@xxxxxxx>
> Fixes: 4961eb60f145 ("drm/ast: Enable atomic modesetting")
> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> Cc: Dave Airlie <airlied@xxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
> Cc: Emil Velikov <emil.l.velikov@xxxxxxxxx>
> Cc: "Y.C. Chen" <yc_chen@xxxxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # 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@xxxxxxxx>
> +
> 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
