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

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

Hi Hans,

On Thursday, 15 February 2018 14:32:55 EET Hans Verkuil wrote:
> On 15/02/18 13:06, Laurent Pinchart wrote:
> > 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 usefu
> >>>> 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.
> 
> Yes, they do. Many DMA engines can at least do RGB <-> YUV conversions, so
> they should get the colorspace info from their source and pass it on to
> userspace (after correcting for any conversions done by the DMA engine).

Not in the MC case. Video nodes there only model the DMA engine, and are thus 
not aware of colorspaces. What MC drivers do is check at stream on time when 
validating the pipeline that the colorspace set by userspace on the video node 
corresponds to the colorspace on the source pad of the connected subdev, but 
that's only to ensure that userspace gets a coherent view of colorspace across 
the pipeline, not to program the hardware. There could be exceptions, but in 
the general case, the video node implementation of an MC driver will accept 
any colorspace and only validate it at stream on time, similarly to how it 
does for the frame size format instance (and in the frame size case it will 
usually enforce min/max limits when the DMA engine limits the frame size).

-- 
Regards,

Laurent Pinchart