Lorenzo Colitti <lorenzo@xxxxxxxxxx> writes:
> 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.
makes sense to me :-)
>> > +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:
there shouldn't be any compatibility issues, no.
> /* 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?
yup, should be fine. A separate patch is okay too :-)
--
balbi
Attachment:
signature.asc
Description: PGP signature
