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

[Sowjanya Komatineni <skomatineni@nvidia.com>]

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@nvidia.com>
>>>>>>>>>> ---
>>>>>>>>>>   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