Re: [PATCH v5 10/13] drm/mediatek: Support CRC in display driver
Re: [PATCH v5 10/13] drm/mediatek: Support CRC in display driver
Re: [PATCH v5 10/13] drm/mediatek: Support CRC in display driver
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
> > ---
> > 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.
Cheers, Sima
>
>
> > + /*
> > +* skip the first crc because the first frame is configured by
> > +* mtk_crtc_ddp_hw_init() when atomic enable
> > +*/
> > + if (++mtk_crtc->frames > 1) {
> > + drm_crtc_add_crc_entry(crtc, true,
> > + drm_crtc_vblank_count(crtc),
> > +
> > comp->funcs->crc_entry(comp->dev));
> > + }
> > + } else {
> > + mtk_crtc->frames = 0;
> > + }
> > mtk_drm_finish_page_flip(mtk_crtc);
> > }
> >
> > @@ -704,6 +733,40 @@ static void mtk_drm_crtc_update_output(struct drm_crtc
> > *crtc,
> > }
> > }
> >
> > +static int mtk_drm_crtc_set_crc_source(struct drm_crtc *crtc, const char
> > *src)
> > +{
> > + if (src && strcmp(src, "auto") != 0) {
> > + DRM_ERROR("%s(crtc-%d): unknown source '%s'\n",
> > + __func__, drm_crtc_index(crtc),
Re: [PATCH v5 10/13] drm/mediatek: Support CRC in display driver
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
> ---
> 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.
Cheers, Sima
> + /*
> + * skip the first crc because the first frame is configured by
> + * mtk_crtc_ddp_hw_init() when atomic enable
> + */
> + if (++mtk_crtc->frames > 1) {
> + drm_crtc_add_crc_entry(crtc, true,
> +drm_crtc_vblank_count(crtc),
> +
> comp->funcs->crc_entry(comp->dev));
> + }
> + } else {
> + mtk_crtc->frames = 0;
> + }
> mtk_drm_finish_page_flip(mtk_crtc);
> }
>
> @@ -704,6 +733,40 @@ static void mtk_drm_crtc_update_output(struct drm_crtc
> *crtc,
> }
> }
>
> +static int mtk_drm_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *src)
> +{
> + if (src && strcmp(src, "auto") != 0) {
> + DRM_ERROR("%s(crtc-%d): unknown source '%s'\n",
> + __func__, drm_crtc_index(crtc), src);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int mtk_drm_crtc_verify_crc_source(struct drm_crtc *crtc,
> + const char *src,
> + size_t *cnt)
> +{
> + struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
> + struct mtk_ddp_comp *comp = mtk_crtc->crc_provider;
> +
> + if (!comp) {
> + DRM_ERROR("%s(crtc-%d): no crc provider\n",
> + __func__, drm_crtc_index(crtc));
> + return -ENOENT;
> + }
> +
> + if (src && strcmp(src, "auto") != 0) {
> + DRM_ERROR("%s(crtc-%d): unknown source '%s'\n",
> + __func__, drm_crtc_index(crtc), src);
> + return -EINVAL;
> + }
> +
> + *cnt = comp->funcs->crc_cnt(comp->dev);
> +
> + return 0;
> +}
> +
> int mtk_drm_crtc_plane_check(struct drm_crtc *crtc, struct drm_plane *plane,
>struct mtk_plane_state *state)
> {
> @@ -841,6 +904,8 @@ static const struct drm_crtc_funcs mtk_crtc_funcs = {
> .atomic_destroy_state = mtk_drm_crtc_destroy_state,
> .enable_vblank =
Re: [PATCH v5 10/13] drm/mediatek: Support CRC in display driver
Re: [PATCH v5 10/13] drm/mediatek: Support CRC in display driver
Il 15/02/24 11:11, Hsiao Chien Sung ha scritto:
Register CRC related function pointers to support
CRC retrieval.
Signed-off-by: Hsiao Chien Sung
---
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
+*/
+ if (crtc->crc.opened) {
+ /*
+* skip the first crc because the first frame is configured by
+* mtk_crtc_ddp_hw_init() when atomic enable
+*/
+ if (++mtk_crtc->frames > 1) {
+ drm_crtc_add_crc_entry(crtc, true,
+ drm_crtc_vblank_count(crtc),
+
comp->funcs->crc_entry(comp->dev));
+ }
+ } else {
+ mtk_crtc->frames = 0;
+ }
mtk_drm_finish_page_flip(mtk_crtc);
}
@@ -704,6 +733,40 @@ static void mtk_drm_crtc_update_output(struct drm_crtc *crtc,
}
}
+static int mtk_drm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src)
+{
+ if (src && strcmp(src, "auto") != 0) {
+ DRM_ERROR("%s(crtc-%d): unknown source '%s'\n",
+ __func__, drm_crtc_index(crtc), src);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int mtk_drm_crtc_verify_crc_source(struct drm_crtc *crtc,
+ const char *src,
+ size_t *cnt)
+{
+ struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
+ struct mtk_ddp_comp *comp = mtk_crtc->crc_provider;
+
+ if (!comp) {
+ DRM_ERROR("%s(crtc-%d): no crc provider\n",
+ __func__, drm_crtc_index(crtc));
+ return -ENOENT;
+ }
+
+ if (src && strcmp(src, "auto") != 0) {
+ DRM_ERROR("%s(crtc-%d): unknown source '%s'\n",
+ __func__, drm_crtc_index(crtc), src);
+ return -EINVAL;
+ }
+
+ *cnt = comp->funcs->crc_cnt(comp->dev);
+
+ return 0;
+}
+
int mtk_drm_crtc_plane_check(struct drm_crtc *crtc, struct drm_plane *plane,
struct mtk_plane_state *state)
{
@@ -841,6 +904,8 @@ static const struct drm_crtc_funcs mtk_crtc_funcs = {
.atomic_destroy_state = mtk_drm_crtc_destroy_state,
.enable_vblank = mtk_drm_crtc_enable_vblank,
.disable_vblank = mtk_drm_crtc_disable_vblank,
+ .set_crc_source = mtk_drm_crtc_set_crc_source,
+ .verify_crc_source = mtk_drm_crtc_verify_crc_source,
};
static const struct drm_crtc_helper_funcs mtk_crtc_helper_funcs = {
@@ -1033,6 +1098,11 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
if (comp->funcs->ctm_set)
has_ctm = true;
+
+ if (comp->funcs->crc_cnt &&
+ comp->funcs->crc_entry &&
+ comp->funcs->crc_read)
+ mtk_crtc->crc_provider = comp;
}
mtk_ddp_comp_register_vblank_cb(comp, mtk_crtc_ddp_irq,
@@ -1137,3 +1207,172 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
return 0;
}
+
+void mtk_drm_crc_init(struct mtk_drm_crc *crc,
+ const u32 *crc_offset_table, size_t crc_count,
+ u32 reset_offset, u32
[PATCH v5 10/13] drm/mediatek: Support CRC in display driver
Register CRC related function pointers to support
CRC retrieval.
Signed-off-by: Hsiao Chien Sung
---
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
+*/
+ if (crtc->crc.opened) {
+ /*
+* skip the first crc because the first frame is configured by
+* mtk_crtc_ddp_hw_init() when atomic enable
+*/
+ if (++mtk_crtc->frames > 1) {
+ drm_crtc_add_crc_entry(crtc, true,
+ drm_crtc_vblank_count(crtc),
+
comp->funcs->crc_entry(comp->dev));
+ }
+ } else {
+ mtk_crtc->frames = 0;
+ }
mtk_drm_finish_page_flip(mtk_crtc);
}
@@ -704,6 +733,40 @@ static void mtk_drm_crtc_update_output(struct drm_crtc
*crtc,
}
}
+static int mtk_drm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src)
+{
+ if (src && strcmp(src, "auto") != 0) {
+ DRM_ERROR("%s(crtc-%d): unknown source '%s'\n",
+ __func__, drm_crtc_index(crtc), src);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int mtk_drm_crtc_verify_crc_source(struct drm_crtc *crtc,
+ const char *src,
+ size_t *cnt)
+{
+ struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
+ struct mtk_ddp_comp *comp = mtk_crtc->crc_provider;
+
+ if (!comp) {
+ DRM_ERROR("%s(crtc-%d): no crc provider\n",
+ __func__, drm_crtc_index(crtc));
+ return -ENOENT;
+ }
+
+ if (src && strcmp(src, "auto") != 0) {
+ DRM_ERROR("%s(crtc-%d): unknown source '%s'\n",
+ __func__, drm_crtc_index(crtc), src);
+ return -EINVAL;
+ }
+
+ *cnt = comp->funcs->crc_cnt(comp->dev);
+
+ return 0;
+}
+
int mtk_drm_crtc_plane_check(struct drm_crtc *crtc, struct drm_plane *plane,
struct mtk_plane_state *state)
{
@@ -841,6 +904,8 @@ static const struct drm_crtc_funcs mtk_crtc_funcs = {
.atomic_destroy_state = mtk_drm_crtc_destroy_state,
.enable_vblank = mtk_drm_crtc_enable_vblank,
.disable_vblank = mtk_drm_crtc_disable_vblank,
+ .set_crc_source = mtk_drm_crtc_set_crc_source,
+ .verify_crc_source = mtk_drm_crtc_verify_crc_source,
};
static const struct drm_crtc_helper_funcs mtk_crtc_helper_funcs = {
@@ -1033,6 +1098,11 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
if (comp->funcs->ctm_set)
has_ctm = true;
+
+ if (comp->funcs->crc_cnt &&
+ comp->funcs->crc_entry &&
+ comp->funcs->crc_read)
+ mtk_crtc->crc_provider = comp;
}
mtk_ddp_comp_register_vblank_cb(comp, mtk_crtc_ddp_irq,
@@ -1137,3 +1207,172 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
return 0;
}
+
+void mtk_drm_crc_init(struct mtk_drm_crc *crc,
+ const u32 *crc_offset_table, size_t crc_count,
+ u32 reset_offset, u32 reset_mask)
+{
+ crc->ofs =
