[PATCH RESEND v4 0/2] Fix OVL iommu fault in cursor plane

[PATCH RESEND v4 0/2] Fix OVL iommu fault in cursor plane

[Jason-JH.Lin]

Fix some IGT tests fail at iommu fault in OVL cursor plane.

Change in RESEND v4 :
1. Remove redundant plane_state assigning.

Change in v4:
1. Change disable all layer method to update mtk_plane_state stored
   in mtk_crtc by drm_atomic_state from mtk_drm_crtc_atomic_enable().

Change in v3:
1. Add Fixes tag before s.o.b.

Change in v2:
1. Add Fixes tag.

Jason-JH.Lin (2):
  drm/mediatek: Fix iommu fault by swapping FBs after updating plane
    state
  drm/mediatek: Fix iommu fault during crtc enabling

 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 13 +++++++++----
 drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
 3 files changed, 21 insertions(+), 8 deletions(-)

-- 
2.18.0

[PATCH RESEND v4 0/2] Fix OVL iommu fault in cursor plane

[Jason-JH.Lin]

Fix some IGT tests fail at iommu fault in OVL cursor plane.

Change in RESEND v4 :
1. Remove redundant plane_state assigning.

Change in v4:
1. Change disable all layer method to update mtk_plane_state stored
   in mtk_crtc by drm_atomic_state from mtk_drm_crtc_atomic_enable().

Change in v3:
1. Add Fixes tag before s.o.b.

Change in v2:
1. Add Fixes tag.

Jason-JH.Lin (2):
  drm/mediatek: Fix iommu fault by swapping FBs after updating plane
    state
  drm/mediatek: Fix iommu fault during crtc enabling

 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 13 +++++++++----
 drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
 3 files changed, 21 insertions(+), 8 deletions(-)

-- 
2.18.0

[PATCH RESEND v4 0/2] Fix OVL iommu fault in cursor plane

[Jason-JH.Lin]

Fix some IGT tests fail at iommu fault in OVL cursor plane.

Change in RESEND v4 :
1. Remove redundant plane_state assigning.

Change in v4:
1. Change disable all layer method to update mtk_plane_state stored
   in mtk_crtc by drm_atomic_state from mtk_drm_crtc_atomic_enable().

Change in v3:
1. Add Fixes tag before s.o.b.

Change in v2:
1. Add Fixes tag.

Jason-JH.Lin (2):
  drm/mediatek: Fix iommu fault by swapping FBs after updating plane
    state
  drm/mediatek: Fix iommu fault during crtc enabling

 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 13 +++++++++----
 drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
 3 files changed, 21 insertions(+), 8 deletions(-)

-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RESEND v4 1/2] drm/mediatek: Fix iommu fault by swapping FBs after updating plane state

[Jason-JH.Lin]

According to the comment in drm_atomic_helper_async_commit(),
we should make sure FBs have been swapped, so that cleanups in the
new_state performs a cleanup in the old FB.

So we should move swapping FBs after calling mtk_plane_update_new_state(),
to avoid using the old FB which could be freed.

Fixes: 1a64a7aff8da ("drm/mediatek: Fix cursor plane no update")
Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index 31f9420aff6f..b1a918ffe457 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -210,9 +210,9 @@ static void mtk_plane_atomic_async_update(struct drm_plane *plane,
 	plane->state->src_y = new_state->src_y;
 	plane->state->src_h = new_state->src_h;
 	plane->state->src_w = new_state->src_w;
-	swap(plane->state->fb, new_state->fb);
 
 	mtk_plane_update_new_state(new_state, new_plane_state);
+	swap(plane->state->fb, new_state->fb);
 	wmb(); /* Make sure the above parameters are set before update */
 	new_plane_state->pending.async_dirty = true;
 	mtk_drm_crtc_async_update(new_state->crtc, plane, state);
-- 
2.18.0

[PATCH RESEND v4 1/2] drm/mediatek: Fix iommu fault by swapping FBs after updating plane state

[Jason-JH.Lin]

According to the comment in drm_atomic_helper_async_commit(),
we should make sure FBs have been swapped, so that cleanups in the
new_state performs a cleanup in the old FB.

So we should move swapping FBs after calling mtk_plane_update_new_state(),
to avoid using the old FB which could be freed.

Fixes: 1a64a7aff8da ("drm/mediatek: Fix cursor plane no update")
Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index 31f9420aff6f..b1a918ffe457 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -210,9 +210,9 @@ static void mtk_plane_atomic_async_update(struct drm_plane *plane,
 	plane->state->src_y = new_state->src_y;
 	plane->state->src_h = new_state->src_h;
 	plane->state->src_w = new_state->src_w;
-	swap(plane->state->fb, new_state->fb);
 
 	mtk_plane_update_new_state(new_state, new_plane_state);
+	swap(plane->state->fb, new_state->fb);
 	wmb(); /* Make sure the above parameters are set before update */
 	new_plane_state->pending.async_dirty = true;
 	mtk_drm_crtc_async_update(new_state->crtc, plane, state);
-- 
2.18.0

[PATCH RESEND v4 1/2] drm/mediatek: Fix iommu fault by swapping FBs after updating plane state

[Jason-JH.Lin]

According to the comment in drm_atomic_helper_async_commit(),
we should make sure FBs have been swapped, so that cleanups in the
new_state performs a cleanup in the old FB.

So we should move swapping FBs after calling mtk_plane_update_new_state(),
to avoid using the old FB which could be freed.

Fixes: 1a64a7aff8da ("drm/mediatek: Fix cursor plane no update")
Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index 31f9420aff6f..b1a918ffe457 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -210,9 +210,9 @@ static void mtk_plane_atomic_async_update(struct drm_plane *plane,
 	plane->state->src_y = new_state->src_y;
 	plane->state->src_h = new_state->src_h;
 	plane->state->src_w = new_state->src_w;
-	swap(plane->state->fb, new_state->fb);
 
 	mtk_plane_update_new_state(new_state, new_plane_state);
+	swap(plane->state->fb, new_state->fb);
 	wmb(); /* Make sure the above parameters are set before update */
 	new_plane_state->pending.async_dirty = true;
 	mtk_drm_crtc_async_update(new_state->crtc, plane, state);
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Jason-JH.Lin]

The plane_state of drm_atomic_state is not sync to the mtk_plane_state
stored in mtk_crtc during crtc enabling.

So we need to update the mtk_plane_state stored in mtk_crtc by the
drm_atomic_state carried from mtk_drm_crtc_atomic_enable().

While updating mtk_plane_state, OVL layer should be disabled when the fb
in plane_state of drm_atomic_state is NULL.

Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
Change in RESEND v4:
Remove redundant plane_state assigning.
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
 drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index d40142842f85..7db4d6551da7 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
 }
 #endif
 
-static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
+static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc, struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc = &mtk_crtc->base;
 	struct drm_connector *connector;
@@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
 	/* Initially configure all planes */
 	for (i = 0; i < mtk_crtc->layer_nr; i++) {
 		struct drm_plane *plane = &mtk_crtc->planes[i];
-		struct mtk_plane_state *plane_state;
+		struct drm_plane_state *new_state;
+		struct mtk_plane_state *plane_state = to_mtk_plane_state(plane->state);
 		struct mtk_ddp_comp *comp;
 		unsigned int local_layer;
 
-		plane_state = to_mtk_plane_state(plane->state);
+		/* sync the new plane state from drm_atomic_state */
+		if (state->planes[i].ptr) {
+			new_state = drm_atomic_get_new_plane_state(state, state->planes[i].ptr);
+			mtk_plane_update_new_state(new_state, plane_state);
+		}
+
 		comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
 		if (comp)
 			mtk_ddp_comp_layer_config(comp, local_layer,
@@ -687,7 +693,7 @@ static void mtk_drm_crtc_atomic_enable(struct drm_crtc *crtc,
 		return;
 	}
 
-	ret = mtk_crtc_ddp_hw_init(mtk_crtc);
+	ret = mtk_crtc_ddp_hw_init(mtk_crtc, state);
 	if (ret) {
 		pm_runtime_put(comp->dev);
 		return;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index b1a918ffe457..ef4460f98c07 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -134,8 +134,8 @@ static int mtk_plane_atomic_async_check(struct drm_plane *plane,
 						   true, true);
 }
 
-static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
-				       struct mtk_plane_state *mtk_plane_state)
+void mtk_plane_update_new_state(struct drm_plane_state *new_state,
+				struct mtk_plane_state *mtk_plane_state)
 {
 	struct drm_framebuffer *fb = new_state->fb;
 	struct drm_gem_object *gem;
@@ -146,6 +146,11 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
 	dma_addr_t hdr_addr = 0;
 	unsigned int hdr_pitch = 0;
 
+	if (!fb) {
+		mtk_plane_state->pending.enable = false;
+		return;
+	}
+
 	gem = fb->obj[0];
 	mtk_gem = to_mtk_gem_obj(gem);
 	addr = mtk_gem->dma_addr;
@@ -180,7 +185,7 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
 		       fb->format->cpp[0] * (x_offset_in_blocks + 1);
 	}
 
-	mtk_plane_state->pending.enable = true;
+	mtk_plane_state->pending.enable = new_state->visible;
 	mtk_plane_state->pending.pitch = pitch;
 	mtk_plane_state->pending.hdr_pitch = hdr_pitch;
 	mtk_plane_state->pending.format = format;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
index 99aff7da0831..0a7d70d13e43 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
@@ -46,6 +46,8 @@ to_mtk_plane_state(struct drm_plane_state *state)
 	return container_of(state, struct mtk_plane_state, base);
 }
 
+void mtk_plane_update_new_state(struct drm_plane_state *new_state,
+				struct mtk_plane_state *mtk_plane_state);
 int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		   unsigned long possible_crtcs, enum drm_plane_type type,
 		   unsigned int supported_rotations, const u32 *formats,
-- 
2.18.0

[PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Jason-JH.Lin]

The plane_state of drm_atomic_state is not sync to the mtk_plane_state
stored in mtk_crtc during crtc enabling.

So we need to update the mtk_plane_state stored in mtk_crtc by the
drm_atomic_state carried from mtk_drm_crtc_atomic_enable().

While updating mtk_plane_state, OVL layer should be disabled when the fb
in plane_state of drm_atomic_state is NULL.

Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
Change in RESEND v4:
Remove redundant plane_state assigning.
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
 drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index d40142842f85..7db4d6551da7 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
 }
 #endif
 
-static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
+static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc, struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc = &mtk_crtc->base;
 	struct drm_connector *connector;
@@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
 	/* Initially configure all planes */
 	for (i = 0; i < mtk_crtc->layer_nr; i++) {
 		struct drm_plane *plane = &mtk_crtc->planes[i];
-		struct mtk_plane_state *plane_state;
+		struct drm_plane_state *new_state;
+		struct mtk_plane_state *plane_state = to_mtk_plane_state(plane->state);
 		struct mtk_ddp_comp *comp;
 		unsigned int local_layer;
 
-		plane_state = to_mtk_plane_state(plane->state);
+		/* sync the new plane state from drm_atomic_state */
+		if (state->planes[i].ptr) {
+			new_state = drm_atomic_get_new_plane_state(state, state->planes[i].ptr);
+			mtk_plane_update_new_state(new_state, plane_state);
+		}
+
 		comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
 		if (comp)
 			mtk_ddp_comp_layer_config(comp, local_layer,
@@ -687,7 +693,7 @@ static void mtk_drm_crtc_atomic_enable(struct drm_crtc *crtc,
 		return;
 	}
 
-	ret = mtk_crtc_ddp_hw_init(mtk_crtc);
+	ret = mtk_crtc_ddp_hw_init(mtk_crtc, state);
 	if (ret) {
 		pm_runtime_put(comp->dev);
 		return;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index b1a918ffe457..ef4460f98c07 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -134,8 +134,8 @@ static int mtk_plane_atomic_async_check(struct drm_plane *plane,
 						   true, true);
 }
 
-static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
-				       struct mtk_plane_state *mtk_plane_state)
+void mtk_plane_update_new_state(struct drm_plane_state *new_state,
+				struct mtk_plane_state *mtk_plane_state)
 {
 	struct drm_framebuffer *fb = new_state->fb;
 	struct drm_gem_object *gem;
@@ -146,6 +146,11 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
 	dma_addr_t hdr_addr = 0;
 	unsigned int hdr_pitch = 0;
 
+	if (!fb) {
+		mtk_plane_state->pending.enable = false;
+		return;
+	}
+
 	gem = fb->obj[0];
 	mtk_gem = to_mtk_gem_obj(gem);
 	addr = mtk_gem->dma_addr;
@@ -180,7 +185,7 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
 		       fb->format->cpp[0] * (x_offset_in_blocks + 1);
 	}
 
-	mtk_plane_state->pending.enable = true;
+	mtk_plane_state->pending.enable = new_state->visible;
 	mtk_plane_state->pending.pitch = pitch;
 	mtk_plane_state->pending.hdr_pitch = hdr_pitch;
 	mtk_plane_state->pending.format = format;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
index 99aff7da0831..0a7d70d13e43 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
@@ -46,6 +46,8 @@ to_mtk_plane_state(struct drm_plane_state *state)
 	return container_of(state, struct mtk_plane_state, base);
 }
 
+void mtk_plane_update_new_state(struct drm_plane_state *new_state,
+				struct mtk_plane_state *mtk_plane_state);
 int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		   unsigned long possible_crtcs, enum drm_plane_type type,
 		   unsigned int supported_rotations, const u32 *formats,
-- 
2.18.0

[PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Jason-JH.Lin]

The plane_state of drm_atomic_state is not sync to the mtk_plane_state
stored in mtk_crtc during crtc enabling.

So we need to update the mtk_plane_state stored in mtk_crtc by the
drm_atomic_state carried from mtk_drm_crtc_atomic_enable().

While updating mtk_plane_state, OVL layer should be disabled when the fb
in plane_state of drm_atomic_state is NULL.

Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
Change in RESEND v4:
Remove redundant plane_state assigning.
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
 drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index d40142842f85..7db4d6551da7 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
 }
 #endif
 
-static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
+static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc, struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc = &mtk_crtc->base;
 	struct drm_connector *connector;
@@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
 	/* Initially configure all planes */
 	for (i = 0; i < mtk_crtc->layer_nr; i++) {
 		struct drm_plane *plane = &mtk_crtc->planes[i];
-		struct mtk_plane_state *plane_state;
+		struct drm_plane_state *new_state;
+		struct mtk_plane_state *plane_state = to_mtk_plane_state(plane->state);
 		struct mtk_ddp_comp *comp;
 		unsigned int local_layer;
 
-		plane_state = to_mtk_plane_state(plane->state);
+		/* sync the new plane state from drm_atomic_state */
+		if (state->planes[i].ptr) {
+			new_state = drm_atomic_get_new_plane_state(state, state->planes[i].ptr);
+			mtk_plane_update_new_state(new_state, plane_state);
+		}
+
 		comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
 		if (comp)
 			mtk_ddp_comp_layer_config(comp, local_layer,
@@ -687,7 +693,7 @@ static void mtk_drm_crtc_atomic_enable(struct drm_crtc *crtc,
 		return;
 	}
 
-	ret = mtk_crtc_ddp_hw_init(mtk_crtc);
+	ret = mtk_crtc_ddp_hw_init(mtk_crtc, state);
 	if (ret) {
 		pm_runtime_put(comp->dev);
 		return;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index b1a918ffe457..ef4460f98c07 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -134,8 +134,8 @@ static int mtk_plane_atomic_async_check(struct drm_plane *plane,
 						   true, true);
 }
 
-static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
-				       struct mtk_plane_state *mtk_plane_state)
+void mtk_plane_update_new_state(struct drm_plane_state *new_state,
+				struct mtk_plane_state *mtk_plane_state)
 {
 	struct drm_framebuffer *fb = new_state->fb;
 	struct drm_gem_object *gem;
@@ -146,6 +146,11 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
 	dma_addr_t hdr_addr = 0;
 	unsigned int hdr_pitch = 0;
 
+	if (!fb) {
+		mtk_plane_state->pending.enable = false;
+		return;
+	}
+
 	gem = fb->obj[0];
 	mtk_gem = to_mtk_gem_obj(gem);
 	addr = mtk_gem->dma_addr;
@@ -180,7 +185,7 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
 		       fb->format->cpp[0] * (x_offset_in_blocks + 1);
 	}
 
-	mtk_plane_state->pending.enable = true;
+	mtk_plane_state->pending.enable = new_state->visible;
 	mtk_plane_state->pending.pitch = pitch;
 	mtk_plane_state->pending.hdr_pitch = hdr_pitch;
 	mtk_plane_state->pending.format = format;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
index 99aff7da0831..0a7d70d13e43 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
@@ -46,6 +46,8 @@ to_mtk_plane_state(struct drm_plane_state *state)
 	return container_of(state, struct mtk_plane_state, base);
 }
 
+void mtk_plane_update_new_state(struct drm_plane_state *new_state,
+				struct mtk_plane_state *mtk_plane_state);
 int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		   unsigned long possible_crtcs, enum drm_plane_type type,
 		   unsigned int supported_rotations, const u32 *formats,
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[CK Hu (胡俊光)]

Hi, Jason:

On Mon, 2023-08-07 at 09:51 +0800, Jason-JH.Lin wrote:
> The plane_state of drm_atomic_state is not sync to the
> mtk_plane_state
> stored in mtk_crtc during crtc enabling.
> 
> So we need to update the mtk_plane_state stored in mtk_crtc by the
> drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> 
> While updating mtk_plane_state, OVL layer should be disabled when the
> fb
> in plane_state of drm_atomic_state is NULL.
> 
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> MT8173.")
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
> Change in RESEND v4:
> Remove redundant plane_state assigning.
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
>  drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index d40142842f85..7db4d6551da7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl,
> void *mssg)
>  }
>  #endif
>  
> -static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc,
> struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc = &mtk_crtc->base;
>  	struct drm_connector *connector;
> @@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct
> mtk_drm_crtc *mtk_crtc)
>  	/* Initially configure all planes */
>  	for (i = 0; i < mtk_crtc->layer_nr; i++) {
>  		struct drm_plane *plane = &mtk_crtc->planes[i];
> -		struct mtk_plane_state *plane_state;
> +		struct drm_plane_state *new_state;
> +		struct mtk_plane_state *plane_state =
> to_mtk_plane_state(plane->state);
>  		struct mtk_ddp_comp *comp;
>  		unsigned int local_layer;
>  
> -		plane_state = to_mtk_plane_state(plane->state);
> +		/* sync the new plane state from drm_atomic_state */
> +		if (state->planes[i].ptr) {
> +			new_state =
> drm_atomic_get_new_plane_state(state, state->planes[i].ptr);

for_each_new_plane_in_state()?

> +			mtk_plane_update_new_state(new_state,
> plane_state);
> +		}
> +
>  		comp = mtk_drm_ddp_comp_for_plane(crtc, plane,
> &local_layer);
>  		if (comp)
>  			mtk_ddp_comp_layer_config(comp, local_layer,
> @@ -687,7 +693,7 @@ static void mtk_drm_crtc_atomic_enable(struct
> drm_crtc *crtc,
>  		return;
>  	}
>  
> -	ret = mtk_crtc_ddp_hw_init(mtk_crtc);
> +	ret = mtk_crtc_ddp_hw_init(mtk_crtc, state);
>  	if (ret) {
>  		pm_runtime_put(comp->dev);
>  		return;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index b1a918ffe457..ef4460f98c07 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -134,8 +134,8 @@ static int mtk_plane_atomic_async_check(struct
> drm_plane *plane,
>  						   true, true);
>  }
>  
> -static void mtk_plane_update_new_state(struct drm_plane_state
> *new_state,
> -				       struct mtk_plane_state
> *mtk_plane_state)
> +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> +				struct mtk_plane_state
> *mtk_plane_state)
>  {
>  	struct drm_framebuffer *fb = new_state->fb;
>  	struct drm_gem_object *gem;
> @@ -146,6 +146,11 @@ static void mtk_plane_update_new_state(struct
> drm_plane_state *new_state,
>  	dma_addr_t hdr_addr = 0;
>  	unsigned int hdr_pitch = 0;
>  
> +	if (!fb) {
> +		mtk_plane_state->pending.enable = false;
> +		return;
> +	}

This seems done in mtk_plane_atomic_update(), so you may call
mtk_plane_atomic_update() instead of mtk_plane_update_new_state().

Regards,
CK

> +
>  	gem = fb->obj[0];
>  	mtk_gem = to_mtk_gem_obj(gem);
>  	addr = mtk_gem->dma_addr;
> @@ -180,7 +185,7 @@ static void mtk_plane_update_new_state(struct
> drm_plane_state *new_state,
>  		       fb->format->cpp[0] * (x_offset_in_blocks + 1);
>  	}
>  
> -	mtk_plane_state->pending.enable = true;
> +	mtk_plane_state->pending.enable = new_state->visible;
>  	mtk_plane_state->pending.pitch = pitch;
>  	mtk_plane_state->pending.hdr_pitch = hdr_pitch;
>  	mtk_plane_state->pending.format = format;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> index 99aff7da0831..0a7d70d13e43 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> @@ -46,6 +46,8 @@ to_mtk_plane_state(struct drm_plane_state *state)
>  	return container_of(state, struct mtk_plane_state, base);
>  }
>  
> +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> +				struct mtk_plane_state
> *mtk_plane_state);
>  int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  		   unsigned long possible_crtcs, enum drm_plane_type
> type,
>  		   unsigned int supported_rotations, const u32
> *formats,

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[CK Hu (胡俊光)]

Hi, Jason:

On Mon, 2023-08-07 at 09:51 +0800, Jason-JH.Lin wrote:
> The plane_state of drm_atomic_state is not sync to the
> mtk_plane_state
> stored in mtk_crtc during crtc enabling.
> 
> So we need to update the mtk_plane_state stored in mtk_crtc by the
> drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> 
> While updating mtk_plane_state, OVL layer should be disabled when the
> fb
> in plane_state of drm_atomic_state is NULL.
> 
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> MT8173.")
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
> Change in RESEND v4:
> Remove redundant plane_state assigning.
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
>  drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index d40142842f85..7db4d6551da7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl,
> void *mssg)
>  }
>  #endif
>  
> -static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc,
> struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc = &mtk_crtc->base;
>  	struct drm_connector *connector;
> @@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct
> mtk_drm_crtc *mtk_crtc)
>  	/* Initially configure all planes */
>  	for (i = 0; i < mtk_crtc->layer_nr; i++) {
>  		struct drm_plane *plane = &mtk_crtc->planes[i];
> -		struct mtk_plane_state *plane_state;
> +		struct drm_plane_state *new_state;
> +		struct mtk_plane_state *plane_state =
> to_mtk_plane_state(plane->state);
>  		struct mtk_ddp_comp *comp;
>  		unsigned int local_layer;
>  
> -		plane_state = to_mtk_plane_state(plane->state);
> +		/* sync the new plane state from drm_atomic_state */
> +		if (state->planes[i].ptr) {
> +			new_state =
> drm_atomic_get_new_plane_state(state, state->planes[i].ptr);

for_each_new_plane_in_state()?

> +			mtk_plane_update_new_state(new_state,
> plane_state);
> +		}
> +
>  		comp = mtk_drm_ddp_comp_for_plane(crtc, plane,
> &local_layer);
>  		if (comp)
>  			mtk_ddp_comp_layer_config(comp, local_layer,
> @@ -687,7 +693,7 @@ static void mtk_drm_crtc_atomic_enable(struct
> drm_crtc *crtc,
>  		return;
>  	}
>  
> -	ret = mtk_crtc_ddp_hw_init(mtk_crtc);
> +	ret = mtk_crtc_ddp_hw_init(mtk_crtc, state);
>  	if (ret) {
>  		pm_runtime_put(comp->dev);
>  		return;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index b1a918ffe457..ef4460f98c07 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -134,8 +134,8 @@ static int mtk_plane_atomic_async_check(struct
> drm_plane *plane,
>  						   true, true);
>  }
>  
> -static void mtk_plane_update_new_state(struct drm_plane_state
> *new_state,
> -				       struct mtk_plane_state
> *mtk_plane_state)
> +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> +				struct mtk_plane_state
> *mtk_plane_state)
>  {
>  	struct drm_framebuffer *fb = new_state->fb;
>  	struct drm_gem_object *gem;
> @@ -146,6 +146,11 @@ static void mtk_plane_update_new_state(struct
> drm_plane_state *new_state,
>  	dma_addr_t hdr_addr = 0;
>  	unsigned int hdr_pitch = 0;
>  
> +	if (!fb) {
> +		mtk_plane_state->pending.enable = false;
> +		return;
> +	}

This seems done in mtk_plane_atomic_update(), so you may call
mtk_plane_atomic_update() instead of mtk_plane_update_new_state().

Regards,
CK

> +
>  	gem = fb->obj[0];
>  	mtk_gem = to_mtk_gem_obj(gem);
>  	addr = mtk_gem->dma_addr;
> @@ -180,7 +185,7 @@ static void mtk_plane_update_new_state(struct
> drm_plane_state *new_state,
>  		       fb->format->cpp[0] * (x_offset_in_blocks + 1);
>  	}
>  
> -	mtk_plane_state->pending.enable = true;
> +	mtk_plane_state->pending.enable = new_state->visible;
>  	mtk_plane_state->pending.pitch = pitch;
>  	mtk_plane_state->pending.hdr_pitch = hdr_pitch;
>  	mtk_plane_state->pending.format = format;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> index 99aff7da0831..0a7d70d13e43 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> @@ -46,6 +46,8 @@ to_mtk_plane_state(struct drm_plane_state *state)
>  	return container_of(state, struct mtk_plane_state, base);
>  }
>  
> +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> +				struct mtk_plane_state
> *mtk_plane_state);
>  int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  		   unsigned long possible_crtcs, enum drm_plane_type
> type,
>  		   unsigned int supported_rotations, const u32
> *formats,
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[CK Hu (胡俊光)]

[-- Attachment #1: Type: text/html, Size: 8468 bytes --]

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

Hi, Jason:

On Mon, 2023-08-07 at 09:51 +0800, Jason-JH.Lin wrote:
> The plane_state of drm_atomic_state is not sync to the
> mtk_plane_state
> stored in mtk_crtc during crtc enabling.
> 
> So we need to update the mtk_plane_state stored in mtk_crtc by the
> drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> 
> While updating mtk_plane_state, OVL layer should be disabled when the
> fb
> in plane_state of drm_atomic_state is NULL.
> 
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> MT8173.")
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
> Change in RESEND v4:
> Remove redundant plane_state assigning.
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
>  drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index d40142842f85..7db4d6551da7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl,
> void *mssg)
>  }
>  #endif
>  
> -static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc,
> struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc = &mtk_crtc->base;
>  	struct drm_connector *connector;
> @@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct
> mtk_drm_crtc *mtk_crtc)
>  	/* Initially configure all planes */
>  	for (i = 0; i < mtk_crtc->layer_nr; i++) {
>  		struct drm_plane *plane = &mtk_crtc->planes[i];
> -		struct mtk_plane_state *plane_state;
> +		struct drm_plane_state *new_state;
> +		struct mtk_plane_state *plane_state =
> to_mtk_plane_state(plane->state);
>  		struct mtk_ddp_comp *comp;
>  		unsigned int local_layer;
>  
> -		plane_state = to_mtk_plane_state(plane->state);
> +		/* sync the new plane state from drm_atomic_state */
> +		if (state->planes[i].ptr) {
> +			new_state =
> drm_atomic_get_new_plane_state(state, state->planes[i].ptr);

for_each_new_plane_in_state()?

> +			mtk_plane_update_new_state(new_state,
> plane_state);
> +		}
> +
>  		comp = mtk_drm_ddp_comp_for_plane(crtc, plane,
> &local_layer);
>  		if (comp)
>  			mtk_ddp_comp_layer_config(comp, local_layer,
> @@ -687,7 +693,7 @@ static void mtk_drm_crtc_atomic_enable(struct
> drm_crtc *crtc,
>  		return;
>  	}
>  
> -	ret = mtk_crtc_ddp_hw_init(mtk_crtc);
> +	ret = mtk_crtc_ddp_hw_init(mtk_crtc, state);
>  	if (ret) {
>  		pm_runtime_put(comp->dev);
>  		return;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index b1a918ffe457..ef4460f98c07 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -134,8 +134,8 @@ static int mtk_plane_atomic_async_check(struct
> drm_plane *plane,
>  						   true, true);
>  }
>  
> -static void mtk_plane_update_new_state(struct drm_plane_state
> *new_state,
> -				       struct mtk_plane_state
> *mtk_plane_state)
> +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> +				struct mtk_plane_state
> *mtk_plane_state)
>  {
>  	struct drm_framebuffer *fb = new_state->fb;
>  	struct drm_gem_object *gem;
> @@ -146,6 +146,11 @@ static void mtk_plane_update_new_state(struct
> drm_plane_state *new_state,
>  	dma_addr_t hdr_addr = 0;
>  	unsigned int hdr_pitch = 0;
>  
> +	if (!fb) {
> +		mtk_plane_state->pending.enable = false;
> +		return;
> +	}

This seems done in mtk_plane_atomic_update(), so you may call
mtk_plane_atomic_update() instead of mtk_plane_update_new_state().

Regards,
CK

> +
>  	gem = fb->obj[0];
>  	mtk_gem = to_mtk_gem_obj(gem);
>  	addr = mtk_gem->dma_addr;
> @@ -180,7 +185,7 @@ static void mtk_plane_update_new_state(struct
> drm_plane_state *new_state,
>  		       fb->format->cpp[0] * (x_offset_in_blocks + 1);
>  	}
>  
> -	mtk_plane_state->pending.enable = true;
> +	mtk_plane_state->pending.enable = new_state->visible;
>  	mtk_plane_state->pending.pitch = pitch;
>  	mtk_plane_state->pending.hdr_pitch = hdr_pitch;
>  	mtk_plane_state->pending.format = format;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> index 99aff7da0831..0a7d70d13e43 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> @@ -46,6 +46,8 @@ to_mtk_plane_state(struct drm_plane_state *state)
>  	return container_of(state, struct mtk_plane_state, base);
>  }
>  
> +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> +				struct mtk_plane_state
> *mtk_plane_state);
>  int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  		   unsigned long possible_crtcs, enum drm_plane_type
> type,
>  		   unsigned int supported_rotations, const u32
> *formats,

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Alexandre Mergnat]

Hi Jason,

You forgot to put the Reviewed-by got from the V3 in your commit message.


On 07/08/2023 03:51, Jason-JH.Lin wrote:
> The plane_state of drm_atomic_state is not sync to the mtk_plane_state
> stored in mtk_crtc during crtc enabling.
> 
> So we need to update the mtk_plane_state stored in mtk_crtc by the
> drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> 
> While updating mtk_plane_state, OVL layer should be disabled when the fb
> in plane_state of drm_atomic_state is NULL.
> 
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
> Change in RESEND v4:
> Remove redundant plane_state assigning.
> ---


-- 
Regards,
Alexandre

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Alexandre Mergnat]

Hi Jason,

You forgot to put the Reviewed-by got from the V3 in your commit message.


On 07/08/2023 03:51, Jason-JH.Lin wrote:
> The plane_state of drm_atomic_state is not sync to the mtk_plane_state
> stored in mtk_crtc during crtc enabling.
> 
> So we need to update the mtk_plane_state stored in mtk_crtc by the
> drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> 
> While updating mtk_plane_state, OVL layer should be disabled when the fb
> in plane_state of drm_atomic_state is NULL.
> 
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
> Change in RESEND v4:
> Remove redundant plane_state assigning.
> ---


-- 
Regards,
Alexandre

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Alexandre Mergnat]

Hi Jason,

You forgot to put the Reviewed-by got from the V3 in your commit message.


On 07/08/2023 03:51, Jason-JH.Lin wrote:
> The plane_state of drm_atomic_state is not sync to the mtk_plane_state
> stored in mtk_crtc during crtc enabling.
> 
> So we need to update the mtk_plane_state stored in mtk_crtc by the
> drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> 
> While updating mtk_plane_state, OVL layer should be disabled when the fb
> in plane_state of drm_atomic_state is NULL.
> 
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
> Change in RESEND v4:
> Remove redundant plane_state assigning.
> ---


-- 
Regards,
Alexandre

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Jason-JH Lin (林睿祥)]

Hi Alexandre,

On Mon, 2023-08-07 at 11:45 +0200, Alexandre Mergnat wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hi Jason,
> 
> You forgot to put the Reviewed-by got from the V3 in your commit
> message.
> 

Since I changed the original method at the new version, I removed all
reviewed-by tags.
In order to avoid the version you have reviewed is too different.

Regards,
Jason-JH.Lin

> 
> On 07/08/2023 03:51, Jason-JH.Lin wrote:
> > The plane_state of drm_atomic_state is not sync to the
> mtk_plane_state
> > stored in mtk_crtc during crtc enabling.
> > 
> > So we need to update the mtk_plane_state stored in mtk_crtc by the
> > drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> > 
> > While updating mtk_plane_state, OVL layer should be disabled when
> the fb
> > in plane_state of drm_atomic_state is NULL.
> > 
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> MT8173.")
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> > Change in RESEND v4:
> > Remove redundant plane_state assigning.
> > ---
> 
> 
> -- 
> Regards,
> Alexandre

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Jason-JH Lin (林睿祥)]

Hi Alexandre,

On Mon, 2023-08-07 at 11:45 +0200, Alexandre Mergnat wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hi Jason,
> 
> You forgot to put the Reviewed-by got from the V3 in your commit
> message.
> 

Since I changed the original method at the new version, I removed all
reviewed-by tags.
In order to avoid the version you have reviewed is too different.

Regards,
Jason-JH.Lin

> 
> On 07/08/2023 03:51, Jason-JH.Lin wrote:
> > The plane_state of drm_atomic_state is not sync to the
> mtk_plane_state
> > stored in mtk_crtc during crtc enabling.
> > 
> > So we need to update the mtk_plane_state stored in mtk_crtc by the
> > drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> > 
> > While updating mtk_plane_state, OVL layer should be disabled when
> the fb
> > in plane_state of drm_atomic_state is NULL.
> > 
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> MT8173.")
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> > Change in RESEND v4:
> > Remove redundant plane_state assigning.
> > ---
> 
> 
> -- 
> Regards,
> Alexandre
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Jason-JH Lin (林睿祥)]

[-- Attachment #1: Type: text/html, Size: 3018 bytes --]

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

Hi Alexandre,

On Mon, 2023-08-07 at 11:45 +0200, Alexandre Mergnat wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hi Jason,
> 
> You forgot to put the Reviewed-by got from the V3 in your commit
> message.
> 

Since I changed the original method at the new version, I removed all
reviewed-by tags.
In order to avoid the version you have reviewed is too different.

Regards,
Jason-JH.Lin

> 
> On 07/08/2023 03:51, Jason-JH.Lin wrote:
> > The plane_state of drm_atomic_state is not sync to the
> mtk_plane_state
> > stored in mtk_crtc during crtc enabling.
> > 
> > So we need to update the mtk_plane_state stored in mtk_crtc by the
> > drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> > 
> > While updating mtk_plane_state, OVL layer should be disabled when
> the fb
> > in plane_state of drm_atomic_state is NULL.
> > 
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> MT8173.")
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> > Change in RESEND v4:
> > Remove redundant plane_state assigning.
> > ---
> 
> 
> -- 
> Regards,
> Alexandre

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Jason-JH Lin (林睿祥)]

Hi CK,

On Mon, 2023-08-07 at 08:59 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Mon, 2023-08-07 at 09:51 +0800, Jason-JH.Lin wrote:
> > The plane_state of drm_atomic_state is not sync to the
> > mtk_plane_state
> > stored in mtk_crtc during crtc enabling.
> > 
> > So we need to update the mtk_plane_state stored in mtk_crtc by the
> > drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> > 
> > While updating mtk_plane_state, OVL layer should be disabled when
> > the
> > fb
> > in plane_state of drm_atomic_state is NULL.
> > 
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> > MT8173.")
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> > Change in RESEND v4:
> > Remove redundant plane_state assigning.
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
> >  3 files changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index d40142842f85..7db4d6551da7 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl,
> > void *mssg)
> >  }
> >  #endif
> >  
> > -static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> > +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc,
> > struct drm_atomic_state *state)
> >  {
> >  	struct drm_crtc *crtc = &mtk_crtc->base;
> >  	struct drm_connector *connector;
> > @@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct
> > mtk_drm_crtc *mtk_crtc)
> >  	/* Initially configure all planes */
> >  	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> >  		struct drm_plane *plane = &mtk_crtc->planes[i];
> > -		struct mtk_plane_state *plane_state;
> > +		struct drm_plane_state *new_state;
> > +		struct mtk_plane_state *plane_state =
> > to_mtk_plane_state(plane->state);
> >  		struct mtk_ddp_comp *comp;
> >  		unsigned int local_layer;
> >  
> > -		plane_state = to_mtk_plane_state(plane->state);
> > +		/* sync the new plane state from drm_atomic_state */
> > +		if (state->planes[i].ptr) {
> > +			new_state =
> > drm_atomic_get_new_plane_state(state, state->planes[i].ptr);
> 
> for_each_new_plane_in_state()?

I'vd tried for_each_new_plane_in_state(), but it crashed in the fifth
round.
Because mode_config.num_total_plane in this macro is 8, but mtk_crtc-
>layer_nr is 4.

So I think we can't use this macro here.

> 
> > +			mtk_plane_update_new_state(new_state,
> > plane_state);
> > +		}
> > +
> >  		comp = mtk_drm_ddp_comp_for_plane(crtc, plane,
> > &local_layer);
> >  		if (comp)
> >  			mtk_ddp_comp_layer_config(comp, local_layer,
> > @@ -687,7 +693,7 @@ static void mtk_drm_crtc_atomic_enable(struct
> > drm_crtc *crtc,
> >  		return;
> >  	}
> >  
> > -	ret = mtk_crtc_ddp_hw_init(mtk_crtc);
> > +	ret = mtk_crtc_ddp_hw_init(mtk_crtc, state);
> >  	if (ret) {
> >  		pm_runtime_put(comp->dev);
> >  		return;
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > index b1a918ffe457..ef4460f98c07 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > @@ -134,8 +134,8 @@ static int mtk_plane_atomic_async_check(struct
> > drm_plane *plane,
> >  						   true, true);
> >  }
> >  
> > -static void mtk_plane_update_new_state(struct drm_plane_state
> > *new_state,
> > -				       struct mtk_plane_state
> > *mtk_plane_state)
> > +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> > +				struct mtk_plane_state
> > *mtk_plane_state)
> >  {
> >  	struct drm_framebuffer *fb = new_state->fb;
> >  	struct drm_gem_object *gem;
> > @@ -146,6 +146,11 @@ static void mtk_plane_update_new_state(struct
> > drm_plane_state *new_state,
> >  	dma_addr_t hdr_addr = 0;
> >  	unsigned int hdr_pitch = 0;
> >  
> > +	if (!fb) {
> > +		mtk_plane_state->pending.enable = false;
> > +		return;
> > +	}
> 
> This seems done in mtk_plane_atomic_update(), so you may call
> mtk_plane_atomic_update() instead of mtk_plane_update_new_state().

mtk_plane_atomic_update() won't update the mtk_plane_state in mtk_crtc,
so I use mtk_plane_update_new_state() here.

But I found that we can fix up iommu fault issue by handling the NULL
fb case.
So I'll change the previous method to disable the plane if its fb in
new state is NULL.

Regards,
Jason-JH.Lin

> 
> Regards,
> CK
> 
> > +
> >  	gem = fb->obj[0];
> >  	mtk_gem = to_mtk_gem_obj(gem);
> >  	addr = mtk_gem->dma_addr;
> > @@ -180,7 +185,7 @@ static void mtk_plane_update_new_state(struct
> > drm_plane_state *new_state,
> >  		       fb->format->cpp[0] * (x_offset_in_blocks + 1);
> >  	}
> >  
> > -	mtk_plane_state->pending.enable = true;
> > +	mtk_plane_state->pending.enable = new_state->visible;
> >  	mtk_plane_state->pending.pitch = pitch;
> >  	mtk_plane_state->pending.hdr_pitch = hdr_pitch;
> >  	mtk_plane_state->pending.format = format;
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > index 99aff7da0831..0a7d70d13e43 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > @@ -46,6 +46,8 @@ to_mtk_plane_state(struct drm_plane_state *state)
> >  	return container_of(state, struct mtk_plane_state, base);
> >  }
> >  
> > +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> > +				struct mtk_plane_state
> > *mtk_plane_state);
> >  int mtk_plane_init(struct drm_device *dev, struct drm_plane
> > *plane,
> >  		   unsigned long possible_crtcs, enum drm_plane_type
> > type,
> >  		   unsigned int supported_rotations, const u32
> > *formats,

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Jason-JH Lin (林睿祥)]

Hi CK,

On Mon, 2023-08-07 at 08:59 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Mon, 2023-08-07 at 09:51 +0800, Jason-JH.Lin wrote:
> > The plane_state of drm_atomic_state is not sync to the
> > mtk_plane_state
> > stored in mtk_crtc during crtc enabling.
> > 
> > So we need to update the mtk_plane_state stored in mtk_crtc by the
> > drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> > 
> > While updating mtk_plane_state, OVL layer should be disabled when
> > the
> > fb
> > in plane_state of drm_atomic_state is NULL.
> > 
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> > MT8173.")
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> > Change in RESEND v4:
> > Remove redundant plane_state assigning.
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
> >  3 files changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index d40142842f85..7db4d6551da7 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl,
> > void *mssg)
> >  }
> >  #endif
> >  
> > -static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> > +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc,
> > struct drm_atomic_state *state)
> >  {
> >  	struct drm_crtc *crtc = &mtk_crtc->base;
> >  	struct drm_connector *connector;
> > @@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct
> > mtk_drm_crtc *mtk_crtc)
> >  	/* Initially configure all planes */
> >  	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> >  		struct drm_plane *plane = &mtk_crtc->planes[i];
> > -		struct mtk_plane_state *plane_state;
> > +		struct drm_plane_state *new_state;
> > +		struct mtk_plane_state *plane_state =
> > to_mtk_plane_state(plane->state);
> >  		struct mtk_ddp_comp *comp;
> >  		unsigned int local_layer;
> >  
> > -		plane_state = to_mtk_plane_state(plane->state);
> > +		/* sync the new plane state from drm_atomic_state */
> > +		if (state->planes[i].ptr) {
> > +			new_state =
> > drm_atomic_get_new_plane_state(state, state->planes[i].ptr);
> 
> for_each_new_plane_in_state()?

I'vd tried for_each_new_plane_in_state(), but it crashed in the fifth
round.
Because mode_config.num_total_plane in this macro is 8, but mtk_crtc-
>layer_nr is 4.

So I think we can't use this macro here.

> 
> > +			mtk_plane_update_new_state(new_state,
> > plane_state);
> > +		}
> > +
> >  		comp = mtk_drm_ddp_comp_for_plane(crtc, plane,
> > &local_layer);
> >  		if (comp)
> >  			mtk_ddp_comp_layer_config(comp, local_layer,
> > @@ -687,7 +693,7 @@ static void mtk_drm_crtc_atomic_enable(struct
> > drm_crtc *crtc,
> >  		return;
> >  	}
> >  
> > -	ret = mtk_crtc_ddp_hw_init(mtk_crtc);
> > +	ret = mtk_crtc_ddp_hw_init(mtk_crtc, state);
> >  	if (ret) {
> >  		pm_runtime_put(comp->dev);
> >  		return;
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > index b1a918ffe457..ef4460f98c07 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > @@ -134,8 +134,8 @@ static int mtk_plane_atomic_async_check(struct
> > drm_plane *plane,
> >  						   true, true);
> >  }
> >  
> > -static void mtk_plane_update_new_state(struct drm_plane_state
> > *new_state,
> > -				       struct mtk_plane_state
> > *mtk_plane_state)
> > +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> > +				struct mtk_plane_state
> > *mtk_plane_state)
> >  {
> >  	struct drm_framebuffer *fb = new_state->fb;
> >  	struct drm_gem_object *gem;
> > @@ -146,6 +146,11 @@ static void mtk_plane_update_new_state(struct
> > drm_plane_state *new_state,
> >  	dma_addr_t hdr_addr = 0;
> >  	unsigned int hdr_pitch = 0;
> >  
> > +	if (!fb) {
> > +		mtk_plane_state->pending.enable = false;
> > +		return;
> > +	}
> 
> This seems done in mtk_plane_atomic_update(), so you may call
> mtk_plane_atomic_update() instead of mtk_plane_update_new_state().

mtk_plane_atomic_update() won't update the mtk_plane_state in mtk_crtc,
so I use mtk_plane_update_new_state() here.

But I found that we can fix up iommu fault issue by handling the NULL
fb case.
So I'll change the previous method to disable the plane if its fb in
new state is NULL.

Regards,
Jason-JH.Lin

> 
> Regards,
> CK
> 
> > +
> >  	gem = fb->obj[0];
> >  	mtk_gem = to_mtk_gem_obj(gem);
> >  	addr = mtk_gem->dma_addr;
> > @@ -180,7 +185,7 @@ static void mtk_plane_update_new_state(struct
> > drm_plane_state *new_state,
> >  		       fb->format->cpp[0] * (x_offset_in_blocks + 1);
> >  	}
> >  
> > -	mtk_plane_state->pending.enable = true;
> > +	mtk_plane_state->pending.enable = new_state->visible;
> >  	mtk_plane_state->pending.pitch = pitch;
> >  	mtk_plane_state->pending.hdr_pitch = hdr_pitch;
> >  	mtk_plane_state->pending.format = format;
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > index 99aff7da0831..0a7d70d13e43 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > @@ -46,6 +46,8 @@ to_mtk_plane_state(struct drm_plane_state *state)
> >  	return container_of(state, struct mtk_plane_state, base);
> >  }
> >  
> > +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> > +				struct mtk_plane_state
> > *mtk_plane_state);
> >  int mtk_plane_init(struct drm_device *dev, struct drm_plane
> > *plane,
> >  		   unsigned long possible_crtcs, enum drm_plane_type
> > type,
> >  		   unsigned int supported_rotations, const u32
> > *formats,
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Jason-JH Lin (林睿祥)]

[-- Attachment #1: Type: text/html, Size: 10770 bytes --]

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

Hi CK,

On Mon, 2023-08-07 at 08:59 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Mon, 2023-08-07 at 09:51 +0800, Jason-JH.Lin wrote:
> > The plane_state of drm_atomic_state is not sync to the
> > mtk_plane_state
> > stored in mtk_crtc during crtc enabling.
> > 
> > So we need to update the mtk_plane_state stored in mtk_crtc by the
> > drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> > 
> > While updating mtk_plane_state, OVL layer should be disabled when
> > the
> > fb
> > in plane_state of drm_atomic_state is NULL.
> > 
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> > MT8173.")
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> > Change in RESEND v4:
> > Remove redundant plane_state assigning.
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
> >  3 files changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index d40142842f85..7db4d6551da7 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl,
> > void *mssg)
> >  }
> >  #endif
> >  
> > -static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> > +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc,
> > struct drm_atomic_state *state)
> >  {
> >  	struct drm_crtc *crtc = &mtk_crtc->base;
> >  	struct drm_connector *connector;
> > @@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct
> > mtk_drm_crtc *mtk_crtc)
> >  	/* Initially configure all planes */
> >  	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> >  		struct drm_plane *plane = &mtk_crtc->planes[i];
> > -		struct mtk_plane_state *plane_state;
> > +		struct drm_plane_state *new_state;
> > +		struct mtk_plane_state *plane_state =
> > to_mtk_plane_state(plane->state);
> >  		struct mtk_ddp_comp *comp;
> >  		unsigned int local_layer;
> >  
> > -		plane_state = to_mtk_plane_state(plane->state);
> > +		/* sync the new plane state from drm_atomic_state */
> > +		if (state->planes[i].ptr) {
> > +			new_state =
> > drm_atomic_get_new_plane_state(state, state->planes[i].ptr);
> 
> for_each_new_plane_in_state()?

I'vd tried for_each_new_plane_in_state(), but it crashed in the fifth
round.
Because mode_config.num_total_plane in this macro is 8, but mtk_crtc-
>layer_nr is 4.

So I think we can't use this macro here.

> 
> > +			mtk_plane_update_new_state(new_state,
> > plane_state);
> > +		}
> > +
> >  		comp = mtk_drm_ddp_comp_for_plane(crtc, plane,
> > &local_layer);
> >  		if (comp)
> >  			mtk_ddp_comp_layer_config(comp, local_layer,
> > @@ -687,7 +693,7 @@ static void mtk_drm_crtc_atomic_enable(struct
> > drm_crtc *crtc,
> >  		return;
> >  	}
> >  
> > -	ret = mtk_crtc_ddp_hw_init(mtk_crtc);
> > +	ret = mtk_crtc_ddp_hw_init(mtk_crtc, state);
> >  	if (ret) {
> >  		pm_runtime_put(comp->dev);
> >  		return;
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > index b1a918ffe457..ef4460f98c07 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > @@ -134,8 +134,8 @@ static int mtk_plane_atomic_async_check(struct
> > drm_plane *plane,
> >  						   true, true);
> >  }
> >  
> > -static void mtk_plane_update_new_state(struct drm_plane_state
> > *new_state,
> > -				       struct mtk_plane_state
> > *mtk_plane_state)
> > +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> > +				struct mtk_plane_state
> > *mtk_plane_state)
> >  {
> >  	struct drm_framebuffer *fb = new_state->fb;
> >  	struct drm_gem_object *gem;
> > @@ -146,6 +146,11 @@ static void mtk_plane_update_new_state(struct
> > drm_plane_state *new_state,
> >  	dma_addr_t hdr_addr = 0;
> >  	unsigned int hdr_pitch = 0;
> >  
> > +	if (!fb) {
> > +		mtk_plane_state->pending.enable = false;
> > +		return;
> > +	}
> 
> This seems done in mtk_plane_atomic_update(), so you may call
> mtk_plane_atomic_update() instead of mtk_plane_update_new_state().

mtk_plane_atomic_update() won't update the mtk_plane_state in mtk_crtc,
so I use mtk_plane_update_new_state() here.

But I found that we can fix up iommu fault issue by handling the NULL
fb case.
So I'll change the previous method to disable the plane if its fb in
new state is NULL.

Regards,
Jason-JH.Lin

> 
> Regards,
> CK
> 
> > +
> >  	gem = fb->obj[0];
> >  	mtk_gem = to_mtk_gem_obj(gem);
> >  	addr = mtk_gem->dma_addr;
> > @@ -180,7 +185,7 @@ static void mtk_plane_update_new_state(struct
> > drm_plane_state *new_state,
> >  		       fb->format->cpp[0] * (x_offset_in_blocks + 1);
> >  	}
> >  
> > -	mtk_plane_state->pending.enable = true;
> > +	mtk_plane_state->pending.enable = new_state->visible;
> >  	mtk_plane_state->pending.pitch = pitch;
> >  	mtk_plane_state->pending.hdr_pitch = hdr_pitch;
> >  	mtk_plane_state->pending.format = format;
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > index 99aff7da0831..0a7d70d13e43 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > @@ -46,6 +46,8 @@ to_mtk_plane_state(struct drm_plane_state *state)
> >  	return container_of(state, struct mtk_plane_state, base);
> >  }
> >  
> > +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> > +				struct mtk_plane_state
> > *mtk_plane_state);
> >  int mtk_plane_init(struct drm_device *dev, struct drm_plane
> > *plane,
> >  		   unsigned long possible_crtcs, enum drm_plane_type
> > type,
> >  		   unsigned int supported_rotations, const u32
> > *formats,

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Eugen Hristev]

Hi Jason,

On 8/7/23 04:51, Jason-JH.Lin wrote:
> The plane_state of drm_atomic_state is not sync to the mtk_plane_state
> stored in mtk_crtc during crtc enabling.
> 
> So we need to update the mtk_plane_state stored in mtk_crtc by the
> drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> 
> While updating mtk_plane_state, OVL layer should be disabled when the fb
> in plane_state of drm_atomic_state is NULL.
> 
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
> Change in RESEND v4:
> Remove redundant plane_state assigning.
> ---
>   drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
>   drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
>   drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
>   3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index d40142842f85..7db4d6551da7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
>   }
>   #endif
>   
> -static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc, struct drm_atomic_state *state)
>   {
>   	struct drm_crtc *crtc = &mtk_crtc->base;
>   	struct drm_connector *connector;
> @@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
>   	/* Initially configure all planes */
>   	for (i = 0; i < mtk_crtc->layer_nr; i++) {
>   		struct drm_plane *plane = &mtk_crtc->planes[i];
> -		struct mtk_plane_state *plane_state;
> +		struct drm_plane_state *new_state;
> +		struct mtk_plane_state *plane_state = to_mtk_plane_state(plane->state);
>   		struct mtk_ddp_comp *comp;
>   		unsigned int local_layer;
>   
> -		plane_state = to_mtk_plane_state(plane->state);

any reason why you moved the initialization of plane_state at the 
declaration phase ?

> +		/* sync the new plane state from drm_atomic_state */
> +		if (state->planes[i].ptr) {
> +			new_state = drm_atomic_get_new_plane_state(state, state->planes[i].ptr);
Can drm_atomic_get_new_plane_state fail ? and new_state becomes null ?

I see mtk_plane_update_new_state assumes new_state being a correct 
state/pointer.

Regards,

> +			mtk_plane_update_new_state(new_state, plane_state);
> +		}
> +
>   		comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
>   		if (comp)
>   			mtk_ddp_comp_layer_config(comp, local_layer,
> @@ -687,7 +693,7 @@ static void mtk_drm_crtc_atomic_enable(struct drm_crtc *crtc,
>   		return;
>   	}
>   
> -	ret = mtk_crtc_ddp_hw_init(mtk_crtc);
> +	ret = mtk_crtc_ddp_hw_init(mtk_crtc, state);
>   	if (ret) {
>   		pm_runtime_put(comp->dev);
>   		return;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index b1a918ffe457..ef4460f98c07 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -134,8 +134,8 @@ static int mtk_plane_atomic_async_check(struct drm_plane *plane,
>   						   true, true);
>   }
>   
> -static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> -				       struct mtk_plane_state *mtk_plane_state)
> +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> +				struct mtk_plane_state *mtk_plane_state)
>   {
>   	struct drm_framebuffer *fb = new_state->fb;
>   	struct drm_gem_object *gem;
> @@ -146,6 +146,11 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
>   	dma_addr_t hdr_addr = 0;
>   	unsigned int hdr_pitch = 0;
>   
> +	if (!fb) {
> +		mtk_plane_state->pending.enable = false;
> +		return;
> +	}
> +
>   	gem = fb->obj[0];
>   	mtk_gem = to_mtk_gem_obj(gem);
>   	addr = mtk_gem->dma_addr;
> @@ -180,7 +185,7 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
>   		       fb->format->cpp[0] * (x_offset_in_blocks + 1);
>   	}
>   
> -	mtk_plane_state->pending.enable = true;
> +	mtk_plane_state->pending.enable = new_state->visible;
>   	mtk_plane_state->pending.pitch = pitch;
>   	mtk_plane_state->pending.hdr_pitch = hdr_pitch;
>   	mtk_plane_state->pending.format = format;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> index 99aff7da0831..0a7d70d13e43 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> @@ -46,6 +46,8 @@ to_mtk_plane_state(struct drm_plane_state *state)
>   	return container_of(state, struct mtk_plane_state, base);
>   }
>   
> +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> +				struct mtk_plane_state *mtk_plane_state);
>   int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>   		   unsigned long possible_crtcs, enum drm_plane_type type,
>   		   unsigned int supported_rotations, const u32 *formats,

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Eugen Hristev]

Hi Jason,

On 8/7/23 04:51, Jason-JH.Lin wrote:
> The plane_state of drm_atomic_state is not sync to the mtk_plane_state
> stored in mtk_crtc during crtc enabling.
> 
> So we need to update the mtk_plane_state stored in mtk_crtc by the
> drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> 
> While updating mtk_plane_state, OVL layer should be disabled when the fb
> in plane_state of drm_atomic_state is NULL.
> 
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
> Change in RESEND v4:
> Remove redundant plane_state assigning.
> ---
>   drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
>   drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
>   drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
>   3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index d40142842f85..7db4d6551da7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
>   }
>   #endif
>   
> -static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc, struct drm_atomic_state *state)
>   {
>   	struct drm_crtc *crtc = &mtk_crtc->base;
>   	struct drm_connector *connector;
> @@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
>   	/* Initially configure all planes */
>   	for (i = 0; i < mtk_crtc->layer_nr; i++) {
>   		struct drm_plane *plane = &mtk_crtc->planes[i];
> -		struct mtk_plane_state *plane_state;
> +		struct drm_plane_state *new_state;
> +		struct mtk_plane_state *plane_state = to_mtk_plane_state(plane->state);
>   		struct mtk_ddp_comp *comp;
>   		unsigned int local_layer;
>   
> -		plane_state = to_mtk_plane_state(plane->state);

any reason why you moved the initialization of plane_state at the 
declaration phase ?

> +		/* sync the new plane state from drm_atomic_state */
> +		if (state->planes[i].ptr) {
> +			new_state = drm_atomic_get_new_plane_state(state, state->planes[i].ptr);
Can drm_atomic_get_new_plane_state fail ? and new_state becomes null ?

I see mtk_plane_update_new_state assumes new_state being a correct 
state/pointer.

Regards,

> +			mtk_plane_update_new_state(new_state, plane_state);
> +		}
> +
>   		comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
>   		if (comp)
>   			mtk_ddp_comp_layer_config(comp, local_layer,
> @@ -687,7 +693,7 @@ static void mtk_drm_crtc_atomic_enable(struct drm_crtc *crtc,
>   		return;
>   	}
>   
> -	ret = mtk_crtc_ddp_hw_init(mtk_crtc);
> +	ret = mtk_crtc_ddp_hw_init(mtk_crtc, state);
>   	if (ret) {
>   		pm_runtime_put(comp->dev);
>   		return;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index b1a918ffe457..ef4460f98c07 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -134,8 +134,8 @@ static int mtk_plane_atomic_async_check(struct drm_plane *plane,
>   						   true, true);
>   }
>   
> -static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> -				       struct mtk_plane_state *mtk_plane_state)
> +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> +				struct mtk_plane_state *mtk_plane_state)
>   {
>   	struct drm_framebuffer *fb = new_state->fb;
>   	struct drm_gem_object *gem;
> @@ -146,6 +146,11 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
>   	dma_addr_t hdr_addr = 0;
>   	unsigned int hdr_pitch = 0;
>   
> +	if (!fb) {
> +		mtk_plane_state->pending.enable = false;
> +		return;
> +	}
> +
>   	gem = fb->obj[0];
>   	mtk_gem = to_mtk_gem_obj(gem);
>   	addr = mtk_gem->dma_addr;
> @@ -180,7 +185,7 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
>   		       fb->format->cpp[0] * (x_offset_in_blocks + 1);
>   	}
>   
> -	mtk_plane_state->pending.enable = true;
> +	mtk_plane_state->pending.enable = new_state->visible;
>   	mtk_plane_state->pending.pitch = pitch;
>   	mtk_plane_state->pending.hdr_pitch = hdr_pitch;
>   	mtk_plane_state->pending.format = format;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> index 99aff7da0831..0a7d70d13e43 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> @@ -46,6 +46,8 @@ to_mtk_plane_state(struct drm_plane_state *state)
>   	return container_of(state, struct mtk_plane_state, base);
>   }
>   
> +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> +				struct mtk_plane_state *mtk_plane_state);
>   int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>   		   unsigned long possible_crtcs, enum drm_plane_type type,
>   		   unsigned int supported_rotations, const u32 *formats,

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Eugen Hristev]

Hi Jason,

On 8/7/23 04:51, Jason-JH.Lin wrote:
> The plane_state of drm_atomic_state is not sync to the mtk_plane_state
> stored in mtk_crtc during crtc enabling.
> 
> So we need to update the mtk_plane_state stored in mtk_crtc by the
> drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> 
> While updating mtk_plane_state, OVL layer should be disabled when the fb
> in plane_state of drm_atomic_state is NULL.
> 
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
> Change in RESEND v4:
> Remove redundant plane_state assigning.
> ---
>   drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
>   drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
>   drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
>   3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index d40142842f85..7db4d6551da7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
>   }
>   #endif
>   
> -static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc, struct drm_atomic_state *state)
>   {
>   	struct drm_crtc *crtc = &mtk_crtc->base;
>   	struct drm_connector *connector;
> @@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
>   	/* Initially configure all planes */
>   	for (i = 0; i < mtk_crtc->layer_nr; i++) {
>   		struct drm_plane *plane = &mtk_crtc->planes[i];
> -		struct mtk_plane_state *plane_state;
> +		struct drm_plane_state *new_state;
> +		struct mtk_plane_state *plane_state = to_mtk_plane_state(plane->state);
>   		struct mtk_ddp_comp *comp;
>   		unsigned int local_layer;
>   
> -		plane_state = to_mtk_plane_state(plane->state);

any reason why you moved the initialization of plane_state at the 
declaration phase ?

> +		/* sync the new plane state from drm_atomic_state */
> +		if (state->planes[i].ptr) {
> +			new_state = drm_atomic_get_new_plane_state(state, state->planes[i].ptr);
Can drm_atomic_get_new_plane_state fail ? and new_state becomes null ?

I see mtk_plane_update_new_state assumes new_state being a correct 
state/pointer.

Regards,

> +			mtk_plane_update_new_state(new_state, plane_state);
> +		}
> +
>   		comp = mtk_drm_ddp_comp_for_plane(crtc, plane, &local_layer);
>   		if (comp)
>   			mtk_ddp_comp_layer_config(comp, local_layer,
> @@ -687,7 +693,7 @@ static void mtk_drm_crtc_atomic_enable(struct drm_crtc *crtc,
>   		return;
>   	}
>   
> -	ret = mtk_crtc_ddp_hw_init(mtk_crtc);
> +	ret = mtk_crtc_ddp_hw_init(mtk_crtc, state);
>   	if (ret) {
>   		pm_runtime_put(comp->dev);
>   		return;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index b1a918ffe457..ef4460f98c07 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -134,8 +134,8 @@ static int mtk_plane_atomic_async_check(struct drm_plane *plane,
>   						   true, true);
>   }
>   
> -static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> -				       struct mtk_plane_state *mtk_plane_state)
> +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> +				struct mtk_plane_state *mtk_plane_state)
>   {
>   	struct drm_framebuffer *fb = new_state->fb;
>   	struct drm_gem_object *gem;
> @@ -146,6 +146,11 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
>   	dma_addr_t hdr_addr = 0;
>   	unsigned int hdr_pitch = 0;
>   
> +	if (!fb) {
> +		mtk_plane_state->pending.enable = false;
> +		return;
> +	}
> +
>   	gem = fb->obj[0];
>   	mtk_gem = to_mtk_gem_obj(gem);
>   	addr = mtk_gem->dma_addr;
> @@ -180,7 +185,7 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
>   		       fb->format->cpp[0] * (x_offset_in_blocks + 1);
>   	}
>   
> -	mtk_plane_state->pending.enable = true;
> +	mtk_plane_state->pending.enable = new_state->visible;
>   	mtk_plane_state->pending.pitch = pitch;
>   	mtk_plane_state->pending.hdr_pitch = hdr_pitch;
>   	mtk_plane_state->pending.format = format;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> index 99aff7da0831..0a7d70d13e43 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> @@ -46,6 +46,8 @@ to_mtk_plane_state(struct drm_plane_state *state)
>   	return container_of(state, struct mtk_plane_state, base);
>   }
>   
> +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> +				struct mtk_plane_state *mtk_plane_state);
>   int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>   		   unsigned long possible_crtcs, enum drm_plane_type type,
>   		   unsigned int supported_rotations, const u32 *formats,


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Jason-JH Lin (林睿祥)]

[-- Attachment #1: Type: text/html, Size: 6112 bytes --]

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

Hi Eugen,

Thanks for the reviews.

On Tue, 2023-08-08 at 14:55 +0300, Eugen Hristev wrote:
> Hi Jason,
> 
> On 8/7/23 04:51, Jason-JH.Lin wrote:
> > The plane_state of drm_atomic_state is not sync to the
> > mtk_plane_state
> > stored in mtk_crtc during crtc enabling.
> > 
> > So we need to update the mtk_plane_state stored in mtk_crtc by the
> > drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> > 
> > While updating mtk_plane_state, OVL layer should be disabled when
> > the fb
> > in plane_state of drm_atomic_state is NULL.
> > 
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> > MT8173.")
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> > Change in RESEND v4:
> > Remove redundant plane_state assigning.
> > ---
> >   drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
> >   drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
> >   drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
> >   3 files changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index d40142842f85..7db4d6551da7 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl,
> > void *mssg)
> >   }
> >   #endif
> >   
> > -static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> > +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc,
> > struct drm_atomic_state *state)
> >   {
> >   	struct drm_crtc *crtc = &mtk_crtc->base;
> >   	struct drm_connector *connector;
> > @@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct
> > mtk_drm_crtc *mtk_crtc)
> >   	/* Initially configure all planes */
> >   	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> >   		struct drm_plane *plane = &mtk_crtc->planes[i];
> > -		struct mtk_plane_state *plane_state;
> > +		struct drm_plane_state *new_state;
> > +		struct mtk_plane_state *plane_state =
> > to_mtk_plane_state(plane->state);
> >   		struct mtk_ddp_comp *comp;
> >   		unsigned int local_layer;
> >   
> > -		plane_state = to_mtk_plane_state(plane->state);
> 
> any reason why you moved the initialization of plane_state at the 
> declaration phase ?
> 
I just like to assign it at the declaration phase.
But it's not related to this fix patch, so I'll move it back.
Thanks.

> > +		/* sync the new plane state from drm_atomic_state */
> > +		if (state->planes[i].ptr) {
> > +			new_state =
> > drm_atomic_get_new_plane_state(state, state->planes[i].ptr);
> 
> Can drm_atomic_get_new_plane_state fail ? and new_state becomes null
> ?

drm_atomic_get_new_plane_state() won't fail and new_state won't be
NULL.

But if state->planes[i].ptr is NULL, it'll crash in drm_plane_index()
inside drm_atomic_get_new_plane_state().

> 
> I see mtk_plane_update_new_state assumes new_state being a correct 
> state/pointer.

The usage is the same as mtk_plane_atomic_update().

Regards,
Jason-JH.Lin

> 
> Regards,
> 

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Jason-JH Lin (林睿祥)]

Hi Eugen,

Thanks for the reviews.

On Tue, 2023-08-08 at 14:55 +0300, Eugen Hristev wrote:
> Hi Jason,
> 
> On 8/7/23 04:51, Jason-JH.Lin wrote:
> > The plane_state of drm_atomic_state is not sync to the
> > mtk_plane_state
> > stored in mtk_crtc during crtc enabling.
> > 
> > So we need to update the mtk_plane_state stored in mtk_crtc by the
> > drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> > 
> > While updating mtk_plane_state, OVL layer should be disabled when
> > the fb
> > in plane_state of drm_atomic_state is NULL.
> > 
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> > MT8173.")
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> > Change in RESEND v4:
> > Remove redundant plane_state assigning.
> > ---
> >   drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
> >   drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
> >   drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
> >   3 files changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index d40142842f85..7db4d6551da7 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl,
> > void *mssg)
> >   }
> >   #endif
> >   
> > -static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> > +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc,
> > struct drm_atomic_state *state)
> >   {
> >   	struct drm_crtc *crtc = &mtk_crtc->base;
> >   	struct drm_connector *connector;
> > @@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct
> > mtk_drm_crtc *mtk_crtc)
> >   	/* Initially configure all planes */
> >   	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> >   		struct drm_plane *plane = &mtk_crtc->planes[i];
> > -		struct mtk_plane_state *plane_state;
> > +		struct drm_plane_state *new_state;
> > +		struct mtk_plane_state *plane_state =
> > to_mtk_plane_state(plane->state);
> >   		struct mtk_ddp_comp *comp;
> >   		unsigned int local_layer;
> >   
> > -		plane_state = to_mtk_plane_state(plane->state);
> 
> any reason why you moved the initialization of plane_state at the 
> declaration phase ?
> 
I just like to assign it at the declaration phase.
But it's not related to this fix patch, so I'll move it back.
Thanks.

> > +		/* sync the new plane state from drm_atomic_state */
> > +		if (state->planes[i].ptr) {
> > +			new_state =
> > drm_atomic_get_new_plane_state(state, state->planes[i].ptr);
> 
> Can drm_atomic_get_new_plane_state fail ? and new_state becomes null
> ?

drm_atomic_get_new_plane_state() won't fail and new_state won't be
NULL.

But if state->planes[i].ptr is NULL, it'll crash in drm_plane_index()
inside drm_atomic_get_new_plane_state().

> 
> I see mtk_plane_update_new_state assumes new_state being a correct 
> state/pointer.

The usage is the same as mtk_plane_atomic_update().

Regards,
Jason-JH.Lin

> 
> Regards,
> 

Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Jason-JH Lin (林睿祥)]

Hi Eugen,

Thanks for the reviews.

On Tue, 2023-08-08 at 14:55 +0300, Eugen Hristev wrote:
> Hi Jason,
> 
> On 8/7/23 04:51, Jason-JH.Lin wrote:
> > The plane_state of drm_atomic_state is not sync to the
> > mtk_plane_state
> > stored in mtk_crtc during crtc enabling.
> > 
> > So we need to update the mtk_plane_state stored in mtk_crtc by the
> > drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> > 
> > While updating mtk_plane_state, OVL layer should be disabled when
> > the fb
> > in plane_state of drm_atomic_state is NULL.
> > 
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> > MT8173.")
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> > Change in RESEND v4:
> > Remove redundant plane_state assigning.
> > ---
> >   drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
> >   drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
> >   drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
> >   3 files changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index d40142842f85..7db4d6551da7 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl,
> > void *mssg)
> >   }
> >   #endif
> >   
> > -static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> > +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc,
> > struct drm_atomic_state *state)
> >   {
> >   	struct drm_crtc *crtc = &mtk_crtc->base;
> >   	struct drm_connector *connector;
> > @@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct
> > mtk_drm_crtc *mtk_crtc)
> >   	/* Initially configure all planes */
> >   	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> >   		struct drm_plane *plane = &mtk_crtc->planes[i];
> > -		struct mtk_plane_state *plane_state;
> > +		struct drm_plane_state *new_state;
> > +		struct mtk_plane_state *plane_state =
> > to_mtk_plane_state(plane->state);
> >   		struct mtk_ddp_comp *comp;
> >   		unsigned int local_layer;
> >   
> > -		plane_state = to_mtk_plane_state(plane->state);
> 
> any reason why you moved the initialization of plane_state at the 
> declaration phase ?
> 
I just like to assign it at the declaration phase.
But it's not related to this fix patch, so I'll move it back.
Thanks.

> > +		/* sync the new plane state from drm_atomic_state */
> > +		if (state->planes[i].ptr) {
> > +			new_state =
> > drm_atomic_get_new_plane_state(state, state->planes[i].ptr);
> 
> Can drm_atomic_get_new_plane_state fail ? and new_state becomes null
> ?

drm_atomic_get_new_plane_state() won't fail and new_state won't be
NULL.

But if state->planes[i].ptr is NULL, it'll crash in drm_plane_index()
inside drm_atomic_get_new_plane_state().

> 
> I see mtk_plane_update_new_state assumes new_state being a correct 
> state/pointer.

The usage is the same as mtk_plane_atomic_update().

Regards,
Jason-JH.Lin

> 
> Regards,
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel