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

From: Changhuang Liang
Date: Sat Sep 14 2024 - 08:09:29 EST

Next message: Denis Benato: "Re: [PATCH 2/2] iio: bmi323: fix reversed if statement in bmi323_core_runtime_resume()"
Previous message: Denis Benato: "Re: There is a potential buffer overflow issue in bmi323"
Next in thread: Laurent Pinchart: "Re: 回复: 回复: [PATCH v5 01/14] media: starfive: Add JH7110 ISP module definitions"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

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

Next message: Denis Benato: "Re: [PATCH 2/2] iio: bmi323: fix reversed if statement in bmi323_core_runtime_resume()"
Previous message: Denis Benato: "Re: There is a potential buffer overflow issue in bmi323"
Next in thread: Laurent Pinchart: "Re: 回复: 回复: [PATCH v5 01/14] media: starfive: Add JH7110 ISP module definitions"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]