Re: [PATCH v2 06/11] drm: Use state helper instead of plane state pointer in atomic_check

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Maxime Ripard <maxime@cerno.tech>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Daniel Vetter <daniel.vetter@intel.com>,
	David Airlie <airlied@linux.ie>, Sean Paul <sean@poorly.run>,
	freedreno@lists.freedesktop.org,
	Tomi Valkeinen <tomba@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Jyri Sarha <jyri.sarha@iki.fi>,
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	NXP Linux Team <linux-imx@nxp.com>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	linux-arm-msm@vger.kernel.org, Shawn Guo <shawnguo@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 06/11] drm: Use state helper instead of plane state pointer in atomic_check
Date: Fri, 22 Jan 2021 14:07:22 +0200	[thread overview]
Message-ID: <YAq/+udQfTwdamQ0@intel.com> (raw)
In-Reply-To: <20210121163537.1466118-6-maxime@cerno.tech>

On Thu, Jan 21, 2021 at 05:35:31PM +0100, Maxime Ripard wrote:
> Many drivers reference the plane->state pointer in order to get the
> current plane state in their atomic_check hook, which would be the old
> plane state in the global atomic state since _swap_state hasn't happened
> when atomic_check is run.
> 
> Use the drm_atomic_get_old_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 struct drm_plane_helper_funcs helpers = {
> 	...,
> 	.atomic_check = func,
> 	...,
> };
> 
> @ replaces_old_state @
> identifier plane_atomic_func.func;
> identifier plane, state, plane_state;
> @@
> 
>  func(struct drm_plane *plane, struct drm_atomic_state *state) {
>  	...
> -	struct drm_plane_state *plane_state = plane->state;
> +	struct drm_plane_state *plane_state = drm_atomic_get_old_plane_state(state, plane);
>  	...
>  }
> 
> @@
> identifier plane_atomic_func.func;
> identifier plane, state, 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
> +	plane_state
>  	...

We don't need the <... ...> style here? It's been a while since
I did any serious cocci so I'm getting a bit rusty on the details...

Otherwise looks great
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
> 
> @ adds_old_state @
> identifier plane_atomic_func.func;
> identifier plane, state;
> @@
> 
>  func(struct drm_plane *plane, struct drm_atomic_state *state) {
> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
>  	...
> -	plane->state
> +	old_plane_state
>  	...
>  }
> 
> @ include depends on adds_old_state || replaces_old_state @
> @@
> 
>  #include <drm/drm_atomic.h>
> 
> @ no_include depends on !include && (adds_old_state || replaces_old_state) @
> @@
> 
> + #include <drm/drm_atomic.h>
>   #include <drm/...>
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/imx/ipuv3-plane.c          | 3 ++-
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 4 +++-
>  drivers/gpu/drm/tilcdc/tilcdc_plane.c      | 3 ++-
>  3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index b5f6123850bb..6484592e3f86 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -341,7 +341,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>  {
>  	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
>  									   plane);
> -	struct drm_plane_state *old_state = plane->state;
> +	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
> +									   plane);
>  	struct drm_crtc_state *crtc_state;
>  	struct device *dev = plane->dev->dev;
>  	struct drm_framebuffer *fb = new_state->fb;
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> index 4aac6217a5ad..6ce6ce09fecc 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> @@ -406,12 +406,14 @@ static int mdp5_plane_atomic_check_with_state(struct drm_crtc_state *crtc_state,
>  static int mdp5_plane_atomic_check(struct drm_plane *plane,
>  				   struct drm_atomic_state *state)
>  {
> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state,
> +										 plane);
>  	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
>  										 plane);
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  
> -	crtc = new_plane_state->crtc ? new_plane_state->crtc : plane->state->crtc;
> +	crtc = new_plane_state->crtc ? new_plane_state->crtc : old_plane_state->crtc;
>  	if (!crtc)
>  		return 0;
>  
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> index ebdd42dcaf82..c86258132432 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> @@ -26,7 +26,8 @@ static int tilcdc_plane_atomic_check(struct drm_plane *plane,
>  	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
>  									   plane);
>  	struct drm_crtc_state *crtc_state;
> -	struct drm_plane_state *old_state = plane->state;
> +	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
> +									   plane);
>  	unsigned int pitch;
>  
>  	if (!new_state->crtc)
> -- 
> 2.29.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel