[{"id":4520,"web_url":"https://patchwork.libcamera.org/comment/4520/","msgid":"<20200425210357.GE10975@pendragon.ideasonboard.com>","date":"2020-04-25T21:03:57","subject":"Re: [libcamera-devel] [PATCH v3 11/13] libcamera: camera_sensor:\n\tAdd method to get sensor info","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 Fri, Apr 24, 2020 at 11:53:02PM +0200, Jacopo Mondi wrote:\n> Add method to retrieve the CameraSensorInfo to the CameraSensor class.\n\ns/method/a method/\n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/camera_sensor.cpp | 84 +++++++++++++++++++++++++++\n> src/libcamera/include/camera_sensor.h | 1 +\n> 2 files changed, 85 insertions(+)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index e39f277622ae..c1ef4adb579c 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -492,6 +492,90 @@ int CameraSensor::setControls(ControlList *ctrls)\n> * \\return The list of camera sensor properties\n> */\n> \n> +/**\n> + * \\brief Assemble and return the camera sensor info\n> + * \\param[out] info The camera sensor info that report the sensor information\n\ns/report/reports/\n\n> + *\n> + * The CameraSensorInfo content is assembled by inspecting the currently\n> + * applied sensor configuration and the sensor static properties.\n> + *\n> + * \\return 0 on success, a negative error code otherwise\n> + */\n> +int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> +{\n> +\t/*\n> +\t * Make sure the sub-device exports all the controls we need to operate.\n> +\t *\n> +\t * Currently V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK are required.\n> +\t *\n> +\t * \\todo This is an hard policy that all sensors that want to export\n> +\t * a properly populated CameraSensorInfo have to adhere to. Consider if\n> +\t * it's worth relaxing it and providing default values instead.\n\nI like such hard policies :-) I don't see a way to provide a sane\ndefault, so I think this is fine. It's about time to put a bit of order\nin the V4L2 sensor drivers anyway.\n\n> +\t */\n> +\tconst ControlInfoMap &controlMap = subdev_->controls();\n> +\tif (controlMap.find(V4L2_CID_PIXEL_RATE) == controlMap.end()) {\n> +\t\tLOG(CameraSensor, Error) << \"'Pixel rate' control not available\";\n\nYou can drop the quotes here and below. I would also either write\nPIXEL_RATE here, or Horizontal blanking below.\n\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (controlMap.find(V4L2_CID_HBLANK) == controlMap.end()) {\n> +\t\tLOG(CameraSensor, Error) << \"'HBLANK' control not available\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/*\n> +\t * Construct the camera sensor name using the media entity name.\n> +\t *\n> +\t * \\todo There is no standardized naming scheme for sensor entities\n> +\t * in the Linux kernel at the moment. The most common naming scheme\n> +\t * is the one obtained by associating the sensor name and its I2C\n> +\t * device and bus addresses in the form of: \"devname i2c-adapt:i2c-addr\"\n> +\t * Assume this is the standard naming scheme and parse the first part\n> +\t * of the entity name to obtain \"devname\".\n> +\t */\n> +\tstd::string entityName = subdev_->entity()->name();\n> +\tinfo->name = *utils::split(entityName, \" \").begin();\n\n\tinfo->name = entityName.substr(0, entityName.find(' '));\n\nwill be more efficient (and would avoid me the need to check if the\ntemporary object returned by utils::split() will last until the\nassignement operator is executed).\n\n> +\n> +\t/*\n> +\t * Get the active area size from the ActiveAreas property.\n> +\t *\n> +\t * \\todo The ActiveAreas property aims to describe multiple\n> +\t * active area rectangles. At the moment only a single active\n> +\t * area rectangle is exposed by the subdevice API. Use that single\n> +\t * rectangle width and height to construct the ActiveAreaSize.\n> +\t */\n> +\tSpan activeArea = properties_.get(properties::ActiveAreas);\n> +\tinfo->activeAreaSize = { static_cast(activeArea[2]),\n> +\t\t\t\t static_cast(activeArea[3]) };\n> +\n> +\t/* The bit-depth and image size depend on the currently applied format. */\n> +\tV4L2SubdeviceFormat format{};\n> +\tint ret = subdev_->getFormat(0, &format);\n> +\tif (ret)\n> +\t\treturn ret;\n\nIt doesn't have to be done as part of this series, but do you think it\nwould make sense to cache the format in the CameraSensor class ? We'll\nlikely have to rework the CameraSensor class when supporting sensors\nthat expose multiple subdevs, so we can rework this at that time.\n\n> +\tinfo->bitsPerPixel = format.bitsPerPixel();\n> +\tinfo->outputSize = format.size;\n> +\n> +\t/* It's mandatory for the subdevice to report its crop rectangle. */\n> +\tret = subdev_->getCropRectangle(0, &info->analogCrop);\n> +\tif (ret) {\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"Failed to construct camera sensor info: \"\n> +\t\t\t<< \"the camera sensor does not report the crop rectangle\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tint64_t pixelRate;\n> +\tsubdev_->getControl(V4L2_CID_PIXEL_RATE, &pixelRate);\n\nShould we check for errors here ? The control is guaranteed to exist,\nbut other errors could occur. And if we add an error check here, is it\nworth it checking if the controls exist at the top ?\n\n> +\tinfo->pixelClock = pixelRate;\n\nShould the pixelClock field already be extended to 64 bits ?\n\n> +\n> +\tint32_t hblank;\n> +\tsubdev_->getControl(V4L2_CID_HBLANK, &hblank);\n\nSame here, error checking is needed.\n\n> +\tinfo->lineLength = info->outputSize.width + hblank;\n> +\n> +\treturn 0;\n> +}\n> +\n> std::string CameraSensor::logPrefix() const\n> {\n> \treturn \"'\" + subdev_->entity()->name() + \"'\";\n> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> index e19852d4cabe..b162c3886b1d 100644\n> --- a/src/libcamera/include/camera_sensor.h\n> +++ b/src/libcamera/include/camera_sensor.h\n> @@ -61,6 +61,7 @@ public:\n> \tint setControls(ControlList *ctrls);\n> \n> \tconst ControlList &properties() const { return properties_; }\n> +\tint sensorInfo(CameraSensorInfo *info) const;\n> \n> protected:\n> \tstd::string logPrefix() const;","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 B7937603FC\n\tfor ;\n\tSat, 25 Apr 2020 23:04:12 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 33F0B4F7;\n\tSat, 25 Apr 2020 23:04:12 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"QSDBhRB/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587848652;\n\tbh=IhoLYApRFNi28gi3njCCxSLCMiyc3HwmNqiOc3YuC1Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QSDBhRB/Syz57WtMMDWXxZICwisz7aCijq8s5Q+TOD48qbYSyW/+H9byuHwP5+TKe\n\t3SYHqyHHafLOwz+9IcMa5QOdo3/mFW8LlzJ3E+S6iurn1ySOpb39OjAHqqrCk46yYq\n\tLXgZk7ZB0NTS+3YlEn9OqhGzdteDw3l5QS6dQWuQ=","Date":"Sun, 26 Apr 2020 00:03:57 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200425210357.GE10975@pendragon.ideasonboard.com>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-12-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200424215304.558317-12-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 11/13] libcamera: camera_sensor:\n\tAdd method to get sensor info","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","X-List-Received-Date":"Sat, 25 Apr 2020 21:04:12 -0000"}},{"id":4524,"web_url":"https://patchwork.libcamera.org/comment/4524/","msgid":"<20200425231651.GI10975@pendragon.ideasonboard.com>","date":"2020-04-25T23:16:51","subject":"Re: [libcamera-devel] [PATCH v3 11/13] libcamera: camera_sensor:\n\tAdd method to get sensor info","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Sun, Apr 26, 2020 at 12:03:58AM +0300, Laurent Pinchart wrote:\n> On Fri, Apr 24, 2020 at 11:53:02PM +0200, Jacopo Mondi wrote:\n> > Add method to retrieve the CameraSensorInfo to the CameraSensor class.\n> \n> s/method/a method/\n> \n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/camera_sensor.cpp | 84 +++++++++++++++++++++++++++\n> > src/libcamera/include/camera_sensor.h | 1 +\n> > 2 files changed, 85 insertions(+)\n> > \n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index e39f277622ae..c1ef4adb579c 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -492,6 +492,90 @@ int CameraSensor::setControls(ControlList *ctrls)\n> > * \\return The list of camera sensor properties\n> > */\n> > \n> > +/**\n> > + * \\brief Assemble and return the camera sensor info\n> > + * \\param[out] info The camera sensor info that report the sensor information\n> \n> s/report/reports/\n> \n> > + *\n> > + * The CameraSensorInfo content is assembled by inspecting the currently\n> > + * applied sensor configuration and the sensor static properties.\n> > + *\n> > + * \\return 0 on success, a negative error code otherwise\n> > + */\n> > +int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> > +{\n> > +\t/*\n> > +\t * Make sure the sub-device exports all the controls we need to operate.\n> > +\t *\n> > +\t * Currently V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK are required.\n> > +\t *\n> > +\t * \\todo This is an hard policy that all sensors that want to export\n> > +\t * a properly populated CameraSensorInfo have to adhere to. Consider if\n> > +\t * it's worth relaxing it and providing default values instead.\n> \n> I like such hard policies :-) I don't see a way to provide a sane\n> default, so I think this is fine. It's about time to put a bit of order\n> in the V4L2 sensor drivers anyway.\n> \n> > +\t */\n> > +\tconst ControlInfoMap &controlMap = subdev_->controls();\n> > +\tif (controlMap.find(V4L2_CID_PIXEL_RATE) == controlMap.end()) {\n> > +\t\tLOG(CameraSensor, Error) << \"'Pixel rate' control not available\";\n> \n> You can drop the quotes here and below. I would also either write\n> PIXEL_RATE here, or Horizontal blanking below.\n> \n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tif (controlMap.find(V4L2_CID_HBLANK) == controlMap.end()) {\n> > +\t\tLOG(CameraSensor, Error) << \"'HBLANK' control not available\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Construct the camera sensor name using the media entity name.\n> > +\t *\n> > +\t * \\todo There is no standardized naming scheme for sensor entities\n> > +\t * in the Linux kernel at the moment. The most common naming scheme\n> > +\t * is the one obtained by associating the sensor name and its I2C\n> > +\t * device and bus addresses in the form of: \"devname i2c-adapt:i2c-addr\"\n> > +\t * Assume this is the standard naming scheme and parse the first part\n> > +\t * of the entity name to obtain \"devname\".\n> > +\t */\n> > +\tstd::string entityName = subdev_->entity()->name();\n> > +\tinfo->name = *utils::split(entityName, \" \").begin();\n> \n> \tinfo->name = entityName.substr(0, entityName.find(' '));\n> \n> will be more efficient (and would avoid me the need to check if the\n> temporary object returned by utils::split() will last until the\n> assignement operator is executed).\n> \n> > +\n> > +\t/*\n> > +\t * Get the active area size from the ActiveAreas property.\n> > +\t *\n> > +\t * \\todo The ActiveAreas property aims to describe multiple\n> > +\t * active area rectangles. At the moment only a single active\n> > +\t * area rectangle is exposed by the subdevice API. Use that single\n> > +\t * rectangle width and height to construct the ActiveAreaSize.\n> > +\t */\n> > +\tSpan activeArea = properties_.get(properties::ActiveAreas);\n> > +\tinfo->activeAreaSize = { static_cast(activeArea[2]),\n> > +\t\t\t\t static_cast(activeArea[3]) };\n> > +\n> > +\t/* The bit-depth and image size depend on the currently applied format. */\n> > +\tV4L2SubdeviceFormat format{};\n> > +\tint ret = subdev_->getFormat(0, &format);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> \n> It doesn't have to be done as part of this series, but do you think it\n> would make sense to cache the format in the CameraSensor class ? We'll\n> likely have to rework the CameraSensor class when supporting sensors\n> that expose multiple subdevs, so we can rework this at that time.\n> \n> > +\tinfo->bitsPerPixel = format.bitsPerPixel();\n> > +\tinfo->outputSize = format.size;\n> > +\n> > +\t/* It's mandatory for the subdevice to report its crop rectangle. */\n> > +\tret = subdev_->getCropRectangle(0, &info->analogCrop);\n> > +\tif (ret) {\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"Failed to construct camera sensor info: \"\n> > +\t\t\t<< \"the camera sensor does not report the crop rectangle\";\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tint64_t pixelRate;\n> > +\tsubdev_->getControl(V4L2_CID_PIXEL_RATE, &pixelRate);\n> \n> Should we check for errors here ? The control is guaranteed to exist,\n> but other errors could occur. And if we add an error check here, is it\n> worth it checking if the controls exist at the top ?\n> \n> > +\tinfo->pixelClock = pixelRate;\n> \n> Should the pixelClock field already be extended to 64 bits ?\n> \n> > +\n> > +\tint32_t hblank;\n> > +\tsubdev_->getControl(V4L2_CID_HBLANK, &hblank);\n> \n> Same here, error checking is needed.\n> \n> > +\tinfo->lineLength = info->outputSize.width + hblank;\n\nAnother comment, to improve efficiency, would it make sense to read the\ntwo controls in one go ? I've just sent a small series that simplifies\nthe API of V4L2Device::getControls(), and you can then write\n\n\tControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,\n\t\t\t\t\t\t V4L2_CID_HBLANK });\n\tif (ctrls.empty()) {\n\t\tLOG(CameraSensor, Error)\n\t\t\t<< \"Failed to retrieve camera info controls\";\n\t\treturn -EINVAL;\n\t}\n\n\tint32_t hblank = ctrls.get(V4L2_CID_HBLANK).get();\n\tinfo->lineLength = info->outputSize.width + hblank;\n\tinfo->pixelClock = ctrls.get(V4L2_CID_PIXEL_RATE).get();\n\nDo you think the API would be good enough ?\n\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > std::string CameraSensor::logPrefix() const\n> > {\n> > \treturn \"'\" + subdev_->entity()->name() + \"'\";\n> > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > index e19852d4cabe..b162c3886b1d 100644\n> > --- a/src/libcamera/include/camera_sensor.h\n> > +++ b/src/libcamera/include/camera_sensor.h\n> > @@ -61,6 +61,7 @@ public:\n> > \tint setControls(ControlList *ctrls);\n> > \n> > \tconst ControlList &properties() const { return properties_; }\n> > +\tint sensorInfo(CameraSensorInfo *info) const;\n> > \n> > protected:\n> > \tstd::string logPrefix() const;","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 3656E603FD\n\tfor ;\n\tSun, 26 Apr 2020 01:17:07 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C14774F7;\n\tSun, 26 Apr 2020 01:17:06 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"vWjOM1nj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587856627;\n\tbh=8Gm1Rl2LBU1eoQmqV1HIgjX/2fmTiNXMONQ7BVz4M5c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vWjOM1nj7C08ZOrwI4C7LK75TJZqCPboY8ds/p8SZndVsfA9Dv63tkXR4+6jw7alF\n\tXFaOLL4Kd/i0ZdtxUw/b5GqVNYHDNMyrE2CT1ip/jhjyiQgC1MLyUnljVv6Ee9ok8l\n\tGHofa9l0tMGYZyKzuHQkA68N48fmr2FJHR/URRLo=","Date":"Sun, 26 Apr 2020 02:16:51 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200425231651.GI10975@pendragon.ideasonboard.com>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-12-jacopo@jmondi.org>\n\t<20200425210357.GE10975@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200425210357.GE10975@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 11/13] libcamera: camera_sensor:\n\tAdd method to get sensor info","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","X-List-Received-Date":"Sat, 25 Apr 2020 23:17:07 -0000"}}]