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

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Fri, Nov 12, 2021 at 10:03:04AM +0000, Naushir Patuck wrote:
> For simplicity, the pipeline handler currently look at the maximum number of
> buffers set in the StreamConfiguration by the user 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.
> 
> The key change is to mostly decouple the number of internal buffers allocated
> from number of buffers requested by the user through the StreamConfiguration.
> 
> 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 2 internal buffers to minimise frame drops.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

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

> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 43 ++++++++++++++-----
>  1 file changed, 32 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 11d3c2b120dd..4f6c699a4379 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1211,21 +1211,42 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>  int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  {
>  	RPiCameraData *data = cameraData(camera);
> +	unsigned int numRawBuffers = 0;
>  	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().
> -	 */
> -	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)) {
> +			numRawBuffers = s->configuration().bufferCount;
> +			break;
> +		}
> +	}
>  
> +	/* Decide how many internal buffers to allocate. */
>  	for (auto const stream : data->streams_) {
> -		ret = stream->prepareBuffers(maxBuffers);
> +		unsigned int numBuffers;
> +
> +		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 2 sets of internal
> +			 * buffers to use to minimise frame drops.
> +			 */
> +			constexpr unsigned int minBuffers = 4;
> +			numBuffers = std::max<int>(2, minBuffers - numRawBuffers);
> +		} 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);
>  		if (ret < 0)
>  			return ret;
>  	}

-- 
Regards,

Laurent Pinchart