| Message ID | 20240820195016.16028-5-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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. BR, Harvey On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede <hdegoede@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@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 > >
Hi Hans, Thank you for the patch. On Tue, Aug 20, 2024 at 09:50:15PM +0200, Hans de Goede wrote: > Some pipeline handlers may want to open / close /dev/video# nodes > from pipe_->acquireDevices() / pipe_->releaseDevices(). See my reply to 3/5. I'm fine with doing so in the UVC pipeline handler now, but not in "some pipeline handlers" before we redesign the application-facing API. > 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. This will effectively serialize all calls to acquire() and release(), removing the need for the functions to be thread-safe. The locks will also not be needed anymore. Documentation should be updated. > Signed-off-by: Hans de Goede <hdegoede@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); >
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@redhat.com <mailto:hdegoede@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@redhat.com <mailto:hdegoede@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 >
Hi, On 8/25/24 2:58 AM, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Tue, Aug 20, 2024 at 09:50:15PM +0200, Hans de Goede wrote: >> Some pipeline handlers may want to open / close /dev/video# nodes >> from pipe_->acquireDevices() / pipe_->releaseDevices(). > > See my reply to 3/5. I'm fine with doing so in the UVC pipeline handler > now, but not in "some pipeline handlers" before we redesign the > application-facing API. Ack, I'll update the commit message for v2. >> 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. > > This will effectively serialize all calls to acquire() and release(), > removing the need for the functions to be thread-safe. The locks will > also not be needed anymore. Documentation should be updated. Good point about the locks no longer being needed because of the invokeMethod call serializing things. I'll drop the lock_ and I'll update the \context of acquire[Device]() and release[Device]() to match. Regards, Hans >> Signed-off-by: Hans de Goede <hdegoede@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); >> >
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);
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@redhat.com> --- src/libcamera/camera.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)