[PATCH] drm/mediatek: Add wait_event_timeout when disabling plane

[PATCH] drm/mediatek: Add wait_event_timeout when disabling plane

[Jason-JH Lin]

Our hardware registers are set through GCE, not by the CPU.
DRM might assume the hardware is disabled immediately after calling
atomic_disable() of drm_plane, but it is only truly disabled after the
GCE IRQ is triggered.

Additionally, the cursor plane in DRM uses async_commit, so DRM will
not wait for vblank and will free the buffer immediately after calling
atomic_disable().

To prevent the framebuffer from being freed before the layer disable
settings are configured into the hardware, which can cause an IOMMU
fault error, a wait_event_timeout has been added to wait for the
ddp_cmdq_cb() callback,indicating that the GCE IRQ has been triggered.

Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_crtc.c  | 30 ++++++++++++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_crtc.h  |  1 +
 drivers/gpu/drm/mediatek/mtk_plane.c |  5 +++++
 3 files changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
index 8f6fba4217ec..944a3d1e5ec9 100644
--- a/drivers/gpu/drm/mediatek/mtk_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_crtc.c
@@ -719,6 +719,36 @@ int mtk_crtc_plane_check(struct drm_crtc *crtc, struct drm_plane *plane,
 	return 0;
 }
 
+void mtk_crtc_plane_disable(struct drm_crtc *crtc, struct drm_plane *plane)
+{
+	struct mtk_crtc *mtk_crtc = to_mtk_crtc(crtc);
+	struct mtk_plane_state *plane_state = to_mtk_plane_state(plane->state);
+	int i;
+
+	if (!mtk_crtc->enabled)
+		return;
+
+	/* set pending plane state to disabled */
+	for (i = 0; i < mtk_crtc->layer_nr; i++) {
+		struct drm_plane *mtk_plane = &mtk_crtc->planes[i];
+		struct mtk_plane_state *mtk_plane_state = to_mtk_plane_state(mtk_plane->state);
+
+		if (mtk_plane->index == plane->index) {
+			memcpy(mtk_plane_state, plane_state, sizeof(*plane_state));
+			break;
+		}
+	}
+	mtk_crtc_update_config(mtk_crtc, false);
+
+#if IS_REACHABLE(CONFIG_MTK_CMDQ)
+	/* wait for planes to be disabled by cmdq */
+	if (mtk_crtc->cmdq_client.chan)
+		wait_event_timeout(mtk_crtc->cb_blocking_queue,
+				   mtk_crtc->cmdq_vblank_cnt == 0,
+				   msecs_to_jiffies(500));
+#endif
+}
+
 void mtk_crtc_async_update(struct drm_crtc *crtc, struct drm_plane *plane,
 			   struct drm_atomic_state *state)
 {
diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.h b/drivers/gpu/drm/mediatek/mtk_crtc.h
index 388e900b6f4d..828f109b83e7 100644
--- a/drivers/gpu/drm/mediatek/mtk_crtc.h
+++ b/drivers/gpu/drm/mediatek/mtk_crtc.h
@@ -21,6 +21,7 @@ int mtk_crtc_create(struct drm_device *drm_dev, const unsigned int *path,
 		    unsigned int num_conn_routes);
 int mtk_crtc_plane_check(struct drm_crtc *crtc, struct drm_plane *plane,
 			 struct mtk_plane_state *state);
+void mtk_crtc_plane_disable(struct drm_crtc *crtc, struct drm_plane *plane);
 void mtk_crtc_async_update(struct drm_crtc *crtc, struct drm_plane *plane,
 			   struct drm_atomic_state *plane_state);
 struct device *mtk_crtc_dma_dev_get(struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/mediatek/mtk_plane.c b/drivers/gpu/drm/mediatek/mtk_plane.c
index 655106bbb76d..59edbe26f01e 100644
--- a/drivers/gpu/drm/mediatek/mtk_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_plane.c
@@ -285,9 +285,14 @@ 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);
+
 	mtk_plane_state->pending.enable = false;
 	wmb(); /* Make sure the above parameter is set before update */
 	mtk_plane_state->pending.dirty = true;
+
+	mtk_crtc_plane_disable(old_state->crtc, plane);
 }
 
 static void mtk_plane_atomic_update(struct drm_plane *plane,
-- 
2.43.0

Re: [PATCH] drm/mediatek: Add wait_event_timeout when disabling plane

[AngeloGioacchino Del Regno]

Il 22/05/25 10:34, Jason-JH Lin ha scritto:
> Our hardware registers are set through GCE, not by the CPU.
> DRM might assume the hardware is disabled immediately after calling
> atomic_disable() of drm_plane, but it is only truly disabled after the
> GCE IRQ is triggered.
> 
> Additionally, the cursor plane in DRM uses async_commit, so DRM will
> not wait for vblank and will free the buffer immediately after calling
> atomic_disable().
> 
> To prevent the framebuffer from being freed before the layer disable
> settings are configured into the hardware, which can cause an IOMMU
> fault error, a wait_event_timeout has been added to wait for the
> ddp_cmdq_cb() callback,indicating that the GCE IRQ has been triggered.
> 
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
> Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH] drm/mediatek: Add wait_event_timeout when disabling plane

[Daniel Stone]

Hi Jason,

On Thu, 22 May 2025 at 09:52, Jason-JH Lin <jason-jh.lin@mediatek.com> wrote:
> Our hardware registers are set through GCE, not by the CPU.
> DRM might assume the hardware is disabled immediately after calling
> atomic_disable() of drm_plane, but it is only truly disabled after the
> GCE IRQ is triggered.
>
> Additionally, the cursor plane in DRM uses async_commit, so DRM will
> not wait for vblank and will free the buffer immediately after calling
> atomic_disable().
>
> To prevent the framebuffer from being freed before the layer disable
> settings are configured into the hardware, which can cause an IOMMU
> fault error, a wait_event_timeout has been added to wait for the
> ddp_cmdq_cb() callback,indicating that the GCE IRQ has been triggered.

Waiting up to 500ms for each plane to be disabled is ... not ideal.
Especially as multiple planes can be disabled at once. This may happen
dynamically during runtime, e.g. when a video is playing and a user
moves their cursor over the plane to make the UI controls visible.

I think this should be handled through the atomic_commit() handler,
with asynchronous tracking of the state, instead of the hard block
here.

Cheers,
Daniel

Re: [PATCH] drm/mediatek: Add wait_event_timeout when disabling plane

[Jason-JH Lin (林睿祥)]

Hi Daniel,

Thanks for the review.

On Sat, 2025-05-24 at 13:47 +0100, Daniel Stone wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Hi Jason,
> 
> On Thu, 22 May 2025 at 09:52, Jason-JH Lin
> <jason-jh.lin@mediatek.com> wrote:
> > Our hardware registers are set through GCE, not by the CPU.
> > DRM might assume the hardware is disabled immediately after calling
> > atomic_disable() of drm_plane, but it is only truly disabled after
> > the
> > GCE IRQ is triggered.
> > 
> > Additionally, the cursor plane in DRM uses async_commit, so DRM
> > will
> > not wait for vblank and will free the buffer immediately after
> > calling
> > atomic_disable().
> > 
> > To prevent the framebuffer from being freed before the layer
> > disable
> > settings are configured into the hardware, which can cause an IOMMU
> > fault error, a wait_event_timeout has been added to wait for the
> > ddp_cmdq_cb() callback,indicating that the GCE IRQ has been
> > triggered.
> 
> Waiting up to 500ms for each plane to be disabled is ... not ideal.

It a timeout it won't really wait up to 500ms. I can shrink the timeout
to 1 VSYNC because it must be less than 16.666ms (depend on encoder's
timing).
But it's enough to free the framebuffer at this moment.

> Especially as multiple planes can be disabled at once. This may
> happen
> dynamically during runtime, e.g. when a video is playing and a user
> moves their cursor over the plane to make the UI controls visible.

The video layer will be disabled or video layer and UI layer will be
swapped by the atomic_comit() with drm_pending_vblank_event in this
case.

> 
> I think this should be handled through the atomic_commit() handler,
> with asynchronous tracking of the state, instead of the hard block
> here.

In my case, I encounter the UI process is killed and all the
framebuffer are set to NULL to each plane.

The log is shown as below:

// After the process is killed
// framebuffer(0xf9c00000) of UI layer0 will be freed after ddp_cmdq_cb

[   55.996903] mtk_plane_atomic_disable 296: idx:0, addr=0xf9c00000
[   55.996923] mtk_crtc_update_config 980: needs_vblank=1
[   56.001300] mtk_gem_free_object 110: dma_va = 00000000b49ad5c8,
dma_iova = 0x00000000fb980000, size = 262144
[   56.001538] ddp_cmdq_cb 533: need_vblank:1, sta:0
[   56.017297] mtk_gem_free_object 110: dma_va = 000000003f29e25b,
dma_iova = 0x00000000fb000000, size = 9216000
[   56.017553] mtk_gem_free_object 110: dma_va = 000000000fb130cb,
dma_iova = 0x00000000fa600000, size = 9216000
[   56.017750] mtk_gem_free_object 110: dma_va = 00000000f786f524,
dma_iova = 0x00000000f9c00000, size = 9216000

...

// framebuffer(0xfb9c0000) of cursor layer3 will be freed before
ddp_cmdq_cb

[   62.851250] mtk_plane_atomic_disable 296: idx:3, addr=0xfb9c0000
[   62.851263] mtk_crtc_update_config 980: needs_vblank=0
[   62.851273] mtk_gem_free_object 110: dma_va = 00000000716d147d,
dma_iova = 0x00000000fb9c0000, size = 262144
[   62.851442] arm-smmu-v3 30800000.iommu: TBU_id-0-
fault_id:0x40c(0x40c), TF read in NORMAL world, iova:0xfb9c5000, 
sid:144, ssid:0, ssidv:0, secsidv:0
[   62.851453] arm-smmu-v3 30800000.iommu: event 0x10 received:
[   62.851457] arm-smmu-v3 30800000.iommu:      0x0000009000000010
[   62.851461] arm-smmu-v3 30800000.iommu:      0x0000020800000000
[   62.851464] arm-smmu-v3 30800000.iommu:      0x00000000fb9c5000
[   62.851467] arm-smmu-v3 30800000.iommu:      0x0000000000000000
...
[   62.854666] ddp_cmdq_cb 533: need_vblank:1, sta:-103
[   62.854691] mtk_crtc_update_config 980: needs_vblank=1
[   62.867886] ddp_cmdq_cb 533: need_vblank:1, sta:0

---

After applying this path:

// framebuffer(0xfb9c0000) of cursor layer3 will be freed after
ddp_cmdq_cb
[ 1106.422478] mtk_plane_atomic_disable 296: idx:3, addr=0xfb9c0000
[ 1106.422487] ddp_cmdq_cb 533: need_vblank:0, sta:-103
[ 1106.422508] mtk_crtc_update_config 980: needs_vblank=0
[ 1106.434976] ddp_cmdq_cb 533: need_vblank:0, sta:0
[ 1106.435027] mtk_plane_atomic_disable 308: idx:3 done!
[ 1106.435052] mtk_crtc_update_config 980: needs_vblank=0
[ 1106.435077] mtk_gem_free_object 110: dma_va = 0000000075285c43,
dma_iova = 0x00000000fb9c0000, size = 262144

---

We have already tracking the async state for cursor layer3 in
atomic_commit(), but it seems its framebuffer will still be freed
before ddp_cmdq_cb.

Do you have any ideal rather than adding a hard block in
mtk_plane_atomic_disable()?

Regards,
Jason-JH Lin

> 
> Cheers,
> Daniel

Re: [PATCH] drm/mediatek: Add wait_event_timeout when disabling plane

[CK Hu (胡俊光)]

On Thu, 2025-05-22 at 16:34 +0800, Jason-JH Lin wrote:
> Our hardware registers are set through GCE, not by the CPU.
> DRM might assume the hardware is disabled immediately after calling
> atomic_disable() of drm_plane, but it is only truly disabled after the
> GCE IRQ is triggered.
> 
> Additionally, the cursor plane in DRM uses async_commit, so DRM will
> not wait for vblank and will free the buffer immediately after calling
> atomic_disable().
> 
> To prevent the framebuffer from being freed before the layer disable
> settings are configured into the hardware, which can cause an IOMMU
> fault error, a wait_event_timeout has been added to wait for the
> ddp_cmdq_cb() callback,indicating that the GCE IRQ has been triggered.
> 
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
> Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_crtc.c  | 30 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/mediatek/mtk_crtc.h  |  1 +
>  drivers/gpu/drm/mediatek/mtk_plane.c |  5 +++++
>  3 files changed, 36 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
> index 8f6fba4217ec..944a3d1e5ec9 100644
> --- a/drivers/gpu/drm/mediatek/mtk_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_crtc.c
> @@ -719,6 +719,36 @@ int mtk_crtc_plane_check(struct drm_crtc *crtc, struct drm_plane *plane,
>  	return 0;
>  }
>  
> +void mtk_crtc_plane_disable(struct drm_crtc *crtc, struct drm_plane *plane)
> +{
> +	struct mtk_crtc *mtk_crtc = to_mtk_crtc(crtc);
> +	struct mtk_plane_state *plane_state = to_mtk_plane_state(plane->state);
> +	int i;
> +
> +	if (!mtk_crtc->enabled)
> +		return;
> +
> +	/* set pending plane state to disabled */
> +	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> +		struct drm_plane *mtk_plane = &mtk_crtc->planes[i];
> +		struct mtk_plane_state *mtk_plane_state = to_mtk_plane_state(mtk_plane->state);
> +
> +		if (mtk_plane->index == plane->index) {
> +			memcpy(mtk_plane_state, plane_state, sizeof(*plane_state));

In commit message, you mention GCE flow has problem.
This also modify non-GCE flow.
If non-GCE flow does not need this, move this to GCE flow.

> +			break;
> +		}
> +	}
> +	mtk_crtc_update_config(mtk_crtc, false);
> +
> +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> +	/* wait for planes to be disabled by cmdq */
> +	if (mtk_crtc->cmdq_client.chan)
> +		wait_event_timeout(mtk_crtc->cb_blocking_queue,
> +				   mtk_crtc->cmdq_vblank_cnt == 0,

Check 'mtk_crtc->cmdq_vblank_cnt == 0' may be not good.
If a video is playing and mtk_crtc_update_config() would be call every frame,
mtk_crtc->cmdq_vblank_cnt may not be zero and cursor would be block until timeout.

Regards,
CK

> +				   msecs_to_jiffies(500));
> +#endif
> +}
> +
>  void mtk_crtc_async_update(struct drm_crtc *crtc, struct drm_plane *plane,
>  			   struct drm_atomic_state *state)
>  {
> diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.h b/drivers/gpu/drm/mediatek/mtk_crtc.h
> index 388e900b6f4d..828f109b83e7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_crtc.h
> +++ b/drivers/gpu/drm/mediatek/mtk_crtc.h
> @@ -21,6 +21,7 @@ int mtk_crtc_create(struct drm_device *drm_dev, const unsigned int *path,
>  		    unsigned int num_conn_routes);
>  int mtk_crtc_plane_check(struct drm_crtc *crtc, struct drm_plane *plane,
>  			 struct mtk_plane_state *state);
> +void mtk_crtc_plane_disable(struct drm_crtc *crtc, struct drm_plane *plane);
>  void mtk_crtc_async_update(struct drm_crtc *crtc, struct drm_plane *plane,
>  			   struct drm_atomic_state *plane_state);
>  struct device *mtk_crtc_dma_dev_get(struct drm_crtc *crtc);
> diff --git a/drivers/gpu/drm/mediatek/mtk_plane.c b/drivers/gpu/drm/mediatek/mtk_plane.c
> index 655106bbb76d..59edbe26f01e 100644
> --- a/drivers/gpu/drm/mediatek/mtk_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_plane.c
> @@ -285,9 +285,14 @@ 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);
> +
>  	mtk_plane_state->pending.enable = false;
>  	wmb(); /* Make sure the above parameter is set before update */
>  	mtk_plane_state->pending.dirty = true;
> +
> +	mtk_crtc_plane_disable(old_state->crtc, plane);
>  }
>  
>  static void mtk_plane_atomic_update(struct drm_plane *plane,

Re: [PATCH] drm/mediatek: Add wait_event_timeout when disabling plane

[Jason-JH Lin (林睿祥)]

Hi CK,

Thanks for the reviews.

On Tue, 2025-06-24 at 09:39 +0000, CK Hu (胡俊光) wrote:
> On Thu, 2025-05-22 at 16:34 +0800, Jason-JH Lin wrote:
> > Our hardware registers are set through GCE, not by the CPU.
> > DRM might assume the hardware is disabled immediately after calling
> > atomic_disable() of drm_plane, but it is only truly disabled after
> > the
> > GCE IRQ is triggered.
> > 
> > Additionally, the cursor plane in DRM uses async_commit, so DRM
> > will
> > not wait for vblank and will free the buffer immediately after
> > calling
> > atomic_disable().
> > 
> > To prevent the framebuffer from being freed before the layer
> > disable
> > settings are configured into the hardware, which can cause an IOMMU
> > fault error, a wait_event_timeout has been added to wait for the
> > ddp_cmdq_cb() callback,indicating that the GCE IRQ has been
> > triggered.
> > 
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> > MT8173.")
> > Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_crtc.c  | 30
> > ++++++++++++++++++++++++++++
> >  drivers/gpu/drm/mediatek/mtk_crtc.h  |  1 +
> >  drivers/gpu/drm/mediatek/mtk_plane.c |  5 +++++
> >  3 files changed, 36 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_crtc.c
> > index 8f6fba4217ec..944a3d1e5ec9 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_crtc.c
> > @@ -719,6 +719,36 @@ int mtk_crtc_plane_check(struct drm_crtc
> > *crtc, struct drm_plane *plane,
> >  	return 0;
> >  }
> >  
> > +void mtk_crtc_plane_disable(struct drm_crtc *crtc, struct
> > drm_plane *plane)
> > +{
> > +	struct mtk_crtc *mtk_crtc = to_mtk_crtc(crtc);
> > +	struct mtk_plane_state *plane_state =
> > to_mtk_plane_state(plane->state);
> > +	int i;
> > +
> > +	if (!mtk_crtc->enabled)
> > +		return;
> > +
> > +	/* set pending plane state to disabled */
> > +	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> > +		struct drm_plane *mtk_plane = &mtk_crtc-
> > >planes[i];
> > +		struct mtk_plane_state *mtk_plane_state =
> > to_mtk_plane_state(mtk_plane->state);
> > +
> > +		if (mtk_plane->index == plane->index) {
> > +			memcpy(mtk_plane_state, plane_state,
> > sizeof(*plane_state));
> 
> In commit message, you mention GCE flow has problem.
> This also modify non-GCE flow.
> If non-GCE flow does not need this, move this to GCE flow.
> 

Yes, this API is only used for GCE flow.

I'll add the condition in the beginning of this API to avoid breaking
the non-GCE flow.

> > +			break;
> > +		}
> > +	}
> > +	mtk_crtc_update_config(mtk_crtc, false);
> > +
> > +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > +	/* wait for planes to be disabled by cmdq */
> > +	if (mtk_crtc->cmdq_client.chan)
> > +		wait_event_timeout(mtk_crtc->cb_blocking_queue,
> > +				   mtk_crtc->cmdq_vblank_cnt == 0,
> 
> Check 'mtk_crtc->cmdq_vblank_cnt == 0' may be not good.
> If a video is playing and mtk_crtc_update_config() would be call
> every frame,
> mtk_crtc->cmdq_vblank_cnt may not be zero and cursor would be block
> until timeout.
> 

I think the cursor won't be blocked until timeout here.

mtk_crtc->cmdq_vblank_cnt will be reset to 0 in ddp_cmdq_cb(), so that
we can make sure GCE has configured all the HW settings.

Regards,
Jason-JH Lin

> Regards,
> CK
> 
>