[libcamera-devel] [PATCH] ipa: rkisp1: Add support for V12 isp blocks

[Heiko Stübner]

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 at 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 at ideasonboard.com>
> >
> > thanks ... do I need to poke someone to apply this?
> >
> >
> > Thanks
> > Heiko
> >
> >
> > > > Signed-off-by: Heiko Stuebner <heiko.stuebner at 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;
> > > >
> > > >
> > >
> >
> >
> >
> >
>