Re: [PATCH v3 02/11] drm: Rename plane atomic_check state names

2021-02-22 Thread Thomas Zimmermann 



Am 19.02.21 um 16:12 schrieb Maxime Ripard:

Hi Thomas,

Thanks for your review!

On Fri, Feb 19, 2021 at 03:49:22PM +0100, Thomas Zimmermann wrote:

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c 
b/drivers/gpu/drm/imx/ipuv3-plane.c
index 075508051b5f..1873a155bb26 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -337,12 +337,12 @@ static const struct drm_plane_funcs ipu_plane_funcs = {
   };
   static int ipu_plane_atomic_check(struct drm_plane *plane,
- struct drm_plane_state *state)
+ struct drm_plane_state *new_state)


This function uses a different naming convention then the others?


   {
struct drm_plane_state *old_state = plane->state;


So, the function already had a variable named old_state, so I was
actually trying to make the drivers consistent here: having one variable
with old_state and new_plane_state felt weird.

The heuristic is thus to use the convention of the driver if one exists
already, and if there's none pick new_plane_state.

It makes it indeed inconsistent across drivers, but it felt more natural
to be consistent within a single driver.


Makes sense.



Maxime



--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer



OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH v3 02/11] drm: Rename plane atomic_check state names

2021-02-19 Thread Maxime Ripard 
Hi Thomas,

Thanks for your review!

On Fri, Feb 19, 2021 at 03:49:22PM +0100, Thomas Zimmermann wrote:
> > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c 
> > b/drivers/gpu/drm/imx/ipuv3-plane.c
> > index 075508051b5f..1873a155bb26 100644
> > --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> > @@ -337,12 +337,12 @@ static const struct drm_plane_funcs ipu_plane_funcs = 
> > {
> >   };
> >   static int ipu_plane_atomic_check(struct drm_plane *plane,
> > - struct drm_plane_state *state)
> > + struct drm_plane_state *new_state)
> 
> This function uses a different naming convention then the others?
> 
> >   {
> > struct drm_plane_state *old_state = plane->state;

So, the function already had a variable named old_state, so I was
actually trying to make the drivers consistent here: having one variable
with old_state and new_plane_state felt weird.

The heuristic is thus to use the convention of the driver if one exists
already, and if there's none pick new_plane_state.

It makes it indeed inconsistent across drivers, but it felt more natural
to be consistent within a single driver.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 02/11] drm: Rename plane atomic_check state names

2021-02-19 Thread Thomas Zimmermann 



Am 19.02.21 um 13:00 schrieb Maxime Ripard:

Most drivers call the argument to the plane atomic_check hook simply
state, which is going to conflict with the global atomic state in a
later rework. Let's rename it to new_plane_state (or new_state depending
on the convention used in the driver).

This was done using the coccinelle script below, and built tested:

@ plane_atomic_func @
identifier helpers;
identifier func;
@@

  static const struct drm_plane_helper_funcs helpers = {
.atomic_check = func,
  };

@ has_old_state @
identifier plane_atomic_func.func;
identifier plane;
expression e;
symbol old_state;
symbol state;
@@

  func(struct drm_plane *plane, struct drm_plane_state *state)
  {
...
struct drm_plane_state *old_state = e;
...
  }

@ depends on has_old_state @
identifier plane_atomic_func.func;
identifier plane;
symbol old_state;
@@

  func(struct drm_plane *plane,
-   struct drm_plane_state *state
+   struct drm_plane_state *new_state
  )
  {
<+...
-   state
+   new_state
...+>
  }

@ has_state @
identifier plane_atomic_func.func;
identifier plane;
symbol state;
@@

  func(struct drm_plane *plane, struct drm_plane_state *state)
  {
...
  }

@ depends on has_state @
identifier plane_atomic_func.func;
identifier plane;
symbol old_state;
@@

  func(struct drm_plane *plane,
-   struct drm_plane_state *state
+   struct drm_plane_state *new_plane_state
  )
  {
<+...
-   state
+   new_plane_state
...+>
  }

Reviewed-by: Laurent Pinchart 
Signed-off-by: Maxime Ripard 

---

Changes from v1:
   - Updated the variable name in the comment in omapdrm
---
  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 13 +++---
  .../gpu/drm/arm/display/komeda/komeda_plane.c | 11 ++---
  drivers/gpu/drm/arm/hdlcd_crtc.c  | 18 
  drivers/gpu/drm/arm/malidp_planes.c   | 36 
  drivers/gpu/drm/armada/armada_plane.c | 41 ++-
  drivers/gpu/drm/ast/ast_mode.c| 26 ++--
  drivers/gpu/drm/exynos/exynos_drm_plane.c |  6 +--
  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c   |  6 +--
  .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c| 22 +-
  .../gpu/drm/hisilicon/kirin/kirin_drm_ade.c   | 24 +--
  drivers/gpu/drm/imx/dcss/dcss-plane.c | 26 ++--
  drivers/gpu/drm/imx/ipuv3-plane.c | 31 +++---
  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 27 ++--
  drivers/gpu/drm/ingenic/ingenic-ipu.c | 30 +++---
  drivers/gpu/drm/kmb/kmb_plane.c   | 22 +-
  drivers/gpu/drm/mediatek/mtk_drm_plane.c  | 16 
  drivers/gpu/drm/meson/meson_overlay.c | 10 +++--
  drivers/gpu/drm/meson/meson_plane.c   | 10 +++--
  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 35 
  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c|  9 ++--
  drivers/gpu/drm/nouveau/dispnv50/wndw.c   |  5 ++-
  drivers/gpu/drm/omapdrm/omap_plane.c  | 21 +-
  drivers/gpu/drm/qxl/qxl_display.c |  6 +--
  drivers/gpu/drm/rcar-du/rcar_du_plane.c   |  7 ++--
  drivers/gpu/drm/rcar-du/rcar_du_vsp.c |  7 ++--
  drivers/gpu/drm/rockchip/rockchip_drm_vop.c   | 27 ++--
  drivers/gpu/drm/sti/sti_cursor.c  | 22 +-
  drivers/gpu/drm/sti/sti_gdp.c | 26 ++--
  drivers/gpu/drm/sti/sti_hqvdp.c   | 24 +--
  drivers/gpu/drm/stm/ltdc.c| 10 ++---
  drivers/gpu/drm/sun4i/sun8i_ui_layer.c| 10 +++--
  drivers/gpu/drm/sun4i/sun8i_vi_layer.c| 10 +++--
  drivers/gpu/drm/tegra/dc.c| 38 -
  drivers/gpu/drm/tegra/hub.c   | 18 
  drivers/gpu/drm/tidss/tidss_plane.c   | 34 ---
  drivers/gpu/drm/tilcdc/tilcdc_plane.c | 24 +--
  drivers/gpu/drm/vc4/vc4_plane.c   | 10 ++---
  drivers/gpu/drm/virtio/virtgpu_plane.c|  9 ++--
  drivers/gpu/drm/vkms/vkms_plane.c | 11 ++---
  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c   | 13 +++---
  drivers/gpu/drm/xlnx/zynqmp_disp.c| 10 +++--
  41 files changed, 403 insertions(+), 358 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 63f839679a0a..906fa4ae25c9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6432,7 +6432,7 @@ static int dm_plane_helper_check_state(struct 
drm_plane_state *state,
  }
  
  static int dm_plane_atomic_check(struct drm_plane *plane,

-struct drm_plane_state *state)
+struct drm_plane_state *new_plane_state)
  {
struct amdgpu_device *adev = drm_to_adev(plane->dev);
struct dc *dc = adev->dm.dc;
@@

[PATCH v3 02/11] drm: Rename plane atomic_check state names

2021-02-19 Thread Maxime Ripard 
Most drivers call the argument to the plane atomic_check hook simply
state, which is going to conflict with the global atomic state in a
later rework. Let's rename it to new_plane_state (or new_state depending
on the convention used in the driver).

This was done using the coccinelle script below, and built tested:

@ plane_atomic_func @
identifier helpers;
identifier func;
@@

 static const struct drm_plane_helper_funcs helpers = {
.atomic_check = func,
 };

@ has_old_state @
identifier plane_atomic_func.func;
identifier plane;
expression e;
symbol old_state;
symbol state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *state)
 {
...
struct drm_plane_state *old_state = e;
...
 }

@ depends on has_old_state @
identifier plane_atomic_func.func;
identifier plane;
symbol old_state;
@@

 func(struct drm_plane *plane,
-   struct drm_plane_state *state
+   struct drm_plane_state *new_state
 )
 {
<+...
-   state
+   new_state
...+>
 }

@ has_state @
identifier plane_atomic_func.func;
identifier plane;
symbol state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *state)
 {
...
 }

@ depends on has_state @
identifier plane_atomic_func.func;
identifier plane;
symbol old_state;
@@

 func(struct drm_plane *plane,
-   struct drm_plane_state *state
+   struct drm_plane_state *new_plane_state
 )
 {
<+...
-   state
+   new_plane_state
...+>
 }

Reviewed-by: Laurent Pinchart 
Signed-off-by: Maxime Ripard 

---

Changes from v1:
  - Updated the variable name in the comment in omapdrm
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 13 +++---
 .../gpu/drm/arm/display/komeda/komeda_plane.c | 11 ++---
 drivers/gpu/drm/arm/hdlcd_crtc.c  | 18 
 drivers/gpu/drm/arm/malidp_planes.c   | 36 
 drivers/gpu/drm/armada/armada_plane.c | 41 ++-
 drivers/gpu/drm/ast/ast_mode.c| 26 ++--
 drivers/gpu/drm/exynos/exynos_drm_plane.c |  6 +--
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c   |  6 +--
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c| 22 +-
 .../gpu/drm/hisilicon/kirin/kirin_drm_ade.c   | 24 +--
 drivers/gpu/drm/imx/dcss/dcss-plane.c | 26 ++--
 drivers/gpu/drm/imx/ipuv3-plane.c | 31 +++---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 27 ++--
 drivers/gpu/drm/ingenic/ingenic-ipu.c | 30 +++---
 drivers/gpu/drm/kmb/kmb_plane.c   | 22 +-
 drivers/gpu/drm/mediatek/mtk_drm_plane.c  | 16 
 drivers/gpu/drm/meson/meson_overlay.c | 10 +++--
 drivers/gpu/drm/meson/meson_plane.c   | 10 +++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 35 
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c|  9 ++--
 drivers/gpu/drm/nouveau/dispnv50/wndw.c   |  5 ++-
 drivers/gpu/drm/omapdrm/omap_plane.c  | 21 +-
 drivers/gpu/drm/qxl/qxl_display.c |  6 +--
 drivers/gpu/drm/rcar-du/rcar_du_plane.c   |  7 ++--
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c |  7 ++--
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   | 27 ++--
 drivers/gpu/drm/sti/sti_cursor.c  | 22 +-
 drivers/gpu/drm/sti/sti_gdp.c | 26 ++--
 drivers/gpu/drm/sti/sti_hqvdp.c   | 24 +--
 drivers/gpu/drm/stm/ltdc.c| 10 ++---
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c| 10 +++--
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c| 10 +++--
 drivers/gpu/drm/tegra/dc.c| 38 -
 drivers/gpu/drm/tegra/hub.c   | 18 
 drivers/gpu/drm/tidss/tidss_plane.c   | 34 ---
 drivers/gpu/drm/tilcdc/tilcdc_plane.c | 24 +--
 drivers/gpu/drm/vc4/vc4_plane.c   | 10 ++---
 drivers/gpu/drm/virtio/virtgpu_plane.c|  9 ++--
 drivers/gpu/drm/vkms/vkms_plane.c | 11 ++---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c   | 13 +++---
 drivers/gpu/drm/xlnx/zynqmp_disp.c| 10 +++--
 41 files changed, 403 insertions(+), 358 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 63f839679a0a..906fa4ae25c9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6432,7 +6432,7 @@ static int dm_plane_helper_check_state(struct 
drm_plane_state *state,
 }
 
 static int dm_plane_atomic_check(struct drm_plane *plane,
-struct drm_plane_state *state)
+struct drm_plane_state *new_plane_state)
 {
struct amdgpu_device *adev = drm_to_adev(plane->dev);
struct dc *dc = adev->dm.dc;
@@ -6441,23 +6441,24 @@ static int dm_plane_atomic_check(struct drm_plane 
*plane,
struct drm_crtc_state