Re: [PATCH v7 02/16] doc-rst: Add Intel IPU3 documentation

2018-11-29 Thread Laurent Pinchart
Hello Yong,

Thank you for the patch.

On Tuesday, 30 October 2018 00:22:56 EET Yong Zhi wrote:
> From: Rajmohan Mani 
> 
> This patch adds the details about the IPU3 Imaging Unit driver.

Strictly speaking this documents both the CIO2 and the IMGU. As they're 
handled by two separate drivers, should they be split in two separate files ? 
If you prefer keeping them together you should update the commit message 
accordingly. I would in that case also split the documentation in a CIO2 and a 
IMGU section in the file, instead of mixing them.

> Change-Id: I560cecf673df2dcc3ec72767cf8077708d649656

The Change-Id: tag isn't suitable for mainline, you can drop it.

> Signed-off-by: Rajmohan Mani 
> ---
>  Documentation/media/v4l-drivers/index.rst |   1 +
>  Documentation/media/v4l-drivers/ipu3.rst  | 326 +++
>  2 files changed, 327 insertions(+)
>  create mode 100644 Documentation/media/v4l-drivers/ipu3.rst
> 
> diff --git a/Documentation/media/v4l-drivers/index.rst
> b/Documentation/media/v4l-drivers/index.rst index 679238e..179a393 100644
> --- a/Documentation/media/v4l-drivers/index.rst
> +++ b/Documentation/media/v4l-drivers/index.rst
> @@ -44,6 +44,7 @@ For more details see the file COPYING in the source
> distribution of Linux. davinci-vpbe
>   fimc
>   imx
> + ipu3
>   ivtv
>   max2175
>   meye
> diff --git a/Documentation/media/v4l-drivers/ipu3.rst
> b/Documentation/media/v4l-drivers/ipu3.rst new file mode 100644
> index 000..045bf42
> --- /dev/null
> +++ b/Documentation/media/v4l-drivers/ipu3.rst
> @@ -0,0 +1,326 @@
> +.. include:: 
> +
> +===
> +Intel Image Processing Unit 3 (IPU3) Imaging Unit (ImgU) driver
> +===
> +
> +Copyright |copy| 2018 Intel Corporation
> +
> +Introduction
> +
> +
> +This file documents Intel IPU3 (3rd generation Image Processing Unit)
> Imaging

s/documents Intel/documents the Intel/

> +Unit driver located under drivers/media/pci/intel/ipu3.
> +
> +The Intel IPU3 found in certain Kaby Lake (as well as certain Sky Lake)
> +platforms (U/Y processor lines) is made up of two parts namely Imaging Unit
> +(ImgU) and CIO2 device (MIPI CSI2 receiver).

s/namely Imaging Unit/, namely the Imaging Unit/
s/and CIO2/and the CIO2/

> +
> +The CIO2 device receives the raw bayer data from the sensors and outputs
> the +frames in a format that is specific to IPU3 (for consumption by IPU3
> ImgU).

s/to IPU3/to the IPU3/
s/by IPU3/by the IPU3/

> +CIO2 driver is available as drivers/media/pci/intel/ipu3/ipu3-cio2*
> and is +enabled through the CONFIG_VIDEO_IPU3_CIO2 config option.

s/CIO2 driver/The CIO2 driver/

> +
> +The Imaging Unit (ImgU) is responsible for processing images captured

s/images/the images/

> +through IPU3 CIO2 device. The ImgU driver sources can be found under

s/IPU3 CIO2/the IPU3 CIO2/

> +drivers/media/pci/intel/ipu3 directory. The driver is enabled through the
> +CONFIG_VIDEO_IPU3_IMGU config option.
> +
> +The two driver modules are named ipu3-csi2 and ipu3-imgu, respectively.
> +
> +The driver has been tested on Kaby Lake platforms (U/Y processor lines).

I assume both drivers have been tested, so I would write

s/The driver has/The drivers have/

> +The driver implements V4L2, Media controller and V4L2 sub-device
> interfaces.

As this is true for both drivers,

s/The driver implements V4L2/Both drivers implement the V4L2/
s/Media controller/Media Controller/

> +Camera sensors that have CSI-2 bus, which are connected to the IPU3 CIO2
> +device are supported.

I would rephrase this slightly, as "The IPU3 CIO2 driver supports camera 
sensors connected to the CIO2 MIPI CSI-2 interfaces through V4L2 sub-device 
sensor drivers."

> Support for lens and flash drivers depends on the
> +above sensors.

That's a very good introduction !

I would follow with two sections, "IPU3 CIO2" followed by "IPU3 ImgU".

> +ImgU device nodes
> +=
> +
> +The ImgU is represented as two V4L2 subdevs, each of which provides a V4L2
> +subdev interface to the user space.

Not just subdevs, but video nodes too. I would rephrase as follows (please 
note that this might not be valid .rst content, I haven't tried compiling it, 
it might need to be reworked slightly) :

"The ImgU contains two independent pipes, each modelled as a V4L2 sub-device 
exposed to userspace as a V4L2 sub-device node.

Each pipe has two input pads and three output pads for the following purpose:

Pad 0 (input): Input raw video stream
Pad 1 (input): Processing parameters
Pad 2 (output): Output processed video stream
Pad 3 (output): Output viewfinder video stream
Pad 4 (output): 3A statistics

Each pad is connected to a corresponding V4L2 video interface, exposed to 
userspace as a V4L2 video device node."

> +Each V4L2 subdev represents a pipe, which can support a maximum of 2
> +streams. A private ioctl can be used to 

[PATCH v7 02/16] doc-rst: Add Intel IPU3 documentation

2018-10-29 Thread Yong Zhi
From: Rajmohan Mani 

This patch adds the details about the IPU3 Imaging Unit driver.

Change-Id: I560cecf673df2dcc3ec72767cf8077708d649656
Signed-off-by: Rajmohan Mani 
---
 Documentation/media/v4l-drivers/index.rst |   1 +
 Documentation/media/v4l-drivers/ipu3.rst  | 326 ++
 2 files changed, 327 insertions(+)
 create mode 100644 Documentation/media/v4l-drivers/ipu3.rst

diff --git a/Documentation/media/v4l-drivers/index.rst 
b/Documentation/media/v4l-drivers/index.rst
index 679238e..179a393 100644
--- a/Documentation/media/v4l-drivers/index.rst
+++ b/Documentation/media/v4l-drivers/index.rst
@@ -44,6 +44,7 @@ For more details see the file COPYING in the source 
distribution of Linux.
davinci-vpbe
fimc
imx
+   ipu3
ivtv
max2175
meye
diff --git a/Documentation/media/v4l-drivers/ipu3.rst 
b/Documentation/media/v4l-drivers/ipu3.rst
new file mode 100644
index 000..045bf42
--- /dev/null
+++ b/Documentation/media/v4l-drivers/ipu3.rst
@@ -0,0 +1,326 @@
+.. include:: 
+
+===
+Intel Image Processing Unit 3 (IPU3) Imaging Unit (ImgU) driver
+===
+
+Copyright |copy| 2018 Intel Corporation
+
+Introduction
+
+
+This file documents Intel IPU3 (3rd generation Image Processing Unit) Imaging
+Unit driver located under drivers/media/pci/intel/ipu3.
+
+The Intel IPU3 found in certain Kaby Lake (as well as certain Sky Lake)
+platforms (U/Y processor lines) is made up of two parts namely Imaging Unit
+(ImgU) and CIO2 device (MIPI CSI2 receiver).
+
+The CIO2 device receives the raw bayer data from the sensors and outputs the
+frames in a format that is specific to IPU3 (for consumption by IPU3 ImgU).
+CIO2 driver is available as drivers/media/pci/intel/ipu3/ipu3-cio2* and is
+enabled through the CONFIG_VIDEO_IPU3_CIO2 config option.
+
+The Imaging Unit (ImgU) is responsible for processing images captured
+through IPU3 CIO2 device. The ImgU driver sources can be found under
+drivers/media/pci/intel/ipu3 directory. The driver is enabled through the
+CONFIG_VIDEO_IPU3_IMGU config option.
+
+The two driver modules are named ipu3-csi2 and ipu3-imgu, respectively.
+
+The driver has been tested on Kaby Lake platforms (U/Y processor lines).
+
+The driver implements V4L2, Media controller and V4L2 sub-device interfaces.
+Camera sensors that have CSI-2 bus, which are connected to the IPU3 CIO2
+device are supported. Support for lens and flash drivers depends on the
+above sensors.
+
+ImgU device nodes
+=
+
+The ImgU is represented as two V4L2 subdevs, each of which provides a V4L2
+subdev interface to the user space.
+
+Each V4L2 subdev represents a pipe, which can support a maximum of 2
+streams. A private ioctl can be used to configure the mode (video or still)
+of the pipe.
+
+This helps to support advanced camera features like Continuous View Finder
+(CVF) and Snapshot During Video(SDV).
+
+CIO2 device
+===
+
+The CIO2 is represented as a single V4L2 subdev, which provides a V4L2 subdev
+interface to the user space. There is a video node for each CSI-2 receiver,
+with a single media controller interface for the entire device.
+
+Media controller
+
+
+The media device interface allows to configure the ImgU links, which defines
+the behavior of the IPU3 firmware.
+
+Device operation
+
+
+With IPU3, once the input video node ("ipu3-imgu 0/1":0,
+in : format) is queued with buffer (in packed raw bayer
+format), IPU3 ISP starts processing the buffer and produces the video output
+in YUV format and statistics output on respective output nodes. The driver
+is expected to have buffers ready for all of parameter, output and
+statistics nodes, when input video node is queued with buffer.
+
+At a minimum, all of input, main output, 3A statistics and viewfinder
+video nodes should be enabled for IPU3 to start image processing.
+
+Each ImgU V4L2 subdev has the following set of video nodes.
+
+input, output and viewfinder video nodes
+
+
+The frames (in packed raw bayer format specific to IPU3) received by the
+input video node is processed by the IPU3 Imaging Unit and is output to 2
+video nodes, with each targeting different purpose (main output and viewfinder
+output).
+
+Details on raw bayer format specific to IPU3 can be found as below.
+Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
+
+The driver supports V4L2 Video Capture Interface as defined at :ref:`devices`.
+
+Only the multi-planar API is supported. More details can be found at
+:ref:`planar-apis`.
+
+
+parameters video node
+-
+
+The parameter video node receives the ISP algorithm parameters that are used
+to configure how the ISP algorithms process the image.
+
+Details on raw bayer format specific to IPU3 can be found as below.