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

[David Plowman]

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.

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
>