[{"id":36742,"web_url":"https://patchwork.libcamera.org/comment/36742/","msgid":"","date":"2025-11-06T18:37:39","subject":"Re: [PATCH v1 14/35] 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 Fri, Oct 24, 2025 at 10:50:38AM +0200, 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> 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> 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 a55bac260026..7a7b7682e242 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\nShouldn't this go through the frame context queue alloc() function ?\n\nOr as we have no real frame context associated with start, we\nshouldn't allocate one ?\n\nOr is it fine to allocate a frame context for frame #0 as the sensor's\nsequence starts from #1 ?\n\n\n> +\tinitializeFrameContext(0, frameContext, controls);\n\nWhat would really be nice would be for the frame context queue to be\nable to call in the IPA.\n\nThe frame context queue already has logic in place to call\nFrameContext::init() at the right time according to the condition\nwhere get() and alloc() are called.\n\nIt seems that with the introduction of initializeFrameContext() you're\nreplicating part of that in the IPA (an indication of that is that the\nbase struct FrameContext has an initalised_ member which you have now\nduplicated in RkISP1FrameContext).\n\nIn\nipa: libipa: Reduce log level when obtaining an uninitialized frame context\n\nyou have demoted the Warning message when a get() is called before an\nalloc(). This is a sign that possibily the two functions are now\nequally valid as receiving a computeParams() or processStats() call\nbefore a queueRequest() is a valid use case ?\n\nMore on this on\nipa: rkisp1: Lazy initialise frame context\n\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 0e6e45bba75b..d937c94e351f 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1290,13 +1290,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> +\t\tctrls = *controls;\n> +\n> +\tipa::rkisp1::StartResult res;\n> +\tdata->ipa_->start(ctrls, &res);\n> +\tif (res.code) {\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.48.1\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 A6A43BDE4C\n\tfor ;\n\tThu, 6 Nov 2025 18:37:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ADC02606E6;\n\tThu, 6 Nov 2025 19:37:45 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 66ACA606E6\n\tfor ;\n\tThu, 6 Nov 2025 19:37:43 +0100 (CET)","from ideasonboard.com (mob-5-90-141-71.net.vodafone.it\n\t[5.90.141.71])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EB3146AF;\n\tThu, 6 Nov 2025 19:35:47 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SFIeDe6G\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762454148;\n\tbh=F7JqLL9oqD5MqeuTHVcQOiDqPYVXWFfNIfcXcBW55H4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SFIeDe6GHo+iX3ICyA3owQmJKDu8pAzmZrrW4AM2tp6IS+15f1z9Kl/szoePnrkSl\n\tl3VzrRABnqUdj2hDd1qf06SdirsnPPwowoolRW9p0mf1hw9eVzsa2YafXkOK3C0TPD\n\t47KSduws+XsjNB2rS3eH5fjL1vj47osCceZD60WQ=","Date":"Thu, 6 Nov 2025 19:37:39 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 14/35] pipeline: rkisp1: Apply initial controls","Message-ID":"","References":"<20251024085130.995967-1-stefan.klug@ideasonboard.com>\n\t<20251024085130.995967-15-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251024085130.995967-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\" "}}]