<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<!--[if !mso]><style>v\:* {behavior:url(#default#VML);}
o\:* {behavior:url(#default#VML);}
w\:* {behavior:url(#default#VML);}
.shape {behavior:url(#default#VML);}
</style><![endif]--><style><!--
/* Font Definitions */
@font-face
        {font-family:SimSun;
        panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
        {font-family:"Cambria Math";
        panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
        {font-family:Calibri;
        panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
        {font-family:"Segoe UI";
        panose-1:2 11 5 2 4 2 4 2 2 3;}
@font-face
        {font-family:SimSun;
        panose-1:2 1 6 0 3 1 1 1 1 1;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
        {margin:0in;
        margin-bottom:.0001pt;
        font-size:11.0pt;
        font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
        {mso-style-priority:99;
        color:blue;
        text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
        {mso-style-priority:99;
        color:purple;
        text-decoration:underline;}
p.msonormal0, li.msonormal0, div.msonormal0
        {mso-style-name:msonormal;
        margin:0in;
        margin-bottom:.0001pt;
        font-size:11.0pt;
        font-family:"Calibri",sans-serif;}
span.EmailStyle20
        {mso-style-type:personal-reply;
        font-family:"Calibri",sans-serif;
        color:windowtext;}
.MsoChpDefault
        {mso-style-type:export-only;
        font-size:10.0pt;}
@page WordSection1
        {size:8.5in 11.0in;
        margin:1.0in 1.25in 1.0in 1.25in;}
div.WordSection1
        {page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal">Comment inline<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></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="MsoNormal"><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<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal"><span style="font-size:12.0pt;color:black">comment inline.<o:p></o:p></span></p>
</div>
<div class="MsoNormal" align="center" style="text-align:center">
<hr size="2" width="98%" align="center">
</div>
<div id="divRplyFwdMsg">
<p class="MsoNormal"><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>
<o:p></o:p></p>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
</div>
<div>
<div>
<p class="MsoNormal">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><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><b>[kevin]: this information is not neccessary for public, please remove it.</b><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><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>
+static 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>
+       }<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">//[kevin]: the code logic is a little redundant.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">could you use bellow macro to replace that?<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">header : <span style="font-family:"Segoe UI",sans-serif;color:black;background:white">linux/bitmap.h</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> *  bitmap_and(dst, src1, src2, nbits)          *dst = *src1 & *src2<o:p></o:p></p>
<div>
<p class="MsoNormal"> *  <b>bitmap_or</b>(dst, src1, src2, nbits)           *dst = *src1 | *src2<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> *  bitmap_xor(dst, src1, src2, nbits)          *dst = *src1 ^ *src2<o:p></o:p></p>
</div>
<p class="MsoNormal"> *  <b>bitmap_andnot</b>(dst, src1, src2, nbits)       *dst = *src1 & ~(*src2)<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">+       mutex_unlock(&feature->mutex);<br>
+<br>
+       return ret;<br>
+}<br>
+<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><b><i>[Quan, Evan] updated in v3.<o:p></o:p></i></b></p>
<p class="MsoNormal"><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>
 }<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">[kevin]:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">in this patch, i know you only want to fix not cached feature cache issue,<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">but in v2 patch,<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">the patch adjust the order of code functions, it seems that this is a brand new function,<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">I don't think it is necessary,<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">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>.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">thanks.<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><b><i>[Quan, Evan] Move the API before the place it’s called. No problem here.</i></b><o:p></o:p></p>
</div>
<p class="MsoNormal">-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><o:p></o:p></p>
</div>
</div>
</div>
</div>
</body>
</html>