Re: vb2_queue type question

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

Hi Hans,

On 22/03/2021 11:49, 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.

So what's the expected behavior here if I have both normal video ops 
(vidioc_g_fmt_vid_cap & co.) and metadata ops (vidioc_g_fmt_meta_cap & 
co.) defined?

I can change the queue type in s_fmt as you suggest above, but what 
should, say, vidioc_g_fmt_meta_cap return if the current format is not 
metadata format? I made it return -EINVAL, but then v4l2-compliance says 
"Metadata Capture G_FMT failed, but Metadata Capture formats defined". I 
could also make vidioc_enum_fmt_meta_cap return an error, but then it 
would look like no metadata is supported.

Should all the metadata ops always change the queue type? That would 
prevent g_fmt from working when the queue is busy.

  Tomi