| Message ID | 20210622112200.13914-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | V4L2 driver documentation, v4l2-async improvements | expand |
Hi Sakari, thanks for addressing comments on v1 just a few minros, the rest looks very good! On Tue, Jun 22, 2021 at 02:21:55PM +0300, 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> > --- > .../driver-api/media/camera-sensor.rst | 45 +++---- > Documentation/driver-api/media/index.rst | 2 +- > .../driver-api/media/{csi2.rst => tx-rx.rst} | 114 +++++++++++------- > .../media/v4l/ext-ctrls-image-process.rst | 2 + > 4 files changed, 90 insertions(+), 73 deletions(-) > rename Documentation/driver-api/media/{csi2.rst => tx-rx.rst} (39%) > > 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 > +--------------------------------------------- > > -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/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/csi2.rst b/Documentation/driver-api/media/tx-rx.rst > similarity index 39% > rename from Documentation/driver-api/media/csi2.rst > rename to Documentation/driver-api/media/tx-rx.rst > index 11c52b0be8b8..6331f93fb249 100644 > --- a/Documentation/driver-api/media/csi2.rst > +++ b/Documentation/driver-api/media/tx-rx.rst > @@ -1,39 +1,71 @@ > .. SPDX-License-Identifier: GPL-2.0 > > -.. _MIPI_CSI_2: > +.. _transmitter-receiver: > + > +Pixel data transmitter and receiver drivers > +=========================================== > + > +V4L2 supports various devices that transmit and receiver pixel data. Examples of s/and receiver/and receives/ > +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`: http://www.mipi.org/ > +.. _`MIPI alliance`: https://www.mipi.org/ > > -Media bus formats > ------------------ > +Parallel > +^^^^^^^^ > > -See :ref:`v4l2-mbus-pixelcode` for details on which media bus formats should > -be used for CSI-2 interfaces. > +`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 > ------------------- > > -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. > +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 > +^^^^^^^^^^^^^^^^^^^^ > + > +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. I guess you intend to document that, in example, MEDIA_BUS_FMT_YUYV8_2X8 has to be used on parallel busses, as the bus width is meaningful, while MEDIA_BUS_FMT_YUYV8_1X16 should be used on serial busses as the bus width it's not. I guess this could be reported as an example ? > + > +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). > + > +``.s_stream()`` callback > +^^^^^^^^^^^^^^^^^^^^^^^^ > > -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 struct struct v4l2_subdev_video_ops->s_stream() callback is used by the > +receiver driver to control the transmitter driver's streaming state. > > -The value of the V4L2_CID_PIXEL_RATE is calculated as follows:: > + > +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 > > @@ -45,27 +77,38 @@ where > * - variable or constant > - description > * - link_freq > - - The value of the V4L2_CID_LINK_FREQ integer64 menu item. > + - 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. > + - 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 > > -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*. > +.. note:: > + > + The pixel rate calculated this way is **not** be the same thing than > + the pixel rate on the camera sensor's pixel array. .. as reported by the V4L2_CID_PIXEL_RATE control. > + > +LP-11 and LP-111 modes > +^^^^^^^^^^^^^^^^^^^^^^ > + > +The transmitter drivers must, if possible, configure the CSI-2 transmitter to s/drivers/driver/ Or if you want to keep it plural drop "The" ? > +*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 "Only at stream on time the transmitter should" Sounds better to me, but not being a native speaker I might very well be wrong :) All minors, thanks for clearing this up Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > +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 > ^^^^^^^^^^^^^^^^^^^^^^^^ > > @@ -75,20 +118,3 @@ 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/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 > -- > 2.30.2 >
Hi Sakari, On Tue, Jun 22, 2021 at 02:21:58PM +0300, Sakari Ailus wrote: > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > There are only one user left of __v4l2_async_nf_parse_fwnode_ep() > since [1], v4l2_async_nf_parse_fwnode_endpoints(). The two > functions can be merged. > > The merge of the two highlights a dead code block conditioned by the > argument 'has_port' that always is false and can therefor be removed. > > 1. commit 0ae426ebd0dcef81 ("media: v4l2-fwnode: Remove v4l2_async_notifier_parse_fwnode_endpoints_by_port()") > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > [Sakari Ailus: Aligned some lines to opening parentheses.] > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> I guess my tag went lost Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > drivers/media/v4l2-core/v4l2-fwnode.c | 31 +++++---------------------- > 1 file changed, 5 insertions(+), 26 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c > index e5507501b0f3..00457e1e93f6 100644 > --- a/drivers/media/v4l2-core/v4l2-fwnode.c > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > @@ -839,12 +839,11 @@ v4l2_async_nf_fwnode_parse_endpoint(struct device *dev, > return ret == -ENOTCONN ? 0 : ret; > } > > -static int > -__v4l2_async_nf_parse_fwnode_ep(struct device *dev, > - struct v4l2_async_notifier *notifier, > - size_t asd_struct_size, unsigned int port, > - bool has_port, > - parse_endpoint_func parse_endpoint) > +int > +v4l2_async_nf_parse_fwnode_endpoints(struct device *dev, > + struct v4l2_async_notifier *notifier, > + size_t asd_struct_size, > + parse_endpoint_func parse_endpoint) > { > struct fwnode_handle *fwnode; > int ret = 0; > @@ -862,16 +861,6 @@ __v4l2_async_nf_parse_fwnode_ep(struct device *dev, > if (!is_available) > continue; > > - if (has_port) { > - struct fwnode_endpoint ep; > - > - ret = fwnode_graph_parse_endpoint(fwnode, &ep); > - if (ret) > - break; > - > - if (ep.port != port) > - continue; > - } > > ret = v4l2_async_nf_fwnode_parse_endpoint(dev, notifier, > fwnode, > @@ -885,16 +874,6 @@ __v4l2_async_nf_parse_fwnode_ep(struct device *dev, > > return ret; > } > - > -int > -v4l2_async_nf_parse_fwnode_endpoints(struct device *dev, > - struct v4l2_async_notifier *notifier, > - size_t asd_struct_size, > - parse_endpoint_func parse_endpoint) > -{ > - return __v4l2_async_nf_parse_fwnode_ep(dev, notifier, asd_struct_size, > - 0, false, parse_endpoint); > -} > EXPORT_SYMBOL_GPL(v4l2_async_nf_parse_fwnode_endpoints); > > /* > -- > 2.30.2 >
Hi Sakari, On Tue, Jun 22, 2021 at 02:22:00PM +0300, 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. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > .../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 37dad2f4df8c..6d681af95624 100644 > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > @@ -38,7 +38,7 @@ Image Process Control IDs > of this control is Hz. > > ``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)`` > -- > 2.30.2 >
Hi Jacopo, Thanks for the review. On Wed, Jun 23, 2021 at 05:11:01PM +0200, Jacopo Mondi wrote: > Hi Sakari, > thanks for addressing comments on v1 > > just a few minros, the rest looks very good! > > On Tue, Jun 22, 2021 at 02:21:55PM +0300, 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> > > --- > > .../driver-api/media/camera-sensor.rst | 45 +++---- > > Documentation/driver-api/media/index.rst | 2 +- > > .../driver-api/media/{csi2.rst => tx-rx.rst} | 114 +++++++++++------- > > .../media/v4l/ext-ctrls-image-process.rst | 2 + > > 4 files changed, 90 insertions(+), 73 deletions(-) > > rename Documentation/driver-api/media/{csi2.rst => tx-rx.rst} (39%) > > > > 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 > > +--------------------------------------------- > > > > -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/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/csi2.rst b/Documentation/driver-api/media/tx-rx.rst > > similarity index 39% > > rename from Documentation/driver-api/media/csi2.rst > > rename to Documentation/driver-api/media/tx-rx.rst > > index 11c52b0be8b8..6331f93fb249 100644 > > --- a/Documentation/driver-api/media/csi2.rst > > +++ b/Documentation/driver-api/media/tx-rx.rst > > @@ -1,39 +1,71 @@ > > .. SPDX-License-Identifier: GPL-2.0 > > > > -.. _MIPI_CSI_2: > > +.. _transmitter-receiver: > > + > > +Pixel data transmitter and receiver drivers > > +=========================================== > > + > > +V4L2 supports various devices that transmit and receiver pixel data. Examples of > > s/and receiver/and receives/ "receive"; it's plural. :-) > > > +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`: http://www.mipi.org/ > > +.. _`MIPI alliance`: https://www.mipi.org/ > > > > -Media bus formats > > ------------------ > > +Parallel > > +^^^^^^^^ > > > > -See :ref:`v4l2-mbus-pixelcode` for details on which media bus formats should > > -be used for CSI-2 interfaces. > > +`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 > > ------------------- > > > > -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. > > +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 > > +^^^^^^^^^^^^^^^^^^^^ > > + > > +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. > > I guess you intend to document that, in example, > MEDIA_BUS_FMT_YUYV8_2X8 has to be used on parallel busses, as the bus > width is meaningful, while MEDIA_BUS_FMT_YUYV8_1X16 should be used on > serial busses as the bus width it's not. I guess this could be > reported as an example ? We actually have such an example in media bus pixel code documentation. How about removing the paragraph and leaving just the reference below? > > > + > > +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). > > + > > +``.s_stream()`` callback > > +^^^^^^^^^^^^^^^^^^^^^^^^ > > > > -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 struct struct v4l2_subdev_video_ops->s_stream() callback is used by the > > +receiver driver to control the transmitter driver's streaming state. > > > > -The value of the V4L2_CID_PIXEL_RATE is calculated as follows:: > > + > > +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 > > > > @@ -45,27 +77,38 @@ where > > * - variable or constant > > - description > > * - link_freq > > - - The value of the V4L2_CID_LINK_FREQ integer64 menu item. > > + - 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. > > + - 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 > > > > -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*. > > +.. note:: > > + > > + The pixel rate calculated this way is **not** be the same thing than > > + the pixel rate on the camera sensor's pixel array. > > .. as reported by the V4L2_CID_PIXEL_RATE control. Good point. I'll use "... which is reported by ...", to > > > + > > +LP-11 and LP-111 modes > > +^^^^^^^^^^^^^^^^^^^^^^ > > + > > +The transmitter drivers must, if possible, configure the CSI-2 transmitter to > > s/drivers/driver/ > Or if you want to keep it plural drop "The" ? It could be singular, and indefinite. I think this is better since it obviously applies to all such drivers. > > > +*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 > > "Only at stream on time the transmitter should" > > Sounds better to me, but not being a native speaker I might very well > be wrong :) Agreed, I added "time". > > All minors, thanks for clearing this up > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thank you! -- Kind regards, Sakari Ailus
Hi Sakari, Thank you for this work. And thank you Jacopo for the review. Looks fine overall; just few minor things. On 23.06.2021 18:50, Sakari Ailus wrote: > Hi Jacopo, > > Thanks for the review. > > On Wed, Jun 23, 2021 at 05:11:01PM +0200, Jacopo Mondi wrote: >> Hi Sakari, >> thanks for addressing comments on v1 >> >> just a few minros, the rest looks very good! >> >> On Tue, Jun 22, 2021 at 02:21:55PM +0300, 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> >>> --- >>> .../driver-api/media/camera-sensor.rst | 45 +++---- >>> Documentation/driver-api/media/index.rst | 2 +- >>> .../driver-api/media/{csi2.rst => tx-rx.rst} | 114 +++++++++++------- >>> .../media/v4l/ext-ctrls-image-process.rst | 2 + >>> 4 files changed, 90 insertions(+), 73 deletions(-) >>> rename Documentation/driver-api/media/{csi2.rst => tx-rx.rst} (39%) >>> >>> 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 >>> +--------------------------------------------- >>> >>> -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/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/csi2.rst b/Documentation/driver-api/media/tx-rx.rst >>> similarity index 39% >>> rename from Documentation/driver-api/media/csi2.rst >>> rename to Documentation/driver-api/media/tx-rx.rst >>> index 11c52b0be8b8..6331f93fb249 100644 >>> --- a/Documentation/driver-api/media/csi2.rst >>> +++ b/Documentation/driver-api/media/tx-rx.rst >>> @@ -1,39 +1,71 @@ >>> .. SPDX-License-Identifier: GPL-2.0 >>> >>> -.. _MIPI_CSI_2: >>> +.. _transmitter-receiver: >>> + >>> +Pixel data transmitter and receiver drivers >>> +=========================================== >>> + >>> +V4L2 supports various devices that transmit and receiver pixel data. Examples of >> >> s/and receiver/and receives/ > > "receive"; it's plural. :-) Correct >> >>> +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`: http://www.mipi.org/ >>> +.. _`MIPI alliance`: https://www.mipi.org/ >>> >>> -Media bus formats >>> ------------------ >>> +Parallel >>> +^^^^^^^^ >>> >>> -See :ref:`v4l2-mbus-pixelcode` for details on which media bus formats should >>> -be used for CSI-2 interfaces. >>> +`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 >>> ------------------- >>> >>> -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. >>> +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 >>> +^^^^^^^^^^^^^^^^^^^^ >>> + >>> +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. >> >> I guess you intend to document that, in example, >> MEDIA_BUS_FMT_YUYV8_2X8 has to be used on parallel busses, as the bus >> width is meaningful, while MEDIA_BUS_FMT_YUYV8_1X16 should be used on >> serial busses as the bus width it's not. I guess this could be >> reported as an example ? Looks like a good idea for me. > We actually have such an example in media bus pixel code documentation. > > How about removing the paragraph and leaving just the reference below? The reference would work for me. >> >>> + >>> +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). >>> + >>> +``.s_stream()`` callback >>> +^^^^^^^^^^^^^^^^^^^^^^^^ >>> >>> -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 struct struct v4l2_subdev_video_ops->s_stream() callback is used by the >>> +receiver driver to control the transmitter driver's streaming state. >>> >>> -The value of the V4L2_CID_PIXEL_RATE is calculated as follows:: >>> + >>> +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 >>> >>> @@ -45,27 +77,38 @@ where >>> * - variable or constant >>> - description >>> * - link_freq >>> - - The value of the V4L2_CID_LINK_FREQ integer64 menu item. >>> + - 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. >>> + - 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 >>> >>> -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*. >>> +.. note:: >>> + >>> + The pixel rate calculated this way is **not** be the same thing than s/is **not** be the same thing/is **not** the same thing/ Also maybe s/than/as ("not the same thing as"), but this is up to you. >>> + the pixel rate on the camera sensor's pixel array. >> >> .. as reported by the V4L2_CID_PIXEL_RATE control. > > Good point. I'll use "... which is reported by ...", to > >> >>> + >>> +LP-11 and LP-111 modes >>> +^^^^^^^^^^^^^^^^^^^^^^ >>> + >>> +The transmitter drivers must, if possible, configure the CSI-2 transmitter to >> >> s/drivers/driver/ >> Or if you want to keep it plural drop "The" ? > > It could be singular, and indefinite. I think this is better since it > obviously applies to all such drivers. I am for the original "The transmitter drivers" or even "All the transmitter drivers" >> >>> +*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 >> >> "Only at stream on time the transmitter should" >> >> Sounds better to me, but not being a native speaker I might very well >> be wrong :) > > Agreed, I added "time". > >> >> All minors, thanks for clearing this up >> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Andrey Konovalov <andrey.konovalov@linaro.org> Thanks, Andrey > Thank you! >
Hi Sakari, Thank you for your patch! On 22.06.2021 14:22, 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. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Reviewed-by: Andrey Konovalov <andrey.konovalov@linaro.org> Thanks, Andrey > --- > .../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 37dad2f4df8c..6d681af95624 100644 > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > @@ -38,7 +38,7 @@ Image Process Control IDs > of this control is Hz. > > ``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)`` >
Hi Andrey, Thanks for the review. On Thu, Jun 24, 2021 at 07:50:46AM +0300, Andrey Konovalov wrote: > Hi Sakari, > > Thank you for this work. > And thank you Jacopo for the review. > > Looks fine overall; just few minor things. > > On 23.06.2021 18:50, Sakari Ailus wrote: > > Hi Jacopo, > > > > Thanks for the review. > > > > On Wed, Jun 23, 2021 at 05:11:01PM +0200, Jacopo Mondi wrote: > > > Hi Sakari, > > > thanks for addressing comments on v1 > > > > > > just a few minros, the rest looks very good! > > > > > > On Tue, Jun 22, 2021 at 02:21:55PM +0300, 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> > > > > --- > > > > .../driver-api/media/camera-sensor.rst | 45 +++---- > > > > Documentation/driver-api/media/index.rst | 2 +- > > > > .../driver-api/media/{csi2.rst => tx-rx.rst} | 114 +++++++++++------- > > > > .../media/v4l/ext-ctrls-image-process.rst | 2 + > > > > 4 files changed, 90 insertions(+), 73 deletions(-) > > > > rename Documentation/driver-api/media/{csi2.rst => tx-rx.rst} (39%) > > > > > > > > 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 > > > > +--------------------------------------------- > > > > > > > > -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/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/csi2.rst b/Documentation/driver-api/media/tx-rx.rst > > > > similarity index 39% > > > > rename from Documentation/driver-api/media/csi2.rst > > > > rename to Documentation/driver-api/media/tx-rx.rst > > > > index 11c52b0be8b8..6331f93fb249 100644 > > > > --- a/Documentation/driver-api/media/csi2.rst > > > > +++ b/Documentation/driver-api/media/tx-rx.rst > > > > @@ -1,39 +1,71 @@ > > > > .. SPDX-License-Identifier: GPL-2.0 > > > > > > > > -.. _MIPI_CSI_2: > > > > +.. _transmitter-receiver: > > > > + > > > > +Pixel data transmitter and receiver drivers > > > > +=========================================== > > > > + > > > > +V4L2 supports various devices that transmit and receiver pixel data. Examples of > > > > > > s/and receiver/and receives/ > > > > "receive"; it's plural. :-) > > Correct > > > > > > > > +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`: http://www.mipi.org/ > > > > +.. _`MIPI alliance`: https://www.mipi.org/ > > > > > > > > -Media bus formats > > > > ------------------ > > > > +Parallel > > > > +^^^^^^^^ > > > > > > > > -See :ref:`v4l2-mbus-pixelcode` for details on which media bus formats should > > > > -be used for CSI-2 interfaces. > > > > +`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 > > > > ------------------- > > > > > > > > -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. > > > > +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 > > > > +^^^^^^^^^^^^^^^^^^^^ > > > > + > > > > +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. > > > > > > I guess you intend to document that, in example, > > > MEDIA_BUS_FMT_YUYV8_2X8 has to be used on parallel busses, as the bus > > > width is meaningful, while MEDIA_BUS_FMT_YUYV8_1X16 should be used on > > > serial busses as the bus width it's not. I guess this could be > > > reported as an example ? > > Looks like a good idea for me. > > > We actually have such an example in media bus pixel code documentation. > > > > How about removing the paragraph and leaving just the reference below? > > The reference would work for me. Left the reference only. > > > > > > > > + > > > > +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). > > > > + > > > > +``.s_stream()`` callback > > > > +^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > > > > -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 struct struct v4l2_subdev_video_ops->s_stream() callback is used by the > > > > +receiver driver to control the transmitter driver's streaming state. > > > > > > > > -The value of the V4L2_CID_PIXEL_RATE is calculated as follows:: > > > > + > > > > +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 > > > > > > > > @@ -45,27 +77,38 @@ where > > > > * - variable or constant > > > > - description > > > > * - link_freq > > > > - - The value of the V4L2_CID_LINK_FREQ integer64 menu item. > > > > + - 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. > > > > + - 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 > > > > > > > > -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*. > > > > +.. note:: > > > > + > > > > + The pixel rate calculated this way is **not** be the same thing than > > s/is **not** be the same thing/is **not** the same thing/ Oops! > > Also maybe s/than/as ("not the same thing as"), but this is up to you. This is correct indeed. Fixed both. > > > > > + the pixel rate on the camera sensor's pixel array. > > > > > > .. as reported by the V4L2_CID_PIXEL_RATE control. > > > > Good point. I'll use "... which is reported by ...", to > > > > > > > > > + > > > > +LP-11 and LP-111 modes > > > > +^^^^^^^^^^^^^^^^^^^^^^ > > > > + > > > > +The transmitter drivers must, if possible, configure the CSI-2 transmitter to > > > > > > s/drivers/driver/ > > > Or if you want to keep it plural drop "The" ? > > > > It could be singular, and indefinite. I think this is better since it > > obviously applies to all such drivers. > > I am for the original "The transmitter drivers" or even "All the transmitter drivers" I kept the original. > > > > > > > > +*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 > > > > > > "Only at stream on time the transmitter should" > > > > > > Sounds better to me, but not being a native speaker I might very well > > > be wrong :) > > > > Agreed, I added "time". > > > > > > > > All minors, thanks for clearing this up > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Reviewed-by: Andrey Konovalov <andrey.konovalov@linaro.org> Thanks! I'll send v3 soon. There was also a typo in the third patch (why wait for compile testing?) that I'll also fix with that. -- Kind regards, Sakari Ailus
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. 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/index.rst | 2 +- .../driver-api/media/{csi2.rst => tx-rx.rst} | 114 +++++++----- .../driver-api/media/v4l2-subdev.rst | 14 +- .../media/v4l/ext-ctrls-image-process.rst | 4 +- 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 | 8 +- 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 +- 45 files changed, 554 insertions(+), 586 deletions(-) rename Documentation/driver-api/media/{csi2.rst => tx-rx.rst} (39%)