Message ID | 20201209095339.4107-1-naush@raspberrypi.com |
---|---|
State | Changes Requested |
Headers | show |
Series |
|
Related | show |
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.
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 >
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.
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(-)