Hi Lad,
2015-01-23 6:18 GMT+08:00 Lad, Prabhakar <[email protected]>:
> this patch adds support to vb2_ioctl_* helpers.
>
> Signed-off-by: Lad, Prabhakar <[email protected]>
> ---
> 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;
>
>
On 02/03/15 09:27, Scott Jiang wrote:
> Hi Lad,
>
> 2015-01-23 6:18 GMT+08:00 Lad, Prabhakar <[email protected]>:
>> this patch adds support to vb2_ioctl_* helpers.
>>
>> Signed-off-by: Lad, Prabhakar <[email protected]>
>> ---
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>