<html><body><p>
<pre>
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@mediatek.com>
> > > ---
> > > 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
</pre>
</p></body></html><!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be
conveyed only to the designated recipient(s). Any use, dissemination,
distribution, printing, retaining or copying of this e-mail (including its
attachments) by unintended recipient(s) is strictly prohibited and may
be unlawful. If you are not an intended recipient of this e-mail, or believe
that you have received this e-mail in error, please notify the sender
immediately (by replying to this e-mail), delete any and all copies of
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
</pre><!--}-->