[{"id":22255,"web_url":"https://patchwork.libcamera.org/comment/22255/","msgid":"","date":"2022-03-10T10:57:31","subject":"Re: [libcamera-devel] [PATCH v1 5/6] pipeline: raspberrypi:\n\tRepurpose RPi::Stream::reset()","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 this patch!\n\nOn Mon, 7 Mar 2022 at 12:46, Naushir Patuck via libcamera-devel\n wrote:\n>\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. This reset() is now called from PipelineHandlerRPi::start(),\n> allowing the pipeline handler to correctly deal with start()/stop()/start()\n> sequences and reusing the buffers allocated on the first start().\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 ++++-\n> src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 11 +++++------\n> 2 files changed, 9 insertions(+), 7 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index b17ffa235ac7..193361686d3c 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -692,7 +692,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> /* Start by freeing all buffers and resetting the Unicam and ISP stream states. */\n> data->freeBuffers();\n> for (auto const stream : data->streams_)\n> - stream->reset();\n> + stream->setExternal(false);\n\nSo I think that replacing the former calls to reset() by setExternal()\nis a good thing, because \"reset\" didn't give you much of a clue about\nwhat was being reset!\n\nI guess my only feeling about repurposing the name \"reset\" is that\nit's still rather vague. Could we think of a name that gives someone a\nclue as to what is being reset? Maybe \"resetAvailableBuffers\", or just\n\"resetBuffers\"?\n\n>\n> BayerFormat::Packing packing = BayerFormat::Packing::CSI2;\n> Size maxSize, sensorSize;\n> @@ -985,6 +985,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->reset();\n> +\n> if (data->reconfigured_) {\n> /* Allocate buffers for internal pipeline usage. */\n> ret = prepareBuffers(camera);\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> index f446e1ce66c7..0840ec4f54a0 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> @@ -28,8 +28,10 @@ std::string Stream::name() const\n>\n> void Stream::reset()\n> {\n> - external_ = false;\n> - clearBuffers();\n> + /* Add all internal buffers to the queue of usable buffers. */\n> + availableBuffers_ = {};\n> + for (auto const &buffer : internalBuffers_)\n> + availableBuffers_.push(buffer.get());\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> + reset();\n> }\n>\n> /* We must import all internal/external exported buffers. */\n> --\n> 2.25.1\n>\n\nBut either way, with or without any further changes:\n\nReviewed-by: David Plowman \nTested-by: David Plowman \n\nThanks!\nDavid","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 26C3CBE08A\n\tfor ;\n\tThu, 10 Mar 2022 10:57:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 56630632E3;\n\tThu, 10 Mar 2022 11:57:46 +0100 (CET)","from mail-wm1-x329.google.com (mail-wm1-x329.google.com\n\t[IPv6:2a00:1450:4864:20::329])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 50139601F8\n\tfor ;\n\tThu, 10 Mar 2022 11:57:44 +0100 (CET)","by mail-wm1-x329.google.com with SMTP id\n\ti9-20020a1c3b09000000b00389d0a5c511so789416wma.5\n\tfor ;\n\tThu, 10 Mar 2022 02:57:44 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1646909866;\n\tbh=pG20JXhsUpT4o5fPhAX5WfIYVkLL/5VnoX6D4ZQS/Tc=;\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=CExsPEqCIjWQHmN19bk0RmNLT2UXz9UNhCH+KE4AMLIvop+oS0kyjT3j+CwcZil06\n\t2MFDo7EBoTirfNg8Gk40S2hhdxJV/+h/7MHXP5MV8fPuJX4VObgTXmELpXVJE32IXR\n\td3f6XCKku+vf5JjX1AcVfFKb0sVVboTlYK1WMy70QKo2YfwFf05LA5TBg3yEGf/mbG\n\to2m+2AQcFpYmvSkR9TkcIRVc1UzN/1as79JxNZ7xVW229hcLWKapfmzbZ0f/j5Yrm2\n\t4xsY2xyn3hE6cxPm4ctn6nBLzT1j3PSXcu5xbhYDqP3Hv4mqXTzqUJXJbiWg0Zrj2M\n\ttNOPuX1Fg8o3w==","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=vYOdvVUqaL/4I+Zb1rEcr66Mz4MTcboVGyxCntUodU4=;\n\tb=nBQLP+oU1drFmKWwCPie7zOSmc35v/6pUZY4f9zkUduz9+kY3duFQSQwqjFiW+AJdi\n\tNDosOMfmxOyuc8zcbccsKRHpyzvdXYho5mJ9ogWJtjEZMqh7tH6zJJbU/6+v6SjEn+Sm\n\t9bY/4pKfXwwUrnzPBiTfWfA8H1ejdm8/2WgpSuaLLmEM3t6qCLo2KWMXpXvZQr+tCvR4\n\tYZpIiRabtp64Jxd42iBfvX2Fr5nvdXe9IUD8ZheLgfRr+lUW4YbwFON1JGePDDyyZ9ZO\n\tsuVXwEsJoaX77hQKf126pZTtvYs5X4WORHY+d+JMTQ8VR1iek5LSt0in2DG3GSJN6Yg7\n\tgTDw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"nBQLP+oU\"; 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=vYOdvVUqaL/4I+Zb1rEcr66Mz4MTcboVGyxCntUodU4=;\n\tb=5LmGvzUBcD6zqEed5kL+WPwmwOt8CQql8QNJDT8bgu2jNBL7pE/Oy45i1JIbVJjQ1w\n\tDA7uzProg09yi0r3coFsv0/eWpVMlrUZ8Q/Mq2f/2RMveWsmMy+M0PwdIgeg5T1awmdJ\n\t5hjRGGQs2bP6o8jji7jjtszbzDm7i7RGQW4u+1QTaFcz3CuPMfJKOcby6aAW6iz69Of5\n\tiQqSGykcEkC2nj9NHPwyai+72ER08A/6WO7wusSGAt6/Ipiy3pXUlict/Moj4s8WjNSc\n\tdCroGEQ90nO23TbfH5yOK+2M+/xoSLGKcCJj5BWapVLZOpgzoUh2n0bxsK9KSZNq5ZE4\n\tTULA==","X-Gm-Message-State":"AOAM5314M+7m9e/j09yd74V5auqunui5tAWe5SjdKRVCgZTRv3tiI8bB\n\tqs5cBuZPdOfSZKPLwcfy2aoZv+O/m4YEViF1m3Xdspqo3ZQ=","X-Google-Smtp-Source":"ABdhPJw0b9YgqFgvpVrVhaOVu0yyOVRVKpysf+TW/jRDe8KLygWmcV98a0WNkVGhnfNJLnfDcCo8FNihyx2YDk3Cl9w=","X-Received":"by 2002:a05:600c:3b13:b0:389:cf43:da5f with SMTP id\n\tm19-20020a05600c3b1300b00389cf43da5fmr6887983wms.201.1646909862696;\n\tThu, 10 Mar 2022 02:57:42 -0800 (PST)","MIME-Version":"1.0","References":"<20220307124633.115452-1-naush@raspberrypi.com>\n\t<20220307124633.115452-6-naush@raspberrypi.com>","In-Reply-To":"<20220307124633.115452-6-naush@raspberrypi.com>","Date":"Thu, 10 Mar 2022 10:57:31 +0000","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 5/6] pipeline: raspberrypi:\n\tRepurpose RPi::Stream::reset()","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":22257,"web_url":"https://patchwork.libcamera.org/comment/22257/","msgid":"","date":"2022-03-10T11:21:44","subject":"Re: [libcamera-devel] [PATCH v1 5/6] pipeline: raspberrypi:\n\tRepurpose RPi::Stream::reset()","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\nThank you for your review on this and the other patches in this series!\n\nOn Thu, 10 Mar 2022 at 10:57, David Plowman \nwrote:\n\n> Hi Naush\n>\n> Thanks for this patch!\n>\n> On Mon, 7 Mar 2022 at 12:46, Naushir Patuck via libcamera-devel\n> wrote:\n> >\n> > The original use of RPi::Stream::reset() was to clear the external flag\n> state\n> > and free/clear out the framebuffers for the stream. However, the latter\n> 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 buffer\n> handling\n> > logic, where all internally allocated buffers are put back into the\n> queue of\n> > available buffers. This reset() is now called from\n> PipelineHandlerRPi::start(),\n> > allowing the pipeline handler to correctly deal with\n> start()/stop()/start()\n> > sequences and reusing the buffers allocated on the first start().\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 ++++-\n> > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 11 +++++------\n> > 2 files changed, 9 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index b17ffa235ac7..193361686d3c 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -692,7 +692,7 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> > /* Start by freeing all buffers and resetting 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> So I think that replacing the former calls to reset() by setExternal()\n> is a good thing, because \"reset\" didn't give you much of a clue about\n> what was being reset!\n>\n> I guess my only feeling about repurposing the name \"reset\" is that\n> it's still rather vague. Could we think of a name that gives someone a\n> clue as to what is being reset? Maybe \"resetAvailableBuffers\", or just\n> \"resetBuffers\"?\n>\n\nSure I'll rename this to resetBuffers in v2 to be clearer of the new\nfunction.\n\nRegards,\nNaush\n\n\n>\n> >\n> > BayerFormat::Packing packing = BayerFormat::Packing::CSI2;\n> > Size maxSize, sensorSize;\n> > @@ -985,6 +985,9 @@ int PipelineHandlerRPi::start(Camera *camera, const\n> ControlList *controls)\n> > RPiCameraData *data = cameraData(camera);\n> > int ret;\n> >\n> > + for (auto const stream : data->streams_)\n> > + stream->reset();\n> > +\n> > if (data->reconfigured_) {\n> > /* Allocate buffers for internal pipeline usage. */\n> > ret = prepareBuffers(camera);\n> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > index f446e1ce66c7..0840ec4f54a0 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > @@ -28,8 +28,10 @@ std::string Stream::name() const\n> >\n> > void Stream::reset()\n> > {\n> > - external_ = false;\n> > - clearBuffers();\n> > + /* Add all internal buffers to the queue of usable buffers. */\n> > + availableBuffers_ = {};\n> > + for (auto const &buffer : internalBuffers_)\n> > + availableBuffers_.push(buffer.get());\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> > + reset();\n> > }\n> >\n> > /* We must import all internal/external exported\n> buffers. */\n> > --\n> > 2.25.1\n> >\n>\n> But either way, with or without any further changes:\n>\n> Reviewed-by: David Plowman \n> Tested-by: David Plowman \n>\n> Thanks!\n> David\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 35B89BF415\n\tfor ;\n\tThu, 10 Mar 2022 11:22:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 70EB4632E2;\n\tThu, 10 Mar 2022 12:22:02 +0100 (CET)","from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B949601F5\n\tfor ;\n\tThu, 10 Mar 2022 12:22:01 +0100 (CET)","by mail-lj1-x233.google.com with SMTP id r22so7226055ljd.4\n\tfor ;\n\tThu, 10 Mar 2022 03:22:01 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1646911322;\n\tbh=mltnkLhiV8CXY4hJ3uxtumr+iOarnG6XPkImx5IHFHs=;\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=CnBBAR00v0wzR9kK7GFIcePddmfeSjixMwKHikyBG9Tor/+0k7EkLEpxBRZ7rFPje\n\txxnDtQxCvUDOPZlT3p8gNjFm7BQBB58Q2N5JWJSBouPbhWFy06qIwhDyLB4DiUiVuN\n\t9aWHXkD0Z1D0coIaH1kzNjOW1fu5MpexTZSDXOj9GTREP1kiqE/IbhOkfF+6bHl4KP\n\txDn+UEmEaJV5ckbgy3nSZa6XPez84K2liDRhDzdK6r0f56LqZHJKl1dF9XsVsfhr+7\n\t7TFAHPrIW/AwgGnGXK4FK4tcWYsJ/dS7yJvaDheh3wiwLUwEbzDfwG9YBrwrKqUQjr\n\tgYeNrZmDjE1JQ==","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=NHvH1s9ptEGFAKo+ZDVolhWJOno3HuHPldMNiwfLUdU=;\n\tb=m4pWaktF74o2NEfX6AvUeQeYD1kOwN50mDC/se64CUk4MYXdzoipMsr7m3NcM+/11E\n\ty1i66YXFzjkxtWyXgAiNFPVwmzwuJBb/xXJDQnToYFkADF4/RvAziJeA48ZF+i080S/F\n\tuOYeZ5KP4X+Ngn2z7VSL91gVzVKuNWUAVqrYIb6dbrvKb5PDSA/Se+mPxwtxMBla69b0\n\tdAxxjvocpaoWawOvtjBbRYGSd5c79IykHDg/9BZ8VrrVo69to0nOuFfvSOfcaQnl/UZj\n\tzWuy/NVgUO462bP6Kgv74fW5kXNqoaKcjPu1vwSVHh0Vz7dzAQyoiu6ZZc+jYkIzoy7a\n\t1xFA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"m4pWaktF\"; 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=NHvH1s9ptEGFAKo+ZDVolhWJOno3HuHPldMNiwfLUdU=;\n\tb=aPSxQXIYNI+XM5rnDitQbNJcCVoEF76wsz3pPgFYxmZLe/MhIjSG0XPbEXgu4AgefN\n\tFB3PuN4wAJ5KY62ykjjcP+GFO7yxlC8TsJAJus0gmfMmfm6JvRm1nb9A5nnHWk+vZYdX\n\teR1n91HlWkJiLQ3E52n1O8OJk7wI1zY2DZ8ix1J4wNXF4LPfQZwsMMlm5dg7UvylWELM\n\tUMR3VxRpOCCVJchv0pnU1plc0Q+4FbEy+Gnvjyrdr/4GgiBdaVsjRKMsntJgMk0TqSQF\n\tdz0SjFNll3Bxa62jxnM0oTTe9h/vn1XVgJjIoJoYgvCij7oIm3ohy1LHOPUtByxzJhol\n\tCx7Q==","X-Gm-Message-State":"AOAM532oVFRYIPuJenH3JAcAKVot/xdpOByhauY1BdvBujzRT1Uwt4Wv\n\t5gt8gYMGZvtYs3wnH8gaMttj6IKCho8yaxyZIT+6Cg==","X-Google-Smtp-Source":"ABdhPJzIS0d34RNVWgejNprAGKWqxzT4EBMWkkGM7xn91HT3LqLM+1j9fxgKT4R+Wsp78IZSv+iYb7tQQ5ybtgpk0Mg=","X-Received":"by 2002:a05:651c:b10:b0:247:f37f:f595 with SMTP id\n\tb16-20020a05651c0b1000b00247f37ff595mr2692596ljr.444.1646911320525;\n\tThu, 10 Mar 2022 03:22:00 -0800 (PST)","MIME-Version":"1.0","References":"<20220307124633.115452-1-naush@raspberrypi.com>\n\t<20220307124633.115452-6-naush@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Thu, 10 Mar 2022 11:21:44 +0000","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"0000000000003ca48a05d9db6c1f\"","Subject":"Re: [libcamera-devel] [PATCH v1 5/6] pipeline: raspberrypi:\n\tRepurpose RPi::Stream::reset()","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\" "}}]