Re: vb2_queue type question

[Hans Verkuil <hverkuil@xs4all.nl>]

On 26/03/2021 10:57, Tomi Valkeinen wrote:
> 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.

No, the format ioctls are not changed.

The only thing that you do is update the queue type when you set the
video or metadata format. When you start allocating buffers the queue type
of the last set format is used. At that moment any attempt to set the format
to another type will fail since vb2_is_busy(queue) will be true.

So only the s_fmt ioctl will change the type. The g/try_fmt ioctls just must
keep working as-is.

Regards,

	Hans

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