<pre>
Hi, Angelo:

On Fri, 2023-08-04 at 09:28 +0200, AngeloGioacchino Del Regno wrote:
> In preparation for adding a 12-bits gamma support for the DISP_GAMMA
> IP, remove the mtk_gamma_set_common() function and move the relevant
> bits in mtk_gamma_set() for DISP_GAMMA and mtk_aal_gamma_set() for
> DISP_AAL: since the latter has no more support for gamma manipulation
> (being moved to a different IP) in newer revisions, those functions
> are about to diverge and it makes no sense to keep a common one (with
> all the complications of passing common data and making exclusions
> for device driver data) for just a few bits.
>
> This commit brings no functional changes.
>
> Signed-off-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> ---
> drivers/gpu/drm/mediatek/mtk_disp_aal.c | 39
> +++++++++++++++++++++--
> drivers/gpu/drm/mediatek/mtk_disp_drv.h | 1 -
> drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 34 ++++----------------
> 3 files changed, 44 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> index bec035780db0..21b25470e9b7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
> @@ -17,10 +17,18 @@
>
> #define DISP_AAL_EN0x0000
> #define AAL_ENBIT(0)
> +#define DISP_AAL_CFG0x0020
> +#define AAL_RELAY_MODEBIT(0)

You does not use this bit in this patch, so move it to [12/16].

> +#define AAL_GAMMA_LUT_ENBIT(1)

You should enable this bit.

> #define DISP_AAL_SIZE0x0030
> #define DISP_AAL_SIZE_HSIZEGENMASK(28, 16)
> #define DISP_AAL_SIZE_VSIZEGENMASK(12, 0)
> #define DISP_AAL_OUTPUT_SIZE0x04d8
> +#define DISP_AAL_GAMMA_LUT0x0700
> +#define DISP_AAL_GAMMA_LUT_RGENMASK(29, 20)
> +#define DISP_AAL_GAMMA_LUT_GGENMASK(19, 10)
> +#define DISP_AAL_GAMMA_LUT_BGENMASK(9, 0)
> +#define DISP_AAL_LUT_BITS10
> #define DISP_AAL_LUT_SIZE512
>
> struct mtk_disp_aal_data {
> @@ -85,9 +93,36 @@ unsigned int mtk_aal_gamma_get_lut_size(struct
> device *dev)
> void mtk_aal_gamma_set(struct device *dev, struct drm_crtc_state
> *state)
> {
> struct mtk_disp_aal *aal = dev_get_drvdata(dev);
> +struct drm_color_lut *lut;
> +unsigned int i;
> +u32 cfg_val;
> +
> +/* If gamma is not supported in AAL, go out immediately */
> +if (!(aal->data && aal->data->has_gamma))
> +return;
> +
> +/* Also, if there's no gamma lut there's nothing to do here. */
> +if (!state->gamma_lut)
> +return;
> +
> +cfg_val = readl(aal->regs + DISP_AAL_CFG);
> +lut = (struct drm_color_lut *)state->gamma_lut->data;
> +
> +for (i = 0; i < DISP_AAL_LUT_SIZE; i++) {
> +struct drm_color_lut hwlut = {
> +.red = drm_color_lut_extract(lut[i].red,
> DISP_AAL_LUT_BITS),
> +.green = drm_color_lut_extract(lut[i].green,
> DISP_AAL_LUT_BITS),
> +.blue = drm_color_lut_extract(lut[i].blue,
> DISP_AAL_LUT_BITS)
> +};
> +u32 word;
> +
> +word = FIELD_PREP(DISP_AAL_GAMMA_LUT_R, hwlut.red);
> +word |= FIELD_PREP(DISP_AAL_GAMMA_LUT_G, hwlut.green);
> +word |= FIELD_PREP(DISP_AAL_GAMMA_LUT_B, hwlut.blue);
> +writel(word, (aal->regs + DISP_AAL_GAMMA_LUT) + (i *
> 4));

This could be more simple:

writel(word, aal->regs + DISP_AAL_GAMMA_LUT + i * 4);

Regards,
CK

> +}
>
> -if (aal->data && aal->data->has_gamma)
> -mtk_gamma_set_common(NULL, aal->regs, state);
> +writel(cfg_val, aal->regs + DISP_AAL_CFG);
> }
>
> 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 ca377265e5eb..54d3712e2afd 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -56,7 +56,6 @@ void mtk_gamma_config(struct device *dev, unsigned
> int w,
> 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);
> 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 ea91d3619716..001b98694761 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> @@ -73,42 +73,29 @@ unsigned int mtk_gamma_get_lut_size(struct device
> *dev)
> return LUT_SIZE_DEFAULT;
> }
>
> -void mtk_gamma_set_common(struct device *dev, void __iomem *regs,
> struct drm_crtc_state *state)
> +void mtk_gamma_set(struct device *dev, struct drm_crtc_state *state)
> {
> -struct mtk_disp_gamma *gamma;
> +struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> unsigned int i;
> struct drm_color_lut *lut;
> void __iomem *lut_base;
> -bool lut_diff;
> -u16 lut_size;
> u32 cfg_val, word;
>
> /* 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;
> -
> -if (gamma && gamma->data) {
> -lut_diff = gamma->data->lut_diff;
> -lut_size = gamma->data->lut_size;
> -} else {
> -lut_diff = false;
> -lut_size = LUT_SIZE_DEFAULT;
> -}
> -
> -cfg_val = readl(regs + DISP_GAMMA_CFG);
> -lut_base = regs + DISP_GAMMA_LUT;
> +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 < lut_size; i++) {
> +for (i = 0; i < gamma->data->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);
>
> -if (!lut_diff || (i % 2 == 0)) {
> +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);
> @@ -132,14 +119,7 @@ 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);
>
> -writel(cfg_val, regs + DISP_GAMMA_CFG);
> -}
> -
> -void mtk_gamma_set(struct device *dev, struct drm_crtc_state *state)
> -{
> -struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> -
> -mtk_gamma_set_common(dev, gamma->regs, state);
> +writel(cfg_val, gamma->regs + DISP_GAMMA_CFG);
> }
>
> void mtk_gamma_config(struct device *dev, unsigned int w,

</pre><!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice
 ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe
 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
</pre><!--}-->