[libcamera-devel] [PATCH v1 3/8] pipeline: raspberrypi: Handle MandatoryStream hints for ISP Output0

[Jacopo Mondi]

On Fri, Feb 03, 2023 at 09:44:19AM +0000, Naushir Patuck via libcamera-devel wrote:
> Look for MandatoryStream flag in the hints field of the ISP Output0
> StreamConfiguration structure. If this flag is set, it guarantees that
> the application will provide buffers for the ISP, do not allocate any
> internal buffers for the device.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 29 ++++++++++++++-----
>  1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 7d786fe839f7..b6ed66a1cf46 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1522,7 +1522,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  	RPiCameraData *data = cameraData(camera);
>  	unsigned int minUnicamBuffers = data->config_.minUnicamBuffers;
>  	unsigned int minTotalUnicamBuffers = data->config_.minTotalUnicamBuffers;
> -	unsigned int numRawBuffers = 0;
> +	unsigned int numRawBuffers = 0, minIspBuffers = 1;

nit: I guess we prefer one variable per line (not sure how much we can
enforce it though)

>  	int ret;
>
>  	for (Stream *s : camera->streams()) {
> @@ -1546,8 +1546,21 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  					minTotalUnicamBuffers = 0;
>  				}
>  			}
> -
> -			break;
> +		} else if (s == &data->isp_[Isp::Output0]) {
> +			/*
> +			 * Since the ISP runs synchronous with the IPA and requests,
> +			 * 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().
> +			 *
> +			 * However, as above, if the application provides a guarantee
> +			 * that the buffer will always be provided for the ISP Output0
> +			 * stream in a Request, we don't need any internal buffers
> +			 * allocated.
> +			 */
> +			if (!data->dropFrameCount_ &&
> +			    s->configuration().hints & StreamConfiguration::Hint::MandatoryStream)
> +				minIspBuffers = 0;
>  		}
>  	}
>
> @@ -1583,12 +1596,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  			 * so allocate the minimum required to avoid frame drops.
>  			 */
>  			numBuffers = data->config_.minTotalUnicamBuffers;
> +		} else if (stream == &data->isp_[Isp::Output0]) {
> +			/* Buffer count for this is handled in the earlier loop above. */
> +			numBuffers = minIspBuffers;
>  		} 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().
> +			 * Same reasoning as for ISP Output 0, we only ever need

Isn't it different ? I got that Ouput1 is not a concern and we can
unconditionally allocate a buffer due to its smaller resolution ?

> +			 * a maximum of one internal buffer for Output1 (required
> +			 * for colour denoise) and ISP statistics.
>  			 */
>  			numBuffers = 1;
>  		}
> --
> 2.25.1
>