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

[Mauro Carvalho Chehab <mchehab+samsung@kernel.org>]

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