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

[Naushir Patuck]

Hi Laurent,


On Tue, 15 Mar 2022 at 08:03, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> 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 agree, this may not be the best approach then.


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

I'm actually coming round to the idea that the pipeline handler has an
internal
timeout that is used to free the buffers.  Partly because I need a timeout
functionality for something entirely unrelated :)  But it does remove the
decision
from the application side. I'll prototype something along these lines to
see
how it works.


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

It would certainly help, but is the extra complexity worth the savings on
allocation
time, I can't say...

Regards,
Naush


>
> > > > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220317/9a4bf97f/attachment-0001.htm>