RE: [PATCH v12 7/8] documentation: media: add documentation for Toshiba Visconti Video Input Interface driver

Hello Laurent

Thank you for your review comments.

-----Original Message-----
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Sent: Friday, January 3, 2025 6:26 AM
To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
[off-list ref]
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Rob Herring
[off-list ref]; Krzysztof Kozlowski [off-list ref]; Conor Dooley
[off-list ref]; Sakari Ailus [off-list ref]; Hans
Verkuil [off-list ref]; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ
□ＤＩＴ○ＯＳＴ) [off-list ref];
linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org
Subject: Re: [PATCH v12 7/8] documentation: media: add documentation for
Toshiba Visconti Video Input Interface driver

Hi Ishikawa-san,

Thank you for the patch.

Overall the documentation looks quite good, it has significantly improved
compared to early versions.

On Mon, Nov 25, 2024 at 06:21:45PM +0900, Yuji Ishikawa wrote:

quoted

Added description of Video Input Interface driver of

s/Added/Add/

I'll fix it

quoted

Toshiba Visconti architecture.
It includes hardware organization, structure of the driver and
metadata format for embedded image signal processor.

Signed-off-by: Yuji Ishikawa <redacted>
---
Changelog v3:
- Newly add documentation to describe SW and HW

Changelog v4:
- no change

Changelog v5:
- no change

Changelog v6:
- add description of CSI2RX subdevice
- add ordering of ioctl(S_FMT) and ioctl(S_EXT_CTRLS)

Changelog v7:
- no change

Changelog v8:
- add usage of V4L2_CTRL_TYPE_VISCONTI_ISP

Changelog v9:
- fix warning: set reference target for keyword
V4L2_CTRL_TYPE_VISCONTI_ISP

Changelog v10:
- use parameter buffers instead of compound control
  - removed description of vendor specific compound control
  - add description of parameter buffers for ISP control
- update directory structure
  - remove documents under driver-api
  - add documents to admin-guide, userspace-api

Changelog v11:
- update usage of the driver

Changelog v12:
- add description of CSI2RX driver
- description of resizer subdevice
- add block diagrams of VIIF and ISP
- update usage of the driver

 .../admin-guide/media/v4l-drivers.rst         |   1 +
 .../admin-guide/media/visconti-viif.dot       |  22 +
 .../admin-guide/media/visconti-viif.rst       | 435

++++++++++++++++++

quoted

 .../userspace-api/media/v4l/meta-formats.rst  |   1 +
 .../media/v4l/metafmt-visconti-viif.rst       |  48 ++
 5 files changed, 507 insertions(+)
 create mode 100644 Documentation/admin-guide/media/visconti-viif.dot
 create mode 100644 Documentation/admin-guide/media/visconti-viif.rst
 create mode 100644
Documentation/userspace-api/media/v4l/metafmt-visconti-viif.rst

diff --git a/Documentation/admin-guide/media/v4l-drivers.rst

b/Documentation/admin-guide/media/v4l-drivers.rst
index b6af448b9fe9..81054512e768 100644

--- a/Documentation/admin-guide/media/v4l-drivers.rst
+++ b/Documentation/admin-guide/media/v4l-drivers.rst

@@ -32,5 +32,6 @@ Video4Linux (V4L) driver-specific documentation
 	si476x
 	starfive_camss
 	vimc
+	visconti-viif
 	visl
 	vivid

diff --git a/Documentation/admin-guide/media/visconti-viif.dot

b/Documentation/admin-guide/media/visconti-viif.dot
new file mode 100644
index 000000000000..cc75c73336fb

--- /dev/null
+++ b/Documentation/admin-guide/media/visconti-viif.dot

@@ -0,0 +1,22 @@
+digraph board {
+        rankdir=TB
+        n00000001 [label="{{<port0> 0 | <port4> 4} |

visconti-viif:isp\n/dev/v4l-subdev0 | {<port1> 1 | <port2> 2 | <port3> 3 |
<port5> 5}}", shape=Mrecord, style=filled, fillcolor=green]

quoted

+        n00000001:port1 -> n00000008:port0
+        n00000001:port2 -> n0000000b:port0
+        n00000001:port3 -> n00000016
+        n00000001:port5 -> n0000001e
+        n00000008 [label="{{<port0> 0} |

visconti-viif:resizer0\n/dev/v4l-subdev1 | {<port1> 1}}", shape=Mrecord,
style=filled, fillcolor=green]

quoted

+        n00000008:port1 -> n0000000e
+        n0000000b [label="{{<port0> 0} |

visconti-viif:resizer1\n/dev/v4l-subdev2 | {<port1> 1}}", shape=Mrecord,
style=filled, fillcolor=green]

quoted

+        n0000000b:port1 -> n00000012
+        n0000000e [label="viif_capture_post0\n/dev/video0", shape=box,

style=filled, fillcolor=yellow]

quoted

+        n00000012 [label="viif_capture_post1\n/dev/video1", shape=box,

style=filled, fillcolor=yellow]

quoted

+        n00000016 [label="viif_capture_sub\n/dev/video2", shape=box,

style=filled, fillcolor=yellow]

quoted

+        n0000001a [label="viif_params\n/dev/video3", shape=box,

style=filled, fillcolor=yellow]

quoted

+        n0000001a -> n00000001:port4
+        n0000001e [label="viif_stats\n/dev/video4", shape=box,

style=filled, fillcolor=yellow]

quoted

+        n00000030 [label="{{<port0> 0} | visconti_csi2rx

1c008000.csi2rx\n/dev/v4l-subdev3 | {<port1> 1}}", shape=Mrecord,
style=filled, fillcolor=green]

quoted

+        n00000030:port1 -> n00000001:port0
+        n00000035 [label="{{} | imx219 1-0010\n/dev/v4l-subdev4 |

{<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]

quoted

+        n00000035:port0 -> n00000030:port0 }

diff --git a/Documentation/admin-guide/media/visconti-viif.rst

b/Documentation/admin-guide/media/visconti-viif.rst
new file mode 100644
index 000000000000..c2e85fb6f8c1

--- /dev/null
+++ b/Documentation/admin-guide/media/visconti-viif.rst

@@ -0,0 +1,435 @@
+.. SPDX-License-Identifier: GPL-2.0
+

+================================================
======

quoted

+Visconti Video Input Interface Driver (visconti-viif)

+================================================
======

quoted

+
+Introduction
+============
+
+This file documents the driver for the Video Input Interface (VIIF)
+that is part of Toshiba Visconti SoCs.
+The driver is located under drivers/media/platform/toshiba/visconti
+and uses the Media-Controller API.

If you intend to make this two paragraphs, you should have a blank line
in-between:

--------
This file documents the driver for the Video Input Interface (VIIF) that is part of
Toshiba Visconti SoCs.

The driver is located under drivers/media/platform/toshiba/visconti and uses
the Media-Controller API.
--------

If you intend to make this a single paragraph, there should be no line break
after the first sentence:

--------
This file documents the driver for the Video Input Interface (VIIF) that is part of
Toshiba Visconti SoCs. The driver is located under
drivers/media/platform/toshiba/visconti and uses the Media-Controller API.
--------

I meant that the introduction has a single paragraph.
I'll update the paragraph's style.

quoted

+
+The driver module is named visconti-viif, and is enabled through the
+CONFIG_VIDEO_VISCONTI_VIIF config option.
+The CSI-2 receiver part is controlled by another module named
+visconti-csi2rx, which is enabled through the

CONFIG_VIDEO_VISCONTI_CSI2RX config option.

Please reflow text to reach as close to 80 columns:

--------
The driver module is named visconti-viif, and is enabled through the
CONFIG_VIDEO_VISCONTI_VIIF config option. The CSI-2 receiver part is
controlled by another module named visconti-csi2rx, which is enabled through
the CONFIG_VIDEO_VISCONTI_CSI2RX config option.
--------

Same comment below where applicable.

I'll fix it.

quoted

+
+The Visconti VIIF Hardware
+==========================
+
+The Visconti VIIF hardware is an internally developed video capture device.
+Following function modules are integrated:
+
+* MIPI CSI-2 receiver (CSI2RX)
+* L1 Image Signal Processor (L1ISP)
+
+  * Correction, enhancement, adjustment on bayer images.
+
+* L2 Image Signal Processor (L2ISP)
+
+  * Lens distortion correction
+  * Scaling & Cropping with up to 2 parameter sets
+  * Formatting picture (RGB, YUV, Grayscale, ...)
+  * Integrated DMAC: Writing picture into main memory
+
+* Video DMAC
+
+  * Writing picture into main memory
+
+Visconti5 SoC has two VIIF hardware instances.
+
+
+The hardware block diagram is shown below.::
+
+  The VIIF hardware
+

"POST0"

quoted

+                                                              "RGB

with scale 0"

quoted

+  +--------+    +----------+    +-----+    +-----+    +-----+

+--------+

quoted

+  | Sensor |--->|  CSI2RX  |--->|     |    |     |    |     |--->|

memory |

quoted

+  +--------+    +----------+    |     |    |     |    |     |

+--------+

quoted

+                                |     |    | L1  |    | L2  | "POST1"
+                                |     |--->| ISP |--->| ISP | "RGB with

scale 1"

quoted

+                                |     |    |     |    |     |

+--------+

quoted

+                                | MUX |    |     |    |     |--->|

memory |

quoted

+                                |     |    +-----+    +-----+

+--------+

quoted

+                                |     |                       "SUB"
+                                |     |                       "RAW

w/o scale"

quoted

+                                |     |        +------------+

+--------+

quoted

+                                |     |------> | Video DMAC |--->|

memory |

quoted

+                                +-----+        +------------+

+--------+

quoted

+
+Topology
+========
+
+Graph
+-----
+
+.. _visconti_viif_topology_graph:
+
+.. kernel-figure:: visconti-viif.dot
+	:alt: Diagram of the default media pipeline topology
+	:align: center
+
+The driver has 3 video devices for capturing images:
+
+- viif_capture_post0: capture device for image.
+    - corresponds to L2ISP.
+- viif_capture_post1: capture device for image.
+    - corresponds to L2ISP.
+- viif_capture_sub: capture device for bayer image.
+    - corresponds to Video DMAC.
+
+The driver has 2 video devices for controlling ISP.
+
+- viif_params: a metadata output device that receives ISP parameters.
+    - corresponds to L1ISP and L2ISP.
+- viif_stats: a metadata capture device that sends statistics.
+    - corresponds to L1ISP and L2ISP.
+
+The driver has 2 subdevices:

I count three subdevs in the list below (and there are actually two resizer
instances, so that would be 4 subdevs).

The number should have been updated.

quoted

+
+- visconti_csi2rx: CSI-2 receiver operation.
+    - corresponds to CSI2RX.
+- visconti-viif:isp: Image Signal Processor operation.
+    - corresponds to L1ISP and L2ISP.
+- visconti-viif:resizer: Scaling operation of Image Signal Processor.
+    - corresponds to L2ISP.
+
+visconti_csi2rx - CSI2 Receiver Subdevice Node
+---------------------------------------------------

The title underline should have the same length as the title.

I'll fix it.

quoted

+
+This subdevice node corresponds to a MIPI CSI2 receiver.
+It resides between an image sensor subdevice and the ISP subdevice.
+It controls CSI2 link configuration and training process.
+
+visconti-viif:isp - ISP Subdevice Node
+--------------------------------------
+
+This subdevice node corresponds to L1/L2 ISPs.
+It receives pictures from an sensor (via CSI2RX),

s/an sensor/a sensor/

I'll check the use of articles in comments/documents again.

quoted

+applies multiple operations on pictures, then passes resulting images to

capture nodes.

quoted

+
+ISP configurations/parameters are passed from userland via viif_params

node.

quoted

+The status of ISP operations are passed to userland via viif_stats node.

"stats" stands for "statistics", so I would write "The statistics computed by the
ISP are passed to ...".

And after reading more of this document, I realize that the viif_stats node
captures both statistics and status information. You could write "The statistics
computed by the ISP and the frame processing status are passed to ...".

Yes, the node captures both statistics and status information.
I'll update the sentence according to your suggestion.

quoted

+
+L1 ISP provides following operations:
+
+- Input: accepts 8, 10, 12, 14bit bayer format
+    - Operation selector; :c:type:`viif_l1_input_mode_config`
+        - HDR image / PWL (Piecewse Linear Compression) image
+        - with preprocessing / without preprocessing
+    - HDRE: HDR expansion (only for PWL image);
+      see :c:type:`viif_l1_hdre_config`
+- Preprocessing: generate intermediate data (24bit RAW)
+    - SLIC: Bit slicing (x3 12bit planes for preprocessing);
+      see :c:type:`viif_l1_img_extraction_config`
+    - ABPC/DPC: Blemish/Defect pixel

correction :c:type:`viif_l1_dpc_config`

quoted

+    - PWHB: Preset white balance;

see :c:type:`viif_l1_preset_white_balance_config`

quoted

+    - RCNR: RAW color noise reduction;

see :c:type:`viif_l1_raw_color_noise_reduction_config`

quoted

+    - HDRS: HDR synthesis; see :c:type:`viif_l1_hdrs_config`
+- Processing on RAW image: Main Process (MPRO)
+    - BLVC: black level correction and normalization;
+      see :c:type:`viif_l1_black_level_correction_config`
+    - LSSC: Lens shading correction; see :c:type:`viif_l1_lsc_config`
+    - MPRO: digital amplifier; see :c:type:`viif_l1_main_process_config`
+    - MPRO: bayer demosaicing; see :c:type:`viif_l1_main_process_config`
+    - MPRO: color matrix correction;

see :c:type:`viif_l1_main_process_config`

quoted

+    - HDRC: HDR compression;

If my understanding is correct, this implements global and local tone mapping. I
would mention it explicitly here, those terms are more common than "HDR
compression". You could write for instance

   - HDRC: HDR compression (global and local tone mapping);

As you pointed out, VIIF provides global and local tone mapping.
I'll update the sentence according to your suggestion.

quoted

+      see :c:type:`viif_l1_hdrc_config`, :c:type:`viif_l1_hdrc_ltm_config`,
+      :c:type:`viif_l1_rgb_to_y_coef_config`
+- Processing on RGB/YUV image: Video Process (VPRO)
+    - VPRO: gamma correction; see :c:type:`viif_l1_gamma_config`
+    - VPRO: RGB2YUV;
+      see :c:type:`viif_l1_rgb_to_y_coef_config`,
+      :c:type:`viif_l1_img_quality_adjustment_config`
+    - VPRO: image quality adjustment; see
+:c:type:`viif_l1_img_quality_adjustment_config`
+- Output: 16bit YUV
+- Feedback loop
+    - AWHB: auto white balance; see :c:type:`viif_l1_awb_config`,
+      :c:type:`viif_isp_capture_status`

Does this mean that the ISP can compute white balance gains automatically,
implementing AWB in hardware ? That's interesting, it's a feature I haven't seen
before.

Sorry for the lack of explanation.
The AWB HW calculates the average values of U and V for the specified area of
the input image and adjust R-gain and B-gain so that the difference of the two
values approaches to zero. The software specifies the configurations through
viif_l1_awb_config but does not intervene in the processing of each frame.
The software can know the final gain obtained, the average values of U and V,
and whether the calculation has converged.

It is also possible to calculate the average value in hardware, then compute
the gain in software, and set the gain in the register. However, this is not
recommended due to significant control delay.

quoted

+    - AEXP: auto exposure (average luminance calculation);
+      see :c:type:`viif_l1_avg_lum_generation_config`,

+      :c:type:`viif_l1_rgb_to_y_coef_config`, :c:type:`viif_isp_capture_status
`

quoted

+    - AG: analog gain calculation;
+      see :c:type:`viif_l1_ag_mode_config`,
+ :c:type:`viif_l1_ag_config`
+
+Below is the block diagram::
+
+  L1ISP::INPUT
+
+  +--------+                +-----+                      +-----+
+  | Input  |--------------->|     |--------------------->|     |
+  | 24bHDR |                |     |                      |     |
+  +--------+                | 24b |                      | 24b |
+                            | RAW |                      | RAW |
+  +--------+    +------+    | (0) |                      | (1) |
+  | Input  |--->| HDRE |--->|     |    +------------+    |     |
+  | 24bPWL |    |      |    |     |--->| preprocess |--->|     |
+  +--------+    +------+    +-----+    +------------+    +-----+

I'm not entirely sure to understand this correctly, could you please correct me
where I'm mistaken ?

I understand that the PWL input is HDR contents merged/stitched on the
sensor side, and compressed to a smaller than 24bpp width to reduce bus
bandwidth using a PWL function. The HDRE block applies the inverse function
to recover linear 24 bit data. In that case, the "preprocess"
pipeline operates on a single channel (using one of the param_h, param_m and
param_l set of parameters - which one ?). The HDRS blocks should be disabled
(how ?).

The HDR input, on the other hand, provides 2 or 3 channels with different
sensitivities (high, middle and low, from sensors that implement DOL or DCG
HDR). The preprocess pipeline operates on those channels with different sets
of parameters, and the HDRS combines the channels into a single 24 bit image.

I'm not entirely sure how the two or three channels are provided to the ISP, with
DOL sensors there's a delay before the sensor starts outputting the short (and
very short) lines, so line buffers are needed to realign the lines. I don't see this
in the block diagram.

It would be nice to expand the documentation a bit with such information about
HDR processing, as I assume other developers may face similar questions.

The L1ISP hardware supports following RAW formats.
* uncompressed single HDR image [8-24 bit]
    * This driver supports 8-14bits, as 16-24bit is not fully tested.
* PWL compressed single HDR image [8-14 bit]
* multiple SDR images [8-12 bit, 1-3 planes]
    * This driver does not support it for the following reasons.

As you pointed out, the PWL input is an 8-14 bit image, which is expanded to
a 24-bit image in the HDRE block. In the preprocess stage, the SLIC (bit slice)
block divides the 24-bit image into three images with different sensitivities
for processing by ABPC, PWHB, and RCNR. Finally, the HDRS block generates a 24-bit image.

The L1ISP hardware supports DOL HDR for multiple SDR inputs. In the document's
diagram, up to three images bypass the SLIC and are processed by the HDRS block.
However, as you pointed out, it is not possible to connect a typical DOL sensor
due to the lack of sufficient line buffer depth. Because it is difficult to
operate in practical cases, this driver does not support multiple SDR image inputs
nor DOL HDR.

Although this driver does not support memory-to-memory operation, the VIIF hardware
has an option to read in-memory images via a DMA reader. With that (possible) datapath,
properly organized line-interleaved images can be processed as DOL HDR input.

quoted

+
+  L1ISP::INPUT::preprocess
+
+  +-----+

+-----+

quoted

+  | 24b |    +------+    +------+    +------+    +------+

+------+    | 24b |

quoted

+  | RAW |--->| SLIC |--->| ABPC |--->| PWHB |--->| RCNR |--->| HDRS

|--->| RAW |

quoted

+  | (0) |    +------+    +------+    +------+    +------+    +------+

| (1) |

quoted

+  +-----+

+-----+

quoted

+
+  L1ISP::MainProcess(MPRO)
+
+  +-----+
+  | 24b |    +------+    +------+
+  | RAW |--->| BLVC |--->| LSSC |---+
+  | (1) |    +------+    +------+   |
+  +-----+                           |
+                                    |
+     +------------------------------+
+     |
+     |    +-----------+    +-------------+    +--------+

+-----+

quoted

+     +--->|   MPRO    |    |    MPRO     |    |  MPRO  |

+------+    | 16b |

quoted

+          |  Digital  |--->| Demosaicing |----| Color  |--->| HDRC

|--->| RGB |

quoted

+     +--->| Amplifier |    |             |    | Matrix |    +------+

|     |

quoted

+     |    +-----------+    +-------------+    +--------+

+-----+

quoted

+     |                         |    |
+     |    +--------------+     |    |    +------+
+     +----| Auto         |<----+    +--->| AEXP |---> Auto-Exposure

statistics

quoted

+          | Whitebalance |               +------+
+          +--------------+
+                 |
+                 +------------------------------> Auto-Whitebalance
+ statistics
+
+  L1ISP::VideoProcess(VPRO)
+
+  +-----+    +------------+    +------------+    +---------------+

+--------+

quoted

+  | 16b |--->| Gamma      |--->| RGB2YUV    |--->| Image Quality

|--->| Output |

quoted

+  | RGB |    | Correction |    | Conversion |    | Adjustment    |    |

16b   |

quoted

+  |     |    +------------+    +------------+    +---------------+

|  YUV   |

quoted

+  +-----+

+--------+

quoted

+
+  L1ISP::AnalogGain
+
+  statistics                     +-------------+

+------------------+

quoted

+  information ---> (user SW) --->| Analog Gain |--->| ABPC, RCNR, LSSC

|

quoted

+                                 +-------------+    |       MPRO,

VPRO |

quoted

+
+ +------------------+

I'm a bit puzzled by "analog gain" here, as the ISP operates on digital data only.
Does the ISP need to be informed of the analog gain values computed by the
AEGC software algorithm and applied to the camera sensor, for instance to
estimate the noise level based on the analog gain and adapt noise filtering
accordingly ? Or is it something else ?

Edit: the text below answers this question :-)

The name "analog gain" was coined by the hardware team and is quite misleading.
It has nothing to do with the gain of the image sensor. Instead, it is a parameter
used to collectively change the processing intensity of several functional blocks
in the L1ISP. The intensity is calculated from the gain value provided by
viif_l1_ag_config and the linear transformation parameters (grad, ofst) provided
by viif_l1_ag_mode_config. Each of the blocks determines the coefficients and
thresholds from the passed intensity.

To avoid misunderstandings, it seems better to stop using the term 'analog gain'
and replace it with expressions like 'application gain' or 'algorithm gain,'
which maintain the abbreviation 'AG' that appears in register names 
and structure member names."

quoted

+
+L2 ISP provides following operations:
+
+- Input: accepts 16bit YUV / RGB
+- Operations:
+    - Lens undistortion; see :c:type:`viif_l2_undist`

The structure is named viif_l2_undist_config. Please make sure to compile the
documentation, you should then get warnings for C types that are not found.

It seems I missed error messages.
I'll fix it.

quoted

+    - Scaling; see :c:type:`viif_l2_roi`

viif_l2_roi_config

I'll fix it too.

quoted

+    - Cropping; see :c:type:`viif_l2_roi`

Looking at the implementation of the resizer subdevs, I see that the crop and
compose rectangles can be set, but they don't seem to ne used to configure the
resizer. Instead, the viif_l2_roi_config parameters are used to configure
cropping on the resizer input and scaling. This discrepancy isn't good. I see two
options to fix it:

- Keep configuring the resizer through viif_l2_roi_config and drop the resizer
  subdevs. This will simplify the driver. The main drawback is that it
  won't be possible to implement digital zoom (by changing the resizer
  configuration) asynchronously from the ISP parameters buffers, which
  can be useful to lower the latency of digital zoom.

- Drop viif_l2_roi_config, and configure the resizer from the formats,
  crop and selection rectangles on the resizer subdev pads. This makes
  the driver more complex. The main advantage is that digital zoom will
  be configurable with a smaller latency, but the drawback is that it
  won't be possible (or it will be more difficult) to configure it
  synchronously with other ISP parameters.

There are drivers in mainline that implement either of those options, so you can
pick the one you think is best.

An additional issue is that the hardware seems to implement cropping on the
output of the resizer only, not on the input. Given that the size of the images
output by the ISP pipeline must stay constant during video capture (otherwise
there would be a risk of buffer overflow), modifying the crop rectangle on the
output of the resizer is usually not allowed during streaming. I think this could
be worked around by allowing modification only of the left and top coordinates
during streaming, but configuring everything through viif_l2_roi_config would
likely be easier. In that case, you should probably extend viif_l2_roi_config with
the crop offsets.

All of this reflects my current understanding of the ISP architecture, based on
this document and on the driver code, so please correct me if there's anything I
misunderstood. We can discuss the different options further before you modify
the driver and send a new version.

First of all, I apologize for attempting to make significant changes to the
driver structure without consulting you. The hardware specifications of VIIF
are unique, and the knowledge from the drivers I referred to (mainly rkisp1)
cannot be directly applied. As a result, I am still struggling with how to
write a driver that conforms to the V4L2 framework.

I am considering removing the resizer subdevice because viif_l2_roi_config
can set more detailed parameters compared to the resizer subdevice's pad.

To explain my idea and answer your questions, let me describe the processing of
the VIIF L2-ISP.

The central functions of the L2 ISP are lens undistortion and scaling. They are
integrated and executed simultaneously. To explain the coordinate transformation here,
several virtual images are defined.

* L1ISP output image
    * the input of L2ISP
    * geometry: {width, height}
    * mapped to sink.format
* Input image
    * geometry: {center_x, center_y, width, height}
    * relative to the L1ISP output image
    * mapped to sink.crop
    * resizer subdevice assumes that sink.crop = sink.format
* Corrected image after undistortion
    * geometry: {width, height}
    * relative to the Input image
* Corrected image with scale
    * geometry: {width, height}
    * relative to the Input image
    * mapped to sink.compose
    * resizer subdevice assumes that scale factor = sink.compose / sink.format
* ROI image
    * geometry: {left, top, width, height}
    * relative to the Corrected image with scale
    * mapped to source.crop
* Crop image
    * the output of L2ISP.
    * geometry: {left, top, width, height}
    * relative to the Corrected image with scale
    * mapped to source.crop

These images mentioned here do not have corresponding frame buffers. However,
there are control registers corresponding to the width and height of each image,
and appropriate values must be set. Otherwise, the output image may be corrupted,
or the L2ISP may abort.

Regarding your question of why sink.crop is not used.
According to the list above, sink.crop corresponds to the Input image. However,
the coordinates of the Input image must be set to align with the optical center
for undistortion, so it cannot be used to crop arbitrary rectangles.

Regarding digital zoom.
In the existing use cases of VIIF, the scale and crop are fixed during recording.
Therefore, I did not consider changing the selection parameter like digital zoom.

Regarding resizer subdevice.
The purpose of the resizer subdevice is to set the initial values of L2ISP
from pad's selection parameters. Most virtual image rectangles can be mapped to
the pad's parameters. Since the parameters of undistortion and the rectangle of
the Corrected image after undistortion are unknown, I assumed that undistortion 
is disabled (pass-through) and the Corrected image after undistortion matches
the Input image. In this case, the scaling parameters can be calculated from 
sink.compose and sink.input.
On the other hand, in cases where both undistortion and scaling are utilized,
it is necessary to set viif_l2_undist_config and viif_l2_roi_config. Therefore,
I made it possible to overwrite these parameters through the parameter buffer.

To configure cropping with the parameter buffer instead of pad's selection,
I'll add two members (left and top) to viif_l2_roi_config.
Otherwise, I'll newly add struct viif_l2_crop_config to the parameter buffer.

quoted

+    - Gamma correction; see :c:type:`viif_l2_gamma_config`
+    - YUV2RGB
+- Output: RGB, YUV422, YUV444
+
+Below is the block diagram::
+
+  L2ISP
+
+  +-------+    +------------+    +--------------+    +---------+
+  | Input |--->| YUV2RGB    |--->| Lens         |--->| Scaling |---> |
+  | Image |    | Conversion |    | Undistortion |    |         |---> |
+  +-------+    +------------+    +--------------+    +---------+

|

Is the scaling configuration for the two outputs independent ? If so I would
move the scaling block just before gamma correction, in each of the branches
below.

Scaling configurations are independent of each other.
I'll update the diagram as follows:

+--------------+      +---------+      +------------+
| Lens         | -+-> | Scaling | ---> | Gamma      | 
| Undistortion |  |   |         |      | Correction |
+--------------+  |   +---------+      +------------+
                  |
                  |   +---------+      +------------+
                  +-> | Scaling | ---> | Gamma      |
                      |         |      | Correction |
                      +---------+      +------------+

quoted

+

|

quoted

+          +----------------------------------------------------------+
+          |
+          |    +----------+    +------------+    +--------+

+--------+

quoted

+          +--->|Gamma     |--->| Colorspace |--->| Data   |--->|

Output |

quoted

+          |    |Correction|    | Conversion |    | Packer |    | Image  |
+          |    +----------+    +------------+    +--------+

+--------+

quoted

+          |
+          |    +----------+    +------------+    +--------+

+--------+

quoted

+          +--->|Gamma     |--->| Colorspace |--->| Data   |--->|

Output |

quoted

+               |Correction|    | Conversion |    | Packer |    | Image  |
+               +----------+    +------------+    +--------+

+--------+

quoted

+
+visconti-viif:resizer - Resizer Subdevice Node
+----------------------------------------------
+
+The resizer subdevice resides between ISP subdevice and Capture
+device on a capture path for post0 and post1.
+It receives resize and crop parameters for the specific capture path
+and controls L2ISP HW.
+
+following selection rectangles can be passed at VIDIOC_S_SELECTION

ioctl.

s/following/The following/

I'll fix it.

quoted

+
+- sink pads's compose rectangle (V4L2_SEL_TGT_COMPOSE) for scaling
+- source pad's crop rectangle (V4L2_SEL_TGT_CROP) for cropping
+
+
+viif_capture_post0, viif_capture_post1 - Processed Image Capture
+Video Node
+---------------------------------------------------------------------
+------
+
+These video nodes are used for capturing images processed at ISPs.
+Supported capture formats are as follows:
+
+- V4L2_PIX_FMT_RGB24
+- V4L2_PIX_FMT_ABGR32
+- V4L2_PIX_FMT_YUV422M
+- V4L2_PIX_FMT_YUV444M

The hardware doesn't support semi-planar formats (NV12 or NV16) or packed
formats (YUYV) ?

The hardware does not support NV12 nor NV16.
For packed formats, UYVY is avaiable but not tested fully.
I'll check if there are any issues with adding UYVY.

quoted

+- V4L2_PIX_FMT_Y16
+
+Bayer format is not supported. Use viif_capture_sub instead.
+
+POST0 and POST1 can output images from the same input image using
+different cropping and scaling settings.
+
+viif_capture_sub - Raw Image Capture Video Node
+-----------------------------------------------
+
+This video node is used for capturing bayer image from the sensor.
+The output picture has exactly the same resolution and format as the

sensor input.

quoted

+The pipeline does not edit pixel values.
+However, when writing pixel values to memory, they are shifted to the
+MSB to match either 8bit or 16bit.
+
+Therefore, resulting capture formats are as follows:
+
+- for 8bit RAW input:
+    - V4L2_PIX_FMT_SRGGB8
+    - V4L2_PIX_FMT_SGRBG8
+    - V4L2_PIX_FMT_SGBRG8
+    - V4L2_PIX_FMT_SBGGR8
+- for 10, 12, 14bit RAW input:
+    - V4L2_PIX_FMT_SRGGB16
+    - V4L2_PIX_FMT_SGRBG16
+    - V4L2_PIX_FMT_SGBRG16
+    - V4L2_PIX_FMT_SBGGR16
+
+.. _viif_params:
+
+viif_params - ISP Parameters Video Node
+---------------------------------------
+
+The viif_params video node receives a set of ISP parameters from
+userspace to be applied to the hardware during a video stream.
+
+The buffer format is defined by struct
+:c:type:`visconti_viif_isp_config`, and userspace should

set :ref:`V4L2_META_FMT_VISCONTI_VIIF_PARAMS
<v4l2-meta-fmt-visconti-viif-params>` as the data format.

quoted

+
+.. _viif_stats:
+
+viif_stats - Statistics Video Node
+----------------------------------
+
+The viif_stats video node provides current status of ISP.

The viif_stats video node provides statistics computed by the ISP and frame
processing status.

I'll update the sentence as you suggested.

quoted

+
+Following information is included:
+
+* statistics of auto white balance
+* average luminance information which can be used by auto exposure

software impl.

s/impl/implementation/

I should not have abbreviated a word.
I'll fix it.

I would also add

 * CSI-2 receiver calibration and error status
 * ISP error status

It's quite uncommon to provide this kind of status through ISP stats buffers, but
it sounds like an interesting idea. Other drivers usually keep error counters in
the kernel and expose them through debugfs.

These status information have existed since the initial implementation and must
be passed to the user space in some way. On the other hand, I think that these,
especially the CSI2 receiver error status, are not related to the 
frame-by-frame processing of the ISP, so they should not be included in the
parameter buffer. I should classify the information and update the members of
the parameter buffer accordingly. I think some of the information should be
exposed through debugfs. However, I'm not sure how to classify them and how to
implement the debugfs interface. Could you tell me the source code that
should be referred to?

quoted

+
+The buffer format is defined by struct
+:c:type:`visconti_viif_isp_stat`, and userspace should

set :ref:`V4L2_META_FMT_VISCONTI_VIIF_STATS
<v4l2-meta-fmt-visconti-viif-stats>` as the data format.

quoted

+
+Feedback Operations
+===================
+
+Among the so-called 3A functions, VIIF provides only auto-whitebalance

and auto-exposure.

quoted

+Auto-whitebalance is a standalone hardware feature.
+Some status information is available through the ISP statistics interface.
+
+Auto-exposure is realized through a combination of hardware and userland

software.

quoted

+VIIF provides weighted average luminance information through the ISP

statistics interface.

quoted

+The userland application calculates the sensor gain, sensor exposure and

ISP digital gain.

quoted

+The calculated parameters are then passed to sensor's controls and the ISP

parameter interface.

quoted

+
+Among ISP parameters, there are parameters called AG (analog gain).
+Actually, AG parameters have nothing to do with auto-exposure.
+It controls "strength" in several signal correction algorithms.
+Below is the list of the functions which may be affected by AG parameters:
+
+- ABPC/DPC
+- RCNR
+- LSSC
+- MPRO: color matrix correction
+- VPRO
+
+Capturing Video Frames Example
+==============================
+
+In the following example,
+imx219 camera is connected to pad 0 of 'visconti_csi2rx' subdevice.
+
+The following commands yield three pictures with different zoom ratio:
+- main path 0: 1.5x zoom, crop 1920x1080, RGB picture
+- main path 1: 0.67x zoom, crop 640x480, RGB picture
+- sub path: 1920x1080 RAW picture
+
+.. code-block:: bash
+
+	# set the links
+	media-ctl -d platform:visconti-viif-0 -r
+	media-ctl -d platform:visconti-viif-0 -l '"imx219 1-0010":0 ->

"visconti_csi2rx 1c008000.csi2rx":0 [1]'

quoted

+	media-ctl -d platform:visconti-viif-0 -l '"visconti_csi2rx

1c008000.csi2rx":1 -> "visconti-viif:isp":0 [1]'

quoted

+	media-ctl -d platform:visconti-viif-0 -l '"visconti-viif:isp":1 ->

"visconti-viif:resizer0":0 [1]'

quoted

+	media-ctl -d platform:visconti-viif-0 -l '"visconti-viif:isp":2 ->

"visconti-viif:resizer1":0 [1]'

quoted

+	media-ctl -d platform:visconti-viif-0 -l '"visconti-viif:isp":3 ->

"viif_capture_sub":0 [1]'

quoted

+	media-ctl -d platform:visconti-viif-0 -l '"visconti-viif:resizer0":1 ->

"viif_capture_post0":0 [1]'

quoted

+	media-ctl -d platform:visconti-viif-0 -l '"visconti-viif:resizer1":1 ->

"viif_capture_post1":0 [1]'

quoted

+	media-ctl -d platform:visconti-viif-0 -l '"viif_params":0 ->

"visconti-viif:isp":4 [1]'

quoted

+	media-ctl -d platform:visconti-viif-0 -l '"visconti-viif:isp":5 ->

"viif_stats":0 [1]'

quoted

+
+	# set format for imx219
+	media-ctl -d platform:visconti-viif-0 --set-v4l2 '"imx219 1-0010":0

[fmt:SRGGB10_1X10/1920x1080]'

quoted

+
+	# set format for csi2rx
+	media-ctl -d platform:visconti-viif-0 --set-v4l2 '"visconti_csi2rx

1c008000.csi2rx":0 [fmt:SRGGB10_1X10/1920x1080  field:none
colorspace:raw xfer:none ycbcr:601 quantization:full-range]'

quoted

+
+	# set format for isp sink pad
+	media-ctl -d platform:visconti-viif-0 --set-v4l2 '"visconti-viif:isp":0

[fmt:SRGGB10_1X10/1920x1080]'

quoted

+
+	# set format for resizer pads
+	media-ctl -d platform:visconti-viif-0 --set-v4l2

'"visconti-viif:resizer0":0 '"[fmt:YUV8_1X24/1920x1080
compose:(0,0)/2880x1620]"

quoted

+	media-ctl -d platform:visconti-viif-0 --set-v4l2

'"visconti-viif:resizer0":1 '"[crop:(480,16)/1920x1080]"

quoted

+	media-ctl -d platform:visconti-viif-0 --set-v4l2

'"visconti-viif:resizer1":0 '"[fmt:YUV8_1X24/1920x1080
compose:(0,0)/1280x720]"

quoted

+	media-ctl -d platform:visconti-viif-0 --set-v4l2

'"visconti-viif:resizer1":1 '"[crop:(320,32)/640x480]"

quoted

+
+	media-ctl -d platform:visconti-viif-0 --set-v4l2 '"visconti-viif:isp":1

[fmt:YUV8_1X24/1024 crop:(640,0)/1024x1024]'

quoted

+
+	# set format for main path0
+	v4l2-ctl -z platform:visconti-viif-0 -d viif_capture_post0 -v

"width=1920,height=1080"

quoted

+	v4l2-ctl -z platform:visconti-viif-0 -d viif_capture_post0 -v

"pixelformat=RGB3"

quoted

+
+	# set format for main path1
+	v4l2-ctl -z platform:visconti-viif-0 -d viif_capture_post0 -v

"width=640,height=480"

quoted

+	v4l2-ctl -z platform:visconti-viif-0 -d viif_capture_post0 -v

"pixelformat=RGB3"

quoted

+
+	# start streaming
+	v4l2-ctl -z platform:visconti-viif-0 -d viif_capture_post0
+--stream-mmap --stream-count 1000 &
+
+	# start streaming with other devices while viif_capture_post0 is

running

quoted

+	v4l2-ctl -z platform:visconti-viif-0 -d viif_capture_post1

--stream-mmap --stream-count 10

quoted

+	v4l2-ctl -z platform:visconti-viif-0 -d viif_capture_sub
+--stream-mmap --stream-count 10
+
+Use of coherent memory
+======================
+
+Visconti5 SoC has two independent DDR SDRAM controllers.
+Each controller is mapped to 36bit address space.
+
+Accelerator bus masters have two paths to access memory; one is
+directly connected to SDRAM controller, the another is connected via
+a cache coherency bus which keeps coherency among CPUs.
+
+From accelerators and CPUs, the address map is following:
+
+* 0x0_8000_0000 DDR0 direct access
+* 0x4_8000_0000 DDR0 coherency bus
+* 0x8_8000_0000 DDR1 direct access
+* 0xC_8000_0000 DDR1 coherency bus
+
+The base address can be specified with "memory" and "reserved-memory"
+elements in a device tree description.
+It's not recommended to mix direct address and coherent address.
+
+The Visconti5 VIIF driver always use only direct address to configure Video

DMACs of the hardware.

quoted

+This design is to avoid great performance loss at coherency bus caused by

massive memory access.

quoted

+You should not put the dma_coherent attribute to viif element in device

tree.

quoted

+Cache operations are done automatically by videobuf2 driver.

diff --git a/Documentation/userspace-api/media/v4l/meta-formats.rst

b/Documentation/userspace-api/media/v4l/meta-formats.rst
index 86ffb3bc8ade..2336842f0c26 100644

--- a/Documentation/userspace-api/media/v4l/meta-formats.rst
+++ b/Documentation/userspace-api/media/v4l/meta-formats.rst

@@ -19,6 +19,7 @@ These formats are used for the :ref:`metadata` interface

only.

quoted

     metafmt-pisp-fe
     metafmt-rkisp1
     metafmt-uvc
+    metafmt-visconti-viif
     metafmt-vivid
     metafmt-vsp1-hgo
     metafmt-vsp1-hgt
diff --git
a/Documentation/userspace-api/media/v4l/metafmt-visconti-viif.rst
b/Documentation/userspace-api/media/v4l/metafmt-visconti-viif.rst
new file mode 100644
index 000000000000..dc4b31627fe1

--- /dev/null
+++ b/Documentation/userspace-api/media/v4l/metafmt-visconti-viif.rst

@@ -0,0 +1,48 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+.. _v4l2-meta-fmt-visconti-viif-params:
+
+.. _v4l2-meta-fmt-visconti-viif-stats:
+
+*********************************************************************
+****************** V4L2_META_FMT_VISCONTI_VIIF_PARAMS ('vifp'),
+V4L2_META_FMT_VISCONTI_VIIF_STATS ('vifs')
+*********************************************************************
+******************
+
+Configuration parameters
+========================
+
+The configuration parameters are passed to the :ref:`viif_params
+<viif_params>` metadata output video node, using the
+:c:type:`v4l2_meta_format` interface. The buffer contains a single
+instance of the C structure :c:type:`visconti_viif_isp_config`
+defined in ``visconti_viif.h``. So the structure can be obtained from the

buffer by:

quoted

+
+.. code-block:: c
+
+	struct visconti_viif_isp_config *params = (struct
+visconti_viif_isp_config*) buffer;
+
+VIIF statistics
+===============
+
+The VIIF device collects different statistics over an input Bayer frame.
+Those statistics are obtained from the :ref:`viif_stats <viif_stats>`
+metadata capture video node, using the :c:type:`v4l2_meta_format`
+interface. The buffer contains a single instance of the C structure
+:c:type:`visconti_viif_isp_stat` defined in ``visconti_viif.h``. So
+the structure can be obtained from the buffer by:
+
+.. code-block:: c
+
+	struct visconti_viif_isp_stat *stats = (struct
+visconti_viif_isp_stat*) buffer;
+
+The statistics collected are Exposure, AWB (auto white balance) and errors.
+See :c:type:`visconti_viif_isp_stat` for details of the statistics.
+
+The statistics and configuration parameters described here are
+usually consumed and produced by dedicated user space libraries that
+comprise the tuning tools using software control loop.
+
+visconti viif uAPI data types
+=============================
+
+.. kernel-doc:: include/uapi/linux/visconti_viif.h

Assuming the documentation in the header file is adequate, the level of detail
here is fine.

--
Regards,

Laurent Pinchart

Regards,

Yuji Ishikawa