[libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Rework the internal buffer allocation scheme

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Wed, Nov 10, 2021 at 10:08:01AM +0000, Naushir Patuck wrote:
> For simplicity, the pipeline handler would look at the maximum number of
> buffers set in the StreamConfiguration and allocate the same number of internal
> buffers for all device nodes. This would likely overallocate buffers for some
> nodes. Rework this logic to try and minimise overallcations without compromising
> performance.
> 
> For ISP nodes, we only ever need 1 set of internal buffers, as the hardware runs
> synchronous with the requests and IPA.
> 
> For Unicam nodes, allocate a minimum for 4 buffers (exported + internal), but
> also require at least 1 internal buffer.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 44 ++++++++++++++-----
>  1 file changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 11d3c2b120dd..5f0f00aacd59 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1211,21 +1211,43 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>  int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  {
>  	RPiCameraData *data = cameraData(camera);
> +	unsigned int numBuffers;

I'd name the variable numRawBuffers.

> +	bool rawStream = false;
>  	int ret;
>  
> -	/*
> -	 * Decide how many internal buffers to allocate. For now, simply look
> -	 * at how many external buffers will be provided. We'll need to improve
> -	 * this logic. However, we really must have all streams allocate the same
> -	 * number of buffers to simplify error handling in queueRequestDevice().

Does the error logic still work after this change ?

> -	 */
> -	unsigned int maxBuffers = 0;
> -	for (const Stream *s : camera->streams())
> -		if (static_cast<const RPi::Stream *>(s)->isExternal())
> -			maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
> +	for (Stream *s : camera->streams()) {
> +		if (isRaw(s->configuration().pixelFormat)) {
> +			numBuffers = s->configuration().bufferCount;
> +			rawStream = true;
> +			break;
> +		}
> +	}
>  
> +	/* Decide how many internal buffers to allocate. */
>  	for (auto const stream : data->streams_) {
> -		ret = stream->prepareBuffers(maxBuffers);

And here add a numBuffers local variable.

> +		if (stream == &data->unicam_[Unicam::Image] ||
> +		    stream == &data->unicam_[Unicam::Embedded]) {
> +			/*
> +			 * For Unicam, allocate a minimum of 4 buffers as we want
> +			 * to avoid any frame drops. If an application has configured
> +			 * a RAW stream, allocate additional buffers to make up the
> +			 * minimum, but ensure we have at least 1 set of internal
> +			 * buffers to use to minimise frame drops.
> +			 */
> +			constexpr unsigned int minBuffers = 4;
> +			numBuffers = rawStream ? std::max<int>(1, minBuffers - numBuffers)
> +					       : minBuffers;

You could initialize numRawBuffers to 0 and drop rawStream, as if
!rawStream, then numRawBuffers will be equal to 0, so

			numBuffers = std::max<int>(1, minBuffers - numRawBuffers);

would do the right thing.

> +		} else {
> +			/*
> +			 * Since the ISP runs synchronous with the IPA and requests,
> +			 * we only ever need one set of internal buffers. Any buffers
> +			 * the application wants to hold onto will already be exported
> +			 * through PipelineHandlerRPi::exportFrameBuffers().
> +			 */
> +			numBuffers = 1;
> +		}
> +
> +		ret = stream->prepareBuffers(numBuffers);

I have a hard time follow this. Before the patch, the pipeline handler
calls prepareBuffers() with the number of buffers requested by the
application. Now it calls it with a number of internal buffers only,
without any change to the prepareBuffers() function itself. This would
benefit from an explanation in the commit message, as it's not clear why
it's correct.

Furthermore, what will happen if an application requests 4 buffers for
the raw stream and any number of buffers for a processed streams, and
repeatedly queues requests with no buffer for the raw stream ? It seems
to me that you'll now allocate a single extra buffer for the raw stream,
which won't be enough to keep the unicam queue running.

>  		if (ret < 0)
>  			return ret;
>  	}

-- 
Regards,

Laurent Pinchart