<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">smu_feature_update_enable_state() is used only in amdgpu_smu.c. <o:p></o:p></p> <p class="MsoNormal">As a common sense, these APIs should be declared as ‘static’.<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> Friday, August 23, 2019 1:30 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"> Quan, Evan <<a href="mailto:Evan.Quan@amd.com">Evan.Quan@amd.com</a>><br> <b>Sent:</b> Friday, August 23, 2019 12:50 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: 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="xmsonormal">Comment inline<o:p></o:p></p> <p class="xmsonormal"> <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> Thursday, August 22, 2019 8: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: update cached feature enablement status V2<o:p></o:p></p> </div> </div> <p class="xmsonormal"> <o:p></o:p></p> <div> <p class="xmsonormal"><span style="font-size:12.0pt;color:black">comment inline.</span><o:p></o:p></p> </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 <<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="xmsonormal"> <o:p></o:p></p> </div> </div> <div> <div> <p class="xmsonormal">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="xmsonormal"><b>[kevin]: this information is not neccessary for public, please remove it.</b><o:p></o:p></p> </div> <div> <p class="xmsonormal"><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> +<b>static</b> 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="xmsonormal"> <o:p></o:p></p> </div> <div> <p class="xmsonormal">//[kevin]: the code logic is a little redundant.<o:p></o:p></p> </div> <div> <p class="xmsonormal">could you use bellow macro to replace that?<o:p></o:p></p> </div> <div> <p class="xmsonormal">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="xmsonormal"> * bitmap_and(dst, src1, src2, nbits) *dst = *src1 & *src2<o:p></o:p></p> <div> <p class="xmsonormal"> * <b>bitmap_or</b>(dst, src1, src2, nbits) *dst = *src1 | *src2<o:p></o:p></p> </div> <div> <p class="xmsonormal"> * bitmap_xor(dst, src1, src2, nbits) *dst = *src1 ^ *src2<o:p></o:p></p> </div> <p class="xmsonormal"> * <b>bitmap_andnot</b>(dst, src1, src2, nbits) *dst = *src1 & ~(*src2)<o:p></o:p></p> </div> <div> <p class="xmsonormal"> <o:p></o:p></p> </div> <div> <p class="xmsonormal">+ mutex_unlock(&feature->mutex);<br> +<br> + return ret;<br> +}<br> +<o:p></o:p></p> </div> <div> <p class="xmsonormal"> <o:p></o:p></p> <p class="xmsonormal"><b><i>[Quan, Evan] updated in v3.</i></b><o:p></o:p></p> <p class="xmsonormal"><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="xmsonormal"> <o:p></o:p></p> </div> <div> <p class="xmsonormal">[kevin]:<o:p></o:p></p> </div> <div> <p class="xmsonormal">in this patch, i know you only want to fix not cached feature cache issue,<o:p></o:p></p> </div> <div> <p class="xmsonormal">but in v2 patch,<o:p></o:p></p> </div> <div> <p class="xmsonormal">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="xmsonormal">I don't think it is necessary,<o:p></o:p></p> </div> <div> <p class="xmsonormal">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="xmsonormal">thanks.<o:p></o:p></p> <div> <p class="xmsonormal"> <o:p></o:p></p> <p class="xmsonormal"><b><i>[Quan, Evan] Move the API before the place it’s called. No problem here.</i></b><o:p></o:p></p> <p class="xmsonormal"><o:p> </o:p></p> <p class="xmsonormal"><b><i>[kevin]: in this patch, you don't need to adjust function order in this file,</i></b><o:p></o:p></p> <p class="xmsonormal"><b><i>because the driver is already export this function in amdgpu_smu.h.</i></b><o:p></o:p></p> <p class="xmsonormal"><b><i>int smu_feature_update_enable_state(struct smu_context *smu, uint64_t feature_mask, bool enabled);</i></b><o:p></o:p></p> <p class="xmsonormal"><b><i>and in this patch, it make it is static function, but the declaration section remains in the amdgpu_smu.h file.</i></b><o:p></o:p></p> <p class="xmsonormal"><b><i>so i don't want to you adjust the function order and make it is static funtion in this patch.</i></b><o:p></o:p></p> <p class="xmsonormal"><b><i>except you have other reason for it.<o:p></o:p></i></b></p> <div> <p class="MsoNormal"><b><i><o:p> </o:p></i></b></p> </div> </div> <p class="xmsonormal">-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> </div> </div> </div> </body> </html>