| Message ID | mailman.58.1677765273.25031.libcamera-devel@lists.libcamera.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Thu, Mar 02, 2023 at 01:54:29PM +0000, Naushir Patuck via libcamera-devel wrote: > Date: Thu, 2 Mar 2023 13:54:29 +0000 > From: Naushir Patuck <naush@raspberrypi.com> > To: libcamera-devel@lists.libcamera.org > Subject: [PATCH v2 3/3] pipeline: raspberrypi: Add a Unicam timeout > override config options > X-Mailer: git-send-email 2.34.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@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> My understanding is that emitting a signal and have no slot connected is safe. Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.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 > } > } > 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(); > + 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"; > -- > 2.34.1 >
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@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@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";
Hi Laurent, Thank you for your feedback! On Mon, 6 Mar 2023 at 17:28, Laurent Pinchart < laurent.pinchart@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@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@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 >
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@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@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";
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..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(); + 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";