[{"id":25140,"web_url":"https://patchwork.libcamera.org/comment/25140/","msgid":"<20220928060037.et3km4qmcrokttuc@uno.localdomain>","date":"2022-09-28T06:00:37","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_device: Log control\n\tid instead of errorIdx","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang\n\nOn Sun, Sep 25, 2022 at 05:53:24PM +0530, Umang Jain via libcamera-devel wrote:\n> v4l2_ext_controls.errorIdx (in the case of single failing control for\n> VIDIOC_*_EXT_CTRLS calls) represents the index of that control.\n> Since it is a single control, we can print the control id rather than\n> its index. This improves logging as the id can be easily co-related\n> with the controls while reading the log.\n\nIf we do that we lose the ability to report errors during the\nvaliation step:\n\nhttps://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-g-ext-ctrls.html?highlight=ext_ctrl#c.V4L.VIDIOC_S_EXT_CTRLS\nIf the error is associated with a particular control, then error_idx\nis set to the index of that control. If the error is not related to a\nspecific control, or the validation step failed (see below), then\nerror_idx is set to count. The value is undefined if the ioctl\nreturned 0 (success).\n\nThere's more context in the documentation page about validation.\n\nAlso, if a single control has failed, error_idx will report the index\nof such control already, so I'm missing what we gain here...\n\nThanks\n j\n\n>\n> Signed-off-by: Umang Jain \n> ---\n> src/libcamera/v4l2_device.cpp | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 83901763..c60f7c91 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -244,7 +244,8 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> \t\t}\n>\n> \t\t/* A specific control failed. */\n> -\t\tLOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n> +\t\tconst unsigned int id = v4l2Ctrls[errorIdx].id;\n> +\t\tLOG(V4L2, Error) << \"Unable to read control \" << utils::hex(id)\n> \t\t\t\t << \": \" << strerror(-ret);\n>\n> \t\tv4l2Ctrls.resize(errorIdx);\n> @@ -354,7 +355,8 @@ int V4L2Device::setControls(ControlList *ctrls)\n> \t\t}\n>\n> \t\t/* A specific control failed. */\n> -\t\tLOG(V4L2, Error) << \"Unable to set control \" << errorIdx\n> +\t\tconst unsigned int id = v4l2Ctrls[errorIdx].id;\n> +\t\tLOG(V4L2, Error) << \"Unable to set control \" << utils::hex(id)\n> \t\t\t\t << \": \" << strerror(-ret);\n>\n> \t\tv4l2Ctrls.resize(errorIdx);\n> --\n> 2.37.3\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 C1D5DC0DA4\n\tfor ;\n\tWed, 28 Sep 2022 06:00:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9E79562280;\n\tWed, 28 Sep 2022 08:00:41 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F18D61F78\n\tfor ;\n\tWed, 28 Sep 2022 08:00:40 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 8E2E024000D;\n\tWed, 28 Sep 2022 06:00:39 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664344841;\n\tbh=upsuNgeDVyYl7gdmaslaKUNBP9HLefgynYncPZMo7xo=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=UdTMsDASEgR1rCO4SHYSFH4ies+BAQyyIr1SNbNHvTd9GBCpCvXtvCwSwsH1BDPYH\n\tr+Qa/rRT0lruxZxp49UmzXWGM3ljTCpsnqToIOGJe4N7TSgoSwrcQmjRDL+otH4Fty\n\tCXBTlEVuJ6n6V4AL6H+TCISfzC4fIZj1uih+z/9bz7fLwatuJBDUcmZR2qWmR5ojiV\n\txN2QqQOjnbnSCom/HDHaIdhM1gzVNn5ii+2kOS1QlY3v0EvWZ2zI8nX2P7e4xK/Gmb\n\to34mTU0zNPEIlk/ZzvSVc2RpOD93flQGrcHAeCmOP1oBt3eG2wyKLGDNoKDCSXorQ6\n\tFTxg/c7eKcTtw==","Date":"Wed, 28 Sep 2022 08:00:37 +0200","To":"Umang Jain ","Message-ID":"<20220928060037.et3km4qmcrokttuc@uno.localdomain>","References":"<20220925122324.260251-1-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220925122324.260251-1-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_device: Log control\n\tid instead of errorIdx","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25143,"web_url":"https://patchwork.libcamera.org/comment/25143/","msgid":"<88d7d3d6-cd43-13eb-8156-5815374910fa@ideasonboard.com>","date":"2022-09-28T06:29:02","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_device: Log control\n\tid instead of errorIdx","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 9/28/22 11:30 AM, Jacopo Mondi wrote:\n> Hi Umang\n>\n> On Sun, Sep 25, 2022 at 05:53:24PM +0530, Umang Jain via libcamera-devel wrote:\n>> v4l2_ext_controls.errorIdx (in the case of single failing control for\n>> VIDIOC_*_EXT_CTRLS calls) represents the index of that control.\n>> Since it is a single control, we can print the control id rather than\n>> its index. This improves logging as the id can be easily co-related\n>> with the controls while reading the log.\n> If we do that we lose the ability to report errors during the\n> valiation step:\n\nValidation errors are reported separately, just above this patch's hunk(s).\n\nSee here: \nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n239\n>\n> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-g-ext-ctrls.html?highlight=ext_ctrl#c.V4L.VIDIOC_S_EXT_CTRLS\n> If the error is associated with a particular control, then error_idx\n> is set to the index of that control. If the error is not related to a\n> specific control, or the validation step failed (see below), then\n> error_idx is set to count. The value is undefined if the ioctl\n> returned 0 (success).\n>\n> There's more context in the documentation page about validation.\n>\n> Also, if a single control has failed, error_idx will report the index\n> of such control already, so I'm missing what we gain here...\n\nindex for e.g.  '3' is less readable than control id, say : (0x009819e2)\n\nHence we already print controls (in the debug mode, see below) with \ntheir control-ids, one can easily figure out which (specific) control is \nfailing just by looking at the log. Hence, we \"gain\" read-ability here.\n...\n[1:40:41.345462669] [16014] DEBUG V4L2 v4l2_device.cpp:625 \n/dev/video13[16:cap]: Control: Blue Balance (0x0098090f)\n[1:40:41.345590216] [16014] DEBUG V4L2 v4l2_device.cpp:625 \n/dev/video13[16:cap]: Control: Colour Correction Matrix (0x009819e1)\n[1:40:41.345707893] [16014] DEBUG V4L2 v4l2_device.cpp:625 \n/dev/video13[16:cap]: Control: Lens Shading (0x009819e2)\n[1:40:41.345818848] [16014] DEBUG V4L2 v4l2_device.cpp:625 \n/dev/video13[16:cap]: Control: Black Level (0x009819e3)\n....\n\n\n>\n> Thanks\n> j\n>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/libcamera/v4l2_device.cpp | 6 ++++--\n>> 1 file changed, 4 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>> index 83901763..c60f7c91 100644\n>> --- a/src/libcamera/v4l2_device.cpp\n>> +++ b/src/libcamera/v4l2_device.cpp\n>> @@ -244,7 +244,8 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n>> \t\t}\n>>\n>> \t\t/* A specific control failed. */\n>> -\t\tLOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n>> +\t\tconst unsigned int id = v4l2Ctrls[errorIdx].id;\n>> +\t\tLOG(V4L2, Error) << \"Unable to read control \" << utils::hex(id)\n>> \t\t\t\t << \": \" << strerror(-ret);\n>>\n>> \t\tv4l2Ctrls.resize(errorIdx);\n>> @@ -354,7 +355,8 @@ int V4L2Device::setControls(ControlList *ctrls)\n>> \t\t}\n>>\n>> \t\t/* A specific control failed. */\n>> -\t\tLOG(V4L2, Error) << \"Unable to set control \" << errorIdx\n>> +\t\tconst unsigned int id = v4l2Ctrls[errorIdx].id;\n>> +\t\tLOG(V4L2, Error) << \"Unable to set control \" << utils::hex(id)\n>> \t\t\t\t << \": \" << strerror(-ret);\n>>\n>> \t\tv4l2Ctrls.resize(errorIdx);\n>> --\n>> 2.37.3\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 162ADC0DA4\n\tfor ;\n\tWed, 28 Sep 2022 06:29:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E2366228A;\n\tWed, 28 Sep 2022 08:29:11 +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 D197561F78\n\tfor ;\n\tWed, 28 Sep 2022 08:29:09 +0200 (CEST)","from [192.168.1.102] (unknown [103.238.109.17])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7285C47C;\n\tWed, 28 Sep 2022 08:29:08 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664346551;\n\tbh=UsrfQJ7OWnZG779fv6TWQzRjHAcHO6Vw6upEOXqzeXQ=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=HL5BimMcpKG55FtyJeQsgv5MvMwolwTtfsnJlk9MAcKDiIrMwQzIE3mj2cbKLDG/b\n\ty9CTcVCUTCcd12joASlsOTAAk5Lpb4n/XtoNpQnBaZVV9to1xoGqWFJIPwqZJy8sB0\n\tEDl5rGClaukZVRXAW0N4G6ZUnjENYv6Yosm7IV+sKWqMSIEx1oVWXaEjDwOEcPtJQT\n\tcfn6UlIfw8NyR+YztL7rYHk5j7DT2mQegphcpy+rM3peBpRwHSJ1IFvxRoqHoTSPRH\n\tHKKphmo6KsCMXSoI2tzR0TH1ncIYCjG6aikIqqebsLxxct2pNgbx/BScO0ArKBGEDP\n\t5UGmVk9wPUIvg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664346549;\n\tbh=UsrfQJ7OWnZG779fv6TWQzRjHAcHO6Vw6upEOXqzeXQ=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=aqfEfmZ19Dlh34sP2iRmhnhmtXtU6c3r9/TJxulXbAdK8jRuCscyhct4c4M8DclJd\n\tmre7+bJEaxJOyLyuvI+kwyTBj244qaWkl8PzfjgZF2b6n7xAJI3sos7OMDmhEjEx1t\n\tQd4YBryf6RKFlJvZQdAD+3u4xAydqpqGQPY1/ENg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"aqfEfmZ1\"; dkim-atps=neutral","Message-ID":"<88d7d3d6-cd43-13eb-8156-5815374910fa@ideasonboard.com>","Date":"Wed, 28 Sep 2022 11:59:02 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.2.1","To":"Jacopo Mondi ","References":"<20220925122324.260251-1-umang.jain@ideasonboard.com>\n\t<20220928060037.et3km4qmcrokttuc@uno.localdomain>","Content-Language":"en-US","In-Reply-To":"<20220928060037.et3km4qmcrokttuc@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_device: Log control\n\tid instead of errorIdx","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25162,"web_url":"https://patchwork.libcamera.org/comment/25162/","msgid":"","date":"2022-09-28T13:04:32","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_device: Log control\n\tid instead of errorIdx","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Sep 28, 2022 at 11:59:02AM +0530, Umang Jain via libcamera-devel wrote:\n> On 9/28/22 11:30 AM, Jacopo Mondi wrote:\n> > On Sun, Sep 25, 2022 at 05:53:24PM +0530, Umang Jain via libcamera-devel wrote:\n> >> v4l2_ext_controls.errorIdx (in the case of single failing control for\n> >> VIDIOC_*_EXT_CTRLS calls) represents the index of that control.\n> >> Since it is a single control, we can print the control id rather than\n> >> its index. This improves logging as the id can be easily co-related\n> >> with the controls while reading the log.\n> > \n> > If we do that we lose the ability to report errors during the\n> > valiation step:\n> \n> Validation errors are reported separately, just above this patch's hunk(s).\n> \n> See here: \n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n239\n> \n> > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-g-ext-ctrls.html?highlight=ext_ctrl#c.V4L.VIDIOC_S_EXT_CTRLS\n> > If the error is associated with a particular control, then error_idx\n> > is set to the index of that control. If the error is not related to a\n> > specific control, or the validation step failed (see below), then\n> > error_idx is set to count. The value is undefined if the ioctl\n> > returned 0 (success).\n> >\n> > There's more context in the documentation page about validation.\n> >\n> > Also, if a single control has failed, error_idx will report the index\n> > of such control already, so I'm missing what we gain here...\n> \n> index for e.g.  '3' is less readable than control id, say : (0x009819e2)\n> \n> Hence we already print controls (in the debug mode, see below) with \n> their control-ids, one can easily figure out which (specific) control is \n> failing just by looking at the log. Hence, we \"gain\" read-ability here.\n> ...\n> [1:40:41.345462669] [16014] DEBUG V4L2 v4l2_device.cpp:625 \n> /dev/video13[16:cap]: Control: Blue Balance (0x0098090f)\n> [1:40:41.345590216] [16014] DEBUG V4L2 v4l2_device.cpp:625 \n> /dev/video13[16:cap]: Control: Colour Correction Matrix (0x009819e1)\n> [1:40:41.345707893] [16014] DEBUG V4L2 v4l2_device.cpp:625 \n> /dev/video13[16:cap]: Control: Lens Shading (0x009819e2)\n> [1:40:41.345818848] [16014] DEBUG V4L2 v4l2_device.cpp:625 \n> /dev/video13[16:cap]: Control: Black Level (0x009819e3)\n> ....\n> \n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/libcamera/v4l2_device.cpp | 6 ++++--\n> >> 1 file changed, 4 insertions(+), 2 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >> index 83901763..c60f7c91 100644\n> >> --- a/src/libcamera/v4l2_device.cpp\n> >> +++ b/src/libcamera/v4l2_device.cpp\n> >> @@ -244,7 +244,8 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> >> \t\t}\n> >>\n> >> \t\t/* A specific control failed. */\n> >> -\t\tLOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n> >> +\t\tconst unsigned int id = v4l2Ctrls[errorIdx].id;\n> >> +\t\tLOG(V4L2, Error) << \"Unable to read control \" << utils::hex(id)\n> >> \t\t\t\t << \": \" << strerror(-ret);\n\nThis wouldn't almpw us to differentiate between entries for controls\nwith identical IDs, but that's such a corner case that I don't mind.\n\nReviewed-by: Laurent Pinchart \n\n> >>\n> >> \t\tv4l2Ctrls.resize(errorIdx);\n> >> @@ -354,7 +355,8 @@ int V4L2Device::setControls(ControlList *ctrls)\n> >> \t\t}\n> >>\n> >> \t\t/* A specific control failed. */\n> >> -\t\tLOG(V4L2, Error) << \"Unable to set control \" << errorIdx\n> >> +\t\tconst unsigned int id = v4l2Ctrls[errorIdx].id;\n> >> +\t\tLOG(V4L2, Error) << \"Unable to set control \" << utils::hex(id)\n> >> \t\t\t\t << \": \" << strerror(-ret);\n> >>\n> >> \t\tv4l2Ctrls.resize(errorIdx);","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 92FE8BD16B\n\tfor ;\n\tWed, 28 Sep 2022 13:04:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F33D622AE;\n\tWed, 28 Sep 2022 15:04:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E411B622A6\n\tfor ;\n\tWed, 28 Sep 2022 15:04:33 +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 31E7F47C;\n\tWed, 28 Sep 2022 15:04:33 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664370275;\n\tbh=WRLbz1JvcI6Kyp2RhnJbPsuDV/cjCl3DBeT+wv6JUcw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=L7QGxF98lLHAu346H2n5KEO8OucultwwkXLMT9OakszNWrwkrGMOeKdssno4HW2H9\n\tUY9iJbK/2HT3dOs3FH3tUsKHF38AVjTPLA5wigBsMFJWdEdRZ3Iiuq4F9gcx+NTeSu\n\tQehnp/2hIIpE+797Jsascqo2tBk1YZIznDWVLSF2nT5SEP99ryq/ZGMzFH0KWxr/ho\n\tNpf3crP2nqe0UvqfdYqiHCP54ACOWnzan3FeHmqIZDMcZ1vjumYKEMnm/lYdraE8mb\n\ttDY51qFF6muYI7En4MoGqeOcYlsXBxbI6MVsvlTPnHKrNWqUuCP+/PHwIsdVP5SUJ9\n\t8jMlT1AUaRzqA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664370273;\n\tbh=WRLbz1JvcI6Kyp2RhnJbPsuDV/cjCl3DBeT+wv6JUcw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UdB8x1pGTWDk6nJ4TmxdrXDcadlJSD2V9sDu643Oahp6It05ekYlw5QgMvOf/WwH1\n\tl9y8+ybi+GSGp/gjodKnAgZ0MxGmVfs4++H8SwBjWC18Hs3utYPENZYiAxt6Y4vlth\n\tUaFVaYi3/955otrG5EZDFLyNljDo86z27Gpgej1c="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UdB8x1pG\"; dkim-atps=neutral","Date":"Wed, 28 Sep 2022 16:04:32 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220925122324.260251-1-umang.jain@ideasonboard.com>\n\t<20220928060037.et3km4qmcrokttuc@uno.localdomain>\n\t<88d7d3d6-cd43-13eb-8156-5815374910fa@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<88d7d3d6-cd43-13eb-8156-5815374910fa@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_device: Log control\n\tid instead of errorIdx","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25163,"web_url":"https://patchwork.libcamera.org/comment/25163/","msgid":"<20220928131951.i4rtrmwlvahs2s6b@uno.localdomain>","date":"2022-09-28T13:19:51","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_device: Log control\n\tid instead of errorIdx","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang\n\nOn Wed, Sep 28, 2022 at 04:04:32PM +0300, Laurent Pinchart wrote:\n> Hello,\n>\n> On Wed, Sep 28, 2022 at 11:59:02AM +0530, Umang Jain via libcamera-devel wrote:\n> > On 9/28/22 11:30 AM, Jacopo Mondi wrote:\n> > > On Sun, Sep 25, 2022 at 05:53:24PM +0530, Umang Jain via libcamera-devel wrote:\n> > >> v4l2_ext_controls.errorIdx (in the case of single failing control for\n> > >> VIDIOC_*_EXT_CTRLS calls) represents the index of that control.\n> > >> Since it is a single control, we can print the control id rather than\n> > >> its index. This improves logging as the id can be easily co-related\n> > >> with the controls while reading the log.\n> > >\n> > > If we do that we lose the ability to report errors during the\n> > > valiation step:\n> >\n> > Validation errors are reported separately, just above this patch's hunk(s).\n> >\n> > See here:\n> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n239\n> >\n\nOh, missed the fact we already handle errorIdx >= numControls\n\n\n> > > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-g-ext-ctrls.html?highlight=ext_ctrl#c.V4L.VIDIOC_S_EXT_CTRLS\n> > > If the error is associated with a particular control, then error_idx\n> > > is set to the index of that control. If the error is not related to a\n> > > specific control, or the validation step failed (see below), then\n> > > error_idx is set to count. The value is undefined if the ioctl\n> > > returned 0 (success).\n> > >\n> > > There's more context in the documentation page about validation.\n> > >\n> > > Also, if a single control has failed, error_idx will report the index\n> > > of such control already, so I'm missing what we gain here...\n> >\n> > index for e.g.  '3' is less readable than control id, say : (0x009819e2)\n> >\n> > Hence we already print controls (in the debug mode, see below) with\n> > their control-ids, one can easily figure out which (specific) control is\n> > failing just by looking at the log. Hence, we \"gain\" read-ability here.\n> > ...\n> > [1:40:41.345462669] [16014] DEBUG V4L2 v4l2_device.cpp:625\n> > /dev/video13[16:cap]: Control: Blue Balance (0x0098090f)\n> > [1:40:41.345590216] [16014] DEBUG V4L2 v4l2_device.cpp:625\n> > /dev/video13[16:cap]: Control: Colour Correction Matrix (0x009819e1)\n> > [1:40:41.345707893] [16014] DEBUG V4L2 v4l2_device.cpp:625\n> > /dev/video13[16:cap]: Control: Lens Shading (0x009819e2)\n> > [1:40:41.345818848] [16014] DEBUG V4L2 v4l2_device.cpp:625\n> > /dev/video13[16:cap]: Control: Black Level (0x009819e3)\n> > ....\n> >\n> > >> Signed-off-by: Umang Jain \n> > >> ---\n> > >> src/libcamera/v4l2_device.cpp | 6 ++++--\n> > >> 1 file changed, 4 insertions(+), 2 deletions(-)\n> > >>\n> > >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > >> index 83901763..c60f7c91 100644\n> > >> --- a/src/libcamera/v4l2_device.cpp\n> > >> +++ b/src/libcamera/v4l2_device.cpp\n> > >> @@ -244,7 +244,8 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > >> \t\t}\n> > >>\n> > >> \t\t/* A specific control failed. */\n> > >> -\t\tLOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n> > >> +\t\tconst unsigned int id = v4l2Ctrls[errorIdx].id;\n\nAfaict 'const' has no purpose.\n\nApart from that\nReviewed-by: Jacopo Mondi \n\n\n> > >> +\t\tLOG(V4L2, Error) << \"Unable to read control \" << utils::hex(id)\n> > >> \t\t\t\t << \": \" << strerror(-ret);\n>\n> This wouldn't almpw us to differentiate between entries for controls\n> with identical IDs, but that's such a corner case that I don't mind.\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > >>\n> > >> \t\tv4l2Ctrls.resize(errorIdx);\n> > >> @@ -354,7 +355,8 @@ int V4L2Device::setControls(ControlList *ctrls)\n> > >> \t\t}\n> > >>\n> > >> \t\t/* A specific control failed. */\n> > >> -\t\tLOG(V4L2, Error) << \"Unable to set control \" << errorIdx\n> > >> +\t\tconst unsigned int id = v4l2Ctrls[errorIdx].id;\n> > >> +\t\tLOG(V4L2, Error) << \"Unable to set control \" << utils::hex(id)\n> > >> \t\t\t\t << \": \" << strerror(-ret);\n> > >>\n> > >> \t\tv4l2Ctrls.resize(errorIdx);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2BBA7C0DA4\n\tfor ;\n\tWed, 28 Sep 2022 13:19:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 874C7622AF;\n\tWed, 28 Sep 2022 15:19:55 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0CE11622A6\n\tfor ;\n\tWed, 28 Sep 2022 15:19:54 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id EF4FB1C0009;\n\tWed, 28 Sep 2022 13:19:52 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664371195;\n\tbh=2UgkPn9QgIXB4dxy27kTbIRKMhquSqEY8jofxVAxGE8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=zBWVFtBs66jpe+9HhOPBk7hSmIl7+2Y4l9PFX7EVhLABotl0WUG+8as1IWAiAXG6a\n\ttuYAmt48Har1e92cDc0poqcjOM52TaMe49hPdBJHjW3S7dCp9+ZG/1pR66tkBSxUQ3\n\tRMaQHZLJPSY4kDuvyCP5T9dTXXhnZRGi64wfmg6scp/EG3q9rh8VJm92py0kRzS4MT\n\tkPLm2qityqJBGnyN7eNYYeFqY1Sak1KGzzSOcEu6AVznQ5BntCY7ecPE1hzB8XuFCz\n\tfNGyUeCpNPBXo+D+J8pfSxnDuT/1P03GAIOpqYwtcNf13Wu8klluPlvVTXtZn+dCSz\n\tv1Wfg23WOx0Ng==","Date":"Wed, 28 Sep 2022 15:19:51 +0200","To":"Laurent Pinchart ","Message-ID":"<20220928131951.i4rtrmwlvahs2s6b@uno.localdomain>","References":"<20220925122324.260251-1-umang.jain@ideasonboard.com>\n\t<20220928060037.et3km4qmcrokttuc@uno.localdomain>\n\t<88d7d3d6-cd43-13eb-8156-5815374910fa@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_device: Log control\n\tid instead of errorIdx","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]