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
