drm/amd/powerplay: update cached feature enablement status V2

Submitted by Wang, Kevin(Yang) on Aug. 23, 2019, 5:29 a.m.

Details

Message ID MN2PR12MB32969ABC3F90939200C01E64A2A40@MN2PR12MB3296.namprd12.prod.outlook.com
State New
Headers show
Series "drm/amd/powerplay: update cached feature enablement status" ( rev: 5 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Wang, Kevin(Yang) Aug. 23, 2019, 5:29 a.m.
comment inline

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
index 4df7fb6eaf3c..3e1cd5d9c29e 100644
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
@@ -94,6 +94,55 @@  size_t smu_sys_get_pp_feature_mask(struct smu_context *smu, char *buf)
         return size;
 }

+static int smu_feature_update_enable_state(struct smu_context *smu,
+                                          uint64_t feature_mask,
+                                          bool enabled)
+{
+       struct smu_feature *feature = &smu->smu_feature;
+       uint32_t feature_low = 0, feature_high = 0;
+       uint64_t feature_id;
+       int ret = 0;
+
+       if (!smu->pm_enabled)
+               return ret;
+
+       feature_low = (feature_mask >> 0 ) & 0xffffffff;
+       feature_high = (feature_mask >> 32) & 0xffffffff;
+
+       if (enabled) {
+               ret = smu_send_smc_msg_with_param(smu, SMU_MSG_EnableSmuFeaturesLow,
+                                                 feature_low);
+               if (ret)
+                       return ret;
+               ret = smu_send_smc_msg_with_param(smu, SMU_MSG_EnableSmuFeaturesHigh,
+                                                 feature_high);
+               if (ret)
+                       return ret;
+       } else {
+               ret = smu_send_smc_msg_with_param(smu, SMU_MSG_DisableSmuFeaturesLow,
+                                                 feature_low);
+               if (ret)
+                       return ret;
+               ret = smu_send_smc_msg_with_param(smu, SMU_MSG_DisableSmuFeaturesHigh,
+                                                 feature_high);
+               if (ret)
+                       return ret;
+       }
+
+       mutex_lock(&feature->mutex);
+       for (feature_id = 0; feature_id < 64; feature_id++) {
+               if (feature_mask & (1ULL << feature_id)) {
+                       if (enabled)
+                               test_and_set_bit(feature_id, feature->enabled);
+                       else
+                               test_and_clear_bit(feature_id, feature->enabled);
+               }
+       }



//[kevin]: the code logic is a little redundant.

could you use bellow macro to replace that?

header : linux/bitmap.h

 *  bitmap_and(dst, src1, src2, nbits)          *dst = *src1 & *src2

 *  bitmap_or(dst, src1, src2, nbits)           *dst = *src1 | *src2

 *  bitmap_xor(dst, src1, src2, nbits)          *dst = *src1 ^ *src2

 *  bitmap_andnot(dst, src1, src2, nbits)       *dst = *src1 & ~(*src2)



+       mutex_unlock(&feature->mutex);
+
+       return ret;
+}
+



[Quan, Evan] updated in v3.

 int smu_sys_set_pp_feature_mask(struct smu_context *smu, uint64_t new_mask)
 {
         int ret = 0;
@@ -591,41 +640,7 @@  int smu_feature_init_dpm(struct smu_context *smu)

         return ret;
 }



[kevin]:

in this patch, i know you only want to fix not cached feature cache issue,

but in v2 patch,

the patch adjust the order of code functions, it seems that this is a brand new function,

I don't think it is necessary,

could you just reflect the modified content in the patch, which can facilitate us to trace problems and review.

thanks.



[Quan, Evan] Move the API before the place it’s called. No problem here.


[kevin]: in this patch, you don't need to adjust function order in this file,

because the driver is already export this function in amdgpu_smu.h.

int smu_feature_update_enable_state(struct smu_context *smu, uint64_t feature_mask, bool enabled);

and in this patch, it make it is static function, but the declaration section remains in the amdgpu_smu.h file.

so i don't want to you adjust the function order and make it is static funtion in this patch.

except you have other reason for it.


-int smu_feature_update_enable_state(struct smu_context *smu, uint64_t feature_mask, bool enabled)
-{
-       uint32_t feature_low = 0, feature_high = 0;
-       int ret = 0;

-       if (!smu->pm_enabled)
-               return ret;
-
-       feature_low = (feature_mask >> 0 ) & 0xffffffff;
-       feature_high = (feature_mask >> 32) & 0xffffffff;
-
-       if (enabled) {
-               ret = smu_send_smc_msg_with_param(smu, SMU_MSG_EnableSmuFeaturesLow,
-                                                 feature_low);
-               if (ret)
-                       return ret;
-               ret = smu_send_smc_msg_with_param(smu, SMU_MSG_EnableSmuFeaturesHigh,
-                                                 feature_high);
-               if (ret)
-                       return ret;
-
-       } else {
-               ret = smu_send_smc_msg_with_param(smu, SMU_MSG_DisableSmuFeaturesLow,
-                                                 feature_low);
-               if (ret)
-                       return ret;
-               ret = smu_send_smc_msg_with_param(smu, SMU_MSG_DisableSmuFeaturesHigh,
-                                                 feature_high);
-               if (ret)
-                       return ret;
-
-       }
-
-       return ret;
-}

 int smu_feature_is_enabled(struct smu_context *smu, enum smu_feature_mask mask)
 {
@@ -651,8 +666,6 @@  int smu_feature_set_enabled(struct smu_context *smu, enum smu_feature_mask mask,
 {
         struct smu_feature *feature = &smu->smu_feature;
         int feature_id;
-       uint64_t feature_mask = 0;
-       int ret = 0;

         feature_id = smu_feature_get_index(smu, mask);
         if (feature_id < 0)
@@ -660,22 +673,9 @@  int smu_feature_set_enabled(struct smu_context *smu, enum smu_feature_mask mask,

         WARN_ON(feature_id > feature->feature_num);

-       feature_mask = 1ULL << feature_id;
-
-       mutex_lock(&feature->mutex);
-       ret = smu_feature_update_enable_state(smu, feature_mask, enable);
-       if (ret)
-               goto failed;
-
-       if (enable)
-               test_and_set_bit(feature_id, feature->enabled);
-       else
-               test_and_clear_bit(feature_id, feature->enabled);
-
-failed:
-       mutex_unlock(&feature->mutex);
-
-       return ret;
+       return smu_feature_update_enable_state(smu,
+                                              1ULL << feature_id,
+                                              enable);
 }

 int smu_feature_is_supported(struct smu_context *smu, enum smu_feature_mask mask)
diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
index e80c81552d29..fbf68fd42b93 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
@@ -807,7 +807,6 @@  enum amd_dpm_forced_level smu_get_performance_level(struct smu_context *smu);
 int smu_force_performance_level(struct smu_context *smu, enum amd_dpm_forced_level level);
 int smu_set_display_count(struct smu_context *smu, uint32_t count);
 bool smu_clk_dpm_is_enabled(struct smu_context *smu, enum smu_clk_type clk_type);
-int smu_feature_update_enable_state(struct smu_context *smu, uint64_t feature_mask, bool enabled);
 const char *smu_get_message_name(struct smu_context *smu, enum smu_message_type type);
 const char *smu_get_feature_name(struct smu_context *smu, enum smu_feature_mask feature);
 size_t smu_sys_get_pp_feature_mask(struct smu_context *smu, char *buf);