mbox series

[v3,0/6] V4L2 driver documentation, v4l2-async improvements

Message ID 20210624084046.13136-1-sakari.ailus@linux.intel.com
Headers show
Series V4L2 driver documentation, v4l2-async improvements | expand

Message

Sakari Ailus June 24, 2021, 8:40 a.m. UTC
Hi folks,

Here's a refresh of my somewhat old patches, improving V4L2 camera sensor,
CSI-2 and parallel interface documentation as well as v4l2-async. I've
tried to take feedback into account but unfortunately I no longer have
details of that. A lot has been rewritten in the documentation in any
case.

I'd like to get these to 5.15 early on (unless somehow 5.14 is possible)
so comments would be welcome now.

Note that the documentation of frame rate configuration can be improved,
too, but I think that can be after this set.

since v1:

- Fix compile problems in async notifier rename patch.

- Language fixes and other minor improvements in tx-rx documentation.

Niklas Söderlund (2):
  media: v4l2-fwnode: Simplify v4l2_async_nf_parse_fwnode_endpoints()
  media: rcar-vin: Remove explicit device availability check

Sakari Ailus (4):
  Documentation: media: Improve camera sensor documentation
  Documentation: media: Fix v4l2-async kerneldoc syntax
  v4l: async: Rename async nf functions, clean up long lines
  Documentation: v4l: Fix V4L2_CID_PIXEL_RATE documentation

 .../driver-api/media/camera-sensor.rst        |  45 ++---
 Documentation/driver-api/media/csi2.rst       |  94 ----------
 Documentation/driver-api/media/index.rst      |   2 +-
 Documentation/driver-api/media/tx-rx.rst      | 117 ++++++++++++
 .../driver-api/media/v4l2-subdev.rst          |  14 +-
 .../media/v4l/ext-ctrls-image-process.rst     |   6 +-
 drivers/media/i2c/max9286.c                   |  17 +-
 drivers/media/i2c/st-mipid02.c                |  22 ++-
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  17 +-
 drivers/media/platform/am437x/am437x-vpfe.c   |  19 +-
 drivers/media/platform/atmel/atmel-isc-base.c |   4 +-
 drivers/media/platform/atmel/atmel-isi.c      |  17 +-
 .../media/platform/atmel/atmel-sama5d2-isc.c  |  15 +-
 .../media/platform/atmel/atmel-sama7g5-isc.c  |  15 +-
 drivers/media/platform/cadence/cdns-csi2rx.c  |  14 +-
 drivers/media/platform/davinci/vpif_capture.c |  21 +--
 drivers/media/platform/exynos4-is/media-dev.c |  20 +--
 .../media/platform/marvell-ccic/cafe-driver.c |   9 +-
 .../media/platform/marvell-ccic/mcam-core.c   |  10 +-
 .../media/platform/marvell-ccic/mmp-driver.c  |   6 +-
 drivers/media/platform/omap3isp/isp.c         |  21 ++-
 drivers/media/platform/pxa_camera.c           |  26 ++-
 drivers/media/platform/qcom/camss/camss.c     |  18 +-
 drivers/media/platform/rcar-vin/rcar-core.c   |  41 ++---
 drivers/media/platform/rcar-vin/rcar-csi2.c   |  19 +-
 drivers/media/platform/rcar_drif.c            |  14 +-
 drivers/media/platform/renesas-ceu.c          |  29 ++-
 .../platform/rockchip/rkisp1/rkisp1-dev.c     |  17 +-
 drivers/media/platform/stm32/stm32-dcmi.c     |  18 +-
 .../platform/sunxi/sun4i-csi/sun4i_csi.c      |  12 +-
 .../platform/sunxi/sun6i-csi/sun6i_csi.c      |  19 +-
 drivers/media/platform/ti-vpe/cal.c           |  16 +-
 drivers/media/platform/video-mux.c            |  17 +-
 drivers/media/platform/xilinx/xilinx-vipp.c   |  17 +-
 drivers/media/v4l2-core/v4l2-async.c          | 168 +++++++++---------
 drivers/media/v4l2-core/v4l2-fwnode.c         |  83 +++------
 drivers/staging/media/imx/imx-media-csi.c     |  17 +-
 .../staging/media/imx/imx-media-dev-common.c  |   7 +-
 drivers/staging/media/imx/imx-media-dev.c     |   6 +-
 drivers/staging/media/imx/imx-media-of.c      |   6 +-
 drivers/staging/media/imx/imx6-mipi-csi2.c    |  17 +-
 drivers/staging/media/imx/imx7-media-csi.c    |  24 +--
 drivers/staging/media/imx/imx7-mipi-csis.c    |  16 +-
 drivers/staging/media/tegra-video/vi.c        |  17 +-
 include/media/v4l2-async.h                    | 105 ++++++-----
 include/media/v4l2-fwnode.h                   |  12 +-
 46 files changed, 607 insertions(+), 639 deletions(-)
 delete mode 100644 Documentation/driver-api/media/csi2.rst
 create mode 100644 Documentation/driver-api/media/tx-rx.rst

Comments

Kieran Bingham July 27, 2021, 10:54 a.m. UTC | #1
Hi Sakari

On 24/06/2021 09:40, Sakari Ailus wrote:
> Modernise the documentation to make it more precise and update the use of

> pixel rate control and various other changes. In particular:

> 

> - Use non-proportional font for file names, properties as well as

>   controls.

> 

> - The unit of the HBLANK control is pixels, not lines.

> 

> - The unit of PIXEL_RATE control is pixels per second, not Hz.

> 

> - Merge common requirements for CSI-2 and parallel busses.>

> - Include all DT properties needed for assigned clocks.

> 

> - Fix referencing the link rate control.

> 

> - SMIA driver's new name is CCS driver.

> 

> - The PIXEL_RATE control denotes pixel rate on the pixel array on camera

>   sensors. Do not suggest it is used to tell the maximum pixel rate on the

>   bus anymore.

> 

> - Improve ReST syntax (plain struct and function names).

> 

> - Remove the suggestion to use s_power() in receiver drivers.

> 

> - Make MIPI website URL use HTTPS, add Wikipedia links to BT.601 and

>   BT.656.

> 

> Fixes: e4cf8c58af75 ("media: Documentation: media: Document how to write camera sensor drivers")

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

> Reviewed-by: Andrey Konovalov <andrey.konovalov@linaro.org>

> ---

>  .../driver-api/media/camera-sensor.rst        |  45 +++----

>  Documentation/driver-api/media/csi2.rst       |  94 --------------

>  Documentation/driver-api/media/index.rst      |   2 +-

>  Documentation/driver-api/media/tx-rx.rst      | 117 ++++++++++++++++++

>  .../media/v4l/ext-ctrls-image-process.rst     |   2 +

>  5 files changed, 137 insertions(+), 123 deletions(-)

>  delete mode 100644 Documentation/driver-api/media/csi2.rst

>  create mode 100644 Documentation/driver-api/media/tx-rx.rst

> 

> diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst

> index 7160336aa475..c7d4891bd24e 100644

> --- a/Documentation/driver-api/media/camera-sensor.rst

> +++ b/Documentation/driver-api/media/camera-sensor.rst

> @@ -3,10 +3,10 @@

>  Writing camera sensor drivers

>  =============================

>  

> -CSI-2

> ------

> +CSI-2 and parallel (BT.601 and BT.656) busses


Busses looks odd to me, so I've got to look it up:
  https://www.grammarly.com/blog/busses-buses/

Though I found this one more of an entertaining read:
  https://www.merriam-webster.com/words-at-play/plural-of-bus

Technically, some dictionaries still reference busses it seems, so this
isn't specifically an error, just that buses is more commonly used (and
it doesn't appear to be a US/UK thing?)


> +---------------------------------------------

>  

> -Please see what is written on :ref:`MIPI_CSI_2`.

> +Please see :ref:`transmitter-receiver`.

>  

>  Handling clocks

>  ---------------

> @@ -26,15 +26,16 @@ user.

>  ACPI

>  ~~~~

>  

> -Read the "clock-frequency" _DSD property to denote the frequency. The driver can

> -rely on this frequency being used.

> +Read the ``clock-frequency`` _DSD property to denote the frequency. The driver

> +can rely on this frequency being used.

>  

>  Devicetree

>  ~~~~~~~~~~

>  

> -The currently preferred way to achieve this is using "assigned-clock-rates"

> -property. See Documentation/devicetree/bindings/clock/clock-bindings.txt for

> -more information. The driver then gets the frequency using clk_get_rate().

> +The currently preferred way to achieve this is using ``assigned-clocks``,

> +``assigned-clock-parents`` and ``assigned-clock-rates`` properties. See

> +``Documentation/devicetree/bindings/clock/clock-bindings.txt`` for more

> +information. The driver then gets the frequency using ``clk_get_rate()``.

>  

>  This approach has the drawback that there's no guarantee that the frequency

>  hasn't been modified directly or indirectly by another driver, or supported by

> @@ -55,7 +56,7 @@ processing pipeline as one or more sub-devices with different cropping and

>  scaling configurations. The output size of the device is the result of a series

>  of cropping and scaling operations from the device's pixel array's size.

>  

> -An example of such a driver is the smiapp driver (see drivers/media/i2c/smiapp).

> +An example of such a driver is the CCS driver (see ``drivers/media/i2c/ccs``).

>  

>  Register list based drivers

>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~

> @@ -67,7 +68,7 @@ level are independent. How a driver picks such configuration is based on the

>  format set on a source pad at the end of the device's internal pipeline.

>  

>  Most sensor drivers are implemented this way, see e.g.

> -drivers/media/i2c/imx319.c for an example.

> +``drivers/media/i2c/imx319.c`` for an example.

>  

>  Frame interval configuration

>  ----------------------------

> @@ -94,9 +95,10 @@ large variety of devices beyond camera sensors. Devices that have no analogue

>  crop, use the full source image size, i.e. pixel array size.

>  

>  Horizontal and vertical blanking are specified by ``V4L2_CID_HBLANK`` and

> -``V4L2_CID_VBLANK``, respectively. The unit of these controls are lines. The

> -pixel rate is specified by ``V4L2_CID_PIXEL_RATE`` in the same sub-device. The

> -unit of that control is Hz.

> +``V4L2_CID_VBLANK``, respectively. The unit of the ``V4L2_CID_HBLANK`` control

> +is pixels and the unit of the ``V4L2_CID_VBLANK`` is lines. The pixel rate in

> +the sensor's **pixel array** is specified by ``V4L2_CID_PIXEL_RATE`` in the same

> +sub-device. The unit of that control is pixels per second.

>  

>  Register list based drivers need to implement read-only sub-device nodes for the

>  purpose. Devices that are not register list based need these to configure the

> @@ -125,14 +127,14 @@ general, the device must be powered on at least when its registers are being

>  accessed and when it is streaming.

>  

>  Existing camera sensor drivers may rely on the old

> -:c:type:`v4l2_subdev_core_ops`->s_power() callback for bridge or ISP drivers to

> +struct v4l2_subdev_core_ops->s_power() callback for bridge or ISP drivers to

>  manage their power state. This is however **deprecated**. If you feel you need

>  to begin calling an s_power from an ISP or a bridge driver, instead please add

>  runtime PM support to the sensor driver you are using. Likewise, new drivers

>  should not use s_power.

>  

>  Please see examples in e.g. ``drivers/media/i2c/ov8856.c`` and

> -``drivers/media/i2c/smiapp/smiapp-core.c``. The two drivers work in both ACPI

> +``drivers/media/i2c/ccs/ccs-core.c``. The two drivers work in both ACPI

>  and DT based systems.

>  

>  Control framework

> @@ -149,16 +151,3 @@ used to obtain device's power state after the power state transition:

>  The function returns a non-zero value if it succeeded getting the power count or

>  runtime PM was disabled, in either of which cases the driver may proceed to

>  access the device.

> -

> -Controls

> ---------

> -

> -For camera sensors that are connected to a bus where transmitter and receiver

> -require common configuration set by drivers, such as CSI-2 or parallel (BT.601

> -or BT.656) bus, the ``V4L2_CID_LINK_FREQ`` control is mandatory on transmitter

> -drivers. Receiver drivers can use the ``V4L2_CID_LINK_FREQ`` to query the

> -frequency used on the bus.

> -

> -The transmitter drivers should also implement ``V4L2_CID_PIXEL_RATE`` control in

> -order to tell the maximum pixel rate to the receiver. This is required on raw

> -camera sensors.

> diff --git a/Documentation/driver-api/media/csi2.rst b/Documentation/driver-api/media/csi2.rst

> deleted file mode 100644

> index 11c52b0be8b8..000000000000

> --- a/Documentation/driver-api/media/csi2.rst

> +++ /dev/null

> @@ -1,94 +0,0 @@

> -.. SPDX-License-Identifier: GPL-2.0

> -

> -.. _MIPI_CSI_2:

> -

> -MIPI CSI-2

> -==========

> -

> -CSI-2 is a data bus intended for transferring images from cameras to

> -the host SoC. It is defined by the `MIPI alliance`_.

> -

> -.. _`MIPI alliance`: http://www.mipi.org/

> -

> -Media bus formats

> ------------------

> -

> -See :ref:`v4l2-mbus-pixelcode` for details on which media bus formats should

> -be used for CSI-2 interfaces.

> -

> -Transmitter drivers

> --------------------

> -

> -CSI-2 transmitter, such as a sensor or a TV tuner, drivers need to

> -provide the CSI-2 receiver with information on the CSI-2 bus

> -configuration. These include the V4L2_CID_LINK_FREQ and

> -V4L2_CID_PIXEL_RATE controls and

> -(:c:type:`v4l2_subdev_video_ops`->s_stream() callback). These

> -interface elements must be present on the sub-device represents the

> -CSI-2 transmitter.

> -

> -The V4L2_CID_LINK_FREQ control is used to tell the receiver driver the

> -frequency (and not the symbol rate) of the link. The V4L2_CID_PIXEL_RATE

> -control may be used by the receiver to obtain the pixel rate the transmitter

> -uses. The :c:type:`v4l2_subdev_video_ops`->s_stream() callback provides an

> -ability to start and stop the stream.

> -

> -The value of the V4L2_CID_PIXEL_RATE is calculated as follows::

> -

> -	pixel_rate = link_freq * 2 * nr_of_lanes * 16 / k / bits_per_sample

> -

> -where

> -

> -.. list-table:: variables in pixel rate calculation

> -   :header-rows: 1

> -

> -   * - variable or constant

> -     - description

> -   * - link_freq

> -     - The value of the V4L2_CID_LINK_FREQ integer64 menu item.

> -   * - nr_of_lanes

> -     - Number of data lanes used on the CSI-2 link. This can

> -       be obtained from the OF endpoint configuration.

> -   * - 2

> -     - Two bits are transferred per clock cycle per lane.

> -   * - bits_per_sample

> -     - Number of bits per sample.

> -   * - k

> -     - 16 for D-PHY and 7 for C-PHY

> -

> -The transmitter drivers must, if possible, configure the CSI-2

> -transmitter to *LP-11 mode* whenever the transmitter is powered on but

> -not active, and maintain *LP-11 mode* until stream on. Only at stream

> -on should the transmitter activate the clock on the clock lane and

> -transition to *HS mode*.

> -

> -Some transmitters do this automatically but some have to be explicitly

> -programmed to do so, and some are unable to do so altogether due to

> -hardware constraints.

> -

> -Stopping the transmitter

> -^^^^^^^^^^^^^^^^^^^^^^^^

> -

> -A transmitter stops sending the stream of images as a result of

> -calling the ``.s_stream()`` callback. Some transmitters may stop the

> -stream at a frame boundary whereas others stop immediately,

> -effectively leaving the current frame unfinished. The receiver driver

> -should not make assumptions either way, but function properly in both

> -cases.

> -

> -Receiver drivers

> -----------------

> -

> -Before the receiver driver may enable the CSI-2 transmitter by using

> -the :c:type:`v4l2_subdev_video_ops`->s_stream(), it must have powered

> -the transmitter up by using the

> -:c:type:`v4l2_subdev_core_ops`->s_power() callback. This may take

> -place either indirectly by using :c:func:`v4l2_pipeline_pm_get` or

> -directly.

> -

> -Formats

> --------

> -

> -The media bus pixel codes document parallel formats. Should the pixel data be

> -transported over a serial bus, the media bus pixel code that describes a

> -parallel format that transfers a sample on a single clock cycle is used.

> diff --git a/Documentation/driver-api/media/index.rst b/Documentation/driver-api/media/index.rst

> index 813d7db59da7..08e206567408 100644

> --- a/Documentation/driver-api/media/index.rst

> +++ b/Documentation/driver-api/media/index.rst

> @@ -37,7 +37,7 @@ Documentation/userspace-api/media/index.rst

>      rc-core

>      mc-core

>      cec-core

> -    csi2

> +    tx-rx

>      camera-sensor

>  

>      drivers/index

> diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst

> new file mode 100644

> index 000000000000..4c8584e7b6f2

> --- /dev/null

> +++ b/Documentation/driver-api/media/tx-rx.rst

> @@ -0,0 +1,117 @@

> +.. SPDX-License-Identifier: GPL-2.0

> +

> +.. _transmitter-receiver:

> +

> +Pixel data transmitter and receiver drivers

> +===========================================

> +

> +V4L2 supports various devices that transmit and receiver pixel data. Examples of

> +these devices include a camera sensor, a TV tuner and a parallel or a CSI-2

> +receiver in an SoC.

> +

> +Bus types

> +---------

> +

> +The following busses are the most common. This section discusses these two only.

> +

> +MIPI CSI-2

> +^^^^^^^^^^

> +

> +CSI-2 is a data bus intended for transferring images from cameras to

> +the host SoC. It is defined by the `MIPI alliance`_.

> +

> +.. _`MIPI alliance`: https://www.mipi.org/

> +

> +Parallel

> +^^^^^^^^

> +

> +`BT.601`_ and `BT.656`_ are the most common parallel busses.

> +

> +.. _`BT.601`: https://en.wikipedia.org/wiki/Rec._601

> +.. _`BT.656`: https://en.wikipedia.org/wiki/ITU-R_BT.656

> +

> +Transmitter drivers

> +-------------------

> +

> +Transmitter drivers generally need to provide the receiver drivers with the

> +configuration of the transmitter. What is required depends on the type of the

> +bus. These are common for both busses.

> +

> +Media bus pixel code

> +^^^^^^^^^^^^^^^^^^^^

> +

> +See :ref:`v4l2-mbus-pixelcode`.

> +

> +Link frequency

> +^^^^^^^^^^^^^^

> +

> +The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the

> +receiver the frequency of the bus (i.e. it is not the same as the symbol rate).

> +


Would the symbol rate be the same as the pixel rate, or is the 'bit'
rate? ( I believe it's the bit rate, but I wonder if it needs to be
defined here to make it clear?)

I guess this is the distinction that the bus may send two bits per clock
cycle or such.


> +``.s_stream()`` callback

> +^^^^^^^^^^^^^^^^^^^^^^^^

> +

> +The struct struct v4l2_subdev_video_ops->s_stream() callback is used by the

> +receiver driver to control the transmitter driver's streaming state.

> +

> +

> +CSI-2 transmitter drivers

> +-------------------------

> +

> +Pixel rate

> +^^^^^^^^^^

> +

> +The pixel rate on the bus is calculated as follows::

> +

> +	pixel_rate = link_freq * 2 * nr_of_lanes * 16 / k / bits_per_sample

> +

> +where

> +

> +.. list-table:: variables in pixel rate calculation

> +   :header-rows: 1

> +

> +   * - variable or constant

> +     - description

> +   * - link_freq

> +     - The value of the ``V4L2_CID_LINK_FREQ`` integer64 menu item.

> +   * - nr_of_lanes

> +     - Number of data lanes used on the CSI-2 link. This can

> +       be obtained from the OF endpoint configuration.

> +   * - 2

> +     - Data is transferred on both rising and falling edge of the signal.

> +   * - bits_per_sample

> +     - Number of bits per sample.

> +   * - k

> +     - 16 for D-PHY and 7 for C-PHY


Is 'k' a defined term here? It makes me assume kilo ... which clearly
isn't its usage with values of 16 and 7?


Fairly optional comments though so :

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>



> +

> +.. note::

> +

> +	The pixel rate calculated this way is **not** the same thing as the

> +	pixel rate on the camera sensor's pixel array which is indicated by the

> +	:ref:`V4L2_CID_PIXEL_RATE <v4l2-cid-pixel-rate>` control.

> +

> +LP-11 and LP-111 modes

> +^^^^^^^^^^^^^^^^^^^^^^

> +

> +The transmitter drivers must, if possible, configure the CSI-2 transmitter to

> +*LP-11 or LP-111 mode* whenever the transmitter is powered on but not active,

> +and maintain *LP-11 or LP-111 mode* until stream on. Only at stream on should

> +the transmitter activate the clock on the clock lane and transition to *HS

> +mode*.

> +

> +Some transmitters do this automatically but some have to be explicitly

> +programmed to do so, and some are unable to do so altogether due to

> +hardware constraints.

> +

> +The receiver thus need to be configured to expect LP-11 or LP-111 mode from the

> +transmitter before the transmitter driver's ``.s_stream()`` op is called.

> +

> +Stopping the transmitter

> +^^^^^^^^^^^^^^^^^^^^^^^^

> +

> +A transmitter stops sending the stream of images as a result of

> +calling the ``.s_stream()`` callback. Some transmitters may stop the

> +stream at a frame boundary whereas others stop immediately,

> +effectively leaving the current frame unfinished. The receiver driver

> +should not make assumptions either way, but function properly in both

> +cases.

> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst

> index 87698c15c027..37dad2f4df8c 100644

> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst

> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst

> @@ -20,6 +20,8 @@ Image Process Control IDs

>  ``V4L2_CID_IMAGE_PROC_CLASS (class)``

>      The IMAGE_PROC class descriptor.

>  

> +.. _v4l2-cid-link-freq:

> +

>  ``V4L2_CID_LINK_FREQ (integer menu)``

>      Data bus frequency. Together with the media bus pixel code, bus type

>      (clock cycles per sample), the data bus frequency defines the pixel

>
Sakari Ailus July 27, 2021, 11:27 a.m. UTC | #2
Hi Kieran,

Thanks for the review.

On Tue, Jul 27, 2021 at 11:54:14AM +0100, Kieran Bingham wrote:
> Hi Sakari

> 

> On 24/06/2021 09:40, Sakari Ailus wrote:

> > Modernise the documentation to make it more precise and update the use of

> > pixel rate control and various other changes. In particular:

> > 

> > - Use non-proportional font for file names, properties as well as

> >   controls.

> > 

> > - The unit of the HBLANK control is pixels, not lines.

> > 

> > - The unit of PIXEL_RATE control is pixels per second, not Hz.

> > 

> > - Merge common requirements for CSI-2 and parallel busses.>

> > - Include all DT properties needed for assigned clocks.

> > 

> > - Fix referencing the link rate control.

> > 

> > - SMIA driver's new name is CCS driver.

> > 

> > - The PIXEL_RATE control denotes pixel rate on the pixel array on camera

> >   sensors. Do not suggest it is used to tell the maximum pixel rate on the

> >   bus anymore.

> > 

> > - Improve ReST syntax (plain struct and function names).

> > 

> > - Remove the suggestion to use s_power() in receiver drivers.

> > 

> > - Make MIPI website URL use HTTPS, add Wikipedia links to BT.601 and

> >   BT.656.

> > 

> > Fixes: e4cf8c58af75 ("media: Documentation: media: Document how to write camera sensor drivers")

> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

> > Reviewed-by: Andrey Konovalov <andrey.konovalov@linaro.org>

> > ---

> >  .../driver-api/media/camera-sensor.rst        |  45 +++----

> >  Documentation/driver-api/media/csi2.rst       |  94 --------------

> >  Documentation/driver-api/media/index.rst      |   2 +-

> >  Documentation/driver-api/media/tx-rx.rst      | 117 ++++++++++++++++++

> >  .../media/v4l/ext-ctrls-image-process.rst     |   2 +

> >  5 files changed, 137 insertions(+), 123 deletions(-)

> >  delete mode 100644 Documentation/driver-api/media/csi2.rst

> >  create mode 100644 Documentation/driver-api/media/tx-rx.rst

> > 

> > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst

> > index 7160336aa475..c7d4891bd24e 100644

> > --- a/Documentation/driver-api/media/camera-sensor.rst

> > +++ b/Documentation/driver-api/media/camera-sensor.rst

> > @@ -3,10 +3,10 @@

> >  Writing camera sensor drivers

> >  =============================

> >  

> > -CSI-2

> > ------

> > +CSI-2 and parallel (BT.601 and BT.656) busses

> 

> Busses looks odd to me, so I've got to look it up:

>   https://www.grammarly.com/blog/busses-buses/

> 

> Though I found this one more of an entertaining read:

>   https://www.merriam-webster.com/words-at-play/plural-of-bus

> 

> Technically, some dictionaries still reference busses it seems, so this

> isn't specifically an error, just that buses is more commonly used (and

> it doesn't appear to be a US/UK thing?)


It's interesting you say that.

The Cambridge dictionary notes "busses" is American and then contradicts
itself by saying it's the other way around. I thought so, too.

Foldoc exclusively uses "busses" as plural of "bus" in its description of
the word "bus".

Then I also read in Wiktionary "buss" is also entirely valid, but has other
meanings, too. I thought it was only in Swedish. "Bus" means a "hobo" in
Swedish.

I like busses. I don't think there's much risk of mistaking it with e.g. a
herring buss in this case.

> 

> 

> > +---------------------------------------------

> >  

> > -Please see what is written on :ref:`MIPI_CSI_2`.

> > +Please see :ref:`transmitter-receiver`.

> >  

> >  Handling clocks

> >  ---------------

> > @@ -26,15 +26,16 @@ user.

> >  ACPI

> >  ~~~~

> >  

> > -Read the "clock-frequency" _DSD property to denote the frequency. The driver can

> > -rely on this frequency being used.

> > +Read the ``clock-frequency`` _DSD property to denote the frequency. The driver

> > +can rely on this frequency being used.

> >  

> >  Devicetree

> >  ~~~~~~~~~~

> >  

> > -The currently preferred way to achieve this is using "assigned-clock-rates"

> > -property. See Documentation/devicetree/bindings/clock/clock-bindings.txt for

> > -more information. The driver then gets the frequency using clk_get_rate().

> > +The currently preferred way to achieve this is using ``assigned-clocks``,

> > +``assigned-clock-parents`` and ``assigned-clock-rates`` properties. See

> > +``Documentation/devicetree/bindings/clock/clock-bindings.txt`` for more

> > +information. The driver then gets the frequency using ``clk_get_rate()``.

> >  

> >  This approach has the drawback that there's no guarantee that the frequency

> >  hasn't been modified directly or indirectly by another driver, or supported by

> > @@ -55,7 +56,7 @@ processing pipeline as one or more sub-devices with different cropping and

> >  scaling configurations. The output size of the device is the result of a series

> >  of cropping and scaling operations from the device's pixel array's size.

> >  

> > -An example of such a driver is the smiapp driver (see drivers/media/i2c/smiapp).

> > +An example of such a driver is the CCS driver (see ``drivers/media/i2c/ccs``).

> >  

> >  Register list based drivers

> >  ~~~~~~~~~~~~~~~~~~~~~~~~~~~

> > @@ -67,7 +68,7 @@ level are independent. How a driver picks such configuration is based on the

> >  format set on a source pad at the end of the device's internal pipeline.

> >  

> >  Most sensor drivers are implemented this way, see e.g.

> > -drivers/media/i2c/imx319.c for an example.

> > +``drivers/media/i2c/imx319.c`` for an example.

> >  

> >  Frame interval configuration

> >  ----------------------------

> > @@ -94,9 +95,10 @@ large variety of devices beyond camera sensors. Devices that have no analogue

> >  crop, use the full source image size, i.e. pixel array size.

> >  

> >  Horizontal and vertical blanking are specified by ``V4L2_CID_HBLANK`` and

> > -``V4L2_CID_VBLANK``, respectively. The unit of these controls are lines. The

> > -pixel rate is specified by ``V4L2_CID_PIXEL_RATE`` in the same sub-device. The

> > -unit of that control is Hz.

> > +``V4L2_CID_VBLANK``, respectively. The unit of the ``V4L2_CID_HBLANK`` control

> > +is pixels and the unit of the ``V4L2_CID_VBLANK`` is lines. The pixel rate in

> > +the sensor's **pixel array** is specified by ``V4L2_CID_PIXEL_RATE`` in the same

> > +sub-device. The unit of that control is pixels per second.

> >  

> >  Register list based drivers need to implement read-only sub-device nodes for the

> >  purpose. Devices that are not register list based need these to configure the

> > @@ -125,14 +127,14 @@ general, the device must be powered on at least when its registers are being

> >  accessed and when it is streaming.

> >  

> >  Existing camera sensor drivers may rely on the old

> > -:c:type:`v4l2_subdev_core_ops`->s_power() callback for bridge or ISP drivers to

> > +struct v4l2_subdev_core_ops->s_power() callback for bridge or ISP drivers to

> >  manage their power state. This is however **deprecated**. If you feel you need

> >  to begin calling an s_power from an ISP or a bridge driver, instead please add

> >  runtime PM support to the sensor driver you are using. Likewise, new drivers

> >  should not use s_power.

> >  

> >  Please see examples in e.g. ``drivers/media/i2c/ov8856.c`` and

> > -``drivers/media/i2c/smiapp/smiapp-core.c``. The two drivers work in both ACPI

> > +``drivers/media/i2c/ccs/ccs-core.c``. The two drivers work in both ACPI

> >  and DT based systems.

> >  

> >  Control framework

> > @@ -149,16 +151,3 @@ used to obtain device's power state after the power state transition:

> >  The function returns a non-zero value if it succeeded getting the power count or

> >  runtime PM was disabled, in either of which cases the driver may proceed to

> >  access the device.

> > -

> > -Controls

> > ---------

> > -

> > -For camera sensors that are connected to a bus where transmitter and receiver

> > -require common configuration set by drivers, such as CSI-2 or parallel (BT.601

> > -or BT.656) bus, the ``V4L2_CID_LINK_FREQ`` control is mandatory on transmitter

> > -drivers. Receiver drivers can use the ``V4L2_CID_LINK_FREQ`` to query the

> > -frequency used on the bus.

> > -

> > -The transmitter drivers should also implement ``V4L2_CID_PIXEL_RATE`` control in

> > -order to tell the maximum pixel rate to the receiver. This is required on raw

> > -camera sensors.

> > diff --git a/Documentation/driver-api/media/csi2.rst b/Documentation/driver-api/media/csi2.rst

> > deleted file mode 100644

> > index 11c52b0be8b8..000000000000

> > --- a/Documentation/driver-api/media/csi2.rst

> > +++ /dev/null

> > @@ -1,94 +0,0 @@

> > -.. SPDX-License-Identifier: GPL-2.0

> > -

> > -.. _MIPI_CSI_2:

> > -

> > -MIPI CSI-2

> > -==========

> > -

> > -CSI-2 is a data bus intended for transferring images from cameras to

> > -the host SoC. It is defined by the `MIPI alliance`_.

> > -

> > -.. _`MIPI alliance`: http://www.mipi.org/

> > -

> > -Media bus formats

> > ------------------

> > -

> > -See :ref:`v4l2-mbus-pixelcode` for details on which media bus formats should

> > -be used for CSI-2 interfaces.

> > -

> > -Transmitter drivers

> > --------------------

> > -

> > -CSI-2 transmitter, such as a sensor or a TV tuner, drivers need to

> > -provide the CSI-2 receiver with information on the CSI-2 bus

> > -configuration. These include the V4L2_CID_LINK_FREQ and

> > -V4L2_CID_PIXEL_RATE controls and

> > -(:c:type:`v4l2_subdev_video_ops`->s_stream() callback). These

> > -interface elements must be present on the sub-device represents the

> > -CSI-2 transmitter.

> > -

> > -The V4L2_CID_LINK_FREQ control is used to tell the receiver driver the

> > -frequency (and not the symbol rate) of the link. The V4L2_CID_PIXEL_RATE

> > -control may be used by the receiver to obtain the pixel rate the transmitter

> > -uses. The :c:type:`v4l2_subdev_video_ops`->s_stream() callback provides an

> > -ability to start and stop the stream.

> > -

> > -The value of the V4L2_CID_PIXEL_RATE is calculated as follows::

> > -

> > -	pixel_rate = link_freq * 2 * nr_of_lanes * 16 / k / bits_per_sample

> > -

> > -where

> > -

> > -.. list-table:: variables in pixel rate calculation

> > -   :header-rows: 1

> > -

> > -   * - variable or constant

> > -     - description

> > -   * - link_freq

> > -     - The value of the V4L2_CID_LINK_FREQ integer64 menu item.

> > -   * - nr_of_lanes

> > -     - Number of data lanes used on the CSI-2 link. This can

> > -       be obtained from the OF endpoint configuration.

> > -   * - 2

> > -     - Two bits are transferred per clock cycle per lane.

> > -   * - bits_per_sample

> > -     - Number of bits per sample.

> > -   * - k

> > -     - 16 for D-PHY and 7 for C-PHY

> > -

> > -The transmitter drivers must, if possible, configure the CSI-2

> > -transmitter to *LP-11 mode* whenever the transmitter is powered on but

> > -not active, and maintain *LP-11 mode* until stream on. Only at stream

> > -on should the transmitter activate the clock on the clock lane and

> > -transition to *HS mode*.

> > -

> > -Some transmitters do this automatically but some have to be explicitly

> > -programmed to do so, and some are unable to do so altogether due to

> > -hardware constraints.

> > -

> > -Stopping the transmitter

> > -^^^^^^^^^^^^^^^^^^^^^^^^

> > -

> > -A transmitter stops sending the stream of images as a result of

> > -calling the ``.s_stream()`` callback. Some transmitters may stop the

> > -stream at a frame boundary whereas others stop immediately,

> > -effectively leaving the current frame unfinished. The receiver driver

> > -should not make assumptions either way, but function properly in both

> > -cases.

> > -

> > -Receiver drivers

> > -----------------

> > -

> > -Before the receiver driver may enable the CSI-2 transmitter by using

> > -the :c:type:`v4l2_subdev_video_ops`->s_stream(), it must have powered

> > -the transmitter up by using the

> > -:c:type:`v4l2_subdev_core_ops`->s_power() callback. This may take

> > -place either indirectly by using :c:func:`v4l2_pipeline_pm_get` or

> > -directly.

> > -

> > -Formats

> > --------

> > -

> > -The media bus pixel codes document parallel formats. Should the pixel data be

> > -transported over a serial bus, the media bus pixel code that describes a

> > -parallel format that transfers a sample on a single clock cycle is used.

> > diff --git a/Documentation/driver-api/media/index.rst b/Documentation/driver-api/media/index.rst

> > index 813d7db59da7..08e206567408 100644

> > --- a/Documentation/driver-api/media/index.rst

> > +++ b/Documentation/driver-api/media/index.rst

> > @@ -37,7 +37,7 @@ Documentation/userspace-api/media/index.rst

> >      rc-core

> >      mc-core

> >      cec-core

> > -    csi2

> > +    tx-rx

> >      camera-sensor

> >  

> >      drivers/index

> > diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst

> > new file mode 100644

> > index 000000000000..4c8584e7b6f2

> > --- /dev/null

> > +++ b/Documentation/driver-api/media/tx-rx.rst

> > @@ -0,0 +1,117 @@

> > +.. SPDX-License-Identifier: GPL-2.0

> > +

> > +.. _transmitter-receiver:

> > +

> > +Pixel data transmitter and receiver drivers

> > +===========================================

> > +

> > +V4L2 supports various devices that transmit and receiver pixel data. Examples of

> > +these devices include a camera sensor, a TV tuner and a parallel or a CSI-2

> > +receiver in an SoC.

> > +

> > +Bus types

> > +---------

> > +

> > +The following busses are the most common. This section discusses these two only.

> > +

> > +MIPI CSI-2

> > +^^^^^^^^^^

> > +

> > +CSI-2 is a data bus intended for transferring images from cameras to

> > +the host SoC. It is defined by the `MIPI alliance`_.

> > +

> > +.. _`MIPI alliance`: https://www.mipi.org/

> > +

> > +Parallel

> > +^^^^^^^^

> > +

> > +`BT.601`_ and `BT.656`_ are the most common parallel busses.

> > +

> > +.. _`BT.601`: https://en.wikipedia.org/wiki/Rec._601

> > +.. _`BT.656`: https://en.wikipedia.org/wiki/ITU-R_BT.656

> > +

> > +Transmitter drivers

> > +-------------------

> > +

> > +Transmitter drivers generally need to provide the receiver drivers with the

> > +configuration of the transmitter. What is required depends on the type of the

> > +bus. These are common for both busses.

> > +

> > +Media bus pixel code

> > +^^^^^^^^^^^^^^^^^^^^

> > +

> > +See :ref:`v4l2-mbus-pixelcode`.

> > +

> > +Link frequency

> > +^^^^^^^^^^^^^^

> > +

> > +The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the

> > +receiver the frequency of the bus (i.e. it is not the same as the symbol rate).

> > +

> 

> Would the symbol rate be the same as the pixel rate, or is the 'bit'

> rate? ( I believe it's the bit rate, but I wonder if it needs to be

> defined here to make it clear?)


Neither. :-) Actually we could probably just drop this note as the symbol
rate isn't what the user space works with in general anyway.

> 

> I guess this is the distinction that the bus may send two bits per clock

> cycle or such.

> 

> 

> > +``.s_stream()`` callback

> > +^^^^^^^^^^^^^^^^^^^^^^^^

> > +

> > +The struct struct v4l2_subdev_video_ops->s_stream() callback is used by the

> > +receiver driver to control the transmitter driver's streaming state.

> > +

> > +

> > +CSI-2 transmitter drivers

> > +-------------------------

> > +

> > +Pixel rate

> > +^^^^^^^^^^

> > +

> > +The pixel rate on the bus is calculated as follows::

> > +

> > +	pixel_rate = link_freq * 2 * nr_of_lanes * 16 / k / bits_per_sample

> > +

> > +where

> > +

> > +.. list-table:: variables in pixel rate calculation

> > +   :header-rows: 1

> > +

> > +   * - variable or constant

> > +     - description

> > +   * - link_freq

> > +     - The value of the ``V4L2_CID_LINK_FREQ`` integer64 menu item.

> > +   * - nr_of_lanes

> > +     - Number of data lanes used on the CSI-2 link. This can

> > +       be obtained from the OF endpoint configuration.

> > +   * - 2

> > +     - Data is transferred on both rising and falling edge of the signal.

> > +   * - bits_per_sample

> > +     - Number of bits per sample.

> > +   * - k

> > +     - 16 for D-PHY and 7 for C-PHY

> 

> Is 'k' a defined term here? It makes me assume kilo ... which clearly

> isn't its usage with values of 16 and 7?


It's a name of a constant used in relevant MIPI specs. So I thought using
the same should be fine.

> 

> 

> Fairly optional comments though so :

> 

> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


Thank you. I already sent a pull request so if something goes wrong I'll
put this there, too.

-- 
Kind regards,

Sakari Ailus
Kieran Bingham July 27, 2021, 12:19 p.m. UTC | #3
Hi Sakari,

On 24/06/2021 09:40, Sakari Ailus wrote:
> Fix kerneldoc syntax in v4l2-async. The references were not produced

> correctly.

> 

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

> ---

>  Documentation/driver-api/media/tx-rx.rst      |  8 ++---

>  .../media/v4l/ext-ctrls-image-process.rst     |  2 ++

>  include/media/v4l2-async.h                    | 30 +++++++++----------

>  3 files changed, 21 insertions(+), 19 deletions(-)

> 

> diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst

> index 4c8584e7b6f2..12d492d25df2 100644

> --- a/Documentation/driver-api/media/tx-rx.rst

> +++ b/Documentation/driver-api/media/tx-rx.rst

> @@ -5,7 +5,7 @@

>  Pixel data transmitter and receiver drivers

>  ===========================================

>  

> -V4L2 supports various devices that transmit and receiver pixel data. Examples of

> +V4L2 supports various devices that transmit and receive pixel data. Examples of

>  these devices include a camera sensor, a TV tuner and a parallel or a CSI-2

>  receiver in an SoC.

>  

> @@ -95,9 +95,9 @@ LP-11 and LP-111 modes

>  

>  The transmitter drivers must, if possible, configure the CSI-2 transmitter to

>  *LP-11 or LP-111 mode* whenever the transmitter is powered on but not active,

> -and maintain *LP-11 or LP-111 mode* until stream on. Only at stream on should

> -the transmitter activate the clock on the clock lane and transition to *HS

> -mode*.

> +and maintain *LP-11 or LP-111 mode* until stream on. Only at stream on time

> +should the transmitter activate the clock on the clock lane and transition to

> +*HS mode*.

>  

>  Some transmitters do this automatically but some have to be explicitly

>  programmed to do so, and some are unable to do so altogether due to



Should those changes have been in the previous patch ?
(The changes above look OK though).


> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst

> index 37dad2f4df8c..ed65fb594cc8 100644

> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst

> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst

> @@ -37,6 +37,8 @@ Image Process Control IDs

>      by selecting the desired horizontal and vertical blanking. The unit

>      of this control is Hz.

>  

> +.. _v4l2-cid-pixel-rate:

> +

>  ``V4L2_CID_PIXEL_RATE (64-bit integer)``

>      Pixel rate in the source pads of the subdev. This control is

>      read-only and its unit is pixels / second.

> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h

> index 5b275a845c20..fa4901162663 100644

> --- a/include/media/v4l2-async.h

> +++ b/include/media/v4l2-async.h

> @@ -129,11 +129,11 @@ void v4l2_async_debug_init(struct dentry *debugfs_dir);

>   *

>   * This function initializes the notifier @asd_list. It must be called

>   * before adding a subdevice to a notifier, using one of:

> - * @v4l2_async_notifier_add_fwnode_remote_subdev,

> - * @v4l2_async_notifier_add_fwnode_subdev,

> - * @v4l2_async_notifier_add_i2c_subdev,

> - * @__v4l2_async_notifier_add_subdev or

> - * @v4l2_async_notifier_parse_fwnode_endpoints.

> + * v4l2_async_notifier_add_fwnode_remote_subdev(),

> + * v4l2_async_notifier_add_fwnode_subdev(),

> + * v4l2_async_notifier_add_i2c_subdev(),

> + * __v4l2_async_notifier_add_subdev() or

> + * v4l2_async_notifier_parse_fwnode_endpoints().

>   */

>  void v4l2_async_notifier_init(struct v4l2_async_notifier *notifier);

>  

> @@ -145,9 +145,9 @@ void v4l2_async_notifier_init(struct v4l2_async_notifier *notifier);

>   * @asd: pointer to &struct v4l2_async_subdev

>   *

>   * \warning: Drivers should avoid using this function and instead use one of:

> - * @v4l2_async_notifier_add_fwnode_subdev,

> - * @v4l2_async_notifier_add_fwnode_remote_subdev or

> - * @v4l2_async_notifier_add_i2c_subdev.

> + * v4l2_async_notifier_add_fwnode_subdev(),

> + * v4l2_async_notifier_add_fwnode_remote_subdev() or

> + * v4l2_async_notifier_add_i2c_subdev().

>   *

>   * Call this function before registering a notifier to link the provided @asd to

>   * the notifiers master @asd_list. The @asd must be allocated with k*alloc() as

> @@ -200,7 +200,7 @@ __v4l2_async_notifier_add_fwnode_remote_subdev(struct v4l2_async_notifier *notif

>   * function also gets a reference of the fwnode which is released later at

>   * notifier cleanup time.

>   *

> - * This is just like @v4l2_async_notifier_add_fwnode_subdev, but with the

> + * This is just like v4l2_async_notifier_add_fwnode_subdev(), but with the

>   * exception that the fwnode refers to a local endpoint, not the remote one.

>   */

>  #define v4l2_async_notifier_add_fwnode_remote_subdev(notifier, ep, type) \

> @@ -265,13 +265,13 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);

>   * sub-devices allocated for the purposes of the notifier but not the notifier

>   * itself. The user is responsible for calling this function to clean up the

>   * notifier after calling

> - * @v4l2_async_notifier_add_fwnode_remote_subdev,

> - * @v4l2_async_notifier_add_fwnode_subdev,

> - * @v4l2_async_notifier_add_i2c_subdev,

> - * @__v4l2_async_notifier_add_subdev or

> - * @v4l2_async_notifier_parse_fwnode_endpoints.

> + * v4l2_async_notifier_add_fwnode_remote_subdev(),

> + * v4l2_async_notifier_add_fwnode_subdev(),

> + * v4l2_async_notifier_add_i2c_subdev(),

> + * __v4l2_async_notifier_add_subdev() or

> + * v4l2_async_notifier_parse_fwnode_endpoints().

>   *

> - * There is no harm from calling v4l2_async_notifier_cleanup in other

> + * There is no harm from calling v4l2_async_notifier_cleanup() in other

>   * cases as long as its memory has been zeroed after it has been

>   * allocated.

>   */

> 


But these all look fine.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Kieran Bingham July 27, 2021, 12:54 p.m. UTC | #4
On 24/06/2021 09:40, Sakari Ailus wrote:
> The V4L2_CID_PIXEL_RATE is nowadays used to tell pixel sampling rate in

> the sub-device's pixel array, not the pixel rate over a link (for which it

> also becomes unfit with the addition of multiplexed streams later on). Fix

> this.


Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>



> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

> Reviewed-by: Andrey Konovalov <andrey.konovalov@linaro.org>

> ---

>  .../userspace-api/media/v4l/ext-ctrls-image-process.rst         | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst

> index ed65fb594cc8..2b5a13dc843f 100644

> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst

> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst

> @@ -40,7 +40,7 @@ Image Process Control IDs

>  .. _v4l2-cid-pixel-rate:

>  

>  ``V4L2_CID_PIXEL_RATE (64-bit integer)``

> -    Pixel rate in the source pads of the subdev. This control is

> +    Pixel sampling rate in the device's pixel array. This control is

>      read-only and its unit is pixels / second.

>  

>  ``V4L2_CID_TEST_PATTERN (menu)``

>