<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 6 Mar 2023 at 17:28, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Mar 02, 2023 at 01:54:29PM +0000, Naushir Patuck via libcamera-devel wrote:<br>
> <br>
> Add a new parameter to the pipeline handler config file named<br>
> "unicam_timeout_value_ms" to allow users to override the automiatically<br>
<br>
s/automiatically/automatically/<br>
<br>
> computed Unicam timeout value.<br>
> <br>
> This value is given in milliseconds, and setting a value of 0 (the<br>
> default value) disables the override.<br>
<br>
Could you explain the use case for setting a custom timeout ?<br></blockquote><div><br></div><div>This is partially from a user requested use case.<br><br>Assume an app is configured a RAW stream, and provides buffers for the stream on<br>every request (i.e. our mandatory stream hint). If the application holds off on<br>sending requests for whatever reason (the dreaded timelapse comes to mind agin),<br>then we will possibly hit the watchdog timeout as it is only a small multiple of<br>the frame length. This override allows an application to select a larger value<br>with the knowledge that it may space requests longer than the calculated timeout<br>value.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
> .../pipeline/raspberrypi/data/example.yaml | 11 ++++++++++-<br>
> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++++<br>
> 2 files changed, 24 insertions(+), 1 deletion(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/raspberrypi/data/example.yaml<br>
> index ad5f2344384f..5d4ca997b7a0 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/data/example.yaml<br>
> +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml<br>
> @@ -32,6 +32,15 @@<br>
> # Override any request from the IPA to drop a number of startup<br>
> # frames.<br>
> #<br>
> - # "disable_startup_frame_drops": false<br>
> + # "disable_startup_frame_drops": false,<br>
> +<br>
> + # Custom timeout value (in ms) for Unicam to use. This overrides<br>
> + # the value computed by the pipeline handler based on frame<br>
> + # durations.<br>
> + #<br>
> + # Set this value to 0 to use the pipeline handler computed<br>
> + # timeout value.<br>
> + #<br>
> + # "unicam_timeout_value_ms": 0<br>
<br>
While at it, I'd add a trailing comma here to minimize the diff next<br>
time you will add a parameter.<br>
<br>
> }<br>
> }<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 3d04842a2440..6b0880c579eb 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -319,6 +319,11 @@ public:<br>
> * frames.<br>
> */<br>
> bool disableStartupFrameDrops;<br>
> + /*<br>
> + * Override the Unicam timeout value calculated by the IPA based<br>
> + * on frame durations.<br>
> + */<br>
> + unsigned int unicamTimeoutValue;<br>
> };<br>
> <br>
> Config config_;<br>
> @@ -1715,6 +1720,7 @@ int RPiCameraData::loadPipelineConfiguration()<br>
> .minUnicamBuffers = 2,<br>
> .minTotalUnicamBuffers = 4,<br>
> .disableStartupFrameDrops = false,<br>
> + .unicamTimeoutValue = 0,<br>
> };<br>
> <br>
> char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");<br>
> @@ -1750,6 +1756,14 @@ int RPiCameraData::loadPipelineConfiguration()<br>
> phConfig["min_total_unicam_buffers"].get<unsigned int>(config_.minTotalUnicamBuffers);<br>
> config_.disableStartupFrameDrops =<br>
> phConfig["disable_startup_frame_drops"].get<bool>(config_.disableStartupFrameDrops);<br>
> + config_.unicamTimeoutValue =<br>
> + phConfig["unicam_timeout_value_ms"].get<unsigned int>(config_.unicamTimeoutValue);<br>
> +<br>
> + if (config_.unicamTimeoutValue) {<br>
> + /* Disable the IPA signal to control timeout and set the user requested value. */<br>
> + unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect();<br>
<br>
Didn't you mean to disconnect the signal emitted by the IPA module to<br>
set the timeout, not the signal emitted by the V4L2VideoDevice when a<br>
timeout occurs ?<br></blockquote><div><br></div><div>You are right, and annoyingly I was certain I fixed this before submitting :-(<br>I will fix up for the next revision!<br></div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + unicam_[Unicam::Image].dev()->setDequeueTimeout(config_.unicamTimeoutValue * 1ms);<br>
> + }<br>
> <br>
> if (config_.minTotalUnicamBuffers < config_.minUnicamBuffers) {<br>
> LOG(RPI, Error) << "Invalid configuration: min_total_unicam_buffers must be >= min_unicam_buffers";<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>