[PATCH 1/3] Revert "drm/amd/powerply: fix power reading on Fiji"

[PATCH 1/3] Revert "drm/amd/powerply: fix power reading on Fiji"

[Rex Zhu]

we don't have limit of [50ms, 4sec] sampling period.
smu calculate average gpu power in real time.
we can read average gpu power through smu message or
read special register.

This reverts commit 462d8dcc9fec0d89f1ff6a1f93f1d4f670878c71.

Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
index 21c021b..388184e 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
@@ -3364,8 +3364,7 @@ static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
 			"Failed to start pm status log!",
 			return -1);
 
-	/* Sampling period from 50ms to 4sec */
-	msleep_interruptible(200);
+	msleep_interruptible(20);
 
 	PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
 			PPSMC_MSG_PmStatusLogSample),
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/3] drm/amd/pp: Refine get_gpu_power for VI

[Rex Zhu]

pkgpwr is the average gpu power of 100ms. it is calculated by
firmware in real time.

1. we can send smu message PPSMC_MSG_GetCurrPkgPwr to read currentpkgpwr directly.

2. On Fiji/tonga/bonaire/hawwii, without PPSMC_MSG_GetCurrPkgPwr support.
   Send PPSMC_MSG_PmStatusLogStart/Sample to let smu write currentpkgpwr
   to ixSMU_PM_STATUS_94. driver can read pkgpwr from ixSMU_PM_STATUS_94.

Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c   | 51 ++++++++++++----------
 drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c | 10 +++--
 2 files changed, 34 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
index 388184e..20f5a6f 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
@@ -3359,30 +3359,33 @@ static int smu7_get_pp_table_entry(struct pp_hwmgr *hwmgr,
 static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
 		struct pp_gpu_power *query)
 {
-	PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
-			PPSMC_MSG_PmStatusLogStart),
-			"Failed to start pm status log!",
-			return -1);
-
-	msleep_interruptible(20);
-
-	PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
-			PPSMC_MSG_PmStatusLogSample),
-			"Failed to sample pm status log!",
-			return -1);
-
-	query->vddc_power = cgs_read_ind_register(hwmgr->device,
-			CGS_IND_REG__SMC,
-			ixSMU_PM_STATUS_40);
-	query->vddci_power = cgs_read_ind_register(hwmgr->device,
-			CGS_IND_REG__SMC,
-			ixSMU_PM_STATUS_49);
-	query->max_gpu_power = cgs_read_ind_register(hwmgr->device,
-			CGS_IND_REG__SMC,
-			ixSMU_PM_STATUS_94);
-	query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
-			CGS_IND_REG__SMC,
-			ixSMU_PM_STATUS_95);
+	int i;
+
+	if (!query)
+		return -EINVAL;
+
+
+	memset(query, 0, sizeof *query);
+
+	smum_send_msg_to_smc_with_parameter(hwmgr, PPSMC_MSG_GetCurrPkgPwr, 0);
+	query->average_gpu_power = cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
+
+	if (query->average_gpu_power != 0)
+		return 0;
+
+	smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogStart);
+	cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC,
+							ixSMU_PM_STATUS_94, 0);
+
+	for (i = 0; i < 20; i++) {
+		mdelay(1);
+		smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogSample);
+		query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
+						CGS_IND_REG__SMC,
+						ixSMU_PM_STATUS_94);
+		if (query->average_gpu_power != 0)
+			break;
+	}
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c b/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
index fb32a3f..10a1123 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
@@ -171,8 +171,10 @@ int smu7_send_msg_to_smc(struct pp_hwmgr *hwmgr, uint16_t msg)
 
 	ret = PHM_READ_FIELD(hwmgr->device, SMC_RESP_0, SMC_RESP);
 
-	if (ret != 1)
-		pr_info("\n failed to send pre message %x ret is %d \n",  msg, ret);
+	if (ret == 0xFE)
+		pr_debug("last message was not supported\n");
+	else if (ret != 1)
+		pr_info("\n last message was failed ret is %d\n", ret);
 
 	cgs_write_register(hwmgr->device, mmSMC_MESSAGE_0, msg);
 
@@ -180,7 +182,9 @@ int smu7_send_msg_to_smc(struct pp_hwmgr *hwmgr, uint16_t msg)
 
 	ret = PHM_READ_FIELD(hwmgr->device, SMC_RESP_0, SMC_RESP);
 
-	if (ret != 1)
+	if (ret == 0xFE)
+		pr_debug("message %x was not supported\n", msg);
+	else if (ret != 1)
 		pr_info("\n failed to send message %x ret is %d \n",  msg, ret);
 
 	return 0;
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amd/pp: Refine get_gpu_power for VI

[Alex Deucher]

On Wed, Apr 11, 2018 at 2:31 AM, Rex Zhu <Rex.Zhu@amd.com> wrote:
> pkgpwr is the average gpu power of 100ms. it is calculated by
> firmware in real time.
>
> 1. we can send smu message PPSMC_MSG_GetCurrPkgPwr to read currentpkgpwr directly.
>
> 2. On Fiji/tonga/bonaire/hawwii, without PPSMC_MSG_GetCurrPkgPwr support.
>    Send PPSMC_MSG_PmStatusLogStart/Sample to let smu write currentpkgpwr
>    to ixSMU_PM_STATUS_94. driver can read pkgpwr from ixSMU_PM_STATUS_94.
>
> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>

Assuming Eric is ok with removing the other power readings,

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c   | 51 ++++++++++++----------
>  drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c | 10 +++--
>  2 files changed, 34 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> index 388184e..20f5a6f 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> @@ -3359,30 +3359,33 @@ static int smu7_get_pp_table_entry(struct pp_hwmgr *hwmgr,
>  static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
>                 struct pp_gpu_power *query)
>  {
> -       PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> -                       PPSMC_MSG_PmStatusLogStart),
> -                       "Failed to start pm status log!",
> -                       return -1);
> -
> -       msleep_interruptible(20);
> -
> -       PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> -                       PPSMC_MSG_PmStatusLogSample),
> -                       "Failed to sample pm status log!",
> -                       return -1);
> -
> -       query->vddc_power = cgs_read_ind_register(hwmgr->device,
> -                       CGS_IND_REG__SMC,
> -                       ixSMU_PM_STATUS_40);
> -       query->vddci_power = cgs_read_ind_register(hwmgr->device,
> -                       CGS_IND_REG__SMC,
> -                       ixSMU_PM_STATUS_49);
> -       query->max_gpu_power = cgs_read_ind_register(hwmgr->device,
> -                       CGS_IND_REG__SMC,
> -                       ixSMU_PM_STATUS_94);
> -       query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
> -                       CGS_IND_REG__SMC,
> -                       ixSMU_PM_STATUS_95);
> +       int i;
> +
> +       if (!query)
> +               return -EINVAL;
> +
> +
> +       memset(query, 0, sizeof *query);
> +
> +       smum_send_msg_to_smc_with_parameter(hwmgr, PPSMC_MSG_GetCurrPkgPwr, 0);
> +       query->average_gpu_power = cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
> +
> +       if (query->average_gpu_power != 0)
> +               return 0;
> +
> +       smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogStart);
> +       cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC,
> +                                                       ixSMU_PM_STATUS_94, 0);
> +
> +       for (i = 0; i < 20; i++) {
> +               mdelay(1);
> +               smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogSample);
> +               query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
> +                                               CGS_IND_REG__SMC,
> +                                               ixSMU_PM_STATUS_94);
> +               if (query->average_gpu_power != 0)
> +                       break;
> +       }
>
>         return 0;
>  }
> diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c b/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
> index fb32a3f..10a1123 100644
> --- a/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
> @@ -171,8 +171,10 @@ int smu7_send_msg_to_smc(struct pp_hwmgr *hwmgr, uint16_t msg)
>
>         ret = PHM_READ_FIELD(hwmgr->device, SMC_RESP_0, SMC_RESP);
>
> -       if (ret != 1)
> -               pr_info("\n failed to send pre message %x ret is %d \n",  msg, ret);
> +       if (ret == 0xFE)
> +               pr_debug("last message was not supported\n");
> +       else if (ret != 1)
> +               pr_info("\n last message was failed ret is %d\n", ret);
>
>         cgs_write_register(hwmgr->device, mmSMC_MESSAGE_0, msg);
>
> @@ -180,7 +182,9 @@ int smu7_send_msg_to_smc(struct pp_hwmgr *hwmgr, uint16_t msg)
>
>         ret = PHM_READ_FIELD(hwmgr->device, SMC_RESP_0, SMC_RESP);
>
> -       if (ret != 1)
> +       if (ret == 0xFE)
> +               pr_debug("message %x was not supported\n", msg);
> +       else if (ret != 1)
>                 pr_info("\n failed to send message %x ret is %d \n",  msg, ret);
>
>         return 0;
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amd/pp: Refine get_gpu_power for VI

[Eric Huang]

This patch change the power registers reading from average to maximum. 
If SMU team verifies it, I am OK with it.

Regards,
Eric

On 2018-04-11 01:21 PM, Alex Deucher wrote:
> On Wed, Apr 11, 2018 at 2:31 AM, Rex Zhu <Rex.Zhu@amd.com> wrote:
>> pkgpwr is the average gpu power of 100ms. it is calculated by
>> firmware in real time.
>>
>> 1. we can send smu message PPSMC_MSG_GetCurrPkgPwr to read currentpkgpwr directly.
>>
>> 2. On Fiji/tonga/bonaire/hawwii, without PPSMC_MSG_GetCurrPkgPwr support.
>>     Send PPSMC_MSG_PmStatusLogStart/Sample to let smu write currentpkgpwr
>>     to ixSMU_PM_STATUS_94. driver can read pkgpwr from ixSMU_PM_STATUS_94.
>>
>> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> Assuming Eric is ok with removing the other power readings,
>
> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>
>> ---
>>   drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c   | 51 ++++++++++++----------
>>   drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c | 10 +++--
>>   2 files changed, 34 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> index 388184e..20f5a6f 100644
>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> @@ -3359,30 +3359,33 @@ static int smu7_get_pp_table_entry(struct pp_hwmgr *hwmgr,
>>   static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
>>                  struct pp_gpu_power *query)
>>   {
>> -       PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
>> -                       PPSMC_MSG_PmStatusLogStart),
>> -                       "Failed to start pm status log!",
>> -                       return -1);
>> -
>> -       msleep_interruptible(20);
>> -
>> -       PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
>> -                       PPSMC_MSG_PmStatusLogSample),
>> -                       "Failed to sample pm status log!",
>> -                       return -1);
>> -
>> -       query->vddc_power = cgs_read_ind_register(hwmgr->device,
>> -                       CGS_IND_REG__SMC,
>> -                       ixSMU_PM_STATUS_40);
>> -       query->vddci_power = cgs_read_ind_register(hwmgr->device,
>> -                       CGS_IND_REG__SMC,
>> -                       ixSMU_PM_STATUS_49);
>> -       query->max_gpu_power = cgs_read_ind_register(hwmgr->device,
>> -                       CGS_IND_REG__SMC,
>> -                       ixSMU_PM_STATUS_94);
>> -       query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
>> -                       CGS_IND_REG__SMC,
>> -                       ixSMU_PM_STATUS_95);
>> +       int i;
>> +
>> +       if (!query)
>> +               return -EINVAL;
>> +
>> +
>> +       memset(query, 0, sizeof *query);
>> +
>> +       smum_send_msg_to_smc_with_parameter(hwmgr, PPSMC_MSG_GetCurrPkgPwr, 0);
>> +       query->average_gpu_power = cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
>> +
>> +       if (query->average_gpu_power != 0)
>> +               return 0;
>> +
>> +       smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogStart);
>> +       cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC,
>> +                                                       ixSMU_PM_STATUS_94, 0);
>> +
>> +       for (i = 0; i < 20; i++) {
>> +               mdelay(1);
>> +               smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogSample);
>> +               query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
>> +                                               CGS_IND_REG__SMC,
>> +                                               ixSMU_PM_STATUS_94);
>> +               if (query->average_gpu_power != 0)
>> +                       break;
>> +       }
>>
>>          return 0;
>>   }
>> diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c b/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
>> index fb32a3f..10a1123 100644
>> --- a/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
>> +++ b/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
>> @@ -171,8 +171,10 @@ int smu7_send_msg_to_smc(struct pp_hwmgr *hwmgr, uint16_t msg)
>>
>>          ret = PHM_READ_FIELD(hwmgr->device, SMC_RESP_0, SMC_RESP);
>>
>> -       if (ret != 1)
>> -               pr_info("\n failed to send pre message %x ret is %d \n",  msg, ret);
>> +       if (ret == 0xFE)
>> +               pr_debug("last message was not supported\n");
>> +       else if (ret != 1)
>> +               pr_info("\n last message was failed ret is %d\n", ret);
>>
>>          cgs_write_register(hwmgr->device, mmSMC_MESSAGE_0, msg);
>>
>> @@ -180,7 +182,9 @@ int smu7_send_msg_to_smc(struct pp_hwmgr *hwmgr, uint16_t msg)
>>
>>          ret = PHM_READ_FIELD(hwmgr->device, SMC_RESP_0, SMC_RESP);
>>
>> -       if (ret != 1)
>> +       if (ret == 0xFE)
>> +               pr_debug("message %x was not supported\n", msg);
>> +       else if (ret != 1)
>>                  pr_info("\n failed to send message %x ret is %d \n",  msg, ret);
>>
>>          return 0;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/3] drm/amd/pp: Remove struct pp_gpu_power

[Rex Zhu]

This struct can't be used for all asics.

Currently smu only calculate average gpu power in real time.

for vddc/vddci/max power,
we can read them per 1ms through some registers. but

1. the type of return values is not unified.
    For Vi, return type is uint. For vega, return type is float.

2. need to assign the unit time to calculate the average power.

so remove this struct.

if user need to know the power on vddc/vddci.
we can export them with new common interface/struct.

Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c            |  7 ++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c             | 22 +++++++--------------
 drivers/gpu/drm/amd/include/kgd_pp_interface.h     |  7 -------
 drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c   | 23 +++++++++-------------
 drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 17 +++++++---------
 drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c | 13 ++++--------
 6 files changed, 29 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 487d39e..1e59818 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -701,9 +701,6 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
 		}
 	}
 	case AMDGPU_INFO_SENSOR: {
-		struct pp_gpu_power query = {0};
-		int query_size = sizeof(query);
-
 		if (!adev->pm.dpm_enabled)
 			return -ENOENT;
 
@@ -746,10 +743,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
 			/* get average GPU power */
 			if (amdgpu_dpm_read_sensor(adev,
 						   AMDGPU_PP_SENSOR_GPU_POWER,
-						   (void *)&query, &query_size)) {
+						   (void *)&ui32, &ui32_size)) {
 				return -EINVAL;
 			}
-			ui32 = query.average_gpu_power >> 8;
+			ui32 >>= 8;
 			break;
 		case AMDGPU_INFO_SENSOR_VDDNB:
 			/* get VDDNB in millivolts */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index e5f60fc..744f105 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -1020,8 +1020,8 @@ static ssize_t amdgpu_hwmon_show_power_avg(struct device *dev,
 {
 	struct amdgpu_device *adev = dev_get_drvdata(dev);
 	struct drm_device *ddev = adev->ddev;
-	struct pp_gpu_power query = {0};
-	int r, size = sizeof(query);
+	u32 query = 0;
+	int r, size = sizeof(u32);
 	unsigned uw;
 
 	/* Can't get power when the card is off */
@@ -1041,7 +1041,7 @@ static ssize_t amdgpu_hwmon_show_power_avg(struct device *dev,
 		return r;
 
 	/* convert to microwatts */
-	uw = (query.average_gpu_power >> 8) * 1000000;
+	uw = (query >> 8) * 1000000 + (query & 0xff) * 1000;
 
 	return snprintf(buf, PAGE_SIZE, "%u\n", uw);
 }
@@ -1752,7 +1752,7 @@ void amdgpu_pm_compute_clocks(struct amdgpu_device *adev)
 static int amdgpu_debugfs_pm_info_pp(struct seq_file *m, struct amdgpu_device *adev)
 {
 	uint32_t value;
-	struct pp_gpu_power query = {0};
+	uint32_t query = 0;
 	int size;
 
 	/* sanity check PP is enabled */
@@ -1775,17 +1775,9 @@ static int amdgpu_debugfs_pm_info_pp(struct seq_file *m, struct amdgpu_device *a
 		seq_printf(m, "\t%u mV (VDDGFX)\n", value);
 	if (!amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_VDDNB, (void *)&value, &size))
 		seq_printf(m, "\t%u mV (VDDNB)\n", value);
-	size = sizeof(query);
-	if (!amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GPU_POWER, (void *)&query, &size)) {
-		seq_printf(m, "\t%u.%u W (VDDC)\n", query.vddc_power >> 8,
-				query.vddc_power & 0xff);
-		seq_printf(m, "\t%u.%u W (VDDCI)\n", query.vddci_power >> 8,
-				query.vddci_power & 0xff);
-		seq_printf(m, "\t%u.%u W (max GPU)\n", query.max_gpu_power >> 8,
-				query.max_gpu_power & 0xff);
-		seq_printf(m, "\t%u.%u W (average GPU)\n", query.average_gpu_power >> 8,
-				query.average_gpu_power & 0xff);
-	}
+	size = sizeof(uint32_t);
+	if (!amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GPU_POWER, (void *)&query, &size))
+		seq_printf(m, "\t%u.%u W (average GPU)\n", query >> 8, query & 0xff);
 	size = sizeof(value);
 	seq_printf(m, "\n");
 
diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
index 5c840c0..1bec907 100644
--- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
+++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
@@ -149,13 +149,6 @@ struct pp_states_info {
 	uint32_t states[16];
 };
 
-struct pp_gpu_power {
-	uint32_t vddc_power;
-	uint32_t vddci_power;
-	uint32_t max_gpu_power;
-	uint32_t average_gpu_power;
-};
-
 #define PP_GROUP_MASK        0xF0000000
 #define PP_GROUP_SHIFT       28
 
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
index 20f5a6f..494c1ff 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
@@ -3356,36 +3356,34 @@ static int smu7_get_pp_table_entry(struct pp_hwmgr *hwmgr,
 	return 0;
 }
 
-static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
-		struct pp_gpu_power *query)
+static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr, u32 *query)
 {
 	int i;
+	u32 tmp = 0;
 
 	if (!query)
 		return -EINVAL;
 
-
-	memset(query, 0, sizeof *query);
-
 	smum_send_msg_to_smc_with_parameter(hwmgr, PPSMC_MSG_GetCurrPkgPwr, 0);
-	query->average_gpu_power = cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
+	tmp = cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
 
-	if (query->average_gpu_power != 0)
+	if (tmp != 0)
 		return 0;
 
 	smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogStart);
 	cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC,
 							ixSMU_PM_STATUS_94, 0);
 
-	for (i = 0; i < 20; i++) {
+	for (i = 0; i < 10; i++) {
 		mdelay(1);
 		smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogSample);
-		query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
+		tmp = cgs_read_ind_register(hwmgr->device,
 						CGS_IND_REG__SMC,
 						ixSMU_PM_STATUS_94);
-		if (query->average_gpu_power != 0)
+		if (tmp != 0)
 			break;
 	}
+	*query = tmp;
 
 	return 0;
 }
@@ -3438,10 +3436,7 @@ static int smu7_read_sensor(struct pp_hwmgr *hwmgr, int idx,
 		*size = 4;
 		return 0;
 	case AMDGPU_PP_SENSOR_GPU_POWER:
-		if (*size < sizeof(struct pp_gpu_power))
-			return -EINVAL;
-		*size = sizeof(struct pp_gpu_power);
-		return smu7_get_gpu_power(hwmgr, (struct pp_gpu_power *)value);
+		return smu7_get_gpu_power(hwmgr, (uint32_t *)value);
 	case AMDGPU_PP_SENSOR_VDDGFX:
 		if ((data->vr_config & 0xff) == 0x2)
 			val_vid = PHM_READ_INDIRECT_FIELD(hwmgr->device,
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
index 127c550..0bbc564 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
@@ -3781,16 +3781,18 @@ static uint32_t vega10_dpm_get_mclk(struct pp_hwmgr *hwmgr, bool low)
 }
 
 static int vega10_get_gpu_power(struct pp_hwmgr *hwmgr,
-		struct pp_gpu_power *query)
+		uint32_t *query)
 {
 	uint32_t value;
 
+	if (!query)
+		return -EINVAL;
+
 	smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetCurrPkgPwr);
 	value = smum_get_argument(hwmgr);
 
-	/* power value is an integer */
-	memset(query, 0, sizeof *query);
-	query->average_gpu_power = value << 8;
+	/* SMC returning actual watts, keep consistent with legacy asics, low 8 bit as 8 fractional bits */
+	*query = value << 8;
 
 	return 0;
 }
@@ -3840,12 +3842,7 @@ static int vega10_read_sensor(struct pp_hwmgr *hwmgr, int idx,
 		*size = 4;
 		break;
 	case AMDGPU_PP_SENSOR_GPU_POWER:
-		if (*size < sizeof(struct pp_gpu_power))
-			ret = -EINVAL;
-		else {
-			*size = sizeof(struct pp_gpu_power);
-			ret = vega10_get_gpu_power(hwmgr, (struct pp_gpu_power *)value);
-		}
+		ret = vega10_get_gpu_power(hwmgr, (uint32_t *)value);
 		break;
 	case AMDGPU_PP_SENSOR_VDDGFX:
 		val_vid = (RREG32_SOC15(SMUIO, 0, mmSMUSVI0_PLANE0_CURRENTVID) &
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
index 3e1ed0a..782e209 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
@@ -1113,8 +1113,7 @@ static uint32_t vega12_dpm_get_mclk(struct pp_hwmgr *hwmgr, bool low)
 	return (mem_clk * 100);
 }
 
-static int vega12_get_gpu_power(struct pp_hwmgr *hwmgr,
-		struct pp_gpu_power *query)
+static int vega12_get_gpu_power(struct pp_hwmgr *hwmgr, uint32_t *query)
 {
 #if 0
 	uint32_t value;
@@ -1126,7 +1125,7 @@ static int vega12_get_gpu_power(struct pp_hwmgr *hwmgr,
 
 	vega12_read_arg_from_smc(hwmgr, &value);
 	/* power value is an integer */
-	query->average_gpu_power = value << 8;
+	*query = value << 8;
 #endif
 	return 0;
 }
@@ -1235,12 +1234,8 @@ static int vega12_read_sensor(struct pp_hwmgr *hwmgr, int idx,
 		*size = 4;
 		break;
 	case AMDGPU_PP_SENSOR_GPU_POWER:
-		if (*size < sizeof(struct pp_gpu_power))
-			ret = -EINVAL;
-		else {
-			*size = sizeof(struct pp_gpu_power);
-			ret = vega12_get_gpu_power(hwmgr, (struct pp_gpu_power *)value);
-		}
+		ret = vega12_get_gpu_power(hwmgr, (uint32_t *)value);
+
 		break;
 	default:
 		ret = -EINVAL;
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/3] drm/amd/pp: Remove struct pp_gpu_power

[Alex Deucher]

On Wed, Apr 11, 2018 at 2:31 AM, Rex Zhu <Rex.Zhu@amd.com> wrote:
> This struct can't be used for all asics.
>
> Currently smu only calculate average gpu power in real time.
>
> for vddc/vddci/max power,
> we can read them per 1ms through some registers. but
>
> 1. the type of return values is not unified.
>     For Vi, return type is uint. For vega, return type is float.
>
> 2. need to assign the unit time to calculate the average power.
>
> so remove this struct.
>
> if user need to know the power on vddc/vddci.
> we can export them with new common interface/struct.
>

If Tom and Eric are ok with this:
Acked-by: Alex Deucher <alexander.deucher@amd.com>

> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c            |  7 ++-----
>  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c             | 22 +++++++--------------
>  drivers/gpu/drm/amd/include/kgd_pp_interface.h     |  7 -------
>  drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c   | 23 +++++++++-------------
>  drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 17 +++++++---------
>  drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c | 13 ++++--------
>  6 files changed, 29 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 487d39e..1e59818 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -701,9 +701,6 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>                 }
>         }
>         case AMDGPU_INFO_SENSOR: {
> -               struct pp_gpu_power query = {0};
> -               int query_size = sizeof(query);
> -
>                 if (!adev->pm.dpm_enabled)
>                         return -ENOENT;
>
> @@ -746,10 +743,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>                         /* get average GPU power */
>                         if (amdgpu_dpm_read_sensor(adev,
>                                                    AMDGPU_PP_SENSOR_GPU_POWER,
> -                                                  (void *)&query, &query_size)) {
> +                                                  (void *)&ui32, &ui32_size)) {
>                                 return -EINVAL;
>                         }
> -                       ui32 = query.average_gpu_power >> 8;
> +                       ui32 >>= 8;
>                         break;
>                 case AMDGPU_INFO_SENSOR_VDDNB:
>                         /* get VDDNB in millivolts */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index e5f60fc..744f105 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -1020,8 +1020,8 @@ static ssize_t amdgpu_hwmon_show_power_avg(struct device *dev,
>  {
>         struct amdgpu_device *adev = dev_get_drvdata(dev);
>         struct drm_device *ddev = adev->ddev;
> -       struct pp_gpu_power query = {0};
> -       int r, size = sizeof(query);
> +       u32 query = 0;
> +       int r, size = sizeof(u32);
>         unsigned uw;
>
>         /* Can't get power when the card is off */
> @@ -1041,7 +1041,7 @@ static ssize_t amdgpu_hwmon_show_power_avg(struct device *dev,
>                 return r;
>
>         /* convert to microwatts */
> -       uw = (query.average_gpu_power >> 8) * 1000000;
> +       uw = (query >> 8) * 1000000 + (query & 0xff) * 1000;
>
>         return snprintf(buf, PAGE_SIZE, "%u\n", uw);
>  }
> @@ -1752,7 +1752,7 @@ void amdgpu_pm_compute_clocks(struct amdgpu_device *adev)
>  static int amdgpu_debugfs_pm_info_pp(struct seq_file *m, struct amdgpu_device *adev)
>  {
>         uint32_t value;
> -       struct pp_gpu_power query = {0};
> +       uint32_t query = 0;
>         int size;
>
>         /* sanity check PP is enabled */
> @@ -1775,17 +1775,9 @@ static int amdgpu_debugfs_pm_info_pp(struct seq_file *m, struct amdgpu_device *a
>                 seq_printf(m, "\t%u mV (VDDGFX)\n", value);
>         if (!amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_VDDNB, (void *)&value, &size))
>                 seq_printf(m, "\t%u mV (VDDNB)\n", value);
> -       size = sizeof(query);
> -       if (!amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GPU_POWER, (void *)&query, &size)) {
> -               seq_printf(m, "\t%u.%u W (VDDC)\n", query.vddc_power >> 8,
> -                               query.vddc_power & 0xff);
> -               seq_printf(m, "\t%u.%u W (VDDCI)\n", query.vddci_power >> 8,
> -                               query.vddci_power & 0xff);
> -               seq_printf(m, "\t%u.%u W (max GPU)\n", query.max_gpu_power >> 8,
> -                               query.max_gpu_power & 0xff);
> -               seq_printf(m, "\t%u.%u W (average GPU)\n", query.average_gpu_power >> 8,
> -                               query.average_gpu_power & 0xff);
> -       }
> +       size = sizeof(uint32_t);
> +       if (!amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GPU_POWER, (void *)&query, &size))
> +               seq_printf(m, "\t%u.%u W (average GPU)\n", query >> 8, query & 0xff);
>         size = sizeof(value);
>         seq_printf(m, "\n");
>
> diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> index 5c840c0..1bec907 100644
> --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> @@ -149,13 +149,6 @@ struct pp_states_info {
>         uint32_t states[16];
>  };
>
> -struct pp_gpu_power {
> -       uint32_t vddc_power;
> -       uint32_t vddci_power;
> -       uint32_t max_gpu_power;
> -       uint32_t average_gpu_power;
> -};
> -
>  #define PP_GROUP_MASK        0xF0000000
>  #define PP_GROUP_SHIFT       28
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> index 20f5a6f..494c1ff 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> @@ -3356,36 +3356,34 @@ static int smu7_get_pp_table_entry(struct pp_hwmgr *hwmgr,
>         return 0;
>  }
>
> -static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
> -               struct pp_gpu_power *query)
> +static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr, u32 *query)
>  {
>         int i;
> +       u32 tmp = 0;
>
>         if (!query)
>                 return -EINVAL;
>
> -
> -       memset(query, 0, sizeof *query);
> -
>         smum_send_msg_to_smc_with_parameter(hwmgr, PPSMC_MSG_GetCurrPkgPwr, 0);
> -       query->average_gpu_power = cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
> +       tmp = cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
>
> -       if (query->average_gpu_power != 0)
> +       if (tmp != 0)
>                 return 0;
>
>         smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogStart);
>         cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC,
>                                                         ixSMU_PM_STATUS_94, 0);
>
> -       for (i = 0; i < 20; i++) {
> +       for (i = 0; i < 10; i++) {
>                 mdelay(1);
>                 smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogSample);
> -               query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
> +               tmp = cgs_read_ind_register(hwmgr->device,
>                                                 CGS_IND_REG__SMC,
>                                                 ixSMU_PM_STATUS_94);
> -               if (query->average_gpu_power != 0)
> +               if (tmp != 0)
>                         break;
>         }
> +       *query = tmp;
>
>         return 0;
>  }
> @@ -3438,10 +3436,7 @@ static int smu7_read_sensor(struct pp_hwmgr *hwmgr, int idx,
>                 *size = 4;
>                 return 0;
>         case AMDGPU_PP_SENSOR_GPU_POWER:
> -               if (*size < sizeof(struct pp_gpu_power))
> -                       return -EINVAL;
> -               *size = sizeof(struct pp_gpu_power);
> -               return smu7_get_gpu_power(hwmgr, (struct pp_gpu_power *)value);
> +               return smu7_get_gpu_power(hwmgr, (uint32_t *)value);
>         case AMDGPU_PP_SENSOR_VDDGFX:
>                 if ((data->vr_config & 0xff) == 0x2)
>                         val_vid = PHM_READ_INDIRECT_FIELD(hwmgr->device,
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> index 127c550..0bbc564 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> @@ -3781,16 +3781,18 @@ static uint32_t vega10_dpm_get_mclk(struct pp_hwmgr *hwmgr, bool low)
>  }
>
>  static int vega10_get_gpu_power(struct pp_hwmgr *hwmgr,
> -               struct pp_gpu_power *query)
> +               uint32_t *query)
>  {
>         uint32_t value;
>
> +       if (!query)
> +               return -EINVAL;
> +
>         smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetCurrPkgPwr);
>         value = smum_get_argument(hwmgr);
>
> -       /* power value is an integer */
> -       memset(query, 0, sizeof *query);
> -       query->average_gpu_power = value << 8;
> +       /* SMC returning actual watts, keep consistent with legacy asics, low 8 bit as 8 fractional bits */
> +       *query = value << 8;
>
>         return 0;
>  }
> @@ -3840,12 +3842,7 @@ static int vega10_read_sensor(struct pp_hwmgr *hwmgr, int idx,
>                 *size = 4;
>                 break;
>         case AMDGPU_PP_SENSOR_GPU_POWER:
> -               if (*size < sizeof(struct pp_gpu_power))
> -                       ret = -EINVAL;
> -               else {
> -                       *size = sizeof(struct pp_gpu_power);
> -                       ret = vega10_get_gpu_power(hwmgr, (struct pp_gpu_power *)value);
> -               }
> +               ret = vega10_get_gpu_power(hwmgr, (uint32_t *)value);
>                 break;
>         case AMDGPU_PP_SENSOR_VDDGFX:
>                 val_vid = (RREG32_SOC15(SMUIO, 0, mmSMUSVI0_PLANE0_CURRENTVID) &
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
> index 3e1ed0a..782e209 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
> @@ -1113,8 +1113,7 @@ static uint32_t vega12_dpm_get_mclk(struct pp_hwmgr *hwmgr, bool low)
>         return (mem_clk * 100);
>  }
>
> -static int vega12_get_gpu_power(struct pp_hwmgr *hwmgr,
> -               struct pp_gpu_power *query)
> +static int vega12_get_gpu_power(struct pp_hwmgr *hwmgr, uint32_t *query)
>  {
>  #if 0
>         uint32_t value;
> @@ -1126,7 +1125,7 @@ static int vega12_get_gpu_power(struct pp_hwmgr *hwmgr,
>
>         vega12_read_arg_from_smc(hwmgr, &value);
>         /* power value is an integer */
> -       query->average_gpu_power = value << 8;
> +       *query = value << 8;
>  #endif
>         return 0;
>  }
> @@ -1235,12 +1234,8 @@ static int vega12_read_sensor(struct pp_hwmgr *hwmgr, int idx,
>                 *size = 4;
>                 break;
>         case AMDGPU_PP_SENSOR_GPU_POWER:
> -               if (*size < sizeof(struct pp_gpu_power))
> -                       ret = -EINVAL;
> -               else {
> -                       *size = sizeof(struct pp_gpu_power);
> -                       ret = vega12_get_gpu_power(hwmgr, (struct pp_gpu_power *)value);
> -               }
> +               ret = vega12_get_gpu_power(hwmgr, (uint32_t *)value);
> +
>                 break;
>         default:
>                 ret = -EINVAL;
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/3] drm/amd/pp: Remove struct pp_gpu_power

[Tom St Denis]

On 04/11/2018 01:23 PM, Alex Deucher wrote:
> On Wed, Apr 11, 2018 at 2:31 AM, Rex Zhu <Rex.Zhu@amd.com> wrote:
>> This struct can't be used for all asics.
>>
>> Currently smu only calculate average gpu power in real time.
>>
>> for vddc/vddci/max power,
>> we can read them per 1ms through some registers. but
>>
>> 1. the type of return values is not unified.
>>      For Vi, return type is uint. For vega, return type is float.
>>
>> 2. need to assign the unit time to calculate the average power.
>>
>> so remove this struct.
>>
>> if user need to know the power on vddc/vddci.
>> we can export them with new common interface/struct.
>>
> 
> If Tom and Eric are ok with this:
> Acked-by: Alex Deucher <alexander.deucher@amd.com>

It'll break the sensor reading in umr but that's fairly minor and easy 
to fix.  I don't really consider the --top output of umr as mission 
critical (even though it can be handy).  I'm ok with the change.

Once this lands on drm-next I'll upgrade umr to support it.

Cheers,
Tom


> 
>> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c            |  7 ++-----
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c             | 22 +++++++--------------
>>   drivers/gpu/drm/amd/include/kgd_pp_interface.h     |  7 -------
>>   drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c   | 23 +++++++++-------------
>>   drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 17 +++++++---------
>>   drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c | 13 ++++--------
>>   6 files changed, 29 insertions(+), 60 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index 487d39e..1e59818 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -701,9 +701,6 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>>                  }
>>          }
>>          case AMDGPU_INFO_SENSOR: {
>> -               struct pp_gpu_power query = {0};
>> -               int query_size = sizeof(query);
>> -
>>                  if (!adev->pm.dpm_enabled)
>>                          return -ENOENT;
>>
>> @@ -746,10 +743,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>>                          /* get average GPU power */
>>                          if (amdgpu_dpm_read_sensor(adev,
>>                                                     AMDGPU_PP_SENSOR_GPU_POWER,
>> -                                                  (void *)&query, &query_size)) {
>> +                                                  (void *)&ui32, &ui32_size)) {
>>                                  return -EINVAL;
>>                          }
>> -                       ui32 = query.average_gpu_power >> 8;
>> +                       ui32 >>= 8;
>>                          break;
>>                  case AMDGPU_INFO_SENSOR_VDDNB:
>>                          /* get VDDNB in millivolts */
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>> index e5f60fc..744f105 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>> @@ -1020,8 +1020,8 @@ static ssize_t amdgpu_hwmon_show_power_avg(struct device *dev,
>>   {
>>          struct amdgpu_device *adev = dev_get_drvdata(dev);
>>          struct drm_device *ddev = adev->ddev;
>> -       struct pp_gpu_power query = {0};
>> -       int r, size = sizeof(query);
>> +       u32 query = 0;
>> +       int r, size = sizeof(u32);
>>          unsigned uw;
>>
>>          /* Can't get power when the card is off */
>> @@ -1041,7 +1041,7 @@ static ssize_t amdgpu_hwmon_show_power_avg(struct device *dev,
>>                  return r;
>>
>>          /* convert to microwatts */
>> -       uw = (query.average_gpu_power >> 8) * 1000000;
>> +       uw = (query >> 8) * 1000000 + (query & 0xff) * 1000;
>>
>>          return snprintf(buf, PAGE_SIZE, "%u\n", uw);
>>   }
>> @@ -1752,7 +1752,7 @@ void amdgpu_pm_compute_clocks(struct amdgpu_device *adev)
>>   static int amdgpu_debugfs_pm_info_pp(struct seq_file *m, struct amdgpu_device *adev)
>>   {
>>          uint32_t value;
>> -       struct pp_gpu_power query = {0};
>> +       uint32_t query = 0;
>>          int size;
>>
>>          /* sanity check PP is enabled */
>> @@ -1775,17 +1775,9 @@ static int amdgpu_debugfs_pm_info_pp(struct seq_file *m, struct amdgpu_device *a
>>                  seq_printf(m, "\t%u mV (VDDGFX)\n", value);
>>          if (!amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_VDDNB, (void *)&value, &size))
>>                  seq_printf(m, "\t%u mV (VDDNB)\n", value);
>> -       size = sizeof(query);
>> -       if (!amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GPU_POWER, (void *)&query, &size)) {
>> -               seq_printf(m, "\t%u.%u W (VDDC)\n", query.vddc_power >> 8,
>> -                               query.vddc_power & 0xff);
>> -               seq_printf(m, "\t%u.%u W (VDDCI)\n", query.vddci_power >> 8,
>> -                               query.vddci_power & 0xff);
>> -               seq_printf(m, "\t%u.%u W (max GPU)\n", query.max_gpu_power >> 8,
>> -                               query.max_gpu_power & 0xff);
>> -               seq_printf(m, "\t%u.%u W (average GPU)\n", query.average_gpu_power >> 8,
>> -                               query.average_gpu_power & 0xff);
>> -       }
>> +       size = sizeof(uint32_t);
>> +       if (!amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GPU_POWER, (void *)&query, &size))
>> +               seq_printf(m, "\t%u.%u W (average GPU)\n", query >> 8, query & 0xff);
>>          size = sizeof(value);
>>          seq_printf(m, "\n");
>>
>> diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>> index 5c840c0..1bec907 100644
>> --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>> +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>> @@ -149,13 +149,6 @@ struct pp_states_info {
>>          uint32_t states[16];
>>   };
>>
>> -struct pp_gpu_power {
>> -       uint32_t vddc_power;
>> -       uint32_t vddci_power;
>> -       uint32_t max_gpu_power;
>> -       uint32_t average_gpu_power;
>> -};
>> -
>>   #define PP_GROUP_MASK        0xF0000000
>>   #define PP_GROUP_SHIFT       28
>>
>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> index 20f5a6f..494c1ff 100644
>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> @@ -3356,36 +3356,34 @@ static int smu7_get_pp_table_entry(struct pp_hwmgr *hwmgr,
>>          return 0;
>>   }
>>
>> -static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
>> -               struct pp_gpu_power *query)
>> +static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr, u32 *query)
>>   {
>>          int i;
>> +       u32 tmp = 0;
>>
>>          if (!query)
>>                  return -EINVAL;
>>
>> -
>> -       memset(query, 0, sizeof *query);
>> -
>>          smum_send_msg_to_smc_with_parameter(hwmgr, PPSMC_MSG_GetCurrPkgPwr, 0);
>> -       query->average_gpu_power = cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
>> +       tmp = cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
>>
>> -       if (query->average_gpu_power != 0)
>> +       if (tmp != 0)
>>                  return 0;
>>
>>          smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogStart);
>>          cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC,
>>                                                          ixSMU_PM_STATUS_94, 0);
>>
>> -       for (i = 0; i < 20; i++) {
>> +       for (i = 0; i < 10; i++) {
>>                  mdelay(1);
>>                  smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogSample);
>> -               query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
>> +               tmp = cgs_read_ind_register(hwmgr->device,
>>                                                  CGS_IND_REG__SMC,
>>                                                  ixSMU_PM_STATUS_94);
>> -               if (query->average_gpu_power != 0)
>> +               if (tmp != 0)
>>                          break;
>>          }
>> +       *query = tmp;
>>
>>          return 0;
>>   }
>> @@ -3438,10 +3436,7 @@ static int smu7_read_sensor(struct pp_hwmgr *hwmgr, int idx,
>>                  *size = 4;
>>                  return 0;
>>          case AMDGPU_PP_SENSOR_GPU_POWER:
>> -               if (*size < sizeof(struct pp_gpu_power))
>> -                       return -EINVAL;
>> -               *size = sizeof(struct pp_gpu_power);
>> -               return smu7_get_gpu_power(hwmgr, (struct pp_gpu_power *)value);
>> +               return smu7_get_gpu_power(hwmgr, (uint32_t *)value);
>>          case AMDGPU_PP_SENSOR_VDDGFX:
>>                  if ((data->vr_config & 0xff) == 0x2)
>>                          val_vid = PHM_READ_INDIRECT_FIELD(hwmgr->device,
>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
>> index 127c550..0bbc564 100644
>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
>> @@ -3781,16 +3781,18 @@ static uint32_t vega10_dpm_get_mclk(struct pp_hwmgr *hwmgr, bool low)
>>   }
>>
>>   static int vega10_get_gpu_power(struct pp_hwmgr *hwmgr,
>> -               struct pp_gpu_power *query)
>> +               uint32_t *query)
>>   {
>>          uint32_t value;
>>
>> +       if (!query)
>> +               return -EINVAL;
>> +
>>          smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetCurrPkgPwr);
>>          value = smum_get_argument(hwmgr);
>>
>> -       /* power value is an integer */
>> -       memset(query, 0, sizeof *query);
>> -       query->average_gpu_power = value << 8;
>> +       /* SMC returning actual watts, keep consistent with legacy asics, low 8 bit as 8 fractional bits */
>> +       *query = value << 8;
>>
>>          return 0;
>>   }
>> @@ -3840,12 +3842,7 @@ static int vega10_read_sensor(struct pp_hwmgr *hwmgr, int idx,
>>                  *size = 4;
>>                  break;
>>          case AMDGPU_PP_SENSOR_GPU_POWER:
>> -               if (*size < sizeof(struct pp_gpu_power))
>> -                       ret = -EINVAL;
>> -               else {
>> -                       *size = sizeof(struct pp_gpu_power);
>> -                       ret = vega10_get_gpu_power(hwmgr, (struct pp_gpu_power *)value);
>> -               }
>> +               ret = vega10_get_gpu_power(hwmgr, (uint32_t *)value);
>>                  break;
>>          case AMDGPU_PP_SENSOR_VDDGFX:
>>                  val_vid = (RREG32_SOC15(SMUIO, 0, mmSMUSVI0_PLANE0_CURRENTVID) &
>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
>> index 3e1ed0a..782e209 100644
>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
>> @@ -1113,8 +1113,7 @@ static uint32_t vega12_dpm_get_mclk(struct pp_hwmgr *hwmgr, bool low)
>>          return (mem_clk * 100);
>>   }
>>
>> -static int vega12_get_gpu_power(struct pp_hwmgr *hwmgr,
>> -               struct pp_gpu_power *query)
>> +static int vega12_get_gpu_power(struct pp_hwmgr *hwmgr, uint32_t *query)
>>   {
>>   #if 0
>>          uint32_t value;
>> @@ -1126,7 +1125,7 @@ static int vega12_get_gpu_power(struct pp_hwmgr *hwmgr,
>>
>>          vega12_read_arg_from_smc(hwmgr, &value);
>>          /* power value is an integer */
>> -       query->average_gpu_power = value << 8;
>> +       *query = value << 8;
>>   #endif
>>          return 0;
>>   }
>> @@ -1235,12 +1234,8 @@ static int vega12_read_sensor(struct pp_hwmgr *hwmgr, int idx,
>>                  *size = 4;
>>                  break;
>>          case AMDGPU_PP_SENSOR_GPU_POWER:
>> -               if (*size < sizeof(struct pp_gpu_power))
>> -                       ret = -EINVAL;
>> -               else {
>> -                       *size = sizeof(struct pp_gpu_power);
>> -                       ret = vega12_get_gpu_power(hwmgr, (struct pp_gpu_power *)value);
>> -               }
>> +               ret = vega12_get_gpu_power(hwmgr, (uint32_t *)value);
>> +
>>                  break;
>>          default:
>>                  ret = -EINVAL;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/3] drm/amd/pp: Remove struct pp_gpu_power

[Eric Huang]

I am OK with this change.

Regards,
Eric

On 2018-04-11 01:25 PM, Tom St Denis wrote:
>
>
> On 04/11/2018 01:23 PM, Alex Deucher wrote:
>> On Wed, Apr 11, 2018 at 2:31 AM, Rex Zhu <Rex.Zhu@amd.com> wrote:
>>> This struct can't be used for all asics.
>>>
>>> Currently smu only calculate average gpu power in real time.
>>>
>>> for vddc/vddci/max power,
>>> we can read them per 1ms through some registers. but
>>>
>>> 1. the type of return values is not unified.
>>>      For Vi, return type is uint. For vega, return type is float.
>>>
>>> 2. need to assign the unit time to calculate the average power.
>>>
>>> so remove this struct.
>>>
>>> if user need to know the power on vddc/vddci.
>>> we can export them with new common interface/struct.
>>>
>>
>> If Tom and Eric are ok with this:
>> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>
> It'll break the sensor reading in umr but that's fairly minor and easy 
> to fix.  I don't really consider the --top output of umr as mission 
> critical (even though it can be handy).  I'm ok with the change.
>
> Once this lands on drm-next I'll upgrade umr to support it.
>
> Cheers,
> Tom
>
>
>>
>>> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c            |  7 ++-----
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c             | 22 
>>> +++++++--------------
>>>   drivers/gpu/drm/amd/include/kgd_pp_interface.h     |  7 -------
>>>   drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c   | 23 
>>> +++++++++-------------
>>>   drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 17 
>>> +++++++---------
>>>   drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c | 13 ++++--------
>>>   6 files changed, 29 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> index 487d39e..1e59818 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> @@ -701,9 +701,6 @@ static int amdgpu_info_ioctl(struct drm_device 
>>> *dev, void *data, struct drm_file
>>>                  }
>>>          }
>>>          case AMDGPU_INFO_SENSOR: {
>>> -               struct pp_gpu_power query = {0};
>>> -               int query_size = sizeof(query);
>>> -
>>>                  if (!adev->pm.dpm_enabled)
>>>                          return -ENOENT;
>>>
>>> @@ -746,10 +743,10 @@ static int amdgpu_info_ioctl(struct drm_device 
>>> *dev, void *data, struct drm_file
>>>                          /* get average GPU power */
>>>                          if (amdgpu_dpm_read_sensor(adev,
>>> AMDGPU_PP_SENSOR_GPU_POWER,
>>> -                                                  (void *)&query, 
>>> &query_size)) {
>>> +                                                  (void *)&ui32, 
>>> &ui32_size)) {
>>>                                  return -EINVAL;
>>>                          }
>>> -                       ui32 = query.average_gpu_power >> 8;
>>> +                       ui32 >>= 8;
>>>                          break;
>>>                  case AMDGPU_INFO_SENSOR_VDDNB:
>>>                          /* get VDDNB in millivolts */
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>>> index e5f60fc..744f105 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>>> @@ -1020,8 +1020,8 @@ static ssize_t 
>>> amdgpu_hwmon_show_power_avg(struct device *dev,
>>>   {
>>>          struct amdgpu_device *adev = dev_get_drvdata(dev);
>>>          struct drm_device *ddev = adev->ddev;
>>> -       struct pp_gpu_power query = {0};
>>> -       int r, size = sizeof(query);
>>> +       u32 query = 0;
>>> +       int r, size = sizeof(u32);
>>>          unsigned uw;
>>>
>>>          /* Can't get power when the card is off */
>>> @@ -1041,7 +1041,7 @@ static ssize_t 
>>> amdgpu_hwmon_show_power_avg(struct device *dev,
>>>                  return r;
>>>
>>>          /* convert to microwatts */
>>> -       uw = (query.average_gpu_power >> 8) * 1000000;
>>> +       uw = (query >> 8) * 1000000 + (query & 0xff) * 1000;
>>>
>>>          return snprintf(buf, PAGE_SIZE, "%u\n", uw);
>>>   }
>>> @@ -1752,7 +1752,7 @@ void amdgpu_pm_compute_clocks(struct 
>>> amdgpu_device *adev)
>>>   static int amdgpu_debugfs_pm_info_pp(struct seq_file *m, struct 
>>> amdgpu_device *adev)
>>>   {
>>>          uint32_t value;
>>> -       struct pp_gpu_power query = {0};
>>> +       uint32_t query = 0;
>>>          int size;
>>>
>>>          /* sanity check PP is enabled */
>>> @@ -1775,17 +1775,9 @@ static int amdgpu_debugfs_pm_info_pp(struct 
>>> seq_file *m, struct amdgpu_device *a
>>>                  seq_printf(m, "\t%u mV (VDDGFX)\n", value);
>>>          if (!amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_VDDNB, 
>>> (void *)&value, &size))
>>>                  seq_printf(m, "\t%u mV (VDDNB)\n", value);
>>> -       size = sizeof(query);
>>> -       if (!amdgpu_dpm_read_sensor(adev, 
>>> AMDGPU_PP_SENSOR_GPU_POWER, (void *)&query, &size)) {
>>> -               seq_printf(m, "\t%u.%u W (VDDC)\n", query.vddc_power 
>>> >> 8,
>>> -                               query.vddc_power & 0xff);
>>> -               seq_printf(m, "\t%u.%u W (VDDCI)\n", 
>>> query.vddci_power >> 8,
>>> -                               query.vddci_power & 0xff);
>>> -               seq_printf(m, "\t%u.%u W (max GPU)\n", 
>>> query.max_gpu_power >> 8,
>>> -                               query.max_gpu_power & 0xff);
>>> -               seq_printf(m, "\t%u.%u W (average GPU)\n", 
>>> query.average_gpu_power >> 8,
>>> -                               query.average_gpu_power & 0xff);
>>> -       }
>>> +       size = sizeof(uint32_t);
>>> +       if (!amdgpu_dpm_read_sensor(adev, 
>>> AMDGPU_PP_SENSOR_GPU_POWER, (void *)&query, &size))
>>> +               seq_printf(m, "\t%u.%u W (average GPU)\n", query >> 
>>> 8, query & 0xff);
>>>          size = sizeof(value);
>>>          seq_printf(m, "\n");
>>>
>>> diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h 
>>> b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>>> index 5c840c0..1bec907 100644
>>> --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>>> +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>>> @@ -149,13 +149,6 @@ struct pp_states_info {
>>>          uint32_t states[16];
>>>   };
>>>
>>> -struct pp_gpu_power {
>>> -       uint32_t vddc_power;
>>> -       uint32_t vddci_power;
>>> -       uint32_t max_gpu_power;
>>> -       uint32_t average_gpu_power;
>>> -};
>>> -
>>>   #define PP_GROUP_MASK        0xF0000000
>>>   #define PP_GROUP_SHIFT       28
>>>
>>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c 
>>> b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>>> index 20f5a6f..494c1ff 100644
>>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>>> @@ -3356,36 +3356,34 @@ static int smu7_get_pp_table_entry(struct 
>>> pp_hwmgr *hwmgr,
>>>          return 0;
>>>   }
>>>
>>> -static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
>>> -               struct pp_gpu_power *query)
>>> +static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr, u32 *query)
>>>   {
>>>          int i;
>>> +       u32 tmp = 0;
>>>
>>>          if (!query)
>>>                  return -EINVAL;
>>>
>>> -
>>> -       memset(query, 0, sizeof *query);
>>> -
>>>          smum_send_msg_to_smc_with_parameter(hwmgr, 
>>> PPSMC_MSG_GetCurrPkgPwr, 0);
>>> -       query->average_gpu_power = cgs_read_register(hwmgr->device, 
>>> mmSMC_MSG_ARG_0);
>>> +       tmp = cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
>>>
>>> -       if (query->average_gpu_power != 0)
>>> +       if (tmp != 0)
>>>                  return 0;
>>>
>>>          smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogStart);
>>>          cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC,
>>> ixSMU_PM_STATUS_94, 0);
>>>
>>> -       for (i = 0; i < 20; i++) {
>>> +       for (i = 0; i < 10; i++) {
>>>                  mdelay(1);
>>>                  smum_send_msg_to_smc(hwmgr, 
>>> PPSMC_MSG_PmStatusLogSample);
>>> -               query->average_gpu_power = 
>>> cgs_read_ind_register(hwmgr->device,
>>> +               tmp = cgs_read_ind_register(hwmgr->device,
>>> CGS_IND_REG__SMC,
>>> ixSMU_PM_STATUS_94);
>>> -               if (query->average_gpu_power != 0)
>>> +               if (tmp != 0)
>>>                          break;
>>>          }
>>> +       *query = tmp;
>>>
>>>          return 0;
>>>   }
>>> @@ -3438,10 +3436,7 @@ static int smu7_read_sensor(struct pp_hwmgr 
>>> *hwmgr, int idx,
>>>                  *size = 4;
>>>                  return 0;
>>>          case AMDGPU_PP_SENSOR_GPU_POWER:
>>> -               if (*size < sizeof(struct pp_gpu_power))
>>> -                       return -EINVAL;
>>> -               *size = sizeof(struct pp_gpu_power);
>>> -               return smu7_get_gpu_power(hwmgr, (struct 
>>> pp_gpu_power *)value);
>>> +               return smu7_get_gpu_power(hwmgr, (uint32_t *)value);
>>>          case AMDGPU_PP_SENSOR_VDDGFX:
>>>                  if ((data->vr_config & 0xff) == 0x2)
>>>                          val_vid = 
>>> PHM_READ_INDIRECT_FIELD(hwmgr->device,
>>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c 
>>> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
>>> index 127c550..0bbc564 100644
>>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
>>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
>>> @@ -3781,16 +3781,18 @@ static uint32_t vega10_dpm_get_mclk(struct 
>>> pp_hwmgr *hwmgr, bool low)
>>>   }
>>>
>>>   static int vega10_get_gpu_power(struct pp_hwmgr *hwmgr,
>>> -               struct pp_gpu_power *query)
>>> +               uint32_t *query)
>>>   {
>>>          uint32_t value;
>>>
>>> +       if (!query)
>>> +               return -EINVAL;
>>> +
>>>          smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetCurrPkgPwr);
>>>          value = smum_get_argument(hwmgr);
>>>
>>> -       /* power value is an integer */
>>> -       memset(query, 0, sizeof *query);
>>> -       query->average_gpu_power = value << 8;
>>> +       /* SMC returning actual watts, keep consistent with legacy 
>>> asics, low 8 bit as 8 fractional bits */
>>> +       *query = value << 8;
>>>
>>>          return 0;
>>>   }
>>> @@ -3840,12 +3842,7 @@ static int vega10_read_sensor(struct pp_hwmgr 
>>> *hwmgr, int idx,
>>>                  *size = 4;
>>>                  break;
>>>          case AMDGPU_PP_SENSOR_GPU_POWER:
>>> -               if (*size < sizeof(struct pp_gpu_power))
>>> -                       ret = -EINVAL;
>>> -               else {
>>> -                       *size = sizeof(struct pp_gpu_power);
>>> -                       ret = vega10_get_gpu_power(hwmgr, (struct 
>>> pp_gpu_power *)value);
>>> -               }
>>> +               ret = vega10_get_gpu_power(hwmgr, (uint32_t *)value);
>>>                  break;
>>>          case AMDGPU_PP_SENSOR_VDDGFX:
>>>                  val_vid = (RREG32_SOC15(SMUIO, 0, 
>>> mmSMUSVI0_PLANE0_CURRENTVID) &
>>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c 
>>> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
>>> index 3e1ed0a..782e209 100644
>>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
>>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
>>> @@ -1113,8 +1113,7 @@ static uint32_t vega12_dpm_get_mclk(struct 
>>> pp_hwmgr *hwmgr, bool low)
>>>          return (mem_clk * 100);
>>>   }
>>>
>>> -static int vega12_get_gpu_power(struct pp_hwmgr *hwmgr,
>>> -               struct pp_gpu_power *query)
>>> +static int vega12_get_gpu_power(struct pp_hwmgr *hwmgr, uint32_t 
>>> *query)
>>>   {
>>>   #if 0
>>>          uint32_t value;
>>> @@ -1126,7 +1125,7 @@ static int vega12_get_gpu_power(struct 
>>> pp_hwmgr *hwmgr,
>>>
>>>          vega12_read_arg_from_smc(hwmgr, &value);
>>>          /* power value is an integer */
>>> -       query->average_gpu_power = value << 8;
>>> +       *query = value << 8;
>>>   #endif
>>>          return 0;
>>>   }
>>> @@ -1235,12 +1234,8 @@ static int vega12_read_sensor(struct pp_hwmgr 
>>> *hwmgr, int idx,
>>>                  *size = 4;
>>>                  break;
>>>          case AMDGPU_PP_SENSOR_GPU_POWER:
>>> -               if (*size < sizeof(struct pp_gpu_power))
>>> -                       ret = -EINVAL;
>>> -               else {
>>> -                       *size = sizeof(struct pp_gpu_power);
>>> -                       ret = vega12_get_gpu_power(hwmgr, (struct 
>>> pp_gpu_power *)value);
>>> -               }
>>> +               ret = vega12_get_gpu_power(hwmgr, (uint32_t *)value);
>>> +
>>>                  break;
>>>          default:
>>>                  ret = -EINVAL;
>>> -- 
>>> 1.9.1
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] Revert "drm/amd/powerply: fix power reading on Fiji"

[Alex Deucher]

On Wed, Apr 11, 2018 at 2:31 AM, Rex Zhu <Rex.Zhu@amd.com> wrote:
> we don't have limit of [50ms, 4sec] sampling period.
> smu calculate average gpu power in real time.
> we can read average gpu power through smu message or
> read special register.
>
> This reverts commit 462d8dcc9fec0d89f1ff6a1f93f1d4f670878c71.
>
> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>

If Eric is ok with this,

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> index 21c021b..388184e 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> @@ -3364,8 +3364,7 @@ static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
>                         "Failed to start pm status log!",
>                         return -1);
>
> -       /* Sampling period from 50ms to 4sec */
> -       msleep_interruptible(200);
> +       msleep_interruptible(20);
>
>         PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
>                         PPSMC_MSG_PmStatusLogSample),
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] Revert "drm/amd/powerply: fix power reading on Fiji"

[Eric Huang]

If it is verified by SMU team, I am OK with it.

Regards,
Eric

On 2018-04-11 01:19 PM, Alex Deucher wrote:
> On Wed, Apr 11, 2018 at 2:31 AM, Rex Zhu <Rex.Zhu@amd.com> wrote:
>> we don't have limit of [50ms, 4sec] sampling period.
>> smu calculate average gpu power in real time.
>> we can read average gpu power through smu message or
>> read special register.
>>
>> This reverts commit 462d8dcc9fec0d89f1ff6a1f93f1d4f670878c71.
>>
>> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> If Eric is ok with this,
>
> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>
>> ---
>>   drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> index 21c021b..388184e 100644
>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> @@ -3364,8 +3364,7 @@ static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
>>                          "Failed to start pm status log!",
>>                          return -1);
>>
>> -       /* Sampling period from 50ms to 4sec */
>> -       msleep_interruptible(200);
>> +       msleep_interruptible(20);
>>
>>          PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
>>                          PPSMC_MSG_PmStatusLogSample),
>> --
>> 1.9.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/3] Revert "drm/amd/powerply: fix power reading on Fiji"

[Rex Zhu]

we don't have limit of [50ms, 4sec] sampling period.
only confused the average power and max power.
so don't need to increase delay time.

This reverts commit 462d8dcc9fec0d89f1ff6a1f93f1d4f670878c71.

Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
index aaa9f5b..40f2f87 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
@@ -3368,8 +3368,7 @@ static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
 			"Failed to start pm status log!",
 			return -1);
 
-	/* Sampling period from 50ms to 4sec */
-	msleep_interruptible(200);
+	msleep_interruptible(20);
 
 	PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
 			PPSMC_MSG_PmStatusLogSample),
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/3] drm/amd/pp: Refine get_gpu_power for VI

[Rex Zhu]

1. On polaris10/11/12, Sending smu message PPSMC_MSG_GetCurrPkgPwr to
   read currentpkgpwr directly.
2. On Fiji/tonga/bonaire/hawwii, no PPSMC_MSG_GetCurrPkgPwr support.
   Send PPSMC_MSG_PmStatusLogStart to let smu write currentpkgpwr
   to ixSMU_PM_STATUS_94. this is asynchronous. need to delay no less
   than 1ms.
3. Clean ixSMU_PM_STATUS_94 immediately when send PPSMC_MSG_PmStatusLogStart
   to avoid read old value.
4. delay 10 ms instand of 20 ms. so the result will more similar to
   the output of PPSMC_MSG_GetCurrPkgPwr.
5. remove max power/vddc/vddci output to keep consistent with vega.
6. for vddc/vddci power, we can calculate the average value per
   [10ms, 4s] in other interface if needed.

Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 45 +++++++++++-------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
index 40f2f87..c0ce672 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
@@ -3363,30 +3363,27 @@ static int smu7_get_pp_table_entry(struct pp_hwmgr *hwmgr,
 static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
 		struct pp_gpu_power *query)
 {
-	PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
-			PPSMC_MSG_PmStatusLogStart),
-			"Failed to start pm status log!",
-			return -1);
-
-	msleep_interruptible(20);
-
-	PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
-			PPSMC_MSG_PmStatusLogSample),
-			"Failed to sample pm status log!",
-			return -1);
-
-	query->vddc_power = cgs_read_ind_register(hwmgr->device,
-			CGS_IND_REG__SMC,
-			ixSMU_PM_STATUS_40);
-	query->vddci_power = cgs_read_ind_register(hwmgr->device,
-			CGS_IND_REG__SMC,
-			ixSMU_PM_STATUS_49);
-	query->max_gpu_power = cgs_read_ind_register(hwmgr->device,
-			CGS_IND_REG__SMC,
-			ixSMU_PM_STATUS_94);
-	query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
-			CGS_IND_REG__SMC,
-			ixSMU_PM_STATUS_95);
+	if (!query)
+		return -EINVAL;
+
+	memset(query, 0, sizeof *query);
+
+	if (hwmgr->chip_id >= CHIP_POLARIS10) {
+		smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetCurrPkgPwr);
+		query->average_gpu_power = cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
+	} else {
+		smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogStart);
+		cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC,
+					ixSMU_PM_STATUS_94, 0);
+
+		msleep_interruptible(10);
+
+		smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogSample);
+
+		query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
+							CGS_IND_REG__SMC,
+							ixSMU_PM_STATUS_94);
+	}
 
 	return 0;
 }
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amd/pp: Refine get_gpu_power for VI

[Eric Huang]

Sampling period is too short. The power reading value will be not 
aligned with AGT's. It will confuse user that why AMD provides two 
different power results.

Regards,
Eric

On 2018-04-04 04:25 AM, Rex Zhu wrote:
> 1. On polaris10/11/12, Sending smu message PPSMC_MSG_GetCurrPkgPwr to
>     read currentpkgpwr directly.
> 2. On Fiji/tonga/bonaire/hawwii, no PPSMC_MSG_GetCurrPkgPwr support.
>     Send PPSMC_MSG_PmStatusLogStart to let smu write currentpkgpwr
>     to ixSMU_PM_STATUS_94. this is asynchronous. need to delay no less
>     than 1ms.
> 3. Clean ixSMU_PM_STATUS_94 immediately when send PPSMC_MSG_PmStatusLogStart
>     to avoid read old value.
> 4. delay 10 ms instand of 20 ms. so the result will more similar to
>     the output of PPSMC_MSG_GetCurrPkgPwr.
> 5. remove max power/vddc/vddci output to keep consistent with vega.
> 6. for vddc/vddci power, we can calculate the average value per
>     [10ms, 4s] in other interface if needed.
>
> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> ---
>   drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 45 +++++++++++-------------
>   1 file changed, 21 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> index 40f2f87..c0ce672 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> @@ -3363,30 +3363,27 @@ static int smu7_get_pp_table_entry(struct pp_hwmgr *hwmgr,
>   static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
>   		struct pp_gpu_power *query)
>   {
> -	PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> -			PPSMC_MSG_PmStatusLogStart),
> -			"Failed to start pm status log!",
> -			return -1);
> -
> -	msleep_interruptible(20);
> -
> -	PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> -			PPSMC_MSG_PmStatusLogSample),
> -			"Failed to sample pm status log!",
> -			return -1);
> -
> -	query->vddc_power = cgs_read_ind_register(hwmgr->device,
> -			CGS_IND_REG__SMC,
> -			ixSMU_PM_STATUS_40);
> -	query->vddci_power = cgs_read_ind_register(hwmgr->device,
> -			CGS_IND_REG__SMC,
> -			ixSMU_PM_STATUS_49);
> -	query->max_gpu_power = cgs_read_ind_register(hwmgr->device,
> -			CGS_IND_REG__SMC,
> -			ixSMU_PM_STATUS_94);
> -	query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
> -			CGS_IND_REG__SMC,
> -			ixSMU_PM_STATUS_95);
> +	if (!query)
> +		return -EINVAL;
> +
> +	memset(query, 0, sizeof *query);
> +
> +	if (hwmgr->chip_id >= CHIP_POLARIS10) {
> +		smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetCurrPkgPwr);
> +		query->average_gpu_power = cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
> +	} else {
> +		smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogStart);
> +		cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC,
> +					ixSMU_PM_STATUS_94, 0);
> +
> +		msleep_interruptible(10);
> +
> +		smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogSample);
> +
> +		query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
> +							CGS_IND_REG__SMC,
> +							ixSMU_PM_STATUS_94);
> +	}
>   
>   	return 0;
>   }

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amd/pp: Refine get_gpu_power for VI

[Zhu, Rex]

[-- Attachment #1.1: Type: text/plain, Size: 5060 bytes --]

If user send message PPSMC_MSG_GetCurrPkgPwr, firmware will return the pkgpwr immediately as current power value.

as no PPSMC_MSG_GetCurrPkgPwr support, so send message  let firmware write pkgpwr  to ixSMU_PM_STATUS_94,

and driver delay 10ms to read ixSMU_PM_STATUS_94.


I don't think there is any problem. otherwise, there is same issue on polaris/vega.


I clean ixSMU_PM_STATUS_94 before let smu write it.

The delay time is enough unless we got 0 from ixSMU_PM_STATUS_94 .



Best Regards

Rex


________________________________
From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of Eric Huang <jinhuieric.huang-5C7GfCeVMHo@public.gmane.org>
Sent: Wednesday, April 4, 2018 11:36 PM
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH 2/3] drm/amd/pp: Refine get_gpu_power for VI

Sampling period is too short. The power reading value will be not
aligned with AGT's. It will confuse user that why AMD provides two
different power results.

Regards,
Eric

On 2018-04-04 04:25 AM, Rex Zhu wrote:
> 1. On polaris10/11/12, Sending smu message PPSMC_MSG_GetCurrPkgPwr to
>     read currentpkgpwr directly.
> 2. On Fiji/tonga/bonaire/hawwii, no PPSMC_MSG_GetCurrPkgPwr support.
>     Send PPSMC_MSG_PmStatusLogStart to let smu write currentpkgpwr
>     to ixSMU_PM_STATUS_94. this is asynchronous. need to delay no less
>     than 1ms.
> 3. Clean ixSMU_PM_STATUS_94 immediately when send PPSMC_MSG_PmStatusLogStart
>     to avoid read old value.
> 4. delay 10 ms instand of 20 ms. so the result will more similar to
>     the output of PPSMC_MSG_GetCurrPkgPwr.
> 5. remove max power/vddc/vddci output to keep consistent with vega.
> 6. for vddc/vddci power, we can calculate the average value per
>     [10ms, 4s] in other interface if needed.
>
> Signed-off-by: Rex Zhu <Rex.Zhu-5C7GfCeVMHo@public.gmane.org>
> ---
>   drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 45 +++++++++++-------------
>   1 file changed, 21 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> index 40f2f87..c0ce672 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> @@ -3363,30 +3363,27 @@ static int smu7_get_pp_table_entry(struct pp_hwmgr *hwmgr,
>   static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
>                struct pp_gpu_power *query)
>   {
> -     PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> -                     PPSMC_MSG_PmStatusLogStart),
> -                     "Failed to start pm status log!",
> -                     return -1);
> -
> -     msleep_interruptible(20);
> -
> -     PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> -                     PPSMC_MSG_PmStatusLogSample),
> -                     "Failed to sample pm status log!",
> -                     return -1);
> -
> -     query->vddc_power = cgs_read_ind_register(hwmgr->device,
> -                     CGS_IND_REG__SMC,
> -                     ixSMU_PM_STATUS_40);
> -     query->vddci_power = cgs_read_ind_register(hwmgr->device,
> -                     CGS_IND_REG__SMC,
> -                     ixSMU_PM_STATUS_49);
> -     query->max_gpu_power = cgs_read_ind_register(hwmgr->device,
> -                     CGS_IND_REG__SMC,
> -                     ixSMU_PM_STATUS_94);
> -     query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
> -                     CGS_IND_REG__SMC,
> -                     ixSMU_PM_STATUS_95);
> +     if (!query)
> +             return -EINVAL;
> +
> +     memset(query, 0, sizeof *query);
> +
> +     if (hwmgr->chip_id >= CHIP_POLARIS10) {
> +             smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetCurrPkgPwr);
> +             query->average_gpu_power = cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
> +     } else {
> +             smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogStart);
> +             cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC,
> +                                     ixSMU_PM_STATUS_94, 0);
> +
> +             msleep_interruptible(10);
> +
> +             smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogSample);
> +
> +             query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
> +                                                     CGS_IND_REG__SMC,
> +                                                     ixSMU_PM_STATUS_94);
> +     }
>
>        return 0;
>   }

_______________________________________________
amd-gfx mailing list
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
amd-gfx Info Page - freedesktop.org<https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
lists.freedesktop.org
Subscribing to amd-gfx: Subscribe to amd-gfx by filling out the following form. Use of all freedesktop.org lists is subject to our Code of Conduct.




[-- Attachment #1.2: Type: text/html, Size: 15722 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amd/pp: Refine get_gpu_power for VI

[Eric Huang]

[-- Attachment #1.1: Type: text/plain, Size: 5631 bytes --]

So you didn't compare the result with AGT, but I did. The reality is not 
like your speculation.


Regards,

Eric


On 2018-04-04 11:50 AM, Zhu, Rex wrote:
>
> If user send message PPSMC_MSG_GetCurrPkgPwr, firmware will return the 
> pkgpwr immediately as current power value.
>
> as no PPSMC_MSG_GetCurrPkgPwr support, so send message  let firmware 
> write pkgpwr  to ixSMU_PM_STATUS_94,
>
> and driver delay 10ms to read ixSMU_PM_STATUS_94.
>
>
> I don't think there is any problem. otherwise, there is same issue on 
> polaris/vega.
>
>
> I clean ixSMU_PM_STATUS_94 before let smu write it.
>
> The delay time is enough unless we got 0 from ixSMU_PM_STATUS_94 .
>
>
>
> Best Regards
>
> Rex
>
>
>
> ------------------------------------------------------------------------
> *From:* amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of 
> Eric Huang <jinhuieric.huang-5C7GfCeVMHo@public.gmane.org>
> *Sent:* Wednesday, April 4, 2018 11:36 PM
> *To:* amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> *Subject:* Re: [PATCH 2/3] drm/amd/pp: Refine get_gpu_power for VI
> Sampling period is too short. The power reading value will be not
> aligned with AGT's. It will confuse user that why AMD provides two
> different power results.
>
> Regards,
> Eric
>
> On 2018-04-04 04:25 AM, Rex Zhu wrote:
> > 1. On polaris10/11/12, Sending smu message PPSMC_MSG_GetCurrPkgPwr to
> >     read currentpkgpwr directly.
> > 2. On Fiji/tonga/bonaire/hawwii, no PPSMC_MSG_GetCurrPkgPwr support.
> >     Send PPSMC_MSG_PmStatusLogStart to let smu write currentpkgpwr
> >     to ixSMU_PM_STATUS_94. this is asynchronous. need to delay no less
> >     than 1ms.
> > 3. Clean ixSMU_PM_STATUS_94 immediately when send 
> PPSMC_MSG_PmStatusLogStart
> >     to avoid read old value.
> > 4. delay 10 ms instand of 20 ms. so the result will more similar to
> >     the output of PPSMC_MSG_GetCurrPkgPwr.
> > 5. remove max power/vddc/vddci output to keep consistent with vega.
> > 6. for vddc/vddci power, we can calculate the average value per
> >     [10ms, 4s] in other interface if needed.
> >
> > Signed-off-by: Rex Zhu <Rex.Zhu-5C7GfCeVMHo@public.gmane.org>
> > ---
> > drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 45 
> +++++++++++-------------
> >   1 file changed, 21 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c 
> b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> > index 40f2f87..c0ce672 100644
> > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> > @@ -3363,30 +3363,27 @@ static int smu7_get_pp_table_entry(struct 
> pp_hwmgr *hwmgr,
> >   static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
> >                struct pp_gpu_power *query)
> >   {
> > - PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> > - PPSMC_MSG_PmStatusLogStart),
> > -                     "Failed to start pm status log!",
> > -                     return -1);
> > -
> > -     msleep_interruptible(20);
> > -
> > - PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> > - PPSMC_MSG_PmStatusLogSample),
> > -                     "Failed to sample pm status log!",
> > -                     return -1);
> > -
> > -     query->vddc_power = cgs_read_ind_register(hwmgr->device,
> > -                     CGS_IND_REG__SMC,
> > -                     ixSMU_PM_STATUS_40);
> > -     query->vddci_power = cgs_read_ind_register(hwmgr->device,
> > -                     CGS_IND_REG__SMC,
> > -                     ixSMU_PM_STATUS_49);
> > -     query->max_gpu_power = cgs_read_ind_register(hwmgr->device,
> > -                     CGS_IND_REG__SMC,
> > -                     ixSMU_PM_STATUS_94);
> > -     query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
> > -                     CGS_IND_REG__SMC,
> > -                     ixSMU_PM_STATUS_95);
> > +     if (!query)
> > +             return -EINVAL;
> > +
> > +     memset(query, 0, sizeof *query);
> > +
> > +     if (hwmgr->chip_id >= CHIP_POLARIS10) {
> > +             smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetCurrPkgPwr);
> > +             query->average_gpu_power = 
> cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
> > +     } else {
> > +             smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogStart);
> > + cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC,
> > + ixSMU_PM_STATUS_94, 0);
> > +
> > +             msleep_interruptible(10);
> > +
> > +             smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogSample);
> > +
> > +             query->average_gpu_power = 
> cgs_read_ind_register(hwmgr->device,
> > + CGS_IND_REG__SMC,
> > + ixSMU_PM_STATUS_94);
> > +     }
> >
> >        return 0;
> >   }
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> amd-gfx Info Page - freedesktop.org 
> <https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
> lists.freedesktop.org
> Subscribing to amd-gfx: Subscribe to amd-gfx by filling out the 
> following form. Use of all freedesktop.org lists is subject to our 
> Code of Conduct.
>
>
>


[-- Attachment #1.2: Type: text/html, Size: 19432 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amd/pp: Refine get_gpu_power for VI

[Zhu, Rex]

[-- Attachment #1.1: Type: text/plain, Size: 5845 bytes --]

with this patch, there should be no difference between AGT and sysfs.



Best Regards

Rex

________________________________
From: Huang, JinHuiEric
Sent: Thursday, April 5, 2018 12:00 AM
To: Zhu, Rex; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH 2/3] drm/amd/pp: Refine get_gpu_power for VI


So you didn't compare the result with AGT, but I did. The reality is not like your speculation.


Regards,

Eric

On 2018-04-04 11:50 AM, Zhu, Rex wrote:

If user send message PPSMC_MSG_GetCurrPkgPwr, firmware will return the pkgpwr immediately as current power value.

as no PPSMC_MSG_GetCurrPkgPwr support, so send message  let firmware write pkgpwr  to ixSMU_PM_STATUS_94,

and driver delay 10ms to read ixSMU_PM_STATUS_94.


I don't think there is any problem. otherwise, there is same issue on polaris/vega.


I clean ixSMU_PM_STATUS_94 before let smu write it.

The delay time is enough unless we got 0 from ixSMU_PM_STATUS_94 .



Best Regards

Rex


________________________________
From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org><mailto:amd-gfx-bounces@lists.freedesktop.org> on behalf of Eric Huang <jinhuieric.huang-5C7GfCeVMHo@public.gmane.org><mailto:jinhuieric.huang-5C7GfCeVMHo@public.gmane.org>
Sent: Wednesday, April 4, 2018 11:36 PM
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
Subject: Re: [PATCH 2/3] drm/amd/pp: Refine get_gpu_power for VI

Sampling period is too short. The power reading value will be not
aligned with AGT's. It will confuse user that why AMD provides two
different power results.

Regards,
Eric

On 2018-04-04 04:25 AM, Rex Zhu wrote:
> 1. On polaris10/11/12, Sending smu message PPSMC_MSG_GetCurrPkgPwr to
>     read currentpkgpwr directly.
> 2. On Fiji/tonga/bonaire/hawwii, no PPSMC_MSG_GetCurrPkgPwr support.
>     Send PPSMC_MSG_PmStatusLogStart to let smu write currentpkgpwr
>     to ixSMU_PM_STATUS_94. this is asynchronous. need to delay no less
>     than 1ms.
> 3. Clean ixSMU_PM_STATUS_94 immediately when send PPSMC_MSG_PmStatusLogStart
>     to avoid read old value.
> 4. delay 10 ms instand of 20 ms. so the result will more similar to
>     the output of PPSMC_MSG_GetCurrPkgPwr.
> 5. remove max power/vddc/vddci output to keep consistent with vega.
> 6. for vddc/vddci power, we can calculate the average value per
>     [10ms, 4s] in other interface if needed.
>
> Signed-off-by: Rex Zhu <Rex.Zhu-5C7GfCeVMHo@public.gmane.org><mailto:Rex.Zhu-5C7GfCeVMHo@public.gmane.org>
> ---
>   drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 45 +++++++++++-------------
>   1 file changed, 21 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> index 40f2f87..c0ce672 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> @@ -3363,30 +3363,27 @@ static int smu7_get_pp_table_entry(struct pp_hwmgr *hwmgr,
>   static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
>                struct pp_gpu_power *query)
>   {
> -     PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> -                     PPSMC_MSG_PmStatusLogStart),
> -                     "Failed to start pm status log!",
> -                     return -1);
> -
> -     msleep_interruptible(20);
> -
> -     PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> -                     PPSMC_MSG_PmStatusLogSample),
> -                     "Failed to sample pm status log!",
> -                     return -1);
> -
> -     query->vddc_power = cgs_read_ind_register(hwmgr->device,
> -                     CGS_IND_REG__SMC,
> -                     ixSMU_PM_STATUS_40);
> -     query->vddci_power = cgs_read_ind_register(hwmgr->device,
> -                     CGS_IND_REG__SMC,
> -                     ixSMU_PM_STATUS_49);
> -     query->max_gpu_power = cgs_read_ind_register(hwmgr->device,
> -                     CGS_IND_REG__SMC,
> -                     ixSMU_PM_STATUS_94);
> -     query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
> -                     CGS_IND_REG__SMC,
> -                     ixSMU_PM_STATUS_95);
> +     if (!query)
> +             return -EINVAL;
> +
> +     memset(query, 0, sizeof *query);
> +
> +     if (hwmgr->chip_id >= CHIP_POLARIS10) {
> +             smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetCurrPkgPwr);
> +             query->average_gpu_power = cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
> +     } else {
> +             smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogStart);
> +             cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC,
> +                                     ixSMU_PM_STATUS_94, 0);
> +
> +             msleep_interruptible(10);
> +
> +             smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogSample);
> +
> +             query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
> +                                                     CGS_IND_REG__SMC,
> +                                                     ixSMU_PM_STATUS_94);
> +     }
>
>        return 0;
>   }

_______________________________________________
amd-gfx mailing list
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
amd-gfx Info Page - freedesktop.org<https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
lists.freedesktop.org
Subscribing to amd-gfx: Subscribe to amd-gfx by filling out the following form. Use of all freedesktop.org lists is subject to our Code of Conduct.





[-- Attachment #1.2: Type: text/html, Size: 13751 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx