[{"id":2455,"web_url":"https://patchwork.libcamera.org/comment/2455/","msgid":"<20190817155717.GD22846@wyvern>","date":"2019-08-17T15:57:17","subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: camera_sensor: Store\n\tthe camera location","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-17 12:59:35 +0200, Jacopo Mondi wrote:\n> Store the camera sensor location by parsing the\n> V4L2_CID_CAMERA_SENSOR_LOCATION V4L2 control.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/camera_sensor.cpp | 25 +++++++++++++++++++++++++\n> src/libcamera/include/camera_sensor.h | 3 +++\n> 2 files changed, 28 insertions(+)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index a7670b449b31..2703d10c719e 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -89,6 +89,31 @@ int CameraSensor::init()\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> +\t/* Retrieve and store the camera sensor location. */\n> +\tV4L2ControlList controls;\n> +\tcontrols.add(V4L2_CID_CAMERA_SENSOR_LOCATION);\n> +\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> +\tV4L2Control *locationControl = controls[V4L2_CID_CAMERA_SENSOR_LOCATION];\n> +\tint64_t v4l2Location = locationControl->value();\n\nI would skip the temporary variable here, but it's just bikeshedding ;-)\n\n> +\tswitch (v4l2Location) {\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\nShould we not handle V4L2_LOCATION_EXTERNAL also?\n\n> +\tdefault:\n> +\t\tLOG(CameraSensor, Error) << \"Unsupported camera location\";\n> +\t\treturn -EINVAL;\n\nIs it not a bit harsh to fail if location information is not available? \nIs it possible to set it to V4L2_LOCATION_UNKOWN and move on. For the \nAndriod HAL we could map UNKWON to EXTERNAL and print a warning, or if \nwe require location information at that point skip camers which do not \nprovided it.\n\n> +\t}\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..a237a1684605 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,8 @@ private:\n> \n> \tstd::vector mbusCodes_;\n> \tstd::vector sizes_;\n> +\n> +\tCameraLocation location_;\n> };\n> \n> } /* namespace libcamera */\n> -- \n> 2.22.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-ed1-x544.google.com (mail-ed1-x544.google.com\n\t[IPv6:2a00:1450:4864:20::544])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 45667600F9\n\tfor ;\n\tSat, 17 Aug 2019 17:57:19 +0200 (CEST)","by mail-ed1-x544.google.com with SMTP id w5so7595347edl.8\n\tfor ;\n\tSat, 17 Aug 2019 08:57:19 -0700 (PDT)","from localhost ([185.224.57.161]) by smtp.gmail.com with ESMTPSA id\n\tva28sm1264155ejb.36.2019.08.17.08.57.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 17 Aug 2019 08:57:18 -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=GvwPTDzTae8dKxhlDhVOztMGOwIg6Y7uWYOMUr4TO+w=;\n\tb=FaLcHjS6Ln/o712GfVdXirdUpz15Ae/cDHUlkFpheqNbqfa2iRusDNxvYRfLhtT4TH\n\tQb6qVGvbHxdsLLGVQj4t/ibNp3TOTEeq+oOCaPu6aWKRQan7T8q22d/GJGMlK72u0aqo\n\tAttf06tDHpobZYpnAWBszjntm7oRgKtbT5FvlLSwOdOT2eKGtc4Ydex+U9h9HH2oAIXg\n\tA2wEHWQ619uejCkz2qu4YrblQ6hagUfPdZ+t0y8RnHQeyw7iiZCaCe8iy3IvETezZbT4\n\tttIKQAt1CorO27cWsWAJoG8uaSFhVpdqQVpoE1JMhvXGEL/z/0X5lkIXOpmtPtGIAovS\n\tphsA==","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=GvwPTDzTae8dKxhlDhVOztMGOwIg6Y7uWYOMUr4TO+w=;\n\tb=Z0Zz1rdjQtWHZ4b4BMPnFxQNshsexmS7wJBOT5tHhITOZvtBFsEjrd8gGy+tSN4vWN\n\tctmtC1e21tfzyebVWCNKz0oTFc1ztp5FRHjgmW2wJD8sFK80tYO/XE6nfKuClI87q/Zx\n\tsPkIdKYfwAtQ1NYoL7xvIqosb+ayh1EIYr6NVd2nUQlHzbsYiBWwKgEHytDyLPKPeWLm\n\tLpzQfcz7d6TNG9LUzxmoKyn4rgrYdDCovzm8rlhI5ttlkjxVZIgzh/8OShThxYAKO5Di\n\tTKRPqUROEVSG5QnGE8236iNG2cE25sSCpMLiwOogwDV6jdrXlaY0mSQrY90tqdZuZn4Q\n\tStqA==","X-Gm-Message-State":"APjAAAUxKA4+bOZNbbFKqrHVtsbY66UaCstSsR+QmP7/HK0XR9Aq2YdO\n\t78fVK6JeJkMGdd3onCm3FFwqbUjCwSA=","X-Google-Smtp-Source":"APXvYqzJYHPTLkAwRcvueVyzU6/EkEL3odHFDInfBEbMON3Vrc9iaTajbAd8BxN7pUwIR0mfLswoyw==","X-Received":"by 2002:a50:f315:: with SMTP id\n\tp21mr17077395edm.195.1566057438998; \n\tSat, 17 Aug 2019 08:57:18 -0700 (PDT)","Date":"Sat, 17 Aug 2019 17:57:17 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190817155717.GD22846@wyvern>","References":"<20190817105937.29353-1-jacopo@jmondi.org>\n\t<20190817105937.29353-4-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":"<20190817105937.29353-4-jacopo@jmondi.org>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: camera_sensor: Store\n\tthe camera location","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":"Sat, 17 Aug 2019 15:57:19 -0000"}},{"id":2457,"web_url":"https://patchwork.libcamera.org/comment/2457/","msgid":"<20190817160411.GE15630@pendragon.ideasonboard.com>","date":"2019-08-17T16:04:11","subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: camera_sensor: Store\n\tthe camera location","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Sat, Aug 17, 2019 at 05:57:17PM +0200, Niklas Söderlund wrote:\n> On 2019-08-17 12:59:35 +0200, Jacopo Mondi wrote:\n> > Store the camera sensor location by parsing the\n> > V4L2_CID_CAMERA_SENSOR_LOCATION V4L2 control.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/camera_sensor.cpp | 25 +++++++++++++++++++++++++\n> > src/libcamera/include/camera_sensor.h | 3 +++\n> > 2 files changed, 28 insertions(+)\n> > \n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index a7670b449b31..2703d10c719e 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -89,6 +89,31 @@ int CameraSensor::init()\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> > \n> > +\t/* Retrieve and store the camera sensor location. */\n> > +\tV4L2ControlList controls;\n> > +\tcontrols.add(V4L2_CID_CAMERA_SENSOR_LOCATION);\n\nUnrelated to this patch, would it make sense to have a V4L2ControlList\nconstructor that would take a list of control ids ?\n\n\tV4L2ControlList controls{ V4L2_CID_CAMERA_SENSOR_LOCATION };\n\tret = subdev_->getControls(&controls);\n\t...\n\n> > +\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> > +\tV4L2Control *locationControl = controls[V4L2_CID_CAMERA_SENSOR_LOCATION];\n> > +\tint64_t v4l2Location = locationControl->value();\n> \n> I would skip the temporary variable here, but it's just bikeshedding ;-)\n> \n> > +\tswitch (v4l2Location) {\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> \n> Should we not handle V4L2_LOCATION_EXTERNAL also?\n> \n> > +\tdefault:\n> > +\t\tLOG(CameraSensor, Error) << \"Unsupported camera location\";\n> > +\t\treturn -EINVAL;\n> \n> Is it not a bit harsh to fail if location information is not available? \n> Is it possible to set it to V4L2_LOCATION_UNKOWN and move on. For the \n> Andriod HAL we could map UNKWON to EXTERNAL and print a warning, or if \n> we require location information at that point skip camers which do not \n> provided it.\n\nThat's one option, but I also like the idea of libcamera pushing for the\nkernel drivers to implement the required APIs :-)\n\n> > +\t}\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..a237a1684605 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,8 @@ private:\n> > \n> > \tstd::vector mbusCodes_;\n> > \tstd::vector sizes_;\n> > +\n> > +\tCameraLocation location_;\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 B1565600F9\n\tfor ;\n\tSat, 17 Aug 2019 18:04:16 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 13813556;\n\tSat, 17 Aug 2019 18:04:15 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1566057856;\n\tbh=n+EcnbvJfkA1cC5ODiBWY1QC742wPXibwYlQNCFsrmA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tT6a8LdmLeZFOu+Kmxg0frMUuiGx8CKJLjlJLwygC6N0ky5FZ4b/djtOkXr5/spVk\n\th1iQ93S7UW/BpGphgrN5GI2vv1YESQQiGM3m8g4eQ+DAjJ/AekKjICG2FYX3RpjOWw\n\tHhkte1Rwwl0ee1HNe694GcAZYIdhdgaauKUa/9Jc=","Date":"Sat, 17 Aug 2019 19:04:11 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190817160411.GE15630@pendragon.ideasonboard.com>","References":"<20190817105937.29353-1-jacopo@jmondi.org>\n\t<20190817105937.29353-4-jacopo@jmondi.org>\n\t<20190817155717.GD22846@wyvern>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190817155717.GD22846@wyvern>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: camera_sensor: Store\n\tthe camera location","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":"Sat, 17 Aug 2019 16:04:16 -0000"}},{"id":2465,"web_url":"https://patchwork.libcamera.org/comment/2465/","msgid":"<20190819073236.24kih2nkfghp3xuc@uno.localdomain>","date":"2019-08-19T07:32:36","subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: camera_sensor: Store\n\tthe camera location","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Aug 17, 2019 at 07:04:11PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Sat, Aug 17, 2019 at 05:57:17PM +0200, Niklas Söderlund wrote:\n> > On 2019-08-17 12:59:35 +0200, Jacopo Mondi wrote:\n> > > Store the camera sensor location by parsing the\n> > > V4L2_CID_CAMERA_SENSOR_LOCATION V4L2 control.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/camera_sensor.cpp | 25 +++++++++++++++++++++++++\n> > > src/libcamera/include/camera_sensor.h | 3 +++\n> > > 2 files changed, 28 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index a7670b449b31..2703d10c719e 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -89,6 +89,31 @@ int CameraSensor::init()\n> > > \tif (ret < 0)\n> > > \t\treturn ret;\n> > >\n> > > +\t/* Retrieve and store the camera sensor location. */\n> > > +\tV4L2ControlList controls;\n> > > +\tcontrols.add(V4L2_CID_CAMERA_SENSOR_LOCATION);\n>\n> Unrelated to this patch, would it make sense to have a V4L2ControlList\n> constructor that would take a list of control ids ?\n>\n> \tV4L2ControlList controls{ V4L2_CID_CAMERA_SENSOR_LOCATION };\n> \tret = subdev_->getControls(&controls);\n> \t...\n>\n\nIt does and should not be hard to implement...\n\n> > > +\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> > > +\tV4L2Control *locationControl = controls[V4L2_CID_CAMERA_SENSOR_LOCATION];\n> > > +\tint64_t v4l2Location = locationControl->value();\n> >\n> > I would skip the temporary variable here, but it's just bikeshedding ;-)\n> >\n> > > +\tswitch (v4l2Location) {\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> >\n> > Should we not handle V4L2_LOCATION_EXTERNAL also?\n> >\n> > > +\tdefault:\n> > > +\t\tLOG(CameraSensor, Error) << \"Unsupported camera location\";\n> > > +\t\treturn -EINVAL;\n> >\n> > Is it not a bit harsh to fail if location information is not available?\n> > Is it possible to set it to V4L2_LOCATION_UNKOWN and move on. For the\n> > Andriod HAL we could map UNKWON to EXTERNAL and print a warning, or if\n> > we require location information at that point skip camers which do not\n> > provided it.\n>\n> That's one option, but I also like the idea of libcamera pushing for the\n> kernel drivers to implement the required APIs :-)\n>\n\nThat was my thinking, we should push drivers to use the APIs we need,\nbut I agree it might be a bit harsh... However I'm not sure what we\nshould default this property to to be honest...\n\n> > > +\t}\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..a237a1684605 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,8 @@ private:\n> > >\n> > > \tstd::vector mbusCodes_;\n> > > \tstd::vector sizes_;\n> > > +\n> > > +\tCameraLocation location_;\n> > > };\n> > >\n> > > } /* namespace libcamera */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5F44160E38\n\tfor ;\n\tMon, 19 Aug 2019 09:31:08 +0200 (CEST)","from uno.localdomain (unknown [87.18.63.98])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 50DE4C000D;\n\tMon, 19 Aug 2019 07:31:07 +0000 (UTC)"],"X-Originating-IP":"87.18.63.98","Date":"Mon, 19 Aug 2019 09:32:36 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190819073236.24kih2nkfghp3xuc@uno.localdomain>","References":"<20190817105937.29353-1-jacopo@jmondi.org>\n\t<20190817105937.29353-4-jacopo@jmondi.org>\n\t<20190817155717.GD22846@wyvern>\n\t<20190817160411.GE15630@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"55lsdhscg46qognz\"","Content-Disposition":"inline","In-Reply-To":"<20190817160411.GE15630@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: camera_sensor: Store\n\tthe camera location","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":"Mon, 19 Aug 2019 07:31:08 -0000"}},{"id":2483,"web_url":"https://patchwork.libcamera.org/comment/2483/","msgid":"<20190819132657.GD5011@pendragon.ideasonboard.com>","date":"2019-08-19T13:26:57","subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: camera_sensor: Store\n\tthe camera location","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Aug 19, 2019 at 09:32:36AM +0200, Jacopo Mondi wrote:\n> On Sat, Aug 17, 2019 at 07:04:11PM +0300, Laurent Pinchart wrote:\n> > On Sat, Aug 17, 2019 at 05:57:17PM +0200, Niklas Söderlund wrote:\n> >> On 2019-08-17 12:59:35 +0200, Jacopo Mondi wrote:\n> >>> Store the camera sensor location by parsing the\n> >>> V4L2_CID_CAMERA_SENSOR_LOCATION V4L2 control.\n> >>>\n> >>> Signed-off-by: Jacopo Mondi \n> >>> ---\n> >>> src/libcamera/camera_sensor.cpp | 25 +++++++++++++++++++++++++\n> >>> src/libcamera/include/camera_sensor.h | 3 +++\n> >>> 2 files changed, 28 insertions(+)\n> >>>\n> >>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> >>> index a7670b449b31..2703d10c719e 100644\n> >>> --- a/src/libcamera/camera_sensor.cpp\n> >>> +++ b/src/libcamera/camera_sensor.cpp\n> >>> @@ -89,6 +89,31 @@ int CameraSensor::init()\n> >>> \tif (ret < 0)\n> >>> \t\treturn ret;\n> >>>\n> >>> +\t/* Retrieve and store the camera sensor location. */\n> >>> +\tV4L2ControlList controls;\n> >>> +\tcontrols.add(V4L2_CID_CAMERA_SENSOR_LOCATION);\n> >\n> > Unrelated to this patch, would it make sense to have a V4L2ControlList\n> > constructor that would take a list of control ids ?\n> >\n> > \tV4L2ControlList controls{ V4L2_CID_CAMERA_SENSOR_LOCATION };\n> > \tret = subdev_->getControls(&controls);\n> > \t...\n> \n> It does and should not be hard to implement...\n> \n> >>> +\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> >>> +\tV4L2Control *locationControl = controls[V4L2_CID_CAMERA_SENSOR_LOCATION];\n> >>> +\tint64_t v4l2Location = locationControl->value();\n> >>\n> >> I would skip the temporary variable here, but it's just bikeshedding ;-)\n> >>\n> >>> +\tswitch (v4l2Location) {\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> >>\n> >> Should we not handle V4L2_LOCATION_EXTERNAL also?\n> >>\n> >>> +\tdefault:\n> >>> +\t\tLOG(CameraSensor, Error) << \"Unsupported camera location\";\n> >>> +\t\treturn -EINVAL;\n> >>\n> >> Is it not a bit harsh to fail if location information is not available?\n> >> Is it possible to set it to V4L2_LOCATION_UNKOWN and move on. For the\n> >> Andriod HAL we could map UNKWON to EXTERNAL and print a warning, or if\n> >> we require location information at that point skip camers which do not\n> >> provided it.\n> >\n> > That's one option, but I also like the idea of libcamera pushing for the\n> > kernel drivers to implement the required APIs :-)\n> \n> That was my thinking, we should push drivers to use the APIs we need,\n> but I agree it might be a bit harsh... However I'm not sure what we\n> should default this property to to be honest...\n\nIf we require kernel drivers to implement that control, we don't need to\npick a default.\n\n> >>> +\t}\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..a237a1684605 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,8 @@ private:\n> >>>\n> >>> \tstd::vector mbusCodes_;\n> >>> \tstd::vector sizes_;\n> >>> +\n> >>> +\tCameraLocation location_;\n> >>> };\n> >>>\n> >>> } /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C77B60C1E\n\tfor ;\n\tMon, 19 Aug 2019 15:27:03 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E5D55510;\n\tMon, 19 Aug 2019 15:27:02 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1566221223;\n\tbh=RRFS3+/chbvHCnQAT5P3H3nQJlWYExtzm1J09vqTuw0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l5X48JcJQ1+hynF+WrPjOCeMjimxEYxThAON6iu0I7QVLWDQcci7haCvTtDmMSTMv\n\tu5LlailoY37rCRVmzxGcHmaxEuW2X8kcXlooNDF/TqhH2Q2YiifZAYVbODZYk4akJA\n\tpjyVI7XWUPpKdGI7o6ZSBnbM0NGcC6TbJHtlS74Q=","Date":"Mon, 19 Aug 2019 16:26:57 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190819132657.GD5011@pendragon.ideasonboard.com>","References":"<20190817105937.29353-1-jacopo@jmondi.org>\n\t<20190817105937.29353-4-jacopo@jmondi.org>\n\t<20190817155717.GD22846@wyvern>\n\t<20190817160411.GE15630@pendragon.ideasonboard.com>\n\t<20190819073236.24kih2nkfghp3xuc@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190819073236.24kih2nkfghp3xuc@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: camera_sensor: Store\n\tthe camera location","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":"Mon, 19 Aug 2019 13:27:03 -0000"}}]