diff mbox

dma: fix ipu_idmac.c to not discard the last queued buffer

Message ID Pine.LNX.4.64.0905120908220.5087@axis700.grange (mailing list archive)
State RFC
Headers show

Commit Message

Guennadi Liakhovetski May 12, 2009, 9:16 a.m. UTC 
This also fixes the case of a single queued buffer, for example, when taking a
single frame snapshot with the mx3_camera driver.

Reported-by: Agustin <gatoguan-os@yahoo.com>
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

Subject: Re: Grabbing single stills on MX31 - Re: Solved? - Re: 
soc-camera: timing out during capture - Re: Testing latest mx3_camera.c

On Mon, 11 May 2009, Agustin wrote:

> On Thu, 7 May 2009, Guennadi Liakhovetski wrote:
> 
> > On Thu, 7 May 2009, Agustin Ferrin Pozuelo wrote:
> > > ...
> > > I thought about the fact that I was only queuing one buffer, and that 
> > > this might be a corner case as sample code uses a lot of them. And that 
> > > in the older code that funny things could happen in the handler if we 
> > > ran out of buffers, though they didn't happen.
> > > 
> > > So I have queued an extra buffer and voila, got it working.
> > > 
> > > So thanks again!
> > > 
> > > However, this could be a bug in ipu_idmac (or some other point), as 
> > > using a single buffer is very plausible, specially when grabbing huge 
> > > stills.
> > 
> > Great, thanks for testing and debugging! Ok, so, I will have to test this 
> > case some time...

Agustin, does this patch fix your problem? Dan, please, pull it as soon as 
we get a tested-by from Agustin.

> This workaround (queuing 2 buffers when needing only one) is having the 
> side effect of greatly increasing the time taken.
> 
> I did several tests playing with camera vertical blanking and looking at 
> capture times:
> 
>   Vblank / real             / user             / sys time:
>        0 / real    0m 0.90s / user    0m 0.00s / sys     0m 0.34s
>  1 frame / real    0m 1.04s / user    0m 0.00s / sys     0m 0.34s
> 2 frames / real    0m 1.18s / user    0m 0.00s / sys     0m 0.33s
> 2.5 (max)/ real    0m 1.23s / user    0m 0.00s / sys     0m 0.35s
> 
> I think the second frame is being captured altogether, and its dma 
> transfer is not allowing any other process to run meanwhile. 
> (VIDIOC_STREAMOFF is being called as soon as the first buffer is ready.)

I don't quite understand this. What exactly are you doing above? You 
submit 2 buffers and change vertical blanking? Where do you change the 
blanking - in the driver? How exactly?

> Do you think it will be too hard to fix?

The blanking issue or the 1-buffer problem?

Thanks
Guennadi

 drivers/dma/ipu/ipu_idmac.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Comments

Agustin May 12, 2009, 12:14 p.m. UTC | #1 
On Tue, 12 May 2009, Guennadi Liakhovetski wrote:

> 
> This also fixes the case of a single queued buffer, for example, when taking a
> single frame snapshot with the mx3_camera driver.
> 
> Reported-by: Agustin 
> Signed-off-by: Guennadi Liakhovetski

Signed-off-by: Agustin Ferrin Pozuelo

> ---
> 
> Subject: Re: Grabbing single stills on MX31 - Re: Solved? - Re: 
> soc-camera: timing out during capture - Re: Testing latest mx3_camera.c
> 
> On Mon, 11 May 2009, Agustin wrote:
> 
> > On Thu, 7 May 2009, Guennadi Liakhovetski wrote:
> > 
> > > On Thu, 7 May 2009, Agustin Ferrin Pozuelo wrote:
> > > > ...
> > > > I thought about the fact that I was only queuing one buffer, and that 
> > > > this might be a corner case as sample code uses a lot of them. And that 
> > > > in the older code that funny things could happen in the handler if we 
> > > > ran out of buffers, though they didn't happen.
> > > > 
> > > > So I have queued an extra buffer and voila, got it working.
> > > > 
> > > > So thanks again!
> > > > 
> > > > However, this could be a bug in ipu_idmac (or some other point), as 
> > > > using a single buffer is very plausible, specially when grabbing huge 
> > > > stills.
> > > 
> > > Great, thanks for testing and debugging! Ok, so, I will have to test this 
> > > case some time...
> 
> Agustin, does this patch fix your problem? Dan, please, pull it as soon as 
> we get a tested-by from Agustin.

Yes it works. And it happens to save 33% of system CPU time in addition to freeing a lot of memory bandwith. Timings after this fix:

  Vblank  /  real        /  user       / sys time:
   [any]  /  real 0.50s  /  user 0.00s / sys 0.22s

(Everything was for a 3888x1944x15bpp still)

> 
> > This workaround (queuing 2 buffers when needing only one) is having the 
> > side effect of greatly increasing the time taken.
> > 
> > I did several tests playing with camera vertical blanking and looking at 
> > capture times:
> > 
> >   Vblank / real             / user             / sys time:
> >        0 / real    0m 0.90s / user    0m 0.00s / sys     0m 0.34s
> >  1 frame / real    0m 1.04s / user    0m 0.00s / sys     0m 0.34s
> > 2 frames / real    0m 1.18s / user    0m 0.00s / sys     0m 0.33s
> > 2.5 (max)/ real    0m 1.23s / user    0m 0.00s / sys     0m 0.35s
> > 
> > I think the second frame is being captured altogether, and its dma 
> > transfer is not allowing any other process to run meanwhile. 
> > (VIDIOC_STREAMOFF is being called as soon as the first buffer is ready.)
> 
> I don't quite understand this. What exactly are you doing above? You 
> submit 2 buffers and change vertical blanking? Where do you change the 
> blanking - in the driver? How exactly?
> 
> > Do you think it will be too hard to fix?
> 
> The blanking issue or the 1-buffer problem?

Eh-eh, it was the same!
Thanks a lot!
--Agustín.

> 
> Thanks
> Guennadi
> 
> drivers/dma/ipu/ipu_idmac.c |    3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/dma/ipu/ipu_idmac.c b/drivers/dma/ipu/ipu_idmac.c
> index 3a4deea..9a5bc1a 100644
> --- a/drivers/dma/ipu/ipu_idmac.c
> +++ b/drivers/dma/ipu/ipu_idmac.c
> @@ -1272,7 +1272,8 @@ static irqreturn_t idmac_interrupt(int irq, void *dev_id)
>     /* Other interrupts do not interfere with this channel */
>     spin_lock(&ichan->lock);
>     if (unlikely(chan_id != IDMAC_SDC_0 && chan_id != IDMAC_SDC_1 &&
> -             ((curbuf >> chan_id) & 1) == ichan->active_buffer)) {
> +             ((curbuf >> chan_id) & 1) == ichan->active_buffer &&
> +             !list_is_last(ichan->queue.next, &ichan->queue))) {
>         int i = 100;
> 
>         /* This doesn't help. See comment in ipu_disable_channel() */
> -- 
> 1.6.2.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams May 12, 2009, 6:31 p.m. UTC | #2 
On Tue, May 12, 2009 at 5:14 AM, Agustin <gatoguan-os@yahoo.com> wrote:
>
> On Tue, 12 May 2009, Guennadi Liakhovetski wrote:
>
>>
>> This also fixes the case of a single queued buffer, for example, when taking a
>> single frame snapshot with the mx3_camera driver.
>>
>> Reported-by: Agustin
>> Signed-off-by: Guennadi Liakhovetski
>
> Signed-off-by: Agustin Ferrin Pozuelo

Applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux May 16, 2009, 4:46 p.m. UTC | #3 
On Tue, May 12, 2009 at 11:31:14AM -0700, Dan Williams wrote:
> On Tue, May 12, 2009 at 5:14 AM, Agustin <gatoguan-os@yahoo.com> wrote:
> >
> > On Tue, 12 May 2009, Guennadi Liakhovetski wrote:
> >
> >>
> >> This also fixes the case of a single queued buffer, for example, when taking a
> >> single frame snapshot with the mx3_camera driver.
> >>
> >> Reported-by: Agustin
> >> Signed-off-by: Guennadi Liakhovetski
> >
> > Signed-off-by: Agustin Ferrin Pozuelo
> 
> Applied.

Hopefully with real tags (iow, with email addresses) rather than what's
shown above (which is unacceptable.)
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski May 16, 2009, 6:22 p.m. UTC | #4 
On Sat, 16 May 2009, Russell King - ARM Linux wrote:

> On Tue, May 12, 2009 at 11:31:14AM -0700, Dan Williams wrote:
> > On Tue, May 12, 2009 at 5:14 AM, Agustin <gatoguan-os@yahoo.com> wrote:
> > >
> > > On Tue, 12 May 2009, Guennadi Liakhovetski wrote:
> > >
> > >>
> > >> This also fixes the case of a single queued buffer, for example, when taking a
> > >> single frame snapshot with the mx3_camera driver.
> > >>
> > >> Reported-by: Agustin
> > >> Signed-off-by: Guennadi Liakhovetski
> > >
> > > Signed-off-by: Agustin Ferrin Pozuelo
> > 
> > Applied.
> 
> Hopefully with real tags (iow, with email addresses) rather than what's
> shown above (which is unacceptable.)

Sure, Dan has done it perfectly:

http://git.kernel.org/?p=linux/kernel/git/djbw/async_tx.git;a=commitdiff;h=ad567ffb32f067b30606071eb568cf637fe42185

as it also was in the patch submission

http://marc.info/?l=linux-arm-kernel&m=124212146702853&w=2

and _not_ as it was in the mail that you quote.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux May 16, 2009, 6:50 p.m. UTC | #5 
On Sat, May 16, 2009 at 08:22:18PM +0200, Guennadi Liakhovetski wrote:
> On Sat, 16 May 2009, Russell King - ARM Linux wrote:
> 
> > On Tue, May 12, 2009 at 11:31:14AM -0700, Dan Williams wrote:
> > > On Tue, May 12, 2009 at 5:14 AM, Agustin <gatoguan-os@yahoo.com> wrote:
> > > >
> > > > On Tue, 12 May 2009, Guennadi Liakhovetski wrote:
> > > >
> > > >>
> > > >> This also fixes the case of a single queued buffer, for example, when taking a
> > > >> single frame snapshot with the mx3_camera driver.
> > > >>
> > > >> Reported-by: Agustin
> > > >> Signed-off-by: Guennadi Liakhovetski
> > > >
> > > > Signed-off-by: Agustin Ferrin Pozuelo
> > > 
> > > Applied.
> > 
> > Hopefully with real tags (iow, with email addresses) rather than what's
> > shown above (which is unacceptable.)
> 
> Sure, Dan has done it perfectly:
> 
> http://git.kernel.org/?p=linux/kernel/git/djbw/async_tx.git;a=commitdiff;h=ad567ffb32f067b30606071eb568cf637fe42185
> 
> as it also was in the patch submission
> 
> http://marc.info/?l=linux-arm-kernel&m=124212146702853&w=2
> 
> and _not_ as it was in the mail that you quote.

Looks like Agustin's mail client is removing the <...> parts from those
tags - look at the next message in the thread.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/dma/ipu/ipu_idmac.c b/drivers/dma/ipu/ipu_idmac.c
index 3a4deea..9a5bc1a 100644
--- a/drivers/dma/ipu/ipu_idmac.c
+++ b/drivers/dma/ipu/ipu_idmac.c
@@ -1272,7 +1272,8 @@  static irqreturn_t idmac_interrupt(int irq, void *dev_id)
 	/* Other interrupts do not interfere with this channel */
 	spin_lock(&ichan->lock);
 	if (unlikely(chan_id != IDMAC_SDC_0 && chan_id != IDMAC_SDC_1 &&
-		     ((curbuf >> chan_id) & 1) == ichan->active_buffer)) {
+		     ((curbuf >> chan_id) & 1) == ichan->active_buffer &&
+		     !list_is_last(ichan->queue.next, &ichan->queue))) {
 		int i = 100;
 
 		/* This doesn't help. See comment in ipu_disable_channel() */