Hi Felipe,
On 7/19/2017 1:16 PM, Felipe Balbi wrote:
> Hi,
>
> Manu Gautam <mgautam@xxxxxxxxxxxxxx> writes:
>>> Manu Gautam <mgautam@xxxxxxxxxxxxxx> writes:
>>>>> Manu Gautam <mgautam@xxxxxxxxxxxxxx> writes:
>>>>> 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?
>> Still need to take care of two things:
>> 1. Handle case if If req>length is more than 3K (buggy function driver)
>> 2. We don't need to send extra packet for isoc if length is multiple of maxp.
>> Hence, remainder must be checked.
>>
>>> Or, if we want to make use of ep->mult, we could:
>>>
>>> unsigned int mult = ep->mult - 1;
It should be:
mult = 2;
Otherwise this logic works correctly only for 3K transfers. And for short packets '11' is
programmed as PCM1 (as mult becomes negative).
I didn't test updated patch for other mult values earlier, sorry about that. Will be sending
a fix for this.
>>>
>>> if (req->length < (usb_endpoint_maxp() << 1))
>>> mult--;
>> I think it should be <=
>> E.g. for 2k size only two transfers should take place)
>>
>>
>>> if (req->length < usb_endpoint_maxp())
>>> mult--;
>> <=
>>
>>> trb->size |= DWC3_TRB_SIZE_PCM1(mult);
>>>
>>> how about that?
>>>
>> This also looks fine and I can send the updated patch.
> please do. While doing that, please also add a comment pointing out the
> USB Spec section you took it from and a simplified text of why we need
> it. This way, nobody will dare changing that part of the code without
> checking the spec ;-)
>
> IOW, add something akin to:
>
> /*
> * USB Specification X.x Section Y states that "...."
> *
> * IOW, we should satisfy the following cases:
> *
> * i) req->length <= wMaxPacketSize
> * - DATA0
> *
> * ii) wMaxPacketSize < req->length <= (2 * wMaxPacketSize)
> * - DATA0, DATA1
> *
> * iii) (2 * maxPayloadSize) < req->length <= (3 * maxPayloadSize)
> * - DATA2, DATA1, DATA0
> */
>
> Or something similar to that.
>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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
