[{"id":16864,"web_url":"https://patchwork.libcamera.org/comment/16864/","msgid":"","date":"2021-05-10T20:25:19","subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add\n\tRequest::cancel()","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2021-05-10 12:52:29 +0200, Jacopo Mondi wrote:\n> Add a cancel() function to the Request class that allows to forcefully\n> complete the request and its associated buffers in error state.\n> \n> Only pending requests can be forcefully cancelled. Enforce that\n> by asserting the request state to be RequestPending.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/request.h | 1 +\n> src/libcamera/request.cpp | 30 ++++++++++++++++++++++++++++++\n> 2 files changed, 31 insertions(+)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 4cf5ff3f7d3b..5596901ddd8e 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -65,6 +65,7 @@ private:\n> \tfriend class PipelineHandler;\n> \n> \tvoid complete();\n> +\tvoid cancel();\n> \n> \tbool completeBuffer(FrameBuffer *buffer);\n> \n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index ce2dd7b17f10..fc5e25199112 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -292,6 +292,36 @@ void Request::complete()\n> \tLIBCAMERA_TRACEPOINT(request_complete, this);\n> }\n> \n> +/**\n> + * \\brief Cancel a queued request\n> + *\n> + * Mark the request and its associated buffers as cancelled and complete it.\n> + *\n> + * Set the status of each buffer in the request to the frame cancelled state and\n> + * remove them from the pending buffer queue before completing the request with\n> + * error.\n> + */\n> +void Request::cancel()\n> +{\n> +\tLIBCAMERA_TRACEPOINT(request_cancel, this);\n> +\n> +\tASSERT(status_ == RequestPending);\n> +\n> +\t/*\n> +\t * We can't simply loop and call completeBuffer() as erase() invalidates\n> +\t * pointers and iterators, so we have to manually cancel the buffer and\n> +\t * erase it from the pending buffers list.\n> +\t */\n> +\tfor (auto buffer = pending_.begin(); buffer != pending_.end();) {\n> +\t\t(*buffer)->cancel();\n> +\t\t(*buffer)->setRequest(nullptr);\n> +\t\tbuffer = pending_.erase(buffer);\n> +\t}\n\nI wonder how this works if a buffer have been queued to hardware but not \nyet completed? Do we need a new Request status RequestProcessing(?) to \ndeal with such a corner case or maybe it's not a problem?\n\n> +\n> +\tcancelled_ = true;\n> +\tcomplete();\n> +}\n> +\n> /**\n> * \\brief Complete a buffer for the request\n> * \\param[in] buffer The buffer that has completed\n> -- \n> 2.31.1\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 C8FC8BF835\n\tfor ;\n\tMon, 10 May 2021 20:25:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 097C9688E5;\n\tMon, 10 May 2021 22:25:24 +0200 (CEST)","from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F3C4B602BE\n\tfor ;\n\tMon, 10 May 2021 22:25:21 +0200 (CEST)","by mail-lf1-x130.google.com with SMTP id z13so25329870lft.1\n\tfor ;\n\tMon, 10 May 2021 13:25:21 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tq27sm3390535ljm.127.2021.05.10.13.25.20\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 10 May 2021 13:25:20 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"Eaj99SO9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=IDvcq7MUsqb8gm0zzpknu3AePCEs017NhGLwxFOcjY8=;\n\tb=Eaj99SO9CIQN9NUSgjRKEmjGA3hsBIqotUZ0poY6VpyuGlG6eYU/a2a+QLbH4iLY5o\n\tqk7uYK9fxahZBD1u4dLpEhGySjGXeMq7v+fLClIai1kjPOVKlf2B20nHLGNo6fTM4dw0\n\tQdemQDjmX86fA6/er1DtGU8OWJIK8F6O5lF5IZIjvgX6FmsUh7KD4W4+TGpTWiuKXS9Z\n\tZiuoqRWde7embAgPAi343CCQ0P+rEjfTgkOwx30HxCnrON02Q2HDWbFKMYuA+Cl2zB0Z\n\tig4aa2+jEGsK4L4KAb42Fc17Lnf+iGUlEdpIEQzob1SODoSUf2+VOHNt6EN6ADV8EL6/\n\tg1zg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=IDvcq7MUsqb8gm0zzpknu3AePCEs017NhGLwxFOcjY8=;\n\tb=AlqNKk76ga7VaDR4hxt0xg0EN3E4SIOn00aRfsa7+cVEy69c+cW7naDTUGdUpl0xwT\n\tpECRo+GytLq0o5JtCiPuEfWfgga1OWaMUKbwCpZ8Pfy7aWQx52FxSYScct7tnFISU7Hu\n\taN9++hWzkWzKyccGfjGl8D684NPUOu3zGmcXKIi3bZbAD+RR/06Z3egEwAWC/mqvGWSC\n\tVhHWU04syBxY4yhsd+B9HC/aXMkD/w4XimZOVkpjN99YsTuS7qkMEVUGl/8gaY/cyD42\n\tZ7HzxXuQKCwVM45vHj6EH18kYP/CnSu31/Gd2V89qVmZ9p+npXLvyLRMRjzhh2uLLK2m\n\th/HA==","X-Gm-Message-State":"AOAM533Fc7eROq5sgX5QPOXSFLNAvV/B2QUI8fYVHaSOO1OrhBPC+HFZ\n\toMeeZQyi9kIwuE6dECERsp6PWg==","X-Google-Smtp-Source":"ABdhPJzthVYI7jhdgohELirF7mIujTrGi9mRT3F5c2fRFQhh5yALFy8Yx3+LhMQzIvUdk1jF5d29eg==","X-Received":"by 2002:a05:6512:45a:: with SMTP id\n\ty26mr18181066lfk.128.1620678321188; \n\tMon, 10 May 2021 13:25:21 -0700 (PDT)","Date":"Mon, 10 May 2021 22:25:19 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210510105235.28319-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add\n\tRequest::cancel()","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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16879,"web_url":"https://patchwork.libcamera.org/comment/16879/","msgid":"","date":"2021-05-11T04:45:26","subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add\n\tRequest::cancel()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for the patch.\n\nOn Tue, May 11, 2021 at 5:25 AM Niklas Söderlund <\nniklas.soderlund@ragnatech.se> wrote:\n\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2021-05-10 12:52:29 +0200, Jacopo Mondi wrote:\n> > Add a cancel() function to the Request class that allows to forcefully\n> > complete the request and its associated buffers in error state.\n> >\n> > Only pending requests can be forcefully cancelled. Enforce that\n> > by asserting the request state to be RequestPending.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/request.h | 1 +\n> > src/libcamera/request.cpp | 30 ++++++++++++++++++++++++++++++\n> > 2 files changed, 31 insertions(+)\n> >\n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index 4cf5ff3f7d3b..5596901ddd8e 100644\n> > --- a/include/libcamera/request.h\n> > +++ b/include/libcamera/request.h\n> > @@ -65,6 +65,7 @@ private:\n> > friend class PipelineHandler;\n> >\n> > void complete();\n> > + void cancel();\n> >\n> > bool completeBuffer(FrameBuffer *buffer);\n> >\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index ce2dd7b17f10..fc5e25199112 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -292,6 +292,36 @@ void Request::complete()\n> > LIBCAMERA_TRACEPOINT(request_complete, this);\n> > }\n> >\n> > +/**\n> > + * \\brief Cancel a queued request\n> > + *\n> > + * Mark the request and its associated buffers as cancelled and\n> complete it.\n> > + *\n> > + * Set the status of each buffer in the request to the frame cancelled\n> state and\n> > + * remove them from the pending buffer queue before completing the\n> request with\n> > + * error.\n> > + */\n> > +void Request::cancel()\n> > +{\n> > + LIBCAMERA_TRACEPOINT(request_cancel, this);\n> > +\n> > + ASSERT(status_ == RequestPending);\n> > +\n> > + /*\n> > + * We can't simply loop and call completeBuffer() as erase()\n> invalidates\n> > + * pointers and iterators, so we have to manually cancel the\n> buffer and\n> > + * erase it from the pending buffers list.\n> > + */\n> > + for (auto buffer = pending_.begin(); buffer != pending_.end();) {\n> > + (*buffer)->cancel();\n> > + (*buffer)->setRequest(nullptr);\n>\n\nShall we do (*buffer)->metadata().status = FrameMetadata::FrameCancelled?\n\n\n> > + buffer = pending_.erase(buffer);\n> > + }\n>\n> I wonder how this works if a buffer have been queued to hardware but not\n> yet completed? Do we need a new Request status RequestProcessing(?) to\n> deal with such a corner case or maybe it's not a problem?\n>\n>\nI think Request::cancel() should be called when a request and its buffers\nare not in the hardware.\n\n-Hiro\n\n\n> > +\n> > + cancelled_ = true;\n> > + complete();\n> > +}\n> > +\n> > /**\n> > * \\brief Complete a buffer for the request\n> > * \\param[in] buffer The buffer that has completed\n> > --\n> > 2.31.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 86F9DBF839\n\tfor ;\n\tTue, 11 May 2021 04:45:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EA42D6891C;\n\tTue, 11 May 2021 06:45:38 +0200 (CEST)","from mail-ej1-x636.google.com (mail-ej1-x636.google.com\n\t[IPv6:2a00:1450:4864:20::636])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 880E368915\n\tfor ;\n\tTue, 11 May 2021 06:45:37 +0200 (CEST)","by mail-ej1-x636.google.com with SMTP id ga25so1974413ejb.12\n\tfor ;\n\tMon, 10 May 2021 21:45:37 -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=\"LKrLXJrY\"; 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=QNBE0eoIJBYPtVbKBZd1XcfqdUKJH1oG8ZMhyYxkZqQ=;\n\tb=LKrLXJrYjW/aBIls//TAgb1wDl/Sr2DP79DaqJT3NvcOV5aX2qY7gGSubW4Z3zXGQT\n\tNKdiSxyVKZGdRi0rUeSu0RABYZZ4IDOlwPpuHEPFOAkHJXC1MycofaECKJTyZzAcWb7X\n\tBW5IX/rIcFm4rO97IQ31TV3NXuoGSzy5WS5RI=","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=QNBE0eoIJBYPtVbKBZd1XcfqdUKJH1oG8ZMhyYxkZqQ=;\n\tb=iBx0EfOjNGfZm4WMhJANYrffH4EUrsIyCegRdeDyCOG7O8QWgVOSAntzMphNccn78h\n\tiy0IfIIF/ynyI2zox/AWP4CdR1VNW2SJdprEa8Sfor1BKc1P6if33TajjrlE6oWozXgC\n\tcn1SdBkNheejJKtIqlnhGzsWNm3A2cVDQEB1aUP9AlC4XpDpKAxIX1RLraWoyosaysuc\n\tqSsV3Taou1++QuqIaE3SAhBvN0ZyjWY9hCBZfSfEd0nJIwRHafdDRhClTcTHQbeDyvUR\n\tYJoiTsGG1EEb1It9IXabWW50MUT2uczBZZ7Mf8X+fcAqgzo4wjivZJ6PxBu7AI03iuSU\n\togVg==","X-Gm-Message-State":"AOAM530C9nu38MlMbOyyU6MCJpsExrvc5KMkjag9yLC+rbDeo2s0ZO/u\n\t52gLPYtk3eJkWB4cUSOrvGSPDHsJ1Ieuv6hN6UL/7Q==","X-Google-Smtp-Source":"ABdhPJyaTy8UoxKYs3k/YCfBxALC3++imWZJURHYYnePXf3bJxshxRPZzUHGQii4Ee8fZNPraWUis8x6ieEZjz1N8Ao=","X-Received":"by 2002:a17:907:1b06:: with SMTP id\n\tmp6mr29875869ejc.292.1620708337162; \n\tMon, 10 May 2021 21:45:37 -0700 (PDT)","MIME-Version":"1.0","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-3-jacopo@jmondi.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 11 May 2021 13:45:26 +0900","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add\n\tRequest::cancel()","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":"multipart/mixed;\n\tboundary=\"===============7201616390705348551==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16886,"web_url":"https://patchwork.libcamera.org/comment/16886/","msgid":"<20210511082655.xqxdbuslo2o6kmoo@uno.localdomain>","date":"2021-05-11T08:26:55","subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add\n\tRequest::cancel()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Tue, May 11, 2021 at 01:45:26PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for the patch.\n>\n> On Tue, May 11, 2021 at 5:25 AM Niklas Söderlund <\n> niklas.soderlund@ragnatech.se> wrote:\n>\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2021-05-10 12:52:29 +0200, Jacopo Mondi wrote:\n> > > Add a cancel() function to the Request class that allows to forcefully\n> > > complete the request and its associated buffers in error state.\n> > >\n> > > Only pending requests can be forcefully cancelled. Enforce that\n> > > by asserting the request state to be RequestPending.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/request.h | 1 +\n> > > src/libcamera/request.cpp | 30 ++++++++++++++++++++++++++++++\n> > > 2 files changed, 31 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > index 4cf5ff3f7d3b..5596901ddd8e 100644\n> > > --- a/include/libcamera/request.h\n> > > +++ b/include/libcamera/request.h\n> > > @@ -65,6 +65,7 @@ private:\n> > > friend class PipelineHandler;\n> > >\n> > > void complete();\n> > > + void cancel();\n> > >\n> > > bool completeBuffer(FrameBuffer *buffer);\n> > >\n> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > index ce2dd7b17f10..fc5e25199112 100644\n> > > --- a/src/libcamera/request.cpp\n> > > +++ b/src/libcamera/request.cpp\n> > > @@ -292,6 +292,36 @@ void Request::complete()\n> > > LIBCAMERA_TRACEPOINT(request_complete, this);\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Cancel a queued request\n> > > + *\n> > > + * Mark the request and its associated buffers as cancelled and\n> > complete it.\n> > > + *\n> > > + * Set the status of each buffer in the request to the frame cancelled\n> > state and\n> > > + * remove them from the pending buffer queue before completing the\n> > request with\n> > > + * error.\n> > > + */\n> > > +void Request::cancel()\n> > > +{\n> > > + LIBCAMERA_TRACEPOINT(request_cancel, this);\n> > > +\n> > > + ASSERT(status_ == RequestPending);\n> > > +\n> > > + /*\n> > > + * We can't simply loop and call completeBuffer() as erase()\n> > invalidates\n> > > + * pointers and iterators, so we have to manually cancel the\n> > buffer and\n> > > + * erase it from the pending buffers list.\n> > > + */\n> > > + for (auto buffer = pending_.begin(); buffer != pending_.end();) {\n> > > + (*buffer)->cancel();\n> > > + (*buffer)->setRequest(nullptr);\n> >\n>\n> Shall we do (*buffer)->metadata().status = FrameMetadata::FrameCancelled?\n\nThat's exactly what (*buffer)->cancel() does :)\n\nclass FrameBuffer\n{\n ....\n\n\tvoid cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n\n}\n\n>\n>\n> > > + buffer = pending_.erase(buffer);\n> > > + }\n> >\n> > I wonder how this works if a buffer have been queued to hardware but not\n> > yet completed? Do we need a new Request status RequestProcessing(?) to\n> > deal with such a corner case or maybe it's not a problem?\n> >\n> >\n> I think Request::cancel() should be called when a request and its buffers\n> are not in the hardware.\n\nIdeally yes, but there's one issue. Once we add this method it becomes\napplication visible, and application can call it, at any time, beside\nthe fact that ph should be diligent and keep the state clean. Should\nthen queueRequestDevice look like:\n\n ret = csi2.queueRawBuffer(raw)\n if (ret)\n return ret;\n\n ret = isp.queueViewfinderBuffer(buffer);\n if (ret) {\n dequeueRawBuffer()\n return ret;\n }\n\nis this even possible ?\n\n>\n> -Hiro\n>\n>\n> > > +\n> > > + cancelled_ = true;\n> > > + complete();\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Complete a buffer for the request\n> > > * \\param[in] buffer The buffer that has completed\n> > > --\n> > > 2.31.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B8021BF829\n\tfor ;\n\tTue, 11 May 2021 08:26:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D973688E4;\n\tTue, 11 May 2021 10:26:14 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8894D602B7\n\tfor ;\n\tTue, 11 May 2021 10:26:12 +0200 (CEST)","from uno.localdomain (host-82-59-136-116.retail.telecomitalia.it\n\t[82.59.136.116]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id C23B81C000F;\n\tTue, 11 May 2021 08:26:11 +0000 (UTC)"],"X-Originating-IP":"82.59.136.116","Date":"Tue, 11 May 2021 10:26:55 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210511082655.xqxdbuslo2o6kmoo@uno.localdomain>","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-3-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add\n\tRequest::cancel()","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16887,"web_url":"https://patchwork.libcamera.org/comment/16887/","msgid":"","date":"2021-05-11T09:20:30","subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add\n\tRequest::cancel()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Tue, May 11, 2021 at 5:26 PM Jacopo Mondi wrote:\n\n> Hi Hiro,\n>\n> On Tue, May 11, 2021 at 01:45:26PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo, thank you for the patch.\n> >\n> > On Tue, May 11, 2021 at 5:25 AM Niklas Söderlund <\n> > niklas.soderlund@ragnatech.se> wrote:\n> >\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2021-05-10 12:52:29 +0200, Jacopo Mondi wrote:\n> > > > Add a cancel() function to the Request class that allows to\n> forcefully\n> > > > complete the request and its associated buffers in error state.\n> > > >\n> > > > Only pending requests can be forcefully cancelled. Enforce that\n> > > > by asserting the request state to be RequestPending.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > include/libcamera/request.h | 1 +\n> > > > src/libcamera/request.cpp | 30 ++++++++++++++++++++++++++++++\n> > > > 2 files changed, 31 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/request.h\n> b/include/libcamera/request.h\n> > > > index 4cf5ff3f7d3b..5596901ddd8e 100644\n> > > > --- a/include/libcamera/request.h\n> > > > +++ b/include/libcamera/request.h\n> > > > @@ -65,6 +65,7 @@ private:\n> > > > friend class PipelineHandler;\n> > > >\n> > > > void complete();\n> > > > + void cancel();\n> > > >\n> > > > bool completeBuffer(FrameBuffer *buffer);\n> > > >\n> > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > index ce2dd7b17f10..fc5e25199112 100644\n> > > > --- a/src/libcamera/request.cpp\n> > > > +++ b/src/libcamera/request.cpp\n> > > > @@ -292,6 +292,36 @@ void Request::complete()\n> > > > LIBCAMERA_TRACEPOINT(request_complete, this);\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\brief Cancel a queued request\n> > > > + *\n> > > > + * Mark the request and its associated buffers as cancelled and\n> > > complete it.\n> > > > + *\n> > > > + * Set the status of each buffer in the request to the frame\n> cancelled\n> > > state and\n> > > > + * remove them from the pending buffer queue before completing the\n> > > request with\n> > > > + * error.\n> > > > + */\n> > > > +void Request::cancel()\n> > > > +{\n> > > > + LIBCAMERA_TRACEPOINT(request_cancel, this);\n> > > > +\n> > > > + ASSERT(status_ == RequestPending);\n> > > > +\n> > > > + /*\n> > > > + * We can't simply loop and call completeBuffer() as erase()\n> > > invalidates\n> > > > + * pointers and iterators, so we have to manually cancel the\n> > > buffer and\n> > > > + * erase it from the pending buffers list.\n> > > > + */\n> > > > + for (auto buffer = pending_.begin(); buffer !=\n> pending_.end();) {\n> > > > + (*buffer)->cancel();\n> > > > + (*buffer)->setRequest(nullptr);\n> > >\n> >\n> > Shall we do (*buffer)->metadata().status = FrameMetadata::FrameCancelled?\n>\n> That's exactly what (*buffer)->cancel() does :)\n>\n> class FrameBuffer\n> {\n> ....\n>\n> void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n>\n> }\n>\n>\nAh, Ack.\n\n\n> >\n> >\n> > > > + buffer = pending_.erase(buffer);\n> > > > + }\n> > >\n> > > I wonder how this works if a buffer have been queued to hardware but\n> not\n> > > yet completed? Do we need a new Request status RequestProcessing(?) to\n> > > deal with such a corner case or maybe it's not a problem?\n> > >\n> > >\n> > I think Request::cancel() should be called when a request and its buffers\n> > are not in the hardware.\n>\n> Ideally yes, but there's one issue. Once we add this method it becomes\n> application visible, and application can call it, at any time, beside\n> the fact that ph should be diligent and keep the state clean. Should\n> then queueRequestDevice look like:\n>\n> ret = csi2.queueRawBuffer(raw)\n> if (ret)\n> return ret;\n>\n> ret = isp.queueViewfinderBuffer(buffer);\n> if (ret) {\n> dequeueRawBuffer()\n> return ret;\n> }\n>\n> is this even possible ?\n>\n>\nI think so. IPU3 is implemented so, isn't it?\n\n\n> >\n> > -Hiro\n> >\n> >\n> > > > +\n> > > > + cancelled_ = true;\n> > > > + complete();\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Complete a buffer for the request\n> > > > * \\param[in] buffer The buffer that has completed\n> > > > --\n> > > > 2.31.1\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\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 18A1CBF829\n\tfor ;\n\tTue, 11 May 2021 09:20:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 632F068915;\n\tTue, 11 May 2021 11:20:43 +0200 (CEST)","from mail-ej1-x62b.google.com (mail-ej1-x62b.google.com\n\t[IPv6:2a00:1450:4864:20::62b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 40A25602B7\n\tfor ;\n\tTue, 11 May 2021 11:20:41 +0200 (CEST)","by mail-ej1-x62b.google.com with SMTP id m12so28740817eja.2\n\tfor ;\n\tTue, 11 May 2021 02:20:41 -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=\"jx4ei9wL\"; 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=Mlfo18wyk2G5TGYyF4dbvoTugZQatzmwUZgYQhQLy6g=;\n\tb=jx4ei9wL4clrZiZIuDRwUwDqcThLNEpSWEzSA9QMxHoOuk76Im8rheUgAGTYBGYv7x\n\tnv+wUqsAghmp6pyfNMXeLMMX3bz2nyt5fNyuur2Rc0LxikHEyceOC26H+c+u3HV8t5BH\n\tBXplPQgrv/LLX1/Qi8mF1mJbMvon1ASdDlj18=","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=Mlfo18wyk2G5TGYyF4dbvoTugZQatzmwUZgYQhQLy6g=;\n\tb=i3JInlTHdJ+qQFTbeqxZhNfVNtjgjfhmkh3hlO8brOMyoYIYNVi31O463cBaVR/jzv\n\tOiEqmXXjSSq5gD4HmQbdceLpWeYSbePdTZVvDVxZtF/E0vmB3IUF5RYPJ4pv1d7ZYVGV\n\tSfwXsp595poKjLCW3XCbZNZecrkXz/X8yaF3643eXTxxzBA0p0Rqm1Uy3ErEyRkeuJ+D\n\t4Zy16WkXkhavkwwilVwugnW5Z2u7POsqeUkcZrJRpRb7eRVl9rgG7j/UXw+5PEZH9SiG\n\t+EUQO3HSfSQsBNDMIEzK9QMjocI8S2P1E/iMFBwmyefutq4iQPu0ERL34yKpVyUGqAT8\n\t7jVA==","X-Gm-Message-State":"AOAM5316BO9zY9y6d0MXGGm9qWqQFyWrrWMJexYZnRwhMsXChx2XZO4Z\n\t5IY51inPnRhbiJCvm8SsWCzsbk6uz8Mut+4U2ie24w==","X-Google-Smtp-Source":"ABdhPJyuhtTXqJ29pgVloK7/85PO5tWVFd1Tc+U0jpn0p/NQxMp/Jy1U5CPK7XGbD/HHBG3Md0r3PIi9Qs5c3JHEOoE=","X-Received":"by 2002:a17:906:3a04:: with SMTP id\n\tz4mr13933820eje.221.1620724840781; \n\tTue, 11 May 2021 02:20:40 -0700 (PDT)","MIME-Version":"1.0","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-3-jacopo@jmondi.org>\n\t\n\t\n\t<20210511082655.xqxdbuslo2o6kmoo@uno.localdomain>","In-Reply-To":"<20210511082655.xqxdbuslo2o6kmoo@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 11 May 2021 18:20:30 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add\n\tRequest::cancel()","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":"multipart/mixed;\n\tboundary=\"===============2303010651828162343==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16895,"web_url":"https://patchwork.libcamera.org/comment/16895/","msgid":"<20210511104455.bvwhwokewixixykb@uno.localdomain>","date":"2021-05-11T10:44:55","subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add\n\tRequest::cancel()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Tue, May 11, 2021 at 06:20:30PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo,\n>\n> On Tue, May 11, 2021 at 5:26 PM Jacopo Mondi wrote:\n>\n> > Hi Hiro,\n> >\n> > On Tue, May 11, 2021 at 01:45:26PM +0900, Hirokazu Honda wrote:\n> > > Hi Jacopo, thank you for the patch.\n> > >\n> > > On Tue, May 11, 2021 at 5:25 AM Niklas Söderlund <\n> > > niklas.soderlund@ragnatech.se> wrote:\n> > >\n> > > > Hi Jacopo,\n> > > >\n> > > > Thanks for your work.\n> > > >\n> > > > On 2021-05-10 12:52:29 +0200, Jacopo Mondi wrote:\n> > > > > Add a cancel() function to the Request class that allows to\n> > forcefully\n> > > > > complete the request and its associated buffers in error state.\n> > > > >\n> > > > > Only pending requests can be forcefully cancelled. Enforce that\n> > > > > by asserting the request state to be RequestPending.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > include/libcamera/request.h | 1 +\n> > > > > src/libcamera/request.cpp | 30 ++++++++++++++++++++++++++++++\n> > > > > 2 files changed, 31 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/request.h\n> > b/include/libcamera/request.h\n> > > > > index 4cf5ff3f7d3b..5596901ddd8e 100644\n> > > > > --- a/include/libcamera/request.h\n> > > > > +++ b/include/libcamera/request.h\n> > > > > @@ -65,6 +65,7 @@ private:\n> > > > > friend class PipelineHandler;\n> > > > >\n> > > > > void complete();\n> > > > > + void cancel();\n> > > > >\n> > > > > bool completeBuffer(FrameBuffer *buffer);\n> > > > >\n> > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > > index ce2dd7b17f10..fc5e25199112 100644\n> > > > > --- a/src/libcamera/request.cpp\n> > > > > +++ b/src/libcamera/request.cpp\n> > > > > @@ -292,6 +292,36 @@ void Request::complete()\n> > > > > LIBCAMERA_TRACEPOINT(request_complete, this);\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Cancel a queued request\n> > > > > + *\n> > > > > + * Mark the request and its associated buffers as cancelled and\n> > > > complete it.\n> > > > > + *\n> > > > > + * Set the status of each buffer in the request to the frame\n> > cancelled\n> > > > state and\n> > > > > + * remove them from the pending buffer queue before completing the\n> > > > request with\n> > > > > + * error.\n> > > > > + */\n> > > > > +void Request::cancel()\n> > > > > +{\n> > > > > + LIBCAMERA_TRACEPOINT(request_cancel, this);\n> > > > > +\n> > > > > + ASSERT(status_ == RequestPending);\n> > > > > +\n> > > > > + /*\n> > > > > + * We can't simply loop and call completeBuffer() as erase()\n> > > > invalidates\n> > > > > + * pointers and iterators, so we have to manually cancel the\n> > > > buffer and\n> > > > > + * erase it from the pending buffers list.\n> > > > > + */\n> > > > > + for (auto buffer = pending_.begin(); buffer !=\n> > pending_.end();) {\n> > > > > + (*buffer)->cancel();\n> > > > > + (*buffer)->setRequest(nullptr);\n> > > >\n> > >\n> > > Shall we do (*buffer)->metadata().status = FrameMetadata::FrameCancelled?\n> >\n> > That's exactly what (*buffer)->cancel() does :)\n> >\n> > class FrameBuffer\n> > {\n> > ....\n> >\n> > void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> >\n> > }\n> >\n> >\n> Ah, Ack.\n>\n>\n> > >\n> > >\n> > > > > + buffer = pending_.erase(buffer);\n> > > > > + }\n> > > >\n> > > > I wonder how this works if a buffer have been queued to hardware but\n> > not\n> > > > yet completed? Do we need a new Request status RequestProcessing(?) to\n> > > > deal with such a corner case or maybe it's not a problem?\n> > > >\n> > > >\n> > > I think Request::cancel() should be called when a request and its buffers\n> > > are not in the hardware.\n> >\n> > Ideally yes, but there's one issue. Once we add this method it becomes\n> > application visible, and application can call it, at any time, beside\n> > the fact that ph should be diligent and keep the state clean. Should\n> > then queueRequestDevice look like:\n> >\n> > ret = csi2.queueRawBuffer(raw)\n> > if (ret)\n> > return ret;\n> >\n> > ret = isp.queueViewfinderBuffer(buffer);\n> > if (ret) {\n> > dequeueRawBuffer()\n> > return ret;\n> > }\n> >\n> > is this even possible ?\n> >\n> >\n> I think so. IPU3 is implemented so, isn't it?\n\nIPU3 should be ok, as well as RPi as their state machine is IPA\ndriven, so they queue the raw frame and wait for the IPA to compute\nparameters before queuing buffers to the ISP.\n\nOther pipelines might require special attention to exit from a failed\nqueueRequestDevice() in a clean state. But that's very ph specific.\nLooking at how you're changing the ipu3 one, do you think using\nRequest::cancel() could be useful ?\n\n>\n>\n> > >\n> > > -Hiro\n> > >\n> > >\n> > > > > +\n> > > > > + cancelled_ = true;\n> > > > > + complete();\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\brief Complete a buffer for the request\n> > > > > * \\param[in] buffer The buffer that has completed\n> > > > > --\n> > > > > 2.31.1\n> > > > >\n> > > > > _______________________________________________\n> > > > > libcamera-devel mailing list\n> > > > > libcamera-devel@lists.libcamera.org\n> > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > >\n> > > > --\n> > > > Regards,\n> > > > Niklas Söderlund\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > >\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 43640BF829\n\tfor ;\n\tTue, 11 May 2021 10:44:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C39326890C;\n\tTue, 11 May 2021 12:44:15 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B093761538\n\tfor ;\n\tTue, 11 May 2021 12:44:13 +0200 (CEST)","from uno.localdomain (host-82-59-136-116.retail.telecomitalia.it\n\t[82.59.136.116]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 95441240003;\n\tTue, 11 May 2021 10:44:12 +0000 (UTC)"],"Date":"Tue, 11 May 2021 12:44:55 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210511104455.bvwhwokewixixykb@uno.localdomain>","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-3-jacopo@jmondi.org>\n\t\n\t\n\t<20210511082655.xqxdbuslo2o6kmoo@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add\n\tRequest::cancel()","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16901,"web_url":"https://patchwork.libcamera.org/comment/16901/","msgid":"","date":"2021-05-11T13:00:55","subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add\n\tRequest::cancel()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Tue, May 11, 2021 at 7:44 PM Jacopo Mondi wrote:\n\n> Hi Hiro,\n>\n> On Tue, May 11, 2021 at 06:20:30PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo,\n> >\n> > On Tue, May 11, 2021 at 5:26 PM Jacopo Mondi wrote:\n> >\n> > > Hi Hiro,\n> > >\n> > > On Tue, May 11, 2021 at 01:45:26PM +0900, Hirokazu Honda wrote:\n> > > > Hi Jacopo, thank you for the patch.\n> > > >\n> > > > On Tue, May 11, 2021 at 5:25 AM Niklas Söderlund <\n> > > > niklas.soderlund@ragnatech.se> wrote:\n> > > >\n> > > > > Hi Jacopo,\n> > > > >\n> > > > > Thanks for your work.\n> > > > >\n> > > > > On 2021-05-10 12:52:29 +0200, Jacopo Mondi wrote:\n> > > > > > Add a cancel() function to the Request class that allows to\n> > > forcefully\n> > > > > > complete the request and its associated buffers in error state.\n> > > > > >\n> > > > > > Only pending requests can be forcefully cancelled. Enforce that\n> > > > > > by asserting the request state to be RequestPending.\n> > > > > >\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > include/libcamera/request.h | 1 +\n> > > > > > src/libcamera/request.cpp | 30 ++++++++++++++++++++++++++++++\n> > > > > > 2 files changed, 31 insertions(+)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/request.h\n> > > b/include/libcamera/request.h\n> > > > > > index 4cf5ff3f7d3b..5596901ddd8e 100644\n> > > > > > --- a/include/libcamera/request.h\n> > > > > > +++ b/include/libcamera/request.h\n> > > > > > @@ -65,6 +65,7 @@ private:\n> > > > > > friend class PipelineHandler;\n> > > > > >\n> > > > > > void complete();\n> > > > > > + void cancel();\n> > > > > >\n> > > > > > bool completeBuffer(FrameBuffer *buffer);\n> > > > > >\n> > > > > > diff --git a/src/libcamera/request.cpp\n> b/src/libcamera/request.cpp\n> > > > > > index ce2dd7b17f10..fc5e25199112 100644\n> > > > > > --- a/src/libcamera/request.cpp\n> > > > > > +++ b/src/libcamera/request.cpp\n> > > > > > @@ -292,6 +292,36 @@ void Request::complete()\n> > > > > > LIBCAMERA_TRACEPOINT(request_complete, this);\n> > > > > > }\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\brief Cancel a queued request\n> > > > > > + *\n> > > > > > + * Mark the request and its associated buffers as cancelled and\n> > > > > complete it.\n> > > > > > + *\n> > > > > > + * Set the status of each buffer in the request to the frame\n> > > cancelled\n> > > > > state and\n> > > > > > + * remove them from the pending buffer queue before completing\n> the\n> > > > > request with\n> > > > > > + * error.\n> > > > > > + */\n> > > > > > +void Request::cancel()\n> > > > > > +{\n> > > > > > + LIBCAMERA_TRACEPOINT(request_cancel, this);\n> > > > > > +\n> > > > > > + ASSERT(status_ == RequestPending);\n> > > > > > +\n> > > > > > + /*\n> > > > > > + * We can't simply loop and call completeBuffer() as\n> erase()\n> > > > > invalidates\n> > > > > > + * pointers and iterators, so we have to manually cancel\n> the\n> > > > > buffer and\n> > > > > > + * erase it from the pending buffers list.\n> > > > > > + */\n> > > > > > + for (auto buffer = pending_.begin(); buffer !=\n> > > pending_.end();) {\n> > > > > > + (*buffer)->cancel();\n> > > > > > + (*buffer)->setRequest(nullptr);\n> > > > >\n> > > >\n> > > > Shall we do (*buffer)->metadata().status =\n> FrameMetadata::FrameCancelled?\n> > >\n> > > That's exactly what (*buffer)->cancel() does :)\n> > >\n> > > class FrameBuffer\n> > > {\n> > > ....\n> > >\n> > > void cancel() { metadata_.status =\n> FrameMetadata::FrameCancelled; }\n> > >\n> > > }\n> > >\n> > >\n> > Ah, Ack.\n> >\n> >\n> > > >\n> > > >\n> > > > > > + buffer = pending_.erase(buffer);\n> > > > > > + }\n> > > > >\n> > > > > I wonder how this works if a buffer have been queued to hardware\n> but\n> > > not\n> > > > > yet completed? Do we need a new Request status\n> RequestProcessing(?) to\n> > > > > deal with such a corner case or maybe it's not a problem?\n> > > > >\n> > > > >\n> > > > I think Request::cancel() should be called when a request and its\n> buffers\n> > > > are not in the hardware.\n> > >\n> > > Ideally yes, but there's one issue. Once we add this method it becomes\n> > > application visible, and application can call it, at any time, beside\n> > > the fact that ph should be diligent and keep the state clean. Should\n> > > then queueRequestDevice look like:\n> > >\n> > > ret = csi2.queueRawBuffer(raw)\n> > > if (ret)\n> > > return ret;\n> > >\n> > > ret = isp.queueViewfinderBuffer(buffer);\n> > > if (ret) {\n> > > dequeueRawBuffer()\n> > > return ret;\n> > > }\n> > >\n> > > is this even possible ?\n> > >\n> > >\n> > I think so. IPU3 is implemented so, isn't it?\n>\n> IPU3 should be ok, as well as RPi as their state machine is IPA\n> driven, so they queue the raw frame and wait for the IPA to compute\n> parameters before queuing buffers to the ISP.\n>\n> Other pipelines might require special attention to exit from a failed\n> queueRequestDevice() in a clean state. But that's very ph specific.\n> Looking at how you're changing the ipu3 one, do you think using\n> Request::cancel() could be useful ?\n>\n>\nYes, I did the same thing in the change. I will replace the code with\nRequest::cancel().\n\n\n> >\n> >\n> > > >\n> > > > -Hiro\n> > > >\n> > > >\n> > > > > > +\n> > > > > > + cancelled_ = true;\n> > > > > > + complete();\n> > > > > > +}\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\brief Complete a buffer for the request\n> > > > > > * \\param[in] buffer The buffer that has completed\n> > > > > > --\n> > > > > > 2.31.1\n> > > > > >\n> > > > > > _______________________________________________\n> > > > > > libcamera-devel mailing list\n> > > > > > libcamera-devel@lists.libcamera.org\n> > > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > >\n> > > > > --\n> > > > > Regards,\n> > > > > Niklas Söderlund\n> > > > > _______________________________________________\n> > > > > libcamera-devel mailing list\n> > > > > libcamera-devel@lists.libcamera.org\n> > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > >\n> > >\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 7956EBF829\n\tfor ;\n\tTue, 11 May 2021 13:01:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DEB956890C;\n\tTue, 11 May 2021 15:01:08 +0200 (CEST)","from mail-ed1-x535.google.com (mail-ed1-x535.google.com\n\t[IPv6:2a00:1450:4864:20::535])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C220761538\n\tfor ;\n\tTue, 11 May 2021 15:01:06 +0200 (CEST)","by mail-ed1-x535.google.com with SMTP id di13so22757558edb.2\n\tfor ;\n\tTue, 11 May 2021 06:01:06 -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=\"dGPC6CXH\"; 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=+r1Oedvll//6JZzmZ8qZL8mX4gOjFa8Ecs8wI6UY1Fw=;\n\tb=dGPC6CXHWy5l4NqReZPanN/82Tvk7tB5RzQ45gj0N1lwRoHmH3pwzTmjp2FfKUfA9O\n\t2GOol5KvGLRIPn3jGxpXybLr0FZG5q0UkOOuEBQzKruH5uIpDH+4gjLL5h++hM7HLiKS\n\tlKcVBE/LwqDUQMN5c20n5VCM1GQLnwhUMw6tU=","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=+r1Oedvll//6JZzmZ8qZL8mX4gOjFa8Ecs8wI6UY1Fw=;\n\tb=Ge6sWl8rw0g+oYLWNOaF8XabK9B4MtInccYnrQTvavmGZmczvfk0JFVHYxxK5jDdoM\n\tVuxQH234p+/P3BqPeR/XHERPI/sDfBcDWazRx5TOY2suzntcgf+3mfE7vb+zleoK00B+\n\t9MCFLML2aAKPezCup2V8TZkKrHna4RRyIzbmtYhCbh3NYPD0oIc7CccuwbNTAvihi+xX\n\tm3/DsWmp8SIaytQfa2dWn3V3CJZroT8Y8jzBvu6RlAUCvhhJaf67XgqYNfWbIPYwiaWQ\n\tAuVzuRZMjNiAdHSH03Egr9wWSRnjfhEiSOM0jQ9HCy8oFD32ZEtbL7z+1zBMHNq6xKvy\n\t1seA==","X-Gm-Message-State":"AOAM531yvntLZsAVV11UEIzPkSmoKNF66lZe4T+y4m4O+met4/HvSL8c\n\tt08n7BrfFBRZWSND/ayGd+wqNyNeZ07V/7S/ct17kw==","X-Google-Smtp-Source":"ABdhPJx1QiVg68qtmveBE4pQXGxoTP2mTktRf4wGtikpF9Gx9Hkttqlj4NL/jAXw5tJaWPOXDcrTgRA7vt/759CGh9w=","X-Received":"by 2002:a05:6402:50c6:: with SMTP id\n\th6mr36641699edb.327.1620738066216; \n\tTue, 11 May 2021 06:01:06 -0700 (PDT)","MIME-Version":"1.0","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-3-jacopo@jmondi.org>\n\t\n\t\n\t<20210511082655.xqxdbuslo2o6kmoo@uno.localdomain>\n\t\n\t<20210511104455.bvwhwokewixixykb@uno.localdomain>","In-Reply-To":"<20210511104455.bvwhwokewixixykb@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 11 May 2021 22:00:55 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add\n\tRequest::cancel()","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":"multipart/mixed;\n\tboundary=\"===============7464163446697390593==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16918,"web_url":"https://patchwork.libcamera.org/comment/16918/","msgid":"<20210512095437.5cyy6r7whnpfjv2b@uno.localdomain>","date":"2021-05-12T09:54:37","subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add\n\tRequest::cancel()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Tue, May 11, 2021 at 10:26:55AM +0200, Jacopo Mondi wrote:\n> Hi Hiro,\n>\n> On Tue, May 11, 2021 at 01:45:26PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo, thank you for the patch.\n> >\n> > On Tue, May 11, 2021 at 5:25 AM Niklas Söderlund <\n> > niklas.soderlund@ragnatech.se> wrote:\n> >\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2021-05-10 12:52:29 +0200, Jacopo Mondi wrote:\n> > > > Add a cancel() function to the Request class that allows to forcefully\n> > > > complete the request and its associated buffers in error state.\n> > > >\n> > > > Only pending requests can be forcefully cancelled. Enforce that\n> > > > by asserting the request state to be RequestPending.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > include/libcamera/request.h | 1 +\n> > > > src/libcamera/request.cpp | 30 ++++++++++++++++++++++++++++++\n> > > > 2 files changed, 31 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > > index 4cf5ff3f7d3b..5596901ddd8e 100644\n> > > > --- a/include/libcamera/request.h\n> > > > +++ b/include/libcamera/request.h\n> > > > @@ -65,6 +65,7 @@ private:\n> > > > friend class PipelineHandler;\n> > > >\n> > > > void complete();\n> > > > + void cancel();\n> > > >\n> > > > bool completeBuffer(FrameBuffer *buffer);\n> > > >\n> > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > index ce2dd7b17f10..fc5e25199112 100644\n> > > > --- a/src/libcamera/request.cpp\n> > > > +++ b/src/libcamera/request.cpp\n> > > > @@ -292,6 +292,36 @@ void Request::complete()\n> > > > LIBCAMERA_TRACEPOINT(request_complete, this);\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\brief Cancel a queued request\n> > > > + *\n> > > > + * Mark the request and its associated buffers as cancelled and\n> > > complete it.\n> > > > + *\n> > > > + * Set the status of each buffer in the request to the frame cancelled\n> > > state and\n> > > > + * remove them from the pending buffer queue before completing the\n> > > request with\n> > > > + * error.\n> > > > + */\n> > > > +void Request::cancel()\n> > > > +{\n> > > > + LIBCAMERA_TRACEPOINT(request_cancel, this);\n> > > > +\n> > > > + ASSERT(status_ == RequestPending);\n> > > > +\n> > > > + /*\n> > > > + * We can't simply loop and call completeBuffer() as erase()\n> > > invalidates\n> > > > + * pointers and iterators, so we have to manually cancel the\n> > > buffer and\n> > > > + * erase it from the pending buffers list.\n> > > > + */\n> > > > + for (auto buffer = pending_.begin(); buffer != pending_.end();) {\n> > > > + (*buffer)->cancel();\n> > > > + (*buffer)->setRequest(nullptr);\n> > >\n> >\n> > Shall we do (*buffer)->metadata().status = FrameMetadata::FrameCancelled?\n>\n> That's exactly what (*buffer)->cancel() does :)\n>\n> class FrameBuffer\n> {\n> ....\n>\n> \tvoid cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n>\n> }\n>\n> >\n> >\n> > > > + buffer = pending_.erase(buffer);\n> > > > + }\n> > >\n> > > I wonder how this works if a buffer have been queued to hardware but not\n> > > yet completed? Do we need a new Request status RequestProcessing(?) to\n> > > deal with such a corner case or maybe it's not a problem?\n> > >\n> > >\n> > I think Request::cancel() should be called when a request and its buffers\n> > are not in the hardware.\n>\n> Ideally yes, but there's one issue. Once we add this method it becomes\n> application visible, and application can call it, at any time, beside\n\nJust to add that I made the method private and only accessible to\nfriend class for the moment for this specific reason.","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 85005C31E3\n\tfor ;\n\tWed, 12 May 2021 09:53:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 118C76891A;\n\tWed, 12 May 2021 11:53:56 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A34AB688E4\n\tfor ;\n\tWed, 12 May 2021 11:53:54 +0200 (CEST)","from uno.localdomain (host-79-50-130-20.retail.telecomitalia.it\n\t[79.50.130.20]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id CC23660011;\n\tWed, 12 May 2021 09:53:53 +0000 (UTC)"],"X-Originating-IP":"79.50.130.20","Date":"Wed, 12 May 2021 11:54:37 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210512095437.5cyy6r7whnpfjv2b@uno.localdomain>","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-3-jacopo@jmondi.org>\n\t\n\t\n\t<20210511082655.xqxdbuslo2o6kmoo@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210511082655.xqxdbuslo2o6kmoo@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add\n\tRequest::cancel()","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]