Hi,

Doug Anderson <[email protected]> writes:
>> Douglas Anderson <[email protected]> writes:
>>> Until we have logic to determine which devices share the same TT let's
>>> add logic to assume that all devices on a given dwc2 controller are on
>>> one single_tt hub. This is better than the previous code that assumed
>>> that all devices were on one multi_tt hub, since single_tt hubs
>>> appear (in my experience) to be much more common and any schedule that
>>> would work on a single_tt hub will also work on a multi_tt hub. This
>>> will prevent more than 8 total low/full speed devices to be on the bus
>>> at one time, but that's a reasonable restriction until we've made things
>>> smarter.
>>>
>>> Signed-off-by: Douglas Anderson <[email protected]>
>>> ---
>>> Changes in v3:
>>> - Assuming single_tt is new for v3; not terribly well tested (yet).
>>>
>>> Changes in v2: None
>>>
>>> drivers/usb/dwc2/core.h | 1 +
>>> drivers/usb/dwc2/hcd_queue.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>> 2 files changed, 40 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>> index 567ee2c9e69f..09aa2b5ae29e 100644
>>> --- a/drivers/usb/dwc2/core.h
>>> +++ b/drivers/usb/dwc2/core.h
>>> @@ -782,6 +782,7 @@ struct dwc2_hsotg {
>>> u16 periodic_usecs;
>>> unsigned long periodic_bitmap[DIV_ROUND_UP(TOTAL_PERIODIC_USEC,
>>> BITS_PER_LONG)];
>>> + bool has_split[8];
>>
>> why don't you use a u8 instead then just set each bit for each
>> "has_split" you need to take care of. This array is kinda ugly.
>
> Let's actually drop this patch completely. Julius and I sat down and
> he talked me through things, and with my current understanding the
> current microframe scheduler in dwc2 is broken enough that small
> little band-aids like this will do little more than just push the
> problems around.
>
> I'm a good portion of the way through a better microframe scheduler.
> I have no doubt that it won't be perfect, but hopefully it will at
> least be based in reality...
>
> My latest thinking on the patches in this series:
>
> 1. usb: dwc2: rockchip: Make the max_transfer_size automatic
>
> I'll probably separate this out into its own patch so I can stop
> sending it as part of this series. ...or if someone wanted to land it
> then I won't bother.
>
>
> 2. usb: dwc2: host: Get aligned DMA in a more supported way
>
> Still can land any time and has good benefits. I believe that I can't
> separate this because it will cause conflicts with scheduler patches.
>
>
> 3. usb: dwc2: host: Add scheduler tracing
>
> Would be nice to land.
>
>
> 4. usb: dwc2: host: Rewrite the microframe scheduler
> 5. usb: dwc2: host: Keep track of and use our scheduled microframe
> 6. usb: dwc2: host: Assume all devices are on one single_tt hub
>
> Please drop patches 4-6 right now.
>
>
> 7. usb: dwc2: host: Add a delay before releasing periodic bandwidth
> 8. usb: dwc2: host: Giveback URB in tasklet context

if you can, it's best to send a new series with the changes. This makes
mine and John's life a lot easier :-)

> I'll probably move these back up in the series (like in v2) and put
> microframe rewrite atop them. With my current understanding the
> scheduling is so broken today that the concerns Alan brought up can
> wait until we have a proper scheduler to be addressed. In the
> meantime getting the huge boost in interrupt speed will help with
> dwc2's correctness (and performance) because it means we're much less
> likely to miss SOF interrupts.
>
> If anyone has any review time, giving a review to v2 of these patches
> would be helpful. Otherwise I'll double check that v2 still looks
> good with my current understanding and eventually post them again.

That would have to be someone experienced with that IP. I don't even
have docs :-)

--
balbi

Attachments:

signature.asc (818.00 B)

2015-11-20 04:33:46

by John Youn

[permalink] [raw]

Subject: Re: [PATCH v3 6/8] usb: dwc2: host: Assume all devices are on one single_tt hub

On 11/19/2015 8:27 AM, Doug Anderson wrote:
> Felipe,
>
> On Thu, Nov 19, 2015 at 7:34 AM, Felipe Balbi <[email protected]> wrote:
>>
>> Hi,
>>
>> Douglas Anderson <[email protected]> writes:
>>> Until we have logic to determine which devices share the same TT let's
>>> add logic to assume that all devices on a given dwc2 controller are on
>>> one single_tt hub. This is better than the previous code that assumed
>>> that all devices were on one multi_tt hub, since single_tt hubs
>>> appear (in my experience) to be much more common and any schedule that
>>> would work on a single_tt hub will also work on a multi_tt hub. This
>>> will prevent more than 8 total low/full speed devices to be on the bus
>>> at one time, but that's a reasonable restriction until we've made things
>>> smarter.
>>>
>>> Signed-off-by: Douglas Anderson <[email protected]>
>>> ---
>>> Changes in v3:
>>> - Assuming single_tt is new for v3; not terribly well tested (yet).
>>>
>>> Changes in v2: None
>>>
>>> drivers/usb/dwc2/core.h | 1 +
>>> drivers/usb/dwc2/hcd_queue.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>> 2 files changed, 40 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>> index 567ee2c9e69f..09aa2b5ae29e 100644
>>> --- a/drivers/usb/dwc2/core.h
>>> +++ b/drivers/usb/dwc2/core.h
>>> @@ -782,6 +782,7 @@ struct dwc2_hsotg {
>>> u16 periodic_usecs;
>>> unsigned long periodic_bitmap[DIV_ROUND_UP(TOTAL_PERIODIC_USEC,
>>> BITS_PER_LONG)];
>>> + bool has_split[8];
>>
>> why don't you use a u8 instead then just set each bit for each
>> "has_split" you need to take care of. This array is kinda ugly.
>
> Let's actually drop this patch completely. Julius and I sat down and
> he talked me through things, and with my current understanding the
> current microframe scheduler in dwc2 is broken enough that small
> little band-aids like this will do little more than just push the
> problems around.
>
> I'm a good portion of the way through a better microframe scheduler.
> I have no doubt that it won't be perfect, but hopefully it will at
> least be based in reality...
>
> My latest thinking on the patches in this series:
>
> 1. usb: dwc2: rockchip: Make the max_transfer_size automatic
>
> I'll probably separate this out into its own patch so I can stop
> sending it as part of this series. ...or if someone wanted to land it
> then I won't bother.
>
>
> 2. usb: dwc2: host: Get aligned DMA in a more supported way
>
> Still can land any time and has good benefits. I believe that I can't
> separate this because it will cause conflicts with scheduler patches.
>
>
> 3. usb: dwc2: host: Add scheduler tracing
>
> Would be nice to land.
>
>
> 4. usb: dwc2: host: Rewrite the microframe scheduler
> 5. usb: dwc2: host: Keep track of and use our scheduled microframe
> 6. usb: dwc2: host: Assume all devices are on one single_tt hub
>
> Please drop patches 4-6 right now.
>
>
> 7. usb: dwc2: host: Add a delay before releasing periodic bandwidth
> 8. usb: dwc2: host: Giveback URB in tasklet context
>
> I'll probably move these back up in the series (like in v2) and put
> microframe rewrite atop them. With my current understanding the
> scheduling is so broken today that the concerns Alan brought up can
> wait until we have a proper scheduler to be addressed. In the
> meantime getting the huge boost in interrupt speed will help with
> dwc2's correctness (and performance) because it means we're much less
> likely to miss SOF interrupts.
>
> If anyone has any review time, giving a review to v2 of these patches
> would be helpful. Otherwise I'll double check that v2 still looks
> good with my current understanding and eventually post them again.
>
> -Doug
>

Hi Doug,

Patches 1-3:
Acked-by: John Youn <[email protected]>

Patch 2:
Tested-by: John Youn <[email protected]>

Tested on core version 3.20 using internal TE for un-aligned
buffers.

I haven't had time to look into the scheduling patches yet. But I
agree with you that there are fundamental problems. I'll await
your rewrite.

Regards,
John

2015-11-24 00:28:08

by Doug Anderson

[permalink] [raw]

Subject: Re: [PATCH v3 6/8] usb: dwc2: host: Assume all devices are on one single_tt hub

John,

On Thu, Nov 19, 2015 at 8:33 PM, John Youn <[email protected]> wrote:
> Patches 1-3:
> Acked-by: John Youn <[email protected]>
>
> Patch 2:
> Tested-by: John Youn <[email protected]>
>
> Tested on core version 3.20 using internal TE for un-aligned
> buffers.
>
> I haven't had time to look into the scheduling patches yet. But I
> agree with you that there are fundamental problems. I'll await
> your rewrite.

Thanks! I'm going to attempt to at least get things reposted with
your tags before Thanksgiving. I'm hopeful that I'll actually be able
to post a RFC patch for the microframe rewrite by then, too! :)

I have something that seems to be sane at
<https://chromium-review.googlesource.com/#/c/313808/>, but it
obviously still needs a lot of cleanup. I also need to pick all of
the things that have landed on Felipe's tree recently and rebase atop
those.

-Doug

2015-11-26 00:44:25

by Doug Anderson

[permalink] [raw]

Subject: Re: [PATCH v3 6/8] usb: dwc2: host: Assume all devices are on one single_tt hub

Hi,

On Mon, Nov 23, 2015 at 4:28 PM, Doug Anderson <[email protected]> wrote:
> John,
>
> On Thu, Nov 19, 2015 at 8:33 PM, John Youn <[email protected]> wrote:
>> Patches 1-3:
>> Acked-by: John Youn <[email protected]>
>>
>> Patch 2:
>> Tested-by: John Youn <[email protected]>
>>
>> Tested on core version 3.20 using internal TE for un-aligned
>> buffers.
>>
>> I haven't had time to look into the scheduling patches yet. But I
>> agree with you that there are fundamental problems. I'll await
>> your rewrite.
>
> Thanks! I'm going to attempt to at least get things reposted with
> your tags before Thanksgiving. I'm hopeful that I'll actually be able
> to post a RFC patch for the microframe rewrite by then, too! :)
>
> I have something that seems to be sane at
> <https://chromium-review.googlesource.com/#/c/313808/>, but it
> obviously still needs a lot of cleanup. I also need to pick all of
> the things that have landed on Felipe's tree recently and rebase atop
> those.

I didn't succeed at getting something postable. :( I could easily
repost some of the earlier patches (like fixing aligned memory, etc)
if there's a desire for those, but I figured I'd wait until I had
something postable for the microframe scheduler.

I've definitely made progress in the last few days and things are a
bit cleaner at <https://chromium-review.googlesource.com/#/c/313808/3>.
It's also working reasonably well, but I'd still like to understand
some of the behavior I'm seeing before I actually post it to the
mailing list.

I'm going to be away from my computer for about a week, so I'll plan
to pick this up again when I'm back...

-Doug