| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<TY3PR01MB9982FC3EF15A3307E752616592F72@TY3PR01MB9982.jpnprd01.prod.outlook.com>
Date: Wed, 5 Feb 2025 12:29:56 +0000
From: <yuji2.ishikawa@...hiba.co.jp>
To: <laurent.pinchart@...asonboard.com>
CC: <mchehab@...nel.org>, <robh@...nel.org>, <krzk+dt@...nel.org>,
<conor+dt@...nel.org>, <sakari.ailus@...ux.intel.com>,
<hverkuil-cisco@...all.nl>, <nobuhiro1.iwamatsu@...hiba.co.jp>,
<linux-media@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <devicetree@...r.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.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Sent: Friday, January 3, 2025 6:26 AM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@...hiba.co.jp>
> Cc: Mauro Carvalho Chehab <mchehab@...nel.org>; Rob Herring
> <robh@...nel.org>; Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor Dooley
> <conor+dt@...nel.org>; Sakari Ailus <sakari.ailus@...ux.intel.com>; Hans
> Verkuil <hverkuil-cisco@...all.nl>; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ
> □ＤＩＴ○ＯＳＴ) <nobuhiro1.iwamatsu@...hiba.co.jp>;
> linux-media@...r.kernel.org; linux-kernel@...r.kernel.org;
> linux-arm-kernel@...ts.infradead.org; devicetree@...r.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:
> > Added description of Video Input Interface driver of
>
> s/Added/Add/
>
I'll fix it
> > 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@...hiba.co.jp>
> > ---
> > 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
> ++++++++++++++++++
> > .../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]
> > + 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]
> > + n00000008:port1 -> n0000000e
> > + n0000000b [label="{{<port0> 0} |
> visconti-viif:resizer1\n/dev/v4l-subdev2 | {<port1> 1}}", shape=Mrecord,
> style=filled, fillcolor=green]
> > + n0000000b:port1 -> n00000012
> > + n0000000e [label="viif_capture_post0\n/dev/video0", shape=box,
> style=filled, fillcolor=yellow]
> > + n00000012 [label="viif_capture_post1\n/dev/video1", shape=box,
> style=filled, fillcolor=yellow]
> > + n00000016 [label="viif_capture_sub\n/dev/video2", shape=box,
> style=filled, fillcolor=yellow]
> > + n0000001a [label="viif_params\n/dev/video3", shape=box,
> style=filled, fillcolor=yellow]
> > + n0000001a -> n00000001:port4
> > + n0000001e [label="viif_stats\n/dev/video4", shape=box,
> style=filled, fillcolor=yellow]
> > + n00000030 [label="{{<port0> 0} | visconti_csi2rx
> 1c008000.csi2rx\n/dev/v4l-subdev3 | {<port1> 1}}", shape=Mrecord,
> style=filled, fillcolor=green]
> > + n00000030:port1 -> n00000001:port0
> > + n00000035 [label="{{} | imx219 1-0010\n/dev/v4l-subdev4 |
> {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
> > + 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
> > +
> >
> +================================================
> ======
> > +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.
>
> 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.
> > +
> > +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.
> > +
> > +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"
> > + "RGB
> with scale 0"
> > + +--------+ +----------+ +-----+ +-----+ +-----+
> +--------+
> > + | Sensor |--->| CSI2RX |--->| | | | | |--->|
> memory |
> > + +--------+ +----------+ | | | | | |
> +--------+
> > + | | | L1 | | L2 | "POST1"
> > + | |--->| ISP |--->| ISP | "RGB with
> scale 1"
> > + | | | | | |
> +--------+
> > + | MUX | | | | |--->|
> memory |
> > + | | +-----+ +-----+
> +--------+
> > + | | "SUB"
> > + | | "RAW
> w/o scale"
> > + | | +------------+
> +--------+
> > + | |------> | Video DMAC |--->|
> memory |
> > + +-----+ +------------+
> +--------+
> > +
> > +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.
> > +
> > +- 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.
> > +
> > +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.
> > +applies multiple operations on pictures, then passes resulting images to
> capture nodes.
> > +
> > +ISP configurations/parameters are passed from userland via viif_params
> node.
> > +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.
> > +
> > +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`
> > + - 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: 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`
> > + - 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.
> > + 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.
> > + - 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`
> > +
> > +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.
> > +
> > + L1ISP::INPUT::preprocess
> > +
> > + +-----+
> +-----+
> > + | 24b | +------+ +------+ +------+ +------+
> +------+ | 24b |
> > + | RAW |--->| SLIC |--->| ABPC |--->| PWHB |--->| RCNR |--->| HDRS
> |--->| RAW |
> > + | (0) | +------+ +------+ +------+ +------+ +------+
> | (1) |
> > + +-----+
> +-----+
> > +
> > + L1ISP::MainProcess(MPRO)
> > +
> > + +-----+
> > + | 24b | +------+ +------+
> > + | RAW |--->| BLVC |--->| LSSC |---+
> > + | (1) | +------+ +------+ |
> > + +-----+ |
> > + |
> > + +------------------------------+
> > + |
> > + | +-----------+ +-------------+ +--------+
> +-----+
> > + +--->| MPRO | | MPRO | | MPRO |
> +------+ | 16b |
> > + | Digital |--->| Demosaicing |----| Color |--->| HDRC
> |--->| RGB |
> > + +--->| Amplifier | | | | Matrix | +------+
> | |
> > + | +-----------+ +-------------+ +--------+
> +-----+
> > + | | |
> > + | +--------------+ | | +------+
> > + +----| Auto |<----+ +--->| AEXP |---> Auto-Exposure
> statistics
> > + | Whitebalance | +------+
> > + +--------------+
> > + |
> > + +------------------------------> Auto-Whitebalance
> > + statistics
> > +
> > + L1ISP::VideoProcess(VPRO)
> > +
> > + +-----+ +------------+ +------------+ +---------------+
> +--------+
> > + | 16b |--->| Gamma |--->| RGB2YUV |--->| Image Quality
> |--->| Output |
> > + | RGB | | Correction | | Conversion | | Adjustment | |
> 16b |
> > + | | +------------+ +------------+ +---------------+
> | YUV |
> > + +-----+
> +--------+
> > +
> > + L1ISP::AnalogGain
> > +
> > + statistics +-------------+
> +------------------+
> > + information ---> (user SW) --->| Analog Gain |--->| ABPC, RCNR, LSSC
> |
> > + +-------------+ | MPRO,
> VPRO |
> > +
> > + +------------------+
>
> 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."
> > +
> > +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.
> > + - Scaling; see :c:type:`viif_l2_roi`
>
> viif_l2_roi_config
>
I'll fix it too.
> > + - 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.
> > + - 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 |
+---------+ +------------+
> > +
> |
> > + +----------------------------------------------------------+
> > + |
> > + | +----------+ +------------+ +--------+
> +--------+
> > + +--->|Gamma |--->| Colorspace |--->| Data |--->|
> Output |
> > + | |Correction| | Conversion | | Packer | | Image |
> > + | +----------+ +------------+ +--------+
> +--------+
> > + |
> > + | +----------+ +------------+ +--------+
> +--------+
> > + +--->|Gamma |--->| Colorspace |--->| Data |--->|
> Output |
> > + |Correction| | Conversion | | Packer | | Image |
> > + +----------+ +------------+ +--------+
> +--------+
> > +
> > +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.
> > +
> > +- 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.
> > +- 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.
> > +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.
> > +
> > +.. _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?
> > +
> > +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
Powered by blists - more mailing lists