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

[Laurent Pinchart]

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.

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

> 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