[libcamera-devel] ipa: raspberrypi: Only validate ISP and Unicam controls during configure
diff mbox series

Message ID 20201209095339.4107-1-naush@raspberrypi.com
State Changes Requested
Headers show
Series
  • [libcamera-devel] ipa: raspberrypi: Only validate ISP and Unicam controls during configure
Related show

Commit Message

Naushir Patuck Dec. 9, 2020, 9:53 a.m. UTC 
There is no need to validate all the ISP and Unicam V4L2 controls on
every frame. Simply validate them once during the IPA configuration, and
fail if a required control is not available.

Also add some whitespace for readability.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/raspberrypi/raspberrypi.cpp | 131 ++++++++++++++--------------
 1 file changed, 64 insertions(+), 67 deletions(-)

Comments

Laurent Pinchart Dec. 11, 2020, 9 p.m. UTC | #1 
Hi Naush,

Thank you for the patch.

On Wed, Dec 09, 2020 at 09:53:39AM +0000, Naushir Patuck wrote:
> There is no need to validate all the ISP and Unicam V4L2 controls on
> every frame. Simply validate them once during the IPA configuration, and
> fail if a required control is not available.
> 
> Also add some whitespace for readability.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/raspberrypi.cpp | 131 ++++++++++++++--------------
>  1 file changed, 64 insertions(+), 67 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 60cfdc27..e5d2b41e 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -92,6 +92,8 @@ public:
>  
>  private:
>  	void setMode(const CameraSensorInfo &sensorInfo);
> +	bool validateUnicamControls();
> +	bool validateIspControls();
>  	void queueRequest(const ControlList &controls);
>  	void returnEmbeddedBuffer(unsigned int bufferId);
>  	void prepareISP(unsigned int bufferId);
> @@ -238,6 +240,16 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  	unicamCtrls_ = entityControls.at(0);
>  	ispCtrls_ = entityControls.at(1);
>  
> +	if (!validateUnicamControls()) {
> +		LOG(IPARPI, Error) << "Unicam control validation failed.";
> +		return -EINVAL;
> +	}
> +
> +	if (!validateIspControls()) {
> +		LOG(IPARPI, Error) << "ISP control validation failed.";
> +		return -EINVAL;
> +	}

configure() is a void function. Rebase issue ?

> +
>  	/* Setup a metadata ControlList to output metadata. */
>  	libcameraMetadata_ = ControlList(controls::controls);
>  
> @@ -473,6 +485,51 @@ void IPARPi::reportMetadata()
>  	}
>  }
>  
> +bool IPARPi::validateUnicamControls()
> +{
> +	const uint32_t ctrls[] = {

static const ? Same below.

> +		V4L2_CID_ANALOGUE_GAIN,
> +		V4L2_CID_EXPOSURE
> +	};
> +
> +	for (auto c : ctrls) {
> +		if (unicamCtrls_.find(c) == unicamCtrls_.end()) {
> +			LOG(IPARPI, Error) << "Unable to find Unicam control "
> +					   << c;

I'd use utils::hex(c) as otherwise the value is hard to read. Same
below.

> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}

Isn't this sensor controls, not unicam controls ? While at it, we could
rename unicamCtrls_ to sensorCtrls_.

The rest looks good to me.

> +
> +bool IPARPi::validateIspControls()
> +{
> +	const uint32_t ctrls[] = {
> +		V4L2_CID_RED_BALANCE,
> +		V4L2_CID_BLUE_BALANCE,
> +		V4L2_CID_DIGITAL_GAIN,
> +		V4L2_CID_USER_BCM2835_ISP_CC_MATRIX,
> +		V4L2_CID_USER_BCM2835_ISP_GAMMA,
> +		V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL,
> +		V4L2_CID_USER_BCM2835_ISP_GEQ,
> +		V4L2_CID_USER_BCM2835_ISP_DENOISE,
> +		V4L2_CID_USER_BCM2835_ISP_SHARPEN,
> +		V4L2_CID_USER_BCM2835_ISP_DPC,
> +		V4L2_CID_USER_BCM2835_ISP_LENS_SHADING
> +	};
> +
> +	for (auto c : ctrls) {
> +		if (ispCtrls_.find(c) == ispCtrls_.end()) {
> +			LOG(IPARPI, Error) << "Unable to find ISP control "
> +					   << c;
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>  /*
>   * Converting between enums (used in the libcamera API) and the names that
>   * we use to identify different modes. Unfortunately, the conversion tables
> @@ -858,18 +915,6 @@ void IPARPi::processStats(unsigned int bufferId)
>  
>  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
>  {
> -	const auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);
> -	if (gainR == ispCtrls_.end()) {
> -		LOG(IPARPI, Error) << "Can't find red gain control";
> -		return;
> -	}
> -
> -	const auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);
> -	if (gainB == ispCtrls_.end()) {
> -		LOG(IPARPI, Error) << "Can't find blue gain control";
> -		return;
> -	}
> -
>  	LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gain_r << " B: "
>  			   << awbStatus->gain_b;
>  
> @@ -884,16 +929,6 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>  	int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
>  	int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);
>  
> -	if (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {
> -		LOG(IPARPI, Error) << "Can't find analogue gain control";
> -		return;
> -	}
> -
> -	if (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {
> -		LOG(IPARPI, Error) << "Can't find exposure control";
> -		return;
> -	}
> -
>  	LOG(IPARPI, Debug) << "Applying AGC Exposure: " << agcStatus->shutter_time
>  			   << " (Shutter lines: " << exposureLines << ") Gain: "
>  			   << agcStatus->analogue_gain << " (Gain Code: "
> @@ -905,23 +940,14 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>  
>  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
>  {
> -	if (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {
> -		LOG(IPARPI, Error) << "Can't find digital gain control";
> -		return;
> -	}
> -
>  	ctrls.set(V4L2_CID_DIGITAL_GAIN,
>  		  static_cast<int32_t>(dgStatus->digital_gain * 1000));
>  }
>  
>  void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
>  {
> -	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {
> -		LOG(IPARPI, Error) << "Can't find CCM control";
> -		return;
> -	}
> -
>  	bcm2835_isp_custom_ccm ccm;
> +
>  	for (int i = 0; i < 9; i++) {
>  		ccm.ccm.ccm[i / 3][i % 3].den = 1000;
>  		ccm.ccm.ccm[i / 3][i % 3].num = 1000 * ccmStatus->matrix[i];
> @@ -937,12 +963,8 @@ void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
>  
>  void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)
>  {
> -	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {
> -		LOG(IPARPI, Error) << "Can't find Gamma control";
> -		return;
> -	}
> -
>  	struct bcm2835_isp_gamma gamma;
> +
>  	gamma.enabled = 1;
>  	for (int i = 0; i < CONTRAST_NUM_POINTS; i++) {
>  		gamma.x[i] = contrastStatus->points[i].x;
> @@ -956,12 +978,8 @@ void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList
>  
>  void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)
>  {
> -	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {
> -		LOG(IPARPI, Error) << "Can't find black level control";
> -		return;
> -	}
> -
>  	bcm2835_isp_black_level blackLevel;
> +
>  	blackLevel.enabled = 1;
>  	blackLevel.black_level_r = blackLevelStatus->black_level_r;
>  	blackLevel.black_level_g = blackLevelStatus->black_level_g;
> @@ -974,12 +992,8 @@ void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co
>  
>  void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
>  {
> -	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {
> -		LOG(IPARPI, Error) << "Can't find geq control";
> -		return;
> -	}
> -
>  	bcm2835_isp_geq geq;
> +
>  	geq.enabled = 1;
>  	geq.offset = geqStatus->offset;
>  	geq.slope.den = 1000;
> @@ -992,12 +1006,8 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
>  
>  void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)
>  {
> -	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {
> -		LOG(IPARPI, Error) << "Can't find denoise control";
> -		return;
> -	}
> -
>  	bcm2835_isp_denoise denoise;
> +
>  	denoise.enabled = 1;
>  	denoise.constant = denoiseStatus->noise_constant;
>  	denoise.slope.num = 1000 * denoiseStatus->noise_slope;
> @@ -1012,12 +1022,8 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct
>  
>  void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)
>  {
> -	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {
> -		LOG(IPARPI, Error) << "Can't find sharpen control";
> -		return;
> -	}
> -
>  	bcm2835_isp_sharpen sharpen;
> +
>  	sharpen.enabled = 1;
>  	sharpen.threshold.num = 1000 * sharpenStatus->threshold;
>  	sharpen.threshold.den = 1000;
> @@ -1033,12 +1039,8 @@ void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList
>  
>  void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
>  {
> -	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {
> -		LOG(IPARPI, Error) << "Can't find DPC control";
> -		return;
> -	}
> -
>  	bcm2835_isp_dpc dpc;
> +
>  	dpc.enabled = 1;
>  	dpc.strength = dpcStatus->strength;
>  
> @@ -1049,11 +1051,6 @@ void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
>  
>  void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>  {
> -	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {
> -		LOG(IPARPI, Error) << "Can't find LS control";
> -		return;
> -	}
> -
>  	/*
>  	 * Program lens shading tables into pipeline.
>  	 * Choose smallest cell size that won't exceed 63x48 cells.
Naushir Patuck Dec. 11, 2020, 9:41 p.m. UTC | #2 
Hi Laurent,

Thank you for your review comments.




On Fri, 11 Dec 2020, 9:00 pm Laurent Pinchart, <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Wed, Dec 09, 2020 at 09:53:39AM +0000, Naushir Patuck wrote:
> > There is no need to validate all the ISP and Unicam V4L2 controls on
> > every frame. Simply validate them once during the IPA configuration, and
> > fail if a required control is not available.
> >
> > Also add some whitespace for readability.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/raspberrypi.cpp | 131 ++++++++++++++--------------
> >  1 file changed, 64 insertions(+), 67 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 60cfdc27..e5d2b41e 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -92,6 +92,8 @@ public:
> >
> >  private:
> >       void setMode(const CameraSensorInfo &sensorInfo);
> > +     bool validateUnicamControls();
> > +     bool validateIspControls();
> >       void queueRequest(const ControlList &controls);
> >       void returnEmbeddedBuffer(unsigned int bufferId);
> >       void prepareISP(unsigned int bufferId);
> > @@ -238,6 +240,16 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> >       unicamCtrls_ = entityControls.at(0);
> >       ispCtrls_ = entityControls.at(1);
> >
> > +     if (!validateUnicamControls()) {
> > +             LOG(IPARPI, Error) << "Unicam control validation failed.";
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (!validateIspControls()) {
> > +             LOG(IPARPI, Error) << "ISP control validation failed.";
> > +             return -EINVAL;
> > +     }
>
> configure() is a void function. Rebase issue ?
>

I'm sorry I really should have withdrawn this patch. It was based off your
changes for returning error codes from configure(). I jumped the gun a bit
in submitting this.

Now that we are using my alternate mechanism, I will resubmit with the
appropriate changes. I will also address the points below.

Regards,
Naush


> > +
> >       /* Setup a metadata ControlList to output metadata. */
> >       libcameraMetadata_ = ControlList(controls::controls);
> >
> > @@ -473,6 +485,51 @@ void IPARPi::reportMetadata()
> >       }
> >  }
> >
> > +bool IPARPi::validateUnicamControls()
> > +{
> > +     const uint32_t ctrls[] = {
>
> static const ? Same below.
>
> > +             V4L2_CID_ANALOGUE_GAIN,
> > +             V4L2_CID_EXPOSURE
> > +     };
> > +
> > +     for (auto c : ctrls) {
> > +             if (unicamCtrls_.find(c) == unicamCtrls_.end()) {
> > +                     LOG(IPARPI, Error) << "Unable to find Unicam
> control "
> > +                                        << c;
>
> I'd use utils::hex(c) as otherwise the value is hard to read. Same
> below.
>
> > +                     return false;
> > +             }
> > +     }
> > +
> > +     return true;
> > +}
>
> Isn't this sensor controls, not unicam controls ? While at it, we could
> rename unicamCtrls_ to sensorCtrls_.
>
> The rest looks good to me.
>
> > +
> > +bool IPARPi::validateIspControls()
> > +{
> > +     const uint32_t ctrls[] = {
> > +             V4L2_CID_RED_BALANCE,
> > +             V4L2_CID_BLUE_BALANCE,
> > +             V4L2_CID_DIGITAL_GAIN,
> > +             V4L2_CID_USER_BCM2835_ISP_CC_MATRIX,
> > +             V4L2_CID_USER_BCM2835_ISP_GAMMA,
> > +             V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL,
> > +             V4L2_CID_USER_BCM2835_ISP_GEQ,
> > +             V4L2_CID_USER_BCM2835_ISP_DENOISE,
> > +             V4L2_CID_USER_BCM2835_ISP_SHARPEN,
> > +             V4L2_CID_USER_BCM2835_ISP_DPC,
> > +             V4L2_CID_USER_BCM2835_ISP_LENS_SHADING
> > +     };
> > +
> > +     for (auto c : ctrls) {
> > +             if (ispCtrls_.find(c) == ispCtrls_.end()) {
> > +                     LOG(IPARPI, Error) << "Unable to find ISP control "
> > +                                        << c;
> > +                     return false;
> > +             }
> > +     }
> > +
> > +     return true;
> > +}
> > +
> >  /*
> >   * Converting between enums (used in the libcamera API) and the names
> that
> >   * we use to identify different modes. Unfortunately, the conversion
> tables
> > @@ -858,18 +915,6 @@ void IPARPi::processStats(unsigned int bufferId)
> >
> >  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList
> &ctrls)
> >  {
> > -     const auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);
> > -     if (gainR == ispCtrls_.end()) {
> > -             LOG(IPARPI, Error) << "Can't find red gain control";
> > -             return;
> > -     }
> > -
> > -     const auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);
> > -     if (gainB == ispCtrls_.end()) {
> > -             LOG(IPARPI, Error) << "Can't find blue gain control";
> > -             return;
> > -     }
> > -
> >       LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gain_r << "
> B: "
> >                          << awbStatus->gain_b;
> >
> > @@ -884,16 +929,6 @@ void IPARPi::applyAGC(const struct AgcStatus
> *agcStatus, ControlList &ctrls)
> >       int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
> >       int32_t exposureLines =
> helper_->ExposureLines(agcStatus->shutter_time);
> >
> > -     if (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) ==
> unicamCtrls_.end()) {
> > -             LOG(IPARPI, Error) << "Can't find analogue gain control";
> > -             return;
> > -     }
> > -
> > -     if (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {
> > -             LOG(IPARPI, Error) << "Can't find exposure control";
> > -             return;
> > -     }
> > -
> >       LOG(IPARPI, Debug) << "Applying AGC Exposure: " <<
> agcStatus->shutter_time
> >                          << " (Shutter lines: " << exposureLines << ")
> Gain: "
> >                          << agcStatus->analogue_gain << " (Gain Code: "
> > @@ -905,23 +940,14 @@ void IPARPi::applyAGC(const struct AgcStatus
> *agcStatus, ControlList &ctrls)
> >
> >  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList
> &ctrls)
> >  {
> > -     if (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {
> > -             LOG(IPARPI, Error) << "Can't find digital gain control";
> > -             return;
> > -     }
> > -
> >       ctrls.set(V4L2_CID_DIGITAL_GAIN,
> >                 static_cast<int32_t>(dgStatus->digital_gain * 1000));
> >  }
> >
> >  void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList
> &ctrls)
> >  {
> > -     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) ==
> ispCtrls_.end()) {
> > -             LOG(IPARPI, Error) << "Can't find CCM control";
> > -             return;
> > -     }
> > -
> >       bcm2835_isp_custom_ccm ccm;
> > +
> >       for (int i = 0; i < 9; i++) {
> >               ccm.ccm.ccm[i / 3][i % 3].den = 1000;
> >               ccm.ccm.ccm[i / 3][i % 3].num = 1000 *
> ccmStatus->matrix[i];
> > @@ -937,12 +963,8 @@ void IPARPi::applyCCM(const struct CcmStatus
> *ccmStatus, ControlList &ctrls)
> >
> >  void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus,
> ControlList &ctrls)
> >  {
> > -     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) ==
> ispCtrls_.end()) {
> > -             LOG(IPARPI, Error) << "Can't find Gamma control";
> > -             return;
> > -     }
> > -
> >       struct bcm2835_isp_gamma gamma;
> > +
> >       gamma.enabled = 1;
> >       for (int i = 0; i < CONTRAST_NUM_POINTS; i++) {
> >               gamma.x[i] = contrastStatus->points[i].x;
> > @@ -956,12 +978,8 @@ void IPARPi::applyGamma(const struct ContrastStatus
> *contrastStatus, ControlList
> >
> >  void IPARPi::applyBlackLevel(const struct BlackLevelStatus
> *blackLevelStatus, ControlList &ctrls)
> >  {
> > -     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) ==
> ispCtrls_.end()) {
> > -             LOG(IPARPI, Error) << "Can't find black level control";
> > -             return;
> > -     }
> > -
> >       bcm2835_isp_black_level blackLevel;
> > +
> >       blackLevel.enabled = 1;
> >       blackLevel.black_level_r = blackLevelStatus->black_level_r;
> >       blackLevel.black_level_g = blackLevelStatus->black_level_g;
> > @@ -974,12 +992,8 @@ void IPARPi::applyBlackLevel(const struct
> BlackLevelStatus *blackLevelStatus, Co
> >
> >  void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList
> &ctrls)
> >  {
> > -     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) ==
> ispCtrls_.end()) {
> > -             LOG(IPARPI, Error) << "Can't find geq control";
> > -             return;
> > -     }
> > -
> >       bcm2835_isp_geq geq;
> > +
> >       geq.enabled = 1;
> >       geq.offset = geqStatus->offset;
> >       geq.slope.den = 1000;
> > @@ -992,12 +1006,8 @@ void IPARPi::applyGEQ(const struct GeqStatus
> *geqStatus, ControlList &ctrls)
> >
> >  void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus,
> ControlList &ctrls)
> >  {
> > -     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) ==
> ispCtrls_.end()) {
> > -             LOG(IPARPI, Error) << "Can't find denoise control";
> > -             return;
> > -     }
> > -
> >       bcm2835_isp_denoise denoise;
> > +
> >       denoise.enabled = 1;
> >       denoise.constant = denoiseStatus->noise_constant;
> >       denoise.slope.num = 1000 * denoiseStatus->noise_slope;
> > @@ -1012,12 +1022,8 @@ void IPARPi::applyDenoise(const struct SdnStatus
> *denoiseStatus, ControlList &ct
> >
> >  void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus,
> ControlList &ctrls)
> >  {
> > -     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) ==
> ispCtrls_.end()) {
> > -             LOG(IPARPI, Error) << "Can't find sharpen control";
> > -             return;
> > -     }
> > -
> >       bcm2835_isp_sharpen sharpen;
> > +
> >       sharpen.enabled = 1;
> >       sharpen.threshold.num = 1000 * sharpenStatus->threshold;
> >       sharpen.threshold.den = 1000;
> > @@ -1033,12 +1039,8 @@ void IPARPi::applySharpen(const struct
> SharpenStatus *sharpenStatus, ControlList
> >
> >  void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList
> &ctrls)
> >  {
> > -     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) ==
> ispCtrls_.end()) {
> > -             LOG(IPARPI, Error) << "Can't find DPC control";
> > -             return;
> > -     }
> > -
> >       bcm2835_isp_dpc dpc;
> > +
> >       dpc.enabled = 1;
> >       dpc.strength = dpcStatus->strength;
> >
> > @@ -1049,11 +1051,6 @@ void IPARPi::applyDPC(const struct DpcStatus
> *dpcStatus, ControlList &ctrls)
> >
> >  void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList
> &ctrls)
> >  {
> > -     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) ==
> ispCtrls_.end()) {
> > -             LOG(IPARPI, Error) << "Can't find LS control";
> > -             return;
> > -     }
> > -
> >       /*
> >        * Program lens shading tables into pipeline.
> >        * Choose smallest cell size that won't exceed 63x48 cells.
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 60cfdc27..e5d2b41e 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -92,6 +92,8 @@  public:
 
 private:
 	void setMode(const CameraSensorInfo &sensorInfo);
+	bool validateUnicamControls();
+	bool validateIspControls();
 	void queueRequest(const ControlList &controls);
 	void returnEmbeddedBuffer(unsigned int bufferId);
 	void prepareISP(unsigned int bufferId);
@@ -238,6 +240,16 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 	unicamCtrls_ = entityControls.at(0);
 	ispCtrls_ = entityControls.at(1);
 
+	if (!validateUnicamControls()) {
+		LOG(IPARPI, Error) << "Unicam control validation failed.";
+		return -EINVAL;
+	}
+
+	if (!validateIspControls()) {
+		LOG(IPARPI, Error) << "ISP control validation failed.";
+		return -EINVAL;
+	}
+
 	/* Setup a metadata ControlList to output metadata. */
 	libcameraMetadata_ = ControlList(controls::controls);
 
@@ -473,6 +485,51 @@  void IPARPi::reportMetadata()
 	}
 }
 
+bool IPARPi::validateUnicamControls()
+{
+	const uint32_t ctrls[] = {
+		V4L2_CID_ANALOGUE_GAIN,
+		V4L2_CID_EXPOSURE
+	};
+
+	for (auto c : ctrls) {
+		if (unicamCtrls_.find(c) == unicamCtrls_.end()) {
+			LOG(IPARPI, Error) << "Unable to find Unicam control "
+					   << c;
+			return false;
+		}
+	}
+
+	return true;
+}
+
+bool IPARPi::validateIspControls()
+{
+	const uint32_t ctrls[] = {
+		V4L2_CID_RED_BALANCE,
+		V4L2_CID_BLUE_BALANCE,
+		V4L2_CID_DIGITAL_GAIN,
+		V4L2_CID_USER_BCM2835_ISP_CC_MATRIX,
+		V4L2_CID_USER_BCM2835_ISP_GAMMA,
+		V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL,
+		V4L2_CID_USER_BCM2835_ISP_GEQ,
+		V4L2_CID_USER_BCM2835_ISP_DENOISE,
+		V4L2_CID_USER_BCM2835_ISP_SHARPEN,
+		V4L2_CID_USER_BCM2835_ISP_DPC,
+		V4L2_CID_USER_BCM2835_ISP_LENS_SHADING
+	};
+
+	for (auto c : ctrls) {
+		if (ispCtrls_.find(c) == ispCtrls_.end()) {
+			LOG(IPARPI, Error) << "Unable to find ISP control "
+					   << c;
+			return false;
+		}
+	}
+
+	return true;
+}
+
 /*
  * Converting between enums (used in the libcamera API) and the names that
  * we use to identify different modes. Unfortunately, the conversion tables
@@ -858,18 +915,6 @@  void IPARPi::processStats(unsigned int bufferId)
 
 void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
 {
-	const auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);
-	if (gainR == ispCtrls_.end()) {
-		LOG(IPARPI, Error) << "Can't find red gain control";
-		return;
-	}
-
-	const auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);
-	if (gainB == ispCtrls_.end()) {
-		LOG(IPARPI, Error) << "Can't find blue gain control";
-		return;
-	}
-
 	LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gain_r << " B: "
 			   << awbStatus->gain_b;
 
@@ -884,16 +929,6 @@  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
 	int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
 	int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);
 
-	if (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {
-		LOG(IPARPI, Error) << "Can't find analogue gain control";
-		return;
-	}
-
-	if (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {
-		LOG(IPARPI, Error) << "Can't find exposure control";
-		return;
-	}
-
 	LOG(IPARPI, Debug) << "Applying AGC Exposure: " << agcStatus->shutter_time
 			   << " (Shutter lines: " << exposureLines << ") Gain: "
 			   << agcStatus->analogue_gain << " (Gain Code: "
@@ -905,23 +940,14 @@  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
 
 void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
 {
-	if (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {
-		LOG(IPARPI, Error) << "Can't find digital gain control";
-		return;
-	}
-
 	ctrls.set(V4L2_CID_DIGITAL_GAIN,
 		  static_cast<int32_t>(dgStatus->digital_gain * 1000));
 }
 
 void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
 {
-	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {
-		LOG(IPARPI, Error) << "Can't find CCM control";
-		return;
-	}
-
 	bcm2835_isp_custom_ccm ccm;
+
 	for (int i = 0; i < 9; i++) {
 		ccm.ccm.ccm[i / 3][i % 3].den = 1000;
 		ccm.ccm.ccm[i / 3][i % 3].num = 1000 * ccmStatus->matrix[i];
@@ -937,12 +963,8 @@  void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
 
 void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)
 {
-	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {
-		LOG(IPARPI, Error) << "Can't find Gamma control";
-		return;
-	}
-
 	struct bcm2835_isp_gamma gamma;
+
 	gamma.enabled = 1;
 	for (int i = 0; i < CONTRAST_NUM_POINTS; i++) {
 		gamma.x[i] = contrastStatus->points[i].x;
@@ -956,12 +978,8 @@  void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList
 
 void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)
 {
-	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {
-		LOG(IPARPI, Error) << "Can't find black level control";
-		return;
-	}
-
 	bcm2835_isp_black_level blackLevel;
+
 	blackLevel.enabled = 1;
 	blackLevel.black_level_r = blackLevelStatus->black_level_r;
 	blackLevel.black_level_g = blackLevelStatus->black_level_g;
@@ -974,12 +992,8 @@  void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co
 
 void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
 {
-	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {
-		LOG(IPARPI, Error) << "Can't find geq control";
-		return;
-	}
-
 	bcm2835_isp_geq geq;
+
 	geq.enabled = 1;
 	geq.offset = geqStatus->offset;
 	geq.slope.den = 1000;
@@ -992,12 +1006,8 @@  void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
 
 void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)
 {
-	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {
-		LOG(IPARPI, Error) << "Can't find denoise control";
-		return;
-	}
-
 	bcm2835_isp_denoise denoise;
+
 	denoise.enabled = 1;
 	denoise.constant = denoiseStatus->noise_constant;
 	denoise.slope.num = 1000 * denoiseStatus->noise_slope;
@@ -1012,12 +1022,8 @@  void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct
 
 void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)
 {
-	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {
-		LOG(IPARPI, Error) << "Can't find sharpen control";
-		return;
-	}
-
 	bcm2835_isp_sharpen sharpen;
+
 	sharpen.enabled = 1;
 	sharpen.threshold.num = 1000 * sharpenStatus->threshold;
 	sharpen.threshold.den = 1000;
@@ -1033,12 +1039,8 @@  void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList
 
 void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
 {
-	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {
-		LOG(IPARPI, Error) << "Can't find DPC control";
-		return;
-	}
-
 	bcm2835_isp_dpc dpc;
+
 	dpc.enabled = 1;
 	dpc.strength = dpcStatus->strength;
 
@@ -1049,11 +1051,6 @@  void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
 
 void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
 {
-	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {
-		LOG(IPARPI, Error) << "Can't find LS control";
-		return;
-	}
-
 	/*
 	 * Program lens shading tables into pipeline.
 	 * Choose smallest cell size that won't exceed 63x48 cells.