[{"id":18776,"web_url":"https://patchwork.libcamera.org/comment/18776/","msgid":"","date":"2021-08-13T14:36:04","subject":"Re: [libcamera-devel] [PATCH] controls: Report required control\n\tnames","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Of course the title prefix of this should be\n\nlibcamera: camera_sensor:\n\n--\nKieran\n\n\nOn 13/08/2021 15:32, Kieran Bingham wrote:\n> The V4L2 controls which are optional, recommended, and mandatory report\n> errors if they are not found by CameraSensor::validateSensorDriver().\n> \n> These errors report hex values for the controls, which can easily lead\n> to confusion and incorrect assumption about which control needs to be\n> investigated.\n> \n> This can be seen occuring already in issues such as [0] and is generally\n> an unfriendly result to report to users in a warning or error message.\n> \n> Adapt the tables of controls to store both the id and name of the\n> controls to report a user friendly message that can ease diagnosis of\n> any CameraSensor validation issues.\n> \n> [0] https://github.com/raspberrypi/linux/issues/4500\n> \n> Signed-off-by: Kieran Bingham \n> \n> ---\n> I'm aware of course of the checkstyle changes suggested on V4L2_CID, but\n> I thought it was better to keep those macros 'scoped' here, and indented\n> accordingly.\n> ---\n> src/libcamera/camera_sensor.cpp | 45 ++++++++++++++++++++-------------\n> 1 file changed, 27 insertions(+), 18 deletions(-)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 7a3864150c89..1409da1b071c 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -168,19 +168,26 @@ int CameraSensor::validateSensorDriver()\n> {\n> \tint err = 0;\n> \n> +\tstruct v4l2_cid {\n> +\t\tconst uint32_t id;\n> +\t\tconst char *name;\n> +\t};\n> +\n> +\t#define V4L2_CID(x) { x, #x }\n> +\n> \t/*\n> \t * Optional controls are used to register optional sensor properties. If\n> \t * not present, some values will be defaulted.\n> \t */\n> -\tstatic constexpr uint32_t optionalControls[] = {\n> -\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> +\tstatic constexpr v4l2_cid optionalControls[] = {\n> +\t\tV4L2_CID(V4L2_CID_CAMERA_SENSOR_ROTATION),\n> \t};\n> \n> \tconst ControlIdMap &controls = subdev_->controls().idmap();\n> -\tfor (uint32_t ctrl : optionalControls) {\n> -\t\tif (!controls.count(ctrl))\n> +\tfor (auto &ctrl : optionalControls) {\n> +\t\tif (!controls.count(ctrl.id))\n> \t\t\tLOG(CameraSensor, Debug)\n> -\t\t\t\t<< \"Optional V4L2 control \" << utils::hex(ctrl)\n> +\t\t\t\t<< \"Optional V4L2 control \" << ctrl.name\n> \t\t\t\t<< \" not supported\";\n> \t}\n> \n> @@ -188,14 +195,14 @@ int CameraSensor::validateSensorDriver()\n> \t * Recommended controls are similar to optional controls, but will\n> \t * become mandatory in the near future. Be loud if they're missing.\n> \t */\n> -\tstatic constexpr uint32_t recommendedControls[] = {\n> -\t\tV4L2_CID_CAMERA_ORIENTATION,\n> +\tstatic constexpr v4l2_cid recommendedControls[] = {\n> +\t\tV4L2_CID(V4L2_CID_CAMERA_ORIENTATION),\n> \t};\n> \n> -\tfor (uint32_t ctrl : recommendedControls) {\n> -\t\tif (!controls.count(ctrl)) {\n> +\tfor (auto &ctrl : recommendedControls) {\n> +\t\tif (!controls.count(ctrl.id)) {\n> \t\t\tLOG(CameraSensor, Warning)\n> -\t\t\t\t<< \"Recommended V4L2 control \" << utils::hex(ctrl)\n> +\t\t\t\t<< \"Recommended V4L2 control \" << ctrl.name\n> \t\t\t\t<< \" not supported\";\n> \t\t\terr = -EINVAL;\n> \t\t}\n> @@ -259,18 +266,20 @@ int CameraSensor::validateSensorDriver()\n> \t * For raw sensors, make sure the sensor driver supports the controls\n> \t * required by the CameraSensor class.\n> \t */\n> -\tstatic constexpr uint32_t mandatoryControls[] = {\n> -\t\tV4L2_CID_EXPOSURE,\n> -\t\tV4L2_CID_HBLANK,\n> -\t\tV4L2_CID_PIXEL_RATE,\n> -\t\tV4L2_CID_VBLANK,\n> +\tstatic constexpr v4l2_cid mandatoryControls[] = {\n> +\t\tV4L2_CID(V4L2_CID_EXPOSURE),\n> +\t\tV4L2_CID(V4L2_CID_HBLANK),\n> +\t\tV4L2_CID(V4L2_CID_PIXEL_RATE),\n> +\t\tV4L2_CID(V4L2_CID_VBLANK),\n> \t};\n> \n> +\t#undef V4L2_CID\n> +\n> \terr = 0;\n> -\tfor (uint32_t ctrl : mandatoryControls) {\n> -\t\tif (!controls.count(ctrl)) {\n> +\tfor (auto &ctrl : mandatoryControls) {\n> +\t\tif (!controls.count(ctrl.id)) {\n> \t\t\tLOG(CameraSensor, Error)\n> -\t\t\t\t<< \"Mandatory V4L2 control \" << utils::hex(ctrl)\n> +\t\t\t\t<< \"Mandatory V4L2 control \" << ctrl.name\n> \t\t\t\t<< \" not available\";\n> \t\t\terr = -EINVAL;\n> \t\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 19688C3240\n\tfor ;\n\tFri, 13 Aug 2021 14:36:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A556E6888F;\n\tFri, 13 Aug 2021 16:36:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AB9CB687FA\n\tfor ;\n\tFri, 13 Aug 2021 16:36:07 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 35B6E8F;\n\tFri, 13 Aug 2021 16:36:07 +0200 (CEST)"],"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=\"LNcs1Gow\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628865367;\n\tbh=OA1JIFS7IKIezaYcsQHTS7nYHMObyGP4zurf7eOoA1o=;\n\th=From:Subject:To:References:Date:In-Reply-To:From;\n\tb=LNcs1GowezgSk4BqM1NvacH2A+z+NHDQCOvop3Z3tfv7ONk8kdkCpG0Kl61B8Orrh\n\t5lFiFI2NSafUQHI1REQu/8VM9pROIoNvhOr8rVjg/bNTfxp401edgj4md8ODlaooOw\n\tfd5TZD8ggSyhUSsW9gUgYJ4GIXDrnvwHQ/ya4k4U=","From":"Kieran Bingham ","To":"Kieran Bingham ,\n\tLibCamera Devel ","References":"<20210813143241.3528968-1-kieran.bingham@ideasonboard.com>","Message-ID":"","Date":"Fri, 13 Aug 2021 15:36:04 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210813143241.3528968-1-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] controls: Report required control\n\tnames","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18779,"web_url":"https://patchwork.libcamera.org/comment/18779/","msgid":"","date":"2021-08-13T14:45:00","subject":"Re: [libcamera-devel] [PATCH] controls: Report required control\n\tnames","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Fri, Aug 13, 2021 at 03:32:41PM +0100, Kieran Bingham wrote:\n> The V4L2 controls which are optional, recommended, and mandatory report\n> errors if they are not found by CameraSensor::validateSensorDriver().\n> \n> These errors report hex values for the controls, which can easily lead\n> to confusion and incorrect assumption about which control needs to be\n> investigated.\n> \n> This can be seen occuring already in issues such as [0] and is generally\n> an unfriendly result to report to users in a warning or error message.\n> \n> Adapt the tables of controls to store both the id and name of the\n> controls to report a user friendly message that can ease diagnosis of\n> any CameraSensor validation issues.\n> \n> [0] https://github.com/raspberrypi/linux/issues/4500\n> \n> Signed-off-by: Kieran Bingham \n\nI'm not fond of this ad-hoc fix, as we would need similar code in other\nlocations then. I'd rather try to revive the patch that creates Control\ninstances for V4L2 controls.\n\n> ---\n> I'm aware of course of the checkstyle changes suggested on V4L2_CID, but\n> I thought it was better to keep those macros 'scoped' here, and indented\n> accordingly.\n> ---\n> src/libcamera/camera_sensor.cpp | 45 ++++++++++++++++++++-------------\n> 1 file changed, 27 insertions(+), 18 deletions(-)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 7a3864150c89..1409da1b071c 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -168,19 +168,26 @@ int CameraSensor::validateSensorDriver()\n> {\n> \tint err = 0;\n> \n> +\tstruct v4l2_cid {\n> +\t\tconst uint32_t id;\n> +\t\tconst char *name;\n> +\t};\n> +\n> +\t#define V4L2_CID(x) { x, #x }\n> +\n> \t/*\n> \t * Optional controls are used to register optional sensor properties. If\n> \t * not present, some values will be defaulted.\n> \t */\n> -\tstatic constexpr uint32_t optionalControls[] = {\n> -\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> +\tstatic constexpr v4l2_cid optionalControls[] = {\n> +\t\tV4L2_CID(V4L2_CID_CAMERA_SENSOR_ROTATION),\n> \t};\n> \n> \tconst ControlIdMap &controls = subdev_->controls().idmap();\n> -\tfor (uint32_t ctrl : optionalControls) {\n> -\t\tif (!controls.count(ctrl))\n> +\tfor (auto &ctrl : optionalControls) {\n> +\t\tif (!controls.count(ctrl.id))\n> \t\t\tLOG(CameraSensor, Debug)\n> -\t\t\t\t<< \"Optional V4L2 control \" << utils::hex(ctrl)\n> +\t\t\t\t<< \"Optional V4L2 control \" << ctrl.name\n> \t\t\t\t<< \" not supported\";\n> \t}\n> \n> @@ -188,14 +195,14 @@ int CameraSensor::validateSensorDriver()\n> \t * Recommended controls are similar to optional controls, but will\n> \t * become mandatory in the near future. Be loud if they're missing.\n> \t */\n> -\tstatic constexpr uint32_t recommendedControls[] = {\n> -\t\tV4L2_CID_CAMERA_ORIENTATION,\n> +\tstatic constexpr v4l2_cid recommendedControls[] = {\n> +\t\tV4L2_CID(V4L2_CID_CAMERA_ORIENTATION),\n> \t};\n> \n> -\tfor (uint32_t ctrl : recommendedControls) {\n> -\t\tif (!controls.count(ctrl)) {\n> +\tfor (auto &ctrl : recommendedControls) {\n> +\t\tif (!controls.count(ctrl.id)) {\n> \t\t\tLOG(CameraSensor, Warning)\n> -\t\t\t\t<< \"Recommended V4L2 control \" << utils::hex(ctrl)\n> +\t\t\t\t<< \"Recommended V4L2 control \" << ctrl.name\n> \t\t\t\t<< \" not supported\";\n> \t\t\terr = -EINVAL;\n> \t\t}\n> @@ -259,18 +266,20 @@ int CameraSensor::validateSensorDriver()\n> \t * For raw sensors, make sure the sensor driver supports the controls\n> \t * required by the CameraSensor class.\n> \t */\n> -\tstatic constexpr uint32_t mandatoryControls[] = {\n> -\t\tV4L2_CID_EXPOSURE,\n> -\t\tV4L2_CID_HBLANK,\n> -\t\tV4L2_CID_PIXEL_RATE,\n> -\t\tV4L2_CID_VBLANK,\n> +\tstatic constexpr v4l2_cid mandatoryControls[] = {\n> +\t\tV4L2_CID(V4L2_CID_EXPOSURE),\n> +\t\tV4L2_CID(V4L2_CID_HBLANK),\n> +\t\tV4L2_CID(V4L2_CID_PIXEL_RATE),\n> +\t\tV4L2_CID(V4L2_CID_VBLANK),\n> \t};\n> \n> +\t#undef V4L2_CID\n> +\n> \terr = 0;\n> -\tfor (uint32_t ctrl : mandatoryControls) {\n> -\t\tif (!controls.count(ctrl)) {\n> +\tfor (auto &ctrl : mandatoryControls) {\n> +\t\tif (!controls.count(ctrl.id)) {\n> \t\t\tLOG(CameraSensor, Error)\n> -\t\t\t\t<< \"Mandatory V4L2 control \" << utils::hex(ctrl)\n> +\t\t\t\t<< \"Mandatory V4L2 control \" << ctrl.name\n> \t\t\t\t<< \" not available\";\n> \t\t\terr = -EINVAL;\n> \t\t}","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 CCAD7C3240\n\tfor ;\n\tFri, 13 Aug 2021 14:45:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 88DA86888F;\n\tFri, 13 Aug 2021 16:45:07 +0200 (CEST)","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 35319687FA\n\tfor ;\n\tFri, 13 Aug 2021 16:45:06 +0200 (CEST)","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 C596B8F;\n\tFri, 13 Aug 2021 16:45:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"TLbzyHeg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628865906;\n\tbh=2JCu6fGkbVPhkRSFxlRCKlvUcRs9cAFmYKn5BX2zoJo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TLbzyHegGkgAfrHWH+j+OgtbmWv70n7zHzGQI50xa6ZW1yM/Gesq7rvKOGa7oo7zU\n\tZyGJ10cXi+NmCgIfqNnpQRMP7c9IXUXpmxP2a/zIGP9vjvhfAcHyyG22bkVNzw87yU\n\tZM01MIA32aNxteOr5FmXOjmwsI0yZvgsYc3WIIPo=","Date":"Fri, 13 Aug 2021 17:45:00 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210813143241.3528968-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210813143241.3528968-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] controls: Report required control\n\tnames","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18780,"web_url":"https://patchwork.libcamera.org/comment/18780/","msgid":"","date":"2021-08-13T14:47:36","subject":"Re: [libcamera-devel] [PATCH] controls: Report required control\n\tnames","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 13/08/2021 15:45, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Fri, Aug 13, 2021 at 03:32:41PM +0100, Kieran Bingham wrote:\n>> The V4L2 controls which are optional, recommended, and mandatory report\n>> errors if they are not found by CameraSensor::validateSensorDriver().\n>>\n>> These errors report hex values for the controls, which can easily lead\n>> to confusion and incorrect assumption about which control needs to be\n>> investigated.\n>>\n>> This can be seen occuring already in issues such as [0] and is generally\n>> an unfriendly result to report to users in a warning or error message.\n>>\n>> Adapt the tables of controls to store both the id and name of the\n>> controls to report a user friendly message that can ease diagnosis of\n>> any CameraSensor validation issues.\n>>\n>> [0] https://github.com/raspberrypi/linux/issues/4500\n>>\n>> Signed-off-by: Kieran Bingham \n> \n> I'm not fond of this ad-hoc fix, as we would need similar code in other\n> locations then. I'd rather try to revive the patch that creates Control\n> instances for V4L2 controls.\n\n\nI posted something like this ... many months ago, and it didtn' get\nintegrated for probably similar push back.\n\nYet - here we are - still lacking a /very simple/ key feature.\n\nLeaving the debug^WError logs giving people hex codes to decipher.\n\nHow about I add a \\todo Remove this when V4L2 controsl print their names\nnatively ;-)\n\nI want /something/\n\nYou're blocked/ not keen on your V4L2 Control implementation because it\ndoes things you don't like, so I have no idea when you'll merge that.\n\n--\nKieran\n\n\n> \n>> ---\n>> I'm aware of course of the checkstyle changes suggested on V4L2_CID, but\n>> I thought it was better to keep those macros 'scoped' here, and indented\n>> accordingly.\n>> ---\n>> src/libcamera/camera_sensor.cpp | 45 ++++++++++++++++++++-------------\n>> 1 file changed, 27 insertions(+), 18 deletions(-)\n>>\n>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n>> index 7a3864150c89..1409da1b071c 100644\n>> --- a/src/libcamera/camera_sensor.cpp\n>> +++ b/src/libcamera/camera_sensor.cpp\n>> @@ -168,19 +168,26 @@ int CameraSensor::validateSensorDriver()\n>> {\n>> \tint err = 0;\n>> \n>> +\tstruct v4l2_cid {\n>> +\t\tconst uint32_t id;\n>> +\t\tconst char *name;\n>> +\t};\n>> +\n>> +\t#define V4L2_CID(x) { x, #x }\n>> +\n>> \t/*\n>> \t * Optional controls are used to register optional sensor properties. If\n>> \t * not present, some values will be defaulted.\n>> \t */\n>> -\tstatic constexpr uint32_t optionalControls[] = {\n>> -\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n>> +\tstatic constexpr v4l2_cid optionalControls[] = {\n>> +\t\tV4L2_CID(V4L2_CID_CAMERA_SENSOR_ROTATION),\n>> \t};\n>> \n>> \tconst ControlIdMap &controls = subdev_->controls().idmap();\n>> -\tfor (uint32_t ctrl : optionalControls) {\n>> -\t\tif (!controls.count(ctrl))\n>> +\tfor (auto &ctrl : optionalControls) {\n>> +\t\tif (!controls.count(ctrl.id))\n>> \t\t\tLOG(CameraSensor, Debug)\n>> -\t\t\t\t<< \"Optional V4L2 control \" << utils::hex(ctrl)\n>> +\t\t\t\t<< \"Optional V4L2 control \" << ctrl.name\n>> \t\t\t\t<< \" not supported\";\n>> \t}\n>> \n>> @@ -188,14 +195,14 @@ int CameraSensor::validateSensorDriver()\n>> \t * Recommended controls are similar to optional controls, but will\n>> \t * become mandatory in the near future. Be loud if they're missing.\n>> \t */\n>> -\tstatic constexpr uint32_t recommendedControls[] = {\n>> -\t\tV4L2_CID_CAMERA_ORIENTATION,\n>> +\tstatic constexpr v4l2_cid recommendedControls[] = {\n>> +\t\tV4L2_CID(V4L2_CID_CAMERA_ORIENTATION),\n>> \t};\n>> \n>> -\tfor (uint32_t ctrl : recommendedControls) {\n>> -\t\tif (!controls.count(ctrl)) {\n>> +\tfor (auto &ctrl : recommendedControls) {\n>> +\t\tif (!controls.count(ctrl.id)) {\n>> \t\t\tLOG(CameraSensor, Warning)\n>> -\t\t\t\t<< \"Recommended V4L2 control \" << utils::hex(ctrl)\n>> +\t\t\t\t<< \"Recommended V4L2 control \" << ctrl.name\n>> \t\t\t\t<< \" not supported\";\n>> \t\t\terr = -EINVAL;\n>> \t\t}\n>> @@ -259,18 +266,20 @@ int CameraSensor::validateSensorDriver()\n>> \t * For raw sensors, make sure the sensor driver supports the controls\n>> \t * required by the CameraSensor class.\n>> \t */\n>> -\tstatic constexpr uint32_t mandatoryControls[] = {\n>> -\t\tV4L2_CID_EXPOSURE,\n>> -\t\tV4L2_CID_HBLANK,\n>> -\t\tV4L2_CID_PIXEL_RATE,\n>> -\t\tV4L2_CID_VBLANK,\n>> +\tstatic constexpr v4l2_cid mandatoryControls[] = {\n>> +\t\tV4L2_CID(V4L2_CID_EXPOSURE),\n>> +\t\tV4L2_CID(V4L2_CID_HBLANK),\n>> +\t\tV4L2_CID(V4L2_CID_PIXEL_RATE),\n>> +\t\tV4L2_CID(V4L2_CID_VBLANK),\n>> \t};\n>> \n>> +\t#undef V4L2_CID\n>> +\n>> \terr = 0;\n>> -\tfor (uint32_t ctrl : mandatoryControls) {\n>> -\t\tif (!controls.count(ctrl)) {\n>> +\tfor (auto &ctrl : mandatoryControls) {\n>> +\t\tif (!controls.count(ctrl.id)) {\n>> \t\t\tLOG(CameraSensor, Error)\n>> -\t\t\t\t<< \"Mandatory V4L2 control \" << utils::hex(ctrl)\n>> +\t\t\t\t<< \"Mandatory V4L2 control \" << ctrl.name\n>> \t\t\t\t<< \" not available\";\n>> \t\t\terr = -EINVAL;\n>> \t\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 BC055BD87D\n\tfor ;\n\tFri, 13 Aug 2021 14:47:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 17E336888F;\n\tFri, 13 Aug 2021 16:47:41 +0200 (CEST)","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 CD71F687FA\n\tfor ;\n\tFri, 13 Aug 2021 16:47:39 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 565A58F;\n\tFri, 13 Aug 2021 16:47:39 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QJzaPsSV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628866059;\n\tbh=BEjhx1Ej1Y0ZfRk2v9/cyu0SLemTAutXCj9LdEC9YeM=;\n\th=From:Subject:To:Cc:References:Date:In-Reply-To:From;\n\tb=QJzaPsSVpVU3p2YXuGx+w8GyyZcx0LEvB2nxwUEaIqY9iZvFRw7n+wbncAW0rmWdq\n\t34m7vgeQfr20tTz5R4mTnti01taOMXMyLv33riqbtyMR0pdsx9GvxKuwwlFhnw8OSF\n\t+sN9bo6RHD8tExLOntzd+Ts3eGb2yM7EK7mhPnO0=","From":"Kieran Bingham ","To":"Laurent Pinchart ","References":"<20210813143241.3528968-1-kieran.bingham@ideasonboard.com>\n\t","Message-ID":"","Date":"Fri, 13 Aug 2021 15:47:36 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] controls: Report required control\n\tnames","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18787,"web_url":"https://patchwork.libcamera.org/comment/18787/","msgid":"","date":"2021-08-13T23:58:53","subject":"Re: [libcamera-devel] [PATCH] controls: Report required control\n\tnames","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Aug 13, 2021 at 03:47:36PM +0100, Kieran Bingham wrote:\n> On 13/08/2021 15:45, Laurent Pinchart wrote:\n> > On Fri, Aug 13, 2021 at 03:32:41PM +0100, Kieran Bingham wrote:\n> >> The V4L2 controls which are optional, recommended, and mandatory report\n> >> errors if they are not found by CameraSensor::validateSensorDriver().\n> >>\n> >> These errors report hex values for the controls, which can easily lead\n> >> to confusion and incorrect assumption about which control needs to be\n> >> investigated.\n> >>\n> >> This can be seen occuring already in issues such as [0] and is generally\n> >> an unfriendly result to report to users in a warning or error message.\n> >>\n> >> Adapt the tables of controls to store both the id and name of the\n> >> controls to report a user friendly message that can ease diagnosis of\n> >> any CameraSensor validation issues.\n> >>\n> >> [0] https://github.com/raspberrypi/linux/issues/4500\n> >>\n> >> Signed-off-by: Kieran Bingham \n> > \n> > I'm not fond of this ad-hoc fix, as we would need similar code in other\n> > locations then. I'd rather try to revive the patch that creates Control\n> > instances for V4L2 controls.\n> \n> I posted something like this ... many months ago, and it didtn' get\n> integrated for probably similar push back.\n> \n> Yet - here we are - still lacking a /very simple/ key feature.\n> \n> Leaving the debug^WError logs giving people hex codes to decipher.\n> \n> How about I add a \\todo Remove this when V4L2 controsl print their names\n> natively ;-)\n> \n> I want /something/\n> \n> You're blocked/ not keen on your V4L2 Control implementation because it\n> does things you don't like, so I have no idea when you'll merge that.\n\nI've rebased that patch series, and I think I like it even less than\nbefore :-S I'd rather try to move away from V4L2 in IPA modules if\npossible, handling V4L2 in the pipeline handlers only.\n\nIn any case, as that series isn't about the get merged, let's try to\nfind a solution for the problem at hand.\n\nWhat would you think about moving control names to v4l2_device.cpp, with\na static function to lookup the name based on the ID ?\n\n> >> ---\n> >> I'm aware of course of the checkstyle changes suggested on V4L2_CID, but\n> >> I thought it was better to keep those macros 'scoped' here, and indented\n> >> accordingly.\n> >> ---\n> >> src/libcamera/camera_sensor.cpp | 45 ++++++++++++++++++++-------------\n> >> 1 file changed, 27 insertions(+), 18 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> >> index 7a3864150c89..1409da1b071c 100644\n> >> --- a/src/libcamera/camera_sensor.cpp\n> >> +++ b/src/libcamera/camera_sensor.cpp\n> >> @@ -168,19 +168,26 @@ int CameraSensor::validateSensorDriver()\n> >> {\n> >> \tint err = 0;\n> >> \n> >> +\tstruct v4l2_cid {\n> >> +\t\tconst uint32_t id;\n> >> +\t\tconst char *name;\n> >> +\t};\n> >> +\n> >> +\t#define V4L2_CID(x) { x, #x }\n> >> +\n> >> \t/*\n> >> \t * Optional controls are used to register optional sensor properties. If\n> >> \t * not present, some values will be defaulted.\n> >> \t */\n> >> -\tstatic constexpr uint32_t optionalControls[] = {\n> >> -\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> >> +\tstatic constexpr v4l2_cid optionalControls[] = {\n> >> +\t\tV4L2_CID(V4L2_CID_CAMERA_SENSOR_ROTATION),\n> >> \t};\n> >> \n> >> \tconst ControlIdMap &controls = subdev_->controls().idmap();\n> >> -\tfor (uint32_t ctrl : optionalControls) {\n> >> -\t\tif (!controls.count(ctrl))\n> >> +\tfor (auto &ctrl : optionalControls) {\n> >> +\t\tif (!controls.count(ctrl.id))\n> >> \t\t\tLOG(CameraSensor, Debug)\n> >> -\t\t\t\t<< \"Optional V4L2 control \" << utils::hex(ctrl)\n> >> +\t\t\t\t<< \"Optional V4L2 control \" << ctrl.name\n> >> \t\t\t\t<< \" not supported\";\n> >> \t}\n> >> \n> >> @@ -188,14 +195,14 @@ int CameraSensor::validateSensorDriver()\n> >> \t * Recommended controls are similar to optional controls, but will\n> >> \t * become mandatory in the near future. Be loud if they're missing.\n> >> \t */\n> >> -\tstatic constexpr uint32_t recommendedControls[] = {\n> >> -\t\tV4L2_CID_CAMERA_ORIENTATION,\n> >> +\tstatic constexpr v4l2_cid recommendedControls[] = {\n> >> +\t\tV4L2_CID(V4L2_CID_CAMERA_ORIENTATION),\n> >> \t};\n> >> \n> >> -\tfor (uint32_t ctrl : recommendedControls) {\n> >> -\t\tif (!controls.count(ctrl)) {\n> >> +\tfor (auto &ctrl : recommendedControls) {\n> >> +\t\tif (!controls.count(ctrl.id)) {\n> >> \t\t\tLOG(CameraSensor, Warning)\n> >> -\t\t\t\t<< \"Recommended V4L2 control \" << utils::hex(ctrl)\n> >> +\t\t\t\t<< \"Recommended V4L2 control \" << ctrl.name\n> >> \t\t\t\t<< \" not supported\";\n> >> \t\t\terr = -EINVAL;\n> >> \t\t}\n> >> @@ -259,18 +266,20 @@ int CameraSensor::validateSensorDriver()\n> >> \t * For raw sensors, make sure the sensor driver supports the controls\n> >> \t * required by the CameraSensor class.\n> >> \t */\n> >> -\tstatic constexpr uint32_t mandatoryControls[] = {\n> >> -\t\tV4L2_CID_EXPOSURE,\n> >> -\t\tV4L2_CID_HBLANK,\n> >> -\t\tV4L2_CID_PIXEL_RATE,\n> >> -\t\tV4L2_CID_VBLANK,\n> >> +\tstatic constexpr v4l2_cid mandatoryControls[] = {\n> >> +\t\tV4L2_CID(V4L2_CID_EXPOSURE),\n> >> +\t\tV4L2_CID(V4L2_CID_HBLANK),\n> >> +\t\tV4L2_CID(V4L2_CID_PIXEL_RATE),\n> >> +\t\tV4L2_CID(V4L2_CID_VBLANK),\n> >> \t};\n> >> \n> >> +\t#undef V4L2_CID\n> >> +\n> >> \terr = 0;\n> >> -\tfor (uint32_t ctrl : mandatoryControls) {\n> >> -\t\tif (!controls.count(ctrl)) {\n> >> +\tfor (auto &ctrl : mandatoryControls) {\n> >> +\t\tif (!controls.count(ctrl.id)) {\n> >> \t\t\tLOG(CameraSensor, Error)\n> >> -\t\t\t\t<< \"Mandatory V4L2 control \" << utils::hex(ctrl)\n> >> +\t\t\t\t<< \"Mandatory V4L2 control \" << ctrl.name\n> >> \t\t\t\t<< \" not available\";\n> >> \t\t\terr = -EINVAL;\n> >> \t\t}","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 F395BBD87D\n\tfor ;\n\tFri, 13 Aug 2021 23:59:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6FA916888F;\n\tSat, 14 Aug 2021 01:59:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D4B9F68823\n\tfor ;\n\tSat, 14 Aug 2021 01:58:58 +0200 (CEST)","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 52C5E3F0;\n\tSat, 14 Aug 2021 01:58:58 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dDhK0UHO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628899138;\n\tbh=oA8knTFOCFpiLGJyNZkksyC6ETrqOMFMGnjMM2wfoS8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dDhK0UHOMXIjqcZPGOkHXYDtPeIdK/P6EWQMh0T6k3VeVYdPyJSJcP0AQiY2OZYMZ\n\tWbor2i3MOoNCf0iRVmsdLiIIJuJux6sBU2cYViS7Ocv/DxYDPBxUd1Z9uF9ZbyGgAl\n\t4bY7ZdqIy9rcofILOSAYYQ3lZpGzV/n8byRGBkWE=","Date":"Sat, 14 Aug 2021 02:58:53 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210813143241.3528968-1-kieran.bingham@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] controls: Report required control\n\tnames","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]