rkisp in mainline (destaging) vs. rk3326/px30 uapi differences

rkisp in mainline (destaging) vs. rk3326/px30 uapi differences

[Heiko Stuebner]

Hi,

the rkisp driver in the mainline Linux kernel moved out of staging with
5.11-rc1, so the uapi will be fixed after 5.11 proper is released.

The rkisp driver currently only supports the rk3399 and while working
on porting the support for rk3326/px30 I noticed discrepancies.

Hence it would be somewhat urgent to clarify this, as later it will get
really cumbersome.

----

The rkisp on the px30 (v12) has some changes compared to the rk3399 (v10).
Some sub-blocks moved around or seem to have been replaced with newer
variants and the gist of changes can be seen in [0] with the important
part being the uapi changes [1] and those values also exist in mainline.


See functions in that patch:
- isp_goc_config_v12()
- rkisp1_stats_get_aec_meas_v12()
- rkisp1_stats_get_hst_meas_v12()

Looking at the code, the register locations are different, for gammas and
the histogram the actual amount of raw registers is the same, while the
"aec" seems to use 25 registers on V10 while 21 registers on V12. Though
their content gets split into multiple values in that v12 variant.


As somehow expected the whole thing is pretty undocumented and I
have no clue what these "bins" or "gammas" mean and why the amount of
entries now differs and how this relates to userspace at all.

Also looking through libcamera as the one open user of the driver,
the whole rkisp1_cif_isp_isp_other_cfg (containing the gamma config)
as well as the rkisp1_cif_isp_stat struct (for ae and histogram)
don't seem to be used so far.

Hence I also added some Rockchip people in the hope of getting
a bit of clarification ;-) .


Ideas on how to proceed?

Thanks
Heiko


[0] https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c
[1]
diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
index b471f01a8459..fbeb6b5dba03 100644
--- a/include/uapi/linux/rkisp1-config.h
+++ b/include/uapi/linux/rkisp1-config.h
@@ -32,8 +32,8 @@
 #define CIFISP_CTK_COEFF_MAX            0x100
 #define CIFISP_CTK_OFFSET_MAX           0x800
 
-#define CIFISP_AE_MEAN_MAX              25
-#define CIFISP_HIST_BIN_N_MAX           16
+#define CIFISP_AE_MEAN_MAX              81
+#define CIFISP_HIST_BIN_N_MAX           32
 #define CIFISP_AFM_MAX_WINDOWS          3
 #define CIFISP_DEGAMMA_CURVE_SIZE       17
 
@@ -69,7 +69,7 @@
  * Gamma out
  */
 /* Maximum number of color samples supported */
-#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
+#define CIFISP_GAMMA_OUT_MAX_SAMPLES       34

Re: rkisp in mainline (destaging) vs. rk3326/px30 uapi differences

[Dafna Hirschfeld]

Hi,

Am 07.01.21 um 21:23 schrieb Heiko Stuebner:
> Hi,
> 
> the rkisp driver in the mainline Linux kernel moved out of staging with
> 5.11-rc1, so the uapi will be fixed after 5.11 proper is released.
> 
> The rkisp driver currently only supports the rk3399 and while working
> on porting the support for rk3326/px30 I noticed discrepancies.
> 
> Hence it would be somewhat urgent to clarify this, as later it will get
> really cumbersome.

I see that we are now on 5.11-rc2 so that gives us about 4-5 weeks,

> 
> ----
> 
> The rkisp on the px30 (v12) has some changes compared to the rk3399 (v10).

How do you know that the isp of rk3399 is v10 ? I looked at the RK3399 TRM
and the datasheet for the isp and could not find this information.

> Some sub-blocks moved around or seem to have been replaced with newer
> variants and the gist of changes can be seen in [0] with the important
> part being the uapi changes [1] and those values also exist in mainline.
> 
> 
> See functions in that patch:
> - isp_goc_config_v12()
> - rkisp1_stats_get_aec_meas_v12()
> - rkisp1_stats_get_hst_meas_v12()
> 
> Looking at the code, the register locations are different, for gammas and
> the histogram the actual amount of raw registers is the same, while the
> "aec" seems to use 25 registers on V10 while 21 registers on V12. Though
> their content gets split into multiple values in that v12 variant.
> 
> 
> As somehow expected the whole thing is pretty undocumented and I
> have no clue what these "bins" or "gammas" mean and why the amount of
> entries now differs and how this relates to userspace at all.
> 
> Also looking through libcamera as the one open user of the driver,
> the whole rkisp1_cif_isp_isp_other_cfg (containing the gamma config)
> as well as the rkisp1_cif_isp_stat struct (for ae and histogram)
> don't seem to be used so far.

yes, that's a shame. There is a simple implementation using the ae in
stuct rkisp1_cif_isp_stat in src/ipa/rkisp1.c

> 
> Hence I also added some Rockchip people in the hope of getting
> a bit of clarification ;-) .
> 
> 
> Ideas on how to proceed?
> 
> Thanks
> Heiko
> 
> 
> [0] https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c
> [1]
> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> index b471f01a8459..fbeb6b5dba03 100644
> --- a/include/uapi/linux/rkisp1-config.h
> +++ b/include/uapi/linux/rkisp1-config.h
> @@ -32,8 +32,8 @@
>   #define CIFISP_CTK_COEFF_MAX            0x100
>   #define CIFISP_CTK_OFFSET_MAX           0x800
>   
> -#define CIFISP_AE_MEAN_MAX              25
> -#define CIFISP_HIST_BIN_N_MAX           16
> +#define CIFISP_AE_MEAN_MAX              81
> +#define CIFISP_HIST_BIN_N_MAX           32
>   #define CIFISP_AFM_MAX_WINDOWS          3
>   #define CIFISP_DEGAMMA_CURVE_SIZE       17
>   
> @@ -69,7 +69,7 @@
>    * Gamma out
>    */
>   /* Maximum number of color samples supported */
> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES       34

I see that in that code you use the old names of the registers.
The names are different in the current version of the driver,
in the media tree: git://linuxtv.org/media_tree.git
Also, I guess that instead of changing the values you should
add a separated define, something like:

-#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
+#define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
+#define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34


Thanks for working on that, hope we could still fix this in 5.11,

I don't have a rk3326/px30 hardware so I can't test your patches.
Do you have a hardware to test it?
I suggest that you send a patchset to the mailing list then I can
review it and test it on rk3399. Unfortunately there is indeed no way
to thoroughly test the params/stats since there is no userspace for that.

Thanks,
Dafna

> 
> 
> 

Re: rkisp in mainline (destaging) vs. rk3326/px30 uapi differences

[Heiko Stuebner]

Hi Dafna,

Am Freitag, 8. Januar 2021, 12:17:43 CET schrieb Dafna Hirschfeld:
> Am 07.01.21 um 21:23 schrieb Heiko Stuebner:
> > the rkisp driver in the mainline Linux kernel moved out of staging with
> > 5.11-rc1, so the uapi will be fixed after 5.11 proper is released.
> > 
> > The rkisp driver currently only supports the rk3399 and while working
> > on porting the support for rk3326/px30 I noticed discrepancies.
> > 
> > Hence it would be somewhat urgent to clarify this, as later it will get
> > really cumbersome.
> 
> I see that we are now on 5.11-rc2 so that gives us about 4-5 weeks,
> 
> > 
> > ----
> > 
> > The rkisp on the px30 (v12) has some changes compared to the rk3399 (v10).
> 
> How do you know that the isp of rk3399 is v10 ? I looked at the RK3399 TRM
> and the datasheet for the isp and could not find this information.

That's from Rockchip's upstream sources where they introduced the new code.
There're some (if v12) conditionals in there ;-) .


> > Some sub-blocks moved around or seem to have been replaced with newer
> > variants and the gist of changes can be seen in [0] with the important
> > part being the uapi changes [1] and those values also exist in mainline.
> > 
> > 
> > See functions in that patch:
> > - isp_goc_config_v12()
> > - rkisp1_stats_get_aec_meas_v12()
> > - rkisp1_stats_get_hst_meas_v12()
> > 
> > Looking at the code, the register locations are different, for gammas and
> > the histogram the actual amount of raw registers is the same, while the
> > "aec" seems to use 25 registers on V10 while 21 registers on V12. Though
> > their content gets split into multiple values in that v12 variant.
> > 
> > 
> > As somehow expected the whole thing is pretty undocumented and I
> > have no clue what these "bins" or "gammas" mean and why the amount of
> > entries now differs and how this relates to userspace at all.
> > 
> > Also looking through libcamera as the one open user of the driver,
> > the whole rkisp1_cif_isp_isp_other_cfg (containing the gamma config)
> > as well as the rkisp1_cif_isp_stat struct (for ae and histogram)
> > don't seem to be used so far.
> 
> yes, that's a shame. There is a simple implementation using the ae in
> stuct rkisp1_cif_isp_stat in src/ipa/rkisp1.c

Thanks for pointing me to that :-)


> > Hence I also added some Rockchip people in the hope of getting
> > a bit of clarification ;-) .
> > 
> > 
> > Ideas on how to proceed?
> > 
> > Thanks
> > Heiko
> > 
> > 
> > [0] https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c
> > [1]
> > diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> > index b471f01a8459..fbeb6b5dba03 100644
> > --- a/include/uapi/linux/rkisp1-config.h
> > +++ b/include/uapi/linux/rkisp1-config.h
> > @@ -32,8 +32,8 @@
> >   #define CIFISP_CTK_COEFF_MAX            0x100
> >   #define CIFISP_CTK_OFFSET_MAX           0x800
> >   
> > -#define CIFISP_AE_MEAN_MAX              25
> > -#define CIFISP_HIST_BIN_N_MAX           16
> > +#define CIFISP_AE_MEAN_MAX              81
> > +#define CIFISP_HIST_BIN_N_MAX           32
> >   #define CIFISP_AFM_MAX_WINDOWS          3
> >   #define CIFISP_DEGAMMA_CURVE_SIZE       17
> >   
> > @@ -69,7 +69,7 @@
> >    * Gamma out
> >    */
> >   /* Maximum number of color samples supported */
> > -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> > +#define CIFISP_GAMMA_OUT_MAX_SAMPLES       34
> 
> I see that in that code you use the old names of the registers.
> The names are different in the current version of the driver,
> in the media tree: git://linuxtv.org/media_tree.git
> Also, I guess that instead of changing the values you should
> add a separated define, something like:
> 
> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34

Just for clarity, that is Rockchip's commit in their vendor kernel.
I'm just using that as base to get the changes needed for mainline :-) .

The main issue I see is that these max-values directly influence the sizes
of arrays inside the uapi - where the "v12" seems to need bigger arrays
on first glance.
^^^ which is essentially the part I'm mostly worried about

The vendor-code only used the MAX-constants for the uapi to get the
biggest size needed and then defines the real per-version maximums
inside the driver, see
https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R378

and for the auto-exposure:
https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R265

> Thanks for working on that, hope we could still fix this in 5.11,
> 
> I don't have a rk3326/px30 hardware so I can't test your patches.
> Do you have a hardware to test it?

Yep, I'm working on a px30-evb and thankfully the driver for the camera
on it is also already part of mainline.


> I suggest that you send a patchset to the mailing list then I can
> review it and test it on rk3399. Unfortunately there is indeed no way
> to thoroughly test the params/stats since there is no userspace for that.

From looking at the currently newest version [0] it looks like these
new max values seem to have stayed the same, so one solution might be
to just make the uapi structures bigger to these new max values and
hope for the best?

It seems rk3568 and newer will use a really different isp block (they
seem to call it rkisp2 already), so will probably have a separate userspace
interface?


Thanks for your input and help
Heiko


[0] https://github.com/rockchip-linux/kernel/blob/develop-4.19/include/uapi/linux/rkisp1-config.h

Re: rkisp in mainline (destaging) vs. rk3326/px30 uapi differences

[Dafna Hirschfeld]

Am 08.01.21 um 13:05 schrieb Heiko Stuebner:
> Hi Dafna,
> 
> Am Freitag, 8. Januar 2021, 12:17:43 CET schrieb Dafna Hirschfeld:
>> Am 07.01.21 um 21:23 schrieb Heiko Stuebner:
>>> the rkisp driver in the mainline Linux kernel moved out of staging with
>>> 5.11-rc1, so the uapi will be fixed after 5.11 proper is released.
>>>
>>> The rkisp driver currently only supports the rk3399 and while working
>>> on porting the support for rk3326/px30 I noticed discrepancies.
>>>
>>> Hence it would be somewhat urgent to clarify this, as later it will get
>>> really cumbersome.
>>
>> I see that we are now on 5.11-rc2 so that gives us about 4-5 weeks,
>>
>>>
>>> ----
>>>
>>> The rkisp on the px30 (v12) has some changes compared to the rk3399 (v10).
>>
>> How do you know that the isp of rk3399 is v10 ? I looked at the RK3399 TRM
>> and the datasheet for the isp and could not find this information.
> 
> That's from Rockchip's upstream sources where they introduced the new code.
> There're some (if v12) conditionals in there ;-) .
> 
> 
>>> Some sub-blocks moved around or seem to have been replaced with newer
>>> variants and the gist of changes can be seen in [0] with the important
>>> part being the uapi changes [1] and those values also exist in mainline.
>>>
>>>
>>> See functions in that patch:
>>> - isp_goc_config_v12()
>>> - rkisp1_stats_get_aec_meas_v12()
>>> - rkisp1_stats_get_hst_meas_v12()
>>>
>>> Looking at the code, the register locations are different, for gammas and
>>> the histogram the actual amount of raw registers is the same, while the
>>> "aec" seems to use 25 registers on V10 while 21 registers on V12. Though
>>> their content gets split into multiple values in that v12 variant.
>>>
>>>
>>> As somehow expected the whole thing is pretty undocumented and I
>>> have no clue what these "bins" or "gammas" mean and why the amount of
>>> entries now differs and how this relates to userspace at all.
>>>
>>> Also looking through libcamera as the one open user of the driver,
>>> the whole rkisp1_cif_isp_isp_other_cfg (containing the gamma config)
>>> as well as the rkisp1_cif_isp_stat struct (for ae and histogram)
>>> don't seem to be used so far.
>>
>> yes, that's a shame. There is a simple implementation using the ae in
>> stuct rkisp1_cif_isp_stat in src/ipa/rkisp1.c
> 
> Thanks for pointing me to that :-)
> 
> 
>>> Hence I also added some Rockchip people in the hope of getting
>>> a bit of clarification ;-) .
>>>
>>>
>>> Ideas on how to proceed?
>>>
>>> Thanks
>>> Heiko
>>>
>>>
>>> [0] https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c
>>> [1]
>>> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
>>> index b471f01a8459..fbeb6b5dba03 100644
>>> --- a/include/uapi/linux/rkisp1-config.h
>>> +++ b/include/uapi/linux/rkisp1-config.h
>>> @@ -32,8 +32,8 @@
>>>    #define CIFISP_CTK_COEFF_MAX            0x100
>>>    #define CIFISP_CTK_OFFSET_MAX           0x800
>>>    
>>> -#define CIFISP_AE_MEAN_MAX              25
>>> -#define CIFISP_HIST_BIN_N_MAX           16
>>> +#define CIFISP_AE_MEAN_MAX              81
>>> +#define CIFISP_HIST_BIN_N_MAX           32
>>>    #define CIFISP_AFM_MAX_WINDOWS          3
>>>    #define CIFISP_DEGAMMA_CURVE_SIZE       17
>>>    
>>> @@ -69,7 +69,7 @@
>>>     * Gamma out
>>>     */
>>>    /* Maximum number of color samples supported */
>>> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
>>> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES       34
>>
>> I see that in that code you use the old names of the registers.
>> The names are different in the current version of the driver,
>> in the media tree: git://linuxtv.org/media_tree.git
>> Also, I guess that instead of changing the values you should
>> add a separated define, something like:
>>
>> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
>> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
>> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
> 
> Just for clarity, that is Rockchip's commit in their vendor kernel.
> I'm just using that as base to get the changes needed for mainline :-) .
> 
> The main issue I see is that these max-values directly influence the sizes
> of arrays inside the uapi - where the "v12" seems to need bigger arrays
> on first glance.
> ^^^ which is essentially the part I'm mostly worried about

Oh, ok, I thought it's your code.
So maybe we should change the uapi to look like:

/* v10 is the isp version for rk3399 */
#define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
/* v12 is the isp version for rk3326/px30 */
#define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
#define CIFISP_GAMMA_OUT_MAX_SAMPLES       CIFISP_GAMMA_OUT_MAX_SAMPLES_v12

This way we inform userspace how many samples are supported according to the
version.
I don't know if there are other versions with higher maximum,

What do you think?

Thanks,
Dafna


> 
> The vendor-code only used the MAX-constants for the uapi to get the
> biggest size needed and then defines the real per-version maximums
> inside the driver, see
> https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R378
> 
> and for the auto-exposure:
> https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R265
> >> Thanks for working on that, hope we could still fix this in 5.11,
>>
>> I don't have a rk3326/px30 hardware so I can't test your patches.
>> Do you have a hardware to test it?
> 
> Yep, I'm working on a px30-evb and thankfully the driver for the camera
> on it is also already part of mainline.
> 
> 
>> I suggest that you send a patchset to the mailing list then I can
>> review it and test it on rk3399. Unfortunately there is indeed no way
>> to thoroughly test the params/stats since there is no userspace for that.
> 
>  From looking at the currently newest version [0] it looks like these
> new max values seem to have stayed the same, so one solution might be
> to just make the uapi structures bigger to these new max values and
> hope for the best?
> 
> It seems rk3568 and newer will use a really different isp block (they
> seem to call it rkisp2 already), so will probably have a separate userspace
> interface?
> 
> 
> Thanks for your input and help
> Heiko
> 
> 
> [0] https://github.com/rockchip-linux/kernel/blob/develop-4.19/include/uapi/linux/rkisp1-config.h
> 
> 
> 

Re: rkisp in mainline (destaging) vs. rk3326/px30 uapi differences

[Laurent Pinchart]

Hi Dafna and Heiko,

On Fri, Jan 08, 2021 at 04:21:49PM +0100, Dafna Hirschfeld wrote:
> Am 08.01.21 um 13:05 schrieb Heiko Stuebner:
> > Am Freitag, 8. Januar 2021, 12:17:43 CET schrieb Dafna Hirschfeld:
> >> Am 07.01.21 um 21:23 schrieb Heiko Stuebner:
> >>> the rkisp driver in the mainline Linux kernel moved out of staging with
> >>> 5.11-rc1, so the uapi will be fixed after 5.11 proper is released.
> >>>
> >>> The rkisp driver currently only supports the rk3399 and while working
> >>> on porting the support for rk3326/px30 I noticed discrepancies.
> >>>
> >>> Hence it would be somewhat urgent to clarify this, as later it will get
> >>> really cumbersome.
> >>
> >> I see that we are now on 5.11-rc2 so that gives us about 4-5 weeks,
> >>
> >>>
> >>> ----
> >>>
> >>> The rkisp on the px30 (v12) has some changes compared to the rk3399 (v10).
> >>
> >> How do you know that the isp of rk3399 is v10 ? I looked at the RK3399 TRM
> >> and the datasheet for the isp and could not find this information.
> > 
> > That's from Rockchip's upstream sources where they introduced the new code.
> > There're some (if v12) conditionals in there ;-) .
> > 
> >>> Some sub-blocks moved around or seem to have been replaced with newer
> >>> variants and the gist of changes can be seen in [0] with the important
> >>> part being the uapi changes [1] and those values also exist in mainline.
> >>>
> >>>
> >>> See functions in that patch:
> >>> - isp_goc_config_v12()
> >>> - rkisp1_stats_get_aec_meas_v12()
> >>> - rkisp1_stats_get_hst_meas_v12()
> >>>
> >>> Looking at the code, the register locations are different, for gammas and
> >>> the histogram the actual amount of raw registers is the same, while the
> >>> "aec" seems to use 25 registers on V10 while 21 registers on V12. Though
> >>> their content gets split into multiple values in that v12 variant.
> >>>
> >>>
> >>> As somehow expected the whole thing is pretty undocumented and I
> >>> have no clue what these "bins" or "gammas" mean and why the amount of
> >>> entries now differs and how this relates to userspace at all.
> >>>
> >>> Also looking through libcamera as the one open user of the driver,
> >>> the whole rkisp1_cif_isp_isp_other_cfg (containing the gamma config)
> >>> as well as the rkisp1_cif_isp_stat struct (for ae and histogram)
> >>> don't seem to be used so far.
> >>
> >> yes, that's a shame. There is a simple implementation using the ae in
> >> stuct rkisp1_cif_isp_stat in src/ipa/rkisp1.c
> > 
> > Thanks for pointing me to that :-)
> > 
> >>> Hence I also added some Rockchip people in the hope of getting
> >>> a bit of clarification ;-) .
> >>>
> >>> Ideas on how to proceed?
> >>>
> >>> Thanks
> >>> Heiko
> >>>
> >>>
> >>> [0] https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c
> >>> [1]
> >>> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> >>> index b471f01a8459..fbeb6b5dba03 100644
> >>> --- a/include/uapi/linux/rkisp1-config.h
> >>> +++ b/include/uapi/linux/rkisp1-config.h
> >>> @@ -32,8 +32,8 @@
> >>>    #define CIFISP_CTK_COEFF_MAX            0x100
> >>>    #define CIFISP_CTK_OFFSET_MAX           0x800
> >>>    
> >>> -#define CIFISP_AE_MEAN_MAX              25
> >>> -#define CIFISP_HIST_BIN_N_MAX           16
> >>> +#define CIFISP_AE_MEAN_MAX              81
> >>> +#define CIFISP_HIST_BIN_N_MAX           32
> >>>    #define CIFISP_AFM_MAX_WINDOWS          3
> >>>    #define CIFISP_DEGAMMA_CURVE_SIZE       17
> >>>    
> >>> @@ -69,7 +69,7 @@
> >>>     * Gamma out
> >>>     */
> >>>    /* Maximum number of color samples supported */
> >>> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> >>> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES       34
> >>
> >> I see that in that code you use the old names of the registers.
> >> The names are different in the current version of the driver,
> >> in the media tree: git://linuxtv.org/media_tree.git
> >> Also, I guess that instead of changing the values you should
> >> add a separated define, something like:
> >>
> >> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> >> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
> >> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
> > 
> > Just for clarity, that is Rockchip's commit in their vendor kernel.
> > I'm just using that as base to get the changes needed for mainline :-) .
> > 
> > The main issue I see is that these max-values directly influence the sizes
> > of arrays inside the uapi - where the "v12" seems to need bigger arrays
> > on first glance.
> > ^^^ which is essentially the part I'm mostly worried about
> 
> Oh, ok, I thought it's your code.
> So maybe we should change the uapi to look like:
> 
> /* v10 is the isp version for rk3399 */
> #define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
> /* v12 is the isp version for rk3326/px30 */
> #define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
> #define CIFISP_GAMMA_OUT_MAX_SAMPLES       CIFISP_GAMMA_OUT_MAX_SAMPLES_v12
> 
> This way we inform userspace how many samples are supported according to the
> version.
> I don't know if there are other versions with higher maximum,
> 
> What do you think?

This makes sense to me. Userspace will need to know how many samples are
actually present in the array, so corresponding macros should be defined
in the header.

> > The vendor-code only used the MAX-constants for the uapi to get the
> > biggest size needed and then defines the real per-version maximums
> > inside the driver, see
> > https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R378
> > 
> > and for the auto-exposure:
> > https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R265
> > >> Thanks for working on that, hope we could still fix this in 5.11,
> >>
> >> I don't have a rk3326/px30 hardware so I can't test your patches.
> >> Do you have a hardware to test it?
> > 
> > Yep, I'm working on a px30-evb and thankfully the driver for the camera
> > on it is also already part of mainline.
> > 
> >> I suggest that you send a patchset to the mailing list then I can
> >> review it and test it on rk3399. Unfortunately there is indeed no way
> >> to thoroughly test the params/stats since there is no userspace for that.
> > 
> >  From looking at the currently newest version [0] it looks like these
> > new max values seem to have stayed the same, so one solution might be
> > to just make the uapi structures bigger to these new max values and
> > hope for the best?

This is one option, the other option would be to make the array size
dynamic by turning them into pointers. That leads to additional
complications though, so given that the extra memory consumed for the
largest array is reasonable, simply increasing the array size may be the
best option. Do we expect other ISP versions in the future with
differences that would require other changes to the userspace API ? How
about v1 to v9 and v11, do they exist ?

> > It seems rk3568 and newer will use a really different isp block (they
> > seem to call it rkisp2 already), so will probably have a separate userspace
> > interface?
> > 
> > Thanks for your input and help
> > Heiko
> > 
> > [0] https://github.com/rockchip-linux/kernel/blob/develop-4.19/include/uapi/linux/rkisp1-config.h

-- 
Regards,

Laurent Pinchart

Re: rkisp in mainline (destaging) vs. rk3326/px30 uapi differences

[Heiko Stuebner]

Hi Laurent,

Am Samstag, 9. Januar 2021, 02:21:43 CET schrieb Laurent Pinchart:
> On Fri, Jan 08, 2021 at 04:21:49PM +0100, Dafna Hirschfeld wrote:
> > Am 08.01.21 um 13:05 schrieb Heiko Stuebner:
> > > Am Freitag, 8. Januar 2021, 12:17:43 CET schrieb Dafna Hirschfeld:
> > >> Am 07.01.21 um 21:23 schrieb Heiko Stuebner:
> > >>> the rkisp driver in the mainline Linux kernel moved out of staging with
> > >>> 5.11-rc1, so the uapi will be fixed after 5.11 proper is released.
> > >>>
> > >>> The rkisp driver currently only supports the rk3399 and while working
> > >>> on porting the support for rk3326/px30 I noticed discrepancies.
> > >>>
> > >>> Hence it would be somewhat urgent to clarify this, as later it will get
> > >>> really cumbersome.
> > >>
> > >> I see that we are now on 5.11-rc2 so that gives us about 4-5 weeks,
> > >>
> > >>>
> > >>> ----
> > >>>
> > >>> The rkisp on the px30 (v12) has some changes compared to the rk3399 (v10).
> > >>
> > >> How do you know that the isp of rk3399 is v10 ? I looked at the RK3399 TRM
> > >> and the datasheet for the isp and could not find this information.
> > > 
> > > That's from Rockchip's upstream sources where they introduced the new code.
> > > There're some (if v12) conditionals in there ;-) .
> > > 
> > >>> Some sub-blocks moved around or seem to have been replaced with newer
> > >>> variants and the gist of changes can be seen in [0] with the important
> > >>> part being the uapi changes [1] and those values also exist in mainline.
> > >>>
> > >>>
> > >>> See functions in that patch:
> > >>> - isp_goc_config_v12()
> > >>> - rkisp1_stats_get_aec_meas_v12()
> > >>> - rkisp1_stats_get_hst_meas_v12()
> > >>>
> > >>> Looking at the code, the register locations are different, for gammas and
> > >>> the histogram the actual amount of raw registers is the same, while the
> > >>> "aec" seems to use 25 registers on V10 while 21 registers on V12. Though
> > >>> their content gets split into multiple values in that v12 variant.
> > >>>
> > >>>
> > >>> As somehow expected the whole thing is pretty undocumented and I
> > >>> have no clue what these "bins" or "gammas" mean and why the amount of
> > >>> entries now differs and how this relates to userspace at all.
> > >>>
> > >>> Also looking through libcamera as the one open user of the driver,
> > >>> the whole rkisp1_cif_isp_isp_other_cfg (containing the gamma config)
> > >>> as well as the rkisp1_cif_isp_stat struct (for ae and histogram)
> > >>> don't seem to be used so far.
> > >>
> > >> yes, that's a shame. There is a simple implementation using the ae in
> > >> stuct rkisp1_cif_isp_stat in src/ipa/rkisp1.c
> > > 
> > > Thanks for pointing me to that :-)
> > > 
> > >>> Hence I also added some Rockchip people in the hope of getting
> > >>> a bit of clarification ;-) .
> > >>>
> > >>> Ideas on how to proceed?
> > >>>
> > >>> Thanks
> > >>> Heiko
> > >>>
> > >>>
> > >>> [0] https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c
> > >>> [1]
> > >>> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> > >>> index b471f01a8459..fbeb6b5dba03 100644
> > >>> --- a/include/uapi/linux/rkisp1-config.h
> > >>> +++ b/include/uapi/linux/rkisp1-config.h
> > >>> @@ -32,8 +32,8 @@
> > >>>    #define CIFISP_CTK_COEFF_MAX            0x100
> > >>>    #define CIFISP_CTK_OFFSET_MAX           0x800
> > >>>    
> > >>> -#define CIFISP_AE_MEAN_MAX              25
> > >>> -#define CIFISP_HIST_BIN_N_MAX           16
> > >>> +#define CIFISP_AE_MEAN_MAX              81
> > >>> +#define CIFISP_HIST_BIN_N_MAX           32
> > >>>    #define CIFISP_AFM_MAX_WINDOWS          3
> > >>>    #define CIFISP_DEGAMMA_CURVE_SIZE       17
> > >>>    
> > >>> @@ -69,7 +69,7 @@
> > >>>     * Gamma out
> > >>>     */
> > >>>    /* Maximum number of color samples supported */
> > >>> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> > >>> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES       34
> > >>
> > >> I see that in that code you use the old names of the registers.
> > >> The names are different in the current version of the driver,
> > >> in the media tree: git://linuxtv.org/media_tree.git
> > >> Also, I guess that instead of changing the values you should
> > >> add a separated define, something like:
> > >>
> > >> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> > >> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
> > >> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
> > > 
> > > Just for clarity, that is Rockchip's commit in their vendor kernel.
> > > I'm just using that as base to get the changes needed for mainline :-) .
> > > 
> > > The main issue I see is that these max-values directly influence the sizes
> > > of arrays inside the uapi - where the "v12" seems to need bigger arrays
> > > on first glance.
> > > ^^^ which is essentially the part I'm mostly worried about
> > 
> > Oh, ok, I thought it's your code.
> > So maybe we should change the uapi to look like:
> > 
> > /* v10 is the isp version for rk3399 */
> > #define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
> > /* v12 is the isp version for rk3326/px30 */
> > #define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
> > #define CIFISP_GAMMA_OUT_MAX_SAMPLES       CIFISP_GAMMA_OUT_MAX_SAMPLES_v12
> > 
> > This way we inform userspace how many samples are supported according to the
> > version.
> > I don't know if there are other versions with higher maximum,
> > 
> > What do you think?
> 
> This makes sense to me. Userspace will need to know how many samples are
> actually present in the array, so corresponding macros should be defined
> in the header.

ok, though as it seems to have been discussed on irc, we'll also need a
version field to indicate the IP version.


> > > The vendor-code only used the MAX-constants for the uapi to get the
> > > biggest size needed and then defines the real per-version maximums
> > > inside the driver, see
> > > https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R378
> > > 
> > > and for the auto-exposure:
> > > https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R265
> > > >> Thanks for working on that, hope we could still fix this in 5.11,
> > >>
> > >> I don't have a rk3326/px30 hardware so I can't test your patches.
> > >> Do you have a hardware to test it?
> > > 
> > > Yep, I'm working on a px30-evb and thankfully the driver for the camera
> > > on it is also already part of mainline.
> > > 
> > >> I suggest that you send a patchset to the mailing list then I can
> > >> review it and test it on rk3399. Unfortunately there is indeed no way
> > >> to thoroughly test the params/stats since there is no userspace for that.
> > > 
> > >  From looking at the currently newest version [0] it looks like these
> > > new max values seem to have stayed the same, so one solution might be
> > > to just make the uapi structures bigger to these new max values and
> > > hope for the best?
> 
> This is one option, the other option would be to make the array size
> dynamic by turning them into pointers. That leads to additional
> complications though, so given that the extra memory consumed for the
> largest array is reasonable, simply increasing the array size may be the
> best option. Do we expect other ISP versions in the future with
> differences that would require other changes to the userspace API ? How
> about v1 to v9 and v11, do they exist ?

I do believe the version indication is v10 for v1.0 and so on.

Looking at the vendor tree, I see versions:

- V10: rk3288 + rk3399
- V10_1: rk3368 (only supports MP streams - whatever these are)
- V11: unused
- V12: rk3326 / px30
- V13: rk1808
- V20: rk3568 and probably following

gamma_out, hist_grid_size, ae_mean_max, hist_bin
v10:  17, 28, 25, 16
v12: 34, 81, 81, 32
v13: same as v12

Looking at the general change for V20 [0] it really looks like a big rework
of the ISP block happenend with 100K of new register definitions and there
are of course no chips nor boards on the market yet at all, so part of me
would expect this to need a separate userspace when the time comes.


Heiko

[0] https://github.com/rockchip-linux/kernel/commit/e631e47fe7012d165f185009f224d52a81b0f74f

Re: rkisp in mainline (destaging) vs. rk3326/px30 uapi differences

[Laurent Pinchart]

Hi Heiko,

On Mon, Jan 11, 2021 at 11:53:00AM +0100, Heiko Stuebner wrote:
> Am Samstag, 9. Januar 2021, 02:21:43 CET schrieb Laurent Pinchart:
> > On Fri, Jan 08, 2021 at 04:21:49PM +0100, Dafna Hirschfeld wrote:
> > > Am 08.01.21 um 13:05 schrieb Heiko Stuebner:
> > > > Am Freitag, 8. Januar 2021, 12:17:43 CET schrieb Dafna Hirschfeld:
> > > >> Am 07.01.21 um 21:23 schrieb Heiko Stuebner:
> > > >>> the rkisp driver in the mainline Linux kernel moved out of staging with
> > > >>> 5.11-rc1, so the uapi will be fixed after 5.11 proper is released.
> > > >>>
> > > >>> The rkisp driver currently only supports the rk3399 and while working
> > > >>> on porting the support for rk3326/px30 I noticed discrepancies.
> > > >>>
> > > >>> Hence it would be somewhat urgent to clarify this, as later it will get
> > > >>> really cumbersome.
> > > >>
> > > >> I see that we are now on 5.11-rc2 so that gives us about 4-5 weeks,
> > > >>
> > > >>>
> > > >>> ----
> > > >>>
> > > >>> The rkisp on the px30 (v12) has some changes compared to the rk3399 (v10).
> > > >>
> > > >> How do you know that the isp of rk3399 is v10 ? I looked at the RK3399 TRM
> > > >> and the datasheet for the isp and could not find this information.
> > > > 
> > > > That's from Rockchip's upstream sources where they introduced the new code.
> > > > There're some (if v12) conditionals in there ;-) .
> > > > 
> > > >>> Some sub-blocks moved around or seem to have been replaced with newer
> > > >>> variants and the gist of changes can be seen in [0] with the important
> > > >>> part being the uapi changes [1] and those values also exist in mainline.
> > > >>>
> > > >>>
> > > >>> See functions in that patch:
> > > >>> - isp_goc_config_v12()
> > > >>> - rkisp1_stats_get_aec_meas_v12()
> > > >>> - rkisp1_stats_get_hst_meas_v12()
> > > >>>
> > > >>> Looking at the code, the register locations are different, for gammas and
> > > >>> the histogram the actual amount of raw registers is the same, while the
> > > >>> "aec" seems to use 25 registers on V10 while 21 registers on V12. Though
> > > >>> their content gets split into multiple values in that v12 variant.
> > > >>>
> > > >>>
> > > >>> As somehow expected the whole thing is pretty undocumented and I
> > > >>> have no clue what these "bins" or "gammas" mean and why the amount of
> > > >>> entries now differs and how this relates to userspace at all.
> > > >>>
> > > >>> Also looking through libcamera as the one open user of the driver,
> > > >>> the whole rkisp1_cif_isp_isp_other_cfg (containing the gamma config)
> > > >>> as well as the rkisp1_cif_isp_stat struct (for ae and histogram)
> > > >>> don't seem to be used so far.
> > > >>
> > > >> yes, that's a shame. There is a simple implementation using the ae in
> > > >> stuct rkisp1_cif_isp_stat in src/ipa/rkisp1.c
> > > > 
> > > > Thanks for pointing me to that :-)
> > > > 
> > > >>> Hence I also added some Rockchip people in the hope of getting
> > > >>> a bit of clarification ;-) .
> > > >>>
> > > >>> Ideas on how to proceed?
> > > >>>
> > > >>> Thanks
> > > >>> Heiko
> > > >>>
> > > >>>
> > > >>> [0] https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c
> > > >>> [1]
> > > >>> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> > > >>> index b471f01a8459..fbeb6b5dba03 100644
> > > >>> --- a/include/uapi/linux/rkisp1-config.h
> > > >>> +++ b/include/uapi/linux/rkisp1-config.h
> > > >>> @@ -32,8 +32,8 @@
> > > >>>    #define CIFISP_CTK_COEFF_MAX            0x100
> > > >>>    #define CIFISP_CTK_OFFSET_MAX           0x800
> > > >>>    
> > > >>> -#define CIFISP_AE_MEAN_MAX              25
> > > >>> -#define CIFISP_HIST_BIN_N_MAX           16
> > > >>> +#define CIFISP_AE_MEAN_MAX              81
> > > >>> +#define CIFISP_HIST_BIN_N_MAX           32
> > > >>>    #define CIFISP_AFM_MAX_WINDOWS          3
> > > >>>    #define CIFISP_DEGAMMA_CURVE_SIZE       17
> > > >>>    
> > > >>> @@ -69,7 +69,7 @@
> > > >>>     * Gamma out
> > > >>>     */
> > > >>>    /* Maximum number of color samples supported */
> > > >>> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> > > >>> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES       34
> > > >>
> > > >> I see that in that code you use the old names of the registers.
> > > >> The names are different in the current version of the driver,
> > > >> in the media tree: git://linuxtv.org/media_tree.git
> > > >> Also, I guess that instead of changing the values you should
> > > >> add a separated define, something like:
> > > >>
> > > >> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> > > >> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
> > > >> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
> > > > 
> > > > Just for clarity, that is Rockchip's commit in their vendor kernel.
> > > > I'm just using that as base to get the changes needed for mainline :-) .
> > > > 
> > > > The main issue I see is that these max-values directly influence the sizes
> > > > of arrays inside the uapi - where the "v12" seems to need bigger arrays
> > > > on first glance.
> > > > ^^^ which is essentially the part I'm mostly worried about
> > > 
> > > Oh, ok, I thought it's your code.
> > > So maybe we should change the uapi to look like:
> > > 
> > > /* v10 is the isp version for rk3399 */
> > > #define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
> > > /* v12 is the isp version for rk3326/px30 */
> > > #define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
> > > #define CIFISP_GAMMA_OUT_MAX_SAMPLES       CIFISP_GAMMA_OUT_MAX_SAMPLES_v12
> > > 
> > > This way we inform userspace how many samples are supported according to the
> > > version.
> > > I don't know if there are other versions with higher maximum,
> > > 
> > > What do you think?
> > 
> > This makes sense to me. Userspace will need to know how many samples are
> > actually present in the array, so corresponding macros should be defined
> > in the header.
> 
> ok, though as it seems to have been discussed on irc, we'll also need a
> version field to indicate the IP version.

In the statistics buffer that could be done, but in the params buffer it
won't help userspace figure out what version of the IP is in use as
params are filled by the application, not the kernel. I think reporting
the IP version through the media controller API should be enough,
possibly in media_device_info.hw_revision, and/or in the model string.

> > > > The vendor-code only used the MAX-constants for the uapi to get the
> > > > biggest size needed and then defines the real per-version maximums
> > > > inside the driver, see
> > > > https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R378
> > > > 
> > > > and for the auto-exposure:
> > > > https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R265
> > > > >> Thanks for working on that, hope we could still fix this in 5.11,
> > > >>
> > > >> I don't have a rk3326/px30 hardware so I can't test your patches.
> > > >> Do you have a hardware to test it?
> > > > 
> > > > Yep, I'm working on a px30-evb and thankfully the driver for the camera
> > > > on it is also already part of mainline.
> > > > 
> > > >> I suggest that you send a patchset to the mailing list then I can
> > > >> review it and test it on rk3399. Unfortunately there is indeed no way
> > > >> to thoroughly test the params/stats since there is no userspace for that.
> > > > 
> > > >  From looking at the currently newest version [0] it looks like these
> > > > new max values seem to have stayed the same, so one solution might be
> > > > to just make the uapi structures bigger to these new max values and
> > > > hope for the best?
> > 
> > This is one option, the other option would be to make the array size
> > dynamic by turning them into pointers. That leads to additional
> > complications though, so given that the extra memory consumed for the
> > largest array is reasonable, simply increasing the array size may be the
> > best option. Do we expect other ISP versions in the future with
> > differences that would require other changes to the userspace API ? How
> > about v1 to v9 and v11, do they exist ?
> 
> I do believe the version indication is v10 for v1.0 and so on.
> 
> Looking at the vendor tree, I see versions:
> 
> - V10: rk3288 + rk3399
> - V10_1: rk3368 (only supports MP streams - whatever these are)
> - V11: unused
> - V12: rk3326 / px30
> - V13: rk1808
> - V20: rk3568 and probably following
>
> gamma_out, hist_grid_size, ae_mean_max, hist_bin
> v10:  17, 28, 25, 16
> v12: 34, 81, 81, 32
> v13: same as v12

Are v10 and v12 software versions introduced by rockchip, or is there a
version reported in the hardware registers ?

> Looking at the general change for V20 [0] it really looks like a big rework
> of the ISP block happenend with 100K of new register definitions and there
> are of course no chips nor boards on the market yet at all, so part of me
> would expect this to need a separate userspace when the time comes.

Is it an evoluation of the IP core, or something completely different ?
It may even make sense to have a separate kernel driver.

> [0] https://github.com/rockchip-linux/kernel/commit/e631e47fe7012d165f185009f224d52a81b0f74f

-- 
Regards,

Laurent Pinchart

Re: rkisp in mainline (destaging) vs. rk3326/px30 uapi differences

[Heiko Stuebner]

Hi Laurent,

Am Montag, 11. Januar 2021, 12:04:56 CET schrieb Laurent Pinchart:
> On Mon, Jan 11, 2021 at 11:53:00AM +0100, Heiko Stuebner wrote:
> > Am Samstag, 9. Januar 2021, 02:21:43 CET schrieb Laurent Pinchart:
> > > On Fri, Jan 08, 2021 at 04:21:49PM +0100, Dafna Hirschfeld wrote:
> > > > Am 08.01.21 um 13:05 schrieb Heiko Stuebner:
> > > > > Am Freitag, 8. Januar 2021, 12:17:43 CET schrieb Dafna Hirschfeld:
> > > > >> Am 07.01.21 um 21:23 schrieb Heiko Stuebner:
> > > > >>> the rkisp driver in the mainline Linux kernel moved out of staging with
> > > > >>> 5.11-rc1, so the uapi will be fixed after 5.11 proper is released.
> > > > >>>
> > > > >>> The rkisp driver currently only supports the rk3399 and while working
> > > > >>> on porting the support for rk3326/px30 I noticed discrepancies.
> > > > >>>
> > > > >>> Hence it would be somewhat urgent to clarify this, as later it will get
> > > > >>> really cumbersome.
> > > > >>
> > > > >> I see that we are now on 5.11-rc2 so that gives us about 4-5 weeks,
> > > > >>
> > > > >>>
> > > > >>> ----
> > > > >>>
> > > > >>> The rkisp on the px30 (v12) has some changes compared to the rk3399 (v10).
> > > > >>
> > > > >> How do you know that the isp of rk3399 is v10 ? I looked at the RK3399 TRM
> > > > >> and the datasheet for the isp and could not find this information.
> > > > > 
> > > > > That's from Rockchip's upstream sources where they introduced the new code.
> > > > > There're some (if v12) conditionals in there ;-) .
> > > > > 
> > > > >>> Some sub-blocks moved around or seem to have been replaced with newer
> > > > >>> variants and the gist of changes can be seen in [0] with the important
> > > > >>> part being the uapi changes [1] and those values also exist in mainline.
> > > > >>>
> > > > >>>
> > > > >>> See functions in that patch:
> > > > >>> - isp_goc_config_v12()
> > > > >>> - rkisp1_stats_get_aec_meas_v12()
> > > > >>> - rkisp1_stats_get_hst_meas_v12()
> > > > >>>
> > > > >>> Looking at the code, the register locations are different, for gammas and
> > > > >>> the histogram the actual amount of raw registers is the same, while the
> > > > >>> "aec" seems to use 25 registers on V10 while 21 registers on V12. Though
> > > > >>> their content gets split into multiple values in that v12 variant.
> > > > >>>
> > > > >>>
> > > > >>> As somehow expected the whole thing is pretty undocumented and I
> > > > >>> have no clue what these "bins" or "gammas" mean and why the amount of
> > > > >>> entries now differs and how this relates to userspace at all.
> > > > >>>
> > > > >>> Also looking through libcamera as the one open user of the driver,
> > > > >>> the whole rkisp1_cif_isp_isp_other_cfg (containing the gamma config)
> > > > >>> as well as the rkisp1_cif_isp_stat struct (for ae and histogram)
> > > > >>> don't seem to be used so far.
> > > > >>
> > > > >> yes, that's a shame. There is a simple implementation using the ae in
> > > > >> stuct rkisp1_cif_isp_stat in src/ipa/rkisp1.c
> > > > > 
> > > > > Thanks for pointing me to that :-)
> > > > > 
> > > > >>> Hence I also added some Rockchip people in the hope of getting
> > > > >>> a bit of clarification ;-) .
> > > > >>>
> > > > >>> Ideas on how to proceed?
> > > > >>>
> > > > >>> Thanks
> > > > >>> Heiko
> > > > >>>
> > > > >>>
> > > > >>> [0] https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c
> > > > >>> [1]
> > > > >>> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> > > > >>> index b471f01a8459..fbeb6b5dba03 100644
> > > > >>> --- a/include/uapi/linux/rkisp1-config.h
> > > > >>> +++ b/include/uapi/linux/rkisp1-config.h
> > > > >>> @@ -32,8 +32,8 @@
> > > > >>>    #define CIFISP_CTK_COEFF_MAX            0x100
> > > > >>>    #define CIFISP_CTK_OFFSET_MAX           0x800
> > > > >>>    
> > > > >>> -#define CIFISP_AE_MEAN_MAX              25
> > > > >>> -#define CIFISP_HIST_BIN_N_MAX           16
> > > > >>> +#define CIFISP_AE_MEAN_MAX              81
> > > > >>> +#define CIFISP_HIST_BIN_N_MAX           32
> > > > >>>    #define CIFISP_AFM_MAX_WINDOWS          3
> > > > >>>    #define CIFISP_DEGAMMA_CURVE_SIZE       17
> > > > >>>    
> > > > >>> @@ -69,7 +69,7 @@
> > > > >>>     * Gamma out
> > > > >>>     */
> > > > >>>    /* Maximum number of color samples supported */
> > > > >>> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> > > > >>> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES       34
> > > > >>
> > > > >> I see that in that code you use the old names of the registers.
> > > > >> The names are different in the current version of the driver,
> > > > >> in the media tree: git://linuxtv.org/media_tree.git
> > > > >> Also, I guess that instead of changing the values you should
> > > > >> add a separated define, something like:
> > > > >>
> > > > >> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> > > > >> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
> > > > >> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
> > > > > 
> > > > > Just for clarity, that is Rockchip's commit in their vendor kernel.
> > > > > I'm just using that as base to get the changes needed for mainline :-) .
> > > > > 
> > > > > The main issue I see is that these max-values directly influence the sizes
> > > > > of arrays inside the uapi - where the "v12" seems to need bigger arrays
> > > > > on first glance.
> > > > > ^^^ which is essentially the part I'm mostly worried about
> > > > 
> > > > Oh, ok, I thought it's your code.
> > > > So maybe we should change the uapi to look like:
> > > > 
> > > > /* v10 is the isp version for rk3399 */
> > > > #define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
> > > > /* v12 is the isp version for rk3326/px30 */
> > > > #define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
> > > > #define CIFISP_GAMMA_OUT_MAX_SAMPLES       CIFISP_GAMMA_OUT_MAX_SAMPLES_v12
> > > > 
> > > > This way we inform userspace how many samples are supported according to the
> > > > version.
> > > > I don't know if there are other versions with higher maximum,
> > > > 
> > > > What do you think?
> > > 
> > > This makes sense to me. Userspace will need to know how many samples are
> > > actually present in the array, so corresponding macros should be defined
> > > in the header.
> > 
> > ok, though as it seems to have been discussed on irc, we'll also need a
> > version field to indicate the IP version.
> 
> In the statistics buffer that could be done, but in the params buffer it
> won't help userspace figure out what version of the IP is in use as
> params are filled by the application, not the kernel. I think reporting
> the IP version through the media controller API should be enough,
> possibly in media_device_info.hw_revision, and/or in the model string.

hw_revision sounds like the ideal place :-)

and I've added a line doing that, thanks for the pointer.


> > > > > The vendor-code only used the MAX-constants for the uapi to get the
> > > > > biggest size needed and then defines the real per-version maximums
> > > > > inside the driver, see
> > > > > https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R378
> > > > > 
> > > > > and for the auto-exposure:
> > > > > https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R265
> > > > > >> Thanks for working on that, hope we could still fix this in 5.11,
> > > > >>
> > > > >> I don't have a rk3326/px30 hardware so I can't test your patches.
> > > > >> Do you have a hardware to test it?
> > > > > 
> > > > > Yep, I'm working on a px30-evb and thankfully the driver for the camera
> > > > > on it is also already part of mainline.
> > > > > 
> > > > >> I suggest that you send a patchset to the mailing list then I can
> > > > >> review it and test it on rk3399. Unfortunately there is indeed no way
> > > > >> to thoroughly test the params/stats since there is no userspace for that.
> > > > > 
> > > > >  From looking at the currently newest version [0] it looks like these
> > > > > new max values seem to have stayed the same, so one solution might be
> > > > > to just make the uapi structures bigger to these new max values and
> > > > > hope for the best?
> > > 
> > > This is one option, the other option would be to make the array size
> > > dynamic by turning them into pointers. That leads to additional
> > > complications though, so given that the extra memory consumed for the
> > > largest array is reasonable, simply increasing the array size may be the
> > > best option. Do we expect other ISP versions in the future with
> > > differences that would require other changes to the userspace API ? How
> > > about v1 to v9 and v11, do they exist ?
> > 
> > I do believe the version indication is v10 for v1.0 and so on.
> > 
> > Looking at the vendor tree, I see versions:
> > 
> > - V10: rk3288 + rk3399
> > - V10_1: rk3368 (only supports MP streams - whatever these are)
> > - V11: unused
> > - V12: rk3326 / px30
> > - V13: rk1808
> > - V20: rk3568 and probably following
> >
> > gamma_out, hist_grid_size, ae_mean_max, hist_bin
> > v10:  17, 28, 25, 16
> > v12: 34, 81, 81, 32
> > v13: same as v12
> 
> Are v10 and v12 software versions introduced by rockchip, or is there a
> version reported in the hardware registers ?

The version designations are introduced by Rockchip - living in the
dt-compatible-based match-data.

Looking at the registers in the regs header, I sadly didn't see any
version-registers - though V12 moved a number of registers arond
and introduced new ones (for the data sources requiring these
bigger arrays)


> > Looking at the general change for V20 [0] it really looks like a big rework
> > of the ISP block happenend with 100K of new register definitions and there
> > are of course no chips nor boards on the market yet at all, so part of me
> > would expect this to need a separate userspace when the time comes.
> 
> Is it an evoluation of the IP core, or something completely different ?
> It may even make sense to have a separate kernel driver.

From my short glance it seems to share a lot of the basic parts for capture
etc with small evolutions ... but the stats and params parts seem to have
gotten a major evolution.

I guess we'll cross that bridge after the chips are actually available ;-) .
Rockchip pushed that code into their public repo only last week after all.


Heiko

Re: rkisp in mainline (destaging) vs. rk3326/px30 uapi differences

[Laurent Pinchart]

Hi Heiko,

On Mon, Jan 11, 2021 at 04:05:20PM +0100, Heiko Stuebner wrote:
> Am Montag, 11. Januar 2021, 12:04:56 CET schrieb Laurent Pinchart:
> > On Mon, Jan 11, 2021 at 11:53:00AM +0100, Heiko Stuebner wrote:
> >> Am Samstag, 9. Januar 2021, 02:21:43 CET schrieb Laurent Pinchart:
> >>> On Fri, Jan 08, 2021 at 04:21:49PM +0100, Dafna Hirschfeld wrote:
> >>>> Am 08.01.21 um 13:05 schrieb Heiko Stuebner:
> >>>>> Am Freitag, 8. Januar 2021, 12:17:43 CET schrieb Dafna Hirschfeld:
> >>>>>> Am 07.01.21 um 21:23 schrieb Heiko Stuebner:
> >>>>>>> the rkisp driver in the mainline Linux kernel moved out of staging with
> >>>>>>> 5.11-rc1, so the uapi will be fixed after 5.11 proper is released.
> >>>>>>>
> >>>>>>> The rkisp driver currently only supports the rk3399 and while working
> >>>>>>> on porting the support for rk3326/px30 I noticed discrepancies.
> >>>>>>>
> >>>>>>> Hence it would be somewhat urgent to clarify this, as later it will get
> >>>>>>> really cumbersome.
> >>>>>>
> >>>>>> I see that we are now on 5.11-rc2 so that gives us about 4-5 weeks,
> >>>>>>
> >>>>>>>
> >>>>>>> ----
> >>>>>>>
> >>>>>>> The rkisp on the px30 (v12) has some changes compared to the rk3399 (v10).
> >>>>>>
> >>>>>> How do you know that the isp of rk3399 is v10 ? I looked at the RK3399 TRM
> >>>>>> and the datasheet for the isp and could not find this information.
> >>>>> 
> >>>>> That's from Rockchip's upstream sources where they introduced the new code.
> >>>>> There're some (if v12) conditionals in there ;-) .
> >>>>> 
> >>>>>>> Some sub-blocks moved around or seem to have been replaced with newer
> >>>>>>> variants and the gist of changes can be seen in [0] with the important
> >>>>>>> part being the uapi changes [1] and those values also exist in mainline.
> >>>>>>>
> >>>>>>>
> >>>>>>> See functions in that patch:
> >>>>>>> - isp_goc_config_v12()
> >>>>>>> - rkisp1_stats_get_aec_meas_v12()
> >>>>>>> - rkisp1_stats_get_hst_meas_v12()
> >>>>>>>
> >>>>>>> Looking at the code, the register locations are different, for gammas and
> >>>>>>> the histogram the actual amount of raw registers is the same, while the
> >>>>>>> "aec" seems to use 25 registers on V10 while 21 registers on V12. Though
> >>>>>>> their content gets split into multiple values in that v12 variant.
> >>>>>>>
> >>>>>>>
> >>>>>>> As somehow expected the whole thing is pretty undocumented and I
> >>>>>>> have no clue what these "bins" or "gammas" mean and why the amount of
> >>>>>>> entries now differs and how this relates to userspace at all.
> >>>>>>>
> >>>>>>> Also looking through libcamera as the one open user of the driver,
> >>>>>>> the whole rkisp1_cif_isp_isp_other_cfg (containing the gamma config)
> >>>>>>> as well as the rkisp1_cif_isp_stat struct (for ae and histogram)
> >>>>>>> don't seem to be used so far.
> >>>>>>
> >>>>>> yes, that's a shame. There is a simple implementation using the ae in
> >>>>>> stuct rkisp1_cif_isp_stat in src/ipa/rkisp1.c
> >>>>> 
> >>>>> Thanks for pointing me to that :-)
> >>>>> 
> >>>>>>> Hence I also added some Rockchip people in the hope of getting
> >>>>>>> a bit of clarification ;-) .
> >>>>>>>
> >>>>>>> Ideas on how to proceed?
> >>>>>>>
> >>>>>>> Thanks
> >>>>>>> Heiko
> >>>>>>>
> >>>>>>>
> >>>>>>> [0] https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c
> >>>>>>> [1]
> >>>>>>> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> >>>>>>> index b471f01a8459..fbeb6b5dba03 100644
> >>>>>>> --- a/include/uapi/linux/rkisp1-config.h
> >>>>>>> +++ b/include/uapi/linux/rkisp1-config.h
> >>>>>>> @@ -32,8 +32,8 @@
> >>>>>>>    #define CIFISP_CTK_COEFF_MAX            0x100
> >>>>>>>    #define CIFISP_CTK_OFFSET_MAX           0x800
> >>>>>>>    
> >>>>>>> -#define CIFISP_AE_MEAN_MAX              25
> >>>>>>> -#define CIFISP_HIST_BIN_N_MAX           16
> >>>>>>> +#define CIFISP_AE_MEAN_MAX              81
> >>>>>>> +#define CIFISP_HIST_BIN_N_MAX           32
> >>>>>>>    #define CIFISP_AFM_MAX_WINDOWS          3
> >>>>>>>    #define CIFISP_DEGAMMA_CURVE_SIZE       17
> >>>>>>>    
> >>>>>>> @@ -69,7 +69,7 @@
> >>>>>>>     * Gamma out
> >>>>>>>     */
> >>>>>>>    /* Maximum number of color samples supported */
> >>>>>>> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> >>>>>>> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES       34
> >>>>>>
> >>>>>> I see that in that code you use the old names of the registers.
> >>>>>> The names are different in the current version of the driver,
> >>>>>> in the media tree: git://linuxtv.org/media_tree.git
> >>>>>> Also, I guess that instead of changing the values you should
> >>>>>> add a separated define, something like:
> >>>>>>
> >>>>>> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> >>>>>> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
> >>>>>> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
> >>>>> 
> >>>>> Just for clarity, that is Rockchip's commit in their vendor kernel.
> >>>>> I'm just using that as base to get the changes needed for mainline :-) .
> >>>>> 
> >>>>> The main issue I see is that these max-values directly influence the sizes
> >>>>> of arrays inside the uapi - where the "v12" seems to need bigger arrays
> >>>>> on first glance.
> >>>>> ^^^ which is essentially the part I'm mostly worried about
> >>>> 
> >>>> Oh, ok, I thought it's your code.
> >>>> So maybe we should change the uapi to look like:
> >>>> 
> >>>> /* v10 is the isp version for rk3399 */
> >>>> #define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
> >>>> /* v12 is the isp version for rk3326/px30 */
> >>>> #define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
> >>>> #define CIFISP_GAMMA_OUT_MAX_SAMPLES       CIFISP_GAMMA_OUT_MAX_SAMPLES_v12
> >>>> 
> >>>> This way we inform userspace how many samples are supported according to the
> >>>> version.
> >>>> I don't know if there are other versions with higher maximum,
> >>>> 
> >>>> What do you think?
> >>> 
> >>> This makes sense to me. Userspace will need to know how many samples are
> >>> actually present in the array, so corresponding macros should be defined
> >>> in the header.
> >> 
> >> ok, though as it seems to have been discussed on irc, we'll also need a
> >> version field to indicate the IP version.
> > 
> > In the statistics buffer that could be done, but in the params buffer it
> > won't help userspace figure out what version of the IP is in use as
> > params are filled by the application, not the kernel. I think reporting
> > the IP version through the media controller API should be enough,
> > possibly in media_device_info.hw_revision, and/or in the model string.
> 
> hw_revision sounds like the ideal place :-)
> 
> and I've added a line doing that, thanks for the pointer.
> 
> >>>>> The vendor-code only used the MAX-constants for the uapi to get the
> >>>>> biggest size needed and then defines the real per-version maximums
> >>>>> inside the driver, see
> >>>>> https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R378
> >>>>> 
> >>>>> and for the auto-exposure:
> >>>>> https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R265
> >>>>>>> Thanks for working on that, hope we could still fix this in 5.11,
> >>>>>>
> >>>>>> I don't have a rk3326/px30 hardware so I can't test your patches.
> >>>>>> Do you have a hardware to test it?
> >>>>> 
> >>>>> Yep, I'm working on a px30-evb and thankfully the driver for the camera
> >>>>> on it is also already part of mainline.
> >>>>> 
> >>>>>> I suggest that you send a patchset to the mailing list then I can
> >>>>>> review it and test it on rk3399. Unfortunately there is indeed no way
> >>>>>> to thoroughly test the params/stats since there is no userspace for that.
> >>>>> 
> >>>>>  From looking at the currently newest version [0] it looks like these
> >>>>> new max values seem to have stayed the same, so one solution might be
> >>>>> to just make the uapi structures bigger to these new max values and
> >>>>> hope for the best?
> >>> 
> >>> This is one option, the other option would be to make the array size
> >>> dynamic by turning them into pointers. That leads to additional
> >>> complications though, so given that the extra memory consumed for the
> >>> largest array is reasonable, simply increasing the array size may be the
> >>> best option. Do we expect other ISP versions in the future with
> >>> differences that would require other changes to the userspace API ? How
> >>> about v1 to v9 and v11, do they exist ?
> >> 
> >> I do believe the version indication is v10 for v1.0 and so on.
> >> 
> >> Looking at the vendor tree, I see versions:
> >> 
> >> - V10: rk3288 + rk3399
> >> - V10_1: rk3368 (only supports MP streams - whatever these are)
> >> - V11: unused
> >> - V12: rk3326 / px30
> >> - V13: rk1808
> >> - V20: rk3568 and probably following
> >>
> >> gamma_out, hist_grid_size, ae_mean_max, hist_bin
> >> v10:  17, 28, 25, 16
> >> v12: 34, 81, 81, 32
> >> v13: same as v12
> > 
> > Are v10 and v12 software versions introduced by rockchip, or is there a
> > version reported in the hardware registers ?
> 
> The version designations are introduced by Rockchip - living in the
> dt-compatible-based match-data.
> 
> Looking at the registers in the regs header, I sadly didn't see any
> version-registers - though V12 moved a number of registers arond
> and introduced new ones (for the data sources requiring these
> bigger arrays)
> 
> >> Looking at the general change for V20 [0] it really looks like a big rework
> >> of the ISP block happenend with 100K of new register definitions and there
> >> are of course no chips nor boards on the market yet at all, so part of me
> >> would expect this to need a separate userspace when the time comes.
> > 
> > Is it an evoluation of the IP core, or something completely different ?
> > It may even make sense to have a separate kernel driver.
> 
> From my short glance it seems to share a lot of the basic parts for capture
> etc with small evolutions ... but the stats and params parts seem to have
> gotten a major evolution.

While the format of the stats-related structures are partly
hardware-driven the params structures (at least up to v13) are not too
tied to the hardware as the driver writes registers manually. Of course
parameters stored in the buffer are likely to be in the form expected by
hardware registers, but the layout of the structures is quite
independent. We'll have to look at the code in the v20 driver that
interacts with the hardware to try and figure out what the differences
really are.

> I guess we'll cross that bridge after the chips are actually available ;-) .
> Rockchip pushed that code into their public repo only last week after all.

-- 
Regards,

Laurent Pinchart

Re: rkisp in mainline (destaging) vs. rk3326/px30 uapi differences

[Tomasz Figa]

On Tue, Jan 12, 2021 at 12:05 AM Heiko Stuebner
<heiko.stuebner@theobroma-systems.com> wrote:
>
> Hi Laurent,
>
> Am Montag, 11. Januar 2021, 12:04:56 CET schrieb Laurent Pinchart:
> > On Mon, Jan 11, 2021 at 11:53:00AM +0100, Heiko Stuebner wrote:
> > > Am Samstag, 9. Januar 2021, 02:21:43 CET schrieb Laurent Pinchart:
> > > > On Fri, Jan 08, 2021 at 04:21:49PM +0100, Dafna Hirschfeld wrote:
> > > > > Am 08.01.21 um 13:05 schrieb Heiko Stuebner:
> > > > > > Am Freitag, 8. Januar 2021, 12:17:43 CET schrieb Dafna Hirschfeld:
> > > > > >> Am 07.01.21 um 21:23 schrieb Heiko Stuebner:
> > > > > >>> the rkisp driver in the mainline Linux kernel moved out of staging with
> > > > > >>> 5.11-rc1, so the uapi will be fixed after 5.11 proper is released.
> > > > > >>>
> > > > > >>> The rkisp driver currently only supports the rk3399 and while working
> > > > > >>> on porting the support for rk3326/px30 I noticed discrepancies.
> > > > > >>>
> > > > > >>> Hence it would be somewhat urgent to clarify this, as later it will get
> > > > > >>> really cumbersome.
> > > > > >>
> > > > > >> I see that we are now on 5.11-rc2 so that gives us about 4-5 weeks,
> > > > > >>
> > > > > >>>
> > > > > >>> ----
> > > > > >>>
> > > > > >>> The rkisp on the px30 (v12) has some changes compared to the rk3399 (v10).
> > > > > >>
> > > > > >> How do you know that the isp of rk3399 is v10 ? I looked at the RK3399 TRM
> > > > > >> and the datasheet for the isp and could not find this information.
> > > > > >
> > > > > > That's from Rockchip's upstream sources where they introduced the new code.
> > > > > > There're some (if v12) conditionals in there ;-) .
> > > > > >
> > > > > >>> Some sub-blocks moved around or seem to have been replaced with newer
> > > > > >>> variants and the gist of changes can be seen in [0] with the important
> > > > > >>> part being the uapi changes [1] and those values also exist in mainline.
> > > > > >>>
> > > > > >>>
> > > > > >>> See functions in that patch:
> > > > > >>> - isp_goc_config_v12()
> > > > > >>> - rkisp1_stats_get_aec_meas_v12()
> > > > > >>> - rkisp1_stats_get_hst_meas_v12()
> > > > > >>>
> > > > > >>> Looking at the code, the register locations are different, for gammas and
> > > > > >>> the histogram the actual amount of raw registers is the same, while the
> > > > > >>> "aec" seems to use 25 registers on V10 while 21 registers on V12. Though
> > > > > >>> their content gets split into multiple values in that v12 variant.
> > > > > >>>
> > > > > >>>
> > > > > >>> As somehow expected the whole thing is pretty undocumented and I
> > > > > >>> have no clue what these "bins" or "gammas" mean and why the amount of
> > > > > >>> entries now differs and how this relates to userspace at all.
> > > > > >>>
> > > > > >>> Also looking through libcamera as the one open user of the driver,
> > > > > >>> the whole rkisp1_cif_isp_isp_other_cfg (containing the gamma config)
> > > > > >>> as well as the rkisp1_cif_isp_stat struct (for ae and histogram)
> > > > > >>> don't seem to be used so far.
> > > > > >>
> > > > > >> yes, that's a shame. There is a simple implementation using the ae in
> > > > > >> stuct rkisp1_cif_isp_stat in src/ipa/rkisp1.c
> > > > > >
> > > > > > Thanks for pointing me to that :-)
> > > > > >
> > > > > >>> Hence I also added some Rockchip people in the hope of getting
> > > > > >>> a bit of clarification ;-) .
> > > > > >>>
> > > > > >>> Ideas on how to proceed?
> > > > > >>>
> > > > > >>> Thanks
> > > > > >>> Heiko
> > > > > >>>
> > > > > >>>
> > > > > >>> [0] https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c
> > > > > >>> [1]
> > > > > >>> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> > > > > >>> index b471f01a8459..fbeb6b5dba03 100644
> > > > > >>> --- a/include/uapi/linux/rkisp1-config.h
> > > > > >>> +++ b/include/uapi/linux/rkisp1-config.h
> > > > > >>> @@ -32,8 +32,8 @@
> > > > > >>>    #define CIFISP_CTK_COEFF_MAX            0x100
> > > > > >>>    #define CIFISP_CTK_OFFSET_MAX           0x800
> > > > > >>>
> > > > > >>> -#define CIFISP_AE_MEAN_MAX              25
> > > > > >>> -#define CIFISP_HIST_BIN_N_MAX           16
> > > > > >>> +#define CIFISP_AE_MEAN_MAX              81
> > > > > >>> +#define CIFISP_HIST_BIN_N_MAX           32
> > > > > >>>    #define CIFISP_AFM_MAX_WINDOWS          3
> > > > > >>>    #define CIFISP_DEGAMMA_CURVE_SIZE       17
> > > > > >>>
> > > > > >>> @@ -69,7 +69,7 @@
> > > > > >>>     * Gamma out
> > > > > >>>     */
> > > > > >>>    /* Maximum number of color samples supported */
> > > > > >>> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> > > > > >>> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES       34
> > > > > >>
> > > > > >> I see that in that code you use the old names of the registers.
> > > > > >> The names are different in the current version of the driver,
> > > > > >> in the media tree: git://linuxtv.org/media_tree.git
> > > > > >> Also, I guess that instead of changing the values you should
> > > > > >> add a separated define, something like:
> > > > > >>
> > > > > >> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> > > > > >> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
> > > > > >> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
> > > > > >
> > > > > > Just for clarity, that is Rockchip's commit in their vendor kernel.
> > > > > > I'm just using that as base to get the changes needed for mainline :-) .
> > > > > >
> > > > > > The main issue I see is that these max-values directly influence the sizes
> > > > > > of arrays inside the uapi - where the "v12" seems to need bigger arrays
> > > > > > on first glance.
> > > > > > ^^^ which is essentially the part I'm mostly worried about
> > > > >
> > > > > Oh, ok, I thought it's your code.
> > > > > So maybe we should change the uapi to look like:
> > > > >
> > > > > /* v10 is the isp version for rk3399 */
> > > > > #define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
> > > > > /* v12 is the isp version for rk3326/px30 */
> > > > > #define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
> > > > > #define CIFISP_GAMMA_OUT_MAX_SAMPLES       CIFISP_GAMMA_OUT_MAX_SAMPLES_v12
> > > > >
> > > > > This way we inform userspace how many samples are supported according to the
> > > > > version.
> > > > > I don't know if there are other versions with higher maximum,
> > > > >
> > > > > What do you think?
> > > >
> > > > This makes sense to me. Userspace will need to know how many samples are
> > > > actually present in the array, so corresponding macros should be defined
> > > > in the header.
> > >
> > > ok, though as it seems to have been discussed on irc, we'll also need a
> > > version field to indicate the IP version.
> >
> > In the statistics buffer that could be done, but in the params buffer it
> > won't help userspace figure out what version of the IP is in use as
> > params are filled by the application, not the kernel. I think reporting
> > the IP version through the media controller API should be enough,
> > possibly in media_device_info.hw_revision, and/or in the model string.
>
> hw_revision sounds like the ideal place :-)
>
> and I've added a line doing that, thanks for the pointer.
>

One problem with that approach is that we would still need to change
those arrays in the existing UAPI structs. That might be fine for now,
since they still reside under staging/, but even then it would be
cumbersome for existing users, such as Chrome OS.

I think we can still adjust our userspace in Chrome OS, but once we
move out of staging, such changes wouldn't be possible. In that case,
I think there would be two options left:
 - Create a new FourCC and introduce a new structure for the new hardware,
 - Extend the existing structure at the end to allow the userspace
retain the existing layout.

Best regards,
Tomasz

>
> > > > > > The vendor-code only used the MAX-constants for the uapi to get the
> > > > > > biggest size needed and then defines the real per-version maximums
> > > > > > inside the driver, see
> > > > > > https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R378
> > > > > >
> > > > > > and for the auto-exposure:
> > > > > > https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R265
> > > > > > >> Thanks for working on that, hope we could still fix this in 5.11,
> > > > > >>
> > > > > >> I don't have a rk3326/px30 hardware so I can't test your patches.
> > > > > >> Do you have a hardware to test it?
> > > > > >
> > > > > > Yep, I'm working on a px30-evb and thankfully the driver for the camera
> > > > > > on it is also already part of mainline.
> > > > > >
> > > > > >> I suggest that you send a patchset to the mailing list then I can
> > > > > >> review it and test it on rk3399. Unfortunately there is indeed no way
> > > > > >> to thoroughly test the params/stats since there is no userspace for that.
> > > > > >
> > > > > >  From looking at the currently newest version [0] it looks like these
> > > > > > new max values seem to have stayed the same, so one solution might be
> > > > > > to just make the uapi structures bigger to these new max values and
> > > > > > hope for the best?
> > > >
> > > > This is one option, the other option would be to make the array size
> > > > dynamic by turning them into pointers. That leads to additional
> > > > complications though, so given that the extra memory consumed for the
> > > > largest array is reasonable, simply increasing the array size may be the
> > > > best option. Do we expect other ISP versions in the future with
> > > > differences that would require other changes to the userspace API ? How
> > > > about v1 to v9 and v11, do they exist ?
> > >
> > > I do believe the version indication is v10 for v1.0 and so on.
> > >
> > > Looking at the vendor tree, I see versions:
> > >
> > > - V10: rk3288 + rk3399
> > > - V10_1: rk3368 (only supports MP streams - whatever these are)
> > > - V11: unused
> > > - V12: rk3326 / px30
> > > - V13: rk1808
> > > - V20: rk3568 and probably following
> > >
> > > gamma_out, hist_grid_size, ae_mean_max, hist_bin
> > > v10:  17, 28, 25, 16
> > > v12: 34, 81, 81, 32
> > > v13: same as v12
> >
> > Are v10 and v12 software versions introduced by rockchip, or is there a
> > version reported in the hardware registers ?
>
> The version designations are introduced by Rockchip - living in the
> dt-compatible-based match-data.
>
> Looking at the registers in the regs header, I sadly didn't see any
> version-registers - though V12 moved a number of registers arond
> and introduced new ones (for the data sources requiring these
> bigger arrays)
>
>
> > > Looking at the general change for V20 [0] it really looks like a big rework
> > > of the ISP block happenend with 100K of new register definitions and there
> > > are of course no chips nor boards on the market yet at all, so part of me
> > > would expect this to need a separate userspace when the time comes.
> >
> > Is it an evoluation of the IP core, or something completely different ?
> > It may even make sense to have a separate kernel driver.
>
> From my short glance it seems to share a lot of the basic parts for capture
> etc with small evolutions ... but the stats and params parts seem to have
> gotten a major evolution.
>
> I guess we'll cross that bridge after the chips are actually available ;-) .
> Rockchip pushed that code into their public repo only last week after all.
>
>
> Heiko
>
>

Re: rkisp in mainline (destaging) vs. rk3326/px30 uapi differences

[Heiko Stuebner]

Hi Tomasz,

Am Dienstag, 12. Januar 2021, 07:10:16 CET schrieb Tomasz Figa:
> On Tue, Jan 12, 2021 at 12:05 AM Heiko Stuebner
> <heiko.stuebner@theobroma-systems.com> wrote:
> > Am Montag, 11. Januar 2021, 12:04:56 CET schrieb Laurent Pinchart:
> > > On Mon, Jan 11, 2021 at 11:53:00AM +0100, Heiko Stuebner wrote:
> > > > Am Samstag, 9. Januar 2021, 02:21:43 CET schrieb Laurent Pinchart:
> > > > > On Fri, Jan 08, 2021 at 04:21:49PM +0100, Dafna Hirschfeld wrote:
> > > > > > Am 08.01.21 um 13:05 schrieb Heiko Stuebner:
> > > > > > > Am Freitag, 8. Januar 2021, 12:17:43 CET schrieb Dafna Hirschfeld:
> > > > > > >> Am 07.01.21 um 21:23 schrieb Heiko Stuebner:
> > > > > > >>> the rkisp driver in the mainline Linux kernel moved out of staging with
> > > > > > >>> 5.11-rc1, so the uapi will be fixed after 5.11 proper is released.
> > > > > > >>>
> > > > > > >>> The rkisp driver currently only supports the rk3399 and while working
> > > > > > >>> on porting the support for rk3326/px30 I noticed discrepancies.
> > > > > > >>>
> > > > > > >>> Hence it would be somewhat urgent to clarify this, as later it will get
> > > > > > >>> really cumbersome.
> > > > > > >>
> > > > > > >> I see that we are now on 5.11-rc2 so that gives us about 4-5 weeks,
> > > > > > >>
> > > > > > >>>
> > > > > > >>> ----
> > > > > > >>>
> > > > > > >>> The rkisp on the px30 (v12) has some changes compared to the rk3399 (v10).
> > > > > > >>
> > > > > > >> How do you know that the isp of rk3399 is v10 ? I looked at the RK3399 TRM
> > > > > > >> and the datasheet for the isp and could not find this information.
> > > > > > >
> > > > > > > That's from Rockchip's upstream sources where they introduced the new code.
> > > > > > > There're some (if v12) conditionals in there ;-) .
> > > > > > >
> > > > > > >>> Some sub-blocks moved around or seem to have been replaced with newer
> > > > > > >>> variants and the gist of changes can be seen in [0] with the important
> > > > > > >>> part being the uapi changes [1] and those values also exist in mainline.
> > > > > > >>>
> > > > > > >>>
> > > > > > >>> See functions in that patch:
> > > > > > >>> - isp_goc_config_v12()
> > > > > > >>> - rkisp1_stats_get_aec_meas_v12()
> > > > > > >>> - rkisp1_stats_get_hst_meas_v12()
> > > > > > >>>
> > > > > > >>> Looking at the code, the register locations are different, for gammas and
> > > > > > >>> the histogram the actual amount of raw registers is the same, while the
> > > > > > >>> "aec" seems to use 25 registers on V10 while 21 registers on V12. Though
> > > > > > >>> their content gets split into multiple values in that v12 variant.
> > > > > > >>>
> > > > > > >>>
> > > > > > >>> As somehow expected the whole thing is pretty undocumented and I
> > > > > > >>> have no clue what these "bins" or "gammas" mean and why the amount of
> > > > > > >>> entries now differs and how this relates to userspace at all.
> > > > > > >>>
> > > > > > >>> Also looking through libcamera as the one open user of the driver,
> > > > > > >>> the whole rkisp1_cif_isp_isp_other_cfg (containing the gamma config)
> > > > > > >>> as well as the rkisp1_cif_isp_stat struct (for ae and histogram)
> > > > > > >>> don't seem to be used so far.
> > > > > > >>
> > > > > > >> yes, that's a shame. There is a simple implementation using the ae in
> > > > > > >> stuct rkisp1_cif_isp_stat in src/ipa/rkisp1.c
> > > > > > >
> > > > > > > Thanks for pointing me to that :-)
> > > > > > >
> > > > > > >>> Hence I also added some Rockchip people in the hope of getting
> > > > > > >>> a bit of clarification ;-) .
> > > > > > >>>
> > > > > > >>> Ideas on how to proceed?
> > > > > > >>>
> > > > > > >>> Thanks
> > > > > > >>> Heiko
> > > > > > >>>
> > > > > > >>>
> > > > > > >>> [0] https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c
> > > > > > >>> [1]
> > > > > > >>> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> > > > > > >>> index b471f01a8459..fbeb6b5dba03 100644
> > > > > > >>> --- a/include/uapi/linux/rkisp1-config.h
> > > > > > >>> +++ b/include/uapi/linux/rkisp1-config.h
> > > > > > >>> @@ -32,8 +32,8 @@
> > > > > > >>>    #define CIFISP_CTK_COEFF_MAX            0x100
> > > > > > >>>    #define CIFISP_CTK_OFFSET_MAX           0x800
> > > > > > >>>
> > > > > > >>> -#define CIFISP_AE_MEAN_MAX              25
> > > > > > >>> -#define CIFISP_HIST_BIN_N_MAX           16
> > > > > > >>> +#define CIFISP_AE_MEAN_MAX              81
> > > > > > >>> +#define CIFISP_HIST_BIN_N_MAX           32
> > > > > > >>>    #define CIFISP_AFM_MAX_WINDOWS          3
> > > > > > >>>    #define CIFISP_DEGAMMA_CURVE_SIZE       17
> > > > > > >>>
> > > > > > >>> @@ -69,7 +69,7 @@
> > > > > > >>>     * Gamma out
> > > > > > >>>     */
> > > > > > >>>    /* Maximum number of color samples supported */
> > > > > > >>> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> > > > > > >>> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES       34
> > > > > > >>
> > > > > > >> I see that in that code you use the old names of the registers.
> > > > > > >> The names are different in the current version of the driver,
> > > > > > >> in the media tree: git://linuxtv.org/media_tree.git
> > > > > > >> Also, I guess that instead of changing the values you should
> > > > > > >> add a separated define, something like:
> > > > > > >>
> > > > > > >> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> > > > > > >> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
> > > > > > >> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
> > > > > > >
> > > > > > > Just for clarity, that is Rockchip's commit in their vendor kernel.
> > > > > > > I'm just using that as base to get the changes needed for mainline :-) .
> > > > > > >
> > > > > > > The main issue I see is that these max-values directly influence the sizes
> > > > > > > of arrays inside the uapi - where the "v12" seems to need bigger arrays
> > > > > > > on first glance.
> > > > > > > ^^^ which is essentially the part I'm mostly worried about
> > > > > >
> > > > > > Oh, ok, I thought it's your code.
> > > > > > So maybe we should change the uapi to look like:
> > > > > >
> > > > > > /* v10 is the isp version for rk3399 */
> > > > > > #define CIFISP_GAMMA_OUT_MAX_SAMPLES_V10       17
> > > > > > /* v12 is the isp version for rk3326/px30 */
> > > > > > #define CIFISP_GAMMA_OUT_MAX_SAMPLES_v12       34
> > > > > > #define CIFISP_GAMMA_OUT_MAX_SAMPLES       CIFISP_GAMMA_OUT_MAX_SAMPLES_v12
> > > > > >
> > > > > > This way we inform userspace how many samples are supported according to the
> > > > > > version.
> > > > > > I don't know if there are other versions with higher maximum,
> > > > > >
> > > > > > What do you think?
> > > > >
> > > > > This makes sense to me. Userspace will need to know how many samples are
> > > > > actually present in the array, so corresponding macros should be defined
> > > > > in the header.
> > > >
> > > > ok, though as it seems to have been discussed on irc, we'll also need a
> > > > version field to indicate the IP version.
> > >
> > > In the statistics buffer that could be done, but in the params buffer it
> > > won't help userspace figure out what version of the IP is in use as
> > > params are filled by the application, not the kernel. I think reporting
> > > the IP version through the media controller API should be enough,
> > > possibly in media_device_info.hw_revision, and/or in the model string.
> >
> > hw_revision sounds like the ideal place :-)
> >
> > and I've added a line doing that, thanks for the pointer.
> >
> 
> One problem with that approach is that we would still need to change
> those arrays in the existing UAPI structs. That might be fine for now,
> since they still reside under staging/, but even then it would be
> cumbersome for existing users, such as Chrome OS.
> 
> I think we can still adjust our userspace in Chrome OS, but once we
> move out of staging, such changes wouldn't be possible. In that case,
> I think there would be two options left:
>  - Create a new FourCC and introduce a new structure for the new hardware,
>  - Extend the existing structure at the end to allow the userspace
> retain the existing layout.

rkisp1 moved out of staging with 5.11-rc1 so essentially we have the rest
of 5.11-rc to fix this without high cost.

Yesterday evening I send a 2-patch serie, doing the extension which you can
find on [0]. It increases the numbers and thus also extends the arrays in the
uapi.

As I said before, all versions that are close to the rk3399 original seem
to be using these values and they haven't been increased anymore since.


Heiko


[0] http://lore.kernel.org/r/20210111234011.3642481-1-heiko@sntech.de

> > > > > > > The vendor-code only used the MAX-constants for the uapi to get the
> > > > > > > biggest size needed and then defines the real per-version maximums
> > > > > > > inside the driver, see
> > > > > > > https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R378
> > > > > > >
> > > > > > > and for the auto-exposure:
> > > > > > > https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c#diff-961dbaed00164098bb082b01d6c9446501cfcef808cf5a71bf18405067fb5426R265
> > > > > > > >> Thanks for working on that, hope we could still fix this in 5.11,
> > > > > > >>
> > > > > > >> I don't have a rk3326/px30 hardware so I can't test your patches.
> > > > > > >> Do you have a hardware to test it?
> > > > > > >
> > > > > > > Yep, I'm working on a px30-evb and thankfully the driver for the camera
> > > > > > > on it is also already part of mainline.
> > > > > > >
> > > > > > >> I suggest that you send a patchset to the mailing list then I can
> > > > > > >> review it and test it on rk3399. Unfortunately there is indeed no way
> > > > > > >> to thoroughly test the params/stats since there is no userspace for that.
> > > > > > >
> > > > > > >  From looking at the currently newest version [0] it looks like these
> > > > > > > new max values seem to have stayed the same, so one solution might be
> > > > > > > to just make the uapi structures bigger to these new max values and
> > > > > > > hope for the best?
> > > > >
> > > > > This is one option, the other option would be to make the array size
> > > > > dynamic by turning them into pointers. That leads to additional
> > > > > complications though, so given that the extra memory consumed for the
> > > > > largest array is reasonable, simply increasing the array size may be the
> > > > > best option. Do we expect other ISP versions in the future with
> > > > > differences that would require other changes to the userspace API ? How
> > > > > about v1 to v9 and v11, do they exist ?
> > > >
> > > > I do believe the version indication is v10 for v1.0 and so on.
> > > >
> > > > Looking at the vendor tree, I see versions:
> > > >
> > > > - V10: rk3288 + rk3399
> > > > - V10_1: rk3368 (only supports MP streams - whatever these are)
> > > > - V11: unused
> > > > - V12: rk3326 / px30
> > > > - V13: rk1808
> > > > - V20: rk3568 and probably following
> > > >
> > > > gamma_out, hist_grid_size, ae_mean_max, hist_bin
> > > > v10:  17, 28, 25, 16
> > > > v12: 34, 81, 81, 32
> > > > v13: same as v12
> > >
> > > Are v10 and v12 software versions introduced by rockchip, or is there a
> > > version reported in the hardware registers ?
> >
> > The version designations are introduced by Rockchip - living in the
> > dt-compatible-based match-data.
> >
> > Looking at the registers in the regs header, I sadly didn't see any
> > version-registers - though V12 moved a number of registers arond
> > and introduced new ones (for the data sources requiring these
> > bigger arrays)
> >
> >
> > > > Looking at the general change for V20 [0] it really looks like a big rework
> > > > of the ISP block happenend with 100K of new register definitions and there
> > > > are of course no chips nor boards on the market yet at all, so part of me
> > > > would expect this to need a separate userspace when the time comes.
> > >
> > > Is it an evoluation of the IP core, or something completely different ?
> > > It may even make sense to have a separate kernel driver.
> >
> > From my short glance it seems to share a lot of the basic parts for capture
> > etc with small evolutions ... but the stats and params parts seem to have
> > gotten a major evolution.
> >
> > I guess we'll cross that bridge after the chips are actually available ;-) .
> > Rockchip pushed that code into their public repo only last week after all.
> >
> >
> > Heiko
> >
> >
> 

Re: rkisp in mainline (destaging) vs. rk3326/px30 uapi differences

[Tomasz Figa]

Hi Heiko,

On Fri, Jan 8, 2021 at 5:48 AM Heiko Stuebner
<heiko.stuebner@theobroma-systems.com> wrote:
>
> Hi,
>
> the rkisp driver in the mainline Linux kernel moved out of staging with
> 5.11-rc1, so the uapi will be fixed after 5.11 proper is released.
>
> The rkisp driver currently only supports the rk3399 and while working
> on porting the support for rk3326/px30 I noticed discrepancies.
>
> Hence it would be somewhat urgent to clarify this, as later it will get
> really cumbersome.
>
> ----
>
> The rkisp on the px30 (v12) has some changes compared to the rk3399 (v10).
> Some sub-blocks moved around or seem to have been replaced with newer
> variants and the gist of changes can be seen in [0] with the important
> part being the uapi changes [1] and those values also exist in mainline.
>
>
> See functions in that patch:
> - isp_goc_config_v12()
> - rkisp1_stats_get_aec_meas_v12()
> - rkisp1_stats_get_hst_meas_v12()
>
> Looking at the code, the register locations are different, for gammas and
> the histogram the actual amount of raw registers is the same, while the
> "aec" seems to use 25 registers on V10 while 21 registers on V12. Though
> their content gets split into multiple values in that v12 variant.
>
>
> As somehow expected the whole thing is pretty undocumented and I
> have no clue what these "bins" or "gammas" mean and why the amount of
> entries now differs and how this relates to userspace at all.
>
> Also looking through libcamera as the one open user of the driver,
> the whole rkisp1_cif_isp_isp_other_cfg (containing the gamma config)
> as well as the rkisp1_cif_isp_stat struct (for ae and histogram)
> don't seem to be used so far.

FWIW, another open source user of the driver is the Rockchip HAL for
Chrome OS. [1]
The link points to an unmerged version that is updated for the
upstream driver, which we want to transition to, as per the stack of
patches at [2].
For example, RKISP1_CIF_ISP_HIST_BIN_N_MAX (which
CIFISP_HIST_BIN_N_MAX was renamed to) seems to be referenced in [3].

[1] https://chromium.googlesource.com/chromiumos/platform2/+/5c055d6ae727e347c1a9fd8acad061e427b1e4ff/camera/hal/rockchip/
[2] https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2521601/
[3] https://chromium.googlesource.com/chromiumos/platform2/+/5c055d6ae727e347c1a9fd8acad061e427b1e4ff/camera/hal/rockchip/psl/rkisp1/workers/StatisticsWorker.cpp#220

Best regards,
Tomasz

>
> Hence I also added some Rockchip people in the hope of getting
> a bit of clarification ;-) .
>
>
> Ideas on how to proceed?
>
> Thanks
> Heiko
>
>
> [0] https://github.com/rockchip-linux/kernel/commit/2ff670508e8fdfefd67318e885effb8cee4a0f4c
> [1]
> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> index b471f01a8459..fbeb6b5dba03 100644
> --- a/include/uapi/linux/rkisp1-config.h
> +++ b/include/uapi/linux/rkisp1-config.h
> @@ -32,8 +32,8 @@
>  #define CIFISP_CTK_COEFF_MAX            0x100
>  #define CIFISP_CTK_OFFSET_MAX           0x800
>
> -#define CIFISP_AE_MEAN_MAX              25
> -#define CIFISP_HIST_BIN_N_MAX           16
> +#define CIFISP_AE_MEAN_MAX              81
> +#define CIFISP_HIST_BIN_N_MAX           32
>  #define CIFISP_AFM_MAX_WINDOWS          3
>  #define CIFISP_DEGAMMA_CURVE_SIZE       17
>
> @@ -69,7 +69,7 @@
>   * Gamma out
>   */
>  /* Maximum number of color samples supported */
> -#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES       34
>
>
>