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

[David Plowman]

Hi Naush

Thanks for the patch.

On Thu, 23 Feb 2023 at 12:49, Naushir Patuck via libcamera-devel
<libcamera-devel at lists.libcamera.org> 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.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/data/example.yaml    | 11 ++++++-
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 29 ++++++++++++++-----
>  2 files changed, 32 insertions(+), 8 deletions(-)
>
> 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
>          }
>  }
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 3d04842a2440..7c8c66129014 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_;
> @@ -1158,6 +1163,9 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>
>         data->state_ = RPiCameraData::State::Idle;
>
> +       if (data->config_.unicamTimeoutValue)
> +               data->setCameraTimeout(data->config_.unicamTimeoutValue);
> +
>         /* Start all streams. */
>         for (auto const stream : data->streams_) {
>                 ret = stream->dev()->streamOn();
> @@ -1715,6 +1723,7 @@ int RPiCameraData::loadPipelineConfiguration()
>                 .minUnicamBuffers = 2,
>                 .minTotalUnicamBuffers = 4,
>                 .disableStartupFrameDrops = false,
> +               .unicamTimeoutValue = 0,
>         };
>
>         char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");
> @@ -1750,6 +1759,8 @@ 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<bool>(config_.disableStartupFrameDrops);

Is bool the right type here? Also is the appearance of
"disableStartupFrameDrops" a copy/paste typo?

Apart from these minor things:

Reviewed-by: David Plowman <david.plowman at raspberrypi.com>

Thanks!
David

>
>         if (config_.minTotalUnicamBuffers < config_.minUnicamBuffers) {
>                 LOG(RPI, Error) << "Invalid configuration: min_total_unicam_buffers must be >= min_unicam_buffers";
> @@ -1953,13 +1964,17 @@ void RPiCameraData::setLensControls(const ControlList &controls)
>
>  void RPiCameraData::setCameraTimeout(uint32_t maxFrameLengthMs)
>  {
> -       /*
> -        * Set the dequeue timeout to the larger of 5x the maximum reported
> -        * frame length advertised by the IPA over a number of frames. Allow
> -        * a minimum timeout value of 1s.
> -        */
> -       utils::Duration timeout =
> -               std::max<utils::Duration>(1s, 5 * maxFrameLengthMs * 1ms);
> +       utils::Duration timeout;
> +
> +       if (!config_.unicamTimeoutValue) {
> +               /*
> +                * Set the dequeue timeout to the larger of 5x the maximum reported
> +                * frame length advertised by the IPA over a number of frames. Allow
> +                * a minimum timeout value of 1s.
> +                */
> +               timeout = std::max<utils::Duration>(1s, 5 * maxFrameLengthMs * 1ms);
> +       } else
> +               timeout = config_.unicamTimeoutValue * 1ms;
>
>         LOG(RPI, Debug) << "Setting Unicam timeout to " << timeout;
>         unicam_[Unicam::Image].dev()->setDequeueTimeout(timeout);
> --
> 2.25.1
>