On Mon, Jul 27, 2020 at 1:24 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> Hi
>
> Am 27.07.20 um 12:40 schrieb daniel@xxxxxxxx:
> > 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@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.
>
> Really? But what's the old state when the first mode is being programmed?
Uh, atomic _always_ has a state. That's why you need to call
drm_mode_config_reset, to get this entire machinery started. Also, the
sw state is supposed to always match the hw state (at least once all
the nonblocking commit workers have caught up).
> >
> >> + 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.
So if you have actual checks in atomic_check for validation, then this
makes sense - it avoids computing stuff twice and getting in wrong in
one case.
But from reading ast code all you do is store the temporary lookup in
there, and that's really just confusing. You can just look up the
atomic state, at least in atomic_check (in the commit side it's mildly
more annoying, we need to fix a few things). But it's also not totally
horrible ofc, just would be nice to improve this.
-Daniel
> 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@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
> >>
> >
>
> --
> 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
