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

[Naushir Patuck]

Hi Laurent,

Thank you for your feedback!

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

> Hi Naush,
>
> Thank you for the patch.
>
> On Thu, Mar 02, 2023 at 01:54:27PM +0000, Naushir Patuck via
> libcamera-devel 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.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.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(-)
> >
> > 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>());
>
> I'm not a great fan of adding more function calls between the pipeline
> handler and the IPA module, as they're costly when the IPA module runs
> in a separate process. While that's not expected to be a common use case
> on Raspberry Pi platforms, it would be best to still avoid the overhead
> if possible.
>

I understand the concern.  One alternative would be to pass it as a control
value (FrameDuration?) through the setDelayedControls signal that happens
roughly at the same time?  But then I'm overloading the meaning of
FrameDuration
control, which is not very tidy.  I'll leave things as-is for now.  There
is a
bit of an IPA-API rework happening in the background where I will look to
reduce
the inter-process calls.


>
> Please also note that this is one of the few cases where the camera
> manager event loop is run reentrantly. When the pipeline handler calls
> the start() function of the IPA module, it will wait synchronously for
> the call to complete, but re-enters the event loop to wait for the IPA
> to signal completion of the start() call by processing replies. The
> setCameraTimeout signal is emitted by the IPA module before the start()
> function returns, so from a pipeline handler point of view it will be
> processed while the start() call is in progress. I don't think there's
> any issue in this case as the connected slot shouldn't interfere with
> the pipeline handler start() function, but it could easily lead to
> unexpected behaviour later if you don't pay attention.
>

It does seem to work ;-) I will look to remove this setCameraTimeout()
signal as
part of the rework to avoid any future nasties.

Regards,
Naush


> >
> >       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)
>
> Note to self: we should really add support for duration types to the IPA
> parameters.
>
> In case you'd prefer not minimizing the number of function calls (as
> discussed above),
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +{
> > +     /*
> > +      * 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)
> >  {
> >       /*
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230307/a47d19b4/attachment.htm>