[{"id":14369,"web_url":"https://patchwork.libcamera.org/comment/14369/","msgid":"<20201228082657.GF1933@pyrite.rasen.tech>","date":"2020-12-28T08:26:57","subject":"Re: [libcamera-devel] [PATCH 2/5] libcamera: camera_sensor:\n\tInitialize controls","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Dec 23, 2020 at 07:45:13PM +0100, Jacopo Mondi wrote:\n> Initialize the ControlInfoMap of sensor available controls by\n> reporting the sensor exposure time range.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/camera_sensor.h | 2 ++\n> src/libcamera/camera_sensor.cpp | 42 +++++++++++++++++++++-\n> 2 files changed, 43 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index aee10aa6e3c7..0357b2a630f7 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -71,6 +71,7 @@ private:\n> \tint generateId();\n> \tint validateSensorDriver();\n> \tint initProperties();\n> +\tint initControls();\n> \n> \tconst MediaEntity *entity_;\n> \tstd::unique_ptr subdev_;\n> @@ -85,6 +86,7 @@ private:\n> \tstd::vector sizes_;\n> \n> \tControlList properties_;\n> +\tControlInfoMap controls_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 3a65ac3de5bc..609f948c56a6 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -15,6 +15,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> \n> #include \"libcamera/internal/bayer_format.h\"\n> @@ -215,7 +216,7 @@ int CameraSensor::init()\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\treturn 0;\n> +\treturn initControls();\n> }\n> \n> int CameraSensor::validateSensorDriver()\n> @@ -229,6 +230,7 @@ int CameraSensor::validateSensorDriver()\n> \tconst std::vector mandatoryControls{\n> \t\tV4L2_CID_PIXEL_RATE,\n> \t\tV4L2_CID_HBLANK,\n> +\t\tV4L2_CID_EXPOSURE,\n> \t};\n> \n> \tControlList ctrls = subdev_->getControls(mandatoryControls);\n> @@ -430,6 +432,44 @@ int CameraSensor::initProperties()\n> \treturn 0;\n> }\n> \n> +int CameraSensor::initControls()\n> +{\n> +\tconst ControlInfoMap &v4l2Controls = subdev_->controls();\n> +\n> +\t/* Exposure time limits expressed in micro-seconds. */\n> +\n> +\t/* Calculate the line length in pixels. */\n> +\tControlList ctrls = subdev_->getControls({ V4L2_CID_HBLANK });\n\nI see that the presence of this control is declared as mandatory in\nvalidateSensorDriver() in 1/5, but it just returns -EINVAL and\ncontinues; would that cause a problem here?\n\n> +\tint32_t hblank = ctrls.get(V4L2_CID_HBLANK).get();\n\nWould it just make this zero and continue? And the user has been warned\nof unexpected behavior in the validation step already.\n\n> +\tV4L2SubdeviceFormat format{};\n> +\tint ret = subdev_->getFormat(pad_, &format);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\tint32_t ppl = format.size.width + hblank;\n> +\n> +\tconst ControlInfo &v4l2ExposureInfo = v4l2Controls.find(V4L2_CID_EXPOSURE)->second;\n> +\tint32_t minExposurePixels = v4l2ExposureInfo.min().get() * ppl;\n> +\tint32_t maxExposurePixels = v4l2ExposureInfo.max().get() * ppl;\n> +\tint32_t defExposurePixels = v4l2ExposureInfo.max().get() * ppl;\n\ns/max/def/ ?\n\n\nPaul\n\n> +\n> +\t/* Get the pixel rate (in useconds) and calculate the exposure timings. */\n> +\tconst ControlInfo &pixelRateInfo = v4l2Controls.find(V4L2_CID_PIXEL_RATE)->second;\n> +\tfloat minPixelRate = pixelRateInfo.min().get() / 1e6f;\n> +\tfloat maxPixelRate = pixelRateInfo.max().get() / 1e6f;\n> +\tfloat defPixelRate = pixelRateInfo.def().get() / 1e6f;\n> +\n> +\tint32_t minExposure = static_cast(minExposurePixels / maxPixelRate);\n> +\tint32_t maxExposure = static_cast(maxExposurePixels / minPixelRate);\n> +\tint32_t defExposure = static_cast(defExposurePixels / defPixelRate);\n> +\n> +\tControlInfoMap::Map map;\n> +\tmap[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> +\t\t\t\t\t\t defExposure);\n> +\tcontrols_ = std::move(map);\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\fn CameraSensor::model()\n> * \\brief Retrieve the sensor model name\n> -- \n> 2.29.2\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 0DF57C0F1B\n\tfor ;\n\tMon, 28 Dec 2020 08:27:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8D1536159A;\n\tMon, 28 Dec 2020 09:27:06 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED2466031E\n\tfor ;\n\tMon, 28 Dec 2020 09:27:04 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4D80C25C;\n\tMon, 28 Dec 2020 09:27:03 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gmd3uztC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1609144024;\n\tbh=Wad6OwCYryhnL0STZ5u4UZVLNHcvaK0yEi/2AHkphTk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gmd3uztCcRQlyEykLfj9Vq1ouoVfndJV4As5z/8etCU834ZjjflU6shEweRWmCg91\n\tgxcK2D1IL1mTqiUoQfbGBwTMlyYXUrp2c68D8EqhtJ96qd/Da25NGJvSi5a3D/tGgY\n\tprEw1uqSE2XggOu5nn9c6hzyVzMFp/qwifaXA9fk=","Date":"Mon, 28 Dec 2020 17:26:57 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20201228082657.GF1933@pyrite.rasen.tech>","References":"<20201223184516.58791-1-jacopo@jmondi.org>\n\t<20201223184516.58791-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201223184516.58791-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 2/5] libcamera: camera_sensor:\n\tInitialize controls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14374,"web_url":"https://patchwork.libcamera.org/comment/14374/","msgid":"<20201228093925.bjh6o25pnha3qvdy@uno.localdomain>","date":"2020-12-28T09:39:25","subject":"Re: [libcamera-devel] [PATCH 2/5] libcamera: camera_sensor:\n\tInitialize controls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Mon, Dec 28, 2020 at 05:26:57PM +0900, paul.elder@ideasonboard.com wrote:\n> Hi Jacopo,\n>\n> On Wed, Dec 23, 2020 at 07:45:13PM +0100, Jacopo Mondi wrote:\n> > Initialize the ControlInfoMap of sensor available controls by\n> > reporting the sensor exposure time range.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/camera_sensor.h | 2 ++\n> > src/libcamera/camera_sensor.cpp | 42 +++++++++++++++++++++-\n> > 2 files changed, 43 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index aee10aa6e3c7..0357b2a630f7 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -71,6 +71,7 @@ private:\n> > \tint generateId();\n> > \tint validateSensorDriver();\n> > \tint initProperties();\n> > +\tint initControls();\n> >\n> > \tconst MediaEntity *entity_;\n> > \tstd::unique_ptr subdev_;\n> > @@ -85,6 +86,7 @@ private:\n> > \tstd::vector sizes_;\n> >\n> > \tControlList properties_;\n> > +\tControlInfoMap controls_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 3a65ac3de5bc..609f948c56a6 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -15,6 +15,7 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \n> >\n> > #include \"libcamera/internal/bayer_format.h\"\n> > @@ -215,7 +216,7 @@ int CameraSensor::init()\n> > \tif (ret)\n> > \t\treturn ret;\n> >\n> > -\treturn 0;\n> > +\treturn initControls();\n> > }\n> >\n> > int CameraSensor::validateSensorDriver()\n> > @@ -229,6 +230,7 @@ int CameraSensor::validateSensorDriver()\n> > \tconst std::vector mandatoryControls{\n> > \t\tV4L2_CID_PIXEL_RATE,\n> > \t\tV4L2_CID_HBLANK,\n> > +\t\tV4L2_CID_EXPOSURE,\n> > \t};\n> >\n> > \tControlList ctrls = subdev_->getControls(mandatoryControls);\n> > @@ -430,6 +432,44 @@ int CameraSensor::initProperties()\n> > \treturn 0;\n> > }\n> >\n> > +int CameraSensor::initControls()\n> > +{\n> > +\tconst ControlInfoMap &v4l2Controls = subdev_->controls();\n> > +\n> > +\t/* Exposure time limits expressed in micro-seconds. */\n> > +\n> > +\t/* Calculate the line length in pixels. */\n> > +\tControlList ctrls = subdev_->getControls({ V4L2_CID_HBLANK });\n>\n> I see that the presence of this control is declared as mandatory in\n> validateSensorDriver() in 1/5, but it just returns -EINVAL and\n> continues; would that cause a problem here?\n\nDo you mean 'validateSensorDriver()' returns -EINVAL ? In that case\nCameraSensor::init() bails out before calling this method. Or have I\nmis-understood your question ?\n\nOne thing I would add: reading a control might fail for other reason\nthan not being implemented in the driver. I should probably check for\n if (ctrl.empty())\nnonetheless\n\n>\n> > +\tint32_t hblank = ctrls.get(V4L2_CID_HBLANK).get();\n>\n> Would it just make this zero and continue? And the user has been warned\n> of unexpected behavior in the validation step already.\n>\n\nWe don't get here if validation fails if that's what you mean.\n\nAnd in case reading the control fails for unrelated reasons, I would\nfail instead of zeroing...\n\n> > +\tV4L2SubdeviceFormat format{};\n> > +\tint ret = subdev_->getFormat(pad_, &format);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\tint32_t ppl = format.size.width + hblank;\n> > +\n> > +\tconst ControlInfo &v4l2ExposureInfo = v4l2Controls.find(V4L2_CID_EXPOSURE)->second;\n> > +\tint32_t minExposurePixels = v4l2ExposureInfo.min().get() * ppl;\n> > +\tint32_t maxExposurePixels = v4l2ExposureInfo.max().get() * ppl;\n> > +\tint32_t defExposurePixels = v4l2ExposureInfo.max().get() * ppl;\n>\n> s/max/def/ ?\n\nGood catch!\n\nThanks\n j\n\n>\n>\n> Paul\n>\n> > +\n> > +\t/* Get the pixel rate (in useconds) and calculate the exposure timings. */\n> > +\tconst ControlInfo &pixelRateInfo = v4l2Controls.find(V4L2_CID_PIXEL_RATE)->second;\n> > +\tfloat minPixelRate = pixelRateInfo.min().get() / 1e6f;\n> > +\tfloat maxPixelRate = pixelRateInfo.max().get() / 1e6f;\n> > +\tfloat defPixelRate = pixelRateInfo.def().get() / 1e6f;\n> > +\n> > +\tint32_t minExposure = static_cast(minExposurePixels / maxPixelRate);\n> > +\tint32_t maxExposure = static_cast(maxExposurePixels / minPixelRate);\n> > +\tint32_t defExposure = static_cast(defExposurePixels / defPixelRate);\n> > +\n> > +\tControlInfoMap::Map map;\n> > +\tmap[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > +\t\t\t\t\t\t defExposure);\n> > +\tcontrols_ = std::move(map);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > /**\n> > * \\fn CameraSensor::model()\n> > * \\brief Retrieve the sensor model name\n> > --\n> > 2.29.2\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 C55FAC0F1A\n\tfor ;\n\tMon, 28 Dec 2020 09:39:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1FDC96159A;\n\tMon, 28 Dec 2020 10:39:14 +0100 (CET)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A10786031E\n\tfor ;\n\tMon, 28 Dec 2020 10:39:12 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 1B42A200009;\n\tMon, 28 Dec 2020 09:39:11 +0000 (UTC)"],"Date":"Mon, 28 Dec 2020 10:39:25 +0100","From":"Jacopo Mondi ","To":"paul.elder@ideasonboard.com","Message-ID":"<20201228093925.bjh6o25pnha3qvdy@uno.localdomain>","References":"<20201223184516.58791-1-jacopo@jmondi.org>\n\t<20201223184516.58791-3-jacopo@jmondi.org>\n\t<20201228082657.GF1933@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201228082657.GF1933@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH 2/5] libcamera: camera_sensor:\n\tInitialize controls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14378,"web_url":"https://patchwork.libcamera.org/comment/14378/","msgid":"<20201228101225.GA67590@pyrite.rasen.tech>","date":"2020-12-28T10:12:25","subject":"Re: [libcamera-devel] [PATCH 2/5] libcamera: camera_sensor:\n\tInitialize controls","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Dec 28, 2020 at 10:39:25AM +0100, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Mon, Dec 28, 2020 at 05:26:57PM +0900, paul.elder@ideasonboard.com wrote:\n> > Hi Jacopo,\n> >\n> > On Wed, Dec 23, 2020 at 07:45:13PM +0100, Jacopo Mondi wrote:\n> > > Initialize the ControlInfoMap of sensor available controls by\n> > > reporting the sensor exposure time range.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/internal/camera_sensor.h | 2 ++\n> > > src/libcamera/camera_sensor.cpp | 42 +++++++++++++++++++++-\n> > > 2 files changed, 43 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > index aee10aa6e3c7..0357b2a630f7 100644\n> > > --- a/include/libcamera/internal/camera_sensor.h\n> > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > @@ -71,6 +71,7 @@ private:\n> > > \tint generateId();\n> > > \tint validateSensorDriver();\n> > > \tint initProperties();\n> > > +\tint initControls();\n> > >\n> > > \tconst MediaEntity *entity_;\n> > > \tstd::unique_ptr subdev_;\n> > > @@ -85,6 +86,7 @@ private:\n> > > \tstd::vector sizes_;\n> > >\n> > > \tControlList properties_;\n> > > +\tControlInfoMap controls_;\n> > > };\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index 3a65ac3de5bc..609f948c56a6 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -15,6 +15,7 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > >\n> > > #include \"libcamera/internal/bayer_format.h\"\n> > > @@ -215,7 +216,7 @@ int CameraSensor::init()\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > >\n> > > -\treturn 0;\n> > > +\treturn initControls();\n> > > }\n> > >\n> > > int CameraSensor::validateSensorDriver()\n> > > @@ -229,6 +230,7 @@ int CameraSensor::validateSensorDriver()\n> > > \tconst std::vector mandatoryControls{\n> > > \t\tV4L2_CID_PIXEL_RATE,\n> > > \t\tV4L2_CID_HBLANK,\n> > > +\t\tV4L2_CID_EXPOSURE,\n> > > \t};\n> > >\n> > > \tControlList ctrls = subdev_->getControls(mandatoryControls);\n> > > @@ -430,6 +432,44 @@ int CameraSensor::initProperties()\n> > > \treturn 0;\n> > > }\n> > >\n> > > +int CameraSensor::initControls()\n> > > +{\n> > > +\tconst ControlInfoMap &v4l2Controls = subdev_->controls();\n> > > +\n> > > +\t/* Exposure time limits expressed in micro-seconds. */\n> > > +\n> > > +\t/* Calculate the line length in pixels. */\n> > > +\tControlList ctrls = subdev_->getControls({ V4L2_CID_HBLANK });\n> >\n> > I see that the presence of this control is declared as mandatory in\n> > validateSensorDriver() in 1/5, but it just returns -EINVAL and\n> > continues; would that cause a problem here?\n> \n> Do you mean 'validateSensorDriver()' returns -EINVAL ? In that case\n\nAh yeah, I mean in the case that the mandatory control is not present,\nso validateSensorDriver() returns -EINVAL.\n\n> CameraSensor::init() bails out before calling this method. Or have I\n\nWouldn't that mean that this method won't be called either if the\noptional controls aren't present in the driver?\n\n> mis-understood your question ?\n> \n> One thing I would add: reading a control might fail for other reason\n> than not being implemented in the driver. I should probably check for\n> if (ctrl.empty())\n> nonetheless\n> \n> >\n> > > +\tint32_t hblank = ctrls.get(V4L2_CID_HBLANK).get();\n> >\n> > Would it just make this zero and continue? And the user has been warned\n> > of unexpected behavior in the validation step already.\n> >\n> \n> We don't get here if validation fails if that's what you mean.\n\nI see.\n\n\nPaul\n\n> And in case reading the control fails for unrelated reasons, I would\n> fail instead of zeroing...\n> \n> > > +\tV4L2SubdeviceFormat format{};\n> > > +\tint ret = subdev_->getFormat(pad_, &format);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\tint32_t ppl = format.size.width + hblank;\n> > > +\n> > > +\tconst ControlInfo &v4l2ExposureInfo = v4l2Controls.find(V4L2_CID_EXPOSURE)->second;\n> > > +\tint32_t minExposurePixels = v4l2ExposureInfo.min().get() * ppl;\n> > > +\tint32_t maxExposurePixels = v4l2ExposureInfo.max().get() * ppl;\n> > > +\tint32_t defExposurePixels = v4l2ExposureInfo.max().get() * ppl;\n> >\n> > s/max/def/ ?\n> \n> Good catch!\n> \n> Thanks\n> j\n> \n> >\n> >\n> > Paul\n> >\n> > > +\n> > > +\t/* Get the pixel rate (in useconds) and calculate the exposure timings. */\n> > > +\tconst ControlInfo &pixelRateInfo = v4l2Controls.find(V4L2_CID_PIXEL_RATE)->second;\n> > > +\tfloat minPixelRate = pixelRateInfo.min().get() / 1e6f;\n> > > +\tfloat maxPixelRate = pixelRateInfo.max().get() / 1e6f;\n> > > +\tfloat defPixelRate = pixelRateInfo.def().get() / 1e6f;\n> > > +\n> > > +\tint32_t minExposure = static_cast(minExposurePixels / maxPixelRate);\n> > > +\tint32_t maxExposure = static_cast(maxExposurePixels / minPixelRate);\n> > > +\tint32_t defExposure = static_cast(defExposurePixels / defPixelRate);\n> > > +\n> > > +\tControlInfoMap::Map map;\n> > > +\tmap[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > > +\t\t\t\t\t\t defExposure);\n> > > +\tcontrols_ = std::move(map);\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > /**\n> > > * \\fn CameraSensor::model()\n> > > * \\brief Retrieve the sensor model name\n> > > --\n> > > 2.29.2\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel","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 E1D5CC0F1A\n\tfor ;\n\tMon, 28 Dec 2020 10:12:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B09E260526;\n\tMon, 28 Dec 2020 11:12:35 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 88A5360525\n\tfor ;\n\tMon, 28 Dec 2020 11:12:33 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 20307A39;\n\tMon, 28 Dec 2020 11:12:31 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Y1M/7iNA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1609150353;\n\tbh=7KW0/5btxKlHzNNCVxNV+hXJwAU+dXCxGwauNCIYSlw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Y1M/7iNAYWmGHdDRrhBWAVSlsalgKBc7Zddohmv/CKcg8lj/uHxc7+I9TBRBvKxuj\n\tOZCE4OG06DxbRf+hw6pzM99uRNPBtpUEX1VKfAvFw+NBVxUhhaRTv/LRxCffeMxb2Z\n\t4yYwtlyeWz89QW40shIkU8lc+lu9Llh+Mmafv6rk=","Date":"Mon, 28 Dec 2020 19:12:25 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20201228101225.GA67590@pyrite.rasen.tech>","References":"<20201223184516.58791-1-jacopo@jmondi.org>\n\t<20201223184516.58791-3-jacopo@jmondi.org>\n\t<20201228082657.GF1933@pyrite.rasen.tech>\n\t<20201228093925.bjh6o25pnha3qvdy@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201228093925.bjh6o25pnha3qvdy@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 2/5] libcamera: camera_sensor:\n\tInitialize controls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14379,"web_url":"https://patchwork.libcamera.org/comment/14379/","msgid":"<20201228101350.GB67590@pyrite.rasen.tech>","date":"2020-12-28T10:13:50","subject":"Re: [libcamera-devel] [PATCH 2/5] libcamera: camera_sensor:\n\tInitialize controls","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Mon, Dec 28, 2020 at 07:12:25PM +0900, paul.elder@ideasonboard.com wrote:\n> Hi Jacopo,\n> \n> On Mon, Dec 28, 2020 at 10:39:25AM +0100, Jacopo Mondi wrote:\n> > Hi Paul,\n> > \n> > On Mon, Dec 28, 2020 at 05:26:57PM +0900, paul.elder@ideasonboard.com wrote:\n> > > Hi Jacopo,\n> > >\n> > > On Wed, Dec 23, 2020 at 07:45:13PM +0100, Jacopo Mondi wrote:\n> > > > Initialize the ControlInfoMap of sensor available controls by\n> > > > reporting the sensor exposure time range.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > include/libcamera/internal/camera_sensor.h | 2 ++\n> > > > src/libcamera/camera_sensor.cpp | 42 +++++++++++++++++++++-\n> > > > 2 files changed, 43 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > > > index aee10aa6e3c7..0357b2a630f7 100644\n> > > > --- a/include/libcamera/internal/camera_sensor.h\n> > > > +++ b/include/libcamera/internal/camera_sensor.h\n> > > > @@ -71,6 +71,7 @@ private:\n> > > > \tint generateId();\n> > > > \tint validateSensorDriver();\n> > > > \tint initProperties();\n> > > > +\tint initControls();\n> > > >\n> > > > \tconst MediaEntity *entity_;\n> > > > \tstd::unique_ptr subdev_;\n> > > > @@ -85,6 +86,7 @@ private:\n> > > > \tstd::vector sizes_;\n> > > >\n> > > > \tControlList properties_;\n> > > > +\tControlInfoMap controls_;\n> > > > };\n> > > >\n> > > > } /* namespace libcamera */\n> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > index 3a65ac3de5bc..609f948c56a6 100644\n> > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > @@ -15,6 +15,7 @@\n> > > > #include \n> > > > #include \n> > > >\n> > > > +#include \n> > > > #include \n> > > >\n> > > > #include \"libcamera/internal/bayer_format.h\"\n> > > > @@ -215,7 +216,7 @@ int CameraSensor::init()\n> > > > \tif (ret)\n> > > > \t\treturn ret;\n> > > >\n> > > > -\treturn 0;\n> > > > +\treturn initControls();\n> > > > }\n> > > >\n> > > > int CameraSensor::validateSensorDriver()\n> > > > @@ -229,6 +230,7 @@ int CameraSensor::validateSensorDriver()\n> > > > \tconst std::vector mandatoryControls{\n> > > > \t\tV4L2_CID_PIXEL_RATE,\n> > > > \t\tV4L2_CID_HBLANK,\n> > > > +\t\tV4L2_CID_EXPOSURE,\n> > > > \t};\n> > > >\n> > > > \tControlList ctrls = subdev_->getControls(mandatoryControls);\n> > > > @@ -430,6 +432,44 @@ int CameraSensor::initProperties()\n> > > > \treturn 0;\n> > > > }\n> > > >\n> > > > +int CameraSensor::initControls()\n> > > > +{\n> > > > +\tconst ControlInfoMap &v4l2Controls = subdev_->controls();\n> > > > +\n> > > > +\t/* Exposure time limits expressed in micro-seconds. */\n> > > > +\n> > > > +\t/* Calculate the line length in pixels. */\n> > > > +\tControlList ctrls = subdev_->getControls({ V4L2_CID_HBLANK });\n> > >\n> > > I see that the presence of this control is declared as mandatory in\n> > > validateSensorDriver() in 1/5, but it just returns -EINVAL and\n> > > continues; would that cause a problem here?\n> > \n> > Do you mean 'validateSensorDriver()' returns -EINVAL ? In that case\n> \n> Ah yeah, I mean in the case that the mandatory control is not present,\n> so validateSensorDriver() returns -EINVAL.\n> \n> > CameraSensor::init() bails out before calling this method. Or have I\n> \n> Wouldn't that mean that this method won't be called either if the\n> optional controls aren't present in the driver?\n\nOh nvm, I misread the last patch.\n\n\nPaul\n\n> > mis-understood your question ?\n> > \n> > One thing I would add: reading a control might fail for other reason\n> > than not being implemented in the driver. I should probably check for\n> > if (ctrl.empty())\n> > nonetheless\n> > \n> > >\n> > > > +\tint32_t hblank = ctrls.get(V4L2_CID_HBLANK).get();\n> > >\n> > > Would it just make this zero and continue? And the user has been warned\n> > > of unexpected behavior in the validation step already.\n> > >\n> > \n> > We don't get here if validation fails if that's what you mean.\n> \n> I see.\n> \n> \n> Paul\n> \n> > And in case reading the control fails for unrelated reasons, I would\n> > fail instead of zeroing...\n> > \n> > > > +\tV4L2SubdeviceFormat format{};\n> > > > +\tint ret = subdev_->getFormat(pad_, &format);\n> > > > +\tif (ret)\n> > > > +\t\treturn ret;\n> > > > +\tint32_t ppl = format.size.width + hblank;\n> > > > +\n> > > > +\tconst ControlInfo &v4l2ExposureInfo = v4l2Controls.find(V4L2_CID_EXPOSURE)->second;\n> > > > +\tint32_t minExposurePixels = v4l2ExposureInfo.min().get() * ppl;\n> > > > +\tint32_t maxExposurePixels = v4l2ExposureInfo.max().get() * ppl;\n> > > > +\tint32_t defExposurePixels = v4l2ExposureInfo.max().get() * ppl;\n> > >\n> > > s/max/def/ ?\n> > \n> > Good catch!\n> > \n> > Thanks\n> > j\n> > \n> > >\n> > >\n> > > Paul\n> > >\n> > > > +\n> > > > +\t/* Get the pixel rate (in useconds) and calculate the exposure timings. */\n> > > > +\tconst ControlInfo &pixelRateInfo = v4l2Controls.find(V4L2_CID_PIXEL_RATE)->second;\n> > > > +\tfloat minPixelRate = pixelRateInfo.min().get() / 1e6f;\n> > > > +\tfloat maxPixelRate = pixelRateInfo.max().get() / 1e6f;\n> > > > +\tfloat defPixelRate = pixelRateInfo.def().get() / 1e6f;\n> > > > +\n> > > > +\tint32_t minExposure = static_cast(minExposurePixels / maxPixelRate);\n> > > > +\tint32_t maxExposure = static_cast(maxExposurePixels / minPixelRate);\n> > > > +\tint32_t defExposure = static_cast(defExposurePixels / defPixelRate);\n> > > > +\n> > > > +\tControlInfoMap::Map map;\n> > > > +\tmap[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > > > +\t\t\t\t\t\t defExposure);\n> > > > +\tcontrols_ = std::move(map);\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\fn CameraSensor::model()\n> > > > * \\brief Retrieve the sensor model name\n> > > > --\n> > > > 2.29.2\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 541DBC0F1B\n\tfor ;\n\tMon, 28 Dec 2020 10:13:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 23864615B2;\n\tMon, 28 Dec 2020 11:13:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D394260525\n\tfor ;\n\tMon, 28 Dec 2020 11:13:56 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 61AF525C;\n\tMon, 28 Dec 2020 11:13:55 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"T3xXevs8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1609150436;\n\tbh=IijJPE1x1EMGweBAOnlbVEy5QJQqVuhb71malUlaYBA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T3xXevs8fEcuA8RuB9V5yItDcrHgbZVSPPVhnf7GAYyNK++KFB+y+6BHEG+upqhmE\n\t8snq4MttQXsmJlWtm7ePeNMRm/8dHOqsGYs5cmLyq2aIIUB5PKs8CkPqjUAGboZdbS\n\trDZDFULkQMkDvLWb2oxEAYqhSwAxyhOEbWEDmL6g=","Date":"Mon, 28 Dec 2020 19:13:50 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20201228101350.GB67590@pyrite.rasen.tech>","References":"<20201223184516.58791-1-jacopo@jmondi.org>\n\t<20201223184516.58791-3-jacopo@jmondi.org>\n\t<20201228082657.GF1933@pyrite.rasen.tech>\n\t<20201228093925.bjh6o25pnha3qvdy@uno.localdomain>\n\t<20201228101225.GA67590@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201228101225.GA67590@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH 2/5] libcamera: camera_sensor:\n\tInitialize controls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]