Re: [PATCH] usb: dwc3: gadget: Correct ISOC DATA PIDs for short packets

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

Manu Gautam <mgautam@xxxxxxxxxxxxxx> writes:
>> Manu Gautam <mgautam@xxxxxxxxxxxxxx> writes:
>>> The PIDs for Isochronous data transfers are incorrect
>>> for high bandwidth IN endpoints when the request length
>>> is less than EP wMaxPacketSize.
>>> As per spec correct PIDs for ISOC data transfers are:
>>> ->For request length < maxPayloadSize
>>> 	- DATA0,
>>> ->For maxPayloadSize < length < 2*maxPayloadSize
>>> 	- DATA0,DATA1
>>> ->For 2*maxPayloadSize <  length < 3*maxPayloadSize
>>> 	- DATA2, DATA1, DATA0.
>>>
>>> Fix this by setting the PCM field of trb->size depending
>>> on request length rather than fixing it to the value
>>> depending on wMaxPacketSize.
>>>
>>> Ideally it shouldn't give any issues as dwc3 will send
>>> 0-length packet for next IN token if host sends even
>>> after receiving a short packet. Windows seems to ignore
>>> this but with MacOS frame loss observed when using f_uvc.
>>>
>>> Signed-off-by: Manu Gautam <mgautam@xxxxxxxxxxxxxx>
>> Roger, you guys have been using isoc transfers lately. Does this work
>> for you? Is the current setup really buggy in any way?
>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index aea9a5b..b81547d 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -854,8 +854,13 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
>>>  			trb->ctrl = DWC3_TRBCTL_ISOCHRONOUS_FIRST;
>>>  
>>>  			if (speed == USB_SPEED_HIGH) {
>>> -				struct usb_ep *ep = &dep->endpoint;
>>> -				trb->size |= DWC3_TRB_SIZE_PCM1(ep->mult - 1);
>>> +				unsigned int maxp = usb_endpoint_maxp(
>>> +							dep->endpoint.desc);
>>> +				unsigned int rem = length % maxp;
>>> +				unsigned int mult = (length / maxp) & 0x3;
>>> +
>>> +				trb->size |= DWC3_TRB_SIZE_PCM1(
>>> +						rem ? mult : mult - 1);
>> Manu, It seems to me like we shouldn't be relying on req->length. Which
>> gadget driver are you using to test this?
>
> f_uvc function is used.
> In bus analyzer logs there are DATA2, DATA1 PIDs even for a 2K byte TRB
> (also last packet of the video frame are always less than maxpacket size).

Understood, yeah it makes sense, although I think your patch can be
simplified. Seems to me that it should be enough to set PCM1 to
req->length / usb_endpoint_maxp(), no?

Or, if we want to make use of ep->mult, we could:

	unsigned int mult = ep->mult - 1;

	if (req->length < (usb_endpoint_maxp() << 1))
		mult--;

	if (req->length < usb_endpoint_maxp())
		mult--;

	trb->size |= DWC3_TRB_SIZE_PCM1(mult);

how about that?

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux