[{"id":25726,"web_url":"https://patchwork.libcamera.org/comment/25726/","msgid":"","date":"2022-11-03T11:02:17","subject":"Re: [libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do\n\tnot clear camera flips when listing formats","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi David\n\nOn Thu, 3 Nov 2022 at 10:40, David Plowman via libcamera-devel\n wrote:\n>\n> Previously the code used to clear the camnera's h and v flip bits when\n> enumerating the supported formats so as to obtain any Bayer formats in\n> the sensor's native (untransformed) orientation. However this fails\n> when the camera is already in use elsewhere.\n>\n> Instead, we query the current state of the flip bits and transform the\n> formats - which we obtain in their flipped orientation - back into\n> their native orientation to be stored.\n\nI don't believe that libcamera knows for definite whether the sensor\nchanges the Bayer order or not. Several of the OnSemi sensors I have\nseen do not change, presumably as they shift their crop by 1 pixel.\n\nThe original comment mentioned that scenario:\n\"This is harmless for sensors where the flips don't affect the Bayer order\"\n\n Dave\n\n> Signed-off-by: David Plowman \n> ---\n> src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++------\n> 1 file changed, 43 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 572a313a..6670dfb9 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -19,6 +19,8 @@\n>\n> #include \n>\n> +#include \n> +\n> #include \"libcamera/internal/bayer_format.h\"\n> #include \"libcamera/internal/camera_lens.h\"\n> #include \"libcamera/internal/camera_sensor_properties.h\"\n> @@ -108,18 +110,51 @@ int CameraSensor::init()\n> return ret;\n>\n> /*\n> - * Clear any flips to be sure we get the \"native\" Bayer order. This is\n> - * harmless for sensors where the flips don't affect the Bayer order.\n> + * We want to get the native mbus codes for the sensor, without any flips.\n> + * We can't clear any flips here, so we have to read the current values\n> + * (if the flip controls exist), decide whether they actually modify any\n> + * output Bayer pattern, and finally undo their effect on the formats.\n> + *\n> + * First, check if the flip controls exist and if so read them.\n> */\n> ControlList ctrls(subdev_->controls());\n> - if (subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end())\n> - ctrls.set(V4L2_CID_HFLIP, 0);\n> - if (subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end())\n> - ctrls.set(V4L2_CID_VFLIP, 0);\n> - subdev_->setControls(&ctrls);\n> + std::vector flipCtrlIds;\n> + bool hasHflip = subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end();\n> + bool hasVflip = subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end();\n> + if (hasHflip)\n> + flipCtrlIds.push_back(V4L2_CID_HFLIP);\n> + if (hasVflip)\n> + flipCtrlIds.push_back(V4L2_CID_VFLIP);\n> + ControlList flipCtrls = subdev_->getControls(flipCtrlIds);\n> +\n> + /* Now construct a transform that would undo any flips. */\n> + Transform transform = Transform::Identity;\n> + if (hasHflip && flipCtrls.get(V4L2_CID_HFLIP).get()) {\n> + const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_HFLIP);\n> + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)\n> + transform |= Transform::HFlip;\n> + }\n> + if (hasVflip && flipCtrls.get(V4L2_CID_VFLIP).get()) {\n> + const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_VFLIP);\n> + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)\n> + transform |= Transform::VFlip;\n> + }\n> +\n> + /* Finally get the formats, and apply the transform to the mbus codes. */\n> + auto formats = subdev_->formats(pad_);\n> + for (const auto &format : formats) {\n> + unsigned int mbusCode = format.first;\n> + BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> + bool valid = true;\n> +\n> + if (bayerFormat.isValid())\n> + mbusCode = bayerFormat.transform(transform).toMbusCode(valid);\n> +\n> + if (valid)\n> + formats_[mbusCode] = std::move(format.second);\n> + }\n>\n> /* Enumerate, sort and cache media bus codes and sizes. */\n> - formats_ = subdev_->formats(pad_);\n> if (formats_.empty()) {\n> LOG(CameraSensor, Error) << \"No image format found\";\n> return -EINVAL;\n> --\n> 2.30.2\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 96538BDB16\n\tfor ;\n\tThu, 3 Nov 2022 11:02:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E9F1763036;\n\tThu, 3 Nov 2022 12:02:37 +0100 (CET)","from mail-ej1-x629.google.com (mail-ej1-x629.google.com\n\t[IPv6:2a00:1450:4864:20::629])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 707F661F42\n\tfor ;\n\tThu, 3 Nov 2022 12:02:36 +0100 (CET)","by mail-ej1-x629.google.com with SMTP id f5so4184810ejc.5\n\tfor ;\n\tThu, 03 Nov 2022 04:02:36 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667473357;\n\tbh=AUAR8qnWOkSMj2UJYik1+L0PFxTnwz2OVsvN66A8N8g=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=i1QluLPU4ILxZf7cvCDTW0Walz7UNNz3yjPGPsl8Tm2Pdr66Tf1r+eKlQJcMkOV/h\n\tvtxHPGBAEf3R5c98Etc/4T0Betl/PKpT6QzQiQTtrKh4DIVEWOGFBpfLHtWX1MZcas\n\tGEAt91+VihGEKwN0trOYHFkDGAHEbRJuxDczcFrrAmHgTf3IIb3SR7trl+yRlOXk3f\n\tzAfQdr188/BUGRE51NUfMsyG/ke76cYISyiD4RfJGbSZ0ySSUTraSfwyiNI5Ns71wS\n\tPcaNTmrV4si6CWMM2Ih+8Vl2tZD/hwhd15ZJcXtsB4uuDLF3RhwqP7xJNkGHEgQtGv\n\t2o1ZuBkFxmNdA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=/KucpG7bGMDzO3M4R6LxFggikL3Q34DNo0GgSZooqes=;\n\tb=dZn21xAF4XWb4aNaZFvfcBvUVpMQL8U91V4JkGI+XX4BL4dBMkt37J504s1f9FsILq\n\t6qCjYK/1qLW3eEKdYoNPbJOxVCQ1Gj5K4It9i9FJ1EJBxaVp5NdbZwcHryVJHY06JqD4\n\tGYfBkSe4FRkU3Prip9iMvXIAAPFMJRqwfmN2GDhkZSqj9/gYHfGNI/gGVjFxTuNHG2hO\n\tR+1ZjqwUXHnoO7Wcf9fALCYA3rEOQJ8l1cHFhB/BcaG8BHb4bOdj8ODjp67ycENy3gnC\n\tcMDCSLV9gCxidb6/wcuT0St6NDKjaNJjNn+VFUlzWOac89AIFgnMm3vqcbHOxINw59Ei\n\tLBmg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"dZn21xAF\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=/KucpG7bGMDzO3M4R6LxFggikL3Q34DNo0GgSZooqes=;\n\tb=XYMNOZc2XNP16o3Z7GUXyJLfZOdoBhISseI1hetuhrKCjViXtbXsoFnAn9zZvn0xgi\n\tjDhgscULlokcYP618nVGHLTcRGerm8+wcf+V8z1YjzAv0lzd7dcxw6XIeJEfODG3Gf/n\n\tKrOybiKn0peNyqAHo7kqJ8z+Oh9a+8XC31LQUYjKKKJeUJk/qhE13l+O0sl27YaurLha\n\tJ6T122QsPW5GVApMGqHonjHc9S8NHcZN0W5AWl3IJEYQ2gHp4rnFv4v1UOUBMbr+/5g+\n\tCFIwF/u4WFnD+95LZmbOu7xUj+WjCgdziuyelWOBgNVYhyb1U6B1afglgQ0je4syyceb\n\tF/pg==","X-Gm-Message-State":"ACrzQf0S2tihqzEfuRp/ZHvprEOEJhN+jCMBEcbM4qRDPvC6IGVBiF7G\n\tej3dV5MwQQ2/xp8+IxA8j8aBnl4Q9bul0hsIqc8Xv/yDp9Gaaw==","X-Google-Smtp-Source":"AMsMyM7jcSBEFqtFr4v5iOQtFvFPExZIUm00tGhpUG4jGFoN2ZywgD2YDz1tAghNRec1N1Lmj7pGQmGWeYf2jHKqwb0=","X-Received":"by 2002:a17:907:94c7:b0:78e:1c4f:51f9 with SMTP id\n\tdn7-20020a17090794c700b0078e1c4f51f9mr29303629ejc.200.1667473356019;\n\tThu, 03 Nov 2022 04:02:36 -0700 (PDT)","MIME-Version":"1.0","References":"<20221103104027.4197-1-david.plowman@raspberrypi.com>\n\t<20221103104027.4197-3-david.plowman@raspberrypi.com>","In-Reply-To":"<20221103104027.4197-3-david.plowman@raspberrypi.com>","Date":"Thu, 3 Nov 2022 11:02:17 +0000","Message-ID":"","To":"David Plowman ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do\n\tnot clear camera flips when listing formats","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","From":"Dave Stevenson via libcamera-devel\n\t","Reply-To":"Dave Stevenson ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25727,"web_url":"https://patchwork.libcamera.org/comment/25727/","msgid":"","date":"2022-11-03T11:24:23","subject":"Re: [libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do\n\tnot clear camera flips when listing formats","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Dave\n\nOn Thu, 3 Nov 2022 at 11:02, Dave Stevenson\n wrote:\n>\n> Hi David\n>\n> On Thu, 3 Nov 2022 at 10:40, David Plowman via libcamera-devel\n> wrote:\n> >\n> > Previously the code used to clear the camnera's h and v flip bits when\n> > enumerating the supported formats so as to obtain any Bayer formats in\n> > the sensor's native (untransformed) orientation. However this fails\n> > when the camera is already in use elsewhere.\n> >\n> > Instead, we query the current state of the flip bits and transform the\n> > formats - which we obtain in their flipped orientation - back into\n> > their native orientation to be stored.\n>\n> I don't believe that libcamera knows for definite whether the sensor\n> changes the Bayer order or not. Several of the OnSemi sensors I have\n> seen do not change, presumably as they shift their crop by 1 pixel.\n\nYes, that's a good point. I've vaguely assumed that I can check the\ncontrol's V4L2_CTRL_FLAG_MODIFY_LAYOUT to discover whether this is the\ncase or not, but as we keep on discovering, there may be no guarantees\nabout this. Do we know what these particular sensors do here? The Pi\nPH actually already assumes this flag \"works\", so the changes here\npossibly don't make anything worse, though they do engrain the\nassumption more deeply...\n\nPresumably you can rely on the formats being returned correctly after\nsetting the flip bits, though having to re-query the formats from the\ndevice like that would be irritating too. And you'd never be able to\npredict the format ahead of time, only after setting it. I don't know\nif that's better or worse than saying \"change the driver\". Opinions\nwelcome on that one!\n\nTo be honest I always have difficulty with the Bayer order being part\nof the format. An application might legitimately want to have a say\nabout the bit depth or packing, but the Bayer order?? Being able to\nrequest an XXXX (\"anything goes\") Bayer order, which gets updated once\nthe sensor is configured, would be much more application-friendly. But\nanother can of worms!!\n\nDavid\n\n>\n> The original comment mentioned that scenario:\n> \"This is harmless for sensors where the flips don't affect the Bayer order\"\n>\n> Dave\n>\n> > Signed-off-by: David Plowman \n> > ---\n> > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++------\n> > 1 file changed, 43 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 572a313a..6670dfb9 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -19,6 +19,8 @@\n> >\n> > #include \n> >\n> > +#include \n> > +\n> > #include \"libcamera/internal/bayer_format.h\"\n> > #include \"libcamera/internal/camera_lens.h\"\n> > #include \"libcamera/internal/camera_sensor_properties.h\"\n> > @@ -108,18 +110,51 @@ int CameraSensor::init()\n> > return ret;\n> >\n> > /*\n> > - * Clear any flips to be sure we get the \"native\" Bayer order. This is\n> > - * harmless for sensors where the flips don't affect the Bayer order.\n> > + * We want to get the native mbus codes for the sensor, without any flips.\n> > + * We can't clear any flips here, so we have to read the current values\n> > + * (if the flip controls exist), decide whether they actually modify any\n> > + * output Bayer pattern, and finally undo their effect on the formats.\n> > + *\n> > + * First, check if the flip controls exist and if so read them.\n> > */\n> > ControlList ctrls(subdev_->controls());\n> > - if (subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end())\n> > - ctrls.set(V4L2_CID_HFLIP, 0);\n> > - if (subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end())\n> > - ctrls.set(V4L2_CID_VFLIP, 0);\n> > - subdev_->setControls(&ctrls);\n> > + std::vector flipCtrlIds;\n> > + bool hasHflip = subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end();\n> > + bool hasVflip = subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end();\n> > + if (hasHflip)\n> > + flipCtrlIds.push_back(V4L2_CID_HFLIP);\n> > + if (hasVflip)\n> > + flipCtrlIds.push_back(V4L2_CID_VFLIP);\n> > + ControlList flipCtrls = subdev_->getControls(flipCtrlIds);\n> > +\n> > + /* Now construct a transform that would undo any flips. */\n> > + Transform transform = Transform::Identity;\n> > + if (hasHflip && flipCtrls.get(V4L2_CID_HFLIP).get()) {\n> > + const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_HFLIP);\n> > + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)\n> > + transform |= Transform::HFlip;\n> > + }\n> > + if (hasVflip && flipCtrls.get(V4L2_CID_VFLIP).get()) {\n> > + const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_VFLIP);\n> > + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)\n> > + transform |= Transform::VFlip;\n> > + }\n> > +\n> > + /* Finally get the formats, and apply the transform to the mbus codes. */\n> > + auto formats = subdev_->formats(pad_);\n> > + for (const auto &format : formats) {\n> > + unsigned int mbusCode = format.first;\n> > + BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> > + bool valid = true;\n> > +\n> > + if (bayerFormat.isValid())\n> > + mbusCode = bayerFormat.transform(transform).toMbusCode(valid);\n> > +\n> > + if (valid)\n> > + formats_[mbusCode] = std::move(format.second);\n> > + }\n> >\n> > /* Enumerate, sort and cache media bus codes and sizes. */\n> > - formats_ = subdev_->formats(pad_);\n> > if (formats_.empty()) {\n> > LOG(CameraSensor, Error) << \"No image format found\";\n> > return -EINVAL;\n> > --\n> > 2.30.2\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 A4E66BD16B\n\tfor ;\n\tThu, 3 Nov 2022 11:24:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F30CB6307D;\n\tThu, 3 Nov 2022 12:24:36 +0100 (CET)","from mail-pj1-x102c.google.com (mail-pj1-x102c.google.com\n\t[IPv6:2607:f8b0:4864:20::102c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3228661F42\n\tfor ;\n\tThu, 3 Nov 2022 12:24:36 +0100 (CET)","by mail-pj1-x102c.google.com with SMTP id\n\td13-20020a17090a3b0d00b00213519dfe4aso1510688pjc.2\n\tfor ;\n\tThu, 03 Nov 2022 04:24:36 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667474677;\n\tbh=7JlnHpGnM/V2bIyBk4d9tV0mbvJK1tQu9u0A+uplzyI=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=vi1ffRhVFLJLOR2vR7KGUKmoLVs3I06g/yGtffuwOpZV3azdycTNTzdka6OIHDpkh\n\tyGwjlpGQwnqF3POORmjZuyAWxAb7yRfOYYMmzhMH4j1Vp5U/PLMxZR5q/60PTy9/Z8\n\tck5F8PFoJxbzzmG4wOWWQJ1X+uK578uDk9QNeUnvH6sCe9pMprWF9sl++T47MbPqEZ\n\tfkZaNOSWr5c+0aS9sMuBvBMDAQH2XhAohUbrBKhP2jFlQYCV1BBCiLTxJD9isx5uly\n\tlAsC9sHyMOwldswVgyRMsWy4KWHIr8xNT1HZrXZzXazINSrjlExGg7icx+QIcTGZCD\n\tziEJuPxYQQRpw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=siDWlj+NjzwntOjipCVwnLBbCFXj0hkz2rcQKuIE5qo=;\n\tb=EqGD+bkDoQAgq8tI1lUMKKHqacq05rg334r/JXYRGBrFsFPpe3SIrMgH92p4aa248c\n\tf1XDP46UHbzM4oBZSutwrDnG7T2XcF3iBL/28cuALZ5KGlxwzSmXPOk9n2xS5NTAR/kZ\n\t9h/t+qUkgKn1tmybdki9Eht39lcJMwpkIJp/7sJ/80hOGQdBtxDV4EDIEJ9kIcyjYh7x\n\txRFZjZHcXqFEl83+jyVdByxx088fSVikeiMMxvhm71gx+Hznj3hKQNQ4LTTdx7ZQ42N4\n\tUyGZ+G1JTMEewDuWkl3F2QsXMD1XvC/P55kZ9OY6gEHBdabMnHhHxTlx5DZ06oPoVQ/I\n\tcW2A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"EqGD+bkD\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=siDWlj+NjzwntOjipCVwnLBbCFXj0hkz2rcQKuIE5qo=;\n\tb=FclJ9td+zvaeZYkZa11nEiHMSczVQtD1DhydvBKTc3MJhlu7MqckNQHnru9mlhnbd4\n\tZM+CzBm93XAyUuQ1h9HqXrUJJmndlXTgqzPTjwIKCB1Uk8nKjsKz5e9pt4Pa0Rget5if\n\tbm5Siz0kLefE4ScuEzOGjogSNgRYbLNrLDBYvpLwzs2IL7tAtNXbbdHIQSRLdLb40F5l\n\ttNd77oYFrUBxDRqQGLKVwgoB26PZVubNs1ThRxmixsWaJ5e8y9xTgVkPvwq4NOsR2XMz\n\t/2EbZEPhMuiKiiXWBql9ldDuXi+TGvCTloV3xrdRLkObtwOOb1g0WeH+dwBIuaOL+OKS\n\tM17w==","X-Gm-Message-State":"ACrzQf2rxcRlYJiu5t7qrl+7rUq4UVpl6DwsJD9BPi+Pa1lqwFwKGNjA\n\tHrC2Xr823Fykm6wPmL9+gFSYlGflXw4ZAtAXrszw+ajKGlM=","X-Google-Smtp-Source":"AMsMyM4iEMJRrEcqOqdkIXaVmXgVGHY49bsAeqWFxs6+jmfQK5QzfXXnjsSJKoIozAHINHylreglsZAgjOZjjlpaaDI=","X-Received":"by 2002:a17:903:284:b0:186:bb2c:b041 with SMTP id\n\tj4-20020a170903028400b00186bb2cb041mr29420387plr.36.1667474674528;\n\tThu, 03 Nov 2022 04:24:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20221103104027.4197-1-david.plowman@raspberrypi.com>\n\t<20221103104027.4197-3-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Thu, 3 Nov 2022 11:24:23 +0000","Message-ID":"","To":"Dave Stevenson ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do\n\tnot clear camera flips when listing formats","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25728,"web_url":"https://patchwork.libcamera.org/comment/25728/","msgid":"","date":"2022-11-03T11:25:06","subject":"Re: [libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do\n\tnot clear camera flips when listing formats","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Thu, 3 Nov 2022 at 11:02, Dave Stevenson via libcamera-devel <\nlibcamera-devel@lists.libcamera.org> wrote:\n\n> Hi David\n>\n> On Thu, 3 Nov 2022 at 10:40, David Plowman via libcamera-devel\n> wrote:\n> >\n> > Previously the code used to clear the camnera's h and v flip bits when\n> > enumerating the supported formats so as to obtain any Bayer formats in\n> > the sensor's native (untransformed) orientation. However this fails\n> > when the camera is already in use elsewhere.\n> >\n> > Instead, we query the current state of the flip bits and transform the\n> > formats - which we obtain in their flipped orientation - back into\n> > their native orientation to be stored.\n>\n> I don't believe that libcamera knows for definite whether the sensor\n> changes the Bayer order or not. Several of the OnSemi sensors I have\n> seen do not change, presumably as they shift their crop by 1 pixel.\n>\n\nThere is a flag associated with the H/V flips that can tell us if the\nsensor changes the bayer order:\n\nconst struct v4l2_query_ext_ctrl *hflipCtrl =\nsensor->controlInfo(V4L2_CID_HFLIP);\ndata->flipsAlterBayerOrder_ = hflipCtrl->flags &\nV4L2_CTRL_FLAG_MODIFY_LAYOUT;\n\nThis ought to help here.\n\n\n> The original comment mentioned that scenario:\n> \"This is harmless for sensors where the flips don't affect the Bayer order\"\n>\n> Dave\n>\n> > Signed-off-by: David Plowman \n> > ---\n> > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++------\n> > 1 file changed, 43 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp\n> b/src/libcamera/camera_sensor.cpp\n> > index 572a313a..6670dfb9 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -19,6 +19,8 @@\n> >\n> > #include \n> >\n> > +#include \n> > +\n> > #include \"libcamera/internal/bayer_format.h\"\n> > #include \"libcamera/internal/camera_lens.h\"\n> > #include \"libcamera/internal/camera_sensor_properties.h\"\n> > @@ -108,18 +110,51 @@ int CameraSensor::init()\n> > return ret;\n> >\n> > /*\n> > - * Clear any flips to be sure we get the \"native\" Bayer order.\n> This is\n> > - * harmless for sensors where the flips don't affect the Bayer\n> order.\n> > + * We want to get the native mbus codes for the sensor, without\n> any flips.\n> > + * We can't clear any flips here, so we have to read the current\n> values\n> > + * (if the flip controls exist), decide whether they actually\n> modify any\n> > + * output Bayer pattern, and finally undo their effect on the\n> formats.\n> > + *\n> > + * First, check if the flip controls exist and if so read them.\n> > */\n> > ControlList ctrls(subdev_->controls());\n> > - if (subdev_->controls().find(V4L2_CID_HFLIP) !=\n> subdev_->controls().end())\n> > - ctrls.set(V4L2_CID_HFLIP, 0);\n> > - if (subdev_->controls().find(V4L2_CID_VFLIP) !=\n> subdev_->controls().end())\n> > - ctrls.set(V4L2_CID_VFLIP, 0);\n> > - subdev_->setControls(&ctrls);\n> > + std::vector flipCtrlIds;\n> > + bool hasHflip = subdev_->controls().find(V4L2_CID_HFLIP) !=\n> subdev_->controls().end();\n> > + bool hasVflip = subdev_->controls().find(V4L2_CID_VFLIP) !=\n> subdev_->controls().end();\n> > + if (hasHflip)\n> > + flipCtrlIds.push_back(V4L2_CID_HFLIP);\n> > + if (hasVflip)\n> > + flipCtrlIds.push_back(V4L2_CID_VFLIP);\n> > + ControlList flipCtrls = subdev_->getControls(flipCtrlIds);\n> > +\n> > + /* Now construct a transform that would undo any flips. */\n> > + Transform transform = Transform::Identity;\n> > + if (hasHflip && flipCtrls.get(V4L2_CID_HFLIP).get()) {\n> > + const struct v4l2_query_ext_ctrl *extCtrl =\n> subdev_->controlInfo(V4L2_CID_HFLIP);\n> > + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)\n> > + transform |= Transform::HFlip;\n> > + }\n> > + if (hasVflip && flipCtrls.get(V4L2_CID_VFLIP).get()) {\n> > + const struct v4l2_query_ext_ctrl *extCtrl =\n> subdev_->controlInfo(V4L2_CID_VFLIP);\n> > + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)\n> > + transform |= Transform::VFlip;\n> > + }\n> > +\n> > + /* Finally get the formats, and apply the transform to the mbus\n> codes. */\n> > + auto formats = subdev_->formats(pad_);\n> > + for (const auto &format : formats) {\n> > + unsigned int mbusCode = format.first;\n> > + BayerFormat bayerFormat =\n> BayerFormat::fromMbusCode(mbusCode);\n> > + bool valid = true;\n> > +\n> > + if (bayerFormat.isValid())\n> > + mbusCode =\n> bayerFormat.transform(transform).toMbusCode(valid);\n> > +\n> > + if (valid)\n> > + formats_[mbusCode] = std::move(format.second);\n> > + }\n> >\n> > /* Enumerate, sort and cache media bus codes and sizes. */\n> > - formats_ = subdev_->formats(pad_);\n> > if (formats_.empty()) {\n> > LOG(CameraSensor, Error) << \"No image format found\";\n> > return -EINVAL;\n> > --\n> > 2.30.2\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 05508BD16B\n\tfor ;\n\tThu, 3 Nov 2022 11:25:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A210863036;\n\tThu, 3 Nov 2022 12:25:24 +0100 (CET)","from mail-qk1-x732.google.com (mail-qk1-x732.google.com\n\t[IPv6:2607:f8b0:4864:20::732])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 39B7C61F42\n\tfor ;\n\tThu, 3 Nov 2022 12:25:23 +0100 (CET)","by mail-qk1-x732.google.com with SMTP id l9so844883qkk.11\n\tfor ;\n\tThu, 03 Nov 2022 04:25:23 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667474724;\n\tbh=PZlNWZAf6i97NxVkfh7+UYhXROxBXGDqrivlblL8aJw=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=E1lGYPe2Q9Ilo3jKf1m5wXuzrwXgeiYRYVw7cxNGQVwR3Xyp+AALwqzg2wZ5tBvgT\n\tvy8lmR/CqCfBD1x9Y7YF0uwfOqN6qJoeq/jqTwOsGLCJXHbCrjwRQY5K+x0iXxcvCW\n\tsK1q1W2e32x5yy6ONTm/AVDOTZcrgtAPzBhpCX/s67X8vKYMUB6eFtafw+WePscikZ\n\tLdp1l35M4EIceM5UhLBp5sCgpU6I9WSp/PIAdoj4lIteSk03SKC/B3/H01UviiPnmF\n\tAp8w04fkKBsoA0jXBT52DQRWaFcsLjWtaJ6404lGPJuOCdIbi/wSQD7an/+r0nmTYn\n\tEML43b+w8/1lQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=N4sSRuzPtk7nI3j2YIW2/XoQpwJhJ56lMOet3yWx8d8=;\n\tb=NA0tmdpTUUWpKtw9044th/JTQp/ogjXuUqLOgleByjpwZKutBE8gN43lxcuJJpxwXK\n\tQIE89I/2k/qW70H+hpaaHeQCEszXpAbb2Nv6EF8/oFTjhiaRwNMemk2r59TBRSrweWuP\n\twwy85Y41t8t0RUztFHbyVmu9z0fgV+Pa3CkwJQxwzCyHDyyL+tv6sRopKHA706dYiIeC\n\tw4bS6FjdURMgPNGaIye3rGuaxoF84RhEWCtygpvF+d3RVfesoXX1l7s2KUnLgpZuaPml\n\tPR120ZEVnsPjk0FOkdfFQB6MIAC9UBDG95A4M0Xxg9t8GG1gWvBFkfYAoMTGR9rc7RPb\n\tfVVQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"NA0tmdpT\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=N4sSRuzPtk7nI3j2YIW2/XoQpwJhJ56lMOet3yWx8d8=;\n\tb=nGfiGq48g1cUrLyUwMpBT2Jfrx9VYoG9wYDER2cRk9NmnWP97x9IpqfgXjpQJgEb6W\n\tJxUnw5MzoN/r5C/QHoqTRfqyVZh02BrLsmsdBPfLKNT3dx+75x2Va9V6xAm8dWSWNRjd\n\tGHtKfajoN/Sd7tgdPOCO0bWYuXYvlfyjHEQhP++BG3imezQjA4Samn4CSA89Suc33/Db\n\tlav5FeNya3xSuTjQ1LcjRazw7lrx0vrWZnq5R/FzV4hAfe0J5efrWhTVVgarA47f0JM9\n\tIE34+6R9lT1IWr6Mf8o8t6GeKppsYi7JzxhpX51p5aP/VlfaBi5CRhH+5X6ZCyye6+2x\n\tCdEg==","X-Gm-Message-State":"ACrzQf3f5uC3QcjH2wEE5YmNQiqNguRDw1/ZYMslU8ag3CHIzpBQNWe0\n\tIZG6XqfSWXVYVbShr2nJmnltjxKG5/5k2YftJF2MGg==","X-Google-Smtp-Source":"AMsMyM52IyojIquWEoDiQQKMrG19GIutMIcS0X5SMRy7Bef06P/7F1pFPNsk2VV5xj0rYg8FbeJlFnsafgmUms7hO+g=","X-Received":"by 2002:a37:f502:0:b0:6f8:c3b2:51b9 with SMTP id\n\tl2-20020a37f502000000b006f8c3b251b9mr21291115qkk.616.1667474722006;\n\tThu, 03 Nov 2022 04:25:22 -0700 (PDT)","MIME-Version":"1.0","References":"<20221103104027.4197-1-david.plowman@raspberrypi.com>\n\t<20221103104027.4197-3-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Thu, 3 Nov 2022 11:25:06 +0000","Message-ID":"","To":"Dave Stevenson ","Content-Type":"multipart/alternative; boundary=\"0000000000007a393f05ec8f364d\"","Subject":"Re: [libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do\n\tnot clear camera flips when listing formats","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25730,"web_url":"https://patchwork.libcamera.org/comment/25730/","msgid":"","date":"2022-11-03T11:52:23","subject":"Re: [libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do\n\tnot clear camera flips when listing formats","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"On Thu, 3 Nov 2022 at 11:24, David Plowman\n wrote:\n>\n> Hi Dave\n>\n> On Thu, 3 Nov 2022 at 11:02, Dave Stevenson\n> wrote:\n> >\n> > Hi David\n> >\n> > On Thu, 3 Nov 2022 at 10:40, David Plowman via libcamera-devel\n> > wrote:\n> > >\n> > > Previously the code used to clear the camnera's h and v flip bits when\n> > > enumerating the supported formats so as to obtain any Bayer formats in\n> > > the sensor's native (untransformed) orientation. However this fails\n> > > when the camera is already in use elsewhere.\n> > >\n> > > Instead, we query the current state of the flip bits and transform the\n> > > formats - which we obtain in their flipped orientation - back into\n> > > their native orientation to be stored.\n> >\n> > I don't believe that libcamera knows for definite whether the sensor\n> > changes the Bayer order or not. Several of the OnSemi sensors I have\n> > seen do not change, presumably as they shift their crop by 1 pixel.\n>\n> Yes, that's a good point. I've vaguely assumed that I can check the\n> control's V4L2_CTRL_FLAG_MODIFY_LAYOUT to discover whether this is the\n> case or not, but as we keep on discovering, there may be no guarantees\n> about this. Do we know what these particular sensors do here? The Pi\n> PH actually already assumes this flag \"works\", so the changes here\n> possibly don't make anything worse, though they do engrain the\n> assumption more deeply...\n\nYes I believe V4L2_CTRL_FLAG_MODIFY_LAYOUT should do what you want.\n\nThe OnSemi sensors I'm thinking of don't have drivers in mainline, so\nthere's no driver to check currently.\n\nLooking at mainline, only imx219 and imx258 set the flag.\n- ov2680, imx208, imx319, imx355, mt9m001, ov08d10 all change the\norder but don't set V4L2_CTRL_FLAG_MODIFY_LAYOUT\n- hi847 and ov13b10 move the crop to maintain the Bayer order.\n- imx274 and ov5648 only ever report one order, but supports flips and\nhas no apparent cropping shift.\n- I have missed it in my imx290 patches to add flips that I was about to send!\n\nSo, as Jacopo summarised the situation on event handlers for controls,\nroom for improvement!\n\n Dave\n\n> Presumably you can rely on the formats being returned correctly after\n> setting the flip bits, though having to re-query the formats from the\n> device like that would be irritating too. And you'd never be able to\n> predict the format ahead of time, only after setting it. I don't know\n> if that's better or worse than saying \"change the driver\". Opinions\n> welcome on that one!\n>\n> To be honest I always have difficulty with the Bayer order being part\n> of the format. An application might legitimately want to have a say\n> about the bit depth or packing, but the Bayer order?? Being able to\n> request an XXXX (\"anything goes\") Bayer order, which gets updated once\n> the sensor is configured, would be much more application-friendly. But\n> another can of worms!!\n>\n> David\n>\n> >\n> > The original comment mentioned that scenario:\n> > \"This is harmless for sensors where the flips don't affect the Bayer order\"\n> >\n> > Dave\n> >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++------\n> > > 1 file changed, 43 insertions(+), 8 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index 572a313a..6670dfb9 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -19,6 +19,8 @@\n> > >\n> > > #include \n> > >\n> > > +#include \n> > > +\n> > > #include \"libcamera/internal/bayer_format.h\"\n> > > #include \"libcamera/internal/camera_lens.h\"\n> > > #include \"libcamera/internal/camera_sensor_properties.h\"\n> > > @@ -108,18 +110,51 @@ int CameraSensor::init()\n> > > return ret;\n> > >\n> > > /*\n> > > - * Clear any flips to be sure we get the \"native\" Bayer order. This is\n> > > - * harmless for sensors where the flips don't affect the Bayer order.\n> > > + * We want to get the native mbus codes for the sensor, without any flips.\n> > > + * We can't clear any flips here, so we have to read the current values\n> > > + * (if the flip controls exist), decide whether they actually modify any\n> > > + * output Bayer pattern, and finally undo their effect on the formats.\n> > > + *\n> > > + * First, check if the flip controls exist and if so read them.\n> > > */\n> > > ControlList ctrls(subdev_->controls());\n> > > - if (subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end())\n> > > - ctrls.set(V4L2_CID_HFLIP, 0);\n> > > - if (subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end())\n> > > - ctrls.set(V4L2_CID_VFLIP, 0);\n> > > - subdev_->setControls(&ctrls);\n> > > + std::vector flipCtrlIds;\n> > > + bool hasHflip = subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end();\n> > > + bool hasVflip = subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end();\n> > > + if (hasHflip)\n> > > + flipCtrlIds.push_back(V4L2_CID_HFLIP);\n> > > + if (hasVflip)\n> > > + flipCtrlIds.push_back(V4L2_CID_VFLIP);\n> > > + ControlList flipCtrls = subdev_->getControls(flipCtrlIds);\n> > > +\n> > > + /* Now construct a transform that would undo any flips. */\n> > > + Transform transform = Transform::Identity;\n> > > + if (hasHflip && flipCtrls.get(V4L2_CID_HFLIP).get()) {\n> > > + const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_HFLIP);\n> > > + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)\n> > > + transform |= Transform::HFlip;\n> > > + }\n> > > + if (hasVflip && flipCtrls.get(V4L2_CID_VFLIP).get()) {\n> > > + const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_VFLIP);\n> > > + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)\n> > > + transform |= Transform::VFlip;\n> > > + }\n> > > +\n> > > + /* Finally get the formats, and apply the transform to the mbus codes. */\n> > > + auto formats = subdev_->formats(pad_);\n> > > + for (const auto &format : formats) {\n> > > + unsigned int mbusCode = format.first;\n> > > + BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> > > + bool valid = true;\n> > > +\n> > > + if (bayerFormat.isValid())\n> > > + mbusCode = bayerFormat.transform(transform).toMbusCode(valid);\n> > > +\n> > > + if (valid)\n> > > + formats_[mbusCode] = std::move(format.second);\n> > > + }\n> > >\n> > > /* Enumerate, sort and cache media bus codes and sizes. */\n> > > - formats_ = subdev_->formats(pad_);\n> > > if (formats_.empty()) {\n> > > LOG(CameraSensor, Error) << \"No image format found\";\n> > > return -EINVAL;\n> > > --\n> > > 2.30.2\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 4F4A1BDB16\n\tfor ;\n\tThu, 3 Nov 2022 11:52:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B62F46307D;\n\tThu, 3 Nov 2022 12:52:41 +0100 (CET)","from mail-ej1-x62c.google.com (mail-ej1-x62c.google.com\n\t[IPv6:2a00:1450:4864:20::62c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1BAE61F42\n\tfor ;\n\tThu, 3 Nov 2022 12:52:39 +0100 (CET)","by mail-ej1-x62c.google.com with SMTP id f5so4515084ejc.5\n\tfor ;\n\tThu, 03 Nov 2022 04:52:39 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667476361;\n\tbh=wgpZS4eq1cOib8r2pc9UZ39DIWG1zJUBoqE9q1T5/Po=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=omZcetaqnel/8hBb//mV8uP7dlkBsPxeqrtyhnSkP6QIstRFrtQN10BUMz7yFj17v\n\tAEoemYKyLRHhftFkq6zssFpow15rQ4YQcGO1iOtQolTkhj3ctgoP7F8U9A0u0Q2vQy\n\tOafEKbCODxTJGRSZWQf1DJOQszVIlCNSN86l5ZYgHV6otDMI2Q9g9ks1+sBDfolOpm\n\tMgVHVCi+0OG+hOLVXEsf9MKVPnjvw39mI27Wx+Oo0L1yYWdpUFeW+hXQlZj8TxUbaE\n\toD7XDzE6krnFgOjpE8a6gNeSQvCJ+kl3iWpPvNbeH4oWtQPotRDVpCsvwTU5ug1mUz\n\ti9nOx+8uWShSQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=aUo6LFFbMUFkZUUN8DOcWGxkLQe+G3WlkriZkKWDFto=;\n\tb=Zqq3yaCsvmLKE/vRHJVwJIDZj9FQU5VzYzflUKQKXxP5RWm8+IEGNscdvTbdzrY61j\n\tFY1g/dp/6Zaj/lcdrjDiIzfawqSBqSQfpBE36v25hbTO7gIfDsK3jAltiQrXshpI4e9L\n\tzusRfjdCAz/E02aNYCm42boFInz5dYo0fxvEaCa3Hv2B6ojCCpEv0gjft9jfkm/kG3+/\n\twZQFksEeijwZfW6eqavlL2bZq8TPcPXbLm+POn0zookv9vh+NdG2lRZTpmwGDREHfHxr\n\tOKBpkEGadc2yk6mjDZnFwDEs2ngeiMM0ur1XJiXg0GL0gWarsC719lfFdqmWNdwUhLTr\n\tlBDQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Zqq3yaCs\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=aUo6LFFbMUFkZUUN8DOcWGxkLQe+G3WlkriZkKWDFto=;\n\tb=uSy4NRZvd1BiXqnL4vyIIR7zLfUs8OESV8/LUwNsWMRWRXwZq57LkDyVqzSY68sxIm\n\tTWSgTlPjnG4UGSHUIQxyTKVBM+MoolnwaASiHnJD2sHjtpamRc06Hix79IPJfYFsI5Rf\n\t6RP3/bKBU+tEr/AVX6FeQFVKP7ve2oWVKbmqUMyypC+ehLAZNwvBiNWAqPPdFVBM9S0M\n\tZn3hS8cHaDlhVmf6GlGjzKbBYv6Vi6GzwK+8eb5Q0+YU9OwYO4giX0hQ4+lK0wZ9A1QZ\n\t8aL24GkpDTpWHKlPW+LXzemgjeCGzj8pHSvOUIygeM1w2Jmqb/EEwI+26DjF64G6LMg/\n\tGBNw==","X-Gm-Message-State":"ACrzQf0GQzoIfMsdDXkuwd65XkMtNb8r+CDVkY7Nsv2quDGpvGUSMOvr\n\toQL1YWIwIOtRREfq9DuQX2zHjbGW0rVNN5PtdAMmnA==","X-Google-Smtp-Source":"AMsMyM4vjAtXsWjdhO8Cikrhfqpr50B7CBKCkfeDJGcZzGzC13coQFBayncz4juZySWMEdtyY8BDdWQ7E6fFlh9yTCY=","X-Received":"by 2002:a17:907:94c7:b0:78e:1c4f:51f9 with SMTP id\n\tdn7-20020a17090794c700b0078e1c4f51f9mr29543575ejc.200.1667476359425;\n\tThu, 03 Nov 2022 04:52:39 -0700 (PDT)","MIME-Version":"1.0","References":"<20221103104027.4197-1-david.plowman@raspberrypi.com>\n\t<20221103104027.4197-3-david.plowman@raspberrypi.com>\n\t\n\t","In-Reply-To":"","Date":"Thu, 3 Nov 2022 11:52:23 +0000","Message-ID":"","To":"David Plowman ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do\n\tnot clear camera flips when listing formats","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","From":"Dave Stevenson via libcamera-devel\n\t","Reply-To":"Dave Stevenson ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25737,"web_url":"https://patchwork.libcamera.org/comment/25737/","msgid":"<20221103152655.av2wiqpyic2pjgea@uno.localdomain>","date":"2022-11-03T15:26:55","subject":"Re: [libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do\n\tnot clear camera flips when listing formats","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi again\n\nOn Thu, Nov 03, 2022 at 11:52:23AM +0000, Dave Stevenson via libcamera-devel wrote:\n> On Thu, 3 Nov 2022 at 11:24, David Plowman\n> wrote:\n> >\n> > Hi Dave\n> >\n> > On Thu, 3 Nov 2022 at 11:02, Dave Stevenson\n> > wrote:\n> > >\n> > > Hi David\n> > >\n> > > On Thu, 3 Nov 2022 at 10:40, David Plowman via libcamera-devel\n> > > wrote:\n> > > >\n> > > > Previously the code used to clear the camnera's h and v flip bits when\n> > > > enumerating the supported formats so as to obtain any Bayer formats in\n> > > > the sensor's native (untransformed) orientation. However this fails\n> > > > when the camera is already in use elsewhere.\n> > > >\n> > > > Instead, we query the current state of the flip bits and transform the\n> > > > formats - which we obtain in their flipped orientation - back into\n> > > > their native orientation to be stored.\n> > >\n> > > I don't believe that libcamera knows for definite whether the sensor\n> > > changes the Bayer order or not. Several of the OnSemi sensors I have\n> > > seen do not change, presumably as they shift their crop by 1 pixel.\n> >\n> > Yes, that's a good point. I've vaguely assumed that I can check the\n> > control's V4L2_CTRL_FLAG_MODIFY_LAYOUT to discover whether this is the\n> > case or not, but as we keep on discovering, there may be no guarantees\n> > about this. Do we know what these particular sensors do here? The Pi\n> > PH actually already assumes this flag \"works\", so the changes here\n> > possibly don't make anything worse, though they do engrain the\n> > assumption more deeply...\n>\n> Yes I believe V4L2_CTRL_FLAG_MODIFY_LAYOUT should do what you want.\n>\n> The OnSemi sensors I'm thinking of don't have drivers in mainline, so\n> there's no driver to check currently.\n>\n> Looking at mainline, only imx219 and imx258 set the flag.\n> - ov2680, imx208, imx319, imx355, mt9m001, ov08d10 all change the\n> order but don't set V4L2_CTRL_FLAG_MODIFY_LAYOUT\n> - hi847 and ov13b10 move the crop to maintain the Bayer order.\n> - imx274 and ov5648 only ever report one order, but supports flips and\n> has no apparent cropping shift.\n> - I have missed it in my imx290 patches to add flips that I was about to send!\n>\n> So, as Jacopo summarised the situation on event handlers for controls,\n> room for improvement!\n>\n\nTo add more pleasure, as discussed on a review comment on Dave's\nseries\nhttps://patchwork.linuxtv.org/project/linux-media/patch/20221005152809.3785786-12-dave.stevenson@raspberrypi.com/\nSome sensors assumes to be rotated and bury their rotation\nsettings in their register tables.\n\nIn the example of the imx258 sensor Dave has mentioned:\n\n- The bayer order reported through the format is GRBG\n https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c#L822\n\n- The sensor driver applies mirroring by default at s_stream\n time (without specifying the direction, vertical or horizontal)\n\n /* Set Orientation be 180 degree */\n ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,\n IMX258_REG_VALUE_08BIT, REG_CONFIG_MIRROR_FLIP);\n\n- I found an interesting comment on the patch series changelog:\n https://www.spinics.net/lists/linux-media/msg133840.html\n Output order of test pattern is always BGGR, so it needs a flip to\n rotate bayer pattern to required one (GRBG)\n\nSo it might seem legit to presume the native Bayer pattern on the\npixel array is BGGR, read from the right-bottom corner:\n\n\n RG RG RG RG RG\n GB GB GB GB GB\n (1, n) RG RG RG RG RG (1,0)\n GB GB GB GB GB\n (0, n) <-- (0,0)\n\nTo obtain the Bayer GRBG order advertised as output format I presume the\nREG_CONFIG_MIRROR_FLIP vertically flips the image, moving the (0,0)\npixel coordinate, from where reading is started, on the top-right corner:\n\n (0, n) <-- (0,0)\n RG RG RG RG RG\n (1, n) GB GB GB GB GB (1, 0)\n RG RG RG RG RG\n GB GB GB GB GB\n\nAccording to your below patch and Bayer::transform():\n- BGGR becomes GRBG via a vertical flip, right ? So this should\n confirm the above assumption about an unconditional vertical flip\n\nIf the driver would register flip controls, it would register them as\nVFLIP = 1, HFLIP = 0. Here below you would get GRBG, vertically\ntransform it to BGGR and store BGGR as \"native\" format, but now you\nwon't be able to apply it to the sensor, as it only supports GRBG.\n\nWhat drivers should do is to register somehow their -native- Bayer\npattern, use a generic BAYER format in set_fmt and let userspace deal\nwith rotations and Bayer pattern re-odering as userspace is\nable to access the mounting rotation via the CID_CAMERA_ROTATION\ncontrol.\n\nUnfortunately no driver afaict behaves this way. They might report via\nv4l2-ctl if any flip is applied to them, as Dave does in the series,\nbut adjusting the Bayer pattern as you do below might lead to results\nthat won't apply on the sensor. Do you agree or did I get lost ?\n\nTo be honest I would drop clearing the flips, and to\nprotect against concurrent usages, validate that the current flip values\nmatch the default controls value. If the two do not match, and the\ndriver advertises V4L2_CTRL_FLAG_MODIFY_LAYOUT, it means the mbus code\nwe're seeing right now is the result of another user having flipped\nthe image. In this case you could flip the format like you're doing\nbelow to obtain the \"unflipped\" Bayer pattern, as we know it will work\nas it is the \"defaul configuration\" order.\n\nFinal question, and sorry for the long email, but what would it be the\nreal purpose of obtaining the native bayer pattern order in your use case. I\npresume we had that attempt to get the native format because android\nwants to have the native order reported. I think that CameraSensor\nshould instead try to use a bayer order that work for the drive in its\ndefault configuration which, if the driver claims to be rotated by\ndefault, won't match the actual native order.\n\nWe have a camera properties database where such native information\ncould eventually be stored if it's of interest!\n\nHope I didn't get lost!\n\n> Dave\n>\n> > Presumably you can rely on the formats being returned correctly after\n> > setting the flip bits, though having to re-query the formats from the\n> > device like that would be irritating too. And you'd never be able to\n> > predict the format ahead of time, only after setting it. I don't know\n> > if that's better or worse than saying \"change the driver\". Opinions\n> > welcome on that one!\n> >\n> > To be honest I always have difficulty with the Bayer order being part\n> > of the format. An application might legitimately want to have a say\n> > about the bit depth or packing, but the Bayer order?? Being able to\n> > request an XXXX (\"anything goes\") Bayer order, which gets updated once\n> > the sensor is configured, would be much more application-friendly. But\n> > another can of worms!!\n\nAgreed, userspace has all the information to adjust the ordering as it\nlikes..\n\n> >\n> > David\n> >\n> > >\n> > > The original comment mentioned that scenario:\n> > > \"This is harmless for sensors where the flips don't affect the Bayer order\"\n> > >\n> > > Dave\n> > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++------\n> > > > 1 file changed, 43 insertions(+), 8 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > index 572a313a..6670dfb9 100644\n> > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > @@ -19,6 +19,8 @@\n> > > >\n> > > > #include \n> > > >\n> > > > +#include \n> > > > +\n> > > > #include \"libcamera/internal/bayer_format.h\"\n> > > > #include \"libcamera/internal/camera_lens.h\"\n> > > > #include \"libcamera/internal/camera_sensor_properties.h\"\n> > > > @@ -108,18 +110,51 @@ int CameraSensor::init()\n> > > > return ret;\n> > > >\n> > > > /*\n> > > > - * Clear any flips to be sure we get the \"native\" Bayer order. This is\n> > > > - * harmless for sensors where the flips don't affect the Bayer order.\n> > > > + * We want to get the native mbus codes for the sensor, without any flips.\n> > > > + * We can't clear any flips here, so we have to read the current values\n> > > > + * (if the flip controls exist), decide whether they actually modify any\n> > > > + * output Bayer pattern, and finally undo their effect on the formats.\n> > > > + *\n> > > > + * First, check if the flip controls exist and if so read them.\n> > > > */\n> > > > ControlList ctrls(subdev_->controls());\n> > > > - if (subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end())\n> > > > - ctrls.set(V4L2_CID_HFLIP, 0);\n> > > > - if (subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end())\n> > > > - ctrls.set(V4L2_CID_VFLIP, 0);\n> > > > - subdev_->setControls(&ctrls);\n> > > > + std::vector flipCtrlIds;\n> > > > + bool hasHflip = subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end();\n> > > > + bool hasVflip = subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end();\n> > > > + if (hasHflip)\n> > > > + flipCtrlIds.push_back(V4L2_CID_HFLIP);\n> > > > + if (hasVflip)\n> > > > + flipCtrlIds.push_back(V4L2_CID_VFLIP);\n> > > > + ControlList flipCtrls = subdev_->getControls(flipCtrlIds);\n> > > > +\n> > > > + /* Now construct a transform that would undo any flips. */\n> > > > + Transform transform = Transform::Identity;\n> > > > + if (hasHflip && flipCtrls.get(V4L2_CID_HFLIP).get()) {\n> > > > + const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_HFLIP);\n> > > > + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)\n> > > > + transform |= Transform::HFlip;\n> > > > + }\n> > > > + if (hasVflip && flipCtrls.get(V4L2_CID_VFLIP).get()) {\n> > > > + const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_VFLIP);\n> > > > + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)\n> > > > + transform |= Transform::VFlip;\n> > > > + }\n> > > > +\n> > > > + /* Finally get the formats, and apply the transform to the mbus codes. */\n> > > > + auto formats = subdev_->formats(pad_);\n> > > > + for (const auto &format : formats) {\n> > > > + unsigned int mbusCode = format.first;\n> > > > + BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> > > > + bool valid = true;\n> > > > +\n> > > > + if (bayerFormat.isValid())\n> > > > + mbusCode = bayerFormat.transform(transform).toMbusCode(valid);\n> > > > +\n> > > > + if (valid)\n> > > > + formats_[mbusCode] = std::move(format.second);\n> > > > + }\n> > > >\n> > > > /* Enumerate, sort and cache media bus codes and sizes. */\n> > > > - formats_ = subdev_->formats(pad_);\n> > > > if (formats_.empty()) {\n> > > > LOG(CameraSensor, Error) << \"No image format found\";\n> > > > return -EINVAL;\n> > > > --\n> > > > 2.30.2\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 19DC2BDB16\n\tfor ;\n\tThu, 3 Nov 2022 15:27:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A1566307D;\n\tThu, 3 Nov 2022 16:27:00 +0100 (CET)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BD77D61F42\n\tfor ;\n\tThu, 3 Nov 2022 16:26:58 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id E473DE0005;\n\tThu, 3 Nov 2022 15:26:57 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667489220;\n\tbh=dS+g5Ziv8XELtQHeBKLuaXDj1ODkFZ8lVT3nwsZjPbU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=O3m377qjqg+tZKYtkA8278ESdxtvXS0IInY0iJ4OS6aFrQ2EXjAk/U/PR3KYNZ67R\n\to7zwt9psewpestitDDVhSxAlBW8rxN0RKsikK9XizjL5KqheBgofPCHTIKHJKRkrQU\n\tnQ5a/9JyxFgDygIAskz7HQYHMWiElU6CihYpwwu70PNnxJXS94TMr2SN8+f5xRbEkM\n\t56cC5yFCNO2AtPenclY4DrjRj9v252dQeuns40zxRoRPZZxNRWk5Tm3Pw39IFvTtHW\n\tJRECNWm1IUizg5o7HRd+caw5PLQnkEaP+VhhxF6iynntTZszG233HM4ZnhBy9iroO1\n\tXU/i/FwAftIEQ==","Date":"Thu, 3 Nov 2022 16:26:55 +0100","To":"Dave Stevenson ","Message-ID":"<20221103152655.av2wiqpyic2pjgea@uno.localdomain>","References":"<20221103104027.4197-1-david.plowman@raspberrypi.com>\n\t<20221103104027.4197-3-david.plowman@raspberrypi.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do\n\tnot clear camera flips when listing formats","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25738,"web_url":"https://patchwork.libcamera.org/comment/25738/","msgid":"","date":"2022-11-03T16:26:07","subject":"Re: [libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do\n\tnot clear camera flips when listing formats","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi Jacopo\n\nOn Thu, 3 Nov 2022 at 15:26, Jacopo Mondi wrote:\n>\n> Hi again\n>\n> On Thu, Nov 03, 2022 at 11:52:23AM +0000, Dave Stevenson via libcamera-devel wrote:\n> > On Thu, 3 Nov 2022 at 11:24, David Plowman\n> > wrote:\n> > >\n> > > Hi Dave\n> > >\n> > > On Thu, 3 Nov 2022 at 11:02, Dave Stevenson\n> > > wrote:\n> > > >\n> > > > Hi David\n> > > >\n> > > > On Thu, 3 Nov 2022 at 10:40, David Plowman via libcamera-devel\n> > > > wrote:\n> > > > >\n> > > > > Previously the code used to clear the camnera's h and v flip bits when\n> > > > > enumerating the supported formats so as to obtain any Bayer formats in\n> > > > > the sensor's native (untransformed) orientation. However this fails\n> > > > > when the camera is already in use elsewhere.\n> > > > >\n> > > > > Instead, we query the current state of the flip bits and transform the\n> > > > > formats - which we obtain in their flipped orientation - back into\n> > > > > their native orientation to be stored.\n> > > >\n> > > > I don't believe that libcamera knows for definite whether the sensor\n> > > > changes the Bayer order or not. Several of the OnSemi sensors I have\n> > > > seen do not change, presumably as they shift their crop by 1 pixel.\n> > >\n> > > Yes, that's a good point. I've vaguely assumed that I can check the\n> > > control's V4L2_CTRL_FLAG_MODIFY_LAYOUT to discover whether this is the\n> > > case or not, but as we keep on discovering, there may be no guarantees\n> > > about this. Do we know what these particular sensors do here? The Pi\n> > > PH actually already assumes this flag \"works\", so the changes here\n> > > possibly don't make anything worse, though they do engrain the\n> > > assumption more deeply...\n> >\n> > Yes I believe V4L2_CTRL_FLAG_MODIFY_LAYOUT should do what you want.\n> >\n> > The OnSemi sensors I'm thinking of don't have drivers in mainline, so\n> > there's no driver to check currently.\n> >\n> > Looking at mainline, only imx219 and imx258 set the flag.\n> > - ov2680, imx208, imx319, imx355, mt9m001, ov08d10 all change the\n> > order but don't set V4L2_CTRL_FLAG_MODIFY_LAYOUT\n> > - hi847 and ov13b10 move the crop to maintain the Bayer order.\n> > - imx274 and ov5648 only ever report one order, but supports flips and\n> > has no apparent cropping shift.\n> > - I have missed it in my imx290 patches to add flips that I was about to send!\n> >\n> > So, as Jacopo summarised the situation on event handlers for controls,\n> > room for improvement!\n> >\n>\n> To add more pleasure, as discussed on a review comment on Dave's\n> series\n> https://patchwork.linuxtv.org/project/linux-media/patch/20221005152809.3785786-12-dave.stevenson@raspberrypi.com/\n> Some sensors assumes to be rotated and bury their rotation\n> settings in their register tables.\n>\n> In the example of the imx258 sensor Dave has mentioned:\n>\n> - The bayer order reported through the format is GRBG\n> https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c#L822\n>\n> - The sensor driver applies mirroring by default at s_stream\n> time (without specifying the direction, vertical or horizontal)\n>\n> /* Set Orientation be 180 degree */\n> ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,\n> IMX258_REG_VALUE_08BIT, REG_CONFIG_MIRROR_FLIP);\n\n#define REG_CONFIG_MIRROR_FLIP 0x03 [1]\nso this is doing BOTH H & V flips in one hit.\n\nDatasheet for imx258 at [2].\n\n[1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c#L78\n[2] https://web.archive.org/web/20201027131326/www.hi.app/IMX258-datasheet.pdf\n\n> - I found an interesting comment on the patch series changelog:\n> https://www.spinics.net/lists/linux-media/msg133840.html\n> Output order of test pattern is always BGGR, so it needs a flip to\n> rotate bayer pattern to required one (GRBG)\n\nIt's not uncommon that the test patterns are independent of flips as\nthere is no image array to read out in the reverse direction.\nThere are often 4 registers to configure the values to stick into each\nof the 4 colour channels, and those are simply inserted in order.\nRegisters 0x0602-0x0609 on IMX258.\n\nTBH I've never understood the love of implementing test patterns in\ndrivers. If a driver is merged into mainline then there should be a\nfair confidence that it works, therefore why worry about test\npatterns? Isn't a captured image more valuable?\n\n> So it might seem legit to presume the native Bayer pattern on the\n> pixel array is BGGR, read from the right-bottom corner:\n\nI'm not sure if you're referencing imx258 specifically here, or\ntalking generically.\nimx258 is a total mess, and I need to get around to upstreaming my\npatches at [3].\n\nThe native Bayer order for imx258 is RGGB (see Fig 5-4 page 84, Fig\n5-3 page 79, and several places in section 4). Surely the datasheet\nhas to count as definitive.\n\nAs in patch [4], Y_ADD_STA register is set to 0, and Y_ADD_END to\n3118, giving 3119 lines total for 3118 lines of readout. The hardcoded\nV flip on that starts us on the \"wrong\" line of the Bayer pattern for\nany sane person looking at it and trying to work out what the heck is\ngoing on - we're still getting RGGB. H flip that and the resulting\nBayer order is GRBG. QED.\n\n[3] https://github.com/raspberrypi/linux/commits/rpi-5.15.y/drivers/media/i2c/imx258.c\n[4] https://github.com/raspberrypi/linux/commit/1942d0da85bac79f3d2a1c2d8e797e97bd16b618\n\n>\n> RG RG RG RG RG\n> GB GB GB GB GB\n> (1, n) RG RG RG RG RG (1,0)\n> GB GB GB GB GB\n> (0, n) <-- (0,0)\n>\n> To obtain the Bayer GRBG order advertised as output format I presume the\n> REG_CONFIG_MIRROR_FLIP vertically flips the image, moving the (0,0)\n> pixel coordinate, from where reading is started, on the top-right corner:\n>\n> (0, n) <-- (0,0)\n> RG RG RG RG RG\n> (1, n) GB GB GB GB GB (1, 0)\n> RG RG RG RG RG\n> GB GB GB GB GB\n\nDue to the error on Y_ADD_END, implementing a VFLIP gave no change in\nBayer order! Work that one out in userspace (although it is a driver\nbug that causes it).\n\n> According to your below patch and Bayer::transform():\n> - BGGR becomes GRBG via a vertical flip, right ? So this should\n> confirm the above assumption about an unconditional vertical flip\n>\n> If the driver would register flip controls, it would register them as\n> VFLIP = 1, HFLIP = 0. Here below you would get GRBG, vertically\n> transform it to BGGR and store BGGR as \"native\" format, but now you\n> won't be able to apply it to the sensor, as it only supports GRBG.\n>\n> What drivers should do is to register somehow their -native- Bayer\n> pattern, use a generic BAYER format in set_fmt and let userspace deal\n> with rotations and Bayer pattern re-odering as userspace is\n> able to access the mounting rotation via the CID_CAMERA_ROTATION\n> control.\n\nDoing so goes against the V4L2 principle of the V4L2_PIX_FMT_* telling\nyou exactly how to interpret the pixel data.\n\nI need to check the wording, but I thought CID_CAMERA_ROTATION told\nyou the orientation of the sensor within the device such that you\ncould stick it into EXIF or similar metadata. It doesn't tell you how\nthe Bayer order would change.\n\n> Unfortunately no driver afaict behaves this way. They might report via\n> v4l2-ctl if any flip is applied to them, as Dave does in the series,\n> but adjusting the Bayer pattern as you do below might lead to results\n> that won't apply on the sensor. Do you agree or did I get lost ?\n\nAIUI At the point of actually wanting to stream, flips will be\napplied, and the format reported by the sensor driver will then be\nused to configure the rest of the pipeline.\nI hope that userspace never thinks it knows better than the driver\nwith regard to Bayer order - as above there are enough quirks that it\nwill get it wrong.\n\n> To be honest I would drop clearing the flips, and to\n> protect against concurrent usages, validate that the current flip values\n> match the default controls value. If the two do not match, and the\n> driver advertises V4L2_CTRL_FLAG_MODIFY_LAYOUT, it means the mbus code\n> we're seeing right now is the result of another user having flipped\n> the image. In this case you could flip the format like you're doing\n> below to obtain the \"unflipped\" Bayer pattern, as we know it will work\n> as it is the \"defaul configuration\" order.\n\nA colour equivalent to ov9282 is going to blow up your assumption on\ndefault value. The original driver didn't support flips, but H & V\nflipped the image (much like imx258), so to avoid regressions we have\na default of being flipped.\nSame would have been true for imx258, except that there the original\ndriver read the \"rotation\" fwnode parameter and ensured it was 180. I\nthink that is now going to be incorrectly interpreted as the sensor\nhas done the rot180, and it will tell userspace rot180 as well, so\nEXIF headers etc will say that it is stored inverted.\n\n> Final question, and sorry for the long email, but what would it be the\n> real purpose of obtaining the native bayer pattern order in your use case. I\n> presume we had that attempt to get the native format because android\n> wants to have the native order reported. I think that CameraSensor\n> should instead try to use a bayer order that work for the drive in its\n> default configuration which, if the driver claims to be rotated by\n> default, won't match the actual native order.\n>\n> We have a camera properties database where such native information\n> could eventually be stored if it's of interest!\n\nStore it in two places - increase the chance of it being wrong! ;)\n\nI am starting to write a guide for things to check with sensor\ndrivers, so hopefully future drivers will do what is defined as the\ncorrect thing. Perhaps we'll find the time to fix older drivers - I'm\njust doing a patch set for V4L2_CTRL_FLAG_MODIFY_LAYOUT.\n\n Dave\n\n> Hope I didn't get lost!\n>\n> > Dave\n> >\n> > > Presumably you can rely on the formats being returned correctly after\n> > > setting the flip bits, though having to re-query the formats from the\n> > > device like that would be irritating too. And you'd never be able to\n> > > predict the format ahead of time, only after setting it. I don't know\n> > > if that's better or worse than saying \"change the driver\". Opinions\n> > > welcome on that one!\n> > >\n> > > To be honest I always have difficulty with the Bayer order being part\n> > > of the format. An application might legitimately want to have a say\n> > > about the bit depth or packing, but the Bayer order?? Being able to\n> > > request an XXXX (\"anything goes\") Bayer order, which gets updated once\n> > > the sensor is configured, would be much more application-friendly. But\n> > > another can of worms!!\n>\n> Agreed, userspace has all the information to adjust the ordering as it\n> likes..\n>\n> > >\n> > > David\n> > >\n> > > >\n> > > > The original comment mentioned that scenario:\n> > > > \"This is harmless for sensors where the flips don't affect the Bayer order\"\n> > > >\n> > > > Dave\n> > > >\n> > > > > Signed-off-by: David Plowman \n> > > > > ---\n> > > > > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++------\n> > > > > 1 file changed, 43 insertions(+), 8 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > > index 572a313a..6670dfb9 100644\n> > > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > > @@ -19,6 +19,8 @@\n> > > > >\n> > > > > #include \n> > > > >\n> > > > > +#include \n> > > > > +\n> > > > > #include \"libcamera/internal/bayer_format.h\"\n> > > > > #include \"libcamera/internal/camera_lens.h\"\n> > > > > #include \"libcamera/internal/camera_sensor_properties.h\"\n> > > > > @@ -108,18 +110,51 @@ int CameraSensor::init()\n> > > > > return ret;\n> > > > >\n> > > > > /*\n> > > > > - * Clear any flips to be sure we get the \"native\" Bayer order. This is\n> > > > > - * harmless for sensors where the flips don't affect the Bayer order.\n> > > > > + * We want to get the native mbus codes for the sensor, without any flips.\n> > > > > + * We can't clear any flips here, so we have to read the current values\n> > > > > + * (if the flip controls exist), decide whether they actually modify any\n> > > > > + * output Bayer pattern, and finally undo their effect on the formats.\n> > > > > + *\n> > > > > + * First, check if the flip controls exist and if so read them.\n> > > > > */\n> > > > > ControlList ctrls(subdev_->controls());\n> > > > > - if (subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end())\n> > > > > - ctrls.set(V4L2_CID_HFLIP, 0);\n> > > > > - if (subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end())\n> > > > > - ctrls.set(V4L2_CID_VFLIP, 0);\n> > > > > - subdev_->setControls(&ctrls);\n> > > > > + std::vector flipCtrlIds;\n> > > > > + bool hasHflip = subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end();\n> > > > > + bool hasVflip = subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end();\n> > > > > + if (hasHflip)\n> > > > > + flipCtrlIds.push_back(V4L2_CID_HFLIP);\n> > > > > + if (hasVflip)\n> > > > > + flipCtrlIds.push_back(V4L2_CID_VFLIP);\n> > > > > + ControlList flipCtrls = subdev_->getControls(flipCtrlIds);\n> > > > > +\n> > > > > + /* Now construct a transform that would undo any flips. */\n> > > > > + Transform transform = Transform::Identity;\n> > > > > + if (hasHflip && flipCtrls.get(V4L2_CID_HFLIP).get()) {\n> > > > > + const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_HFLIP);\n> > > > > + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)\n> > > > > + transform |= Transform::HFlip;\n> > > > > + }\n> > > > > + if (hasVflip && flipCtrls.get(V4L2_CID_VFLIP).get()) {\n> > > > > + const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_VFLIP);\n> > > > > + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)\n> > > > > + transform |= Transform::VFlip;\n> > > > > + }\n> > > > > +\n> > > > > + /* Finally get the formats, and apply the transform to the mbus codes. */\n> > > > > + auto formats = subdev_->formats(pad_);\n> > > > > + for (const auto &format : formats) {\n> > > > > + unsigned int mbusCode = format.first;\n> > > > > + BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> > > > > + bool valid = true;\n> > > > > +\n> > > > > + if (bayerFormat.isValid())\n> > > > > + mbusCode = bayerFormat.transform(transform).toMbusCode(valid);\n> > > > > +\n> > > > > + if (valid)\n> > > > > + formats_[mbusCode] = std::move(format.second);\n> > > > > + }\n> > > > >\n> > > > > /* Enumerate, sort and cache media bus codes and sizes. */\n> > > > > - formats_ = subdev_->formats(pad_);\n> > > > > if (formats_.empty()) {\n> > > > > LOG(CameraSensor, Error) << \"No image format found\";\n> > > > > return -EINVAL;\n> > > > > --\n> > > > > 2.30.2\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 D8CA6BD16B\n\tfor ;\n\tThu, 3 Nov 2022 16:26:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 182916307D;\n\tThu, 3 Nov 2022 17:26:27 +0100 (CET)","from mail-ed1-x530.google.com (mail-ed1-x530.google.com\n\t[IPv6:2a00:1450:4864:20::530])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D4FFF61F42\n\tfor ;\n\tThu, 3 Nov 2022 17:26:25 +0100 (CET)","by mail-ed1-x530.google.com with SMTP id a5so3818992edb.11\n\tfor ;\n\tThu, 03 Nov 2022 09:26:25 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667492787;\n\tbh=OGbRu6YnY+hr4GLNAOs9SkAjUakNJfplAmD93SL7oq8=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=PT0VbNlVQUlMJD12DA7TS4DlchKiox43QoyjGoONvVOuNjF+mFdrq5WBqldolUn23\n\tj7lGUnhjhoAN+qn/dkDDSFzdmUYB3xGwFJT/Tywoz+MyGVpLAXjXkJ9str+g/GO2in\n\ttIwwbr6R/vdx1YzFJ5Kw+ngY8sZy3gu5xay2VTGDkImnQ2OzrMYt2x81SZAjXwW1eD\n\tJ9xgnQ8rrOTJCj8EsmKPAUnQ8cBHMSzC+C8ZBwoBVqs32z6NpIQnR1jNG+ZUNixdTI\n\t6A7BuxAmr5EiOnaxBGEeL5zRNjyTaQHSnFHIJPe/O1oPG2ThROUXpeMYYeMsWlr49f\n\t/fN8IV0xVbMsw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=jxjXqmUbUswsZodyQaeRvvXK4xaUUmSAn7PGKKrWp0c=;\n\tb=VGHmUupfnhsgcvSxXRq2s9gn1N28saQJ06x74xyoc8gGJR64QON7/P9WCj+sO6Zz+K\n\tuY+GVQ9NeVqGn++VVCym09K0s3Cbv2QmJ6Eql8it22AGV6hLdMpa7GDkcFKq7waEE1Q6\n\th5VbSnBPac3+dhuxwbX+J4KD63f2c7SSxZ2TZhXBClwfEiJzocy6AMQFGyiZ3OKIOWhG\n\t89e8yukefTxg55rmB1ieDOj4gfV3ZhwgHHxyaH9WZAe0I6D6nX6xQmKzOeupt4/7lDws\n\tPlwjd3VwP5X6IfH8eXxpPD+W2iQ87obYZEc0/aGTLmJJzu4pnTG0KIwJlgOj0R85znr4\n\t/N8g=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"VGHmUupf\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=jxjXqmUbUswsZodyQaeRvvXK4xaUUmSAn7PGKKrWp0c=;\n\tb=ttWZjd49eH3K4vjqeb3iOEj79seeg1TQaHQJfsQUoBs14xkZYbK2JsGt/iBCzGPLp2\n\tvFO1BdFyZ1lwNJANvJ43LlvLvFbWR6OJlJVnYXzzr6USOgCMICH3BdZYXqt5XOyLMM1e\n\txqhqiON5G0qS7O/kRCJ1l9dP60tEeGw0Wo9+24jkc/8npLz8gJ0FqKy+MCz7yApjxyVj\n\tW9J1aA9jzLPhu+E3FwYE/j9DnPfAFI2k1d5T3UJcvlDr6WwyNw05HvS5ScUFZjFov/9Q\n\tLcS2/xB91Ewf/Pref8zaiXkCVXqpnp/U9z9RQrxcxPO8PcHl1r5iT/RMkwN5glsOeYHi\n\tzJ2w==","X-Gm-Message-State":"ACrzQf23qWMgmQypO804rjly7G3oMoi5W/fVTIBOhPW5yi6aNtZBXFxY\n\tDOnWcvVd4LqbsaLczjEz9C3zdYoqquqkF/V2k+4XzAwiRzcNTg==","X-Google-Smtp-Source":"AMsMyM7P0m2Km55eihcEgpMLJj7alhn/mXWtFPZa1tauTCCZhL4+XG7+ZFdXXRiDU253xxiA9vtx5FNbuXm14Y8DRnQ=","X-Received":"by 2002:a05:6402:1454:b0:461:9b5c:2fbc with SMTP id\n\td20-20020a056402145400b004619b5c2fbcmr30395343edx.276.1667492785237;\n\tThu, 03 Nov 2022 09:26:25 -0700 (PDT)","MIME-Version":"1.0","References":"<20221103104027.4197-1-david.plowman@raspberrypi.com>\n\t<20221103104027.4197-3-david.plowman@raspberrypi.com>\n\t\n\t\n\t\n\t<20221103152655.av2wiqpyic2pjgea@uno.localdomain>","In-Reply-To":"<20221103152655.av2wiqpyic2pjgea@uno.localdomain>","Date":"Thu, 3 Nov 2022 16:26:07 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do\n\tnot clear camera flips when listing formats","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","From":"Dave Stevenson via libcamera-devel\n\t","Reply-To":"Dave Stevenson ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25739,"web_url":"https://patchwork.libcamera.org/comment/25739/","msgid":"<20221103172928.b6qegcdsxbocp2xh@uno.localdomain>","date":"2022-11-03T17:29:28","subject":"Re: [libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do\n\tnot clear camera flips when listing formats","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Dave\n\nOn Thu, Nov 03, 2022 at 04:26:07PM +0000, Dave Stevenson wrote:\n> Hi Jacopo\n>\n> On Thu, 3 Nov 2022 at 15:26, Jacopo Mondi wrote:\n> >\n> > Hi again\n> >\n> > On Thu, Nov 03, 2022 at 11:52:23AM +0000, Dave Stevenson via libcamera-devel wrote:\n> > > On Thu, 3 Nov 2022 at 11:24, David Plowman\n> > > wrote:\n> > > >\n> > > > Hi Dave\n> > > >\n> > > > On Thu, 3 Nov 2022 at 11:02, Dave Stevenson\n> > > > wrote:\n> > > > >\n> > > > > Hi David\n> > > > >\n> > > > > On Thu, 3 Nov 2022 at 10:40, David Plowman via libcamera-devel\n> > > > > wrote:\n> > > > > >\n> > > > > > Previously the code used to clear the camnera's h and v flip bits when\n> > > > > > enumerating the supported formats so as to obtain any Bayer formats in\n> > > > > > the sensor's native (untransformed) orientation. However this fails\n> > > > > > when the camera is already in use elsewhere.\n> > > > > >\n> > > > > > Instead, we query the current state of the flip bits and transform the\n> > > > > > formats - which we obtain in their flipped orientation - back into\n> > > > > > their native orientation to be stored.\n> > > > >\n> > > > > I don't believe that libcamera knows for definite whether the sensor\n> > > > > changes the Bayer order or not. Several of the OnSemi sensors I have\n> > > > > seen do not change, presumably as they shift their crop by 1 pixel.\n> > > >\n> > > > Yes, that's a good point. I've vaguely assumed that I can check the\n> > > > control's V4L2_CTRL_FLAG_MODIFY_LAYOUT to discover whether this is the\n> > > > case or not, but as we keep on discovering, there may be no guarantees\n> > > > about this. Do we know what these particular sensors do here? The Pi\n> > > > PH actually already assumes this flag \"works\", so the changes here\n> > > > possibly don't make anything worse, though they do engrain the\n> > > > assumption more deeply...\n> > >\n> > > Yes I believe V4L2_CTRL_FLAG_MODIFY_LAYOUT should do what you want.\n> > >\n> > > The OnSemi sensors I'm thinking of don't have drivers in mainline, so\n> > > there's no driver to check currently.\n> > >\n> > > Looking at mainline, only imx219 and imx258 set the flag.\n> > > - ov2680, imx208, imx319, imx355, mt9m001, ov08d10 all change the\n> > > order but don't set V4L2_CTRL_FLAG_MODIFY_LAYOUT\n> > > - hi847 and ov13b10 move the crop to maintain the Bayer order.\n> > > - imx274 and ov5648 only ever report one order, but supports flips and\n> > > has no apparent cropping shift.\n> > > - I have missed it in my imx290 patches to add flips that I was about to send!\n> > >\n> > > So, as Jacopo summarised the situation on event handlers for controls,\n> > > room for improvement!\n> > >\n> >\n> > To add more pleasure, as discussed on a review comment on Dave's\n> > series\n> > https://patchwork.linuxtv.org/project/linux-media/patch/20221005152809.3785786-12-dave.stevenson@raspberrypi.com/\n> > Some sensors assumes to be rotated and bury their rotation\n> > settings in their register tables.\n> >\n> > In the example of the imx258 sensor Dave has mentioned:\n> >\n> > - The bayer order reported through the format is GRBG\n> > https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c#L822\n> >\n> > - The sensor driver applies mirroring by default at s_stream\n> > time (without specifying the direction, vertical or horizontal)\n> >\n> > /* Set Orientation be 180 degree */\n> > ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,\n> > IMX258_REG_VALUE_08BIT, REG_CONFIG_MIRROR_FLIP);\n>\n> #define REG_CONFIG_MIRROR_FLIP 0x03 [1]\n> so this is doing BOTH H & V flips in one hit.\n>\n> Datasheet for imx258 at [2].\n\nYour google-fu is certainly better than mine\n\n>\n> [1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c#L78\n> [2] https://web.archive.org/web/20201027131326/www.hi.app/IMX258-datasheet.pdf\n>\n> > - I found an interesting comment on the patch series changelog:\n> > https://www.spinics.net/lists/linux-media/msg133840.html\n> > Output order of test pattern is always BGGR, so it needs a flip to\n> > rotate bayer pattern to required one (GRBG)\n>\n> It's not uncommon that the test patterns are independent of flips as\n> there is no image array to read out in the reverse direction.\n> There are often 4 registers to configure the values to stick into each\n> of the 4 colour channels, and those are simply inserted in order.\n> Registers 0x0602-0x0609 on IMX258.\n\nThanks, indeed my assumption that the test pattern was reflecting the\nnative order was incorrect\n\n>\n> TBH I've never understood the love of implementing test patterns in\n> drivers. If a driver is merged into mainline then there should be a\n> fair confidence that it works, therefore why worry about test\n> patterns? Isn't a captured image more valuable?\n>\n> > So it might seem legit to presume the native Bayer pattern on the\n> > pixel array is BGGR, read from the right-bottom corner:\n>\n> I'm not sure if you're referencing imx258 specifically here, or\n> talking generically.\n> imx258 is a total mess, and I need to get around to upstreaming my\n> patches at [3].\n>\n> The native Bayer order for imx258 is RGGB (see Fig 5-4 page 84, Fig\n> 5-3 page 79, and several places in section 4). Surely the datasheet\n> has to count as definitive.\n>\n\nIndeed\n\n> As in patch [4], Y_ADD_STA register is set to 0, and Y_ADD_END to\n> 3118, giving 3119 lines total for 3118 lines of readout. The hardcoded\n> V flip on that starts us on the \"wrong\" line of the Bayer pattern for\n> any sane person looking at it and trying to work out what the heck is\n> going on - we're still getting RGGB. H flip that and the resulting\n> Bayer order is GRBG. QED.\n>\n\nAs usual, everything works by chance!\n\nWith a correct Y_ADD_END we would then get\n\n RGGB -> vflip -> GBRG -> hflip -> BGGR\n\nIt mean that fixing the driver will imply changing the reported\nbayer pattern I presume..\n\n> [3] https://github.com/raspberrypi/linux/commits/rpi-5.15.y/drivers/media/i2c/imx258.c\n> [4] https://github.com/raspberrypi/linux/commit/1942d0da85bac79f3d2a1c2d8e797e97bd16b618\n\nAh yes it does :)\n\n>\n> >\n> > RG RG RG RG RG\n> > GB GB GB GB GB\n> > (1, n) RG RG RG RG RG (1,0)\n> > GB GB GB GB GB\n> > (0, n) <-- (0,0)\n> >\n> > To obtain the Bayer GRBG order advertised as output format I presume the\n> > REG_CONFIG_MIRROR_FLIP vertically flips the image, moving the (0,0)\n> > pixel coordinate, from where reading is started, on the top-right corner:\n> >\n> > (0, n) <-- (0,0)\n> > RG RG RG RG RG\n> > (1, n) GB GB GB GB GB (1, 0)\n> > RG RG RG RG RG\n> > GB GB GB GB GB\n>\n> Due to the error on Y_ADD_END, implementing a VFLIP gave no change in\n> Bayer order! Work that one out in userspace (although it is a driver\n> bug that causes it).\n>\n> > According to your below patch and Bayer::transform():\n> > - BGGR becomes GRBG via a vertical flip, right ? So this should\n> > confirm the above assumption about an unconditional vertical flip\n> >\n> > If the driver would register flip controls, it would register them as\n> > VFLIP = 1, HFLIP = 0. Here below you would get GRBG, vertically\n> > transform it to BGGR and store BGGR as \"native\" format, but now you\n> > won't be able to apply it to the sensor, as it only supports GRBG.\n> >\n> > What drivers should do is to register somehow their -native- Bayer\n> > pattern, use a generic BAYER format in set_fmt and let userspace deal\n> > with rotations and Bayer pattern re-odering as userspace is\n> > able to access the mounting rotation via the CID_CAMERA_ROTATION\n> > control.\n>\n> Doing so goes against the V4L2 principle of the V4L2_PIX_FMT_* telling\n> you exactly how to interpret the pixel data.\n>\n> I need to check the wording, but I thought CID_CAMERA_ROTATION told\n> you the orientation of the sensor within the device such that you\n> could stick it into EXIF or similar metadata. It doesn't tell you how\n> the Bayer order would change.\n>\n\nRotation != flipping, you're right I was mixing the two.\n\n> > Unfortunately no driver afaict behaves this way. They might report via\n> > v4l2-ctl if any flip is applied to them, as Dave does in the series,\n> > but adjusting the Bayer pattern as you do below might lead to results\n> > that won't apply on the sensor. Do you agree or did I get lost ?\n>\n> AIUI At the point of actually wanting to stream, flips will be\n> applied, and the format reported by the sensor driver will then be\n> used to configure the rest of the pipeline.\n> I hope that userspace never thinks it knows better than the driver\n> with regard to Bayer order - as above there are enough quirks that it\n> will get it wrong.\n>\n\njust for sake of discussion:\nlibcamera would simply get from somewhere (driver or sensor properties\ndatabase) the native (or non-rotated) format, inspect the flips and\nknow what format report to applications. Driver will accept a wildcard\nBAYER format. But yes, good luck configuring the pipeline if any\ncomponent along the way is sensible to the bayer ordering.\n\n> > To be honest I would drop clearing the flips, and to\n> > protect against concurrent usages, validate that the current flip values\n> > match the default controls value. If the two do not match, and the\n> > driver advertises V4L2_CTRL_FLAG_MODIFY_LAYOUT, it means the mbus code\n> > we're seeing right now is the result of another user having flipped\n> > the image. In this case you could flip the format like you're doing\n> > below to obtain the \"unflipped\" Bayer pattern, as we know it will work\n> > as it is the \"defaul configuration\" order.\n>\n> A colour equivalent to ov9282 is going to blow up your assumption on\n> default value. The original driver didn't support flips, but H & V\n> flipped the image (much like imx258), so to avoid regressions we have\n> a default of being flipped.\n\nNot sure I get why. The color equivalent will report the bayer order\nof the H&V flipped Bayer format version. According to\nyour series, it will also report VFLIP = HFLIP = 1 by default and\nread-only. As they're RO, no V4L2_CTRL_FLAG_MODIFY_LAYOUT flag is\nregistered, and no transform is applied. The sensor will only speak\nthe rot180 bayer format version.\n\nThe patch below transforms only if V4L2_CTRL_FLAG_MODIFY_LAYOUT, which\nimplies the driver can speak the non-modified format too, so I guess\nit works under the assumptions that:\n\n1) Drivers report all the flips they apply implicitly and not all of\n them do\n2) They correctly register V4L2_CTRL_FLAG_MODIFY_LAYOUT if flip\n modifies the bayer pattern order and as you noticed not all of them\n do so\n\nI guess it will be hard to compliance-check drivers for that and this\nrequirements will have to be enforced during review, possibly by\ninspecting the register tables making sure any flip bit is there\nsilently set.\n\n> Same would have been true for imx258, except that there the original\n> driver read the \"rotation\" fwnode parameter and ensured it was 180. I\n> think that is now going to be incorrectly interpreted as the sensor\n> has done the rot180, and it will tell userspace rot180 as well, so\n> EXIF headers etc will say that it is stored inverted.\n>\n\nYes, the ROTATION property should not be used to decide if the driver\nshould flip or not, but just by application to correctly visualize the\nimages..\n\n> > Final question, and sorry for the long email, but what would it be the\n> > real purpose of obtaining the native bayer pattern order in your use case. I\n> > presume we had that attempt to get the native format because android\n> > wants to have the native order reported. I think that CameraSensor\n> > should instead try to use a bayer order that work for the drive in its\n> > default configuration which, if the driver claims to be rotated by\n> > default, won't match the actual native order.\n> >\n> > We have a camera properties database where such native information\n> > could eventually be stored if it's of interest!\n>\n> Store it in two places - increase the chance of it being wrong! ;)\n>\n> I am starting to write a guide for things to check with sensor\n> drivers, so hopefully future drivers will do what is defined as the\n> correct thing. Perhaps we'll find the time to fix older drivers - I'm\n> just doing a patch set for V4L2_CTRL_FLAG_MODIFY_LAYOUT.\n>\n\nThanks, hopefully this will increases the sensor drivers consistency in\nthe kernel.\n\n> Dave\n>\n> > Hope I didn't get lost!\n> >\n> > > Dave\n> > >\n> > > > Presumably you can rely on the formats being returned correctly after\n> > > > setting the flip bits, though having to re-query the formats from the\n> > > > device like that would be irritating too. And you'd never be able to\n> > > > predict the format ahead of time, only after setting it. I don't know\n> > > > if that's better or worse than saying \"change the driver\". Opinions\n> > > > welcome on that one!\n> > > >\n> > > > To be honest I always have difficulty with the Bayer order being part\n> > > > of the format. An application might legitimately want to have a say\n> > > > about the bit depth or packing, but the Bayer order?? Being able to\n> > > > request an XXXX (\"anything goes\") Bayer order, which gets updated once\n> > > > the sensor is configured, would be much more application-friendly. But\n> > > > another can of worms!!\n> >\n> > Agreed, userspace has all the information to adjust the ordering as it\n> > likes..\n> >\n> > > >\n> > > > David\n> > > >\n> > > > >\n> > > > > The original comment mentioned that scenario:\n> > > > > \"This is harmless for sensors where the flips don't affect the Bayer order\"\n> > > > >\n> > > > > Dave\n> > > > >\n> > > > > > Signed-off-by: David Plowman \n> > > > > > ---\n> > > > > > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++------\n> > > > > > 1 file changed, 43 insertions(+), 8 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > > > index 572a313a..6670dfb9 100644\n> > > > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > > > @@ -19,6 +19,8 @@\n> > > > > >\n> > > > > > #include \n> > > > > >\n> > > > > > +#include \n> > > > > > +\n> > > > > > #include \"libcamera/internal/bayer_format.h\"\n> > > > > > #include \"libcamera/internal/camera_lens.h\"\n> > > > > > #include \"libcamera/internal/camera_sensor_properties.h\"\n> > > > > > @@ -108,18 +110,51 @@ int CameraSensor::init()\n> > > > > > return ret;\n> > > > > >\n> > > > > > /*\n> > > > > > - * Clear any flips to be sure we get the \"native\" Bayer order. This is\n> > > > > > - * harmless for sensors where the flips don't affect the Bayer order.\n> > > > > > + * We want to get the native mbus codes for the sensor, without any flips.\n> > > > > > + * We can't clear any flips here, so we have to read the current values\n> > > > > > + * (if the flip controls exist), decide whether they actually modify any\n> > > > > > + * output Bayer pattern, and finally undo their effect on the formats.\n> > > > > > + *\n> > > > > > + * First, check if the flip controls exist and if so read them.\n> > > > > > */\n> > > > > > ControlList ctrls(subdev_->controls());\n> > > > > > - if (subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end())\n> > > > > > - ctrls.set(V4L2_CID_HFLIP, 0);\n> > > > > > - if (subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end())\n> > > > > > - ctrls.set(V4L2_CID_VFLIP, 0);\n> > > > > > - subdev_->setControls(&ctrls);\n> > > > > > + std::vector flipCtrlIds;\n> > > > > > + bool hasHflip = subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end();\n> > > > > > + bool hasVflip = subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end();\n> > > > > > + if (hasHflip)\n> > > > > > + flipCtrlIds.push_back(V4L2_CID_HFLIP);\n> > > > > > + if (hasVflip)\n> > > > > > + flipCtrlIds.push_back(V4L2_CID_VFLIP);\n> > > > > > + ControlList flipCtrls = subdev_->getControls(flipCtrlIds);\n> > > > > > +\n> > > > > > + /* Now construct a transform that would undo any flips. */\n> > > > > > + Transform transform = Transform::Identity;\n> > > > > > + if (hasHflip && flipCtrls.get(V4L2_CID_HFLIP).get()) {\n> > > > > > + const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_HFLIP);\n> > > > > > + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)\n> > > > > > + transform |= Transform::HFlip;\n> > > > > > + }\n> > > > > > + if (hasVflip && flipCtrls.get(V4L2_CID_VFLIP).get()) {\n> > > > > > + const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_VFLIP);\n> > > > > > + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)\n> > > > > > + transform |= Transform::VFlip;\n> > > > > > + }\n> > > > > > +\n> > > > > > + /* Finally get the formats, and apply the transform to the mbus codes. */\n> > > > > > + auto formats = subdev_->formats(pad_);\n> > > > > > + for (const auto &format : formats) {\n> > > > > > + unsigned int mbusCode = format.first;\n> > > > > > + BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);\n> > > > > > + bool valid = true;\n> > > > > > +\n> > > > > > + if (bayerFormat.isValid())\n> > > > > > + mbusCode = bayerFormat.transform(transform).toMbusCode(valid);\n> > > > > > +\n> > > > > > + if (valid)\n> > > > > > + formats_[mbusCode] = std::move(format.second);\n> > > > > > + }\n> > > > > >\n> > > > > > /* Enumerate, sort and cache media bus codes and sizes. */\n> > > > > > - formats_ = subdev_->formats(pad_);\n> > > > > > if (formats_.empty()) {\n> > > > > > LOG(CameraSensor, Error) << \"No image format found\";\n> > > > > > return -EINVAL;\n> > > > > > --\n> > > > > > 2.30.2\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 3C3F7BDB16\n\tfor ;\n\tThu, 3 Nov 2022 17:29:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6995263036;\n\tThu, 3 Nov 2022 18:29:33 +0100 (CET)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4CC0C61F42\n\tfor ;\n\tThu, 3 Nov 2022 18:29:31 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 6DF3760009;\n\tThu, 3 Nov 2022 17:29:30 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667496573;\n\tbh=mDgPnC8fokY7PNmQSzCfmJlSFn02nO8jorOeiKd3nb8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=RtrJap7n8h5idBrO/pJN0K+ckFVbr8WBUD5KldbKVnHu8LPHRQtVEOpP0rdASp9zn\n\tx/KiWDAf6iApIeyNFg+LEzWByZl0pnjoOWM5jJC8jP2B9OR0dRPSPeTCPlRz/RqzE9\n\t9MULoHs6ECKqIzAxzG/ntmp355DuhDehP7/4jfHhNW8oRRP2InbdPogqnkLz0Md/kx\n\t7Xe1iGA7PxrCpIwoo+aN3F4Gn0IGeHIMskbmrkKJ99uVcTzv4ioZAJm5q4UjXrfKum\n\tfIydYIj3+NZ7fwJjlEFw8JqfnZ54suJr8deYoCdv9eoJiWX47um8CmQsEOznCVH8gI\n\thvJivrFNFbVdQ==","Date":"Thu, 3 Nov 2022 18:29:28 +0100","To":"Dave Stevenson ","Message-ID":"<20221103172928.b6qegcdsxbocp2xh@uno.localdomain>","References":"<20221103104027.4197-1-david.plowman@raspberrypi.com>\n\t<20221103104027.4197-3-david.plowman@raspberrypi.com>\n\t\n\t\n\t\n\t<20221103152655.av2wiqpyic2pjgea@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do\n\tnot clear camera flips when listing formats","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]