[{"id":22329,"web_url":"https://patchwork.libcamera.org/comment/22329/","msgid":"<164786349700.2130830.13966278381415607326@Monstersaurus>","date":"2022-03-21T11:51:37","subject":"Re: [libcamera-devel] [PATCH v3 4/5] pipeline: raspberrypi:\n\tRepurpose RPi::Stream::reset()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2022-03-21 10:27:01)\n> The original use of RPi::Stream::reset() was to clear the external flag state\n> and free/clear out the framebuffers for the stream. However, the latter is now\n> done through PipelineHandlerRPi::configure(). Rework\n> PipelineHandlerRPi::configure() to call RPi::Stream::setExternal() instead of\n> RPi::Stream::reset() to achieve the same thing.\n> \n> Repurpose RPi::Stream::reset() to instead reset the state of the buffer handling\n> logic, where all internally allocated buffers are put back into the queue of\n> available buffers. As such, rename the function to RPi::Stream::resetbuffers()\n> This resetbuffers() is now called from PipelineHandlerRPi::start(), allowing the\n\n/resetbuffers/resetBuffers/ twice above. But could be handled while\napplying.\n\n\n> pipeline handler to correctly deal with start()/stop()/start() sequences and\n> reusing the buffers allocated on the first start().\n> \n> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> Tested-by: David Plowman <david.plowman@raspberrypi.com>\n> ---\n>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  7 +++++--\n>  src/libcamera/pipeline/raspberrypi/rpi_stream.cpp  | 13 ++++++-------\n>  src/libcamera/pipeline/raspberrypi/rpi_stream.h    |  2 +-\n>  3 files changed, 12 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 92043962494b..0ff6acdceb66 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -693,7 +693,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n>         /* Start by freeing all buffers and reset the Unicam and ISP stream states. */\n>         data->freeBuffers();\n>         for (auto const stream : data->streams_)\n> -               stream->reset();\n> +               stream->setExternal(false);\n>  \n>         BayerFormat::Packing packing = BayerFormat::Packing::CSI2;\n>         Size maxSize, sensorSize;\n> @@ -991,6 +991,9 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n>         RPiCameraData *data = cameraData(camera);\n>         int ret;\n>  \n> +       for (auto const stream : data->streams_)\n> +               stream->resetBuffers();\n> +\n>         if (!data->buffersAllocated_) {\n>                 /* Allocate buffers for internal pipeline usage. */\n>                 ret = prepareBuffers(camera);\n> @@ -1031,7 +1034,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n>         data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n>  \n>         /*\n> -        * Reset the delayed controls with the gain and exposure values set by\n> +        * resetBuffers the delayed controls with the gain and exposure values set by\n\nPossibly a little too much of a match on the find/replace?\n\nCould easily be dropped while applying.\n\n\n>          * the IPA.\n>          */\n>         data->delayedCtrls_->reset();\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> index f446e1ce66c7..7a93efaa29da 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> @@ -26,10 +26,12 @@ std::string Stream::name() const\n>         return name_;\n>  }\n>  \n> -void Stream::reset()\n> +void Stream::resetBuffers()\n>  {\n> -       external_ = false;\n> -       clearBuffers();\n> +       /* Add all internal buffers to the queue of usable buffers. */\n> +       availableBuffers_ = {};\n\nI think this is ok, but I get a bit worried when I see blanket buffer\nstate changes like that.\n\n> +       for (auto const &buffer : internalBuffers_)\n> +               availableBuffers_.push(buffer.get());\n\nIs there any other action that is required to update the state of the\nbuffer if it was currently in use when this gets called? (Currently I\nthink the only call site is below so it's fine).\n\n\nWith the above fixes (that can be fixed while applying if suitable)\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n>  }\n>  \n>  void Stream::setExternal(bool external)\n> @@ -97,10 +99,7 @@ int Stream::prepareBuffers(unsigned int count)\n>  \n>                         /* Add these exported buffers to the internal/external buffer list. */\n>                         setExportedBuffers(&internalBuffers_);\n> -\n> -                       /* Add these buffers to the queue of internal usable buffers. */\n> -                       for (auto const &buffer : internalBuffers_)\n> -                               availableBuffers_.push(buffer.get());\n> +                       resetBuffers();\n>                 }\n>  \n>                 /* We must import all internal/external exported buffers. */\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> index d6f49d34f8c8..c37f7e82eef6 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> @@ -45,7 +45,7 @@ public:\n>         V4L2VideoDevice *dev() const;\n>         std::string name() const;\n>         bool isImporter() const;\n> -       void reset();\n> +       void resetBuffers();\n>  \n>         void setExternal(bool external);\n>         bool isExternal() const;\n> -- \n> 2.25.1\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 BDB54BDE17\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 21 Mar 2022 11:51:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4186604D4;\n\tMon, 21 Mar 2022 12:51:40 +0100 (CET)","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 49C7E604C6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 21 Mar 2022 12:51:39 +0100 (CET)","from pendragon.ideasonboard.com\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 D2A10291;\n\tMon, 21 Mar 2022 12:51:38 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647863500;\n\tbh=dzhSWoM2LpglAtx2mzp1fsaiaiZ2qEyWJu8KBRINYrM=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=TMpV306ED0x99Ouo4k/6XFjQW3HZsxs8fNb/B+vt0v9zYX5sXAoDoO6Mrp12gVQLD\n\tTe4fJRlv6pRprDHnfI9jB3PfJVMkRa/kRE/L0BX2/y653yYXHlC8T/R8FXE6MUYoAk\n\tM+7bEdAmjoaAWrNw9zAKZrjVz4huEvXi8plCBfPZl9pBFmzURD7SpSbnopu1Mm5/3g\n\tjoYJuJ0IQ8GOrZ5aHmcEdXTdgmjn/OUNk1rfnn/u7320QQ03zJlgXuUnq7z5/S0MIW\n\t6ouUGjwulkOBXf30l+Kc7XC5X1q/tDXNxmgFmJfQjQFSkOAShsrRtZLAqG26OdZKOp\n\tsvLle6L1P501A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1647863498;\n\tbh=dzhSWoM2LpglAtx2mzp1fsaiaiZ2qEyWJu8KBRINYrM=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=wi6grtcVUL5zaPSwiLo+TjHDI/pXKUsZ0zQ4J388i8ZX9CQZ8t+m9d+4wQ6nwf58T\n\ttsyWoLHjFmhEDG89a/ysZNAcj/LQ/EPylqsZOKoITL1qZ8vdji0hqUQ5XzbPCTj3LA\n\tFWap4Ku1ZcsxZyYUJ/DMqXJGA9IA4Tv0KNZn4+uk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"wi6grtcV\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220321102702.1753800-5-naush@raspberrypi.com>","References":"<20220321102702.1753800-1-naush@raspberrypi.com>\n\t<20220321102702.1753800-5-naush@raspberrypi.com>","To":"Naushir Patuck <naush@raspberrypi.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 21 Mar 2022 11:51:37 +0000","Message-ID":"<164786349700.2130830.13966278381415607326@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] pipeline: raspberrypi:\n\tRepurpose RPi::Stream::reset()","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>","From":"Kieran Bingham via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":22330,"web_url":"https://patchwork.libcamera.org/comment/22330/","msgid":"<164786372452.2130830.7010329288344259026@Monstersaurus>","date":"2022-03-21T11:55:24","subject":"Re: [libcamera-devel] [PATCH v3 4/5] pipeline: raspberrypi:\n\tRepurpose RPi::Stream::reset()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2022-03-21 11:51:37)\n> Quoting Naushir Patuck via libcamera-devel (2022-03-21 10:27:01)\n> > The original use of RPi::Stream::reset() was to clear the external flag state\n> > and free/clear out the framebuffers for the stream. However, the latter is now\n> > done through PipelineHandlerRPi::configure(). Rework\n> > PipelineHandlerRPi::configure() to call RPi::Stream::setExternal() instead of\n> > RPi::Stream::reset() to achieve the same thing.\n> > \n> > Repurpose RPi::Stream::reset() to instead reset the state of the buffer handling\n> > logic, where all internally allocated buffers are put back into the queue of\n> > available buffers. As such, rename the function to RPi::Stream::resetbuffers()\n> > This resetbuffers() is now called from PipelineHandlerRPi::start(), allowing the\n> \n> /resetbuffers/resetBuffers/ twice above. But could be handled while\n> applying.\n> \n> \n> > pipeline handler to correctly deal with start()/stop()/start() sequences and\n> > reusing the buffers allocated on the first start().\n> > \n> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> > Tested-by: David Plowman <david.plowman@raspberrypi.com>\n> > ---\n> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  7 +++++--\n> >  src/libcamera/pipeline/raspberrypi/rpi_stream.cpp  | 13 ++++++-------\n> >  src/libcamera/pipeline/raspberrypi/rpi_stream.h    |  2 +-\n> >  3 files changed, 12 insertions(+), 10 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 92043962494b..0ff6acdceb66 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -693,7 +693,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> >         /* Start by freeing all buffers and reset the Unicam and ISP stream states. */\n> >         data->freeBuffers();\n> >         for (auto const stream : data->streams_)\n> > -               stream->reset();\n> > +               stream->setExternal(false);\n> >  \n> >         BayerFormat::Packing packing = BayerFormat::Packing::CSI2;\n> >         Size maxSize, sensorSize;\n> > @@ -991,6 +991,9 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> >         RPiCameraData *data = cameraData(camera);\n> >         int ret;\n> >  \n> > +       for (auto const stream : data->streams_)\n> > +               stream->resetBuffers();\n> > +\n> >         if (!data->buffersAllocated_) {\n> >                 /* Allocate buffers for internal pipeline usage. */\n> >                 ret = prepareBuffers(camera);\n> > @@ -1031,7 +1034,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> >         data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n> >  \n> >         /*\n> > -        * Reset the delayed controls with the gain and exposure values set by\n> > +        * resetBuffers the delayed controls with the gain and exposure values set by\n> \n> Possibly a little too much of a match on the find/replace?\n> \n> Could easily be dropped while applying.\n> \n> \n> >          * the IPA.\n> >          */\n> >         data->delayedCtrls_->reset();\n> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > index f446e1ce66c7..7a93efaa29da 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > @@ -26,10 +26,12 @@ std::string Stream::name() const\n> >         return name_;\n> >  }\n> >  \n> > -void Stream::reset()\n> > +void Stream::resetBuffers()\n> >  {\n> > -       external_ = false;\n> > -       clearBuffers();\n> > +       /* Add all internal buffers to the queue of usable buffers. */\n> > +       availableBuffers_ = {};\n> \n> I think this is ok, but I get a bit worried when I see blanket buffer\n> state changes like that.\n> \n> > +       for (auto const &buffer : internalBuffers_)\n> > +               availableBuffers_.push(buffer.get());\n> \n> Is there any other action that is required to update the state of the\n> buffer if it was currently in use when this gets called? (Currently I\n> think the only call site is below so it's fine).\n\nAha, it's not the only call site, there's the one up in start() as well\nof course.\n\nSo at start() time, is there any possiblity that some buffer isn't\nalready in availableBuffers_ ? and if not ... why not ?\n\nIs there some worry about buffer state on calls to stop() not moving\nbuffers into the availableBuffers_ list?\n\n--\nKieran\n\n\n> With the above fixes (that can be fixed while applying if suitable)\n> \n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> >  }\n> >  \n> >  void Stream::setExternal(bool external)\n> > @@ -97,10 +99,7 @@ int Stream::prepareBuffers(unsigned int count)\n> >  \n> >                         /* Add these exported buffers to the internal/external buffer list. */\n> >                         setExportedBuffers(&internalBuffers_);\n> > -\n> > -                       /* Add these buffers to the queue of internal usable buffers. */\n> > -                       for (auto const &buffer : internalBuffers_)\n> > -                               availableBuffers_.push(buffer.get());\n> > +                       resetBuffers();\n> >                 }\n> >  \n> >                 /* We must import all internal/external exported buffers. */\n> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > index d6f49d34f8c8..c37f7e82eef6 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > @@ -45,7 +45,7 @@ public:\n> >         V4L2VideoDevice *dev() const;\n> >         std::string name() const;\n> >         bool isImporter() const;\n> > -       void reset();\n> > +       void resetBuffers();\n> >  \n> >         void setExternal(bool external);\n> >         bool isExternal() const;\n> > -- \n> > 2.25.1\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 62189BD80A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 21 Mar 2022 11:55:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5035604DC;\n\tMon, 21 Mar 2022 12:55:28 +0100 (CET)","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 A80FB604C6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 21 Mar 2022 12:55:27 +0100 (CET)","from pendragon.ideasonboard.com\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 3E46B291;\n\tMon, 21 Mar 2022 12:55:27 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647863728;\n\tbh=qPo5jb5I7Rf8PIi4LpHr8to29UQ27Fwmtyh1qKKGF2E=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=gt0Qeug56gdYIZZgyIW+8WI9QFkYNjtuXBNSl8lmIaIGrTSsh8dPRMRdQazt5DRGv\n\t8/k8Mvll2is31Hv5AvNCiZqWFqgjimPskakVbsBT7Bx1bmt8iA6A0JjhgW1/WvMrzK\n\t7ijvpKqyaifH+4lk/gf9AqqB/2N1rOy9xYf9kgPxo4c+1veswZywoggDpv8Igjg0d0\n\thYimi7GPW7NK1AE7x10NBDbUSZQBEAyKB+NqDnyec7gVZ+woBL5OonA5ImhUe4zCQQ\n\trid8g48g4wdAP0a2QaK8XXDPsv3Iup+INZl+FeJ22NbGNht9iUixyRQL5H5Gk3/nFe\n\t6IizuF0b4CoNw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1647863727;\n\tbh=qPo5jb5I7Rf8PIi4LpHr8to29UQ27Fwmtyh1qKKGF2E=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=ce578fb1kvBI9dZbeYe1GIyKeMelvpgOLyTY+NzulAS4NxAlU1MH1sZFE/Mr/EPhv\n\tiQNRhXGmOSKi1y9/zAwfsVAiKD+eBrX1CIxv0C8vr0af41HAddSmV0tzx0lftVJDvw\n\ttzBjGMouLYtVzC963qkG/9wor1f4PdFlqn82JFck="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ce578fb1\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<164786349700.2130830.13966278381415607326@Monstersaurus>","References":"<20220321102702.1753800-1-naush@raspberrypi.com>\n\t<20220321102702.1753800-5-naush@raspberrypi.com>\n\t<164786349700.2130830.13966278381415607326@Monstersaurus>","To":"Naushir Patuck <naush@raspberrypi.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 21 Mar 2022 11:55:24 +0000","Message-ID":"<164786372452.2130830.7010329288344259026@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] pipeline: raspberrypi:\n\tRepurpose RPi::Stream::reset()","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>","From":"Kieran Bingham via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":22331,"web_url":"https://patchwork.libcamera.org/comment/22331/","msgid":"<CAEmqJPqbSARWK0QBk+jXe58wp8WqLn+d1ZizjCy3SeY2hEqkXA@mail.gmail.com>","date":"2022-03-21T12:24:01","subject":"Re: [libcamera-devel] [PATCH v3 4/5] pipeline: raspberrypi:\n\tRepurpose RPi::Stream::reset()","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nOn Mon, 21 Mar 2022 at 11:55, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Kieran Bingham (2022-03-21 11:51:37)\n> > Quoting Naushir Patuck via libcamera-devel (2022-03-21 10:27:01)\n> > > The original use of RPi::Stream::reset() was to clear the external\n> flag state\n> > > and free/clear out the framebuffers for the stream. However, the\n> latter is now\n> > > done through PipelineHandlerRPi::configure(). Rework\n> > > PipelineHandlerRPi::configure() to call RPi::Stream::setExternal()\n> instead of\n> > > RPi::Stream::reset() to achieve the same thing.\n> > >\n> > > Repurpose RPi::Stream::reset() to instead reset the state of the\n> buffer handling\n> > > logic, where all internally allocated buffers are put back into the\n> queue of\n> > > available buffers. As such, rename the function to\n> RPi::Stream::resetbuffers()\n> > > This resetbuffers() is now called from PipelineHandlerRPi::start(),\n> allowing the\n> >\n> > /resetbuffers/resetBuffers/ twice above. But could be handled while\n> > applying.\n> >\n> >\n> > > pipeline handler to correctly deal with start()/stop()/start()\n> sequences and\n> > > reusing the buffers allocated on the first start().\n> > >\n> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> > > Tested-by: David Plowman <david.plowman@raspberrypi.com>\n> > > ---\n> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  7 +++++--\n> > >  src/libcamera/pipeline/raspberrypi/rpi_stream.cpp  | 13 ++++++-------\n> > >  src/libcamera/pipeline/raspberrypi/rpi_stream.h    |  2 +-\n> > >  3 files changed, 12 insertions(+), 10 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 92043962494b..0ff6acdceb66 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -693,7 +693,7 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> > >         /* Start by freeing all buffers and reset the Unicam and ISP\n> stream states. */\n> > >         data->freeBuffers();\n> > >         for (auto const stream : data->streams_)\n> > > -               stream->reset();\n> > > +               stream->setExternal(false);\n> > >\n> > >         BayerFormat::Packing packing = BayerFormat::Packing::CSI2;\n> > >         Size maxSize, sensorSize;\n> > > @@ -991,6 +991,9 @@ int PipelineHandlerRPi::start(Camera *camera,\n> const ControlList *controls)\n> > >         RPiCameraData *data = cameraData(camera);\n> > >         int ret;\n> > >\n> > > +       for (auto const stream : data->streams_)\n> > > +               stream->resetBuffers();\n> > > +\n> > >         if (!data->buffersAllocated_) {\n> > >                 /* Allocate buffers for internal pipeline usage. */\n> > >                 ret = prepareBuffers(camera);\n> > > @@ -1031,7 +1034,7 @@ int PipelineHandlerRPi::start(Camera *camera,\n> const ControlList *controls)\n> > >         data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n> > >\n> > >         /*\n> > > -        * Reset the delayed controls with the gain and exposure\n> values set by\n> > > +        * resetBuffers the delayed controls with the gain and\n> exposure values set by\n> >\n> > Possibly a little too much of a match on the find/replace?\n> >\n> > Could easily be dropped while applying.\n>\n\n:-(\n\nSorry, my brain is functioning like it's still Sunday this morning.  I'll\nfix this up and\npost an update.\n\n\n> >\n> >\n> > >          * the IPA.\n> > >          */\n> > >         data->delayedCtrls_->reset();\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > index f446e1ce66c7..7a93efaa29da 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > @@ -26,10 +26,12 @@ std::string Stream::name() const\n> > >         return name_;\n> > >  }\n> > >\n> > > -void Stream::reset()\n> > > +void Stream::resetBuffers()\n> > >  {\n> > > -       external_ = false;\n> > > -       clearBuffers();\n> > > +       /* Add all internal buffers to the queue of usable buffers. */\n> > > +       availableBuffers_ = {};\n> >\n> > I think this is ok, but I get a bit worried when I see blanket buffer\n> > state changes like that.\n> >\n> > > +       for (auto const &buffer : internalBuffers_)\n> > > +               availableBuffers_.push(buffer.get());\n> >\n> > Is there any other action that is required to update the state of the\n> > buffer if it was currently in use when this gets called? (Currently I\n> > think the only call site is below so it's fine).\n>\n> Aha, it's not the only call site, there's the one up in start() as well\n> of course.\n>\n> So at start() time, is there any possiblity that some buffer isn't\n> already in availableBuffers_ ? and if not ... why not ?\n>\n\nAt start time, all internal buffers would have been allocated, so any/every\nbuffer will be listed in  availableBuffers_.\n\n\n> Is there some worry about buffer state on calls to stop() not moving\n> buffers into the availableBuffers_ list?\n>\n\nI think this is ok, as the resetBuffers() call on start() will (as it says\non the tin)\nreset all buffer states and mark them as available again, if that makes\nsense.\n\nRegards,\nNaush\n\n\n>\n> --\n> Kieran\n>\n>\n> > With the above fixes (that can be fixed while applying if suitable)\n> >\n> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >\n> > >  }\n> > >\n> > >  void Stream::setExternal(bool external)\n> > > @@ -97,10 +99,7 @@ int Stream::prepareBuffers(unsigned int count)\n> > >\n> > >                         /* Add these exported buffers to the\n> internal/external buffer list. */\n> > >                         setExportedBuffers(&internalBuffers_);\n> > > -\n> > > -                       /* Add these buffers to the queue of internal\n> usable buffers. */\n> > > -                       for (auto const &buffer : internalBuffers_)\n> > > -                               availableBuffers_.push(buffer.get());\n> > > +                       resetBuffers();\n> > >                 }\n> > >\n> > >                 /* We must import all internal/external exported\n> buffers. */\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > index d6f49d34f8c8..c37f7e82eef6 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > @@ -45,7 +45,7 @@ public:\n> > >         V4L2VideoDevice *dev() const;\n> > >         std::string name() const;\n> > >         bool isImporter() const;\n> > > -       void reset();\n> > > +       void resetBuffers();\n> > >\n> > >         void setExternal(bool external);\n> > >         bool isExternal() const;\n> > > --\n> > > 2.25.1\n> > >\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 10A3FBDE17\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 21 Mar 2022 12:24:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 56E20604DC;\n\tMon, 21 Mar 2022 13:24:20 +0100 (CET)","from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com\n\t[IPv6:2a00:1450:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0E0B3604C6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 21 Mar 2022 13:24:18 +0100 (CET)","by mail-lj1-x22c.google.com with SMTP id 25so19549442ljv.10\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 21 Mar 2022 05:24:17 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647865460;\n\tbh=YElc8JC/SLEvRQnyVZ7esRiw36QdK+F1tl/k/FSAsFA=;\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=fzZclzKcTMNR5o5rrpYMFhNRKDZ60Bdkj3anqHpwAsn3B/ERY3YbxTVEZHY9r7GtW\n\tc3xG9VVeKcJYKyI45GHsBaW/NtYX71arPvXLEQ78MG3ofhjaETSZqzUSlVolUs+P7R\n\tO2F2Go8emu82zrYRUfg7RK2kD8F4Hr2pHtNRifQIh/hakq3yzIFidTB1oIybFlPHn/\n\t8Qqth9Fyxm5eS60NmPEnwl74CJAvHl1akpXQr997dwK2XthWIcwCWX2AIfresdKwO+\n\t3REMlEBsP/l0jHF4aMwSjwT7k8EoSQ+DFtj4QACMIJICCJ6WfdK/QpX1Mjx3BYeKJ2\n\tHRrXmdr0+NfVw==","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=L8n2YovQF9JDGd4cLnI2aUK3h1c+TiklMObP1tPvzSU=;\n\tb=karo/21XrXm02A44UzpbRiiAa5gqiN3n9sLNHOCFenY7XOLjE0Mkz3cgfkiXfLnrRC\n\t86Uw9RPhNbPXK/40Ddk9UsM2Gt+mfxc7MXRYE7jQ0xw45mLekfTyYuOoeyEBt5HzVmE4\n\t3XlKWGTVsBOA4WLpTPO6BfOmnaQIclfp3ETjfC+EtXU1tBr79Uu5EkNxbZu8NDjuzz7g\n\tSRsXWv0cf9nYZmIWvH8buvfn1QaDVAhnEwwoG8CqMJJadAg+okM+B+sLogj04IK7Fpj0\n\tQJ3OX/k7QyFdvb6lYrnwcNZ4TYWcLrzjPQ48aOdjaduVarUeFlMpbAJi9oTQBkfSt1PL\n\t+1LQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"karo/21X\"; 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=L8n2YovQF9JDGd4cLnI2aUK3h1c+TiklMObP1tPvzSU=;\n\tb=mGd9Xn0I3XWonzr3gg8kpM5+WPr0N4znX5tLRtNyxuyE9QHK8lPT8myJ+TV2JjQz/2\n\t7+JcnQVRqrCGvFBL3c2QPi4CV4tSWg3w7TFwEm/n6EKncsvAF0anC3B0BSGad7UsKU3w\n\tsRMQehPW/mtkr7GnNArLFka19tudmYzLz7crqtxEHAWUGTsPnoe3b3e+N7fkDm/cEaTH\n\tGKqRZxdAzuI+AHK6ORCNNALErtiDchdT2AVmgCf6uosSkA78Kh+0JWK01llmigVvQEyE\n\tIL6azRgqj57ESgHmMueLad0xpIIxp4DqsGclTCzb1uwy/BNT5VlowEknWZ7X23iUKElB\n\tbHUA==","X-Gm-Message-State":"AOAM531I5I5vdcAQesKsnvPTdoWEDcDR+U89WScT7a5Mm9SOYvB8EQm5\n\tYgjPiBMSnt9JTVExA3WT3USVqQDQ9E9zYGCNwhsZw0p6tB34gg==","X-Google-Smtp-Source":"ABdhPJyOx5lu50wdAGFF48eceNCwOJsu2vgKfeQviqpPp/N4Y2sEPrj4Zta5ZGBzLGX94aL9gNTAb59wKK7DuiAWQkk=","X-Received":"by 2002:a2e:6e0b:0:b0:247:f07d:ed69 with SMTP id\n\tj11-20020a2e6e0b000000b00247f07ded69mr15386773ljc.381.1647865457123;\n\tMon, 21 Mar 2022 05:24:17 -0700 (PDT)","MIME-Version":"1.0","References":"<20220321102702.1753800-1-naush@raspberrypi.com>\n\t<20220321102702.1753800-5-naush@raspberrypi.com>\n\t<164786349700.2130830.13966278381415607326@Monstersaurus>\n\t<164786372452.2130830.7010329288344259026@Monstersaurus>","In-Reply-To":"<164786372452.2130830.7010329288344259026@Monstersaurus>","Date":"Mon, 21 Mar 2022 12:24:01 +0000","Message-ID":"<CAEmqJPqbSARWK0QBk+jXe58wp8WqLn+d1ZizjCy3SeY2hEqkXA@mail.gmail.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Content-Type":"multipart/alternative; boundary=\"00000000000035b76905dab993cc\"","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] pipeline: raspberrypi:\n\tRepurpose RPi::Stream::reset()","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>","From":"Naushir Patuck via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Naushir Patuck <naush@raspberrypi.com>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":22332,"web_url":"https://patchwork.libcamera.org/comment/22332/","msgid":"<164786622180.1484799.12529566158222614191@Monstersaurus>","date":"2022-03-21T12:37:01","subject":"Re: [libcamera-devel] [PATCH v3 4/5] pipeline: raspberrypi:\n\tRepurpose RPi::Stream::reset()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2022-03-21 12:24:01)\n> Hi Kieran,\n> \n> On Mon, 21 Mar 2022 at 11:55, Kieran Bingham <\n> kieran.bingham@ideasonboard.com> wrote:\n> \n> > Quoting Kieran Bingham (2022-03-21 11:51:37)\n> > > Quoting Naushir Patuck via libcamera-devel (2022-03-21 10:27:01)\n> > > > The original use of RPi::Stream::reset() was to clear the external\n> > flag state\n> > > > and free/clear out the framebuffers for the stream. However, the\n> > latter is now\n> > > > done through PipelineHandlerRPi::configure(). Rework\n> > > > PipelineHandlerRPi::configure() to call RPi::Stream::setExternal()\n> > instead of\n> > > > RPi::Stream::reset() to achieve the same thing.\n> > > >\n> > > > Repurpose RPi::Stream::reset() to instead reset the state of the\n> > buffer handling\n> > > > logic, where all internally allocated buffers are put back into the\n> > queue of\n> > > > available buffers. As such, rename the function to\n> > RPi::Stream::resetbuffers()\n> > > > This resetbuffers() is now called from PipelineHandlerRPi::start(),\n> > allowing the\n> > >\n> > > /resetbuffers/resetBuffers/ twice above. But could be handled while\n> > > applying.\n> > >\n> > >\n> > > > pipeline handler to correctly deal with start()/stop()/start()\n> > sequences and\n> > > > reusing the buffers allocated on the first start().\n> > > >\n> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> > > > Tested-by: David Plowman <david.plowman@raspberrypi.com>\n> > > > ---\n> > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  7 +++++--\n> > > >  src/libcamera/pipeline/raspberrypi/rpi_stream.cpp  | 13 ++++++-------\n> > > >  src/libcamera/pipeline/raspberrypi/rpi_stream.h    |  2 +-\n> > > >  3 files changed, 12 insertions(+), 10 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 92043962494b..0ff6acdceb66 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -693,7 +693,7 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> > CameraConfiguration *config)\n> > > >         /* Start by freeing all buffers and reset the Unicam and ISP\n> > stream states. */\n> > > >         data->freeBuffers();\n> > > >         for (auto const stream : data->streams_)\n> > > > -               stream->reset();\n> > > > +               stream->setExternal(false);\n> > > >\n> > > >         BayerFormat::Packing packing = BayerFormat::Packing::CSI2;\n> > > >         Size maxSize, sensorSize;\n> > > > @@ -991,6 +991,9 @@ int PipelineHandlerRPi::start(Camera *camera,\n> > const ControlList *controls)\n> > > >         RPiCameraData *data = cameraData(camera);\n> > > >         int ret;\n> > > >\n> > > > +       for (auto const stream : data->streams_)\n> > > > +               stream->resetBuffers();\n> > > > +\n> > > >         if (!data->buffersAllocated_) {\n> > > >                 /* Allocate buffers for internal pipeline usage. */\n> > > >                 ret = prepareBuffers(camera);\n> > > > @@ -1031,7 +1034,7 @@ int PipelineHandlerRPi::start(Camera *camera,\n> > const ControlList *controls)\n> > > >         data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n> > > >\n> > > >         /*\n> > > > -        * Reset the delayed controls with the gain and exposure\n> > values set by\n> > > > +        * resetBuffers the delayed controls with the gain and\n> > exposure values set by\n> > >\n> > > Possibly a little too much of a match on the find/replace?\n> > >\n> > > Could easily be dropped while applying.\n> >\n> \n> :-(\n> \n> Sorry, my brain is functioning like it's still Sunday this morning.  I'll\n> fix this up and\n> post an update.\n> \n> \n> > >\n> > >\n> > > >          * the IPA.\n> > > >          */\n> > > >         data->delayedCtrls_->reset();\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > index f446e1ce66c7..7a93efaa29da 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > @@ -26,10 +26,12 @@ std::string Stream::name() const\n> > > >         return name_;\n> > > >  }\n> > > >\n> > > > -void Stream::reset()\n> > > > +void Stream::resetBuffers()\n> > > >  {\n> > > > -       external_ = false;\n> > > > -       clearBuffers();\n> > > > +       /* Add all internal buffers to the queue of usable buffers. */\n> > > > +       availableBuffers_ = {};\n> > >\n> > > I think this is ok, but I get a bit worried when I see blanket buffer\n> > > state changes like that.\n> > >\n> > > > +       for (auto const &buffer : internalBuffers_)\n> > > > +               availableBuffers_.push(buffer.get());\n> > >\n> > > Is there any other action that is required to update the state of the\n> > > buffer if it was currently in use when this gets called? (Currently I\n> > > think the only call site is below so it's fine).\n> >\n> > Aha, it's not the only call site, there's the one up in start() as well\n> > of course.\n> >\n> > So at start() time, is there any possiblity that some buffer isn't\n> > already in availableBuffers_ ? and if not ... why not ?\n> >\n> \n> At start time, all internal buffers would have been allocated, so any/every\n> buffer will be listed in  availableBuffers_.\n> \n> \n> > Is there some worry about buffer state on calls to stop() not moving\n> > buffers into the availableBuffers_ list?\n> >\n> \n> I think this is ok, as the resetBuffers() call on start() will (as it says\n> on the tin)\n> reset all buffer states and mark them as available again, if that makes\n> sense.\n\n\nYes, that makes sense, but I would call this the 'shotgun defence', and\nshoots everything instead of just the target required, so I wonder if\nthere is a corresponding ASSERT() (or non-fatal print) that is worth\nadding to catch a case when buffers were not in the expected state. The\nclear and re-add will put them all in the right list - but if that was\npreceeded by an unknown state, it can be worth noting it.\n\nNot essential though, it's only if this is a potential for identifying\nbuffer state leaks or otherwise catching when buffers didn't go through\nthe 'normal/expected' flow.\n\n--\nKieran\n\n\n> Regards,\n> Naush\n> \n> \n> >\n> > --\n> > Kieran\n> >\n> >\n> > > With the above fixes (that can be fixed while applying if suitable)\n> > >\n> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > >\n> > > >  }\n> > > >\n> > > >  void Stream::setExternal(bool external)\n> > > > @@ -97,10 +99,7 @@ int Stream::prepareBuffers(unsigned int count)\n> > > >\n> > > >                         /* Add these exported buffers to the\n> > internal/external buffer list. */\n> > > >                         setExportedBuffers(&internalBuffers_);\n> > > > -\n> > > > -                       /* Add these buffers to the queue of internal\n> > usable buffers. */\n> > > > -                       for (auto const &buffer : internalBuffers_)\n> > > > -                               availableBuffers_.push(buffer.get());\n> > > > +                       resetBuffers();\n> > > >                 }\n> > > >\n> > > >                 /* We must import all internal/external exported\n> > buffers. */\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > > index d6f49d34f8c8..c37f7e82eef6 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > > @@ -45,7 +45,7 @@ public:\n> > > >         V4L2VideoDevice *dev() const;\n> > > >         std::string name() const;\n> > > >         bool isImporter() const;\n> > > > -       void reset();\n> > > > +       void resetBuffers();\n> > > >\n> > > >         void setExternal(bool external);\n> > > >         bool isExternal() const;\n> > > > --\n> > > > 2.25.1\n> > > >\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 D09D0BD80A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 21 Mar 2022 12:37:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 275AE604DC;\n\tMon, 21 Mar 2022 13:37:06 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 879FA604C6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 21 Mar 2022 13:37:04 +0100 (CET)","from pendragon.ideasonboard.com\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 1F532D6E;\n\tMon, 21 Mar 2022 13:37:04 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647866226;\n\tbh=kM2vGNyTzk8O8v2hCcr7i3mWLnp4RMHbDtnlamb9l5Q=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=cbgmP+kvKtCw2GuW+xMtXFO2InwuBN/QNsLZmf7HosOQRth6UsuXkoP4sFJ9WOeZh\n\t6q96B3kyE6JP1RvTv7Xg3dbNT73EGxhwXDcL8QUWPAwl3LAvAcIgZh+WOasrbSkffn\n\t7H9n/3ZFaSrUDRYT6x0D/b0sAiCGFYDddoVBjU+KiWolvh8l0ZZC3M6mi+q7XYPfX4\n\tB82nGLvNutOLT/1tWeWeu/3KSkZlBF6Eaxfsl4/AjFDiaYfadduCIOvjWS8UC0JlZ/\n\tPGJHs4FNNfULPKNCcQdANf86E4+GsDoXJSu7yFzq1+DI/s0oq9QvQnAF+7Ld+omAc0\n\tcA/gAujl6Sr6g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1647866224;\n\tbh=kM2vGNyTzk8O8v2hCcr7i3mWLnp4RMHbDtnlamb9l5Q=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Y4AUPIfo25ahszEJ+UWOklKAIapxPj+gaR/wgAsqxV44yHkO2c76KCldo6ivhItf0\n\tB3dDqUQXeug9peIFjW05AQV+daBgBY2kmoYYrtmB3FCwdNy1q5oF4sxVd7VuSZxMvs\n\tD39ZDjqV4Z0MV1bHif6ihsWzDa2brSmKktVMqL24="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Y4AUPIfo\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<CAEmqJPqbSARWK0QBk+jXe58wp8WqLn+d1ZizjCy3SeY2hEqkXA@mail.gmail.com>","References":"<20220321102702.1753800-1-naush@raspberrypi.com>\n\t<20220321102702.1753800-5-naush@raspberrypi.com>\n\t<164786349700.2130830.13966278381415607326@Monstersaurus>\n\t<164786372452.2130830.7010329288344259026@Monstersaurus>\n\t<CAEmqJPqbSARWK0QBk+jXe58wp8WqLn+d1ZizjCy3SeY2hEqkXA@mail.gmail.com>","To":"Naushir Patuck <naush@raspberrypi.com>","Date":"Mon, 21 Mar 2022 12:37:01 +0000","Message-ID":"<164786622180.1484799.12529566158222614191@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] pipeline: raspberrypi:\n\tRepurpose RPi::Stream::reset()","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>","From":"Kieran Bingham via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]