[libcamera-devel] [PATCH v2 03/10] pipeline: raspberrypi: Split out ISP Output0 buffer allocation

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Tue, Nov 29, 2022 at 01:45:27PM +0000, Naushir Patuck via libcamera-devel wrote:
> Add a new config parameter numOutput0Buffers specifying the number of internally
> allocated ISP Output0 buffers. This parameter defaults to 1.
> 
> Split out the buffer allocation logic so that the buffer count for ISP Output0
> can be different from the ISP Output1 and Statistics streams.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 21 +++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 4486d31ea78d..f2b695af2399 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -305,6 +305,11 @@ public:
>  		 * the Unicam Image steram.
>  		 */
>  		unsigned int minTotalUnicamBuffers;
> +		/*
> +		 * The number of internal buffers allocated for the ISP Output0
> +		 * stream.

I'd comment here that the value can only be 0 or 1.

> +		 */
> +		unsigned int numOutput0Buffers;
>  	};
>  
>  	Config config_;
> @@ -1418,6 +1423,7 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)
>  	config = {
>  		.minUnicamBuffers = 2,
>  		.minTotalUnicamBuffers = 4,
> +		.numOutput0Buffers = 1,
>  	};
>  
>  	return 0;
> @@ -1502,12 +1508,19 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  			 * so allocate the minimum required to avoid frame drops.
>  			 */
>  			numBuffers = minBuffers;
> -		} else {
> +		} else if (stream == &data->isp_[Isp::Output0]) {
>  			/*
>  			 * 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().
> +			 * we only ever need a maximum of one internal buffer. Any
> +			 * buffers the application wants to hold onto will already
> +			 * be exported through PipelineHandlerRPi::exportFrameBuffers().
> +			 */
> +			numBuffers = std::min(1u, data->config_.numOutput0Buffers);

Is it OK to call Stream::prepareBuffers() below if numBuffers is 0 ?

> +		} else {
> +			/*
> +			 * Same reasoning as above, we only ever need a maximum
> +			 * of one internal buffer for Output1 (required for colour
> +			 * denoise) and ISP statistics.
>  			 */
>  			numBuffers = 1;
>  		}

-- 
Regards,

Laurent Pinchart