Re: [PATCH 07/10] drm: Store new plane state in a variable for atomic_update and disable

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Maxime,

Thank you for the patch.

On Fri, Jan 15, 2021 at 01:56:59PM +0100, Maxime Ripard wrote:
> In order to store the new plane state in a subsequent helper, let's move
> the plane->state dereferences into a variable.
> 
> This was done using the following coccinelle script, plus some hand
> changes for vmwgfx:
> 
> @ 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,
> 	...,
>  };
> )
> 
> @ has_new_state_old_state @
> identifier plane_atomic_func.func;
> identifier plane;
> identifier new_state;
> symbol old_state;
> @@
> 
>  func(struct drm_plane *plane, struct drm_plane_state *old_state)
>  {
>  	...
>  	struct drm_plane_state *new_state = plane->state;
> 	...
>  }
> 
> @ depends on !has_new_state_old_state @
> identifier plane_atomic_func.func;
> identifier plane;
> symbol old_state;
> @@
> 
>  func(struct drm_plane *plane, struct drm_plane_state *old_state)
>  {
> +	struct drm_plane_state *new_state = plane->state;
>  	<+...
> -	plane->state
> +	new_state
> 	...+>
>  }
> 
> @ has_new_state_state @
> identifier plane_atomic_func.func;
> identifier plane;
> identifier new_state;
> symbol state;
> @@
> 
>  func(struct drm_plane *plane, struct drm_plane_state *state)
>  {
>  	...
>  	struct drm_plane_state *new_state = plane->state;
> 	...
>  }
> 
> @ depends on !has_new_state_state @
> identifier plane_atomic_func.func;
> identifier plane;
> symbol state;
> @@
> 
>  func(struct drm_plane *plane, struct drm_plane_state *state)
>  {
> +	struct drm_plane_state *new_plane_state = plane->state;
>  	<+...
> -	plane->state
> +	new_plane_state
> 	...+>
>  }
> 
> @ has_new_state_old_s @
> identifier plane_atomic_func.func;
> identifier plane;
> identifier new_state;
> symbol old_s;
> @@
> 
>  func(struct drm_plane *plane, struct drm_plane_state *old_s)
>  {
>  	...
>  	struct drm_plane_state *new_state = plane->state;
> 	...
>  }
> 
> @ depends on !has_new_state_old_s @
> identifier plane_atomic_func.func;
> identifier plane;
> symbol old_s;
> @@
> 
>  func(struct drm_plane *plane, struct drm_plane_state *old_s)
>  {
> +	struct drm_plane_state *new_s = plane->state;
>  	<+...
> -	plane->state
> +	new_s
> 	...+>
>  }

I may have taken this as an opportunity to align naming conventions for
variables across drivers, but that may just be me.

> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

[snip]

>  drivers/gpu/drm/omapdrm/omap_plane.c          |  5 ++-
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c       |  5 ++-
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c         |  3 +-
>  drivers/gpu/drm/xlnx/zynqmp_disp.c            |  7 ++--

For these, with the small issue below addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

[snip]

> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> index 1042e1147e74..de5ad69af4cb 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -88,11 +88,12 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
>  static void omap_plane_atomic_disable(struct drm_plane *plane,
>  				      struct drm_plane_state *old_state)
>  {
> +	struct drm_plane_state *new_state = plane->state;
>  	struct omap_drm_private *priv = plane->dev->dev_private;
>  	struct omap_plane *omap_plane = to_omap_plane(plane);
>  
> -	plane->state->rotation = DRM_MODE_ROTATE_0;
> -	plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
> +	new_state->rotation = DRM_MODE_ROTATE_0;
> +	new_state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
>  			   ? 0 : omap_plane->id;

Can you fix the indentation ?

>  	dispc_ovl_enable(priv->dispc, omap_plane->id, false);

[snip]

-- 
Regards,

Laurent Pinchart