Re: [PATCH] dma: fix ipu_idmac.c to not discard the last queued buffer

Re: [PATCH] dma: fix ipu_idmac.c to not discard the last queued buffer

[Agustin]

--- On Sat, 16/5/09, Russell King wrote:
> On Sat, May 16, 2009 at 08:22:18PM
> +0200, Guennadi Liakhovetski wrote:
> > > 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.

Indeed. It was the new flash-based memory-hogging client from Yahoo. Just switched to the old one, not so 'smart' but not very compliant either... I will have to look for a nicer option.

--Agustín.

Solved? - Re: soc-camera: timing out during capture - Re: Testing latest mx3_camera.c

[Agustin Ferrin Pozuelo]

Holy cow...


On Tue, 5 May 2009, Agustin wrote:
> On Tue, 5 May 2009, Guennadi Liakhovetski wrote:
> > On Tue, 5 May 2009, Agustin wrote:
> > 
> > > No, as there is no driver_match_device() in 2.6.29 nor in my patched 
> > > version. How important is that?
> > 
> > No, sorry, forget it, that's not your problem.
> > 
> > > Meanwhile I noticed that IRQ 176 is being triggered, then discarded as 
> > > "unhandled" by ipu_idmac, who gives the message "IRQ on active buffer on 
> > > channel 7, active 0, ready 0, 0, current 0!" below...
> > 
> > Yes, and this is not good. If you look in drivers/dma/ipu/ipu_idmac.c 
> > idmac_interrupt() you'll see, that this message is printed when IDMAC 
> > produces an interrupt for a DMA buffer, but at the same time it says, that 
> > the buffer, that should have completed is still in use... I've seen a few 
> > of such inconsistencies, and up to now always managed to get rid of them 
> > in one or another way. But that should not be related to the conversion. 
> > Maybe your formats on the sensor and on the SoC do not match, verify that.
> 
> Thanks for the tip but I am still out of luck. I enabled DEBUG in ipu_idmac.c 
> just to see that frame start and end are happening more or less when they 
> should:
> 
>    [...]
>    camera 0-0: mx3_camera: Submitted cookie 2 DMA 0x86400000
>    Got SOF IRQ 177 on Channel 7
>    Got EOF IRQ 178 on Channel 7
>    dma dma0chan7: ipu_idmac: IDMAC irq 176, buf 0
>    dma dma0chan7: ipu_idmac: IRQ on active buffer on channel 7, active 0, ready 
> 0, 0, current 0!
>    Select timeout.
>    [...]
> 
> I also configured everything to the simplest mode I can have: 8-bit bus, sample 
> falling.
> 
> So I am now looking at IDMAC, trying to guess what could be wrong... [snip]

After checking out every single bit in CSI and IDMAC to be correct according to reference and the same I had in the previous/working version...

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.

--Agustín.

[snip!]

Re: Solved? - Re: soc-camera: timing out during capture - Re: Testing latest mx3_camera.c

[Guennadi Liakhovetski]

On Thu, 7 May 2009, Agustin Ferrin Pozuelo wrote:

> Holy cow...

mu-u-u-u-u-u?:-)

> After checking out every single bit in CSI and IDMAC to be correct 
> according to reference and the same I had in the previous/working 
> version...
> 
> 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...

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

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

[Agustin]

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...

Guennadi,

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.)

Do you think it will be too hard to fix?

Regards,
--Agustín.

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

[Guennadi Liakhovetski]

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(-)

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

Re: [PATCH] dma: fix ipu_idmac.c to not discard the last queued buffer

[Agustin]

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

Re: [PATCH] dma: fix ipu_idmac.c to not discard the last queued buffer

[Dan Williams]

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.

Re: [PATCH] dma: fix ipu_idmac.c to not discard the last queued buffer

[Russell King - ARM Linux]

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.)

Re: [PATCH] dma: fix ipu_idmac.c to not discard the last queued buffer

[Guennadi Liakhovetski]

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/

Re: [PATCH] dma: fix ipu_idmac.c to not discard the last queued buffer

[Russell King - ARM Linux]

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.