[{"id":16012,"web_url":"https://patchwork.libcamera.org/comment/16012/","msgid":"<20210329094039.s7e2qv3brcr7btym@uno.localdomain>","date":"2021-03-29T09:40:39","subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: Request: Add\n\tRequestError to Status","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Mon, Mar 29, 2021 at 06:15:51PM +0900, Hirokazu Honda wrote:\n> This adds a new libcamera::Request::Status, RequestError, which\n> represents a request isn't successfully processed due to some\n> error.\n>\n\nIt seems to me that RequestCancelled is now only set if the frame\nmetadata are cancelled by the IPA and is signaled by the pipeline\nhandlers by calling completeBuffer() and have Request detect such an\nerror condition.\n\nbool Request::completeBuffer(FrameBuffer *buffer)\n{\n\tLIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);\n\n\tint ret = pending_.erase(buffer);\n\tASSERT(ret == 1);\n\n\tbuffer->request_ = nullptr;\n\n\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n\t\tcancelled_ = true;\n\n\treturn !hasPendingBuffers();\n}\n\nThe cancelled request state is then propagate to application at\nrequest completion\n\nvoid Request::complete()\n{\n\tASSERT(status_ == RequestPending);\n\tASSERT(!hasPendingBuffers());\n\n\tstatus_ = cancelled_ ? RequestCancelled : RequestComplete;\n\n\tLOG(Request, Debug)\n\t\t<< \"Request has completed - cookie: \" << cookie_\n\t\t<< (cancelled_ ? \" [Cancelled]\" : \"\");\n\n\tLIBCAMERA_TRACEPOINT(request_complete, this);\n}\n\nWouldn't it be better to keep using RequestComplete and add an error_\nfield like you're doing here to be inspected by Request::complete()\nthat could transport different errors ? In example\n\n enum RequestError {\n MetadataCancelled,\n BufferUnderrun,\n ...\n };\n\nAnd change completeBuffers() to use the new error field as well as\nPipelineHandler::queueRequest() and change Request::complete() to\ninspect the new field so that you can drop cancelled_ ?\n\nIn this way a failed Request will always have status ==\nRequestCancelled (which can be changed to RequestFailed if we like it\nmore) and the exact failure reason can be deduced inspecting\nRequest::error() ?\n\n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/request.h | 2 ++\n> src/libcamera/pipeline_handler.cpp | 5 ++++-\n> src/libcamera/request.cpp | 17 +++++++++++------\n> 3 files changed, 17 insertions(+), 7 deletions(-)\n>\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 6e5aad5f..598fcf86 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -30,6 +30,7 @@ public:\n> \t\tRequestPending,\n> \t\tRequestComplete,\n> \t\tRequestCancelled,\n> +\t\tRequestError,\n> \t};\n>\n> \tenum ReuseFlag {\n> @@ -73,6 +74,7 @@ private:\n>\n> \tconst uint64_t cookie_;\n> \tStatus status_;\n> +\tbool error_;\n> \tbool cancelled_;\n> };\n>\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index eba00ed3..66326ce0 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -381,8 +381,11 @@ void PipelineHandler::queueRequest(Request *request)\n> \tdata->queuedRequests_.push_back(request);\n>\n> \tint ret = queueRequestDevice(camera, request);\n> -\tif (ret)\n> +\tif (ret) {\n> +\t\trequest->error_ = true;\n> \t\tdata->queuedRequests_.remove(request);\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..176f59dd 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -36,6 +36,8 @@ LOG_DEFINE_CATEGORY(Request)\n> * The request has completed\n> * \\var Request::RequestCancelled\n> * The request has been cancelled due to capture stop\n> + * \\var Request::RequestError\n> + * The request is not completed successfully due to an error.\n> */\n>\n> /**\n> @@ -73,7 +75,7 @@ LOG_DEFINE_CATEGORY(Request)\n> */\n> Request::Request(Camera *camera, uint64_t cookie)\n> \t: camera_(camera), cookie_(cookie), status_(RequestPending),\n> -\t cancelled_(false)\n> +\t error_(false), cancelled_(false)\n> {\n> \t/**\n> \t * \\todo Should the Camera expose a validator instance, to avoid\n> @@ -126,6 +128,7 @@ void Request::reuse(ReuseFlag flags)\n> \t}\n>\n> \tstatus_ = RequestPending;\n> +\terror_ = false;\n> \tcancelled_ = false;\n>\n> \tcontrols_->clear();\n> @@ -256,20 +259,22 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> /**\n> * \\brief Complete a queued request\n> *\n> - * Mark the request as complete by updating its status to RequestComplete,\n> - * unless buffers have been cancelled in which case the status is set to\n> - * RequestCancelled.\n> + * Mark the request as complete by updating its status to RequestError on error,\n> + * RequestCancelled if buffers have been cancelled, or otherwise RequestComplete.\n> */\n> void Request::complete()\n> {\n> \tASSERT(status_ == RequestPending);\n> \tASSERT(!hasPendingBuffers());\n>\n> -\tstatus_ = cancelled_ ? RequestCancelled : RequestComplete;\n> +\tif (error_)\n> +\t\tstatus_ = RequestError;\n> +\telse\n> +\t\tstatus_ = cancelled_ ? RequestCancelled : RequestComplete;\n>\n> \tLOG(Request, Debug)\n> \t\t<< \"Request has completed - cookie: \" << cookie_\n> -\t\t<< (cancelled_ ? \" [Cancelled]\" : \"\");\n> +\t\t<< (error_ ? \" [Error]\" : (cancelled_ ? \" [Cancelled]\" : \"\"));\n>\n> \tLIBCAMERA_TRACEPOINT(request_complete, this);\n> }\n> --\n> 2.31.0.291.g576ba9dcdaf-goog\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 3E735C32EA\n\tfor ;\n\tMon, 29 Mar 2021 09:40:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9E6EA68787;\n\tMon, 29 Mar 2021 11:40:06 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 20DEE6877E\n\tfor ;\n\tMon, 29 Mar 2021 11:40:05 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 643AB100004;\n\tMon, 29 Mar 2021 09:40:03 +0000 (UTC)"],"Date":"Mon, 29 Mar 2021 11:40:39 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210329094039.s7e2qv3brcr7btym@uno.localdomain>","References":"<20210329091552.157208-1-hiroh@chromium.org>\n\t<20210329091552.157208-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210329091552.157208-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: Request: Add\n\tRequestError to Status","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":16053,"web_url":"https://patchwork.libcamera.org/comment/16053/","msgid":"","date":"2021-03-30T06:08:46","subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: Request: Add\n\tRequestError to Status","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thanks for reviewing and comments.\n\n\nOn Mon, Mar 29, 2021 at 6:40 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Mar 29, 2021 at 06:15:51PM +0900, Hirokazu Honda wrote:\n> > This adds a new libcamera::Request::Status, RequestError, which\n> > represents a request isn't successfully processed due to some\n> > error.\n> >\n>\n> It seems to me that RequestCancelled is now only set if the frame\n> metadata are cancelled by the IPA and is signaled by the pipeline\n> handlers by calling completeBuffer() and have Request detect such an\n> error condition.\n>\n> bool Request::completeBuffer(FrameBuffer *buffer)\n> {\n> LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);\n>\n> int ret = pending_.erase(buffer);\n> ASSERT(ret == 1);\n>\n> buffer->request_ = nullptr;\n>\n> if (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> cancelled_ = true;\n>\n> return !hasPendingBuffers();\n> }\n>\n> The cancelled request state is then propagate to application at\n> request completion\n>\n> void Request::complete()\n> {\n> ASSERT(status_ == RequestPending);\n> ASSERT(!hasPendingBuffers());\n>\n> status_ = cancelled_ ? RequestCancelled : RequestComplete;\n>\n> LOG(Request, Debug)\n> << \"Request has completed - cookie: \" << cookie_\n> << (cancelled_ ? \" [Cancelled]\" : \"\");\n>\n> LIBCAMERA_TRACEPOINT(request_complete, this);\n> }\n>\n> Wouldn't it be better to keep using RequestComplete and add an error_\n> field like you're doing here to be inspected by Request::complete()\n> that could transport different errors ? In example\n>\n> enum RequestError {\n> MetadataCancelled,\n> BufferUnderrun,\n> ...\n> };\n>\n> And change completeBuffers() to use the new error field as well as\n> PipelineHandler::queueRequest() and change Request::complete() to\n> inspect the new field so that you can drop cancelled_ ?\n>\n> In this way a failed Request will always have status ==\n> RequestCancelled (which can be changed to RequestFailed if we like it\n> more) and the exact failure reason can be deduced inspecting\n> Request::error() ?\n>\n\n+Laurent Pinchart\nI and Laurent have discussed in the patch version 1.\nThe concern was the coherency of status and error values.\nAdding a new status enum, RequestError, to Request::Status is the\nsimplest solution.\nI think we should add error() function to acquire integer error code\nwhen status()=RequestError rather than declaring a new enum\nRequestError.\nHow do you think?\n\nRegards,\n-Hiro\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/request.h | 2 ++\n> > src/libcamera/pipeline_handler.cpp | 5 ++++-\n> > src/libcamera/request.cpp | 17 +++++++++++------\n> > 3 files changed, 17 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index 6e5aad5f..598fcf86 100644\n> > --- a/include/libcamera/request.h\n> > +++ b/include/libcamera/request.h\n> > @@ -30,6 +30,7 @@ public:\n> > RequestPending,\n> > RequestComplete,\n> > RequestCancelled,\n> > + RequestError,\n> > };\n> >\n> > enum ReuseFlag {\n> > @@ -73,6 +74,7 @@ private:\n> >\n> > const uint64_t cookie_;\n> > Status status_;\n> > + bool error_;\n> > bool cancelled_;\n> > };\n> >\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index eba00ed3..66326ce0 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -381,8 +381,11 @@ void PipelineHandler::queueRequest(Request *request)\n> > data->queuedRequests_.push_back(request);\n> >\n> > int ret = queueRequestDevice(camera, request);\n> > - if (ret)\n> > + if (ret) {\n> > + request->error_ = true;\n> > data->queuedRequests_.remove(request);\n> > + completeRequest(request);\n> > + }\n> > }\n> >\n> > /**\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index 0071667e..176f59dd 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -36,6 +36,8 @@ LOG_DEFINE_CATEGORY(Request)\n> > * The request has completed\n> > * \\var Request::RequestCancelled\n> > * The request has been cancelled due to capture stop\n> > + * \\var Request::RequestError\n> > + * The request is not completed successfully due to an error.\n> > */\n> >\n> > /**\n> > @@ -73,7 +75,7 @@ LOG_DEFINE_CATEGORY(Request)\n> > */\n> > Request::Request(Camera *camera, uint64_t cookie)\n> > : camera_(camera), cookie_(cookie), status_(RequestPending),\n> > - cancelled_(false)\n> > + error_(false), cancelled_(false)\n> > {\n> > /**\n> > * \\todo Should the Camera expose a validator instance, to avoid\n> > @@ -126,6 +128,7 @@ void Request::reuse(ReuseFlag flags)\n> > }\n> >\n> > status_ = RequestPending;\n> > + error_ = false;\n> > cancelled_ = false;\n> >\n> > controls_->clear();\n> > @@ -256,20 +259,22 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> > /**\n> > * \\brief Complete a queued request\n> > *\n> > - * Mark the request as complete by updating its status to RequestComplete,\n> > - * unless buffers have been cancelled in which case the status is set to\n> > - * RequestCancelled.\n> > + * Mark the request as complete by updating its status to RequestError on error,\n> > + * RequestCancelled if buffers have been cancelled, or otherwise RequestComplete.\n> > */\n> > void Request::complete()\n> > {\n> > ASSERT(status_ == RequestPending);\n> > ASSERT(!hasPendingBuffers());\n> >\n> > - status_ = cancelled_ ? RequestCancelled : RequestComplete;\n> > + if (error_)\n> > + status_ = RequestError;\n> > + else\n> > + status_ = cancelled_ ? RequestCancelled : RequestComplete;\n> >\n> > LOG(Request, Debug)\n> > << \"Request has completed - cookie: \" << cookie_\n> > - << (cancelled_ ? \" [Cancelled]\" : \"\");\n> > + << (error_ ? \" [Error]\" : (cancelled_ ? \" [Cancelled]\" : \"\"));\n> >\n> > LIBCAMERA_TRACEPOINT(request_complete, this);\n> > }\n> > --\n> > 2.31.0.291.g576ba9dcdaf-goog\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 C6867C32F0\n\tfor ;\n\tTue, 30 Mar 2021 06:09:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 302C068782;\n\tTue, 30 Mar 2021 08:09:00 +0200 (CEST)","from mail-ej1-x634.google.com (mail-ej1-x634.google.com\n\t[IPv6:2a00:1450:4864:20::634])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3D53C602D7\n\tfor ;\n\tTue, 30 Mar 2021 08:08:58 +0200 (CEST)","by mail-ej1-x634.google.com with SMTP id ap14so9853552ejc.0\n\tfor ;\n\tMon, 29 Mar 2021 23:08:58 -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=\"XizXVPli\"; 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=lntPofI4Ba4Zdv7z6VMnHkYXT4hf60NTA8zU7vP5Bzs=;\n\tb=XizXVPliGERXpVd9XaFlxgBgY8GdkMrZFN9T/+xMFOqhUME6PRrD7mHyGRTZlcJqxm\n\tk1exsWIpUdYGSXW42Z8+3kCHr5SS3VDpyhthGUKnQQNk54Xd9eyd7+SihvM6wqY1Pk/p\n\tp7QHBI0s0PK+/sV6TzmGsGokQxH5La2OkgDZM=","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=lntPofI4Ba4Zdv7z6VMnHkYXT4hf60NTA8zU7vP5Bzs=;\n\tb=sVKyF72ozY/SZgSnefjY0HVDr6YwoLxeR8qMjKuhxtbq8V266QaP35BaHcGGmerw9G\n\tWHe2DolpwZnUl/VwxOJ09FX7Zmom9n46LmJu2bCOzECPmrrZh8oS6q2sUTYd9iBA8w1a\n\tPw7FyWPxqQH7FcVKX5VFI+Zuqs/Gk/4INM07CuZv5tBkmemGhqAEigdQCa79eV9AmUsI\n\tYiQ9nqnMseYOd1V+fYXOhyYdEBMz5MxwwWgRghpGpLg4IU44CjUpm5bjiYU6TStyr//0\n\teolKHWMvec9Z28fCiS3h+Xe8yyKlmW9zkjgqnuAfikIqgtIyb6TPqYgMUkxublC8L7af\n\tvjKA==","X-Gm-Message-State":"AOAM531iwv66Qp6MicVCrrs9oA4YqT5kwVfwZolKyJpqz07ZftQUraQ7\n\tQ6CZ76nAa00nUzUu98vJbRbf1ZxdHiSgdfdfcomkow==","X-Google-Smtp-Source":"ABdhPJwT2InEpOIXOORcUiVS7U+9r2/Lgv/sVK06XYHW3hnW/++nEiXIDgEzOtmrEqJrqop8PgKmX0ROpLjWYV4n3Fo=","X-Received":"by 2002:a17:907:1b06:: with SMTP id\n\tmp6mr21420978ejc.292.1617084537803; \n\tMon, 29 Mar 2021 23:08:57 -0700 (PDT)","MIME-Version":"1.0","References":"<20210329091552.157208-1-hiroh@chromium.org>\n\t<20210329091552.157208-3-hiroh@chromium.org>\n\t<20210329094039.s7e2qv3brcr7btym@uno.localdomain>","In-Reply-To":"<20210329094039.s7e2qv3brcr7btym@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 30 Mar 2021 15:08:46 +0900","Message-ID":"","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: Request: Add\n\tRequestError to Status","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":16102,"web_url":"https://patchwork.libcamera.org/comment/16102/","msgid":"","date":"2021-04-03T01:35:09","subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: Request: Add\n\tRequestError to Status","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 Tue, Mar 30, 2021 at 03:08:46PM +0900, Hirokazu Honda wrote:\n> On Mon, Mar 29, 2021 at 6:40 PM Jacopo Mondi wrote:\n> > On Mon, Mar 29, 2021 at 06:15:51PM +0900, Hirokazu Honda wrote:\n> > > This adds a new libcamera::Request::Status, RequestError, which\n> > > represents a request isn't successfully processed due to some\n> > > error.\n> >\n> > It seems to me that RequestCancelled is now only set if the frame\n> > metadata are cancelled by the IPA and is signaled by the pipeline\n> > handlers by calling completeBuffer() and have Request detect such an\n> > error condition.\n> >\n> > bool Request::completeBuffer(FrameBuffer *buffer)\n> > {\n> > LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);\n> >\n> > int ret = pending_.erase(buffer);\n> > ASSERT(ret == 1);\n> >\n> > buffer->request_ = nullptr;\n> >\n> > if (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> > cancelled_ = true;\n> >\n> > return !hasPendingBuffers();\n> > }\n> >\n> > The cancelled request state is then propagate to application at\n> > request completion\n> >\n> > void Request::complete()\n> > {\n> > ASSERT(status_ == RequestPending);\n> > ASSERT(!hasPendingBuffers());\n> >\n> > status_ = cancelled_ ? RequestCancelled : RequestComplete;\n> >\n> > LOG(Request, Debug)\n> > << \"Request has completed - cookie: \" << cookie_\n> > << (cancelled_ ? \" [Cancelled]\" : \"\");\n> >\n> > LIBCAMERA_TRACEPOINT(request_complete, this);\n> > }\n> >\n> > Wouldn't it be better to keep using RequestComplete and add an error_\n> > field like you're doing here to be inspected by Request::complete()\n> > that could transport different errors ? In example\n> >\n> > enum RequestError {\n> > MetadataCancelled,\n> > BufferUnderrun,\n> > ...\n> > };\n> >\n> > And change completeBuffers() to use the new error field as well as\n> > PipelineHandler::queueRequest() and change Request::complete() to\n> > inspect the new field so that you can drop cancelled_ ?\n> >\n> > In this way a failed Request will always have status ==\n> > RequestCancelled (which can be changed to RequestFailed if we like it\n> > more) and the exact failure reason can be deduced inspecting\n> > Request::error() ?\n> \n> +Laurent Pinchart\n> I and Laurent have discussed in the patch version 1.\n> The concern was the coherency of status and error values.\n> Adding a new status enum, RequestError, to Request::Status is the\n> simplest solution.\n> I think we should add error() function to acquire integer error code\n> when status()=RequestError rather than declaring a new enum\n> RequestError.\n> How do you think?\n\nI've reviewed 3/3 before 2/3 and have written a lengthy reply there, so\nlet's discuss it there :-)\n\nOn the above topic though, I think this series, with the introduction of\nRequestError and without a separate error code, is the least intrusive\nway to fix the issue at hand. It is, however, a work in progress, as we\nneed to reconsider the design of error reporting (let's discuss this in\n3/3). As that will take a bit of time, the question is whether we need\nto fast-track a quick fix for this issue and rework the API on top, or\nif we can wait for the full fix with the API redesign. I'd prefer the\nlatter to avoid introducing temporary could, but if there are reasons\nwhy a short-term quick fix is useful, I'm not opposed to it.\n\nLet's discuss in 3/3 and then come back to this question.\n\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > include/libcamera/request.h | 2 ++\n> > > src/libcamera/pipeline_handler.cpp | 5 ++++-\n> > > src/libcamera/request.cpp | 17 +++++++++++------\n> > > 3 files changed, 17 insertions(+), 7 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > index 6e5aad5f..598fcf86 100644\n> > > --- a/include/libcamera/request.h\n> > > +++ b/include/libcamera/request.h\n> > > @@ -30,6 +30,7 @@ public:\n> > > RequestPending,\n> > > RequestComplete,\n> > > RequestCancelled,\n> > > + RequestError,\n> > > };\n> > >\n> > > enum ReuseFlag {\n> > > @@ -73,6 +74,7 @@ private:\n> > >\n> > > const uint64_t cookie_;\n> > > Status status_;\n> > > + bool error_;\n> > > bool cancelled_;\n> > > };\n> > >\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index eba00ed3..66326ce0 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -381,8 +381,11 @@ void PipelineHandler::queueRequest(Request *request)\n> > > data->queuedRequests_.push_back(request);\n> > >\n> > > int ret = queueRequestDevice(camera, request);\n> > > - if (ret)\n> > > + if (ret) {\n> > > + request->error_ = true;\n> > > data->queuedRequests_.remove(request);\n> > > + completeRequest(request);\n> > > + }\n> > > }\n> > >\n> > > /**\n> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > index 0071667e..176f59dd 100644\n> > > --- a/src/libcamera/request.cpp\n> > > +++ b/src/libcamera/request.cpp\n> > > @@ -36,6 +36,8 @@ LOG_DEFINE_CATEGORY(Request)\n> > > * The request has completed\n> > > * \\var Request::RequestCancelled\n> > > * The request has been cancelled due to capture stop\n> > > + * \\var Request::RequestError\n> > > + * The request is not completed successfully due to an error.\n> > > */\n> > >\n> > > /**\n> > > @@ -73,7 +75,7 @@ LOG_DEFINE_CATEGORY(Request)\n> > > */\n> > > Request::Request(Camera *camera, uint64_t cookie)\n> > > : camera_(camera), cookie_(cookie), status_(RequestPending),\n> > > - cancelled_(false)\n> > > + error_(false), cancelled_(false)\n> > > {\n> > > /**\n> > > * \\todo Should the Camera expose a validator instance, to avoid\n> > > @@ -126,6 +128,7 @@ void Request::reuse(ReuseFlag flags)\n> > > }\n> > >\n> > > status_ = RequestPending;\n> > > + error_ = false;\n> > > cancelled_ = false;\n> > >\n> > > controls_->clear();\n> > > @@ -256,20 +259,22 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> > > /**\n> > > * \\brief Complete a queued request\n> > > *\n> > > - * Mark the request as complete by updating its status to RequestComplete,\n> > > - * unless buffers have been cancelled in which case the status is set to\n> > > - * RequestCancelled.\n> > > + * Mark the request as complete by updating its status to RequestError on error,\n> > > + * RequestCancelled if buffers have been cancelled, or otherwise RequestComplete.\n> > > */\n> > > void Request::complete()\n> > > {\n> > > ASSERT(status_ == RequestPending);\n> > > ASSERT(!hasPendingBuffers());\n> > >\n> > > - status_ = cancelled_ ? RequestCancelled : RequestComplete;\n> > > + if (error_)\n> > > + status_ = RequestError;\n> > > + else\n> > > + status_ = cancelled_ ? RequestCancelled : RequestComplete;\n> > >\n> > > LOG(Request, Debug)\n> > > << \"Request has completed - cookie: \" << cookie_\n> > > - << (cancelled_ ? \" [Cancelled]\" : \"\");\n> > > + << (error_ ? \" [Error]\" : (cancelled_ ? \" [Cancelled]\" : \"\"));\n> > >\n> > > LIBCAMERA_TRACEPOINT(request_complete, this);\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 CBD36C0DA3\n\tfor ;\n\tSat, 3 Apr 2021 01:35:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3C36668786;\n\tSat, 3 Apr 2021 03:35:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 33C95602CF\n\tfor ;\n\tSat, 3 Apr 2021 03:35:54 +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 92D403D7;\n\tSat, 3 Apr 2021 03:35:53 +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=\"XgFg63Wl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617413753;\n\tbh=o8poRqQjlBhHQ1cTGhmStuYGnlZzqLnH31m3rStUuGY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XgFg63Wlln4AlYhleLa0pwi0hId6fkB6z99ustuOBAt+Cx2orrcNovym0HmCAfXCi\n\toW5TXa9aqZEDg/HkDVhtRJAeaWZnJ9uZkKuhKz/6cgRK0jKzJOoy2lqZWuSMcoMrKA\n\tJcMyFajElRntfnFyxyho+3lgSZBgVaN3kdIqe6eE=","Date":"Sat, 3 Apr 2021 04:35:09 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210329091552.157208-1-hiroh@chromium.org>\n\t<20210329091552.157208-3-hiroh@chromium.org>\n\t<20210329094039.s7e2qv3brcr7btym@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: Request: Add\n\tRequestError to Status","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\" "}}]