RE: [EXT] Re: [PATCH v5] usb: gadget: uvc: add bulk transfer support

[Jing Leng]

Hi Dan Scally,

>> -		===================	=============================
>> +		===================	===================================
>>   		streaming_maxburst	0..15 (ss only)
>> -		streaming_maxpacket	1..1023 (fs), 1..3072 (hs/ss)
>> +		streaming_maxpacket	isoc: 1..1023 (fs), 1..3072 (hs/ss)
>> +					bulk: 1024..0x40000000
>
>
>I'm not really sure that I like this way of representing things, since what you're setting with streaming_maxpacket here is not wMaxPacketSize but the internal max_payload_size variable. I think that that's apt to be quite confusing to people, since the possible values you've listed disagree with the specs. It also precludes setting non-maximum values for full-speed endpoints, which ought to be able to support 8, 16 and 32 bits too. I'd prefer another attribute / some other way of determining the max_payload_size and full control over the bulk endpoint sizes through streaming_maxpacket.

UVC requires high bandwidth, so we'd better give the maximum wMaxPacketSize for full-speed ep.
And wMaxPacketSize of bulk ep is a fixed value in high-speed and supper-speed, so if we use streaming_maxpacket to set wMaxPacketSize for bulk ep, it not a good idea.
If we respond to UVC_SC_VIDEOSTREAMING request, we should set max_payload_size to dwMaxPayloadTransferSize in bulk mode, Reusing streaming_maxpacket keeps setting dwMaxPayloadTransferSize uniform in bulk mode and isoc mode.


>>   static void
>>   uvc_function_ep0_complete(struct usb_ep *ep, struct usb_request *req)
>>   {
>> @@ -219,6 +225,13 @@ uvc_function_ep0_complete(struct usb_ep *ep, struct usb_request *req)
>>   		uvc_event->data.length = req->actual;
>>   		memcpy(&uvc_event->data.data, req->buf, req->actual);
>>   		v4l2_event_queue(&uvc->vdev, &v4l2_event);
>> +
>> +		/*
>> +		 * Bulk mode only has one alt, so we should set STREAM ON after
>> +		 * responding the SET UVC_VS_COMMIT_CONTROL request.
>> +		 */
>> +		if (uvc->state == UVC_STATE_BULK_SETTING)
>> +			uvc_function_set_alt(&uvc->func, uvc->streaming_intf, 1);
>>   	}
>>   }
>
>
>Given bulk mode only has one alt, perhaps it's better to add new functions for stream on/off and call those (including in
>uvc_function_set_alt()) to make it clear what's happening.

UVC will set 'alt 0' when be configured, not only in the SET UVC_VS_COMMIT_CONTROL request  added by me.

>>   
>> @@ -228,6 +241,9 @@ uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
>>   	struct uvc_device *uvc = to_uvc(f);
>>   	struct v4l2_event v4l2_event;
>>   	struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
>> +	struct f_uvc_opts *opts = fi_to_f_uvc_opts(f->fi);
>> +	unsigned int interface = le16_to_cpu(ctrl->wIndex) & 0xff;
>> +	unsigned int cs = le16_to_cpu(ctrl->wValue) >> 8 & 0xff;
>>   
>>   	if ((ctrl->bRequestType & USB_TYPE_MASK) != USB_TYPE_CLASS) {
>>   		uvcg_info(f, "invalid request type\n"); @@ -245,6 +261,21 @@ 
>> uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
>>   	uvc->event_setup_out = !(ctrl->bRequestType & USB_DIR_IN);
>>   	uvc->event_length = le16_to_cpu(ctrl->wLength);
>>   
>> +	/*
>> +	 * Bulk mode only has one alt, when the SET UVC_VS_COMMIT_CONTROL request
>> +	 * is received, if the streaming is in transit, we need to set STREAM OFF,
>> +	 * if the uvc state is UVC_STATE_BULK_WAITING, we only need to change it.
>> +	 */
>> +	if (opts->streaming_transfer == USB_ENDPOINT_XFER_BULK &&
>> +		uvc->event_setup_out &&
>> +		uvc->streaming_intf == interface &&
>> +		cs == UVC_VS_COMMIT_CONTROL) {
>> +		if (uvc->state == UVC_STATE_STREAMING)
>> +			uvc_function_set_alt(&uvc->func, uvc->streaming_intf, 0);
>> +		else if (uvc->state == UVC_STATE_BULK_WAITING)
>> +			uvc->state = UVC_STATE_BULK_SETTING;
>> +	}
>
>
>As far as I can tell this path doesn't quite work out; when I test the code I don't get uvc_function_set_alt(1, 0) being called, so the streamoff isn't happening correctly when in bulk mode. 
uvc_video_stop_streaming() in drivers/media/usb/uvc/uvc_video.c has this
comment:

UVC will set 'alt 0' when be configured, so the first play will execute 'uvc->state = UVC_STATE_BULK_SETTING', and  'alt 0' will be executed when replaying.
So I add these logics in uvc_function_setup and uvc_function_ep0_complete.

>/*
>* UVC doesn't specify how to inform a bulk-based device
>* when the video stream is stopped. Windows sends a
>* CLEAR_FEATURE(HALT) request to the video streaming
>* bulk endpoint, mimic the same behaviour.
>*/
>
>Perhaps it would be good to accommodate that method?

I test uvc on Windows 10 (PotPlayer) and Ubuntu 20.04 (v4l2-ctl) with cdnsp UDC, composite_setup doesn't recieve a CLEAR_FEATURE request when replaying, See the following print info:

[  764.069697] composite_setup: 21 1 100 1 1a
[  764.069914] composite_setup: a1 82 100 1 1a
[  764.070088] composite_setup: a1 83 100 1 1a
[  764.070389] composite_setup: 21 1 100 1 1a
[  764.070552] composite_setup: a1 81 100 1 1a
[  764.072136] composite_setup: 21 1 200 1 1a
[  764.072141] configfs-gadget gadget: uvc: uvc_function_set_alt(1, 0)
[  765.057620] configfs-gadget gadget: uvc: uvc_function_set_alt(1, 1)
[  765.063911] configfs-gadget gadget: uvc: reset UVC
[  767.094115] composite_setup: 21 1 100 1 1a
[  767.094321] composite_setup: a1 82 100 1 1a
[  767.094502] composite_setup: a1 83 100 1 1a
[  767.094683] composite_setup: 21 1 100 1 1a
[  767.094864] composite_setup: a1 81 100 1 1a
[  767.096469] composite_setup: 21 1 200 1 1a
[  767.096475] configfs-gadget gadget: uvc: uvc_function_set_alt(1, 0)
[  768.082019] configfs-gadget gadget: uvc: uvc_function_set_alt(1, 1)
[  768.088310] configfs-gadget gadget: uvc: reset UVC

**********************************************************************
This email and attachments contain Ambarella Proprietary and/or Confidential Information and is intended solely for the use of the individual(s) to whom it is addressed. Any unauthorized review, use, disclosure, distribute, copy, or print is prohibited. If you are not an intended recipient, please contact the sender by reply email and destroy all copies of the original message. Thank you.