On 22/03/2021 11:49, Hans Verkuil wrote:

On 22/03/2021 10:18, Tomi Valkeinen wrote:

quoted

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.

Thanks, this works fine and is simple to manage.

  Tomi