Re: [RFC PATCH v5 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

Hi Sakari/Laurent,

Few questions to confirm my understanding on below discussion.

1. Some sensors that you are referring as don't work with single devnode 
controlling pipeline devices are ISP built-in sensors where setup of 
pipeline and subdevices happen separately?

2. With driver supporting single device node control of entire pipeline 
devices compared to MC-based, limitation is with userspace apps for only 
these complex camera sensors?

3. Does all upstream video capture drivers eventually will be moved to 
support MC-based?

4. Based on libcamera doc looks like it will work with both types of 
MC-based and single devnode based pipeline setup drivers for normal 
sensors and limitation is when we use ISP built-in sensor or ISP HW 
block. Is my understanding correct?

Thanks

Sowjanya


On 3/31/20 11:33 AM, Sowjanya Komatineni wrote:
> On 3/31/20 9:40 AM, Sowjanya Komatineni wrote:
> > On 3/31/20 4:52 AM, Laurent Pinchart wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > Hello,
> > >
> > > On Tue, Mar 31, 2020 at 01:27:19PM +0200, Hans Verkuil wrote:
> > > > On 3/31/20 1:10 PM, Sakari Ailus wrote:
> > > > > On Tue, Mar 31, 2020 at 12:56:57PM +0200, Hans Verkuil wrote:
> > > > > > On 3/31/20 12:32 PM, Sakari Ailus wrote:
> > > > > > > On Mon, Mar 30, 2020 at 12:59:15PM +0200, Hans Verkuil wrote:
> > > > > > > > On 3/25/20 12:03 PM, Sakari Ailus wrote:
> > > > > > > > > On Mon, Mar 23, 2020 at 10:52:32AM -0700, 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>
> > > > > > > > > > ---
> > > > > > > > > > Â drivers/staging/media/KconfigÂÂÂÂÂÂÂÂÂÂÂÂÂ | 2 +
> > > > > > > > > > Â drivers/staging/media/MakefileÂÂÂÂÂÂÂÂÂÂÂÂ | 1 +
> > > > > > > > > > Â drivers/staging/media/tegra/KconfigÂÂÂÂÂÂÂ | 10 +
> > > > > > > > > > Â drivers/staging/media/tegra/MakefileÂÂÂÂÂÂ | 8 +
> > > > > > > > > > Â drivers/staging/media/tegra/TODOÂÂÂÂÂÂÂÂÂÂ | 10 +
> > > > > > > > > > Â drivers/staging/media/tegra/tegra-common.h | 263 +++++++
> > > > > > > > > > Â drivers/staging/media/tegra/tegra-csi.cÂÂÂ | 522 
> > > > > > > > > > ++++++++++++++
> > > > > > > > > > Â drivers/staging/media/tegra/tegra-csi.hÂÂÂ | 118 ++++
> > > > > > > > > > Â drivers/staging/media/tegra/tegra-vi.cÂÂÂÂ | 1058 
> > > > > > > > > > ++++++++++++++++++++++++++++
> > > > > > > > > > Â drivers/staging/media/tegra/tegra-vi.hÂÂÂÂ | 83 +++
> > > > > > > > > >  drivers/staging/media/tegra/tegra-video.c | 129 ++++
> > > > > > > > > >  drivers/staging/media/tegra/tegra-video.h | 32 +
> > > > > > > > > > Â drivers/staging/media/tegra/tegra210.cÂÂÂÂ | 754 
> > > > > > > > > > ++++++++++++++++++++
> > > > > > > > > > Â drivers/staging/media/tegra/tegra210.hÂÂÂÂ | 192 +++++
> > > > > > > > > Why staging? Are there reasons not to aim this to the kernel 
> > > > > > > > > proper right
> > > > > > > > > away? If you only support TPG, the driver may not have too 
> > > > > > > > > many (if any)
> > > > > > > > > real users anyway.
> > > > > > > > >
> > > > > > > > > > Â 14 files changed, 3182 insertions(+)
> > > > > > > > > > Â create mode 100644 drivers/staging/media/tegra/Kconfig
> > > > > > > > > > Â create mode 100644 drivers/staging/media/tegra/Makefile
> > > > > > > > > > Â create mode 100644 drivers/staging/media/tegra/TODO
> > > > > > > > > > Â create mode 100644 drivers/staging/media/tegra/tegra-common.h
> > > > > > > > > > Â create mode 100644 drivers/staging/media/tegra/tegra-csi.c
> > > > > > > > > > Â create mode 100644 drivers/staging/media/tegra/tegra-csi.h
> > > > > > > > > > Â create mode 100644 drivers/staging/media/tegra/tegra-vi.c
> > > > > > > > > > Â create mode 100644 drivers/staging/media/tegra/tegra-vi.h
> > > > > > > > > > Â create mode 100644 drivers/staging/media/tegra/tegra-video.c
> > > > > > > > > > Â create mode 100644 drivers/staging/media/tegra/tegra-video.h
> > > > > > > > > > Â create mode 100644 drivers/staging/media/tegra/tegra210.c
> > > > > > > > > > Â create mode 100644 drivers/staging/media/tegra/tegra210.h
> > > > > > > > <snip>
> > > > > > > >
> > > > > > > > > > +static int tegra_channel_g_input(struct file *file, void *priv,
> > > > > > > > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int *i)
> > > > > > > > > > +{
> > > > > > > > > > +ÂÂÂÂÂ *i = 0;
> > > > > > > > > > +ÂÂÂÂÂ return 0;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +static int tegra_channel_s_input(struct file *file, void *priv,
> > > > > > > > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int input)
> > > > > > > > > > +{
> > > > > > > > > > +ÂÂÂÂÂ if (input > 0)
> > > > > > > > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ return -EINVAL;
> > > > > > > > > > +
> > > > > > > > > > +ÂÂÂÂÂ return 0;
> > > > > > > > > > +}
> > > > > > > > > Please see patchset on topic "v4l2-dev/ioctl: Add 
> > > > > > > > > V4L2_CAP_IO_MC" on
> > > > > > > > > linux-media; it's relevant here, too.
> > > > > > > > No, it isn't. The pipeline is controlled by the driver, not by 
> > > > > > > > userspace.
> > > > > > > > This is a regular video capture driver, not an ISP driver.
> > > > > > > I don't think that really makes a difference, whether a device 
> > > > > > > is an ISP or
> > > > > > > not, but instead what does is whether there is something to 
> > > > > > > control in its
> > > > > > > pipeline that cannot be generally done through the regular V4L2 
> > > > > > > interface.
> > > > > > > Even plain CSI-2 receiver drivers should be media device centric 
> > > > > > > these days
> > > > > > > as doing otherwise excludes using a range of sensor drivers with 
> > > > > > > them,
> > > > > > > including any possible future support for e.g. sensor embedded 
> > > > > > > data.
> > > > > > We've been back and forth on this before for this driver. I see 
> > > > > > no reason to make things
> > > > > > complicated, these are simple video pipelines for video capture. 
> > > > > > Making this media
> > > > > > device centric means that existing software using the BSP version 
> > > > > > of this driver require
> > > > > > a full rewrite, which is not desirable.
> > > > > >
> > > > > > If we are going to require CSI receiver drivers to be media 
> > > > > > centric, then that's a
> > > > > > major departure of existing practice. And something that needs to 
> > > > > > be discussed first,
> > > > > I'd be happy to discuss that.
> > > > >
> > > > > Either way, the current design is problematic as it excludes a 
> > > > > range of
> > > > > camera sensors being used with the driver --- addressing of which 
> > > > > requires
> > > > > converting the driver MC centric. If the driver is merged to 
> > > > > mainline, then
> > > > > the user might face a Kconfig option or a module parameter to choose
> > > > > between the two --- this defines uAPI behaviour after all.
> > > > >
> > > > > The only way to avoid that in the future is to make it MC-centric 
> > > > > right
> > > > > away.
> > > > >
> > > > > > since that will require that support for each csi receiver driver 
> > > > > > is added to libcamera.
> > > > > > Is libcamera ready for that? Are common applications using 
> > > > > > libcamera yet?
> > > > > >
> > > > > > Obviously, if NVIDIA decides that this is worth the effort, then 
> > > > > > I have no objection.
> > > > > > But I don't think it is something we should require at this stage.
> > > > > Works for me. But in that case NVIDIA should also be aware that 
> > > > > doing so
> > > > > has consequences.
> > > > >
> > > > > We also haven't discussed what to do with old V4L2-centric drivers 
> > > > > which
> > > > > you'd use with sensors that expose their own subdevs. The 
> > > > > proportion of all
> > > > > sensors might not be large currently but it is almost certainly 
> > > > > bound to
> > > > > grow in the future.
> > > > >
> > > > > FWIW, Intel ipu3-cio2 CSI-2 receiver driver is MC-centric e.g. for 
> > > > > the
> > > > > above reasons. Libcamera supports it currently. I'll let Laurent 
> > > > > (cc'd)
> > > > > comment on the details.
> > > > I think it would be good to at least describe in some detail what 
> > > > you gain
> > > > by taking the media centric route, and what the obstacles are (loss 
> > > > of compatibility
> > > > with existing applications, requiring libcamera support).
> > > In this case the main gain is control of the camera sensor. Sensors can
> > > appear as simple when you don't look too closely at them, but many
> > > sensors (especially the ones modelled after SMIA++ and the now standard
> > > - and open! - MIPI CCS specification) have 3 locations to perform
> > > cropping (analog, digital and output), and 3 locations to perform
> > > scaling (binning, skipping, and full-featured scaler). All of these 
> > > need
> > > to be controlled by userspace one way or another if you want to
> > > implement proper camera algorithms, which those platforms target.
> > Thanks Laurent/Sakari/Hans.
> >
> > Based on discussion, seems like its good to change driver now to 
> > media-centric rather than later.
> >
> > As Jetson is devkit and custom camera sensor module meeting spec can 
> > be used, its good to let sensor control to user space.
> >
> > Will look into and update to use media-centric APIs.
> Will discuss this internally and will get back on this...
> > > > My personal feeling has always been that for ISP drivers the pros 
> > > > of making
> > > > a media-centric driver outweigh the cons, but that for a standard 
> > > > video capture
> > > > pipeline without complex processing blocks the cons outweigh the pros.
> > > >
> > > > This might change if libcamera becomes widely used, but we're not 
> > > > there yet.
> > > >
> > > > To be honest, I am not opposed to having a kernel config option for 
> > > > drivers
> > > > like this that select the media-centric API vs a regular API, if 
> > > > that can be
> > > > done without too much work. If you need full control for your 
> > > > embedded system,
> > > > then you enable the option. If you want full compatibility with 
> > > > existing
> > > > applications, then disable it.
> > > How would distributions be supposed to handle those ? That could in the
> > > end need to be a per-driver option, and it would be very messy. Maybe
> > > it's unavoidable, I'm trying to figure out a way to avoid such an 
> > > option
> > > for sensor drivers, to decide to expose them as a single subdev or
> > > multiple subdevs in order to support multiple streams CSI-2 streams, 
> > > and
> > > I'm not sure I'll succeed.
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart