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

[David Plowman]

Hi Naush

Thanks for the patch!

On Thu, 23 Feb 2023 at 12:49, Naushir Patuck via libcamera-devel
<libcamera-devel at 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 at raspberrypi.com>

Reviewed-by: David Plowman <david.plowman at 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
>