From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5D5F4C00140 for ; Tue, 2 Aug 2022 13:27:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236237AbiHBN1J (ORCPT ); Tue, 2 Aug 2022 09:27:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60378 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236225AbiHBN1J (ORCPT ); Tue, 2 Aug 2022 09:27:09 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id B8C4216599 for ; Tue, 2 Aug 2022 06:27:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1659446826; 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=x2oMPhZkETmScl9rWSPc47NQggucbQ2fxbTG8GNsj0I=; b=XnTT6YaUXHKEasZEM8+xGxMdFiU1Zdtx15x4FNUoK1XM4nOjHLgO/DZiMBa4fqZHnyeZTo Q7kU4E6eFHXl4jWPo/PlJFewa0C4xOTh9ePDWN2AN9kj81HbBrAZpJQzFWZct474g9K+qD rhX9fl6owu18+xrwyS2HrVdUwW/YkMQ= Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-385-NwpbDLCQOk24dtyw0_Ga2Q-1; Tue, 02 Aug 2022 09:27:05 -0400 X-MC-Unique: NwpbDLCQOk24dtyw0_Ga2Q-1 Received: by mail-ed1-f71.google.com with SMTP id f13-20020a0564021e8d00b00437a2acb543so9046131edf.7 for ; Tue, 02 Aug 2022 06:27:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:from:to:cc:references:in-reply-to :content-transfer-encoding; bh=x2oMPhZkETmScl9rWSPc47NQggucbQ2fxbTG8GNsj0I=; b=KiXfE9JdgDKW3vXRxjclJLXaUsDSiDJc/WDgzNQ3QjRqocF6ayHb38pyA7hnTpxpLq b2L/zsUSAHtS9uRTySZDCahwWx9YVbJV/bQvFInNVNiWY2k2BSP3/AVC7VEwaFYnBshb zOKVyDDzV9eRwYpI5Ocpi0bJGVw63ttUX+Jd/ybf+fLQ/NR4YW66PDFStS5eEA74VvFe 95e1d+2mXrRWKEnkWvgGh/qMUDn3/A/keZYnwLeUGLxfkbKPkuzpqnuRYS+G9tJLiMcZ 6JdmMqIW25DddS6luGht2L9LsoL7oHNeCiHSvZ+dxU6f8iJFvqdNxuTSo+KRAbcB5Srf TJng== X-Gm-Message-State: AJIora9IA8m+2wEZlhe6RFQ/2465g/AL3HQI2k3jnKJGsJqm20pSZli9 MBKRtE8+RG+9x6p/AT9JDbf8fTiiDOqw4iquyfpVZdNwv1UvJ7wc94lJ0EFHNcqxV1hmXP53KXF tGwAexZ/wS3F/Aa97LRpH96e0t+EDhLaVYw== X-Received: by 2002:a17:907:7ea7:b0:72b:6e6b:4895 with SMTP id qb39-20020a1709077ea700b0072b6e6b4895mr16302052ejc.338.1659446822736; Tue, 02 Aug 2022 06:27:02 -0700 (PDT) X-Google-Smtp-Source: AGRyM1uGH/gqLCPmIBelzZwUbt92GZSXE7ahaOtV5De9iBMeFT7yuqPTBGs8ceMhI74DQlFeOcJoMQ== X-Received: by 2002:a17:907:7ea7:b0:72b:6e6b:4895 with SMTP id qb39-20020a1709077ea700b0072b6e6b4895mr16302032ejc.338.1659446822434; Tue, 02 Aug 2022 06:27:02 -0700 (PDT) Received: from ?IPV6:2001:1c00:c1e:bf00:d69d:5353:dba5:ee81? (2001-1c00-0c1e-bf00-d69d-5353-dba5-ee81.cable.dynamic.v6.ziggo.nl. [2001:1c00:c1e:bf00:d69d:5353:dba5:ee81]) by smtp.gmail.com with ESMTPSA id f27-20020a17090631db00b0072b342ad997sm6264634ejf.199.2022.08.02.06.27.01 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 02 Aug 2022 06:27:01 -0700 (PDT) Message-ID: Date: Tue, 2 Aug 2022 15:27:01 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v3 09/11] platform/x86/amd/pmf: Handle AMT and CQL events for Auto mode Content-Language: en-US From: Hans de Goede To: Shyam Sundar S K , markgross@kernel.org Cc: platform-driver-x86@vger.kernel.org, Patil.Reddy@amd.com, Mario Limonciello , Mark Pearson References: <20220802112545.2118632-1-Shyam-sundar.S-k@amd.com> <20220802112545.2118632-10-Shyam-sundar.S-k@amd.com> <0d4f175f-3d5b-dfe5-3ab3-db7777fe875e@redhat.com> In-Reply-To: <0d4f175f-3d5b-dfe5-3ab3-db7777fe875e@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: platform-driver-x86@vger.kernel.org Hi, On 8/2/22 15:24, Hans de Goede wrote: > Hi, > > On 8/2/22 13:25, Shyam Sundar S K wrote: >> The transition to auto-mode happens when the PMF driver receives >> AMT (Auto Mode transition) event. transition logic will reside in the >> PMF driver but the events would come from other supported drivers[1]. >> >> The thermal parameters would vary between when a performance "on-lap" mode >> is detected and versus when not. The CQL event would get triggered from >> other drivers, so that PMF driver would adjust the system thermal config >> based on the ACPI inputs. >> >> OEMs can control whether or not to enable AMT or CQL via other supported >> drivers[1] but the actual transition logic resides in the AMD PMF driver. >> When an AMT event is received the automatic mode transition RAPL algorithm >> will run. When a CQL event is received an performance "on-lap" mode will >> be enabled and thermal parameters will be adjusted accordingly. >> >> [1] >> Link: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=review-hans&id=755b249250df1b612d982f3b702c831b26ecdf73 >> >> Cc: Mario Limonciello >> Cc: Mark Pearson >> Signed-off-by: Shyam Sundar S K >> --- >> drivers/platform/x86/amd/pmf/acpi.c | 64 ++++++++++++++++++++++++ >> drivers/platform/x86/amd/pmf/auto-mode.c | 38 ++++++++++++++ >> drivers/platform/x86/amd/pmf/core.c | 9 ++++ >> drivers/platform/x86/amd/pmf/pmf.h | 22 ++++++++ >> 4 files changed, 133 insertions(+) >> >> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c >> index 200e31033d94..70084807a2e7 100644 >> --- a/drivers/platform/x86/amd/pmf/acpi.c >> +++ b/drivers/platform/x86/amd/pmf/acpi.c >> @@ -11,6 +11,9 @@ >> #include >> #include "pmf.h" >> >> +#define APMF_CQL_NOTIFICATION 2 >> +#define APMF_AMT_NOTIFICATION 3 >> + >> static union acpi_object *apmf_if_call(struct amd_pmf_dev *pdev, int fn, struct acpi_buffer *param) >> { >> struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; >> @@ -154,6 +157,47 @@ int apmf_get_auto_mode_def(struct amd_pmf_dev *pdev, struct apmf_auto_mode *data >> return apmf_if_call_store_buffer(pdev, APMF_FUNC_AUTO_MODE, data, sizeof(*data)); >> } >> >> +int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req) >> +{ >> + return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS, >> + req, sizeof(*req)); >> +} >> + >> +static void apmf_event_handler(acpi_handle handle, u32 event, void *data) >> +{ >> + struct amd_pmf_dev *pmf_dev = data; >> + struct apmf_sbios_req req; >> + int ret; >> + >> + mutex_lock(&pmf_dev->update_mutex); >> + ret = apmf_get_sbios_requests(pmf_dev, &req); >> + if (ret) { >> + dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret); >> + return; >> + } >> + >> + if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) { >> + dev_dbg(pmf_dev->dev, "AMT is supported and notifications %s\n", >> + req.amt_event ? "Enabled" : "Disabled"); >> + pmf_dev->amt_enabled = !!req.amt_event; >> + } >> + >> + if (pmf_dev->amt_enabled) >> + amd_pmf_handle_amt(pmf_dev); >> + else >> + amd_pmf_reset_amt(pmf_dev); > > AFAICT this should be inside the: > > if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) { > > block, pmf_dev->amt_enabled only is updated in that block and > re-doing the amd_pmf_handle_amt() / reset_amt() calls when > pmf_dev->amt_enabled is not changed is not necessary. > >> + >> + if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) { >> + dev_dbg(pmf_dev->dev, "CQL is supported and notifications %s\n", >> + req.cql_event ? "Enabled" : "Disabled"); >> + >> + /* update the target mode information */ >> + if (pmf_dev->amt_enabled) >> + amd_pmf_update_2_cql(pmf_dev, req.cql_event); >> + } >> + mutex_unlock(&pmf_dev->update_mutex); >> +} >> + >> static int apmf_if_verify_interface(struct amd_pmf_dev *pdev) >> { >> struct apmf_verify_interface output; >> @@ -195,12 +239,20 @@ static int apmf_get_system_params(struct amd_pmf_dev *dev) >> >> void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev) >> { >> + acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev); >> + >> if (pmf_dev->hb_interval) >> cancel_delayed_work_sync(&pmf_dev->heart_beat); >> + >> + if (is_apmf_func_supported(pmf_dev, APMF_FUNC_AUTO_MODE) && >> + is_apmf_func_supported(pmf_dev, APMF_FUNC_SBIOS_REQUESTS)) >> + acpi_remove_notify_handler(ahandle, ACPI_ALL_NOTIFY, apmf_event_handler); >> } >> >> int apmf_acpi_init(struct amd_pmf_dev *pmf_dev) >> { >> + acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev); >> + acpi_status status; >> int ret; >> >> ret = apmf_if_verify_interface(pmf_dev); >> @@ -221,6 +273,18 @@ int apmf_acpi_init(struct amd_pmf_dev *pmf_dev) >> schedule_delayed_work(&pmf_dev->heart_beat, 0); >> } >> >> + /* Install the APMF Notify handler */ >> + if (is_apmf_func_supported(pmf_dev, APMF_FUNC_AUTO_MODE) && >> + is_apmf_func_supported(pmf_dev, APMF_FUNC_SBIOS_REQUESTS)) { >> + status = acpi_install_notify_handler(ahandle, >> + ACPI_ALL_NOTIFY, >> + apmf_event_handler, pmf_dev); >> + if (ACPI_FAILURE(status)) { >> + dev_err(pmf_dev->dev, "failed to install notify handler\n"); >> + return -ENODEV; >> + } As mentioned in my latest "Re: [PATCH v2 RESEND 09/11] platform/x86/amd/pmf: Handle AMT and CQL events for Auto mode" email I think you should call apmf_event_handler() manually here once to catch up with possibly missed notifies. >> + } >> + >> out: >> return ret; >> } >> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c >> index 99f5a2396b0b..4e4ec6023525 100644 >> --- a/drivers/platform/x86/amd/pmf/auto-mode.c >> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c >> @@ -108,6 +108,21 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t >> } >> } >> >> +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev, bool is_cql_event) >> +{ >> + int mode = config_store.current_mode; >> + >> + config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode = >> + is_cql_event ? AUTO_PERFORMANCE_ON_LAP : AUTO_PERFORMANCE; >> + >> + if ((mode == AUTO_PERFORMANCE || mode == AUTO_PERFORMANCE_ON_LAP) && >> + mode != config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode) { >> + mode = config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode; >> + amd_pmf_set_automode(dev, mode, NULL); >> + } >> + dev_dbg(dev->dev, "updated CQL thermals\n"); >> +} >> + >> static void amd_pmf_get_power_threshold(void) >> { >> config_store.transition[AUTO_TRANSITION_TO_QUIET].power_threshold = >> @@ -249,6 +264,29 @@ void amd_pmf_load_defaults_auto_mode(struct amd_pmf_dev *dev) >> dev->socket_power_history_idx = -1; >> } >> >> +void amd_pmf_reset_amt(struct amd_pmf_dev *dev) >> +{ >> + /* >> + * OEM BIOS implementation guide says that if the auto mode is enabled >> + * the platform_profile registration shall be done by the OEM driver. >> + * There could be cases where both static slider and auto mode BIOS >> + * functions are enabled, in that case enable static slider updates >> + * only if it advertised as supported. >> + */ >> + >> + if (is_apmf_func_supported(dev, APMF_FUNC_STATIC_SLIDER_GRANULAR)) { >> + u8 mode = amd_pmf_get_pprof_modes(dev); >> + >> + dev_dbg(dev->dev, "resetting AMT thermals\n"); >> + amd_pmf_update_slider(dev, SLIDER_OP_SET, mode, NULL); >> + } >> +} >> + >> +void amd_pmf_handle_amt(struct amd_pmf_dev *dev) >> +{ >> + amd_pmf_set_automode(dev, config_store.current_mode, NULL); >> +} >> + >> void amd_pmf_deinit_auto_mode(struct amd_pmf_dev *dev) >> { >> cancel_delayed_work_sync(&dev->work_buffer); >> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c >> index 42d803b49d97..4467d682cd11 100644 >> --- a/drivers/platform/x86/amd/pmf/core.c >> +++ b/drivers/platform/x86/amd/pmf/core.c >> @@ -99,6 +99,7 @@ static void amd_pmf_get_metrics(struct work_struct *work) >> ktime_t time_elapsed_ms; >> int socket_power; >> >> + mutex_lock(&dev->update_mutex); >> /* Transfer table contents */ >> memset(dev->buf, 0, sizeof(dev->m_table)); >> amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL); >> @@ -108,8 +109,14 @@ static void amd_pmf_get_metrics(struct work_struct *work) >> /* Calculate the avg SoC power consumption */ >> socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power; >> >> + if (dev->amt_enabled) { >> + /* Apply the Auto Mode transition */ >> + amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms); >> + } >> + >> dev->start_time = ktime_to_ms(ktime_get()); >> schedule_delayed_work(&dev->work_buffer, msecs_to_jiffies(metrics_table_loop_ms)); >> + mutex_unlock(&dev->update_mutex); >> } >> >> static inline u32 amd_pmf_reg_read(struct amd_pmf_dev *dev, int reg_offset) >> @@ -329,6 +336,7 @@ static int amd_pmf_probe(struct platform_device *pdev) >> amd_pmf_dbgfs_register(dev); >> >> mutex_init(&dev->lock); >> + mutex_init(&dev->update_mutex); >> dev_info(dev->dev, "registered PMF device successfully\n"); >> >> return 0; >> @@ -339,6 +347,7 @@ static int amd_pmf_remove(struct platform_device *pdev) >> struct amd_pmf_dev *dev = platform_get_drvdata(pdev); >> >> mutex_destroy(&dev->lock); >> + mutex_destroy(&dev->update_mutex); >> amd_pmf_deinit_features(dev); >> apmf_acpi_deinit(dev); >> amd_pmf_dbgfs_unregister(dev); >> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h >> index 8c92cd6871df..cc88a02b488d 100644 >> --- a/drivers/platform/x86/amd/pmf/pmf.h >> +++ b/drivers/platform/x86/amd/pmf/pmf.h >> @@ -17,6 +17,7 @@ >> /* APMF Functions */ >> #define APMF_FUNC_VERIFY_INTERFACE 0 >> #define APMF_FUNC_GET_SYS_PARAMS 1 >> +#define APMF_FUNC_SBIOS_REQUESTS 2 >> #define APMF_FUNC_SBIOS_HEARTBEAT 4 >> #define APMF_FUNC_AUTO_MODE 5 >> #define APMF_FUNC_SET_FAN_IDX 7 >> @@ -63,6 +64,21 @@ struct apmf_system_params { >> u32 heartbeat_int; >> } __packed; >> >> +struct apmf_sbios_req { >> + u16 size; >> + u32 pending_req; >> + u8 rsd; >> + u8 cql_event; >> + u8 amt_event; >> + u32 fppt; >> + u32 sppt; >> + u32 fppt_apu_only; >> + u32 spl; >> + u32 stt_min_limit; >> + u8 skin_temp_apu; >> + u8 skin_temp_hs2; >> +} __packed; >> + >> struct apmf_fan_idx { >> u16 size; >> u8 fan_ctl_mode; >> @@ -147,6 +163,8 @@ struct amd_pmf_dev { >> ktime_t start_time; >> int socket_power_history[AVG_SAMPLE_SIZE]; >> int socket_power_history_idx; >> + bool amt_enabled; >> + struct mutex update_mutex; /* protects race between ACPI handler and metrics thread */ >> }; >> >> struct apmf_sps_prop_granular { >> @@ -307,5 +325,9 @@ int apmf_get_auto_mode_def(struct amd_pmf_dev *pdev, struct apmf_auto_mode *data >> void amd_pmf_init_auto_mode(struct amd_pmf_dev *dev); >> void amd_pmf_deinit_auto_mode(struct amd_pmf_dev *dev); >> void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms); >> +int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req); >> >> +void amd_pmf_update_2_cql(struct amd_pmf_dev *dev, bool is_cql_event); >> +void amd_pmf_reset_amt(struct amd_pmf_dev *dev); >> +void amd_pmf_handle_amt(struct amd_pmf_dev *dev); >> #endif /* PMF_H */ > > Except for the 1 remark this looks good to me. Make that 2 remarks... Regards, Hans