From: Sowjanya Komatineni <skomatineni@nvidia.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Hans Verkuil <hverkuil@xs4all.nl>
Cc: Sakari Ailus <sakari.ailus@iki.fi>, <thierry.reding@gmail.com>,
<jonathanh@nvidia.com>, <frankc@nvidia.com>,
<helen.koike@collabora.com>, <digetx@gmail.com>,
<sboyd@kernel.org>, <linux-media@vger.kernel.org>,
<devicetree@vger.kernel.org>, <linux-clk@vger.kernel.org>,
<linux-tegra@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH v5 6/9] media: tegra: Add Tegra210 Video input driver
Date: Tue, 31 Mar 2020 11:33:39 -0700 [thread overview]
Message-ID: <3b00a559-992a-2da9-92b1-bee44e137ba2@nvidia.com> (raw)
In-Reply-To: <6aa7d86c-3943-d508-ccf6-5ac46546abe9@nvidia.com>
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
next prev parent reply other threads:[~2020-03-31 18:33 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-23 17:52 [RFC PATCH v5 0/9] Add Tegra driver for video capture Sowjanya Komatineni
2020-03-23 17:52 ` [RFC PATCH v5 1/9] arm64: tegra: Fix sor powergate clocks and reset Sowjanya Komatineni
2020-03-23 17:52 ` [RFC PATCH v5 2/9] arm64: tegra: Add reset-cells to mc Sowjanya Komatineni
2020-03-23 17:52 ` [RFC PATCH v5 3/9] dt-bindings: clock: tegra: Add clk id for CSI TPG clock Sowjanya Komatineni
2020-03-23 17:52 ` [RFC PATCH v5 4/9] clk: tegra: Add Tegra210 CSI TPG clock gate Sowjanya Komatineni
2020-03-23 17:52 ` [RFC PATCH v5 5/9] dt-binding: tegra: Add VI and CSI bindings Sowjanya Komatineni
2020-03-24 19:20 ` Dmitry Osipenko
2020-03-24 21:16 ` Sowjanya Komatineni
2020-03-23 17:52 ` [RFC PATCH v5 6/9] media: tegra: Add Tegra210 Video input driver Sowjanya Komatineni
2020-03-25 0:34 ` Dmitry Osipenko
2020-03-25 1:08 ` Sowjanya Komatineni
2020-03-25 1:15 ` Sowjanya Komatineni
2020-03-25 19:43 ` Dmitry Osipenko
[not found] ` <20200325110358.GB853@valkosipuli.retiisi.org.uk>
2020-03-25 11:09 ` Hans Verkuil
[not found] ` <a219aeb2-3d00-016e-eed9-503a9fbd0d13@nvidia.com>
2020-03-26 14:48 ` Sakari Ailus
2020-03-26 17:04 ` Sowjanya Komatineni
2020-03-30 10:59 ` Hans Verkuil
2020-03-31 10:32 ` Sakari Ailus
2020-03-31 10:56 ` Hans Verkuil
2020-03-31 11:10 ` Sakari Ailus
2020-03-31 11:27 ` Hans Verkuil
2020-03-31 11:52 ` Laurent Pinchart
2020-03-31 16:40 ` Sowjanya Komatineni
2020-03-31 18:33 ` Sowjanya Komatineni [this message]
2020-04-01 16:36 ` Sowjanya Komatineni
2020-04-01 16:58 ` Laurent Pinchart
2020-04-01 18:24 ` Sowjanya Komatineni
2020-04-03 7:36 ` Sowjanya Komatineni
2020-03-23 17:52 ` [RFC PATCH v5 7/9] MAINTAINERS: Add Tegra Video driver section Sowjanya Komatineni
2020-03-23 17:52 ` [RFC PATCH v5 8/9] dt-bindings: reset: Add ID for Tegra210 VI reset Sowjanya Komatineni
2020-03-23 17:52 ` [RFC PATCH v5 9/9] arm64: tegra: Add Tegra VI CSI support in device tree Sowjanya Komatineni
2020-03-24 19:19 ` Dmitry Osipenko
2020-03-24 21:04 ` Sowjanya Komatineni
2020-03-24 22:48 ` Dmitry Osipenko
2020-03-25 0:01 ` Sowjanya Komatineni
2020-03-25 0:22 ` Dmitry Osipenko
2020-03-30 10:04 ` [RFC PATCH v5 0/9] Add Tegra driver for video capture Hans Verkuil
2020-03-30 11:02 ` Hans Verkuil
2020-03-30 16:16 ` Sowjanya Komatineni
2020-04-03 5:45 ` Sowjanya Komatineni
2020-04-03 7:19 ` Hans Verkuil
2020-04-03 7:31 ` Sowjanya Komatineni
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3b00a559-992a-2da9-92b1-bee44e137ba2@nvidia.com \
--to=skomatineni@nvidia.com \
--cc=devicetree@vger.kernel.org \
--cc=digetx@gmail.com \
--cc=frankc@nvidia.com \
--cc=helen.koike@collabora.com \
--cc=hverkuil@xs4all.nl \
--cc=jonathanh@nvidia.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=sakari.ailus@iki.fi \
--cc=sboyd@kernel.org \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).