[v3,4/4] media i.MX27 camera: handle overflows properly.

  Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 Changes since v2:
 - Use true and false for bool variables.

---
 drivers/media/video/mx2_camera.c |   38 ++++++++++++++++----------------------
 1 files changed, 16 insertions(+), 22 deletions(-)
  

Hi Javier

Sorry again for a delayed reaction... I'm currently trying to prepare a 
push for 3.4 and I stumbled over this your patch:

On Mon, 30 Jan 2012, Javier Martin wrote:

> 
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> ---
>  Changes since v2:
>  - Use true and false for bool variables.
> 
> ---
>  drivers/media/video/mx2_camera.c |   38 ++++++++++++++++----------------------
>  1 files changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> index e7ccd97..09bcfe0 100644
> --- a/drivers/media/video/mx2_camera.c
> +++ b/drivers/media/video/mx2_camera.c
> @@ -1210,7 +1210,7 @@ static struct soc_camera_host_ops mx2_soc_camera_host_ops = {
>  };
>  
>  static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
> -		int bufnum)
> +		int bufnum, bool err)
>  {
>  	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
>  	struct mx2_buffer *buf;
> @@ -1258,7 +1258,10 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
>  		list_del_init(&buf->queue);
>  		do_gettimeofday(&vb->v4l2_buf.timestamp);
>  		vb->v4l2_buf.sequence = pcdev->frame_count;
> -		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> +		if (err)
> +			vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
> +		else
> +			vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
>  	}
>  
>  	pcdev->frame_count++;
> @@ -1302,21 +1305,12 @@ static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
>  			__func__);
>  
>  	if (status & (1 << 7)) { /* overflow */
> -		u32 cntl;
> -		/*
> -		 * We only disable channel 1 here since this is the only
> -		 * enabled channel
> -		 *
> -		 * FIXME: the correct DMA overflow handling should be resetting
> -		 * the buffer, returning an error frame, and continuing with
> -		 * the next one.
> -		 */
> -		cntl = readl(pcdev->base_emma + PRP_CNTL);
> -		writel(cntl & ~(PRP_CNTL_CH1EN | PRP_CNTL_CH2EN),
> -		       pcdev->base_emma + PRP_CNTL);
> -		writel(cntl, pcdev->base_emma + PRP_CNTL);
> -	}
> -	if (((status & (3 << 5)) == (3 << 5)) ||
> +		buf = list_entry(pcdev->active_bufs.next,
> +			struct mx2_buffer, queue);
> +		mx27_camera_frame_done_emma(pcdev,
> +					buf->bufnum, true);
> +		status &= ~(1 << 7);
> +	} else if (((status & (3 << 5)) == (3 << 5)) ||

This means, in case of an overflow you don't reset the channels any more? 
Is there a reason for that?

Thanks
Guennadi

>  		((status & (3 << 3)) == (3 << 3))) {
>  		/*
>  		 * Both buffers have triggered, process the one we're expecting

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

Hi Guennadi,

On 20 February 2012 13:17, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
>> @@ -1302,21 +1305,12 @@ static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
>>                       __func__);
>>
>>       if (status & (1 << 7)) { /* overflow */
>> -             u32 cntl;
>> -             /*
>> -              * We only disable channel 1 here since this is the only
>> -              * enabled channel
>> -              *
>> -              * FIXME: the correct DMA overflow handling should be resetting
>> -              * the buffer, returning an error frame, and continuing with
>> -              * the next one.
>> -              */
>> -             cntl = readl(pcdev->base_emma + PRP_CNTL);
>> -             writel(cntl & ~(PRP_CNTL_CH1EN | PRP_CNTL_CH2EN),
>> -                    pcdev->base_emma + PRP_CNTL);
>> -             writel(cntl, pcdev->base_emma + PRP_CNTL);
>> -     }
>> -     if (((status & (3 << 5)) == (3 << 5)) ||
>> +             buf = list_entry(pcdev->active_bufs.next,
>> +                     struct mx2_buffer, queue);
>> +             mx27_camera_frame_done_emma(pcdev,
>> +                                     buf->bufnum, true);
>> +             status &= ~(1 << 7);
>> +     } else if (((status & (3 << 5)) == (3 << 5)) ||
>
> This means, in case of an overflow you don't reset the channels any more?
> Is there a reason for that?

Apparently, while I added the "returning an error frame, and continue
with the next one" part I accidentally removed the "resetting the
buffer" part.

Let me send a v4 version of this patch. I hope to have it ready for tomorrow.