Re: [PATCH v3 10/11] drm: Use state helper instead of the plane state pointer

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 3550 bytes --]

Hi Stephen,

On Mon, Mar 29, 2021 at 06:52:01PM -0700, Stephen Boyd wrote:
> Trimming Cc list way down, sorry if that's too much.
> 
> Quoting Maxime Ripard (2021-02-19 04:00:30)
> > Many drivers reference the plane->state pointer in order to get the
> > current plane state in their atomic_update or atomic_disable hooks,
> > which would be the new plane state in the global atomic state since
> > _swap_state happened when those hooks are run.
> 
> Does this mean drm_atomic_helper_swap_state()?

Yep. Previous to that call in drm_atomic_helper_commit, plane->state is
the state currently programmed in the hardware, so the old state (that's
the case you have with atomic_check for example)

Once drm_atomic_helper_swap_state has run, plane->state is now the state
that needs to be programmed into the hardware, so the new state.

> > Use the drm_atomic_get_new_plane_state helper to get that state to make it
> > more obvious.
> > 
> > This was made using the coccinelle script below:
> > 
> > @ plane_atomic_func @
> > identifier helpers;
> > identifier func;
> > @@
> > 
> > (
> >  static const struct drm_plane_helper_funcs helpers = {
> >         ...,
> >         .atomic_disable = func,
> >         ...,
> >  };
> > |
> >  static const struct drm_plane_helper_funcs helpers = {
> >         ...,
> >         .atomic_update = func,
> >         ...,
> >  };
> > )
> > 
> > @ adds_new_state @
> > identifier plane_atomic_func.func;
> > identifier plane, state;
> > identifier new_state;
> > @@
> > 
> >  func(struct drm_plane *plane, struct drm_atomic_state *state)
> >  {
> >         ...
> > -       struct drm_plane_state *new_state = plane->state;
> > +       struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, plane);
> >         ...
> >  }
> > 
> > @ include depends on adds_new_state @
> > @@
> > 
> >  #include <drm/drm_atomic.h>
> > 
> > @ no_include depends on !include && adds_new_state @
> > @@
> > 
> > + #include <drm/drm_atomic.h>
> >   #include <drm/...>
> > 
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c       | 3 ++-
> >  drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c      | 4 +++-
> >  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c      | 3 ++-
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index 31071f9e21d7..e8ce72fe54a4 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -1244,7 +1244,8 @@ static void dpu_plane_atomic_update(struct drm_plane *plane,
> >                                 struct drm_atomic_state *state)
> >  {
> >         struct dpu_plane *pdpu = to_dpu_plane(plane);
> > -       struct drm_plane_state *new_state = plane->state;
> > +       struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
> > +                                                                          plane);
> >  
> >         pdpu->is_error = false;
> >  
> 
> This is oopsing for me. It turns out that 'new_state' is NULL. According
> to the comments drm_atomic_get_new_plane_state() can return NULL if the
> plane isn't part of the global state. I haven't looked much further but
> wanted to report it here in case that type of return value makes sense.

Yeah, it can return NULL, but in this case I'm not really sure how we
could end up with a plane_state that isn't in the global state, but
somehow with the associated plane atomic_update call being run :/

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]