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;
>>
>> 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.
--
balbi
Attachment:
signature.asc
Description: PGP signature
