[v2] media i.MX27 camera: properly detect frame loss.

  As V4L2 specification states, frame_count must also
regard lost frames so that the user can handle that
case properly.

This patch adds a mechanism to increment the frame
counter even when a video buffer is not available
and a discard buffer is used.

---
Changes since v1:
 - Initialize "frame_count" to -1 instead of using
   "firstirq" variable.

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/media/video/mx2_camera.c |   45 ++++++++++++++++++++-----------------
 1 files changed, 24 insertions(+), 21 deletions(-)
  

Hi Javier

Please, excuse my curiosity and bear with my lack of understanding :-)

On Wed, 11 Jan 2012, Javier Martin wrote:

> As V4L2 specification states, frame_count must also
> regard lost frames so that the user can handle that
> case properly.
> 
> This patch adds a mechanism to increment the frame
> counter even when a video buffer is not available
> and a discard buffer is used.
> 
> ---
> Changes since v1:
>  - Initialize "frame_count" to -1 instead of using
>    "firstirq" variable.
> 
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> ---
>  drivers/media/video/mx2_camera.c |   45 ++++++++++++++++++++-----------------
>  1 files changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> index ca76dd2..68038e7 100644
> --- a/drivers/media/video/mx2_camera.c
> +++ b/drivers/media/video/mx2_camera.c
> @@ -369,7 +369,7 @@ static int mx2_camera_add_device(struct soc_camera_device *icd)
>  	writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
>  
>  	pcdev->icd = icd;
> -	pcdev->frame_count = 0;
> +	pcdev->frame_count = -1;

I'm adding a comment above this line:

+	/* Discard the first frame, begin valid frames with 0 */

>  
>  	dev_info(icd->parent, "Camera driver attached to camera %d\n",
>  		 icd->devnum);
> @@ -572,6 +572,7 @@ static void mx2_videobuf_queue(struct videobuf_queue *vq,
>  	struct soc_camera_host *ici =
>  		to_soc_camera_host(icd->parent);
>  	struct mx2_camera_dev *pcdev = ici->priv;
> +	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
>  	struct mx2_buffer *buf = container_of(vb, struct mx2_buffer, vb);
>  	unsigned long flags;
>  
> @@ -584,6 +585,26 @@ static void mx2_videobuf_queue(struct videobuf_queue *vq,
>  	list_add_tail(&vb->queue, &pcdev->capture);
>  
>  	if (mx27_camera_emma(pcdev)) {
> +		if (prp->cfg.channel == 1) {
> +			writel(PRP_CNTL_CH1EN |
> +				PRP_CNTL_CSIEN |
> +				prp->cfg.in_fmt |
> +				prp->cfg.out_fmt |
> +				PRP_CNTL_CH1_LEN |
> +				PRP_CNTL_CH1BYP |
> +				PRP_CNTL_CH1_TSKIP(0) |
> +				PRP_CNTL_IN_TSKIP(0),
> +				pcdev->base_emma + PRP_CNTL);
> +		} else {
> +			writel(PRP_CNTL_CH2EN |
> +				PRP_CNTL_CSIEN |
> +				prp->cfg.in_fmt |
> +				prp->cfg.out_fmt |
> +				PRP_CNTL_CH2_LEN |
> +				PRP_CNTL_CH2_TSKIP(0) |
> +				PRP_CNTL_IN_TSKIP(0),
> +				pcdev->base_emma + PRP_CNTL);
> +		}

Enabling the channel on each QBUF didn't seem like a good idea to me, so,
I looked a bit further. If you really want to be extremely careful to only
capture frames, when so requested by the user, don't you have to disable
channels upom STREAMOFF, i.e., when the last buffer is released by
.buf_release()? I don't think it makes sense to keep counting buffers, 
when not streaming - they are not really lost. So, wouldn't something like 
this not be better:

 	if (mx27_camera_emma(pcdev)) {
+		if (pcdev->frame_count < 0)
+			mx27_camera_emma_channel_enable(prp);

and then disable in .buf_release() if your queue is empty?

>  		goto out;

I think, this goto shall die, and with it the label too.

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
  

A small addendum

On Sun, 22 Jan 2012, Guennadi Liakhovetski wrote:

> Hi Javier
> 
> Please, excuse my curiosity and bear with my lack of understanding :-)
> 
> On Wed, 11 Jan 2012, Javier Martin wrote:
> 
> > As V4L2 specification states, frame_count must also
> > regard lost frames so that the user can handle that
> > case properly.
> > 
> > This patch adds a mechanism to increment the frame
> > counter even when a video buffer is not available
> > and a discard buffer is used.
> > 
> > ---
> > Changes since v1:
> >  - Initialize "frame_count" to -1 instead of using
> >    "firstirq" variable.
> > 
> > Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> > ---
> >  drivers/media/video/mx2_camera.c |   45 ++++++++++++++++++++-----------------
> >  1 files changed, 24 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> > index ca76dd2..68038e7 100644
> > --- a/drivers/media/video/mx2_camera.c
> > +++ b/drivers/media/video/mx2_camera.c
> > @@ -369,7 +369,7 @@ static int mx2_camera_add_device(struct soc_camera_device *icd)
> >  	writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
> >  
> >  	pcdev->icd = icd;
> > -	pcdev->frame_count = 0;
> > +	pcdev->frame_count = -1;
> 
> I'm adding a comment above this line:
> 
> +	/* Discard the first frame, begin valid frames with 0 */
> 
> >  
> >  	dev_info(icd->parent, "Camera driver attached to camera %d\n",
> >  		 icd->devnum);
> > @@ -572,6 +572,7 @@ static void mx2_videobuf_queue(struct videobuf_queue *vq,
> >  	struct soc_camera_host *ici =
> >  		to_soc_camera_host(icd->parent);
> >  	struct mx2_camera_dev *pcdev = ici->priv;
> > +	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
> >  	struct mx2_buffer *buf = container_of(vb, struct mx2_buffer, vb);
> >  	unsigned long flags;
> >  
> > @@ -584,6 +585,26 @@ static void mx2_videobuf_queue(struct videobuf_queue *vq,
> >  	list_add_tail(&vb->queue, &pcdev->capture);
> >  
> >  	if (mx27_camera_emma(pcdev)) {
> > +		if (prp->cfg.channel == 1) {
> > +			writel(PRP_CNTL_CH1EN |
> > +				PRP_CNTL_CSIEN |
> > +				prp->cfg.in_fmt |
> > +				prp->cfg.out_fmt |
> > +				PRP_CNTL_CH1_LEN |
> > +				PRP_CNTL_CH1BYP |
> > +				PRP_CNTL_CH1_TSKIP(0) |
> > +				PRP_CNTL_IN_TSKIP(0),
> > +				pcdev->base_emma + PRP_CNTL);
> > +		} else {
> > +			writel(PRP_CNTL_CH2EN |
> > +				PRP_CNTL_CSIEN |
> > +				prp->cfg.in_fmt |
> > +				prp->cfg.out_fmt |
> > +				PRP_CNTL_CH2_LEN |
> > +				PRP_CNTL_CH2_TSKIP(0) |
> > +				PRP_CNTL_IN_TSKIP(0),
> > +				pcdev->base_emma + PRP_CNTL);
> > +		}
> 
> Enabling the channel on each QBUF didn't seem like a good idea to me, so,
> I looked a bit further. If you really want to be extremely careful to only
> capture frames, when so requested by the user, don't you have to disable
> channels upom STREAMOFF, i.e., when the last buffer is released by
> .buf_release()? I don't think it makes sense to keep counting buffers, 
> when not streaming - they are not really lost. So, wouldn't something like 
> this not be better:
> 
>  	if (mx27_camera_emma(pcdev)) {
> +		if (pcdev->frame_count < 0)
> +			mx27_camera_emma_channel_enable(prp);
> 
> and then disable in .buf_release() if your queue is empty?

The condition "pcdev->frame_count < 0" is not going to work for you here. 
You have to enable on the first .buf_queue() after each STREAMON. You 
probably will need a new flag for this in your struct mx2_camera_dev.

Thanks
Guennadi

> 
> >  		goto out;
> 
> I think, this goto shall die, and with it the label too.

---
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,
thank you for your attention.

I've recently sent a new patch series on top of this patch:
[PATCH 0/4] media i.MX27 camera: fix buffer handling and videobuf2
support. (http://www.mail-archive.com/linux-media@vger.kernel.org/msg42255.html)

Among other things, it adds videobuf2 support and adds "stream_stop"
and "stream_start" callbacks which allow to enable/disable capturing
of buffers at the right moment.
This also makes the sequence number trick disappear and a much cleaner
approach is used instead.

I suggest you hold on this patch until the new series is accepted and
then merge both at the same time.

What do you think?
  

Hi Javier

On Mon, 23 Jan 2012, javier Martin wrote:

> Hi Guennadi,
> thank you for your attention.
> 
> I've recently sent a new patch series on top of this patch:
> [PATCH 0/4] media i.MX27 camera: fix buffer handling and videobuf2
> support. (http://www.mail-archive.com/linux-media@vger.kernel.org/msg42255.html)
> 
> Among other things, it adds videobuf2 support and adds "stream_stop"
> and "stream_start" callbacks which allow to enable/disable capturing
> of buffers at the right moment.
> This also makes the sequence number trick disappear and a much cleaner
> approach is used instead.
> 
> I suggest you hold on this patch until the new series is accepted and
> then merge both at the same time.
> 
> What do you think?

Ok, I'll be reviewing that patch series hopefully soon, and in principle 
it is good, that the buffer counting will really be fixed in it, but in an 
ideal world it would be better to have this your patch merged into patch 
2/4 of the series, agree? Would I be asking too much of you if I suggest 
that? Feel free to explain why this wouldn't work or just reject if you're 
just too tight on schedule. I'll see ifI can swallow it that way or maybe 
merge myself :-)

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
  

Hi Guennadi,

>> I suggest you hold on this patch until the new series is accepted and
>> then merge both at the same time.
>>
>> What do you think?
>
> Ok, I'll be reviewing that patch series hopefully soon, and in principle
> it is good, that the buffer counting will really be fixed in it, but in an
> ideal world it would be better to have this your patch merged into patch
> 2/4 of the series, agree? Would I be asking too much of you if I suggest
> that? Feel free to explain why this wouldn't work or just reject if you're
> just too tight on schedule. I'll see ifI can swallow it that way or maybe
> merge myself :-)

If you, Sascha, or someone else comes up with some requests or fixes
to the new series I don't mind to rebase it so you can just ignore
this patch, since I would have to sent a v2 version of the series
anyway.

Regards.