Re: [PATCH v2] videodev2.h: add helper to validate colorspace

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

Hi Hans,

On Thursday, 15 February 2018 13:56:44 EET Hans Verkuil wrote:
> On 15/02/18 12:08, Laurent Pinchart wrote:
> > On Thursday, 15 February 2018 12:57:44 EET Hans Verkuil wrote:
> >> On 14/02/18 16:16, Laurent Pinchart wrote:
> >>> On Wednesday, 14 February 2018 12:36:43 EET Niklas Söderlund wrote:
> >>>> There is no way for drivers to validate a colorspace value, which could
> >>>> be provided by user-space by VIDIOC_S_FMT for example. Add a helper to
> >>>> validate that the colorspace value is part of enum v4l2_colorspace.
> >>>> 
> >>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>>> ---
> >>>> 
> >>>>  include/uapi/linux/videodev2.h | 4 ++++
> >>>>  1 file changed, 4 insertions(+)
> >>>> 
> >>>> Hi,
> >>>> 
> >>>> I hope this is the correct header to add this helper to. I think it's
> >>>> since if it's in uapi not only can v4l2 drivers use it but tools like
> >>>> v4l-compliance gets access to it and can be updated to use this instead
> >>>> of the hard-coded check of just < 0xff as it was last time I checked.
> >>>> 
> >>>> * Changes since v1
> >>>> - Cast colorspace to u32 as suggested by Sakari and only check the
> >>>>   upper boundary to address a potential issue brought up by Laurent if
> >>>>   the data type tested is u32 which is not uncommon:
> >>>>     enum.c:30:16: warning: comparison of unsigned expression >= 0 is
> >>>>     always true [-Wtype-limits]
> >>>> 
> >>>>       return V4L2_COLORSPACE_IS_VALID(colorspace);
> >>>> 
> >>>> diff --git a/include/uapi/linux/videodev2.h
> >>>> b/include/uapi/linux/videodev2.h index
> >>>> 9827189651801e12..1f27c0f4187cbded 100644
> >>>> --- a/include/uapi/linux/videodev2.h
> >>>> +++ b/include/uapi/linux/videodev2.h
> >>>> @@ -238,6 +238,10 @@ enum v4l2_colorspace {
> >>>>  	V4L2_COLORSPACE_DCI_P3        = 12,
> >>>>  };
> >>>> 
> >>>> +/* Determine if a colorspace is defined in enum v4l2_colorspace */
> >>>> +#define V4L2_COLORSPACE_IS_VALID(colorspace)		\
> >>>> +	((u32)(colorspace) <= V4L2_COLORSPACE_DCI_P3)
> >> 
> >> Sorry, this won't work. Use __u32. u32 is only available in the kernel,
> >> not in userspace and this is a public header.
> > 
> > Indeed, that I should have caught.
> > 
> >> I am not convinced about the usefulness of this check either. Drivers
> >> will typically only support a subset of the available colorspaces, so
> >> they need a switch to test for that.
> > 
> > Most MC drivers won't, as they don't care about colorspaces in most
> > subdevs. It's important for the colorspace to be propagated within
> > subdevs, and validated across the pipeline, but in most case, apart from
> > the image source subdev, other subdevs won't care. They should accept any
> > valid colorspace given to them and propagate it to their source pads
> > unchanged (except of course for subdevs that can change the colorspace,
> > but that's the exception, not the rule).
> 
> Right. So 'passthrough' subdevs should just copy this information from
> source to sink, and only pure source or pure sink subdevs should validate
> these fields. That makes sense.
> 
> >> There is nothing wrong with userspace giving them an unknown colorspace:
> >> either they will map anything they don't support to something that they
> >> DO
> >> support, or they will return -EINVAL.
> > 
> > The former, not the latter. S_FMT should not return -EINVAL for
> > unsupported
> > colorspace, the same way it doesn't return -EINVAL for unsupported pixel
> > formats.
> > 
> >> If memory serves the spec requires the first option, so anything unknown
> >> will just be replaced.
> >> 
> >> And anyway, this raises the question of why you do this for the
> >> colorspace
> >> but not for all the other enums in the V4L2 API.
> > 
> > Because v4l2-compliance tries to set a colorspace > 0xff and expects that
> > to be replaced by a colorspace <= 0xff. That seems like a bogus check to
> > me, 0xff is as random as it can get.
> 
> v4l2-compliance fills all fields with 0xff, then it checks after calling the
> ioctl if all fields have been set to valid values.
> 
> But in this case it should ignore the colorspace-related fields for
> passthrough subdevs. The only passthrough devices that should set
> colorspace are colorspace converter devices. I'm not sure if we can
> reliably detect that.
> 
> >> It all seems rather pointless to me.
> >> 
> >> I won't accept this unless I see it being used in a driver in a useful
> >> way.
> >> 
> >> So for now:
> >> 
> >> Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > Can you then fix v4l2-compliance to stop testing colorspace against 0xff ?
> 
> For now I can simply relax this test for subdevs with sources and sinks.

You also need to relax it for video nodes with MC drivers, as the DMA engines 
don't care about colorspaces.

> >>>> +
> >>> 
> >>> Casting to u32 has the added benefit that the colorspace expression is
> >>> evaluated once only, I like that.
> >>> 
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> 
> >>>>  /*
> >>>>   * Determine how COLORSPACE_DEFAULT should map to a proper colorspace.
> >>>>   * This depends on whether this is a SDTV image (use SMPTE 170M), an

-- 
Regards,

Laurent Pinchart