[{"id":15201,"web_url":"https://patchwork.libcamera.org/comment/15201/","msgid":"<20210217101846.GH17707@pyrite.rasen.tech>","date":"2021-02-17T10:18:46","subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups 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 10:08:50AM +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> - Switch to camel case for ISPConfig::embeddedbufferId and\n> ISPConfig::bayerbufferId.\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 | 8 ++---\n> src/ipa/raspberrypi/raspberrypi.cpp | 8 ++---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 36 ++++++++++---------\n> 3 files changed, 27 insertions(+), 25 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> index bab19a946e18..9c05cc68cceb 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> @@ -27,8 +27,8 @@ struct SensorConfig {\n> };\n> \n> struct ISPConfig {\n> -\tuint32 embeddedbufferId;\n> -\tuint32 bayerbufferId;\n> +\tuint32 embeddedBufferId;\n> +\tuint32 bayerBufferId;\n> };\n> \n> struct ConfigInput {\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..974f4ec63058 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> @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const ipa::rpi::ISPConfig &data)\n> \t * avoid running the control algos for a few frames in case\n> \t * they are \"unreliable\".\n> \t */\n> -\tprepareISP(data.embeddedbufferId);\n> +\tprepareISP(data.embeddedBufferId);\n> \tframeCount_++;\n> \n> \t/* Ready to push the input buffer into the ISP. */\n> -\trunIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);\n> +\trunIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);\n> }\n> \n> void IPARPi::reportMetadata()\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..8770ae66a21a 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> @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)\n> {\n> \tif (state_ == State::Stopped)\n> -\t\thandleState();\n> +\t\treturn;\n\nI thought handleState() still needs to be called in this (and below) case?\n\n\nPaul\n\n> \n> \tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> \n> @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n> void RPiCameraData::runIsp(uint32_t bufferId)\n> {\n> \tif (state_ == State::Stopped)\n> -\t\thandleState();\n> +\t\treturn;\n> \n> \tFrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> \n> @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)\n> void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> {\n> \tif (state_ == State::Stopped)\n> -\t\thandleState();\n> +\t\treturn;\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> +\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> +\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> @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()\n> \t\t\t<< \" Embedded buffer id: \" << embeddedId;\n> \n> \tipa::rpi::ISPConfig ispPrepare;\n> -\tispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData | embeddedId;\n> -\tispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;\n> +\tispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData | embeddedId;\n> +\tispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;\n> \tipa_->signalIspPrepare(ispPrepare);\n> }\n> \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 7516CBD1F6\n\tfor ;\n\tWed, 17 Feb 2021 10:18:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0BE556381F;\n\tWed, 17 Feb 2021 11:18:56 +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 7B139637F7\n\tfor ;\n\tWed, 17 Feb 2021 11:18:54 +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 E7AB68C4;\n\tWed, 17 Feb 2021 11:18:52 +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=\"T6C4D5x+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613557134;\n\tbh=k1/wMjDmQOb+hiEktAceKlXtKhsvhrR4xDylnwXBgoc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T6C4D5x+l8zcqqnElL1fUkNfttU2Jl4pBJlprVhgqrB4sR4Su0wfhAXPE6AaEH2Sd\n\t/GtsSYKojLUy90r/Q/AjLxKxG3bbTDVrrdtoUPH7htRMEut+QR3B8b5laUcsqhCrZT\n\tk9MYBqWpTlTnFz5XcxmhFkMkUZxvWj5jBCbXwCN0=","Date":"Wed, 17 Feb 2021 19:18:46 +0900","From":"paul.elder@ideasonboard.com","To":"Naushir Patuck ","Message-ID":"<20210217101846.GH17707@pyrite.rasen.tech>","References":"<20210217100852.1542397-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210217100852.1542397-1-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups 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":15203,"web_url":"https://patchwork.libcamera.org/comment/15203/","msgid":"","date":"2021-02-17T10:25:29","subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups 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 10:18, wrote:\n\n> Hi Naush,\n>\n> On Wed, Feb 17, 2021 at 10:08:50AM +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> > - Switch to camel case for ISPConfig::embeddedbufferId and\n> > ISPConfig::bayerbufferId.\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 | 8 ++---\n> > src/ipa/raspberrypi/raspberrypi.cpp | 8 ++---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 36 ++++++++++---------\n> > 3 files changed, 27 insertions(+), 25 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> b/include/libcamera/ipa/raspberrypi.mojom\n> > index bab19a946e18..9c05cc68cceb 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> > @@ -27,8 +27,8 @@ struct SensorConfig {\n> > };\n> >\n> > struct ISPConfig {\n> > - uint32 embeddedbufferId;\n> > - uint32 bayerbufferId;\n> > + uint32 embeddedBufferId;\n> > + uint32 bayerBufferId;\n> > };\n> >\n> > struct ConfigInput {\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..974f4ec63058 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> > @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const\n> ipa::rpi::ISPConfig &data)\n> > * avoid running the control algos for a few frames in case\n> > * they are \"unreliable\".\n> > */\n> > - prepareISP(data.embeddedbufferId);\n> > + prepareISP(data.embeddedBufferId);\n> > frameCount_++;\n> >\n> > /* Ready to push the input buffer into the ISP. */\n> > - runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);\n> > + runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);\n> > }\n> >\n> > void IPARPi::reportMetadata()\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..8770ae66a21a 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> > @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> > void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const\n> ControlList &controls)\n> > {\n> > if (state_ == State::Stopped)\n> > - handleState();\n> > + return;\n>\n> I thought handleState() still needs to be called in this (and below) case?\n>\n\nI thought so too :)\n\nBut if you have a look at handleState(), in the case State::Stopped, it\nsimply breaks and returns doing nothing.\nSo, replacing by a single return is sufficient. Clearly it did do\nsomething in the past, but we should be ok without\ncalling handleState now.\n\nRegards,\nNaush\n\n\n\n>\n>\n> Paul\n>\n> >\n> > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> >\n> > @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t\n> bufferId, const ControlList &\n> > void RPiCameraData::runIsp(uint32_t bufferId)\n> > {\n> > if (state_ == State::Stopped)\n> > - handleState();\n> > + return;\n> >\n> > FrameBuffer *buffer =\n> unicam_[Unicam::Image].getBuffers().at(bufferId);\n> >\n> > @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)\n> > void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> > {\n> > if (state_ == State::Stopped)\n> > - handleState();\n> > + return;\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> > @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()\n> > << \" Embedded buffer id: \" << embeddedId;\n> >\n> > ipa::rpi::ISPConfig ispPrepare;\n> > - ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData |\n> embeddedId;\n> > - ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;\n> > + ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData |\n> embeddedId;\n> > + ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;\n> > ipa_->signalIspPrepare(ispPrepare);\n> > }\n> >\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 A9A5CBD1F1\n\tfor ;\n\tWed, 17 Feb 2021 10:25:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 76D40660F9;\n\tWed, 17 Feb 2021 11:25:47 +0100 (CET)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CF3796380C\n\tfor ;\n\tWed, 17 Feb 2021 11:25:45 +0100 (CET)","by mail-lf1-x133.google.com with SMTP id d24so20561398lfs.8\n\tfor ;\n\tWed, 17 Feb 2021 02:25:45 -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=\"CBe54hQM\"; 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=0rtdTC8HW2nDO9Bw279D+ggkwyUTCo1GYXmKt6bPp0M=;\n\tb=CBe54hQMRFtaVsdMZ/OUKCkiPudVVT/fRbYNGjIdjVvVdk4J3Okna4Szoq52MYY3HN\n\tZPgQgtX64SSzr9EXtRweL2DtbyvyDNN3mCFchxyXGjUJdjo8gsUvyF1QwuSVmWKlhhOn\n\tzjvC/LhFxusZvJ9sYd0uDvesf41CcqG2wD/yfR2EiN8vnlGX5qYS7objTdie/bu5lMUF\n\ttRpG3DGsthgIcG5vSO7jCeAYkuMHU8PVUjCFpVOByDZt6ftwrzKM7pLOPNLJwCmgrslF\n\tPkImupOzJXnKK4NbEVmZf+Lh9zKrd3fT7Xt1Unlhh4aQXC5wTTP4cwU14olZAOwFm23w\n\tZwSw==","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=0rtdTC8HW2nDO9Bw279D+ggkwyUTCo1GYXmKt6bPp0M=;\n\tb=XUJFz0ABd++DDzcJKuao9rBVMOvp8dII5bK/3/nzFZ8eIIfsvMsrolrisKriW3VOE4\n\t3f9uSzCJjh3bWxBv/hxRigCruQkLMkgprVvMuwYtGEFfyHXPSo1ukTffS+7qMNVh41c3\n\t5UMrqwJgFXsL6Ps1NkgRh94EZ/KeHaWMOyY68pyLJnEsvpqL3nh7bEYxtQjxYNBnGq3h\n\tNGMgzmlgBH5urS+refv7Zi4X5ABvOCxdcDblC/Gw9ouGw7Fj+dksyBuhglImBJXYLzOf\n\tLQjIak7vS0Syla5RXWiJq0CTqnhC7/IOSqnfmuyWVQebqEgkYKyvcudDzOLlADVrRR+3\n\tDnpg==","X-Gm-Message-State":"AOAM5318be3GfN88O3ocJAS/L8a+mqlbQdK6ovnNzpU0w55j2OF972gY\n\ttUIp9Q+MJqbX7KrY9Ae0QPxjnTfs6ovSLUKv5h7ntA==","X-Google-Smtp-Source":"ABdhPJxW/YiAJH7uHhg3KWvOcA2EntNeHCZbp2inUq61xG9AnKkofRzTH573rREvdHtP6zlinklTD8beicOVVlzCahw=","X-Received":"by 2002:a05:6512:114e:: with SMTP id\n\tm14mr14146651lfg.617.1613557545166; \n\tWed, 17 Feb 2021 02:25:45 -0800 (PST)","MIME-Version":"1.0","References":"<20210217100852.1542397-1-naush@raspberrypi.com>\n\t<20210217101846.GH17707@pyrite.rasen.tech>","In-Reply-To":"<20210217101846.GH17707@pyrite.rasen.tech>","From":"Naushir Patuck ","Date":"Wed, 17 Feb 2021 10:25:29 +0000","Message-ID":"","To":"Paul Elder ","Subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups 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=\"===============5919850469282023069==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15205,"web_url":"https://patchwork.libcamera.org/comment/15205/","msgid":"<20210217102851.GJ17707@pyrite.rasen.tech>","date":"2021-02-17T10:28:51","subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups 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 10:25:29AM +0000, Naushir Patuck wrote:\n> Hi Paul,\n> \n> \n> On Wed, 17 Feb 2021 at 10:18, wrote:\n> \n> Hi Naush,\n> \n> On Wed, Feb 17, 2021 at 10:08:50AM +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> > - Switch to camel case for ISPConfig::embeddedbufferId and\n> > ISPConfig::bayerbufferId.\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       |  8 ++---\n> >  src/ipa/raspberrypi/raspberrypi.cpp           |  8 ++---\n> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 36 ++++++++++---------\n> >  3 files changed, 27 insertions(+), 25 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/\n> ipa/raspberrypi.mojom\n> > index bab19a946e18..9c05cc68cceb 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> > @@ -27,8 +27,8 @@ struct SensorConfig {\n> >  };\n> > \n> >  struct ISPConfig {\n> > -     uint32 embeddedbufferId;\n> > -     uint32 bayerbufferId;\n> > +     uint32 embeddedBufferId;\n> > +     uint32 bayerBufferId;\n> >  };\n> > \n> >  struct ConfigInput {\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..974f4ec63058 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> > @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const\n> ipa::rpi::ISPConfig &data)\n> >        * avoid running the control algos for a few frames in case\n> >        * they are \"unreliable\".\n> >        */\n> > -     prepareISP(data.embeddedbufferId);\n> > +     prepareISP(data.embeddedBufferId);\n> >       frameCount_++;\n> > \n> >       /* Ready to push the input buffer into the ISP. */\n> > -     runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);\n> > +     runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);\n> >  }\n> > \n> >  void IPARPi::reportMetadata()\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..8770ae66a21a 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> > @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> >  void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const\n> ControlList &controls)\n> >  {\n> >       if (state_ == State::Stopped)\n> > -             handleState();\n> > +             return;\n> \n> I thought handleState() still needs to be called in this (and below) case?\n> \n> \n> I thought so too :)\n> \n> But if you have a look at handleState(), in the case State::Stopped, it simply\n> breaks and returns doing nothing.\n> So, replacing by a single return is sufficient.  Clearly it did do something in\n> the past, but we should be ok without\n> calling handleState now.\n\nAh, I see. I just saw your reply to v1 too :)\n\nWell that's nice, it makes everything cleaner.\n\nReviewed-by: Paul Elder \n\n\n> \n> > \n> >       FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> > \n> > @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t\n> bufferId, const ControlList &\n> >  void RPiCameraData::runIsp(uint32_t bufferId)\n> >  {\n> >       if (state_ == State::Stopped)\n> > -             handleState();\n> > +             return;\n> > \n> >       FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at\n> (bufferId);\n> > \n> > @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)\n> >  void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> >  {\n> >       if (state_ == State::Stopped)\n> > -             handleState();\n> > +             return;\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> > @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()\n> >                       << \" Embedded buffer id: \" << embeddedId;\n> > \n> >       ipa::rpi::ISPConfig ispPrepare;\n> > -     ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData |\n> embeddedId;\n> > -     ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;\n> > +     ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData |\n> embeddedId;\n> > +     ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;\n> >       ipa_->signalIspPrepare(ispPrepare);\n> >  }\n> > \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 3FFC7BD1F6\n\tfor ;\n\tWed, 17 Feb 2021 10:29:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C05F7680A7;\n\tWed, 17 Feb 2021 11:29:01 +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 AD8976380C\n\tfor ;\n\tWed, 17 Feb 2021 11:28: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 E4E788C4;\n\tWed, 17 Feb 2021 11:28: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=\"sJICvJ6k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613557739;\n\tbh=WvasMZaaqb/z2S8+uRjs0LPdNpPLZVObG6Yk0RuuFGY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sJICvJ6k34vIS2Xx6gM11//Tq60XbvBjrurxmheywfsvxTq9zEqK2Oa70kYpkdfci\n\tez64QZztv2HSMP6/YQntZvyShouWSOiR2fONUTL/g9PtPBZeR6mGmb4r2eqK+Ss3d1\n\tor3oIc0eeBnvbEWMa2iHcrYCI4d/MgzvTGk6bjCQ=","Date":"Wed, 17 Feb 2021 19:28:51 +0900","From":"paul.elder@ideasonboard.com","To":"Naushir Patuck ","Message-ID":"<20210217102851.GJ17707@pyrite.rasen.tech>","References":"<20210217100852.1542397-1-naush@raspberrypi.com>\n\t<20210217101846.GH17707@pyrite.rasen.tech>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups 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":15206,"web_url":"https://patchwork.libcamera.org/comment/15206/","msgid":"","date":"2021-02-17T11:18:55","subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups after IPAInterface changes","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for these patches. They fix an intermittent crash I was\nexperiencing when trying to stop libcamera (in fact exactly as\npredicted in the commit message in relation to the signal handlers),\nthus:\n\nTested-by: David Plowman \n\nBest regards\nDavid\n\nOn Wed, 17 Feb 2021 at 10:29, wrote:\n>\n> Hi Naush,\n>\n> On Wed, Feb 17, 2021 at 10:25:29AM +0000, Naushir Patuck wrote:\n> > Hi Paul,\n> >\n> >\n> > On Wed, 17 Feb 2021 at 10:18, wrote:\n> >\n> > Hi Naush,\n> >\n> > On Wed, Feb 17, 2021 at 10:08:50AM +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> > > - Switch to camel case for ISPConfig::embeddedbufferId and\n> > > ISPConfig::bayerbufferId.\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 | 8 ++---\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 8 ++---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 36 ++++++++++---------\n> > > 3 files changed, 27 insertions(+), 25 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/\n> > ipa/raspberrypi.mojom\n> > > index bab19a946e18..9c05cc68cceb 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> > > @@ -27,8 +27,8 @@ struct SensorConfig {\n> > > };\n> > >\n> > > struct ISPConfig {\n> > > - uint32 embeddedbufferId;\n> > > - uint32 bayerbufferId;\n> > > + uint32 embeddedBufferId;\n> > > + uint32 bayerBufferId;\n> > > };\n> > >\n> > > struct ConfigInput {\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..974f4ec63058 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> > > @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const\n> > ipa::rpi::ISPConfig &data)\n> > > * avoid running the control algos for a few frames in case\n> > > * they are \"unreliable\".\n> > > */\n> > > - prepareISP(data.embeddedbufferId);\n> > > + prepareISP(data.embeddedBufferId);\n> > > frameCount_++;\n> > >\n> > > /* Ready to push the input buffer into the ISP. */\n> > > - runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);\n> > > + runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);\n> > > }\n> > >\n> > > void IPARPi::reportMetadata()\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..8770ae66a21a 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> > > @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const\n> > CameraConfiguration *config)\n> > > void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const\n> > ControlList &controls)\n> > > {\n> > > if (state_ == State::Stopped)\n> > > - handleState();\n> > > + return;\n> >\n> > I thought handleState() still needs to be called in this (and below) case?\n> >\n> >\n> > I thought so too :)\n> >\n> > But if you have a look at handleState(), in the case State::Stopped, it simply\n> > breaks and returns doing nothing.\n> > So, replacing by a single return is sufficient. Clearly it did do something in\n> > the past, but we should be ok without\n> > calling handleState now.\n>\n> Ah, I see. I just saw your reply to v1 too :)\n>\n> Well that's nice, it makes everything cleaner.\n>\n> Reviewed-by: Paul Elder \n>\n>\n> >\n> > >\n> > > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> > >\n> > > @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t\n> > bufferId, const ControlList &\n> > > void RPiCameraData::runIsp(uint32_t bufferId)\n> > > {\n> > > if (state_ == State::Stopped)\n> > > - handleState();\n> > > + return;\n> > >\n> > > FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at\n> > (bufferId);\n> > >\n> > > @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)\n> > > void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> > > {\n> > > if (state_ == State::Stopped)\n> > > - handleState();\n> > > + return;\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> > > @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()\n> > > << \" Embedded buffer id: \" << embeddedId;\n> > >\n> > > ipa::rpi::ISPConfig ispPrepare;\n> > > - ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData |\n> > embeddedId;\n> > > - ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;\n> > > + ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData |\n> > embeddedId;\n> > > + ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;\n> > > ipa_->signalIspPrepare(ispPrepare);\n> > > }\n> > >\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> _______________________________________________\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 D04ADBD1F6\n\tfor ;\n\tWed, 17 Feb 2021 11:19:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 53C76680E9;\n\tWed, 17 Feb 2021 12:19:10 +0100 (CET)","from mail-oi1-x229.google.com (mail-oi1-x229.google.com\n\t[IPv6:2607:f8b0:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D5FD1637FB\n\tfor ;\n\tWed, 17 Feb 2021 12:19:08 +0100 (CET)","by mail-oi1-x229.google.com with SMTP id f3so14507397oiw.13\n\tfor ;\n\tWed, 17 Feb 2021 03:19:08 -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=\"O5Q2uu5v\"; 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=51QrSpPCY1R5ZXCeIRZaob9H9AIBYE8XstmdnLk9LdY=;\n\tb=O5Q2uu5v5k9jKXMP3oBkYLH4kgUBWMQG6MnjLFiOMY/iOd1wrHa3ALXdyEwjnmwKYW\n\tf/XNvhIT4avKJO0NOk8fwVY+eBZQEWM8505Lc2ixUp9datUO5lYneAmFfc7wWi/AWTYF\n\t0AW7OYWOKwgdNqmPGyxmywbanjQ8eCKrCyPSjg0her4kpEI8D8kziTsOhSS/CRG9zErN\n\txs8pHTGnAuhQs6HHG2wAVur55jzvzr/ECmXpNWBZGqF6/IvXIuPZOjj0/tE7RO9JmeaI\n\t+AuVZ5QM8hIRyLrI+gsGp4qQMtIlLPsFv349M3Pj+kXlpL4cOWA/W3b5kJs8AB0QsSAk\n\tcozw==","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=51QrSpPCY1R5ZXCeIRZaob9H9AIBYE8XstmdnLk9LdY=;\n\tb=VK7gZcPt1HAC8915/c79g2MHvT5cK5uRvE2Zz2No54+mJXMfSkEBTru1pY0ZnDNUJu\n\tFzTxU1b+1yRn/Zk8fNpC9KCqwTpo7UuxWkv2rzodmceegs3SLI1v2gjWEHRcampN++qK\n\t0/BBL1vbIWefGocDLbFH3bIms2vE7aEXnIdwpH38EDuHjrGRSSb6g9TlgJyhc9TzQ0qv\n\t3VUtREBJ2Tj7s1UiFhcLWsNfwkuivtmbZGBEp/9ohPRNAj16l0mJHcWLZccAxcZCIkCK\n\twgsjGleguGAumhrJkGg8mzznWOOmqYFoNUdD5S9l7O3YZDIxCSZIN6o6kYyV+qGu3/QA\n\tIqAA==","X-Gm-Message-State":"AOAM531Q+9m2kq31q9mzESO0DmR6MKmaxl35zmPhty22pqSyZKLQJ5za\n\tk2UkJZUegkVPUSPUnMg31EJ3qiG6sSfebw9A2Y8ruA==","X-Google-Smtp-Source":"ABdhPJwiHbAqaVFKTZkgKzLifJ9F1g92y/FbUmKz5t0o5BrdnU9C8GW6X2ZLj9rdtTdB1lzf/R7jSlR0wVV+yVDgHgI=","X-Received":"by 2002:aca:acd3:: with SMTP id\n\tv202mr5125567oie.107.1613560747575; \n\tWed, 17 Feb 2021 03:19:07 -0800 (PST)","MIME-Version":"1.0","References":"<20210217100852.1542397-1-naush@raspberrypi.com>\n\t<20210217101846.GH17707@pyrite.rasen.tech>\n\t\n\t<20210217102851.GJ17707@pyrite.rasen.tech>","In-Reply-To":"<20210217102851.GJ17707@pyrite.rasen.tech>","From":"David Plowman ","Date":"Wed, 17 Feb 2021 11:18:55 +0000","Message-ID":"","To":"paul.elder@ideasonboard.com","Subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups 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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15209,"web_url":"https://patchwork.libcamera.org/comment/15209/","msgid":"","date":"2021-02-18T10:19:34","subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups 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\nWould I be able to get another review on this commit please? Without this\ncommit, the code on top-of-tree can cause crashes at runtime.\n\nMany thanks,\nNaush\n\n\nOn Wed, 17 Feb 2021 at 10:08, 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> - Switch to camel case for ISPConfig::embeddedbufferId and\n> ISPConfig::bayerbufferId.\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 | 8 ++---\n> src/ipa/raspberrypi/raspberrypi.cpp | 8 ++---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 36 ++++++++++---------\n> 3 files changed, 27 insertions(+), 25 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> b/include/libcamera/ipa/raspberrypi.mojom\n> index bab19a946e18..9c05cc68cceb 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> @@ -27,8 +27,8 @@ struct SensorConfig {\n> };\n>\n> struct ISPConfig {\n> - uint32 embeddedbufferId;\n> - uint32 bayerbufferId;\n> + uint32 embeddedBufferId;\n> + uint32 bayerBufferId;\n> };\n>\n> struct ConfigInput {\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..974f4ec63058 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> @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const\n> ipa::rpi::ISPConfig &data)\n> * avoid running the control algos for a few frames in case\n> * they are \"unreliable\".\n> */\n> - prepareISP(data.embeddedbufferId);\n> + prepareISP(data.embeddedBufferId);\n> frameCount_++;\n>\n> /* Ready to push the input buffer into the ISP. */\n> - runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);\n> + runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);\n> }\n>\n> void IPARPi::reportMetadata()\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..8770ae66a21a 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> @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const\n> ControlList &controls)\n> {\n> if (state_ == State::Stopped)\n> - handleState();\n> + return;\n>\n> FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n>\n> @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t\n> bufferId, const ControlList &\n> void RPiCameraData::runIsp(uint32_t bufferId)\n> {\n> if (state_ == State::Stopped)\n> - handleState();\n> + return;\n>\n> FrameBuffer *buffer =\n> unicam_[Unicam::Image].getBuffers().at(bufferId);\n>\n> @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)\n> void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> {\n> if (state_ == State::Stopped)\n> - handleState();\n> + return;\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> @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()\n> << \" Embedded buffer id: \" << embeddedId;\n>\n> ipa::rpi::ISPConfig ispPrepare;\n> - ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData |\n> embeddedId;\n> - ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;\n> + ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData |\n> embeddedId;\n> + ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;\n> ipa_->signalIspPrepare(ispPrepare);\n> }\n>\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 28A07BD1F1\n\tfor ;\n\tThu, 18 Feb 2021 10:19:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C998680E9;\n\tThu, 18 Feb 2021 11:19:52 +0100 (CET)","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 87B41637BF\n\tfor ;\n\tThu, 18 Feb 2021 11:19:51 +0100 (CET)","by mail-lj1-x235.google.com with SMTP id a22so3436427ljp.10\n\tfor ;\n\tThu, 18 Feb 2021 02:19:51 -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=\"tGitRPZA\"; 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=jkxcvxuQgXfHxJ/FWXbwodNsDUq5/ZFjLqWWcrBychc=;\n\tb=tGitRPZA77CySEA0SVKDbqBMS8rMi9dzsztsb8v7N3+ATurCrTzLPdVxI8VgBd6DGe\n\t0Fw/70Aq8hMba23fL6YE9Z3kivPKPWLghThyGJonANtijdcONlbnVMV3J3Slx8vioOKR\n\tr6QUgGKgGeV7XFQpVZitE8SonLevzjx1h/3iPFpn9/Tu7u7dHQZH8Lm/oa+DAMLuDe0P\n\th8DY9g0fgZHAea3hdHaw4elZTFP751YyxLmh1jAHQEmAj80wPYZZ/C4QW1zrMokjX3Su\n\ta7CIyyhqD33MVgYMAm6QzRIvBksvz9CjlxhMNorSUP3kOWZs64puetykSw/Ft+URP4OD\n\tVTeg==","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=jkxcvxuQgXfHxJ/FWXbwodNsDUq5/ZFjLqWWcrBychc=;\n\tb=Mq1neBi4XOKImx5vv9se3lNoaKMPsiThvs/DoakobGJnYp8rcoDXtg4g/quO7L/CyZ\n\tNNHM3p2ExwqG7xFg0q2kANrE19+wY8dGpxNxGIvyBtbT4LN0hOEkg+ksPXDAA5+AU5mv\n\tE/JTAcfBNxgKf+11Rceu+/NBeDoF+xm4uYSO353FocU36J1zvgnKCVVFiPuWk64qenB5\n\tO5lXD6WR2slzsmTAA/T32ZudJWeQ5dxu4qebYVxHR0JVjx+OfyVG1Wlh8c/aqv/F4KB1\n\tD20umW1D8fckPVW6J/pROmyW6i1yUZ5OJqw/LA4It+U6483Z1KmpTy6BVnicXK2Aezac\n\tsw6w==","X-Gm-Message-State":"AOAM530XzFV6EtUCgH+W8amHIBXPHIfGq025zUMPdLMut3KlpFEtF0hO\n\towwWQaj/LMXIswMJg7P1FgpnvJ2v8jWVNR8v0u0u2IWFyv7v7w==","X-Google-Smtp-Source":"ABdhPJyHhbB2As5a3uxN6eT8ibchnE1uLegC+8LiCeB6MwQZDqu1cQu6kVXl149JZccQVdM68dYHjtlT8po59dBtyNk=","X-Received":"by 2002:a05:651c:236:: with SMTP id\n\tz22mr2275137ljn.169.1613643590229; \n\tThu, 18 Feb 2021 02:19:50 -0800 (PST)","MIME-Version":"1.0","References":"<20210217100852.1542397-1-naush@raspberrypi.com>","In-Reply-To":"<20210217100852.1542397-1-naush@raspberrypi.com>","From":"Naushir Patuck ","Date":"Thu, 18 Feb 2021 10:19:34 +0000","Message-ID":"","To":"libcamera devel ","Subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups 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=\"===============8152489304028557848==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15214,"web_url":"https://patchwork.libcamera.org/comment/15214/","msgid":"<3b63bd97-1c6c-0814-3c69-b8b798aeeee9@ideasonboard.com>","date":"2021-02-18T11:25:48","subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups after IPAInterface changes","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 17/02/2021 10:08, 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> - Switch to camel case for ISPConfig::embeddedbufferId and\n> ISPConfig::bayerbufferId.\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\nThat's quite a lot of change for a single patch, but I don't think\nthere's a point for splitting this out now, especially as it's required\nto fix a crash.\n\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 | 8 ++---\n> src/ipa/raspberrypi/raspberrypi.cpp | 8 ++---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 36 ++++++++++---------\n> 3 files changed, 27 insertions(+), 25 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> index bab19a946e18..9c05cc68cceb 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> @@ -27,8 +27,8 @@ struct SensorConfig {\n> };\n> \n> struct ISPConfig {\n> -\tuint32 embeddedbufferId;\n> -\tuint32 bayerbufferId;\n> +\tuint32 embeddedBufferId;\n> +\tuint32 bayerBufferId;\n> };\n> \n> struct ConfigInput {\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..974f4ec63058 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> @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const ipa::rpi::ISPConfig &data)\n> \t * avoid running the control algos for a few frames in case\n> \t * they are \"unreliable\".\n> \t */\n> -\tprepareISP(data.embeddedbufferId);\n> +\tprepareISP(data.embeddedBufferId);\n> \tframeCount_++;\n> \n> \t/* Ready to push the input buffer into the ISP. */\n> -\trunIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);\n> +\trunIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);\n> }\n> \n> void IPARPi::reportMetadata()\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..8770ae66a21a 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> @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)\n> {\n> \tif (state_ == State::Stopped)\n> -\t\thandleState();\n> +\t\treturn;\n> \n> \tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> \n> @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n> void RPiCameraData::runIsp(uint32_t bufferId)\n> {\n> \tif (state_ == State::Stopped)\n> -\t\thandleState();\n> +\t\treturn;\n> \n> \tFrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> \n> @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)\n> void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> {\n> \tif (state_ == State::Stopped)\n> -\t\thandleState();\n> +\t\treturn;\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> +\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> +\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\nI assume it's this hunk which fixes the bug?\nEverything else seems to be a rename or the handleState changes perhaps\nare a no-effect change?\n\nAs this is a bug fix, it would have been nice to see that split so the\nreader can understand what code change actually causes the fix.\n\nThe renames are trivial, so they're fine, and this seesm to make sense\nas it protects against accessing a control which isn't set.\n\nReviewed-by: Kieran Bingham \n\n\n\n> \n> \tisp_[Isp::Input].dev()->setControls(&ctrls);\n> \thandleState();\n> @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()\n> \t\t\t<< \" Embedded buffer id: \" << embeddedId;\n> \n> \tipa::rpi::ISPConfig ispPrepare;\n> -\tispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData | embeddedId;\n> -\tispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;\n> +\tispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData | embeddedId;\n> +\tispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;\n> \tipa_->signalIspPrepare(ispPrepare);\n> }\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 58005BD1F6\n\tfor ;\n\tThu, 18 Feb 2021 11:25:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6A9568110;\n\tThu, 18 Feb 2021 12:25:53 +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 DD8AF637DA\n\tfor ;\n\tThu, 18 Feb 2021 12:25:52 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5DCA63E7;\n\tThu, 18 Feb 2021 12:25:52 +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=\"WfNqIixO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613647552;\n\tbh=rBMAR0P7sdywaKv6Xc7TwopgJDpFxgMaIGdC7gwaMEc=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=WfNqIixOcq33U7wePc9tnePQRRy3FcbJagHjyPexkOpOGNt6Gyavoq4a2j7pAPBpL\n\tkrw8g8CiRUstG0+7fm8YdTnDDjln48a3601f5s/zEWHu1qUkYiH1lZR28bWXHvl5nY\n\tP1JRkIhljRrtBWoaqThlZueHU1MfueZ6qaS2WkWM=","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210217100852.1542397-1-naush@raspberrypi.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<3b63bd97-1c6c-0814-3c69-b8b798aeeee9@ideasonboard.com>","Date":"Thu, 18 Feb 2021 11:25:48 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20210217100852.1542397-1-naush@raspberrypi.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups 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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15216,"web_url":"https://patchwork.libcamera.org/comment/15216/","msgid":"","date":"2021-02-18T11:35:00","subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups after IPAInterface changes","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nOn Thu, 18 Feb 2021 at 11:25, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On 17/02/2021 10:08, 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> > - Switch to camel case for ISPConfig::embeddedbufferId and\n> > ISPConfig::bayerbufferId.\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>\n> That's quite a lot of change for a single patch, but I don't think\n> there's a point for splitting this out now, especially as it's required\n> to fix a crash.\n>\n\nI do agree. If you prefer, I could split this into code changes (for\nfixing the crash) and the other cosmetic changes.\n\n\n\n>\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 | 8 ++---\n> > src/ipa/raspberrypi/raspberrypi.cpp | 8 ++---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 36 ++++++++++---------\n> > 3 files changed, 27 insertions(+), 25 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> b/include/libcamera/ipa/raspberrypi.mojom\n> > index bab19a946e18..9c05cc68cceb 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> > @@ -27,8 +27,8 @@ struct SensorConfig {\n> > };\n> >\n> > struct ISPConfig {\n> > - uint32 embeddedbufferId;\n> > - uint32 bayerbufferId;\n> > + uint32 embeddedBufferId;\n> > + uint32 bayerBufferId;\n> > };\n> >\n> > struct ConfigInput {\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..974f4ec63058 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> > @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const\n> ipa::rpi::ISPConfig &data)\n> > * avoid running the control algos for a few frames in case\n> > * they are \"unreliable\".\n> > */\n> > - prepareISP(data.embeddedbufferId);\n> > + prepareISP(data.embeddedBufferId);\n> > frameCount_++;\n> >\n> > /* Ready to push the input buffer into the ISP. */\n> > - runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);\n> > + runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);\n> > }\n> >\n> > void IPARPi::reportMetadata()\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..8770ae66a21a 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> > @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> > void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const\n> ControlList &controls)\n> > {\n> > if (state_ == State::Stopped)\n> > - handleState();\n> > + return;\n> >\n> > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> >\n> > @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t\n> bufferId, const ControlList &\n> > void RPiCameraData::runIsp(uint32_t bufferId)\n> > {\n> > if (state_ == State::Stopped)\n> > - handleState();\n> > + return;\n> >\n> > FrameBuffer *buffer =\n> unicam_[Unicam::Image].getBuffers().at(bufferId);\n> >\n> > @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)\n> > void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> > {\n> > if (state_ == State::Stopped)\n> > - handleState();\n> > + return;\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> I assume it's this hunk which fixes the bug?\n> Everything else seems to be a rename or the handleState changes perhaps\n> are a no-effect change?\n>\n\nActually not this one, but the hunks just above that fix the bug. Let me\nsplit this change up and I will resubmit a v3. If it's ok, I will add your\nR-b tags to them.\n\nRegards,\nNaush\n\n\n\n>\n> As this is a bug fix, it would have been nice to see that split so the\n> reader can understand what code change actually causes the fix.\n>\n> The renames are trivial, so they're fine, and this seesm to make sense\n> as it protects against accessing a control which isn't set.\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n>\n> >\n> > isp_[Isp::Input].dev()->setControls(&ctrls);\n> > handleState();\n> > @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()\n> > << \" Embedded buffer id: \" << embeddedId;\n> >\n> > ipa::rpi::ISPConfig ispPrepare;\n> > - ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData |\n> embeddedId;\n> > - ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;\n> > + ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData |\n> embeddedId;\n> > + ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;\n> > ipa_->signalIspPrepare(ispPrepare);\n> > }\n> >\n> >\n>\n> --\n> Regards\n> --\n> Kieran\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 76DD9BD1F1\n\tfor ;\n\tThu, 18 Feb 2021 11:35:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 434106836F;\n\tThu, 18 Feb 2021 12:35:19 +0100 (CET)","from mail-qt1-x82d.google.com (mail-qt1-x82d.google.com\n\t[IPv6:2607:f8b0:4864:20::82d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD279637DA\n\tfor ;\n\tThu, 18 Feb 2021 12:35:17 +0100 (CET)","by mail-qt1-x82d.google.com with SMTP id b24so1061720qtp.13\n\tfor ;\n\tThu, 18 Feb 2021 03:35:17 -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=\"LNH0N1rc\"; 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=qaT3G3+4f6SiWV6ETc+qM/Mn6ZA9eaSnFm/dPUY26rw=;\n\tb=LNH0N1rcMDBhw3z2iFnqmGIFDB6chpQIbsZcSxgksCG85MXK5DwhIH4mr0ZUgRBqRA\n\taB6p7gXubscZAi05mZluEGpLfoWuozeogOCPV7BtKdCheJ2Zi2836U0KUEJZzrGZc0Hl\n\tCNtGx6/Kle9RpqtY4oEBbuVEZSz1J7fp1J2BbWLvm6JsvVaYSxiUdRv573egYcBR8Nq5\n\t1gEJFm0LygilrouPrzkxvZUAdRrUkwKK/xJz7duKTC3b3JOOf2yx8djPLJjVUl8oz0L9\n\tqBG4xO3/oozJ2YZPzDTznesuDkFvqr+f04L+WemnKu50EeFhPLhLazT3LVFyGwuRgYAZ\n\t6CPQ==","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=qaT3G3+4f6SiWV6ETc+qM/Mn6ZA9eaSnFm/dPUY26rw=;\n\tb=Ra0MwjC22xV+A0M06VEoRGERSQCCc3SCWCrz1SxiChzzEHpLmb7ZLh94WLbHYYNW3n\n\tEGZ76AJBs5Hfpwr+pJ6oB9pELCRsTTq8TBiwc/W0+E8nhjJojYcKz/3do19FgyQh2a0Y\n\tqMbnfo/djT1oNTqCr0qKKFnpyYlzlaajT8vD3RtObI6V9Ed1Mkzrd6P94TH/wLVHvbzs\n\tf/aYsdHPdYD/gHaRvAGO6AXhQC6yEME4Hr6MtEDRvNo6ACgFF1HF0PCiNvQHLkw6TGyB\n\tyeCwD2IFn7EDhCrrzPwZrOGN9XnFQo72DxAPX4vISRWRkZbfo1zwGDe6lgtR/GiAZhMb\n\tnUaA==","X-Gm-Message-State":"AOAM530FgfdhIOXPSMdebKq/LucEv2Ver7VDcoQmewdbjwQem/24Ivl/\n\tv9xLtq0TFqk4T8IO8P22UW3n26otEa3bBm4kXu0qvd0YJ4fj+w==","X-Google-Smtp-Source":"ABdhPJzc59LWVO1vCoc9/USWkF5trpgSHU7aPCmKfwWX3iOQP6VaUWUnwDHplLfE8QC/EGtchwIDB3p3y0jka+XrFQg=","X-Received":"by 2002:a05:622a:514:: with SMTP id\n\tl20mr3805457qtx.62.1613648116613; \n\tThu, 18 Feb 2021 03:35:16 -0800 (PST)","MIME-Version":"1.0","References":"<20210217100852.1542397-1-naush@raspberrypi.com>\n\t<3b63bd97-1c6c-0814-3c69-b8b798aeeee9@ideasonboard.com>","In-Reply-To":"<3b63bd97-1c6c-0814-3c69-b8b798aeeee9@ideasonboard.com>","From":"Naushir Patuck ","Date":"Thu, 18 Feb 2021 11:35:00 +0000","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups 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=\"===============0838730771646146762==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15218,"web_url":"https://patchwork.libcamera.org/comment/15218/","msgid":"","date":"2021-02-18T11:38:32","subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups after IPAInterface changes","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 18/02/2021 11:35, Naushir Patuck wrote:\n> Hi Kieran,\n> \n> On Thu, 18 Feb 2021 at 11:25, Kieran Bingham\n> > wrote:\n> \n> Hi Naush,\n> \n> On 17/02/2021 10:08, 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> > - Switch to camel case for ISPConfig::embeddedbufferId and\n> > ISPConfig::bayerbufferId.\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)\n> when a LS\n> > control is present.\n> \n> \n> That's quite a lot of change for a single patch, but I don't think\n> there's a point for splitting this out now, especially as it's required\n> to fix a crash.\n> \n> \n> I do agree.  If you prefer, I could split this into code changes (for\n> fixing the crash) and the other cosmetic changes.\n> \n>  \n> \n> \n> \n> > Signed-off-by: Naushir Patuck >\n> > Fixes: e201cb4f5450 (\"libcamera: IPAInterface: Replace C API with\n> the new C++-only API\")\n> > ---\n> >  include/libcamera/ipa/raspberrypi.mojom       |  8 ++---\n> >  src/ipa/raspberrypi/raspberrypi.cpp           |  8 ++---\n> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 36\n> ++++++++++---------\n> >  3 files changed, 27 insertions(+), 25 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> b/include/libcamera/ipa/raspberrypi.mojom\n> > index bab19a946e18..9c05cc68cceb 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> > @@ -27,8 +27,8 @@ struct SensorConfig {\n> >  };\n> > \n> >  struct ISPConfig {\n> > -     uint32 embeddedbufferId;\n> > -     uint32 bayerbufferId;\n> > +     uint32 embeddedBufferId;\n> > +     uint32 bayerBufferId;\n> >  };\n> > \n> >  struct ConfigInput {\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..974f4ec63058 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> > @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const\n> ipa::rpi::ISPConfig &data)\n> >        * avoid running the control algos for a few frames in case\n> >        * they are \"unreliable\".\n> >        */\n> > -     prepareISP(data.embeddedbufferId);\n> > +     prepareISP(data.embeddedBufferId);\n> >       frameCount_++;\n> > \n> >       /* Ready to push the input buffer into the ISP. */\n> > -     runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);\n> > +     runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);\n> >  }\n> > \n> >  void IPARPi::reportMetadata()\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..8770ae66a21a 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 &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() + \".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> > @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> >  void RPiCameraData::statsMetadataComplete(uint32_t bufferId,\n> const ControlList &controls)\n> >  {\n> >       if (state_ == State::Stopped)\n> > -             handleState();\n> > +             return;\n> > \n> >       FrameBuffer *buffer =\n> isp_[Isp::Stats].getBuffers().at(bufferId);\n> > \n> > @@ -1314,7 +1314,7 @@ void\n> RPiCameraData::statsMetadataComplete(uint32_t bufferId, const\n> ControlList &\n> >  void RPiCameraData::runIsp(uint32_t bufferId)\n> >  {\n> >       if (state_ == State::Stopped)\n> > -             handleState();\n> > +             return;\n> > \n> >       FrameBuffer *buffer =\n> unicam_[Unicam::Image].getBuffers().at(bufferId);\n> > \n> > @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)\n> >  void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> >  {\n> >       if (state_ == State::Stopped)\n> > -             handleState();\n> > +             return;\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> > -           \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 bcm2835_isp_lens_shading *>(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> I assume it's this hunk which fixes the bug?\n> Everything else seems to be a rename or the handleState changes perhaps\n> are a no-effect change?\n> \n> \n> Actually not this one, but the hunks just above that fix the bug.  Let\n> me split this change up and I will resubmit a v3.  If it's ok, I will\n> add your R-b tags to them.\n> \n\nAhaha, great, I messed up, so yes - splitting at least functional\nchanges from cosmetic would be helpful.\n\nYes, please keep the tag though.\n\n--\nKieran\n\n\n> Regards,\n> Naush\n> \n>  \n> \n> \n> As this is a bug fix, it would have been nice to see that split so the\n> reader can understand what code change actually causes the fix.\n> \n> The renames are trivial, so they're fine, and this seesm to make sense\n> as it protects against accessing a control which isn't set.\n> \n> Reviewed-by: Kieran Bingham >\n> \n> \n> \n> > \n> >       isp_[Isp::Input].dev()->setControls(&ctrls);\n> >       handleState();\n> > @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()\n> >                       << \" Embedded buffer id: \" << embeddedId;\n> > \n> >       ipa::rpi::ISPConfig ispPrepare;\n> > -     ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData |\n> embeddedId;\n> > -     ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;\n> > +     ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData |\n> embeddedId;\n> > +     ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;\n> >       ipa_->signalIspPrepare(ispPrepare);\n> >  }\n> > \n> >\n> \n> -- \n> Regards\n> --\n> Kieran\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 2FED3BD1F6\n\tfor ;\n\tThu, 18 Feb 2021 11:38:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A9496836F;\n\tThu, 18 Feb 2021 12:38:37 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 46EDA68110\n\tfor ;\n\tThu, 18 Feb 2021 12:38:36 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C5CE43E7;\n\tThu, 18 Feb 2021 12:38:35 +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=\"it9+wGkh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613648316;\n\tbh=Orws8lrI3LNGy5FMw7BJkzoSFFEqYrGaoGBJWESSKU0=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=it9+wGkh4j9Iig+8id6RRxjcPVJf6jxGuJyuOon42vSMT2PMRtMCZRd2OB0TkqVCl\n\tjnR+LsntAApDbl0hf+qjnLV2jquclY6NGNl9dV2JBXRMOcEgNe/2LKuSOooz++ng+h\n\tZuzOq58jmnwUZ/CeG6EmK/FVrZFG9k67EnK2pG+w=","To":"Naushir Patuck ","References":"<20210217100852.1542397-1-naush@raspberrypi.com>\n\t<3b63bd97-1c6c-0814-3c69-b8b798aeeee9@ideasonboard.com>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Thu, 18 Feb 2021 11:38:32 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi:\n\tVarious fixups 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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]