Re: [PATCH v5 10/13] drm/mediatek: Support CRC in display driver

[Shawn Sung (宋孝謙)]

Hi Daniel,

On Fri, 2024-02-16 at 18:04 +0100, Daniel Vetter wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Thu, Feb 15, 2024 at 06:11:16PM +0800, Hsiao Chien Sung wrote:
> > Register CRC related function pointers to support
> > CRC retrieval.
> > 
> > Signed-off-by: Hsiao Chien Sung <shawn.sung@xxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c     | 239
> ++++++++++++++++++++
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.h     |  39 ++++
> >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |   3 +
> >  3 files changed, 281 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index 14cf75fa217f9..6cb1ed419dee7 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -68,6 +68,9 @@ struct mtk_drm_crtc {
> >  /* lock for display hardware access */
> >  struct mutexhw_lock;
> >  boolconfig_updating;
> > +
> > +struct mtk_ddp_comp*crc_provider;
> > +unsigned intframes;
> >  };
> >  
> >  struct mtk_crtc_state {
> > @@ -635,6 +638,14 @@ static void mtk_crtc_ddp_irq(void *data)
> >  struct drm_crtc *crtc = data;
> >  struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
> >  struct mtk_drm_private *priv = crtc->dev->dev_private;
> > +struct mtk_ddp_comp *comp = mtk_crtc->crc_provider;
> > +
> > +/*
> > + * crc providers should make sure the crc is always correct
> > + * by resetting it in .crc_read()
> > + */
> > +if (crtc->crc.opened)
> > +comp->funcs->crc_read(comp->dev);
> >  
> >  #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> >  if (!priv->data->shadow_register && !mtk_crtc->cmdq_client.chan)
> > @@ -646,6 +657,24 @@ static void mtk_crtc_ddp_irq(void *data)
> >  if (!priv->data->shadow_register)
> >  mtk_crtc_ddp_config(crtc, NULL);
> >  #endif
> > +
> > +/*
> > + * drm_crtc_add_crc_entry() could take more than 50ms to finish
> > + * put it at the end of the isr
> > + */
> 
> Uh this looks really scary, especially since you put this before the
> call
> to drm_crtc_handle_vblank in the function below, which really
> shouldn't be
> unecessarily delayed (because that's the one that takes the vblank
> timestamp).

Thank you for pointing this out. This kind of expensive works should be
deferred to the bottom halve instead of in the interrupt context. This
is indeed an issue that was originally to be solved in a future
version, but since it may take some time to adjust the flow and verifiy
it, I fixed other minor issues and pushed this version so the reviewers
could check it first. Will resolve this problem in the next version.

> 
> This sounds like the perfect application for a vblank worker though,
> so
> you please look into drm_vblank_work.h. And if that is not useable
> due to
> hardware constraint, then please explain in a comment here and in the
> commit message why you cannot use that and have to roll your own.
> vblank
> work really should be your first choice here, because:
> - it's designed for expensive vblank work
> - it gives you all the flush/cancel_sync functions you need for
> disabling
>   crc again, and in a race-free implementation. Much better to use
> common
>   code than to reinvent synchronization wheels in drivers :-)
> 
> > +if (crtc->crc.opened) {
> 
> Because this is probably not race-free, so we need something solid
> here.

Thank you very much for the hint :)
Didn't know there is such a useful
tool in DRM.
Will try to integrate the CRC function with
drm_vblank_work.

Cheers, Sima
>