Re: [PATCH v8 15/17] media: v4l: Add Intel IPU3 meta buffer formats

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello Yong,

Thank you for the patch.

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@intel.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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.

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