[libcamera-devel] [PATCH 1/3] pipeline: raspberrypi: Allow Stream::returnBuffer() to handle internal buffers

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Mon, Feb 07, 2022 at 03:12:12PM +0000, Naushir Patuck wrote:
> If Stream::returnBuffer() gets passed an internally allocated buffer, it now
> simply re-queues it back to the device. With this change, the pipeline handler
> code can be simplified slightly as it does not need multiple code paths for
> internally allocated and non-internally allocated buffers.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 53 +++++++++----------
>  .../pipeline/raspberrypi/rpi_stream.cpp       |  7 ++-
>  2 files changed, 29 insertions(+), 31 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 49af56edc1f9..0755de84c70c 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1877,34 +1877,29 @@ void RPiCameraData::clearIncompleteRequests()
>  
>  void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream)
>  {
> -	if (stream->isExternal()) {
> +	/*
> +	 * It is possible to be here without a pending request, so check
> +	 * that we actually have one to action, otherwise we just return
> +	 * buffer back to the stream.
> +	 */
> +	Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front();
> +	if (!dropFrameCount_ && request && request->findBuffer(stream) == buffer) {
>  		/*
> -		 * It is possible to be here without a pending request, so check
> -		 * that we actually have one to action, otherwise we just return
> -		 * buffer back to the stream.
> +		 * Check if this is an externally provided buffer, and if
> +		 * so, we must stop tracking it in the pipeline handler.
>  		 */
> -		Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front();
> -		if (!dropFrameCount_ && request && request->findBuffer(stream) == buffer) {
> -			/*
> -			 * Check if this is an externally provided buffer, and if
> -			 * so, we must stop tracking it in the pipeline handler.
> -			 */
> -			handleExternalBuffer(buffer, stream);
> -			/*
> -			 * Tag the buffer as completed, returning it to the
> -			 * application.
> -			 */
> -			pipe()->completeBuffer(request, buffer);
> -		} else {
> -			/*
> -			 * This buffer was not part of the Request, or there is no
> -			 * pending request, so we can recycle it.
> -			 */
> -			stream->returnBuffer(buffer);
> -		}
> +		handleExternalBuffer(buffer, stream);
> +		/*
> +		 * Tag the buffer as completed, returning it to the
> +		 * application.
> +		 */
> +		pipe()->completeBuffer(request, buffer);
>  	} else {
> -		/* Simply re-queue the buffer to the requested stream. */
> -		stream->queueBuffer(buffer);
> +		/*
> +		 * This buffer was not part of the Request, or there is no
> +		 * pending request, so we can recycle it.

As buffer handling code isn't necessarily straightforward to follow, I'd
expand this to

		 * This buffer was not part of the Request (which happens if an
		 * internal buffer was used for an external stream, or
		 * unconditionally for internal streams), or there is no pending
		 * request, so we can recycle it.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +		 */
> +		stream->returnBuffer(buffer);
>  	}
>  }
>  
> @@ -2104,7 +2099,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em
>  			FrameBuffer *b = embeddedQueue_.front();
>  			if (!unicam_[Unicam::Embedded].isExternal() && b->metadata().timestamp < ts) {
>  				embeddedQueue_.pop();
> -				unicam_[Unicam::Embedded].queueBuffer(b);
> +				unicam_[Unicam::Embedded].returnBuffer(b);
>  				embeddedRequeueCount++;
>  				LOG(RPI, Warning) << "Dropping unmatched input frame in stream "
>  						  << unicam_[Unicam::Embedded].name();
> @@ -2140,7 +2135,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em
>  				 * the front of the queue. This buffer is now orphaned, so requeue
>  				 * it back to the device.
>  				 */
> -				unicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);
> +				unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer);
>  				bayerQueue_.pop();
>  				bayerRequeueCount++;
>  				LOG(RPI, Warning) << "Dropping unmatched input frame in stream "
> @@ -2158,7 +2153,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em
>  
>  				LOG(RPI, Warning) << "Flushing bayer stream!";
>  				while (!bayerQueue_.empty()) {
> -					unicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);
> +					unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer);
>  					bayerQueue_.pop();
>  				}
>  				flushedBuffers = true;
> @@ -2175,7 +2170,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em
>  
>  				LOG(RPI, Warning) << "Flushing embedded data stream!";
>  				while (!embeddedQueue_.empty()) {
> -					unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
> +					unicam_[Unicam::Embedded].returnBuffer(embeddedQueue_.front());
>  					embeddedQueue_.pop();
>  				}
>  				flushedBuffers = true;
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index a421ad09ba50..f446e1ce66c7 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -163,8 +163,11 @@ int Stream::queueBuffer(FrameBuffer *buffer)
>  
>  void Stream::returnBuffer(FrameBuffer *buffer)
>  {
> -	/* This can only be called for external streams. */
> -	ASSERT(external_);
> +	if (!external_) {
> +		/* For internal buffers, simply requeue back to the device. */
> +		queueToDevice(buffer);
> +		return;
> +	}
>  
>  	/* Push this buffer back into the queue to be used again. */
>  	availableBuffers_.push(buffer);

-- 
Regards,

Laurent Pinchart