[{"id":18053,"web_url":"https://patchwork.libcamera.org/comment/18053/","msgid":"<32d54e42-5cb4-55f4-627b-60f34d718614@ideasonboard.com>","date":"2021-07-09T13:42:26","subject":"Re: [libcamera-devel] [PATCH v5 6/8] pipeline: raspberrypi: Use\n\tpriority write for vblank when writing sensor ctrls","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 09/07/2021 10:56, Naushir Patuck wrote:\n> When directly writing controls to the sensor device, ensure that VBLANK is\n> written ahead of and before the EXPOSURE control. This is the same priority\n> write mechanism used in DelayedControls.\n\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 37 ++++++++++++++-----\n> 1 file changed, 27 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 082eb1ee1c23..53a30cff1864 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -154,6 +154,7 @@ public:\n> \tvoid embeddedComplete(uint32_t bufferId);\n> \tvoid setIspControls(const ControlList &controls);\n> \tvoid setDelayedControls(const ControlList &controls);\n> +\tvoid setSensorControls(ControlList &controls);\n> \n> \t/* bufferComplete signal handlers. */\n> \tvoid unicamBufferDequeue(FrameBuffer *buffer);\n> @@ -828,7 +829,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> \n> \t/* Apply any gain/exposure settings that the IPA may have passed back. */\n> \tif (!startConfig.controls.empty())\n> -\t\tdata->unicam_[Unicam::Image].dev()->setControls(&startConfig.controls);\n> +\t\tdata->setSensorControls(startConfig.controls);\n> \n> \t/* Configure the number of dropped frames required on startup. */\n> \tdata->dropFrameCount_ = startConfig.dropFrameCount;\n> @@ -1294,22 +1295,20 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t\treturn -EPIPE;\n> \t}\n> \n> -\tif (!controls.empty())\n> -\t\tunicam_[Unicam::Image].dev()->setControls(&controls);\n> -\n> \t/*\n> \t * Configure the H/V flip controls based on the combination of\n> \t * the sensor and user transform.\n> \t */\n> \tif (supportsFlips_) {\n> -\t\tControlList ctrls(unicam_[Unicam::Image].dev()->controls());\n\nHrm ... ctrls is initialised from the unicam dev, but I can't see where\ncontrols gets initialised...\n\nIs that an issue?\n\nHrm - it looks like it might be constructed with entityControls, which\nis both unicam and ISP controls, so I guess it gets there from those.\n\n\n\n> -\t\tctrls.set(V4L2_CID_HFLIP,\n> -\t\t\t static_cast(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));\n> -\t\tctrls.set(V4L2_CID_VFLIP,\n> -\t\t\t static_cast(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));\n> -\t\tunicam_[Unicam::Image].dev()->setControls(&ctrls);\n> +\t\tcontrols.set(V4L2_CID_HFLIP,\n> +\t\t\t static_cast(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));\n> +\t\tcontrols.set(V4L2_CID_VFLIP,\n> +\t\t\t static_cast(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));\n> \t}\n> \n> +\tif (!controls.empty())\n> +\t\tsetSensorControls(controls);\n> +\n> \treturn 0;\n> }\n> \n> @@ -1379,6 +1378,24 @@ void RPiCameraData::setDelayedControls(const ControlList &controls)\n> \thandleState();\n> }\n> \n> +void RPiCameraData::setSensorControls(ControlList &controls)\n> +{\n> +\t/*\n> +\t * We need to ensure that if both VBLANK and EXPOSURE are present, the\n> +\t * former must be written ahead of, and separately from EXPOSURE to avoid\n> +\t * V4L2 rejecting the latter. This is identical to what DelayedControls\n> +\t * does with the priority write flag.\n> +\t */\n> +\tif (controls.contains(V4L2_CID_EXPOSURE) && controls.contains(V4L2_CID_VBLANK)) {\n> +\t\tControlList vblank_ctrl;\n> +\n> +\t\tvblank_ctrl.set(V4L2_CID_VBLANK, controls.get(V4L2_CID_VBLANK));\n> +\t\tunicam_[Unicam::Image].dev()->setControls(&vblank_ctrl);\n\nDoes controls need to have V4L2_CID_VBLANK removed from the control list\nto stop it being set twice? (once above, and once below?\n\nWith that done, or if it's not actually needed:\n\nReviewed-by: Kieran Bingham \n\n\n> +\t}\n> +\n> +\tunicam_[Unicam::Image].dev()->setControls(&controls);\n> +}\n> +\n> void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> {\n> \tRPi::Stream *stream = nullptr;\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 3E268BD794\n\tfor ;\n\tFri, 9 Jul 2021 13:42:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 51C85684E7;\n\tFri, 9 Jul 2021 15:42:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EB65C605AC\n\tfor ;\n\tFri, 9 Jul 2021 15:42:28 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9F923E7;\n\tFri, 9 Jul 2021 15:42:28 +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=\"gP1DwFp8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625838148;\n\tbh=vdf9RrRsD7nRGzdVz9YZ+K1qGe8vJXDArbH+VtaOHsY=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=gP1DwFp8HYZputZuq4p40wihRyJ2AZoE/ExVpAfJ9u/UveUPl5LZFyneiq7TzOMhR\n\tti97lew8utAyGTmVdcBcd5FZHeQaqZmpx1FFDGg8hM0vAaFiauvVuE3+DOs1CP/2zy\n\t5/uCAwA5dpDJIiIBTFIKFG/tJ5RWp7hvCaoh2am0=","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210709095638.2801713-1-naush@raspberrypi.com>\n\t<20210709095638.2801713-6-naush@raspberrypi.com>","From":"Kieran Bingham ","Message-ID":"<32d54e42-5cb4-55f4-627b-60f34d718614@ideasonboard.com>","Date":"Fri, 9 Jul 2021 14:42:26 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210709095638.2801713-6-naush@raspberrypi.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v5 6/8] pipeline: raspberrypi: Use\n\tpriority write for vblank when writing sensor ctrls","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":18056,"web_url":"https://patchwork.libcamera.org/comment/18056/","msgid":"","date":"2021-07-09T14:08:15","subject":"Re: [libcamera-devel] [PATCH v5 6/8] pipeline: raspberrypi: Use\n\tpriority write for vblank when writing sensor ctrls","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nOn Fri, 9 Jul 2021 at 14:42, Kieran Bingham \nwrote:\n\n> Hi Naush,\n>\n> On 09/07/2021 10:56, Naushir Patuck wrote:\n> > When directly writing controls to the sensor device, ensure that VBLANK\n> is\n> > written ahead of and before the EXPOSURE control. This is the same\n> priority\n> > write mechanism used in DelayedControls.\n>\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 37 ++++++++++++++-----\n> > 1 file changed, 27 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 082eb1ee1c23..53a30cff1864 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -154,6 +154,7 @@ public:\n> > void embeddedComplete(uint32_t bufferId);\n> > void setIspControls(const ControlList &controls);\n> > void setDelayedControls(const ControlList &controls);\n> > + void setSensorControls(ControlList &controls);\n> >\n> > /* bufferComplete signal handlers. */\n> > void unicamBufferDequeue(FrameBuffer *buffer);\n> > @@ -828,7 +829,7 @@ int PipelineHandlerRPi::start(Camera *camera, const\n> ControlList *controls)\n> >\n> > /* Apply any gain/exposure settings that the IPA may have passed\n> back. */\n> > if (!startConfig.controls.empty())\n> > -\n> data->unicam_[Unicam::Image].dev()->setControls(&startConfig.controls);\n> > + data->setSensorControls(startConfig.controls);\n> >\n> > /* Configure the number of dropped frames required on startup. */\n> > data->dropFrameCount_ = startConfig.dropFrameCount;\n> > @@ -1294,22 +1295,20 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> > return -EPIPE;\n> > }\n> >\n> > - if (!controls.empty())\n> > - unicam_[Unicam::Image].dev()->setControls(&controls);\n> > -\n> > /*\n> > * Configure the H/V flip controls based on the combination of\n> > * the sensor and user transform.\n> > */\n> > if (supportsFlips_) {\n> > - ControlList\n> ctrls(unicam_[Unicam::Image].dev()->controls());\n>\n> Hrm ... ctrls is initialised from the unicam dev, but I can't see where\n> controls gets initialised...\n>\n> Is that an issue?\n>\n> Hrm - it looks like it might be constructed with entityControls, which\n> is both unicam and ISP controls, so I guess it gets there from those.\n>\n>\n>\n> > - ctrls.set(V4L2_CID_HFLIP,\n> > -\n> static_cast(!!(rpiConfig->combinedTransform_ &\n> Transform::HFlip)));\n> > - ctrls.set(V4L2_CID_VFLIP,\n> > -\n> static_cast(!!(rpiConfig->combinedTransform_ &\n> Transform::VFlip)));\n> > - unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > + controls.set(V4L2_CID_HFLIP,\n> > +\n> static_cast(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));\n> > + controls.set(V4L2_CID_VFLIP,\n> > +\n> static_cast(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));\n> > }\n> >\n> > + if (!controls.empty())\n> > + setSensorControls(controls);\n> > +\n> > return 0;\n> > }\n> >\n> > @@ -1379,6 +1378,24 @@ void RPiCameraData::setDelayedControls(const\n> ControlList &controls)\n> > handleState();\n> > }\n> >\n> > +void RPiCameraData::setSensorControls(ControlList &controls)\n> > +{\n> > + /*\n> > + * We need to ensure that if both VBLANK and EXPOSURE are present,\n> the\n> > + * former must be written ahead of, and separately from EXPOSURE\n> to avoid\n> > + * V4L2 rejecting the latter. This is identical to what\n> DelayedControls\n> > + * does with the priority write flag.\n> > + */\n> > + if (controls.contains(V4L2_CID_EXPOSURE) &&\n> controls.contains(V4L2_CID_VBLANK)) {\n> > + ControlList vblank_ctrl;\n> > +\n> > + vblank_ctrl.set(V4L2_CID_VBLANK,\n> controls.get(V4L2_CID_VBLANK));\n> > + unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl);\n>\n> Does controls need to have V4L2_CID_VBLANK removed from the control list\n> to stop it being set twice? (once above, and once below?\n>\n\nThis is a bit of a tricky one. Ideally, I want to remove V4L2_CID_VBLANK,\nbut we don't have\na ControlList::Erase() type method. I could create a whole new ControlList\nby iterating over\ncontrols and adding all but V4L2_CID_VBLANK. This seems a bit inefficient.\n\nInstead, I am relying on the v4l2 framework to not pass on the control to\nthe driver if the value\nhas not changed from the last set value, which is what happens here. Do\nyou think that's a bit\nugly, and perhaps I should not rely on that?\n\nRegards,\nNaush\n\n\n>\n> With that done, or if it's not actually needed:\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> > + }\n> > +\n> > + unicam_[Unicam::Image].dev()->setControls(&controls);\n> > +}\n> > +\n> > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > {\n> > RPi::Stream *stream = nullptr;\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6A076BD794\n\tfor ;\n\tFri, 9 Jul 2021 14:08:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3ECEF6851D;\n\tFri, 9 Jul 2021 16:08:33 +0200 (CEST)","from mail-lj1-x22a.google.com (mail-lj1-x22a.google.com\n\t[IPv6:2a00:1450:4864:20::22a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 06560605AC\n\tfor ;\n\tFri, 9 Jul 2021 16:08:32 +0200 (CEST)","by mail-lj1-x22a.google.com with SMTP id s18so8221443ljg.7\n\tfor ;\n\tFri, 09 Jul 2021 07:08:31 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"lisyB8yY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=vTX/wgNUg7ITot4BnzZa1nfGfaft6QimlFa6OppDcdY=;\n\tb=lisyB8yY+kJIMlNVrt5NCI5A8yHkyMyB+CvVxhYwfx9pPGqvo6gr0JnfnawZFHm+Fh\n\tsl9pc8pRx0SKXR1tqV9LTnrddIKW/OUovFsfHJZE6p7JBwGHk+C5U+LHaY4yNZjaRv1i\n\tYr84ZiyfJhuovxfGw3t0/OReN4cZsGrMDaMfrn35cz3YAO28eKnStKh/fXrh3aI4OS01\n\t0mUaicJJP1KkcjMmswdnG6xe6PrqkdYn7Bs7V94U9VFUb2eHIF0oJVuls4C7GzRMUXwm\n\tKH/yMrrM9huTZ+xYtVPRppFOVB/kyjNQo5t2iP2CLHnbyxphpBGbg9WSsC/cciVUzKC+\n\tLAOA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=vTX/wgNUg7ITot4BnzZa1nfGfaft6QimlFa6OppDcdY=;\n\tb=EPFqbE/wCN6LVDFSm6wO31KDS6DY0ynEwkT5dlnc5fiM66gxhV/3FN19JwPoF5nMOe\n\trmbDsGzyggi1itny7nVQ+KKV4aoYHVBc0E4ILyUejnaSfaJPNixTjeQqlvFpOiImCqCj\n\tejT3iztK8XOfYdLTRyqNf222vThBmYBn0aw3oRaPPxqB+tPQFmamwPz7I0bu0/BJ8air\n\tfPEdBk9l1Bm6kwojMybTWQeQ7Rc6NALslHpAB3IyohYatlQJr7bEHp0x1VseqzFs7IMr\n\tEzvth9YGHMVyrbMRktPe6rT84OoUeaPN3JCyPBbN2FxGZXAH3PRaoCJUE1HXDBvXX81i\n\tVUjg==","X-Gm-Message-State":"AOAM532f1qZQ8TRQMJVTGsg6em3BGLcPmLxYyb2epV1TEfnkACQTpCu+\n\taHgi5Cf2IOv0yqhXmLA8TycbGFSrOE3pfLMfhR4DBAfEFMJpyg==","X-Google-Smtp-Source":"ABdhPJzEangbVHOJlvuVvDBzy2+ShqBbbI4z4RPapt+58zvTgNWJGOm0knZmF9skldqWaaEGJegOp5eJ+NhmcM7c+FA=","X-Received":"by 2002:a2e:9910:: with SMTP id\n\tv16mr10931484lji.499.1625839710991; \n\tFri, 09 Jul 2021 07:08:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20210709095638.2801713-1-naush@raspberrypi.com>\n\t<20210709095638.2801713-6-naush@raspberrypi.com>\n\t<32d54e42-5cb4-55f4-627b-60f34d718614@ideasonboard.com>","In-Reply-To":"<32d54e42-5cb4-55f4-627b-60f34d718614@ideasonboard.com>","From":"Naushir Patuck ","Date":"Fri, 9 Jul 2021 15:08:15 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"0000000000006f885805c6b14e92\"","Subject":"Re: [libcamera-devel] [PATCH v5 6/8] pipeline: raspberrypi: Use\n\tpriority write for vblank when writing sensor ctrls","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18058,"web_url":"https://patchwork.libcamera.org/comment/18058/","msgid":"<27d98e61-8ed1-8cf6-7dcb-8547ca1c4aa9@ideasonboard.com>","date":"2021-07-09T14:16:00","subject":"Re: [libcamera-devel] [PATCH v5 6/8] pipeline: raspberrypi: Use\n\tpriority write for vblank when writing sensor ctrls","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 09/07/2021 15:08, Naushir Patuck wrote:\n> Hi Kieran,\n> \n> On Fri, 9 Jul 2021 at 14:42, Kieran Bingham\n> > wrote:\n> \n> Hi Naush,\n> \n> On 09/07/2021 10:56, Naushir Patuck wrote:\n> > When directly writing controls to the sensor device, ensure that\n> VBLANK is\n> > written ahead of and before the EXPOSURE control. This is the same\n> priority\n> > write mechanism used in DelayedControls.\n> \n> > Signed-off-by: Naushir Patuck >\n> > Reviewed-by: David Plowman >\n> > ---\n> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 37\n> ++++++++++++++-----\n> >  1 file changed, 27 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 082eb1ee1c23..53a30cff1864 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -154,6 +154,7 @@ public:\n> >       void embeddedComplete(uint32_t bufferId);\n> >       void setIspControls(const ControlList &controls);\n> >       void setDelayedControls(const ControlList &controls);\n> > +     void setSensorControls(ControlList &controls);\n> > \n> >       /* bufferComplete signal handlers. */\n> >       void unicamBufferDequeue(FrameBuffer *buffer);\n> > @@ -828,7 +829,7 @@ int PipelineHandlerRPi::start(Camera *camera,\n> const ControlList *controls)\n> > \n> >       /* Apply any gain/exposure settings that the IPA may have\n> passed back. */\n> >       if (!startConfig.controls.empty())\n> > -           \n>  data->unicam_[Unicam::Image].dev()->setControls(&startConfig.controls);\n> > +             data->setSensorControls(startConfig.controls);\n> > \n> >       /* Configure the number of dropped frames required on\n> startup. */\n> >       data->dropFrameCount_ = startConfig.dropFrameCount;\n> > @@ -1294,22 +1295,20 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> >               return -EPIPE;\n> >       }\n> > \n> > -     if (!controls.empty())\n> > -             unicam_[Unicam::Image].dev()->setControls(&controls);\n> > -\n> >       /*\n> >        * Configure the H/V flip controls based on the combination of\n> >        * the sensor and user transform.\n> >        */\n> >       if (supportsFlips_) {\n> > -             ControlList\n> ctrls(unicam_[Unicam::Image].dev()->controls());\n> \n> Hrm ... ctrls is initialised from the unicam dev, but I can't see where\n> controls gets initialised...\n> \n> Is that an issue?\n> \n> Hrm - it looks like it might be constructed with entityControls, which\n> is both unicam and ISP controls, so I guess it gets there from those.\n> \n> \n> \n> > -             ctrls.set(V4L2_CID_HFLIP,\n> > -                     \n>  static_cast(!!(rpiConfig->combinedTransform_ &\n> Transform::HFlip)));\n> > -             ctrls.set(V4L2_CID_VFLIP,\n> > -                     \n>  static_cast(!!(rpiConfig->combinedTransform_ &\n> Transform::VFlip)));\n> > -             unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > +             controls.set(V4L2_CID_HFLIP,\n> > +                         \n> static_cast(!!(rpiConfig->combinedTransform_ &\n> Transform::HFlip)));\n> > +             controls.set(V4L2_CID_VFLIP,\n> > +                         \n> static_cast(!!(rpiConfig->combinedTransform_ &\n> Transform::VFlip)));\n> >       }\n> > \n> > +     if (!controls.empty())\n> > +             setSensorControls(controls);\n> > +\n> >       return 0;\n> >  }\n> > \n> > @@ -1379,6 +1378,24 @@ void\n> RPiCameraData::setDelayedControls(const ControlList &controls)\n> >       handleState();\n> >  }\n> > \n> > +void RPiCameraData::setSensorControls(ControlList &controls)\n> > +{\n> > +     /*\n> > +      * We need to ensure that if both VBLANK and EXPOSURE are\n> present, the\n> > +      * former must be written ahead of, and separately from\n> EXPOSURE to avoid\n> > +      * V4L2 rejecting the latter. This is identical to what\n> DelayedControls\n> > +      * does with the priority write flag.\n> > +      */\n> > +     if (controls.contains(V4L2_CID_EXPOSURE) &&\n> controls.contains(V4L2_CID_VBLANK)) {\n> > +             ControlList vblank_ctrl;\n> > +\n> > +             vblank_ctrl.set(V4L2_CID_VBLANK,\n> controls.get(V4L2_CID_VBLANK));\n> > +             unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl);\n> \n> Does controls need to have V4L2_CID_VBLANK removed from the control list\n> to stop it being set twice? (once above, and once below?\n> \n> \n> This is a bit of a tricky one.  Ideally, I want to remove\n> V4L2_CID_VBLANK, but we don't have\n> a ControlList::Erase() type method.  I could create a whole new\n> ControlList by iterating over\n> controls and adding all but V4L2_CID_VBLANK.  This seems a bit inefficient.\n> \n> Instead, I am relying on the v4l2 framework to not pass on the control\n> to the driver if the value\n> has not changed from the last set value, which is what happens here.  Do\n> you think that's a bit\n> ugly, and perhaps I should not rely on that?\n\nOh I see ...\n\nIndeed, I don't think we should copy the lists just to erase a value...\n\nSo I suspect we either need to implement an erase function on a control\nlist control - or ... (perhaps the simpler short term option) put an\nexplicit comment here stating that it relies on the behaviour you've\ndescribed so it's not implicit ...\n\n\n> Regards,\n> Naush\n>  \n> \n> \n> With that done, or if it's not actually needed:\n> \n> Reviewed-by: Kieran Bingham >\n> \n> \n> > +     }\n> > +\n> > +     unicam_[Unicam::Image].dev()->setControls(&controls);\n> > +}\n> > +\n> >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> >  {\n> >       RPi::Stream *stream = nullptr;\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 712B5BD794\n\tfor ;\n\tFri, 9 Jul 2021 14:16:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E408D6851B;\n\tFri, 9 Jul 2021 16:16:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EF7F1605AC\n\tfor ;\n\tFri, 9 Jul 2021 16:16:03 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 88708E7;\n\tFri, 9 Jul 2021 16:16: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=\"I6Q2Bdnw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625840163;\n\tbh=B8UXoSJR96pul/e8ImepSeHdYz9jc50X4q8EULVYRXE=;\n\th=From:Subject:To:Cc:References:Date:In-Reply-To:From;\n\tb=I6Q2Bdnw9vfgi5jKnzff8KpAZNEm3Gw0VuoWwRfhDVTB+j25qQZokEAFWxAReCueL\n\tJz1J9Hy/gabaME9cmkk31VKXX6Uy7d+IcP3TJPGbdF1x1zjdnUdBoQIMJRwF0IjE/B\n\ttkCGgCn5TZRnVNpqLVWycGYW7VOs8gaXe3epPlao=","From":"Kieran Bingham ","To":"Naushir Patuck ","References":"<20210709095638.2801713-1-naush@raspberrypi.com>\n\t<20210709095638.2801713-6-naush@raspberrypi.com>\n\t<32d54e42-5cb4-55f4-627b-60f34d718614@ideasonboard.com>\n\t","Message-ID":"<27d98e61-8ed1-8cf6-7dcb-8547ca1c4aa9@ideasonboard.com>","Date":"Fri, 9 Jul 2021 15:16:00 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v5 6/8] pipeline: raspberrypi: Use\n\tpriority write for vblank when writing sensor ctrls","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18060,"web_url":"https://patchwork.libcamera.org/comment/18060/","msgid":"","date":"2021-07-09T14:23:13","subject":"Re: [libcamera-devel] [PATCH v5 6/8] pipeline: raspberrypi: Use\n\tpriority write for vblank when writing sensor ctrls","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Fri, 9 Jul 2021 at 15:16, Kieran Bingham \nwrote:\n\n> Hi Naush,\n>\n> On 09/07/2021 15:08, Naushir Patuck wrote:\n> > Hi Kieran,\n> >\n> > On Fri, 9 Jul 2021 at 14:42, Kieran Bingham\n> > > > wrote:\n> >\n> > Hi Naush,\n> >\n> > On 09/07/2021 10:56, Naushir Patuck wrote:\n> > > When directly writing controls to the sensor device, ensure that\n> > VBLANK is\n> > > written ahead of and before the EXPOSURE control. This is the same\n> > priority\n> > > write mechanism used in DelayedControls.\n> >\n> > > Signed-off-by: Naushir Patuck > >\n> > > Reviewed-by: David Plowman > >\n> > > ---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 37\n> > ++++++++++++++-----\n> > > 1 file changed, 27 insertions(+), 10 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 082eb1ee1c23..53a30cff1864 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -154,6 +154,7 @@ public:\n> > > void embeddedComplete(uint32_t bufferId);\n> > > void setIspControls(const ControlList &controls);\n> > > void setDelayedControls(const ControlList &controls);\n> > > + void setSensorControls(ControlList &controls);\n> > >\n> > > /* bufferComplete signal handlers. */\n> > > void unicamBufferDequeue(FrameBuffer *buffer);\n> > > @@ -828,7 +829,7 @@ int PipelineHandlerRPi::start(Camera *camera,\n> > const ControlList *controls)\n> > >\n> > > /* Apply any gain/exposure settings that the IPA may have\n> > passed back. */\n> > > if (!startConfig.controls.empty())\n> > > -\n> >\n> data->unicam_[Unicam::Image].dev()->setControls(&startConfig.controls);\n> > > + data->setSensorControls(startConfig.controls);\n> > >\n> > > /* Configure the number of dropped frames required on\n> > startup. */\n> > > data->dropFrameCount_ = startConfig.dropFrameCount;\n> > > @@ -1294,22 +1295,20 @@ int RPiCameraData::configureIPA(const\n> > CameraConfiguration *config)\n> > > return -EPIPE;\n> > > }\n> > >\n> > > - if (!controls.empty())\n> > > - unicam_[Unicam::Image].dev()->setControls(&controls);\n> > > -\n> > > /*\n> > > * Configure the H/V flip controls based on the combination\n> of\n> > > * the sensor and user transform.\n> > > */\n> > > if (supportsFlips_) {\n> > > - ControlList\n> > ctrls(unicam_[Unicam::Image].dev()->controls());\n> >\n> > Hrm ... ctrls is initialised from the unicam dev, but I can't see\n> where\n> > controls gets initialised...\n> >\n> > Is that an issue?\n> >\n> > Hrm - it looks like it might be constructed with entityControls,\n> which\n> > is both unicam and ISP controls, so I guess it gets there from those.\n> >\n> >\n> >\n> > > - ctrls.set(V4L2_CID_HFLIP,\n> > > -\n> > static_cast(!!(rpiConfig->combinedTransform_ &\n> > Transform::HFlip)));\n> > > - ctrls.set(V4L2_CID_VFLIP,\n> > > -\n> > static_cast(!!(rpiConfig->combinedTransform_ &\n> > Transform::VFlip)));\n> > > - unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > > + controls.set(V4L2_CID_HFLIP,\n> > > +\n> > static_cast(!!(rpiConfig->combinedTransform_ &\n> > Transform::HFlip)));\n> > > + controls.set(V4L2_CID_VFLIP,\n> > > +\n> > static_cast(!!(rpiConfig->combinedTransform_ &\n> > Transform::VFlip)));\n> > > }\n> > >\n> > > + if (!controls.empty())\n> > > + setSensorControls(controls);\n> > > +\n> > > return 0;\n> > > }\n> > >\n> > > @@ -1379,6 +1378,24 @@ void\n> > RPiCameraData::setDelayedControls(const ControlList &controls)\n> > > handleState();\n> > > }\n> > >\n> > > +void RPiCameraData::setSensorControls(ControlList &controls)\n> > > +{\n> > > + /*\n> > > + * We need to ensure that if both VBLANK and EXPOSURE are\n> > present, the\n> > > + * former must be written ahead of, and separately from\n> > EXPOSURE to avoid\n> > > + * V4L2 rejecting the latter. This is identical to what\n> > DelayedControls\n> > > + * does with the priority write flag.\n> > > + */\n> > > + if (controls.contains(V4L2_CID_EXPOSURE) &&\n> > controls.contains(V4L2_CID_VBLANK)) {\n> > > + ControlList vblank_ctrl;\n> > > +\n> > > + vblank_ctrl.set(V4L2_CID_VBLANK,\n> > controls.get(V4L2_CID_VBLANK));\n> > > +\n> unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl);\n> >\n> > Does controls need to have V4L2_CID_VBLANK removed from the control\n> list\n> > to stop it being set twice? (once above, and once below?\n> >\n> >\n> > This is a bit of a tricky one. Ideally, I want to remove\n> > V4L2_CID_VBLANK, but we don't have\n> > a ControlList::Erase() type method. I could create a whole new\n> > ControlList by iterating over\n> > controls and adding all but V4L2_CID_VBLANK. This seems a bit\n> inefficient.\n> >\n> > Instead, I am relying on the v4l2 framework to not pass on the control\n> > to the driver if the value\n> > has not changed from the last set value, which is what happens here. Do\n> > you think that's a bit\n> > ugly, and perhaps I should not rely on that?\n>\n> Oh I see ...\n>\n> Indeed, I don't think we should copy the lists just to erase a value...\n>\n> So I suspect we either need to implement an erase function on a control\n> list control - or ... (perhaps the simpler short term option) put an\n> explicit comment here stating that it relies on the behaviour you've\n> described so it's not implicit ...\n>\n\nI'll add the comment and post an updated series shortly!\n\n\n\n>\n>\n> > Regards,\n> > Naush\n> >\n> >\n> >\n> > With that done, or if it's not actually needed:\n> >\n> > Reviewed-by: Kieran Bingham > >\n> >\n> >\n> > > + }\n> > > +\n> > > + unicam_[Unicam::Image].dev()->setControls(&controls);\n> > > +}\n> > > +\n> > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > > {\n> > > RPi::Stream *stream = nullptr;\n> > >\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BD5CCC3224\n\tfor ;\n\tFri, 9 Jul 2021 14:23:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3991B6851B;\n\tFri, 9 Jul 2021 16:23:31 +0200 (CEST)","from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 876A2605AC\n\tfor ;\n\tFri, 9 Jul 2021 16:23:29 +0200 (CEST)","by mail-lf1-x130.google.com with SMTP id t17so23668257lfq.0\n\tfor ;\n\tFri, 09 Jul 2021 07:23:29 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"gv6bl6/2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=xQ9Epc4+XzqKg7BeHqCvTXJ8uEx3TFxXOl/yl2PfYP8=;\n\tb=gv6bl6/2LdpRzaOGhyyzeLOire480MNcf6lBJBZDa8Si6yfqOCTzOhz47eJOVK8Otg\n\tMGw+8uR9tLLvcilmcU+tvSKqW/E/vDnZR/ktsSeWZCBqo+YwezArYyY4PY5EbtG6Jdze\n\tUtxxhI4rIfYjYe0zE/azub8EjIubOfx9hQT8DU6DxnKjMADRQKDF6Z7eFHdnkfeQqsMU\n\tspSTddCry+ksfqOTZHDWL0p7L51mhz2IVEj41BJ+npViUqGhpCgfA/w3xHcOtlg8ZBVG\n\tuifv85s/VIFe7i9ezpfyHmOh2TzZjmNM7/bxSxeZG2ZJEfiAmbgzlOMh99xgjIGQadIW\n\tTMEQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=xQ9Epc4+XzqKg7BeHqCvTXJ8uEx3TFxXOl/yl2PfYP8=;\n\tb=Y8wgspLbjWjJgUtjwYdMaQ/jFlONe/6BqVKgreBUqqAQMmIf/sGCnIX9Z146bUcqrK\n\tqR6xxpj9HmACrn9Rri7jukEZarmuk/lkuwqBJhvGT+YcBe4LAthGd3brsiIaZwSFLPgI\n\tWKKvja9S7qGng1TxTZauczilCjsk4GrT3Xyi/AA+kO34YVnjl7ShlPGpjVaxvYPu4UA/\n\t39/IKkfExKJyL0MbM7wB44KNZOVezuX92Ta+v1ukHmNFLiXeq4tUr90iZPk1JvhDkl4/\n\t1HhvRyVgpLivnYNHDrM7hCQYEc5nnPT2L7ooRO7mSzysPwZYTbWACnu0kd3KNC55K3yi\n\t+Auw==","X-Gm-Message-State":"AOAM530a5hzn7jhd9ST6ERQ23xB2Is/VzBlN/d/xSsiwaFRkAV4706Sb\n\tfJJEEoSZ37GTN6I897nGbHGHbltSYcsGG9s3Vmi/ZA==","X-Google-Smtp-Source":"ABdhPJw6TjNEuEMXuZyrE4GMKHylfpWaU6c+iC4iRcSssX1FCORUEbUubUwcJ+9Y9iB+smpHp1e4ZEDVZJ/DO53Eqbg=","X-Received":"by 2002:ac2:4342:: with SMTP id\n\to2mr28683332lfl.375.1625840608953; \n\tFri, 09 Jul 2021 07:23:28 -0700 (PDT)","MIME-Version":"1.0","References":"<20210709095638.2801713-1-naush@raspberrypi.com>\n\t<20210709095638.2801713-6-naush@raspberrypi.com>\n\t<32d54e42-5cb4-55f4-627b-60f34d718614@ideasonboard.com>\n\t\n\t<27d98e61-8ed1-8cf6-7dcb-8547ca1c4aa9@ideasonboard.com>","In-Reply-To":"<27d98e61-8ed1-8cf6-7dcb-8547ca1c4aa9@ideasonboard.com>","From":"Naushir Patuck ","Date":"Fri, 9 Jul 2021 15:23:13 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000f55e4b05c6b183d1\"","Subject":"Re: [libcamera-devel] [PATCH v5 6/8] pipeline: raspberrypi: Use\n\tpriority write for vblank when writing sensor ctrls","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]