[libcamera-devel] [PATCH] ipa: raspberrypi: Only validate ISP and Unicam controls during configure
Naushir Patuck
naush at raspberrypi.com
Fri Dec 11 22:41:16 CET 2020
Hi Laurent,
Thank you for your review comments.
On Fri, 11 Dec 2020, 9:00 pm Laurent Pinchart, <
laurent.pinchart at 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 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 ?
>
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201211/b9507453/attachment-0001.htm>
More information about the libcamera-devel
mailing list