[{"id":23968,"web_url":"https://patchwork.libcamera.org/comment/23968/","msgid":"<20220719154012.ti5p6bopyhmqj4nw@uno.localdomain>","date":"2022-07-19T15:40:12","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: request: Add support\n\tfor error flags","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran\n\nOn Tue, Jul 19, 2022 at 11:31:43AM +0100, Kieran Bingham 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 occured 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> ControlErrors occur 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> ---\n> include/libcamera/internal/request.h | 3 +++\n> include/libcamera/request.h | 9 +++++++\n> src/libcamera/request.cpp | 36 ++++++++++++++++++++++++++++\n> 3 files changed, 48 insertions(+)\n>\n> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> index 9dadd6c60361..8e592272cfae 100644\n> --- a/include/libcamera/internal/request.h\n> +++ b/include/libcamera/internal/request.h\n> @@ -38,6 +38,7 @@ public:\n> \tvoid complete();\n> \tvoid cancel();\n> \tvoid reuse();\n> +\tvoid setErrorFlags(ErrorFlags flags);\n>\n> \tvoid prepare(std::chrono::milliseconds timeout = 0ms);\n> \tSignal<> prepared;\n> @@ -59,6 +60,8 @@ private:\n> \tstd::unordered_set pending_;\n> \tstd::map> notifiers_;\n> \tstd::unique_ptr timer_;\n> +\n> +\tErrorFlags error_;\n> };\n>\n> } /* namespace libcamera */\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index dffde1536cad..992629e11aa4 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 ErrorFlags = 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> +\tErrorFlags error() const;\n>\n> \tbool hasPendingBuffers() const;\n>\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 07613cb33709..d585ef0b0ae4 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> +\terror_ = Request::NoError;\n> \tsequence_ = 0;\n> \tcancelled_ = false;\n> \tprepared_ = false;\n> @@ -284,6 +285,11 @@ void Request::Private::notifierActivated(FrameBuffer *buffer)\n> \temitPrepareCompleted();\n> }\n>\n> +void Request::Private::setErrorFlags(ErrorFlags flags)\n> +{\n> +\terror_ |= flags;\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 +324,22 @@ 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 non-fatal errors\n\nI'm afraid the documentation should likely be expanded a bit :)\n\nIn particular, what does it mean for application an error is non-fatal ?\nThat the request contains valid data ? That the request does not contain\nvalid data but the device is operating and new requests can be queued ?\n\n> + * \\var Request::NoError\n> + * No error\n\nDo we need a NoError ?\n\nIsn't testing with:\n\n if (request->error())\n\nenough ?\n\n> + * \\var Request::ControlError\n> + * Control Error. At least on control was not able to be applied to the device.\n\ns/on control/one control\n\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::ErrorFlags\n> + * The error state of the request.\n> + */\n> +\n> /**\n> * \\typedef Request::BufferMap\n> * \\brief A map of Stream to FrameBuffer pointers\n> @@ -560,6 +582,20 @@ uint32_t Request::sequence() const\n> * \\return The request completion status\n> */\n>\n> +/**\n> + * \\brief Retrieve the error flags\n> + *\n> + * The request could complete with non-fatal error. The completion status will\n> + * indicate success. This function returns the non-fatal errors that the\n> + * request completed with\n> + *\n\nHere as well I think a better definition of success and what fatal\nmeans is required. Linking to the flags enum documentation, when\nexpanded, might be enough\n\n> + * \\return Flags of non-fatal errors that the request completed with\n> + */\n> +Request::ErrorFlags Request::error() const\n> +{\n> +\treturn _d()->error_;\n> +}\n> +\n> /**\n> * \\brief Check if a request has buffers yet to be completed\n> *\n> --\n> 2.34.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 79FDEBE173\n\tfor ;\n\tTue, 19 Jul 2022 15:40:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BB15763312;\n\tTue, 19 Jul 2022 17:40:16 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 439CD603F4\n\tfor ;\n\tTue, 19 Jul 2022 17:40:15 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id B18202000D;\n\tTue, 19 Jul 2022 15:40:14 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658245216;\n\tbh=sWQe05ZLrB/oIpyouYFlE9++Q3aNPcYR7B8XV5sipKg=;\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=0SSCzGTkYFF8CxZNQ7ZZD2BES6mhw2mJrctBce2ZwRniOdrf2cjm+5E3wkcAcAY3w\n\t0be5njFCskhnESzjPCwRcr0KFtN4ckXwcxfmdUUxShhfYd00pSaeFBCTrJXhmbCiua\n\t5ii8CcNza9gcpuHsMg6A6VByM8pFL8/G0xwCglxYJoju/YBLp4m5vor2BZ9Rbz1NIT\n\ttOBPyYdZePBgYp/NIzI7Z5lZvbbCVOibgSzVF1UjpdjqLAw/IFpKpFSNUK4Ecu8jGz\n\teztzv874guzogwDCrnQJYwk9yPdDgCVVhlzBreFbDDNAMjikDpScOAmAwsE5lUSUvG\n\tFhi+rfAuroH7g==","Date":"Tue, 19 Jul 2022 17:40:12 +0200","To":"Kieran Bingham ","Message-ID":"<20220719154012.ti5p6bopyhmqj4nw@uno.localdomain>","References":"<20220719103144.3686313-1-kieran.bingham@ideasonboard.com>\n\t<20220719103144.3686313-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220719103144.3686313-2-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: request: Add support\n\tfor 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24005,"web_url":"https://patchwork.libcamera.org/comment/24005/","msgid":"<165832175119.2021905.12803463908294551855@Monstersaurus>","date":"2022-07-20T12:55:51","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: request: Add support\n\tfor error flags","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2022-07-19 16:40:12)\n> Hi Kieran\n> \n> On Tue, Jul 19, 2022 at 11:31:43AM +0100, Kieran Bingham 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 occured 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> > ControlErrors occur 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> > ---\n> > include/libcamera/internal/request.h | 3 +++\n> > include/libcamera/request.h | 9 +++++++\n> > src/libcamera/request.cpp | 36 ++++++++++++++++++++++++++++\n> > 3 files changed, 48 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > index 9dadd6c60361..8e592272cfae 100644\n> > --- a/include/libcamera/internal/request.h\n> > +++ b/include/libcamera/internal/request.h\n> > @@ -38,6 +38,7 @@ public:\n> > void complete();\n> > void cancel();\n> > void reuse();\n> > + void setErrorFlags(ErrorFlags flags);\n> >\n> > void prepare(std::chrono::milliseconds timeout = 0ms);\n> > Signal<> prepared;\n> > @@ -59,6 +60,8 @@ private:\n> > std::unordered_set pending_;\n> > std::map> notifiers_;\n> > std::unique_ptr timer_;\n> > +\n> > + ErrorFlags error_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index dffde1536cad..992629e11aa4 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> > ReuseBuffers = (1 << 0),\n> > };\n> >\n> > + enum ErrorFlag {\n> > + NoError = 0,\n> > + ControlError = (1 << 0),\n> > + };\n> > +\n> > + using ErrorFlags = Flags;\n> > +\n> > using BufferMap = std::map;\n> >\n> > Request(Camera *camera, uint64_t cookie = 0);\n> > @@ -60,6 +68,7 @@ public:\n> > uint32_t sequence() const;\n> > uint64_t cookie() const { return cookie_; }\n> > Status status() const { return status_; }\n> > + ErrorFlags error() const;\n> >\n> > bool hasPendingBuffers() const;\n> >\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index 07613cb33709..d585ef0b0ae4 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> > + error_ = Request::NoError;\n> > sequence_ = 0;\n> > cancelled_ = false;\n> > prepared_ = false;\n> > @@ -284,6 +285,11 @@ void Request::Private::notifierActivated(FrameBuffer *buffer)\n> > emitPrepareCompleted();\n> > }\n> >\n> > +void Request::Private::setErrorFlags(ErrorFlags flags)\n> > +{\n> > + error_ |= flags;\n> > +}\n> > +\n> > void Request::Private::timeout()\n> > {\n> > /* A timeout can only happen if there are fences not yet signalled. */\n> > @@ -318,6 +324,22 @@ 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 non-fatal errors\n> \n> I'm afraid the documentation should likely be expanded a bit :)\n\nWell this was supposed to be marked RFC ;-) so sure.\n \n> In particular, what does it mean for application an error is non-fatal ?\n> That the request contains valid data ? That the request does not contain\n> valid data but the device is operating and new requests can be queued ?\n> \n> > + * \\var Request::NoError\n> > + * No error\n> \n> Do we need a NoError ?\n\nYes, we need to be able to initialise to 'No Error'.\nSee Request::Private::reuse() above.\n\n(You can't set error_ = 0, with Flags<>);\n\n> \n> Isn't testing with:\n> \n> if (request->error())\n\nYes, that's also sufficient. These two are equivalent:\n\n\tif (request->error())\n\tif (request->error() != Request::NoError)\n\n\n\n> enough ?\n> \n> > + * \\var Request::ControlError\n> > + * Control Error. At least on control was not able to be applied to the device.\n> \n> s/on control/one control\n> \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::ErrorFlags\n> > + * The error state of the request.\n> > + */\n> > +\n> > /**\n> > * \\typedef Request::BufferMap\n> > * \\brief A map of Stream to FrameBuffer pointers\n> > @@ -560,6 +582,20 @@ uint32_t Request::sequence() const\n> > * \\return The request completion status\n> > */\n> >\n> > +/**\n> > + * \\brief Retrieve the error flags\n> > + *\n> > + * The request could complete with non-fatal error. The completion status will\n> > + * indicate success. This function returns the non-fatal errors that the\n> > + * request completed with\n> > + *\n> \n> Here as well I think a better definition of success and what fatal\n> means is required. Linking to the flags enum documentation, when\n> expanded, might be enough\n\nLinking to the detail of the enums is certainly a good addition.\n\n\n> \n> > + * \\return Flags of non-fatal errors that the request completed with\n> > + */\n> > +Request::ErrorFlags Request::error() const\n> > +{\n> > + return _d()->error_;\n> > +}\n> > +\n> > /**\n> > * \\brief Check if a request has buffers yet to be completed\n> > *\n> > --\n> > 2.34.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 D101DBE173\n\tfor ;\n\tWed, 20 Jul 2022 12:55:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 36BEE63310;\n\tWed, 20 Jul 2022 14:55:55 +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 0D08D601B8\n\tfor ;\n\tWed, 20 Jul 2022 14:55:54 +0200 (CEST)","from pendragon.ideasonboard.com\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 94DF16DB;\n\tWed, 20 Jul 2022 14:55:53 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658321755;\n\tbh=hJw6zfknvx6oAQmbQJvLktiheotalcvjYVPMCU7x81A=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=3QFFKpX3eoiZsB9JqhOqy1w/46fq5iBhgcJytRqLeWh1SQTatSVCuVwkaT77J2/WB\n\tHsrCvLjJ5/TkE+hk5PaL+jTQX1OSg9i3E08DC8KnGzKo72P40VVrs3GAht7aiLFFuB\n\t96YL2YWpftGn+HM5xg/QuE9Gidh+PQZiVwTKSFN8TGa2tUEKTCljOmRDgCcGIHK51s\n\tmtRfl+FXHPrbN9RI4bfruFvaxsiwaVnSODX878BThkx8q2mEEuesVlE8LkTZPG4ag6\n\tE7e2IF106cDwTI/xKzZARHwv9suoqIJuSD910/YTByE/BhRio/GPCimGvIAdFVVPD/\n\t/kzXwTW6TX+WA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658321753;\n\tbh=hJw6zfknvx6oAQmbQJvLktiheotalcvjYVPMCU7x81A=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=NHvQYBeVR1hL9FyXCaX0deYb4nHX5ZyZPGt4GdBBAGZ+PAMBRn/w3a2jlHjZdXh6M\n\tSltV6FGq/BHJZYV48FxMk+oopISmdsgl2yzz7oASOFHSRl8QCoDPjkw2tDuz2GFCZO\n\t5K4gmMPoU+VsxqewXPHMyk/0//3QpTKaNWOvt6Ew="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NHvQYBeV\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220719154012.ti5p6bopyhmqj4nw@uno.localdomain>","References":"<20220719103144.3686313-1-kieran.bingham@ideasonboard.com>\n\t<20220719103144.3686313-2-kieran.bingham@ideasonboard.com>\n\t<20220719154012.ti5p6bopyhmqj4nw@uno.localdomain>","To":"Jacopo Mondi ","Date":"Wed, 20 Jul 2022 13:55:51 +0100","Message-ID":"<165832175119.2021905.12803463908294551855@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: request: Add support\n\tfor 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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]