[libcamera-devel,v1,1/3] pipeline: ipa: raspberrypi: Change Unicam timeout handling

18301 diff mbox series

Add an explicit helper function setCameraTimeout() in the pipeline
handler to set the Unicam timeout value. This function is signalled from
the IPA to set up an appropriate timeout. This replaces the
maxSensorFrameLengthMs value parameter returned back from
IPARPi::start().

Adjust the timeout to be 5x the maximum frame duration reported by the
IPA.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/ipa/raspberrypi.mojom       |  2 +-
 src/ipa/raspberrypi/raspberrypi.cpp           |  2 +-
 .../pipeline/raspberrypi/raspberrypi.cpp      | 24 ++++++++++++-------
 3 files changed, 18 insertions(+), 10 deletions(-)

Hi Naush

Thanks for the patch!

On Thu, 23 Feb 2023 at 12:49, Naushir Patuck via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Add an explicit helper function setCameraTimeout() in the pipeline
> handler to set the Unicam timeout value. This function is signalled from
> the IPA to set up an appropriate timeout. This replaces the
> maxSensorFrameLengthMs value parameter returned back from
> IPARPi::start().
>
> Adjust the timeout to be 5x the maximum frame duration reported by the
> IPA.

(I guess you could have added "or 1s (whichever is longer)", but I
really wouldn't bother resubmitting just for that!!)

>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks!
David

> ---
>  include/libcamera/ipa/raspberrypi.mojom       |  2 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           |  2 +-
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 24 ++++++++++++-------
>  3 files changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index 8e78f167f179..80e0126618c8 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -49,7 +49,6 @@ struct IPAConfigResult {
>  struct StartConfig {
>         libcamera.ControlList controls;
>         int32 dropFrameCount;
> -       uint32 maxSensorFrameLengthMs;
>  };
>
>  interface IPARPiInterface {
> @@ -132,4 +131,5 @@ interface IPARPiEventInterface {
>         setIspControls(libcamera.ControlList controls);
>         setDelayedControls(libcamera.ControlList controls, uint32 delayContext);
>         setLensControls(libcamera.ControlList controls);
> +       setCameraTimeout(uint32 maxFrameLengthMs);
>  };
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 9b08ae4ca622..f6826bf27fe1 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -341,7 +341,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
>
>         startConfig->dropFrameCount = dropFrameCount_;
>         const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;
> -       startConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get<std::milli>();
> +       setCameraTimeout.emit(maxSensorFrameDuration.get<std::milli>());
>
>         firstStart_ = false;
>         lastRunTimestamp_ = 0;
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 841209548350..3d04842a2440 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -212,6 +212,7 @@ public:
>         void setIspControls(const ControlList &controls);
>         void setDelayedControls(const ControlList &controls, uint32_t delayContext);
>         void setLensControls(const ControlList &controls);
> +       void setCameraTimeout(uint32_t maxExposureTimeMs);
>         void setSensorControls(ControlList &controls);
>         void unicamTimeout();
>
> @@ -1166,14 +1167,6 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>                 }
>         }
>
> -       /*
> -        * Set the dequeue timeout to the larger of 2x the maximum possible
> -        * frame duration or 1 second.
> -        */
> -       utils::Duration timeout =
> -               std::max<utils::Duration>(1s, 2 * startConfig.maxSensorFrameLengthMs * 1ms);
> -       data->unicam_[Unicam::Image].dev()->setDequeueTimeout(timeout);
> -
>         return 0;
>  }
>
> @@ -1645,6 +1638,7 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)
>         ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
>         ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);
>         ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls);
> +       ipa_->setCameraTimeout.connect(this, &RPiCameraData::setCameraTimeout);
>
>         /*
>          * The configuration (tuning file) is made from the sensor name unless
> @@ -1957,6 +1951,20 @@ 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);
> +
> +       LOG(RPI, Debug) << "Setting Unicam timeout to " << timeout;
> +       unicam_[Unicam::Image].dev()->setDequeueTimeout(timeout);
> +}
> +
>  void RPiCameraData::setSensorControls(ControlList &controls)
>  {
>         /*
> --
> 2.25.1
>

Hi Naush

On Thu, Feb 23, 2023 at 12:49:55PM +0000, Naushir Patuck via libcamera-devel wrote:
> Date: Thu, 23 Feb 2023 12:49:55 +0000
> From: Naushir Patuck <naush@raspberrypi.com>
> To: libcamera-devel@lists.libcamera.org
> Subject: [PATCH v1 1/3] pipeline: ipa: raspberrypi: Change Unicam timeout
>  handling
> X-Mailer: git-send-email 2.25.1
>
> Add an explicit helper function setCameraTimeout() in the pipeline
> handler to set the Unicam timeout value. This function is signalled from
> the IPA to set up an appropriate timeout. This replaces the
> maxSensorFrameLengthMs value parameter returned back from
> IPARPi::start().
>
> Adjust the timeout to be 5x the maximum frame duration reported by the
> IPA.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks

   j
> ---
>  include/libcamera/ipa/raspberrypi.mojom       |  2 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           |  2 +-
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 24 ++++++++++++-------
>  3 files changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index 8e78f167f179..80e0126618c8 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -49,7 +49,6 @@ struct IPAConfigResult {
>  struct StartConfig {
>  	libcamera.ControlList controls;
>  	int32 dropFrameCount;
> -	uint32 maxSensorFrameLengthMs;
>  };
>
>  interface IPARPiInterface {
> @@ -132,4 +131,5 @@ interface IPARPiEventInterface {
>  	setIspControls(libcamera.ControlList controls);
>  	setDelayedControls(libcamera.ControlList controls, uint32 delayContext);
>  	setLensControls(libcamera.ControlList controls);
> +	setCameraTimeout(uint32 maxFrameLengthMs);
>  };
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 9b08ae4ca622..f6826bf27fe1 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -341,7 +341,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
>
>  	startConfig->dropFrameCount = dropFrameCount_;
>  	const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;
> -	startConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get<std::milli>();
> +	setCameraTimeout.emit(maxSensorFrameDuration.get<std::milli>());
>
>  	firstStart_ = false;
>  	lastRunTimestamp_ = 0;
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 841209548350..3d04842a2440 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -212,6 +212,7 @@ public:
>  	void setIspControls(const ControlList &controls);
>  	void setDelayedControls(const ControlList &controls, uint32_t delayContext);
>  	void setLensControls(const ControlList &controls);
> +	void setCameraTimeout(uint32_t maxExposureTimeMs);
>  	void setSensorControls(ControlList &controls);
>  	void unicamTimeout();
>
> @@ -1166,14 +1167,6 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>  		}
>  	}
>
> -	/*
> -	 * Set the dequeue timeout to the larger of 2x the maximum possible
> -	 * frame duration or 1 second.
> -	 */
> -	utils::Duration timeout =
> -		std::max<utils::Duration>(1s, 2 * startConfig.maxSensorFrameLengthMs * 1ms);
> -	data->unicam_[Unicam::Image].dev()->setDequeueTimeout(timeout);
> -
>  	return 0;
>  }
>
> @@ -1645,6 +1638,7 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)
>  	ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
>  	ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);
>  	ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls);
> +	ipa_->setCameraTimeout.connect(this, &RPiCameraData::setCameraTimeout);
>
>  	/*
>  	 * The configuration (tuning file) is made from the sensor name unless
> @@ -1957,6 +1951,20 @@ 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);
> +
> +	LOG(RPI, Debug) << "Setting Unicam timeout to " << timeout;
> +	unicam_[Unicam::Image].dev()->setDequeueTimeout(timeout);
> +}
> +
>  void RPiCameraData::setSensorControls(ControlList &controls)
>  {
>  	/*
> --
> 2.25.1
>