[{"id":2514,"web_url":"https://patchwork.libcamera.org/comment/2514/","msgid":"<20190828112758.GP28351@bigcity.dyn.berto.se>","date":"2019-08-28T11:27:58","subject":"Re: [libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor:\n\tCollect camera properties","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-08-27 11:50:05 +0200, Jacopo Mondi wrote:\n> Store the camera sensor location and rotation by parsing the\n> associated V4L2 controls.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/camera_sensor.cpp | 39 +++++++++++++++++++++++++++\n> src/libcamera/include/camera_sensor.h | 4 +++\n> 2 files changed, 43 insertions(+)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index a7670b449b31..fc7fdcdcaf5b 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -89,6 +89,45 @@ int CameraSensor::init()\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> +\t/* Retrieve and store the camera sensor properties. */\n> +\tV4L2ControlList controls({\n> +\t\tV4L2_CID_CAMERA_SENSOR_LOCATION,\n> +\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> +\t});\n> +\tret = subdev_->getControls(&controls);\n> +\tif (ret) {\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"Failed to get camera sensor controls: \" << ret;\n> +\t\treturn ret;\n> +\t}\n\nI still feel this is a bit harsh for the moment. If the sensor don't \nsupport V4L2_CID_CAMERA_SENSOR_LOCATION and\nV4L2_CID_CAMERA_SENSOR_ROTATION this will fail and the camera is not \nregistered with the system. I see two problems with this,\n\n1. If the camera location is EXTERNAL is rotation really a mandatory \n property?\n\n2. I agree that in the future when the kernel support for \n SENSOR_LOCATION have taken hold we can make it mandatory. For now \n would it not be a good idea to set location to EXTERNAL if it's not \n present and print a warning?\n\n I fear we will block this series for a very long time without easing \n the constraints for a while.\n\n> +\n> +\tV4L2Control *control = controls[V4L2_CID_CAMERA_SENSOR_LOCATION];\n> +\tint64_t value = control->value();\n> +\tswitch (value) {\n> +\tcase V4L2_LOCATION_EXTERNAL:\n> +\t\tlocation_ = CAMERA_LOCATION_EXTERNAL;\n> +\t\tbreak;\n> +\tcase V4L2_LOCATION_FRONT:\n> +\t\tlocation_ = CAMERA_LOCATION_FRONT;\n> +\t\tbreak;\n> +\tcase V4L2_LOCATION_BACK:\n> +\t\tlocation_ = CAMERA_LOCATION_BACK;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"Unsupported camera location: \" << value;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tcontrol = controls[V4L2_CID_CAMERA_SENSOR_ROTATION];\n> +\tvalue = control->value();\n> +\tif (value < 0 || value > 360) {\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"Unsupported camera rotation: \" << value;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\trotation_ = value;\n> +\n> \t/* Enumerate and cache media bus codes and sizes. */\n> \tconst ImageFormats formats = subdev_->formats(0);\n> \tif (formats.isEmpty()) {\n> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> index fe033fb374c1..32d39127b275 100644\n> --- a/src/libcamera/include/camera_sensor.h\n> +++ b/src/libcamera/include/camera_sensor.h\n> @@ -10,6 +10,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> \n> #include \"log.h\"\n> @@ -55,6 +56,9 @@ private:\n> \n> \tstd::vector mbusCodes_;\n> \tstd::vector sizes_;\n> +\n> +\tCameraLocation location_;\n> +\tunsigned int rotation_;\n> };\n> \n> } /* namespace libcamera */\n> -- \n> 2.23.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DC73D60BF6\n\tfor ;\n\tWed, 28 Aug 2019 13:27:59 +0200 (CEST)","by mail-lf1-x142.google.com with SMTP id h27so1855614lfp.2\n\tfor ;\n\tWed, 28 Aug 2019 04:27:59 -0700 (PDT)","from localhost (h-177-236.A463.priv.bahnhof.se. [217.31.177.236])\n\tby smtp.gmail.com with ESMTPSA id\n\t83sm568217lje.39.2019.08.28.04.27.58\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 28 Aug 2019 04:27:58 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=1DkVKLraeUuwn76MDvcqMwjbLIzJNKHv9uQhAiNYhvM=;\n\tb=VqxXVBfYeNQO+JuMdNUCppYNg6kcB31krgBunOvJn2u9Ndks4K4KZALCJzO+iD/XBm\n\tiRoEpnJr9lIPEoaOm9MOWQXk5FpLiT51k5buzFeIFVXqy5+6wCOZW/Wweo43xADJEDpn\n\tD+jtWjTwJl2R8p5NTNEWHzNcmE7CryEWoCLuWDVM5qzvZfKTHTIIMlw66XZrK+FPRRHy\n\tdKiO6AKB797VnJj9D6SE4DaTkFAw50ZPIYJNLUrhdJt1hwmcz6CdPiJaukWB7gF7rSDa\n\tIgKhApXpBsxjYO+frWrr4TwPkqdKBF5Mcp3k6ul6Q2GHuSpIcPahZboTuFpmnwo1l9f6\n\tBmBg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=1DkVKLraeUuwn76MDvcqMwjbLIzJNKHv9uQhAiNYhvM=;\n\tb=uE5pYXxMrtFgJHHLpDC2M2rZZKrUCyEssfVVr9mksgD0+Je3fE2yVASGUNLs5Zf7Kc\n\tWDYDt3hfe1UpzEH689euRkoiMDqqKZvPynm0lvfZglVz6vWcKbdk4Kio4QKSSxCTVyt2\n\tNAZTnJ/EEhLgMZdAoEmlvATeBfP2oQzhwsem5NpTxwCOCBIAP2cacxKiHzH0Kpalc13h\n\tXf4YSPoFbEnBNSJPDW5IAAkgTMuKcFMvLMO4kV8B5ag9EBqSmBnRhAUrBGWby+K3OPLj\n\tcYK7ZIB488QzDYIboD2QMzmkOnBQbPllC4JyEz+OAo+qexbvD0+1a6QVDBgKTEMGUSc3\n\t1Erw==","X-Gm-Message-State":"APjAAAXOxlkWpcVdHzsq1BimcJ4VkCZtsf1R74Ckq6HVBhKP7r9cUEjD\n\t44TDALnmBSxD+9bdTqRoxBmH+w==","X-Google-Smtp-Source":"APXvYqz8x4cGcCznkxq8I0ODKzO/XWPObucdAdUH3LHuQc17oSJHV7kvJA/6tfeCP4BH8iCseh88FA==","X-Received":"by 2002:ac2:558a:: with SMTP id\n\tv10mr2217234lfg.162.1566991679401; \n\tWed, 28 Aug 2019 04:27:59 -0700 (PDT)","Date":"Wed, 28 Aug 2019 13:27:58 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190828112758.GP28351@bigcity.dyn.berto.se>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190827095008.11405-6-jacopo@jmondi.org>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor:\n\tCollect camera properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 28 Aug 2019 11:28:00 -0000"}},{"id":2526,"web_url":"https://patchwork.libcamera.org/comment/2526/","msgid":"<20190828163101.eojh5kea5t32tppy@uno.localdomain>","date":"2019-08-28T16:31:01","subject":"Re: [libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor:\n\tCollect camera properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Aug 28, 2019 at 01:27:58PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2019-08-27 11:50:05 +0200, Jacopo Mondi wrote:\n> > Store the camera sensor location and rotation by parsing the\n> > associated V4L2 controls.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/camera_sensor.cpp | 39 +++++++++++++++++++++++++++\n> > src/libcamera/include/camera_sensor.h | 4 +++\n> > 2 files changed, 43 insertions(+)\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index a7670b449b31..fc7fdcdcaf5b 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -89,6 +89,45 @@ int CameraSensor::init()\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> >\n> > +\t/* Retrieve and store the camera sensor properties. */\n> > +\tV4L2ControlList controls({\n> > +\t\tV4L2_CID_CAMERA_SENSOR_LOCATION,\n> > +\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> > +\t});\n> > +\tret = subdev_->getControls(&controls);\n> > +\tif (ret) {\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"Failed to get camera sensor controls: \" << ret;\n> > +\t\treturn ret;\n> > +\t}\n>\n> I still feel this is a bit harsh for the moment. If the sensor don't\n> support V4L2_CID_CAMERA_SENSOR_LOCATION and\n> V4L2_CID_CAMERA_SENSOR_ROTATION this will fail and the camera is not\n> registered with the system. I see two problems with this,\n>\n> 1. If the camera location is EXTERNAL is rotation really a mandatory\n> property?\n>\n> 2. I agree that in the future when the kernel support for\n> SENSOR_LOCATION have taken hold we can make it mandatory. For now\n> would it not be a good idea to set location to EXTERNAL if it's not\n> present and print a warning?\n>\n> I fear we will block this series for a very long time without easing\n> the constraints for a while.\n>\n\nIdeally we should have all of this properties set. It's not a big work\nto implement them, and I fear if we don't enforce them, people will\njust ignore them, but I would rather defer policies decisions like this\none to Laurent...\n\nThanks\n j\n\n> > +\n> > +\tV4L2Control *control = controls[V4L2_CID_CAMERA_SENSOR_LOCATION];\n> > +\tint64_t value = control->value();\n> > +\tswitch (value) {\n> > +\tcase V4L2_LOCATION_EXTERNAL:\n> > +\t\tlocation_ = CAMERA_LOCATION_EXTERNAL;\n> > +\t\tbreak;\n> > +\tcase V4L2_LOCATION_FRONT:\n> > +\t\tlocation_ = CAMERA_LOCATION_FRONT;\n> > +\t\tbreak;\n> > +\tcase V4L2_LOCATION_BACK:\n> > +\t\tlocation_ = CAMERA_LOCATION_BACK;\n> > +\t\tbreak;\n> > +\tdefault:\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"Unsupported camera location: \" << value;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tcontrol = controls[V4L2_CID_CAMERA_SENSOR_ROTATION];\n> > +\tvalue = control->value();\n> > +\tif (value < 0 || value > 360) {\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"Unsupported camera rotation: \" << value;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\trotation_ = value;\n> > +\n> > \t/* Enumerate and cache media bus codes and sizes. */\n> > \tconst ImageFormats formats = subdev_->formats(0);\n> > \tif (formats.isEmpty()) {\n> > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > index fe033fb374c1..32d39127b275 100644\n> > --- a/src/libcamera/include/camera_sensor.h\n> > +++ b/src/libcamera/include/camera_sensor.h\n> > @@ -10,6 +10,7 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \n> >\n> > #include \"log.h\"\n> > @@ -55,6 +56,9 @@ private:\n> >\n> > \tstd::vector mbusCodes_;\n> > \tstd::vector sizes_;\n> > +\n> > +\tCameraLocation location_;\n> > +\tunsigned int rotation_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > --\n> > 2.23.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DAB1360BF6\n\tfor ;\n\tWed, 28 Aug 2019 18:29:30 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 5877F24000B;\n\tWed, 28 Aug 2019 16:29:29 +0000 (UTC)"],"Date":"Wed, 28 Aug 2019 18:31:01 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190828163101.eojh5kea5t32tppy@uno.localdomain>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-6-jacopo@jmondi.org>\n\t<20190828112758.GP28351@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"z4pnt6fovc5z5okx\"","Content-Disposition":"inline","In-Reply-To":"<20190828112758.GP28351@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor:\n\tCollect camera properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 28 Aug 2019 16:29:31 -0000"}},{"id":2531,"web_url":"https://patchwork.libcamera.org/comment/2531/","msgid":"<20190829082947.GU28351@bigcity.dyn.berto.se>","date":"2019-08-29T08:29:47","subject":"Re: [libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor:\n\tCollect camera properties","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2019-08-28 18:31:01 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Wed, Aug 28, 2019 at 01:27:58PM +0200, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2019-08-27 11:50:05 +0200, Jacopo Mondi wrote:\n> > > Store the camera sensor location and rotation by parsing the\n> > > associated V4L2 controls.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/camera_sensor.cpp | 39 +++++++++++++++++++++++++++\n> > > src/libcamera/include/camera_sensor.h | 4 +++\n> > > 2 files changed, 43 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index a7670b449b31..fc7fdcdcaf5b 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -89,6 +89,45 @@ int CameraSensor::init()\n> > > \tif (ret < 0)\n> > > \t\treturn ret;\n> > >\n> > > +\t/* Retrieve and store the camera sensor properties. */\n> > > +\tV4L2ControlList controls({\n> > > +\t\tV4L2_CID_CAMERA_SENSOR_LOCATION,\n> > > +\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> > > +\t});\n> > > +\tret = subdev_->getControls(&controls);\n> > > +\tif (ret) {\n> > > +\t\tLOG(CameraSensor, Error)\n> > > +\t\t\t<< \"Failed to get camera sensor controls: \" << ret;\n> > > +\t\treturn ret;\n> > > +\t}\n> >\n> > I still feel this is a bit harsh for the moment. If the sensor don't\n> > support V4L2_CID_CAMERA_SENSOR_LOCATION and\n> > V4L2_CID_CAMERA_SENSOR_ROTATION this will fail and the camera is not\n> > registered with the system. I see two problems with this,\n> >\n> > 1. If the camera location is EXTERNAL is rotation really a mandatory\n> > property?\n> >\n> > 2. I agree that in the future when the kernel support for\n> > SENSOR_LOCATION have taken hold we can make it mandatory. For now\n> > would it not be a good idea to set location to EXTERNAL if it's not\n> > present and print a warning?\n> >\n> > I fear we will block this series for a very long time without easing\n> > the constraints for a while.\n> >\n> \n> Ideally we should have all of this properties set.\n\nSo rotation should be a mandatory property for external cameras? How do \nyou see that being implemented pluggable USB cameras such as UVC?\n\n> It's not a big work\n> to implement them, and I fear if we don't enforce them, people will\n> just ignore them, but I would rather defer policies decisions like this\n> one to Laurent...\n\nI agree that we should enforce it down the line, but for now I think we \nneed to either hold of on this series until upstream Linux support is \navailable for all our sensors used in our \"supported\" platforms. Or \nrelax the constraint to a warning, merge this series in libcamera and \nonce upstream Liunx support is available change the warning to an error.\n\nI'm not super happy to be forced to run kernels with out of tree code \nfor libcamera to function :-)\n\n> \n> Thanks\n> j\n> \n> > > +\n> > > +\tV4L2Control *control = controls[V4L2_CID_CAMERA_SENSOR_LOCATION];\n> > > +\tint64_t value = control->value();\n> > > +\tswitch (value) {\n> > > +\tcase V4L2_LOCATION_EXTERNAL:\n> > > +\t\tlocation_ = CAMERA_LOCATION_EXTERNAL;\n> > > +\t\tbreak;\n> > > +\tcase V4L2_LOCATION_FRONT:\n> > > +\t\tlocation_ = CAMERA_LOCATION_FRONT;\n> > > +\t\tbreak;\n> > > +\tcase V4L2_LOCATION_BACK:\n> > > +\t\tlocation_ = CAMERA_LOCATION_BACK;\n> > > +\t\tbreak;\n> > > +\tdefault:\n> > > +\t\tLOG(CameraSensor, Error)\n> > > +\t\t\t<< \"Unsupported camera location: \" << value;\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tcontrol = controls[V4L2_CID_CAMERA_SENSOR_ROTATION];\n> > > +\tvalue = control->value();\n> > > +\tif (value < 0 || value > 360) {\n> > > +\t\tLOG(CameraSensor, Error)\n> > > +\t\t\t<< \"Unsupported camera rotation: \" << value;\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\trotation_ = value;\n> > > +\n> > > \t/* Enumerate and cache media bus codes and sizes. */\n> > > \tconst ImageFormats formats = subdev_->formats(0);\n> > > \tif (formats.isEmpty()) {\n> > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > > index fe033fb374c1..32d39127b275 100644\n> > > --- a/src/libcamera/include/camera_sensor.h\n> > > +++ b/src/libcamera/include/camera_sensor.h\n> > > @@ -10,6 +10,7 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > >\n> > > #include \"log.h\"\n> > > @@ -55,6 +56,9 @@ private:\n> > >\n> > > \tstd::vector mbusCodes_;\n> > > \tstd::vector sizes_;\n> > > +\n> > > +\tCameraLocation location_;\n> > > +\tunsigned int rotation_;\n> > > };\n> > >\n> > > } /* namespace libcamera */\n> > > --\n> > > 2.23.0\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","headers":{"Return-Path":"","Received":["from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6640A60BCF\n\tfor ;\n\tThu, 29 Aug 2019 10:29:49 +0200 (CEST)","by mail-lj1-x244.google.com with SMTP id x4so2163202ljj.6\n\tfor ;\n\tThu, 29 Aug 2019 01:29:49 -0700 (PDT)","from localhost (h-177-236.A463.priv.bahnhof.se. [217.31.177.236])\n\tby smtp.gmail.com with ESMTPSA id\n\ta20sm281862lff.78.2019.08.29.01.29.47\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 29 Aug 2019 01:29:47 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=KC8VIpZZTj6KcKhdLQO1eZ1tW8d8POrSipADM337BDg=;\n\tb=d2UvBVsoIoCrzmZDIhjY/ONHqvY2zsZkKQxxQBx0gSylPnTzvozNvvxwXerJCGz+mq\n\tCqcPy4h9uVAb/83dSPT0/7YGM5dF8RRyaRwoAkocQQllnYWqhVgmEQ34gQJVBkRoZ9Gh\n\teploWvYpOYKiWGPjh+h6aL8tXkZnnveqAsa7SXf6S8z+cBhhyirTLfPSOZMqTODL0BRO\n\tQqwq5+H0KA8zdCI1qAJKHCTSaCZBhC7+wDyNe5obZOIbcLgnb1+f8bYVA8kcVCU5ldLN\n\tKNYMn9PE2CgOysHmMhbbzbjs6sev0Bj4y1gJ286LE8Y16GbuFNDIylOaei35dKiAxc8M\n\tcANQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=KC8VIpZZTj6KcKhdLQO1eZ1tW8d8POrSipADM337BDg=;\n\tb=WO14muYnbNkVoj5RHx54jClWii7GkC6JkTW0WMlsvbjZsk2cto4pbIqN8NwC4TPpfz\n\thP4fldCt9+74j8jREsG9y9w2NK8wClZykToOaBIUYKqWTFFWpzkxokbb1uV+flkttBwH\n\t+7db53vXBKtByzqYT6M49qdtWHtGTJKn0HZITA7vXucakBNjdkFpl7+xmR+NB5U+2ops\n\tGZHXGsYGzcXkPyfC4L1p8K4gArUoK6mzsSZNnzDZZ1vCXlIZ3FfuctyXnhEr1haIBQsb\n\tvxAAkVd33FAyo3ndE0kSpNN1zpTmKj1s5vqECvr6jpMVX4Z1xdiCu1crE+MUYobi/LBz\n\tt3aw==","X-Gm-Message-State":"APjAAAVLX/lPQ1E/G/lgpwKWAgRAeMEFDtiCVuvfV/DDNJYwAlPMG821\n\tJH2cslAcLFSx+wHdYLmIlVy+uA==","X-Google-Smtp-Source":"APXvYqykW6CXtgkQf24lY/kqhcPrCjLYRbYoMGnZbPzmweDWyJPuKxBul/Eb4oj4zNRjLwRxBV7pSA==","X-Received":"by 2002:a2e:864c:: with SMTP id i12mr4508633ljj.88.1567067388857;\n\tThu, 29 Aug 2019 01:29:48 -0700 (PDT)","Date":"Thu, 29 Aug 2019 10:29:47 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190829082947.GU28351@bigcity.dyn.berto.se>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-6-jacopo@jmondi.org>\n\t<20190828112758.GP28351@bigcity.dyn.berto.se>\n\t<20190828163101.eojh5kea5t32tppy@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190828163101.eojh5kea5t32tppy@uno.localdomain>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor:\n\tCollect camera properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 29 Aug 2019 08:29:49 -0000"}},{"id":2534,"web_url":"https://patchwork.libcamera.org/comment/2534/","msgid":"<20190829090334.6mrcwsugiasnamia@uno.localdomain>","date":"2019-08-29T09:03:34","subject":"Re: [libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor:\n\tCollect camera properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Aug 29, 2019 at 10:29:47AM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> On 2019-08-28 18:31:01 +0200, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Wed, Aug 28, 2019 at 01:27:58PM +0200, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2019-08-27 11:50:05 +0200, Jacopo Mondi wrote:\n> > > > Store the camera sensor location and rotation by parsing the\n> > > > associated V4L2 controls.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/libcamera/camera_sensor.cpp | 39 +++++++++++++++++++++++++++\n> > > > src/libcamera/include/camera_sensor.h | 4 +++\n> > > > 2 files changed, 43 insertions(+)\n> > > >\n> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > index a7670b449b31..fc7fdcdcaf5b 100644\n> > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > @@ -89,6 +89,45 @@ int CameraSensor::init()\n> > > > \tif (ret < 0)\n> > > > \t\treturn ret;\n> > > >\n> > > > +\t/* Retrieve and store the camera sensor properties. */\n> > > > +\tV4L2ControlList controls({\n> > > > +\t\tV4L2_CID_CAMERA_SENSOR_LOCATION,\n> > > > +\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> > > > +\t});\n> > > > +\tret = subdev_->getControls(&controls);\n> > > > +\tif (ret) {\n> > > > +\t\tLOG(CameraSensor, Error)\n> > > > +\t\t\t<< \"Failed to get camera sensor controls: \" << ret;\n> > > > +\t\treturn ret;\n> > > > +\t}\n> > >\n> > > I still feel this is a bit harsh for the moment. If the sensor don't\n> > > support V4L2_CID_CAMERA_SENSOR_LOCATION and\n> > > V4L2_CID_CAMERA_SENSOR_ROTATION this will fail and the camera is not\n> > > registered with the system. I see two problems with this,\n> > >\n> > > 1. If the camera location is EXTERNAL is rotation really a mandatory\n> > > property?\n> > >\n> > > 2. I agree that in the future when the kernel support for\n> > > SENSOR_LOCATION have taken hold we can make it mandatory. For now\n> > > would it not be a good idea to set location to EXTERNAL if it's not\n> > > present and print a warning?\n> > >\n> > > I fear we will block this series for a very long time without easing\n> > > the constraints for a while.\n> > >\n> >\n> > Ideally we should have all of this properties set.\n>\n> So rotation should be a mandatory property for external cameras? How do\n> you see that being implemented pluggable USB cameras such as UVC?\n>\n\nSet to 0? I don't know why one should produce a USB camera that works\nupside down, but hey, the world is a strange place isn't it?\n\nI'm fine accepting to default it to 0 for external cameras, but for\ninternal ones, I think this should be mandatory. Question is, it is\nnot mandatory on kernel side, do we want libcamera to enforce it? I\ndon't think forcing it to be mandatory on kernel side will go much far\n(probably rightfully).\n\n> > It's not a big work\n> > to implement them, and I fear if we don't enforce them, people will\n> > just ignore them, but I would rather defer policies decisions like this\n> > one to Laurent...\n>\n> I agree that we should enforce it down the line, but for now I think we\n> need to either hold of on this series until upstream Linux support is\n> available for all our sensors used in our \"supported\" platforms. Or\n> relax the constraint to a warning, merge this series in libcamera and\n> once upstream Liunx support is available change the warning to an error.\n>\n> I'm not super happy to be forced to run kernels with out of tree code\n> for libcamera to function :-)\n\nHopefully that code (at least for the devices we support) will get in\nsooner or later, so I'm not too concerned for the devices we have, and\nI think people wanting to run libcamera on their device should be\n'encouraged' to make their devices kernel interface as compliant as\npossible. How bad of a showstopper would this be I cannot tell, that's\nwhy I'm happy to defer the decision here to the group :)\n\n>\n> >\n> > Thanks\n> > j\n> >\n> > > > +\n> > > > +\tV4L2Control *control = controls[V4L2_CID_CAMERA_SENSOR_LOCATION];\n> > > > +\tint64_t value = control->value();\n> > > > +\tswitch (value) {\n> > > > +\tcase V4L2_LOCATION_EXTERNAL:\n> > > > +\t\tlocation_ = CAMERA_LOCATION_EXTERNAL;\n> > > > +\t\tbreak;\n> > > > +\tcase V4L2_LOCATION_FRONT:\n> > > > +\t\tlocation_ = CAMERA_LOCATION_FRONT;\n> > > > +\t\tbreak;\n> > > > +\tcase V4L2_LOCATION_BACK:\n> > > > +\t\tlocation_ = CAMERA_LOCATION_BACK;\n> > > > +\t\tbreak;\n> > > > +\tdefault:\n> > > > +\t\tLOG(CameraSensor, Error)\n> > > > +\t\t\t<< \"Unsupported camera location: \" << value;\n> > > > +\t\treturn -EINVAL;\n> > > > +\t}\n> > > > +\n> > > > +\tcontrol = controls[V4L2_CID_CAMERA_SENSOR_ROTATION];\n> > > > +\tvalue = control->value();\n> > > > +\tif (value < 0 || value > 360) {\n> > > > +\t\tLOG(CameraSensor, Error)\n> > > > +\t\t\t<< \"Unsupported camera rotation: \" << value;\n> > > > +\t\treturn -EINVAL;\n> > > > +\t}\n> > > > +\trotation_ = value;\n> > > > +\n> > > > \t/* Enumerate and cache media bus codes and sizes. */\n> > > > \tconst ImageFormats formats = subdev_->formats(0);\n> > > > \tif (formats.isEmpty()) {\n> > > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > > > index fe033fb374c1..32d39127b275 100644\n> > > > --- a/src/libcamera/include/camera_sensor.h\n> > > > +++ b/src/libcamera/include/camera_sensor.h\n> > > > @@ -10,6 +10,7 @@\n> > > > #include \n> > > > #include \n> > > >\n> > > > +#include \n> > > > #include \n> > > >\n> > > > #include \"log.h\"\n> > > > @@ -55,6 +56,9 @@ private:\n> > > >\n> > > > \tstd::vector mbusCodes_;\n> > > > \tstd::vector sizes_;\n> > > > +\n> > > > +\tCameraLocation location_;\n> > > > +\tunsigned int rotation_;\n> > > > };\n> > > >\n> > > > } /* namespace libcamera */\n> > > > --\n> > > > 2.23.0\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n>\n>\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 551A860BCF\n\tfor ;\n\tThu, 29 Aug 2019 11:02:02 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id B00DEFF80D;\n\tThu, 29 Aug 2019 09:02:01 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 29 Aug 2019 11:03:34 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190829090334.6mrcwsugiasnamia@uno.localdomain>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-6-jacopo@jmondi.org>\n\t<20190828112758.GP28351@bigcity.dyn.berto.se>\n\t<20190828163101.eojh5kea5t32tppy@uno.localdomain>\n\t<20190829082947.GU28351@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"oqis5blxe2cfalwx\"","Content-Disposition":"inline","In-Reply-To":"<20190829082947.GU28351@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor:\n\tCollect camera properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 29 Aug 2019 09:02:02 -0000"}},{"id":2539,"web_url":"https://patchwork.libcamera.org/comment/2539/","msgid":"<20190829091828.GX28351@bigcity.dyn.berto.se>","date":"2019-08-29T09:18:28","subject":"Re: [libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor:\n\tCollect camera properties","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2019-08-29 11:03:34 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Thu, Aug 29, 2019 at 10:29:47AM +0200, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > On 2019-08-28 18:31:01 +0200, Jacopo Mondi wrote:\n> > > Hi Niklas,\n> > >\n> > > On Wed, Aug 28, 2019 at 01:27:58PM +0200, Niklas Söderlund wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Thanks for your work.\n> > > >\n> > > > On 2019-08-27 11:50:05 +0200, Jacopo Mondi wrote:\n> > > > > Store the camera sensor location and rotation by parsing the\n> > > > > associated V4L2 controls.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/libcamera/camera_sensor.cpp | 39 +++++++++++++++++++++++++++\n> > > > > src/libcamera/include/camera_sensor.h | 4 +++\n> > > > > 2 files changed, 43 insertions(+)\n> > > > >\n> > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > > index a7670b449b31..fc7fdcdcaf5b 100644\n> > > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > > @@ -89,6 +89,45 @@ int CameraSensor::init()\n> > > > > \tif (ret < 0)\n> > > > > \t\treturn ret;\n> > > > >\n> > > > > +\t/* Retrieve and store the camera sensor properties. */\n> > > > > +\tV4L2ControlList controls({\n> > > > > +\t\tV4L2_CID_CAMERA_SENSOR_LOCATION,\n> > > > > +\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> > > > > +\t});\n> > > > > +\tret = subdev_->getControls(&controls);\n> > > > > +\tif (ret) {\n> > > > > +\t\tLOG(CameraSensor, Error)\n> > > > > +\t\t\t<< \"Failed to get camera sensor controls: \" << ret;\n> > > > > +\t\treturn ret;\n> > > > > +\t}\n> > > >\n> > > > I still feel this is a bit harsh for the moment. If the sensor don't\n> > > > support V4L2_CID_CAMERA_SENSOR_LOCATION and\n> > > > V4L2_CID_CAMERA_SENSOR_ROTATION this will fail and the camera is not\n> > > > registered with the system. I see two problems with this,\n> > > >\n> > > > 1. If the camera location is EXTERNAL is rotation really a mandatory\n> > > > property?\n> > > >\n> > > > 2. I agree that in the future when the kernel support for\n> > > > SENSOR_LOCATION have taken hold we can make it mandatory. For now\n> > > > would it not be a good idea to set location to EXTERNAL if it's not\n> > > > present and print a warning?\n> > > >\n> > > > I fear we will block this series for a very long time without easing\n> > > > the constraints for a while.\n> > > >\n> > >\n> > > Ideally we should have all of this properties set.\n> >\n> > So rotation should be a mandatory property for external cameras? How do\n> > you see that being implemented pluggable USB cameras such as UVC?\n> >\n> \n> Set to 0? I don't know why one should produce a USB camera that works\n> upside down, but hey, the world is a strange place isn't it?\n\nI agree it should be set to 0 inside libcamera, but do we wish for the \nV4L2_CID_CAMERA_SENSOR_ROTATION control to be made mandatory for \nEXTERNAL cameras? In my view we could look for the rotation control on \nexternal cameras an use the value if it exists, else default to 0. While \non FRONT/BACK cameras we could error out if the control is not \nsupported.\n\n> \n> I'm fine accepting to default it to 0 for external cameras, but for\n> internal ones, I think this should be mandatory. Question is, it is\n> not mandatory on kernel side, do we want libcamera to enforce it? I\n> don't think forcing it to be mandatory on kernel side will go much far\n> (probably rightfully).\n\nFor internal cameras I think libcamera should make the two controls \nmandatory. But only once the upstream Linux support is available. I \nshare your view that making it mandatory in Linux might be a bit far \nfetched.\n\n> \n> > > It's not a big work\n> > > to implement them, and I fear if we don't enforce them, people will\n> > > just ignore them, but I would rather defer policies decisions like this\n> > > one to Laurent...\n> >\n> > I agree that we should enforce it down the line, but for now I think we\n> > need to either hold of on this series until upstream Linux support is\n> > available for all our sensors used in our \"supported\" platforms. Or\n> > relax the constraint to a warning, merge this series in libcamera and\n> > once upstream Liunx support is available change the warning to an error.\n> >\n> > I'm not super happy to be forced to run kernels with out of tree code\n> > for libcamera to function :-)\n> \n> Hopefully that code (at least for the devices we support) will get in\n> sooner or later, so I'm not too concerned for the devices we have, and\n> I think people wanting to run libcamera on their device should be\n> 'encouraged' to make their devices kernel interface as compliant as\n> possible. How bad of a showstopper would this be I cannot tell, that's\n> why I'm happy to defer the decision here to the group :)\n\nI share your view, that the code hopefully will soon be upstream, but I \ndon't think we should depend or fail cameras until it's present in an \nreal upstream release.\n\n> \n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > > > +\n> > > > > +\tV4L2Control *control = controls[V4L2_CID_CAMERA_SENSOR_LOCATION];\n> > > > > +\tint64_t value = control->value();\n> > > > > +\tswitch (value) {\n> > > > > +\tcase V4L2_LOCATION_EXTERNAL:\n> > > > > +\t\tlocation_ = CAMERA_LOCATION_EXTERNAL;\n> > > > > +\t\tbreak;\n> > > > > +\tcase V4L2_LOCATION_FRONT:\n> > > > > +\t\tlocation_ = CAMERA_LOCATION_FRONT;\n> > > > > +\t\tbreak;\n> > > > > +\tcase V4L2_LOCATION_BACK:\n> > > > > +\t\tlocation_ = CAMERA_LOCATION_BACK;\n> > > > > +\t\tbreak;\n> > > > > +\tdefault:\n> > > > > +\t\tLOG(CameraSensor, Error)\n> > > > > +\t\t\t<< \"Unsupported camera location: \" << value;\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tcontrol = controls[V4L2_CID_CAMERA_SENSOR_ROTATION];\n> > > > > +\tvalue = control->value();\n> > > > > +\tif (value < 0 || value > 360) {\n> > > > > +\t\tLOG(CameraSensor, Error)\n> > > > > +\t\t\t<< \"Unsupported camera rotation: \" << value;\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\t}\n> > > > > +\trotation_ = value;\n> > > > > +\n> > > > > \t/* Enumerate and cache media bus codes and sizes. */\n> > > > > \tconst ImageFormats formats = subdev_->formats(0);\n> > > > > \tif (formats.isEmpty()) {\n> > > > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > > > > index fe033fb374c1..32d39127b275 100644\n> > > > > --- a/src/libcamera/include/camera_sensor.h\n> > > > > +++ b/src/libcamera/include/camera_sensor.h\n> > > > > @@ -10,6 +10,7 @@\n> > > > > #include \n> > > > > #include \n> > > > >\n> > > > > +#include \n> > > > > #include \n> > > > >\n> > > > > #include \"log.h\"\n> > > > > @@ -55,6 +56,9 @@ private:\n> > > > >\n> > > > > \tstd::vector mbusCodes_;\n> > > > > \tstd::vector sizes_;\n> > > > > +\n> > > > > +\tCameraLocation location_;\n> > > > > +\tunsigned int rotation_;\n> > > > > };\n> > > > >\n> > > > > } /* namespace libcamera */\n> > > > > --\n> > > > > 2.23.0\n> > > > >\n> > > > > _______________________________________________\n> > > > > libcamera-devel mailing list\n> > > > > libcamera-devel@lists.libcamera.org\n> > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > >\n> > > > --\n> > > > Regards,\n> > > > Niklas Söderlund\n> >\n> >\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","headers":{"Return-Path":"","Received":["from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7FC3160BCF\n\tfor ;\n\tThu, 29 Aug 2019 11:18:30 +0200 (CEST)","by mail-lj1-x244.google.com with SMTP id x3so2300008lji.5\n\tfor ;\n\tThu, 29 Aug 2019 02:18:30 -0700 (PDT)","from localhost (h-177-236.A463.priv.bahnhof.se. [217.31.177.236])\n\tby smtp.gmail.com with ESMTPSA id\n\th9sm253679lfp.40.2019.08.29.02.18.29\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 29 Aug 2019 02:18:29 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=lzUWVM6W6hGAGXjqQuMgS9sGP8CKkiLhTkecfOb91HQ=;\n\tb=NFFikaL9pXhqgo+sV5noglO+n5aL9ph8KxzhKq4xq2I9etRKuga+1j2EjhX9U0KUxU\n\tMH25decE9pyr1YW8n4JpUe10I8euYhgatTQ5NjzEqsceBLJ/CILRfSI94+g34CzfL8m8\n\tUXHA8PuboIdatgWfIoOUnaIulAttF7PfRB/YzzfKApumtgXokGl/1H0kO55+gcILOeVv\n\tdbva1BW/CewZnCQNapaschPylHOVPpvpaSJ6uFpsfmakYbup0yB29f2R5pZ1uCdNt9Hd\n\tmRC4HksC5FCtSgHcnH1AyENnSwgZV/ui0PZmgGDUXNGdJTcbU+V9aaYXLlVgfW7eNOZ7\n\tMnng==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=lzUWVM6W6hGAGXjqQuMgS9sGP8CKkiLhTkecfOb91HQ=;\n\tb=O5g4XjdaKkN2RQYTUUjJCAuplT+MqaUNiSv+YDu1Wa63P7m6tLTrN1hbhxjaKgjwTK\n\tXN3+FHRQvpm7/DL3p0zoQnO/CWIURc/AYg5xLd2ZyrkczzuNOMg7ffm/kxdMzB5akgTo\n\tNyk9/G6XABiK2c6qaityJZ6MUcF30avmOa9xhKQsgpGtsncNZFq7bvBnLUfQrBuXWsiu\n\tfVJTzrTey0jL+Yln/6vcZave2soABLZPb1EzoibAWfRpEydHewJY1R4C4Mk22Q8D5cgg\n\ta9hVhBR14PGY1Jj7vLgMTcahN5ADjPfPU47ElqCUrzQmiybpdkoOEXNLElAnK0zmXs6S\n\tMMmQ==","X-Gm-Message-State":"APjAAAXURmZmDVOt9rhREjxvlJ56I0wl6Em+hIzpqxb7WhXPSN3iTMSU\n\t12jwnfW4pKfLwNmT++4QHVsCHS1LlNY=","X-Google-Smtp-Source":"APXvYqzbY3Ecfe+qbyuC+mobtP3BfUitsGc2ZlvENKyCf5oQJUSRHQIx7soXiZ83S7MK1oq7bvudSA==","X-Received":"by 2002:a2e:4c02:: with SMTP id z2mr4765282lja.177.1567070309924;\n\tThu, 29 Aug 2019 02:18:29 -0700 (PDT)","Date":"Thu, 29 Aug 2019 11:18:28 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190829091828.GX28351@bigcity.dyn.berto.se>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-6-jacopo@jmondi.org>\n\t<20190828112758.GP28351@bigcity.dyn.berto.se>\n\t<20190828163101.eojh5kea5t32tppy@uno.localdomain>\n\t<20190829082947.GU28351@bigcity.dyn.berto.se>\n\t<20190829090334.6mrcwsugiasnamia@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190829090334.6mrcwsugiasnamia@uno.localdomain>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor:\n\tCollect camera properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 29 Aug 2019 09:18:30 -0000"}},{"id":2574,"web_url":"https://patchwork.libcamera.org/comment/2574/","msgid":"<20190903184144.GC5270@pendragon.ideasonboard.com>","date":"2019-09-03T20:17:25","subject":"Re: [libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor:\n\tCollect camera properties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Thu, Aug 29, 2019 at 11:18:28AM +0200, Niklas Söderlund wrote:\n> On 2019-08-29 11:03:34 +0200, Jacopo Mondi wrote:\n> > On Thu, Aug 29, 2019 at 10:29:47AM +0200, Niklas Söderlund wrote:\n> >> On 2019-08-28 18:31:01 +0200, Jacopo Mondi wrote:\n> >>> On Wed, Aug 28, 2019 at 01:27:58PM +0200, Niklas Söderlund wrote:\n> >>>> On 2019-08-27 11:50:05 +0200, Jacopo Mondi wrote:\n> >>>>> Store the camera sensor location and rotation by parsing the\n> >>>>> associated V4L2 controls.\n> >>>>>\n> >>>>> Signed-off-by: Jacopo Mondi \n> >>>>> ---\n> >>>>> src/libcamera/camera_sensor.cpp | 39 +++++++++++++++++++++++++++\n> >>>>> src/libcamera/include/camera_sensor.h | 4 +++\n> >>>>> 2 files changed, 43 insertions(+)\n> >>>>>\n> >>>>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> >>>>> index a7670b449b31..fc7fdcdcaf5b 100644\n> >>>>> --- a/src/libcamera/camera_sensor.cpp\n> >>>>> +++ b/src/libcamera/camera_sensor.cpp\n> >>>>> @@ -89,6 +89,45 @@ int CameraSensor::init()\n> >>>>> \tif (ret < 0)\n> >>>>> \t\treturn ret;\n> >>>>>\n> >>>>> +\t/* Retrieve and store the camera sensor properties. */\n> >>>>> +\tV4L2ControlList controls({\n> >>>>> +\t\tV4L2_CID_CAMERA_SENSOR_LOCATION,\n> >>>>> +\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> >>>>> +\t});\n> >>>>> +\tret = subdev_->getControls(&controls);\n> >>>>> +\tif (ret) {\n> >>>>> +\t\tLOG(CameraSensor, Error)\n> >>>>> +\t\t\t<< \"Failed to get camera sensor controls: \" << ret;\n> >>>>> +\t\treturn ret;\n> >>>>> +\t}\n> >>>>\n> >>>> I still feel this is a bit harsh for the moment. If the sensor don't\n> >>>> support V4L2_CID_CAMERA_SENSOR_LOCATION and\n> >>>> V4L2_CID_CAMERA_SENSOR_ROTATION this will fail and the camera is not\n> >>>> registered with the system. I see two problems with this,\n> >>>>\n> >>>> 1. If the camera location is EXTERNAL is rotation really a mandatory\n> >>>> property?\n> >>>>\n> >>>> 2. I agree that in the future when the kernel support for\n> >>>> SENSOR_LOCATION have taken hold we can make it mandatory. For now\n> >>>> would it not be a good idea to set location to EXTERNAL if it's not\n> >>>> present and print a warning?\n> >>>>\n> >>>> I fear we will block this series for a very long time without easing\n> >>>> the constraints for a while.\n> >>>>\n> >>>\n> >>> Ideally we should have all of this properties set.\n> >>\n> >> So rotation should be a mandatory property for external cameras? How do\n> >> you see that being implemented pluggable USB cameras such as UVC?\n> > \n> > Set to 0? I don't know why one should produce a USB camera that works\n> > upside down, but hey, the world is a strange place isn't it?\n> \n> I agree it should be set to 0 inside libcamera, but do we wish for the \n> V4L2_CID_CAMERA_SENSOR_ROTATION control to be made mandatory for \n> EXTERNAL cameras? In my view we could look for the rotation control on \n> external cameras an use the value if it exists, else default to 0. While \n> on FRONT/BACK cameras we could error out if the control is not \n> supported.\n\nUVC doesn't use the CameraSensor class... I expect other external\ncameras not to use it either. Note that several laptops have an internal\nUVC camera that is mounted upside-down, so we'll have to handle this at\nsome point.\n\n> > I'm fine accepting to default it to 0 for external cameras, but for\n> > internal ones, I think this should be mandatory. Question is, it is\n> > not mandatory on kernel side, do we want libcamera to enforce it? I\n> > don't think forcing it to be mandatory on kernel side will go much far\n> > (probably rightfully).\n> \n> For internal cameras I think libcamera should make the two controls \n> mandatory. But only once the upstream Linux support is available. I \n> share your view that making it mandatory in Linux might be a bit far \n> fetched.\n\nI actually think it should be mandatory in Linux, as well as in\nlibcamera. New sensor drivers should always have those properties, and\nexisting drivers should get support for them when used on platform\nsupported by libcamera.\n\nI however think they should be treated as optional in libcamera until\nthe kernel patches land upstream. We then have two options, either\ndelaying the merge of this series, or changing the code. If we change it\n(which I think would be the best option), should we keep this patch\nas-is and add a patch on top to make the controls optional, so we can\nlater just revert that patch ?\n\n> >>> It's not a big work\n> >>> to implement them, and I fear if we don't enforce them, people will\n> >>> just ignore them, but I would rather defer policies decisions like this\n> >>> one to Laurent...\n> >>\n> >> I agree that we should enforce it down the line, but for now I think we\n> >> need to either hold of on this series until upstream Linux support is\n> >> available for all our sensors used in our \"supported\" platforms. Or\n> >> relax the constraint to a warning, merge this series in libcamera and\n> >> once upstream Liunx support is available change the warning to an error.\n> >>\n> >> I'm not super happy to be forced to run kernels with out of tree code\n> >> for libcamera to function :-)\n> > \n> > Hopefully that code (at least for the devices we support) will get in\n> > sooner or later, so I'm not too concerned for the devices we have, and\n> > I think people wanting to run libcamera on their device should be\n> > 'encouraged' to make their devices kernel interface as compliant as\n> > possible. How bad of a showstopper would this be I cannot tell, that's\n> > why I'm happy to defer the decision here to the group :)\n> \n> I share your view, that the code hopefully will soon be upstream, but I \n> don't think we should depend or fail cameras until it's present in an \n> real upstream release.\n> \n> >>>>> +\n> >>>>> +\tV4L2Control *control = controls[V4L2_CID_CAMERA_SENSOR_LOCATION];\n> >>>>> +\tint64_t value = control->value();\n> >>>>> +\tswitch (value) {\n> >>>>> +\tcase V4L2_LOCATION_EXTERNAL:\n> >>>>> +\t\tlocation_ = CAMERA_LOCATION_EXTERNAL;\n> >>>>> +\t\tbreak;\n> >>>>> +\tcase V4L2_LOCATION_FRONT:\n> >>>>> +\t\tlocation_ = CAMERA_LOCATION_FRONT;\n> >>>>> +\t\tbreak;\n> >>>>> +\tcase V4L2_LOCATION_BACK:\n> >>>>> +\t\tlocation_ = CAMERA_LOCATION_BACK;\n> >>>>> +\t\tbreak;\n> >>>>> +\tdefault:\n> >>>>> +\t\tLOG(CameraSensor, Error)\n> >>>>> +\t\t\t<< \"Unsupported camera location: \" << value;\n> >>>>> +\t\treturn -EINVAL;\n> >>>>> +\t}\n> >>>>> +\n> >>>>> +\tcontrol = controls[V4L2_CID_CAMERA_SENSOR_ROTATION];\n> >>>>> +\tvalue = control->value();\n> >>>>> +\tif (value < 0 || value > 360) {\n\n360 is equivalent to 0, so I would change this to value < 0 || value >=\n360. Should we reject rotations other than 0, 90, 180 and 270 ?\n\n> >>>>> +\t\tLOG(CameraSensor, Error)\n> >>>>> +\t\t\t<< \"Unsupported camera rotation: \" << value;\n> >>>>> +\t\treturn -EINVAL;\n> >>>>> +\t}\n> >>>>> +\trotation_ = value;\n> >>>>> +\n> >>>>> \t/* Enumerate and cache media bus codes and sizes. */\n> >>>>> \tconst ImageFormats formats = subdev_->formats(0);\n> >>>>> \tif (formats.isEmpty()) {\n> >>>>> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> >>>>> index fe033fb374c1..32d39127b275 100644\n> >>>>> --- a/src/libcamera/include/camera_sensor.h\n> >>>>> +++ b/src/libcamera/include/camera_sensor.h\n> >>>>> @@ -10,6 +10,7 @@\n> >>>>> #include \n> >>>>> #include \n> >>>>>\n> >>>>> +#include \n> >>>>> #include \n> >>>>>\n> >>>>> #include \"log.h\"\n> >>>>> @@ -55,6 +56,9 @@ private:\n> >>>>>\n> >>>>> \tstd::vector mbusCodes_;\n> >>>>> \tstd::vector sizes_;\n> >>>>> +\n> >>>>> +\tCameraLocation location_;\n> >>>>> +\tunsigned int rotation_;\n\nShould we store this in a list of properties already ?\n\n> >>>>> };\n> >>>>>\n> >>>>> } /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B892760C18\n\tfor ;\n\tTue, 3 Sep 2019 22:17:32 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-18-41-nat.elisa-mobile.fi\n\t[85.76.18.41])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3A92B542;\n\tTue, 3 Sep 2019 22:17:32 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1567541852;\n\tbh=1uTyZ3BRxM59yPeLHGz1zbtQFSSCvUqBxvyMuN6P0v8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TgxU918BGQTgKA0zofHnDTEAtrrbXQMKrALm92JAcsP5gy3lH7b/WiDHDCUVvuYUP\n\tMz9Ddm6Mrdkj86RmP1ONkbNz+9aKKy9PVZ/5B5nxFmYfPhvPh7W5pjvWZBW01I9bS+\n\tY9CetpjcAZL9NMOfT7E/NMw06+0ROjmbPR/1kkWk=","Date":"Tue, 3 Sep 2019 23:17:25 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20190903184144.GC5270@pendragon.ideasonboard.com>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-6-jacopo@jmondi.org>\n\t<20190828112758.GP28351@bigcity.dyn.berto.se>\n\t<20190828163101.eojh5kea5t32tppy@uno.localdomain>\n\t<20190829082947.GU28351@bigcity.dyn.berto.se>\n\t<20190829090334.6mrcwsugiasnamia@uno.localdomain>\n\t<20190829091828.GX28351@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190829091828.GX28351@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor:\n\tCollect camera properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 03 Sep 2019 20:17:32 -0000"}}]