Re: [PATCH 2/2] drm/atomic: Pass the full state to CRTC atomic begin and flush

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Oct 31, 2020 at 1:35 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Sat, Oct 31, 2020 at 10:59:05AM +0100, Maxime Ripard wrote:
> > On Thu, Oct 29, 2020 at 03:55:37PM +0200, Ville Syrjälä wrote:
> > > On Wed, Oct 28, 2020 at 01:32:22PM +0100, Maxime Ripard wrote:
> > > > The current atomic helpers have either their object state being passed as
> > > > an argument or the full atomic state.
> > > >
> > > > The former is the pattern that was done at first, before switching to the
> > > > latter for new hooks or when it was needed.
> > > >
> > > > Let's start convert all the remaining helpers to provide a consistent
> > > > interface, starting with the CRTC's atomic_begin and atomic_flush.
> > > >
> > > > The conversion was done using the coccinelle script below, built tested on
> > > > all the drivers and actually tested on vc4.
> > > >
> > > <snip>
> > > > @@ -323,26 +323,27 @@ static void ingenic_drm_crtc_atomic_begin(struct drm_crtc *crtc,
> > > >  }
> > > >
> > > >  static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
> > > > -                                   struct drm_crtc_state *oldstate)
> > > > +                                   struct drm_atomic_state *state)
> > > >  {
> > > >   struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> > > > - struct drm_crtc_state *state = crtc->state;
> > > > - struct drm_pending_vblank_event *event = state->event;
> > > > + struct drm_crtc_state *crtc_state = crtc->state;
> > >
> > > Looks like quite a few places could use a followup to
> > > switch to get_{old,new}_crtc_state().
> >
> > That one is not entirely clear to me. crtc->state is documented as the
> > current CRTC state, but in atomic_begin / atomic_flush, does that mean
> > that it's the old state that we're going to replace, or the new state
> > that is going to replace the current state?
>
> That depends on whether it's being used before or after the
> swap_state(). Before swap_state() foo->state is the old state,
> after swap_state() foo->state is the new state.

The problem with obj->state pointers in atomic commit is when we start
to queue up more than one commit, it gets messy. Hence the long term
push to pick the right old/new state from the drm_atomic_state, so you
know you get the right one. Plus it's more self-documenting, with the
_new_/_old_ infix. Like if you lookd at _new_ in an atomic_disable
hook, that's a good reason to double-check the code does the right
thing. Or if you look at _old_ in an enable function. For special
transitions this is sometimes required, but really should stick out as
an exception.

Hence also why replacing the various obj_old_state or obj_state
pointers is a good idea, and just passing drm_atomic_state everywhere.

Oh and I guess eventually we should we should perhaps rename
drm_atomic_state to drm_atomic_state_update or similar, to really
drive how what this thing does.
-Daniel

>
> --
> Ville Syrjälä
> Intel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel