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

[Manu Gautam]

Hi Felipe,


On 7/19/2017 1:16 PM, Felipe Balbi wrote:
> Hi,
>
> Manu Gautam  writes:
>>> Manu Gautam  writes:
> Manu Gautam  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 = >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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

Hi,

Manu Gautam  writes:
>> Manu Gautam  writes:
 Manu Gautam  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 = >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


signature.asc
Description: PGP signature

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

[Manu Gautam]

Hi,

On 7/18/2017 4:27 PM, Felipe Balbi wrote:
> Hi,
>
> Manu Gautam  writes:
>>> Manu Gautam  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 = >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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

Hi,

Manu Gautam  writes:
>> Manu Gautam  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 
>> 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 = >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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Manu Gautam]

On 7/18/2017 11:53 AM, Felipe Balbi wrote:
> Hi,
>
> Manu Gautam  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 
> 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 = >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).

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

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

[Felipe Balbi]

Hi,

Manu Gautam  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 

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 = >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?

cheers

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