Hi Hans, Thanks for your time and reviews. > -----Original Message----- > From: Hans Verkuil [mailto:hverkuil@xxxxxxxxx] > Sent: Monday, August 13, 2018 6:43 AM > To: Zhi, Yong <yong.zhi@xxxxxxxxx>; sakari.ailus@xxxxxxxxxxxxxxx; linux- > media@xxxxxxxxxxxxxxx > Cc: tfiga@xxxxxxxxxxxx; mchehab@xxxxxxxxxx; hans.verkuil@xxxxxxxxx; > laurent.pinchart@xxxxxxxxxxxxxxxx; Mani, Rajmohan > <rajmohan.mani@xxxxxxxxx>; Zheng, Jian Xu <jian.xu.zheng@xxxxxxxxx>; Hu, > Jerry W <jerry.w.hu@xxxxxxxxx>; Li, Chao C <chao.c.li@xxxxxxxxx>; Qiu, Tian > Shu <tian.shu.qiu@xxxxxxxxx> > Subject: Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI > > On 15/06/18 05:29, Yong Zhi wrote: > > These meta formats are used on Intel IPU3 ImgU video queues > > to carry 3A statistics and ISP pipeline parameters. > > > > V4L2_META_FMT_IPU3_3A > > V4L2_META_FMT_IPU3_PARAMS > > > > Signed-off-by: Yong Zhi <yong.zhi@xxxxxxxxx> > > Signed-off-by: Chao C Li <chao.c.li@xxxxxxxxx> > > Signed-off-by: Rajmohan Mani <rajmohan.mani@xxxxxxxxx> > > --- > > Documentation/media/uapi/v4l/meta-formats.rst | 1 + > > .../media/uapi/v4l/pixfmt-meta-intel-ipu3.rst | 174 ++ > > include/uapi/linux/intel-ipu3.h | 2816 ++++++++++++++++++++ > > 3 files changed, 2991 insertions(+) > > create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-intel- > ipu3.rst > > create mode 100644 include/uapi/linux/intel-ipu3.h > > > > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst > b/Documentation/media/uapi/v4l/meta-formats.rst > > index 0c4e1ec..b887fca 100644 > > --- a/Documentation/media/uapi/v4l/meta-formats.rst > > +++ b/Documentation/media/uapi/v4l/meta-formats.rst > > @@ -12,6 +12,7 @@ These formats are used for the :ref:`metadata` interface > only. > > .. toctree:: > > :maxdepth: 1 > > > > + pixfmt-meta-intel-ipu3 > > pixfmt-meta-uvc > > pixfmt-meta-vsp1-hgo > > pixfmt-meta-vsp1-hgt > > 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 0000000..5c050e6 > > --- /dev/null > > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst > > @@ -0,0 +1,174 @@ > > +.. -*- coding: utf-8; mode: rst -*- > > + > > +.. _intel-ipu3: > > + > > > +*************************************************************** > *** > > +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 meta output obtained from "ipu3-imgu 3a > stat" > > +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 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 { > > + IPU3_ALIGN struct ipu3_uapi_awb_raw_buffer awb_raw_buffer; > > IPU3_ALIGN? What's that? > > OK, after reading the header I see what it does, but I think you should > drop it in the documentation since it doesn't help the reader. > Ack [snip] > > +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 anamolies / noise in pixel > > anamolies -> anomalies > Ack > > + 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. > > Nice overview of all the blocks! > Thanks. > > +======================== > =======================================================> + > > +A few stages of the pipe will be executed by firmware running on the ISP > > pipe -> pipeline > Ack [snip] > > + > > +#define IPU3_UAPI_ISP_WORD_BYTES 32 > > +#define IMGU_ABI_PAD __aligned(IPU3_UAPI_ISP_WORD_BYTES) > > +#define IPU3_ALIGN > __attribute__((aligned(IPU3_UAPI_ISP_WORD_BYTES))) > > That's weird. Isn't __aligned identical to __attribute__((aligned( ? > If so, then these last two defines are also identical. > Ack > Why is it aligned to a 32 bytes? Is it a hardware requirement? > IPU3 DMA operations require the addresses to be aligned on 32 byte boundaries. > > + > > +/******************* ipu3_uapi_stats_3a *******************/ > > + > > +#define IPU3_UAPI_MAX_STRIPES 2 > > +#define IPU3_UAPI_MAX_BUBBLE_SIZE 10 [snip] > > +struct ipu3_uapi_ae_grid_config { > > + __u8 width; > > + __u8 height; > > + __u8 block_width_log2:4; > > + __u8 block_height_log2:4; > > + __u8 __reserved0:5; > > Why add a reserved bitfield? What is its purpose? > Currently this (5 bits) bit field does not have any purpose and hence it's marked as a reserved field. > > + __u8 ae_en:1; > > + __u8 rst_hist_array:1; > > + __u8 done_rst_hist_array:1; > > + __u16 x_start; > > + __u16 y_start; > > + __u16 x_end; > > + __u16 y_end; > > +} __packed; > > + [snip] > > +struct ipu3_uapi_af_meta_data { > > + __u8 y_table[IPU3_UAPI_AF_Y_TABLE_MAX_SIZE] IPU3_ALIGN; > > Here IPU3_ALIGN is put at the end... > > > +} __packed; > > + > > +/** > > + * struct ipu3_uapi_af_raw_buffer - AF raw buffer > > + * > > + * @meta_data: raw buffer &ipu3_uapi_af_meta_data for auto focus meta > data. > > + */ > > +struct ipu3_uapi_af_raw_buffer { > > + IPU3_ALIGN struct ipu3_uapi_af_meta_data meta_data; > > ... and here at the start. Is that due to the difference between an array and a > struct? > No. When preparing uAPI kernelDoc using "make htmldocs", the kernel-doc encounters two type of error/warnings caused by IPU3_ALIGN. case 1: struct IPU3_ALIGN ipu3_uapi_dummy { ... } __packed; "error: Cannot parse struct or union!" case 2: struct ipu3_uapi_dummy { struct ipu3_uapi_x x IPU3_ALIGN; } __packed; "warning: Function parameter or member 'IPU3_ALIGN' not described in 'ipu3_uapi_dummy'" Positioned the attribute syntax without altering the mem layout of the structs, while also making "make htmldocs" to compile fine. Let us know if it's okay to ignore Sphinx warnings. > > +} __packed; > > + > > +/** > > + * struct ipu3_uapi_af_config_s - AF config > > + * > > + * @filter_config: AF uses Y1 and Y2 filters as configured in > > + * &ipu3_uapi_af_filter_config > > + * @padding: paddings > > + * @grid_cfg: See &ipu3_uapi_grid_config, default resolution 16x16. Use > large > > + * grid size for large image and vice versa. > > + */ > > +struct ipu3_uapi_af_config_s { > > + IPU3_ALIGN struct ipu3_uapi_af_filter_config filter_config; > > + __u8 padding[4]; > > Is the padding still needed with the IPU3_ALIGNs? > Yes. There were some initial review comments around this to make sure the size and layout of all these structs remain the same with and without "__packed" attribute. Hence there is a need to have this "padding" fields to ensure the same. > > + IPU3_ALIGN struct ipu3_uapi_grid_config grid_cfg; > > +} __packed; > > + > > +/** > > + * struct ipu3_uapi_af_config - AF config wrapper > > + * > > + * @config: config for auto focus as defined by &ipu3_uapi_af_config_s > > + */ > > +struct ipu3_uapi_af_config { > > + struct ipu3_uapi_af_config_s config; > > +} __packed; > > + > > +#define IPU3_UAPI_AWB_FR_MAX_SETS 24 > > +#define IPU3_UAPI_AWB_FR_MD_ITEM_SIZE 8 > > +#define IPU3_UAPI_AWB_FR_BAYER_TBL_SIZE 0x100 > > +#define IPU3_UAPI_AWB_FR_SPARE_FOR_BUBBLES \ > > + (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \ > > + IPU3_UAPI_AWB_FR_MD_ITEM_SIZE) > > +#define IPU3_UAPI_AWB_FR_BAYER_TABLE_MAX_SIZE \ > > + (IPU3_UAPI_AWB_FR_MAX_SETS * \ > > + (IPU3_UAPI_AWB_FR_BAYER_TBL_SIZE + \ > > + IPU3_UAPI_AWB_FR_SPARE_FOR_BUBBLES) * > IPU3_UAPI_MAX_STRIPES) > > Add newline > Ack [snip] > > +/** > > + * struct ipu3_uapi_4a_config - 4A config > > + * > > + * @awb_config: &ipu3_uapi_awb_config_s, default resolution 16x16 > > + * @ae_grd_config: auto exposure statistics &ipu3_uapi_ae_grid_config > > + * @padding: paddings > > + * @af_config: auto focus config &ipu3_uapi_af_config_s > > + * @awb_fr_config: &ipu3_uapi_awb_fr_config_s, default resolution 16x16 > > + */ > > +struct ipu3_uapi_4a_config { > > + IPU3_ALIGN struct ipu3_uapi_awb_config_s awb_config; > > + struct ipu3_uapi_ae_grid_config ae_grd_config; > > + __u8 padding[20]; > > Can 'padding' be replaced by IPU3_ALIGN? > No. This is required to maintain the struct size the same with and without __packed attribute. > > + struct ipu3_uapi_af_config_s af_config; > > + struct ipu3_uapi_awb_fr_config_s awb_fr_config; > > +} __packed; > > + > > +/** > > + * struct ipu3_uapi_bubble_info - Bubble info for host side debugging > > + * > > + * @num_of_stripes: A single frame is divided into several parts called > stripes > > + * due to limitation on line buffer memory. > > + * The separation between the stripes is vertical. Each such > > + * stripe is processed as a single frame by the ISP pipe. > > + * @padding: padding bytes. > > + * @num_sets: number of sets. > > + * @padding1: padding bytes. > > + * @size_of_set: set size. > > + * @padding2: padding bytes. > > + * @bubble_size: is the amount of padding in the bubble expressed in > “sets”. > > + * @padding3: padding bytes. > > + */ > > +struct ipu3_uapi_bubble_info { > > + IPU3_ALIGN __u32 num_of_stripes; > > + __u8 padding[28]; > > + __u32 num_sets; > > + __u8 padding1[28]; > > + __u32 size_of_set; > > + __u8 padding2[28]; > > + __u32 bubble_size; > > + __u8 padding3[28]; > > Same question here and elsewhere. > Ditto > > +} __packed; > > + > > +/* > > + * struct ipu3_uapi_stats_3a_bubble_info_per_stripe > > + */ > > +struct ipu3_uapi_stats_3a_bubble_info_per_stripe { > > + struct ipu3_uapi_bubble_info awb[IPU3_UAPI_MAX_STRIPES]; > > + struct ipu3_uapi_bubble_info af[IPU3_UAPI_MAX_STRIPES]; > > + struct ipu3_uapi_bubble_info awb_fr[IPU3_UAPI_MAX_STRIPES]; > > +} __packed; > > + [snip] > > +/** > > + * struct ipu3_uapi_anr_transform_config - Advanced noise reduction > transform > > + * > > + * @enable: advanced noise reduction enabled. > > + * @adaptive_treshhold_en: On IPU3, adaptive threshold is always > enabled. > > + * @__reserved1: reserved > > + * @__reserved2: reserved > > + * @alpha: using follow defaults: > > follow -> following > Ack [snip] > > + * struct ipu3_uapi_params as defined below contains a lot of parameters > and > > + * ipu3_uapi_flags selects which parameters to apply. > > + */ > > +struct ipu3_uapi_params { > > + /* Flags which of the settings below are to be applied */ > > + IPU3_ALIGN 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; > > +} __packed; > > + > > +#endif > > > > So the level of documentation looks good to me. Glad to know. > The use of IPU3_ALIGN, > padding and reserved fields > seems a bit of a mix to me, it's not always clear why a certain approach is > taken. > IPU3_ALIGN is used to enforce 32 byte alignment for data structures used by IPU3 DMA. padding fields are used to ensure the size of the struct remains the same with and without __packed attribute. reserved fields are unused fields of either the data that is output by the IPU3 or the parameters that are sent to the IPU3. > Did you check if the layout of all these structs is the same between 32 and 64 > bit code? > I would recommend that you automate this (e.g. abi-dumper might be useful > for this). > I do need to have confirmation that they are indeed identical. > Yes. abi-dumper output of the ipu3-imgu.ko with and without CONFIG_64BIT shows the same layout and offset for the structure members. > Finally I think we need a high-level description of how to use this: > > I gather that with struct ipu3_uapi_flags you can indicate which blocks need to > be updated, > and that for those blocks that do not need updating the corresponding structs > are ignored. > If so, then that should be documented in pixfmt-meta-intel-ipu3.rst. > Ack > And what about the initial values? E.g. after a reboot, do all the blocks have > sane default > values? Or do you need to supply those yourself? And if it is the latter, then we > need > some open source example code that programs it with sane values. Users can > take that as a > starting point and modify it. > > If userspace needs to provide sane initial values, then it would be best if code > is added to v4l2-utils under contrib. Perhaps under a dual GPL/BSD license? > After a reboot, when streaming is started, ImgU driver sets sane default parameter values for all the blocks. This is done by the driver calling ipu3_css_set_parameters() with set_params as NULL pointer, which leverages the default values from ipu3_tables.c for various blocks. Thanks Raj
