Re: [PATCH v4 3/4] media i.MX27 camera: improve discard buffer handling.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Guennadi,
I understand you are probably quite busy right now but it would be
great if you could ack this patch. The sooner you merge it the sooner
I will start working on the cleanup series. I've got some time on my
hands now.

Thank you.

On 7 February 2012 11:14, Javier Martin <javier.martin@xxxxxxxxxxxxxxxxx> 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>
> ---
>  - Add proper locking to avoid races between IRQ and stop callbacks.
>  - If a list is unexpectedly empty want the user and bail out.
>
> ---
>  drivers/media/video/mx2_camera.c |  287 +++++++++++++++++++++-----------------
>  1 files changed, 161 insertions(+), 126 deletions(-)
>
> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> index 35ab971..3880d24 100644
> --- a/drivers/media/video/mx2_camera.c
> +++ b/drivers/media/video/mx2_camera.c
> @@ -237,7 +237,8 @@ struct mx2_buffer {
>        struct list_head                queue;
>        enum mx2_buffer_state           state;
>
> -       int bufnum;
> +       int                             bufnum;
> +       bool                            discard;
>  };
>
>  struct mx2_camera_dev {
> @@ -256,6 +257,7 @@ struct mx2_camera_dev {
>
>        struct list_head        capture;
>        struct list_head        active_bufs;
> +       struct list_head        discard;
>
>        spinlock_t              lock;
>
> @@ -268,6 +270,7 @@ struct mx2_camera_dev {
>
>        u32                     csicr1;
>
> +       struct mx2_buffer       buf_discard[2];
>        void                    *discard_buffer;
>        dma_addr_t              discard_buffer_dma;
>        size_t                  discard_size;
> @@ -329,6 +332,29 @@ static struct mx2_fmt_cfg *mx27_emma_prp_get_format(
>        return &mx27_emma_prp_table[0];
>  };
>
> +static void mx27_update_emma_buf(struct mx2_camera_dev *pcdev,
> +                                unsigned long phys, int bufnum)
> +{
> +       struct mx2_fmt_cfg *prp = pcdev->emma_prp;
> +
> +       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->out_fmt == V4L2_PIX_FMT_YUV420) {
> +                       u32 imgsize = pcdev->icd->user_height *
> +                                       pcdev->icd->user_width;
> +
> +                       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);
> +               }
> +       }
> +}
> +
>  static void mx2_camera_deactivate(struct mx2_camera_dev *pcdev)
>  {
>        unsigned long flags;
> @@ -377,7 +403,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 = -1;
> +       pcdev->frame_count = 0;
>
>        dev_info(icd->parent, "Camera driver attached to camera %d\n",
>                 icd->devnum);
> @@ -397,13 +423,6 @@ static void mx2_camera_remove_device(struct soc_camera_device *icd)
>
>        mx2_camera_deactivate(pcdev);
>
> -       if (pcdev->discard_buffer) {
> -               dma_free_coherent(ici->v4l2_dev.dev, pcdev->discard_size,
> -                               pcdev->discard_buffer,
> -                               pcdev->discard_buffer_dma);
> -               pcdev->discard_buffer = NULL;
> -       }
> -
>        pcdev->icd = NULL;
>  }
>
> @@ -640,7 +659,6 @@ static void mx2_videobuf_release(struct vb2_buffer *vb)
>         */
>
>        spin_lock_irqsave(&pcdev->lock, flags);
> -       list_del_init(&buf->queue);
>        if (cpu_is_mx25() && buf->state == MX2_STATE_ACTIVE) {
>                if (pcdev->fb1_active == buf) {
>                        pcdev->csicr1 &= ~CSICR1_FB1_DMA_INTEN;
> @@ -656,6 +674,34 @@ static void mx2_videobuf_release(struct vb2_buffer *vb)
>        spin_unlock_irqrestore(&pcdev->lock, flags);
>  }
>
> +static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
> +               int bytesperline)
> +{
> +       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;
> +
> +       writel((icd->user_width << 16) | icd->user_height,
> +              pcdev->base_emma + PRP_SRC_FRAME_SIZE);
> +       writel(prp->cfg.src_pixel,
> +              pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
> +       if (prp->cfg.channel == 1) {
> +               writel((icd->user_width << 16) | icd->user_height,
> +                       pcdev->base_emma + PRP_CH1_OUT_IMAGE_SIZE);
> +               writel(bytesperline,
> +                       pcdev->base_emma + PRP_DEST_CH1_LINE_STRIDE);
> +               writel(prp->cfg.ch1_pixel,
> +                       pcdev->base_emma + PRP_CH1_PIXEL_FORMAT_CNTL);
> +       } else { /* channel 2 */
> +               writel((icd->user_width << 16) | icd->user_height,
> +                       pcdev->base_emma + PRP_CH2_OUT_IMAGE_SIZE);
> +       }
> +
> +       /* Enable interrupts */
> +       writel(prp->cfg.irq_flags, pcdev->base_emma + PRP_INTR_CNTL);
> +}
> +
>  static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
>  {
>        struct soc_camera_device *icd = soc_camera_from_vb2q(q);
> @@ -663,6 +709,10 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
>                to_soc_camera_host(icd->parent);
>        struct mx2_camera_dev *pcdev = ici->priv;
>        struct mx2_fmt_cfg *prp = pcdev->emma_prp;
> +       struct vb2_buffer *vb;
> +       struct mx2_buffer *buf;
> +       unsigned long phys;
> +       int bytesperline;
>
>        if (mx27_camera_emma(pcdev)) {
>                unsigned long flags;
> @@ -670,6 +720,56 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
>                        return -EINVAL;
>
>                spin_lock_irqsave(&pcdev->lock, flags);
> +
> +               buf = list_entry(pcdev->capture.next,
> +                                struct mx2_buffer, queue);
> +               buf->bufnum = 0;
> +               vb = &buf->vb;
> +               buf->state = MX2_STATE_ACTIVE;
> +
> +               phys = vb2_dma_contig_plane_dma_addr(vb, 0);
> +               mx27_update_emma_buf(pcdev, phys, buf->bufnum);
> +               list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
> +
> +               buf = list_entry(pcdev->capture.next,
> +                                struct mx2_buffer, queue);
> +               buf->bufnum = 1;
> +               vb = &buf->vb;
> +               buf->state = MX2_STATE_ACTIVE;
> +
> +               phys = vb2_dma_contig_plane_dma_addr(vb, 0);
> +               mx27_update_emma_buf(pcdev, phys, buf->bufnum);
> +               list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
> +
> +               bytesperline = soc_mbus_bytes_per_line(icd->user_width,
> +                               icd->current_fmt->host_fmt);
> +               if (bytesperline < 0)
> +                       return bytesperline;
> +
> +               /*
> +                * I didn't manage to properly enable/disable the prp
> +                * on a per frame basis during running transfers,
> +                * thus we allocate a buffer here and use it to
> +                * discard frames when no buffer is available.
> +                * Feel free to work on this ;)
> +                */
> +               pcdev->discard_size = icd->user_height * bytesperline;
> +               pcdev->discard_buffer = dma_alloc_coherent(ici->v4l2_dev.dev,
> +                               pcdev->discard_size, &pcdev->discard_buffer_dma,
> +                               GFP_KERNEL);
> +               if (!pcdev->discard_buffer)
> +                       return -ENOMEM;
> +
> +               pcdev->buf_discard[0].discard = true;
> +               list_add_tail(&pcdev->buf_discard[0].queue,
> +                                     &pcdev->discard);
> +
> +               pcdev->buf_discard[1].discard = true;
> +               list_add_tail(&pcdev->buf_discard[1].queue,
> +                                     &pcdev->discard);
> +
> +               mx27_camera_emma_buf_init(icd, bytesperline);
> +
>                if (prp->cfg.channel == 1) {
>                        writel(PRP_CNTL_CH1EN |
>                                PRP_CNTL_CSIEN |
> @@ -704,10 +804,12 @@ static int mx2_stop_streaming(struct vb2_queue *q)
>        struct mx2_camera_dev *pcdev = ici->priv;
>        struct mx2_fmt_cfg *prp = pcdev->emma_prp;
>        unsigned long flags;
> +       void *b;
>        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 +818,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);
> +
> +               b = pcdev->discard_buffer;
> +               pcdev->discard_buffer = NULL;
> +
> +               spin_unlock_irqrestore(&pcdev->lock, flags);
> +
> +               dma_free_coherent(ici->v4l2_dev.dev,
> +                       pcdev->discard_size, b, pcdev->discard_buffer_dma);
>        }
> -       spin_unlock_irqrestore(&pcdev->lock, flags);
>
>        return 0;
>  }
> @@ -771,63 +883,6 @@ static int mx27_camera_emma_prp_reset(struct mx2_camera_dev *pcdev)
>        return -ETIMEDOUT;
>  }
>
> -static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
> -               int bytesperline)
> -{
> -       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;
> -       u32 imgsize = pcdev->icd->user_height * pcdev->icd->user_width;
> -
> -       if (prp->cfg.channel == 1) {
> -               writel(pcdev->discard_buffer_dma,
> -                               pcdev->base_emma + PRP_DEST_RGB1_PTR);
> -               writel(pcdev->discard_buffer_dma,
> -                               pcdev->base_emma + PRP_DEST_RGB2_PTR);
> -
> -               writel((icd->user_width << 16) | icd->user_height,
> -                       pcdev->base_emma + PRP_SRC_FRAME_SIZE);
> -               writel((icd->user_width << 16) | icd->user_height,
> -                       pcdev->base_emma + PRP_CH1_OUT_IMAGE_SIZE);
> -               writel(bytesperline,
> -                       pcdev->base_emma + PRP_DEST_CH1_LINE_STRIDE);
> -               writel(prp->cfg.src_pixel,
> -                       pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
> -               writel(prp->cfg.ch1_pixel,
> -                       pcdev->base_emma + PRP_CH1_PIXEL_FORMAT_CNTL);
> -       } else { /* channel 2 */
> -               writel(pcdev->discard_buffer_dma,
> -                       pcdev->base_emma + PRP_DEST_Y_PTR);
> -               writel(pcdev->discard_buffer_dma,
> -                       pcdev->base_emma + PRP_SOURCE_Y_PTR);
> -
> -               if (prp->cfg.out_fmt == PRP_CNTL_CH2_OUT_YUV420) {
> -                       writel(pcdev->discard_buffer_dma + imgsize,
> -                               pcdev->base_emma + PRP_DEST_CB_PTR);
> -                       writel(pcdev->discard_buffer_dma + ((5 * imgsize) / 4),
> -                               pcdev->base_emma + PRP_DEST_CR_PTR);
> -                       writel(pcdev->discard_buffer_dma + imgsize,
> -                               pcdev->base_emma + PRP_SOURCE_CB_PTR);
> -                       writel(pcdev->discard_buffer_dma + ((5 * imgsize) / 4),
> -                               pcdev->base_emma + PRP_SOURCE_CR_PTR);
> -               }
> -
> -               writel((icd->user_width << 16) | icd->user_height,
> -                       pcdev->base_emma + PRP_SRC_FRAME_SIZE);
> -
> -               writel((icd->user_width << 16) | icd->user_height,
> -                       pcdev->base_emma + PRP_CH2_OUT_IMAGE_SIZE);
> -
> -               writel(prp->cfg.src_pixel,
> -                       pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
> -
> -       }
> -
> -       /* Enable interrupts */
> -       writel(prp->cfg.irq_flags, pcdev->base_emma + PRP_INTR_CNTL);
> -}
> -
>  static int mx2_camera_set_bus_param(struct soc_camera_device *icd,
>                __u32 pixfmt)
>  {
> @@ -911,27 +966,6 @@ static int mx2_camera_set_bus_param(struct soc_camera_device *icd,
>                ret = mx27_camera_emma_prp_reset(pcdev);
>                if (ret)
>                        return ret;
> -
> -               if (pcdev->discard_buffer)
> -                       dma_free_coherent(ici->v4l2_dev.dev,
> -                               pcdev->discard_size, pcdev->discard_buffer,
> -                               pcdev->discard_buffer_dma);
> -
> -               /*
> -                * I didn't manage to properly enable/disable the prp
> -                * on a per frame basis during running transfers,
> -                * thus we allocate a buffer here and use it to
> -                * discard frames when no buffer is available.
> -                * Feel free to work on this ;)
> -                */
> -               pcdev->discard_size = icd->user_height * bytesperline;
> -               pcdev->discard_buffer = dma_alloc_coherent(ici->v4l2_dev.dev,
> -                               pcdev->discard_size, &pcdev->discard_buffer_dma,
> -                               GFP_KERNEL);
> -               if (!pcdev->discard_buffer)
> -                       return -ENOMEM;
> -
> -               mx27_camera_emma_buf_init(icd, bytesperline);
>        } else if (cpu_is_mx25()) {
>                writel((bytesperline * icd->user_height) >> 2,
>                                pcdev->base_csi + CSIRXCNT);
> @@ -1179,18 +1213,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)
>  {
> -       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);
> @@ -1225,29 +1264,25 @@ 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__);
> +                       return;
>                }
> +
> +               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);
>                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 +1290,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)
> @@ -1274,6 +1298,15 @@ static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
>        struct mx2_camera_dev *pcdev = data;
>        unsigned int status = readl(pcdev->base_emma + PRP_INTRSTATUS);
>        struct mx2_buffer *buf;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&pcdev->lock, flags);
> +
> +       if (list_empty(&pcdev->active_bufs)) {
> +               dev_warn(pcdev->dev, "%s: called while active list is empty\n",
> +                       __func__);
> +               goto irq_ok;
> +       }
>
>        if (status & (1 << 7)) { /* overflow */
>                u32 cntl;
> @@ -1290,9 +1323,8 @@ static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
>                       pcdev->base_emma + PRP_CNTL);
>                writel(cntl, pcdev->base_emma + PRP_CNTL);
>        }
> -       if ((((status & (3 << 5)) == (3 << 5)) ||
> -               ((status & (3 << 3)) == (3 << 3)))
> -                       && !list_empty(&pcdev->active_bufs)) {
> +       if (((status & (3 << 5)) == (3 << 5)) ||
> +               ((status & (3 << 3)) == (3 << 3))) {
>                /*
>                 * Both buffers have triggered, process the one we're expecting
>                 * to first
> @@ -1307,6 +1339,8 @@ static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
>        if ((status & (1 << 5)) || (status & (1 << 3)))
>                mx27_camera_frame_done_emma(pcdev, 1);
>
> +irq_ok:
> +       spin_unlock_irqrestore(&pcdev->lock, flags);
>        writel(status, pcdev->base_emma + PRP_INTRSTATUS);
>
>        return IRQ_HANDLED;
> @@ -1418,6 +1452,7 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev)
>
>        INIT_LIST_HEAD(&pcdev->capture);
>        INIT_LIST_HEAD(&pcdev->active_bufs);
> +       INIT_LIST_HEAD(&pcdev->discard);
>        spin_lock_init(&pcdev->lock);
>
>        /*
> --
> 1.7.0.4
>



-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com
--
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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux