Hi Hans,
Thank you for the review.
On Tuesday 03 March 2015 12:28:24 Hans Verkuil wrote:
> Hi Laurent,
>
> Thanks for this patch. I do have a few comments, see below. Note that I am
> OK with the new DT format description.
>
> On 03/02/2015 02:48 AM, Laurent Pinchart wrote:
> > Xilinx platforms have no hardwired video capture or video processing
> > interface. Users create capture and memory to memory processing
> > pipelines in the FPGA fabric to suit their particular needs, by
> > instantiating video IP cores from a large library.
> >
> > The Xilinx Video IP core is a framework that models a video pipeline
> > described in the device tree and expose the pipeline to userspace
> > through the media controller and V4L2 APIs.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Hyun Kwon <hyun.kwon@xxxxxxxxxx>
> > Signed-off-by: Radhey Shyam Pandey <radheys@xxxxxxxxxx>
> > Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
> >
> > ---
>
> <snip>
>
> > diff --git a/drivers/media/platform/xilinx/xilinx-dma.c
> > b/drivers/media/platform/xilinx/xilinx-dma.c new file mode 100644
> > index 0000000..afed6c3
> > --- /dev/null
> > +++ b/drivers/media/platform/xilinx/xilinx-dma.c
> > @@ -0,0 +1,753 @@
[snip]
> > +static void xvip_dma_complete(void *param)
> > +{
> > + struct xvip_dma_buffer *buf = param;
> > + struct xvip_dma *dma = buf->dma;
> > +
> > + spin_lock(&dma->queued_lock);
> > + list_del(&buf->queue);
> > + spin_unlock(&dma->queued_lock);
> > +
> > + buf->buf.v4l2_buf.sequence = dma->sequence++;
>
> buf->buf.v4l2_buf.field isn't set. I think you only support progressive
> formats at the moment, so this should be set to V4L2_FIELD_NONE.
Agreed, that was an oversight. I'll fix it.
> > + v4l2_get_timestamp(&buf->buf.v4l2_buf.timestamp);
> > + vb2_set_plane_payload(&buf->buf, 0, dma->format.sizeimage);
> > + vb2_buffer_done(&buf->buf, VB2_BUF_STATE_DONE);
> > +}
> > +
> > +static int
> > +xvip_dma_queue_setup(struct vb2_queue *vq, const struct v4l2_format *fmt,
> > + unsigned int *nbuffers, unsigned int *nplanes,
> > + unsigned int sizes[], void *alloc_ctxs[])
> > +{
> > + struct xvip_dma *dma = vb2_get_drv_priv(vq);
> > +
> > + *nplanes = 1;
> > +
> > + sizes[0] = dma->format.sizeimage;
>
> I would suggest that you add support for vb2_ioctl_create_bufs by changing
> this code to:
>
> if (fmt && fmt->fmt.pix.sizeimage < dma->format.sizeimage)
> return -EINVAL;
> sizes[0] = fmt ? fmt->fmt.pix.sizeimage : dma->format.sizeimage;
Looks good, I'll fix that.
> > + alloc_ctxs[0] = dma->alloc_ctx;
> > +
> > + return 0;
> > +}
[snip]
> > +static int xvip_dma_start_streaming(struct vb2_queue *vq, unsigned int
> > count) +{
> > + struct xvip_dma *dma = vb2_get_drv_priv(vq);
> > + struct xvip_dma_buffer *buf, *nbuf;
> > + struct xvip_pipeline *pipe;
> > + int ret;
> > +
> > + dma->sequence = 0;
> > +
> > + /*
> > + * Start streaming on the pipeline. No link touching an entity in the
> > + * pipeline can be activated or deactivated once streaming is
started.
> > + *
> > + * Use the pipeline object embedded in the first DMA object that
starts
> > + * streaming.
> > + */
> > + pipe = dma->video.entity.pipe
> > + ? to_xvip_pipeline(&dma->video.entity) : &dma->pipe;
> > +
> > + ret = media_entity_pipeline_start(&dma->video.entity, &pipe->pipe);
> > + if (ret < 0)
> > + goto error;
> > +
> > + /* Verify that the configured format matches the output of the
> > + * connected subdev.
> > + */
> > + ret = xvip_dma_verify_format(dma);
> > + if (ret < 0)
> > + goto error_stop;
> > +
> > + ret = xvip_pipeline_prepare(pipe, dma);
> > + if (ret < 0)
> > + goto error_stop;
> > +
> > + /* Start the DMA engine. This must be done before starting the blocks
> > + * in the pipeline to avoid DMA synchronization issues.
> > + */
> > + dma_async_issue_pending(dma->dma);
>
> Question: can the DMA engine be started without any buffers queued?
Yes. In that case the dma_async_issue_pending() call will be a no-op, as there
will be no pending DMA transfer queued.
> The vb2_queue struct has a min_buffers_needed field that can be set to a
> non-zero value. In that case start_streaming won't be called until at least
> that many buffers have been queued. Many DMA engines need that so this was
> added to the vb2 core to avoid having to hack around this in the driver.
I don't see a need for that here. I actually think the min_buffers_needed
field shouldn't be set, even if it could be set to 1, to avoid reporting
VIDIOC_STREAMON errors at VIDIOC_QBUF time. The alternative would be to move
the validation code to a custom .video_streamon handler, but that seems
pointless to me.
> > +
> > + /* Start the pipeline. */
> > + xvip_pipeline_set_stream(pipe, true);
> > +
> > + return 0;
> > +
> > +error_stop:
> > + media_entity_pipeline_stop(&dma->video.entity);
> > +
> > +error:
> > + /* Give back all queued buffers to videobuf2. */
> > + spin_lock_irq(&dma->queued_lock);
> > + list_for_each_entry_safe(buf, nbuf, &dma->queued_bufs, queue) {
> > + vb2_buffer_done(&buf->buf, VB2_BUF_STATE_QUEUED);
> > + list_del(&buf->queue);
> > + }
> > + spin_unlock_irq(&dma->queued_lock);
> > +
> > + return ret;
> > +}
[snip]
> > +/* ----------------------------------------------------------------------
> > + * V4L2 file operations
> > + */
> > +
> > +static const struct v4l2_file_operations xvip_dma_fops = {
> > + .owner = THIS_MODULE,
> > + .unlocked_ioctl = video_ioctl2,
> > + .open = v4l2_fh_open,
> > + .release = vb2_fop_release,
> > + .poll = vb2_fop_poll,
> > + .mmap = vb2_fop_mmap,
>
> I would also add:
>
> .read = vb2_fop_read,
> .write = vb2_fop_write,
>
> and add VB2_READ or VB2_WRITE to dma->queue.io_modes.
>
> You get it for free, it doesn't take any additional resources, so why not?
My usual comment : because I'd rather not have users using the read() API with
this driver :-) The usual argument of "but then it would be easy to just cat
/dev/video? to check whether the device works" doesn't apply here as the
pipeline needs to be configured from userspace through V4L2 subdev pad ops
anyway, so users can as well use a proper V4L2 command line test tool.
> However, to make that work correctly you need this patch:
>
> https://patchwork.linuxtv.org/patch/28478/
>
> It would make sense if you just add that patch to your xilinx tree.
--
Regards,
Laurent Pinchart
--
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
