[{"id":38966,"web_url":"https://patchwork.libcamera.org/comment/38966/","msgid":"","date":"2026-05-28T10:04:34","subject":"Re: [PATCH v2 14/32] pipeline: rkisp1: Apply initial controls","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Wed, Mar 25, 2026 at 04:13:46PM +0100, Stefan Klug wrote:\n> Controls passed to Camera::start() are not handled at the moment.\n> Implement that by initializing a separate frame\n> context and then returning the controls synchronously to the caller. In\n\nplease reflow the above lines\n\n> the pipeline the controls get passed to the sensor before the call to\n> streamOn() to ensure the controls are applied right away. Passing the\n> controls to the pipeline using the setSensorControls signal is not\n> appropriate because it is asynchronous and would reach the sensor too\n> late (initial controls need to be applied before the sensor starts and\n> before delayed controls initializes it's start condition.)\n\nAm I correct this only support manual controls ?\n\nAs I understand it we emit 3 sensor controls from the RkISP1 pipeline\n(EXPOSURE, VBLANK, ANALOGUE_GAIN) and these are populated by\ngetSensorControls() with the values from the FrameContext.\n\nAs you call initializeFrameContext() which runs algo->queueRequest(),\ngains and exposure and frame duration are only populated by the Agc if\ncontrols are in manual mode:\n\n\tif (!frameContext.agc.autoExposureEnabled)\n\t\tframeContext.agc.exposure = agc.manual.exposure;\n\tif (!frameContext.agc.autoGainEnabled)\n\t\tframeContext.agc.gain = agc.manual.gain;\n\n(vblank seems not handled. I presume it's properly initialized ?)\n\nis it worth clarifying this only apply to manual controls, or is it\nobvious and it's just me having realized it now ?\n\n>\n> Signed-off-by: Stefan Klug \n> ---\n> include/libcamera/ipa/rkisp1.mojom | 7 ++++++-\n> src/ipa/rkisp1/rkisp1.cpp | 10 ++++++----\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 11 +++++++++--\n> 3 files changed, 21 insertions(+), 7 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index 068e898848c4..4c29b53cd7f9 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -14,13 +14,18 @@ struct IPAConfigInfo {\n> \tuint32 paramFormat;\n> };\n>\n> +struct StartResult {\n> +\tlibcamera.ControlList controls;\n> +\tint32 code;\n> +};\n> +\n> interface IPARkISP1Interface {\n> \tinit(libcamera.IPASettings settings,\n> \t uint32 hwRevision, uint32 supportedBlocks,\n> \t libcamera.IPACameraSensorInfo sensorInfo,\n> \t libcamera.ControlInfoMap sensorControls)\n> \t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> -\tstart() => (int32 ret);\n> +\tstart(libcamera.ControlList controls) => (StartResult result);\n> \tstop();\n>\n> \tconfigure(IPAConfigInfo configInfo,\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 98ed4a8ff16d..3f943a08d011 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -56,7 +56,7 @@ public:\n> \t\t const IPACameraSensorInfo &sensorInfo,\n> \t\t const ControlInfoMap &sensorControls,\n> \t\t ControlInfoMap *ipaControls) override;\n> -\tint start() override;\n> +\tvoid start(const ControlList &controls, StartResult *result) override;\n> \tvoid stop() override;\n>\n> \tint configure(const IPAConfigInfo &ipaConfig,\n> @@ -215,10 +215,12 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> \treturn 0;\n> }\n>\n> -int IPARkISP1::start()\n> +void IPARkISP1::start(const ControlList &controls, StartResult *result)\n> {\n> -\t/* \\todo Properly handle startup controls. */\n> -\treturn 0;\n> +\tIPAFrameContext frameContext = {};\n> +\tinitializeFrameContext(0, frameContext, controls);\n> +\tresult->controls = getSensorControls(frameContext);\n> +\tresult->code = 0;\n> }\n>\n> void IPARkISP1::stop()\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 7352237dbb86..89e8ab0999f4 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1238,13 +1238,20 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> \t\treturn ret;\n> \tactions += [&]() { freeBuffers(camera); };\n>\n> -\tret = data->ipa_->start();\n> -\tif (ret) {\n> +\tControlList ctrls;\n> +\tif (!!controls)\n\nDo you need !! ?\n\n> +\t\tctrls = *controls;\n\nWhy a copy ?\n\n> +\n> +\tipa::rkisp1::StartResult res;\n> +\tdata->ipa_->start(ctrls, &res);\n\nI see rpi passing in an empty control list in case !controls\n\n\tdata->ipa_->start(controls ? *controls : ControlList{ controls::controls },\n\t\t\t &result);\n\nCan't we do the same ?\n\n> +\tif (res.code) {\n\nWhat are the possibile error conditions ?\nI don't see IPA::start() ever initalizing it to anything != 0\n\n> \t\tLOG(RkISP1, Error)\n> \t\t\t<< \"Failed to start IPA \" << camera->id();\n> \t\treturn ret;\n> \t}\n> \tactions += [&]() { data->ipa_->stop(); };\n> +\tdata->sensor_->setControls(&res.controls);\n> +\tdata->delayedCtrls_->reset();\n>\n> \tdata->frame_ = 0;\n>\n> --\n> 2.51.0\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 32361C328C\n\tfor ;\n\tThu, 28 May 2026 10:04:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1CAB063024;\n\tThu, 28 May 2026 12:04:42 +0200 (CEST)","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 3E0956175A\n\tfor ;\n\tThu, 28 May 2026 12:04:40 +0200 (CEST)","from ideasonboard.com (unknown [91.80.69.123])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CF50157E;\n\tThu, 28 May 2026 12:04:19 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"I3RzzNVq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1779962660;\n\tbh=lqqVA0aW4DGCIhLnubVgOfHo2UTk72P8m2yYse9KeJk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=I3RzzNVq1UhhNlxyMZg0L4ry011FdgczpiPeSWDuurExjeICefUmePo8FGcqmitXC\n\tme+RmuPHsiahuH17fF88tJk+MIxxIyuataxlRuKWuDEkQ231cpAcZUtqlHnT/H2yEX\n\t4R4ivsbxNhkPN0Hn88mysAp6RQOFBbwNDkc4hVbY=","Date":"Thu, 28 May 2026 12:04:34 +0200","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 14/32] pipeline: rkisp1: Apply initial controls","Message-ID":"","References":"<20260325151416.2114564-1-stefan.klug@ideasonboard.com>\n\t<20260325151416.2114564-15-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20260325151416.2114564-15-stefan.klug@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":38970,"web_url":"https://patchwork.libcamera.org/comment/38970/","msgid":"<177997585911.8301.3523814267982861651@localhost>","date":"2026-05-28T13:44:19","subject":"Re: [PATCH v2 14/32] pipeline: rkisp1: Apply initial controls","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nQuoting Jacopo Mondi (2026-05-28 12:04:34)\n> Hi Stefan\n> \n> On Wed, Mar 25, 2026 at 04:13:46PM +0100, Stefan Klug wrote:\n> > Controls passed to Camera::start() are not handled at the moment.\n> > Implement that by initializing a separate frame\n> > context and then returning the controls synchronously to the caller. In\n> \n> please reflow the above lines\n> \n> > the pipeline the controls get passed to the sensor before the call to\n> > streamOn() to ensure the controls are applied right away. Passing the\n> > controls to the pipeline using the setSensorControls signal is not\n> > appropriate because it is asynchronous and would reach the sensor too\n> > late (initial controls need to be applied before the sensor starts and\n> > before delayed controls initializes it's start condition.)\n> \n> Am I correct this only support manual controls ?\n> \n> As I understand it we emit 3 sensor controls from the RkISP1 pipeline\n> (EXPOSURE, VBLANK, ANALOGUE_GAIN) and these are populated by\n> getSensorControls() with the values from the FrameContext.\n> \n> As you call initializeFrameContext() which runs algo->queueRequest(),\n> gains and exposure and frame duration are only populated by the Agc if\n> controls are in manual mode:\n> \n> if (!frameContext.agc.autoExposureEnabled)\n> frameContext.agc.exposure = agc.manual.exposure;\n> if (!frameContext.agc.autoGainEnabled)\n> frameContext.agc.gain = agc.manual.gain;\n> \n> (vblank seems not handled. I presume it's properly initialized ?)\n\nYes, you're righht. This is fixed in [PATCH v2 16/32] ipa: rkisp1: agc:\nProcess frame duration at the right time.\n\n> \n> is it worth clarifying this only apply to manual controls, or is it\n> obvious and it's just me having realized it now ?\n\nYes, I might need to reorder the patches here. Setting these in auto\nmode also is implemented in the next patch \"[PATCH v2 15/32] ipa:\nrkisp1: Set frameContext.agc in queueRequest for auto mode also\"\n\nBest regards,\nStefan\n\n\n> \n> >\n> > Signed-off-by: Stefan Klug \n> > ---\n> > include/libcamera/ipa/rkisp1.mojom | 7 ++++++-\n> > src/ipa/rkisp1/rkisp1.cpp | 10 ++++++----\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 11 +++++++++--\n> > 3 files changed, 21 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > index 068e898848c4..4c29b53cd7f9 100644\n> > --- a/include/libcamera/ipa/rkisp1.mojom\n> > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > @@ -14,13 +14,18 @@ struct IPAConfigInfo {\n> > uint32 paramFormat;\n> > };\n> >\n> > +struct StartResult {\n> > + libcamera.ControlList controls;\n> > + int32 code;\n> > +};\n> > +\n> > interface IPARkISP1Interface {\n> > init(libcamera.IPASettings settings,\n> > uint32 hwRevision, uint32 supportedBlocks,\n> > libcamera.IPACameraSensorInfo sensorInfo,\n> > libcamera.ControlInfoMap sensorControls)\n> > => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > - start() => (int32 ret);\n> > + start(libcamera.ControlList controls) => (StartResult result);\n> > stop();\n> >\n> > configure(IPAConfigInfo configInfo,\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 98ed4a8ff16d..3f943a08d011 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -56,7 +56,7 @@ public:\n> > const IPACameraSensorInfo &sensorInfo,\n> > const ControlInfoMap &sensorControls,\n> > ControlInfoMap *ipaControls) override;\n> > - int start() override;\n> > + void start(const ControlList &controls, StartResult *result) override;\n> > void stop() override;\n> >\n> > int configure(const IPAConfigInfo &ipaConfig,\n> > @@ -215,10 +215,12 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > return 0;\n> > }\n> >\n> > -int IPARkISP1::start()\n> > +void IPARkISP1::start(const ControlList &controls, StartResult *result)\n> > {\n> > - /* \\todo Properly handle startup controls. */\n> > - return 0;\n> > + IPAFrameContext frameContext = {};\n> > + initializeFrameContext(0, frameContext, controls);\n> > + result->controls = getSensorControls(frameContext);\n> > + result->code = 0;\n> > }\n> >\n> > void IPARkISP1::stop()\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 7352237dbb86..89e8ab0999f4 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -1238,13 +1238,20 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > return ret;\n> > actions += [&]() { freeBuffers(camera); };\n> >\n> > - ret = data->ipa_->start();\n> > - if (ret) {\n> > + ControlList ctrls;\n> > + if (!!controls)\n> \n> Do you need !! ?\n> \n> > + ctrls = *controls;\n> \n> Why a copy ?\n> \n> > +\n> > + ipa::rkisp1::StartResult res;\n> > + data->ipa_->start(ctrls, &res);\n> \n> I see rpi passing in an empty control list in case !controls\n> \n> data->ipa_->start(controls ? *controls : ControlList{ controls::controls },\n> &result);\n> \n> Can't we do the same ?\n> \n> > + if (res.code) {\n> \n> What are the possibile error conditions ?\n> I don't see IPA::start() ever initalizing it to anything != 0\n> \n> > LOG(RkISP1, Error)\n> > << \"Failed to start IPA \" << camera->id();\n> > return ret;\n> > }\n> > actions += [&]() { data->ipa_->stop(); };\n> > + data->sensor_->setControls(&res.controls);\n> > + data->delayedCtrls_->reset();\n> >\n> > data->frame_ = 0;\n> >\n> > --\n> > 2.51.0\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 80D29BDCBC\n\tfor ;\n\tThu, 28 May 2026 13:44:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C80562FEC;\n\tThu, 28 May 2026 15:44:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A73A76175A\n\tfor ;\n\tThu, 28 May 2026 15:44:23 +0200 (CEST)","from ideasonboard.com (tmo-086-128.customers.d1-online.com\n\t[80.187.86.128])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id E47AF8E0;\n\tThu, 28 May 2026 15:44:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"NOT8BArE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1779975844;\n\tbh=5Y0skuapSE84UBBTeYMwigBQcXeHF1JPZnELZfP5uTw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=NOT8BArEXmo+Y9anhvuadiecaH4NP+0j9ZDW83nGBKA3kbwN+c9C4VOJM/MZQ+vxM\n\t7TUg9tfyWjfmuFawlrHoZhNkSa9AiSEkW4NC1U9Ejpjk8vH+M7ybF7ZMTBuYs6/2Pg\n\t8+BbtgLYFNPOZZrxY2YmY27s/fj2Kgf0EMpwIy1M=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20260325151416.2114564-1-stefan.klug@ideasonboard.com>\n\t<20260325151416.2114564-15-stefan.klug@ideasonboard.com>\n\t","Subject":"Re: [PATCH v2 14/32] pipeline: rkisp1: Apply initial controls","From":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","To":"Jacopo Mondi ","Date":"Thu, 28 May 2026 15:44:19 +0200","Message-ID":"<177997585911.8301.3523814267982861651@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]