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

Commit Message

Zhi, Yong Dec. 7, 2018, 1:03 a.m. UTC

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 <[e-mail]>
Reviewed-by: Laurent Pinchart <[e-mail]>
---
 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

Comments

Laurent Pinchart Dec. 11, 2018, 12:58 p.m. UTC | #1

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 <[e-mail]>
> Reviewed-by: Laurent Pinchart <[e-mail]>

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

Laurent Pinchart Jan. 10, 2019, 12:29 p.m. UTC | #2

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 <[e-mail]>
> > Reviewed-by: Laurent Pinchart <[e-mail]>
> 
> 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

Zhi, Yong Jan. 10, 2019, 6:35 p.m. UTC | #3

Hi, Laurent,

Thanks for the review.

> -----Original Message-----
> From: Laurent Pinchart [mailto:[e-mail]]
> Sent: Tuesday, December 11, 2018 6:59 AM
> To: Zhi, Yong <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; Mani, Rajmohan <[e-mail]>;
> Toivonen, Tuukka <[e-mail]>; Hu, Jerry W
> <[e-mail]>; Qiu, Tian Shu <[e-mail]>;
> [e-mail]; [e-mail]; Cao, Bingbu
> <[e-mail]>; Zheng, Jian Xu <[e-mail]>
> Subject: Re: [PATCH v8 15/17] media: v4l: Add Intel IPU3 meta buffer formats
> 
> 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 <[e-mail]>
> > Reviewed-by: Laurent Pinchart <[e-mail]>
> 
> 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.
> 

Ack.

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

Ack.

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

Thanks for refining and improving the text :)

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

Ack.

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

The 4th "a" refers to the AWB filter response 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.
> 

Ack.

> > +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 generated block diagram looks better indeed, thanks for sharing!!

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

Ack, will move block diagram to /media/v4l-drivers/ipu3.rst as suggested. I prefer to leave the above short description here though.

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

Sure, thanks!

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

Ack, Sakari has already taken care of this one.

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

Laurent Pinchart Jan. 22, 2019, 9:22 p.m. UTC | #4

Hi Yong,

On Thu, Jan 10, 2019 at 06:35:11PM +0000, Zhi, Yong wrote:
> On Tuesday, December 11, 2018 6:59 AM, 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 <[e-mail]>
> >> Reviewed-by: Laurent Pinchart <[e-mail]>
> > 
> > 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

[snip]

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

[snip]

> >> +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 ?
> > 
> 
> The 4th "a" refers to the AWB filter response config.

But the automatic algorithms are still automatic white balance,
automatic exposure and automatic focus, right, with
ipu3_uapi_awb_fr_raw_buffer being part of AWB, right ?

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

[snip]

Zhi, Yong Jan. 22, 2019, 9:46 p.m. UTC | #5

Hi, Laurent,

> -----Original Message-----
> From: Laurent Pinchart [mailto:[e-mail]]
> Sent: Tuesday, January 22, 2019 1:22 PM
> To: Zhi, Yong <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; Mani, Rajmohan <[e-mail]>;
> Toivonen, Tuukka <[e-mail]>; Hu, Jerry W
> <[e-mail]>; Qiu, Tian Shu <[e-mail]>;
> [e-mail]; [e-mail]; Cao, Bingbu
> <[e-mail]>
> Subject: Re: [PATCH v8 15/17] media: v4l: Add Intel IPU3 meta buffer formats
> 
> Hi Yong,
> 
> On Thu, Jan 10, 2019 at 06:35:11PM +0000, Zhi, Yong wrote:
> > On Tuesday, December 11, 2018 6:59 AM, 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 <[e-mail]>
> > >> Reviewed-by: Laurent Pinchart <[e-mail]>
> > >
> > > 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
> 
> [snip]
> 
> > >> 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
> 
> [snip]
> 
> > >> +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 ?
> > >
> >
> > The 4th "a" refers to the AWB filter response config.
> 
> But the automatic algorithms are still automatic white balance, automatic
> exposure and automatic focus, right, with ipu3_uapi_awb_fr_raw_buffer
> being part of AWB, right ?
> 

That's right, we still call it ipu3_uapi_stats_3a instead of ipu3_uapi_stats_4a.
I have no problem to rename ipu3_uapi_4a_config to ipu3_uapi_3a_config.

I can resend the patch with this update if no one opposes, thanks!!

> > >> +
> > >> +.. 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
> 
> [snip]
> 
> --
> Regards,
> 
> Laurent Pinchart

Patch

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
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
+
+3A statistics
+=============
+
+For IPU3 ImgU, the 3A statistics accelerators collect different statistics over
+an input bayer frame. Those statistics, defined in data struct :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.
+
+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.
+
+struct :c:type:`ipu3_uapi_4a_config` saves configurable parameters for all above.
+
+.. 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
+
+Pipeline parameters
+===================
+
+IPU3 pipeline has a number of image processing stages, each of which takes 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
+
+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.
+
+The meta data :c:type:`ipu3_uapi_params` will be sent to "ipu3-imgu parameters"
+video node in ``V4L2_BUF_TYPE_META_CAPTURE`` format.
+
+.. 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
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