[v5,01/14] media: starfive: Add JH7110 ISP module definitions

  Add JH7110 ISP module definitions for user space.

Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
Signed-off-by: Zejian Su <zejian.su@starfivetech.com>
---
 MAINTAINERS                     |   1 +
 include/uapi/linux/jh7110-isp.h | 739 ++++++++++++++++++++++++++++++++
 2 files changed, 740 insertions(+)
 create mode 100644 include/uapi/linux/jh7110-isp.h
  

Hello Changhuang

On Tue, Jul 09, 2024 at 01:38:11AM GMT, Changhuang Liang wrote:
> Add JH7110 ISP module definitions for user space.
>
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> Signed-off-by: Zejian Su <zejian.su@starfivetech.com>
> ---
>  MAINTAINERS                     |   1 +
>  include/uapi/linux/jh7110-isp.h | 739 ++++++++++++++++++++++++++++++++

With the recently merged support for the RaspberryPi PiSP BE we have
introduced include/uapi/linux/media/raspberry.

Would you consider placing this in
include/uapi/linux/media/startfive/ ?

>  2 files changed, 740 insertions(+)
>  create mode 100644 include/uapi/linux/jh7110-isp.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 507f04a80499..890604eb0d64 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21305,6 +21305,7 @@ S:	Maintained
>  F:	Documentation/admin-guide/media/starfive_camss.rst
>  F:	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
>  F:	drivers/staging/media/starfive/camss
> +F:	include/uapi/linux/jh7110-isp.h
>
>  STARFIVE CRYPTO DRIVER
>  M:	Jia Jie Ho <jiajie.ho@starfivetech.com>
> diff --git a/include/uapi/linux/jh7110-isp.h b/include/uapi/linux/jh7110-isp.h
> new file mode 100644
> index 000000000000..4939cd63e771
> --- /dev/null
> +++ b/include/uapi/linux/jh7110-isp.h
> @@ -0,0 +1,739 @@
> +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR BSD-3-Clause) */
> +/*
> + * jh7110-isp.h
> + *
> + * JH7110 ISP driver - user space header file.
> + *
> + * Copyright © 2023 StarFive Technology Co., Ltd.
> + *
> + * Author: Zejian Su <zejian.su@starfivetech.com>
> + *
> + */
> +
> +#ifndef __JH7110_ISP_H_
> +#define __JH7110_ISP_H_
> +

Do you need to include

#include <linux/types.h>

> +/**

Is this kernel-doc or a single * would do ?

> + * ISP Module Diagram
> + * ------------------
> + *
> + *  Raw  +-----+    +------+    +------+    +----+
> + *  ---->| OBC |--->| OECF |--->| LCCF |--->| WB |-----+
> + *       +-----+    +------+    +------+    +----+     |
> + *                                                     |
> + *  +--------------------------------------------------+
> + *  |
> + *  |    +-----+    +-----+    +-----+    +-----+
> + *  +--->| DBC |--->| CTC |--->| CFA |--->| CAR |------+
> + *       +-----+    +-----+    +-----+    +-----+      |
> + *                                                     |
> + *  +--------------------------------------------------+
> + *  |
> + *  |    +-----+    +--------+    +-----+    +------+
> + *  +--->| CCM |--->| GMARGB |--->| R2Y |--->| YCRV |--+
> + *       +-----+    +--------+    +-----+    +------+  |
> + *                                                     |
> + *  +--------------------------------------------------+
> + *  |
> + *  |    +-------+    +-------+    +-----+    +----+
> + *  +--->| SHARP |--->| DNYUV |--->| SAT |--->| SC |
> + *       +-------+    +-------+    +-----+    +----+
> + *
> + */
> +
> +/* Auto White Balance */
> +#define JH7110_ISP_MODULE_WB_SETTING			(1U << 0)
> +/* Color Artifact Removal */
> +#define JH7110_ISP_MODULE_CAR_SETTING			(1U << 1)
> +/* Color Correction Matrix */
> +#define JH7110_ISP_MODULE_CCM_SETTING			(1U << 2)
> +/* Color Filter Arrays */
> +#define JH7110_ISP_MODULE_CFA_SETTING			(1U << 3)
> +/* Crosstalk Correction */
> +#define JH7110_ISP_MODULE_CTC_SETTING			(1U << 4)
> +/* Defect Bad Pixel Correction */
> +#define JH7110_ISP_MODULE_DBC_SETTING			(1U << 5)
> +/* Denoise YUV */
> +#define JH7110_ISP_MODULE_DNYUV_SETTING			(1U << 6)
> +/* RGB Gamma */
> +#define JH7110_ISP_MODULE_GMARGB_SETTING		(1U << 7)
> +/* Lens Correction Cosine Fourth */
> +#define JH7110_ISP_MODULE_LCCF_SETTING			(1U << 8)
> +/* Optical Black Correction */
> +#define JH7110_ISP_MODULE_OBC_SETTING			(1U << 9)
> +/* Opto-Electric Conversion Function */
> +#define JH7110_ISP_MODULE_OECF_SETTING			(1U << 10)
> +/* RGB To YUV */
> +#define JH7110_ISP_MODULE_R2Y_SETTING			(1U << 11)
> +/* Saturation */
> +#define JH7110_ISP_MODULE_SAT_SETTING			(1U << 12)
> +/* Sharpen */
> +#define JH7110_ISP_MODULE_SHARP_SETTING			(1U << 13)
> +/* Y Curve */
> +#define JH7110_ISP_MODULE_YCRV_SETTING			(1U << 14)
> +/* Statistics Collection */
> +#define JH7110_ISP_MODULE_SC_SETTING			(1U << 15)
> +
> +/**
> + * struct jh7110_isp_wb_gain - auto white balance gain
> + *
> + * @gain_r: gain value for red component.
> + * @gain_g: gain value for green component.
> + * @gain_b: gain value for blue component.
> + */
> +struct jh7110_isp_wb_gain {
> +	__u16 gain_r;
> +	__u16 gain_g;
> +	__u16 gain_b;
> +};
> +
> +/**
> + * struct jh7110_isp_wb_setting - Configuration used by auto white balance gain
> + *
> + * @enabled: enabled setting flag.
> + * @gains: auto white balance gain setting.
> + */
> +struct jh7110_isp_wb_setting {
> +	__u32 enabled;
> +	struct jh7110_isp_wb_gain gains;
> +};
> +
> +/**
> + * struct jh7110_isp_car_setting - Configuration used by color artifact removal
> + *
> + * @enabled: enabled setting flag.
> + */
> +struct jh7110_isp_car_setting {
> +	__u32 enabled;
> +};
> +
> +/**
> + * struct jh7110_isp_ccm_smlow - Color correction matrix
> + *
> + * @ccm: color transform matrix with size 3 by 3.
> + * @offsets: the offsets of R, G, B after the transform by the ccm.
> + */
> +struct jh7110_isp_ccm_smlow {
> +	__s32 ccm[3][3];
> +	__s32 offsets[3];
> +};
> +
> +/**
> + * struct jh7110_isp_ccm_setting - Configuration used by color correction matrix
> + *
> + * @enabled: enabled setting flag.
> + * @ccm_smlow: Color correction matrix.
> + */
> +struct jh7110_isp_ccm_setting {
> +	__u32 enabled;
> +	struct jh7110_isp_ccm_smlow ccm_smlow;
> +};
> +
> +/**
> + * struct jh7110_isp_cfa_params - demosaic parameters
> + *
> + * @hv_width: detail smooth factor
> + * @cross_cov: Cross covariance weighting.
> + */
> +struct jh7110_isp_cfa_params {
> +	__s32 hv_width;
> +	__s32 cross_cov;
> +};
> +
> +/**
> + * struct jh7110_isp_cfa_params - Configuration used by demosaic module
> + *
> + * @enabled: enabled setting flag.
> + * @settings: demosaic parameters.
> + */
> +struct jh7110_isp_cfa_setting {
> +	__u32 enabled;
> +	struct jh7110_isp_cfa_params settings;
> +};
> +
> +/**
> + * struct jh7110_isp_ctc_params - crosstalk remove parameters
> + *
> + * @saf_mode: smooth area filter mode.
> + * @daf_mode: detail area filter mode.
> + * @max_gt: the threshold for imbalance detection when pixel intensity is close to maximum.

You could easily make this < 80 cols (here and in other places).

I know there are different opinions on how strict on the 80 cols limit
we should be, so up to you.

> + * @min_gt: the threshold for imbalance detection when pixel intensity is close to 0.
> + */
> +struct jh7110_isp_ctc_params {
> +	__u8 saf_mode;
> +	__u8 daf_mode;
> +	__s32 max_gt;
> +	__s32 min_gt;
> +};
> +
> +/**
> + * struct jh7110_isp_ctc_params - Configuration used by crosstalk remove
> + *
> + * @enabled: enabled setting flag.
> + * @settings: corsstalk remove parameters.

crosstalk

> + */
> +struct jh7110_isp_ctc_setting {
> +	__u32 enabled;
> +	struct jh7110_isp_ctc_params settings;
> +};
> +
> +/**
> + * struct jh7110_isp_dbc_params - defect pixels correction parameters
> + *
> + * @bad_gt: bad pixel threshold for the green channel.
> + * @bad_xt: bad pixel threshold for the red and blue channels.
> + */
> +struct jh7110_isp_dbc_params {
> +	__s32 bad_gt;
> +	__s32 bad_xt;
> +};
> +
> +/**
> + * struct jh7110_isp_dbc_params - Configuration used by defect bad pixels correction
> + *
> + * @enabled: enabled setting flag.
> + * @settings: defect pixels correction parameters.
> + */
> +struct jh7110_isp_dbc_setting {
> +	__u32 enabled;
> +	struct jh7110_isp_dbc_params settings;
> +};
> +
> +/**
> + * struct jh7110_isp_dnyuv_params - yuv domain denoise parameters
> + *
> + * @y_sweight: ten coefficients of 7x7 spatial filter for Y channel.
> + * @y_curve: intensity difference (similarity) weight lookup table for Y channel.
> + * @uv_sweight: ten coefficients of 7x7 spatial filter for U and V channel.
> + * @uv_curve: intensity difference (similarity) weight lookup table for U and V channel.
> + */
> +struct jh7110_isp_dnyuv_params {
> +	__u8 y_sweight[10];
> +	__u16 y_curve[7];
> +	__u8 uv_sweight[10];
> +	__u16 uv_curve[7];
> +};
> +
> +/**
> + * struct jh7110_isp_dnyuv_params - Configuration used by yuv domain denoise
> + *
> + * @enabled: enabled setting flag.
> + * @settings: yuv domain denoise parameters.
> + */
> +struct jh7110_isp_dnyuv_setting {
> +	__u32 enabled;
> +	struct jh7110_isp_dnyuv_params settings;
> +};
> +
> +/**
> + * struct jh7110_isp_gmargb_point - RGB Gamma point
> + *
> + * @g_val: RGB gamma value.
> + * @sg_val: RGB gamma slope value.
> + */
> +struct jh7110_isp_gmargb_point {
> +	__u16 g_val;
> +	__u16 sg_val;
> +};
> +
> +/**
> + * struct jh7110_isp_gmargb_setting - Configuration used by RGB gamma
> + *
> + * @enabled: enabled setting flag.
> + * @curve: RGB Gamma point table.
> + */
> +struct jh7110_isp_gmargb_setting {
> +	__u32 enabled;
> +	struct jh7110_isp_gmargb_point curve[15];
> +};
> +
> +/**
> + * struct jh7110_isp_lccf_circle - len circle

lens ?

> + *
> + * @center_x: center X distance from capture window.
> + * @center_y: center Y distance from capture window.
> + * @radius: len circle radius.

here as well

> + */
> +struct jh7110_isp_lccf_circle {
> +	__s16 center_x;
> +	__s16 center_y;
> +	__u8 radius;
> +};
> +
> +/**
> + * struct jh7110_isp_lccf_curve_param - lens correction cosine fourth curve param
> + *
> + * @f1: F1 parameter.
> + * @f2: F2 parameter.
> + */
> +struct jh7110_isp_lccf_curve_param {
> +	__s16 f1;
> +	__s16 f2;
> +};
> +
> +/**
> + * struct jh7110_isp_lccf_setting - Configuration used by lens correction cosine fourth
> + *
> + * @enabled: enabled setting flag.
> + * @circle: len circle.
> + * @r_param: lens correction cosine fourth curve param for Bayer pattern R.
> + * @gr_param: lens correction cosine fourth curve param for Bayer pattern Gr.
> + * @gb_param: lens correction cosine fourth curve param for Bayer pattern Gb.
> + * @b_param: lens correction cosine fourth curve param for Bayer pattern B.
> + */
> +struct jh7110_isp_lccf_setting {
> +	__u32 enabled;
> +	struct jh7110_isp_lccf_circle circle;
> +	struct jh7110_isp_lccf_curve_param r_param;
> +	struct jh7110_isp_lccf_curve_param gr_param;
> +	struct jh7110_isp_lccf_curve_param gb_param;
> +	struct jh7110_isp_lccf_curve_param b_param;
> +};
> +
> +/**
> + * struct jh7110_isp_obc_win_size - optical black correction window size
> + *
> + * @width: window width.
> + * @height: window height.
> + */
> +struct jh7110_isp_obc_win_size {
> +	__u32 width;
> +	__u32 height;
> +};
> +
> +/**
> + * struct jh7110_isp_obc_gain - optical black correction symbol gain
> + *
> + * @tl_gain: gain at point A for symbol.
> + * @tr_gain: gain at point B for symbol.
> + * @bl_gain: gain at point C for symbol.
> + * @br_gain: gain at point D for symbol.
> + */
> +struct jh7110_isp_obc_gain {
> +	__u8 tl_gain;
> +	__u8 tr_gain;
> +	__u8 bl_gain;
> +	__u8 br_gain;
> +};
> +
> +/**
> + * struct jh7110_isp_obc_offset - optical black correction symbol offset
> + *
> + * @tl_offset: offset at point A for symbol.
> + * @tr_offset: offset at point B for symbol.
> + * @bl_offset: offset at point C for symbol.
> + * @br_offset: offset at point D for symbol.
> + */
> +struct jh7110_isp_obc_offset {
> +	__u8 tl_offset;
> +	__u8 tr_offset;
> +	__u8 bl_offset;
> +	__u8 br_offset;
> +};
> +
> +/**
> + * struct jh7110_isp_obc_setting - Configuration used by optical black correction
> + *
> + * @enabled: enabled setting flag.
> + * @win_size: optical black correction window size.
> + * @gain: optical black correction symbol gain.
> + * @offset: optical black correction symbol offset.
> + */
> +struct jh7110_isp_obc_setting {
> +	__u32 enabled;
> +	struct jh7110_isp_obc_win_size win_size;
> +	struct jh7110_isp_obc_gain gain[4];
> +	struct jh7110_isp_obc_offset offset[4];
> +};
> +
> +/**
> + * struct jh7110_isp_oecf_point - oecf curve
> + *
> + * @x: x coordinate.
> + * @y: y coordinate.
> + * @slope: the slope between this point and the next point.
> + */
> +struct jh7110_isp_oecf_point {
> +	__u16 x;
> +	__u16 y;
> +	__s16 slope;
> +};
> +
> +/**
> + * struct jh7110_isp_oecf_setting - Configuration used by opto-electric conversion function
> + *
> + * @enabled: enabled setting flag.
> + * @r_curve: red pixel oecf curve.
> + * @gr_curve: green pixel oecf curve in GR line.
> + * @gb_curve: green pixel oecf curve in GB line.
> + * @b_curve: blue pixel oecf curve.
> + */
> +struct jh7110_isp_oecf_setting {
> +	__u32 enabled;
> +	struct jh7110_isp_oecf_point r_curve[16];
> +	struct jh7110_isp_oecf_point gr_curve[16];
> +	struct jh7110_isp_oecf_point gb_curve[16];
> +	struct jh7110_isp_oecf_point b_curve[16];
> +};
> +
> +/**
> + * struct jh7110_isp_r2y_matrix - RGB to YUV color conversion matrix
> + *
> + * @m: The 3x3 color conversion matrix coefficient.
> + */
> +struct jh7110_isp_r2y_matrix {
> +	__s16 m[9];
> +};
> +
> +/**
> + * struct jh7110_isp_r2y_setting - Configuration used by RGB To YUV
> + *
> + * @enabled: enabled setting flag.
> + * @matrix: RGB to YUV color conversion matrix.
> + */
> +struct jh7110_isp_r2y_setting {
> +	__u32 enabled;
> +	struct jh7110_isp_r2y_matrix matrix;
> +};
> +
> +/**
> + * struct jh7110_isp_sat_curve - Saturation curve
> + *
> + * @yi_min: the minimum input Y value.
> + * @yo_ir: the ratio of Y output range to input range.
> + * @yo_min: the minimum output Y value.
> + * @yo_max: the maximum output Y value.
> + */
> +struct jh7110_isp_sat_curve {
> +	__s16 yi_min;
> +	__s16 yo_ir;
> +	__s16 yo_min;
> +	__s16 yo_max;
> +};
> +
> +/**
> + * struct jh7110_isp_sat_hue_info - Chroma Saturation Hue Factor
> + *
> + * @cos: COS hue factor.
> + * @sin: SIN hue factor.
> + */
> +struct jh7110_isp_sat_hue_info {
> +	__s16 cos;
> +	__s16 sin;
> +};
> +
> +/**
> + * struct jh7110_isp_sat_info - Saturation information
> + *
> + * @gain_cmab: Chroma saturation magnitude amplification base for gain.
> + * @gain_cmmd: Chroma saturation magnitude amplification delta for gain.
> + * @threshold_cmb: Chroma saturation magnitude base threshold.
> + * @threshold_cmd: Chroma saturation magnitude delta threshold.
> + * @offset_u: Chroma saturation U offset.
> + * @offset_v: Chroma saturation V offset.
> + * @cmsf: Chroma saturation magnitude scaling factor.
> + */
> +struct jh7110_isp_sat_info {
> +	__s16 gain_cmab;
> +	__s16 gain_cmmd;
> +	__s16 threshold_cmb;
> +	__s16 threshold_cmd;
> +	__s16 offset_u;
> +	__s16 offset_v;
> +	__s16 cmsf;
> +};
> +
> +/**
> + * struct jh7110_isp_sat_setting - Configuration used by Saturation
> + *
> + * @enabled: enabled setting flag.
> + * @curve: Saturation curve.
> + * @hue_info: Chroma Saturation Hue Factor.
> + * @sat_info: Saturation information.s

informations.

> + */
> +struct jh7110_isp_sat_setting {
> +	__u32 enabled;
> +	struct jh7110_isp_sat_curve curve;
> +	struct jh7110_isp_sat_hue_info hue_info;
> +	struct jh7110_isp_sat_info sat_info;
> +};
> +
> +/**
> + * struct jh7110_isp_sharp_weight - Sharpe weight
> + *
> + * @weight: Sharpen filter weight.
> + * @recip_wei_sum: Sharpen amplification filter weight normalization factor.
> + */
> +struct jh7110_isp_sharp_weight {
> +	__u8 weight[15];
> +	__u32 recip_wei_sum;
> +};
> +
> +/**
> + * struct jh7110_isp_sharp_strength - Sharpen strength
> + *
> + * @diff: Sharpen Edge amplification delta level.
> + * @f: Sharpen Edge amplification factor.
> + * @s: Sharpen Edge amplification factor slope.
> + */
> +struct jh7110_isp_sharp_strength {
> +	__s16 diff[4];
> +	__s16 f[3];
> +	__s32 s[3];
> +};
> +
> +/**
> + * struct jh7110_isp_sharp_setting - Configuration used by Sharpen
> + *
> + * @enabled: enabled setting flag.
> + * @weight: Sharpe weight.
> + * @strength: Sharpen strength.
> + * @pdirf: Positive Factor Multiplier.
> + * @ndirf: Negative Factor Multiplier.
> + */
> +struct jh7110_isp_sharp_setting {
> +	__u32 enabled;
> +	struct jh7110_isp_sharp_weight weight;
> +	struct jh7110_isp_sharp_strength strength;
> +	__s8 pdirf;
> +	__s8 ndirf;
> +};
> +
> +/**
> + * struct jh7110_isp_ycrv_curve - Y Curve parameters table
> + *
> + * @y: Y curve L parameters value.
> + */
> +struct jh7110_isp_ycrv_curve {
> +	__s16 y[64];
> +};
> +
> +/**
> + * struct jh7110_isp_ycrv_setting - Configuration used by Y Curve
> + *
> + * @enabled: enabled setting flag.
> + * @curve: Y Curve parameters table.
> + */
> +struct jh7110_isp_ycrv_setting {
> +	__u32 enabled;
> +	struct jh7110_isp_ycrv_curve curve;

I am a bit failing in seeing the point of embedding the settings in a
dedicated structure when you have a single instance of the
configuration like this and in other instances. Isn't

        struct jh7110_isp_ycrv_setting {
                __u32 enabled;
                __s16 y[64];
        };

easier ? Or do you need a dedicated type for other reasons ?

> +};
> +
> +/**
> + * struct jh7110_isp_sc_config - statistics collection crop configure
> + *
> + * @h_start: Horizontal starting point for frame cropping.
> + * @v_start: Vertical starting point for frame cropping.
> + * @sw_width: Width of statistics collection sub-window.
> + * @sw_height: Height of statistics collection sub-window.
> + * @hperiod: Horizontal period.
> + * @hkeep: Horizontal keep.
> + * @vperiod: Vertical period.
> + * @vkeep: Vertical keep.
> + */
> +struct jh7110_isp_sc_config {
> +	__u16 h_start;
> +	__u16 v_start;
> +	__u8 sw_width;
> +	__u8 sw_height;
> +	__u8 hperiod;
> +	__u8 hkeep;
> +	__u8 vperiod;
> +	__u8 vkeep;
> +};
> +
> +/**
> + * struct jh7110_isp_sc_af_config - statistics collection auto focus configure
> + *
> + * @es_hor_mode: Horizontal mode.
> + * @es_sum_mode: sum mode.

Other fields are documented with a capital letter -> "Sum mode."

> + * @hor_en: Horizontal enable.
> + * @ver_en: Vertical enable.
> + * @es_ver_thr: Vertical threshold.
> + * @es_hor_thr: Horizontal threshold.
> + */
> +struct jh7110_isp_sc_af_config {
> +	__u8 es_hor_mode;
> +	__u8 es_sum_mode;
> +	__u8 hor_en;
> +	__u8 ver_en;
> +	__u8 es_ver_thr;
> +	__u16 es_hor_thr;
> +};
> +
> +/**
> + * struct jh7110_isp_sc_awb_ps - statistics collection auto white balance pixel sum
> + *
> + * @awb_ps_rl: Lower boundary of R value for pixel sum.
> + * @awb_ps_ru: Upper boundary of R value for pixel sum.
> + * @awb_ps_gl: Lower boundary of G value for pixel sum.
> + * @awb_ps_gu: Upper boundary of G value for pixel sum.
> + * @awb_ps_bl: Lower boundary of B value for pixel sum.
> + * @awb_ps_bu: Upper boundary of B value for pixel sum.
> + * @awb_ps_yl: Lower boundary of Y value for pixel sum.
> + * @awb_ps_yu: Upper boundary of Y value for pixel sum.
> + * @awb_ps_grl: Lower boundary of G/R ratio for pixel sum.
> + * @awb_ps_gru: Upper boundary of G/R ratio for pixel sum.
> + * @awb_ps_gbl: Lower boundary of G/B ratio for pixel sum.
> + * @awb_ps_gbu: Upper boundary of G/B ratio for pixel sum.
> + * @awb_ps_grbl: Lower boundary of (Gr/R + b/a * Gb/B) for pixel sum.
> + * @awb_ps_grbu: Upper boundary of (Gr/R + b/a * Gb/B) for pixel sum.
> + */
> +struct jh7110_isp_sc_awb_ps {
> +	__u8 awb_ps_rl;
> +	__u8 awb_ps_ru;
> +	__u8 awb_ps_gl;
> +	__u8 awb_ps_gu;
> +	__u8 awb_ps_bl;
> +	__u8 awb_ps_bu;
> +	__u8 awb_ps_yl;
> +	__u8 awb_ps_yu;
> +	__u16 awb_ps_grl;
> +	__u16 awb_ps_gru;
> +	__u16 awb_ps_gbl;
> +	__u16 awb_ps_gbu;
> +	__u16 awb_ps_grbl;
> +	__u16 awb_ps_grbu;
> +};
> +
> +/**
> + * struct jh7110_isp_sc_awb_ws - statistics collection auto white balance weight sum
> + *
> + * @awb_ws_rl: Lower boundary of R value for weight sum.
> + * @awb_ws_ru: Upper boundary of R value for weight sum.
> + * @awb_ws_grl: Lower boundary of Gr value for weight sum.
> + * @awb_ws_gru: Upper boundary of Gr value for weight sum.
> + * @awb_ws_gbl: Lower boundary of Gb value for weight sum.
> + * @awb_ws_gbu: Upper boundary of Gb value for weight sum.
> + * @awb_ws_bl: Lower boundary of B value for weight sum.
> + * @awb_ws_bu: Upper boundary of B value for weight sum.
> + */
> +struct jh7110_isp_sc_awb_ws {
> +	__u8 awb_ws_rl;
> +	__u8 awb_ws_ru;
> +	__u8 awb_ws_grl;
> +	__u8 awb_ws_gru;
> +	__u8 awb_ws_gbl;
> +	__u8 awb_ws_gbu;
> +	__u8 awb_ws_bl;
> +	__u8 awb_ws_bu;
> +};
> +
> +/**
> + * struct jh7110_isp_sc_awb_point - statistics collection auto white balance point
> + *
> + * @weight: Weighting value at point.
> + */
> +struct jh7110_isp_sc_awb_point {
> +	__u8 weight;
> +};
> +
> +/**
> + * struct jh7110_isp_sc_awb_config - statistics collection auto white balance configure
> + *
> + * @ps_config: statistics collection auto white balance pixel sum.

nit: please be consistent with using capital letters or not in doc.

> + * @awb_ps_grb_ba: auto white balance b/a value.
> + * @sel: input mux for statistics collection auto white balance.
> + * @ws_config: statistics collection auto white balance weight sum.
> + * @awb_cw: Weighting value at 13x13 point.
> + * @pts: statistics collection auto white balance point.
> + */
> +struct jh7110_isp_sc_awb_config {
> +	struct jh7110_isp_sc_awb_ps ps_config;
> +	__u8 awb_ps_grb_ba;
> +	__u8 sel;
> +	struct jh7110_isp_sc_awb_ws ws_config;
> +	__u8 awb_cw[169];
> +	struct jh7110_isp_sc_awb_point pts[17];
> +};
> +
> +/**
> + * struct jh7110_isp_sc_setting - Configuration used by statistics collection
> + *
> + * @enabled: enabled setting flag.
> + * @crop_config: statistics collection crop configure.
> + * @af_config: statistics collection auto focus configure.
> + * @awb_config: statistics collection auto white balance configure.
> + */
> +struct jh7110_isp_sc_setting {
> +	__u32 enabled;
> +	struct jh7110_isp_sc_config crop_config;
> +	struct jh7110_isp_sc_af_config af_config;
> +	struct jh7110_isp_sc_awb_config awb_config;
> +};
> +
> +/**
> + * struct jh7110_isp_params_buffer - StarFive JH7110 ISP Parameters Meta Data
> + *
> + * @enable_setting: enabled setting module (JH7110_ISP_MODULE_* definitions).
> + * @wb_setting: Configuration used by auto white balance gain.
> + * @car_setting: Configuration used by color artifact removal.
> + * @ccm_setting: Configuration used by color correction matrix.
> + * @cfa_setting: Configuration used by demosaic module.
> + * @ctc_setting: Configuration used by crosstalk remove.
> + * @dbc_setting: Configuration used by defect bad pixels correction.
> + * @dnyuv_setting: Configuration used by yuv domain denoise.
> + * @gmargb_setting: Configuration used by RGB gamma.
> + * @lccf_setting: Configuration used by lens correction cosine fourth.
> + * @obc_setting: Configuration used by optical black compensation.
> + * @oecf_setting: Configuration used by opto-electric conversion function.
> + * @r2y_setting: Configuration used by RGB To YUV.
> + * @sat_setting: Configuration used by Saturation.
> + * @sharp_setting: Configuration used by Sharpen.
> + * @ycrv_setting: Configuration used by Y Curve.
> + * @sc_setting: Configuration used by statistics collection.
> + */
> +struct jh7110_isp_params_buffer {
> +	__u32 enable_setting;
> +	struct jh7110_isp_wb_setting wb_setting;
> +	struct jh7110_isp_car_setting car_setting;
> +	struct jh7110_isp_ccm_setting ccm_setting;
> +	struct jh7110_isp_cfa_setting cfa_setting;
> +	struct jh7110_isp_ctc_setting ctc_setting;
> +	struct jh7110_isp_dbc_setting dbc_setting;
> +	struct jh7110_isp_dnyuv_setting dnyuv_setting;
> +	struct jh7110_isp_gmargb_setting gmargb_setting;
> +	struct jh7110_isp_lccf_setting lccf_setting;
> +	struct jh7110_isp_obc_setting obc_setting;
> +	struct jh7110_isp_oecf_setting oecf_setting;
> +	struct jh7110_isp_r2y_setting r2y_setting;
> +	struct jh7110_isp_sat_setting sat_setting;
> +	struct jh7110_isp_sharp_setting sharp_setting;
> +	struct jh7110_isp_ycrv_setting ycrv_setting;
> +	struct jh7110_isp_sc_setting sc_setting;
> +};
> +
> +/**
> + * Statistics Collection Meta Data Flag
> + */
> +#define JH7110_ISP_SC_FLAG_AWB			0x0
> +#define JH7110_ISP_SC_FLAG_AE_AF		0xffff
> +
> +#pragma pack(1)
> +
> +/**
> + * struct jh7110_isp_sc_buffer - StarFive JH7110 ISP Statistics Collection Meta Data
> + *
> + * @y_histogram: Y histogram data for saturation control.
> + * @reserv0: reserve byte.
> + * @bright_sc: bright statistic. If flag is JH7110_ISP_SC_FLAG_AE_AF, This field is

s/bright statistics/brightness statistics/

no capital "T" after ,

> + *             saved auto exposure and auto focus. If flag is JH7110_ISP_SC_FLAG_AWB,
> + *             This field is saved auto white balance.

no capital "T" after ,

I would replace "this field is saved" which doesn't sound great in
English (not a native speaker though) with "this field stores".

> + * @reserv1: reserve byte.
> + * @ae_hist_y: Y histogram for auto exposure.
> + * @reserv2: reserve byte.
> + * @flag: Statistics Collection Meta Data Flag (JH7110_ISP_SC_FLAG_* definitions)
> + */
> +struct jh7110_isp_sc_buffer {
> +	__u32 y_histogram[64];
> +	__u32 reserv0[33];
> +	__u32 bright_sc[4096];
> +	__u32 reserv1[96];
> +	__u32 ae_hist_y[128];
> +	__u32 reserv2[511];
> +	__u16 flag;
> +};
> +
> +#pragma pack()

This structure is packed, is it populated directly from HW registers
with a memcpy or a DMA transfer ? I guess I'll find it out in the next
patches.

Thanks
   j

> +
> +#endif
> --
> 2.25.1
>
>
  

On Wed, Jul 10, 2024 at 11:11:57AM +0200, Jacopo Mondi wrote:
> On Tue, Jul 09, 2024 at 01:38:11AM GMT, Changhuang Liang wrote:
> > Add JH7110 ISP module definitions for user space.
> >
> > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > Signed-off-by: Zejian Su <zejian.su@starfivetech.com>
> > ---
> >  MAINTAINERS                     |   1 +
> >  include/uapi/linux/jh7110-isp.h | 739 ++++++++++++++++++++++++++++++++
> 
> With the recently merged support for the RaspberryPi PiSP BE we have
> introduced include/uapi/linux/media/raspberry.
> 
> Would you consider placing this in
> include/uapi/linux/media/startfive/ ?

That sounds like a good idea.

> >  2 files changed, 740 insertions(+)
> >  create mode 100644 include/uapi/linux/jh7110-isp.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 507f04a80499..890604eb0d64 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -21305,6 +21305,7 @@ S:	Maintained
> >  F:	Documentation/admin-guide/media/starfive_camss.rst
> >  F:	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
> >  F:	drivers/staging/media/starfive/camss
> > +F:	include/uapi/linux/jh7110-isp.h
> >
> >  STARFIVE CRYPTO DRIVER
> >  M:	Jia Jie Ho <jiajie.ho@starfivetech.com>
> > diff --git a/include/uapi/linux/jh7110-isp.h b/include/uapi/linux/jh7110-isp.h
> > new file mode 100644
> > index 000000000000..4939cd63e771
> > --- /dev/null
> > +++ b/include/uapi/linux/jh7110-isp.h
> > @@ -0,0 +1,739 @@
> > +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR BSD-3-Clause) */
> > +/*
> > + * jh7110-isp.h
> > + *
> > + * JH7110 ISP driver - user space header file.
> > + *
> > + * Copyright © 2023 StarFive Technology Co., Ltd.
> > + *
> > + * Author: Zejian Su <zejian.su@starfivetech.com>
> > + *
> > + */
> > +
> > +#ifndef __JH7110_ISP_H_
> > +#define __JH7110_ISP_H_
> > +
> 
> Do you need to include
> 
> #include <linux/types.h>
> 
> > +/**
> 
> Is this kernel-doc or a single * would do ?
> 
> > + * ISP Module Diagram
> > + * ------------------
> > + *
> > + *  Raw  +-----+    +------+    +------+    +----+
> > + *  ---->| OBC |--->| OECF |--->| LCCF |--->| WB |-----+
> > + *       +-----+    +------+    +------+    +----+     |
> > + *                                                     |
> > + *  +--------------------------------------------------+
> > + *  |
> > + *  |    +-----+    +-----+    +-----+    +-----+
> > + *  +--->| DBC |--->| CTC |--->| CFA |--->| CAR |------+
> > + *       +-----+    +-----+    +-----+    +-----+      |
> > + *                                                     |
> > + *  +--------------------------------------------------+
> > + *  |
> > + *  |    +-----+    +--------+    +-----+    +------+
> > + *  +--->| CCM |--->| GMARGB |--->| R2Y |--->| YCRV |--+
> > + *       +-----+    +--------+    +-----+    +------+  |
> > + *                                                     |
> > + *  +--------------------------------------------------+
> > + *  |
> > + *  |    +-------+    +-------+    +-----+    +----+
> > + *  +--->| SHARP |--->| DNYUV |--->| SAT |--->| SC |
> > + *       +-------+    +-------+    +-----+    +----+
> > + *

The diagram is useful, thank you. A glossary would also be nice, maybe
as

 * - OBC: Optical Black Correction
 * - OECF: Opto-Electric Conversion Function
 * ...

I think that would be easier to read than the comments above each macro
below. Up to you.

> > + */
> > +
> > +/* Auto White Balance */
> > +#define JH7110_ISP_MODULE_WB_SETTING			(1U << 0)
> > +/* Color Artifact Removal */
> > +#define JH7110_ISP_MODULE_CAR_SETTING			(1U << 1)
> > +/* Color Correction Matrix */
> > +#define JH7110_ISP_MODULE_CCM_SETTING			(1U << 2)
> > +/* Color Filter Arrays */
> > +#define JH7110_ISP_MODULE_CFA_SETTING			(1U << 3)
> > +/* Crosstalk Correction */
> > +#define JH7110_ISP_MODULE_CTC_SETTING			(1U << 4)
> > +/* Defect Bad Pixel Correction */
> > +#define JH7110_ISP_MODULE_DBC_SETTING			(1U << 5)
> > +/* Denoise YUV */
> > +#define JH7110_ISP_MODULE_DNYUV_SETTING			(1U << 6)
> > +/* RGB Gamma */
> > +#define JH7110_ISP_MODULE_GMARGB_SETTING		(1U << 7)
> > +/* Lens Correction Cosine Fourth */
> > +#define JH7110_ISP_MODULE_LCCF_SETTING			(1U << 8)
> > +/* Optical Black Correction */
> > +#define JH7110_ISP_MODULE_OBC_SETTING			(1U << 9)
> > +/* Opto-Electric Conversion Function */
> > +#define JH7110_ISP_MODULE_OECF_SETTING			(1U << 10)
> > +/* RGB To YUV */
> > +#define JH7110_ISP_MODULE_R2Y_SETTING			(1U << 11)
> > +/* Saturation */
> > +#define JH7110_ISP_MODULE_SAT_SETTING			(1U << 12)
> > +/* Sharpen */
> > +#define JH7110_ISP_MODULE_SHARP_SETTING			(1U << 13)
> > +/* Y Curve */
> > +#define JH7110_ISP_MODULE_YCRV_SETTING			(1U << 14)
> > +/* Statistics Collection */
> > +#define JH7110_ISP_MODULE_SC_SETTING			(1U << 15)

Unless there's a specific reason to keep the current order, maybe you
could sort those macros in the same order as in the module diagram ?

> > +
> > +/**
> > + * struct jh7110_isp_wb_gain - auto white balance gain
> > + *
> > + * @gain_r: gain value for red component.
> > + * @gain_g: gain value for green component.
> > + * @gain_b: gain value for blue component.

I suppose the gains are expressed as fixed-point integers. This needs
more details, what are the limits, and how many bits of integer and
fractional parts are there ?

Same comment for all the other values below.

> > + */
> > +struct jh7110_isp_wb_gain {
> > +	__u16 gain_r;
> > +	__u16 gain_g;
> > +	__u16 gain_b;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_wb_setting - Configuration used by auto white balance gain
> > + *
> > + * @enabled: enabled setting flag.
> > + * @gains: auto white balance gain setting.
> > + */
> > +struct jh7110_isp_wb_setting {
> > +	__u32 enabled;
> > +	struct jh7110_isp_wb_gain gains;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_car_setting - Configuration used by color artifact removal
> > + *
> > + * @enabled: enabled setting flag.
> > + */
> > +struct jh7110_isp_car_setting {
> > +	__u32 enabled;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_ccm_smlow - Color correction matrix
> > + *
> > + * @ccm: color transform matrix with size 3 by 3.
> > + * @offsets: the offsets of R, G, B after the transform by the ccm.
> > + */
> > +struct jh7110_isp_ccm_smlow {
> > +	__s32 ccm[3][3];
> > +	__s32 offsets[3];
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_ccm_setting - Configuration used by color correction matrix
> > + *
> > + * @enabled: enabled setting flag.
> > + * @ccm_smlow: Color correction matrix.
> > + */
> > +struct jh7110_isp_ccm_setting {
> > +	__u32 enabled;
> > +	struct jh7110_isp_ccm_smlow ccm_smlow;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_cfa_params - demosaic parameters
> > + *
> > + * @hv_width: detail smooth factor
> > + * @cross_cov: Cross covariance weighting.

This documentation doesn't tell how to use those paraemeters. This
comment applies to many other parameters below. There are three main
options to improve that:

- Expanding the documentation in this header file to clearly explain how
  to compute the parameters values.

- Providing an open userspace implementation of ISP algorithms that
  showcase how to calculate the values.

- Providing detailed hardware documentation for the ISP. This is usually
  not favoured by ISP vendors, and we are not pushing for this, but I
  wanted to mention it for completeness.

If you would prefer the second option, any open-source camera framework
would be acceptable, but in practice the only real option is likely
libcamera.

This does not mean that you have to open-source all your ISP control
algorithms. Only the code needed to explain how ISP parameters are
applied to the image and are computed is needed. Other parts, such as
for instance AI-based computation of white balance gains, or complex AGC
calculations, don't need to be open-source.

The explain this requirement different and perhaps more clearly, the
goal is to make sure that developers who have access to only open-source
code (ISP kernel driver, this header, any open-source userspace code,
...) will have enough information to configure and control the ISP.

> > + */
> > +struct jh7110_isp_cfa_params {
> > +	__s32 hv_width;
> > +	__s32 cross_cov;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_cfa_params - Configuration used by demosaic module
> > + *
> > + * @enabled: enabled setting flag.
> > + * @settings: demosaic parameters.
> > + */
> > +struct jh7110_isp_cfa_setting {
> > +	__u32 enabled;
> > +	struct jh7110_isp_cfa_params settings;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_ctc_params - crosstalk remove parameters
> > + *
> > + * @saf_mode: smooth area filter mode.
> > + * @daf_mode: detail area filter mode.
> > + * @max_gt: the threshold for imbalance detection when pixel intensity is close to maximum.
> 
> You could easily make this < 80 cols (here and in other places).
> 
> I know there are different opinions on how strict on the 80 cols limit
> we should be, so up to you.
> 
> > + * @min_gt: the threshold for imbalance detection when pixel intensity is close to 0.
> > + */
> > +struct jh7110_isp_ctc_params {
> > +	__u8 saf_mode;
> > +	__u8 daf_mode;
> > +	__s32 max_gt;
> > +	__s32 min_gt;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_ctc_params - Configuration used by crosstalk remove
> > + *
> > + * @enabled: enabled setting flag.
> > + * @settings: corsstalk remove parameters.
> 
> crosstalk
> 
> > + */
> > +struct jh7110_isp_ctc_setting {
> > +	__u32 enabled;
> > +	struct jh7110_isp_ctc_params settings;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_dbc_params - defect pixels correction parameters
> > + *
> > + * @bad_gt: bad pixel threshold for the green channel.
> > + * @bad_xt: bad pixel threshold for the red and blue channels.
> > + */
> > +struct jh7110_isp_dbc_params {
> > +	__s32 bad_gt;
> > +	__s32 bad_xt;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_dbc_params - Configuration used by defect bad pixels correction
> > + *
> > + * @enabled: enabled setting flag.
> > + * @settings: defect pixels correction parameters.
> > + */
> > +struct jh7110_isp_dbc_setting {
> > +	__u32 enabled;
> > +	struct jh7110_isp_dbc_params settings;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_dnyuv_params - yuv domain denoise parameters
> > + *
> > + * @y_sweight: ten coefficients of 7x7 spatial filter for Y channel.
> > + * @y_curve: intensity difference (similarity) weight lookup table for Y channel.
> > + * @uv_sweight: ten coefficients of 7x7 spatial filter for U and V channel.
> > + * @uv_curve: intensity difference (similarity) weight lookup table for U and V channel.
> > + */
> > +struct jh7110_isp_dnyuv_params {
> > +	__u8 y_sweight[10];
> > +	__u16 y_curve[7];
> > +	__u8 uv_sweight[10];
> > +	__u16 uv_curve[7];
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_dnyuv_params - Configuration used by yuv domain denoise
> > + *
> > + * @enabled: enabled setting flag.
> > + * @settings: yuv domain denoise parameters.
> > + */
> > +struct jh7110_isp_dnyuv_setting {
> > +	__u32 enabled;
> > +	struct jh7110_isp_dnyuv_params settings;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_gmargb_point - RGB Gamma point
> > + *
> > + * @g_val: RGB gamma value.
> > + * @sg_val: RGB gamma slope value.
> > + */
> > +struct jh7110_isp_gmargb_point {
> > +	__u16 g_val;
> > +	__u16 sg_val;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_gmargb_setting - Configuration used by RGB gamma
> > + *
> > + * @enabled: enabled setting flag.
> > + * @curve: RGB Gamma point table.
> > + */
> > +struct jh7110_isp_gmargb_setting {
> > +	__u32 enabled;
> > +	struct jh7110_isp_gmargb_point curve[15];
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_lccf_circle - len circle
> 
> lens ?
> 
> > + *
> > + * @center_x: center X distance from capture window.
> > + * @center_y: center Y distance from capture window.
> > + * @radius: len circle radius.
> 
> here as well
> 
> > + */
> > +struct jh7110_isp_lccf_circle {
> > +	__s16 center_x;
> > +	__s16 center_y;
> > +	__u8 radius;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_lccf_curve_param - lens correction cosine fourth curve param
> > + *
> > + * @f1: F1 parameter.
> > + * @f2: F2 parameter.
> > + */
> > +struct jh7110_isp_lccf_curve_param {
> > +	__s16 f1;
> > +	__s16 f2;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_lccf_setting - Configuration used by lens correction cosine fourth
> > + *
> > + * @enabled: enabled setting flag.
> > + * @circle: len circle.
> > + * @r_param: lens correction cosine fourth curve param for Bayer pattern R.
> > + * @gr_param: lens correction cosine fourth curve param for Bayer pattern Gr.
> > + * @gb_param: lens correction cosine fourth curve param for Bayer pattern Gb.
> > + * @b_param: lens correction cosine fourth curve param for Bayer pattern B.
> > + */
> > +struct jh7110_isp_lccf_setting {
> > +	__u32 enabled;
> > +	struct jh7110_isp_lccf_circle circle;
> > +	struct jh7110_isp_lccf_curve_param r_param;
> > +	struct jh7110_isp_lccf_curve_param gr_param;
> > +	struct jh7110_isp_lccf_curve_param gb_param;
> > +	struct jh7110_isp_lccf_curve_param b_param;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_obc_win_size - optical black correction window size
> > + *
> > + * @width: window width.
> > + * @height: window height.
> > + */
> > +struct jh7110_isp_obc_win_size {
> > +	__u32 width;
> > +	__u32 height;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_obc_gain - optical black correction symbol gain
> > + *
> > + * @tl_gain: gain at point A for symbol.
> > + * @tr_gain: gain at point B for symbol.
> > + * @bl_gain: gain at point C for symbol.
> > + * @br_gain: gain at point D for symbol.
> > + */
> > +struct jh7110_isp_obc_gain {
> > +	__u8 tl_gain;
> > +	__u8 tr_gain;
> > +	__u8 bl_gain;
> > +	__u8 br_gain;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_obc_offset - optical black correction symbol offset
> > + *
> > + * @tl_offset: offset at point A for symbol.
> > + * @tr_offset: offset at point B for symbol.
> > + * @bl_offset: offset at point C for symbol.
> > + * @br_offset: offset at point D for symbol.
> > + */
> > +struct jh7110_isp_obc_offset {
> > +	__u8 tl_offset;
> > +	__u8 tr_offset;
> > +	__u8 bl_offset;
> > +	__u8 br_offset;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_obc_setting - Configuration used by optical black correction
> > + *
> > + * @enabled: enabled setting flag.
> > + * @win_size: optical black correction window size.
> > + * @gain: optical black correction symbol gain.
> > + * @offset: optical black correction symbol offset.
> > + */
> > +struct jh7110_isp_obc_setting {
> > +	__u32 enabled;
> > +	struct jh7110_isp_obc_win_size win_size;
> > +	struct jh7110_isp_obc_gain gain[4];
> > +	struct jh7110_isp_obc_offset offset[4];
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_oecf_point - oecf curve
> > + *
> > + * @x: x coordinate.
> > + * @y: y coordinate.
> > + * @slope: the slope between this point and the next point.
> > + */
> > +struct jh7110_isp_oecf_point {
> > +	__u16 x;
> > +	__u16 y;
> > +	__s16 slope;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_oecf_setting - Configuration used by opto-electric conversion function
> > + *
> > + * @enabled: enabled setting flag.
> > + * @r_curve: red pixel oecf curve.
> > + * @gr_curve: green pixel oecf curve in GR line.
> > + * @gb_curve: green pixel oecf curve in GB line.
> > + * @b_curve: blue pixel oecf curve.
> > + */
> > +struct jh7110_isp_oecf_setting {
> > +	__u32 enabled;
> > +	struct jh7110_isp_oecf_point r_curve[16];
> > +	struct jh7110_isp_oecf_point gr_curve[16];
> > +	struct jh7110_isp_oecf_point gb_curve[16];
> > +	struct jh7110_isp_oecf_point b_curve[16];
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_r2y_matrix - RGB to YUV color conversion matrix
> > + *
> > + * @m: The 3x3 color conversion matrix coefficient.
> > + */
> > +struct jh7110_isp_r2y_matrix {
> > +	__s16 m[9];
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_r2y_setting - Configuration used by RGB To YUV
> > + *
> > + * @enabled: enabled setting flag.
> > + * @matrix: RGB to YUV color conversion matrix.
> > + */
> > +struct jh7110_isp_r2y_setting {
> > +	__u32 enabled;
> > +	struct jh7110_isp_r2y_matrix matrix;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_sat_curve - Saturation curve
> > + *
> > + * @yi_min: the minimum input Y value.
> > + * @yo_ir: the ratio of Y output range to input range.
> > + * @yo_min: the minimum output Y value.
> > + * @yo_max: the maximum output Y value.
> > + */
> > +struct jh7110_isp_sat_curve {
> > +	__s16 yi_min;
> > +	__s16 yo_ir;
> > +	__s16 yo_min;
> > +	__s16 yo_max;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_sat_hue_info - Chroma Saturation Hue Factor
> > + *
> > + * @cos: COS hue factor.
> > + * @sin: SIN hue factor.
> > + */
> > +struct jh7110_isp_sat_hue_info {
> > +	__s16 cos;
> > +	__s16 sin;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_sat_info - Saturation information
> > + *
> > + * @gain_cmab: Chroma saturation magnitude amplification base for gain.
> > + * @gain_cmmd: Chroma saturation magnitude amplification delta for gain.
> > + * @threshold_cmb: Chroma saturation magnitude base threshold.
> > + * @threshold_cmd: Chroma saturation magnitude delta threshold.
> > + * @offset_u: Chroma saturation U offset.
> > + * @offset_v: Chroma saturation V offset.
> > + * @cmsf: Chroma saturation magnitude scaling factor.
> > + */
> > +struct jh7110_isp_sat_info {
> > +	__s16 gain_cmab;
> > +	__s16 gain_cmmd;
> > +	__s16 threshold_cmb;
> > +	__s16 threshold_cmd;
> > +	__s16 offset_u;
> > +	__s16 offset_v;
> > +	__s16 cmsf;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_sat_setting - Configuration used by Saturation
> > + *
> > + * @enabled: enabled setting flag.
> > + * @curve: Saturation curve.
> > + * @hue_info: Chroma Saturation Hue Factor.
> > + * @sat_info: Saturation information.s
> 
> informations.
> 
> > + */
> > +struct jh7110_isp_sat_setting {
> > +	__u32 enabled;
> > +	struct jh7110_isp_sat_curve curve;
> > +	struct jh7110_isp_sat_hue_info hue_info;
> > +	struct jh7110_isp_sat_info sat_info;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_sharp_weight - Sharpe weight
> > + *
> > + * @weight: Sharpen filter weight.
> > + * @recip_wei_sum: Sharpen amplification filter weight normalization factor.
> > + */
> > +struct jh7110_isp_sharp_weight {
> > +	__u8 weight[15];
> > +	__u32 recip_wei_sum;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_sharp_strength - Sharpen strength
> > + *
> > + * @diff: Sharpen Edge amplification delta level.
> > + * @f: Sharpen Edge amplification factor.
> > + * @s: Sharpen Edge amplification factor slope.
> > + */
> > +struct jh7110_isp_sharp_strength {
> > +	__s16 diff[4];
> > +	__s16 f[3];
> > +	__s32 s[3];
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_sharp_setting - Configuration used by Sharpen
> > + *
> > + * @enabled: enabled setting flag.
> > + * @weight: Sharpe weight.
> > + * @strength: Sharpen strength.
> > + * @pdirf: Positive Factor Multiplier.
> > + * @ndirf: Negative Factor Multiplier.
> > + */
> > +struct jh7110_isp_sharp_setting {
> > +	__u32 enabled;
> > +	struct jh7110_isp_sharp_weight weight;
> > +	struct jh7110_isp_sharp_strength strength;
> > +	__s8 pdirf;
> > +	__s8 ndirf;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_ycrv_curve - Y Curve parameters table
> > + *
> > + * @y: Y curve L parameters value.
> > + */
> > +struct jh7110_isp_ycrv_curve {
> > +	__s16 y[64];
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_ycrv_setting - Configuration used by Y Curve
> > + *
> > + * @enabled: enabled setting flag.
> > + * @curve: Y Curve parameters table.
> > + */
> > +struct jh7110_isp_ycrv_setting {
> > +	__u32 enabled;
> > +	struct jh7110_isp_ycrv_curve curve;
> 
> I am a bit failing in seeing the point of embedding the settings in a
> dedicated structure when you have a single instance of the
> configuration like this and in other instances. Isn't
> 
>         struct jh7110_isp_ycrv_setting {
>                 __u32 enabled;
>                 __s16 y[64];
>         };
> 
> easier ? Or do you need a dedicated type for other reasons ?
> 
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_sc_config - statistics collection crop configure
> > + *
> > + * @h_start: Horizontal starting point for frame cropping.
> > + * @v_start: Vertical starting point for frame cropping.
> > + * @sw_width: Width of statistics collection sub-window.
> > + * @sw_height: Height of statistics collection sub-window.
> > + * @hperiod: Horizontal period.
> > + * @hkeep: Horizontal keep.
> > + * @vperiod: Vertical period.
> > + * @vkeep: Vertical keep.
> > + */
> > +struct jh7110_isp_sc_config {
> > +	__u16 h_start;
> > +	__u16 v_start;
> > +	__u8 sw_width;
> > +	__u8 sw_height;
> > +	__u8 hperiod;
> > +	__u8 hkeep;
> > +	__u8 vperiod;
> > +	__u8 vkeep;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_sc_af_config - statistics collection auto focus configure
> > + *
> > + * @es_hor_mode: Horizontal mode.
> > + * @es_sum_mode: sum mode.
> 
> Other fields are documented with a capital letter -> "Sum mode."
> 
> > + * @hor_en: Horizontal enable.
> > + * @ver_en: Vertical enable.
> > + * @es_ver_thr: Vertical threshold.
> > + * @es_hor_thr: Horizontal threshold.
> > + */
> > +struct jh7110_isp_sc_af_config {
> > +	__u8 es_hor_mode;
> > +	__u8 es_sum_mode;
> > +	__u8 hor_en;
> > +	__u8 ver_en;
> > +	__u8 es_ver_thr;
> > +	__u16 es_hor_thr;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_sc_awb_ps - statistics collection auto white balance pixel sum
> > + *
> > + * @awb_ps_rl: Lower boundary of R value for pixel sum.
> > + * @awb_ps_ru: Upper boundary of R value for pixel sum.
> > + * @awb_ps_gl: Lower boundary of G value for pixel sum.
> > + * @awb_ps_gu: Upper boundary of G value for pixel sum.
> > + * @awb_ps_bl: Lower boundary of B value for pixel sum.
> > + * @awb_ps_bu: Upper boundary of B value for pixel sum.
> > + * @awb_ps_yl: Lower boundary of Y value for pixel sum.
> > + * @awb_ps_yu: Upper boundary of Y value for pixel sum.
> > + * @awb_ps_grl: Lower boundary of G/R ratio for pixel sum.
> > + * @awb_ps_gru: Upper boundary of G/R ratio for pixel sum.
> > + * @awb_ps_gbl: Lower boundary of G/B ratio for pixel sum.
> > + * @awb_ps_gbu: Upper boundary of G/B ratio for pixel sum.
> > + * @awb_ps_grbl: Lower boundary of (Gr/R + b/a * Gb/B) for pixel sum.
> > + * @awb_ps_grbu: Upper boundary of (Gr/R + b/a * Gb/B) for pixel sum.
> > + */
> > +struct jh7110_isp_sc_awb_ps {
> > +	__u8 awb_ps_rl;
> > +	__u8 awb_ps_ru;
> > +	__u8 awb_ps_gl;
> > +	__u8 awb_ps_gu;
> > +	__u8 awb_ps_bl;
> > +	__u8 awb_ps_bu;
> > +	__u8 awb_ps_yl;
> > +	__u8 awb_ps_yu;
> > +	__u16 awb_ps_grl;
> > +	__u16 awb_ps_gru;
> > +	__u16 awb_ps_gbl;
> > +	__u16 awb_ps_gbu;
> > +	__u16 awb_ps_grbl;
> > +	__u16 awb_ps_grbu;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_sc_awb_ws - statistics collection auto white balance weight sum
> > + *
> > + * @awb_ws_rl: Lower boundary of R value for weight sum.
> > + * @awb_ws_ru: Upper boundary of R value for weight sum.
> > + * @awb_ws_grl: Lower boundary of Gr value for weight sum.
> > + * @awb_ws_gru: Upper boundary of Gr value for weight sum.
> > + * @awb_ws_gbl: Lower boundary of Gb value for weight sum.
> > + * @awb_ws_gbu: Upper boundary of Gb value for weight sum.
> > + * @awb_ws_bl: Lower boundary of B value for weight sum.
> > + * @awb_ws_bu: Upper boundary of B value for weight sum.
> > + */
> > +struct jh7110_isp_sc_awb_ws {
> > +	__u8 awb_ws_rl;
> > +	__u8 awb_ws_ru;
> > +	__u8 awb_ws_grl;
> > +	__u8 awb_ws_gru;
> > +	__u8 awb_ws_gbl;
> > +	__u8 awb_ws_gbu;
> > +	__u8 awb_ws_bl;
> > +	__u8 awb_ws_bu;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_sc_awb_point - statistics collection auto white balance point
> > + *
> > + * @weight: Weighting value at point.
> > + */
> > +struct jh7110_isp_sc_awb_point {
> > +	__u8 weight;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_sc_awb_config - statistics collection auto white balance configure
> > + *
> > + * @ps_config: statistics collection auto white balance pixel sum.
> 
> nit: please be consistent with using capital letters or not in doc.
> 
> > + * @awb_ps_grb_ba: auto white balance b/a value.
> > + * @sel: input mux for statistics collection auto white balance.
> > + * @ws_config: statistics collection auto white balance weight sum.
> > + * @awb_cw: Weighting value at 13x13 point.
> > + * @pts: statistics collection auto white balance point.
> > + */
> > +struct jh7110_isp_sc_awb_config {
> > +	struct jh7110_isp_sc_awb_ps ps_config;
> > +	__u8 awb_ps_grb_ba;
> > +	__u8 sel;
> > +	struct jh7110_isp_sc_awb_ws ws_config;
> > +	__u8 awb_cw[169];
> > +	struct jh7110_isp_sc_awb_point pts[17];
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_sc_setting - Configuration used by statistics collection
> > + *
> > + * @enabled: enabled setting flag.
> > + * @crop_config: statistics collection crop configure.
> > + * @af_config: statistics collection auto focus configure.
> > + * @awb_config: statistics collection auto white balance configure.
> > + */
> > +struct jh7110_isp_sc_setting {
> > +	__u32 enabled;
> > +	struct jh7110_isp_sc_config crop_config;
> > +	struct jh7110_isp_sc_af_config af_config;
> > +	struct jh7110_isp_sc_awb_config awb_config;
> > +};
> > +
> > +/**
> > + * struct jh7110_isp_params_buffer - StarFive JH7110 ISP Parameters Meta Data
> > + *
> > + * @enable_setting: enabled setting module (JH7110_ISP_MODULE_* definitions).
> > + * @wb_setting: Configuration used by auto white balance gain.
> > + * @car_setting: Configuration used by color artifact removal.
> > + * @ccm_setting: Configuration used by color correction matrix.
> > + * @cfa_setting: Configuration used by demosaic module.
> > + * @ctc_setting: Configuration used by crosstalk remove.
> > + * @dbc_setting: Configuration used by defect bad pixels correction.
> > + * @dnyuv_setting: Configuration used by yuv domain denoise.
> > + * @gmargb_setting: Configuration used by RGB gamma.
> > + * @lccf_setting: Configuration used by lens correction cosine fourth.
> > + * @obc_setting: Configuration used by optical black compensation.
> > + * @oecf_setting: Configuration used by opto-electric conversion function.
> > + * @r2y_setting: Configuration used by RGB To YUV.
> > + * @sat_setting: Configuration used by Saturation.
> > + * @sharp_setting: Configuration used by Sharpen.
> > + * @ycrv_setting: Configuration used by Y Curve.
> > + * @sc_setting: Configuration used by statistics collection.
> > + */
> > +struct jh7110_isp_params_buffer {
> > +	__u32 enable_setting;
> > +	struct jh7110_isp_wb_setting wb_setting;
> > +	struct jh7110_isp_car_setting car_setting;
> > +	struct jh7110_isp_ccm_setting ccm_setting;
> > +	struct jh7110_isp_cfa_setting cfa_setting;
> > +	struct jh7110_isp_ctc_setting ctc_setting;
> > +	struct jh7110_isp_dbc_setting dbc_setting;
> > +	struct jh7110_isp_dnyuv_setting dnyuv_setting;
> > +	struct jh7110_isp_gmargb_setting gmargb_setting;
> > +	struct jh7110_isp_lccf_setting lccf_setting;
> > +	struct jh7110_isp_obc_setting obc_setting;
> > +	struct jh7110_isp_oecf_setting oecf_setting;
> > +	struct jh7110_isp_r2y_setting r2y_setting;
> > +	struct jh7110_isp_sat_setting sat_setting;
> > +	struct jh7110_isp_sharp_setting sharp_setting;
> > +	struct jh7110_isp_ycrv_setting ycrv_setting;
> > +	struct jh7110_isp_sc_setting sc_setting;
> > +};
> > +
> > +/**
> > + * Statistics Collection Meta Data Flag
> > + */
> > +#define JH7110_ISP_SC_FLAG_AWB			0x0
> > +#define JH7110_ISP_SC_FLAG_AE_AF		0xffff
> > +
> > +#pragma pack(1)
> > +
> > +/**
> > + * struct jh7110_isp_sc_buffer - StarFive JH7110 ISP Statistics Collection Meta Data
> > + *
> > + * @y_histogram: Y histogram data for saturation control.
> > + * @reserv0: reserve byte.
> > + * @bright_sc: bright statistic. If flag is JH7110_ISP_SC_FLAG_AE_AF, This field is
> 
> s/bright statistics/brightness statistics/
> 
> no capital "T" after ,
> 
> > + *             saved auto exposure and auto focus. If flag is JH7110_ISP_SC_FLAG_AWB,
> > + *             This field is saved auto white balance.
> 
> no capital "T" after ,
> 
> I would replace "this field is saved" which doesn't sound great in
> English (not a native speaker though) with "this field stores".
> 
> > + * @reserv1: reserve byte.
> > + * @ae_hist_y: Y histogram for auto exposure.
> > + * @reserv2: reserve byte.
> > + * @flag: Statistics Collection Meta Data Flag (JH7110_ISP_SC_FLAG_* definitions)
> > + */
> > +struct jh7110_isp_sc_buffer {
> > +	__u32 y_histogram[64];
> > +	__u32 reserv0[33];
> > +	__u32 bright_sc[4096];
> > +	__u32 reserv1[96];
> > +	__u32 ae_hist_y[128];
> > +	__u32 reserv2[511];
> > +	__u16 flag;
> > +};
> > +
> > +#pragma pack()
> 
> This structure is packed, is it populated directly from HW registers
> with a memcpy or a DMA transfer ? I guess I'll find it out in the next
> patches.
> 
> > +
> > +#endif
  

On Mon, Jul 22, 2024 at 05:27:26PM +0300, Laurent Pinchart wrote:
> On Wed, Jul 10, 2024 at 11:11:57AM +0200, Jacopo Mondi wrote:
> > On Tue, Jul 09, 2024 at 01:38:11AM GMT, Changhuang Liang wrote:
> > > Add JH7110 ISP module definitions for user space.
> > >
> > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > Signed-off-by: Zejian Su <zejian.su@starfivetech.com>
> > > ---
> > >  MAINTAINERS                     |   1 +
> > >  include/uapi/linux/jh7110-isp.h | 739 ++++++++++++++++++++++++++++++++
> > 
> > With the recently merged support for the RaspberryPi PiSP BE we have
> > introduced include/uapi/linux/media/raspberry.
> > 
> > Would you consider placing this in
> > include/uapi/linux/media/startfive/ ?
> 
> That sounds like a good idea.
> 
> > >  2 files changed, 740 insertions(+)
> > >  create mode 100644 include/uapi/linux/jh7110-isp.h
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 507f04a80499..890604eb0d64 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -21305,6 +21305,7 @@ S:	Maintained
> > >  F:	Documentation/admin-guide/media/starfive_camss.rst
> > >  F:	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
> > >  F:	drivers/staging/media/starfive/camss
> > > +F:	include/uapi/linux/jh7110-isp.h
> > >
> > >  STARFIVE CRYPTO DRIVER
> > >  M:	Jia Jie Ho <jiajie.ho@starfivetech.com>
> > > diff --git a/include/uapi/linux/jh7110-isp.h b/include/uapi/linux/jh7110-isp.h
> > > new file mode 100644
> > > index 000000000000..4939cd63e771
> > > --- /dev/null
> > > +++ b/include/uapi/linux/jh7110-isp.h
> > > @@ -0,0 +1,739 @@
> > > +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR BSD-3-Clause) */
> > > +/*
> > > + * jh7110-isp.h
> > > + *
> > > + * JH7110 ISP driver - user space header file.
> > > + *
> > > + * Copyright © 2023 StarFive Technology Co., Ltd.
> > > + *
> > > + * Author: Zejian Su <zejian.su@starfivetech.com>
> > > + *
> > > + */
> > > +
> > > +#ifndef __JH7110_ISP_H_
> > > +#define __JH7110_ISP_H_
> > > +
> > 
> > Do you need to include
> > 
> > #include <linux/types.h>
> > 
> > > +/**
> > 
> > Is this kernel-doc or a single * would do ?
> > 
> > > + * ISP Module Diagram
> > > + * ------------------
> > > + *
> > > + *  Raw  +-----+    +------+    +------+    +----+
> > > + *  ---->| OBC |--->| OECF |--->| LCCF |--->| WB |-----+
> > > + *       +-----+    +------+    +------+    +----+     |
> > > + *                                                     |
> > > + *  +--------------------------------------------------+
> > > + *  |
> > > + *  |    +-----+    +-----+    +-----+    +-----+
> > > + *  +--->| DBC |--->| CTC |--->| CFA |--->| CAR |------+
> > > + *       +-----+    +-----+    +-----+    +-----+      |
> > > + *                                                     |
> > > + *  +--------------------------------------------------+
> > > + *  |
> > > + *  |    +-----+    +--------+    +-----+    +------+
> > > + *  +--->| CCM |--->| GMARGB |--->| R2Y |--->| YCRV |--+
> > > + *       +-----+    +--------+    +-----+    +------+  |
> > > + *                                                     |
> > > + *  +--------------------------------------------------+
> > > + *  |
> > > + *  |    +-------+    +-------+    +-----+    +----+
> > > + *  +--->| SHARP |--->| DNYUV |--->| SAT |--->| SC |
> > > + *       +-------+    +-------+    +-----+    +----+
> > > + *
> 
> The diagram is useful, thank you. A glossary would also be nice, maybe
> as
> 
>  * - OBC: Optical Black Correction
>  * - OECF: Opto-Electric Conversion Function
>  * ...
> 
> I think that would be easier to read than the comments above each macro
> below. Up to you.
> 
> > > + */
> > > +
> > > +/* Auto White Balance */
> > > +#define JH7110_ISP_MODULE_WB_SETTING			(1U << 0)
> > > +/* Color Artifact Removal */
> > > +#define JH7110_ISP_MODULE_CAR_SETTING			(1U << 1)
> > > +/* Color Correction Matrix */
> > > +#define JH7110_ISP_MODULE_CCM_SETTING			(1U << 2)
> > > +/* Color Filter Arrays */
> > > +#define JH7110_ISP_MODULE_CFA_SETTING			(1U << 3)
> > > +/* Crosstalk Correction */
> > > +#define JH7110_ISP_MODULE_CTC_SETTING			(1U << 4)
> > > +/* Defect Bad Pixel Correction */
> > > +#define JH7110_ISP_MODULE_DBC_SETTING			(1U << 5)
> > > +/* Denoise YUV */
> > > +#define JH7110_ISP_MODULE_DNYUV_SETTING			(1U << 6)
> > > +/* RGB Gamma */
> > > +#define JH7110_ISP_MODULE_GMARGB_SETTING		(1U << 7)
> > > +/* Lens Correction Cosine Fourth */
> > > +#define JH7110_ISP_MODULE_LCCF_SETTING			(1U << 8)
> > > +/* Optical Black Correction */
> > > +#define JH7110_ISP_MODULE_OBC_SETTING			(1U << 9)
> > > +/* Opto-Electric Conversion Function */
> > > +#define JH7110_ISP_MODULE_OECF_SETTING			(1U << 10)
> > > +/* RGB To YUV */
> > > +#define JH7110_ISP_MODULE_R2Y_SETTING			(1U << 11)
> > > +/* Saturation */
> > > +#define JH7110_ISP_MODULE_SAT_SETTING			(1U << 12)
> > > +/* Sharpen */
> > > +#define JH7110_ISP_MODULE_SHARP_SETTING			(1U << 13)
> > > +/* Y Curve */
> > > +#define JH7110_ISP_MODULE_YCRV_SETTING			(1U << 14)
> > > +/* Statistics Collection */
> > > +#define JH7110_ISP_MODULE_SC_SETTING			(1U << 15)
> 
> Unless there's a specific reason to keep the current order, maybe you
> could sort those macros in the same order as in the module diagram ?
> 
> > > +
> > > +/**
> > > + * struct jh7110_isp_wb_gain - auto white balance gain
> > > + *
> > > + * @gain_r: gain value for red component.
> > > + * @gain_g: gain value for green component.
> > > + * @gain_b: gain value for blue component.
> 
> I suppose the gains are expressed as fixed-point integers. This needs
> more details, what are the limits, and how many bits of integer and
> fractional parts are there ?
> 
> Same comment for all the other values below.
> 
> > > + */
> > > +struct jh7110_isp_wb_gain {
> > > +	__u16 gain_r;
> > > +	__u16 gain_g;
> > > +	__u16 gain_b;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_wb_setting - Configuration used by auto white balance gain
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @gains: auto white balance gain setting.
> > > + */
> > > +struct jh7110_isp_wb_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_wb_gain gains;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_car_setting - Configuration used by color artifact removal
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + */
> > > +struct jh7110_isp_car_setting {
> > > +	__u32 enabled;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_ccm_smlow - Color correction matrix
> > > + *
> > > + * @ccm: color transform matrix with size 3 by 3.
> > > + * @offsets: the offsets of R, G, B after the transform by the ccm.
> > > + */
> > > +struct jh7110_isp_ccm_smlow {
> > > +	__s32 ccm[3][3];
> > > +	__s32 offsets[3];
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_ccm_setting - Configuration used by color correction matrix
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @ccm_smlow: Color correction matrix.
> > > + */
> > > +struct jh7110_isp_ccm_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_ccm_smlow ccm_smlow;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_cfa_params - demosaic parameters
> > > + *
> > > + * @hv_width: detail smooth factor
> > > + * @cross_cov: Cross covariance weighting.
> 
> This documentation doesn't tell how to use those paraemeters. This
> comment applies to many other parameters below. There are three main
> options to improve that:
> 
> - Expanding the documentation in this header file to clearly explain how
>   to compute the parameters values.

Here's an example of the level of details that would be expected:
https://lore.kernel.org/linux-media/20240709132906.3198927-19-dan.scally@ideasonboard.com/T/#m1bc3fe5b5cea831ece6452b13225d291bc4619db

> 
> - Providing an open userspace implementation of ISP algorithms that
>   showcase how to calculate the values.
> 
> - Providing detailed hardware documentation for the ISP. This is usually
>   not favoured by ISP vendors, and we are not pushing for this, but I
>   wanted to mention it for completeness.
> 
> If you would prefer the second option, any open-source camera framework
> would be acceptable, but in practice the only real option is likely
> libcamera.
> 
> This does not mean that you have to open-source all your ISP control
> algorithms. Only the code needed to explain how ISP parameters are
> applied to the image and are computed is needed. Other parts, such as
> for instance AI-based computation of white balance gains, or complex AGC
> calculations, don't need to be open-source.
> 
> The explain this requirement different and perhaps more clearly, the
> goal is to make sure that developers who have access to only open-source
> code (ISP kernel driver, this header, any open-source userspace code,
> ...) will have enough information to configure and control the ISP.
> 
> > > + */
> > > +struct jh7110_isp_cfa_params {
> > > +	__s32 hv_width;
> > > +	__s32 cross_cov;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_cfa_params - Configuration used by demosaic module
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @settings: demosaic parameters.
> > > + */
> > > +struct jh7110_isp_cfa_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_cfa_params settings;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_ctc_params - crosstalk remove parameters
> > > + *
> > > + * @saf_mode: smooth area filter mode.
> > > + * @daf_mode: detail area filter mode.
> > > + * @max_gt: the threshold for imbalance detection when pixel intensity is close to maximum.
> > 
> > You could easily make this < 80 cols (here and in other places).
> > 
> > I know there are different opinions on how strict on the 80 cols limit
> > we should be, so up to you.
> > 
> > > + * @min_gt: the threshold for imbalance detection when pixel intensity is close to 0.
> > > + */
> > > +struct jh7110_isp_ctc_params {
> > > +	__u8 saf_mode;
> > > +	__u8 daf_mode;
> > > +	__s32 max_gt;
> > > +	__s32 min_gt;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_ctc_params - Configuration used by crosstalk remove
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @settings: corsstalk remove parameters.
> > 
> > crosstalk
> > 
> > > + */
> > > +struct jh7110_isp_ctc_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_ctc_params settings;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_dbc_params - defect pixels correction parameters
> > > + *
> > > + * @bad_gt: bad pixel threshold for the green channel.
> > > + * @bad_xt: bad pixel threshold for the red and blue channels.
> > > + */
> > > +struct jh7110_isp_dbc_params {
> > > +	__s32 bad_gt;
> > > +	__s32 bad_xt;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_dbc_params - Configuration used by defect bad pixels correction
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @settings: defect pixels correction parameters.
> > > + */
> > > +struct jh7110_isp_dbc_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_dbc_params settings;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_dnyuv_params - yuv domain denoise parameters
> > > + *
> > > + * @y_sweight: ten coefficients of 7x7 spatial filter for Y channel.
> > > + * @y_curve: intensity difference (similarity) weight lookup table for Y channel.
> > > + * @uv_sweight: ten coefficients of 7x7 spatial filter for U and V channel.
> > > + * @uv_curve: intensity difference (similarity) weight lookup table for U and V channel.
> > > + */
> > > +struct jh7110_isp_dnyuv_params {
> > > +	__u8 y_sweight[10];
> > > +	__u16 y_curve[7];
> > > +	__u8 uv_sweight[10];
> > > +	__u16 uv_curve[7];
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_dnyuv_params - Configuration used by yuv domain denoise
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @settings: yuv domain denoise parameters.
> > > + */
> > > +struct jh7110_isp_dnyuv_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_dnyuv_params settings;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_gmargb_point - RGB Gamma point
> > > + *
> > > + * @g_val: RGB gamma value.
> > > + * @sg_val: RGB gamma slope value.
> > > + */
> > > +struct jh7110_isp_gmargb_point {
> > > +	__u16 g_val;
> > > +	__u16 sg_val;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_gmargb_setting - Configuration used by RGB gamma
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @curve: RGB Gamma point table.
> > > + */
> > > +struct jh7110_isp_gmargb_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_gmargb_point curve[15];
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_lccf_circle - len circle
> > 
> > lens ?
> > 
> > > + *
> > > + * @center_x: center X distance from capture window.
> > > + * @center_y: center Y distance from capture window.
> > > + * @radius: len circle radius.
> > 
> > here as well
> > 
> > > + */
> > > +struct jh7110_isp_lccf_circle {
> > > +	__s16 center_x;
> > > +	__s16 center_y;
> > > +	__u8 radius;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_lccf_curve_param - lens correction cosine fourth curve param
> > > + *
> > > + * @f1: F1 parameter.
> > > + * @f2: F2 parameter.
> > > + */
> > > +struct jh7110_isp_lccf_curve_param {
> > > +	__s16 f1;
> > > +	__s16 f2;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_lccf_setting - Configuration used by lens correction cosine fourth
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @circle: len circle.
> > > + * @r_param: lens correction cosine fourth curve param for Bayer pattern R.
> > > + * @gr_param: lens correction cosine fourth curve param for Bayer pattern Gr.
> > > + * @gb_param: lens correction cosine fourth curve param for Bayer pattern Gb.
> > > + * @b_param: lens correction cosine fourth curve param for Bayer pattern B.
> > > + */
> > > +struct jh7110_isp_lccf_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_lccf_circle circle;
> > > +	struct jh7110_isp_lccf_curve_param r_param;
> > > +	struct jh7110_isp_lccf_curve_param gr_param;
> > > +	struct jh7110_isp_lccf_curve_param gb_param;
> > > +	struct jh7110_isp_lccf_curve_param b_param;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_obc_win_size - optical black correction window size
> > > + *
> > > + * @width: window width.
> > > + * @height: window height.
> > > + */
> > > +struct jh7110_isp_obc_win_size {
> > > +	__u32 width;
> > > +	__u32 height;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_obc_gain - optical black correction symbol gain
> > > + *
> > > + * @tl_gain: gain at point A for symbol.
> > > + * @tr_gain: gain at point B for symbol.
> > > + * @bl_gain: gain at point C for symbol.
> > > + * @br_gain: gain at point D for symbol.
> > > + */
> > > +struct jh7110_isp_obc_gain {
> > > +	__u8 tl_gain;
> > > +	__u8 tr_gain;
> > > +	__u8 bl_gain;
> > > +	__u8 br_gain;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_obc_offset - optical black correction symbol offset
> > > + *
> > > + * @tl_offset: offset at point A for symbol.
> > > + * @tr_offset: offset at point B for symbol.
> > > + * @bl_offset: offset at point C for symbol.
> > > + * @br_offset: offset at point D for symbol.
> > > + */
> > > +struct jh7110_isp_obc_offset {
> > > +	__u8 tl_offset;
> > > +	__u8 tr_offset;
> > > +	__u8 bl_offset;
> > > +	__u8 br_offset;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_obc_setting - Configuration used by optical black correction
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @win_size: optical black correction window size.
> > > + * @gain: optical black correction symbol gain.
> > > + * @offset: optical black correction symbol offset.
> > > + */
> > > +struct jh7110_isp_obc_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_obc_win_size win_size;
> > > +	struct jh7110_isp_obc_gain gain[4];
> > > +	struct jh7110_isp_obc_offset offset[4];
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_oecf_point - oecf curve
> > > + *
> > > + * @x: x coordinate.
> > > + * @y: y coordinate.
> > > + * @slope: the slope between this point and the next point.
> > > + */
> > > +struct jh7110_isp_oecf_point {
> > > +	__u16 x;
> > > +	__u16 y;
> > > +	__s16 slope;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_oecf_setting - Configuration used by opto-electric conversion function
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @r_curve: red pixel oecf curve.
> > > + * @gr_curve: green pixel oecf curve in GR line.
> > > + * @gb_curve: green pixel oecf curve in GB line.
> > > + * @b_curve: blue pixel oecf curve.
> > > + */
> > > +struct jh7110_isp_oecf_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_oecf_point r_curve[16];
> > > +	struct jh7110_isp_oecf_point gr_curve[16];
> > > +	struct jh7110_isp_oecf_point gb_curve[16];
> > > +	struct jh7110_isp_oecf_point b_curve[16];
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_r2y_matrix - RGB to YUV color conversion matrix
> > > + *
> > > + * @m: The 3x3 color conversion matrix coefficient.
> > > + */
> > > +struct jh7110_isp_r2y_matrix {
> > > +	__s16 m[9];
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_r2y_setting - Configuration used by RGB To YUV
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @matrix: RGB to YUV color conversion matrix.
> > > + */
> > > +struct jh7110_isp_r2y_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_r2y_matrix matrix;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_sat_curve - Saturation curve
> > > + *
> > > + * @yi_min: the minimum input Y value.
> > > + * @yo_ir: the ratio of Y output range to input range.
> > > + * @yo_min: the minimum output Y value.
> > > + * @yo_max: the maximum output Y value.
> > > + */
> > > +struct jh7110_isp_sat_curve {
> > > +	__s16 yi_min;
> > > +	__s16 yo_ir;
> > > +	__s16 yo_min;
> > > +	__s16 yo_max;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_sat_hue_info - Chroma Saturation Hue Factor
> > > + *
> > > + * @cos: COS hue factor.
> > > + * @sin: SIN hue factor.
> > > + */
> > > +struct jh7110_isp_sat_hue_info {
> > > +	__s16 cos;
> > > +	__s16 sin;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_sat_info - Saturation information
> > > + *
> > > + * @gain_cmab: Chroma saturation magnitude amplification base for gain.
> > > + * @gain_cmmd: Chroma saturation magnitude amplification delta for gain.
> > > + * @threshold_cmb: Chroma saturation magnitude base threshold.
> > > + * @threshold_cmd: Chroma saturation magnitude delta threshold.
> > > + * @offset_u: Chroma saturation U offset.
> > > + * @offset_v: Chroma saturation V offset.
> > > + * @cmsf: Chroma saturation magnitude scaling factor.
> > > + */
> > > +struct jh7110_isp_sat_info {
> > > +	__s16 gain_cmab;
> > > +	__s16 gain_cmmd;
> > > +	__s16 threshold_cmb;
> > > +	__s16 threshold_cmd;
> > > +	__s16 offset_u;
> > > +	__s16 offset_v;
> > > +	__s16 cmsf;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_sat_setting - Configuration used by Saturation
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @curve: Saturation curve.
> > > + * @hue_info: Chroma Saturation Hue Factor.
> > > + * @sat_info: Saturation information.s
> > 
> > informations.
> > 
> > > + */
> > > +struct jh7110_isp_sat_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_sat_curve curve;
> > > +	struct jh7110_isp_sat_hue_info hue_info;
> > > +	struct jh7110_isp_sat_info sat_info;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_sharp_weight - Sharpe weight
> > > + *
> > > + * @weight: Sharpen filter weight.
> > > + * @recip_wei_sum: Sharpen amplification filter weight normalization factor.
> > > + */
> > > +struct jh7110_isp_sharp_weight {
> > > +	__u8 weight[15];
> > > +	__u32 recip_wei_sum;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_sharp_strength - Sharpen strength
> > > + *
> > > + * @diff: Sharpen Edge amplification delta level.
> > > + * @f: Sharpen Edge amplification factor.
> > > + * @s: Sharpen Edge amplification factor slope.
> > > + */
> > > +struct jh7110_isp_sharp_strength {
> > > +	__s16 diff[4];
> > > +	__s16 f[3];
> > > +	__s32 s[3];
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_sharp_setting - Configuration used by Sharpen
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @weight: Sharpe weight.
> > > + * @strength: Sharpen strength.
> > > + * @pdirf: Positive Factor Multiplier.
> > > + * @ndirf: Negative Factor Multiplier.
> > > + */
> > > +struct jh7110_isp_sharp_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_sharp_weight weight;
> > > +	struct jh7110_isp_sharp_strength strength;
> > > +	__s8 pdirf;
> > > +	__s8 ndirf;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_ycrv_curve - Y Curve parameters table
> > > + *
> > > + * @y: Y curve L parameters value.
> > > + */
> > > +struct jh7110_isp_ycrv_curve {
> > > +	__s16 y[64];
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_ycrv_setting - Configuration used by Y Curve
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @curve: Y Curve parameters table.
> > > + */
> > > +struct jh7110_isp_ycrv_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_ycrv_curve curve;
> > 
> > I am a bit failing in seeing the point of embedding the settings in a
> > dedicated structure when you have a single instance of the
> > configuration like this and in other instances. Isn't
> > 
> >         struct jh7110_isp_ycrv_setting {
> >                 __u32 enabled;
> >                 __s16 y[64];
> >         };
> > 
> > easier ? Or do you need a dedicated type for other reasons ?
> > 
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_sc_config - statistics collection crop configure
> > > + *
> > > + * @h_start: Horizontal starting point for frame cropping.
> > > + * @v_start: Vertical starting point for frame cropping.
> > > + * @sw_width: Width of statistics collection sub-window.
> > > + * @sw_height: Height of statistics collection sub-window.
> > > + * @hperiod: Horizontal period.
> > > + * @hkeep: Horizontal keep.
> > > + * @vperiod: Vertical period.
> > > + * @vkeep: Vertical keep.
> > > + */
> > > +struct jh7110_isp_sc_config {
> > > +	__u16 h_start;
> > > +	__u16 v_start;
> > > +	__u8 sw_width;
> > > +	__u8 sw_height;
> > > +	__u8 hperiod;
> > > +	__u8 hkeep;
> > > +	__u8 vperiod;
> > > +	__u8 vkeep;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_sc_af_config - statistics collection auto focus configure
> > > + *
> > > + * @es_hor_mode: Horizontal mode.
> > > + * @es_sum_mode: sum mode.
> > 
> > Other fields are documented with a capital letter -> "Sum mode."
> > 
> > > + * @hor_en: Horizontal enable.
> > > + * @ver_en: Vertical enable.
> > > + * @es_ver_thr: Vertical threshold.
> > > + * @es_hor_thr: Horizontal threshold.
> > > + */
> > > +struct jh7110_isp_sc_af_config {
> > > +	__u8 es_hor_mode;
> > > +	__u8 es_sum_mode;
> > > +	__u8 hor_en;
> > > +	__u8 ver_en;
> > > +	__u8 es_ver_thr;
> > > +	__u16 es_hor_thr;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_sc_awb_ps - statistics collection auto white balance pixel sum
> > > + *
> > > + * @awb_ps_rl: Lower boundary of R value for pixel sum.
> > > + * @awb_ps_ru: Upper boundary of R value for pixel sum.
> > > + * @awb_ps_gl: Lower boundary of G value for pixel sum.
> > > + * @awb_ps_gu: Upper boundary of G value for pixel sum.
> > > + * @awb_ps_bl: Lower boundary of B value for pixel sum.
> > > + * @awb_ps_bu: Upper boundary of B value for pixel sum.
> > > + * @awb_ps_yl: Lower boundary of Y value for pixel sum.
> > > + * @awb_ps_yu: Upper boundary of Y value for pixel sum.
> > > + * @awb_ps_grl: Lower boundary of G/R ratio for pixel sum.
> > > + * @awb_ps_gru: Upper boundary of G/R ratio for pixel sum.
> > > + * @awb_ps_gbl: Lower boundary of G/B ratio for pixel sum.
> > > + * @awb_ps_gbu: Upper boundary of G/B ratio for pixel sum.
> > > + * @awb_ps_grbl: Lower boundary of (Gr/R + b/a * Gb/B) for pixel sum.
> > > + * @awb_ps_grbu: Upper boundary of (Gr/R + b/a * Gb/B) for pixel sum.
> > > + */
> > > +struct jh7110_isp_sc_awb_ps {
> > > +	__u8 awb_ps_rl;
> > > +	__u8 awb_ps_ru;
> > > +	__u8 awb_ps_gl;
> > > +	__u8 awb_ps_gu;
> > > +	__u8 awb_ps_bl;
> > > +	__u8 awb_ps_bu;
> > > +	__u8 awb_ps_yl;
> > > +	__u8 awb_ps_yu;
> > > +	__u16 awb_ps_grl;
> > > +	__u16 awb_ps_gru;
> > > +	__u16 awb_ps_gbl;
> > > +	__u16 awb_ps_gbu;
> > > +	__u16 awb_ps_grbl;
> > > +	__u16 awb_ps_grbu;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_sc_awb_ws - statistics collection auto white balance weight sum
> > > + *
> > > + * @awb_ws_rl: Lower boundary of R value for weight sum.
> > > + * @awb_ws_ru: Upper boundary of R value for weight sum.
> > > + * @awb_ws_grl: Lower boundary of Gr value for weight sum.
> > > + * @awb_ws_gru: Upper boundary of Gr value for weight sum.
> > > + * @awb_ws_gbl: Lower boundary of Gb value for weight sum.
> > > + * @awb_ws_gbu: Upper boundary of Gb value for weight sum.
> > > + * @awb_ws_bl: Lower boundary of B value for weight sum.
> > > + * @awb_ws_bu: Upper boundary of B value for weight sum.
> > > + */
> > > +struct jh7110_isp_sc_awb_ws {
> > > +	__u8 awb_ws_rl;
> > > +	__u8 awb_ws_ru;
> > > +	__u8 awb_ws_grl;
> > > +	__u8 awb_ws_gru;
> > > +	__u8 awb_ws_gbl;
> > > +	__u8 awb_ws_gbu;
> > > +	__u8 awb_ws_bl;
> > > +	__u8 awb_ws_bu;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_sc_awb_point - statistics collection auto white balance point
> > > + *
> > > + * @weight: Weighting value at point.
> > > + */
> > > +struct jh7110_isp_sc_awb_point {
> > > +	__u8 weight;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_sc_awb_config - statistics collection auto white balance configure
> > > + *
> > > + * @ps_config: statistics collection auto white balance pixel sum.
> > 
> > nit: please be consistent with using capital letters or not in doc.
> > 
> > > + * @awb_ps_grb_ba: auto white balance b/a value.
> > > + * @sel: input mux for statistics collection auto white balance.
> > > + * @ws_config: statistics collection auto white balance weight sum.
> > > + * @awb_cw: Weighting value at 13x13 point.
> > > + * @pts: statistics collection auto white balance point.
> > > + */
> > > +struct jh7110_isp_sc_awb_config {
> > > +	struct jh7110_isp_sc_awb_ps ps_config;
> > > +	__u8 awb_ps_grb_ba;
> > > +	__u8 sel;
> > > +	struct jh7110_isp_sc_awb_ws ws_config;
> > > +	__u8 awb_cw[169];
> > > +	struct jh7110_isp_sc_awb_point pts[17];
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_sc_setting - Configuration used by statistics collection
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @crop_config: statistics collection crop configure.
> > > + * @af_config: statistics collection auto focus configure.
> > > + * @awb_config: statistics collection auto white balance configure.
> > > + */
> > > +struct jh7110_isp_sc_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_sc_config crop_config;
> > > +	struct jh7110_isp_sc_af_config af_config;
> > > +	struct jh7110_isp_sc_awb_config awb_config;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_params_buffer - StarFive JH7110 ISP Parameters Meta Data
> > > + *
> > > + * @enable_setting: enabled setting module (JH7110_ISP_MODULE_* definitions).
> > > + * @wb_setting: Configuration used by auto white balance gain.
> > > + * @car_setting: Configuration used by color artifact removal.
> > > + * @ccm_setting: Configuration used by color correction matrix.
> > > + * @cfa_setting: Configuration used by demosaic module.
> > > + * @ctc_setting: Configuration used by crosstalk remove.
> > > + * @dbc_setting: Configuration used by defect bad pixels correction.
> > > + * @dnyuv_setting: Configuration used by yuv domain denoise.
> > > + * @gmargb_setting: Configuration used by RGB gamma.
> > > + * @lccf_setting: Configuration used by lens correction cosine fourth.
> > > + * @obc_setting: Configuration used by optical black compensation.
> > > + * @oecf_setting: Configuration used by opto-electric conversion function.
> > > + * @r2y_setting: Configuration used by RGB To YUV.
> > > + * @sat_setting: Configuration used by Saturation.
> > > + * @sharp_setting: Configuration used by Sharpen.
> > > + * @ycrv_setting: Configuration used by Y Curve.
> > > + * @sc_setting: Configuration used by statistics collection.
> > > + */
> > > +struct jh7110_isp_params_buffer {
> > > +	__u32 enable_setting;
> > > +	struct jh7110_isp_wb_setting wb_setting;
> > > +	struct jh7110_isp_car_setting car_setting;
> > > +	struct jh7110_isp_ccm_setting ccm_setting;
> > > +	struct jh7110_isp_cfa_setting cfa_setting;
> > > +	struct jh7110_isp_ctc_setting ctc_setting;
> > > +	struct jh7110_isp_dbc_setting dbc_setting;
> > > +	struct jh7110_isp_dnyuv_setting dnyuv_setting;
> > > +	struct jh7110_isp_gmargb_setting gmargb_setting;
> > > +	struct jh7110_isp_lccf_setting lccf_setting;
> > > +	struct jh7110_isp_obc_setting obc_setting;
> > > +	struct jh7110_isp_oecf_setting oecf_setting;
> > > +	struct jh7110_isp_r2y_setting r2y_setting;
> > > +	struct jh7110_isp_sat_setting sat_setting;
> > > +	struct jh7110_isp_sharp_setting sharp_setting;
> > > +	struct jh7110_isp_ycrv_setting ycrv_setting;
> > > +	struct jh7110_isp_sc_setting sc_setting;
> > > +};
> > > +
> > > +/**
> > > + * Statistics Collection Meta Data Flag
> > > + */
> > > +#define JH7110_ISP_SC_FLAG_AWB			0x0
> > > +#define JH7110_ISP_SC_FLAG_AE_AF		0xffff
> > > +
> > > +#pragma pack(1)
> > > +
> > > +/**
> > > + * struct jh7110_isp_sc_buffer - StarFive JH7110 ISP Statistics Collection Meta Data
> > > + *
> > > + * @y_histogram: Y histogram data for saturation control.
> > > + * @reserv0: reserve byte.
> > > + * @bright_sc: bright statistic. If flag is JH7110_ISP_SC_FLAG_AE_AF, This field is
> > 
> > s/bright statistics/brightness statistics/
> > 
> > no capital "T" after ,
> > 
> > > + *             saved auto exposure and auto focus. If flag is JH7110_ISP_SC_FLAG_AWB,
> > > + *             This field is saved auto white balance.
> > 
> > no capital "T" after ,
> > 
> > I would replace "this field is saved" which doesn't sound great in
> > English (not a native speaker though) with "this field stores".
> > 
> > > + * @reserv1: reserve byte.
> > > + * @ae_hist_y: Y histogram for auto exposure.
> > > + * @reserv2: reserve byte.
> > > + * @flag: Statistics Collection Meta Data Flag (JH7110_ISP_SC_FLAG_* definitions)
> > > + */

The statistics also require more documentation. See
https://lore.kernel.org/linux-media/20240709132906.3198927-19-dan.scally@ideasonboard.com/T/#m107e94a23f6deccddf2afdfba9c9cf5bf63522a9
for an example of the level of details that is expected.

> > > +struct jh7110_isp_sc_buffer {
> > > +	__u32 y_histogram[64];
> > > +	__u32 reserv0[33];
> > > +	__u32 bright_sc[4096];
> > > +	__u32 reserv1[96];
> > > +	__u32 ae_hist_y[128];
> > > +	__u32 reserv2[511];
> > > +	__u16 flag;
> > > +};
> > > +
> > > +#pragma pack()
> > 
> > This structure is packed, is it populated directly from HW registers
> > with a memcpy or a DMA transfer ? I guess I'll find it out in the next
> > patches.
> > 
> > > +
> > > +#endif
  

Hi, Laurent

Thanks for your comments.

> 
> On Wed, Jul 10, 2024 at 11:11:57AM +0200, Jacopo Mondi wrote:
> > On Tue, Jul 09, 2024 at 01:38:11AM GMT, Changhuang Liang wrote:
> > > Add JH7110 ISP module definitions for user space.
> > >
> > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > Signed-off-by: Zejian Su <zejian.su@starfivetech.com>
> > > ---
> > >  MAINTAINERS                     |   1 +
> > >  include/uapi/linux/jh7110-isp.h | 739
> > > ++++++++++++++++++++++++++++++++
> >
> > With the recently merged support for the RaspberryPi PiSP BE we have
> > introduced include/uapi/linux/media/raspberry.
> >
> > Would you consider placing this in
> > include/uapi/linux/media/startfive/ ?
> 
> That sounds like a good idea.
> 
> > >  2 files changed, 740 insertions(+)
> > >  create mode 100644 include/uapi/linux/jh7110-isp.h
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS index
> > > 507f04a80499..890604eb0d64 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -21305,6 +21305,7 @@ S:	Maintained
> > >  F:	Documentation/admin-guide/media/starfive_camss.rst
> > >  F:
> 	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
> > >  F:	drivers/staging/media/starfive/camss
> > > +F:	include/uapi/linux/jh7110-isp.h
> > >
> > >  STARFIVE CRYPTO DRIVER
> > >  M:	Jia Jie Ho <jiajie.ho@starfivetech.com>
> > > diff --git a/include/uapi/linux/jh7110-isp.h
> > > b/include/uapi/linux/jh7110-isp.h new file mode 100644 index
> > > 000000000000..4939cd63e771
> > > --- /dev/null
> > > +++ b/include/uapi/linux/jh7110-isp.h
> > > @@ -0,0 +1,739 @@
> > > +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR
> > > +BSD-3-Clause) */
> > > +/*
> > > + * jh7110-isp.h
> > > + *
> > > + * JH7110 ISP driver - user space header file.
> > > + *
> > > + * Copyright © 2023 StarFive Technology Co., Ltd.
> > > + *
> > > + * Author: Zejian Su <zejian.su@starfivetech.com>
> > > + *
> > > + */
> > > +
> > > +#ifndef __JH7110_ISP_H_
> > > +#define __JH7110_ISP_H_
> > > +
> >
> > Do you need to include
> >
> > #include <linux/types.h>
> >
> > > +/**
> >
> > Is this kernel-doc or a single * would do ?
> >
> > > + * ISP Module Diagram
> > > + * ------------------
> > > + *
> > > + *  Raw  +-----+    +------+    +------+    +----+
> > > + *  ---->| OBC |--->| OECF |--->| LCCF |--->| WB |-----+
> > > + *       +-----+    +------+    +------+    +----+     |
> > > + *                                                     |
> > > + *  +--------------------------------------------------+
> > > + *  |
> > > + *  |    +-----+    +-----+    +-----+    +-----+
> > > + *  +--->| DBC |--->| CTC |--->| CFA |--->| CAR |------+
> > > + *       +-----+    +-----+    +-----+    +-----+      |
> > > + *                                                     |
> > > + *  +--------------------------------------------------+
> > > + *  |
> > > + *  |    +-----+    +--------+    +-----+    +------+
> > > + *  +--->| CCM |--->| GMARGB |--->| R2Y |--->| YCRV |--+
> > > + *       +-----+    +--------+    +-----+    +------+  |
> > > + *                                                     |
> > > + *  +--------------------------------------------------+
> > > + *  |
> > > + *  |    +-------+    +-------+    +-----+    +----+
> > > + *  +--->| SHARP |--->| DNYUV |--->| SAT |--->| SC |
> > > + *       +-------+    +-------+    +-----+    +----+
> > > + *
> 
> The diagram is useful, thank you. A glossary would also be nice, maybe as
> 
>  * - OBC: Optical Black Correction
>  * - OECF: Opto-Electric Conversion Function
>  * ...
> 
> I think that would be easier to read than the comments above each macro
> below. Up to you.
> 
> > > + */
> > > +
> > > +/* Auto White Balance */
> > > +#define JH7110_ISP_MODULE_WB_SETTING			(1U << 0)
> > > +/* Color Artifact Removal */
> > > +#define JH7110_ISP_MODULE_CAR_SETTING			(1U << 1)
> > > +/* Color Correction Matrix */
> > > +#define JH7110_ISP_MODULE_CCM_SETTING			(1U << 2)
> > > +/* Color Filter Arrays */
> > > +#define JH7110_ISP_MODULE_CFA_SETTING			(1U << 3)
> > > +/* Crosstalk Correction */
> > > +#define JH7110_ISP_MODULE_CTC_SETTING			(1U << 4)
> > > +/* Defect Bad Pixel Correction */
> > > +#define JH7110_ISP_MODULE_DBC_SETTING			(1U << 5)
> > > +/* Denoise YUV */
> > > +#define JH7110_ISP_MODULE_DNYUV_SETTING			(1U << 6)
> > > +/* RGB Gamma */
> > > +#define JH7110_ISP_MODULE_GMARGB_SETTING		(1U << 7)
> > > +/* Lens Correction Cosine Fourth */
> > > +#define JH7110_ISP_MODULE_LCCF_SETTING			(1U << 8)
> > > +/* Optical Black Correction */
> > > +#define JH7110_ISP_MODULE_OBC_SETTING			(1U << 9)
> > > +/* Opto-Electric Conversion Function */
> > > +#define JH7110_ISP_MODULE_OECF_SETTING			(1U << 10)
> > > +/* RGB To YUV */
> > > +#define JH7110_ISP_MODULE_R2Y_SETTING			(1U << 11)
> > > +/* Saturation */
> > > +#define JH7110_ISP_MODULE_SAT_SETTING			(1U << 12)
> > > +/* Sharpen */
> > > +#define JH7110_ISP_MODULE_SHARP_SETTING			(1U << 13)
> > > +/* Y Curve */
> > > +#define JH7110_ISP_MODULE_YCRV_SETTING			(1U << 14)
> > > +/* Statistics Collection */
> > > +#define JH7110_ISP_MODULE_SC_SETTING			(1U << 15)
> 
> Unless there's a specific reason to keep the current order, maybe you could
> sort those macros in the same order as in the module diagram ?
> 
> > > +
> > > +/**
> > > + * struct jh7110_isp_wb_gain - auto white balance gain
> > > + *
> > > + * @gain_r: gain value for red component.
> > > + * @gain_g: gain value for green component.
> > > + * @gain_b: gain value for blue component.
> 
> I suppose the gains are expressed as fixed-point integers. This needs more
> details, what are the limits, and how many bits of integer and fractional parts
> are there ?
> 
> Same comment for all the other values below.
> 
> > > + */
> > > +struct jh7110_isp_wb_gain {
> > > +	__u16 gain_r;
> > > +	__u16 gain_g;
> > > +	__u16 gain_b;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_wb_setting - Configuration used by auto white
> > > +balance gain
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @gains: auto white balance gain setting.
> > > + */
> > > +struct jh7110_isp_wb_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_wb_gain gains;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_car_setting - Configuration used by color
> > > +artifact removal
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + */
> > > +struct jh7110_isp_car_setting {
> > > +	__u32 enabled;
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_ccm_smlow - Color correction matrix
> > > + *
> > > + * @ccm: color transform matrix with size 3 by 3.
> > > + * @offsets: the offsets of R, G, B after the transform by the ccm.
> > > + */
> > > +struct jh7110_isp_ccm_smlow {
> > > +	__s32 ccm[3][3];
> > > +	__s32 offsets[3];
> > > +};
> > > +
> > > +/**
> > > + * struct jh7110_isp_ccm_setting - Configuration used by color
> > > +correction matrix
> > > + *
> > > + * @enabled: enabled setting flag.
> > > + * @ccm_smlow: Color correction matrix.
> > > + */
> > > +struct jh7110_isp_ccm_setting {
> > > +	__u32 enabled;
> > > +	struct jh7110_isp_ccm_smlow ccm_smlow; };
> > > +
> > > +/**
> > > + * struct jh7110_isp_cfa_params - demosaic parameters
> > > + *
> > > + * @hv_width: detail smooth factor
> > > + * @cross_cov: Cross covariance weighting.
> 
> This documentation doesn't tell how to use those paraemeters. This comment
> applies to many other parameters below. There are three main options to
> improve that:
> 
> - Expanding the documentation in this header file to clearly explain how
>   to compute the parameters values.
> 
> - Providing an open userspace implementation of ISP algorithms that
>   showcase how to calculate the values.
> 
> - Providing detailed hardware documentation for the ISP. This is usually
>   not favoured by ISP vendors, and we are not pushing for this, but I
>   wanted to mention it for completeness.
> 

We prefer the first option. It will take a lot of time for us to supplement the documentation.

> If you would prefer the second option, any open-source camera framework
> would be acceptable, but in practice the only real option is likely libcamera.
> 
> This does not mean that you have to open-source all your ISP control
> algorithms. Only the code needed to explain how ISP parameters are applied
> to the image and are computed is needed. Other parts, such as for instance
> AI-based computation of white balance gains, or complex AGC calculations,
> don't need to be open-source.
> 
> The explain this requirement different and perhaps more clearly, the goal is to
> make sure that developers who have access to only open-source code (ISP
> kernel driver, this header, any open-source userspace code,
> ...) will have enough information to configure and control the ISP.
> 

Regards
Changhuang
  

Hello Changhuang,

On Wed, Jul 24, 2024 at 02:20:17AM +0000, Changhuang Liang wrote:
> > On Wed, Jul 10, 2024 at 11:11:57AM +0200, Jacopo Mondi wrote:
> > > On Tue, Jul 09, 2024 at 01:38:11AM GMT, Changhuang Liang wrote:
> > > > Add JH7110 ISP module definitions for user space.
> > > >
> > > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > > Signed-off-by: Zejian Su <zejian.su@starfivetech.com>
> > > > ---
> > > >  MAINTAINERS                     |   1 +
> > > >  include/uapi/linux/jh7110-isp.h | 739 ++++++++++++++++++++++++++++++++
> > >
> > > With the recently merged support for the RaspberryPi PiSP BE we have
> > > introduced include/uapi/linux/media/raspberry.
> > >
> > > Would you consider placing this in
> > > include/uapi/linux/media/startfive/ ?
> > 
> > That sounds like a good idea.
> > 
> > > >  2 files changed, 740 insertions(+)
> > > >  create mode 100644 include/uapi/linux/jh7110-isp.h
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS index
> > > > 507f04a80499..890604eb0d64 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -21305,6 +21305,7 @@ S:	Maintained
> > > >  F:	Documentation/admin-guide/media/starfive_camss.rst
> > > >  F:
> > 	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
> > > >  F:	drivers/staging/media/starfive/camss
> > > > +F:	include/uapi/linux/jh7110-isp.h
> > > >
> > > >  STARFIVE CRYPTO DRIVER
> > > >  M:	Jia Jie Ho <jiajie.ho@starfivetech.com>
> > > > diff --git a/include/uapi/linux/jh7110-isp.h
> > > > b/include/uapi/linux/jh7110-isp.h new file mode 100644 index
> > > > 000000000000..4939cd63e771
> > > > --- /dev/null
> > > > +++ b/include/uapi/linux/jh7110-isp.h
> > > > @@ -0,0 +1,739 @@
> > > > +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR
> > > > +BSD-3-Clause) */
> > > > +/*
> > > > + * jh7110-isp.h
> > > > + *
> > > > + * JH7110 ISP driver - user space header file.
> > > > + *
> > > > + * Copyright © 2023 StarFive Technology Co., Ltd.
> > > > + *
> > > > + * Author: Zejian Su <zejian.su@starfivetech.com>
> > > > + *
> > > > + */
> > > > +
> > > > +#ifndef __JH7110_ISP_H_
> > > > +#define __JH7110_ISP_H_
> > > > +
> > >
> > > Do you need to include
> > >
> > > #include <linux/types.h>
> > >
> > > > +/**
> > >
> > > Is this kernel-doc or a single * would do ?
> > >
> > > > + * ISP Module Diagram
> > > > + * ------------------
> > > > + *
> > > > + *  Raw  +-----+    +------+    +------+    +----+
> > > > + *  ---->| OBC |--->| OECF |--->| LCCF |--->| WB |-----+
> > > > + *       +-----+    +------+    +------+    +----+     |
> > > > + *                                                     |
> > > > + *  +--------------------------------------------------+
> > > > + *  |
> > > > + *  |    +-----+    +-----+    +-----+    +-----+
> > > > + *  +--->| DBC |--->| CTC |--->| CFA |--->| CAR |------+
> > > > + *       +-----+    +-----+    +-----+    +-----+      |
> > > > + *                                                     |
> > > > + *  +--------------------------------------------------+
> > > > + *  |
> > > > + *  |    +-----+    +--------+    +-----+    +------+
> > > > + *  +--->| CCM |--->| GMARGB |--->| R2Y |--->| YCRV |--+
> > > > + *       +-----+    +--------+    +-----+    +------+  |
> > > > + *                                                     |
> > > > + *  +--------------------------------------------------+
> > > > + *  |
> > > > + *  |    +-------+    +-------+    +-----+    +----+
> > > > + *  +--->| SHARP |--->| DNYUV |--->| SAT |--->| SC |
> > > > + *       +-------+    +-------+    +-----+    +----+
> > > > + *
> > 
> > The diagram is useful, thank you. A glossary would also be nice, maybe as
> > 
> >  * - OBC: Optical Black Correction
> >  * - OECF: Opto-Electric Conversion Function
> >  * ...
> > 
> > I think that would be easier to read than the comments above each macro
> > below. Up to you.
> > 
> > > > + */
> > > > +
> > > > +/* Auto White Balance */
> > > > +#define JH7110_ISP_MODULE_WB_SETTING			(1U << 0)
> > > > +/* Color Artifact Removal */
> > > > +#define JH7110_ISP_MODULE_CAR_SETTING			(1U << 1)
> > > > +/* Color Correction Matrix */
> > > > +#define JH7110_ISP_MODULE_CCM_SETTING			(1U << 2)
> > > > +/* Color Filter Arrays */
> > > > +#define JH7110_ISP_MODULE_CFA_SETTING			(1U << 3)
> > > > +/* Crosstalk Correction */
> > > > +#define JH7110_ISP_MODULE_CTC_SETTING			(1U << 4)
> > > > +/* Defect Bad Pixel Correction */
> > > > +#define JH7110_ISP_MODULE_DBC_SETTING			(1U << 5)
> > > > +/* Denoise YUV */
> > > > +#define JH7110_ISP_MODULE_DNYUV_SETTING			(1U << 6)
> > > > +/* RGB Gamma */
> > > > +#define JH7110_ISP_MODULE_GMARGB_SETTING		(1U << 7)
> > > > +/* Lens Correction Cosine Fourth */
> > > > +#define JH7110_ISP_MODULE_LCCF_SETTING			(1U << 8)
> > > > +/* Optical Black Correction */
> > > > +#define JH7110_ISP_MODULE_OBC_SETTING			(1U << 9)
> > > > +/* Opto-Electric Conversion Function */
> > > > +#define JH7110_ISP_MODULE_OECF_SETTING			(1U << 10)
> > > > +/* RGB To YUV */
> > > > +#define JH7110_ISP_MODULE_R2Y_SETTING			(1U << 11)
> > > > +/* Saturation */
> > > > +#define JH7110_ISP_MODULE_SAT_SETTING			(1U << 12)
> > > > +/* Sharpen */
> > > > +#define JH7110_ISP_MODULE_SHARP_SETTING			(1U << 13)
> > > > +/* Y Curve */
> > > > +#define JH7110_ISP_MODULE_YCRV_SETTING			(1U << 14)
> > > > +/* Statistics Collection */
> > > > +#define JH7110_ISP_MODULE_SC_SETTING			(1U << 15)
> > 
> > Unless there's a specific reason to keep the current order, maybe you could
> > sort those macros in the same order as in the module diagram ?
> > 
> > > > +
> > > > +/**
> > > > + * struct jh7110_isp_wb_gain - auto white balance gain
> > > > + *
> > > > + * @gain_r: gain value for red component.
> > > > + * @gain_g: gain value for green component.
> > > > + * @gain_b: gain value for blue component.
> > 
> > I suppose the gains are expressed as fixed-point integers. This needs more
> > details, what are the limits, and how many bits of integer and fractional parts
> > are there ?
> > 
> > Same comment for all the other values below.
> > 
> > > > + */
> > > > +struct jh7110_isp_wb_gain {
> > > > +	__u16 gain_r;
> > > > +	__u16 gain_g;
> > > > +	__u16 gain_b;
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct jh7110_isp_wb_setting - Configuration used by auto white
> > > > +balance gain
> > > > + *
> > > > + * @enabled: enabled setting flag.
> > > > + * @gains: auto white balance gain setting.
> > > > + */
> > > > +struct jh7110_isp_wb_setting {
> > > > +	__u32 enabled;
> > > > +	struct jh7110_isp_wb_gain gains;
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct jh7110_isp_car_setting - Configuration used by color
> > > > +artifact removal
> > > > + *
> > > > + * @enabled: enabled setting flag.
> > > > + */
> > > > +struct jh7110_isp_car_setting {
> > > > +	__u32 enabled;
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct jh7110_isp_ccm_smlow - Color correction matrix
> > > > + *
> > > > + * @ccm: color transform matrix with size 3 by 3.
> > > > + * @offsets: the offsets of R, G, B after the transform by the ccm.
> > > > + */
> > > > +struct jh7110_isp_ccm_smlow {
> > > > +	__s32 ccm[3][3];
> > > > +	__s32 offsets[3];
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct jh7110_isp_ccm_setting - Configuration used by color
> > > > +correction matrix
> > > > + *
> > > > + * @enabled: enabled setting flag.
> > > > + * @ccm_smlow: Color correction matrix.
> > > > + */
> > > > +struct jh7110_isp_ccm_setting {
> > > > +	__u32 enabled;
> > > > +	struct jh7110_isp_ccm_smlow ccm_smlow; };
> > > > +
> > > > +/**
> > > > + * struct jh7110_isp_cfa_params - demosaic parameters
> > > > + *
> > > > + * @hv_width: detail smooth factor
> > > > + * @cross_cov: Cross covariance weighting.
> > 
> > This documentation doesn't tell how to use those paraemeters. This comment
> > applies to many other parameters below. There are three main options to
> > improve that:
> > 
> > - Expanding the documentation in this header file to clearly explain how
> >   to compute the parameters values.
> > 
> > - Providing an open userspace implementation of ISP algorithms that
> >   showcase how to calculate the values.
> > 
> > - Providing detailed hardware documentation for the ISP. This is usually
> >   not favoured by ISP vendors, and we are not pushing for this, but I
> >   wanted to mention it for completeness.
> > 
> 
> We prefer the first option. It will take a lot of time for us to
> supplement the documentation.

That is fine. Very broadly speaking, the level of documentation we are
aiming for should be enough for a third party developer to implement
support for the ISP control algorithms in libcamera. Please let me know
if you would like to discuss this in more details, to make sure there's
no misunderstanding in the expectations.

> > If you would prefer the second option, any open-source camera framework
> > would be acceptable, but in practice the only real option is likely libcamera.
> > 
> > This does not mean that you have to open-source all your ISP control
> > algorithms. Only the code needed to explain how ISP parameters are applied
> > to the image and are computed is needed. Other parts, such as for instance
> > AI-based computation of white balance gains, or complex AGC calculations,
> > don't need to be open-source.
> > 
> > The explain this requirement different and perhaps more clearly, the goal is to
> > make sure that developers who have access to only open-source code (ISP
> > kernel driver, this header, any open-source userspace code,
> > ...) will have enough information to configure and control the ISP.
  

Hi, Laurent

> 
> Hello Changhuang,
> 
> On Wed, Jul 24, 2024 at 02:20:17AM +0000, Changhuang Liang wrote:
> > > On Wed, Jul 10, 2024 at 11:11:57AM +0200, Jacopo Mondi wrote:
> > > > On Tue, Jul 09, 2024 at 01:38:11AM GMT, Changhuang Liang wrote:
> > > > > Add JH7110 ISP module definitions for user space.
> > > > >
> > > > > Signed-off-by: Changhuang Liang
> > > > > <changhuang.liang@starfivetech.com>
> > > > > Signed-off-by: Zejian Su <zejian.su@starfivetech.com>
> > > > > ---
> > > > >  MAINTAINERS                     |   1 +
> > > > >  include/uapi/linux/jh7110-isp.h | 739
> > > > > ++++++++++++++++++++++++++++++++
> > > >
> > > > With the recently merged support for the RaspberryPi PiSP BE we
> > > > have introduced include/uapi/linux/media/raspberry.
> > > >
> > > > Would you consider placing this in
> > > > include/uapi/linux/media/startfive/ ?
> > >
> > > That sounds like a good idea.
> > >
> > > > >  2 files changed, 740 insertions(+)  create mode 100644
> > > > > include/uapi/linux/jh7110-isp.h
> > > > >
> > > > > diff --git a/MAINTAINERS b/MAINTAINERS index
> > > > > 507f04a80499..890604eb0d64 100644
> > > > > --- a/MAINTAINERS
> > > > > +++ b/MAINTAINERS
> > > > > @@ -21305,6 +21305,7 @@ S:	Maintained
> > > > >  F:	Documentation/admin-guide/media/starfive_camss.rst
> > > > >  F:
> > > 	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
> > > > >  F:	drivers/staging/media/starfive/camss
> > > > > +F:	include/uapi/linux/jh7110-isp.h
> > > > >
> > > > >  STARFIVE CRYPTO DRIVER
> > > > >  M:	Jia Jie Ho <jiajie.ho@starfivetech.com>
> > > > > diff --git a/include/uapi/linux/jh7110-isp.h
> > > > > b/include/uapi/linux/jh7110-isp.h new file mode 100644 index
> > > > > 000000000000..4939cd63e771
> > > > > --- /dev/null
> > > > > +++ b/include/uapi/linux/jh7110-isp.h
> > > > > @@ -0,0 +1,739 @@
> > > > > +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note)
> > > > > +OR
> > > > > +BSD-3-Clause) */
> > > > > +/*
> > > > > + * jh7110-isp.h
> > > > > + *
> > > > > + * JH7110 ISP driver - user space header file.
> > > > > + *
> > > > > + * Copyright © 2023 StarFive Technology Co., Ltd.
> > > > > + *
> > > > > + * Author: Zejian Su <zejian.su@starfivetech.com>
> > > > > + *
> > > > > + */
> > > > > +
> > > > > +#ifndef __JH7110_ISP_H_
> > > > > +#define __JH7110_ISP_H_
> > > > > +
> > > >
> > > > Do you need to include
> > > >
> > > > #include <linux/types.h>
> > > >
> > > > > +/**
> > > >
> > > > Is this kernel-doc or a single * would do ?
> > > >
> > > > > + * ISP Module Diagram
> > > > > + * ------------------
> > > > > + *
> > > > > + *  Raw  +-----+    +------+    +------+    +----+
> > > > > + *  ---->| OBC |--->| OECF |--->| LCCF |--->| WB |-----+
> > > > > + *       +-----+    +------+    +------+    +----+     |
> > > > > + *                                                     |
> > > > > + *  +--------------------------------------------------+
> > > > > + *  |
> > > > > + *  |    +-----+    +-----+    +-----+    +-----+
> > > > > + *  +--->| DBC |--->| CTC |--->| CFA |--->| CAR |------+
> > > > > + *       +-----+    +-----+    +-----+    +-----+      |
> > > > > + *                                                     |
> > > > > + *  +--------------------------------------------------+
> > > > > + *  |
> > > > > + *  |    +-----+    +--------+    +-----+    +------+
> > > > > + *  +--->| CCM |--->| GMARGB |--->| R2Y |--->| YCRV |--+
> > > > > + *       +-----+    +--------+    +-----+    +------+  |
> > > > > + *                                                     |
> > > > > + *  +--------------------------------------------------+
> > > > > + *  |
> > > > > + *  |    +-------+    +-------+    +-----+    +----+
> > > > > + *  +--->| SHARP |--->| DNYUV |--->| SAT |--->| SC |
> > > > > + *       +-------+    +-------+    +-----+    +----+
> > > > > + *
> > >
> > > The diagram is useful, thank you. A glossary would also be nice,
> > > maybe as
> > >
> > >  * - OBC: Optical Black Correction
> > >  * - OECF: Opto-Electric Conversion Function
> > >  * ...
> > >
> > > I think that would be easier to read than the comments above each
> > > macro below. Up to you.
> > >
> > > > > + */
> > > > > +
> > > > > +/* Auto White Balance */
> > > > > +#define JH7110_ISP_MODULE_WB_SETTING			(1U << 0)
> > > > > +/* Color Artifact Removal */
> > > > > +#define JH7110_ISP_MODULE_CAR_SETTING			(1U << 1)
> > > > > +/* Color Correction Matrix */
> > > > > +#define JH7110_ISP_MODULE_CCM_SETTING			(1U << 2)
> > > > > +/* Color Filter Arrays */
> > > > > +#define JH7110_ISP_MODULE_CFA_SETTING			(1U << 3)
> > > > > +/* Crosstalk Correction */
> > > > > +#define JH7110_ISP_MODULE_CTC_SETTING			(1U << 4)
> > > > > +/* Defect Bad Pixel Correction */
> > > > > +#define JH7110_ISP_MODULE_DBC_SETTING			(1U << 5)
> > > > > +/* Denoise YUV */
> > > > > +#define JH7110_ISP_MODULE_DNYUV_SETTING			(1U << 6)
> > > > > +/* RGB Gamma */
> > > > > +#define JH7110_ISP_MODULE_GMARGB_SETTING		(1U << 7)
> > > > > +/* Lens Correction Cosine Fourth */
> > > > > +#define JH7110_ISP_MODULE_LCCF_SETTING			(1U << 8)
> > > > > +/* Optical Black Correction */
> > > > > +#define JH7110_ISP_MODULE_OBC_SETTING			(1U << 9)
> > > > > +/* Opto-Electric Conversion Function */
> > > > > +#define JH7110_ISP_MODULE_OECF_SETTING			(1U <<
> 10)
> > > > > +/* RGB To YUV */
> > > > > +#define JH7110_ISP_MODULE_R2Y_SETTING			(1U <<
> 11)
> > > > > +/* Saturation */
> > > > > +#define JH7110_ISP_MODULE_SAT_SETTING			(1U <<
> 12)
> > > > > +/* Sharpen */
> > > > > +#define JH7110_ISP_MODULE_SHARP_SETTING			(1U <<
> 13)
> > > > > +/* Y Curve */
> > > > > +#define JH7110_ISP_MODULE_YCRV_SETTING			(1U <<
> 14)
> > > > > +/* Statistics Collection */
> > > > > +#define JH7110_ISP_MODULE_SC_SETTING			(1U << 15)
> > >
> > > Unless there's a specific reason to keep the current order, maybe
> > > you could sort those macros in the same order as in the module diagram ?
> > >
> > > > > +
> > > > > +/**
> > > > > + * struct jh7110_isp_wb_gain - auto white balance gain
> > > > > + *
> > > > > + * @gain_r: gain value for red component.
> > > > > + * @gain_g: gain value for green component.
> > > > > + * @gain_b: gain value for blue component.
> > >
> > > I suppose the gains are expressed as fixed-point integers. This
> > > needs more details, what are the limits, and how many bits of
> > > integer and fractional parts are there ?
> > >
> > > Same comment for all the other values below.
> > >
> > > > > + */
> > > > > +struct jh7110_isp_wb_gain {
> > > > > +	__u16 gain_r;
> > > > > +	__u16 gain_g;
> > > > > +	__u16 gain_b;
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * struct jh7110_isp_wb_setting - Configuration used by auto
> > > > > +white balance gain
> > > > > + *
> > > > > + * @enabled: enabled setting flag.
> > > > > + * @gains: auto white balance gain setting.
> > > > > + */
> > > > > +struct jh7110_isp_wb_setting {
> > > > > +	__u32 enabled;
> > > > > +	struct jh7110_isp_wb_gain gains; };
> > > > > +
> > > > > +/**
> > > > > + * struct jh7110_isp_car_setting - Configuration used by color
> > > > > +artifact removal
> > > > > + *
> > > > > + * @enabled: enabled setting flag.
> > > > > + */
> > > > > +struct jh7110_isp_car_setting {
> > > > > +	__u32 enabled;
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * struct jh7110_isp_ccm_smlow - Color correction matrix
> > > > > + *
> > > > > + * @ccm: color transform matrix with size 3 by 3.
> > > > > + * @offsets: the offsets of R, G, B after the transform by the ccm.
> > > > > + */
> > > > > +struct jh7110_isp_ccm_smlow {
> > > > > +	__s32 ccm[3][3];
> > > > > +	__s32 offsets[3];
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * struct jh7110_isp_ccm_setting - Configuration used by color
> > > > > +correction matrix
> > > > > + *
> > > > > + * @enabled: enabled setting flag.
> > > > > + * @ccm_smlow: Color correction matrix.
> > > > > + */
> > > > > +struct jh7110_isp_ccm_setting {
> > > > > +	__u32 enabled;
> > > > > +	struct jh7110_isp_ccm_smlow ccm_smlow; };
> > > > > +
> > > > > +/**
> > > > > + * struct jh7110_isp_cfa_params - demosaic parameters
> > > > > + *
> > > > > + * @hv_width: detail smooth factor
> > > > > + * @cross_cov: Cross covariance weighting.
> > >
> > > This documentation doesn't tell how to use those paraemeters. This
> > > comment applies to many other parameters below. There are three main
> > > options to improve that:
> > >
> > > - Expanding the documentation in this header file to clearly explain how
> > >   to compute the parameters values.
> > >
> > > - Providing an open userspace implementation of ISP algorithms that
> > >   showcase how to calculate the values.
> > >
> > > - Providing detailed hardware documentation for the ISP. This is usually
> > >   not favoured by ISP vendors, and we are not pushing for this, but I
> > >   wanted to mention it for completeness.
> > >
> >
> > We prefer the first option. It will take a lot of time for us to
> > supplement the documentation.
> 
> That is fine. Very broadly speaking, the level of documentation we are aiming
> for should be enough for a third party developer to implement support for the
> ISP control algorithms in libcamera. Please let me know if you would like to
> discuss this in more details, to make sure there's no misunderstanding in the
> expectations.
> 

After the discussion at our company, our company is not going to open source the ISP algorithm section in the short term.
That's why I selected the first option at the first time.

As you said, more ISP information is needed, however, due to the suspension of ISP and the dissolution of the team, we 
actually lack ISP information ourselves. 

So now we can only get more information about the ISP from the source code.

Therefore, we can only provide the libcamera repository that is not open source ISP algorithm.
https://github.com/changhuangliang/libcamera/tree/starfive_isp_3a
(At the beginning, I was responsible for implementing the drivers part, and Zejian was responsible
for implementing the libcamera, but after zejian left the company, I took over the libcamera part, 
but I am not familiar with this part)

We won't open source in the short term, But we'll probably consider open source later when ISP 
has no commercial value.

So I am confused whether to continue the next version.

> > > If you would prefer the second option, any open-source camera
> > > framework would be acceptable, but in practice the only real option is
> likely libcamera.
> > >
> > > This does not mean that you have to open-source all your ISP control
> > > algorithms. Only the code needed to explain how ISP parameters are
> > > applied to the image and are computed is needed. Other parts, such
> > > as for instance AI-based computation of white balance gains, or
> > > complex AGC calculations, don't need to be open-source.
> > >
> > > The explain this requirement different and perhaps more clearly, the
> > > goal is to make sure that developers who have access to only
> > > open-source code (ISP kernel driver, this header, any open-source
> > > userspace code,
> > > ...) will have enough information to configure and control the ISP.
> 
> --

Best Regards
Changhuang
  

Hello Changhuang,

On Sat, Sep 14, 2024 at 11:53:31AM +0000, Changhuang Liang wrote:
> > On Wed, Jul 24, 2024 at 02:20:17AM +0000, Changhuang Liang wrote:
> > > > On Wed, Jul 10, 2024 at 11:11:57AM +0200, Jacopo Mondi wrote:
> > > > > On Tue, Jul 09, 2024 at 01:38:11AM GMT, Changhuang Liang wrote:
> > > > > > Add JH7110 ISP module definitions for user space.
> > > > > >
> > > > > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > > > > Signed-off-by: Zejian Su <zejian.su@starfivetech.com>
> > > > > > ---
> > > > > >  MAINTAINERS                     |   1 +
> > > > > >  include/uapi/linux/jh7110-isp.h | 739 ++++++++++++++++++++++++++++++++
> > > > >
> > > > > With the recently merged support for the RaspberryPi PiSP BE we
> > > > > have introduced include/uapi/linux/media/raspberry.
> > > > >
> > > > > Would you consider placing this in
> > > > > include/uapi/linux/media/startfive/ ?
> > > >
> > > > That sounds like a good idea.
> > > >
> > > > > >  2 files changed, 740 insertions(+)  create mode 100644
> > > > > > include/uapi/linux/jh7110-isp.h
> > > > > >
> > > > > > diff --git a/MAINTAINERS b/MAINTAINERS index
> > > > > > 507f04a80499..890604eb0d64 100644
> > > > > > --- a/MAINTAINERS
> > > > > > +++ b/MAINTAINERS
> > > > > > @@ -21305,6 +21305,7 @@ S:	Maintained
> > > > > >  F:	Documentation/admin-guide/media/starfive_camss.rst
> > > > > >  F:	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
> > > > > >  F:	drivers/staging/media/starfive/camss
> > > > > > +F:	include/uapi/linux/jh7110-isp.h
> > > > > >
> > > > > >  STARFIVE CRYPTO DRIVER
> > > > > >  M:	Jia Jie Ho <jiajie.ho@starfivetech.com>
> > > > > > diff --git a/include/uapi/linux/jh7110-isp.h b/include/uapi/linux/jh7110-isp.h
> > > > > > new file mode 100644 index 000000000000..4939cd63e771
> > > > > > --- /dev/null
> > > > > > +++ b/include/uapi/linux/jh7110-isp.h
> > > > > > @@ -0,0 +1,739 @@
> > > > > > +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR BSD-3-Clause) */
> > > > > > +/*
> > > > > > + * jh7110-isp.h
> > > > > > + *
> > > > > > + * JH7110 ISP driver - user space header file.
> > > > > > + *
> > > > > > + * Copyright © 2023 StarFive Technology Co., Ltd.
> > > > > > + *
> > > > > > + * Author: Zejian Su <zejian.su@starfivetech.com>
> > > > > > + *
> > > > > > + */
> > > > > > +
> > > > > > +#ifndef __JH7110_ISP_H_
> > > > > > +#define __JH7110_ISP_H_
> > > > > > +
> > > > >
> > > > > Do you need to include
> > > > >
> > > > > #include <linux/types.h>
> > > > >
> > > > > > +/**
> > > > >
> > > > > Is this kernel-doc or a single * would do ?
> > > > >
> > > > > > + * ISP Module Diagram
> > > > > > + * ------------------
> > > > > > + *
> > > > > > + *  Raw  +-----+    +------+    +------+    +----+
> > > > > > + *  ---->| OBC |--->| OECF |--->| LCCF |--->| WB |-----+
> > > > > > + *       +-----+    +------+    +------+    +----+     |
> > > > > > + *                                                     |
> > > > > > + *  +--------------------------------------------------+
> > > > > > + *  |
> > > > > > + *  |    +-----+    +-----+    +-----+    +-----+
> > > > > > + *  +--->| DBC |--->| CTC |--->| CFA |--->| CAR |------+
> > > > > > + *       +-----+    +-----+    +-----+    +-----+      |
> > > > > > + *                                                     |
> > > > > > + *  +--------------------------------------------------+
> > > > > > + *  |
> > > > > > + *  |    +-----+    +--------+    +-----+    +------+
> > > > > > + *  +--->| CCM |--->| GMARGB |--->| R2Y |--->| YCRV |--+
> > > > > > + *       +-----+    +--------+    +-----+    +------+  |
> > > > > > + *                                                     |
> > > > > > + *  +--------------------------------------------------+
> > > > > > + *  |
> > > > > > + *  |    +-------+    +-------+    +-----+    +----+
> > > > > > + *  +--->| SHARP |--->| DNYUV |--->| SAT |--->| SC |
> > > > > > + *       +-------+    +-------+    +-----+    +----+
> > > > > > + *
> > > >
> > > > The diagram is useful, thank you. A glossary would also be nice,
> > > > maybe as
> > > >
> > > >  * - OBC: Optical Black Correction
> > > >  * - OECF: Opto-Electric Conversion Function
> > > >  * ...
> > > >
> > > > I think that would be easier to read than the comments above each
> > > > macro below. Up to you.
> > > >
> > > > > > + */
> > > > > > +
> > > > > > +/* Auto White Balance */
> > > > > > +#define JH7110_ISP_MODULE_WB_SETTING			(1U << 0)
> > > > > > +/* Color Artifact Removal */
> > > > > > +#define JH7110_ISP_MODULE_CAR_SETTING			(1U << 1)
> > > > > > +/* Color Correction Matrix */
> > > > > > +#define JH7110_ISP_MODULE_CCM_SETTING			(1U << 2)
> > > > > > +/* Color Filter Arrays */
> > > > > > +#define JH7110_ISP_MODULE_CFA_SETTING			(1U << 3)
> > > > > > +/* Crosstalk Correction */
> > > > > > +#define JH7110_ISP_MODULE_CTC_SETTING			(1U << 4)
> > > > > > +/* Defect Bad Pixel Correction */
> > > > > > +#define JH7110_ISP_MODULE_DBC_SETTING			(1U << 5)
> > > > > > +/* Denoise YUV */
> > > > > > +#define JH7110_ISP_MODULE_DNYUV_SETTING			(1U << 6)
> > > > > > +/* RGB Gamma */
> > > > > > +#define JH7110_ISP_MODULE_GMARGB_SETTING		(1U << 7)
> > > > > > +/* Lens Correction Cosine Fourth */
> > > > > > +#define JH7110_ISP_MODULE_LCCF_SETTING			(1U << 8)
> > > > > > +/* Optical Black Correction */
> > > > > > +#define JH7110_ISP_MODULE_OBC_SETTING			(1U << 9)
> > > > > > +/* Opto-Electric Conversion Function */
> > > > > > +#define JH7110_ISP_MODULE_OECF_SETTING			(1U << 10)
> > > > > > +/* RGB To YUV */
> > > > > > +#define JH7110_ISP_MODULE_R2Y_SETTING			(1U << 11)
> > > > > > +/* Saturation */
> > > > > > +#define JH7110_ISP_MODULE_SAT_SETTING			(1U << 12)
> > > > > > +/* Sharpen */
> > > > > > +#define JH7110_ISP_MODULE_SHARP_SETTING			(1U << 13)
> > > > > > +/* Y Curve */
> > > > > > +#define JH7110_ISP_MODULE_YCRV_SETTING			(1U << 14)
> > > > > > +/* Statistics Collection */
> > > > > > +#define JH7110_ISP_MODULE_SC_SETTING			(1U << 15)
> > > >
> > > > Unless there's a specific reason to keep the current order, maybe
> > > > you could sort those macros in the same order as in the module diagram ?
> > > >
> > > > > > +
> > > > > > +/**
> > > > > > + * struct jh7110_isp_wb_gain - auto white balance gain
> > > > > > + *
> > > > > > + * @gain_r: gain value for red component.
> > > > > > + * @gain_g: gain value for green component.
> > > > > > + * @gain_b: gain value for blue component.
> > > >
> > > > I suppose the gains are expressed as fixed-point integers. This
> > > > needs more details, what are the limits, and how many bits of
> > > > integer and fractional parts are there ?
> > > >
> > > > Same comment for all the other values below.
> > > >
> > > > > > + */
> > > > > > +struct jh7110_isp_wb_gain {
> > > > > > +	__u16 gain_r;
> > > > > > +	__u16 gain_g;
> > > > > > +	__u16 gain_b;
> > > > > > +};
> > > > > > +
> > > > > > +/**
> > > > > > + * struct jh7110_isp_wb_setting - Configuration used by auto
> > > > > > +white balance gain
> > > > > > + *
> > > > > > + * @enabled: enabled setting flag.
> > > > > > + * @gains: auto white balance gain setting.
> > > > > > + */
> > > > > > +struct jh7110_isp_wb_setting {
> > > > > > +	__u32 enabled;
> > > > > > +	struct jh7110_isp_wb_gain gains; };
> > > > > > +
> > > > > > +/**
> > > > > > + * struct jh7110_isp_car_setting - Configuration used by color
> > > > > > +artifact removal
> > > > > > + *
> > > > > > + * @enabled: enabled setting flag.
> > > > > > + */
> > > > > > +struct jh7110_isp_car_setting {
> > > > > > +	__u32 enabled;
> > > > > > +};
> > > > > > +
> > > > > > +/**
> > > > > > + * struct jh7110_isp_ccm_smlow - Color correction matrix
> > > > > > + *
> > > > > > + * @ccm: color transform matrix with size 3 by 3.
> > > > > > + * @offsets: the offsets of R, G, B after the transform by the ccm.
> > > > > > + */
> > > > > > +struct jh7110_isp_ccm_smlow {
> > > > > > +	__s32 ccm[3][3];
> > > > > > +	__s32 offsets[3];
> > > > > > +};
> > > > > > +
> > > > > > +/**
> > > > > > + * struct jh7110_isp_ccm_setting - Configuration used by color
> > > > > > +correction matrix
> > > > > > + *
> > > > > > + * @enabled: enabled setting flag.
> > > > > > + * @ccm_smlow: Color correction matrix.
> > > > > > + */
> > > > > > +struct jh7110_isp_ccm_setting {
> > > > > > +	__u32 enabled;
> > > > > > +	struct jh7110_isp_ccm_smlow ccm_smlow; };
> > > > > > +
> > > > > > +/**
> > > > > > + * struct jh7110_isp_cfa_params - demosaic parameters
> > > > > > + *
> > > > > > + * @hv_width: detail smooth factor
> > > > > > + * @cross_cov: Cross covariance weighting.
> > > >
> > > > This documentation doesn't tell how to use those paraemeters. This
> > > > comment applies to many other parameters below. There are three main
> > > > options to improve that:
> > > >
> > > > - Expanding the documentation in this header file to clearly explain how
> > > >   to compute the parameters values.
> > > >
> > > > - Providing an open userspace implementation of ISP algorithms that
> > > >   showcase how to calculate the values.
> > > >
> > > > - Providing detailed hardware documentation for the ISP. This is usually
> > > >   not favoured by ISP vendors, and we are not pushing for this, but I
> > > >   wanted to mention it for completeness.
> > >
> > > We prefer the first option. It will take a lot of time for us to
> > > supplement the documentation.
> > 
> > That is fine. Very broadly speaking, the level of documentation we are aiming
> > for should be enough for a third party developer to implement support for the
> > ISP control algorithms in libcamera. Please let me know if you would like to
> > discuss this in more details, to make sure there's no misunderstanding in the
> > expectations.
> 
> After the discussion at our company, our company is not going to open source the ISP algorithm section in the short term.
> That's why I selected the first option at the first time.
> 
> As you said, more ISP information is needed, however, due to the suspension of ISP and the dissolution of the team, we 
> actually lack ISP information ourselves. 
> 
> So now we can only get more information about the ISP from the source code.
> 
> Therefore, we can only provide the libcamera repository that is not open source ISP algorithm.
> https://github.com/changhuangliang/libcamera/tree/starfive_isp_3a
> (At the beginning, I was responsible for implementing the drivers part, and Zejian was responsible
> for implementing the libcamera, but after zejian left the company, I took over the libcamera part, 
> but I am not familiar with this part)
> 
> We won't open source in the short term, But we'll probably consider open source later when ISP 
> has no commercial value.
> 
> So I am confused whether to continue the next version.

A kernel driver must be usable by open and closed userspace alike. With
neither of documentation of the ISP or a reference open-source userspace
implementation being available, this is not possible. Therefore, I'm
afraid we won't be able to accept ISP support in the mainline kernel.

> > > > If you would prefer the second option, any open-source camera
> > > > framework would be acceptable, but in practice the only real option is
> > > > likely libcamera.
> > > >
> > > > This does not mean that you have to open-source all your ISP control
> > > > algorithms. Only the code needed to explain how ISP parameters are
> > > > applied to the image and are computed is needed. Other parts, such
> > > > as for instance AI-based computation of white balance gains, or
> > > > complex AGC calculations, don't need to be open-source.
> > > >
> > > > The explain this requirement different and perhaps more clearly, the
> > > > goal is to make sure that developers who have access to only
> > > > open-source code (ISP kernel driver, this header, any open-source
> > > > userspace code,
> > > > ...) will have enough information to configure and control the ISP.
  

Hi, Laurent

> Hello Changhuang,
> 
> On Sat, Sep 14, 2024 at 11:53:31AM +0000, Changhuang Liang wrote:
> > > On Wed, Jul 24, 2024 at 02:20:17AM +0000, Changhuang Liang wrote:
> > > > > On Wed, Jul 10, 2024 at 11:11:57AM +0200, Jacopo Mondi wrote:
> > > > > > On Tue, Jul 09, 2024 at 01:38:11AM GMT, Changhuang Liang wrote:
> > > > > > > Add JH7110 ISP module definitions for user space.
> > > > > > >
> > > > > > > Signed-off-by: Changhuang Liang
> > > > > > > <changhuang.liang@starfivetech.com>
> > > > > > > Signed-off-by: Zejian Su <zejian.su@starfivetech.com>
> > > > > > > ---
> > > > > > >  MAINTAINERS                     |   1 +
> > > > > > >  include/uapi/linux/jh7110-isp.h | 739
> > > > > > > ++++++++++++++++++++++++++++++++
> > > > > >
> > > > > > With the recently merged support for the RaspberryPi PiSP BE
> > > > > > we have introduced include/uapi/linux/media/raspberry.
> > > > > >
> > > > > > Would you consider placing this in
> > > > > > include/uapi/linux/media/startfive/ ?
> > > > >
> > > > > That sounds like a good idea.
> > > > >
> > > > > > >  2 files changed, 740 insertions(+)  create mode 100644
> > > > > > > include/uapi/linux/jh7110-isp.h
> > > > > > >
> > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS index
> > > > > > > 507f04a80499..890604eb0d64 100644
> > > > > > > --- a/MAINTAINERS
> > > > > > > +++ b/MAINTAINERS
> > > > > > > @@ -21305,6 +21305,7 @@ S:	Maintained
> > > > > > >  F:	Documentation/admin-guide/media/starfive_camss.rst
> > > > > > >  F:
> 	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
> > > > > > >  F:	drivers/staging/media/starfive/camss
> > > > > > > +F:	include/uapi/linux/jh7110-isp.h
> > > > > > >
> > > > > > >  STARFIVE CRYPTO DRIVER
> > > > > > >  M:	Jia Jie Ho <jiajie.ho@starfivetech.com>
> > > > > > > diff --git a/include/uapi/linux/jh7110-isp.h
> > > > > > > b/include/uapi/linux/jh7110-isp.h new file mode 100644 index
> > > > > > > 000000000000..4939cd63e771
> > > > > > > --- /dev/null
> > > > > > > +++ b/include/uapi/linux/jh7110-isp.h
> > > > > > > @@ -0,0 +1,739 @@
> > > > > > > +/* SPDX-License-Identifier: ((GPL-2.0+ WITH
> > > > > > > +Linux-syscall-note) OR BSD-3-Clause) */
> > > > > > > +/*
> > > > > > > + * jh7110-isp.h
> > > > > > > + *
> > > > > > > + * JH7110 ISP driver - user space header file.
> > > > > > > + *
> > > > > > > + * Copyright © 2023 StarFive Technology Co., Ltd.
> > > > > > > + *
> > > > > > > + * Author: Zejian Su <zejian.su@starfivetech.com>
> > > > > > > + *
> > > > > > > + */
> > > > > > > +
> > > > > > > +#ifndef __JH7110_ISP_H_
> > > > > > > +#define __JH7110_ISP_H_
> > > > > > > +
> > > > > >
> > > > > > Do you need to include
> > > > > >
> > > > > > #include <linux/types.h>
> > > > > >
> > > > > > > +/**
> > > > > >
> > > > > > Is this kernel-doc or a single * would do ?
> > > > > >
> > > > > > > + * ISP Module Diagram
> > > > > > > + * ------------------
> > > > > > > + *
> > > > > > > + *  Raw  +-----+    +------+    +------+    +----+
> > > > > > > + *  ---->| OBC |--->| OECF |--->| LCCF |--->| WB |-----+
> > > > > > > + *       +-----+    +------+    +------+    +----+     |
> > > > > > > + *
> |
> > > > > > > + *  +--------------------------------------------------+
> > > > > > > + *  |
> > > > > > > + *  |    +-----+    +-----+    +-----+    +-----+
> > > > > > > + *  +--->| DBC |--->| CTC |--->| CFA |--->| CAR |------+
> > > > > > > + *       +-----+    +-----+    +-----+    +-----+      |
> > > > > > > + *
> |
> > > > > > > + *  +--------------------------------------------------+
> > > > > > > + *  |
> > > > > > > + *  |    +-----+    +--------+    +-----+    +------+
> > > > > > > + *  +--->| CCM |--->| GMARGB |--->| R2Y |--->| YCRV |--+
> > > > > > > + *       +-----+    +--------+    +-----+    +------+  |
> > > > > > > + *
> |
> > > > > > > + *  +--------------------------------------------------+
> > > > > > > + *  |
> > > > > > > + *  |    +-------+    +-------+    +-----+    +----+
> > > > > > > + *  +--->| SHARP |--->| DNYUV |--->| SAT |--->| SC |
> > > > > > > + *       +-------+    +-------+    +-----+    +----+
> > > > > > > + *
> > > > >
> > > > > The diagram is useful, thank you. A glossary would also be nice,
> > > > > maybe as
> > > > >
> > > > >  * - OBC: Optical Black Correction
> > > > >  * - OECF: Opto-Electric Conversion Function
> > > > >  * ...
> > > > >
> > > > > I think that would be easier to read than the comments above
> > > > > each macro below. Up to you.
> > > > >
> > > > > > > + */
> > > > > > > +
> > > > > > > +/* Auto White Balance */
> > > > > > > +#define JH7110_ISP_MODULE_WB_SETTING			(1U << 0)
> > > > > > > +/* Color Artifact Removal */
> > > > > > > +#define JH7110_ISP_MODULE_CAR_SETTING			(1U << 1)
> > > > > > > +/* Color Correction Matrix */
> > > > > > > +#define JH7110_ISP_MODULE_CCM_SETTING			(1U
> << 2)
> > > > > > > +/* Color Filter Arrays */
> > > > > > > +#define JH7110_ISP_MODULE_CFA_SETTING			(1U << 3)
> > > > > > > +/* Crosstalk Correction */
> > > > > > > +#define JH7110_ISP_MODULE_CTC_SETTING			(1U << 4)
> > > > > > > +/* Defect Bad Pixel Correction */
> > > > > > > +#define JH7110_ISP_MODULE_DBC_SETTING			(1U << 5)
> > > > > > > +/* Denoise YUV */
> > > > > > > +#define JH7110_ISP_MODULE_DNYUV_SETTING			(1U
> << 6)
> > > > > > > +/* RGB Gamma */
> > > > > > > +#define JH7110_ISP_MODULE_GMARGB_SETTING		(1U
> << 7)
> > > > > > > +/* Lens Correction Cosine Fourth */
> > > > > > > +#define JH7110_ISP_MODULE_LCCF_SETTING			(1U
> << 8)
> > > > > > > +/* Optical Black Correction */
> > > > > > > +#define JH7110_ISP_MODULE_OBC_SETTING			(1U
> << 9)
> > > > > > > +/* Opto-Electric Conversion Function */
> > > > > > > +#define JH7110_ISP_MODULE_OECF_SETTING			(1U
> << 10)
> > > > > > > +/* RGB To YUV */
> > > > > > > +#define JH7110_ISP_MODULE_R2Y_SETTING			(1U <<
> 11)
> > > > > > > +/* Saturation */
> > > > > > > +#define JH7110_ISP_MODULE_SAT_SETTING			(1U <<
> 12)
> > > > > > > +/* Sharpen */
> > > > > > > +#define JH7110_ISP_MODULE_SHARP_SETTING			(1U
> << 13)
> > > > > > > +/* Y Curve */
> > > > > > > +#define JH7110_ISP_MODULE_YCRV_SETTING			(1U
> << 14)
> > > > > > > +/* Statistics Collection */
> > > > > > > +#define JH7110_ISP_MODULE_SC_SETTING			(1U <<
> 15)
> > > > >
> > > > > Unless there's a specific reason to keep the current order,
> > > > > maybe you could sort those macros in the same order as in the module
> diagram ?
> > > > >
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * struct jh7110_isp_wb_gain - auto white balance gain
> > > > > > > + *
> > > > > > > + * @gain_r: gain value for red component.
> > > > > > > + * @gain_g: gain value for green component.
> > > > > > > + * @gain_b: gain value for blue component.
> > > > >
> > > > > I suppose the gains are expressed as fixed-point integers. This
> > > > > needs more details, what are the limits, and how many bits of
> > > > > integer and fractional parts are there ?
> > > > >
> > > > > Same comment for all the other values below.
> > > > >
> > > > > > > + */
> > > > > > > +struct jh7110_isp_wb_gain {
> > > > > > > +	__u16 gain_r;
> > > > > > > +	__u16 gain_g;
> > > > > > > +	__u16 gain_b;
> > > > > > > +};
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * struct jh7110_isp_wb_setting - Configuration used by
> > > > > > > +auto white balance gain
> > > > > > > + *
> > > > > > > + * @enabled: enabled setting flag.
> > > > > > > + * @gains: auto white balance gain setting.
> > > > > > > + */
> > > > > > > +struct jh7110_isp_wb_setting {
> > > > > > > +	__u32 enabled;
> > > > > > > +	struct jh7110_isp_wb_gain gains; };
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * struct jh7110_isp_car_setting - Configuration used by
> > > > > > > +color artifact removal
> > > > > > > + *
> > > > > > > + * @enabled: enabled setting flag.
> > > > > > > + */
> > > > > > > +struct jh7110_isp_car_setting {
> > > > > > > +	__u32 enabled;
> > > > > > > +};
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * struct jh7110_isp_ccm_smlow - Color correction matrix
> > > > > > > + *
> > > > > > > + * @ccm: color transform matrix with size 3 by 3.
> > > > > > > + * @offsets: the offsets of R, G, B after the transform by the ccm.
> > > > > > > + */
> > > > > > > +struct jh7110_isp_ccm_smlow {
> > > > > > > +	__s32 ccm[3][3];
> > > > > > > +	__s32 offsets[3];
> > > > > > > +};
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * struct jh7110_isp_ccm_setting - Configuration used by
> > > > > > > +color correction matrix
> > > > > > > + *
> > > > > > > + * @enabled: enabled setting flag.
> > > > > > > + * @ccm_smlow: Color correction matrix.
> > > > > > > + */
> > > > > > > +struct jh7110_isp_ccm_setting {
> > > > > > > +	__u32 enabled;
> > > > > > > +	struct jh7110_isp_ccm_smlow ccm_smlow; };
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * struct jh7110_isp_cfa_params - demosaic parameters
> > > > > > > + *
> > > > > > > + * @hv_width: detail smooth factor
> > > > > > > + * @cross_cov: Cross covariance weighting.
> > > > >
> > > > > This documentation doesn't tell how to use those paraemeters.
> > > > > This comment applies to many other parameters below. There are
> > > > > three main options to improve that:
> > > > >
> > > > > - Expanding the documentation in this header file to clearly explain
> how
> > > > >   to compute the parameters values.
> > > > >
> > > > > - Providing an open userspace implementation of ISP algorithms that
> > > > >   showcase how to calculate the values.
> > > > >
> > > > > - Providing detailed hardware documentation for the ISP. This is usually
> > > > >   not favoured by ISP vendors, and we are not pushing for this, but I
> > > > >   wanted to mention it for completeness.
> > > >
> > > > We prefer the first option. It will take a lot of time for us to
> > > > supplement the documentation.
> > >
> > > That is fine. Very broadly speaking, the level of documentation we
> > > are aiming for should be enough for a third party developer to
> > > implement support for the ISP control algorithms in libcamera.
> > > Please let me know if you would like to discuss this in more
> > > details, to make sure there's no misunderstanding in the expectations.
> >
> > After the discussion at our company, our company is not going to open
> source the ISP algorithm section in the short term.
> > That's why I selected the first option at the first time.
> >
> > As you said, more ISP information is needed, however, due to the
> > suspension of ISP and the dissolution of the team, we actually lack ISP
> information ourselves.
> >
> > So now we can only get more information about the ISP from the source
> code.
> >
> > Therefore, we can only provide the libcamera repository that is not open
> source ISP algorithm.
> > https://github.com/changhuangliang/libcamera/tree/starfive_isp_3a
> > (At the beginning, I was responsible for implementing the drivers
> > part, and Zejian was responsible for implementing the libcamera, but
> > after zejian left the company, I took over the libcamera part, but I
> > am not familiar with this part)
> >
> > We won't open source in the short term, But we'll probably consider
> > open source later when ISP has no commercial value.
> >
> > So I am confused whether to continue the next version.
> 
> A kernel driver must be usable by open and closed userspace alike. With
> neither of documentation of the ISP or a reference open-source userspace
> implementation being available, this is not possible. Therefore, I'm afraid we
> won't be able to accept ISP support in the mainline kernel.
> 

Understand, so I will pause the series first. Once it can be open-source, I will 
Work with the new series.

Thank you all in review during this period

> > > > > If you would prefer the second option, any open-source camera
> > > > > framework would be acceptable, but in practice the only real
> > > > > option is likely libcamera.
> > > > >
> > > > > This does not mean that you have to open-source all your ISP
> > > > > control algorithms. Only the code needed to explain how ISP
> > > > > parameters are applied to the image and are computed is needed.
> > > > > Other parts, such as for instance AI-based computation of white
> > > > > balance gains, or complex AGC calculations, don't need to be
> open-source.
> > > > >
> > > > > The explain this requirement different and perhaps more clearly,
> > > > > the goal is to make sure that developers who have access to only
> > > > > open-source code (ISP kernel driver, this header, any
> > > > > open-source userspace code,
> > > > > ...) will have enough information to configure and control the ISP.
> 
> --

Best Regards
Changhuang