[libcamera-devel] [PATCH v1 3/6] pipeline: raspberrypi: Free buffers in the RPiCamera destructor and re-configure

[Laurent Pinchart]

Hi Naush,

On Mon, Mar 14, 2022 at 11:13:44AM +0000, Naushir Patuck wrote:
> On Sun, 13 Mar 2022 at 16:02, Laurent Pinchart wrote:
> > On Mon, Mar 07, 2022 at 12:46:30PM +0000, Naushir Patuck via
> > libcamera-devel wrote:
> > > Currently, all framebuffer allocations get freed and cleared on a stop in
> > > PipelineHandlerRPi::stopDevice(). If PipelineHandlerRPi::start() is then called
> > > without an intermediate PipelineHandlerRPi::configure(), it will re-allocate and
> > > prepare all the buffers again, which is unnecessary.
> > >
> > > Fix this by not freeing the buffer in PipelineHandlerRPi::stopDevice(), but
> > > insted doing it in PipelineHandlerRPi::configure(), as the buffers might have
> > > to be resized.
> >
> > I see where you're going, but doesn't this mean that buffers will stay
> > allocated forever ? If an application uses a camera for some time and
> > then stops it, memory won't be released until the application
> > terminates. That's trading an issue for another one, and which one is
> > worse really depends on the use case.
> >
> > There are (at least) two ways to address this. The simplest one would be
> > to fire a timer at stop() time, and free buffers when it elapses. The
> > timer would be cancelled if the camera is restarted first.
> >
> > The second option is to make this controllable by the application. We
> > could hook it up to the Camera::release() call for instance, adding a
> > new pipeline handler operation to handle it. release() may not be the
> > best option though, maybe we need a new cleanup function. Or maybe an
> > argument to stop(), to tell if the camera is expected to be restarted
> > soon ? I haven't really thought about the pros and cons of the different
> > options, I'm just brainstorming here.
> 
> Yes, I do see a possible issue here with holding onto buffers for longer than
> expected.  My preferred option would be to have a Camera::release() call
> that explicitly requests the pipeline handler to remove all buffer allocations.
> That way, the application controls the intended behavior if it chooses to, but
> the pipeline handler keeps buffers allocated otherwise.

The downside of tying this to Camera::release() is that the application
will need to let go of exclusive ownership of the camera to free the
buffers. Someone else could then acquire() it. It's probably a corner
case, but I'd prefer not hardcoding this limitation in the API.

I'm also wondering if adding a hint flag to stop() to tell if streaming
will soon tbe restarted could also have other use cases. A pipeline
handler could avoid powering down hardware for instance, to decrease the
time needed when restarting (although with recent drivers PM is handled
using runtime PM in the sensor driver, with auto-suspend, so the sensor
itself won't benefit much).

While brainstorming on this topic, would it be useful to have a mean to
allocate internal buffers large enough for different resolutions, and
use them in different capture cycles instead of reallocating ?

> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 3781e4e0e3c4..8bb9fc429912 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -189,6 +189,11 @@ public:
> > >       {
> > >       }
> > >
> > > +     ~RPiCameraData()
> > > +     {
> > > +             freeBuffers();
> > > +     }
> > > +
> > >       void freeBuffers();
> > >       void frameStarted(uint32_t sequence);
> > >
> > > @@ -681,7 +686,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > >       RPiCameraData *data = cameraData(camera);
> > >       int ret;
> > >
> > > -     /* Start by resetting the Unicam and ISP stream states. */
> > > +     /* Start by freeing all buffers and resetting the Unicam and ISP stream states. */
> > > +     data->freeBuffers();
> > >       for (auto const stream : data->streams_)
> > >               stream->reset();
> > >
> > > @@ -1048,8 +1054,6 @@ void PipelineHandlerRPi::stopDevice(Camera *camera)
> > >
> > >       /* Stop the IPA. */
> > >       data->ipa_->stop();
> > > -
> > > -     data->freeBuffers();
> > >  }
> > >
> > >  int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)

-- 
Regards,

Laurent Pinchart