[libcamera-devel] [PATCH v2 3/3] pipeline: raspberrypi: Add a Unicam timeout override config options

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Thu, Mar 02, 2023 at 01:54:29PM +0000, Naushir Patuck via libcamera-devel wrote:
> 
> Add a new parameter to the pipeline handler config file named
> "unicam_timeout_value_ms" to allow users to override the automiatically

s/automiatically/automatically/

> computed Unicam timeout value.
> 
> This value is given in milliseconds, and setting a value of 0 (the
> default value) disables the override.

Could you explain the use case for setting a custom timeout ?

> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/data/example.yaml         | 11 ++++++++++-
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/raspberrypi/data/example.yaml
> index ad5f2344384f..5d4ca997b7a0 100644
> --- a/src/libcamera/pipeline/raspberrypi/data/example.yaml
> +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml
> @@ -32,6 +32,15 @@
>                  # Override any request from the IPA to drop a number of startup
>                  # frames.
>                  #
> -                # "disable_startup_frame_drops": false
> +                # "disable_startup_frame_drops": false,
> +
> +                # Custom timeout value (in ms) for Unicam to use. This overrides
> +                # the value computed by the pipeline handler based on frame
> +                # durations.
> +                #
> +                # Set this value to 0 to use the pipeline handler computed
> +                # timeout value.
> +                #
> +                # "unicam_timeout_value_ms": 0

While at it, I'd add a trailing comma here to minimize the diff next
time you will add a parameter.

>          }
>  }
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 3d04842a2440..6b0880c579eb 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -319,6 +319,11 @@ public:
>  		 * frames.
>  		 */
>  		bool disableStartupFrameDrops;
> +		/*
> +		 * Override the Unicam timeout value calculated by the IPA based
> +		 * on frame durations.
> +		 */
> +		unsigned int unicamTimeoutValue;
>  	};
>  
>  	Config config_;
> @@ -1715,6 +1720,7 @@ int RPiCameraData::loadPipelineConfiguration()
>  		.minUnicamBuffers = 2,
>  		.minTotalUnicamBuffers = 4,
>  		.disableStartupFrameDrops = false,
> +		.unicamTimeoutValue = 0,
>  	};
>  
>  	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");
> @@ -1750,6 +1756,14 @@ int RPiCameraData::loadPipelineConfiguration()
>  		phConfig["min_total_unicam_buffers"].get<unsigned int>(config_.minTotalUnicamBuffers);
>  	config_.disableStartupFrameDrops =
>  		phConfig["disable_startup_frame_drops"].get<bool>(config_.disableStartupFrameDrops);
> +	config_.unicamTimeoutValue =
> +		phConfig["unicam_timeout_value_ms"].get<unsigned int>(config_.unicamTimeoutValue);
> +
> +	if (config_.unicamTimeoutValue) {
> +		/* Disable the IPA signal to control timeout and set the user requested value. */
> +		unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect();

Didn't you mean to disconnect the signal emitted by the IPA module to
set the timeout, not the signal emitted by the V4L2VideoDevice when a
timeout occurs ?

> +		unicam_[Unicam::Image].dev()->setDequeueTimeout(config_.unicamTimeoutValue * 1ms);
> +	}
>  
>  	if (config_.minTotalUnicamBuffers < config_.minUnicamBuffers) {
>  		LOG(RPI, Error) << "Invalid configuration: min_total_unicam_buffers must be >= min_unicam_buffers";

-- 
Regards,

Laurent Pinchart