[{"id":28111,"web_url":"https://patchwork.libcamera.org/comment/28111/","msgid":"<170051925245.133283.13641407666207121882@ping.linuxembedded.co.uk>","date":"2023-11-20T22:27:32","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: fix HBLANK\n\tRO check","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Alain,\n\nQuoting Alain Volmat via libcamera-devel (2023-11-20 18:45:29)\n> Perform the HBLANK readonly check by looking at the v4l2_query_ext_ctrl\n> struct for the V4L2_CID_HBLANK instead of checking for min/max values.\n> \n\nAha, you beat me to it. (ref:\nhttps://patchwork.libcamera.org/patch/19189/)\n\nI would say we need to fix both here and src/ipa/rpi/common/ipa_base.cpp\nwith the same change though. Could you also update that and compile test\nplease?\n\n\n\n> Signed-off-by: Alain Volmat \n> ---\n> src/libcamera/camera_sensor.cpp | 17 ++++-------------\n> 1 file changed, 4 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 0ef78d9c..3281c1f9 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -188,21 +188,12 @@ int CameraSensor::init()\n> * Set HBLANK to the minimum to start with a well-defined line length,\n> * allowing IPA modules that do not modify HBLANK to use the sensor\n> * minimum line length in their calculations.\n> - *\n> - * At present, there is no way of knowing if a control is read-only.\n> - * As a workaround, assume that if the minimum and maximum values of\n> - * the V4L2_CID_HBLANK control are the same, it implies the control\n> - * is read-only.\n> - *\n> - * \\todo The control API ought to have a flag to specify if a control\n> - * is read-only which could be used below.\n> */\n> if (ctrls.infoMap()->find(V4L2_CID_HBLANK) != ctrls.infoMap()->end()) {\n> - const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> - const int32_t hblankMin = hblank.min().get();\n> - const int32_t hblankMax = hblank.max().get();\n> -\n> - if (hblankMin != hblankMax) {\n> + const struct v4l2_query_ext_ctrl *hblankInfo = subdev_->controlInfo(V4L2_CID_HBLANK);\n> + if (!(hblankInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) {\n\nI don't know if it actually helps or makes sense yet, but if I got here\nfirst I was going to see if it would make sense to put a helper into the\nv4l2_device class to make this more succinct (but still V4L2 specific\nrather than Control specific).\n\nEither with or without a helper - both locations covered would earn a\ntag from me.\n\n--\nKieran\n\n\n> + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> + const int32_t hblankMin = hblank.min().get();\n> ControlList ctrl(subdev_->controls());\n> \n> ctrl.set(V4L2_CID_HBLANK, hblankMin);\n> -- \n> 2.25.1\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 3AC17C3200\n\tfor ;\n\tMon, 20 Nov 2023 22:27:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 77D6C61DAF;\n\tMon, 20 Nov 2023 23:27:37 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B620E61DAF\n\tfor ;\n\tMon, 20 Nov 2023 23:27:35 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7C67D480;\n\tMon, 20 Nov 2023 23:27:05 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1700519257;\n\tbh=6P0nNtWo59jxe45diBT+hWkKiThn45m+LhwmMqoy0dY=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=TqvoXQ2JcU+2TrHig0IFVBvIydNUAAQUMZfrbD4p9Tpzo8G9AxpHj4DdZeq2RMWO/\n\tt1BVqKOPyhjDluo4sLYJNMNBT4rZ0BFbF1G8mgh2ib3p7ovZ8H8BHCmUlSdyNKpF5w\n\trcm4YDIxXACMAgqWlD0w4ZKCp0ZCKSqYyYyTYC5HQDqoIS8g5YOR5cB8UwE5V+Z1bq\n\tSr6W2KORG/nF1ygLurQLybujdmuZoqi2hgP+zAQzPK16ZjurXrmMumAXRGxC7ZJo1h\n\taD4lKAV2ZtPEspthen/fENd5WC5wwJb/OkSWyefHSuPrIRe+iBODJHJV9yhWeZOBdN\n\tzm5TsGcikMWsg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1700519225;\n\tbh=6P0nNtWo59jxe45diBT+hWkKiThn45m+LhwmMqoy0dY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=IAK/m2uvxyLPlUp5IXDLdRv3ZQ/F4pmP8mFQ5rA9tShaBsau4xWaoDf5XKdN6HNq/\n\taEH7yLGaT3PUc9CzdirslYaE9wTU+49qAIZyEhb404DhA8Ld0oQLbztUSyxkemxWsg\n\t1Yzddcxr9Q6AUJZJ16vhyQOfur0B15e31ynhNSsA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"IAK/m2uv\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20231120184529.730565-1-alain.volmat@foss.st.com>","References":"<20231120184529.730565-1-alain.volmat@foss.st.com>","To":"Alain Volmat ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 20 Nov 2023 22:27:32 +0000","Message-ID":"<170051925245.133283.13641407666207121882@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: fix HBLANK\n\tRO check","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Jacopo Mondi ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28117,"web_url":"https://patchwork.libcamera.org/comment/28117/","msgid":"","date":"2023-11-21T09:47:28","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: fix HBLANK\n\tRO check","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\n\nOn Mon, Nov 20, 2023 at 10:27:32PM +0000, Kieran Bingham via libcamera-devel wrote:\n> Hi Alain,\n>\n> Quoting Alain Volmat via libcamera-devel (2023-11-20 18:45:29)\n> > Perform the HBLANK readonly check by looking at the v4l2_query_ext_ctrl\n> > struct for the V4L2_CID_HBLANK instead of checking for min/max values.\n> >\n>\n> Aha, you beat me to it. (ref:\n> https://patchwork.libcamera.org/patch/19189/)\n>\n\nThanks for digging it out\n\n> I would say we need to fix both here and src/ipa/rpi/common/ipa_base.cpp\n> with the same change though. Could you also update that and compile test\n> please?\n>\n\nWhy not resume the work done by Naush then and re-propose his series ?\nAlain could you maybe consider that ?\n\n>\n>\n> > Signed-off-by: Alain Volmat \n> > ---\n> > src/libcamera/camera_sensor.cpp | 17 ++++-------------\n> > 1 file changed, 4 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 0ef78d9c..3281c1f9 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -188,21 +188,12 @@ int CameraSensor::init()\n> > * Set HBLANK to the minimum to start with a well-defined line length,\n> > * allowing IPA modules that do not modify HBLANK to use the sensor\n> > * minimum line length in their calculations.\n> > - *\n> > - * At present, there is no way of knowing if a control is read-only.\n> > - * As a workaround, assume that if the minimum and maximum values of\n> > - * the V4L2_CID_HBLANK control are the same, it implies the control\n> > - * is read-only.\n> > - *\n> > - * \\todo The control API ought to have a flag to specify if a control\n> > - * is read-only which could be used below.\n> > */\n> > if (ctrls.infoMap()->find(V4L2_CID_HBLANK) != ctrls.infoMap()->end()) {\n> > - const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > - const int32_t hblankMin = hblank.min().get();\n> > - const int32_t hblankMax = hblank.max().get();\n> > -\n> > - if (hblankMin != hblankMax) {\n> > + const struct v4l2_query_ext_ctrl *hblankInfo = subdev_->controlInfo(V4L2_CID_HBLANK);\n> > + if (!(hblankInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) {\n>\n> I don't know if it actually helps or makes sense yet, but if I got here\n> first I was going to see if it would make sense to put a helper into the\n> v4l2_device class to make this more succinct (but still V4L2 specific\n> rather than Control specific).\n>\n> Either with or without a helper - both locations covered would earn a\n> tag from me.\n>\n> --\n> Kieran\n>\n>\n> > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > + const int32_t hblankMin = hblank.min().get();\n> > ControlList ctrl(subdev_->controls());\n> >\n> > ctrl.set(V4L2_CID_HBLANK, hblankMin);\n> > --\n> > 2.25.1\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 8A455BDE6B\n\tfor ;\n\tTue, 21 Nov 2023 09:47:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E2576629BA;\n\tTue, 21 Nov 2023 10:47:33 +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 30A03629B6\n\tfor ;\n\tTue, 21 Nov 2023 10:47:32 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 57EDB2E4;\n\tTue, 21 Nov 2023 10:47:01 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1700560053;\n\tbh=ys6BKGUXf2rKjgfuJKdB+pXMFjAJDNthLDLkm4MAQw0=;\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=O74hKdJAwPg0r9q6YRBOqKFkkNNFNp4fuU05M16tXNtNyPRdIIhDfg3j2de8PMKVM\n\tb/sPaX7pKod8+vUGb9Dd1Gw89rDKqoM/BGDM/0dg7fnt30JzLbIbiWEW/GlvDjeCH4\n\td4JV4vY0u9RT1BLplaT4I/d1B9E942eh6CHpU3p0micwaIVF8yJOFDKKy8ZWGO70Mm\n\twy3PCa2WdaeJhrDRgjM2wcxNUFQa4XSqdQtg+kYeI8A3gtlspZIfQ/VtoMvBhuWCPI\n\tthAcWZSynVtu/kk1vgamkWVU/pkvF0tB8Fh9sxR995krcQl197YurD8Upg/tQOPTCh\n\tLUyaJsynpHBtw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1700560021;\n\tbh=ys6BKGUXf2rKjgfuJKdB+pXMFjAJDNthLDLkm4MAQw0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=G+e8tLazs6mou9LxyLBdRytvnYlxYDnwzlXLudUkoAWwXho8qlBRtaXHstQFUFEK5\n\tHwp0SevUNMm0LI///LirWtcWFu8ut/azgKlA7P7JZFe5tLg0QInKRIYyeJI+eHZ5YH\n\tB732VHZrMuy8H6YwkXAn/GlHBFROzsUWT265wSIQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"G+e8tLaz\"; dkim-atps=neutral","Date":"Tue, 21 Nov 2023 10:47:28 +0100","To":"Kieran Bingham ","Message-ID":"","References":"<20231120184529.730565-1-alain.volmat@foss.st.com>\n\t<170051925245.133283.13641407666207121882@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<170051925245.133283.13641407666207121882@ping.linuxembedded.co.uk>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: fix HBLANK\n\tRO check","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":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28120,"web_url":"https://patchwork.libcamera.org/comment/28120/","msgid":"<170057232541.451646.11994781400031083073@ping.linuxembedded.co.uk>","date":"2023-11-21T13:12:05","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: fix HBLANK\n\tRO check","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2023-11-21 09:47:28)\n> Hi Kieran\n> \n> On Mon, Nov 20, 2023 at 10:27:32PM +0000, Kieran Bingham via libcamera-devel wrote:\n> > Hi Alain,\n> >\n> > Quoting Alain Volmat via libcamera-devel (2023-11-20 18:45:29)\n> > > Perform the HBLANK readonly check by looking at the v4l2_query_ext_ctrl\n> > > struct for the V4L2_CID_HBLANK instead of checking for min/max values.\n> > >\n> >\n> > Aha, you beat me to it. (ref:\n> > https://patchwork.libcamera.org/patch/19189/)\n> >\n> \n> Thanks for digging it out\n> \n> > I would say we need to fix both here and src/ipa/rpi/common/ipa_base.cpp\n> > with the same change though. Could you also update that and compile test\n> > please?\n> >\n> \n> Why not resume the work done by Naush then and re-propose his series ?\n> Alain could you maybe consider that ?\n\nI did. It sounded rejected to me.\n\nhttps://patchwork.libcamera.org/cover/19187/\n\n\"\"\"\n> Only modified one ControlInfo constructor is modified which is used by the\n> V4L2Device class to allow this flag to be set, as setting it for a non-v4l2\n> control probably does not make sense at this point.\n\nThis is the part that bothers me a bit. If the feature is only used\ninternally, it shouldn't be exposed in the public API.\n\nOne possible workaround would be to add flag controls as being settable\nin a request and as being reported in metadata. This is a feature that\nis useful for applications, and it could then be used internally do\nindicate read-only internal controls.\n\"\"\"\n\n--\nKieran\n\n\n> \n> >\n> >\n> > > Signed-off-by: Alain Volmat \n> > > ---\n> > > src/libcamera/camera_sensor.cpp | 17 ++++-------------\n> > > 1 file changed, 4 insertions(+), 13 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index 0ef78d9c..3281c1f9 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -188,21 +188,12 @@ int CameraSensor::init()\n> > > * Set HBLANK to the minimum to start with a well-defined line length,\n> > > * allowing IPA modules that do not modify HBLANK to use the sensor\n> > > * minimum line length in their calculations.\n> > > - *\n> > > - * At present, there is no way of knowing if a control is read-only.\n> > > - * As a workaround, assume that if the minimum and maximum values of\n> > > - * the V4L2_CID_HBLANK control are the same, it implies the control\n> > > - * is read-only.\n> > > - *\n> > > - * \\todo The control API ought to have a flag to specify if a control\n> > > - * is read-only which could be used below.\n> > > */\n> > > if (ctrls.infoMap()->find(V4L2_CID_HBLANK) != ctrls.infoMap()->end()) {\n> > > - const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > > - const int32_t hblankMin = hblank.min().get();\n> > > - const int32_t hblankMax = hblank.max().get();\n> > > -\n> > > - if (hblankMin != hblankMax) {\n> > > + const struct v4l2_query_ext_ctrl *hblankInfo = subdev_->controlInfo(V4L2_CID_HBLANK);\n> > > + if (!(hblankInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) {\n> >\n> > I don't know if it actually helps or makes sense yet, but if I got here\n> > first I was going to see if it would make sense to put a helper into the\n> > v4l2_device class to make this more succinct (but still V4L2 specific\n> > rather than Control specific).\n> >\n> > Either with or without a helper - both locations covered would earn a\n> > tag from me.\n> >\n> > --\n> > Kieran\n> >\n> >\n> > > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > > + const int32_t hblankMin = hblank.min().get();\n> > > ControlList ctrl(subdev_->controls());\n> > >\n> > > ctrl.set(V4L2_CID_HBLANK, hblankMin);\n> > > --\n> > > 2.25.1\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 5706ABDE6B\n\tfor ;\n\tTue, 21 Nov 2023 13:12:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDBE761DAE;\n\tTue, 21 Nov 2023 14:12:12 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3FEEB61DAE\n\tfor ;\n\tTue, 21 Nov 2023 14:12:08 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8BE372E4;\n\tTue, 21 Nov 2023 14:11:37 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1700572332;\n\tbh=m7c0ux/B9Fodyb+au/8V2asUbXcq/BEgpZ65N6nwTfY=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=lLa10KYcAKJ7pNAlGmckNwqWD8ZOewMYpzNSKY6i2YmtYk8y3fioXc3J5jbLjQRnv\n\t3DN+w+7lUUQ6qS29nWnElHp0ul8i9UPdV2oqhO3/pBTfMFFF45lQ6jNIDAmIZ9nVT0\n\tpo2081VND+hBp/3YM7+EGh4hGKci7n4N9/iD8k9VIF6LMQdrslNBtRrCMCrGC1GpEf\n\tYB50gZwZeCxdgvzzb41O2V8IXRZgLAbcp0qRKF1vUUu9NuEp2Pc6ToII6Uun34hGwW\n\tOKsVxKPjIZ/+VUFgsOinkGfgDm075/OUoCIOnDhAYOfoJNKHWkUjQZkLDmtDlRvUTj\n\tW0cEhL2OL3kHw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1700572297;\n\tbh=m7c0ux/B9Fodyb+au/8V2asUbXcq/BEgpZ65N6nwTfY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=aMXil+VK7hVeKV9XrDmCQUuWNM4zk0+ATpeINhL2Za+G7Mb+d4I7JS/+e++dSFyHB\n\tyNFWonSMSyw7KfQ08MSqqqNaasRbplHz8pfQr4TnR8Z/93LeXHbrXy2bMiHS12GoIg\n\tEJBjvtMOrjad0Vb3DUTBPWm/UAqnwH66OWlK4K1Y="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"aMXil+VK\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20231120184529.730565-1-alain.volmat@foss.st.com>\n\t<170051925245.133283.13641407666207121882@ping.linuxembedded.co.uk>\n\t","To":"Jacopo Mondi ","Date":"Tue, 21 Nov 2023 13:12:05 +0000","Message-ID":"<170057232541.451646.11994781400031083073@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: fix HBLANK\n\tRO check","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28862,"web_url":"https://patchwork.libcamera.org/comment/28862/","msgid":"<20240305111432.GA24759@pendragon.ideasonboard.com>","date":"2024-03-05T11:14:32","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: fix HBLANK\n\tRO check","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nResurecting this thread, as I've recently sent a similar patch\n(https://patchwork.libcamera.org/patch/19608/).\n\nOn Tue, Nov 21, 2023 at 01:12:05PM +0000, 📷-dev wrote:\n> Quoting Jacopo Mondi (2023-11-21 09:47:28)\n> > On Mon, Nov 20, 2023 at 10:27:32PM +0000, Kieran Bingham via libcamera-devel wrote:\n> > > Quoting Alain Volmat via libcamera-devel (2023-11-20 18:45:29)\n> > > > Perform the HBLANK readonly check by looking at the v4l2_query_ext_ctrl\n> > > > struct for the V4L2_CID_HBLANK instead of checking for min/max values.\n> > >\n> > > Aha, you beat me to it. (ref:\n> > > https://patchwork.libcamera.org/patch/19189/)\n> > \n> > Thanks for digging it out\n> > \n> > > I would say we need to fix both here and src/ipa/rpi/common/ipa_base.cpp\n> > > with the same change though. Could you also update that and compile test\n> > > please?\n> > \n> > Why not resume the work done by Naush then and re-propose his series ?\n> > Alain could you maybe consider that ?\n> \n> I did. It sounded rejected to me.\n> \n> https://patchwork.libcamera.org/cover/19187/\n> \n> \"\"\"\n> > Only modified one ControlInfo constructor is modified which is used by the\n> > V4L2Device class to allow this flag to be set, as setting it for a non-v4l2\n> > control probably does not make sense at this point.\n> \n> This is the part that bothers me a bit. If the feature is only used\n> internally, it shouldn't be exposed in the public API.\n> \n> One possible workaround would be to add flag controls as being settable\n> in a request and as being reported in metadata. This is a feature that\n> is useful for applications, and it could then be used internally do\n> indicate read-only internal controls.\n> \"\"\"\n\nI think it makes sense to decouple the two issues, fixing the hack as\ndone in this patch, and considering extensions to ControlInfo\nseparately.\n\n> > > > Signed-off-by: Alain Volmat \n> > > > ---\n> > > > src/libcamera/camera_sensor.cpp | 17 ++++-------------\n> > > > 1 file changed, 4 insertions(+), 13 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > > index 0ef78d9c..3281c1f9 100644\n> > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > @@ -188,21 +188,12 @@ int CameraSensor::init()\n> > > > * Set HBLANK to the minimum to start with a well-defined line length,\n> > > > * allowing IPA modules that do not modify HBLANK to use the sensor\n> > > > * minimum line length in their calculations.\n> > > > - *\n> > > > - * At present, there is no way of knowing if a control is read-only.\n> > > > - * As a workaround, assume that if the minimum and maximum values of\n> > > > - * the V4L2_CID_HBLANK control are the same, it implies the control\n> > > > - * is read-only.\n> > > > - *\n> > > > - * \\todo The control API ought to have a flag to specify if a control\n> > > > - * is read-only which could be used below.\n> > > > */\n> > > > if (ctrls.infoMap()->find(V4L2_CID_HBLANK) != ctrls.infoMap()->end()) {\n> > > > - const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > > > - const int32_t hblankMin = hblank.min().get();\n> > > > - const int32_t hblankMax = hblank.max().get();\n> > > > -\n> > > > - if (hblankMin != hblankMax) {\n> > > > + const struct v4l2_query_ext_ctrl *hblankInfo = subdev_->controlInfo(V4L2_CID_HBLANK);\n> > > > + if (!(hblankInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) {\n\nThe patch I sent drops the ctrls.infoMap()->find(V4L2_CID_HBLANK) check\nabove, and instead checks that hblankInfo is not null. The rest of the\nseries drops the ctrls variable from the context where tha HBLANK\nhandling is located, so I would prefer keeping that. \n\n> > >\n> > > I don't know if it actually helps or makes sense yet, but if I got here\n> > > first I was going to see if it would make sense to put a helper into the\n> > > v4l2_device class to make this more succinct (but still V4L2 specific\n> > > rather than Control specific).\n> > >\n> > > Either with or without a helper - both locations covered would earn a\n> > > tag from me.\n> > >\n> > > > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > > > + const int32_t hblankMin = hblank.min().get();\n> > > > ControlList ctrl(subdev_->controls());\n> > > >\n> > > > ctrl.set(V4L2_CID_HBLANK, hblankMin);","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 213DDBD160\n\tfor ;\n\tTue, 5 Mar 2024 11:14:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8530B61C92;\n\tTue, 5 Mar 2024 12:14:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3974C61C8D\n\tfor ;\n\tTue, 5 Mar 2024 12:14:31 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1F46D2B3;\n\tTue, 5 Mar 2024 12:14:14 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VA0HobD5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709637254;\n\tbh=jCj33HdjktiEQmXBXKHvZ5T551UZpMRgtj2F0XYuyh8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VA0HobD52QOO8BVSwSKkdnEK9TOLqerRy9iJHgB+2cb1hf2kBYLurnTvY7/HCAJ8q\n\tWMoIERduvaHA4yQmjl6sxYchfCKzROCLjYgFCGRbRFBT//Qs0586xHHZwAlaU9Zrpc\n\t7SaObHEyBuyFbLxN2X/qvchl+KR1Y5lLrMbQiRek=","Date":"Tue, 5 Mar 2024 13:14:32 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: fix HBLANK\n\tRO check","Message-ID":"<20240305111432.GA24759@pendragon.ideasonboard.com>","References":"<20231120184529.730565-1-alain.volmat@foss.st.com>\n\t<170051925245.133283.13641407666207121882@ping.linuxembedded.co.uk>\n\t\n\t<170057232541.451646.11994781400031083073@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<170057232541.451646.11994781400031083073@ping.linuxembedded.co.uk>","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":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]