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

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

Hi Vincenzo

On Fri, Jun 26, 2026 at 10:52:38AM +0100, Vincenzo Frascino wrote:
> Hi Jacopo,
>
> thank you for your patch!
>
> On 16/06/2026 15:36, Jacopo Mondi 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>
>
> I have a couple of comments in addition to what Linus mentioned in his review.
> With this:
>
> Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
>
> >
> > ---
> > v2:
> > - Remove unused 'rgb_enable' member
> > - Address checkpatch issues
> > ---
> >  .../media/platform/arm/mali-c55/mali-c55-params.c  | 75 ++++++++++++++++++++++
> >  .../platform/arm/mali-c55/mali-c55-registers.h     |  5 ++
> >  include/uapi/linux/media/arm/mali-c55-config.h     | 45 ++++++++++++-
> >  3 files changed, 124 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-params.c b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > index 96f1b28a6d77..21f031aaf595 100644
> > --- a/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > @@ -47,6 +47,8 @@
> >   * @shading_config:	For header->type == MALI_C55_PARAM_MESH_SHADING_CONFIG
> >   * @shading_selection:	For header->type == MALI_C55_PARAM_MESH_SHADING_SELECTION
> >   * @ccm:		For header->type == MALI_C55_PARAM_BLOCK_CCM
> > + * @gamma:		For header->type == MALI_C55_PARAM_BLOCK_GAMMA_FR and
> > + *			header->type = MALI_C55_PARAM_BLOCK_GAMMA_DS
> >   * @data:		Allows easy initialisation of a union variable with a
> >   *			pointer into a __u8 array.
> >   */
> > @@ -61,6 +63,7 @@ union mali_c55_params_block {
> >  	const struct mali_c55_params_mesh_shading_config *shading_config;
> >  	const struct mali_c55_params_mesh_shading_selection *shading_selection;
> >  	const struct mali_c55_params_ccm *ccm;
> > +	const struct mali_c55_params_gamma *gamma;
> >  	const __u8 *data;
> >  };
> >
> > @@ -462,6 +465,70 @@ static void mali_c55_params_ccm(struct mali_c55 *mali_c55,
> >  	mali_c55_ctx_write(mali_c55, MALI_C55_REG_CCM_ENABLE, 1);
> >  }
> >
> > +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]));
> > +	mali_c55_ctx_update_bits(mali_c55, MALI_C55_REG_GAMMA_GAINS_2 + offset,
> > +				 MALI_C55_GAMMA_GAIN_B_MASK, params->gains[2]);
> > +	mali_c55_ctx_update_bits(mali_c55,
> > +				 MALI_C55_REG_GAMMA_OFFSETS_1 + offset,
> > +				 MALI_C55_GAMMA_OFFSET_R_MASK,
> > +				 params->offs[0]);
> > +	mali_c55_ctx_update_bits(mali_c55,
> > +				 MALI_C55_REG_GAMMA_OFFSETS_1 + offset,
> > +				 MALI_C55_GAMMA_OFFSET_G_MASK,
> > +				 MALI_C55_GAMMA_OFFSET_G(params->offs[1]));
> > +	mali_c55_ctx_update_bits(mali_c55,
> > +				 MALI_C55_REG_GAMMA_OFFSETS_2 + offset,
> > +				 MALI_C55_GAMMA_OFFSET_B_MASK,
> > +				 params->offs[2]);
> > +
> > +	for (unsigned int i = 0; i < MALI_C55_NUM_GAMMA_LUT_ELEMENTS; i++) {
> > +		__u32 addr = lut_base + (i * 4);
> > +
> > +		mali_c55_ctx_write(mali_c55, addr, params->lut[i]);
> > +	}
> > +
> > +	mali_c55_ctx_update_bits(mali_c55,
> > +				 MALI_C55_REG_GAMMA_RGB_ENABLE + offset,
> > +				 MALI_C55_GAMMA_ENABLE_MASK, 0x1);
> > +}
> > +
> > +static void mali_c55_params_gamma_fr(struct mali_c55 *mali_c55,
> > +				     union mali_c55_params_block block)
> > +{
> > +	return mali_c55_params_gamma(mali_c55, block,
> > +				     MALI_C55_CAP_DEV_FR_REG_OFFSET,
> > +				     MALI_C55_REG_FR_GAMMA_RGB_MEM);
> > +}
> > +
> > +static void mali_c55_params_gamma_ds(struct mali_c55 *mali_c55,
> > +				     union mali_c55_params_block block)
> > +{
> > +	/* We cannot apply parameters to DS if it is not fitted. */
> > +	if (!(mali_c55->capabilities & MALI_C55_GPS_DS_PIPE_FITTED))
> > +		return;
> > +
> > +	return mali_c55_params_gamma(mali_c55, block,
> > +				     MALI_C55_CAP_DEV_DS_REG_OFFSET,
> > +				     MALI_C55_REG_DS_GAMMA_RGB_MEM);
> > +}
> > +
> >  static const mali_c55_params_handler mali_c55_params_handlers[] = {
> >  	[MALI_C55_PARAM_BLOCK_SENSOR_OFFS] = &mali_c55_params_sensor_offs,
> >  	[MALI_C55_PARAM_BLOCK_AEXP_HIST] = &mali_c55_params_aexp_hist,
> > @@ -475,6 +542,8 @@ static const mali_c55_params_handler mali_c55_params_handlers[] = {
> >  	[MALI_C55_PARAM_MESH_SHADING_CONFIG] = &mali_c55_params_lsc_config,
> >  	[MALI_C55_PARAM_MESH_SHADING_SELECTION] = &mali_c55_params_lsc_selection,
> >  	[MALI_C55_PARAM_BLOCK_CCM] = &mali_c55_params_ccm,
> > +	[MALI_C55_PARAM_BLOCK_GAMMA_FR] = &mali_c55_params_gamma_fr,
> > +	[MALI_C55_PARAM_BLOCK_GAMMA_DS] = &mali_c55_params_gamma_ds,
> >  };
> >
> >  static const struct v4l2_isp_params_block_type_info
> > @@ -515,6 +584,12 @@ mali_c55_params_block_types_info[] = {
> >  	[MALI_C55_PARAM_BLOCK_CCM] = {
> >  		.size = sizeof(struct mali_c55_params_ccm),
> >  	},
> > +	[MALI_C55_PARAM_BLOCK_GAMMA_FR] = {
> > +		.size = sizeof(struct mali_c55_params_gamma),
> > +	},
> > +	[MALI_C55_PARAM_BLOCK_GAMMA_DS] = {
> > +		.size = sizeof(struct mali_c55_params_gamma),
> > +	},
> >  };
> >
> >  static_assert(ARRAY_SIZE(mali_c55_params_handlers) ==
> > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-registers.h b/drivers/media/platform/arm/mali-c55/mali-c55-registers.h
> > index f098effde7b4..7a606bd2e843 100644
> > --- a/drivers/media/platform/arm/mali-c55/mali-c55-registers.h
> > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-registers.h
> > @@ -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)
> > +#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)
> >  #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)
> > +#define MALI_C55_GAMMA_OFFSET_G(x)				((x) << 16)
>
> Nit: there is an extra tab before ((x) << 16) compared to the surrounding
> defines. Please align this with the style of the nearby MALI_C55_GAMMA_GAIN_G()
> macro.
>

Good catch, thanks

> >  #define MALI_C55_REG_GAMMA_OFFSETS_2			0x1c074
> >  #define MALI_C55_GAMMA_OFFSET_B_MASK			GENMASK(11, 0)
> >
> > @@ -441,6 +443,9 @@ enum mali_c55_interrupts {
> >  #define MALI_C55_REG_FR_GAMMA_RGB_ENABLE		0x1c064
> >  #define MALI_C55_REG_DS_GAMMA_RGB_ENABLE		0x1c1d8
> >
> > +#define MALI_C55_REG_FR_GAMMA_RGB_MEM			0x18280
> > +#define MALI_C55_REG_DS_GAMMA_RGB_MEM			0x18484
> > +
> >  #define MALI_C55_REG_FR_SCALER_HFILT			0x34a8
> >  #define MALI_C55_REG_FR_SCALER_VFILT			0x44a8
> >  #define MALI_C55_REG_DS_SCALER_HFILT			0x14a8
> > diff --git a/include/uapi/linux/media/arm/mali-c55-config.h b/include/uapi/linux/media/arm/mali-c55-config.h
> > index 0b2085eed81b..7e96564cb89c 100644
> > --- a/include/uapi/linux/media/arm/mali-c55-config.h
> > +++ b/include/uapi/linux/media/arm/mali-c55-config.h
> > @@ -36,6 +36,9 @@
> >   */
> >  #define MALI_C55_MAX_ZONES	(15 * 15)
> >
> > +/* Number of RGB gamma LUT entries. */
> > +#define MALI_C55_NUM_GAMMA_LUT_ELEMENTS 129
> > +
> >  /**
> >   * struct mali_c55_ae_1024bin_hist - Auto Exposure 1024-bin histogram statistics
> >   *
> > @@ -220,6 +223,8 @@ struct mali_c55_stats_buffer {
> >   * @MALI_C55_PARAM_MESH_SHADING_CONFIG : Mesh shading tables configuration
> >   * @MALI_C55_PARAM_MESH_SHADING_SELECTION: Mesh shading table selection
> >   * @MALI_C55_PARAM_BLOCK_CCM: Colour correction matrix
> > + * @MALI_C55_PARAM_BLOCK_GAMMA_FR: Gamma gain and offset for FR pipe
> > + * @MALI_C55_PARAM_BLOCK_GAMMA_DS: Gamma gain and offset for DS pipe
> >   */
> >  enum mali_c55_param_block_type {
> >  	MALI_C55_PARAM_BLOCK_SENSOR_OFFS,
> > @@ -234,6 +239,8 @@ enum mali_c55_param_block_type {
> >  	MALI_C55_PARAM_MESH_SHADING_CONFIG,
> >  	MALI_C55_PARAM_MESH_SHADING_SELECTION,
> >  	MALI_C55_PARAM_BLOCK_CCM,
> > +	MALI_C55_PARAM_BLOCK_GAMMA_FR,
> > +	MALI_C55_PARAM_BLOCK_GAMMA_DS,
> >  };
> >
> >  /**
> > @@ -795,6 +802,40 @@ struct mali_c55_params_ccm {
> >  	__u16 offs[3];
> >  };
> >
> > +/**
> > + * struct mali_c55_params_gamma - RGB Gamma correction
> > + *
> > + * Gamma correction is used to program a standard gamma curve such as the sRGB
> > + * one. It provides gains and offsets to implement contrast adjustments.
> > + *
> > + * Gamma correction is applied on both the FR and DS pipes separately in the RGB
> > + * colour domain where the following operations take place:
> > + * 1) An offset is subtracted from each colour channel
> > + * 2) Each colour channel is multiplied by a gain
> > + * 3) The Gamma LUT is applied to each colour channel
> > + *
> > + * The Gamma LUT has 129 entries where each node is an unsigned 12 bit number.
> > + * It is expected that LUT[0]=0 and LUT[128]=0xffff, with the other 127 values
>
> lut entries are documented as unsigned 12-bit values, so 0xffff looks
> inconsistent. Should this be 0xfff instead?

Oh yes!

Thanks
   j

>
> > + * defining the Gamma correction curve.
> > + *
> > + * As one Gamma correction block is available on both the FR and DS pipes, the
> > + * header.type field should be set to one of either
> > + * MALI_C55_PARAM_BLOCK_GAMMA_FR or MALI_C55_PARAM_BLOCK_GAMMA_DS from
> > + * :c:type:`mali_c55_param_block_type`.
> > + *
> > + * @header:	The Mali-C55 parameters block header
> > + * @gains:	Gains for the red, green and blue channel in unsigned Q4.8 format
> > + * @offs:	Offsets subtracted from the red, green and blue channels
> > + *		in unsigned 12-bit format
> > + * @lut:	129-node Gamma LUT in u0.12 format
> > + */
> > +struct mali_c55_params_gamma {
> > +	struct v4l2_isp_params_block_header header;
> > +	__u16 gains[3];
> > +	__u16 offs[3];
> > +	__u32 lut[MALI_C55_NUM_GAMMA_LUT_ELEMENTS];
> > +};
> > +
> >  /**
> >   * define MALI_C55_PARAMS_MAX_SIZE - Maximum size of all Mali C55 Parameters
> >   *
> > @@ -819,6 +860,8 @@ struct mali_c55_params_ccm {
> >  	sizeof(struct mali_c55_params_awb_gains) +		\
> >  	sizeof(struct mali_c55_params_mesh_shading_config) +	\
> >  	sizeof(struct mali_c55_params_mesh_shading_selection) +	\
> > -	sizeof(struct mali_c55_params_ccm))
> > +	sizeof(struct mali_c55_params_ccm) +			\
> > +	sizeof(struct mali_c55_params_gamma) +			\
> > +	sizeof(struct mali_c55_params_gamma))
> >
> >  #endif /* __UAPI_MALI_C55_CONFIG_H */
> >
>
> --
> Regards,
> Vincenzo
>
>