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
