Hi Andrey and Jacopo,
On Thu, Feb 04, 2021 at 03:39:14PM +0300, Andrey Konovalov wrote:
> Hi Jacopo,
>
> On 04.02.2021 13:31, Jacopo Mondi wrote:
> > Hi Sakari,
> >
> > On Wed, Feb 03, 2021 at 06:50:38PM +0200, Sakari Ailus wrote:
> > > Modernise the documentation to make it more precise and update the use of
> > > pixel rate control. 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.
> > >
> > > - Separate CSI-2 and parallel documentation. CSI-2 already has its own
> > > section.
> > >
> > > - Include all DT properties needed for assigned clocks.
> > >
> > > - 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.
> > >
> > > Fixes: e4cf8c58af75 ("media: Documentation: media: Document how to write camera sensor drivers")
> > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > ---
> > > This replaces an earlier patch here:
> > >
> > > <URL:https://patchwork.linuxtv.org/project/linux-media/patch/20210201093914.12994-1-sakari.ailus@xxxxxxxxxxxxxxx/>
> > >
> > > .../driver-api/media/camera-sensor.rst | 47 +++++++++----------
> > > Documentation/driver-api/media/csi2.rst | 36 +++++++-------
> > > 2 files changed, 43 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > > index 3fc378b3b269..4e2efa6e8fa1 100644
> > > --- a/Documentation/driver-api/media/camera-sensor.rst
> > > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > > @@ -8,6 +8,15 @@ CSI-2
> > >
> > > Please see what is written on :ref:`MIPI_CSI_2`.
> > >
> > > +Parallel (BT.601 and BT.656)
> > > +----------------------------
> > > +
> > > +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.
> > > +
> >
> > Is this really part of the 'Parallel.." subsection ? It describes
> > something that applies to both busses. Actually, do parallel receivers
> > need to know the pixel clock lane frequency ? Actually yes, I see
> > omap3isp/ispvideo.c using the control to validate the frequency
> > against some platform constraints.
> >
> > So yes, this applies to both parallel and CSI-2, so I would not place
> > it in the "Parallel" section
I'll see if these could be unified in a nice way.
> >
> > > Handling clocks
> > > ---------------
> > >
> > > @@ -26,15 +35,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 +65,8 @@ 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``).
> >
> > Do you need to break the line ?
Indeed not anymore. :-)
> >
> > >
> > > Register list based drivers
> > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > @@ -67,7 +78,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 +105,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.
> >
> > Awesome, having the VBLANK cotrol unit specified is good. Do we need to say
> > the unit is 'frame lines, including blankings' or is it implied ?
For this class of hardware it is implied but it wouldn't hurt to document
that here.
> >
> > Also note that in V4L2 the EXPOSURE control has currently no unit
> > specified as well and drivers implement it differently :(
Yes, well. Also some (at least some old OV sensors) sensors have different
units on e.g. exposure. So perhaps the above should be changed to "should"
form? That's of course annoying for the user space.
> >
> > >
> > > 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
> > > @@ -132,7 +144,7 @@ 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
> > > @@ -150,16 +162,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
> > > index 11c52b0be8b8..c79df33bdeaa 100644
> > > --- a/Documentation/driver-api/media/csi2.rst
> > > +++ b/Documentation/driver-api/media/csi2.rst
> > > @@ -19,21 +19,18 @@ 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::
> > > +CSI-2 transmitter, such as a sensor or a TV tuner, drivers need to provide the
> >
> > CSI-2 transmitters ?
Yes.
> >
> > > +CSI-2 receiver with information on the CSI-2 bus configuration. These include
> > > +the ``V4L2_CID_LINK_FREQ`` control and
> > > +(:c:type:`v4l2_subdev_video_ops`->s_stream() callback). These interface elements
> > > +must be present on the sub-device representing 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
> > > +:c:type:`v4l2_subdev_video_ops`->s_stream() callback provides an ability to
> > > +start and stop the stream.
> > > +
> > > +The pixel rate on the bus is calculated as follows::
> > >
> > > pixel_rate = link_freq * 2 * nr_of_lanes * 16 / k / bits_per_sample
> >
> > What's 'k' and why '* 16' ?
>
> This is explained below (see at --->).
>
> > Isn't the bus pixel rate simply
> >
> > link_freq * 2 * nr_of_lanes / bits_per_sample ?
>
> This is correct for D-PHY only (k == 16). For C-PHY different value of k
> must be used.
>
> > >
> > > @@ -45,7 +42,7 @@ 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.
> > > @@ -56,6 +53,13 @@ where
>
> The 'k' definition is here
> --->
> > > * - k
> > > - 16 for D-PHY and 7 for C-PHY
> > > +.. note::
> > > +
> > > + The pixel rate calculated this way is **not** the same as the pixel rate
> > > + on the camera sensor's pixel array, and should not be used as the value
> > > + of the control (unless the value also matches the rate on the pixel
> > > + array).
> >
> > I would say
> >
> > The pixel rate calculated this way is **not** the same as the
> > pixel sampling rate on the sensor's pixel matrix, but only
> > represents the pixel transmission rate on the bus.
>
> This sounds OK for me. Just maybe the "is **not** the same" statement
> is a bit too strong. I would say "may be **not** the same", as for most
> of the current camera sensor drivers these two rates have the same value
> (though a modification to a driver can change that any time).
Seems good to me. I'll update this for v2.
>
> > Followed by a description of what PIXEL_RATE represents and how it
> > should be used, to make it clear the two are different.
> >
> > The pixel matrix sampling rate is instead used to calculate
> > the sensor timings, in example to transform an image exposure
> > duration from unit of lines in wall-clock time.
> >
> > Please be aware that the controls documentation reports:
> >
> > ``V4L2_CID_LINK_FREQ (integer menu)``
> > Data bus frequency.
>
> Yeah... This:
>
> > Together with the media bus pixel code, bus type
> > (clock cycles per sample), the data bus frequency defines the pixel
> > rate (``V4L2_CID_PIXEL_RATE``) in the pixel array (or possibly
> > elsewhere, if the device is not an image sensor).
>
> - isn't (always) true...
>
> > The frame rate can
> > be calculated from the pixel clock, image width and height and
> > horizontal and vertical blanking. While the pixel rate control may
> > be defined elsewhere than in the subdev containing the pixel array,
> > the frame rate cannot be obtained from that information. This is
> > because only on the pixel array it can be assumed that the vertical
> > and horizontal blanking information is exact: no other blanking is
> > allowed in the pixel array. The selection of frame rate is performed
> > by selecting the desired horizontal and vertical blanking. The unit
> > of this control is Hz.
> >
> > ``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.
> >
> > I think these needs to be reworked to make it clear PIXEL_RATE !=
> > pixel sampling rate.
>
> I second that.
Agreed. I'll come up with something for these in v2.
>
> > Would you like to do so if you agree, or should I send a patch on top
> > of this one ?
> >
> > > +
> >
> > Thanks, much apreciated effort.
>
> +1
Thanks!
--
Regards,
Sakari Ailus
