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

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

[Niklas Söderlund]

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)
+
 /*
  * Determine how COLORSPACE_DEFAULT should map to a proper colorspace.
  * This depends on whether this is a SDTV image (use SMPTE 170M), an
-- 
2.16.1

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

[Sakari Ailus]

On Wed, Feb 14, 2018 at 11:36:43AM +0100, 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>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> ---
>  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)
> +
>  /*
>   * Determine how COLORSPACE_DEFAULT should map to a proper colorspace.
>   * This depends on whether this is a SDTV image (use SMPTE 170M), an
> -- 
> 2.16.1
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

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

[Sakari Ailus]

On Wed, Feb 14, 2018 at 11:36:43AM +0100, 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>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> ---
>  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)
> +
>  /*
>   * Determine how COLORSPACE_DEFAULT should map to a proper colorspace.
>   * This depends on whether this is a SDTV image (use SMPTE 170M), an
> -- 
> 2.16.1
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

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)
> +

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

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

[Hans Verkuil]

On 14/02/18 16:16, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> 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.

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. 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. 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.

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>

Sorry,

	Hans

>> +
> 
> 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
> 
> 

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

[Laurent Pinchart]

Hi Hans,

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).

> 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.

> 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 ?

> >> +
> > 
> > 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

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

[Hans Verkuil]

On 15/02/18 12:08, Laurent Pinchart wrote:
> Hi Hans,
> 
> 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.

Regards,

	Hans

> 
>>>> +
>>>
>>> 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
> 

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

[Laurent Pinchart]

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

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

[Hans Verkuil]

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@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.

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).

Regards,

	Hans

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

[Hans Verkuil]

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@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.
> 
> 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

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

[Laurent Pinchart]

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

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

[Niklas Söderlund]

Hi Hans,

Thanks for your feedback.

[snip]

> > >>> 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).

I'm afraid the issue described above by Laurent is what sparked me to 
write this commit to begin with. In my never ending VIN Gen3 patch-set I 
currency need to carry a patch [1] to implement a hack to make sure 
v4l2-compliance do not fail for the VIN Gen3 MC-centric use-case. This 
patch was an attempt to be able to validate the colorspace using the 
magic value 0xff.

I don't feel strongly for this patch in particular and I'm happy to drop 
it.  But I would like to receive some guidance on how to then properly 
be able to handle this problem for the MC-centric VIN driver use-case.  
One option is as you suggested to relax the test in v4l-compliance to 
not check colorspace, but commit [2] is not enough to resolve the issue 
for my MC use-case.

As Laurent stated above, the use-case is that the video device shall 
accept any colorspace set from user-space. This colorspace is then only 
used as stream on time to validate the MC pipeline. The VIN driver do 
not care about colorspace, but I care about not breaking v4l2-compliance 
as I find it's a very useful tool :-)

I'm basing the following on the latest v4l-utils master 
(4665ab1fbab1ddaa)  which contains commit [2]. The core issue is that if 
I do not have a patch like [1] the v4l2-compliance run fails for format 
ioctls:

Format ioctls (Input 0):
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
	test VIDIOC_G/S_PARM: OK (Not Supported)
	test VIDIOC_G_FBUF: OK (Not Supported)
		fail: v4l2-test-formats.cpp(330): !colorspace
		fail: v4l2-test-formats.cpp(439): testColorspace(pix.pixelformat, pix.colorspace, pix.ycbcr_enc, pix.quantization)
	test VIDIOC_G_FMT: FAIL
	test VIDIOC_TRY_FMT: OK (Not Supported)
	test VIDIOC_S_FMT: OK (Not Supported)
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
	test Cropping: OK (Not Supported)
	test Composing: OK (Not Supported)
	test Scaling: OK

Well that is OK as that fails when colorspace is V4L2_COLORSPACE_DEFAULT 
and that is not valid. If I instead of reverting [1] only test for 
V4L2_COLORSPACE_DEFAULT which would not require this patch to implement:

-       if (!pix->colorspace || pix->colorspace >= 0xff)
+       if (pix->colorspace == V4L2_COLORSPACE_DEFAULT)

I still fail for the format ioctls:

Format ioctls (Input 0):
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
	test VIDIOC_G/S_PARM: OK (Not Supported)
	test VIDIOC_G_FBUF: OK (Not Supported)
	test VIDIOC_G_FMT: OK
		fail: v4l2-test-formats.cpp(336): colorspace >= 0xff
		fail: v4l2-test-formats.cpp(439): testColorspace(pix.pixelformat, pix.colorspace, pix.ycbcr_enc, pix.quantization)
		fail: v4l2-test-formats.cpp(753): Video Capture is valid, but TRY_FMT failed to return a format
	test VIDIOC_TRY_FMT: FAIL
		fail: v4l2-test-formats.cpp(336): colorspace >= 0xff
		fail: v4l2-test-formats.cpp(439): testColorspace(pix.pixelformat, pix.colorspace, pix.ycbcr_enc, pix.quantization)
		fail: v4l2-test-formats.cpp(1018): Video Capture is valid, but no S_FMT was implemented
	test VIDIOC_S_FMT: FAIL
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
	test Cropping: OK (Not Supported)
	test Composing: OK (Not Supported)
	test Scaling: OK

But now I fail on that colorspace >= 0xff which was what the patch in my 
VIN Gen3 series tries to address but as Laurent points out and I agree 
is not a good idea as the 0xff is a magic number and this patch tried to 
add a remedy too. The root of this in v4l2-compliance is 
createInvalidFmt() which quiet effectively creates an invalid format 
with the colorspace set to 0xff but there are no way for drivers to 
verify that a colorspace value from user-space is valid :-(

If this patch is to be dropped, and I'm fine with that I would like your 
opinion on how I can still keep the VIN driver compatible with the 
compliance tool. The option's I see are:

1. Keep the patch [1] and accept that there is a need for a 0xff magic 
   value. This 'works' but I don't think it's the correct solution.

2. Move the check this patch tries to add a helper for directly to the 
   VIN driver. This works but will require the driver to be updated if a 
   new colorspace is added.

3. Update createInvalidFmt() v4l2-compliance to not set the colorspace 
   to 0xff but instead set it to V4L2_COLORSPACE_DEFAULT. A similar 
   thing is already done for the filed in this function.

	memset(&fmt, 0xff, sizeof(fmt));
	fmt.type = type;
	fmt.fmt.pix.field = V4L2_FIELD_ANY;
	...

   I'm not sure if this is the best solution as it would not test 
   drivers for what happens if they are presented with an invalid 
   colorspace. But with such a change the driver can be written to still 
   pass the test.

4. Always forcing a specific colorspace (V4L2_COLORSPACE_SRGB) in the 
   VIN format functions and ignoring what was requested by the user.

   I don't think this is good at all as it would be possible that 
   pipeline validation fails due to a colorspace mismatch. This can be 
   worked around by dropping the colorspace check from the VIN stream on 
   function.


I'm sure there are other options open which I can't think of, in fact I 
hope there are. I'm not over excited about any of the options above as 
they all in one way or another just works around the problem of being 
able to validate input from user-space. But I'm even less excited about 
breaking v4l2-compliance compatibility so any path I can take here to 
keep the user being able to specify the colorspace and v4l2-compliance 
being happy would be a better solution for me :-)

1. https://patchwork.linuxtv.org/patch/46717/
2. 432d9ebfcea65337 ("v4l2-compliance: ignore colorspace tests for passthu subdevs")

-- 
Regards,
Niklas Söderlund

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

[Niklas Söderlund]

Hi Hans,

Thanks for your feedback.

[snip]

> > >>> 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).

I'm afraid the issue described above by Laurent is what sparked me to 
write this commit to begin with. In my never ending VIN Gen3 patch-set I 
currency need to carry a patch [1] to implement a hack to make sure 
v4l2-compliance do not fail for the VIN Gen3 MC-centric use-case. This 
patch was an attempt to be able to validate the colorspace using the 
magic value 0xff.

I don't feel strongly for this patch in particular and I'm happy to drop 
it.  But I would like to receive some guidance on how to then properly 
be able to handle this problem for the MC-centric VIN driver use-case.  
One option is as you suggested to relax the test in v4l-compliance to 
not check colorspace, but commit [2] is not enough to resolve the issue 
for my MC use-case.

As Laurent stated above, the use-case is that the video device shall 
accept any colorspace set from user-space. This colorspace is then only 
used as stream on time to validate the MC pipeline. The VIN driver do 
not care about colorspace, but I care about not breaking v4l2-compliance 
as I find it's a very useful tool :-)

I'm basing the following on the latest v4l-utils master 
(4665ab1fbab1ddaa)  which contains commit [2]. The core issue is that if 
I do not have a patch like [1] the v4l2-compliance run fails for format 
ioctls:

Format ioctls (Input 0):
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
	test VIDIOC_G/S_PARM: OK (Not Supported)
	test VIDIOC_G_FBUF: OK (Not Supported)
		fail: v4l2-test-formats.cpp(330): !colorspace
		fail: v4l2-test-formats.cpp(439): testColorspace(pix.pixelformat, pix.colorspace, pix.ycbcr_enc, pix.quantization)
	test VIDIOC_G_FMT: FAIL
	test VIDIOC_TRY_FMT: OK (Not Supported)
	test VIDIOC_S_FMT: OK (Not Supported)
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
	test Cropping: OK (Not Supported)
	test Composing: OK (Not Supported)
	test Scaling: OK

Well that is OK as that fails when colorspace is V4L2_COLORSPACE_DEFAULT 
and that is not valid. If I instead of reverting [1] only test for 
V4L2_COLORSPACE_DEFAULT which would not require this patch to implement:

-       if (!pix->colorspace || pix->colorspace >= 0xff)
+       if (pix->colorspace == V4L2_COLORSPACE_DEFAULT)

I still fail for the format ioctls:

Format ioctls (Input 0):
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
	test VIDIOC_G/S_PARM: OK (Not Supported)
	test VIDIOC_G_FBUF: OK (Not Supported)
	test VIDIOC_G_FMT: OK
		fail: v4l2-test-formats.cpp(336): colorspace >= 0xff
		fail: v4l2-test-formats.cpp(439): testColorspace(pix.pixelformat, pix.colorspace, pix.ycbcr_enc, pix.quantization)
		fail: v4l2-test-formats.cpp(753): Video Capture is valid, but TRY_FMT failed to return a format
	test VIDIOC_TRY_FMT: FAIL
		fail: v4l2-test-formats.cpp(336): colorspace >= 0xff
		fail: v4l2-test-formats.cpp(439): testColorspace(pix.pixelformat, pix.colorspace, pix.ycbcr_enc, pix.quantization)
		fail: v4l2-test-formats.cpp(1018): Video Capture is valid, but no S_FMT was implemented
	test VIDIOC_S_FMT: FAIL
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
	test Cropping: OK (Not Supported)
	test Composing: OK (Not Supported)
	test Scaling: OK

But now I fail on that colorspace >= 0xff which was what the patch in my 
VIN Gen3 series tries to address but as Laurent points out and I agree 
is not a good idea as the 0xff is a magic number and this patch tried to 
add a remedy too. The root of this in v4l2-compliance is 
createInvalidFmt() which quiet effectively creates an invalid format 
with the colorspace set to 0xff but there are no way for drivers to 
verify that a colorspace value from user-space is valid :-(

If this patch is to be dropped, and I'm fine with that I would like your 
opinion on how I can still keep the VIN driver compatible with the 
compliance tool. The option's I see are:

1. Keep the patch [1] and accept that there is a need for a 0xff magic 
   value. This 'works' but I don't think it's the correct solution.

2. Move the check this patch tries to add a helper for directly to the 
   VIN driver. This works but will require the driver to be updated if a 
   new colorspace is added.

3. Update createInvalidFmt() v4l2-compliance to not set the colorspace 
   to 0xff but instead set it to V4L2_COLORSPACE_DEFAULT. A similar 
   thing is already done for the filed in this function.

	memset(&fmt, 0xff, sizeof(fmt));
	fmt.type = type;
	fmt.fmt.pix.field = V4L2_FIELD_ANY;
	...

   I'm not sure if this is the best solution as it would not test 
   drivers for what happens if they are presented with an invalid 
   colorspace. But with such a change the driver can be written to still 
   pass the test.

4. Always forcing a specific colorspace (V4L2_COLORSPACE_SRGB) in the 
   VIN format functions and ignoring what was requested by the user.

   I don't think this is good at all as it would be possible that 
   pipeline validation fails due to a colorspace mismatch. This can be 
   worked around by dropping the colorspace check from the VIN stream on 
   function.


I'm sure there are other options open which I can't think of, in fact I 
hope there are. I'm not over excited about any of the options above as 
they all in one way or another just works around the problem of being 
able to validate input from user-space. But I'm even less excited about 
breaking v4l2-compliance compatibility so any path I can take here to 
keep the user being able to specify the colorspace and v4l2-compliance 
being happy would be a better solution for me :-)

1. https://patchwork.linuxtv.org/patch/46717/
2. 432d9ebfcea65337 ("v4l2-compliance: ignore colorspace tests for passthu subdevs")

-- 
Regards,
Niklas S�derlund

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

[Hans Verkuil]

Hi Niklas,

On 02/19/2018 11:28 PM, Niklas Söderlund wrote:
> Hi Hans,
> 
> Thanks for your feedback.
> 
> [snip]
> 
>>>>>> 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).
> 
> I'm afraid the issue described above by Laurent is what sparked me to 
> write this commit to begin with. In my never ending VIN Gen3 patch-set I 
> currency need to carry a patch [1] to implement a hack to make sure 
> v4l2-compliance do not fail for the VIN Gen3 MC-centric use-case. This 
> patch was an attempt to be able to validate the colorspace using the 
> magic value 0xff.

This is NOT a magic value. The test that's done here is to memset the
format structure with 0xff, then call the ioctl. Afterwards it checks
if there are any remaining 0xff bytes left in the struct since it expects
the driver to have overwritten it by something else. That's where the 0xff
comes from.

> 
> I don't feel strongly for this patch in particular and I'm happy to drop 
> it.  But I would like to receive some guidance on how to then properly 
> be able to handle this problem for the MC-centric VIN driver use-case.  
> One option is as you suggested to relax the test in v4l-compliance to 
> not check colorspace, but commit [2] is not enough to resolve the issue 
> for my MC use-case.
> 
> As Laurent stated above, the use-case is that the video device shall 
> accept any colorspace set from user-space. This colorspace is then only 
> used as stream on time to validate the MC pipeline. The VIN driver do 
> not care about colorspace, but I care about not breaking v4l2-compliance 
> as I find it's a very useful tool :-)

I think part of my confusion here is that there are two places where you
deal with colorspaces in a DMA engine: first there is a input pad of the
DMA engine entity, secondly there is the v4l2_pix_format for the memory
description.

The second is set by the driver based on what userspace specified for the
input pad, together with any changes due to additional conversions such
as quantization range and RGB <-> YUV by the DMA engine.

So any colorspace validation is done for the input pad. The question is
what that validation should be. It's never been defined.

Also the handling of COLORSPACE_DEFAULT for pad formats needs to be defined.

This is not the first time this cropped up, see e.g. this RFC patch:

https://patchwork.linuxtv.org/patch/41734/

> I'm basing the following on the latest v4l-utils master 
> (4665ab1fbab1ddaa)  which contains commit [2]. The core issue is that if 
> I do not have a patch like [1] the v4l2-compliance run fails for format 
> ioctls:
> 
> Format ioctls (Input 0):
> 	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> 	test VIDIOC_G/S_PARM: OK (Not Supported)
> 	test VIDIOC_G_FBUF: OK (Not Supported)
> 		fail: v4l2-test-formats.cpp(330): !colorspace
> 		fail: v4l2-test-formats.cpp(439): testColorspace(pix.pixelformat, pix.colorspace, pix.ycbcr_enc, pix.quantization)
> 	test VIDIOC_G_FMT: FAIL
> 	test VIDIOC_TRY_FMT: OK (Not Supported)
> 	test VIDIOC_S_FMT: OK (Not Supported)
> 	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> 	test Cropping: OK (Not Supported)
> 	test Composing: OK (Not Supported)
> 	test Scaling: OK
> 
> Well that is OK as that fails when colorspace is V4L2_COLORSPACE_DEFAULT 
> and that is not valid. If I instead of reverting [1] only test for 
> V4L2_COLORSPACE_DEFAULT which would not require this patch to implement:
> 
> -       if (!pix->colorspace || pix->colorspace >= 0xff)
> +       if (pix->colorspace == V4L2_COLORSPACE_DEFAULT)
> 
> I still fail for the format ioctls:
> 
> Format ioctls (Input 0):
> 	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> 	test VIDIOC_G/S_PARM: OK (Not Supported)
> 	test VIDIOC_G_FBUF: OK (Not Supported)
> 	test VIDIOC_G_FMT: OK
> 		fail: v4l2-test-formats.cpp(336): colorspace >= 0xff
> 		fail: v4l2-test-formats.cpp(439): testColorspace(pix.pixelformat, pix.colorspace, pix.ycbcr_enc, pix.quantization)
> 		fail: v4l2-test-formats.cpp(753): Video Capture is valid, but TRY_FMT failed to return a format
> 	test VIDIOC_TRY_FMT: FAIL
> 		fail: v4l2-test-formats.cpp(336): colorspace >= 0xff
> 		fail: v4l2-test-formats.cpp(439): testColorspace(pix.pixelformat, pix.colorspace, pix.ycbcr_enc, pix.quantization)
> 		fail: v4l2-test-formats.cpp(1018): Video Capture is valid, but no S_FMT was implemented
> 	test VIDIOC_S_FMT: FAIL
> 	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> 	test Cropping: OK (Not Supported)
> 	test Composing: OK (Not Supported)
> 	test Scaling: OK
> 
> But now I fail on that colorspace >= 0xff which was what the patch in my 
> VIN Gen3 series tries to address but as Laurent points out and I agree 
> is not a good idea as the 0xff is a magic number and this patch tried to 
> add a remedy too. The root of this in v4l2-compliance is 
> createInvalidFmt() which quiet effectively creates an invalid format 
> with the colorspace set to 0xff but there are no way for drivers to 
> verify that a colorspace value from user-space is valid :-(
> 
> If this patch is to be dropped, and I'm fine with that I would like your 
> opinion on how I can still keep the VIN driver compatible with the 
> compliance tool. The option's I see are:
> 
> 1. Keep the patch [1] and accept that there is a need for a 0xff magic 
>    value. This 'works' but I don't think it's the correct solution.
> 
> 2. Move the check this patch tries to add a helper for directly to the 
>    VIN driver. This works but will require the driver to be updated if a 
>    new colorspace is added.
> 
> 3. Update createInvalidFmt() v4l2-compliance to not set the colorspace 
>    to 0xff but instead set it to V4L2_COLORSPACE_DEFAULT. A similar 
>    thing is already done for the filed in this function.
> 
> 	memset(&fmt, 0xff, sizeof(fmt));
> 	fmt.type = type;
> 	fmt.fmt.pix.field = V4L2_FIELD_ANY;
> 	...
> 
>    I'm not sure if this is the best solution as it would not test 
>    drivers for what happens if they are presented with an invalid 
>    colorspace. But with such a change the driver can be written to still 
>    pass the test.
> 
> 4. Always forcing a specific colorspace (V4L2_COLORSPACE_SRGB) in the 
>    VIN format functions and ignoring what was requested by the user.
> 
>    I don't think this is good at all as it would be possible that 
>    pipeline validation fails due to a colorspace mismatch. This can be 
>    worked around by dropping the colorspace check from the VIN stream on 
>    function.
> 
> 
> I'm sure there are other options open which I can't think of, in fact I 
> hope there are. I'm not over excited about any of the options above as 
> they all in one way or another just works around the problem of being 
> able to validate input from user-space. But I'm even less excited about 
> breaking v4l2-compliance compatibility so any path I can take here to 
> keep the user being able to specify the colorspace and v4l2-compliance 
> being happy would be a better solution for me :-)
> 
> 1. https://patchwork.linuxtv.org/patch/46717/
> 2. 432d9ebfcea65337 ("v4l2-compliance: ignore colorspace tests for passthu subdevs")
> 

I do not want to relax the test for colorspace for the v4l2_pix_format in
the compliance test. That test is good and should remain.

For the input pad format there are two issues:

1) Setting the initial colorspace information. If you open the v4l-subdev
device then the initial colorspace data is likely all 0. That's wrong for
the colorspace since 0 is not allowed. So init_cfg should set it to something
non-0 (I'd expect either SRGB or RAW). Or perhaps this should be done in the
core as a standard initialization in the absence of the init_cfg op (or before
it is called).

An alternative is to allow a 0 (DEFAULT) value here, but then what do you use
in v4l2_pix_format? That definitely can't be 0. I think I prefer to always
have something explicit here.

2) Validation of the colorspace fields when setting the format on the input pad.
After thinking about this some more I believe that the only reasonable thing
you can do here is to indeed validate it based on the current range of known
colorspaces (or more strict of course if the hardware has limitations).

But rather than adding validate defines I would just add something like this
to the colorspace enum:

	/* Update when a new colorspace is added */
	V4L2_COLORSPACE_LAST        = V4L2_COLORSPACE_DCI_P3

and do the same for the other colorspace-related enums.

In v4l2-subdev.c you can then check and validate this. Anything out-of-range
would map to COLORSPACE_SRGB/RAW or 0 (DEFAULT) for the other fields.

I think SRGB is best here since it is the most widely used and understood
colorspace.

Another issue is pipeline validation. See the link to the RFC patch above.
The biggest issue here is that filling in these fields has been hit-and-miss
and you don't want things to suddenly fail.

If the core validates/initializes these fields, then at least we always see
something valid here (i.e. never 0 or > V4L2_COLORSPACE_LAST for the colorspace).
I'd have to think about it some more.

This whole mess once again shows the importance of good compliance tests, especially
for complex APIs like this. It forces you to think about how this should be handled
instead of doing some vague hand-waving.

Regards,

	Hans

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

[Laurent Pinchart]

Hi Hans,

On Tuesday, 20 February 2018 10:37:22 EET Hans Verkuil wrote:
> On 02/19/2018 11:28 PM, Niklas Söderlund wrote:
> > Hi Hans,
> > 
> > Thanks for your feedback.
> > 
> > [snip]
> > 
> >>>>>> 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).> 
> > I'm afraid the issue described above by Laurent is what sparked me to
> > write this commit to begin with. In my never ending VIN Gen3 patch-set I
> > currency need to carry a patch [1] to implement a hack to make sure
> > v4l2-compliance do not fail for the VIN Gen3 MC-centric use-case. This
> > patch was an attempt to be able to validate the colorspace using the
> > magic value 0xff.
> 
> This is NOT a magic value. The test that's done here is to memset the
> format structure with 0xff, then call the ioctl. Afterwards it checks
> if there are any remaining 0xff bytes left in the struct since it expects
> the driver to have overwritten it by something else. That's where the 0xff
> comes from.

It's no less or more magic than using 0xdeadbeef or any fixed value :-) I 
think we all agree that it isn't a value that is meant to be handled 
specifically by drivers, so it's not magic in that sense.

> > I don't feel strongly for this patch in particular and I'm happy to drop
> > it.  But I would like to receive some guidance on how to then properly
> > be able to handle this problem for the MC-centric VIN driver use-case.
> > One option is as you suggested to relax the test in v4l-compliance to
> > not check colorspace, but commit [2] is not enough to resolve the issue
> > for my MC use-case.
> > 
> > As Laurent stated above, the use-case is that the video device shall
> > accept any colorspace set from user-space. This colorspace is then only
> > used as stream on time to validate the MC pipeline. The VIN driver do
> > not care about colorspace, but I care about not breaking v4l2-compliance
> > as I find it's a very useful tool :-)
> 
> I think part of my confusion here is that there are two places where you
> deal with colorspaces in a DMA engine: first there is a input pad of the
> DMA engine entity, secondly there is the v4l2_pix_format for the memory
> description.
> 
> The second is set by the driver based on what userspace specified for the
> input pad, together with any changes due to additional conversions such
> as quantization range and RGB <-> YUV by the DMA engine.

No, I'm sorry, for MC-based drivers this isn't correct. The media entity that 
symbolizes the DMA engine indeed has a sink pad, but it's a video node, not a 
subdev. It thus has no media bus format configured for its sink pad. The 
closest pad in the pipeline that has a media bus format is the source pad of 
the subdev connected to the video node.

There's no communication within the kernel at G/S_FMT time between the video 
node and its connected subdev. The only time we look at the pipeline as a 
whole is when starting the stream to validate that the pipeline is correctly 
configured. We thus have to implement G/S_FMT on the video node without any 
knowledge about the connected subdev, and thus accept any colorspace.

> So any colorspace validation is done for the input pad. The question is
> what that validation should be. It's never been defined.

No format is set on the video node's entity sink pad for the reason above, so 
no validation occurs when setting the colorspace on the sink pad as that never 
happens.

> Also the handling of COLORSPACE_DEFAULT for pad formats needs to be defined.
> 
> This is not the first time this cropped up, see e.g. this RFC patch:
> 
> https://patchwork.linuxtv.org/patch/41734/
> 
> > I'm basing the following on the latest v4l-utils master
> > (4665ab1fbab1ddaa)  which contains commit [2]. The core issue is that if
> > I do not have a patch like [1] the v4l2-compliance run fails for format
> > ioctls:
> > 
> > Format ioctls (Input 0):
> > 	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> > 	test VIDIOC_G/S_PARM: OK (Not Supported)
> > 	test VIDIOC_G_FBUF: OK (Not Supported)
> > 	
> > 		fail: v4l2-test-formats.cpp(330): !colorspace
> > 		fail: v4l2-test-formats.cpp(439): testColorspace(pix.pixelformat,
> > 		pix.colorspace, pix.ycbcr_enc, pix.quantization)> 	
> > 	test VIDIOC_G_FMT: FAIL
> > 	test VIDIOC_TRY_FMT: OK (Not Supported)
> > 	test VIDIOC_S_FMT: OK (Not Supported)
> > 	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> > 	test Cropping: OK (Not Supported)
> > 	test Composing: OK (Not Supported)
> > 	test Scaling: OK
> > 
> > Well that is OK as that fails when colorspace is V4L2_COLORSPACE_DEFAULT
> > and that is not valid. If I instead of reverting [1] only test for
> > V4L2_COLORSPACE_DEFAULT which would not require this patch to implement:
> > 
> > -       if (!pix->colorspace || pix->colorspace >= 0xff)
> > +       if (pix->colorspace == V4L2_COLORSPACE_DEFAULT)
> > 
> > I still fail for the format ioctls:
> > 
> > Format ioctls (Input 0):
> > 	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> > 	test VIDIOC_G/S_PARM: OK (Not Supported)
> > 	test VIDIOC_G_FBUF: OK (Not Supported)
> > 	test VIDIOC_G_FMT: OK
> > 	
> > 		fail: v4l2-test-formats.cpp(336): colorspace >= 0xff
> > 		fail: v4l2-test-formats.cpp(439): testColorspace(pix.pixelformat,
> > 		pix.colorspace, pix.ycbcr_enc, pix.quantization) fail:
> > 		v4l2-test-formats.cpp(753): Video Capture is valid, but TRY_FMT
> > 		failed to return a format
> > 	test VIDIOC_TRY_FMT: FAIL
> > 	
> > 		fail: v4l2-test-formats.cpp(336): colorspace >= 0xff
> > 		fail: v4l2-test-formats.cpp(439): testColorspace(pix.pixelformat,
> > 		pix.colorspace, pix.ycbcr_enc, pix.quantization) fail:
> > 		v4l2-test-formats.cpp(1018): Video Capture is valid, but no S_FMT
> > 		was implemented
> > 	test VIDIOC_S_FMT: FAIL
> > 	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> > 	test Cropping: OK (Not Supported)
> > 	test Composing: OK (Not Supported)
> > 	test Scaling: OK
> > 
> > But now I fail on that colorspace >= 0xff which was what the patch in my
> > VIN Gen3 series tries to address but as Laurent points out and I agree
> > is not a good idea as the 0xff is a magic number and this patch tried to
> > add a remedy too. The root of this in v4l2-compliance is
> > createInvalidFmt() which quiet effectively creates an invalid format
> > with the colorspace set to 0xff but there are no way for drivers to
> > verify that a colorspace value from user-space is valid :-(
> > 
> > If this patch is to be dropped, and I'm fine with that I would like your
> > opinion on how I can still keep the VIN driver compatible with the
> > compliance tool. The option's I see are:
> > 
> > 1. Keep the patch [1] and accept that there is a need for a 0xff magic
> >    value. This 'works' but I don't think it's the correct solution.
> > 
> > 2. Move the check this patch tries to add a helper for directly to the
> >    VIN driver. This works but will require the driver to be updated if a
> >    new colorspace is added.
> > 
> > 3. Update createInvalidFmt() v4l2-compliance to not set the colorspace
> >    to 0xff but instead set it to V4L2_COLORSPACE_DEFAULT. A similar
> >    thing is already done for the filed in this function.
> > 	
> > 	memset(&fmt, 0xff, sizeof(fmt));
> > 	fmt.type = type;
> > 	fmt.fmt.pix.field = V4L2_FIELD_ANY;
> > 	...
> > 	
> >    I'm not sure if this is the best solution as it would not test
> >    drivers for what happens if they are presented with an invalid
> >    colorspace. But with such a change the driver can be written to still
> >    pass the test.
> > 
> > 4. Always forcing a specific colorspace (V4L2_COLORSPACE_SRGB) in the
> >    VIN format functions and ignoring what was requested by the user.
> >    
> >    I don't think this is good at all as it would be possible that
> >    pipeline validation fails due to a colorspace mismatch. This can be
> >    worked around by dropping the colorspace check from the VIN stream on
> >    function.
> > 
> > I'm sure there are other options open which I can't think of, in fact I
> > hope there are. I'm not over excited about any of the options above as
> > they all in one way or another just works around the problem of being
> > able to validate input from user-space. But I'm even less excited about
> > breaking v4l2-compliance compatibility so any path I can take here to
> > keep the user being able to specify the colorspace and v4l2-compliance
> > being happy would be a better solution for me :-)
> > 
> > 1. https://patchwork.linuxtv.org/patch/46717/
> > 2. 432d9ebfcea65337 ("v4l2-compliance: ignore colorspace tests for passthu
> > subdevs")
> 
> I do not want to relax the test for colorspace for the v4l2_pix_format in
> the compliance test. That test is good and should remain.
> 
> For the input pad format there are two issues:

There's no input pad format :-)

> 1) Setting the initial colorspace information. If you open the v4l-subdev
> device then the initial colorspace data is likely all 0. That's wrong for
> the colorspace since 0 is not allowed. So init_cfg should set it to
> something non-0 (I'd expect either SRGB or RAW). Or perhaps this should be
> done in the core as a standard initialization in the absence of the
> init_cfg op (or before it is called).
> 
> An alternative is to allow a 0 (DEFAULT) value here, but then what do you
> use in v4l2_pix_format? That definitely can't be 0. I think I prefer to
> always have something explicit here.
> 
> 2) Validation of the colorspace fields when setting the format on the input
> pad. After thinking about this some more I believe that the only reasonable
> thing you can do here is to indeed validate it based on the current range
> of known colorspaces (or more strict of course if the hardware has
> limitations).
> 
> But rather than adding validate defines I would just add something like this
> to the colorspace enum:
> 
> 	/* Update when a new colorspace is added */
> 	V4L2_COLORSPACE_LAST        = V4L2_COLORSPACE_DCI_P3
> 
> and do the same for the other colorspace-related enums.
> 
> In v4l2-subdev.c you can then check and validate this. Anything out-of-range
> would map to COLORSPACE_SRGB/RAW or 0 (DEFAULT) for the other fields.
> 
> I think SRGB is best here since it is the most widely used and understood
> colorspace.
> 
> Another issue is pipeline validation. See the link to the RFC patch above.
> The biggest issue here is that filling in these fields has been hit-and-miss
> and you don't want things to suddenly fail.
> 
> If the core validates/initializes these fields, then at least we always see
> something valid here (i.e. never 0 or > V4L2_COLORSPACE_LAST for the
> colorspace). I'd have to think about it some more.
> 
> This whole mess once again shows the importance of good compliance tests,
> especially for complex APIs like this. It forces you to think about how
> this should be handled instead of doing some vague hand-waving.

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent and Hans,

On Wed, Feb 21, 2018 at 10:16:25PM +0200, Laurent Pinchart wrote:
> No, I'm sorry, for MC-based drivers this isn't correct. The media entity that 
> symbolizes the DMA engine indeed has a sink pad, but it's a video node, not a 
> subdev. It thus has no media bus format configured for its sink pad. The 
> closest pad in the pipeline that has a media bus format is the source pad of 
> the subdev connected to the video node.
> 
> There's no communication within the kernel at G/S_FMT time between the video 
> node and its connected subdev. The only time we look at the pipeline as a 
> whole is when starting the stream to validate that the pipeline is correctly 
> configured. We thus have to implement G/S_FMT on the video node without any 
> knowledge about the connected subdev, and thus accept any colorspace.

A few more notes related to this --- there's no propagation of sub-device
format across the entities; there were several reasons for the design
choice. The V4L2 pixel format in the video node must be compatible with the
sub-device format when streaming is started. And... the streaming will
start in the pipeline defined by the enabled links to/from the video node.
In principle nothign prevents having multiple links there, and at the time
S_FMT IOCTL is called on the video node, none of those links could be
enabled. And that's perfectly valid use of the API.

A lot of the DMA engine drivers are simply devices that receive data and
write that to system memory, they really don't necessarily know anything
else. For the hardware this data is just pixels (or even bits, especially
in the case of CSI-2!).

So I agree with Laurent here that requiring correct colour space for
[GS]_FMT IOCTLs on video nodes in the general case is not feasible
(especially on MC-centric devices), due to the way the Media controller
pipeline and formats along that pipeline are configured and validated (i.e.
at streamon time).

But what to do here then? The colourspace field is not verified even in
link validation so there's no guarantee it's correctly set more or less
anywhere else than in the source of the stream. And if the stream has
multiple sources with different colour spaces, then what do you do? That's
perhaps a theoretical case but the current frameworks and APIs do in
principle support that.

Perhaps we should specify that the user should always set the same
colourspace on the sink end of a link that was there in the source? The
same should likely apply to the rest of the fields apart from width, height
and code, too. Before the user configures formats this doesn't work though,
and this does not address the matter with v4l2-compliance.

Granted that the drivers will themselves handle the colour space
information correctly, it'd still provide a way for the user to gain the
knowledge of the colour space which I believe is what matters.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

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

[Hans Verkuil]

On 02/21/2018 09:16 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Tuesday, 20 February 2018 10:37:22 EET Hans Verkuil wrote:
>> On 02/19/2018 11:28 PM, Niklas Söderlund wrote:
>>> Hi Hans,
>>>
>>> Thanks for your feedback.
>>>
>>> [snip]
>>>
>>>>>>>> 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).> 
>>> I'm afraid the issue described above by Laurent is what sparked me to
>>> write this commit to begin with. In my never ending VIN Gen3 patch-set I
>>> currency need to carry a patch [1] to implement a hack to make sure
>>> v4l2-compliance do not fail for the VIN Gen3 MC-centric use-case. This
>>> patch was an attempt to be able to validate the colorspace using the
>>> magic value 0xff.
>>
>> This is NOT a magic value. The test that's done here is to memset the
>> format structure with 0xff, then call the ioctl. Afterwards it checks
>> if there are any remaining 0xff bytes left in the struct since it expects
>> the driver to have overwritten it by something else. That's where the 0xff
>> comes from.
> 
> It's no less or more magic than using 0xdeadbeef or any fixed value :-) I 
> think we all agree that it isn't a value that is meant to be handled 
> specifically by drivers, so it's not magic in that sense.
> 
>>> I don't feel strongly for this patch in particular and I'm happy to drop
>>> it.  But I would like to receive some guidance on how to then properly
>>> be able to handle this problem for the MC-centric VIN driver use-case.
>>> One option is as you suggested to relax the test in v4l-compliance to
>>> not check colorspace, but commit [2] is not enough to resolve the issue
>>> for my MC use-case.
>>>
>>> As Laurent stated above, the use-case is that the video device shall
>>> accept any colorspace set from user-space. This colorspace is then only
>>> used as stream on time to validate the MC pipeline. The VIN driver do
>>> not care about colorspace, but I care about not breaking v4l2-compliance
>>> as I find it's a very useful tool :-)
>>
>> I think part of my confusion here is that there are two places where you
>> deal with colorspaces in a DMA engine: first there is a input pad of the
>> DMA engine entity, secondly there is the v4l2_pix_format for the memory
>> description.
>>
>> The second is set by the driver based on what userspace specified for the
>> input pad, together with any changes due to additional conversions such
>> as quantization range and RGB <-> YUV by the DMA engine.
> 
> No, I'm sorry, for MC-based drivers this isn't correct. The media entity that 
> symbolizes the DMA engine indeed has a sink pad, but it's a video node, not a 
> subdev. It thus has no media bus format configured for its sink pad. The 
> closest pad in the pipeline that has a media bus format is the source pad of 
> the subdev connected to the video node.
> 
> There's no communication within the kernel at G/S_FMT time between the video 
> node and its connected subdev. The only time we look at the pipeline as a 
> whole is when starting the stream to validate that the pipeline is correctly 
> configured. We thus have to implement G/S_FMT on the video node without any 
> knowledge about the connected subdev, and thus accept any colorspace.
> 
>> So any colorspace validation is done for the input pad. The question is
>> what that validation should be. It's never been defined.
> 
> No format is set on the video node's entity sink pad for the reason above, so 
> no validation occurs when setting the colorspace on the sink pad as that never 
> happens.

Is this documented anywhere? Certainly VIDIOC_G/S/TRY_FMT doesn't mention it.

It is certainly a surprise to me.

Regards,

	Hans

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

[Hans Verkuil]

On 02/21/2018 10:01 PM, Sakari Ailus wrote:
> Hi Laurent and Hans,
> 
> On Wed, Feb 21, 2018 at 10:16:25PM +0200, Laurent Pinchart wrote:
>> No, I'm sorry, for MC-based drivers this isn't correct. The media entity that 
>> symbolizes the DMA engine indeed has a sink pad, but it's a video node, not a 
>> subdev. It thus has no media bus format configured for its sink pad. The 
>> closest pad in the pipeline that has a media bus format is the source pad of 
>> the subdev connected to the video node.
>>
>> There's no communication within the kernel at G/S_FMT time between the video 
>> node and its connected subdev. The only time we look at the pipeline as a 
>> whole is when starting the stream to validate that the pipeline is correctly 
>> configured. We thus have to implement G/S_FMT on the video node without any 
>> knowledge about the connected subdev, and thus accept any colorspace.
> 
> A few more notes related to this --- there's no propagation of sub-device
> format across the entities; there were several reasons for the design
> choice. The V4L2 pixel format in the video node must be compatible with the
> sub-device format when streaming is started. And... the streaming will
> start in the pipeline defined by the enabled links to/from the video node.
> In principle nothign prevents having multiple links there, and at the time
> S_FMT IOCTL is called on the video node, none of those links could be
> enabled. And that's perfectly valid use of the API.
> 
> A lot of the DMA engine drivers are simply devices that receive data and
> write that to system memory, they really don't necessarily know anything
> else. For the hardware this data is just pixels (or even bits, especially
> in the case of CSI-2!).

Not in my experience. Most DMA engines I've ever worked with can do at
least quantization and RGB <-> YUV conversions. Which is pretty much
required functionality if you work with video receivers.

And in order to program that correctly in the DMA engine you have to
know what you receive.

Full-fledged colorspace converters that can convert between different
colorspaces and transfer functions are likely to be separate subdevs
due to the complexity of that.

> So I agree with Laurent here that requiring correct colour space for
> [GS]_FMT IOCTLs on video nodes in the general case is not feasible
> (especially on MC-centric devices), due to the way the Media controller
> pipeline and formats along that pipeline are configured and validated (i.e.
> at streamon time).
> 
> But what to do here then? The colourspace field is not verified even in
> link validation so there's no guarantee it's correctly set more or less
> anywhere else than in the source of the stream. And if the stream has
> multiple sources with different colour spaces, then what do you do? That's
> perhaps a theoretical case but the current frameworks and APIs do in
> principle support that.

It's not theoretical at all. But anyway, in that case it is up to userspace
to decide. A typical example is an sRGB OSD on top of a Rec.709 video.

In practice the differences in color at too small to be a problem, you'd
just use Rec. 709 and slight color differences in the sRGB OSD is not something
that is noticeable. But with HDR and BT.2020 this becomes much more complicated.

However, that is out of scope of the kernel driver.

> 
> Perhaps we should specify that the user should always set the same
> colourspace on the sink end of a link that was there in the source? The
> same should likely apply to the rest of the fields apart from width, height
> and code, too. Before the user configures formats this doesn't work though,
> and this does not address the matter with v4l2-compliance.
> 
> Granted that the drivers will themselves handle the colour space
> information correctly, it'd still provide a way for the user to gain the
> knowledge of the colour space which I believe is what matters.
> 

Urgh. It's really wrong IMHO that the DMA engine's input pad can't be
configured. It's inconsistent. I don't think we ever thought this through
properly.

Let me first fix all the other compliance issues and then I'll have to get
back to this. It's a mess.

Regards,

	Hans

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

[Laurent Pinchart]

Hi Hans,

On Thursday, 22 February 2018 10:01:13 EET Hans Verkuil wrote:
> On 02/21/2018 10:01 PM, Sakari Ailus wrote:
> > On Wed, Feb 21, 2018 at 10:16:25PM +0200, Laurent Pinchart wrote:
> >> No, I'm sorry, for MC-based drivers this isn't correct. The media entity
> >> that symbolizes the DMA engine indeed has a sink pad, but it's a video
> >> node, not a subdev. It thus has no media bus format configured for its
> >> sink pad. The closest pad in the pipeline that has a media bus format is
> >> the source pad of the subdev connected to the video node.
> >> 
> >> There's no communication within the kernel at G/S_FMT time between the
> >> video node and its connected subdev. The only time we look at the
> >> pipeline as a whole is when starting the stream to validate that the
> >> pipeline is correctly configured. We thus have to implement G/S_FMT on
> >> the video node without any knowledge about the connected subdev, and
> >> thus accept any colorspace.
> > 
> > A few more notes related to this --- there's no propagation of sub-device
> > format across the entities; there were several reasons for the design
> > choice. The V4L2 pixel format in the video node must be compatible with
> > the sub-device format when streaming is started. And... the streaming will
> > start in the pipeline defined by the enabled links to/from the video node.
> > In principle nothign prevents having multiple links there, and at the time
> > S_FMT IOCTL is called on the video node, none of those links could be
> > enabled. And that's perfectly valid use of the API.
> > 
> > A lot of the DMA engine drivers are simply devices that receive data and
> > write that to system memory, they really don't necessarily know anything
> > else. For the hardware this data is just pixels (or even bits, especially
> > in the case of CSI-2!).
> 
> Not in my experience. Most DMA engines I've ever worked with can do at
> least quantization and RGB <-> YUV conversions. Which is pretty much
> required functionality if you work with video receivers.

That's because you have a coarser view of the hardware in the PC world, where 
the RGB <-> YUV converter and DMA engine are bundled in a single block in 
documentation. In the embedded world a DMA engine is just a DMA engine. There 
is in many cases a RGB <-> YUV converter just before the DMA engine, but it's 
then usually exposed as a subdevice separate from the video node.

> And in order to program that correctly in the DMA engine you have to
> know what you receive.

Depending on the hardware I'm fine letting driver authors decide whether to 
expose the RGB <-> YUV conversion as a separate subdev, or as part of the 
video node. In the latter case the video node implementation will need to 
program the RGB <-> YUV conversion hardware based on the output pixel format 
and input media bus format. This can be done without any issue at stream on 
time: the video node implementation can access the format of the connected 
source pad in the pipeline when starting the stream, as links are configured 
and frozen at that time. What the video node implementation can't do is access 
that information at G/S_FMT time, as G/S_FMT on both subdevs and video nodes 
must be contained to a single entity when using MC-based drivers.

> Full-fledged colorspace converters that can convert between different
> colorspaces and transfer functions are likely to be separate subdevs
> due to the complexity of that.
> 
> > So I agree with Laurent here that requiring correct colour space for
> > [GS]_FMT IOCTLs on video nodes in the general case is not feasible
> > (especially on MC-centric devices), due to the way the Media controller
> > pipeline and formats along that pipeline are configured and validated
> > (i.e. at streamon time).
> > 
> > But what to do here then? The colourspace field is not verified even in
> > link validation so there's no guarantee it's correctly set more or less
> > anywhere else than in the source of the stream. And if the stream has
> > multiple sources with different colour spaces, then what do you do? That's
> > perhaps a theoretical case but the current frameworks and APIs do in
> > principle support that.
> 
> It's not theoretical at all. But anyway, in that case it is up to userspace
> to decide. A typical example is an sRGB OSD on top of a Rec.709 video.
> 
> In practice the differences in color at too small to be a problem, you'd
> just use Rec. 709 and slight color differences in the sRGB OSD is not
> something that is noticeable. But with HDR and BT.2020 this becomes much
> more complicated.
> 
> However, that is out of scope of the kernel driver.

What I believe should be in scope for the kernel driver is reporting correct 
colorspace information. Once a pipeline is correctly configured for streaming, 
the colorspace reported by G_FMT on a capture video node should correspond to 
the colorspace of the data that will be written to memory. That's why we 
validate pipelines at stream on time: even if the device would work exactly 
the same way with a wrong colorspace set on the video node, an application 
that calls G_FMT would see a colorspace that doesn't correspond to reality. Of 
course the application can walk up the pipeline and check the colorspace on 
the sensor at the input, but I think we need to keep parameters valid across 
the whole pipeline to avoid confusion in userspace.

To summarize my position, in the VIN case, we need to accept any colorspace at 
S_FMT time, as the DMA engine is colorspace-agnostic and doesn't know at S_FMT 
time what it will receive, and validate the configured colorspace at stream on 
time to ensure that userspace receives consistent information.

> > Perhaps we should specify that the user should always set the same
> > colourspace on the sink end of a link that was there in the source? The
> > same should likely apply to the rest of the fields apart from width,
> > height and code, too. Before the user configures formats this doesn't work
> > though, and this does not address the matter with v4l2-compliance.
> > 
> > Granted that the drivers will themselves handle the colour space
> > information correctly, it'd still provide a way for the user to gain the
> > knowledge of the colour space which I believe is what matters.
> 
> Urgh. It's really wrong IMHO that the DMA engine's input pad can't be
> configured. It's inconsistent. I don't think we ever thought this through
> properly.

The video node is an entity but isn't a subdev, so it has pads, but no media 
bus format exposed there. And frankly I don't think that's an issue. Today we 
do the following.

1. In the VIDIOC_S_FMT handler, accept any colorspace, as we can't know what 
we will be fed (as explained above and a few times before, S_FMT is contained 
in a single entity, it doesn't follow links inside the kernel).

2. At stream on time, validate the input link to verify that the colorspace on 
the connected subdev's source pad is identical to the colorspace on the video 
node.

If we could configure the media bus format on the sink pad of the video node, 
we would

1. Require userspace to set the format on the sink pad. We would need to 
accept any colorspace there too (again we can't know what we will be fed).

2. In the VIDIOC_S_FMT handler, validate the passed colorspace to verify it is 
identical to the colorspace configured on the sink pad.

3. At stream on time, validate the input link to verify that the colorspace on 
the connected subdev's source pad is identical to the colorspace on the video 
node's sink pad.

Why would that be better ? In both cases we need to accept any colorspace at 
S_FMT time (on the video node in the first case, on the video node's sink pad 
in the second case) and validate it at stream on time.

> Let me first fix all the other compliance issues and then I'll have to get
> back to this. It's a mess.

How do we move forward with VIN though ? I don't think we should block the 
driver due to this.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Hans,

On Thursday, 22 February 2018 09:38:46 EET Hans Verkuil wrote:
> On 02/21/2018 09:16 PM, Laurent Pinchart wrote:
> > On Tuesday, 20 February 2018 10:37:22 EET Hans Verkuil wrote:
> >> On 02/19/2018 11:28 PM, Niklas Söderlund wrote:
> >>> Hi Hans,
> >>> 
> >>> Thanks for your feedback.
> >>> 
> >>> [snip]
> >>> 
> >>>>>>>> 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).
> >>> 
> >>> I'm afraid the issue described above by Laurent is what sparked me to
> >>> write this commit to begin with. In my never ending VIN Gen3 patch-set I
> >>> currency need to carry a patch [1] to implement a hack to make sure
> >>> v4l2-compliance do not fail for the VIN Gen3 MC-centric use-case. This
> >>> patch was an attempt to be able to validate the colorspace using the
> >>> magic value 0xff.
> >> 
> >> This is NOT a magic value. The test that's done here is to memset the
> >> format structure with 0xff, then call the ioctl. Afterwards it checks
> >> if there are any remaining 0xff bytes left in the struct since it expects
> >> the driver to have overwritten it by something else. That's where the
> >> 0xff comes from.
> > 
> > It's no less or more magic than using 0xdeadbeef or any fixed value :-) I
> > think we all agree that it isn't a value that is meant to be handled
> > specifically by drivers, so it's not magic in that sense.
> > 
> >>> I don't feel strongly for this patch in particular and I'm happy to drop
> >>> it.  But I would like to receive some guidance on how to then properly
> >>> be able to handle this problem for the MC-centric VIN driver use-case.
> >>> One option is as you suggested to relax the test in v4l-compliance to
> >>> not check colorspace, but commit [2] is not enough to resolve the issue
> >>> for my MC use-case.
> >>> 
> >>> As Laurent stated above, the use-case is that the video device shall
> >>> accept any colorspace set from user-space. This colorspace is then only
> >>> used as stream on time to validate the MC pipeline. The VIN driver do
> >>> not care about colorspace, but I care about not breaking v4l2-compliance
> >>> as I find it's a very useful tool :-)
> >> 
> >> I think part of my confusion here is that there are two places where you
> >> deal with colorspaces in a DMA engine: first there is a input pad of the
> >> DMA engine entity, secondly there is the v4l2_pix_format for the memory
> >> description.
> >> 
> >> The second is set by the driver based on what userspace specified for the
> >> input pad, together with any changes due to additional conversions such
> >> as quantization range and RGB <-> YUV by the DMA engine.
> > 
> > No, I'm sorry, for MC-based drivers this isn't correct. The media entity
> > that symbolizes the DMA engine indeed has a sink pad, but it's a video
> > node, not a subdev. It thus has no media bus format configured for its
> > sink pad. The closest pad in the pipeline that has a media bus format is
> > the source pad of the subdev connected to the video node.
> > 
> > There's no communication within the kernel at G/S_FMT time between the
> > video node and its connected subdev. The only time we look at the
> > pipeline as a whole is when starting the stream to validate that the
> > pipeline is correctly configured. We thus have to implement G/S_FMT on
> > the video node without any knowledge about the connected subdev, and thus
> > accept any colorspace.
> > 
> >> So any colorspace validation is done for the input pad. The question is
> >> what that validation should be. It's never been defined.
> > 
> > No format is set on the video node's entity sink pad for the reason above,
> > so no validation occurs when setting the colorspace on the sink pad as
> > that never happens.
> 
> Is this documented anywhere? Certainly VIDIOC_G/S/TRY_FMT doesn't mention
> it.
> 
> It is certainly a surprise to me.

We don't document as such that no format is set on the video node's entity 
sink pad, no. I've always considered that as implicit given that we don't 
expose an API to configure a format there :-)

-- 
Regards,

Laurent Pinchart