Re: 回复: [PATCH v5 01/14] media: starfive: Add JH7110 ISP module definitions

[Laurent Pinchart]

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

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.

-- 
Regards,

Laurent Pinchart