From mboxrd@z Thu Jan  1 00:00:00 1970
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Subject: Re: [PATCH v5 6/8] v4l: xilinx: Add Xilinx Video IP core
Date: Wed, 04 Mar 2015 00:15:55 +0200
Message-ID: <6955407.oAVS26tV3L@avalon>
References: <1425260925-12064-1-git-send-email-laurent.pinchart@ideasonboard.com> <1425260925-12064-7-git-send-email-laurent.pinchart@ideasonboard.com> <54F59AD8.6080903@xs4all.nl>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Return-path: <linux-media-owner@vger.kernel.org>
In-Reply-To: <54F59AD8.6080903@xs4all.nl>
Sender: linux-media-owner@vger.kernel.org
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: linux-media@vger.kernel.org, Michal Simek <michal.simek@xilinx.com>, Chris Kohn <christian.kohn@xilinx.com>, Hyun Kwon <hyun.kwon@xilinx.com>, devicetree@vger.kernel.org
List-Id: devicetree@vger.kernel.org

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@ideasonboard.com>
> > Signed-off-by: Hyun Kwon <hyun.kwon@xilinx.com>
> > Signed-off-by: Radhey Shyam Pandey <radheys@xilinx.com>
> > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > 
> > ---
> 
> <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