[libcamera-devel] ipa: rkisp1: Add support for V12 isp blocks
diff mbox series

Message ID 20210621145947.53909-1-heiko@sntech.de
State Accepted
Headers show
Series
  • [libcamera-devel] ipa: rkisp1: Add support for V12 isp blocks
Related show

Commit Message

Heiko Stübner June 21, 2021, 2:59 p.m. UTC 
From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

Some values for array sizes differ between v10 and v12, so set them
in init() and adjust the auto exposure algorithm to the ae value
from there.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 src/ipa/rkisp1/rkisp1.cpp | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

Comments

Dafna Hirschfeld June 22, 2021, 7:42 a.m. UTC | #1 
On 21.06.21 17:59, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> Some values for array sizes differ between v10 and v12, so set them
> in init() and adjust the auto exposure algorithm to the ae value
> from there.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

Reviewed-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>


> ---
>   src/ipa/rkisp1/rkisp1.cpp | 23 +++++++++++++++++++++--
>   1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index b47ea324..5f529334 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -66,12 +66,31 @@ private:
>   	uint32_t gain_;
>   	uint32_t minGain_;
>   	uint32_t maxGain_;
> +
> +	/* revision-specific data */
> +	int hwAeMeanMax_;
> +	int hwHistBinNMax_;
> +	int hwGammaOutMaxSamples_;
> +	int hwHistogramWeightGridsSize_;
>   };
>   
>   int IPARkISP1::init(unsigned int hwRevision)
>   {
>   	/* \todo Add support for other revisions */
> -	if (hwRevision != RKISP1_V10) {
> +	switch (hwRevision) {
> +	case RKISP1_V10:
> +		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
> +		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;
> +		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;
> +		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;
> +		break;
> +	case RKISP1_V12:
> +		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
> +		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;
> +		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;
> +		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;
> +		break;
> +	default:
>   		LOG(IPARkISP1, Error)
>   			<< "Hardware revision " << hwRevision
>   			<< " is currently not supported";
> @@ -236,7 +255,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
>   
>   		unsigned int value = 0;
>   		unsigned int num = 0;
> -		for (int i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX_V10; i++) {
> +		for (int i = 0; i < hwAeMeanMax_; i++) {
>   			if (ae->exp_mean[i] <= 15)
>   				continue;
>   
>
Kieran Bingham July 13, 2021, 1:35 p.m. UTC | #2 
Hi Heiko,

On 21/06/2021 15:59, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> Some values for array sizes differ between v10 and v12, so set them
> in init() and adjust the auto exposure algorithm to the ae value
> from there.

This looks reasonable to me. It looks like some of these are not yet
used, but I don't see that as a problem in this instance, as the
implementation abstracts the platform version differences.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>



> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> ---
>  src/ipa/rkisp1/rkisp1.cpp | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index b47ea324..5f529334 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -66,12 +66,31 @@ private:
>  	uint32_t gain_;
>  	uint32_t minGain_;
>  	uint32_t maxGain_;
> +
> +	/* revision-specific data */
> +	int hwAeMeanMax_;
> +	int hwHistBinNMax_;
> +	int hwGammaOutMaxSamples_;
> +	int hwHistogramWeightGridsSize_;
>  };
>  
>  int IPARkISP1::init(unsigned int hwRevision)
>  {
>  	/* \todo Add support for other revisions */
> -	if (hwRevision != RKISP1_V10) {
> +	switch (hwRevision) {
> +	case RKISP1_V10:
> +		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
> +		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;
> +		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;
> +		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;
> +		break;
> +	case RKISP1_V12:
> +		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
> +		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;
> +		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;
> +		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;
> +		break;
> +	default:
>  		LOG(IPARkISP1, Error)
>  			<< "Hardware revision " << hwRevision
>  			<< " is currently not supported";
> @@ -236,7 +255,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
>  
>  		unsigned int value = 0;
>  		unsigned int num = 0;
> -		for (int i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX_V10; i++) {
> +		for (int i = 0; i < hwAeMeanMax_; i++) {
>  			if (ae->exp_mean[i] <= 15)
>  				continue;
>  
>
Heiko Stübner July 22, 2021, 9:32 a.m. UTC | #3 
Hi,

Am Dienstag, 13. Juli 2021, 15:35:52 CEST schrieb Kieran Bingham:
> Hi Heiko,
> 
> On 21/06/2021 15:59, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > 
> > Some values for array sizes differ between v10 and v12, so set them
> > in init() and adjust the auto exposure algorithm to the ae value
> > from there.
> 
> This looks reasonable to me. It looks like some of these are not yet
> used, but I don't see that as a problem in this instance, as the
> implementation abstracts the platform version differences.

That were my thoughts as well, as we already have the differentiation
in the kernel uapi for these values, so there wasn't really a need to wait ;-) .

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

thanks ... do I need to poke someone to apply this?


Thanks
Heiko


> > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > ---
> >  src/ipa/rkisp1/rkisp1.cpp | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index b47ea324..5f529334 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -66,12 +66,31 @@ private:
> >  	uint32_t gain_;
> >  	uint32_t minGain_;
> >  	uint32_t maxGain_;
> > +
> > +	/* revision-specific data */
> > +	int hwAeMeanMax_;
> > +	int hwHistBinNMax_;
> > +	int hwGammaOutMaxSamples_;
> > +	int hwHistogramWeightGridsSize_;
> >  };
> >  
> >  int IPARkISP1::init(unsigned int hwRevision)
> >  {
> >  	/* \todo Add support for other revisions */
> > -	if (hwRevision != RKISP1_V10) {
> > +	switch (hwRevision) {
> > +	case RKISP1_V10:
> > +		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
> > +		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;
> > +		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;
> > +		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;
> > +		break;
> > +	case RKISP1_V12:
> > +		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
> > +		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;
> > +		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;
> > +		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;
> > +		break;
> > +	default:
> >  		LOG(IPARkISP1, Error)
> >  			<< "Hardware revision " << hwRevision
> >  			<< " is currently not supported";
> > @@ -236,7 +255,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> >  
> >  		unsigned int value = 0;
> >  		unsigned int num = 0;
> > -		for (int i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX_V10; i++) {
> > +		for (int i = 0; i < hwAeMeanMax_; i++) {
> >  			if (ae->exp_mean[i] <= 15)
> >  				continue;
> >  
> > 
>
Jacopo Mondi July 22, 2021, 9:47 a.m. UTC | #4 
Hello Heiko,

On Thu, Jul 22, 2021 at 11:32:12AM +0200, Heiko Stübner wrote:
> Hi,
>
> Am Dienstag, 13. Juli 2021, 15:35:52 CEST schrieb Kieran Bingham:
> > Hi Heiko,
> >
> > On 21/06/2021 15:59, Heiko Stuebner wrote:
> > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > >
> > > Some values for array sizes differ between v10 and v12, so set them
> > > in init() and adjust the auto exposure algorithm to the ae value
> > > from there.
> >
> > This looks reasonable to me. It looks like some of these are not yet
> > used, but I don't see that as a problem in this instance, as the
> > implementation abstracts the platform version differences.
>
> That were my thoughts as well, as we already have the differentiation
> in the kernel uapi for these values, so there wasn't really a need to wait ;-) .
>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> thanks ... do I need to poke someone to apply this?

Sorry for the delay, I'll take the patch in!

Thanks
   j

>
>
> Thanks
> Heiko
>
>
> > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > ---
> > >  src/ipa/rkisp1/rkisp1.cpp | 23 +++++++++++++++++++++--
> > >  1 file changed, 21 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index b47ea324..5f529334 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -66,12 +66,31 @@ private:
> > >  	uint32_t gain_;
> > >  	uint32_t minGain_;
> > >  	uint32_t maxGain_;
> > > +
> > > +	/* revision-specific data */
> > > +	int hwAeMeanMax_;
> > > +	int hwHistBinNMax_;
> > > +	int hwGammaOutMaxSamples_;
> > > +	int hwHistogramWeightGridsSize_;
> > >  };
> > >
> > >  int IPARkISP1::init(unsigned int hwRevision)
> > >  {
> > >  	/* \todo Add support for other revisions */
> > > -	if (hwRevision != RKISP1_V10) {
> > > +	switch (hwRevision) {
> > > +	case RKISP1_V10:
> > > +		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
> > > +		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;
> > > +		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;
> > > +		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;
> > > +		break;
> > > +	case RKISP1_V12:
> > > +		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
> > > +		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;
> > > +		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;
> > > +		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;
> > > +		break;
> > > +	default:
> > >  		LOG(IPARkISP1, Error)
> > >  			<< "Hardware revision " << hwRevision
> > >  			<< " is currently not supported";
> > > @@ -236,7 +255,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> > >
> > >  		unsigned int value = 0;
> > >  		unsigned int num = 0;
> > > -		for (int i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX_V10; i++) {
> > > +		for (int i = 0; i < hwAeMeanMax_; i++) {
> > >  			if (ae->exp_mean[i] <= 15)
> > >  				continue;
> > >
> > >
> >
>
>
>
>
Jacopo Mondi July 22, 2021, 10:26 a.m. UTC | #5 
Hi again,

  just a small note

On Thu, Jul 22, 2021 at 11:32:12AM +0200, Heiko Stübner wrote:
> Hi,
>
> Am Dienstag, 13. Juli 2021, 15:35:52 CEST schrieb Kieran Bingham:
> > Hi Heiko,
> >
> > On 21/06/2021 15:59, Heiko Stuebner wrote:
> > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > >
> > > Some values for array sizes differ between v10 and v12, so set them
> > > in init() and adjust the auto exposure algorithm to the ae value
> > > from there.
> >
> > This looks reasonable to me. It looks like some of these are not yet
> > used, but I don't see that as a problem in this instance, as the
> > implementation abstracts the platform version differences.
>
> That were my thoughts as well, as we already have the differentiation
> in the kernel uapi for these values, so there wasn't really a need to wait ;-) .
>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> thanks ... do I need to poke someone to apply this?
>
>
> Thanks
> Heiko
>
>
> > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > ---
> > >  src/ipa/rkisp1/rkisp1.cpp | 23 +++++++++++++++++++++--
> > >  1 file changed, 21 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index b47ea324..5f529334 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -66,12 +66,31 @@ private:
> > >  	uint32_t gain_;
> > >  	uint32_t minGain_;
> > >  	uint32_t maxGain_;
> > > +
> > > +	/* revision-specific data */
> > > +	int hwAeMeanMax_;
> > > +	int hwHistBinNMax_;
> > > +	int hwGammaOutMaxSamples_;
> > > +	int hwHistogramWeightGridsSize_;

Should these be unsigned ?

> > >  };
> > >
> > >  int IPARkISP1::init(unsigned int hwRevision)
> > >  {
> > >  	/* \todo Add support for other revisions */
> > > -	if (hwRevision != RKISP1_V10) {
> > > +	switch (hwRevision) {
> > > +	case RKISP1_V10:
> > > +		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
> > > +		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;
> > > +		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;
> > > +		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;
> > > +		break;
> > > +	case RKISP1_V12:
> > > +		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
> > > +		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;
> > > +		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;
> > > +		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;
> > > +		break;
> > > +	default:
> > >  		LOG(IPARkISP1, Error)
> > >  			<< "Hardware revision " << hwRevision
> > >  			<< " is currently not supported";
> > > @@ -236,7 +255,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> > >
> > >  		unsigned int value = 0;
> > >  		unsigned int num = 0;
> > > -		for (int i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX_V10; i++) {
> > > +		for (int i = 0; i < hwAeMeanMax_; i++) {

as well as the i loop counter

with your ack I'll fix when applying

Thanks
  j

> > >  			if (ae->exp_mean[i] <= 15)
> > >  				continue;
> > >
> > >
> >
>
>
>
>
Heiko Stübner July 22, 2021, 10:59 a.m. UTC | #6 
Hi,

Am Donnerstag, 22. Juli 2021, 12:26:19 CEST schrieb Jacopo Mondi:
> Hi again,
> 
>   just a small note
> 
> On Thu, Jul 22, 2021 at 11:32:12AM +0200, Heiko Stübner wrote:
> > Hi,
> >
> > Am Dienstag, 13. Juli 2021, 15:35:52 CEST schrieb Kieran Bingham:
> > > Hi Heiko,
> > >
> > > On 21/06/2021 15:59, Heiko Stuebner wrote:
> > > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > >
> > > > Some values for array sizes differ between v10 and v12, so set them
> > > > in init() and adjust the auto exposure algorithm to the ae value
> > > > from there.
> > >
> > > This looks reasonable to me. It looks like some of these are not yet
> > > used, but I don't see that as a problem in this instance, as the
> > > implementation abstracts the platform version differences.
> >
> > That were my thoughts as well, as we already have the differentiation
> > in the kernel uapi for these values, so there wasn't really a need to wait ;-) .
> >
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > thanks ... do I need to poke someone to apply this?
> >
> >
> > Thanks
> > Heiko
> >
> >
> > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > > ---
> > > >  src/ipa/rkisp1/rkisp1.cpp | 23 +++++++++++++++++++++--
> > > >  1 file changed, 21 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > index b47ea324..5f529334 100644
> > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > @@ -66,12 +66,31 @@ private:
> > > >  	uint32_t gain_;
> > > >  	uint32_t minGain_;
> > > >  	uint32_t maxGain_;
> > > > +
> > > > +	/* revision-specific data */
> > > > +	int hwAeMeanMax_;
> > > > +	int hwHistBinNMax_;
> > > > +	int hwGammaOutMaxSamples_;
> > > > +	int hwHistogramWeightGridsSize_;
> 
> Should these be unsigned ?
> 
> > > >  };
> > > >
> > > >  int IPARkISP1::init(unsigned int hwRevision)
> > > >  {
> > > >  	/* \todo Add support for other revisions */
> > > > -	if (hwRevision != RKISP1_V10) {
> > > > +	switch (hwRevision) {
> > > > +	case RKISP1_V10:
> > > > +		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
> > > > +		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;
> > > > +		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;
> > > > +		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;
> > > > +		break;
> > > > +	case RKISP1_V12:
> > > > +		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
> > > > +		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;
> > > > +		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;
> > > > +		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;
> > > > +		break;
> > > > +	default:
> > > >  		LOG(IPARkISP1, Error)
> > > >  			<< "Hardware revision " << hwRevision
> > > >  			<< " is currently not supported";
> > > > @@ -236,7 +255,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> > > >
> > > >  		unsigned int value = 0;
> > > >  		unsigned int num = 0;
> > > > -		for (int i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX_V10; i++) {
> > > > +		for (int i = 0; i < hwAeMeanMax_; i++) {
> 
> as well as the i loop counter
> 
> with your ack I'll fix when applying

I don't think there is anything speaking against them being
unsigned, so sure go ahead and change them :-)

Thanks
Heiko


> 
> Thanks
>   j
> 
> > > >  			if (ae->exp_mean[i] <= 15)
> > > >  				continue;
> > > >
> > > >
> > >
> >
> >
> >
> >
>
Jacopo Mondi July 22, 2021, 12:40 p.m. UTC | #7 
Hi Heiko,

On Thu, Jul 22, 2021 at 12:59:11PM +0200, Heiko Stübner wrote:
> Hi,
>
> Am Donnerstag, 22. Juli 2021, 12:26:19 CEST schrieb Jacopo Mondi:
> > Hi again,
> >
> >   just a small note
> >
> > On Thu, Jul 22, 2021 at 11:32:12AM +0200, Heiko Stübner wrote:
> > > Hi,
> > >
> > > Am Dienstag, 13. Juli 2021, 15:35:52 CEST schrieb Kieran Bingham:
> > > > Hi Heiko,
> > > >
> > > > On 21/06/2021 15:59, Heiko Stuebner wrote:
> > > > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > > >
> > > > > Some values for array sizes differ between v10 and v12, so set them
> > > > > in init() and adjust the auto exposure algorithm to the ae value
> > > > > from there.
> > > >
> > > > This looks reasonable to me. It looks like some of these are not yet
> > > > used, but I don't see that as a problem in this instance, as the
> > > > implementation abstracts the platform version differences.
> > >
> > > That were my thoughts as well, as we already have the differentiation
> > > in the kernel uapi for these values, so there wasn't really a need to wait ;-) .
> > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >
> > > thanks ... do I need to poke someone to apply this?
> > >
> > >
> > > Thanks
> > > Heiko
> > >
> > >
> > > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > > > ---
> > > > >  src/ipa/rkisp1/rkisp1.cpp | 23 +++++++++++++++++++++--
> > > > >  1 file changed, 21 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > > index b47ea324..5f529334 100644
> > > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > > @@ -66,12 +66,31 @@ private:
> > > > >  	uint32_t gain_;
> > > > >  	uint32_t minGain_;
> > > > >  	uint32_t maxGain_;
> > > > > +
> > > > > +	/* revision-specific data */
> > > > > +	int hwAeMeanMax_;
> > > > > +	int hwHistBinNMax_;
> > > > > +	int hwGammaOutMaxSamples_;
> > > > > +	int hwHistogramWeightGridsSize_;
> >
> > Should these be unsigned ?
> >
> > > > >  };
> > > > >
> > > > >  int IPARkISP1::init(unsigned int hwRevision)
> > > > >  {
> > > > >  	/* \todo Add support for other revisions */
> > > > > -	if (hwRevision != RKISP1_V10) {
> > > > > +	switch (hwRevision) {
> > > > > +	case RKISP1_V10:
> > > > > +		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
> > > > > +		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;
> > > > > +		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;
> > > > > +		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;
> > > > > +		break;
> > > > > +	case RKISP1_V12:
> > > > > +		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
> > > > > +		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;
> > > > > +		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;
> > > > > +		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;
> > > > > +		break;
> > > > > +	default:
> > > > >  		LOG(IPARkISP1, Error)
> > > > >  			<< "Hardware revision " << hwRevision
> > > > >  			<< " is currently not supported";
> > > > > @@ -236,7 +255,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> > > > >
> > > > >  		unsigned int value = 0;
> > > > >  		unsigned int num = 0;
> > > > > -		for (int i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX_V10; i++) {
> > > > > +		for (int i = 0; i < hwAeMeanMax_; i++) {
> >
> > as well as the i loop counter
> >
> > with your ack I'll fix when applying
>
> I don't think there is anything speaking against them being
> unsigned, so sure go ahead and change them :-)

Great, now applied

Thanks
  j

>
> Thanks
> Heiko
>
>
> >
> > Thanks
> >   j
> >
> > > > >  			if (ae->exp_mean[i] <= 15)
> > > > >  				continue;
> > > > >
> > > > >
> > > >
> > >
> > >
> > >
> > >
> >
>
>
>
>

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index b47ea324..5f529334 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -66,12 +66,31 @@  private:
 	uint32_t gain_;
 	uint32_t minGain_;
 	uint32_t maxGain_;
+
+	/* revision-specific data */
+	int hwAeMeanMax_;
+	int hwHistBinNMax_;
+	int hwGammaOutMaxSamples_;
+	int hwHistogramWeightGridsSize_;
 };
 
 int IPARkISP1::init(unsigned int hwRevision)
 {
 	/* \todo Add support for other revisions */
-	if (hwRevision != RKISP1_V10) {
+	switch (hwRevision) {
+	case RKISP1_V10:
+		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
+		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;
+		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;
+		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;
+		break;
+	case RKISP1_V12:
+		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
+		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;
+		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;
+		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;
+		break;
+	default:
 		LOG(IPARkISP1, Error)
 			<< "Hardware revision " << hwRevision
 			<< " is currently not supported";
@@ -236,7 +255,7 @@  void IPARkISP1::updateStatistics(unsigned int frame,
 
 		unsigned int value = 0;
 		unsigned int num = 0;
-		for (int i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX_V10; i++) {
+		for (int i = 0; i < hwAeMeanMax_; i++) {
 			if (ae->exp_mean[i] <= 15)
 				continue;