Re: [PATCH v4 09/10] v4l: xilinx: dma: Add multi-planar support

[Hyun Kwon <hyun.kwon@xilinx.com>]

On Mon, 2018-04-30 at 18:35:12 -0700, Satish Kumar Nagireddy wrote:
> The current v4l driver supports single plane formats. This patch
> adds support to handle multi-planar formats. Driver can handle
> both single and multi-planar formats.
> 
> Signed-off-by: Satish Kumar Nagireddy <satish.nagireddy.nagireddy@xilinx.com>
> ---
>  drivers/media/platform/xilinx/xilinx-dma.c  | 159 ++++++++++++++++++----------
>  drivers/media/platform/xilinx/xilinx-dma.h  |   4 +-
>  drivers/media/platform/xilinx/xilinx-vipp.c |  16 +--
>  3 files changed, 111 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c
> index 658586e..a714057 100644
> --- a/drivers/media/platform/xilinx/xilinx-dma.c
> +++ b/drivers/media/platform/xilinx/xilinx-dma.c
> @@ -74,8 +74,8 @@ static int xvip_dma_verify_format(struct xvip_dma *dma)
>  		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
>  
>  	if (dma->fmtinfo->code != fmt.format.code ||
> -	    dma->format.height != fmt.format.height ||
> -	    dma->format.width != fmt.format.width)
> +	    dma->format.fmt.pix_mp.width != fmt.format.width ||
> +	    dma->format.fmt.pix_mp.height != fmt.format.height)
>  		return -EINVAL;
>  
>  	return 0;
> @@ -310,7 +310,8 @@ static void xvip_dma_complete(void *param)
>  	buf->buf.field = V4L2_FIELD_NONE;
>  	buf->buf.sequence = dma->sequence++;
>  	buf->buf.vb2_buf.timestamp = ktime_get_ns();
> -	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, dma->format.sizeimage);
> +	vb2_set_plane_payload(&buf->buf.vb2_buf, 0,
> +			      dma->format.fmt.pix_mp.plane_fmt[0].sizeimage);
>  	vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
>  }
>  
> @@ -320,13 +321,15 @@ xvip_dma_queue_setup(struct vb2_queue *vq,
>  		     unsigned int sizes[], struct device *alloc_devs[])
>  {
>  	struct xvip_dma *dma = vb2_get_drv_priv(vq);
> +	s32 sizeimage;

u32?

>  
>  	/* Make sure the image size is large enough. */
> +	sizeimage = dma->format.fmt.pix_mp.plane_fmt[0].sizeimage;

I'm a little confused again. :-) This doesn't seem handling nplanes > 1,
while there are such formats in the format table. is my understanding
correct?

>  	if (*nplanes)
> -		return sizes[0] < dma->format.sizeimage ? -EINVAL : 0;
> +		return sizes[0] < sizeimage ? -EINVAL : 0;
>  
>  	*nplanes = 1;
> -	sizes[0] = dma->format.sizeimage;
> +	sizes[0] = sizeimage;
>  
>  	return 0;
>  }
> @@ -350,14 +353,15 @@ static void xvip_dma_buffer_queue(struct vb2_buffer *vb)
>  	struct dma_async_tx_descriptor *desc;
>  	dma_addr_t addr = vb2_dma_contig_plane_dma_addr(vb, 0);
>  	u32 flags;
> +	struct v4l2_pix_format_mplane *pix_mp = &dma->format.fmt.pix_mp;
>  
> -	if (dma->queue.type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> +	if (dma->queue.type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
>  		flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
>  		dma->xt.dir = DMA_DEV_TO_MEM;
>  		dma->xt.src_sgl = false;
>  		dma->xt.dst_sgl = true;
>  		dma->xt.dst_start = addr;
> -	} else {
> +	} else if (dma->queue.type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {

I'd do 'else' even if any other value is not possible at this point.

>  		flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
>  		dma->xt.dir = DMA_MEM_TO_DEV;
>  		dma->xt.src_sgl = true;
> @@ -365,10 +369,11 @@ static void xvip_dma_buffer_queue(struct vb2_buffer *vb)
>  		dma->xt.src_start = addr;
>  	}
>  
> -	dma->xt.frame_size = 1;
> -	dma->sgl[0].size = dma->format.width * dma->fmtinfo->bpp[0];
> -	dma->sgl[0].icg = dma->format.bytesperline - dma->sgl[0].size;
> -	dma->xt.numf = dma->format.height;
> +	dma->xt.frame_size = dma->fmtinfo->num_planes;
> +	dma->sgl[0].size = pix_mp->width * dma->fmtinfo->bpp[0];
> +	dma->sgl[0].icg = pix_mp->plane_fmt[0].bytesperline - dma->sgl[0].size;
> +	dma->xt.numf = pix_mp->height;
> +	dma->sgl[0].dst_icg = 0;
>  
>  	desc = dmaengine_prep_interleaved_dma(dma->dma, &dma->xt, flags);
>  	if (!desc) {
> @@ -497,10 +502,15 @@ xvip_dma_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
>  	cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING
>  			  | dma->xdev->v4l2_caps;
>  
> -	if (dma->queue.type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> -		cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> -	else
> -		cap->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
> +	cap->device_caps = V4L2_CAP_STREAMING;
> +	switch (dma->queue.type) {
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> +		cap->device_caps |= V4L2_CAP_VIDEO_CAPTURE_MPLANE;
> +		break;
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> +		cap->device_caps |= V4L2_CAP_VIDEO_OUTPUT_MPLANE;
> +		break;
> +	}

I'd keep if - else rather than switch - case.

>  
>  	strlcpy(cap->driver, "xilinx-vipp", sizeof(cap->driver));
>  	strlcpy(cap->card, dma->video.name, sizeof(cap->card));
> @@ -524,7 +534,7 @@ xvip_dma_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
>  	if (f->index > 0)
>  		return -EINVAL;
>  
> -	f->pixelformat = dma->format.pixelformat;
> +	f->pixelformat = dma->format.fmt.pix_mp.pixelformat;
>  	strlcpy(f->description, dma->fmtinfo->description,
>  		sizeof(f->description));
>  
> @@ -537,13 +547,14 @@ xvip_dma_get_format(struct file *file, void *fh, struct v4l2_format *format)
>  	struct v4l2_fh *vfh = file->private_data;
>  	struct xvip_dma *dma = to_xvip_dma(vfh->vdev);
>  
> -	format->fmt.pix = dma->format;
> +	format->fmt.pix_mp = dma->format.fmt.pix_mp;
>  
>  	return 0;
>  }
>  
>  static void
> -__xvip_dma_try_format(struct xvip_dma *dma, struct v4l2_pix_format *pix,
> +__xvip_dma_try_format(struct xvip_dma *dma,
> +		      struct v4l2_format *format,
>  		      const struct xvip_video_format **fmtinfo)
>  {
>  	const struct xvip_video_format *info;
> @@ -551,19 +562,21 @@ __xvip_dma_try_format(struct xvip_dma *dma, struct v4l2_pix_format *pix,
>  	unsigned int max_width;
>  	unsigned int min_bpl;
>  	unsigned int max_bpl;
> -	unsigned int width;
> +	unsigned int width, height;
>  	unsigned int align;
>  	unsigned int bpl;
> +	unsigned int i;
> +	struct v4l2_pix_format_mplane *pix_mp = &format->fmt.pix_mp;
> +	struct v4l2_plane_pix_format *plane_fmt = pix_mp->plane_fmt;
>  
>  	/* Retrieve format information and select the default format if the
>  	 * requested format isn't supported.
>  	 */
> -	info = xvip_get_format_by_fourcc(pix->pixelformat);
> +	info = xvip_get_format_by_fourcc(pix_mp->pixelformat);
>  	if (IS_ERR(info))
>  		info = xvip_get_format_by_fourcc(XVIP_DMA_DEF_FORMAT);
>  
> -	pix->pixelformat = info->fourcc;
> -	pix->field = V4L2_FIELD_NONE;
> +	pix_mp->field = V4L2_FIELD_NONE;
>  
>  	/* The transfer alignment requirements are expressed in bytes. Compute
>  	 * the minimum and maximum values, clamp the requested width and convert
> @@ -572,22 +585,38 @@ __xvip_dma_try_format(struct xvip_dma *dma, struct v4l2_pix_format *pix,
>  	align = lcm(dma->align, info->bpp[0]);
>  	min_width = roundup(XVIP_DMA_MIN_WIDTH, align);
>  	max_width = rounddown(XVIP_DMA_MAX_WIDTH, align);
> -	width = rounddown(pix->width * info->bpp[0], align);
> -
> -	pix->width = clamp(width, min_width, max_width) / info->bpp[0];
> -	pix->height = clamp(pix->height, XVIP_DMA_MIN_HEIGHT,
> -			    XVIP_DMA_MAX_HEIGHT);
> +	pix_mp->width = clamp(pix_mp->width, min_width, max_width);
> +	pix_mp->height = clamp(pix_mp->height, XVIP_DMA_MIN_HEIGHT,
> +			       XVIP_DMA_MAX_HEIGHT);
>  
> -	/* Clamp the requested bytes per line value. If the maximum bytes per
> -	 * line value is zero, the module doesn't support user configurable line
> -	 * sizes. Override the requested value with the minimum in that case.
> +	/*
> +	 * Clamp the requested bytes per line value. If the maximum
> +	 * bytes per line value is zero, the module doesn't support
> +	 * user configurable line sizes. Override the requested value
> +	 * with the minimum in that case.
>  	 */
> -	min_bpl = pix->width * info->bpp[0];
> -	max_bpl = rounddown(XVIP_DMA_MAX_WIDTH, dma->align);
> -	bpl = rounddown(pix->bytesperline, dma->align);
> -
> -	pix->bytesperline = clamp(bpl, min_bpl, max_bpl);
> -	pix->sizeimage = pix->bytesperline * pix->height;
> +	max_bpl = rounddown(XVIP_DMA_MAX_WIDTH, align);
> +	min_bpl = pix_mp->width * info->bpp[0];
> +	min_bpl = roundup(min_bpl, align);
> +	bpl = roundup(plane_fmt[0].bytesperline, align);
> +	plane_fmt[0].bytesperline = clamp(bpl, min_bpl, max_bpl);
> +
> +	if (info->num_planes == 1) {
> +		/* Single plane formats */
> +		plane_fmt[0].sizeimage = plane_fmt[0].bytesperline *
> +					 pix_mp->height;
> +	} else {
> +		/* Supports Multi plane formats in a contiguous buffer,
> +		 * so we need only one buffer
> +		 */

Please use multiline comment style.

> +		plane_fmt[0].sizeimage = 0;

You can declare 'i' here.

> +		for (i = 0; i < info->num_planes; i++) {
> +			width = plane_fmt[0].bytesperline /
> +				(i ? info->hsub : 1);
> +			height = pix_mp->height / (i ? info->vsub : 1);
> +			plane_fmt[0].sizeimage += width * info->bpp[i] * height;

Just to clarify, only contiguous formats are supported, which is aligned with
the comment above on nplane > 1, correct?

> +		}
> +	}
>  
>  	if (fmtinfo)
>  		*fmtinfo = info;
> @@ -599,7 +628,7 @@ xvip_dma_try_format(struct file *file, void *fh, struct v4l2_format *format)
>  	struct v4l2_fh *vfh = file->private_data;
>  	struct xvip_dma *dma = to_xvip_dma(vfh->vdev);
>  
> -	__xvip_dma_try_format(dma, &format->fmt.pix, NULL);
> +	__xvip_dma_try_format(dma, format, NULL);
>  	return 0;
>  }
>  
> @@ -610,12 +639,13 @@ xvip_dma_set_format(struct file *file, void *fh, struct v4l2_format *format)
>  	struct xvip_dma *dma = to_xvip_dma(vfh->vdev);
>  	const struct xvip_video_format *info;
>  
> -	__xvip_dma_try_format(dma, &format->fmt.pix, &info);
> +	__xvip_dma_try_format(dma, format, &info);
>  
>  	if (vb2_is_busy(&dma->queue))
>  		return -EBUSY;
>  
> -	dma->format = format->fmt.pix;
> +	dma->format.fmt.pix_mp = format->fmt.pix_mp;
> +
>  	dma->fmtinfo = info;
>  
>  	return 0;
> @@ -623,13 +653,14 @@ xvip_dma_set_format(struct file *file, void *fh, struct v4l2_format *format)
>  
>  static const struct v4l2_ioctl_ops xvip_dma_ioctl_ops = {
>  	.vidioc_querycap		= xvip_dma_querycap,
> -	.vidioc_enum_fmt_vid_cap	= xvip_dma_enum_format,
> -	.vidioc_g_fmt_vid_cap		= xvip_dma_get_format,
> -	.vidioc_g_fmt_vid_out		= xvip_dma_get_format,
> -	.vidioc_s_fmt_vid_cap		= xvip_dma_set_format,
> -	.vidioc_s_fmt_vid_out		= xvip_dma_set_format,
> -	.vidioc_try_fmt_vid_cap		= xvip_dma_try_format,
> -	.vidioc_try_fmt_vid_out		= xvip_dma_try_format,
> +	.vidioc_enum_fmt_vid_cap_mplane	= xvip_dma_enum_format,
> +	.vidioc_enum_fmt_vid_out_mplane	= xvip_dma_enum_format,
> +	.vidioc_g_fmt_vid_cap_mplane	= xvip_dma_get_format,
> +	.vidioc_g_fmt_vid_out_mplane	= xvip_dma_get_format,
> +	.vidioc_s_fmt_vid_cap_mplane	= xvip_dma_set_format,
> +	.vidioc_s_fmt_vid_out_mplane	= xvip_dma_set_format,
> +	.vidioc_try_fmt_vid_cap_mplane	= xvip_dma_try_format,
> +	.vidioc_try_fmt_vid_out_mplane	= xvip_dma_try_format,
>  	.vidioc_reqbufs			= vb2_ioctl_reqbufs,
>  	.vidioc_querybuf		= vb2_ioctl_querybuf,
>  	.vidioc_qbuf			= vb2_ioctl_qbuf,
> @@ -662,6 +693,7 @@ int xvip_dma_init(struct xvip_composite_device *xdev, struct xvip_dma *dma,
>  {
>  	char name[16];
>  	int ret;
> +	struct v4l2_pix_format_mplane *pix_mp;
>  
>  	dma->xdev = xdev;
>  	dma->port = port;
> @@ -671,17 +703,23 @@ int xvip_dma_init(struct xvip_composite_device *xdev, struct xvip_dma *dma,
>  	spin_lock_init(&dma->queued_lock);
>  
>  	dma->fmtinfo = xvip_get_format_by_fourcc(XVIP_DMA_DEF_FORMAT);
> -	dma->format.pixelformat = dma->fmtinfo->fourcc;
> -	dma->format.colorspace = V4L2_COLORSPACE_SRGB;
> -	dma->format.field = V4L2_FIELD_NONE;
> -	dma->format.width = XVIP_DMA_DEF_WIDTH;
> -	dma->format.height = XVIP_DMA_DEF_HEIGHT;
> -	dma->format.bytesperline = dma->format.width * dma->fmtinfo->bpp[0];
> -	dma->format.sizeimage = dma->format.bytesperline * dma->format.height;
> +	dma->format.type = type;
> +
> +	pix_mp = &dma->format.fmt.pix_mp;
> +	pix_mp->pixelformat = dma->fmtinfo->fourcc;
> +	pix_mp->colorspace = V4L2_COLORSPACE_SRGB;
> +	pix_mp->field = V4L2_FIELD_NONE;
> +	pix_mp->width = XVIP_DMA_DEF_WIDTH;
> +	pix_mp->plane_fmt[0].bytesperline = pix_mp->width *
> +					    dma->fmtinfo->bpp[0];
> +	pix_mp->plane_fmt[0].sizeimage = pix_mp->plane_fmt[0].bytesperline *
> +					 pix_mp->height;
>  
>  	/* Initialize the media entity... */
> -	dma->pad.flags = type == V4L2_BUF_TYPE_VIDEO_CAPTURE
> -		       ? MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE;
> +	if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +		dma->pad.flags = MEDIA_PAD_FL_SINK;
> +	else if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> +		dma->pad.flags = MEDIA_PAD_FL_SOURCE;

I'd use 'else' or stick to the previous line just changing to MPLANE.

>  
>  	ret = media_entity_pads_init(&dma->video.entity, 1, &dma->pad);
>  	if (ret < 0)
> @@ -693,11 +731,16 @@ int xvip_dma_init(struct xvip_composite_device *xdev, struct xvip_dma *dma,
>  	dma->video.queue = &dma->queue;
>  	snprintf(dma->video.name, sizeof(dma->video.name), "%s %s %u",
>  		 xdev->dev->of_node->name,
> -		 type == V4L2_BUF_TYPE_VIDEO_CAPTURE ? "output" : "input",
> +		 type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> +					? "output" : "input",
>  		 port);
> +
>  	dma->video.vfl_type = VFL_TYPE_GRABBER;
> -	dma->video.vfl_dir = type == V4L2_BUF_TYPE_VIDEO_CAPTURE
> -			   ? VFL_DIR_RX : VFL_DIR_TX;
> +	if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +		dma->video.vfl_dir = VFL_DIR_RX;
> +	else if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> +		dma->video.vfl_dir = VFL_DIR_TX;

Ditto.

Thanks,
-hyun

> +
>  	dma->video.release = video_device_release_empty;
>  	dma->video.ioctl_ops = &xvip_dma_ioctl_ops;
>  	dma->video.lock = &dma->lock;
> diff --git a/drivers/media/platform/xilinx/xilinx-dma.h b/drivers/media/platform/xilinx/xilinx-dma.h
> index e95d136..96933ed 100644
> --- a/drivers/media/platform/xilinx/xilinx-dma.h
> +++ b/drivers/media/platform/xilinx/xilinx-dma.h
> @@ -62,7 +62,7 @@ static inline struct xvip_pipeline *to_xvip_pipeline(struct media_entity *e)
>   * @pipe: pipeline belonging to the DMA channel
>   * @port: composite device DT node port number for the DMA channel
>   * @lock: protects the @format, @fmtinfo and @queue fields
> - * @format: active V4L2 pixel format
> + * @format: V4L2 format
>   * @fmtinfo: format information corresponding to the active @format
>   * @queue: vb2 buffers queue
>   * @sequence: V4L2 buffers sequence number
> @@ -83,7 +83,7 @@ struct xvip_dma {
>  	unsigned int port;
>  
>  	struct mutex lock;
> -	struct v4l2_pix_format format;
> +	struct v4l2_format format;
>  	const struct xvip_video_format *fmtinfo;
>  
>  	struct vb2_queue queue;
> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
> index 6bb28cd..509b50f 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -433,12 +433,15 @@ static int xvip_graph_dma_init_one(struct xvip_composite_device *xdev,
>  	if (ret < 0)
>  		return ret;
>  
> -	if (strcmp(direction, "input") == 0)
> -		type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> -	else if (strcmp(direction, "output") == 0)
> -		type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
> -	else
> +	if (strcmp(direction, "input") == 0) {
> +		type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> +		xdev->v4l2_caps |= V4L2_CAP_VIDEO_CAPTURE_MPLANE;
> +	} else if (strcmp(direction, "output") == 0) {
> +		type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> +		xdev->v4l2_caps |= V4L2_CAP_VIDEO_OUTPUT_MPLANE;
> +	} else {
>  		return -EINVAL;
> +	}
>  
>  	of_property_read_u32(node, "reg", &index);
>  
> @@ -454,9 +457,6 @@ static int xvip_graph_dma_init_one(struct xvip_composite_device *xdev,
>  
>  	list_add_tail(&dma->list, &xdev->dmas);
>  
> -	xdev->v4l2_caps |= type == V4L2_BUF_TYPE_VIDEO_CAPTURE
> -			 ? V4L2_CAP_VIDEO_CAPTURE : V4L2_CAP_VIDEO_OUTPUT;
> -
>  	return 0;
>  }
>  
> -- 
> 2.1.1
>