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 

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: 

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

[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

[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.ai...@iki.fi

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 

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 

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 

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
> >> 
> >> ---
> >> 
> >>  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 
> >>> 
> >>> Can you then fix v4l2-compliance to stop testing colorspace against 0xff
> >>> ?
> >> 
> >> For 

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

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

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

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

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

  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 
> 
> 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 
>>>
  /*
   * 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 
> >> ---
> >> 
> >>  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 

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 
> > 
> >>  /*
> >>   * 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 
>> ---
>>  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 

Sorry,

Hans

>> +
> 
> Casting to u32 has the added benefit that the colorspace expression is 
> evaluated once only, I like that.
> 
> Reviewed-by: Laurent Pinchart 
> 
>>  /*
>>   * 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 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 
> ---
>  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 

>  /*
>   * 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

[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 

Acked-by: Sakari Ailus 

> ---
>  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.ai...@iki.fi

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