<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 7 Feb 2022 at 23:36, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Mon, Feb 07, 2022 at 03:12:12PM +0000, Naushir Patuck wrote:<br>
> If Stream::returnBuffer() gets passed an internally allocated buffer, it now<br>
> simply re-queues it back to the device. With this change, the pipeline handler<br>
> code can be simplified slightly as it does not need multiple code paths for<br>
> internally allocated and non-internally allocated buffers.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 53 +++++++++----------<br>
> .../pipeline/raspberrypi/rpi_stream.cpp | 7 ++-<br>
> 2 files changed, 29 insertions(+), 31 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 49af56edc1f9..0755de84c70c 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1877,34 +1877,29 @@ void RPiCameraData::clearIncompleteRequests()<br>
> <br>
> void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream)<br>
> {<br>
> - if (stream->isExternal()) {<br>
> + /*<br>
> + * It is possible to be here without a pending request, so check<br>
> + * that we actually have one to action, otherwise we just return<br>
> + * buffer back to the stream.<br>
> + */<br>
> + Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front();<br>
> + if (!dropFrameCount_ && request && request->findBuffer(stream) == buffer) {<br>
> /*<br>
> - * It is possible to be here without a pending request, so check<br>
> - * that we actually have one to action, otherwise we just return<br>
> - * buffer back to the stream.<br>
> + * Check if this is an externally provided buffer, and if<br>
> + * so, we must stop tracking it in the pipeline handler.<br>
> */<br>
> - Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front();<br>
> - if (!dropFrameCount_ && request && request->findBuffer(stream) == buffer) {<br>
> - /*<br>
> - * Check if this is an externally provided buffer, and if<br>
> - * so, we must stop tracking it in the pipeline handler.<br>
> - */<br>
> - handleExternalBuffer(buffer, stream);<br>
> - /*<br>
> - * Tag the buffer as completed, returning it to the<br>
> - * application.<br>
> - */<br>
> - pipe()->completeBuffer(request, buffer);<br>
> - } else {<br>
> - /*<br>
> - * This buffer was not part of the Request, or there is no<br>
> - * pending request, so we can recycle it.<br>
> - */<br>
> - stream->returnBuffer(buffer);<br>
> - }<br>
> + handleExternalBuffer(buffer, stream);<br>
> + /*<br>
> + * Tag the buffer as completed, returning it to the<br>
> + * application.<br>
> + */<br>
> + pipe()->completeBuffer(request, buffer);<br>
> } else {<br>
> - /* Simply re-queue the buffer to the requested stream. */<br>
> - stream->queueBuffer(buffer);<br>
> + /*<br>
> + * This buffer was not part of the Request, or there is no<br>
> + * pending request, so we can recycle it.<br>
<br>
As buffer handling code isn't necessarily straightforward to follow, I'd<br>
expand this to<br>
<br>
* This buffer was not part of the Request (which happens if an<br>
* internal buffer was used for an external stream, or<br>
* unconditionally for internal streams), or there is no pending<br>
* request, so we can recycle it.<br></blockquote><div><br></div><div>Sure, I can change this. Or would it be possible to update before merging</div><div>provided there are no further changes needed?</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> + */<br>
> + stream->returnBuffer(buffer);<br>
> }<br>
> }<br>
> <br>
> @@ -2104,7 +2099,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em<br>
> FrameBuffer *b = embeddedQueue_.front();<br>
> if (!unicam_[Unicam::Embedded].isExternal() && b->metadata().timestamp < ts) {<br>
> embeddedQueue_.pop();<br>
> - unicam_[Unicam::Embedded].queueBuffer(b);<br>
> + unicam_[Unicam::Embedded].returnBuffer(b);<br>
> embeddedRequeueCount++;<br>
> LOG(RPI, Warning) << "Dropping unmatched input frame in stream "<br>
> << unicam_[Unicam::Embedded].name();<br>
> @@ -2140,7 +2135,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em<br>
> * the front of the queue. This buffer is now orphaned, so requeue<br>
> * it back to the device.<br>
> */<br>
> - unicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);<br>
> + unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer);<br>
> bayerQueue_.pop();<br>
> bayerRequeueCount++;<br>
> LOG(RPI, Warning) << "Dropping unmatched input frame in stream "<br>
> @@ -2158,7 +2153,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em<br>
> <br>
> LOG(RPI, Warning) << "Flushing bayer stream!";<br>
> while (!bayerQueue_.empty()) {<br>
> - unicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);<br>
> + unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer);<br>
> bayerQueue_.pop();<br>
> }<br>
> flushedBuffers = true;<br>
> @@ -2175,7 +2170,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em<br>
> <br>
> LOG(RPI, Warning) << "Flushing embedded data stream!";<br>
> while (!embeddedQueue_.empty()) {<br>
> - unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());<br>
> + unicam_[Unicam::Embedded].returnBuffer(embeddedQueue_.front());<br>
> embeddedQueue_.pop();<br>
> }<br>
> flushedBuffers = true;<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp<br>
> index a421ad09ba50..f446e1ce66c7 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp<br>
> @@ -163,8 +163,11 @@ int Stream::queueBuffer(FrameBuffer *buffer)<br>
> <br>
> void Stream::returnBuffer(FrameBuffer *buffer)<br>
> {<br>
> - /* This can only be called for external streams. */<br>
> - ASSERT(external_);<br>
> + if (!external_) {<br>
> + /* For internal buffers, simply requeue back to the device. */<br>
> + queueToDevice(buffer);<br>
> + return;<br>
> + }<br>
> <br>
> /* Push this buffer back into the queue to be used again. */<br>
> availableBuffers_.push(buffer);<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>