[{"id":15987,"web_url":"https://patchwork.libcamera.org/comment/15987/","msgid":"","date":"2021-03-29T04:52:51","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Request: Add an error\n\tvalue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Mon, Mar 29, 2021 at 09:27:14AM +0900, Hirokazu Honda wrote:\n> libcamera::Request returned by Camera::requestComplete() is\n> always assumed. Therefore, an error occurred in\n> PipelineHandler::queueRequest() is ignored.\n> This adds an error value, which is zero on success, to\n> libcamera::Request() so that a Camera client can know the request\n> error in completing request.\n\nAs the result is only valid when status() is Request::Complete, it\ncreates a risk for libcamera to set status() and result() to incoherent\nvalues, and for applications to badly interpret them. I think it would\nbe better to instead extend Request::Status with a RequestError. We may\nlater need to distinguish between different types of errors, but until\nwe reach that point, extending the status() will be simpler.\n\n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/request.h | 2 ++\n> src/libcamera/pipeline_handler.cpp | 6 ++++--\n> src/libcamera/request.cpp | 21 ++++++++++++++++-----\n> 3 files changed, 22 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 6e5aad5f..60e91d5f 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -52,6 +52,7 @@ public:\n> \n> \tuint64_t cookie() const { return cookie_; }\n> \tStatus status() const { return status_; }\n> +\tint result() const { return result_; }\n> \n> \tbool hasPendingBuffers() const { return !pending_.empty(); }\n> \n> @@ -73,6 +74,7 @@ private:\n> \n> \tconst uint64_t cookie_;\n> \tStatus status_;\n> +\tint result_;\n> \tbool cancelled_;\n> };\n> \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 05b807d6..a9a5523b 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -383,8 +383,10 @@ void PipelineHandler::queueRequest(Request *request)\n> \tdata->queuedRequests_.push_back(request);\n> \n> \tint ret = queueRequestDevice(camera, request);\n> -\tif (ret)\n> -\t\tdata->queuedRequests_.remove(request);\n> +\tif (ret) {\n> +\t\trequest->result_ = ret;\n> +\t\tcompleteRequest(request);\n> +\t}\n> }\n> \n> /**\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 0071667e..8106437e 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -72,7 +72,7 @@ LOG_DEFINE_CATEGORY(Request)\n> *\n> */\n> Request::Request(Camera *camera, uint64_t cookie)\n> -\t: camera_(camera), cookie_(cookie), status_(RequestPending),\n> +\t: camera_(camera), cookie_(cookie), status_(RequestPending), result_(0),\n> \t cancelled_(false)\n> {\n> \t/**\n> @@ -236,15 +236,26 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> * \\fn Request::status()\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> + * The request status indicates whether the request has pended, completed or\n> + * cancelled.. When requests are created and before they complete the request\n> + * status is set to RequestPending, and is updated at completion time to\n> + * RequestComplete. If a request is cancelled at capture stop before it has\n> * completed, its status is set to RequestCancelled.\n> *\n> * \\return The request completion status\n> */\n> \n> +/**\n> + * \\fn Request::result()\n> + * \\brief Retrieve the error value of the request .\n> + *\n> + * The request error indicates whether the request has completed successfully or\n> + * with an error. It is a negative value if an error happens in queueing and\n> + * processing the request, or 0 on success.\n> + *\n> + * \\return The request error value.\n> + */\n> +\n> /**\n> * \\fn Request::hasPendingBuffers()\n> * \\brief Check if a request has buffers yet to be completed","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 1C64FC32EA\n\tfor ;\n\tMon, 29 Mar 2021 04:53:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 53A046877D;\n\tMon, 29 Mar 2021 06:53:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9EC19602D2\n\tfor ;\n\tMon, 29 Mar 2021 06:53:35 +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 0DDD631A;\n\tMon, 29 Mar 2021 06:53:34 +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=\"tBz/3Jfu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616993615;\n\tbh=pdKwlRiCdc+PO04HthLMw7QTucskDA3yvym3e/HNgd4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tBz/3Jfu1p6lyT2ts0E3o1ranvWO6YTBIkV5uPAFktC2kdf4x21N45vh319HrzcmQ\n\t5yxCfifCnLEEXDwPt8EkyFVoJXXU4524JLcSbg1/Vcw33LNGZf1LV3bn8HRSXKI/I8\n\tWzJWw4yreBkCKsCkhSnwghm8bqP5HYvQilix2JNM=","Date":"Mon, 29 Mar 2021 07:52:51 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210329002715.74403-1-hiroh@chromium.org>\n\t<20210329002715.74403-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210329002715.74403-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Request: Add an error\n\tvalue","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":"libcamera-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\" "}},{"id":15988,"web_url":"https://patchwork.libcamera.org/comment/15988/","msgid":"","date":"2021-03-29T05:04:05","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Request: Add an error\n\tvalue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOne more comment.\n\nOn Mon, Mar 29, 2021 at 07:52:52AM +0300, Laurent Pinchart wrote:\n> On Mon, Mar 29, 2021 at 09:27:14AM +0900, Hirokazu Honda wrote:\n> > libcamera::Request returned by Camera::requestComplete() is\n> > always assumed. Therefore, an error occurred in\n> > PipelineHandler::queueRequest() is ignored.\n> > This adds an error value, which is zero on success, to\n> > libcamera::Request() so that a Camera client can know the request\n> > error in completing request.\n> \n> As the result is only valid when status() is Request::Complete, it\n> creates a risk for libcamera to set status() and result() to incoherent\n> values, and for applications to badly interpret them. I think it would\n> be better to instead extend Request::Status with a RequestError. We may\n> later need to distinguish between different types of errors, but until\n> we reach that point, extending the status() will be simpler.\n> \n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/request.h | 2 ++\n> > src/libcamera/pipeline_handler.cpp | 6 ++++--\n> > src/libcamera/request.cpp | 21 ++++++++++++++++-----\n> > 3 files changed, 22 insertions(+), 7 deletions(-)\n> > \n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index 6e5aad5f..60e91d5f 100644\n> > --- a/include/libcamera/request.h\n> > +++ b/include/libcamera/request.h\n> > @@ -52,6 +52,7 @@ public:\n> > \n> > \tuint64_t cookie() const { return cookie_; }\n> > \tStatus status() const { return status_; }\n> > +\tint result() const { return result_; }\n> > \n> > \tbool hasPendingBuffers() const { return !pending_.empty(); }\n> > \n> > @@ -73,6 +74,7 @@ private:\n> > \n> > \tconst uint64_t cookie_;\n> > \tStatus status_;\n> > +\tint result_;\n> > \tbool cancelled_;\n> > };\n> > \n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 05b807d6..a9a5523b 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -383,8 +383,10 @@ void PipelineHandler::queueRequest(Request *request)\n> > \tdata->queuedRequests_.push_back(request);\n> > \n> > \tint ret = queueRequestDevice(camera, request);\n> > -\tif (ret)\n> > -\t\tdata->queuedRequests_.remove(request);\n> > +\tif (ret) {\n> > +\t\trequest->result_ = ret;\n> > +\t\tcompleteRequest(request);\n\nShouldn't the buffers in the request be marked with FrameError ?\n\nInteractions between buffer states and request states may need a\nrevisit, and this includes the way we handle cancelling requests. This\nis a topic that has recently been brought up in the context of the \"IPU3\nDebug Improvements\" series, or a previous version of it (I can't locate\nthe exact message I'm thinking about at the moment). To avoid delaying\nthis fix, let's go for the simplest solution for now, which means\navoiding the new result() function as that has an impact through the\nwhole code base, and adding a new status value instead. We'll then\ndecide how to rework this.\n\n> > +\t}\n> > }\n> > \n> > /**\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index 0071667e..8106437e 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -72,7 +72,7 @@ LOG_DEFINE_CATEGORY(Request)\n> > *\n> > */\n> > Request::Request(Camera *camera, uint64_t cookie)\n> > -\t: camera_(camera), cookie_(cookie), status_(RequestPending),\n> > +\t: camera_(camera), cookie_(cookie), status_(RequestPending), result_(0),\n> > \t cancelled_(false)\n> > {\n> > \t/**\n> > @@ -236,15 +236,26 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> > * \\fn Request::status()\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> > + * The request status indicates whether the request has pended, completed or\n> > + * cancelled.. When requests are created and before they complete the request\n> > + * status is set to RequestPending, and is updated at completion time to\n> > + * RequestComplete. If a request is cancelled at capture stop before it has\n> > * completed, its status is set to RequestCancelled.\n> > *\n> > * \\return The request completion status\n> > */\n> > \n> > +/**\n> > + * \\fn Request::result()\n> > + * \\brief Retrieve the error value of the request .\n> > + *\n> > + * The request error indicates whether the request has completed successfully or\n> > + * with an error. It is a negative value if an error happens in queueing and\n> > + * processing the request, or 0 on success.\n> > + *\n> > + * \\return The request error value.\n> > + */\n> > +\n> > /**\n> > * \\fn Request::hasPendingBuffers()\n> > * \\brief Check if a request has buffers yet to be completed","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 9D33BC32EF\n\tfor ;\n\tMon, 29 Mar 2021 05:04:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F3C936877D;\n\tMon, 29 Mar 2021 07:04:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 45A7F602D2\n\tfor ;\n\tMon, 29 Mar 2021 07:04:49 +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 A6CAF31A;\n\tMon, 29 Mar 2021 07:04:48 +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=\"TnZBtfLS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616994288;\n\tbh=m5g6cBAXpszVrmISIo5y1Ca3+3h61R2QtpCPqSFoZIo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TnZBtfLSn/324SHA2WgiDb+2MRY7bLRwmjb/6PEPiALPvzdF0Xy6VMUVjd3qwXnJx\n\t9reCnk3TQ+1owFRhqwhpx2LbfS7gflwwbjLZYw0RRiZh9k607HAOLzLnPZq2qCXdVf\n\tETL8V0XmJH9sHVxteHIMTGXzPR6cpxBuPvab4Rxs=","Date":"Mon, 29 Mar 2021 08:04:05 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210329002715.74403-1-hiroh@chromium.org>\n\t<20210329002715.74403-3-hiroh@chromium.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Request: Add an error\n\tvalue","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":"libcamera-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\" "}},{"id":15996,"web_url":"https://patchwork.libcamera.org/comment/15996/","msgid":"","date":"2021-03-29T06:32:47","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Request: Add an error\n\tvalue","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thanks for reviewing,\n\n\nOn Mon, Mar 29, 2021 at 2:04 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> One more comment.\n>\n> On Mon, Mar 29, 2021 at 07:52:52AM +0300, Laurent Pinchart wrote:\n> > On Mon, Mar 29, 2021 at 09:27:14AM +0900, Hirokazu Honda wrote:\n> > > libcamera::Request returned by Camera::requestComplete() is\n> > > always assumed. Therefore, an error occurred in\n> > > PipelineHandler::queueRequest() is ignored.\n> > > This adds an error value, which is zero on success, to\n> > > libcamera::Request() so that a Camera client can know the request\n> > > error in completing request.\n> >\n> > As the result is only valid when status() is Request::Complete, it\n> > creates a risk for libcamera to set status() and result() to incoherent\n> > values, and for applications to badly interpret them. I think it would\n> > be better to instead extend Request::Status with a RequestError. We may\n> > later need to distinguish between different types of errors, but until\n> > we reach that point, extending the status() will be simpler.\n> >\n\nI got it. I am adding the new status enum value.\nstatus_ is set in complete(). Currently it is decided by cancelled_,\nso that the status_ is either Complete or Cancelled.\nDo you have any good idea how to solve this issue?\nI would add error_ (error code, int) and decide the status value by\ncancelled_ and error_.\n\nBest Regards,\n-Hiro\n\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > include/libcamera/request.h | 2 ++\n> > > src/libcamera/pipeline_handler.cpp | 6 ++++--\n> > > src/libcamera/request.cpp | 21 ++++++++++++++++-----\n> > > 3 files changed, 22 insertions(+), 7 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > index 6e5aad5f..60e91d5f 100644\n> > > --- a/include/libcamera/request.h\n> > > +++ b/include/libcamera/request.h\n> > > @@ -52,6 +52,7 @@ public:\n> > >\n> > > uint64_t cookie() const { return cookie_; }\n> > > Status status() const { return status_; }\n> > > + int result() const { return result_; }\n> > >\n> > > bool hasPendingBuffers() const { return !pending_.empty(); }\n> > >\n> > > @@ -73,6 +74,7 @@ private:\n> > >\n> > > const uint64_t cookie_;\n> > > Status status_;\n> > > + int result_;\n> > > bool cancelled_;\n> > > };\n> > >\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index 05b807d6..a9a5523b 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -383,8 +383,10 @@ void PipelineHandler::queueRequest(Request *request)\n> > > data->queuedRequests_.push_back(request);\n> > >\n> > > int ret = queueRequestDevice(camera, request);\n> > > - if (ret)\n> > > - data->queuedRequests_.remove(request);\n> > > + if (ret) {\n> > > + request->result_ = ret;\n> > > + completeRequest(request);\n>\n> Shouldn't the buffers in the request be marked with FrameError ?\n>\n> Interactions between buffer states and request states may need a\n> revisit, and this includes the way we handle cancelling requests. This\n> is a topic that has recently been brought up in the context of the \"IPU3\n> Debug Improvements\" series, or a previous version of it (I can't locate\n> the exact message I'm thinking about at the moment). To avoid delaying\n> this fix, let's go for the simplest solution for now, which means\n> avoiding the new result() function as that has an impact through the\n> whole code base, and adding a new status value instead. We'll then\n> decide how to rework this.\n>\n> > > + }\n> > > }\n> > >\n> > > /**\n> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > index 0071667e..8106437e 100644\n> > > --- a/src/libcamera/request.cpp\n> > > +++ b/src/libcamera/request.cpp\n> > > @@ -72,7 +72,7 @@ LOG_DEFINE_CATEGORY(Request)\n> > > *\n> > > */\n> > > Request::Request(Camera *camera, uint64_t cookie)\n> > > - : camera_(camera), cookie_(cookie), status_(RequestPending),\n> > > + : camera_(camera), cookie_(cookie), status_(RequestPending), result_(0),\n> > > cancelled_(false)\n> > > {\n> > > /**\n> > > @@ -236,15 +236,26 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> > > * \\fn Request::status()\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> > > + * The request status indicates whether the request has pended, completed or\n> > > + * cancelled.. When requests are created and before they complete the request\n> > > + * status is set to RequestPending, and is updated at completion time to\n> > > + * RequestComplete. If a request is cancelled at capture stop before it has\n> > > * completed, its status is set to RequestCancelled.\n> > > *\n> > > * \\return The request completion status\n> > > */\n> > >\n> > > +/**\n> > > + * \\fn Request::result()\n> > > + * \\brief Retrieve the error value of the request .\n> > > + *\n> > > + * The request error indicates whether the request has completed successfully or\n> > > + * with an error. It is a negative value if an error happens in queueing and\n> > > + * processing the request, or 0 on success.\n> > > + *\n> > > + * \\return The request error value.\n> > > + */\n> > > +\n> > > /**\n> > > * \\fn Request::hasPendingBuffers()\n> > > * \\brief Check if a request has buffers yet to be completed\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 AC3B5C32EA\n\tfor ;\n\tMon, 29 Mar 2021 06:33:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 163B2602D7;\n\tMon, 29 Mar 2021 08:33:01 +0200 (CEST)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 30C91602D2\n\tfor ;\n\tMon, 29 Mar 2021 08:32:59 +0200 (CEST)","by mail-ed1-x534.google.com with SMTP id y6so12964644eds.1\n\tfor ;\n\tSun, 28 Mar 2021 23:32:59 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"ZmKesO6j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Hp2f75Inp2v6cPuHCFls8JdrhcqeIXEis/gSeVDcbZI=;\n\tb=ZmKesO6jvgaZ7LmtkjBgzUAU5td4OaO/nuOUc+xrJxsQZWAasO7YPeMP7brg7+ztR9\n\tGmEnGbOgM0pfw0OUAGaF9KkxD5kSlYbzmBw8NyTB6X88h1sMRwi8Hgo4LfPxV69znj/6\n\tPNcSpxIQx/4c8D8UdRQh5mPBofy8XWEYQxLyg=","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:cc;\n\tbh=Hp2f75Inp2v6cPuHCFls8JdrhcqeIXEis/gSeVDcbZI=;\n\tb=YRjjy3gwa8yZcc9daUGdjnG/I+h3gWgmjFPqqSajVbVGj3x+YSILOH0UFNxcbnSP0C\n\tH5395mwVRHOHcEQTTaangqV8F828BbceUvkW/20SWaY/gy5alnhfGswXPngw2fu724bA\n\tuTpgyJ4fPk4LiQn140AfYhXptEPL5havZNzV5LtMmn3vkolCBIknbRih0ojLJkVWtxeM\n\tCqF275KrmSxg2iP9d2dOIBXDFCDKOmEVsIkyBxn96gRoz5SjjSd92w7KmjZF+AUwv8ot\n\tlyOseb0aaskPjdiMmBj+xJKwxqvrdo2TfwhQbbXcR8K4I6GgRiLC1CPi1obxsGveeeQG\n\tF1hg==","X-Gm-Message-State":"AOAM5321boB80B/KsWXYM14Gc912d4WVLRvDWpsegqTe8LBBW5ILiXwp\n\tcAAL84vli2YXW7yM1K56d4sggDqIIFYVST8TllRI3g==","X-Google-Smtp-Source":"ABdhPJzMGd5PjCCV7UpodQCIMzCsO9lKk/rkPdUCXatEhChw4mLofUBq8m/alkyqb2TbMoxmJnFc6r9pwcCAW/C7sFA=","X-Received":"by 2002:a05:6402:51cd:: with SMTP id\n\tr13mr27252182edd.116.1616999578737; \n\tSun, 28 Mar 2021 23:32:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20210329002715.74403-1-hiroh@chromium.org>\n\t<20210329002715.74403-3-hiroh@chromium.org>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 29 Mar 2021 15:32:47 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Request: Add an error\n\tvalue","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":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16001,"web_url":"https://patchwork.libcamera.org/comment/16001/","msgid":"","date":"2021-03-29T07:53:57","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Request: Add an error\n\tvalue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, Mar 29, 2021 at 03:32:47PM +0900, Hirokazu Honda wrote:\n> On Mon, Mar 29, 2021 at 2:04 PM Laurent Pinchart wrote:\n> > On Mon, Mar 29, 2021 at 07:52:52AM +0300, Laurent Pinchart wrote:\n> > > On Mon, Mar 29, 2021 at 09:27:14AM +0900, Hirokazu Honda wrote:\n> > > > libcamera::Request returned by Camera::requestComplete() is\n> > > > always assumed. Therefore, an error occurred in\n> > > > PipelineHandler::queueRequest() is ignored.\n> > > > This adds an error value, which is zero on success, to\n> > > > libcamera::Request() so that a Camera client can know the request\n> > > > error in completing request.\n> > >\n> > > As the result is only valid when status() is Request::Complete, it\n> > > creates a risk for libcamera to set status() and result() to incoherent\n> > > values, and for applications to badly interpret them. I think it would\n> > > be better to instead extend Request::Status with a RequestError. We may\n> > > later need to distinguish between different types of errors, but until\n> > > we reach that point, extending the status() will be simpler.\n> > >\n> \n> I got it. I am adding the new status enum value.\n> status_ is set in complete(). Currently it is decided by cancelled_,\n> so that the status_ is either Complete or Cancelled.\n> Do you have any good idea how to solve this issue?\n> I would add error_ (error code, int) and decide the status value by\n> cancelled_ and error_.\n\nLet's keep it simple for now as we know we'll have to rework this.\nAdding a private bool error_ member to Request should be fine.\n\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > include/libcamera/request.h | 2 ++\n> > > > src/libcamera/pipeline_handler.cpp | 6 ++++--\n> > > > src/libcamera/request.cpp | 21 ++++++++++++++++-----\n> > > > 3 files changed, 22 insertions(+), 7 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > > index 6e5aad5f..60e91d5f 100644\n> > > > --- a/include/libcamera/request.h\n> > > > +++ b/include/libcamera/request.h\n> > > > @@ -52,6 +52,7 @@ public:\n> > > >\n> > > > uint64_t cookie() const { return cookie_; }\n> > > > Status status() const { return status_; }\n> > > > + int result() const { return result_; }\n> > > >\n> > > > bool hasPendingBuffers() const { return !pending_.empty(); }\n> > > >\n> > > > @@ -73,6 +74,7 @@ private:\n> > > >\n> > > > const uint64_t cookie_;\n> > > > Status status_;\n> > > > + int result_;\n> > > > bool cancelled_;\n> > > > };\n> > > >\n> > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > index 05b807d6..a9a5523b 100644\n> > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > @@ -383,8 +383,10 @@ void PipelineHandler::queueRequest(Request *request)\n> > > > data->queuedRequests_.push_back(request);\n> > > >\n> > > > int ret = queueRequestDevice(camera, request);\n> > > > - if (ret)\n> > > > - data->queuedRequests_.remove(request);\n> > > > + if (ret) {\n> > > > + request->result_ = ret;\n> > > > + completeRequest(request);\n> >\n> > Shouldn't the buffers in the request be marked with FrameError ?\n> >\n> > Interactions between buffer states and request states may need a\n> > revisit, and this includes the way we handle cancelling requests. This\n> > is a topic that has recently been brought up in the context of the \"IPU3\n> > Debug Improvements\" series, or a previous version of it (I can't locate\n> > the exact message I'm thinking about at the moment). To avoid delaying\n> > this fix, let's go for the simplest solution for now, which means\n> > avoiding the new result() function as that has an impact through the\n> > whole code base, and adding a new status value instead. We'll then\n> > decide how to rework this.\n> >\n> > > > + }\n> > > > }\n> > > >\n> > > > /**\n> > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > index 0071667e..8106437e 100644\n> > > > --- a/src/libcamera/request.cpp\n> > > > +++ b/src/libcamera/request.cpp\n> > > > @@ -72,7 +72,7 @@ LOG_DEFINE_CATEGORY(Request)\n> > > > *\n> > > > */\n> > > > Request::Request(Camera *camera, uint64_t cookie)\n> > > > - : camera_(camera), cookie_(cookie), status_(RequestPending),\n> > > > + : camera_(camera), cookie_(cookie), status_(RequestPending), result_(0),\n> > > > cancelled_(false)\n> > > > {\n> > > > /**\n> > > > @@ -236,15 +236,26 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> > > > * \\fn Request::status()\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> > > > + * The request status indicates whether the request has pended, completed or\n> > > > + * cancelled.. When requests are created and before they complete the request\n> > > > + * status is set to RequestPending, and is updated at completion time to\n> > > > + * RequestComplete. If a request is cancelled at capture stop before it has\n> > > > * completed, its status is set to RequestCancelled.\n> > > > *\n> > > > * \\return The request completion status\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\fn Request::result()\n> > > > + * \\brief Retrieve the error value of the request .\n> > > > + *\n> > > > + * The request error indicates whether the request has completed successfully or\n> > > > + * with an error. It is a negative value if an error happens in queueing and\n> > > > + * processing the request, or 0 on success.\n> > > > + *\n> > > > + * \\return The request error value.\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\fn Request::hasPendingBuffers()\n> > > > * \\brief Check if a request has buffers yet to be completed","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 D1019C32EA\n\tfor ;\n\tMon, 29 Mar 2021 07:54:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D99D68783;\n\tMon, 29 Mar 2021 09:54:44 +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 1B8FD6877E\n\tfor ;\n\tMon, 29 Mar 2021 09:54:42 +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 81F25292;\n\tMon, 29 Mar 2021 09:54:41 +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=\"ebSv7T3C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617004481;\n\tbh=+2Ij+MIX8t0Kb9tCoTCwf2cAk3ffYcLF3iybeikg08Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ebSv7T3CBhxGkzAKRAyzU9sQHw60WrJR3U7/0ZJNYnMxrI5uAfoi+Tew3rK1znx2/\n\tOqaEiX8n+PWKG8Lmsm2ICa/Z46Eq/6Ngs3hrrwipmFrqSHCcFfw9t2wxcNeItixk6i\n\tG5KDqd6+kn7/2GaONFat5XGEGjpzYfAeyc6SQ5hw=","Date":"Mon, 29 Mar 2021 10:53:57 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210329002715.74403-1-hiroh@chromium.org>\n\t<20210329002715.74403-3-hiroh@chromium.org>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Request: Add an error\n\tvalue","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":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]