| Message ID | 1454034013-24657-19-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Doug, On 01/29/2016 10:20 AM, Douglas Anderson wrote: > In dwc2_hcd_qh_deactivate() we will put some things on the > periodic_sched_ready list. These things won't be taken off the ready > list until the next SOF, which might be a little late. Let's put them > on right away. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > Tested-by: Heiko Stuebner <heiko@sntech.de> > Tested-by: Stefan Wahren <stefan.wahren@i2se.com> > --- > Changes in v6: > - Add Heiko's Tested-by. > - Add Stefan's Tested-by. > > Changes in v5: None > Changes in v4: > - Schedule periodic right away if it's time new for v4. > > Changes in v3: None > Changes in v2: None > > drivers/usb/dwc2/hcd_queue.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c > index 9b3c435339ee..3abb34a5fc5b 100644 > --- a/drivers/usb/dwc2/hcd_queue.c > +++ b/drivers/usb/dwc2/hcd_queue.c > @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh, > * Note: we purposely use the frame_number from the "hsotg" structure > * since we know SOF interrupt will handle future frames. > */ > - if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number)) > + if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number)) { > + enum dwc2_transaction_type tr_type; > + > + /* > + * We're bypassing the SOF handler which is normally what puts > + * us on the ready list because we're in a hurry and need to > + * try to catch up. > + */ > + dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x, nxt=%04x\n", > + qh, frame_number, qh->next_active_frame); > list_move_tail(&qh->qh_list_entry, > &hsotg->periodic_sched_ready); > - else > + > + tr_type = dwc2_hcd_select_transactions(hsotg); Do we need to add select_transactions call here? If we get into this function in interrupt and once we put the qh in ready queue, the qh can be handled in this frame again by the later function call of dwc_hcd_select_transactions, so what we need to to here is put it in ready list instead of inactive queue, and wait for the schedule. Thanks, - Kever > + if (tr_type != DWC2_TRANSACTION_NONE) > + dwc2_hcd_queue_transactions(hsotg, tr_type); > + } else { > list_move_tail(&qh->qh_list_entry, > &hsotg->periodic_sched_inactive); > + } > } > > /**
Kever, On Sun, Jan 31, 2016 at 1:36 AM, Kever Yang <kever.yang@rock-chips.com> wrote: > Doug, > > > On 01/29/2016 10:20 AM, Douglas Anderson wrote: >> >> In dwc2_hcd_qh_deactivate() we will put some things on the >> periodic_sched_ready list. These things won't be taken off the ready >> list until the next SOF, which might be a little late. Let's put them >> on right away. >> >> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> Tested-by: Heiko Stuebner <heiko@sntech.de> >> Tested-by: Stefan Wahren <stefan.wahren@i2se.com> >> --- >> Changes in v6: >> - Add Heiko's Tested-by. >> - Add Stefan's Tested-by. >> >> Changes in v5: None >> Changes in v4: >> - Schedule periodic right away if it's time new for v4. >> >> Changes in v3: None >> Changes in v2: None >> >> drivers/usb/dwc2/hcd_queue.c | 18 ++++++++++++++++-- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c >> index 9b3c435339ee..3abb34a5fc5b 100644 >> --- a/drivers/usb/dwc2/hcd_queue.c >> +++ b/drivers/usb/dwc2/hcd_queue.c >> @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg >> *hsotg, struct dwc2_qh *qh, >> * Note: we purposely use the frame_number from the "hsotg" >> structure >> * since we know SOF interrupt will handle future frames. >> */ >> - if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number)) >> + if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number)) >> { >> + enum dwc2_transaction_type tr_type; >> + >> + /* >> + * We're bypassing the SOF handler which is normally what >> puts >> + * us on the ready list because we're in a hurry and need >> to >> + * try to catch up. >> + */ >> + dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x, >> nxt=%04x\n", >> + qh, frame_number, qh->next_active_frame); >> list_move_tail(&qh->qh_list_entry, >> &hsotg->periodic_sched_ready); >> - else >> + >> + tr_type = dwc2_hcd_select_transactions(hsotg); > > Do we need to add select_transactions call here? If we get into this > function in interrupt > and once we put the qh in ready queue, the qh can be handled in this frame > again by the > later function call of dwc_hcd_select_transactions, so what we need to to > here is put > it in ready list instead of inactive queue, and wait for the schedule. I'm not sure I understand. Can you restate? I'll try to explain more in the meantime... Both before and after my change, this function would place something on the ready queue if the next_active_frame <= the frame number as of last SOF interrupt (aka hsotg->frame_number). Otherwise it goes on the inactive queue. Assuming that the previous change ("usb: dwc2: host: Manage frame nums better in scheduler") worked properly then next_active_frame shouldn't be less than (hsotg->frame_number - 1). Remember that next_active_frame is always 1 before the wire frame, so if "next_active_frame == hsotg->frame_number - 1" it means that we need to get the transfer on the wire _right away_. If "next_active_frame == hsotg->frame_number" the transfer doesn't need to go on the wire right away, but since dwc2 can be prepped one frame in advance it doesn't hurt to give it to the hardware right away if there's space. As I understand it, if we stick something on the ready queue it won't generally get looked at until the next SOF interrupt. That means we'll be too late if "next_active_frame == hsotg->frame_number - 1" and we'll possibly be too late (depending on interrupt latency) if "next_active_frame == hsotg->frame_number" Note that before my series, there were more places than just the SOF interrupt that would update hsotg->frame_number (see "usb: dwc2: host: Manage frame nums better in scheduler" for fix). Also before my series (specially "usb: dwc2: host: Manage frame nums better in scheduler") we used the actual current frame number when doing comparisons. Also before my series (specifically "usb: dwc2: host: Properly set even/odd frame") we didn't really place things in the frame that they were scheduled in anyway. Also note that I believe that when dwc2_hcd_qh_deactivate() is called our spinlock is held which means that the SOF interrupt either ran before our function or won't run till after it. > >> + if (tr_type != DWC2_TRANSACTION_NONE) >> + dwc2_hcd_queue_transactions(hsotg, tr_type); >> + } else { >> list_move_tail(&qh->qh_list_entry, >> &hsotg->periodic_sched_inactive); >> + } >> } >> /** > > >
Doug, On 02/01/2016 06:09 AM, Doug Anderson wrote: > Kever, > > On Sun, Jan 31, 2016 at 1:36 AM, Kever Yang <kever.yang@rock-chips.com> wrote: >> Doug, >> >> >> On 01/29/2016 10:20 AM, Douglas Anderson wrote: >>> In dwc2_hcd_qh_deactivate() we will put some things on the >>> periodic_sched_ready list. These things won't be taken off the ready >>> list until the next SOF, which might be a little late. Let's put them >>> on right away. >>> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>> Tested-by: Heiko Stuebner <heiko@sntech.de> >>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com> >>> --- >>> Changes in v6: >>> - Add Heiko's Tested-by. >>> - Add Stefan's Tested-by. >>> >>> Changes in v5: None >>> Changes in v4: >>> - Schedule periodic right away if it's time new for v4. >>> >>> Changes in v3: None >>> Changes in v2: None >>> >>> drivers/usb/dwc2/hcd_queue.c | 18 ++++++++++++++++-- >>> 1 file changed, 16 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c >>> index 9b3c435339ee..3abb34a5fc5b 100644 >>> --- a/drivers/usb/dwc2/hcd_queue.c >>> +++ b/drivers/usb/dwc2/hcd_queue.c >>> @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg >>> *hsotg, struct dwc2_qh *qh, >>> * Note: we purposely use the frame_number from the "hsotg" >>> structure >>> * since we know SOF interrupt will handle future frames. >>> */ >>> - if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number)) >>> + if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number)) >>> { >>> + enum dwc2_transaction_type tr_type; >>> + >>> + /* >>> + * We're bypassing the SOF handler which is normally what >>> puts >>> + * us on the ready list because we're in a hurry and need >>> to >>> + * try to catch up. >>> + */ >>> + dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x, >>> nxt=%04x\n", >>> + qh, frame_number, qh->next_active_frame); >>> list_move_tail(&qh->qh_list_entry, >>> &hsotg->periodic_sched_ready); >>> - else >>> + >>> + tr_type = dwc2_hcd_select_transactions(hsotg); >> Do we need to add select_transactions call here? If we get into this >> function in interrupt >> and once we put the qh in ready queue, the qh can be handled in this frame >> again by the >> later function call of dwc_hcd_select_transactions, so what we need to to >> here is put >> it in ready list instead of inactive queue, and wait for the schedule. > I'm not sure I understand. Can you restate? > > > I'll try to explain more in the meantime... > > Both before and after my change, this function would place something > on the ready queue if the next_active_frame <= the frame number as of > last SOF interrupt (aka hsotg->frame_number). Otherwise it goes on > the inactive queue. Assuming that the previous change ("usb: dwc2: > host: Manage frame nums better in scheduler") worked properly then > next_active_frame shouldn't be less than (hsotg->frame_number - 1). > Remember that next_active_frame is always 1 before the wire frame, so > if "next_active_frame == hsotg->frame_number - 1" it means that we > need to get the transfer on the wire _right away_. If > "next_active_frame == hsotg->frame_number" the transfer doesn't need > to go on the wire right away, but since dwc2 can be prepped one frame > in advance it doesn't hurt to give it to the hardware right away if > there's space. > > As I understand it, if we stick something on the ready queue it won't > generally get looked at until the next SOF interrupt. That means > we'll be too late if "next_active_frame == hsotg->frame_number - 1" > and we'll possibly be too late (depending on interrupt latency) if > "next_active_frame == hsotg->frame_number" > I understand this patch and agree with your point of schedule the periodic right away instead of at least next frame. My point is, there are only two call to dwc2_hcd_qh_deactivate(), from dwc2_hcd_urb_dequeue() and dwc2_release_channel(), we don't need to do the schedule for dequeue, and there is one dwc2_hcd_select_transactions() call at the end of dwc2_release_channel(), maybe we don't need another dwc2_hcd_select_transactions() here. I think the duration from this point to the function call of dwc2_hcd_select_transactions() in dwc2_release_channel() will be the main factor for us to decide if we need to add a function call of dwc2_hcd_select_transactions() here. Thanks, - Kever > Note that before my series, there were more places than just the SOF > interrupt that would update hsotg->frame_number (see "usb: dwc2: host: > Manage frame nums better in scheduler" for fix). Also before my > series (specially "usb: dwc2: host: Manage frame nums better in > scheduler") we used the actual current frame number when doing > comparisons. Also before my series (specifically "usb: dwc2: host: > Properly set even/odd frame") we didn't really place things in the > frame that they were scheduled in anyway. > > > Also note that I believe that when dwc2_hcd_qh_deactivate() is called > our spinlock is held which means that the SOF interrupt either ran > before our function or won't run till after it. > >>> + if (tr_type != DWC2_TRANSACTION_NONE) >>> + dwc2_hcd_queue_transactions(hsotg, tr_type); >>> + } else { >>> list_move_tail(&qh->qh_list_entry, >>> &hsotg->periodic_sched_inactive); >>> + } >>> } >>> /** >> >>
Kever, On Sun, Jan 31, 2016 at 7:32 PM, Kever Yang <kever.yang@rock-chips.com> wrote: > Doug, > > > On 02/01/2016 06:09 AM, Doug Anderson wrote: >> >> Kever, >> >> On Sun, Jan 31, 2016 at 1:36 AM, Kever Yang <kever.yang@rock-chips.com> >> wrote: >>> >>> Doug, >>> >>> >>> On 01/29/2016 10:20 AM, Douglas Anderson wrote: >>>> >>>> In dwc2_hcd_qh_deactivate() we will put some things on the >>>> periodic_sched_ready list. These things won't be taken off the ready >>>> list until the next SOF, which might be a little late. Let's put them >>>> on right away. >>>> >>>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>>> Tested-by: Heiko Stuebner <heiko@sntech.de> >>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com> >>>> --- >>>> Changes in v6: >>>> - Add Heiko's Tested-by. >>>> - Add Stefan's Tested-by. >>>> >>>> Changes in v5: None >>>> Changes in v4: >>>> - Schedule periodic right away if it's time new for v4. >>>> >>>> Changes in v3: None >>>> Changes in v2: None >>>> >>>> drivers/usb/dwc2/hcd_queue.c | 18 ++++++++++++++++-- >>>> 1 file changed, 16 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c >>>> index 9b3c435339ee..3abb34a5fc5b 100644 >>>> --- a/drivers/usb/dwc2/hcd_queue.c >>>> +++ b/drivers/usb/dwc2/hcd_queue.c >>>> @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg >>>> *hsotg, struct dwc2_qh *qh, >>>> * Note: we purposely use the frame_number from the "hsotg" >>>> structure >>>> * since we know SOF interrupt will handle future frames. >>>> */ >>>> - if (dwc2_frame_num_le(qh->next_active_frame, >>>> hsotg->frame_number)) >>>> + if (dwc2_frame_num_le(qh->next_active_frame, >>>> hsotg->frame_number)) >>>> { >>>> + enum dwc2_transaction_type tr_type; >>>> + >>>> + /* >>>> + * We're bypassing the SOF handler which is normally >>>> what >>>> puts >>>> + * us on the ready list because we're in a hurry and >>>> need >>>> to >>>> + * try to catch up. >>>> + */ >>>> + dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x, >>>> nxt=%04x\n", >>>> + qh, frame_number, qh->next_active_frame); >>>> list_move_tail(&qh->qh_list_entry, >>>> &hsotg->periodic_sched_ready); >>>> - else >>>> + >>>> + tr_type = dwc2_hcd_select_transactions(hsotg); >>> >>> Do we need to add select_transactions call here? If we get into this >>> function in interrupt >>> and once we put the qh in ready queue, the qh can be handled in this >>> frame >>> again by the >>> later function call of dwc_hcd_select_transactions, so what we need to to >>> here is put >>> it in ready list instead of inactive queue, and wait for the schedule. >> >> I'm not sure I understand. Can you restate? >> >> >> I'll try to explain more in the meantime... >> >> Both before and after my change, this function would place something >> on the ready queue if the next_active_frame <= the frame number as of >> last SOF interrupt (aka hsotg->frame_number). Otherwise it goes on >> the inactive queue. Assuming that the previous change ("usb: dwc2: >> host: Manage frame nums better in scheduler") worked properly then >> next_active_frame shouldn't be less than (hsotg->frame_number - 1). >> Remember that next_active_frame is always 1 before the wire frame, so >> if "next_active_frame == hsotg->frame_number - 1" it means that we >> need to get the transfer on the wire _right away_. If >> "next_active_frame == hsotg->frame_number" the transfer doesn't need >> to go on the wire right away, but since dwc2 can be prepped one frame >> in advance it doesn't hurt to give it to the hardware right away if >> there's space. >> >> As I understand it, if we stick something on the ready queue it won't >> generally get looked at until the next SOF interrupt. That means >> we'll be too late if "next_active_frame == hsotg->frame_number - 1" >> and we'll possibly be too late (depending on interrupt latency) if >> "next_active_frame == hsotg->frame_number" >> > I understand this patch and agree with your point of schedule the > periodic right away instead of at least next frame. > My point is, there are only two call to dwc2_hcd_qh_deactivate(), from > dwc2_hcd_urb_dequeue() and dwc2_release_channel(), we don't need > to do the schedule for dequeue, and there is one > dwc2_hcd_select_transactions() call at the end of dwc2_release_channel(), > maybe we don't need another dwc2_hcd_select_transactions() here. > > I think the duration from this point to the function call of > dwc2_hcd_select_transactions() > in dwc2_release_channel() will be the main factor for us to decide if > we need to add a function call of dwc2_hcd_select_transactions() here. Oh, now I get what you're saying! A) You've got dwc2_release_channel() -> dwc2_deactivate_qh() -> dwc2_hcd_qh_deactivate() ...and always in that case we'll do a select / queue, so we don't need it there. B) You've got dwc2_hcd_urb_dequeue() -> dwc2_hcd_qh_deactivate() ...but why don't we need it for dwc2_hcd_urb_dequeue()? Yes, you're not continuing a split so timing isn't quite as urgent, but you still might have an INT or ISOC packet that's scheduled with an interval of 1. We still might want to schedule right away if there are remaining QTDs, right? -Doug
Kever, On Sun, Jan 31, 2016 at 8:36 PM, Doug Anderson <dianders@chromium.org> wrote: > Kever, > > On Sun, Jan 31, 2016 at 7:32 PM, Kever Yang <kever.yang@rock-chips.com> wrote: >> Doug, >> >> >> On 02/01/2016 06:09 AM, Doug Anderson wrote: >>> >>> Kever, >>> >>> On Sun, Jan 31, 2016 at 1:36 AM, Kever Yang <kever.yang@rock-chips.com> >>> wrote: >>>> >>>> Doug, >>>> >>>> >>>> On 01/29/2016 10:20 AM, Douglas Anderson wrote: >>>>> >>>>> In dwc2_hcd_qh_deactivate() we will put some things on the >>>>> periodic_sched_ready list. These things won't be taken off the ready >>>>> list until the next SOF, which might be a little late. Let's put them >>>>> on right away. >>>>> >>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>>>> Tested-by: Heiko Stuebner <heiko@sntech.de> >>>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com> >>>>> --- >>>>> Changes in v6: >>>>> - Add Heiko's Tested-by. >>>>> - Add Stefan's Tested-by. >>>>> >>>>> Changes in v5: None >>>>> Changes in v4: >>>>> - Schedule periodic right away if it's time new for v4. >>>>> >>>>> Changes in v3: None >>>>> Changes in v2: None >>>>> >>>>> drivers/usb/dwc2/hcd_queue.c | 18 ++++++++++++++++-- >>>>> 1 file changed, 16 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c >>>>> index 9b3c435339ee..3abb34a5fc5b 100644 >>>>> --- a/drivers/usb/dwc2/hcd_queue.c >>>>> +++ b/drivers/usb/dwc2/hcd_queue.c >>>>> @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg >>>>> *hsotg, struct dwc2_qh *qh, >>>>> * Note: we purposely use the frame_number from the "hsotg" >>>>> structure >>>>> * since we know SOF interrupt will handle future frames. >>>>> */ >>>>> - if (dwc2_frame_num_le(qh->next_active_frame, >>>>> hsotg->frame_number)) >>>>> + if (dwc2_frame_num_le(qh->next_active_frame, >>>>> hsotg->frame_number)) >>>>> { >>>>> + enum dwc2_transaction_type tr_type; >>>>> + >>>>> + /* >>>>> + * We're bypassing the SOF handler which is normally >>>>> what >>>>> puts >>>>> + * us on the ready list because we're in a hurry and >>>>> need >>>>> to >>>>> + * try to catch up. >>>>> + */ >>>>> + dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x, >>>>> nxt=%04x\n", >>>>> + qh, frame_number, qh->next_active_frame); >>>>> list_move_tail(&qh->qh_list_entry, >>>>> &hsotg->periodic_sched_ready); >>>>> - else >>>>> + >>>>> + tr_type = dwc2_hcd_select_transactions(hsotg); >>>> >>>> Do we need to add select_transactions call here? If we get into this >>>> function in interrupt >>>> and once we put the qh in ready queue, the qh can be handled in this >>>> frame >>>> again by the >>>> later function call of dwc_hcd_select_transactions, so what we need to to >>>> here is put >>>> it in ready list instead of inactive queue, and wait for the schedule. >>> >>> I'm not sure I understand. Can you restate? >>> >>> >>> I'll try to explain more in the meantime... >>> >>> Both before and after my change, this function would place something >>> on the ready queue if the next_active_frame <= the frame number as of >>> last SOF interrupt (aka hsotg->frame_number). Otherwise it goes on >>> the inactive queue. Assuming that the previous change ("usb: dwc2: >>> host: Manage frame nums better in scheduler") worked properly then >>> next_active_frame shouldn't be less than (hsotg->frame_number - 1). >>> Remember that next_active_frame is always 1 before the wire frame, so >>> if "next_active_frame == hsotg->frame_number - 1" it means that we >>> need to get the transfer on the wire _right away_. If >>> "next_active_frame == hsotg->frame_number" the transfer doesn't need >>> to go on the wire right away, but since dwc2 can be prepped one frame >>> in advance it doesn't hurt to give it to the hardware right away if >>> there's space. >>> >>> As I understand it, if we stick something on the ready queue it won't >>> generally get looked at until the next SOF interrupt. That means >>> we'll be too late if "next_active_frame == hsotg->frame_number - 1" >>> and we'll possibly be too late (depending on interrupt latency) if >>> "next_active_frame == hsotg->frame_number" >>> >> I understand this patch and agree with your point of schedule the >> periodic right away instead of at least next frame. >> My point is, there are only two call to dwc2_hcd_qh_deactivate(), from >> dwc2_hcd_urb_dequeue() and dwc2_release_channel(), we don't need >> to do the schedule for dequeue, and there is one >> dwc2_hcd_select_transactions() call at the end of dwc2_release_channel(), >> maybe we don't need another dwc2_hcd_select_transactions() here. >> >> I think the duration from this point to the function call of >> dwc2_hcd_select_transactions() >> in dwc2_release_channel() will be the main factor for us to decide if >> we need to add a function call of dwc2_hcd_select_transactions() here. > > Oh, now I get what you're saying! > > A) You've got dwc2_release_channel() -> dwc2_deactivate_qh() -> > dwc2_hcd_qh_deactivate() > ...and always in that case we'll do a select / queue, so we don't need it there. > > B) You've got dwc2_hcd_urb_dequeue() -> dwc2_hcd_qh_deactivate() > > ...but why don't we need it for dwc2_hcd_urb_dequeue()? Yes, you're > not continuing a split so timing isn't quite as urgent, but you still > might have an INT or ISOC packet that's scheduled with an interval of > 1. We still might want to schedule right away if there are remaining > QTDs, right? I ran out of time to fully test today, but I couldn't actually get a case where we needed to schedule right away for B). ...so given your point about the the select / queue already present in case A, we could probably just drop this patch ("usb: dwc2: host: Schedule periodic right away if it's time") and if we can find a case where it's needed in case B we can add the select / queue there. Sound OK? I'll try to do more testing tomorrow... -Doug
Doug, On 02/02/2016 08:36 AM, Doug Anderson wrote: > Kever, > > On Sun, Jan 31, 2016 at 8:36 PM, Doug Anderson <dianders@chromium.org> wrote: >> Kever, >> >> On Sun, Jan 31, 2016 at 7:32 PM, Kever Yang <kever.yang@rock-chips.com> wrote: >>> Doug, >>> >>> >>> On 02/01/2016 06:09 AM, Doug Anderson wrote: >>>> Kever, >>>> >>>> On Sun, Jan 31, 2016 at 1:36 AM, Kever Yang <kever.yang@rock-chips.com> >>>> wrote: >>>>> Doug, >>>>> >>>>> >>>>> On 01/29/2016 10:20 AM, Douglas Anderson wrote: >>>>>> In dwc2_hcd_qh_deactivate() we will put some things on the >>>>>> periodic_sched_ready list. These things won't be taken off the ready >>>>>> list until the next SOF, which might be a little late. Let's put them >>>>>> on right away. >>>>>> >>>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>>>>> Tested-by: Heiko Stuebner <heiko@sntech.de> >>>>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com> >>>>>> --- >>>>>> Changes in v6: >>>>>> - Add Heiko's Tested-by. >>>>>> - Add Stefan's Tested-by. >>>>>> >>>>>> Changes in v5: None >>>>>> Changes in v4: >>>>>> - Schedule periodic right away if it's time new for v4. >>>>>> >>>>>> Changes in v3: None >>>>>> Changes in v2: None >>>>>> >>>>>> drivers/usb/dwc2/hcd_queue.c | 18 ++++++++++++++++-- >>>>>> 1 file changed, 16 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c >>>>>> index 9b3c435339ee..3abb34a5fc5b 100644 >>>>>> --- a/drivers/usb/dwc2/hcd_queue.c >>>>>> +++ b/drivers/usb/dwc2/hcd_queue.c >>>>>> @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg >>>>>> *hsotg, struct dwc2_qh *qh, >>>>>> * Note: we purposely use the frame_number from the "hsotg" >>>>>> structure >>>>>> * since we know SOF interrupt will handle future frames. >>>>>> */ >>>>>> - if (dwc2_frame_num_le(qh->next_active_frame, >>>>>> hsotg->frame_number)) >>>>>> + if (dwc2_frame_num_le(qh->next_active_frame, >>>>>> hsotg->frame_number)) >>>>>> { >>>>>> + enum dwc2_transaction_type tr_type; >>>>>> + >>>>>> + /* >>>>>> + * We're bypassing the SOF handler which is normally >>>>>> what >>>>>> puts >>>>>> + * us on the ready list because we're in a hurry and >>>>>> need >>>>>> to >>>>>> + * try to catch up. >>>>>> + */ >>>>>> + dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x, >>>>>> nxt=%04x\n", >>>>>> + qh, frame_number, qh->next_active_frame); >>>>>> list_move_tail(&qh->qh_list_entry, >>>>>> &hsotg->periodic_sched_ready); >>>>>> - else >>>>>> + >>>>>> + tr_type = dwc2_hcd_select_transactions(hsotg); >>>>> Do we need to add select_transactions call here? If we get into this >>>>> function in interrupt >>>>> and once we put the qh in ready queue, the qh can be handled in this >>>>> frame >>>>> again by the >>>>> later function call of dwc_hcd_select_transactions, so what we need to to >>>>> here is put >>>>> it in ready list instead of inactive queue, and wait for the schedule. >>>> I'm not sure I understand. Can you restate? >>>> >>>> >>>> I'll try to explain more in the meantime... >>>> >>>> Both before and after my change, this function would place something >>>> on the ready queue if the next_active_frame <= the frame number as of >>>> last SOF interrupt (aka hsotg->frame_number). Otherwise it goes on >>>> the inactive queue. Assuming that the previous change ("usb: dwc2: >>>> host: Manage frame nums better in scheduler") worked properly then >>>> next_active_frame shouldn't be less than (hsotg->frame_number - 1). >>>> Remember that next_active_frame is always 1 before the wire frame, so >>>> if "next_active_frame == hsotg->frame_number - 1" it means that we >>>> need to get the transfer on the wire _right away_. If >>>> "next_active_frame == hsotg->frame_number" the transfer doesn't need >>>> to go on the wire right away, but since dwc2 can be prepped one frame >>>> in advance it doesn't hurt to give it to the hardware right away if >>>> there's space. >>>> >>>> As I understand it, if we stick something on the ready queue it won't >>>> generally get looked at until the next SOF interrupt. That means >>>> we'll be too late if "next_active_frame == hsotg->frame_number - 1" >>>> and we'll possibly be too late (depending on interrupt latency) if >>>> "next_active_frame == hsotg->frame_number" >>>> >>> I understand this patch and agree with your point of schedule the >>> periodic right away instead of at least next frame. >>> My point is, there are only two call to dwc2_hcd_qh_deactivate(), from >>> dwc2_hcd_urb_dequeue() and dwc2_release_channel(), we don't need >>> to do the schedule for dequeue, and there is one >>> dwc2_hcd_select_transactions() call at the end of dwc2_release_channel(), >>> maybe we don't need another dwc2_hcd_select_transactions() here. >>> >>> I think the duration from this point to the function call of >>> dwc2_hcd_select_transactions() >>> in dwc2_release_channel() will be the main factor for us to decide if >>> we need to add a function call of dwc2_hcd_select_transactions() here. >> Oh, now I get what you're saying! >> >> A) You've got dwc2_release_channel() -> dwc2_deactivate_qh() -> >> dwc2_hcd_qh_deactivate() >> ...and always in that case we'll do a select / queue, so we don't need it there. >> >> B) You've got dwc2_hcd_urb_dequeue() -> dwc2_hcd_qh_deactivate() >> >> ...but why don't we need it for dwc2_hcd_urb_dequeue()? Yes, you're >> not continuing a split so timing isn't quite as urgent, but you still >> might have an INT or ISOC packet that's scheduled with an interval of >> 1. We still might want to schedule right away if there are remaining >> QTDs, right? > I ran out of time to fully test today, but I couldn't actually get a > case where we needed to schedule right away for B). ...so given your > point about the the select / queue already present in case A, we could > probably just drop this patch ("usb: dwc2: host: Schedule periodic > right away if it's time") and if we can find a case where it's needed > in case B we can add the select / queue there. > > Sound OK? I'll try to do more testing tomorrow... Yes, we don't get a case we need to schedule right away for case B). For INT or ISOC packet, I can recall I have seen somewhere but I can find it now, the synchronous transfer is happen in the next uframe instead of the uframe when the host channel initialized, so there is no difference of setting the host channel register sooner or later inside the same frame. Which means the existent code should be OK for case A). We can drop this patch before we have the exact use case. Thanks, - Kever > > -Doug >
Kever, On Mon, Feb 1, 2016 at 11:04 PM, Kever Yang <kever.yang@rock-chips.com> wrote: >>> Oh, now I get what you're saying! >>> >>> A) You've got dwc2_release_channel() -> dwc2_deactivate_qh() -> >>> dwc2_hcd_qh_deactivate() >>> ...and always in that case we'll do a select / queue, so we don't need it >>> there. >>> >>> B) You've got dwc2_hcd_urb_dequeue() -> dwc2_hcd_qh_deactivate() >>> >>> ...but why don't we need it for dwc2_hcd_urb_dequeue()? Yes, you're >>> not continuing a split so timing isn't quite as urgent, but you still >>> might have an INT or ISOC packet that's scheduled with an interval of >>> 1. We still might want to schedule right away if there are remaining >>> QTDs, right? >> >> I ran out of time to fully test today, but I couldn't actually get a >> case where we needed to schedule right away for B). ...so given your >> point about the the select / queue already present in case A, we could >> probably just drop this patch ("usb: dwc2: host: Schedule periodic >> right away if it's time") and if we can find a case where it's needed >> in case B we can add the select / queue there. >> >> Sound OK? I'll try to do more testing tomorrow... > > Yes, we don't get a case we need to schedule right away for case B). > > For INT or ISOC packet, I can recall I have seen somewhere but I can find > it now, the synchronous transfer is happen in the next uframe instead of the > uframe > when the host channel initialized, so there is no difference of setting the > host channel register sooner or later inside the same frame. > Which means the existent code should be OK for case A). > > We can drop this patch before we have the exact use case. I put in some printouts and I finally did manage to find a place where we needed to queue things up in dwc2_hcd_urb_dequeue(). I saw: 314.587916: QH=d9535340 next(0) fn=2a52, sch=2a51=>2a52 (+1) miss=0 314.588040: QH=d9535340 next(0) fn=2a53, sch=2a52=>2a53 (+1) miss=0 314.588162: QH=d9535340 next(0) fn=2a54, sch=2a53=>2a54 (+1) miss=0 314.588299: QH=d9535340 next(0) fn=2a55, sch=2a54=>2a55 (+1) miss=0 314.588304: QH=d9535340 queue in dwc2_hcd_urb_dequeue 314.588363: QH=d9535340 next(0) fn=2a55, sch=2a55=>2a56 (+1) miss=0 314.588413: dwc2_handle_hcd_intr: ff540000.usb: SCH: QH=e5cea380 ready fn=2a56, nxt=2a56 314.588414: dwc2_handle_hcd_intr: ff540000.usb: SCH: QH=e73ccc40 ready fn=2a56, nxt=2a56 314.588415: dwc2_handle_hcd_intr: ff540000.usb: SCH: QH=e5cea8c0 ready fn=2a56, nxt=2a56 It's not something that's terribly common. It's fine to just drop this patch, or I can replace it with <https://chromium-review.googlesource.com/325540>. -Doug
diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c index 9b3c435339ee..3abb34a5fc5b 100644 --- a/drivers/usb/dwc2/hcd_queue.c +++ b/drivers/usb/dwc2/hcd_queue.c @@ -1080,12 +1080,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh, * Note: we purposely use the frame_number from the "hsotg" structure * since we know SOF interrupt will handle future frames. */ - if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number)) + if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number)) { + enum dwc2_transaction_type tr_type; + + /* + * We're bypassing the SOF handler which is normally what puts + * us on the ready list because we're in a hurry and need to + * try to catch up. + */ + dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x, nxt=%04x\n", + qh, frame_number, qh->next_active_frame); list_move_tail(&qh->qh_list_entry, &hsotg->periodic_sched_ready); - else + + tr_type = dwc2_hcd_select_transactions(hsotg); + if (tr_type != DWC2_TRANSACTION_NONE) + dwc2_hcd_queue_transactions(hsotg, tr_type); + } else { list_move_tail(&qh->qh_list_entry, &hsotg->periodic_sched_inactive); + } } /**