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@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Zejian Su <zejian.su@xxxxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxx>
> > > 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@xxxxxxxxxxxxxxxx>
> > > + *
> > > + */
> > > +
> > > +#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
