by Laurent Pinchart

[permalink] [raw]

Subject: Re: [PATCH 1/3] media: open.rst: document devnode-centric and mc-centric types

Hi Mauro,

On Friday, 25 August 2017 16:38:23 EEST Mauro Carvalho Chehab wrote:
> Em Fri, 25 Aug 2017 16:11:15 +0300 Laurent Pinchart escreveu:
> > On Friday, 25 August 2017 12:40:05 EEST Mauro Carvalho Chehab wrote:
> >> From: "[email protected]" <[email protected]>
> >>
> >> When we added support for omap3, back in 2010, we added a new
> >> type of V4L2 devices that aren't fully controlled via the V4L2
> >> device node. Yet, we never made it clear, at the V4L2 spec,
> >> about the differences between both types.
> >
> > Nitpicking (and there will be lots of nitpicking in this review as I think
> > it's very important to get this piece of the documentation right down to
> > details):
> >
> Sure.
>
> > s/at the V4L2 spec/in the V4L2 spec/
> >
> > "make it clear about" doesn't sound good to me. How about the following ?
> >
> > "Yet, we have never clearly documented in the V4L2 specification the
> > differences between the two types."
> >
> >> Let's document them with the current implementation.
> >
> > s/with the/based on the/
>
> OK.
>
> >> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> >> ---
> >>
> >> Documentation/media/uapi/v4l/open.rst | 50 +++++++++++++++++++++++++++++
> >> 1 file changed, 50 insertions(+)
> >>
> >> diff --git a/Documentation/media/uapi/v4l/open.rst
> >> b/Documentation/media/uapi/v4l/open.rst index afd116edb40d..a72d142897c0
> >> 100644
> >> --- a/Documentation/media/uapi/v4l/open.rst
> >> +++ b/Documentation/media/uapi/v4l/open.rst
> >> @@ -6,6 +6,56 @@
> >> Opening and Closing Devices
> >> ***************************
> >>
> >> +Types of V4L2 hardware control
> >> +==============================
> >> +
> >> +V4L2 devices are usually complex: they are implemented via a main
> >> driver and
> >> +often several additional drivers. The main driver always exposes one or
> >> +more **V4L2 device** devnodes (see :ref:`v4l2_device_naming`).
> >
> > First of all, as stated in a previous e-mail, I think we should start by
> > defining the terms we are going to use.
>
> Well, we can add a Glossary at the spec, but this is a huge work,
> as we'll need to seek for other terms and to adjust all references
> to use the Glossary wording. This is a separate work than what this
> patch proposes to solve.
>
> Yet, I agree that ambiguity should be solved. I believe that the
> second version of this patch series address it.

As discussed over IRC we don't need a full glossary. We can start with
definitions of the terms we use here, and enrich it incrementally in the
future.

> > The word "device" is ambiguous, it can mean
> >
> > - device node
> >
> > The /dev device node through which a character- or block-based API is
> > exposed through userspace.
> >
> > The term "device node" is commonly used in the kernel, I think we can use
> > it as-is without ambiguity.
>
> Yes. IMO, we should use:
>
> - "device node" when it may refer to any device node,
> including mc and subdev;
> - "V4L2 device node" when it refers only to the devices that are now
> described at the "V4L2 Device Node Naming" section (see patch 1 of the
> new patch series).

And "V4L2 subdev device node" and "MC device node" when we want to refer
specifically to the /dev/v4l-subdev* and /dev/media* device nodes ?

> > - kernel struct device
> >
> > An instance of the kernel struct device. Kernel devices are used to
> > represent hardware devices (and are then embedded in bus-specific device
> > structure such as platform_device, pci_device or i2c_client) and logical
> > devices (and are then embedded in class-spcific device structures such as
> > struct video_device or struct net_device).
> >
> > For now I believe this isn't relevant to the V4L2 userspace API
> > discussion, so we can leave it out.
>
> Yes. Let's leave it out.
>
> > - hardware device
> >
> > The hardware counterpart of the first category of the kernel struct
> > device, a hardware (and thus physical) device such as an SoC IP core or
> > an I2C chip.
> >
> > We could call this "hardware device" or "hardware component". Other
> > proposals are welcome.
>
> as proposed by Hans, on the newer version, we're just using "hardware".

Hardware is very generic too. A PCI capture card is hardware, an antenna
connector is hardware, a TV tuner is hardware, a DMA engine is hardware.

The distinction between components and peripherals is in my opinion important,
otherwise we wouldn't have both struct video_device and struct v4l2_device.
Userspace middleware developers (I include libv4l2 in this category) will be
more interested in components, which applications developers will be more
interested in peripherals. In the longer term, we should introduce a
peripheral concept for userspace too (in the interface between middlewares and
applications), the same way we have done it in the kernel with struct
v4l2_device.

> > - hardware peripheral
> >
> > A group of hardware devices that together make a larger user-facing
> > functional peripheral. For instance the SoC ISP IP cores and external
> > camera sensors together make a camera peripheral.
> >
> > If we call the previous device a "hardware component" this could be called
> > "hardware device". Otherwise "hardware peripheral" is a possible name, but
> > we can probably come up with a better name.
> >
> > - software peripheral
> >
> > The software counterpart of the hardware peripheral. In V4L2 this is
> > modeled by a struct v4l2_device, often associated with a struct
> > media_device.
> >
> > I don't like the name "software peripheral", other proposals are welcome.
>
> I don't see the need of using those "peripheral" wording terms in this
> chapter. Let's leave it out for now.

I think they're needed. When you talk about pipeline configuration for
instance, you talk about peripherals. Individual components don't have
pipelines, they are assembled in a pipeline in a peripheral. For instance,
when you say "The entire device was controlled via the V4L2 device nodes", the
usage of the word "device" is ambiguous. I think we should avoid using
"device" without any qualifier except in context where there can be no
confusion (for instance in a section that repeatedly talk about device nodes
and devices nodes only, the word "device" could be used in some places as a
shortcut).

> > Once we agree on names and definitions I'll comment on the reworked
> > version of this patch, as it's pointless to bikeshed the wording without
> > first agreeing on clear definitions of the concepts we're discussing.
> > Please still see below for a few comments not related to particular
> > wording.
> >
> >> +The other drivers are called **V4L2 sub-devices** and provide control
> >> to
> >> +other parts of the hardware usually connected via a serial bus (like
> >> +I?C, SMBus or SPI). They can be implicitly controlled directly by the
> >> +main driver or explicitly through via the **V4L2 sub-device API**
> >> interface.
> > > +
> >> +When V4L2 was originally designed, there was only one type of device
> >> control.
> >> +The entire device was controlled via the **V4L2 device nodes**. We
> >> refer to
> >> +this kind of control as **V4L2 device node centric** (or, simply,
> >> +**vdev-centric**).
> >> +
> >> +Since the end of 2010, a new type of V4L2 device control was added in
> >> order
> >> +to support complex devices that are common for embedded systems. Those
> >> +devices are controlled mainly via the media controller and sub-devices.
> >> +So, they're called: **Media controller centric** (or, simply,
> > > +"**MC-centric**").
> >
> > Do we really need to explain the development history with dates ? I don't
> > think it helps understanding the separation between vdev-centric and MC-
> > centric devices.
>
> No, but it helps to identify when drivers became incompatible with
> generic apps. We could, instead point on what Kernel version it
> happened. That probably makes more sense and will match the history
> part of the spec.

That was v2.6.39. Do you expect userspace developers to target kernels older
than that ? :-)

> > For instance the previous paragraph that explains that V4L2
> > originally had a single type of device control that we now call
> > vdev-centric doesn't really help understanding what a vdev-centric device
> > is.
> >
> > It would be clearer in my opinion to explain the two kind of devices with
> > an associated use case, without referring to the history.
>
> The history is important for app developers, as they need to know
> what Kernel versions are affected by V4L2 device nodes that look
> like vdev-centric ones but aren't.
>
> Yet, you're right on one point: the date doesn't matter, but, instead,
> the Kernel version where the first mc-centric driver were added.
>
> >> +For **vdev-centric** control, the device and their corresponding
> >> hardware
> >> +pipelines are controlled via the **V4L2 device** node. They may
> >> optionally
> >> +expose via the :ref:`media controller API <media_controller>`.
> >> +
> >> +For **MC-centric** control, before using the V4L2 device, it is
> >> required to +set the hardware pipelines via the
> >> +:ref:`media controller API <media_controller>`. For those devices, the
> >> +sub-devices' configuration can be controlled via the
> >> +:ref:`sub-device API <subdev>`, with creates one device node per sub
> >> device.
> >> +
> >> +In summary, for **MC-centric** devices:
> >> +
> >> +- The **V4L2 device** node is responsible for controlling the streaming
> >> + features;
> >> +- The **media controller device** is responsible to setup the
> >> pipelines;
> >> +- The **V4L2 sub-devices** are responsible for sub-device
> >> + specific settings.
> >
> > How about a summary of vdev-centric devices too ? Or, possibly, no summary
> > at all ? The need to summarize 5 short paragraphs probably indicates that
> > those paragraphis are not clear enough :-)
>
> Done at version 2, already submitted.

I'll review v3 once we agree on definitions for device-related terms :-)

> > I can try to help by proposing enhancements to the documentation once we
> > agree on the glossary above.
> >
> >> +.. note::
> >> +
> >> + A **vdev-centric** may optionally expose V4L2 sub-devices via
> >> + :ref:`sub-device API <subdev>`. In that case, it has to implement
> >> + the :ref:`media controller API <media_controller>` as well.
> >
> > The separation between vdev-centric and MC-centric devices is quite clear.
> > If we allow a vdev-centric device to expose subdev nodes we will open the
> > door to all kind of hybrid implementations that have no clear definition
> > today.
>
> There are already such devices, as discussed at #media-maint ML. We're
> just documenting the existing status.

The specification doesn't need to document the odd cases that we would have
rejected if we had known better.

> > It will be very important in that case to document in details what is
> > allowed and what isn't, and how the video device nodes and subdev device
> > nodes interact with each other. I prefer not giving a green light to such
> > implementations until we have to, and I also prefer discussing this topic
> > separately. It will require a fair amount of work to document (and thus
> > first agree on), and there's no need to block the rest of this patch
> > until we complete that work. For those reasons I would like to explicitly
> > disallow those hybrid devices in this patch, and start discussing the use
> > cases we have for them, and how they would operate.
>
> What we can do is to add a notice that this is not recommended or
> that people should not add it without a discussion about what that
> is needed.

How about just saying that's it's forbidden by the specification ? We will
have a few drivers that don't comply, but we will obviously not blame their
authors.

I prefer using the strongest possible wording when we want to discourage
something. There will always be developers who will still try, but the
stronger the wording, the lower the risk.

--
Regards,

Laurent Pinchart

2017-08-25 14:02:06

by Hans Verkuil

[permalink] [raw]

Subject: Re: [PATCH 1/3] media: open.rst: document devnode-centric and mc-centric types

On 25/08/17 15:11, Laurent Pinchart wrote:
> Hi Mauro,
>
> Thank you for the patch.
>
> On Friday, 25 August 2017 12:40:05 EEST Mauro Carvalho Chehab wrote:
>> From: "[email protected]" <[email protected]>
>>
>> When we added support for omap3, back in 2010, we added a new
>> type of V4L2 devices that aren't fully controlled via the V4L2
>> device node. Yet, we never made it clear, at the V4L2 spec,
>> about the differences between both types.
>
> Nitpicking (and there will be lots of nitpicking in this review as I think
> it's very important to get this piece of the documentation right down to
> details):
>
> s/at the V4L2 spec/in the V4L2 spec/
>
> "make it clear about" doesn't sound good to me. How about the following ?
>
> "Yet, we have never clearly documented in the V4L2 specification the
> differences between the two types."
>
>> Let's document them with the current implementation.
>
> s/with the/based on the/
>
>>
>> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
>> ---
>> Documentation/media/uapi/v4l/open.rst | 50 ++++++++++++++++++++++++++++++++
>> 1 file changed, 50 insertions(+)
>>
>> diff --git a/Documentation/media/uapi/v4l/open.rst
>> b/Documentation/media/uapi/v4l/open.rst index afd116edb40d..a72d142897c0
>> 100644
>> --- a/Documentation/media/uapi/v4l/open.rst
>> +++ b/Documentation/media/uapi/v4l/open.rst
>> @@ -6,6 +6,56 @@
>> Opening and Closing Devices
>> ***************************
>>
>> +Types of V4L2 hardware control
>> +==============================
>> +
>> +V4L2 devices are usually complex: they are implemented via a main driver
>> and
>> +often several additional drivers. The main driver always exposes one or
>> +more **V4L2 device** devnodes (see :ref:`v4l2_device_naming`).
>
> First of all, as stated in a previous e-mail, I think we should start by
> defining the terms we are going to use. The word "device" is ambiguous, it can
> mean
>
> - device node
>
> The /dev device node through which a character- or block-based API is exposed
> through userspace.
>
> The term "device node" is commonly used in the kernel, I think we can use it
> as-is without ambiguity.

Ack.

>
> - kernel struct device
>
> An instance of the kernel struct device. Kernel devices are used to represent
> hardware devices (and are then embedded in bus-specific device structure such
> as platform_device, pci_device or i2c_client) and logical devices (and are
> then embedded in class-spcific device structures such as struct video_device
> or struct net_device).
>
> For now I believe this isn't relevant to the V4L2 userspace API discussion, so
> we can leave it out.

Agree.

>
> - hardware device
>
> The hardware counterpart of the first category of the kernel struct device, a
> hardware (and thus physical) device such as an SoC IP core or an I2C chip.
>
> We could call this "hardware device" or "hardware component". Other proposals
> are welcome.

I like "hardware component". I prefer to avoid the word 'device' as much as possible.

>
> - hardware peripheral
>
> A group of hardware devices that together make a larger user-facing functional
> peripheral. For instance the SoC ISP IP cores and external camera sensors
> together make a camera peripheral.
>
> If we call the previous device a "hardware component" this could be called
> "hardware device". Otherwise "hardware peripheral" is a possible name, but we
> can probably come up with a better name.

"hardware device" can be used provided it is clearly defined as being a collection
of hardware components that together form the fully functioning device. Or something
along those lines.

>
> - software peripheral
>
> The software counterpart of the hardware peripheral. In V4L2 this is modeled
> by a struct v4l2_device, often associated with a struct media_device.

struct v4l2_device isn't visible to userspace (and is an awful name, I'm
really sorry about that, guys!).

>
> I don't like the name "software peripheral", other proposals are welcome.

Where would you use the term "software peripheral"? In what context? Do we
need this at all?

>
> Once we agree on names and definitions I'll comment on the reworked version of
> this patch, as it's pointless to bikeshed the wording without first agreeing
> on clear definitions of the concepts we're discussing. Please still see below
> for a few comments not related to particular wording.
>
>> +The other drivers are called **V4L2 sub-devices** and provide control to
>> +other parts of the hardware usually connected via a serial bus (like
>> +I?C, SMBus or SPI). They can be implicitly controlled directly by the
>> +main driver or explicitly through via the **V4L2 sub-device API**
>> interface.
>> +
>> +When V4L2 was originally designed, there was only one type of device
>> control.
>> +The entire device was controlled via the **V4L2 device nodes**. We refer to
>> +this kind of control as **V4L2 device node centric** (or,
>> simply, +**vdev-centric**).
>> +
>> +Since the end of 2010, a new type of V4L2 device control was added in order
>> +to support complex devices that are common for embedded systems. Those
>> +devices are controlled mainly via the media controller and sub-devices.
>> +So, they're called: **Media controller centric** (or, simply,
>> +"**MC-centric**").
>
> Do we really need to explain the development history with dates ? I don't
> think it helps understanding the separation between vdev-centric and MC-
> centric devices. For instance the previous paragraph that explains that V4L2
> originally had a single type of device control that we now call vdev-centric
> doesn't really help understanding what a vdev-centric device is.
>
> It would be clearer in my opinion to explain the two kind of devices with an
> associated use case, without referring to the history.

It doesn't hurt to mention that MC-centric came later. There are after all API
oddities that are due to the history of the API. But we can simply say "Later"
instead of "Since the end of 2010".

>> +For **vdev-centric** control, the device and their corresponding hardware
>> +pipelines are controlled via the **V4L2 device** node. They may optionally
>> +expose via the :ref:`media controller API <media_controller>`.
>> +
>> +For **MC-centric** control, before using the V4L2 device, it is required to
>> +set the hardware pipelines via the
>> +:ref:`media controller API <media_controller>`. For those devices, the
>> +sub-devices' configuration can be controlled via the
>> +:ref:`sub-device API <subdev>`, with creates one device node per sub
>> device.
>> +
>> +In summary, for **MC-centric** devices:
>> +
>> +- The **V4L2 device** node is responsible for controlling the streaming
>> + features;
>> +- The **media controller device** is responsible to setup the pipelines;
>> +- The **V4L2 sub-devices** are responsible for sub-device
>> + specific settings.
>
> How about a summary of vdev-centric devices too ? Or, possibly, no summary at
> all ? The need to summarize 5 short paragraphs probably indicates that those
> paragraphis are not clear enough :-)

I was thinking the same. It should probably be dropped.

> I can try to help by proposing enhancements to the documentation once we agree
> on the glossary above.
>
>> +.. note::
>> +
>> + A **vdev-centric** may optionally expose V4L2 sub-devices via
>> + :ref:`sub-device API <subdev>`. In that case, it has to implement
>> + the :ref:`media controller API <media_controller>` as well.
>
> The separation between vdev-centric and MC-centric devices is quite clear. If
> we allow a vdev-centric device to expose subdev nodes we will open the door to
> all kind of hybrid implementations that have no clear definition today. It
> will be very important in that case to document in details what is allowed and
> what isn't, and how the video device nodes and subdev device nodes interact
> with each other. I prefer not giving a green light to such implementations
> until we have to

Well, they exist already. The original MC design that I made allowed for this.
In practice the only reason for doing this is to set private controls for
subdevs (i.e. controls not inherited by the video device node).

Three drivers use this: rcar-drif, cobalt (not an issue, I can just remove
support for this as Cisco is the only user) and I know there was a third one
that you found, but I keep forgetting the name of that driver.

>, and I also prefer discussing this topic separately. It will
> require a fair amount of work to document (and thus first agree on), and
> there's no need to block the rest of this patch until we complete that work.
> For those reasons I would like to explicitly disallow those hybrid devices in
> this patch, and start discussing the use cases we have for them, and how they
> would operate.

I have no objection to dropping this note for the first version.

Regards,

Hans

>
>> +.. _v4l2_device_naming:
>>
>> Device Naming
>> =============
>
>