Re: [PATCH v2 2/2] media: arm: mali-c55: Add support for RGB Gamma

[Linus Walleij]

Hi Jacopo,

thanks for your patch!

On Tue, Jun 16, 2026 at 4:36 PM Jacopo Mondi
<jacopo.mondi@xxxxxxxxxxxxxxxx> wrote:

> From: Jacopo Mondi <jacopo.mondi+renesas@xxxxxxxxxxxxxxxx>
>
> Add support for Gamma curve correction for the Mali C55 ISP.
>
> Define a new block in the uAPI using the extensible v4l2-isp format and
> implement support for configuring the RGB Gamma parameters in the
> mali-c55 parameters handler.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@xxxxxxxxxxxxxxxx>
(...)

> +static void mali_c55_params_gamma(struct mali_c55 *mali_c55,
> +                                 union mali_c55_params_block block,
> +                                 __u32 offset, __u32 lut_base)
> +{
> +       const struct mali_c55_params_gamma *params = block.gamma;
> +
> +       if (block.header->flags & V4L2_ISP_PARAMS_FL_BLOCK_DISABLE) {
> +               mali_c55_ctx_update_bits(mali_c55,
> +                                        MALI_C55_REG_GAMMA_RGB_ENABLE + offset,
> +                                        MALI_C55_GAMMA_ENABLE_MASK, 0x00);
> +               return;
> +       }
> +
> +       mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_GAMMA_GAINS_1 + offset,
> +                                MALI_C55_GAMMA_GAIN_R_MASK, params->gains[0]);
> +       mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_GAMMA_GAINS_1 + offset,
> +                                MALI_C55_GAMMA_GAIN_G_MASK,
> +                                MALI_C55_GAMMA_GAIN_G(params->gains[1]));

It is a bit of confusion for the head when GAINS_1 is indexed to
gains[0] and gains[1] because the register i split.

> +       /* We cannot apply parameters to DS if it is not fitted. */
> +       if (!(mali_c55->capabilities & MALI_C55_GPS_DS_PIPE_FITTED))
> +               return;

I suppose this is a HW synthesis thin? Whether DS is fitted or not?
(Just curious.)

> @@ -425,11 +425,13 @@ enum mali_c55_interrupts {
>  #define MALI_C55_REG_GAMMA_GAINS_1                     0x1c068
>  #define MALI_C55_GAMMA_GAIN_R_MASK                     GENMASK(11, 0)
>  #define MALI_C55_GAMMA_GAIN_G_MASK                     GENMASK(27, 16)

Because of my confusion I would rename *GAINS_1
to *GAINS_RG..

> +#define MALI_C55_GAMMA_GAIN_G(x)                       ((x) << 16)
>  #define MALI_C55_REG_GAMMA_GAINS_2                     0x1c06c
>  #define MALI_C55_GAMMA_GAIN_B_MASK                     GENMASK(11, 0)

.. and *GAINS_2 to GAINS_B.

This would make it clear what the registers are for.

>  #define MALI_C55_REG_GAMMA_OFFSETS_1                   0x1c070
>  #define MALI_C55_GAMMA_OFFSET_R_MASK                   GENMASK(11, 0)
>  #define MALI_C55_GAMMA_OFFSET_G_MASK                   GENMASK(27, 16)

Same here *GAMMA_OFFSETS_RG

> +#define MALI_C55_GAMMA_OFFSET_G(x)                             ((x) << 16)
>  #define MALI_C55_REG_GAMMA_OFFSETS_2                   0x1c074
>  #define MALI_C55_GAMMA_OFFSET_B_MASK                   GENMASK(11, 0)

Etc.

You might have good reasons for this naming that I don't understand
(like they are named like that in some documentation, I checked the
register map document but it doesn't seem to name the individual registers
but call them as a group "fr gamma rgb".
Maybe I'm looking in the wrong place.
so either way:

Reviewed-by: Linus Walleij <linusw@xxxxxxxxxx>

Yours,
Linus Walleij