[libcamera-devel,v1,2/8] pipeline: raspberrypi: Handle MandatoryStream hints for Unicam Image

18247 diff mbox series

Look for MandatoryStream flag in the hints field of the Unicam Image
StreamConfiguration structure. If this flag is set, it guarantees that the
application will provide buffers for Unicam Image, so override the
minUnicamBuffers and minTotalUnicamBuffers config parameters in the following
way:

- If startup drop frames are required, allocate at least 1 internal buffer.
- If no startup drop frames are required, do not allocate any internal buffers.

All other buffer allocations remain unchanged.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 34 +++++++++++++++----
 1 file changed, 27 insertions(+), 7 deletions(-)

Hi Naush

On Fri, Feb 03, 2023 at 09:44:18AM +0000, Naushir Patuck via libcamera-devel wrote:
> Look for MandatoryStream flag in the hints field of the Unicam Image
> StreamConfiguration structure. If this flag is set, it guarantees that the
> application will provide buffers for Unicam Image, so override the
> minUnicamBuffers and minTotalUnicamBuffers config parameters in the following
> way:
>
> - If startup drop frames are required, allocate at least 1 internal buffer.
> - If no startup drop frames are required, do not allocate any internal buffers.
>
> All other buffer allocations remain unchanged.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 34 +++++++++++++++----
>  1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index d752911ddfff..7d786fe839f7 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1520,12 +1520,33 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>  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;
>  	int ret;
>
>  	for (Stream *s : camera->streams()) {
> -		if (isRaw(s->configuration().pixelFormat)) {
> +		if (s == &data->unicam_[Unicam::Image]) {

What if the sensor produces YUV ? I guess it doesn't really make a
difference, we'll have to drop frames or not regardless of the image
format ?

>  			numRawBuffers = s->configuration().bufferCount;
> +			/*
> +			 * If the application provides a guarantees that Unicam
> +			 * image buffers will always be provided for the RAW stream
> +			 * in a Request, we need:
> +			 * - at least 1 internal Unicam buffer to handle startup frame drops,
> +			 * - no internal Unicam buffers if there are no startup frame drops.


> +			 */
> +			if (s->configuration().hints &
> +			    StreamConfiguration::Hint::MandatoryStream) {
> +				if (data->dropFrameCount_) {
> +					minUnicamBuffers = std::max(1u, minUnicamBuffers);
> +					minTotalUnicamBuffers =
> +						std::max(minUnicamBuffers, minTotalUnicamBuffers);
> +				} else {
> +					minUnicamBuffers = 0;
> +					minTotalUnicamBuffers = 0;
> +				}
> +			}
> +
>  			break;
>  		}
>  	}
> @@ -1537,7 +1558,6 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  		 * For Unicam, allocate a minimum number of buffers for internal
>  		 * use as we want to avoid any frame drops.
>  		 */
> -		const unsigned int minBuffers = data->config_.minTotalUnicamBuffers;
>  		if (stream == &data->unicam_[Unicam::Image]) {
>  			/*
>  			 * If an application has configured a RAW stream, allocate
> @@ -1545,8 +1565,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  			 * we have at least minUnicamBuffers of internal buffers
>  			 * to use to minimise frame drops.
>  			 */
> -			numBuffers = std::max<int>(data->config_.minUnicamBuffers,
> -						   minBuffers - numRawBuffers);
> +			numBuffers = std::max<int>(minUnicamBuffers,
> +						   minTotalUnicamBuffers - numRawBuffers);

if minTotalUnicamBuffers == 0, is there a risk that
(minTotalUnicamBuffers - numRawBuffers) becomes negative ? as
std::max<int> do we risk any wrong type conversion ?

>  		} else if (stream == &data->isp_[Isp::Input]) {
>  			/*
>  			 * ISP input buffers are imported from Unicam, so follow
> @@ -1554,15 +1574,15 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  			 * available.
>  			 */
>  			numBuffers = numRawBuffers +
> -				     std::max<int>(data->config_.minUnicamBuffers,
> -						   minBuffers - numRawBuffers);
> +				     std::max<int>(minUnicamBuffers,
> +						   minTotalUnicamBuffers - numRawBuffers);

ditto

>
>  		} else if (stream == &data->unicam_[Unicam::Embedded]) {
>  			/*
>  			 * Embedded data buffers are (currently) for internal use,
>  			 * so allocate the minimum required to avoid frame drops.
>  			 */
> -			numBuffers = minBuffers;
> +			numBuffers = data->config_.minTotalUnicamBuffers;
>  		} else {
>  			/*
>  			 * Since the ISP runs synchronous with the IPA and requests,
> --
> 2.25.1
>