linux-kernel - Re: [PATCH] drm/mediatek: Add wait_event

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c4c97f7d81a7b2b76bd43665e409a972000be8a4.camel@mediatek.com>
Date: Tue, 24 Jun 2025 10:19:15 +0000
From: Jason-JH Lin (林睿祥) <Jason-JH.Lin@...iatek.com>
To: CK Hu (胡俊光) <ck.hu@...iatek.com>,
	"chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>, "AngeloGioacchino Del
 Regno" <angelogioacchino.delregno@...labora.com>
CC: "linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	Singo Chang (張興國) <Singo.Chang@...iatek.com>,
	Yongqiang Niu (牛永强)
	<yongqiang.niu@...iatek.com>, Project_Global_Chrome_Upstream_Group
	<Project_Global_Chrome_Upstream_Group@...iatek.com>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	Zhenxing Qin (秦振兴) <Zhenxing.Qin@...iatek.com>,
	Xavier Chang (張獻文) <Xavier.Chang@...iatek.com>,
	Sirius Wang (王皓昱) <Sirius.Wang@...iatek.com>,
	Nancy Lin (林欣螢) <Nancy.Lin@...iatek.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "wenst@...omium.org"
	<wenst@...omium.org>, "fshao@...omium.org" <fshao@...omium.org>,
	Paul-pl Chen (陳柏霖) <Paul-pl.Chen@...iatek.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/mediatek: Add wait_event_timeout when disabling plane

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@...iatek.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
> 
>