On Wed, Aug 5, 2020 at 7:21 PM Felipe Balbi <balbi@xxxxxxxxxx> wrote:
> But anyway, if we change 13 to 16, then we 1Gbps. How did you get
> 1.7Gbps? I think we should really update ncm_bitrate() to contain the
> correct equations for bitrate on different speeds.
I got 1.7 Gbps because that's what I measured in iperf. :-)
But actually after reading the spec I think that for SuperSpeed and
above that calculation is meaningless, because bulk transfers are no
longer limited by a set number of packets per microframe. The USB 3.2
spec has mostly replaced the words "microframe" with "bus interval",
but I don't see any place in the spec that says the number of packets
per bus interval is limited. Section 8.12.1.2 (Bulk IN Transactions)
just says that "when the host is ready to receive bulk data, it sends
an ACK TP" saying how many packets it's willing to accept, where the
maximum is the burst size supported by the endpoint. After that, the
host has to respond with an ACK to every data packet received, and
every ACK specifies the number of data packets it expects from then
on.
It seems more correct that for SS and above the driver should simply
just report the link speed minus theoretical bus overhead? Section
4.4.11 suggests that's about 10%, so it would announce 4.5 Gbps.
> > +static struct usb_ss_ep_comp_descriptor ssp_ncm_bulk_comp_desc = {
> > + .bLength = sizeof(ssp_ncm_bulk_comp_desc),
> > + .bDescriptorType = USB_DT_SS_ENDPOINT_COMP,
> > +
> > + /* the following 2 values can be tweaked if necessary */
> > + /* .bMaxBurst = 0, */
>
> should you add bMaxBurst = 15 here?
I'm not sure. On my setup, it provides a fair performance boost (goes
from ~1.7Gbps to ~2.3Gbps in, and ~620Mbps to ~720Mbps out). But I
don't know whether there might be any compatibility constraints or
hardware dependencies. I do see that the f_mass_storage driver sets it
to 15:
/* Calculate bMaxBurst, we know packet size is 1024 */
max_burst = min_t(unsigned, FSG_BUFLEN / 1024, 15);
so perhaps this is fine to do in NCM too? If we want to set bMaxBurst
to 15, should that be in this patch, or in a separate patch?
Cheers,
Lorenzo
