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

[Naushir Patuck]

Hi Laurent,

Thank you for your feedback!

On Mon, 6 Mar 2023 at 17:28, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> 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 ?
>

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.


>
> > 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!

Regards,
Naush



>
> > +
>  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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230307/cd4f1b3c/attachment.htm>