Re: [PATCH v2] drm: Use state helper instead of CRTC state pointer

2020-11-10 Thread Maxime Ripard 
On Thu, Nov 05, 2020 at 08:07:57PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 05.11.20 um 17:45 schrieb Maxime Ripard:
> > Many drivers reference the crtc->pointer in order to get the current CRTC
> > state in their atomic_begin or atomic_flush hooks, which would be the new
> > CRTC state in the global atomic state since _swap_state happened when those
> > hooks are run.
> > 
> > Use the drm_atomic_get_new_crtc_state helper to get that state to make it
> > more obvious.
> > 
> > This was made using the coccinelle script below:
> > 
> > @ crtc_atomic_func @
> > identifier helpers;
> > identifier func;
> > @@
> > 
> > (
> > static struct drm_crtc_helper_funcs helpers = {
> > ...,
> > .atomic_begin = func,
> > ...,
> > };
> > |
> > static struct drm_crtc_helper_funcs helpers = {
> > ...,
> > .atomic_flush = func,
> > ...,
> > };
> > )
> > 
> > @@
> > identifier crtc_atomic_func.func;
> > identifier crtc, state;
> > symbol crtc_state;
> > expression e;
> > @@
> > 
> >   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
> >   ...
> > - struct tegra_dc_state *crtc_state = e;
> > + struct tegra_dc_state *dc_state = e;
> >   <+...
> > -   crtc_state
> > +   dc_state
> >   ...+>
> >   }
> > 
> > @@
> > identifier crtc_atomic_func.func;
> > identifier crtc, state;
> > symbol crtc_state;
> > expression e;
> > @@
> > 
> >   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
> >   ...
> > - struct mtk_crtc_state *crtc_state = e;
> > + struct mtk_crtc_state *mtk_crtc_state = e;
> >   <+...
> > -   crtc_state
> > +   mtk_crtc_state
> >   ...+>
> >   }
> > 
> > @ replaces_new_state @
> > identifier crtc_atomic_func.func;
> > identifier crtc, state, crtc_state;
> > @@
> > 
> >   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
> >   ...
> > - struct drm_crtc_state *crtc_state = crtc->state;
> > + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, 
> > crtc);
> >   ...
> >  }
> > 
> > @@
> > identifier crtc_atomic_func.func;
> > identifier crtc, state, crtc_state;
> > @@
> > 
> >   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
> >   struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, 
> > crtc);
> >   ...
> > - crtc->state
> > + crtc_state
> >   ...
> >  }
> > 
> > @ adds_new_state @
> > identifier crtc_atomic_func.func;
> > identifier crtc, state;
> > @@
> > 
> >   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
> > + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, 
> > crtc);
> >   ...
> > - crtc->state
> > + crtc_state
> >   ...
> >  }
> > 
> > @ include depends on adds_new_state || replaces_new_state @
> > @@
> > 
> >  #include 
> > 
> > @ no_include depends on !include && (adds_new_state || replaces_new_state) @
> > @@
> > 
> > + #include 
> >   #include 
> > 
> > Cc: "James (Qian) Wang" 
> > Cc: Liviu Dudau 
> > Cc: Mihail Atanassov 
> > Cc: Brian Starkey 
> > Cc: Russell King 
> > Cc: Paul Cercueil 
> > Cc: Chun-Kuang Hu 
> > Cc: Philipp Zabel 
> > Cc: Sandy Huang 
> > Cc: "Heiko Stübner" 
> > Cc: Thierry Reding 
> > Cc: Gerd Hoffmann 
> > Reviewed-by: Ville Syrjälä 
> > Suggested-by: Ville Syrjälä 
> > Signed-off-by: Maxime Ripard 
> Acked-by: Thomas Zimmermann 

Applied, thanks!
Maxime


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Use state helper instead of CRTC state pointer

2020-11-10 Thread Liviu Dudau 
On Thu, Nov 05, 2020 at 05:45:18PM +0100, Maxime Ripard wrote:
> Many drivers reference the crtc->pointer in order to get the current CRTC
> state in their atomic_begin or atomic_flush hooks, which would be the new
> CRTC state in the global atomic state since _swap_state happened when those
> hooks are run.
> 
> Use the drm_atomic_get_new_crtc_state helper to get that state to make it
> more obvious.
> 
> This was made using the coccinelle script below:
> 
> @ crtc_atomic_func @
> identifier helpers;
> identifier func;
> @@
> 
> (
> static struct drm_crtc_helper_funcs helpers = {
>   ...,
>   .atomic_begin = func,
>   ...,
> };
> |
> static struct drm_crtc_helper_funcs helpers = {
>   ...,
>   .atomic_flush = func,
>   ...,
> };
> )
> 
> @@
> identifier crtc_atomic_func.func;
> identifier crtc, state;
> symbol crtc_state;
> expression e;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
>   ...
> - struct tegra_dc_state *crtc_state = e;
> + struct tegra_dc_state *dc_state = e;
>   <+...
> -   crtc_state
> + dc_state
>   ...+>
>   }
> 
> @@
> identifier crtc_atomic_func.func;
> identifier crtc, state;
> symbol crtc_state;
> expression e;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
>   ...
> - struct mtk_crtc_state *crtc_state = e;
> + struct mtk_crtc_state *mtk_crtc_state = e;
>   <+...
> -   crtc_state
> + mtk_crtc_state
>   ...+>
>   }
> 
> @ replaces_new_state @
> identifier crtc_atomic_func.func;
> identifier crtc, state, crtc_state;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
>   ...
> - struct drm_crtc_state *crtc_state = crtc->state;
> + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, 
> crtc);
>   ...
>  }
> 
> @@
> identifier crtc_atomic_func.func;
> identifier crtc, state, crtc_state;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
>   struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, 
> crtc);
>   ...
> - crtc->state
> + crtc_state
>   ...
>  }
> 
> @ adds_new_state @
> identifier crtc_atomic_func.func;
> identifier crtc, state;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
> + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, 
> crtc);
>   ...
> - crtc->state
> + crtc_state
>   ...
>  }
> 
> @ include depends on adds_new_state || replaces_new_state @
> @@
> 
>  #include 
> 
> @ no_include depends on !include && (adds_new_state || replaces_new_state) @
> @@
> 
> + #include 
>   #include 
> 
> Cc: "James (Qian) Wang" 
> Cc: Liviu Dudau 
> Cc: Mihail Atanassov 
> Cc: Brian Starkey 
> Cc: Russell King 
> Cc: Paul Cercueil 
> Cc: Chun-Kuang Hu 
> Cc: Philipp Zabel 
> Cc: Sandy Huang 
> Cc: "Heiko Stübner" 
> Cc: Thierry Reding 
> Cc: Gerd Hoffmann 
> Reviewed-by: Ville Syrjälä 
> Suggested-by: Ville Syrjälä 
> Signed-off-by: Maxime Ripard 
> 
> ---
> 
> Changes from v1:
>   - Fixed checkpatch warnings
> ---
>  drivers/gpu/drm/arm/display/komeda/komeda_crtc.c |  4 +++-
>  drivers/gpu/drm/armada/armada_crtc.c |  8 ++--
>  drivers/gpu/drm/ast/ast_mode.c   |  4 +++-
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c|  7 +--
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 15 +--
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c  |  6 --
>  drivers/gpu/drm/tegra/dc.c   |  8 +---
>  drivers/gpu/drm/virtio/virtgpu_display.c |  4 +++-
>  8 files changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c 
> b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index df0b9eeb8933..4b485eb512e2 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -387,10 +387,12 @@ static void
>  komeda_crtc_atomic_flush(struct drm_crtc *crtc,
>struct drm_atomic_state *state)
>  {
> + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +   crtc);
>   struct drm_crtc_state *old = drm_atomic_get_old_crtc_state(state,
>  crtc);
>   /* commit with modeset will be handled in enable/disable */
> - if (drm_atomic_crtc_needs_modeset(crtc->state))
> + if (drm_atomic_crtc_needs_modeset(crtc_state))
>   return;
>  
>   komeda_crtc_do_flush(crtc, old);

For the komeda part:

Acked-by: Liviu Dudau 


> diff --git a/drivers/gpu/drm/armada/armada_crtc.c 
> b/drivers/gpu/drm/armada/armada_crtc.c
> index ca643f4e2064..3ebcf5a52c8b 100644
> --- a/drivers/gpu/drm/armada/armada_crtc.c
> +++ b/drivers/gpu/drm/armada/armada_crtc.c
> @@ -431,11 +431,13 @@ static int armada_drm_crtc_atomic_check(struct drm_crtc 
> *crtc,
>  static void armada_drm_crtc_atomic_begin(struct drm_crtc

[PATCH v2] drm: Use state helper instead of CRTC state pointer

2020-11-06 Thread Maxime Ripard 
Many drivers reference the crtc->pointer in order to get the current CRTC
state in their atomic_begin or atomic_flush hooks, which would be the new
CRTC state in the global atomic state since _swap_state happened when those
hooks are run.

Use the drm_atomic_get_new_crtc_state helper to get that state to make it
more obvious.

This was made using the coccinelle script below:

@ crtc_atomic_func @
identifier helpers;
identifier func;
@@

(
static struct drm_crtc_helper_funcs helpers = {
...,
.atomic_begin = func,
...,
};
|
static struct drm_crtc_helper_funcs helpers = {
...,
.atomic_flush = func,
...,
};
)

@@
identifier crtc_atomic_func.func;
identifier crtc, state;
symbol crtc_state;
expression e;
@@

  func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
  ...
- struct tegra_dc_state *crtc_state = e;
+ struct tegra_dc_state *dc_state = e;
  <+...
-   crtc_state
+   dc_state
  ...+>
  }

@@
identifier crtc_atomic_func.func;
identifier crtc, state;
symbol crtc_state;
expression e;
@@

  func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
  ...
- struct mtk_crtc_state *crtc_state = e;
+ struct mtk_crtc_state *mtk_crtc_state = e;
  <+...
-   crtc_state
+   mtk_crtc_state
  ...+>
  }

@ replaces_new_state @
identifier crtc_atomic_func.func;
identifier crtc, state, crtc_state;
@@

  func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
  ...
- struct drm_crtc_state *crtc_state = crtc->state;
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, 
crtc);
  ...
 }

@@
identifier crtc_atomic_func.func;
identifier crtc, state, crtc_state;
@@

  func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
  struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, 
crtc);
  ...
- crtc->state
+ crtc_state
  ...
 }

@ adds_new_state @
identifier crtc_atomic_func.func;
identifier crtc, state;
@@

  func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, 
crtc);
  ...
- crtc->state
+ crtc_state
  ...
 }

@ include depends on adds_new_state || replaces_new_state @
@@

 #include 

@ no_include depends on !include && (adds_new_state || replaces_new_state) @
@@

+ #include 
  #include 

Cc: "James (Qian) Wang" 
Cc: Liviu Dudau 
Cc: Mihail Atanassov 
Cc: Brian Starkey 
Cc: Russell King 
Cc: Paul Cercueil 
Cc: Chun-Kuang Hu 
Cc: Philipp Zabel 
Cc: Sandy Huang 
Cc: "Heiko Stübner" 
Cc: Thierry Reding 
Cc: Gerd Hoffmann 
Reviewed-by: Ville Syrjälä 
Suggested-by: Ville Syrjälä 
Signed-off-by: Maxime Ripard 

---

Changes from v1:
  - Fixed checkpatch warnings
---
 drivers/gpu/drm/arm/display/komeda/komeda_crtc.c |  4 +++-
 drivers/gpu/drm/armada/armada_crtc.c |  8 ++--
 drivers/gpu/drm/ast/ast_mode.c   |  4 +++-
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c|  7 +--
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 15 +--
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c  |  6 --
 drivers/gpu/drm/tegra/dc.c   |  8 +---
 drivers/gpu/drm/virtio/virtgpu_display.c |  4 +++-
 8 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index df0b9eeb8933..4b485eb512e2 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -387,10 +387,12 @@ static void
 komeda_crtc_atomic_flush(struct drm_crtc *crtc,
 struct drm_atomic_state *state)
 {
+   struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
+ crtc);
struct drm_crtc_state *old = drm_atomic_get_old_crtc_state(state,
   crtc);
/* commit with modeset will be handled in enable/disable */
-   if (drm_atomic_crtc_needs_modeset(crtc->state))
+   if (drm_atomic_crtc_needs_modeset(crtc_state))
return;
 
komeda_crtc_do_flush(crtc, old);
diff --git a/drivers/gpu/drm/armada/armada_crtc.c 
b/drivers/gpu/drm/armada/armada_crtc.c
index ca643f4e2064..3ebcf5a52c8b 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -431,11 +431,13 @@ static int armada_drm_crtc_atomic_check(struct drm_crtc 
*crtc,
 static void armada_drm_crtc_atomic_begin(struct drm_crtc *crtc,
 struct drm_atomic_state *state)
 {
+   struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
+ crtc);
struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
 
DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
 
-   if (crtc->state->color_mgmt_changed)

Re: [PATCH v2] drm: Use state helper instead of CRTC state pointer

2020-11-05 Thread Thomas Zimmermann 
Hi

Am 05.11.20 um 17:45 schrieb Maxime Ripard:
> Many drivers reference the crtc->pointer in order to get the current CRTC
> state in their atomic_begin or atomic_flush hooks, which would be the new
> CRTC state in the global atomic state since _swap_state happened when those
> hooks are run.
> 
> Use the drm_atomic_get_new_crtc_state helper to get that state to make it
> more obvious.
> 
> This was made using the coccinelle script below:
> 
> @ crtc_atomic_func @
> identifier helpers;
> identifier func;
> @@
> 
> (
> static struct drm_crtc_helper_funcs helpers = {
>   ...,
>   .atomic_begin = func,
>   ...,
> };
> |
> static struct drm_crtc_helper_funcs helpers = {
>   ...,
>   .atomic_flush = func,
>   ...,
> };
> )
> 
> @@
> identifier crtc_atomic_func.func;
> identifier crtc, state;
> symbol crtc_state;
> expression e;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
>   ...
> - struct tegra_dc_state *crtc_state = e;
> + struct tegra_dc_state *dc_state = e;
>   <+...
> -   crtc_state
> + dc_state
>   ...+>
>   }
> 
> @@
> identifier crtc_atomic_func.func;
> identifier crtc, state;
> symbol crtc_state;
> expression e;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
>   ...
> - struct mtk_crtc_state *crtc_state = e;
> + struct mtk_crtc_state *mtk_crtc_state = e;
>   <+...
> -   crtc_state
> + mtk_crtc_state
>   ...+>
>   }
> 
> @ replaces_new_state @
> identifier crtc_atomic_func.func;
> identifier crtc, state, crtc_state;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
>   ...
> - struct drm_crtc_state *crtc_state = crtc->state;
> + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, 
> crtc);
>   ...
>  }
> 
> @@
> identifier crtc_atomic_func.func;
> identifier crtc, state, crtc_state;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
>   struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, 
> crtc);
>   ...
> - crtc->state
> + crtc_state
>   ...
>  }
> 
> @ adds_new_state @
> identifier crtc_atomic_func.func;
> identifier crtc, state;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
> + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, 
> crtc);
>   ...
> - crtc->state
> + crtc_state
>   ...
>  }
> 
> @ include depends on adds_new_state || replaces_new_state @
> @@
> 
>  #include 
> 
> @ no_include depends on !include && (adds_new_state || replaces_new_state) @
> @@
> 
> + #include 
>   #include 
> 
> Cc: "James (Qian) Wang" 
> Cc: Liviu Dudau 
> Cc: Mihail Atanassov 
> Cc: Brian Starkey 
> Cc: Russell King 
> Cc: Paul Cercueil 
> Cc: Chun-Kuang Hu 
> Cc: Philipp Zabel 
> Cc: Sandy Huang 
> Cc: "Heiko Stübner" 
> Cc: Thierry Reding 
> Cc: Gerd Hoffmann 
> Reviewed-by: Ville Syrjälä 
> Suggested-by: Ville Syrjälä 
> Signed-off-by: Maxime Ripard 
Acked-by: Thomas Zimmermann 

> 
> ---
> 
> Changes from v1:
>   - Fixed checkpatch warnings
> ---
>  drivers/gpu/drm/arm/display/komeda/komeda_crtc.c |  4 +++-
>  drivers/gpu/drm/armada/armada_crtc.c |  8 ++--
>  drivers/gpu/drm/ast/ast_mode.c   |  4 +++-
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c|  7 +--
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 15 +--
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c  |  6 --
>  drivers/gpu/drm/tegra/dc.c   |  8 +---
>  drivers/gpu/drm/virtio/virtgpu_display.c |  4 +++-
>  8 files changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c 
> b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index df0b9eeb8933..4b485eb512e2 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -387,10 +387,12 @@ static void
>  komeda_crtc_atomic_flush(struct drm_crtc *crtc,
>struct drm_atomic_state *state)
>  {
> + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +   crtc);
>   struct drm_crtc_state *old = drm_atomic_get_old_crtc_state(state,
>  crtc);
>   /* commit with modeset will be handled in enable/disable */
> - if (drm_atomic_crtc_needs_modeset(crtc->state))
> + if (drm_atomic_crtc_needs_modeset(crtc_state))
>   return;
>  
>   komeda_crtc_do_flush(crtc, old);
> diff --git a/drivers/gpu/drm/armada/armada_crtc.c 
> b/drivers/gpu/drm/armada/armada_crtc.c
> index ca643f4e2064..3ebcf5a52c8b 100644
> --- a/drivers/gpu/drm/armada/armada_crtc.c
> +++ b/drivers/gpu/drm/armada/armada_crtc.c
> @@ -431,11 +431,13 @@ static int armada_drm_crtc_atomic_check(struct drm_crtc 
> *crtc,
>  static void armada_drm_crtc_atomic_begin(struct drm_crtc *crtc,
>