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

[Laurent Pinchart]

On Thu, Aug 29, 2024 at 11:34:13PM +0200, Cheng-Hao Yang wrote:
> On Thu, Aug 29, 2024 at 10:59 PM Laurent Pinchart 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.

Yes that's the reason. Sorry, I forgot to reply to your question.

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

Those could be options, yes. Neither sound optimal, but I'm not sure if
there's a better solution. Let's sleep over this. I'd also be happy to
forget about the problem, but I'm sure you'll remind me with patches :-)

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