[PATCH 0/7] porting pq compnent for MT8196

[PATCH 0/7] porting pq compnent for MT8196

[Jay Liu]

add ccorr/dither/gamma/tdshp support for MT8196

This patch series id base on [1]
[1] Add components to support PQ in display path for MT8196
	- https://patchwork.kernel.org/project/linux-mediatek/list/?series=932469

Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Jay Liu (7):
  drm/mediatek: Add CCORR component support for MT8196
  drm/mediatek: fix CCORR mtk_ctm_s31_32_to_s1_n function issue
  drm/mediatek: Add TDSHP component support for MT8196
  dt-bindings: display: mediatek: ccorr: Add support for MT8196
  dt-bindings: display: mediatek: dither: Add support for MT8196
  dt-bindings: display: mediatek: gamma: Add support for MT8196
  dt-bindings: display: mediatek: tdshp: Add support for MT8196

 .../display/mediatek/mediatek,ccorr.yaml      |  1 +
 .../display/mediatek/mediatek,dither.yaml     |  1 +
 .../display/mediatek/mediatek,gamma.yaml      |  1 +
 .../display/mediatek/mediatek,tdshp.yaml      | 51 ++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_ddp_comp.c       | 61 ++++++++++++++++++-
 drivers/gpu/drm/mediatek/mtk_ddp_comp.h       |  1 +
 drivers/gpu/drm/mediatek/mtk_disp_ccorr.c     | 25 ++++++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.c        |  2 +
 8 files changed, 142 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp.yaml

-- 
2.18.0

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

[Jay Liu]

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;
+
 	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[] = {
-- 
2.18.0

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

[AngeloGioacchino Del Regno]

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[] = {

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

[Jay Liu (刘博)]

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[]
> > = {
> 
> 

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

[Jay Liu (刘博)]

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[]
> > = {
> 
> 

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

[AngeloGioacchino Del Regno]

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[]
>>> = {
>>
>>

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

[CK Hu (胡俊光)]

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[] = {

[PATCH 2/7] drm/mediatek: fix CCORR mtk_ctm_s31_32_to_s1_n function issue

[Jay Liu]

if matrixbit is 11,
The range of color matrix is from 0 to (BIT(11) - 1).
Values from 0 to (BIT(11) - 1) represent positive numbers,
values from BIT(11) to (BIT(12) - 1) represent negative numbers.
For example, -1 need converted to 8191.

Fixes: 738ed4156fba ("drm/mediatek: Add matrix_bits private data for ccorr")

Signed-off-by: Jay Liu <jay.liu@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_disp_ccorr.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
index 94e82b3fa2d8..a9f91b71534b 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
@@ -100,6 +100,15 @@ static u16 mtk_ctm_s31_32_to_s1_n(u64 in, u32 n)
 		r |= (in >> (32 - n)) & GENMASK(n, 0);
 	}
 
+	/*
+	 *The range of r is from 0 to (BIT(n + 1) - 1),
+	 *where values from 0 to (BIT(n) - 1) represent positive numbers,
+	 *and values from BIT(n) to (BIT(n + 1) - 1) represent negative numbers.
+	 *For example, if n is 11, -1 will be converted to 8191.
+	 */
+	if (r & BIT(n + 1))
+		r = (~(r & GENMASK(n, 0)) + 1) & GENMASK(n + 1, 0);
+
 	return r;
 }
 
-- 
2.18.0

Re: [PATCH 2/7] drm/mediatek: fix CCORR mtk_ctm_s31_32_to_s1_n function issue

[AngeloGioacchino Del Regno]

Il 19/02/25 10:20, Jay Liu ha scritto:
> if matrixbit is 11,
> The range of color matrix is from 0 to (BIT(11) - 1).
> Values from 0 to (BIT(11) - 1) represent positive numbers,
> values from BIT(11) to (BIT(12) - 1) represent negative numbers.
> For example, -1 need converted to 8191.
> 
> Fixes: 738ed4156fba ("drm/mediatek: Add matrix_bits private data for ccorr")
> 
> Signed-off-by: Jay Liu <jay.liu@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH 2/7] drm/mediatek: fix CCORR mtk_ctm_s31_32_to_s1_n function issue

[CK Hu (胡俊光)]

On Wed, 2025-02-19 at 17:20 +0800, Jay Liu wrote:
> if matrixbit is 11,
> The range of color matrix is from 0 to (BIT(11) - 1).
> Values from 0 to (BIT(11) - 1) represent positive numbers,
> values from BIT(11) to (BIT(12) - 1) represent negative numbers.
> For example, -1 need converted to 8191.
> 
> Fixes: 738ed4156fba ("drm/mediatek: Add matrix_bits private data for ccorr")
> 
> Signed-off-by: Jay Liu <jay.liu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_ccorr.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
> index 94e82b3fa2d8..a9f91b71534b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
> @@ -100,6 +100,15 @@ static u16 mtk_ctm_s31_32_to_s1_n(u64 in, u32 n)
>  		r |= (in >> (32 - n)) & GENMASK(n, 0);
>  	}
>  
> +	/*
> +	 *The range of r is from 0 to (BIT(n + 1) - 1),
> +	 *where values from 0 to (BIT(n) - 1) represent positive numbers,
> +	 *and values from BIT(n) to (BIT(n + 1) - 1) represent negative numbers.
> +	 *For example, if n is 11, -1 will be converted to 8191.
> +	 */
> +	if (r & BIT(n + 1))
> +		r = (~(r & GENMASK(n, 0)) + 1) & GENMASK(n + 1, 0);
> +

This function is to change s31.32 to s1.n, but it seems you change s31.32 to two's complement.
If so, use drm_color_ctm_s31_32_to_qm_n().
In addition, are you sure that MT8183 and MT8192 hardware use two's complement?

Regards,
CK

>  	return r;
>  }
>  

[PATCH 3/7] drm/mediatek: Add TDSHP component support for MT8196

[Jay Liu]

Add TDSHP component support for MT8196.
TDSHP is a hardware module designed to enhance the sharpness and
clarity of displayed images by analyzing and improving edges and
fine details in frames.

Signed-off-by: Jay Liu <jay.liu@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_ddp_comp.c | 58 +++++++++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_ddp_comp.h |  1 +
 drivers/gpu/drm/mediatek/mtk_drm_drv.c  |  2 +
 3 files changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
index d7e230bac53e..b87fde64ee49 100644
--- a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
+++ b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
@@ -57,6 +57,16 @@
 #define POSTMASK_RELAY_MODE				BIT(0)
 #define DISP_REG_POSTMASK_SIZE			0x0030
 
+#define DISP_REG_TDSHP_EN			0x0000
+#define DISP_TDSHP_TDS_EN			BIT(31)
+#define DISP_REG_TDSHP_CTRL			0x0100
+#define DISP_TDSHP_CTRL_EN			BIT(0)
+#define DISP_TDSHP_PWR_SCL_EN			BIT(2)
+#define DISP_REG_TDSHP_CFG			0x0110
+#define DISP_REG_TDSHP_INPUT_SIZE		0x0120
+#define DISP_REG_TDSHP_OUTPUT_OFFSET		0x0124
+#define DISP_REG_TDSHP_OUTPUT_SIZE		0x0128
+
 #define DISP_REG_UFO_START			0x0000
 #define UFO_BYPASS				BIT(2)
 
@@ -261,6 +271,44 @@ static void mtk_postmask_stop(struct device *dev)
 	writel_relaxed(0x0, priv->regs + DISP_REG_POSTMASK_EN);
 }
 
+static void mtk_disp_tdshp_config(struct device *dev, unsigned int w,
+				  unsigned int h, unsigned int vrefresh,
+				  unsigned int bpc, struct cmdq_pkt *cmdq_pkt)
+{
+	struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
+	u32 tdshp_ctrl = (bpc == 8) ? DISP_TDSHP_PWR_SCL_EN | DISP_TDSHP_CTRL_EN : 0;
+
+	mtk_ddp_write(cmdq_pkt, tdshp_ctrl, &priv->cmdq_reg, priv->regs,
+		      DISP_REG_TDSHP_CTRL);
+
+	mtk_ddp_write(cmdq_pkt, w << 16 | h, &priv->cmdq_reg, priv->regs,
+		      DISP_REG_TDSHP_INPUT_SIZE);
+	mtk_ddp_write(cmdq_pkt, w << 16 | h, &priv->cmdq_reg, priv->regs,
+		      DISP_REG_TDSHP_OUTPUT_SIZE);
+	mtk_ddp_write(cmdq_pkt, 0x0, &priv->cmdq_reg, priv->regs,
+		      DISP_REG_TDSHP_OUTPUT_OFFSET);
+
+	mtk_ddp_write(cmdq_pkt, 0x1, &priv->cmdq_reg,
+		      priv->regs, DISP_REG_TDSHP_CFG);
+
+	mtk_ddp_write_mask(cmdq_pkt, DISP_TDSHP_TDS_EN, &priv->cmdq_reg, priv->regs,
+			   DISP_REG_TDSHP_EN, DISP_TDSHP_TDS_EN);
+}
+
+static void mtk_disp_tdshp_start(struct device *dev)
+{
+	struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
+
+	writel(DISP_TDSHP_CTRL_EN, priv->regs + DISP_REG_TDSHP_CTRL);
+}
+
+static void mtk_disp_tdshp_stop(struct device *dev)
+{
+	struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
+
+	writel(0x0, priv->regs + DISP_REG_TDSHP_CTRL);
+}
+
 static void mtk_ufoe_start(struct device *dev)
 {
 	struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
@@ -268,6 +316,14 @@ static void mtk_ufoe_start(struct device *dev)
 	writel(UFO_BYPASS, priv->regs + DISP_REG_UFO_START);
 }
 
+static const struct mtk_ddp_comp_funcs ddp_tdshp = {
+	.clk_enable = mtk_ddp_clk_enable,
+	.clk_disable = mtk_ddp_clk_disable,
+	.config = mtk_disp_tdshp_config,
+	.start = mtk_disp_tdshp_start,
+	.stop = mtk_disp_tdshp_stop,
+};
+
 static const struct mtk_ddp_comp_funcs ddp_aal = {
 	.clk_enable = mtk_aal_clk_enable,
 	.clk_disable = mtk_aal_clk_disable,
@@ -440,6 +496,7 @@ static const char * const mtk_ddp_comp_stem[MTK_DDP_COMP_TYPE_MAX] = {
 	[MTK_DISP_POSTMASK] = "postmask",
 	[MTK_DISP_PWM] = "pwm",
 	[MTK_DISP_RDMA] = "rdma",
+	[MTK_DISP_TDSHP] = "tdshp",
 	[MTK_DISP_UFOE] = "ufoe",
 	[MTK_DISP_WDMA] = "wdma",
 	[MTK_DP_INTF] = "dp-intf",
@@ -495,6 +552,7 @@ static const struct mtk_ddp_comp_match mtk_ddp_matches[DDP_COMPONENT_DRM_ID_MAX]
 	[DDP_COMPONENT_RDMA1]		= { MTK_DISP_RDMA,		1, &ddp_rdma },
 	[DDP_COMPONENT_RDMA2]		= { MTK_DISP_RDMA,		2, &ddp_rdma },
 	[DDP_COMPONENT_RDMA4]		= { MTK_DISP_RDMA,		4, &ddp_rdma },
+	[DDP_COMPONENT_TDSHP0]		= { MTK_DISP_TDSHP,		0, &ddp_tdshp },
 	[DDP_COMPONENT_UFOE]		= { MTK_DISP_UFOE,		0, &ddp_ufoe },
 	[DDP_COMPONENT_WDMA0]		= { MTK_DISP_WDMA,		0, NULL },
 	[DDP_COMPONENT_WDMA1]		= { MTK_DISP_WDMA,		1, NULL },
diff --git a/drivers/gpu/drm/mediatek/mtk_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_ddp_comp.h
index 39720b27f4e9..fc90d32b8c2e 100644
--- a/drivers/gpu/drm/mediatek/mtk_ddp_comp.h
+++ b/drivers/gpu/drm/mediatek/mtk_ddp_comp.h
@@ -38,6 +38,7 @@ enum mtk_ddp_comp_type {
 	MTK_DISP_POSTMASK,
 	MTK_DISP_PWM,
 	MTK_DISP_RDMA,
+	MTK_DISP_TDSHP,
 	MTK_DISP_UFOE,
 	MTK_DISP_WDMA,
 	MTK_DPI,
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index f22ad2882697..4ec09864bc8c 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -790,6 +790,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = {
 	  .data = (void *)MTK_DISP_RDMA },
 	{ .compatible = "mediatek,mt8195-disp-rdma",
 	  .data = (void *)MTK_DISP_RDMA },
+	{ .compatible = "mediatek,mt8196-disp-tdshp",
+	  .data = (void *)MTK_DISP_TDSHP },
 	{ .compatible = "mediatek,mt8173-disp-ufoe",
 	  .data = (void *)MTK_DISP_UFOE },
 	{ .compatible = "mediatek,mt8173-disp-wdma",
-- 
2.18.0

Re: [PATCH 3/7] drm/mediatek: Add TDSHP component support for MT8196

[AngeloGioacchino Del Regno]

Il 19/02/25 10:20, Jay Liu ha scritto:
> Add TDSHP component support for MT8196.
> TDSHP is a hardware module designed to enhance the sharpness and
> clarity of displayed images by analyzing and improving edges and
> fine details in frames.
> 
> Signed-off-by: Jay Liu <jay.liu@mediatek.com>
> ---
>   drivers/gpu/drm/mediatek/mtk_ddp_comp.c | 58 +++++++++++++++++++++++++
>   drivers/gpu/drm/mediatek/mtk_ddp_comp.h |  1 +
>   drivers/gpu/drm/mediatek/mtk_drm_drv.c  |  2 +
>   3 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> index d7e230bac53e..b87fde64ee49 100644
> --- a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> @@ -57,6 +57,16 @@
>   #define POSTMASK_RELAY_MODE				BIT(0)
>   #define DISP_REG_POSTMASK_SIZE			0x0030
>   
> +#define DISP_REG_TDSHP_EN			0x0000
> +#define DISP_TDSHP_TDS_EN			BIT(31)
> +#define DISP_REG_TDSHP_CTRL			0x0100
> +#define DISP_TDSHP_CTRL_EN			BIT(0)
> +#define DISP_TDSHP_PWR_SCL_EN			BIT(2)
> +#define DISP_REG_TDSHP_CFG			0x0110

#define TDSHP_RELAY_MODE	BIT(0)

> +#define DISP_REG_TDSHP_INPUT_SIZE		0x0120
> +#define DISP_REG_TDSHP_OUTPUT_OFFSET		0x0124
> +#define DISP_REG_TDSHP_OUTPUT_SIZE		0x0128
> +
>   #define DISP_REG_UFO_START			0x0000
>   #define UFO_BYPASS				BIT(2)
>   
> @@ -261,6 +271,44 @@ static void mtk_postmask_stop(struct device *dev)
>   	writel_relaxed(0x0, priv->regs + DISP_REG_POSTMASK_EN);
>   }
>   
> +static void mtk_disp_tdshp_config(struct device *dev, unsigned int w,
> +				  unsigned int h, unsigned int vrefresh,
> +				  unsigned int bpc, struct cmdq_pkt *cmdq_pkt)
> +{
> +	struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
> +	u32 tdshp_ctrl = (bpc == 8) ? DISP_TDSHP_PWR_SCL_EN | DISP_TDSHP_CTRL_EN : 0;
> +
> +	mtk_ddp_write(cmdq_pkt, tdshp_ctrl, &priv->cmdq_reg, priv->regs,
> +		      DISP_REG_TDSHP_CTRL);
> +
> +	mtk_ddp_write(cmdq_pkt, w << 16 | h, &priv->cmdq_reg, priv->regs,
> +		      DISP_REG_TDSHP_INPUT_SIZE);
> +	mtk_ddp_write(cmdq_pkt, w << 16 | h, &priv->cmdq_reg, priv->regs,
> +		      DISP_REG_TDSHP_OUTPUT_SIZE);
> +	mtk_ddp_write(cmdq_pkt, 0x0, &priv->cmdq_reg, priv->regs,
> +		      DISP_REG_TDSHP_OUTPUT_OFFSET);
> +
> +	mtk_ddp_write(cmdq_pkt, 0x1, &priv->cmdq_reg,

mtk_ddp_write(cmdq_pkt, TDSHP_RELAY_MODE, ... etc etc

> +		      priv->regs, DISP_REG_TDSHP_CFG);
> +
> +	mtk_ddp_write_mask(cmdq_pkt, DISP_TDSHP_TDS_EN, &priv->cmdq_reg, priv->regs,
> +			   DISP_REG_TDSHP_EN, DISP_TDSHP_TDS_EN);
> +}
> +
> +static void mtk_disp_tdshp_start(struct device *dev)
> +{
> +	struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
> +
> +	writel(DISP_TDSHP_CTRL_EN, priv->regs + DISP_REG_TDSHP_CTRL);
> +}
> +
> +static void mtk_disp_tdshp_stop(struct device *dev)
> +{
> +	struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
> +
> +	writel(0x0, priv->regs + DISP_REG_TDSHP_CTRL);

writel(0, priv->regs + DISP_REG_TDSHP_CTRL);

> +}
> +

After which:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH 3/7] drm/mediatek: Add TDSHP component support for MT8196

[CK Hu (胡俊光)]

On Wed, 2025-02-19 at 17:20 +0800, Jay Liu wrote:
> Add TDSHP component support for MT8196.
> TDSHP is a hardware module designed to enhance the sharpness and
> clarity of displayed images by analyzing and improving edges and
> fine details in frames.
> 
> Signed-off-by: Jay Liu <jay.liu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_ddp_comp.c | 58 +++++++++++++++++++++++++
>  drivers/gpu/drm/mediatek/mtk_ddp_comp.h |  1 +
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c  |  2 +
>  3 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> index d7e230bac53e..b87fde64ee49 100644
> --- a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> @@ -57,6 +57,16 @@
>  #define POSTMASK_RELAY_MODE				BIT(0)
>  #define DISP_REG_POSTMASK_SIZE			0x0030
>  
> +#define DISP_REG_TDSHP_EN			0x0000
> +#define DISP_TDSHP_TDS_EN			BIT(31)
> +#define DISP_REG_TDSHP_CTRL			0x0100
> +#define DISP_TDSHP_CTRL_EN			BIT(0)
> +#define DISP_TDSHP_PWR_SCL_EN			BIT(2)
> +#define DISP_REG_TDSHP_CFG			0x0110
> +#define DISP_REG_TDSHP_INPUT_SIZE		0x0120
> +#define DISP_REG_TDSHP_OUTPUT_OFFSET		0x0124
> +#define DISP_REG_TDSHP_OUTPUT_SIZE		0x0128
> +
>  #define DISP_REG_UFO_START			0x0000
>  #define UFO_BYPASS				BIT(2)
>  
> @@ -261,6 +271,44 @@ static void mtk_postmask_stop(struct device *dev)
>  	writel_relaxed(0x0, priv->regs + DISP_REG_POSTMASK_EN);
>  }
>  
> +static void mtk_disp_tdshp_config(struct device *dev, unsigned int w,
> +				  unsigned int h, unsigned int vrefresh,
> +				  unsigned int bpc, struct cmdq_pkt *cmdq_pkt)
> +{
> +	struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
> +	u32 tdshp_ctrl = (bpc == 8) ? DISP_TDSHP_PWR_SCL_EN | DISP_TDSHP_CTRL_EN : 0;
> +
> +	mtk_ddp_write(cmdq_pkt, tdshp_ctrl, &priv->cmdq_reg, priv->regs,
> +		      DISP_REG_TDSHP_CTRL);
> +
> +	mtk_ddp_write(cmdq_pkt, w << 16 | h, &priv->cmdq_reg, priv->regs,
> +		      DISP_REG_TDSHP_INPUT_SIZE);
> +	mtk_ddp_write(cmdq_pkt, w << 16 | h, &priv->cmdq_reg, priv->regs,
> +		      DISP_REG_TDSHP_OUTPUT_SIZE);
> +	mtk_ddp_write(cmdq_pkt, 0x0, &priv->cmdq_reg, priv->regs,
> +		      DISP_REG_TDSHP_OUTPUT_OFFSET);
> +
> +	mtk_ddp_write(cmdq_pkt, 0x1, &priv->cmdq_reg,
> +		      priv->regs, DISP_REG_TDSHP_CFG);

Symbolize 0x1.

> +
> +	mtk_ddp_write_mask(cmdq_pkt, DISP_TDSHP_TDS_EN, &priv->cmdq_reg, priv->regs,
> +			   DISP_REG_TDSHP_EN, DISP_TDSHP_TDS_EN);

What's the function of DISP_TDSHP_TDS_EN?
I think default would should not enhance sharpness because it may cause some side effect for some image.

> +}
> +
> +static void mtk_disp_tdshp_start(struct device *dev)
> +{
> +	struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
> +
> +	writel(DISP_TDSHP_CTRL_EN, priv->regs + DISP_REG_TDSHP_CTRL);

I don't know why you set DISP_TDSHP_CTRL_EN both here and in mtk_disp_tdshp_config().
If DISP_TDSHP_CTRL_EN is used to enable tdshp, why mtk_disp_tdshp_config() may disable tdshp in some case?

Regards,
CK

> +}
> +
> +static void mtk_disp_tdshp_stop(struct device *dev)
> +{
> +	struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
> +
> +	writel(0x0, priv->regs + DISP_REG_TDSHP_CTRL);
> +}
> +
>  static void mtk_ufoe_start(struct device *dev)
>  {
>  	struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
> @@ -268,6 +316,14 @@ static void mtk_ufoe_start(struct device *dev)
>  	writel(UFO_BYPASS, priv->regs + DISP_REG_UFO_START);
>  }
>  
> +static const struct mtk_ddp_comp_funcs ddp_tdshp = {
> +	.clk_enable = mtk_ddp_clk_enable,
> +	.clk_disable = mtk_ddp_clk_disable,
> +	.config = mtk_disp_tdshp_config,
> +	.start = mtk_disp_tdshp_start,
> +	.stop = mtk_disp_tdshp_stop,
> +};
> +
>  static const struct mtk_ddp_comp_funcs ddp_aal = {
>  	.clk_enable = mtk_aal_clk_enable,
>  	.clk_disable = mtk_aal_clk_disable,
> @@ -440,6 +496,7 @@ static const char * const mtk_ddp_comp_stem[MTK_DDP_COMP_TYPE_MAX] = {
>  	[MTK_DISP_POSTMASK] = "postmask",
>  	[MTK_DISP_PWM] = "pwm",
>  	[MTK_DISP_RDMA] = "rdma",
> +	[MTK_DISP_TDSHP] = "tdshp",
>  	[MTK_DISP_UFOE] = "ufoe",
>  	[MTK_DISP_WDMA] = "wdma",
>  	[MTK_DP_INTF] = "dp-intf",
> @@ -495,6 +552,7 @@ static const struct mtk_ddp_comp_match mtk_ddp_matches[DDP_COMPONENT_DRM_ID_MAX]
>  	[DDP_COMPONENT_RDMA1]		= { MTK_DISP_RDMA,		1, &ddp_rdma },
>  	[DDP_COMPONENT_RDMA2]		= { MTK_DISP_RDMA,		2, &ddp_rdma },
>  	[DDP_COMPONENT_RDMA4]		= { MTK_DISP_RDMA,		4, &ddp_rdma },
> +	[DDP_COMPONENT_TDSHP0]		= { MTK_DISP_TDSHP,		0, &ddp_tdshp },
>  	[DDP_COMPONENT_UFOE]		= { MTK_DISP_UFOE,		0, &ddp_ufoe },
>  	[DDP_COMPONENT_WDMA0]		= { MTK_DISP_WDMA,		0, NULL },
>  	[DDP_COMPONENT_WDMA1]		= { MTK_DISP_WDMA,		1, NULL },
> diff --git a/drivers/gpu/drm/mediatek/mtk_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_ddp_comp.h
> index 39720b27f4e9..fc90d32b8c2e 100644
> --- a/drivers/gpu/drm/mediatek/mtk_ddp_comp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_ddp_comp.h
> @@ -38,6 +38,7 @@ enum mtk_ddp_comp_type {
>  	MTK_DISP_POSTMASK,
>  	MTK_DISP_PWM,
>  	MTK_DISP_RDMA,
> +	MTK_DISP_TDSHP,
>  	MTK_DISP_UFOE,
>  	MTK_DISP_WDMA,
>  	MTK_DPI,
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index f22ad2882697..4ec09864bc8c 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -790,6 +790,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = {
>  	  .data = (void *)MTK_DISP_RDMA },
>  	{ .compatible = "mediatek,mt8195-disp-rdma",
>  	  .data = (void *)MTK_DISP_RDMA },
> +	{ .compatible = "mediatek,mt8196-disp-tdshp",
> +	  .data = (void *)MTK_DISP_TDSHP },
>  	{ .compatible = "mediatek,mt8173-disp-ufoe",
>  	  .data = (void *)MTK_DISP_UFOE },
>  	{ .compatible = "mediatek,mt8173-disp-wdma",

[PATCH 4/7] dt-bindings: display: mediatek: ccorr: Add support for MT8196

[Jay Liu]

Add a compatible string for MediaTek MT8196 SoC

Signed-off-by: Jay Liu <jay.liu@mediatek.com>
---
 .../devicetree/bindings/display/mediatek/mediatek,ccorr.yaml     | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
index fca8e7bb0cbc..581003aa9b9c 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
@@ -32,6 +32,7 @@ properties:
               - mediatek,mt8186-disp-ccorr
               - mediatek,mt8188-disp-ccorr
               - mediatek,mt8195-disp-ccorr
+              - mediatek,mt8196-disp-ccorr
           - const: mediatek,mt8192-disp-ccorr
 
   reg:
-- 
2.18.0

Re: [PATCH 4/7] dt-bindings: display: mediatek: ccorr: Add support for MT8196

[Krzysztof Kozlowski]

On 19/02/2025 10:20, Jay Liu wrote:
> Add a compatible string for MediaTek MT8196 SoC

You do not add compatible for soc but its component. But even if this
was correct commit msg, you still repeat what is in the diff. Say
something useful, is it compatible with 8195 maybe? Or with something else?


Best regards,
Krzysztof

Re: [PATCH 4/7] dt-bindings: display: mediatek: ccorr: Add support for MT8196

[AngeloGioacchino Del Regno]

Il 19/02/25 10:20, Jay Liu ha scritto:
> Add a compatible string for MediaTek MT8196 SoC
> 

Add a compatible string for the CCORR IP found in the MT8196 SoC.
Each CCORR IP of this SoC is fully compatible with the ones found in MT8192.


Please fix the commit description here, other than in dither and gamma.

After which,

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> Signed-off-by: Jay Liu <jay.liu@mediatek.com>
> ---
>   .../devicetree/bindings/display/mediatek/mediatek,ccorr.yaml     | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> index fca8e7bb0cbc..581003aa9b9c 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> @@ -32,6 +32,7 @@ properties:
>                 - mediatek,mt8186-disp-ccorr
>                 - mediatek,mt8188-disp-ccorr
>                 - mediatek,mt8195-disp-ccorr
> +              - mediatek,mt8196-disp-ccorr
>             - const: mediatek,mt8192-disp-ccorr
>   
>     reg:

[PATCH 5/7] dt-bindings: display: mediatek: dither: Add support for MT8196

[Jay Liu]

Add a compatible string for MediaTek MT8196 SoC

Signed-off-by: Jay Liu <jay.liu@mediatek.com>
---
 .../devicetree/bindings/display/mediatek/mediatek,dither.yaml    | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
index abaf27916d13..1f1719228b5d 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
@@ -31,6 +31,7 @@ properties:
               - mediatek,mt8192-disp-dither
               - mediatek,mt8195-disp-dither
               - mediatek,mt8365-disp-dither
+              - mediatek,mt8196-disp-dither
           - const: mediatek,mt8183-disp-dither
 
   reg:
-- 
2.18.0

[PATCH 6/7] dt-bindings: display: mediatek: gamma: Add support for MT8196

[Jay Liu]

Add a compatible string for MediaTek MT8196 SoC

Signed-off-by: Jay Liu <jay.liu@mediatek.com>
---
 .../devicetree/bindings/display/mediatek/mediatek,gamma.yaml     | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
index 48542dc7e784..513e51c6d2b9 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
@@ -40,6 +40,7 @@ properties:
       - items:
           - enum:
               - mediatek,mt8188-disp-gamma
+              - mediatek,mt8196-disp-gamma
           - const: mediatek,mt8195-disp-gamma
 
   reg:
-- 
2.18.0

[PATCH 7/7] dt-bindings: display: mediatek: tdshp: Add support for MT8196

[Jay Liu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 1860 bytes --]

Add a compatible string for MediaTek MT8196 SoC

Signed-off-by: Jay Liu <jay.liu@mediatek.com>
---
 .../display/mediatek/mediatek,tdshp.yaml      | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp.yaml

diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp.yaml
new file mode 100644
index 000000000000..5ed7bdd53d0e
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/mediatek/mediatek,tdshp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek display clarity correction
+
+maintainers:
+  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
+  - Philipp Zabel <p.zabel@pengutronix.de>
+
+description: |
+  MediaTek display clarity correction, namely TDSHP, provides a
+  operation used to adjust sharpness in display system.
+  TDSHP device node must be siblings to the central MMSYS_CONFIG node.
+  For a description of the MMSYS_CONFIG binding, see
+  Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
+  for details.
+properties:
+  compatible:
+    oneOf:
+      - enum:
+          - mediatek,mt8196-disp-tdshp
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        tdshp@321e0000 {
+            compatible = "mediatek,mt8196-disp-tdshp";
+            reg = <0 0x321e0000 0 0x1000>;
+            clocks = <&dispsys_config_clk 107>;
+        };
+    };
-- 
2.18.0

Re: [PATCH 7/7] dt-bindings: display: mediatek: tdshp: Add support for MT8196

[Krzysztof Kozlowski]

On 19/02/2025 10:20, Jay Liu wrote:
> Add a compatible string for MediaTek MT8196 SoC

No, this is just bogus commit msg.

You did not try enough, just pasted same useless and incorrect message
to every patch.

> 
> Signed-off-by: Jay Liu <jay.liu@mediatek.com>
> ---
>  .../display/mediatek/mediatek,tdshp.yaml      | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp.yaml
> new file mode 100644
> index 000000000000..5ed7bdd53d0e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp.yaml

Filename matching exactly compatible.

> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/mediatek/mediatek,tdshp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek display clarity correction
> +
> +maintainers:
> +  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
> +  - Philipp Zabel <p.zabel@pengutronix.de>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  MediaTek display clarity correction, namely TDSHP, provides a
> +  operation used to adjust sharpness in display system.
> +  TDSHP device node must be siblings to the central MMSYS_CONFIG node.
> +  For a description of the MMSYS_CONFIG binding, see
> +  Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
> +  for details.

Missing blank line. Do not introduce own style.

> +properties:
> +  compatible:
> +    oneOf:

Drop, unless this is already going to grow?


> +      - enum:
> +          - mediatek,mt8196-disp-tdshp
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +

Drop


Best regards,
Krzysztof

Re: [PATCH 7/7] dt-bindings: display: mediatek: tdshp: Add support for MT8196

[AngeloGioacchino Del Regno]

Il 19/02/25 10:25, Krzysztof Kozlowski ha scritto:
> On 19/02/2025 10:20, Jay Liu wrote:
>> Add a compatible string for MediaTek MT8196 SoC
> 
> No, this is just bogus commit msg.
> 
> You did not try enough, just pasted same useless and incorrect message
> to every patch.
> 
>>
>> Signed-off-by: Jay Liu <jay.liu@mediatek.com>
>> ---
>>   .../display/mediatek/mediatek,tdshp.yaml      | 51 +++++++++++++++++++
>>   1 file changed, 51 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp.yaml
>> new file mode 100644
>> index 000000000000..5ed7bdd53d0e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp.yaml
> 
> Filename matching exactly compatible.
> 
>> @@ -0,0 +1,51 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/mediatek/mediatek,tdshp.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MediaTek display clarity correction


Adding more comments to Krzysztof's review....

What does "TDSHP" stand for? "Display Clarity Correction" rolls up as "DCC" which
is not "TDSHP".

Please clarify the title by unrolling "TDSHP"

title: MediaTek T.. Display... S... H... P

>> +
>> +maintainers:
>> +  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
>> +  - Philipp Zabel <p.zabel@pengutronix.de>
>> +
>> +description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
>> +  MediaTek display clarity correction, namely TDSHP, provides a

Again, TDSHP does not stand for "display clarity correction" - that's what it is
for, and it is ok to say what it is for, but say what TDSHP stands for.

MediaTek T... Display Sharpness? (TDSHP) provides means to adjust
the image sharpness displayed on a physical screen, therefore this
IP is meant to perform display clarity correction.

...rest of the blurb, etc.

>> +  operation used to adjust sharpness in display system.
>> +  TDSHP device node must be siblings to the central MMSYS_CONFIG node.
>> +  For a description of the MMSYS_CONFIG binding, see
>> +  Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
>> +  for details.
> 
> Missing blank line. Do not introduce own style.
> 
>> +properties:
>> +  compatible:
>> +    oneOf:
> 
> Drop, unless this is already going to grow?
> 
> 

krzk: oh, it is, guaranteed!! but ... not exactly right now (not very soon),
so dropping the oneOf is a sane recommendation, I agree.


Cheers,
Angelo

>> +      - enum:
>> +          - mediatek,mt8196-disp-tdshp
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +
> 
> Drop
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 7/7] dt-bindings: display: mediatek: tdshp: Add support for MT8196

[Jay Liu (刘博)]

On Wed, 2025-02-19 at 10:25 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 19/02/2025 10:20, Jay Liu wrote:
> > Add a compatible string for MediaTek MT8196 SoC
> 
> No, this is just bogus commit msg.
> 
> You did not try enough, just pasted same useless and incorrect
> message
> to every patch.
> 

hi Krzysztof ,I will try fix those issue in next version

Thanks a lot
JAY

> > 
> > Signed-off-by: Jay Liu <jay.liu@mediatek.com>
> > ---
> >  .../display/mediatek/mediatek,tdshp.yaml      | 51
> > +++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp.y
> > aml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp
> > .yaml
> > b/Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp
> > .yaml
> > new file mode 100644
> > index 000000000000..5ed7bdd53d0e
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/display/mediatek/mediatek,tdshp
> > .yaml
> 
> Filename matching exactly compatible.
> 
> > @@ -0,0 +1,51 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/display/mediatek/mediatek,tdshp.yaml*__;Iw!!CTRNKA9wMg0ARbw!nr5z3xMTBUfbNfmzkP2P4_gs0hHjv8OXemH52ipgq-YNOW4IVHIMa1cYYnYmYWdanFFaigHTwloX$
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!nr5z3xMTBUfbNfmzkP2P4_gs0hHjv8OXemH52ipgq-YNOW4IVHIMa1cYYnYmYWdanFFairWCuJ_4$
> > +
> > +title: MediaTek display clarity correction
> > +
> > +maintainers:
> > +  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
> > +  - Philipp Zabel <p.zabel@pengutronix.de>
> > +
> > +description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
> > +  MediaTek display clarity correction, namely TDSHP, provides a
> > +  operation used to adjust sharpness in display system.
> > +  TDSHP device node must be siblings to the central MMSYS_CONFIG
> > node.
> > +  For a description of the MMSYS_CONFIG binding, see
> > +  Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.ya
> > ml
> > +  for details.
> 
> Missing blank line. Do not introduce own style.
> 
> > +properties:
> > +  compatible:
> > +    oneOf:
> 
> Drop, unless this is already going to grow?
> 
> 
> > +      - enum:
> > +          - mediatek,mt8196-disp-tdshp
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> 
> Drop
> 
> 
> Best regards,
> Krzysztof