[libcamera-devel] [PATCH v2 5/6] pipeline: raspberrypi: Add a timeout to free buffers on stopDevice()

[Naushir Patuck]

Hi David,

On Thu, 17 Mar 2022 at 14:32, David Plowman <david.plowman at raspberrypi.com>
wrote:

> Hi Naush
>
> Thanks for the patch!
>
> Perhaps I'm coming a bit late to this particular discussion, but this
> makes me a bit nervous. I'm worried about deallocations/allocations
> happening at unpredictable moments relative to other things in the
> system. Could there be a risk of creating more intermittent and
> hard-to-reproduce failures?
>

> One example is the Pi 3 where I think we know that CMA fragmentation
> is already a problem. I wonder if I would rather be able to control
> when/whether buffers get freed? But it does make the API more involved
> and leaves an application with more to think about.
>

Yes, this is a distinct possibility.  I'm fairly confident this will not be
an issue with libcamera-apps,
as the timeout will never hit.  However, picamera2 can certainly invoke the
timeout and
cause issues.  Hmmm.... not sure which way to go here.... perhaps an
application flag
would indeed be better.

Naush


>
> Possibly I'm just worrying too much?
>
> Thanks!
> David
>
> On Thu, 17 Mar 2022 at 14:08, Naushir Patuck via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
> >
> > Start a 5 second timer when stopDevice() is called, and if it times out,
> > free all internally allocated buffers. The timer is cleared when start()
> is
> > subsequently called.
> >
> > This avoids the pipeline handler from holding onto internal buffers for
> long
> > periods of time due to application inactivity.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp        | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index d3212b193ced..32b282b4bdbd 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -15,6 +15,7 @@
> >  #include <utility>
> >
> >  #include <libcamera/base/shared_fd.h>
> > +#include <libcamera/base/timer.h>
> >  #include <libcamera/base/utils.h>
> >
> >  #include <libcamera/camera.h>
> > @@ -45,6 +46,8 @@
> >  #include "dma_heaps.h"
> >  #include "rpi_stream.h"
> >
> > +using namespace std::literals::chrono_literals;
> > +
> >  namespace libcamera {
> >
> >  LOG_DEFINE_CATEGORY(RPI)
> > @@ -288,6 +291,9 @@ public:
> >         /* Has this camera been reconfigured? */
> >         bool reallocate_;
> >
> > +       /* Timer to free internal buffers once stopDevice() has been
> called. */
> > +       Timer stopTimer_;
> > +
> >  private:
> >         void checkRequestCompleted();
> >         void fillRequestMetadata(const ControlList &bufferControls,
> > @@ -991,6 +997,10 @@ int PipelineHandlerRPi::start(Camera *camera, const
> ControlList *controls)
> >         RPiCameraData *data = cameraData(camera);
> >         int ret;
> >
> > +       /* Disable the stop timeout waiting to release buffers after a
> stop(). */
> > +       data->stopTimer_.stop();
> > +       data->stopTimer_.timeout.disconnect();
> > +
> >         for (auto const stream : data->streams_)
> >                 stream->resetBuffers();
> >
> > @@ -1071,6 +1081,13 @@ void PipelineHandlerRPi::stopDevice(Camera
> *camera)
> >
> >         /* Stop the IPA. */
> >         data->ipa_->stop();
> > +
> > +       /*
> > +        * Start a 5 second timer once the camera is stopped, and on a
> timeout,
> > +        * release all internally allocated buffers.
> > +        */
> > +       data->stopTimer_.timeout.connect(data,
> &RPiCameraData::freeBuffers);
> > +       data->stopTimer_.start(5s);
> >  }
> >
> >  int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request
> *request)
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220317/ac0e3922/attachment-0001.htm>