[libcamera-devel] [PATCH v6 01/10] libcamera: property: Add MinNumRequests property

[Kieran Bingham]

Hi Nicolas,

On 14/07/2021 19:38, Nícolas F. R. A. Prado wrote:
> The MinNumRequests property reports the bare minimum number of requests
> needed in the camera pipeline for capture to be possible. It is equal to
> the number of buffers required by the driver. At this quantity, there's
> no guarantee that frames won't be dropped or that manual per-frame
> controls will apply correctly. The quantity needed for those may be
> added as separate properties in the future.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> ---
> 
> Changes in v6:
> - Thanks to Naushir:
>   - Removed comment from Raspberrypi MinNumRequests setting
> - Removed an extra blank line
> 
>  src/libcamera/pipeline/ipu3/ipu3.cpp               | 6 ++++++
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 ++
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp           | 2 ++
>  src/libcamera/pipeline/simple/simple.cpp           | 2 ++
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp       | 2 ++
>  src/libcamera/pipeline/vimc/vimc.cpp               | 2 ++
>  src/libcamera/property_ids.yaml                    | 8 ++++++++
>  7 files changed, 24 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 76c3bb3d8aa9..017018c845fa 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1111,6 +1111,12 @@ int PipelineHandlerIPU3::registerCameras()
>  		/* Initialize the camera properties. */
>  		data->properties_ = cio2->sensor()->properties();
>  
> +		/*
> +		 * \todo Make sure this is correct even after the stream is
> +		 * configured to CIO2

What does this comment mean?

I see that you don't have a reference to IPU3 in your cover letter, so
perhaps you're not able to test and validate this.

What needs to be tested further?

I'd rather see this tested and set correctly with the series than being
incorrectly introduced, and having to fix up later, as IPU3 is one of
our key targets.



> +		 */
> +		data->properties_.set(properties::MinNumRequests, 1);
> +
>  		ret = initControls(data.get());
>  		if (ret)
>  			continue;
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index f821d8fe1b6c..98a97ffca15d 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1046,6 +1046,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>  	/* Initialize the camera properties. */
>  	data->properties_ = data->sensor_->properties();
>  
> +	data->properties_.set(properties::MinNumRequests, 1);
> +
>  	/*
>  	 * Set a default value for the ScalerCropMaximum property to show
>  	 * that we support its use, however, initialise it to zero because
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 42911a8fdfdb..c81ebf14c6bf 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -24,6 +24,7 @@
>  #include <libcamera/ipa/core_ipa_interface.h>
>  #include <libcamera/ipa/rkisp1_ipa_interface.h>
>  #include <libcamera/ipa/rkisp1_ipa_proxy.h>
> +#include <libcamera/property_ids.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> @@ -944,6 +945,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  
>  	/* Initialize the camera properties. */
>  	data->properties_ = data->sensor_->properties();
> +	data->properties_.set(properties::MinNumRequests, 3);
>  
>  	/*
>  	 * \todo Read dealy values from the sensor itself or from a
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index b29fff9820e5..c4adea61519f 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -25,6 +25,7 @@
>  
>  #include <libcamera/camera.h>
>  #include <libcamera/control_ids.h>
> +#include <libcamera/property_ids.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> @@ -436,6 +437,7 @@ int SimpleCameraData::init()
>  	}
>  
>  	properties_ = sensor_->properties();
> +	properties_.set(properties::MinNumRequests, 3);
>  
>  	return 0;
>  }
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 0f634b8da609..0258111ad6cf 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -525,6 +525,8 @@ int UVCCameraData::init(MediaDevice *media)
>  	properties_.set(properties::PixelArraySize, resolution);
>  	properties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });
>  
> +	properties_.set(properties::MinNumRequests, 1);

Is there any value in initialising this to one in the CameraSensor
constructor? Or is it better to make sure every pipeline sets it.

Will lc-complicance have a test to ensure that all pipelines set this?


> +
>  	/* Initialise the supported controls. */
>  	ControlInfoMap::Map ctrls;
>  
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 12f7517fd0ae..60ec473db0ba 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -21,6 +21,7 @@
>  #include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
>  #include <libcamera/formats.h>
> +#include <libcamera/property_ids.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> @@ -516,6 +517,7 @@ int VimcCameraData::init()
>  
>  	/* Initialize the camera properties. */
>  	properties_ = sensor_->properties();
> +	properties_.set(properties::MinNumRequests, 2);
>  
>  	return 0;
>  }
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index 12ecbce5eed4..0208f92074d3 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -678,6 +678,14 @@ controls:
>          \todo Turn this property into a "maximum control value" for the
>          ScalerCrop control once "dynamic" controls have been implemented.
>  
> +  - MinNumRequests:

As it's v6, perhaps this has been discussed/(bikeshedded) already - but
MinNumRequests grates on me a little

We have property names such as
  - ScalerCropMaximum:
  - ColorFilterArrangement:

so I see full words being used (Particularly looking at the 'Maximum').

That would suggest to me:

 - MinimumNumberRequests

which seems too verbose, so would then push me to:

 - MinimumRequests



> +      type: int32_t
> +      description: |
> +        The bare minimum number of requests needed in the camera pipeline for
> +        capture to be possible, as required by the driver. Note that this
> +        quantity does not guarantee that frames won't be dropped or that manual
> +        per-frame controls will be applied properly.
> +
>    # ----------------------------------------------------------------------------
>    # Draft properties section
>  
>