Re: [PATCH 1/7] drm/mediatek: Add CCORR component support for MT8196

["Jay Liu (刘博)" <Jay.Liu@mediatek.com>]

On Wed, 2025-02-19 at 13:49 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 19/02/25 10:20, Jay Liu ha scritto:
> > Add CCORR component support for MT8196.
> > 
> > CCORR is a hardware module that optimizes the visual effects of
> > images by adjusting the color matrix, enabling features such as
> > night light.
> > 
> > The 8196 hardware platform includes two CCORR (Color Correction)
> > units.
> > However, the `mtk_ccorr_ctm_set` API only utilizes one of these
> > units.
> > To prevent the unused CCORR unit from inadvertently taking effect,
> > we need to block it by adding mandatory_ccorr flag in the
> > driver_data.
> > 
> > Signed-off-by: Jay Liu <jay.liu@mediatek.com>
> 
> This is yet another thing that can be resolved by using OF Graph for
> defining the
> display pipeline: by using that, I don't see how can CCORR1 be used
> instead of
> CCORR0, if the latter is in the pipeline, but not the former.
> 
> NACK.
> 
> Regards,
> Angelo

> hi Angelo, Thank you very much for your feedback. 

> The 8196 SoC has two CCORR hardware units, which must be chained
> together in a fixed order in the display path to display the image
> correctly. In the current project, the ctm_set interface will
> traverse all CCORRs and set parameters, but in fact, only one needs
> to be set. Therefore, setting mandatory_ccorr ensures that only this
> CCORR is used.


> Regards,
> Jay
> > ---
> >   drivers/gpu/drm/mediatek/mtk_ddp_comp.c   |  3 ++-
> >   drivers/gpu/drm/mediatek/mtk_disp_ccorr.c | 16 ++++++++++++++++
> >   2 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> > b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> > index edc6417639e6..d7e230bac53e 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> > @@ -457,7 +457,8 @@ static const struct mtk_ddp_comp_match
> > mtk_ddp_matches[DDP_COMPONENT_DRM_ID_MAX]
> >       [DDP_COMPONENT_AAL0]            = {
> > MTK_DISP_AAL,               0, &ddp_aal },
> >       [DDP_COMPONENT_AAL1]            = {
> > MTK_DISP_AAL,               1, &ddp_aal },
> >       [DDP_COMPONENT_BLS]             = {
> > MTK_DISP_BLS,               0, NULL },
> > -     [DDP_COMPONENT_CCORR]           = {
> > MTK_DISP_CCORR,             0, &ddp_ccorr },
> > +     [DDP_COMPONENT_CCORR0]          = {
> > MTK_DISP_CCORR,             0, &ddp_ccorr },
> > +     [DDP_COMPONENT_CCORR1]          = {
> > MTK_DISP_CCORR,             1, &ddp_ccorr },
> >       [DDP_COMPONENT_COLOR0]          = {
> > MTK_DISP_COLOR,             0, &ddp_color },
> >       [DDP_COMPONENT_COLOR1]          = {
> > MTK_DISP_COLOR,             1, &ddp_color },
> >       [DDP_COMPONENT_DITHER0]         = {
> > MTK_DISP_DITHER,            0, &ddp_dither },
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
> > b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
> > index 10d60d2c2a56..94e82b3fa2d8 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
> > @@ -31,11 +31,13 @@
> > 
> >   struct mtk_disp_ccorr_data {
> >       u32 matrix_bits;
> > +     enum mtk_ddp_comp_id mandatory_ccorr;
> >   };
> > 
> >   struct mtk_disp_ccorr {
> >       struct clk *clk;
> >       void __iomem *regs;
> > +     enum mtk_ddp_comp_id comp_id;
> >       struct cmdq_client_reg cmdq_reg;
> >       const struct mtk_disp_ccorr_data        *data;
> >   };
> > @@ -115,6 +117,9 @@ void mtk_ccorr_ctm_set(struct device *dev,
> > struct drm_crtc_state *state)
> >       if (!blob)
> >               return;
> > 
> > +     if (ccorr->comp_id != ccorr->data->mandatory_ccorr)
> > +             return;
> > +
> >       ctm = (struct drm_color_ctm *)blob->data;
> >       input = ctm->matrix;
> > 
> > @@ -154,6 +159,7 @@ static int mtk_disp_ccorr_probe(struct
> > platform_device *pdev)
> >       struct device *dev = &pdev->dev;
> >       struct mtk_disp_ccorr *priv;
> >       int ret;
> > +     enum mtk_ddp_comp_id comp_id;
> > 
> >       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >       if (!priv)
> > @@ -169,6 +175,14 @@ static int mtk_disp_ccorr_probe(struct
> > platform_device *pdev)
> >               return dev_err_probe(dev, PTR_ERR(priv->regs),
> >                                    "failed to ioremap ccorr\n");
> > 
> > +     comp_id = mtk_ddp_comp_get_id(dev->of_node, MTK_DISP_CCORR);
> > +     if (comp_id < 0) {
> > +             dev_err(dev, "Failed to identify by alias: %d\n",
> > comp_id);
> > +             return comp_id;
> > +     }
> > +
> > +     priv->comp_id = comp_id;
> > +
> >   #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> >       ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
> >       if (ret)
> > @@ -192,10 +206,12 @@ static void mtk_disp_ccorr_remove(struct
> > platform_device *pdev)
> > 
> >   static const struct mtk_disp_ccorr_data mt8183_ccorr_driver_data
> > = {
> >       .matrix_bits = 10,
> > +     .mandatory_ccorr = DDP_COMPONENT_CCORR0,
> >   };
> > 
> >   static const struct mtk_disp_ccorr_data mt8192_ccorr_driver_data
> > = {
> >       .matrix_bits = 11,
> > +     .mandatory_ccorr = DDP_COMPONENT_CCORR0,
> >   };
> > 
> >   static const struct of_device_id mtk_disp_ccorr_driver_dt_match[]
> > = {
> 
>