[{"id":16770,"web_url":"https://patchwork.libcamera.org/comment/16770/","msgid":"","date":"2021-05-05T13:54:52","subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi\n\nOn Wed, 5 May 2021 at 14:53, David Plowman\n wrote:\n>\n> The minimum and maximum vblanking can change when a new format is\n> applied to the sensor subdevice, so be sure to retrieve up-to-date\n> values.\n>\n> The V4L2Device acquires the new updateControlInfo() method to perform\n> this function, and which the CameraSensor calls automatically if its\n> setFormat method is used to update the sensor.\n>\n> However, not all pipeline handlers invoke the setFormat method\n> directly, so the new method must be made publicly available for\n> pipeline handlers to call if they need to.\n>\n> Signed-off-by: David Plowman \n> Reviewed-by: Jacopo Mondi \n> Reviewed-by: Kieran Bingham \n\nOops, typo here! Sorry Kieran. Can that be fixed up while applying please?\n\nThanks\nDavid\n\n> ---\n> include/libcamera/internal/camera_sensor.h | 2 ++\n> include/libcamera/internal/v4l2_device.h | 2 ++\n> src/libcamera/camera_sensor.cpp | 32 ++++++++++++++++++-\n> src/libcamera/v4l2_device.cpp | 37 ++++++++++++++++++++++\n> 4 files changed, 72 insertions(+), 1 deletion(-)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 3e98f71b..6ce513b7 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -68,6 +68,8 @@ public:\n> const ControlList &properties() const { return properties_; }\n> int sensorInfo(CameraSensorInfo *info) const;\n>\n> + void updateControlInfo();\n> +\n> protected:\n> std::string logPrefix() const override;\n>\n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index 087f07e7..5ba9b23b 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -42,6 +42,8 @@ public:\n> int setFrameStartEnabled(bool enable);\n> Signal frameStart;\n>\n> + void updateControlInfo();\n> +\n> protected:\n> V4L2Device(const std::string &deviceNode);\n> ~V4L2Device();\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 2887bb69..322678c8 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -656,15 +656,30 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n> /**\n> * \\brief Set the sensor output format\n> * \\param[in] format The desired sensor output format\n> + *\n> + * The ranges of any controls associated with the sensor are also updated.\n> + *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> {\n> - return subdev_->setFormat(pad_, format);\n> + int ret = subdev_->setFormat(pad_, format);\n> + if (ret)\n> + return ret;\n> +\n> + updateControlInfo();\n> + return 0;\n> }\n>\n> /**\n> * \\brief Retrieve the supported V4L2 controls and their information\n> + *\n> + * Control information is updated automatically to reflect the current sensor\n> + * configuration when the setFormat() function is called, without invalidating\n> + * any iterator on the ControlInfoMap. A manual update can also be forced by\n> + * calling the updateControlInfo() function for pipeline handlers that change\n> + * the sensor configuration wihtout using setFormat().\n> + *\n> * \\return A map of the V4L2 controls supported by the sensor\n> */\n> const ControlInfoMap &CameraSensor::controls() const\n> @@ -750,6 +765,11 @@ int CameraSensor::setControls(ControlList *ctrls)\n> * Sensor information is only available for raw sensors. When called for a YUV\n> * sensor, this function returns -EINVAL.\n> *\n> + * Pipeline handlers that do not change the sensor format using the\n> + * CameraSensor::setFormat method may need to call\n> + * CameraSensor::updateControlInfo beforehand, to ensure all the control\n> + * ranges are up to date.\n> + *\n> * \\return 0 on success, a negative error code otherwise\n> */\n> int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> @@ -821,6 +841,16 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> return 0;\n> }\n>\n> +/**\n> + * \\fn void CameraSensor::updateControlInfo()\n> + * \\brief Update the sensor's ControlInfoMap in case they have changed\n> + * \\sa V4L2Device::updateControlInfo()\n> + */\n> +void CameraSensor::updateControlInfo()\n> +{\n> + subdev_->updateControlInfo();\n> +}\n> +\n> std::string CameraSensor::logPrefix() const\n> {\n> return \"'\" + entity_->name() + \"'\";\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 397029ac..515cbdfe 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -511,6 +511,43 @@ void V4L2Device::listControls()\n> controls_ = std::move(ctrls);\n> }\n>\n> +/**\n> +* \\brief Update the information for all device controls\n> + *\n> + * The V4L2Device class caches information about all controls supported by the\n> + * device and exposes it through the controls() and controlInfo() functions.\n> + * Control information may change at runtime, for instance when formats on a\n> + * subdev are modified. When this occurs, this function can be used to refresh\n> + * control information. The information is refreshed in-place, all pointers to\n> + * v4l2_query_ext_ctrl instances previously returned by controlInfo() and\n> + * iterators to the ControlInfoMap returned by controls() remain valid.\n> + *\n> + * Note that control information isn't refreshed automatically is it may be an\n> + * expensive operation. The V4L2Device users are responsible for calling this\n> + * function when required, based on their usage pattern of the class.\n> + */\n> +void V4L2Device::updateControlInfo()\n> +{\n> + for (auto &[controlId, info] : controls_) {\n> + unsigned int id = controlId->id();\n> +\n> + /*\n> + * Assume controlInfo_ has a corresponding entry, as it has been\n> + * generated by listControls().\n> + */\n> + struct v4l2_query_ext_ctrl &ctrl = controlInfo_[id];\n> +\n> + if (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {\n> + LOG(V4L2, Debug)\n> + << \"Could not refresh control \"\n> + << utils::hex(id);\n> + continue;\n> + }\n> +\n> + info = V4L2ControlInfo(ctrl);\n> + }\n> +}\n> +\n> /*\n> * \\brief Update the value of the first \\a count V4L2 controls in \\a ctrls using\n> * values in \\a v4l2Ctrls\n> --\n> 2.20.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 CFAFBBDE7C\n\tfor ;\n\tWed, 5 May 2021 13:55:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 32FFD68911;\n\tWed, 5 May 2021 15:55:05 +0200 (CEST)","from mail-wr1-x434.google.com (mail-wr1-x434.google.com\n\t[IPv6:2a00:1450:4864:20::434])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8E4AB68901\n\tfor ;\n\tWed, 5 May 2021 15:55:03 +0200 (CEST)","by mail-wr1-x434.google.com with SMTP id z6so1953046wrm.4\n\tfor ;\n\tWed, 05 May 2021 06:55:03 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"kedzpORk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to;\n\tbh=y+ElsJ2qjLIHdeET98NSjZy3teQg6SH5cKS7lO0VHIs=;\n\tb=kedzpORkq5LCIhS9/7BiONKvFV1jNeWnYN4XwIiq00hOHzf/K8AmHnw+J+nr8ZuiWQ\n\t8CRpaGGmbUReZpMV0Wfsls2TX5logB7sZpyMPnlk6IsqX9ta/wRvwv/cp9Q7TMurM42g\n\t/5tHwfdVjyrzVhfngXT7dfz2SSyiDTSMSvzmRQtSdNIdKgysi+OWZkXMMPA4IA6nHyAl\n\tzbySRnsUbCSCzZBHFPbLl+TiEzqV4fAvjlXguFICY98e4IdHgSd5lk5F3/3EdkgijYFh\n\t4SUbcrlb+owUGrW1NKXR0Ey4HwOZMXpkQ/w/EAn0y6tAHtyWmCpviyU/DAzfbVz0WDOE\n\toEtg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to;\n\tbh=y+ElsJ2qjLIHdeET98NSjZy3teQg6SH5cKS7lO0VHIs=;\n\tb=g2FWb6vu43NdHcc3zh0NNVbCRFYwIend8kkINOhN0RTKwr5G/4M8jKIQ2QUH/1O6K2\n\tk1wFKp6omjAAmG9bTmRU/ONU5BndeQBM0XfON/ZeeAAexY09807k3a9mIixDOAsv9goJ\n\tBuOkHqNilHmF1And5/8YcO6tg9BWcNfqjNezbtXfkYS9aRVpfQngswtEmKO+LdTgTLFl\n\tOn+exTFjIgJxMBp26MXYZaH4F5JveQD5r0jOJqcdfonsL+bDRorwUZP1+Q1zD8Mpyzam\n\t0xVU8CsWnl8Muf9Dwj6y6UHeLz7SWkNwgiHN8QaKkspXVqipI7TOi0/Vv3IITM974a+c\n\tgy1Q==","X-Gm-Message-State":"AOAM531XowjvKD+UsxzZzbSINi53AWc2XqH6N6vAc93qaAaGy6f7c4xr\n\tyZBhNCFsGeq6tn1QBhCQCkjmfjSgJfIFTdpU6xz/zHhM30Q=","X-Google-Smtp-Source":"ABdhPJzLNqOqnpRy6da0bvOk4fUILKmNK0o0FKVuHWLOfw5zGuZ4n2C941a1wnTbY/DCcLtC8kqQPkOz0dtbl83DrVM=","X-Received":"by 2002:a05:6000:18a4:: with SMTP id\n\tb4mr31196735wri.86.1620222902991; \n\tWed, 05 May 2021 06:55:02 -0700 (PDT)","MIME-Version":"1.0","References":"<20210505135308.1404-1-david.plowman@raspberrypi.com>\n\t<20210505135308.1404-2-david.plowman@raspberrypi.com>","In-Reply-To":"<20210505135308.1404-2-david.plowman@raspberrypi.com>","From":"David Plowman ","Date":"Wed, 5 May 2021 14:54:52 +0100","Message-ID":"","To":"libcamera devel ","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16771,"web_url":"https://patchwork.libcamera.org/comment/16771/","msgid":"","date":"2021-05-05T14:13:49","subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 05/05/2021 14:54, David Plowman wrote:\n> Hi\n> \n> On Wed, 5 May 2021 at 14:53, David Plowman\n> wrote:\n>>\n>> The minimum and maximum vblanking can change when a new format is\n>> applied to the sensor subdevice, so be sure to retrieve up-to-date\n>> values.\n>>\n>> The V4L2Device acquires the new updateControlInfo() method to perform\n>> this function, and which the CameraSensor calls automatically if its\n>> setFormat method is used to update the sensor.\n>>\n>> However, not all pipeline handlers invoke the setFormat method\n>> directly, so the new method must be made publicly available for\n>> pipeline handlers to call if they need to.\n>>\n>> Signed-off-by: David Plowman \n>> Reviewed-by: Jacopo Mondi \n>> Reviewed-by: Kieran Bingham \n> \n> Oops, typo here! Sorry Kieran. Can that be fixed up while applying please?\n\nThat depends, When will I receive my salary from RPi ;-)\n\n(Otherwise, yes I think so :D)\n--\nKieran\n\n> \n> Thanks\n> David\n> \n>> ---\n>> include/libcamera/internal/camera_sensor.h | 2 ++\n>> include/libcamera/internal/v4l2_device.h | 2 ++\n>> src/libcamera/camera_sensor.cpp | 32 ++++++++++++++++++-\n>> src/libcamera/v4l2_device.cpp | 37 ++++++++++++++++++++++\n>> 4 files changed, 72 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n>> index 3e98f71b..6ce513b7 100644\n>> --- a/include/libcamera/internal/camera_sensor.h\n>> +++ b/include/libcamera/internal/camera_sensor.h\n>> @@ -68,6 +68,8 @@ public:\n>> const ControlList &properties() const { return properties_; }\n>> int sensorInfo(CameraSensorInfo *info) const;\n>>\n>> + void updateControlInfo();\n>> +\n>> protected:\n>> std::string logPrefix() const override;\n>>\n>> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n>> index 087f07e7..5ba9b23b 100644\n>> --- a/include/libcamera/internal/v4l2_device.h\n>> +++ b/include/libcamera/internal/v4l2_device.h\n>> @@ -42,6 +42,8 @@ public:\n>> int setFrameStartEnabled(bool enable);\n>> Signal frameStart;\n>>\n>> + void updateControlInfo();\n>> +\n>> protected:\n>> V4L2Device(const std::string &deviceNode);\n>> ~V4L2Device();\n>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n>> index 2887bb69..322678c8 100644\n>> --- a/src/libcamera/camera_sensor.cpp\n>> +++ b/src/libcamera/camera_sensor.cpp\n>> @@ -656,15 +656,30 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n>> /**\n>> * \\brief Set the sensor output format\n>> * \\param[in] format The desired sensor output format\n>> + *\n>> + * The ranges of any controls associated with the sensor are also updated.\n>> + *\n>> * \\return 0 on success or a negative error code otherwise\n>> */\n>> int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n>> {\n>> - return subdev_->setFormat(pad_, format);\n>> + int ret = subdev_->setFormat(pad_, format);\n>> + if (ret)\n>> + return ret;\n>> +\n>> + updateControlInfo();\n>> + return 0;\n>> }\n>>\n>> /**\n>> * \\brief Retrieve the supported V4L2 controls and their information\n>> + *\n>> + * Control information is updated automatically to reflect the current sensor\n>> + * configuration when the setFormat() function is called, without invalidating\n>> + * any iterator on the ControlInfoMap. A manual update can also be forced by\n>> + * calling the updateControlInfo() function for pipeline handlers that change\n>> + * the sensor configuration wihtout using setFormat().\n>> + *\n>> * \\return A map of the V4L2 controls supported by the sensor\n>> */\n>> const ControlInfoMap &CameraSensor::controls() const\n>> @@ -750,6 +765,11 @@ int CameraSensor::setControls(ControlList *ctrls)\n>> * Sensor information is only available for raw sensors. When called for a YUV\n>> * sensor, this function returns -EINVAL.\n>> *\n>> + * Pipeline handlers that do not change the sensor format using the\n>> + * CameraSensor::setFormat method may need to call\n>> + * CameraSensor::updateControlInfo beforehand, to ensure all the control\n>> + * ranges are up to date.\n>> + *\n>> * \\return 0 on success, a negative error code otherwise\n>> */\n>> int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n>> @@ -821,6 +841,16 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n>> return 0;\n>> }\n>>\n>> +/**\n>> + * \\fn void CameraSensor::updateControlInfo()\n>> + * \\brief Update the sensor's ControlInfoMap in case they have changed\n>> + * \\sa V4L2Device::updateControlInfo()\n>> + */\n>> +void CameraSensor::updateControlInfo()\n>> +{\n>> + subdev_->updateControlInfo();\n>> +}\n>> +\n>> std::string CameraSensor::logPrefix() const\n>> {\n>> return \"'\" + entity_->name() + \"'\";\n>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>> index 397029ac..515cbdfe 100644\n>> --- a/src/libcamera/v4l2_device.cpp\n>> +++ b/src/libcamera/v4l2_device.cpp\n>> @@ -511,6 +511,43 @@ void V4L2Device::listControls()\n>> controls_ = std::move(ctrls);\n>> }\n>>\n>> +/**\n>> +* \\brief Update the information for all device controls\n>> + *\n>> + * The V4L2Device class caches information about all controls supported by the\n>> + * device and exposes it through the controls() and controlInfo() functions.\n>> + * Control information may change at runtime, for instance when formats on a\n>> + * subdev are modified. When this occurs, this function can be used to refresh\n>> + * control information. The information is refreshed in-place, all pointers to\n>> + * v4l2_query_ext_ctrl instances previously returned by controlInfo() and\n>> + * iterators to the ControlInfoMap returned by controls() remain valid.\n>> + *\n>> + * Note that control information isn't refreshed automatically is it may be an\n>> + * expensive operation. The V4L2Device users are responsible for calling this\n>> + * function when required, based on their usage pattern of the class.\n>> + */\n>> +void V4L2Device::updateControlInfo()\n>> +{\n>> + for (auto &[controlId, info] : controls_) {\n>> + unsigned int id = controlId->id();\n>> +\n>> + /*\n>> + * Assume controlInfo_ has a corresponding entry, as it has been\n>> + * generated by listControls().\n>> + */\n>> + struct v4l2_query_ext_ctrl &ctrl = controlInfo_[id];\n>> +\n>> + if (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {\n>> + LOG(V4L2, Debug)\n>> + << \"Could not refresh control \"\n>> + << utils::hex(id);\n>> + continue;\n>> + }\n>> +\n>> + info = V4L2ControlInfo(ctrl);\n>> + }\n>> +}\n>> +\n>> /*\n>> * \\brief Update the value of the first \\a count V4L2 controls in \\a ctrls using\n>> * values in \\a v4l2Ctrls\n>> --\n>> 2.20.1\n>>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 96650BDE7C\n\tfor ;\n\tWed, 5 May 2021 14:13:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1306268915;\n\tWed, 5 May 2021 16:13:53 +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 D8BE968901\n\tfor ;\n\tWed, 5 May 2021 16:13:51 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 52FA23F;\n\tWed, 5 May 2021 16:13:51 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VRRGLh+5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620224031;\n\tbh=Iw4NQlmDL4AjEgu2DPWI8/QLdr1YBXW+rKoGjm31EGg=;\n\th=Reply-To:To:References:From:Subject:Date:In-Reply-To:From;\n\tb=VRRGLh+5sYgm6DvEfw1z+V/Gv7WcA8MIZrVVEyaCD+cAu0d06tv4cTMn9rNgSlNPP\n\t4a03+f5l6x+4sD33JYzbvVMO6uIqPkQrzIQip3xJPkFvTWQcqFINbfbyhue4o6O6mm\n\tAg0lUW4AdZR9BGgquis863H6qnVGO1BPU0dVr5ls=","To":"David Plowman ,\n\tlibcamera devel ","References":"<20210505135308.1404-1-david.plowman@raspberrypi.com>\n\t<20210505135308.1404-2-david.plowman@raspberrypi.com>\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"","Date":"Wed, 5 May 2021 15:13:49 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16873,"web_url":"https://patchwork.libcamera.org/comment/16873/","msgid":"","date":"2021-05-10T23:27:49","subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Wed, May 05, 2021 at 02:53:07PM +0100, David Plowman wrote:\n> The minimum and maximum vblanking can change when a new format is\n> applied to the sensor subdevice, so be sure to retrieve up-to-date\n> values.\n> \n> The V4L2Device acquires the new updateControlInfo() method to perform\n> this function, and which the CameraSensor calls automatically if its\n> setFormat method is used to update the sensor.\n> \n> However, not all pipeline handlers invoke the setFormat method\n> directly, so the new method must be made publicly available for\n> pipeline handlers to call if they need to.\n> \n> Signed-off-by: David Plowman \n> Reviewed-by: Jacopo Mondi \n> Reviewed-by: Kieran Bingham \n> ---\n> include/libcamera/internal/camera_sensor.h | 2 ++\n> include/libcamera/internal/v4l2_device.h | 2 ++\n> src/libcamera/camera_sensor.cpp | 32 ++++++++++++++++++-\n> src/libcamera/v4l2_device.cpp | 37 ++++++++++++++++++++++\n> 4 files changed, 72 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 3e98f71b..6ce513b7 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -68,6 +68,8 @@ public:\n> \tconst ControlList &properties() const { return properties_; }\n> \tint sensorInfo(CameraSensorInfo *info) const;\n> \n> +\tvoid updateControlInfo();\n> +\n> protected:\n> \tstd::string logPrefix() const override;\n> \n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index 087f07e7..5ba9b23b 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -42,6 +42,8 @@ public:\n> \tint setFrameStartEnabled(bool enable);\n> \tSignal frameStart;\n> \n> +\tvoid updateControlInfo();\n> +\n> protected:\n> \tV4L2Device(const std::string &deviceNode);\n> \t~V4L2Device();\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 2887bb69..322678c8 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -656,15 +656,30 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n> /**\n> * \\brief Set the sensor output format\n> * \\param[in] format The desired sensor output format\n> + *\n> + * The ranges of any controls associated with the sensor are also updated.\n> + *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> {\n> -\treturn subdev_->setFormat(pad_, format);\n> +\tint ret = subdev_->setFormat(pad_, format);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tupdateControlInfo();\n> +\treturn 0;\n> }\n> \n> /**\n> * \\brief Retrieve the supported V4L2 controls and their information\n> + *\n> + * Control information is updated automatically to reflect the current sensor\n> + * configuration when the setFormat() function is called, without invalidating\n> + * any iterator on the ControlInfoMap. A manual update can also be forced by\n> + * calling the updateControlInfo() function for pipeline handlers that change\n> + * the sensor configuration wihtout using setFormat().\n> + *\n> * \\return A map of the V4L2 controls supported by the sensor\n> */\n> const ControlInfoMap &CameraSensor::controls() const\n> @@ -750,6 +765,11 @@ int CameraSensor::setControls(ControlList *ctrls)\n> * Sensor information is only available for raw sensors. When called for a YUV\n> * sensor, this function returns -EINVAL.\n> *\n> + * Pipeline handlers that do not change the sensor format using the\n> + * CameraSensor::setFormat method may need to call\n> + * CameraSensor::updateControlInfo beforehand, to ensure all the control\n\nYou can write setFormat() and updateControlInfo(), no need for a class\nname qualifier when the documentation refers to functions in the same\nclass. No need to resend, I'll update this when applying.\n\nReviewed-by: Laurent Pinchart \n\n> + * ranges are up to date.\n> + *\n> * \\return 0 on success, a negative error code otherwise\n> */\n> int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> @@ -821,6 +841,16 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\fn void CameraSensor::updateControlInfo()\n> + * \\brief Update the sensor's ControlInfoMap in case they have changed\n> + * \\sa V4L2Device::updateControlInfo()\n> + */\n> +void CameraSensor::updateControlInfo()\n> +{\n> +\tsubdev_->updateControlInfo();\n> +}\n> +\n> std::string CameraSensor::logPrefix() const\n> {\n> \treturn \"'\" + entity_->name() + \"'\";\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 397029ac..515cbdfe 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -511,6 +511,43 @@ void V4L2Device::listControls()\n> \tcontrols_ = std::move(ctrls);\n> }\n> \n> +/**\n> +* \\brief Update the information for all device controls\n> + *\n> + * The V4L2Device class caches information about all controls supported by the\n> + * device and exposes it through the controls() and controlInfo() functions.\n> + * Control information may change at runtime, for instance when formats on a\n> + * subdev are modified. When this occurs, this function can be used to refresh\n> + * control information. The information is refreshed in-place, all pointers to\n> + * v4l2_query_ext_ctrl instances previously returned by controlInfo() and\n> + * iterators to the ControlInfoMap returned by controls() remain valid.\n> + *\n> + * Note that control information isn't refreshed automatically is it may be an\n> + * expensive operation. The V4L2Device users are responsible for calling this\n> + * function when required, based on their usage pattern of the class.\n> + */\n> +void V4L2Device::updateControlInfo()\n> +{\n> +\tfor (auto &[controlId, info] : controls_) {\n> +\t\tunsigned int id = controlId->id();\n> +\n> +\t\t/*\n> +\t\t * Assume controlInfo_ has a corresponding entry, as it has been\n> +\t\t * generated by listControls().\n> +\t\t */\n> +\t\tstruct v4l2_query_ext_ctrl &ctrl = controlInfo_[id];\n> +\n> +\t\tif (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {\n> +\t\t\tLOG(V4L2, Debug)\n> +\t\t\t\t<< \"Could not refresh control \"\n> +\t\t\t\t<< utils::hex(id);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tinfo = V4L2ControlInfo(ctrl);\n> +\t}\n> +}\n> +\n> /*\n> * \\brief Update the value of the first \\a count V4L2 controls in \\a ctrls using\n> * values in \\a v4l2Ctrls","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 9D59CBF829\n\tfor ;\n\tMon, 10 May 2021 23:27:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F00CC68911;\n\tTue, 11 May 2021 01:27:57 +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 72090602B8\n\tfor ;\n\tTue, 11 May 2021 01:27:56 +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 EA09D6EF;\n\tTue, 11 May 2021 01:27:55 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Evqr6qri\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620689276;\n\tbh=eHqMusI6IJcGlXwhARIX7CEMe2ne298Aopwk/PR17Ko=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Evqr6qrihpBwY9ZeOaG4aWXzJ+oN9Od9tjZlJYU/Pfcr6vUsGBF1wD892QEUXXHJH\n\tZ25jwwIPNoJe/UTxUb1IImJVrkCAuww6LTyffJ+HgBO3m9kHXFmadScEFuVc1H4JnY\n\t3MhYOVsmI2SWpXO8RUGIoFBIkiiuNbU6cvU32C9k=","Date":"Tue, 11 May 2021 02:27:49 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20210505135308.1404-1-david.plowman@raspberrypi.com>\n\t<20210505135308.1404-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210505135308.1404-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","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":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]