[PATCH v5.1 0/2] add requires_requests bit for stateless codecs

[PATCH v5.1 0/2] add requires_requests bit for stateless codecs

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

These two patches replace patches 1-3 of the vicodec v5 patch
series: https://www.spinics.net/lists/linux-media/msg147923.html

The original patch 2/23 (videodev2.h: add V4L2_BUF_CAP_REQUIRES_REQUESTS)
was dropped. This needs a bit more thought.

Patch 1 now returns EBADR if an attempt is made to queue a buffer
without a request and requires_requests is set. This error code is
also documented.

Both patches have a better (hopefully) commit log.

The cedrus patch itself is unchanged from v5.

Regards,

	Hans

Hans Verkuil (2):
  vb2: add requires_requests bit for stateless codecs
  cedrus: set requires_requests

 Documentation/media/uapi/v4l/vidioc-qbuf.rst      | 4 ++++
 drivers/media/common/videobuf2/videobuf2-core.c   | 9 +++++++++
 drivers/media/common/videobuf2/videobuf2-v4l2.c   | 4 ++++
 drivers/staging/media/sunxi/cedrus/cedrus_video.c | 1 +
 include/media/videobuf2-core.h                    | 3 +++
 5 files changed, 21 insertions(+)

-- 
2.20.1

[PATCH v5.1 1/2] vb2: add requires_requests bit for stateless codecs

[hverkuil-cisco]

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.
+
 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;
+
 	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;
 
-- 
2.20.1

[PATCH v5.1 2/2] cedrus: set requires_requests

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

The cedrus stateless decoder requires the use of request, so
indicate this by setting requires_requests to 1.

Note that the cedrus driver never checked for this, and as far
as I can tell would just crash if an attempt was made to queue
a buffer without a request.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/staging/media/sunxi/cedrus/cedrus_video.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index b47854b3bce4..9673874ece10 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -536,6 +536,7 @@ int cedrus_queue_init(void *priv, struct vb2_queue *src_vq,
 	src_vq->lock = &ctx->dev->dev_mutex;
 	src_vq->dev = ctx->dev->dev;
 	src_vq->supports_requests = true;
+	src_vq->requires_requests = true;
 
 	ret = vb2_queue_init(src_vq);
 	if (ret)
-- 
2.20.1

Re: [PATCH v5.1 1/2] vb2: add requires_requests bit for stateless codecs

[Mauro Carvalho Chehab]

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

Re: [PATCH v5.1 2/2] cedrus: set requires_requests

[Mauro Carvalho Chehab]

Em Wed, 20 Mar 2019 13:33:05 +0100
hverkuil-cisco@xs4all.nl escreveu:

> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> The cedrus stateless decoder requires the use of request, so
> indicate this by setting requires_requests to 1.
> 
> Note that the cedrus driver never checked for this, and as far
> as I can tell would just crash if an attempt was made to queue
> a buffer without a request.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus_video.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> index b47854b3bce4..9673874ece10 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> @@ -536,6 +536,7 @@ int cedrus_queue_init(void *priv, struct vb2_queue *src_vq,
>  	src_vq->lock = &ctx->dev->dev_mutex;
>  	src_vq->dev = ctx->dev->dev;
>  	src_vq->supports_requests = true;
> +	src_vq->requires_requests = true;

looks OK to my eyes.

>  
>  	ret = vb2_queue_init(src_vq);
>  	if (ret)



Thanks,
Mauro

Re: [PATCH v5.1 1/2] vb2: add requires_requests bit for stateless codecs

[Hans Verkuil]

On 3/20/19 1:55 PM, Mauro Carvalho Chehab wrote:
> 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.

No. This is already being returned, so changing this will be an API change.

That said, since the only drivers that can return this are vivid, vim2m and cedrus,
(i.e. test and staging drivers), I am OK to change this to EBADR as well.

In that case it would become:

EBADR
	The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device driver does
        not support requests for the given buffer type, or the
        ``V4L2_BUF_FLAG_REQUEST_FD`` flag was not set but the device driver
        requires that the buffer is part of a request.

> 
> 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?

No, this checks that the driver doesn't set requires_requests without
also setting supports_requests. I.e. this indicates a driver bug, hence
the WARN_ON. Requiring requests, but not supporting them makes obviously
no sense.

Regards,

	Hans

> 
>>  	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
> 

Re: [PATCH v5.1 1/2] vb2: add requires_requests bit for stateless codecs

[Mauro Carvalho Chehab]

Em Wed, 20 Mar 2019 14:06:51 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> On 3/20/19 1:55 PM, Mauro Carvalho Chehab wrote:
> > 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.  
> 
> No. This is already being returned, so changing this will be an API change.
> 
> That said, since the only drivers that can return this are vivid, vim2m and cedrus,
> (i.e. test and staging drivers), I am OK to change this to EBADR as well.
> 
> In that case it would become:
> 
> EBADR
> 	The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device driver does
>         not support requests for the given buffer type, or the
>         ``V4L2_BUF_FLAG_REQUEST_FD`` flag was not set but the device driver
>         requires that the buffer is part of a request.

Ok, let's do that, as, IMHO, it makes it a lot more clear.

> 
> > 
> > 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?  
> 
> No, this checks that the driver doesn't set requires_requests without
> also setting supports_requests. I.e. this indicates a driver bug, hence
> the WARN_ON. Requiring requests, but not supporting them makes obviously
> no sense.

Ok.

Thanks,
Mauro

[PATCH v5.1 3/2] media requests: return EBADR instead of EACCES

[Hans Verkuil]

If requests are used when they shouldn't, or not used when they should, then
return EBADR (Invalid request descriptor) instead of EACCES.

The reason for this change is that EACCES has more to do with permissions
(not being the owner of the resource), but in this case the request file
descriptor is just wrong for the current mode of the device.

Update the documentation accordingly.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/Documentation/media/uapi/mediactl/request-api.rst b/Documentation/media/uapi/mediactl/request-api.rst
index 1ad631e549fe..a74c82d95609 100644
--- a/Documentation/media/uapi/mediactl/request-api.rst
+++ b/Documentation/media/uapi/mediactl/request-api.rst
@@ -93,7 +93,7 @@ A queued request cannot be modified anymore.
 .. caution::
    For :ref:`memory-to-memory devices <mem2mem>` you can use requests only for
    output buffers, not for capture buffers. Attempting to add a capture buffer
-   to a request will result in an ``EACCES`` error.
+   to a request will result in an ``EBADR`` error.

 If the request contains configurations for multiple entities, individual drivers
 may synchronize so the requested pipeline's topology is applied before the
diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index 81ffdcb89057..b9a2e9dc707c 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -326,7 +326,7 @@ struct v4l2_buffer
 	Applications should not set ``V4L2_BUF_FLAG_REQUEST_FD`` for any ioctls
 	other than :ref:`VIDIOC_QBUF <VIDIOC_QBUF>`.

-	If the device does not support requests, then ``EACCES`` will be returned.
+	If the device does not support requests, then ``EBADR`` will be returned.
 	If requests are supported but an invalid request file descriptor is
 	given, then ``EINVAL`` will be returned.

diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
index 5739c3676062..dbf7b445a27b 100644
--- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
+++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
@@ -111,7 +111,7 @@ in use. Setting it means that the buffer will not be passed to the driver
 until the request itself is queued. Also, the driver will apply any
 settings associated with the request for this buffer. This field will
 be ignored unless the ``V4L2_BUF_FLAG_REQUEST_FD`` flag is set.
-If the device does not support requests, then ``EACCES`` will be returned.
+If the device does not support requests, then ``EBADR`` will be returned.
 If requests are supported but an invalid request file descriptor is given,
 then ``EINVAL`` will be returned.

@@ -125,7 +125,7 @@ then ``EINVAL`` will be returned.

    For :ref:`memory-to-memory devices <mem2mem>` you can specify the
    ``request_fd`` only for output buffers, not for capture buffers. Attempting
-   to specify this for a capture buffer will result in an ``EACCES`` error.
+   to specify this for a capture buffer will result in an ``EBADR`` error.

 Applications call the ``VIDIOC_DQBUF`` ioctl to dequeue a filled
 (capturing) or displayed (output) buffer from the driver's outgoing
@@ -185,12 +185,10 @@ EPIPE
     codecs if a buffer with the ``V4L2_BUF_FLAG_LAST`` was already
     dequeued and no new buffers are expected to become available.

-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
+    The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not
+    support requests for the given buffer type, or
+    the ``V4L2_BUF_FLAG_REQUEST_FD`` flag was not set but the device requires
     that the buffer is part of a request.

 EBUSY
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 84de18b30a95..b11a779e97b0 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -392,7 +392,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
 		return 0;
 	} else if (!q->supports_requests) {
 		dprintk(1, "%s: queue does not support requests\n", opname);
-		return -EACCES;
+		return -EBADR;
 	} else if (q->uses_qbuf) {
 		dprintk(1, "%s: queue does not use requests\n", opname);
 		return -EBUSY;
diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
index eec2e2b2f6ec..ed87305b29f9 100644
--- a/drivers/media/media-request.c
+++ b/drivers/media/media-request.c
@@ -251,7 +251,7 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd)

 	if (!mdev || !mdev->ops ||
 	    !mdev->ops->req_validate || !mdev->ops->req_queue)
-		return ERR_PTR(-EACCES);
+		return ERR_PTR(-EBADR);

 	filp = fget(request_fd);
 	if (!filp)
@@ -407,7 +407,7 @@ int media_request_object_bind(struct media_request *req,
 	int ret = -EBUSY;

 	if (WARN_ON(!ops->release))
-		return -EACCES;
+		return -EBADR;

 	spin_lock_irqsave(&req->lock, flags);

diff --git a/include/media/media-request.h b/include/media/media-request.h
index bd36d7431698..3cd25a2717ce 100644
--- a/include/media/media-request.h
+++ b/include/media/media-request.h
@@ -198,7 +198,7 @@ void media_request_put(struct media_request *req);
  * Get the request represented by @request_fd that is owned
  * by the media device.
  *
- * Return a -EACCES error pointer if requests are not supported
+ * Return a -EBADR error pointer if requests are not supported
  * by this driver. Return -EINVAL if the request was not found.
  * Return the pointer to the request if found: the caller will
  * have to call @media_request_put when it finished using the
@@ -231,7 +231,7 @@ static inline void media_request_put(struct media_request *req)
 static inline struct media_request *
 media_request_get_by_fd(struct media_device *mdev, int request_fd)
 {
-	return ERR_PTR(-EACCES);
+	return ERR_PTR(-EBADR);
 }

 #endif

Re: [PATCH v5.1 3/2] media requests: return EBADR instead of EACCES

[Mauro Carvalho Chehab]

Em Wed, 20 Mar 2019 15:23:07 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> If requests are used when they shouldn't, or not used when they should, then
> return EBADR (Invalid request descriptor) instead of EACCES.
> 
> The reason for this change is that EACCES has more to do with permissions
> (not being the owner of the resource), but in this case the request file
> descriptor is just wrong for the current mode of the device.
> 
> Update the documentation accordingly.

Looks OK to me.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> diff --git a/Documentation/media/uapi/mediactl/request-api.rst b/Documentation/media/uapi/mediactl/request-api.rst
> index 1ad631e549fe..a74c82d95609 100644
> --- a/Documentation/media/uapi/mediactl/request-api.rst
> +++ b/Documentation/media/uapi/mediactl/request-api.rst
> @@ -93,7 +93,7 @@ A queued request cannot be modified anymore.
>  .. caution::
>     For :ref:`memory-to-memory devices <mem2mem>` you can use requests only for
>     output buffers, not for capture buffers. Attempting to add a capture buffer
> -   to a request will result in an ``EACCES`` error.
> +   to a request will result in an ``EBADR`` error.
> 
>  If the request contains configurations for multiple entities, individual drivers
>  may synchronize so the requested pipeline's topology is applied before the
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index 81ffdcb89057..b9a2e9dc707c 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -326,7 +326,7 @@ struct v4l2_buffer
>  	Applications should not set ``V4L2_BUF_FLAG_REQUEST_FD`` for any ioctls
>  	other than :ref:`VIDIOC_QBUF <VIDIOC_QBUF>`.
> 
> -	If the device does not support requests, then ``EACCES`` will be returned.
> +	If the device does not support requests, then ``EBADR`` will be returned.
>  	If requests are supported but an invalid request file descriptor is
>  	given, then ``EINVAL`` will be returned.
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> index 5739c3676062..dbf7b445a27b 100644
> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> @@ -111,7 +111,7 @@ in use. Setting it means that the buffer will not be passed to the driver
>  until the request itself is queued. Also, the driver will apply any
>  settings associated with the request for this buffer. This field will
>  be ignored unless the ``V4L2_BUF_FLAG_REQUEST_FD`` flag is set.
> -If the device does not support requests, then ``EACCES`` will be returned.
> +If the device does not support requests, then ``EBADR`` will be returned.
>  If requests are supported but an invalid request file descriptor is given,
>  then ``EINVAL`` will be returned.
> 
> @@ -125,7 +125,7 @@ then ``EINVAL`` will be returned.
> 
>     For :ref:`memory-to-memory devices <mem2mem>` you can specify the
>     ``request_fd`` only for output buffers, not for capture buffers. Attempting
> -   to specify this for a capture buffer will result in an ``EACCES`` error.
> +   to specify this for a capture buffer will result in an ``EBADR`` error.
> 
>  Applications call the ``VIDIOC_DQBUF`` ioctl to dequeue a filled
>  (capturing) or displayed (output) buffer from the driver's outgoing
> @@ -185,12 +185,10 @@ EPIPE
>      codecs if a buffer with the ``V4L2_BUF_FLAG_LAST`` was already
>      dequeued and no new buffers are expected to become available.
> 
> -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
> +    The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not
> +    support requests for the given buffer type, or
> +    the ``V4L2_BUF_FLAG_REQUEST_FD`` flag was not set but the device requires
>      that the buffer is part of a request.
> 
>  EBUSY
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 84de18b30a95..b11a779e97b0 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -392,7 +392,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
>  		return 0;
>  	} else if (!q->supports_requests) {
>  		dprintk(1, "%s: queue does not support requests\n", opname);
> -		return -EACCES;
> +		return -EBADR;
>  	} else if (q->uses_qbuf) {
>  		dprintk(1, "%s: queue does not use requests\n", opname);
>  		return -EBUSY;
> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> index eec2e2b2f6ec..ed87305b29f9 100644
> --- a/drivers/media/media-request.c
> +++ b/drivers/media/media-request.c
> @@ -251,7 +251,7 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd)
> 
>  	if (!mdev || !mdev->ops ||
>  	    !mdev->ops->req_validate || !mdev->ops->req_queue)
> -		return ERR_PTR(-EACCES);
> +		return ERR_PTR(-EBADR);
> 
>  	filp = fget(request_fd);
>  	if (!filp)
> @@ -407,7 +407,7 @@ int media_request_object_bind(struct media_request *req,
>  	int ret = -EBUSY;
> 
>  	if (WARN_ON(!ops->release))
> -		return -EACCES;
> +		return -EBADR;
> 
>  	spin_lock_irqsave(&req->lock, flags);
> 
> diff --git a/include/media/media-request.h b/include/media/media-request.h
> index bd36d7431698..3cd25a2717ce 100644
> --- a/include/media/media-request.h
> +++ b/include/media/media-request.h
> @@ -198,7 +198,7 @@ void media_request_put(struct media_request *req);
>   * Get the request represented by @request_fd that is owned
>   * by the media device.
>   *
> - * Return a -EACCES error pointer if requests are not supported
> + * Return a -EBADR error pointer if requests are not supported
>   * by this driver. Return -EINVAL if the request was not found.
>   * Return the pointer to the request if found: the caller will
>   * have to call @media_request_put when it finished using the
> @@ -231,7 +231,7 @@ static inline void media_request_put(struct media_request *req)
>  static inline struct media_request *
>  media_request_get_by_fd(struct media_device *mdev, int request_fd)
>  {
> -	return ERR_PTR(-EACCES);
> +	return ERR_PTR(-EBADR);
>  }
> 
>  #endif



Thanks,
Mauro