[{"id":22132,"web_url":"https://patchwork.libcamera.org/comment/22132/","msgid":"","date":"2022-02-07T23:36:05","subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: raspberrypi: Allow\n\tStream::returnBuffer() to handle internal buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Mon, Feb 07, 2022 at 03:12:12PM +0000, Naushir Patuck wrote:\n> If Stream::returnBuffer() gets passed an internally allocated buffer, it now\n> simply re-queues it back to the device. With this change, the pipeline handler\n> code can be simplified slightly as it does not need multiple code paths for\n> internally allocated and non-internally allocated buffers.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 53 +++++++++----------\n> .../pipeline/raspberrypi/rpi_stream.cpp | 7 ++-\n> 2 files changed, 29 insertions(+), 31 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 49af56edc1f9..0755de84c70c 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1877,34 +1877,29 @@ void RPiCameraData::clearIncompleteRequests()\n> \n> void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream)\n> {\n> -\tif (stream->isExternal()) {\n> +\t/*\n> +\t * It is possible to be here without a pending request, so check\n> +\t * that we actually have one to action, otherwise we just return\n> +\t * buffer back to the stream.\n> +\t */\n> +\tRequest *request = requestQueue_.empty() ? nullptr : requestQueue_.front();\n> +\tif (!dropFrameCount_ && request && request->findBuffer(stream) == buffer) {\n> \t\t/*\n> -\t\t * It is possible to be here without a pending request, so check\n> -\t\t * that we actually have one to action, otherwise we just return\n> -\t\t * buffer back to the stream.\n> +\t\t * Check if this is an externally provided buffer, and if\n> +\t\t * so, we must stop tracking it in the pipeline handler.\n> \t\t */\n> -\t\tRequest *request = requestQueue_.empty() ? nullptr : requestQueue_.front();\n> -\t\tif (!dropFrameCount_ && request && request->findBuffer(stream) == buffer) {\n> -\t\t\t/*\n> -\t\t\t * Check if this is an externally provided buffer, and if\n> -\t\t\t * so, we must stop tracking it in the pipeline handler.\n> -\t\t\t */\n> -\t\t\thandleExternalBuffer(buffer, stream);\n> -\t\t\t/*\n> -\t\t\t * Tag the buffer as completed, returning it to the\n> -\t\t\t * application.\n> -\t\t\t */\n> -\t\t\tpipe()->completeBuffer(request, buffer);\n> -\t\t} else {\n> -\t\t\t/*\n> -\t\t\t * This buffer was not part of the Request, or there is no\n> -\t\t\t * pending request, so we can recycle it.\n> -\t\t\t */\n> -\t\t\tstream->returnBuffer(buffer);\n> -\t\t}\n> +\t\thandleExternalBuffer(buffer, stream);\n> +\t\t/*\n> +\t\t * Tag the buffer as completed, returning it to the\n> +\t\t * application.\n> +\t\t */\n> +\t\tpipe()->completeBuffer(request, buffer);\n> \t} else {\n> -\t\t/* Simply re-queue the buffer to the requested stream. */\n> -\t\tstream->queueBuffer(buffer);\n> +\t\t/*\n> +\t\t * This buffer was not part of the Request, or there is no\n> +\t\t * pending request, so we can recycle it.\n\nAs buffer handling code isn't necessarily straightforward to follow, I'd\nexpand this to\n\n\t\t * This buffer was not part of the Request (which happens if an\n\t\t * internal buffer was used for an external stream, or\n\t\t * unconditionally for internal streams), or there is no pending\n\t\t * request, so we can recycle it.\n\nReviewed-by: Laurent Pinchart \n\n> +\t\t */\n> +\t\tstream->returnBuffer(buffer);\n> \t}\n> }\n> \n> @@ -2104,7 +2099,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em\n> \t\t\tFrameBuffer *b = embeddedQueue_.front();\n> \t\t\tif (!unicam_[Unicam::Embedded].isExternal() && b->metadata().timestamp < ts) {\n> \t\t\t\tembeddedQueue_.pop();\n> -\t\t\t\tunicam_[Unicam::Embedded].queueBuffer(b);\n> +\t\t\t\tunicam_[Unicam::Embedded].returnBuffer(b);\n> \t\t\t\tembeddedRequeueCount++;\n> \t\t\t\tLOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n> \t\t\t\t\t\t << unicam_[Unicam::Embedded].name();\n> @@ -2140,7 +2135,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em\n> \t\t\t\t * the front of the queue. This buffer is now orphaned, so requeue\n> \t\t\t\t * it back to the device.\n> \t\t\t\t */\n> -\t\t\t\tunicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);\n> +\t\t\t\tunicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer);\n> \t\t\t\tbayerQueue_.pop();\n> \t\t\t\tbayerRequeueCount++;\n> \t\t\t\tLOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n> @@ -2158,7 +2153,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em\n> \n> \t\t\t\tLOG(RPI, Warning) << \"Flushing bayer stream!\";\n> \t\t\t\twhile (!bayerQueue_.empty()) {\n> -\t\t\t\t\tunicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);\n> +\t\t\t\t\tunicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer);\n> \t\t\t\t\tbayerQueue_.pop();\n> \t\t\t\t}\n> \t\t\t\tflushedBuffers = true;\n> @@ -2175,7 +2170,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em\n> \n> \t\t\t\tLOG(RPI, Warning) << \"Flushing embedded data stream!\";\n> \t\t\t\twhile (!embeddedQueue_.empty()) {\n> -\t\t\t\t\tunicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> +\t\t\t\t\tunicam_[Unicam::Embedded].returnBuffer(embeddedQueue_.front());\n> \t\t\t\t\tembeddedQueue_.pop();\n> \t\t\t\t}\n> \t\t\t\tflushedBuffers = true;\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> index a421ad09ba50..f446e1ce66c7 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> @@ -163,8 +163,11 @@ int Stream::queueBuffer(FrameBuffer *buffer)\n> \n> void Stream::returnBuffer(FrameBuffer *buffer)\n> {\n> -\t/* This can only be called for external streams. */\n> -\tASSERT(external_);\n> +\tif (!external_) {\n> +\t\t/* For internal buffers, simply requeue back to the device. */\n> +\t\tqueueToDevice(buffer);\n> +\t\treturn;\n> +\t}\n> \n> \t/* Push this buffer back into the queue to be used again. */\n> \tavailableBuffers_.push(buffer);","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 F31B8BDCBF\n\tfor ;\n\tMon, 7 Feb 2022 23:36:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5540B609B9;\n\tTue, 8 Feb 2022 00:36:09 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DEC01609B9\n\tfor ;\n\tTue, 8 Feb 2022 00:36:07 +0100 (CET)","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 5AF3197;\n\tTue, 8 Feb 2022 00:36:07 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uxpQZDDl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1644276967;\n\tbh=49kLg2tNBzRVvH1MXqHgBDcifQ/sWc2/w5UdDTAg/0I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uxpQZDDl9tYvqZgJ8SZct53wPhKOF4m4D703BEmESOcWkWfkeI2hI5xLxg34ZAUMm\n\tyGCD6m1WyEpRiwBmqywBNNNE616lE4URx1z783mfgzt1ilJO1qzzIJ3UgiTvs1tNRB\n\twY2Eb0ev0kX25uYsEVou9r8TwblzOQK2PmTuT/j8=","Date":"Tue, 8 Feb 2022 01:36:05 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20220207151214.887140-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220207151214.887140-1-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: raspberrypi: Allow\n\tStream::returnBuffer() to handle internal buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22138,"web_url":"https://patchwork.libcamera.org/comment/22138/","msgid":"","date":"2022-02-08T08:25:05","subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: raspberrypi: Allow\n\tStream::returnBuffer() to handle internal buffers","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Mon, 7 Feb 2022 at 23:36, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Mon, Feb 07, 2022 at 03:12:12PM +0000, Naushir Patuck wrote:\n> > If Stream::returnBuffer() gets passed an internally allocated buffer, it\n> now\n> > simply re-queues it back to the device. With this change, the pipeline\n> handler\n> > code can be simplified slightly as it does not need multiple code paths\n> for\n> > internally allocated and non-internally allocated buffers.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 53 +++++++++----------\n> > .../pipeline/raspberrypi/rpi_stream.cpp | 7 ++-\n> > 2 files changed, 29 insertions(+), 31 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 49af56edc1f9..0755de84c70c 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1877,34 +1877,29 @@ void RPiCameraData::clearIncompleteRequests()\n> >\n> > void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream\n> *stream)\n> > {\n> > - if (stream->isExternal()) {\n> > + /*\n> > + * It is possible to be here without a pending request, so check\n> > + * that we actually have one to action, otherwise we just return\n> > + * buffer back to the stream.\n> > + */\n> > + Request *request = requestQueue_.empty() ? nullptr :\n> requestQueue_.front();\n> > + if (!dropFrameCount_ && request && request->findBuffer(stream) ==\n> buffer) {\n> > /*\n> > - * It is possible to be here without a pending request, so\n> check\n> > - * that we actually have one to action, otherwise we just\n> return\n> > - * buffer back to the stream.\n> > + * Check if this is an externally provided buffer, and if\n> > + * so, we must stop tracking it in the pipeline handler.\n> > */\n> > - Request *request = requestQueue_.empty() ? nullptr :\n> requestQueue_.front();\n> > - if (!dropFrameCount_ && request &&\n> request->findBuffer(stream) == buffer) {\n> > - /*\n> > - * Check if this is an externally provided buffer,\n> and if\n> > - * so, we must stop tracking it in the pipeline\n> handler.\n> > - */\n> > - handleExternalBuffer(buffer, stream);\n> > - /*\n> > - * Tag the buffer as completed, returning it to the\n> > - * application.\n> > - */\n> > - pipe()->completeBuffer(request, buffer);\n> > - } else {\n> > - /*\n> > - * This buffer was not part of the Request, or\n> there is no\n> > - * pending request, so we can recycle it.\n> > - */\n> > - stream->returnBuffer(buffer);\n> > - }\n> > + handleExternalBuffer(buffer, stream);\n> > + /*\n> > + * Tag the buffer as completed, returning it to the\n> > + * application.\n> > + */\n> > + pipe()->completeBuffer(request, buffer);\n> > } else {\n> > - /* Simply re-queue the buffer to the requested stream. */\n> > - stream->queueBuffer(buffer);\n> > + /*\n> > + * This buffer was not part of the Request, or there is no\n> > + * pending request, so we can recycle it.\n>\n> As buffer handling code isn't necessarily straightforward to follow, I'd\n> expand this to\n>\n> * This buffer was not part of the Request (which happens\n> if an\n> * internal buffer was used for an external stream, or\n> * unconditionally for internal streams), or there is no\n> pending\n> * request, so we can recycle it.\n>\n\nSure, I can change this. Or would it be possible to update before merging\nprovided there are no further changes needed?\n\nRegards,\nNaush\n\n\n> Reviewed-by: Laurent Pinchart \n>\n> > + */\n> > + stream->returnBuffer(buffer);\n> > }\n> > }\n> >\n> > @@ -2104,7 +2099,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame\n> &bayerFrame, FrameBuffer *&em\n> > FrameBuffer *b = embeddedQueue_.front();\n> > if (!unicam_[Unicam::Embedded].isExternal() &&\n> b->metadata().timestamp < ts) {\n> > embeddedQueue_.pop();\n> > - unicam_[Unicam::Embedded].queueBuffer(b);\n> > + unicam_[Unicam::Embedded].returnBuffer(b);\n> > embeddedRequeueCount++;\n> > LOG(RPI, Warning) << \"Dropping unmatched\n> input frame in stream \"\n> > <<\n> unicam_[Unicam::Embedded].name();\n> > @@ -2140,7 +2135,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame\n> &bayerFrame, FrameBuffer *&em\n> > * the front of the queue. This buffer is\n> now orphaned, so requeue\n> > * it back to the device.\n> > */\n> > -\n> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);\n> > +\n> unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer);\n> > bayerQueue_.pop();\n> > bayerRequeueCount++;\n> > LOG(RPI, Warning) << \"Dropping unmatched\n> input frame in stream \"\n> > @@ -2158,7 +2153,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame\n> &bayerFrame, FrameBuffer *&em\n> >\n> > LOG(RPI, Warning) << \"Flushing bayer\n> stream!\";\n> > while (!bayerQueue_.empty()) {\n> > -\n> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);\n> > +\n> unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer);\n> > bayerQueue_.pop();\n> > }\n> > flushedBuffers = true;\n> > @@ -2175,7 +2170,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame\n> &bayerFrame, FrameBuffer *&em\n> >\n> > LOG(RPI, Warning) << \"Flushing embedded\n> data stream!\";\n> > while (!embeddedQueue_.empty()) {\n> > -\n> unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> > +\n> unicam_[Unicam::Embedded].returnBuffer(embeddedQueue_.front());\n> > embeddedQueue_.pop();\n> > }\n> > flushedBuffers = true;\n> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > index a421ad09ba50..f446e1ce66c7 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > @@ -163,8 +163,11 @@ int Stream::queueBuffer(FrameBuffer *buffer)\n> >\n> > void Stream::returnBuffer(FrameBuffer *buffer)\n> > {\n> > - /* This can only be called for external streams. */\n> > - ASSERT(external_);\n> > + if (!external_) {\n> > + /* For internal buffers, simply requeue back to the\n> device. */\n> > + queueToDevice(buffer);\n> > + return;\n> > + }\n> >\n> > /* Push this buffer back into the queue to be used again. */\n> > availableBuffers_.push(buffer);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 32B59BDCBF\n\tfor ;\n\tTue, 8 Feb 2022 08:25:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E2CAE610AE;\n\tTue, 8 Feb 2022 09:25:24 +0100 (CET)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ADADA60E55\n\tfor ;\n\tTue, 8 Feb 2022 09:25:22 +0100 (CET)","by mail-lj1-x234.google.com with SMTP id t14so23318526ljh.8\n\tfor ;\n\tTue, 08 Feb 2022 00:25:22 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"lzB+DSQj\"; 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=by9ySEbiPlK+XlMdJTZwBZjDqF9vRCgQlQ1QQIx07f0=;\n\tb=lzB+DSQjLvlISjuYVP0Nlh9g48ugKbTn7DqodhqJhthsP9eLeR3MFYLvQFctrgzbzw\n\t7RU/OqZ106ix0/g31WTuO0mIZS45tDo1bNv0IsVUu2d2seuowLnTOKmPiQXMHAC4KavZ\n\tiE2736kCw3nnx9NxJf2V1mTp48BvpH9OIA9VQOHBw6jjgYD9xDONdZOJvuVSRjJ84n2M\n\tJzIpHQGxZWIK4wbo3H8ki4Cj6ky9uaI7B2Ljo/azCqPeldvdgCfdtbcD6QZo14VwlI4X\n\ty/iVzRrrQvZvrGDwY0JdNt94qBxoir6QQAjnw6mt6jrXlkn/pexwgsz+CEawbktQUcVp\n\tZEmg==","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=by9ySEbiPlK+XlMdJTZwBZjDqF9vRCgQlQ1QQIx07f0=;\n\tb=sdLe9iYJG1bEsd9oRiSHEfqAeooDUH6qA02HoeacOdsokiyv2rbZX9VIaW/GecF0Fw\n\tW6T59EU9jv/joxJmUeAuEZ/T1c/Zp5uRz0jTvsQG6QfXSUDfRS1jDijjUIzavK/c30fT\n\tguux/AHWJmTqD4w8cg/mvbWrhnkL9o9vFrP3lD2/NnjjugbOhgLYeX8nQiPym7jRmH+M\n\tvYnMxS82zROfJluU6DdH5OGvUoaIN3VGqi/akFfzw7sNPEkmNEy0nd6fhSY5j3jB5afs\n\tlNNQOMAF+FPrGDx3Pu33qxgl6wT80PdbZ16W2t+oZBB3vuBLBwgFnaLrd/ohWsze1EEr\n\t4Iag==","X-Gm-Message-State":"AOAM530M1zbh/9O3O00cbOHBH0VQMLWKZbmcBZpcpGiQyvEANoD+/GQ9\n\t+2rkAHBZlbc2UrWpTMRv05giWJNr3p4n00KngxUm/gFDDS8=","X-Google-Smtp-Source":"ABdhPJw6umfvEN3/PrY5tgTdFF5Ik4foKv1DWKpLEMG3My78aivecoTGTp1f8Clv22C0OIwV1CBcV2xdHwYaW53luIk=","X-Received":"by 2002:a2e:960a:: with SMTP id\n\tv10mr2197497ljh.208.1644308721873; \n\tTue, 08 Feb 2022 00:25:21 -0800 (PST)","MIME-Version":"1.0","References":"<20220207151214.887140-1-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 8 Feb 2022 08:25:05 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000044d8e005d77d75e4\"","Subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: raspberrypi: Allow\n\tStream::returnBuffer() to handle internal buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22143,"web_url":"https://patchwork.libcamera.org/comment/22143/","msgid":"","date":"2022-02-08T10:41:45","subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: raspberrypi: Allow\n\tStream::returnBuffer() to handle internal 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 Tue, Feb 08, 2022 at 08:25:05AM +0000, Naushir Patuck wrote:\n> On Mon, 7 Feb 2022 at 23:36, Laurent Pinchart wrote:\n> > On Mon, Feb 07, 2022 at 03:12:12PM +0000, Naushir Patuck wrote:\n> > > If Stream::returnBuffer() gets passed an internally allocated buffer, it now\n> > > simply re-queues it back to the device. With this change, the pipeline handler\n> > > code can be simplified slightly as it does not need multiple code paths for\n> > > internally allocated and non-internally allocated buffers.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 53 +++++++++----------\n> > > .../pipeline/raspberrypi/rpi_stream.cpp | 7 ++-\n> > > 2 files changed, 29 insertions(+), 31 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 49af56edc1f9..0755de84c70c 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1877,34 +1877,29 @@ void RPiCameraData::clearIncompleteRequests()\n> > >\n> > > void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream)\n> > > {\n> > > - if (stream->isExternal()) {\n> > > + /*\n> > > + * It is possible to be here without a pending request, so check\n> > > + * that we actually have one to action, otherwise we just return\n> > > + * buffer back to the stream.\n> > > + */\n> > > + Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front();\n> > > + if (!dropFrameCount_ && request && request->findBuffer(stream) == buffer) {\n> > > /*\n> > > - * It is possible to be here without a pending request, so check\n> > > - * that we actually have one to action, otherwise we just return\n> > > - * buffer back to the stream.\n> > > + * Check if this is an externally provided buffer, and if\n> > > + * so, we must stop tracking it in the pipeline handler.\n> > > */\n> > > - Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front();\n> > > - if (!dropFrameCount_ && request && request->findBuffer(stream) == buffer) {\n> > > - /*\n> > > - * Check if this is an externally provided buffer, and if\n> > > - * so, we must stop tracking it in the pipeline handler.\n> > > - */\n> > > - handleExternalBuffer(buffer, stream);\n> > > - /*\n> > > - * Tag the buffer as completed, returning it to the\n> > > - * application.\n> > > - */\n> > > - pipe()->completeBuffer(request, buffer);\n> > > - } else {\n> > > - /*\n> > > - * This buffer was not part of the Request, or there is no\n> > > - * pending request, so we can recycle it.\n> > > - */\n> > > - stream->returnBuffer(buffer);\n> > > - }\n> > > + handleExternalBuffer(buffer, stream);\n> > > + /*\n> > > + * Tag the buffer as completed, returning it to the\n> > > + * application.\n> > > + */\n> > > + pipe()->completeBuffer(request, buffer);\n> > > } else {\n> > > - /* Simply re-queue the buffer to the requested stream. */\n> > > - stream->queueBuffer(buffer);\n> > > + /*\n> > > + * This buffer was not part of the Request, or there is no\n> > > + * pending request, so we can recycle it.\n> >\n> > As buffer handling code isn't necessarily straightforward to follow, I'd\n> > expand this to\n> >\n> > * This buffer was not part of the Request (which happens if an\n> > * internal buffer was used for an external stream, or\n> > * unconditionally for internal streams), or there is no pending\n> > * request, so we can recycle it.\n> \n> Sure, I can change this. Or would it be possible to update before merging\n> provided there are no further changes needed?\n\nThe updated text is already in my tree :-)\n\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > + */\n> > > + stream->returnBuffer(buffer);\n> > > }\n> > > }\n> > >\n> > > @@ -2104,7 +2099,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em\n> > > FrameBuffer *b = embeddedQueue_.front();\n> > > if (!unicam_[Unicam::Embedded].isExternal() && b->metadata().timestamp < ts) {\n> > > embeddedQueue_.pop();\n> > > - unicam_[Unicam::Embedded].queueBuffer(b);\n> > > + unicam_[Unicam::Embedded].returnBuffer(b);\n> > > embeddedRequeueCount++;\n> > > LOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n> > > << unicam_[Unicam::Embedded].name();\n> > > @@ -2140,7 +2135,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em\n> > > * the front of the queue. This buffer is now orphaned, so requeue\n> > > * it back to the device.\n> > > */\n> > > - unicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);\n> > > + unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer);\n> > > bayerQueue_.pop();\n> > > bayerRequeueCount++;\n> > > LOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n> > > @@ -2158,7 +2153,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em\n> > >\n> > > LOG(RPI, Warning) << \"Flushing bayer stream!\";\n> > > while (!bayerQueue_.empty()) {\n> > > - unicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);\n> > > + unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer);\n> > > bayerQueue_.pop();\n> > > }\n> > > flushedBuffers = true;\n> > > @@ -2175,7 +2170,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em\n> > >\n> > > LOG(RPI, Warning) << \"Flushing embedded data stream!\";\n> > > while (!embeddedQueue_.empty()) {\n> > > - unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> > > + unicam_[Unicam::Embedded].returnBuffer(embeddedQueue_.front());\n> > > embeddedQueue_.pop();\n> > > }\n> > > flushedBuffers = true;\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > index a421ad09ba50..f446e1ce66c7 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > @@ -163,8 +163,11 @@ int Stream::queueBuffer(FrameBuffer *buffer)\n> > >\n> > > void Stream::returnBuffer(FrameBuffer *buffer)\n> > > {\n> > > - /* This can only be called for external streams. */\n> > > - ASSERT(external_);\n> > > + if (!external_) {\n> > > + /* For internal buffers, simply requeue back to the device. */\n> > > + queueToDevice(buffer);\n> > > + return;\n> > > + }\n> > >\n> > > /* Push this buffer back into the queue to be used again. */\n> > > availableBuffers_.push(buffer);","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 30F5DBF415\n\tfor ;\n\tTue, 8 Feb 2022 10:41:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92DD7610BF;\n\tTue, 8 Feb 2022 11:41:49 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A0395610AD\n\tfor ;\n\tTue, 8 Feb 2022 11:41:48 +0100 (CET)","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 242F6480;\n\tTue, 8 Feb 2022 11:41:48 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"L7+9ld0G\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1644316908;\n\tbh=LWm9Rb0odMZynbiUz1Fup9rTbzoOMKgI1rHNOcIYu5c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=L7+9ld0GwS2MzIxG/QO8/CAzVPFwDHaM/wS+i2K1y3svxN+PTCpUgElM3q5UeHF5M\n\t2LNu7FGmBKhytMi8rbDdnY+9H+j4tSppxIW6Tp/O8QwZGhyIT58q4gL4LpSZep2hvO\n\tW+Jqj8mWxh9ngu3deREPTbD4vBK+VZTi7/pg54yM=","Date":"Tue, 8 Feb 2022 12:41:45 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20220207151214.887140-1-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: raspberrypi: Allow\n\tStream::returnBuffer() to handle internal buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]