[PATCH 1/3] drm/nouveau/kms/nv50-: Use state helper instead of crtc pointer

[PATCH 1/3] drm/nouveau/kms/nv50-: Use state helper instead of crtc pointer

[Maxime Ripard]

dispnv50 references the crtc->state pointer in order to get the current
CRTC state in its atomic_check hook, which would be the old CRTC state in
the global atomic state.

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

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/nouveau/dispnv50/head.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c b/drivers/gpu/drm/nouveau/dispnv50/head.c
index 0542ca22b33a..537c1ef2e464 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/head.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/head.c
@@ -313,11 +313,13 @@ nv50_head_atomic_check_mode(struct nv50_head *head, struct nv50_head_atom *asyh)
 static int
 nv50_head_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
 {
+	struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state,
+									      crtc);
 	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
 									  crtc);
 	struct nouveau_drm *drm = nouveau_drm(crtc->dev);
 	struct nv50_head *head = nv50_head(crtc);
-	struct nv50_head_atom *armh = nv50_head_atom(crtc->state);
+	struct nv50_head_atom *armh = nv50_head_atom(old_crtc_state);
 	struct nv50_head_atom *asyh = nv50_head_atom(crtc_state);
 	struct nouveau_conn_atom *asyc = NULL;
 	struct drm_connector_state *conns;
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm: Use state helper instead of CRTC state pointer

[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 <drm/drm_atomic.h>

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

+ #include <drm/drm_atomic.h>
  #include <drm/...>

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 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)
+	if (crtc_state->color_mgmt_changed)
 		armada_drm_update_gamma(crtc);
 
 	dcrtc->regs_idx = 0;
@@ -445,6 +447,8 @@ static void armada_drm_crtc_atomic_begin(struct drm_crtc *crtc,
 static void armada_drm_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 armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
 
 	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
@@ -455,7 +459,7 @@ static void armada_drm_crtc_atomic_flush(struct drm_crtc *crtc,
 	 * If we aren't doing a full modeset, then we need to queue
 	 * the event here.
 	 */
-	if (!drm_atomic_crtc_needs_modeset(crtc->state)) {
+	if (!drm_atomic_crtc_needs_modeset(crtc_state)) {
 		dcrtc->update_pending = true;
 		armada_drm_crtc_queue_state_event(crtc);
 		spin_lock_irq(&dcrtc->irq_lock);
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 22f0e65fbe9a..87c5c5f35f10 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -782,10 +782,12 @@ static void
 ast_crtc_helper_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_crtc_state = drm_atomic_get_old_crtc_state(state,
 									      crtc);
 	struct ast_private *ast = to_ast_private(crtc->dev);
-	struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc->state);
+	struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state);
 	struct ast_crtc_state *old_ast_crtc_state = to_ast_crtc_state(old_crtc_state);
 
 	/*
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index b9c156e13156..7603f86dd0d1 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -305,11 +305,13 @@ ingenic_drm_crtc_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode
 static void ingenic_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 ingenic_drm *priv = drm_crtc_get_priv(crtc);
 	u32 ctrl = 0;
 
 	if (priv->soc_info->has_osd &&
-	    drm_atomic_crtc_needs_modeset(crtc->state)) {
+	    drm_atomic_crtc_needs_modeset(crtc_state)) {
 		/*
 		 * If IPU plane is enabled, enable IPU as source for the F1
 		 * plane; otherwise use regular DMA.
@@ -326,7 +328,8 @@ static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
 					  struct drm_atomic_state *state)
 {
 	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
-	struct drm_crtc_state *crtc_state = crtc->state;
+	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
+									  crtc);
 	struct drm_pending_vblank_event *event = crtc_state->event;
 
 	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 23f5c10b0c67..193848fd7515 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -11,6 +11,7 @@
 #include <asm/barrier.h>
 #include <soc/mediatek/smi.h>
 
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_probe_helper.h>
@@ -577,17 +578,19 @@ static void mtk_drm_crtc_atomic_disable(struct drm_crtc *crtc,
 static void mtk_drm_crtc_atomic_begin(struct drm_crtc *crtc,
 				      struct drm_atomic_state *state)
 {
-	struct mtk_crtc_state *crtc_state = to_mtk_crtc_state(crtc->state);
+	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
+									  crtc);
+	struct mtk_crtc_state *mtk_crtc_state = to_mtk_crtc_state(crtc_state);
 	struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
 
-	if (mtk_crtc->event && crtc_state->base.event)
+	if (mtk_crtc->event && mtk_crtc_state->base.event)
 		DRM_ERROR("new event while there is still a pending event\n");
 
-	if (crtc_state->base.event) {
-		crtc_state->base.event->pipe = drm_crtc_index(crtc);
+	if (mtk_crtc_state->base.event) {
+		mtk_crtc_state->base.event->pipe = drm_crtc_index(crtc);
 		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
-		mtk_crtc->event = crtc_state->base.event;
-		crtc_state->base.event = NULL;
+		mtk_crtc->event = mtk_crtc_state->base.event;
+		mtk_crtc_state->base.event = NULL;
 	}
 }
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 8cd39fca81a3..f820654b2492 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1248,6 +1248,8 @@ static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
 static void vop_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 drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state,
 									      crtc);
 	struct vop *vop = to_vop(crtc);
@@ -1256,8 +1258,8 @@ static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
 	 * Only update GAMMA if the 'active' flag is not changed,
 	 * otherwise it's updated by .atomic_enable.
 	 */
-	if (crtc->state->color_mgmt_changed &&
-	    !crtc->state->active_changed)
+	if (crtc_state->color_mgmt_changed &&
+	    !crtc_state->active_changed)
 		vop_crtc_gamma_set(vop, crtc, old_crtc_state);
 }
 
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 2d86627b0d4e..85dd7131553a 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1939,15 +1939,17 @@ static void tegra_crtc_atomic_begin(struct drm_crtc *crtc,
 static void tegra_crtc_atomic_flush(struct drm_crtc *crtc,
 				    struct drm_atomic_state *state)
 {
-	struct tegra_dc_state *crtc_state = to_dc_state(crtc->state);
+	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
+									  crtc);
+	struct tegra_dc_state *dc_state = to_dc_state(crtc_state);
 	struct tegra_dc *dc = to_tegra_dc(crtc);
 	u32 value;
 
-	value = crtc_state->planes << 8 | GENERAL_UPDATE;
+	value = dc_state->planes << 8 | GENERAL_UPDATE;
 	tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
 	value = tegra_dc_readl(dc, DC_CMD_STATE_CONTROL);
 
-	value = crtc_state->planes | GENERAL_ACT_REQ;
+	value = dc_state->planes | GENERAL_ACT_REQ;
 	tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
 	value = tegra_dc_readl(dc, DC_CMD_STATE_CONTROL);
 }
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
index 4bf74836bd53..a6caebd4a0dd 100644
--- a/drivers/gpu/drm/virtio/virtgpu_display.c
+++ b/drivers/gpu/drm/virtio/virtgpu_display.c
@@ -119,6 +119,8 @@ static int virtio_gpu_crtc_atomic_check(struct drm_crtc *crtc,
 static void virtio_gpu_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 virtio_gpu_output *output = drm_crtc_to_virtio_gpu_output(crtc);
 
 	/*
@@ -127,7 +129,7 @@ static void virtio_gpu_crtc_atomic_flush(struct drm_crtc *crtc,
 	 * in the plane update callback, and here we just check
 	 * whenever we must force the modeset.
 	 */
-	if (drm_atomic_crtc_needs_modeset(crtc->state)) {
+	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
 		output->needs_modeset = true;
 	}
 }
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm: Use the state pointer directly in atomic_check

[Maxime Ripard]

Now that atomic_check takes the global atomic state as a parameter, we
don't need to go through the pointer in the CRTC state.

This was done using the following coccinelle script:

@ crtc_atomic_func @
identifier helpers;
identifier func;
@@

static struct drm_crtc_helper_funcs helpers = {
	...,
	.atomic_check = func,
	...,
};

@@
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);
  ... when != crtc_state
- crtc_state->state
+ state
  ...
 }

@@
struct drm_crtc_state *crtc_state;
identifier crtc_atomic_func.func;
identifier crtc, state;
@@

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

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/drm_simple_kms_helper.c | 2 +-
 drivers/gpu/drm/mxsfb/mxsfb_kms.c       | 2 +-
 drivers/gpu/drm/omapdrm/omap_crtc.c     | 2 +-
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c    | 6 +++---
 drivers/gpu/drm/vc4/vc4_crtc.c          | 2 +-
 drivers/gpu/drm/xlnx/zynqmp_disp.c      | 4 +---
 6 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index 4b46689634dd..743e57c1b44f 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -97,7 +97,7 @@ static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
 	if (has_primary != crtc_state->enable)
 		return -EINVAL;
 
-	return drm_atomic_add_affected_planes(crtc_state->state, crtc);
+	return drm_atomic_add_affected_planes(state, crtc);
 }
 
 static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index eb0e2b08329b..9040835289a8 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -281,7 +281,7 @@ static int mxsfb_crtc_atomic_check(struct drm_crtc *crtc,
 		return -EINVAL;
 
 	/* TODO: Is this needed ? */
-	return drm_atomic_add_affected_planes(crtc_state->state, crtc);
+	return drm_atomic_add_affected_planes(state, crtc);
 }
 
 static void mxsfb_crtc_atomic_flush(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index d7442aa55f89..49621b2e1ab5 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -583,7 +583,7 @@ static int omap_crtc_atomic_check(struct drm_crtc *crtc,
 			return -EINVAL;
 	}
 
-	pri_state = drm_atomic_get_new_plane_state(crtc_state->state,
+	pri_state = drm_atomic_get_new_plane_state(state,
 						   crtc->primary);
 	if (pri_state) {
 		struct omap_crtc_state *omap_crtc_state =
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 40c59f4bd962..30213708fc99 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -665,9 +665,9 @@ static int tilcdc_crtc_atomic_check(struct drm_crtc *crtc,
 	if (!crtc_state->active)
 		return 0;
 
-	if (crtc_state->state->planes[0].ptr != crtc->primary ||
-	    crtc_state->state->planes[0].state == NULL ||
-	    crtc_state->state->planes[0].state->crtc != crtc) {
+	if (state->planes[0].ptr != crtc->primary ||
+	    state->planes[0].state == NULL ||
+	    state->planes[0].state->crtc != crtc) {
 		dev_dbg(crtc->dev->dev, "CRTC primary plane must be present");
 		return -EINVAL;
 	}
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 06088854c647..ea710beb8e00 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -597,7 +597,7 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
-	for_each_new_connector_in_state(crtc_state->state, conn, conn_state,
+	for_each_new_connector_in_state(state, conn, conn_state,
 					i) {
 		if (conn_state->crtc != crtc)
 			continue;
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 444865af9e36..c685d94409b0 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -1506,9 +1506,7 @@ zynqmp_disp_crtc_atomic_disable(struct drm_crtc *crtc,
 static int zynqmp_disp_crtc_atomic_check(struct drm_crtc *crtc,
 					 struct drm_atomic_state *state)
 {
-	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
-									  crtc);
-	return drm_atomic_add_affected_planes(crtc_state->state, crtc);
+	return drm_atomic_add_affected_planes(state, crtc);
 }
 
 static void
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: Use state helper instead of CRTC state pointer

[Ville Syrjälä]

On Mon, Nov 02, 2020 at 02:38:33PM +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
>   ...+>
>   }

These reanames seem a bit out of scpe for this patch. But I guess you
needed then to get the rest of the cocci to work on some drivers?

The basic idea looks good:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

But I guess up to the individual driver folks to bikeshed the variable
naming and whatnot.

One thing I spotted is that a few drivers now gained two aliasing crtc
state pointers in the function: one with the drm type, the other with
a driver specific type. That's something we've outlawed in i915 since
it was making life rather confusing. In i915 we now prefer to use only
the i915 specific types in most places.

> 
> @ 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 <drm/drm_atomic.h>
> 
> @ no_include depends on !include && (adds_new_state || replaces_new_state) @
> @@
> 
> + #include <drm/drm_atomic.h>
>   #include <drm/...>
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  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)
> +	if (crtc_state->color_mgmt_changed)
>  		armada_drm_update_gamma(crtc);
>  
>  	dcrtc->regs_idx = 0;
> @@ -445,6 +447,8 @@ static void armada_drm_crtc_atomic_begin(struct drm_crtc *crtc,
>  static void armada_drm_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 armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
>  
>  	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
> @@ -455,7 +459,7 @@ static void armada_drm_crtc_atomic_flush(struct drm_crtc *crtc,
>  	 * If we aren't doing a full modeset, then we need to queue
>  	 * the event here.
>  	 */
> -	if (!drm_atomic_crtc_needs_modeset(crtc->state)) {
> +	if (!drm_atomic_crtc_needs_modeset(crtc_state)) {
>  		dcrtc->update_pending = true;
>  		armada_drm_crtc_queue_state_event(crtc);
>  		spin_lock_irq(&dcrtc->irq_lock);
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 22f0e65fbe9a..87c5c5f35f10 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -782,10 +782,12 @@ static void
>  ast_crtc_helper_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_crtc_state = drm_atomic_get_old_crtc_state(state,
>  									      crtc);
>  	struct ast_private *ast = to_ast_private(crtc->dev);
> -	struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc->state);
> +	struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state);
>  	struct ast_crtc_state *old_ast_crtc_state = to_ast_crtc_state(old_crtc_state);
>  
>  	/*
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index b9c156e13156..7603f86dd0d1 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -305,11 +305,13 @@ ingenic_drm_crtc_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode
>  static void ingenic_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 ingenic_drm *priv = drm_crtc_get_priv(crtc);
>  	u32 ctrl = 0;
>  
>  	if (priv->soc_info->has_osd &&
> -	    drm_atomic_crtc_needs_modeset(crtc->state)) {
> +	    drm_atomic_crtc_needs_modeset(crtc_state)) {
>  		/*
>  		 * If IPU plane is enabled, enable IPU as source for the F1
>  		 * plane; otherwise use regular DMA.
> @@ -326,7 +328,8 @@ static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
>  					  struct drm_atomic_state *state)
>  {
>  	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> -	struct drm_crtc_state *crtc_state = crtc->state;
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +									  crtc);
>  	struct drm_pending_vblank_event *event = crtc_state->event;
>  
>  	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 23f5c10b0c67..193848fd7515 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -11,6 +11,7 @@
>  #include <asm/barrier.h>
>  #include <soc/mediatek/smi.h>
>  
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_probe_helper.h>
> @@ -577,17 +578,19 @@ static void mtk_drm_crtc_atomic_disable(struct drm_crtc *crtc,
>  static void mtk_drm_crtc_atomic_begin(struct drm_crtc *crtc,
>  				      struct drm_atomic_state *state)
>  {
> -	struct mtk_crtc_state *crtc_state = to_mtk_crtc_state(crtc->state);
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +									  crtc);
> +	struct mtk_crtc_state *mtk_crtc_state = to_mtk_crtc_state(crtc_state);
>  	struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
>  
> -	if (mtk_crtc->event && crtc_state->base.event)
> +	if (mtk_crtc->event && mtk_crtc_state->base.event)
>  		DRM_ERROR("new event while there is still a pending event\n");
>  
> -	if (crtc_state->base.event) {
> -		crtc_state->base.event->pipe = drm_crtc_index(crtc);
> +	if (mtk_crtc_state->base.event) {
> +		mtk_crtc_state->base.event->pipe = drm_crtc_index(crtc);
>  		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> -		mtk_crtc->event = crtc_state->base.event;
> -		crtc_state->base.event = NULL;
> +		mtk_crtc->event = mtk_crtc_state->base.event;
> +		mtk_crtc_state->base.event = NULL;
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 8cd39fca81a3..f820654b2492 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1248,6 +1248,8 @@ static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
>  static void vop_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 drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state,
>  									      crtc);
>  	struct vop *vop = to_vop(crtc);
> @@ -1256,8 +1258,8 @@ static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
>  	 * Only update GAMMA if the 'active' flag is not changed,
>  	 * otherwise it's updated by .atomic_enable.
>  	 */
> -	if (crtc->state->color_mgmt_changed &&
> -	    !crtc->state->active_changed)
> +	if (crtc_state->color_mgmt_changed &&
> +	    !crtc_state->active_changed)
>  		vop_crtc_gamma_set(vop, crtc, old_crtc_state);
>  }
>  
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 2d86627b0d4e..85dd7131553a 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -1939,15 +1939,17 @@ static void tegra_crtc_atomic_begin(struct drm_crtc *crtc,
>  static void tegra_crtc_atomic_flush(struct drm_crtc *crtc,
>  				    struct drm_atomic_state *state)
>  {
> -	struct tegra_dc_state *crtc_state = to_dc_state(crtc->state);
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +									  crtc);
> +	struct tegra_dc_state *dc_state = to_dc_state(crtc_state);
>  	struct tegra_dc *dc = to_tegra_dc(crtc);
>  	u32 value;
>  
> -	value = crtc_state->planes << 8 | GENERAL_UPDATE;
> +	value = dc_state->planes << 8 | GENERAL_UPDATE;
>  	tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
>  	value = tegra_dc_readl(dc, DC_CMD_STATE_CONTROL);
>  
> -	value = crtc_state->planes | GENERAL_ACT_REQ;
> +	value = dc_state->planes | GENERAL_ACT_REQ;
>  	tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
>  	value = tegra_dc_readl(dc, DC_CMD_STATE_CONTROL);
>  }
> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
> index 4bf74836bd53..a6caebd4a0dd 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_display.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c
> @@ -119,6 +119,8 @@ static int virtio_gpu_crtc_atomic_check(struct drm_crtc *crtc,
>  static void virtio_gpu_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 virtio_gpu_output *output = drm_crtc_to_virtio_gpu_output(crtc);
>  
>  	/*
> @@ -127,7 +129,7 @@ static void virtio_gpu_crtc_atomic_flush(struct drm_crtc *crtc,
>  	 * in the plane update callback, and here we just check
>  	 * whenever we must force the modeset.
>  	 */
> -	if (drm_atomic_crtc_needs_modeset(crtc->state)) {
> +	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>  		output->needs_modeset = true;
>  	}
>  }
> -- 
> 2.28.0

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm: Use the state pointer directly in atomic_check

[Ville Syrjälä]

On Mon, Nov 02, 2020 at 02:38:34PM +0100, Maxime Ripard wrote:
> Now that atomic_check takes the global atomic state as a parameter, we
> don't need to go through the pointer in the CRTC state.
> 
> This was done using the following coccinelle script:
> 
> @ crtc_atomic_func @
> identifier helpers;
> identifier func;
> @@
> 
> static struct drm_crtc_helper_funcs helpers = {
> 	...,
> 	.atomic_check = func,
> 	...,
> };
> 
> @@
> 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);
>   ... when != crtc_state
> - crtc_state->state
> + state
>   ...
>  }
> 
> @@
> struct drm_crtc_state *crtc_state;
> identifier crtc_atomic_func.func;
> identifier crtc, state;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
>   ...
> - crtc_state->state
> + state
>   ...
>  }
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

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

> ---
>  drivers/gpu/drm/drm_simple_kms_helper.c | 2 +-
>  drivers/gpu/drm/mxsfb/mxsfb_kms.c       | 2 +-
>  drivers/gpu/drm/omapdrm/omap_crtc.c     | 2 +-
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c    | 6 +++---
>  drivers/gpu/drm/vc4/vc4_crtc.c          | 2 +-
>  drivers/gpu/drm/xlnx/zynqmp_disp.c      | 4 +---
>  6 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 4b46689634dd..743e57c1b44f 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -97,7 +97,7 @@ static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
>  	if (has_primary != crtc_state->enable)
>  		return -EINVAL;
>  
> -	return drm_atomic_add_affected_planes(crtc_state->state, crtc);
> +	return drm_atomic_add_affected_planes(state, crtc);
>  }
>  
>  static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> index eb0e2b08329b..9040835289a8 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> @@ -281,7 +281,7 @@ static int mxsfb_crtc_atomic_check(struct drm_crtc *crtc,
>  		return -EINVAL;
>  
>  	/* TODO: Is this needed ? */
> -	return drm_atomic_add_affected_planes(crtc_state->state, crtc);
> +	return drm_atomic_add_affected_planes(state, crtc);
>  }
>  
>  static void mxsfb_crtc_atomic_flush(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index d7442aa55f89..49621b2e1ab5 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -583,7 +583,7 @@ static int omap_crtc_atomic_check(struct drm_crtc *crtc,
>  			return -EINVAL;
>  	}
>  
> -	pri_state = drm_atomic_get_new_plane_state(crtc_state->state,
> +	pri_state = drm_atomic_get_new_plane_state(state,
>  						   crtc->primary);
>  	if (pri_state) {
>  		struct omap_crtc_state *omap_crtc_state =
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 40c59f4bd962..30213708fc99 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -665,9 +665,9 @@ static int tilcdc_crtc_atomic_check(struct drm_crtc *crtc,
>  	if (!crtc_state->active)
>  		return 0;
>  
> -	if (crtc_state->state->planes[0].ptr != crtc->primary ||
> -	    crtc_state->state->planes[0].state == NULL ||
> -	    crtc_state->state->planes[0].state->crtc != crtc) {
> +	if (state->planes[0].ptr != crtc->primary ||
> +	    state->planes[0].state == NULL ||
> +	    state->planes[0].state->crtc != crtc) {
>  		dev_dbg(crtc->dev->dev, "CRTC primary plane must be present");
>  		return -EINVAL;
>  	}
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 06088854c647..ea710beb8e00 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -597,7 +597,7 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
>  	if (ret)
>  		return ret;
>  
> -	for_each_new_connector_in_state(crtc_state->state, conn, conn_state,
> +	for_each_new_connector_in_state(state, conn, conn_state,
>  					i) {
>  		if (conn_state->crtc != crtc)
>  			continue;
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index 444865af9e36..c685d94409b0 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -1506,9 +1506,7 @@ zynqmp_disp_crtc_atomic_disable(struct drm_crtc *crtc,
>  static int zynqmp_disp_crtc_atomic_check(struct drm_crtc *crtc,
>  					 struct drm_atomic_state *state)
>  {
> -	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> -									  crtc);
> -	return drm_atomic_add_affected_planes(crtc_state->state, crtc);
> +	return drm_atomic_add_affected_planes(state, crtc);
>  }
>  
>  static void
> -- 
> 2.28.0

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/nouveau/kms/nv50-: Use state helper instead of crtc pointer

[Ville Syrjälä]

On Mon, Nov 02, 2020 at 02:38:32PM +0100, Maxime Ripard wrote:
> dispnv50 references the crtc->state pointer in order to get the current
> CRTC state in its atomic_check hook, which would be the old CRTC state in
> the global atomic state.
> 
> Use the drm_atomic_get_old_crtc_state helper to get that state to make it
> more obvious.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Looks correct
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/nouveau/dispnv50/head.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c b/drivers/gpu/drm/nouveau/dispnv50/head.c
> index 0542ca22b33a..537c1ef2e464 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/head.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/head.c
> @@ -313,11 +313,13 @@ nv50_head_atomic_check_mode(struct nv50_head *head, struct nv50_head_atom *asyh)
>  static int
>  nv50_head_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
>  {
> +	struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state,
> +									      crtc);
>  	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
>  									  crtc);
>  	struct nouveau_drm *drm = nouveau_drm(crtc->dev);
>  	struct nv50_head *head = nv50_head(crtc);
> -	struct nv50_head_atom *armh = nv50_head_atom(crtc->state);
> +	struct nv50_head_atom *armh = nv50_head_atom(old_crtc_state);
>  	struct nv50_head_atom *asyh = nv50_head_atom(crtc_state);
>  	struct nouveau_conn_atom *asyc = NULL;
>  	struct drm_connector_state *conns;
> -- 
> 2.28.0

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/nouveau/kms/nv50-: Use state helper instead of crtc pointer

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 656 bytes --]

On Mon, Nov 02, 2020 at 06:06:30PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 02, 2020 at 02:38:32PM +0100, Maxime Ripard wrote:
> > dispnv50 references the crtc->state pointer in order to get the current
> > CRTC state in its atomic_check hook, which would be the old CRTC state in
> > the global atomic state.
> > 
> > Use the drm_atomic_get_old_crtc_state helper to get that state to make it
> > more obvious.
> > 
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> Looks correct
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Applied, thanks!
Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm: Use the state pointer directly in atomic_check

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1385 bytes --]

On Mon, Nov 02, 2020 at 06:06:04PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 02, 2020 at 02:38:34PM +0100, Maxime Ripard wrote:
> > Now that atomic_check takes the global atomic state as a parameter, we
> > don't need to go through the pointer in the CRTC state.
> > 
> > This was done using the following coccinelle script:
> > 
> > @ crtc_atomic_func @
> > identifier helpers;
> > identifier func;
> > @@
> > 
> > static struct drm_crtc_helper_funcs helpers = {
> > 	...,
> > 	.atomic_check = func,
> > 	...,
> > };
> > 
> > @@
> > 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);
> >   ... when != crtc_state
> > - crtc_state->state
> > + state
> >   ...
> >  }
> > 
> > @@
> > struct drm_crtc_state *crtc_state;
> > identifier crtc_atomic_func.func;
> > identifier crtc, state;
> > @@
> > 
> >   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
> >   ...
> > - crtc_state->state
> > + state
> >   ...
> >  }
> > 
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> lgtm
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Applied, thanks!
Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: Use state helper instead of CRTC state pointer

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2780 bytes --]

Hi Ville,

On Mon, Nov 02, 2020 at 06:04:06PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 02, 2020 at 02:38:33PM +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
> >   ...+>
> >   }
> 
> These reanames seem a bit out of scpe for this patch. But I guess you
> needed then to get the rest of the cocci to work on some drivers?

Yeah, those two drivers already had a variable named crtc_state, calling
container_of on crtc->state

It was cleaner to me to have an intermediate variable storing the result
of drm_atomic_get_new_crtc_state, but then the most obvious name was
taken so I had to rename those two variables before doing so.

> The basic idea looks good:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> But I guess up to the individual driver folks to bikeshed the variable
> naming and whatnot.
> 
> One thing I spotted is that a few drivers now gained two aliasing crtc
> state pointers in the function: one with the drm type, the other with
> a driver specific type. That's something we've outlawed in i915 since
> it was making life rather confusing. In i915 we now prefer to use only
> the i915 specific types in most places.

I didn't spot any of those cases, do you have an example of where it
happened?

Thanks!
Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: Use state helper instead of CRTC state pointer

[Ville Syrjälä]

On Tue, Nov 03, 2020 at 05:15:51PM +0100, Maxime Ripard wrote:
> Hi Ville,
> 
> On Mon, Nov 02, 2020 at 06:04:06PM +0200, Ville Syrjälä wrote:
> > On Mon, Nov 02, 2020 at 02:38:33PM +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
> > >   ...+>
> > >   }
> > 
> > These reanames seem a bit out of scpe for this patch. But I guess you
> > needed then to get the rest of the cocci to work on some drivers?
> 
> Yeah, those two drivers already had a variable named crtc_state, calling
> container_of on crtc->state
> 
> It was cleaner to me to have an intermediate variable storing the result
> of drm_atomic_get_new_crtc_state, but then the most obvious name was
> taken so I had to rename those two variables before doing so.
> 
> > The basic idea looks good:
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > But I guess up to the individual driver folks to bikeshed the variable
> > naming and whatnot.
> > 
> > One thing I spotted is that a few drivers now gained two aliasing crtc
> > state pointers in the function: one with the drm type, the other with
> > a driver specific type. That's something we've outlawed in i915 since
> > it was making life rather confusing. In i915 we now prefer to use only
> > the i915 specific types in most places.
> 
> I didn't spot any of those cases, do you have an example of where it
> happened?

eg. ast:
+       struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,                               
+                                                                         crtc);                               
        struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state,                           
                                                                              crtc);                           
        struct ast_private *ast = to_ast_private(crtc->dev);                                                   
-       struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc->state);                                
+       struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state);       

So here both 'crtc_state' and 'ast_crtc_state' are basically the same
thing, which can get a bit confusing especially within larger functions
with lots of variables. 

In i915 we would just have
struct intel_crtc_state *crtc_state = whatever;
and any access to the base class would be done via crtc_state->base.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: Use state helper instead of CRTC state pointer

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 4552 bytes --]

On Tue, Nov 03, 2020 at 06:28:24PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 03, 2020 at 05:15:51PM +0100, Maxime Ripard wrote:
> > Hi Ville,
> > 
> > On Mon, Nov 02, 2020 at 06:04:06PM +0200, Ville Syrjälä wrote:
> > > On Mon, Nov 02, 2020 at 02:38:33PM +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
> > > >   ...+>
> > > >   }
> > > 
> > > These reanames seem a bit out of scpe for this patch. But I guess you
> > > needed then to get the rest of the cocci to work on some drivers?
> > 
> > Yeah, those two drivers already had a variable named crtc_state, calling
> > container_of on crtc->state
> > 
> > It was cleaner to me to have an intermediate variable storing the result
> > of drm_atomic_get_new_crtc_state, but then the most obvious name was
> > taken so I had to rename those two variables before doing so.
> > 
> > > The basic idea looks good:
> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > But I guess up to the individual driver folks to bikeshed the variable
> > > naming and whatnot.
> > > 
> > > One thing I spotted is that a few drivers now gained two aliasing crtc
> > > state pointers in the function: one with the drm type, the other with
> > > a driver specific type. That's something we've outlawed in i915 since
> > > it was making life rather confusing. In i915 we now prefer to use only
> > > the i915 specific types in most places.
> > 
> > I didn't spot any of those cases, do you have an example of where it
> > happened?
> 
> eg. ast:
> +       struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,                               
> +                                                                         crtc);                               
>         struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state,                           
>                                                                               crtc);                           
>         struct ast_private *ast = to_ast_private(crtc->dev);                                                   
> -       struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc->state);                                
> +       struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state);       
> 
> So here both 'crtc_state' and 'ast_crtc_state' are basically the same
> thing, which can get a bit confusing especially within larger functions
> with lots of variables. 

Ah, that kind of aliasing, ok. So it's purely an issue with
ergonomics/convenience?

It seems to be a widespread pattern (I know we're using it in vc4 and
sun4i, and from those reworks I've seen a number of drivers doing so).
I'm not entirely sure how we should fix it through coccinelle too :/

Thanks!
Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: Use state helper instead of CRTC state pointer

[Ville Syrjälä]

On Thu, Nov 05, 2020 at 05:35:28PM +0100, Maxime Ripard wrote:
> On Tue, Nov 03, 2020 at 06:28:24PM +0200, Ville Syrjälä wrote:
> > On Tue, Nov 03, 2020 at 05:15:51PM +0100, Maxime Ripard wrote:
> > > Hi Ville,
> > > 
> > > On Mon, Nov 02, 2020 at 06:04:06PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Nov 02, 2020 at 02:38:33PM +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
> > > > >   ...+>
> > > > >   }
> > > > 
> > > > These reanames seem a bit out of scpe for this patch. But I guess you
> > > > needed then to get the rest of the cocci to work on some drivers?
> > > 
> > > Yeah, those two drivers already had a variable named crtc_state, calling
> > > container_of on crtc->state
> > > 
> > > It was cleaner to me to have an intermediate variable storing the result
> > > of drm_atomic_get_new_crtc_state, but then the most obvious name was
> > > taken so I had to rename those two variables before doing so.
> > > 
> > > > The basic idea looks good:
> > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > But I guess up to the individual driver folks to bikeshed the variable
> > > > naming and whatnot.
> > > > 
> > > > One thing I spotted is that a few drivers now gained two aliasing crtc
> > > > state pointers in the function: one with the drm type, the other with
> > > > a driver specific type. That's something we've outlawed in i915 since
> > > > it was making life rather confusing. In i915 we now prefer to use only
> > > > the i915 specific types in most places.
> > > 
> > > I didn't spot any of those cases, do you have an example of where it
> > > happened?
> > 
> > eg. ast:
> > +       struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,                               
> > +                                                                         crtc);                               
> >         struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state,                           
> >                                                                               crtc);                           
> >         struct ast_private *ast = to_ast_private(crtc->dev);                                                   
> > -       struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc->state);                                
> > +       struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state);       
> > 
> > So here both 'crtc_state' and 'ast_crtc_state' are basically the same
> > thing, which can get a bit confusing especially within larger functions
> > with lots of variables. 
> 
> Ah, that kind of aliasing, ok. So it's purely an issue with
> ergonomics/convenience?

Yeah, I got fed up with it in i915 because I had a hard time
reasoning how the state was being mutated when it was being
accesed through multiple pointers in the same function. And
didn't help that the variable names were all over the place.

> 
> It seems to be a widespread pattern (I know we're using it in vc4 and
> sun4i, and from those reworks I've seen a number of drivers doing so).
> I'm not entirely sure how we should fix it through coccinelle too :/

I think we've been doing these by hand in i915 for the most part.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel