[PATCH v2] drm/amd/pm: consistent approach for smartshift

[PATCH v2] drm/amd/pm: consistent approach for smartshift

[Sathishkumar S]

create smartshift sysfs attributes from dGPU device even
on smartshift 1.0 platform to be consistent. Do not populate
the attributes on platforms that have APU only but not dGPU
or vice versa.

V2: avoid checking for the number of VGA/DISPLAY devices (Lijo)

Suggested-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Sathishkumar S <sathishkumar.sundararaju@amd.com>
Acked-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/pm/amdgpu_pm.c | 136 ++++++++++++++++-------------
 1 file changed, 74 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
index d3228216b2da..292e8c241597 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
@@ -1734,22 +1734,11 @@ static ssize_t amdgpu_get_gpu_metrics(struct device *dev,
 	return size;
 }
 
-/**
- * DOC: smartshift_apu_power
- *
- * The amdgpu driver provides a sysfs API for reporting APU power
- * share if it supports smartshift. The value is expressed as
- * the proportion of stapm limit where stapm limit is the total APU
- * power limit. The result is in percentage. If APU power is 130% of
- * STAPM, then APU is using 30% of the dGPU's headroom.
- */
-
-static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, struct device_attribute *attr,
-					       char *buf)
+static int amdgpu_read_powershift_percent(struct amdgpu_device *adev,
+						uint32_t *ss_power, bool dgpu_share)
 {
-	struct drm_device *ddev = dev_get_drvdata(dev);
-	struct amdgpu_device *adev = drm_to_adev(ddev);
-	uint32_t ss_power, size;
+	struct drm_device *ddev = adev_to_drm(adev);
+	uint32_t size;
 	int r = 0;
 
 	if (amdgpu_in_reset(adev))
@@ -1763,28 +1752,65 @@ static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, struct device
 		return r;
 	}
 
-	r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
-				   (void *)&ss_power, &size);
-	if (r)
-		goto out;
-
-	r = sysfs_emit(buf, "%u%%\n", ss_power);
+	if (dgpu_share)
+		r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
+				   (void *)ss_power, &size);
+	else
+		r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
+				   (void *)ss_power, &size);
 
-out:
 	pm_runtime_mark_last_busy(ddev->dev);
 	pm_runtime_put_autosuspend(ddev->dev);
 	return r;
 }
+/**
+ * DOC: smartshift_apu_power
+ *
+ * The amdgpu driver provides a sysfs API for reporting APU power
+ * shift in percentage if platform supports smartshift. Value 0 means that
+ * there is no powershift and values between [1-100] means that the power
+ * is shifted to APU, the percentage of boost is with respect to APU power
+ * limit on the platform.
+ */
+
+static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, struct device_attribute *attr,
+					       char *buf)
+{
+	struct drm_device *ddev = dev_get_drvdata(dev);
+	struct amdgpu_device *adev = drm_to_adev(ddev);
+	uint32_t ss_power = 0;
+	int r = 0, i;
+
+	r = amdgpu_read_powershift_percent(adev, &ss_power, false);
+	if (r == -EOPNOTSUPP) {
+		/* sensor not available on dGPU, try to read from APU */
+		adev = NULL;
+		mutex_lock(&mgpu_info.mutex);
+		for (i = 0; i < mgpu_info.num_gpu; i++) {
+			if (mgpu_info.gpu_ins[i].adev->flags & AMD_IS_APU) {
+				adev = mgpu_info.gpu_ins[i].adev;
+				break;
+			}
+		}
+		mutex_unlock(&mgpu_info.mutex);
+		if (adev)
+			r = amdgpu_read_powershift_percent(adev, &ss_power, false);
+	}
+
+	if (!r)
+		r = sysfs_emit(buf, "%u%%\n", ss_power);
+
+	return r;
+}
 
 /**
  * DOC: smartshift_dgpu_power
  *
- * The amdgpu driver provides a sysfs API for reporting the dGPU power
- * share if the device is in HG and supports smartshift. The value
- * is expressed as the proportion of stapm limit where stapm limit
- * is the total APU power limit. The value is in percentage. If dGPU
- * power is 20% higher than STAPM power(120%), it's using 20% of the
- * APU's power headroom.
+ * The amdgpu driver provides a sysfs API for reporting dGPU power
+ * shift in percentage if platform supports smartshift. Value 0 means that
+ * there is no powershift and values between [1-100] means that the power is
+ * shifted to dGPU, the percentage of boost is with respect to dGPU power
+ * limit on the platform.
  */
 
 static ssize_t amdgpu_get_smartshift_dgpu_power(struct device *dev, struct device_attribute *attr,
@@ -1792,31 +1818,28 @@ static ssize_t amdgpu_get_smartshift_dgpu_power(struct device *dev, struct devic
 {
 	struct drm_device *ddev = dev_get_drvdata(dev);
 	struct amdgpu_device *adev = drm_to_adev(ddev);
-	uint32_t ss_power, size;
-	int r = 0;
-
-	if (amdgpu_in_reset(adev))
-		return -EPERM;
-	if (adev->in_suspend && !adev->in_runpm)
-		return -EPERM;
-
-	r = pm_runtime_get_sync(ddev->dev);
-	if (r < 0) {
-		pm_runtime_put_autosuspend(ddev->dev);
-		return r;
+	uint32_t ss_power = 0;
+	int r = 0, i;
+
+	r = amdgpu_read_powershift_percent(adev, &ss_power, true);
+	if (r == -EOPNOTSUPP) {
+		/* sensor not available on dGPU, try to read from APU */
+		adev = NULL;
+		mutex_lock(&mgpu_info.mutex);
+		for (i = 0; i < mgpu_info.num_gpu; i++) {
+			if (mgpu_info.gpu_ins[i].adev->flags & AMD_IS_APU) {
+				adev = mgpu_info.gpu_ins[i].adev;
+				break;
+			}
+		}
+		mutex_unlock(&mgpu_info.mutex);
+		if (adev)
+			r = amdgpu_read_powershift_percent(adev, &ss_power, true);
 	}
 
-	r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
-				   (void *)&ss_power, &size);
-
-	if (r)
-		goto out;
-
-	r = sysfs_emit(buf, "%u%%\n", ss_power);
+	if (!r)
+		r = sysfs_emit(buf, "%u%%\n", ss_power);
 
-out:
-	pm_runtime_mark_last_busy(ddev->dev);
-	pm_runtime_put_autosuspend(ddev->dev);
 	return r;
 }
 
@@ -1884,18 +1907,7 @@ static ssize_t amdgpu_set_smartshift_bias(struct device *dev,
 static int ss_power_attr_update(struct amdgpu_device *adev, struct amdgpu_device_attr *attr,
 				uint32_t mask, enum amdgpu_device_attr_states *states)
 {
-	uint32_t ss_power, size;
-
-	if (!amdgpu_acpi_is_power_shift_control_supported())
-		*states = ATTR_STATE_UNSUPPORTED;
-	else if ((adev->flags & AMD_IS_PX) &&
-		 !amdgpu_device_supports_smart_shift(adev_to_drm(adev)))
-		*states = ATTR_STATE_UNSUPPORTED;
-	else if (amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
-		 (void *)&ss_power, &size))
-		*states = ATTR_STATE_UNSUPPORTED;
-	else if (amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
-		 (void *)&ss_power, &size))
+	if (!amdgpu_device_supports_smart_shift(adev_to_drm(adev)))
 		*states = ATTR_STATE_UNSUPPORTED;
 
 	return 0;
-- 
2.25.1

Re: [PATCH v2] drm/amd/pm: consistent approach for smartshift

[Lazar, Lijo]

On 5/12/2022 4:26 PM, Sathishkumar S wrote:
> create smartshift sysfs attributes from dGPU device even
> on smartshift 1.0 platform to be consistent. Do not populate
> the attributes on platforms that have APU only but not dGPU
> or vice versa.
> 
> V2: avoid checking for the number of VGA/DISPLAY devices (Lijo)
> 
> Suggested-by: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Sathishkumar S <sathishkumar.sundararaju@amd.com>
> Acked-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>   drivers/gpu/drm/amd/pm/amdgpu_pm.c | 136 ++++++++++++++++-------------
>   1 file changed, 74 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> index d3228216b2da..292e8c241597 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> @@ -1734,22 +1734,11 @@ static ssize_t amdgpu_get_gpu_metrics(struct device *dev,
>   	return size;
>   }
>   
> -/**
> - * DOC: smartshift_apu_power
> - *
> - * The amdgpu driver provides a sysfs API for reporting APU power
> - * share if it supports smartshift. The value is expressed as
> - * the proportion of stapm limit where stapm limit is the total APU
> - * power limit. The result is in percentage. If APU power is 130% of
> - * STAPM, then APU is using 30% of the dGPU's headroom.
> - */
> -
> -static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, struct device_attribute *attr,
> -					       char *buf)
> +static int amdgpu_read_powershift_percent(struct amdgpu_device *adev,
> +						uint32_t *ss_power, bool dgpu_share)
>   {
> -	struct drm_device *ddev = dev_get_drvdata(dev);
> -	struct amdgpu_device *adev = drm_to_adev(ddev);
> -	uint32_t ss_power, size;
> +	struct drm_device *ddev = adev_to_drm(adev);
> +	uint32_t size;
>   	int r = 0;
>   
>   	if (amdgpu_in_reset(adev))
> @@ -1763,28 +1752,65 @@ static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, struct device
>   		return r;
>   	}
>   
> -	r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
> -				   (void *)&ss_power, &size);
> -	if (r)
> -		goto out;
> -
> -	r = sysfs_emit(buf, "%u%%\n", ss_power);
> +	if (dgpu_share)
> +		r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
> +				   (void *)ss_power, &size);
> +	else
> +		r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
> +				   (void *)ss_power, &size);
>   
> -out:
>   	pm_runtime_mark_last_busy(ddev->dev);
>   	pm_runtime_put_autosuspend(ddev->dev);
>   	return r;
>   }
> +/**
> + * DOC: smartshift_apu_power
> + *
> + * The amdgpu driver provides a sysfs API for reporting APU power
> + * shift in percentage if platform supports smartshift. Value 0 means that
> + * there is no powershift and values between [1-100] means that the power
> + * is shifted to APU, the percentage of boost is with respect to APU power
> + * limit on the platform.
> + */
> +
> +static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, struct device_attribute *attr,
> +					       char *buf)
> +{
> +	struct drm_device *ddev = dev_get_drvdata(dev);
> +	struct amdgpu_device *adev = drm_to_adev(ddev);
> +	uint32_t ss_power = 0;
> +	int r = 0, i;
> +
> +	r = amdgpu_read_powershift_percent(adev, &ss_power, false);
> +	if (r == -EOPNOTSUPP) {
> +		/* sensor not available on dGPU, try to read from APU */
> +		adev = NULL;
> +		mutex_lock(&mgpu_info.mutex);
> +		for (i = 0; i < mgpu_info.num_gpu; i++) {
> +			if (mgpu_info.gpu_ins[i].adev->flags & AMD_IS_APU) {
> +				adev = mgpu_info.gpu_ins[i].adev;
> +				break;
> +			}
> +		}
> +		mutex_unlock(&mgpu_info.mutex);
> +		if (adev)
> +			r = amdgpu_read_powershift_percent(adev, &ss_power, false);
> +	}
> +
> +	if (!r)
> +		r = sysfs_emit(buf, "%u%%\n", ss_power);
> +

In v1 patch comment, I meant something like this
	return amdgpu_show_powershift_percent(adev, false);

And the above logic is kept in amdgpu_show_powershift_percent();

amdgpu_get_smartshift_dgpu_power():
return amdgpu_show_powershift_percent(adev, true);
		
Thanks,
Lijo

> +	return r;
> +}
>   
>   /**
>    * DOC: smartshift_dgpu_power
>    *
> - * The amdgpu driver provides a sysfs API for reporting the dGPU power
> - * share if the device is in HG and supports smartshift. The value
> - * is expressed as the proportion of stapm limit where stapm limit
> - * is the total APU power limit. The value is in percentage. If dGPU
> - * power is 20% higher than STAPM power(120%), it's using 20% of the
> - * APU's power headroom.
> + * The amdgpu driver provides a sysfs API for reporting dGPU power
> + * shift in percentage if platform supports smartshift. Value 0 means that
> + * there is no powershift and values between [1-100] means that the power is
> + * shifted to dGPU, the percentage of boost is with respect to dGPU power
> + * limit on the platform.
>    */
>   
>   static ssize_t amdgpu_get_smartshift_dgpu_power(struct device *dev, struct device_attribute *attr,
> @@ -1792,31 +1818,28 @@ static ssize_t amdgpu_get_smartshift_dgpu_power(struct device *dev, struct devic
>   {
>   	struct drm_device *ddev = dev_get_drvdata(dev);
>   	struct amdgpu_device *adev = drm_to_adev(ddev);
> -	uint32_t ss_power, size;
> -	int r = 0;
> -
> -	if (amdgpu_in_reset(adev))
> -		return -EPERM;
> -	if (adev->in_suspend && !adev->in_runpm)
> -		return -EPERM;
> -
> -	r = pm_runtime_get_sync(ddev->dev);
> -	if (r < 0) {
> -		pm_runtime_put_autosuspend(ddev->dev);
> -		return r;
> +	uint32_t ss_power = 0;
> +	int r = 0, i;
> +
> +	r = amdgpu_read_powershift_percent(adev, &ss_power, true);
> +	if (r == -EOPNOTSUPP) {
> +		/* sensor not available on dGPU, try to read from APU */
> +		adev = NULL;
> +		mutex_lock(&mgpu_info.mutex);
> +		for (i = 0; i < mgpu_info.num_gpu; i++) {
> +			if (mgpu_info.gpu_ins[i].adev->flags & AMD_IS_APU) {
> +				adev = mgpu_info.gpu_ins[i].adev;
> +				break;
> +			}
> +		}
> +		mutex_unlock(&mgpu_info.mutex);
> +		if (adev)
> +			r = amdgpu_read_powershift_percent(adev, &ss_power, true);
>   	}
>   
> -	r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
> -				   (void *)&ss_power, &size);
> -
> -	if (r)
> -		goto out;
> -
> -	r = sysfs_emit(buf, "%u%%\n", ss_power);
> +	if (!r)
> +		r = sysfs_emit(buf, "%u%%\n", ss_power);
>   
> -out:
> -	pm_runtime_mark_last_busy(ddev->dev);
> -	pm_runtime_put_autosuspend(ddev->dev);
>   	return r;
>   }
>   
> @@ -1884,18 +1907,7 @@ static ssize_t amdgpu_set_smartshift_bias(struct device *dev,
>   static int ss_power_attr_update(struct amdgpu_device *adev, struct amdgpu_device_attr *attr,
>   				uint32_t mask, enum amdgpu_device_attr_states *states)
>   {
> -	uint32_t ss_power, size;
> -
> -	if (!amdgpu_acpi_is_power_shift_control_supported())
> -		*states = ATTR_STATE_UNSUPPORTED;
> -	else if ((adev->flags & AMD_IS_PX) &&
> -		 !amdgpu_device_supports_smart_shift(adev_to_drm(adev)))
> -		*states = ATTR_STATE_UNSUPPORTED;
> -	else if (amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
> -		 (void *)&ss_power, &size))
> -		*states = ATTR_STATE_UNSUPPORTED;
> -	else if (amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
> -		 (void *)&ss_power, &size))
> +	if (!amdgpu_device_supports_smart_shift(adev_to_drm(adev)))
>   		*states = ATTR_STATE_UNSUPPORTED;
>   
>   	return 0;
> 

Re: [PATCH v2] drm/amd/pm: consistent approach for smartshift

[Sundararaju, Sathishkumar]

On 5/12/2022 4:37 PM, Lazar, Lijo wrote:
>
>
> On 5/12/2022 4:26 PM, Sathishkumar S wrote:
>> create smartshift sysfs attributes from dGPU device even
>> on smartshift 1.0 platform to be consistent. Do not populate
>> the attributes on platforms that have APU only but not dGPU
>> or vice versa.
>>
>> V2: avoid checking for the number of VGA/DISPLAY devices (Lijo)
>>
>> Suggested-by: Alex Deucher <alexander.deucher@amd.com>
>> Signed-off-by: Sathishkumar S <sathishkumar.sundararaju@amd.com>
>> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>> ---
>>   drivers/gpu/drm/amd/pm/amdgpu_pm.c | 136 ++++++++++++++++-------------
>>   1 file changed, 74 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c 
>> b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>> index d3228216b2da..292e8c241597 100644
>> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>> @@ -1734,22 +1734,11 @@ static ssize_t amdgpu_get_gpu_metrics(struct 
>> device *dev,
>>       return size;
>>   }
>>   -/**
>> - * DOC: smartshift_apu_power
>> - *
>> - * The amdgpu driver provides a sysfs API for reporting APU power
>> - * share if it supports smartshift. The value is expressed as
>> - * the proportion of stapm limit where stapm limit is the total APU
>> - * power limit. The result is in percentage. If APU power is 130% of
>> - * STAPM, then APU is using 30% of the dGPU's headroom.
>> - */
>> -
>> -static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, 
>> struct device_attribute *attr,
>> -                           char *buf)
>> +static int amdgpu_read_powershift_percent(struct amdgpu_device *adev,
>> +                        uint32_t *ss_power, bool dgpu_share)
>>   {
>> -    struct drm_device *ddev = dev_get_drvdata(dev);
>> -    struct amdgpu_device *adev = drm_to_adev(ddev);
>> -    uint32_t ss_power, size;
>> +    struct drm_device *ddev = adev_to_drm(adev);
>> +    uint32_t size;
>>       int r = 0;
>>         if (amdgpu_in_reset(adev))
>> @@ -1763,28 +1752,65 @@ static ssize_t 
>> amdgpu_get_smartshift_apu_power(struct device *dev, struct device
>>           return r;
>>       }
>>   -    r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
>> -                   (void *)&ss_power, &size);
>> -    if (r)
>> -        goto out;
>> -
>> -    r = sysfs_emit(buf, "%u%%\n", ss_power);
>> +    if (dgpu_share)
>> +        r = amdgpu_dpm_read_sensor(adev, 
>> AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
>> +                   (void *)ss_power, &size);
>> +    else
>> +        r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
>> +                   (void *)ss_power, &size);
>>   -out:
>>       pm_runtime_mark_last_busy(ddev->dev);
>>       pm_runtime_put_autosuspend(ddev->dev);
>>       return r;
>>   }
>> +/**
>> + * DOC: smartshift_apu_power
>> + *
>> + * The amdgpu driver provides a sysfs API for reporting APU power
>> + * shift in percentage if platform supports smartshift. Value 0 
>> means that
>> + * there is no powershift and values between [1-100] means that the 
>> power
>> + * is shifted to APU, the percentage of boost is with respect to APU 
>> power
>> + * limit on the platform.
>> + */
>> +
>> +static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, 
>> struct device_attribute *attr,
>> +                           char *buf)
>> +{
>> +    struct drm_device *ddev = dev_get_drvdata(dev);
>> +    struct amdgpu_device *adev = drm_to_adev(ddev);
>> +    uint32_t ss_power = 0;
>> +    int r = 0, i;
>> +
>> +    r = amdgpu_read_powershift_percent(adev, &ss_power, false);
>> +    if (r == -EOPNOTSUPP) {
>> +        /* sensor not available on dGPU, try to read from APU */
>> +        adev = NULL;
>> +        mutex_lock(&mgpu_info.mutex);
>> +        for (i = 0; i < mgpu_info.num_gpu; i++) {
>> +            if (mgpu_info.gpu_ins[i].adev->flags & AMD_IS_APU) {
>> +                adev = mgpu_info.gpu_ins[i].adev;
>> +                break;
>> +            }
>> +        }
>> +        mutex_unlock(&mgpu_info.mutex);
>> +        if (adev)
>> +            r = amdgpu_read_powershift_percent(adev, &ss_power, false);
>> +    }
>> +
>> +    if (!r)
>> +        r = sysfs_emit(buf, "%u%%\n", ss_power);
>> +
>
> In v1 patch comment, I meant something like this
>     return amdgpu_show_powershift_percent(adev, false);
>
> And the above logic is kept in amdgpu_show_powershift_percent();
>
> amdgpu_get_smartshift_dgpu_power():
> return amdgpu_show_powershift_percent(adev, true);

okay got it, will do that. But will retain 
amdgpu_read_powershift_percent() as a separate function or rename it to 
amdgpu_device_read_powershift().

I hope you also didn't mean to combine them all into 
amdgpu_show_powershift_percent().

>
> Thanks,
> Lijo
>
>> +    return r;
>> +}
>>     /**
>>    * DOC: smartshift_dgpu_power
>>    *
>> - * The amdgpu driver provides a sysfs API for reporting the dGPU power
>> - * share if the device is in HG and supports smartshift. The value
>> - * is expressed as the proportion of stapm limit where stapm limit
>> - * is the total APU power limit. The value is in percentage. If dGPU
>> - * power is 20% higher than STAPM power(120%), it's using 20% of the
>> - * APU's power headroom.
>> + * The amdgpu driver provides a sysfs API for reporting dGPU power
>> + * shift in percentage if platform supports smartshift. Value 0 
>> means that
>> + * there is no powershift and values between [1-100] means that the 
>> power is
>> + * shifted to dGPU, the percentage of boost is with respect to dGPU 
>> power
>> + * limit on the platform.
>>    */
>>     static ssize_t amdgpu_get_smartshift_dgpu_power(struct device 
>> *dev, struct device_attribute *attr,
>> @@ -1792,31 +1818,28 @@ static ssize_t 
>> amdgpu_get_smartshift_dgpu_power(struct device *dev, struct devic
>>   {
>>       struct drm_device *ddev = dev_get_drvdata(dev);
>>       struct amdgpu_device *adev = drm_to_adev(ddev);
>> -    uint32_t ss_power, size;
>> -    int r = 0;
>> -
>> -    if (amdgpu_in_reset(adev))
>> -        return -EPERM;
>> -    if (adev->in_suspend && !adev->in_runpm)
>> -        return -EPERM;
>> -
>> -    r = pm_runtime_get_sync(ddev->dev);
>> -    if (r < 0) {
>> -        pm_runtime_put_autosuspend(ddev->dev);
>> -        return r;
>> +    uint32_t ss_power = 0;
>> +    int r = 0, i;
>> +
>> +    r = amdgpu_read_powershift_percent(adev, &ss_power, true);
>> +    if (r == -EOPNOTSUPP) {
>> +        /* sensor not available on dGPU, try to read from APU */
>> +        adev = NULL;
>> +        mutex_lock(&mgpu_info.mutex);
>> +        for (i = 0; i < mgpu_info.num_gpu; i++) {
>> +            if (mgpu_info.gpu_ins[i].adev->flags & AMD_IS_APU) {
>> +                adev = mgpu_info.gpu_ins[i].adev;
>> +                break;
>> +            }
>> +        }
>> +        mutex_unlock(&mgpu_info.mutex);
>> +        if (adev)
>> +            r = amdgpu_read_powershift_percent(adev, &ss_power, true);
>>       }
>>   -    r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
>> -                   (void *)&ss_power, &size);
>> -
>> -    if (r)
>> -        goto out;
>> -
>> -    r = sysfs_emit(buf, "%u%%\n", ss_power);
>> +    if (!r)
>> +        r = sysfs_emit(buf, "%u%%\n", ss_power);
>>   -out:
>> -    pm_runtime_mark_last_busy(ddev->dev);
>> -    pm_runtime_put_autosuspend(ddev->dev);
>>       return r;
>>   }
>>   @@ -1884,18 +1907,7 @@ static ssize_t 
>> amdgpu_set_smartshift_bias(struct device *dev,
>>   static int ss_power_attr_update(struct amdgpu_device *adev, struct 
>> amdgpu_device_attr *attr,
>>                   uint32_t mask, enum amdgpu_device_attr_states *states)
>>   {
>> -    uint32_t ss_power, size;
>> -
>> -    if (!amdgpu_acpi_is_power_shift_control_supported())
>> -        *states = ATTR_STATE_UNSUPPORTED;
>> -    else if ((adev->flags & AMD_IS_PX) &&
>> - !amdgpu_device_supports_smart_shift(adev_to_drm(adev)))
>> -        *states = ATTR_STATE_UNSUPPORTED;
>> -    else if (amdgpu_dpm_read_sensor(adev, 
>> AMDGPU_PP_SENSOR_SS_APU_SHARE,
>> -         (void *)&ss_power, &size))
>> -        *states = ATTR_STATE_UNSUPPORTED;
>> -    else if (amdgpu_dpm_read_sensor(adev, 
>> AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
>> -         (void *)&ss_power, &size))
>> +    if (!amdgpu_device_supports_smart_shift(adev_to_drm(adev)))
>>           *states = ATTR_STATE_UNSUPPORTED;
>>         return 0;
>>

Re: [PATCH v2] drm/amd/pm: consistent approach for smartshift

[Lazar, Lijo]

On 5/12/2022 5:42 PM, Sundararaju, Sathishkumar wrote:
> 
> On 5/12/2022 4:37 PM, Lazar, Lijo wrote:
>>
>>
>> On 5/12/2022 4:26 PM, Sathishkumar S wrote:
>>> create smartshift sysfs attributes from dGPU device even
>>> on smartshift 1.0 platform to be consistent. Do not populate
>>> the attributes on platforms that have APU only but not dGPU
>>> or vice versa.
>>>
>>> V2: avoid checking for the number of VGA/DISPLAY devices (Lijo)
>>>
>>> Suggested-by: Alex Deucher <alexander.deucher@amd.com>
>>> Signed-off-by: Sathishkumar S <sathishkumar.sundararaju@amd.com>
>>> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/pm/amdgpu_pm.c | 136 ++++++++++++++++-------------
>>>   1 file changed, 74 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c 
>>> b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>>> index d3228216b2da..292e8c241597 100644
>>> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>>> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>>> @@ -1734,22 +1734,11 @@ static ssize_t amdgpu_get_gpu_metrics(struct 
>>> device *dev,
>>>       return size;
>>>   }
>>>   -/**
>>> - * DOC: smartshift_apu_power
>>> - *
>>> - * The amdgpu driver provides a sysfs API for reporting APU power
>>> - * share if it supports smartshift. The value is expressed as
>>> - * the proportion of stapm limit where stapm limit is the total APU
>>> - * power limit. The result is in percentage. If APU power is 130% of
>>> - * STAPM, then APU is using 30% of the dGPU's headroom.
>>> - */
>>> -
>>> -static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, 
>>> struct device_attribute *attr,
>>> -                           char *buf)
>>> +static int amdgpu_read_powershift_percent(struct amdgpu_device *adev,
>>> +                        uint32_t *ss_power, bool dgpu_share)
>>>   {
>>> -    struct drm_device *ddev = dev_get_drvdata(dev);
>>> -    struct amdgpu_device *adev = drm_to_adev(ddev);
>>> -    uint32_t ss_power, size;
>>> +    struct drm_device *ddev = adev_to_drm(adev);
>>> +    uint32_t size;
>>>       int r = 0;
>>>         if (amdgpu_in_reset(adev))
>>> @@ -1763,28 +1752,65 @@ static ssize_t 
>>> amdgpu_get_smartshift_apu_power(struct device *dev, struct device
>>>           return r;
>>>       }
>>>   -    r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
>>> -                   (void *)&ss_power, &size);
>>> -    if (r)
>>> -        goto out;
>>> -
>>> -    r = sysfs_emit(buf, "%u%%\n", ss_power);
>>> +    if (dgpu_share)
>>> +        r = amdgpu_dpm_read_sensor(adev, 
>>> AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
>>> +                   (void *)ss_power, &size);
>>> +    else
>>> +        r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
>>> +                   (void *)ss_power, &size);
>>>   -out:
>>>       pm_runtime_mark_last_busy(ddev->dev);
>>>       pm_runtime_put_autosuspend(ddev->dev);
>>>       return r;
>>>   }
>>> +/**
>>> + * DOC: smartshift_apu_power
>>> + *
>>> + * The amdgpu driver provides a sysfs API for reporting APU power
>>> + * shift in percentage if platform supports smartshift. Value 0 
>>> means that
>>> + * there is no powershift and values between [1-100] means that the 
>>> power
>>> + * is shifted to APU, the percentage of boost is with respect to APU 
>>> power
>>> + * limit on the platform.
>>> + */
>>> +
>>> +static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, 
>>> struct device_attribute *attr,
>>> +                           char *buf)
>>> +{
>>> +    struct drm_device *ddev = dev_get_drvdata(dev);
>>> +    struct amdgpu_device *adev = drm_to_adev(ddev);
>>> +    uint32_t ss_power = 0;
>>> +    int r = 0, i;
>>> +
>>> +    r = amdgpu_read_powershift_percent(adev, &ss_power, false);
>>> +    if (r == -EOPNOTSUPP) {
>>> +        /* sensor not available on dGPU, try to read from APU */
>>> +        adev = NULL;
>>> +        mutex_lock(&mgpu_info.mutex);
>>> +        for (i = 0; i < mgpu_info.num_gpu; i++) {
>>> +            if (mgpu_info.gpu_ins[i].adev->flags & AMD_IS_APU) {
>>> +                adev = mgpu_info.gpu_ins[i].adev;
>>> +                break;
>>> +            }
>>> +        }
>>> +        mutex_unlock(&mgpu_info.mutex);
>>> +        if (adev)
>>> +            r = amdgpu_read_powershift_percent(adev, &ss_power, false);
>>> +    }
>>> +
>>> +    if (!r)
>>> +        r = sysfs_emit(buf, "%u%%\n", ss_power);
>>> +
>>
>> In v1 patch comment, I meant something like this
>>     return amdgpu_show_powershift_percent(adev, false);
>>
>> And the above logic is kept in amdgpu_show_powershift_percent();
>>
>> amdgpu_get_smartshift_dgpu_power():
>> return amdgpu_show_powershift_percent(adev, true);
> 
> okay got it, will do that. But will retain 
> amdgpu_read_powershift_percent() as a separate function or rename it to 
> amdgpu_device_read_powershift().
> 
> I hope you also didn't mean to combine them all into 
> amdgpu_show_powershift_percent().

No, I didn't mean to combine all of them. Only the above logic (fetching 
from dGPU, if not fetch from APU) may be kept in a single function. It's 
the same for any device regardless of dGPU or APU.

Thanks,
Lijo

> 
>>
>> Thanks,
>> Lijo
>>
>>> +    return r;
>>> +}
>>>     /**
>>>    * DOC: smartshift_dgpu_power
>>>    *
>>> - * The amdgpu driver provides a sysfs API for reporting the dGPU power
>>> - * share if the device is in HG and supports smartshift. The value
>>> - * is expressed as the proportion of stapm limit where stapm limit
>>> - * is the total APU power limit. The value is in percentage. If dGPU
>>> - * power is 20% higher than STAPM power(120%), it's using 20% of the
>>> - * APU's power headroom.
>>> + * The amdgpu driver provides a sysfs API for reporting dGPU power
>>> + * shift in percentage if platform supports smartshift. Value 0 
>>> means that
>>> + * there is no powershift and values between [1-100] means that the 
>>> power is
>>> + * shifted to dGPU, the percentage of boost is with respect to dGPU 
>>> power
>>> + * limit on the platform.
>>>    */
>>>     static ssize_t amdgpu_get_smartshift_dgpu_power(struct device 
>>> *dev, struct device_attribute *attr,
>>> @@ -1792,31 +1818,28 @@ static ssize_t 
>>> amdgpu_get_smartshift_dgpu_power(struct device *dev, struct devic
>>>   {
>>>       struct drm_device *ddev = dev_get_drvdata(dev);
>>>       struct amdgpu_device *adev = drm_to_adev(ddev);
>>> -    uint32_t ss_power, size;
>>> -    int r = 0;
>>> -
>>> -    if (amdgpu_in_reset(adev))
>>> -        return -EPERM;
>>> -    if (adev->in_suspend && !adev->in_runpm)
>>> -        return -EPERM;
>>> -
>>> -    r = pm_runtime_get_sync(ddev->dev);
>>> -    if (r < 0) {
>>> -        pm_runtime_put_autosuspend(ddev->dev);
>>> -        return r;
>>> +    uint32_t ss_power = 0;
>>> +    int r = 0, i;
>>> +
>>> +    r = amdgpu_read_powershift_percent(adev, &ss_power, true);
>>> +    if (r == -EOPNOTSUPP) {
>>> +        /* sensor not available on dGPU, try to read from APU */
>>> +        adev = NULL;
>>> +        mutex_lock(&mgpu_info.mutex);
>>> +        for (i = 0; i < mgpu_info.num_gpu; i++) {
>>> +            if (mgpu_info.gpu_ins[i].adev->flags & AMD_IS_APU) {
>>> +                adev = mgpu_info.gpu_ins[i].adev;
>>> +                break;
>>> +            }
>>> +        }
>>> +        mutex_unlock(&mgpu_info.mutex);
>>> +        if (adev)
>>> +            r = amdgpu_read_powershift_percent(adev, &ss_power, true);
>>>       }
>>>   -    r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
>>> -                   (void *)&ss_power, &size);
>>> -
>>> -    if (r)
>>> -        goto out;
>>> -
>>> -    r = sysfs_emit(buf, "%u%%\n", ss_power);
>>> +    if (!r)
>>> +        r = sysfs_emit(buf, "%u%%\n", ss_power);
>>>   -out:
>>> -    pm_runtime_mark_last_busy(ddev->dev);
>>> -    pm_runtime_put_autosuspend(ddev->dev);
>>>       return r;
>>>   }
>>>   @@ -1884,18 +1907,7 @@ static ssize_t 
>>> amdgpu_set_smartshift_bias(struct device *dev,
>>>   static int ss_power_attr_update(struct amdgpu_device *adev, struct 
>>> amdgpu_device_attr *attr,
>>>                   uint32_t mask, enum amdgpu_device_attr_states *states)
>>>   {
>>> -    uint32_t ss_power, size;
>>> -
>>> -    if (!amdgpu_acpi_is_power_shift_control_supported())
>>> -        *states = ATTR_STATE_UNSUPPORTED;
>>> -    else if ((adev->flags & AMD_IS_PX) &&
>>> - !amdgpu_device_supports_smart_shift(adev_to_drm(adev)))
>>> -        *states = ATTR_STATE_UNSUPPORTED;
>>> -    else if (amdgpu_dpm_read_sensor(adev, 
>>> AMDGPU_PP_SENSOR_SS_APU_SHARE,
>>> -         (void *)&ss_power, &size))
>>> -        *states = ATTR_STATE_UNSUPPORTED;
>>> -    else if (amdgpu_dpm_read_sensor(adev, 
>>> AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
>>> -         (void *)&ss_power, &size))
>>> +    if (!amdgpu_device_supports_smart_shift(adev_to_drm(adev)))
>>>           *states = ATTR_STATE_UNSUPPORTED;
>>>         return 0;
>>>

Re: [PATCH v2] drm/amd/pm: consistent approach for smartshift

[Lazar, Lijo]

On 5/13/2022 3:29 PM, Sathishkumar S wrote:
> create smartshift sysfs attributes from dGPU device even
> on smartshift 1.0 platform to be consistent. Do not populate
> the attributes on platforms that have APU only but not dGPU
> or vice versa.
> 
> V2:
>   avoid checking for the number of VGA/DISPLAY devices (Lijo)
>   move code to read from dGPU or APU into a function and reuse (Lijo)
> 
> Suggested-by: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Sathishkumar S <sathishkumar.sundararaju@amd.com>
> Acked-by: Alex Deucher <alexander.deucher@amd.com>

Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>

Thanks,
Lijo

> ---
>   drivers/gpu/drm/amd/pm/amdgpu_pm.c | 130 ++++++++++++++---------------
>   1 file changed, 62 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> index d3228216b2da..5e318b3f6c0f 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> @@ -1734,22 +1734,11 @@ static ssize_t amdgpu_get_gpu_metrics(struct device *dev,
>   	return size;
>   }
>   
> -/**
> - * DOC: smartshift_apu_power
> - *
> - * The amdgpu driver provides a sysfs API for reporting APU power
> - * share if it supports smartshift. The value is expressed as
> - * the proportion of stapm limit where stapm limit is the total APU
> - * power limit. The result is in percentage. If APU power is 130% of
> - * STAPM, then APU is using 30% of the dGPU's headroom.
> - */
> -
> -static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, struct device_attribute *attr,
> -					       char *buf)
> +static int amdgpu_device_read_powershift(struct amdgpu_device *adev,
> +						uint32_t *ss_power, bool dgpu_share)
>   {
> -	struct drm_device *ddev = dev_get_drvdata(dev);
> -	struct amdgpu_device *adev = drm_to_adev(ddev);
> -	uint32_t ss_power, size;
> +	struct drm_device *ddev = adev_to_drm(adev);
> +	uint32_t size;
>   	int r = 0;
>   
>   	if (amdgpu_in_reset(adev))
> @@ -1763,61 +1752,77 @@ static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, struct device
>   		return r;
>   	}
>   
> -	r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
> -				   (void *)&ss_power, &size);
> -	if (r)
> -		goto out;
> -
> -	r = sysfs_emit(buf, "%u%%\n", ss_power);
> +	if (dgpu_share)
> +		r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
> +				   (void *)ss_power, &size);
> +	else
> +		r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
> +				   (void *)ss_power, &size);
>   
> -out:
>   	pm_runtime_mark_last_busy(ddev->dev);
>   	pm_runtime_put_autosuspend(ddev->dev);
>   	return r;
>   }
>   
> -/**
> - * DOC: smartshift_dgpu_power
> - *
> - * The amdgpu driver provides a sysfs API for reporting the dGPU power
> - * share if the device is in HG and supports smartshift. The value
> - * is expressed as the proportion of stapm limit where stapm limit
> - * is the total APU power limit. The value is in percentage. If dGPU
> - * power is 20% higher than STAPM power(120%), it's using 20% of the
> - * APU's power headroom.
> - */
> -
> -static ssize_t amdgpu_get_smartshift_dgpu_power(struct device *dev, struct device_attribute *attr,
> -						char *buf)
> +static int amdgpu_show_powershift_percent(struct device *dev,
> +					char *buf, bool dgpu_share)
>   {
>   	struct drm_device *ddev = dev_get_drvdata(dev);
>   	struct amdgpu_device *adev = drm_to_adev(ddev);
> -	uint32_t ss_power, size;
> -	int r = 0;
> -
> -	if (amdgpu_in_reset(adev))
> -		return -EPERM;
> -	if (adev->in_suspend && !adev->in_runpm)
> -		return -EPERM;
> -
> -	r = pm_runtime_get_sync(ddev->dev);
> -	if (r < 0) {
> -		pm_runtime_put_autosuspend(ddev->dev);
> -		return r;
> +	uint32_t ss_power;
> +	int r = 0, i;
> +
> +	r = amdgpu_device_read_powershift(adev, &ss_power, dgpu_share);
> +	if (r == -EOPNOTSUPP) {
> +		/* sensor not available on dGPU, try to read from APU */
> +		adev = NULL;
> +		mutex_lock(&mgpu_info.mutex);
> +		for (i = 0; i < mgpu_info.num_gpu; i++) {
> +			if (mgpu_info.gpu_ins[i].adev->flags & AMD_IS_APU) {
> +				adev = mgpu_info.gpu_ins[i].adev;
> +				break;
> +			}
> +		}
> +		mutex_unlock(&mgpu_info.mutex);
> +		if (adev)
> +			r = amdgpu_device_read_powershift(adev, &ss_power, dgpu_share);
>   	}
>   
> -	r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
> -				   (void *)&ss_power, &size);
> +	if (!r)
> +		r = sysfs_emit(buf, "%u%%\n", ss_power);
>   
> -	if (r)
> -		goto out;
> +	return r;
> +}
> +/**
> + * DOC: smartshift_apu_power
> + *
> + * The amdgpu driver provides a sysfs API for reporting APU power
> + * shift in percentage if platform supports smartshift. Value 0 means that
> + * there is no powershift and values between [1-100] means that the power
> + * is shifted to APU, the percentage of boost is with respect to APU power
> + * limit on the platform.
> + */
>   
> -	r = sysfs_emit(buf, "%u%%\n", ss_power);
> +static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, struct device_attribute *attr,
> +					       char *buf)
> +{
> +	return amdgpu_show_powershift_percent(dev, buf, false);
> +}
>   
> -out:
> -	pm_runtime_mark_last_busy(ddev->dev);
> -	pm_runtime_put_autosuspend(ddev->dev);
> -	return r;
> +/**
> + * DOC: smartshift_dgpu_power
> + *
> + * The amdgpu driver provides a sysfs API for reporting dGPU power
> + * shift in percentage if platform supports smartshift. Value 0 means that
> + * there is no powershift and values between [1-100] means that the power is
> + * shifted to dGPU, the percentage of boost is with respect to dGPU power
> + * limit on the platform.
> + */
> +
> +static ssize_t amdgpu_get_smartshift_dgpu_power(struct device *dev, struct device_attribute *attr,
> +						char *buf)
> +{
> +	return amdgpu_show_powershift_percent(dev, buf, true);
>   }
>   
>   /**
> @@ -1884,18 +1889,7 @@ static ssize_t amdgpu_set_smartshift_bias(struct device *dev,
>   static int ss_power_attr_update(struct amdgpu_device *adev, struct amdgpu_device_attr *attr,
>   				uint32_t mask, enum amdgpu_device_attr_states *states)
>   {
> -	uint32_t ss_power, size;
> -
> -	if (!amdgpu_acpi_is_power_shift_control_supported())
> -		*states = ATTR_STATE_UNSUPPORTED;
> -	else if ((adev->flags & AMD_IS_PX) &&
> -		 !amdgpu_device_supports_smart_shift(adev_to_drm(adev)))
> -		*states = ATTR_STATE_UNSUPPORTED;
> -	else if (amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
> -		 (void *)&ss_power, &size))
> -		*states = ATTR_STATE_UNSUPPORTED;
> -	else if (amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
> -		 (void *)&ss_power, &size))
> +	if (!amdgpu_device_supports_smart_shift(adev_to_drm(adev)))
>   		*states = ATTR_STATE_UNSUPPORTED;
>   
>   	return 0;
> 

[PATCH v2] drm/amd/pm: consistent approach for smartshift

[Sathishkumar S]

create smartshift sysfs attributes from dGPU device even
on smartshift 1.0 platform to be consistent. Do not populate
the attributes on platforms that have APU only but not dGPU
or vice versa.

V2:
 avoid checking for the number of VGA/DISPLAY devices (Lijo)
 move code to read from dGPU or APU into a function and reuse (Lijo)

Suggested-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Sathishkumar S <sathishkumar.sundararaju@amd.com>
Acked-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/pm/amdgpu_pm.c | 130 ++++++++++++++---------------
 1 file changed, 62 insertions(+), 68 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
index d3228216b2da..5e318b3f6c0f 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
@@ -1734,22 +1734,11 @@ static ssize_t amdgpu_get_gpu_metrics(struct device *dev,
 	return size;
 }
 
-/**
- * DOC: smartshift_apu_power
- *
- * The amdgpu driver provides a sysfs API for reporting APU power
- * share if it supports smartshift. The value is expressed as
- * the proportion of stapm limit where stapm limit is the total APU
- * power limit. The result is in percentage. If APU power is 130% of
- * STAPM, then APU is using 30% of the dGPU's headroom.
- */
-
-static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, struct device_attribute *attr,
-					       char *buf)
+static int amdgpu_device_read_powershift(struct amdgpu_device *adev,
+						uint32_t *ss_power, bool dgpu_share)
 {
-	struct drm_device *ddev = dev_get_drvdata(dev);
-	struct amdgpu_device *adev = drm_to_adev(ddev);
-	uint32_t ss_power, size;
+	struct drm_device *ddev = adev_to_drm(adev);
+	uint32_t size;
 	int r = 0;
 
 	if (amdgpu_in_reset(adev))
@@ -1763,61 +1752,77 @@ static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, struct device
 		return r;
 	}
 
-	r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
-				   (void *)&ss_power, &size);
-	if (r)
-		goto out;
-
-	r = sysfs_emit(buf, "%u%%\n", ss_power);
+	if (dgpu_share)
+		r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
+				   (void *)ss_power, &size);
+	else
+		r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
+				   (void *)ss_power, &size);
 
-out:
 	pm_runtime_mark_last_busy(ddev->dev);
 	pm_runtime_put_autosuspend(ddev->dev);
 	return r;
 }
 
-/**
- * DOC: smartshift_dgpu_power
- *
- * The amdgpu driver provides a sysfs API for reporting the dGPU power
- * share if the device is in HG and supports smartshift. The value
- * is expressed as the proportion of stapm limit where stapm limit
- * is the total APU power limit. The value is in percentage. If dGPU
- * power is 20% higher than STAPM power(120%), it's using 20% of the
- * APU's power headroom.
- */
-
-static ssize_t amdgpu_get_smartshift_dgpu_power(struct device *dev, struct device_attribute *attr,
-						char *buf)
+static int amdgpu_show_powershift_percent(struct device *dev,
+					char *buf, bool dgpu_share)
 {
 	struct drm_device *ddev = dev_get_drvdata(dev);
 	struct amdgpu_device *adev = drm_to_adev(ddev);
-	uint32_t ss_power, size;
-	int r = 0;
-
-	if (amdgpu_in_reset(adev))
-		return -EPERM;
-	if (adev->in_suspend && !adev->in_runpm)
-		return -EPERM;
-
-	r = pm_runtime_get_sync(ddev->dev);
-	if (r < 0) {
-		pm_runtime_put_autosuspend(ddev->dev);
-		return r;
+	uint32_t ss_power;
+	int r = 0, i;
+
+	r = amdgpu_device_read_powershift(adev, &ss_power, dgpu_share);
+	if (r == -EOPNOTSUPP) {
+		/* sensor not available on dGPU, try to read from APU */
+		adev = NULL;
+		mutex_lock(&mgpu_info.mutex);
+		for (i = 0; i < mgpu_info.num_gpu; i++) {
+			if (mgpu_info.gpu_ins[i].adev->flags & AMD_IS_APU) {
+				adev = mgpu_info.gpu_ins[i].adev;
+				break;
+			}
+		}
+		mutex_unlock(&mgpu_info.mutex);
+		if (adev)
+			r = amdgpu_device_read_powershift(adev, &ss_power, dgpu_share);
 	}
 
-	r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
-				   (void *)&ss_power, &size);
+	if (!r)
+		r = sysfs_emit(buf, "%u%%\n", ss_power);
 
-	if (r)
-		goto out;
+	return r;
+}
+/**
+ * DOC: smartshift_apu_power
+ *
+ * The amdgpu driver provides a sysfs API for reporting APU power
+ * shift in percentage if platform supports smartshift. Value 0 means that
+ * there is no powershift and values between [1-100] means that the power
+ * is shifted to APU, the percentage of boost is with respect to APU power
+ * limit on the platform.
+ */
 
-	r = sysfs_emit(buf, "%u%%\n", ss_power);
+static ssize_t amdgpu_get_smartshift_apu_power(struct device *dev, struct device_attribute *attr,
+					       char *buf)
+{
+	return amdgpu_show_powershift_percent(dev, buf, false);
+}
 
-out:
-	pm_runtime_mark_last_busy(ddev->dev);
-	pm_runtime_put_autosuspend(ddev->dev);
-	return r;
+/**
+ * DOC: smartshift_dgpu_power
+ *
+ * The amdgpu driver provides a sysfs API for reporting dGPU power
+ * shift in percentage if platform supports smartshift. Value 0 means that
+ * there is no powershift and values between [1-100] means that the power is
+ * shifted to dGPU, the percentage of boost is with respect to dGPU power
+ * limit on the platform.
+ */
+
+static ssize_t amdgpu_get_smartshift_dgpu_power(struct device *dev, struct device_attribute *attr,
+						char *buf)
+{
+	return amdgpu_show_powershift_percent(dev, buf, true);
 }
 
 /**
@@ -1884,18 +1889,7 @@ static ssize_t amdgpu_set_smartshift_bias(struct device *dev,
 static int ss_power_attr_update(struct amdgpu_device *adev, struct amdgpu_device_attr *attr,
 				uint32_t mask, enum amdgpu_device_attr_states *states)
 {
-	uint32_t ss_power, size;
-
-	if (!amdgpu_acpi_is_power_shift_control_supported())
-		*states = ATTR_STATE_UNSUPPORTED;
-	else if ((adev->flags & AMD_IS_PX) &&
-		 !amdgpu_device_supports_smart_shift(adev_to_drm(adev)))
-		*states = ATTR_STATE_UNSUPPORTED;
-	else if (amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_APU_SHARE,
-		 (void *)&ss_power, &size))
-		*states = ATTR_STATE_UNSUPPORTED;
-	else if (amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_SS_DGPU_SHARE,
-		 (void *)&ss_power, &size))
+	if (!amdgpu_device_supports_smart_shift(adev_to_drm(adev)))
 		*states = ATTR_STATE_UNSUPPORTED;
 
 	return 0;
-- 
2.25.1