<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)">
comment inline</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)">
</div>
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>From:</b> Quan, Evan <Evan.Quan@amd.com><br>
<b>Sent:</b> Friday, August 23, 2019 12:50 PM<br>
<b>To:</b> Wang, Kevin(Yang) <Kevin1.Wang@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org><br>
<b>Subject:</b> RE: [PATCH] drm/amd/powerplay: update cached feature enablement status V2</font>
<div> </div>
</div>
<div lang="EN-US">
<div class="x_WordSection1">
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
Comment inline</p>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
</p>
<div style="border:none; border-left:solid blue 1.5pt; padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none; border-top:solid #E1E1E1 1.0pt; padding:3.0pt 0in 0in 0in">
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
<b>From:</b> Wang, Kevin(Yang) <Kevin1.Wang@amd.com> <br>
<b>Sent:</b> Thursday, August 22, 2019 8:00 PM<br>
<b>To:</b> Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org<br>
<b>Subject:</b> Re: [PATCH] drm/amd/powerplay: update cached feature enablement status V2</p>
</div>
</div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
</p>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
<span style="font-size:12.0pt; color:black">comment inline.</span></p>
</div>
<div class="x_MsoNormal" align="center" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;text-align:center">
<hr size="2" width="98%" align="center">
</div>
<div id="x_divRplyFwdMsg">
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
<b><span style="color:black">From:</span></b><span style="color:black"> amd-gfx <<a href="mailto:amd-gfx-bounces@lists.freedesktop.org">amd-gfx-bounces@lists.freedesktop.org</a>> on behalf of Evan Quan <<a href="mailto:evan.quan@amd.com">evan.quan@amd.com</a>><br>
<b>Sent:</b> Thursday, August 22, 2019 6:18 PM<br>
<b>To:</b> <a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> <<a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a>><br>
<b>Cc:</b> Quan, Evan <<a href="mailto:Evan.Quan@amd.com">Evan.Quan@amd.com</a>><br>
<b>Subject:</b> [PATCH] drm/amd/powerplay: update cached feature enablement status V2</span>
</p>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
</p>
</div>
</div>
<div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
Need to update in cache feature enablement status after pp_feature<br>
settings. Another fix for the commit below:<br>
drm/amd/powerplay: implment sysfs feature status function in smu<br>
<br>
V2: update smu_feature_update_enable_state() and relates<br>
<br>
<b>Change-Id: I90e29b0d839df26825d5993212f6097c7ad4bebf </b></p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
<b>[kevin]: this information is not neccessary for public, please remove it.</b></p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
<b>git config gerrit.createchangeid=false<br>
</b>Signed-off-by: Evan Quan <<a href="mailto:evan.quan@amd.com">evan.quan@amd.com</a>><br>
---<br>
drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 104 +++++++++---------<br>
.../gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 1 -<br>
2 files changed, 52 insertions(+), 53 deletions(-)<br>
<br>
diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c<br>
index 4df7fb6eaf3c..3e1cd5d9c29e 100644<br>
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c<br>
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c<br>
@@ -94,6 +94,55 @@ size_t smu_sys_get_pp_feature_mask(struct smu_context *smu, char *buf)<br>
return size;<br>
}<br>
<br>
+<b>static</b> int smu_feature_update_enable_state(struct smu_context *smu,<br>
+ uint64_t feature_mask,<br>
+ bool enabled)<br>
+{<br>
+ struct smu_feature *feature = &smu->smu_feature;<br>
+ uint32_t feature_low = 0, feature_high = 0;<br>
+ uint64_t feature_id;<br>
+ int ret = 0;<br>
+<br>
+ if (!smu->pm_enabled)<br>
+ return ret;<br>
+<br>
+ feature_low = (feature_mask >> 0 ) & 0xffffffff;<br>
+ feature_high = (feature_mask >> 32) & 0xffffffff;<br>
+<br>
+ if (enabled) {<br>
+ ret = smu_send_smc_msg_with_param(smu, SMU_MSG_EnableSmuFeaturesLow,<br>
+ feature_low);<br>
+ if (ret)<br>
+ return ret;<br>
+ ret = smu_send_smc_msg_with_param(smu, SMU_MSG_EnableSmuFeaturesHigh,<br>
+ feature_high);<br>
+ if (ret)<br>
+ return ret;<br>
+ } else {<br>
+ ret = smu_send_smc_msg_with_param(smu, SMU_MSG_DisableSmuFeaturesLow,<br>
+ feature_low);<br>
+ if (ret)<br>
+ return ret;<br>
+ ret = smu_send_smc_msg_with_param(smu, SMU_MSG_DisableSmuFeaturesHigh,<br>
+ feature_high);<br>
+ if (ret)<br>
+ return ret;<br>
+ }<br>
+<br>
+ mutex_lock(&feature->mutex);<br>
+ for (feature_id = 0; feature_id < 64; feature_id++) {<br>
+ if (feature_mask & (1ULL << feature_id)) {<br>
+ if (enabled)<br>
+ test_and_set_bit(feature_id, feature->enabled);<br>
+ else<br>
+ test_and_clear_bit(feature_id, feature->enabled);<br>
+ }<br>
+ }</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
//[kevin]: the code logic is a little redundant.</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
could you use bellow macro to replace that?</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
header : <span style="font-family:"Segoe UI",sans-serif; color:black; background:white">linux/bitmap.h</span></p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
* bitmap_and(dst, src1, src2, nbits) *dst = *src1 & *src2</p>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
* <b>bitmap_or</b>(dst, src1, src2, nbits) *dst = *src1 | *src2</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
* bitmap_xor(dst, src1, src2, nbits) *dst = *src1 ^ *src2</p>
</div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
* <b>bitmap_andnot</b>(dst, src1, src2, nbits) *dst = *src1 & ~(*src2)</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
+ mutex_unlock(&feature->mutex);<br>
+<br>
+ return ret;<br>
+}<br>
+</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
</p>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
<b><i>[Quan, Evan] updated in v3.</i></b></p>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
<br>
int smu_sys_set_pp_feature_mask(struct smu_context *smu, uint64_t new_mask)<br>
{<br>
int ret = 0;<br>
@@ -591,41 +640,7 @@ int smu_feature_init_dpm(struct smu_context *smu)<br>
<br>
return ret;<br>
}</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
[kevin]:</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
in this patch, i know you only want to fix not cached feature cache issue,</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
but in v2 patch,</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
the patch adjust the order of code functions, it seems that this is a brand new function,</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
I don't think it is necessary,</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
could you just reflect the <b>modified content</b> in the patch, which can facilitate us to trace
<b>problems </b>and <b>review</b>.</p>
</div>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
thanks.</p>
<div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
</p>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
<b><i>[Quan, Evan] Move the API before the place it’s called. No problem here.</i></b></p>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
<b><i><br>
</i></b></p>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
<b><i>[kevin]: in this patch, you don't need to adjust function order in this file,</i></b></p>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
<b><i>because the driver is already export this function in amdgpu_smu.h.</i></b></p>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
<b><i>int smu_feature_update_enable_state(struct smu_context *smu, uint64_t feature_mask, bool enabled);<br>
</i></b></p>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
<b><i>and in this patch, it make it is static function, but <span>the declaration section remains in the amdgpu_smu.h file.</span></i></b></p>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
<b><i><span>so i don't want to you adjust the function order and make it is static funtion in this patch.</span></i></b></p>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
<b><i><span>except you have other reason for it.<br>
</span></p>
<div><br>
</div>
<span></span></i></b>
<p></p>
</div>
<p class="x_MsoNormal" style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;">
-int smu_feature_update_enable_state(struct smu_context *smu, uint64_t feature_mask, bool enabled)<br>
-{<br>
- uint32_t feature_low = 0, feature_high = 0;<br>
- int ret = 0;<br>
<br>
- if (!smu->pm_enabled)<br>
- return ret;<br>
-<br>
- feature_low = (feature_mask >> 0 ) & 0xffffffff;<br>
- feature_high = (feature_mask >> 32) & 0xffffffff;<br>
-<br>
- if (enabled) {<br>
- ret = smu_send_smc_msg_with_param(smu, SMU_MSG_EnableSmuFeaturesLow,<br>
- feature_low);<br>
- if (ret)<br>
- return ret;<br>
- ret = smu_send_smc_msg_with_param(smu, SMU_MSG_EnableSmuFeaturesHigh,<br>
- feature_high);<br>
- if (ret)<br>
- return ret;<br>
-<br>
- } else {<br>
- ret = smu_send_smc_msg_with_param(smu, SMU_MSG_DisableSmuFeaturesLow,<br>
- feature_low);<br>
- if (ret)<br>
- return ret;<br>
- ret = smu_send_smc_msg_with_param(smu, SMU_MSG_DisableSmuFeaturesHigh,<br>
- feature_high);<br>
- if (ret)<br>
- return ret;<br>
-<br>
- }<br>
-<br>
- return ret;<br>
-}<br>
<br>
int smu_feature_is_enabled(struct smu_context *smu, enum smu_feature_mask mask)<br>
{<br>
@@ -651,8 +666,6 @@ int smu_feature_set_enabled(struct smu_context *smu, enum smu_feature_mask mask,<br>
{<br>
struct smu_feature *feature = &smu->smu_feature;<br>
int feature_id;<br>
- uint64_t feature_mask = 0;<br>
- int ret = 0;<br>
<br>
feature_id = smu_feature_get_index(smu, mask);<br>
if (<span style="font-family:"Arial",sans-serif">feature_id </span>< 0)<br>
@@ -660,22 +673,9 @@ int smu_feature_set_enabled(struct smu_context *smu, enum smu_feature_mask mask,<br>
<br>
WARN_ON(feature_id > feature->feature_num);<br>
<br>
- feature_mask = 1ULL << feature_id;<br>
-<br>
- mutex_lock(&feature->mutex);<br>
- ret = smu_feature_update_enable_state(smu, feature_mask, enable);<br>
- if (ret)<br>
- goto failed;<br>
-<br>
- if (enable)<br>
- test_and_set_bit(feature_id, feature->enabled);<br>
- else<br>
- test_and_clear_bit(feature_id, feature->enabled);<br>
-<br>
-failed:<br>
- mutex_unlock(&feature->mutex);<br>
-<br>
- return ret;<br>
+ return smu_feature_update_enable_state(smu,<br>
+ 1ULL << feature_id,<br>
+ enable);<br>
}<br>
<br>
int smu_feature_is_supported(struct smu_context *smu, enum smu_feature_mask mask)<br>
diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h<br>
index e80c81552d29..fbf68fd42b93 100644<br>
--- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h<br>
+++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h<br>
@@ -807,7 +807,6 @@ enum amd_dpm_forced_level smu_get_performance_level(struct smu_context *smu);<br>
int smu_force_performance_level(struct smu_context *smu, enum amd_dpm_forced_level level);<br>
int smu_set_display_count(struct smu_context *smu, uint32_t count);<br>
bool smu_clk_dpm_is_enabled(struct smu_context *smu, enum smu_clk_type clk_type);<br>
-int smu_feature_update_enable_state(struct smu_context *smu, uint64_t feature_mask, bool enabled);<br>
const char *smu_get_message_name(struct smu_context *smu, enum smu_message_type type);<br>
const char *smu_get_feature_name(struct smu_context *smu, enum smu_feature_mask feature);<br>
size_t smu_sys_get_pp_feature_mask(struct smu_context *smu, char *buf);<br>
-- <br>
2.23.0<br>
<br>
_______________________________________________<br>
amd-gfx mailing list<br>
<a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a></p>
</div>
</div>
</div>
</div>
</div>
</body>
</html>