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

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:

quoted

Register CRC related function pointers to support
CRC retrieval.

Signed-off-by: Hsiao Chien Sung <redacted>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c     | 239

++++++++++++++++++++

quoted

 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

quoted

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

quoted

+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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel