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

[Laurent Pinchart]

Hi Naush,

On Tue, Mar 07, 2023 at 08:50:42AM +0000, Naushir Patuck wrote:
> On Mon, 6 Mar 2023 at 17:28, Laurent Pinchart wrote:
> > 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 ?
> 
> This is partially from a user requested use case.
> 
> Assume an app is configured a RAW stream, and provides buffers for the stream on
> every request (i.e. our mandatory stream hint).  If the application holds off on
> sending requests for whatever reason (the dreaded timelapse comes to mind agin),
> then we will possibly hit the watchdog timeout as it is only a small multiple of
> the frame length.  This override allows an application to select a larger value
> with the knowledge that it may space requests longer than the calculated timeout
> value.

OK. Could you record that at least in the commit message ?

> > > 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 ?
> 
> You are right, and annoyingly I was certain I fixed this before submitting :-(
> I will fix up for the next revision!
> 
> > > +        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