[PATCH 0/3] Add gamma lut support for mt8195

[PATCH 0/3] Add gamma lut support for mt8195

[zheng-yan.chen]

Since the gamma_set_common() function for previous SoC,
such as  mt8173 and mt8183, is designed for 9bit-to-10bit
conversion.
mt8195 is using 10bit-to-12bit conversion, which is
not compatible with the previous function.

Thus, need to update the function to fit the need of mt8195.

---
Base on series [1]:
[1]: Add driver nodes for MT8195 SoC
- https://patchwork.kernel.org/project/linux-mediatek/list/?series=666741
---

zheng-yan.chen (3):
  dt-bindings: mediatek: Add gamma compatible for mt8195
  drm/mediatek: Add gamma lut support for mt8195
  arm64: dts: Modify gamma compatible for mt8195

 .../display/mediatek/mediatek,gamma.yaml      |  3 +-
 arch/arm64/boot/dts/mediatek/mt8195.dtsi      |  4 +-
 drivers/gpu/drm/mediatek/mtk_disp_aal.c       |  2 +-
 drivers/gpu/drm/mediatek/mtk_disp_drv.h       |  3 +-
 drivers/gpu/drm/mediatek/mtk_disp_gamma.c     | 97 ++++++++++++++-----
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c       |  5 +-
 drivers/gpu/drm/mediatek/mtk_drm_crtc.h       |  1 -
 drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c   |  1 +
 drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h   |  1 +
 drivers/gpu/drm/mediatek/mtk_drm_drv.c        |  2 +
 10 files changed, 87 insertions(+), 32 deletions(-)

-- 
2.18.0

[PATCH 1/3] dt-bindings: mediatek: Add gamma compatible for mt8195

[zheng-yan.chen]

mt8195 uses 10bit-to-12bit gamma-LUT, which is different from
current 9bit-to-10bit gamma-LUT, so this patch add its own compatible
for mt8195.

Signed-off-by: zheng-yan.chen <zheng-yan.chen@mediatek.com>

---
 .../devicetree/bindings/display/mediatek/mediatek,gamma.yaml   | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
index a89ea0ea7542..fbd7b9664a78 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
@@ -25,11 +25,12 @@ properties:
           - const: mediatek,mt8173-disp-gamma
       - items:
           - const: mediatek,mt8183-disp-gamma
+      - items:
+          - const: mediatek,mt8195-disp-gamma
       - items:
           - enum:
               - mediatek,mt8186-disp-gamma
               - mediatek,mt8192-disp-gamma
-              - mediatek,mt8195-disp-gamma
           - const: mediatek,mt8183-disp-gamma
 
   reg:
-- 
2.18.0

Re: [PATCH 1/3] dt-bindings: mediatek: Add gamma compatible for mt8195

[Krzysztof Kozlowski]

On 22/08/2022 12:19, zheng-yan.chen wrote:
> mt8195 uses 10bit-to-12bit gamma-LUT, which is different from
> current 9bit-to-10bit gamma-LUT, so this patch add its own compatible
> for mt8195.

I am not sure if this explains the need for change. Is mt8195 still
compatible with mt8183 or not? Your driver change suggests that it is
and points that this commit is wrong.

> 
> Signed-off-by: zheng-yan.chen <zheng-yan.chen@mediatek.com>
> 
> ---
>  .../devicetree/bindings/display/mediatek/mediatek,gamma.yaml   | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
> index a89ea0ea7542..fbd7b9664a78 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
> @@ -25,11 +25,12 @@ properties:
>            - const: mediatek,mt8173-disp-gamma
>        - items:
>            - const: mediatek,mt8183-disp-gamma
> +      - items:
> +          - const: mediatek,mt8195-disp-gamma
>        - items:
>            - enum:
>                - mediatek,mt8186-disp-gamma
>                - mediatek,mt8192-disp-gamma
> -              - mediatek,mt8195-disp-gamma


Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: mediatek: Add gamma compatible for mt8195

[zheng-yan.chen]

On Tue, 2022-08-23 at 11:38 +0300, Krzysztof Kozlowski wrote:
> On 22/08/2022 12:19, zheng-yan.chen wrote:
> > mt8195 uses 10bit-to-12bit gamma-LUT, which is different from
> > current 9bit-to-10bit gamma-LUT, so this patch add its own
> > compatible
> > for mt8195.
> 
> I am not sure if this explains the need for change. Is mt8195 still
> compatible with mt8183 or not? Your driver change suggests that it is
> and points that this commit is wrong.
> 
> > 
> > Signed-off-by: zheng-yan.chen <zheng-yan.chen@mediatek.com>
> > 
> > ---
> >  .../devicetree/bindings/display/mediatek/mediatek,gamma.yaml   | 3
> > ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma
> > .yaml
> > b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma
> > .yaml
> > index a89ea0ea7542..fbd7b9664a78 100644
> > ---
> > a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma
> > .yaml
> > +++
> > b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma
> > .yaml
> > @@ -25,11 +25,12 @@ properties:
> >            - const: mediatek,mt8173-disp-gamma
> >        - items:
> >            - const: mediatek,mt8183-disp-gamma
> > +      - items:
> > +          - const: mediatek,mt8195-disp-gamma
> >        - items:
> >            - enum:
> >                - mediatek,mt8186-disp-gamma
> >                - mediatek,mt8192-disp-gamma
> > -              - mediatek,mt8195-disp-gamma
> 
> 
> Best regards,
> Krzysztof

mt8195 is not compatible with mt8183 now, I will change commit message
to:

   mt8195 uses 10bit-to-12bit gamma-LUT, which is not compatible with
current 9bit-to-10bit gamma-LUT.
    
    This patch thus add constant compatible for mt8195, which means
that mt8195 should only use specified mt8195 gamma driver data.    
    
    Also, delete related compatible from enum, to ensure that
    mt8195 will not accidentally get others' gamma driver data and thus
    cause fatal error.

Best regards,
Zheng-Yan Chen

[PATCH 2/3] drm/mediatek: Add gamma lut support for mt8195

[zheng-yan.chen]

Since the previous gamma_set_common() function is designed for
9bit-to-10bit conversion, which is not feasible for the
10bit-to-12bit conversion in mt8195.

Update the function to fit the need of mt8195.

Signed-off-by: zheng-yan.chen <zheng-yan.chen@mediatek.com>

---
 drivers/gpu/drm/mediatek/mtk_disp_aal.c     |  2 +-
 drivers/gpu/drm/mediatek/mtk_disp_drv.h     |  3 +-
 drivers/gpu/drm/mediatek/mtk_disp_gamma.c   | 97 ++++++++++++++++-----
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c     |  5 +-
 drivers/gpu/drm/mediatek/mtk_drm_crtc.h     |  1 -
 drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c |  1 +
 drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |  1 +
 drivers/gpu/drm/mediatek/mtk_drm_drv.c      |  2 +
 8 files changed, 83 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_aal.c b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
index 0f9d7efb61d7..f46d4ab73d6a 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
@@ -66,7 +66,7 @@ void mtk_aal_gamma_set(struct device *dev, struct drm_crtc_state *state)
 	struct mtk_disp_aal *aal = dev_get_drvdata(dev);
 
 	if (aal->data && aal->data->has_gamma)
-		mtk_gamma_set_common(aal->regs, state, false);
+		mtk_gamma_set_common(aal->regs, state);
 }
 
 void mtk_aal_start(struct device *dev)
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
index 33e61a136bbc..b662bf8b1c9d 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
@@ -51,8 +51,9 @@ void mtk_gamma_clk_disable(struct device *dev);
 void mtk_gamma_config(struct device *dev, unsigned int w,
 		      unsigned int h, unsigned int vrefresh,
 		      unsigned int bpc, struct cmdq_pkt *cmdq_pkt);
+unsigned int mtk_gamma_size(struct device *dev);
 void mtk_gamma_set(struct device *dev, struct drm_crtc_state *state);
-void mtk_gamma_set_common(void __iomem *regs, struct drm_crtc_state *state, bool lut_diff);
+void mtk_gamma_set_common(void __iomem *regs, struct drm_crtc_state *state);
 void mtk_gamma_start(struct device *dev);
 void mtk_gamma_stop(struct device *dev);
 
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
index bbd558a036ec..a842e5e1962e 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -18,18 +18,26 @@
 #define DISP_GAMMA_EN				0x0000
 #define GAMMA_EN					BIT(0)
 #define DISP_GAMMA_CFG				0x0020
+#define RELAY_MODE					BIT(0)
 #define GAMMA_LUT_EN					BIT(1)
 #define GAMMA_DITHERING					BIT(2)
 #define DISP_GAMMA_SIZE				0x0030
+#define DISP_GAMMA_BANK				0x0100
 #define DISP_GAMMA_LUT				0x0700
-
+#define DISP_GAMMA_LUT1				0x0b00
 #define LUT_10BIT_MASK				0x03ff
-
+#define TABLE_9BIT_SIZE				512
+#define LUT_12BIT_MASK				0x0fff
+#define TABLE_10BIT_SIZE			1024
+#define BANK_SIZE				256
 struct mtk_disp_gamma_data {
 	bool has_dither;
 	bool lut_diff;
+	unsigned int lut_size;
 };
 
+static unsigned int now_lut_size;
+static bool now_lut_diff;
 /*
  * struct mtk_disp_gamma - DISP_GAMMA driver structure
  */
@@ -54,40 +62,73 @@ void mtk_gamma_clk_disable(struct device *dev)
 	clk_disable_unprepare(gamma->clk);
 }
 
-void mtk_gamma_set_common(void __iomem *regs, struct drm_crtc_state *state, bool lut_diff)
+void mtk_gamma_set_common(void __iomem *regs, struct drm_crtc_state *state)
 {
-	unsigned int i, reg;
-	struct drm_color_lut *lut;
+	unsigned int i, reg, idx;
 	void __iomem *lut_base;
-	u32 word;
-	u32 diff[3] = {0};
+	void __iomem *lut1_base;
+	u32 word, word1;
 
 	if (state->gamma_lut) {
+		u32 table_size;
+		u32 mask;
+		struct drm_color_lut color, even, odd;
+		struct drm_color_lut *lut = (struct drm_color_lut *)state->gamma_lut;
+		bool lut_12bit = (now_lut_size == TABLE_10BIT_SIZE);
+
 		reg = readl(regs + DISP_GAMMA_CFG);
+		reg = reg & ~RELAY_MODE;
 		reg = reg | GAMMA_LUT_EN;
 		writel(reg, regs + DISP_GAMMA_CFG);
 		lut_base = regs + DISP_GAMMA_LUT;
-		lut = (struct drm_color_lut *)state->gamma_lut->data;
-		for (i = 0; i < MTK_LUT_SIZE; i++) {
+		lut1_base = regs + DISP_GAMMA_LUT1;
+		if (lut_12bit) {
+			table_size = TABLE_10BIT_SIZE;
+			mask = LUT_12BIT_MASK;
+		} else {
+			table_size = TABLE_9BIT_SIZE;
+			mask = LUT_10BIT_MASK;
+		}
 
-			if (!lut_diff || (i % 2 == 0)) {
-				word = (((lut[i].red >> 6) & LUT_10BIT_MASK) << 20) +
-					(((lut[i].green >> 6) & LUT_10BIT_MASK) << 10) +
-					((lut[i].blue >> 6) & LUT_10BIT_MASK);
+		for (i = 0; i < table_size; i++) {
+			if (!(i % BANK_SIZE) && lut_12bit)
+				writel((i / BANK_SIZE), regs + DISP_GAMMA_BANK);
+
+			color.red = (lut[i].red >> 6) & mask;
+			color.green = (lut[i].green >> 6) & mask;
+			color.blue = (lut[i].blue >> 6) & mask;
+			if ((i % 2) && now_lut_diff) {
+				odd = color;
+				word = (lut_12bit) ? (((odd.green - even.green) << 12) +
+						      (odd.red - even.red))
+						   : (((odd.red - even.red) << 20) +
+						      ((odd.green - even.green) << 10) +
+						      (odd.blue - even.blue));
+				word1 = (odd.blue - even.blue);
 			} else {
-				diff[0] = (lut[i].red >> 6) - (lut[i - 1].red >> 6);
-				diff[1] = (lut[i].green >> 6) - (lut[i - 1].green >> 6);
-				diff[2] = (lut[i].blue >> 6) - (lut[i - 1].blue >> 6);
-
-				word = ((diff[0] & LUT_10BIT_MASK) << 20) +
-					((diff[1] & LUT_10BIT_MASK) << 10) +
-					(diff[2] & LUT_10BIT_MASK);
+				even = color;
+				word =  (lut_12bit) ? ((even.green << 12) + even.red)
+						    : ((even.red << 20) +
+						       (even.green << 10) + even.blue);
+				word1 = even.blue;
 			}
-			writel(word, (lut_base + i * 4));
+			idx = (lut_12bit) ? (i % BANK_SIZE) : i;
+			writel(word, (lut_base + idx * 4));
+			if (lut_12bit)
+				writel(word1, (lut1_base + idx * 4));
 		}
 	}
 }
 
+unsigned int mtk_gamma_size(struct device *dev)
+{
+	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
+
+	if (gamma->data)
+		return gamma->data->lut_size;
+	else
+		return 0;
+}
 void mtk_gamma_set(struct device *dev, struct drm_crtc_state *state)
 {
 	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
@@ -95,8 +136,7 @@ void mtk_gamma_set(struct device *dev, struct drm_crtc_state *state)
 
 	if (gamma->data)
 		lut_diff = gamma->data->lut_diff;
-
-	mtk_gamma_set_common(gamma->regs, state, lut_diff);
+	mtk_gamma_set_common(gamma->regs, state);
 }
 
 void mtk_gamma_config(struct device *dev, unsigned int w,
@@ -178,6 +218,8 @@ static int mtk_disp_gamma_probe(struct platform_device *pdev)
 	ret = component_add(dev, &mtk_disp_gamma_component_ops);
 	if (ret)
 		dev_err(dev, "Failed to add component: %d\n", ret);
+	now_lut_size = priv->data->lut_size;
+	now_lut_diff = priv->data->lut_diff;
 
 	return ret;
 }
@@ -191,10 +233,17 @@ static int mtk_disp_gamma_remove(struct platform_device *pdev)
 
 static const struct mtk_disp_gamma_data mt8173_gamma_driver_data = {
 	.has_dither = true,
+	.lut_size = 512,
 };
 
 static const struct mtk_disp_gamma_data mt8183_gamma_driver_data = {
 	.lut_diff = true,
+	.lut_size = 512,
+};
+
+static const struct mtk_disp_gamma_data mt8195_gamma_driver_data = {
+	.lut_diff = true,
+	.lut_size = 1024,
 };
 
 static const struct of_device_id mtk_disp_gamma_driver_dt_match[] = {
@@ -202,6 +251,8 @@ static const struct of_device_id mtk_disp_gamma_driver_dt_match[] = {
 	  .data = &mt8173_gamma_driver_data},
 	{ .compatible = "mediatek,mt8183-disp-gamma",
 	  .data = &mt8183_gamma_driver_data},
+	{ .compatible = "mediatek,mt8195-disp-gamma",
+	  .data = &mt8195_gamma_driver_data},
 	{},
 };
 MODULE_DEVICE_TABLE(of, mtk_disp_gamma_driver_dt_match);
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 42cc7052b050..2a6513259562 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -930,9 +930,8 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
 		mtk_crtc->ddp_comp[i] = comp;
 
 		if (comp->funcs) {
-			if (comp->funcs->gamma_set)
-				gamma_lut_size = MTK_LUT_SIZE;
-
+			if (comp->funcs->gamma_set && comp->funcs->gamma_size)
+				gamma_lut_size = comp->funcs->gamma_size(comp->dev);
 			if (comp->funcs->ctm_set)
 				has_ctm = true;
 		}
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
index cb9a36c48d4f..1799853ef89a 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
@@ -10,7 +10,6 @@
 #include "mtk_drm_ddp_comp.h"
 #include "mtk_drm_plane.h"
 
-#define MTK_LUT_SIZE	512
 #define MTK_MAX_BPC	10
 #define MTK_MIN_BPC	3
 
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
index 2d72cc5ddaba..4c6538a17b88 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
@@ -323,6 +323,7 @@ static const struct mtk_ddp_comp_funcs ddp_gamma = {
 	.clk_enable = mtk_gamma_clk_enable,
 	.clk_disable = mtk_gamma_clk_disable,
 	.gamma_set = mtk_gamma_set,
+	.gamma_size = mtk_gamma_size,
 	.config = mtk_gamma_config,
 	.start = mtk_gamma_start,
 	.stop = mtk_gamma_stop,
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
index 2d0052c23dcb..bf0cf7f86010 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
@@ -59,6 +59,7 @@ struct mtk_ddp_comp_funcs {
 	void (*disable_vblank)(struct device *dev);
 	unsigned int (*supported_rotations)(struct device *dev);
 	unsigned int (*layer_nr)(struct device *dev);
+	unsigned int (*gamma_size)(struct device *dev);
 	int (*layer_check)(struct device *dev,
 			   unsigned int idx,
 			   struct mtk_plane_state *state);
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 0e4c77724b05..473766be56e1 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -567,6 +567,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = {
 	  .data = (void *)MTK_DISP_GAMMA, },
 	{ .compatible = "mediatek,mt8183-disp-gamma",
 	  .data = (void *)MTK_DISP_GAMMA, },
+	{ .compatible = "mediatek,mt8195-disp-gamma",
+	  .data = (void *)MTK_DISP_GAMMA, },
 	{ .compatible = "mediatek,mt8195-disp-merge",
 	  .data = (void *)MTK_DISP_MERGE },
 	{ .compatible = "mediatek,mt2701-disp-mutex",
-- 
2.18.0

Re: [PATCH 2/3] drm/mediatek: Add gamma lut support for mt8195

[CK Hu]

Hi, Zheng-yan:

On Mon, 2022-08-22 at 17:19 +0800, zheng-yan.chen wrote:
> Since the previous gamma_set_common() function is designed for
> 9bit-to-10bit conversion, which is not feasible for the
> 10bit-to-12bit conversion in mt8195.
> 
> Update the function to fit the need of mt8195.
> 

Add Fixes tag [1].

[1] 
https://www.kernel.org/doc/html/v5.19/process/submitting-patches.html


> Signed-off-by: zheng-yan.chen <zheng-yan.chen@mediatek.com>
> 
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_aal.c     |  2 +-
>  drivers/gpu/drm/mediatek/mtk_disp_drv.h     |  3 +-
>  drivers/gpu/drm/mediatek/mtk_disp_gamma.c   | 97 ++++++++++++++++---
> --
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c     |  5 +-
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.h     |  1 -
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c |  1 +
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |  1 +
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c      |  2 +
>  8 files changed, 83 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> index 0f9d7efb61d7..f46d4ab73d6a 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> @@ -66,7 +66,7 @@ void mtk_aal_gamma_set(struct device *dev, struct
> drm_crtc_state *state)
>  	struct mtk_disp_aal *aal = dev_get_drvdata(dev);
>  
>  	if (aal->data && aal->data->has_gamma)
> -		mtk_gamma_set_common(aal->regs, state, false);
> +		mtk_gamma_set_common(aal->regs, state);
>  }
>  
>  void mtk_aal_start(struct device *dev)
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index 33e61a136bbc..b662bf8b1c9d 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -51,8 +51,9 @@ void mtk_gamma_clk_disable(struct device *dev);
>  void mtk_gamma_config(struct device *dev, unsigned int w,
>  		      unsigned int h, unsigned int vrefresh,
>  		      unsigned int bpc, struct cmdq_pkt *cmdq_pkt);
> +unsigned int mtk_gamma_size(struct device *dev);
>  void mtk_gamma_set(struct device *dev, struct drm_crtc_state
> *state);
> -void mtk_gamma_set_common(void __iomem *regs, struct drm_crtc_state
> *state, bool lut_diff);
> +void mtk_gamma_set_common(void __iomem *regs, struct drm_crtc_state
> *state);
>  void mtk_gamma_start(struct device *dev);
>  void mtk_gamma_stop(struct device *dev);
>  
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> index bbd558a036ec..a842e5e1962e 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> @@ -18,18 +18,26 @@
>  #define DISP_GAMMA_EN				0x0000
>  #define GAMMA_EN					BIT(0)
>  #define DISP_GAMMA_CFG				0x0020
> +#define RELAY_MODE					BIT(0)
>  #define GAMMA_LUT_EN					BIT(1)
>  #define GAMMA_DITHERING					BIT(2)
>  #define DISP_GAMMA_SIZE				0x0030
> +#define DISP_GAMMA_BANK				0x0100
>  #define DISP_GAMMA_LUT				0x0700
> -
> +#define DISP_GAMMA_LUT1				0x0b00
>  #define LUT_10BIT_MASK				0x03ff
> -
> +#define TABLE_9BIT_SIZE				512
> +#define LUT_12BIT_MASK				0x0fff
> +#define TABLE_10BIT_SIZE			1024
> +#define BANK_SIZE				256
>  struct mtk_disp_gamma_data {
>  	bool has_dither;
>  	bool lut_diff;
> +	unsigned int lut_size;
>  };
>  
> +static unsigned int now_lut_size;
> +static bool now_lut_diff;

Don't use global variable.

>  /*
>   * struct mtk_disp_gamma - DISP_GAMMA driver structure
>   */
> @@ -54,40 +62,73 @@ void mtk_gamma_clk_disable(struct device *dev)
>  	clk_disable_unprepare(gamma->clk);
>  }
>  
> -void mtk_gamma_set_common(void __iomem *regs, struct drm_crtc_state
> *state, bool lut_diff)
> +void mtk_gamma_set_common(void __iomem *regs, struct drm_crtc_state
> *state)
>  {
> -	unsigned int i, reg;
> -	struct drm_color_lut *lut;
> +	unsigned int i, reg, idx;
>  	void __iomem *lut_base;
> -	u32 word;
> -	u32 diff[3] = {0};
> +	void __iomem *lut1_base;
> +	u32 word, word1;
>  
>  	if (state->gamma_lut) {
> +		u32 table_size;
> +		u32 mask;
> +		struct drm_color_lut color, even, odd;
> +		struct drm_color_lut *lut = (struct drm_color_lut
> *)state->gamma_lut;
> +		bool lut_12bit = (now_lut_size == TABLE_10BIT_SIZE);
> +
>  		reg = readl(regs + DISP_GAMMA_CFG);
> +		reg = reg & ~RELAY_MODE;

Why do you modify this for other SoC?

>  		reg = reg | GAMMA_LUT_EN;
>  		writel(reg, regs + DISP_GAMMA_CFG);
>  		lut_base = regs + DISP_GAMMA_LUT;
> -		lut = (struct drm_color_lut *)state->gamma_lut->data;
> -		for (i = 0; i < MTK_LUT_SIZE; i++) {
> +		lut1_base = regs + DISP_GAMMA_LUT1;
> +		if (lut_12bit) {
> +			table_size = TABLE_10BIT_SIZE;
> +			mask = LUT_12BIT_MASK;

lut_bits and lut_size are independent factor, so use two variable in
private data for each. And I like to use digital value instead of a
symbol.

lut_bits = 10 or 12;
lut_size = 512 or 1024;
mask = GENMASK(lut_bits - 1, 0);

> +		} else {
> +			table_size = TABLE_9BIT_SIZE;
> +			mask = LUT_10BIT_MASK;
> +		}
>  
> -			if (!lut_diff || (i % 2 == 0)) {
> -				word = (((lut[i].red >> 6) &
> LUT_10BIT_MASK) << 20) +
> -					(((lut[i].green >> 6) &
> LUT_10BIT_MASK) << 10) +
> -					((lut[i].blue >> 6) &
> LUT_10BIT_MASK);
> +		for (i = 0; i < table_size; i++) {
> +			if (!(i % BANK_SIZE) && lut_12bit)
> +				writel((i / BANK_SIZE), regs +
> DISP_GAMMA_BANK);

Group register writing together, so move this to bottom of this for-
loop.

> +
> +			color.red = (lut[i].red >> 6) & mask;
> +			color.green = (lut[i].green >> 6) & mask;
> +			color.blue = (lut[i].blue >> 6) & mask;

Why shift 6 bits for lut 12 bits?

> +			if ((i % 2) && now_lut_diff) {

In original code, !lut_diff is first, so I would like you to keep this
order.

> +				odd = color;
> +				word = (lut_12bit) ? (((odd.green -
> even.green) << 12) +
> +						      (odd.red -
> even.red))
> +						   : (((odd.red -
> even.red) << 20) +
> +						      ((odd.green -
> even.green) << 10) +
> +						      (odd.blue -
> even.blue));
> +				word1 = (odd.blue - even.blue);

I think it's not necessary to create odd and even variable. 

Regards,
CK

>  			} else {
> -				diff[0] = (lut[i].red >> 6) - (lut[i -
> 1].red >> 6);
> -				diff[1] = (lut[i].green >> 6) - (lut[i
> - 1].green >> 6);
> -				diff[2] = (lut[i].blue >> 6) - (lut[i -
> 1].blue >> 6);
> -
> -				word = ((diff[0] & LUT_10BIT_MASK) <<
> 20) +
> -					((diff[1] & LUT_10BIT_MASK) <<
> 10) +
> -					(diff[2] & LUT_10BIT_MASK);
> +				even = color;
> +				word =  (lut_12bit) ? ((even.green <<
> 12) + even.red)
> +						    : ((even.red << 20)
> +
> +						       (even.green <<
> 10) + even.blue);
> +				word1 = even.blue;
>  			}
> -			writel(word, (lut_base + i * 4));
> +			idx = (lut_12bit) ? (i % BANK_SIZE) : i;
> +			writel(word, (lut_base + idx * 4));
> +			if (lut_12bit)
> +				writel(word1, (lut1_base + idx * 4));
>  		}
>  	}
>  }
>  
> +unsigned int mtk_gamma_size(struct device *dev)
> +{
> +	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> +
> +	if (gamma->data)
> +		return gamma->data->lut_size;
> +	else
> +		return 0;
> +}
>  void mtk_gamma_set(struct device *dev, struct drm_crtc_state *state)
>  {
>  	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> @@ -95,8 +136,7 @@ void mtk_gamma_set(struct device *dev, struct
> drm_crtc_state *state)
>  
>  	if (gamma->data)
>  		lut_diff = gamma->data->lut_diff;
> -
> -	mtk_gamma_set_common(gamma->regs, state, lut_diff);
> +	mtk_gamma_set_common(gamma->regs, state);
>  }
>  
>  void mtk_gamma_config(struct device *dev, unsigned int w,
> @@ -178,6 +218,8 @@ static int mtk_disp_gamma_probe(struct
> platform_device *pdev)
>  	ret = component_add(dev, &mtk_disp_gamma_component_ops);
>  	if (ret)
>  		dev_err(dev, "Failed to add component: %d\n", ret);
> +	now_lut_size = priv->data->lut_size;
> +	now_lut_diff = priv->data->lut_diff;
>  
>  	return ret;
>  }
> @@ -191,10 +233,17 @@ static int mtk_disp_gamma_remove(struct
> platform_device *pdev)
>  
>  static const struct mtk_disp_gamma_data mt8173_gamma_driver_data = {
>  	.has_dither = true,
> +	.lut_size = 512,
>  };
>  
>  static const struct mtk_disp_gamma_data mt8183_gamma_driver_data = {
>  	.lut_diff = true,
> +	.lut_size = 512,
> +};
> +
> +static const struct mtk_disp_gamma_data mt8195_gamma_driver_data = {
> +	.lut_diff = true,
> +	.lut_size = 1024,
>  };
>  
>  static const struct of_device_id mtk_disp_gamma_driver_dt_match[] =
> {
> @@ -202,6 +251,8 @@ static const struct of_device_id
> mtk_disp_gamma_driver_dt_match[] = {
>  	  .data = &mt8173_gamma_driver_data},
>  	{ .compatible = "mediatek,mt8183-disp-gamma",
>  	  .data = &mt8183_gamma_driver_data},
> +	{ .compatible = "mediatek,mt8195-disp-gamma",
> +	  .data = &mt8195_gamma_driver_data},
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, mtk_disp_gamma_driver_dt_match);
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 42cc7052b050..2a6513259562 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -930,9 +930,8 @@ int mtk_drm_crtc_create(struct drm_device
> *drm_dev,
>  		mtk_crtc->ddp_comp[i] = comp;
>  
>  		if (comp->funcs) {
> -			if (comp->funcs->gamma_set)
> -				gamma_lut_size = MTK_LUT_SIZE;
> -
> +			if (comp->funcs->gamma_set && comp->funcs-
> >gamma_size)
> +				gamma_lut_size = comp->funcs-
> >gamma_size(comp->dev);
>  			if (comp->funcs->ctm_set)
>  				has_ctm = true;
>  		}
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> index cb9a36c48d4f..1799853ef89a 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> @@ -10,7 +10,6 @@
>  #include "mtk_drm_ddp_comp.h"
>  #include "mtk_drm_plane.h"
>  
> -#define MTK_LUT_SIZE	512
>  #define MTK_MAX_BPC	10
>  #define MTK_MIN_BPC	3
>  
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> index 2d72cc5ddaba..4c6538a17b88 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -323,6 +323,7 @@ static const struct mtk_ddp_comp_funcs ddp_gamma
> = {
>  	.clk_enable = mtk_gamma_clk_enable,
>  	.clk_disable = mtk_gamma_clk_disable,
>  	.gamma_set = mtk_gamma_set,
> +	.gamma_size = mtk_gamma_size,
>  	.config = mtk_gamma_config,
>  	.start = mtk_gamma_start,
>  	.stop = mtk_gamma_stop,
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> index 2d0052c23dcb..bf0cf7f86010 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> @@ -59,6 +59,7 @@ struct mtk_ddp_comp_funcs {
>  	void (*disable_vblank)(struct device *dev);
>  	unsigned int (*supported_rotations)(struct device *dev);
>  	unsigned int (*layer_nr)(struct device *dev);
> +	unsigned int (*gamma_size)(struct device *dev);
>  	int (*layer_check)(struct device *dev,
>  			   unsigned int idx,
>  			   struct mtk_plane_state *state);
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 0e4c77724b05..473766be56e1 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -567,6 +567,8 @@ static const struct of_device_id
> mtk_ddp_comp_dt_ids[] = {
>  	  .data = (void *)MTK_DISP_GAMMA, },
>  	{ .compatible = "mediatek,mt8183-disp-gamma",
>  	  .data = (void *)MTK_DISP_GAMMA, },
> +	{ .compatible = "mediatek,mt8195-disp-gamma",
> +	  .data = (void *)MTK_DISP_GAMMA, },
>  	{ .compatible = "mediatek,mt8195-disp-merge",
>  	  .data = (void *)MTK_DISP_MERGE },
>  	{ .compatible = "mediatek,mt2701-disp-mutex",

Re: [PATCH 2/3] drm/mediatek: Add gamma lut support for mt8195

[zheng-yan.chen]

On Tue, 2022-08-23 at 11:18 +0800, CK Hu wrote:
> Hi, Zheng-yan:
> 
> On Mon, 2022-08-22 at 17:19 +0800, zheng-yan.chen wrote:
> > Since the previous gamma_set_common() function is designed for
> > 9bit-to-10bit conversion, which is not feasible for the
> > 10bit-to-12bit conversion in mt8195.
> > 
> > Update the function to fit the need of mt8195.
> > 
> 
> Add Fixes tag [1].
> 
> [1] 
> https://www.kernel.org/doc/html/v5.19/process/submitting-patches.html
> 
> 
> > Signed-off-by: zheng-yan.chen <zheng-yan.chen@mediatek.com>
> > 
> > ---
> >  drivers/gpu/drm/mediatek/mtk_disp_aal.c     |  2 +-
> >  drivers/gpu/drm/mediatek/mtk_disp_drv.h     |  3 +-
> >  drivers/gpu/drm/mediatek/mtk_disp_gamma.c   | 97 ++++++++++++++++-
> > --
> > --
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c     |  5 +-
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.h     |  1 -
> >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c |  1 +
> >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |  1 +
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.c      |  2 +
> >  8 files changed, 83 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> > b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> > index 0f9d7efb61d7..f46d4ab73d6a 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> > @@ -66,7 +66,7 @@ void mtk_aal_gamma_set(struct device *dev, struct
> > drm_crtc_state *state)
> >  	struct mtk_disp_aal *aal = dev_get_drvdata(dev);
> >  
> >  	if (aal->data && aal->data->has_gamma)
> > -		mtk_gamma_set_common(aal->regs, state, false);
> > +		mtk_gamma_set_common(aal->regs, state);
> >  }
> >  
> >  void mtk_aal_start(struct device *dev)
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > index 33e61a136bbc..b662bf8b1c9d 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > @@ -51,8 +51,9 @@ void mtk_gamma_clk_disable(struct device *dev);
> >  void mtk_gamma_config(struct device *dev, unsigned int w,
> >  		      unsigned int h, unsigned int vrefresh,
> >  		      unsigned int bpc, struct cmdq_pkt *cmdq_pkt);
> > +unsigned int mtk_gamma_size(struct device *dev);
> >  void mtk_gamma_set(struct device *dev, struct drm_crtc_state
> > *state);
> > -void mtk_gamma_set_common(void __iomem *regs, struct
> > drm_crtc_state
> > *state, bool lut_diff);
> > +void mtk_gamma_set_common(void __iomem *regs, struct
> > drm_crtc_state
> > *state);
> >  void mtk_gamma_start(struct device *dev);
> >  void mtk_gamma_stop(struct device *dev);
> >  
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > index bbd558a036ec..a842e5e1962e 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > @@ -18,18 +18,26 @@
> >  #define DISP_GAMMA_EN				0x0000
> >  #define GAMMA_EN					BIT(0)
> >  #define DISP_GAMMA_CFG				0x0020
> > +#define RELAY_MODE					BIT(0)
> >  #define GAMMA_LUT_EN					BIT(1)
> >  #define GAMMA_DITHERING					BIT(2)
> >  #define DISP_GAMMA_SIZE				0x0030
> > +#define DISP_GAMMA_BANK				0x0100
> >  #define DISP_GAMMA_LUT				0x0700
> > -
> > +#define DISP_GAMMA_LUT1				0x0b00
> >  #define LUT_10BIT_MASK				0x03ff
> > -
> > +#define TABLE_9BIT_SIZE				512
> > +#define LUT_12BIT_MASK				0x0fff
> > +#define TABLE_10BIT_SIZE			1024
> > +#define BANK_SIZE				256
> >  struct mtk_disp_gamma_data {
> >  	bool has_dither;
> >  	bool lut_diff;
> > +	unsigned int lut_size;
> >  };
> >  
> > +static unsigned int now_lut_size;
> > +static bool now_lut_diff;
> 
> Don't use global variable.
> 
> >  /*
> >   * struct mtk_disp_gamma - DISP_GAMMA driver structure
> >   */
> > @@ -54,40 +62,73 @@ void mtk_gamma_clk_disable(struct device *dev)
> >  	clk_disable_unprepare(gamma->clk);
> >  }
> >  
> > -void mtk_gamma_set_common(void __iomem *regs, struct
> > drm_crtc_state
> > *state, bool lut_diff)
> > +void mtk_gamma_set_common(void __iomem *regs, struct
> > drm_crtc_state
> > *state)
> >  {
> > -	unsigned int i, reg;
> > -	struct drm_color_lut *lut;
> > +	unsigned int i, reg, idx;
> >  	void __iomem *lut_base;
> > -	u32 word;
> > -	u32 diff[3] = {0};
> > +	void __iomem *lut1_base;
> > +	u32 word, word1;
> >  
> >  	if (state->gamma_lut) {
> > +		u32 table_size;
> > +		u32 mask;
> > +		struct drm_color_lut color, even, odd;
> > +		struct drm_color_lut *lut = (struct drm_color_lut
> > *)state->gamma_lut;
> > +		bool lut_12bit = (now_lut_size == TABLE_10BIT_SIZE);
> > +
> >  		reg = readl(regs + DISP_GAMMA_CFG);
> > +		reg = reg & ~RELAY_MODE;
> 
> Why do you modify this for other SoC?
> 
> >  		reg = reg | GAMMA_LUT_EN;
> >  		writel(reg, regs + DISP_GAMMA_CFG);
> >  		lut_base = regs + DISP_GAMMA_LUT;
> > -		lut = (struct drm_color_lut *)state->gamma_lut->data;
> > -		for (i = 0; i < MTK_LUT_SIZE; i++) {
> > +		lut1_base = regs + DISP_GAMMA_LUT1;
> > +		if (lut_12bit) {
> > +			table_size = TABLE_10BIT_SIZE;
> > +			mask = LUT_12BIT_MASK;
> 
> lut_bits and lut_size are independent factor, so use two variable in
> private data for each. And I like to use digital value instead of a
> symbol.
> 
> lut_bits = 10 or 12;
> lut_size = 512 or 1024;
> mask = GENMASK(lut_bits - 1, 0);
> 
> > +		} else {
> > +			table_size = TABLE_9BIT_SIZE;
> > +			mask = LUT_10BIT_MASK;
> > +		}
> >  
> > -			if (!lut_diff || (i % 2 == 0)) {
> > -				word = (((lut[i].red >> 6) &
> > LUT_10BIT_MASK) << 20) +
> > -					(((lut[i].green >> 6) &
> > LUT_10BIT_MASK) << 10) +
> > -					((lut[i].blue >> 6) &
> > LUT_10BIT_MASK);
> > +		for (i = 0; i < table_size; i++) {
> > +			if (!(i % BANK_SIZE) && lut_12bit)
> > +				writel((i / BANK_SIZE), regs +
> > DISP_GAMMA_BANK);
> 
> Group register writing together, so move this to bottom of this for-
> loop.
> 
> > +
> > +			color.red = (lut[i].red >> 6) & mask;
> > +			color.green = (lut[i].green >> 6) & mask;
> > +			color.blue = (lut[i].blue >> 6) & mask;
> 
> Why shift 6 bits for lut 12 bits?
> 
> > +			if ((i % 2) && now_lut_diff) {
> 
> In original code, !lut_diff is first, so I would like you to keep
> this
> order.
> 
> > +				odd = color;
> > +				word = (lut_12bit) ? (((odd.green -
> > even.green) << 12) +
> > +						      (odd.red -
> > even.red))
> > +						   : (((odd.red -
> > even.red) << 20) +
> > +						      ((odd.green -
> > even.green) << 10) +
> > +						      (odd.blue -
> > even.blue));
> > +				word1 = (odd.blue - even.blue);
> 
> I think it's not necessary to create odd and even variable. 
> 
> Regards,
> CK
> 
> >  			} else {
> > -				diff[0] = (lut[i].red >> 6) - (lut[i -
> > 1].red >> 6);
> > -				diff[1] = (lut[i].green >> 6) - (lut[i
> > - 1].green >> 6);
> > -				diff[2] = (lut[i].blue >> 6) - (lut[i -
> > 1].blue >> 6);
> > -
> > -				word = ((diff[0] & LUT_10BIT_MASK) <<
> > 20) +
> > -					((diff[1] & LUT_10BIT_MASK) <<
> > 10) +
> > -					(diff[2] & LUT_10BIT_MASK);
> > +				even = color;
> > +				word =  (lut_12bit) ? ((even.green <<
> > 12) + even.red)
> > +						    : ((even.red << 20)
> > +
> > +						       (even.green <<
> > 10) + even.blue);
> > +				word1 = even.blue;
> >  			}
> > -			writel(word, (lut_base + i * 4));
> > +			idx = (lut_12bit) ? (i % BANK_SIZE) : i;
> > +			writel(word, (lut_base + idx * 4));
> > +			if (lut_12bit)
> > +				writel(word1, (lut1_base + idx * 4));
> >  		}
> >  	}
> >  }
> >  
> > +unsigned int mtk_gamma_size(struct device *dev)
> > +{
> > +	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> > +
> > +	if (gamma->data)
> > +		return gamma->data->lut_size;
> > +	else
> > +		return 0;
> > +}
> >  void mtk_gamma_set(struct device *dev, struct drm_crtc_state
> > *state)
> >  {
> >  	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> > @@ -95,8 +136,7 @@ void mtk_gamma_set(struct device *dev, struct
> > drm_crtc_state *state)
> >  
> >  	if (gamma->data)
> >  		lut_diff = gamma->data->lut_diff;
> > -
> > -	mtk_gamma_set_common(gamma->regs, state, lut_diff);
> > +	mtk_gamma_set_common(gamma->regs, state);
> >  }
> >  
> >  void mtk_gamma_config(struct device *dev, unsigned int w,
> > @@ -178,6 +218,8 @@ static int mtk_disp_gamma_probe(struct
> > platform_device *pdev)
> >  	ret = component_add(dev, &mtk_disp_gamma_component_ops);
> >  	if (ret)
> >  		dev_err(dev, "Failed to add component: %d\n", ret);
> > +	now_lut_size = priv->data->lut_size;
> > +	now_lut_diff = priv->data->lut_diff;
> >  
> >  	return ret;
> >  }
> > @@ -191,10 +233,17 @@ static int mtk_disp_gamma_remove(struct
> > platform_device *pdev)
> >  
> >  static const struct mtk_disp_gamma_data mt8173_gamma_driver_data =
> > {
> >  	.has_dither = true,
> > +	.lut_size = 512,
> >  };
> >  
> >  static const struct mtk_disp_gamma_data mt8183_gamma_driver_data =
> > {
> >  	.lut_diff = true,
> > +	.lut_size = 512,
> > +};
> > +
> > +static const struct mtk_disp_gamma_data mt8195_gamma_driver_data =
> > {
> > +	.lut_diff = true,
> > +	.lut_size = 1024,
> >  };
> >  
> >  static const struct of_device_id mtk_disp_gamma_driver_dt_match[]
> > =
> > {
> > @@ -202,6 +251,8 @@ static const struct of_device_id
> > mtk_disp_gamma_driver_dt_match[] = {
> >  	  .data = &mt8173_gamma_driver_data},
> >  	{ .compatible = "mediatek,mt8183-disp-gamma",
> >  	  .data = &mt8183_gamma_driver_data},
> > +	{ .compatible = "mediatek,mt8195-disp-gamma",
> > +	  .data = &mt8195_gamma_driver_data},
> >  	{},
> >  };
> >  MODULE_DEVICE_TABLE(of, mtk_disp_gamma_driver_dt_match);
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index 42cc7052b050..2a6513259562 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -930,9 +930,8 @@ int mtk_drm_crtc_create(struct drm_device
> > *drm_dev,
> >  		mtk_crtc->ddp_comp[i] = comp;
> >  
> >  		if (comp->funcs) {
> > -			if (comp->funcs->gamma_set)
> > -				gamma_lut_size = MTK_LUT_SIZE;
> > -
> > +			if (comp->funcs->gamma_set && comp->funcs-
> > > gamma_size)
> > 
> > +				gamma_lut_size = comp->funcs-
> > > gamma_size(comp->dev);
> > 
> >  			if (comp->funcs->ctm_set)
> >  				has_ctm = true;
> >  		}
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> > index cb9a36c48d4f..1799853ef89a 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> > @@ -10,7 +10,6 @@
> >  #include "mtk_drm_ddp_comp.h"
> >  #include "mtk_drm_plane.h"
> >  
> > -#define MTK_LUT_SIZE	512
> >  #define MTK_MAX_BPC	10
> >  #define MTK_MIN_BPC	3
> >  
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > index 2d72cc5ddaba..4c6538a17b88 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > @@ -323,6 +323,7 @@ static const struct mtk_ddp_comp_funcs
> > ddp_gamma
> > = {
> >  	.clk_enable = mtk_gamma_clk_enable,
> >  	.clk_disable = mtk_gamma_clk_disable,
> >  	.gamma_set = mtk_gamma_set,
> > +	.gamma_size = mtk_gamma_size,
> >  	.config = mtk_gamma_config,
> >  	.start = mtk_gamma_start,
> >  	.stop = mtk_gamma_stop,
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > index 2d0052c23dcb..bf0cf7f86010 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > @@ -59,6 +59,7 @@ struct mtk_ddp_comp_funcs {
> >  	void (*disable_vblank)(struct device *dev);
> >  	unsigned int (*supported_rotations)(struct device *dev);
> >  	unsigned int (*layer_nr)(struct device *dev);
> > +	unsigned int (*gamma_size)(struct device *dev);
> >  	int (*layer_check)(struct device *dev,
> >  			   unsigned int idx,
> >  			   struct mtk_plane_state *state);
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > index 0e4c77724b05..473766be56e1 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > @@ -567,6 +567,8 @@ static const struct of_device_id
> > mtk_ddp_comp_dt_ids[] = {
> >  	  .data = (void *)MTK_DISP_GAMMA, },
> >  	{ .compatible = "mediatek,mt8183-disp-gamma",
> >  	  .data = (void *)MTK_DISP_GAMMA, },
> > +	{ .compatible = "mediatek,mt8195-disp-gamma",
> > +	  .data = (void *)MTK_DISP_GAMMA, },
> >  	{ .compatible = "mediatek,mt8195-disp-merge",
> >  	  .data = (void *)MTK_DISP_MERGE },
> >  	{ .compatible = "mediatek,mt2701-disp-mutex",
> 
> 
1. 

> +static unsigned int now_lut_size;
> +static bool now_lut_diff;

Don't use global variable.



	This two variables are only used in mtk_gamma_set_common()
function, which is called by only mtk_aal_gamma_set() and
mtk_gamma_set(). 

	Change definition of mtk_gamma_set_common() to something like
mtk_gamma_set_common( ..., dev)  
			to bring in "lut_size" and "lut_diff" from dev
private data.

2.

void mtk_gamma_set_common(void __iomem *regs, struct drm_crtc_state
> *state)
>  {
> -     unsigned int i, reg;
> -     struct drm_color_lut *lut;
> +     unsigned int i, reg, idx;
>       void __iomem *lut_base;
> -     u32 word;
> -     u32 diff[3] = {0};
> +     void __iomem *lut1_base;
> +     u32 word, word1;
>  
>       if (state->gamma_lut) {
> +             u32 table_size;
> +             u32 mask;
> +             struct drm_color_lut color, even, odd;
> +             struct drm_color_lut *lut = (struct drm_color_lut
> *)state->gamma_lut;
> +             bool lut_12bit = (now_lut_size == TABLE_10BIT_SIZE);
> +
>               reg = readl(regs + DISP_GAMMA_CFG);
> +             reg = reg & ~RELAY_MODE;

Why do you modify this for other SoC?


	Originally, gamma config initialization is in
mtk_gamma_config() , this function would write GAMMA_DITHERING into
gamma_CFG if .has_dither flag is true.
	
	This writing actually shut down the relay mode of gamma, thus,
in the original version only need to set GAMMA_LUT_EN to make use of
gamma.
	
	However, now .has_dither is false, which means
mtk_gamma_config() won't modify gamma_CFG anymore.
	
	Therefore, I added above code to manually shut down relay mode,
to ensure it running properly.


	 

3.

      reg = reg | GAMMA_LUT_EN;
>               writel(reg, regs + DISP_GAMMA_CFG);
>               lut_base = regs + DISP_GAMMA_LUT;
> -             lut = (struct drm_color_lut *)state->gamma_lut->data;
> -             for (i = 0; i < MTK_LUT_SIZE; i++) {
> +             lut1_base = regs + DISP_GAMMA_LUT1;
> +             if (lut_12bit) {
> +                     table_size = TABLE_10BIT_SIZE;
> +                     mask = LUT_12BIT_MASK;

lut_bits and lut_size are independent factor, so use two variable in
private data for each. And I like to use digital value instead of a
symbol.

lut_bits = 10 or 12;
lut_size = 512 or 1024;
mask = GENMASK(lut_bits - 1, 0);

> +             }


	OK, I will fix it.



4.

> +             for (i = 0; i < table_size; i++) {
> +                     if (!(i % BANK_SIZE) && lut_12bit)
> +                             writel((i / BANK_SIZE), regs +
> DISP_GAMMA_BANK);

Group register writing together, so move this to bottom of this for-
loop.


	OK, I will fix it.



5.

> +                     color.red = (lut[i].red >> 6) & mask;
> +                     color.green = (lut[i].green >> 6) & mask;
> +                     color.blue = (lut[i].blue >> 6) & mask;

Why shift 6 bits for lut 12 bits?


	After testing, I find that right-shift 6bits and mask the
Least-Significant(LS) 12bits bring the correct results.

	It actually make more sense to right-shift 4bits to gain the
12bits, however, when I did this,  the results failed.

	I wonder if this is the test data problem, since the original
test data is for mt8183 10bit lut.
	
	I will contact with designer and try to figure it out.  	



6.
> +                     if ((i % 2) && now_lut_diff) {

In original code, !lut_diff is first, so I would like you to keep this
order.


	OK, I will fix it.



7.
 +                             odd = color;
> +                             word = (lut_12bit) ? (((odd.green -
> even.green) << 12) +
> +                                                   (odd.red -
> even.red))
> +                                                : (((odd.red -
> even.red) << 20) +
> +                                                   ((odd.green -
> even.green) << 10) +
> +                                                   (odd.blue -
> even.blue));
> +                             word1 = (odd.blue - even.blue);

I think it's not necessary to create odd and even variable. 

Regards,
CK


	OK, I will fix it.

Re: [PATCH 2/3] drm/mediatek: Add gamma lut support for mt8195

[zheng-yan.chen]

On Wed, 2022-08-24 at 15:03 +0800, zheng-yan.chen wrote:
> On Tue, 2022-08-23 at 11:18 +0800, CK Hu wrote:
> > Hi, Zheng-yan:
> > 
> > On Mon, 2022-08-22 at 17:19 +0800, zheng-yan.chen wrote:
> > > Since the previous gamma_set_common() function is designed for
> > > 9bit-to-10bit conversion, which is not feasible for the
> > > 10bit-to-12bit conversion in mt8195.
> > > 
> > > Update the function to fit the need of mt8195.
> > > 
> > 
> > Add Fixes tag [1].
> > 
> > [1] 
> > 
https://www.kernel.org/doc/html/v5.19/process/submitting-patches.html
> > 
> > 
> > > Signed-off-by: zheng-yan.chen <zheng-yan.chen@mediatek.com>
> > > 
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_disp_aal.c     |  2 +-
> > >  drivers/gpu/drm/mediatek/mtk_disp_drv.h     |  3 +-
> > >  drivers/gpu/drm/mediatek/mtk_disp_gamma.c   | 97
> > > ++++++++++++++++-
> > > --
> > > --
> > >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c     |  5 +-
> > >  drivers/gpu/drm/mediatek/mtk_drm_crtc.h     |  1 -
> > >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c |  1 +
> > >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |  1 +
> > >  drivers/gpu/drm/mediatek/mtk_drm_drv.c      |  2 +
> > >  8 files changed, 83 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> > > b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> > > index 0f9d7efb61d7..f46d4ab73d6a 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> > > @@ -66,7 +66,7 @@ void mtk_aal_gamma_set(struct device *dev,
> > > struct
> > > drm_crtc_state *state)
> > >  	struct mtk_disp_aal *aal = dev_get_drvdata(dev);
> > >  
> > >  	if (aal->data && aal->data->has_gamma)
> > > -		mtk_gamma_set_common(aal->regs, state, false);
> > > +		mtk_gamma_set_common(aal->regs, state);
> > >  }
> > >  
> > >  void mtk_aal_start(struct device *dev)
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > > b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > > index 33e61a136bbc..b662bf8b1c9d 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > > +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > > @@ -51,8 +51,9 @@ void mtk_gamma_clk_disable(struct device *dev);
> > >  void mtk_gamma_config(struct device *dev, unsigned int w,
> > >  		      unsigned int h, unsigned int vrefresh,
> > >  		      unsigned int bpc, struct cmdq_pkt *cmdq_pkt);
> > > +unsigned int mtk_gamma_size(struct device *dev);
> > >  void mtk_gamma_set(struct device *dev, struct drm_crtc_state
> > > *state);
> > > -void mtk_gamma_set_common(void __iomem *regs, struct
> > > drm_crtc_state
> > > *state, bool lut_diff);
> > > +void mtk_gamma_set_common(void __iomem *regs, struct
> > > drm_crtc_state
> > > *state);
> > >  void mtk_gamma_start(struct device *dev);
> > >  void mtk_gamma_stop(struct device *dev);
> > >  
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > > b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > > index bbd558a036ec..a842e5e1962e 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > > @@ -18,18 +18,26 @@
> > >  #define DISP_GAMMA_EN				0x0000
> > >  #define GAMMA_EN					BIT(0)
> > >  #define DISP_GAMMA_CFG				0x0020
> > > +#define RELAY_MODE					BIT(0)
> > >  #define GAMMA_LUT_EN					BIT(1)
> > >  #define GAMMA_DITHERING					BIT(2)
> > >  #define DISP_GAMMA_SIZE				0x0030
> > > +#define DISP_GAMMA_BANK				0x0100
> > >  #define DISP_GAMMA_LUT				0x0700
> > > -
> > > +#define DISP_GAMMA_LUT1				0x0b00
> > >  #define LUT_10BIT_MASK				0x03ff
> > > -
> > > +#define TABLE_9BIT_SIZE				512
> > > +#define LUT_12BIT_MASK				0x0fff
> > > +#define TABLE_10BIT_SIZE			1024
> > > +#define BANK_SIZE				256
> > >  struct mtk_disp_gamma_data {
> > >  	bool has_dither;
> > >  	bool lut_diff;
> > > +	unsigned int lut_size;
> > >  };
> > >  
> > > +static unsigned int now_lut_size;
> > > +static bool now_lut_diff;
> > 
> > Don't use global variable.
> > 
> > >  /*
> > >   * struct mtk_disp_gamma - DISP_GAMMA driver structure
> > >   */
> > > @@ -54,40 +62,73 @@ void mtk_gamma_clk_disable(struct device
> > > *dev)
> > >  	clk_disable_unprepare(gamma->clk);
> > >  }
> > >  
> > > -void mtk_gamma_set_common(void __iomem *regs, struct
> > > drm_crtc_state
> > > *state, bool lut_diff)
> > > +void mtk_gamma_set_common(void __iomem *regs, struct
> > > drm_crtc_state
> > > *state)
> > >  {
> > > -	unsigned int i, reg;
> > > -	struct drm_color_lut *lut;
> > > +	unsigned int i, reg, idx;
> > >  	void __iomem *lut_base;
> > > -	u32 word;
> > > -	u32 diff[3] = {0};
> > > +	void __iomem *lut1_base;
> > > +	u32 word, word1;
> > >  
> > >  	if (state->gamma_lut) {
> > > +		u32 table_size;
> > > +		u32 mask;
> > > +		struct drm_color_lut color, even, odd;
> > > +		struct drm_color_lut *lut = (struct drm_color_lut
> > > *)state->gamma_lut;
> > > +		bool lut_12bit = (now_lut_size == TABLE_10BIT_SIZE);
> > > +
> > >  		reg = readl(regs + DISP_GAMMA_CFG);
> > > +		reg = reg & ~RELAY_MODE;
> > 
> > Why do you modify this for other SoC?
> > 
> > >  		reg = reg | GAMMA_LUT_EN;
> > >  		writel(reg, regs + DISP_GAMMA_CFG);
> > >  		lut_base = regs + DISP_GAMMA_LUT;
> > > -		lut = (struct drm_color_lut *)state->gamma_lut->data;
> > > -		for (i = 0; i < MTK_LUT_SIZE; i++) {
> > > +		lut1_base = regs + DISP_GAMMA_LUT1;
> > > +		if (lut_12bit) {
> > > +			table_size = TABLE_10BIT_SIZE;
> > > +			mask = LUT_12BIT_MASK;
> > 
> > lut_bits and lut_size are independent factor, so use two variable
> > in
> > private data for each. And I like to use digital value instead of a
> > symbol.
> > 
> > lut_bits = 10 or 12;
> > lut_size = 512 or 1024;
> > mask = GENMASK(lut_bits - 1, 0);
> > 
> > > +		} else {
> > > +			table_size = TABLE_9BIT_SIZE;
> > > +			mask = LUT_10BIT_MASK;
> > > +		}
> > >  
> > > -			if (!lut_diff || (i % 2 == 0)) {
> > > -				word = (((lut[i].red >> 6) &
> > > LUT_10BIT_MASK) << 20) +
> > > -					(((lut[i].green >> 6) &
> > > LUT_10BIT_MASK) << 10) +
> > > -					((lut[i].blue >> 6) &
> > > LUT_10BIT_MASK);
> > > +		for (i = 0; i < table_size; i++) {
> > > +			if (!(i % BANK_SIZE) && lut_12bit)
> > > +				writel((i / BANK_SIZE), regs +
> > > DISP_GAMMA_BANK);
> > 
> > Group register writing together, so move this to bottom of this
> > for-
> > loop.
> > 
> > > +
> > > +			color.red = (lut[i].red >> 6) & mask;
> > > +			color.green = (lut[i].green >> 6) & mask;
> > > +			color.blue = (lut[i].blue >> 6) & mask;
> > 
> > Why shift 6 bits for lut 12 bits?
> > 
> > > +			if ((i % 2) && now_lut_diff) {
> > 
> > In original code, !lut_diff is first, so I would like you to keep
> > this
> > order.
> > 
> > > +				odd = color;
> > > +				word = (lut_12bit) ? (((odd.green -
> > > even.green) << 12) +
> > > +						      (odd.red -
> > > even.red))
> > > +						   : (((odd.red -
> > > even.red) << 20) +
> > > +						      ((odd.green -
> > > even.green) << 10) +
> > > +						      (odd.blue -
> > > even.blue));
> > > +				word1 = (odd.blue - even.blue);
> > 
> > I think it's not necessary to create odd and even variable. 
> > 
> > Regards,
> > CK
> > 
> > >  			} else {
> > > -				diff[0] = (lut[i].red >> 6) - (lut[i -
> > > 1].red >> 6);
> > > -				diff[1] = (lut[i].green >> 6) - (lut[i
> > > - 1].green >> 6);
> > > -				diff[2] = (lut[i].blue >> 6) - (lut[i -
> > > 1].blue >> 6);
> > > -
> > > -				word = ((diff[0] & LUT_10BIT_MASK) <<
> > > 20) +
> > > -					((diff[1] & LUT_10BIT_MASK) <<
> > > 10) +
> > > -					(diff[2] & LUT_10BIT_MASK);
> > > +				even = color;
> > > +				word =  (lut_12bit) ? ((even.green <<
> > > 12) + even.red)
> > > +						    : ((even.red << 20)
> > > +
> > > +						       (even.green <<
> > > 10) + even.blue);
> > > +				word1 = even.blue;
> > >  			}
> > > -			writel(word, (lut_base + i * 4));
> > > +			idx = (lut_12bit) ? (i % BANK_SIZE) : i;
> > > +			writel(word, (lut_base + idx * 4));
> > > +			if (lut_12bit)
> > > +				writel(word1, (lut1_base + idx * 4));
> > >  		}
> > >  	}
> > >  }
> > >  
> > > +unsigned int mtk_gamma_size(struct device *dev)
> > > +{
> > > +	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> > > +
> > > +	if (gamma->data)
> > > +		return gamma->data->lut_size;
> > > +	else
> > > +		return 0;
> > > +}
> > >  void mtk_gamma_set(struct device *dev, struct drm_crtc_state
> > > *state)
> > >  {
> > >  	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> > > @@ -95,8 +136,7 @@ void mtk_gamma_set(struct device *dev, struct
> > > drm_crtc_state *state)
> > >  
> > >  	if (gamma->data)
> > >  		lut_diff = gamma->data->lut_diff;
> > > -
> > > -	mtk_gamma_set_common(gamma->regs, state, lut_diff);
> > > +	mtk_gamma_set_common(gamma->regs, state);
> > >  }
> > >  
> > >  void mtk_gamma_config(struct device *dev, unsigned int w,
> > > @@ -178,6 +218,8 @@ static int mtk_disp_gamma_probe(struct
> > > platform_device *pdev)
> > >  	ret = component_add(dev, &mtk_disp_gamma_component_ops);
> > >  	if (ret)
> > >  		dev_err(dev, "Failed to add component: %d\n", ret);
> > > +	now_lut_size = priv->data->lut_size;
> > > +	now_lut_diff = priv->data->lut_diff;
> > >  
> > >  	return ret;
> > >  }
> > > @@ -191,10 +233,17 @@ static int mtk_disp_gamma_remove(struct
> > > platform_device *pdev)
> > >  
> > >  static const struct mtk_disp_gamma_data mt8173_gamma_driver_data
> > > =
> > > {
> > >  	.has_dither = true,
> > > +	.lut_size = 512,
> > >  };
> > >  
> > >  static const struct mtk_disp_gamma_data mt8183_gamma_driver_data
> > > =
> > > {
> > >  	.lut_diff = true,
> > > +	.lut_size = 512,
> > > +};
> > > +
> > > +static const struct mtk_disp_gamma_data mt8195_gamma_driver_data
> > > =
> > > {
> > > +	.lut_diff = true,
> > > +	.lut_size = 1024,
> > >  };
> > >  
> > >  static const struct of_device_id
> > > mtk_disp_gamma_driver_dt_match[]
> > > =
> > > {
> > > @@ -202,6 +251,8 @@ static const struct of_device_id
> > > mtk_disp_gamma_driver_dt_match[] = {
> > >  	  .data = &mt8173_gamma_driver_data},
> > >  	{ .compatible = "mediatek,mt8183-disp-gamma",
> > >  	  .data = &mt8183_gamma_driver_data},
> > > +	{ .compatible = "mediatek,mt8195-disp-gamma",
> > > +	  .data = &mt8195_gamma_driver_data},
> > >  	{},
> > >  };
> > >  MODULE_DEVICE_TABLE(of, mtk_disp_gamma_driver_dt_match);
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > index 42cc7052b050..2a6513259562 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > @@ -930,9 +930,8 @@ int mtk_drm_crtc_create(struct drm_device
> > > *drm_dev,
> > >  		mtk_crtc->ddp_comp[i] = comp;
> > >  
> > >  		if (comp->funcs) {
> > > -			if (comp->funcs->gamma_set)
> > > -				gamma_lut_size = MTK_LUT_SIZE;
> > > -
> > > +			if (comp->funcs->gamma_set && comp->funcs-
> > > > gamma_size)
> > > 
> > > +				gamma_lut_size = comp->funcs-
> > > > gamma_size(comp->dev);
> > > 
> > >  			if (comp->funcs->ctm_set)
> > >  				has_ctm = true;
> > >  		}
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> > > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> > > index cb9a36c48d4f..1799853ef89a 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> > > @@ -10,7 +10,6 @@
> > >  #include "mtk_drm_ddp_comp.h"
> > >  #include "mtk_drm_plane.h"
> > >  
> > > -#define MTK_LUT_SIZE	512
> > >  #define MTK_MAX_BPC	10
> > >  #define MTK_MIN_BPC	3
> > >  
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > > index 2d72cc5ddaba..4c6538a17b88 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > > @@ -323,6 +323,7 @@ static const struct mtk_ddp_comp_funcs
> > > ddp_gamma
> > > = {
> > >  	.clk_enable = mtk_gamma_clk_enable,
> > >  	.clk_disable = mtk_gamma_clk_disable,
> > >  	.gamma_set = mtk_gamma_set,
> > > +	.gamma_size = mtk_gamma_size,
> > >  	.config = mtk_gamma_config,
> > >  	.start = mtk_gamma_start,
> > >  	.stop = mtk_gamma_stop,
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > > index 2d0052c23dcb..bf0cf7f86010 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > > @@ -59,6 +59,7 @@ struct mtk_ddp_comp_funcs {
> > >  	void (*disable_vblank)(struct device *dev);
> > >  	unsigned int (*supported_rotations)(struct device *dev);
> > >  	unsigned int (*layer_nr)(struct device *dev);
> > > +	unsigned int (*gamma_size)(struct device *dev);
> > >  	int (*layer_check)(struct device *dev,
> > >  			   unsigned int idx,
> > >  			   struct mtk_plane_state *state);
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > > index 0e4c77724b05..473766be56e1 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > > @@ -567,6 +567,8 @@ static const struct of_device_id
> > > mtk_ddp_comp_dt_ids[] = {
> > >  	  .data = (void *)MTK_DISP_GAMMA, },
> > >  	{ .compatible = "mediatek,mt8183-disp-gamma",
> > >  	  .data = (void *)MTK_DISP_GAMMA, },
> > > +	{ .compatible = "mediatek,mt8195-disp-gamma",
> > > +	  .data = (void *)MTK_DISP_GAMMA, },
> > >  	{ .compatible = "mediatek,mt8195-disp-merge",
> > >  	  .data = (void *)MTK_DISP_MERGE },
> > >  	{ .compatible = "mediatek,mt2701-disp-mutex",
> > 
> > 
> 
> 1. 
> 
> > +static unsigned int now_lut_size;
> > +static bool now_lut_diff;
> 
> Don't use global variable.
> 
> 
> 
> 	This two variables are only used in mtk_gamma_set_common()
> function, which is called by only mtk_aal_gamma_set() and
> mtk_gamma_set(). 
> 
> 	Change definition of mtk_gamma_set_common() to something like
> mtk_gamma_set_common( ..., dev)  
> 			to bring in "lut_size" and "lut_diff" from dev
> private data.
> 
> 2.
> 
> void mtk_gamma_set_common(void __iomem *regs, struct drm_crtc_state
> > *state)
> >  {
> > -     unsigned int i, reg;
> > -     struct drm_color_lut *lut;
> > +     unsigned int i, reg, idx;
> >       void __iomem *lut_base;
> > -     u32 word;
> > -     u32 diff[3] = {0};
> > +     void __iomem *lut1_base;
> > +     u32 word, word1;
> >  
> >       if (state->gamma_lut) {
> > +             u32 table_size;
> > +             u32 mask;
> > +             struct drm_color_lut color, even, odd;
> > +             struct drm_color_lut *lut = (struct drm_color_lut
> > *)state->gamma_lut;
> > +             bool lut_12bit = (now_lut_size == TABLE_10BIT_SIZE);
> > +
> >               reg = readl(regs + DISP_GAMMA_CFG);
> > +             reg = reg & ~RELAY_MODE;
> 
> Why do you modify this for other SoC?
> 
> 
> 	Originally, gamma config initialization is in
> mtk_gamma_config() , this function would write GAMMA_DITHERING into
> gamma_CFG if .has_dither flag is true.
> 	
> 	This writing actually shut down the relay mode of gamma, thus,
> in the original version only need to set GAMMA_LUT_EN to make use of
> gamma.
> 	
> 	However, now .has_dither is false, which means
> mtk_gamma_config() won't modify gamma_CFG anymore.
> 	
> 	Therefore, I added above code to manually shut down relay mode,
> to ensure it running properly.
> 
> 
> 	 
> 
> 3.
> 
>       reg = reg | GAMMA_LUT_EN;
> >               writel(reg, regs + DISP_GAMMA_CFG);
> >               lut_base = regs + DISP_GAMMA_LUT;
> > -             lut = (struct drm_color_lut *)state->gamma_lut->data;
> > -             for (i = 0; i < MTK_LUT_SIZE; i++) {
> > +             lut1_base = regs + DISP_GAMMA_LUT1;
> > +             if (lut_12bit) {
> > +                     table_size = TABLE_10BIT_SIZE;
> > +                     mask = LUT_12BIT_MASK;
> 
> lut_bits and lut_size are independent factor, so use two variable in
> private data for each. And I like to use digital value instead of a
> symbol.
> 
> lut_bits = 10 or 12;
> lut_size = 512 or 1024;
> mask = GENMASK(lut_bits - 1, 0);
> 
> > +             }
> 
> 
> 	OK, I will fix it.
> 
> 
> 
> 4.
> 
> > +             for (i = 0; i < table_size; i++) {
> > +                     if (!(i % BANK_SIZE) && lut_12bit)
> > +                             writel((i / BANK_SIZE), regs +
> > DISP_GAMMA_BANK);
> 
> Group register writing together, so move this to bottom of this for-
> loop.
> 
> 
> 	OK, I will fix it.
> 
> 
> 
> 5.
> 
> > +                     color.red = (lut[i].red >> 6) & mask;
> > +                     color.green = (lut[i].green >> 6) & mask;
> > +                     color.blue = (lut[i].blue >> 6) & mask;
> 
> Why shift 6 bits for lut 12 bits?
> 
> 
> 	After testing, I find that right-shift 6bits and mask the
> Least-Significant(LS) 12bits bring the correct results.
> 
> 	It actually make more sense to right-shift 4bits to gain the
> 12bits, however, when I did this,  the results failed.
> 
> 	I wonder if this is the test data problem, since the original
> test data is for mt8183 10bit lut.
> 	
> 	I will contact with designer and try to figure it out.  	
Hello CK, 

I have asked the designers about the shift-bits problem and get the
answer.

The bits should be shifted 4 bits instead of 6 bits, the reason why I
need to shift 6 bits to get the correct results is that the test data
is for mt8183 but not for mt8195.

I will revise the code about this part and send v2 later today.

Thanks for your advices again!

Best Regards,
Zheng-Yan Chen  
> 
> 
> 
> 6.
> > +                     if ((i % 2) && now_lut_diff) {
> 
> In original code, !lut_diff is first, so I would like you to keep
> this
> order.
> 
> 
> 	OK, I will fix it.
> 
> 
> 
> 7.
>  +                             odd = color;
> > +                             word = (lut_12bit) ? (((odd.green -
> > even.green) << 12) +
> > +                                                   (odd.red -
> > even.red))
> > +                                                : (((odd.red -
> > even.red) << 20) +
> > +                                                   ((odd.green -
> > even.green) << 10) +
> > +                                                   (odd.blue -
> > even.blue));
> > +                             word1 = (odd.blue - even.blue);
> 
> I think it's not necessary to create odd and even variable. 
> 
> Regards,
> CK
> 
> 
> 	OK, I will fix it.

[PATCH 3/3] arm64: dts: Modify gamma compatible for mt8195

[zheng-yan.chen]

Modify gamma compatible for mt8195.

Signed-off-by: zheng-yan.chen <zheng-yan.chen@mediatek.com>

---
 arch/arm64/boot/dts/mediatek/mt8195.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8195.dtsi b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
index a50ebb5d145f..8504d01b103a 100644
--- a/arch/arm64/boot/dts/mediatek/mt8195.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
@@ -2021,8 +2021,8 @@
 			mediatek,gce-client-reg = <&gce0 SUBSYS_1c00XXXX 0x5000 0x1000>;
 		};
 
-		gamma0: gamma@1c006000 {
-			compatible = "mediatek,mt8195-disp-gamma", "mediatek,mt8183-disp-gamma";
+		gamma0: disp_gamma@1c006000 {
+			compatible = "mediatek,mt8195-disp-gamma";
 			reg = <0 0x1c006000 0 0x1000>;
 			interrupts = <GIC_SPI 642 IRQ_TYPE_LEVEL_HIGH 0>;
 			power-domains = <&spm MT8195_POWER_DOMAIN_VDOSYS0>;
-- 
2.18.0

Re: [PATCH 3/3] arm64: dts: Modify gamma compatible for mt8195

[Krzysztof Kozlowski]

On 22/08/2022 12:19, zheng-yan.chen wrote:
> Modify gamma compatible for mt8195.
> 
> Signed-off-by: zheng-yan.chen <zheng-yan.chen@mediatek.com>
> 
> ---
>  arch/arm64/boot/dts/mediatek/mt8195.dtsi | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8195.dtsi b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
> index a50ebb5d145f..8504d01b103a 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8195.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
> @@ -2021,8 +2021,8 @@
>  			mediatek,gce-client-reg = <&gce0 SUBSYS_1c00XXXX 0x5000 0x1000>;
>  		};
>  
> -		gamma0: gamma@1c006000 {
> -			compatible = "mediatek,mt8195-disp-gamma", "mediatek,mt8183-disp-gamma";
> +		gamma0: disp_gamma@1c006000 {

No, really, no.

Not explained in commit msg, violates naming convention, violates coding
style, not related to the patch at all.

Best regards,
Krzysztof

Re: [PATCH 3/3] arm64: dts: Modify gamma compatible for mt8195

[zheng-yan.chen]

On Tue, 2022-08-23 at 11:40 +0300, Krzysztof Kozlowski wrote:
> On 22/08/2022 12:19, zheng-yan.chen wrote:
> > Modify gamma compatible for mt8195.
> > 
> > Signed-off-by: zheng-yan.chen <zheng-yan.chen@mediatek.com>
> > 
> > ---
> >  arch/arm64/boot/dts/mediatek/mt8195.dtsi | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8195.dtsi
> > b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
> > index a50ebb5d145f..8504d01b103a 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8195.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
> > @@ -2021,8 +2021,8 @@
> >  			mediatek,gce-client-reg = <&gce0
> > SUBSYS_1c00XXXX 0x5000 0x1000>;
> >  		};
> >  
> > -		gamma0: gamma@1c006000 {
> > -			compatible = "mediatek,mt8195-disp-gamma",
> > "mediatek,mt8183-disp-gamma";
> > +		gamma0: disp_gamma@1c006000 {
> 
> No, really, no.
> 
> Not explained in commit msg, violates naming convention, violates
> coding
> style, not related to the patch at all.
> 
> Best regards,
> Krzysztof

Sorry about that, I will change this "disp_gamma" back to "gamma".


Best Regards,
Zheng-Yan Chen