Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1716558 > unrolled thread

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI. Hans Verkuil <hverkuil@xs4all.nl> - 2017-08-21 16:40 +0200
    Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI. Yong <yong.deng@magewell.com> - 2017-08-22 05:10 +0200
      Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI. Hans Verkuil <hverkuil@xs4all.nl> - 2017-08-22 08:50 +0200
        Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI. Yong <yong.deng@magewell.com> - 2017-08-22 10:00 +0200
        Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI. Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-08-22 22:20 +0200
          Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI. Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-08-22 23:00 +0200
          Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI. Hans Verkuil <hverkuil@xs4all.nl> - 2017-08-23 09:00 +0200
            Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI. Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-08-23 09:50 +0200
              Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI. icenowy@aosc.io - 2017-08-23 13:20 +0200

#1716558 — Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

Hi Yong,

First two high-level comments before I start the review:

1) Can you provide the v4l2-compliance output? I can't merge this unless I
   see that it passes the compliance tests. Make sure you compile from the git
   repo (https://git.linuxtv.org/v4l-utils.git/) so you are using the latest
   version of the compliance test. Test with just 'v4l2-compliance' and also
   with the -s option (to test streaming) and with the -f option (to test all
   the various pixel formats).

2) I see you support interlaced formats. Did you actually test that? Interlaced
   is tricky and you shouldn't add support for it unless you know it works and
   that it passes the 'v4l2-compliance -s' test.

On 07/27/2017 07:01 AM, Yong Deng wrote:
> Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> and CSI1 is used for parallel interface. This is not documented in
> datasheet but by testing and guess.
> 
> This patch implement a v4l2 framework driver for it.
> 
> Currently, the driver only support the parallel interface. MIPI-CSI2,
> ISP's support are not included in this patch.
> 
> Signed-off-by: Yong Deng <yong.deng@magewell.com>
> ---
>  drivers/media/platform/Kconfig                   |   1 +
>  drivers/media/platform/Makefile                  |   2 +
>  drivers/media/platform/sun6i-csi/Kconfig         |   9 +
>  drivers/media/platform/sun6i-csi/Makefile        |   3 +
>  drivers/media/platform/sun6i-csi/sun6i_csi.c     | 545 +++++++++++++++
>  drivers/media/platform/sun6i-csi/sun6i_csi.h     | 203 ++++++
>  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c | 827 +++++++++++++++++++++++
>  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h | 206 ++++++
>  drivers/media/platform/sun6i-csi/sun6i_video.c   | 663 ++++++++++++++++++
>  drivers/media/platform/sun6i-csi/sun6i_video.h   |  61 ++
>  10 files changed, 2520 insertions(+)
>  create mode 100644 drivers/media/platform/sun6i-csi/Kconfig
>  create mode 100644 drivers/media/platform/sun6i-csi/Makefile
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.c
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.h
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.c
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.h
> 

<snip>

> diff --git a/drivers/media/platform/sun6i-csi/sun6i_video.c b/drivers/media/platform/sun6i-csi/sun6i_video.c
> new file mode 100644
> index 0000000..0c5dbd2
> --- /dev/null
> +++ b/drivers/media/platform/sun6i-csi/sun6i_video.c
> @@ -0,0 +1,663 @@
> +/*
> + * Copyright (c) 2017 Magewell Electronics Co., Ltd. (Nanjing).
> + * All rights reserved.
> + * Author: Yong Deng <yong.deng@magewell.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/of.h>
> +
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-mc.h>
> +#include <media/videobuf2-dma-contig.h>
> +#include <media/videobuf2-v4l2.h>
> +
> +#include "sun6i_csi.h"
> +#include "sun6i_video.h"
> +
> +struct sun6i_csi_buffer {
> +	struct vb2_v4l2_buffer		vb;
> +	struct list_head		list;
> +
> +	dma_addr_t			dma_addr;
> +};
> +
> +static struct sun6i_csi_format *
> +find_format_by_fourcc(struct sun6i_video *video, unsigned int fourcc)
> +{
> +	unsigned int num_formats = video->num_formats;
> +	struct sun6i_csi_format *fmt;
> +	unsigned int i;
> +
> +	for (i = 0; i < num_formats; i++) {
> +		fmt = &video->formats[i];
> +		if (fmt->fourcc == fourcc)
> +			return fmt;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct v4l2_subdev *
> +sun6i_video_remote_subdev(struct sun6i_video *video, u32 *pad)
> +{
> +	struct media_pad *remote;
> +
> +	remote = media_entity_remote_pad(&video->pad);
> +
> +	if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
> +		return NULL;
> +
> +	if (pad)
> +		*pad = remote->index;
> +
> +	return media_entity_to_v4l2_subdev(remote->entity);
> +}
> +
> +static int sun6i_video_queue_setup(struct vb2_queue *vq,
> +				 unsigned int *nbuffers, unsigned int *nplanes,
> +				 unsigned int sizes[],
> +				 struct device *alloc_devs[])
> +{
> +	struct sun6i_video *video = vb2_get_drv_priv(vq);
> +	unsigned int size = video->fmt.fmt.pix.sizeimage;
> +
> +	if (*nplanes)
> +		return sizes[0] < size ? -EINVAL : 0;
> +
> +	*nplanes = 1;
> +	sizes[0] = size;
> +
> +	return 0;
> +}
> +
> +static int sun6i_video_buffer_prepare(struct vb2_buffer *vb)
> +{
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct sun6i_csi_buffer *buf =
> +			container_of(vbuf, struct sun6i_csi_buffer, vb);
> +	struct sun6i_video *video = vb2_get_drv_priv(vb->vb2_queue);
> +	unsigned long size = video->fmt.fmt.pix.sizeimage;
> +
> +	if (vb2_plane_size(vb, 0) < size) {
> +		v4l2_err(video->vdev.v4l2_dev, "buffer too small (%lu < %lu)\n",
> +			 vb2_plane_size(vb, 0), size);
> +		return -EINVAL;
> +	}
> +
> +	vb2_set_plane_payload(vb, 0, size);
> +
> +	buf->dma_addr = vb2_dma_contig_plane_dma_addr(vb, 0);
> +
> +	vbuf->field = video->fmt.fmt.pix.field;
> +
> +	return 0;
> +}
> +
> +static int sun6i_pipeline_set_stream(struct sun6i_video *video, bool enable)
> +{
> +	struct media_entity *entity;
> +	struct media_pad *pad;
> +	struct v4l2_subdev *subdev;
> +	int ret;
> +
> +	entity = &video->vdev.entity;
> +	while (1) {
> +		pad = &entity->pads[0];
> +		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> +			break;
> +
> +		pad = media_entity_remote_pad(pad);
> +		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> +			break;
> +
> +		entity = pad->entity;
> +		subdev = media_entity_to_v4l2_subdev(entity);
> +
> +		ret = v4l2_subdev_call(subdev, video, s_stream, enable);
> +		if (enable && ret < 0 && ret != -ENOIOCTLCMD)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sun6i_video_start_streaming(struct vb2_queue *vq, unsigned int count)
> +{
> +	struct sun6i_video *video = vb2_get_drv_priv(vq);
> +	struct sun6i_csi_buffer *buf;
> +	struct sun6i_csi_config config;
> +	unsigned long flags;
> +	int ret;
> +
> +	video->sequence = 0;
> +
> +	ret = media_pipeline_start(&video->vdev.entity, &video->vdev.pipe);
> +	if (ret < 0)
> +		goto err_start_pipeline;
> +
> +	ret = sun6i_pipeline_set_stream(video, true);
> +	if (ret < 0)
> +		goto err_start_stream;
> +
> +	config.pixelformat = video->fmt.fmt.pix.pixelformat;
> +	config.code = video->current_fmt->mbus_code;
> +	config.field = video->fmt.fmt.pix.field;
> +	config.width = video->fmt.fmt.pix.width;
> +	config.height = video->fmt.fmt.pix.height;
> +
> +	ret = sun6i_csi_update_config(video->csi, &config);
> +	if (ret < 0)
> +		goto err_update_config;
> +
> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> +	video->cur_frm = list_first_entry(&video->dma_queue,
> +					  struct sun6i_csi_buffer, list);
> +	list_del(&video->cur_frm->list);
> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> +
> +	ret = sun6i_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
> +	if (ret < 0)
> +		goto err_update_addr;
> +
> +	ret = sun6i_csi_set_stream(video->csi, true);
> +	if (ret < 0)
> +		goto err_csi_stream;
> +
> +	return 0;
> +
> +err_csi_stream:
> +err_update_addr:
> +err_update_config:
> +	sun6i_pipeline_set_stream(video, false);
> +err_start_stream:
> +	media_pipeline_stop(&video->vdev.entity);
> +err_start_pipeline:
> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> +	list_for_each_entry(buf, &video->dma_queue, list)
> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
> +	INIT_LIST_HEAD(&video->dma_queue);
> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> +
> +	return ret;
> +}
> +
> +static void sun6i_video_stop_streaming(struct vb2_queue *vq)
> +{
> +	struct sun6i_video *video = vb2_get_drv_priv(vq);
> +	unsigned long flags;
> +	struct sun6i_csi_buffer *buf;
> +
> +	sun6i_pipeline_set_stream(video, false);
> +
> +	sun6i_csi_set_stream(video->csi, false);
> +
> +	media_pipeline_stop(&video->vdev.entity);
> +
> +	/* Release all active buffers */
> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> +	if (unlikely(video->cur_frm)) {
> +		vb2_buffer_done(&video->cur_frm->vb.vb2_buf,
> +				VB2_BUF_STATE_ERROR);
> +		video->cur_frm = NULL;
> +	}
> +	list_for_each_entry(buf, &video->dma_queue, list)
> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> +	INIT_LIST_HEAD(&video->dma_queue);
> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> +}
> +
> +static void sun6i_video_buffer_queue(struct vb2_buffer *vb)
> +{
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct sun6i_csi_buffer *buf =
> +			container_of(vbuf, struct sun6i_csi_buffer, vb);
> +	struct sun6i_video *video = vb2_get_drv_priv(vb->vb2_queue);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> +	if (!video->cur_frm && list_empty(&video->dma_queue) &&
> +		vb2_is_streaming(vb->vb2_queue)) {
> +		video->cur_frm = buf;
> +		sun6i_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
> +		sun6i_csi_set_stream(video->csi, 1);
> +	} else
> +		list_add_tail(&buf->list, &video->dma_queue);
> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> +}
> +
> +void sun6i_video_frame_done(struct sun6i_video *video)
> +{
> +	spin_lock(&video->dma_queue_lock);
> +
> +	if (video->cur_frm) {
> +		struct vb2_v4l2_buffer *vbuf = &video->cur_frm->vb;
> +		struct vb2_buffer *vb = &vbuf->vb2_buf;
> +
> +		vb->timestamp = ktime_get_ns();
> +		vbuf->sequence = video->sequence++;
> +		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> +		video->cur_frm = NULL;
> +	}
> +
> +	if (!list_empty(&video->dma_queue)
> +	    && vb2_is_streaming(&video->vb2_vidq)) {
> +		video->cur_frm = list_first_entry(&video->dma_queue,
> +				struct sun6i_csi_buffer, list);
> +		list_del(&video->cur_frm->list);
> +		sun6i_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
> +	} else
> +		sun6i_csi_set_stream(video->csi, 0);
> +
> +	spin_unlock(&video->dma_queue_lock);
> +}
> +
> +static struct vb2_ops sun6i_csi_vb2_ops = {
> +	.queue_setup		= sun6i_video_queue_setup,
> +	.wait_prepare		= vb2_ops_wait_prepare,
> +	.wait_finish		= vb2_ops_wait_finish,
> +	.buf_prepare		= sun6i_video_buffer_prepare,
> +	.start_streaming	= sun6i_video_start_streaming,
> +	.stop_streaming		= sun6i_video_stop_streaming,
> +	.buf_queue		= sun6i_video_buffer_queue,
> +};
> +
> +static int vidioc_querycap(struct file *file, void *priv,
> +				struct v4l2_capability *cap)
> +{
> +	struct sun6i_video *video = video_drvdata(file);
> +
> +	strlcpy(cap->driver, "sun6i-video", sizeof(cap->driver));
> +	strlcpy(cap->card, video->vdev.name, sizeof(cap->card));
> +	snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> +		 video->csi->dev->of_node->name);
> +
> +	return 0;
> +}
> +
> +static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv,
> +				   struct v4l2_fmtdesc *f)
> +{
> +	struct sun6i_video *video = video_drvdata(file);
> +	u32 index = f->index;
> +
> +	if (index >= video->num_formats)
> +		return -EINVAL;
> +
> +	f->pixelformat = video->formats[index].fourcc;
> +
> +	return 0;
> +}
> +
> +static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
> +				struct v4l2_format *fmt)
> +{
> +	struct sun6i_video *video = video_drvdata(file);
> +
> +	*fmt = video->fmt;
> +
> +	return 0;
> +}
> +
> +static int sun6i_video_try_fmt(struct sun6i_video *video, struct v4l2_format *f,
> +			       struct sun6i_csi_format **current_fmt)
> +{
> +	struct sun6i_csi_format *csi_fmt;
> +	struct v4l2_pix_format *pixfmt = &f->fmt.pix;
> +	struct v4l2_subdev_format format;
> +	struct v4l2_subdev *subdev;
> +	u32 pad;
> +	int ret;
> +
> +	subdev = sun6i_video_remote_subdev(video, &pad);
> +	if (subdev == NULL)
> +		return -ENXIO;
> +
> +	csi_fmt = find_format_by_fourcc(video, pixfmt->pixelformat);
> +	if (csi_fmt == NULL)
> +		return -EINVAL;
> +
> +	format.pad = pad;
> +	format.which = V4L2_SUBDEV_FORMAT_TRY;
> +	v4l2_fill_mbus_format(&format.format, pixfmt, csi_fmt->mbus_code);
> +	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &format);
> +	if (ret)
> +		return ret;
> +
> +	v4l2_fill_pix_format(pixfmt, &format.format);
> +
> +	pixfmt->bytesperline = (pixfmt->width * csi_fmt->bpp) >> 3;
> +	pixfmt->sizeimage = (pixfmt->width * csi_fmt->bpp * pixfmt->height) / 8;
> +
> +	if (current_fmt)
> +		*current_fmt = csi_fmt;
> +
> +	return 0;
> +}
> +
> +static int sun6i_video_set_fmt(struct sun6i_video *video, struct v4l2_format *f)
> +{
> +	struct v4l2_subdev_format format;
> +	struct sun6i_csi_format *current_fmt;
> +	struct v4l2_subdev *subdev;
> +	u32 pad;
> +	int ret;
> +
> +	subdev = sun6i_video_remote_subdev(video, &pad);
> +	if (subdev == NULL)
> +		return -ENXIO;
> +
> +	ret = sun6i_video_try_fmt(video, f, &current_fmt);
> +	if (ret)
> +		return ret;
> +
> +	format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	v4l2_fill_mbus_format(&format.format, &f->fmt.pix,
> +			      current_fmt->mbus_code);
> +	ret = v4l2_subdev_call(subdev, pad, set_fmt, NULL, &format);
> +	if (ret < 0)
> +		return ret;
> +
> +	video->fmt = *f;
> +	video->current_fmt = current_fmt;
> +
> +	return 0;
> +}
> +
> +static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
> +				struct v4l2_format *f)
> +{
> +	struct sun6i_video *video = video_drvdata(file);
> +
> +	if (vb2_is_streaming(&video->vb2_vidq))

This should be vb2_is_busy(). Once buffers are allocated you are no longer allowed
to change the format, regardless of whether you are streaming or not.

> +		return -EBUSY;
> +
> +	return sun6i_video_set_fmt(video, f);
> +}
> +
> +static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
> +				  struct v4l2_format *f)
> +{
> +	struct sun6i_video *video = video_drvdata(file);
> +
> +	return sun6i_video_try_fmt(video, f, NULL);
> +}
> +
> +static const struct v4l2_ioctl_ops sun6i_video_ioctl_ops = {
> +	.vidioc_querycap		= vidioc_querycap,
> +	.vidioc_enum_fmt_vid_cap	= vidioc_enum_fmt_vid_cap,
> +	.vidioc_g_fmt_vid_cap		= vidioc_g_fmt_vid_cap,
> +	.vidioc_s_fmt_vid_cap		= vidioc_s_fmt_vid_cap,
> +	.vidioc_try_fmt_vid_cap		= vidioc_try_fmt_vid_cap,
> +
> +	.vidioc_reqbufs			= vb2_ioctl_reqbufs,
> +	.vidioc_querybuf		= vb2_ioctl_querybuf,
> +	.vidioc_qbuf			= vb2_ioctl_qbuf,
> +	.vidioc_expbuf			= vb2_ioctl_expbuf,
> +	.vidioc_dqbuf			= vb2_ioctl_dqbuf,
> +	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
> +	.vidioc_prepare_buf		= vb2_ioctl_prepare_buf,
> +	.vidioc_streamon		= vb2_ioctl_streamon,
> +	.vidioc_streamoff		= vb2_ioctl_streamoff,
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * V4L2 file operations
> + */
> +static int sun6i_video_open(struct file *file)
> +{
> +	struct sun6i_video *video = video_drvdata(file);
> +	struct v4l2_format format;
> +	int ret;
> +
> +	if (mutex_lock_interruptible(&video->lock))
> +		return -ERESTARTSYS;
> +
> +	ret = v4l2_fh_open(file);
> +	if (ret < 0)
> +		goto unlock;
> +
> +	ret = v4l2_pipeline_pm_use(&video->vdev.entity, 1);
> +	if (ret < 0)
> +		goto fh_release;
> +
> +	if (!v4l2_fh_is_singular_file(file))
> +		goto unlock;
> +
> +	ret = sun6i_csi_set_power(video->csi, true);
> +	if (ret < 0)
> +		goto fh_release;
> +
> +	/* setup default format */
> +	if (video->num_formats > 0) {
> +		format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +		format.fmt.pix.width = 1280;
> +		format.fmt.pix.height = 720;
> +		format.fmt.pix.pixelformat = video->formats[0].fourcc;
> +		sun6i_video_set_fmt(video, &format);
> +	}

No, this should happen when the driver is initialized. The format is
persistent over open()/close() calls so should not be re-initialized
here.

> +
> +	mutex_unlock(&video->lock);
> +	return 0;
> +
> +fh_release:
> +	v4l2_fh_release(file);
> +unlock:
> +	mutex_unlock(&video->lock);
> +	return ret;
> +}
> +
> +static int sun6i_video_close(struct file *file)
> +{
> +	struct sun6i_video *video = video_drvdata(file);
> +	bool last_fh;
> +
> +	mutex_lock(&video->lock);
> +
> +	last_fh = v4l2_fh_is_singular_file(file);
> +
> +	_vb2_fop_release(file, NULL);
> +
> +	v4l2_pipeline_pm_use(&video->vdev.entity, 0);
> +
> +	if (last_fh)
> +		sun6i_csi_set_power(video->csi, false);
> +
> +	mutex_unlock(&video->lock);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_file_operations sun6i_video_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= sun6i_video_open,
> +	.release	= sun6i_video_close,
> +	.unlocked_ioctl	= video_ioctl2,
> +	.mmap		= vb2_fop_mmap,
> +	.poll		= vb2_fop_poll
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Media Operations
> + */
> +static int sun6i_video_formats_init(struct sun6i_video *video)
> +{
> +	struct v4l2_subdev_mbus_code_enum mbus_code = { 0 };
> +	struct sun6i_csi *csi = video->csi;
> +	struct v4l2_subdev *subdev;
> +	u32 pad;
> +	const u32 *pixformats;
> +	int pixformat_count = 0;
> +	u32 subdev_codes[32]; /* subdev format codes, 32 should be enough */
> +	int codes_count = 0;
> +	int num_fmts = 0;
> +	int i, j;
> +
> +	subdev = sun6i_video_remote_subdev(video, &pad);
> +	if (subdev == NULL)
> +		return -ENXIO;
> +
> +	/* Get supported pixformats of CSI */
> +	pixformat_count = sun6i_csi_get_supported_pixformats(csi, &pixformats);
> +	if (pixformat_count <= 0)
> +		return -ENXIO;
> +
> +	/* Get subdev formats codes */
> +	mbus_code.pad = pad;
> +	mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL,
> +			&mbus_code)) {
> +		subdev_codes[codes_count] = mbus_code.code;

This definitely needs a check when codes_count >= ARRAY_SIZE(subdev_codes).

> +		codes_count++;
> +		mbus_code.index++;
> +	}
> +
> +	if (!codes_count)
> +		return -ENXIO;
> +
> +	/* Get supported formats count */
> +	for (i = 0; i < codes_count; i++) {
> +		for (j = 0; j < pixformat_count; j++) {
> +			if (!sun6i_csi_is_format_support(csi, pixformats[j],
> +					mbus_code.code)) {
> +				continue;
> +			}
> +			num_fmts++;
> +		}
> +	}
> +
> +	if (!num_fmts)
> +		return -ENXIO;
> +
> +	video->num_formats = num_fmts;
> +	video->formats = devm_kcalloc(video->csi->dev, num_fmts,
> +			sizeof(struct sun6i_csi_format), GFP_KERNEL);
> +	if (!video->formats) {
> +		dev_err(video->csi->dev, "could not allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* Get supported formats */
> +	num_fmts = 0;
> +	for (i = 0; i < codes_count; i++) {
> +		for (j = 0; j < pixformat_count; j++) {
> +			if (!sun6i_csi_is_format_support(csi, pixformats[j],
> +					mbus_code.code)) {
> +				continue;
> +			}
> +
> +			video->formats[num_fmts].fourcc = pixformats[j];
> +			video->formats[num_fmts].mbus_code =
> +					mbus_code.code;
> +			video->formats[num_fmts].bpp =
> +					v4l2_pixformat_get_bpp(pixformats[j]);
> +			num_fmts++;
> +		}
> +	}

As mentioned above, the initial format should probably be configure here.

> +
> +	return 0;
> +}
> +
> +static int sun6i_video_link_setup(struct media_entity *entity,
> +				  const struct media_pad *local,
> +				  const struct media_pad *remote, u32 flags)
> +{
> +	struct video_device *vdev = media_entity_to_video_device(entity);
> +	struct sun6i_video *video = video_get_drvdata(vdev);
> +
> +	if (WARN_ON(video == NULL))
> +		return 0;
> +
> +	return sun6i_video_formats_init(video);

Why is this called here? Why not in video_init()?

> +}
> +
> +static const struct media_entity_operations sun6i_video_media_ops = {
> +	.link_setup = sun6i_video_link_setup,
> +};
> +
> +int sun6i_video_init(struct sun6i_video *video, struct sun6i_csi *csi,
> +		     const char *name)
> +{
> +	struct video_device *vdev = &video->vdev;
> +	struct vb2_queue *vidq = &video->vb2_vidq;
> +	int ret;
> +
> +	video->csi = csi;
> +
> +	/* Initialize the media entity... */
> +	video->pad.flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
> +	vdev->entity.ops = &sun6i_video_media_ops;
> +	ret = media_entity_pads_init(&vdev->entity, 1, &video->pad);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_init(&video->lock);
> +
> +	INIT_LIST_HEAD(&video->dma_queue);
> +	spin_lock_init(&video->dma_queue_lock);
> +
> +	video->cur_frm = NULL;
> +	video->sequence = 0;
> +	video->num_formats = 0;
> +
> +	/* Initialize videobuf2 queue */
> +	vidq->type			= V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	vidq->io_modes			= VB2_MMAP | VB2_DMABUF;
> +	vidq->drv_priv			= video;
> +	vidq->buf_struct_size		= sizeof(struct sun6i_csi_buffer);
> +	vidq->ops			= &sun6i_csi_vb2_ops;
> +	vidq->mem_ops			= &vb2_dma_contig_memops;
> +	vidq->timestamp_flags		= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> +	vidq->lock			= &video->lock;
> +	vidq->min_buffers_needed	= 1;
> +	vidq->dev			= csi->dev;
> +
> +	ret = vb2_queue_init(vidq);
> +	if (ret) {
> +		v4l2_err(&csi->v4l2_dev, "vb2_queue_init failed: %d\n", ret);
> +		goto error;
> +	}
> +
> +	/* Register video device */
> +	strlcpy(vdev->name, name, sizeof(vdev->name));
> +	vdev->release		= video_device_release_empty;
> +	vdev->fops		= &sun6i_video_fops;
> +	vdev->ioctl_ops		= &sun6i_video_ioctl_ops;
> +	vdev->vfl_type		= VFL_TYPE_GRABBER;
> +	vdev->vfl_dir		= VFL_DIR_RX;
> +	vdev->v4l2_dev		= &csi->v4l2_dev;
> +	vdev->queue		= vidq;
> +	vdev->lock		= &video->lock;
> +	vdev->device_caps	= V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_CAPTURE;
> +	video_set_drvdata(vdev, video);
> +
> +	ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> +	if (ret < 0) {
> +		v4l2_err(&csi->v4l2_dev,
> +			 "video_register_device failed: %d\n", ret);
> +		goto error;
> +	}
> +
> +	return 0;
> +
> +error:
> +	sun6i_video_cleanup(video);
> +	return ret;
> +}
> +
> +void sun6i_video_cleanup(struct sun6i_video *video)
> +{
> +	if (video_is_registered(&video->vdev))
> +		video_unregister_device(&video->vdev);
> +
> +	media_entity_cleanup(&video->vdev.entity);
> +}
> diff --git a/drivers/media/platform/sun6i-csi/sun6i_video.h b/drivers/media/platform/sun6i-csi/sun6i_video.h
> new file mode 100644
> index 0000000..14eac6e
> --- /dev/null
> +++ b/drivers/media/platform/sun6i-csi/sun6i_video.h
> @@ -0,0 +1,61 @@
> +/*
> + * Copyright (c) 2017 Yong Deng <yong.deng@magewell.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __SUN6I_VIDEO_H__
> +#define __SUN6I_VIDEO_H__
> +
> +#include <media/v4l2-dev.h>
> +#include <media/videobuf2-core.h>
> +
> +/*
> + * struct sun6i_csi_format - CSI media bus format information
> + * @fourcc: Fourcc code for this format
> + * @mbus_code: V4L2 media bus format code.
> + * @bpp: Bytes per pixel (when stored in memory)
> + */
> +struct sun6i_csi_format {
> +	u32				fourcc;
> +	u32				mbus_code;
> +	u8				bpp;
> +};
> +
> +struct sun6i_csi;
> +
> +struct sun6i_video {
> +	struct video_device		vdev;
> +	struct media_pad		pad;
> +	struct sun6i_csi		*csi;
> +
> +	struct mutex			lock;
> +
> +	struct vb2_queue		vb2_vidq;
> +	spinlock_t			dma_queue_lock;
> +	struct list_head		dma_queue;
> +
> +	struct sun6i_csi_buffer		*cur_frm;
> +	unsigned int			sequence;
> +
> +	struct sun6i_csi_format		*formats;
> +	unsigned int			num_formats;
> +	struct sun6i_csi_format		*current_fmt;
> +	struct v4l2_format		fmt;
> +};
> +
> +int sun6i_video_init(struct sun6i_video *video, struct sun6i_csi *csi,
> +		     const char *name);
> +void sun6i_video_cleanup(struct sun6i_video *video);
> +
> +void sun6i_video_frame_done(struct sun6i_video *video);
> +
> +#endif /* __SUN6I_VIDEO_H__ */
> 

Regards,

	Hans

[toc] | [next] | [standalone]

#1716994

Hi Hans,

On Mon, 21 Aug 2017 16:37:41 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:

> Hi Yong,
> 
> First two high-level comments before I start the review:
> 
> 1) Can you provide the v4l2-compliance output? I can't merge this unless I
>    see that it passes the compliance tests. Make sure you compile from the git
>    repo (https://git.linuxtv.org/v4l-utils.git/) so you are using the latest
>    version of the compliance test. Test with just 'v4l2-compliance' and also
>    with the -s option (to test streaming) and with the -f option (to test all
>    the various pixel formats).

OK. I will post with the next version.

> 
> 2) I see you support interlaced formats. Did you actually test that? Interlaced
>    is tricky and you shouldn't add support for it unless you know it works and
>    that it passes the 'v4l2-compliance -s' test.

No. I do not have the condition to test the all formats for now. Maybe this can 
be done when ourself's device with V3s is ready in the future months.

> 
> On 07/27/2017 07:01 AM, Yong Deng wrote:
> > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> > and CSI1 is used for parallel interface. This is not documented in
> > datasheet but by testing and guess.
> > 
> > This patch implement a v4l2 framework driver for it.
> > 
> > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > ISP's support are not included in this patch.
> > 
> > Signed-off-by: Yong Deng <yong.deng@magewell.com>
> > ---
> >  drivers/media/platform/Kconfig                   |   1 +
> >  drivers/media/platform/Makefile                  |   2 +
> >  drivers/media/platform/sun6i-csi/Kconfig         |   9 +
> >  drivers/media/platform/sun6i-csi/Makefile        |   3 +
> >  drivers/media/platform/sun6i-csi/sun6i_csi.c     | 545 +++++++++++++++
> >  drivers/media/platform/sun6i-csi/sun6i_csi.h     | 203 ++++++
> >  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c | 827 +++++++++++++++++++++++
> >  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h | 206 ++++++
> >  drivers/media/platform/sun6i-csi/sun6i_video.c   | 663 ++++++++++++++++++
> >  drivers/media/platform/sun6i-csi/sun6i_video.h   |  61 ++
> >  10 files changed, 2520 insertions(+)
> >  create mode 100644 drivers/media/platform/sun6i-csi/Kconfig
> >  create mode 100644 drivers/media/platform/sun6i-csi/Makefile
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.c
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.h
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.c
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.h
> > 
> 
> <snip>
> 
> > diff --git a/drivers/media/platform/sun6i-csi/sun6i_video.c b/drivers/media/platform/sun6i-csi/sun6i_video.c
> > new file mode 100644
> > index 0000000..0c5dbd2
> > --- /dev/null
> > +++ b/drivers/media/platform/sun6i-csi/sun6i_video.c
> > @@ -0,0 +1,663 @@
> > +/*
> > + * Copyright (c) 2017 Magewell Electronics Co., Ltd. (Nanjing).
> > + * All rights reserved.
> > + * Author: Yong Deng <yong.deng@magewell.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/of.h>
> > +
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-ioctl.h>
> > +#include <media/v4l2-mc.h>
> > +#include <media/videobuf2-dma-contig.h>
> > +#include <media/videobuf2-v4l2.h>
> > +
> > +#include "sun6i_csi.h"
> > +#include "sun6i_video.h"
> > +
> > +struct sun6i_csi_buffer {
> > +	struct vb2_v4l2_buffer		vb;
> > +	struct list_head		list;
> > +
> > +	dma_addr_t			dma_addr;
> > +};
> > +
> > +static struct sun6i_csi_format *
> > +find_format_by_fourcc(struct sun6i_video *video, unsigned int fourcc)
> > +{
> > +	unsigned int num_formats = video->num_formats;
> > +	struct sun6i_csi_format *fmt;
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < num_formats; i++) {
> > +		fmt = &video->formats[i];
> > +		if (fmt->fourcc == fourcc)
> > +			return fmt;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +static struct v4l2_subdev *
> > +sun6i_video_remote_subdev(struct sun6i_video *video, u32 *pad)
> > +{
> > +	struct media_pad *remote;
> > +
> > +	remote = media_entity_remote_pad(&video->pad);
> > +
> > +	if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
> > +		return NULL;
> > +
> > +	if (pad)
> > +		*pad = remote->index;
> > +
> > +	return media_entity_to_v4l2_subdev(remote->entity);
> > +}
> > +
> > +static int sun6i_video_queue_setup(struct vb2_queue *vq,
> > +				 unsigned int *nbuffers, unsigned int *nplanes,
> > +				 unsigned int sizes[],
> > +				 struct device *alloc_devs[])
> > +{
> > +	struct sun6i_video *video = vb2_get_drv_priv(vq);
> > +	unsigned int size = video->fmt.fmt.pix.sizeimage;
> > +
> > +	if (*nplanes)
> > +		return sizes[0] < size ? -EINVAL : 0;
> > +
> > +	*nplanes = 1;
> > +	sizes[0] = size;
> > +
> > +	return 0;
> > +}
> > +
> > +static int sun6i_video_buffer_prepare(struct vb2_buffer *vb)
> > +{
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +	struct sun6i_csi_buffer *buf =
> > +			container_of(vbuf, struct sun6i_csi_buffer, vb);
> > +	struct sun6i_video *video = vb2_get_drv_priv(vb->vb2_queue);
> > +	unsigned long size = video->fmt.fmt.pix.sizeimage;
> > +
> > +	if (vb2_plane_size(vb, 0) < size) {
> > +		v4l2_err(video->vdev.v4l2_dev, "buffer too small (%lu < %lu)\n",
> > +			 vb2_plane_size(vb, 0), size);
> > +		return -EINVAL;
> > +	}
> > +
> > +	vb2_set_plane_payload(vb, 0, size);
> > +
> > +	buf->dma_addr = vb2_dma_contig_plane_dma_addr(vb, 0);
> > +
> > +	vbuf->field = video->fmt.fmt.pix.field;
> > +
> > +	return 0;
> > +}
> > +
> > +static int sun6i_pipeline_set_stream(struct sun6i_video *video, bool enable)
> > +{
> > +	struct media_entity *entity;
> > +	struct media_pad *pad;
> > +	struct v4l2_subdev *subdev;
> > +	int ret;
> > +
> > +	entity = &video->vdev.entity;
> > +	while (1) {
> > +		pad = &entity->pads[0];
> > +		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> > +			break;
> > +
> > +		pad = media_entity_remote_pad(pad);
> > +		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> > +			break;
> > +
> > +		entity = pad->entity;
> > +		subdev = media_entity_to_v4l2_subdev(entity);
> > +
> > +		ret = v4l2_subdev_call(subdev, video, s_stream, enable);
> > +		if (enable && ret < 0 && ret != -ENOIOCTLCMD)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int sun6i_video_start_streaming(struct vb2_queue *vq, unsigned int count)
> > +{
> > +	struct sun6i_video *video = vb2_get_drv_priv(vq);
> > +	struct sun6i_csi_buffer *buf;
> > +	struct sun6i_csi_config config;
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	video->sequence = 0;
> > +
> > +	ret = media_pipeline_start(&video->vdev.entity, &video->vdev.pipe);
> > +	if (ret < 0)
> > +		goto err_start_pipeline;
> > +
> > +	ret = sun6i_pipeline_set_stream(video, true);
> > +	if (ret < 0)
> > +		goto err_start_stream;
> > +
> > +	config.pixelformat = video->fmt.fmt.pix.pixelformat;
> > +	config.code = video->current_fmt->mbus_code;
> > +	config.field = video->fmt.fmt.pix.field;
> > +	config.width = video->fmt.fmt.pix.width;
> > +	config.height = video->fmt.fmt.pix.height;
> > +
> > +	ret = sun6i_csi_update_config(video->csi, &config);
> > +	if (ret < 0)
> > +		goto err_update_config;
> > +
> > +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> > +	video->cur_frm = list_first_entry(&video->dma_queue,
> > +					  struct sun6i_csi_buffer, list);
> > +	list_del(&video->cur_frm->list);
> > +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> > +
> > +	ret = sun6i_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
> > +	if (ret < 0)
> > +		goto err_update_addr;
> > +
> > +	ret = sun6i_csi_set_stream(video->csi, true);
> > +	if (ret < 0)
> > +		goto err_csi_stream;
> > +
> > +	return 0;
> > +
> > +err_csi_stream:
> > +err_update_addr:
> > +err_update_config:
> > +	sun6i_pipeline_set_stream(video, false);
> > +err_start_stream:
> > +	media_pipeline_stop(&video->vdev.entity);
> > +err_start_pipeline:
> > +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> > +	list_for_each_entry(buf, &video->dma_queue, list)
> > +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
> > +	INIT_LIST_HEAD(&video->dma_queue);
> > +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> > +
> > +	return ret;
> > +}
> > +
> > +static void sun6i_video_stop_streaming(struct vb2_queue *vq)
> > +{
> > +	struct sun6i_video *video = vb2_get_drv_priv(vq);
> > +	unsigned long flags;
> > +	struct sun6i_csi_buffer *buf;
> > +
> > +	sun6i_pipeline_set_stream(video, false);
> > +
> > +	sun6i_csi_set_stream(video->csi, false);
> > +
> > +	media_pipeline_stop(&video->vdev.entity);
> > +
> > +	/* Release all active buffers */
> > +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> > +	if (unlikely(video->cur_frm)) {
> > +		vb2_buffer_done(&video->cur_frm->vb.vb2_buf,
> > +				VB2_BUF_STATE_ERROR);
> > +		video->cur_frm = NULL;
> > +	}
> > +	list_for_each_entry(buf, &video->dma_queue, list)
> > +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> > +	INIT_LIST_HEAD(&video->dma_queue);
> > +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> > +}
> > +
> > +static void sun6i_video_buffer_queue(struct vb2_buffer *vb)
> > +{
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +	struct sun6i_csi_buffer *buf =
> > +			container_of(vbuf, struct sun6i_csi_buffer, vb);
> > +	struct sun6i_video *video = vb2_get_drv_priv(vb->vb2_queue);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> > +	if (!video->cur_frm && list_empty(&video->dma_queue) &&
> > +		vb2_is_streaming(vb->vb2_queue)) {
> > +		video->cur_frm = buf;
> > +		sun6i_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
> > +		sun6i_csi_set_stream(video->csi, 1);
> > +	} else
> > +		list_add_tail(&buf->list, &video->dma_queue);
> > +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> > +}
> > +
> > +void sun6i_video_frame_done(struct sun6i_video *video)
> > +{
> > +	spin_lock(&video->dma_queue_lock);
> > +
> > +	if (video->cur_frm) {
> > +		struct vb2_v4l2_buffer *vbuf = &video->cur_frm->vb;
> > +		struct vb2_buffer *vb = &vbuf->vb2_buf;
> > +
> > +		vb->timestamp = ktime_get_ns();
> > +		vbuf->sequence = video->sequence++;
> > +		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> > +		video->cur_frm = NULL;
> > +	}
> > +
> > +	if (!list_empty(&video->dma_queue)
> > +	    && vb2_is_streaming(&video->vb2_vidq)) {
> > +		video->cur_frm = list_first_entry(&video->dma_queue,
> > +				struct sun6i_csi_buffer, list);
> > +		list_del(&video->cur_frm->list);
> > +		sun6i_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
> > +	} else
> > +		sun6i_csi_set_stream(video->csi, 0);
> > +
> > +	spin_unlock(&video->dma_queue_lock);
> > +}
> > +
> > +static struct vb2_ops sun6i_csi_vb2_ops = {
> > +	.queue_setup		= sun6i_video_queue_setup,
> > +	.wait_prepare		= vb2_ops_wait_prepare,
> > +	.wait_finish		= vb2_ops_wait_finish,
> > +	.buf_prepare		= sun6i_video_buffer_prepare,
> > +	.start_streaming	= sun6i_video_start_streaming,
> > +	.stop_streaming		= sun6i_video_stop_streaming,
> > +	.buf_queue		= sun6i_video_buffer_queue,
> > +};
> > +
> > +static int vidioc_querycap(struct file *file, void *priv,
> > +				struct v4l2_capability *cap)
> > +{
> > +	struct sun6i_video *video = video_drvdata(file);
> > +
> > +	strlcpy(cap->driver, "sun6i-video", sizeof(cap->driver));
> > +	strlcpy(cap->card, video->vdev.name, sizeof(cap->card));
> > +	snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> > +		 video->csi->dev->of_node->name);
> > +
> > +	return 0;
> > +}
> > +
> > +static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv,
> > +				   struct v4l2_fmtdesc *f)
> > +{
> > +	struct sun6i_video *video = video_drvdata(file);
> > +	u32 index = f->index;
> > +
> > +	if (index >= video->num_formats)
> > +		return -EINVAL;
> > +
> > +	f->pixelformat = video->formats[index].fourcc;
> > +
> > +	return 0;
> > +}
> > +
> > +static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
> > +				struct v4l2_format *fmt)
> > +{
> > +	struct sun6i_video *video = video_drvdata(file);
> > +
> > +	*fmt = video->fmt;
> > +
> > +	return 0;
> > +}
> > +
> > +static int sun6i_video_try_fmt(struct sun6i_video *video, struct v4l2_format *f,
> > +			       struct sun6i_csi_format **current_fmt)
> > +{
> > +	struct sun6i_csi_format *csi_fmt;
> > +	struct v4l2_pix_format *pixfmt = &f->fmt.pix;
> > +	struct v4l2_subdev_format format;
> > +	struct v4l2_subdev *subdev;
> > +	u32 pad;
> > +	int ret;
> > +
> > +	subdev = sun6i_video_remote_subdev(video, &pad);
> > +	if (subdev == NULL)
> > +		return -ENXIO;
> > +
> > +	csi_fmt = find_format_by_fourcc(video, pixfmt->pixelformat);
> > +	if (csi_fmt == NULL)
> > +		return -EINVAL;
> > +
> > +	format.pad = pad;
> > +	format.which = V4L2_SUBDEV_FORMAT_TRY;
> > +	v4l2_fill_mbus_format(&format.format, pixfmt, csi_fmt->mbus_code);
> > +	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &format);
> > +	if (ret)
> > +		return ret;
> > +
> > +	v4l2_fill_pix_format(pixfmt, &format.format);
> > +
> > +	pixfmt->bytesperline = (pixfmt->width * csi_fmt->bpp) >> 3;
> > +	pixfmt->sizeimage = (pixfmt->width * csi_fmt->bpp * pixfmt->height) / 8;
> > +
> > +	if (current_fmt)
> > +		*current_fmt = csi_fmt;
> > +
> > +	return 0;
> > +}
> > +
> > +static int sun6i_video_set_fmt(struct sun6i_video *video, struct v4l2_format *f)
> > +{
> > +	struct v4l2_subdev_format format;
> > +	struct sun6i_csi_format *current_fmt;
> > +	struct v4l2_subdev *subdev;
> > +	u32 pad;
> > +	int ret;
> > +
> > +	subdev = sun6i_video_remote_subdev(video, &pad);
> > +	if (subdev == NULL)
> > +		return -ENXIO;
> > +
> > +	ret = sun6i_video_try_fmt(video, f, &current_fmt);
> > +	if (ret)
> > +		return ret;
> > +
> > +	format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > +	v4l2_fill_mbus_format(&format.format, &f->fmt.pix,
> > +			      current_fmt->mbus_code);
> > +	ret = v4l2_subdev_call(subdev, pad, set_fmt, NULL, &format);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	video->fmt = *f;
> > +	video->current_fmt = current_fmt;
> > +
> > +	return 0;
> > +}
> > +
> > +static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
> > +				struct v4l2_format *f)
> > +{
> > +	struct sun6i_video *video = video_drvdata(file);
> > +
> > +	if (vb2_is_streaming(&video->vb2_vidq))
> 
> This should be vb2_is_busy(). Once buffers are allocated you are no longer allowed
> to change the format, regardless of whether you are streaming or not.

OK.

> 
> > +		return -EBUSY;
> > +
> > +	return sun6i_video_set_fmt(video, f);
> > +}
> > +
> > +static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
> > +				  struct v4l2_format *f)
> > +{
> > +	struct sun6i_video *video = video_drvdata(file);
> > +
> > +	return sun6i_video_try_fmt(video, f, NULL);
> > +}
> > +
> > +static const struct v4l2_ioctl_ops sun6i_video_ioctl_ops = {
> > +	.vidioc_querycap		= vidioc_querycap,
> > +	.vidioc_enum_fmt_vid_cap	= vidioc_enum_fmt_vid_cap,
> > +	.vidioc_g_fmt_vid_cap		= vidioc_g_fmt_vid_cap,
> > +	.vidioc_s_fmt_vid_cap		= vidioc_s_fmt_vid_cap,
> > +	.vidioc_try_fmt_vid_cap		= vidioc_try_fmt_vid_cap,
> > +
> > +	.vidioc_reqbufs			= vb2_ioctl_reqbufs,
> > +	.vidioc_querybuf		= vb2_ioctl_querybuf,
> > +	.vidioc_qbuf			= vb2_ioctl_qbuf,
> > +	.vidioc_expbuf			= vb2_ioctl_expbuf,
> > +	.vidioc_dqbuf			= vb2_ioctl_dqbuf,
> > +	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
> > +	.vidioc_prepare_buf		= vb2_ioctl_prepare_buf,
> > +	.vidioc_streamon		= vb2_ioctl_streamon,
> > +	.vidioc_streamoff		= vb2_ioctl_streamoff,
> > +};
> > +
> > +/* -----------------------------------------------------------------------------
> > + * V4L2 file operations
> > + */
> > +static int sun6i_video_open(struct file *file)
> > +{
> > +	struct sun6i_video *video = video_drvdata(file);
> > +	struct v4l2_format format;
> > +	int ret;
> > +
> > +	if (mutex_lock_interruptible(&video->lock))
> > +		return -ERESTARTSYS;
> > +
> > +	ret = v4l2_fh_open(file);
> > +	if (ret < 0)
> > +		goto unlock;
> > +
> > +	ret = v4l2_pipeline_pm_use(&video->vdev.entity, 1);
> > +	if (ret < 0)
> > +		goto fh_release;
> > +
> > +	if (!v4l2_fh_is_singular_file(file))
> > +		goto unlock;
> > +
> > +	ret = sun6i_csi_set_power(video->csi, true);
> > +	if (ret < 0)
> > +		goto fh_release;
> > +
> > +	/* setup default format */
> > +	if (video->num_formats > 0) {
> > +		format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > +		format.fmt.pix.width = 1280;
> > +		format.fmt.pix.height = 720;
> > +		format.fmt.pix.pixelformat = video->formats[0].fourcc;
> > +		sun6i_video_set_fmt(video, &format);
> > +	}
> 
> No, this should happen when the driver is initialized. The format is
> persistent over open()/close() calls so should not be re-initialized
> here.

OK.

> 
> > +
> > +	mutex_unlock(&video->lock);
> > +	return 0;
> > +
> > +fh_release:
> > +	v4l2_fh_release(file);
> > +unlock:
> > +	mutex_unlock(&video->lock);
> > +	return ret;
> > +}
> > +
> > +static int sun6i_video_close(struct file *file)
> > +{
> > +	struct sun6i_video *video = video_drvdata(file);
> > +	bool last_fh;
> > +
> > +	mutex_lock(&video->lock);
> > +
> > +	last_fh = v4l2_fh_is_singular_file(file);
> > +
> > +	_vb2_fop_release(file, NULL);
> > +
> > +	v4l2_pipeline_pm_use(&video->vdev.entity, 0);
> > +
> > +	if (last_fh)
> > +		sun6i_csi_set_power(video->csi, false);
> > +
> > +	mutex_unlock(&video->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_file_operations sun6i_video_fops = {
> > +	.owner		= THIS_MODULE,
> > +	.open		= sun6i_video_open,
> > +	.release	= sun6i_video_close,
> > +	.unlocked_ioctl	= video_ioctl2,
> > +	.mmap		= vb2_fop_mmap,
> > +	.poll		= vb2_fop_poll
> > +};
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Media Operations
> > + */
> > +static int sun6i_video_formats_init(struct sun6i_video *video)
> > +{
> > +	struct v4l2_subdev_mbus_code_enum mbus_code = { 0 };
> > +	struct sun6i_csi *csi = video->csi;
> > +	struct v4l2_subdev *subdev;
> > +	u32 pad;
> > +	const u32 *pixformats;
> > +	int pixformat_count = 0;
> > +	u32 subdev_codes[32]; /* subdev format codes, 32 should be enough */
> > +	int codes_count = 0;
> > +	int num_fmts = 0;
> > +	int i, j;
> > +
> > +	subdev = sun6i_video_remote_subdev(video, &pad);
> > +	if (subdev == NULL)
> > +		return -ENXIO;
> > +
> > +	/* Get supported pixformats of CSI */
> > +	pixformat_count = sun6i_csi_get_supported_pixformats(csi, &pixformats);
> > +	if (pixformat_count <= 0)
> > +		return -ENXIO;
> > +
> > +	/* Get subdev formats codes */
> > +	mbus_code.pad = pad;
> > +	mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > +	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL,
> > +			&mbus_code)) {
> > +		subdev_codes[codes_count] = mbus_code.code;
> 
> This definitely needs a check when codes_count >= ARRAY_SIZE(subdev_codes).

OK.

> 
> > +		codes_count++;
> > +		mbus_code.index++;
> > +	}
> > +
> > +	if (!codes_count)
> > +		return -ENXIO;
> > +
> > +	/* Get supported formats count */
> > +	for (i = 0; i < codes_count; i++) {
> > +		for (j = 0; j < pixformat_count; j++) {
> > +			if (!sun6i_csi_is_format_support(csi, pixformats[j],
> > +					mbus_code.code)) {
> > +				continue;
> > +			}
> > +			num_fmts++;
> > +		}
> > +	}
> > +
> > +	if (!num_fmts)
> > +		return -ENXIO;
> > +
> > +	video->num_formats = num_fmts;
> > +	video->formats = devm_kcalloc(video->csi->dev, num_fmts,
> > +			sizeof(struct sun6i_csi_format), GFP_KERNEL);
> > +	if (!video->formats) {
> > +		dev_err(video->csi->dev, "could not allocate memory\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	/* Get supported formats */
> > +	num_fmts = 0;
> > +	for (i = 0; i < codes_count; i++) {
> > +		for (j = 0; j < pixformat_count; j++) {
> > +			if (!sun6i_csi_is_format_support(csi, pixformats[j],
> > +					mbus_code.code)) {
> > +				continue;
> > +			}
> > +
> > +			video->formats[num_fmts].fourcc = pixformats[j];
> > +			video->formats[num_fmts].mbus_code =
> > +					mbus_code.code;
> > +			video->formats[num_fmts].bpp =
> > +					v4l2_pixformat_get_bpp(pixformats[j]);
> > +			num_fmts++;
> > +		}
> > +	}
> 
> As mentioned above, the initial format should probably be configure here.

OK.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int sun6i_video_link_setup(struct media_entity *entity,
> > +				  const struct media_pad *local,
> > +				  const struct media_pad *remote, u32 flags)
> > +{
> > +	struct video_device *vdev = media_entity_to_video_device(entity);
> > +	struct sun6i_video *video = video_get_drvdata(vdev);
> > +
> > +	if (WARN_ON(video == NULL))
> > +		return 0;
> > +
> > +	return sun6i_video_formats_init(video);
> 
> Why is this called here? Why not in video_init()?

sun6i_video_init is in the driver probe context. sun6i_video_formats_init
use media_entity_remote_pad and media_entity_to_v4l2_subdev to find the
subdevs.
The media_entity_remote_pad can't work before all the media pad linked.

> 
> > +}
> > +
> > +static const struct media_entity_operations sun6i_video_media_ops = {
> > +	.link_setup = sun6i_video_link_setup,
> > +};
> > +
> > +int sun6i_video_init(struct sun6i_video *video, struct sun6i_csi *csi,
> > +		     const char *name)
> > +{
> > +	struct video_device *vdev = &video->vdev;
> > +	struct vb2_queue *vidq = &video->vb2_vidq;
> > +	int ret;
> > +
> > +	video->csi = csi;
> > +
> > +	/* Initialize the media entity... */
> > +	video->pad.flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
> > +	vdev->entity.ops = &sun6i_video_media_ops;
> > +	ret = media_entity_pads_init(&vdev->entity, 1, &video->pad);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	mutex_init(&video->lock);
> > +
> > +	INIT_LIST_HEAD(&video->dma_queue);
> > +	spin_lock_init(&video->dma_queue_lock);
> > +
> > +	video->cur_frm = NULL;
> > +	video->sequence = 0;
> > +	video->num_formats = 0;
> > +
> > +	/* Initialize videobuf2 queue */
> > +	vidq->type			= V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > +	vidq->io_modes			= VB2_MMAP | VB2_DMABUF;
> > +	vidq->drv_priv			= video;
> > +	vidq->buf_struct_size		= sizeof(struct sun6i_csi_buffer);
> > +	vidq->ops			= &sun6i_csi_vb2_ops;
> > +	vidq->mem_ops			= &vb2_dma_contig_memops;
> > +	vidq->timestamp_flags		= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > +	vidq->lock			= &video->lock;
> > +	vidq->min_buffers_needed	= 1;
> > +	vidq->dev			= csi->dev;
> > +
> > +	ret = vb2_queue_init(vidq);
> > +	if (ret) {
> > +		v4l2_err(&csi->v4l2_dev, "vb2_queue_init failed: %d\n", ret);
> > +		goto error;
> > +	}
> > +
> > +	/* Register video device */
> > +	strlcpy(vdev->name, name, sizeof(vdev->name));
> > +	vdev->release		= video_device_release_empty;
> > +	vdev->fops		= &sun6i_video_fops;
> > +	vdev->ioctl_ops		= &sun6i_video_ioctl_ops;
> > +	vdev->vfl_type		= VFL_TYPE_GRABBER;
> > +	vdev->vfl_dir		= VFL_DIR_RX;
> > +	vdev->v4l2_dev		= &csi->v4l2_dev;
> > +	vdev->queue		= vidq;
> > +	vdev->lock		= &video->lock;
> > +	vdev->device_caps	= V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_CAPTURE;
> > +	video_set_drvdata(vdev, video);
> > +
> > +	ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> > +	if (ret < 0) {
> > +		v4l2_err(&csi->v4l2_dev,
> > +			 "video_register_device failed: %d\n", ret);
> > +		goto error;
> > +	}
> > +
> > +	return 0;
> > +
> > +error:
> > +	sun6i_video_cleanup(video);
> > +	return ret;
> > +}
> > +
> > +void sun6i_video_cleanup(struct sun6i_video *video)
> > +{
> > +	if (video_is_registered(&video->vdev))
> > +		video_unregister_device(&video->vdev);
> > +
> > +	media_entity_cleanup(&video->vdev.entity);
> > +}
> > diff --git a/drivers/media/platform/sun6i-csi/sun6i_video.h b/drivers/media/platform/sun6i-csi/sun6i_video.h
> > new file mode 100644
> > index 0000000..14eac6e
> > --- /dev/null
> > +++ b/drivers/media/platform/sun6i-csi/sun6i_video.h
> > @@ -0,0 +1,61 @@
> > +/*
> > + * Copyright (c) 2017 Yong Deng <yong.deng@magewell.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#ifndef __SUN6I_VIDEO_H__
> > +#define __SUN6I_VIDEO_H__
> > +
> > +#include <media/v4l2-dev.h>
> > +#include <media/videobuf2-core.h>
> > +
> > +/*
> > + * struct sun6i_csi_format - CSI media bus format information
> > + * @fourcc: Fourcc code for this format
> > + * @mbus_code: V4L2 media bus format code.
> > + * @bpp: Bytes per pixel (when stored in memory)
> > + */
> > +struct sun6i_csi_format {
> > +	u32				fourcc;
> > +	u32				mbus_code;
> > +	u8				bpp;
> > +};
> > +
> > +struct sun6i_csi;
> > +
> > +struct sun6i_video {
> > +	struct video_device		vdev;
> > +	struct media_pad		pad;
> > +	struct sun6i_csi		*csi;
> > +
> > +	struct mutex			lock;
> > +
> > +	struct vb2_queue		vb2_vidq;
> > +	spinlock_t			dma_queue_lock;
> > +	struct list_head		dma_queue;
> > +
> > +	struct sun6i_csi_buffer		*cur_frm;
> > +	unsigned int			sequence;
> > +
> > +	struct sun6i_csi_format		*formats;
> > +	unsigned int			num_formats;
> > +	struct sun6i_csi_format		*current_fmt;
> > +	struct v4l2_format		fmt;
> > +};
> > +
> > +int sun6i_video_init(struct sun6i_video *video, struct sun6i_csi *csi,
> > +		     const char *name);
> > +void sun6i_video_cleanup(struct sun6i_video *video);
> > +
> > +void sun6i_video_frame_done(struct sun6i_video *video);
> > +
> > +#endif /* __SUN6I_VIDEO_H__ */
> > 
> 
> Regards,
> 
> 	Hans


Thanks,
Yong

[toc] | [prev] | [next] | [standalone]

#1717073

On 08/22/2017 05:01 AM, Yong wrote:
> Hi Hans,
> 
> On Mon, 21 Aug 2017 16:37:41 +0200
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
>> Hi Yong,
>>
>> First two high-level comments before I start the review:
>>
>> 1) Can you provide the v4l2-compliance output? I can't merge this unless I
>>    see that it passes the compliance tests. Make sure you compile from the git
>>    repo (https://git.linuxtv.org/v4l-utils.git/) so you are using the latest
>>    version of the compliance test. Test with just 'v4l2-compliance' and also
>>    with the -s option (to test streaming) and with the -f option (to test all
>>    the various pixel formats).
> 
> OK. I will post with the next version.
> 
>>
>> 2) I see you support interlaced formats. Did you actually test that? Interlaced
>>    is tricky and you shouldn't add support for it unless you know it works and
>>    that it passes the 'v4l2-compliance -s' test.
> 
> No. I do not have the condition to test the all formats for now. Maybe this can 
> be done when ourself's device with V3s is ready in the future months.

Then just drop the interlaced support until you can test it.

> 
>>
>> On 07/27/2017 07:01 AM, Yong Deng wrote:
>>> Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
>>> and CSI1 is used for parallel interface. This is not documented in
>>> datasheet but by testing and guess.
>>>
>>> This patch implement a v4l2 framework driver for it.
>>>
>>> Currently, the driver only support the parallel interface. MIPI-CSI2,
>>> ISP's support are not included in this patch.
>>>
>>> Signed-off-by: Yong Deng <yong.deng@magewell.com>
>>> ---
>>>  drivers/media/platform/Kconfig                   |   1 +
>>>  drivers/media/platform/Makefile                  |   2 +
>>>  drivers/media/platform/sun6i-csi/Kconfig         |   9 +
>>>  drivers/media/platform/sun6i-csi/Makefile        |   3 +
>>>  drivers/media/platform/sun6i-csi/sun6i_csi.c     | 545 +++++++++++++++
>>>  drivers/media/platform/sun6i-csi/sun6i_csi.h     | 203 ++++++
>>>  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c | 827 +++++++++++++++++++++++
>>>  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h | 206 ++++++
>>>  drivers/media/platform/sun6i-csi/sun6i_video.c   | 663 ++++++++++++++++++
>>>  drivers/media/platform/sun6i-csi/sun6i_video.h   |  61 ++
>>>  10 files changed, 2520 insertions(+)
>>>  create mode 100644 drivers/media/platform/sun6i-csi/Kconfig
>>>  create mode 100644 drivers/media/platform/sun6i-csi/Makefile
>>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.c
>>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.h
>>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c
>>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h
>>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.c
>>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.h
>>>
>>
>> <snip>
>>
>>> diff --git a/drivers/media/platform/sun6i-csi/sun6i_video.c b/drivers/media/platform/sun6i-csi/sun6i_video.c
>>> new file mode 100644
>>> index 0000000..0c5dbd2
>>> --- /dev/null
>>> +++ b/drivers/media/platform/sun6i-csi/sun6i_video.c
>>> @@ -0,0 +1,663 @@
>>> +/*
>>> + * Copyright (c) 2017 Magewell Electronics Co., Ltd. (Nanjing).
>>> + * All rights reserved.
>>> + * Author: Yong Deng <yong.deng@magewell.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/of.h>
>>> +
>>> +#include <media/v4l2-device.h>
>>> +#include <media/v4l2-ioctl.h>
>>> +#include <media/v4l2-mc.h>
>>> +#include <media/videobuf2-dma-contig.h>
>>> +#include <media/videobuf2-v4l2.h>
>>> +
>>> +#include "sun6i_csi.h"
>>> +#include "sun6i_video.h"
>>> +
>>> +struct sun6i_csi_buffer {
>>> +	struct vb2_v4l2_buffer		vb;
>>> +	struct list_head		list;
>>> +
>>> +	dma_addr_t			dma_addr;
>>> +};
>>> +
>>> +static struct sun6i_csi_format *
>>> +find_format_by_fourcc(struct sun6i_video *video, unsigned int fourcc)
>>> +{
>>> +	unsigned int num_formats = video->num_formats;
>>> +	struct sun6i_csi_format *fmt;
>>> +	unsigned int i;
>>> +
>>> +	for (i = 0; i < num_formats; i++) {
>>> +		fmt = &video->formats[i];
>>> +		if (fmt->fourcc == fourcc)
>>> +			return fmt;
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>> +static struct v4l2_subdev *
>>> +sun6i_video_remote_subdev(struct sun6i_video *video, u32 *pad)
>>> +{
>>> +	struct media_pad *remote;
>>> +
>>> +	remote = media_entity_remote_pad(&video->pad);
>>> +
>>> +	if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
>>> +		return NULL;
>>> +
>>> +	if (pad)
>>> +		*pad = remote->index;
>>> +
>>> +	return media_entity_to_v4l2_subdev(remote->entity);
>>> +}
>>> +
>>> +static int sun6i_video_queue_setup(struct vb2_queue *vq,
>>> +				 unsigned int *nbuffers, unsigned int *nplanes,
>>> +				 unsigned int sizes[],
>>> +				 struct device *alloc_devs[])
>>> +{
>>> +	struct sun6i_video *video = vb2_get_drv_priv(vq);
>>> +	unsigned int size = video->fmt.fmt.pix.sizeimage;
>>> +
>>> +	if (*nplanes)
>>> +		return sizes[0] < size ? -EINVAL : 0;
>>> +
>>> +	*nplanes = 1;
>>> +	sizes[0] = size;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int sun6i_video_buffer_prepare(struct vb2_buffer *vb)
>>> +{
>>> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>>> +	struct sun6i_csi_buffer *buf =
>>> +			container_of(vbuf, struct sun6i_csi_buffer, vb);
>>> +	struct sun6i_video *video = vb2_get_drv_priv(vb->vb2_queue);
>>> +	unsigned long size = video->fmt.fmt.pix.sizeimage;
>>> +
>>> +	if (vb2_plane_size(vb, 0) < size) {
>>> +		v4l2_err(video->vdev.v4l2_dev, "buffer too small (%lu < %lu)\n",
>>> +			 vb2_plane_size(vb, 0), size);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	vb2_set_plane_payload(vb, 0, size);
>>> +
>>> +	buf->dma_addr = vb2_dma_contig_plane_dma_addr(vb, 0);
>>> +
>>> +	vbuf->field = video->fmt.fmt.pix.field;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int sun6i_pipeline_set_stream(struct sun6i_video *video, bool enable)
>>> +{
>>> +	struct media_entity *entity;
>>> +	struct media_pad *pad;
>>> +	struct v4l2_subdev *subdev;
>>> +	int ret;
>>> +
>>> +	entity = &video->vdev.entity;
>>> +	while (1) {
>>> +		pad = &entity->pads[0];
>>> +		if (!(pad->flags & MEDIA_PAD_FL_SINK))
>>> +			break;
>>> +
>>> +		pad = media_entity_remote_pad(pad);
>>> +		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
>>> +			break;
>>> +
>>> +		entity = pad->entity;
>>> +		subdev = media_entity_to_v4l2_subdev(entity);
>>> +
>>> +		ret = v4l2_subdev_call(subdev, video, s_stream, enable);
>>> +		if (enable && ret < 0 && ret != -ENOIOCTLCMD)
>>> +			return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int sun6i_video_start_streaming(struct vb2_queue *vq, unsigned int count)
>>> +{
>>> +	struct sun6i_video *video = vb2_get_drv_priv(vq);
>>> +	struct sun6i_csi_buffer *buf;
>>> +	struct sun6i_csi_config config;
>>> +	unsigned long flags;
>>> +	int ret;
>>> +
>>> +	video->sequence = 0;
>>> +
>>> +	ret = media_pipeline_start(&video->vdev.entity, &video->vdev.pipe);
>>> +	if (ret < 0)
>>> +		goto err_start_pipeline;
>>> +
>>> +	ret = sun6i_pipeline_set_stream(video, true);
>>> +	if (ret < 0)
>>> +		goto err_start_stream;
>>> +
>>> +	config.pixelformat = video->fmt.fmt.pix.pixelformat;
>>> +	config.code = video->current_fmt->mbus_code;
>>> +	config.field = video->fmt.fmt.pix.field;
>>> +	config.width = video->fmt.fmt.pix.width;
>>> +	config.height = video->fmt.fmt.pix.height;
>>> +
>>> +	ret = sun6i_csi_update_config(video->csi, &config);
>>> +	if (ret < 0)
>>> +		goto err_update_config;
>>> +
>>> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
>>> +	video->cur_frm = list_first_entry(&video->dma_queue,
>>> +					  struct sun6i_csi_buffer, list);
>>> +	list_del(&video->cur_frm->list);
>>> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
>>> +
>>> +	ret = sun6i_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
>>> +	if (ret < 0)
>>> +		goto err_update_addr;
>>> +
>>> +	ret = sun6i_csi_set_stream(video->csi, true);
>>> +	if (ret < 0)
>>> +		goto err_csi_stream;
>>> +
>>> +	return 0;
>>> +
>>> +err_csi_stream:
>>> +err_update_addr:
>>> +err_update_config:
>>> +	sun6i_pipeline_set_stream(video, false);
>>> +err_start_stream:
>>> +	media_pipeline_stop(&video->vdev.entity);
>>> +err_start_pipeline:
>>> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
>>> +	list_for_each_entry(buf, &video->dma_queue, list)
>>> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
>>> +	INIT_LIST_HEAD(&video->dma_queue);
>>> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static void sun6i_video_stop_streaming(struct vb2_queue *vq)
>>> +{
>>> +	struct sun6i_video *video = vb2_get_drv_priv(vq);
>>> +	unsigned long flags;
>>> +	struct sun6i_csi_buffer *buf;
>>> +
>>> +	sun6i_pipeline_set_stream(video, false);
>>> +
>>> +	sun6i_csi_set_stream(video->csi, false);
>>> +
>>> +	media_pipeline_stop(&video->vdev.entity);
>>> +
>>> +	/* Release all active buffers */
>>> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
>>> +	if (unlikely(video->cur_frm)) {
>>> +		vb2_buffer_done(&video->cur_frm->vb.vb2_buf,
>>> +				VB2_BUF_STATE_ERROR);
>>> +		video->cur_frm = NULL;
>>> +	}
>>> +	list_for_each_entry(buf, &video->dma_queue, list)
>>> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>>> +	INIT_LIST_HEAD(&video->dma_queue);
>>> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
>>> +}
>>> +
>>> +static void sun6i_video_buffer_queue(struct vb2_buffer *vb)
>>> +{
>>> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>>> +	struct sun6i_csi_buffer *buf =
>>> +			container_of(vbuf, struct sun6i_csi_buffer, vb);
>>> +	struct sun6i_video *video = vb2_get_drv_priv(vb->vb2_queue);
>>> +	unsigned long flags;
>>> +
>>> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
>>> +	if (!video->cur_frm && list_empty(&video->dma_queue) &&
>>> +		vb2_is_streaming(vb->vb2_queue)) {
>>> +		video->cur_frm = buf;
>>> +		sun6i_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
>>> +		sun6i_csi_set_stream(video->csi, 1);
>>> +	} else
>>> +		list_add_tail(&buf->list, &video->dma_queue);
>>> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
>>> +}
>>> +
>>> +void sun6i_video_frame_done(struct sun6i_video *video)
>>> +{
>>> +	spin_lock(&video->dma_queue_lock);
>>> +
>>> +	if (video->cur_frm) {
>>> +		struct vb2_v4l2_buffer *vbuf = &video->cur_frm->vb;
>>> +		struct vb2_buffer *vb = &vbuf->vb2_buf;
>>> +
>>> +		vb->timestamp = ktime_get_ns();
>>> +		vbuf->sequence = video->sequence++;
>>> +		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
>>> +		video->cur_frm = NULL;
>>> +	}
>>> +
>>> +	if (!list_empty(&video->dma_queue)
>>> +	    && vb2_is_streaming(&video->vb2_vidq)) {
>>> +		video->cur_frm = list_first_entry(&video->dma_queue,
>>> +				struct sun6i_csi_buffer, list);
>>> +		list_del(&video->cur_frm->list);
>>> +		sun6i_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
>>> +	} else
>>> +		sun6i_csi_set_stream(video->csi, 0);
>>> +
>>> +	spin_unlock(&video->dma_queue_lock);
>>> +}
>>> +
>>> +static struct vb2_ops sun6i_csi_vb2_ops = {
>>> +	.queue_setup		= sun6i_video_queue_setup,
>>> +	.wait_prepare		= vb2_ops_wait_prepare,
>>> +	.wait_finish		= vb2_ops_wait_finish,
>>> +	.buf_prepare		= sun6i_video_buffer_prepare,
>>> +	.start_streaming	= sun6i_video_start_streaming,
>>> +	.stop_streaming		= sun6i_video_stop_streaming,
>>> +	.buf_queue		= sun6i_video_buffer_queue,
>>> +};
>>> +
>>> +static int vidioc_querycap(struct file *file, void *priv,
>>> +				struct v4l2_capability *cap)
>>> +{
>>> +	struct sun6i_video *video = video_drvdata(file);
>>> +
>>> +	strlcpy(cap->driver, "sun6i-video", sizeof(cap->driver));
>>> +	strlcpy(cap->card, video->vdev.name, sizeof(cap->card));
>>> +	snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
>>> +		 video->csi->dev->of_node->name);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv,
>>> +				   struct v4l2_fmtdesc *f)
>>> +{
>>> +	struct sun6i_video *video = video_drvdata(file);
>>> +	u32 index = f->index;
>>> +
>>> +	if (index >= video->num_formats)
>>> +		return -EINVAL;
>>> +
>>> +	f->pixelformat = video->formats[index].fourcc;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
>>> +				struct v4l2_format *fmt)
>>> +{
>>> +	struct sun6i_video *video = video_drvdata(file);
>>> +
>>> +	*fmt = video->fmt;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int sun6i_video_try_fmt(struct sun6i_video *video, struct v4l2_format *f,
>>> +			       struct sun6i_csi_format **current_fmt)
>>> +{
>>> +	struct sun6i_csi_format *csi_fmt;
>>> +	struct v4l2_pix_format *pixfmt = &f->fmt.pix;
>>> +	struct v4l2_subdev_format format;
>>> +	struct v4l2_subdev *subdev;
>>> +	u32 pad;
>>> +	int ret;
>>> +
>>> +	subdev = sun6i_video_remote_subdev(video, &pad);
>>> +	if (subdev == NULL)
>>> +		return -ENXIO;
>>> +
>>> +	csi_fmt = find_format_by_fourcc(video, pixfmt->pixelformat);
>>> +	if (csi_fmt == NULL)
>>> +		return -EINVAL;
>>> +
>>> +	format.pad = pad;
>>> +	format.which = V4L2_SUBDEV_FORMAT_TRY;
>>> +	v4l2_fill_mbus_format(&format.format, pixfmt, csi_fmt->mbus_code);
>>> +	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &format);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	v4l2_fill_pix_format(pixfmt, &format.format);
>>> +
>>> +	pixfmt->bytesperline = (pixfmt->width * csi_fmt->bpp) >> 3;
>>> +	pixfmt->sizeimage = (pixfmt->width * csi_fmt->bpp * pixfmt->height) / 8;
>>> +
>>> +	if (current_fmt)
>>> +		*current_fmt = csi_fmt;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int sun6i_video_set_fmt(struct sun6i_video *video, struct v4l2_format *f)
>>> +{
>>> +	struct v4l2_subdev_format format;
>>> +	struct sun6i_csi_format *current_fmt;
>>> +	struct v4l2_subdev *subdev;
>>> +	u32 pad;
>>> +	int ret;
>>> +
>>> +	subdev = sun6i_video_remote_subdev(video, &pad);
>>> +	if (subdev == NULL)
>>> +		return -ENXIO;
>>> +
>>> +	ret = sun6i_video_try_fmt(video, f, &current_fmt);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>>> +	v4l2_fill_mbus_format(&format.format, &f->fmt.pix,
>>> +			      current_fmt->mbus_code);
>>> +	ret = v4l2_subdev_call(subdev, pad, set_fmt, NULL, &format);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	video->fmt = *f;
>>> +	video->current_fmt = current_fmt;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
>>> +				struct v4l2_format *f)
>>> +{
>>> +	struct sun6i_video *video = video_drvdata(file);
>>> +
>>> +	if (vb2_is_streaming(&video->vb2_vidq))
>>
>> This should be vb2_is_busy(). Once buffers are allocated you are no longer allowed
>> to change the format, regardless of whether you are streaming or not.
> 
> OK.
> 
>>
>>> +		return -EBUSY;
>>> +
>>> +	return sun6i_video_set_fmt(video, f);
>>> +}
>>> +
>>> +static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
>>> +				  struct v4l2_format *f)
>>> +{
>>> +	struct sun6i_video *video = video_drvdata(file);
>>> +
>>> +	return sun6i_video_try_fmt(video, f, NULL);
>>> +}
>>> +
>>> +static const struct v4l2_ioctl_ops sun6i_video_ioctl_ops = {
>>> +	.vidioc_querycap		= vidioc_querycap,
>>> +	.vidioc_enum_fmt_vid_cap	= vidioc_enum_fmt_vid_cap,
>>> +	.vidioc_g_fmt_vid_cap		= vidioc_g_fmt_vid_cap,
>>> +	.vidioc_s_fmt_vid_cap		= vidioc_s_fmt_vid_cap,
>>> +	.vidioc_try_fmt_vid_cap		= vidioc_try_fmt_vid_cap,
>>> +
>>> +	.vidioc_reqbufs			= vb2_ioctl_reqbufs,
>>> +	.vidioc_querybuf		= vb2_ioctl_querybuf,
>>> +	.vidioc_qbuf			= vb2_ioctl_qbuf,
>>> +	.vidioc_expbuf			= vb2_ioctl_expbuf,
>>> +	.vidioc_dqbuf			= vb2_ioctl_dqbuf,
>>> +	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
>>> +	.vidioc_prepare_buf		= vb2_ioctl_prepare_buf,
>>> +	.vidioc_streamon		= vb2_ioctl_streamon,
>>> +	.vidioc_streamoff		= vb2_ioctl_streamoff,
>>> +};
>>> +
>>> +/* -----------------------------------------------------------------------------
>>> + * V4L2 file operations
>>> + */
>>> +static int sun6i_video_open(struct file *file)
>>> +{
>>> +	struct sun6i_video *video = video_drvdata(file);
>>> +	struct v4l2_format format;
>>> +	int ret;
>>> +
>>> +	if (mutex_lock_interruptible(&video->lock))
>>> +		return -ERESTARTSYS;
>>> +
>>> +	ret = v4l2_fh_open(file);
>>> +	if (ret < 0)
>>> +		goto unlock;
>>> +
>>> +	ret = v4l2_pipeline_pm_use(&video->vdev.entity, 1);
>>> +	if (ret < 0)
>>> +		goto fh_release;
>>> +
>>> +	if (!v4l2_fh_is_singular_file(file))
>>> +		goto unlock;
>>> +
>>> +	ret = sun6i_csi_set_power(video->csi, true);
>>> +	if (ret < 0)
>>> +		goto fh_release;
>>> +
>>> +	/* setup default format */
>>> +	if (video->num_formats > 0) {
>>> +		format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>> +		format.fmt.pix.width = 1280;
>>> +		format.fmt.pix.height = 720;
>>> +		format.fmt.pix.pixelformat = video->formats[0].fourcc;
>>> +		sun6i_video_set_fmt(video, &format);
>>> +	}
>>
>> No, this should happen when the driver is initialized. The format is
>> persistent over open()/close() calls so should not be re-initialized
>> here.
> 
> OK.
> 
>>
>>> +
>>> +	mutex_unlock(&video->lock);
>>> +	return 0;
>>> +
>>> +fh_release:
>>> +	v4l2_fh_release(file);
>>> +unlock:
>>> +	mutex_unlock(&video->lock);
>>> +	return ret;
>>> +}
>>> +
>>> +static int sun6i_video_close(struct file *file)
>>> +{
>>> +	struct sun6i_video *video = video_drvdata(file);
>>> +	bool last_fh;
>>> +
>>> +	mutex_lock(&video->lock);
>>> +
>>> +	last_fh = v4l2_fh_is_singular_file(file);
>>> +
>>> +	_vb2_fop_release(file, NULL);
>>> +
>>> +	v4l2_pipeline_pm_use(&video->vdev.entity, 0);
>>> +
>>> +	if (last_fh)
>>> +		sun6i_csi_set_power(video->csi, false);
>>> +
>>> +	mutex_unlock(&video->lock);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct v4l2_file_operations sun6i_video_fops = {
>>> +	.owner		= THIS_MODULE,
>>> +	.open		= sun6i_video_open,
>>> +	.release	= sun6i_video_close,
>>> +	.unlocked_ioctl	= video_ioctl2,
>>> +	.mmap		= vb2_fop_mmap,
>>> +	.poll		= vb2_fop_poll
>>> +};
>>> +
>>> +/* -----------------------------------------------------------------------------
>>> + * Media Operations
>>> + */
>>> +static int sun6i_video_formats_init(struct sun6i_video *video)
>>> +{
>>> +	struct v4l2_subdev_mbus_code_enum mbus_code = { 0 };
>>> +	struct sun6i_csi *csi = video->csi;
>>> +	struct v4l2_subdev *subdev;
>>> +	u32 pad;
>>> +	const u32 *pixformats;
>>> +	int pixformat_count = 0;
>>> +	u32 subdev_codes[32]; /* subdev format codes, 32 should be enough */
>>> +	int codes_count = 0;
>>> +	int num_fmts = 0;
>>> +	int i, j;
>>> +
>>> +	subdev = sun6i_video_remote_subdev(video, &pad);
>>> +	if (subdev == NULL)
>>> +		return -ENXIO;
>>> +
>>> +	/* Get supported pixformats of CSI */
>>> +	pixformat_count = sun6i_csi_get_supported_pixformats(csi, &pixformats);
>>> +	if (pixformat_count <= 0)
>>> +		return -ENXIO;
>>> +
>>> +	/* Get subdev formats codes */
>>> +	mbus_code.pad = pad;
>>> +	mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>>> +	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL,
>>> +			&mbus_code)) {
>>> +		subdev_codes[codes_count] = mbus_code.code;
>>
>> This definitely needs a check when codes_count >= ARRAY_SIZE(subdev_codes).
> 
> OK.
> 
>>
>>> +		codes_count++;
>>> +		mbus_code.index++;
>>> +	}
>>> +
>>> +	if (!codes_count)
>>> +		return -ENXIO;
>>> +
>>> +	/* Get supported formats count */
>>> +	for (i = 0; i < codes_count; i++) {
>>> +		for (j = 0; j < pixformat_count; j++) {
>>> +			if (!sun6i_csi_is_format_support(csi, pixformats[j],
>>> +					mbus_code.code)) {
>>> +				continue;
>>> +			}
>>> +			num_fmts++;
>>> +		}
>>> +	}
>>> +
>>> +	if (!num_fmts)
>>> +		return -ENXIO;
>>> +
>>> +	video->num_formats = num_fmts;
>>> +	video->formats = devm_kcalloc(video->csi->dev, num_fmts,
>>> +			sizeof(struct sun6i_csi_format), GFP_KERNEL);
>>> +	if (!video->formats) {
>>> +		dev_err(video->csi->dev, "could not allocate memory\n");
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	/* Get supported formats */
>>> +	num_fmts = 0;
>>> +	for (i = 0; i < codes_count; i++) {
>>> +		for (j = 0; j < pixformat_count; j++) {
>>> +			if (!sun6i_csi_is_format_support(csi, pixformats[j],
>>> +					mbus_code.code)) {
>>> +				continue;
>>> +			}
>>> +
>>> +			video->formats[num_fmts].fourcc = pixformats[j];
>>> +			video->formats[num_fmts].mbus_code =
>>> +					mbus_code.code;
>>> +			video->formats[num_fmts].bpp =
>>> +					v4l2_pixformat_get_bpp(pixformats[j]);
>>> +			num_fmts++;
>>> +		}
>>> +	}
>>
>> As mentioned above, the initial format should probably be configure here.
> 
> OK.
> 
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int sun6i_video_link_setup(struct media_entity *entity,
>>> +				  const struct media_pad *local,
>>> +				  const struct media_pad *remote, u32 flags)
>>> +{
>>> +	struct video_device *vdev = media_entity_to_video_device(entity);
>>> +	struct sun6i_video *video = video_get_drvdata(vdev);
>>> +
>>> +	if (WARN_ON(video == NULL))
>>> +		return 0;
>>> +
>>> +	return sun6i_video_formats_init(video);
>>
>> Why is this called here? Why not in video_init()?
> 
> sun6i_video_init is in the driver probe context. sun6i_video_formats_init
> use media_entity_remote_pad and media_entity_to_v4l2_subdev to find the
> subdevs.
> The media_entity_remote_pad can't work before all the media pad linked.

A video_init is typically called from the notify_complete callback.
Actually, that's where the video_register_device should be created as well.
When you create it in probe() there is possibly no sensor yet, so it would
be a dead video node (or worse, crash when used).

There are still a lot of platform drivers that create the video node in the
probe, but it's not the right place if you rely on the async loading of
subdevs.

Regards,

	Hans

Regards,

	Hans

[toc] | [prev] | [next] | [standalone]

#1717134

On Tue, 22 Aug 2017 08:43:35 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:

> On 08/22/2017 05:01 AM, Yong wrote:
> > Hi Hans,
> > 
> > On Mon, 21 Aug 2017 16:37:41 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > 
> >> Hi Yong,
> >>
> >> First two high-level comments before I start the review:
> >>
> >> 1) Can you provide the v4l2-compliance output? I can't merge this unless I
> >>    see that it passes the compliance tests. Make sure you compile from the git
> >>    repo (https://git.linuxtv.org/v4l-utils.git/) so you are using the latest
> >>    version of the compliance test. Test with just 'v4l2-compliance' and also
> >>    with the -s option (to test streaming) and with the -f option (to test all
> >>    the various pixel formats).
> > 
> > OK. I will post with the next version.
> > 
> >>
> >> 2) I see you support interlaced formats. Did you actually test that? Interlaced
> >>    is tricky and you shouldn't add support for it unless you know it works and
> >>    that it passes the 'v4l2-compliance -s' test.
> > 
> > No. I do not have the condition to test the all formats for now. Maybe this can 
> > be done when ourself's device with V3s is ready in the future months.
> 
> Then just drop the interlaced support until you can test it.

OK. I will try to find a way to test it. If not, I will drop it.

> 
> > 
> >>
> >> On 07/27/2017 07:01 AM, Yong Deng wrote:
> >>> Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> >>> and CSI1 is used for parallel interface. This is not documented in
> >>> datasheet but by testing and guess.
> >>>
> >>> This patch implement a v4l2 framework driver for it.
> >>>
> >>> Currently, the driver only support the parallel interface. MIPI-CSI2,
> >>> ISP's support are not included in this patch.
> >>>
> >>> Signed-off-by: Yong Deng <yong.deng@magewell.com>
> >>> ---
> >>>  drivers/media/platform/Kconfig                   |   1 +
> >>>  drivers/media/platform/Makefile                  |   2 +
> >>>  drivers/media/platform/sun6i-csi/Kconfig         |   9 +
> >>>  drivers/media/platform/sun6i-csi/Makefile        |   3 +
> >>>  drivers/media/platform/sun6i-csi/sun6i_csi.c     | 545 +++++++++++++++
> >>>  drivers/media/platform/sun6i-csi/sun6i_csi.h     | 203 ++++++
> >>>  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c | 827 +++++++++++++++++++++++
> >>>  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h | 206 ++++++
> >>>  drivers/media/platform/sun6i-csi/sun6i_video.c   | 663 ++++++++++++++++++
> >>>  drivers/media/platform/sun6i-csi/sun6i_video.h   |  61 ++
> >>>  10 files changed, 2520 insertions(+)
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/Kconfig
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/Makefile
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.c
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.h
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.c
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.h
> >>>
> >>
> >> <snip>
> >>
> >>> diff --git a/drivers/media/platform/sun6i-csi/sun6i_video.c b/drivers/media/platform/sun6i-csi/sun6i_video.c
> >>> new file mode 100644
> >>> index 0000000..0c5dbd2
> >>> --- /dev/null
> >>> +++ b/drivers/media/platform/sun6i-csi/sun6i_video.c
> >>> @@ -0,0 +1,663 @@
> >>> +/*
> >>> + * Copyright (c) 2017 Magewell Electronics Co., Ltd. (Nanjing).
> >>> + * All rights reserved.
> >>> + * Author: Yong Deng <yong.deng@magewell.com>
> >>> + *
> >>> + * This program is free software; you can redistribute it and/or modify
> >>> + * it under the terms of the GNU General Public License version 2 as
> >>> + * published by the Free Software Foundation.
> >>> + *
> >>> + * This program is distributed in the hope that it will be useful,
> >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>> + * GNU General Public License for more details.
> >>> + */
> >>> +
> >>> +#include <linux/of.h>
> >>> +
> >>> +#include <media/v4l2-device.h>
> >>> +#include <media/v4l2-ioctl.h>
> >>> +#include <media/v4l2-mc.h>
> >>> +#include <media/videobuf2-dma-contig.h>
> >>> +#include <media/videobuf2-v4l2.h>
> >>> +
> >>> +#include "sun6i_csi.h"
> >>> +#include "sun6i_video.h"
> >>> +
> >>> +struct sun6i_csi_buffer {
> >>> +	struct vb2_v4l2_buffer		vb;
> >>> +	struct list_head		list;
> >>> +
> >>> +	dma_addr_t			dma_addr;
> >>> +};
> >>> +
> >>> +static struct sun6i_csi_format *
> >>> +find_format_by_fourcc(struct sun6i_video *video, unsigned int fourcc)
> >>> +{
> >>> +	unsigned int num_formats = video->num_formats;
> >>> +	struct sun6i_csi_format *fmt;
> >>> +	unsigned int i;
> >>> +
> >>> +	for (i = 0; i < num_formats; i++) {
> >>> +		fmt = &video->formats[i];
> >>> +		if (fmt->fourcc == fourcc)
> >>> +			return fmt;
> >>> +	}
> >>> +
> >>> +	return NULL;
> >>> +}
> >>> +
> >>> +static struct v4l2_subdev *
> >>> +sun6i_video_remote_subdev(struct sun6i_video *video, u32 *pad)
> >>> +{
> >>> +	struct media_pad *remote;
> >>> +
> >>> +	remote = media_entity_remote_pad(&video->pad);
> >>> +
> >>> +	if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
> >>> +		return NULL;
> >>> +
> >>> +	if (pad)
> >>> +		*pad = remote->index;
> >>> +
> >>> +	return media_entity_to_v4l2_subdev(remote->entity);
> >>> +}
> >>> +
> >>> +static int sun6i_video_queue_setup(struct vb2_queue *vq,
> >>> +				 unsigned int *nbuffers, unsigned int *nplanes,
> >>> +				 unsigned int sizes[],
> >>> +				 struct device *alloc_devs[])
> >>> +{
> >>> +	struct sun6i_video *video = vb2_get_drv_priv(vq);
> >>> +	unsigned int size = video->fmt.fmt.pix.sizeimage;
> >>> +
> >>> +	if (*nplanes)
> >>> +		return sizes[0] < size ? -EINVAL : 0;
> >>> +
> >>> +	*nplanes = 1;
> >>> +	sizes[0] = size;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int sun6i_video_buffer_prepare(struct vb2_buffer *vb)
> >>> +{
> >>> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> >>> +	struct sun6i_csi_buffer *buf =
> >>> +			container_of(vbuf, struct sun6i_csi_buffer, vb);
> >>> +	struct sun6i_video *video = vb2_get_drv_priv(vb->vb2_queue);
> >>> +	unsigned long size = video->fmt.fmt.pix.sizeimage;
> >>> +
> >>> +	if (vb2_plane_size(vb, 0) < size) {
> >>> +		v4l2_err(video->vdev.v4l2_dev, "buffer too small (%lu < %lu)\n",
> >>> +			 vb2_plane_size(vb, 0), size);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	vb2_set_plane_payload(vb, 0, size);
> >>> +
> >>> +	buf->dma_addr = vb2_dma_contig_plane_dma_addr(vb, 0);
> >>> +
> >>> +	vbuf->field = video->fmt.fmt.pix.field;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int sun6i_pipeline_set_stream(struct sun6i_video *video, bool enable)
> >>> +{
> >>> +	struct media_entity *entity;
> >>> +	struct media_pad *pad;
> >>> +	struct v4l2_subdev *subdev;
> >>> +	int ret;
> >>> +
> >>> +	entity = &video->vdev.entity;
> >>> +	while (1) {
> >>> +		pad = &entity->pads[0];
> >>> +		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> >>> +			break;
> >>> +
> >>> +		pad = media_entity_remote_pad(pad);
> >>> +		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> >>> +			break;
> >>> +
> >>> +		entity = pad->entity;
> >>> +		subdev = media_entity_to_v4l2_subdev(entity);
> >>> +
> >>> +		ret = v4l2_subdev_call(subdev, video, s_stream, enable);
> >>> +		if (enable && ret < 0 && ret != -ENOIOCTLCMD)
> >>> +			return ret;
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int sun6i_video_start_streaming(struct vb2_queue *vq, unsigned int count)
> >>> +{
> >>> +	struct sun6i_video *video = vb2_get_drv_priv(vq);
> >>> +	struct sun6i_csi_buffer *buf;
> >>> +	struct sun6i_csi_config config;
> >>> +	unsigned long flags;
> >>> +	int ret;
> >>> +
> >>> +	video->sequence = 0;
> >>> +
> >>> +	ret = media_pipeline_start(&video->vdev.entity, &video->vdev.pipe);
> >>> +	if (ret < 0)
> >>> +		goto err_start_pipeline;
> >>> +
> >>> +	ret = sun6i_pipeline_set_stream(video, true);
> >>> +	if (ret < 0)
> >>> +		goto err_start_stream;
> >>> +
> >>> +	config.pixelformat = video->fmt.fmt.pix.pixelformat;
> >>> +	config.code = video->current_fmt->mbus_code;
> >>> +	config.field = video->fmt.fmt.pix.field;
> >>> +	config.width = video->fmt.fmt.pix.width;
> >>> +	config.height = video->fmt.fmt.pix.height;
> >>> +
> >>> +	ret = sun6i_csi_update_config(video->csi, &config);
> >>> +	if (ret < 0)
> >>> +		goto err_update_config;
> >>> +
> >>> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> >>> +	video->cur_frm = list_first_entry(&video->dma_queue,
> >>> +					  struct sun6i_csi_buffer, list);
> >>> +	list_del(&video->cur_frm->list);
> >>> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> >>> +
> >>> +	ret = sun6i_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
> >>> +	if (ret < 0)
> >>> +		goto err_update_addr;
> >>> +
> >>> +	ret = sun6i_csi_set_stream(video->csi, true);
> >>> +	if (ret < 0)
> >>> +		goto err_csi_stream;
> >>> +
> >>> +	return 0;
> >>> +
> >>> +err_csi_stream:
> >>> +err_update_addr:
> >>> +err_update_config:
> >>> +	sun6i_pipeline_set_stream(video, false);
> >>> +err_start_stream:
> >>> +	media_pipeline_stop(&video->vdev.entity);
> >>> +err_start_pipeline:
> >>> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> >>> +	list_for_each_entry(buf, &video->dma_queue, list)
> >>> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
> >>> +	INIT_LIST_HEAD(&video->dma_queue);
> >>> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> >>> +
> >>> +	return ret;
> >>> +}
> >>> +
> >>> +static void sun6i_video_stop_streaming(struct vb2_queue *vq)
> >>> +{
> >>> +	struct sun6i_video *video = vb2_get_drv_priv(vq);
> >>> +	unsigned long flags;
> >>> +	struct sun6i_csi_buffer *buf;
> >>> +
> >>> +	sun6i_pipeline_set_stream(video, false);
> >>> +
> >>> +	sun6i_csi_set_stream(video->csi, false);
> >>> +
> >>> +	media_pipeline_stop(&video->vdev.entity);
> >>> +
> >>> +	/* Release all active buffers */
> >>> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> >>> +	if (unlikely(video->cur_frm)) {
> >>> +		vb2_buffer_done(&video->cur_frm->vb.vb2_buf,
> >>> +				VB2_BUF_STATE_ERROR);
> >>> +		video->cur_frm = NULL;
> >>> +	}
> >>> +	list_for_each_entry(buf, &video->dma_queue, list)
> >>> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> >>> +	INIT_LIST_HEAD(&video->dma_queue);
> >>> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> >>> +}
> >>> +
> >>> +static void sun6i_video_buffer_queue(struct vb2_buffer *vb)
> >>> +{
> >>> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> >>> +	struct sun6i_csi_buffer *buf =
> >>> +			container_of(vbuf, struct sun6i_csi_buffer, vb);
> >>> +	struct sun6i_video *video = vb2_get_drv_priv(vb->vb2_queue);
> >>> +	unsigned long flags;
> >>> +
> >>> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> >>> +	if (!video->cur_frm && list_empty(&video->dma_queue) &&
> >>> +		vb2_is_streaming(vb->vb2_queue)) {
> >>> +		video->cur_frm = buf;
> >>> +		sun6i_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
> >>> +		sun6i_csi_set_stream(video->csi, 1);
> >>> +	} else
> >>> +		list_add_tail(&buf->list, &video->dma_queue);
> >>> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> >>> +}
> >>> +
> >>> +void sun6i_video_frame_done(struct sun6i_video *video)
> >>> +{
> >>> +	spin_lock(&video->dma_queue_lock);
> >>> +
> >>> +	if (video->cur_frm) {
> >>> +		struct vb2_v4l2_buffer *vbuf = &video->cur_frm->vb;
> >>> +		struct vb2_buffer *vb = &vbuf->vb2_buf;
> >>> +
> >>> +		vb->timestamp = ktime_get_ns();
> >>> +		vbuf->sequence = video->sequence++;
> >>> +		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> >>> +		video->cur_frm = NULL;
> >>> +	}
> >>> +
> >>> +	if (!list_empty(&video->dma_queue)
> >>> +	    && vb2_is_streaming(&video->vb2_vidq)) {
> >>> +		video->cur_frm = list_first_entry(&video->dma_queue,
> >>> +				struct sun6i_csi_buffer, list);
> >>> +		list_del(&video->cur_frm->list);
> >>> +		sun6i_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
> >>> +	} else
> >>> +		sun6i_csi_set_stream(video->csi, 0);
> >>> +
> >>> +	spin_unlock(&video->dma_queue_lock);
> >>> +}
> >>> +
> >>> +static struct vb2_ops sun6i_csi_vb2_ops = {
> >>> +	.queue_setup		= sun6i_video_queue_setup,
> >>> +	.wait_prepare		= vb2_ops_wait_prepare,
> >>> +	.wait_finish		= vb2_ops_wait_finish,
> >>> +	.buf_prepare		= sun6i_video_buffer_prepare,
> >>> +	.start_streaming	= sun6i_video_start_streaming,
> >>> +	.stop_streaming		= sun6i_video_stop_streaming,
> >>> +	.buf_queue		= sun6i_video_buffer_queue,
> >>> +};
> >>> +
> >>> +static int vidioc_querycap(struct file *file, void *priv,
> >>> +				struct v4l2_capability *cap)
> >>> +{
> >>> +	struct sun6i_video *video = video_drvdata(file);
> >>> +
> >>> +	strlcpy(cap->driver, "sun6i-video", sizeof(cap->driver));
> >>> +	strlcpy(cap->card, video->vdev.name, sizeof(cap->card));
> >>> +	snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> >>> +		 video->csi->dev->of_node->name);
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv,
> >>> +				   struct v4l2_fmtdesc *f)
> >>> +{
> >>> +	struct sun6i_video *video = video_drvdata(file);
> >>> +	u32 index = f->index;
> >>> +
> >>> +	if (index >= video->num_formats)
> >>> +		return -EINVAL;
> >>> +
> >>> +	f->pixelformat = video->formats[index].fourcc;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
> >>> +				struct v4l2_format *fmt)
> >>> +{
> >>> +	struct sun6i_video *video = video_drvdata(file);
> >>> +
> >>> +	*fmt = video->fmt;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int sun6i_video_try_fmt(struct sun6i_video *video, struct v4l2_format *f,
> >>> +			       struct sun6i_csi_format **current_fmt)
> >>> +{
> >>> +	struct sun6i_csi_format *csi_fmt;
> >>> +	struct v4l2_pix_format *pixfmt = &f->fmt.pix;
> >>> +	struct v4l2_subdev_format format;
> >>> +	struct v4l2_subdev *subdev;
> >>> +	u32 pad;
> >>> +	int ret;
> >>> +
> >>> +	subdev = sun6i_video_remote_subdev(video, &pad);
> >>> +	if (subdev == NULL)
> >>> +		return -ENXIO;
> >>> +
> >>> +	csi_fmt = find_format_by_fourcc(video, pixfmt->pixelformat);
> >>> +	if (csi_fmt == NULL)
> >>> +		return -EINVAL;
> >>> +
> >>> +	format.pad = pad;
> >>> +	format.which = V4L2_SUBDEV_FORMAT_TRY;
> >>> +	v4l2_fill_mbus_format(&format.format, pixfmt, csi_fmt->mbus_code);
> >>> +	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &format);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	v4l2_fill_pix_format(pixfmt, &format.format);
> >>> +
> >>> +	pixfmt->bytesperline = (pixfmt->width * csi_fmt->bpp) >> 3;
> >>> +	pixfmt->sizeimage = (pixfmt->width * csi_fmt->bpp * pixfmt->height) / 8;
> >>> +
> >>> +	if (current_fmt)
> >>> +		*current_fmt = csi_fmt;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int sun6i_video_set_fmt(struct sun6i_video *video, struct v4l2_format *f)
> >>> +{
> >>> +	struct v4l2_subdev_format format;
> >>> +	struct sun6i_csi_format *current_fmt;
> >>> +	struct v4l2_subdev *subdev;
> >>> +	u32 pad;
> >>> +	int ret;
> >>> +
> >>> +	subdev = sun6i_video_remote_subdev(video, &pad);
> >>> +	if (subdev == NULL)
> >>> +		return -ENXIO;
> >>> +
> >>> +	ret = sun6i_video_try_fmt(video, f, &current_fmt);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >>> +	v4l2_fill_mbus_format(&format.format, &f->fmt.pix,
> >>> +			      current_fmt->mbus_code);
> >>> +	ret = v4l2_subdev_call(subdev, pad, set_fmt, NULL, &format);
> >>> +	if (ret < 0)
> >>> +		return ret;
> >>> +
> >>> +	video->fmt = *f;
> >>> +	video->current_fmt = current_fmt;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
> >>> +				struct v4l2_format *f)
> >>> +{
> >>> +	struct sun6i_video *video = video_drvdata(file);
> >>> +
> >>> +	if (vb2_is_streaming(&video->vb2_vidq))
> >>
> >> This should be vb2_is_busy(). Once buffers are allocated you are no longer allowed
> >> to change the format, regardless of whether you are streaming or not.
> > 
> > OK.
> > 
> >>
> >>> +		return -EBUSY;
> >>> +
> >>> +	return sun6i_video_set_fmt(video, f);
> >>> +}
> >>> +
> >>> +static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
> >>> +				  struct v4l2_format *f)
> >>> +{
> >>> +	struct sun6i_video *video = video_drvdata(file);
> >>> +
> >>> +	return sun6i_video_try_fmt(video, f, NULL);
> >>> +}
> >>> +
> >>> +static const struct v4l2_ioctl_ops sun6i_video_ioctl_ops = {
> >>> +	.vidioc_querycap		= vidioc_querycap,
> >>> +	.vidioc_enum_fmt_vid_cap	= vidioc_enum_fmt_vid_cap,
> >>> +	.vidioc_g_fmt_vid_cap		= vidioc_g_fmt_vid_cap,
> >>> +	.vidioc_s_fmt_vid_cap		= vidioc_s_fmt_vid_cap,
> >>> +	.vidioc_try_fmt_vid_cap		= vidioc_try_fmt_vid_cap,
> >>> +
> >>> +	.vidioc_reqbufs			= vb2_ioctl_reqbufs,
> >>> +	.vidioc_querybuf		= vb2_ioctl_querybuf,
> >>> +	.vidioc_qbuf			= vb2_ioctl_qbuf,
> >>> +	.vidioc_expbuf			= vb2_ioctl_expbuf,
> >>> +	.vidioc_dqbuf			= vb2_ioctl_dqbuf,
> >>> +	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
> >>> +	.vidioc_prepare_buf		= vb2_ioctl_prepare_buf,
> >>> +	.vidioc_streamon		= vb2_ioctl_streamon,
> >>> +	.vidioc_streamoff		= vb2_ioctl_streamoff,
> >>> +};
> >>> +
> >>> +/* -----------------------------------------------------------------------------
> >>> + * V4L2 file operations
> >>> + */
> >>> +static int sun6i_video_open(struct file *file)
> >>> +{
> >>> +	struct sun6i_video *video = video_drvdata(file);
> >>> +	struct v4l2_format format;
> >>> +	int ret;
> >>> +
> >>> +	if (mutex_lock_interruptible(&video->lock))
> >>> +		return -ERESTARTSYS;
> >>> +
> >>> +	ret = v4l2_fh_open(file);
> >>> +	if (ret < 0)
> >>> +		goto unlock;
> >>> +
> >>> +	ret = v4l2_pipeline_pm_use(&video->vdev.entity, 1);
> >>> +	if (ret < 0)
> >>> +		goto fh_release;
> >>> +
> >>> +	if (!v4l2_fh_is_singular_file(file))
> >>> +		goto unlock;
> >>> +
> >>> +	ret = sun6i_csi_set_power(video->csi, true);
> >>> +	if (ret < 0)
> >>> +		goto fh_release;
> >>> +
> >>> +	/* setup default format */
> >>> +	if (video->num_formats > 0) {
> >>> +		format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >>> +		format.fmt.pix.width = 1280;
> >>> +		format.fmt.pix.height = 720;
> >>> +		format.fmt.pix.pixelformat = video->formats[0].fourcc;
> >>> +		sun6i_video_set_fmt(video, &format);
> >>> +	}
> >>
> >> No, this should happen when the driver is initialized. The format is
> >> persistent over open()/close() calls so should not be re-initialized
> >> here.
> > 
> > OK.
> > 
> >>
> >>> +
> >>> +	mutex_unlock(&video->lock);
> >>> +	return 0;
> >>> +
> >>> +fh_release:
> >>> +	v4l2_fh_release(file);
> >>> +unlock:
> >>> +	mutex_unlock(&video->lock);
> >>> +	return ret;
> >>> +}
> >>> +
> >>> +static int sun6i_video_close(struct file *file)
> >>> +{
> >>> +	struct sun6i_video *video = video_drvdata(file);
> >>> +	bool last_fh;
> >>> +
> >>> +	mutex_lock(&video->lock);
> >>> +
> >>> +	last_fh = v4l2_fh_is_singular_file(file);
> >>> +
> >>> +	_vb2_fop_release(file, NULL);
> >>> +
> >>> +	v4l2_pipeline_pm_use(&video->vdev.entity, 0);
> >>> +
> >>> +	if (last_fh)
> >>> +		sun6i_csi_set_power(video->csi, false);
> >>> +
> >>> +	mutex_unlock(&video->lock);
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static const struct v4l2_file_operations sun6i_video_fops = {
> >>> +	.owner		= THIS_MODULE,
> >>> +	.open		= sun6i_video_open,
> >>> +	.release	= sun6i_video_close,
> >>> +	.unlocked_ioctl	= video_ioctl2,
> >>> +	.mmap		= vb2_fop_mmap,
> >>> +	.poll		= vb2_fop_poll
> >>> +};
> >>> +
> >>> +/* -----------------------------------------------------------------------------
> >>> + * Media Operations
> >>> + */
> >>> +static int sun6i_video_formats_init(struct sun6i_video *video)
> >>> +{
> >>> +	struct v4l2_subdev_mbus_code_enum mbus_code = { 0 };
> >>> +	struct sun6i_csi *csi = video->csi;
> >>> +	struct v4l2_subdev *subdev;
> >>> +	u32 pad;
> >>> +	const u32 *pixformats;
> >>> +	int pixformat_count = 0;
> >>> +	u32 subdev_codes[32]; /* subdev format codes, 32 should be enough */
> >>> +	int codes_count = 0;
> >>> +	int num_fmts = 0;
> >>> +	int i, j;
> >>> +
> >>> +	subdev = sun6i_video_remote_subdev(video, &pad);
> >>> +	if (subdev == NULL)
> >>> +		return -ENXIO;
> >>> +
> >>> +	/* Get supported pixformats of CSI */
> >>> +	pixformat_count = sun6i_csi_get_supported_pixformats(csi, &pixformats);
> >>> +	if (pixformat_count <= 0)
> >>> +		return -ENXIO;
> >>> +
> >>> +	/* Get subdev formats codes */
> >>> +	mbus_code.pad = pad;
> >>> +	mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >>> +	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL,
> >>> +			&mbus_code)) {
> >>> +		subdev_codes[codes_count] = mbus_code.code;
> >>
> >> This definitely needs a check when codes_count >= ARRAY_SIZE(subdev_codes).
> > 
> > OK.
> > 
> >>
> >>> +		codes_count++;
> >>> +		mbus_code.index++;
> >>> +	}
> >>> +
> >>> +	if (!codes_count)
> >>> +		return -ENXIO;
> >>> +
> >>> +	/* Get supported formats count */
> >>> +	for (i = 0; i < codes_count; i++) {
> >>> +		for (j = 0; j < pixformat_count; j++) {
> >>> +			if (!sun6i_csi_is_format_support(csi, pixformats[j],
> >>> +					mbus_code.code)) {
> >>> +				continue;
> >>> +			}
> >>> +			num_fmts++;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	if (!num_fmts)
> >>> +		return -ENXIO;
> >>> +
> >>> +	video->num_formats = num_fmts;
> >>> +	video->formats = devm_kcalloc(video->csi->dev, num_fmts,
> >>> +			sizeof(struct sun6i_csi_format), GFP_KERNEL);
> >>> +	if (!video->formats) {
> >>> +		dev_err(video->csi->dev, "could not allocate memory\n");
> >>> +		return -ENOMEM;
> >>> +	}
> >>> +
> >>> +	/* Get supported formats */
> >>> +	num_fmts = 0;
> >>> +	for (i = 0; i < codes_count; i++) {
> >>> +		for (j = 0; j < pixformat_count; j++) {
> >>> +			if (!sun6i_csi_is_format_support(csi, pixformats[j],
> >>> +					mbus_code.code)) {
> >>> +				continue;
> >>> +			}
> >>> +
> >>> +			video->formats[num_fmts].fourcc = pixformats[j];
> >>> +			video->formats[num_fmts].mbus_code =
> >>> +					mbus_code.code;
> >>> +			video->formats[num_fmts].bpp =
> >>> +					v4l2_pixformat_get_bpp(pixformats[j]);
> >>> +			num_fmts++;
> >>> +		}
> >>> +	}
> >>
> >> As mentioned above, the initial format should probably be configure here.
> > 
> > OK.
> > 
> >>
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int sun6i_video_link_setup(struct media_entity *entity,
> >>> +				  const struct media_pad *local,
> >>> +				  const struct media_pad *remote, u32 flags)
> >>> +{
> >>> +	struct video_device *vdev = media_entity_to_video_device(entity);
> >>> +	struct sun6i_video *video = video_get_drvdata(vdev);
> >>> +
> >>> +	if (WARN_ON(video == NULL))
> >>> +		return 0;
> >>> +
> >>> +	return sun6i_video_formats_init(video);
> >>
> >> Why is this called here? Why not in video_init()?
> > 
> > sun6i_video_init is in the driver probe context. sun6i_video_formats_init
> > use media_entity_remote_pad and media_entity_to_v4l2_subdev to find the
> > subdevs.
> > The media_entity_remote_pad can't work before all the media pad linked.
> 
> A video_init is typically called from the notify_complete callback.
> Actually, that's where the video_register_device should be created as well.
> When you create it in probe() there is possibly no sensor yet, so it would
> be a dead video node (or worse, crash when used).
> 
> There are still a lot of platform drivers that create the video node in the
> probe, but it's not the right place if you rely on the async loading of
> subdevs.

A video_init with media_entity_pads_init must be called before media pad
linking wether is called from probe() or the notify_complete callback.
But, sun6i_video_formats_init must be called after that.

video_register_device is another question. I can seperate it from 
video_init into a new function sun6i_video_register. And call it in
notify_complete callback.

> 
> Regards,
> 
> 	Hans
> 
> Regards,
> 
> 	Hans


Thanks,
Yong

[toc] | [prev] | [next] | [standalone]

#1717805

[Multipart message — attachments visible in raw view] — view raw

On Tue, Aug 22, 2017 at 08:43:35AM +0200, Hans Verkuil wrote:
> >>> +static int sun6i_video_link_setup(struct media_entity *entity,
> >>> +				  const struct media_pad *local,
> >>> +				  const struct media_pad *remote, u32 flags)
> >>> +{
> >>> +	struct video_device *vdev = media_entity_to_video_device(entity);
> >>> +	struct sun6i_video *video = video_get_drvdata(vdev);
> >>> +
> >>> +	if (WARN_ON(video == NULL))
> >>> +		return 0;
> >>> +
> >>> +	return sun6i_video_formats_init(video);
> >>
> >> Why is this called here? Why not in video_init()?
> > 
> > sun6i_video_init is in the driver probe context. sun6i_video_formats_init
> > use media_entity_remote_pad and media_entity_to_v4l2_subdev to find the
> > subdevs.
> > The media_entity_remote_pad can't work before all the media pad linked.
> 
> A video_init is typically called from the notify_complete callback.
> Actually, that's where the video_register_device should be created as well.
> When you create it in probe() there is possibly no sensor yet, so it would
> be a dead video node (or worse, crash when used).
> 
> There are still a lot of platform drivers that create the video node in the
> probe, but it's not the right place if you rely on the async loading of
> subdevs.

That's not really related, but I'm not really sure it's a good way to
operate. This essentially means that you might wait forever for a
component in your pipeline to be probed, without any chance of it
happening (not compiled, compiled as a module and not loaded, hardware
defect preventing the driver from probing properly, etc), even though
that component might not be essential.

This is how DRM operates, and you sometimes end up in some very dumb
situations where you wait for say, the DSI controller to probe, while
you only care about HDMI in your system.

But this seems to be on of the hot topic these days, so we might
discuss it further in some other thread :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1717830

Hello,

On Tuesday, 22 August 2017 23:17:31 EEST Maxime Ripard wrote:
> On Tue, Aug 22, 2017 at 08:43:35AM +0200, Hans Verkuil wrote:
> >>>> +static int sun6i_video_link_setup(struct media_entity *entity,
> >>>> +				  const struct media_pad *local,
> >>>> +				  const struct media_pad *remote, u32 flags)
> >>>> +{
> >>>> +	struct video_device *vdev = media_entity_to_video_device(entity);
> >>>> +	struct sun6i_video *video = video_get_drvdata(vdev);
> >>>> +
> >>>> +	if (WARN_ON(video == NULL))
> >>>> +		return 0;
> >>>> +
> >>>> +	return sun6i_video_formats_init(video);
> >>> 
> >>> Why is this called here? Why not in video_init()?
> >> 
> >> sun6i_video_init is in the driver probe context.
> >> sun6i_video_formats_init use media_entity_remote_pad and
> >> media_entity_to_v4l2_subdev to find the subdevs.
> >> The media_entity_remote_pad can't work before all the media pad linked.
> > 
> > A video_init is typically called from the notify_complete callback.
> > Actually, that's where the video_register_device should be created as
> > well. When you create it in probe() there is possibly no sensor yet, so it
> > would be a dead video node (or worse, crash when used).
> > 
> > There are still a lot of platform drivers that create the video node in
> > the probe, but it's not the right place if you rely on the async loading
> > of subdevs.
> 
> That's not really related, but I'm not really sure it's a good way to
> operate. This essentially means that you might wait forever for a
> component in your pipeline to be probed, without any chance of it
> happening (not compiled, compiled as a module and not loaded, hardware
> defect preventing the driver from probing properly, etc), even though
> that component might not be essential.

I agree with Maxime here, we should build the media device incrementally, and 
offer userspace access early on without waiting for all pieces to be 
available. If properly implemented (there should definitely be so crash) I 
don't see any drawback to that approach.

> This is how DRM operates, and you sometimes end up in some very dumb
> situations where you wait for say, the DSI controller to probe, while
> you only care about HDMI in your system.
> 
> But this seems to be on of the hot topic these days, so we might
> discuss it further in some other thread :)

Or here :-)

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1718087

On 08/22/2017 10:17 PM, Maxime Ripard wrote:
> On Tue, Aug 22, 2017 at 08:43:35AM +0200, Hans Verkuil wrote:
>>>>> +static int sun6i_video_link_setup(struct media_entity *entity,
>>>>> +				  const struct media_pad *local,
>>>>> +				  const struct media_pad *remote, u32 flags)
>>>>> +{
>>>>> +	struct video_device *vdev = media_entity_to_video_device(entity);
>>>>> +	struct sun6i_video *video = video_get_drvdata(vdev);
>>>>> +
>>>>> +	if (WARN_ON(video == NULL))
>>>>> +		return 0;
>>>>> +
>>>>> +	return sun6i_video_formats_init(video);
>>>>
>>>> Why is this called here? Why not in video_init()?
>>>
>>> sun6i_video_init is in the driver probe context. sun6i_video_formats_init
>>> use media_entity_remote_pad and media_entity_to_v4l2_subdev to find the
>>> subdevs.
>>> The media_entity_remote_pad can't work before all the media pad linked.
>>
>> A video_init is typically called from the notify_complete callback.
>> Actually, that's where the video_register_device should be created as well.
>> When you create it in probe() there is possibly no sensor yet, so it would
>> be a dead video node (or worse, crash when used).
>>
>> There are still a lot of platform drivers that create the video node in the
>> probe, but it's not the right place if you rely on the async loading of
>> subdevs.
> 
> That's not really related, but I'm not really sure it's a good way to
> operate. This essentially means that you might wait forever for a
> component in your pipeline to be probed, without any chance of it
> happening (not compiled, compiled as a module and not loaded, hardware
> defect preventing the driver from probing properly, etc), even though
> that component might not be essential.

We're talking straightforward video pipelines here. I.e. a source, some
processing units and a DMA engine at the end. There is no point in
creating a video node if the pipeline is not complete since you need
the full pipeline.

I've had bad experiences in the past where video nodes were created too
soon and part of the internal state was still incomplete, causing at best
weird behavior and at worst crashes.

More complex devices are a whole different ballgame.

Regards,

	Hans

> This is how DRM operates, and you sometimes end up in some very dumb
> situations where you wait for say, the DSI controller to probe, while
> you only care about HDMI in your system.
> 
> But this seems to be on of the hot topic these days, so we might
> discuss it further in some other thread :)
> 
> Maxime
> 

[toc] | [prev] | [next] | [standalone]

#1718113

Hi Hans,

On Wednesday, 23 August 2017 09:52:00 EEST Hans Verkuil wrote:
> On 08/22/2017 10:17 PM, Maxime Ripard wrote:
> > On Tue, Aug 22, 2017 at 08:43:35AM +0200, Hans Verkuil wrote:
> >>>>> +static int sun6i_video_link_setup(struct media_entity *entity,
> >>>>> +				  const struct media_pad *local,
> >>>>> +				  const struct media_pad *remote, u32 flags)
> >>>>> +{
> >>>>> +	struct video_device *vdev = media_entity_to_video_device(entity);
> >>>>> +	struct sun6i_video *video = video_get_drvdata(vdev);
> >>>>> +
> >>>>> +	if (WARN_ON(video == NULL))
> >>>>> +		return 0;
> >>>>> +
> >>>>> +	return sun6i_video_formats_init(video);
> >>>> 
> >>>> Why is this called here? Why not in video_init()?
> >>> 
> >>> sun6i_video_init is in the driver probe context. 
> >>> sun6i_video_formats_init use media_entity_remote_pad and
> >>> media_entity_to_v4l2_subdev to find the subdevs.
> >>> The media_entity_remote_pad can't work before all the media pad linked.
> >> 
> >> A video_init is typically called from the notify_complete callback.
> >> Actually, that's where the video_register_device should be created as
> >> well. When you create it in probe() there is possibly no sensor yet, so
> >> it would be a dead video node (or worse, crash when used).
> >> 
> >> There are still a lot of platform drivers that create the video node in
> >> the probe, but it's not the right place if you rely on the async loading
> >> of subdevs.
> > 
> > That's not really related, but I'm not really sure it's a good way to
> > operate. This essentially means that you might wait forever for a
> > component in your pipeline to be probed, without any chance of it
> > happening (not compiled, compiled as a module and not loaded, hardware
> > defect preventing the driver from probing properly, etc), even though
> > that component might not be essential.
> 
> We're talking straightforward video pipelines here. I.e. a source, some
> processing units and a DMA engine at the end.

As a first step possibly, but many SoCs have ISPs that are not supported by 
the initial camera driver version.

> There is no point in creating a video node if the pipeline is not complete
> since you need the full pipeline.
> 
> I've had bad experiences in the past where video nodes were created too
> soon and part of the internal state was still incomplete, causing at best
> weird behavior and at worst crashes.

Drivers obviously need to be fixed if they are buggy in that regard. Such race 
conditions are definitely something I keep an eye on when reviewing code.

> More complex devices are a whole different ballgame.
> 
> > This is how DRM operates, and you sometimes end up in some very dumb
> > situations where you wait for say, the DSI controller to probe, while
> > you only care about HDMI in your system.
> > 
> > But this seems to be on of the hot topic these days, so we might
> > discuss it further in some other thread :)

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1718256

在 2017-08-23 15:43，Laurent Pinchart 写道：
> Hi Hans,
> 
> On Wednesday, 23 August 2017 09:52:00 EEST Hans Verkuil wrote:
>> On 08/22/2017 10:17 PM, Maxime Ripard wrote:
>> > On Tue, Aug 22, 2017 at 08:43:35AM +0200, Hans Verkuil wrote:
>> >>>>> +static int sun6i_video_link_setup(struct media_entity *entity,
>> >>>>> +				  const struct media_pad *local,
>> >>>>> +				  const struct media_pad *remote, u32 flags)
>> >>>>> +{
>> >>>>> +	struct video_device *vdev = media_entity_to_video_device(entity);
>> >>>>> +	struct sun6i_video *video = video_get_drvdata(vdev);
>> >>>>> +
>> >>>>> +	if (WARN_ON(video == NULL))
>> >>>>> +		return 0;
>> >>>>> +
>> >>>>> +	return sun6i_video_formats_init(video);
>> >>>>
>> >>>> Why is this called here? Why not in video_init()?
>> >>>
>> >>> sun6i_video_init is in the driver probe context.
>> >>> sun6i_video_formats_init use media_entity_remote_pad and
>> >>> media_entity_to_v4l2_subdev to find the subdevs.
>> >>> The media_entity_remote_pad can't work before all the media pad linked.
>> >>
>> >> A video_init is typically called from the notify_complete callback.
>> >> Actually, that's where the video_register_device should be created as
>> >> well. When you create it in probe() there is possibly no sensor yet, so
>> >> it would be a dead video node (or worse, crash when used).
>> >>
>> >> There are still a lot of platform drivers that create the video node in
>> >> the probe, but it's not the right place if you rely on the async loading
>> >> of subdevs.
>> >
>> > That's not really related, but I'm not really sure it's a good way to
>> > operate. This essentially means that you might wait forever for a
>> > component in your pipeline to be probed, without any chance of it
>> > happening (not compiled, compiled as a module and not loaded, hardware
>> > defect preventing the driver from probing properly, etc), even though
>> > that component might not be essential.
>> 
>> We're talking straightforward video pipelines here. I.e. a source, 
>> some
>> processing units and a DMA engine at the end.
> 
> As a first step possibly, but many SoCs have ISPs that are not 
> supported by
> the initial camera driver version.

I think here it's also the situation.

Allwinner SoCs beyond sun6i (which is this driver targeting at) has an 
ISP
internally called "HawkView ISP", and there's no document of it (even 
the
source code is only released in recent several months).

> 
>> There is no point in creating a video node if the pipeline is not 
>> complete
>> since you need the full pipeline.
>> 
>> I've had bad experiences in the past where video nodes were created 
>> too
>> soon and part of the internal state was still incomplete, causing at 
>> best
>> weird behavior and at worst crashes.
> 
> Drivers obviously need to be fixed if they are buggy in that regard. 
> Such race
> conditions are definitely something I keep an eye on when reviewing 
> code.
> 
>> More complex devices are a whole different ballgame.
>> 
>> > This is how DRM operates, and you sometimes end up in some very dumb
>> > situations where you wait for say, the DSI controller to probe, while
>> > you only care about HDMI in your system.
>> >
>> > But this seems to be on of the hot topic these days, so we might
>> > discuss it further in some other thread :)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web