[{"id":38394,"web_url":"https://patchwork.libcamera.org/comment/38394/","msgid":"<177435837261.3380983.9882713992486460000@ping.linuxembedded.co.uk>","date":"2026-03-24T13:19:32","subject":"Re: [PATCH v1 1/2] libcamera: v4l2_device: setControls(): Do not\n\treturn index","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2026-03-24 12:32:13)\n> This behaviour should be removed for the following reasons:\n> \n> * a `ControlList` is an unordered container, an index is largely meaningless;\n> * most callers either ignore the return value, or blindly return it, likely\n> contradicting their own documentation (e.g. `CameraSensorLegacy::setFormat()`),\n> and the error is already very visibly logged;\n> * an error index of 0 is indistinguishable from success with this approach.\n> \n> Instead, return `-EIO` instead of the index.\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> Documentation/guides/pipeline-handler.rst | 6 ++----\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +-\n> src/libcamera/pipeline/vimc/vimc.cpp | 2 +-\n> src/libcamera/sensor/camera_sensor.cpp | 15 +--------------\n> src/libcamera/v4l2_device.cpp | 14 ++++++--------\n> 5 files changed, 11 insertions(+), 28 deletions(-)\n> \n> diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst\n> index 083cdb745..b33d8b857 100644\n> --- a/Documentation/guides/pipeline-handler.rst\n> +++ b/Documentation/guides/pipeline-handler.rst\n> @@ -1076,7 +1076,7 @@ device:\n> ret = data->video_->setControls(&controls);\n> if (ret) {\n> LOG(VIVID, Error) << \"Failed to set controls: \" << ret;\n> - return ret < 0 ? ret : -EINVAL;\n> + return ret;\n> }\n> \n> These controls configure VIVID to use a default test pattern, and enable all\n> @@ -1285,10 +1285,8 @@ before being set.\n> << \" to \" << ctrl.second.toString();\n> \n> int ret = data->video_->setControls(&controls);\n> - if (ret) {\n> + if (ret)\n> LOG(VIVID, Error) << \"Failed to set controls: \" << ret;\n> - return ret < 0 ? ret : -EINVAL;\n> - }\n> \n> return ret;\n> }\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 3bd51733d..3435a7604 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -439,7 +439,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, const ControlList &\n> int ret = data->video_->setControls(&controls);\n> if (ret) {\n> LOG(UVC, Error) << \"Failed to set controls: \" << ret;\n> - return ret < 0 ? ret : -EINVAL;\n> + return ret;\n> }\n> \n> return ret;\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 08b100752..716f90818 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -434,7 +434,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> int ret = data->sensor_->setControls(&controls);\n> if (ret) {\n> LOG(VIMC, Error) << \"Failed to set controls: \" << ret;\n> - return ret < 0 ? ret : -EINVAL;\n> + return ret;\n> }\n> \n> return ret;\n> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n> index 05390d1e1..602518afc 100644\n> --- a/src/libcamera/sensor/camera_sensor.cpp\n> +++ b/src/libcamera/sensor/camera_sensor.cpp\n> @@ -379,21 +379,8 @@ int CameraSensor::setEmbeddedDataEnabled(bool enable)\n> * ctrls entry. The control identifiers are defined by the V4L2 specification\n> * (V4L2_CID_*).\n> *\n> - * If any control in \\a ctrls is not supported by the device, is disabled (i.e.\n> - * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other\n> - * error occurs during validation of the requested controls, no control is\n> - * written and this function returns -EINVAL.\n> - *\n> - * If an error occurs while writing the controls, the index of the first\n> - * control that couldn't be written is returned. All controls below that index\n> - * are written and their values are updated in \\a ctrls, while all other\n> - * controls are not written and their values are not changed.\n> - *\n> - * \\sa V4L2Device::setControls()\n> - *\n> * \\return 0 on success or an error code otherwise\n> - * \\retval -EINVAL One of the control is not supported or not accessible\n> - * \\retval i The index of the control that failed\n> + * \\sa V4L2Device::setControls()\n> */\n> \n> /**\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index b49d73b1c..53bd7865a 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -285,18 +285,16 @@ ControlList V4L2Device::getControls(Span ids, const V4L2Request\n> * \\param[in] request An optional request\n> *\n> * This function writes the value of all controls contained in \\a ctrls, and\n> - * stores the values actually applied to the device in the corresponding\n> - * \\a ctrls entry.\n> + * updates \\a ctrls with the values actually applied to the device.\n> *\n> * If any control in \\a ctrls is not supported by the device, is disabled (i.e.\n> * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, if any other error\n> * occurs during validation of the requested controls, no control is written and\n> * this function returns -EINVAL.\n> *\n> - * If an error occurs while writing the controls, the index of the first\n> - * control that couldn't be written is returned. All controls below that index\n> - * are written and their values are updated in \\a ctrls, while all other\n> - * controls are not written and their values are not changed.\n> + * If an error occurs while writing the controls, -EIO is returned. All controls\n> + * that were successfully written have their values are updated in \\a ctrls,\n\nSomething isn't right in the grammar here.\n\nPerhaps this meant: s/values are/are/g ?\n\n If an error occurs while writing the controls, -EIO is returned. All\n controls that were successfully written have their values updated in\n \\a ctrls, while all other controls are not written and their values\n are not changed.\n\nI guess it's hard to know if a value was or wasn't written, as there's\nno way to know if the value was changed, or just stayed the same...\n\nBut aside from that ...\n\n\nReviewed-by: Kieran Bingham \n\n> + * while all other controls are not written and their values are not changed.\n> *\n> * If \\a request is set, the controls will be applied to that request. If the\n> * device doesn't support requests, -EACCESS will be returned. If \\a request is\n> @@ -304,8 +302,8 @@ ControlList V4L2Device::getControls(Span ids, const V4L2Request\n> *\n> * \\return 0 on success or an error code otherwise\n> * \\retval -EINVAL One of the controls is not supported or not accessible\n> + * \\retval -EIO One or more controls were rejected by the device\n> * \\retval -EACCESS The device does not support requests\n> - * \\retval i The index of the control that failed\n> */\n> int V4L2Device::setControls(ControlList *ctrls, const V4L2Request *request)\n> {\n> @@ -419,7 +417,7 @@ int V4L2Device::setControls(ControlList *ctrls, const V4L2Request *request)\n> << \": \" << strerror(-ret);\n> \n> v4l2Ctrls.resize(errorIdx);\n> - ret = errorIdx;\n> + ret = -EIO;\n> }\n> \n> updateControls(ctrls, v4l2Ctrls);\n> -- \n> 2.53.0\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 7B922BD87C\n\tfor ;\n\tTue, 24 Mar 2026 13:19:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79AB662787;\n\tTue, 24 Mar 2026 14:19: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 AD74B62780\n\tfor ;\n\tTue, 24 Mar 2026 14:19:35 +0100 (CET)","from monstersaurus.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 52D5D802;\n\tTue, 24 Mar 2026 14:18:18 +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=\"A1pZftTl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1774358298;\n\tbh=wDbTTGKS6Y4MdQZdNblmrtfZuEsEBDGT8SUxjZy8kKU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=A1pZftTlra+oGK1MiBpOoo4HhLtP+nwPbNCzUiuIyTbOqVb1dzyCf0L25XBugz9hR\n\t+1JdU8Epg2knEDVHPzqOWQj1yGeRwy04iIWB+A+l4Uwxu8VBAnlDXqgpDZWbEqhhCA\n\tA9i6dDdZJjjffs8UUxTvuoLVcplpgQI/SkAatqMk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20260324123214.1762198-1-barnabas.pocze@ideasonboard.com>","References":"<20260324123214.1762198-1-barnabas.pocze@ideasonboard.com>","Subject":"Re: [PATCH v1 1/2] libcamera: v4l2_device: setControls(): Do not\n\treturn index","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 24 Mar 2026 13:19:32 +0000","Message-ID":"<177435837261.3380983.9882713992486460000@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":38402,"web_url":"https://patchwork.libcamera.org/comment/38402/","msgid":"<047a4c50-7c76-4179-8386-eca4a4445b52@ideasonboard.com>","date":"2026-03-25T08:38:17","subject":"Re: [PATCH v1 1/2] libcamera: v4l2_device: setControls(): Do not\n\treturn index","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2026. 03. 24. 14:19 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2026-03-24 12:32:13)\n>> This behaviour should be removed for the following reasons:\n>>\n>> * a `ControlList` is an unordered container, an index is largely meaningless;\n>> * most callers either ignore the return value, or blindly return it, likely\n>> contradicting their own documentation (e.g. `CameraSensorLegacy::setFormat()`),\n>> and the error is already very visibly logged;\n>> * an error index of 0 is indistinguishable from success with this approach.\n>>\n>> Instead, return `-EIO` instead of the index.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> Documentation/guides/pipeline-handler.rst | 6 ++----\n>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +-\n>> src/libcamera/pipeline/vimc/vimc.cpp | 2 +-\n>> src/libcamera/sensor/camera_sensor.cpp | 15 +--------------\n>> src/libcamera/v4l2_device.cpp | 14 ++++++--------\n>> 5 files changed, 11 insertions(+), 28 deletions(-)\n>>\n>> diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst\n>> index 083cdb745..b33d8b857 100644\n>> --- a/Documentation/guides/pipeline-handler.rst\n>> +++ b/Documentation/guides/pipeline-handler.rst\n>> @@ -1076,7 +1076,7 @@ device:\n>> ret = data->video_->setControls(&controls);\n>> if (ret) {\n>> LOG(VIVID, Error) << \"Failed to set controls: \" << ret;\n>> - return ret < 0 ? ret : -EINVAL;\n>> + return ret;\n>> }\n>> \n>> These controls configure VIVID to use a default test pattern, and enable all\n>> @@ -1285,10 +1285,8 @@ before being set.\n>> << \" to \" << ctrl.second.toString();\n>> \n>> int ret = data->video_->setControls(&controls);\n>> - if (ret) {\n>> + if (ret)\n>> LOG(VIVID, Error) << \"Failed to set controls: \" << ret;\n>> - return ret < 0 ? ret : -EINVAL;\n>> - }\n>> \n>> return ret;\n>> }\n>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> index 3bd51733d..3435a7604 100644\n>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> @@ -439,7 +439,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, const ControlList &\n>> int ret = data->video_->setControls(&controls);\n>> if (ret) {\n>> LOG(UVC, Error) << \"Failed to set controls: \" << ret;\n>> - return ret < 0 ? ret : -EINVAL;\n>> + return ret;\n>> }\n>> \n>> return ret;\n>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n>> index 08b100752..716f90818 100644\n>> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n>> @@ -434,7 +434,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n>> int ret = data->sensor_->setControls(&controls);\n>> if (ret) {\n>> LOG(VIMC, Error) << \"Failed to set controls: \" << ret;\n>> - return ret < 0 ? ret : -EINVAL;\n>> + return ret;\n>> }\n>> \n>> return ret;\n>> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n>> index 05390d1e1..602518afc 100644\n>> --- a/src/libcamera/sensor/camera_sensor.cpp\n>> +++ b/src/libcamera/sensor/camera_sensor.cpp\n>> @@ -379,21 +379,8 @@ int CameraSensor::setEmbeddedDataEnabled(bool enable)\n>> * ctrls entry. The control identifiers are defined by the V4L2 specification\n>> * (V4L2_CID_*).\n>> *\n>> - * If any control in \\a ctrls is not supported by the device, is disabled (i.e.\n>> - * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other\n>> - * error occurs during validation of the requested controls, no control is\n>> - * written and this function returns -EINVAL.\n>> - *\n>> - * If an error occurs while writing the controls, the index of the first\n>> - * control that couldn't be written is returned. All controls below that index\n>> - * are written and their values are updated in \\a ctrls, while all other\n>> - * controls are not written and their values are not changed.\n>> - *\n>> - * \\sa V4L2Device::setControls()\n>> - *\n>> * \\return 0 on success or an error code otherwise\n>> - * \\retval -EINVAL One of the control is not supported or not accessible\n>> - * \\retval i The index of the control that failed\n>> + * \\sa V4L2Device::setControls()\n>> */\n>> \n>> /**\n>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>> index b49d73b1c..53bd7865a 100644\n>> --- a/src/libcamera/v4l2_device.cpp\n>> +++ b/src/libcamera/v4l2_device.cpp\n>> @@ -285,18 +285,16 @@ ControlList V4L2Device::getControls(Span ids, const V4L2Request\n>> * \\param[in] request An optional request\n>> *\n>> * This function writes the value of all controls contained in \\a ctrls, and\n>> - * stores the values actually applied to the device in the corresponding\n>> - * \\a ctrls entry.\n>> + * updates \\a ctrls with the values actually applied to the device.\n>> *\n>> * If any control in \\a ctrls is not supported by the device, is disabled (i.e.\n>> * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, if any other error\n>> * occurs during validation of the requested controls, no control is written and\n>> * this function returns -EINVAL.\n>> *\n>> - * If an error occurs while writing the controls, the index of the first\n>> - * control that couldn't be written is returned. All controls below that index\n>> - * are written and their values are updated in \\a ctrls, while all other\n>> - * controls are not written and their values are not changed.\n>> + * If an error occurs while writing the controls, -EIO is returned. All controls\n>> + * that were successfully written have their values are updated in \\a ctrls,\n> \n> Something isn't right in the grammar here.\n> \n> Perhaps this meant: s/values are/are/g ?\n> \n> If an error occurs while writing the controls, -EIO is returned. All\n> controls that were successfully written have their values updated in\n> \\a ctrls, while all other controls are not written and their values\n> are not changed.\n\nThanks, that's was indeed my intention.\n\n\n> \n> I guess it's hard to know if a value was or wasn't written, as there's\n> no way to know if the value was changed, or just stayed the same...\n\nYes, arguably this interface is not easy to use. I imagine every reference\nto this behaviour could be removed, since it is not used in the code base\nat the moment, and `ret` could just be returned as is.\n\n\n> \n> But aside from that ...\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n>> + * while all other controls are not written and their values are not changed.\n>> *\n>> * If \\a request is set, the controls will be applied to that request. If the\n>> * device doesn't support requests, -EACCESS will be returned. If \\a request is\n>> @@ -304,8 +302,8 @@ ControlList V4L2Device::getControls(Span ids, const V4L2Request\n>> *\n>> * \\return 0 on success or an error code otherwise\n>> * \\retval -EINVAL One of the controls is not supported or not accessible\n>> + * \\retval -EIO One or more controls were rejected by the device\n>> * \\retval -EACCESS The device does not support requests\n>> - * \\retval i The index of the control that failed\n>> */\n>> int V4L2Device::setControls(ControlList *ctrls, const V4L2Request *request)\n>> {\n>> @@ -419,7 +417,7 @@ int V4L2Device::setControls(ControlList *ctrls, const V4L2Request *request)\n>> << \": \" << strerror(-ret);\n>> \n>> v4l2Ctrls.resize(errorIdx);\n>> - ret = errorIdx;\n>> + ret = -EIO;\n>> }\n>> \n>> updateControls(ctrls, v4l2Ctrls);\n>> -- \n>> 2.53.0\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 D57AFBD87C\n\tfor ;\n\tWed, 25 Mar 2026 08:38:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C855D62791;\n\tWed, 25 Mar 2026 09:38:22 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 993FC620FA\n\tfor ;\n\tWed, 25 Mar 2026 09:38:20 +0100 (CET)","from [192.168.33.22] (185.221.143.129.nat.pool.zt.hu\n\t[185.221.143.129])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BB6B21123;\n\tWed, 25 Mar 2026 09:37:02 +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=\"Koa1wWPg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1774427822;\n\tbh=1IGvl5yH7skqQSVLAUDsCASWHqkZ9UyYFldC2aqgDro=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Koa1wWPgTMGhF8toJmySiHCNl4LSsX+CPr74CHBa7X/3k3t5mIXUAQP/cWdxqJGZc\n\tVbAIGVcUSJxzZcii59C7kSAFVj8UptqoQPibEEqFKXSVIh4G/fv3mrR5ZTMviyv4OY\n\tURvs5R90MXCIY5vvqEPfeFBclrt1CVUwofyMYH3o=","Message-ID":"<047a4c50-7c76-4179-8386-eca4a4445b52@ideasonboard.com>","Date":"Wed, 25 Mar 2026 09:38:17 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 1/2] libcamera: v4l2_device: setControls(): Do not\n\treturn index","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20260324123214.1762198-1-barnabas.pocze@ideasonboard.com>\n\t<177435837261.3380983.9882713992486460000@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<177435837261.3380983.9882713992486460000@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]