[PATCH 4/5] camera: Use invokeMethod() for pipe_->acquire() and pipe_->release()

[Hans de Goede]

Hi Harvey,

On 8/21/24 7:32 PM, Cheng-Hao Yang wrote:
> Hi Hans,
> 
> Ah I left the comments on the 1st/5 patch too early. Should've checked all
> patches in this series first.
> 
> IIUC, the users of libcamera should only get access to `libcamera::Camera`'s
> APIs, which are the only callers of `libcamera::PipelineHandler`'s APIs.
> If this is correct, I agree that we don't need to make `PipelineHandler::acquire`
> and `PipelineHandler::release` thread-safe.
> 
> If we go with this patch, we can remove `PipelineHandler::lock_`, and the
> descriptions of the two functions should be updated.

`PipelineHandler::lock_` is still useful when PipelineHandler::acquire() /
PipelineHandler::release() are called at the same time from different
threads for 2 different cameras.

When this happens we still need the lock to make sure the useCount_ variable
updating works properly.

Regards,

Hans



> On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede <hdegoede at redhat.com <mailto:hdegoede at redhat.com>> wrote:
> 
>     Some pipeline handlers may want to open / close /dev/video# nodes
>     from pipe_->acquireDevices() / pipe_->releaseDevices().
> 
>     V4L2VideoDevice::open() creates an EventNotifier, this notifier needs
>     to be created from the CameraManager thread.
> 
>     Use invokeMethod() for pipe_->acquire() and pipe_->release() so that
>     any EventNotifiers created are created from the CameraManager thread
>     context.
> 
>     Signed-off-by: Hans de Goede <hdegoede at redhat.com <mailto:hdegoede at redhat.com>>
>     ---
>      src/libcamera/camera.cpp | 6 ++++--
>      1 file changed, 4 insertions(+), 2 deletions(-)
> 
>     diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>     index 4e393f89..61925e83 100644
>     --- a/src/libcamera/camera.cpp
>     +++ b/src/libcamera/camera.cpp
>     @@ -995,7 +995,8 @@ int Camera::acquire()
>             if (ret < 0)
>                     return ret == -EACCES ? -EBUSY : ret;
> 
>     -       if (!d->pipe_->acquire(this)) {
>     +       if (!d->pipe_->invokeMethod(&PipelineHandler::acquire,
>     +                                   ConnectionTypeBlocking, this)) {
>                     LOG(Camera, Info)
>                             << "Pipeline handler in use by another process";
>                     return -EBUSY;
>     @@ -1030,7 +1031,8 @@ int Camera::release()
>                     return ret == -EACCES ? -EBUSY : ret;
> 
>             if (d->isAcquired())
>     -               d->pipe_->release(this);
>     +               d->pipe_->invokeMethod(&PipelineHandler::release,
>     +                                      ConnectionTypeBlocking, this);
> 
>             d->setState(Private::CameraAvailable);
> 
>     -- 
>     2.46.0
>