[v5, PATCH] drm/mediatek: add dma buffer control for drm plane disable

[v5, PATCH] drm/mediatek: add dma buffer control for drm plane disable

[Yongqiang Niu]

dma buffer release before overlay disable, that will cause
m4u translation fault warning.

add dma buffer control flow in mediatek driver:
get dma buffer when drm plane disable
put dma buffer when overlay really disable

Fixes: 41016fe1028e ("drm: Rename plane->state variables in atomic update and disable")
Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 25 ++++++++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  1 +
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 12 ++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_plane.h |  1 +
 4 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index d40142842f85..49d671100785 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/dma-buf.h>
 #include <linux/dma-mapping.h>
 #include <linux/mailbox_controller.h>
 #include <linux/pm_runtime.h>
@@ -283,6 +284,23 @@ struct mtk_ddp_comp *mtk_drm_ddp_comp_for_plane(struct drm_crtc *crtc,
 	return NULL;
 }
 
+static void mtk_drm_dma_buf_put(struct mtk_drm_crtc *mtk_crtc)
+{
+	unsigned int i;
+
+	for (i = 0; i < mtk_crtc->layer_nr; i++) {
+		struct drm_plane *plane = &mtk_crtc->planes[i];
+		struct mtk_plane_state *plane_state;
+
+		plane_state = to_mtk_plane_state(plane->state);
+
+		if (plane_state && plane_state->pending.dma_buf) {
+			dma_buf_put(plane_state->pending.dma_buf);
+			plane_state->pending.dma_buf = NULL;
+		}
+	}
+}
+
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
 static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
 {
@@ -323,6 +341,8 @@ static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
 		mtk_crtc->pending_async_planes = false;
 	}
 
+	mtk_drm_dma_buf_put(mtk_crtc);
+
 	mtk_crtc->cmdq_vblank_cnt = 0;
 	wake_up(&mtk_crtc->cb_blocking_queue);
 }
@@ -624,9 +644,14 @@ static void mtk_crtc_ddp_irq(void *data)
 	else if (mtk_crtc->cmdq_vblank_cnt > 0 && --mtk_crtc->cmdq_vblank_cnt == 0)
 		DRM_ERROR("mtk_crtc %d CMDQ execute command timeout!\n",
 			  drm_crtc_index(&mtk_crtc->base));
+
+	if (!mtk_crtc->cmdq_client.chan)
+		mtk_drm_dma_buf_put(mtk_crtc);
 #else
 	if (!priv->data->shadow_register)
 		mtk_crtc_ddp_config(crtc, NULL);
+
+	mtk_drm_dma_buf_put(mtk_crtc);
 #endif
 	mtk_drm_finish_page_flip(mtk_crtc);
 }
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 6dcb4ba2466c..812f1667e070 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -993,4 +993,5 @@ module_exit(mtk_drm_exit);
 
 MODULE_AUTHOR("YT SHEN <yt.shen@mediatek.com>");
 MODULE_DESCRIPTION("Mediatek SoC DRM driver");
+MODULE_IMPORT_NS(DMA_BUF);
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index 31f9420aff6f..66e6393e45ee 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -12,6 +12,7 @@
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <linux/align.h>
+#include <linux/dma-buf.h>
 
 #include "mtk_drm_crtc.h"
 #include "mtk_drm_ddp_comp.h"
@@ -266,6 +267,17 @@ static void mtk_plane_atomic_disable(struct drm_plane *plane,
 	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
 									   plane);
 	struct mtk_plane_state *mtk_plane_state = to_mtk_plane_state(new_state);
+	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
+									   plane);
+
+	if (old_state && old_state->fb) {
+		struct drm_gem_object *gem = old_state->fb->obj[0];
+
+		if (gem && gem->dma_buf) {
+			get_dma_buf(gem->dma_buf);
+			mtk_plane_state->pending.dma_buf = gem->dma_buf;
+		}
+	}
 	mtk_plane_state->pending.enable = false;
 	wmb(); /* Make sure the above parameter is set before update */
 	mtk_plane_state->pending.dirty = true;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
index 99aff7da0831..3aba0b58ef3c 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
@@ -33,6 +33,7 @@ struct mtk_plane_pending_state {
 	bool				async_dirty;
 	bool				async_config;
 	enum drm_color_encoding		color_encoding;
+	struct dma_buf			*dma_buf;
 };
 
 struct mtk_plane_state {
-- 
2.25.1

[v5, PATCH] drm/mediatek: add dma buffer control for drm plane disable

[Yongqiang Niu]

dma buffer release before overlay disable, that will cause
m4u translation fault warning.

add dma buffer control flow in mediatek driver:
get dma buffer when drm plane disable
put dma buffer when overlay really disable

Fixes: 41016fe1028e ("drm: Rename plane->state variables in atomic update and disable")
Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 25 ++++++++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  1 +
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 12 ++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_plane.h |  1 +
 4 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index d40142842f85..49d671100785 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/dma-buf.h>
 #include <linux/dma-mapping.h>
 #include <linux/mailbox_controller.h>
 #include <linux/pm_runtime.h>
@@ -283,6 +284,23 @@ struct mtk_ddp_comp *mtk_drm_ddp_comp_for_plane(struct drm_crtc *crtc,
 	return NULL;
 }
 
+static void mtk_drm_dma_buf_put(struct mtk_drm_crtc *mtk_crtc)
+{
+	unsigned int i;
+
+	for (i = 0; i < mtk_crtc->layer_nr; i++) {
+		struct drm_plane *plane = &mtk_crtc->planes[i];
+		struct mtk_plane_state *plane_state;
+
+		plane_state = to_mtk_plane_state(plane->state);
+
+		if (plane_state && plane_state->pending.dma_buf) {
+			dma_buf_put(plane_state->pending.dma_buf);
+			plane_state->pending.dma_buf = NULL;
+		}
+	}
+}
+
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
 static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
 {
@@ -323,6 +341,8 @@ static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
 		mtk_crtc->pending_async_planes = false;
 	}
 
+	mtk_drm_dma_buf_put(mtk_crtc);
+
 	mtk_crtc->cmdq_vblank_cnt = 0;
 	wake_up(&mtk_crtc->cb_blocking_queue);
 }
@@ -624,9 +644,14 @@ static void mtk_crtc_ddp_irq(void *data)
 	else if (mtk_crtc->cmdq_vblank_cnt > 0 && --mtk_crtc->cmdq_vblank_cnt == 0)
 		DRM_ERROR("mtk_crtc %d CMDQ execute command timeout!\n",
 			  drm_crtc_index(&mtk_crtc->base));
+
+	if (!mtk_crtc->cmdq_client.chan)
+		mtk_drm_dma_buf_put(mtk_crtc);
 #else
 	if (!priv->data->shadow_register)
 		mtk_crtc_ddp_config(crtc, NULL);
+
+	mtk_drm_dma_buf_put(mtk_crtc);
 #endif
 	mtk_drm_finish_page_flip(mtk_crtc);
 }
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 6dcb4ba2466c..812f1667e070 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -993,4 +993,5 @@ module_exit(mtk_drm_exit);
 
 MODULE_AUTHOR("YT SHEN <yt.shen@mediatek.com>");
 MODULE_DESCRIPTION("Mediatek SoC DRM driver");
+MODULE_IMPORT_NS(DMA_BUF);
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index 31f9420aff6f..66e6393e45ee 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -12,6 +12,7 @@
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <linux/align.h>
+#include <linux/dma-buf.h>
 
 #include "mtk_drm_crtc.h"
 #include "mtk_drm_ddp_comp.h"
@@ -266,6 +267,17 @@ static void mtk_plane_atomic_disable(struct drm_plane *plane,
 	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
 									   plane);
 	struct mtk_plane_state *mtk_plane_state = to_mtk_plane_state(new_state);
+	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
+									   plane);
+
+	if (old_state && old_state->fb) {
+		struct drm_gem_object *gem = old_state->fb->obj[0];
+
+		if (gem && gem->dma_buf) {
+			get_dma_buf(gem->dma_buf);
+			mtk_plane_state->pending.dma_buf = gem->dma_buf;
+		}
+	}
 	mtk_plane_state->pending.enable = false;
 	wmb(); /* Make sure the above parameter is set before update */
 	mtk_plane_state->pending.dirty = true;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
index 99aff7da0831..3aba0b58ef3c 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
@@ -33,6 +33,7 @@ struct mtk_plane_pending_state {
 	bool				async_dirty;
 	bool				async_config;
 	enum drm_color_encoding		color_encoding;
+	struct dma_buf			*dma_buf;
 };
 
 struct mtk_plane_state {
-- 
2.25.1


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

[v5, PATCH] drm/mediatek: add dma buffer control for drm plane disable

[Yongqiang Niu]

dma buffer release before overlay disable, that will cause
m4u translation fault warning.

add dma buffer control flow in mediatek driver:
get dma buffer when drm plane disable
put dma buffer when overlay really disable

Fixes: 41016fe1028e ("drm: Rename plane->state variables in atomic update and disable")
Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 25 ++++++++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  1 +
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 12 ++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_plane.h |  1 +
 4 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index d40142842f85..49d671100785 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/dma-buf.h>
 #include <linux/dma-mapping.h>
 #include <linux/mailbox_controller.h>
 #include <linux/pm_runtime.h>
@@ -283,6 +284,23 @@ struct mtk_ddp_comp *mtk_drm_ddp_comp_for_plane(struct drm_crtc *crtc,
 	return NULL;
 }
 
+static void mtk_drm_dma_buf_put(struct mtk_drm_crtc *mtk_crtc)
+{
+	unsigned int i;
+
+	for (i = 0; i < mtk_crtc->layer_nr; i++) {
+		struct drm_plane *plane = &mtk_crtc->planes[i];
+		struct mtk_plane_state *plane_state;
+
+		plane_state = to_mtk_plane_state(plane->state);
+
+		if (plane_state && plane_state->pending.dma_buf) {
+			dma_buf_put(plane_state->pending.dma_buf);
+			plane_state->pending.dma_buf = NULL;
+		}
+	}
+}
+
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
 static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
 {
@@ -323,6 +341,8 @@ static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
 		mtk_crtc->pending_async_planes = false;
 	}
 
+	mtk_drm_dma_buf_put(mtk_crtc);
+
 	mtk_crtc->cmdq_vblank_cnt = 0;
 	wake_up(&mtk_crtc->cb_blocking_queue);
 }
@@ -624,9 +644,14 @@ static void mtk_crtc_ddp_irq(void *data)
 	else if (mtk_crtc->cmdq_vblank_cnt > 0 && --mtk_crtc->cmdq_vblank_cnt == 0)
 		DRM_ERROR("mtk_crtc %d CMDQ execute command timeout!\n",
 			  drm_crtc_index(&mtk_crtc->base));
+
+	if (!mtk_crtc->cmdq_client.chan)
+		mtk_drm_dma_buf_put(mtk_crtc);
 #else
 	if (!priv->data->shadow_register)
 		mtk_crtc_ddp_config(crtc, NULL);
+
+	mtk_drm_dma_buf_put(mtk_crtc);
 #endif
 	mtk_drm_finish_page_flip(mtk_crtc);
 }
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 6dcb4ba2466c..812f1667e070 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -993,4 +993,5 @@ module_exit(mtk_drm_exit);
 
 MODULE_AUTHOR("YT SHEN <yt.shen@mediatek.com>");
 MODULE_DESCRIPTION("Mediatek SoC DRM driver");
+MODULE_IMPORT_NS(DMA_BUF);
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index 31f9420aff6f..66e6393e45ee 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -12,6 +12,7 @@
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <linux/align.h>
+#include <linux/dma-buf.h>
 
 #include "mtk_drm_crtc.h"
 #include "mtk_drm_ddp_comp.h"
@@ -266,6 +267,17 @@ static void mtk_plane_atomic_disable(struct drm_plane *plane,
 	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
 									   plane);
 	struct mtk_plane_state *mtk_plane_state = to_mtk_plane_state(new_state);
+	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
+									   plane);
+
+	if (old_state && old_state->fb) {
+		struct drm_gem_object *gem = old_state->fb->obj[0];
+
+		if (gem && gem->dma_buf) {
+			get_dma_buf(gem->dma_buf);
+			mtk_plane_state->pending.dma_buf = gem->dma_buf;
+		}
+	}
 	mtk_plane_state->pending.enable = false;
 	wmb(); /* Make sure the above parameter is set before update */
 	mtk_plane_state->pending.dirty = true;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
index 99aff7da0831..3aba0b58ef3c 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
@@ -33,6 +33,7 @@ struct mtk_plane_pending_state {
 	bool				async_dirty;
 	bool				async_config;
 	enum drm_color_encoding		color_encoding;
+	struct dma_buf			*dma_buf;
 };
 
 struct mtk_plane_state {
-- 
2.25.1

Re: [v5, PATCH] drm/mediatek: add dma buffer control for drm plane disable

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 658 bytes --]

Hi,

On Wed, Jul 05, 2023 at 02:07:18PM +0800, Yongqiang Niu wrote:
> dma buffer release before overlay disable, that will cause
> m4u translation fault warning.
> 
> add dma buffer control flow in mediatek driver:
> get dma buffer when drm plane disable
> put dma buffer when overlay really disable
> 
> Fixes: 41016fe1028e ("drm: Rename plane->state variables in atomic update and disable")
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>

I think we need more details in the commit message about what the issue
is exactly and how it's fixed.

This definitely feels like it's not something drivers should have to do.

Maxime

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

Re: [v5, PATCH] drm/mediatek: add dma buffer control for drm plane disable

[Maxime Ripard]

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

Hi,

On Wed, Jul 05, 2023 at 02:07:18PM +0800, Yongqiang Niu wrote:
> dma buffer release before overlay disable, that will cause
> m4u translation fault warning.
> 
> add dma buffer control flow in mediatek driver:
> get dma buffer when drm plane disable
> put dma buffer when overlay really disable
> 
> Fixes: 41016fe1028e ("drm: Rename plane->state variables in atomic update and disable")
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>

I think we need more details in the commit message about what the issue
is exactly and how it's fixed.

This definitely feels like it's not something drivers should have to do.

Maxime

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

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

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

Re: [v5, PATCH] drm/mediatek: add dma buffer control for drm plane disable

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 658 bytes --]

Hi,

On Wed, Jul 05, 2023 at 02:07:18PM +0800, Yongqiang Niu wrote:
> dma buffer release before overlay disable, that will cause
> m4u translation fault warning.
> 
> add dma buffer control flow in mediatek driver:
> get dma buffer when drm plane disable
> put dma buffer when overlay really disable
> 
> Fixes: 41016fe1028e ("drm: Rename plane->state variables in atomic update and disable")
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>

I think we need more details in the commit message about what the issue
is exactly and how it's fixed.

This definitely feels like it's not something drivers should have to do.

Maxime

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

Re: [v5, PATCH] drm/mediatek: add dma buffer control for drm plane disable

[CK Hu (胡俊光)]

Hi, Maxime:

On Wed, 2023-07-05 at 09:48 +0200, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Jul 05, 2023 at 02:07:18PM +0800, Yongqiang Niu wrote:
> > dma buffer release before overlay disable, that will cause
> > m4u translation fault warning.
> > 
> > add dma buffer control flow in mediatek driver:
> > get dma buffer when drm plane disable
> > put dma buffer when overlay really disable
> > 
> > Fixes: 41016fe1028e ("drm: Rename plane->state variables in atomic
> > update and disable")
> > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> 
> I think we need more details in the commit message about what the
> issue
> is exactly and how it's fixed.
> 
> This definitely feels like it's not something drivers should have to
> do.

Because MediaTek display hardware has no internal SRAM which could
store the on screen buffer, plane should be updated in vblank period to
prevent tearing effect. In MediaTek plane disable callback function, if
we wait vblank period to disable plane, the software would be blocked
for a long time. If there are four planes to be disabled, the total
blocking time would be almost 4 vsync time. So this patch is just a
work around to use get_dma_buf() to prevent buffer to be freed before
vblank period and not block the software. For the behavior DRM core
free plane buffer just after plane disable callback function return,
I'm not sure it's DRM core's bug or it's DRM core's correct behavior.
If this is correct behavior, it means that DRM core assume that
hardware should have internal SRAM but MediaTek display hardware
violate this assumption. If so, I think I could just accept this work
around patch.

Regards,
CK


> 
> Maxime

Re: [v5, PATCH] drm/mediatek: add dma buffer control for drm plane disable

[CK Hu (胡俊光)]

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

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

Hi, Maxime:

On Wed, 2023-07-05 at 09:48 +0200, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Jul 05, 2023 at 02:07:18PM +0800, Yongqiang Niu wrote:
> > dma buffer release before overlay disable, that will cause
> > m4u translation fault warning.
> > 
> > add dma buffer control flow in mediatek driver:
> > get dma buffer when drm plane disable
> > put dma buffer when overlay really disable
> > 
> > Fixes: 41016fe1028e ("drm: Rename plane->state variables in atomic
> > update and disable")
> > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> 
> I think we need more details in the commit message about what the
> issue
> is exactly and how it's fixed.
> 
> This definitely feels like it's not something drivers should have to
> do.

Because MediaTek display hardware has no internal SRAM which could
store the on screen buffer, plane should be updated in vblank period to
prevent tearing effect. In MediaTek plane disable callback function, if
we wait vblank period to disable plane, the software would be blocked
for a long time. If there are four planes to be disabled, the total
blocking time would be almost 4 vsync time. So this patch is just a
work around to use get_dma_buf() to prevent buffer to be freed before
vblank period and not block the software. For the behavior DRM core
free plane buffer just after plane disable callback function return,
I'm not sure it's DRM core's bug or it's DRM core's correct behavior.
If this is correct behavior, it means that DRM core assume that
hardware should have internal SRAM but MediaTek display hardware
violate this assumption. If so, I think I could just accept this work
around patch.

Regards,
CK


> 
> Maxime

Re: [v5, PATCH] drm/mediatek: add dma buffer control for drm plane disable

[CK Hu (胡俊光)]

Hi, Maxime:

On Wed, 2023-07-05 at 09:48 +0200, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Jul 05, 2023 at 02:07:18PM +0800, Yongqiang Niu wrote:
> > dma buffer release before overlay disable, that will cause
> > m4u translation fault warning.
> > 
> > add dma buffer control flow in mediatek driver:
> > get dma buffer when drm plane disable
> > put dma buffer when overlay really disable
> > 
> > Fixes: 41016fe1028e ("drm: Rename plane->state variables in atomic
> > update and disable")
> > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> 
> I think we need more details in the commit message about what the
> issue
> is exactly and how it's fixed.
> 
> This definitely feels like it's not something drivers should have to
> do.

Because MediaTek display hardware has no internal SRAM which could
store the on screen buffer, plane should be updated in vblank period to
prevent tearing effect. In MediaTek plane disable callback function, if
we wait vblank period to disable plane, the software would be blocked
for a long time. If there are four planes to be disabled, the total
blocking time would be almost 4 vsync time. So this patch is just a
work around to use get_dma_buf() to prevent buffer to be freed before
vblank period and not block the software. For the behavior DRM core
free plane buffer just after plane disable callback function return,
I'm not sure it's DRM core's bug or it's DRM core's correct behavior.
If this is correct behavior, it means that DRM core assume that
hardware should have internal SRAM but MediaTek display hardware
violate this assumption. If so, I think I could just accept this work
around patch.

Regards,
CK


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

Re: [v5, PATCH] drm/mediatek: add dma buffer control for drm plane disable

[CK Hu (胡俊光)]

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

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

Hi, Maxime:

On Tue, 2023-08-01 at 16:02 +0800, CK Hu wrote:
> Hi, Maxime:
> 
> On Wed, 2023-07-05 at 09:48 +0200, Maxime Ripard wrote:
> > Hi,
> > 
> > On Wed, Jul 05, 2023 at 02:07:18PM +0800, Yongqiang Niu wrote:
> > > dma buffer release before overlay disable, that will cause
> > > m4u translation fault warning.
> > > 
> > > add dma buffer control flow in mediatek driver:
> > > get dma buffer when drm plane disable
> > > put dma buffer when overlay really disable
> > > 
> > > Fixes: 41016fe1028e ("drm: Rename plane->state variables in
> > > atomic
> > > update and disable")
> > > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> > 
> > I think we need more details in the commit message about what the
> > issue
> > is exactly and how it's fixed.
> > 
> > This definitely feels like it's not something drivers should have
> > to
> > do.
> 
> Because MediaTek display hardware has no internal SRAM which could
> store the on screen buffer, plane should be updated in vblank period
> to
> prevent tearing effect. In MediaTek plane disable callback function,
> if
> we wait vblank period to disable plane, the software would be blocked
> for a long time. If there are four planes to be disabled, the total
> blocking time would be almost 4 vsync time. So this patch is just a
> work around to use get_dma_buf() to prevent buffer to be freed before
> vblank period and not block the software. For the behavior DRM core
> free plane buffer just after plane disable callback function return,
> I'm not sure it's DRM core's bug or it's DRM core's correct behavior.
> If this is correct behavior, it means that DRM core assume that
> hardware should have internal SRAM but MediaTek display hardware
> violate this assumption. If so, I think I could just accept this work
> around patch.

Sorry. I've found that drm_atomic_helper_commit_tail() has wait for
vblank after commit. So I think that's MediaTek driver's problem.
MediaTek driver should promise that configuration should take effect
before send vblank event.

Regards,
CK

> 
> Regards,
> CK
> 
> 
> > 
> > Maxime

Re: [v5, PATCH] drm/mediatek: add dma buffer control for drm plane disable

[CK Hu (胡俊光)]

Hi, Maxime:

On Tue, 2023-08-01 at 16:02 +0800, CK Hu wrote:
> Hi, Maxime:
> 
> On Wed, 2023-07-05 at 09:48 +0200, Maxime Ripard wrote:
> > Hi,
> > 
> > On Wed, Jul 05, 2023 at 02:07:18PM +0800, Yongqiang Niu wrote:
> > > dma buffer release before overlay disable, that will cause
> > > m4u translation fault warning.
> > > 
> > > add dma buffer control flow in mediatek driver:
> > > get dma buffer when drm plane disable
> > > put dma buffer when overlay really disable
> > > 
> > > Fixes: 41016fe1028e ("drm: Rename plane->state variables in
> > > atomic
> > > update and disable")
> > > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> > 
> > I think we need more details in the commit message about what the
> > issue
> > is exactly and how it's fixed.
> > 
> > This definitely feels like it's not something drivers should have
> > to
> > do.
> 
> Because MediaTek display hardware has no internal SRAM which could
> store the on screen buffer, plane should be updated in vblank period
> to
> prevent tearing effect. In MediaTek plane disable callback function,
> if
> we wait vblank period to disable plane, the software would be blocked
> for a long time. If there are four planes to be disabled, the total
> blocking time would be almost 4 vsync time. So this patch is just a
> work around to use get_dma_buf() to prevent buffer to be freed before
> vblank period and not block the software. For the behavior DRM core
> free plane buffer just after plane disable callback function return,
> I'm not sure it's DRM core's bug or it's DRM core's correct behavior.
> If this is correct behavior, it means that DRM core assume that
> hardware should have internal SRAM but MediaTek display hardware
> violate this assumption. If so, I think I could just accept this work
> around patch.

Sorry. I've found that drm_atomic_helper_commit_tail() has wait for
vblank after commit. So I think that's MediaTek driver's problem.
MediaTek driver should promise that configuration should take effect
before send vblank event.

Regards,
CK

> 
> Regards,
> CK
> 
> 
> > 
> > Maxime

Re: [v5, PATCH] drm/mediatek: add dma buffer control for drm plane disable

[CK Hu (胡俊光)]

Hi, Maxime:

On Tue, 2023-08-01 at 16:02 +0800, CK Hu wrote:
> Hi, Maxime:
> 
> On Wed, 2023-07-05 at 09:48 +0200, Maxime Ripard wrote:
> > Hi,
> > 
> > On Wed, Jul 05, 2023 at 02:07:18PM +0800, Yongqiang Niu wrote:
> > > dma buffer release before overlay disable, that will cause
> > > m4u translation fault warning.
> > > 
> > > add dma buffer control flow in mediatek driver:
> > > get dma buffer when drm plane disable
> > > put dma buffer when overlay really disable
> > > 
> > > Fixes: 41016fe1028e ("drm: Rename plane->state variables in
> > > atomic
> > > update and disable")
> > > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> > 
> > I think we need more details in the commit message about what the
> > issue
> > is exactly and how it's fixed.
> > 
> > This definitely feels like it's not something drivers should have
> > to
> > do.
> 
> Because MediaTek display hardware has no internal SRAM which could
> store the on screen buffer, plane should be updated in vblank period
> to
> prevent tearing effect. In MediaTek plane disable callback function,
> if
> we wait vblank period to disable plane, the software would be blocked
> for a long time. If there are four planes to be disabled, the total
> blocking time would be almost 4 vsync time. So this patch is just a
> work around to use get_dma_buf() to prevent buffer to be freed before
> vblank period and not block the software. For the behavior DRM core
> free plane buffer just after plane disable callback function return,
> I'm not sure it's DRM core's bug or it's DRM core's correct behavior.
> If this is correct behavior, it means that DRM core assume that
> hardware should have internal SRAM but MediaTek display hardware
> violate this assumption. If so, I think I could just accept this work
> around patch.

Sorry. I've found that drm_atomic_helper_commit_tail() has wait for
vblank after commit. So I think that's MediaTek driver's problem.
MediaTek driver should promise that configuration should take effect
before send vblank event.

Regards,
CK

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