[{"id":15293,"web_url":"https://patchwork.libcamera.org/comment/15293/","msgid":"<3ab509d4-5d89-d5fd-2ee1-9dc5544a49bf@fabwu.ch>","date":"2021-02-22T19:55:19","subject":"Re: [libcamera-devel] [PATCH v5] libcamera: ipu3: Add rotation to\n\tipu3 pipeline","submitter":{"id":77,"url":"https://patchwork.libcamera.org/api/people/77/","name":"Fabian Wüthrich","email":"me@fabwu.ch"},"content":"Hi Jacopo\n\nI tested you changes on my Surface Book 2 and everything works fine:\n\nAcked-by: Fabian Wüthrich \n\nOn 22.02.21 12:32, Jacopo Mondi wrote:\n> From: Fabian Wüthrich \n> \n> Use the same transformation logic as in the raspberry pipeline to\n> implement rotations in the ipu3 pipeline.\n> \n> Tested on a Surface Book 2 with an experimental driver for OV5693.\n> \n> Signed-off-by: Fabian Wüthrich \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Jacopo Mondi \n> Signed-off-by: Jacopo Mondi \n> ---\n> \n> I have re-based and re-worked this slightly\n> \n> - Rebased on master\n> - Remove snake_case variables\n> - Move H/V flip setting from the end of configure() to just before\n> the part where bail out if the request contains only raw streams, so that\n> transform can be applied on RAW images as well\n> - Minor style changes such as breaking lines more often\n> \n> Fabian, could you re-ack this, and maybe if you have a setup to do so re-test ?\n> I'll push it with your ack!\n> \n> Thanks\n> j\n> \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 87 +++++++++++++++++++++++++++-\n> 1 file changed, 85 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index b8a655ce0b68..0561a4610007 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -16,6 +16,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -75,6 +76,9 @@ public:\n> \n> \tuint32_t exposureTime_;\n> \tRectangle cropRegion_;\n> +\tbool supportsFlips_;\n> +\tTransform rotationTransform_;\n> +\n> \tstd::unique_ptr delayedCtrls_;\n> \tIPU3Frames frameInfos_;\n> \n> @@ -95,6 +99,9 @@ public:\n> \tconst StreamConfiguration &cio2Format() const { return cio2Configuration_; }\n> \tconst ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }\n> \n> +\t/* Cache the combinedTransform_ that will be applied to the sensor */\n> +\tTransform combinedTransform_;\n> +\n> private:\n> \t/*\n> \t * The IPU3CameraData instance is guaranteed to be valid as long as the\n> @@ -167,11 +174,49 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \tif (config_.empty())\n> \t\treturn Invalid;\n> \n> -\tif (transform != Transform::Identity) {\n> -\t\ttransform = Transform::Identity;\n> +\tTransform combined = transform * data_->rotationTransform_;\n> +\n> +\t/*\n> +\t * We combine the platform and user transform, but must \"adjust away\"\n> +\t * any combined result that includes a transposition, as we can't do\n> +\t * those. In this case, flipping only the transpose bit is helpful to\n> +\t * applications - they either get the transform they requested, or have\n> +\t * to do a simple transpose themselves (they don't have to worry about\n> +\t * the other possible cases).\n> +\t */\n> +\tif (!!(combined & Transform::Transpose)) {\n> +\t\t/*\n> +\t\t * Flipping the transpose bit in \"transform\" flips it in the\n> +\t\t * combined result too (as it's the last thing that happens),\n> +\t\t * which is of course clearing it.\n> +\t\t */\n> +\t\ttransform ^= Transform::Transpose;\n> +\t\tcombined &= ~Transform::Transpose;\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\t/*\n> +\t * We also check if the sensor doesn't do h/vflips at all, in which\n> +\t * case we clear them, and the application will have to do everything.\n> +\t */\n> +\tif (!data_->supportsFlips_ && !!combined) {\n> +\t\t/*\n> +\t\t * If the sensor can do no transforms, then combined must be\n> +\t\t * changed to the identity. The only user transform that gives\n> +\t\t * rise to this is the inverse of the rotation. (Recall that\n> +\t\t * combined = transform * rotationTransform.)\n> +\t\t */\n> +\t\ttransform = -data_->rotationTransform_;\n> +\t\tcombined = Transform::Identity;\n> \t\tstatus = Adjusted;\n> \t}\n> \n> +\t/*\n> +\t * Store the final combined transform that configure() will need to\n> +\t * apply to the sensor to save us working it out again.\n> +\t */\n> +\tcombinedTransform_ = combined;\n> +\n> \t/* Cap the number of entries to the available streams. */\n> \tif (config_.size() > IPU3_MAX_STREAMS) {\n> \t\tconfig_.resize(IPU3_MAX_STREAMS);\n> @@ -503,6 +548,24 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \tcio2->sensor()->sensorInfo(&sensorInfo);\n> \tdata->cropRegion_ = sensorInfo.analogCrop;\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 (data->supportsFlips_) {\n> +\t\tControlList sensorCtrls(cio2->sensor()->controls());\n> +\t\tsensorCtrls.set(V4L2_CID_HFLIP,\n> +\t\t\t\tstatic_cast(!!(config->combinedTransform_\n> +\t\t\t\t\t\t\t& Transform::HFlip)));\n> +\t\tsensorCtrls.set(V4L2_CID_VFLIP,\n> +\t\t\t\tstatic_cast(!!(config->combinedTransform_\n> +\t\t\t\t\t\t & Transform::VFlip)));\n> +\n> +\t\tret = cio2->sensor()->setControls(&sensorCtrls);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> \t/*\n> \t * If the ImgU gets configured, its driver seems to expect that\n> \t * buffers will be queued to its outputs, as otherwise the next\n> @@ -980,6 +1043,26 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\tdata->cio2_.frameStart().connect(data->delayedCtrls_.get(),\n> \t\t\t\t\t\t &DelayedControls::applyControls);\n> \n> +\t\t/* Convert the sensor rotation to a transformation */\n> +\t\tint32_t rotation = 0;\n> +\t\tif (data->properties_.contains(properties::Rotation))\n> +\t\t\trotation = data->properties_.get(properties::Rotation);\n> +\t\telse\n> +\t\t\tLOG(IPU3, Warning) << \"Rotation control not exposed by \"\n> +\t\t\t\t\t << cio2->sensor()->id()\n> +\t\t\t\t\t << \". Assume rotation 0\";\n> +\n> +\t\tbool success;\n> +\t\tdata->rotationTransform_ = transformFromRotation(rotation, &success);\n> +\t\tif (!success)\n> +\t\t\tLOG(IPU3, Warning) << \"Invalid rotation of \" << rotation\n> +\t\t\t\t\t << \" degrees: ignoring\";\n> +\n> +\t\tControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP });\n> +\t\tif (!ctrls.empty())\n> +\t\t\t/* We assume it will support vflips too... */\n> +\t\t\tdata->supportsFlips_ = true;\n> +\n> \t\t/**\n> \t\t * \\todo Dynamically assign ImgU and output devices to each\n> \t\t * stream and camera; as of now, limit support to two cameras\n> --\n> 2.30.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 28E12BD1F1\n\tfor ;\n\tMon, 22 Feb 2021 19:55:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A878968A07;\n\tMon, 22 Feb 2021 20:55:22 +0100 (CET)","from gusto4.metanet.ch (gusto4.metanet.ch [80.74.154.158])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1785768A07\n\tfor ;\n\tMon, 22 Feb 2021 20:55:20 +0100 (CET)","from [192.168.1.132] (localhost [127.0.0.1]) by gusto4.metanet.ch\n\t(Postfix) with ESMTPSA id B75464F01775; \n\tMon, 22 Feb 2021 20:55:19 +0100 (CET)"],"Authentication-Results":["lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=fabwu.ch header.i=@fabwu.ch header.b=\"VmRn5eSM\";\n\tdkim-atps=neutral","gusto.metanet.ch;\n\tspf=pass (sender IP is 62.202.181.20) smtp.mailfrom=me@fabwu.ch\n\tsmtp.helo=[192.168.1.132]"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=fabwu.ch; s=default; \n\tt=1614023719; bh=ciujw/VPpY+Htf3CHq9qoexciVxq5/FJfHnwFL0IVeY=;\n\th=Subject:To:From;\n\tb=VmRn5eSMiLmqW/MAsS+OR8MS1jh8A6uA3NiyWHlh6rfsA2OxD0QG9RzhIl+XTS84+\n\t4aFzrAlCAXILj4dn5x590g3Ct1yNbH0uc6ZC+6tyod8qGtkMcN3rAq7NvyOxdrBrjC\n\trvKICNbDBqnTNX37GwNS1+8GIVa0QSDoQxmJcNUM=","Received-SPF":"pass (gusto.metanet.ch: connection is authenticated)","To":"libcamera-devel@lists.libcamera.org","References":"<20210222113227.395737-1-jacopo@jmondi.org>","From":"=?utf-8?q?Fabian_W=C3=BCthrich?= ","Message-ID":"<3ab509d4-5d89-d5fd-2ee1-9dc5544a49bf@fabwu.ch>","Date":"Mon, 22 Feb 2021 20:55:19 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"<20210222113227.395737-1-jacopo@jmondi.org>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v5] libcamera: ipu3: Add rotation to\n\tipu3 pipeline","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15299,"web_url":"https://patchwork.libcamera.org/comment/15299/","msgid":"<20210223084942.xghjk4zymlxfw76v@uno.localdomain>","date":"2021-02-23T08:49:42","subject":"Re: [libcamera-devel] [PATCH v5] libcamera: ipu3: Add rotation to\n\tipu3 pipeline","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Mon, Feb 22, 2021 at 08:55:19PM +0100, Fabian Wüthrich wrote:\n> Hi Jacopo\n>\n> I tested you changes on my Surface Book 2 and everything works fine:\n>\n> Acked-by: Fabian Wüthrich \n\nThanks, finally pushed!\n\n>\n> On 22.02.21 12:32, Jacopo Mondi wrote:\n> > From: Fabian Wüthrich \n> >\n> > Use the same transformation logic as in the raspberry pipeline to\n> > implement rotations in the ipu3 pipeline.\n> >\n> > Tested on a Surface Book 2 with an experimental driver for OV5693.\n> >\n> > Signed-off-by: Fabian Wüthrich \n> > Reviewed-by: Kieran Bingham \n> > Reviewed-by: Jacopo Mondi \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> >\n> > I have re-based and re-worked this slightly\n> >\n> > - Rebased on master\n> > - Remove snake_case variables\n> > - Move H/V flip setting from the end of configure() to just before\n> > the part where bail out if the request contains only raw streams, so that\n> > transform can be applied on RAW images as well\n> > - Minor style changes such as breaking lines more often\n> >\n> > Fabian, could you re-ack this, and maybe if you have a setup to do so re-test ?\n> > I'll push it with your ack!\n> >\n> > Thanks\n> > j\n> >\n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 87 +++++++++++++++++++++++++++-\n> > 1 file changed, 85 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index b8a655ce0b68..0561a4610007 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -16,6 +16,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -75,6 +76,9 @@ public:\n> >\n> > \tuint32_t exposureTime_;\n> > \tRectangle cropRegion_;\n> > +\tbool supportsFlips_;\n> > +\tTransform rotationTransform_;\n> > +\n> > \tstd::unique_ptr delayedCtrls_;\n> > \tIPU3Frames frameInfos_;\n> >\n> > @@ -95,6 +99,9 @@ public:\n> > \tconst StreamConfiguration &cio2Format() const { return cio2Configuration_; }\n> > \tconst ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }\n> >\n> > +\t/* Cache the combinedTransform_ that will be applied to the sensor */\n> > +\tTransform combinedTransform_;\n> > +\n> > private:\n> > \t/*\n> > \t * The IPU3CameraData instance is guaranteed to be valid as long as the\n> > @@ -167,11 +174,49 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > \tif (config_.empty())\n> > \t\treturn Invalid;\n> >\n> > -\tif (transform != Transform::Identity) {\n> > -\t\ttransform = Transform::Identity;\n> > +\tTransform combined = transform * data_->rotationTransform_;\n> > +\n> > +\t/*\n> > +\t * We combine the platform and user transform, but must \"adjust away\"\n> > +\t * any combined result that includes a transposition, as we can't do\n> > +\t * those. In this case, flipping only the transpose bit is helpful to\n> > +\t * applications - they either get the transform they requested, or have\n> > +\t * to do a simple transpose themselves (they don't have to worry about\n> > +\t * the other possible cases).\n> > +\t */\n> > +\tif (!!(combined & Transform::Transpose)) {\n> > +\t\t/*\n> > +\t\t * Flipping the transpose bit in \"transform\" flips it in the\n> > +\t\t * combined result too (as it's the last thing that happens),\n> > +\t\t * which is of course clearing it.\n> > +\t\t */\n> > +\t\ttransform ^= Transform::Transpose;\n> > +\t\tcombined &= ~Transform::Transpose;\n> > +\t\tstatus = Adjusted;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * We also check if the sensor doesn't do h/vflips at all, in which\n> > +\t * case we clear them, and the application will have to do everything.\n> > +\t */\n> > +\tif (!data_->supportsFlips_ && !!combined) {\n> > +\t\t/*\n> > +\t\t * If the sensor can do no transforms, then combined must be\n> > +\t\t * changed to the identity. The only user transform that gives\n> > +\t\t * rise to this is the inverse of the rotation. (Recall that\n> > +\t\t * combined = transform * rotationTransform.)\n> > +\t\t */\n> > +\t\ttransform = -data_->rotationTransform_;\n> > +\t\tcombined = Transform::Identity;\n> > \t\tstatus = Adjusted;\n> > \t}\n> >\n> > +\t/*\n> > +\t * Store the final combined transform that configure() will need to\n> > +\t * apply to the sensor to save us working it out again.\n> > +\t */\n> > +\tcombinedTransform_ = combined;\n> > +\n> > \t/* Cap the number of entries to the available streams. */\n> > \tif (config_.size() > IPU3_MAX_STREAMS) {\n> > \t\tconfig_.resize(IPU3_MAX_STREAMS);\n> > @@ -503,6 +548,24 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > \tcio2->sensor()->sensorInfo(&sensorInfo);\n> > \tdata->cropRegion_ = sensorInfo.analogCrop;\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 (data->supportsFlips_) {\n> > +\t\tControlList sensorCtrls(cio2->sensor()->controls());\n> > +\t\tsensorCtrls.set(V4L2_CID_HFLIP,\n> > +\t\t\t\tstatic_cast(!!(config->combinedTransform_\n> > +\t\t\t\t\t\t\t& Transform::HFlip)));\n> > +\t\tsensorCtrls.set(V4L2_CID_VFLIP,\n> > +\t\t\t\tstatic_cast(!!(config->combinedTransform_\n> > +\t\t\t\t\t\t & Transform::VFlip)));\n> > +\n> > +\t\tret = cio2->sensor()->setControls(&sensorCtrls);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\t}\n> > +\n> > \t/*\n> > \t * If the ImgU gets configured, its driver seems to expect that\n> > \t * buffers will be queued to its outputs, as otherwise the next\n> > @@ -980,6 +1043,26 @@ int PipelineHandlerIPU3::registerCameras()\n> > \t\tdata->cio2_.frameStart().connect(data->delayedCtrls_.get(),\n> > \t\t\t\t\t\t &DelayedControls::applyControls);\n> >\n> > +\t\t/* Convert the sensor rotation to a transformation */\n> > +\t\tint32_t rotation = 0;\n> > +\t\tif (data->properties_.contains(properties::Rotation))\n> > +\t\t\trotation = data->properties_.get(properties::Rotation);\n> > +\t\telse\n> > +\t\t\tLOG(IPU3, Warning) << \"Rotation control not exposed by \"\n> > +\t\t\t\t\t << cio2->sensor()->id()\n> > +\t\t\t\t\t << \". Assume rotation 0\";\n> > +\n> > +\t\tbool success;\n> > +\t\tdata->rotationTransform_ = transformFromRotation(rotation, &success);\n> > +\t\tif (!success)\n> > +\t\t\tLOG(IPU3, Warning) << \"Invalid rotation of \" << rotation\n> > +\t\t\t\t\t << \" degrees: ignoring\";\n> > +\n> > +\t\tControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP });\n> > +\t\tif (!ctrls.empty())\n> > +\t\t\t/* We assume it will support vflips too... */\n> > +\t\t\tdata->supportsFlips_ = true;\n> > +\n> > \t\t/**\n> > \t\t * \\todo Dynamically assign ImgU and output devices to each\n> > \t\t * stream and camera; as of now, limit support to two cameras\n> > --\n> > 2.30.0\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8C2F9BD1F1\n\tfor ;\n\tTue, 23 Feb 2021 08:49:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2542168A27;\n\tTue, 23 Feb 2021 09:49:17 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8BD668A21\n\tfor ;\n\tTue, 23 Feb 2021 09:49:15 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 1472A1BF20C;\n\tTue, 23 Feb 2021 08:49:14 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 23 Feb 2021 09:49:42 +0100","From":"Jacopo Mondi ","To":"Fabian =?utf-8?b?V8O8dGhyaWNo?= ","Message-ID":"<20210223084942.xghjk4zymlxfw76v@uno.localdomain>","References":"<20210222113227.395737-1-jacopo@jmondi.org>\n\t<3ab509d4-5d89-d5fd-2ee1-9dc5544a49bf@fabwu.ch>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<3ab509d4-5d89-d5fd-2ee1-9dc5544a49bf@fabwu.ch>","Subject":"Re: [libcamera-devel] [PATCH v5] libcamera: ipu3: Add rotation to\n\tipu3 pipeline","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]