| Message ID | 1421965128-10470-9-git-send-email-prabhakar.csengg@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Lad, 2015-01-23 6:18 GMT+08:00 Lad, Prabhakar <prabhakar.csengg@gmail.com>: > this patch adds support to vb2_ioctl_* helpers. > > Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com> > --- > drivers/media/platform/blackfin/bfin_capture.c | 108 ++++++------------------- > 1 file changed, 23 insertions(+), 85 deletions(-) > > diff --git a/drivers/media/platform/blackfin/bfin_capture.c b/drivers/media/platform/blackfin/bfin_capture.c > index b2eeace..04b85e3 100644 > --- a/drivers/media/platform/blackfin/bfin_capture.c > +++ b/drivers/media/platform/blackfin/bfin_capture.c > @@ -272,15 +272,26 @@ static int bcap_start_streaming(struct vb2_queue *vq, unsigned int count) > struct ppi_if *ppi = bcap_dev->ppi; > struct bcap_buffer *buf, *tmp; > struct ppi_params params; > + dma_addr_t addr; > int ret; > > /* enable streamon on the sub device */ > ret = v4l2_subdev_call(bcap_dev->sd, video, s_stream, 1); > if (ret && (ret != -ENOIOCTLCMD)) { > v4l2_err(&bcap_dev->v4l2_dev, "stream on failed in subdev\n"); > + bcap_dev->cur_frm = NULL; > goto err; > } > > + /* get the next frame from the dma queue */ > + bcap_dev->cur_frm = list_entry(bcap_dev->dma_queue.next, > + struct bcap_buffer, list); > + /* remove buffer from the dma queue */ > + list_del_init(&bcap_dev->cur_frm->list); > + addr = vb2_dma_contig_plane_dma_addr(&bcap_dev->cur_frm->vb, 0); > + /* update DMA address */ > + ppi->ops->update_addr(ppi, (unsigned long)addr); > + > /* set ppi params */ > params.width = bcap_dev->fmt.width; > params.height = bcap_dev->fmt.height; > @@ -320,6 +331,9 @@ static int bcap_start_streaming(struct vb2_queue *vq, unsigned int count) > goto err; > } > > + /* enable ppi */ > + ppi->ops->start(ppi); > + Still wrong here. You can't start ppi before request dma and irq. Also it's not good to update dma address before request dma. Please strictly follow the initial sequence in bcap_streamon() because the order is important. That means you should put all functions in bcap_start_streaming() before those in bcap_streamon(). And it seems you removed dma buffer check in bcap_streamon(). Yes, in vb2_internal_streamon() it will check q->queued_count >= q->min_buffers_needed to start streaming. But if the user doesn't queue enough buffer, it will return success and set q->streaming = 1. Is it really right here? > /* attach ppi DMA irq handler */ > ret = ppi->ops->attach_irq(ppi, bcap_isr); > if (ret < 0) { > @@ -334,6 +348,9 @@ static int bcap_start_streaming(struct vb2_queue *vq, unsigned int count) > return 0; > > -- 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
On 02/03/15 09:27, Scott Jiang wrote: > Hi Lad, > > 2015-01-23 6:18 GMT+08:00 Lad, Prabhakar <prabhakar.csengg@gmail.com>: >> this patch adds support to vb2_ioctl_* helpers. >> >> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com> >> --- >> drivers/media/platform/blackfin/bfin_capture.c | 108 ++++++------------------- >> 1 file changed, 23 insertions(+), 85 deletions(-) >> >> diff --git a/drivers/media/platform/blackfin/bfin_capture.c b/drivers/media/platform/blackfin/bfin_capture.c >> index b2eeace..04b85e3 100644 >> --- a/drivers/media/platform/blackfin/bfin_capture.c >> +++ b/drivers/media/platform/blackfin/bfin_capture.c >> @@ -272,15 +272,26 @@ static int bcap_start_streaming(struct vb2_queue *vq, unsigned int count) >> struct ppi_if *ppi = bcap_dev->ppi; >> struct bcap_buffer *buf, *tmp; >> struct ppi_params params; >> + dma_addr_t addr; >> int ret; >> >> /* enable streamon on the sub device */ >> ret = v4l2_subdev_call(bcap_dev->sd, video, s_stream, 1); >> if (ret && (ret != -ENOIOCTLCMD)) { >> v4l2_err(&bcap_dev->v4l2_dev, "stream on failed in subdev\n"); >> + bcap_dev->cur_frm = NULL; >> goto err; >> } >> >> + /* get the next frame from the dma queue */ >> + bcap_dev->cur_frm = list_entry(bcap_dev->dma_queue.next, >> + struct bcap_buffer, list); >> + /* remove buffer from the dma queue */ >> + list_del_init(&bcap_dev->cur_frm->list); >> + addr = vb2_dma_contig_plane_dma_addr(&bcap_dev->cur_frm->vb, 0); >> + /* update DMA address */ >> + ppi->ops->update_addr(ppi, (unsigned long)addr); >> + >> /* set ppi params */ >> params.width = bcap_dev->fmt.width; >> params.height = bcap_dev->fmt.height; >> @@ -320,6 +331,9 @@ static int bcap_start_streaming(struct vb2_queue *vq, unsigned int count) >> goto err; >> } >> >> + /* enable ppi */ >> + ppi->ops->start(ppi); >> + > Still wrong here. You can't start ppi before request dma and irq. Also > it's not good to update dma address before request dma. Please > strictly follow the initial sequence in bcap_streamon() because the > order is important. That means you should put all functions in > bcap_start_streaming() before those in bcap_streamon(). > And it seems you removed dma buffer check in bcap_streamon(). Yes, in > vb2_internal_streamon() it will check q->queued_count >= > q->min_buffers_needed to start streaming. But if the user doesn't > queue enough buffer, it will return success and set q->streaming = 1. > Is it really right here? Yes, that's really right. The V4L2 state is set to streaming after calling VIDIOC_STREAMON, even if the DMA engine hasn't started yet. That's set with the start_streaming_called bitfield. Regards, Hans > >> /* attach ppi DMA irq handler */ >> ret = ppi->ops->attach_irq(ppi, bcap_isr); >> if (ret < 0) { >> @@ -334,6 +348,9 @@ static int bcap_start_streaming(struct vb2_queue *vq, unsigned int count) >> return 0; >> >> > -- > 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 > -- 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
diff --git a/drivers/media/platform/blackfin/bfin_capture.c b/drivers/media/platform/blackfin/bfin_capture.c index b2eeace..04b85e3 100644 --- a/drivers/media/platform/blackfin/bfin_capture.c +++ b/drivers/media/platform/blackfin/bfin_capture.c @@ -272,15 +272,26 @@ static int bcap_start_streaming(struct vb2_queue *vq, unsigned int count) struct ppi_if *ppi = bcap_dev->ppi; struct bcap_buffer *buf, *tmp; struct ppi_params params; + dma_addr_t addr; int ret; /* enable streamon on the sub device */ ret = v4l2_subdev_call(bcap_dev->sd, video, s_stream, 1); if (ret && (ret != -ENOIOCTLCMD)) { v4l2_err(&bcap_dev->v4l2_dev, "stream on failed in subdev\n"); + bcap_dev->cur_frm = NULL; goto err; } + /* get the next frame from the dma queue */ + bcap_dev->cur_frm = list_entry(bcap_dev->dma_queue.next, + struct bcap_buffer, list); + /* remove buffer from the dma queue */ + list_del_init(&bcap_dev->cur_frm->list); + addr = vb2_dma_contig_plane_dma_addr(&bcap_dev->cur_frm->vb, 0); + /* update DMA address */ + ppi->ops->update_addr(ppi, (unsigned long)addr); + /* set ppi params */ params.width = bcap_dev->fmt.width; params.height = bcap_dev->fmt.height; @@ -320,6 +331,9 @@ static int bcap_start_streaming(struct vb2_queue *vq, unsigned int count) goto err; } + /* enable ppi */ + ppi->ops->start(ppi); + /* attach ppi DMA irq handler */ ret = ppi->ops->attach_irq(ppi, bcap_isr); if (ret < 0) { @@ -334,6 +348,9 @@ static int bcap_start_streaming(struct vb2_queue *vq, unsigned int count) return 0; err: + if (bcap_dev->cur_frm) + vb2_buffer_done(&bcap_dev->cur_frm->vb, VB2_BUF_STATE_QUEUED); + list_for_each_entry_safe(buf, tmp, &bcap_dev->dma_queue, list) { list_del(&buf->list); vb2_buffer_done(&buf->vb, VB2_BUF_STATE_QUEUED); @@ -377,40 +394,6 @@ static struct vb2_ops bcap_video_qops = { .stop_streaming = bcap_stop_streaming, }; -static int bcap_reqbufs(struct file *file, void *priv, - struct v4l2_requestbuffers *req_buf) -{ - struct bcap_device *bcap_dev = video_drvdata(file); - struct vb2_queue *vq = &bcap_dev->buffer_queue; - - return vb2_reqbufs(vq, req_buf); -} - -static int bcap_querybuf(struct file *file, void *priv, - struct v4l2_buffer *buf) -{ - struct bcap_device *bcap_dev = video_drvdata(file); - - return vb2_querybuf(&bcap_dev->buffer_queue, buf); -} - -static int bcap_qbuf(struct file *file, void *priv, - struct v4l2_buffer *buf) -{ - struct bcap_device *bcap_dev = video_drvdata(file); - - return vb2_qbuf(&bcap_dev->buffer_queue, buf); -} - -static int bcap_dqbuf(struct file *file, void *priv, - struct v4l2_buffer *buf) -{ - struct bcap_device *bcap_dev = video_drvdata(file); - - return vb2_dqbuf(&bcap_dev->buffer_queue, - buf, file->f_flags & O_NONBLOCK); -} - static irqreturn_t bcap_isr(int irq, void *dev_id) { struct ppi_if *ppi = dev_id; @@ -452,51 +435,6 @@ static irqreturn_t bcap_isr(int irq, void *dev_id) return IRQ_HANDLED; } -static int bcap_streamon(struct file *file, void *priv, - enum v4l2_buf_type buf_type) -{ - struct bcap_device *bcap_dev = video_drvdata(file); - struct ppi_if *ppi = bcap_dev->ppi; - dma_addr_t addr; - int ret; - - /* call streamon to start streaming in videobuf */ - ret = vb2_streamon(&bcap_dev->buffer_queue, buf_type); - if (ret) - return ret; - - /* if dma queue is empty, return error */ - if (list_empty(&bcap_dev->dma_queue)) { - v4l2_err(&bcap_dev->v4l2_dev, "dma queue is empty\n"); - ret = -EINVAL; - goto err; - } - - /* get the next frame from the dma queue */ - bcap_dev->cur_frm = list_entry(bcap_dev->dma_queue.next, - struct bcap_buffer, list); - /* remove buffer from the dma queue */ - list_del_init(&bcap_dev->cur_frm->list); - addr = vb2_dma_contig_plane_dma_addr(&bcap_dev->cur_frm->vb, 0); - /* update DMA address */ - ppi->ops->update_addr(ppi, (unsigned long)addr); - /* enable ppi */ - ppi->ops->start(ppi); - - return 0; -err: - vb2_streamoff(&bcap_dev->buffer_queue, buf_type); - return ret; -} - -static int bcap_streamoff(struct file *file, void *priv, - enum v4l2_buf_type buf_type) -{ - struct bcap_device *bcap_dev = video_drvdata(file); - - return vb2_streamoff(&bcap_dev->buffer_queue, buf_type); -} - static int bcap_querystd(struct file *file, void *priv, v4l2_std_id *std) { struct bcap_device *bcap_dev = video_drvdata(file); @@ -782,12 +720,12 @@ static const struct v4l2_ioctl_ops bcap_ioctl_ops = { .vidioc_g_dv_timings = bcap_g_dv_timings, .vidioc_query_dv_timings = bcap_query_dv_timings, .vidioc_enum_dv_timings = bcap_enum_dv_timings, - .vidioc_reqbufs = bcap_reqbufs, - .vidioc_querybuf = bcap_querybuf, - .vidioc_qbuf = bcap_qbuf, - .vidioc_dqbuf = bcap_dqbuf, - .vidioc_streamon = bcap_streamon, - .vidioc_streamoff = bcap_streamoff, + .vidioc_reqbufs = vb2_ioctl_reqbufs, + .vidioc_querybuf = vb2_ioctl_querybuf, + .vidioc_qbuf = vb2_ioctl_qbuf, + .vidioc_dqbuf = vb2_ioctl_dqbuf, + .vidioc_streamon = vb2_ioctl_streamon, + .vidioc_streamoff = vb2_ioctl_streamoff, .vidioc_g_parm = bcap_g_parm, .vidioc_s_parm = bcap_s_parm, .vidioc_log_status = bcap_log_status,
this patch adds support to vb2_ioctl_* helpers. Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com> --- drivers/media/platform/blackfin/bfin_capture.c | 108 ++++++------------------- 1 file changed, 23 insertions(+), 85 deletions(-)