<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>