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@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);
[snip]