Re: [PATCH v3 4/4] media: mali-c55: Implement Gamma block validation

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

Hi Vincenzo

On Mon, Jun 29, 2026 at 09:10:39AM +0100, Vincenzo Frascino wrote:
> Hi Jacopo,
>
> thank you for your patch.
>
> On 27/06/2026 15:29, Jacopo Mondi wrote:
> > From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> >
> > Implement validation of Gamma block parameters.
> >
> > Gamma gains are expressed as unsigned 12 bits Q4.8 format and their raw
> > value cannot be higher than 4095 (BIT(12) - 1).
> >
> > Gamma offsets are 12 bits unsigned integers and their value cannot be
> > higher than 4095 (BIT(12) - 1).
> >
> > The Gamma LUT table is expected to have 0 as first member and 0xfff
> > as last member.
> >
> > Validate the parameters provided by userspace using the .block_validate
> > callback of struct v4l2_isp_params_block_type_info.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> >
>
> Looks good overall. I have just one comment. With this:
>
> Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
>
> > ---
> > v3:
> > - new patch
> > ---
> >  .../media/platform/arm/mali-c55/mali-c55-params.c  | 34 ++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > 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 5857e9c2daf7..e9ab0e2dee15 100644
> > --- a/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > @@ -581,6 +581,38 @@ static int mali_c55_ccm_validate(struct device *dev,
> >  	return 0;
> >  }
> >
> > +static int mali_c55_gamma_validate(struct device *dev,
> > +				   const struct v4l2_isp_block_header *block)
> > +{
> > +	const struct mali_c55_params_gamma *gamma =
> > +		(const struct mali_c55_params_gamma *)(block);
> > +
> > +	for (unsigned int i = 0; i < 3; i++) {
> > +		/* Gains are 12 bits unsigned Q4.8. */
> > +		if (gamma->gains[i] > 4095) {
> > +			dev_dbg(dev, "Invalid gain value %u\n",
> > +				gamma->gains[i]);
> > +			return -EINVAL;
> > +		}
> > +
> > +		/* Offsets are 12 bits unsigned integers. */
> > +		if (gamma->offs[i] > 4095) {
> > +			dev_dbg(dev, "Invalid offset value %u\n",
> > +				gamma->offs[i]);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	/* Check the first and last gamma lut entries match the expectations. */
> > +	if (gamma->lut[0] != 0 ||
> > +	    gamma->lut[MALI_C55_NUM_GAMMA_LUT_ELEMENTS - 1] != 0xfff) {
>
> I am still learning about these things and might be wrong. Should not we also
> validate that intermediate LUT entries are monotonic and non-decreasing?
>

Eh, good question.

This all new for everyone, as this would be the first user of
'block_validate()' in tree.

Here I checked only the first and last member because it seems like a
good compromise between performances and correctness. This function
might potentially run for every frame (where a CCM table is specified
by userspace), so I'm a bit unsure how far we should go with
validation.

I kept these two patches broken out from the ones that introduce the
uapi specifically to get feedback on this. Maybe I'm overconcerned
about the additional cost of validation ?

> > +		dev_dbg(dev, "Invalid Gamma LUT table\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct v4l2_isp_params_block_type_info
> >  mali_c55_params_block_types_info[] = {
> >  	[MALI_C55_PARAM_BLOCK_SENSOR_OFFS] = {
> > @@ -622,9 +654,11 @@ mali_c55_params_block_types_info[] = {
> >  	},
> >  	[MALI_C55_PARAM_BLOCK_GAMMA_FR] = {
> >  		.size = sizeof(struct mali_c55_params_gamma),
> > +		.block_validate = mali_c55_gamma_validate,
> >  	},
> >  	[MALI_C55_PARAM_BLOCK_GAMMA_DS] = {
> >  		.size = sizeof(struct mali_c55_params_gamma),
> > +		.block_validate = mali_c55_gamma_validate,
> >  	},
> >  };
> >
> >
>
> --
> Regards,
> Vincenzo
>