Hello Guennadi, hello Anatolij

I've tried that with my setup:

Hardware: i.MX35, special CCD camera over FPGA
Kernel: 2.6.34

patch v4l: soc-camera: start stream after queueing the buffers is 
applied and our camera driver handles streamon / streamoff so that no 
sync signal / clock is provided, when not streaming.

Our setup works with 4 buffers

What we see is as we would expect plus no difference with 1st buffer:

[  206.770000] i5ccdhb i5ccdhb.0: soc_i5ccdhb_s_stream - enable 1
[  207.350000] i5ccdhb i5ccdhb.0: i5ccdhb_streamon: fps (29.412)
[  207.370000] idmac_interrupt(): IDMAC irq 177, buf 0, current 0
[  207.410000] idmac_interrupt(): IDMAC irq 177, buf 1, current 1
[  207.440000] idmac_interrupt(): IDMAC irq 177, buf 0, current 0
[  207.470000] idmac_interrupt(): IDMAC irq 177, buf 1, current 1
[  207.540000] idmac_interrupt(): IDMAC irq 177, buf 0, current 0
[  207.580000] idmac_interrupt(): IDMAC irq 177, buf 1, current 1
[  207.610000] idmac_interrupt(): IDMAC irq 177, buf 0, current 0
[  207.650000] idmac_interrupt(): IDMAC irq 177, buf 1, current 1
[  207.680000] idmac_interrupt(): IDMAC irq 177, buf 0, current 0
[  207.710000] idmac_interrupt(): IDMAC irq 177, buf 1, current 1
[  207.750000] idmac_interrupt(): IDMAC irq 177, buf 0, current 0
[  207.780000] idmac_interrupt(): IDMAC irq 177, buf 1, current 1
...
[  241.370000] idmac_interrupt(): IDMAC irq 177, buf 0, current 0
[  241.410000] idmac_interrupt(): IDMAC irq 177, buf 1, current 1
[  241.440000] idmac_interrupt(): IDMAC irq 177, buf 0, current 0
[  241.470000] idmac_interrupt(): IDMAC irq 177, buf 1, current 1
[  241.510000] idmac_interrupt(): IDMAC irq 177, buf 0, current 0
[  241.540000] idmac_interrupt(): IDMAC irq 177, buf 1, current 1
[  241.580000] idmac_interrupt(): IDMAC irq 177, buf 0, current 0
[  241.610000] idmac_interrupt(): IDMAC irq 177, buf 1, current 1
[  257.190000] i5ccdhb i5ccdhb.0: soc_i5ccdhb_s_stream - enable 0



Am 03.02.2011 11:09, schrieb Guennadi Liakhovetski:

Hi Anatolij

On Mon, 31 Jan 2011, Anatolij Gustschin wrote:

I'm afraid there seems to be a problem with your patch. I have no idea
what is causing it, but I'm just observing some wrong behaviour, that is
not there without it. Namely, I added a debug print to the IDMAC interrupt
handler

  	curbuf	= idmac_read_ipureg(&ipu_data, IPU_CHA_CUR_BUF);
  	err	= idmac_read_ipureg(&ipu_data, IPU_INT_STAT_4);

+	printk(KERN_DEBUG "%s(): IDMAC irq %d, buf %d, current %d\n", __func__,
+	       irq, ichan->active_buffer, (curbuf>>  chan_id)&  1);

  	if (err&  (1<<  chan_id)) {
  		idmac_write_ipureg(&ipu_data, 1<<  chan_id, IPU_INT_STAT_4);

and without your patch I see buffer numbers correctly toggling all the
time like

idmac_interrupt(): IDMAC irq 177, buf 0, current 0
idmac_interrupt(): IDMAC irq 177, buf 0, current 1
idmac_interrupt(): IDMAC irq 177, buf 1, current 0
idmac_interrupt(): IDMAC irq 177, buf 0, current 1
idmac_interrupt(): IDMAC irq 177, buf 1, current 0
idmac_interrupt(): IDMAC irq 177, buf 0, current 1
...

Yes, the first interrupt is different, that's where I'm dropping /
postponing it. With your patch only N (equal to the number of buffers
used, I think) first interrupts toggle, then always only one buffer is
used:

idmac_interrupt(): IDMAC irq 177, buf 0, current 0
idmac_interrupt(): IDMAC irq 177, buf 1, current 1
idmac_interrupt(): IDMAC irq 177, buf 0, current 0
idmac_interrupt(): IDMAC irq 177, buf 1, current 1
idmac_interrupt(): IDMAC irq 177, buf 0, current 0
idmac_interrupt(): IDMAC irq 177, buf 1, current 1
idmac_interrupt(): IDMAC irq 177, buf 0, current 0
idmac_interrupt(): IDMAC irq 177, buf 0, current 0
idmac_interrupt(): IDMAC irq 177, buf 0, current 0
...

Verified with both capture.c and mplayer. Could you, please, verify
whether you get the same behaviour and what the problem could be?

Thanks
Guennadi

quoted

Currently when two or more buffers are queued by the camera driver
and so the double buffering is enabled in the idmac, we lose one
frame comming from CSI since the reporting of arrival of the first
frame is deferred by the DMAIC_7_EOF interrupt handler and reporting
of the arrival of the last frame is not done at all. So when requesting
N frames from the image sensor we actually receive N - 1 frames in
user space.

The reason for this behaviour is that the DMAIC_7_EOF interrupt
handler misleadingly assumes that the CUR_BUF flag is pointing to the
buffer used by the IDMAC. Actually it is not the case since the
CUR_BUF flag will be flipped by the FSU when the FSU is sending the
<TASK>_NEW_FRM_RDY signal when new frame data is delivered by the CSI.
When sending this singal, FSU updates the DMA_CUR_BUF and the
DMA_BUFx_RDY flags: the DMA_CUR_BUF is flipped, the DMA_BUFx_RDY
is cleared, indicating that the frame data is beeing written by
the IDMAC to the pointed buffer. DMA_BUFx_RDY is supposed to be
set to the ready state again by the MCU, when it has handled the
received data. DMAIC_7_CUR_BUF flag won't be flipped here by the
IPU, so waiting for this event in the EOF interrupt handler is wrong.
Actually there is no spurious interrupt as described in the comments,
this is the valid DMAIC_7_EOF interrupt indicating reception of the
frame from CSI.

The patch removes code that waits for flipping of the DMAIC_7_CUR_BUF
flag in the DMAIC_7_EOF interrupt handler. As the comment in the
current code denotes, this waiting doesn't help anyway. As a result
of this removal the reporting of the first arrived frame is not
deferred to the time of arrival of the next frame and the drivers
software flag 'ichan->active_buffer' is in sync with DMAIC_7_CUR_BUF
flag, so the reception of all requested frames works.

This has been verified on the hardware which is triggering the
image sensor by the programmable state machine, allowing to
obtain exact number of frames. On this hardware we do not tolerate
losing frames.

This patch also removes resetting the DMA_BUFx_RDY flags of
all channels in ipu_disable_channel() since transfers on other
DMA channels might be triggered by other running tasks and the
buffers should always be ready for data sending or reception.

Signed-off-by: Anatolij Gustschin<agust@denx.de>
---
v2:
     Revise the commit message to provide more and correct
     information about the observed problem and proposed fix

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

diff --git a/drivers/dma/ipu/ipu_idmac.c b/drivers/dma/ipu/ipu_idmac.c
index cb26ee9..c1a125e 100644
--- a/drivers/dma/ipu/ipu_idmac.c
+++ b/drivers/dma/ipu/ipu_idmac.c

@@ -1145,29 +1145,6 @@ static int ipu_disable_channel(struct idmac *idmac, struct idmac_channel *ichan,
  	reg = idmac_read_icreg(ipu, IDMAC_CHA_EN);
  	idmac_write_icreg(ipu, reg&  ~chan_mask, IDMAC_CHA_EN);

-	/*
-	 * Problem (observed with channel DMAIC_7): after enabling the channel
-	 * and initialising buffers, there comes an interrupt with current still
-	 * pointing at buffer 0, whereas it should use buffer 0 first and only
-	 * generate an interrupt when it is done, then current should already
-	 * point to buffer 1. This spurious interrupt also comes on channel
-	 * DMASDC_0. With DMAIC_7 normally, is we just leave the ISR after the
-	 * first interrupt, there comes the second with current correctly
-	 * pointing to buffer 1 this time. But sometimes this second interrupt
-	 * doesn't come and the channel hangs. Clearing BUFx_RDY when disabling
-	 * the channel seems to prevent the channel from hanging, but it doesn't
-	 * prevent the spurious interrupt. This might also be unsafe. Think
-	 * about the IDMAC controller trying to switch to a buffer, when we
-	 * clear the ready bit, and re-enable it a moment later.
-	 */
-	reg = idmac_read_ipureg(ipu, IPU_CHA_BUF0_RDY);
-	idmac_write_ipureg(ipu, 0, IPU_CHA_BUF0_RDY);
-	idmac_write_ipureg(ipu, reg&  ~(1UL<<  channel), IPU_CHA_BUF0_RDY);
-
-	reg = idmac_read_ipureg(ipu, IPU_CHA_BUF1_RDY);
-	idmac_write_ipureg(ipu, 0, IPU_CHA_BUF1_RDY);
-	idmac_write_ipureg(ipu, reg&  ~(1UL<<  channel), IPU_CHA_BUF1_RDY);
-
  	spin_unlock_irqrestore(&ipu->lock, flags);

  	return 0;

@@ -1246,33 +1223,6 @@ 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&&
-		     !list_is_last(ichan->queue.next,&ichan->queue))) {
-		int i = 100;
-
-		/* This doesn't help. See comment in ipu_disable_channel() */
-		while (--i) {
-			curbuf = idmac_read_ipureg(&ipu_data, IPU_CHA_CUR_BUF);
-			if (((curbuf>>  chan_id)&  1) != ichan->active_buffer)
-				break;
-			cpu_relax();
-		}
-
-		if (!i) {
-			spin_unlock(&ichan->lock);
-			dev_dbg(dev,
-				"IRQ on active buffer on channel %x, active "
-				"%d, ready %x, %x, current %x!\n", chan_id,
-				ichan->active_buffer, ready0, ready1, curbuf);
-			return IRQ_NONE;
-		} else
-			dev_dbg(dev,
-				"Buffer deactivated on channel %x, active "
-				"%d, ready %x, %x, current %x, rest %d!\n", chan_id,
-				ichan->active_buffer, ready0, ready1, curbuf, i);
-	}
-
  	if (unlikely((ichan->active_buffer&&  (ready1>>  chan_id)&  1) ||
  		     (!ichan->active_buffer&&  (ready0>>  chan_id)&  1)
  		     )) {
--

1.7.1

---
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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html