From patchwork Thu Jun 27 14:51:40 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Milan Zamazal X-Patchwork-Id: 20439 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 A9352BD87C for ; Thu, 27 Jun 2024 14:52:43 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 5A1C162CA0; Thu, 27 Jun 2024 16:52:43 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.b="X7YoRciN"; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 2B4F562C99 for ; Thu, 27 Jun 2024 16:52:40 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1719499959; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=DDm2Qq3P9BjbCkivRku1/Kyuq1Xfr7KsFtzI6ZJsgyY=; b=X7YoRciNc8Vqq1hHyLxZsN6/FQXNpyzpXvVMzhUqfP80qGfQDXdqGmavP+8LVRe4ky3vLO PD9vm5tyrXQ/zExYa5g1wcqyelk4RqKtCt5gty3hTcu6PujtEcOrPPAejhCr8Nl5QmTWer /NUBuff1Kpos768wk4Vv/5dfLgc6DMU= Received: from mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-517-vjVH3vN7M4G4wGO2jKT4TQ-1; Thu, 27 Jun 2024 10:52:36 -0400 X-MC-Unique: vjVH3vN7M4G4wGO2jKT4TQ-1 Received: from mx-prod-int-02.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-02.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.15]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 30D5C1944CC9; Thu, 27 Jun 2024 14:52:35 +0000 (UTC) Received: from nuthatch.redhat.com (unknown [10.45.225.47]) by mx-prod-int-02.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id 7DFA51955BD4; Thu, 27 Jun 2024 14:52:33 +0000 (UTC) From: Milan Zamazal To: libcamera-devel@lists.libcamera.org Cc: Milan Zamazal , Kieran Bingham Subject: [PATCH v3 03/14] config: Look up logging levels in the configuration file Date: Thu, 27 Jun 2024 16:51:40 +0200 Message-ID: <20240627145156.1094127-4-mzamazal@redhat.com> In-Reply-To: <20240627145156.1094127-1-mzamazal@redhat.com> References: <20240627145156.1094127-1-mzamazal@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.0 on 10.30.177.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com 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: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" In addition to LIBCAMERA_LOG_LEVELS environment variable, expose the same configuration in the configuration file. Because logging asks for configuration and configuration loading uses logging, extra care is needed. We must prevent calling any log function before configuration is loaded. Otherwise the log function would ask for configuration, it would load the configuration files and would try to log the results, which would cause mutex deadlock due to calling logging inside logging. Not logging in configuration is not an option, at least without introducing complicated mechanism such as delayed logging, because we need to log at least errors when reading the configuration. We must also prevent logging in configuration loading before the configuration is loaded and available (or there is an error in loading the configuration). Otherwise logging would see an empty configuration and wouldn't be configured as asked. Preventing logging before configuration is loaded is ensured by asking for configuration in CameraManager constructor and in IPAManager constructor (which is mysteriously loaded from CameraManager constructor before its body is executed). This seems to be sufficient at least for software ISP. The configuration snippet for logging looks as follows: configuration: log: levels: LIBCAMERA_LOG_LEVELS Signed-off-by: Milan Zamazal --- src/libcamera/base/global_configuration.cpp | 18 +++++++++++++++++- src/libcamera/base/log.cpp | 12 ++++++++++-- src/libcamera/camera_manager.cpp | 9 +++++++++ src/libcamera/ipa_manager.cpp | 12 ++++++++++++ 4 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/libcamera/base/global_configuration.cpp b/src/libcamera/base/global_configuration.cpp index 96088b36..b45fd341 100644 --- a/src/libcamera/base/global_configuration.cpp +++ b/src/libcamera/base/global_configuration.cpp @@ -29,6 +29,13 @@ LOG_DEFINE_CATEGORY(Configuration) * The configuration file is a YAML file and the configuration itself is stored * under `configuration' top-level item. * + * Example configuration file content: + * \code{.yaml} + * configuration: + * log: + * levels: 'IPAManager:DEBUG' + * \endcode + * * The configuration file is looked up in user's home directory first and if it * is not found then in system-wide configuration directories. If multiple * configuration files exist then only the first one found is used and no @@ -57,6 +64,11 @@ const std::vector std::filesystem::path("/etc/libcamera/configuration.yaml"), }; +/* + * Care is needed here to not log anything before the configuration is + * loaded otherwise the logger would be initialized with empty configuration. + */ + void GlobalConfiguration::load() { std::filesystem::path userConfigurationDirectory; @@ -87,10 +99,15 @@ void GlobalConfiguration::load() bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName) { + /* + * initialized_ must be set to true before any logging is called! + */ + File file(fileName); if (!file.exists()) { return false; } + initialized_ = true; if (!file.open(File::OpenModeFlag::ReadOnly)) { LOG(Configuration, Warning) @@ -105,7 +122,6 @@ bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName) return true; } configuration_ = std::move(root); - initialized_ = true; LOG(Configuration, Info) << "Configuration file " << fileName << "loaded"; return true; diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp index 3a656b8f..0f676710 100644 --- a/src/libcamera/base/log.cpp +++ b/src/libcamera/base/log.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -25,6 +26,8 @@ #include #include +#include "libcamera/internal/global_configuration.h" + /** * \file base/log.h * \brief Logging infrastructure @@ -626,8 +629,13 @@ void Logger::parseLogFile() void Logger::parseLogLevels() { const char *debug = utils::secure_getenv("LIBCAMERA_LOG_LEVELS"); - if (!debug) - return; + if (!debug) { + const std::optional confDebug = + GlobalConfiguration::configuration()["log"]["levels"].get(); + if (!confDebug.has_value()) + return; + debug = confDebug.value().c_str(); + } for (const char *pair = debug; *debug != '\0'; pair = debug) { const char *comma = strchrnul(debug, ','); diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index 95a9e326..7c5e5606 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -15,6 +15,7 @@ #include "libcamera/internal/camera.h" #include "libcamera/internal/device_enumerator.h" +#include "libcamera/internal/global_configuration.h" #include "libcamera/internal/pipeline_handler.h" /** @@ -37,6 +38,14 @@ LOG_DEFINE_CATEGORY(Camera) CameraManager::Private::Private() : initialized_(false) { + /* + * This is needed to initialize global configuration + * before any logging is called. Actually, IPAManager + * constructor is called before this, so the configuration + * call is superfluous here, but the initialization belongs + * here. + */ + GlobalConfiguration::configuration(); } int CameraManager::Private::start() diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp index f4e0b633..a678d0c5 100644 --- a/src/libcamera/ipa_manager.cpp +++ b/src/libcamera/ipa_manager.cpp @@ -16,6 +16,7 @@ #include #include +#include "libcamera/internal/global_configuration.h" #include "libcamera/internal/ipa_module.h" #include "libcamera/internal/ipa_proxy.h" #include "libcamera/internal/pipeline_handler.h" @@ -105,6 +106,17 @@ IPAManager *IPAManager::self_ = nullptr; */ IPAManager::IPAManager() { + /* + * Make sure configuration is loaded early. + * Otherwise there would be problems with mutual + * configuration-logging dependencies. + * + * This is needed to be here in addition to CameraManager constructor + * because logging is called before CameraManager constructor body is + * executed. + */ + GlobalConfiguration::configuration(); + if (self_) LOG(IPAManager, Fatal) << "Multiple IPAManager objects are not allowed";