Hi Javier
On Mon, 2 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.
>
> Signed-off-by: Javier Martin <javier.martin@xxxxxxxxxxxxxxxxx>
> ---
> drivers/media/video/mx2_camera.c | 54 ++++++++++++++++++++++++--------------
> 1 files changed, 34 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> index ca76dd2..b244714 100644
> --- a/drivers/media/video/mx2_camera.c
> +++ b/drivers/media/video/mx2_camera.c
> @@ -256,6 +256,7 @@ struct mx2_camera_dev {
> size_t discard_size;
> struct mx2_fmt_cfg *emma_prp;
> u32 frame_count;
> + unsigned int firstirq;
_if_ we indeed end up using this field, it seems it can be just a bool.
> };
>
> /* buffer for one video frame */
> @@ -370,6 +371,7 @@ static int mx2_camera_add_device(struct soc_camera_device *icd)
>
> pcdev->icd = icd;
> pcdev->frame_count = 0;
> + pcdev->firstirq = 1;
>
> dev_info(icd->parent, "Camera driver attached to camera %d\n",
> icd->devnum);
> @@ -572,6 +574,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 +587,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);
> + }
Is this related and why is this needed?
> goto out;
> } else { /* cpu_is_mx25() */
> u32 csicr3, dma_inten = 0;
> @@ -747,16 +770,6 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
> writel(pcdev->discard_buffer_dma,
> pcdev->base_emma + PRP_DEST_RGB2_PTR);
>
> - 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);
> -
> writel((icd->user_width << 16) | icd->user_height,
> pcdev->base_emma + PRP_SRC_FRAME_SIZE);
> writel((icd->user_width << 16) | icd->user_height,
> @@ -784,15 +797,6 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
> pcdev->base_emma + PRP_SOURCE_CR_PTR);
> }
>
> - 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);
> -
> writel((icd->user_width << 16) | icd->user_height,
> pcdev->base_emma + PRP_SRC_FRAME_SIZE);
>
> @@ -1214,7 +1218,6 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
> vb->state = state;
> do_gettimeofday(&vb->ts);
> vb->field_count = pcdev->frame_count * 2;
> - pcdev->frame_count++;
>
> wake_up(&vb->done);
> }
Wouldn't you achieve the same by simply initialising frame_count to -1 and
incrementing it unconditionally just below this if?
> @@ -1239,6 +1242,17 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
> return;
> }
>
> + /*
> + * According to V4L2 specification, first valid sequence number must
> + * be 0. However, by design the first received frame is written to the
> + * discard buffer even when a video buffer is available. For that reason
> + * we don't increment frame_count the first time.
> + */
> + if (pcdev->firstirq)
> + pcdev->firstirq = 0;
> + else
> + pcdev->frame_count++;
> +
> buf = list_entry(pcdev->capture.next,
> struct mx2_buffer, vb.queue);
Aren't you losing frames, that you receive, while the capture queue is
empty? Whereas the above proposed approach should catch them all, I think.
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
