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