Hello Yong,
On Tuesday, 11 December 2018 14:58:50 EET Laurent Pinchart wrote:
> On Friday, 7 December 2018 03:03:40 EET Yong Zhi wrote:
> > Add IPU3-specific meta formats for processing parameters and
> > 3A statistics.
> >
> > V4L2_META_FMT_IPU3_PARAMS
> > V4L2_META_FMT_IPU3_STAT_3A
> >
> > Signed-off-by: Yong Zhi <yong.zhi@xxxxxxxxx>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> My Reviewed-by tag was related to the format part only (v4l2-ioctl.c and
> videodev2.h) :-) Please see below for more comments about the documentation.
The IPU3 patch series has been merged without addressing the review comments
below. Could you please go through them and submit a patch ?
The IPU3 documentation needs a fair amount of work to make it usable. This is
blocking review and development of the driver itself, as we can't advice on
how APIs should be used (and possibly extended) without knowing how the device
operates. In particular we need a more detailed description of each processing
block, including how many lines and columns they crop from the image they
process, and an explanation of how to compute the various formats and
selection rectangles based on the input resolution, desired output resolution,
and configuration of the processing blocks. Particular attention should be
given to documentation of the scalers (I know about two of them, the Bayer
downscaler towards the input of the pipeline, and the YUV scaler on the
viewfinder). How the YUV scaler integrates with the rest of the GDC is not
clear from the documentation, and that should be documented too.
> > ---
> >
> > Documentation/media/uapi/v4l/meta-formats.rst | 1 +
> > .../media/uapi/v4l/pixfmt-meta-intel-ipu3.rst | 178 ++++++++++++++++
> > drivers/media/v4l2-core/v4l2-ioctl.c | 2 +
> > include/uapi/linux/videodev2.h | 4 +
> > 4 files changed, 185 insertions(+)
> > create mode 100644
> > Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> >
> > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst
> > b/Documentation/media/uapi/v4l/meta-formats.rst index
> > 438bd244bd2f..5f956fa784b7 100644
> > --- a/Documentation/media/uapi/v4l/meta-formats.rst
> > +++ b/Documentation/media/uapi/v4l/meta-formats.rst
> > @@ -19,6 +19,7 @@ These formats are used for the :ref:`metadata` interface
> > only.
> >
> > .. toctree::
> > :maxdepth: 1
> >
> > + pixfmt-meta-intel-ipu3
> >
> > pixfmt-meta-d4xx
> > pixfmt-meta-uvc
> > pixfmt-meta-vsp1-hgo
>
> Please keep this list alphabetically sorted.
>
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst new file mode
> > 100644
> > index 000000000000..8cd30ffbf8b8
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > @@ -0,0 +1,178 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _v4l2-meta-fmt-params:
> > +.. _v4l2-meta-fmt-stat-3a:
> > +
> > +******************************************************************
> > +V4L2_META_FMT_IPU3_PARAMS ('ip3p'), V4L2_META_FMT_IPU3_3A ('ip3s')
> > +******************************************************************
> > +
> > +.. c:type:: ipu3_uapi_stats_3a
>
> No need for c:type:: here, the structure is already properly defined in
> drivers/staging/media/ipu3/include/intel-ipu3.h
>
> > +3A statistics
> > +=============
> > +
> > +For IPU3 ImgU, the 3A statistics accelerators collect different
> > statistics
>
> I'd write "The IPU3 ImgU 3A statistics accelerators collect" or "The IPU3
> ImgU includes 3A statistics accelerators that collect"
>
> > over +an input bayer frame. Those statistics, defined in data struct
>
> bayer should be spelled Bayer (here and below).
>
> > :c:type:`ipu3_uapi_stats_3a`, +are obtained from "ipu3-imgu 3a stat"
> >
> > metadata capture video node, which are then +passed to user space for
> > statistics analysis using :c:type:`v4l2_meta_format` interface.
>
> How about simply
>
> "Those statistics are obtained from the "ipu3-imgu [01] 3a stat" metadata
> capture video nodes, using the :c:type:`v4l2_meta_format` interface. They
> are formatted as described by the :c:type:`ipu3_uapi_stats_3a` structure."
> > +
> > +The statistics collected are AWB (Auto-white balance) RGBS (Red, Green,
> > Blue and +Saturation measure) cells, AWB filter response, AF (Auto-focus)
> > filter response, +and AE (Auto-exposure) histogram.
>
> Could you please wrap lines at the 80 columns boundary ?
>
> > +struct :c:type:`ipu3_uapi_4a_config` saves configurable parameters for
> > all
> > above.
>
> I would write it as "The
>
> By the way why "4a" when the documentation talks about 3A ? Shouldn't the
> structure be called ipu3_uapi_3a_config ?
>
> > +
> > +.. code-block:: c
> > +
> > + struct ipu3_uapi_stats_3a {
> > + struct ipu3_uapi_awb_raw_buffer awb_raw_buffer;
> > + struct ipu3_uapi_ae_raw_buffer_aligned
> > ae_raw_buffer[IPU3_UAPI_MAX_STRIPES];
> > + struct ipu3_uapi_af_raw_buffer
> > af_raw_buffer;
> > + struct ipu3_uapi_awb_fr_raw_buffer awb_fr_raw_buffer;
> > + struct ipu3_uapi_4a_config stats_4a_config;
> > + __u32 ae_join_buffers;
> > + __u8 padding[28];
> > + struct ipu3_uapi_stats_3a_bubble_info_per_stripe
> > stats_3a_bubble_per_stripe;
> > + struct ipu3_uapi_ff_status stats_3a_status;
> > + };
> >
> > +.. c:type:: ipu3_uapi_params
>
> No need for c:type:: here either.
>
> > +Pipeline parameters
> > +===================
> > +
> > +IPU3 pipeline has a number of image processing stages, each of which
> > takes
>
> s/IPU3/The IPU3/
>
> > a +set of parameters as input. The major stages of pipelines are shown
> > here:
> > +
> > +Raw pixels -> Bayer Downscaling -> Optical Black Correction ->
> > +
> > +Linearization -> Lens Shading Correction -> White Balance / Exposure /
> > +
> > +Focus Apply -> Bayer Noise Reduction -> ANR -> Demosaicing -> Color
> > +
> > +Correction Matrix -> Gamma correction -> Color Space Conversion ->
> > +
> > +Chroma Down Scaling -> Chromatic Noise Reduction -> Total Color
> > +
> > +Correction -> XNR3 -> TNR -> DDR
>
> You can replace this list with
>
> .. kernel-render:: DOT
>
> :alt: IPU3 ImgU Pipeline
> :caption: IPU3 ImgU Pipeline Diagram
>
> digraph "IPU3 ImgU" {
> node [shape=box]
> splines="ortho"
> rankdir="LR"
>
> a [label="Raw pixels"]
> b [label="Bayer Downscaling"]
> c [label="Optical Black Correction"]
> d [label="Linearization"]
> e [label="Lens Shading Correction"]
> f [label="White Balance / Exposure / Focus Apply"]
> g [label="Bayer Noise Reduction"]
> h [label="ANR"]
> i [label="Demosaicing"]
> j [label="Color Correction Matrix"]
> k [label="Gamma correction"]
> l [label="Color Space Conversion"]
> m [label="Chroma Down Scaling"]
> n [label="Chromatic Noise Reduction"]
> o [label="Total Color Correction"]
> p [label="XNR3"]
> q [label="TNR"]
> r [label="DDR"]
>
> { rank=same; a -> b -> c -> d -> e -> f }
> { rank=same; g -> h -> i -> j -> k -> l }
> { rank=same; m -> n -> o -> p -> q -> r }
>
> a -> g -> m [style=invis, weight=10]
>
> f -> g
> l -> m
> }
>
> to get a nicer diagram.
>
> > +The table below presents a description of the above algorithms.
> > +
> > +========================
> > =======================================================
> > +Name Description
> > +========================
> > =======================================================
> > +Optical Black
> > Correction Optical Black Correction block subtracts a pre-defined +
> > value from the respective pixel values to obtain better
> > + image quality.
> > + Defined in :c:type:`ipu3_uapi_obgrid_param`.
> > +Linearization This algo block uses linearization parameters to
> > + address non-linearity sensor effects. The Lookup table
> > + table is defined in
> > + :c:type:`ipu3_uapi_isp_lin_vmem_params`.
> > +SHD Lens shading correction is used to correct spatial
> > + non-uniformity of the pixel response due to optical
> > + lens shading. This is done by applying a different gain
> > + for each pixel. The gain, black level etc are
> > + configured in :c:type:`ipu3_uapi_shd_config_static`.
> > +BNR Bayer noise reduction block removes image noise by
> > + applying a bilateral filter.
> > + See :c:type:`ipu3_uapi_bnr_static_config` for details.
> > +ANR Advanced Noise Reduction is a block based algorithm
> > + that performs noise reduction in the Bayer domain. The
> > + convolution matrix etc can be found in
> > + :c:type:`ipu3_uapi_anr_config`.
> > +Demosaicing Demosaicing converts raw sensor data in Bayer format
> > + into RGB (Red, Green, Blue) presentation. Then add
> > + outputs of estimation of Y channel for following stream
> > + processing by Firmware. The struct is defined as
> > + :c:type:`ipu3_uapi_dm_config`. (TODO)
> > +Color Correction Color Correction algo transforms sensor specific
color
> > + space to the standard "sRGB" color space. This is done
> > + by applying 3x3 matrix defined in
> > + :c:type:`ipu3_uapi_ccm_mat_config`.
> > +Gamma correction Gamma correction :c:type:`ipu3_uapi_gamma_config` is
a
> > + basic non-linear tone mapping correction that is
> > + applied per pixel for each pixel component.
> > +CSC Color space conversion transforms each pixel from the
> > + RGB primary presentation to YUV (Y: brightness,
> > + UV: Luminance) presentation. This is done by applying
> > + a 3x3 matrix defined in
> > + :c:type:`ipu3_uapi_csc_mat_config`
> > +CDS Chroma down sampling
> > + After the CSC is performed, the Chroma Down Sampling
> > + is applied for a UV plane down sampling by a factor
> > + of 2 in each direction for YUV 4:2:0 using a 4x2
> > + configurable filter :c:type:`ipu3_uapi_cds_params`.
> > +CHNR Chroma noise reduction
> > + This block processes only the chrominance pixels and
> > + performs noise reduction by cleaning the high
> > + frequency noise.
> > + See struct :c:type:`ipu3_uapi_yuvp1_chnr_config`.
> > +TCC Total color correction as defined in struct
> > + :c:type:`ipu3_uapi_yuvp2_tcc_static_config`.
> > +XNR3 eXtreme Noise Reduction V3 is the third revision of
> > + noise reduction algorithm used to improve image
> > + quality. This removes the low frequency noise in the
> > + captured image. Two related structs are being defined,
> > + :c:type:`ipu3_uapi_isp_xnr3_params` for ISP data memory
> > + and :c:type:`ipu3_uapi_isp_xnr3_vmem_params` for vector
> > + memory.
> > +TNR Temporal Noise Reduction block compares successive
> > + frames in time to remove anomalies / noise in pixel
> > + values. :c:type:`ipu3_uapi_isp_tnr3_vmem_params` and
> > + :c:type:`ipu3_uapi_isp_tnr3_params` are defined for ISP
> > + vector and data memory respectively.
> > +========================
> > =======================================================
> > +
> > +A few stages of the pipeline will be executed by firmware running on the
> > ISP +processor, while many others will use a set of fixed hardware blocks
> > also +called accelerator cluster (ACC) to crunch pixel data and produce
> > statistics.
> > +
> > +ACC parameters of individual algorithms, as defined by
> > +:c:type:`ipu3_uapi_acc_param`, can be chosen to be applied by the user
> > +space through struct :c:type:`ipu3_uapi_flags` embedded in
> > +:c:type:`ipu3_uapi_params` structure. For parameters that are configured
> > as +not enabled by the user space, the corresponding structs are ignored
> > by the +driver, in which case the existing configuration of the algorithm
> > will be +preserved.
> > +
> > +Both 3A statistics and pipeline parameters described here are closely
> > tied
> > to +the underlying camera sub-system (CSS) APIs. They are usually consumed
> > and +produced by dedicated user space libraries that comprise the
> > important
> > tuning +tools, thus freeing the developers from being bothered with the
> > low
> > level +hardware and algorithm details.
> > +
> > +It should be noted that IPU3 DMA operations require the addresses of all
> > data +structures (that includes both input and output) to be aligned on 32
> > byte +boundaries.
>
> I think most of the above (from the diagram to here) belongs to
> Documentation/ media/v4l-drivers/ipu3.rst. It can be referenced here, but
> this file should focus on the description of the metadata formats, not on
> the description of the IPU3 ImgU internals.
>
> > +The meta data :c:type:`ipu3_uapi_params` will be sent to "ipu3-imgu
> > parameters" +video node in ``V4L2_BUF_TYPE_META_CAPTURE`` format.
>
> To be consistent with the statistics documentation, how about the following
> ?
>
> "The pipeline parameters are passed to the "ipu3-imgu [01] parameters"
> metadata output video nodes, using the :c:type:`v4l2_meta_format` interface.
> They are formatted as described by the :c:type:`ipu3_uapi_params`
> structure."
> > +.. code-block:: c
> > +
> > + struct ipu3_uapi_params {
> > + /* Flags which of the settings below are to be applied */
> > + struct ipu3_uapi_flags use;
> > +
> > + /* Accelerator cluster parameters */
> > + struct ipu3_uapi_acc_param acc_param;
> > +
> > + /* ISP vector address space parameters */
> > + struct ipu3_uapi_isp_lin_vmem_params lin_vmem_params;
> > + struct ipu3_uapi_isp_tnr3_vmem_params tnr3_vmem_params;
> > + struct ipu3_uapi_isp_xnr3_vmem_params xnr3_vmem_params;
> > +
> > + /* ISP data memory (DMEM) parameters */
> > + struct ipu3_uapi_isp_tnr3_params tnr3_dmem_params;
> > + struct ipu3_uapi_isp_xnr3_params xnr3_dmem_params;
> > +
> > + /* Optical black level compensation */
> > + struct ipu3_uapi_obgrid_param obgrid_param;
> > + };
> > +
> > +Intel IPU3 ImgU uAPI data types
> > +===============================
> > +
> > +.. kernel-doc:: include/uapi/linux/intel-ipu3.h
>
> This file has moved to drivers/staging/media/ipu3/include/intel-ipu3.h.
>
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> > b/drivers/media/v4l2-core/v4l2-ioctl.c index a1806d3a1c41..0701cb8a03ef
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -1300,6 +1300,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc
> > *fmt) case V4L2_META_FMT_VSP1_HGO: descr = "R-Car VSP1 1-D Histogram";
> > break; case V4L2_META_FMT_VSP1_HGT: descr = "R-Car VSP1 2-D Histogram";
> > break; case V4L2_META_FMT_UVC: descr = "UVC payload header metadata";
> > break; + case V4L2_META_FMT_IPU3_PARAMS: descr = "IPU3 processing
> > parameters"; break; + case V4L2_META_FMT_IPU3_STAT_3A: descr = "IPU3 3A
> > statistics"; break;
> >
> > default:
> > /* Compressed formats */
> >
> > diff --git a/include/uapi/linux/videodev2.h
> > b/include/uapi/linux/videodev2.h index a9d47b1b9437..f2b973b36e29 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -721,6 +721,10 @@ struct v4l2_pix_format {
> >
> > #define V4L2_META_FMT_UVC v4l2_fourcc('U', 'V', 'C', 'H') /* UVC
> >
> > Payload Header metadata */ #define V4L2_META_FMT_D4XX
> > v4l2_fourcc('D', '4', 'X', 'X') /* D4XX Payload Header metadata */
> >
> > +/* Vendor specific - used for IPU3 camera sub-system */
> > +#define V4L2_META_FMT_IPU3_PARAMS v4l2_fourcc('i', 'p', '3', 'p') /*
IPU3
> > processing parameters */ +#define
> > V4L2_META_FMT_IPU3_STAT_3A v4l2_fourcc('i', 'p', '3', 's') /* IPU3 3A
> > statistics */ +
> >
> > /* priv field value to indicates that subsequent fields are valid. */
> > #define V4L2_PIX_FMT_PRIV_MAGIC 0xfeedcafe
--
Regards,
Laurent Pinchart
