[PATCH RESEND v6 00/11] MediaTek DDP GAMMA - 12-bit LUT support

[PATCH RESEND v6 00/11] MediaTek DDP GAMMA - 12-bit LUT support

[AngeloGioacchino Del Regno]

Changes in RESEND-v6:
 - None.

Changes in v6:
 - Fixed smatch warning in patch 11/11, ref.:
   https://lore.kernel.org/all/202306101458.lRXHEE0Z-lkp@intel.com/

Changes in v5:
 - Removed incorrect comment on default LUT size and bits
 - Removed useless check for num_lut_banks
 - Added comment about CMDQ implementation on patch 5
 - Evaluated passing lut size/bits from AAL, idea discarded as
   the implementation would be rather tricky while bringing no
   benefits.

Changes in v4:
 - Fixed assignment typo appeared in v3

Changes in v3:
 - Fixed issues due to variables renaming during cleanup (oops)
 - This is actually the right series, since v2 was taken from the
   wrong kernel tree.... :-)

Changes in v2:
 - Added explicit inclusion of linux/bitfield.h in patch [06/11]

This series adds support for GAMMA IP requiring and/or supporting
a 12-bits LUT using a slightly different register layout and programming
sequence for multiple LUT banks: this IP version is currently found
on a number of SoCs, not only including the Chromebook/IoT oriented
Kompanio 1200/1380 MT8195/MT8195T, but also Smartphone chips such as
the Dimensity 9200 (MT6985) and others.

This series was tested on MT8195, MT8192, MT8173, MT6795:
 * MT6795, MT8192, MT8173: No regression, works fine.
 * MT8195: Color correction is finally working!

AngeloGioacchino Del Regno (10):
  drm/mediatek: gamma: Reduce indentation in mtk_gamma_set_common()
  drm/mediatek: gamma: Support SoC specific LUT size
  drm/mediatek: gamma: Improve and simplify HW LUT calculation
  drm/mediatek: gamma: Enable the Gamma LUT table only after programming
  drm/mediatek: gamma: Use bitfield macros
  drm/mediatek: gamma: Support specifying number of bits per LUT
    component
  drm/mediatek: gamma: Support multi-bank gamma LUT
  drm/mediatek: gamma: Add support for 12-bit LUT and MT8195
  drm/mediatek: gamma: Make sure relay mode is disabled
  drm/mediatek: gamma: Program gamma LUT type for descending or rising

Jason-JH.Lin (1):
  drm/mediatek: gamma: Adjust mtk_drm_gamma_set_common parameters

 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   | 193 ++++++++++++++++----
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c     |   4 +-
 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 |   9 +
 7 files changed, 177 insertions(+), 36 deletions(-)

-- 
2.40.1

[PATCH RESEND v6 01/11] drm/mediatek: gamma: Adjust mtk_drm_gamma_set_common parameters

[AngeloGioacchino Del Regno]

From: "Jason-JH.Lin" <jason-jh.lin@mediatek.com>

Adjust the parameters in mtk_drm_gamma_set_common()
  - add (struct device *dev) to get lut_diff from gamma's driver data
  - remove (bool lut_diff) and use false as default value in the function

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_disp_aal.c   |  2 +-
 drivers/gpu/drm/mediatek/mtk_disp_drv.h   |  2 +-
 drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 15 +++++++++------
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_aal.c b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
index 434e8a9ce8ab..8ddf7a97e583 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
@@ -67,7 +67,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(NULL, 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 2254038519e1..75045932353e 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
@@ -54,7 +54,7 @@ void mtk_gamma_config(struct device *dev, unsigned int w,
 		      unsigned int h, unsigned int vrefresh,
 		      unsigned int bpc, struct cmdq_pkt *cmdq_pkt);
 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(struct device *dev, 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 c844942603f7..99be515a941b 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -55,14 +55,21 @@ 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(struct device *dev, void __iomem *regs, struct drm_crtc_state *state)
 {
+	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
 	unsigned int i, reg;
 	struct drm_color_lut *lut;
 	void __iomem *lut_base;
+	bool lut_diff;
 	u32 word;
 	u32 diff[3] = {0};
 
+	if (gamma && gamma->data)
+		lut_diff = gamma->data->lut_diff;
+	else
+		lut_diff = false;
+
 	if (state->gamma_lut) {
 		reg = readl(regs + DISP_GAMMA_CFG);
 		reg = reg | GAMMA_LUT_EN;
@@ -92,12 +99,8 @@ void mtk_gamma_set_common(void __iomem *regs, struct drm_crtc_state *state, bool
 void mtk_gamma_set(struct device *dev, struct drm_crtc_state *state)
 {
 	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
-	bool lut_diff = false;
-
-	if (gamma->data)
-		lut_diff = gamma->data->lut_diff;
 
-	mtk_gamma_set_common(gamma->regs, state, lut_diff);
+	mtk_gamma_set_common(dev, gamma->regs, state);
 }
 
 void mtk_gamma_config(struct device *dev, unsigned int w,
-- 
2.40.1

Re: [PATCH RESEND v6 01/11] drm/mediatek: gamma: Adjust mtk_drm_gamma_set_common parameters

[Alexandre Mergnat]

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>

On 27/07/2023 11:46, AngeloGioacchino Del Regno wrote:
> From: "Jason-JH.Lin"<jason-jh.lin@mediatek.com>
> 
> Adjust the parameters in mtk_drm_gamma_set_common()
>    - add (struct device *dev) to get lut_diff from gamma's driver data
>    - remove (bool lut_diff) and use false as default value in the function

-- 
Regards,
Alexandre

[PATCH RESEND v6 02/11] drm/mediatek: gamma: Reduce indentation in mtk_gamma_set_common()

[AngeloGioacchino Del Regno]

Invert the check for state->gamma_lut and move it at the beginning
of the function to reduce indentation: this prepares the code for
keeping readability on later additions.

This commit brings no functional changes.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 45 ++++++++++++-----------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
index 99be515a941b..ce6f2499b891 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -65,34 +65,35 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 	u32 word;
 	u32 diff[3] = {0};
 
+	/* If there's no gamma lut there's nothing to do here. */
+	if (!state->gamma_lut)
+		return;
+
 	if (gamma && gamma->data)
 		lut_diff = gamma->data->lut_diff;
 	else
 		lut_diff = false;
 
-	if (state->gamma_lut) {
-		reg = readl(regs + DISP_GAMMA_CFG);
-		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++) {
-
-			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);
-			} 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);
-			}
-			writel(word, (lut_base + i * 4));
+	reg = readl(regs + DISP_GAMMA_CFG);
+	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++) {
+		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);
+		} 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);
 		}
+		writel(word, (lut_base + i * 4));
 	}
 }
 
-- 
2.40.1

Re: [PATCH RESEND v6 02/11] drm/mediatek: gamma: Reduce indentation in mtk_gamma_set_common()

[Alexandre Mergnat]

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>

On 27/07/2023 11:46, AngeloGioacchino Del Regno wrote:
> Invert the check for state->gamma_lut and move it at the beginning
> of the function to reduce indentation: this prepares the code for
> keeping readability on later additions.
> 
> This commit brings no functional changes.

-- 
Regards,
Alexandre

[PATCH RESEND v6 03/11] drm/mediatek: gamma: Support SoC specific LUT size

[AngeloGioacchino Del Regno]

Newer SoCs support a bigger Gamma LUT table: wire up a callback
to retrieve the correct LUT size for each different Gamma IP.

Co-developed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
[Angelo: Rewritten commit message/description + porting]
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_disp_drv.h     |  1 +
 drivers/gpu/drm/mediatek/mtk_disp_gamma.c   | 25 ++++++++++++++++++---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c     |  4 ++--
 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 |  9 ++++++++
 6 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
index 75045932353e..e554b19f4830 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
@@ -53,6 +53,7 @@ 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_get_lut_size(struct device *dev);
 void mtk_gamma_set(struct device *dev, struct drm_crtc_state *state);
 void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crtc_state *state);
 void mtk_gamma_start(struct device *dev);
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
index ce6f2499b891..b25ba209e7a4 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -25,10 +25,12 @@
 #define DISP_GAMMA_LUT				0x0700
 
 #define LUT_10BIT_MASK				0x03ff
+#define LUT_SIZE_DEFAULT			512
 
 struct mtk_disp_gamma_data {
 	bool has_dither;
 	bool lut_diff;
+	u16 lut_size;
 };
 
 /*
@@ -55,6 +57,17 @@ void mtk_gamma_clk_disable(struct device *dev)
 	clk_disable_unprepare(gamma->clk);
 }
 
+unsigned int mtk_gamma_get_lut_size(struct device *dev)
+{
+	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
+	unsigned int lut_size = LUT_SIZE_DEFAULT;
+
+	if (gamma && gamma->data)
+		lut_size = gamma->data->lut_size;
+
+	return lut_size;
+}
+
 void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crtc_state *state)
 {
 	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
@@ -62,6 +75,7 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 	struct drm_color_lut *lut;
 	void __iomem *lut_base;
 	bool lut_diff;
+	u16 lut_size;
 	u32 word;
 	u32 diff[3] = {0};
 
@@ -69,17 +83,20 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 	if (!state->gamma_lut)
 		return;
 
-	if (gamma && gamma->data)
+	if (gamma && gamma->data) {
 		lut_diff = gamma->data->lut_diff;
-	else
+		lut_size = gamma->data->lut_size;
+	} else {
 		lut_diff = false;
+		lut_size = LUT_SIZE_DEFAULT;
+	}
 
 	reg = readl(regs + DISP_GAMMA_CFG);
 	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++) {
+	for (i = 0; i < lut_size; i++) {
 		if (!lut_diff || (i % 2 == 0)) {
 			word = (((lut[i].red >> 6) & LUT_10BIT_MASK) << 20) +
 				(((lut[i].green >> 6) & LUT_10BIT_MASK) << 10) +
@@ -196,10 +213,12 @@ 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 of_device_id 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 d40142842f85..0df62b076f49 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -958,8 +958,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_get_lut_size)
+				gamma_lut_size = mtk_ddp_gamma_get_lut_size(comp);
 
 			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 3e9046993d09..b2e50292e57d 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 f114da4d36a9..c77af2e4000f 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
@@ -322,6 +322,7 @@ static const struct mtk_ddp_comp_funcs ddp_dsi = {
 static const struct mtk_ddp_comp_funcs ddp_gamma = {
 	.clk_enable = mtk_gamma_clk_enable,
 	.clk_disable = mtk_gamma_clk_disable,
+	.gamma_get_lut_size = mtk_gamma_get_lut_size,
 	.gamma_set = mtk_gamma_set,
 	.config = mtk_gamma_config,
 	.start = mtk_gamma_start,
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
index febcaeef16a1..c1355960e195 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
@@ -67,6 +67,7 @@ struct mtk_ddp_comp_funcs {
 	void (*layer_config)(struct device *dev, unsigned int idx,
 			     struct mtk_plane_state *state,
 			     struct cmdq_pkt *cmdq_pkt);
+	unsigned int (*gamma_get_lut_size)(struct device *dev);
 	void (*gamma_set)(struct device *dev,
 			  struct drm_crtc_state *state);
 	void (*bgclr_in_on)(struct device *dev);
@@ -186,6 +187,14 @@ static inline void mtk_ddp_comp_layer_config(struct mtk_ddp_comp *comp,
 		comp->funcs->layer_config(comp->dev, idx, state, cmdq_pkt);
 }
 
+static inline unsigned int mtk_ddp_gamma_get_lut_size(struct mtk_ddp_comp *comp)
+{
+	if (comp->funcs && comp->funcs->gamma_get_lut_size)
+		return comp->funcs->gamma_get_lut_size(comp->dev);
+
+	return 0;
+}
+
 static inline void mtk_ddp_gamma_set(struct mtk_ddp_comp *comp,
 				     struct drm_crtc_state *state)
 {
-- 
2.40.1

Re: [PATCH RESEND v6 03/11] drm/mediatek: gamma: Support SoC specific LUT size

[Alexandre Mergnat]

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>

On 27/07/2023 11:46, AngeloGioacchino Del Regno wrote:
> Newer SoCs support a bigger Gamma LUT table: wire up a callback
> to retrieve the correct LUT size for each different Gamma IP.

-- 
Regards,
Alexandre

Re: [PATCH RESEND v6 03/11] drm/mediatek: gamma: Support SoC specific LUT size

[CK Hu (胡俊光)]

Hi, Angelo:

On Thu, 2023-07-27 at 11:46 +0200, AngeloGioacchino Del Regno wrote:
> Newer SoCs support a bigger Gamma LUT table: wire up a callback
> to retrieve the correct LUT size for each different Gamma IP.
> 
> Co-developed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> [Angelo: Rewritten commit message/description + porting]
> Signed-off-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_drv.h     |  1 +
>  drivers/gpu/drm/mediatek/mtk_disp_gamma.c   | 25 ++++++++++++++++++-
> --
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c     |  4 ++--
>  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 |  9 ++++++++
>  6 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index 75045932353e..e554b19f4830 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -53,6 +53,7 @@ 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_get_lut_size(struct device *dev);
>  void mtk_gamma_set(struct device *dev, struct drm_crtc_state
> *state);
>  void mtk_gamma_set_common(struct device *dev, void __iomem *regs,
> struct drm_crtc_state *state);
>  void mtk_gamma_start(struct device *dev);
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> index ce6f2499b891..b25ba209e7a4 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> @@ -25,10 +25,12 @@
>  #define DISP_GAMMA_LUT				0x0700
>  
>  #define LUT_10BIT_MASK				0x03ff
> +#define LUT_SIZE_DEFAULT			512
>  
>  struct mtk_disp_gamma_data {
>  	bool has_dither;
>  	bool lut_diff;
> +	u16 lut_size;
>  };
>  
>  /*
> @@ -55,6 +57,17 @@ void mtk_gamma_clk_disable(struct device *dev)
>  	clk_disable_unprepare(gamma->clk);
>  }
>  
> +unsigned int mtk_gamma_get_lut_size(struct device *dev)
> +{
> +	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> +	unsigned int lut_size = LUT_SIZE_DEFAULT;
> +
> +	if (gamma && gamma->data)
> +		lut_size = gamma->data->lut_size;
> +
> +	return lut_size;
> +}
> +
>  void mtk_gamma_set_common(struct device *dev, void __iomem *regs,
> struct drm_crtc_state *state)
>  {
>  	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> @@ -62,6 +75,7 @@ void mtk_gamma_set_common(struct device *dev, void
> __iomem *regs, struct drm_crt
>  	struct drm_color_lut *lut;
>  	void __iomem *lut_base;
>  	bool lut_diff;
> +	u16 lut_size;
>  	u32 word;
>  	u32 diff[3] = {0};
>  
> @@ -69,17 +83,20 @@ void mtk_gamma_set_common(struct device *dev,
> void __iomem *regs, struct drm_crt
>  	if (!state->gamma_lut)
>  		return;
>  
> -	if (gamma && gamma->data)
> +	if (gamma && gamma->data) {
>  		lut_diff = gamma->data->lut_diff;
> -	else
> +		lut_size = gamma->data->lut_size;
> +	} else {
>  		lut_diff = false;
> +		lut_size = LUT_SIZE_DEFAULT;
> +	}
>  
>  	reg = readl(regs + DISP_GAMMA_CFG);
>  	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++) {
> +	for (i = 0; i < lut_size; i++) {
>  		if (!lut_diff || (i % 2 == 0)) {
>  			word = (((lut[i].red >> 6) & LUT_10BIT_MASK) <<
> 20) +
>  				(((lut[i].green >> 6) & LUT_10BIT_MASK)
> << 10) +
> @@ -196,10 +213,12 @@ 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 of_device_id 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 d40142842f85..0df62b076f49 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -958,8 +958,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_get_lut_size)
> +				gamma_lut_size =
> mtk_ddp_gamma_get_lut_size(comp);

In this patch, for AAL, the gamma_lut_size is not defined.

Regards,
CK

>  
>  			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 3e9046993d09..b2e50292e57d 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 f114da4d36a9..c77af2e4000f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -322,6 +322,7 @@ static const struct mtk_ddp_comp_funcs ddp_dsi =
> {
>  static const struct mtk_ddp_comp_funcs ddp_gamma = {
>  	.clk_enable = mtk_gamma_clk_enable,
>  	.clk_disable = mtk_gamma_clk_disable,
> +	.gamma_get_lut_size = mtk_gamma_get_lut_size,
>  	.gamma_set = mtk_gamma_set,
>  	.config = mtk_gamma_config,
>  	.start = mtk_gamma_start,
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> index febcaeef16a1..c1355960e195 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> @@ -67,6 +67,7 @@ struct mtk_ddp_comp_funcs {
>  	void (*layer_config)(struct device *dev, unsigned int idx,
>  			     struct mtk_plane_state *state,
>  			     struct cmdq_pkt *cmdq_pkt);
> +	unsigned int (*gamma_get_lut_size)(struct device *dev);
>  	void (*gamma_set)(struct device *dev,
>  			  struct drm_crtc_state *state);
>  	void (*bgclr_in_on)(struct device *dev);
> @@ -186,6 +187,14 @@ static inline void
> mtk_ddp_comp_layer_config(struct mtk_ddp_comp *comp,
>  		comp->funcs->layer_config(comp->dev, idx, state,
> cmdq_pkt);
>  }
>  
> +static inline unsigned int mtk_ddp_gamma_get_lut_size(struct
> mtk_ddp_comp *comp)
> +{
> +	if (comp->funcs && comp->funcs->gamma_get_lut_size)
> +		return comp->funcs->gamma_get_lut_size(comp->dev);
> +
> +	return 0;
> +}
> +
>  static inline void mtk_ddp_gamma_set(struct mtk_ddp_comp *comp,
>  				     struct drm_crtc_state *state)
>  {

Re: [PATCH RESEND v6 03/11] drm/mediatek: gamma: Support SoC specific LUT size

[AngeloGioacchino Del Regno]

Il 31/07/23 09:49, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
> 
> On Thu, 2023-07-27 at 11:46 +0200, AngeloGioacchino Del Regno wrote:
>> Newer SoCs support a bigger Gamma LUT table: wire up a callback
>> to retrieve the correct LUT size for each different Gamma IP.
>>
>> Co-developed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
>> [Angelo: Rewritten commit message/description + porting]
>> Signed-off-by: AngeloGioacchino Del Regno <
>> angelogioacchino.delregno@collabora.com>
>> Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_disp_drv.h     |  1 +
>>   drivers/gpu/drm/mediatek/mtk_disp_gamma.c   | 25 ++++++++++++++++++-
>> --
>>   drivers/gpu/drm/mediatek/mtk_drm_crtc.c     |  4 ++--
>>   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 |  9 ++++++++
>>   6 files changed, 35 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
>> b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
>> index 75045932353e..e554b19f4830 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
>> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
>> @@ -53,6 +53,7 @@ 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_get_lut_size(struct device *dev);
>>   void mtk_gamma_set(struct device *dev, struct drm_crtc_state
>> *state);
>>   void mtk_gamma_set_common(struct device *dev, void __iomem *regs,
>> struct drm_crtc_state *state);
>>   void mtk_gamma_start(struct device *dev);
>> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
>> b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
>> index ce6f2499b891..b25ba209e7a4 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
>> @@ -25,10 +25,12 @@
>>   #define DISP_GAMMA_LUT				0x0700
>>   
>>   #define LUT_10BIT_MASK				0x03ff
>> +#define LUT_SIZE_DEFAULT			512
>>   
>>   struct mtk_disp_gamma_data {
>>   	bool has_dither;
>>   	bool lut_diff;
>> +	u16 lut_size;
>>   };
>>   
>>   /*
>> @@ -55,6 +57,17 @@ void mtk_gamma_clk_disable(struct device *dev)
>>   	clk_disable_unprepare(gamma->clk);
>>   }
>>   
>> +unsigned int mtk_gamma_get_lut_size(struct device *dev)
>> +{
>> +	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
>> +	unsigned int lut_size = LUT_SIZE_DEFAULT;
>> +
>> +	if (gamma && gamma->data)
>> +		lut_size = gamma->data->lut_size;
>> +
>> +	return lut_size;
>> +}
>> +
>>   void mtk_gamma_set_common(struct device *dev, void __iomem *regs,
>> struct drm_crtc_state *state)
>>   {
>>   	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
>> @@ -62,6 +75,7 @@ void mtk_gamma_set_common(struct device *dev, void
>> __iomem *regs, struct drm_crt
>>   	struct drm_color_lut *lut;
>>   	void __iomem *lut_base;
>>   	bool lut_diff;
>> +	u16 lut_size;
>>   	u32 word;
>>   	u32 diff[3] = {0};
>>   
>> @@ -69,17 +83,20 @@ void mtk_gamma_set_common(struct device *dev,
>> void __iomem *regs, struct drm_crt
>>   	if (!state->gamma_lut)
>>   		return;
>>   
>> -	if (gamma && gamma->data)
>> +	if (gamma && gamma->data) {
>>   		lut_diff = gamma->data->lut_diff;
>> -	else
>> +		lut_size = gamma->data->lut_size;
>> +	} else {
>>   		lut_diff = false;
>> +		lut_size = LUT_SIZE_DEFAULT;
>> +	}
>>   
>>   	reg = readl(regs + DISP_GAMMA_CFG);
>>   	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++) {
>> +	for (i = 0; i < lut_size; i++) {
>>   		if (!lut_diff || (i % 2 == 0)) {
>>   			word = (((lut[i].red >> 6) & LUT_10BIT_MASK) <<
>> 20) +
>>   				(((lut[i].green >> 6) & LUT_10BIT_MASK)
>> << 10) +
>> @@ -196,10 +213,12 @@ 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 of_device_id 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 d40142842f85..0df62b076f49 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>> @@ -958,8 +958,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_get_lut_size)
>> +				gamma_lut_size =
>> mtk_ddp_gamma_get_lut_size(comp);
> 
> In this patch, for AAL, the gamma_lut_size is not defined.
> 

It is defined: AAL will call mtk_gamma_set_common(), which will use
the "DEFAULT" (512) LUT size if no platform data was provided and the
platform data can only come from the mtk_gamma driver - so, if the
call to mtk_gamma_set_common() comes from mtk_aal, it will use 512.

P.S.: The call to drm_mode_crtc_set_gamma_size() is performed only if
       gamma_lut_size > 0.

Regards,
Angelo

> Regards,
> CK
> 
>>   
>>   			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 3e9046993d09..b2e50292e57d 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 f114da4d36a9..c77af2e4000f 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
>> @@ -322,6 +322,7 @@ static const struct mtk_ddp_comp_funcs ddp_dsi =
>> {
>>   static const struct mtk_ddp_comp_funcs ddp_gamma = {
>>   	.clk_enable = mtk_gamma_clk_enable,
>>   	.clk_disable = mtk_gamma_clk_disable,
>> +	.gamma_get_lut_size = mtk_gamma_get_lut_size,
>>   	.gamma_set = mtk_gamma_set,
>>   	.config = mtk_gamma_config,
>>   	.start = mtk_gamma_start,
>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
>> b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
>> index febcaeef16a1..c1355960e195 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
>> @@ -67,6 +67,7 @@ struct mtk_ddp_comp_funcs {
>>   	void (*layer_config)(struct device *dev, unsigned int idx,
>>   			     struct mtk_plane_state *state,
>>   			     struct cmdq_pkt *cmdq_pkt);
>> +	unsigned int (*gamma_get_lut_size)(struct device *dev);
>>   	void (*gamma_set)(struct device *dev,
>>   			  struct drm_crtc_state *state);
>>   	void (*bgclr_in_on)(struct device *dev);
>> @@ -186,6 +187,14 @@ static inline void
>> mtk_ddp_comp_layer_config(struct mtk_ddp_comp *comp,
>>   		comp->funcs->layer_config(comp->dev, idx, state,
>> cmdq_pkt);
>>   }
>>   
>> +static inline unsigned int mtk_ddp_gamma_get_lut_size(struct
>> mtk_ddp_comp *comp)
>> +{
>> +	if (comp->funcs && comp->funcs->gamma_get_lut_size)
>> +		return comp->funcs->gamma_get_lut_size(comp->dev);
>> +
>> +	return 0;
>> +}
>> +
>>   static inline void mtk_ddp_gamma_set(struct mtk_ddp_comp *comp,
>>   				     struct drm_crtc_state *state)
>>   {

Re: [PATCH RESEND v6 03/11] drm/mediatek: gamma: Support SoC specific LUT size

[AngeloGioacchino Del Regno]

Il 31/07/23 12:40, AngeloGioacchino Del Regno ha scritto:
> Il 31/07/23 09:49, CK Hu (胡俊光) ha scritto:
>> Hi, Angelo:
>>
>> On Thu, 2023-07-27 at 11:46 +0200, AngeloGioacchino Del Regno wrote:
>>> Newer SoCs support a bigger Gamma LUT table: wire up a callback
>>> to retrieve the correct LUT size for each different Gamma IP.
>>>
>>> Co-developed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
>>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
>>> [Angelo: Rewritten commit message/description + porting]
>>> Signed-off-by: AngeloGioacchino Del Regno <
>>> angelogioacchino.delregno@collabora.com>
>>> Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
>>> ---
>>>   drivers/gpu/drm/mediatek/mtk_disp_drv.h     |  1 +
>>>   drivers/gpu/drm/mediatek/mtk_disp_gamma.c   | 25 ++++++++++++++++++-
>>> -- 
>>>   drivers/gpu/drm/mediatek/mtk_drm_crtc.c     |  4 ++--
>>>   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 |  9 ++++++++
>>>   6 files changed, 35 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
>>> b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
>>> index 75045932353e..e554b19f4830 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
>>> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
>>> @@ -53,6 +53,7 @@ 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_get_lut_size(struct device *dev);
>>>   void mtk_gamma_set(struct device *dev, struct drm_crtc_state
>>> *state);
>>>   void mtk_gamma_set_common(struct device *dev, void __iomem *regs,
>>> struct drm_crtc_state *state);
>>>   void mtk_gamma_start(struct device *dev);
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
>>> b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
>>> index ce6f2499b891..b25ba209e7a4 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
>>> +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
>>> @@ -25,10 +25,12 @@
>>>   #define DISP_GAMMA_LUT                0x0700
>>>   #define LUT_10BIT_MASK                0x03ff
>>> +#define LUT_SIZE_DEFAULT            512
>>>   struct mtk_disp_gamma_data {
>>>       bool has_dither;
>>>       bool lut_diff;
>>> +    u16 lut_size;
>>>   };
>>>   /*
>>> @@ -55,6 +57,17 @@ void mtk_gamma_clk_disable(struct device *dev)
>>>       clk_disable_unprepare(gamma->clk);
>>>   }
>>> +unsigned int mtk_gamma_get_lut_size(struct device *dev)
>>> +{
>>> +    struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
>>> +    unsigned int lut_size = LUT_SIZE_DEFAULT;
>>> +
>>> +    if (gamma && gamma->data)
>>> +        lut_size = gamma->data->lut_size;
>>> +
>>> +    return lut_size;
>>> +}
>>> +
>>>   void mtk_gamma_set_common(struct device *dev, void __iomem *regs,
>>> struct drm_crtc_state *state)
>>>   {
>>>       struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
>>> @@ -62,6 +75,7 @@ void mtk_gamma_set_common(struct device *dev, void
>>> __iomem *regs, struct drm_crt
>>>       struct drm_color_lut *lut;
>>>       void __iomem *lut_base;
>>>       bool lut_diff;
>>> +    u16 lut_size;
>>>       u32 word;
>>>       u32 diff[3] = {0};
>>> @@ -69,17 +83,20 @@ void mtk_gamma_set_common(struct device *dev,
>>> void __iomem *regs, struct drm_crt
>>>       if (!state->gamma_lut)
>>>           return;
>>> -    if (gamma && gamma->data)
>>> +    if (gamma && gamma->data) {
>>>           lut_diff = gamma->data->lut_diff;
>>> -    else
>>> +        lut_size = gamma->data->lut_size;
>>> +    } else {
>>>           lut_diff = false;
>>> +        lut_size = LUT_SIZE_DEFAULT;
>>> +    }
>>>       reg = readl(regs + DISP_GAMMA_CFG);
>>>       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++) {
>>> +    for (i = 0; i < lut_size; i++) {
>>>           if (!lut_diff || (i % 2 == 0)) {
>>>               word = (((lut[i].red >> 6) & LUT_10BIT_MASK) <<
>>> 20) +
>>>                   (((lut[i].green >> 6) & LUT_10BIT_MASK)
>>> << 10) +
>>> @@ -196,10 +213,12 @@ 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 of_device_id 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 d40142842f85..0df62b076f49 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>> @@ -958,8 +958,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_get_lut_size)
>>> +                gamma_lut_size =
>>> mtk_ddp_gamma_get_lut_size(comp);
>>
>> In this patch, for AAL, the gamma_lut_size is not defined.
>>
> 
> It is defined: AAL will call mtk_gamma_set_common(), which will use
> the "DEFAULT" (512) LUT size if no platform data was provided and the
> platform data can only come from the mtk_gamma driver - so, if the
> call to mtk_gamma_set_common() comes from mtk_aal, it will use 512.
> 
> P.S.: The call to drm_mode_crtc_set_gamma_size() is performed only if
>        gamma_lut_size > 0.
> 

Actually, I see what you mean now. Gets fixed in v7. Thanks!

> Regards,
> Angelo
> 
>> Regards,
>> CK
>>
>>>               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 3e9046993d09..b2e50292e57d 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 f114da4d36a9..c77af2e4000f 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
>>> @@ -322,6 +322,7 @@ static const struct mtk_ddp_comp_funcs ddp_dsi =
>>> {
>>>   static const struct mtk_ddp_comp_funcs ddp_gamma = {
>>>       .clk_enable = mtk_gamma_clk_enable,
>>>       .clk_disable = mtk_gamma_clk_disable,
>>> +    .gamma_get_lut_size = mtk_gamma_get_lut_size,
>>>       .gamma_set = mtk_gamma_set,
>>>       .config = mtk_gamma_config,
>>>       .start = mtk_gamma_start,
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
>>> b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
>>> index febcaeef16a1..c1355960e195 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
>>> @@ -67,6 +67,7 @@ struct mtk_ddp_comp_funcs {
>>>       void (*layer_config)(struct device *dev, unsigned int idx,
>>>                    struct mtk_plane_state *state,
>>>                    struct cmdq_pkt *cmdq_pkt);
>>> +    unsigned int (*gamma_get_lut_size)(struct device *dev);
>>>       void (*gamma_set)(struct device *dev,
>>>                 struct drm_crtc_state *state);
>>>       void (*bgclr_in_on)(struct device *dev);
>>> @@ -186,6 +187,14 @@ static inline void
>>> mtk_ddp_comp_layer_config(struct mtk_ddp_comp *comp,
>>>           comp->funcs->layer_config(comp->dev, idx, state,
>>> cmdq_pkt);
>>>   }
>>> +static inline unsigned int mtk_ddp_gamma_get_lut_size(struct
>>> mtk_ddp_comp *comp)
>>> +{
>>> +    if (comp->funcs && comp->funcs->gamma_get_lut_size)
>>> +        return comp->funcs->gamma_get_lut_size(comp->dev);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static inline void mtk_ddp_gamma_set(struct mtk_ddp_comp *comp,
>>>                        struct drm_crtc_state *state)
>>>   {
> 
> 
> 

[PATCH RESEND v6 04/11] drm/mediatek: gamma: Improve and simplify HW LUT calculation

[AngeloGioacchino Del Regno]

Use drm_color_lut_extract() to avoid open-coding the bits reduction
calculations for each color channel and use a struct drm_color_lut
to temporarily store the information instead of an array of u32.

Also, slightly improve the precision of the HW LUT calculation in the
LUT DIFF case by performing the subtractions on the 16-bits values and
doing the 10 bits conversion later.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 30 +++++++++++++++--------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
index b25ba209e7a4..204a1aa7bfc9 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -77,7 +77,6 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 	bool lut_diff;
 	u16 lut_size;
 	u32 word;
-	u32 diff[3] = {0};
 
 	/* If there's no gamma lut there's nothing to do here. */
 	if (!state->gamma_lut)
@@ -97,18 +96,29 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 	lut_base = regs + DISP_GAMMA_LUT;
 	lut = (struct drm_color_lut *)state->gamma_lut->data;
 	for (i = 0; i < lut_size; i++) {
+		struct drm_color_lut diff, hwlut;
+
+		hwlut.red = drm_color_lut_extract(lut[i].red, 10);
+		hwlut.green = drm_color_lut_extract(lut[i].green, 10);
+		hwlut.blue = drm_color_lut_extract(lut[i].blue, 10);
+
 		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);
+			word = hwlut.red << 20 +
+			       hwlut.green << 10 +
+			       hwlut.red;
 		} 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);
+			diff.red = lut[i].red - lut[i - 1].red;
+			diff.red = drm_color_lut_extract(diff.red, 10);
+
+			diff.green = lut[i].green - lut[i - 1].green;
+			diff.green = drm_color_lut_extract(diff.green, 10);
+
+			diff.blue = lut[i].blue - lut[i - 1].blue;
+			diff.blue = drm_color_lut_extract(diff.blue, 10);
 
-			word = ((diff[0] & LUT_10BIT_MASK) << 20) +
-				((diff[1] & LUT_10BIT_MASK) << 10) +
-				(diff[2] & LUT_10BIT_MASK);
+			word = diff.blue << 20 +
+			       diff.green << 10 +
+			       diff.red;
 		}
 		writel(word, (lut_base + i * 4));
 	}
-- 
2.40.1

Re: [PATCH RESEND v6 04/11] drm/mediatek: gamma: Improve and simplify HW LUT calculation

[Alexandre Mergnat]

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>

On 27/07/2023 11:46, AngeloGioacchino Del Regno wrote:
> Use drm_color_lut_extract() to avoid open-coding the bits reduction
> calculations for each color channel and use a struct drm_color_lut
> to temporarily store the information instead of an array of u32.
> 
> Also, slightly improve the precision of the HW LUT calculation in the
> LUT DIFF case by performing the subtractions on the 16-bits values and
> doing the 10 bits conversion later.

-- 
Regards,
Alexandre

[PATCH RESEND v6 05/11] drm/mediatek: gamma: Enable the Gamma LUT table only after programming

[AngeloGioacchino Del Regno]

Move the write to DISP_GAMMA_CFG to enable the Gamma LUT to after
programming the actual table to avoid potential visual glitches during
table modification.

Note:
GAMMA should get enabled in between vblanks, but this requires many
efforts in order to make this happen, as that requires migrating all
of the writes to make use of CMDQ instead of cpu writes and that's
not trivial. For this reason, this patch only moves the LUT enable.
The CMDQ rework will come at a later time.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
index 204a1aa7bfc9..b75a77af5205 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -71,12 +71,12 @@ unsigned int mtk_gamma_get_lut_size(struct device *dev)
 void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crtc_state *state)
 {
 	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
-	unsigned int i, reg;
+	unsigned int i;
 	struct drm_color_lut *lut;
 	void __iomem *lut_base;
 	bool lut_diff;
 	u16 lut_size;
-	u32 word;
+	u32 cfg_val, word;
 
 	/* If there's no gamma lut there's nothing to do here. */
 	if (!state->gamma_lut)
@@ -90,9 +90,7 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 		lut_size = LUT_SIZE_DEFAULT;
 	}
 
-	reg = readl(regs + DISP_GAMMA_CFG);
-	reg = reg | GAMMA_LUT_EN;
-	writel(reg, regs + DISP_GAMMA_CFG);
+	cfg_val = readl(regs + DISP_GAMMA_CFG);
 	lut_base = regs + DISP_GAMMA_LUT;
 	lut = (struct drm_color_lut *)state->gamma_lut->data;
 	for (i = 0; i < lut_size; i++) {
@@ -122,6 +120,11 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 		}
 		writel(word, (lut_base + i * 4));
 	}
+
+	/* Enable the gamma table */
+	cfg_val = cfg_val | GAMMA_LUT_EN;
+
+	writel(cfg_val, regs + DISP_GAMMA_CFG);
 }
 
 void mtk_gamma_set(struct device *dev, struct drm_crtc_state *state)
-- 
2.40.1

Re: [PATCH RESEND v6 05/11] drm/mediatek: gamma: Enable the Gamma LUT table only after programming

[Alexandre Mergnat]

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>

On 27/07/2023 11:46, AngeloGioacchino Del Regno wrote:
> Move the write to DISP_GAMMA_CFG to enable the Gamma LUT to after
> programming the actual table to avoid potential visual glitches during
> table modification.
> 
> Note:
> GAMMA should get enabled in between vblanks, but this requires many
> efforts in order to make this happen, as that requires migrating all
> of the writes to make use of CMDQ instead of cpu writes and that's
> not trivial. For this reason, this patch only moves the LUT enable.
> The CMDQ rework will come at a later time.

-- 
Regards,
Alexandre

[PATCH RESEND v6 06/11] drm/mediatek: gamma: Use bitfield macros

[AngeloGioacchino Del Regno]

Make the code more robust and improve readability by using bitfield
macros instead of open coding bit operations.
While at it, also add a definition for LUT_BITS_DEFAULT.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 41 ++++++++++++++---------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
index b75a77af5205..f4bf5b37992c 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2021 MediaTek Inc.
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/component.h>
 #include <linux/module.h>
@@ -22,9 +23,16 @@
 #define GAMMA_LUT_EN					BIT(1)
 #define GAMMA_DITHERING					BIT(2)
 #define DISP_GAMMA_SIZE				0x0030
+#define DISP_GAMMA_SIZE_HSIZE				GENMASK(28, 16)
+#define DISP_GAMMA_SIZE_VSIZE				GENMASK(12, 0)
 #define DISP_GAMMA_LUT				0x0700
 
+#define DISP_GAMMA_LUT_10BIT_R			GENMASK(29, 20)
+#define DISP_GAMMA_LUT_10BIT_G			GENMASK(19, 10)
+#define DISP_GAMMA_LUT_10BIT_B			GENMASK(9, 0)
+
 #define LUT_10BIT_MASK				0x03ff
+#define LUT_BITS_DEFAULT			10
 #define LUT_SIZE_DEFAULT			512
 
 struct mtk_disp_gamma_data {
@@ -96,33 +104,33 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 	for (i = 0; i < lut_size; i++) {
 		struct drm_color_lut diff, hwlut;
 
-		hwlut.red = drm_color_lut_extract(lut[i].red, 10);
-		hwlut.green = drm_color_lut_extract(lut[i].green, 10);
-		hwlut.blue = drm_color_lut_extract(lut[i].blue, 10);
+		hwlut.red = drm_color_lut_extract(lut[i].red, LUT_BITS_DEFAULT);
+		hwlut.green = drm_color_lut_extract(lut[i].green, LUT_BITS_DEFAULT);
+		hwlut.blue = drm_color_lut_extract(lut[i].blue, LUT_BITS_DEFAULT);
 
 		if (!lut_diff || (i % 2 == 0)) {
-			word = hwlut.red << 20 +
-			       hwlut.green << 10 +
-			       hwlut.red;
+			word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, hwlut.red);
+			word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, hwlut.green);
+			word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, hwlut.blue);
 		} else {
 			diff.red = lut[i].red - lut[i - 1].red;
-			diff.red = drm_color_lut_extract(diff.red, 10);
+			diff.red = drm_color_lut_extract(diff.red, LUT_BITS_DEFAULT);
 
 			diff.green = lut[i].green - lut[i - 1].green;
-			diff.green = drm_color_lut_extract(diff.green, 10);
+			diff.green = drm_color_lut_extract(diff.green, LUT_BITS_DEFAULT);
 
 			diff.blue = lut[i].blue - lut[i - 1].blue;
-			diff.blue = drm_color_lut_extract(diff.blue, 10);
+			diff.blue = drm_color_lut_extract(diff.blue, LUT_BITS_DEFAULT);
 
-			word = diff.blue << 20 +
-			       diff.green << 10 +
-			       diff.red;
+			word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, diff.red);
+			word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, diff.green);
+			word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, diff.blue);
 		}
 		writel(word, (lut_base + i * 4));
 	}
 
 	/* Enable the gamma table */
-	cfg_val = cfg_val | GAMMA_LUT_EN;
+	cfg_val |= FIELD_PREP(GAMMA_LUT_EN, 1);
 
 	writel(cfg_val, regs + DISP_GAMMA_CFG);
 }
@@ -139,9 +147,12 @@ void mtk_gamma_config(struct device *dev, unsigned int w,
 		      unsigned int bpc, struct cmdq_pkt *cmdq_pkt)
 {
 	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
+	u32 sz;
+
+	sz = FIELD_PREP(DISP_GAMMA_SIZE_HSIZE, w);
+	sz |= FIELD_PREP(DISP_GAMMA_SIZE_VSIZE, h);
 
-	mtk_ddp_write(cmdq_pkt, h << 16 | w, &gamma->cmdq_reg, gamma->regs,
-		      DISP_GAMMA_SIZE);
+	mtk_ddp_write(cmdq_pkt, sz, &gamma->cmdq_reg, gamma->regs, DISP_GAMMA_SIZE);
 	if (gamma->data && gamma->data->has_dither)
 		mtk_dither_set_common(gamma->regs, &gamma->cmdq_reg, bpc,
 				      DISP_GAMMA_CFG, GAMMA_DITHERING, cmdq_pkt);
-- 
2.40.1

Re: [PATCH RESEND v6 06/11] drm/mediatek: gamma: Use bitfield macros

[Alexandre Mergnat]

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>

On 27/07/2023 11:46, AngeloGioacchino Del Regno wrote:
> Make the code more robust and improve readability by using bitfield
> macros instead of open coding bit operations.
> While at it, also add a definition for LUT_BITS_DEFAULT.

-- 
Regards,
Alexandre

[PATCH RESEND v6 07/11] drm/mediatek: gamma: Support specifying number of bits per LUT component

[AngeloGioacchino Del Regno]

New SoCs, like MT8195, not only may support bigger lookup tables, but
have got a different register layout to support bigger precision:
support specifying the number of `lut_bits` for each SoC and use it
in mtk_gamma_set_common() to perform the right calculation.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
index f4bf5b37992c..407fb0264b80 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -39,6 +39,7 @@ struct mtk_disp_gamma_data {
 	bool has_dither;
 	bool lut_diff;
 	u16 lut_size;
+	u8 lut_bits;
 };
 
 /*
@@ -84,6 +85,7 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 	void __iomem *lut_base;
 	bool lut_diff;
 	u16 lut_size;
+	u8 lut_bits;
 	u32 cfg_val, word;
 
 	/* If there's no gamma lut there's nothing to do here. */
@@ -92,9 +94,11 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 
 	if (gamma && gamma->data) {
 		lut_diff = gamma->data->lut_diff;
+		lut_bits = gamma->data->lut_bits;
 		lut_size = gamma->data->lut_size;
 	} else {
 		lut_diff = false;
+		lut_bits = LUT_BITS_DEFAULT;
 		lut_size = LUT_SIZE_DEFAULT;
 	}
 
@@ -104,9 +108,9 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 	for (i = 0; i < lut_size; i++) {
 		struct drm_color_lut diff, hwlut;
 
-		hwlut.red = drm_color_lut_extract(lut[i].red, LUT_BITS_DEFAULT);
-		hwlut.green = drm_color_lut_extract(lut[i].green, LUT_BITS_DEFAULT);
-		hwlut.blue = drm_color_lut_extract(lut[i].blue, LUT_BITS_DEFAULT);
+		hwlut.red = drm_color_lut_extract(lut[i].red, lut_bits);
+		hwlut.green = drm_color_lut_extract(lut[i].green, lut_bits);
+		hwlut.blue = drm_color_lut_extract(lut[i].blue, lut_bits);
 
 		if (!lut_diff || (i % 2 == 0)) {
 			word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, hwlut.red);
@@ -114,13 +118,13 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 			word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, hwlut.blue);
 		} else {
 			diff.red = lut[i].red - lut[i - 1].red;
-			diff.red = drm_color_lut_extract(diff.red, LUT_BITS_DEFAULT);
+			diff.red = drm_color_lut_extract(diff.red, lut_bits);
 
 			diff.green = lut[i].green - lut[i - 1].green;
-			diff.green = drm_color_lut_extract(diff.green, LUT_BITS_DEFAULT);
+			diff.green = drm_color_lut_extract(diff.green, lut_bits);
 
 			diff.blue = lut[i].blue - lut[i - 1].blue;
-			diff.blue = drm_color_lut_extract(diff.blue, LUT_BITS_DEFAULT);
+			diff.blue = drm_color_lut_extract(diff.blue, lut_bits);
 
 			word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, diff.red);
 			word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, diff.green);
@@ -237,10 +241,12 @@ 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_bits = 10,
 	.lut_size = 512,
 };
 
 static const struct mtk_disp_gamma_data mt8183_gamma_driver_data = {
+	.lut_bits = 10,
 	.lut_diff = true,
 	.lut_size = 512,
 };
-- 
2.40.1

Re: [PATCH RESEND v6 07/11] drm/mediatek: gamma: Support specifying number of bits per LUT component

[Alexandre Mergnat]

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>

On 27/07/2023 11:46, AngeloGioacchino Del Regno wrote:
> New SoCs, like MT8195, not only may support bigger lookup tables, but
> have got a different register layout to support bigger precision:
> support specifying the number of `lut_bits` for each SoC and use it
> in mtk_gamma_set_common() to perform the right calculation.

-- 
Regards,
Alexandre

[PATCH RESEND v6 08/11] drm/mediatek: gamma: Support multi-bank gamma LUT

[AngeloGioacchino Del Regno]

Newer Gamma IP have got multiple LUT banks: support specifying the
size of the LUT banks and handle bank-switching before programming
the LUT in mtk_gamma_set_common() in preparation for adding support
for MT8195 and newer SoCs.

Suggested-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
[Angelo: Refactored original commit]
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 74 ++++++++++++++---------
 1 file changed, 47 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
index 407fb0264b80..f1a0b18b6c1a 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -25,6 +25,8 @@
 #define DISP_GAMMA_SIZE				0x0030
 #define DISP_GAMMA_SIZE_HSIZE				GENMASK(28, 16)
 #define DISP_GAMMA_SIZE_VSIZE				GENMASK(12, 0)
+#define DISP_GAMMA_BANK				0x0100
+#define DISP_GAMMA_BANK_BANK				GENMASK(1, 0)
 #define DISP_GAMMA_LUT				0x0700
 
 #define DISP_GAMMA_LUT_10BIT_R			GENMASK(29, 20)
@@ -38,6 +40,7 @@
 struct mtk_disp_gamma_data {
 	bool has_dither;
 	bool lut_diff;
+	u16 lut_bank_size;
 	u16 lut_size;
 	u8 lut_bits;
 };
@@ -84,9 +87,10 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 	struct drm_color_lut *lut;
 	void __iomem *lut_base;
 	bool lut_diff;
-	u16 lut_size;
+	u16 lut_bank_size, lut_size;
 	u8 lut_bits;
-	u32 cfg_val, word;
+	u32 cfg_val, lbank_val, word;
+	int cur_bank, num_lut_banks;
 
 	/* If there's no gamma lut there's nothing to do here. */
 	if (!state->gamma_lut)
@@ -94,43 +98,57 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 
 	if (gamma && gamma->data) {
 		lut_diff = gamma->data->lut_diff;
+		lut_bank_size = gamma->data->lut_bank_size;
 		lut_bits = gamma->data->lut_bits;
 		lut_size = gamma->data->lut_size;
 	} else {
 		lut_diff = false;
+		lut_bank_size = LUT_SIZE_DEFAULT;
 		lut_bits = LUT_BITS_DEFAULT;
 		lut_size = LUT_SIZE_DEFAULT;
 	}
+	num_lut_banks = lut_size / lut_bank_size;
 
 	cfg_val = readl(regs + DISP_GAMMA_CFG);
 	lut_base = regs + DISP_GAMMA_LUT;
 	lut = (struct drm_color_lut *)state->gamma_lut->data;
-	for (i = 0; i < lut_size; i++) {
-		struct drm_color_lut diff, hwlut;
-
-		hwlut.red = drm_color_lut_extract(lut[i].red, lut_bits);
-		hwlut.green = drm_color_lut_extract(lut[i].green, lut_bits);
-		hwlut.blue = drm_color_lut_extract(lut[i].blue, lut_bits);
-
-		if (!lut_diff || (i % 2 == 0)) {
-			word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, hwlut.red);
-			word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, hwlut.green);
-			word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, hwlut.blue);
-		} else {
-			diff.red = lut[i].red - lut[i - 1].red;
-			diff.red = drm_color_lut_extract(diff.red, lut_bits);
-
-			diff.green = lut[i].green - lut[i - 1].green;
-			diff.green = drm_color_lut_extract(diff.green, lut_bits);
-
-			diff.blue = lut[i].blue - lut[i - 1].blue;
-			diff.blue = drm_color_lut_extract(diff.blue, lut_bits);
-
-			word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, diff.red);
-			word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, diff.green);
-			word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, diff.blue);
+
+	for (cur_bank = 0; cur_bank < num_lut_banks; cur_bank++) {
+
+		/* Switch gamma bank and set data mode before writing LUT */
+		if (num_lut_banks > 1) {
+			lbank_val = FIELD_PREP(DISP_GAMMA_BANK_BANK, cur_bank);
+			writel(lbank_val, regs + DISP_GAMMA_BANK);
+		}
+
+		for (i = 0; i < lut_bank_size; i++) {
+			int n = (cur_bank * lut_bank_size) + i;
+			struct drm_color_lut diff, hwlut;
+
+			hwlut.red = drm_color_lut_extract(lut[n].red, lut_bits);
+			hwlut.green = drm_color_lut_extract(lut[n].green, lut_bits);
+			hwlut.blue = drm_color_lut_extract(lut[n].blue, lut_bits);
+
+			if (!lut_diff || (i % 2 == 0)) {
+				word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, hwlut.red);
+				word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, hwlut.green);
+				word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, hwlut.blue);
+			} else {
+				diff.red = lut[n].red - lut[n - 1].red;
+				diff.red = drm_color_lut_extract(diff.red, lut_bits);
+
+				diff.green = lut[n].green - lut[n - 1].green;
+				diff.green = drm_color_lut_extract(diff.green, lut_bits);
+
+				diff.blue = lut[n].blue - lut[n - 1].blue;
+				diff.blue = drm_color_lut_extract(diff.blue, lut_bits);
+
+				word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, diff.red);
+				word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, diff.green);
+				word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, diff.blue);
+			}
+			writel(word, (lut_base + i * 4));
 		}
-		writel(word, (lut_base + i * 4));
 	}
 
 	/* Enable the gamma table */
@@ -241,11 +259,13 @@ 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_bank_size = 512,
 	.lut_bits = 10,
 	.lut_size = 512,
 };
 
 static const struct mtk_disp_gamma_data mt8183_gamma_driver_data = {
+	.lut_bank_size = 512,
 	.lut_bits = 10,
 	.lut_diff = true,
 	.lut_size = 512,
-- 
2.40.1

Re: [PATCH RESEND v6 08/11] drm/mediatek: gamma: Support multi-bank gamma LUT

[Alexandre Mergnat]

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>

On 27/07/2023 11:46, AngeloGioacchino Del Regno wrote:
> Newer Gamma IP have got multiple LUT banks: support specifying the
> size of the LUT banks and handle bank-switching before programming
> the LUT in mtk_gamma_set_common() in preparation for adding support
> for MT8195 and newer SoCs.

-- 
Regards,
Alexandre

[PATCH RESEND v6 09/11] drm/mediatek: gamma: Add support for 12-bit LUT and MT8195

[AngeloGioacchino Del Regno]

Add support for 12-bit gamma lookup tables and introduce the first
user for it: MT8195.
While at it, also reorder the variables in mtk_gamma_set_common()
and rename `lut_base` to `lut0_base` to improve readability.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 61 ++++++++++++++++++-----
 1 file changed, 48 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
index f1a0b18b6c1a..e0e2d2bdbf59 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -27,12 +27,20 @@
 #define DISP_GAMMA_SIZE_VSIZE				GENMASK(12, 0)
 #define DISP_GAMMA_BANK				0x0100
 #define DISP_GAMMA_BANK_BANK				GENMASK(1, 0)
+#define DISP_GAMMA_BANK_DATA_MODE			BIT(2)
 #define DISP_GAMMA_LUT				0x0700
+#define DISP_GAMMA_LUT1				0x0b00
 
+/* For 10 bit LUT layout, R/G/B are in the same register */
 #define DISP_GAMMA_LUT_10BIT_R			GENMASK(29, 20)
 #define DISP_GAMMA_LUT_10BIT_G			GENMASK(19, 10)
 #define DISP_GAMMA_LUT_10BIT_B			GENMASK(9, 0)
 
+/* For 12 bit LUT layout, R/G are in LUT, B is in LUT1 */
+#define DISP_GAMMA_LUT_12BIT_R			GENMASK(11, 0)
+#define DISP_GAMMA_LUT_12BIT_G			GENMASK(23, 12)
+#define DISP_GAMMA_LUT_12BIT_B			GENMASK(11, 0)
+
 #define LUT_10BIT_MASK				0x03ff
 #define LUT_BITS_DEFAULT			10
 #define LUT_SIZE_DEFAULT			512
@@ -83,14 +91,15 @@ unsigned int mtk_gamma_get_lut_size(struct device *dev)
 void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crtc_state *state)
 {
 	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
-	unsigned int i;
+	void __iomem *lut0_base = regs + DISP_GAMMA_LUT;
+	void __iomem *lut1_base = regs + DISP_GAMMA_LUT1;
+	u32 cfg_val, data_mode, lbank_val, word[2];
+	int cur_bank, num_lut_banks;
+	u16 lut_bank_size, lut_size;
 	struct drm_color_lut *lut;
-	void __iomem *lut_base;
+	unsigned int i;
 	bool lut_diff;
-	u16 lut_bank_size, lut_size;
 	u8 lut_bits;
-	u32 cfg_val, lbank_val, word;
-	int cur_bank, num_lut_banks;
 
 	/* If there's no gamma lut there's nothing to do here. */
 	if (!state->gamma_lut)
@@ -110,14 +119,17 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 	num_lut_banks = lut_size / lut_bank_size;
 
 	cfg_val = readl(regs + DISP_GAMMA_CFG);
-	lut_base = regs + DISP_GAMMA_LUT;
 	lut = (struct drm_color_lut *)state->gamma_lut->data;
 
+	/* Switch to 12 bits data mode if supported */
+	data_mode = FIELD_PREP(DISP_GAMMA_BANK_DATA_MODE, !!(lut_bits == 12));
+
 	for (cur_bank = 0; cur_bank < num_lut_banks; cur_bank++) {
 
 		/* Switch gamma bank and set data mode before writing LUT */
 		if (num_lut_banks > 1) {
 			lbank_val = FIELD_PREP(DISP_GAMMA_BANK_BANK, cur_bank);
+			lbank_val |= data_mode;
 			writel(lbank_val, regs + DISP_GAMMA_BANK);
 		}
 
@@ -130,9 +142,15 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 			hwlut.blue = drm_color_lut_extract(lut[n].blue, lut_bits);
 
 			if (!lut_diff || (i % 2 == 0)) {
-				word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, hwlut.red);
-				word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, hwlut.green);
-				word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, hwlut.blue);
+				if (lut_bits == 12) {
+					word[0] = FIELD_PREP(DISP_GAMMA_LUT_12BIT_R, hwlut.red);
+					word[0] |= FIELD_PREP(DISP_GAMMA_LUT_12BIT_G, hwlut.green);
+					word[1] = FIELD_PREP(DISP_GAMMA_LUT_12BIT_B, hwlut.blue);
+				} else {
+					word[0] = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, hwlut.red);
+					word[0] |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, hwlut.green);
+					word[0] |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, hwlut.blue);
+				}
 			} else {
 				diff.red = lut[n].red - lut[n - 1].red;
 				diff.red = drm_color_lut_extract(diff.red, lut_bits);
@@ -143,11 +161,19 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 				diff.blue = lut[n].blue - lut[n - 1].blue;
 				diff.blue = drm_color_lut_extract(diff.blue, lut_bits);
 
-				word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, diff.red);
-				word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, diff.green);
-				word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, diff.blue);
+				if (lut_bits == 12) {
+					word[0] = FIELD_PREP(DISP_GAMMA_LUT_12BIT_R, diff.red);
+					word[0] |= FIELD_PREP(DISP_GAMMA_LUT_12BIT_G, diff.green);
+					word[1] = FIELD_PREP(DISP_GAMMA_LUT_12BIT_B, diff.blue);
+				} else {
+					word[0] = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, diff.red);
+					word[0] |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, diff.green);
+					word[0] |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, diff.blue);
+				}
 			}
-			writel(word, (lut_base + i * 4));
+			writel(word[0], (lut0_base + i * 4));
+			if (lut_bits == 12)
+				writel(word[1], (lut1_base + i * 4));
 		}
 	}
 
@@ -271,11 +297,20 @@ static const struct mtk_disp_gamma_data mt8183_gamma_driver_data = {
 	.lut_size = 512,
 };
 
+static const struct mtk_disp_gamma_data mt8195_gamma_driver_data = {
+	.lut_bank_size = 256,
+	.lut_bits = 12,
+	.lut_diff = true,
+	.lut_size = 1024,
+};
+
 static const struct of_device_id mtk_disp_gamma_driver_dt_match[] = {
 	{ .compatible = "mediatek,mt8173-disp-gamma",
 	  .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);
-- 
2.40.1

Re: [PATCH RESEND v6 09/11] drm/mediatek: gamma: Add support for 12-bit LUT and MT8195

[Alexandre Mergnat]

Hi Angelo !

On 27/07/2023 11:46, AngeloGioacchino Del Regno wrote:
> Add support for 12-bit gamma lookup tables and introduce the first
> user for it: MT8195.
> While at it, also reorder the variables in mtk_gamma_set_common()
> and rename `lut_base` to `lut0_base` to improve readability.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>   drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 61 ++++++++++++++++++-----
>   1 file changed, 48 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> index f1a0b18b6c1a..e0e2d2bdbf59 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> @@ -27,12 +27,20 @@
>   #define DISP_GAMMA_SIZE_VSIZE				GENMASK(12, 0)
>   #define DISP_GAMMA_BANK				0x0100
>   #define DISP_GAMMA_BANK_BANK				GENMASK(1, 0)
> +#define DISP_GAMMA_BANK_DATA_MODE			BIT(2)
>   #define DISP_GAMMA_LUT				0x0700
> +#define DISP_GAMMA_LUT1				0x0b00

Is this offset generic to all MTK SoC which support this driver ?

>   
> +/* For 10 bit LUT layout, R/G/B are in the same register */
>   #define DISP_GAMMA_LUT_10BIT_R			GENMASK(29, 20)
>   #define DISP_GAMMA_LUT_10BIT_G			GENMASK(19, 10)
>   #define DISP_GAMMA_LUT_10BIT_B			GENMASK(9, 0)
>   
> +/* For 12 bit LUT layout, R/G are in LUT, B is in LUT1 */

As I understood from the application processor registers (v0.4), R/G are 
in LUT, B is in LUT1 for 10bit and 12bit for MT8195. Can you check 
please to be sure ?

> +#define DISP_GAMMA_LUT_12BIT_R			GENMASK(11, 0)
> +#define DISP_GAMMA_LUT_12BIT_G			GENMASK(23, 12)
> +#define DISP_GAMMA_LUT_12BIT_B			GENMASK(11, 0)
> +
>   #define LUT_10BIT_MASK				0x03ff
>   #define LUT_BITS_DEFAULT			10
>   #define LUT_SIZE_DEFAULT			512
> @@ -83,14 +91,15 @@ unsigned int mtk_gamma_get_lut_size(struct device *dev)
>   void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crtc_state *state)
>   {
>   	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> -	unsigned int i;
> +	void __iomem *lut0_base = regs + DISP_GAMMA_LUT;
> +	void __iomem *lut1_base = regs + DISP_GAMMA_LUT1;
> +	u32 cfg_val, data_mode, lbank_val, word[2];
> +	int cur_bank, num_lut_banks;
> +	u16 lut_bank_size, lut_size;
>   	struct drm_color_lut *lut;
> -	void __iomem *lut_base;
> +	unsigned int i;
>   	bool lut_diff;
> -	u16 lut_bank_size, lut_size;
>   	u8 lut_bits;
> -	u32 cfg_val, lbank_val, word;
> -	int cur_bank, num_lut_banks;
>   
>   	/* If there's no gamma lut there's nothing to do here. */
>   	if (!state->gamma_lut)
> @@ -110,14 +119,17 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
>   	num_lut_banks = lut_size / lut_bank_size;
>   
>   	cfg_val = readl(regs + DISP_GAMMA_CFG);
> -	lut_base = regs + DISP_GAMMA_LUT;
>   	lut = (struct drm_color_lut *)state->gamma_lut->data;
>   
> +	/* Switch to 12 bits data mode if supported */
> +	data_mode = FIELD_PREP(DISP_GAMMA_BANK_DATA_MODE, !!(lut_bits == 12));
> +
>   	for (cur_bank = 0; cur_bank < num_lut_banks; cur_bank++) {
>   
>   		/* Switch gamma bank and set data mode before writing LUT */
>   		if (num_lut_banks > 1) {
>   			lbank_val = FIELD_PREP(DISP_GAMMA_BANK_BANK, cur_bank);
> +			lbank_val |= data_mode;
>   			writel(lbank_val, regs + DISP_GAMMA_BANK);
>   		}
>   
> @@ -130,9 +142,15 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
>   			hwlut.blue = drm_color_lut_extract(lut[n].blue, lut_bits);
>   
>   			if (!lut_diff || (i % 2 == 0)) {
> -				word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, hwlut.red);
> -				word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, hwlut.green);
> -				word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, hwlut.blue);
> +				if (lut_bits == 12) {
> +					word[0] = FIELD_PREP(DISP_GAMMA_LUT_12BIT_R, hwlut.red);
> +					word[0] |= FIELD_PREP(DISP_GAMMA_LUT_12BIT_G, hwlut.green);
> +					word[1] = FIELD_PREP(DISP_GAMMA_LUT_12BIT_B, hwlut.blue);
> +				} else {
> +					word[0] = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, hwlut.red);
> +					word[0] |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, hwlut.green);
> +					word[0] |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, hwlut.blue);
> +				}
>   			} else {
>   				diff.red = lut[n].red - lut[n - 1].red;
>   				diff.red = drm_color_lut_extract(diff.red, lut_bits);
> @@ -143,11 +161,19 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
>   				diff.blue = lut[n].blue - lut[n - 1].blue;
>   				diff.blue = drm_color_lut_extract(diff.blue, lut_bits);
>   
> -				word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, diff.red);
> -				word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, diff.green);
> -				word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, diff.blue);
> +				if (lut_bits == 12) {
> +					word[0] = FIELD_PREP(DISP_GAMMA_LUT_12BIT_R, diff.red);
> +					word[0] |= FIELD_PREP(DISP_GAMMA_LUT_12BIT_G, diff.green);
> +					word[1] = FIELD_PREP(DISP_GAMMA_LUT_12BIT_B, diff.blue);
> +				} else {
> +					word[0] = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, diff.red);
> +					word[0] |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, diff.green);
> +					word[0] |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, diff.blue);
> +				}
>   			}
> -			writel(word, (lut_base + i * 4));
> +			writel(word[0], (lut0_base + i * 4));
> +			if (lut_bits == 12)
> +				writel(word[1], (lut1_base + i * 4));

ditto

>   		}
>   	}
>   
> @@ -271,11 +297,20 @@ static const struct mtk_disp_gamma_data mt8183_gamma_driver_data = {
>   	.lut_size = 512,
>   };
>   
> +static const struct mtk_disp_gamma_data mt8195_gamma_driver_data = {
> +	.lut_bank_size = 256,
> +	.lut_bits = 12,

If I'm right, ".lut_bits = 10" will not work properly.

> +	.lut_diff = true,
> +	.lut_size = 1024,
> +};
> +
>   static const struct of_device_id mtk_disp_gamma_driver_dt_match[] = {
>   	{ .compatible = "mediatek,mt8173-disp-gamma",
>   	  .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);

-- 
Regards,
Alexandre

Re: [PATCH RESEND v6 09/11] drm/mediatek: gamma: Add support for 12-bit LUT and MT8195

[AngeloGioacchino Del Regno]

Il 27/07/23 13:03, Alexandre Mergnat ha scritto:
> Hi Angelo !
> 
> On 27/07/2023 11:46, AngeloGioacchino Del Regno wrote:
>> Add support for 12-bit gamma lookup tables and introduce the first
>> user for it: MT8195.
>> While at it, also reorder the variables in mtk_gamma_set_common()
>> and rename `lut_base` to `lut0_base` to improve readability.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 61 ++++++++++++++++++-----
>>   1 file changed, 48 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c 
>> b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
>> index f1a0b18b6c1a..e0e2d2bdbf59 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
>> @@ -27,12 +27,20 @@
>>   #define DISP_GAMMA_SIZE_VSIZE                GENMASK(12, 0)
>>   #define DISP_GAMMA_BANK                0x0100
>>   #define DISP_GAMMA_BANK_BANK                GENMASK(1, 0)
>> +#define DISP_GAMMA_BANK_DATA_MODE            BIT(2)
>>   #define DISP_GAMMA_LUT                0x0700
>> +#define DISP_GAMMA_LUT1                0x0b00
> 
> Is this offset generic to all MTK SoC which support this driver ?
> 
>> +/* For 10 bit LUT layout, R/G/B are in the same register */
>>   #define DISP_GAMMA_LUT_10BIT_R            GENMASK(29, 20)
>>   #define DISP_GAMMA_LUT_10BIT_G            GENMASK(19, 10)
>>   #define DISP_GAMMA_LUT_10BIT_B            GENMASK(9, 0)
>> +/* For 12 bit LUT layout, R/G are in LUT, B is in LUT1 */
> 
> As I understood from the application processor registers (v0.4), R/G are in LUT, B 
> is in LUT1 for 10bit and 12bit for MT8195. Can you check please to be sure ?
> 

That's right, but here I'm implying that 10-bit LUT is only for older SoCs, and
all of them have got the same register layout with one LUT register for R, G, B,
while all the new SoCs, which have got 12-bits LUT support, have got the new
register layout with two LUT registers (and multiple banks).
Infact, the MT8195 SoC was added here with 12-bits LUT support only (as the LUT
parameters extraction is easily handled by the drm_color_lut_extract() function).

The alternative would've been to add two compatibles, like
"mediatek,mt8195-disp-gamma-10bits" and "mediatek,mt8195-disp-gamma-12bits",
or a boolean property like "mediatek,lut-12bits" which would appear literally
everywhere starting from a certain point in time (since there's no reason to
use 10-bits LUT on MT8195, that starts now!).

Even then, consider the complication in code, where mtk_gamma_set_common()
would have to handle:
- 10-bits, layout A
- 10-bits, layout B -> but fallback to layout A if this is AAL
- 12-bits layout

is_aal = !(gamma && gamma->data);

for_each_bank()
{
	if (num_lut_banks > 1) write_num_bank();

	for (i = 0; i < lut_bank_size; i++) {
		.......

		if (!lut_diff || (i % 2 == 0)) {
			if (lut_bits == 12 || (lut_bits == 10 && layout_b)) {
				... setup word[0],[1] ...
			} else if (layout_b && !is_aal) {
				...setup word[0],[1]...
			} else {
				...setup word[0]
			}
		} else {
			 ^^^ almost repeat the same ^^^
		}
		writel(word[0], (...));
		if (lut_bits == 12 || (lut_bits == 10 && layout_b) && !is_aal)
			writel(word[i] (....));
	}
}

probe() {
	if (of_property_read_bool(dev->of_node, "mediatek,lut-12bits") ||
	    data->supports_only_12bits)
		priv->lut_bits = 12;
	else
		priv->lut_bits = 10;
}

...at least, that's the implementation that I would do to solve your concern,
which isn't *too bad*, but still, a big question arises here...


Why should we care about supporting *both* 10-bit and 12-bit Gamma LUTs on
the *same* SoC?


A 12-bit LUT gives us more precision and there's no penalty if we want to
convert a 10-bit LUT to a 12-bits one, as we're simply "ignoring" the value
of two bits per component (no expensive calculation involved)...

Is there anything that I'm underestimating here?

Cheers,
Angelo

>> +#define DISP_GAMMA_LUT_12BIT_R            GENMASK(11, 0)
>> +#define DISP_GAMMA_LUT_12BIT_G            GENMASK(23, 12)
>> +#define DISP_GAMMA_LUT_12BIT_B            GENMASK(11, 0)
>> +
>>   #define LUT_10BIT_MASK                0x03ff
>>   #define LUT_BITS_DEFAULT            10
>>   #define LUT_SIZE_DEFAULT            512
>> @@ -83,14 +91,15 @@ unsigned int mtk_gamma_get_lut_size(struct device *dev)
>>   void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct 
>> drm_crtc_state *state)
>>   {
>>       struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
>> -    unsigned int i;
>> +    void __iomem *lut0_base = regs + DISP_GAMMA_LUT;
>> +    void __iomem *lut1_base = regs + DISP_GAMMA_LUT1;
>> +    u32 cfg_val, data_mode, lbank_val, word[2];
>> +    int cur_bank, num_lut_banks;
>> +    u16 lut_bank_size, lut_size;
>>       struct drm_color_lut *lut;
>> -    void __iomem *lut_base;
>> +    unsigned int i;
>>       bool lut_diff;
>> -    u16 lut_bank_size, lut_size;
>>       u8 lut_bits;
>> -    u32 cfg_val, lbank_val, word;
>> -    int cur_bank, num_lut_banks;
>>       /* If there's no gamma lut there's nothing to do here. */
>>       if (!state->gamma_lut)
>> @@ -110,14 +119,17 @@ void mtk_gamma_set_common(struct device *dev, void __iomem 
>> *regs, struct drm_crt
>>       num_lut_banks = lut_size / lut_bank_size;
>>       cfg_val = readl(regs + DISP_GAMMA_CFG);
>> -    lut_base = regs + DISP_GAMMA_LUT;
>>       lut = (struct drm_color_lut *)state->gamma_lut->data;
>> +    /* Switch to 12 bits data mode if supported */
>> +    data_mode = FIELD_PREP(DISP_GAMMA_BANK_DATA_MODE, !!(lut_bits == 12));
>> +
>>       for (cur_bank = 0; cur_bank < num_lut_banks; cur_bank++) {
>>           /* Switch gamma bank and set data mode before writing LUT */
>>           if (num_lut_banks > 1) {
>>               lbank_val = FIELD_PREP(DISP_GAMMA_BANK_BANK, cur_bank);
>> +            lbank_val |= data_mode;
>>               writel(lbank_val, regs + DISP_GAMMA_BANK);
>>           }
>> @@ -130,9 +142,15 @@ void mtk_gamma_set_common(struct device *dev, void __iomem 
>> *regs, struct drm_crt
>>               hwlut.blue = drm_color_lut_extract(lut[n].blue, lut_bits);
>>               if (!lut_diff || (i % 2 == 0)) {
>> -                word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, hwlut.red);
>> -                word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, hwlut.green);
>> -                word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, hwlut.blue);
>> +                if (lut_bits == 12) {
>> +                    word[0] = FIELD_PREP(DISP_GAMMA_LUT_12BIT_R, hwlut.red);
>> +                    word[0] |= FIELD_PREP(DISP_GAMMA_LUT_12BIT_G, hwlut.green);
>> +                    word[1] = FIELD_PREP(DISP_GAMMA_LUT_12BIT_B, hwlut.blue);
>> +                } else {
>> +                    word[0] = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, hwlut.red);
>> +                    word[0] |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, hwlut.green);
>> +                    word[0] |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, hwlut.blue);
>> +                }
>>               } else {
>>                   diff.red = lut[n].red - lut[n - 1].red;
>>                   diff.red = drm_color_lut_extract(diff.red, lut_bits);
>> @@ -143,11 +161,19 @@ void mtk_gamma_set_common(struct device *dev, void __iomem 
>> *regs, struct drm_crt
>>                   diff.blue = lut[n].blue - lut[n - 1].blue;
>>                   diff.blue = drm_color_lut_extract(diff.blue, lut_bits);
>> -                word = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, diff.red);
>> -                word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, diff.green);
>> -                word |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, diff.blue);
>> +                if (lut_bits == 12) {
>> +                    word[0] = FIELD_PREP(DISP_GAMMA_LUT_12BIT_R, diff.red);
>> +                    word[0] |= FIELD_PREP(DISP_GAMMA_LUT_12BIT_G, diff.green);
>> +                    word[1] = FIELD_PREP(DISP_GAMMA_LUT_12BIT_B, diff.blue);
>> +                } else {
>> +                    word[0] = FIELD_PREP(DISP_GAMMA_LUT_10BIT_R, diff.red);
>> +                    word[0] |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_G, diff.green);
>> +                    word[0] |= FIELD_PREP(DISP_GAMMA_LUT_10BIT_B, diff.blue);
>> +                }
>>               }
>> -            writel(word, (lut_base + i * 4));
>> +            writel(word[0], (lut0_base + i * 4));
>> +            if (lut_bits == 12)
>> +                writel(word[1], (lut1_base + i * 4));
> 
> ditto
> 
>>           }
>>       }
>> @@ -271,11 +297,20 @@ static const struct mtk_disp_gamma_data 
>> mt8183_gamma_driver_data = {
>>       .lut_size = 512,
>>   };
>> +static const struct mtk_disp_gamma_data mt8195_gamma_driver_data = {
>> +    .lut_bank_size = 256,
>> +    .lut_bits = 12,
> 
> If I'm right, ".lut_bits = 10" will not work properly.
> 
>> +    .lut_diff = true,
>> +    .lut_size = 1024,
>> +};
>> +
>>   static const struct of_device_id mtk_disp_gamma_driver_dt_match[] = {
>>       { .compatible = "mediatek,mt8173-disp-gamma",
>>         .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);
> 

Re: [PATCH RESEND v6 09/11] drm/mediatek: gamma: Add support for 12-bit LUT and MT8195

[Alexandre Mergnat]

Hi Angelo

On 27/07/2023 15:06, AngeloGioacchino Del Regno wrote:
>>> +/* For 10 bit LUT layout, R/G/B are in the same register */
>>>   #define DISP_GAMMA_LUT_10BIT_R            GENMASK(29, 20)
>>>   #define DISP_GAMMA_LUT_10BIT_G            GENMASK(19, 10)
>>>   #define DISP_GAMMA_LUT_10BIT_B            GENMASK(9, 0)
>>> +/* For 12 bit LUT layout, R/G are in LUT, B is in LUT1 */
>>
>> As I understood from the application processor registers (v0.4), R/G 
>> are in LUT, B is in LUT1 for 10bit and 12bit for MT8195. Can you check 
>> please to be sure ?
>>
> 
> That's right, but here I'm implying that 10-bit LUT is only for older 
> SoCs, and
> all of them have got the same register layout with one LUT register for 
> R, G, B,
> while all the new SoCs, which have got 12-bits LUT support, have got the 
> new
> register layout with two LUT registers (and multiple banks).
> Infact, the MT8195 SoC was added here with 12-bits LUT support only (as 
> the LUT
> parameters extraction is easily handled by the drm_color_lut_extract() 
> function).
> 
> The alternative would've been to add two compatibles, like
> "mediatek,mt8195-disp-gamma-10bits" and 
> "mediatek,mt8195-disp-gamma-12bits",
> or a boolean property like "mediatek,lut-12bits" which would appear 
> literally
> everywhere starting from a certain point in time (since there's no 
> reason to
> use 10-bits LUT on MT8195, that starts now!).
> 
> Even then, consider the complication in code, where mtk_gamma_set_common()
> would have to handle:
> - 10-bits, layout A
> - 10-bits, layout B -> but fallback to layout A if this is AAL
> - 12-bits layout
> 
> is_aal = !(gamma && gamma->data);
> 
> for_each_bank()
> {
>      if (num_lut_banks > 1) write_num_bank();
> 
>      for (i = 0; i < lut_bank_size; i++) {
>          .......
> 
>          if (!lut_diff || (i % 2 == 0)) {
>              if (lut_bits == 12 || (lut_bits == 10 && layout_b)) {
>                  ... setup word[0],[1] ...
>              } else if (layout_b && !is_aal) {
>                  ...setup word[0],[1]...
>              } else {
>                  ...setup word[0]
>              }
>          } else {
>               ^^^ almost repeat the same ^^^
>          }
>          writel(word[0], (...));
>          if (lut_bits == 12 || (lut_bits == 10 && layout_b) && !is_aal)
>              writel(word[i] (....));
>      }
> }
> 
> probe() {
>      if (of_property_read_bool(dev->of_node, "mediatek,lut-12bits") ||
>          data->supports_only_12bits)
>          priv->lut_bits = 12;
>      else
>          priv->lut_bits = 10;
> }
> 
> ...at least, that's the implementation that I would do to solve your 
> concern,
> which isn't *too bad*, but still, a big question arises here...
> 
> 
> Why should we care about supporting *both* 10-bit and 12-bit Gamma LUTs on
> the *same* SoC?
> 
> 
> A 12-bit LUT gives us more precision and there's no penalty if we want to
> convert a 10-bit LUT to a 12-bits one, as we're simply "ignoring" the value
> of two bits per component (no expensive calculation involved)...
> 
> Is there anything that I'm underestimating here?

Thanks for you explanation !
I think your choice is not bad, but it's not clear that MT8195 10 bit 
LUT isn't supported at all.
So, IMHO, the first solution is to support it like you explained it 
above, and the second solution is to add comment somewhere to clarify 
that driver doesn't support 10 bit LUT if the SoC is able to use 12 bit 
LUT, like MT8195 10 bit.

Is that relevant ? :D

-- 
Regards,
Alexandre

Re: [PATCH RESEND v6 09/11] drm/mediatek: gamma: Add support for 12-bit LUT and MT8195

[AngeloGioacchino Del Regno]

Il 28/07/23 14:58, Alexandre Mergnat ha scritto:
> Hi Angelo
> 
> On 27/07/2023 15:06, AngeloGioacchino Del Regno wrote:
>>>> +/* For 10 bit LUT layout, R/G/B are in the same register */
>>>>   #define DISP_GAMMA_LUT_10BIT_R            GENMASK(29, 20)
>>>>   #define DISP_GAMMA_LUT_10BIT_G            GENMASK(19, 10)
>>>>   #define DISP_GAMMA_LUT_10BIT_B            GENMASK(9, 0)
>>>> +/* For 12 bit LUT layout, R/G are in LUT, B is in LUT1 */
>>>
>>> As I understood from the application processor registers (v0.4), R/G are in LUT, 
>>> B is in LUT1 for 10bit and 12bit for MT8195. Can you check please to be sure ?
>>>
>>
>> That's right, but here I'm implying that 10-bit LUT is only for older SoCs, and
>> all of them have got the same register layout with one LUT register for R, G, B,
>> while all the new SoCs, which have got 12-bits LUT support, have got the new
>> register layout with two LUT registers (and multiple banks).
>> Infact, the MT8195 SoC was added here with 12-bits LUT support only (as the LUT
>> parameters extraction is easily handled by the drm_color_lut_extract() function).
>>
>> The alternative would've been to add two compatibles, like
>> "mediatek,mt8195-disp-gamma-10bits" and "mediatek,mt8195-disp-gamma-12bits",
>> or a boolean property like "mediatek,lut-12bits" which would appear literally
>> everywhere starting from a certain point in time (since there's no reason to
>> use 10-bits LUT on MT8195, that starts now!).
>>
>> Even then, consider the complication in code, where mtk_gamma_set_common()
>> would have to handle:
>> - 10-bits, layout A
>> - 10-bits, layout B -> but fallback to layout A if this is AAL
>> - 12-bits layout
>>
>> is_aal = !(gamma && gamma->data);
>>
>> for_each_bank()
>> {
>>      if (num_lut_banks > 1) write_num_bank();
>>
>>      for (i = 0; i < lut_bank_size; i++) {
>>          .......
>>
>>          if (!lut_diff || (i % 2 == 0)) {
>>              if (lut_bits == 12 || (lut_bits == 10 && layout_b)) {
>>                  ... setup word[0],[1] ...
>>              } else if (layout_b && !is_aal) {
>>                  ...setup word[0],[1]...
>>              } else {
>>                  ...setup word[0]
>>              }
>>          } else {
>>               ^^^ almost repeat the same ^^^
>>          }
>>          writel(word[0], (...));
>>          if (lut_bits == 12 || (lut_bits == 10 && layout_b) && !is_aal)
>>              writel(word[i] (....));
>>      }
>> }
>>
>> probe() {
>>      if (of_property_read_bool(dev->of_node, "mediatek,lut-12bits") ||
>>          data->supports_only_12bits)
>>          priv->lut_bits = 12;
>>      else
>>          priv->lut_bits = 10;
>> }
>>
>> ...at least, that's the implementation that I would do to solve your concern,
>> which isn't *too bad*, but still, a big question arises here...
>>
>>
>> Why should we care about supporting *both* 10-bit and 12-bit Gamma LUTs on
>> the *same* SoC?
>>
>>
>> A 12-bit LUT gives us more precision and there's no penalty if we want to
>> convert a 10-bit LUT to a 12-bits one, as we're simply "ignoring" the value
>> of two bits per component (no expensive calculation involved)...
>>
>> Is there anything that I'm underestimating here?
> 
> Thanks for you explanation !
> I think your choice is not bad, but it's not clear that MT8195 10 bit LUT isn't 
> supported at all.
> So, IMHO, the first solution is to support it like you explained it above, and the 
> second solution is to add comment somewhere to clarify that driver doesn't support 
> 10 bit LUT if the SoC is able to use 12 bit LUT, like MT8195 10 bit.
> 
> Is that relevant ? :D
> 

Even though the same as whhat I'm doing here was already done before, as the
current 10-bits LUT support ignores 9-bits LUT support, I can add a comment to
the code:

/*
  * SoCs supporting 12-bits LUTs are using a new register layout that does
  * always support (by HW) both 12-bits and 10-bits LUT but, on those, we
  * ignore the support for 10-bits in this driver and always use 12-bits.
  *
  * Summarizing:
  * - SoC HW support 9/10-bits LUT only
  *   - Old register layout
  *     - 10-bits LUT supported
  *     - 9-bits LUT not supported
  * - SoC HW support both 10/12bits LUT
  *   - New register layout
  *    - 12-bits LUT supported
  *    - 10-its LUT not supported
  */

Where the SoCs supporting 9-bits and 10-bits: mt6795, 8173, 8192,others and
12-bits are 8195, 8186, others.. of course.

Would that work for you?

Regards,
Angelo

Re: [PATCH RESEND v6 09/11] drm/mediatek: gamma: Add support for 12-bit LUT and MT8195

[Alexandre Mergnat]

On 31/07/2023 12:27, AngeloGioacchino Del Regno wrote:
> Il 28/07/23 14:58, Alexandre Mergnat ha scritto:
>> Hi Angelo
>>
>> On 27/07/2023 15:06, AngeloGioacchino Del Regno wrote:
>>>>> +/* For 10 bit LUT layout, R/G/B are in the same register */
>>>>>   #define DISP_GAMMA_LUT_10BIT_R            GENMASK(29, 20)
>>>>>   #define DISP_GAMMA_LUT_10BIT_G            GENMASK(19, 10)
>>>>>   #define DISP_GAMMA_LUT_10BIT_B            GENMASK(9, 0)
>>>>> +/* For 12 bit LUT layout, R/G are in LUT, B is in LUT1 */
>>>>
>>>> As I understood from the application processor registers (v0.4), R/G 
>>>> are in LUT, B is in LUT1 for 10bit and 12bit for MT8195. Can you 
>>>> check please to be sure ?
>>>>
>>>
>>> That's right, but here I'm implying that 10-bit LUT is only for older 
>>> SoCs, and
>>> all of them have got the same register layout with one LUT register 
>>> for R, G, B,
>>> while all the new SoCs, which have got 12-bits LUT support, have got 
>>> the new
>>> register layout with two LUT registers (and multiple banks).
>>> Infact, the MT8195 SoC was added here with 12-bits LUT support only 
>>> (as the LUT
>>> parameters extraction is easily handled by the 
>>> drm_color_lut_extract() function).
>>>
>>> The alternative would've been to add two compatibles, like
>>> "mediatek,mt8195-disp-gamma-10bits" and 
>>> "mediatek,mt8195-disp-gamma-12bits",
>>> or a boolean property like "mediatek,lut-12bits" which would appear 
>>> literally
>>> everywhere starting from a certain point in time (since there's no 
>>> reason to
>>> use 10-bits LUT on MT8195, that starts now!).
>>>
>>> Even then, consider the complication in code, where 
>>> mtk_gamma_set_common()
>>> would have to handle:
>>> - 10-bits, layout A
>>> - 10-bits, layout B -> but fallback to layout A if this is AAL
>>> - 12-bits layout
>>>
>>> is_aal = !(gamma && gamma->data);
>>>
>>> for_each_bank()
>>> {
>>>      if (num_lut_banks > 1) write_num_bank();
>>>
>>>      for (i = 0; i < lut_bank_size; i++) {
>>>          .......
>>>
>>>          if (!lut_diff || (i % 2 == 0)) {
>>>              if (lut_bits == 12 || (lut_bits == 10 && layout_b)) {
>>>                  ... setup word[0],[1] ...
>>>              } else if (layout_b && !is_aal) {
>>>                  ...setup word[0],[1]...
>>>              } else {
>>>                  ...setup word[0]
>>>              }
>>>          } else {
>>>               ^^^ almost repeat the same ^^^
>>>          }
>>>          writel(word[0], (...));
>>>          if (lut_bits == 12 || (lut_bits == 10 && layout_b) && !is_aal)
>>>              writel(word[i] (....));
>>>      }
>>> }
>>>
>>> probe() {
>>>      if (of_property_read_bool(dev->of_node, "mediatek,lut-12bits") ||
>>>          data->supports_only_12bits)
>>>          priv->lut_bits = 12;
>>>      else
>>>          priv->lut_bits = 10;
>>> }
>>>
>>> ...at least, that's the implementation that I would do to solve your 
>>> concern,
>>> which isn't *too bad*, but still, a big question arises here...
>>>
>>>
>>> Why should we care about supporting *both* 10-bit and 12-bit Gamma 
>>> LUTs on
>>> the *same* SoC?
>>>
>>>
>>> A 12-bit LUT gives us more precision and there's no penalty if we 
>>> want to
>>> convert a 10-bit LUT to a 12-bits one, as we're simply "ignoring" the 
>>> value
>>> of two bits per component (no expensive calculation involved)...
>>>
>>> Is there anything that I'm underestimating here?
>>
>> Thanks for you explanation !
>> I think your choice is not bad, but it's not clear that MT8195 10 bit 
>> LUT isn't supported at all.
>> So, IMHO, the first solution is to support it like you explained it 
>> above, and the second solution is to add comment somewhere to clarify 
>> that driver doesn't support 10 bit LUT if the SoC is able to use 12 
>> bit LUT, like MT8195 10 bit.
>>
>> Is that relevant ? :D
>>
> 
> Even though the same as whhat I'm doing here was already done before, as 
> the
> current 10-bits LUT support ignores 9-bits LUT support, I can add a 
> comment to
> the code:
> 
> /*
>   * SoCs supporting 12-bits LUTs are using a new register layout that does
>   * always support (by HW) both 12-bits and 10-bits LUT but, on those, we
>   * ignore the support for 10-bits in this driver and always use 12-bits.
>   *
>   * Summarizing:
>   * - SoC HW support 9/10-bits LUT only
>   *   - Old register layout
>   *     - 10-bits LUT supported
>   *     - 9-bits LUT not supported
>   * - SoC HW support both 10/12bits LUT
>   *   - New register layout
>   *    - 12-bits LUT supported
>   *    - 10-its LUT not supported
>   */
> 
> Where the SoCs supporting 9-bits and 10-bits: mt6795, 8173, 8192,others and
> 12-bits are 8195, 8186, others.. of course.
> 
> Would that work for you?

Sound good for me. After that:

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>

-- 
Regards,
Alexandre

[PATCH RESEND v6 10/11] drm/mediatek: gamma: Make sure relay mode is disabled

[AngeloGioacchino Del Regno]

Disable relay mode at the end of LUT programming to make sure that the
processed image goes through.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
index e0e2d2bdbf59..e9655b661364 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -20,6 +20,7 @@
 #define DISP_GAMMA_EN				0x0000
 #define GAMMA_EN					BIT(0)
 #define DISP_GAMMA_CFG				0x0020
+#define GAMMA_RELAY_MODE				BIT(0)
 #define GAMMA_LUT_EN					BIT(1)
 #define GAMMA_DITHERING					BIT(2)
 #define DISP_GAMMA_SIZE				0x0030
@@ -180,6 +181,9 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 	/* Enable the gamma table */
 	cfg_val |= FIELD_PREP(GAMMA_LUT_EN, 1);
 
+	/* Disable RELAY mode to pass the processed image */
+	cfg_val &= ~GAMMA_RELAY_MODE;
+
 	writel(cfg_val, regs + DISP_GAMMA_CFG);
 }
 
-- 
2.40.1

Re: [PATCH RESEND v6 10/11] drm/mediatek: gamma: Make sure relay mode is disabled

[Alexandre Mergnat]

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>

On 27/07/2023 11:46, AngeloGioacchino Del Regno wrote:
> Disable relay mode at the end of LUT programming to make sure that the
> processed image goes through.

-- 
Regards,
Alexandre

[PATCH RESEND v6 11/11] drm/mediatek: gamma: Program gamma LUT type for descending or rising

[AngeloGioacchino Del Regno]

All of the SoCs that don't have dithering control in the gamma IP
have got a GAMMA_LUT_TYPE bit that tells to the IP if the LUT is
"descending" (bit set) or "rising" (bit cleared): make sure to set
it correctly after programming the LUT.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
index e9655b661364..020755ae0ec0 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -23,6 +23,7 @@
 #define GAMMA_RELAY_MODE				BIT(0)
 #define GAMMA_LUT_EN					BIT(1)
 #define GAMMA_DITHERING					BIT(2)
+#define GAMMA_LUT_TYPE					BIT(2)
 #define DISP_GAMMA_SIZE				0x0030
 #define DISP_GAMMA_SIZE_HSIZE				GENMASK(28, 16)
 #define DISP_GAMMA_SIZE_VSIZE				GENMASK(12, 0)
@@ -89,6 +90,16 @@ unsigned int mtk_gamma_get_lut_size(struct device *dev)
 	return lut_size;
 }
 
+static bool mtk_gamma_lut_is_descending(struct drm_color_lut *lut, u32 lut_size)
+{
+	u64 first, last;
+
+	first = lut[0].red + lut[0].green + lut[0].blue;
+	last = lut[lut_size].red + lut[lut_size].green + lut[lut_size].blue;
+
+	return !!(first > last);
+}
+
 void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crtc_state *state)
 {
 	struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
@@ -178,6 +189,14 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
 		}
 	}
 
+	if (gamma && gamma->data && !gamma->data->has_dither) {
+		/* Descending or Rising LUT */
+		if (mtk_gamma_lut_is_descending(lut, lut_size))
+			cfg_val |= FIELD_PREP(GAMMA_LUT_TYPE, 1);
+		else
+			cfg_val &= ~GAMMA_LUT_TYPE;
+	}
+
 	/* Enable the gamma table */
 	cfg_val |= FIELD_PREP(GAMMA_LUT_EN, 1);
 
-- 
2.40.1

Re: [PATCH RESEND v6 11/11] drm/mediatek: gamma: Program gamma LUT type for descending or rising

[Alexandre Mergnat]

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>

On 27/07/2023 11:46, AngeloGioacchino Del Regno wrote:
> All of the SoCs that don't have dithering control in the gamma IP
> have got a GAMMA_LUT_TYPE bit that tells to the IP if the LUT is
> "descending" (bit set) or "rising" (bit cleared): make sure to set
> it correctly after programming the LUT.

-- 
Regards,
Alexandre