Re: vb2_queue type question

[Hans Verkuil <hverkuil@xs4all.nl>]

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