<div dir="auto"><div>Hi Laurent,<div dir="auto"><br></div><div dir="auto">Thank you for your review comments.</div><div dir="auto"><br></div><div dir="auto"><br></div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 11 Dec 2020, 9:00 pm Laurent Pinchart, <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Wed, Dec 09, 2020 at 09:53:39AM +0000, Naushir Patuck wrote:<br>
> There is no need to validate all the ISP and Unicam V4L2 controls on<br>
> every frame. Simply validate them once during the IPA configuration, and<br>
> fail if a required control is not available.<br>
> <br>
> Also add some whitespace for readability.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank" rel="noreferrer">naush@raspberrypi.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 131 ++++++++++++++--------------<br>
> 1 file changed, 64 insertions(+), 67 deletions(-)<br>
> <br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 60cfdc27..e5d2b41e 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -92,6 +92,8 @@ public:<br>
> <br>
> private:<br>
> void setMode(const CameraSensorInfo &sensorInfo);<br>
> + bool validateUnicamControls();<br>
> + bool validateIspControls();<br>
> void queueRequest(const ControlList &controls);<br>
> void returnEmbeddedBuffer(unsigned int bufferId);<br>
> void prepareISP(unsigned int bufferId);<br>
> @@ -238,6 +240,16 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> unicamCtrls_ = entityControls.at(0);<br>
> ispCtrls_ = entityControls.at(1);<br>
> <br>
> + if (!validateUnicamControls()) {<br>
> + LOG(IPARPI, Error) << "Unicam control validation failed.";<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + if (!validateIspControls()) {<br>
> + LOG(IPARPI, Error) << "ISP control validation failed.";<br>
> + return -EINVAL;<br>
> + }<br>
<br>
configure() is a void function. Rebase issue ?<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">Now that we are using my alternate mechanism, I will resubmit with the appropriate changes. I will also address the points below.</div><div dir="auto"><br></div><div dir="auto">Regards,</div><div dir="auto">Naush</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> +<br>
> /* Setup a metadata ControlList to output metadata. */<br>
> libcameraMetadata_ = ControlList(controls::controls);<br>
> <br>
> @@ -473,6 +485,51 @@ void IPARPi::reportMetadata()<br>
> }<br>
> }<br>
> <br>
> +bool IPARPi::validateUnicamControls()<br>
> +{<br>
> + const uint32_t ctrls[] = {<br>
<br>
static const ? Same below.<br>
<br>
> + V4L2_CID_ANALOGUE_GAIN,<br>
> + V4L2_CID_EXPOSURE<br>
> + };<br>
> +<br>
> + for (auto c : ctrls) {<br>
> + if (unicamCtrls_.find(c) == unicamCtrls_.end()) {<br>
> + LOG(IPARPI, Error) << "Unable to find Unicam control "<br>
> + << c;<br>
<br>
I'd use utils::hex(c) as otherwise the value is hard to read. Same<br>
below.<br>
<br>
> + return false;<br>
> + }<br>
> + }<br>
> +<br>
> + return true;<br>
> +}<br>
<br>
Isn't this sensor controls, not unicam controls ? While at it, we could<br>
rename unicamCtrls_ to sensorCtrls_.<br>
<br>
The rest looks good to me.<br>
<br>
> +<br>
> +bool IPARPi::validateIspControls()<br>
> +{<br>
> + const uint32_t ctrls[] = {<br>
> + V4L2_CID_RED_BALANCE,<br>
> + V4L2_CID_BLUE_BALANCE,<br>
> + V4L2_CID_DIGITAL_GAIN,<br>
> + V4L2_CID_USER_BCM2835_ISP_CC_MATRIX,<br>
> + V4L2_CID_USER_BCM2835_ISP_GAMMA,<br>
> + V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL,<br>
> + V4L2_CID_USER_BCM2835_ISP_GEQ,<br>
> + V4L2_CID_USER_BCM2835_ISP_DENOISE,<br>
> + V4L2_CID_USER_BCM2835_ISP_SHARPEN,<br>
> + V4L2_CID_USER_BCM2835_ISP_DPC,<br>
> + V4L2_CID_USER_BCM2835_ISP_LENS_SHADING<br>
> + };<br>
> +<br>
> + for (auto c : ctrls) {<br>
> + if (ispCtrls_.find(c) == ispCtrls_.end()) {<br>
> + LOG(IPARPI, Error) << "Unable to find ISP control "<br>
> + << c;<br>
> + return false;<br>
> + }<br>
> + }<br>
> +<br>
> + return true;<br>
> +}<br>
> +<br>
> /*<br>
> * Converting between enums (used in the libcamera API) and the names that<br>
> * we use to identify different modes. Unfortunately, the conversion tables<br>
> @@ -858,18 +915,6 @@ void IPARPi::processStats(unsigned int bufferId)<br>
> <br>
> void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)<br>
> {<br>
> - const auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);<br>
> - if (gainR == ispCtrls_.end()) {<br>
> - LOG(IPARPI, Error) << "Can't find red gain control";<br>
> - return;<br>
> - }<br>
> -<br>
> - const auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);<br>
> - if (gainB == ispCtrls_.end()) {<br>
> - LOG(IPARPI, Error) << "Can't find blue gain control";<br>
> - return;<br>
> - }<br>
> -<br>
> LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gain_r << " B: "<br>
> << awbStatus->gain_b;<br>
> <br>
> @@ -884,16 +929,6 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)<br>
> int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);<br>
> int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);<br>
> <br>
> - if (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {<br>
> - LOG(IPARPI, Error) << "Can't find analogue gain control";<br>
> - return;<br>
> - }<br>
> -<br>
> - if (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {<br>
> - LOG(IPARPI, Error) << "Can't find exposure control";<br>
> - return;<br>
> - }<br>
> -<br>
> LOG(IPARPI, Debug) << "Applying AGC Exposure: " << agcStatus->shutter_time<br>
> << " (Shutter lines: " << exposureLines << ") Gain: "<br>
> << agcStatus->analogue_gain << " (Gain Code: "<br>
> @@ -905,23 +940,14 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)<br>
> <br>
> void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)<br>
> {<br>
> - if (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {<br>
> - LOG(IPARPI, Error) << "Can't find digital gain control";<br>
> - return;<br>
> - }<br>
> -<br>
> ctrls.set(V4L2_CID_DIGITAL_GAIN,<br>
> static_cast<int32_t>(dgStatus->digital_gain * 1000));<br>
> }<br>
> <br>
> void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)<br>
> {<br>
> - if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {<br>
> - LOG(IPARPI, Error) << "Can't find CCM control";<br>
> - return;<br>
> - }<br>
> -<br>
> bcm2835_isp_custom_ccm ccm;<br>
> +<br>
> for (int i = 0; i < 9; i++) {<br>
> ccm.ccm.ccm[i / 3][i % 3].den = 1000;<br>
> ccm.ccm.ccm[i / 3][i % 3].num = 1000 * ccmStatus->matrix[i];<br>
> @@ -937,12 +963,8 @@ void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)<br>
> <br>
> void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)<br>
> {<br>
> - if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {<br>
> - LOG(IPARPI, Error) << "Can't find Gamma control";<br>
> - return;<br>
> - }<br>
> -<br>
> struct bcm2835_isp_gamma gamma;<br>
> +<br>
> gamma.enabled = 1;<br>
> for (int i = 0; i < CONTRAST_NUM_POINTS; i++) {<br>
> gamma.x[i] = contrastStatus->points[i].x;<br>
> @@ -956,12 +978,8 @@ void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList<br>
> <br>
> void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)<br>
> {<br>
> - if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {<br>
> - LOG(IPARPI, Error) << "Can't find black level control";<br>
> - return;<br>
> - }<br>
> -<br>
> bcm2835_isp_black_level blackLevel;<br>
> +<br>
> blackLevel.enabled = 1;<br>
> blackLevel.black_level_r = blackLevelStatus->black_level_r;<br>
> blackLevel.black_level_g = blackLevelStatus->black_level_g;<br>
> @@ -974,12 +992,8 @@ void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co<br>
> <br>
> void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)<br>
> {<br>
> - if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {<br>
> - LOG(IPARPI, Error) << "Can't find geq control";<br>
> - return;<br>
> - }<br>
> -<br>
> bcm2835_isp_geq geq;<br>
> +<br>
> geq.enabled = 1;<br>
> geq.offset = geqStatus->offset;<br>
> geq.slope.den = 1000;<br>
> @@ -992,12 +1006,8 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)<br>
> <br>
> void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)<br>
> {<br>
> - if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {<br>
> - LOG(IPARPI, Error) << "Can't find denoise control";<br>
> - return;<br>
> - }<br>
> -<br>
> bcm2835_isp_denoise denoise;<br>
> +<br>
> denoise.enabled = 1;<br>
> denoise.constant = denoiseStatus->noise_constant;<br>
> denoise.slope.num = 1000 * denoiseStatus->noise_slope;<br>
> @@ -1012,12 +1022,8 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct<br>
> <br>
> void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)<br>
> {<br>
> - if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {<br>
> - LOG(IPARPI, Error) << "Can't find sharpen control";<br>
> - return;<br>
> - }<br>
> -<br>
> bcm2835_isp_sharpen sharpen;<br>
> +<br>
> sharpen.enabled = 1;<br>
> sharpen.threshold.num = 1000 * sharpenStatus->threshold;<br>
> sharpen.threshold.den = 1000;<br>
> @@ -1033,12 +1039,8 @@ void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList<br>
> <br>
> void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)<br>
> {<br>
> - if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {<br>
> - LOG(IPARPI, Error) << "Can't find DPC control";<br>
> - return;<br>
> - }<br>
> -<br>
> bcm2835_isp_dpc dpc;<br>
> +<br>
> dpc.enabled = 1;<br>
> dpc.strength = dpcStatus->strength;<br>
> <br>
> @@ -1049,11 +1051,6 @@ void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)<br>
> <br>
> void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)<br>
> {<br>
> - if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {<br>
> - LOG(IPARPI, Error) << "Can't find LS control";<br>
> - return;<br>
> - }<br>
> -<br>
> /*<br>
> * Program lens shading tables into pipeline.<br>
> * Choose smallest cell size that won't exceed 63x48 cells.<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div></div>