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

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

[Bin Liu]

From: Manu Gautam 

commit 40d829fb2ec636b6b4b0cc95e2546ab9aca04cc9 upstream.

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:

1) For request length <= maxpacket
- DATA0,

2) For maxpacket < length <= (2 * maxpacket)
- DATA1, DATA0

3) For (2 * maxpacket) <  length <= (3 * maxpacket)
- DATA2, DATA1, DATA0.

But driver always sets PCM fields based on wMaxPacketSize
due to which DATA2 happens even for small requests.

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 
Signed-off-by: Felipe Balbi 
Cc: sta...@vger.kernel.org # v4.9 only
[b-...@ti.com added following change for v4.9.]

-   unsigned int maxp = usb_endpoint_maxp(ep->desc);
+   unsigned int maxp;
+   maxp = usb_endpoint_maxp(ep->desc) & 0x07ff;

Signed-off-by: Bin Liu 
---
 drivers/usb/dwc3/gadget.c | 35 ++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 0546fb60d095..b1dbf2d65553 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -817,9 +817,42 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
if (!node) {
trb->ctrl = DWC3_TRBCTL_ISOCHRONOUS_FIRST;
 
+   /*
+* USB Specification 2.0 Section 5.9.2 states that: "If
+* there is only a single transaction in the microframe,
+* only a DATA0 data packet PID is used.  If there are
+* two transactions per microframe, DATA1 is used for
+* the first transaction data packet and DATA0 is used
+* for the second transaction data packet.  If there are
+* three transactions per microframe, DATA2 is used for
+* the first transaction data packet, DATA1 is used for
+* the second, and DATA0 is used for the third."
+*
+* IOW, we should satisfy the following cases:
+*
+* 1) length <= maxpacket
+*  - DATA0
+*
+* 2) maxpacket < length <= (2 * maxpacket)
+*  - DATA1, DATA0
+*
+* 3) (2 * maxpacket) < length <= (3 * maxpacket)
+*  - DATA2, DATA1, DATA0
+*/
if (speed == USB_SPEED_HIGH) {
struct usb_ep *ep = >endpoint;
-   trb->size |= DWC3_TRB_SIZE_PCM1(ep->mult - 1);
+   unsigned int mult = ep->mult - 1;
+   unsigned int maxp;
+
+   maxp = usb_endpoint_maxp(ep->desc) & 0x07ff;
+
+   if (length <= (2 * maxp))
+   mult--;
+
+   if (length <= maxp)
+   mult--;
+
+   trb->size |= DWC3_TRB_SIZE_PCM1(mult);
}
} else {
trb->ctrl = DWC3_TRBCTL_ISOCHRONOUS;
-- 
1.9.1

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

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

[Manu Gautam]

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 

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);
}
} else {
trb->ctrl = DWC3_TRBCTL_ISOCHRONOUS;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na 
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