Re: [RFC v4 1/3] usb: dwc3: Resize TX FIFOs to meet EP bursting requirements

[Felipe Balbi <balbi@kernel.org>]

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


Hi,

Wesley Cheng <wcheng@codeaurora.org> writes:
> On 8/10/2020 5:27 AM, Felipe Balbi wrote:
>> Wesley Cheng <wcheng@codeaurora.org> writes:
>> 
>> Hi,
>> 
>>> Some devices have USB compositions which may require multiple endpoints
>>> that support EP bursting.  HW defined TX FIFO sizes may not always be
>>> sufficient for these compositions.  By utilizing flexible TX FIFO
>>> allocation, this allows for endpoints to request the required FIFO depth to
>>> achieve higher bandwidth.  With some higher bMaxBurst configurations, using
>>> a larger TX FIFO size results in better TX throughput.
>> 
>> how much better? What's the impact? Got some real numbers of this
>> running with upstream kernel? I guess mass storage gadget is the
>> simplest one to test.
>> 
> Hi Felipe,
>
> Thanks for the input.
>
> Sorry for not including the numbers in the patch itself, but I did
> mention the set of mass storage tests I ran w/ the upstream kernel on
> SM8150 in the cover letter.  Let me just share that here:
>
> Test Parameters:
>  - Platform: Qualcomm SM8150
>  - bMaxBurst = 6
>  - USB req size = 256kB
>  - Num of USB reqs = 16
>  - USB Speed = Super-Speed
>  - Function Driver: Mass Storage (w/ ramdisk)
>  - Test Application: CrystalDiskMark
>
> Results:
>
> TXFIFO Depth = 3 max packets
>
> Test Case | Data Size | AVG tput (in MB/s)
> -------------------------------------------
> Sequential|1 GB x     |
> Read      |9 loops    | 193.60
> 	  |           | 195.86
>           |           | 184.77
>           |           | 193.60
> -------------------------------------------
>
> TXFIFO Depth = 6 max packets
>
> Test Case | Data Size | AVG tput (in MB/s)
> -------------------------------------------
> Sequential|1 GB x     |
> Read      |9 loops    | 287.35
> 	  |           | 304.94
>           |           | 289.64
>           |           | 293.61
> -------------------------------------------

awesome, thanks a lot for this :-) It's a considerable increase in your
setup. My only fear here is that we may end up creating a situation
where we can't allocate enough FIFO for all endpoints. This is, of
course, a consequence of the fact that we enable one endpoint at a
time.

Perhaps we could envision a way where function driver requests endpoints
in bulk, i.e. combines all endpoint requirements into a single method
call for gadget framework and, consequently, for UDC.

>>> +	if (!dwc->needs_fifo_resize)
>>> +		return 0;
>>> +
>>> +	/* resize IN endpoints except ep0 */
>>> +	if (!usb_endpoint_dir_in(dep->endpoint.desc) || dep->number <= 1)
>>> +		return 0;
>>> +
>>> +	/* Don't resize already resized IN endpoint */
>>> +	if (dep->fifo_depth)
>> 
>> using fifo_depth as a flag seems flakey to me. What happens when someone
>> in the future changes the behavior below and this doesn't apply anymore?
>> 
>> Also, why is this procedure called more than once for the same endpoint?
>> Does that really happen?
>> 
> I guess it can be considered a bug elsewhere (ie usb gadget or function
> driver) if this happens twice.  Plus, if we decide to keep this in the
> dwc3 enable endpoint path, the DWC3_EP_ENABLED flag will ensure it's
> called only once as well.  Its probably overkill to check fifo_depth here.

We could add a dev_WARN_ONCE() just to catch any possible bugs elsewhere.

>>> +	if (remaining < fifo_size) {
>>> +		if (remaining > 0)
>>> +			fifo_size = remaining;
>>> +		else
>>> +			fifo_size = 0;
>>> +	}
>>> +
>>> +	fifo_size += fifo;
>>> +	fifo_size++;
>> 
>> why the increment?
>> 
> This is to account for the last +1 in the equation from the DWC3 databook:
> fifo_size = mult * ((max_packet + mdwidth)/mdwidth + 1) + 1 <- this one

great, could you add this detail as a comment so it doesn't look as
cryptic? :-)

>>> +	return 0;
>>> +}
>>> +
>>>  static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
>>>  {
>>>  	const struct usb_ss_ep_comp_descriptor *comp_desc;
>>> @@ -620,6 +731,10 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
>>>  	int			ret;
>>>  
>>>  	if (!(dep->flags & DWC3_EP_ENABLED)) {
>>> +		ret = dwc3_gadget_resize_tx_fifos(dep);
>>> +		if (ret)
>>> +			return ret;
>> 
>> doesn't it look odd that you're resizing every fifo every time a new
>> endpoint is enabled? Is there a better way to achieve this?
>> 
> We're only resizing a single fifo per call, and clearing the previous
> fifo configuration upon receiving the set address.  In the past, I know
> the change was to resize all fifos after receiving the set configuration
> packet.  With that approach, I believe we saw issues with some function
> drivers that immediately queued a USB request during their set_alt()
> routine, followed by the dwc3 ep0 driver calling the TX fifo resize
> API.(as the tx fifo resize was executed after we delegated the set
> config packet to the USB composite)

I don't remember seeing such an issue. Allocating FIFOs after we know
the entire requirements would avoid another possible situation, that of
dwc3 exausting FIFO space before it knows there are more enpdoints to
enable.

One possibility around this was suggested above, something along the
lines of:

	usb_gadget_ep_enable_bulk(struct usb_gadget *, struct
		usb_ep_alloc_desc *alloc_desc)

(please think of better names, I'm hopeless haha)

-- 
balbi

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