On 22/03/2021 10:59, Laurent Pinchart wrote: > Hi Hans, > > On Mon, Mar 22, 2021 at 10:49:26AM +0100, Hans Verkuil wrote: >> On 22/03/2021 10:18, Tomi Valkeinen wrote: >>> Hi Hans, >>> >>> We were discussing this with Laurent and Sakari, I thought I'd ask if >>> you have any feedback on this. >>> >>> struct vb2_queue has 'type' field, so you can only use a queue for >>> buffers of a single type. struct video_device has 'queue' field, so you >>> can only use a single queue for a video_device instance. >>> >>> TI's SoCs have a CSI-2 receiver, with a bunch of DMA engines. The HW >>> doesn't care if we are currently capturing pixel buffers or metadata >>> buffers (I don't have experience with other HW, but I imagine this >>> shouldn't be a rare case). However, due to vb2_queue, the driver needs >>> to decide which one to support, which limits the possible use cases. >>> >>> I was browsing the code, and afaics the type field doesn't do much. It >>> is, of course, used to reject queuing buffers of wrong type, and also >>> (mostly in mem-2-mem code) to find out if functions are called in input >>> or output context. >>> >>> The latter one could be easily removed by just comparing the given queue >>> pointer to a stored pointer (e.g. queue == priv->input_queue). >>> >>> Do you see any problems if we were to change the type field to >>> type_mask, allowing multiple buffer types per queue? Or even remove the >>> vb2_queue->type. This raises some questions, like should a queue contain >>> only buffers of a single type or can it contain a mix of buffers (I >>> think it shouldn't contain a mix of buffers), or can a queue's type_mask >>> contain both input and output types (I don't see why not). >>> >>> An alternate which I tried was creating two vb2_queues, and switching >>> the video_device->queue at runtime based on set_format. It kind of >>> works, but I think the behavior is a bit unclear, and it might be >>> difficult to catch all the corner cases. >> >> A vb2_queue basically represents a buffer queue that will be fed to a >> DMA engine. It assumes that all the buffers are of the same format, >> which typically is tied directly to the type. >> >> The type of a vb2_queue can be changed if you like, but once buffers >> are allocated it is fixed and can't be changed again until all buffers >> are released. So you can't mix buffers of different types. >> >> This is actually done in the vivid driver: see vidioc_s_fmt_vbi_cap() >> and vidioc_s_fmt_sliced_vbi_cap(): depending on the format the queue >> type will be set to either capture raw or sliced VBI. >> >> The ivtv driver does the same thing. >> >> So as long as vb2_is_busy() returns false, you are free to change the >> queue type. >> >> There is no need for a type_mask or anything like that. That's up to >> the bridge driver to check. The vb2_queue type is there to ensure that >> userspace isn't trying to mix buffers of different types, but as long >> as no buffers are allocated it doesn't do anything and you are free to >> change it. > > I wasn't aware of this design rationale. It would be useful to expand > the documentation of vb2_queue.type to document this. Or have I missed a > different location where this is already explained ? > It's probably not explained anywhere. It such a rare corner case that nobody bothered. If this is going to be used more often, then I think it would be a good idea to make a proper vb2 helper function through which you can change the type, and that will also check with vb2_is_busy() if you are allowed to do that. Then that function can be documented in the header. Changing types may also require you to change other vb2_queue fields as well, but that's the responsibility of the bridge driver as well. For this particular use-case I would expect that only the type field requires changing. Everything else would stay the same, except perhaps for the vb2_ops. Regards, Hans
