Re: [PATCH 1/3] [media] v4l: tegra: Add NVIDIA Tegra VI driver

[Hans Verkuil <hverkuil@xs4all.nl>]

Hi Bryan,

Thanks for this patch series!

The switch to a kthread helps a lot, but I still have a number of comments
about it, primarily locking related.

On 09/16/2015 03:35 AM, Bryan Wu wrote:
> NVIDIA Tegra processor contains a powerful Video Input (VI) hardware
> controller which can support up to 6 MIPI CSI camera sensors.
> 
> This patch adds a V4L2 media controller and capture driver to support
> Tegra VI hardware. It's verified with Tegra built-in test pattern
> generator.
> 
> Signed-off-by: Bryan Wu <pengw@nvidia.com>
> Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/platform/Kconfig               |   1 +
>  drivers/media/platform/Makefile              |   2 +
>  drivers/media/platform/tegra/Kconfig         |  10 +
>  drivers/media/platform/tegra/Makefile        |   3 +
>  drivers/media/platform/tegra/tegra-channel.c | 802 +++++++++++++++++++++++++++
>  drivers/media/platform/tegra/tegra-core.c    | 252 +++++++++
>  drivers/media/platform/tegra/tegra-core.h    | 162 ++++++
>  drivers/media/platform/tegra/tegra-csi.c     | 566 +++++++++++++++++++
>  drivers/media/platform/tegra/tegra-vi.c      | 581 +++++++++++++++++++
>  drivers/media/platform/tegra/tegra-vi.h      | 213 +++++++
>  10 files changed, 2592 insertions(+)
>  create mode 100644 drivers/media/platform/tegra/Kconfig
>  create mode 100644 drivers/media/platform/tegra/Makefile
>  create mode 100644 drivers/media/platform/tegra/tegra-channel.c
>  create mode 100644 drivers/media/platform/tegra/tegra-core.c
>  create mode 100644 drivers/media/platform/tegra/tegra-core.h
>  create mode 100644 drivers/media/platform/tegra/tegra-csi.c
>  create mode 100644 drivers/media/platform/tegra/tegra-vi.c
>  create mode 100644 drivers/media/platform/tegra/tegra-vi.h
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index f6bed19..553867f 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -119,6 +119,7 @@ source "drivers/media/platform/exynos4-is/Kconfig"
>  source "drivers/media/platform/s5p-tv/Kconfig"
>  source "drivers/media/platform/am437x/Kconfig"
>  source "drivers/media/platform/xilinx/Kconfig"
> +source "drivers/media/platform/tegra/Kconfig"
>  
>  endif # V4L_PLATFORM_DRIVERS
>  

<snip>

> diff --git a/drivers/media/platform/tegra/tegra-channel.c b/drivers/media/platform/tegra/tegra-channel.c
> new file mode 100644
> index 0000000..8149016
> --- /dev/null
> +++ b/drivers/media/platform/tegra/tegra-channel.c
> @@ -0,0 +1,802 @@

<snip>

> +static int tegra_channel_capture_frame(struct tegra_channel *chan)
> +{
> +	struct tegra_channel_buffer *buf = chan->active;

I think this can just be passed as an argument instead of being a field
in the chan struct. I'm not 100% certain, you'd have to check this.

> +	struct vb2_buffer *vb = &buf->buf;
> +	int err = 0;
> +	u32 thresh, value, frame_start;
> +	int bytes_per_line = chan->format.bytesperline;
> +
> +	if (!vb2_start_streaming_called(&chan->queue) || !buf)
> +		return -EINVAL;

Can this ever happen? Perhaps this should be using WARN_ON?

> +
> +	/* Program buffer address by using surface 0 */
> +	csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_MSB, 0x0);
> +	csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_LSB, buf->addr);
> +	csi_write(chan, TEGRA_VI_CSI_SURFACE0_STRIDE, bytes_per_line);
> +
> +	/* Program syncpoint */
> +	frame_start = VI_CSI_PP_FRAME_START(chan->port);
> +	value = VI_CFG_VI_INCR_SYNCPT_COND(frame_start) |
> +		host1x_syncpt_id(chan->sp);
> +	tegra_channel_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value);
> +
> +	csi_write(chan, TEGRA_VI_CSI_SINGLE_SHOT, SINGLE_SHOT_CAPTURE);
> +
> +	/* Use syncpoint to wake up */
> +	thresh = host1x_syncpt_incr_max(chan->sp, 1);
> +	err = host1x_syncpt_wait(chan->sp, thresh,
> +			         TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);
> +	if (err) {
> +		dev_err(&chan->video.dev, "frame start syncpt timeout!\n");
> +		tegra_channel_capture_error(chan);
> +	}
> +
> +	/* Captured one frame */
> +	spin_lock_irq(&chan->queued_lock);

Why use spinlock_irq? You're not in interrupt context. A normal spinlock is
sufficient. Or perhaps the mutex_lock is enough (this function is already
called with that lock held).

> +	vb->v4l2_buf.sequence = chan->sequence++;
> +	vb->v4l2_buf.field = V4L2_FIELD_NONE;

Add a comment here that this will need to be changed if you ever start supporting
interlaced formats.

> +	v4l2_get_timestamp(&vb->v4l2_buf.timestamp);
> +	vb2_set_plane_payload(vb, 0, chan->format.sizeimage);
> +	vb2_buffer_done(vb, err < 0 ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
> +	spin_unlock_irq(&chan->queued_lock);
> +
> +	return err;
> +}
> +
> +static int tegra_channel_kthread_capture(void *data)
> +{
> +	struct tegra_channel *chan = data;
> +
> +	set_freezable();
> +
> +	while (1) {
> +		try_to_freeze();
> +		if (kthread_should_stop())
> +			break;
> +
> +		wait_event_interruptible(chan->wait,
> +					 !list_empty(&chan->capture));
> +
> +		spin_lock_irq(&chan->queued_lock);

Can the capture queue be emptied between the wait_event_interruptible() and
the spin_lock_irq()? It might be safer to do another list_empty() check here.

> +		chan->active = list_entry(chan->capture.next,
> +				struct tegra_channel_buffer, queue);
> +		list_del_init(&chan->active->queue);
> +		spin_unlock_irq(&chan->queued_lock);
> +		mutex_lock(&chan->lock);

Here is another possible race condition. Actually, is the chan->lock even
necessary? Only three functions take this lock: start_streaming, stop_streaming
and this function. But this thread won't be started until the end of start_streaming
(i.e. when that function already released the lock) and this thread will be
stopped before stop_streaming takes the lock. So I don't think you ever need
this lock since you never have concurrent access.

That leaves a simple spin_lock (not spin_lock_irq!) to control access to the
capture list.

> +		tegra_channel_capture_frame(chan);
> +		mutex_unlock(&chan->lock);
> +	}
> +
> +	return 0;
> +}

<snip>

> +static void tegra_channel_buffer_queue(struct vb2_buffer *vb)
> +{
> +	struct tegra_channel *chan = vb2_get_drv_priv(vb->vb2_queue);
> +	struct tegra_channel_buffer *buf = to_tegra_channel_buffer(vb);
> +
> +	/* Put buffer into the capture queue */
> +	spin_lock_irq(&chan->queued_lock);

See spin_lock_irq vs spin_lock vs mutex_lock discussion above. I won't refer to it
anymore in the remainder of this review.

> +	list_add_tail(&buf->queue, &chan->capture);
> +	spin_unlock_irq(&chan->queued_lock);
> +
> +	/* Wait up kthread for capture */
> +	if (waitqueue_active(&chan->wait))

No need for this test, wake_up_interruptible does the right thing if nobody
is waiting for the wq.

> +		wake_up_interruptible(&chan->wait);
> +}
> +
> +static int tegra_channel_set_stream(struct tegra_channel *chan, bool on)
> +{
> +	struct media_entity *entity;
> +	struct media_pad *pad;
> +	struct v4l2_subdev *subdev;
> +	int ret = 0;
> +
> +	entity = &chan->video.entity;
> +
> +	while (1) {
> +		if (entity->num_pads > 1 && (chan->port & 0x1))
> +			pad = &entity->pads[2];

Shouldn't it be entity->num_pads > 2? You're getting the third pad here.

> +		else
> +			pad = &entity->pads[0];

Magic numbers for pads[2] and pads[0]: this probably should use defines or at
the minimum a comment.

> +
> +		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> +			break;
> +
> +		pad = media_entity_remote_pad(pad);
> +		if (pad == NULL ||
> +		    media_entity_type(pad->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
> +			break;
> +
> +		entity = pad->entity;
> +		subdev = media_entity_to_v4l2_subdev(entity);
> +		subdev->host_priv = chan;
> +		ret = v4l2_subdev_call(subdev, video, s_stream, on);
> +		if (on && ret < 0 && ret != -ENOIOCTLCMD)
> +			return ret;

You loop using this subdev entity, but the test at the start of the 'while'
('entity->num_pads > 1 && (chan->port & 0x1)') probably makes no sense for
this entity.

Slightly off-topic since this is our problem, not yours: I think this function
should become a core helper function (it's used in several drivers now). I also
wonder if the assumption in most drivers that do this that the first pad is the
sink pad is correct: is it really always a sink pad? Can there be multiple sink
pads? If so, then you need to walk over all sink pads and call set_stream for
any that has enabled links.

All this is typically something that should be handled by a core function and
that should wait until the whole MC 'next-gen' code is merged.

> +	}
> +	return ret;
> +}
> +
> +static int tegra_channel_start_streaming(struct vb2_queue *vq, u32 count)
> +{
> +	struct tegra_channel *chan = vb2_get_drv_priv(vq);
> +	struct media_pipeline *pipe = chan->video.entity.pipe;
> +	struct tegra_channel_buffer *buf, *nbuf;
> +	int ret = 0;
> +
> +	if (!chan->vi->pg_mode && !chan->vi->has_sensors) {

I'm not sure about the has_sensors test: this makes the assumption that there is
a v4l2_subdev that drives a sensor or a HDMI-CSI bridge like the Toshiba TC358743.

But I know from past experience that sometimes it is fed via an FPGA or some other
path where there is no v4l2-subdev driver available (or often even needed).

So with this approach you would be forced to create a dummy subdev, just to
satisfy this condition. I think this is too restrictive.

> +		dev_err(&chan->video.dev,
> +			"is not in TPG mode and doesn't have \
> +			 any sensor connected!\n");
> +		ret = -EINVAL;
> +		goto vb2_queued;
> +	}
> +
> +	mutex_lock(&chan->lock);
> +
> +	/* The first open then turn on power*/
> +	if (atomic_add_return(1, &chan->vi->power_on_refcnt) == 1) {
> +		tegra_vi_power_on(chan->vi);
> +
> +		usleep_range(5, 100);
> +		tegra_channel_write(chan, TEGRA_VI_CFG_CG_CTRL,
> +				    VI_CG_2ND_LEVEL_EN);
> +		usleep_range(10, 15);
> +	}
> +
> +	/* Disable DPD */
> +	ret = tegra_io_rail_power_on(chan->io_id);
> +	if (ret < 0) {
> +		dev_err(&chan->video.dev,
> +			"failed to power on CSI rail: %d\n", ret);
> +		goto error_power_on;
> +	}
> +
> +	/* Clean up status */
> +	csi_write(chan, TEGRA_VI_CSI_ERROR_STATUS, 0xFFFFFFFF);
> +
> +	ret = media_entity_pipeline_start(&chan->video.entity, pipe);
> +	if (ret < 0)
> +		goto error_pipeline_start;
> +
> +	/* Start the pipeline. */
> +	ret = tegra_channel_set_stream(chan, true);
> +	if (ret < 0)
> +		goto error_set_stream;
> +
> +	/* Note: Program VI registers after TPG, sensors and CSI streaming */
> +	ret = tegra_channel_capture_setup(chan);
> +	if (ret < 0)
> +		goto error_capture_setup;
> +
> +	chan->sequence = 0;
> +	mutex_unlock(&chan->lock);
> +
> +	/* Start kthread to capture data to buffer */
> +	chan->kthread_capture = kthread_run(tegra_channel_kthread_capture, chan,
> +					    chan->video.name);
> +	if (IS_ERR(chan->kthread_capture)) {

I'd invert the test:

	if (!IS_ERR(chan->kthread_capture))
		return 0;

> +		dev_err(&chan->video.dev,
> +			"failed to start kthread for capture!\n");
> +		ret = PTR_ERR(chan->kthread_capture);
> +		goto error_kthread_run;
> +	}
> +
> +	return 0;
> +
> +error_kthread_run:
> +error_capture_setup:
> +	tegra_channel_set_stream(chan, false);
> +error_set_stream:
> +	media_entity_pipeline_stop(&chan->video.entity);
> +error_pipeline_start:
> +	tegra_io_rail_power_off(chan->io_id);
> +error_power_on:
> +	mutex_unlock(&chan->lock);
> +vb2_queued:
> +	/* Return all queued buffers back to vb2 */
> +	spin_lock_irq(&chan->queued_lock);
> +	vq->start_streaming_called = 0;
> +	list_for_each_entry_safe(buf, nbuf, &chan->capture, queue) {
> +		vb2_buffer_done(&buf->buf, VB2_BUF_STATE_QUEUED);
> +		list_del(&buf->queue);
> +	}
> +	spin_unlock_irq(&chan->queued_lock);
> +	return ret;
> +}
> +
> +static void tegra_channel_stop_streaming(struct vb2_queue *vq)
> +{
> +	struct tegra_channel *chan = vb2_get_drv_priv(vq);
> +	struct tegra_channel_buffer *buf, *nbuf;
> +	u32 thresh, value, mw_ack_done;
> +	int err;
> +
> +	/* Stop the kthread for capture */
> +	kthread_stop(chan->kthread_capture);

I don't think this is enough: kthread_stop will wake up the thread, but
the wake-up condition in the thread doesn't check for whether the thread
should stop, so it will just continue waiting. The wake-up condition should
be extended with a check that the kthread should exit.

You should also double-check that whatever buffer is being processed by the
thread is also returned with vb2_buffer_done, even if the thread is stopped.

> +	chan->kthread_capture = NULL;
> +
> +	mutex_lock(&chan->lock);
> +	/* Program syncpoint */
> +	mw_ack_done = VI_CSI_MW_ACK_DONE(chan->port);
> +	value = VI_CFG_VI_INCR_SYNCPT_COND(mw_ack_done) |
> +		host1x_syncpt_id(chan->sp);
> +	tegra_channel_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value);
> +
> +	/* Use syncpoint to wake up */
> +	thresh = host1x_syncpt_incr_max(chan->sp, 1);
> +	err = host1x_syncpt_wait(chan->sp, thresh,
> +			TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);
> +	if (err)
> +		dev_err(&chan->video.dev, "MW_ACK_DONE syncpoint time out!\n");
> +
> +	media_entity_pipeline_stop(&chan->video.entity);
> +
> +	tegra_channel_set_stream(chan, false);
> +
> +	tegra_io_rail_power_off(chan->io_id);
> +
> +	/* The last release then turn off power */
> +	if (atomic_dec_and_test(&chan->vi->power_on_refcnt))
> +		tegra_vi_power_off(chan->vi);
> +
> +	mutex_unlock(&chan->lock);
> +
> +	/* Give back all queued buffers to videobuf2. */
> +	spin_lock_irq(&chan->queued_lock);
> +	list_for_each_entry_safe(buf, nbuf, &chan->capture, queue) {
> +		vb2_buffer_done(&buf->buf, VB2_BUF_STATE_ERROR);
> +		list_del(&buf->queue);
> +	}
> +	spin_unlock_irq(&chan->queued_lock);
> +}
> +
> +static const struct vb2_ops tegra_channel_queue_qops = {
> +	.queue_setup = tegra_channel_queue_setup,
> +	.buf_prepare = tegra_channel_buffer_prepare,
> +	.buf_queue = tegra_channel_buffer_queue,
> +	.wait_prepare = vb2_ops_wait_prepare,
> +	.wait_finish = vb2_ops_wait_finish,
> +	.start_streaming = tegra_channel_start_streaming,
> +	.stop_streaming = tegra_channel_stop_streaming,
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * V4L2 ioctls
> + */
> +
> +static int
> +tegra_channel_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
> +{
> +	struct v4l2_fh *vfh = file->private_data;
> +	struct tegra_channel *chan = to_tegra_channel(vfh->vdev);
> +
> +	cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> +	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> +
> +	strlcpy(cap->driver, "tegra-video", sizeof(cap->driver));
> +	strlcpy(cap->card, chan->video.name, sizeof(cap->card));
> +	snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s:%u",
> +		 dev_name(chan->vi->dev), chan->port);
> +
> +	return 0;
> +}
> +
> +static int
> +tegra_channel_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
> +{
> +	struct v4l2_fh *vfh = file->private_data;
> +	struct tegra_channel *chan = to_tegra_channel(vfh->vdev);
> +	unsigned int index = 0, i;
> +	unsigned long *fmts_bitmap = NULL;
> +
> +	if (chan->vi->pg_mode)
> +		fmts_bitmap = chan->vi->tpg_fmts_bitmap;
> +	else if (chan->vi->has_sensors)
> +		fmts_bitmap = chan->fmts_bitmap;

Hmm, interesting. If has_sensors == 0, then which formats should be supported.
I think in that case you should just support *all* formats that the hardware
supports. So fmts_bitmap should never be NULL.

> +
> +	if (!fmts_bitmap ||
> +	    f->index > bitmap_weight(fmts_bitmap, MAX_FORMAT_NUM) - 1)
> +		return -EINVAL;
> +
> +	for (i = 0; i < f->index + 1; i++, index++)
> +		index = find_next_bit(fmts_bitmap, MAX_FORMAT_NUM, index);
> +
> +	f->pixelformat = tegra_core_get_fourcc_by_idx(index - 1);
> +
> +	return 0;
> +}
> +
> +static int
> +tegra_channel_get_format(struct file *file, void *fh, struct v4l2_format *format)
> +{
> +	struct v4l2_fh *vfh = file->private_data;
> +	struct tegra_channel *chan = to_tegra_channel(vfh->vdev);
> +
> +	format->fmt.pix = chan->format;
> +
> +	return 0;
> +}
> +
> +static void
> +__tegra_channel_try_format(struct tegra_channel *chan, struct v4l2_pix_format *pix,
> +			   const struct tegra_video_format **fmtinfo)
> +{
> +	const struct tegra_video_format *info;
> +	unsigned int min_width;
> +	unsigned int max_width;
> +	unsigned int min_bpl;
> +	unsigned int max_bpl;
> +	unsigned int width;
> +	unsigned int align;
> +	unsigned int bpl;
> +
> +	/* Retrieve format information and select the default format if the
> +	 * requested format isn't supported.
> +	 */
> +	info = tegra_core_get_format_by_fourcc(pix->pixelformat);
> +	if (!info)
> +		info = tegra_core_get_format_by_fourcc(TEGRA_VF_DEF);
> +
> +	pix->pixelformat = info->fourcc;
> +	pix->field = V4L2_FIELD_NONE;
> +
> +	/* The transfer alignment requirements are expressed in bytes. Compute
> +	 * the minimum and maximum values, clamp the requested width and convert
> +	 * it back to pixels.
> +	 */
> +	align = lcm(chan->align, info->bpp);
> +	min_width = roundup(TEGRA_MIN_WIDTH, align);
> +	max_width = rounddown(TEGRA_MAX_WIDTH, align);
> +	width = roundup(pix->width * info->bpp, align);
> +
> +	pix->width = clamp(width, min_width, max_width) / info->bpp;
> +	pix->height = clamp(pix->height, TEGRA_MIN_HEIGHT, TEGRA_MAX_HEIGHT);
> +
> +	/* Clamp the requested bytes per line value. If the maximum bytes per
> +	 * line value is zero, the module doesn't support user configurable line
> +	 * sizes. Override the requested value with the minimum in that case.
> +	 */
> +	min_bpl = pix->width * info->bpp;
> +	max_bpl = rounddown(TEGRA_MAX_WIDTH, chan->align);
> +	bpl = roundup(pix->bytesperline, chan->align);
> +
> +	pix->bytesperline = clamp(bpl, min_bpl, max_bpl);
> +	pix->sizeimage = pix->bytesperline * pix->height;
> +	pix->colorspace = V4L2_COLORSPACE_SRGB;

This information should come from the subdev. Only if there is no subdev would you
fallback to SRGB. If the subdev is for example an HDMI->CSI bridge, then the colorspace
can be quite different.

> +
> +	if (fmtinfo)
> +		*fmtinfo = info;
> +
> +	return;

This return can be removed.

> +}
> +
> +static int
> +tegra_channel_try_format(struct file *file, void *fh, struct v4l2_format *format)
> +{
> +	struct v4l2_fh *vfh = file->private_data;
> +	struct tegra_channel *chan = to_tegra_channel(vfh->vdev);
> +
> +	__tegra_channel_try_format(chan, &format->fmt.pix, NULL);
> +
> +	return 0;
> +}
> +
> +static int
> +tegra_channel_set_format(struct file *file, void *fh, struct v4l2_format *format)
> +{
> +	struct v4l2_fh *vfh = file->private_data;
> +	struct tegra_channel *chan = to_tegra_channel(vfh->vdev);
> +	const struct tegra_video_format *info;
> +
> +	if (vb2_is_busy(&chan->queue))
> +		return -EBUSY;
> +
> +	__tegra_channel_try_format(chan, &format->fmt.pix, &info);
> +
> +	chan->format = format->fmt.pix;
> +	chan->fmtinfo = info;
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_ioctl_ops tegra_channel_ioctl_ops = {
> +	.vidioc_querycap		= tegra_channel_querycap,
> +	.vidioc_enum_fmt_vid_cap	= tegra_channel_enum_format,
> +	.vidioc_g_fmt_vid_cap		= tegra_channel_get_format,
> +	.vidioc_s_fmt_vid_cap		= tegra_channel_set_format,
> +	.vidioc_try_fmt_vid_cap		= tegra_channel_try_format,
> +	.vidioc_reqbufs			= vb2_ioctl_reqbufs,
> +	.vidioc_querybuf		= vb2_ioctl_querybuf,
> +	.vidioc_qbuf			= vb2_ioctl_qbuf,
> +	.vidioc_dqbuf			= vb2_ioctl_dqbuf,
> +	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
> +	.vidioc_expbuf			= vb2_ioctl_expbuf,
> +	.vidioc_streamon		= vb2_ioctl_streamon,
> +	.vidioc_streamoff		= vb2_ioctl_streamoff,

In order to support HDMI->CSI bridges we'll likely need to add support for various
other ioctls here (*_dv_timings, *_edid) which would pretty much pass on the arguments
to the subdev.

However, this can be added later.

> +};
> +
> +/* -----------------------------------------------------------------------------
> + * V4L2 file operations
> + */
> +
> +static const struct v4l2_file_operations tegra_channel_fops = {
> +	.owner		= THIS_MODULE,
> +	.unlocked_ioctl	= video_ioctl2,
> +	.open		= v4l2_fh_open,
> +	.release	= vb2_fop_release,
> +	.read		= vb2_fop_read,
> +	.poll		= vb2_fop_poll,
> +	.mmap		= vb2_fop_mmap,
> +};
> +
> +int tegra_channel_init(struct tegra_vi *vi, unsigned int port)
> +{
> +	int ret;
> +	struct tegra_channel *chan  = &vi->chans[port];
> +
> +	chan->vi = vi;
> +	chan->port = port;
> +
> +	/* Init channel register base */
> +	chan->csi = vi->iomem + TEGRA_VI_CSI_BASE(port);
> +
> +	/* VI Channel is 64 bytes alignment */
> +	chan->align = 64;
> +	chan->io_id = tegra_io_rail_csi_ids[chan->port];
> +	mutex_init(&chan->lock);
> +	mutex_init(&chan->video_lock);
> +	INIT_LIST_HEAD(&chan->capture);
> +	init_waitqueue_head(&chan->wait);
> +	spin_lock_init(&chan->queued_lock);
> +
> +	/* Init video format */
> +	chan->fmtinfo = tegra_core_get_format_by_fourcc(TEGRA_VF_DEF);
> +	chan->format.pixelformat = chan->fmtinfo->fourcc;
> +	chan->format.colorspace = V4L2_COLORSPACE_SRGB;
> +	chan->format.field = V4L2_FIELD_NONE;
> +	chan->format.width = TEGRA_DEF_WIDTH;
> +	chan->format.height = TEGRA_DEF_HEIGHT;
> +	chan->format.bytesperline = chan->format.width * chan->fmtinfo->bpp;
> +	chan->format.sizeimage = chan->format.bytesperline *
> +				    chan->format.height;
> +
> +	/* Initialize the media entity... */
> +	chan->pad.flags = MEDIA_PAD_FL_SINK;
> +
> +	ret = media_entity_init(&chan->video.entity, 1, &chan->pad, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* ... and the video node... */
> +	chan->video.fops = &tegra_channel_fops;
> +	chan->video.v4l2_dev = &vi->v4l2_dev;
> +	chan->video.queue = &chan->queue;
> +	snprintf(chan->video.name, sizeof(chan->video.name), "%s-%s-%u",
> +		 dev_name(vi->dev), "output", port);
> +	chan->video.vfl_type = VFL_TYPE_GRABBER;
> +	chan->video.vfl_dir = VFL_DIR_RX;
> +	chan->video.release = video_device_release_empty;
> +	chan->video.ioctl_ops = &tegra_channel_ioctl_ops;
> +	chan->video.lock = &chan->video_lock;
> +
> +	video_set_drvdata(&chan->video, chan);
> +
> +	/* Init host1x interface */
> +	INIT_LIST_HEAD(&chan->client.list);
> +	chan->client.dev = chan->vi->dev;
> +
> +	ret = host1x_client_register(&chan->client);
> +	if (ret < 0) {
> +		dev_err(chan->vi->dev, "failed to register host1x client: %d\n",
> +			ret);
> +		ret = -ENODEV;
> +		goto host1x_register_error;
> +	}
> +
> +	chan->sp = host1x_syncpt_request(chan->client.dev,
> +					 HOST1X_SYNCPT_HAS_BASE);
> +	if (!chan->sp) {
> +		dev_err(chan->vi->dev, "failed to request host1x syncpoint\n");
> +		ret = -ENOMEM;
> +		goto host1x_sp_error;
> +	}
> +
> +	/* ... and the buffers queue... */
> +	chan->alloc_ctx = vb2_dma_contig_init_ctx(&chan->video.dev);
> +	if (IS_ERR(chan->alloc_ctx)) {
> +		dev_err(chan->vi->dev, "failed to init vb2 buffer\n");
> +		ret = -ENOMEM;
> +		goto vb2_init_error;
> +	}
> +
> +	chan->queue.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	chan->queue.io_modes = VB2_MMAP | VB2_DMABUF | VB2_READ;
> +	chan->queue.lock = &chan->video_lock;
> +	chan->queue.drv_priv = chan;
> +	chan->queue.buf_struct_size = sizeof(struct tegra_channel_buffer);
> +	chan->queue.ops = &tegra_channel_queue_qops;
> +	chan->queue.mem_ops = &vb2_dma_contig_memops;
> +	chan->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC
> +				   | V4L2_BUF_FLAG_TSTAMP_SRC_EOF;
> +	ret = vb2_queue_init(&chan->queue);
> +	if (ret < 0) {
> +		dev_err(chan->vi->dev, "failed to initialize VB2 queue\n");
> +		goto vb2_queue_error;
> +	}
> +
> +	ret = video_register_device(&chan->video, VFL_TYPE_GRABBER, -1);
> +	if (ret < 0) {
> +		dev_err(&chan->video.dev, "failed to register video device\n");
> +		goto video_register_error;
> +	}
> +
> +	return 0;
> +
> +video_register_error:
> +	vb2_queue_release(&chan->queue);
> +vb2_queue_error:
> +	vb2_dma_contig_cleanup_ctx(chan->alloc_ctx);
> +vb2_init_error:
> +	host1x_syncpt_free(chan->sp);
> +host1x_sp_error:
> +	host1x_client_unregister(&chan->client);
> +host1x_register_error:
> +	media_entity_cleanup(&chan->video.entity);
> +	return ret;
> +}
> +
> +int tegra_channel_cleanup(struct tegra_channel *chan)
> +{
> +	video_unregister_device(&chan->video);
> +
> +	vb2_queue_release(&chan->queue);
> +	vb2_dma_contig_cleanup_ctx(chan->alloc_ctx);
> +
> +	host1x_syncpt_free(chan->sp);
> +	host1x_client_unregister(&chan->client);
> +
> +	media_entity_cleanup(&chan->video.entity);
> +
> +	return 0;
> +}

<snip>

> diff --git a/drivers/media/platform/tegra/tegra-vi.c b/drivers/media/platform/tegra/tegra-vi.c
> new file mode 100644
> index 0000000..295c25f
> --- /dev/null
> +++ b/drivers/media/platform/tegra/tegra-vi.c
> @@ -0,0 +1,581 @@
> +/*
> + * NVIDIA Tegra Video Input Device
> + *
> + * Copyright (c) 2015, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * Author: Bryan Wu <pengw@nvidia.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.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +#include <media/media-device.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-of.h>
> +
> +#include <soc/tegra/pmc.h>
> +
> +#include "tegra-vi.h"
> +
> +/* In TPG mode, VI only support 2 formats */
> +static void vi_tpg_fmts_bitmap_init(struct tegra_vi *vi)
> +{
> +	int index;
> +
> +	bitmap_zero(vi->tpg_fmts_bitmap, MAX_FORMAT_NUM);
> +
> +	index = tegra_core_get_idx_by_code(MEDIA_BUS_FMT_SRGGB10_1X10);
> +	bitmap_set(vi->tpg_fmts_bitmap, index, 1);
> +
> +	index = tegra_core_get_idx_by_code(MEDIA_BUS_FMT_RGB888_1X32_PADHI);
> +	bitmap_set(vi->tpg_fmts_bitmap, index, 1);
> +}
> +
> +/*
> + * Control Config
> + */
> +
> +static const char *const vi_pattern_strings[] = {
> +	"Disabled",
> +	"Black/White Direct Mode",
> +	"Color Patch Mode",
> +};
> +
> +static int vi_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct tegra_vi *vi = container_of(ctrl->handler, struct tegra_vi,
> +					   ctrl_handler);
> +	switch (ctrl->id) {
> +	case V4L2_CID_TEST_PATTERN:
> +		vi->pg_mode = ctrl->val;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_ctrl_ops vi_ctrl_ops = {
> +	.s_ctrl	= vi_s_ctrl,
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Media Controller and V4L2
> + */
> +
> +static void tegra_vi_v4l2_cleanup(struct tegra_vi *vi)
> +{
> +	v4l2_ctrl_handler_free(&vi->ctrl_handler);
> +	v4l2_device_unregister(&vi->v4l2_dev);
> +	media_device_unregister(&vi->media_dev);
> +}
> +
> +static int tegra_vi_v4l2_init(struct tegra_vi *vi)
> +{
> +	int ret;
> +
> +	vi->media_dev.dev = vi->dev;
> +	strlcpy(vi->media_dev.model, "NVIDIA Tegra Video Input Device",
> +		sizeof(vi->media_dev.model));
> +	vi->media_dev.hw_revision = 3;
> +
> +	ret = media_device_register(&vi->media_dev);

This will likely change. See https://lkml.org/lkml/2015/9/15/371 for more info.

> +	if (ret < 0) {
> +		dev_err(vi->dev, "media device registration failed (%d)\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	vi->v4l2_dev.mdev = &vi->media_dev;
> +	ret = v4l2_device_register(vi->dev, &vi->v4l2_dev);
> +	if (ret < 0) {
> +		dev_err(vi->dev, "V4L2 device registration failed (%d)\n",
> +			ret);
> +		goto register_error;
> +	}
> +
> +	v4l2_ctrl_handler_init(&vi->ctrl_handler, 1);
> +	vi->pattern = v4l2_ctrl_new_std_menu_items(&vi->ctrl_handler,
> +					&vi_ctrl_ops, V4L2_CID_TEST_PATTERN,
> +					ARRAY_SIZE(vi_pattern_strings) - 1,
> +					0, 0, vi_pattern_strings);
> +
> +	if (vi->ctrl_handler.error) {
> +		dev_err(vi->dev, "failed to add controls\n");
> +		ret = vi->ctrl_handler.error;
> +		goto ctrl_error;
> +	}
> +	vi->v4l2_dev.ctrl_handler = &vi->ctrl_handler;
> +
> +	ret = v4l2_ctrl_handler_setup(&vi->ctrl_handler);
> +	if (ret < 0) {
> +		dev_err(vi->dev, "failed to set controls\n");
> +		goto ctrl_error;
> +	}
> +	return 0;
> +
> +
> +ctrl_error:
> +	v4l2_ctrl_handler_free(&vi->ctrl_handler);
> +	v4l2_device_unregister(&vi->v4l2_dev);
> +register_error:
> +	media_device_unregister(&vi->media_dev);
> +	return ret;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Platform Device Driver
> + */
> +
> +int tegra_vi_power_on(struct tegra_vi *vi)
> +{
> +	int ret;
> +
> +	ret = regulator_enable(vi->vi_reg);
> +	if (ret)
> +		return ret;
> +
> +	ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_VENC,
> +						vi->vi_clk, vi->vi_rst);
> +	if (ret) {
> +		dev_err(vi->dev, "failed to power up!\n");
> +		goto error_power_up;
> +	}
> +
> +	ret = clk_set_rate(vi->vi_clk, 408000000);
> +	if (ret) {
> +		dev_err(vi->dev, "failed to set vi clock to 408MHz!\n");
> +		goto error_clk_set_rate;
> +	}
> +
> +	clk_prepare_enable(vi->csi_clk);
> +
> +	return 0;
> +
> +error_clk_set_rate:
> +	tegra_powergate_power_off(TEGRA_POWERGATE_VENC);
> +error_power_up:
> +	regulator_disable(vi->vi_reg);
> +	return ret;
> +}
> +
> +void tegra_vi_power_off(struct tegra_vi *vi)
> +{
> +	reset_control_assert(vi->vi_rst);
> +	clk_disable_unprepare(vi->vi_clk);
> +	clk_disable_unprepare(vi->csi_clk);
> +	tegra_powergate_power_off(TEGRA_POWERGATE_VENC);
> +	regulator_disable(vi->vi_reg);
> +}
> +
> +static int tegra_vi_channels_init(struct tegra_vi *vi)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(vi->chans); i++) {
> +		ret = tegra_channel_init(vi, i);
> +		if (ret < 0) {
> +			dev_err(vi->dev, "channel %d init failed\n", i);
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int tegra_vi_channels_cleanup(struct tegra_vi *vi)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(vi->chans); i++) {
> +		ret = tegra_channel_cleanup(&vi->chans[i]);
> +		if (ret < 0) {
> +			dev_err(vi->dev, "channel %d cleanup failed\n", i);
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Graph Management
> + */
> +
> +static struct tegra_vi_graph_entity *
> +tegra_vi_graph_find_entity(struct tegra_vi *vi,
> +		       const struct device_node *node)
> +{
> +	struct tegra_vi_graph_entity *entity;
> +
> +	list_for_each_entry(entity, &vi->entities, list) {
> +		if (entity->node == node)
> +			return entity;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int tegra_vi_graph_build_links(struct tegra_vi *vi)
> +{
> +	u32 link_flags = MEDIA_LNK_FL_ENABLED;
> +	struct device_node *node = vi->dev->of_node;
> +	struct media_entity *source;
> +	struct media_entity *sink;
> +	struct media_pad *source_pad;
> +	struct media_pad *sink_pad;
> +	struct tegra_vi_graph_entity *ent;
> +	struct v4l2_of_link link;
> +	struct device_node *ep = NULL;
> +	struct device_node *next;
> +	struct tegra_channel *chan;
> +	int ret = 0;
> +
> +	dev_dbg(vi->dev, "creating links for channels\n");
> +
> +	while (1) {
> +		/* Get the next endpoint and parse its link. */
> +		next = of_graph_get_next_endpoint(node, ep);
> +		if (next == NULL)
> +			break;
> +
> +		of_node_put(ep);
> +		ep = next;
> +
> +		dev_dbg(vi->dev, "processing endpoint %s\n", ep->full_name);
> +
> +		ret = v4l2_of_parse_link(ep, &link);
> +		if (ret < 0) {
> +			dev_err(vi->dev, "failed to parse link for %s\n",
> +				ep->full_name);
> +			continue;
> +		}
> +
> +		if (link.local_port > MAX_CHAN_NUM) {
> +			dev_err(vi->dev, "wrong channel number for port %u\n",
> +				link.local_port);
> +			v4l2_of_put_link(&link);
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		chan = &vi->chans[link.local_port];
> +
> +		dev_dbg(vi->dev, "creating link for channel %s\n",
> +			chan->video.name);
> +
> +		/* Find the remote entity. */
> +		ent = tegra_vi_graph_find_entity(vi, link.remote_node);
> +		if (ent == NULL) {
> +			dev_err(vi->dev, "no entity found for %s\n",
> +				link.remote_node->full_name);
> +			v4l2_of_put_link(&link);
> +			ret = -ENODEV;
> +			break;
> +		}
> +
> +		source = ent->entity;
> +		source_pad = &source->pads[(link.remote_port & 1) * 2 + 1];
> +		sink = &chan->video.entity;
> +		sink_pad = &chan->pad;
> +
> +		v4l2_of_put_link(&link);
> +
> +		/* Create the media link. */
> +		dev_dbg(vi->dev, "creating %s:%u -> %s:%u link\n",
> +			source->name, source_pad->index,
> +			sink->name, sink_pad->index);
> +
> +		ret = media_entity_create_link(source, source_pad->index,
> +					       sink, sink_pad->index,
> +					       link_flags);
> +		if (ret < 0) {
> +			dev_err(vi->dev,
> +				"failed to create %s:%u -> %s:%u link\n",
> +				source->name, source_pad->index,
> +				sink->name, sink_pad->index);
> +			break;
> +		}
> +
> +		tegra_channel_fmts_bitmap_init(chan, ent);
> +	}
> +
> +	of_node_put(ep);
> +	return ret;
> +}
> +
> +static int tegra_vi_graph_notify_complete(struct v4l2_async_notifier *notifier)
> +{
> +	struct tegra_vi *vi =
> +		container_of(notifier, struct tegra_vi, notifier);
> +	int ret;
> +
> +	dev_dbg(vi->dev, "notify complete, all subdevs registered\n");
> +
> +	/* Create links for every entity. */
> +	ret = tegra_vi_graph_build_links(vi);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = v4l2_device_register_subdev_nodes(&vi->v4l2_dev);
> +	if (ret < 0)
> +		dev_err(vi->dev, "failed to register subdev nodes\n");
> +
> +	return ret;
> +}
> +
> +static int tegra_vi_graph_notify_bound(struct v4l2_async_notifier *notifier,
> +				   struct v4l2_subdev *subdev,
> +				   struct v4l2_async_subdev *asd)
> +{
> +	struct tegra_vi *vi = container_of(notifier, struct tegra_vi, notifier);
> +	struct tegra_vi_graph_entity *entity;
> +
> +	/* Locate the entity corresponding to the bound subdev and store the
> +	 * subdev pointer.
> +	 */
> +	list_for_each_entry(entity, &vi->entities, list) {
> +		if (entity->node != subdev->dev->of_node)
> +			continue;
> +
> +		if (entity->subdev) {
> +			dev_err(vi->dev, "duplicate subdev for node %s\n",
> +				entity->node->full_name);
> +			return -EINVAL;
> +		}
> +
> +		dev_dbg(vi->dev, "subdev %s bound\n", subdev->name);
> +		entity->entity = &subdev->entity;
> +		entity->subdev = subdev;
> +		return 0;
> +	}
> +
> +	dev_err(vi->dev, "no entity for subdev %s\n", subdev->name);
> +	return -EINVAL;
> +}
> +
> +static void tegra_vi_graph_cleanup(struct tegra_vi *vi)
> +{
> +	struct tegra_vi_graph_entity *entityp;
> +	struct tegra_vi_graph_entity *entity;
> +
> +	v4l2_async_notifier_unregister(&vi->notifier);
> +
> +	list_for_each_entry_safe(entity, entityp, &vi->entities, list) {
> +		of_node_put(entity->node);
> +		list_del(&entity->list);
> +	}
> +}
> +
> +static int tegra_vi_graph_init(struct tegra_vi *vi)
> +{
> +	struct device_node *node = vi->dev->of_node;
> +	struct device_node *ep = NULL;
> +	struct device_node *next;
> +	struct device_node *remote = NULL;
> +	struct tegra_vi_graph_entity *entity;
> +	struct v4l2_async_subdev **subdevs = NULL;
> +	unsigned int num_subdevs = 0;
> +	int ret = 0, i;
> +
> +	/* Parse all the remote entities and put them into the list */
> +	while (1) {
> +		next = of_graph_get_next_endpoint(node, ep);
> +		if (!next)
> +			break;
> +
> +		of_node_put(ep);
> +		ep = next;
> +
> +		remote = of_graph_get_remote_port_parent(ep);
> +		if (!remote) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		entity = tegra_vi_graph_find_entity(vi, remote);
> +		if (!entity) {
> +			entity = devm_kzalloc(vi->dev, sizeof(*entity),
> +					      GFP_KERNEL);
> +			if (entity == NULL) {
> +				of_node_put(remote);
> +				ret = -ENOMEM;
> +				break;
> +			}
> +
> +			entity->node = remote;
> +			entity->asd.match_type = V4L2_ASYNC_MATCH_OF;
> +			entity->asd.match.of.node = remote;
> +			list_add_tail(&entity->list, &vi->entities);
> +			vi->num_subdevs++;
> +		}
> +	}
> +	of_node_put(ep);
> +
> +	if (!vi->num_subdevs) {
> +		dev_warn(vi->dev, "no subdev found in graph\n");
> +		goto done;
> +	}
> +
> +	/*
> +	 * VI has at least MAX_CHAN_NUM / 2 subdevices for CSI blocks.
> +	 * If just found CSI subdevices connecting to VI, VI has no sensors
> +	 * described in DT but has to use test pattern generator mode.
> +	 * Otherwise VI has sensors connecting.
> +	 */
> +	vi->has_sensors = (vi->num_subdevs > MAX_CHAN_NUM / 2);
> +
> +	/* Register the subdevices notifier. */
> +	num_subdevs = vi->num_subdevs;
> +	subdevs = devm_kzalloc(vi->dev, sizeof(*subdevs) * num_subdevs,
> +			       GFP_KERNEL);
> +	if (subdevs == NULL) {
> +		ret = -ENOMEM;
> +		goto done;
> +	}
> +
> +	i = 0;
> +	list_for_each_entry(entity, &vi->entities, list)
> +		subdevs[i++] = &entity->asd;
> +
> +	vi->notifier.subdevs = subdevs;
> +	vi->notifier.num_subdevs = num_subdevs;
> +	vi->notifier.bound = tegra_vi_graph_notify_bound;
> +	vi->notifier.complete = tegra_vi_graph_notify_complete;
> +
> +	ret = v4l2_async_notifier_register(&vi->v4l2_dev, &vi->notifier);
> +	if (ret < 0) {
> +		dev_err(vi->dev, "notifier registration failed\n");
> +		goto done;
> +	}
> +
> +	return 0;
> +
> +done:
> +	if (ret < 0)
> +		tegra_vi_graph_cleanup(vi);
> +
> +	return ret;
> +}

Hmmm. All this code should become helper core code. We'll have to see
how we're going to do this once the MC next-gen patches are merged.

<snip>

Regards,

	Hans