On 15/02/18 13:32, Hans Verkuil wrote:
> On 15/02/18 13:06, Laurent Pinchart wrote:
>> 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@xxxxxxxxxxxx>
>>>>>>> ---
>>>>>>>
>>>>>>> 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@xxxxxxxxx>
>>>>
>>>> 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).
Of course this assumes that the colorspace information of the subdev that feeds
the DMA engine is correct. That's something I haven't looked at yet.
Regards,
Hans
