| Message ID | CABb+yY3iPoXBFpKgmdTijeee71Xz9dkwjcay8keu+vsdWtDfXw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/07/2013 12:31 PM, Jassi Brar wrote: > On Thu, Feb 7, 2013 at 4:08 PM, Alvaro Moran<dirac3000@gmail.com> wrote: >> Due to the original driver design, only one request was processed at a >> time by the driver, even if the low-level part of the driver was able to >> handle 2 requests. >> With this patch we are able to create 2 microcodes per thread and to >> launch the second transfer on the interrupt handler of the first one, >> instead of having to wait for the tasklet to generate the microcode. >> > The following seems more appropriate and complete. Does it fix your problem? > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index 758122f..a821d71 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -2292,13 +2292,12 @@ static inline void fill_queue(struct > dma_pl330_chan *pch) > > /* If already submitted */ > if (desc->status == BUSY) > - break; > + continue; > > ret = pl330_submit_req(pch->pl330_chid, > &desc->req); > if (!ret) { > desc->status = BUSY; > - break; > } else if (ret == -EAGAIN) { > /* QFull or DMAC Dying */ > break; Actually that isn't good enough. With your patch it will keep on looping on the pch->work_list entries, but it will call pl330_submit_req the first time only. I want it to call the function twice, so it will generate 2 microcodes (one per available request) and it will be ready the moment we get into the interrupt handler.
On Thu, Feb 7, 2013 at 7:16 PM, dirac3000 <dirac3000@gmail.com> wrote: > On 02/07/2013 12:31 PM, Jassi Brar wrote: > >> On Thu, Feb 7, 2013 at 4:08 PM, Alvaro Moran<dirac3000@gmail.com> wrote: >>> >>> Due to the original driver design, only one request was processed at a >>> time by the driver, even if the low-level part of the driver was able to >>> handle 2 requests. >>> With this patch we are able to create 2 microcodes per thread and to >>> launch the second transfer on the interrupt handler of the first one, >>> instead of having to wait for the tasklet to generate the microcode. >>> >> The following seems more appropriate and complete. Does it fix your >> problem? >> >> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >> index 758122f..a821d71 100644 >> --- a/drivers/dma/pl330.c >> +++ b/drivers/dma/pl330.c >> @@ -2292,13 +2292,12 @@ static inline void fill_queue(struct >> dma_pl330_chan *pch) >> >> /* If already submitted */ >> if (desc->status == BUSY) >> - break; >> + continue; >> >> ret = pl330_submit_req(pch->pl330_chid, >> &desc->req); >> if (!ret) { >> desc->status = BUSY; >> - break; >> } else if (ret == -EAGAIN) { >> /* QFull or DMAC Dying */ >> break; > > > > Actually that isn't good enough. With your patch it will keep on looping on > the pch->work_list entries, but it will call pl330_submit_req the first time > only. I want it to call the function twice, so it will generate 2 microcodes > (one per available request) and it will be ready the moment we get into the > interrupt handler. Why would it "keep on looping"? It's a for loop that will exit after iterating over the list once or when the lower layer indicates QFull - whichever comes first. Practically it achieves the same effect only without introducing a new local variable 'busy_reqs' Did you actually test the patch? If yes and it didn't work, please share some log suitable log. thnx.
On 02/07/2013 03:12 PM, Jassi Brar wrote: > On Thu, Feb 7, 2013 at 7:16 PM, dirac3000<dirac3000@gmail.com> wrote: >> On 02/07/2013 12:31 PM, Jassi Brar wrote: >> >>> On Thu, Feb 7, 2013 at 4:08 PM, Alvaro Moran<dirac3000@gmail.com> wrote: >>>> >>>> Due to the original driver design, only one request was processed at a >>>> time by the driver, even if the low-level part of the driver was able to >>>> handle 2 requests. >>>> With this patch we are able to create 2 microcodes per thread and to >>>> launch the second transfer on the interrupt handler of the first one, >>>> instead of having to wait for the tasklet to generate the microcode. >>>> >>> The following seems more appropriate and complete. Does it fix your >>> problem? >>> >>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >>> index 758122f..a821d71 100644 >>> --- a/drivers/dma/pl330.c >>> +++ b/drivers/dma/pl330.c >>> @@ -2292,13 +2292,12 @@ static inline void fill_queue(struct >>> dma_pl330_chan *pch) >>> >>> /* If already submitted */ >>> if (desc->status == BUSY) >>> - break; >>> + continue; >>> >>> ret = pl330_submit_req(pch->pl330_chid, >>> &desc->req); >>> if (!ret) { >>> desc->status = BUSY; >>> - break; >>> } else if (ret == -EAGAIN) { >>> /* QFull or DMAC Dying */ >>> break; >> >> >> >> Actually that isn't good enough. With your patch it will keep on looping on >> the pch->work_list entries, but it will call pl330_submit_req the first time >> only. I want it to call the function twice, so it will generate 2 microcodes >> (one per available request) and it will be ready the moment we get into the >> interrupt handler. > > Why would it "keep on looping"? It's a for loop that will exit after > iterating over the list once or when the lower layer indicates QFull - > whichever comes first. Practically it achieves the same effect only > without introducing a new local variable 'busy_reqs' > Did you actually test the patch? If yes and it didn't work, please > share some log suitable log. > thnx. Oh, my fault, you are right, I didn't read it carefully! Now I actually tested the patch, so I am 100% sure it works and it increases the performance of the requests when they are correctly queued. Thanks, -Alvaro
On 02/07/2013 06:08 PM, dirac3000 wrote: > On 02/07/2013 03:12 PM, Jassi Brar wrote: > >> On Thu, Feb 7, 2013 at 7:16 PM, dirac3000<dirac3000@gmail.com> wrote: >>> On 02/07/2013 12:31 PM, Jassi Brar wrote: >>> >>>> On Thu, Feb 7, 2013 at 4:08 PM, Alvaro Moran<dirac3000@gmail.com> >>>> wrote: >>>>> >>>>> Due to the original driver design, only one request was processed at a >>>>> time by the driver, even if the low-level part of the driver was >>>>> able to >>>>> handle 2 requests. >>>>> With this patch we are able to create 2 microcodes per thread and to >>>>> launch the second transfer on the interrupt handler of the first one, >>>>> instead of having to wait for the tasklet to generate the microcode. >>>>> >>>> The following seems more appropriate and complete. Does it fix your >>>> problem? >>>> >>>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >>>> index 758122f..a821d71 100644 >>>> --- a/drivers/dma/pl330.c >>>> +++ b/drivers/dma/pl330.c >>>> @@ -2292,13 +2292,12 @@ static inline void fill_queue(struct >>>> dma_pl330_chan *pch) >>>> >>>> /* If already submitted */ >>>> if (desc->status == BUSY) >>>> - break; >>>> + continue; >>>> >>>> ret = pl330_submit_req(pch->pl330_chid, >>>> &desc->req); >>>> if (!ret) { >>>> desc->status = BUSY; >>>> - break; >>>> } else if (ret == -EAGAIN) { >>>> /* QFull or DMAC Dying */ >>>> break; >>> >>> >>> >>> Actually that isn't good enough. With your patch it will keep on >>> looping on >>> the pch->work_list entries, but it will call pl330_submit_req the >>> first time >>> only. I want it to call the function twice, so it will generate 2 >>> microcodes >>> (one per available request) and it will be ready the moment we get >>> into the >>> interrupt handler. >> >> Why would it "keep on looping"? It's a for loop that will exit after >> iterating over the list once or when the lower layer indicates QFull - >> whichever comes first. Practically it achieves the same effect only >> without introducing a new local variable 'busy_reqs' >> Did you actually test the patch? If yes and it didn't work, please >> share some log suitable log. >> thnx. > > > Oh, my fault, you are right, I didn't read it carefully! > Now I actually tested the patch, so I am 100% sure it works and it > increases the performance of the requests when they are correctly queued. > > Thanks, > > -Alvaro As I told you in a previous email I tested the patch and it works fine. Any chance of seeing it accepted in the next version?
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index 758122f..a821d71 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -2292,13 +2292,12 @@ static inline void fill_queue(struct dma_pl330_chan *pch) /* If already submitted */ if (desc->status == BUSY) - break; + continue; ret = pl330_submit_req(pch->pl330_chid, &desc->req); if (!ret) { desc->status = BUSY; - break; } else if (ret == -EAGAIN) { /* QFull or DMAC Dying */