<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=Windows-1252"> <style type="text/css" style="display:none;"></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">in fact, the smu feature bitmap is cached of smc firmware hardware bitmap,</p> <p style="margin-top:0;margin-bottom:0">most of time, when driver w<span style="font-size: 12pt;">ant to check whether enable is available.</span></p> <div><span style="font-size: 12pt;">the driver should talk with smc to check featue enabled. </span></div> <div><span style="font-size: 12pt;">i think it is very l</span><span style="font-size: 12pt;">ow efficiency.</span></div> <p></p> <p style="margin-top:0;margin-bottom:0"></p> <p style="margin-top:0;margin-bottom:0">so the driver will provide a bitmap structure to store hardware feature status.</p> <p style="margin-top:0;margin-bottom:0">every time, the driver update hardware feature state, also should be update software bitmap to sync it.</p> <p style="margin-top:0;margin-bottom:0"><br> </p> <p style="margin-top:0;margin-bottom:0">if only update bitmap and don't set firmware feature enabled, the smu_feature_is_enabeld is maybe not work correctly.</p> <p style="margin-top:0;margin-bottom:0"><br> </p> <p style="margin-top:0;margin-bottom:0">in your previous patch:</p> <p style="margin-top:0;margin-bottom:0"><span style="color: rgb(33, 33, 33); font-family: Calibri, sans-serif, serif, EmojiFont; font-size: 14.6667px;">drm/amd/powerplay: correct Navi10 VCN powergate control.</span></p> <p style="margin-top:0;margin-bottom:0">after your patch, </p> <p style="margin-top:0;margin-bottom:0">the function smu_feature_is_enabled(smu, SMU_FEATURE_VCN_PG_BIT)</p> <p style="margin-top:0;margin-bottom:0">will retrun true, but in the firmware, this feature maybe is not enabled.</p> <p style="margin-top:0;margin-bottom:0"><br> </p> <p style="margin-top:0;margin-bottom:0">so the function smu_feature_is_enabled is not work well.</p> <p style="margin-top:0;margin-bottom:0"><br> </p> <p style="margin-top:0;margin-bottom:0">and smu_feature_is_supported is full software feature, this is helper function to set allowed feature mask when smu power on,</p> <p style="margin-top:0;margin-bottom:0">but this function is not used in smu driver, <span style="font-size: 12pt;">i want to remove them long long ago.</span></p> <p style="margin-top:0;margin-bottom:0"><br> </p> <p style="margin-top:0;margin-bottom:0">Best Regards,<br> Kevin</p> <p style="margin-top:0;margin-bottom:0"><span style="font-size: 12pt;"></span><span style="font-size: 12pt;"></span></p> <div><br> </div> <p></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 3:17:55 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>  </style> <div lang="EN-US" link="blue" vlink="purple"> <div class="x_WordSection1"> <p class="x_MsoNormal">I cannot get your point. What do you mean “pairs of functions”?</p> <p class="x_MsoNormal">Yes, this patch does not bring real changes.</p> <p class="x_MsoNormal">But this helps for future maintain and fit common logic.</p> <p class="x_MsoNormal">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_MsoNormal">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_MsoNormal">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_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 3: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: correct confusing naming for smu_feature_set_enabled</p> </div> </div> <p class="x_MsoNormal"> </p> <div id="x_divtagdefaultwrapper"> <p class="x_MsoNormal"><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_MsoNormal"><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_MsoNormal"><span style="font-size:12.0pt; color:black"> </span></p> </div> <div> <p class="x_MsoNormal"><span style="font-size:12.0pt; color:black">before your patch:</span></p> </div> <div> <p class="x_MsoNormal"><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_MsoNormal"><span style="font-size:12.0pt; color:black">after your patch:</span></p> </div> <div> <p class="x_MsoNormal"><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_MsoNormal"><span style="font-size:12.0pt; color:black"> </span></p> </div> <div> <p class="x_MsoNormal"><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_MsoNormal"><span style="font-size:12.0pt; color:black">do you agree it?</span></p> </div> <div> <p class="x_MsoNormal"><span style="font-size:12.0pt; color:black"> </span></p> </div> <div> <p class="x_MsoNormal"><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_MsoNormal"><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_MsoNormal"><span style="font-size:12.0pt; color:black"> </span></p> </div> <div> <div> <p class="x_MsoNormal"><span style="font-size:9.0pt; color:black">extern int smu_feature_init_dpm(struct smu_context *smu);</span><span style="font-size:12.0pt; color:black"></span></p> </div> <div> <p class="x_MsoNormal"><span style="font-size:9.0pt; color:black">extern int smu_feature_is_enabled(struct smu_context *smu,</span><span style="font-size:12.0pt; color:black"></span></p> </div> <div> <p class="x_MsoNormal"><span style="font-size:9.0pt; color:black"> enum smu_feature_mask mask);</span><span style="font-size:12.0pt; color:black"></span></p> </div> <div> <p class="x_MsoNormal"><span style="font-size:9.0pt; color:black">extern int smu_feature_set_enabled(struct smu_context *smu,</span><span style="font-size:12.0pt; color:black"></span></p> </div> <div> <p class="x_MsoNormal"><span style="font-size:9.0pt; color:black"> enum smu_feature_mask mask, bool enable);</span><span style="font-size:12.0pt; color:black"></span></p> </div> <div> <p class="x_MsoNormal"><span style="font-size:9.0pt; color:black">extern int smu_feature_is_supported(struct smu_context *smu,</span><span style="font-size:12.0pt; color:black"></span></p> </div> <div> <p class="x_MsoNormal"><span style="font-size:9.0pt; color:black"> enum smu_feature_mask mask);</span><span style="font-size:12.0pt; color:black"></span></p> </div> <div> <p class="x_MsoNormal"><span style="font-size:9.0pt; color:black">extern int smu_feature_set_supported(struct smu_context *smu,</span><span style="font-size:12.0pt; color:black"></span></p> </div> <div> <p class="x_MsoNormal"><span style="font-size:9.0pt; color:black"> enum smu_feature_mask mask, bool enable);</span><span style="font-size:12.0pt; color:black"></span></p> </div> <p class="x_MsoNormal"><span style="font-size:12.0pt; color:black"> </span></p> </div> <div> <p class="x_MsoNormal"><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_divRplyFwdMsg"> <p class="x_MsoNormal"><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_MsoNormal"> </p> </div> </div> <div> <div> <p class="x_MsoNormal">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> </body> </html>