Message-ID:
 <ZQ0PR01MB13028CC19A3B60DE42B2DC10F2622@ZQ0PR01MB1302.CHNPR01.prod.partner.outlook.cn>
Date: Wed, 18 Sep 2024 01:56:44 +0000
From: Changhuang Liang <changhuang.liang@...rfivetech.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
CC: Jacopo Mondi <jacopo.mondi@...asonboard.com>, Mauro Carvalho Chehab
	<mchehab@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Sakari
 Ailus <sakari.ailus@...ux.intel.com>, Hans Verkuil
	<hverkuil-cisco@...all.nl>, Jean-Michel Hautbois
	<jeanmichel.hautbois@...asonboard.com>, Benjamin Gaignard
	<benjamin.gaignard@...labora.com>, Tomi Valkeinen
	<tomi.valkeinen+renesas@...asonboard.com>, Mingjia Zhang
	<mingjia.zhang@...iatek.com>, Jack Zhu <jack.zhu@...rfivetech.com>, Keith
 Zhao <keith.zhao@...rfivetech.com>, "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-staging@...ts.linux.dev"
	<linux-staging@...ts.linux.dev>
Subject:
 回复: 回复: 回复: [PATCH v5 01/14] media: starfive: Add JH7110 ISP module definitions

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@...rfivetech.com>
> > > > > > > Signed-off-by: Zejian Su <zejian.su@...rfivetech.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@...rfivetech.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@...rfivetech.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

Powered by blists - more mailing lists