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