diff mbox series

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

Message ID 20210121163537.1466118-6-maxime@cerno.tech (mailing list archive)
State Not Applicable, archived
Headers show
Series [v2,01/11] drm/atomic: Pass the full state to planes async atomic check and update | expand

Commit Message

Maxime Ripard Jan. 21, 2021, 4:35 p.m. UTC
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
 	...
 }

@ 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(-)

Comments

Ville Syrjälä Jan. 22, 2021, 12:07 p.m. UTC | #1
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
Maxime Ripard Jan. 25, 2021, 9:07 a.m. UTC | #2
Hi Ville,

On Fri, Jan 22, 2021 at 02:07:22PM +0200, Ville Syrjälä wrote:
> 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...

You're right, I've changed it and caught some more users (ingenic). I'll update it.

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

Thanks!
Maxime
diff mbox series

Patch

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)