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

Hello Laurent,

I will begin the revision work on the VIIF driver v13 patch according to the following revision policy proposal submitted on 4/15. 
If there are more appropriate methods or points to consider, especially regarding structural-level changes to the driver, please let me know.

-----Original Message-----
From: ishikawa yuji(石川 悠司 □ＡＩＤＣ○ＥＡ開)
Sent: Tuesday, April 15, 2025 6:01 PM
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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

Hello Laurent,

As I reconsider the implementation of the v12 driver, I have decided on how to
change the important parts pointed out in the review comments. On the other
hand, I'm still looking for better implementation methods in some other areas.
I would like to request your comments on these matters.

- Topic 1: Removing the resizer subdevice
- Topic 2: Adding new members to the parameter buffer
- Topic 3: Passing error information via debugfs
- Topic 4: Reconsidering the topology of the capture device (sub path)

quoted

-----Original Message-----
From: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
Sent: Wednesday, February 5, 2025 9:30 PM
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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

Hello Laurent

Thank you for your review comments.

quoted

quoted

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

quoted

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

quoted

options to fix it:

- Keep configuring the resizer through viif_l2_roi_config and drop the

resizer

quoted

quoted

  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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

misunderstood. We can discuss the different options further before
you

modify

quoted

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.

quoted

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

subdevice's pad.

quoted

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.

quoted

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.

quoted

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.

quoted

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.

I am removing the resizer sub-device. In the new implementation, the
undistortion and scaling functions of L2ISP are controlled using the l2_undist
member and l2_roi member of the parameter buffer. To control the cropping
function of L2ISP, the l2_crop_post0 and l2_crop_post1 members are added to
the parameter buffer to indicate the top-left coordinates for cropping.
These new members have the type struct l2_crop_config {__u32 left; __u32 top}.
The default values of the members are (0, 0).

ISP subdevice pads have the following specifications:
- The sink_pad::fmt and source_pad(post0)::fmt, source_pad(post1)::fmt can
be set independently.
- Each pad cannot be set for crop and compose.
- The maximum size of source_pad::fmt is either the hardware limit of L2ISP or
2x sink_pad::fmt.
- The minimum size of source_pad::fmt is the hardware limit of L2ISP.
- Due to the inclusion relationship of the planes, the following constraint should
be met:

  l2_crop_post0::left + source_pad(post0)::fmt::width <=
l2_roi::corrected_width(post0)

  A similar constraint should be applied to the height and the post1.

There are two possible behaviors when constraints are not met. If the
instructions through the parameter buffer are not accepted, how should the
driver behave?

- Increment the error counter (referred by debugfs) and ignore the instructions
  from the parameter buffer.
- Modify l2_roi::corrected_width(post0) and l2_crop_post0::left and apply them
  to the hardware. As a result, an L2ISP error might occur. The drawback is that
  the user program cannot know the modified coordinates.

quoted

quoted

quoted

+.. _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

+

Please advise on how to handle unverified ISP functions in the parameter
buffer.

The L1ISP hardware has a function to calculate the luminance histogram.
The software for this function is experimental and has not yet been developed
as a Linux driver. However, since the structure that the parameter buffer
handles comprehensively represents all ISP functions, I understand that it
would be difficult to add members for the histogram function later.

Should members corresponding to the histogram function be added to the
parameter buffer structure even if the implementation does not exist? Or, is it
permissible to add new members to the parameter buffer structure based on
certain conditions?

quoted

quoted

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

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.

quoted

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

quoted

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?

In the new implementation, the following information will be passed to user
space using debugfs:

- Errors during capture (originally, struct viif_reported_errors)
  - Counters will be prepared for each error cause. This will be implemented
    using debugfs_create_ulong().
  - The original specification was to "clear on read", but since it seems
    difficult to implement concisely, it will be changed to a count-up operation.
- CSI2RX calibration status
  - Information in three states (OK, Not-done, Error) for each signal line.
    This will be represented using debugfs_create_x8() or
debugfs_create_file()
    in a human-readable format.
- CSI2RX error status
  - Bit vector information for each error cause. This will be represented using
    debugfs_create_x32().

The following information will remain in the parameter buffer's statistics
interface:

- Average luminance information
- Auto white balance information

--------------------

I would like to reconsider the connection between the Capture device (sub) and
other subdevices. Could you provide some feedback?

Currently, the Capture device (sub) is connected to the ISP subdevice.
This topology is based on my best understanding of the media controller
framework at the time of design. However, the Capture device (sub) saves RAW
images without processing them, and its operation does not depend on ISP
functions. Therefore, for better maintenance in the future, I am considering
disconnecting the Capture device (sub) from the ISP subdevice.

Additionally, I want to prepare for improvements regarding streams. The
CSI2RX hardware can handle multiple virtual channels. Both the ISP hardware
and Capture hardware (sub) can each select and receive one virtual channel.
Currently, due to insufficient tests, both are limited to receiving virtual channel
0, but in the future, I want to be able to handle multiple channels using the
stream API.

I'm considering three options for the topology.

- Option 1: Connect the Capture device (sub) to the ISP subdevice as it is
currently.
  The switching of streams will be realized as a function of the ISP subdevice.
- Option 2: Create a new MUX subdevice and connect the ISP subdevice and
  Capture device (sub) to it. When handling multiple streams in the future,
  the switching function will be realized by the MUX subdevice. This will
reduce
  the complexity of the ISP subdevice implementation.
- Option 3: Connect both the ISP subdevice and Capture device (sub) to
  the source port of CSI2RX. Is such a configuration really possible? The
switching
  of multiple streams will be handled by both the ISP subdevice and Capture
device respectively.


[Option1: Current design]
CSI2RX|0 -- ISP|1 -- Capture(post0)
               |2 -- Capture(post1)
               |3 -- Capture(sub)

[Option2]
CSI2RX|0 -- MUX|1 -- ISP|1 -- Capture(post0)
               |        |2 -- Capture(post1)
               |2 -- Capture(sub)

[Option3]
CSI2RX|0 --+-- ISP|1 -- Capture(post0)
           |      |2 -- Capture(post1)
           +-- Capture(sub)

quoted

quoted

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.

quoted

quoted

quoted

+
+Auto-exposure is realized through a combination of hardware and

userland

quoted

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

quoted

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:

quoted

quoted

+
+- 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"

quoted

quoted

+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

quoted

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

quoted

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

quoted

quoted

@@ -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:

quoted

quoted

quoted

+
+.. 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.

quoted

quoted

+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

Regards,

Yuji Ishikawa

Regards,

Yuji Ishikawa