[{"id":14411,"web_url":"https://patchwork.libcamera.org/comment/14411/","msgid":"","date":"2020-12-30T09:51:46","subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: camera_sensor:\n\tValidate driver support","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 Mon, Dec 28, 2020 at 05:55:56PM +0100, Jacopo Mondi wrote:\n> The CameraSensor class requires the sensor driver to report\n> information through V4L2 controls and through the V4L2 selection API,\n> and uses those information to register Camera properties and to\n\ns/those information/that information/\n\n> construct CameraSensorInfo class instances to provide them to the IPA.\n> \n> Currently, validation of the kernel support happens each time a\n> feature is requested, with slighly similar debug/error messages\n> output to the user in case a feature is not supported.\n> \n> Rationalize this by:\n> - Validate the sensor driver requirements in a single function\n> - Expand the debug output when a property gets defaulted to a value\n> - Be more verbose when constructing CameraSensorInfo is not possible\n> \n> Reviewed-by: Paul Elder \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/camera_sensor.h | 1 +\n> src/libcamera/camera_sensor.cpp | 106 +++++++++++++++++++--\n> 2 files changed, 100 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index f80d836161a5..aee10aa6e3c7 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -69,6 +69,7 @@ protected:\n> \n> private:\n> \tint generateId();\n> +\tint validateSensorDriver();\n> \tint initProperties();\n> \n> \tconst MediaEntity *entity_;\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index e786821d4ba2..71d7c862e69a 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -207,6 +207,10 @@ int CameraSensor::init()\n> \t */\n> \tresolution_ = sizes_.back();\n> \n> +\tret = validateSensorDriver();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \tret = initProperties();\n> \tif (ret)\n> \t\treturn ret;\n> @@ -214,6 +218,81 @@ int CameraSensor::init()\n> \treturn 0;\n> }\n> \n> +int CameraSensor::validateSensorDriver()\n> +{\n> +\t/*\n> +\t * Make sure the sensor driver supports the mandatory controls\n> +\t * required by the CameraSensor class.\n> +\t * - V4L2_CID_PIXEL_RATE is used to calculate the sensor timings\n> +\t * - V4L2_CID_HBLANK is used to calculate the line length\n> +\t */\n> +\tconst std::vector mandatoryControls{\n> +\t\tV4L2_CID_PIXEL_RATE,\n> +\t\tV4L2_CID_HBLANK,\n> +\t};\n> +\n> +\tControlList ctrls = subdev_->getControls(mandatoryControls);\n> +\tif (ctrls.empty()) {\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"Mandatory V4L2 controls not available\";\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"Please consider upgrading the sensor driver\";\n\nMaybe \"The sensor kernel driver needs to be fixed\" ? Users may wonder\nwhat to upgrade to with a \"please upgrade\" message. But maybe I worry\ntoo much :-) Up to you.\n\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tint err = 0;\n> +\t/*\n> +\t * Optional controls are used to register optional sensor\n> +\t * properties. If not present, some values will be defaulted.\n> +\t */\n> +\tconst std::vector optionalControls{\n> +\t\tV4L2_CID_CAMERA_ORIENTATION,\n> +\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> +\t};\n> +\n> +\tctrls = subdev_->getControls(optionalControls);\n> +\tif (ctrls.empty()) {\n> +\t\tLOG(CameraSensor, Info)\n> +\t\t\t<< \"Optional V4L2 controls not supported\";\n> +\t\terr = -EINVAL;\n> +\t}\n> +\n> +\t/*\n> +\t * Make sure the required selection targets are supported.\n> +\t *\n> +\t * Failures in reading any of the targets are not deemed to be fatal,\n> +\t * but some properties and features, like constructing a\n> +\t * CameraSensorInfo for the IPA module, won't be supported.\n> +\t */\n> +\tRectangle rect;\n> +\tint ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> +\tif (ret) {\n> +\t\tLOG(CameraSensor, Info)\n> +\t\t\t<< \"Failed to retrieve the readable pixel area size\";\n> +\t\terr = -EINVAL;\n> +\t}\n> +\n> +\tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);\n> +\tif (ret) {\n> +\t\tLOG(CameraSensor, Info)\n> +\t\t\t<< \"Failed to retrieve the active pixel area size\";\n> +\t\terr = -EINVAL;\n> +\t}\n> +\n> +\tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect);\n> +\tif (ret) {\n> +\t\tLOG(CameraSensor, Info)\n> +\t\t\t<< \"Failed to retreive the sensor crop rectangle\";\n> +\t\terr = -EINVAL;\n> +\t}\n> +\n> +\tif (err)\n> +\t\tLOG(CameraSensor, Info)\n> +\t\t\t<< \"Please consider upgrading the sensor driver\";\n\nSame as above.\n\n> +\n> +\treturn 0;\n\nI think we need to make this fatal fairly soon, and I wonder whether we\ncould do so already. What platforms would we break ?\n\n> +}\n> +\n> int CameraSensor::initProperties()\n> {\n> \t/*\n> @@ -278,21 +357,29 @@ int CameraSensor::initProperties()\n> \t\t}\n> \t} else {\n> \t\tpropertyValue = properties::CameraLocationFront;\n> +\t\tLOG(CameraSensor, Debug)\n> +\t\t\t<< \"Location property defaulted to 'Front Camera'\";\n\nIf we make the failures above fatal, we will be able to drop the 'else'\nbranch here and below, right ?\n\n> \t}\n> \tproperties_.set(properties::Location, propertyValue);\n> \n> \t/* Camera Rotation: default is 0 degrees. */\n> \tconst auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> -\tif (rotationControl != controls.end())\n> +\tif (rotationControl != controls.end()) {\n> \t\tpropertyValue = rotationControl->second.def().get();\n> -\telse\n> +\t} else {\n> \t\tpropertyValue = 0;\n> +\t\tLOG(CameraSensor, Debug)\n> +\t\t\t<< \"Rotation property defaulted to 0 degrees\";\n> +\t}\n> \tproperties_.set(properties::Rotation, propertyValue);\n> \n> \tRectangle bounds;\n> \tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &bounds);\n> \tif (!ret)\n> \t\tproperties_.set(properties::PixelArraySize, bounds.size());\n> +\telse\n> +\t\tLOG(CameraSensor, Debug)\n> +\t\t\t<< \"PixelArraySize property not registered\";\n> \n> \tRectangle crop;\n> \tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &crop);\n> @@ -306,6 +393,9 @@ int CameraSensor::initProperties()\n> \t\tcrop.x -= bounds.x;\n> \t\tcrop.y -= bounds.y;\n> \t\tproperties_.set(properties::PixelArrayActiveAreas, { crop });\n> +\t} else {\n> +\t\tLOG(CameraSensor, Debug)\n> +\t\t\t<< \"PixelArrayActiveAreas property not registered\";\n> \t}\n> \n> \t/* Color filter array pattern, register only for RAW sensors. */\n> @@ -569,6 +659,8 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\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 active area\";\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"The IPA might not work correctly\";\n\nDo we need this message and the one below ? In those error paths\nsensorInfo() returns an error, and the caller will fail. It's not that\nthe IPA may not work correctly, the whole camera configuration will fail\n:-) I'm tempted to take the opposite approach: now that we validate that\nthe sensor driver provides the right API, we could have less verbose\nmessages here. I'd drop the \"Failed to construct camera sensor info:\"\nprefix, turning this particular error message into\n\n \t\tLOG(CameraSensor, Error) << \"Failed to get active area\";\n\nand similarly below.\n\n> \n> \t\treturn ret;\n> \t}\n> @@ -580,6 +672,8 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\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\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"The IPA might not work correctly\";\n> \t\treturn ret;\n> \t}\n> \n> @@ -601,16 +695,14 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\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> +\t/* Retrieve the pixel rate and the line length through V4L2 controls. */\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\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"The IPA might not work correctly\";\n> \t\treturn -EINVAL;\n> \t}\n>","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 51E6EC0F1A\n\tfor ;\n\tWed, 30 Dec 2020 09:52:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D50AD615B2;\n\tWed, 30 Dec 2020 10:51:59 +0100 (CET)","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 001586031B\n\tfor ;\n\tWed, 30 Dec 2020 10:51:58 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 65F6DEF;\n\tWed, 30 Dec 2020 10:51:58 +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=\"D4mNxSTo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1609321918;\n\tbh=ewf52n7vHaybTcb5OLaXLWO7z5t22M4a6sjHbqT7iHk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=D4mNxSTohu5PuX97Efo2Kwh+tZpxgGGtgQTojoLTBdJ5tySs+mofsQXXQN0QGVY6h\n\twOKW+e40BHQ4ta4ATXuK0KnnqWKXn6JldKy4Ss3tFuodunRHieLiNWB+kCEM4UFLUv\n\tfkKZQ7k+MChQL86c9xOjtAPMa+xCxWP2AJeWA6eY=","Date":"Wed, 30 Dec 2020 11:51:46 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20201228165600.53987-1-jacopo@jmondi.org>\n\t<20201228165600.53987-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201228165600.53987-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: camera_sensor:\n\tValidate driver support","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":14413,"web_url":"https://patchwork.libcamera.org/comment/14413/","msgid":"<20201230101607.vf35cvr35gbbhx54@uno.localdomain>","date":"2020-12-30T10:16:07","subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: camera_sensor:\n\tValidate driver support","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Wed, Dec 30, 2020 at 11:51:46AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Dec 28, 2020 at 05:55:56PM +0100, Jacopo Mondi wrote:\n> > The CameraSensor class requires the sensor driver to report\n> > information through V4L2 controls and through the V4L2 selection API,\n> > and uses those information to register Camera properties and to\n>\n> s/those information/that information/\n>\n> > construct CameraSensorInfo class instances to provide them to the IPA.\n> >\n> > Currently, validation of the kernel support happens each time a\n> > feature is requested, with slighly similar debug/error messages\n> > output to the user in case a feature is not supported.\n> >\n> > Rationalize this by:\n> > - Validate the sensor driver requirements in a single function\n> > - Expand the debug output when a property gets defaulted to a value\n> > - Be more verbose when constructing CameraSensorInfo is not possible\n> >\n> > Reviewed-by: Paul Elder \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/camera_sensor.h | 1 +\n> > src/libcamera/camera_sensor.cpp | 106 +++++++++++++++++++--\n> > 2 files changed, 100 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index f80d836161a5..aee10aa6e3c7 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -69,6 +69,7 @@ protected:\n> >\n> > private:\n> > \tint generateId();\n> > +\tint validateSensorDriver();\n> > \tint initProperties();\n> >\n> > \tconst MediaEntity *entity_;\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index e786821d4ba2..71d7c862e69a 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -207,6 +207,10 @@ int CameraSensor::init()\n> > \t */\n> > \tresolution_ = sizes_.back();\n> >\n> > +\tret = validateSensorDriver();\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > \tret = initProperties();\n> > \tif (ret)\n> > \t\treturn ret;\n> > @@ -214,6 +218,81 @@ int CameraSensor::init()\n> > \treturn 0;\n> > }\n> >\n> > +int CameraSensor::validateSensorDriver()\n> > +{\n> > +\t/*\n> > +\t * Make sure the sensor driver supports the mandatory controls\n> > +\t * required by the CameraSensor class.\n> > +\t * - V4L2_CID_PIXEL_RATE is used to calculate the sensor timings\n> > +\t * - V4L2_CID_HBLANK is used to calculate the line length\n> > +\t */\n> > +\tconst std::vector mandatoryControls{\n> > +\t\tV4L2_CID_PIXEL_RATE,\n> > +\t\tV4L2_CID_HBLANK,\n> > +\t};\n> > +\n> > +\tControlList ctrls = subdev_->getControls(mandatoryControls);\n> > +\tif (ctrls.empty()) {\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"Mandatory V4L2 controls not available\";\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"Please consider upgrading the sensor driver\";\n>\n> Maybe \"The sensor kernel driver needs to be fixed\" ? Users may wonder\n> what to upgrade to with a \"please upgrade\" message. But maybe I worry\n> too much :-) Up to you.\n\nWouldn't the user wonder what to fix with \"driver needs to be fixed\" :)\n\nAnyway, I can easily change the message\n\n>\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tint err = 0;\n> > +\t/*\n> > +\t * Optional controls are used to register optional sensor\n> > +\t * properties. If not present, some values will be defaulted.\n> > +\t */\n> > +\tconst std::vector optionalControls{\n> > +\t\tV4L2_CID_CAMERA_ORIENTATION,\n> > +\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> > +\t};\n> > +\n> > +\tctrls = subdev_->getControls(optionalControls);\n> > +\tif (ctrls.empty()) {\n> > +\t\tLOG(CameraSensor, Info)\n> > +\t\t\t<< \"Optional V4L2 controls not supported\";\n> > +\t\terr = -EINVAL;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Make sure the required selection targets are supported.\n> > +\t *\n> > +\t * Failures in reading any of the targets are not deemed to be fatal,\n> > +\t * but some properties and features, like constructing a\n> > +\t * CameraSensorInfo for the IPA module, won't be supported.\n> > +\t */\n> > +\tRectangle rect;\n> > +\tint ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect);\n> > +\tif (ret) {\n> > +\t\tLOG(CameraSensor, Info)\n> > +\t\t\t<< \"Failed to retrieve the readable pixel area size\";\n> > +\t\terr = -EINVAL;\n> > +\t}\n> > +\n> > +\tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);\n> > +\tif (ret) {\n> > +\t\tLOG(CameraSensor, Info)\n> > +\t\t\t<< \"Failed to retrieve the active pixel area size\";\n> > +\t\terr = -EINVAL;\n> > +\t}\n> > +\n> > +\tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect);\n> > +\tif (ret) {\n> > +\t\tLOG(CameraSensor, Info)\n> > +\t\t\t<< \"Failed to retreive the sensor crop rectangle\";\n> > +\t\terr = -EINVAL;\n> > +\t}\n> > +\n> > +\tif (err)\n> > +\t\tLOG(CameraSensor, Info)\n> > +\t\t\t<< \"Please consider upgrading the sensor driver\";\n>\n> Same as above.\n>\n> > +\n> > +\treturn 0;\n>\n> I think we need to make this fatal fairly soon, and I wonder whether we\n> could do so already. What platforms would we break ?\n>\n\nAt the moment Soraka for sure until three sensor patches I have out\nwon't be backported. RPi should be fine, Scarlet I have not checked\ntbh.\n\nI agree failing early is the most efficient way to have those sensor\ndrivers fixed\n\n> > +}\n> > +\n> > int CameraSensor::initProperties()\n> > {\n> > \t/*\n> > @@ -278,21 +357,29 @@ int CameraSensor::initProperties()\n> > \t\t}\n> > \t} else {\n> > \t\tpropertyValue = properties::CameraLocationFront;\n> > +\t\tLOG(CameraSensor, Debug)\n> > +\t\t\t<< \"Location property defaulted to 'Front Camera'\";\n>\n> If we make the failures above fatal, we will be able to drop the 'else'\n> branch here and below, right ?\n\nDepends if we want to make what I named \"optionalControls\" mandatory.\nIn this case we will break most platforms, as none (afaict) provides\nthe required information in the firmward (being OF for RPi or ACPI for\nSoraka)\n\n>\n> > \t}\n> > \tproperties_.set(properties::Location, propertyValue);\n> >\n> > \t/* Camera Rotation: default is 0 degrees. */\n> > \tconst auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> > -\tif (rotationControl != controls.end())\n> > +\tif (rotationControl != controls.end()) {\n> > \t\tpropertyValue = rotationControl->second.def().get();\n> > -\telse\n> > +\t} else {\n> > \t\tpropertyValue = 0;\n> > +\t\tLOG(CameraSensor, Debug)\n> > +\t\t\t<< \"Rotation property defaulted to 0 degrees\";\n> > +\t}\n> > \tproperties_.set(properties::Rotation, propertyValue);\n> >\n> > \tRectangle bounds;\n> > \tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &bounds);\n> > \tif (!ret)\n> > \t\tproperties_.set(properties::PixelArraySize, bounds.size());\n> > +\telse\n> > +\t\tLOG(CameraSensor, Debug)\n> > +\t\t\t<< \"PixelArraySize property not registered\";\n> >\n> > \tRectangle crop;\n> > \tret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &crop);\n> > @@ -306,6 +393,9 @@ int CameraSensor::initProperties()\n> > \t\tcrop.x -= bounds.x;\n> > \t\tcrop.y -= bounds.y;\n> > \t\tproperties_.set(properties::PixelArrayActiveAreas, { crop });\n> > +\t} else {\n> > +\t\tLOG(CameraSensor, Debug)\n> > +\t\t\t<< \"PixelArrayActiveAreas property not registered\";\n> > \t}\n> >\n> > \t/* Color filter array pattern, register only for RAW sensors. */\n> > @@ -569,6 +659,8 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\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 active area\";\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"The IPA might not work correctly\";\n>\n> Do we need this message and the one below ? In those error paths\n> sensorInfo() returns an error, and the caller will fail. It's not that\n\ndepends on the caller implementation :)\n\n> the IPA may not work correctly, the whole camera configuration will fail\n> :-) I'm tempted to take the opposite approach: now that we validate that\n> the sensor driver provides the right API, we could have less verbose\n> messages here. I'd drop the \"Failed to construct camera sensor info:\"\n> prefix, turning this particular error message into\n>\n> \t\tLOG(CameraSensor, Error) << \"Failed to get active area\";\n>\n> and similarly below.\n\nMakes sense, we are verbose enough during validation\nI'll make these shorter.\n\n>\n> >\n> > \t\treturn ret;\n> > \t}\n> > @@ -580,6 +672,8 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\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\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"The IPA might not work correctly\";\n> > \t\treturn ret;\n> > \t}\n> >\n> > @@ -601,16 +695,14 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\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> > +\t/* Retrieve the pixel rate and the line length through V4L2 controls. */\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\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"The IPA might not work correctly\";\n> > \t\treturn -EINVAL;\n> > \t}\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 0B9B6C0F1A\n\tfor ;\n\tWed, 30 Dec 2020 10:15:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 94734615D2;\n\tWed, 30 Dec 2020 11:15:54 +0100 (CET)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9B2F16031B\n\tfor ;\n\tWed, 30 Dec 2020 11:15:53 +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 relay10.mail.gandi.net (Postfix) with ESMTPSA id 269AB24000A;\n\tWed, 30 Dec 2020 10:15:52 +0000 (UTC)"],"Date":"Wed, 30 Dec 2020 11:16:07 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201230101607.vf35cvr35gbbhx54@uno.localdomain>","References":"<20201228165600.53987-1-jacopo@jmondi.org>\n\t<20201228165600.53987-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: camera_sensor:\n\tValidate driver support","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\" "}}]