Re: [PATCH] drm/amdgpu/pm: bail on sysfs/debugfs queries during platform suspend

2021-03-25 Thread Christian König 

Am 25.03.21 um 04:29 schrieb Quan, Evan:

[AMD Public Use]

Maybe we can have an API like is_hw_access_blocked(). So that we can put all 
those checks below within it.
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;


Sounds like a good idea to me as well.

But my question is how the heck have we managed to access those files 
during suspend?



Anyway, patch is reviewed-by: Evan Quan 


Acked-by: Christian König 



-Original Message-
From: amd-gfx  On Behalf Of Alex Deucher
Sent: Thursday, March 25, 2021 5:18 AM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander 
Subject: [PATCH] drm/amdgpu/pm: bail on sysfs/debugfs queries during platform 
suspend

The GPU is in the process of being shutdown.  Spurious queries during
suspend and resume can put the SMU into a bad state.  Runtime PM is
handled dynamically so we check if we are in non-runtime suspend.

Signed-off-by: Alex Deucher 
---
  drivers/gpu/drm/amd/pm/amdgpu_pm.c | 98 ++
  1 file changed, 98 insertions(+)

diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c 
b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
index 2627870a786e..3c1b5483688b 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
@@ -129,6 +129,8 @@ static ssize_t amdgpu_get_power_dpm_state(struct device 
*dev,
  
  	if (amdgpu_in_reset(adev))

return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
  
  	ret = pm_runtime_get_sync(ddev->dev);

if (ret < 0) {
@@ -162,6 +164,8 @@ static ssize_t amdgpu_set_power_dpm_state(struct device 
*dev,
  
  	if (amdgpu_in_reset(adev))

return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
  
  	if (strncmp("battery", buf, strlen("battery")) == 0)

state = POWER_STATE_TYPE_BATTERY;
@@ -268,6 +272,8 @@ static ssize_t 
amdgpu_get_power_dpm_force_performance_level(struct device *dev,
  
  	if (amdgpu_in_reset(adev))

return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
  
  	ret = pm_runtime_get_sync(ddev->dev);

if (ret < 0) {
@@ -310,6 +316,8 @@ static ssize_t 
amdgpu_set_power_dpm_force_performance_level(struct device *dev,
  
  	if (amdgpu_in_reset(adev))

return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
  
  	if (strncmp("low", buf, strlen("low")) == 0) {

level = AMD_DPM_FORCED_LEVEL_LOW;
@@ -408,6 +416,8 @@ static ssize_t amdgpu_get_pp_num_states(struct device *dev,
  
  	if (amdgpu_in_reset(adev))

return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
  
  	ret = pm_runtime_get_sync(ddev->dev);

if (ret < 0) {
@@ -448,6 +458,8 @@ static ssize_t amdgpu_get_pp_cur_state(struct device *dev,
  
  	if (amdgpu_in_reset(adev))

return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
  
  	ret = pm_runtime_get_sync(ddev->dev);

if (ret < 0) {
@@ -484,6 +496,8 @@ static ssize_t amdgpu_get_pp_force_state(struct device *dev,
  
  	if (amdgpu_in_reset(adev))

return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
  
  	if (adev->pp_force_state_enabled)

return amdgpu_get_pp_cur_state(dev, attr, buf);
@@ -504,6 +518,8 @@ static ssize_t amdgpu_set_pp_force_state(struct device *dev,
  
  	if (amdgpu_in_reset(adev))

return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
  
  	if (strlen(buf) == 1)

adev->pp_force_state_enabled = false;
@@ -564,6 +580,8 @@ static ssize_t amdgpu_get_pp_table(struct device *dev,
  
  	if (amdgpu_in_reset(adev))

return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
  
  	ret = pm_runtime_get_sync(ddev->dev);

if (ret < 0) {
@@ -602,6 +620,8 @@ static ssize_t amdgpu_set_pp_table(struct device *dev,
  
  	if (amdgpu_in_reset(adev))

return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
  
  	ret = pm_runtime_get_sync(ddev->dev);

if (ret < 0) {
@@ -764,6 +784,8 @@ static ssize_t amdgpu_set_pp_od_clk_voltage(struct device 
*dev,
  
  	if (amdgpu_in_reset(adev))

return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
  
  	if (count > 127)

return -EINVAL;
@@ -865,6 +887,8 @@ static ssize

RE: [PATCH] drm/amdgpu/pm: bail on sysfs/debugfs queries during platform suspend

2021-03-24 Thread Quan, Evan 
[AMD Public Use]

Maybe we can have an API like is_hw_access_blocked(). So that we can put all 
those checks below within it.
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
Anyway, patch is reviewed-by: Evan Quan 

-Original Message-
From: amd-gfx  On Behalf Of Alex Deucher
Sent: Thursday, March 25, 2021 5:18 AM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander 
Subject: [PATCH] drm/amdgpu/pm: bail on sysfs/debugfs queries during platform 
suspend

The GPU is in the process of being shutdown.  Spurious queries during
suspend and resume can put the SMU into a bad state.  Runtime PM is
handled dynamically so we check if we are in non-runtime suspend.

Signed-off-by: Alex Deucher 
---
 drivers/gpu/drm/amd/pm/amdgpu_pm.c | 98 ++
 1 file changed, 98 insertions(+)

diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c 
b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
index 2627870a786e..3c1b5483688b 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
@@ -129,6 +129,8 @@ static ssize_t amdgpu_get_power_dpm_state(struct device 
*dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
ret = pm_runtime_get_sync(ddev->dev);
if (ret < 0) {
@@ -162,6 +164,8 @@ static ssize_t amdgpu_set_power_dpm_state(struct device 
*dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
if (strncmp("battery", buf, strlen("battery")) == 0)
state = POWER_STATE_TYPE_BATTERY;
@@ -268,6 +272,8 @@ static ssize_t 
amdgpu_get_power_dpm_force_performance_level(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
ret = pm_runtime_get_sync(ddev->dev);
if (ret < 0) {
@@ -310,6 +316,8 @@ static ssize_t 
amdgpu_set_power_dpm_force_performance_level(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
if (strncmp("low", buf, strlen("low")) == 0) {
level = AMD_DPM_FORCED_LEVEL_LOW;
@@ -408,6 +416,8 @@ static ssize_t amdgpu_get_pp_num_states(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
ret = pm_runtime_get_sync(ddev->dev);
if (ret < 0) {
@@ -448,6 +458,8 @@ static ssize_t amdgpu_get_pp_cur_state(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
ret = pm_runtime_get_sync(ddev->dev);
if (ret < 0) {
@@ -484,6 +496,8 @@ static ssize_t amdgpu_get_pp_force_state(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
if (adev->pp_force_state_enabled)
return amdgpu_get_pp_cur_state(dev, attr, buf);
@@ -504,6 +518,8 @@ static ssize_t amdgpu_set_pp_force_state(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
if (strlen(buf) == 1)
adev->pp_force_state_enabled = false;
@@ -564,6 +580,8 @@ static ssize_t amdgpu_get_pp_table(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
ret = pm_runtime_get_sync(ddev->dev);
if (ret < 0) {
@@ -602,6 +620,8 @@ static ssize_t amdgpu_set_pp_table(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
ret = pm_runtime_get_sync(ddev->dev);
if (ret < 0) {
@@ -764,6 +784,8 @@ static ssize_t amdgpu_set_pp_od_clk_voltage(struct device 
*dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
if (count > 127)
return -EINVAL;
@@ -865,6 +887,8 @@ static ssize_t amdgpu_get_pp_od_clk_voltage(struct device 
*dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if

[PATCH] drm/amdgpu/pm: bail on sysfs/debugfs queries during platform suspend

2021-03-24 Thread Alex Deucher 
The GPU is in the process of being shutdown.  Spurious queries during
suspend and resume can put the SMU into a bad state.  Runtime PM is
handled dynamically so we check if we are in non-runtime suspend.

Signed-off-by: Alex Deucher 
---
 drivers/gpu/drm/amd/pm/amdgpu_pm.c | 98 ++
 1 file changed, 98 insertions(+)

diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c 
b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
index 2627870a786e..3c1b5483688b 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
@@ -129,6 +129,8 @@ static ssize_t amdgpu_get_power_dpm_state(struct device 
*dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
ret = pm_runtime_get_sync(ddev->dev);
if (ret < 0) {
@@ -162,6 +164,8 @@ static ssize_t amdgpu_set_power_dpm_state(struct device 
*dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
if (strncmp("battery", buf, strlen("battery")) == 0)
state = POWER_STATE_TYPE_BATTERY;
@@ -268,6 +272,8 @@ static ssize_t 
amdgpu_get_power_dpm_force_performance_level(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
ret = pm_runtime_get_sync(ddev->dev);
if (ret < 0) {
@@ -310,6 +316,8 @@ static ssize_t 
amdgpu_set_power_dpm_force_performance_level(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
if (strncmp("low", buf, strlen("low")) == 0) {
level = AMD_DPM_FORCED_LEVEL_LOW;
@@ -408,6 +416,8 @@ static ssize_t amdgpu_get_pp_num_states(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
ret = pm_runtime_get_sync(ddev->dev);
if (ret < 0) {
@@ -448,6 +458,8 @@ static ssize_t amdgpu_get_pp_cur_state(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
ret = pm_runtime_get_sync(ddev->dev);
if (ret < 0) {
@@ -484,6 +496,8 @@ static ssize_t amdgpu_get_pp_force_state(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
if (adev->pp_force_state_enabled)
return amdgpu_get_pp_cur_state(dev, attr, buf);
@@ -504,6 +518,8 @@ static ssize_t amdgpu_set_pp_force_state(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
if (strlen(buf) == 1)
adev->pp_force_state_enabled = false;
@@ -564,6 +580,8 @@ static ssize_t amdgpu_get_pp_table(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
ret = pm_runtime_get_sync(ddev->dev);
if (ret < 0) {
@@ -602,6 +620,8 @@ static ssize_t amdgpu_set_pp_table(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
ret = pm_runtime_get_sync(ddev->dev);
if (ret < 0) {
@@ -764,6 +784,8 @@ static ssize_t amdgpu_set_pp_od_clk_voltage(struct device 
*dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
if (count > 127)
return -EINVAL;
@@ -865,6 +887,8 @@ static ssize_t amdgpu_get_pp_od_clk_voltage(struct device 
*dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
ret = pm_runtime_get_sync(ddev->dev);
if (ret < 0) {
@@ -916,6 +940,8 @@ static ssize_t amdgpu_set_pp_features(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
ret = kstrtou64(buf, 0, &featuremask);
if (ret)
@@ -959,6 +985,8 @@ static ssize_t amdgpu_get_pp_features(struct device *dev,
 
if (amdgpu_in_reset(adev))
return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
 
ret = pm_runtime_get_sync(ddev->dev);
if (ret < 0) {
@@ -1018,6 +1046,8 @@ static ssize_t amdgpu_get_pp_dpm_clock(struct device *dev,
 
if (amdgpu_