[{"id":28086,"web_url":"https://patchwork.libcamera.org/comment/28086/","msgid":"<169988207429.2466661.5037786340380259121@ping.linuxembedded.co.uk>","date":"2023-11-13T13:27:54","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: only access\n\tV4L_CID_HBLANK if existing","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-13 10:08:51)\n> Correct a crash in CameraSensor::init when trying to set the\n> V4L2_CID_HBLANK control on sensor not implementing this control.\n> The HBLANK sensor not being mandatory for non-RAW sensors, it can\n> happen that the sensor does not expose this control.\n> Perform check against availability of the control prior to usage in\n> order to avoid the crash.\n> \n> Signed-off-by: Alain Volmat \n> ---\n> src/libcamera/camera_sensor.cpp | 24 +++++++++++++-----------\n> 1 file changed, 13 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index d9261672..0ef78d9c 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -197,17 +197,19 @@ int CameraSensor::init()\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> - 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> - ControlList ctrl(subdev_->controls());\n> -\n> - ctrl.set(V4L2_CID_HBLANK, hblankMin);\n> - ret = subdev_->setControls(&ctrl);\n> - if (ret)\n> - return ret;\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> + ControlList ctrl(subdev_->controls());\n> +\n> + ctrl.set(V4L2_CID_HBLANK, hblankMin);\n> + ret = subdev_->setControls(&ctrl);\n> + if (ret)\n> + return ret;\n> + }\n\nDo we have a way to know if we're dealing with a raw or yuv/rgb sensor\nin the CameraSensor class presently?\n\nIt might be beneficial to make sure we are explicit that this is not\nrequired when !rawSensor here.\n\nI see in validateSensorDriver() we already check V4L2_CID_HBLANK is a\nmandatoryControl after a check on \n\n if (!bayerFormat_)\n\nso I guess that's our current switch point.\n\nIs the HBLANK usable at all if there is no bayerFormat?\n\nI'm also weary about the usage at CameraSensor::sensorInfo() where there\nis a call to:\n\n\n /*\n * Retrieve the pixel rate, line length and minimum/maximum frame\n * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,\n * V4L2_CID_HBLANK and V4L2_CID_VBLANK controls is mandatory.\n */\n ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,\n V4L2_CID_HBLANK,\n V4L2_CID_VBLANK });\n if (ctrls.empty()) {\n LOG(CameraSensor, Error)\n << \"Failed to retrieve camera info controls\";\n return -EINVAL;\n }\n\nDoes supporting !HBLANK require a larger rework ? or is this the only\nplace that you have hit which requires a check?\n\n--\nKieran\n\n\n\n> }\n> \n> return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);\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 59D18BDE6B\n\tfor ;\n\tMon, 13 Nov 2023 13:28:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BBB9C629BC;\n\tMon, 13 Nov 2023 14:27:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19AE4629A9\n\tfor ;\n\tMon, 13 Nov 2023 14:27:58 +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 0F9B610A;\n\tMon, 13 Nov 2023 14:27:32 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1699882079;\n\tbh=wHJ3LotrZJJuCF7q8DXwbPKAWwNhENISwuKrKKR5PW4=;\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:\n\tFrom;\n\tb=YT5f+JmCl7p+7TnoHpiIvU8cAZF51c7tBr0FFuV9htSgjIcBKN+0lkfIMVaIhg+qT\n\tF61p9aG0mCgnvLa1bHV2LJ9Mxs7/CSGyuraaHhCofADTy6e4hlgmcihfJKTp+Ruqzt\n\tAPL7XCkftDveK4xQlxiNkEWQD3PxVIdtETsc/AFh6RHArYXxKR0N9091CIBJ6QhM7s\n\tje8X9vsmHcbUjozMrdp1ZhgA4aO/i6UQWq3W9MwLbFtJesy3iNHZvnkwjyrPzV1GpQ\n\tyHfB+UUHGREI1JsUf9dJy2WBpMi7I/9C5+yPHkblgSMgaWqo756MgFaGGbAlRvPKB/\n\tDlBts5erf3JJQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1699882052;\n\tbh=wHJ3LotrZJJuCF7q8DXwbPKAWwNhENISwuKrKKR5PW4=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=YHHDLRDzyUSj2SSrtFzS0uN+pDzeoqsETIhnMfhCl2F8O9qc9RM7EzIR9+1woIK9p\n\tH07m8vRAHMTMqZOoerKIM3ZxUL0CyvUY8+NGYw8AJSej8CpLz9y6k7VadCa809dxFw\n\tapE4zmaOWvL36TjwGrVwo3IO6YkURFv8VBBy4xJM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YHHDLRDz\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20231113100851.73886-1-alain.volmat@foss.st.com>","References":"<20231113100851.73886-1-alain.volmat@foss.st.com>","To":"Alain Volmat ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 13 Nov 2023 13:27:54 +0000","Message-ID":"<169988207429.2466661.5037786340380259121@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: only access\n\tV4L_CID_HBLANK if existing","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28087,"web_url":"https://patchwork.libcamera.org/comment/28087/","msgid":"<20231113144508.GA89055@gnbcxd0016.gnb.st.com>","date":"2023-11-13T14:45:08","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: only access\n\tV4L_CID_HBLANK if existing","submitter":{"id":162,"url":"https://patchwork.libcamera.org/api/people/162/","name":"Alain Volmat","email":"alain.volmat@foss.st.com"},"content":"Hi Kieran,\n\nOn Mon, Nov 13, 2023 at 01:27:54PM +0000, Kieran Bingham wrote:\n> Hi Alain,\n> \n> Quoting Alain Volmat via libcamera-devel (2023-11-13 10:08:51)\n> > Correct a crash in CameraSensor::init when trying to set the\n> > V4L2_CID_HBLANK control on sensor not implementing this control.\n> > The HBLANK sensor not being mandatory for non-RAW sensors, it can\n> > happen that the sensor does not expose this control.\n> > Perform check against availability of the control prior to usage in\n> > order to avoid the crash.\n> > \n> > Signed-off-by: Alain Volmat \n> > ---\n> > src/libcamera/camera_sensor.cpp | 24 +++++++++++++-----------\n> > 1 file changed, 13 insertions(+), 11 deletions(-)\n> > \n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index d9261672..0ef78d9c 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -197,17 +197,19 @@ int CameraSensor::init()\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> > - 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> > - ControlList ctrl(subdev_->controls());\n> > -\n> > - ctrl.set(V4L2_CID_HBLANK, hblankMin);\n> > - ret = subdev_->setControls(&ctrl);\n> > - if (ret)\n> > - return ret;\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> > + ControlList ctrl(subdev_->controls());\n> > +\n> > + ctrl.set(V4L2_CID_HBLANK, hblankMin);\n> > + ret = subdev_->setControls(&ctrl);\n> > + if (ret)\n> > + return ret;\n> > + }\n> \n> Do we have a way to know if we're dealing with a raw or yuv/rgb sensor\n> in the CameraSensor class presently?\n> \n> It might be beneficial to make sure we are explicit that this is not\n> required when !rawSensor here.\n\nMy understanding is that HBLANK is mandatory in case of RAW sensors but\noptional for yuv/rgb sensors. However even if optional in case of\nyuv/rgb sensor, it might be the only way to control the framerate\nsometimes (as with the gc2145 for which we've decided to only support\nhblank/vblank since it will support both yuv/rgb and raw).\n\n> \n> I see in validateSensorDriver() we already check V4L2_CID_HBLANK is a\n> mandatoryControl after a check on \n> \n> if (!bayerFormat_)\n> \n> so I guess that's our current switch point.\n> \n> Is the HBLANK usable at all if there is no bayerFormat?\n\nYes, on GC2145 that's usable.\n\n> \n> I'm also weary about the usage at CameraSensor::sensorInfo() where there\n> is a call to:\n> \n> \n> /*\n> * Retrieve the pixel rate, line length and minimum/maximum frame\n> * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,\n> * V4L2_CID_HBLANK and V4L2_CID_VBLANK controls is mandatory.\n> */\n> ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,\n> V4L2_CID_HBLANK,\n> V4L2_CID_VBLANK });\n> if (ctrls.empty()) {\n> LOG(CameraSensor, Error)\n> << \"Failed to retrieve camera info controls\";\n> return -EINVAL;\n> }\n> \n> Does supporting !HBLANK require a larger rework ? or is this the only\n> place that you have hit which requires a check?\n\nAt least this function only make sense in case of raw format and will\nreturn rapidly EINVAL when called on a rgb/yuv format sensor, so this\npoint won't get reached for a yuv/rgb sensor.\n\n> \n> --\n> Kieran\n> \n> \n> \n> > }\n> > \n> > return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);\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 911D8C3213\n\tfor ;\n\tMon, 13 Nov 2023 14:45:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CFC6B629AF;\n\tMon, 13 Nov 2023 15:45:18 +0100 (CET)","from mx08-00178001.pphosted.com (mx08-00178001.pphosted.com\n\t[91.207.212.93])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A217E629A9\n\tfor ;\n\tMon, 13 Nov 2023 15:45:16 +0100 (CET)","from pps.filterd (m0369457.ppops.net [127.0.0.1])\n\tby mx07-00178001.pphosted.com (8.17.1.22/8.17.1.22) with ESMTP id\n\t3ADB7McC018996; Mon, 13 Nov 2023 15:45:15 +0100","from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35])\n\tby mx07-00178001.pphosted.com (PPS) with ESMTPS id 3uanenn1yq-1\n\t(version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256\n\tverify=NOT); Mon, 13 Nov 2023 15:45:15 +0100 (CET)","from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20])\n\tby beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id DCA3D10005B; \n\tMon, 13 Nov 2023 15:45:14 +0100 (CET)","from Webmail-eu.st.com (shfdag1node1.st.com [10.75.129.69])\n\tby euls16034.sgp.st.com (STMicroelectronics) with ESMTP id\n\tCE33121861F; Mon, 13 Nov 2023 15:45:14 +0100 (CET)","from gnbcxd0016.gnb.st.com (10.129.178.213) by SHFDAG1NODE1.st.com\n\t(10.75.129.69) with Microsoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.27;\n\tMon, 13 Nov 2023 15:45:14 +0100"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1699886718;\n\tbh=jfnBk0RDarO4PPY2uGJrNe6Y6mlYOb3wDDqkYg8dK1s=;\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=sFe+0tal84iS/9Lv/XsS5fCnsVaSam/tsKZgXcBBUiAe3l6OJNYYB18Frt/lfFNEJ\n\texjbz2GVqBHuBhsU8PQYScm+FyjmEftZ8kh73miRCMAbs50pnbXPJtK9OO0zO/jnqq\n\tr1S6KMlH64NWw3Ss1sg47XcIOsb5OQZ/i8CyxxXtjHJUiujrZWfupctuVairOicZb6\n\t98nyZBqj1VYHbdubE5Ny2OTv6uv1OCL4H/YQnRFR/YGqaXeoGE2qzoB7WNe2THYfjz\n\txAqZhnjaBgUUp0kIohHHUOvseNdziuLLC+X7u/2tue/GQeKJ/C4/FTN97wStd6r04S\n\tUSp1/yN4UkKug==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h=\n\tdate:from:to:cc:subject:message-id:references:mime-version\n\t:content-type:in-reply-to; s=selector1; bh=4zkiiHWB8RPw2mVCHBg+S\n\t6UMDREJehjle97/B4DfPw4=; b=tPFgM0LmEJNusvgYEqTFVTzUXgI2udU6o3U69\n\t4HdLW6Buz8f4NwtC7GC+VCFMvr9L29qY4hbBL4kn5jJNtQhmesh3B+7qP74CfixL\n\tN/UXAuiiwyOKantPaiQ3RLPTDHNreSpEgU5exfaw/XOt1b46VL/oGEpH53ijS98C\n\t2Db2bT/EJjRnbCHlwt9VnoIAe9sfZLuX3DNoPgxwZVd5R71Jas+MSGRJVvePi/bu\n\tUQuXQ/kgfU1oOBFoyFSbt0qygUk87Jnm5MMZvhDZpcRRK3aqHqiDw7XabAVXB00Q\n\tmLB9u0DC/yRYhEE2kQScWpf3t1f1OcfH9vZt3FFtRH2VPfpNQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=foss.st.com\n\theader.i=@foss.st.com header.b=\"tPFgM0Lm\"; \n\tdkim-atps=neutral","Date":"Mon, 13 Nov 2023 15:45:08 +0100","To":"Kieran Bingham ","Message-ID":"<20231113144508.GA89055@gnbcxd0016.gnb.st.com>","References":"<20231113100851.73886-1-alain.volmat@foss.st.com>\n\t<169988207429.2466661.5037786340380259121@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Disposition":"inline","In-Reply-To":"<169988207429.2466661.5037786340380259121@ping.linuxembedded.co.uk>","X-Disclaimer":"ce message est personnel / this message is private","X-Originating-IP":"[10.129.178.213]","X-ClientProxiedBy":"SHFCAS1NODE2.st.com (10.75.129.73) To SHFDAG1NODE1.st.com\n\t(10.75.129.69)","X-Proofpoint-Virus-Version":"vendor=baseguard\n\tengine=ICAP:2.0.272, Aquarius:18.0.987, Hydra:6.0.619,\n\tFMLib:17.11.176.26\n\tdefinitions=2023-11-13_05,2023-11-09_01,2023-05-22_02","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: only access\n\tV4L_CID_HBLANK if existing","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":"Alain Volmat via libcamera-devel ","Reply-To":"Alain Volmat ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28094,"web_url":"https://patchwork.libcamera.org/comment/28094/","msgid":"","date":"2023-11-15T14:57:23","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: only access\n\tV4L_CID_HBLANK if existing","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Alain\n\nOn Mon, Nov 13, 2023 at 11:08:51AM +0100, Alain Volmat via libcamera-devel wrote:\n> Correct a crash in CameraSensor::init when trying to set the\n> V4L2_CID_HBLANK control on sensor not implementing this control.\n> The HBLANK sensor not being mandatory for non-RAW sensors, it can\n> happen that the sensor does not expose this control.\n> Perform check against availability of the control prior to usage in\n> order to avoid the crash.\n>\n> Signed-off-by: Alain Volmat \n> ---\n> src/libcamera/camera_sensor.cpp | 24 +++++++++++++-----------\n> 1 file changed, 13 insertions(+), 11 deletions(-)\n>\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index d9261672..0ef78d9c 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -197,17 +197,19 @@ int CameraSensor::init()\n> \t * \\todo The control API ought to have a flag to specify if a control\n> \t * is read-only which could be used below.\n> \t */\n\nActually the above comment suggest that there's no way to know if a\ncontrol is RO, while I see\n\n\tif (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) &&\n\t vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY))\n\nin other parts of the class.\n\nThis is unrelated to this change though\n\n> -\tconst ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> -\tconst int32_t hblankMin = hblank.min().get();\n> -\tconst int32_t hblankMax = hblank.max().get();\n> -\n> -\tif (hblankMin != hblankMax) {\n> -\t\tControlList ctrl(subdev_->controls());\n> -\n> -\t\tctrl.set(V4L2_CID_HBLANK, hblankMin);\n> -\t\tret = subdev_->setControls(&ctrl);\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n> +\tif (ctrls.infoMap()->find(V4L2_CID_HBLANK) != ctrls.infoMap()->end()) {\n\nMost YUV/RGB sensor drivers I know of do not support HBLANK, so I\nwould have expected this issue to be hit earlier than this. Possibly\nlibcamera is not very populare with YUV/RGB sensor :)\n\n> +\t\tconst ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> +\t\tconst int32_t hblankMin = hblank.min().get();\n> +\t\tconst int32_t hblankMax = hblank.max().get();\n> +\n> +\t\tif (hblankMin != hblankMax) {\n> +\t\t\tControlList ctrl(subdev_->controls());\n> +\n> +\t\t\tctrl.set(V4L2_CID_HBLANK, hblankMin);\n> +\t\t\tret = subdev_->setControls(&ctrl);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\t\t}\n\nThe patch looks good to me\nReviewed-by: Jacopo Mondi \n\n> \t}\n>\n> \treturn applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);\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 81491BDE6B\n\tfor ;\n\tWed, 15 Nov 2023 14:57:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE6C7629BC;\n\tWed, 15 Nov 2023 15:57:27 +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 E8D9961DB4\n\tfor ;\n\tWed, 15 Nov 2023 15:57:26 +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 61F4C8D;\n\tWed, 15 Nov 2023 15:57:00 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1700060247;\n\tbh=cQlZJNVq+/lTSxdvrg5bMlk1IVTagHe0FvPaKlimGjU=;\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=C1JKmuCAvNeOLsYB5DUYp+a0K+AtJCHWb+JqBN6nHxXP03XyOrcmglwfaqyXZwjST\n\tWY3Uvnat6jI6TlZphq2FFlDxqL/KCqo/G89ymdemJkDbqnXfLvl4r6JZm806EWfOSI\n\t2VmR3Xuez/2osTJ6WkHJKR9Id5y/CYSqD4dop3CihMiIHqUeEF9xCqKNigOg+xyjcO\n\tIu1ifLIziMad/gH3yMiktu6HDzD/Wg3uCVTPPnHox+CrJkjAb+mmdUgpvoLeUVT9Oz\n\tX2Qpv2BAnPligHmB++q5fdaiOPcn4mrtCV5UewvIZlYA/RYGp5iCy0MWZUzCiDpYUy\n\tegpLV4Cs1ogTA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1700060220;\n\tbh=cQlZJNVq+/lTSxdvrg5bMlk1IVTagHe0FvPaKlimGjU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SEb7Zk8J+qAqnnnwJNQmeZYWVL9ugwisnzv58ZC3scbLrIy6Pc4P0gRye0as8KW6O\n\tKx/Xu2uz+8VRw0EeuFNUHWNhDG7HYXng88V4Hz7CzJnpUs47J3D1v4V/mGdUqBRQ72\n\tzMZ7NQHuGcanwUZt5qTK3hFWsR7C5uok5D/frLNc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"SEb7Zk8J\"; dkim-atps=neutral","Date":"Wed, 15 Nov 2023 15:57:23 +0100","To":"Alain Volmat ","Message-ID":"","References":"<20231113100851.73886-1-alain.volmat@foss.st.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20231113100851.73886-1-alain.volmat@foss.st.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: only access\n\tV4L_CID_HBLANK if existing","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":28116,"web_url":"https://patchwork.libcamera.org/comment/28116/","msgid":"<20231121094441.GF28790@pendragon.ideasonboard.com>","date":"2023-11-21T09:44:41","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: only access\n\tV4L_CID_HBLANK if existing","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Nov 15, 2023 at 03:57:23PM +0100, Jacopo Mondi via libcamera-devel wrote:\n> Hi Alain\n> \n> On Mon, Nov 13, 2023 at 11:08:51AM +0100, Alain Volmat via libcamera-devel wrote:\n> > Correct a crash in CameraSensor::init when trying to set the\n> > V4L2_CID_HBLANK control on sensor not implementing this control.\n> > The HBLANK sensor not being mandatory for non-RAW sensors, it can\n> > happen that the sensor does not expose this control.\n> > Perform check against availability of the control prior to usage in\n> > order to avoid the crash.\n> >\n> > Signed-off-by: Alain Volmat \n> > ---\n> > src/libcamera/camera_sensor.cpp | 24 +++++++++++++-----------\n> > 1 file changed, 13 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index d9261672..0ef78d9c 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -197,17 +197,19 @@ int CameraSensor::init()\n> > \t * \\todo The control API ought to have a flag to specify if a control\n> > \t * is read-only which could be used below.\n> > \t */\n> \n> Actually the above comment suggest that there's no way to know if a\n> control is RO, while I see\n> \n> \tif (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) &&\n> \t vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY))\n> \n> in other parts of the class.\n> \n> This is unrelated to this change though\n> \n> > -\tconst ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > -\tconst int32_t hblankMin = hblank.min().get();\n> > -\tconst int32_t hblankMax = hblank.max().get();\n> > -\n> > -\tif (hblankMin != hblankMax) {\n> > -\t\tControlList ctrl(subdev_->controls());\n> > -\n> > -\t\tctrl.set(V4L2_CID_HBLANK, hblankMin);\n> > -\t\tret = subdev_->setControls(&ctrl);\n> > -\t\tif (ret)\n> > -\t\t\treturn ret;\n> > +\tif (ctrls.infoMap()->find(V4L2_CID_HBLANK) != ctrls.infoMap()->end()) {\n> \n> Most YUV/RGB sensor drivers I know of do not support HBLANK, so I\n> would have expected this issue to be hit earlier than this. Possibly\n> libcamera is not very populare with YUV/RGB sensor :)\n> \n> > +\t\tconst ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > +\t\tconst int32_t hblankMin = hblank.min().get();\n> > +\t\tconst int32_t hblankMax = hblank.max().get();\n> > +\n> > +\t\tif (hblankMin != hblankMax) {\n> > +\t\t\tControlList ctrl(subdev_->controls());\n> > +\n> > +\t\t\tctrl.set(V4L2_CID_HBLANK, hblankMin);\n> > +\t\t\tret = subdev_->setControls(&ctrl);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\t\t}\n> \n> The patch looks good to me\n> Reviewed-by: Jacopo Mondi \n\nLikewise;\n\nReviewed-by: Laurent Pinchart \n\n> > \t}\n> >\n> > \treturn applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);","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 DE039BDE6B\n\tfor ;\n\tTue, 21 Nov 2023 09:44:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 42702629B6;\n\tTue, 21 Nov 2023 10:44:37 +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 ED25761DAC\n\tfor ;\n\tTue, 21 Nov 2023 10:44:35 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4C7892E4;\n\tTue, 21 Nov 2023 10:44:05 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1700559877;\n\tbh=haZMtVhl832OI8eHCaQKuekbjTSAozJFZZc6Eq+Hh3w=;\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=WujmLX5YEqNe9rCQVu0Wvv86XFEYgcsiu+VpguGgWxSdOSC4dPyvB0xS+bGBGjyB+\n\tdAjPZaGSzPU9+aCujG/f9qmAC/bA/4rIJg+HGV6rRyTY2Lc2juQWsWVCmOH+4t94CQ\n\tF64+jcqL+bPH2qIqpFlQ6waFoqHlERB5bvNs2X6zPbDYIap4HKft9pb/cojqJqPdYp\n\tlI5FEvjw0rZ+NJO8pqdm2VeaxPp6UEqGmEUs6wWaQdSuC3gAAZLJY4bCfn/j6dzGqk\n\tYx9/uD5W9E+IhxLPUaCmcAxQTNYW4E9lvQwgHDe72dGY20WwYbuXUPSG7sJX+AJy/R\n\t1bG/3mrLSB2PQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1700559845;\n\tbh=haZMtVhl832OI8eHCaQKuekbjTSAozJFZZc6Eq+Hh3w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YNIySMZzDjza5kEZtjM5cUk8R/j1bWRDWX/INbzXLSxZ9n53e2NoRJPogooKzZp3F\n\tSBPJVx5ftulpbtkSZKm8SuR4OKCh/ndB/YURY0t4U9S4iLsLY6ya5rmjHTSddN+ENG\n\tA9HaoyrYIYxO1MPaWyZMD36jaKW02DWY5jocfKi8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YNIySMZz\"; dkim-atps=neutral","Date":"Tue, 21 Nov 2023 11:44:41 +0200","To":"Jacopo Mondi ","Message-ID":"<20231121094441.GF28790@pendragon.ideasonboard.com>","References":"<20231113100851.73886-1-alain.volmat@foss.st.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera_sensor: only access\n\tV4L_CID_HBLANK if existing","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\" "}}]