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

[Naushir Patuck]

Hi Jacopo,

Thank you for the feedback!

On Wed, 1 Mar 2023 at 15:17, Jacopo Mondi <jacopo.mondi at ideasonboard.com> wrote:
>
> Hi Naush
>
> On Thu, Feb 23, 2023 at 12:49:57PM +0000, Naushir Patuck via libcamera-devel wrote:
> > Date: Thu, 23 Feb 2023 12:49:57 +0000
> > From: Naushir Patuck <naush at raspberrypi.com>
> > To: libcamera-devel at lists.libcamera.org
> > Subject: [PATCH v1 3/3] pipeline: raspberrypi: Add a Unicam timeout
> >  override config options
> > X-Mailer: git-send-email 2.25.1
> >
> > Add a new parameter to the pipeline handler config file named
> > "unicam_timeout_value_ms" to allow users to override the automiatically
> > 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);
> >
> >       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) {
>
> I wonder, when the value comes from configuration file, can it change
> at run-time ? If it can't is there any purpose in emitting the signal,
> handling it here and setting the same value on the video device ? If
> that's the case, I would try to figure out early if unicamTimeoutValue
> != 0 and if that's the case set the value on the video device and not
> connected (or disconnect) the signal

Good point. If the value comes from the config file, we will never have a
runtime change. In these cases, I can disconnect the signal.

For simplicity, I'll continue emitting the signal from the IPA, but presumably
that's ok if the handler is disconnected?

Regards,
Naush

>
> > +             /*
> > +              * 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
> >