Laurent Pinchart
2021-Jan-15 21:20 UTC
[PATCH 10/10] drm: Use state helper instead of the plane state pointer
Hi Maxime, Thank you for the patch. On Fri, Jan 15, 2021 at 01:57:02PM +0100, Maxime Ripard wrote:> Many drivers reference the plane->state pointer in order to get the > current plane state in their atomic_update or atomic_disable hooks,Please don't use the word "current", it's ambiguous. Do you mean old state or new state ?> which would be the new plane state in the global atomic state since > _swap_state happened when those hooks are run.Is this relevant ? drm_atomic_helper_swap_state() doesn't change the old_state and new_state pointers in drm_atomic_state as far as I can tell.> 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/...> > > Signed-off-by: Maxime Ripard <maxime at cerno.tech> > ---[snip]> drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++-- > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 3 ++- > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 3 ++- > drivers/gpu/drm/xlnx/zynqmp_disp.c | 3 ++-[snip]> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c > index cd8cf7c786b5..021a94de84a1 100644 > --- a/drivers/gpu/drm/omapdrm/omap_plane.c > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c > @@ -44,7 +44,8 @@ static void omap_plane_atomic_update(struct drm_plane *plane, > { > struct omap_drm_private *priv = plane->dev->dev_private; > struct omap_plane *omap_plane = to_omap_plane(plane); > - struct drm_plane_state *new_state = plane->state;This seems to imply that you're interested in the new state.> + struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, > + plane);Does this really make things more obvious ?> struct omap_overlay_info info; > int ret; > > @@ -89,7 +90,8 @@ static void omap_plane_atomic_update(struct drm_plane *plane, > static void omap_plane_atomic_disable(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); > struct omap_drm_private *priv = plane->dev->dev_private; > struct omap_plane *omap_plane = to_omap_plane(plane); >[snip] -- Regards, Laurent Pinchart