Hi Hans,
On Wednesday 04 March 2015 10:30:40 Hans Verkuil wrote:
> On 03/03/15 23:15, Laurent Pinchart wrote:
> > On Tuesday 03 March 2015 12:28:24 Hans Verkuil wrote:
> >> 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 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.
>
> Would it make sense to add a validate_streamon op to vb2? For devices like
> this that need to validate the pipeline it makes sense that the validation
> happens on VIDIOC_STREAMON. The actual DMA engine start stays with the
> start_streaming op.
It might be useful, but maybe we should wait until we get a couple more
drivers with similar requirements before implementing it.
For this particular driver I don't see what it would bring though, I don't
think there's any particular hack to work around the problem on the driver
side.
> It would be easy to add to vb2.
>
> >>> +
> >>> + /* 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.
>
> Good point. But perhaps it is a good idea to add a comment why
> VB2_READ/WRITE isn't supported, if only to prevent me from asking this
> again in the future :-)
Will do.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
