<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<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);">
Does the older powerplay code need a similar fix?</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<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> amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Wang, Kevin(Yang) <Kevin1.Wang@amd.com><br>
<b>Sent:</b> Thursday, September 26, 2019 9:06 AM<br>
<b>To:</b> Quan, Evan <Evan.Quan@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 type="text/css" style="display:none">
<!--
p
{margin-top:0;
margin-bottom:0}
-->
</style>
<div dir="ltr">
<div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)">
sure, you are right, the origin design should be add this lock into these functions,</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)">
but only add these functions can't fix this issue.</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)">
eg:</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)">
<span style="margin:0px; font-size:12pt; font-family:Calibri,Arial,Helvetica,sans-serif; background-color:rgb(255,255,255)"><i>"watch -n 0 /sys/kenel/debug/dri0/amdgpu_pm_info"</i></span><br>
</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)">
<span style="margin:0px; font-size:12pt; font-family:Calibri,Arial,Helvetica,sans-serif; background-color:rgb(255,255,255)"><i>16 threads run this command will cause smu error.</i></span></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)">
so this is workaound fix about sensor interface.<br>
</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)">
<span>in fact, the smu driver need more lock to protect smu resource.</span></div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)">
<span><span>but too many locks can easily lead to deadlocks in smu driver.<br>
<span>solve the problem temporarily first and optimize this part later</span><br>
</span></span></div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)">
<ol>
<li>Message + Param ==> message param lock</li><li>Message + Message Result ==> message result lock</li><li>Message1 + Message2 ==> message pair lock (eg: SetFeatureLow and SetFeatureHigh)</li></ol>
<div><br>
</div>
<div>Best Regars,<br>
Kevin</div>
</div>
<div id="x_appendonsend"></div>
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="x_divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>From:</b> Quan, Evan <Evan.Quan@amd.com><br>
<b>Sent:</b> Thursday, September 26, 2019 8:22 PM<br>
<b>To:</b> 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>; Wang, Kevin(Yang) <Kevin1.Wang@amd.com><br>
<b>Subject:</b> RE: [PATCH 2/2] drm/amd/powerplay: add sensor lock support for smu</font>
<div> </div>
</div>
<div class="x_BodyFragment"><font size="2"><span style="font-size:11pt">
<div class="x_PlainText">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 <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Wang, Kevin(Yang)<br>
Sent: Thursday, September 26, 2019 4:56 PM<br>
To: amd-gfx@lists.freedesktop.org<br>
Cc: Huang, Ray <Ray.Huang@amd.com>; Feng, Kenneth <Kenneth.Feng@amd.com>; Wang, Kevin(Yang) <Kevin1.Wang@amd.com><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 <kevin1.wang@amd.com><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>
amd-gfx@lists.freedesktop.org<br>
<a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a><br>
</div>
</span></font></div>
</div>
</body>
</html>