Re: [PATCH 5/7] usb: gadget: uvc: only pump video data if necessary

[Michael Grzeschik <mgr@pengutronix.de>]

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

On Tue, Oct 05, 2021 at 01:29:25AM +0300, Laurent Pinchart wrote:
>Hi Michael,
>
>Thank you for the patch.
>
>On Thu, Sep 30, 2021 at 12:27:15PM +0200, Michael Grzeschik wrote:
>> If the streaming endpoint is not enabled, the worker has nothing to do.
>> In the case it still got enqueued, this patch ensueres that it will bail
>
>s/it still got enqueued/buffers are still queued/
>s/ensueres/ensures/

Thanks, will fix this and your other comments on the patches in v3.

>> out without handling any data.
>
>Does this happen when uvc_function_set_alt() calls usb_ep_disable() ?
>The current implementation will cause usb_ep_queue() (called from
>uvcg_video_ep_queue(), from uvcg_video_pump()) to return an error in
>that case, which will result in uvcg_queue_cancel() being called in
>uvcg_video_pump(). With this patch, I believe this will still work fine
>as userspace is notified of the stream off event and calls
>VIDIOC_STREAMOFF, which in turn calls uvcg_video_enable(0) from
>uvc_v4l2_streamoff() (or uvcg_video_enable(0) gets called from
>uvc_v4l2_release() in the worst case if the application closes the
>device). Could you confirm that your understanding matches this analysis
>? If so,

Yes! I can confirm your analysis correct.

>
>Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>>  drivers/usb/gadget/function/uvc_video.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>> index ccee35177411d..152647495fa61 100644
>> --- a/drivers/usb/gadget/function/uvc_video.c
>> +++ b/drivers/usb/gadget/function/uvc_video.c
>> @@ -335,12 +335,12 @@ 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;
>> -	struct usb_request *req;
>> +	struct usb_request *req = NULL;
>>  	struct uvc_buffer *buf;
>>  	unsigned long flags;
>>  	int ret;
>>
>> -	while (1) {
>> +	while (video->ep->enabled) {
>>  		/* Retrieve the first available USB request, protected by the
>>  		 * request lock.
>>  		 */
>> @@ -390,6 +390,9 @@ static void uvcg_video_pump(struct work_struct *work)
>>  		video->req_int_count++;
>>  	}
>>
>> +	if (!req)
>> +		return;
>> +
>>  	spin_lock_irqsave(&video->req_lock, flags);
>>  	list_add_tail(&req->list, &video->req_free);
>>  	spin_unlock_irqrestore(&video->req_lock, flags);
>
>-- 
>Regards,
>
>Laurent Pinchart
>

-- 
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 --]