From: "Matwey V. Kornilov" <matwey@sai.msu.ru> To: "Bin Liu" <b-liu@ti.com>, "Matwey V. Kornilov" <matwey@sai.msu.ru>, "Greg KH" <gregkh@linuxfoundation.org>, "Матвей Корнилов" <matwey.kornilov@gmail.com>, "open list" <linux-kernel@vger.kernel.org>, "open list:MUSB MULTIPOINT HIGH SPEED DUAL-ROLE CONTROLLER" <linux-usb@vger.kernel.org> Subject: [6/6] usb: musb: Decrease URB starting latency in musb_advance_schedule() Date: Sat, 4 May 2019 12:38:22 +0300 [thread overview] Message-ID: <CAJs94EZLDotLHQmfhvzyRZWDAEL6hnUTmkXKMoVrO_JBJcHX4A@mail.gmail.com> (raw) вт, 30 апр. 2019 г. в 18:31, Bin Liu <b-liu@ti.com>: > > Hi Greg and all devs, > > On Wed, Apr 03, 2019 at 09:53:10PM +0300, Matwey V. Kornilov wrote: > > Previously, the algorithm was the following: > > > > 1. giveback current URB > > 2. if current qh is not empty > > then start next URB > > 3. if current qh is empty > > then dispose the qh, find next qh if any, and start URB. > > > > It may take a while to run urb->callback inside URB giveback which is > > run synchronously in musb. In order to improve the latency we rearrange > > the function behaviour for the case when qh is not empty: next URB is > > started before URB giveback. When qh is empty then the behaviour is > > intentionally kept in order not to break existing inter qh scheduling: > > URB giveback could potentionally enqueue other URB to the empty qh > > preventing it from being disposed. > > This patch changes the sequence of urb giveback in musb. > > before after > ------ ----- > 1. giveback current urb 1. start next urb if qh != empty > 2. start next urb if qh != empty 2. giveback current urb > > I see there is a potential that the urb giveback could be out of order, > for example, if urb giveback in BH and the next urb finishes before BH > runs. Could you please give more details? Frankly speaking, I am not sure that I understand the reordering issue origin correctly. I see in the existing implementation that the function call order is the following: 1. glue interrupt handler (for instance dsps_interrupt() in my am335x case) holds musb->lock; 2. musb_interrupt() 3. musb_host_rx() (or *_tx()) 4. musb_advance_schedule() 5. musb_giveback() releases and reacquires musb->lock around: 6. usb_hcd_giveback_urb() So, when musb_giveback() is called inside musb_advance_schedule() then the second instance of musb_advance_schedule() can be started simultaneously when the following interrupt is being handled at other CPU core. And we can see two usb_hcd_giveback_urb() running concurrently. Is it correct? > > If this potential is possible, is it a problem for any class driver? > > Thanks, > -Bin. > > > > > Before this patch, time spent in urb->callback led to the following > > glitches between the host and a hub during isoc transfer (line 4): > > > > 11.624492 d= 0.000124 [130.6 + 1.050] [ 4] SPLIT > > 11.624492 d= 0.000000 [130.6 + 1.467] [ 3] IN : 3.5 > > 11.624493 d= 0.000000 [130.6 + 1.967] [ 37] DATA0: aa 08 [skipped...] > > 11.625617 d= 0.001124 [131.7 + 1.050] [ 4] SPLIT > > 11.625617 d= 0.000000 [131.7 + 1.467] [ 3] IN : 3.5 > > 11.625867 d= 0.000250 [132.1 + 1.050] [ 4] SPLIT > > 11.625867 d= 0.000000 [132.1 + 1.467] [ 3] IN : 3.5 > > 11.625868 d= 0.000001 [132.1 + 1.983] [ 3] DATA0: 00 00 > > 11.626617 d= 0.000749 [132.7 + 1.050] [ 4] SPLIT > > 11.626617 d= 0.000000 [132.7 + 1.467] [ 3] IN : 3.5 > > 11.626867 d= 0.000250 [133.1 + 1.050] [ 4] SPLIT > > 11.626867 d= 0.000000 [133.1 + 1.467] [ 3] IN : 3.5 > > 11.626868 d= 0.000000 [133.1 + 1.967] [ 3] DATA0: 00 00 > > > > After the hub, they look as the following and may lead to broken > > perepherial transfer (as in case of PWC based webcam): > > > > 11.332004 d= 0.000997 [ 30.0 + 3.417] [ 3] IN : 5.5 > > 11.332007 d= 0.000003 [ 30.0 + 6.833] [800] DATA0: 8a 1c [skipped...] > > 11.334004 d= 0.001997 [ 32.0 + 3.417] [ 3] IN : 5.5 > > 11.334007 d= 0.000003 [ 32.0 + 6.750] [ 3] DATA0: 00 00 > > 11.335004 d= 0.000997 [ 33 + 3.417] [ 3] IN : 5.5 > > 11.335007 d= 0.000003 [ 33 + 6.750] [ 3] DATA0: 00 00 > > > > Removing this glitches makes us able to successfully run 10fps > > video stream from the webcam attached via USB hub. That was > > previously impossible. > > > > Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru> > > --- > > drivers/usb/musb/musb_host.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c > > index ed99ecd4e63a..75be92873b5b 100644 > > --- a/drivers/usb/musb/musb_host.c > > +++ b/drivers/usb/musb/musb_host.c > > @@ -85,6 +85,11 @@ static bool musb_qh_empty(struct musb_qh *qh) > > return list_empty(&qh->hep->urb_list); > > } > > > > +static bool musb_qh_singular(struct musb_qh *qh) > > +{ > > + return list_is_singular(&qh->hep->urb_list); > > +} > > + > > static void musb_qh_unlink_hep(struct musb_qh *qh) > > { > > if (!qh->hep) > > @@ -362,6 +367,19 @@ static void musb_advance_schedule(struct musb *musb, struct urb *urb, > > break; > > } > > > > + if (ready && !musb_qh_singular(qh)) { > > + struct urb *next_urb = list_next_entry(urb, urb_list); > > + > > + musb_dbg(musb, "... next ep%d %cX urb %p", hw_ep->epnum, is_in ? 'R' : 'T', next_urb); > > + musb_start_urb(musb, is_in, qh, next_urb); > > + > > + qh->is_ready = 0; > > + musb_giveback(musb, urb, status); > > + qh->is_ready = ready; > > + > > + return; > > + } > > + > > qh->is_ready = 0; > > musb_giveback(musb, urb, status); > > qh->is_ready = ready; > > -- > > 2.16.4 > >
WARNING: multiple messages have this Message-ID (diff)
From: "Matwey V. Kornilov" <matwey@sai.msu.ru> To: "Bin Liu" <b-liu@ti.com>, "Matwey V. Kornilov" <matwey@sai.msu.ru>, "Greg KH" <gregkh@linuxfoundation.org>, "Матвей Корнилов" <matwey.kornilov@gmail.com>, "open list" <linux-kernel@vger.kernel.org>, "open list:MUSB MULTIPOINT HIGH SPEED DUAL-ROLE CONTROLLER" <linux-usb@vger.kernel.org> Subject: Re: [PATCH 6/6] usb: musb: Decrease URB starting latency in musb_advance_schedule() Date: Sat, 4 May 2019 12:38:22 +0300 [thread overview] Message-ID: <CAJs94EZLDotLHQmfhvzyRZWDAEL6hnUTmkXKMoVrO_JBJcHX4A@mail.gmail.com> (raw) Message-ID: <20190504093822.VWkgm5pT5JOT2N_PgI-YYhSL9wLrxgPyVNLSPDsgnr0@z> (raw) In-Reply-To: <20190430153118.GI20993@uda0271908> вт, 30 апр. 2019 г. в 18:31, Bin Liu <b-liu@ti.com>: > > Hi Greg and all devs, > > On Wed, Apr 03, 2019 at 09:53:10PM +0300, Matwey V. Kornilov wrote: > > Previously, the algorithm was the following: > > > > 1. giveback current URB > > 2. if current qh is not empty > > then start next URB > > 3. if current qh is empty > > then dispose the qh, find next qh if any, and start URB. > > > > It may take a while to run urb->callback inside URB giveback which is > > run synchronously in musb. In order to improve the latency we rearrange > > the function behaviour for the case when qh is not empty: next URB is > > started before URB giveback. When qh is empty then the behaviour is > > intentionally kept in order not to break existing inter qh scheduling: > > URB giveback could potentionally enqueue other URB to the empty qh > > preventing it from being disposed. > > This patch changes the sequence of urb giveback in musb. > > before after > ------ ----- > 1. giveback current urb 1. start next urb if qh != empty > 2. start next urb if qh != empty 2. giveback current urb > > I see there is a potential that the urb giveback could be out of order, > for example, if urb giveback in BH and the next urb finishes before BH > runs. Could you please give more details? Frankly speaking, I am not sure that I understand the reordering issue origin correctly. I see in the existing implementation that the function call order is the following: 1. glue interrupt handler (for instance dsps_interrupt() in my am335x case) holds musb->lock; 2. musb_interrupt() 3. musb_host_rx() (or *_tx()) 4. musb_advance_schedule() 5. musb_giveback() releases and reacquires musb->lock around: 6. usb_hcd_giveback_urb() So, when musb_giveback() is called inside musb_advance_schedule() then the second instance of musb_advance_schedule() can be started simultaneously when the following interrupt is being handled at other CPU core. And we can see two usb_hcd_giveback_urb() running concurrently. Is it correct? > > If this potential is possible, is it a problem for any class driver? > > Thanks, > -Bin. > > > > > Before this patch, time spent in urb->callback led to the following > > glitches between the host and a hub during isoc transfer (line 4): > > > > 11.624492 d= 0.000124 [130.6 + 1.050] [ 4] SPLIT > > 11.624492 d= 0.000000 [130.6 + 1.467] [ 3] IN : 3.5 > > 11.624493 d= 0.000000 [130.6 + 1.967] [ 37] DATA0: aa 08 [skipped...] > > 11.625617 d= 0.001124 [131.7 + 1.050] [ 4] SPLIT > > 11.625617 d= 0.000000 [131.7 + 1.467] [ 3] IN : 3.5 > > 11.625867 d= 0.000250 [132.1 + 1.050] [ 4] SPLIT > > 11.625867 d= 0.000000 [132.1 + 1.467] [ 3] IN : 3.5 > > 11.625868 d= 0.000001 [132.1 + 1.983] [ 3] DATA0: 00 00 > > 11.626617 d= 0.000749 [132.7 + 1.050] [ 4] SPLIT > > 11.626617 d= 0.000000 [132.7 + 1.467] [ 3] IN : 3.5 > > 11.626867 d= 0.000250 [133.1 + 1.050] [ 4] SPLIT > > 11.626867 d= 0.000000 [133.1 + 1.467] [ 3] IN : 3.5 > > 11.626868 d= 0.000000 [133.1 + 1.967] [ 3] DATA0: 00 00 > > > > After the hub, they look as the following and may lead to broken > > perepherial transfer (as in case of PWC based webcam): > > > > 11.332004 d= 0.000997 [ 30.0 + 3.417] [ 3] IN : 5.5 > > 11.332007 d= 0.000003 [ 30.0 + 6.833] [800] DATA0: 8a 1c [skipped...] > > 11.334004 d= 0.001997 [ 32.0 + 3.417] [ 3] IN : 5.5 > > 11.334007 d= 0.000003 [ 32.0 + 6.750] [ 3] DATA0: 00 00 > > 11.335004 d= 0.000997 [ 33 + 3.417] [ 3] IN : 5.5 > > 11.335007 d= 0.000003 [ 33 + 6.750] [ 3] DATA0: 00 00 > > > > Removing this glitches makes us able to successfully run 10fps > > video stream from the webcam attached via USB hub. That was > > previously impossible. > > > > Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru> > > --- > > drivers/usb/musb/musb_host.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c > > index ed99ecd4e63a..75be92873b5b 100644 > > --- a/drivers/usb/musb/musb_host.c > > +++ b/drivers/usb/musb/musb_host.c > > @@ -85,6 +85,11 @@ static bool musb_qh_empty(struct musb_qh *qh) > > return list_empty(&qh->hep->urb_list); > > } > > > > +static bool musb_qh_singular(struct musb_qh *qh) > > +{ > > + return list_is_singular(&qh->hep->urb_list); > > +} > > + > > static void musb_qh_unlink_hep(struct musb_qh *qh) > > { > > if (!qh->hep) > > @@ -362,6 +367,19 @@ static void musb_advance_schedule(struct musb *musb, struct urb *urb, > > break; > > } > > > > + if (ready && !musb_qh_singular(qh)) { > > + struct urb *next_urb = list_next_entry(urb, urb_list); > > + > > + musb_dbg(musb, "... next ep%d %cX urb %p", hw_ep->epnum, is_in ? 'R' : 'T', next_urb); > > + musb_start_urb(musb, is_in, qh, next_urb); > > + > > + qh->is_ready = 0; > > + musb_giveback(musb, urb, status); > > + qh->is_ready = ready; > > + > > + return; > > + } > > + > > qh->is_ready = 0; > > musb_giveback(musb, urb, status); > > qh->is_ready = ready; > > -- > > 2.16.4 > > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
next prev reply other threads:[~2019-05-04 9:38 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <20190403185310.8437-1-matwey@sai.msu.ru> 2019-04-03 18:53 ` [6/6] usb: musb: Decrease URB starting latency in musb_advance_schedule() Matwey V. Kornilov 2019-04-30 15:31 ` Bin Liu 2019-04-30 15:31 ` [PATCH 6/6] " Bin Liu 2019-04-30 17:29 ` [6/6] " Alan Stern 2019-04-30 17:29 ` [PATCH 6/6] " Alan Stern 2019-05-04 9:38 ` Matwey V. Kornilov [this message] 2019-05-04 9:38 ` Matwey V. Kornilov 2019-04-24 15:42 ` [PATCH 0/6] musb: Improve performance for hub-attached webcams Matwey V. Kornilov 2019-04-30 15:20 ` Bin Liu 2019-06-14 16:45 ` [PATCH v2 " Matwey V. Kornilov 2019-06-14 16:45 ` [PATCH v2 1/6] usb: musb: Use USB_DIR_IN when calling musb_advance_schedule() Matwey V. Kornilov 2019-06-14 16:45 ` [PATCH v2 2/6] usb: musb: Introduce musb_qh_empty() helper function Matwey V. Kornilov 2019-06-14 16:45 ` [PATCH v2 3/6] usb: musb: Introduce musb_qh_free() " Matwey V. Kornilov 2019-06-14 16:45 ` [PATCH v2 4/6] usb: musb: Rename musb_start_urb() to musb_start_next_urb() Matwey V. Kornilov 2019-06-14 16:45 ` [PATCH v2 5/6] usb: musb: Introduce musb_start_urb() Matwey V. Kornilov 2019-06-14 16:45 ` [PATCH v2 6/6] usb: musb: Decrease URB starting latency in musb_advance_schedule() Matwey V. Kornilov 2019-07-02 17:29 ` [PATCH v2 0/6] musb: Improve performance for hub-attached webcams Matwey V. Kornilov 2019-07-02 17:33 ` Bin Liu 2019-09-09 16:33 ` Matwey V. Kornilov 2019-10-23 8:12 ` Matwey V. Kornilov 2020-01-01 16:26 ` [PATCH RESEND " Matwey V. Kornilov 2020-01-01 16:26 ` [PATCH RESEND v2 1/6] usb: musb: Use USB_DIR_IN when calling musb_advance_schedule() Matwey V. Kornilov 2020-01-01 16:26 ` [PATCH RESEND v2 2/6] usb: musb: Introduce musb_qh_empty() helper function Matwey V. Kornilov 2020-01-01 16:26 ` [PATCH RESEND v2 3/6] usb: musb: Introduce musb_qh_free() " Matwey V. Kornilov 2020-01-01 16:26 ` [PATCH RESEND v2 4/6] usb: musb: Rename musb_start_urb() to musb_start_next_urb() Matwey V. Kornilov 2020-01-01 16:26 ` [PATCH RESEND v2 5/6] usb: musb: Introduce musb_start_urb() Matwey V. Kornilov 2020-01-01 16:26 ` [PATCH RESEND v2 6/6] usb: musb: Decrease URB starting latency in musb_advance_schedule() Matwey V. Kornilov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAJs94EZLDotLHQmfhvzyRZWDAEL6hnUTmkXKMoVrO_JBJcHX4A@mail.gmail.com \ --to=matwey@sai.msu.ru \ --cc=b-liu@ti.com \ --cc=gregkh@linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=matwey.kornilov@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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).