[{"id":15185,"web_url":"https://patchwork.libcamera.org/comment/15185/","msgid":"","date":"2021-02-17T08:05:20","subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various\n\tfixups after IPAInterface changes","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\nThis change addresses some tidying up after the IPA interface changes.\nHowever, I do have a question, see below.\n\nOn Wed, 17 Feb 2021 at 08:02, Naushir Patuck wrote:\n\n> This commit addresses a few fixes and tidy-ups after the IPAInterface\n> rework:\n> - Rename ConfigStaggeredWrite -> ConfigSensorParams\n> - Rename setIsp -> setIspControls\n> - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete()\n> should only run when state != Stopped. This matches the behavior of\n> the original code. Without this, start/stop cycles may cause errors.\n> - In setIspControls(), only update the LS config (with the fd) when a LS\n> control is present.\n>\n> Signed-off-by: Naushir Patuck \n> Fixes: e201cb4f5450 (\"libcamera: IPAInterface: Replace C API with the new\n> C++-only API\")\n> ---\n> include/libcamera/ipa/raspberrypi.mojom | 4 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 4 +-\n> .../pipeline/raspberrypi/raspberrypi.cpp | 91 ++++++++++---------\n> 3 files changed, 50 insertions(+), 49 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> b/include/libcamera/ipa/raspberrypi.mojom\n> index bab19a946e18..8a256d022270 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -16,7 +16,7 @@ enum BufferMask {\n> const uint32 MaxLsGridSize = 0x8000;\n>\n> enum ConfigOutputParameters {\n> - ConfigStaggeredWrite = 0x01,\n> + ConfigSensorParams = 0x01,\n> };\n>\n> struct SensorConfig {\n> @@ -126,6 +126,6 @@ interface IPARPiEventInterface {\n> statsMetadataComplete(uint32 bufferId, ControlList controls);\n> runIsp(uint32 bufferId);\n> embeddedComplete(uint32 bufferId);\n> - setIsp(ControlList controls);\n> + setIspControls(ControlList controls);\n> setDelayedControls(ControlList controls);\n> };\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 81a3195c82e9..0bf88bcc5f72 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> helper_->GetDelays(exposureDelay, gainDelay);\n> sensorMetadata = helper_->SensorEmbeddedDataPresent();\n>\n> - result->params |= ipa::rpi::ConfigStaggeredWrite;\n> + result->params |= ipa::rpi::ConfigSensorParams;\n> result->sensorConfig.gainDelay = gainDelay;\n> result->sensorConfig.exposureDelay = exposureDelay;\n> result->sensorConfig.vblank = exposureDelay;\n> @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)\n> applyDPC(dpcStatus, ctrls);\n>\n> if (!ctrls.empty())\n> - setIsp.emit(ctrls);\n> + setIspControls.emit(ctrls);\n> }\n> }\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 15aa600ed581..d0ca015a01dc 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -152,7 +152,7 @@ public:\n> void statsMetadataComplete(uint32_t bufferId, const ControlList\n> &controls);\n> void runIsp(uint32_t bufferId);\n> void embeddedComplete(uint32_t bufferId);\n> - void setIsp(const ControlList &controls);\n> + void setIspControls(const ControlList &controls);\n> void setDelayedControls(const ControlList &controls);\n>\n> /* bufferComplete signal handlers. */\n> @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()\n> ipa_->statsMetadataComplete.connect(this,\n> &RPiCameraData::statsMetadataComplete);\n> ipa_->runIsp.connect(this, &RPiCameraData::runIsp);\n> ipa_->embeddedComplete.connect(this,\n> &RPiCameraData::embeddedComplete);\n> - ipa_->setIsp.connect(this, &RPiCameraData::setIsp);\n> + ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> ipa_->setDelayedControls.connect(this,\n> &RPiCameraData::setDelayedControls);\n>\n> IPASettings settings(ipa_->configurationFile(sensor_->model() +\n> \".json\"));\n> @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> return -EPIPE;\n> }\n>\n> - if (result.params & ipa::rpi::ConfigStaggeredWrite) {\n> + if (result.params & ipa::rpi::ConfigSensorParams) {\n> /*\n> * Setup our delayed control writer with the sensor default\n> * gain and exposure delays.\n> @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n>\n> void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const\n> ControlList &controls)\n> {\n> - if (state_ == State::Stopped)\n> - handleState();\n> + if (state_ != State::Stopped) {\n> + FrameBuffer *buffer =\n> isp_[Isp::Stats].getBuffers().at(bufferId);\n>\n> - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> + handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n>\n> - handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> + /* Fill the Request metadata buffer with what the IPA has\n> provided */\n> + Request *request = requestQueue_.front();\n> + request->metadata() = std::move(controls);\n>\n> - /* Fill the Request metadata buffer with what the IPA has provided\n> */\n> - Request *request = requestQueue_.front();\n> - request->metadata() = std::move(controls);\n> + /*\n> + * Also update the ScalerCrop in the metadata with what we\n> actually\n> + * used. But we must first rescale that from ISP (camera\n> mode) pixels\n> + * back into sensor native pixels.\n> + *\n> + * Sending this information on every frame may be helpful.\n> + */\n> + if (updateScalerCrop_) {\n> + updateScalerCrop_ = false;\n> + scalerCrop_ =\n> ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> +\n> sensorInfo_.outputSize);\n> +\n> scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> + }\n> + request->metadata().set(controls::ScalerCrop, scalerCrop_);\n>\n> - /*\n> - * Also update the ScalerCrop in the metadata with what we actually\n> - * used. But we must first rescale that from ISP (camera mode)\n> pixels\n> - * back into sensor native pixels.\n> - *\n> - * Sending this information on every frame may be helpful.\n> - */\n> - if (updateScalerCrop_) {\n> - updateScalerCrop_ = false;\n> - scalerCrop_ =\n> ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> - sensorInfo_.outputSize);\n> - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> + state_ = State::IpaComplete;\n> }\n> - request->metadata().set(controls::ScalerCrop, scalerCrop_);\n> -\n> - state_ = State::IpaComplete;\n>\n> handleState();\n> }\n>\n> void RPiCameraData::runIsp(uint32_t bufferId)\n> {\n> - if (state_ == State::Stopped)\n> - handleState();\n> + if (state_ != State::Stopped) {\n> + FrameBuffer *buffer =\n> unicam_[Unicam::Image].getBuffers().at(bufferId);\n>\n> - FrameBuffer *buffer =\n> unicam_[Unicam::Image].getBuffers().at(bufferId);\n> + LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" <<\n> bufferId\n> + << \", timestamp: \" <<\n> buffer->metadata().timestamp;\n>\n> - LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << bufferId\n> - << \", timestamp: \" << buffer->metadata().timestamp;\n> + isp_[Isp::Input].queueBuffer(buffer);\n> + ispOutputCount_ = 0;\n> + }\n>\n> - isp_[Isp::Input].queueBuffer(buffer);\n> - ispOutputCount_ = 0;\n> handleState();\n> }\n>\n> void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> {\n> - if (state_ == State::Stopped)\n> - handleState();\n> + if (state_ != State::Stopped) {\n> + FrameBuffer *buffer =\n> unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> + }\n>\n> - FrameBuffer *buffer =\n> unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> handleState();\n> }\n>\n> -void RPiCameraData::setIsp(const ControlList &controls)\n> +void RPiCameraData::setIspControls(const ControlList &controls)\n> {\n> ControlList ctrls = controls;\n>\n> - Span s =\n> - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n> - bcm2835_isp_lens_shading ls =\n> - *reinterpret_cast *>(s.data());\n> - ls.dmabuf = lsTable_.fd();\n> + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {\n> + Span s =\n> +\n> ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n> + bcm2835_isp_lens_shading ls =\n> + *reinterpret_cast *>(s.data());\n> + ls.dmabuf = lsTable_.fd();\n>\n> - ControlValue c(Span{ reinterpret_cast *>(&ls),\n> - sizeof(ls) });\n> - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);\n> + ControlValue c(Span{\n> reinterpret_cast(&ls),\n> + sizeof(ls) });\n> + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);\n>\n\nIs this ctrls.set() call required? Does the Span s address the memory in\nthe ControlList or is it a copy (in which case we do need the set)?\n\nThanks,\nNaush\n\n\n> + }\n>\n> isp_[Isp::Input].dev()->setControls(&ctrls);\n> handleState();\n> --\n> 2.25.1\n>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 87A60BD1F6\n\tfor ;\n\tWed, 17 Feb 2021 08:05:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 11DCF637F5;\n\tWed, 17 Feb 2021 09:05:39 +0100 (CET)","from mail-lf1-x134.google.com (mail-lf1-x134.google.com\n\t[IPv6:2a00:1450:4864:20::134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9CE8A602FA\n\tfor ;\n\tWed, 17 Feb 2021 09:05:37 +0100 (CET)","by mail-lf1-x134.google.com with SMTP id p21so19966424lfu.11\n\tfor ;\n\tWed, 17 Feb 2021 00:05:37 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"SrFNaR2y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to;\n\tbh=hHA1cqQaA00d2qISCk6Uva3X+S+Vv3TCp3Nn9xAEuWY=;\n\tb=SrFNaR2yQ95cAkOpdbW+9ny4aBCMf2alnaqHlmPkADel28IS1Mgpu7BDB3Er3X5p1j\n\te7NCrtrcXzz2KlB4+DrWTcVfMQe5jC13HYdoVmx8hwBJKsxSBAqQitmunwshsFTu5hJt\n\teGzHpZgBoMyC+8mVpQruCaMeHQccc9/Saoith3IUxSxcnCWXUwSpUL8sKepEozqGXtUi\n\tEYeO+L+mwylsOedWeeHEj1cijEFKn8e0bWcvpIEEkqZFwUIhv7MYQxWLtR+PLts+Crjo\n\tMNDADLyYxNSlDDBjIihjUsCL+Muzp9xKnddTftAhY27fm+wDSBNFgmBrrZhB4u6j9fwO\n\tI0Xg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to;\n\tbh=hHA1cqQaA00d2qISCk6Uva3X+S+Vv3TCp3Nn9xAEuWY=;\n\tb=SJCTeJkeQr3KZBjuqok6xbitvzgu3zJocNirQD0OgkxaQbVvzNvkDc8xQyYVmKROnh\n\tva43z5l2by/Jg5KYWXXipLRtoOjsRxCtkflJJHAWEI4iMCwWByaNYxmh1Bv7LcoaeqYO\n\tu6DwIjtS9awAZ5ZxOJ0mKDjizdLVb9te8GcziLGy8bj72kZmHCe+Ybyvi75cu0JWfT6Z\n\tAV4/hLxQfJA6gkcsOy9SJdS9AEmGIMY2+GV2+Df0AK5MGPCH4lJmkkXzVZP9x7rprNhg\n\tjcD34G8/MBfhOcrjtENJj5yI80m24npdxrnLiRSrayheSrzWyNROe0fa3nz8dZdS/iKH\n\t6/iw==","X-Gm-Message-State":"AOAM532CHwFnqgt15brFAmIA+YCWYDCo8IVl09Rp2gCwF21o+nQ2jxql\n\tj4EL1rxVkgBJmnYWXWCjKLHbj1Y/k3fRGHaousxmySjZl8ffag==","X-Google-Smtp-Source":"ABdhPJxpAbhxFuEXsQ602FcI88mFzXil93Glh9HpGp2XhCTQ1CFVQA9+obdebLmXByFFDW2rxJjPHUklJpJOVL/UF8A=","X-Received":"by 2002:a05:6512:114e:: with SMTP id\n\tm14mr13878846lfg.617.1613549136792; \n\tWed, 17 Feb 2021 00:05:36 -0800 (PST)","MIME-Version":"1.0","References":"<20210217080241.1492143-1-naush@raspberrypi.com>","In-Reply-To":"<20210217080241.1492143-1-naush@raspberrypi.com>","From":"Naushir Patuck ","Date":"Wed, 17 Feb 2021 08:05:20 +0000","Message-ID":"","To":"libcamera devel ","Subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various\n\tfixups after IPAInterface changes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"multipart/mixed;\n\tboundary=\"===============1286994539054836974==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15186,"web_url":"https://patchwork.libcamera.org/comment/15186/","msgid":"","date":"2021-02-17T08:28:37","subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various\n\tfixups after IPAInterface changes","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\nSorry, I did have one more question on the new IPA interface topic. In\nraspberrypi.mojom, the module namespace is defined as module \"ipa.rpi\".\n\nWould it make sense to rename this to \"ipa.RPi\" so that it is consistent\nwith the \"RPi\" namespace used elsewhere?\n\nThanks,\nNaush\n\n\nOn Wed, 17 Feb 2021 at 08:02, Naushir Patuck wrote:\n\n> This commit addresses a few fixes and tidy-ups after the IPAInterface\n> rework:\n> - Rename ConfigStaggeredWrite -> ConfigSensorParams\n> - Rename setIsp -> setIspControls\n> - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete()\n> should only run when state != Stopped. This matches the behavior of\n> the original code. Without this, start/stop cycles may cause errors.\n> - In setIspControls(), only update the LS config (with the fd) when a LS\n> control is present.\n>\n> Signed-off-by: Naushir Patuck \n> Fixes: e201cb4f5450 (\"libcamera: IPAInterface: Replace C API with the new\n> C++-only API\")\n> ---\n> include/libcamera/ipa/raspberrypi.mojom | 4 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 4 +-\n> .../pipeline/raspberrypi/raspberrypi.cpp | 91 ++++++++++---------\n> 3 files changed, 50 insertions(+), 49 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> b/include/libcamera/ipa/raspberrypi.mojom\n> index bab19a946e18..8a256d022270 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -16,7 +16,7 @@ enum BufferMask {\n> const uint32 MaxLsGridSize = 0x8000;\n>\n> enum ConfigOutputParameters {\n> - ConfigStaggeredWrite = 0x01,\n> + ConfigSensorParams = 0x01,\n> };\n>\n> struct SensorConfig {\n> @@ -126,6 +126,6 @@ interface IPARPiEventInterface {\n> statsMetadataComplete(uint32 bufferId, ControlList controls);\n> runIsp(uint32 bufferId);\n> embeddedComplete(uint32 bufferId);\n> - setIsp(ControlList controls);\n> + setIspControls(ControlList controls);\n> setDelayedControls(ControlList controls);\n> };\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 81a3195c82e9..0bf88bcc5f72 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> helper_->GetDelays(exposureDelay, gainDelay);\n> sensorMetadata = helper_->SensorEmbeddedDataPresent();\n>\n> - result->params |= ipa::rpi::ConfigStaggeredWrite;\n> + result->params |= ipa::rpi::ConfigSensorParams;\n> result->sensorConfig.gainDelay = gainDelay;\n> result->sensorConfig.exposureDelay = exposureDelay;\n> result->sensorConfig.vblank = exposureDelay;\n> @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)\n> applyDPC(dpcStatus, ctrls);\n>\n> if (!ctrls.empty())\n> - setIsp.emit(ctrls);\n> + setIspControls.emit(ctrls);\n> }\n> }\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 15aa600ed581..d0ca015a01dc 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -152,7 +152,7 @@ public:\n> void statsMetadataComplete(uint32_t bufferId, const ControlList\n> &controls);\n> void runIsp(uint32_t bufferId);\n> void embeddedComplete(uint32_t bufferId);\n> - void setIsp(const ControlList &controls);\n> + void setIspControls(const ControlList &controls);\n> void setDelayedControls(const ControlList &controls);\n>\n> /* bufferComplete signal handlers. */\n> @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()\n> ipa_->statsMetadataComplete.connect(this,\n> &RPiCameraData::statsMetadataComplete);\n> ipa_->runIsp.connect(this, &RPiCameraData::runIsp);\n> ipa_->embeddedComplete.connect(this,\n> &RPiCameraData::embeddedComplete);\n> - ipa_->setIsp.connect(this, &RPiCameraData::setIsp);\n> + ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> ipa_->setDelayedControls.connect(this,\n> &RPiCameraData::setDelayedControls);\n>\n> IPASettings settings(ipa_->configurationFile(sensor_->model() +\n> \".json\"));\n> @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> return -EPIPE;\n> }\n>\n> - if (result.params & ipa::rpi::ConfigStaggeredWrite) {\n> + if (result.params & ipa::rpi::ConfigSensorParams) {\n> /*\n> * Setup our delayed control writer with the sensor default\n> * gain and exposure delays.\n> @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n>\n> void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const\n> ControlList &controls)\n> {\n> - if (state_ == State::Stopped)\n> - handleState();\n> + if (state_ != State::Stopped) {\n> + FrameBuffer *buffer =\n> isp_[Isp::Stats].getBuffers().at(bufferId);\n>\n> - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> + handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n>\n> - handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> + /* Fill the Request metadata buffer with what the IPA has\n> provided */\n> + Request *request = requestQueue_.front();\n> + request->metadata() = std::move(controls);\n>\n> - /* Fill the Request metadata buffer with what the IPA has provided\n> */\n> - Request *request = requestQueue_.front();\n> - request->metadata() = std::move(controls);\n> + /*\n> + * Also update the ScalerCrop in the metadata with what we\n> actually\n> + * used. But we must first rescale that from ISP (camera\n> mode) pixels\n> + * back into sensor native pixels.\n> + *\n> + * Sending this information on every frame may be helpful.\n> + */\n> + if (updateScalerCrop_) {\n> + updateScalerCrop_ = false;\n> + scalerCrop_ =\n> ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> +\n> sensorInfo_.outputSize);\n> +\n> scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> + }\n> + request->metadata().set(controls::ScalerCrop, scalerCrop_);\n>\n> - /*\n> - * Also update the ScalerCrop in the metadata with what we actually\n> - * used. But we must first rescale that from ISP (camera mode)\n> pixels\n> - * back into sensor native pixels.\n> - *\n> - * Sending this information on every frame may be helpful.\n> - */\n> - if (updateScalerCrop_) {\n> - updateScalerCrop_ = false;\n> - scalerCrop_ =\n> ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> - sensorInfo_.outputSize);\n> - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> + state_ = State::IpaComplete;\n> }\n> - request->metadata().set(controls::ScalerCrop, scalerCrop_);\n> -\n> - state_ = State::IpaComplete;\n>\n> handleState();\n> }\n>\n> void RPiCameraData::runIsp(uint32_t bufferId)\n> {\n> - if (state_ == State::Stopped)\n> - handleState();\n> + if (state_ != State::Stopped) {\n> + FrameBuffer *buffer =\n> unicam_[Unicam::Image].getBuffers().at(bufferId);\n>\n> - FrameBuffer *buffer =\n> unicam_[Unicam::Image].getBuffers().at(bufferId);\n> + LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" <<\n> bufferId\n> + << \", timestamp: \" <<\n> buffer->metadata().timestamp;\n>\n> - LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << bufferId\n> - << \", timestamp: \" << buffer->metadata().timestamp;\n> + isp_[Isp::Input].queueBuffer(buffer);\n> + ispOutputCount_ = 0;\n> + }\n>\n> - isp_[Isp::Input].queueBuffer(buffer);\n> - ispOutputCount_ = 0;\n> handleState();\n> }\n>\n> void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> {\n> - if (state_ == State::Stopped)\n> - handleState();\n> + if (state_ != State::Stopped) {\n> + FrameBuffer *buffer =\n> unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> + }\n>\n> - FrameBuffer *buffer =\n> unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> handleState();\n> }\n>\n> -void RPiCameraData::setIsp(const ControlList &controls)\n> +void RPiCameraData::setIspControls(const ControlList &controls)\n> {\n> ControlList ctrls = controls;\n>\n> - Span s =\n> - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n> - bcm2835_isp_lens_shading ls =\n> - *reinterpret_cast *>(s.data());\n> - ls.dmabuf = lsTable_.fd();\n> + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {\n> + Span s =\n> +\n> ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n> + bcm2835_isp_lens_shading ls =\n> + *reinterpret_cast *>(s.data());\n> + ls.dmabuf = lsTable_.fd();\n>\n> - ControlValue c(Span{ reinterpret_cast *>(&ls),\n> - sizeof(ls) });\n> - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);\n> + ControlValue c(Span{\n> reinterpret_cast(&ls),\n> + sizeof(ls) });\n> + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);\n> + }\n>\n> isp_[Isp::Input].dev()->setControls(&ctrls);\n> handleState();\n> --\n> 2.25.1\n>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CA8E1BD1F1\n\tfor ;\n\tWed, 17 Feb 2021 08:28:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 59A5E637F2;\n\tWed, 17 Feb 2021 09:28:55 +0100 (CET)","from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 03D9E602FA\n\tfor ;\n\tWed, 17 Feb 2021 09:28:54 +0100 (CET)","by mail-lf1-x130.google.com with SMTP id m22so20117171lfg.5\n\tfor ;\n\tWed, 17 Feb 2021 00:28:53 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"AsVOb1Td\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to;\n\tbh=qhFqDniCuFO8NdbGoqqVFTqChRC95k7y8rnDj6/X9Kw=;\n\tb=AsVOb1TddsvMcdXRwh/cTGmVlE4scewka1rWFqtjRbjhByE5i9dTxWin1V6+sKvNdq\n\t7spexfrfrJywQzO9GB4O+J67gttFVvGutY82HUM2ByJxaqfaBb3X7snMbRedpM+zfFvz\n\tbB+VFDYkxbuwtyOf0qP9ABqKFd4DlUbIzXHoWNGhnaZK1R/oy6isl10mhQcP9QvoTjfc\n\tuhggoJnxFSV/PZBspTkRn2jNx39P4VhBDubIiGEXFr8mOLpqUXmYAlB1AOuyQ2ds0TTX\n\tVRi3OZ0pnN66jonUPobGfTyQz7/UGYD14cDKggJ9KyzNPh2PtzsYKu8zzsxbzxAWBHia\n\tXpRg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to;\n\tbh=qhFqDniCuFO8NdbGoqqVFTqChRC95k7y8rnDj6/X9Kw=;\n\tb=L00LqcRuQlNPmem6lvTnIEUTfjoN0ijpNv+Spf1jXMM6Jo4liH67D/Sdtab5U0uR4F\n\tuFmTSOR4ecgBBm9NE3RgE3sCkI1xX9DHb59gUZeGLTVN7wsjv+xFXSssSlWteowm2/mW\n\twZDC0ZOhy/GmTh+TT+CS7p4iuo0s0phnzpLx/i+FmtPVWyMts0vREACYTXRRsGcwrstN\n\tDC8VZ/b6ECWaTUCkicNPOHKamr41jhcdB6KjNkJ2yfGkNk/1KramLgeZdwzOegi1ndXV\n\too4nKlOKIkEYrAEpmRVLy/qGz3OldaT/m54kdivcd/aJztTXZWhu6r8ek52lfLTMJ1PV\n\t+sqw==","X-Gm-Message-State":"AOAM530qgf4f86/2HXvtbGvWWq7FEcFm0rCfQcmxNldKCNvG2a+n371y\n\tP3JUHRUJkNx2zBanhf2GshZR/OOJ1ShpwXL/kDfNS1yilAkvB5NB","X-Google-Smtp-Source":"ABdhPJwVb8U5rTY3aU28HcpvXLFb6+82J+oRrb/JOm/9J3nwjUBVpDp9UVkb4wchkZGKsBNnwV97YDe89DCSLTif0Yg=","X-Received":"by 2002:a19:48c2:: with SMTP id\n\tv185mr13921657lfa.375.1613550533092; \n\tWed, 17 Feb 2021 00:28:53 -0800 (PST)","MIME-Version":"1.0","References":"<20210217080241.1492143-1-naush@raspberrypi.com>","In-Reply-To":"<20210217080241.1492143-1-naush@raspberrypi.com>","From":"Naushir Patuck ","Date":"Wed, 17 Feb 2021 08:28:37 +0000","Message-ID":"","To":"libcamera devel ","Subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various\n\tfixups after IPAInterface changes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"multipart/mixed;\n\tboundary=\"===============8948535842291653752==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15187,"web_url":"https://patchwork.libcamera.org/comment/15187/","msgid":"<20210217083851.GA1786@pyrite.rasen.tech>","date":"2021-02-17T08:38:51","subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various\n\tfixups after IPAInterface changes","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Wed, Feb 17, 2021 at 08:02:41AM +0000, Naushir Patuck wrote:\n> This commit addresses a few fixes and tidy-ups after the IPAInterface\n> rework:\n> - Rename ConfigStaggeredWrite -> ConfigSensorParams\n> - Rename setIsp -> setIspControls\n> - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete()\n> should only run when state != Stopped. This matches the behavior of\n> the original code. Without this, start/stop cycles may cause errors.\n> - In setIspControls(), only update the LS config (with the fd) when a LS\n> control is present.\n> \n> Signed-off-by: Naushir Patuck \n> Fixes: e201cb4f5450 (\"libcamera: IPAInterface: Replace C API with the new C++-only API\")\n> ---\n> include/libcamera/ipa/raspberrypi.mojom | 4 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 4 +-\n> .../pipeline/raspberrypi/raspberrypi.cpp | 91 ++++++++++---------\n> 3 files changed, 50 insertions(+), 49 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> index bab19a946e18..8a256d022270 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -16,7 +16,7 @@ enum BufferMask {\n> const uint32 MaxLsGridSize = 0x8000;\n> \n> enum ConfigOutputParameters {\n> -\tConfigStaggeredWrite = 0x01,\n> +\tConfigSensorParams = 0x01,\n> };\n> \n> struct SensorConfig {\n> @@ -126,6 +126,6 @@ interface IPARPiEventInterface {\n> \tstatsMetadataComplete(uint32 bufferId, ControlList controls);\n> \trunIsp(uint32 bufferId);\n> \tembeddedComplete(uint32 bufferId);\n> -\tsetIsp(ControlList controls);\n> +\tsetIspControls(ControlList controls);\n> \tsetDelayedControls(ControlList controls);\n> };\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 81a3195c82e9..0bf88bcc5f72 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\thelper_->GetDelays(exposureDelay, gainDelay);\n> \t\tsensorMetadata = helper_->SensorEmbeddedDataPresent();\n> \n> -\t\tresult->params |= ipa::rpi::ConfigStaggeredWrite;\n> +\t\tresult->params |= ipa::rpi::ConfigSensorParams;\n> \t\tresult->sensorConfig.gainDelay = gainDelay;\n> \t\tresult->sensorConfig.exposureDelay = exposureDelay;\n> \t\tresult->sensorConfig.vblank = exposureDelay;\n> @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)\n> \t\t\tapplyDPC(dpcStatus, ctrls);\n> \n> \t\tif (!ctrls.empty())\n> -\t\t\tsetIsp.emit(ctrls);\n> +\t\t\tsetIspControls.emit(ctrls);\n> \t}\n> }\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 15aa600ed581..d0ca015a01dc 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -152,7 +152,7 @@ public:\n> \tvoid statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> \tvoid runIsp(uint32_t bufferId);\n> \tvoid embeddedComplete(uint32_t bufferId);\n> -\tvoid setIsp(const ControlList &controls);\n> +\tvoid setIspControls(const ControlList &controls);\n> \tvoid setDelayedControls(const ControlList &controls);\n> \n> \t/* bufferComplete signal handlers. */\n> @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()\n> \tipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);\n> \tipa_->runIsp.connect(this, &RPiCameraData::runIsp);\n> \tipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);\n> -\tipa_->setIsp.connect(this, &RPiCameraData::setIsp);\n> +\tipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> \tipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);\n> \n> \tIPASettings settings(ipa_->configurationFile(sensor_->model() + \".json\"));\n> @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t\treturn -EPIPE;\n> \t}\n> \n> -\tif (result.params & ipa::rpi::ConfigStaggeredWrite) {\n> +\tif (result.params & ipa::rpi::ConfigSensorParams) {\n> \t\t/*\n> \t\t * Setup our delayed control writer with the sensor default\n> \t\t * gain and exposure delays.\n> @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \n> void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)\n> {\n> -\tif (state_ == State::Stopped)\n> -\t\thandleState();\n\nFor this (and the ones below) I wonder if it would be cleaner to return\nhere instead. That was actually my intention, I guess it slipped through\nwhen I did the translation.\n\n> +\tif (state_ != State::Stopped) {\n> +\t\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> \n> -\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> +\t\thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> \n> -\thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> +\t\t/* Fill the Request metadata buffer with what the IPA has provided */\n> +\t\tRequest *request = requestQueue_.front();\n> +\t\trequest->metadata() = std::move(controls);\n> \n> -\t/* Fill the Request metadata buffer with what the IPA has provided */\n> -\tRequest *request = requestQueue_.front();\n> -\trequest->metadata() = std::move(controls);\n> +\t\t/*\n> +\t\t* Also update the ScalerCrop in the metadata with what we actually\n> +\t\t* used. But we must first rescale that from ISP (camera mode) pixels\n> +\t\t* back into sensor native pixels.\n> +\t\t*\n> +\t\t* Sending this information on every frame may be helpful.\n> +\t\t*/\n> +\t\tif (updateScalerCrop_) {\n> +\t\t\tupdateScalerCrop_ = false;\n> +\t\t\tscalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> +\t\t\t\t\t\t\tsensorInfo_.outputSize);\n> +\t\t\tscalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> +\t\t}\n> +\t\trequest->metadata().set(controls::ScalerCrop, scalerCrop_);\n> \n> -\t/*\n> -\t * Also update the ScalerCrop in the metadata with what we actually\n> -\t * used. But we must first rescale that from ISP (camera mode) pixels\n> -\t * back into sensor native pixels.\n> -\t *\n> -\t * Sending this information on every frame may be helpful.\n> -\t */\n> -\tif (updateScalerCrop_) {\n> -\t\tupdateScalerCrop_ = false;\n> -\t\tscalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> -\t\t\t\t\t\tsensorInfo_.outputSize);\n> -\t\tscalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> +\t\tstate_ = State::IpaComplete;\n> \t}\n> -\trequest->metadata().set(controls::ScalerCrop, scalerCrop_);\n> -\n> -\tstate_ = State::IpaComplete;\n> \n> \thandleState();\n> }\n> \n> void RPiCameraData::runIsp(uint32_t bufferId)\n> {\n> -\tif (state_ == State::Stopped)\n> -\t\thandleState();\n> +\tif (state_ != State::Stopped) {\n> +\t\tFrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> \n> -\tFrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> +\t\tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << bufferId\n> +\t\t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> \n> -\tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << bufferId\n> -\t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> +\t\tisp_[Isp::Input].queueBuffer(buffer);\n> +\t\tispOutputCount_ = 0;\n> +\t}\n> \n> -\tisp_[Isp::Input].queueBuffer(buffer);\n> -\tispOutputCount_ = 0;\n> \thandleState();\n> }\n> \n> void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> {\n> -\tif (state_ == State::Stopped)\n> -\t\thandleState();\n> +\tif (state_ != State::Stopped) {\n> +\t\tFrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> +\t\thandleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> +\t}\n> \n> -\tFrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> -\thandleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> \thandleState();\n> }\n> \n> -void RPiCameraData::setIsp(const ControlList &controls)\n> +void RPiCameraData::setIspControls(const ControlList &controls)\n> {\n> \tControlList ctrls = controls;\n> \n> -\tSpan s =\n> -\t\tctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n> -\tbcm2835_isp_lens_shading ls =\n> -\t\t*reinterpret_cast(s.data());\n> -\tls.dmabuf = lsTable_.fd();\n> +\tif (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {\n\nGood catch :)\n\n> +\t\tSpan s =\n> +\t\t\tctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n> +\t\tbcm2835_isp_lens_shading ls =\n> +\t\t\t*reinterpret_cast(s.data());\n\nI think you are right that we don't need to set() the control again\nbelow. In that case ls would have to be a pointer.\n\nWith or without the suggested changes,\n\nReviewed-by: Paul Elder \n\n> +\t\tls.dmabuf = lsTable_.fd();\n> \n> -\tControlValue c(Span{ reinterpret_cast(&ls),\n> -\t\t\t\t\t sizeof(ls) });\n> -\tctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);\n> +\t\tControlValue c(Span{ reinterpret_cast(&ls),\n> +\t\t\t\t\t\t sizeof(ls) });\n> +\t\tctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);\n> +\t}\n> \n> \tisp_[Isp::Input].dev()->setControls(&ctrls);\n> \thandleState();\n> -- \n> 2.25.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 05A67BD1F6\n\tfor ;\n\tWed, 17 Feb 2021 08:39:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 76916637F6;\n\tWed, 17 Feb 2021 09:39:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 560B6602FA\n\tfor ;\n\tWed, 17 Feb 2021 09:38:59 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9CBB68C4;\n\tWed, 17 Feb 2021 09:38:57 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"B6MG7P6D\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613551138;\n\tbh=FlsfCHCcZSRDEi8ED2WfKmazoqmcysSh+BcW4T90fTc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=B6MG7P6DFKBMyfB7gAldM1/uVR0KkLusE7toaFLkyabiJapEzCccet4n5c4DFJTNG\n\tp0qIdHZzR24beEvlJiP1QyEU1zgN8HwXoxmmy1NI/jOGtaPhBnwcUyG1UiC9CUgmF9\n\tg1dpag/DV6BaAc9D+vB7ZVqyThzIZo0XyD/agOvo=","Date":"Wed, 17 Feb 2021 17:38:51 +0900","From":"paul.elder@ideasonboard.com","To":"Naushir Patuck ","Message-ID":"<20210217083851.GA1786@pyrite.rasen.tech>","References":"<20210217080241.1492143-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210217080241.1492143-1-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various\n\tfixups after IPAInterface changes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15191,"web_url":"https://patchwork.libcamera.org/comment/15191/","msgid":"<20210217085650.GE1786@pyrite.rasen.tech>","date":"2021-02-17T08:56:50","subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various\n\tfixups after IPAInterface changes","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, Feb 17, 2021 at 08:28:37AM +0000, Naushir Patuck wrote:\n> Hi all,\n> \n> Sorry, I did have one more question on the new IPA interface topic.  In\n> raspberrypi.mojom, the module namespace is defined as module \"ipa.rpi\".\n> \n> Would it make sense to rename this to \"ipa.RPi\" so that it is consistent with\n> the \"RPi\" namespace used elsewhere?\n\nThere's nothing technical preventing it. I just chose that\ncapitalization arbitrarily and nobody objected.\n\n\nPaul\n\n> \n> On Wed, 17 Feb 2021 at 08:02, Naushir Patuck wrote:\n> \n> This commit addresses a few fixes and tidy-ups after the IPAInterface\n> rework:\n> - Rename ConfigStaggeredWrite -> ConfigSensorParams\n> - Rename setIsp -> setIspControls\n> - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete()\n> should only run when state != Stopped. This matches the behavior of\n> the original code. Without this, start/stop cycles may cause errors.\n> - In setIspControls(), only update the LS config (with the fd) when a LS\n> control is present.\n> \n> Signed-off-by: Naushir Patuck \n> Fixes: e201cb4f5450 (\"libcamera: IPAInterface: Replace C API with the new\n> C++-only API\")\n> ---\n>  include/libcamera/ipa/raspberrypi.mojom       |  4 +-\n>  src/ipa/raspberrypi/raspberrypi.cpp           |  4 +-\n>  .../pipeline/raspberrypi/raspberrypi.cpp      | 91 ++++++++++---------\n>  3 files changed, 50 insertions(+), 49 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/\n> ipa/raspberrypi.mojom\n> index bab19a946e18..8a256d022270 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -16,7 +16,7 @@ enum BufferMask {\n>  const uint32 MaxLsGridSize = 0x8000;\n> \n>  enum ConfigOutputParameters {\n> -       ConfigStaggeredWrite = 0x01,\n> +       ConfigSensorParams = 0x01,\n>  };\n> \n>  struct SensorConfig {\n> @@ -126,6 +126,6 @@ interface IPARPiEventInterface {\n>         statsMetadataComplete(uint32 bufferId, ControlList controls);\n>         runIsp(uint32 bufferId);\n>         embeddedComplete(uint32 bufferId);\n> -       setIsp(ControlList controls);\n> +       setIspControls(ControlList controls);\n>         setDelayedControls(ControlList controls);\n>  };\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/\n> raspberrypi.cpp\n> index 81a3195c82e9..0bf88bcc5f72 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo &\n> sensorInfo,\n>                 helper_->GetDelays(exposureDelay, gainDelay);\n>                 sensorMetadata = helper_->SensorEmbeddedDataPresent();\n> \n> -               result->params |= ipa::rpi::ConfigStaggeredWrite;\n> +               result->params |= ipa::rpi::ConfigSensorParams;\n>                 result->sensorConfig.gainDelay = gainDelay;\n>                 result->sensorConfig.exposureDelay = exposureDelay;\n>                 result->sensorConfig.vblank = exposureDelay;\n> @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)\n>                         applyDPC(dpcStatus, ctrls);\n> \n>                 if (!ctrls.empty())\n> -                       setIsp.emit(ctrls);\n> +                       setIspControls.emit(ctrls);\n>         }\n>  }\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/\n> libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 15aa600ed581..d0ca015a01dc 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -152,7 +152,7 @@ public:\n>         void statsMetadataComplete(uint32_t bufferId, const ControlList &\n> controls);\n>         void runIsp(uint32_t bufferId);\n>         void embeddedComplete(uint32_t bufferId);\n> -       void setIsp(const ControlList &controls);\n> +       void setIspControls(const ControlList &controls);\n>         void setDelayedControls(const ControlList &controls);\n> \n>         /* bufferComplete signal handlers. */\n> @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()\n>         ipa_->statsMetadataComplete.connect(this, &\n> RPiCameraData::statsMetadataComplete);\n>         ipa_->runIsp.connect(this, &RPiCameraData::runIsp);\n>         ipa_->embeddedComplete.connect(this, &\n> RPiCameraData::embeddedComplete);\n> -       ipa_->setIsp.connect(this, &RPiCameraData::setIsp);\n> +       ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n>         ipa_->setDelayedControls.connect(this, &\n> RPiCameraData::setDelayedControls);\n> \n>         IPASettings settings(ipa_->configurationFile(sensor_->model() +\n> \".json\"));\n> @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n>                 return -EPIPE;\n>         }\n> \n> -       if (result.params & ipa::rpi::ConfigStaggeredWrite) {\n> +       if (result.params & ipa::rpi::ConfigSensorParams) {\n>                 /*\n>                  * Setup our delayed control writer with the sensor default\n>                  * gain and exposure delays.\n> @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> \n>  void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const\n> ControlList &controls)\n>  {\n> -       if (state_ == State::Stopped)\n> -               handleState();\n> +       if (state_ != State::Stopped) {\n> +               FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at\n> (bufferId);\n> \n> -       FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> +               handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> \n> -       handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> +               /* Fill the Request metadata buffer with what the IPA has\n> provided */\n> +               Request *request = requestQueue_.front();\n> +               request->metadata() = std::move(controls);\n> \n> -       /* Fill the Request metadata buffer with what the IPA has provided\n> */\n> -       Request *request = requestQueue_.front();\n> -       request->metadata() = std::move(controls);\n> +               /*\n> +               * Also update the ScalerCrop in the metadata with what we\n> actually\n> +               * used. But we must first rescale that from ISP (camera\n> mode) pixels\n> +               * back into sensor native pixels.\n> +               *\n> +               * Sending this information on every frame may be helpful.\n> +               */\n> +               if (updateScalerCrop_) {\n> +                       updateScalerCrop_ = false;\n> +                       scalerCrop_ = ispCrop_.scaledBy\n> (sensorInfo_.analogCrop.size(),\n> +                                                     \n>  sensorInfo_.outputSize);\n> +                       scalerCrop_.translateBy\n> (sensorInfo_.analogCrop.topLeft());\n> +               }\n> +               request->metadata().set(controls::ScalerCrop, scalerCrop_);\n> \n> -       /*\n> -        * Also update the ScalerCrop in the metadata with what we actually\n> -        * used. But we must first rescale that from ISP (camera mode)\n> pixels\n> -        * back into sensor native pixels.\n> -        *\n> -        * Sending this information on every frame may be helpful.\n> -        */\n> -       if (updateScalerCrop_) {\n> -               updateScalerCrop_ = false;\n> -               scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size\n> (),\n> -                                               sensorInfo_.outputSize);\n> -               scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> +               state_ = State::IpaComplete;\n>         }\n> -       request->metadata().set(controls::ScalerCrop, scalerCrop_);\n> -\n> -       state_ = State::IpaComplete;\n> \n>         handleState();\n>  }\n> \n>  void RPiCameraData::runIsp(uint32_t bufferId)\n>  {\n> -       if (state_ == State::Stopped)\n> -               handleState();\n> +       if (state_ != State::Stopped) {\n> +               FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers\n> ().at(bufferId);\n> \n> -       FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at\n> (bufferId);\n> +               LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" <<\n> bufferId\n> +                               << \", timestamp: \" << buffer->metadata\n> ().timestamp;\n> \n> -       LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << bufferId\n> -                       << \", timestamp: \" << buffer->metadata().timestamp;\n> +               isp_[Isp::Input].queueBuffer(buffer);\n> +               ispOutputCount_ = 0;\n> +       }\n> \n> -       isp_[Isp::Input].queueBuffer(buffer);\n> -       ispOutputCount_ = 0;\n>         handleState();\n>  }\n> \n>  void RPiCameraData::embeddedComplete(uint32_t bufferId)\n>  {\n> -       if (state_ == State::Stopped)\n> -               handleState();\n> +       if (state_ != State::Stopped) {\n> +               FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers\n> ().at(bufferId);\n> +               handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> +       }\n> \n> -       FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at\n> (bufferId);\n> -       handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n>         handleState();\n>  }\n> \n> -void RPiCameraData::setIsp(const ControlList &controls)\n> +void RPiCameraData::setIspControls(const ControlList &controls)\n>  {\n>         ControlList ctrls = controls;\n> \n> -       Span s =\n> -               ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n> -       bcm2835_isp_lens_shading ls =\n> -               *reinterpret_cast(s.data\n> ());\n> -       ls.dmabuf = lsTable_.fd();\n> +       if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {\n> +               Span s =\n> +                       ctrls.get\n> (V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n> +               bcm2835_isp_lens_shading ls =\n> +                       *reinterpret_cast\n> (s.data());\n> +               ls.dmabuf = lsTable_.fd();\n> \n> -       ControlValue c(Span{ reinterpret_cast(&\n> ls),\n> -                                           sizeof(ls) });\n> -       ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);\n> +               ControlValue c(Span{ reinterpret_cast\n> (&ls),\n> +                                                   sizeof(ls) });\n> +               ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);\n> +       }\n> \n>         isp_[Isp::Input].dev()->setControls(&ctrls);\n>         handleState();\n> --\n> 2.25.1\n> \n> \n\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 384C6BD1F1\n\tfor ;\n\tWed, 17 Feb 2021 08:56:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9713C637FE;\n\tWed, 17 Feb 2021 09:56:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7C9F7637F2\n\tfor ;\n\tWed, 17 Feb 2021 09:56:57 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E62E58C4;\n\tWed, 17 Feb 2021 09:56:55 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DKdxIF4a\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613552217;\n\tbh=evhMZEF+tRUstghusfgi62BXZob6BvWlJWZJ4aDQ0RU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DKdxIF4aM/U2AHBEd8ULJcxp7BzbmMcXuq9Sks2yly8W3NtNDCXNWFD3W/L8U9d2m\n\tcVWWDvijLvElFDLqE9vOhBjV4j0djtvFAteAdSoqTaNQNT1IyR15cL8pgDCR0A56KP\n\tSoR8U4mKri7hCY11WUvNZEotEVHPG/DbIMLGQ1e4=","Date":"Wed, 17 Feb 2021 17:56:50 +0900","From":"paul.elder@ideasonboard.com","To":"Naushir Patuck ","Message-ID":"<20210217085650.GE1786@pyrite.rasen.tech>","References":"<20210217080241.1492143-1-naush@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various\n\tfixups after IPAInterface changes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15192,"web_url":"https://patchwork.libcamera.org/comment/15192/","msgid":"","date":"2021-02-17T09:03:11","subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various\n\tfixups after IPAInterface changes","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Paul,\n\nThank you for your review feedback.\n\nOn Wed, 17 Feb 2021 at 08:38, wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Wed, Feb 17, 2021 at 08:02:41AM +0000, Naushir Patuck wrote:\n> > This commit addresses a few fixes and tidy-ups after the IPAInterface\n> > rework:\n> > - Rename ConfigStaggeredWrite -> ConfigSensorParams\n> > - Rename setIsp -> setIspControls\n> > - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete()\n> > should only run when state != Stopped. This matches the behavior of\n> > the original code. Without this, start/stop cycles may cause errors.\n> > - In setIspControls(), only update the LS config (with the fd) when a LS\n> > control is present.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Fixes: e201cb4f5450 (\"libcamera: IPAInterface: Replace C API with the\n> new C++-only API\")\n> > ---\n> > include/libcamera/ipa/raspberrypi.mojom | 4 +-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 4 +-\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 91 ++++++++++---------\n> > 3 files changed, 50 insertions(+), 49 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> b/include/libcamera/ipa/raspberrypi.mojom\n> > index bab19a946e18..8a256d022270 100644\n> > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > @@ -16,7 +16,7 @@ enum BufferMask {\n> > const uint32 MaxLsGridSize = 0x8000;\n> >\n> > enum ConfigOutputParameters {\n> > - ConfigStaggeredWrite = 0x01,\n> > + ConfigSensorParams = 0x01,\n> > };\n> >\n> > struct SensorConfig {\n> > @@ -126,6 +126,6 @@ interface IPARPiEventInterface {\n> > statsMetadataComplete(uint32 bufferId, ControlList controls);\n> > runIsp(uint32 bufferId);\n> > embeddedComplete(uint32 bufferId);\n> > - setIsp(ControlList controls);\n> > + setIspControls(ControlList controls);\n> > setDelayedControls(ControlList controls);\n> > };\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 81a3195c82e9..0bf88bcc5f72 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > helper_->GetDelays(exposureDelay, gainDelay);\n> > sensorMetadata = helper_->SensorEmbeddedDataPresent();\n> >\n> > - result->params |= ipa::rpi::ConfigStaggeredWrite;\n> > + result->params |= ipa::rpi::ConfigSensorParams;\n> > result->sensorConfig.gainDelay = gainDelay;\n> > result->sensorConfig.exposureDelay = exposureDelay;\n> > result->sensorConfig.vblank = exposureDelay;\n> > @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)\n> > applyDPC(dpcStatus, ctrls);\n> >\n> > if (!ctrls.empty())\n> > - setIsp.emit(ctrls);\n> > + setIspControls.emit(ctrls);\n> > }\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 15aa600ed581..d0ca015a01dc 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -152,7 +152,7 @@ public:\n> > void statsMetadataComplete(uint32_t bufferId, const ControlList\n> &controls);\n> > void runIsp(uint32_t bufferId);\n> > void embeddedComplete(uint32_t bufferId);\n> > - void setIsp(const ControlList &controls);\n> > + void setIspControls(const ControlList &controls);\n> > void setDelayedControls(const ControlList &controls);\n> >\n> > /* bufferComplete signal handlers. */\n> > @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()\n> > ipa_->statsMetadataComplete.connect(this,\n> &RPiCameraData::statsMetadataComplete);\n> > ipa_->runIsp.connect(this, &RPiCameraData::runIsp);\n> > ipa_->embeddedComplete.connect(this,\n> &RPiCameraData::embeddedComplete);\n> > - ipa_->setIsp.connect(this, &RPiCameraData::setIsp);\n> > + ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> > ipa_->setDelayedControls.connect(this,\n> &RPiCameraData::setDelayedControls);\n> >\n> > IPASettings settings(ipa_->configurationFile(sensor_->model() +\n> \".json\"));\n> > @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> > return -EPIPE;\n> > }\n> >\n> > - if (result.params & ipa::rpi::ConfigStaggeredWrite) {\n> > + if (result.params & ipa::rpi::ConfigSensorParams) {\n> > /*\n> > * Setup our delayed control writer with the sensor default\n> > * gain and exposure delays.\n> > @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> >\n> > void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const\n> ControlList &controls)\n> > {\n> > - if (state_ == State::Stopped)\n> > - handleState();\n>\n> For this (and the ones below) I wonder if it would be cleaner to return\n> here instead. That was actually my intention, I guess it slipped through\n> when I did the translation.\n>\n\nI'm happy to do either. What do others feel is cleaner?\n\n\n>\n> > + if (state_ != State::Stopped) {\n> > + FrameBuffer *buffer =\n> isp_[Isp::Stats].getBuffers().at(bufferId);\n> >\n> > - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> > + handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> >\n> > - handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > + /* Fill the Request metadata buffer with what the IPA has\n> provided */\n> > + Request *request = requestQueue_.front();\n> > + request->metadata() = std::move(controls);\n> >\n> > - /* Fill the Request metadata buffer with what the IPA has provided\n> */\n> > - Request *request = requestQueue_.front();\n> > - request->metadata() = std::move(controls);\n> > + /*\n> > + * Also update the ScalerCrop in the metadata with what we\n> actually\n> > + * used. But we must first rescale that from ISP (camera\n> mode) pixels\n> > + * back into sensor native pixels.\n> > + *\n> > + * Sending this information on every frame may be helpful.\n> > + */\n> > + if (updateScalerCrop_) {\n> > + updateScalerCrop_ = false;\n> > + scalerCrop_ =\n> ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> > +\n> sensorInfo_.outputSize);\n> > +\n> scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> > + }\n> > + request->metadata().set(controls::ScalerCrop, scalerCrop_);\n> >\n> > - /*\n> > - * Also update the ScalerCrop in the metadata with what we actually\n> > - * used. But we must first rescale that from ISP (camera mode)\n> pixels\n> > - * back into sensor native pixels.\n> > - *\n> > - * Sending this information on every frame may be helpful.\n> > - */\n> > - if (updateScalerCrop_) {\n> > - updateScalerCrop_ = false;\n> > - scalerCrop_ =\n> ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> > - sensorInfo_.outputSize);\n> > - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> > + state_ = State::IpaComplete;\n> > }\n> > - request->metadata().set(controls::ScalerCrop, scalerCrop_);\n> > -\n> > - state_ = State::IpaComplete;\n> >\n> > handleState();\n> > }\n> >\n> > void RPiCameraData::runIsp(uint32_t bufferId)\n> > {\n> > - if (state_ == State::Stopped)\n> > - handleState();\n> > + if (state_ != State::Stopped) {\n> > + FrameBuffer *buffer =\n> unicam_[Unicam::Image].getBuffers().at(bufferId);\n> >\n> > - FrameBuffer *buffer =\n> unicam_[Unicam::Image].getBuffers().at(bufferId);\n> > + LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" <<\n> bufferId\n> > + << \", timestamp: \" <<\n> buffer->metadata().timestamp;\n> >\n> > - LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << bufferId\n> > - << \", timestamp: \" << buffer->metadata().timestamp;\n> > + isp_[Isp::Input].queueBuffer(buffer);\n> > + ispOutputCount_ = 0;\n> > + }\n> >\n> > - isp_[Isp::Input].queueBuffer(buffer);\n> > - ispOutputCount_ = 0;\n> > handleState();\n> > }\n> >\n> > void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> > {\n> > - if (state_ == State::Stopped)\n> > - handleState();\n> > + if (state_ != State::Stopped) {\n> > + FrameBuffer *buffer =\n> unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> > + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > + }\n> >\n> > - FrameBuffer *buffer =\n> unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> > - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > handleState();\n> > }\n> >\n> > -void RPiCameraData::setIsp(const ControlList &controls)\n> > +void RPiCameraData::setIspControls(const ControlList &controls)\n> > {\n> > ControlList ctrls = controls;\n> >\n> > - Span s =\n> > - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n> > - bcm2835_isp_lens_shading ls =\n> > - *reinterpret_cast *>(s.data());\n> > - ls.dmabuf = lsTable_.fd();\n> > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {\n>\n> Good catch :)\n>\n> > + Span s =\n> > +\n> ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n> > + bcm2835_isp_lens_shading ls =\n> > + *reinterpret_cast *>(s.data());\n>\n> I think you are right that we don't need to set() the control again\n> below. In that case ls would have to be a pointer.\n>\n\nGreat, I will make that change so we can remove the ctrls.set() call.\n\nRegards,\nNaush\n\n\n>\n> With or without the suggested changes,\n>\n> Reviewed-by: Paul Elder \n>\n> > + ls.dmabuf = lsTable_.fd();\n> >\n> > - ControlValue c(Span{ reinterpret_cast *>(&ls),\n> > - sizeof(ls) });\n> > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);\n> > + ControlValue c(Span{\n> reinterpret_cast(&ls),\n> > + sizeof(ls) });\n> > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);\n> > + }\n> >\n> > isp_[Isp::Input].dev()->setControls(&ctrls);\n> > handleState();\n> > --\n> > 2.25.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 50F67BD1F6\n\tfor ;\n\tWed, 17 Feb 2021 09:03:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C3300637FB;\n\tWed, 17 Feb 2021 10:03:29 +0100 (CET)","from mail-lj1-x22a.google.com (mail-lj1-x22a.google.com\n\t[IPv6:2a00:1450:4864:20::22a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 015AD637F8\n\tfor ;\n\tWed, 17 Feb 2021 10:03:27 +0100 (CET)","by mail-lj1-x22a.google.com with SMTP id u4so15139982ljh.6\n\tfor ;\n\tWed, 17 Feb 2021 01:03:27 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"Iqtlm1sj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=BZrOsgpP4YFhZXnibAqwW5yl7/CXzt/nPslejP/m3VE=;\n\tb=Iqtlm1sjZjPZXw6LVJrjCI0v3WUxXVWDENb5N3XgV/1FtzCr9Cfg35/q7EF26Y8Gp0\n\t9n8JtfxLa/Sgd5gtoukKVxYGQbrDXt4o70uTL4RAlrivP6tbSMOt1fDa+uRt8gQBfUGH\n\tEIj7BQNgvDsp7UpoVr4bc0N6zPnY5B22MTV5q3mHMVPCmk8pEBz0+BsbeW7dODP/smlu\n\t6n/jEOr8wLPRXXWOa9ZQk1i8RUoAanBHTr9ObAA9iLf47KjDlTDt096dhtjjjC499iXj\n\tec1Gr4beIACQycVIxskfadlWxNwx/GmU2Yed/+khTDe1kMhAX8brnp9FJNCYwkFJAv1q\n\tstsw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=BZrOsgpP4YFhZXnibAqwW5yl7/CXzt/nPslejP/m3VE=;\n\tb=EtDKUHOvdY4YsARzUjtOMbIhQw8MV1LSwlHDNJqoislN+wFL/55ks3u/jtW/KD+FRi\n\tLzkuei2Jsf01pjS+10rKTT3UOlNzKXnQEhZ/kAhuE6e+9VK70Zpps+8pt9eqf0X9IwlY\n\tEwVSSSfIPs1zpsEaBfq2Tww01ggMbKpQuTlgvVwvL4sPmrYK/T8sCGcq58VO49p9BCde\n\t5627nCKnPQj2Qe52XHdjOtF3RQ52SH2qLQRIKV6GF0/xoaDmLRt2EJtAg/WXIDnQ6pIW\n\tvlnW+sAvHROiK9jt4A+p9/R2hQJ/ldQrvB9jJ0BStclsOe6K0NwvPHoT4iy/8VUVZCJ5\n\tCYlA==","X-Gm-Message-State":"AOAM532OeYSqGxvstoBKHx2Ic5VdxT8u5F0JqK2Dferdm/IylZYvUCvH\n\tBVwvOKU2RW45AF1BwskDakg3je75sXm7Ouwm9yNOyA==","X-Google-Smtp-Source":"ABdhPJwJZ+/3MUNIQKWzhJDPLNRsr7anWQIAzRVij2LJ8qhDS6KJ1T76/tM/t7ANB/Kty5OJ+lGqbPpHSeZ/mRmi4os=","X-Received":"by 2002:a2e:87d1:: with SMTP id v17mr9136176ljj.48.1613552607280;\n\tWed, 17 Feb 2021 01:03:27 -0800 (PST)","MIME-Version":"1.0","References":"<20210217080241.1492143-1-naush@raspberrypi.com>\n\t<20210217083851.GA1786@pyrite.rasen.tech>","In-Reply-To":"<20210217083851.GA1786@pyrite.rasen.tech>","From":"Naushir Patuck ","Date":"Wed, 17 Feb 2021 09:03:11 +0000","Message-ID":"","To":"Paul Elder ","Subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various\n\tfixups after IPAInterface changes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============5935788736737599727==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15193,"web_url":"https://patchwork.libcamera.org/comment/15193/","msgid":"","date":"2021-02-17T09:04:25","subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various\n\tfixups after IPAInterface changes","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Paul,\n\nOn Wed, 17 Feb 2021 at 08:56, wrote:\n\n> Hi Naush,\n>\n> On Wed, Feb 17, 2021 at 08:28:37AM +0000, Naushir Patuck wrote:\n> > Hi all,\n> >\n> > Sorry, I did have one more question on the new IPA interface topic. In\n> > raspberrypi.mojom, the module namespace is defined as module \"ipa.rpi\".\n> >\n> > Would it make sense to rename this to \"ipa.RPi\" so that it is consistent\n> with\n> > the \"RPi\" namespace used elsewhere?\n>\n> There's nothing technical preventing it. I just chose that\n> capitalization arbitrarily and nobody objected.\n>\n\nI'll change it to RPi then, just to keep the consistency. Sorry I did\nnot flag this in your review rounds.\n\nRegards,\nNaush\n\n\n>\n>\n> Paul\n>\n> >\n> > On Wed, 17 Feb 2021 at 08:02, Naushir Patuck \n> wrote:\n> >\n> > This commit addresses a few fixes and tidy-ups after the IPAInterface\n> > rework:\n> > - Rename ConfigStaggeredWrite -> ConfigSensorParams\n> > - Rename setIsp -> setIspControls\n> > - Signal handlers statsMetadataComplete(), runISP(),\n> embeddedComplete()\n> > should only run when state != Stopped. This matches the behavior of\n> > the original code. Without this, start/stop cycles may cause errors.\n> > - In setIspControls(), only update the LS config (with the fd) when\n> a LS\n> > control is present.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Fixes: e201cb4f5450 (\"libcamera: IPAInterface: Replace C API with\n> the new\n> > C++-only API\")\n> > ---\n> > include/libcamera/ipa/raspberrypi.mojom | 4 +-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 4 +-\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 91\n> ++++++++++---------\n> > 3 files changed, 50 insertions(+), 49 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> b/include/libcamera/\n> > ipa/raspberrypi.mojom\n> > index bab19a946e18..8a256d022270 100644\n> > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > @@ -16,7 +16,7 @@ enum BufferMask {\n> > const uint32 MaxLsGridSize = 0x8000;\n> >\n> > enum ConfigOutputParameters {\n> > - ConfigStaggeredWrite = 0x01,\n> > + ConfigSensorParams = 0x01,\n> > };\n> >\n> > struct SensorConfig {\n> > @@ -126,6 +126,6 @@ interface IPARPiEventInterface {\n> > statsMetadataComplete(uint32 bufferId, ControlList controls);\n> > runIsp(uint32 bufferId);\n> > embeddedComplete(uint32 bufferId);\n> > - setIsp(ControlList controls);\n> > + setIspControls(ControlList controls);\n> > setDelayedControls(ControlList controls);\n> > };\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/\n> > raspberrypi.cpp\n> > index 81a3195c82e9..0bf88bcc5f72 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo &\n> > sensorInfo,\n> > helper_->GetDelays(exposureDelay, gainDelay);\n> > sensorMetadata =\n> helper_->SensorEmbeddedDataPresent();\n> >\n> > - result->params |= ipa::rpi::ConfigStaggeredWrite;\n> > + result->params |= ipa::rpi::ConfigSensorParams;\n> > result->sensorConfig.gainDelay = gainDelay;\n> > result->sensorConfig.exposureDelay = exposureDelay;\n> > result->sensorConfig.vblank = exposureDelay;\n> > @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)\n> > applyDPC(dpcStatus, ctrls);\n> >\n> > if (!ctrls.empty())\n> > - setIsp.emit(ctrls);\n> > + setIspControls.emit(ctrls);\n> > }\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/\n> > libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 15aa600ed581..d0ca015a01dc 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -152,7 +152,7 @@ public:\n> > void statsMetadataComplete(uint32_t bufferId, const\n> ControlList &\n> > controls);\n> > void runIsp(uint32_t bufferId);\n> > void embeddedComplete(uint32_t bufferId);\n> > - void setIsp(const ControlList &controls);\n> > + void setIspControls(const ControlList &controls);\n> > void setDelayedControls(const ControlList &controls);\n> >\n> > /* bufferComplete signal handlers. */\n> > @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()\n> > ipa_->statsMetadataComplete.connect(this, &\n> > RPiCameraData::statsMetadataComplete);\n> > ipa_->runIsp.connect(this, &RPiCameraData::runIsp);\n> > ipa_->embeddedComplete.connect(this, &\n> > RPiCameraData::embeddedComplete);\n> > - ipa_->setIsp.connect(this, &RPiCameraData::setIsp);\n> > + ipa_->setIspControls.connect(this,\n> &RPiCameraData::setIspControls);\n> > ipa_->setDelayedControls.connect(this, &\n> > RPiCameraData::setDelayedControls);\n> >\n> > IPASettings\n> settings(ipa_->configurationFile(sensor_->model() +\n> > \".json\"));\n> > @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const\n> > CameraConfiguration *config)\n> > return -EPIPE;\n> > }\n> >\n> > - if (result.params & ipa::rpi::ConfigStaggeredWrite) {\n> > + if (result.params & ipa::rpi::ConfigSensorParams) {\n> > /*\n> > * Setup our delayed control writer with the sensor\n> default\n> > * gain and exposure delays.\n> > @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const\n> > CameraConfiguration *config)\n> >\n> > void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const\n> > ControlList &controls)\n> > {\n> > - if (state_ == State::Stopped)\n> > - handleState();\n> > + if (state_ != State::Stopped) {\n> > + FrameBuffer *buffer =\n> isp_[Isp::Stats].getBuffers().at\n> > (bufferId);\n> >\n> > - FrameBuffer *buffer =\n> isp_[Isp::Stats].getBuffers().at(bufferId);\n> > + handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> >\n> > - handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > + /* Fill the Request metadata buffer with what the\n> IPA has\n> > provided */\n> > + Request *request = requestQueue_.front();\n> > + request->metadata() = std::move(controls);\n> >\n> > - /* Fill the Request metadata buffer with what the IPA has\n> provided\n> > */\n> > - Request *request = requestQueue_.front();\n> > - request->metadata() = std::move(controls);\n> > + /*\n> > + * Also update the ScalerCrop in the metadata with\n> what we\n> > actually\n> > + * used. But we must first rescale that from ISP\n> (camera\n> > mode) pixels\n> > + * back into sensor native pixels.\n> > + *\n> > + * Sending this information on every frame may be\n> helpful.\n> > + */\n> > + if (updateScalerCrop_) {\n> > + updateScalerCrop_ = false;\n> > + scalerCrop_ = ispCrop_.scaledBy\n> > (sensorInfo_.analogCrop.size(),\n> > +\n> > sensorInfo_.outputSize);\n> > + scalerCrop_.translateBy\n> > (sensorInfo_.analogCrop.topLeft());\n> > + }\n> > + request->metadata().set(controls::ScalerCrop,\n> scalerCrop_);\n> >\n> > - /*\n> > - * Also update the ScalerCrop in the metadata with what we\n> actually\n> > - * used. But we must first rescale that from ISP (camera\n> mode)\n> > pixels\n> > - * back into sensor native pixels.\n> > - *\n> > - * Sending this information on every frame may be helpful.\n> > - */\n> > - if (updateScalerCrop_) {\n> > - updateScalerCrop_ = false;\n> > - scalerCrop_ =\n> ispCrop_.scaledBy(sensorInfo_.analogCrop.size\n> > (),\n> > -\n> sensorInfo_.outputSize);\n> > -\n> scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> > + state_ = State::IpaComplete;\n> > }\n> > - request->metadata().set(controls::ScalerCrop, scalerCrop_);\n> > -\n> > - state_ = State::IpaComplete;\n> >\n> > handleState();\n> > }\n> >\n> > void RPiCameraData::runIsp(uint32_t bufferId)\n> > {\n> > - if (state_ == State::Stopped)\n> > - handleState();\n> > + if (state_ != State::Stopped) {\n> > + FrameBuffer *buffer =\n> unicam_[Unicam::Image].getBuffers\n> > ().at(bufferId);\n> >\n> > - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at\n> > (bufferId);\n> > + LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id\n> \" <<\n> > bufferId\n> > + << \", timestamp: \" <<\n> buffer->metadata\n> > ().timestamp;\n> >\n> > - LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" <<\n> bufferId\n> > - << \", timestamp: \" <<\n> buffer->metadata().timestamp;\n> > + isp_[Isp::Input].queueBuffer(buffer);\n> > + ispOutputCount_ = 0;\n> > + }\n> >\n> > - isp_[Isp::Input].queueBuffer(buffer);\n> > - ispOutputCount_ = 0;\n> > handleState();\n> > }\n> >\n> > void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> > {\n> > - if (state_ == State::Stopped)\n> > - handleState();\n> > + if (state_ != State::Stopped) {\n> > + FrameBuffer *buffer =\n> unicam_[Unicam::Embedded].getBuffers\n> > ().at(bufferId);\n> > + handleStreamBuffer(buffer,\n> &unicam_[Unicam::Embedded]);\n> > + }\n> >\n> > - FrameBuffer *buffer =\n> unicam_[Unicam::Embedded].getBuffers().at\n> > (bufferId);\n> > - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > handleState();\n> > }\n> >\n> > -void RPiCameraData::setIsp(const ControlList &controls)\n> > +void RPiCameraData::setIspControls(const ControlList &controls)\n> > {\n> > ControlList ctrls = controls;\n> >\n> > - Span s =\n> > -\n> ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n> > - bcm2835_isp_lens_shading ls =\n> > - *reinterpret_cast *>(s.data\n> > ());\n> > - ls.dmabuf = lsTable_.fd();\n> > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {\n> > + Span s =\n> > + ctrls.get\n> > (V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n> > + bcm2835_isp_lens_shading ls =\n> > + *reinterpret_cast bcm2835_isp_lens_shading *>\n> > (s.data());\n> > + ls.dmabuf = lsTable_.fd();\n> >\n> > - ControlValue c(Span{ reinterpret_cast *>(&\n> > ls),\n> > - sizeof(ls) });\n> > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);\n> > + ControlValue c(Span{ reinterpret_cast\n> > (&ls),\n> > + sizeof(ls) });\n> > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);\n> > + }\n> >\n> > isp_[Isp::Input].dev()->setControls(&ctrls);\n> > handleState();\n> > --\n> > 2.25.1\n> >\n> >\n>\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A5045BD1F6\n\tfor ;\n\tWed, 17 Feb 2021 09:04:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6255763805;\n\tWed, 17 Feb 2021 10:04:43 +0100 (CET)","from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 560B9637FD\n\tfor ;\n\tWed, 17 Feb 2021 10:04:42 +0100 (CET)","by mail-lf1-x130.google.com with SMTP id j19so20189032lfr.12\n\tfor ;\n\tWed, 17 Feb 2021 01:04:42 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"js7FZp4r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=H7J3f2hv5KB4mdqOtGdDu3LbwFwIwivkoNgXb9Kvfc8=;\n\tb=js7FZp4rbDTzWZRM6dwD9yqfA6s78v+0TBlKA501HD44XPyfv2G5wOVtUb5Dy7jtaq\n\tmKreJUzayCaxugytufykEJCeEaCWi7bYEjUXa/8ZdII57v5SVBGO3yU7rQMdrjNNmg0R\n\tI7BbDXv08aldkQxU1K+HniOvJ2/sBbmUnxLm0gulJId0TjaE01ayCj4qJNxS6oGkdLAi\n\tMtcBkSHMQS5H3ULf2CN1wsmYncaQXRDjRSwpAdc2ZteERqW0/GkjkJbp++ls9HD+cEJz\n\tm0JjQy6pklcQTZ9tNssJEEoZX6rQS4IPJ169ui7ZriGSVfWYqNgzzjXBKy4mvW+04Hkd\n\t5nmA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=H7J3f2hv5KB4mdqOtGdDu3LbwFwIwivkoNgXb9Kvfc8=;\n\tb=SwysEA6fDoYLgXyRLLbEZb6BRV0LOMr36TelGBhkPVJ9PaofckuTSxW0ewQusNQPp7\n\t9dHFFNgiqiEtw5GzYiI2tzhWGHx7xIJCkm/rykEEjsFHkl62tE9MUHKfrCI/bg2qLGea\n\tC9l2csKvyUPazQZWmz8xSTREtyCQpEghwTHjD1ImIGviDWojNHncb1Ar70/dYlyWADgz\n\t14FQCtWJV1HZ2dKAn9UDZiKLoW/84DQeEHspm9bOOpiupnX4JiLJDgOXF7HvhjE0RP2K\n\t+dXKpIv7GJdmB4wn8qjVNdrmUGDxehqJf+3UO/IvKzeAs28BDbK5wk1jeu4RWIlkagPi\n\tDO9w==","X-Gm-Message-State":"AOAM530htLNzzPmqRWXGcPhdogcJYLEz8X1StU8gIHMu1UMeANvmWvnB\n\tGCEv1SHEV+C9UZBNtKXF2qiXHCx10lfOlkdOX2V8hfohE8o+Vn8w","X-Google-Smtp-Source":"ABdhPJyJsd5gCzQ8AB0TrJGXyhPxR+lFcmIkUcatCIBOziFZETu2QEDskstTNriWtKsdRJrO21gsea4QXJua37E5T+0=","X-Received":"by 2002:a19:48c2:: with SMTP id\n\tv185mr13990323lfa.375.1613552681585; \n\tWed, 17 Feb 2021 01:04:41 -0800 (PST)","MIME-Version":"1.0","References":"<20210217080241.1492143-1-naush@raspberrypi.com>\n\t\n\t<20210217085650.GE1786@pyrite.rasen.tech>","In-Reply-To":"<20210217085650.GE1786@pyrite.rasen.tech>","From":"Naushir Patuck ","Date":"Wed, 17 Feb 2021 09:04:25 +0000","Message-ID":"","To":"Paul Elder ","Subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various\n\tfixups after IPAInterface changes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============1206483465036372045==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15198,"web_url":"https://patchwork.libcamera.org/comment/15198/","msgid":"","date":"2021-02-17T09:59:36","subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various\n\tfixups after IPAInterface changes","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Paul,\n\n\nOn Wed, 17 Feb 2021 at 09:03, Naushir Patuck wrote:\n\n> Hi Paul,\n>\n> Thank you for your review feedback.\n>\n> On Wed, 17 Feb 2021 at 08:38, wrote:\n>\n>> Hi Naush,\n>>\n>> Thank you for the patch.\n>>\n>> On Wed, Feb 17, 2021 at 08:02:41AM +0000, Naushir Patuck wrote:\n>> > This commit addresses a few fixes and tidy-ups after the IPAInterface\n>> > rework:\n>> > - Rename ConfigStaggeredWrite -> ConfigSensorParams\n>> > - Rename setIsp -> setIspControls\n>> > - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete()\n>> > should only run when state != Stopped. This matches the behavior of\n>> > the original code. Without this, start/stop cycles may cause errors.\n>> > - In setIspControls(), only update the LS config (with the fd) when a LS\n>> > control is present.\n>> >\n>> > Signed-off-by: Naushir Patuck \n>> > Fixes: e201cb4f5450 (\"libcamera: IPAInterface: Replace C API with the\n>> new C++-only API\")\n>> > ---\n>> > include/libcamera/ipa/raspberrypi.mojom | 4 +-\n>> > src/ipa/raspberrypi/raspberrypi.cpp | 4 +-\n>> > .../pipeline/raspberrypi/raspberrypi.cpp | 91 ++++++++++---------\n>> > 3 files changed, 50 insertions(+), 49 deletions(-)\n>> >\n>> > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n>> b/include/libcamera/ipa/raspberrypi.mojom\n>> > index bab19a946e18..8a256d022270 100644\n>> > --- a/include/libcamera/ipa/raspberrypi.mojom\n>> > +++ b/include/libcamera/ipa/raspberrypi.mojom\n>> > @@ -16,7 +16,7 @@ enum BufferMask {\n>> > const uint32 MaxLsGridSize = 0x8000;\n>> >\n>> > enum ConfigOutputParameters {\n>> > - ConfigStaggeredWrite = 0x01,\n>> > + ConfigSensorParams = 0x01,\n>> > };\n>> >\n>> > struct SensorConfig {\n>> > @@ -126,6 +126,6 @@ interface IPARPiEventInterface {\n>> > statsMetadataComplete(uint32 bufferId, ControlList controls);\n>> > runIsp(uint32 bufferId);\n>> > embeddedComplete(uint32 bufferId);\n>> > - setIsp(ControlList controls);\n>> > + setIspControls(ControlList controls);\n>> > setDelayedControls(ControlList controls);\n>> > };\n>> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n>> b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > index 81a3195c82e9..0bf88bcc5f72 100644\n>> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo\n>> &sensorInfo,\n>> > helper_->GetDelays(exposureDelay, gainDelay);\n>> > sensorMetadata = helper_->SensorEmbeddedDataPresent();\n>> >\n>> > - result->params |= ipa::rpi::ConfigStaggeredWrite;\n>> > + result->params |= ipa::rpi::ConfigSensorParams;\n>> > result->sensorConfig.gainDelay = gainDelay;\n>> > result->sensorConfig.exposureDelay = exposureDelay;\n>> > result->sensorConfig.vblank = exposureDelay;\n>> > @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)\n>> > applyDPC(dpcStatus, ctrls);\n>> >\n>> > if (!ctrls.empty())\n>> > - setIsp.emit(ctrls);\n>> > + setIspControls.emit(ctrls);\n>> > }\n>> > }\n>> >\n>> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > index 15aa600ed581..d0ca015a01dc 100644\n>> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > @@ -152,7 +152,7 @@ public:\n>> > void statsMetadataComplete(uint32_t bufferId, const ControlList\n>> &controls);\n>> > void runIsp(uint32_t bufferId);\n>> > void embeddedComplete(uint32_t bufferId);\n>> > - void setIsp(const ControlList &controls);\n>> > + void setIspControls(const ControlList &controls);\n>> > void setDelayedControls(const ControlList &controls);\n>> >\n>> > /* bufferComplete signal handlers. */\n>> > @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()\n>> > ipa_->statsMetadataComplete.connect(this,\n>> &RPiCameraData::statsMetadataComplete);\n>> > ipa_->runIsp.connect(this, &RPiCameraData::runIsp);\n>> > ipa_->embeddedComplete.connect(this,\n>> &RPiCameraData::embeddedComplete);\n>> > - ipa_->setIsp.connect(this, &RPiCameraData::setIsp);\n>> > + ipa_->setIspControls.connect(this,\n>> &RPiCameraData::setIspControls);\n>> > ipa_->setDelayedControls.connect(this,\n>> &RPiCameraData::setDelayedControls);\n>> >\n>> > IPASettings settings(ipa_->configurationFile(sensor_->model() +\n>> \".json\"));\n>> > @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const\n>> CameraConfiguration *config)\n>> > return -EPIPE;\n>> > }\n>> >\n>> > - if (result.params & ipa::rpi::ConfigStaggeredWrite) {\n>> > + if (result.params & ipa::rpi::ConfigSensorParams) {\n>> > /*\n>> > * Setup our delayed control writer with the sensor\n>> default\n>> > * gain and exposure delays.\n>> > @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const\n>> CameraConfiguration *config)\n>> >\n>> > void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const\n>> ControlList &controls)\n>> > {\n>> > - if (state_ == State::Stopped)\n>> > - handleState();\n>>\n>> For this (and the ones below) I wonder if it would be cleaner to return\n>> here instead. That was actually my intention, I guess it slipped through\n>> when I did the translation.\n>>\n>\n> I'm happy to do either. What do others feel is cleaner?\n>\n\nActually having read the code in handleState(), it does not do anything\nwhen state_ == State::Stopped anymore. So, these can simply be replaced\nwith a single return statement.\nI'll make the change and submit a revision including the changes for the\nipa::rpi namespace change and the LS fixes. I will not add your R-B tag so\nyou have a chance to re-review if that's ok.\n\nRegards,\nNaush\n\n\n>\n>\n>>\n>> > + if (state_ != State::Stopped) {\n>> > + FrameBuffer *buffer =\n>> isp_[Isp::Stats].getBuffers().at(bufferId);\n>> >\n>> > - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n>> > + handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n>> >\n>> > - handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n>> > + /* Fill the Request metadata buffer with what the IPA has\n>> provided */\n>> > + Request *request = requestQueue_.front();\n>> > + request->metadata() = std::move(controls);\n>> >\n>> > - /* Fill the Request metadata buffer with what the IPA has\n>> provided */\n>> > - Request *request = requestQueue_.front();\n>> > - request->metadata() = std::move(controls);\n>> > + /*\n>> > + * Also update the ScalerCrop in the metadata with what we\n>> actually\n>> > + * used. But we must first rescale that from ISP (camera\n>> mode) pixels\n>> > + * back into sensor native pixels.\n>> > + *\n>> > + * Sending this information on every frame may be helpful.\n>> > + */\n>> > + if (updateScalerCrop_) {\n>> > + updateScalerCrop_ = false;\n>> > + scalerCrop_ =\n>> ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n>> > +\n>> sensorInfo_.outputSize);\n>> > +\n>> scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n>> > + }\n>> > + request->metadata().set(controls::ScalerCrop,\n>> scalerCrop_);\n>> >\n>> > - /*\n>> > - * Also update the ScalerCrop in the metadata with what we\n>> actually\n>> > - * used. But we must first rescale that from ISP (camera mode)\n>> pixels\n>> > - * back into sensor native pixels.\n>> > - *\n>> > - * Sending this information on every frame may be helpful.\n>> > - */\n>> > - if (updateScalerCrop_) {\n>> > - updateScalerCrop_ = false;\n>> > - scalerCrop_ =\n>> ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n>> > - sensorInfo_.outputSize);\n>> > - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n>> > + state_ = State::IpaComplete;\n>> > }\n>> > - request->metadata().set(controls::ScalerCrop, scalerCrop_);\n>> > -\n>> > - state_ = State::IpaComplete;\n>> >\n>> > handleState();\n>> > }\n>> >\n>> > void RPiCameraData::runIsp(uint32_t bufferId)\n>> > {\n>> > - if (state_ == State::Stopped)\n>> > - handleState();\n>> > + if (state_ != State::Stopped) {\n>> > + FrameBuffer *buffer =\n>> unicam_[Unicam::Image].getBuffers().at(bufferId);\n>> >\n>> > - FrameBuffer *buffer =\n>> unicam_[Unicam::Image].getBuffers().at(bufferId);\n>> > + LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" <<\n>> bufferId\n>> > + << \", timestamp: \" <<\n>> buffer->metadata().timestamp;\n>> >\n>> > - LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << bufferId\n>> > - << \", timestamp: \" <<\n>> buffer->metadata().timestamp;\n>> > + isp_[Isp::Input].queueBuffer(buffer);\n>> > + ispOutputCount_ = 0;\n>> > + }\n>> >\n>> > - isp_[Isp::Input].queueBuffer(buffer);\n>> > - ispOutputCount_ = 0;\n>> > handleState();\n>> > }\n>> >\n>> > void RPiCameraData::embeddedComplete(uint32_t bufferId)\n>> > {\n>> > - if (state_ == State::Stopped)\n>> > - handleState();\n>> > + if (state_ != State::Stopped) {\n>> > + FrameBuffer *buffer =\n>> unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n>> > + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n>> > + }\n>> >\n>> > - FrameBuffer *buffer =\n>> unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n>> > - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n>> > handleState();\n>> > }\n>> >\n>> > -void RPiCameraData::setIsp(const ControlList &controls)\n>> > +void RPiCameraData::setIspControls(const ControlList &controls)\n>> > {\n>> > ControlList ctrls = controls;\n>> >\n>> > - Span s =\n>> > - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n>> > - bcm2835_isp_lens_shading ls =\n>> > - *reinterpret_cast> *>(s.data());\n>> > - ls.dmabuf = lsTable_.fd();\n>> > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {\n>>\n>> Good catch :)\n>>\n>> > + Span s =\n>> > +\n>> ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n>> > + bcm2835_isp_lens_shading ls =\n>> > + *reinterpret_cast> *>(s.data());\n>>\n>> I think you are right that we don't need to set() the control again\n>> below. In that case ls would have to be a pointer.\n>>\n>\n> Great, I will make that change so we can remove the ctrls.set() call.\n>\n> Regards,\n> Naush\n>\n>\n>>\n>> With or without the suggested changes,\n>>\n>> Reviewed-by: Paul Elder \n>>\n>> > + ls.dmabuf = lsTable_.fd();\n>> >\n>> > - ControlValue c(Span{ reinterpret_cast> *>(&ls),\n>> > - sizeof(ls) });\n>> > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);\n>> > + ControlValue c(Span{\n>> reinterpret_cast(&ls),\n>> > + sizeof(ls) });\n>> > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);\n>> > + }\n>> >\n>> > isp_[Isp::Input].dev()->setControls(&ctrls);\n>> > handleState();\n>> > --\n>> > 2.25.1\n>> >\n>> > _______________________________________________\n>> > libcamera-devel mailing list\n>> > libcamera-devel@lists.libcamera.org\n>> > https://lists.libcamera.org/listinfo/libcamera-devel\n>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5B328BD1F1\n\tfor ;\n\tWed, 17 Feb 2021 09:59:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D38726380E;\n\tWed, 17 Feb 2021 10:59:53 +0100 (CET)","from mail-lj1-x232.google.com (mail-lj1-x232.google.com\n\t[IPv6:2a00:1450:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C6C7E602FA\n\tfor ;\n\tWed, 17 Feb 2021 10:59:52 +0100 (CET)","by mail-lj1-x232.google.com with SMTP id b16so15268814lji.13\n\tfor ;\n\tWed, 17 Feb 2021 01:59:52 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"UmtvNNMH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=tGJpSM8AHmswdtqp/qghpgAsKLhsaO3tkAVtkmEha3I=;\n\tb=UmtvNNMH9WrbfmT0mETWaOCAUsvk2H3eCujKZl5aTO9f/YPVa3rK2GmSlRFXCitaGm\n\tOt7jmC+8CjScTPch9nH4yboRvy4tTcQtFp0PRPow4E1SEdC63w1uOrRE8Ufq3ce7sX3N\n\tRZ5WWVLiyX97ayvO4zxRAGYzyvmpSjlvKLan2M43HGA0Sfy62kmzsT9sDuEZH5kYZGKC\n\tCp2a7MTInf4NCCYHspbvuIOBeQBFhOCHwrX9+kQFJA7z9W9yanmiRvbpewJmPzg8w0kw\n\twd9OljmlASwioe9U/uhtJ41Kk7az5iyvv5EjjA5MSlER2eR01+Gum2rGbJh8m57c5Gdd\n\t4Q8w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=tGJpSM8AHmswdtqp/qghpgAsKLhsaO3tkAVtkmEha3I=;\n\tb=mzzo/sfPs0AIBgTkQFYeVx3naRmxhkE0NddgZ/jDxJyRF9IPnuzWnBVUIhFXVRlET2\n\tiQEMPR/Et31IBTNKjDsALCgDm5wO2SEEIh7GnCSM4Hsp9oxQ7tSLGEry2EmO/lcQ/ghs\n\t79w7cI1Xn+50qYOxa+am4os650UyuF2q/ay+YpeLfU2JygtR8zNzmsJ5TeD5pXgq553+\n\tMIQaS7UdwCW3fWu/DLNJyVLYqYWwChvyh6pJkNsOO3eZUszswv6EMzKH8yx9p9pYCKUi\n\tSOfH3EFtB5cd6paDpkRqDPjR+epKceA/hKu39e+MglR1Q2KKFUAGdySay/NML78B8Cpi\n\trLpw==","X-Gm-Message-State":"AOAM530NjYkvS953d52/0u7pj+ycJ5OdT8liqqbhLFrqJXp8TgVGHPPi\n\to4+X0Rvje8pa7YlLppO89B2r9BK/UfYzZewroCl1WCWGvY1ycX4W","X-Google-Smtp-Source":"ABdhPJyzEZ3Gf/RdRKl298eqLcrDQB7897dCV43U0ZVkea/ij3rhz6o0bthu4MBwzNFkTKRJau/bI+AV4cpUjDRSw9Y=","X-Received":"by 2002:a2e:a376:: with SMTP id\n\ti22mr14250594ljn.299.1613555992149; \n\tWed, 17 Feb 2021 01:59:52 -0800 (PST)","MIME-Version":"1.0","References":"<20210217080241.1492143-1-naush@raspberrypi.com>\n\t<20210217083851.GA1786@pyrite.rasen.tech>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 17 Feb 2021 09:59:36 +0000","Message-ID":"","To":"Paul Elder ","Subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various\n\tfixups after IPAInterface changes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============3706902865240676728==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]