Hi Javier
Thanks for the update! Let's see, whether this one can be improved a bit
more.
On Mon, 30 Jan 2012, Javier Martin wrote:
> The way discard buffer was previously handled lead
> to possible races that made a buffer that was not
> yet ready to be overwritten by new video data. This
> is easily detected at 25fps just adding "#define DEBUG"
> to enable the "memset" check and seeing how the image
> is corrupted.
>
> A new "discard" queue and two discard buffers have
> been added to make them flow trough the pipeline
> of queues and thus provide suitable event ordering.
>
> Signed-off-by: Javier Martin <javier.martin@xxxxxxxxxxxxxxxxx>
> ---
> Changes since v2:
> - Remove BUG_ON when active list is empty.
> - Replace empty list checks with warnings.
I think, the best would be to warn and bail out, instead of implicitly
crashing.
>
> ---
> drivers/media/video/mx2_camera.c | 280 +++++++++++++++++++++-----------------
> 1 files changed, 153 insertions(+), 127 deletions(-)
>
> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> index 35ab971..e7ccd97 100644
> --- a/drivers/media/video/mx2_camera.c
> +++ b/drivers/media/video/mx2_camera.c
[snip]
> @@ -706,8 +806,9 @@ static int mx2_stop_streaming(struct vb2_queue *q)
> unsigned long flags;
> u32 cntl;
>
> - spin_lock_irqsave(&pcdev->lock, flags);
> if (mx27_camera_emma(pcdev)) {
> + spin_lock_irqsave(&pcdev->lock, flags);
> +
> cntl = readl(pcdev->base_emma + PRP_CNTL);
> if (prp->cfg.channel == 1) {
> writel(cntl & ~PRP_CNTL_CH1EN,
> @@ -716,8 +817,18 @@ static int mx2_stop_streaming(struct vb2_queue *q)
> writel(cntl & ~PRP_CNTL_CH2EN,
> pcdev->base_emma + PRP_CNTL);
> }
> + INIT_LIST_HEAD(&pcdev->capture);
> + INIT_LIST_HEAD(&pcdev->active_bufs);
> + INIT_LIST_HEAD(&pcdev->discard);
> +
> + spin_unlock_irqrestore(&pcdev->lock, flags);
> +
> + dma_free_coherent(ici->v4l2_dev.dev,
> + pcdev->discard_size, pcdev->discard_buffer,
> + pcdev->discard_buffer_dma);
> + pcdev->discard_buffer = NULL;
AFAICS, the IRQ handler runs without taking any locks, so, there's a
theoretical SMP race here with using the discard buffers from the ISR. So,
I think, you'd have to add some locking to the ISR and here do something
like
+ x = pcdev->discard_buffer;
+ pcdev->discard_buffer = NULL;
+
+ spin_unlock_irqrestore(&pcdev->lock, flags);
+
+ dma_free_coherent(ici->v4l2_dev.dev,
+ pcdev->discard_size, x,
+ pcdev->discard_buffer_dma);
> }
> - spin_unlock_irqrestore(&pcdev->lock, flags);
> +
You're adding an empty line here.
>
> return 0;
> }
[snip]
> @@ -1179,18 +1212,23 @@ 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)
> {
This function is called from the ISR, so, I presume, you'll have to
spin_lock() somewhere here.
> - u32 imgsize = pcdev->icd->user_height * pcdev->icd->user_width;
> struct mx2_fmt_cfg *prp = pcdev->emma_prp;
> struct mx2_buffer *buf;
> struct vb2_buffer *vb;
> unsigned long phys;
>
> - if (!list_empty(&pcdev->active_bufs)) {
> - buf = list_entry(pcdev->active_bufs.next,
> - struct mx2_buffer, queue);
> + buf = list_entry(pcdev->active_bufs.next,
> + struct mx2_buffer, queue);
>
> - BUG_ON(buf->bufnum != bufnum);
> + BUG_ON(buf->bufnum != bufnum);
>
> + if (buf->discard) {
> + /*
> + * Discard buffer must not be returned to user space.
> + * Just return it to the discard queue.
> + */
> + list_move_tail(pcdev->active_bufs.next, &pcdev->discard);
> + } else {
> vb = &buf->vb;
> #ifdef DEBUG
> phys = vb2_dma_contig_plane_dma_addr(vb, 0);
> @@ -1212,6 +1250,7 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
> }
> }
> #endif
> +
> dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%p %lu\n", __func__, vb,
> vb2_plane_vaddr(vb, 0),
> vb2_get_plane_payload(vb, 0));
> @@ -1225,29 +1264,23 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
> pcdev->frame_count++;
>
> if (list_empty(&pcdev->capture)) {
> - if (prp->cfg.channel == 1) {
> - writel(pcdev->discard_buffer_dma, pcdev->base_emma +
> - PRP_DEST_RGB1_PTR + 4 * bufnum);
> - } else {
> - writel(pcdev->discard_buffer_dma, pcdev->base_emma +
> - PRP_DEST_Y_PTR -
> - 0x14 * bufnum);
> - if (prp->out_fmt == V4L2_PIX_FMT_YUV420) {
> - writel(pcdev->discard_buffer_dma + imgsize,
> - pcdev->base_emma + PRP_DEST_CB_PTR -
> - 0x14 * bufnum);
> - writel(pcdev->discard_buffer_dma +
> - ((5 * imgsize) / 4), pcdev->base_emma +
> - PRP_DEST_CR_PTR - 0x14 * bufnum);
> - }
> - }
> + if (list_empty(&pcdev->discard))
> + dev_warn(pcdev->dev, "%s: trying to access empty discard list\n",
> + __func__);
It is good, that you check for this error, but
> +
> + buf = list_entry(pcdev->discard.next,
> + struct mx2_buffer, queue);
> + buf->bufnum = bufnum;
> +
> + list_move_tail(pcdev->discard.next, &pcdev->active_bufs);
> + mx27_update_emma_buf(pcdev, pcdev->discard_buffer_dma, bufnum);
here even in the above error case you continue to access the invalid list
entry...
> return;
> }
>
> buf = list_entry(pcdev->capture.next,
> struct mx2_buffer, queue);
>
> - buf->bufnum = !bufnum;
> + buf->bufnum = bufnum;
>
> list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
>
> @@ -1255,18 +1288,7 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
> buf->state = MX2_STATE_ACTIVE;
>
> phys = vb2_dma_contig_plane_dma_addr(vb, 0);
> - if (prp->cfg.channel == 1) {
> - writel(phys, pcdev->base_emma + PRP_DEST_RGB1_PTR + 4 * bufnum);
> - } else {
> - writel(phys, pcdev->base_emma +
> - PRP_DEST_Y_PTR - 0x14 * bufnum);
> - if (prp->cfg.out_fmt == PRP_CNTL_CH2_OUT_YUV420) {
> - writel(phys + imgsize, pcdev->base_emma +
> - PRP_DEST_CB_PTR - 0x14 * bufnum);
> - writel(phys + ((5 * imgsize) / 4), pcdev->base_emma +
> - PRP_DEST_CR_PTR - 0x14 * bufnum);
> - }
> - }
> + mx27_update_emma_buf(pcdev, phys, bufnum);
> }
>
> static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
> @@ -1275,6 +1297,10 @@ static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
> unsigned int status = readl(pcdev->base_emma + PRP_INTRSTATUS);
> struct mx2_buffer *buf;
>
> + if (list_empty(&pcdev->active_bufs))
> + dev_warn(pcdev->dev, "%s: called while active list is empty\n",
> + __func__);
> +
Similarly here: if this is a possible condition, shouldn't you nicely bail
out here? Of course, interrupts have to be acked still.
> if (status & (1 << 7)) { /* overflow */
> u32 cntl;
> /*
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
