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

[David Plowman]

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?

Thanks!
David

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