[{"id":4597,"web_url":"https://patchwork.libcamera.org/comment/4597/","msgid":"<20200428002839.GJ1165729@oden.dyn.berto.se>","date":"2020-04-28T00:28:39","subject":"Re: [libcamera-devel] [PATCH v4 6/7] libcamera: camera_sensor: Add\n\tmethod to get sensor info","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 2020-04-27 23:32:35 +0200, Jacopo Mondi wrote:\n> Add method to retrieve the CameraSensorInfo to the CameraSensor class.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/camera_sensor.cpp | 83 +++++++++++++++++++++++++++\n> src/libcamera/include/camera_sensor.h | 1 +\n> 2 files changed, 84 insertions(+)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 4fd1ee84eb47..8b37f63dc04e 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -440,6 +440,89 @@ int CameraSensor::setControls(ControlList *ctrls)\n> \treturn subdev_->setControls(ctrls);\n> }\n> \n> +/**\n> + * \\brief Assemble and return the camera sensor info\n> + * \\param[out] info The camera sensor info that reports the sensor information\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 * 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->model = entityName.substr(0, entityName.find(' '));\n\nIf we need to keep the model name I like the interface provided by \nCameraSensor::model() from '[PATCH] libcamera: camera_sensor: Add \nmodel() function'.\n\nAs stated in the patch mention above I have comments on the \ndocumentation and it's implementation.\n\n> +\n> +\t/*\n> +\t * Get the active area size from the ActiveAreas property. Do\n> +\t * not use the content of properties::ActiveAreas but fetch it from the\n> +\t * subdevice as the property registration is optional, but it's\n> +\t * mandatory to construct the CameraSensorInfo.\n\nI'm sorry but this paragraph confuses me.\n\n Get FOO from property. Do not use the content of properties::FOO.\n\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> +\tRectangle rect = {};\n> +\tint ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_DEFAULT, &rect);\n> +\tif (ret) {\n> +\t\tLOG(CameraSensor, Error)\n\nIf the target is mandatory for libcamera shall this be a fatal error ? \nHere and bellow.\n\n> +\t\t\t<< \"Failed to construct camera sensor info: \"\n> +\t\t\t<< \"the camera sensor does not report the active area\";\n> +\n> +\t\treturn ret;\n> +\t}\n> +\tinfo->activeAreaSize = { rect.width, rect.height };\n> +\n> +\t/* It's mandatory for the subdevice to report its crop rectangle. */\n> +\tret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP, &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> +\t/* The bit-depth and image size depend on the currently applied format. */\n> +\tV4L2SubdeviceFormat format{};\n> +\tret = subdev_->getFormat(0, &format);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\tinfo->bitsPerPixel = format.bitsPerPixel();\n> +\tinfo->outputSize = format.size;\n> +\n> +\t/*\n> +\t * Retrieve the pixel rate and the line length through V4L2 controls.\n> +\t * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK controls is\n> +\t * mandatory.\n> +\t */\n> +\tControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,\n> +\t\t\t\t\t\t V4L2_CID_HBLANK });\n> +\tif (ctrls.empty()) {\n\nWhat if only one of the controls are implemented by the driver? Would it \nmake more sens to check ctrls.size() != 2 ?\n\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->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get();\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 28ebfaa30c22..91d270552d3f 100644\n> --- a/src/libcamera/include/camera_sensor.h\n> +++ b/src/libcamera/include/camera_sensor.h\n> @@ -60,6 +60,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;\n> -- \n> 2.26.1\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-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E7A95603FB\n\tfor ;\n\tTue, 28 Apr 2020 02:28:41 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id w20so19631706ljj.0\n\tfor ;\n\tMon, 27 Apr 2020 17:28:41 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tf15sm12323686lfh.76.2020.04.27.17.28.40\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 27 Apr 2020 17:28:40 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"UR0mXTzh\"; \n\tdkim-atps=neutral","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\tbh=hj8WkMxcD9CM2btZqvNEqCX+4Y9qbP/lahkeTXy9P88=;\n\tb=UR0mXTzhHMUPIdsl6DngaRbXBoCRGsV9JngFu7lHFlJF6LcHu1PCQVwIu+xE15uHAd\n\t6mI1W4c7JIs3sOPvPWT4+2W10tKGFl/MApIL0gqwXLgGGxyE/6KhnrEg+CpgLl1wJBhi\n\tCaBfwWkaQIlMNqrL2FU3CsL9Ht7iqpKhIaXvhZZx7AUTkqjYh6R3NNBoaHWh2yO3Q6tx\n\tZ/7beERAM9ooeaQLRjz9Up2ULLdhY7GvAIlqFcZdwNlBbhlDK861zac/6rm5R828cox6\n\tHH/lK+piT0x0IwJ9IMAWUQcQ4/Ay5d44J+qo8xPpikRrRcRPbP0WbEXefx/cnX7sQfcy\n\t6wvw==","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;\n\tbh=hj8WkMxcD9CM2btZqvNEqCX+4Y9qbP/lahkeTXy9P88=;\n\tb=l+63VGplpFQ3yU0gFsQJA2NZvNhZYKB55008T/Hy+RvsB8IJjq3UydKKKXcVdcFEy1\n\tv8isKBIpYOPqIHI+xS3dWB6JHqCHd/2U2Dz9ZxzY2p2DvILwbukYeS2e4gYEa+UQcCgZ\n\tcXqk3opyCttdzYjHSxWbudIjXESZ7SNsibhV4Avp7qMVarM2hOJae5/U3zGBHp9zmpaG\n\tsBX9A0KeoGkrhf1jmLVAMyuFYiMQ5IJ5hw9gFqjTkm3Pw0otgL4vRyXQbHf1sIuk4kQw\n\t0hGVsr3cJ8ZmEqxn3UB++OMwS2/tE+NaUdxNuxCrvaTUCWdz30KMxTL1ChDIB34d6oR7\n\tj3/w==","X-Gm-Message-State":"AGi0PuZbKmnmr44Hstp50C7idInfErGOdteqbYhlnx1t0bFNfXm7c5s+\n\trej3VduUHEBwfVSE5no/AKJxim0osQI=","X-Google-Smtp-Source":"APiQypL4cv7ik2ToMQ2+gVGqcdUSxFKW1nuXXIR6g2W0SHct+6/xg4AfLPSU3KmNsUjrL3bxkKIbFw==","X-Received":"by 2002:a2e:2201:: with SMTP id i1mr15733452lji.31.1588033721133;\n\tMon, 27 Apr 2020 17:28:41 -0700 (PDT)","Date":"Tue, 28 Apr 2020 02:28:39 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200428002839.GJ1165729@oden.dyn.berto.se>","References":"<20200427213236.333777-1-jacopo@jmondi.org>\n\t<20200427213236.333777-7-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":"<20200427213236.333777-7-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 6/7] libcamera: camera_sensor: Add\n\tmethod 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":"Tue, 28 Apr 2020 00:28:42 -0000"}},{"id":4603,"web_url":"https://patchwork.libcamera.org/comment/4603/","msgid":"<20200428020753.GF3579@pendragon.ideasonboard.com>","date":"2020-04-28T02:07:53","subject":"Re: [libcamera-devel] [PATCH v4 6/7] libcamera: camera_sensor: Add\n\tmethod 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 Niklas and Jacopo,\n\nOn Tue, Apr 28, 2020 at 02:28:39AM +0200, Niklas Söderlund wrote:\n> On 2020-04-27 23:32:35 +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 | 83 +++++++++++++++++++++++++++\n> > src/libcamera/include/camera_sensor.h | 1 +\n> > 2 files changed, 84 insertions(+)\n> > \n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 4fd1ee84eb47..8b37f63dc04e 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -440,6 +440,89 @@ int CameraSensor::setControls(ControlList *ctrls)\n> > \treturn subdev_->setControls(ctrls);\n> > }\n> > \n> > +/**\n> > + * \\brief Assemble and return the camera sensor info\n> > + * \\param[out] info The camera sensor info that reports the sensor information\n\nI would s/ that reports.*$// as it's redundant I think, up to you.\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 * 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->model = entityName.substr(0, entityName.find(' '));\n> \n> If we need to keep the model name I like the interface provided by \n> CameraSensor::model() from '[PATCH] libcamera: camera_sensor: Add \n> model() function'.\n> \n> As stated in the patch mention above I have comments on the \n> documentation and it's implementation.\n\nI've copied the code for that function from this patch :-) I think we\nshould then replace this by just a call to model(). Let's discuss your\ncomments as part of the other patch.\n\n> > +\n> > +\t/*\n> > +\t * Get the active area size from the ActiveAreas property. Do\n> > +\t * not use the content of properties::ActiveAreas but fetch it from the\n> > +\t * subdevice as the property registration is optional, but it's\n> > +\t * mandatory to construct the CameraSensorInfo.\n> \n> I'm sorry but this paragraph confuses me.\n> \n> Get FOO from property. Do not use the content of properties::FOO.\n> \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\nI agree with Niklas. I think you can just write\n\n\t/* Get the active area size. */\n\n> > +\tRectangle rect = {};\n> > +\tint ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_DEFAULT, &rect);\n> > +\tif (ret) {\n> > +\t\tLOG(CameraSensor, Error)\n> \n> If the target is mandatory for libcamera shall this be a fatal error ? \n> Here and bellow.\n\nI don't think an abort() is required here, returning an error should\nallow us to handle that gracefully and, for instance, display a message\nto the user in the GUI instead of killing the process.\n\n> > +\t\t\t<< \"Failed to construct camera sensor info: \"\n> > +\t\t\t<< \"the camera sensor does not report the active area\";\n> > +\n> > +\t\treturn ret;\n> > +\t}\n> > +\tinfo->activeAreaSize = { rect.width, rect.height };\n> > +\n> > +\t/* It's mandatory for the subdevice to report its crop rectangle. */\n> > +\tret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP, &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> > +\t/* The bit-depth and image size depend on the currently applied format. */\n\ns/bit-depth/bit depth/\n\n> > +\tV4L2SubdeviceFormat format{};\n> > +\tret = subdev_->getFormat(0, &format);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\tinfo->bitsPerPixel = format.bitsPerPixel();\n> > +\tinfo->outputSize = format.size;\n> > +\n> > +\t/*\n> > +\t * Retrieve the pixel rate and the line length through V4L2 controls.\n> > +\t * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK controls is\n> > +\t * mandatory.\n> > +\t */\n> > +\tControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,\n> > +\t\t\t\t\t\t V4L2_CID_HBLANK });\n> > +\tif (ctrls.empty()) {\n> \n> What if only one of the controls are implemented by the driver? Would it \n> make more sens to check ctrls.size() != 2 ?\n\nIf any of the controls is unsupported, getControls() returns an empty\nlist. I'm fine with != 2 too though, up to Jacopo.\n\nAssuming we call model() above and thus don't need to discuss how the\nmodel is retrieved as part of this patch,\n\nReviewed-by: Laurent Pinchart \n\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->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get();\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 28ebfaa30c22..91d270552d3f 100644\n> > --- a/src/libcamera/include/camera_sensor.h\n> > +++ b/src/libcamera/include/camera_sensor.h\n> > @@ -60,6 +60,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 99814603F5\n\tfor ;\n\tTue, 28 Apr 2020 04:08:09 +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 1374472C;\n\tTue, 28 Apr 2020 04:08:09 +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=\"MaxbTweB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1588039689;\n\tbh=J1qoPI94fQozb59Ui5Q7f4/w0heHzZsLP3EnMWCHnK0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MaxbTweBUCCThzUZP2RKKN0KGGKiRVYSP80ecjyfOvs94b4JXvLkwp8OSu73z+VaB\n\tz3k+TAEYEYTcvKCR4waWtGP23PZKiWQKRdwvOfqu8pkJAivuwTE5lydgPrtBW6QMxN\n\tsJ5TsA4S+9t9bqpdV46iurVZHPfTsuQ2imlCQQIo=","Date":"Tue, 28 Apr 2020 05:07:53 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20200428020753.GF3579@pendragon.ideasonboard.com>","References":"<20200427213236.333777-1-jacopo@jmondi.org>\n\t<20200427213236.333777-7-jacopo@jmondi.org>\n\t<20200428002839.GJ1165729@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200428002839.GJ1165729@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v4 6/7] libcamera: camera_sensor: Add\n\tmethod 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":"Tue, 28 Apr 2020 02:08:09 -0000"}},{"id":4611,"web_url":"https://patchwork.libcamera.org/comment/4611/","msgid":"<20200428072418.3qz7o5o2qvqq2fkb@uno.localdomain>","date":"2020-04-28T07:24:18","subject":"Re: [libcamera-devel] [PATCH v4 6/7] libcamera: camera_sensor: Add\n\tmethod to get sensor info","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas, Laurent,\n\nOn Tue, Apr 28, 2020 at 05:07:53AM +0300, Laurent Pinchart wrote:\n> Hi Niklas and Jacopo,\n>\n> On Tue, Apr 28, 2020 at 02:28:39AM +0200, Niklas Söderlund wrote:\n> > On 2020-04-27 23:32:35 +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 | 83 +++++++++++++++++++++++++++\n> > > src/libcamera/include/camera_sensor.h | 1 +\n> > > 2 files changed, 84 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index 4fd1ee84eb47..8b37f63dc04e 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -440,6 +440,89 @@ int CameraSensor::setControls(ControlList *ctrls)\n> > > \treturn subdev_->setControls(ctrls);\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Assemble and return the camera sensor info\n> > > + * \\param[out] info The camera sensor info that reports the sensor information\n>\n> I would s/ that reports.*$// as it's redundant I think, up to you.\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 * 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->model = entityName.substr(0, entityName.find(' '));\n> >\n> > If we need to keep the model name I like the interface provided by\n> > CameraSensor::model() from '[PATCH] libcamera: camera_sensor: Add\n> > model() function'.\n\nThere was no such a patch when I sent this out\n\n> >\n> > As stated in the patch mention above I have comments on the\n> > documentation and it's implementation.\n>\n> I've copied the code for that function from this patch :-) I think we\n> should then replace this by just a call to model(). Let's discuss your\n> comments as part of the other patch.\n>\n\nAck\n\n> > > +\n> > > +\t/*\n> > > +\t * Get the active area size from the ActiveAreas property. Do\n> > > +\t * not use the content of properties::ActiveAreas but fetch it from the\n> > > +\t * subdevice as the property registration is optional, but it's\n> > > +\t * mandatory to construct the CameraSensorInfo.\n> >\n> > I'm sorry but this paragraph confuses me.\n> >\n> > Get FOO from property. Do not use the content of properties::FOO.\n> >\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>\n> I agree with Niklas. I think you can just write\n>\n> \t/* Get the active area size. */\n>\n\nYes, leftovers\n\n> > > +\tRectangle rect = {};\n> > > +\tint ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_DEFAULT, &rect);\n> > > +\tif (ret) {\n> > > +\t\tLOG(CameraSensor, Error)\n> >\n> > If the target is mandatory for libcamera shall this be a fatal error ?\n> > Here and bellow.\n>\n> I don't think an abort() is required here, returning an error should\n> allow us to handle that gracefully and, for instance, display a message\n> to the user in the GUI instead of killing the process.\n\nI don't see a reason to abort() to be honest. This method is called by\npipeline handlers, they should be able to exit gracefully and stop\ncameras and whatnot. Why would you tear down the whole library here ?\n\n>\n> > > +\t\t\t<< \"Failed to construct camera sensor info: \"\n> > > +\t\t\t<< \"the camera sensor does not report the active area\";\n> > > +\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\tinfo->activeAreaSize = { rect.width, rect.height };\n> > > +\n> > > +\t/* It's mandatory for the subdevice to report its crop rectangle. */\n> > > +\tret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP, &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> > > +\t/* The bit-depth and image size depend on the currently applied format. */\n>\n> s/bit-depth/bit depth/\n>\n> > > +\tV4L2SubdeviceFormat format{};\n> > > +\tret = subdev_->getFormat(0, &format);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\tinfo->bitsPerPixel = format.bitsPerPixel();\n> > > +\tinfo->outputSize = format.size;\n> > > +\n> > > +\t/*\n> > > +\t * Retrieve the pixel rate and the line length through V4L2 controls.\n> > > +\t * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK controls is\n> > > +\t * mandatory.\n> > > +\t */\n> > > +\tControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,\n> > > +\t\t\t\t\t\t V4L2_CID_HBLANK });\n> > > +\tif (ctrls.empty()) {\n> >\n> > What if only one of the controls are implemented by the driver? Would it\n> > make more sens to check ctrls.size() != 2 ?\n\nWe need both controls, there's no use for a single one, so != 2 or\nempty() lead to the same result. And to get something != empty() on\nerror the getControls() function should be reworked.\n\n>\n> If any of the controls is unsupported, getControls() returns an empty\n> list. I'm fine with != 2 too though, up to Jacopo.\n>\n> Assuming we call model() above and thus don't need to discuss how the\n> model is retrieved as part of this patch,\n>\n> Reviewed-by: Laurent Pinchart \n\nThanks\n j\n\n>\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->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get();\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 28ebfaa30c22..91d270552d3f 100644\n> > > --- a/src/libcamera/include/camera_sensor.h\n> > > +++ b/src/libcamera/include/camera_sensor.h\n> > > @@ -60,6 +60,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;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 63BCD603F7\n\tfor ;\n\tTue, 28 Apr 2020 09:21:09 +0200 (CEST)","from uno.localdomain (a-ur1-85.tin.it [212.216.150.148])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id E6BBD40011;\n\tTue, 28 Apr 2020 07:21:07 +0000 (UTC)"],"X-Originating-IP":"212.216.150.148","Date":"Tue, 28 Apr 2020 09:24:18 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200428072418.3qz7o5o2qvqq2fkb@uno.localdomain>","References":"<20200427213236.333777-1-jacopo@jmondi.org>\n\t<20200427213236.333777-7-jacopo@jmondi.org>\n\t<20200428002839.GJ1165729@oden.dyn.berto.se>\n\t<20200428020753.GF3579@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200428020753.GF3579@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 6/7] libcamera: camera_sensor: Add\n\tmethod 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":"Tue, 28 Apr 2020 07:21:09 -0000"}}]