[2/2,v2] dma: ipu_idmac: do not lose valid received data in the irq handler

Message ID 1296476549-10421-1-git-send-email-agust@denx.de (mailing list archive)
State Superseded, archived
Headers

Commit Message

Anatolij Gustschin Jan. 31, 2011, 12:22 p.m. UTC
  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(-)
  

Comments

Guennadi Liakhovetski Feb. 3, 2011, 10:09 a.m. UTC | #1
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@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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Markus Niebel Feb. 4, 2011, 9:19 a.m. UTC | #2
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
>
>> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
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
  
Anatolij Gustschin Feb. 4, 2011, 9:35 a.m. UTC | #3
Hi all,

On Thu, 3 Feb 2011 11:09:54 +0100 (CET)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
...
> 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?

Currently I'm quite busy, but I'll look at it this week end.

Thanks,
Anatolij
--
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 Feb. 4, 2011, 9:37 a.m. UTC | #4
Hi Markus

On Fri, 4 Feb 2011, Markus Niebel wrote:

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

But you haven't applied the patch, that my reply was actually referring to 
- the change to ipu_idmac.c? I think, that's the one, killing the 
double-buffering. But thanks for testing the streamon patch too!

Thanks
Guennadi

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

---
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
  
Markus Niebel Feb. 4, 2011, 11:37 a.m. UTC | #5
Hello Guennadi,

sorry, forget to explicitly write it in my mail, I also applied the
patch in question:

dma: ipu_idmac: do not lose valid received data in the irq handler

Will try the other way round without the patch later ...

Thanks
Markus

Am 04.02.2011 10:37, schrieb Guennadi Liakhovetski:
> Hi Markus
>
> On Fri, 4 Feb 2011, Markus Niebel wrote:
>
>> 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:
>
> But you haven't applied the patch, that my reply was actually referring to
> - the change to ipu_idmac.c? I think, that's the one, killing the
> double-buffering. But thanks for testing the streamon patch too!
>
> Thanks
> Guennadi
>
>>
>> [  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
>>>
>>>> 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@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> ---
> 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

--
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
  
Anatolij Gustschin Feb. 5, 2011, 1:35 p.m. UTC | #6
Hi Guennadi,

On Thu, 3 Feb 2011 11:09:54 +0100 (CET)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

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

Now I did some further testing with idmac patch applied and with
added debug print in the IDMAC interrupt handler. There is no problem.
Testing with capture.c (4 buffers used as default) shows that buffer
numbers toggle correctly for all 100 captured frames:
...
mx3-camera mx3-camera.0: MX3 Camera driver attached to camera 0
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 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
mx3-camera mx3-camera.0: MX3 Camera driver detached from camera 0

Also testing with my test application didn't show any problem.
When using more than 1 buffer (tested with 2, 3 and 4 queued
buffers) double buffering works as expected and frame numbers
toggle correctly. Capturing 30 frames produce:

mx3-camera mx3-camera.0: MX3 Camera driver attached to camera 0
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 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 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 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 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
mx3-camera mx3-camera.0: MX3 Camera driver detached from camera 0

Anatolij
--
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 Feb. 5, 2011, 4:36 p.m. UTC | #7
On Sat, 5 Feb 2011, Anatolij Gustschin wrote:

> Hi Guennadi,
> 
> On Thu, 3 Feb 2011 11:09:54 +0100 (CET)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 
> > 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?
> 
> Now I did some further testing with idmac patch applied and with
> added debug print in the IDMAC interrupt handler. There is no problem.
> Testing with capture.c (4 buffers used as default) shows that buffer
> numbers toggle correctly for all 100 captured frames:

Hm, interesting, I'll have to look at my testing in more detail then 
(once back from FOSDEM). Could you maybe try mplayer too?

Thanks
Guennadi

> ...
> mx3-camera mx3-camera.0: MX3 Camera driver attached to camera 0
> 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 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
> mx3-camera mx3-camera.0: MX3 Camera driver detached from camera 0
> 
> Also testing with my test application didn't show any problem.
> When using more than 1 buffer (tested with 2, 3 and 4 queued
> buffers) double buffering works as expected and frame numbers
> toggle correctly. Capturing 30 frames produce:
> 
> mx3-camera mx3-camera.0: MX3 Camera driver attached to camera 0
> 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 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 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 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 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
> mx3-camera mx3-camera.0: MX3 Camera driver detached from camera 0
> 
> Anatolij
> 

---
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
  
Anatolij Gustschin Feb. 5, 2011, 8:04 p.m. UTC | #8
On Sat, 5 Feb 2011 17:36:37 +0100 (CET)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
...
> > > Verified with both capture.c and mplayer. Could you, please, verify 
> > > whether you get the same behaviour and what the problem could be?
> > 
> > Now I did some further testing with idmac patch applied and with
> > added debug print in the IDMAC interrupt handler. There is no problem.
> > Testing with capture.c (4 buffers used as default) shows that buffer
> > numbers toggle correctly for all 100 captured frames:
> 
> Hm, interesting, I'll have to look at my testing in more detail then 
> (once back from FOSDEM). Could you maybe try mplayer too?

I can't try mplayer since I don't have mplayer setup for this.
But looking at the mplayer source I don't see why it should
behave differently. Depending on mode mplayer queues 2 or 6
buffers. Testing with my test app with 6 queued buffers shows
no issues, here the buffer numbers toggle correctly, too.

Anatolij
--
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 Feb. 7, 2011, 11:09 a.m. UTC | #9
On Sat, 5 Feb 2011, Anatolij Gustschin wrote:

> On Sat, 5 Feb 2011 17:36:37 +0100 (CET)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> ...
> > > > Verified with both capture.c and mplayer. Could you, please, verify 
> > > > whether you get the same behaviour and what the problem could be?
> > > 
> > > Now I did some further testing with idmac patch applied and with
> > > added debug print in the IDMAC interrupt handler. There is no problem.
> > > Testing with capture.c (4 buffers used as default) shows that buffer
> > > numbers toggle correctly for all 100 captured frames:
> > 
> > Hm, interesting, I'll have to look at my testing in more detail then 
> > (once back from FOSDEM). Could you maybe try mplayer too?
> 
> I can't try mplayer since I don't have mplayer setup for this.
> But looking at the mplayer source I don't see why it should
> behave differently. Depending on mode mplayer queues 2 or 6
> buffers. Testing with my test app with 6 queued buffers shows
> no issues, here the buffer numbers toggle correctly, too.

Ok, I've done a couple more tests. With larger frames, and, therefore 
lower fps - yes, with your patch buffers toggle correctly. Whereas in my 
tests with smaller frames and higher fps either only one buffer is used, 
or one is used much more often, than the other, e.g., 0 0 0 1 0 0 0 1 0... 
Could you try to verify? Without your patch with any fps buffers toggle 
consistently.

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
  
Anatolij Gustschin Feb. 7, 2011, 11:21 a.m. UTC | #10
On Mon, 7 Feb 2011 12:09:15 +0100 (CET)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
...
> > I can't try mplayer since I don't have mplayer setup for this.
> > But looking at the mplayer source I don't see why it should
> > behave differently. Depending on mode mplayer queues 2 or 6
> > buffers. Testing with my test app with 6 queued buffers shows
> > no issues, here the buffer numbers toggle correctly, too.
> 
> Ok, I've done a couple more tests. With larger frames, and, therefore 
> lower fps - yes, with your patch buffers toggle correctly. Whereas in my 
> tests with smaller frames and higher fps either only one buffer is used, 
> or one is used much more often, than the other, e.g., 0 0 0 1 0 0 0 1 0... 
> Could you try to verify? Without your patch with any fps buffers toggle 
> consistently.

How small are the frames in you test? What is the highest fps value in
your test?

Thanks,
Anatolij
--
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 Feb. 7, 2011, 11:35 a.m. UTC | #11
On Mon, 7 Feb 2011, Anatolij Gustschin wrote:

> On Mon, 7 Feb 2011 12:09:15 +0100 (CET)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> ...
> > > I can't try mplayer since I don't have mplayer setup for this.
> > > But looking at the mplayer source I don't see why it should
> > > behave differently. Depending on mode mplayer queues 2 or 6
> > > buffers. Testing with my test app with 6 queued buffers shows
> > > no issues, here the buffer numbers toggle correctly, too.
> > 
> > Ok, I've done a couple more tests. With larger frames, and, therefore 
> > lower fps - yes, with your patch buffers toggle correctly. Whereas in my 
> > tests with smaller frames and higher fps either only one buffer is used, 
> > or one is used much more often, than the other, e.g., 0 0 0 1 0 0 0 1 0... 
> > Could you try to verify? Without your patch with any fps buffers toggle 
> > consistently.
> 
> How small are the frames in you test? What is the highest fps value in
> your test?

QVGA, don't know fps exactly, pretty high, between 20 and 60fps, I think. 
Just try different frams sizes, go down to 64x48 or something.

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
  
Detlev Zundel Feb. 7, 2011, 1:01 p.m. UTC | #12
Hi Guennadi,

>> How small are the frames in you test? What is the highest fps value in
>> your test?
>
> QVGA, don't know fps exactly, pretty high, between 20 and 60fps, I think. 
> Just try different frams sizes, go down to 64x48 or something.

Is this a "real" usage scenario?  It feels that this is not what most
users will do and it certainly is not relevant for our application.  

Is it possible that if you are interested in such a scenario that you do
the testing?  We have spent quite a lot of time to fix the driver for
real (well full frame) capturing already and I am relucatant to spend
more time for corner cases.  Maybe we should document this as "known
limitations" of the setup?  What do you think?  I'll much rather have a
driver working for real world scenarios than for marginal test cases.

Thanks
  Detlev

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu@denx.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
  
Guennadi Liakhovetski Feb. 7, 2011, 1:35 p.m. UTC | #13
Hi Detlev

On Mon, 7 Feb 2011, Detlev Zundel wrote:

> Hi Guennadi,
> 
> >> How small are the frames in you test? What is the highest fps value in
> >> your test?
> >
> > QVGA, don't know fps exactly, pretty high, between 20 and 60fps, I think. 
> > Just try different frams sizes, go down to 64x48 or something.
> 
> Is this a "real" usage scenario?  It feels that this is not what most
> users will do and it certainly is not relevant for our application.  

QVGA at 25 / 50 / 60 fps is _certainly_ very much a real-life scenario.

> Is it possible that if you are interested in such a scenario that you do
> the testing?  We have spent quite a lot of time to fix the driver for
> real (well full frame) capturing already and I am relucatant to spend
> more time for corner cases.  Maybe we should document this as "known
> limitations" of the setup?  What do you think?  I'll much rather have a
> driver working for real world scenarios than for marginal test cases.

I am interested in avoiding regressions. In principle, this is a DMA 
driver, which I am not maintaining. Dan asked for my ack, so, I tested it 
and found an issue, which I would prefer to have resolved before 
committing. Of course, I don't have a decisive voice in this matter, so, 
the patch can also be merged without my ack. Otherwise - of course you 
don't have to continue testing, I will try to look at the issue as the 
time permits, and Dan will have to decide, whether he is prepared to 
commit this patch in its present form, or he would prefer this issue to be 
clarified.

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
  
Anatolij Gustschin Feb. 7, 2011, 1:45 p.m. UTC | #14
On Mon, 7 Feb 2011 12:35:44 +0100 (CET)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

> On Mon, 7 Feb 2011, Anatolij Gustschin wrote:
> 
> > On Mon, 7 Feb 2011 12:09:15 +0100 (CET)
> > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > ...
> > > > I can't try mplayer since I don't have mplayer setup for this.
> > > > But looking at the mplayer source I don't see why it should
> > > > behave differently. Depending on mode mplayer queues 2 or 6
> > > > buffers. Testing with my test app with 6 queued buffers shows
> > > > no issues, here the buffer numbers toggle correctly, too.
> > > 
> > > Ok, I've done a couple more tests. With larger frames, and, therefore 
> > > lower fps - yes, with your patch buffers toggle correctly. Whereas in my 
> > > tests with smaller frames and higher fps either only one buffer is used, 
> > > or one is used much more often, than the other, e.g., 0 0 0 1 0 0 0 1 0... 
> > > Could you try to verify? Without your patch with any fps buffers toggle 
> > > consistently.
> > 
> > How small are the frames in you test? What is the highest fps value in
> > your test?
> 
> QVGA, don't know fps exactly, pretty high, between 20 and 60fps, I think. 
> Just try different frams sizes, go down to 64x48 or something.

Testing of 960x243 frames at 30 fps has been done during all my previous
tests. I didn't see any issues at 30 fps.

Anatolij
--
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 Feb. 7, 2011, 2 p.m. UTC | #15
On Mon, 7 Feb 2011, Anatolij Gustschin wrote:

> On Mon, 7 Feb 2011 12:35:44 +0100 (CET)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 
> > On Mon, 7 Feb 2011, Anatolij Gustschin wrote:
> > 
> > > On Mon, 7 Feb 2011 12:09:15 +0100 (CET)
> > > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > > ...
> > > > > I can't try mplayer since I don't have mplayer setup for this.
> > > > > But looking at the mplayer source I don't see why it should
> > > > > behave differently. Depending on mode mplayer queues 2 or 6
> > > > > buffers. Testing with my test app with 6 queued buffers shows
> > > > > no issues, here the buffer numbers toggle correctly, too.
> > > > 
> > > > Ok, I've done a couple more tests. With larger frames, and, therefore 
> > > > lower fps - yes, with your patch buffers toggle correctly. Whereas in my 
> > > > tests with smaller frames and higher fps either only one buffer is used, 
> > > > or one is used much more often, than the other, e.g., 0 0 0 1 0 0 0 1 0... 
> > > > Could you try to verify? Without your patch with any fps buffers toggle 
> > > > consistently.
> > > 
> > > How small are the frames in you test? What is the highest fps value in
> > > your test?
> > 
> > QVGA, don't know fps exactly, pretty high, between 20 and 60fps, I think. 
> > Just try different frams sizes, go down to 64x48 or something.
> 
> Testing of 960x243 frames at 30 fps has been done during all my previous
> tests. I didn't see any issues at 30 fps.

Thanks for the clarification. You certainly realise, that your frame is 3 
times as large as mine. Sorry for not mentioning this before, I do 
appreciate your effort, and have a generally positive feeling at least 
regarding your IDMAC patch, but I don't want to just trust my feeling, 
that's why I tried to study the patch a bit more carefully. I also 
understand that you don't have infinite time to dedicate to this work. 
Neither do I. So, I will try as good as I can to try to find out the 
reason for this behaviour, any help from your side is greatly appreciated. 
But if we don't clarify this before the 2.6.39 merge window, I'm not sure, 
whether it would be smart to commit the patch as is.

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
  
Detlev Zundel Feb. 7, 2011, 2:43 p.m. UTC | #16
Hi Guennadi,

> Hi Detlev
>
> On Mon, 7 Feb 2011, Detlev Zundel wrote:
>
>> Hi Guennadi,
>> 
>> >> How small are the frames in you test? What is the highest fps value in
>> >> your test?
>> >
>> > QVGA, don't know fps exactly, pretty high, between 20 and 60fps, I think. 
>> > Just try different frams sizes, go down to 64x48 or something.
>> 
>> Is this a "real" usage scenario?  It feels that this is not what most
>> users will do and it certainly is not relevant for our application.  
>
> QVGA at 25 / 50 / 60 fps is _certainly_ very much a real-life scenario.

Yes, sure.  It was the 64x48 pixel you suggested which I believe to be
of doubtful value here.

>> Is it possible that if you are interested in such a scenario that you do
>> the testing?  We have spent quite a lot of time to fix the driver for
>> real (well full frame) capturing already and I am relucatant to spend
>> more time for corner cases.  Maybe we should document this as "known
>> limitations" of the setup?  What do you think?  I'll much rather have a
>> driver working for real world scenarios than for marginal test cases.
>
> I am interested in avoiding regressions. In principle, this is a DMA 
> driver, which I am not maintaining. Dan asked for my ack, so, I tested it 
> and found an issue, which I would prefer to have resolved before 
> committing. Of course, I don't have a decisive voice in this matter, so, 
> the patch can also be merged without my ack. Otherwise - of course you 
> don't have to continue testing, I will try to look at the issue as the 
> time permits, and Dan will have to decide, whether he is prepared to 
> commit this patch in its present form, or he would prefer this issue to be 
> clarified.

I'm fully in line with not wanting any regressions.  But is it a
regression if two independent testers report that the patch _improves_
the current situation?  As was shown by Anatolijs log, the current
driver certainly has a bug with respect to the handling of individual
frames.  This buggy behaviour only never showed up because nobody used
the driver on such a granularity.

We certainly appreciate if you can look into your scenario.

Thanks
  Detlev

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu@denx.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
  
Guennadi Liakhovetski Feb. 7, 2011, 4:49 p.m. UTC | #17
On Mon, 7 Feb 2011, Anatolij Gustschin wrote:

> On Mon, 7 Feb 2011 12:35:44 +0100 (CET)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 
> > On Mon, 7 Feb 2011, Anatolij Gustschin wrote:
> > 
> > > On Mon, 7 Feb 2011 12:09:15 +0100 (CET)
> > > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > > ...
> > > > > I can't try mplayer since I don't have mplayer setup for this.
> > > > > But looking at the mplayer source I don't see why it should
> > > > > behave differently. Depending on mode mplayer queues 2 or 6
> > > > > buffers. Testing with my test app with 6 queued buffers shows
> > > > > no issues, here the buffer numbers toggle correctly, too.
> > > > 
> > > > Ok, I've done a couple more tests. With larger frames, and, therefore 
> > > > lower fps - yes, with your patch buffers toggle correctly. Whereas in my 
> > > > tests with smaller frames and higher fps either only one buffer is used, 
> > > > or one is used much more often, than the other, e.g., 0 0 0 1 0 0 0 1 0... 
> > > > Could you try to verify? Without your patch with any fps buffers toggle 
> > > > consistently.
> > > 
> > > How small are the frames in you test? What is the highest fps value in
> > > your test?
> > 
> > QVGA, don't know fps exactly, pretty high, between 20 and 60fps, I think. 
> > Just try different frams sizes, go down to 64x48 or something.
> 
> Testing of 960x243 frames at 30 fps has been done during all my previous
> tests. I didn't see any issues at 30 fps.

Ok, I've found the reason. Buffer number repeats, when there is an 
underrun, which is happening in my tests, when frames are arriving quickly 
enough, but the user-space is not fast enough to process them, e.g., when 
it is writing them to files over NFS or even just displaying on the LCD. 
Without your patch these underruns happen just as well, they just don't 
get recognised, because there's always one buffer delayed, so, the queue 
is never empty.

Dan, please add my

Reviewed-(and-tested-)by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

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
  
Dan Williams Feb. 14, 2011, 9:57 a.m. UTC | #18
On Mon, Feb 7, 2011 at 8:49 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> Ok, I've found the reason. Buffer number repeats, when there is an
> underrun, which is happening in my tests, when frames are arriving quickly
> enough, but the user-space is not fast enough to process them, e.g., when
> it is writing them to files over NFS or even just displaying on the LCD.
> Without your patch these underruns happen just as well, they just don't
> get recognised, because there's always one buffer delayed, so, the queue
> is never empty.
>
> Dan, please add my
>
> Reviewed-(and-tested-)by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks, applied v2.

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

Patch

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