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