drivers/usb/gadget/function/uvc_video.c | 38 ++++++++++++++++--------- 1 file changed, 25 insertions(+), 13 deletions(-)
This patch elaborates on some of the edge cases handled by
video_pump around setting no_interrupt flag, and brings the
code style in line with rest of the file.
Link: https://lore.kernel.org/20230602151916.GH26944@pendragon.ideasonboard.com/
Signed-off-by: Avichal Rakesh <arakesh@google.com>
---
Changelog:
v2:
- Updated commit message to make it clear that userspace application is not
required to match the ISOC rate.
- Styling and comment revision based on review
v3:
- Rebased on to Greg's usb-next where v1 had already merged
- Updated commit message to match the actual changes after rebase.
drivers/usb/gadget/function/uvc_video.c | 38 ++++++++++++++++---------
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index e81865978299..91af3b1ef0d4 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -382,13 +382,13 @@ static void uvcg_video_pump(struct work_struct *work)
{
struct uvc_video *video = container_of(work, struct uvc_video, pump);
struct uvc_video_queue *queue = &video->queue;
+ /* video->max_payload_size is only set when using bulk transfer */
+ bool is_bulk = video->max_payload_size;
struct usb_request *req = NULL;
struct uvc_buffer *buf;
unsigned long flags;
+ bool buf_done;
int ret;
- bool buf_int;
- /* video->max_payload_size is only set when using bulk transfer */
- bool is_bulk = video->max_payload_size;
while (video->ep->enabled) {
/*
@@ -414,20 +414,19 @@ static void uvcg_video_pump(struct work_struct *work)
if (buf != NULL) {
video->encode(req, video, buf);
- /* Always interrupt for the last request of a video buffer */
- buf_int = buf->state == UVC_BUF_STATE_DONE;
+ buf_done = buf->state == UVC_BUF_STATE_DONE;
} else if (!(queue->flags & UVC_QUEUE_DISCONNECTED) && !is_bulk) {
/*
* No video buffer available; the queue is still connected and
- * we're traferring over ISOC. Queue a 0 length request to
+ * we're transferring over ISOC. Queue a 0 length request to
* prevent missed ISOC transfers.
*/
req->length = 0;
- buf_int = false;
+ buf_done = false;
} else {
/*
- * Either queue has been disconnected or no video buffer
- * available to bulk transfer. Either way, stop processing
+ * Either the queue has been disconnected or no video buffer
+ * available for bulk transfer. Either way, stop processing
* further.
*/
spin_unlock_irqrestore(&queue->irqlock, flags);
@@ -435,11 +434,24 @@ static void uvcg_video_pump(struct work_struct *work)
}
/*
- * With usb3 we have more requests. This will decrease the
- * interrupt load to a quarter but also catches the corner
- * cases, which needs to be handled.
+ * With USB3 handling more requests at a higher speed, we can't
+ * afford to generate an interrupt for every request. Decide to
+ * interrupt:
+ *
+ * - When no more requests are available in the free queue, as
+ * this may be our last chance to refill the endpoint's
+ * request queue.
+ *
+ * - When this is request is the last request for the video
+ * buffer, as we want to start sending the next video buffer
+ * ASAP in case it doesn't get started already in the next
+ * iteration of this loop.
+ *
+ * - Four times over the length of the requests queue (as
+ * indicated by video->uvc_num_requests), as a trade-off
+ * between latency and interrupt load.
*/
- if (list_empty(&video->req_free) || buf_int ||
+ if (list_empty(&video->req_free) || buf_done ||
!(video->req_int_count %
DIV_ROUND_UP(video->uvc_num_requests, 4))) {
video->req_int_count = 0;
--
2.41.0.rc0.172.g3f132b7071-googHi Avichal,
Thank you for the patch.
On Fri, Jun 02, 2023 at 03:04:55PM -0700, Avichal Rakesh wrote:
> This patch elaborates on some of the edge cases handled by
> video_pump around setting no_interrupt flag, and brings the
> code style in line with rest of the file.
>
> Link: https://lore.kernel.org/20230602151916.GH26944@pendragon.ideasonboard.com/
> Signed-off-by: Avichal Rakesh <arakesh@google.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changelog:
> v2:
> - Updated commit message to make it clear that userspace application is not
> required to match the ISOC rate.
> - Styling and comment revision based on review
> v3:
> - Rebased on to Greg's usb-next where v1 had already merged
> - Updated commit message to match the actual changes after rebase.
>
>
> drivers/usb/gadget/function/uvc_video.c | 38 ++++++++++++++++---------
> 1 file changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index e81865978299..91af3b1ef0d4 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -382,13 +382,13 @@ static void uvcg_video_pump(struct work_struct *work)
> {
> struct uvc_video *video = container_of(work, struct uvc_video, pump);
> struct uvc_video_queue *queue = &video->queue;
> + /* video->max_payload_size is only set when using bulk transfer */
> + bool is_bulk = video->max_payload_size;
> struct usb_request *req = NULL;
> struct uvc_buffer *buf;
> unsigned long flags;
> + bool buf_done;
> int ret;
> - bool buf_int;
> - /* video->max_payload_size is only set when using bulk transfer */
> - bool is_bulk = video->max_payload_size;
>
> while (video->ep->enabled) {
> /*
> @@ -414,20 +414,19 @@ static void uvcg_video_pump(struct work_struct *work)
>
> if (buf != NULL) {
> video->encode(req, video, buf);
> - /* Always interrupt for the last request of a video buffer */
> - buf_int = buf->state == UVC_BUF_STATE_DONE;
> + buf_done = buf->state == UVC_BUF_STATE_DONE;
> } else if (!(queue->flags & UVC_QUEUE_DISCONNECTED) && !is_bulk) {
> /*
> * No video buffer available; the queue is still connected and
> - * we're traferring over ISOC. Queue a 0 length request to
> + * we're transferring over ISOC. Queue a 0 length request to
> * prevent missed ISOC transfers.
> */
> req->length = 0;
> - buf_int = false;
> + buf_done = false;
> } else {
> /*
> - * Either queue has been disconnected or no video buffer
> - * available to bulk transfer. Either way, stop processing
> + * Either the queue has been disconnected or no video buffer
> + * available for bulk transfer. Either way, stop processing
> * further.
> */
> spin_unlock_irqrestore(&queue->irqlock, flags);
> @@ -435,11 +434,24 @@ static void uvcg_video_pump(struct work_struct *work)
> }
>
> /*
> - * With usb3 we have more requests. This will decrease the
> - * interrupt load to a quarter but also catches the corner
> - * cases, which needs to be handled.
> + * With USB3 handling more requests at a higher speed, we can't
> + * afford to generate an interrupt for every request. Decide to
> + * interrupt:
> + *
> + * - When no more requests are available in the free queue, as
> + * this may be our last chance to refill the endpoint's
> + * request queue.
> + *
> + * - When this is request is the last request for the video
> + * buffer, as we want to start sending the next video buffer
> + * ASAP in case it doesn't get started already in the next
> + * iteration of this loop.
> + *
> + * - Four times over the length of the requests queue (as
> + * indicated by video->uvc_num_requests), as a trade-off
> + * between latency and interrupt load.
> */
> - if (list_empty(&video->req_free) || buf_int ||
> + if (list_empty(&video->req_free) || buf_done ||
> !(video->req_int_count %
> DIV_ROUND_UP(video->uvc_num_requests, 4))) {
> video->req_int_count = 0;
--
Regards,
Laurent Pinchart
On Fri, Jun 02, 2023 at 03:04:55PM -0700, Avichal Rakesh wrote: > This patch elaborates on some of the edge cases handled by > video_pump around setting no_interrupt flag, and brings the > code style in line with rest of the file. When you say "and" that usually means it should be a separate patch. But I really don't see what coding style changes you made here, what was it? I can't see any logical changes made here, am I missing them? Or is this all just a style-cleanup patch? thanks, greg k-h
Hi Greg, On Sun, Jun 04, 2023 at 09:43:40AM +0200, Greg KH wrote: > On Fri, Jun 02, 2023 at 03:04:55PM -0700, Avichal Rakesh wrote: > > This patch elaborates on some of the edge cases handled by > > video_pump around setting no_interrupt flag, and brings the > > code style in line with rest of the file. > > When you say "and" that usually means it should be a separate patch. > > But I really don't see what coding style changes you made here, what was > it? > > I can't see any logical changes made here, am I missing them? Or is > this all just a style-cleanup patch? It's all style cleanup (variable declaration ordering), typo fixes, and naming and documentation improvement, yes. I reviewed Avichal's original patch when coming back from holidays, neither of us realizing that you had merged it already. He sent a v2 and got told to rebase it on top of your tree. That's what v3 is, just handling the review comments. I generally ask for patches to be split with one change per patch, but given the small changes bundled here, and the fact that all of this just incorporates the review comments, I think it would be a bit overkill. -- Regards, Laurent Pinchart
On Sun, Jun 4, 2023 at 12:55 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Greg, > > On Sun, Jun 04, 2023 at 09:43:40AM +0200, Greg KH wrote: > > On Fri, Jun 02, 2023 at 03:04:55PM -0700, Avichal Rakesh wrote: > > > This patch elaborates on some of the edge cases handled by > > > video_pump around setting no_interrupt flag, and brings the > > > code style in line with rest of the file. > > > > When you say "and" that usually means it should be a separate patch. > > > > But I really don't see what coding style changes you made here, what was > > it? > > > > I can't see any logical changes made here, am I missing them? Or is > > this all just a style-cleanup patch? > > It's all style cleanup (variable declaration ordering), typo fixes, and > naming and documentation improvement, yes. I reviewed Avichal's original > patch when coming back from holidays, neither of us realizing that you > had merged it already. He sent a v2 and got told to rebase it on top of > your tree. That's what v3 is, just handling the review comments. > > I generally ask for patches to be split with one change per patch, but > given the small changes bundled here, and the fact that all of this just > incorporates the review comments, I think it would be a bit overkill. > Apologies for the goof up. I hadn't realized that Linus' master wasn't the very tip of the Linux tree, and assumed the v1 merge had fallen behind. I am happy to split it up into smaller patches if you want, but as Laurent already mentioned: this is just documentation and styling cleanup from V1 with no logic fixes. - Avi.
© 2016 - 2024 Red Hat, Inc.