<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=Windows-1252"> </head> <body> [AMD Official Use Only - General] <div> <div dir="auto">> 'j' was initially set as 'num_of_wbrf_ranges - 1'. So, I suppose 'num_of_wbrf_ranges' should be set as 'j' instead of 'j - 1'. Right?</div> <div dir="auto"> </div> <div dir="auto">Yes.</div> <div id="ms-outlook-mobile-signature" dir="auto"> <div> </div> Thanks, Lijo</div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Quan, Evan <Evan.Quan@amd.com> Sent: Monday, August 28, 2023 7:23:55 AM To: Lazar, Lijo <Lijo.Lazar@amd.com>; lenb@kernel.org <lenb@kernel.org>; johannes@sipsolutions.net <johannes@sipsolutions.net>; davem@davemloft.net <davem@davemloft.net>; edumazet@google.com <edumazet@google.com>; kuba@kernel.org <kuba@kernel.org>; pabeni@redhat.com <pabeni@redhat.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; rafael@kernel.org <rafael@kernel.org>; Limonciello, Mario <Mario.Limonciello@amd.com> Cc: linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-acpi@vger.kernel.org <linux-acpi@vger.kernel.org>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org>; linux-wireless@vger.kernel.org <linux-wireless@vger.kernel.org>; netdev@vger.kernel.org <netdev@vger.kernel.org> Subject: RE: [V10 5/8] drm/amd/pm: setup the framework to support Wifi RFI mitigation feature <div> </div> </div> <div class="BodyFragment"><a name="BM_BEGIN"></a> <div>[AMD Official Use Only - General] > -----Original Message----- > From: Lazar, Lijo <Lijo.Lazar@amd.com> > Sent: Friday, August 25, 2023 10:09 PM > To: Quan, Evan <Evan.Quan@amd.com>; lenb@kernel.org; > johannes@sipsolutions.net; davem@davemloft.net; edumazet@google.com; > kuba@kernel.org; pabeni@redhat.com; Deucher, Alexander > <Alexander.Deucher@amd.com>; rafael@kernel.org; Limonciello, Mario > <Mario.Limonciello@amd.com> > Cc: linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org; amd- > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux- > wireless@vger.kernel.org; netdev@vger.kernel.org > Subject: Re: [V10 5/8] drm/amd/pm: setup the framework to support Wifi > RFI mitigation feature > > > > On 8/25/2023 2:08 PM, Evan Quan wrote: > > With WBRF feature supported, as a driver responding to the > > frequencies, amdgpu driver is able to do shadow pstate switching to > > mitigate possible interference(between its (G-)DDR memory clocks and > > local radio module frequency bands used by Wifi 6/6e/7). > > > > Signed-off-by: Evan Quan <evan.quan@amd.com> > > Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> > > -- > > v1->v2: > > - update the prompt for feature support(Lijo) > > v8->v9: > > - update parameter document for smu_wbrf_event_handler(Simon) > > v9->v10: > > - correct the logics for wbrf range sorting(Lijo) > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 + > > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 17 ++ > > drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 195 > ++++++++++++++++++ > > drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h | 23 +++ > > drivers/gpu/drm/amd/pm/swsmu/smu_internal.h | 3 + > > 5 files changed, 240 insertions(+) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > index a3b86b86dc47..2bfc9111ab00 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > @@ -247,6 +247,8 @@ extern int amdgpu_sg_display; > > > > extern int amdgpu_user_partt_mode; > > > > +extern int amdgpu_wbrf; > > + > > #define AMDGPU_VM_MAX_NUM_CTX 4096 > > #define AMDGPU_SG_THRESHOLD (256*1024*1024) > > #define AMDGPU_WAIT_IDLE_TIMEOUT_IN_MS 3000 > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > > index 0593ef8fe0a6..1c574bd3b60d 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > > @@ -195,6 +195,7 @@ int amdgpu_use_xgmi_p2p = 1; > > int amdgpu_vcnfw_log; > > int amdgpu_sg_display = -1; /* auto */ > > int amdgpu_user_partt_mode = > AMDGPU_AUTO_COMPUTE_PARTITION_MODE; > > +int amdgpu_wbrf = -1; > > > > static void amdgpu_drv_delayed_reset_work_handler(struct work_struct > > *work); > > > > @@ -981,6 +982,22 @@ module_param_named(user_partt_mode, > amdgpu_user_partt_mode, uint, 0444); > > module_param(enforce_isolation, bool, 0444); > > MODULE_PARM_DESC(enforce_isolation, "enforce process isolation > > between graphics and compute . enforce_isolation = on"); > > > > +/** > > + * DOC: wbrf (int) > > + * Enable Wifi RFI interference mitigation feature. > > + * Due to electrical and mechanical constraints there may be likely > > +interference of > > + * relatively high-powered harmonics of the (G-)DDR memory clocks > > +with local radio > > + * module frequency bands used by Wifi 6/6e/7. To mitigate the > > +possible RFI interference, > > + * with this feature enabled, PMFW will use either “shadowed P-State” > > +or “P-State” based > > + * on active list of frequencies in-use (to be avoided) as part of > > +initial setting or > > + * P-state transition. However, there may be potential performance > > +impact with this > > + * feature enabled. > > + * (0 = disabled, 1 = enabled, -1 = auto (default setting, will be > > +enabled if supported)) */ MODULE_PARM_DESC(wbrf, > > + "Enable Wifi RFI interference mitigation (0 = disabled, 1 = enabled, > > +-1 = auto(default)"); module_param_named(wbrf, amdgpu_wbrf, int, > > +0444); > > + > > /* These devices are not supported by amdgpu. > > * They are supported by the mach64, r128, radeon drivers > > */ > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c > > b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c > > index ce41a8309582..bdfd234d1558 100644 > > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c > > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c > > @@ -1228,6 +1228,174 @@ static int > smu_get_thermal_temperature_range(struct smu_context *smu) > > return ret; > > } > > > > +/** > > + * smu_wbrf_handle_exclusion_ranges - consume the wbrf exclusion > > +ranges > > + * > > + * @smu: smu_context pointer > > + * > > + * Retrieve the wbrf exclusion ranges and send them to PMFW for proper > handling. > > + * Returns 0 on success, error on failure. > > + */ > > +static int smu_wbrf_handle_exclusion_ranges(struct smu_context *smu) > > +{ > > + struct wbrf_ranges_in_out wbrf_exclusion = {0}; > > + struct exclusion_range *wifi_bands = wbrf_exclusion.band_list; > > + struct amdgpu_device *adev = smu->adev; > > + uint32_t num_of_wbrf_ranges = MAX_NUM_OF_WBRF_RANGES; > > + uint64_t start, end; > > + int ret, i, j; > > + > > + ret = acpi_amd_wbrf_retrieve_exclusions(adev->dev, > &wbrf_exclusion); > > + if (ret) { > > + dev_err(adev->dev, "Failed to retrieve exclusion ranges!\n"); > > + return ret; > > + } > > + > > + /* > > + * The exclusion ranges array we got might be filled with holes and > duplicate > > + * entries. For example: > > + * {(2400, 2500), (0, 0), (6882, 6962), (2400, 2500), (0, 0), (6117, > 6189), (0, 0)...} > > + * We need to do some sortups to eliminate those holes and duplicate > entries. > > + * Expected output: {(2400, 2500), (6117, 6189), (6882, 6962), (0, > 0)...} > > + */ > > + for (i = 0; i < num_of_wbrf_ranges; i++) { > > + start = wifi_bands[i].start; > > + end = wifi_bands[i].end; > > + > > + /* get the last valid entry to fill the intermediate hole */ > > + if (!start && !end) { > > + for (j = num_of_wbrf_ranges - 1; j > i; j--) > > + if (wifi_bands[j].start && > > + wifi_bands[j].end) > > + break; > > + > > + /* no valid entry left */ > > + if (j <= i) > > + break; > > + > > + wifi_bands[i].start = wifi_bands[j].start; > > + wifi_bands[i].end = wifi_bands[j].end; > > start/end variables remain 0. Thus it won't have any effect on the loop below > which looks for duplicates. start/end need to be reassigned here. Ah, right. Thanks, I can fix that. > > > + wifi_bands[j].start = 0; > > + wifi_bands[j].end = 0; > > + num_of_wbrf_ranges--; > > Instead of decrementing by 1, this can be kept equal to j - 1 as jth entry is 0 > now. 'j' was initially set as 'num_of_wbrf_ranges - 1'. So, I suppose 'num_of_wbrf_ranges' should be set as 'j' instead of 'j - 1'. Right? Evan > > Thanks, > Lijo > > > + } > > + > > + /* eliminate duplicate entries */ > > + for (j = i + 1; j < num_of_wbrf_ranges; j++) { > > + if ((wifi_bands[j].start == start) && > > + (wifi_bands[j].end == end)) { > > + wifi_bands[j].start = 0; > > + wifi_bands[j].end = 0; > > + } > > + } > > + } > > + > > + /* Send the sorted wifi_bands to PMFW */ > > + ret = smu_set_wbrf_exclusion_ranges(smu, wifi_bands); > > + /* Give it another chance */ > > + if (unlikely(ret == -EBUSY)) { > > + mdelay(5); > > + ret = smu_set_wbrf_exclusion_ranges(smu, wifi_bands); > > + } > > + > > + return ret; > > +} > > + > > +/** > > + * smu_wbrf_event_handler - handle notify events > > + * > > + * @nb: notifier block > > + * @action: event type > > + * @_arg: event data > > + * > > + * Calls relevant amdgpu function in response to wbrf event > > + * notification from kernel. > > + */ > > +static int smu_wbrf_event_handler(struct notifier_block *nb, > > + unsigned long action, void *_arg) { > > + struct smu_context *smu = container_of(nb, struct smu_context, > > + wbrf_notifier); > > + > > + switch (action) { > > + case WBRF_CHANGED: > > + smu_wbrf_handle_exclusion_ranges(smu); > > + break; > > + default: > > + return NOTIFY_DONE; > > + }; > > + > > + return NOTIFY_OK; > > +} > > + > > +/** > > + * smu_wbrf_support_check - check wbrf support > > + * > > + * @smu: smu_context pointer > > + * > > + * Verifies the ACPI interface whether wbrf is supported. > > + */ > > +static void smu_wbrf_support_check(struct smu_context *smu) { > > + struct amdgpu_device *adev = smu->adev; > > + > > + smu->wbrf_supported = smu_is_asic_wbrf_supported(smu) && > > + !!amdgpu_wbrf && > > + acpi_amd_wbrf_supported_consumer(adev->dev); > > + > > + if (smu->wbrf_supported) > > + dev_info(adev->dev, "RF interference mitigation is > supported\n"); } > > + > > +/** > > + * smu_wbrf_init - init driver wbrf support > > + * > > + * @smu: smu_context pointer > > + * > > + * Verifies the AMD ACPI interfaces and registers with the wbrf > > + * notifier chain if wbrf feature is supported. > > + * Returns 0 on success, error on failure. > > + */ > > +static int smu_wbrf_init(struct smu_context *smu) { > > + struct amdgpu_device *adev = smu->adev; > > + int ret; > > + > > + if (!smu->wbrf_supported) > > + return 0; > > + > > + smu->wbrf_notifier.notifier_call = smu_wbrf_event_handler; > > + ret = acpi_amd_wbrf_register_notifier(&smu->wbrf_notifier); > > + if (ret) > > + return ret; > > + > > + /* > > + * Some wifiband exclusion ranges may be already there > > + * before our driver loaded. To make sure our driver > > + * is awared of those exclusion ranges. > > + */ > > + ret = smu_wbrf_handle_exclusion_ranges(smu); > > + if (ret) > > + dev_err(adev->dev, "Failed to handle wbrf exclusion > ranges\n"); > > + > > + return ret; > > +} > > + > > +/** > > + * smu_wbrf_fini - tear down driver wbrf support > > + * > > + * @smu: smu_context pointer > > + * > > + * Unregisters with the wbrf notifier chain. > > + */ > > +static void smu_wbrf_fini(struct smu_context *smu) { > > + if (!smu->wbrf_supported) > > + return; > > + > > + acpi_amd_wbrf_unregister_notifier(&smu->wbrf_notifier); > > +} > > + > > static int smu_smc_hw_setup(struct smu_context *smu) > > { > > struct smu_feature *feature = &smu->smu_feature; @@ -1320,6 > > +1488,15 @@ static int smu_smc_hw_setup(struct smu_context *smu) > > if (ret) > > return ret; > > > > + /* Enable UclkShadow on wbrf supported */ > > + if (smu->wbrf_supported) { > > + ret = smu_enable_uclk_shadow(smu, true); > > + if (ret) { > > + dev_err(adev->dev, "Failed to enable UclkShadow > feature to support wbrf!\n"); > > + return ret; > > + } > > + } > > + > > /* > > * With SCPM enabled, these actions(and relevant messages) are > > * not needed and permitted. > > @@ -1416,6 +1593,15 @@ static int smu_smc_hw_setup(struct > smu_context *smu) > > */ > > ret = smu_set_min_dcef_deep_sleep(smu, > > smu->smu_table.boot_values.dcefclk > / 100); > > + if (ret) { > > + dev_err(adev->dev, "Error setting min deepsleep dcefclk\n"); > > + return ret; > > + } > > + > > + /* Init wbrf support. Properly setup the notifier */ > > + ret = smu_wbrf_init(smu); > > + if (ret) > > + dev_err(adev->dev, "Error during wbrf init call\n"); > > > > return ret; > > } > > @@ -1471,6 +1657,13 @@ static int smu_hw_init(void *handle) > > return ret; > > } > > > > + /* > > + * Check whether wbrf is supported. This needs to be done > > + * before SMU setup starts since part of SMU configuration > > + * relies on this. > > + */ > > + smu_wbrf_support_check(smu); > > + > > if (smu->is_apu) { > > ret = smu_set_gfx_imu_enable(smu); > > if (ret) > > @@ -1623,6 +1816,8 @@ static int smu_smc_hw_cleanup(struct > smu_context *smu) > > struct amdgpu_device *adev = smu->adev; > > int ret = 0; > > > > + smu_wbrf_fini(smu); > > + > > cancel_work_sync(&smu->throttling_logging_work); > > cancel_work_sync(&smu->interrupt_work); > > > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h > > b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h > > index 6e2069dcb6b9..3eb1c72a76f1 100644 > > --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h > > +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h > > @@ -22,6 +22,8 @@ > > #ifndef __AMDGPU_SMU_H__ > > #define __AMDGPU_SMU_H__ > > > > +#include <linux/acpi_amd_wbrf.h> > > + > > #include "amdgpu.h" > > #include "kgd_pp_interface.h" > > #include "dm_pp_interface.h" > > @@ -575,6 +577,10 @@ struct smu_context > > u32 debug_resp_reg; > > > > struct delayed_work swctf_delayed_work; > > + > > + /* data structures for wbrf feature support */ > > + bool wbrf_supported; > > + struct notifier_block wbrf_notifier; > > }; > > > > struct i2c_adapter; > > @@ -1356,6 +1362,23 @@ struct pptable_funcs { > > * @init_pptable_microcode: Prepare the pptable microcode to upload > via PSP > > */ > > int (*init_pptable_microcode)(struct smu_context *smu); > > + > > + /** > > + * @is_asic_wbrf_supported: check whether PMFW supports the wbrf > feature > > + */ > > + bool (*is_asic_wbrf_supported)(struct smu_context *smu); > > + > > + /** > > + * @enable_uclk_shadow: Enable the uclk shadow feature on wbrf > supported > > + */ > > + int (*enable_uclk_shadow)(struct smu_context *smu, > > + bool enablement); > > + > > + /** > > + * @set_wbrf_exclusion_ranges: notify SMU the wifi bands occupied > > + */ > > + int (*set_wbrf_exclusion_ranges)(struct smu_context *smu, > > + struct exclusion_range > *exclusion_ranges); > > }; > > > > typedef enum { > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_internal.h > > b/drivers/gpu/drm/amd/pm/swsmu/smu_internal.h > > index ceb13c838067..67d7495ab49e 100644 > > --- a/drivers/gpu/drm/amd/pm/swsmu/smu_internal.h > > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_internal.h > > @@ -97,6 +97,9 @@ > > #define smu_get_default_config_table_settings(smu, config_table) > smu_ppt_funcs(get_default_config_table_settings, -EOPNOTSUPP, > smu, config_table) > > #define smu_set_config_table(smu, config_table) > smu_ppt_funcs(set_config_table, -EOPNOTSUPP, smu, config_table) > > #define smu_init_pptable_microcode(smu) > smu_ppt_funcs(init_pptable_microcode, 0, smu) > > +#define smu_is_asic_wbrf_supported(smu) > smu_ppt_funcs(is_asic_wbrf_supported, false, smu) > > +#define smu_enable_uclk_shadow(smu, enablement) > smu_ppt_funcs(enable_uclk_shadow, 0, smu, enablement) > > +#define smu_set_wbrf_exclusion_ranges(smu, exclusion_ranges) > smu_ppt_funcs(set_wbrf_exclusion_ranges, -EOPNOTSUPP, smu, > exclusion_ranges) > > > > #endif > > #endif </div> </div> </div> </body> </html>