Re: [PATCH 09/10] drm/atomic: Pass the full state to planes atomic disable and update
From: Laurent Pinchart
Date: Fri Jan 15 2021 - 16:07:51 EST
Hi Maxime,
Thank you for the patch.
On Fri, Jan 15, 2021 at 01:57:01PM +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
s/start convert/convert/ ?
> interface, starting with the planes atomic_update and atomic_disable.
You're not starting anymore, its 09/10 already :-)
> The conversion was done using the coccinelle script below, built tested on
> all the drivers.
>
> @@
> identifier plane, plane_state;
> symbol state;
> @@
>
> struct drm_plane_helper_funcs {
> ...
> void (*atomic_update)(struct drm_plane *plane,
> - struct drm_plane_state *plane_state);
> + struct drm_atomic_state *state);
> ...
> }
>
> @@
> identifier plane, plane_state;
> symbol state;
> @@
>
> struct drm_plane_helper_funcs {
> ...
> void (*atomic_disable)(struct drm_plane *plane,
> - struct drm_plane_state *plane_state);
> + struct drm_atomic_state *state);
> ...
> }
>
> @ plane_atomic_func @
> identifier helpers;
> identifier func;
> @@
>
> (
> static const struct drm_plane_helper_funcs helpers = {
> ...,
> .atomic_update = func,
> ...,
> };
> |
> static const struct drm_plane_helper_funcs helpers = {
> ...,
> .atomic_disable = func,
> ...,
> };
> )
>
> @@
> struct drm_plane_helper_funcs *FUNCS;
> identifier f;
> identifier crtc_state;
> identifier plane, plane_state, state;
> expression e;
> @@
>
> f(struct drm_crtc_state *crtc_state)
> {
> ...
> struct drm_atomic_state *state = e;
> <+...
> (
> - FUNCS->atomic_disable(plane, plane_state)
> + FUNCS->atomic_disable(plane, state)
> |
> - FUNCS->atomic_update(plane, plane_state)
> + FUNCS->atomic_update(plane, state)
> )
> ...+>
> }
>
> @@
> identifier plane_atomic_func.func;
> identifier plane;
> symbol state;
> @@
>
> func(struct drm_plane *plane,
> - struct drm_plane_state *state)
> + struct drm_plane_state *old_plane_state)
> {
> <...
> - state
> + old_plane_state
> ...>
> }
>
> @ ignores_old_state @
> identifier plane_atomic_func.func;
> identifier plane, old_state;
> @@
>
> func(struct drm_plane *plane, struct drm_plane_state *old_state)
> {
> ... when != old_state
> }
>
> @ adds_old_state depends on plane_atomic_func && !ignores_old_state @
> identifier plane_atomic_func.func;
> identifier plane, plane_state;
> @@
>
> func(struct drm_plane *plane, struct drm_plane_state *plane_state)
> {
> + struct drm_plane_state *plane_state = drm_atomic_get_old_plane_state(state, plane);
> ...
> }
>
> @ depends on plane_atomic_func @
> identifier plane_atomic_func.func;
> identifier plane, plane_state;
> @@
>
> func(struct drm_plane *plane,
> - struct drm_plane_state *plane_state
> + struct drm_atomic_state *state
> )
> { ... }
>
> @ include depends on adds_old_state @
> @@
>
> #include <drm/drm_atomic.h>
>
> @ no_include depends on !include && adds_old_state @
> @@
>
> + #include <drm/drm_atomic.h>
> #include <drm/...>
>
> @@
> identifier plane_atomic_func.func;
> identifier plane, state;
> identifier plane_state;
> @@
>
> func(struct drm_plane *plane, struct drm_atomic_state *state) {
> ...
> struct drm_plane_state *plane_state = drm_atomic_get_old_plane_state(state, plane);
> <+...
> - plane_state->state
> + state
> ...+>
> }
>
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> ---
[snip]
> drivers/gpu/drm/drm_atomic_helper.c | 8 ++++----
> drivers/gpu/drm/drm_simple_kms_helper.c | 4 +++-
> drivers/gpu/drm/omapdrm/omap_plane.c | 4 ++--
> drivers/gpu/drm/rcar-du/rcar_du_plane.c | 4 +++-
> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 4 +++-
> include/drm/drm_modeset_helper_vtables.h | 4 ++--
For these,
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
For drivers/gpu/drm/xlnx/zynqmp_disp.c, please see below.
[snip]
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index e278680b7d5a..39f9e6e76064 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -1166,8 +1166,10 @@ zynqmp_disp_plane_atomic_check(struct drm_plane *plane,
>
> static void
> zynqmp_disp_plane_atomic_disable(struct drm_plane *plane,
> - struct drm_plane_state *old_state)
> + struct drm_atomic_state *state)
> {
> + struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
> + plane);
> struct zynqmp_disp_layer *layer = plane_to_layer(plane);
>
> if (!old_state->fb)
> @@ -1178,8 +1180,10 @@ zynqmp_disp_plane_atomic_disable(struct drm_plane *plane,
>
> static void
> zynqmp_disp_plane_atomic_update(struct drm_plane *plane,
> - struct drm_plane_state *old_state)
> + struct drm_atomic_state *state)
> {
> + struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
> + plane);
> struct drm_plane_state *new_state = plane->state;
> struct zynqmp_disp_layer *layer = plane_to_layer(plane);
> bool format_changed = false;
> @@ -1485,20 +1489,12 @@ static void
> zynqmp_disp_crtc_atomic_disable(struct drm_crtc *crtc,
> struct drm_atomic_state *state)
> {
> - struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state,
> - crtc);
> struct zynqmp_disp *disp = crtc_to_disp(crtc);
> - struct drm_plane_state *old_plane_state;
>
> /*
> - * Disable the plane if active. The old plane state can be NULL in the
> - * .shutdown() path if the plane is already disabled, skip
> - * zynqmp_disp_plane_atomic_disable() in that case.
> + * Disable the plane if active.
> */
> - old_plane_state = drm_atomic_get_old_plane_state(old_crtc_state->state,
> - crtc->primary);
> - if (old_plane_state)
You're removing this check, but there's no safeguard in
zynqmp_disp_plane_atomic_disable(). Can drm_atomic_get_old_plane_state()
return NULL there ?
> - zynqmp_disp_plane_atomic_disable(crtc->primary, old_plane_state);
> + zynqmp_disp_plane_atomic_disable(crtc->primary, state);
>
> zynqmp_disp_disable(disp);
>
[snip]
>
--
Regards,
Laurent Pinchart