From patchwork Wed Aug 25 21:18:50 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nicolas Dufresne X-Patchwork-Id: 13494 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 0C4F2BD87D for ; Wed, 25 Aug 2021 21:19:15 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id D537968893; Wed, 25 Aug 2021 23:19:14 +0200 (CEST) Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 03F9760288 for ; Wed, 25 Aug 2021 23:19:13 +0200 (CEST) Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: nicolas) with ESMTPSA id 49C0E1F435D8 From: Nicolas Dufresne To: libcamera-devel@lists.libcamera.org Date: Wed, 25 Aug 2021 17:18:50 -0400 Message-Id: <20210825211852.1207168-2-nicolas@ndufresne.ca> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20210825211852.1207168-1-nicolas@ndufresne.ca> References: <20210825211852.1207168-1-nicolas@ndufresne.ca> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v1 1/3] gstreamer: Fix deadlock when last allocator ref is held by buffer X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Nicolas Dufresne Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" From: Nicolas Dufresne This deadlock occurs when a buffer is holding the last reference on the allocator. In gst_libcamera_allocator_release() we must drop the object lock before dropping the last ref of that object since the destructor will lock it again causing deadlock. This was notice while switching camera or resolution in Cheese software. Signed-off-by: Nicolas Dufresne Reviewed-by: Laurent Pinchart --- src/gstreamer/gstlibcameraallocator.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp index 7bd8ba2d..a8fa4f86 100644 --- a/src/gstreamer/gstlibcameraallocator.cpp +++ b/src/gstreamer/gstlibcameraallocator.cpp @@ -108,15 +108,18 @@ gst_libcamera_allocator_release(GstMiniObject *mini_object) { GstMemory *mem = GST_MEMORY_CAST(mini_object); GstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(mem->allocator); - GLibLocker lock(GST_OBJECT(self)); - auto *frame = reinterpret_cast(gst_mini_object_get_qdata(mini_object, FrameWrap::getQuark())); - gst_memory_ref(mem); + { + GLibLocker lock(GST_OBJECT(self)); + auto *frame = reinterpret_cast(gst_mini_object_get_qdata(mini_object, FrameWrap::getQuark())); + + gst_memory_ref(mem); - if (frame->releasePlane()) { - auto *pool = reinterpret_cast(g_hash_table_lookup(self->pools, frame->stream_)); - g_return_val_if_fail(pool, TRUE); - g_queue_push_tail(pool, frame); + if (frame->releasePlane()) { + auto *pool = reinterpret_cast(g_hash_table_lookup(self->pools, frame->stream_)); + g_return_val_if_fail(pool, TRUE); + g_queue_push_tail(pool, frame); + } } /* Keep last in case we are holding on the last allocator ref. */ From patchwork Wed Aug 25 21:18:51 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nicolas Dufresne X-Patchwork-Id: 13495 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 57469BD87D for ; Wed, 25 Aug 2021 21:19:16 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 18BBC688C4; Wed, 25 Aug 2021 23:19:16 +0200 (CEST) Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 8105C60288 for ; Wed, 25 Aug 2021 23:19:15 +0200 (CEST) Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: nicolas) with ESMTPSA id C830D1F435D8 From: Nicolas Dufresne To: libcamera-devel@lists.libcamera.org Date: Wed, 25 Aug 2021 17:18:51 -0400 Message-Id: <20210825211852.1207168-3-nicolas@ndufresne.ca> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20210825211852.1207168-1-nicolas@ndufresne.ca> References: <20210825211852.1207168-1-nicolas@ndufresne.ca> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v1 2/3] gstreamer: Fix concurrent access issues to CameraManager X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Nicolas Dufresne Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" From: Nicolas Dufresne It's not allowed to have multiple instances of CameraManager. This requirement is not easy for GStreamer were the device monitor and the camerasrc, or two camerasrc instances don't usually have any interaction between each other. This this by implementing a minimalist singleton around CameraManager constructor and start()/stop() operations. Signed-off-by: Nicolas Dufresne --- src/gstreamer/gstlibcamera-utils.cpp | 31 ++++++++++++++++++++++++++ src/gstreamer/gstlibcamera-utils.h | 6 +++-- src/gstreamer/gstlibcameraprovider.cpp | 22 ++---------------- src/gstreamer/gstlibcamerasrc.cpp | 11 +++++---- 4 files changed, 42 insertions(+), 28 deletions(-) diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp index 007d6a64..34b64c4a 100644 --- a/src/gstreamer/gstlibcamera-utils.cpp +++ b/src/gstreamer/gstlibcamera-utils.cpp @@ -221,3 +221,34 @@ gst_libcamera_resume_task(GstTask *task) GST_TASK_SIGNAL(task); } } + +G_LOCK_DEFINE_STATIC(cm_singleton_lock); +std::weak_ptr cm_singleton_ptr; + +void _cm_deleter(CameraManager *cm) +{ + g_print("Delete CameraManager\n"); + cm->stop(); + delete cm; +} + +std::shared_ptr +gst_libcamera_get_camera_mananger(int &ret) +{ + std::shared_ptr cm; + + G_LOCK(cm_singleton_lock); + + cm = cm_singleton_ptr.lock(); + if (cm_singleton_ptr.expired()) { + std::shared_ptr ptr(new CameraManager(), _cm_deleter); + cm_singleton_ptr = cm = ptr; + ret = cm->start(); + } else { + ret = 0; + } + + G_UNLOCK(cm_singleton_lock); + + return cm; +} diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h index 2b3f26b6..67a06db3 100644 --- a/src/gstreamer/gstlibcamera-utils.h +++ b/src/gstreamer/gstlibcamera-utils.h @@ -9,16 +9,18 @@ #ifndef __GST_LIBCAMERA_UTILS_H__ #define __GST_LIBCAMERA_UTILS_H__ +#include +#include + #include #include -#include - GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats); GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg); void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, GstCaps *caps); void gst_libcamera_resume_task(GstTask *task); +std::shared_ptr gst_libcamera_get_camera_mananger(int &ret); /** * \class GLibLocker diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp index 29da6c32..948ba0d1 100644 --- a/src/gstreamer/gstlibcameraprovider.cpp +++ b/src/gstreamer/gstlibcameraprovider.cpp @@ -158,7 +158,6 @@ gst_libcamera_device_new(const std::shared_ptr &camera) struct _GstLibcameraProvider { GstDeviceProvider parent; - CameraManager *cm; }; G_DEFINE_TYPE_WITH_CODE(GstLibcameraProvider, gst_libcamera_provider, @@ -170,7 +169,7 @@ static GList * gst_libcamera_provider_probe(GstDeviceProvider *provider) { GstLibcameraProvider *self = GST_LIBCAMERA_PROVIDER(provider); - CameraManager *cm = self->cm; + std::shared_ptr cm; GList *devices = nullptr; gint ret; @@ -181,7 +180,7 @@ gst_libcamera_provider_probe(GstDeviceProvider *provider) * gains monitoring support. Meanwhile we need to cycle start()/stop() * to ensure every probe() calls return the latest list. */ - ret = cm->start(); + cm = gst_libcamera_get_camera_mananger(ret); if (ret) { GST_ERROR_OBJECT(self, "Failed to retrieve device list: %s", g_strerror(-ret)); @@ -194,8 +193,6 @@ gst_libcamera_provider_probe(GstDeviceProvider *provider) g_object_ref_sink(gst_libcamera_device_new(camera))); } - cm->stop(); - return devices; } @@ -204,31 +201,16 @@ gst_libcamera_provider_init(GstLibcameraProvider *self) { GstDeviceProvider *provider = GST_DEVICE_PROVIDER(self); - self->cm = new CameraManager(); - /* Avoid devices being duplicated. */ gst_device_provider_hide_provider(provider, "v4l2deviceprovider"); } -static void -gst_libcamera_provider_finalize(GObject *object) -{ - GstLibcameraProvider *self = GST_LIBCAMERA_PROVIDER(object); - gpointer klass = gst_libcamera_provider_parent_class; - - delete self->cm; - - return G_OBJECT_CLASS(klass)->finalize(object); -} - static void gst_libcamera_provider_class_init(GstLibcameraProviderClass *klass) { GstDeviceProviderClass *provider_class = GST_DEVICE_PROVIDER_CLASS(klass); - GObjectClass *object_class = G_OBJECT_CLASS(klass); provider_class->probe = gst_libcamera_provider_probe; - object_class->finalize = gst_libcamera_provider_finalize; gst_device_provider_class_set_metadata(provider_class, "libcamera Device Provider", diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 4c7b36ae..bdd9f88c 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -104,7 +104,7 @@ GstBuffer *RequestWrap::detachBuffer(Stream *stream) struct GstLibcameraSrcState { GstLibcameraSrc *src_; - std::unique_ptr cm_; + std::shared_ptr cm_; std::shared_ptr cam_; std::unique_ptr config_; std::vector srcpads_; @@ -198,13 +198,13 @@ GstLibcameraSrcState::requestCompleted(Request *request) static bool gst_libcamera_src_open(GstLibcameraSrc *self) { - std::unique_ptr cm = std::make_unique(); + std::shared_ptr cm; std::shared_ptr cam; - gint ret = 0; + gint ret; GST_DEBUG_OBJECT(self, "Opening camera device ..."); - ret = cm->start(); + cm = gst_libcamera_get_camera_mananger(ret); if (ret) { GST_ELEMENT_ERROR(self, LIBRARY, INIT, ("Failed listing cameras."), @@ -250,7 +250,7 @@ gst_libcamera_src_open(GstLibcameraSrc *self) cam->requestCompleted.connect(self->state, &GstLibcameraSrcState::requestCompleted); /* No need to lock here, we didn't start our threads yet. */ - self->state->cm_ = std::move(cm); + self->state->cm_ = cm; self->state->cam_ = cam; return true; @@ -517,7 +517,6 @@ gst_libcamera_src_close(GstLibcameraSrc *self) } state->cam_.reset(); - state->cm_->stop(); state->cm_.reset(); } From patchwork Wed Aug 25 21:18:52 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nicolas Dufresne X-Patchwork-Id: 13496 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id DAB78BD87D for ; Wed, 25 Aug 2021 21:19:18 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id A80EF68891; Wed, 25 Aug 2021 23:19:18 +0200 (CEST) Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 4BAC9688AE for ; Wed, 25 Aug 2021 23:19:17 +0200 (CEST) Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: nicolas) with ESMTPSA id 895081F435D8 From: Nicolas Dufresne To: libcamera-devel@lists.libcamera.org Date: Wed, 25 Aug 2021 17:18:52 -0400 Message-Id: <20210825211852.1207168-4-nicolas@ndufresne.ca> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20210825211852.1207168-1-nicolas@ndufresne.ca> References: <20210825211852.1207168-1-nicolas@ndufresne.ca> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v1 3/3] libcamerasrc: Fix deadlock on EOS X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Nicolas Dufresne Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" From: Nicolas Dufresne It's not allowed in GStreamer to push events while holding the object lock. This reduce the scope into which we hold the object lock. In fact we don't need to protect against gst_task_resume() concurrency when we stop the task as resume only do something if the task is paused. This fixes a deadlock when running multiple instances of libcamerasrc and closing one of the streaming window. Signed-off-by: Nicolas Dufresne Reviewed-by: Laurent Pinchart --- src/gstreamer/gstlibcamerasrc.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index bdd9f88c..2be44edd 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -312,12 +312,6 @@ gst_libcamera_src_task_run(gpointer user_data) } { - /* - * Here we need to decide if we want to pause or stop the task. This - * needs to happen in lock step with the callback thread which may want - * to resume the task. - */ - GLibLocker lock(GST_OBJECT(self)); if (ret != GST_FLOW_OK) { if (ret == GST_FLOW_EOS) { g_autoptr(GstEvent) eos = gst_event_new_eos(); @@ -332,6 +326,12 @@ gst_libcamera_src_task_run(gpointer user_data) return; } + /* + * Here we need to decide if we want to pause. This needs to + * happen in lock step with the callback thread which may want + * to resume the task and might push pending buffers. + */ + GLibLocker lock(GST_OBJECT(self)); bool do_pause = true; for (GstPad *srcpad : state->srcpads_) { if (gst_libcamera_pad_has_pending(srcpad)) {