[PATCH] drm/amd/pm: validate SMU feature enable message for getting feature enabled mask
Lazar, Lijo
lijo.lazar at amd.com
Fri Feb 18 05:29:20 UTC 2022
On 2/18/2022 9:57 AM, Liang, Prike wrote:
> [Public]
>
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>> Sent: Friday, February 18, 2022 12:05 PM
>> To: Liang, Prike <Prike.Liang at amd.com>; amd-gfx at lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Quan, Evan
>> <Evan.Quan at amd.com>; Huang, Ray <Ray.Huang at amd.com>
>> Subject: Re: [PATCH] drm/amd/pm: validate SMU feature enable message for
>> getting feature enabled mask
>>
>>
>>
>> On 2/18/2022 9:25 AM, Prike Liang wrote:
>>> There's always miss the SMU feature enabled checked in the NPI phase,
>>> so let validate the SMU feature enable message directly rather than
>>> add more and more MP1 version check.
>>>
>>> Signed-off-by: Prike Liang <Prike.Liang at amd.com>
>>> ---
>>> drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c | 28 ++++++-------------------
>> -
>>> 1 file changed, 6 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>> index f24111d28290..da1ac70ed455 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>> @@ -552,10 +552,9 @@ bool smu_cmn_clk_dpm_is_enabled(struct
>> smu_context *smu,
>>> int smu_cmn_get_enabled_mask(struct smu_context *smu,
>>> uint64_t *feature_mask)
>>> {
>>> - struct amdgpu_device *adev = smu->adev;
>>> uint32_t *feature_mask_high;
>>> uint32_t *feature_mask_low;
>>> - int ret = 0;
>>> + int ret = 0, index = 0;
>>>
>>> if (!feature_mask)
>>> return -EINVAL;
>>> @@ -563,12 +562,10 @@ int smu_cmn_get_enabled_mask(struct
>> smu_context *smu,
>>> feature_mask_low = &((uint32_t *)feature_mask)[0];
>>> feature_mask_high = &((uint32_t *)feature_mask)[1];
>>>
>>> - switch (adev->ip_versions[MP1_HWIP][0]) {
>>> - /* For Vangogh and Yellow Carp */
>>> - case IP_VERSION(11, 5, 0):
>>> - case IP_VERSION(13, 0, 1):
>>> - case IP_VERSION(13, 0, 3):
>>> - case IP_VERSION(13, 0, 8):
>>> + index = smu_cmn_to_asic_specific_index(smu,
>>> + CMN2ASIC_MAPPING_MSG,
>>> +
>> SMU_MSG_GetEnabledSmuFeatures);
>>> + if (index > 0) {
>>> ret = smu_cmn_send_smc_msg_with_param(smu,
>>>
>> SMU_MSG_GetEnabledSmuFeatures,
>>> 0,
>>> @@ -580,19 +577,7 @@ int smu_cmn_get_enabled_mask(struct
>> smu_context *smu,
>>>
>> SMU_MSG_GetEnabledSmuFeatures,
>>> 1,
>>> feature_mask_high);
>>> - break;
>>> - /*
>>> - * For Cyan Skillfish and Renoir, there is no interface provided by
>> PMFW
>>> - * to retrieve the enabled features. So, we assume all features are
>> enabled.
>>> - * TODO: add other APU ASICs which suffer from the same issue here
>>> - */
>>> - case IP_VERSION(11, 0, 8):
>>> - case IP_VERSION(12, 0, 0):
>>> - case IP_VERSION(12, 0, 1):
>>> - memset(feature_mask, 0xff, sizeof(*feature_mask));
>>> - break;
>>
>> This is broken now as these ASICs don't support any message. It is best to
>> take out smu_cmn_get_enabled_mask altogether and move to smu_v*.c or
>> *_ppt.c as this is a callback function.
>>
>> Thanks,
>> Lijo
>>
> Before this solution I also consider put the smu_cmn_get_enabled_mask implementation in each *_ppt directly, but seems need some effort for implementing on each *_ppt. How about we also check the SMU_MSG_GetEnabledSmuFeaturesHigh mapping index? As to the ASCI not support neither SMU_MSG_GetEnabledSmuFeatures nor SMU_MSG_GetEnabledSmuFeaturesHigh will hard code the feature mask in this case.
>
Something like attached refactoring and then for the rest you could
apply your patch.
Thanks,
Lijo
>>> - /* other dGPU ASICs */
>>> - default:
>>> + } else {
>>> ret = smu_cmn_send_smc_msg(smu,
>>>
>> SMU_MSG_GetEnabledSmuFeaturesHigh,
>>> feature_mask_high);
>>> @@ -602,7 +587,6 @@ int smu_cmn_get_enabled_mask(struct
>> smu_context *smu,
>>> ret = smu_cmn_send_smc_msg(smu,
>>>
>> SMU_MSG_GetEnabledSmuFeaturesLow,
>>> feature_mask_low);
>>> - break;
>>> }
>>>
>>> return ret;
>>>
-------------- next part --------------
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
index b3a0f3fb3e65..f1a4a720d426 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
@@ -552,6 +552,16 @@ static int cyan_skillfish_get_dpm_ultimate_freq(struct smu_context *smu,
return 0;
}
+static int cyan_skillfish_get_enabled_mask(struct smu_context *smu,
+ uint64_t *feature_mask)
+{
+ if (!feature_mask)
+ return -EINVAL;
+ memset(feature_mask, 0xff, sizeof(*feature_mask));
+
+ return 0;
+}
+
static const struct pptable_funcs cyan_skillfish_ppt_funcs = {
.check_fw_status = smu_v11_0_check_fw_status,
@@ -562,7 +572,7 @@ static const struct pptable_funcs cyan_skillfish_ppt_funcs = {
.fini_smc_tables = smu_v11_0_fini_smc_tables,
.read_sensor = cyan_skillfish_read_sensor,
.print_clk_levels = cyan_skillfish_print_clk_levels,
- .get_enabled_mask = smu_cmn_get_enabled_mask,
+ .get_enabled_mask = cyan_skillfish_get_enabled_mask,
.is_dpm_running = cyan_skillfish_is_dpm_running,
.get_gpu_metrics = cyan_skillfish_get_gpu_metrics,
.od_edit_dpm_table = cyan_skillfish_od_edit_dpm_table,
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
index e99e7b2bf25b..fd6c44ece168 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
@@ -1366,6 +1366,16 @@ static int renoir_gfx_state_change_set(struct smu_context *smu, uint32_t state)
return 0;
}
+static int renoir_get_enabled_mask(struct smu_context *smu,
+ uint64_t *feature_mask)
+{
+ if (!feature_mask)
+ return -EINVAL;
+ memset(feature_mask, 0xff, sizeof(*feature_mask));
+
+ return 0;
+}
+
static const struct pptable_funcs renoir_ppt_funcs = {
.set_power_state = NULL,
.print_clk_levels = renoir_print_clk_levels,
@@ -1390,7 +1400,7 @@ static const struct pptable_funcs renoir_ppt_funcs = {
.init_smc_tables = renoir_init_smc_tables,
.fini_smc_tables = smu_v12_0_fini_smc_tables,
.set_default_dpm_table = smu_v12_0_set_default_dpm_tables,
- .get_enabled_mask = smu_cmn_get_enabled_mask,
+ .get_enabled_mask = renoir_get_enabled_mask,
.feature_is_enabled = smu_cmn_feature_is_enabled,
.disable_all_features_with_exception = smu_cmn_disable_all_features_with_exception,
.get_dpm_ultimate_freq = renoir_get_dpm_ultimate_freq,
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
index 4c12abcd995d..aa0ca579ad6d 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
@@ -59,6 +59,12 @@ static const char * const __smu_message_names[] = {
SMU_MESSAGE_TYPES
};
+#define smu_cmn_call_asic_func(intf, smu, args...) \
+ ((smu)->ppt_funcs ? ((smu)->ppt_funcs->intf ? \
+ (smu)->ppt_funcs->intf(smu, ##args) : \
+ -ENOTSUPP) : \
+ -EINVAL)
+
static const char *smu_get_message_name(struct smu_context *smu,
enum smu_message_type type)
{
@@ -493,6 +499,12 @@ int smu_cmn_feature_is_supported(struct smu_context *smu,
return test_bit(feature_id, feature->supported);
}
+int __smu_get_enabled_features(struct smu_context *smu,
+ uint64_t *enabled_features)
+{
+ return smu_cmn_call_asic_func(get_enabled_mask, smu, enabled_features);
+}
+
int smu_cmn_feature_is_enabled(struct smu_context *smu,
enum smu_feature_mask mask)
{
@@ -500,7 +512,7 @@ int smu_cmn_feature_is_enabled(struct smu_context *smu,
uint64_t enabled_features;
int feature_id;
- if (smu_cmn_get_enabled_mask(smu, &enabled_features)) {
+ if (__smu_get_enabled_features(smu, &enabled_features)) {
dev_err(adev->dev, "Failed to retrieve enabled ppfeatures!\n");
return 0;
}
@@ -695,8 +707,7 @@ size_t smu_cmn_get_pp_feature_mask(struct smu_context *smu,
int ret = 0, i;
int feature_id;
- ret = smu_cmn_get_enabled_mask(smu,
- &feature_mask);
+ ret = __smu_get_enabled_features(smu, &feature_mask);
if (ret)
return 0;
@@ -748,8 +759,7 @@ int smu_cmn_set_pp_feature_mask(struct smu_context *smu,
uint64_t feature_2_enabled = 0;
uint64_t feature_2_disabled = 0;
- ret = smu_cmn_get_enabled_mask(smu,
- &feature_mask);
+ ret = __smu_get_enabled_features(smu, &feature_mask);
if (ret)
return ret;
More information about the amd-gfx
mailing list