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

14588 diff mbox series

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@raspberrypi.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 43 ++++++++++++++-----
 1 file changed, 32 insertions(+), 11 deletions(-)

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@raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@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;
>  	}