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

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Linus,
  thanks for the review

On Fri, Jun 26, 2026 at 12:53:45AM +0200, Linus Walleij wrote:
> Hi Jacopo,
>
> thanks for your patch!
>
> On Tue, Jun 16, 2026 at 4:36 PM Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
>
> > From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> >
> > 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@ideasonboard.com>
> (...)
>
> > +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.
>

See below

> > +       /* 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.)

I presume so ?

>
> > @@ -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.
>

I can certainly do so

> >  #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

Aren't the R and G masks different ?

Or are you suggesting
#define MALI_C55_REG_GAMMA_OFFSETS_RG                   0x1c070

>
> > +#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

I think there register names were already there before this patch
didn't they ?

> (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@kernel.org>

Thanks!

>
> Yours,
> Linus Walleij
>