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

[Cheng-Hao Yang]

Hi Laurent,

On Fri, Aug 30, 2024 at 12:07 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

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

No worries. Thanks for the confirmation.


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

Actually, I might not :)
In mtkisp7, the proprietary libraries don't provide the control list, and
all the
controls are hard-coded in the match() function. I don't think Intel's
proprietary
libraries provide the info either.
Let's see though haha...

BR,
Harvey


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