[libcamera-devel] [PATCH 1/2] libcamera: Add a PipelineHandler::releaseDevice method

[Kieran Bingham]

Quoting David Plowman via libcamera-devel (2022-11-15 10:37:46)
> Hi Laurent
> 
> Thanks for the comments. I see that this patch is already merged so
> please let me know how you'd like to proceed.
> 
> On Mon, 14 Nov 2022 at 16:56, Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> >
> > Hi David,
> >
> > Thank you for the patch.
> >
> > On Fri, Nov 11, 2022 at 01:30:24PM +0000, David Plowman via libcamera-devel wrote:
> > > This notifies pipeline handlers when a camera is released, in case
> > > they want to free any resources or memory buffers.
> > >
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  include/libcamera/internal/pipeline_handler.h |  4 +++-
> > >  src/libcamera/camera.cpp                      |  2 +-
> > >  src/libcamera/pipeline_handler.cpp            | 16 +++++++++++++++-
> >
> > The pipeline handler writer's guide
> > (Documentation/guides/pipeline-handler.rst) needs to be udpated. The
> > vivid pipeline handler may also need some attention, but we can handle
> > that.
> 
> Thank you!
> 
> >
> > >  3 files changed, 19 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > > index 96aab9d6..ec4f662d 100644
> > > --- a/include/libcamera/internal/pipeline_handler.h
> > > +++ b/include/libcamera/internal/pipeline_handler.h
> > > @@ -46,7 +46,7 @@ public:
> > >                                       const DeviceMatch &dm);
> > >
> > >       bool acquire();
> > > -     void release();
> > > +     void release(Camera *camera);
> > >
> > >       virtual std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > >               const StreamRoles &roles) = 0;
> > > @@ -74,6 +74,8 @@ protected:
> > >       virtual int queueRequestDevice(Camera *camera, Request *request) = 0;
> > >       virtual void stopDevice(Camera *camera) = 0;
> > >
> > > +     virtual void releaseDevice(Camera *camera);
> > > +
> > >       CameraManager *manager_;
> > >
> > >  private:
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index c4f65c1a..2d947a44 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -870,7 +870,7 @@ int Camera::release()
> > >               return ret == -EACCES ? -EBUSY : ret;
> > >
> > >       if (d->isAcquired())
> > > -             d->pipe_->release();
> > > +             d->pipe_->release(this);
> > >
> > >       d->setState(Private::CameraAvailable);
> > >
> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > > index 825aff5a..cfade490 100644
> > > --- a/src/libcamera/pipeline_handler.cpp
> > > +++ b/src/libcamera/pipeline_handler.cpp
> > > @@ -183,6 +183,7 @@ bool PipelineHandler::acquire()
> > >
> > >  /**
> > >   * \brief Release exclusive access to the pipeline handler
> > > + * \param[in] camera The camera for which to release data
> >
> > This function doesn't release "data".
> 
> Sorry, I was using the word "data" generically. Perhaps "resources"?
> Let me know if you'd like me to update the patch.
> 
> >
> > >   *
> > >   * This function releases access to the pipeline handler previously acquired by
> > >   * a call to acquire(). Every release() call shall match a previous successful
> > > @@ -196,7 +197,7 @@ bool PipelineHandler::acquire()
> > >   *
> > >   * \sa acquire()
> > >   */
> > > -void PipelineHandler::release()
> > > +void PipelineHandler::release(Camera *camera)
> > >  {
> > >       MutexLocker locker(lock_);
> > >
> > > @@ -204,9 +205,22 @@ void PipelineHandler::release()
> > >
> > >       unlockMediaDevices();
> > >
> > > +     releaseDevice(camera);
> > > +
> > >       --useCount_;
> > >  }
> > >
> > > +/**
> > > + * \brief Release resources associated with this camera
> > > + * \param[in] camera The camera for which to release resources
> > > + *
> > > + * Pipeline handlers may override this in order to perform cleanup operations
> > > + * when a camera is released, such as freeing memory.
> >
> > This needs further documentation, to clearly explain what can or can't
> > be done here. Examples could help.
> >
> > This being said, I'm not sure hooking up this feature in the
> > Camera::release() function is the right option. In addition to the lack
> > of balance between acquire() and release() (in this series the release()
> > function does more than counterbalance acquire()), the usage pattern
> > that was envisioned is that application can hold onto cameras for a long
> > time. Having to call release() to free memory means that another
> > application could then acquire the camera.
> 
> I did wonder about "acquire", but it seemed less urgent as you always
> find out if someone really wants to use the camera! But anyway, I see
> that Kieran is doing work in this area now.
> 
> I guess I might be a bit confused about the purpose of "release". I
> expected the idea would be that other applications could then use it,
> but maybe there are 2 levels of "releasing"? One really means
> "release" (and someone else can have it). The other is perhaps closer
> to "disable it" (but don't let anyone else have it)? Does that sound
> right?

In my understanding, a Camera is created when an application launches
the CameraManager. Those Camera's exist and have been initialised, so if
that process is long living (like a Camera Daemon, or Pipewire), those
objects will persist. But I don't think they should acquire or lock
resources until an explicit call to acquire() has occurred.

It may be unlikely that a Pipeline should need to allocate memory during
require, but I'm sure that during release, they should release anything
that was allocated additionally during configure()!

It would be nicer to keep the base implementation symetrical I think, so
an equivalent acquire() could be passed to the pipeline handler
(optionally) ... but it does depend on it being needed. I tried only
opening the video node for UVC on acquire (or rather, open it during
init(), obtain information required, then close it until acquire() ),
and hit (unexpected) issues.

I think I'll just post the patches anyway, with a [DNI] tag on the UVC
part to share. Maybe it will be obvious to someone what either I've done
wrong, or what the bug is.

--
Kieran



> 
> Thanks!
> David
> 
> >
> > > + */
> > > +void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)
> > > +{
> > > +}
> > > +
> > >  void PipelineHandler::unlockMediaDevices()
> > >  {
> > >       for (std::shared_ptr<MediaDevice> &media : mediaDevices_)
> >
> > --
> > Regards,
> >
> > Laurent Pinchart