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 ? -- Regards, Laurent Pinchart
