[PATCH 2/5] pipeline_handler: Call releaseDevice() before unlocking media devices

Thu Aug 29 23:34:13 CEST 2024

Hi Laurent,

On Thu, Aug 29, 2024 at 10:59 PM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> On Sun, Aug 25, 2024 at 01:21:21PM +0200, Cheng-Hao Yang wrote:
> > On Sun, Aug 25, 2024 at 2:27 AM Laurent Pinchart wrote:
> > > On Wed, Aug 21, 2024 at 06:34:17PM +0200, Cheng-Hao Yang wrote:
> > > > Hi Hans,
> > > >
> > > > Thanks for the patch. I've tested it on mtkisp7, and it works fine.
> > > >
> > > > Reviewed-by: Harvey Yang <chenghaoyang at chromium.org>
> > > >
> > > > One question that might not be related though:
> > > > `PipelineHandler::release` is defined to be thread safe, while
> > > > there's no such description for `PipelineHandler::releaseDevice`.
> > > >
> > > > In mtkisp7, we have a CL [1] to make sure
> > > > `PipelineHandler::releaseDevice` is always blockingly executed
> > > > on  PipelineHandler's thread.
> > > >
> > > > Although it makes the normal cases (that it's returning true
> > > > directly) run longer, as it waits for another thread's execution,
> > > > it prevents PipelineHandler's mis-implementation.
> > > > I'd suggest either we do the same in the upstream, or we add
> > > > the description of being thread-safe for
> > > > `PipelineHandler::releaseDevice` to remind the developers of
> > > > pipeline handlers.
> > >
> > > I don't think releaseDevice() qualifies as thread safe. Here's how
> > > libcamera documents thread safety:
> > >
> > >  * - A **reentrant** function may be called simultaneously from
> multiple
> > >  *   threads if and only if each invocation uses a different instance
> of the
> > >  *   class. This is the default for all member functions not explictly
> marked
> > >  *   otherwise.
> > >  *
> > >  * - \anchor thread-safe A **thread-safe** function may be called
> > >  *   simultaneously from multiple threads on the same instance of a
> class. A
> > >  *   thread-safe function is thus reentrant. Thread-safe functions may
> also be
> > >  *   called simultaneously with any other reentrant function of the
> same class
> > >  *   on the same instance.
> > >
> > > Pipeline handlers shouldn't have to deal with releaseDevice() being
> > > called concurrently on the same PipelineHandler instance for different
> > > cameras, which is why the caller serializes the calls.
> > >
> > > releaseDevice() should never race with other pipeline handler calls for
> > > the same camera. However, it could race with other calls for different
> > > cameras. We should document this, or change the behaviour.
> >
> > Yes, you're right. Thanks for the clarification.
> >
> > > > WDYT?
> > > >
> > > > BR,
> > > > Harvey
> > > >
> > > > [1]:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5528925
> > >
> > > The commit message doesn't really explain why the change is needed.
> > > Could you elaborate, maybe providing an example of incorrect behaviour
> > > of a pipeline handler implementation with the existing calling
> > > convention ?
> >
> > The main issue that CrOS wants to solve is: We use acquire() & release()
> > to handle the IPA (proxy & the sandboxed process)'s lifetime [1][2].
> Within the
> > current libcamera API, the IPA should be active when configure() is
> called,
> > and therefore it doesn't make sense to terminate the IPA (and release
> some
> > DMA buffers) when the camera is stopped.
> > acquire() and release() are the best places to construct & destruct the
> IPA
> > proxy & sandboxed process.
> > The reason that we want to destruct the IPA proxy at some point is that
> it's
> > the easiest way to clean up proprietary libraries' memory usage, which is
> > different from how ipu3 works now. ipu3 creates the proxy in match() and
> > never destructs it.
>
> I'd argue that proprietary libraries are the problem here, but that's a
> longer term problem.
>
> > [1]:
> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1290
> > [2]:
> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1298
> >
> > However, IIUC, an IPA proxy needs to be constructed, used, and destructed
> > on the same thread. (Please correct me if I'm wrong.)
>
> They are thread-bound, which means they indeed have to be used and
> destroyed in the same thread. Construction can happen in another thread
> if the objects are then moved to the pipeline handler thread with
> moveToThread(), but it's easier to just construct the objects in the
> same thread.
>
> > During the development, we spent some time debugging this issue.
> > I think ipu3's proxy doesn't run into such an issue because it uses
> InThread mode?
>

Just realized that ipu3's proxy is created in `PipelineHandler::match`,
which is called on the pipeline handler thread, and it's never destructed.

> > Therefore, I think it'd be better to at least leave comments to remind
> developers
> > that releaseDevice() (and the potential acquireDevice()) might be called
> from
> > any thread.
> >
> > I've just tried to call releaseDevice() directly in release() without
> switching the
> > thread, and got a FATAL error:
> > ```
> >
> > FATAL default event_dispatcher_poll.cpp:285 assertion "iter !=
> > notifiers_.end()" failed in processNotifiers()
> > ```
> >
> > Please also check if constructing the IPA proxy in acquireDevice() makes
> > sense.
>
> In the context of the problem you mention above, yes, it does make sense
> (until we can stop dealing with memory leaks from closed-source blobs).
> However, it introduces a problem. The IPA module is responsible for
> advertising the controls it supports, and applications can expect from
> the current API to list those controls before acquiring a camera. That's
> a conflicting requirement, how do you think we should address it ?
>

Right. I think the easiest way is to construct the IPA proxy during
`PipelineHandler::match`, where a camera is registered, and choose to
destruct it right after using it to list controls.

It can also be kept there, if proprietary libraries don't consume lots of
memory just to list controls before starting to process statistics. The
`acquireDevice()` would need to check if an IPA proxy already exists
or not though.

> > > On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede wrote:
> > > >
> > > > > It is better / more logical to call releaseDevice() before
> unlocking
> > > > > the devices. ATM the only pipeline handler implementing
> releaseDevice()
> > >
> > > s/ATM/At the moment/
> > >
> > > > > is the rpi pipeline handler which releases buffers from its
> releaseDevice()
> > > > > implementation.
> > > > >
> > > > > Releasing buffers before unlocking the media devices is ok to do
> > > > > and arguably it is better to release the buffers before unlocking.
> > > > >
> > > > > Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >
> > > > > ---
> > > > >  src/libcamera/pipeline_handler.cpp | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/src/libcamera/pipeline_handler.cpp
> > > > > b/src/libcamera/pipeline_handler.cpp
> > > > > index a20cd27d..1fc22d6a 100644
> > > > > --- a/src/libcamera/pipeline_handler.cpp
> > > > > +++ b/src/libcamera/pipeline_handler.cpp
> > > > > @@ -205,11 +205,11 @@ void PipelineHandler::release(Camera *camera)
> > > > >
> > > > >         ASSERT(useCount_);
> > > > >
> > > > > +       releaseDevice(camera);
> > > > > +
> > > > >         if (useCount_ == 1)
> > > > >                 unlockMediaDevices();
> > > > >
> > > > > -       releaseDevice(camera);
> > > > > -
> > > > >         --useCount_;
> > > > >  }
> > > > >
>
> --
> Regards,
>
> Laurent Pinchart
>

BR,
Harvey
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240829/69f70870/attachment.htm>