[Freedreno] [PATCH 09/10] drm/atomic: Pass the full state to planes atomic disable and update

[Laurent Pinchart]

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 at cerno.tech>
> ---

[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 at ideasonboard.com>

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