* [2/4] usb: dwc3: gadget: check if dep->frame_number is still valid
@ 2018-08-20 10:29 Felipe Balbi
0 siblings, 0 replies; 5+ messages in thread
From: Felipe Balbi @ 2018-08-20 10:29 UTC (permalink / raw)
To: linux-usb; +Cc: laurent.pinchart, Felipe Balbi
Gadget driver may take an unbounded amount of time to queue requests
after XferNotReady. This is important for isochronous endpoints which
need to be started for a specific (micro-)frame.
Before kicking the transfer, let's check if current frame number is
still less than our aligned frame number that we got from the
previous XferNotReady. If it isn't, then we'll increment
dep->frame_number to make sure it's ahead of current frame number.
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
drivers/usb/dwc3/core.h | 2 ++
drivers/usb/dwc3/gadget.c | 11 +++++++++++
2 files changed, 13 insertions(+)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 285ce0ef3b91..3acf8788a680 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -685,6 +685,8 @@ struct dwc3_ep {
u8 type;
u8 resource_index;
u32 frame_number;
+#define DWC3_EP_FRAME_NUMBER_MASK 0x3fff
+
u32 interval;
char name[20];
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index f61a4250c883..0bac9b02f28b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1257,6 +1257,9 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
{
+ u16 current_frame_number;
+ u16 frame_number;
+
if (list_empty(&dep->pending_list)) {
dev_info(dep->dwc->dev, "%s: ran out of requests\n",
dep->name);
@@ -1265,6 +1268,14 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
}
dep->frame_number = DWC3_ALIGN_FRAME(dep);
+ current_frame_number = __dwc3_gadget_get_frame(dep->dwc);
+ frame_number = dep->frame_number & DWC3_EP_FRAME_NUMBER_MASK;
+
+ if (frame_number <= current_frame_number) {
+ dep->frame_number += current_frame_number - frame_number;
+ dep->frame_number = DWC3_ALIGN_FRAME(dep);
+ }
+
return __dwc3_gadget_kick_transfer(dep);
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [2/4] usb: dwc3: gadget: check if dep->frame_number is still valid
@ 2018-08-20 11:28 Laurent Pinchart
0 siblings, 0 replies; 5+ messages in thread
From: Laurent Pinchart @ 2018-08-20 11:28 UTC (permalink / raw)
To: Felipe Balbi; +Cc: linux-usb, Kieran Bingham
Hi Felipe,
(CC'ing Kieran)
Thank you for the patch.
On Monday, 20 August 2018 13:29:59 EEST Felipe Balbi wrote:
> Gadget driver may take an unbounded amount of time to queue requests
> after XferNotReady. This is important for isochronous endpoints which
> need to be started for a specific (micro-)frame.
>
> Before kicking the transfer, let's check if current frame number is
> still less than our aligned frame number that we got from the
> previous XferNotReady. If it isn't, then we'll increment
> dep->frame_number to make sure it's ahead of current frame number.
>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
> drivers/usb/dwc3/core.h | 2 ++
> drivers/usb/dwc3/gadget.c | 11 +++++++++++
> 2 files changed, 13 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 285ce0ef3b91..3acf8788a680 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -685,6 +685,8 @@ struct dwc3_ep {
> u8 type;
> u8 resource_index;
> u32 frame_number;
> +#define DWC3_EP_FRAME_NUMBER_MASK 0x3fff
> +
> u32 interval;
>
> char name[20];
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index f61a4250c883..0bac9b02f28b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1257,6 +1257,9 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>
> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> {
> + u16 current_frame_number;
> + u16 frame_number;
> +
> if (list_empty(&dep->pending_list)) {
> dev_info(dep->dwc->dev, "%s: ran out of requests\n",
> dep->name);
> @@ -1265,6 +1268,14 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep
> *dep) }
>
> dep->frame_number = DWC3_ALIGN_FRAME(dep);
> + current_frame_number = __dwc3_gadget_get_frame(dep->dwc);
> + frame_number = dep->frame_number & DWC3_EP_FRAME_NUMBER_MASK;
> +
> + if (frame_number <= current_frame_number) {
> + dep->frame_number += current_frame_number - frame_number;
> + dep->frame_number = DWC3_ALIGN_FRAME(dep);
> + }
> +
This greatly improves the situation with the UVC function driver. However,
we're still receiving errors from time to time related to missed transfer
intervals, and those errors tend to occur in bursts. Kieran should be able to
provide you with more information. Before acking this patch I would thus like
to make sure that we're not missing part of the problem.
> return __dwc3_gadget_kick_transfer(dep);
> }
^ permalink raw reply [flat|nested] 5+ messages in thread
* [2/4] usb: dwc3: gadget: check if dep->frame_number is still valid
@ 2018-08-20 18:35 Thinh Nguyen
0 siblings, 0 replies; 5+ messages in thread
From: Thinh Nguyen @ 2018-08-20 18:35 UTC (permalink / raw)
To: Felipe Balbi, linux-usb@vger.kernel.org; +Cc: laurent.pinchart@ideasonboard.com
Hi Felipe,
On 8/20/2018 3:34 AM, Felipe Balbi wrote:
> Gadget driver may take an unbounded amount of time to queue requests
> after XferNotReady. This is important for isochronous endpoints which
> need to be started for a specific (micro-)frame.
>
> Before kicking the transfer, let's check if current frame number is
> still less than our aligned frame number that we got from the
> previous XferNotReady. If it isn't, then we'll increment
> dep->frame_number to make sure it's ahead of current frame number.
>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
> drivers/usb/dwc3/core.h | 2 ++
> drivers/usb/dwc3/gadget.c | 11 +++++++++++
> 2 files changed, 13 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 285ce0ef3b91..3acf8788a680 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -685,6 +685,8 @@ struct dwc3_ep {
> u8 type;
> u8 resource_index;
> u32 frame_number;
> +#define DWC3_EP_FRAME_NUMBER_MASK 0x3fff
> +
> u32 interval;
>
> char name[20];
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index f61a4250c883..0bac9b02f28b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1257,6 +1257,9 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>
> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> {
> + u16 current_frame_number;
> + u16 frame_number;
> +
> if (list_empty(&dep->pending_list)) {
> dev_info(dep->dwc->dev, "%s: ran out of requests\n",
> dep->name);
> @@ -1265,6 +1268,14 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> }
>
> dep->frame_number = DWC3_ALIGN_FRAME(dep);
> + current_frame_number = __dwc3_gadget_get_frame(dep->dwc);
> + frame_number = dep->frame_number & DWC3_EP_FRAME_NUMBER_MASK;
> +
> + if (frame_number <= current_frame_number) {
This is not right. You're comparing (what should be a 16-bit but masked)
frame_number to a 14-bit current_frame_number.
If the current frame number is 2+ seconds ahead of dep->frame_number,
then this calculation is wrong.
> + dep->frame_number += current_frame_number - frame_number;
> + dep->frame_number = DWC3_ALIGN_FRAME(dep);
> + }
> +
> return __dwc3_gadget_kick_transfer(dep);
> }
>
Thinh
^ permalink raw reply [flat|nested] 5+ messages in thread
* [2/4] usb: dwc3: gadget: check if dep->frame_number is still valid
@ 2018-08-23 10:11 Felipe Balbi
0 siblings, 0 replies; 5+ messages in thread
From: Felipe Balbi @ 2018-08-23 10:11 UTC (permalink / raw)
To: Thinh Nguyen, linux-usb@vger.kernel.org; +Cc: laurent.pinchart@ideasonboard.com
Hi,
Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> On 8/20/2018 3:34 AM, Felipe Balbi wrote:
>> Gadget driver may take an unbounded amount of time to queue requests
>> after XferNotReady. This is important for isochronous endpoints which
>> need to be started for a specific (micro-)frame.
>>
>> Before kicking the transfer, let's check if current frame number is
>> still less than our aligned frame number that we got from the
>> previous XferNotReady. If it isn't, then we'll increment
>> dep->frame_number to make sure it's ahead of current frame number.
>>
>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>> ---
>> drivers/usb/dwc3/core.h | 2 ++
>> drivers/usb/dwc3/gadget.c | 11 +++++++++++
>> 2 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 285ce0ef3b91..3acf8788a680 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -685,6 +685,8 @@ struct dwc3_ep {
>> u8 type;
>> u8 resource_index;
>> u32 frame_number;
>> +#define DWC3_EP_FRAME_NUMBER_MASK 0x3fff
>> +
>> u32 interval;
>>
>> char name[20];
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index f61a4250c883..0bac9b02f28b 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1257,6 +1257,9 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>>
>> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>> {
>> + u16 current_frame_number;
>> + u16 frame_number;
>> +
>> if (list_empty(&dep->pending_list)) {
>> dev_info(dep->dwc->dev, "%s: ran out of requests\n",
>> dep->name);
>> @@ -1265,6 +1268,14 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>> }
>>
>> dep->frame_number = DWC3_ALIGN_FRAME(dep);
>> + current_frame_number = __dwc3_gadget_get_frame(dep->dwc);
>> + frame_number = dep->frame_number & DWC3_EP_FRAME_NUMBER_MASK;
>> +
>> + if (frame_number <= current_frame_number) {
>
> This is not right. You're comparing (what should be a 16-bit but masked)
> frame_number to a 14-bit current_frame_number.
> If the current frame number is 2+ seconds ahead of dep->frame_number,
> then this calculation is wrong.
According to docs, the numbers are the same and the top 2 bits are used
internally by the IP. I.E. they're not part of the frame number. This is
described in table 6-51 on databook 2.60a.
In any case, there's no other way to account for the overflow since the
HW doesn't give me all the bits I need in DSTS. Any ideas?
^ permalink raw reply [flat|nested] 5+ messages in thread
* [2/4] usb: dwc3: gadget: check if dep->frame_number is still valid
@ 2018-08-24 0:28 Thinh Nguyen
0 siblings, 0 replies; 5+ messages in thread
From: Thinh Nguyen @ 2018-08-24 0:28 UTC (permalink / raw)
To: Felipe Balbi, Thinh Nguyen, linux-usb@vger.kernel.org
Cc: laurent.pinchart@ideasonboard.com
On 8/23/2018 3:15 AM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> On 8/20/2018 3:34 AM, Felipe Balbi wrote:
>>> Gadget driver may take an unbounded amount of time to queue requests
>>> after XferNotReady. This is important for isochronous endpoints which
>>> need to be started for a specific (micro-)frame.
>>>
>>> Before kicking the transfer, let's check if current frame number is
>>> still less than our aligned frame number that we got from the
>>> previous XferNotReady. If it isn't, then we'll increment
>>> dep->frame_number to make sure it's ahead of current frame number.
>>>
>>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>>> ---
>>> drivers/usb/dwc3/core.h | 2 ++
>>> drivers/usb/dwc3/gadget.c | 11 +++++++++++
>>> 2 files changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 285ce0ef3b91..3acf8788a680 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -685,6 +685,8 @@ struct dwc3_ep {
>>> u8 type;
>>> u8 resource_index;
>>> u32 frame_number;
>>> +#define DWC3_EP_FRAME_NUMBER_MASK 0x3fff
>>> +
>>> u32 interval;
>>>
>>> char name[20];
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index f61a4250c883..0bac9b02f28b 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -1257,6 +1257,9 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>>>
>>> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>>> {
>>> + u16 current_frame_number;
>>> + u16 frame_number;
>>> +
>>> if (list_empty(&dep->pending_list)) {
>>> dev_info(dep->dwc->dev, "%s: ran out of requests\n",
>>> dep->name);
>>> @@ -1265,6 +1268,14 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>>> }
>>>
>>> dep->frame_number = DWC3_ALIGN_FRAME(dep);
>>> + current_frame_number = __dwc3_gadget_get_frame(dep->dwc);
>>> + frame_number = dep->frame_number & DWC3_EP_FRAME_NUMBER_MASK;
>>> +
>>> + if (frame_number <= current_frame_number) {
>> This is not right. You're comparing (what should be a 16-bit but masked)
>> frame_number to a 14-bit current_frame_number.
>> If the current frame number is 2+ seconds ahead of dep->frame_number,
>> then this calculation is wrong.
> According to docs, the numbers are the same and the top 2 bits are used
> internally by the IP. I.E. they're not part of the frame number. This is
> described in table 6-51 on databook 2.60a.
>
> In any case, there's no other way to account for the overflow since the
> HW doesn't give me all the bits I need in DSTS. Any ideas?
>
You can check if there's pending/started request, then you can kick the
transfer. Otherwise don't kick transfer and send END_TRANSFER command so
that the controller can resend XferNotReady event to restart again.
Thinh
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-08-24 0:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-08-20 11:28 [2/4] usb: dwc3: gadget: check if dep->frame_number is still valid Laurent Pinchart
-- strict thread matches above, loose matches on Subject: below --
2018-08-24 0:28 Thinh Nguyen
2018-08-23 10:11 Felipe Balbi
2018-08-20 18:35 Thinh Nguyen
2018-08-20 10:29 Felipe Balbi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).