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

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Tue, Mar 07, 2023 at 09:11:09AM +0000, Naushir Patuck wrote:
> On Mon, 6 Mar 2023 at 17:11, Laurent Pinchart wrote:
> > 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.

Yes, that's not great. I think a better option would be to add a
name parameter to the setDelayedControls signal (or any other signal
that is periodically emitted with the right timings).

> 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.

Thanks.

> > >
> > >       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