Re: [RFC PATCH v1 4/5] media: tegra: Add Tegra Video input driver for Tegra210

From: Sowjanya Komatineni
Date: Wed Jan 29 2020 - 00:59:29 EST


On 1/28/20 6:11 PM, Sowjanya Komatineni wrote:

On 1/28/20 5:05 PM, Helen Koike wrote:
External email: Use caution opening links or attachments


On 1/28/20 10:49 PM, Sowjanya Komatineni wrote:
On 1/28/20 2:13 PM, Sowjanya Komatineni wrote:
On 1/28/20 1:45 PM, Helen Koike wrote:
External email: Use caution opening links or attachments


Hi Sowjanya,

I just took a really quick look, I didn't check the driver in deep, so just some small comments below.

On 1/28/20 4:23 PM, Sowjanya Komatineni wrote:
Tegra210 contains a powerful Video Input (VI) hardware controller
which can support up to 6 MIPI CSI camera sensors.

Each Tegra CSI port can be one-to-one mapped to VI channel and can
capture from an external camera sensor connected to CSI or from
built-in test pattern generator.

Tegra210 supports built-in test pattern generator from CSI to VI.

This patch adds a V4L2 media controller and capture driver support
for Tegra210 built-in CSI to VI test pattern generator.

Signed-off-by: Sowjanya Komatineni <skomatineni@xxxxxxxxxx>
Could you send us the output of media-ctl --print-dot ? So we can view the media topology easily?
root@tegra-ubuntu:/home/ubuntu# ./media-ctl --print-dot
digraph board {
ÂÂÂÂÂÂÂÂ rankdir=TB
ÂÂÂÂÂÂÂÂ n00000001 [label="54080000.vi-output-0\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
ÂÂÂÂÂÂÂÂ n00000005 [label="54080000.vi-output-1\n/dev/video1", shape=box, style=filled, fillcolor=yellow]
ÂÂÂÂÂÂÂÂ n00000009 [label="54080000.vi-output-2\n/dev/video2", shape=box, style=filled, fillcolor=yellow]
ÂÂÂÂÂÂÂÂ n0000000d [label="54080000.vi-output-3\n/dev/video3", shape=box, style=filled, fillcolor=yellow]
ÂÂÂÂÂÂÂÂ n00000011 [label="54080000.vi-output-4\n/dev/video4", shape=box, style=filled, fillcolor=yellow]
ÂÂÂÂÂÂÂÂ n00000015 [label="54080000.vi-output-5\n/dev/video5", shape=box, style=filled, fillcolor=yellow]
ÂÂÂÂÂÂÂÂ n00000019 [label="{{} | tpg-0 | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
ÂÂÂÂÂÂÂÂ n00000019:port0 -> n00000001
ÂÂÂÂÂÂÂÂ n0000001d [label="{{} | tpg-1 | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
ÂÂÂÂÂÂÂÂ n0000001d:port0 -> n00000005
ÂÂÂÂÂÂÂÂ n00000021 [label="{{} | tpg-2 | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
ÂÂÂÂÂÂÂÂ n00000021:port0 -> n00000009
ÂÂÂÂÂÂÂÂ n00000025 [label="{{} | tpg-3 | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
ÂÂÂÂÂÂÂÂ n00000025:port0 -> n0000000d
ÂÂÂÂÂÂÂÂ n00000029 [label="{{} | tpg-4 | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
ÂÂÂÂÂÂÂÂ n00000029:port0 -> n00000011
ÂÂÂÂÂÂÂÂ n0000002d [label="{{} | tpg-5 | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
ÂÂÂÂÂÂÂÂ n0000002d:port0 -> n00000015
}

--- diff --git a/drivers/staging/media/tegra/host1x-video.h b/drivers/staging/media/tegra/host1x-video.h
new file mode 100644
index 000000000000..84d28e6f4362
--- /dev/null
+++ b/drivers/staging/media/tegra/host1x-video.h
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020 NVIDIA CORPORATION. All rights reserved.
+ */
+
+#ifndef HOST1X_VIDEO_H
+#define HOST1X_VIDEO_H 1
+
+#include <linux/host1x.h>
+
+#include <media/media-device.h>
+#include <media/media-entity.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-dev.h>
+#include <media/videobuf2-v4l2.h>
+
+#include "tegra-vi.h"
+#include "csi.h"
+
+struct tegra_camera {
+ÂÂÂÂ struct v4l2_device v4l2_dev;
+ÂÂÂÂ struct media_device media_dev;
+ÂÂÂÂ struct device *dev;
You can use cam->media_dev.dev instead of having this pointer.

Will fix in v2
+ÂÂÂÂ struct tegra_vi *vi;
+ÂÂÂÂ struct tegra_csi_device *csi;
+};
+
+
+#define to_tegra_channel(vdev) \
+ÂÂÂÂ container_of(vdev, struct tegra_channel, video)
Why not inline instead of define. Inlines has the advantage of checking types.
Will change in v2
+static int __tegra_channel_try_format(struct tegra_channel *chan,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_pix_format *pix,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ const struct tegra_video_format **vfmt)
+{
+ÂÂÂÂ const struct tegra_video_format *fmt_info;
+ÂÂÂÂ struct v4l2_subdev *subdev;
+ÂÂÂÂ struct v4l2_subdev_format fmt;
+ÂÂÂÂ struct v4l2_subdev_pad_config *pad_cfg;
+
+ÂÂÂÂ subdev = tegra_channel_get_remote_subdev(chan);
+ÂÂÂÂ pad_cfg = v4l2_subdev_alloc_pad_config(subdev);
+ÂÂÂÂ if (!pad_cfg)
+ÂÂÂÂÂÂÂÂÂÂÂÂ return -ENOMEM;
+
+ÂÂÂÂ /*
+ÂÂÂÂÂ * Retrieve format information and select the default format if the
+ÂÂÂÂÂ * requested format isn't supported.
+ÂÂÂÂÂ */
+ÂÂÂÂ fmt_info = tegra_core_get_format_by_fourcc(chan, pix->pixelformat);
+ÂÂÂÂ if (!fmt_info) {
+ÂÂÂÂÂÂÂÂÂÂÂÂ pix->pixelformat = chan->format.pixelformat;
+ÂÂÂÂÂÂÂÂÂÂÂÂ pix->colorspace = chan->format.colorspace;
+ÂÂÂÂÂÂÂÂÂÂÂÂ fmt_info = tegra_core_get_format_by_fourcc(chan,
+ pix->pixelformat);
+ÂÂÂÂ }
+
+ÂÂÂÂ /* Change this when start adding interlace format support */
+ÂÂÂÂ pix->field = V4L2_FIELD_NONE;
+ÂÂÂÂ fmt.which = V4L2_SUBDEV_FORMAT_TRY;
+ÂÂÂÂ fmt.pad = 0;
+ÂÂÂÂ v4l2_fill_mbus_format(&fmt.format, pix, fmt_info->code);
+ÂÂÂÂ v4l2_subdev_call(subdev, pad, set_fmt, pad_cfg, &fmt);
As fas as I understand, entities formats should be independent, it is up to link_validate
to check formats between entities.
The capture shouldn't change the format of the subdevice.

Tegra Built-in TPG on CSI accepts specific TPG sizes and CSI is source and VI is sink.

link validation happens only for sink ends of the link.
And what is the problem with it being on the sink end?
You just need to implement custom link validation in tegra_csi_media_ops that also checks the format
between the capture and the subdevice, no? Unless I missunderstood something here (which is quite possible).

Examples:
drivers/staging/media/rkisp1/rkisp1-capture.c - rkisp1_capture_link_validate()
drivers/media/pci/intel/ipu3/ipu3-cio2.c - cio2_video_link_validate()
drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c - sun6i_video_link_validate()

Regards,
Helen

But if we move subdevice side format/size check into its link_validation, any incorrect image size set thru set-fmt-video will be taken and get-fmt-video will also show same as it doesn't validate formats/sizes supported by CSI subdev during this time. link validation happens during pipeline start. So thought to prevent accepting incorrect format/size during set-fmt-video/get-fmt-video.

Other than this I don't see any issue moving it to link_validation.

link validation happens for all the links in the pipeline where both of the ends of the links are V4L2 sub-devices.

v4l2_subdev_link_validate calls sink pad link_validate.

This driver has currently TPG support only where CSI v4l2 subdevice is the source and VI is the sink video entity.



So with CSI subdev set_fmt sets width/height to default incase if width/height is not from one of the supported sizes.

+
+ÂÂÂÂ v4l2_fill_pix_format(pix, &fmt.format);
+ÂÂÂÂ tegra_channel_fmt_align(chan, &fmt_info->bpp, &pix->width, &pix->height,
+ &pix->bytesperline);
+ÂÂÂÂ pix->sizeimage = pix->bytesperline * pix->height;
+
+ÂÂÂÂ if (vfmt)
+ÂÂÂÂÂÂÂÂÂÂÂÂ *vfmt = fmt_info;
+
+ÂÂÂÂ v4l2_subdev_free_pad_config(pad_cfg);
+
+ÂÂÂÂ return 0;
+}
+
+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);
+
+ÂÂÂÂ return __tegra_channel_try_format(chan, &format->fmt.pix, NULL);
+}
+
+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;
+ÂÂÂÂ int ret;
+ÂÂÂÂ struct v4l2_subdev_format fmt;
+ÂÂÂÂ struct v4l2_subdev *subdev;
+ÂÂÂÂ struct v4l2_pix_format *pix = &format->fmt.pix;
+
+ÂÂÂÂ if (vb2_is_busy(&chan->queue))
+ÂÂÂÂÂÂÂÂÂÂÂÂ return -EBUSY;
+
+ÂÂÂÂ /* get supported format by try_fmt */
+ÂÂÂÂ ret = __tegra_channel_try_format(chan, pix, &info);
+ÂÂÂÂ if (ret)
+ÂÂÂÂÂÂÂÂÂÂÂÂ return ret;
+
+ÂÂÂÂ subdev = tegra_channel_get_remote_subdev(chan);
+
+ÂÂÂÂ fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ÂÂÂÂ fmt.pad = 0;
+ÂÂÂÂ v4l2_fill_mbus_format(&fmt.format, pix, info->code);
+ÂÂÂÂ v4l2_subdev_call(subdev, pad, set_fmt, NULL, &fmt);
same here.

Calling subdev set_fmt here for the same reason as explained above.
+
+ÂÂÂÂ v4l2_fill_pix_format(pix, &fmt.format);
+ÂÂÂÂ chan->format = *pix;
+ÂÂÂÂ chan->fmtinfo = info;
+ÂÂÂÂ tegra_channel_update_format(chan, pix->width,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pix->height, info->fourcc,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &info->bpp,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pix->bytesperline);
+ÂÂÂÂ *pix = chan->format;
+
+ÂÂÂÂ return 0;
+}
+
+static int tegra_channel_enum_input(struct file *file, void *fh,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_input *inp)
+{
+ÂÂÂÂ /* Currently driver supports internal TPG only */
+ÂÂÂÂ if (inp->index != 0)
just
if (inp->index)

Will update in v2
+ÂÂÂÂÂÂÂÂÂÂÂÂ return -EINVAL;
+
+ÂÂÂÂ inp->type = V4L2_INPUT_TYPE_CAMERA;
+ÂÂÂÂ strscpy(inp->name, "Tegra TPG", sizeof(inp->name));
+
+ÂÂÂÂ return 0;
+}
+static const struct tegra_video_format tegra_default_format = {
+ÂÂÂÂ /* RAW 10 */
+ÂÂÂÂ TEGRA_VF_RAW10,
+ÂÂÂÂ 10,
+ÂÂÂÂ MEDIA_BUS_FMT_SRGGB10_1X10,
+ÂÂÂÂ {2, 1},
+ÂÂÂÂ TEGRA_IMAGE_FORMAT_DEF,
+ÂÂÂÂ TEGRA_IMAGE_DT_RAW10,
+ÂÂÂÂ V4L2_PIX_FMT_SRGGB10,
+ÂÂÂÂ "RGRG.. GBGB..",
It would be more readable to do:

.code = TEGRA_VF_RAW10,
.width = 10,
.code = MEDIA_BUS_FMT_SRGGB10_1X10,

and so on
Will update in v2
+};
+
+/*
+ * Helper functions
+ */
+
+/**
+ * tegra_core_get_default_format - Get default format
+ *
+ * Return: pointer to the format where the default format needs
+ * to be filled in.
+ */
+const struct tegra_video_format *tegra_core_get_default_format(void)
+{
+ÂÂÂÂ return &tegra_default_format;
+}
This is only used in tegra-channel.c, why not to declare it there as static?

Will move all video format retrieval helper functions to corresponding file as static in v2
+ +static struct v4l2_frmsize_discrete tegra_csi_tpg_sizes[] = {
+ÂÂÂÂ {1280, 720},
+ÂÂÂÂ {1920, 1080},
+ÂÂÂÂ {3840, 2160},
+};
+
+/*
+ * V4L2 Subdevice Pad Operations
+ */
+static int tegra_csi_get_format(struct v4l2_subdev *subdev,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_pad_config *cfg,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_format *fmt)
+{
+ÂÂÂÂ struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
+
+ÂÂÂÂ mutex_lock(&csi_chan->format_lock);
Do you need this lock? I think there is already a serialization in the ioctls in place (to be confirmed).

This is on CSI v4l2 subdevice side during format updates
+ÂÂÂÂ memcpy(fmt, &csi_chan->ports->format,
+ÂÂÂÂÂÂÂÂÂÂÂ sizeof(struct v4l2_mbus_framefmt));
I would prefer just:
*fmt = *csi_chan->ports->format;

I think it is easier to read IMHO.
same in tegra_csi_set_format().

Will fix in v2
+ mutex_unlock(&csi_chan->format_lock);
+
+ÂÂÂÂ return 0;
+}
+
+static void tegra_csi_try_mbus_fmt(struct v4l2_subdev *subdev,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_mbus_framefmt *mfmt)
+{
+ÂÂÂÂ struct tegra_csi_channel *csi_chan = to_csi_chan(subdev);
+ÂÂÂÂ struct tegra_csi_device *csi = csi_chan->csi;
+ÂÂÂÂ const struct v4l2_frmsize_discrete *sizes;
+ÂÂÂÂ int i, j;
unsigned

Will fix in v2
+
+ÂÂÂÂ for (i = 0; i < ARRAY_SIZE(tegra_csi_tpg_fmts); i++) {
+ÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_mbus_framefmt *mbus_fmt = &tegra_csi_tpg_fmts[i];
+
+ÂÂÂÂÂÂÂÂÂÂÂÂ if (mfmt->code == mbus_fmt->code) {
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ for (j = 0; j < ARRAY_SIZE(tegra_csi_tpg_sizes); j++) {
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sizes = &tegra_csi_tpg_sizes[j];
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (mfmt->width == sizes->width &&
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mfmt->height == sizes->height) {
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return;
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ }
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ }
+ÂÂÂÂÂÂÂÂÂÂÂÂ }
+
+ÂÂÂÂÂÂÂÂÂÂÂÂ dev_info(csi->dev, "using Tegra default RAW10 video format\n");
+ÂÂÂÂ }
+
+ÂÂÂÂ dev_info(csi->dev, "using Tegra default WIDTH X HEIGHT (1920x1080)\n");
+ÂÂÂÂ memcpy(mfmt, tegra_csi_tpg_fmts, sizeof(struct v4l2_mbus_framefmt));
+}
+
+