Hello Laurent,
Thank you for your review.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Tuesday, July 23, 2024 3:03 AM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@xxxxxxxxxxxxx>
> Cc: Hans Verkuil <hverkuil@xxxxxxxxx>; Mauro Carvalho Chehab
> <mchehab@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Mark Brown
> <broonie@xxxxxxxxxx>; Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>; iwamatsu
> nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○ＯＳＴ)
> <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v11 5/6] documentation: media: add documentation for
> Toshiba Visconti Video Input Interface driver
>
> Hi Ishikawa-san,
>
> Thank you for the patch.
>
> On Tue, Jul 09, 2024 at 09:08:47AM +0900, Yuji Ishikawa wrote:
> > Added description of Video Input Interface driver of Toshiba Visconti
> > architecture.
> > It includes hardware organization, structure of the driver and
> > metadata format for embedded image signal processor.
> >
> > Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@xxxxxxxxxxxxx>
> > ---
> > 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
> >
> > .../admin-guide/media/v4l-drivers.rst | 1 +
> > .../admin-guide/media/visconti-viif.dot | 18 ++
> > .../admin-guide/media/visconti-viif.rst | 255
> ++++++++++++++++++
> > .../userspace-api/media/v4l/meta-formats.rst | 1 +
> > .../media/v4l/metafmt-visconti-viif.rst | 48 ++++
> > 5 files changed, 323 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 4120eded9a..c7e67f455a 100644
> > --- a/Documentation/admin-guide/media/v4l-drivers.rst
> > +++ b/Documentation/admin-guide/media/v4l-drivers.rst
> > @@ -31,5 +31,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 0000000000..239d9fa2b3
> > --- /dev/null
> > +++ b/Documentation/admin-guide/media/visconti-viif.dot
> > @@ -0,0 +1,18 @@
> > +digraph board {
> > + rankdir=TB
> > + n00000001 [label="{{<port0> 0} |
> visconti-viif:csi2rx\n/dev/v4l-subdev0 | {<port1> 1}}", shape=Mrecord,
> style=filled, fillcolor=green]
> > + n00000001:port1 -> n00000004:port0
> > + n00000004 [label="{{<port0> 0 | <port4> 4} |
> visconti-viif:isp\n/dev/v4l-subdev1 | {<port1> 1 | <port2> 2 | <port3> 3 |
> <port5> 5}}", shape=Mrecord, style=filled, fillcolor=green]
> > + n00000004:port1 -> n0000000b
> > + n00000004:port2 -> n0000000f
> > + n00000004:port3 -> n00000013
> > + n00000004:port5 -> n0000001b
> > + n0000000b [label="viif_capture_post0\n/dev/video0", shape=box,
> style=filled, fillcolor=yellow]
> > + n0000000f [label="viif_capture_post1\n/dev/video1", shape=box,
> style=filled, fillcolor=yellow]
> > + n00000013 [label="viif_capture_sub\n/dev/video2", shape=box,
> style=filled, fillcolor=yellow]
> > + n00000017 [label="viif_params\n/dev/video3", shape=box,
> style=filled, fillcolor=yellow]
> > + n00000017 -> n00000004:port4
> > + n0000001b [label="viif_stats\n/dev/video4", shape=box,
> style=filled, fillcolor=yellow]
> > + n0000002b [label="{{} | imx219 1-0010\n/dev/v4l-subdev2 |
> {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
> > + n0000002b:port0 -> n00000001:port0 }
> > diff --git a/Documentation/admin-guide/media/visconti-viif.rst
> > b/Documentation/admin-guide/media/visconti-viif.rst
> > new file mode 100644
> > index 0000000000..4ef676754c
> > --- /dev/null
> > +++ b/Documentation/admin-guide/media/visconti-viif.rst
> > @@ -0,0 +1,255 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> >
> +================================================
> ======
> > +Visconti Video Input Interface Driver (visconti-viif)
> >
> +================================================
> ======
> > +
> > +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.
> > +
> > +The Visconti VIIF Hardware
> > +==========================
> > +
> > +The Visconti VIIF hardware is a proprietary video capture device.
>
> In what sense is it "proprietary" ? I know the hardware itself isn't open-source,
> but that's the case of most of the devices the kernel supports :-)
>
It's more accurate to say "in-house" device rather than "proprietary" one.
I'll fix it.
> > +Following function modules are integrated:
> > +
> > +* MIPI CSI2 receiver (CSI2RX)
> > +* L1 Image Signal Processor (L1ISP)
> > +
> > + * Correction, enhancement, adjustment on RAW pictures.
> > +
> > +* L2 Image Signal Processor (L2ISP)
> > +
> > + * Lens distortion correction
> > + * Scaling
> > + * Cropping
> > +
> > +* Video DMAC
> > +
> > + * format picture (RGB, YUV, Grayscale, ...)
> > + * write picture into DRAM
> > +
> > +Visconti5 SoC has two VIIF hardware instances.
>
> Could you provide a block diagram of the hardware, that shows how the
> components are connected ? The .dot file above shows the media graph, but
> doesn't map to a more detailed hardware view. I would like to see, in particular,
> how the CSI-2 RX, L1ISP, L2ISP and DMAC are connected, and ideally also the
> processing blocks inside the L1ISP and L2ISP, and how they are ordered. It is
> very important, for instance, to know exactly where in the processing pipeline
> the statistics are collected.
>
> See
> https://lore.kernel.org/linux-media/20240709132906.3198927-9-dan.scally@i
> deasonboard.com/
> and
> https://lore.kernel.org/linux-media/20240709132906.3198927-15-dan.scally@
> ideasonboard.com/
> for examples of the level of details. The diagram in the first link is a good
> candidate for this section of the documentation, and the one from the second
> link could go below in the "visconti-viif:isp - ISP Subdevice Node" section. You
> can use ascii art of .dot or .svg, up to you.
>
I'll add block diagrams of the hardware (whole VIIF / inside ISPs).
> > +
> > +Topology
> > +========
> > +
> > +.. _visconti_viif_topology_graph:
> > +
> > +.. kernel-figure:: visconti-viif.dot
> > + :alt: Diagram of the default media pipeline topology
> > + :align: center
> > +
> > +The driver has 5 video devices:
> > +
> > +- viif_capture_post0: capture device for image.
> > + - corresponds to L2ISP and Video DMAC HW.
> > +- viif_capture_post1: capture device for image.
> > + - corresponds to L2ISP and Video DMAC HW.
> > +- viif_capture_sub: capture device for RAW image.
> > + - corresponds to Video DMAC HW.
>
> Does the raw capture occur right after the CSI-2 receiver, or is there any
> processing applied to the image ? That's a question that would be answered by
> a hardware block diagram.
>
RAW capture occurs right after the CSI-2 receiver without any image processings.
> > +- viif_params: a metadata output device that receives ISP parameters.
> > + - corresponds to L1ISP and L2ISP HW.
> > +- viif_stats: a metadata capture device that sends statistics.
> > + - corresponds to L1ISP and L2ISP HW.
> > +
> > +The driver has 2 subdevices:
> > +
> > +- visconti-viif:csi2rx: CSI2 receiver operation.
> > + - corresponds to CSI2RX HW.
> > +- visconti-viif:isp: Image Signal Processor operation.
> > + - corresponds to L1ISP and L2ISP HW.
> > +
> > +viif_capture_post0, viif_capture_post1 - Processed Image Capture
> > +Video Node
> > +---------------------------------------------------------------------
> > +------
> > +
> > +These video nodes are used for capturing images processed at ISPs.
> > +They capture only following formats:
> > +
> > +- RGB3
> > +- AR24
> > +- YM16
> > +- YM24
> > +- Y16
>
> Please list the formats using the V4L2_PIX_FMT_* macros instead of the 4CC
> value, it will be easier to read.
>
I'll use V4L2_PIX_FMT_*.
> > +
> > +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.
>
> Are there any other processing parameters that can differ between POST0 and
> POST1 ? I'm thinking for instance about colour spaces. If so, this should be
> explained here.
>
POST0 and POST1 support the same functions and color spaces.
> > +
> > +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.
> > +The following depth of bayer format is supported:
> > +
> > +- 8bit
> > +- 10bit
> > +- 12bit
> > +- 14bit
>
> Does the hardware support capturing embedded data from the sensor ?
>
The hardware supports capturing embdded data, however the software is not fully tested for that feature.
> > +
> > +visconti-viif:csi2rx - CSI2 Receiver Subdevice Node
> > +---------------------------------------------------
> > +
> > +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), applies multiple
> > +operations on pictures, then passes resulting images to capture nodes.
> > +
> > +L1 ISP provides following operations:
> > +
> > +- Input: accepts 8, 10, 12, 14bit bayer format
> > +- Preprocessing: generate intermediate data (24bit RAW)
> > + - HDRE: HDR expansion (only for PWL image);
> > +
> see :c:type:`viif_l1_hdre_config`, :c:type:`viif_l1_input_mode_config`
> > + - SLIC: Image Extraction (x3 12bit planes for preprocessing);
> > + see :c:type:`viif_l1_img_extraction_config`
> > + - ABPC/DPC: Defect pixel correction :c:type:`viif_l1_dpc_config`
> > + - PWHB: Preset white balance;
> see :c:type:`viif_l1_preset_white_balance_config`
> > + - RCNR: RAW color noise reduction;
> see :c:type:`viif_l1_raw_color_noise_reduction_config`
> > + - HDRS: HDR synthesis; see :c:type:`viif_l1_hdrs_config`
> > +- Processing on RAW image
> > + - 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`
> > + - HDRC: HDR compression;
> > + 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
> > + - 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 with software
> > + - AWHB: auto white balance; see :c:type:`viif_l1_awb_config`,
> > + :c:type:`viif_isp_capture_status`
> > + - 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
> `
> > + - AG: analog gain calculation;
> > + see :c:type:`viif_l1_ag_mode_config`,
> > +:c:type:`viif_l1_ag_config`
> > +
> > +L2 ISP provides following operations:
> > +
> > +- Input: accepts 16bit YUV
> > +- Operations:
> > + - Lens undistortion; see :c:type:`viif_l2_undist`
> > + - Scaling; see :c:type:`viif_l2_roi`
> > + - Cropping; see :c:type:`viif_l2_roi`
> > + - Gamma correction; see :c:type:`viif_l2_gamma_config`
> > + - YUV2RGB
> > +- Output: RGB, YUV422, YUV444
> > +
> > +ISP configurations/parameters are passed from userland via viif_params
> node.
> > +The status of ISP operations are passed to userland via viif_stats node.
> > +
> > +.. _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.
> > +
> > +.. _viif_stats:
> > +
> > +viif_stats - Statistics Video Node
> > +----------------------------------
> > +
> > +The viif_stats video node provides current status of ISP.
> > +
> > +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'll fix it.
> Out of curiosity, is there no statistics for auto-focus ?
>
The VIIF HW does not support auto-focus.
It supports only auto-exposure and auto-white-balance.
> > +
> > +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.
> > +
> > +Capturing Video Frames Example
> > +==============================
> > +
> > +In the following example,
> > +imx219 camera is connected to pad 0 of 'visconti-viif:isp'.
> > +
> > +The following commands can be used to capture video from the main
> > +path 0 with dimension 1024x1024 and RGB format.
> > +
> > +.. 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-viif:csi2rx":0 [1]'
> > + media-ctl -d platform:visconti-viif-0 -l '"visconti-viif:csi2rx":1 ->
> "visconti-viif:isp":0 [1]'
> > + media-ctl -d platform:visconti-viif-0 -l '"visconti-viif:isp":1 ->
> "viif_capture_post0":0 [1]'
> > + media-ctl -d platform:visconti-viif-0 -l '"visconti-viif:isp":2 ->
> "viif_capture_post1":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 '"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-viif:csi2rx":0
> [fmt:SRGGB10_1X10/1920x1080]'
> > +
> > + # set format for isp sink pad
> > + media-ctl -d platform:visconti-viif-0 --set-v4l2 '"visconti-viif:isp":0
> [fmt:SRGGB10_1X10/1920x1080 compose:(0,0)/1920x1080]'
> > +
> > + # set format for isp source pad
> > + 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=1024,height=1024"
> > + 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 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 c6e56b5888..23fa0ebe45 100644
> > --- a/Documentation/userspace-api/media/v4l/meta-formats.rst
> > +++ b/Documentation/userspace-api/media/v4l/meta-formats.rst
> > @@ -18,6 +18,7 @@ These formats are used for the :ref:`metadata` interface
> only.
> > metafmt-pisp-be
> > 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 0000000000..dc4b31627f
> > --- /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
>
> --
> Regards,
>
> Laurent Pinchart
Regards,
Yuji Ishikawa
