From patchwork Thu Jun 23 23:22:10 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 16359 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 6CFF7BD808 for ; Thu, 23 Jun 2022 23:22:51 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id D29A66565C; Fri, 24 Jun 2022 01:22:50 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org; s=mail; t=1656026571; bh=NPCkwNXRJdjGrG38Wc76cbEo79KkSu6n0yqhqAccaj0=; h=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=yCJlKeJU2LBlc5t/red5m940H+Yq8iDEYRZXWxP+Qz8vp6LUEi7+b57VaJ5AZSMI0 CbDLu8tr0Qjcy5CcRS40SsDmRLaZ6Nm5vlzWHZSey3vakm5QHWUR1dXtVljuIxIRmf onIG1Rx1jPCrLlGRCcf+9wo3FcfGxk+T+tf2nM36ZV+klu97YwI3nz+QPG0TBQRjj9 I3WeZCyZfRatVS4mnipNoSTmx8zD4Mq8GPwmJoL4XwhlWLigrCi8FUMW2gFkVNhsPz cMGr/cKTaIOGZErOLfhtddB/qi19T9j+p+mJIeq7/TPPJJ/Y4fQ8yNlKoSEXSUoZn6 yzlj76BiqvZ7Q== Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id E372765646 for ; Fri, 24 Jun 2022 01:22:34 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="LiOqdHn5"; dkim-atps=neutral Received: from pendragon.lan (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 725BD6BB; Fri, 24 Jun 2022 01:22:34 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1656026554; bh=NPCkwNXRJdjGrG38Wc76cbEo79KkSu6n0yqhqAccaj0=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=LiOqdHn5Uq3hcWU2dldkYYioiOYyMsEHbLDwmMF6G9CTip8Zv38I9XhxXbq0/3RWK OFZZkpnynUIDikyWTskk6s73Fwh6b/P0H4ASlKb2CFuNiy1WNhRdRT7ia2KGqDuzqz xhIP877V5FPeD4NkWZMrsWoIfpAhO+zvs7AndNuk= To: libcamera-devel@lists.libcamera.org Date: Fri, 24 Jun 2022 02:22:10 +0300 Message-Id: <20220623232210.18742-14-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20220623232210.18742-1-laurent.pinchart@ideasonboard.com> References: <20220623232210.18742-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 13/13] gstreamer: Fix race conditions in task pause/resume 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: , X-Patchwork-Original-From: Laurent Pinchart via libcamera-devel From: Laurent Pinchart Reply-To: Laurent Pinchart Cc: Nicolas Dufresne , Vedant Paranjape Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" The task run function races with two other threads that want to resume the task: the requestCompleted() handler and the buffer-notify signal handler. If the former queues completed requests or the latter queues back buffers to the pool, and then resume the task, after the task run handler checks the queues but before it attemps to pause the task, then the task may be paused without noticing that more work is available. The most immediate way to fix this is to take the stream_lock in the requestCompleted() and buffer-notify signal handlers, or cover the whole task run handler with the GstLibcameraSrcState lock. This could cause long delays in the requestCompleted() handler, so that's not a good option. Instead, add a wakeup flag, preotected by the GstLibcameraSrcState lock, that allows detection of a lost race, and retry the task run. Signed-off-by: Laurent Pinchart --- src/gstreamer/gstlibcamerasrc.cpp | 82 +++++++++++++++++++++++-------- 1 file changed, 62 insertions(+), 20 deletions(-) diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 3feb87254916..59400f17ae85 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -120,6 +120,7 @@ struct GstLibcameraSrcState { LIBCAMERA_TSA_GUARDED_BY(lock_); std::queue> completedRequests_ LIBCAMERA_TSA_GUARDED_BY(lock_); + bool wakeup_ LIBCAMERA_TSA_GUARDED_BY(lock_); guint group_id_; @@ -237,14 +238,16 @@ GstLibcameraSrcState::requestCompleted(Request *request) { MutexLocker locker(lock_); completedRequests_.push(std::move(wrap)); - } + wakeup_ = true; - gst_task_resume(src_->task); + gst_task_resume(src_->task); + } } int GstLibcameraSrcState::processRequest() { std::unique_ptr wrap; + int err = 0; { MutexLocker locker(lock_); @@ -253,10 +256,13 @@ int GstLibcameraSrcState::processRequest() wrap = std::move(completedRequests_.front()); completedRequests_.pop(); } + + if (completedRequests_.empty()) + err = -ENOBUFS; } if (!wrap) - return -ENODATA; + return -ENOBUFS; GstFlowReturn ret = GST_FLOW_OK; gst_flow_combiner_reset(src_->flow_combiner); @@ -296,7 +302,7 @@ int GstLibcameraSrcState::processRequest() return -EPIPE; } - return 0; + return err; } static bool @@ -360,53 +366,88 @@ gst_libcamera_src_open(GstLibcameraSrc *self) return true; } +static void +gst_libcamera_src_task_resume(gpointer user_data) +{ + GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); + GstLibcameraSrcState *state = self->state; + + MutexLocker locker(state->lock_); + state->wakeup_ = true; + gst_task_resume(self->task); +} + static void gst_libcamera_src_task_run(gpointer user_data) { GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); GstLibcameraSrcState *state = self->state; + { + MutexLocker locker(state->lock_); + state->wakeup_ = true; + } + + bool doPause = true; + /* * Create and queue one request. If no buffers are available the * function returns -ENOBUFS, which we ignore here as that's not a * fatal error. */ int ret = state->queueRequest(); - if (ret == -ENOMEM) { + switch (ret) { + case 0: + /* + * The request was successfully queued, there may be enough + * buffers to create a new one. Don't pause the task to give it + * another try. + */ + doPause = false; + break; + + case -ENOMEM: GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, ("Failed to allocate request for camera '%s'.", state->cam_->id().c_str()), ("libcamera::Camera::createRequest() failed")); gst_task_stop(self->task); return; + + case -ENOBUFS: + default: + break; } - /* Process one completed request, if available. */ + /* + * Process one completed request, if available, and record if further + * requests are ready for processing. + */ ret = state->processRequest(); switch (ret) { + case -ENOBUFS: + doPause = false; + break; + case -EPIPE: gst_task_stop(self->task); return; - case -ENODATA: - gst_task_pause(self->task); - return; + case 0: + default: + break; } /* - * 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. + * Here we need to decide if we want to pause. This needs to happen in + * lock step with the requestCompleted callback and the buffer-notify + * signal handler that resume the task. */ - bool do_pause; - - { + if (doPause) { MutexLocker locker(state->lock_); - do_pause = state->completedRequests_.empty(); + if (!state->wakeup_) + gst_task_pause(self->task); } - - if (do_pause) - gst_task_pause(self->task); } static void @@ -517,7 +558,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator, stream_cfg.stream()); g_signal_connect_swapped(pool, "buffer-notify", - G_CALLBACK(gst_task_resume), task); + G_CALLBACK(gst_libcamera_src_task_resume), + self); gst_libcamera_pad_set_pool(srcpad, pool); gst_flow_combiner_add_pad(self->flow_combiner, srcpad);