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

2021-02-23 Thread Maxime Ripard 
Hi Thomas,

On Mon, Feb 22, 2021 at 10:12:49AM +0100, Thomas Zimmermann wrote:
> Am 19.02.21 um 13:00 schrieb Maxime Ripard:
> > 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 
> > 
> > @ no_include depends on !include && (adds_old_state || replaces_old_state) @
> > @@
> > 
> > + #include 
> >#include 
> > 
> > Reviewed-by: Ville Syrjälä 
> > Signed-off-by: Maxime Ripard 
> 
> Acked-by: Thomas Zimmermann 
> 
> However, I find 'old plane state' somewhat confusing in this context,
> because it's actually the current plane state. Would it make sense to use
> drm_atomic_get_existing_plane_state() instead?

drm_atomic_get_existing_plane_state is deprecated nowadays, in favour of either
drm_atomic_get_old_plane_state or drm_atomic_get_new_plane_state

Maxime


signature.asc
Description: PGP signature

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

2021-02-22 Thread Thomas Zimmermann 



Am 19.02.21 um 13:00 schrieb Maxime Ripard:

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 

@ no_include depends on !include && (adds_old_state || replaces_old_state) @
@@

+ #include 
   #include 

Reviewed-by: Ville Syrjälä 
Signed-off-by: Maxime Ripard 


Acked-by: Thomas Zimmermann 

However, I find 'old plane state' somewhat confusing in this context, 
because it's actually the current plane state. Would it make sense to 
use drm_atomic_get_existing_plane_state() instead?


Best regards
Thomas



---

Changes from v2:
   - s/.../<.../ in the coccinelle script as suggested by Ville
---
  drivers/gpu/drm/imx/ipuv3-plane.c  |  3 ++-
  drivers/gpu/drm/ingenic/ingenic-drm-drv.c  | 16 +---
  drivers/gpu/drm/ingenic/ingenic-ipu.c  |  8 +---
  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c |  4 +++-
  drivers/gpu/drm/tilcdc/tilcdc_plane.c  |  3 ++-
  5 files changed, 21 insertions(+), 13 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/ingenic/ingenic-drm-drv.c 
b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index e6d7d0a04ddb..c022d9f1e737 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -361,11 +361,13 @@ static void ingenic_drm_crtc_atomic_flush(struct drm_crtc 
*crtc,
  static int ingenic_drm_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 ingenic_drm *priv = drm_device_get_priv(plane->dev);
struct drm_crtc_state *crtc_state;
-   struct drm_crtc *crtc = new_plane_state->crtc ?: plane->state->crtc;
+   struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc;
int ret;
  
  	if (!crtc)

@@ -399,12 +401,12 @@ static int ingenic_drm_plane_atomic_check(struct 
drm_plane *plane,
 * its position, size or depth.
 */
if (priv->soc_info->has_osd &&
-   (!plane->state->fb || !new_plane_state->fb ||
-plane->state->crtc_x != new_plane_state->crtc_x ||
-plane->state->crtc_y != new_plane_state->crtc_y ||
-plane->state->crtc_w != new_plane_state->crtc_w ||
-plane->state->crtc_h != new_plane_state->crtc_h ||
-plane->state->fb->format->format

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

2021-02-19 Thread Maxime Ripard 
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 

@ no_include depends on !include && (adds_old_state || replaces_old_state) @
@@

+ #include 
  #include 

Reviewed-by: Ville Syrjälä 
Signed-off-by: Maxime Ripard 

---

Changes from v2:
  - s/.../<.../ in the coccinelle script as suggested by Ville
---
 drivers/gpu/drm/imx/ipuv3-plane.c  |  3 ++-
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c  | 16 +---
 drivers/gpu/drm/ingenic/ingenic-ipu.c  |  8 +---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c |  4 +++-
 drivers/gpu/drm/tilcdc/tilcdc_plane.c  |  3 ++-
 5 files changed, 21 insertions(+), 13 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/ingenic/ingenic-drm-drv.c 
b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index e6d7d0a04ddb..c022d9f1e737 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -361,11 +361,13 @@ static void ingenic_drm_crtc_atomic_flush(struct drm_crtc 
*crtc,
 static int ingenic_drm_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 ingenic_drm *priv = drm_device_get_priv(plane->dev);
struct drm_crtc_state *crtc_state;
-   struct drm_crtc *crtc = new_plane_state->crtc ?: plane->state->crtc;
+   struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc;
int ret;
 
if (!crtc)
@@ -399,12 +401,12 @@ static int ingenic_drm_plane_atomic_check(struct 
drm_plane *plane,
 * its position, size or depth.
 */
if (priv->soc_info->has_osd &&
-   (!plane->state->fb || !new_plane_state->fb ||
-plane->state->crtc_x != new_plane_state->crtc_x ||
-plane->state->crtc_y != new_plane_state->crtc_y ||
-plane->state->crtc_w != new_plane_state->crtc_w ||
-plane->state->crtc_h != new_plane_state->crtc_h ||
-plane->state->fb->format->format != 
new_plane_state->fb->format->format))
+   (!old_plane_state->fb || !new_plane_state->fb ||
+old_plane_state->crtc_x != new_plane_state->crtc_x ||
+old_plane_state->crtc_y != new_plane_state->crtc_y ||
+old_plane_state->crtc_w != new_plane_state->crtc_w ||
+