[{"id":14572,"web_url":"https://patchwork.libcamera.org/comment/14572/","msgid":"<20210118100631.l66zk2uuhnpsllm6@uno.localdomain>","date":"2021-01-18T10:06:31","subject":"Re: [libcamera-devel] [PATCH v2] 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":"Hi Fabian,\n\nOn Sat, Jan 16, 2021 at 04:02:58PM +0100, Fabian Wüthrich wrote:\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> ---\n> Changes in v2:\n> - Cache rotationTransform in CameraData\n> - Use separate controls for sensor\n>\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 81 +++++++++++++++++++++++++++-\n> 1 file changed, 79 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 73304ea7..8db5f2f1 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n>\n> @@ -62,6 +63,15 @@ public:\n> \tStream outStream_;\n> \tStream vfStream_;\n> \tStream rawStream_;\n> +\n> +\t/* Save transformation given by the sensor rotation */\n> +\tTransform rotationTransform_;\n> +\n> +\t/*\n> +\t * Manage horizontal and vertical flips supported (or not) by the\n> +\t * sensor.\n> +\t */\n> +\tbool supportsFlips_;\n> };\n>\n> class IPU3CameraConfiguration : public CameraConfiguration\n> @@ -74,6 +84,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> @@ -143,11 +156,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 those.\n> +\t * 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> @@ -540,6 +591,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t\treturn ret;\n> \t}\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 sensor_ctrls(cio2->sensor()->controls());\n> +\t\tsensor_ctrls.set(V4L2_CID_HFLIP,\n> +\t\t\t\t static_cast(!!(config->combinedTransform_ & Transform::HFlip)));\n> +\t\tsensor_ctrls.set(V4L2_CID_VFLIP,\n> +\t\t\t\t static_cast(!!(config->combinedTransform_ & Transform::VFlip)));\n> +\t\tcio2->sensor()->setControls(&sensor_ctrls);\n> +\t}\n> +\n> \treturn 0;\n> }\n>\n> @@ -775,9 +839,22 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\t/* Initialize the camera properties. */\n> \t\tdata->properties_ = cio2->sensor()->properties();\n>\n> +\t\t/* Convert the sensor rotation to a transformation */\n> +\t\tint32_t rotation = data->properties_.get(properties::Rotation);\n\nThere's no guarantee the CameraSensor class register the Rotation\nproperty. It was defaulted to 0 if the information was not available\nfrom the firmware interface.\n\nIt is now not anymore (a very recent change, sorry)\nhttps://git.libcamera.org/libcamera/libcamera.git/commit/?id=c35123e6f91b6269cf6f5ae9216aeccebc545c75\n\nNot sure what would be best, default to 0 in each pipeline handler\nthat need that information ?\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 << \" degrees - ignoring\";\n> +\n> \t\t/* Initialze the camera controls. */\n> \t\tdata->controlInfo_ = IPU3Controls;\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> +\t\t}\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 6E9F9BD808\n\tfor ;\n\tMon, 18 Jan 2021 10:06:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A2C06811B;\n\tMon, 18 Jan 2021 11:06:13 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 744AB60314\n\tfor ;\n\tMon, 18 Jan 2021 11:06:12 +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 relay2-d.mail.gandi.net (Postfix) with ESMTPSA id E823C40015;\n\tMon, 18 Jan 2021 10:06:11 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 18 Jan 2021 11:06:31 +0100","From":"Jacopo Mondi ","To":"Fabian =?utf-8?b?V8O8dGhyaWNo?= ","Message-ID":"<20210118100631.l66zk2uuhnpsllm6@uno.localdomain>","References":"<20210106133513.34118-1-me@fabwu.ch>\n\t<20210116150257.2970-1-me@fabwu.ch>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210116150257.2970-1-me@fabwu.ch>","Subject":"Re: [libcamera-devel] [PATCH v2] 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\" "}},{"id":14641,"web_url":"https://patchwork.libcamera.org/comment/14641/","msgid":"<7fc0a92d-e44d-d77a-bcec-6c19302a31de@fabwu.ch>","date":"2021-01-21T07:38:48","subject":"Re: [libcamera-devel] [PATCH v2] 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've just got the message that my driver needs to be fixed ^^\n\nI like this change as it transparently states the requirements.\n\nOn 18.01.21 11:06, Jacopo Mondi wrote:\n> Hi Fabian,\n> \n> On Sat, Jan 16, 2021 at 04:02:58PM +0100, Fabian Wüthrich wrote:\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>> ---\n>> Changes in v2:\n>> - Cache rotationTransform in CameraData\n>> - Use separate controls for sensor\n>>\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 81 +++++++++++++++++++++++++++-\n>> 1 file changed, 79 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 73304ea7..8db5f2f1 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -14,6 +14,7 @@\n>> #include \n>> #include \n>> #include \n>> +#include \n>> #include \n>> #include \n>>\n>> @@ -62,6 +63,15 @@ public:\n>> \tStream outStream_;\n>> \tStream vfStream_;\n>> \tStream rawStream_;\n>> +\n>> +\t/* Save transformation given by the sensor rotation */\n>> +\tTransform rotationTransform_;\n>> +\n>> +\t/*\n>> +\t * Manage horizontal and vertical flips supported (or not) by the\n>> +\t * sensor.\n>> +\t */\n>> +\tbool supportsFlips_;\n>> };\n>>\n>> class IPU3CameraConfiguration : public CameraConfiguration\n>> @@ -74,6 +84,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>> @@ -143,11 +156,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 those.\n>> +\t * 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>> @@ -540,6 +591,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>> \t\treturn ret;\n>> \t}\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 sensor_ctrls(cio2->sensor()->controls());\n>> +\t\tsensor_ctrls.set(V4L2_CID_HFLIP,\n>> +\t\t\t\t static_cast(!!(config->combinedTransform_ & Transform::HFlip)));\n>> +\t\tsensor_ctrls.set(V4L2_CID_VFLIP,\n>> +\t\t\t\t static_cast(!!(config->combinedTransform_ & Transform::VFlip)));\n>> +\t\tcio2->sensor()->setControls(&sensor_ctrls);\n>> +\t}\n>> +\n>> \treturn 0;\n>> }\n>>\n>> @@ -775,9 +839,22 @@ int PipelineHandlerIPU3::registerCameras()\n>> \t\t/* Initialize the camera properties. */\n>> \t\tdata->properties_ = cio2->sensor()->properties();\n>>\n>> +\t\t/* Convert the sensor rotation to a transformation */\n>> +\t\tint32_t rotation = data->properties_.get(properties::Rotation);\n> \n> There's no guarantee the CameraSensor class register the Rotation\n> property. It was defaulted to 0 if the information was not available\n> from the firmware interface.\n> \n> It is now not anymore (a very recent change, sorry)\n> https://git.libcamera.org/libcamera/libcamera.git/commit/?id=c35123e6f91b6269cf6f5ae9216aeccebc545c75\n> \n> Not sure what would be best, default to 0 in each pipeline handler\n> that need that information ?\n> \n\nYes I can do that. Should I post a patch for the raspberry pipeline as well?\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 << \" degrees - ignoring\";\n>> +\n>> \t\t/* Initialze the camera controls. */\n>> \t\tdata->controlInfo_ = IPU3Controls;\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>> +\t\t}\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 3FD91C0F2A\n\tfor ;\n\tThu, 21 Jan 2021 07:38:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8FBE5681B0;\n\tThu, 21 Jan 2021 08:38:51 +0100 (CET)","from gusto4.metanet.ch (gusto4.metanet.ch [80.74.154.158])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5EA9A6030E\n\tfor ;\n\tThu, 21 Jan 2021 08:38:49 +0100 (CET)","from [IPv6:2001:1715:9d9c:4e40:649e:8d07:9d6c:59c2] (localhost\n\t[127.0.0.1]) by gusto4.metanet.ch (Postfix) with ESMTPSA id\n\tD1AAD4F01766; Thu, 21 Jan 2021 08:38:48 +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=\"48UXzyTs\";\n\tdkim-atps=neutral","gusto.metanet.ch;\n\tspf=pass (sender IP is 2001:1715:9d9c:4e40:649e:8d07:9d6c:59c2)\n\tsmtp.mailfrom=me@fabwu.ch\n\tsmtp.helo=[IPv6:2001:1715:9d9c:4e40:649e:8d07:9d6c:59c2]"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=fabwu.ch; s=default; \n\tt=1611214728; bh=O7KHX4oq25C04RBv2gQNBQDQwMD6l6aSwfZWqtJZKDo=;\n\th=Subject:To:From;\n\tb=48UXzyTs8vQCiCszSV5R8vFBExi1u3u79e8N/fGQexuTmahRaw1B6Q/jDWCGCzgYh\n\tuZTwDeAz4A7KudXwzK/XJfjZ9vXyI93lISJy7iBD2Fur+Y/Ore8VEUF+NaI9msEqr5\n\tXyAl9YElZEGDcBrrmfmQA268l1zTSAhpT6PYcing=","Received-SPF":"pass (gusto.metanet.ch: connection is authenticated)","To":"Jacopo Mondi ","References":"<20210106133513.34118-1-me@fabwu.ch>\n\t<20210116150257.2970-1-me@fabwu.ch>\n\t<20210118100631.l66zk2uuhnpsllm6@uno.localdomain>","From":"=?utf-8?q?Fabian_W=C3=BCthrich?= ","Message-ID":"<7fc0a92d-e44d-d77a-bcec-6c19302a31de@fabwu.ch>","Date":"Thu, 21 Jan 2021 08:38:48 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.6.1","MIME-Version":"1.0","In-Reply-To":"<20210118100631.l66zk2uuhnpsllm6@uno.localdomain>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2] 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\" "}},{"id":14676,"web_url":"https://patchwork.libcamera.org/comment/14676/","msgid":"","date":"2021-01-21T23:37:59","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: ipu3: Add rotation to\n\tipu3 pipeline","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo and Fabian,\n\nOn Mon, Jan 18, 2021 at 11:06:31AM +0100, Jacopo Mondi wrote:\n> On Sat, Jan 16, 2021 at 04:02:58PM +0100, Fabian Wüthrich wrote:\n> > Use the same transformation logic as in the raspberry pipeline to\n> > implement rotations in the ipu3 pipeline.\n\nThis should eventually be shared between different pipeline handlers,\nbut for now it's fine.\n\n> > Tested on a Surface Book 2 with an experimental driver for OV5693.\n> >\n> > Signed-off-by: Fabian Wüthrich \n> > ---\n> > Changes in v2:\n> > - Cache rotationTransform in CameraData\n> > - Use separate controls for sensor\n> >\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 81 +++++++++++++++++++++++++++-\n> > 1 file changed, 79 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 73304ea7..8db5f2f1 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -14,6 +14,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -62,6 +63,15 @@ public:\n> > \tStream outStream_;\n> > \tStream vfStream_;\n> > \tStream rawStream_;\n> > +\n> > +\t/* Save transformation given by the sensor rotation */\n> > +\tTransform rotationTransform_;\n> > +\n> > +\t/*\n> > +\t * Manage horizontal and vertical flips supported (or not) by the\n> > +\t * sensor.\n> > +\t */\n> > +\tbool supportsFlips_;\n> > };\n> >\n> > class IPU3CameraConfiguration : public CameraConfiguration\n> > @@ -74,6 +84,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> > @@ -143,11 +156,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 those.\n> > +\t * 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> > @@ -540,6 +591,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > \t\treturn ret;\n> > \t}\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 sensor_ctrls(cio2->sensor()->controls());\n> > +\t\tsensor_ctrls.set(V4L2_CID_HFLIP,\n> > +\t\t\t\t static_cast(!!(config->combinedTransform_ & Transform::HFlip)));\n> > +\t\tsensor_ctrls.set(V4L2_CID_VFLIP,\n> > +\t\t\t\t static_cast(!!(config->combinedTransform_ & Transform::VFlip)));\n> > +\t\tcio2->sensor()->setControls(&sensor_ctrls);\n> > +\t}\n> > +\n> > \treturn 0;\n> > }\n> >\n> > @@ -775,9 +839,22 @@ int PipelineHandlerIPU3::registerCameras()\n> > \t\t/* Initialize the camera properties. */\n> > \t\tdata->properties_ = cio2->sensor()->properties();\n> >\n> > +\t\t/* Convert the sensor rotation to a transformation */\n> > +\t\tint32_t rotation = data->properties_.get(properties::Rotation);\n> \n> There's no guarantee the CameraSensor class register the Rotation\n> property. It was defaulted to 0 if the information was not available\n> from the firmware interface.\n> \n> It is now not anymore (a very recent change, sorry)\n> https://git.libcamera.org/libcamera/libcamera.git/commit/?id=c35123e6f91b6269cf6f5ae9216aeccebc545c75\n> \n> Not sure what would be best, default to 0 in each pipeline handler\n> that need that information ?\n\nThat sounds good to me for now.\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 << \" degrees - ignoring\";\n> > +\n> > \t\t/* Initialze the camera controls. */\n> > \t\tdata->controlInfo_ = IPU3Controls;\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> > +\t\t}\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","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 0C0AABD808\n\tfor ;\n\tThu, 21 Jan 2021 23:38:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 448F968223;\n\tFri, 22 Jan 2021 00:38:20 +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 9D51068195\n\tfor ;\n\tFri, 22 Jan 2021 00:38:18 +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 068FD50E;\n\tFri, 22 Jan 2021 00:38:17 +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=\"Xi3aQqme\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611272298;\n\tbh=d3dw9aezUTQGjrlTP+h0aXEVQrYbbHg2taBmIg0WWGI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Xi3aQqmeJ2Hh401WrOVX8fbudsCBPgmXk9BEouBYR2taDNwtyTF88z3eU0aexkPY+\n\tH9ap8fwsSVfUyM24/+slz7CePXKBM6j6n2/owG1nu6Ujkc5GMlJ9Vp0Eb80ixP7Ng8\n\txTVV7hO/AIwvx0+1F1ii6N0TQtnzO21PB/h0Mhg8=","Date":"Fri, 22 Jan 2021 01:37:59 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210106133513.34118-1-me@fabwu.ch>\n\t<20210116150257.2970-1-me@fabwu.ch>\n\t<20210118100631.l66zk2uuhnpsllm6@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210118100631.l66zk2uuhnpsllm6@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2] 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\" "}},{"id":14683,"web_url":"https://patchwork.libcamera.org/comment/14683/","msgid":"","date":"2021-01-22T08:10:42","subject":"Re: [libcamera-devel] [PATCH v2] 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 Laurent,\n\nOn 22.01.21 00:37, Laurent Pinchart wrote:\n> Hi Jacopo and Fabian,\n> \n> On Mon, Jan 18, 2021 at 11:06:31AM +0100, Jacopo Mondi wrote:\n>> On Sat, Jan 16, 2021 at 04:02:58PM +0100, Fabian Wüthrich wrote:\n>>> Use the same transformation logic as in the raspberry pipeline to\n>>> implement rotations in the ipu3 pipeline.\n> \n> This should eventually be shared between different pipeline handlers,\n> but for now it's fine.\n> \n\nAgreed.\n\n>>> Tested on a Surface Book 2 with an experimental driver for OV5693.\n>>>\n>>> Signed-off-by: Fabian Wüthrich \n>>> ---\n>>> Changes in v2:\n>>> - Cache rotationTransform in CameraData\n>>> - Use separate controls for sensor\n>>>\n>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 81 +++++++++++++++++++++++++++-\n>>> 1 file changed, 79 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> index 73304ea7..8db5f2f1 100644\n>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> @@ -14,6 +14,7 @@\n>>> #include \n>>> #include \n>>> #include \n>>> +#include \n>>> #include \n>>> #include \n>>>\n>>> @@ -62,6 +63,15 @@ public:\n>>> \tStream outStream_;\n>>> \tStream vfStream_;\n>>> \tStream rawStream_;\n>>> +\n>>> +\t/* Save transformation given by the sensor rotation */\n>>> +\tTransform rotationTransform_;\n>>> +\n>>> +\t/*\n>>> +\t * Manage horizontal and vertical flips supported (or not) by the\n>>> +\t * sensor.\n>>> +\t */\n>>> +\tbool supportsFlips_;\n>>> };\n>>>\n>>> class IPU3CameraConfiguration : public CameraConfiguration\n>>> @@ -74,6 +84,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>>> @@ -143,11 +156,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 those.\n>>> +\t * 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>>> @@ -540,6 +591,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>>> \t\treturn ret;\n>>> \t}\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 sensor_ctrls(cio2->sensor()->controls());\n>>> +\t\tsensor_ctrls.set(V4L2_CID_HFLIP,\n>>> +\t\t\t\t static_cast(!!(config->combinedTransform_ & Transform::HFlip)));\n>>> +\t\tsensor_ctrls.set(V4L2_CID_VFLIP,\n>>> +\t\t\t\t static_cast(!!(config->combinedTransform_ & Transform::VFlip)));\n>>> +\t\tcio2->sensor()->setControls(&sensor_ctrls);\n>>> +\t}\n>>> +\n>>> \treturn 0;\n>>> }\n>>>\n>>> @@ -775,9 +839,22 @@ int PipelineHandlerIPU3::registerCameras()\n>>> \t\t/* Initialize the camera properties. */\n>>> \t\tdata->properties_ = cio2->sensor()->properties();\n>>>\n>>> +\t\t/* Convert the sensor rotation to a transformation */\n>>> +\t\tint32_t rotation = data->properties_.get(properties::Rotation);\n>>\n>> There's no guarantee the CameraSensor class register the Rotation\n>> property. It was defaulted to 0 if the information was not available\n>> from the firmware interface.\n>>\n>> It is now not anymore (a very recent change, sorry)\n>> https://git.libcamera.org/libcamera/libcamera.git/commit/?id=c35123e6f91b6269cf6f5ae9216aeccebc545c75\n>>\n>> Not sure what would be best, default to 0 in each pipeline handler\n>> that need that information ?\n> \n> That sounds good to me for now.\n> \n\nOk, thanks for checking. Should I provide a patch for the raspberry pipeline as well?\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 << \" degrees - ignoring\";\n>>> +\n>>> \t\t/* Initialze the camera controls. */\n>>> \t\tdata->controlInfo_ = IPU3Controls;\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>>> +\t\t}\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>","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 BC13BBD808\n\tfor ;\n\tFri, 22 Jan 2021 08:10:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 32FD86822D;\n\tFri, 22 Jan 2021 09:10:44 +0100 (CET)","from gusto4.metanet.ch (gusto4.metanet.ch [80.74.154.158])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1E495681DF\n\tfor ;\n\tFri, 22 Jan 2021 09:10:43 +0100 (CET)","from [IPv6:2001:1715:9d9c:4e40:649e:8d07:9d6c:59c2] (localhost\n\t[127.0.0.1]) by gusto4.metanet.ch (Postfix) with ESMTPSA id\n\t9F05F4F01781; Fri, 22 Jan 2021 09:10:42 +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=\"UWpYzjTG\";\n\tdkim-atps=neutral","gusto.metanet.ch;\n\tspf=pass (sender IP is 2001:1715:9d9c:4e40:649e:8d07:9d6c:59c2)\n\tsmtp.mailfrom=me@fabwu.ch\n\tsmtp.helo=[IPv6:2001:1715:9d9c:4e40:649e:8d07:9d6c:59c2]"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=fabwu.ch; s=default; \n\tt=1611303042; bh=yCRz2U+Ajl+xhxV+Z84TXRS+6/1M7j4H2jcdSD9iAHk=;\n\th=Subject:To:From;\n\tb=UWpYzjTGGS3yjlcGpyx/jHSQCJnK7zPe3+SnG+sx48qPqYD6ku/BXORMV1k3DOTZ/\n\tEMOGeMBeLCQ4IBQiFSXrnmBuLV1YyALf2XEUBpZyILNvJamYS5OPJmInKPp2pAZpsF\n\tVD6iWmIBGFLuxMjuKDjO7UcOOfPI3yBuWvanMbdU=","Received-SPF":"pass (gusto.metanet.ch: connection is authenticated)","To":"Laurent Pinchart ,\n\tJacopo Mondi ","References":"<20210106133513.34118-1-me@fabwu.ch>\n\t<20210116150257.2970-1-me@fabwu.ch>\n\t<20210118100631.l66zk2uuhnpsllm6@uno.localdomain>\n\t","From":"=?utf-8?q?Fabian_W=C3=BCthrich?= ","Message-ID":"","Date":"Fri, 22 Jan 2021 09:10:42 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.6.1","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2] 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\" "}},{"id":14685,"web_url":"https://patchwork.libcamera.org/comment/14685/","msgid":"<20210122085210.2k3ihvvwancteljx@uno.localdomain>","date":"2021-01-22T08:52:10","subject":"Re: [libcamera-devel] [PATCH v2] 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":"Hi Fabian,\n + Naush for RPi question\n\nOn Fri, Jan 22, 2021 at 09:10:42AM +0100, Fabian Wüthrich wrote:\n> Hi Laurent,\n>\n> On 22.01.21 00:37, Laurent Pinchart wrote:\n> > Hi Jacopo and Fabian,\n> >\n> > On Mon, Jan 18, 2021 at 11:06:31AM +0100, Jacopo Mondi wrote:\n> >> On Sat, Jan 16, 2021 at 04:02:58PM +0100, Fabian Wüthrich wrote:\n> >>> Use the same transformation logic as in the raspberry pipeline to\n> >>> implement rotations in the ipu3 pipeline.\n> >\n> > This should eventually be shared between different pipeline handlers,\n> > but for now it's fine.\n> >\n>\n> Agreed.\n>\n> >>> Tested on a Surface Book 2 with an experimental driver for OV5693.\n> >>>\n> >>> Signed-off-by: Fabian Wüthrich \n> >>> ---\n> >>> Changes in v2:\n> >>> - Cache rotationTransform in CameraData\n> >>> - Use separate controls for sensor\n> >>>\n> >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 81 +++++++++++++++++++++++++++-\n> >>> 1 file changed, 79 insertions(+), 2 deletions(-)\n> >>>\n> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> index 73304ea7..8db5f2f1 100644\n> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> @@ -14,6 +14,7 @@\n> >>> #include \n> >>> #include \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>>\n> >>> @@ -62,6 +63,15 @@ public:\n> >>> \tStream outStream_;\n> >>> \tStream vfStream_;\n> >>> \tStream rawStream_;\n> >>> +\n> >>> +\t/* Save transformation given by the sensor rotation */\n> >>> +\tTransform rotationTransform_;\n> >>> +\n> >>> +\t/*\n> >>> +\t * Manage horizontal and vertical flips supported (or not) by the\n> >>> +\t * sensor.\n> >>> +\t */\n> >>> +\tbool supportsFlips_;\n> >>> };\n> >>>\n> >>> class IPU3CameraConfiguration : public CameraConfiguration\n> >>> @@ -74,6 +84,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> >>> @@ -143,11 +156,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 those.\n> >>> +\t * 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> >>> @@ -540,6 +591,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> >>> \t\treturn ret;\n> >>> \t}\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 sensor_ctrls(cio2->sensor()->controls());\n> >>> +\t\tsensor_ctrls.set(V4L2_CID_HFLIP,\n> >>> +\t\t\t\t static_cast(!!(config->combinedTransform_ & Transform::HFlip)));\n> >>> +\t\tsensor_ctrls.set(V4L2_CID_VFLIP,\n> >>> +\t\t\t\t static_cast(!!(config->combinedTransform_ & Transform::VFlip)));\n> >>> +\t\tcio2->sensor()->setControls(&sensor_ctrls);\n> >>> +\t}\n> >>> +\n> >>> \treturn 0;\n> >>> }\n> >>>\n> >>> @@ -775,9 +839,22 @@ int PipelineHandlerIPU3::registerCameras()\n> >>> \t\t/* Initialize the camera properties. */\n> >>> \t\tdata->properties_ = cio2->sensor()->properties();\n> >>>\n> >>> +\t\t/* Convert the sensor rotation to a transformation */\n> >>> +\t\tint32_t rotation = data->properties_.get(properties::Rotation);\n> >>\n> >> There's no guarantee the CameraSensor class register the Rotation\n> >> property. It was defaulted to 0 if the information was not available\n> >> from the firmware interface.\n> >>\n> >> It is now not anymore (a very recent change, sorry)\n> >> https://git.libcamera.org/libcamera/libcamera.git/commit/?id=c35123e6f91b6269cf6f5ae9216aeccebc545c75\n> >>\n> >> Not sure what would be best, default to 0 in each pipeline handler\n> >> that need that information ?\n> >\n> > That sounds good to me for now.\n> >\n>\n> Ok, thanks for checking. Should I provide a patch for the raspberry pipeline as well?\n\nI would not mind, but feel free to stick to IPU3 if that's quicker.\n\nCc Naush for RPi. I'm thinking that, being RPi a bit more 'strict' as\nplatform and with a little more control over sensor drivers/fw, maybe\nthey want to refuse setups where no rotation is specified.\n\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 << \" degrees - ignoring\";\n> >>> +\n> >>> \t\t/* Initialze the camera controls. */\n> >>> \t\tdata->controlInfo_ = IPU3Controls;\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> >>> +\t\t}\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> >","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 BABE2C0F2A\n\tfor ;\n\tFri, 22 Jan 2021 08:51:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2D2006822F;\n\tFri, 22 Jan 2021 09:51:53 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 01457681DF\n\tfor ;\n\tFri, 22 Jan 2021 09:51:51 +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 relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 14A412001F;\n\tFri, 22 Jan 2021 08:51:50 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 22 Jan 2021 09:52:10 +0100","From":"Jacopo Mondi ","To":"Fabian =?utf-8?b?V8O8dGhyaWNo?= ","Message-ID":"<20210122085210.2k3ihvvwancteljx@uno.localdomain>","References":"<20210106133513.34118-1-me@fabwu.ch>\n\t<20210116150257.2970-1-me@fabwu.ch>\n\t<20210118100631.l66zk2uuhnpsllm6@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] 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\" "}},{"id":14704,"web_url":"https://patchwork.libcamera.org/comment/14704/","msgid":"","date":"2021-01-22T12:20:26","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: ipu3: Add rotation to\n\tipu3 pipeline","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Fabian, Jacopo, Naush, David ;-)\n\nOn 22/01/2021 08:52, Jacopo Mondi wrote:\n> Hi Fabian,\n> + Naush for RPi question\n\nFixing the +CC for the question below - but also adding David as he\nwrote the original Transforms.\n\n--\nKieran\n\n\n> \n> On Fri, Jan 22, 2021 at 09:10:42AM +0100, Fabian Wüthrich wrote:\n>> Hi Laurent,\n>>\n>> On 22.01.21 00:37, Laurent Pinchart wrote:\n>>> Hi Jacopo and Fabian,\n>>>\n>>> On Mon, Jan 18, 2021 at 11:06:31AM +0100, Jacopo Mondi wrote:\n>>>> On Sat, Jan 16, 2021 at 04:02:58PM +0100, Fabian Wüthrich wrote:\n>>>>> Use the same transformation logic as in the raspberry pipeline to\n>>>>> implement rotations in the ipu3 pipeline.\n>>>\n>>> This should eventually be shared between different pipeline handlers,\n>>> but for now it's fine.\n>>>\n>>\n>> Agreed.\n>>\n>>>>> Tested on a Surface Book 2 with an experimental driver for OV5693.\n>>>>>\n>>>>> Signed-off-by: Fabian Wüthrich \n>>>>> ---\n>>>>> Changes in v2:\n>>>>> - Cache rotationTransform in CameraData\n>>>>> - Use separate controls for sensor\n>>>>>\n>>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 81 +++++++++++++++++++++++++++-\n>>>>> 1 file changed, 79 insertions(+), 2 deletions(-)\n>>>>>\n>>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>>> index 73304ea7..8db5f2f1 100644\n>>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>>> @@ -14,6 +14,7 @@\n>>>>> #include \n>>>>> #include \n>>>>> #include \n>>>>> +#include \n>>>>> #include \n>>>>> #include \n>>>>>\n>>>>> @@ -62,6 +63,15 @@ public:\n>>>>> \tStream outStream_;\n>>>>> \tStream vfStream_;\n>>>>> \tStream rawStream_;\n>>>>> +\n>>>>> +\t/* Save transformation given by the sensor rotation */\n>>>>> +\tTransform rotationTransform_;\n>>>>> +\n>>>>> +\t/*\n>>>>> +\t * Manage horizontal and vertical flips supported (or not) by the\n>>>>> +\t * sensor.\n>>>>> +\t */\n>>>>> +\tbool supportsFlips_;\n>>>>> };\n>>>>>\n>>>>> class IPU3CameraConfiguration : public CameraConfiguration\n>>>>> @@ -74,6 +84,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>>>>> @@ -143,11 +156,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 those.\n>>>>> +\t * 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>>>>> @@ -540,6 +591,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>>>>> \t\treturn ret;\n>>>>> \t}\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 sensor_ctrls(cio2->sensor()->controls());\n>>>>> +\t\tsensor_ctrls.set(V4L2_CID_HFLIP,\n>>>>> +\t\t\t\t static_cast(!!(config->combinedTransform_ & Transform::HFlip)));\n>>>>> +\t\tsensor_ctrls.set(V4L2_CID_VFLIP,\n>>>>> +\t\t\t\t static_cast(!!(config->combinedTransform_ & Transform::VFlip)));\n>>>>> +\t\tcio2->sensor()->setControls(&sensor_ctrls);\n>>>>> +\t}\n>>>>> +\n>>>>> \treturn 0;\n>>>>> }\n>>>>>\n>>>>> @@ -775,9 +839,22 @@ int PipelineHandlerIPU3::registerCameras()\n>>>>> \t\t/* Initialize the camera properties. */\n>>>>> \t\tdata->properties_ = cio2->sensor()->properties();\n>>>>>\n>>>>> +\t\t/* Convert the sensor rotation to a transformation */\n>>>>> +\t\tint32_t rotation = data->properties_.get(properties::Rotation);\n>>>>\n>>>> There's no guarantee the CameraSensor class register the Rotation\n>>>> property. It was defaulted to 0 if the information was not available\n>>>> from the firmware interface.\n>>>>\n>>>> It is now not anymore (a very recent change, sorry)\n>>>> https://git.libcamera.org/libcamera/libcamera.git/commit/?id=c35123e6f91b6269cf6f5ae9216aeccebc545c75\n>>>>\n>>>> Not sure what would be best, default to 0 in each pipeline handler\n>>>> that need that information ?\n>>>\n>>> That sounds good to me for now.\n>>>\n>>\n>> Ok, thanks for checking. Should I provide a patch for the raspberry pipeline as well?\n> \n> I would not mind, but feel free to stick to IPU3 if that's quicker.\n> \n> Cc Naush for RPi. I'm thinking that, being RPi a bit more 'strict' as\n> platform and with a little more control over sensor drivers/fw, maybe\n> they want to refuse setups where no rotation is specified.\n> \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 << \" degrees - ignoring\";\n>>>>> +\n>>>>> \t\t/* Initialze the camera controls. */\n>>>>> \t\tdata->controlInfo_ = IPU3Controls;\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>>>>> +\t\t}\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> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 48404BD808\n\tfor ;\n\tFri, 22 Jan 2021 12:20:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC2D86825E;\n\tFri, 22 Jan 2021 13:20:31 +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 C1B9C6824C\n\tfor ;\n\tFri, 22 Jan 2021 13:20:30 +0100 (CET)","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 21F1F4FB;\n\tFri, 22 Jan 2021 13:20:30 +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=\"S+/ysS/j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611318030;\n\tbh=9kk682sIQcoSJu6tFpg4QIt5w5iIaoCUW0UK7DYPgUQ=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=S+/ysS/jQRQmM+1UzX347P4MNLD3+OZZ46tqGzyDCVIVT0V+5KCVL2pcJw0m0ETgw\n\tM8PTP99mPi5clXTh2vK2Wk0hEI7lqXVqQXmFo4K/pqus9oUvwgUmPc2c7yVGDyafxp\n\tTvD4vzbmaY9WyWDFbh3pcqVuG8ekEatvzC1/NMNA=","To":"Jacopo Mondi , =?utf-8?q?Fabian_W=C3=BCthrich?=\n\t","References":"<20210106133513.34118-1-me@fabwu.ch>\n\t<20210116150257.2970-1-me@fabwu.ch>\n\t<20210118100631.l66zk2uuhnpsllm6@uno.localdomain>\n\t\n\t\n\t<20210122085210.2k3ihvvwancteljx@uno.localdomain>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Fri, 22 Jan 2021 12:20:26 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20210122085210.2k3ihvvwancteljx@uno.localdomain>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v2] 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","Reply-To":"kieran.bingham@ideasonboard.com","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\" "}},{"id":14709,"web_url":"https://patchwork.libcamera.org/comment/14709/","msgid":"","date":"2021-01-22T13:07:32","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: ipu3: Add rotation to\n\tipu3 pipeline","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\n\nOn Fri, 22 Jan 2021 at 12:20, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Hi Fabian, Jacopo, Naush, David ;-)\n>\n> On 22/01/2021 08:52, Jacopo Mondi wrote:\n> > Hi Fabian,\n> > + Naush for RPi question\n>\n> Fixing the +CC for the question below - but also adding David as he\n> wrote the original Transforms.\n>\n> --\n> Kieran\n>\n>\n> >\n> > On Fri, Jan 22, 2021 at 09:10:42AM +0100, Fabian Wüthrich wrote:\n> >> Hi Laurent,\n> >>\n> >> On 22.01.21 00:37, Laurent Pinchart wrote:\n> >>> Hi Jacopo and Fabian,\n> >>>\n> >>> On Mon, Jan 18, 2021 at 11:06:31AM +0100, Jacopo Mondi wrote:\n> >>>> On Sat, Jan 16, 2021 at 04:02:58PM +0100, Fabian Wüthrich wrote:\n> >>>>> Use the same transformation logic as in the raspberry pipeline to\n> >>>>> implement rotations in the ipu3 pipeline.\n> >>>\n> >>> This should eventually be shared between different pipeline handlers,\n> >>> but for now it's fine.\n> >>>\n> >>\n> >> Agreed.\n> >>\n> >>>>> Tested on a Surface Book 2 with an experimental driver for OV5693.\n> >>>>>\n> >>>>> Signed-off-by: Fabian Wüthrich \n> >>>>> ---\n> >>>>> Changes in v2:\n> >>>>> - Cache rotationTransform in CameraData\n> >>>>> - Use separate controls for sensor\n> >>>>>\n> >>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 81\n> +++++++++++++++++++++++++++-\n> >>>>> 1 file changed, 79 insertions(+), 2 deletions(-)\n> >>>>>\n> >>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>>> index 73304ea7..8db5f2f1 100644\n> >>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>>> @@ -14,6 +14,7 @@\n> >>>>> #include \n> >>>>> #include \n> >>>>> #include \n> >>>>> +#include \n> >>>>> #include \n> >>>>> #include \n> >>>>>\n> >>>>> @@ -62,6 +63,15 @@ public:\n> >>>>> Stream outStream_;\n> >>>>> Stream vfStream_;\n> >>>>> Stream rawStream_;\n> >>>>> +\n> >>>>> + /* Save transformation given by the sensor rotation */\n> >>>>> + Transform rotationTransform_;\n> >>>>> +\n> >>>>> + /*\n> >>>>> + * Manage horizontal and vertical flips supported (or not) by the\n> >>>>> + * sensor.\n> >>>>> + */\n> >>>>> + bool supportsFlips_;\n> >>>>> };\n> >>>>>\n> >>>>> class IPU3CameraConfiguration : public CameraConfiguration\n> >>>>> @@ -74,6 +84,9 @@ public:\n> >>>>> const StreamConfiguration &cio2Format() const { return\n> cio2Configuration_; }\n> >>>>> const ImgUDevice::PipeConfig imguConfig() const { return\n> pipeConfig_; }\n> >>>>>\n> >>>>> + /* Cache the combinedTransform_ that will be applied to the sensor\n> */\n> >>>>> + Transform combinedTransform_;\n> >>>>> +\n> >>>>> private:\n> >>>>> /*\n> >>>>> * The IPU3CameraData instance is guaranteed to be valid as long\n> as the\n> >>>>> @@ -143,11 +156,49 @@ CameraConfiguration::Status\n> IPU3CameraConfiguration::validate()\n> >>>>> if (config_.empty())\n> >>>>> return Invalid;\n> >>>>>\n> >>>>> - if (transform != Transform::Identity) {\n> >>>>> - transform = Transform::Identity;\n> >>>>> + Transform combined = transform * data_->rotationTransform_;\n> >>>>> +\n> >>>>> + /*\n> >>>>> + * We combine the platform and user transform, but must \"adjust\n> away\"\n> >>>>> + * any combined result that includes a transposition, as we can't\n> do those.\n> >>>>> + * In this case, flipping only the transpose bit is helpful to\n> >>>>> + * applications - they either get the transform they requested, or\n> have\n> >>>>> + * to do a simple transpose themselves (they don't have to worry\n> about\n> >>>>> + * the other possible cases).\n> >>>>> + */\n> >>>>> + if (!!(combined & Transform::Transpose)) {\n> >>>>> + /*\n> >>>>> + * Flipping the transpose bit in \"transform\" flips it in\n> the\n> >>>>> + * combined result too (as it's the last thing that\n> happens),\n> >>>>> + * which is of course clearing it.\n> >>>>> + */\n> >>>>> + transform ^= Transform::Transpose;\n> >>>>> + combined &= ~Transform::Transpose;\n> >>>>> status = Adjusted;\n> >>>>> }\n> >>>>>\n> >>>>> + /*\n> >>>>> + * We also check if the sensor doesn't do h/vflips at all, in which\n> >>>>> + * case we clear them, and the application will have to do\n> everything.\n> >>>>> + */\n> >>>>> + if (!data_->supportsFlips_ && !!combined) {\n> >>>>> + /*\n> >>>>> + * If the sensor can do no transforms, then combined must\n> be\n> >>>>> + * changed to the identity. The only user transform that\n> gives\n> >>>>> + * rise to this is the inverse of the rotation. (Recall\n> that\n> >>>>> + * combined = transform * rotationTransform.)\n> >>>>> + */\n> >>>>> + transform = -data_->rotationTransform_;\n> >>>>> + combined = Transform::Identity;\n> >>>>> + status = Adjusted;\n> >>>>> + }\n> >>>>> +\n> >>>>> + /*\n> >>>>> + * Store the final combined transform that configure() will need to\n> >>>>> + * apply to the sensor to save us working it out again.\n> >>>>> + */\n> >>>>> + combinedTransform_ = combined;\n> >>>>> +\n> >>>>> /* Cap the number of entries to the available streams. */\n> >>>>> if (config_.size() > IPU3_MAX_STREAMS) {\n> >>>>> config_.resize(IPU3_MAX_STREAMS);\n> >>>>> @@ -540,6 +591,19 @@ int PipelineHandlerIPU3::configure(Camera\n> *camera, CameraConfiguration *c)\n> >>>>> return ret;\n> >>>>> }\n> >>>>>\n> >>>>> + /*\n> >>>>> + * Configure the H/V flip controls based on the combination of\n> >>>>> + * the sensor and user transform.\n> >>>>> + */\n> >>>>> + if (data->supportsFlips_) {\n> >>>>> + ControlList sensor_ctrls(cio2->sensor()->controls());\n> >>>>> + sensor_ctrls.set(V4L2_CID_HFLIP,\n> >>>>> +\n> static_cast(!!(config->combinedTransform_ & Transform::HFlip)));\n> >>>>> + sensor_ctrls.set(V4L2_CID_VFLIP,\n> >>>>> +\n> static_cast(!!(config->combinedTransform_ & Transform::VFlip)));\n> >>>>> + cio2->sensor()->setControls(&sensor_ctrls);\n> >>>>> + }\n> >>>>> +\n> >>>>> return 0;\n> >>>>> }\n> >>>>>\n> >>>>> @@ -775,9 +839,22 @@ int PipelineHandlerIPU3::registerCameras()\n> >>>>> /* Initialize the camera properties. */\n> >>>>> data->properties_ = cio2->sensor()->properties();\n> >>>>>\n> >>>>> + /* Convert the sensor rotation to a transformation */\n> >>>>> + int32_t rotation =\n> data->properties_.get(properties::Rotation);\n> >>>>\n> >>>> There's no guarantee the CameraSensor class register the Rotation\n> >>>> property. It was defaulted to 0 if the information was not available\n> >>>> from the firmware interface.\n> >>>>\n> >>>> It is now not anymore (a very recent change, sorry)\n> >>>>\n> https://git.libcamera.org/libcamera/libcamera.git/commit/?id=c35123e6f91b6269cf6f5ae9216aeccebc545c75\n> >>>>\n> >>>> Not sure what would be best, default to 0 in each pipeline handler\n> >>>> that need that information ?\n> >>>\n> >>> That sounds good to me for now.\n> >>>\n> >>\n> >> Ok, thanks for checking. Should I provide a patch for the raspberry\n> pipeline as well?\n> >\n> > I would not mind, but feel free to stick to IPU3 if that's quicker.\n> >\n> > Cc Naush for RPi. I'm thinking that, being RPi a bit more 'strict' as\n> > platform and with a little more control over sensor drivers/fw, maybe\n> > they want to refuse setups where no rotation is specified.\n>\n\nI think we should be ok with defaulting to 0 if the rotation property is\nunavailable. Worst that would happen is that we would produce an inverted\npicture - and then the user knows to add rotation in the driver :-)\nI'll let David have the final say though.\n\nRegards,\nNaush\n\n\n> >\n> >>\n> >>>>> + bool success;\n> >>>>> + data->rotationTransform_ = transformFromRotation(rotation,\n> &success);\n> >>>>> + if (!success)\n> >>>>> + LOG(IPU3, Warning) << \"Invalid rotation of \" <<\n> rotation << \" degrees - ignoring\";\n> >>>>> +\n> >>>>> /* Initialze the camera controls. */\n> >>>>> data->controlInfo_ = IPU3Controls;\n> >>>>>\n> >>>>> + ControlList ctrls = cio2->sensor()->getControls({\n> V4L2_CID_HFLIP });\n> >>>>> + if (!ctrls.empty()) {\n> >>>>> + /* We assume it will support vflips too... */\n> >>>>> + data->supportsFlips_ = true;\n> >>>>> + }\n> >>>>> +\n> >>>>> /**\n> >>>>> * \\todo Dynamically assign ImgU and output devices to each\n> >>>>> * stream and camera; as of now, limit support to two\n> cameras\n> >>>\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n>\n> --\n> Regards\n> --\n> Kieran\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 497ADC0F2A\n\tfor ;\n\tFri, 22 Jan 2021 13:07:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D400F68273;\n\tFri, 22 Jan 2021 14:07:51 +0100 (CET)","from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DB9046826B\n\tfor ;\n\tFri, 22 Jan 2021 14:07:49 +0100 (CET)","by mail-lf1-x129.google.com with SMTP id o19so7460824lfo.1\n\tfor ;\n\tFri, 22 Jan 2021 05:07:49 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"EmZ+E62S\"; 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=Am6kTbeQ8leVUBNOWShcluAOOz5rrT6Ix8GQ+a2KneQ=;\n\tb=EmZ+E62SrtpRfbPKkdKtWMzkcxl2ZZhDLIFPlNFUB4l1bc4W4zg501WLK8kSpLUEmV\n\tmJvJmWk/kfedfymSHd+7j3U4xHSQxCX9cC364p6dVS8qrfdwQQHKlZ2H2ONcIht8TnXO\n\tRR6CM5eTabggFwb2pCMsPDYkaqq7KtT8dnbRKU8BdS2UzZ5bwTMV6VjQgjG+TQUAGrvj\n\t/Ve14uYT/BX2AN3boJf5wIYodoyuS/fJj/KqTPJlz8vMTh/jJf/VNABnGckindDnA5Gn\n\t5T1SwHDIXtIEaHC8YxVioaWuTLbfEkKpberwK8EBaZUps7YRE3ekL2pFhGgIVLX6BvIR\n\t17KA==","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=Am6kTbeQ8leVUBNOWShcluAOOz5rrT6Ix8GQ+a2KneQ=;\n\tb=G3fdvnt8GY31zA7dPGwIlEhCoRKjn0qHru3ND7fWY23U0b1id0tRqmRn9iYVBrRxbo\n\t+VcjO91kU9UlbxTI9jkSQbRLgGFB7OYsGmzxiTeyvZCxyU7NS7gRGCtsQm6CEW8+l8XU\n\txEsSFnUYoiXhsPubVVD72Az44/2X86l/u0giA+RPbv5axdiJ4e1zue5XlBZDcZ3PtFIE\n\tpvxAQWn/GY49tTDv0arqdeQwfWvjgzduIDSkFSgC//84/dXEyauidKCFCIkEgXdyDc/n\n\tuR1LASvRcW9ZkkXHouCliT4HCezVo6/XhH4vFAhRA6/FZH7TU83N1oT8igNlTdXKBJmH\n\tflWg==","X-Gm-Message-State":"AOAM530FaXPaxjvQCOI31f8SiRE3i9zMVSD/pfG1Bpk+pfYF6dysYrAF\n\t+bkKSyYee6Y54WN8Ryz73gwNfZZhg8VH6FDGU5cRNKWMQ7s=","X-Google-Smtp-Source":"ABdhPJxzdWB8Cok6yi/Hm31Mhk081Ek8MATiMouK9RwfGLe/wxSuxLzvNPDyPkDWK84dvpq4kRgxCJcwQIcyFqKITbs=","X-Received":"by 2002:ac2:5b03:: with SMTP id v3mr1448037lfn.171.1611320869026;\n\tFri, 22 Jan 2021 05:07:49 -0800 (PST)","MIME-Version":"1.0","References":"<20210106133513.34118-1-me@fabwu.ch>\n\t<20210116150257.2970-1-me@fabwu.ch>\n\t<20210118100631.l66zk2uuhnpsllm6@uno.localdomain>\n\t\n\t\n\t<20210122085210.2k3ihvvwancteljx@uno.localdomain>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 22 Jan 2021 13:07:32 +0000","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v2] 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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============2470067906558655915==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14715,"web_url":"https://patchwork.libcamera.org/comment/14715/","msgid":"","date":"2021-01-22T13:57:33","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: ipu3: Add rotation to\n\tipu3 pipeline","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nOn Fri, 22 Jan 2021 at 13:07, Naushir Patuck wrote:\n>\n> Hi all,\n>\n>\n> On Fri, 22 Jan 2021 at 12:20, Kieran Bingham wrote:\n>>\n>> Hi Fabian, Jacopo, Naush, David ;-)\n>>\n>> On 22/01/2021 08:52, Jacopo Mondi wrote:\n>> > Hi Fabian,\n>> > + Naush for RPi question\n>>\n>> Fixing the +CC for the question below - but also adding David as he\n>> wrote the original Transforms.\n>>\n>> --\n>> Kieran\n>>\n>>\n>> >\n>> > On Fri, Jan 22, 2021 at 09:10:42AM +0100, Fabian Wüthrich wrote:\n>> >> Hi Laurent,\n>> >>\n>> >> On 22.01.21 00:37, Laurent Pinchart wrote:\n>> >>> Hi Jacopo and Fabian,\n>> >>>\n>> >>> On Mon, Jan 18, 2021 at 11:06:31AM +0100, Jacopo Mondi wrote:\n>> >>>> On Sat, Jan 16, 2021 at 04:02:58PM +0100, Fabian Wüthrich wrote:\n>> >>>>> Use the same transformation logic as in the raspberry pipeline to\n>> >>>>> implement rotations in the ipu3 pipeline.\n>> >>>\n>> >>> This should eventually be shared between different pipeline handlers,\n>> >>> but for now it's fine.\n>> >>>\n>> >>\n>> >> Agreed.\n>> >>\n>> >>>>> Tested on a Surface Book 2 with an experimental driver for OV5693.\n>> >>>>>\n>> >>>>> Signed-off-by: Fabian Wüthrich \n>> >>>>> ---\n>> >>>>> Changes in v2:\n>> >>>>> - Cache rotationTransform in CameraData\n>> >>>>> - Use separate controls for sensor\n>> >>>>>\n>> >>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 81 +++++++++++++++++++++++++++-\n>> >>>>> 1 file changed, 79 insertions(+), 2 deletions(-)\n>> >>>>>\n>> >>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> >>>>> index 73304ea7..8db5f2f1 100644\n>> >>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> >>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> >>>>> @@ -14,6 +14,7 @@\n>> >>>>> #include \n>> >>>>> #include \n>> >>>>> #include \n>> >>>>> +#include \n>> >>>>> #include \n>> >>>>> #include \n>> >>>>>\n>> >>>>> @@ -62,6 +63,15 @@ public:\n>> >>>>> Stream outStream_;\n>> >>>>> Stream vfStream_;\n>> >>>>> Stream rawStream_;\n>> >>>>> +\n>> >>>>> + /* Save transformation given by the sensor rotation */\n>> >>>>> + Transform rotationTransform_;\n>> >>>>> +\n>> >>>>> + /*\n>> >>>>> + * Manage horizontal and vertical flips supported (or not) by the\n>> >>>>> + * sensor.\n>> >>>>> + */\n>> >>>>> + bool supportsFlips_;\n>> >>>>> };\n>> >>>>>\n>> >>>>> class IPU3CameraConfiguration : public CameraConfiguration\n>> >>>>> @@ -74,6 +84,9 @@ public:\n>> >>>>> const StreamConfiguration &cio2Format() const { return cio2Configuration_; }\n>> >>>>> const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }\n>> >>>>>\n>> >>>>> + /* Cache the combinedTransform_ that will be applied to the sensor */\n>> >>>>> + Transform combinedTransform_;\n>> >>>>> +\n>> >>>>> private:\n>> >>>>> /*\n>> >>>>> * The IPU3CameraData instance is guaranteed to be valid as long as the\n>> >>>>> @@ -143,11 +156,49 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>> >>>>> if (config_.empty())\n>> >>>>> return Invalid;\n>> >>>>>\n>> >>>>> - if (transform != Transform::Identity) {\n>> >>>>> - transform = Transform::Identity;\n>> >>>>> + Transform combined = transform * data_->rotationTransform_;\n>> >>>>> +\n>> >>>>> + /*\n>> >>>>> + * We combine the platform and user transform, but must \"adjust away\"\n>> >>>>> + * any combined result that includes a transposition, as we can't do those.\n>> >>>>> + * In this case, flipping only the transpose bit is helpful to\n>> >>>>> + * applications - they either get the transform they requested, or have\n>> >>>>> + * to do a simple transpose themselves (they don't have to worry about\n>> >>>>> + * the other possible cases).\n>> >>>>> + */\n>> >>>>> + if (!!(combined & Transform::Transpose)) {\n>> >>>>> + /*\n>> >>>>> + * Flipping the transpose bit in \"transform\" flips it in the\n>> >>>>> + * combined result too (as it's the last thing that happens),\n>> >>>>> + * which is of course clearing it.\n>> >>>>> + */\n>> >>>>> + transform ^= Transform::Transpose;\n>> >>>>> + combined &= ~Transform::Transpose;\n>> >>>>> status = Adjusted;\n>> >>>>> }\n>> >>>>>\n>> >>>>> + /*\n>> >>>>> + * We also check if the sensor doesn't do h/vflips at all, in which\n>> >>>>> + * case we clear them, and the application will have to do everything.\n>> >>>>> + */\n>> >>>>> + if (!data_->supportsFlips_ && !!combined) {\n>> >>>>> + /*\n>> >>>>> + * If the sensor can do no transforms, then combined must be\n>> >>>>> + * changed to the identity. The only user transform that gives\n>> >>>>> + * rise to this is the inverse of the rotation. (Recall that\n>> >>>>> + * combined = transform * rotationTransform.)\n>> >>>>> + */\n>> >>>>> + transform = -data_->rotationTransform_;\n>> >>>>> + combined = Transform::Identity;\n>> >>>>> + status = Adjusted;\n>> >>>>> + }\n>> >>>>> +\n>> >>>>> + /*\n>> >>>>> + * Store the final combined transform that configure() will need to\n>> >>>>> + * apply to the sensor to save us working it out again.\n>> >>>>> + */\n>> >>>>> + combinedTransform_ = combined;\n>> >>>>> +\n>> >>>>> /* Cap the number of entries to the available streams. */\n>> >>>>> if (config_.size() > IPU3_MAX_STREAMS) {\n>> >>>>> config_.resize(IPU3_MAX_STREAMS);\n>> >>>>> @@ -540,6 +591,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>> >>>>> return ret;\n>> >>>>> }\n>> >>>>>\n>> >>>>> + /*\n>> >>>>> + * Configure the H/V flip controls based on the combination of\n>> >>>>> + * the sensor and user transform.\n>> >>>>> + */\n>> >>>>> + if (data->supportsFlips_) {\n>> >>>>> + ControlList sensor_ctrls(cio2->sensor()->controls());\n>> >>>>> + sensor_ctrls.set(V4L2_CID_HFLIP,\n>> >>>>> + static_cast(!!(config->combinedTransform_ & Transform::HFlip)));\n>> >>>>> + sensor_ctrls.set(V4L2_CID_VFLIP,\n>> >>>>> + static_cast(!!(config->combinedTransform_ & Transform::VFlip)));\n>> >>>>> + cio2->sensor()->setControls(&sensor_ctrls);\n>> >>>>> + }\n>> >>>>> +\n>> >>>>> return 0;\n>> >>>>> }\n>> >>>>>\n>> >>>>> @@ -775,9 +839,22 @@ int PipelineHandlerIPU3::registerCameras()\n>> >>>>> /* Initialize the camera properties. */\n>> >>>>> data->properties_ = cio2->sensor()->properties();\n>> >>>>>\n>> >>>>> + /* Convert the sensor rotation to a transformation */\n>> >>>>> + int32_t rotation = data->properties_.get(properties::Rotation);\n>> >>>>\n>> >>>> There's no guarantee the CameraSensor class register the Rotation\n>> >>>> property. It was defaulted to 0 if the information was not available\n>> >>>> from the firmware interface.\n>> >>>>\n>> >>>> It is now not anymore (a very recent change, sorry)\n>> >>>> https://git.libcamera.org/libcamera/libcamera.git/commit/?id=c35123e6f91b6269cf6f5ae9216aeccebc545c75\n>> >>>>\n>> >>>> Not sure what would be best, default to 0 in each pipeline handler\n>> >>>> that need that information ?\n>> >>>\n>> >>> That sounds good to me for now.\n>> >>>\n>> >>\n>> >> Ok, thanks for checking. Should I provide a patch for the raspberry pipeline as well?\n>> >\n>> > I would not mind, but feel free to stick to IPU3 if that's quicker.\n>> >\n>> > Cc Naush for RPi. I'm thinking that, being RPi a bit more 'strict' as\n>> > platform and with a little more control over sensor drivers/fw, maybe\n>> > they want to refuse setups where no rotation is specified.\n>\n>\n> I think we should be ok with defaulting to 0 if the rotation property is unavailable. Worst that would happen is that we would produce an inverted picture - and then the user knows to add rotation in the driver :-)\n> I'll let David have the final say though.\n\nSounds fine to me too!\n\nThanks\nDavid\n\n>\n> Regards,\n> Naush\n>\n>>\n>> >\n>> >>\n>> >>>>> + bool success;\n>> >>>>> + data->rotationTransform_ = transformFromRotation(rotation, &success);\n>> >>>>> + if (!success)\n>> >>>>> + LOG(IPU3, Warning) << \"Invalid rotation of \" << rotation << \" degrees - ignoring\";\n>> >>>>> +\n>> >>>>> /* Initialze the camera controls. */\n>> >>>>> data->controlInfo_ = IPU3Controls;\n>> >>>>>\n>> >>>>> + ControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP });\n>> >>>>> + if (!ctrls.empty()) {\n>> >>>>> + /* We assume it will support vflips too... */\n>> >>>>> + data->supportsFlips_ = true;\n>> >>>>> + }\n>> >>>>> +\n>> >>>>> /**\n>> >>>>> * \\todo Dynamically assign ImgU and output devices to each\n>> >>>>> * stream and camera; as of now, limit support to two cameras\n>> >>>\n>> > _______________________________________________\n>> > libcamera-devel mailing list\n>> > libcamera-devel@lists.libcamera.org\n>> > https://lists.libcamera.org/listinfo/libcamera-devel\n>> >\n>>\n>> --\n>> Regards\n>> --\n>> Kieran","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 A62CBC0F2A\n\tfor ;\n\tFri, 22 Jan 2021 13:57:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A2EC68291;\n\tFri, 22 Jan 2021 14:57:46 +0100 (CET)","from mail-oi1-x236.google.com (mail-oi1-x236.google.com\n\t[IPv6:2607:f8b0:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 192D96827A\n\tfor ;\n\tFri, 22 Jan 2021 14:57:45 +0100 (CET)","by mail-oi1-x236.google.com with SMTP id 15so6029001oix.8\n\tfor ;\n\tFri, 22 Jan 2021 05:57:45 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"ZatuTAkt\"; 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:content-transfer-encoding;\n\tbh=1hNJNK89OoTu3m/hFsfnQ1sj/UEUrqQPCDbrIar44kY=;\n\tb=ZatuTAktbveQDyYucmj8Cb0y7FLbyN6emAWwbnMxw1KL/YsOxFixQ58yK78DwWj7IY\n\tGtF2A5f9d1FozxjrVhXV2fzBXowFFoRl2Srjcbaqm1hlEf33D0h59vfZPKnkpnfFgefe\n\tnIfhC/AGEpdcJilgWGV0IAdkcFKKdDr+r/2cE8pW8m7g+MX5QeJBUPcrAjMhvzzR/XAt\n\tcHWSWj636JnsKihGbJFQUeNn9fWSiuxUJ7RfvhTiGju3++wbH/xTvMEM9IjRvG15MSIK\n\tpmIPNS+UnIK9xEovHywda/UtNSdLNXK2Olb3CDkbi9BuZy4ijG3QI7N8o65LZ+s67hvd\n\tZ1uw==","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:content-transfer-encoding;\n\tbh=1hNJNK89OoTu3m/hFsfnQ1sj/UEUrqQPCDbrIar44kY=;\n\tb=nL7vIPcw+Y6abSxrcuimhUL1OVzstd+f3qq+r44DUH1YZgd2QIWHX7ltT4zDbyGiIb\n\trLxtdp/R47+5sP2X9DWmgJnjLqsj5e7NusDBwagjB1uC8g1F8jKVxWSDPmVFiJHgx6mI\n\tkGuAH64LAf++feI7K4i8vS0W4eoN3CzBxJd8yt9pOVYGXW5ej48+YBhWuxd2ECydQp8H\n\tP6A5YNuBGZiNW8b4hSMtdSJtEE8dUGkIJ5CDnJCyq8t1jP8w+NastIQRsi7cY+3rAP7a\n\tiWASqNRcl5FtifpM2kHiUZEjIYYTGx+LlSJ99rvrargZfxsK6LSG1bSHH9DTyki2/OTx\n\tU0SA==","X-Gm-Message-State":"AOAM530WxHfdLD7BPveiOvWdf09U0vc3uc4ueHdMmLb2o/u3IBh0WwZc\n\tA3A2u4HlKC1Du5tWR61RSVEpteVB0WBAbs3XwqAOAg==","X-Google-Smtp-Source":"ABdhPJzjXa2h2xxwplzGIEr0j64RpS43/q1MFkQnXaezzW1VCuNIORDJG+0AJWhdEyR+IAdJzto9WX2TpR/EliW7R9Y=","X-Received":"by 2002:aca:5c0a:: with SMTP id q10mr3353053oib.55.1611323863930;\n\tFri, 22 Jan 2021 05:57:43 -0800 (PST)","MIME-Version":"1.0","References":"<20210106133513.34118-1-me@fabwu.ch>\n\t<20210116150257.2970-1-me@fabwu.ch>\n\t<20210118100631.l66zk2uuhnpsllm6@uno.localdomain>\n\t\n\t\n\t<20210122085210.2k3ihvvwancteljx@uno.localdomain>\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Fri, 22 Jan 2021 13:57:33 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH v2] 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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]