[{"id":15032,"web_url":"https://patchwork.libcamera.org/comment/15032/","msgid":"","date":"2021-02-06T07:43:53","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: camera_sensor: Report\n\tmandatory control names","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, Feb 05, 2021 at 05:03:17PM +0000, Kieran Bingham wrote:\n> We can not obtain the control names directly from V4L2 so create\n> a map of the control name, when declaring the list of mandatory\n> controls to enable a human readable print of any missing requirements.\n\nIf we want to print V4L2 controls by name to ease debugging, it would be\nbest not to limit the feature to this particular instance. I've been\ntoying for a long time with the idea of creating Control instances for\nV4L2 controls, which would also allow the usage of a nicer get() and\nset() in the ControlList class.\n\n> Signed-off-by: Kieran Bingham \n> ---\n> src/libcamera/camera_sensor.cpp | 21 +++++++++++++--------\n> 1 file changed, 13 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index c9e8d49b7935..9a510108f171 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -346,18 +346,23 @@ 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 struct {\n> +\t\tuint32_t ctrl;\n> +\t\tstd::string name;\n> +\t} mandatoryControls[] = {\n> +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl }\n> +\t\tMANDATORY_CONTROL(V4L2_CID_EXPOSURE),\n> +\t\tMANDATORY_CONTROL(V4L2_CID_HBLANK),\n> +\t\tMANDATORY_CONTROL(V4L2_CID_PIXEL_RATE),\n> +\t\tMANDATORY_CONTROL(V4L2_CID_VBLANK),\n> +#undef MANDATORY_CONTROL\n> \t};\n> \n> \terr = 0;\n> -\tfor (uint32_t ctrl : mandatoryControls) {\n> -\t\tif (!controls.count(ctrl)) {\n> +\tfor (auto &c : mandatoryControls) {\n> +\t\tif (!controls.count(c.ctrl)) {\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 \" << c.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 61500BD162\n\tfor ;\n\tSat, 6 Feb 2021 07:44:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C4BB8614BC;\n\tSat, 6 Feb 2021 08:44:18 +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 3829E60301\n\tfor ;\n\tSat, 6 Feb 2021 08:44:17 +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 91C2D580;\n\tSat, 6 Feb 2021 08:44:16 +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=\"qhjnG31r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612597456;\n\tbh=SChfuTDKUWg/pFRjhEXRdgbwXBOTNj1HzM/WlNYUYJU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qhjnG31rZWzo53/CpKOWHw5yJ6zevQci+5Pz2UfPZgob+B640AbQO96no3mnP6ZbU\n\t3MBOj4Mpqan8T0Ad3Vu2W7rNtE3HIQHAeJO9+CPwsC1uoVB0l180e1DGDC2sYvkU8R\n\tCvkImBnXa+CT24RRaxGLdXZ5YUmGQRXVM1B/lkdY=","Date":"Sat, 6 Feb 2021 09:43:53 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210205170317.267605-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210205170317.267605-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: camera_sensor: Report\n\tmandatory control names","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15033,"web_url":"https://patchwork.libcamera.org/comment/15033/","msgid":"","date":"2021-02-06T10:12:37","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: camera_sensor: Report\n\tmandatory control names","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 06/02/2021 07:43, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Fri, Feb 05, 2021 at 05:03:17PM +0000, Kieran Bingham wrote:\n>> We can not obtain the control names directly from V4L2 so create\n>> a map of the control name, when declaring the list of mandatory\n>> controls to enable a human readable print of any missing requirements.\n> \n> If we want to print V4L2 controls by name to ease debugging, it would be\n> best not to limit the feature to this particular instance. I've been\n> toying for a long time with the idea of creating Control instances for\n> V4L2 controls, which would also allow the usage of a nicer get() and\n> set() in the ControlList class.\n\nBut we already establish controls with names from V4L2 when they are\navailable don't we?\n\nIn src/libcamera/v4l2_controls.cpp, there is a helper\n\n std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)\n\nwhich is used to establish a V4L2ControlId after it has been queried\nfrom the kernel.\n\nThe key issue here is that at the point of reporting the Mandatory\nControls are not available on the device - well - they don't exist and\nthus haven't been queried, so there is no name available.\n\n\nDo you envisage a static list of *all* v4l2 controls, over just the 4\nwhich are 'required' here? Or that we add all v4l2 controls we use to\nthe static declarations perhaps? (That could be handled just like the\nother controls then)\n\nRight now - with the recent addition of mandatory controls and many\nsensor drivers not supporting all the required controls, we have several\ninstances of people hitting failures and being told an obscure hex value\nin the error reporting, which is not very helpful to an end user trying\nto debug the issue of why their capture which worked a couple of weeks\nago now fails.\n\nI don't expect that we wish to provide a manual lookup service every\ntime someone reports or queries what those values are - so I'd hope we\ncan have a solution to this quite quickly.\n\n--\nKieran\n\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> src/libcamera/camera_sensor.cpp | 21 +++++++++++++--------\n>> 1 file changed, 13 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n>> index c9e8d49b7935..9a510108f171 100644\n>> --- a/src/libcamera/camera_sensor.cpp\n>> +++ b/src/libcamera/camera_sensor.cpp\n>> @@ -346,18 +346,23 @@ 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 struct {\n>> +\t\tuint32_t ctrl;\n>> +\t\tstd::string name;\n>> +\t} mandatoryControls[] = {\n>> +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl }\n>> +\t\tMANDATORY_CONTROL(V4L2_CID_EXPOSURE),\n>> +\t\tMANDATORY_CONTROL(V4L2_CID_HBLANK),\n>> +\t\tMANDATORY_CONTROL(V4L2_CID_PIXEL_RATE),\n>> +\t\tMANDATORY_CONTROL(V4L2_CID_VBLANK),\n>> +#undef MANDATORY_CONTROL\n>> \t};\n>> \n>> \terr = 0;\n>> -\tfor (uint32_t ctrl : mandatoryControls) {\n>> -\t\tif (!controls.count(ctrl)) {\n>> +\tfor (auto &c : mandatoryControls) {\n>> +\t\tif (!controls.count(c.ctrl)) {\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 \" << c.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 CB30DBD160\n\tfor ;\n\tSat, 6 Feb 2021 10:12:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 510DE614BC;\n\tSat, 6 Feb 2021 11:12:41 +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 16E87613F3\n\tfor ;\n\tSat, 6 Feb 2021 11:12:40 +0100 (CET)","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 7B2342C1;\n\tSat, 6 Feb 2021 11:12:39 +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=\"PYmxoSgh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612606359;\n\tbh=6q/2ASqi9qOynaIzb5gBXrJDgLpY7c+314TuntfVosk=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=PYmxoSghZwml+yBb8Z4Dd60q/I6EDha96SnzQKhExLzC58htb5XsLQfOepQQUVqDp\n\tlFGZQk53sQ5yaYjBGmkhaQc86t0gisAQZgr4o3HE1BAEsiN5vHdL8PV+atIZ0otDPY\n\tKVJ+zgNMvZ6EnqFxaMttPqWwZC+/KO4nb7aeJOj0=","To":"Laurent Pinchart ","References":"<20210205170317.267605-1-kieran.bingham@ideasonboard.com>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Sat, 6 Feb 2021 10:12:37 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: camera_sensor: Report\n\tmandatory control names","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15050,"web_url":"https://patchwork.libcamera.org/comment/15050/","msgid":"","date":"2021-02-08T12:20:57","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: camera_sensor: Report\n\tmandatory control names","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 06/02/2021 10:12, Kieran Bingham wrote:\n> Hi Laurent,\n> \n> On 06/02/2021 07:43, Laurent Pinchart wrote:\n>> Hi Kieran,\n>>\n>> Thank you for the patch.\n>>\n>> On Fri, Feb 05, 2021 at 05:03:17PM +0000, Kieran Bingham wrote:\n>>> We can not obtain the control names directly from V4L2 so create\n>>> a map of the control name, when declaring the list of mandatory\n>>> controls to enable a human readable print of any missing requirements.\n>>\n>> If we want to print V4L2 controls by name to ease debugging, it would be\n>> best not to limit the feature to this particular instance. I've been\n>> toying for a long time with the idea of creating Control instances for\n>> V4L2 controls, which would also allow the usage of a nicer get() and\n>> set() in the ControlList class.\n> \n> But we already establish controls with names from V4L2 when they are\n> available don't we?\n> \n> In src/libcamera/v4l2_controls.cpp, there is a helper\n> \n> std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)\n> \n> which is used to establish a V4L2ControlId after it has been queried\n> from the kernel.\n> \n> The key issue here is that at the point of reporting the Mandatory\n> Controls are not available on the device - well - they don't exist and\n> thus haven't been queried, so there is no name available.\n> \n> \n> Do you envisage a static list of *all* v4l2 controls, over just the 4\n> which are 'required' here? Or that we add all v4l2 controls we use to\n> the static declarations perhaps? (That could be handled just like the\n> other controls then)\n> \n> Right now - with the recent addition of mandatory controls and many\n> sensor drivers not supporting all the required controls, we have several\n> instances of people hitting failures and being told an obscure hex value\n> in the error reporting, which is not very helpful to an end user trying\n> to debug the issue of why their capture which worked a couple of weeks\n> ago now fails.\n> \n> I don't expect that we wish to provide a manual lookup service every\n> time someone reports or queries what those values are - so I'd hope we\n> can have a solution to this quite quickly.\n\nIndeed of course this is more than just the mandatory controls:\n\n\n> [0:51:10.226221013] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0922 not supported\n> [0:51:10.226238875] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0923 not supported\n\nIt might be optional, but it would be very nice to know what it is ;-)\n\n--\nKieran\n\n\n>>> Signed-off-by: Kieran Bingham \n>>> ---\n>>> src/libcamera/camera_sensor.cpp | 21 +++++++++++++--------\n>>> 1 file changed, 13 insertions(+), 8 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n>>> index c9e8d49b7935..9a510108f171 100644\n>>> --- a/src/libcamera/camera_sensor.cpp\n>>> +++ b/src/libcamera/camera_sensor.cpp\n>>> @@ -346,18 +346,23 @@ 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 struct {\n>>> +\t\tuint32_t ctrl;\n>>> +\t\tstd::string name;\n>>> +\t} mandatoryControls[] = {\n>>> +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl }\n>>> +\t\tMANDATORY_CONTROL(V4L2_CID_EXPOSURE),\n>>> +\t\tMANDATORY_CONTROL(V4L2_CID_HBLANK),\n>>> +\t\tMANDATORY_CONTROL(V4L2_CID_PIXEL_RATE),\n>>> +\t\tMANDATORY_CONTROL(V4L2_CID_VBLANK),\n>>> +#undef MANDATORY_CONTROL\n>>> \t};\n>>> \n>>> \terr = 0;\n>>> -\tfor (uint32_t ctrl : mandatoryControls) {\n>>> -\t\tif (!controls.count(ctrl)) {\n>>> +\tfor (auto &c : mandatoryControls) {\n>>> +\t\tif (!controls.count(c.ctrl)) {\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 \" << c.name\n>>> \t\t\t\t<< \" not available\";\n>>> \t\t\terr = -EINVAL;\n>>> \t\t}\n>>\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 CDAA1BD162\n\tfor ;\n\tMon, 8 Feb 2021 12:21:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5548760D20;\n\tMon, 8 Feb 2021 13:21:01 +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 6015E60D18\n\tfor ;\n\tMon, 8 Feb 2021 13:21:00 +0100 (CET)","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 C304E3D7;\n\tMon, 8 Feb 2021 13:20:59 +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=\"pbOJIA4L\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612786859;\n\tbh=MMKIJnB/O9hQZAXQM4qWNUfLTMVnoy4ZnSAJKpkE8pM=;\n\th=Reply-To:Subject:From:To:Cc:References:Date:In-Reply-To:From;\n\tb=pbOJIA4L7i4B/CBNAxoh1MR/O4k5FVS5XMJicfEVW/skhxwddXJRc7RIYGnSUefkE\n\tew7Oh5Un6/e0AyUcziEyG44jQANTbJh38vENsUFjwlx6bsjMN1JlD1qLUvELICoFPn\n\tBrg1MOUSViSwBKfujyE8cPvCtBU8r3vHvjojWO1E=","From":"Kieran Bingham ","To":"Laurent Pinchart ","References":"<20210205170317.267605-1-kieran.bingham@ideasonboard.com>\n\t\n\t","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Mon, 8 Feb 2021 12:20:57 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: camera_sensor: Report\n\tmandatory control names","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15057,"web_url":"https://patchwork.libcamera.org/comment/15057/","msgid":"<20210208161529.2vuiopaw3jhg7uhc@uno.localdomain>","date":"2021-02-08T16:15:29","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: camera_sensor: Report\n\tmandatory control names","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Mon, Feb 08, 2021 at 12:20:57PM +0000, Kieran Bingham wrote:\n> On 06/02/2021 10:12, Kieran Bingham wrote:\n> > Hi Laurent,\n> >\n> > On 06/02/2021 07:43, Laurent Pinchart wrote:\n> >> Hi Kieran,\n> >>\n> >> Thank you for the patch.\n> >>\n> >> On Fri, Feb 05, 2021 at 05:03:17PM +0000, Kieran Bingham wrote:\n> >>> We can not obtain the control names directly from V4L2 so create\n> >>> a map of the control name, when declaring the list of mandatory\n> >>> controls to enable a human readable print of any missing requirements.\n> >>\n> >> If we want to print V4L2 controls by name to ease debugging, it would be\n> >> best not to limit the feature to this particular instance. I've been\n> >> toying for a long time with the idea of creating Control instances for\n> >> V4L2 controls, which would also allow the usage of a nicer get() and\n> >> set() in the ControlList class.\n> >\n> > But we already establish controls with names from V4L2 when they are\n> > available don't we?\n> >\n> > In src/libcamera/v4l2_controls.cpp, there is a helper\n> >\n> > std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)\n> >\n> > which is used to establish a V4L2ControlId after it has been queried\n> > from the kernel.\n> >\n> > The key issue here is that at the point of reporting the Mandatory\n> > Controls are not available on the device - well - they don't exist and\n> > thus haven't been queried, so there is no name available.\n> >\n> >\n> > Do you envisage a static list of *all* v4l2 controls, over just the 4\n> > which are 'required' here? Or that we add all v4l2 controls we use to\n> > the static declarations perhaps? (That could be handled just like the\n> > other controls then)\n> >\n> > Right now - with the recent addition of mandatory controls and many\n> > sensor drivers not supporting all the required controls, we have several\n> > instances of people hitting failures and being told an obscure hex value\n> > in the error reporting, which is not very helpful to an end user trying\n> > to debug the issue of why their capture which worked a couple of weeks\n> > ago now fails.\n> >\n> > I don't expect that we wish to provide a manual lookup service every\n> > time someone reports or queries what those values are - so I'd hope we\n> > can have a solution to this quite quickly.\n>\n> Indeed of course this is more than just the mandatory controls:\n>\n>\n> > [0:51:10.226221013] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0922 not supported\n> > [0:51:10.226238875] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0923 not supported\n>\n> It might be optional, but it would be very nice to know what it is ;-)\n\nPersonal opinion: if one is interesting in knowing why the driver\nvalidation fails, I hope she would not only rely on the debug\nprintouts but rather turn to the code and cross-check for the controls\npresence in the driver. An ad-hoc solution for this is nice, but it's\na bit a waste of effort, considering how much nicer would our control\nframework be if we could get rid of the V4L2Control indexed by id and\ntreat all controls we deal with (libcamera and v4l2) as ControlId *\n\nWe import v4l2-controls.h so we have all we need to parse and create\nControlId instances at build time from there. It's not a 2 hours task\nunfortunately as it require quite some some of parsing and\nsubstitution.\n>\n> --\n> Kieran\n>\n>\n> >>> Signed-off-by: Kieran Bingham \n> >>> ---\n> >>> src/libcamera/camera_sensor.cpp | 21 +++++++++++++--------\n> >>> 1 file changed, 13 insertions(+), 8 deletions(-)\n> >>>\n> >>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> >>> index c9e8d49b7935..9a510108f171 100644\n> >>> --- a/src/libcamera/camera_sensor.cpp\n> >>> +++ b/src/libcamera/camera_sensor.cpp\n> >>> @@ -346,18 +346,23 @@ 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 struct {\n> >>> +\t\tuint32_t ctrl;\n> >>> +\t\tstd::string name;\n> >>> +\t} mandatoryControls[] = {\n> >>> +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl }\n> >>> +\t\tMANDATORY_CONTROL(V4L2_CID_EXPOSURE),\n> >>> +\t\tMANDATORY_CONTROL(V4L2_CID_HBLANK),\n> >>> +\t\tMANDATORY_CONTROL(V4L2_CID_PIXEL_RATE),\n> >>> +\t\tMANDATORY_CONTROL(V4L2_CID_VBLANK),\n> >>> +#undef MANDATORY_CONTROL\n> >>> \t};\n> >>>\n> >>> \terr = 0;\n> >>> -\tfor (uint32_t ctrl : mandatoryControls) {\n> >>> -\t\tif (!controls.count(ctrl)) {\n> >>> +\tfor (auto &c : mandatoryControls) {\n> >>> +\t\tif (!controls.count(c.ctrl)) {\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 \" << c.name\n> >>> \t\t\t\t<< \" not available\";\n> >>> \t\t\terr = -EINVAL;\n> >>> \t\t}\n> >>\n> >\n>\n> --\n> Regards\n> --\n> Kieran\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BA2DDBD162\n\tfor ;\n\tMon, 8 Feb 2021 16:15:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4FBFD60D33;\n\tMon, 8 Feb 2021 17:15:07 +0100 (CET)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1FD3B60D2B\n\tfor ;\n\tMon, 8 Feb 2021 17:15:06 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 49F1A20000D;\n\tMon, 8 Feb 2021 16:15:04 +0000 (UTC)"],"Date":"Mon, 8 Feb 2021 17:15:29 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20210208161529.2vuiopaw3jhg7uhc@uno.localdomain>","References":"<20210205170317.267605-1-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: camera_sensor: Report\n\tmandatory control names","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15060,"web_url":"https://patchwork.libcamera.org/comment/15060/","msgid":"","date":"2021-02-08T16:21:16","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: camera_sensor: Report\n\tmandatory control names","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Feb 08, 2021 at 05:15:29PM +0100, Jacopo Mondi wrote:\n> On Mon, Feb 08, 2021 at 12:20:57PM +0000, Kieran Bingham wrote:\n> > On 06/02/2021 10:12, Kieran Bingham wrote:\n> > > On 06/02/2021 07:43, Laurent Pinchart wrote:\n> > >> On Fri, Feb 05, 2021 at 05:03:17PM +0000, Kieran Bingham wrote:\n> > >>> We can not obtain the control names directly from V4L2 so create\n> > >>> a map of the control name, when declaring the list of mandatory\n> > >>> controls to enable a human readable print of any missing requirements.\n> > >>\n> > >> If we want to print V4L2 controls by name to ease debugging, it would be\n> > >> best not to limit the feature to this particular instance. I've been\n> > >> toying for a long time with the idea of creating Control instances for\n> > >> V4L2 controls, which would also allow the usage of a nicer get() and\n> > >> set() in the ControlList class.\n> > >\n> > > But we already establish controls with names from V4L2 when they are\n> > > available don't we?\n> > >\n> > > In src/libcamera/v4l2_controls.cpp, there is a helper\n> > >\n> > > std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)\n> > >\n> > > which is used to establish a V4L2ControlId after it has been queried\n> > > from the kernel.\n> > >\n> > > The key issue here is that at the point of reporting the Mandatory\n> > > Controls are not available on the device - well - they don't exist and\n> > > thus haven't been queried, so there is no name available.\n> > >\n> > >\n> > > Do you envisage a static list of *all* v4l2 controls, over just the 4\n> > > which are 'required' here? Or that we add all v4l2 controls we use to\n> > > the static declarations perhaps? (That could be handled just like the\n> > > other controls then)\n> > >\n> > > Right now - with the recent addition of mandatory controls and many\n> > > sensor drivers not supporting all the required controls, we have several\n> > > instances of people hitting failures and being told an obscure hex value\n> > > in the error reporting, which is not very helpful to an end user trying\n> > > to debug the issue of why their capture which worked a couple of weeks\n> > > ago now fails.\n> > >\n> > > I don't expect that we wish to provide a manual lookup service every\n> > > time someone reports or queries what those values are - so I'd hope we\n> > > can have a solution to this quite quickly.\n> >\n> > Indeed of course this is more than just the mandatory controls:\n> >\n> >\n> > > [0:51:10.226221013] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0922 not supported\n> > > [0:51:10.226238875] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0923 not supported\n> >\n> > It might be optional, but it would be very nice to know what it is ;-)\n> \n> Personal opinion: if one is interesting in knowing why the driver\n> validation fails, I hope she would not only rely on the debug\n> printouts but rather turn to the code and cross-check for the controls\n> presence in the driver. An ad-hoc solution for this is nice, but it's\n> a bit a waste of effort, considering how much nicer would our control\n> framework be if we could get rid of the V4L2Control indexed by id and\n> treat all controls we deal with (libcamera and v4l2) as ControlId *\n\nI think I generally agree.\n\n> We import v4l2-controls.h so we have all we need to parse and create\n> ControlId instances at build time from there. It's not a 2 hours task\n> unfortunately as it require quite some some of parsing and\n> substitution.\n\nWe need type information too, which we can't get from the\nv4l2-controls.h header. I was thus considering a YAML file listing the\nV4L2 controls we need.\n\n> > >>> Signed-off-by: Kieran Bingham \n> > >>> ---\n> > >>> src/libcamera/camera_sensor.cpp | 21 +++++++++++++--------\n> > >>> 1 file changed, 13 insertions(+), 8 deletions(-)\n> > >>>\n> > >>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > >>> index c9e8d49b7935..9a510108f171 100644\n> > >>> --- a/src/libcamera/camera_sensor.cpp\n> > >>> +++ b/src/libcamera/camera_sensor.cpp\n> > >>> @@ -346,18 +346,23 @@ 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 struct {\n> > >>> +\t\tuint32_t ctrl;\n> > >>> +\t\tstd::string name;\n> > >>> +\t} mandatoryControls[] = {\n> > >>> +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl }\n> > >>> +\t\tMANDATORY_CONTROL(V4L2_CID_EXPOSURE),\n> > >>> +\t\tMANDATORY_CONTROL(V4L2_CID_HBLANK),\n> > >>> +\t\tMANDATORY_CONTROL(V4L2_CID_PIXEL_RATE),\n> > >>> +\t\tMANDATORY_CONTROL(V4L2_CID_VBLANK),\n> > >>> +#undef MANDATORY_CONTROL\n> > >>> \t};\n> > >>>\n> > >>> \terr = 0;\n> > >>> -\tfor (uint32_t ctrl : mandatoryControls) {\n> > >>> -\t\tif (!controls.count(ctrl)) {\n> > >>> +\tfor (auto &c : mandatoryControls) {\n> > >>> +\t\tif (!controls.count(c.ctrl)) {\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 \" << c.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 3B4A2BD160\n\tfor ;\n\tMon, 8 Feb 2021 16:21:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DCB4760D37;\n\tMon, 8 Feb 2021 17:21:41 +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 8A80960D24\n\tfor ;\n\tMon, 8 Feb 2021 17:21:40 +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 0284A3D7;\n\tMon, 8 Feb 2021 17:21:39 +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=\"cpQQkt3y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612801300;\n\tbh=ZKvUp63Z6yScS+IBc9+I7jgBTvleOhxf9a8YIsw2ex0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cpQQkt3yatbbLreZeaqiiqpm83B0iXEONFmdSj72SZ9rk0VDhVTTlmkg9fZUL+rdf\n\tFYTg+UXXWN1uMuD3/TnqtFdMwbbKYi7nhsoGypb5+aW189KRf//0VpyMKcuT7ZUieq\n\tudzHaMUEhUk/6qgVyCxUFGTJv/kjtq+6+xK8eHuM=","Date":"Mon, 8 Feb 2021 18:21:16 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210205170317.267605-1-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t\n\t<20210208161529.2vuiopaw3jhg7uhc@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210208161529.2vuiopaw3jhg7uhc@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: camera_sensor: Report\n\tmandatory control names","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15061,"web_url":"https://patchwork.libcamera.org/comment/15061/","msgid":"<2b6bf91d-b2ab-3205-abeb-a3b5b991626a@ideasonboard.com>","date":"2021-02-08T16:47:46","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: camera_sensor: Report\n\tmandatory control names","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi,\n\nOn 08/02/2021 16:21, Laurent Pinchart wrote:\n> Hello,\n> \n> On Mon, Feb 08, 2021 at 05:15:29PM +0100, Jacopo Mondi wrote:\n>> On Mon, Feb 08, 2021 at 12:20:57PM +0000, Kieran Bingham wrote:\n>>> On 06/02/2021 10:12, Kieran Bingham wrote:\n>>>> On 06/02/2021 07:43, Laurent Pinchart wrote:\n>>>>> On Fri, Feb 05, 2021 at 05:03:17PM +0000, Kieran Bingham wrote:\n>>>>>> We can not obtain the control names directly from V4L2 so create\n>>>>>> a map of the control name, when declaring the list of mandatory\n>>>>>> controls to enable a human readable print of any missing requirements.\n>>>>>\n>>>>> If we want to print V4L2 controls by name to ease debugging, it would be\n>>>>> best not to limit the feature to this particular instance. I've been\n>>>>> toying for a long time with the idea of creating Control instances for\n>>>>> V4L2 controls, which would also allow the usage of a nicer get() and\n>>>>> set() in the ControlList class.\n>>>>\n>>>> But we already establish controls with names from V4L2 when they are\n>>>> available don't we?\n>>>>\n>>>> In src/libcamera/v4l2_controls.cpp, there is a helper\n>>>>\n>>>> std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)\n>>>>\n>>>> which is used to establish a V4L2ControlId after it has been queried\n>>>> from the kernel.\n>>>>\n>>>> The key issue here is that at the point of reporting the Mandatory\n>>>> Controls are not available on the device - well - they don't exist and\n>>>> thus haven't been queried, so there is no name available.\n>>>>\n>>>>\n>>>> Do you envisage a static list of *all* v4l2 controls, over just the 4\n>>>> which are 'required' here? Or that we add all v4l2 controls we use to\n>>>> the static declarations perhaps? (That could be handled just like the\n>>>> other controls then)\n>>>>\n>>>> Right now - with the recent addition of mandatory controls and many\n>>>> sensor drivers not supporting all the required controls, we have several\n>>>> instances of people hitting failures and being told an obscure hex value\n>>>> in the error reporting, which is not very helpful to an end user trying\n>>>> to debug the issue of why their capture which worked a couple of weeks\n>>>> ago now fails.\n>>>>\n>>>> I don't expect that we wish to provide a manual lookup service every\n>>>> time someone reports or queries what those values are - so I'd hope we\n>>>> can have a solution to this quite quickly.\n>>>\n>>> Indeed of course this is more than just the mandatory controls:\n>>>\n>>>\n>>>> [0:51:10.226221013] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0922 not supported\n>>>> [0:51:10.226238875] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0923 not supported\n>>>\n>>> It might be optional, but it would be very nice to know what it is ;-)\n>>\n>> Personal opinion: if one is interesting in knowing why the driver\n>> validation fails, I hope she would not only rely on the debug\n>> printouts but rather turn to the code and cross-check for the controls\n\nThe problem is the hex values are opaque from a human-readable\nperspective. The VBLANK control was only added at the end of last week,\nand there was already a misunderstanding that it was a missing HBLANK\ncontrol preventing the running of mainline master, until it was clear\nthat actually we were now failing on a missing VBLANK.\n\nPrinting hex values just feels like we're saying \"Oh we don't care about\nusers, or others having to debug the situation\".\n\n>> presence in the driver. An ad-hoc solution for this is nice, but it's\n>> a bit a waste of effort, considering how much nicer would our control\n>> framework be if we could get rid of the V4L2Control indexed by id and\n>> treat all controls we deal with (libcamera and v4l2) as ControlId *\n\nWaste of effort or interim solution? It was a 15 minute patch +\ndiscussion (hence RFC of course).\n\n> I think I generally agree.\n> \n>> We import v4l2-controls.h so we have all we need to parse and create\n>> ControlId instances at build time from there. It's not a 2 hours task\n>> unfortunately as it require quite some some of parsing and\n>> substitution.\n> \n> We need type information too, which we can't get from the\n> v4l2-controls.h header. I was thus considering a YAML file listing the\n> V4L2 controls we need.\n\nSo how do we make this happen?\n\nI agree that a full control interface would be nicer, but that is going\nto take time that none of us have right now.\n\n(Though we did have volunteers looking for tasks recently didn't we?)\n\n\nThis is why I would like to still consider an interim solution, because\nwe have actively chosen to cause breakage - and that breakage results in\npeople seeing hex codes as the 'summary' which isn't friendly or helpful\nto end users (or ... me....)\n\n--\nKieran\n\n\n\n>>>>>> Signed-off-by: Kieran Bingham \n>>>>>> ---\n>>>>>> src/libcamera/camera_sensor.cpp | 21 +++++++++++++--------\n>>>>>> 1 file changed, 13 insertions(+), 8 deletions(-)\n>>>>>>\n>>>>>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n>>>>>> index c9e8d49b7935..9a510108f171 100644\n>>>>>> --- a/src/libcamera/camera_sensor.cpp\n>>>>>> +++ b/src/libcamera/camera_sensor.cpp\n>>>>>> @@ -346,18 +346,23 @@ 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 struct {\n>>>>>> +\t\tuint32_t ctrl;\n>>>>>> +\t\tstd::string name;\n>>>>>> +\t} mandatoryControls[] = {\n>>>>>> +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl }\n>>>>>> +\t\tMANDATORY_CONTROL(V4L2_CID_EXPOSURE),\n>>>>>> +\t\tMANDATORY_CONTROL(V4L2_CID_HBLANK),\n>>>>>> +\t\tMANDATORY_CONTROL(V4L2_CID_PIXEL_RATE),\n>>>>>> +\t\tMANDATORY_CONTROL(V4L2_CID_VBLANK),\n>>>>>> +#undef MANDATORY_CONTROL\n>>>>>> \t};\n>>>>>>\n>>>>>> \terr = 0;\n>>>>>> -\tfor (uint32_t ctrl : mandatoryControls) {\n>>>>>> -\t\tif (!controls.count(ctrl)) {\n>>>>>> +\tfor (auto &c : mandatoryControls) {\n>>>>>> +\t\tif (!controls.count(c.ctrl)) {\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 \" << c.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 C59EBBD162\n\tfor ;\n\tMon, 8 Feb 2021 16:47:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4187560D34;\n\tMon, 8 Feb 2021 17:47:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6D88660D21\n\tfor ;\n\tMon, 8 Feb 2021 17:47:49 +0100 (CET)","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 C2C883D7;\n\tMon, 8 Feb 2021 17:47:48 +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=\"ORhc8EbU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612802868;\n\tbh=lRuYAdud62T+mqVEu4y20ttMQARYwqAAIvDPm267X+E=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=ORhc8EbUj7YMTou5SP2CTVm6JH/72TLS/cD4CxEthuyv4OQRQF8IhJYzrnjVX/V3U\n\tSnyB+dtqpv3lviyNFpBLVOCbBcW34I+PnPYb+Ktggos8WiSBAvMKcTGd1ihsxuRCRV\n\t09xXfYp14XWDht0LLSRNrtaNongsahOOdFMunNqU=","To":"Laurent Pinchart ,\n\tJacopo Mondi ","References":"<20210205170317.267605-1-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t\n\t<20210208161529.2vuiopaw3jhg7uhc@uno.localdomain>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<2b6bf91d-b2ab-3205-abeb-a3b5b991626a@ideasonboard.com>","Date":"Mon, 8 Feb 2021 16:47:46 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: camera_sensor: Report\n\tmandatory control names","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15066,"web_url":"https://patchwork.libcamera.org/comment/15066/","msgid":"","date":"2021-02-08T22:55:18","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: camera_sensor: Report\n\tmandatory control names","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Feb 08, 2021 at 04:47:46PM +0000, Kieran Bingham wrote:\n> On 08/02/2021 16:21, Laurent Pinchart wrote:\n> > On Mon, Feb 08, 2021 at 05:15:29PM +0100, Jacopo Mondi wrote:\n> >> On Mon, Feb 08, 2021 at 12:20:57PM +0000, Kieran Bingham wrote:\n> >>> On 06/02/2021 10:12, Kieran Bingham wrote:\n> >>>> On 06/02/2021 07:43, Laurent Pinchart wrote:\n> >>>>> On Fri, Feb 05, 2021 at 05:03:17PM +0000, Kieran Bingham wrote:\n> >>>>>> We can not obtain the control names directly from V4L2 so create\n> >>>>>> a map of the control name, when declaring the list of mandatory\n> >>>>>> controls to enable a human readable print of any missing requirements.\n> >>>>>\n> >>>>> If we want to print V4L2 controls by name to ease debugging, it would be\n> >>>>> best not to limit the feature to this particular instance. I've been\n> >>>>> toying for a long time with the idea of creating Control instances for\n> >>>>> V4L2 controls, which would also allow the usage of a nicer get() and\n> >>>>> set() in the ControlList class.\n> >>>>\n> >>>> But we already establish controls with names from V4L2 when they are\n> >>>> available don't we?\n> >>>>\n> >>>> In src/libcamera/v4l2_controls.cpp, there is a helper\n> >>>>\n> >>>> std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)\n> >>>>\n> >>>> which is used to establish a V4L2ControlId after it has been queried\n> >>>> from the kernel.\n> >>>>\n> >>>> The key issue here is that at the point of reporting the Mandatory\n> >>>> Controls are not available on the device - well - they don't exist and\n> >>>> thus haven't been queried, so there is no name available.\n> >>>>\n> >>>>\n> >>>> Do you envisage a static list of *all* v4l2 controls, over just the 4\n> >>>> which are 'required' here? Or that we add all v4l2 controls we use to\n> >>>> the static declarations perhaps? (That could be handled just like the\n> >>>> other controls then)\n> >>>>\n> >>>> Right now - with the recent addition of mandatory controls and many\n> >>>> sensor drivers not supporting all the required controls, we have several\n> >>>> instances of people hitting failures and being told an obscure hex value\n> >>>> in the error reporting, which is not very helpful to an end user trying\n> >>>> to debug the issue of why their capture which worked a couple of weeks\n> >>>> ago now fails.\n> >>>>\n> >>>> I don't expect that we wish to provide a manual lookup service every\n> >>>> time someone reports or queries what those values are - so I'd hope we\n> >>>> can have a solution to this quite quickly.\n> >>>\n> >>> Indeed of course this is more than just the mandatory controls:\n> >>>\n> >>>> [0:51:10.226221013] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0922 not supported\n> >>>> [0:51:10.226238875] [7894] DEBUG CameraSensor camera_sensor.cpp:291 'imx258 8-001a': Optional V4L2 control 0x009a0923 not supported\n> >>>\n> >>> It might be optional, but it would be very nice to know what it is ;-)\n> >>\n> >> Personal opinion: if one is interesting in knowing why the driver\n> >> validation fails, I hope she would not only rely on the debug\n> >> printouts but rather turn to the code and cross-check for the controls\n> \n> The problem is the hex values are opaque from a human-readable\n> perspective. The VBLANK control was only added at the end of last week,\n> and there was already a misunderstanding that it was a missing HBLANK\n> control preventing the running of mainline master, until it was clear\n> that actually we were now failing on a missing VBLANK.\n> \n> Printing hex values just feels like we're saying \"Oh we don't care about\n> users, or others having to debug the situation\".\n\nI think that Jacopo's point, and mine, was that this message is really\nfor developers, not end users, and developers who would need to add\ncontrols to a camera sensor kernel driver are expected to be able to\nread v4l2-controls.h.\n\nThat being said, a name is of course nicer to read, and not just here.\nMy concern was thus whether printing a name here was important enough to\ncall for a ad-hoc short term fix, or if we should instead take one extra\nstep. In hindsight, I've probably underestimated the lack of\nuser-friendliness of hex values in this case.\n\n> >> presence in the driver. An ad-hoc solution for this is nice, but it's\n> >> a bit a waste of effort, considering how much nicer would our control\n> >> framework be if we could get rid of the V4L2Control indexed by id and\n> >> treat all controls we deal with (libcamera and v4l2) as ControlId *\n> \n> Waste of effort or interim solution? It was a 15 minute patch +\n> discussion (hence RFC of course).\n\nTurns out the discussion is the most time consuming part :-)\n\n> > I think I generally agree.\n> > \n> >> We import v4l2-controls.h so we have all we need to parse and create\n> >> ControlId instances at build time from there. It's not a 2 hours task\n> >> unfortunately as it require quite some some of parsing and\n> >> substitution.\n> > \n> > We need type information too, which we can't get from the\n> > v4l2-controls.h header. I was thus considering a YAML file listing the\n> > V4L2 controls we need.\n> \n> So how do we make this happen?\n\nPlease see the RFC series on the list, let's continue the discussion\nthere.\n\n> I agree that a full control interface would be nicer, but that is going\n> to take time that none of us have right now.\n> \n> (Though we did have volunteers looking for tasks recently didn't we?)\n> \n> \n> This is why I would like to still consider an interim solution, because\n> we have actively chosen to cause breakage - and that breakage results in\n> people seeing hex codes as the 'summary' which isn't friendly or helpful\n> to end users (or ... me....)\n> \n> >>>>>> Signed-off-by: Kieran Bingham \n> >>>>>> ---\n> >>>>>> src/libcamera/camera_sensor.cpp | 21 +++++++++++++--------\n> >>>>>> 1 file changed, 13 insertions(+), 8 deletions(-)\n> >>>>>>\n> >>>>>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> >>>>>> index c9e8d49b7935..9a510108f171 100644\n> >>>>>> --- a/src/libcamera/camera_sensor.cpp\n> >>>>>> +++ b/src/libcamera/camera_sensor.cpp\n> >>>>>> @@ -346,18 +346,23 @@ 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 struct {\n> >>>>>> +\t\tuint32_t ctrl;\n> >>>>>> +\t\tstd::string name;\n> >>>>>> +\t} mandatoryControls[] = {\n> >>>>>> +#define MANDATORY_CONTROL(__ctrl) { __ctrl, #__ctrl }\n> >>>>>> +\t\tMANDATORY_CONTROL(V4L2_CID_EXPOSURE),\n> >>>>>> +\t\tMANDATORY_CONTROL(V4L2_CID_HBLANK),\n> >>>>>> +\t\tMANDATORY_CONTROL(V4L2_CID_PIXEL_RATE),\n> >>>>>> +\t\tMANDATORY_CONTROL(V4L2_CID_VBLANK),\n> >>>>>> +#undef MANDATORY_CONTROL\n> >>>>>> \t};\n> >>>>>>\n> >>>>>> \terr = 0;\n> >>>>>> -\tfor (uint32_t ctrl : mandatoryControls) {\n> >>>>>> -\t\tif (!controls.count(ctrl)) {\n> >>>>>> +\tfor (auto &c : mandatoryControls) {\n> >>>>>> +\t\tif (!controls.count(c.ctrl)) {\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 \" << c.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 A001BBD160\n\tfor ;\n\tMon, 8 Feb 2021 22:55:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6CF39613DA;\n\tMon, 8 Feb 2021 23:55:44 +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 5027C60D37\n\tfor ;\n\tMon, 8 Feb 2021 23:55:43 +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 D13D73D7;\n\tMon, 8 Feb 2021 23:55:42 +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=\"LfNJ5IPu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612824943;\n\tbh=D/E6N3xrvJFgLPdAOBnwP9Fu7KeelJ6RriCeyizXSQ4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LfNJ5IPu0R+0SO4rnLaxVw1lIiYAmE49ungcTbO5gAqHBJrjWDaB9+8Y4Ujvy1kiJ\n\tYsKPvZFAUBIrAvoUWTjTv/hsMfyIrmFmwPNdKOH8r5Dj9uW/65EAHStftuB1Z83abV\n\t5eNjvvUY4a1Ar6ElQ9ve5hAsOJWnW+9LR+V9bEUE=","Date":"Tue, 9 Feb 2021 00:55:18 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210205170317.267605-1-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t\n\t<20210208161529.2vuiopaw3jhg7uhc@uno.localdomain>\n\t\n\t<2b6bf91d-b2ab-3205-abeb-a3b5b991626a@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<2b6bf91d-b2ab-3205-abeb-a3b5b991626a@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: camera_sensor: Report\n\tmandatory control names","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]