[{"id":24532,"web_url":"https://patchwork.libcamera.org/comment/24532/","msgid":"<4f6262a2-55dd-ddca-88e7-fc37b16036c7@ideasonboard.com>","date":"2022-08-11T04:54:19","subject":"Re: [libcamera-devel] [PATCH v2 01/10] libcamera: request: Add\n\tsupport for error flags","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn 8/5/22 19:23, Jacopo Mondi via libcamera-devel wrote:\n> From: Paul Elder \n>\n> Add error flags to the Request to indicate non-fatal errors.\n>\n> Applications should check the error() state of the Request to determine\n> if any fault occurred while processing the request.\n>\n> Initially, a single error flag ControlError is added to allow a pipeline\n> handler to report a failure to set controls on a hardware device while\n> processing the request.\n>\n> ControlError occurs when a Request was asked to set a control and the\n> pipeline handler attempted to do so, but it was rejected by the kernel.\n>\n> Signed-off-by: Paul Elder \n> Signed-off-by: Kieran Bingham \n> Signed-off-by: Jacopo Mondi \n\n\nReviewed-by: Umang Jain \n\n> ---\n> include/libcamera/internal/request.h | 4 ++\n> include/libcamera/request.h | 9 ++++\n> src/libcamera/request.cpp | 72 ++++++++++++++++++++++++++--\n> 3 files changed, 81 insertions(+), 4 deletions(-)\n>\n> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> index 9dadd6c60361..76cc32f73f9c 100644\n> --- a/include/libcamera/internal/request.h\n> +++ b/include/libcamera/internal/request.h\n> @@ -42,6 +42,8 @@ public:\n> \tvoid prepare(std::chrono::milliseconds timeout = 0ms);\n> \tSignal<> prepared;\n> \n> +\tvoid setError(Errors error);\n> +\n> private:\n> \tfriend class PipelineHandler;\n> \tfriend std::ostream &operator<<(std::ostream &out, const Request &r);\n> @@ -59,6 +61,8 @@ private:\n> \tstd::unordered_set pending_;\n> \tstd::map> notifiers_;\n> \tstd::unique_ptr timer_;\n> +\n> +\tErrors errors_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index dffde1536cad..3304da31200d 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -15,6 +15,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -43,6 +44,13 @@ public:\n> \t\tReuseBuffers = (1 << 0),\n> \t};\n> \n> +\tenum ErrorFlag {\n> +\t\tNoError = 0,\n> +\t\tControlError = (1 << 0),\n> +\t};\n> +\n> +\tusing Errors = Flags;\n> +\n> \tusing BufferMap = std::map;\n> \n> \tRequest(Camera *camera, uint64_t cookie = 0);\n> @@ -60,6 +68,7 @@ public:\n> \tuint32_t sequence() const;\n> \tuint64_t cookie() const { return cookie_; }\n> \tStatus status() const { return status_; }\n> +\tErrors error() const;\n> \n> \tbool hasPendingBuffers() const;\n> \n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index d2af1d2212ad..9a808f19fc03 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -162,6 +162,7 @@ void Request::Private::cancel()\n> */\n> void Request::Private::reuse()\n> {\n> +\terrors_ = Request::NoError;\n> \tsequence_ = 0;\n> \tcancelled_ = false;\n> \tprepared_ = false;\n> @@ -284,6 +285,21 @@ void Request::Private::notifierActivated(FrameBuffer *buffer)\n> \temitPrepareCompleted();\n> }\n> \n> +/**\n> + * \\brief Update the error flags of the Request\n> + * \\param[in] error Error flags to apply on the Request\n> + *\n> + * Flag \\a error in the Request which will get reported to the application when\n> + * the Request completes.\n> + *\n> + * Setting an error flag does not cause a Request to fail, and once set it can\n> + * only be cleared by the application destroying the Request or calling reuse().\n> + */\n> +void Request::Private::setError(Errors error)\n> +{\n> +\terrors_ |= error;\n> +}\n> +\n> void Request::Private::timeout()\n> {\n> \t/* A timeout can only happen if there are fences not yet signalled. */\n> @@ -318,6 +334,25 @@ void Request::Private::timeout()\n> * Reuse the buffers that were previously added by addBuffer()\n> */\n> \n> +/**\n> + * \\enum Request::ErrorFlag\n> + * Flags to report errors on a completed request\n> + *\n> + * \\var Request::NoError\n> + * No error. The Request completed succesfully and all its buffer contain\n> + * valid data\n> + *\n> + * \\var Request::ControlError\n> + * Control Error. At least one control was not applied correctly to the camera.\n> + * The application should compare the metadata to the requested control values\n> + * to check which controls weren't applied.\n> + */\n> +\n> +/**\n> + * \\typedef Request::Errors\n> + * The error state of the request defined by \\a Request::ErrorFlag\n> + */\n> +\n> /**\n> * \\typedef Request::BufferMap\n> * \\brief A map of Stream to FrameBuffer pointers\n> @@ -552,14 +587,43 @@ uint32_t Request::sequence() const\n> * \\brief Retrieve the request completion status\n> *\n> * The request status indicates whether the request has completed successfully\n> - * or with an error. When requests are created and before they complete the\n> - * request status is set to RequestPending, and is updated at completion time\n> - * to RequestComplete. If a request is cancelled at capture stop before it has\n> - * completed, its status is set to RequestCancelled.\n> + * or has been cancelled before being processed.\n> + *\n> + * Requests are created with their status set to RequestPending. When\n> + * a Request is successfully processed and completed by the Camera its\n> + * status is set to RequestComplete. If a Request is cancelled before\n> + * being processed, for example because the Camera has been stopped\n> + * before the request is completed, its status is set to RequestCancelled.\n> + *\n> + * Successfully completed requests can complete with errors. Applications shall\n> + * inspect the error mask returned by Request::error() before accessing buffers\n> + * and data associated with a completed request.\n> *\n> * \\return The request completion status\n> */\n> \n> +/**\n> + * \\brief Retrieve the mask of error flags associated with a completed request\n> + *\n> + * The request could complete with errors, which indicate failures in\n> + * completing correctly parts of the request submitted by the application.\n> + *\n> + * The possible failure reasons are defined by the error flags defined\n> + * by Request::ErrorFlag and application are expected to retrieve the\n> + * mask of error flags by using this function before accessing the\n> + * buffers and data associated with a completed request.\n> + *\n> + * Error conditions reported through this function do not change the\n> + * request completion status retrieved through Request::status() which\n> + * indicates if the Request has been processed or not.\n> + *\n> + * \\return A mask of error identifier with which the request was completed\n> + */\n> +Request::Errors Request::error() const\n> +{\n> +\treturn _d()->errors_;\n> +}\n> +\n> /**\n> * \\brief Check if a request has buffers yet to be completed\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 6A06FC3272\n\tfor ;\n\tThu, 11 Aug 2022 04:54:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 769396332B;\n\tThu, 11 Aug 2022 06:54:27 +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 92BAC61FA8\n\tfor ;\n\tThu, 11 Aug 2022 06:54:25 +0200 (CEST)","from [IPV6:2401:4900:1f3f:c7a1:27b3:9637:38a7:6084] (unknown\n\t[IPv6:2401:4900:1f3f:c7a1:27b3:9637:38a7:6084])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 73B363F1;\n\tThu, 11 Aug 2022 06:54:24 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660193667;\n\tbh=bvXJL1/iAjw31yaqx83Wwmarh9flihzlJIxOCfVqzIM=;\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:\n\tFrom;\n\tb=23FjlxComVbwQGzquHqXrM5p2NUZrvDS7eOhMe43Nti0czzETx0gMwvcNid1vEF3X\n\tDO9eNvtKYpbIh+TlGoBj27ofyE7TTQtWuCkAXSH+J9zFzbDoLrzfeDgjQTUU5OJAII\n\t1fJboOzuVDpQZkIRkcZzES+k8VF8BVrSypdcBjXkELyUB4MKSfsPQeXskzmVGMJRb3\n\tj/uJYDd8SDtvGxznYm9JKR0YwfTpIXdIWtky/rbVQBhObnNiWbjvApE1pmc0+yepgo\n\tdNV/zwHfh/SXZLzBSa0HgUIdWO9TCS0c2KlPXDiMVVEAkdO1xQ80Cg6kqyzJRurVIW\n\tq4DZ8kTHDbYEw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660193665;\n\tbh=bvXJL1/iAjw31yaqx83Wwmarh9flihzlJIxOCfVqzIM=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=eDyaSjr4xeeJgptJlp7UShHbKzxh769kN16HVxfhrapAVgvgCiyrvM/8TpPVaxjV0\n\tk7Tyv5OjXLcpl6WrklG8+Kk1q/IppO0yda+ULHFr6NeOJ5rgSOMSioAllOVQZxcJ3Y\n\tdNlq0DnwAuG6L7SLOvHBXU12Lu21mzo8rk9agIxQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"eDyaSjr4\"; dkim-atps=neutral","Message-ID":"<4f6262a2-55dd-ddca-88e7-fc37b16036c7@ideasonboard.com>","Date":"Thu, 11 Aug 2022 10:24:19 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20220805135312.47497-1-jacopo@jmondi.org>\n\t<20220805135312.47497-2-jacopo@jmondi.org>","In-Reply-To":"<20220805135312.47497-2-jacopo@jmondi.org>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 01/10] libcamera: request: Add\n\tsupport for error flags","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24594,"web_url":"https://patchwork.libcamera.org/comment/24594/","msgid":"","date":"2022-08-16T03:19:43","subject":"Re: [libcamera-devel] [PATCH v2 01/10] libcamera: request: Add\n\tsupport for error flags","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo and Paul,\n\nThank you for the patch.\n\nOn Fri, Aug 05, 2022 at 03:53:03PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> From: Paul Elder \n> \n> Add error flags to the Request to indicate non-fatal errors.\n\nWhat differentiates fatal and non-fatal errors ? Asked differently, if\nthese are non-fatal errors, what are the fatal errors ? I don't think we\ndefine that concept anywhere.\n\n> Applications should check the error() state of the Request to determine\n> if any fault occurred while processing the request.\n\nI think this needs to be documented in appropriate places in\nDocumentation/, especially in the guides, with an update to simple-cam.\nThe cam and qcam applications, as well as the adaptation layers, also\nneed to be updated.\n\n> Initially, a single error flag ControlError is added to allow a pipeline\n> handler to report a failure to set controls on a hardware device while\n> processing the request.\n> \n> ControlError occurs when a Request was asked to set a control and the\n> pipeline handler attempted to do so, but it was rejected by the kernel.\n\nThis doesn't really match the definition in the documentation below.\n\n> Signed-off-by: Paul Elder \n> Signed-off-by: Kieran Bingham \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/request.h | 4 ++\n> include/libcamera/request.h | 9 ++++\n> src/libcamera/request.cpp | 72 ++++++++++++++++++++++++++--\n> 3 files changed, 81 insertions(+), 4 deletions(-)\n> \n> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> index 9dadd6c60361..76cc32f73f9c 100644\n> --- a/include/libcamera/internal/request.h\n> +++ b/include/libcamera/internal/request.h\n> @@ -42,6 +42,8 @@ public:\n> \tvoid prepare(std::chrono::milliseconds timeout = 0ms);\n> \tSignal<> prepared;\n> \n> +\tvoid setError(Errors error);\n> +\n> private:\n> \tfriend class PipelineHandler;\n> \tfriend std::ostream &operator<<(std::ostream &out, const Request &r);\n> @@ -59,6 +61,8 @@ private:\n> \tstd::unordered_set pending_;\n> \tstd::map> notifiers_;\n> \tstd::unique_ptr timer_;\n> +\n> +\tErrors errors_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index dffde1536cad..3304da31200d 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -15,6 +15,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -43,6 +44,13 @@ public:\n> \t\tReuseBuffers = (1 << 0),\n> \t};\n> \n> +\tenum ErrorFlag {\n> +\t\tNoError = 0,\n> +\t\tControlError = (1 << 0),\n> +\t};\n> +\n> +\tusing Errors = Flags;\n\nI'd s/Errors/ErrorFlags/ to be more explicit (and to match MapFlags from\nthe File and MappedFrameBuffer classes).\n\n> +\n> \tusing BufferMap = std::map;\n> \n> \tRequest(Camera *camera, uint64_t cookie = 0);\n> @@ -60,6 +68,7 @@ public:\n> \tuint32_t sequence() const;\n> \tuint64_t cookie() const { return cookie_; }\n> \tStatus status() const { return status_; }\n> +\tErrors error() const;\n\nThe function name is confusing, singular implies a single error. There's\na similar issue with the error parameter to setError. Renaming the\nfunctions to errorFlags and setErrorFlags could help, there may be\nbetter names.\n\n> \n> \tbool hasPendingBuffers() const;\n> \n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index d2af1d2212ad..9a808f19fc03 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -162,6 +162,7 @@ void Request::Private::cancel()\n> */\n> void Request::Private::reuse()\n> {\n> +\terrors_ = Request::NoError;\n> \tsequence_ = 0;\n> \tcancelled_ = false;\n> \tprepared_ = false;\n> @@ -284,6 +285,21 @@ void Request::Private::notifierActivated(FrameBuffer *buffer)\n> \temitPrepareCompleted();\n> }\n> \n> +/**\n> + * \\brief Update the error flags of the Request\n> + * \\param[in] error Error flags to apply on the Request\n> + *\n> + * Flag \\a error in the Request which will get reported to the application when\n> + * the Request completes.\n\nSame as above, this should be adjusted depending on whether the\nsetError() function is meant to set a single error (it should take an\nErrorFlag then) or possibly multiple errors. Thinking ahead about\nasynchronous error reporting, as well as typical usage patterns for this\nfunction (would a pipeline handler have to set multiple error flags at\nonce ?) could help decide.\n\n> + *\n> + * Setting an error flag does not cause a Request to fail, and once set it can\n> + * only be cleared by the application destroying the Request or calling reuse().\n> + */\n> +void Request::Private::setError(Errors error)\n> +{\n> +\terrors_ |= error;\n> +}\n> +\n> void Request::Private::timeout()\n> {\n> \t/* A timeout can only happen if there are fences not yet signalled. */\n> @@ -318,6 +334,25 @@ void Request::Private::timeout()\n> * Reuse the buffers that were previously added by addBuffer()\n> */\n> \n> +/**\n> + * \\enum Request::ErrorFlag\n> + * Flags to report errors on a completed request\n> + *\n> + * \\var Request::NoError\n> + * No error. The Request completed succesfully and all its buffer contain\n> + * valid data\n> + *\n> + * \\var Request::ControlError\n> + * Control Error. At least one control was not applied correctly to the camera.\n> + * The application should compare the metadata to the requested control values\n> + * to check which controls weren't applied.\n> + */\n> +\n> +/**\n> + * \\typedef Request::Errors\n> + * The error state of the request defined by \\a Request::ErrorFlag\n> + */\n> +\n> /**\n> * \\typedef Request::BufferMap\n> * \\brief A map of Stream to FrameBuffer pointers\n> @@ -552,14 +587,43 @@ uint32_t Request::sequence() const\n> * \\brief Retrieve the request completion status\n> *\n> * The request status indicates whether the request has completed successfully\n> - * or with an error. When requests are created and before they complete the\n> - * request status is set to RequestPending, and is updated at completion time\n> - * to RequestComplete. If a request is cancelled at capture stop before it has\n> - * completed, its status is set to RequestCancelled.\n> + * or has been cancelled before being processed.\n> + *\n> + * Requests are created with their status set to RequestPending. When\n> + * a Request is successfully processed and completed by the Camera its\n> + * status is set to RequestComplete. If a Request is cancelled before\n> + * being processed, for example because the Camera has been stopped\n> + * before the request is completed, its status is set to RequestCancelled.\n\nI wonder if request cancellation should be reported as an error flag. We\nmay then possibly drop the request status. No need to do so now, but\nopinions will be welcome.\n\n> + *\n> + * Successfully completed requests can complete with errors. Applications shall\n> + * inspect the error mask returned by Request::error() before accessing buffers\n> + * and data associated with a completed request.\n> *\n> * \\return The request completion status\n> */\n> \n> +/**\n> + * \\brief Retrieve the mask of error flags associated with a completed request\n> + *\n> + * The request could complete with errors, which indicate failures in\n> + * completing correctly parts of the request submitted by the application.\n> + *\n> + * The possible failure reasons are defined by the error flags defined\n> + * by Request::ErrorFlag and application are expected to retrieve the\n> + * mask of error flags by using this function before accessing the\n> + * buffers and data associated with a completed request.\n> + *\n> + * Error conditions reported through this function do not change the\n> + * request completion status retrieved through Request::status() which\n> + * indicates if the Request has been processed or not.\n> + *\n> + * \\return A mask of error identifier with which the request was completed\n> + */\n> +Request::Errors Request::error() const\n> +{\n> +\treturn _d()->errors_;\n> +}\n\nOne point that bothers me a bit is that I don't have a good view on what\nother type of errors could be reported later through this mechanism. If\nwe end up with a single error flag for the request, then usage of the\nFlags<> class will be overkill.\n\n> +\n> /**\n> * \\brief Check if a request has buffers yet to be completed\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 A0454C3272\n\tfor ;\n\tTue, 16 Aug 2022 03:19:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E1D7061FC0;\n\tTue, 16 Aug 2022 05:19:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3435A603E3\n\tfor ;\n\tTue, 16 Aug 2022 05:19:57 +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 9B824496;\n\tTue, 16 Aug 2022 05:19:56 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660619998;\n\tbh=bRSO2oRgvPqgKqDtPu2iItna3GDA70u6PQLXagWabks=;\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=i87hVjWmPemTtEVInZcvln6kDApeoy6iS2Lg3Pa+ym0Gz2EuOHmIcw3nt/ntym1KT\n\tz2YAeV83WbLWwxngG1y5JxufUDfGZP3c9VOtzrLORwi0M0ylIEoyWdzZWYw9BDsP3R\n\t5m+31BHK3TxDm+ySMbvEkJt/HCDyhd6JRfUGc1lXi5FPJiViF1MWUU9Q8XMy44cWCa\n\tbJ65NwzjAiuEVHK2a8KW/sQiafeTqSSj3vGuTD0SJpUoVJBAzdMSpRZX6gCX8vKHry\n\tie/fa2NPlkFvk8bMU+ezLfRGiO9QVYRU7nyW/7fZIDn4A4P0JvKYMwUklQB66pmBmt\n\tykJViZEmwpIiA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660619996;\n\tbh=bRSO2oRgvPqgKqDtPu2iItna3GDA70u6PQLXagWabks=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bh7QlMP0L+A8+xNNWvpDJtSLdXTKQ3hBs2eV+TLqLn4ahTiED2R+ee6GrPYPJXZpX\n\tgAadhHHZCwEf1HnMk/F/5NeezrkttFUUGeKIQeVp8iKrd0k4qxATOBB8u55N6l22TS\n\tl6fDCb8CJhtd74ottHfjN7wSG23KkfRQ1an1B/PI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"bh7QlMP0\"; dkim-atps=neutral","Date":"Tue, 16 Aug 2022 06:19:43 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220805135312.47497-1-jacopo@jmondi.org>\n\t<20220805135312.47497-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220805135312.47497-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 01/10] libcamera: request: Add\n\tsupport for error flags","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24618,"web_url":"https://patchwork.libcamera.org/comment/24618/","msgid":"<20220817085217.2pz3tmxvsliyhwl5@uno.localdomain>","date":"2022-08-17T08:52:17","subject":"Re: [libcamera-devel] [PATCH v2 01/10] libcamera: request: Add\n\tsupport for error flags","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Tue, Aug 16, 2022 at 06:19:43AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo and Paul,\n>\n> Thank you for the patch.\n>\n> On Fri, Aug 05, 2022 at 03:53:03PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > From: Paul Elder \n> >\n> > Add error flags to the Request to indicate non-fatal errors.\n>\n> What differentiates fatal and non-fatal errors ? Asked differently, if\n> these are non-fatal errors, what are the fatal errors ? I don't think we\n> define that concept anywhere.\n>\n\nI pushed to remove the terms \"fatal\" vs \"non-fatal\" in the PFC\ndesign and in this series, and this is a left-over.\n\nFatal vs non-fatal comes from the requirement of defining what errors\ncause a Request to be cancelled vs errors that are set on a Request\nthat completes correctly. As we have no errors that cause a Request to\nbe cancelled I agree the distinction creates more confusion to a\nreader that is not aware of the background. I'll remove it.\n\n\n> > Applications should check the error() state of the Request to determine\n> > if any fault occurred while processing the request.\n>\n> I think this needs to be documented in appropriate places in\n> Documentation/, especially in the guides, with an update to simple-cam.\n> The cam and qcam applications, as well as the adaptation layers, also\n> need to be updated.\n>\n\nThat's a good point.\n\nLooking at your comments, it seems you prefer to merge from 04 on and\nbreak-out error flags, as they clearly need more attention.\n\n> > Initially, a single error flag ControlError is added to allow a pipeline\n> > handler to report a failure to set controls on a hardware device while\n> > processing the request.\n> >\n> > ControlError occurs when a Request was asked to set a control and the\n> > pipeline handler attempted to do so, but it was rejected by the kernel.\n>\n> This doesn't really match the definition in the documentation below.\n\nThis is the error flag documentation\n\n + * \\var Request::ControlError\n + * Control Error. At least one control was not applied correctly to the camera.\n + * The application should compare the metadata to the requested control values\n + * to check which controls weren't applied.\n\nIs it the \"by the kernel\" part that bothers you ?\n\n>\n> > Signed-off-by: Paul Elder \n> > Signed-off-by: Kieran Bingham \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/request.h | 4 ++\n> > include/libcamera/request.h | 9 ++++\n> > src/libcamera/request.cpp | 72 ++++++++++++++++++++++++++--\n> > 3 files changed, 81 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > index 9dadd6c60361..76cc32f73f9c 100644\n> > --- a/include/libcamera/internal/request.h\n> > +++ b/include/libcamera/internal/request.h\n> > @@ -42,6 +42,8 @@ public:\n> > \tvoid prepare(std::chrono::milliseconds timeout = 0ms);\n> > \tSignal<> prepared;\n> >\n> > +\tvoid setError(Errors error);\n> > +\n> > private:\n> > \tfriend class PipelineHandler;\n> > \tfriend std::ostream &operator<<(std::ostream &out, const Request &r);\n> > @@ -59,6 +61,8 @@ private:\n> > \tstd::unordered_set pending_;\n> > \tstd::map> notifiers_;\n> > \tstd::unique_ptr timer_;\n> > +\n> > +\tErrors errors_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index dffde1536cad..3304da31200d 100644\n> > --- a/include/libcamera/request.h\n> > +++ b/include/libcamera/request.h\n> > @@ -15,6 +15,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -43,6 +44,13 @@ public:\n> > \t\tReuseBuffers = (1 << 0),\n> > \t};\n> >\n> > +\tenum ErrorFlag {\n> > +\t\tNoError = 0,\n> > +\t\tControlError = (1 << 0),\n> > +\t};\n> > +\n> > +\tusing Errors = Flags;\n>\n> I'd s/Errors/ErrorFlags/ to be more explicit (and to match MapFlags from\n> the File and MappedFrameBuffer classes).\n>\n\nThat's what Kieran had and I shortened it.\n\nI can revert it back, but it really seems a duplication and what is\nrelevant for applications is knowing that this is a mask of errors,\nthe fact it is implemented with Flags<> is secondary.\n\nAnyway, no need to bikeshed. As Kieran had it in this way and you feel\nthe same (I don't recall about Umang's opinion here) I'll change it\nback.\n\n> > +\n> > \tusing BufferMap = std::map;\n> >\n> > \tRequest(Camera *camera, uint64_t cookie = 0);\n> > @@ -60,6 +68,7 @@ public:\n> > \tuint32_t sequence() const;\n> > \tuint64_t cookie() const { return cookie_; }\n> > \tStatus status() const { return status_; }\n> > +\tErrors error() const;\n>\n> The function name is confusing, singular implies a single error. There's\n\nerrors() ?\n\n> a similar issue with the error parameter to setError. Renaming the\n\nsetError(ErrorFlags errors) ?\n\n> functions to errorFlags and setErrorFlags could help, there may be\n> better names.\n>\n> >\n> > \tbool hasPendingBuffers() const;\n> >\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index d2af1d2212ad..9a808f19fc03 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -162,6 +162,7 @@ void Request::Private::cancel()\n> > */\n> > void Request::Private::reuse()\n> > {\n> > +\terrors_ = Request::NoError;\n> > \tsequence_ = 0;\n> > \tcancelled_ = false;\n> > \tprepared_ = false;\n> > @@ -284,6 +285,21 @@ void Request::Private::notifierActivated(FrameBuffer *buffer)\n> > \temitPrepareCompleted();\n> > }\n> >\n> > +/**\n> > + * \\brief Update the error flags of the Request\n> > + * \\param[in] error Error flags to apply on the Request\n> > + *\n> > + * Flag \\a error in the Request which will get reported to the application when\n> > + * the Request completes.\n>\n> Same as above, this should be adjusted depending on whether the\n> NetError() function is meant to set a single error (it should take an\n> ErrorFlag then) or possibly multiple errors. Thinking ahead about\n> asynchronous error reporting, as well as typical usage patterns for this\n> function (would a pipeline handler have to set multiple error flags at\n> once ?) could help decide.\n>\n\nI do feel like, in the context of async error signalling, that\nsetting/signalling single errors might be more opportune. Can we start\nwith that ?\n\n> > + *\n> > + * Setting an error flag does not cause a Request to fail, and once set it can\n> > + * only be cleared by the application destroying the Request or calling reuse().\n> > + */\n> > +void Request::Private::setError(Errors error)\n> > +{\n> > +\terrors_ |= error;\n> > +}\n> > +\n> > void Request::Private::timeout()\n> > {\n> > \t/* A timeout can only happen if there are fences not yet signalled. */\n> > @@ -318,6 +334,25 @@ void Request::Private::timeout()\n> > * Reuse the buffers that were previously added by addBuffer()\n> > */\n> >\n> > +/**\n> > + * \\enum Request::ErrorFlag\n> > + * Flags to report errors on a completed request\n> > + *\n> > + * \\var Request::NoError\n> > + * No error. The Request completed succesfully and all its buffer contain\n> > + * valid data\n> > + *\n> > + * \\var Request::ControlError\n> > + * Control Error. At least one control was not applied correctly to the camera.\n> > + * The application should compare the metadata to the requested control values\n> > + * to check which controls weren't applied.\n> > + */\n> > +\n> > +/**\n> > + * \\typedef Request::Errors\n> > + * The error state of the request defined by \\a Request::ErrorFlag\n> > + */\n> > +\n> > /**\n> > * \\typedef Request::BufferMap\n> > * \\brief A map of Stream to FrameBuffer pointers\n> > @@ -552,14 +587,43 @@ uint32_t Request::sequence() const\n> > * \\brief Retrieve the request completion status\n> > *\n> > * The request status indicates whether the request has completed successfully\n> > - * or with an error. When requests are created and before they complete the\n> > - * request status is set to RequestPending, and is updated at completion time\n> > - * to RequestComplete. If a request is cancelled at capture stop before it has\n> > - * completed, its status is set to RequestCancelled.\n> > + * or has been cancelled before being processed.\n> > + *\n> > + * Requests are created with their status set to RequestPending. When\n> > + * a Request is successfully processed and completed by the Camera its\n> > + * status is set to RequestComplete. If a Request is cancelled before\n> > + * being processed, for example because the Camera has been stopped\n> > + * before the request is completed, its status is set to RequestCancelled.\n>\n> I wonder if request cancellation should be reported as an error flag. We\n> may then possibly drop the request status. No need to do so now, but\n> opinions will be welcome.\n>\n\nWe should really think if we want to maintain the semantic currently\nassociated with StatusCancelled. Right now, it only serves to report\nto application that the camera has been stopped and no requests can be\nqueued anymore. It doesn't feel like an async error would fit there.\nWe're back to discuss what's fatal vs what's not fatal, also\nconsidering what future error flags we might have to add.\n\nCurrently I'm fine with \"non fatal\" errors being set through flags and\nrequest cancellation being signalled by the status. It nicely matches\nthe idea that \"Completed\" requests have been processed and might\ncomplete with errors, \"Cancelled\" requests have been returned before\nbeing processed because the camera device has stopped.\n\nAsking the same question again: do we foresee errors that might cause\nthe device to stop and make it impossible to process all future requests ?\nCurrently we have none, and we thought it over a bit and so far all\nthe use cases for error flags are \"non-fatal\" ones.\n\n\n> > + *\n> > + * Successfully completed requests can complete with errors. Applications shall\n> > + * inspect the error mask returned by Request::error() before accessing buffers\n> > + * and data associated with a completed request.\n> > *\n> > * \\return The request completion status\n> > */\n> >\n> > +/**\n> > + * \\brief Retrieve the mask of error flags associated with a completed request\n> > + *\n> > + * The request could complete with errors, which indicate failures in\n> > + * completing correctly parts of the request submitted by the application.\n> > + *\n> > + * The possible failure reasons are defined by the error flags defined\n> > + * by Request::ErrorFlag and application are expected to retrieve the\n> > + * mask of error flags by using this function before accessing the\n> > + * buffers and data associated with a completed request.\n> > + *\n> > + * Error conditions reported through this function do not change the\n> > + * request completion status retrieved through Request::status() which\n> > + * indicates if the Request has been processed or not.\n> > + *\n> > + * \\return A mask of error identifier with which the request was completed\n> > + */\n> > +Request::Errors Request::error() const\n> > +{\n> > +\treturn _d()->errors_;\n> > +}\n>\n> One point that bothers me a bit is that I don't have a good view on what\n> other type of errors could be reported later through this mechanism. If\n> we end up with a single error flag for the request, then usage of the\n> Flags<> class will be overkill.\n>\n\nWe'll have more, and we tried collecting them in the PFC design\ndocument.\n\n> > +\n> > /**\n> > * \\brief Check if a request has buffers yet to be completed\n> > *\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1A7A5C3272\n\tfor ;\n\tWed, 17 Aug 2022 08:52:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 64FDE61FC0;\n\tWed, 17 Aug 2022 10:52:22 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6BD3B61FA6\n\tfor ;\n\tWed, 17 Aug 2022 10:52:20 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id AF739200002;\n\tWed, 17 Aug 2022 08:52:19 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660726342;\n\tbh=7VExIVm0tprISg54gFor06qKpl62QvnEG5GxOht2v3k=;\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=1i7wuZtquOGzfDDulSmfYMtCJnteWXyw7HgguzpsgTjX5umRNk0TEfoyX8PeDzkxP\n\tS88SOxUzYslBdBYcvT0rV0eiJT1Zndg9THKb+L1OUC3OcbzJyi3vl4FdV7O4kq2unC\n\tLaOT+2wKsmWQhdl4ssn1IAQXsWMM2+QR3KdwnabUt5Sz2giWfqEz1MW6I8F3Yicrog\n\t7TsRfLWQRgjIPhfoYwQyer+lCIecprrBR0tesHLZWS+xoCPWkHBW1FPJwtnWhBE46A\n\tJ0je7hgO60VIQTc7J60KTt7KQtm0lORrqlXoCUOb17CS8erOBajaT/OOMRPcr4Js0n\n\tKK3lDJ6F/VR7A==","Date":"Wed, 17 Aug 2022 10:52:17 +0200","To":"Laurent Pinchart ","Message-ID":"<20220817085217.2pz3tmxvsliyhwl5@uno.localdomain>","References":"<20220805135312.47497-1-jacopo@jmondi.org>\n\t<20220805135312.47497-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 01/10] libcamera: request: Add\n\tsupport for error flags","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\" "}}]