<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 id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr">
<p style="margin-top:0;margin-bottom:0">it seems this feature is specific feature on some asic in smc firmware.</p>
<p style="margin-top:0;margin-bottom:0">why not create a specific function to handle this case,</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">the apis of smu_feature_is_enabled and smu_feature_set_enabled is public api for all smu asic.<br>
<span style="font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols; font-size: 16px;">how can you be sure that message must have the same logic on other
 asics in SMC firwamre.</span><br>
</p>
<p style="margin-top:0;margin-bottom:0"><span style="font-size: 12pt;">may be this case not work on other smu or other asic.</span></p>
<div><br>
</div>
<p style="margin-top:0;margin-bottom:0">if you still do, it means that when developer use smu_feature_is_enabled api,</p>
<p style="margin-top:0;margin-bottom:0">the user must know the message <span>detail</span> information in smc firmware.</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">sw-bitmap and smc firwmare bitmap must be same in smu driver.</p>
<p style="margin-top:0;margin-bottom:0"></p>
<div>if not, this sw-bitmap is meaningless.</div>
<div><br>
</div>
<p style="margin-top:0;margin-bottom:0">Best Regards,</p>
<p style="margin-top:0;margin-bottom:0">Kevin</p>
</div>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> Quan, Evan <Evan.Quan@amd.com><br>
<b>Sent:</b> Monday, July 22, 2019 8:45:15 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: correct confusing naming for smu_feature_set_enabled</font>
<div> </div>
</div>
<style>
<!--
@font-face
        {font-family:SimSun}
@font-face
        {font-family:"Cambria Math"}
@font-face
        {font-family:Calibri}
@font-face
        {font-family:SimSun}
p.x_MsoNormal, li.x_MsoNormal, div.x_MsoNormal
        {margin:0in;
        margin-bottom:.0001pt;
        font-size:11.0pt;
        font-family:"Calibri",sans-serif}
a:link, span.x_MsoHyperlink
        {color:blue;
        text-decoration:underline}
a:visited, span.x_MsoHyperlinkFollowed
        {color:purple;
        text-decoration:underline}
p.x_msonormal0, li.x_msonormal0, div.x_msonormal0
        {margin:0in;
        margin-bottom:.0001pt;
        font-size:11.0pt;
        font-family:"Calibri",sans-serif}
p.x_xmsonormal, li.x_xmsonormal, div.x_xmsonormal
        {margin:0in;
        margin-bottom:.0001pt;
        font-size:11.0pt;
        font-family:"Calibri",sans-serif}
p.x_xmsonormal0, li.x_xmsonormal0, div.x_xmsonormal0
        {margin:0in;
        margin-bottom:.0001pt;
        font-size:11.0pt;
        font-family:"Calibri",sans-serif}
p.x_xmsochpdefault, li.x_xmsochpdefault, div.x_xmsochpdefault
        {margin-right:0in;
        margin-left:0in;
        font-size:10.0pt;
        font-family:"Calibri",sans-serif}
span.x_xmsohyperlink
        {color:blue;
        text-decoration:underline}
span.x_xmsohyperlinkfollowed
        {color:purple;
        text-decoration:underline}
span.x_xemailstyle18
        {font-family:"Calibri",sans-serif;
        color:windowtext}
span.x_EmailStyle26
        {font-family:"Calibri",sans-serif;
        color:windowtext}
.x_MsoChpDefault
        {font-size:10.0pt}
@page WordSection1
        {margin:1.0in 1.25in 1.0in 1.25in}
div.x_WordSection1
        {}
-->
</style>
<div lang="EN-US" link="blue" vlink="purple">
<div class="x_WordSection1">
<p class="x_MsoNormal">The implementation of SMU_MSG_PowerDown[UP]Vcn (in smu fw) already includes the identical hardware enablement/disablement of SMU_FEATURE_VCN_PG_BIT.</p>
<p class="x_MsoNormal">Thus after that, only update the bitmask is enough. This is done on purpose.</p>
<p class="x_MsoNormal"> </p>
<p class="x_MsoNormal">Regards,</p>
<p class="x_MsoNormal">Evan</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"><b>From:</b> Wang, Kevin(Yang) <Kevin1.Wang@amd.com> <br>
<b>Sent:</b> Monday, July 22, 2019 4:11 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: correct confusing naming for smu_feature_set_enabled</p>
</div>
</div>
<p class="x_MsoNormal"> </p>
<div id="x_divtagdefaultwrapper">
<p><span style="font-size:12.0pt; color:black">in fact, the smu feature bitmap is cached of smc firmware hardware bitmap,</span></p>
<p><span style="font-size:12.0pt; color:black">most of time, when driver want to check whether enable is available.</span></p>
<div>
<p class="x_MsoNormal"><span style="font-size:12.0pt; color:black">the driver should talk with smc to check featue enabled. </span></p>
</div>
<div>
<p class="x_MsoNormal"><span style="font-size:12.0pt; color:black">i think it is very low efficiency.</span></p>
</div>
<p><span style="font-size:12.0pt; color:black">so the driver will provide a bitmap structure to store hardware feature status.</span></p>
<p><span style="font-size:12.0pt; color:black">every time, the driver update hardware feature state, also should be update software bitmap to sync it.</span></p>
<p><span style="font-size:12.0pt; color:black"> </span></p>
<p><span style="font-size:12.0pt; color:black">if only update bitmap and don't set firmware feature enabled, the smu_feature_is_enabeld is maybe not work correctly.</span></p>
<p><span style="font-size:12.0pt; color:black"> </span></p>
<p><span style="font-size:12.0pt; color:black">in your previous patch:</span></p>
<p><span style="color:#212121">drm/amd/powerplay: correct Navi10 VCN powergate control.</span><span style="font-size:12.0pt; color:black"></span></p>
<p><span style="font-size:12.0pt; color:black">after your patch, </span></p>
<p><span style="font-size:12.0pt; color:black">the function smu_feature_is_enabled(smu, SMU_FEATURE_VCN_PG_BIT)</span></p>
<p><span style="font-size:12.0pt; color:black">will retrun true, but in the firmware, this feature maybe is not enabled.</span></p>
<p><span style="font-size:12.0pt; color:black"> </span></p>
<p><span style="font-size:12.0pt; color:black">so the function smu_feature_is_enabled is not work well.</span></p>
<p><span style="font-size:12.0pt; color:black"> </span></p>
<p><span style="font-size:12.0pt; color:black">and smu_feature_is_supported  is full software feature, this is helper function to set allowed feature mask when smu power on,</span></p>
<p><span style="font-size:12.0pt; color:black">but this function is not used in smu driver, i want to remove them long long ago.</span></p>
<p><span style="font-size:12.0pt; color:black"> </span></p>
<p><span style="font-size:12.0pt; color:black">Best Regards,<br>
Kevin</span></p>
<div>
<p class="x_MsoNormal"><span style="font-size:12.0pt; color:black"> </span></p>
</div>
</div>
<div class="x_MsoNormal" align="center" style="text-align:center">
<hr size="2" width="98%" align="center">
</div>
<div id="x_divRplyFwdMsg">
<p class="x_MsoNormal"><b><span style="color:black">From:</span></b><span style="color:black"> Quan, Evan <<a href="mailto:Evan.Quan@amd.com">Evan.Quan@amd.com</a>><br>
<b>Sent:</b> Monday, July 22, 2019 3:17:55 PM<br>
<b>To:</b> Wang, Kevin(Yang) <<a href="mailto:Kevin1.Wang@amd.com">Kevin1.Wang@amd.com</a>>;
<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>Subject:</b> RE: [PATCH] drm/amd/powerplay: correct confusing naming for smu_feature_set_enabled</span>
</p>
<div>
<p class="x_MsoNormal"> </p>
</div>
</div>
<div>
<div>
<p class="x_xmsonormal">I cannot get your point. What do you mean “pairs of functions”?</p>
<p class="x_xmsonormal">Yes, this patch does not bring real changes.</p>
<p class="x_xmsonormal">But this helps for future maintain and fit common logic.</p>
<p class="x_xmsonormal">1. As in my previous patch(“drm/amd/powerplay: correct Navi10 VCN powergate control” ),  “smu_feature_set_enabled(smu, SMU_FEATURE_VCN_PG_BIT, enable);” was mistakenly used. I thought and expected smu_feature_set_enabled set the bitmask
 on only. I do not want it to enable or disable the feature.</p>
<p class="x_xmsonormal">2. smu_feature_set_enabled should in the same logic as smu_feature_set_supported. It updates only the saved bit mask. That’s the expected behavior for some APIs named as _<i>set</i>_enabled/supported.</p>
<p class="x_xmsonormal">3. The original callers who want feature enablement/disablement are updated to use smu_enable_smc_feature(). I do not see any problem with that.</p>
<p class="x_xmsonormal"> </p>
<p class="x_xmsonormal">Regards,</p>
<p class="x_xmsonormal">Evan</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_xmsonormal"><b>From:</b> Wang, Kevin(Yang) <<a href="mailto:Kevin1.Wang@amd.com">Kevin1.Wang@amd.com</a>>
<br>
<b>Sent:</b> Monday, July 22, 2019 3:00 PM<br>
<b>To:</b> Quan, Evan <<a href="mailto:Evan.Quan@amd.com">Evan.Quan@amd.com</a>>;
<a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a><br>
<b>Subject:</b> Re: [PATCH] drm/amd/powerplay: correct confusing naming for smu_feature_set_enabled</p>
</div>
</div>
<p class="x_xmsonormal"> </p>
<div id="x_x_divtagdefaultwrapper">
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black">before this patch, we have 4 apis to manage smu feature bits.
</span></p>
<div>
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black">the patch add a new one in them, but it is not add any feature in smu.</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black"> </span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black">before your patch:</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black">smu_feature_is_enabled and smu_feature_set_enabled is pair of functions,</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black">after your patch:</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black">smu_feature_is_enabled and smu_enable_smc_feature is pair of functions;</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black"> </span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black">so the driver don't have scenario needs to use smu_feature_set_enabeld, </span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black">do you agree it?</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black"> </span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black">most of the time we update SMC feature, we must update software bitmap in smu_feature structure.</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black">if not, the smu_feature_is_enabled funciton is not work well.</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black"> </span></p>
</div>
<div>
<div>
<p class="x_xmsonormal"><span style="font-size:9.0pt; color:black">extern int smu_feature_init_dpm(struct smu_context *smu);</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:9.0pt; color:black">extern int smu_feature_is_enabled(struct smu_context *smu,</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:9.0pt; color:black">  enum smu_feature_mask mask);</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:9.0pt; color:black">extern int smu_feature_set_enabled(struct smu_context *smu,</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:9.0pt; color:black">   enum smu_feature_mask mask, bool enable);</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:9.0pt; color:black">extern int smu_feature_is_supported(struct smu_context *smu,</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:9.0pt; color:black">    enum smu_feature_mask mask);</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:9.0pt; color:black">extern int smu_feature_set_supported(struct smu_context *smu,</span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:9.0pt; color:black">     enum smu_feature_mask mask, bool enable);</span></p>
</div>
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black"> </span></p>
</div>
<div>
<p class="x_xmsonormal"><span style="font-size:12.0pt; color:black">Best Regards,<br>
Kevin</span></p>
</div>
</div>
<div class="x_MsoNormal" align="center" style="text-align:center">
<hr size="2" width="98%" align="center">
</div>
<div id="x_x_divRplyFwdMsg">
<p class="x_xmsonormal"><b><span style="color:black">From:</span></b><span style="color:black"> amd-gfx <</span><a href="mailto:amd-gfx-bounces@lists.freedesktop.org">amd-gfx-bounces@lists.freedesktop.org</a><span style="color:black">> on behalf of Evan Quan
 <</span><a href="mailto:evan.quan@amd.com">evan.quan@amd.com</a><span style="color:black">><br>
<b>Sent:</b> Monday, July 22, 2019 2:34:26 PM<br>
<b>To:</b> </span><a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a><span style="color:black"> <</span><a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a><span style="color:black">><br>
<b>Cc:</b> Quan, Evan <</span><a href="mailto:Evan.Quan@amd.com">Evan.Quan@amd.com</a><span style="color:black">><br>
<b>Subject:</b> [PATCH] drm/amd/powerplay: correct confusing naming for smu_feature_set_enabled</span>
</p>
<div>
<p class="x_xmsonormal"> </p>
</div>
</div>
<div>
<div>
<p class="x_xmsonormal">It does more than updating the bitmask. In fact it enables also the<br>
feature. That's confusing. As for this, a new API is added for the<br>
feature enablement job. And smu_feature_set_enabled is updated to<br>
setting the bitmask only(as smu_feature_set_supported).<br>
<br>
Change-Id: I758e4461be34c0fcbdf19448e34180a5251926c4<br>
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    | 29 +++++++++++++------<br>
 .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h    |  2 ++<br>
 drivers/gpu/drm/amd/powerplay/smu_v11_0.c     |  2 +-<br>
 drivers/gpu/drm/amd/powerplay/vega20_ppt.c    |  4 +--<br>
 4 files changed, 25 insertions(+), 12 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 4e18f33a1bab..9262883d4031 100644<br>
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c<br>
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c<br>
@@ -511,28 +511,39 @@ 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>
-       int ret = 0;<br>
 <br>
         feature_id = smu_feature_get_index(smu, mask);<br>
         if (feature_id < 0)<br>
                 return -EINVAL;<br>
 <br>
-       WARN_ON(feature_id > feature->feature_num);<br>
-<br>
         mutex_lock(&feature->mutex);<br>
-       ret = smu_feature_update_enable_state(smu, feature_id, 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 0;<br>
+}<br>
+<br>
+int smu_enable_smc_feature(struct smu_context *smu,<br>
+                          enum smu_feature_mask mask,<br>
+                          bool enable)<br>
+{<br>
+       int feature_id;<br>
+       int ret = 0;<br>
+<br>
+       feature_id = smu_feature_get_index(smu, mask);<br>
+       if (feature_id < 0)<br>
+               return -EINVAL;<br>
+<br>
+       ret = smu_feature_update_enable_state(smu, feature_id, enable);<br>
+       if (ret)<br>
+               return ret;<br>
+<br>
+       return smu_feature_set_enabled(smu, mask, enable);<br>
 }<br>
 <br>
 int smu_feature_is_supported(struct smu_context *smu, enum smu_feature_mask mask)<br>
@@ -1186,7 +1197,7 @@ static int smu_suspend(void *handle)<br>
                 return ret;<br>
 <br>
         if (adev->in_gpu_reset && baco_feature_is_enabled) {<br>
-               ret = smu_feature_set_enabled(smu, SMU_FEATURE_BACO_BIT, true);<br>
+               ret = smu_enable_smc_feature(smu, SMU_FEATURE_BACO_BIT, true);<br>
                 if (ret) {<br>
                         pr_warn("set BACO feature enabled failed, return %d\n", ret);<br>
                         return ret;<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 b702c9ee975f..267b879796f9 100644<br>
--- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h<br>
+++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h<br>
@@ -946,6 +946,8 @@ extern int smu_feature_is_enabled(struct smu_context *smu,<br>
                                   enum smu_feature_mask mask);<br>
 extern int smu_feature_set_enabled(struct smu_context *smu,<br>
                                    enum smu_feature_mask mask, bool enable);<br>
+extern int smu_enable_smc_feature(struct smu_context *smu,<br>
+                                  enum smu_feature_mask mask, bool enable);<br>
 extern int smu_feature_is_supported(struct smu_context *smu,<br>
                                     enum smu_feature_mask mask);<br>
 extern int smu_feature_set_supported(struct smu_context *smu,<br>
diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c<br>
index e3a178403546..0f59d2178d01 100644<br>
--- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c<br>
+++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c<br>
@@ -1419,7 +1419,7 @@ smu_v11_0_smc_fan_control(struct smu_context *smu, bool start)<br>
         if (smu_feature_is_supported(smu, SMU_FEATURE_FAN_CONTROL_BIT))<br>
                 return 0;<br>
 <br>
-       ret = smu_feature_set_enabled(smu, SMU_FEATURE_FAN_CONTROL_BIT, start);<br>
+       ret = smu_enable_smc_feature(smu, SMU_FEATURE_FAN_CONTROL_BIT, start);<br>
         if (ret)<br>
                 pr_err("[%s]%s smc FAN CONTROL feature failed!",<br>
                        __func__, (start ? "Start" : "Stop"));<br>
diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c<br>
index 9ead36192787..536ff884ddca 100644<br>
--- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c<br>
+++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c<br>
@@ -2845,7 +2845,7 @@ static int vega20_dpm_set_uvd_enable(struct smu_context *smu, bool enable)<br>
         if (enable == smu_feature_is_enabled(smu, SMU_FEATURE_DPM_UVD_BIT))<br>
                 return 0;<br>
 <br>
-       return smu_feature_set_enabled(smu, SMU_FEATURE_DPM_UVD_BIT, enable);<br>
+       return smu_enable_smc_feature(smu, SMU_FEATURE_DPM_UVD_BIT, enable);<br>
 }<br>
 <br>
 static int vega20_dpm_set_vce_enable(struct smu_context *smu, bool enable)<br>
@@ -2856,7 +2856,7 @@ static int vega20_dpm_set_vce_enable(struct smu_context *smu, bool enable)<br>
         if (enable == smu_feature_is_enabled(smu, SMU_FEATURE_DPM_VCE_BIT))<br>
                 return 0;<br>
 <br>
-       return smu_feature_set_enabled(smu, SMU_FEATURE_DPM_VCE_BIT, enable);<br>
+       return smu_enable_smc_feature(smu, SMU_FEATURE_DPM_VCE_BIT, enable);<br>
 }<br>
 <br>
 static int vega20_get_enabled_smc_features(struct smu_context *smu,<br>
-- <br>
2.22.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>
</div>
</div>
</div>
</body>
</html>