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
