[{"id":16391,"web_url":"https://patchwork.libcamera.org/comment/16391/","msgid":"<e5932d60-5437-306e-d80e-c17ec9daf626@ideasonboard.com>","date":"2021-04-20T17:25:12","subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kieran,\n\nThanks for the patch !\n\nOn 20/04/2021 15:07, Kieran Bingham wrote:\n> When the CIO2 returns a cancelled buffer, we will not queue buffers\n> to the IMGU.\n\nThis is my personal preference: s/IMGU/ImgU but do what you want ;-p\n\n> These buffers should be explicitly marked as cancelled to ensure\n> the application knows there is no valid metadata or frame data\n> provided in the buffer.\n> \n> Provide a cancel() method on the FrameBuffer to allow explicitly\n> cancelling a buffer.\n> \n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\nReviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n> ---\n>  include/libcamera/buffer.h           | 2 ++\n>  src/libcamera/buffer.cpp             | 8 ++++++++\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n>  3 files changed, 15 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 620f8a66f6a2..e0af00900409 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -53,6 +53,8 @@ public:\n>  \tunsigned int cookie() const { return cookie_; }\n>  \tvoid setCookie(unsigned int cookie) { cookie_ = cookie; }\n>  \n> +\tvoid cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> +\n>  private:\n>  \tLIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n>  \n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 75b2693281a7..7635226b9045 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -226,6 +226,14 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)\n>   * core never modifies the buffer cookie.\n>   */\n>  \n> +/**\n> + * \\fn FrameBuffer::cancel()\n> + * \\brief Marks the buffer as cancelled\n> + *\n> + * If a buffer is not used by a request, it shall be marked as cancelled to\n> + * indicate that the metadata is invalid.\n> + */\n> +\n>  /**\n>   * \\class MappedBuffer\n>   * \\brief Provide an interface to support managing memory mapped buffers\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 51446fcf5bc1..73306cea6b37 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1257,8 +1257,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>  \n>  \t/* If the buffer is cancelled force a complete of the whole request. */\n>  \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> -\t\tfor (auto it : request->buffers())\n> -\t\t\tpipe_->completeBuffer(request, it.second);\n> +\t\tfor (auto it : request->buffers()) {\n> +\t\t\tFrameBuffer *b = it.second;\n> +\t\t\tb->cancel();\n> +\t\t\tpipe_->completeBuffer(request, b);\n> +\t\t}\n>  \n>  \t\tframeInfos_.remove(info);\n>  \t\tpipe_->completeRequest(request);\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 98983BDB16\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 20 Apr 2021 17:25:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 168D760516;\n\tTue, 20 Apr 2021 19:25:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D22460516\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 20 Apr 2021 19:25:13 +0200 (CEST)","from [IPv6:2a01:e0a:169:7140:6781:d49:79ac:cf13] (unknown\n\t[IPv6:2a01:e0a:169:7140:6781:d49:79ac:cf13])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AAB3A411;\n\tTue, 20 Apr 2021 19:25:12 +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=\"ncLwfKx4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618939512;\n\tbh=Xnw2zb3Z64zmCxW3CMftIdiEnAtccxFR+hGCT3qkTNg=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=ncLwfKx41WATBQlJ/BN+d5sWGOcKxdndxnF8wt5V6ZtSM+JT9iwPbBA5eaLYql5V7\n\toFAwQ6k9I/xSizS/MwmprlF+KaeTVUZAh6l3XTv/NmY6sVJDOpq2fg91CTJNGblrEn\n\tfIsz4gtChycxOqDxVD8WzUV/8oq+Urk+U9gZsOYs=","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera devel <libcamera-devel@lists.libcamera.org>","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-5-kieran.bingham@ideasonboard.com>","From":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<e5932d60-5437-306e-d80e-c17ec9daf626@ideasonboard.com>","Date":"Tue, 20 Apr 2021 19:25:12 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"<20210420130741.236848-5-kieran.bingham@ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16395,"web_url":"https://patchwork.libcamera.org/comment/16395/","msgid":"<CAEmqJPqW1HPtw4cj-n4kQCjLgpETY2dt30n7mJytk8RToFiuDQ@mail.gmail.com>","date":"2021-04-20T18:56:58","subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\n\nOn Tue, 20 Apr 2021 at 14:07, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> When the CIO2 returns a cancelled buffer, we will not queue buffers\n> to the IMGU.\n>\n> These buffers should be explicitly marked as cancelled to ensure\n> the application knows there is no valid metadata or frame data\n> provided in the buffer.\n>\n> Provide a cancel() method on the FrameBuffer to allow explicitly\n> cancelling a buffer.\n>\n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n>  include/libcamera/buffer.h           | 2 ++\n>  src/libcamera/buffer.cpp             | 8 ++++++++\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n>  3 files changed, 15 insertions(+), 2 deletions(-)\n>\n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 620f8a66f6a2..e0af00900409 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -53,6 +53,8 @@ public:\n>         unsigned int cookie() const { return cookie_; }\n>         void setCookie(unsigned int cookie) { cookie_ = cookie; }\n>\n> +       void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> +\n>  private:\n>         LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n>\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 75b2693281a7..7635226b9045 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -226,6 +226,14 @@ FrameBuffer::FrameBuffer(const std::vector<Plane>\n> &planes, unsigned int cookie)\n>   * core never modifies the buffer cookie.\n>   */\n>\n> +/**\n> + * \\fn FrameBuffer::cancel()\n> + * \\brief Marks the buffer as cancelled\n> + *\n> + * If a buffer is not used by a request, it shall be marked as cancelled\n> to\n> + * indicate that the metadata is invalid.\n> + */\n> +\n>  /**\n>   * \\class MappedBuffer\n>   * \\brief Provide an interface to support managing memory mapped buffers\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 51446fcf5bc1..73306cea6b37 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1257,8 +1257,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer\n> *buffer)\n>\n>         /* If the buffer is cancelled force a complete of the whole\n> request. */\n>         if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> -               for (auto it : request->buffers())\n> -                       pipe_->completeBuffer(request, it.second);\n> +               for (auto it : request->buffers()) {\n> +                       FrameBuffer *b = it.second;\n> +                       b->cancel();\n> +                       pipe_->completeBuffer(request, b);\n> +               }\n>\n\nWould you consider adding a Request::cancel() method to the API that\npipeline\nhandlers can invoke to essentially perform the above logic?  I found it a\nbit\nconfusing having to call completeBuffer on a cancelled buffer, and\ncompleteRequest\nto complete cancelling the request if you see what I mean?  This is\nprobably a bit\nmore relevant to the Raspberry Pi pipeline handler where we queue up\nrequests\nif required.\n\nRegards,\nNaush\n\n\n>                 frameInfos_.remove(info);\n>                 pipe_->completeRequest(request);\n> --\n> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 5BBF4BDB16\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 20 Apr 2021 18:57:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8DFA768840;\n\tTue, 20 Apr 2021 20:57:16 +0200 (CEST)","from mail-lj1-x232.google.com (mail-lj1-x232.google.com\n\t[IPv6:2a00:1450:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 373F760516\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 20 Apr 2021 20:57:15 +0200 (CEST)","by mail-lj1-x232.google.com with SMTP id a36so33961458ljq.8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 20 Apr 2021 11:57:15 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"Z6pYm48I\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=IRX8Q7uXEZBL5yAmIG2mbDRPuz2rcFOEA8YoGaahzq0=;\n\tb=Z6pYm48I61Kxlm14SlA5DPfkC3AdKlF3oPo53RC8EHDidx0tAqf2RbviNpDAyuArGY\n\t6FaDeW5m+Uf9Fd5oRLSnB+5dlqwhh0RTlYZANSmhfFaBT7BP8y28ZdDFf2a4rh43lOAx\n\tpqkU2rvsuHzYi1Q5CIeVZuL6qnvmBDIKSRZGKm443SNOgIwiFrOzyNSYUBssCsaUf9oa\n\tQh6crk9WtRHLST5KxIR5ja9zcQ1iB2TW2P0l03DJczAw0M9vewUKPumLUGVd39EwBG/A\n\t19c8lMZwEvCbBHpjg+olRjUweI4ZJzljPGtZHNXbd546PBn4Q3p3Xbkr5d6dAQgIPtVC\n\tF8NQ==","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=IRX8Q7uXEZBL5yAmIG2mbDRPuz2rcFOEA8YoGaahzq0=;\n\tb=Npf3tsbA6dCa8VJrNbppIFgPyrgy3p6/Z56OKVMu7wd8Qd4+AtYvtKbwcy9lYYFKX8\n\tInhgMqPL2bg2TbtYA4U5bPwwLMp8DMo7mhTjjStUZj1nsKPNJVdJJ0fKvAA7nMMn21rD\n\tZfsPXFlCgJBxqvHExoC8l4Q3Og5wpWwHunNpASPQIo6tnRS+Bni80ROVb24FrlqzMdN9\n\t+QR2Tkh77iUvGuevvgQAhyeMn9KmeMsWJs7hC3wx5cAUQbid89bH/oqk7pgnp7vyhn4W\n\t0Z1qkVtyYEsur1WnzfdedeODQZClbgJt5ViqrcN4WqYJHdjImyWiqs4PrgFuBl6yTFPz\n\tM8tw==","X-Gm-Message-State":"AOAM531akRoVQ2Pr0T4NxHPamhvZbfRfhv22ePN/lc30vpHvHF5J4K7i\n\tws84bo6jgpws3zMONxkzYBd0U1387xDO31x9b2KlBA==","X-Google-Smtp-Source":"ABdhPJyUiLZFYPRB4j+BU5uhna/mQ31P2Yy4zJyelqXmB9XG7u4augiTf5xUNnUHU5pKjDVLhST+Q9AtEgK18Ssnyp0=","X-Received":"by 2002:a05:651c:335:: with SMTP id\n\tb21mr2463523ljp.299.1618945034570; \n\tTue, 20 Apr 2021 11:57:14 -0700 (PDT)","MIME-Version":"1.0","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-5-kieran.bingham@ideasonboard.com>","In-Reply-To":"<20210420130741.236848-5-kieran.bingham@ideasonboard.com>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Tue, 20 Apr 2021 19:56:58 +0100","Message-ID":"<CAEmqJPqW1HPtw4cj-n4kQCjLgpETY2dt30n7mJytk8RToFiuDQ@mail.gmail.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"multipart/mixed;\n\tboundary=\"===============7315337931744602868==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16396,"web_url":"https://patchwork.libcamera.org/comment/16396/","msgid":"<ae85ed78-e9e2-bbb0-e677-9e689e745a64@ideasonboard.com>","date":"2021-04-20T19:13:25","subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 20/04/2021 19:56, Naushir Patuck wrote:\n> Hi Kieran,\n> \n> \n> On Tue, 20 Apr 2021 at 14:07, Kieran Bingham\n> <kieran.bingham@ideasonboard.com\n> <mailto:kieran.bingham@ideasonboard.com>> wrote:\n> \n>     When the CIO2 returns a cancelled buffer, we will not queue buffers\n>     to the IMGU.\n> \n>     These buffers should be explicitly marked as cancelled to ensure\n>     the application knows there is no valid metadata or frame data\n>     provided in the buffer.\n> \n>     Provide a cancel() method on the FrameBuffer to allow explicitly\n>     cancelling a buffer.\n> \n>     Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com\n>     <mailto:kieran.bingham@ideasonboard.com>>\n>     ---\n>      include/libcamera/buffer.h           | 2 ++\n>      src/libcamera/buffer.cpp             | 8 ++++++++\n>      src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n>      3 files changed, 15 insertions(+), 2 deletions(-)\n> \n>     diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n>     index 620f8a66f6a2..e0af00900409 100644\n>     --- a/include/libcamera/buffer.h\n>     +++ b/include/libcamera/buffer.h\n>     @@ -53,6 +53,8 @@ public:\n>             unsigned int cookie() const { return cookie_; }\n>             void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> \n>     +       void cancel() { metadata_.status =\n>     FrameMetadata::FrameCancelled; }\n>     +\n>      private:\n>             LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> \n>     diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n>     index 75b2693281a7..7635226b9045 100644\n>     --- a/src/libcamera/buffer.cpp\n>     +++ b/src/libcamera/buffer.cpp\n>     @@ -226,6 +226,14 @@ FrameBuffer::FrameBuffer(const\n>     std::vector<Plane> &planes, unsigned int cookie)\n>       * core never modifies the buffer cookie.\n>       */\n> \n>     +/**\n>     + * \\fn FrameBuffer::cancel()\n>     + * \\brief Marks the buffer as cancelled\n>     + *\n>     + * If a buffer is not used by a request, it shall be marked as\n>     cancelled to\n>     + * indicate that the metadata is invalid.\n>     + */\n>     +\n>      /**\n>       * \\class MappedBuffer\n>       * \\brief Provide an interface to support managing memory mapped\n>     buffers\n>     diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>     b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>     index 51446fcf5bc1..73306cea6b37 100644\n>     --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>     +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>     @@ -1257,8 +1257,11 @@ void\n>     IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> \n>             /* If the buffer is cancelled force a complete of the whole\n>     request. */\n>             if (buffer->metadata().status ==\n>     FrameMetadata::FrameCancelled) {\n>     -               for (auto it : request->buffers())\n>     -                       pipe_->completeBuffer(request, it.second);\n>     +               for (auto it : request->buffers()) {\n>     +                       FrameBuffer *b = it.second;\n>     +                       b->cancel();\n>     +                       pipe_->completeBuffer(request, b);\n>     +               }\n> \n> \n> Would you consider adding a Request::cancel() method to the API that\n> pipeline\n> handlers can invoke to essentially perform the above logic?  I found it\n> a bit\n> confusing having to call completeBuffer on a cancelled buffer, and\n> completeRequest\n> to complete cancelling the request if you see what I mean?  This is\n> probably a bit\n> more relevant to the Raspberry Pi pipeline handler where we queue up\n> requests\n> if required.\n\n\nI think this could be helpful sure, but maybe as a private/internal call\nonly (not in the public API). That might be possible now that we have\nRequest as an Extensible class later in this series.\n   (In fact, this Buffer->cancel() is public ... eep?)\n\nIn this instance where the CIO2 (the receiver) has caused the\ncancellation, a simple helper makes sense to clear the request entirely\nand return it as we know that there are no other resources pending.\n\nBut if this were on the completion path in the IMGU (the ISP), then I'd\nbe concerned about races with multiple completion handlers trying to\n'cancel' the request ...\n\nBut if we can find ways to simplify handling of shutdown paths I'm all\nfor it ;-)\n\nAny thoughts?\n\n--\nKieran\n\n\n> \n> Regards,\n> Naush\n> \n> \n>                     frameInfos_.remove(info);\n>                     pipe_->completeRequest(request);\n>     -- \n>     2.25.1\n> \n>     _______________________________________________\n>     libcamera-devel mailing list\n>     libcamera-devel@lists.libcamera.org\n>     <mailto:libcamera-devel@lists.libcamera.org>\n>     https://lists.libcamera.org/listinfo/libcamera-devel\n>     <https://lists.libcamera.org/listinfo/libcamera-devel>\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 BAA43BDB15\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 20 Apr 2021 19:13:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 410CD68845;\n\tTue, 20 Apr 2021 21:13:30 +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 7777B68839\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 20 Apr 2021 21:13:29 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C364E411;\n\tTue, 20 Apr 2021 21:13:28 +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=\"dqkowo4t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618946008;\n\tbh=+XWdyGG8s1rbua7BkW/l5J/XjjEW8fxbEVsQvJ/rBIQ=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=dqkowo4tUWtpdGwT/9snpBUc+nW6EwMzD5n271Feyp8a67nGpQQbnlzmGMW8kjvKk\n\toDsYyBd69dalRkrST8rc+5sRwnu4IvP7Y/D7bKp/trXpzwzbZirgLw7tdNvNgk/j2X\n\t3GxPWeF7+XgI8kSKJmX6Dfb/yzSu0mCBMwsMgDTc=","To":"Naushir Patuck <naush@raspberrypi.com>","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-5-kieran.bingham@ideasonboard.com>\n\t<CAEmqJPqW1HPtw4cj-n4kQCjLgpETY2dt30n7mJytk8RToFiuDQ@mail.gmail.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Organization":"Ideas on Board","Message-ID":"<ae85ed78-e9e2-bbb0-e677-9e689e745a64@ideasonboard.com>","Date":"Tue, 20 Apr 2021 20:13:25 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"<CAEmqJPqW1HPtw4cj-n4kQCjLgpETY2dt30n7mJytk8RToFiuDQ@mail.gmail.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16418,"web_url":"https://patchwork.libcamera.org/comment/16418/","msgid":"<YH9WtpwYyd0Xq6tU@pendragon.ideasonboard.com>","date":"2021-04-20T22:33:26","subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Tue, Apr 20, 2021 at 08:13:25PM +0100, Kieran Bingham wrote:\n> On 20/04/2021 19:56, Naushir Patuck wrote:\n> > On Tue, 20 Apr 2021 at 14:07, Kieran Bingham wrote:\n> > >\n> > > When the CIO2 returns a cancelled buffer, we will not queue buffers\n> > > to the IMGU.\n> > >\n> > > These buffers should be explicitly marked as cancelled to ensure\n> > > the application knows there is no valid metadata or frame data\n> > > provided in the buffer.\n> > >\n> > > Provide a cancel() method on the FrameBuffer to allow explicitly\n> > > cancelling a buffer.\n> > >\n> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > ---\n> > >  include/libcamera/buffer.h           | 2 ++\n> > >  src/libcamera/buffer.cpp             | 8 ++++++++\n> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n> > >  3 files changed, 15 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > > index 620f8a66f6a2..e0af00900409 100644\n> > > --- a/include/libcamera/buffer.h\n> > > +++ b/include/libcamera/buffer.h\n> > > @@ -53,6 +53,8 @@ public:\n> > >         unsigned int cookie() const { return cookie_; }\n> > >         void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > >\n> > > +       void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> > > +\n> > >  private:\n> > >         LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> > >\n> > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > > index 75b2693281a7..7635226b9045 100644\n> > > --- a/src/libcamera/buffer.cpp\n> > > +++ b/src/libcamera/buffer.cpp\n> > > @@ -226,6 +226,14 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)\n> > >   * core never modifies the buffer cookie.\n> > >   */\n> > >\n> > > +/**\n> > > + * \\fn FrameBuffer::cancel()\n> > > + * \\brief Marks the buffer as cancelled\n> > > + *\n> > > + * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > + * indicate that the metadata is invalid.\n> > > + */\n> > > +\n> > >  /**\n> > >   * \\class MappedBuffer\n> > >   * \\brief Provide an interface to support managing memory mapped buffers\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 51446fcf5bc1..73306cea6b37 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -1257,8 +1257,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > \n> > >         /* If the buffer is cancelled force a complete of the whole request. */\n> > >         if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > -               for (auto it : request->buffers())\n> > > -                       pipe_->completeBuffer(request, it.second);\n> > > +               for (auto it : request->buffers()) {\n> > > +                       FrameBuffer *b = it.second;\n> > > +                       b->cancel();\n> > > +                       pipe_->completeBuffer(request, b);\n> > > +               }\n> > \n> > Would you consider adding a Request::cancel() method to the API that pipeline\n> > handlers can invoke to essentially perform the above logic?  I found it a bit\n> > confusing having to call completeBuffer on a cancelled buffer, and completeRequest\n> > to complete cancelling the request if you see what I mean?  This is probably a bit\n> > more relevant to the Raspberry Pi pipeline handler where we queue up requests\n> > if required.\n> \n> I think this could be helpful sure, but maybe as a private/internal call\n> only (not in the public API). That might be possible now that we have\n> Request as an Extensible class later in this series.\n>    (In fact, this Buffer->cancel() is public ... eep?)\n> \n> In this instance where the CIO2 (the receiver) has caused the\n> cancellation, a simple helper makes sense to clear the request entirely\n> and return it as we know that there are no other resources pending.\n> \n> But if this were on the completion path in the IMGU (the ISP), then I'd\n> be concerned about races with multiple completion handlers trying to\n> 'cancel' the request ...\n> \n> But if we can find ways to simplify handling of shutdown paths I'm all\n> for it ;-)\n> \n> Any thoughts?\n\nA Request::cancel() function would make sense I believe. It's certainly\nuseful to cancel requests that have been put in an internal pipeline\nhandler queue, but whose buffers haven't been queued to the device. It\ncould also be used in this case I believe, as there should be no race.\nIf the pipeline handler had queued more than one buffer from the request\n(including internal buffers) to the device, that would be a different\nquestion. For instance, if we queued a raw buffer and an embedded data\nbuffer, they would complete (in the cancelled state) separately, in\nunknown order, so we couldn't call Request::cancel() sefely in that\ncase.\n\nWe could start with a first implementation the limits usage of\nRequest::cancel() to the cases where no buffers or a single buffer has\nbeen queued to the device, and improve on top, but I'd be interested in\nhearing proposals about how this could be further improved.","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 794F4BDB15\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 20 Apr 2021 22:33:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE0556883E;\n\tWed, 21 Apr 2021 00:33:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 857E668839\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 21 Apr 2021 00:33:31 +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 CA5AA45E;\n\tWed, 21 Apr 2021 00:33:30 +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=\"LWlh9hVz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618958011;\n\tbh=BGtGPyKEEeUljYm35pfdkK3NAYHp4zQj6QFl7B8uQUg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LWlh9hVz35M9+36Xd/wlH6Gw0R5A/sWSNhi9UDSKmfnYPXOsc5BMcMF0kBcsjryjm\n\t8baH5oPRwq8of/odO8EOpkRm4ac2V0SPG5yKK6QxayMZ+2BVIQ4p7yUcQK9fAGgzik\n\tEC+2pie6SIKi5C48YfkhbCknEEIumFcNdX94zIQA=","Date":"Wed, 21 Apr 2021 01:33:26 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YH9WtpwYyd0Xq6tU@pendragon.ideasonboard.com>","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-5-kieran.bingham@ideasonboard.com>\n\t<CAEmqJPqW1HPtw4cj-n4kQCjLgpETY2dt30n7mJytk8RToFiuDQ@mail.gmail.com>\n\t<ae85ed78-e9e2-bbb0-e677-9e689e745a64@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<ae85ed78-e9e2-bbb0-e677-9e689e745a64@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16427,"web_url":"https://patchwork.libcamera.org/comment/16427/","msgid":"<CAO5uPHNNkg14ZM8g-LYnAW9H4wsQM6imc=u4ANBdsVgK9xBQhA@mail.gmail.com>","date":"2021-04-21T06:12:50","subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Kieran, Thanks for the patch.\n\nOn Wed, Apr 21, 2021 at 7:33 AM Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> Hi Kieran,\n>\n> Thank you for the patch.\n>\n> On Tue, Apr 20, 2021 at 08:13:25PM +0100, Kieran Bingham wrote:\n> > On 20/04/2021 19:56, Naushir Patuck wrote:\n> > > On Tue, 20 Apr 2021 at 14:07, Kieran Bingham wrote:\n> > > >\n> > > > When the CIO2 returns a cancelled buffer, we will not queue buffers\n> > > > to the IMGU.\n> > > >\n> > > > These buffers should be explicitly marked as cancelled to ensure\n> > > > the application knows there is no valid metadata or frame data\n> > > > provided in the buffer.\n> > > >\n> > > > Provide a cancel() method on the FrameBuffer to allow explicitly\n> > > > cancelling a buffer.\n> > > >\n> > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > > ---\n> > > >  include/libcamera/buffer.h           | 2 ++\n> > > >  src/libcamera/buffer.cpp             | 8 ++++++++\n> > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n> > > >  3 files changed, 15 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > > > index 620f8a66f6a2..e0af00900409 100644\n> > > > --- a/include/libcamera/buffer.h\n> > > > +++ b/include/libcamera/buffer.h\n> > > > @@ -53,6 +53,8 @@ public:\n> > > >         unsigned int cookie() const { return cookie_; }\n> > > >         void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > > >\n> > > > +       void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> > > > +\n> > > >  private:\n> > > >         LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> > > >\n> > > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > > > index 75b2693281a7..7635226b9045 100644\n> > > > --- a/src/libcamera/buffer.cpp\n> > > > +++ b/src/libcamera/buffer.cpp\n> > > > @@ -226,6 +226,14 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)\n> > > >   * core never modifies the buffer cookie.\n> > > >   */\n> > > >\n> > > > +/**\n> > > > + * \\fn FrameBuffer::cancel()\n> > > > + * \\brief Marks the buffer as cancelled\n> > > > + *\n> > > > + * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > > + * indicate that the metadata is invalid.\n> > > > + */\n> > > > +\n> > > >  /**\n> > > >   * \\class MappedBuffer\n> > > >   * \\brief Provide an interface to support managing memory mapped buffers\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index 51446fcf5bc1..73306cea6b37 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -1257,8 +1257,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > >\n> > > >         /* If the buffer is cancelled force a complete of the whole request. */\n> > > >         if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > -               for (auto it : request->buffers())\n> > > > -                       pipe_->completeBuffer(request, it.second);\n> > > > +               for (auto it : request->buffers()) {\n> > > > +                       FrameBuffer *b = it.second;\n> > > > +                       b->cancel();\n> > > > +                       pipe_->completeBuffer(request, b);\n> > > > +               }\n> > >\n> > > Would you consider adding a Request::cancel() method to the API that pipeline\n> > > handlers can invoke to essentially perform the above logic?  I found it a bit\n> > > confusing having to call completeBuffer on a cancelled buffer, and completeRequest\n> > > to complete cancelling the request if you see what I mean?  This is probably a bit\n> > > more relevant to the Raspberry Pi pipeline handler where we queue up requests\n> > > if required.\n> >\n> > I think this could be helpful sure, but maybe as a private/internal call\n> > only (not in the public API). That might be possible now that we have\n> > Request as an Extensible class later in this series.\n> >    (In fact, this Buffer->cancel() is public ... eep?)\n> >\n> > In this instance where the CIO2 (the receiver) has caused the\n> > cancellation, a simple helper makes sense to clear the request entirely\n> > and return it as we know that there are no other resources pending.\n> >\n> > But if this were on the completion path in the IMGU (the ISP), then I'd\n> > be concerned about races with multiple completion handlers trying to\n> > 'cancel' the request ...\n> >\n> > But if we can find ways to simplify handling of shutdown paths I'm all\n> > for it ;-)\n> >\n> > Any thoughts?\n>\n> A Request::cancel() function would make sense I believe. It's certainly\n> useful to cancel requests that have been put in an internal pipeline\n> handler queue, but whose buffers haven't been queued to the device. It\n> could also be used in this case I believe, as there should be no race.\n> If the pipeline handler had queued more than one buffer from the request\n> (including internal buffers) to the device, that would be a different\n> question. For instance, if we queued a raw buffer and an embedded data\n> buffer, they would complete (in the cancelled state) separately, in\n> unknown order, so we couldn't call Request::cancel() sefely in that\n> case.\n>\n> We could start with a first implementation the limits usage of\n> Request::cancel() to the cases where no buffers or a single buffer has\n> been queued to the device, and improve on top, but I'd be interested in\n> hearing proposals about how this could be further improved.\n>\n\nAs this patch does a correct thing,\nReviewed-by: Hirokazu Honda <hiroh@chromium.org>\n\n> --\n> Regards,\n>\n> Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 DF6F4BDB16\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 21 Apr 2021 06:13:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40BB36883E;\n\tWed, 21 Apr 2021 08:13:03 +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 BB38E602C3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 21 Apr 2021 08:13:01 +0200 (CEST)","by mail-ed1-x535.google.com with SMTP id cq11so4253622edb.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 20 Apr 2021 23:13:01 -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=\"KGHTTwx5\"; 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=+SRveBMKkKverSRNkkuOZPai2Z3xE2CNH8xA1Vw0CEo=;\n\tb=KGHTTwx5Ob9NWLnRp9Wee64VVEOCLOxd0auX4ZjpdyU/cWh5hPdrIwsKL2tnKfOFVL\n\tciMgbZIRrh7SVc/LMTEK6KHY+/HGp11zkE+a80ou3b3IR5+p9+3F4N3Hzf9LWGs5582F\n\t71uOdaotexcDMg+hdg6XsyzHJKn2oGGJ1qbb8=","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=+SRveBMKkKverSRNkkuOZPai2Z3xE2CNH8xA1Vw0CEo=;\n\tb=nDcKwqQnSGT5ju+esSCKHFqX9Psgx6yhSVwgRE1/6/PrOWS+t4FQWU0wuW2NZ/c3I5\n\tf0b4jjZi3+YBahBClXkwDXjNGmRYwx+3Bv5WTKFQ6dtYYvdK3PC8/kRTt3r2N2q/hQCk\n\tFunlR/JlLj9qwFeiS58+QiKsFxCHaawVcPbgOzacI+adqXke4V+Ym30f3QZceS8TUW8o\n\tRVtvNc/KTmIL1+z6LGF7a7VfDUiXNKlw0MRl6Nah3uA8Z4Te8xCnOsOkKy4cAZi0uXgu\n\tNfL8K5eICNS2LGe1j0IyUg5F/9K43hRVK+ioPKOfS2zFxaN4Ueb+rgqFf2ZvdXPUUoHE\n\tYRsQ==","X-Gm-Message-State":"AOAM531SH/jlmcRQLPPE5MqprjMzBSliplYIW5bSZQKoKZ+5u0vSNl8Y\n\tMkNDOdBML6jFY6TEWoECfeo5herbfpCFFFGTlIaVM0fvqko=","X-Google-Smtp-Source":"ABdhPJw7mMXLJKzHmLQojXyVBvHGyGPdc6VWYiRluNXvbOfqlsI4QB5KqLqcgArRElePgZMjuTTIoqVS60M4Vvtqkbw=","X-Received":"by 2002:a05:6402:488:: with SMTP id\n\tk8mr36216606edv.233.1618985581424; \n\tTue, 20 Apr 2021 23:13:01 -0700 (PDT)","MIME-Version":"1.0","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-5-kieran.bingham@ideasonboard.com>\n\t<CAEmqJPqW1HPtw4cj-n4kQCjLgpETY2dt30n7mJytk8RToFiuDQ@mail.gmail.com>\n\t<ae85ed78-e9e2-bbb0-e677-9e689e745a64@ideasonboard.com>\n\t<YH9WtpwYyd0Xq6tU@pendragon.ideasonboard.com>","In-Reply-To":"<YH9WtpwYyd0Xq6tU@pendragon.ideasonboard.com>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Wed, 21 Apr 2021 15:12:50 +0900","Message-ID":"<CAO5uPHNNkg14ZM8g-LYnAW9H4wsQM6imc=u4ANBdsVgK9xBQhA@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16434,"web_url":"https://patchwork.libcamera.org/comment/16434/","msgid":"<CAEmqJPr=-je9G4cdwJzf8AXVJLokZRtnb=GuJq1+tU+D-5OFAA@mail.gmail.com>","date":"2021-04-21T08:58:07","subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent and Kieran,\n\n\nOn Tue, 20 Apr 2021 at 23:33, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Kieran,\n>\n> Thank you for the patch.\n>\n> On Tue, Apr 20, 2021 at 08:13:25PM +0100, Kieran Bingham wrote:\n> > On 20/04/2021 19:56, Naushir Patuck wrote:\n> > > On Tue, 20 Apr 2021 at 14:07, Kieran Bingham wrote:\n> > > >\n> > > > When the CIO2 returns a cancelled buffer, we will not queue buffers\n> > > > to the IMGU.\n> > > >\n> > > > These buffers should be explicitly marked as cancelled to ensure\n> > > > the application knows there is no valid metadata or frame data\n> > > > provided in the buffer.\n> > > >\n> > > > Provide a cancel() method on the FrameBuffer to allow explicitly\n> > > > cancelling a buffer.\n> > > >\n> > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > > ---\n> > > >  include/libcamera/buffer.h           | 2 ++\n> > > >  src/libcamera/buffer.cpp             | 8 ++++++++\n> > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n> > > >  3 files changed, 15 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > > > index 620f8a66f6a2..e0af00900409 100644\n> > > > --- a/include/libcamera/buffer.h\n> > > > +++ b/include/libcamera/buffer.h\n> > > > @@ -53,6 +53,8 @@ public:\n> > > >         unsigned int cookie() const { return cookie_; }\n> > > >         void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > > >\n> > > > +       void cancel() { metadata_.status =\n> FrameMetadata::FrameCancelled; }\n> > > > +\n> > > >  private:\n> > > >         LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> > > >\n> > > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > > > index 75b2693281a7..7635226b9045 100644\n> > > > --- a/src/libcamera/buffer.cpp\n> > > > +++ b/src/libcamera/buffer.cpp\n> > > > @@ -226,6 +226,14 @@ FrameBuffer::FrameBuffer(const\n> std::vector<Plane> &planes, unsigned int cookie)\n> > > >   * core never modifies the buffer cookie.\n> > > >   */\n> > > >\n> > > > +/**\n> > > > + * \\fn FrameBuffer::cancel()\n> > > > + * \\brief Marks the buffer as cancelled\n> > > > + *\n> > > > + * If a buffer is not used by a request, it shall be marked as\n> cancelled to\n> > > > + * indicate that the metadata is invalid.\n> > > > + */\n> > > > +\n> > > >  /**\n> > > >   * \\class MappedBuffer\n> > > >   * \\brief Provide an interface to support managing memory mapped\n> buffers\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index 51446fcf5bc1..73306cea6b37 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -1257,8 +1257,11 @@ void\n> IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > >\n> > > >         /* If the buffer is cancelled force a complete of the whole\n> request. */\n> > > >         if (buffer->metadata().status ==\n> FrameMetadata::FrameCancelled) {\n> > > > -               for (auto it : request->buffers())\n> > > > -                       pipe_->completeBuffer(request, it.second);\n> > > > +               for (auto it : request->buffers()) {\n> > > > +                       FrameBuffer *b = it.second;\n> > > > +                       b->cancel();\n> > > > +                       pipe_->completeBuffer(request, b);\n> > > > +               }\n> > >\n> > > Would you consider adding a Request::cancel() method to the API that\n> pipeline\n> > > handlers can invoke to essentially perform the above logic?  I found\n> it a bit\n> > > confusing having to call completeBuffer on a cancelled buffer, and\n> completeRequest\n> > > to complete cancelling the request if you see what I mean?  This is\n> probably a bit\n> > > more relevant to the Raspberry Pi pipeline handler where we queue up\n> requests\n> > > if required.\n> >\n> > I think this could be helpful sure, but maybe as a private/internal call\n> > only (not in the public API). That might be possible now that we have\n> > Request as an Extensible class later in this series.\n> >    (In fact, this Buffer->cancel() is public ... eep?)\n> >\n> > In this instance where the CIO2 (the receiver) has caused the\n> > cancellation, a simple helper makes sense to clear the request entirely\n> > and return it as we know that there are no other resources pending.\n> >\n> > But if this were on the completion path in the IMGU (the ISP), then I'd\n> > be concerned about races with multiple completion handlers trying to\n> > 'cancel' the request ...\n> >\n> > But if we can find ways to simplify handling of shutdown paths I'm all\n> > for it ;-)\n> >\n> > Any thoughts?\n>\n> A Request::cancel() function would make sense I believe. It's certainly\n> useful to cancel requests that have been put in an internal pipeline\n> handler queue, but whose buffers haven't been queued to the device. It\n> could also be used in this case I believe, as there should be no race.\n> If the pipeline handler had queued more than one buffer from the request\n> (including internal buffers) to the device, that would be a different\n> question. For instance, if we queued a raw buffer and an embedded data\n> buffer, they would complete (in the cancelled state) separately, in\n> unknown order, so we couldn't call Request::cancel() sefely in that\n> case.\n>\n\nMaybe I misunderstood the above, but I think you can tell in the above\nexample if the raw buffer had been completed as part of a request and\nthe embedded buffer had not.  This can be checked by looking at the\nFrameBuffer::request() value which gets nulled on a\nRequest::completeBuffer().\nThe code in our pipeline handler to deal with cancelling requests [1] does\nhave to deal with this, so I assume it is valid.\n\n[1]\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1482\n\nNaush\n\n\n>\n> We could start with a first implementation the limits usage of\n> Request::cancel() to the cases where no buffers or a single buffer has\n> been queued to the device, and improve on top, but I'd be interested in\n> hearing proposals about how this could be further improved.\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 88B45BDB15\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 21 Apr 2021 08:58:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3C3AC68806;\n\tWed, 21 Apr 2021 10:58:25 +0200 (CEST)","from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com\n\t[IPv6:2a00:1450:4864:20::12c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8ADCC602C8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 21 Apr 2021 10:58:23 +0200 (CEST)","by mail-lf1-x12c.google.com with SMTP id 12so65533036lfq.13\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 21 Apr 2021 01:58:23 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"PEii+CxO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=aa4x1ZYNAPjj2juqrz6dtIbf94Cx9v2aSy8qCgwkmsk=;\n\tb=PEii+CxOoNm8YW8g9wn+gdOpH6RoRWqwT00UZdZnbkNoRnFTlcnjMCDLGX5+Zb6lHL\n\tVWFErvdQSv0genMMqyvx0XKcFxD9lIsKaNNa5X3wsZqsujKK5ax3znCBd341kT/Jquzy\n\tLPkNn85QapD2hCCnbd8rHVHGxMX1fL/81KKPUxezBcm4JkEIq/x9JTohqDpXfCxx8NgK\n\tm1bfYX58150h+OBghgimg34G1+woBfsri87+oiSZBkEshW5VBUwtB4lXnfxft7C6kC80\n\tV617Xq7YJBFliiNSfEg1ZygqEqLeECZSsRX/6J/LP+iq0DbKT2vffd7OdG2ule+swcSk\n\t31Nw==","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=aa4x1ZYNAPjj2juqrz6dtIbf94Cx9v2aSy8qCgwkmsk=;\n\tb=WNKHvCB0WkXsTWdb1fOZxrQxXGhGJZ8HSnRdcBOnnnbq1WC7qNdu/YYj4hEKLiXyq3\n\thJwXL6+T/lFCLGqL+bh3l4yAI/bdgZr0nuea6BzJK0cC29UE1jiht8pfi9MiVA4uTh04\n\trXG0PwyPKlJgE+YCn9722oaAtSZWZSevTK789GOcjpT45cA2ueibbt74FFBoh5NQMk8K\n\tmInk8jE7Nd/fgMT/zK2l8uDgHYgtcFObUCqvgxGKPz2Xp8dm6etUpta2R98zDwG/3rWm\n\tBWlchHqOP5PjQca/9faiiXlu+HHft6ekTjXvJH3nf5XTW5jJNo63J2AlpnDdrSYQ9mdq\n\tZsEw==","X-Gm-Message-State":"AOAM532uywG7qCTDvz7RBxKmWyHztxSkJwqSg8JoQ4iV9qMKVYMUPUsf\n\tDwr3zdqoIZjY8JqXiShh9BcE77bQusPBuwZdwhFPUw==","X-Google-Smtp-Source":"ABdhPJwyVczfrnqDeCzW9BBPK+uDoX8u6OK9Mm5MfiZe02dih26s3/58ytHAXTyG7tDbgUFgAuXcdEYPjqSj3Xt90V4=","X-Received":"by 2002:a05:6512:b25:: with SMTP id\n\tw37mr19076223lfu.272.1618995502996; \n\tWed, 21 Apr 2021 01:58:22 -0700 (PDT)","MIME-Version":"1.0","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-5-kieran.bingham@ideasonboard.com>\n\t<CAEmqJPqW1HPtw4cj-n4kQCjLgpETY2dt30n7mJytk8RToFiuDQ@mail.gmail.com>\n\t<ae85ed78-e9e2-bbb0-e677-9e689e745a64@ideasonboard.com>\n\t<YH9WtpwYyd0Xq6tU@pendragon.ideasonboard.com>","In-Reply-To":"<YH9WtpwYyd0Xq6tU@pendragon.ideasonboard.com>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Wed, 21 Apr 2021 09:58:07 +0100","Message-ID":"<CAEmqJPr=-je9G4cdwJzf8AXVJLokZRtnb=GuJq1+tU+D-5OFAA@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"multipart/mixed;\n\tboundary=\"===============1176058719029102927==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16437,"web_url":"https://patchwork.libcamera.org/comment/16437/","msgid":"<YH/ryWUnuMbfoXR8@pendragon.ideasonboard.com>","date":"2021-04-21T09:09:29","subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, Apr 21, 2021 at 09:58:07AM +0100, Naushir Patuck wrote:\n> On Tue, 20 Apr 2021 at 23:33, Laurent Pinchart wrote:\n> > On Tue, Apr 20, 2021 at 08:13:25PM +0100, Kieran Bingham wrote:\n> > > On 20/04/2021 19:56, Naushir Patuck wrote:\n> > > > On Tue, 20 Apr 2021 at 14:07, Kieran Bingham wrote:\n> > > > >\n> > > > > When the CIO2 returns a cancelled buffer, we will not queue buffers\n> > > > > to the IMGU.\n> > > > >\n> > > > > These buffers should be explicitly marked as cancelled to ensure\n> > > > > the application knows there is no valid metadata or frame data\n> > > > > provided in the buffer.\n> > > > >\n> > > > > Provide a cancel() method on the FrameBuffer to allow explicitly\n> > > > > cancelling a buffer.\n> > > > >\n> > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > > > ---\n> > > > >  include/libcamera/buffer.h           | 2 ++\n> > > > >  src/libcamera/buffer.cpp             | 8 ++++++++\n> > > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n> > > > >  3 files changed, 15 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > > > > index 620f8a66f6a2..e0af00900409 100644\n> > > > > --- a/include/libcamera/buffer.h\n> > > > > +++ b/include/libcamera/buffer.h\n> > > > > @@ -53,6 +53,8 @@ public:\n> > > > >         unsigned int cookie() const { return cookie_; }\n> > > > >         void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > > > >\n> > > > > +       void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> > > > > +\n> > > > >  private:\n> > > > >         LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> > > > >\n> > > > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > > > > index 75b2693281a7..7635226b9045 100644\n> > > > > --- a/src/libcamera/buffer.cpp\n> > > > > +++ b/src/libcamera/buffer.cpp\n> > > > > @@ -226,6 +226,14 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)\n> > > > >   * core never modifies the buffer cookie.\n> > > > >   */\n> > > > >\n> > > > > +/**\n> > > > > + * \\fn FrameBuffer::cancel()\n> > > > > + * \\brief Marks the buffer as cancelled\n> > > > > + *\n> > > > > + * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > > > + * indicate that the metadata is invalid.\n> > > > > + */\n> > > > > +\n> > > > >  /**\n> > > > >   * \\class MappedBuffer\n> > > > >   * \\brief Provide an interface to support managing memory mapped buffers\n> > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > index 51446fcf5bc1..73306cea6b37 100644\n> > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > @@ -1257,8 +1257,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > > > >\n> > > > >         /* If the buffer is cancelled force a complete of the whole request. */\n> > > > >         if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > > -               for (auto it : request->buffers())\n> > > > > -                       pipe_->completeBuffer(request, it.second);\n> > > > > +               for (auto it : request->buffers()) {\n> > > > > +                       FrameBuffer *b = it.second;\n> > > > > +                       b->cancel();\n> > > > > +                       pipe_->completeBuffer(request, b);\n> > > > > +               }\n> > > >\n> > > > Would you consider adding a Request::cancel() method to the API that pipeline\n> > > > handlers can invoke to essentially perform the above logic?  I found it a bit\n> > > > confusing having to call completeBuffer on a cancelled buffer, and completeRequest\n> > > > to complete cancelling the request if you see what I mean?  This is probably a bit\n> > > > more relevant to the Raspberry Pi pipeline handler where we queue up requests\n> > > > if required.\n> > >\n> > > I think this could be helpful sure, but maybe as a private/internal call\n> > > only (not in the public API). That might be possible now that we have\n> > > Request as an Extensible class later in this series.\n> > >    (In fact, this Buffer->cancel() is public ... eep?)\n> > >\n> > > In this instance where the CIO2 (the receiver) has caused the\n> > > cancellation, a simple helper makes sense to clear the request entirely\n> > > and return it as we know that there are no other resources pending.\n> > >\n> > > But if this were on the completion path in the IMGU (the ISP), then I'd\n> > > be concerned about races with multiple completion handlers trying to\n> > > 'cancel' the request ...\n> > >\n> > > But if we can find ways to simplify handling of shutdown paths I'm all\n> > > for it ;-)\n> > >\n> > > Any thoughts?\n> >\n> > A Request::cancel() function would make sense I believe. It's certainly\n> > useful to cancel requests that have been put in an internal pipeline\n> > handler queue, but whose buffers haven't been queued to the device. It\n> > could also be used in this case I believe, as there should be no race.\n> > If the pipeline handler had queued more than one buffer from the request\n> > (including internal buffers) to the device, that would be a different\n> > question. For instance, if we queued a raw buffer and an embedded data\n> > buffer, they would complete (in the cancelled state) separately, in\n> > unknown order, so we couldn't call Request::cancel() sefely in that\n> > case.\n> \n> Maybe I misunderstood the above, but I think you can tell in the above\n> example if the raw buffer had been completed as part of a request and\n> the embedded buffer had not.  This can be checked by looking at the\n> FrameBuffer::request() value which gets nulled on a\n> Request::completeBuffer().\n> The code in our pipeline handler to deal with cancelling requests [1] does\n> have to deal with this, so I assume it is valid.\n\nMy point was that the following pseudo-code would be invalid:\n\nFoo::rawCompletionCallback(FrameBuffer *buffer)\n{\n\tif (buffer->metadata().status == Cancelled) {\n\t\tRequest *request = buffer->request();\n\t\trequest->cancel();\n\t\trequest->complete();\n\t\treturn;\n\t}\n\n\t...\n}\n\nFoo::embeddedCompletionCallback(FrameBuffer *buffer)\n{\n\tif (buffer->metadata().status == Cancelled) {\n\t\tRequest *request = buffer->request();\n\t\trequest->cancel();\n\t\trequest->complete();\n\t\treturn;\n\t}\n\n\t...\n}\n\nWe would call cancel() and complete() twice. We also can't have the\ncancellation handling in Foo::rawCompletionCallback() or\nFoo::embeddedCompletionCallback() only, as we don't control in which\norder those two will be called.\n\nIt's not a very complicated issue, and I'm sure we'll come up with a\nnice API to handle request completion with cancellation support. My\npoint is that it requires a bit of careful API design.\n\n> [1]\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1482\n> \n> > We could start with a first implementation the limits usage of\n> > Request::cancel() to the cases where no buffers or a single buffer has\n> > been queued to the device, and improve on top, but I'd be interested in\n> > hearing proposals about how this could be further improved.","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 4EB76BDB15\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 21 Apr 2021 09:09:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8D1CA68843;\n\tWed, 21 Apr 2021 11:09:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 77450602D1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 21 Apr 2021 11:09:34 +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 DD1943EE;\n\tWed, 21 Apr 2021 11:09:33 +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=\"tJhtyF5i\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618996174;\n\tbh=6/8Lg9+Qc1070QlhMzQdCt0HJexXMnmxL6acwIk6RZM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tJhtyF5irq9AJnRyKKqRm4v7BrwStskNxCUKJbSvX/cAVsgBd4KU/3tPbp/MNOW7j\n\tNtAEDbVFo2RPlDX6sSEz+nkz0VdH6JEg7i8UoFYHW6cYUVALVm9mT4B1Umk5+h/3Sz\n\tqKgFxbyQA5SjJbYiyNQ2rDJ816JtwD2oEamuDbog=","Date":"Wed, 21 Apr 2021 12:09:29 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<YH/ryWUnuMbfoXR8@pendragon.ideasonboard.com>","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-5-kieran.bingham@ideasonboard.com>\n\t<CAEmqJPqW1HPtw4cj-n4kQCjLgpETY2dt30n7mJytk8RToFiuDQ@mail.gmail.com>\n\t<ae85ed78-e9e2-bbb0-e677-9e689e745a64@ideasonboard.com>\n\t<YH9WtpwYyd0Xq6tU@pendragon.ideasonboard.com>\n\t<CAEmqJPr=-je9G4cdwJzf8AXVJLokZRtnb=GuJq1+tU+D-5OFAA@mail.gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<CAEmqJPr=-je9G4cdwJzf8AXVJLokZRtnb=GuJq1+tU+D-5OFAA@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16440,"web_url":"https://patchwork.libcamera.org/comment/16440/","msgid":"<CAEmqJPpGeV=0MULdfyn=Rwj=Z2SEDbX3-x8GUbpegYGd1vjMxg@mail.gmail.com>","date":"2021-04-21T09:20:27","subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Wed, 21 Apr 2021 at 10:09, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Wed, Apr 21, 2021 at 09:58:07AM +0100, Naushir Patuck wrote:\n> > On Tue, 20 Apr 2021 at 23:33, Laurent Pinchart wrote:\n> > > On Tue, Apr 20, 2021 at 08:13:25PM +0100, Kieran Bingham wrote:\n> > > > On 20/04/2021 19:56, Naushir Patuck wrote:\n> > > > > On Tue, 20 Apr 2021 at 14:07, Kieran Bingham wrote:\n> > > > > >\n> > > > > > When the CIO2 returns a cancelled buffer, we will not queue\n> buffers\n> > > > > > to the IMGU.\n> > > > > >\n> > > > > > These buffers should be explicitly marked as cancelled to ensure\n> > > > > > the application knows there is no valid metadata or frame data\n> > > > > > provided in the buffer.\n> > > > > >\n> > > > > > Provide a cancel() method on the FrameBuffer to allow explicitly\n> > > > > > cancelling a buffer.\n> > > > > >\n> > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > > > > ---\n> > > > > >  include/libcamera/buffer.h           | 2 ++\n> > > > > >  src/libcamera/buffer.cpp             | 8 ++++++++\n> > > > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n> > > > > >  3 files changed, 15 insertions(+), 2 deletions(-)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/buffer.h\n> b/include/libcamera/buffer.h\n> > > > > > index 620f8a66f6a2..e0af00900409 100644\n> > > > > > --- a/include/libcamera/buffer.h\n> > > > > > +++ b/include/libcamera/buffer.h\n> > > > > > @@ -53,6 +53,8 @@ public:\n> > > > > >         unsigned int cookie() const { return cookie_; }\n> > > > > >         void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > > > > >\n> > > > > > +       void cancel() { metadata_.status =\n> FrameMetadata::FrameCancelled; }\n> > > > > > +\n> > > > > >  private:\n> > > > > >         LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > > > > > index 75b2693281a7..7635226b9045 100644\n> > > > > > --- a/src/libcamera/buffer.cpp\n> > > > > > +++ b/src/libcamera/buffer.cpp\n> > > > > > @@ -226,6 +226,14 @@ FrameBuffer::FrameBuffer(const\n> std::vector<Plane> &planes, unsigned int cookie)\n> > > > > >   * core never modifies the buffer cookie.\n> > > > > >   */\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\fn FrameBuffer::cancel()\n> > > > > > + * \\brief Marks the buffer as cancelled\n> > > > > > + *\n> > > > > > + * If a buffer is not used by a request, it shall be marked as\n> cancelled to\n> > > > > > + * indicate that the metadata is invalid.\n> > > > > > + */\n> > > > > > +\n> > > > > >  /**\n> > > > > >   * \\class MappedBuffer\n> > > > > >   * \\brief Provide an interface to support managing memory\n> mapped buffers\n> > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > index 51446fcf5bc1..73306cea6b37 100644\n> > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > @@ -1257,8 +1257,11 @@ void\n> IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > > > > >\n> > > > > >         /* If the buffer is cancelled force a complete of the\n> whole request. */\n> > > > > >         if (buffer->metadata().status ==\n> FrameMetadata::FrameCancelled) {\n> > > > > > -               for (auto it : request->buffers())\n> > > > > > -                       pipe_->completeBuffer(request,\n> it.second);\n> > > > > > +               for (auto it : request->buffers()) {\n> > > > > > +                       FrameBuffer *b = it.second;\n> > > > > > +                       b->cancel();\n> > > > > > +                       pipe_->completeBuffer(request, b);\n> > > > > > +               }\n> > > > >\n> > > > > Would you consider adding a Request::cancel() method to the API\n> that pipeline\n> > > > > handlers can invoke to essentially perform the above logic?  I\n> found it a bit\n> > > > > confusing having to call completeBuffer on a cancelled buffer, and\n> completeRequest\n> > > > > to complete cancelling the request if you see what I mean?  This\n> is probably a bit\n> > > > > more relevant to the Raspberry Pi pipeline handler where we queue\n> up requests\n> > > > > if required.\n> > > >\n> > > > I think this could be helpful sure, but maybe as a private/internal\n> call\n> > > > only (not in the public API). That might be possible now that we have\n> > > > Request as an Extensible class later in this series.\n> > > >    (In fact, this Buffer->cancel() is public ... eep?)\n> > > >\n> > > > In this instance where the CIO2 (the receiver) has caused the\n> > > > cancellation, a simple helper makes sense to clear the request\n> entirely\n> > > > and return it as we know that there are no other resources pending.\n> > > >\n> > > > But if this were on the completion path in the IMGU (the ISP), then\n> I'd\n> > > > be concerned about races with multiple completion handlers trying to\n> > > > 'cancel' the request ...\n> > > >\n> > > > But if we can find ways to simplify handling of shutdown paths I'm\n> all\n> > > > for it ;-)\n> > > >\n> > > > Any thoughts?\n> > >\n> > > A Request::cancel() function would make sense I believe. It's certainly\n> > > useful to cancel requests that have been put in an internal pipeline\n> > > handler queue, but whose buffers haven't been queued to the device. It\n> > > could also be used in this case I believe, as there should be no race.\n> > > If the pipeline handler had queued more than one buffer from the\n> request\n> > > (including internal buffers) to the device, that would be a different\n> > > question. For instance, if we queued a raw buffer and an embedded data\n> > > buffer, they would complete (in the cancelled state) separately, in\n> > > unknown order, so we couldn't call Request::cancel() sefely in that\n> > > case.\n> >\n> > Maybe I misunderstood the above, but I think you can tell in the above\n> > example if the raw buffer had been completed as part of a request and\n> > the embedded buffer had not.  This can be checked by looking at the\n> > FrameBuffer::request() value which gets nulled on a\n> > Request::completeBuffer().\n> > The code in our pipeline handler to deal with cancelling requests [1]\n> does\n> > have to deal with this, so I assume it is valid.\n>\n> My point was that the following pseudo-code would be invalid:\n>\n> Foo::rawCompletionCallback(FrameBuffer *buffer)\n> {\n>         if (buffer->metadata().status == Cancelled) {\n>                 Request *request = buffer->request();\n>                 request->cancel();\n>                 request->complete();\n>                 return;\n>         }\n>\n>         ...\n> }\n>\n> Foo::embeddedCompletionCallback(FrameBuffer *buffer)\n> {\n>         if (buffer->metadata().status == Cancelled) {\n>                 Request *request = buffer->request();\n>                 request->cancel();\n>                 request->complete();\n>                 return;\n>         }\n>\n>         ...\n> }\n>\n> We would call cancel() and complete() twice. We also can't have the\n> cancellation handling in Foo::rawCompletionCallback() or\n> Foo::embeddedCompletionCallback() only, as we don't control in which\n> order those two will be called.\n>\n\nI got it now! The RPi PH logic is slightly different to the above (and\nprobably\nother PHs) in that there is a state-machine that checks for all buffers to\nbe complete before signalling request->complete() in only one place.\n\nNaush\n\n\n> It's not a very complicated issue, and I'm sure we'll come up with a\n> nice API to handle request completion with cancellation support. My\n> point is that it requires a bit of careful API design.\n>\n> > [1]\n> >\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1482\n> >\n> > > We could start with a first implementation the limits usage of\n> > > Request::cancel() to the cases where no buffers or a single buffer has\n> > > been queued to the device, and improve on top, but I'd be interested in\n> > > hearing proposals about how this could be further improved.\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 D402BBDB15\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 21 Apr 2021 09:20:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BEEBC68843;\n\tWed, 21 Apr 2021 11:20:45 +0200 (CEST)","from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com\n\t[IPv6:2a00:1450:4864:20::22e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 59664602D1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 21 Apr 2021 11:20:44 +0200 (CEST)","by mail-lj1-x22e.google.com with SMTP id o5so13824669ljc.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 21 Apr 2021 02:20:44 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"WjI+KQKp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=zaVUmmEMwEKIDOVZpNi0BANe94Bt8u0oXUWUMww6jkQ=;\n\tb=WjI+KQKpcmuwO2mzCUxby7JNHaID6BcoE+hOafraLK/dTlcbMKfV/kWjHXMOaWlfwH\n\tBx+UN6p+dPx4WjZdsSiuaw9cTQuU5yxiGhDsO6U4Xu4Gb2hf50yDNCKNQ5yec5eBzPyr\n\trAZfdMdMREw9G/9wd7414atFmZj0amyhWxDywcjRusKLaoz+JNt9rsV26JLx/xK5wJGh\n\t+ORfz0O8bGBicwipOfGOH1o3wpBc02RvYYTubuGlxRqQgFmARd01U08Gc+btEpPt1U5U\n\t7j5QWBVDmem6HtO3/GnfOZyrfNZiCIqJWzF9vmWy+B4eS06XqyHIG78bSWu7ZZyn68Zc\n\tlaaA==","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=zaVUmmEMwEKIDOVZpNi0BANe94Bt8u0oXUWUMww6jkQ=;\n\tb=BTwcdkDruVbGTQL25kBEpKWgvYvWcFAIlf98vXEUUFNUm+uff2iXlvS/TkJ4ddT+ZG\n\tFYnDH07fFte+bqKsHC4hAqT/WY7T6N93o4ppOIBG1gFi1Kqa6f8KQYZ/8cLRoRt9ZdYX\n\tk/CqV1BkBhQf07AlNxmueSJJAj4sXrmO8RxiH/tRaLTaSLReCaXw/oTTucvARfGiqGMi\n\tix3lD6IX2y6xDg4Ooat1xug+rg+cJixnXEtkmo/C4oY57SsyPeWPOyQm/Ctakq+228RG\n\tNJSH9uZURTo9JAI8pD8GBleXJnZ7TWpIL3XnQ+0QpHIEiEHQh7/IdFojawWPZKkZIfWt\n\tkO9w==","X-Gm-Message-State":"AOAM533zCd5cXz0Ez5aQvpx1gza/xqjZpFKB3wl8Z1IEGJx5j/4V2LZ5\n\tvUWT8pQ2KspdhN/XaOGMXKhk+jKt796/BfZqUXaTpw==","X-Google-Smtp-Source":"ABdhPJyboXYyY0j2vbceXWDpHLlkHXChWOcQ/QR5hwYFTytlUnqLXzkbbYa0kvWt6APWi4A0WRqITQvN+Xa+Tw7B1ng=","X-Received":"by 2002:a05:651c:1245:: with SMTP id\n\th5mr14211544ljh.499.1618996843095; \n\tWed, 21 Apr 2021 02:20:43 -0700 (PDT)","MIME-Version":"1.0","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-5-kieran.bingham@ideasonboard.com>\n\t<CAEmqJPqW1HPtw4cj-n4kQCjLgpETY2dt30n7mJytk8RToFiuDQ@mail.gmail.com>\n\t<ae85ed78-e9e2-bbb0-e677-9e689e745a64@ideasonboard.com>\n\t<YH9WtpwYyd0Xq6tU@pendragon.ideasonboard.com>\n\t<CAEmqJPr=-je9G4cdwJzf8AXVJLokZRtnb=GuJq1+tU+D-5OFAA@mail.gmail.com>\n\t<YH/ryWUnuMbfoXR8@pendragon.ideasonboard.com>","In-Reply-To":"<YH/ryWUnuMbfoXR8@pendragon.ideasonboard.com>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Wed, 21 Apr 2021 10:20:27 +0100","Message-ID":"<CAEmqJPpGeV=0MULdfyn=Rwj=Z2SEDbX3-x8GUbpegYGd1vjMxg@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"multipart/mixed;\n\tboundary=\"===============7561404825132902355==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16443,"web_url":"https://patchwork.libcamera.org/comment/16443/","msgid":"<YH/xgyq/xlCD5CTx@pendragon.ideasonboard.com>","date":"2021-04-21T09:33:55","subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, Apr 21, 2021 at 10:20:27AM +0100, Naushir Patuck wrote:\n> On Wed, 21 Apr 2021 at 10:09, Laurent Pinchart wrote:\n> > On Wed, Apr 21, 2021 at 09:58:07AM +0100, Naushir Patuck wrote:\n> > > On Tue, 20 Apr 2021 at 23:33, Laurent Pinchart wrote:\n> > > > On Tue, Apr 20, 2021 at 08:13:25PM +0100, Kieran Bingham wrote:\n> > > > > On 20/04/2021 19:56, Naushir Patuck wrote:\n> > > > > > On Tue, 20 Apr 2021 at 14:07, Kieran Bingham wrote:\n> > > > > > >\n> > > > > > > When the CIO2 returns a cancelled buffer, we will not queue buffers\n> > > > > > > to the IMGU.\n> > > > > > >\n> > > > > > > These buffers should be explicitly marked as cancelled to ensure\n> > > > > > > the application knows there is no valid metadata or frame data\n> > > > > > > provided in the buffer.\n> > > > > > >\n> > > > > > > Provide a cancel() method on the FrameBuffer to allow explicitly\n> > > > > > > cancelling a buffer.\n> > > > > > >\n> > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > > > > > ---\n> > > > > > >  include/libcamera/buffer.h           | 2 ++\n> > > > > > >  src/libcamera/buffer.cpp             | 8 ++++++++\n> > > > > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n> > > > > > >  3 files changed, 15 insertions(+), 2 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > > > > > > index 620f8a66f6a2..e0af00900409 100644\n> > > > > > > --- a/include/libcamera/buffer.h\n> > > > > > > +++ b/include/libcamera/buffer.h\n> > > > > > > @@ -53,6 +53,8 @@ public:\n> > > > > > >         unsigned int cookie() const { return cookie_; }\n> > > > > > >         void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > > > > > >\n> > > > > > > +       void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> > > > > > > +\n> > > > > > >  private:\n> > > > > > >         LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> > > > > > >\n> > > > > > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > > > > > > index 75b2693281a7..7635226b9045 100644\n> > > > > > > --- a/src/libcamera/buffer.cpp\n> > > > > > > +++ b/src/libcamera/buffer.cpp\n> > > > > > > @@ -226,6 +226,14 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)\n> > > > > > >   * core never modifies the buffer cookie.\n> > > > > > >   */\n> > > > > > >\n> > > > > > > +/**\n> > > > > > > + * \\fn FrameBuffer::cancel()\n> > > > > > > + * \\brief Marks the buffer as cancelled\n> > > > > > > + *\n> > > > > > > + * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > > > > > + * indicate that the metadata is invalid.\n> > > > > > > + */\n> > > > > > > +\n> > > > > > >  /**\n> > > > > > >   * \\class MappedBuffer\n> > > > > > >   * \\brief Provide an interface to support managing memory mapped buffers\n> > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > index 51446fcf5bc1..73306cea6b37 100644\n> > > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > @@ -1257,8 +1257,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > > > > > >\n> > > > > > >         /* If the buffer is cancelled force a complete of the whole request. */\n> > > > > > >         if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > > > > -               for (auto it : request->buffers())\n> > > > > > > -                       pipe_->completeBuffer(request, it.second);\n> > > > > > > +               for (auto it : request->buffers()) {\n> > > > > > > +                       FrameBuffer *b = it.second;\n> > > > > > > +                       b->cancel();\n> > > > > > > +                       pipe_->completeBuffer(request, b);\n> > > > > > > +               }\n> > > > > >\n> > > > > > Would you consider adding a Request::cancel() method to the API that pipeline\n> > > > > > handlers can invoke to essentially perform the above logic?  I found it a bit\n> > > > > > confusing having to call completeBuffer on a cancelled buffer, and completeRequest\n> > > > > > to complete cancelling the request if you see what I mean?  This is probably a bit\n> > > > > > more relevant to the Raspberry Pi pipeline handler where we queue up requests\n> > > > > > if required.\n> > > > >\n> > > > > I think this could be helpful sure, but maybe as a private/internal call\n> > > > > only (not in the public API). That might be possible now that we have\n> > > > > Request as an Extensible class later in this series.\n> > > > >    (In fact, this Buffer->cancel() is public ... eep?)\n> > > > >\n> > > > > In this instance where the CIO2 (the receiver) has caused the\n> > > > > cancellation, a simple helper makes sense to clear the request entirely\n> > > > > and return it as we know that there are no other resources pending.\n> > > > >\n> > > > > But if this were on the completion path in the IMGU (the ISP), then I'd\n> > > > > be concerned about races with multiple completion handlers trying to\n> > > > > 'cancel' the request ...\n> > > > >\n> > > > > But if we can find ways to simplify handling of shutdown paths I'm all\n> > > > > for it ;-)\n> > > > >\n> > > > > Any thoughts?\n> > > >\n> > > > A Request::cancel() function would make sense I believe. It's certainly\n> > > > useful to cancel requests that have been put in an internal pipeline\n> > > > handler queue, but whose buffers haven't been queued to the device. It\n> > > > could also be used in this case I believe, as there should be no race.\n> > > > If the pipeline handler had queued more than one buffer from the request\n> > > > (including internal buffers) to the device, that would be a different\n> > > > question. For instance, if we queued a raw buffer and an embedded data\n> > > > buffer, they would complete (in the cancelled state) separately, in\n> > > > unknown order, so we couldn't call Request::cancel() sefely in that\n> > > > case.\n> > >\n> > > Maybe I misunderstood the above, but I think you can tell in the above\n> > > example if the raw buffer had been completed as part of a request and\n> > > the embedded buffer had not.  This can be checked by looking at the\n> > > FrameBuffer::request() value which gets nulled on a\n> > > Request::completeBuffer().\n> > > The code in our pipeline handler to deal with cancelling requests [1] does\n> > > have to deal with this, so I assume it is valid.\n> >\n> > My point was that the following pseudo-code would be invalid:\n> >\n> > Foo::rawCompletionCallback(FrameBuffer *buffer)\n> > {\n> >         if (buffer->metadata().status == Cancelled) {\n> >                 Request *request = buffer->request();\n> >                 request->cancel();\n> >                 request->complete();\n> >                 return;\n> >         }\n> >\n> >         ...\n> > }\n> >\n> > Foo::embeddedCompletionCallback(FrameBuffer *buffer)\n> > {\n> >         if (buffer->metadata().status == Cancelled) {\n> >                 Request *request = buffer->request();\n> >                 request->cancel();\n> >                 request->complete();\n> >                 return;\n> >         }\n> >\n> >         ...\n> > }\n> >\n> > We would call cancel() and complete() twice. We also can't have the\n> > cancellation handling in Foo::rawCompletionCallback() or\n> > Foo::embeddedCompletionCallback() only, as we don't control in which\n> > order those two will be called.\n> \n> I got it now! The RPi PH logic is slightly different to the above (and probably\n> other PHs) in that there is a state-machine that checks for all buffers to\n> be complete before signalling request->complete() in only one place.\n\nNow that we have the same need implemented in different pipeline\nhandlers with different logics, I think we're reached the point where we\nhave enough data to refactor that and create common helpers. That's on\nmy todo list, and I don't think it should be a blocker to already\nimprove the request completion API for some use cases.\n\n> > It's not a very complicated issue, and I'm sure we'll come up with a\n> > nice API to handle request completion with cancellation support. My\n> > point is that it requires a bit of careful API design.\n> >\n> > > [1] https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1482\n> > >\n> > > > We could start with a first implementation the limits usage of\n> > > > Request::cancel() to the cases where no buffers or a single buffer has\n> > > > been queued to the device, and improve on top, but I'd be interested in\n> > > > hearing proposals about how this could be further improved.","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 0DC35BDB15\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 21 Apr 2021 09:34:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 508AD68844;\n\tWed, 21 Apr 2021 11:34:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0F11D602D1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 21 Apr 2021 11:34:00 +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 79B8F3EE;\n\tWed, 21 Apr 2021 11:33:59 +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=\"QH51VHPZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618997639;\n\tbh=RAVupPMfATc2Vd0isz8TWYXW2f2RYePl62j5QO46A8k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QH51VHPZDEbQ09xvQKGPAW4tywroNcatIlwjcbt+O+OkwBaF/mYQWrc5gskbyo2O6\n\tHyNpe5boj0FCsPeo137QMIHXdlDBHXNASRVaoKRr4ws8Sab1skF7Ne/03JLrcTDB9Z\n\tAiFhPVsypDD7+86jakHwOEBAJlryIxsDeqVbVeLU=","Date":"Wed, 21 Apr 2021 12:33:55 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<YH/xgyq/xlCD5CTx@pendragon.ideasonboard.com>","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-5-kieran.bingham@ideasonboard.com>\n\t<CAEmqJPqW1HPtw4cj-n4kQCjLgpETY2dt30n7mJytk8RToFiuDQ@mail.gmail.com>\n\t<ae85ed78-e9e2-bbb0-e677-9e689e745a64@ideasonboard.com>\n\t<YH9WtpwYyd0Xq6tU@pendragon.ideasonboard.com>\n\t<CAEmqJPr=-je9G4cdwJzf8AXVJLokZRtnb=GuJq1+tU+D-5OFAA@mail.gmail.com>\n\t<YH/ryWUnuMbfoXR8@pendragon.ideasonboard.com>\n\t<CAEmqJPpGeV=0MULdfyn=Rwj=Z2SEDbX3-x8GUbpegYGd1vjMxg@mail.gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<CAEmqJPpGeV=0MULdfyn=Rwj=Z2SEDbX3-x8GUbpegYGd1vjMxg@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3:\n\tCancel unused buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <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\" <libcamera-devel-bounces@lists.libcamera.org>"}}]