| Message ID | 20250219092040.11227-2-jay.liu@mediatek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | porting pq compnent for MT8196 | expand |
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 > --- > 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[] = {
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[] > > = { > >
On Wed, 2025-02-19 at 17:20 +0800, Jay Liu wrote: > 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> > --- > 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; Change mtk_ddp_ctm_set() return value to bool. If this component support ctm_set(), return true, else return false. And in mtk_crtc_atomic_flush(), check return value of mtk_ddp_ctm_set(). If once a component has support ctm_set(), do not call mtk_ddp_ctm_set() for the rest component. Regards, CK > + > 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[] = {
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 for your review, The 8196 IC has two CCORRs, and they must be chained together in a fixed order, for example: MDP_RSZ0->DISP_TDSHP0->DISP_CCORR0- >DISP_CC0RR1->DISP_GAMMA0->DISP_POSTMASK0->DISP_DITHER0. Among them, DISP_CCORR0 is used for ctm_set, and DISP_CCORR1 was originally for PQ functions, but the current project does not have PQ functions, so relay can be used. Moreover, ctm_set only needs to configure one CCORR, so currently, mandatory_ccorr is set. Considering that previous ICs, such as 8195, only have one CCORR, so mandatory_ccorr is set to DISP_CCORR0. This is the current practice. Do you have any other suggestions to achieve similar things? For example, adding a property in the dts to set mandatory_ccorr, but this will inevitably change the dts of past ICs, and we are worried that such changes will be significant. Thanks a lot 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[] > > = { > >
Il 26/02/25 12:36, Jay Liu (刘博) ha scritto: > 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 for your review, > > The 8196 IC has two CCORRs, and they must be chained together in a > fixed order, for example: MDP_RSZ0->DISP_TDSHP0->DISP_CCORR0- >> DISP_CC0RR1->DISP_GAMMA0->DISP_POSTMASK0->DISP_DITHER0. Among them, > DISP_CCORR0 is used for ctm_set, and DISP_CCORR1 was originally for PQ > functions, but the current project does not have PQ functions, so relay > can be used. Moreover, ctm_set only needs to configure one CCORR, so > currently, mandatory_ccorr is set. Considering that previous ICs, such > as 8195, only have one CCORR, so mandatory_ccorr is set to DISP_CCORR0. > This is the current practice. Do you have any other suggestions to > achieve similar things? For example, adding a property in the dts to Really, just use OF graphs to set the display controller path, you can like that guarantee that the exact path that you define will be respected. This means that you can simply: - Point the output endpoint of TDSHP0 to the input endpoint of CCORR0 - Point the input endpoint of CCORR0 to TDSHP0 - Point the output endpoint of CCORR0 to CCORR1 - etc Check the upstream bindings, and also check MT8195 and MT8188 device trees on the current linux-next (starting from next-20250226). Cheers, Angelo > set mandatory_ccorr, but this will inevitably change the dts of past > ICs, and we are worried that such changes will be significant. > > Thanks a lot > 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[] >>> = { >> >>
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[] = {
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> --- 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(-)