[{"id":20732,"web_url":"https://patchwork.libcamera.org/comment/20732/","msgid":"<163640979559.1410963.1188049944090814334@Monstersaurus>","date":"2021-11-08T22:16:35","subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: pipeline: ipu3:\n\tApply the requested test pattern mode","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hirokazu Honda (2021-11-04 06:42:52)\n> This enables ipu3 pipeline handler to apply the test pattern mode\n> per frame.\n> \n> Signed-off-by: Hirokazu Honda \n> Reviewed-by: Jean-Michel Hautbois \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 28 ++++++++++++++++++++++++++--\n> 1 file changed, 26 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 63cb7f11..cb141ba9 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -59,6 +59,7 @@ public:\n> void statBufferReady(FrameBuffer *buffer);\n> void queuePendingRequests();\n> void cancelPendingRequests();\n> + void frameStart(uint32_t sequence);\n> \n> CIO2Device cio2_;\n> ImgUDevice *imgu_;\n> @@ -78,6 +79,7 @@ public:\n> std::unique_ptr ipa_;\n> \n> std::queue pendingRequests_;\n> + std::queue processingRequests_;\n\nI'm a bit weary of having requests on multiple queues like this.\nIt might be better if we could add some documentation describing the\npurpose of each queue, and what lifetime a request has on each one.\n\n\nPerhaps a banner comment before processingRequests_ to describe briefly\nthat the requests will sit on this queue to synchronise setting controls\nat frameStart time? I'm not sure what level will help - but it seems a\nbit vague for 'processingRequests' and 'pendingRequests' otherwise.\n\n\n\n> \n> ControlInfoMap ipaControls_;\n> \n> @@ -569,6 +571,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> cio2->sensor()->sensorInfo(&sensorInfo);\n> data->cropRegion_ = sensorInfo.analogCrop;\n> \n> + cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff);\n> +\n\nThis looks like something the CameraSensor should do at the end of initialisation. (CameraSensor::init())\n\n\n> /*\n> * Configure the H/V flip controls based on the combination of\n> * the sensor and user transform.\n> @@ -797,11 +801,13 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> int ret = 0;\n> \n> data->cancelPendingRequests();\n> + data->processingRequests_ = {};\n\nWe add requests to the processingRequests_ during\nqueuePendingRequests(), so maybe we should clear them during\ncancelPendingRequests()?\n\n> data->ipa_->stop();\n> \n> ret |= data->imgu_->stop();\n> ret |= data->cio2_.stop();\n> +\n> if (ret)\n> LOG(IPU3, Warning) << \"Failed to stop camera \" << camera->id();\n> \n> @@ -852,6 +858,8 @@ void IPU3CameraData::queuePendingRequests()\n> \n> info->rawBuffer = rawBuffer;\n> \n> + processingRequests_.push(request);\n> +\n> ipa::ipu3::IPU3Event ev;\n> ev.op = ipa::ipu3::EventProcessControls;\n> ev.frame = info->id;\n> @@ -1131,8 +1139,8 @@ int PipelineHandlerIPU3::registerCameras()\n> data->delayedCtrls_ =\n> std::make_unique(cio2->sensor()->device(),\n> params);\n> - data->cio2_.frameStart().connect(data->delayedCtrls_.get(),\n> - &DelayedControls::applyControls);\n> + data->cio2_.frameStart().connect(data.get(),\n> + &IPU3CameraData::frameStart);\n\nI like having an explicit/clear frameStart event function here.\n\n> \n> /* Convert the sensor rotation to a transformation */\n> int32_t rotation = 0;\n> @@ -1423,6 +1431,22 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> ipa_->processEvent(ev);\n> }\n> \n> +void IPU3CameraData::frameStart(uint32_t sequence)\n> +{\n> + if (!processingRequests_.empty()) {\n> + /* Assumes applying the test pattern mode affects immediately. */\n\nThat comment seems misplaced now.\nPerhaps you could add:\n\n\t\t /*\n\t\t * Handle controls which are to be set ready for the\n\t\t * next frame to start.\n\t\t */\n\nBut I'm not sure of the race/sequencing here. I presume at the point we\nhave a frameStart event, any controls we apply will take effect for the\n/next/ frame ? This frame has already started...\n\nOr do we expect to set a control here and have it take effect on this\nframe? (That sounds way too racy to be possible).\n\nWe're going to need to be able to set other controls at this point,\nperhaps from the IPA too, so I expect we'll see some further rework here \nlater.\n\n> + Request *request = processingRequests_.front();\n> + processingRequests_.pop();\n> +\n> + int ret = cio2_.sensor()->applyRequestControls(request);\n> + if (ret)\n> + LOG(IPU3, Error) << \"Failed applying controls: \" << ret;\n> + }\n> +\n> + /* Controls that don't affect immediately are applied in delayedCtrls. */\n> + delayedCtrls_->applyControls(sequence);\n> +}\n> +\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)\n> \n> } /* namespace libcamera */\n> -- \n> 2.33.1.1089.g2158813163f-goog\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 F11C9BF415\n\tfor ;\n\tMon, 8 Nov 2021 22:16:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 49F3F6035D;\n\tMon, 8 Nov 2021 23:16:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C20FD6032C\n\tfor ;\n\tMon, 8 Nov 2021 23:16:39 +0100 (CET)","from pendragon.ideasonboard.com\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 2980518CE;\n\tMon, 8 Nov 2021 23:16:39 +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=\"u966XFtF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636409799;\n\tbh=PAhwnebJcutFkT37q1Cj6PiKRBkl+HPeXU857DeKMP4=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=u966XFtFAJy8FkVYqeSBK3z1SPURofhvTwzXX5Hi9TyzooZJQ8LQiArGk1H60UuKT\n\tLDm8uaKE1TSFfb2EBuiLUY6vTPV4Trn/uUG3liNr+ovwZBRi2IGhfBcJ4MXaUaWfHy\n\tFnD2Gj+N4Tn98EQ0YL/uF2kx2uEo8sqzMaAlyp9I=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211104064252.2091330-4-hiroh@chromium.org>","References":"<20211104064252.2091330-1-hiroh@chromium.org>\n\t<20211104064252.2091330-4-hiroh@chromium.org>","From":"Kieran Bingham ","To":"Hirokazu Honda , libcamera-devel@lists.libcamera.org","Date":"Mon, 08 Nov 2021 22:16:35 +0000","Message-ID":"<163640979559.1410963.1188049944090814334@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: pipeline: ipu3:\n\tApply the requested test pattern mode","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":20741,"web_url":"https://patchwork.libcamera.org/comment/20741/","msgid":"","date":"2021-11-09T06:39:43","subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: pipeline: ipu3:\n\tApply the requested test pattern mode","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Kieran,\n\nOn Tue, Nov 9, 2021 at 7:16 AM Kieran Bingham\n wrote:\n>\n> Quoting Hirokazu Honda (2021-11-04 06:42:52)\n> > This enables ipu3 pipeline handler to apply the test pattern mode\n> > per frame.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > Reviewed-by: Jean-Michel Hautbois \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 28 ++++++++++++++++++++++++++--\n> > 1 file changed, 26 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 63cb7f11..cb141ba9 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -59,6 +59,7 @@ public:\n> > void statBufferReady(FrameBuffer *buffer);\n> > void queuePendingRequests();\n> > void cancelPendingRequests();\n> > + void frameStart(uint32_t sequence);\n> >\n> > CIO2Device cio2_;\n> > ImgUDevice *imgu_;\n> > @@ -78,6 +79,7 @@ public:\n> > std::unique_ptr ipa_;\n> >\n> > std::queue pendingRequests_;\n> > + std::queue processingRequests_;\n>\n> I'm a bit weary of having requests on multiple queues like this.\n> It might be better if we could add some documentation describing the\n> purpose of each queue, and what lifetime a request has on each one.\n>\n>\n> Perhaps a banner comment before processingRequests_ to describe briefly\n> that the requests will sit on this queue to synchronise setting controls\n> at frameStart time? I'm not sure what level will help - but it seems a\n> bit vague for 'processingRequests' and 'pendingRequests' otherwise.\n>\n>\n>\n> >\n> > ControlInfoMap ipaControls_;\n> >\n> > @@ -569,6 +571,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > cio2->sensor()->sensorInfo(&sensorInfo);\n> > data->cropRegion_ = sensorInfo.analogCrop;\n> >\n> > + cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff);\n> > +\n>\n> This looks like something the CameraSensor should do at the end of initialisation. (CameraSensor::init())\n>\n>\n> > /*\n> > * Configure the H/V flip controls based on the combination of\n> > * the sensor and user transform.\n> > @@ -797,11 +801,13 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > int ret = 0;\n> >\n> > data->cancelPendingRequests();\n> > + data->processingRequests_ = {};\n>\n> We add requests to the processingRequests_ during\n> queuePendingRequests(), so maybe we should clear them during\n> cancelPendingRequests()?\n>\n> > data->ipa_->stop();\n> >\n> > ret |= data->imgu_->stop();\n> > ret |= data->cio2_.stop();\n> > +\n> > if (ret)\n> > LOG(IPU3, Warning) << \"Failed to stop camera \" << camera->id();\n> >\n> > @@ -852,6 +858,8 @@ void IPU3CameraData::queuePendingRequests()\n> >\n> > info->rawBuffer = rawBuffer;\n> >\n> > + processingRequests_.push(request);\n> > +\n> > ipa::ipu3::IPU3Event ev;\n> > ev.op = ipa::ipu3::EventProcessControls;\n> > ev.frame = info->id;\n> > @@ -1131,8 +1139,8 @@ int PipelineHandlerIPU3::registerCameras()\n> > data->delayedCtrls_ =\n> > std::make_unique(cio2->sensor()->device(),\n> > params);\n> > - data->cio2_.frameStart().connect(data->delayedCtrls_.get(),\n> > - &DelayedControls::applyControls);\n> > + data->cio2_.frameStart().connect(data.get(),\n> > + &IPU3CameraData::frameStart);\n>\n> I like having an explicit/clear frameStart event function here.\n>\n\nSorry, I don't get this comment. Could you elaborate?\n\n> >\n> > /* Convert the sensor rotation to a transformation */\n> > int32_t rotation = 0;\n> > @@ -1423,6 +1431,22 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > ipa_->processEvent(ev);\n> > }\n> >\n> > +void IPU3CameraData::frameStart(uint32_t sequence)\n> > +{\n> > + if (!processingRequests_.empty()) {\n> > + /* Assumes applying the test pattern mode affects immediately. */\n>\n> That comment seems misplaced now.\n> Perhaps you could add:\n>\n> /*\n> * Handle controls which are to be set ready for the\n> * next frame to start.\n> */\n>\n> But I'm not sure of the race/sequencing here. I presume at the point we\n> have a frameStart event, any controls we apply will take effect for the\n> /next/ frame ? This frame has already started...\n>\n> Or do we expect to set a control here and have it take effect on this\n> frame? (That sounds way too racy to be possible).\n>\n\nThat's good point.. It is difficult to set control in a proper time as\nwe're not sure when the proper time is.\nWe also need to think about the effect to the proceeding frames that\nare being processed.\n\nThanks,\n-Hiro\n> We're going to need to be able to set other controls at this point,\n> perhaps from the IPA too, so I expect we'll see some further rework here\n> later.\n>\n> > + Request *request = processingRequests_.front();\n> > + processingRequests_.pop();\n> > +\n> > + int ret = cio2_.sensor()->applyRequestControls(request);\n> > + if (ret)\n> > + LOG(IPU3, Error) << \"Failed applying controls: \" << ret;\n> > + }\n> > +\n> > + /* Controls that don't affect immediately are applied in delayedCtrls. */\n> > + delayedCtrls_->applyControls(sequence);\n> > +}\n> > +\n> > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)\n> >\n> > } /* namespace libcamera */\n> > --\n> > 2.33.1.1089.g2158813163f-goog\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 58B09BDB1C\n\tfor ;\n\tTue, 9 Nov 2021 06:39:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9E206034A;\n\tTue, 9 Nov 2021 07:39:55 +0100 (CET)","from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com\n\t[IPv6:2a00:1450:4864:20::52d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C606760121\n\tfor ;\n\tTue, 9 Nov 2021 07:39:53 +0100 (CET)","by mail-ed1-x52d.google.com with SMTP id f8so72614439edy.4\n\tfor ;\n\tMon, 08 Nov 2021 22:39:53 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"knjZJLSf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=/YB+MLQkVvLpvOYcXbLeaLDLV+qvfgojxr6FKrS2KuQ=;\n\tb=knjZJLSftZAfboyrRSAPtv48MVYmfQaxMjBaaVmOcTBtSmNl6H7Wm5BJxyeCITpyKH\n\tsyE9MGZHeTaRU54S4ePVmlGAnkpsQ8CKDlmtRgvSeb2cRK/8MKzfwIh1CIRlKPCpD6SO\n\t2jgenRoqyM+NJiEt89OYpc6KxdkBCpOuxI3u4=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=/YB+MLQkVvLpvOYcXbLeaLDLV+qvfgojxr6FKrS2KuQ=;\n\tb=LTq5HFsJrxqCH3qBcpdv44YHH+ag2UxNxCMYd+Es6kqhJ0IbnBY0VgxZLxv2KwmIsn\n\t+rbfGH5EtKiWN0+/ZQyM0W/KQAgDb1yFmh1sW39qOJ6WtokhtsbwKT/BneMftesgZlv8\n\tB3kk+CgBm2qimP+NyoIfGoktnVfTTGQT8MlyDgi36GWUZSyXHVqMc1JrSd7Yiuzkj2i6\n\tbGhRRRDFcO3YGJ7MergfCpuVRwntLlahrMY/iz2o3yRmqWO1+COaXtT+/VLPWKgODJf5\n\tDuj6pRoumhOgsZT+uJYvm/Qr/3u5rjaYh8SrdkEiV6ztYnujYUmfIbYJxPOOhcWk7x9u\n\t8GiA==","X-Gm-Message-State":"AOAM532ivjmLmhD/HhrfUviae9IV+4k5vjlJBhnvniYTuFm1928txc1h\n\t6In1w89mgZK0hGr5xTlv/Pte0rHqdy1eJSE3GISjl7CDtfw=","X-Google-Smtp-Source":"ABdhPJw5CLOaAOJdrhu/AO4Hn9INMl9H0EHeMevfMnzXkjkVdjpKwjUdTR2i6rn3GflxSp69p6xpnnZFjzYOErPbMDI=","X-Received":"by 2002:a17:906:52d8:: with SMTP id\n\tw24mr6706314ejn.296.1636439993345; \n\tMon, 08 Nov 2021 22:39:53 -0800 (PST)","MIME-Version":"1.0","References":"<20211104064252.2091330-1-hiroh@chromium.org>\n\t<20211104064252.2091330-4-hiroh@chromium.org>\n\t<163640979559.1410963.1188049944090814334@Monstersaurus>","In-Reply-To":"<163640979559.1410963.1188049944090814334@Monstersaurus>","From":"Hirokazu Honda ","Date":"Tue, 9 Nov 2021 15:39:43 +0900","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: pipeline: ipu3:\n\tApply the requested test pattern mode","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20748,"web_url":"https://patchwork.libcamera.org/comment/20748/","msgid":"<163645884985.1598741.12441753379613008400@Monstersaurus>","date":"2021-11-09T11:54:09","subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: pipeline: ipu3:\n\tApply the requested test pattern mode","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hirokazu Honda (2021-11-09 06:39:43)\n> Hi Kieran,\n> \n> On Tue, Nov 9, 2021 at 7:16 AM Kieran Bingham\n> wrote:\n> >\n> > Quoting Hirokazu Honda (2021-11-04 06:42:52)\n> > > This enables ipu3 pipeline handler to apply the test pattern mode\n> > > per frame.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > Reviewed-by: Jean-Michel Hautbois \n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 28 ++++++++++++++++++++++++++--\n> > > 1 file changed, 26 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 63cb7f11..cb141ba9 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -59,6 +59,7 @@ public:\n> > > void statBufferReady(FrameBuffer *buffer);\n> > > void queuePendingRequests();\n> > > void cancelPendingRequests();\n> > > + void frameStart(uint32_t sequence);\n> > >\n> > > CIO2Device cio2_;\n> > > ImgUDevice *imgu_;\n> > > @@ -78,6 +79,7 @@ public:\n> > > std::unique_ptr ipa_;\n> > >\n> > > std::queue pendingRequests_;\n> > > + std::queue processingRequests_;\n> >\n> > I'm a bit weary of having requests on multiple queues like this.\n> > It might be better if we could add some documentation describing the\n> > purpose of each queue, and what lifetime a request has on each one.\n> >\n> >\n> > Perhaps a banner comment before processingRequests_ to describe briefly\n> > that the requests will sit on this queue to synchronise setting controls\n> > at frameStart time? I'm not sure what level will help - but it seems a\n> > bit vague for 'processingRequests' and 'pendingRequests' otherwise.\n> >\n> >\n> >\n> > >\n> > > ControlInfoMap ipaControls_;\n> > >\n> > > @@ -569,6 +571,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > > cio2->sensor()->sensorInfo(&sensorInfo);\n> > > data->cropRegion_ = sensorInfo.analogCrop;\n> > >\n> > > + cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff);\n> > > +\n> >\n> > This looks like something the CameraSensor should do at the end of initialisation. (CameraSensor::init())\n> >\n> >\n> > > /*\n> > > * Configure the H/V flip controls based on the combination of\n> > > * the sensor and user transform.\n> > > @@ -797,11 +801,13 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > > int ret = 0;\n> > >\n> > > data->cancelPendingRequests();\n> > > + data->processingRequests_ = {};\n> >\n> > We add requests to the processingRequests_ during\n> > queuePendingRequests(), so maybe we should clear them during\n> > cancelPendingRequests()?\n> >\n> > > data->ipa_->stop();\n> > >\n> > > ret |= data->imgu_->stop();\n> > > ret |= data->cio2_.stop();\n> > > +\n> > > if (ret)\n> > > LOG(IPU3, Warning) << \"Failed to stop camera \" << camera->id();\n> > >\n> > > @@ -852,6 +858,8 @@ void IPU3CameraData::queuePendingRequests()\n> > >\n> > > info->rawBuffer = rawBuffer;\n> > >\n> > > + processingRequests_.push(request);\n> > > +\n> > > ipa::ipu3::IPU3Event ev;\n> > > ev.op = ipa::ipu3::EventProcessControls;\n> > > ev.frame = info->id;\n> > > @@ -1131,8 +1139,8 @@ int PipelineHandlerIPU3::registerCameras()\n> > > data->delayedCtrls_ =\n> > > std::make_unique(cio2->sensor()->device(),\n> > > params);\n> > > - data->cio2_.frameStart().connect(data->delayedCtrls_.get(),\n> > > - &DelayedControls::applyControls);\n> > > + data->cio2_.frameStart().connect(data.get(),\n> > > + &IPU3CameraData::frameStart);\n> >\n> > I like having an explicit/clear frameStart event function here.\n> >\n> \n> Sorry, I don't get this comment. Could you elaborate?\n\nI meant that I think this is good ;-)\n\nIt makes an explicit call on the IPU3CameraData when there is a\nframeStart event, rather than the short cut to delayed controls..\n\n> > >\n> > > /* Convert the sensor rotation to a transformation */\n> > > int32_t rotation = 0;\n> > > @@ -1423,6 +1431,22 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > > ipa_->processEvent(ev);\n> > > }\n> > >\n> > > +void IPU3CameraData::frameStart(uint32_t sequence)\n> > > +{\n> > > + if (!processingRequests_.empty()) {\n> > > + /* Assumes applying the test pattern mode affects immediately. */\n> >\n> > That comment seems misplaced now.\n> > Perhaps you could add:\n> >\n> > /*\n> > * Handle controls which are to be set ready for the\n> > * next frame to start.\n> > */\n> >\n> > But I'm not sure of the race/sequencing here. I presume at the point we\n> > have a frameStart event, any controls we apply will take effect for the\n> > /next/ frame ? This frame has already started...\n> >\n> > Or do we expect to set a control here and have it take effect on this\n> > frame? (That sounds way too racy to be possible).\n> >\n> \n> That's good point.. It is difficult to set control in a proper time as\n> we're not sure when the proper time is.\n> We also need to think about the effect to the proceeding frames that\n> are being processed.\n> \n> Thanks,\n> -Hiro\n> > We're going to need to be able to set other controls at this point,\n> > perhaps from the IPA too, so I expect we'll see some further rework here\n> > later.\n> >\n> > > + Request *request = processingRequests_.front();\n> > > + processingRequests_.pop();\n> > > +\n> > > + int ret = cio2_.sensor()->applyRequestControls(request);\n\nTalking with Jacopo, he really dislikes passing the request to the\nsensor ;-( - While I dislike having to filter out the test pattern\ncontrol (or CameraSensor specific controls) in the frameStart event for\neach pipeline handler.\n\nNot sure what the middle ground is yet. But it might just be renaming\napplyRequestControls() to applyTestPatternControls() or something that\ncan't be suggested that it might be used by other controls. Perhaps\nthat's closer to your original implementation anyway ...\n\n\n> > > + if (ret)\n> > > + LOG(IPU3, Error) << \"Failed applying controls: \" << ret;\n> > > + }\n> > > +\n> > > + /* Controls that don't affect immediately are applied in delayedCtrls. */\n> > > + delayedCtrls_->applyControls(sequence);\n> > > +}\n> > > +\n> > > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)\n> > >\n> > > } /* namespace libcamera */\n> > > --\n> > > 2.33.1.1089.g2158813163f-goog\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 AB7CEBDB1C\n\tfor ;\n\tTue, 9 Nov 2021 11:54:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D600E6034E;\n\tTue, 9 Nov 2021 12:54:13 +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 DC08A600BF\n\tfor ;\n\tTue, 9 Nov 2021 12:54:12 +0100 (CET)","from pendragon.ideasonboard.com\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 6CD60FD1;\n\tTue, 9 Nov 2021 12:54:12 +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=\"sBpvGxrM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636458852;\n\tbh=Rzu3WzPxvfb1/8PKOfp9xk+gzEUlGYGQLP05ZBT0J/0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=sBpvGxrMY8PbgHkdiGiIye95T91tbwLZ+nRhfPjE5H4f9xjg8p1dWyhREGP+/j2Eb\n\tWvU+hWc7joRoGO+BQrtaDHwlhsr4EsdUNA/UyGAqOKsMBRP8nZsMjDMe0pCtW4MD/h\n\tTwYKxtCs9tvQtRTSqerZDsQqhcxrpiIMqvGeZ83U=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211104064252.2091330-1-hiroh@chromium.org>\n\t<20211104064252.2091330-4-hiroh@chromium.org>\n\t<163640979559.1410963.1188049944090814334@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"Hirokazu Honda ","Date":"Tue, 09 Nov 2021 11:54:09 +0000","Message-ID":"<163645884985.1598741.12441753379613008400@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: pipeline: ipu3:\n\tApply the requested test pattern mode","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20778,"web_url":"https://patchwork.libcamera.org/comment/20778/","msgid":"","date":"2021-11-10T03:18:25","subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: pipeline: ipu3:\n\tApply the requested test pattern mode","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Kieran,\n\nOn Tue, Nov 9, 2021 at 8:54 PM Kieran Bingham\n wrote:\n>\n> Quoting Hirokazu Honda (2021-11-09 06:39:43)\n> > Hi Kieran,\n> >\n> > On Tue, Nov 9, 2021 at 7:16 AM Kieran Bingham\n> > wrote:\n> > >\n> > > Quoting Hirokazu Honda (2021-11-04 06:42:52)\n> > > > This enables ipu3 pipeline handler to apply the test pattern mode\n> > > > per frame.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > Reviewed-by: Jean-Michel Hautbois \n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 28 ++++++++++++++++++++++++++--\n> > > > 1 file changed, 26 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index 63cb7f11..cb141ba9 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -59,6 +59,7 @@ public:\n> > > > void statBufferReady(FrameBuffer *buffer);\n> > > > void queuePendingRequests();\n> > > > void cancelPendingRequests();\n> > > > + void frameStart(uint32_t sequence);\n> > > >\n> > > > CIO2Device cio2_;\n> > > > ImgUDevice *imgu_;\n> > > > @@ -78,6 +79,7 @@ public:\n> > > > std::unique_ptr ipa_;\n> > > >\n> > > > std::queue pendingRequests_;\n> > > > + std::queue processingRequests_;\n> > >\n> > > I'm a bit weary of having requests on multiple queues like this.\n> > > It might be better if we could add some documentation describing the\n> > > purpose of each queue, and what lifetime a request has on each one.\n> > >\n> > >\n> > > Perhaps a banner comment before processingRequests_ to describe briefly\n> > > that the requests will sit on this queue to synchronise setting controls\n> > > at frameStart time? I'm not sure what level will help - but it seems a\n> > > bit vague for 'processingRequests' and 'pendingRequests' otherwise.\n> > >\n> > >\n> > >\n> > > >\n> > > > ControlInfoMap ipaControls_;\n> > > >\n> > > > @@ -569,6 +571,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > > > cio2->sensor()->sensorInfo(&sensorInfo);\n> > > > data->cropRegion_ = sensorInfo.analogCrop;\n> > > >\n> > > > + cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff);\n> > > > +\n> > >\n> > > This looks like something the CameraSensor should do at the end of initialisation. (CameraSensor::init())\n> > >\n> > >\n> > > > /*\n> > > > * Configure the H/V flip controls based on the combination of\n> > > > * the sensor and user transform.\n> > > > @@ -797,11 +801,13 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > > > int ret = 0;\n> > > >\n> > > > data->cancelPendingRequests();\n> > > > + data->processingRequests_ = {};\n> > >\n> > > We add requests to the processingRequests_ during\n> > > queuePendingRequests(), so maybe we should clear them during\n> > > cancelPendingRequests()?\n> > >\n> > > > data->ipa_->stop();\n> > > >\n> > > > ret |= data->imgu_->stop();\n> > > > ret |= data->cio2_.stop();\n> > > > +\n> > > > if (ret)\n> > > > LOG(IPU3, Warning) << \"Failed to stop camera \" << camera->id();\n> > > >\n> > > > @@ -852,6 +858,8 @@ void IPU3CameraData::queuePendingRequests()\n> > > >\n> > > > info->rawBuffer = rawBuffer;\n> > > >\n> > > > + processingRequests_.push(request);\n> > > > +\n> > > > ipa::ipu3::IPU3Event ev;\n> > > > ev.op = ipa::ipu3::EventProcessControls;\n> > > > ev.frame = info->id;\n> > > > @@ -1131,8 +1139,8 @@ int PipelineHandlerIPU3::registerCameras()\n> > > > data->delayedCtrls_ =\n> > > > std::make_unique(cio2->sensor()->device(),\n> > > > params);\n> > > > - data->cio2_.frameStart().connect(data->delayedCtrls_.get(),\n> > > > - &DelayedControls::applyControls);\n> > > > + data->cio2_.frameStart().connect(data.get(),\n> > > > + &IPU3CameraData::frameStart);\n> > >\n> > > I like having an explicit/clear frameStart event function here.\n> > >\n> >\n> > Sorry, I don't get this comment. Could you elaborate?\n>\n> I meant that I think this is good ;-)\n>\n> It makes an explicit call on the IPU3CameraData when there is a\n> frameStart event, rather than the short cut to delayed controls..\n>\n> > > >\n> > > > /* Convert the sensor rotation to a transformation */\n> > > > int32_t rotation = 0;\n> > > > @@ -1423,6 +1431,22 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > > > ipa_->processEvent(ev);\n> > > > }\n> > > >\n> > > > +void IPU3CameraData::frameStart(uint32_t sequence)\n> > > > +{\n> > > > + if (!processingRequests_.empty()) {\n> > > > + /* Assumes applying the test pattern mode affects immediately. */\n> > >\n> > > That comment seems misplaced now.\n> > > Perhaps you could add:\n> > >\n> > > /*\n> > > * Handle controls which are to be set ready for the\n> > > * next frame to start.\n> > > */\n> > >\n> > > But I'm not sure of the race/sequencing here. I presume at the point we\n> > > have a frameStart event, any controls we apply will take effect for the\n> > > /next/ frame ? This frame has already started...\n> > >\n> > > Or do we expect to set a control here and have it take effect on this\n> > > frame? (That sounds way too racy to be possible).\n> > >\n> >\n> > That's good point.. It is difficult to set control in a proper time as\n> > we're not sure when the proper time is.\n> > We also need to think about the effect to the proceeding frames that\n> > are being processed.\n> >\n> > Thanks,\n> > -Hiro\n> > > We're going to need to be able to set other controls at this point,\n> > > perhaps from the IPA too, so I expect we'll see some further rework here\n> > > later.\n> > >\n> > > > + Request *request = processingRequests_.front();\n> > > > + processingRequests_.pop();\n> > > > +\n> > > > + int ret = cio2_.sensor()->applyRequestControls(request);\n>\n> Talking with Jacopo, he really dislikes passing the request to the\n> sensor ;-( - While I dislike having to filter out the test pattern\n> control (or CameraSensor specific controls) in the frameStart event for\n> each pipeline handler.\n>\n> Not sure what the middle ground is yet. But it might just be renaming\n> applyRequestControls() to applyTestPatternControls() or something that\n> can't be suggested that it might be used by other controls. Perhaps\n> that's closer to your original implementation anyway ...\n>\n\nI will ask Laurent about this today.\n\n-Hiro\n>\n> > > > + if (ret)\n> > > > + LOG(IPU3, Error) << \"Failed applying controls: \" << ret;\n> > > > + }\n> > > > +\n> > > > + /* Controls that don't affect immediately are applied in delayedCtrls. */\n> > > > + delayedCtrls_->applyControls(sequence);\n> > > > +}\n> > > > +\n> > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)\n> > > >\n> > > > } /* namespace libcamera */\n> > > > --\n> > > > 2.33.1.1089.g2158813163f-goog\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 301C2BDB1C\n\tfor ;\n\tWed, 10 Nov 2021 03:18:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAFE260362;\n\tWed, 10 Nov 2021 04:18:37 +0100 (CET)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1B8A460234\n\tfor ;\n\tWed, 10 Nov 2021 04:18:37 +0100 (CET)","by mail-ed1-x534.google.com with SMTP id j21so4631797edt.11\n\tfor ;\n\tTue, 09 Nov 2021 19:18:37 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"CA8DfEOP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=r3QpJlTeokSavAJTdFnFHSw+Gaqqet1G7c9XavYTPNo=;\n\tb=CA8DfEOPqPcpv4SzIY58keNXtDfywcDj+6cyWP3tHFhpCUe3NIrWxxB0qS/G3Zl5z/\n\tEh43GslIEusMc96CpQqn2p7puNlKa1cOBr9BquERWQE/caqo5AJwAW9TxMbSSl2yKuOc\n\tYDJZ5zzZooQhPMNSG1FC/4aW+B3nbh8I0nkzA=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=r3QpJlTeokSavAJTdFnFHSw+Gaqqet1G7c9XavYTPNo=;\n\tb=bdXS4midUjbBcNMQgbeGkHq4gV9Wr6KI8C8lHtDIAuxGIVRYXIf9gbZl7AJYhETqrl\n\tl7Zn72Ji/Eqeh8VCzUZkF2KrY8xnk/8WtnV7HPLgpaD9jhhwTFRcmgnC9TLZI4DVxRhq\n\tokdpDMbp0fUIBi/cPuJ38qR5kywmkQaKkwLuNte0Rur7Km90a7WO97XkKITXxPtZBNlO\n\tj1qSFFNWNc86tHP0Sqbni8HpNugdtEDnCUbji3dd7E1DcL19HY4YzhD5EQy+4Sfr17hQ\n\teAlMS/5jib8pGIjE5JHtY70yNAhs3lRtjKcWKlsYsNIiWePXJ9xO7YAdO0IQzKPCtxQF\n\tFr8w==","X-Gm-Message-State":"AOAM530EsXTCZLmUtn4g1paEHHJp2sN1vx06FIfvIUVUVSls4iEshKsn\n\tVF+HgiPn9B0RNlXUE133jCodvW2DYLeU2Pe5A2rZsv6Llws=","X-Google-Smtp-Source":"ABdhPJxShgfm1exz724ZXONbAbuyu3KxXddmmGJL3tl1eFsjXoAVYoLjqEW6vqv6n6ilx4JlMXgM/MpJwl6+i0mO4pc=","X-Received":"by 2002:a17:906:7310:: with SMTP id\n\tdi16mr14816478ejc.92.1636514316670; \n\tTue, 09 Nov 2021 19:18:36 -0800 (PST)","MIME-Version":"1.0","References":"<20211104064252.2091330-1-hiroh@chromium.org>\n\t<20211104064252.2091330-4-hiroh@chromium.org>\n\t<163640979559.1410963.1188049944090814334@Monstersaurus>\n\t\n\t<163645884985.1598741.12441753379613008400@Monstersaurus>","In-Reply-To":"<163645884985.1598741.12441753379613008400@Monstersaurus>","From":"Hirokazu Honda ","Date":"Wed, 10 Nov 2021 12:18:25 +0900","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: pipeline: ipu3:\n\tApply the requested test pattern mode","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21090,"web_url":"https://patchwork.libcamera.org/comment/21090/","msgid":"","date":"2021-11-22T02:13:25","subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: pipeline: ipu3:\n\tApply the requested test pattern mode","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Wed, Nov 10, 2021 at 12:18:25PM +0900, Hirokazu Honda wrote:\n> On Tue, Nov 9, 2021 at 8:54 PM Kieran Bingham wrote:\n> > Quoting Hirokazu Honda (2021-11-09 06:39:43)\n> > > On Tue, Nov 9, 2021 at 7:16 AM Kieran Bingham wrote:\n> > > > Quoting Hirokazu Honda (2021-11-04 06:42:52)\n> > > > > This enables ipu3 pipeline handler to apply the test pattern mode\n> > > > > per frame.\n> > > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > > > Reviewed-by: Jean-Michel Hautbois \n> > > > > ---\n> > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 28 ++++++++++++++++++++++++++--\n> > > > > 1 file changed, 26 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > index 63cb7f11..cb141ba9 100644\n> > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > @@ -59,6 +59,7 @@ public:\n> > > > > void statBufferReady(FrameBuffer *buffer);\n> > > > > void queuePendingRequests();\n> > > > > void cancelPendingRequests();\n> > > > > + void frameStart(uint32_t sequence);\n> > > > >\n> > > > > CIO2Device cio2_;\n> > > > > ImgUDevice *imgu_;\n> > > > > @@ -78,6 +79,7 @@ public:\n> > > > > std::unique_ptr ipa_;\n> > > > >\n> > > > > std::queue pendingRequests_;\n> > > > > + std::queue processingRequests_;\n> > > >\n> > > > I'm a bit weary of having requests on multiple queues like this.\n> > > > It might be better if we could add some documentation describing the\n> > > > purpose of each queue, and what lifetime a request has on each one.\n> > > >\n> > > >\n> > > > Perhaps a banner comment before processingRequests_ to describe briefly\n> > > > that the requests will sit on this queue to synchronise setting controls\n> > > > at frameStart time? I'm not sure what level will help - but it seems a\n> > > > bit vague for 'processingRequests' and 'pendingRequests' otherwise.\n> > > >\n> > > >\n> > > >\n> > > > >\n> > > > > ControlInfoMap ipaControls_;\n> > > > >\n> > > > > @@ -569,6 +571,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > > > > cio2->sensor()->sensorInfo(&sensorInfo);\n> > > > > data->cropRegion_ = sensorInfo.analogCrop;\n> > > > >\n> > > > > + cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff);\n> > > > > +\n> > > >\n> > > > This looks like something the CameraSensor should do at the end of initialisation. (CameraSensor::init())\n> > > >\n> > > >\n> > > > > /*\n> > > > > * Configure the H/V flip controls based on the combination of\n> > > > > * the sensor and user transform.\n> > > > > @@ -797,11 +801,13 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > > > > int ret = 0;\n> > > > >\n> > > > > data->cancelPendingRequests();\n> > > > > + data->processingRequests_ = {};\n> > > >\n> > > > We add requests to the processingRequests_ during\n> > > > queuePendingRequests(), so maybe we should clear them during\n> > > > cancelPendingRequests()?\n> > > >\n> > > > > data->ipa_->stop();\n> > > > >\n> > > > > ret |= data->imgu_->stop();\n> > > > > ret |= data->cio2_.stop();\n> > > > > +\n> > > > > if (ret)\n> > > > > LOG(IPU3, Warning) << \"Failed to stop camera \" << camera->id();\n> > > > >\n> > > > > @@ -852,6 +858,8 @@ void IPU3CameraData::queuePendingRequests()\n> > > > >\n> > > > > info->rawBuffer = rawBuffer;\n> > > > >\n> > > > > + processingRequests_.push(request);\n> > > > > +\n> > > > > ipa::ipu3::IPU3Event ev;\n> > > > > ev.op = ipa::ipu3::EventProcessControls;\n> > > > > ev.frame = info->id;\n> > > > > @@ -1131,8 +1139,8 @@ int PipelineHandlerIPU3::registerCameras()\n> > > > > data->delayedCtrls_ =\n> > > > > std::make_unique(cio2->sensor()->device(),\n> > > > > params);\n> > > > > - data->cio2_.frameStart().connect(data->delayedCtrls_.get(),\n> > > > > - &DelayedControls::applyControls);\n> > > > > + data->cio2_.frameStart().connect(data.get(),\n> > > > > + &IPU3CameraData::frameStart);\n> > > >\n> > > > I like having an explicit/clear frameStart event function here.\n> > > >\n> > >\n> > > Sorry, I don't get this comment. Could you elaborate?\n> >\n> > I meant that I think this is good ;-)\n> >\n> > It makes an explicit call on the IPU3CameraData when there is a\n> > frameStart event, rather than the short cut to delayed controls..\n> >\n> > > > >\n> > > > > /* Convert the sensor rotation to a transformation */\n> > > > > int32_t rotation = 0;\n> > > > > @@ -1423,6 +1431,22 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > > > > ipa_->processEvent(ev);\n> > > > > }\n> > > > >\n> > > > > +void IPU3CameraData::frameStart(uint32_t sequence)\n> > > > > +{\n> > > > > + if (!processingRequests_.empty()) {\n> > > > > + /* Assumes applying the test pattern mode affects immediately. */\n> > > >\n> > > > That comment seems misplaced now.\n> > > > Perhaps you could add:\n> > > >\n> > > > /*\n> > > > * Handle controls which are to be set ready for the\n> > > > * next frame to start.\n> > > > */\n> > > >\n> > > > But I'm not sure of the race/sequencing here. I presume at the point we\n> > > > have a frameStart event, any controls we apply will take effect for the\n> > > > /next/ frame ? This frame has already started...\n> > > >\n> > > > Or do we expect to set a control here and have it take effect on this\n> > > > frame? (That sounds way too racy to be possible).\n> > > >\n> > >\n> > > That's good point.. It is difficult to set control in a proper time as\n> > > we're not sure when the proper time is.\n> > > We also need to think about the effect to the proceeding frames that\n> > > are being processed.\n> > >\n> > > Thanks,\n> > > -Hiro\n> > > > We're going to need to be able to set other controls at this point,\n> > > > perhaps from the IPA too, so I expect we'll see some further rework here\n> > > > later.\n> > > >\n> > > > > + Request *request = processingRequests_.front();\n> > > > > + processingRequests_.pop();\n> > > > > +\n> > > > > + int ret = cio2_.sensor()->applyRequestControls(request);\n> >\n> > Talking with Jacopo, he really dislikes passing the request to the\n> > sensor ;-( - While I dislike having to filter out the test pattern\n> > control (or CameraSensor specific controls) in the frameStart event for\n> > each pipeline handler.\n> >\n> > Not sure what the middle ground is yet. But it might just be renaming\n> > applyRequestControls() to applyTestPatternControls() or something that\n> > can't be suggested that it might be used by other controls. Perhaps\n> > that's closer to your original implementation anyway ...\n> \n> I will ask Laurent about this today.\n\nI've replied to this in patch 2/3.\n\n> > > > > + if (ret)\n> > > > > + LOG(IPU3, Error) << \"Failed applying controls: \" << ret;\n> > > > > + }\n> > > > > +\n> > > > > + /* Controls that don't affect immediately are applied in delayedCtrls. */\n> > > > > + delayedCtrls_->applyControls(sequence);\n> > > > > +}\n> > > > > +\n> > > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)\n> > > > >\n> > > > > } /* namespace libcamera */","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 BB48CBF415\n\tfor ;\n\tMon, 22 Nov 2021 02:13:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 75E026036F;\n\tMon, 22 Nov 2021 03:13:50 +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 A04E160228\n\tfor ;\n\tMon, 22 Nov 2021 03:13:48 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3C87FA1B;\n\tMon, 22 Nov 2021 03:13:48 +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=\"ZSf1QCDq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637547228;\n\tbh=AFG93JXbxpIpuCGDUI/WjebWgbfjiFc4f9C/jclViBA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZSf1QCDqwGqxbetA+X57nJdwa5CttQ57kVVqFJnvwxWG3UH92uhNXcUPCTW4eKUDx\n\tqxWk7yuOKyuyY61sypQKNZufm2ETuF/zxZrckuMMevDaqmw8te0q+bOT97U4heJ74d\n\tR59z2+wUfC5p5evwKp4bwwFZkcv9PanXHwL0WwoE=","Date":"Mon, 22 Nov 2021 04:13:25 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20211104064252.2091330-1-hiroh@chromium.org>\n\t<20211104064252.2091330-4-hiroh@chromium.org>\n\t<163640979559.1410963.1188049944090814334@Monstersaurus>\n\t\n\t<163645884985.1598741.12441753379613008400@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: pipeline: ipu3:\n\tApply the requested test pattern mode","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]