From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Sakari Ailus <sakari.ailus@linux.intel.com>,
linux-media@vger.kernel.org
Subject: Re: vb2_queue type question
Date: Fri, 26 Mar 2021 11:57:40 +0200 [thread overview]
Message-ID: <a9325fd6-8f14-ced5-97c4-3d05dc7ab150@ideasonboard.com> (raw)
In-Reply-To: <67107267-69c8-d87d-6579-5e7dac0400fb@xs4all.nl>
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
next prev parent reply other threads:[~2021-03-26 9:58 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-22 9:18 vb2_queue type question Tomi Valkeinen
2021-03-22 9:49 ` Hans Verkuil
2021-03-22 9:59 ` Laurent Pinchart
2021-03-22 10:08 ` Hans Verkuil
2021-03-22 10:30 ` Tomi Valkeinen
2021-03-26 9:57 ` Tomi Valkeinen [this message]
2021-03-26 10:18 ` Hans Verkuil
2021-03-30 8:18 ` Tomi Valkeinen
2021-03-30 8:56 ` Hans Verkuil
2021-03-31 5:22 ` Tomi Valkeinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a9325fd6-8f14-ced5-97c4-3d05dc7ab150@ideasonboard.com \
--to=tomi.valkeinen@ideasonboard.com \
--cc=hverkuil@xs4all.nl \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=sakari.ailus@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).