From: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
To: hverkuil-cisco@xs4all.nl
Cc: linux-media@vger.kernel.org,
Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Subject: Re: [PATCH v5.1 1/2] vb2: add requires_requests bit for stateless codecs
Date: Wed, 20 Mar 2019 09:55:01 -0300 [thread overview]
Message-ID: <20190320095501.62ff031e@coco.lan> (raw)
In-Reply-To: <20190320123305.5224-2-hverkuil-cisco@xs4all.nl>
Em Wed, 20 Mar 2019 13:33:04 +0100
hverkuil-cisco@xs4all.nl escreveu:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>
> Stateless codecs require the use of the Request API as opposed of it
> being optional.
>
> So add a bit to indicate this and let vb2 check for this.
>
> If an attempt is made to queue a buffer without an associated request,
> then the EBADR error is returned to userspace.
>
> Doing this check in the vb2 core simplifies drivers, since they
> don't have to check for this, they can just set this flag.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
> Documentation/media/uapi/v4l/vidioc-qbuf.rst | 4 ++++
> drivers/media/common/videobuf2/videobuf2-core.c | 9 +++++++++
> drivers/media/common/videobuf2/videobuf2-v4l2.c | 4 ++++
> include/media/videobuf2-core.h | 3 +++
> 4 files changed, 20 insertions(+)
>
> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> index c138d149faea..5739c3676062 100644
> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> @@ -189,6 +189,10 @@ EACCES
> The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not
> support requests for the given buffer type.
>
> +EBADR
> + The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was not set but the device requires
> + that the buffer is part of a request.
> +
Hmm... IMO, you should replace the previous text instead:
EACCES
The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not
support requests for the given buffer type.
Also, I would replace:
device -> device driver
As this ia a device driver limitation of the current implementation,
with may or may not reflect a hardware limitation.
> EBUSY
> The first buffer was queued via a request, but the application now tries
> to queue it directly, or vice versa (it is not permitted to mix the two
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 678a31a2b549..b98ec6e1a222 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1507,6 +1507,12 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
>
> vb = q->bufs[index];
>
> + if (!req && vb->state != VB2_BUF_STATE_IN_REQUEST &&
> + q->requires_requests) {
> + dprintk(1, "qbuf requires a request\n");
> + return -EBADR;
> + }
> +
> if ((req && q->uses_qbuf) ||
> (!req && vb->state != VB2_BUF_STATE_IN_REQUEST &&
> q->uses_requests)) {
> @@ -2238,6 +2244,9 @@ int vb2_core_queue_init(struct vb2_queue *q)
> WARN_ON(!q->ops->buf_queue))
> return -EINVAL;
>
> + if (WARN_ON(q->requires_requests && !q->supports_requests))
> + return -EINVAL;
> +
Shouldn't it also be EBADR?
> INIT_LIST_HEAD(&q->queued_list);
> INIT_LIST_HEAD(&q->done_list);
> spin_lock_init(&q->done_lock);
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 74d3abf33b50..84de18b30a95 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -381,6 +381,10 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
> return 0;
>
> if (!(b->flags & V4L2_BUF_FLAG_REQUEST_FD)) {
> + if (q->requires_requests) {
> + dprintk(1, "%s: queue requires requests\n", opname);
> + return -EBADR;
> + }
> if (q->uses_requests) {
> dprintk(1, "%s: queue uses requests\n", opname);
> return -EBUSY;
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index c02af6370e9b..fe010ad62b90 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -482,6 +482,8 @@ struct vb2_buf_ops {
> * has not been called. This is a vb1 idiom that has been adopted
> * also by vb2.
> * @supports_requests: this queue supports the Request API.
> + * @requires_requests: this queue requires the Request API. If this is set to 1,
> + * then supports_requests must be set to 1 as well.
> * @uses_qbuf: qbuf was used directly for this queue. Set to 1 the first
> * time this is called. Set to 0 when the queue is canceled.
> * If this is 1, then you cannot queue buffers from a request.
> @@ -556,6 +558,7 @@ struct vb2_queue {
> unsigned allow_zero_bytesused:1;
> unsigned quirk_poll_must_check_waiting_for_buffers:1;
> unsigned supports_requests:1;
> + unsigned requires_requests:1;
> unsigned uses_qbuf:1;
> unsigned uses_requests:1;
>
Thanks,
Mauro
next prev parent reply other threads:[~2019-03-20 12:55 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-20 12:33 [PATCH v5.1 0/2] add requires_requests bit for stateless codecs hverkuil-cisco
2019-03-20 12:33 ` [PATCH v5.1 1/2] vb2: " hverkuil-cisco
2019-03-20 12:55 ` Mauro Carvalho Chehab [this message]
2019-03-20 13:06 ` Hans Verkuil
2019-03-20 13:22 ` Mauro Carvalho Chehab
2019-03-20 12:33 ` [PATCH v5.1 2/2] cedrus: set requires_requests hverkuil-cisco
2019-03-20 12:55 ` Mauro Carvalho Chehab
2019-03-20 14:23 ` [PATCH v5.1 3/2] media requests: return EBADR instead of EACCES Hans Verkuil
2019-03-20 14:28 ` Mauro Carvalho Chehab
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=20190320095501.62ff031e@coco.lan \
--to=mchehab+samsung@kernel.org \
--cc=hverkuil-cisco@xs4all.nl \
--cc=linux-media@vger.kernel.org \
--cc=paul.kocialkowski@bootlin.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).