Changes in v10:
- Moved snippet from patch [7/15] to patch [6/15] as that was
intended to be there instead; fixes build issue for patch [6/15]
as pointed out by the kernel text robot (oops, sorry!)
Changes in v9:
- As per previous conversation with CK Hu, added a commit that
de-commonizes the gamma setting function that was used in
both DISP_AAL and DISP_GAMMA, now each of them have their
own .gamma_set() callback (mtk_disp_gamma_set_common() has
been removed).
- Added a change to use bitfield macros in mtk_disp_aal.c
- Added a change to compress of_device_id entries in mtk_disp_aal.c
- Tested again on MT6795, MT8173, MT8186, MT8192, MT8195
Changes in v8:
- Changed lut_size to be a mtk_disp_gamma_set_common() function
parameter to pass lut size from AAL
Changes in v7:
- Added check for NULL dev for AAL-gamma case
- Added get_lut_size callback for AAL-gamma
- Added comment to clarify SoC 10/12 bits support and old vs new
register layout as suggested by Alexandre M.
Changes in v6:
- Fixed smatch warning in patch 11/11, ref.:
https://lore.kernel.org/all/[email protected]/
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 (15):
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: aal: Use bitfield macros
drm/mediatek: De-commonize disp_aal/disp_gamma gamma_set functions
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
drm/mediatek: aal: Rewrite kerneldoc for struct mtk_disp_aal
drm/mediatek: gamma: Add kerneldoc for struct mtk_disp_gamma
drm/mediatek: aal: Compress of_device_id entries and add sentinel
Jason-JH.Lin (1):
drm/mediatek: gamma: Adjust mtk_drm_gamma_set_common parameters
drivers/gpu/drm/mediatek/mtk_disp_aal.c | 84 ++++++--
drivers/gpu/drm/mediatek/mtk_disp_drv.h | 3 +-
drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 203 ++++++++++++++++----
drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 8 +-
drivers/gpu/drm/mediatek/mtk_drm_crtc.h | 1 -
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 2 +
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 9 +
7 files changed, 256 insertions(+), 54 deletions(-)
--
2.41.0
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 <[email protected]>
Reviewed-by: Jason-JH.Lin <[email protected]>
Reviewed-by: Alexandre Mergnat <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
index fbff9f97b737..d9a70238d524 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -22,6 +22,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)
@@ -86,6 +87,17 @@ unsigned int mtk_gamma_get_lut_size(struct device *dev)
return LUT_SIZE_DEFAULT;
}
+static bool mtk_gamma_lut_is_descending(struct drm_color_lut *lut, u32 lut_size)
+{
+ u64 first, last;
+ int last_entry = lut_size - 1;
+
+ first = lut[0].red + lut[0].green + lut[0].blue;
+ last = lut[last_entry].red + lut[last_entry].green + lut[last_entry].blue;
+
+ return !!(first > last);
+}
+
/*
* 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
@@ -175,6 +187,14 @@ void mtk_gamma_set(struct device *dev, struct drm_crtc_state *state)
}
}
+ if (!gamma->data->has_dither) {
+ /* Descending or Rising LUT */
+ if (mtk_gamma_lut_is_descending(lut, gamma->data->lut_size - 1))
+ 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.41.0
The kerneldoc for struct mtk_disp_aal was entirely wrong: rewrite it
to actually document the structure.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_disp_aal.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_aal.c b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
index 992dc1424c91..e6ab3eaa1126 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
@@ -36,9 +36,11 @@ struct mtk_disp_aal_data {
};
/**
- * struct mtk_disp_aal - DISP_AAL driver structure
- * @ddp_comp - structure containing type enum and hardware resources
- * @crtc - associated crtc to report irq events to
+ * struct mtk_disp_aal - Display Adaptive Ambient Light driver structure
+ * @clk: clock for DISP_AAL controller
+ * @regs: MMIO registers base
+ * @cmdq_reg: CMDQ Client register
+ * @data: platform specific data for DISP_AAL
*/
struct mtk_disp_aal {
struct clk *clk;
--
2.41.0
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 <[email protected]>
Reviewed-by: Jason-JH.Lin <[email protected]>
Reviewed-by: Alexandre Mergnat <[email protected]>
---
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 d42cc0698d83..47751864bd5c 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -64,6 +64,10 @@ 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 we're called from AAL, dev is NULL */
gamma = dev ? dev_get_drvdata(dev) : NULL;
@@ -72,29 +76,26 @@ void mtk_gamma_set_common(struct device *dev, void __iomem *regs, struct drm_crt
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.41.0
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 <[email protected]>
[Angelo: Refactored original commit]
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
Reviewed-by: Alexandre Mergnat <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 70 ++++++++++++++---------
1 file changed, 44 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
index 1845bd326a6d..3f1c6815ea5a 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -24,6 +24,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)
@@ -37,6 +39,7 @@
struct mtk_disp_gamma_data {
bool has_dither;
bool lut_diff;
+ u16 lut_bank_size;
u16 lut_size;
u8 lut_bits;
};
@@ -80,41 +83,54 @@ void mtk_gamma_set(struct device *dev, struct drm_crtc_state *state)
unsigned int i;
struct drm_color_lut *lut;
void __iomem *lut_base;
- 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)
return;
+ num_lut_banks = gamma->data->lut_size / gamma->data->lut_bank_size;
cfg_val = readl(gamma->regs + DISP_GAMMA_CFG);
lut_base = gamma->regs + DISP_GAMMA_LUT;
lut = (struct drm_color_lut *)state->gamma_lut->data;
- for (i = 0; i < gamma->data->lut_size; i++) {
- struct drm_color_lut diff, hwlut;
-
- hwlut.red = drm_color_lut_extract(lut[i].red, gamma->data->lut_bits);
- hwlut.green = drm_color_lut_extract(lut[i].green, gamma->data->lut_bits);
- hwlut.blue = drm_color_lut_extract(lut[i].blue, gamma->data->lut_bits);
-
- if (!gamma->data->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, gamma->data->lut_bits);
-
- diff.green = lut[i].green - lut[i - 1].green;
- diff.green = drm_color_lut_extract(diff.green, gamma->data->lut_bits);
-
- diff.blue = lut[i].blue - lut[i - 1].blue;
- diff.blue = drm_color_lut_extract(diff.blue, gamma->data->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, gamma->regs + DISP_GAMMA_BANK);
+ }
+
+ for (i = 0; i < gamma->data->lut_bank_size; i++) {
+ int n = (cur_bank * gamma->data->lut_bank_size) + i;
+ struct drm_color_lut diff, hwlut;
+
+ hwlut.red = drm_color_lut_extract(lut[n].red, gamma->data->lut_bits);
+ hwlut.green = drm_color_lut_extract(lut[n].green, gamma->data->lut_bits);
+ hwlut.blue = drm_color_lut_extract(lut[n].blue, gamma->data->lut_bits);
+
+ if (!gamma->data->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, gamma->data->lut_bits);
+
+ diff.green = lut[n].green - lut[n - 1].green;
+ diff.green = drm_color_lut_extract(diff.green, gamma->data->lut_bits);
+
+ diff.blue = lut[n].blue - lut[n - 1].blue;
+ diff.blue = drm_color_lut_extract(diff.blue, gamma->data->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 */
@@ -218,11 +234,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.41.0
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 <[email protected]>
Reviewed-by: Jason-JH.Lin <[email protected]>
Reviewed-by: Alexandre Mergnat <[email protected]>
---
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 fd6a75a64a9f..18b102bef370 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -68,12 +68,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;
- 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.41.0
Compress the entry for mediatek,mt8173-disp-aal, as it fits in one
line, and fix the style; while at it, also add the usual sentinel
comment to the last entry.
This commit brings no functional changes.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_disp_aal.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_aal.c b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
index e6ab3eaa1126..70de5f3007e4 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
@@ -212,10 +212,9 @@ static const struct mtk_disp_aal_data mt8173_aal_driver_data = {
};
static const struct of_device_id mtk_disp_aal_driver_dt_match[] = {
- { .compatible = "mediatek,mt8173-disp-aal",
- .data = &mt8173_aal_driver_data},
- { .compatible = "mediatek,mt8183-disp-aal"},
- {},
+ { .compatible = "mediatek,mt8173-disp-aal", .data = &mt8173_aal_driver_data },
+ { .compatible = "mediatek,mt8183-disp-aal" },
+ { /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, mtk_disp_aal_driver_dt_match);
--
2.41.0
Hi, Angelo:
On Fri, 2023-08-04 at 09:28 +0200, AngeloGioacchino Del Regno wrote:
> Compress the entry for mediatek,mt8173-disp-aal, as it fits in one
> line, and fix the style; while at it, also add the usual sentinel
> comment to the last entry.
>
> This commit brings no functional changes.
Reviewed-by: CK Hu <[email protected]>
>
> Signed-off-by: AngeloGioacchino Del Regno <
> [email protected]>
> ---
> drivers/gpu/drm/mediatek/mtk_disp_aal.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> index e6ab3eaa1126..70de5f3007e4 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> @@ -212,10 +212,9 @@ static const struct mtk_disp_aal_data
> mt8173_aal_driver_data = {
> };
>
> static const struct of_device_id mtk_disp_aal_driver_dt_match[] = {
> - { .compatible = "mediatek,mt8173-disp-aal",
> - .data = &mt8173_aal_driver_data},
> - { .compatible = "mediatek,mt8183-disp-aal"},
> - {},
> + { .compatible = "mediatek,mt8173-disp-aal", .data =
> &mt8173_aal_driver_data },
> + { .compatible = "mediatek,mt8183-disp-aal" },
> + { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, mtk_disp_aal_driver_dt_match);
>
Hi, Angelo:
On Fri, 2023-08-04 at 09:28 +0200, AngeloGioacchino Del Regno wrote:
> The kerneldoc for struct mtk_disp_aal was entirely wrong: rewrite it
> to actually document the structure.
Reviewed-by: CK Hu <[email protected]>
>
> Signed-off-by: AngeloGioacchino Del Regno <
> [email protected]>
> ---
> drivers/gpu/drm/mediatek/mtk_disp_aal.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> index 992dc1424c91..e6ab3eaa1126 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> @@ -36,9 +36,11 @@ struct mtk_disp_aal_data {
> };
>
> /**
> - * struct mtk_disp_aal - DISP_AAL driver structure
> - * @ddp_comp - structure containing type enum and hardware resources
> - * @crtc - associated crtc to report irq events to
> + * struct mtk_disp_aal - Display Adaptive Ambient Light driver
> structure
> + * @clk: clock for DISP_AAL controller
> + * @regs: MMIO registers base
> + * @cmdq_reg: CMDQ Client register
> + * @data: platform specific data for DISP_AAL
> */
> struct mtk_disp_aal {
> struct clk *clk;
Il 04/08/23 09:28, AngeloGioacchino Del Regno ha scritto:
> Changes in v10:
> - Moved snippet from patch [7/15] to patch [6/15] as that was
> intended to be there instead; fixes build issue for patch [6/15]
> as pointed out by the kernel text robot (oops, sorry!)
>
Hello CK,
is there anything wrong about this series?
All commits do work on their own, there are no regressions as we tested this
on multiple Chromebooks (MT8173, MT8183, MT8192, MT8195) and a MT6795 Helio
X10 smartphone, and kept testing for months now.
Can you please pick it?
Thanks,
Angelo
> Changes in v9:
> - As per previous conversation with CK Hu, added a commit that
> de-commonizes the gamma setting function that was used in
> both DISP_AAL and DISP_GAMMA, now each of them have their
> own .gamma_set() callback (mtk_disp_gamma_set_common() has
> been removed).
> - Added a change to use bitfield macros in mtk_disp_aal.c
> - Added a change to compress of_device_id entries in mtk_disp_aal.c
> - Tested again on MT6795, MT8173, MT8186, MT8192, MT8195
>
> Changes in v8:
> - Changed lut_size to be a mtk_disp_gamma_set_common() function
> parameter to pass lut size from AAL
>
> Changes in v7:
> - Added check for NULL dev for AAL-gamma case
> - Added get_lut_size callback for AAL-gamma
> - Added comment to clarify SoC 10/12 bits support and old vs new
> register layout as suggested by Alexandre M.
>
> Changes in v6:
> - Fixed smatch warning in patch 11/11, ref.:
> https://lore.kernel.org/all/[email protected]/
>
> 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 (15):
> 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: aal: Use bitfield macros
> drm/mediatek: De-commonize disp_aal/disp_gamma gamma_set functions
> 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
> drm/mediatek: aal: Rewrite kerneldoc for struct mtk_disp_aal
> drm/mediatek: gamma: Add kerneldoc for struct mtk_disp_gamma
> drm/mediatek: aal: Compress of_device_id entries and add sentinel
>
> Jason-JH.Lin (1):
> drm/mediatek: gamma: Adjust mtk_drm_gamma_set_common parameters
>
> drivers/gpu/drm/mediatek/mtk_disp_aal.c | 84 ++++++--
> drivers/gpu/drm/mediatek/mtk_disp_drv.h | 3 +-
> drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 203 ++++++++++++++++----
> drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 8 +-
> drivers/gpu/drm/mediatek/mtk_drm_crtc.h | 1 -
> drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 2 +
> drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 9 +
> 7 files changed, 256 insertions(+), 54 deletions(-)
>
Hi, Angelo:
On Fri, 2023-08-04 at 09:28 +0200, 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.
>
> Signed-off-by: AngeloGioacchino Del Regno <
> [email protected]>
> Reviewed-by: Jason-JH.Lin <[email protected]>
> Reviewed-by: Alexandre Mergnat <[email protected]>
> ---
> drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> index fbff9f97b737..d9a70238d524 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> @@ -22,6 +22,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)
> @@ -86,6 +87,17 @@ unsigned int mtk_gamma_get_lut_size(struct device
> *dev)
> return LUT_SIZE_DEFAULT;
> }
>
> +static bool mtk_gamma_lut_is_descending(struct drm_color_lut *lut,
> u32 lut_size)
> +{
> + u64 first, last;
> + int last_entry = lut_size - 1;
> +
> + first = lut[0].red + lut[0].green + lut[0].blue;
> + last = lut[last_entry].red + lut[last_entry].green +
> lut[last_entry].blue;
> +
> + return !!(first > last);
> +}
> +
> /*
> * 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
> @@ -175,6 +187,14 @@ void mtk_gamma_set(struct device *dev, struct
> drm_crtc_state *state)
> }
> }
>
> + if (!gamma->data->has_dither) {
> + /* Descending or Rising LUT */
> + if (mtk_gamma_lut_is_descending(lut, gamma->data-
> >lut_size - 1))
> + cfg_val |= FIELD_PREP(GAMMA_LUT_TYPE, 1);
Reviewed-by: CK Hu <[email protected]>
But I'm not sure why not write it more simply as
cfg_val |= GAMMA_LUT_TYPE;
Regards,
CK
> + else
> + cfg_val &= ~GAMMA_LUT_TYPE;
> + }
> +
> /* Enable the gamma table */
> cfg_val |= FIELD_PREP(GAMMA_LUT_EN, 1);
>
Hi, Angelo:
On Fri, 2023-08-04 at 09:28 +0200, 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.
>
> Suggested-by: Jason-JH.Lin <[email protected]>
> [Angelo: Refactored original commit]
> Signed-off-by: AngeloGioacchino Del Regno <
> [email protected]>
> Reviewed-by: Alexandre Mergnat <[email protected]>
> ---
> drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 70 ++++++++++++++-------
> --
> 1 file changed, 44 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> index 1845bd326a6d..3f1c6815ea5a 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> @@ -24,6 +24,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)
> @@ -37,6 +39,7 @@
> struct mtk_disp_gamma_data {
> bool has_dither;
> bool lut_diff;
> + u16 lut_bank_size;
> u16 lut_size;
> u8 lut_bits;
> };
> @@ -80,41 +83,54 @@ void mtk_gamma_set(struct device *dev, struct
> drm_crtc_state *state)
> unsigned int i;
> struct drm_color_lut *lut;
> void __iomem *lut_base;
> - 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)
> return;
>
> + num_lut_banks = gamma->data->lut_size / gamma->data-
> >lut_bank_size;
> cfg_val = readl(gamma->regs + DISP_GAMMA_CFG);
> lut_base = gamma->regs + DISP_GAMMA_LUT;
> lut = (struct drm_color_lut *)state->gamma_lut->data;
> - for (i = 0; i < gamma->data->lut_size; i++) {
> - struct drm_color_lut diff, hwlut;
> -
> - hwlut.red = drm_color_lut_extract(lut[i].red, gamma-
> >data->lut_bits);
> - hwlut.green = drm_color_lut_extract(lut[i].green,
> gamma->data->lut_bits);
> - hwlut.blue = drm_color_lut_extract(lut[i].blue, gamma-
> >data->lut_bits);
> -
> - if (!gamma->data->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,
> gamma->data->lut_bits);
> -
> - diff.green = lut[i].green - lut[i - 1].green;
> - diff.green = drm_color_lut_extract(diff.green,
> gamma->data->lut_bits);
> -
> - diff.blue = lut[i].blue - lut[i - 1].blue;
> - diff.blue = drm_color_lut_extract(diff.blue,
> gamma->data->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, gamma->regs +
> DISP_GAMMA_BANK);
> + }
> +
> + for (i = 0; i < gamma->data->lut_bank_size; i++) {
> + int n = (cur_bank * gamma->data->lut_bank_size)
> + i;
int n = cur_bank * gamma->data->lut_bank_size + i;
After this modification,
Reviewed-by: CK Hu <[email protected]>
> + struct drm_color_lut diff, hwlut;
> +
> + hwlut.red = drm_color_lut_extract(lut[n].red,
> gamma->data->lut_bits);
> + hwlut.green =
> drm_color_lut_extract(lut[n].green, gamma->data->lut_bits);
> + hwlut.blue = drm_color_lut_extract(lut[n].blue,
> gamma->data->lut_bits);
> +
> + if (!gamma->data->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, gamma->data->lut_bits);
> +
> + diff.green = lut[n].green - lut[n -
> 1].green;
> + diff.green =
> drm_color_lut_extract(diff.green, gamma->data->lut_bits);
> +
> + diff.blue = lut[n].blue - lut[n -
> 1].blue;
> + diff.blue =
> drm_color_lut_extract(diff.blue, gamma->data->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 */
> @@ -218,11 +234,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,
Hi, Angelo:
On Fri, 2023-08-04 at 09:28 +0200, 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.
>
> Signed-off-by: AngeloGioacchino Del Regno <
> [email protected]>
> Reviewed-by: Jason-JH.Lin <[email protected]>
> Reviewed-by: Alexandre Mergnat <[email protected]>
> ---
> 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 fd6a75a64a9f..18b102bef370 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> @@ -68,12 +68,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;
> - 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);
Move this to bottom of this function. Move here in the patch which
need.
After this modification,
Reviewed-by: CK Hu <[email protected]>
> 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)
Hi, Angelo:
On Fri, 2023-08-04 at 09:28 +0200, 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.
Reviewed-by: CK Hu <[email protected]>
>
> Signed-off-by: AngeloGioacchino Del Regno <
> [email protected]>
> Reviewed-by: Jason-JH.Lin <[email protected]>
> Reviewed-by: Alexandre Mergnat <[email protected]>
> ---
> 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 d42cc0698d83..47751864bd5c 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> @@ -64,6 +64,10 @@ 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 we're called from AAL, dev is NULL */
> gamma = dev ? dev_get_drvdata(dev) : NULL;
>
> @@ -72,29 +76,26 @@ void mtk_gamma_set_common(struct device *dev,
> void __iomem *regs, struct drm_crt
> 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));
> }
> }
>