<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 style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> That might be easier for swSMU as well since this is generally not performance sensitive.</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <br> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Alex</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <br> </div> <div id="appendonsend"></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> Thursday, September 26, 2019 9:18 PM<br> <b>To:</b> Deucher, Alexander <Alexander.Deucher@amd.com>; Wang, Kevin(Yang) <Kevin1.Wang@amd.com>; amd-gfx@lists.freedesktop.org <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</font> <div> </div> </div> <style>  </style> <div lang="EN-US" link="blue" vlink="purple"> <div class="x_WordSection1"> <p class="x_MsoNormal">There should be no issue with old powerplay routines. </p> <p class="x_MsoNormal">As there is API lock(hwmgr->smu_lock) in amd_powerplay.c.</p> <p class="x_MsoNormal">It’s quite coarse-grained but working.</p> <p class="x_MsoNormal"> </p> <p class="x_MsoNormal">In fact, I’m considering whether we need to take the same way in swSMU routine.</p> <p class="x_MsoNormal">Since for now it’s hard to define what we are protecting for and thus find a better fine-grained mutex.</p> <p class="x_MsoNormal"> </p> <p class="x_MsoNormal">Regards,</p> <p class="x_MsoNormal">Evan</p> <div> <div style="border:none; border-top:solid #E1E1E1 1.0pt; padding:3.0pt 0in 0in 0in"> <p class="x_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</p> </div> </div> <p class="x_MsoNormal"> </p> <div> <p class="x_MsoNormal"><span style="font-size:12.0pt; color:black">Does the older powerplay code need a similar fix?</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">Alex</span></p> </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 <<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> </p> <div> <p class="x_MsoNormal"> </p> </div> </div> <div> <div> <p class="x_MsoNormal"><span style="font-size:12.0pt; color:black">sure, you are right, the origin design should be add this lock into these functions,</span></p> </div> <div> <p class="x_MsoNormal"><span style="font-size:12.0pt; color:black">but only add these functions can't fix this issue.</span></p> </div> <div> <p class="x_MsoNormal"><span style="font-size:12.0pt; color:black">eg:</span></p> </div> <div> <p class="x_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"></span></p> </div> <div> <p class="x_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"></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 this is workaound fix about sensor interface.</span></p> </div> <div> <p class="x_MsoNormal"><span style="font-size:12.0pt; color:black">in fact, the smu driver need more lock to protect smu resource.</span></p> </div> <div> <p class="x_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</span></p> </div> <div> <ol start="1" type="1"> <li class="x_MsoNormal" style="color:black"><span style="font-size:12.0pt">Message + Param ==> message param lock</span></li><li class="x_MsoNormal" style="color:black"><span style="font-size:12.0pt">Message + Message Result ==> message result lock</span></li><li class="x_MsoNormal" style="color:black"><span style="font-size:12.0pt">Message1 + Message2 ==> message pair lock (eg: SetFeatureLow and SetFeatureHigh)</span></li></ol> <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 Regars,<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_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> </p> <div> <p class="x_MsoNormal"> </p> </div> </div> <div> <div> <p class="x_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></p> </div> </div> </div> </div> </div> </body> </html>