<div dir="ltr"><div dir="ltr">Hi Jacopo,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 1 Feb 2021 at 10:53, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush<br>
<br>
On Sat, Jan 30, 2021 at 11:16:51AM +0000, Naushir Patuck wrote:<br>
> Rename the enum values to indicate pipeline handler -> IPA actions<br>
> (IPA_CONFIG_*) and IPA -> pipeline handler return results (IPA_RESULT_*).<br>
> Additionally, provide more descriptive names for these values.<br>
<br>
This part is ok, make naming more clear<br>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">><br>
> Fixup logic when handling IPA_RESULT_SENSOR_PARAMS where we must<br>
> overwrite the parameters if provided by IPA. In the current codebase,<br>
> this only happens once on startup, so there is no effective functional<br>
> difference. But this change allows the option for the IPA to request new<br>
> sensor parameters per-mode if required.<br>
<br>
This seems it's worth a separate patch, as it does not depends on<br>
renaming but it's rather a change in behaviour which it would probably<br>
better be separate.<br></blockquote><div><br></div><div>Ack. I can separate these out into 2 patches. Will post an update shortly.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
If I got this right you refer to 'parameters' in the commit message<br>
here to indicate the controls delay could be overwritten at each<br>
configureIPA() am I right ? Out of curiosity, how does the sensor mode<br>
change impact on the control delay ?<br></blockquote><div><br></div><div>In almost all cases it won't. However, I have sensors that will disable embedded data in certain modes, but again, this is very rare.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Thanks<br>
j<br>
<br>
<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> include/libcamera/ipa/raspberrypi.h | 10 +++---<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 18 +++++------<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 31 +++++++++----------<br>
> 3 files changed, 28 insertions(+), 31 deletions(-)<br>
><br>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h<br>
> index 5a9433825d5a..970b9e931188 100644<br>
> --- a/include/libcamera/ipa/raspberrypi.h<br>
> +++ b/include/libcamera/ipa/raspberrypi.h<br>
> @@ -20,11 +20,11 @@ namespace RPi {<br>
><br>
> enum ConfigParameters {<br>
> IPA_CONFIG_LS_TABLE = (1 << 0),<br>
> - IPA_CONFIG_STAGGERED_WRITE = (1 << 1),<br>
> - IPA_CONFIG_SENSOR = (1 << 2),<br>
> - IPA_CONFIG_DROP_FRAMES = (1 << 3),<br>
> - IPA_CONFIG_FAILED = (1 << 4),<br>
> - IPA_CONFIG_STARTUP = (1 << 5),<br>
> + IPA_CONFIG_STARTUP_CTRLS = (1 << 1),<br>
> + IPA_RESULT_CONFIG_FAILED = (1 << 2),<br>
> + IPA_RESULT_SENSOR_PARAMS = (1 << 3),<br>
> + IPA_RESULT_SENSOR_CTRLS = (1 << 4),<br>
> + IPA_RESULT_DROP_FRAMES = (1 << 5),<br>
> };<br>
><br>
> enum Operations {<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 681ab9211b7c..fea1ea3957bb 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -170,7 +170,7 @@ int IPARPi::start(const IPAOperationData &data, IPAOperationData *result)<br>
><br>
> ASSERT(result);<br>
> result->operation = 0;<br>
> - if (data.operation & RPi::IPA_CONFIG_STARTUP) {<br>
> + if (data.operation & RPi::IPA_CONFIG_STARTUP_CTRLS) {<br>
> /* We have been given some controls to action before start. */<br>
> queueRequest(data.controls[0]);<br>
> }<br>
> @@ -188,7 +188,7 @@ int IPARPi::start(const IPAOperationData &data, IPAOperationData *result)<br>
> ControlList ctrls(sensorCtrls_);<br>
> applyAGC(&agcStatus, ctrls);<br>
> result->controls.emplace_back(ctrls);<br>
> - result->operation |= RPi::IPA_CONFIG_SENSOR;<br>
> + result->operation |= RPi::IPA_RESULT_SENSOR_CTRLS;<br>
> }<br>
><br>
> /*<br>
> @@ -236,7 +236,7 @@ int IPARPi::start(const IPAOperationData &data, IPAOperationData *result)<br>
> }<br>
><br>
> result->data.push_back(dropFrame);<br>
> - result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;<br>
> + result->operation |= RPi::IPA_RESULT_DROP_FRAMES;<br>
><br>
> firstStart_ = false;<br>
><br>
> @@ -289,7 +289,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> {<br>
> if (entityControls.size() != 2) {<br>
> LOG(IPARPI, Error) << "No ISP or sensor controls found.";<br>
> - result->operation = RPi::IPA_CONFIG_FAILED;<br>
> + result->operation = RPi::IPA_RESULT_CONFIG_FAILED;<br>
> return;<br>
> }<br>
><br>
> @@ -300,13 +300,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
><br>
> if (!validateSensorControls()) {<br>
> LOG(IPARPI, Error) << "Sensor control validation failed.";<br>
> - result->operation = RPi::IPA_CONFIG_FAILED;<br>
> + result->operation = RPi::IPA_RESULT_CONFIG_FAILED;<br>
> return;<br>
> }<br>
><br>
> if (!validateIspControls()) {<br>
> LOG(IPARPI, Error) << "ISP control validation failed.";<br>
> - result->operation = RPi::IPA_CONFIG_FAILED;<br>
> + result->operation = RPi::IPA_RESULT_CONFIG_FAILED;<br>
> return;<br>
> }<br>
><br>
> @@ -325,7 +325,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> if (!helper_) {<br>
> LOG(IPARPI, Error) << "Could not create camera helper for "<br>
> << cameraName;<br>
> - result->operation = RPi::IPA_CONFIG_FAILED;<br>
> + result->operation = RPi::IPA_RESULT_CONFIG_FAILED;<br>
> return;<br>
> }<br>
><br>
> @@ -342,7 +342,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> result->data.push_back(exposureDelay); /* For VBLANK ctrl */<br>
> result->data.push_back(sensorMetadata);<br>
><br>
> - result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;<br>
> + result->operation |= RPi::IPA_RESULT_SENSOR_PARAMS;<br>
> }<br>
><br>
> /* Re-assemble camera mode using the sensor info. */<br>
> @@ -395,7 +395,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> applyAGC(&agcStatus, ctrls);<br>
><br>
> result->controls.emplace_back(ctrls);<br>
> - result->operation |= RPi::IPA_CONFIG_SENSOR;<br>
> + result->operation |= RPi::IPA_RESULT_SENSOR_CTRLS;<br>
> }<br>
><br>
> lastMode_ = mode_;<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 5ad12d99638f..c44cb437a596 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -753,7 +753,7 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont<br>
> IPAOperationData ipaData = {};<br>
> IPAOperationData result = {};<br>
> if (controls) {<br>
> - ipaData.operation = RPi::IPA_CONFIG_STARTUP;<br>
> + ipaData.operation = RPi::IPA_CONFIG_STARTUP_CTRLS;<br>
> ipaData.controls.emplace_back(*controls);<br>
> }<br>
> ret = data->ipa_->start(ipaData, &result);<br>
> @@ -765,12 +765,12 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont<br>
> }<br>
><br>
> /* Apply any gain/exposure settings that the IPA may have passed back. */<br>
> - if (result.operation & RPi::IPA_CONFIG_SENSOR) {<br>
> + if (result.operation & RPi::IPA_RESULT_SENSOR_CTRLS) {<br>
> ControlList &ctrls = result.controls[0];<br>
> data->unicam_[Unicam::Image].dev()->setControls(&ctrls);<br>
> }<br>
><br>
> - if (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) {<br>
> + if (result.operation & RPi::IPA_RESULT_DROP_FRAMES) {<br>
> /* Configure the number of dropped frames required on startup. */<br>
> data->dropFrameCount_ = result.data[0];<br>
> }<br>
> @@ -1213,31 +1213,28 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
> ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,<br>
> &result);<br>
><br>
> - if (result.operation & RPi::IPA_CONFIG_FAILED) {<br>
> + if (result.operation & RPi::IPA_RESULT_CONFIG_FAILED) {<br>
> LOG(RPI, Error) << "IPA configuration failed!";<br>
> return -EPIPE;<br>
> }<br>
><br>
> unsigned int resultIdx = 0;<br>
> - if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {<br>
> + if (result.operation & RPi::IPA_RESULT_SENSOR_PARAMS) {<br>
> /*<br>
> * Setup our delayed control writer with the sensor default<br>
> * gain and exposure delays.<br>
> */<br>
> - if (!delayedCtrls_) {<br>
> - std::unordered_map<uint32_t, unsigned int> delays = {<br>
> - { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },<br>
> - { V4L2_CID_EXPOSURE, result.data[resultIdx++] },<br>
> - { V4L2_CID_VBLANK, result.data[resultIdx++] }<br>
> - };<br>
> -<br>
> - delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays);<br>
> -<br>
> - sensorMetadata_ = result.data[resultIdx++];<br>
> - }<br>
> + std::unordered_map<uint32_t, unsigned int> delays = {<br>
> + { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },<br>
> + { V4L2_CID_EXPOSURE, result.data[resultIdx++] },<br>
> + { V4L2_CID_VBLANK, result.data[resultIdx++] }<br>
> + };<br>
> +<br>
> + delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays);<br>
> + sensorMetadata_ = result.data[resultIdx++];<br>
> }<br>
><br>
> - if (result.operation & RPi::IPA_CONFIG_SENSOR) {<br>
> + if (result.operation & RPi::IPA_RESULT_SENSOR_CTRLS) {<br>
> ControlList &ctrls = result.controls[0];<br>
> unicam_[Unicam::Image].dev()->setControls(&ctrls);<br>
> }<br>
> --<br>
> 2.25.1<br>
><br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>