<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">There should be no issue with old powerplay routines. <o:p></o:p></p> <p class="MsoNormal">As there is API lock(hwmgr->smu_lock) in amd_powerplay.c.<o:p></o:p></p> <p class="MsoNormal">It’s quite coarse-grained but working.<o:p></o:p></p> <p class="MsoNormal"><o:p> </o:p></p> <p class="MsoNormal">In fact, I’m considering whether we need to take the same way in swSMU routine.<o:p></o:p></p> <p class="MsoNormal">Since for now it’s hard to define what we are protecting for and thus find a better fine-grained mutex.<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> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> <p class="MsoNormal"><b>From:</b> Deucher, Alexander <Alexander.Deucher@amd.com> <br> <b>Sent:</b> Thursday, September 26, 2019 11:06 PM<br> <b>To:</b> Wang, Kevin(Yang) <Kevin1.Wang@amd.com>; Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org<br> <b>Cc:</b> Huang, Ray <Ray.Huang@amd.com>; Feng, Kenneth <Kenneth.Feng@amd.com><br> <b>Subject:</b> Re: [PATCH 2/2] drm/amd/powerplay: add sensor lock support for smu<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">Does the older powerplay code need a similar fix?<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="font-size:12.0pt;color:black"><o:p> </o:p></span></p> </div> <div> <p class="MsoNormal"><span style="font-size:12.0pt;color:black">Alex<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 Wang, Kevin(Yang) <<a href="mailto:Kevin1.Wang@amd.com">Kevin1.Wang@amd.com</a>><br> <b>Sent:</b> Thursday, September 26, 2019 9:06 AM<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> <<a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a>><br> <b>Cc:</b> Huang, Ray <<a href="mailto:Ray.Huang@amd.com">Ray.Huang@amd.com</a>>; Feng, Kenneth <<a href="mailto:Kenneth.Feng@amd.com">Kenneth.Feng@amd.com</a>><br> <b>Subject:</b> Re: [PATCH 2/2] drm/amd/powerplay: add sensor lock support for smu</span> <o:p></o:p></p> <div> <p class="MsoNormal"> <o:p></o:p></p> </div> </div> <div> <div> <p class="MsoNormal"><span style="font-size:12.0pt;color:black">sure, you are right, the origin design should be add this lock into these functions,<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="font-size:12.0pt;color:black">but only add these functions can't fix this issue.<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="font-size:12.0pt;color:black">eg:<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><i><span style="font-size:12.0pt;color:black;background:white">"watch -n 0 /sys/kenel/debug/dri0/amdgpu_pm_info"</span></i><span style="font-size:12.0pt;color:black"><o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><i><span style="font-size:12.0pt;color:black;background:white">16 threads run this command will cause smu error.</span></i><span style="font-size:12.0pt;color:black"><o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="font-size:12.0pt;color:black"><o:p> </o:p></span></p> </div> <div> <p class="MsoNormal"><span style="font-size:12.0pt;color:black">so this is workaound fix about sensor interface.<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="font-size:12.0pt;color:black">in fact, the smu driver need more lock to protect smu resource.<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><span style="font-size:12.0pt;color:black">but too many locks can easily lead to deadlocks in smu driver.<br> solve the problem temporarily first and optimize this part later<o:p></o:p></span></p> </div> <div> <ol start="1" type="1"> <li class="MsoNormal" style="color:black;mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level1 lfo1"> <span style="font-size:12.0pt">Message + Param ==> message param lock<o:p></o:p></span></li><li class="MsoNormal" style="color:black;mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level1 lfo1"> <span style="font-size:12.0pt">Message + Message Result ==> message result lock<o:p></o:p></span></li><li class="MsoNormal" style="color:black;mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0 level1 lfo1"> <span style="font-size:12.0pt">Message1 + Message2 ==> message pair lock (eg: SetFeatureLow and SetFeatureHigh)<o:p></o:p></span></li></ol> <div> <p class="MsoNormal"><span style="font-size:12.0pt;color:black"><o:p> </o:p></span></p> </div> <div> <p class="MsoNormal"><span style="font-size:12.0pt;color:black">Best Regars,<br> Kevin<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="x_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> Thursday, September 26, 2019 8:22 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>Cc:</b> Huang, Ray <<a href="mailto:Ray.Huang@amd.com">Ray.Huang@amd.com</a>>; Feng, Kenneth <<a href="mailto:Kenneth.Feng@amd.com">Kenneth.Feng@amd.com</a>>; Wang, Kevin(Yang) <<a href="mailto:Kevin1.Wang@amd.com">Kevin1.Wang@amd.com</a>><br> <b>Subject:</b> RE: [PATCH 2/2] drm/amd/powerplay: add sensor lock support for smu</span> <o:p></o:p></p> <div> <p class="MsoNormal"> <o:p></o:p></p> </div> </div> <div> <div> <p class="MsoNormal">How about adding the mutex protection in smu_v11_0_send_msg_with_param and smu_v11_0_send_msg?<br> That seems able to simplify the code.<br> <br> -----Original Message-----<br> From: amd-gfx <<a href="mailto:amd-gfx-bounces@lists.freedesktop.org">amd-gfx-bounces@lists.freedesktop.org</a>> On Behalf Of Wang, Kevin(Yang)<br> Sent: Thursday, September 26, 2019 4:56 PM<br> To: <a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a><br> Cc: Huang, Ray <<a href="mailto:Ray.Huang@amd.com">Ray.Huang@amd.com</a>>; Feng, Kenneth <<a href="mailto:Kenneth.Feng@amd.com">Kenneth.Feng@amd.com</a>>; Wang, Kevin(Yang) <<a href="mailto:Kevin1.Wang@amd.com">Kevin1.Wang@amd.com</a>><br> Subject: [PATCH 2/2] drm/amd/powerplay: add sensor lock support for smu<br> <br> when multithreading access sysfs of amdgpu_pm_info at the sametime.<br> the swsmu driver cause smu firmware hang.<br> <br> eg:<br> single thread access:<br> Message A + Param A ==> right<br> Message B + Param B ==> right<br> Message C + Param C ==> right<br> multithreading access:<br> Message A + Param B ==> error<br> Message B + Param A ==> error<br> Message C + Param C ==> right<br> <br> the patch will add sensor lock(mutex) to avoid this error.<br> <br> Signed-off-by: Kevin Wang <<a href="mailto:kevin1.wang@amd.com">kevin1.wang@amd.com</a>><br> ---<br> drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 2 ++<br> drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 2 ++<br> drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 1 +<br> drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 2 ++<br> drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 2 ++<br> 5 files changed, 9 insertions(+)<br> <br> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c<br> index 23293e15636b..df510cb86da5 100644<br> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c<br> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c<br> @@ -840,6 +840,8 @@ static int smu_sw_init(void *handle)<br> smu->smu_baco.state = SMU_BACO_STATE_EXIT;<br> smu->smu_baco.platform_support = false;<br> <br> + mutex_init(&smu->sensor_lock);<br> +<br> smu->watermarks_bitmap = 0;<br> smu->power_profile_mode = PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;<br> smu->default_power_profile_mode = PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;<br> diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c<br> index a047a7ec3698..b9b7b73354a0 100644<br> --- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c<br> +++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c<br> @@ -1025,6 +1025,7 @@ static int arcturus_read_sensor(struct smu_context *smu,<br> if (!data || !size)<br> return -EINVAL;<br> <br> + mutex_lock(&smu->sensor_lock);<br> switch (sensor) {<br> case AMDGPU_PP_SENSOR_MAX_FAN_RPM:<br> *(uint32_t *)data = pptable->FanMaximumRpm; @@ -1051,6 +1052,7 @@ static int arcturus_read_sensor(struct smu_context *smu,<br> default:<br> ret = smu_smc_read_sensor(smu, sensor, data, size);<br> }<br> + mutex_unlock(&smu->sensor_lock);<br> <br> return ret;<br> }<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 5c898444ff97..bc4b73e0718e 100644<br> --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h<br> +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h<br> @@ -350,6 +350,7 @@ struct smu_context<br> const struct smu_funcs *funcs;<br> const struct pptable_funcs *ppt_funcs;<br> struct mutex mutex;<br> + struct mutex sensor_lock;<br> uint64_t pool_size;<br> <br> struct smu_table_context smu_table;<br> diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c<br> index db2e181ba45a..c0b640d8d9e1 100644<br> --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c<br> +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c<br> @@ -1387,6 +1387,7 @@ static int navi10_read_sensor(struct smu_context *smu,<br> if(!data || !size)<br> return -EINVAL;<br> <br> + mutex_lock(&smu->sensor_lock);<br> switch (sensor) {<br> case AMDGPU_PP_SENSOR_MAX_FAN_RPM:<br> *(uint32_t *)data = pptable->FanMaximumRpm; @@ -1410,6 +1411,7 @@ static int navi10_read_sensor(struct smu_context *smu,<br> default:<br> ret = smu_smc_read_sensor(smu, sensor, data, size);<br> }<br> + mutex_unlock(&smu->sensor_lock);<br> <br> return ret;<br> }<br> diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c<br> index 9082da1578d1..f655ebd9ba22 100644<br> --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c<br> +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c<br> @@ -3017,6 +3017,7 @@ static int vega20_read_sensor(struct smu_context *smu,<br> if(!data || !size)<br> return -EINVAL;<br> <br> + mutex_lock(&smu->sensor_lock);<br> switch (sensor) {<br> case AMDGPU_PP_SENSOR_MAX_FAN_RPM:<br> *(uint32_t *)data = pptable->FanMaximumRpm; @@ -3042,6 +3043,7 @@ static int vega20_read_sensor(struct smu_context *smu,<br> default:<br> ret = smu_smc_read_sensor(smu, sensor, data, size);<br> }<br> + mutex_unlock(&smu->sensor_lock);<br> <br> return ret;<br> }<br> --<br> 2.17.1<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>