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

["Jason-JH Lin (林睿祥)" <Jason-JH.Lin@mediatek.com>]

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