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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Jun 29, 2026 at 02:17:37PM +0100, Vincenzo Frascino wrote:
> On 29/06/2026 13:05, Laurent Pinchart wrote:
> > On Mon, Jun 29, 2026 at 12:08:08PM +0100, Vincenzo Frascino wrote:
> >> On 29/06/2026 10:57, Laurent Pinchart wrote:
> >>> On Sat, Jun 27, 2026 at 04:29:14PM +0200, Jacopo Mondi wrote:
> >>>> From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> >>>>
> >>>> Implement validation of CCM block parameters.
> >>>>
> >>>> CCM coefficients are expressed as 13 bits signed Q4.8 format and their
> >>>> raw value cannot be higher than 8191 (BIT(13) - 1).
> >>>>
> >>>> CCM gains are expressed as unsigned 12 bits Q4.8 format and their raw
> >>>> value cannot be higher than 4095 (BIT(12) - 1).
> >>>>
> >>>> CCM offsets are 12 bits unsigned integers and their value cannot be
> >>>> higher than 4095 (BIT(12) - 1).
> >>>>
> >>>> Validate the parameters provided by userspace using the .block_validate
> >>>> callback of struct v4l2_isp_params_block_type_info.
> >>> I don't think this is needed.
> >>>
> >>> We need to validate parameters that can cause the ISP to malfunction in
> >>> ways that requires a system reset, or in ways that cause malfunction of
> >>> other system components (e.g. buffer overflows, memory bus lock ups,
> >>> ...). The rest doesn't need to be validated.
> >>>
> >>> If you want to be cautious, you can just mask the value when writing to
> >>> registers, which I think you're doing in patch 1/4.
> >>
> >> According to me here is not a matter of being cautious, but of honouring the
> >> contract with the userspace.
> >>
> >> If the userspace is doing something wrong it should be notified. The only
> >> reasonable argument against this would be if this code is on a critical path and
> >> the validations have a performance impact.
> > 
> > I don't agree with this. As long as it doesn't have an impact on other
> > parts of the system, there's no need to notify userspace. It's purely a
> > userspace issue, it's pointless to waste CPU cycles every frame.
> 
> I don't think it's only about protecting the hardware from invalid values.
> 
> The userspace API defines the valid range for these parameters. If userspace
> provides values outside that range, returning an error makes the issue visible
> immediately instead of silently changing the requested configuration by masking
> or clamping the values. From userspace's perspective, silently accepting invalid
> input can make debugging harder, as the configuration that gets applied is no
> longer the one that was requested.

That's a valid concern, but I would handle it with a validation layer in
userspace if needed for debugging. That will allow userspace
implementations to select the amount of validation they deem fit for
their use cases.

> This also matches the documented behaviour of the V4L2 extended controls ioctl,
> which specifies that EINVAL should be returned when the value of a control is
> invalid:
> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-g-ext-ctrls.html
> 
> The validation also isn't on the critical per-frame path. It's only performed
> when userspace updates the parameters, so the CPU cost is negligible.
> 
> That said, if the expectation for the V4L2 ISP API is that drivers should only
> validate values that could affect system stability or security, and silently
> mask everything else, I'm happy to follow that approach for consistency.

That's my expectation at least :-) That's what we aimed for when
designing the API. The expectation may not have always been voiced
clearly though.

> >> @Jacopo, can you please confirm if this is the case?

-- 
Regards,

Laurent Pinchart