Re: [RFT PATCH v3 5/6] uvcvideo: queue: Support asynchronous buffer handling

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday, 12 January 2018 11:19:26 EET Kieran Bingham wrote:
> The buffer queue interface currently operates sequentially, processing
> buffers after they have fully completed.
> 
> In preparation for supporting parallel tasks operating on the buffers,
> we will need to support buffers being processed on multiple CPUs.
> 
> Adapt the uvc_queue_next_buffer() such that a reference count tracks the
> active use of the buffer, returning the buffer to the VB2 stack at
> completion.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> ---
>  drivers/media/usb/uvc/uvc_queue.c | 61 ++++++++++++++++++++++++++------
>  drivers/media/usb/uvc/uvcvideo.h  |  4 ++-
>  2 files changed, 54 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_queue.c
> b/drivers/media/usb/uvc/uvc_queue.c index ddac4d89a291..5a9987e547d3 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -131,6 +131,7 @@ static void uvc_buffer_queue(struct vb2_buffer *vb)
> 
>  	spin_lock_irqsave(&queue->irqlock, flags);
>  	if (likely(!(queue->flags & UVC_QUEUE_DISCONNECTED))) {
> +		kref_init(&buf->ref);
>  		list_add_tail(&buf->queue, &queue->irqqueue);
>  	} else {
>  		/* If the device is disconnected return the buffer to userspace
> @@ -424,28 +425,66 @@ struct uvc_buffer *uvc_queue_get_current_buffer(struct
> uvc_video_queue *queue) return nextbuf;
>  }
> 
> -struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
> +/*
> + * uvc_queue_requeue: Requeue a buffer on our internal irqqueue
> + *
> + * Reuse a buffer through our internal queue without the need to 'prepare'
> + * The buffer will be returned to userspace through the uvc_buffer_queue
> call if
> + * the device has been disconnected
> + */
> +static void uvc_queue_requeue(struct uvc_video_queue *queue,
>  		struct uvc_buffer *buf)
>  {
> -	struct uvc_buffer *nextbuf;
> -	unsigned long flags;
> +	buf->error = 0;
> +	buf->state = UVC_BUF_STATE_QUEUED;
> +	buf->bytesused = 0;
> +	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, 0);
> +
> +	uvc_buffer_queue(&buf->buf.vb2_buf);
> +}

You could have inlined this in uvc_queue_buffer_complete(), but the above 
documentation is useful, so I'm fine if you prefer keeping it as a separate 
function. Maybe you could call it uvc_queue_buffer_requeue() to be consistent 
with the other functions ?

> +static void uvc_queue_buffer_complete(struct kref *ref)
> +{
> +	struct uvc_buffer *buf = container_of(ref, struct uvc_buffer, ref);
> +	struct vb2_buffer *vb = &buf->buf.vb2_buf;
> +	struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
> 
>  	if ((queue->flags & UVC_QUEUE_DROP_CORRUPTED) && buf->error) {
> -		buf->error = 0;
> -		buf->state = UVC_BUF_STATE_QUEUED;
> -		buf->bytesused = 0;
> -		vb2_set_plane_payload(&buf->buf.vb2_buf, 0, 0);
> -		return buf;
> +		uvc_queue_requeue(queue, buf);
> +		return;

This changes the behaviour of the driver. Currently when an erroneous buffer 
is encountered, it will be immediately reused. With this patch applied it will 
be pushed to the back of the queue for later reuse. This will result in 
buffers being reordered, possibly causing issues later when we'll implement 
fences support (or possibly even today).

I think the whole drop corrupted buffers mechanism was a bad idea in the first 
place and I'd like to remove it at some point, buffers in the error state 
should be handled by applications. However, until that's done, I wonder 
whether it would be possible to keep the current order. I unfortunately don't 
see an easy option to do so at the moment, but maybe you would. Otherwise I 
suppose we'll have to leave it as is.

I'm tempted to flip the driver to not drop corrupted buffers by default. I've 
done so on my computer, I'll see if I run into any issue. It could be useful 
if you could set the nodrop parameter to 1 on your systems too when performing 
tests.

>  	}
> 
> +	buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
> +	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
> +	vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
> +}
> +
> +/*
> + * Release a reference on the buffer. Complete the buffer when the last
> + * reference is released
> + */
> +void uvc_queue_buffer_release(struct uvc_buffer *buf)
> +{
> +	kref_put(&buf->ref, uvc_queue_buffer_complete);
> +}
> +
> +/*
> + * Remove this buffer from the queue. Lifetime will persist while async
> actions
> + * are still running (if any), and uvc_queue_buffer_release will give the
> buffer
> + * back to VB2 when all users have completed.
> + */
> +struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
> +		struct uvc_buffer *buf)
> +{
> +	struct uvc_buffer *nextbuf;
> +	unsigned long flags;
> +
>  	spin_lock_irqsave(&queue->irqlock, flags);
>  	list_del(&buf->queue);
>  	nextbuf = __uvc_queue_get_current_buffer(queue);
>  	spin_unlock_irqrestore(&queue->irqlock, flags);
> 
> -	buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
> -	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
> -	vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
> +	uvc_queue_buffer_release(buf);
> 
>  	return nextbuf;
>  }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 5caa1f4de3cb..6a18dbfc3e5b 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -404,6 +404,9 @@ struct uvc_buffer {
>  	unsigned int bytesused;
> 
>  	u32 pts;
> +
> +	/* asynchronous buffer handling */

Please capitalize the first word to match other comments in the driver.

> +	struct kref ref;
>  };
> 
>  #define UVC_QUEUE_DISCONNECTED		(1 << 0)
> @@ -696,6 +699,7 @@ extern struct uvc_buffer *
>  		uvc_queue_get_current_buffer(struct uvc_video_queue *queue);
>  extern struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue
> *queue, struct uvc_buffer *buf);
> +extern void uvc_queue_buffer_release(struct uvc_buffer *buf);

No need for the extern keyboard. I'll submit a patch to drop it for all 
functions.

>  extern int uvc_queue_mmap(struct uvc_video_queue *queue,
>  		struct vm_area_struct *vma);
>  extern unsigned int uvc_queue_poll(struct uvc_video_queue *queue,

-- 
Regards,

Laurent Pinchart