[libcamera-devel] [PATCH] ipa: raspberrypi: Only validate ISP and Unicam controls during configure
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Dec 11 22:00:45 CET 2020
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 at 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.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list