Re: [RFC PATCH]: intel-ipu3: Add uAPI documentation

2018-04-19 Thread Mauro Carvalho Chehab
Em Tue,  3 Apr 2018 19:52:25 -0500
Yong Zhi  escreveu:

> This is a preliminary effort to add documentation for the
> following BNR(bayer noise reduction) structs:
> 
> ipu3_uapi_bnr_static_config_wb_gains_config
> ipu3_uapi_bnr_static_config_wb_gains_thr_config
> ipu3_uapi_bnr_static_config_thr_coeffs_config
> ipu3_uapi_bnr_static_config_thr_ctrl_shd_config
> ipu3_uapi_bnr_static_config_opt_center_sqr_config
> ipu3_uapi_bnr_static_config_bp_ctrl_config
> ipu3_uapi_bnr_static_config_dn_detect_ctrl_config
> ipu3_uapi_bnr_static_config_opt_center_sqr_config
> ipu3_uapi_bnr_static_config_green_disparity
> 
> The feedback on this patch will be used towards the
> documentation of reset of the uAPI in intel-ipu3.h.

I'm assuming that, on the final review, you'll be adding documentation
for all structs, right?

Due to amount of structs and complexity, and, as the IPU3 driver has
seem to be used also on PC products, like on Dell Latitude 5285 detachable
laptop[1], I'm expecting that the documentation will be good enough for people
to be able to develop an Open Source code that will be providing good
quality images from its camera.

It should also receive some sort of logic to make it work as a normal
camera for applications (either via some Kernel support, via libv4l2
or both).

[1] https://www.spinics.net/lists/linux-usb/msg167478.html

> 
> Link to v6 IPU3 ImgU patchset:
> 
> https://patchwork.kernel.org/patch/10316725/>
> 
> Signed-off-by: Yong Zhi 
> Signed-off-by: Chao C Li 
> ---
>  Documentation/media/media_uapi.rst  |1 +
>  Documentation/media/uapi/intel-ipu3.rst |8 +
>  include/uapi/linux/intel-ipu3.h | 1520 
> +++
>  3 files changed, 1529 insertions(+)
>  create mode 100644 Documentation/media/uapi/intel-ipu3.rst
>  create mode 100644 include/uapi/linux/intel-ipu3.h
> 
> diff --git a/Documentation/media/media_uapi.rst 
> b/Documentation/media/media_uapi.rst
> index 28eb35a1f965..edfa674b49c3 100644
> --- a/Documentation/media/media_uapi.rst
> +++ b/Documentation/media/media_uapi.rst
> @@ -31,3 +31,4 @@ License".
>  uapi/cec/cec-api
>  uapi/gen-errors
>  uapi/fdl-appendix
> +uapi/intel-ipu3
> diff --git a/Documentation/media/uapi/intel-ipu3.rst 
> b/Documentation/media/uapi/intel-ipu3.rst
> new file mode 100644
> index ..d4d9b2806fe9
> --- /dev/null
> +++ b/Documentation/media/uapi/intel-ipu3.rst
> @@ -0,0 +1,8 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _intel-ipu3:
> +
> +Intel IPU3 ImgU uAPI headers
> +
> +
> +.. kernel-doc:: include/uapi/linux/intel-ipu3.h

I'm pretty sure that just the headers are not enough for someone to use
the new uAPI. You need to provide some documentation there that would
be enough for someone to write their own code to use this uAPI.

> diff --git a/include/uapi/linux/intel-ipu3.h b/include/uapi/linux/intel-ipu3.h
> new file mode 100644
> index ..34b071524beb
> --- /dev/null
> +++ b/include/uapi/linux/intel-ipu3.h
> @@ -0,0 +1,1520 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2018 Intel Corporation */
> +
> +#ifndef __IPU3_UAPI_H
> +#define __IPU3_UAPI_H
> +
> +#include 
> +
> +#define IPU3_UAPI_ISP_VEC_ELEMS  64
> +
> +#define IMGU_ABI_PAD __aligned(IPU3_UAPI_ISP_WORD_BYTES)
> +#define IPU3_ALIGN   __attribute__((aligned(IPU3_UAPI_ISP_WORD_BYTES)))
> +
> +#define IPU3_UAPI_ISP_WORD_BYTES 32
> +#define IPU3_UAPI_MAX_STRIPES2
> +
> +/*** ipu3_uapi_stats_3a ***/
> +
> +#define IPU3_UAPI_MAX_BUBBLE_SIZE10
> +
> +#define IPU3_UAPI_AE_COLORS  4
> +#define IPU3_UAPI_AE_BINS256
> +
> +#define IPU3_UAPI_AWB_MD_ITEM_SIZE   8
> +#define IPU3_UAPI_AWB_MAX_SETS   60
> +#define IPU3_UAPI_AWB_SET_SIZE   0x500
> +#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \
> + (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
> +  IPU3_UAPI_AWB_MD_ITEM_SIZE)
> +#define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
> + (IPU3_UAPI_AWB_MAX_SETS * \
> +  (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
> +
> +#define IPU3_UAPI_AF_MAX_SETS24
> +#define IPU3_UAPI_AF_MD_ITEM_SIZE4
> +#define IPU3_UAPI_AF_SPARE_FOR_BUBBLES \
> + (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
> +  IPU3_UAPI_AF_MD_ITEM_SIZE)
> +#define IPU3_UAPI_AF_Y_TABLE_SET_SIZE0x80
> +#define IPU3_UAPI_AF_Y_TABLE_MAX_SIZE \
> + (IPU3_UAPI_AF_MAX_SETS * \
> +  (IPU3_UAPI_AF_Y_TABLE_SET_SIZE + IPU3_UAPI_AF_SPARE_FOR_BUBBLES) * \
> +  IPU3_UAPI_MAX_STRIPES)
> +
> +#define IPU3_UAPI_AWB_FR_MAX_SETS24
> +#define IPU3_UAPI_AWB_FR_MD_ITEM_SIZE8
> +#define IPU3_UAPI_AWB_FR_BAYER_TBL_SI

Re: [RFC PATCH]: intel-ipu3: Add uAPI documentation

2018-04-10 Thread Sakari Ailus
Hi Yong,

Thanks for the patch.

On Tue, Apr 03, 2018 at 07:52:25PM -0500, Yong Zhi wrote:
> This is a preliminary effort to add documentation for the
> following BNR(bayer noise reduction) structs:
> 
> ipu3_uapi_bnr_static_config_wb_gains_config
> ipu3_uapi_bnr_static_config_wb_gains_thr_config
> ipu3_uapi_bnr_static_config_thr_coeffs_config
> ipu3_uapi_bnr_static_config_thr_ctrl_shd_config
> ipu3_uapi_bnr_static_config_opt_center_sqr_config
> ipu3_uapi_bnr_static_config_bp_ctrl_config
> ipu3_uapi_bnr_static_config_dn_detect_ctrl_config
> ipu3_uapi_bnr_static_config_opt_center_sqr_config
> ipu3_uapi_bnr_static_config_green_disparity
> 
> The feedback on this patch will be used towards the
> documentation of reset of the uAPI in intel-ipu3.h.
> 
> Link to v6 IPU3 ImgU patchset:
> 
> https://patchwork.kernel.org/patch/10316725/>
> 
> Signed-off-by: Yong Zhi 
> Signed-off-by: Chao C Li 
> ---
>  Documentation/media/media_uapi.rst  |1 +
>  Documentation/media/uapi/intel-ipu3.rst |8 +
>  include/uapi/linux/intel-ipu3.h | 1520 
> +++
>  3 files changed, 1529 insertions(+)
>  create mode 100644 Documentation/media/uapi/intel-ipu3.rst
>  create mode 100644 include/uapi/linux/intel-ipu3.h
> 
> diff --git a/Documentation/media/media_uapi.rst 
> b/Documentation/media/media_uapi.rst
> index 28eb35a1f965..edfa674b49c3 100644
> --- a/Documentation/media/media_uapi.rst
> +++ b/Documentation/media/media_uapi.rst
> @@ -31,3 +31,4 @@ License".
>  uapi/cec/cec-api
>  uapi/gen-errors
>  uapi/fdl-appendix
> +uapi/intel-ipu3
> diff --git a/Documentation/media/uapi/intel-ipu3.rst 
> b/Documentation/media/uapi/intel-ipu3.rst
> new file mode 100644
> index ..d4d9b2806fe9
> --- /dev/null
> +++ b/Documentation/media/uapi/intel-ipu3.rst
> @@ -0,0 +1,8 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _intel-ipu3:
> +
> +Intel IPU3 ImgU uAPI headers
> +
> +
> +.. kernel-doc:: include/uapi/linux/intel-ipu3.h

This should be located in the same file documenting the format. See also
e.g. Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst .

Have you built the documentation?

> diff --git a/include/uapi/linux/intel-ipu3.h b/include/uapi/linux/intel-ipu3.h
> new file mode 100644
> index ..34b071524beb
> --- /dev/null
> +++ b/include/uapi/linux/intel-ipu3.h
> @@ -0,0 +1,1520 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2018 Intel Corporation */

2017--2018 ?

...

> +/*** ipu3_uapi_acc_param ***/
> +
> +#define IPU3_UAPI_BNR_LUT_SIZE   32
> +
> +/* number of elements in gamma correction LUT */
> +#define IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES 256
> +
> +#define IPU3_UAPI_SHD_MAX_CELLS_PER_SET  146
> +/* largest grid is 73x56 */
> +#define IPU3_UAPI_SHD_MAX_CFG_SETS   28
> +
> +#define IPU3_UAPI_YUVP2_YTM_LUT_ENTRIES  256
> +#define IPU3_UAPI_YUVP2_TCC_MACC_TABLE_ELEMENTS  16
> +#define IPU3_UAPI_YUVP2_TCC_INV_Y_LUT_ELEMENTS   14
> +#define IPU3_UAPI_YUVP2_TCC_GAIN_PCWL_LUT_ELEMENTS   258
> +#define IPU3_UAPI_YUVP2_TCC_R_SQR_LUT_ELEMENTS   24
> +
> +#define IPU3_UAPI_BDS_SAMPLE_PATTERN_ARRAY_SIZE  8
> +#define IPU3_UAPI_BDS_PHASE_COEFFS_ARRAY_SIZE32
> +
> +#define IPU3_UAPI_ANR_LUT_SIZE   26
> +#define IPU3_UAPI_ANR_PYRAMID_SIZE   22
> +
> +#define IPU3_UAPI_AE_WEIGHTS 96
> +
> +/* Bayer Noise Reduction related structs */
> +
> +/**
> + * struct ipu3_uapi_bnr_static_config_wb_gains_config - White balance gains
> + * @gr:  white balance gain for Gr channel.
> + * @r:   white balance gain for R channel.
> + * @b:   white balance gain for B channel.
> + * @gb:  white balance gain for Gb channel.

What's the format of these numbers? u8.u8 fixed point for instance?

> + */
> +struct ipu3_uapi_bnr_static_config_wb_gains_config {
> + __u16 gr;
> + __u16 r;
> + __u16 b;
> + __u16 gb;
> +} __packed;
> +
> +/**
> + * struct ipu3_uapi_bnr_static_config_wb_gains_thr_config - Threshold config
> + * @gr:  white balance threshold gain for Gr channel.
> + * @r:   white balance threshold gain for R channel.
> + * @b:   white balance threshold gain for B channel.
> + * @gb:  white balance threshold gain for Gb channel.

Same for these, and below.

> + */
> +struct ipu3_uapi_bnr_static_config_wb_gains_thr_config {
> + __u8 gr;
> + __u8 r;
> + __u8 b;
> + __u8 gb;
> +} __packed;
> +
> +/**
> + * struct ipu3_uapi_bnr_static_config_thr_coeffs_config - Threshold coeffs
> + * @cf:  Free coefficient for threshold calculation
> + * @__reserved0: reserved
> + * @cg:  Gain coefficient for threshold calculation
> + * @ci:  Intensity coefficient for threshold calculation
> + * @__reserve