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

[Umang Jain]

Hi Naush

On 11/10/21 3:38 PM, 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 would initialise this value to 0, it might stand a chance in future 
(though unlikely) that it is used uninitialised below

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

So just future-proof the usage

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

> +	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().
> -	 */
> -	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);
> +		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;
> +		} 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;
>   	}