[libcamera-devel] [PATCH v7 01/11] libcamera: property: Add MinimumRequests property

[Laurent Pinchart]

Hi Nícolas,

Thank you for the patch.

On Thu, Jul 22, 2021 at 08:28:41PM -0300, Nícolas F. R. A. Prado wrote:
> The MinimumRequests property reports the minimum number of capture
> requests that the camera pipeline requires to have queued in order to
> sustain capture operations. 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.

The second sentence should be part of the property definition, as it's
fairly important.

> By default the property is set to 1 in the CameraSensor constructor, and
> it can be overridden by each pipeline. The uvcvideo pipeline explicitly
> sets the property to 1 since it doesn't have a CameraSensor.

The value is a property of a pipeline, so I don't think setting a
default in CameraSensor is a good idea. I'd rather have all pipeline
handlers setting the property. Most of them do already, so it shouldn't
be a big issue.

> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> ---
> 
> Changes in v7:
> - Thanks to Kieran:
>   - Renamed property from MinNumRequests to MinimumRequests
> - Thanks to Jacopo:
>   - Changed property's description
> 
> Changes in v6:
> - Thanks to Naushir:
>   - Removed comment from Raspberrypi MinNumRequests setting
> - Removed an extra blank line
> 
>  src/libcamera/camera_sensor.cpp              |  1 +
>  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              | 10 ++++++++++
>  6 files changed, 19 insertions(+)
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index cde431cc4c80..d8ed010bfd08 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -57,6 +57,7 @@ CameraSensor::CameraSensor(const MediaEntity *entity)
>  	: entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),
>  	  properties_(properties::properties)
>  {
> +	properties_.set(properties::MinimumRequests, 1);
>  }
>  
>  /**
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 42911a8fdfdb..cb4ca9a85fa8 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::MinimumRequests, 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..348c554c8be4 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::MinimumRequests, 3);

The value needs to be device-dependent. Could you store it in the
SimplePipelineInfo structure ? imx7-csi needs 2 buffers, sun6i-csi needs
3, and I believe qcom-camss needs 1.

>  
>  	return 0;
>  }
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 0f634b8da609..6ad7fb3c9f0c 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::MinimumRequests, 1);
> +
>  	/* 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..44c198ccf8b8 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::MinimumRequests, 2);

It would be nice if the vimc driver could function with a single buffer
(out of scope for this patch series of course).

>  
>  	return 0;
>  }
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index 12ecbce5eed4..23ef0e9d38c4 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -678,6 +678,16 @@ controls:
>          \todo Turn this property into a "maximum control value" for the
>          ScalerCrop control once "dynamic" controls have been implemented.
>  
> +  - MinimumRequests:
> +      type: int32_t
> +      description: |
> +        The minimum number of capture requests that the camera pipeline requires
> +        to have queued in order to sustain capture operations.

So this would become

	The minimum number of capture requests that the camera pipeline requires
	to have queued in order to sustain capture operations. At this quantity,
	there's no guarantee that frames won't be dropped or that manual
	per-frame controls will apply correctly.

> +
> +        This property usually corresponds to the minimum number of memory
> +        buffers needed to fill the capture pipeline composed of hardware
> +        processing blocks.

Could you add the following comment ?

	\todo Extend this property to expose the different minimum buffer and
	request counts (the minimum number of buffers to be able to capture at
	all, the minimum number of buffers to sustain capture without frame
	drop, and the minimum number of requests to ensure proper operation of
	per-frame controls).

With this,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +
>    # ----------------------------------------------------------------------------
>    # Draft properties section
>  

-- 
Regards,

Laurent Pinchart