Re: [PATCH 05/11] drm/mediatek: ovl: Implement support for Inverse Gamma

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

On Tue, 2025-12-23 at 16:44 -0300, Nícolas F. R. A. Prado wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> The OVL hardware allows selecting between different fixed transfer
> functions for each layer through the Inverse Gamma setting. Available
> functions are scRGB and BT.709. Implement support for it and expose it
> as a colorop through the DRM plane color pipeline uAPI.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---

[snip]

> 
>  #include <linux/clk.h>
>  #include <linux/component.h>
> @@ -47,6 +50,12 @@
>  #define OVL_CON_CLRFMT_BIT_DEPTH(depth, n)             ((depth) << (4 * (n)))
>  #define OVL_CON_CLRFMT_8_BIT                           (0)
>  #define OVL_CON_CLRFMT_10_BIT                          (1)
> +#define DISP_REG_OVL_WCG_CFG1                  0x2d8
> +#define IGAMMA_EN(layer)                               BIT(0 + 4 * (layer))

BIT(4 * (layer))

> +#define DISP_REG_OVL_WCG_CFG2                  0x2dc
> +#define IGAMMA_MASK(layer)                             GENMASK((layer) * 4 + 1, (layer) * 4)
> +#define IGAMMA_SCRGB                                   0
> +#define IGAMMA_BT709                                   1
>  #define DISP_REG_OVL_ADDR_MT8173               0x0f40
>  #define DISP_REG_OVL_ADDR(ovl, n)              ((ovl)->data->addr + 0x20 * (n))
>  #define DISP_REG_OVL_HDR_ADDR(ovl, n)          ((ovl)->data->addr + 0x20 * (n) + 0x04)
> @@ -492,6 +501,91 @@ static void mtk_ovl_afbc_layer_config(struct mtk_disp_ovl *ovl,
>         }
>  }
> 
> +static int mtk_ovl_colorop_curve_to_reg_val(enum drm_colorop_curve_1d_type curve)
> +{
> +       switch (curve) {
> +       case DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF:
> +               return IGAMMA_SCRGB;
> +       case DRM_COLOROP_1D_CURVE_BT2020_OETF:

I don't exactly know what DRM_COLOROP_1D_CURVE_BT2020_OETF mean, but there is DRM_COLOROP_1D_CURVE_BT2020_INV_OETF.
This patch is about inverse gamma, so this should be DRM_COLOROP_1D_CURVE_BT2020_INV_OETF?

> +               return IGAMMA_BT709;
> +       default:
> +               return -EINVAL;
> +       }
> +}
> +
> +static void mtk_ovl_apply_igamma(struct mtk_disp_ovl *ovl, unsigned int idx,
> +                                struct drm_colorop *colorop,
> +                                struct cmdq_pkt *cmdq_pkt)
> +{
> +       int curve_reg_val;
> +
> +       if (colorop->state->bypass) {
> +               /* igamma curve needs to be set to default when igamma is disabled */
> +               curve_reg_val = IGAMMA_SCRGB;

I would like to set igamma curve after disable.
So

if (colorop->state->bypass) {
	mtk_ovl_disable_colorops();
	return;
}
	

> +       } else {
> +               curve_reg_val = mtk_ovl_colorop_curve_to_reg_val(colorop->state->curve_1d_type);
> +               if (curve_reg_val < 0) {
> +                       drm_WARN(ovl->crtc->dev, 1,
> +                                "Invalid curve 1d type %u\n",
> +                                colorop->state->curve_1d_type);
> +                       return;
> +               }
> +       }
> +
> +       mtk_ddp_write_mask(cmdq_pkt,
> +                          field_prep(IGAMMA_MASK(idx), curve_reg_val),
> +                          &ovl->cmdq_reg, ovl->regs, DISP_REG_OVL_WCG_CFG2,
> +                          IGAMMA_MASK(idx));
> +
> +       mtk_ddp_write_mask(cmdq_pkt,
> +                          colorop->state->bypass ? 0 : IGAMMA_EN(idx),
> +                          &ovl->cmdq_reg, ovl->regs, DISP_REG_OVL_WCG_CFG1,
> +                          IGAMMA_EN(idx));
> +}
> +

[snip]

> 
> +static const struct drm_colorop_funcs mtk_ovl_colorop_funcs = {
> +       .destroy = drm_colorop_destroy,
> +};
> +
> +static const u64 igamma_supported_tfs =
> +       BIT(DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF) |
> +       BIT(DRM_COLOROP_1D_CURVE_BT2020_OETF);
> +
> +#define MAX_COLOR_PIPELINE_OPS 3

Why is 3? Is this hardware limitation?
Provide some information about this value,
so we know it's possible to change it or not.

> +
> +static int
> +mtk_ovl_initialize_plane_color_pipeline(struct drm_plane *plane,
> +                                       struct drm_prop_enum_list *pipeline)
> +{
> +       struct drm_colorop *ops[MAX_COLOR_PIPELINE_OPS];
> +       struct drm_device *dev = plane->dev;
> +       int i = 0;
> +       int ret;
> +
> +       memset(ops, 0, sizeof(ops));
> +
> +       /* 1st op: OVL's Inverse Gamma */
> +       ops[i] = kzalloc(sizeof(*ops[i]), GFP_KERNEL);
> +       if (!ops[i]) {
> +               ret = -ENOMEM;
> +               goto err_alloc;
> +       }
> +
> +       ret = drm_plane_colorop_curve_1d_init(dev, ops[i], plane,
> +                                             &mtk_ovl_colorop_funcs,
> +                                             igamma_supported_tfs,
> +                                             DRM_COLOROP_FLAG_ALLOW_BYPASS);

If other sub driver (maybe ovl adapter) also has inverse gamma,
most part of this function looks common and only mtk_ovl_colorop_funcs and igamma_supported_tfs are sub driver specific.
So move the common part to mtk_crtc.c and sub driver provide function to query mtk_ovl_colorop_funcs and igamma_supported_tfs.
(mtk_ovl_colorop_funcs looks not a sub driver specific function, maybe it could be common part)

Regards,
CK

> +       if (ret)
> +               goto err_colorop_init;
> +
> +       pipeline->type = ops[0]->base.id;
> +       pipeline->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", ops[0]->base.id);
> +
> +       return 0;
> +
> +err_colorop_init:
> +       kfree(ops[i]);
> +
> +err_alloc:
> +       i--;
> +       for (; i >= 0; i--) {
> +               drm_colorop_cleanup(ops[i]);
> +               kfree(ops[i]);
> +       }
> +
> +       return ret;
> +}
> +
>