[PATCH 2/2] drm/amd/powerplay: add sensor lock support for smu

Deucher, Alexander Alexander.Deucher at amd.com
Fri Sep 27 14:24:55 UTC 2019


That might be easier for swSMU as well since this is generally not performance sensitive.

Alex

________________________________
From: Quan, Evan <Evan.Quan at amd.com>
Sent: Thursday, September 26, 2019 9:18 PM
To: Deucher, Alexander <Alexander.Deucher at amd.com>; Wang, Kevin(Yang) <Kevin1.Wang at amd.com>; amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>
Cc: Huang, Ray <Ray.Huang at amd.com>; Feng, Kenneth <Kenneth.Feng at amd.com>
Subject: RE: [PATCH 2/2] drm/amd/powerplay: add sensor lock support for smu


There should be no issue with old powerplay routines.

As there is API lock(hwmgr->smu_lock) in amd_powerplay.c.

It’s quite coarse-grained but working.



In fact, I’m considering whether we need to take the same way in swSMU routine.

Since for now it’s hard to define what we are protecting for and thus find a better fine-grained mutex.



Regards,

Evan

From: Deucher, Alexander <Alexander.Deucher at amd.com>
Sent: Thursday, September 26, 2019 11:06 PM
To: Wang, Kevin(Yang) <Kevin1.Wang at amd.com>; Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Huang, Ray <Ray.Huang at amd.com>; Feng, Kenneth <Kenneth.Feng at amd.com>
Subject: Re: [PATCH 2/2] drm/amd/powerplay: add sensor lock support for smu



Does the older powerplay code need a similar fix?



Alex

________________________________

From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org<mailto:amd-gfx-bounces at lists.freedesktop.org>> on behalf of Wang, Kevin(Yang) <Kevin1.Wang at amd.com<mailto:Kevin1.Wang at amd.com>>
Sent: Thursday, September 26, 2019 9:06 AM
To: Quan, Evan <Evan.Quan at amd.com<mailto:Evan.Quan at amd.com>>; amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org> <amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>>
Cc: Huang, Ray <Ray.Huang at amd.com<mailto:Ray.Huang at amd.com>>; Feng, Kenneth <Kenneth.Feng at amd.com<mailto:Kenneth.Feng at amd.com>>
Subject: Re: [PATCH 2/2] drm/amd/powerplay: add sensor lock support for smu



sure, you are right, the origin design should be add this lock into these functions,

but only add these functions can't fix this issue.

eg:

"watch -n 0 /sys/kenel/debug/dri0/amdgpu_pm_info"

16 threads run this command will cause smu error.



so this is workaound fix about sensor interface.

in fact, the smu driver need more lock to protect smu resource.

but too many locks can easily lead to deadlocks in smu driver.
solve the problem temporarily first and optimize this part later

  1.  Message + Param ==> message param lock
  2.  Message + Message Result ==> message result lock
  3.  Message1 + Message2 ==> message pair lock (eg: SetFeatureLow and SetFeatureHigh)



Best Regars,
Kevin

________________________________

From: Quan, Evan <Evan.Quan at amd.com<mailto:Evan.Quan at amd.com>>
Sent: Thursday, September 26, 2019 8:22 PM
To: Wang, Kevin(Yang) <Kevin1.Wang at amd.com<mailto:Kevin1.Wang at amd.com>>; amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org> <amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>>
Cc: Huang, Ray <Ray.Huang at amd.com<mailto:Ray.Huang at amd.com>>; Feng, Kenneth <Kenneth.Feng at amd.com<mailto:Kenneth.Feng at amd.com>>; Wang, Kevin(Yang) <Kevin1.Wang at amd.com<mailto:Kevin1.Wang at amd.com>>
Subject: RE: [PATCH 2/2] drm/amd/powerplay: add sensor lock support for smu



How about adding the mutex protection in smu_v11_0_send_msg_with_param and smu_v11_0_send_msg?
That seems able to simplify the code.

-----Original Message-----
From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org<mailto:amd-gfx-bounces at lists.freedesktop.org>> On Behalf Of Wang, Kevin(Yang)
Sent: Thursday, September 26, 2019 4:56 PM
To: amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>
Cc: Huang, Ray <Ray.Huang at amd.com<mailto:Ray.Huang at amd.com>>; Feng, Kenneth <Kenneth.Feng at amd.com<mailto:Kenneth.Feng at amd.com>>; Wang, Kevin(Yang) <Kevin1.Wang at amd.com<mailto:Kevin1.Wang at amd.com>>
Subject: [PATCH 2/2] drm/amd/powerplay: add sensor lock support for smu

when multithreading access sysfs of amdgpu_pm_info at the sametime.
the swsmu driver cause smu firmware hang.

eg:
single thread access:
Message A + Param A ==> right
Message B + Param B ==> right
Message C + Param C ==> right
multithreading access:
Message A + Param B ==> error
Message B + Param A ==> error
Message C + Param C ==> right

the patch will add sensor lock(mutex) to avoid this error.

Signed-off-by: Kevin Wang <kevin1.wang at amd.com<mailto:kevin1.wang at amd.com>>
---
 drivers/gpu/drm/amd/powerplay/amdgpu_smu.c     | 2 ++
 drivers/gpu/drm/amd/powerplay/arcturus_ppt.c   | 2 ++
 drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 1 +
 drivers/gpu/drm/amd/powerplay/navi10_ppt.c     | 2 ++
 drivers/gpu/drm/amd/powerplay/vega20_ppt.c     | 2 ++
 5 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
index 23293e15636b..df510cb86da5 100644
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
@@ -840,6 +840,8 @@ static int smu_sw_init(void *handle)
         smu->smu_baco.state = SMU_BACO_STATE_EXIT;
         smu->smu_baco.platform_support = false;

+       mutex_init(&smu->sensor_lock);
+
         smu->watermarks_bitmap = 0;
         smu->power_profile_mode = PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;
         smu->default_power_profile_mode = PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;
diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
index a047a7ec3698..b9b7b73354a0 100644
--- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
@@ -1025,6 +1025,7 @@ static int arcturus_read_sensor(struct smu_context *smu,
         if (!data || !size)
                 return -EINVAL;

+       mutex_lock(&smu->sensor_lock);
         switch (sensor) {
         case AMDGPU_PP_SENSOR_MAX_FAN_RPM:
                 *(uint32_t *)data = pptable->FanMaximumRpm; @@ -1051,6 +1052,7 @@ static int arcturus_read_sensor(struct smu_context *smu,
         default:
                 ret = smu_smc_read_sensor(smu, sensor, data, size);
         }
+       mutex_unlock(&smu->sensor_lock);

         return ret;
 }
diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
index 5c898444ff97..bc4b73e0718e 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
@@ -350,6 +350,7 @@ struct smu_context
         const struct smu_funcs          *funcs;
         const struct pptable_funcs      *ppt_funcs;
         struct mutex                    mutex;
+       struct mutex                    sensor_lock;
         uint64_t pool_size;

         struct smu_table_context        smu_table;
diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
index db2e181ba45a..c0b640d8d9e1 100644
--- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
@@ -1387,6 +1387,7 @@ static int navi10_read_sensor(struct smu_context *smu,
         if(!data || !size)
                 return -EINVAL;

+       mutex_lock(&smu->sensor_lock);
         switch (sensor) {
         case AMDGPU_PP_SENSOR_MAX_FAN_RPM:
                 *(uint32_t *)data = pptable->FanMaximumRpm; @@ -1410,6 +1411,7 @@ static int navi10_read_sensor(struct smu_context *smu,
         default:
                 ret = smu_smc_read_sensor(smu, sensor, data, size);
         }
+       mutex_unlock(&smu->sensor_lock);

         return ret;
 }
diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
index 9082da1578d1..f655ebd9ba22 100644
--- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
@@ -3017,6 +3017,7 @@ static int vega20_read_sensor(struct smu_context *smu,
         if(!data || !size)
                 return -EINVAL;

+       mutex_lock(&smu->sensor_lock);
         switch (sensor) {
         case AMDGPU_PP_SENSOR_MAX_FAN_RPM:
                 *(uint32_t *)data = pptable->FanMaximumRpm; @@ -3042,6 +3043,7 @@ static int vega20_read_sensor(struct smu_context *smu,
         default:
                 ret = smu_smc_read_sensor(smu, sensor, data, size);
         }
+       mutex_unlock(&smu->sensor_lock);

         return ret;
 }
--
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20190927/13c2b6ea/attachment-0001.html>


More information about the amd-gfx mailing list