[{"id":22320,"web_url":"https://patchwork.libcamera.org/comment/22320/","msgid":"","date":"2022-03-17T14:32:39","subject":"Re: [libcamera-devel] [PATCH v2 5/6] pipeline: raspberrypi: Add a\n\ttimeout to free buffers on stopDevice()","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Naush\n\nThanks for the patch!\n\nPerhaps I'm coming a bit late to this particular discussion, but this\nmakes me a bit nervous. I'm worried about deallocations/allocations\nhappening at unpredictable moments relative to other things in the\nsystem. Could there be a risk of creating more intermittent and\nhard-to-reproduce failures?\n\nOne example is the Pi 3 where I think we know that CMA fragmentation\nis already a problem. I wonder if I would rather be able to control\nwhen/whether buffers get freed? But it does make the API more involved\nand leaves an application with more to think about.\n\nPossibly I'm just worrying too much?\n\nThanks!\nDavid\n\nOn Thu, 17 Mar 2022 at 14:08, Naushir Patuck via libcamera-devel\n wrote:\n>\n> Start a 5 second timer when stopDevice() is called, and if it times out,\n> free all internally allocated buffers. The timer is cleared when start() is\n> subsequently called.\n>\n> This avoids the pipeline handler from holding onto internal buffers for long\n> periods of time due to application inactivity.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 17 +++++++++++++++++\n> 1 file changed, 17 insertions(+)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index d3212b193ced..32b282b4bdbd 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -15,6 +15,7 @@\n> #include \n>\n> #include \n> +#include \n> #include \n>\n> #include \n> @@ -45,6 +46,8 @@\n> #include \"dma_heaps.h\"\n> #include \"rpi_stream.h\"\n>\n> +using namespace std::literals::chrono_literals;\n> +\n> namespace libcamera {\n>\n> LOG_DEFINE_CATEGORY(RPI)\n> @@ -288,6 +291,9 @@ public:\n> /* Has this camera been reconfigured? */\n> bool reallocate_;\n>\n> + /* Timer to free internal buffers once stopDevice() has been called. */\n> + Timer stopTimer_;\n> +\n> private:\n> void checkRequestCompleted();\n> void fillRequestMetadata(const ControlList &bufferControls,\n> @@ -991,6 +997,10 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> RPiCameraData *data = cameraData(camera);\n> int ret;\n>\n> + /* Disable the stop timeout waiting to release buffers after a stop(). */\n> + data->stopTimer_.stop();\n> + data->stopTimer_.timeout.disconnect();\n> +\n> for (auto const stream : data->streams_)\n> stream->resetBuffers();\n>\n> @@ -1071,6 +1081,13 @@ void PipelineHandlerRPi::stopDevice(Camera *camera)\n>\n> /* Stop the IPA. */\n> data->ipa_->stop();\n> +\n> + /*\n> + * Start a 5 second timer once the camera is stopped, and on a timeout,\n> + * release all internally allocated buffers.\n> + */\n> + data->stopTimer_.timeout.connect(data, &RPiCameraData::freeBuffers);\n> + data->stopTimer_.start(5s);\n> }\n>\n> int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> --\n> 2.25.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 31DF0BDE17\n\tfor ;\n\tThu, 17 Mar 2022 14:32:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 56D67604E9;\n\tThu, 17 Mar 2022 15:32:51 +0100 (CET)","from mail-wm1-x334.google.com (mail-wm1-x334.google.com\n\t[IPv6:2a00:1450:4864:20::334])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8AA05601F8\n\tfor ;\n\tThu, 17 Mar 2022 15:32:50 +0100 (CET)","by mail-wm1-x334.google.com with SMTP id p26so2067986wma.3\n\tfor ;\n\tThu, 17 Mar 2022 07:32:50 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647527571;\n\tbh=iyhnIwOZOCevoK2mUb7kqKzB2e+/ZBQ3K54N/RC1xZk=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=S2gvHCoHXCuxuvOET+Hy7PkG6W0bzZDXHRvUNfJIF/rsq99uc9cdXekKlxboU40+1\n\tPmud/FdrcEtjKAwWzyJp3faG8kilJqoXg2VKxxSBxcz87AcwmUl6j9efPh/UZIPVcG\n\tuqYlNNebn/MXcGSDuhSLi3OD4IIGpSMCfk3kLhD98YotBcvxW0ATu28rSCae/H2qfM\n\tm/PdIP9FQEr5ahZ4JtH9fBAUquho/dmtP8b56EL/eWg6lIgVbsm90/tr5OsorBbQ6q\n\tkbOPRKeKFUF03uRKy6/+wNIKZ7oXt1c1Bx3dN7Akiix6FsX+FQpiEd7KVEajVYtlT4\n\tZB3vULtTGhJZA==","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=TFlkY6DyEO3BFuLV6Dfl9wcRgFTZFNryoeSpBNS43js=;\n\tb=sOCJth6/WhBMANVOG9e9M26Xdxx/QkfJUzmIs3y65dBFELNgeClt5l7vLl7kzPhQoJ\n\tugl6cmCY1UEexH8MwvFa2NeTNFuLm8YlWWWuRtxonrcSljnschCTGdoUQ0de8Iw5JwBe\n\t6KllZUjShG/eBxoYiK2SDSOnekcVihwhJmWHW2N/SOI5FR37miUG0eNM3lQYHuJ+nOpY\n\t4lhNpEi19sKbUNa45XohPlRlKninpS2Wz4R4tP4A+oY0nRuoUrjGwia0Ule9JToMSeKd\n\tF8bm5HDtU3GReCvI8s0I2lZO2dIHCLeVVqj+pm2ZsiQnaXp0lLgIgbzNneG7TvrjwC1k\n\twd0w=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"sOCJth6/\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=TFlkY6DyEO3BFuLV6Dfl9wcRgFTZFNryoeSpBNS43js=;\n\tb=vpWlFl4DBNtJyrdoKxrBe8i9J/85ujsGe0uCAS8ln5qfwqYjIlr33uhbnB4GD/te2H\n\t3QGK/RrxJMYR0bjDP/1Dq4HnQxKXERSAp5FrD8E7LjQUdmByVwNKpIhUETH8mhr4HP6Z\n\thHKKBQR7Eq+8V+YIW5kpWyM2rvt+MGWjF7dbOEoKNx1ZUcxrDLdhwSFHSCZwr8iUEUgM\n\tKyB58zJi1irVnnAu9XN58JJvpSKlMO28VgDI4KihEESiWCNdzIBBN6cJJ7+rf3/n/zmZ\n\tHbE0Rd5uDeWv1wsmBcseVhTncm85PqKSvX2/oMOO5+Tzxx69AvkUjfQKruvUvx7wb2xg\n\t+Duw==","X-Gm-Message-State":"AOAM53131blWsq523tiC/qYogSJCOTG6QPXjLEMLeex5oKKqJR1IThbv\n\tVhUulted/PNa4m0StPLQ8klAhDGA5TPC+bNfejAaD0eNyiM=","X-Google-Smtp-Source":"ABdhPJz/o7JlLa/IToB5YydiXaOseTcyATY1+5rGhVCDkugYx3NLYDTutoXRewbM4wccGuRRj6tAHiObO70fDvUm4V0=","X-Received":"by 2002:a05:600c:2249:b0:38c:8232:3004 with SMTP id\n\ta9-20020a05600c224900b0038c82323004mr1629157wmm.164.1647527570092;\n\tThu, 17 Mar 2022 07:32:50 -0700 (PDT)","MIME-Version":"1.0","References":"<20220317140827.1835029-1-naush@raspberrypi.com>\n\t<20220317140827.1835029-6-naush@raspberrypi.com>","In-Reply-To":"<20220317140827.1835029-6-naush@raspberrypi.com>","Date":"Thu, 17 Mar 2022 14:32:39 +0000","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] pipeline: raspberrypi: Add a\n\ttimeout to free buffers on stopDevice()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22321,"web_url":"https://patchwork.libcamera.org/comment/22321/","msgid":"","date":"2022-03-17T15:23:02","subject":"Re: [libcamera-devel] [PATCH v2 5/6] pipeline: raspberrypi: Add a\n\ttimeout to free buffers on stopDevice()","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi David,\n\nOn Thu, 17 Mar 2022 at 14:32, David Plowman \nwrote:\n\n> Hi Naush\n>\n> Thanks for the patch!\n>\n> Perhaps I'm coming a bit late to this particular discussion, but this\n> makes me a bit nervous. I'm worried about deallocations/allocations\n> happening at unpredictable moments relative to other things in the\n> system. Could there be a risk of creating more intermittent and\n> hard-to-reproduce failures?\n>\n\n> One example is the Pi 3 where I think we know that CMA fragmentation\n> is already a problem. I wonder if I would rather be able to control\n> when/whether buffers get freed? But it does make the API more involved\n> and leaves an application with more to think about.\n>\n\nYes, this is a distinct possibility. I'm fairly confident this will not be\nan issue with libcamera-apps,\nas the timeout will never hit. However, picamera2 can certainly invoke the\ntimeout and\ncause issues. Hmmm.... not sure which way to go here.... perhaps an\napplication flag\nwould indeed be better.\n\nNaush\n\n\n>\n> Possibly I'm just worrying too much?\n>\n> Thanks!\n> David\n>\n> On Thu, 17 Mar 2022 at 14:08, Naushir Patuck via libcamera-devel\n> wrote:\n> >\n> > Start a 5 second timer when stopDevice() is called, and if it times out,\n> > free all internally allocated buffers. The timer is cleared when start()\n> is\n> > subsequently called.\n> >\n> > This avoids the pipeline handler from holding onto internal buffers for\n> long\n> > periods of time due to application inactivity.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 17 +++++++++++++++++\n> > 1 file changed, 17 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index d3212b193ced..32b282b4bdbd 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -15,6 +15,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -45,6 +46,8 @@\n> > #include \"dma_heaps.h\"\n> > #include \"rpi_stream.h\"\n> >\n> > +using namespace std::literals::chrono_literals;\n> > +\n> > namespace libcamera {\n> >\n> > LOG_DEFINE_CATEGORY(RPI)\n> > @@ -288,6 +291,9 @@ public:\n> > /* Has this camera been reconfigured? */\n> > bool reallocate_;\n> >\n> > + /* Timer to free internal buffers once stopDevice() has been\n> called. */\n> > + Timer stopTimer_;\n> > +\n> > private:\n> > void checkRequestCompleted();\n> > void fillRequestMetadata(const ControlList &bufferControls,\n> > @@ -991,6 +997,10 @@ int PipelineHandlerRPi::start(Camera *camera, const\n> ControlList *controls)\n> > RPiCameraData *data = cameraData(camera);\n> > int ret;\n> >\n> > + /* Disable the stop timeout waiting to release buffers after a\n> stop(). */\n> > + data->stopTimer_.stop();\n> > + data->stopTimer_.timeout.disconnect();\n> > +\n> > for (auto const stream : data->streams_)\n> > stream->resetBuffers();\n> >\n> > @@ -1071,6 +1081,13 @@ void PipelineHandlerRPi::stopDevice(Camera\n> *camera)\n> >\n> > /* Stop the IPA. */\n> > data->ipa_->stop();\n> > +\n> > + /*\n> > + * Start a 5 second timer once the camera is stopped, and on a\n> timeout,\n> > + * release all internally allocated buffers.\n> > + */\n> > + data->stopTimer_.timeout.connect(data,\n> &RPiCameraData::freeBuffers);\n> > + data->stopTimer_.start(5s);\n> > }\n> >\n> > int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request\n> *request)\n> > --\n> > 2.25.1\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 25E65BD80A\n\tfor ;\n\tThu, 17 Mar 2022 15:23:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5A718610F8;\n\tThu, 17 Mar 2022 16:23:21 +0100 (CET)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D52D2601F8\n\tfor ;\n\tThu, 17 Mar 2022 16:23:19 +0100 (CET)","by mail-lj1-x236.google.com with SMTP id bn33so7676881ljb.6\n\tfor ;\n\tThu, 17 Mar 2022 08:23:19 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647530601;\n\tbh=ii1TJklGYIXkL6IfGmwQEy/mGpyBgpWaQ6UzdZyNnms=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=KVkixXfGrWOi8S36A/AAS1T4y9FdEuY0J7PUx3Kq3DSTZCkwnrNLK3+e8QQjYhiGo\n\tipuELyF1uvVwgwf0fr7xzPMdjfZIjEAJt4FPMdFrsKQSQbhW8blUMjnxc+U/cVojJj\n\tzqvx0XCxWe//bWJ7enY59fGNWen74f0USIyGtPzkOnxh/QUYpnSHUboJH/7FcIdi6v\n\tbe6Q3nMk2THzSgFtgihfdf3qahq4TaTktbIpgcK1pLvuIGuR7MTSY8xvXS9LoYL8pO\n\tUqf8b0Nz5ZNOFLXrVS+AnSOmj1VioCaApk/ciOsuW8y85/9Ajtih9Kb80BKS2admBc\n\tnqx2gEoG0HTWQ==","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=TyEY3oNY9rKlY8Zj4WERAz07u9OQGnxVIJ7XvOERASQ=;\n\tb=BJ30exBX8wg6WYIfFZaF+DQY9McBxWXBnnw70ZIxJE4qFfkwB6QzlqmaTS8csOc8pW\n\tKdEWRrgrAvk3MtHGNRSNYCkpqN+g2JwDzsNjm0zWVMESvzsYlijh50YKsCzd6in/Po06\n\tpyAVORTQa6KXRa3TD4FMqI/RwWhiKkNjeJ/GDy+yzAWRk3xwP9FQk/r5C6hQNNdWN6NA\n\tcTJH7Gwax2Mwd9JKxeVYw9p4ZAsyhKjT1POYyTR5+eC1W9NFArguWciD0ZNWq57QkW4N\n\t4MzsQXUmoJyc7te3B+6zugydjf5T1TX6SagVEUxfsY7K/REgZIerDdiKwHM5+0vBYLKb\n\tBR6A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"BJ30exBX\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=TyEY3oNY9rKlY8Zj4WERAz07u9OQGnxVIJ7XvOERASQ=;\n\tb=YsPwzgNTUIIddncHFuJUkKcZNUU0y36TQuxc8m7A/7qHUfO5oXu5d2Jxojmk0ct8kv\n\tmDfMIsAekx0XNQlz9qIAVPj1x5m2XmRTKPiFCoBcGogm35U0IPUWGec4MWOO2DRuxdkt\n\tpd2/jrr7rmkBk1aWoPLyXLYJodp0qwWE8DaB8GRv2NM7Td3L4oeunSdR3Wx6JB2LWmvD\n\tyis+LGqsaht4WzE4Y8ZhpauuYYpS5wHiMsMU+paXvX0P+WoA33Nzn5dK9XsZWN4vVv93\n\txhwYQV134wtBL5naI2qHDtv6ERxLbW2KKV1TdJWOeC9PNj79GfCQwB4RgeY9s6CqAijy\n\t5tvg==","X-Gm-Message-State":"AOAM532c/abr7RgIz5kt1VGQrDK76DzUDd7NaTIDZ/jztgL6u/+EeHDZ\n\tlKedTxMnXkVXG/N+HDMcKy3hMyWggP7OPEv54ICteu0M4i7IHA==","X-Google-Smtp-Source":"ABdhPJx8Qd3fTTMI/+8pR+JjRC5CJF56U55fAn5Kkf0RUEnFT9BNeaXT97VKfK14WBq18z75+ZsI9iY/P3NQPFHUWgw=","X-Received":"by 2002:a2e:a552:0:b0:249:34da:15e4 with SMTP id\n\te18-20020a2ea552000000b0024934da15e4mr3346786ljn.372.1647530598888;\n\tThu, 17 Mar 2022 08:23:18 -0700 (PDT)","MIME-Version":"1.0","References":"<20220317140827.1835029-1-naush@raspberrypi.com>\n\t<20220317140827.1835029-6-naush@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Thu, 17 Mar 2022 15:23:02 +0000","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"0000000000001a8aaa05da6b9ceb\"","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] pipeline: raspberrypi: Add a\n\ttimeout to free buffers on stopDevice()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]