[PATCH 1/2] usb: gadget: uvc: clean and rename uvcg_queue_next_buffer to uvcg_complete_buffer

[PATCH 1/2] usb: gadget: uvc: clean and rename uvcg_queue_next_buffer to uvcg_complete_buffer

[Michael Grzeschik]

The functions purpose is different to its name. We change the function
name and remove all unused code.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/uvc_queue.c | 18 +-----------------
 drivers/usb/gadget/function/uvc_queue.h |  2 +-
 drivers/usb/gadget/function/uvc_video.c |  6 +++---
 3 files changed, 5 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index d852ac9e47e72c..73ff56043d2e6a 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -326,24 +326,10 @@ int uvcg_queue_enable(struct uvc_video_queue *queue, int enable)
 }
 
 /* called with &queue_irqlock held.. */
-struct uvc_buffer *uvcg_queue_next_buffer(struct uvc_video_queue *queue,
+void uvcg_complete_buffer(struct uvc_video_queue *queue,
 					  struct uvc_buffer *buf)
 {
-	struct uvc_buffer *nextbuf;
-
-	if ((queue->flags & UVC_QUEUE_DROP_INCOMPLETE) &&
-	     buf->length != buf->bytesused) {
-		buf->state = UVC_BUF_STATE_QUEUED;
-		vb2_set_plane_payload(&buf->buf.vb2_buf, 0, 0);
-		return buf;
-	}
-
 	list_del(&buf->queue);
-	if (!list_empty(&queue->irqqueue))
-		nextbuf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
-					   queue);
-	else
-		nextbuf = NULL;
 
 	buf->buf.field = V4L2_FIELD_NONE;
 	buf->buf.sequence = queue->sequence++;
@@ -351,8 +337,6 @@ struct uvc_buffer *uvcg_queue_next_buffer(struct uvc_video_queue *queue,
 
 	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
 	vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
-
-	return nextbuf;
 }
 
 struct uvc_buffer *uvcg_queue_head(struct uvc_video_queue *queue)
diff --git a/drivers/usb/gadget/function/uvc_queue.h b/drivers/usb/gadget/function/uvc_queue.h
index 05360a0767f61f..b668927b5d2c4e 100644
--- a/drivers/usb/gadget/function/uvc_queue.h
+++ b/drivers/usb/gadget/function/uvc_queue.h
@@ -93,7 +93,7 @@ void uvcg_queue_cancel(struct uvc_video_queue *queue, int disconnect);
 
 int uvcg_queue_enable(struct uvc_video_queue *queue, int enable);
 
-struct uvc_buffer *uvcg_queue_next_buffer(struct uvc_video_queue *queue,
+void uvcg_complete_buffer(struct uvc_video_queue *queue,
 					  struct uvc_buffer *buf);
 
 struct uvc_buffer *uvcg_queue_head(struct uvc_video_queue *queue);
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 7f59a0c4740209..0c8d146b840321 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -112,7 +112,7 @@ uvc_video_encode_bulk(struct usb_request *req, struct uvc_video *video,
 	if (buf->bytesused == video->queue.buf_used) {
 		video->queue.buf_used = 0;
 		buf->state = UVC_BUF_STATE_DONE;
-		uvcg_queue_next_buffer(&video->queue, buf);
+		uvcg_complete_buffer(&video->queue, buf);
 		video->fid ^= UVC_STREAM_FID;
 
 		video->payload_size = 0;
@@ -183,7 +183,7 @@ uvc_video_encode_isoc_sg(struct usb_request *req, struct uvc_video *video,
 		video->queue.buf_used = 0;
 		buf->state = UVC_BUF_STATE_DONE;
 		buf->offset = 0;
-		uvcg_queue_next_buffer(&video->queue, buf);
+		uvcg_complete_buffer(&video->queue, buf);
 		video->fid ^= UVC_STREAM_FID;
 	}
 }
@@ -210,7 +210,7 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video,
 	if (buf->bytesused == video->queue.buf_used) {
 		video->queue.buf_used = 0;
 		buf->state = UVC_BUF_STATE_DONE;
-		uvcg_queue_next_buffer(&video->queue, buf);
+		uvcg_complete_buffer(&video->queue, buf);
 		video->fid ^= UVC_STREAM_FID;
 	}
 }
-- 
2.30.2

[PATCH 2/2] usb: gadget: uvc: giveback vb2 buffer on req complete

[Michael Grzeschik]

On uvc_video_encode_isoc_sg the mapped vb2 buffer is returned
to early. Only after the last usb_request referencing the buffer
it is allowed to give it back to vb2. This patch fixes that.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/uvc.h       | 1 +
 drivers/usb/gadget/function/uvc_queue.c | 2 --
 drivers/usb/gadget/function/uvc_video.c | 9 ++++++++-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index c3607a32b98624..9eec104b3666ad 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -79,6 +79,7 @@ struct uvc_request {
 	struct uvc_video *video;
 	struct sg_table sgt;
 	u8 header[UVCG_REQUEST_HEADER_LEN];
+	struct uvc_buffer *last_buf;
 };
 
 struct uvc_video {
diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index 73ff56043d2e6a..d9a0a2809b74c7 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -329,8 +329,6 @@ int uvcg_queue_enable(struct uvc_video_queue *queue, int enable)
 void uvcg_complete_buffer(struct uvc_video_queue *queue,
 					  struct uvc_buffer *buf)
 {
-	list_del(&buf->queue);
-
 	buf->buf.field = V4L2_FIELD_NONE;
 	buf->buf.sequence = queue->sequence++;
 	buf->buf.vb2_buf.timestamp = ktime_get_ns();
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 0c8d146b840321..7cbe6e81f171a4 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -183,8 +183,9 @@ uvc_video_encode_isoc_sg(struct usb_request *req, struct uvc_video *video,
 		video->queue.buf_used = 0;
 		buf->state = UVC_BUF_STATE_DONE;
 		buf->offset = 0;
-		uvcg_complete_buffer(&video->queue, buf);
+		list_del(&buf->queue);
 		video->fid ^= UVC_STREAM_FID;
+		ureq->last_buf = buf;
 	}
 }
 
@@ -264,6 +265,11 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 		uvcg_queue_cancel(queue, 0);
 	}
 
+	if (ureq->last_buf) {
+		uvcg_complete_buffer(&video->queue, ureq->last_buf);
+		ureq->last_buf = NULL;
+	}
+
 	spin_lock_irqsave(&video->req_lock, flags);
 	list_add_tail(&req->list, &video->req_free);
 	spin_unlock_irqrestore(&video->req_lock, flags);
@@ -332,6 +338,7 @@ uvc_video_alloc_requests(struct uvc_video *video)
 		video->ureq[i].req->complete = uvc_video_complete;
 		video->ureq[i].req->context = &video->ureq[i];
 		video->ureq[i].video = video;
+		video->ureq[i].last_buf = NULL;
 
 		list_add_tail(&video->ureq[i].req->list, &video->req_free);
 		/* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */
-- 
2.30.2

Re: [PATCH 1/2] usb: gadget: uvc: clean and rename uvcg_queue_next_buffer to uvcg_complete_buffer

[paul.elder]

Hi Michael,

Thanks for the patch.

On Mon, Feb 28, 2022 at 03:16:58PM +0100, Michael Grzeschik wrote:
> The functions purpose is different to its name. We change the function
> name and remove all unused code.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/gadget/function/uvc_queue.c | 18 +-----------------
>  drivers/usb/gadget/function/uvc_queue.h |  2 +-
>  drivers/usb/gadget/function/uvc_video.c |  6 +++---
>  3 files changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> index d852ac9e47e72c..73ff56043d2e6a 100644
> --- a/drivers/usb/gadget/function/uvc_queue.c
> +++ b/drivers/usb/gadget/function/uvc_queue.c
> @@ -326,24 +326,10 @@ int uvcg_queue_enable(struct uvc_video_queue *queue, int enable)
>  }
>  
>  /* called with &queue_irqlock held.. */
> -struct uvc_buffer *uvcg_queue_next_buffer(struct uvc_video_queue *queue,
> +void uvcg_complete_buffer(struct uvc_video_queue *queue,
>  					  struct uvc_buffer *buf)
>  {
> -	struct uvc_buffer *nextbuf;
> -
> -	if ((queue->flags & UVC_QUEUE_DROP_INCOMPLETE) &&
> -	     buf->length != buf->bytesused) {
> -		buf->state = UVC_BUF_STATE_QUEUED;
> -		vb2_set_plane_payload(&buf->buf.vb2_buf, 0, 0);
> -		return buf;
> -	}
> -
>  	list_del(&buf->queue);
> -	if (!list_empty(&queue->irqqueue))
> -		nextbuf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
> -					   queue);
> -	else
> -		nextbuf = NULL;

Is it fine to drop these? They look important to me. If they're not,
then the reason should be explained in the commit message.

>  
>  	buf->buf.field = V4L2_FIELD_NONE;
>  	buf->buf.sequence = queue->sequence++;
> @@ -351,8 +337,6 @@ struct uvc_buffer *uvcg_queue_next_buffer(struct uvc_video_queue *queue,
>  
>  	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
>  	vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
> -
> -	return nextbuf;

This looks fine since all callers ignore it anyway.


Paul

>  }
>  
>  struct uvc_buffer *uvcg_queue_head(struct uvc_video_queue *queue)
> diff --git a/drivers/usb/gadget/function/uvc_queue.h b/drivers/usb/gadget/function/uvc_queue.h
> index 05360a0767f61f..b668927b5d2c4e 100644
> --- a/drivers/usb/gadget/function/uvc_queue.h
> +++ b/drivers/usb/gadget/function/uvc_queue.h
> @@ -93,7 +93,7 @@ void uvcg_queue_cancel(struct uvc_video_queue *queue, int disconnect);
>  
>  int uvcg_queue_enable(struct uvc_video_queue *queue, int enable);
>  
> -struct uvc_buffer *uvcg_queue_next_buffer(struct uvc_video_queue *queue,
> +void uvcg_complete_buffer(struct uvc_video_queue *queue,
>  					  struct uvc_buffer *buf);
>  
>  struct uvc_buffer *uvcg_queue_head(struct uvc_video_queue *queue);
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index 7f59a0c4740209..0c8d146b840321 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -112,7 +112,7 @@ uvc_video_encode_bulk(struct usb_request *req, struct uvc_video *video,
>  	if (buf->bytesused == video->queue.buf_used) {
>  		video->queue.buf_used = 0;
>  		buf->state = UVC_BUF_STATE_DONE;
> -		uvcg_queue_next_buffer(&video->queue, buf);
> +		uvcg_complete_buffer(&video->queue, buf);
>  		video->fid ^= UVC_STREAM_FID;
>  
>  		video->payload_size = 0;
> @@ -183,7 +183,7 @@ uvc_video_encode_isoc_sg(struct usb_request *req, struct uvc_video *video,
>  		video->queue.buf_used = 0;
>  		buf->state = UVC_BUF_STATE_DONE;
>  		buf->offset = 0;
> -		uvcg_queue_next_buffer(&video->queue, buf);
> +		uvcg_complete_buffer(&video->queue, buf);
>  		video->fid ^= UVC_STREAM_FID;
>  	}
>  }
> @@ -210,7 +210,7 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video,
>  	if (buf->bytesused == video->queue.buf_used) {
>  		video->queue.buf_used = 0;
>  		buf->state = UVC_BUF_STATE_DONE;
> -		uvcg_queue_next_buffer(&video->queue, buf);
> +		uvcg_complete_buffer(&video->queue, buf);
>  		video->fid ^= UVC_STREAM_FID;
>  	}
>  }
> -- 
> 2.30.2
> 

Re: [PATCH 1/2] usb: gadget: uvc: clean and rename uvcg_queue_next_buffer to uvcg_complete_buffer

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 3031 bytes --]

On Fri, Mar 11, 2022 at 04:04:44PM +0900, paul.elder@ideasonboard.com wrote:
>Thanks for the patch.
>
>On Mon, Feb 28, 2022 at 03:16:58PM +0100, Michael Grzeschik wrote:
>> The functions purpose is different to its name. We change the function
>> name and remove all unused code.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>>  drivers/usb/gadget/function/uvc_queue.c | 18 +-----------------
>>  drivers/usb/gadget/function/uvc_queue.h |  2 +-
>>  drivers/usb/gadget/function/uvc_video.c |  6 +++---
>>  3 files changed, 5 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
>> index d852ac9e47e72c..73ff56043d2e6a 100644
>> --- a/drivers/usb/gadget/function/uvc_queue.c
>> +++ b/drivers/usb/gadget/function/uvc_queue.c
>> @@ -326,24 +326,10 @@ int uvcg_queue_enable(struct uvc_video_queue *queue, int enable)
>>  }
>>
>>  /* called with &queue_irqlock held.. */
>> -struct uvc_buffer *uvcg_queue_next_buffer(struct uvc_video_queue *queue,
>> +void uvcg_complete_buffer(struct uvc_video_queue *queue,
>>  					  struct uvc_buffer *buf)
>>  {
>> -	struct uvc_buffer *nextbuf;
>> -
>> -	if ((queue->flags & UVC_QUEUE_DROP_INCOMPLETE) &&
>> -	     buf->length != buf->bytesused) {
>> -		buf->state = UVC_BUF_STATE_QUEUED;
>> -		vb2_set_plane_payload(&buf->buf.vb2_buf, 0, 0);
>> -		return buf;
>> -	}
>> -
>>  	list_del(&buf->queue);
>> -	if (!list_empty(&queue->irqqueue))
>> -		nextbuf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
>> -					   queue);
>> -	else
>> -		nextbuf = NULL;
>
>Is it fine to drop these? They look important to me. If they're not,
>then the reason should be explained in the commit message.

As UVC_QUEUE_DROP_INCOMPLETE is not used anywhere in the whole gadget,
removing it should be save. However this should probably be handled in
an extra patch before.

The pulling of nextbuf from the irequeue is done only to handover the
next_buffer like the functions purpose actually was intended for. Since
the function does not do this anymore, we can savely remove the code.

I will improve the description in v2.

>>
>>  	buf->buf.field = V4L2_FIELD_NONE;
>>  	buf->buf.sequence = queue->sequence++;
>> @@ -351,8 +337,6 @@ struct uvc_buffer *uvcg_queue_next_buffer(struct uvc_video_queue *queue,
>>
>>  	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
>>  	vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
>> -
>> -	return nextbuf;
>
>This looks fine since all callers ignore it anyway.

Right, and beacause of that the above list_first_entry assignment can be
dropped.

Thanks,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]