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

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

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
> 
>