[{"id":21213,"web_url":"https://patchwork.libcamera.org/comment/21213/","msgid":"<20211125000150.u47qhsdu7qy2cyfe@uno.localdomain>","date":"2021-11-25T00:01:50","subject":"Re: [libcamera-devel] [PATCH v7 3/3] libcamera: pipeline: ipu3:\n\tApply a requested test pattern mode","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Wed, Nov 24, 2021 at 04:08:12AM +0900, Hirokazu Honda wrote:\n> This enables ipu3 pipeline handler to apply a test pattern mode\n> per frame.\n\nYes, and this also introduces a a way to set controls\nimmediately, the first (and only one for now) is the test pattern\nmode.\n\nDo you think it would be mentioned ?\n\n>\n> Signed-off-by: Hirokazu Honda \n> Reviewed-by: Jean-Michel Hautbois \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 45 ++++++++++++++++++++++++++--\n> 1 file changed, 43 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 25490dcf..a15c6466 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -59,6 +59,7 @@ public:\n> \tvoid statBufferReady(FrameBuffer *buffer);\n> \tvoid queuePendingRequests();\n> \tvoid cancelPendingRequests();\n> +\tvoid frameStart(uint32_t sequence);\n>\n> \tCIO2Device cio2_;\n> \tImgUDevice *imgu_;\n> @@ -76,7 +77,10 @@ public:\n>\n> \tstd::unique_ptr ipa_;\n>\n> +\t/* Requests before queueing cio2 device. */\n> \tstd::queue pendingRequests_;\n> +\t/* Requests queued in cio2 device and before passing imgu device. */\n> +\tstd::queue processingRequests_;\n>\n> \tControlInfoMap ipaControls_;\n>\n> @@ -803,6 +807,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n>\n> \tret |= data->imgu_->stop();\n> \tret |= data->cio2_.stop();\n> +\n\nIntentional ?\n\n> \tif (ret)\n> \t\tLOG(IPU3, Warning) << \"Failed to stop camera \" << camera->id();\n>\n> @@ -823,6 +828,8 @@ void IPU3CameraData::cancelPendingRequests()\n> \t\tpipe()->completeRequest(request);\n> \t\tpendingRequests_.pop();\n> \t}\n> +\n> +\tprocessingRequests_ = {};\n\nShould processingRequests_ be completed one by one as the pending ones\ninstead of being dropped ?\n\n> }\n>\n> void IPU3CameraData::queuePendingRequests()\n> @@ -853,6 +860,8 @@ void IPU3CameraData::queuePendingRequests()\n>\n> \t\tinfo->rawBuffer = rawBuffer;\n>\n> +\t\tprocessingRequests_.push(request);\n> +\n> \t\tipa::ipu3::IPU3Event ev;\n> \t\tev.op = ipa::ipu3::EventProcessControls;\n> \t\tev.frame = info->id;\n> @@ -1121,8 +1130,8 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\tdata->delayedCtrls_ =\n> \t\t\tstd::make_unique(cio2->sensor()->device(),\n> \t\t\t\t\t\t\t params);\n> -\t\tdata->cio2_.frameStart().connect(data->delayedCtrls_.get(),\n> -\t\t\t\t\t\t &DelayedControls::applyControls);\n> +\t\tdata->cio2_.frameStart().connect(data.get(),\n> +\t\t\t\t\t\t &IPU3CameraData::frameStart);\n>\n> \t\t/* Convert the sensor rotation to a transformation */\n> \t\tint32_t rotation = 0;\n> @@ -1414,6 +1423,38 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> \tipa_->processEvent(ev);\n> }\n>\n> +void IPU3CameraData::frameStart(uint32_t sequence)\n\nMaybe a small comment block on the function\n\n/*\n * Handle the start of frame exposure signal.\n *\n * Inspect the list of pending requests waiting for a RAW frame to be\n * produced and apply controls for the 'next' one.\n *\n * Some controls need to be applied immediately, such as the\n * TestPatternMode one. Other controls are handled through the delayed\n * controls class.\n */\n\nThis shows that this functions does two things:\n- Apply some libcamera::controls from the Request immediately\n- Inform the DelayedControl class that a new frame has started to\n advance its internal frame-counting\n\n> +{\n> +\tif (!processingRequests_.empty()) {\n> +\t\t/* Handle controls which are to be set ready for the next frame to start. */\n\n /* Apply controls that have to take effect immediately. */\n\nI now wonder how did we get to the notion that some controls take\neffect 'immediately'. Did we get here simply because we had to way to\napply libcamera::controls to CameraSensor without going through the\nIPA ? I then wonder if talking about 'immediate' and 'next frame' is\nnot misleading. Aren't we just applying some controls which the\npipelinehandler is in charge of bypassing the IPA ?\n\ndo we need a notion of immediate controls ?\n\n> +\t\tRequest *request = processingRequests_.front();\n> +\t\tprocessingRequests_.pop();\n> +\n> +\t\t/* Assumes applying the test pattern mode affects immediately. */\n> +\t\tif (request->controls().contains(controls::draft::TestPatternMode)) {\n\n\t\tif (!request->controls().contains(controls::draft::TestPatternMode))\n break;\n\nAnd reduce the indendation below\n\n> +\t\t\tconst int32_t testPatternMode = request->controls().get(\n> +\t\t\t\tcontrols::draft::TestPatternMode);\n> +\n> +\t\t\tLOG(IPU3, Debug) << \"Apply test pattern mode: \"\n> +\t\t\t\t\t << testPatternMode;\n> +\n> +\t\t\tint ret = cio2_.sensor()->setTestPatternMode(\n> +\t\t\t\tstatic_cast(\n> +\t\t\t\t\ttestPatternMode));\n> +\t\t\tif (ret) {\n> +\t\t\t\tLOG(IPU3, Error) << \"Failed to set test pattern mode: \"\n> +\t\t\t\t\t\t << ret;\n break;\nAnd remove the else {} clause\n\n> +\t\t\t} else {\n> +\t\t\t\trequest->metadata().set(controls::draft::TestPatternMode,\n> +\t\t\t\t\t\t\ttestPatternMode);\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\t/* Controls that don't affect immediately are applied in delayedCtrls. */\n\nI would drop this with the block at the beginning of the function\n\n> +\tdelayedCtrls_->applyControls(sequence);\n\nMinors apart\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +}\n> +\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)\n>\n> } /* namespace libcamera */\n> --\n> 2.34.0.rc2.393.gf8c9666880-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 85BD9BDB13\n\tfor ;\n\tThu, 25 Nov 2021 00:00:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F25FA60371;\n\tThu, 25 Nov 2021 01:00:58 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7B8E860228\n\tfor ;\n\tThu, 25 Nov 2021 01:00:58 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id EFF60C0008;\n\tThu, 25 Nov 2021 00:00:57 +0000 (UTC)"],"Date":"Thu, 25 Nov 2021 01:01:50 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211125000150.u47qhsdu7qy2cyfe@uno.localdomain>","References":"<20211123190812.69805-1-hiroh@chromium.org>\n\t<20211123190812.69805-4-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211123190812.69805-4-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v7 3/3] libcamera: pipeline: ipu3:\n\tApply a 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":21264,"web_url":"https://patchwork.libcamera.org/comment/21264/","msgid":"","date":"2021-11-26T04:42:05","subject":"Re: [libcamera-devel] [PATCH v7 3/3] libcamera: pipeline: ipu3:\n\tApply a requested test pattern mode","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for reviewing.\n\nOn Thu, Nov 25, 2021 at 9:00 AM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Wed, Nov 24, 2021 at 04:08:12AM +0900, Hirokazu Honda wrote:\n> > This enables ipu3 pipeline handler to apply a test pattern mode\n> > per frame.\n>\n> Yes, and this also introduces a a way to set controls\n> immediately, the first (and only one for now) is the test pattern\n> mode.\n>\n> Do you think it would be mentioned ?\n>\n> >\n> > Signed-off-by: Hirokazu Honda \n> > Reviewed-by: Jean-Michel Hautbois \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 45 ++++++++++++++++++++++++++--\n> > 1 file changed, 43 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 25490dcf..a15c6466 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> > @@ -76,7 +77,10 @@ public:\n> >\n> > std::unique_ptr ipa_;\n> >\n> > + /* Requests before queueing cio2 device. */\n> > std::queue pendingRequests_;\n> > + /* Requests queued in cio2 device and before passing imgu device. */\n> > + std::queue processingRequests_;\n> >\n> > ControlInfoMap ipaControls_;\n> >\n> > @@ -803,6 +807,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> >\n> > ret |= data->imgu_->stop();\n> > ret |= data->cio2_.stop();\n> > +\n>\n> Intentional ?\n>\n> > if (ret)\n> > LOG(IPU3, Warning) << \"Failed to stop camera \" << camera->id();\n> >\n> > @@ -823,6 +828,8 @@ void IPU3CameraData::cancelPendingRequests()\n> > pipe()->completeRequest(request);\n> > pendingRequests_.pop();\n> > }\n> > +\n> > + processingRequests_ = {};\n>\n> Should processingRequests_ be completed one by one as the pending ones\n> instead of being dropped ?\n>\n\nHmm, this is a good question.\nI think the metadata for aborted capture doesn't have to be applied to a camera.\nSo I just discard pending requests.\n\n> > }\n> >\n> > void IPU3CameraData::queuePendingRequests()\n> > @@ -853,6 +860,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> > @@ -1121,8 +1130,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> > /* Convert the sensor rotation to a transformation */\n> > int32_t rotation = 0;\n> > @@ -1414,6 +1423,38 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > ipa_->processEvent(ev);\n> > }\n> >\n> > +void IPU3CameraData::frameStart(uint32_t sequence)\n>\n> Maybe a small comment block on the function\n>\n> /*\n> * Handle the start of frame exposure signal.\n> *\n> * Inspect the list of pending requests waiting for a RAW frame to be\n> * produced and apply controls for the 'next' one.\n> *\n> * Some controls need to be applied immediately, such as the\n> * TestPatternMode one. Other controls are handled through the delayed\n> * controls class.\n> */\n>\n> This shows that this functions does two things:\n> - Apply some libcamera::controls from the Request immediately\n> - Inform the DelayedControl class that a new frame has started to\n> advance its internal frame-counting\n>\n> > +{\n> > + if (!processingRequests_.empty()) {\n> > + /* Handle controls which are to be set ready for the next frame to start. */\n>\n> /* Apply controls that have to take effect immediately. */\n>\n> I now wonder how did we get to the notion that some controls take\n> effect 'immediately'. Did we get here simply because we had to way to\n> apply libcamera::controls to CameraSensor without going through the\n> IPA ? I then wonder if talking about 'immediate' and 'next frame' is\n> not misleading. Aren't we just applying some controls which the\n> pipelinehandler is in charge of bypassing the IPA ?\n>\n> do we need a notion of immediate controls ?\n\nIt effects immediately that a control is applied to a capture which\nassociates the control.\nI think the time of applying the control is dependent on camera hardware.\n\n\n>\n> > + Request *request = processingRequests_.front();\n> > + processingRequests_.pop();\n> > +\n> > + /* Assumes applying the test pattern mode affects immediately. */\n> > + if (request->controls().contains(controls::draft::TestPatternMode)) {\n>\n> if (!request->controls().contains(controls::draft::TestPatternMode))\n> break;\n>\n> And reduce the indendation below\n>\n> > + const int32_t testPatternMode = request->controls().get(\n> > + controls::draft::TestPatternMode);\n> > +\n> > + LOG(IPU3, Debug) << \"Apply test pattern mode: \"\n> > + << testPatternMode;\n> > +\n> > + int ret = cio2_.sensor()->setTestPatternMode(\n> > + static_cast(\n> > + testPatternMode));\n> > + if (ret) {\n> > + LOG(IPU3, Error) << \"Failed to set test pattern mode: \"\n> > + << ret;\n> break;\n> And remove the else {} clause\n>\n\nhmm, break cannot be used because this is neither for nor while.\nI think the later delayedCtrls can be invoked in the begging of the\nfunction and then can use \"return\"?\n\n-Hiro\n> > + } else {\n> > + request->metadata().set(controls::draft::TestPatternMode,\n> > + testPatternMode);\n> > + }\n> > + }\n> > + }\n> > +\n> > + /* Controls that don't affect immediately are applied in delayedCtrls. */\n>\n> I would drop this with the block at the beginning of the function\n>\n> > + delayedCtrls_->applyControls(sequence);\n>\n> Minors apart\n>\n> Reviewed-by: Jacopo Mondi \n>\n> Thanks\n> j\n>\n> > +}\n> > +\n> > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)\n> >\n> > } /* namespace libcamera */\n> > --\n> > 2.34.0.rc2.393.gf8c9666880-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 74F51BF415\n\tfor ;\n\tFri, 26 Nov 2021 04:42:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C8D9B6049A;\n\tFri, 26 Nov 2021 05:42:17 +0100 (CET)","from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com\n\t[IPv6:2a00:1450:4864:20::52c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4433D6011D\n\tfor ;\n\tFri, 26 Nov 2021 05:42:16 +0100 (CET)","by mail-ed1-x52c.google.com with SMTP id r25so33558438edq.7\n\tfor ;\n\tThu, 25 Nov 2021 20:42:16 -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=\"Wn69uLZO\"; 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=CkHqlfZDBdg745t3ac2NAxPSXdOj+2c7SksfzVLrSXc=;\n\tb=Wn69uLZOdvBHKRbnYs70N4gRpjn8+xKsqjkYRDDp1N0WK9l3t3k+m1T89zYRY7Wj2D\n\tkjvZhjLBLNXV2OMx4BqNxWrh7vzovH2GfjJ+6Tf7YM6lUThS6KcymAVUWQMrP25OFUH7\n\t5aouJdyMba6uEnn+cQ5DC1wHWKwUtgSh/Q7fQ=","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=CkHqlfZDBdg745t3ac2NAxPSXdOj+2c7SksfzVLrSXc=;\n\tb=MopmlvaoCV39XlUrVibkmfAURdTeKgnyvNGmPc8xDPTE31WCYS6B0WBC2qnj/nsri4\n\tmk1X4xHAKAKQbtjF68d5zVrY5dicDt/0iHUw4zk2Ny2V2fHrNiuCpNguouFXs4isDXci\n\t4j/02fZqVqNLs3HI2zOBjqwFXG3VBJjojiLtJtzyu0+j92YfnVHM/dEuErNLRP6wryQJ\n\tJgR6A1c0CoIUZ/bTUVLopkI6UfvSHWyuQuoeU57I6wQAf3xA5ZzC5ToZfNM1ZqyaJDJB\n\t7gWC12mPEoGh9S+XnVo20YxzdaemIf7ETZ+e32zFOC09LFrubA1pvhb0g1Xs6QqokSk2\n\tUZkg==","X-Gm-Message-State":"AOAM530al+6y0GpRwDCtF+WjyRSWFXcYgiaw322/RVBNvujUBelsaKAK\n\tuUV53dcvymtQ03BrTK0E9aAZ0e0WF2kqwjbZsrPIwtAyZjM=","X-Google-Smtp-Source":"ABdhPJxFeosxzAg4odRklDgNp1tEFLaYuSopRLnTCezVNg5tmnylYpcEpg4quE12aBz6EYpDqA5Uj1iaYQJ5QCZyQ54=","X-Received":"by 2002:aa7:c406:: with SMTP id j6mr45712972edq.76.1637901735805;\n\tThu, 25 Nov 2021 20:42:15 -0800 (PST)","MIME-Version":"1.0","References":"<20211123190812.69805-1-hiroh@chromium.org>\n\t<20211123190812.69805-4-hiroh@chromium.org>\n\t<20211125000150.u47qhsdu7qy2cyfe@uno.localdomain>","In-Reply-To":"<20211125000150.u47qhsdu7qy2cyfe@uno.localdomain>","From":"Hirokazu Honda ","Date":"Fri, 26 Nov 2021 13:42:05 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v7 3/3] libcamera: pipeline: ipu3:\n\tApply a 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":21270,"web_url":"https://patchwork.libcamera.org/comment/21270/","msgid":"<20211126082704.qwmygsfp27njngkd@uno.localdomain>","date":"2021-11-26T08:27:04","subject":"Re: [libcamera-devel] [PATCH v7 3/3] libcamera: pipeline: ipu3:\n\tApply a requested test pattern mode","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Fri, Nov 26, 2021 at 01:42:05PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for reviewing.\n>\n> On Thu, Nov 25, 2021 at 9:00 AM Jacopo Mondi wrote:\n> >\n> > Hi Hiro,\n> >\n> > On Wed, Nov 24, 2021 at 04:08:12AM +0900, Hirokazu Honda wrote:\n> > > This enables ipu3 pipeline handler to apply a test pattern mode\n> > > per frame.\n> >\n> > Yes, and this also introduces a a way to set controls\n> > immediately, the first (and only one for now) is the test pattern\n> > mode.\n> >\n> > Do you think it would be mentioned ?\n> >\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > Reviewed-by: Jean-Michel Hautbois \n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 45 ++++++++++++++++++++++++++--\n> > > 1 file changed, 43 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 25490dcf..a15c6466 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> > > @@ -76,7 +77,10 @@ public:\n> > >\n> > > std::unique_ptr ipa_;\n> > >\n> > > + /* Requests before queueing cio2 device. */\n> > > std::queue pendingRequests_;\n> > > + /* Requests queued in cio2 device and before passing imgu device. */\n> > > + std::queue processingRequests_;\n> > >\n> > > ControlInfoMap ipaControls_;\n> > >\n> > > @@ -803,6 +807,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > >\n> > > ret |= data->imgu_->stop();\n> > > ret |= data->cio2_.stop();\n> > > +\n> >\n> > Intentional ?\n> >\n> > > if (ret)\n> > > LOG(IPU3, Warning) << \"Failed to stop camera \" << camera->id();\n> > >\n> > > @@ -823,6 +828,8 @@ void IPU3CameraData::cancelPendingRequests()\n> > > pipe()->completeRequest(request);\n> > > pendingRequests_.pop();\n> > > }\n> > > +\n> > > + processingRequests_ = {};\n> >\n> > Should processingRequests_ be completed one by one as the pending ones\n> > instead of being dropped ?\n> >\n>\n> Hmm, this is a good question.\n> I think the metadata for aborted capture doesn't have to be applied to a camera.\n> So I just discard pending requests.\n>\n\nSo we'll lose requests when the Camera is stopped. Please have a look\nat IPU3CameraData::cancelPendingRequests(). I think we'll have to do\nthe same for processingRequests_\n\n> > > }\n> > >\n> > > void IPU3CameraData::queuePendingRequests()\n> > > @@ -853,6 +860,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> > > @@ -1121,8 +1130,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> > > /* Convert the sensor rotation to a transformation */\n> > > int32_t rotation = 0;\n> > > @@ -1414,6 +1423,38 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > > ipa_->processEvent(ev);\n> > > }\n> > >\n> > > +void IPU3CameraData::frameStart(uint32_t sequence)\n> >\n> > Maybe a small comment block on the function\n> >\n> > /*\n> > * Handle the start of frame exposure signal.\n> > *\n> > * Inspect the list of pending requests waiting for a RAW frame to be\n> > * produced and apply controls for the 'next' one.\n> > *\n> > * Some controls need to be applied immediately, such as the\n> > * TestPatternMode one. Other controls are handled through the delayed\n> > * controls class.\n> > */\n> >\n> > This shows that this functions does two things:\n> > - Apply some libcamera::controls from the Request immediately\n> > - Inform the DelayedControl class that a new frame has started to\n> > advance its internal frame-counting\n> >\n> > > +{\n> > > + if (!processingRequests_.empty()) {\n> > > + /* Handle controls which are to be set ready for the next frame to start. */\n> >\n> > /* Apply controls that have to take effect immediately. */\n> >\n> > I now wonder how did we get to the notion that some controls take\n> > effect 'immediately'. Did we get here simply because we had to way to\n> > apply libcamera::controls to CameraSensor without going through the\n> > IPA ? I then wonder if talking about 'immediate' and 'next frame' is\n> > not misleading. Aren't we just applying some controls which the\n> > pipelinehandler is in charge of bypassing the IPA ?\n> >\n> > do we need a notion of immediate controls ?\n>\n> It effects immediately that a control is applied to a capture which\n> associates the control.\n> I think the time of applying the control is dependent on camera hardware.\n>\n>\n> >\n> > > + Request *request = processingRequests_.front();\n> > > + processingRequests_.pop();\n> > > +\n> > > + /* Assumes applying the test pattern mode affects immediately. */\n> > > + if (request->controls().contains(controls::draft::TestPatternMode)) {\n> >\n> > if (!request->controls().contains(controls::draft::TestPatternMode))\n> > break;\n> >\n> > And reduce the indendation below\n> >\n> > > + const int32_t testPatternMode = request->controls().get(\n> > > + controls::draft::TestPatternMode);\n> > > +\n> > > + LOG(IPU3, Debug) << \"Apply test pattern mode: \"\n> > > + << testPatternMode;\n> > > +\n> > > + int ret = cio2_.sensor()->setTestPatternMode(\n> > > + static_cast(\n> > > + testPatternMode));\n> > > + if (ret) {\n> > > + LOG(IPU3, Error) << \"Failed to set test pattern mode: \"\n> > > + << ret;\n> > break;\n> > And remove the else {} clause\n> >\n>\n> hmm, break cannot be used because this is neither for nor while.\n\nAh ups\n\n> I think the later delayedCtrls can be invoked in the begging of the\n> function and then can use \"return\"?\n\nThat would be nice, thanks\n\n>\n> -Hiro\n> > > + } else {\n> > > + request->metadata().set(controls::draft::TestPatternMode,\n> > > + testPatternMode);\n> > > + }\n> > > + }\n> > > + }\n> > > +\n> > > + /* Controls that don't affect immediately are applied in delayedCtrls. */\n> >\n> > I would drop this with the block at the beginning of the function\n> >\n> > > + delayedCtrls_->applyControls(sequence);\n> >\n> > Minors apart\n> >\n> > Reviewed-by: Jacopo Mondi \n> >\n> > Thanks\n> > j\n> >\n> > > +}\n> > > +\n> > > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)\n> > >\n> > > } /* namespace libcamera */\n> > > --\n> > > 2.34.0.rc2.393.gf8c9666880-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 7893EBF415\n\tfor ;\n\tFri, 26 Nov 2021 08:26:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EDF216049B;\n\tFri, 26 Nov 2021 09:26:12 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5ADC860227\n\tfor ;\n\tFri, 26 Nov 2021 09:26:12 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id D798BC000D;\n\tFri, 26 Nov 2021 08:26:11 +0000 (UTC)"],"Date":"Fri, 26 Nov 2021 09:27:04 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211126082704.qwmygsfp27njngkd@uno.localdomain>","References":"<20211123190812.69805-1-hiroh@chromium.org>\n\t<20211123190812.69805-4-hiroh@chromium.org>\n\t<20211125000150.u47qhsdu7qy2cyfe@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v7 3/3] libcamera: pipeline: ipu3:\n\tApply a 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\" "}}]