<div dir="ltr"><div dir="ltr">Hi Laurent,<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, 22 Mar 2021 at 21:07, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</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>
Thank you for the patch.<br>
<br>
On Wed, Mar 17, 2021 at 10:02:06AM +0000, Naushir Patuck wrote:<br>
> Move the opening of the CamHelper from ipa::configure() to ipa::init().<br>
> This allows the pipeline handler to get the sensor specific parameters<br>
> in in pipeline_handler::match() where the ipa is initialised.<br>
<br>
s/in in/in/<br>
<br>
> <br>
> Having the sensor parameters available earlier will allow selective<br>
> use of the embedded data node in a future change.<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.mojom | 7 +--<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 60 +++++++++----------<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 36 ++++++-----<br>
> 3 files changed, 46 insertions(+), 57 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom<br>
> index eb427697d349..a713c88eee19 100644<br>
> --- a/include/libcamera/ipa/raspberrypi.mojom<br>
> +++ b/include/libcamera/ipa/raspberrypi.mojom<br>
> @@ -15,10 +15,6 @@ enum BufferMask {<br>
> /* Size of the LS grid allocation. */<br>
> const uint32 MaxLsGridSize = 0x8000;<br>
> <br>
> -enum ConfigOutputParameters {<br>
> - ConfigSensorParams = 0x01,<br>
> -};<br>
> -<br>
> struct SensorConfig {<br>
> uint32 gainDelay;<br>
> uint32 exposureDelay;<br>
> @@ -41,7 +37,6 @@ struct ConfigInput {<br>
> <br>
> struct ConfigOutput {<br>
> uint32 params;<br>
<br>
Unless I'm mistaken, the params field isn't used anymore. Unless it's<br>
used in a subsequent patch in the series, you can drop it.<br></blockquote><div><br></div><div>This (and other bits) does get tidied up in the later commits for this series.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> - SensorConfig sensorConfig;<br>
> ControlList controls;<br>
> };<br>
> <br>
> @@ -51,7 +46,7 @@ struct StartControls {<br>
> };<br>
> <br>
> interface IPARPiInterface {<br>
> - init(IPASettings settings) => (int32 ret);<br>
> + init(IPASettings settings) => (int32 ret, SensorConfig sensorConfig);<br>
> start(StartControls controls) => (StartControls result);<br>
> stop();<br>
> <br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 7904225a29fa..8a9eb92b39ec 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -79,7 +79,7 @@ public:<br>
> munmap(lsTable_, ipa::RPi::MaxLsGridSize);<br>
> }<br>
> <br>
> - int init(const IPASettings &settings) override;<br>
> + int init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig) override;<br>
> void start(const ipa::RPi::StartControls &data,<br>
> ipa::RPi::StartControls *result) override;<br>
> void stop() override {}<br>
> @@ -164,9 +164,35 @@ private:<br>
> double maxFrameDuration_;<br>
> };<br>
> <br>
> -int IPARPi::init(const IPASettings &settings)<br>
> +int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig)<br>
> {<br>
> tuningFile_ = settings.configurationFile;<br>
> +<br>
> + /*<br>
> + * Load the "helper" for this sensor. This tells us all the device specific stuff<br>
> + * that the kernel driver doesn't. We only do this the first time; we don't need<br>
> + * to re-parse the metadata after a simple mode-switch for no reason.<br>
> + */<br>
> + helper_ = std::unique_ptr<RPiController::CamHelper>(RPiController::CamHelper::Create(settings.sensorModel));<br>
<br>
Not something that needs to be done as a prerequisite, but would it make<br>
sense to change the return type of RPiController::CamHelper::Create() to<br>
std::unique_ptr<RPiController::CamHelper> ?<br></blockquote><div><br></div><div>Yes, that seems reasonable. I will make the change separately to this series.</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 (!helper_) {<br>
> + LOG(IPARPI, Error) << "Could not create camera helper for "<br>
> + << settings.sensorModel;<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + /*<br>
> + * Pass out the sensor config to the pipeline handler in order<br>
> + * to setup the staggered writer class.<br>
> + */<br>
> + int gainDelay, exposureDelay, vblankDelay, sensorMetadata;<br>
> + helper_->GetDelays(exposureDelay, gainDelay, vblankDelay);<br>
> + sensorMetadata = helper_->SensorEmbeddedDataPresent();<br>
> +<br>
> + sensorConfig->gainDelay = gainDelay;<br>
> + sensorConfig->exposureDelay = exposureDelay;<br>
> + sensorConfig->vblankDelay = vblankDelay;<br>
> + sensorConfig->sensorMetadata = sensorMetadata;<br>
> +<br>
> return 0;<br>
> }<br>
> <br>
> @@ -319,36 +345,6 @@ int IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> /* Setup a metadata ControlList to output metadata. */<br>
> libcameraMetadata_ = ControlList(controls::controls);<br>
> <br>
> - /*<br>
> - * Load the "helper" for this sensor. This tells us all the device specific stuff<br>
> - * that the kernel driver doesn't. We only do this the first time; we don't need<br>
> - * to re-parse the metadata after a simple mode-switch for no reason.<br>
> - */<br>
> - std::string cameraName(sensorInfo.model);<br>
> - if (!helper_) {<br>
> - helper_ = std::unique_ptr<RPiController::CamHelper>(RPiController::CamHelper::Create(cameraName));<br>
> -<br>
> - if (!helper_) {<br>
> - LOG(IPARPI, Error) << "Could not create camera helper for "<br>
> - << cameraName;<br>
> - return -1;<br>
> - }<br>
> -<br>
> - /*<br>
> - * Pass out the sensor config to the pipeline handler in order<br>
> - * to setup the staggered writer class.<br>
> - */<br>
> - int gainDelay, exposureDelay, vblankDelay, sensorMetadata;<br>
> - helper_->GetDelays(exposureDelay, gainDelay, vblankDelay);<br>
> - sensorMetadata = helper_->SensorEmbeddedDataPresent();<br>
> -<br>
> - result->params |= ipa::RPi::ConfigSensorParams;<br>
> - result->sensorConfig.gainDelay = gainDelay;<br>
> - result->sensorConfig.exposureDelay = exposureDelay;<br>
> - result->sensorConfig.vblankDelay = vblankDelay;<br>
> - result->sensorConfig.sensorMetadata = sensorMetadata;<br>
> - }<br>
> -<br>
> /* Re-assemble camera mode using the sensor info. */<br>
> setMode(sensorInfo);<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 2c8ae31a28ad..ce1994186d66 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -146,7 +146,7 @@ public:<br>
> <br>
> void frameStarted(uint32_t sequence);<br>
> <br>
> - int loadIPA();<br>
> + int loadIPA(ipa::RPi::SensorConfig *sensorConfig);<br>
> int configureIPA(const CameraConfiguration *config);<br>
> <br>
> void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);<br>
> @@ -1030,11 +1030,24 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)<br>
> if (data->sensor_->init())<br>
> return false;<br>
> <br>
> - if (data->loadIPA()) {<br>
> + ipa::RPi::SensorConfig sensorConfig;<br>
> + if (data->loadIPA(&sensorConfig)) {<br>
> LOG(RPI, Error) << "Failed to load a suitable IPA library";<br>
> return false;<br>
> }<br>
> <br>
> + /*<br>
> + * Setup our delayed control writer with the sensor default<br>
> + * gain and exposure delays. Mark VBLANK for priority write.<br>
> + */<br>
> + std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {<br>
> + { V4L2_CID_ANALOGUE_GAIN, { sensorConfig.gainDelay, false } },<br>
> + { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false } },<br>
> + { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } }<br>
> + };<br>
> + data->delayedCtrls_ = std::make_unique<DelayedControls>(data->unicam_[Unicam::Image].dev(), params);<br>
> + data->sensorMetadata_ = sensorConfig.sensorMetadata;<br>
> +<br>
<br>
As this code deals with RPiCameraData only, could you move it to<br>
RPiCameraData::loadIPA() ? That way you won't have to prefix everything<br>
with data->, and you'll avoid passing sensorConfig to loadIPA().<br></blockquote><div><br></div><div>My original code did have this block in loadIPA(), but I ran into a small problem.</div><div>DelayedControls() needs to have the device (unicam_[Unicam::Image] in this case)</div><div>opened in order to validate the controls that are passed in via params. loadIPA()</div><div>gets called before the devices are opened, so I had to move this code block here.</div><div><br></div><div>I could have moved it into loadIPA() with unicam_[Unicam::Image] already opened,</div><div>but then I would have to split the opening of
unicam_[Unicam::Image] and </div><div>unicam_[Unicam::Embedded] into two separate locations, which I thought might be<br></div><div>undesirable. I am not completely against it, what are your thoughts?</div><div><br></div><div>Regards,</div><div>Naush</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>
With these small changes,<br>
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> /* Register the controls that the Raspberry Pi IPA can handle. */<br>
> data->controlInfo_ = RPi::Controls;<br>
> /* Initialize the camera properties. */<br>
> @@ -1214,7 +1227,7 @@ void RPiCameraData::frameStarted(uint32_t sequence)<br>
> delayedCtrls_->applyControls(sequence);<br>
> }<br>
> <br>
> -int RPiCameraData::loadIPA()<br>
> +int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)<br>
> {<br>
> ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe_, 1, 1);<br>
> <br>
> @@ -1230,7 +1243,7 @@ int RPiCameraData::loadIPA()<br>
> IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"),<br>
> sensor_->model());<br>
> <br>
> - return ipa_->init(settings);<br>
> + return ipa_->init(settings, sensorConfig);<br>
> }<br>
> <br>
> int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
> @@ -1293,21 +1306,6 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
> return -EPIPE;<br>
> }<br>
> <br>
> - if (result.params & ipa::RPi::ConfigSensorParams) {<br>
> - /*<br>
> - * Setup our delayed control writer with the sensor default<br>
> - * gain and exposure delays. Mark VBLANK for priority write.<br>
> - */<br>
> - std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {<br>
> - { V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },<br>
> - { V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },<br>
> - { V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }<br>
> - };<br>
> -<br>
> - delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), params);<br>
> - sensorMetadata_ = result.sensorConfig.sensorMetadata;<br>
> - }<br>
> -<br>
> if (!result.controls.empty()) {<br>
> ControlList &ctrls = result.controls;<br>
> unicam_[Unicam::Image].dev()->setControls(&ctrls);<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>