<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)"> <style></style> </head> <body lang="EN-US" link="blue" vlink="purple"> <div class="WordSection1"> <p class="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.<o:p></o:p></p> <p class="MsoNormal">Thus after that, only update the bitmask is enough. This is done on purpose.<o:p></o:p></p> <p class="MsoNormal"><o:p> </o:p></p> <p class="MsoNormal">Regards,<o:p></o:p></p> <p class="MsoNormal">Evan<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> 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<o:p></o:p></p> </div> </div> <p class="MsoNormal"><o:p> </o:p></p> <div id="divtagdefaultwrapper"> <p><span style="font-size:12.0pt;color:black">in fact, the smu feature bitmap is cached of smc firmware hardware bitmap,<o:p></o:p></span></p> <p><span style="font-size:12.0pt;color:black">most of time, when driver want to check whether enable is available.<o:p></o:p></span></p> <div> <p class="MsoNormal"><span style="font-size:12.0pt;color:black">the driver should talk with smc to check featue enabled. <o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="font-size:12.0pt;color:black">i think it is very low efficiency.<o:p></o:p></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.<o:p></o:p></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.<o:p></o:p></span></p> <p><span style="font-size:12.0pt;color:black"><o:p> </o:p></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.<o:p></o:p></span></p> <p><span style="font-size:12.0pt;color:black"><o:p> </o:p></span></p> <p><span style="font-size:12.0pt;color:black">in your previous patch:<o:p></o:p></span></p> <p><span style="color:#212121">drm/amd/powerplay: correct Navi10 VCN powergate control.</span><span style="font-size:12.0pt;color:black"><o:p></o:p></span></p> <p><span style="font-size:12.0pt;color:black">after your patch, <o:p></o:p></span></p> <p><span style="font-size:12.0pt;color:black">the function smu_feature_is_enabled(smu, SMU_FEATURE_VCN_PG_BIT)<o:p></o:p></span></p> <p><span style="font-size:12.0pt;color:black">will retrun true, but in the firmware, this feature maybe is not enabled.<o:p></o:p></span></p> <p><span style="font-size:12.0pt;color:black"><o:p> </o:p></span></p> <p><span style="font-size:12.0pt;color:black">so the function smu_feature_is_enabled is not work well.<o:p></o:p></span></p> <p><span style="font-size:12.0pt;color:black"><o:p> </o:p></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,<o:p></o:p></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.<o:p></o:p></span></p> <p><span style="font-size:12.0pt;color:black"><o:p> </o:p></span></p> <p><span style="font-size:12.0pt;color:black">Best Regards,<br> Kevin<o:p></o:p></span></p> <div> <p class="MsoNormal"><span style="font-size:12.0pt;color:black"><o:p> </o:p></span></p> </div> </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"> 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> <o:p></o:p></p> <div> <p class="MsoNormal"> <o:p></o:p></p> </div> </div> <div> <div> <p class="xmsonormal">I cannot get your point. What do you mean “pairs of functions”?<o:p></o:p></p> <p class="xmsonormal">Yes, this patch does not bring real changes.<o:p></o:p></p> <p class="xmsonormal">But this helps for future maintain and fit common logic.<o:p></o:p></p> <p class="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.<o:p></o:p></p> <p class="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.<o:p></o:p></p> <p class="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.<o:p></o:p></p> <p class="xmsonormal"> <o:p></o:p></p> <p class="xmsonormal">Regards,<o:p></o:p></p> <p class="xmsonormal">Evan<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="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<o:p></o:p></p> </div> </div> <p class="xmsonormal"> <o:p></o:p></p> <div id="x_divtagdefaultwrapper"> <p class="xmsonormal"><span style="font-size:12.0pt;color:black">before this patch, we have 4 apis to manage smu feature bits. </span><o:p></o:p></p> <div> <p class="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><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:12.0pt;color:black"> </span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:12.0pt;color:black">before your patch:</span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:12.0pt;color:black">smu_feature_is_enabled and smu_feature_set_enabled is pair of functions,</span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:12.0pt;color:black">after your patch:</span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:12.0pt;color:black">smu_feature_is_enabled and smu_enable_smc_feature is pair of functions;</span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:12.0pt;color:black"> </span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:12.0pt;color:black">so the driver don't have scenario needs to use smu_feature_set_enabeld, </span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:12.0pt;color:black">do you agree it?</span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:12.0pt;color:black"> </span><o:p></o:p></p> </div> <div> <p class="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><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:12.0pt;color:black">if not, the smu_feature_is_enabled funciton is not work well.</span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:12.0pt;color:black"> </span><o:p></o:p></p> </div> <div> <div> <p class="xmsonormal"><span style="font-size:9.0pt;color:black">extern int smu_feature_init_dpm(struct smu_context *smu);</span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:9.0pt;color:black">extern int smu_feature_is_enabled(struct smu_context *smu,</span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:9.0pt;color:black"> enum smu_feature_mask mask);</span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:9.0pt;color:black">extern int smu_feature_set_enabled(struct smu_context *smu,</span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:9.0pt;color:black"> enum smu_feature_mask mask, bool enable);</span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:9.0pt;color:black">extern int smu_feature_is_supported(struct smu_context *smu,</span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:9.0pt;color:black"> enum smu_feature_mask mask);</span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:9.0pt;color:black">extern int smu_feature_set_supported(struct smu_context *smu,</span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:9.0pt;color:black"> enum smu_feature_mask mask, bool enable);</span><o:p></o:p></p> </div> <p class="xmsonormal"><span style="font-size:12.0pt;color:black"> </span><o:p></o:p></p> </div> <div> <p class="xmsonormal"><span style="font-size:12.0pt;color:black">Best Regards,<br> Kevin</span><o:p></o:p></p> </div> </div> <div class="MsoNormal" align="center" style="text-align:center"> <hr size="2" width="98%" align="center"> </div> <div id="x_divRplyFwdMsg"> <p class="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> <o:p></o:p></p> <div> <p class="xmsonormal"> <o:p></o:p></p> </div> </div> <div> <div> <p class="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><o:p></o:p></p> </div> </div> </div> </div> </div> </div> </div> </body> </html>