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

From: yuji2.ishikawa
Date: Tue Apr 15 2025 - 07:48:16 EST


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)

> -----Original Message-----
> From: ishikawa yuji(石川 悠司 ○RDC□AITC○EA開)
> Sent: Wednesday, February 5, 2025 9:30 PM
> To: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>; Rob Herring
> <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>; Hans
> Verkuil <hverkuil-cisco@xxxxxxxxx>; iwamatsu nobuhiro(岩松 信洋 ○DITC
> □DIT○OST) <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>;
> linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> 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.
>

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

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.

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

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?

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

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)

> > > +
> > > +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.
> > > +
> > > +Feedback Operations
> > > +===================
> > > +
> > > +Among the so-called 3A functions, VIIF provides only auto-whitebalance
> > and auto-exposure.
> > > +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.
> > > +VIIF provides weighted average luminance information through the ISP
> > statistics interface.
> > > +The userland application calculates the sensor gain, sensor exposure and
> > ISP digital gain.
> > > +The calculated parameters are then passed to sensor's controls and the
> ISP
> > parameter interface.
> > > +
> > > +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]'
> > > + media-ctl -d platform:visconti-viif-0 -l '"visconti_csi2rx
> > 1c008000.csi2rx":1 -> "visconti-viif:isp":0 [1]'
> > > + media-ctl -d platform:visconti-viif-0 -l '"visconti-viif:isp":1 ->
> > "visconti-viif:resizer0":0 [1]'
> > > + media-ctl -d platform:visconti-viif-0 -l '"visconti-viif:isp":2 ->
> > "visconti-viif:resizer1":0 [1]'
> > > + media-ctl -d platform:visconti-viif-0 -l '"visconti-viif:isp":3 ->
> > "viif_capture_sub":0 [1]'
> > > + media-ctl -d platform:visconti-viif-0 -l '"visconti-viif:resizer0":1 ->
> > "viif_capture_post0":0 [1]'
> > > + media-ctl -d platform:visconti-viif-0 -l '"visconti-viif:resizer1":1 ->
> > "viif_capture_post1":0 [1]'
> > > + media-ctl -d platform:visconti-viif-0 -l '"viif_params":0 ->
> > "visconti-viif:isp":4 [1]'
> > > + media-ctl -d platform:visconti-viif-0 -l '"visconti-viif:isp":5 ->
> > "viif_stats":0 [1]'
> > > +
> > > + # set format for imx219
> > > + media-ctl -d platform:visconti-viif-0 --set-v4l2 '"imx219 1-0010":0
> > [fmt:SRGGB10_1X10/1920x1080]'
> > > +
> > > + # 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]'
> > > +
> > > + # set format for isp sink pad
> > > + media-ctl -d platform:visconti-viif-0 --set-v4l2 '"visconti-viif:isp":0
> > [fmt:SRGGB10_1X10/1920x1080]'
> > > +
> > > + # 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]"
> > > + media-ctl -d platform:visconti-viif-0 --set-v4l2
> > '"visconti-viif:resizer0":1 '"[crop:(480,16)/1920x1080]"
> > > + media-ctl -d platform:visconti-viif-0 --set-v4l2
> > '"visconti-viif:resizer1":0 '"[fmt:YUV8_1X24/1920x1080
> > compose:(0,0)/1280x720]"
> > > + media-ctl -d platform:visconti-viif-0 --set-v4l2
> > '"visconti-viif:resizer1":1 '"[crop:(320,32)/640x480]"
> > > +
> > > + media-ctl -d platform:visconti-viif-0 --set-v4l2 '"visconti-viif:isp":1
> > [fmt:YUV8_1X24/1024 crop:(640,0)/1024x1024]'
> > > +
> > > + # set format for main path0
> > > + v4l2-ctl -z platform:visconti-viif-0 -d viif_capture_post0 -v
> > "width=1920,height=1080"
> > > + v4l2-ctl -z platform:visconti-viif-0 -d viif_capture_post0 -v
> > "pixelformat=RGB3"
> > > +
> > > + # set format for main path1
> > > + v4l2-ctl -z platform:visconti-viif-0 -d viif_capture_post0 -v
> > "width=640,height=480"
> > > + v4l2-ctl -z platform:visconti-viif-0 -d viif_capture_post0 -v
> > "pixelformat=RGB3"
> > > +
> > > + # 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
> > > + v4l2-ctl -z platform:visconti-viif-0 -d viif_capture_post1
> > --stream-mmap --stream-count 10
> > > + 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.
> > > +This design is to avoid great performance loss at coherency bus caused by
> > massive memory access.
> > > +You should not put the dma_coherent attribute to viif element in device
> > tree.
> > > +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.
> > > 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:
> > > +
> > > +.. 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

Regards,

Yuji Ishikawa