Hi Jayant, thanks for the update. I just have a couple of styling comments.
On 09/11/2023 07:34, Jayant Chowdhary wrote:
> When we use an async work queue to perform the function of pumping
> usb requests to the usb controller, it is possible that amongst other
> factors, thread scheduling affects at what cadence we're able to pump
> requests. This could mean isoc usb requests miss their uframes - resulting
> in video stream flickers on the host device.
>
> To avoid this, we make the async_wq thread only produce isoc usb_requests
> with uvc buffers encoded into them. The process of queueing to the
> endpoint is done by the uvc_video_complete() handler. In case no
> usb_requests are ready with encoded information, we just queue a zero
> length request to the endpoint from the complete handler.
>
> For bulk endpoints the async_wq thread still queues usb requests to the
> endpoint.
>
> Signed-off-by: Michael Grzeschik <[email protected]>
> Signed-off-by: Jayant Chowdhary <[email protected]>
> Suggested-by: Avichal Rakesh <[email protected]>
> Suggested-by: Alan Stern <[email protected]>
> ---
> Based on top of
> https://lore.kernel.org/linux-usb/[email protected]/T/#t:
> v1->v2: Added self Signed-Off-by and addressed review comments
> v2->v3: Encode to usb requests in async_wq; queue to ep in complete handler
> for isoc transfers.
> v3->v4: Address review comments around code style.
> v4->v5: Update comments. Remove 0 length request queueing from async_wq
> thread since it is already done by the complete handler.
> v5->v6: Fix checkpatch.pl suggestions.
>
> drivers/usb/gadget/function/uvc.h | 8 +
> drivers/usb/gadget/function/uvc_video.c | 204 ++++++++++++++++++------
> 2 files changed, 166 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index e8d4c87f1e09..5ff454528bd8 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -105,7 +105,15 @@ struct uvc_video {
> bool is_enabled; /* tracks whether video stream is enabled */
> unsigned int req_size;
> struct list_head ureqs; /* all uvc_requests allocated by uvc_video */
> +
> + /* USB requests that the video pump thread can encode into */
> struct list_head req_free;
> +
> + /*
> + * USB requests video pump thread has already encoded into. These are
> + * ready to be queued to the endpoint.
> + */
> + struct list_head req_ready;
> spinlock_t req_lock;
>
> unsigned int req_int_count;
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index 53feb790a4c3..d5311456fa8a 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -268,6 +268,100 @@ static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req)
> return ret;
> }
>
> +/* This function must be called with video->req_lock held. */
> +static int uvcg_video_usb_req_queue(struct uvc_video *video,
> + struct usb_request *req, bool queue_to_ep)
> +{
> + bool is_bulk = video->max_payload_size;
> + struct list_head *list = NULL;
> +
> + if (!video->is_enabled) {
> + uvc_video_free_request(req->context, video->ep);
> + return -ENODEV;
> + }
> + if (queue_to_ep) {
> + struct uvc_request *ureq = req->context;
> + /*
> + * 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) || ureq->last_buf ||
> + !(video->req_int_count %
> + DIV_ROUND_UP(video->uvc_num_requests, 4))) {
> + video->req_int_count = 0;
> + req->no_interrupt = 0;
> + } else {
> + req->no_interrupt = 1;
> + }
> + video->req_int_count++;
> + return uvcg_video_ep_queue(video, req);
> + }
> + /*
> + * If we're not queuing to the ep, for isoc we're queuing
> + * to the req_ready list, otherwise req_free.
> + */
> + list = is_bulk ? &video->req_free : &video->req_ready;
> + list_add_tail(&req->list, list);
> + return 0;
> +}
> +
> +/*
> + * Must only be called from uvcg_video_enable - since after that we only want to
> + * queue requests to the endpoint from the uvc_video_complete complete handler.
> + * This function is needed in order to 'kick start' the flow of requests from
> + * gadget driver to the usb controller.
> + */
> +static void uvc_video_ep_queue_initial_requests(struct uvc_video *video)
> +{
> + struct usb_request *req = NULL;
> + unsigned long flags = 0;
> + unsigned int count = 0;
> + int ret = 0;
Add an empty line here please
> + /*
> + * We only queue half of the free list since we still want to have
> + * some free usb_requests in the free list for the video_pump async_wq
> + * thread to encode uvc buffers into. Otherwise we could get into a
> + * situation where the free list does not have any usb requests to
> + * encode into - we always end up queueing 0 length requests to the
> + * end point.
> + */
> + unsigned int half_list_size = video->uvc_num_requests / 2;
> +
> + spin_lock_irqsave(&video->req_lock, flags);
> + /*
> + * Take these requests off the free list and queue them all to the
> + * endpoint. Since we queue 0 length requests with the req_lock held,
> + * there isn't any 'data' race involved here with the complete handler.
> + */
> + while (count < half_list_size) {
> + req = list_first_entry(&video->req_free, struct usb_request,
> + list);
> + list_del(&req->list);
> + req->length = 0;
> + ret = uvcg_video_ep_queue(video, req);
> + if (ret < 0) {
> + uvcg_queue_cancel(&video->queue, /*disconnect*/0);
Drop the /*disconnect*/ comment please
> + break;
> + }
> + count++;
> + }
> + spin_unlock_irqrestore(&video->req_lock, flags);
> +}
> +
> static void
> uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
> {
> @@ -276,6 +370,8 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
> struct uvc_video_queue *queue = &video->queue;
> struct uvc_buffer *last_buf = NULL;
> unsigned long flags;
> + bool is_bulk = video->max_payload_size;
> + int ret = 0;
>
> spin_lock_irqsave(&video->req_lock, flags);
> if (!video->is_enabled) {
> @@ -329,8 +425,46 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
> * back to req_free
> */
> if (video->is_enabled) {
> - list_add_tail(&req->list, &video->req_free);
> - queue_work(video->async_wq, &video->pump);
> + /*
> + * Here we check whether any request is available in the ready
> + * list. If it is, queue it to the ep and add the current
> + * usb_request to the req_free list - for video_pump to fill in.
> + * Otherwise, just use the current usb_request to queue a 0
> + * length request to the ep. Since we always add to the req_free
> + * list if we dequeue from the ready list, there will never
> + * be a situation where the req_free list is completely out of
> + * requests and cannot recover.
> + */
> + struct usb_request *to_queue = req;
> +
> + to_queue->length = 0;
> + if (!list_empty(&video->req_ready)) {
> + to_queue = list_first_entry(&video->req_ready,
> + struct usb_request, list);
> + list_del(&to_queue->list);
> + /* Add it to the free list. */
I would drop the "Add it to the free list" comment; the code is clear already.
> + list_add_tail(&req->list, &video->req_free);
> + /*
> + * Queue work to the wq as well since it is possible that a
> + * buffer may not have been completely encoded with the set of
> + * in-flight usb requests for whih the complete callbacks are
> + * firing.
> + * In that case, if we do not queue work to the worker thread,
> + * the buffer will never be marked as complete - and therefore
> + * not be returned to userpsace. As a result,
> + * dequeue -> queue -> dequeue flow of uvc buffers will not
> + * happen.
> + */
> + queue_work(video->async_wq, &video->pump);
> + }
> + /*
> + * Queue to the endpoint. The actual queueing to ep will
> + * only happen on one thread - the async_wq for bulk endpoints
> + * and this thread for isoc endpoints.
> + */
> + ret = uvcg_video_usb_req_queue(video, to_queue, !is_bulk);
> + if (ret < 0)
> + uvcg_queue_cancel(queue, 0);
> } else {
> uvc_video_free_request(ureq, ep);
> }
> @@ -347,6 +481,7 @@ uvc_video_free_requests(struct uvc_video *video)
>
> INIT_LIST_HEAD(&video->ureqs);
> INIT_LIST_HEAD(&video->req_free);
> + INIT_LIST_HEAD(&video->req_ready);
> video->req_size = 0;
> return 0;
> }
> @@ -424,8 +559,7 @@ static void uvcg_video_pump(struct work_struct *work)
> struct usb_request *req = NULL;
> struct uvc_buffer *buf;
> unsigned long flags;
> - bool buf_done;
> - int ret;
> + int ret = 0;
>
> while (true) {
> if (!video->ep->enabled)
> @@ -454,15 +588,6 @@ static void uvcg_video_pump(struct work_struct *work)
>
> if (buf != NULL) {
> video->encode(req, video, buf);
> - 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 transferring over ISOC. Queue a 0 length request to
> - * prevent missed ISOC transfers.
> - */
> - req->length = 0;
> - buf_done = false;
> } else {
> /*
> * Either the queue has been disconnected or no video buffer
> @@ -473,45 +598,25 @@ static void uvcg_video_pump(struct work_struct *work)
> break;
> }
>
> - /*
> - * 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_done ||
> - !(video->req_int_count %
> - DIV_ROUND_UP(video->uvc_num_requests, 4))) {
> - video->req_int_count = 0;
> - req->no_interrupt = 0;
> - } else {
> - req->no_interrupt = 1;
> - }
> -
> - /* Queue the USB request */
> - ret = uvcg_video_ep_queue(video, req);
> spin_unlock_irqrestore(&queue->irqlock, flags);
>
> + spin_lock_irqsave(&video->req_lock, flags);
> + /* For bulk end points we queue from the worker thread
> + * since we would preferably not want to wait on requests
> + * to be ready, in the uvcg_video_complete() handler.
> + * For isoc endpoints we add the request to the ready list
> + * and only queue it to the endpoint from the complete handler.
> + */
> + ret = uvcg_video_usb_req_queue(video, req, is_bulk);
> + spin_unlock_irqrestore(&video->req_lock, flags);
> +
> if (ret < 0) {
> uvcg_queue_cancel(queue, 0);
> break;
> }
>
> - /* Endpoint now owns the request */
> + /* The request is owned by the endpoint / ready list. */
> req = NULL;
> - video->req_int_count++;
> }
>
> if (!req)
> @@ -567,7 +672,7 @@ uvcg_video_disable(struct uvc_video *video)
>
> spin_lock_irqsave(&video->req_lock, flags);
> /*
> - * Remove all uvc_reqeusts from ureqs with list_del_init
> + * Remove all uvc_requests from ureqs with list_del_init
Did the alignment of the * get messed up here as well as the typo fix or is it just my mail client
being weird?
> * This lets uvc_video_free_request correctly identify
> * if the uvc_request is attached to a list or not when freeing
> * memory.
> @@ -580,8 +685,14 @@ uvcg_video_disable(struct uvc_video *video)
> uvc_video_free_request(req->context, video->ep);
> }
>
> + list_for_each_entry_safe(req, temp, &video->req_ready, list) {
> + list_del(&req->list);
> + uvc_video_free_request(req->context, video->ep);
> + }
> +
> INIT_LIST_HEAD(&video->ureqs);
> INIT_LIST_HEAD(&video->req_free);
> + INIT_LIST_HEAD(&video->req_ready);
> video->req_size = 0;
> spin_unlock_irqrestore(&video->req_lock, flags);
>
> @@ -635,7 +746,7 @@ int uvcg_video_enable(struct uvc_video *video)
>
> video->req_int_count = 0;
>
> - queue_work(video->async_wq, &video->pump);
> + uvc_video_ep_queue_initial_requests(video);
>
> return ret;
> }
> @@ -648,6 +759,7 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
> video->is_enabled = false;
> INIT_LIST_HEAD(&video->ureqs);
> INIT_LIST_HEAD(&video->req_free);
> + INIT_LIST_HEAD(&video->req_ready);
> spin_lock_init(&video->req_lock);
> INIT_WORK(&video->pump, uvcg_video_pump);
>
Hi Dan,
Thanks for the comments. I've uploaded a new patch at https://lore.kernel.org/linux-usb/[email protected]/T/#u.
On 11/16/23 02:09, Dan Scally wrote:
> Hi Jayant, thanks for the update. I just have a couple of styling comments.
>
> On 09/11/2023 07:34, Jayant Chowdhary wrote:
>> When we use an async work queue to perform the function of pumping
>> usb requests to the usb controller, it is possible that amongst other
>> factors, thread scheduling affects at what cadence we're able to pump
>> requests. This could mean isoc usb requests miss their uframes - resulting
>> in video stream flickers on the host device.
>>
>> To avoid this, we make the async_wq thread only produce isoc usb_requests
>> with uvc buffers encoded into them. The process of queueing to the
>> endpoint is done by the uvc_video_complete() handler. In case no
>> usb_requests are ready with encoded information, we just queue a zero
>> length request to the endpoint from the complete handler.
>>
>> For bulk endpoints the async_wq thread still queues usb requests to the
>> endpoint.
>>
>> Signed-off-by: Michael Grzeschik <[email protected]>
>> Signed-off-by: Jayant Chowdhary <[email protected]>
>> Suggested-by: Avichal Rakesh <[email protected]>
>> Suggested-by: Alan Stern <[email protected]>
>> ---
>> Based on top of
>> https://lore.kernel.org/linux-usb/[email protected]/T/#t:
>> v1->v2: Added self Signed-Off-by and addressed review comments
>> v2->v3: Encode to usb requests in async_wq; queue to ep in complete handler
>> for isoc transfers.
>> v3->v4: Address review comments around code style.
>> v4->v5: Update comments. Remove 0 length request queueing from async_wq
>> thread since it is already done by the complete handler.
>> v5->v6: Fix checkpatch.pl suggestions.
>>
>> drivers/usb/gadget/function/uvc.h | 8 +
>> drivers/usb/gadget/function/uvc_video.c | 204 ++++++++++++++++++------
>> 2 files changed, 166 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
>> index e8d4c87f1e09..5ff454528bd8 100644
>> --- a/drivers/usb/gadget/function/uvc.h
>> +++ b/drivers/usb/gadget/function/uvc.h
>> @@ -105,7 +105,15 @@ struct uvc_video {
>> bool is_enabled; /* tracks whether video stream is enabled */
>> unsigned int req_size;
>> struct list_head ureqs; /* all uvc_requests allocated by uvc_video */
>> +
>> + /* USB requests that the video pump thread can encode into */
>> struct list_head req_free;
>> +
>> + /*
>> + * USB requests video pump thread has already encoded into. These are
>> + * ready to be queued to the endpoint.
>> + */
>> + struct list_head req_ready;
>> spinlock_t req_lock;
>> unsigned int req_int_count;
>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>> index 53feb790a4c3..d5311456fa8a 100644
>> --- a/drivers/usb/gadget/function/uvc_video.c
>> +++ b/drivers/usb/gadget/function/uvc_video.c
>> @@ -268,6 +268,100 @@ static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req)
>> return ret;
>> }
>> +/* This function must be called with video->req_lock held. */
>> +static int uvcg_video_usb_req_queue(struct uvc_video *video,
>> + struct usb_request *req, bool queue_to_ep)
>> +{
>> + bool is_bulk = video->max_payload_size;
>> + struct list_head *list = NULL;
>> +
>> + if (!video->is_enabled) {
>> + uvc_video_free_request(req->context, video->ep);
>> + return -ENODEV;
>> + }
>> + if (queue_to_ep) {
>> + struct uvc_request *ureq = req->context;
>> + /*
>> + * 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) || ureq->last_buf ||
>> + !(video->req_int_count %
>> + DIV_ROUND_UP(video->uvc_num_requests, 4))) {
>> + video->req_int_count = 0;
>> + req->no_interrupt = 0;
>> + } else {
>> + req->no_interrupt = 1;
>> + }
>> + video->req_int_count++;
>> + return uvcg_video_ep_queue(video, req);
>> + }
>> + /*
>> + * If we're not queuing to the ep, for isoc we're queuing
>> + * to the req_ready list, otherwise req_free.
>> + */
>> + list = is_bulk ? &video->req_free : &video->req_ready;
>> + list_add_tail(&req->list, list);
>> + return 0;
>> +}
>> +
>> +/*
>> + * Must only be called from uvcg_video_enable - since after that we only want to
>> + * queue requests to the endpoint from the uvc_video_complete complete handler.
>> + * This function is needed in order to 'kick start' the flow of requests from
>> + * gadget driver to the usb controller.
>> + */
>> +static void uvc_video_ep_queue_initial_requests(struct uvc_video *video)
>> +{
>> + struct usb_request *req = NULL;
>> + unsigned long flags = 0;
>> + unsigned int count = 0;
>> + int ret = 0;
> Add an empty line here please
Done.
>> + /*
>> + * We only queue half of the free list since we still want to have
>> + * some free usb_requests in the free list for the video_pump async_wq
>> + * thread to encode uvc buffers into. Otherwise we could get into a
>> + * situation where the free list does not have any usb requests to
>> + * encode into - we always end up queueing 0 length requests to the
>> + * end point.
>> + */
>> + unsigned int half_list_size = video->uvc_num_requests / 2;
>> +
>> + spin_lock_irqsave(&video->req_lock, flags);
>> + /*
>> + * Take these requests off the free list and queue them all to the
>> + * endpoint. Since we queue 0 length requests with the req_lock held,
>> + * there isn't any 'data' race involved here with the complete handler.
>> + */
>> + while (count < half_list_size) {
>> + req = list_first_entry(&video->req_free, struct usb_request,
>> + list);
>> + list_del(&req->list);
>> + req->length = 0;
>> + ret = uvcg_video_ep_queue(video, req);
>> + if (ret < 0) {
>> + uvcg_queue_cancel(&video->queue, /*disconnect*/0);
>
>
> Drop the /*disconnect*/ comment please
Done.
>
>> + break;
>> + }
>> + count++;
>> + }
>> + spin_unlock_irqrestore(&video->req_lock, flags);
>> +}
>> +
>> static void
>> uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>> {
>> @@ -276,6 +370,8 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>> struct uvc_video_queue *queue = &video->queue;
>> struct uvc_buffer *last_buf = NULL;
>> unsigned long flags;
>> + bool is_bulk = video->max_payload_size;
>> + int ret = 0;
>> spin_lock_irqsave(&video->req_lock, flags);
>> if (!video->is_enabled) {
>> @@ -329,8 +425,46 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>> * back to req_free
>> */
>> if (video->is_enabled) {
>> - list_add_tail(&req->list, &video->req_free);
>> - queue_work(video->async_wq, &video->pump);
>> + /*
>> + * Here we check whether any request is available in the ready
>> + * list. If it is, queue it to the ep and add the current
>> + * usb_request to the req_free list - for video_pump to fill in.
>> + * Otherwise, just use the current usb_request to queue a 0
>> + * length request to the ep. Since we always add to the req_free
>> + * list if we dequeue from the ready list, there will never
>> + * be a situation where the req_free list is completely out of
>> + * requests and cannot recover.
>> + */
>> + struct usb_request *to_queue = req;
>> +
>> + to_queue->length = 0;
>> + if (!list_empty(&video->req_ready)) {
>> + to_queue = list_first_entry(&video->req_ready,
>> + struct usb_request, list);
>> + list_del(&to_queue->list);
>> + /* Add it to the free list. */
> I would drop the "Add it to the free list" comment; the code is clear already.
Done.
>> + list_add_tail(&req->list, &video->req_free);
>> + /*
>> + * Queue work to the wq as well since it is possible that a
>> + * buffer may not have been completely encoded with the set of
>> + * in-flight usb requests for whih the complete callbacks are
>> + * firing.
>> + * In that case, if we do not queue work to the worker thread,
>> + * the buffer will never be marked as complete - and therefore
>> + * not be returned to userpsace. As a result,
>> + * dequeue -> queue -> dequeue flow of uvc buffers will not
>> + * happen.
>> + */
>> + queue_work(video->async_wq, &video->pump);
>> + }
>> + /*
>> + * Queue to the endpoint. The actual queueing to ep will
>> + * only happen on one thread - the async_wq for bulk endpoints
>> + * and this thread for isoc endpoints.
>> + */
>> + ret = uvcg_video_usb_req_queue(video, to_queue, !is_bulk);
>> + if (ret < 0)
>> + uvcg_queue_cancel(queue, 0);
>> } else {
>> uvc_video_free_request(ureq, ep);
>> }
>> @@ -347,6 +481,7 @@ uvc_video_free_requests(struct uvc_video *video)
>> INIT_LIST_HEAD(&video->ureqs);
>> INIT_LIST_HEAD(&video->req_free);
>> + INIT_LIST_HEAD(&video->req_ready);
>> video->req_size = 0;
>> return 0;
>> }
>> @@ -424,8 +559,7 @@ static void uvcg_video_pump(struct work_struct *work)
>> struct usb_request *req = NULL;
>> struct uvc_buffer *buf;
>> unsigned long flags;
>> - bool buf_done;
>> - int ret;
>> + int ret = 0;
>> while (true) {
>> if (!video->ep->enabled)
>> @@ -454,15 +588,6 @@ static void uvcg_video_pump(struct work_struct *work)
>> if (buf != NULL) {
>> video->encode(req, video, buf);
>> - 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 transferring over ISOC. Queue a 0 length request to
>> - * prevent missed ISOC transfers.
>> - */
>> - req->length = 0;
>> - buf_done = false;
>> } else {
>> /*
>> * Either the queue has been disconnected or no video buffer
>> @@ -473,45 +598,25 @@ static void uvcg_video_pump(struct work_struct *work)
>> break;
>> }
>> - /*
>> - * 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_done ||
>> - !(video->req_int_count %
>> - DIV_ROUND_UP(video->uvc_num_requests, 4))) {
>> - video->req_int_count = 0;
>> - req->no_interrupt = 0;
>> - } else {
>> - req->no_interrupt = 1;
>> - }
>> -
>> - /* Queue the USB request */
>> - ret = uvcg_video_ep_queue(video, req);
>> spin_unlock_irqrestore(&queue->irqlock, flags);
>> + spin_lock_irqsave(&video->req_lock, flags);
>> + /* For bulk end points we queue from the worker thread
>> + * since we would preferably not want to wait on requests
>> + * to be ready, in the uvcg_video_complete() handler.
>> + * For isoc endpoints we add the request to the ready list
>> + * and only queue it to the endpoint from the complete handler.
>> + */
>> + ret = uvcg_video_usb_req_queue(video, req, is_bulk);
>> + spin_unlock_irqrestore(&video->req_lock, flags);
>> +
>> if (ret < 0) {
>> uvcg_queue_cancel(queue, 0);
>> break;
>> }
>> - /* Endpoint now owns the request */
>> + /* The request is owned by the endpoint / ready list. */
>> req = NULL;
>> - video->req_int_count++;
>> }
>> if (!req)
>> @@ -567,7 +672,7 @@ uvcg_video_disable(struct uvc_video *video)
>> spin_lock_irqsave(&video->req_lock, flags);
>> /*
>> - * Remove all uvc_reqeusts from ureqs with list_del_init
>> + * Remove all uvc_requests from ureqs with list_del_init
>
> Did the alignment of the * get messed up here as well as the typo fix or is it just my mail client being weird?
>
I think the alignment did indeed get messed up. Fixed.
Thank you,
Jayant