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

[Shawn Sung (宋孝謙)]

Hi Daniel,

On Fri, 2024-02-16 at 18:17 +0100, Daniel Vetter wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Fri, Feb 16, 2024 at 06:04:43PM +0100, Daniel Vetter wrote:
> > 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).
> > 
> > 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.
> 
> Since it's maybe a bit tricky to see how to use drm_vblank_work:
> 
> - in your crtc initialization you also need to setup the crc work
> with
>   drm_vblank_work_init().
> - Your mtk_drm_crtc_set_sourc needs to actually enable the crc by
> calling
>   drm_vblank_work_schedule for current vblank + 1, so that it
> immediately
>   starts
> - your vblank worker itself needs to again re-arm itself with
>   drm_vblank_work_schedule, again for the very next vblank
> - then your set_source also needs to handle the case where you
> disable the
>   crc again (source == NULL) by calling drm_vblank_work_cancel_sync
> - also you probably need to call drm_vblank_work_flush when shutting
> down
>   the crtc, or you might have use-after-free issues on driver unload.
>   Could probably also just put that in your crtc release function.
> 
> No changes to your interrupt handler needed, and also definitely no
> digging around in drm_crtc->crc data structure without locking -
> that's
> entirely internal to the common crc code and drivers must never look
> into it.
> 

I am deeply appreciative for the information you have shared. Will try
to implement it with DRM_vblank_work APIs.

Sincerely,
Shawn