[PATCH 0/3] Add limit_type to (pptable_funcs)->set_power_limit signature

[PATCH 0/3] Add limit_type to (pptable_funcs)->set_power_limit signature

[Darren Powell]

=== Description ===
Add limit_type to (pptable_funcs)->set_power_limit signature

plus two small patches
 Fix for incorrect power limit readback in smu11 if POWER_SOURCE_DC
 Explicit initialization of cached power limits in smu struct


=== Test System ===
* DESKTOP(AMD FX-8350 + NAVI10(731F/ca), BIOS: F2)
 + ISO(Ubuntu 20.04.3 LTS)
 + Kernel(5.13.0-g9e50250d2084-fdoagd5f)

=== Patch Summary ===
   linux: (git@gitlab.freedesktop.org:agd5f) origin/amd-staging-drm-next @ d6119c68a46e
    + ca5057580168 amdgpu/pm: add limit_type to (pptable_funcs)->set_power_limit signature
    + 6e269294d363 drm/amd/pm: Fix incorrect power limit readback in smu11 if POWER_SOURCE_DC
    + 50715de0a584 drm/amd/pm: explicitly initialize cached power limits in smu struct


Darren Powell (3):
  amdgpu/pm: add limit_type to (pptable_funcs)->set_power_limit
    signature
  drm/amd/pm: Fix incorrect power limit readback in smu11 if
    POWER_SOURCE_DC
  drm/amd/pm: explicitly initialize cached power limits in smu struct

 drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h           |  4 +++-
 drivers/gpu/drm/amd/pm/inc/smu_v11_0.h            |  4 +++-
 drivers/gpu/drm/amd/pm/inc/smu_v13_0.h            |  4 +++-
 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c         |  9 +++++++--
 drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c    | 15 +++++++++------
 drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c  |  7 ++++---
 .../gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    |  6 ++++--
 drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  8 +++++---
 8 files changed, 38 insertions(+), 19 deletions(-)


base-commit: d6119c68a46eae0b48c77353aa81e6e38b009d24
-- 
2.33.0

[PATCH 1/3] amdgpu/pm: add limit_type to (pptable_funcs)->set_power_limit signature

[Darren Powell]

 modify (pptable_funcs)->set_power_limit signature
 modify smu11 set_power_limit signature (arcturus, navi10, sienna_cichlid)
 modify smu13 set_power_limit signature (aldabaran)
 modify vangogh_set_power_limit signature (vangogh)

=== Test ===
sudo bash

AMDGPU_PCI_ADDR=`lspci -nn | grep "VGA\|Display" | cut -d " " -f 1`
AMDGPU_HWMON=`ls -la /sys/class/hwmon | grep $AMDGPU_PCI_ADDR | awk '{print $9}'`
HWMON_DIR=/sys/class/hwmon/${AMDGPU_HWMON}
LOGFILE=pp_show_power_cap.log

cp $LOGFILE{,.old}
lspci -nn | grep "VGA\|Display" > $LOGFILE
FILES="
power1_cap
power2_cap"

for f in $FILES
do
  if test -f "$HWMON_DIR/$f"; then
    echo === $f === >> $LOGFILE
    cat $HWMON_DIR/$f >> $LOGFILE
    RESTORE_VALUE=`cat $HWMON_DIR/$f` 2>&1  >> $LOGFILE
    echo RESTORE_VALUE $RESTORE_VALUE >> $LOGFILE
    echo 120000000 > $HWMON_DIR/$f
    sleep 3
    cat $HWMON_DIR/$f >> $LOGFILE
    echo $RESTORE_VALUE > $HWMON_DIR/$f
    sleep 3
    cat $HWMON_DIR/$f >> $LOGFILE
  else
    echo === $f === >> $LOGFILE
    echo File Not Found >> $LOGFILE
  fi
done
cat $LOGFILE

Signed-off-by: Darren Powell <darren.powell@amd.com>
---
 drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h            |  4 +++-
 drivers/gpu/drm/amd/pm/inc/smu_v11_0.h             |  4 +++-
 drivers/gpu/drm/amd/pm/inc/smu_v13_0.h             |  4 +++-
 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c          |  5 +++--
 drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c     | 14 ++++++++------
 drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c   |  7 ++++---
 drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c |  6 ++++--
 drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c     |  8 +++++---
 8 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
index 8156729c370b..3557f4e7fc30 100644
--- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
@@ -1008,7 +1008,9 @@ struct pptable_funcs {
 	/**
 	 * @set_power_limit: Set power limit in watts.
 	 */
-	int (*set_power_limit)(struct smu_context *smu, uint32_t n);
+	int (*set_power_limit)(struct smu_context *smu,
+			       enum smu_ppt_limit_type limit_type,
+			       uint32_t limit);
 
 	/**
 	 * @init_max_sustainable_clocks: Populate max sustainable clock speed
diff --git a/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h b/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h
index cbdae8a2c698..2d422e6a9feb 100644
--- a/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h
@@ -197,7 +197,9 @@ int smu_v11_0_notify_display_change(struct smu_context *smu);
 int smu_v11_0_get_current_power_limit(struct smu_context *smu,
 				      uint32_t *power_limit);
 
-int smu_v11_0_set_power_limit(struct smu_context *smu, uint32_t n);
+int smu_v11_0_set_power_limit(struct smu_context *smu,
+			      enum smu_ppt_limit_type limit_type,
+			      uint32_t limit);
 
 int smu_v11_0_init_max_sustainable_clocks(struct smu_context *smu);
 
diff --git a/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h b/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h
index dc91eb608791..e5d3b0d1a032 100644
--- a/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h
@@ -163,7 +163,9 @@ int smu_v13_0_notify_display_change(struct smu_context *smu);
 int smu_v13_0_get_current_power_limit(struct smu_context *smu,
 				      uint32_t *power_limit);
 
-int smu_v13_0_set_power_limit(struct smu_context *smu, uint32_t n);
+int smu_v13_0_set_power_limit(struct smu_context *smu,
+			      enum smu_ppt_limit_type limit_type,
+			      uint32_t limit);
 
 int smu_v13_0_init_max_sustainable_clocks(struct smu_context *smu);
 
diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index a2a2a8398cd7..faa78a048b1f 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -2342,9 +2342,10 @@ static int smu_set_power_limit(void *handle, uint32_t limit)
 
 	mutex_lock(&smu->mutex);
 
+	limit &= (1<<24)-1;
 	if (limit_type != SMU_DEFAULT_PPT_LIMIT)
 		if (smu->ppt_funcs->set_power_limit) {
-			ret = smu->ppt_funcs->set_power_limit(smu, limit);
+			ret = smu->ppt_funcs->set_power_limit(smu, limit_type, limit);
 			goto out;
 		}
 
@@ -2360,7 +2361,7 @@ static int smu_set_power_limit(void *handle, uint32_t limit)
 		limit = smu->current_power_limit;
 
 	if (smu->ppt_funcs->set_power_limit) {
-		ret = smu->ppt_funcs->set_power_limit(smu, limit);
+		ret = smu->ppt_funcs->set_power_limit(smu, limit_type, limit);
 		if (!ret && !(smu->user_dpm_profile.flags & SMU_DPM_USER_PROFILE_RESTORE))
 			smu->user_dpm_profile.power_limit = limit;
 	}
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
index 3470c33ee09d..aedaa4bb15c2 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
@@ -978,7 +978,9 @@ int smu_v11_0_get_current_power_limit(struct smu_context *smu,
 	return ret;
 }
 
-int smu_v11_0_set_power_limit(struct smu_context *smu, uint32_t n)
+int smu_v11_0_set_power_limit(struct smu_context *smu,
+			      enum smu_ppt_limit_type limit_type,
+			      uint32_t limit)
 {
 	int power_src;
 	int ret = 0;
@@ -1001,16 +1003,16 @@ int smu_v11_0_set_power_limit(struct smu_context *smu, uint32_t n)
 	 * BIT 16-23: PowerSource
 	 * BIT 0-15: PowerLimit
 	 */
-	n &= 0xFFFF;
-	n |= 0 << 24;
-	n |= (power_src) << 16;
-	ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, n, NULL);
+	limit &= 0xFFFF;
+	limit |= 0 << 24;
+	limit |= (power_src) << 16;
+	ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit, NULL);
 	if (ret) {
 		dev_err(smu->adev->dev, "[%s] Set power limit Failed!\n", __func__);
 		return ret;
 	}
 
-	smu->current_power_limit = n;
+	smu->current_power_limit = limit;
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
index f6ef0ce6e9e2..eba516428f1b 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
@@ -2144,11 +2144,12 @@ static int vangogh_get_ppt_limit(struct smu_context *smu,
 	return 0;
 }
 
-static int vangogh_set_power_limit(struct smu_context *smu, uint32_t ppt_limit)
+static int vangogh_set_power_limit(struct smu_context *smu,
+				   enum smu_ppt_limit_type limit_type,
+				   uint32_t ppt_limit)
 {
 	struct smu_11_5_power_context *power_context =
-							smu->smu_power.power_context;
-	uint32_t limit_type = ppt_limit >> 24;
+			smu->smu_power.power_context;
 	int ret = 0;
 
 	if (!smu_cmn_feature_is_enabled(smu, SMU_FEATURE_PPT_BIT)) {
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
index 5019903db492..59a7d276541d 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
@@ -1241,11 +1241,13 @@ static int aldebaran_get_power_limit(struct smu_context *smu,
 	return 0;
 }
 
-static int aldebaran_set_power_limit(struct smu_context *smu, uint32_t n)
+static int aldebaran_set_power_limit(struct smu_context *smu,
+				     enum smu_ppt_limit_type limit_type,
+				     uint32_t limit)
 {
 	/* Power limit can be set only through primary die */
 	if (aldebaran_is_primary(smu))
-		return smu_v13_0_set_power_limit(smu, n);
+		return smu_v13_0_set_power_limit(smu, limit_type, limit);
 
 	return -EINVAL;
 }
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
index 05c5e61f3506..58d837d9a414 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
@@ -945,7 +945,9 @@ int smu_v13_0_get_current_power_limit(struct smu_context *smu,
 	return ret;
 }
 
-int smu_v13_0_set_power_limit(struct smu_context *smu, uint32_t n)
+int smu_v13_0_set_power_limit(struct smu_context *smu,
+			      enum smu_ppt_limit_type limit_type,
+			      uint32_t limit)
 {
 	int ret = 0;
 
@@ -954,13 +956,13 @@ int smu_v13_0_set_power_limit(struct smu_context *smu, uint32_t n)
 		return -EOPNOTSUPP;
 	}
 
-	ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, n, NULL);
+	ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit, NULL);
 	if (ret) {
 		dev_err(smu->adev->dev, "[%s] Set power limit Failed!\n", __func__);
 		return ret;
 	}
 
-	smu->current_power_limit = n;
+	smu->current_power_limit = limit;
 
 	return 0;
 }
-- 
2.33.0

[PATCH 2/3] drm/amd/pm: Fix incorrect power limit readback in smu11 if POWER_SOURCE_DC

[Darren Powell]

 when smu->adev->pm.ac_power == 0, message parameter with bit 16 set is saved
 to smu->current_power_limit.

 Fixes: 0cb4c62125a9 ("drm/amd/pm: correct power limit setting for SMU V11)"

Signed-off-by: Darren Powell <darren.powell@amd.com>
---
 drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
index aedaa4bb15c2..9bb6da99d5b5 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
@@ -984,6 +984,7 @@ int smu_v11_0_set_power_limit(struct smu_context *smu,
 {
 	int power_src;
 	int ret = 0;
+	uint32_t limit_param;
 
 	if (!smu_cmn_feature_is_enabled(smu, SMU_FEATURE_PPT_BIT)) {
 		dev_err(smu->adev->dev, "Setting new power limit is not supported!\n");
@@ -1003,10 +1004,10 @@ int smu_v11_0_set_power_limit(struct smu_context *smu,
 	 * BIT 16-23: PowerSource
 	 * BIT 0-15: PowerLimit
 	 */
-	limit &= 0xFFFF;
-	limit |= 0 << 24;
-	limit |= (power_src) << 16;
-	ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit, NULL);
+	limit_param  = (limit & 0xFFFF);
+	limit_param |= 0 << 24;
+	limit_param |= (power_src) << 16;
+	ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit_param, NULL);
 	if (ret) {
 		dev_err(smu->adev->dev, "[%s] Set power limit Failed!\n", __func__);
 		return ret;
-- 
2.33.0

[PATCH 3/3] drm/amd/pm: explicitly initialize cached power limits in smu struct

[Darren Powell]

Code appears to initialize values but macro will exit without error
or initializing value if function is not implmented

Signed-off-by: Darren Powell <darren.powell@amd.com>
---
 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index faa78a048b1f..210f047e136d 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -712,6 +712,10 @@ static int smu_late_init(void *handle)
 		return ret;
 	}
 
+	smu->current_power_limit = 0;
+	smu->default_power_limit = 0;
+	smu->max_power_limit = 0;
+
 	ret = smu_get_asic_power_limits(smu,
 					&smu->current_power_limit,
 					&smu->default_power_limit,
-- 
2.33.0

Re: [PATCH 2/3] drm/amd/pm: Fix incorrect power limit readback in smu11 if POWER_SOURCE_DC

[Nils Wallménius]

[-- Attachment #1: Type: text/plain, Size: 1932 bytes --]

Hi, sorry for the drive-by comment but

limit_param |= 0 << 24;

Doesn't do anything.

Best regards
Nils

Den sön 3 okt. 2021 06:47Darren Powell <darren.powell@amd.com> skrev:

>  when smu->adev->pm.ac_power == 0, message parameter with bit 16 set is
> saved
>  to smu->current_power_limit.
>
>  Fixes: 0cb4c62125a9 ("drm/amd/pm: correct power limit setting for SMU
> V11)"
>
> Signed-off-by: Darren Powell <darren.powell@amd.com>
> ---
>  drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> index aedaa4bb15c2..9bb6da99d5b5 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> @@ -984,6 +984,7 @@ int smu_v11_0_set_power_limit(struct smu_context *smu,
>  {
>         int power_src;
>         int ret = 0;
> +       uint32_t limit_param;
>
>         if (!smu_cmn_feature_is_enabled(smu, SMU_FEATURE_PPT_BIT)) {
>                 dev_err(smu->adev->dev, "Setting new power limit is not
> supported!\n");
> @@ -1003,10 +1004,10 @@ int smu_v11_0_set_power_limit(struct smu_context
> *smu,
>          * BIT 16-23: PowerSource
>          * BIT 0-15: PowerLimit
>          */
> -       limit &= 0xFFFF;
> -       limit |= 0 << 24;
> -       limit |= (power_src) << 16;
> -       ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit,
> limit, NULL);
> +       limit_param  = (limit & 0xFFFF);
> +       limit_param |= 0 << 24;
> +       limit_param |= (power_src) << 16;
> +       ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit,
> limit_param, NULL);
>         if (ret) {
>                 dev_err(smu->adev->dev, "[%s] Set power limit Failed!\n",
> __func__);
>                 return ret;
> --
> 2.33.0
>
>

[-- Attachment #2: Type: text/html, Size: 2946 bytes --]

Re: [PATCH 2/3] drm/amd/pm: Fix incorrect power limit readback in smu11 if POWER_SOURCE_DC

[Powell, Darren]

[-- Attachment #1: Type: text/plain, Size: 2465 bytes --]

[AMD Official Use Only]

Yup, agreed, but I rationalized that it:
 - minimizes code changes, leave existing code in place
 - reinforces the comment, which states that only 0 is supported in this bitfield at this time

Thanks
Darren
________________________________
From: Nils Wallménius <nils.wallmenius@gmail.com>
Sent: Sunday, October 3, 2021 1:31 AM
To: Powell, Darren <Darren.Powell@amd.com>
Cc: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 2/3] drm/amd/pm: Fix incorrect power limit readback in smu11 if POWER_SOURCE_DC

Hi, sorry for the drive-by comment but

limit_param |= 0 << 24;

Doesn't do anything.

Best regards
Nils

Den sön 3 okt. 2021 06:47Darren Powell <darren.powell@amd.com<mailto:darren.powell@amd.com>> skrev:
 when smu->adev->pm.ac_power == 0, message parameter with bit 16 set is saved
 to smu->current_power_limit.

 Fixes: 0cb4c62125a9 ("drm/amd/pm: correct power limit setting for SMU V11)"

Signed-off-by: Darren Powell <darren.powell@amd.com<mailto:darren.powell@amd.com>>
---
 drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
index aedaa4bb15c2..9bb6da99d5b5 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
@@ -984,6 +984,7 @@ int smu_v11_0_set_power_limit(struct smu_context *smu,
 {
        int power_src;
        int ret = 0;
+       uint32_t limit_param;

        if (!smu_cmn_feature_is_enabled(smu, SMU_FEATURE_PPT_BIT)) {
                dev_err(smu->adev->dev, "Setting new power limit is not supported!\n");
@@ -1003,10 +1004,10 @@ int smu_v11_0_set_power_limit(struct smu_context *smu,
         * BIT 16-23: PowerSource
         * BIT 0-15: PowerLimit
         */
-       limit &= 0xFFFF;
-       limit |= 0 << 24;
-       limit |= (power_src) << 16;
-       ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit, NULL);
+       limit_param  = (limit & 0xFFFF);
+       limit_param |= 0 << 24;
+       limit_param |= (power_src) << 16;
+       ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit_param, NULL);
        if (ret) {
                dev_err(smu->adev->dev, "[%s] Set power limit Failed!\n", __func__);
                return ret;
--
2.33.0


[-- Attachment #2: Type: text/html, Size: 5268 bytes --]

Re: [PATCH 1/3] amdgpu/pm: add limit_type to (pptable_funcs)->set_power_limit signature

[Lazar, Lijo]

On 10/3/2021 10:16 AM, Darren Powell wrote:
>   modify (pptable_funcs)->set_power_limit signature
>   modify smu11 set_power_limit signature (arcturus, navi10, sienna_cichlid)
>   modify smu13 set_power_limit signature (aldabaran)
>   modify vangogh_set_power_limit signature (vangogh)
> 
> === Test ===
> sudo bash
> 
> AMDGPU_PCI_ADDR=`lspci -nn | grep "VGA\|Display" | cut -d " " -f 1`
> AMDGPU_HWMON=`ls -la /sys/class/hwmon | grep $AMDGPU_PCI_ADDR | awk '{print $9}'`
> HWMON_DIR=/sys/class/hwmon/${AMDGPU_HWMON}
> LOGFILE=pp_show_power_cap.log
> 
> cp $LOGFILE{,.old}
> lspci -nn | grep "VGA\|Display" > $LOGFILE
> FILES="
> power1_cap
> power2_cap"
> 
> for f in $FILES
> do
>    if test -f "$HWMON_DIR/$f"; then
>      echo === $f === >> $LOGFILE
>      cat $HWMON_DIR/$f >> $LOGFILE
>      RESTORE_VALUE=`cat $HWMON_DIR/$f` 2>&1  >> $LOGFILE
>      echo RESTORE_VALUE $RESTORE_VALUE >> $LOGFILE
>      echo 120000000 > $HWMON_DIR/$f
>      sleep 3
>      cat $HWMON_DIR/$f >> $LOGFILE
>      echo $RESTORE_VALUE > $HWMON_DIR/$f
>      sleep 3
>      cat $HWMON_DIR/$f >> $LOGFILE
>    else
>      echo === $f === >> $LOGFILE
>      echo File Not Found >> $LOGFILE
>    fi
> done
> cat $LOGFILE
> 
> Signed-off-by: Darren Powell <darren.powell@amd.com>
> ---
>   drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h            |  4 +++-
>   drivers/gpu/drm/amd/pm/inc/smu_v11_0.h             |  4 +++-
>   drivers/gpu/drm/amd/pm/inc/smu_v13_0.h             |  4 +++-
>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c          |  5 +++--
>   drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c     | 14 ++++++++------
>   drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c   |  7 ++++---
>   drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c |  6 ++++--
>   drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c     |  8 +++++---
>   8 files changed, 33 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> index 8156729c370b..3557f4e7fc30 100644
> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> @@ -1008,7 +1008,9 @@ struct pptable_funcs {
>   	/**
>   	 * @set_power_limit: Set power limit in watts.
>   	 */
> -	int (*set_power_limit)(struct smu_context *smu, uint32_t n);
> +	int (*set_power_limit)(struct smu_context *smu,
> +			       enum smu_ppt_limit_type limit_type,
> +			       uint32_t limit);
>   
>   	/**
>   	 * @init_max_sustainable_clocks: Populate max sustainable clock speed
> diff --git a/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h b/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h
> index cbdae8a2c698..2d422e6a9feb 100644
> --- a/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h
> +++ b/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h
> @@ -197,7 +197,9 @@ int smu_v11_0_notify_display_change(struct smu_context *smu);
>   int smu_v11_0_get_current_power_limit(struct smu_context *smu,
>   				      uint32_t *power_limit);
>   
> -int smu_v11_0_set_power_limit(struct smu_context *smu, uint32_t n);
> +int smu_v11_0_set_power_limit(struct smu_context *smu,
> +			      enum smu_ppt_limit_type limit_type,
> +			      uint32_t limit);
>   
>   int smu_v11_0_init_max_sustainable_clocks(struct smu_context *smu);
>   
> diff --git a/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h b/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h
> index dc91eb608791..e5d3b0d1a032 100644
> --- a/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h
> +++ b/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h
> @@ -163,7 +163,9 @@ int smu_v13_0_notify_display_change(struct smu_context *smu);
>   int smu_v13_0_get_current_power_limit(struct smu_context *smu,
>   				      uint32_t *power_limit);
>   
> -int smu_v13_0_set_power_limit(struct smu_context *smu, uint32_t n);
> +int smu_v13_0_set_power_limit(struct smu_context *smu,
> +			      enum smu_ppt_limit_type limit_type,
> +			      uint32_t limit);
>   
>   int smu_v13_0_init_max_sustainable_clocks(struct smu_context *smu);
>   
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index a2a2a8398cd7..faa78a048b1f 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -2342,9 +2342,10 @@ static int smu_set_power_limit(void *handle, uint32_t limit)
>   
>   	mutex_lock(&smu->mutex);
>   
> +	limit &= (1<<24)-1;
>   	if (limit_type != SMU_DEFAULT_PPT_LIMIT)
>   		if (smu->ppt_funcs->set_power_limit) {
> -			ret = smu->ppt_funcs->set_power_limit(smu, limit);
> +			ret = smu->ppt_funcs->set_power_limit(smu, limit_type, limit);
>   			goto out;
>   		}
>   
> @@ -2360,7 +2361,7 @@ static int smu_set_power_limit(void *handle, uint32_t limit)
>   		limit = smu->current_power_limit;
>   
>   	if (smu->ppt_funcs->set_power_limit) {
> -		ret = smu->ppt_funcs->set_power_limit(smu, limit);
> +		ret = smu->ppt_funcs->set_power_limit(smu, limit_type, limit);
>   		if (!ret && !(smu->user_dpm_profile.flags & SMU_DPM_USER_PROFILE_RESTORE))
>   			smu->user_dpm_profile.power_limit = limit;
>   	}
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> index 3470c33ee09d..aedaa4bb15c2 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> @@ -978,7 +978,9 @@ int smu_v11_0_get_current_power_limit(struct smu_context *smu,
>   	return ret;
>   }
>   
> -int smu_v11_0_set_power_limit(struct smu_context *smu, uint32_t n)
> +int smu_v11_0_set_power_limit(struct smu_context *smu,
> +			      enum smu_ppt_limit_type limit_type,
> +			      uint32_t limit)
>   {
>   	int power_src;
>   	int ret = 0;
> @@ -1001,16 +1003,16 @@ int smu_v11_0_set_power_limit(struct smu_context *smu, uint32_t n)
>   	 * BIT 16-23: PowerSource
>   	 * BIT 0-15: PowerLimit
>   	 */
> -	n &= 0xFFFF;
> -	n |= 0 << 24;
> -	n |= (power_src) << 16;
> -	ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, n, NULL);

Since limit_type is introduced as arg, could you also add below to 
smuv11/v13? Currently, anything other than default is used only in vangogh.

	if (limit_type != SMU_DEFAULT_PPT_LIMIT)
		return -EINVAL;

That will also avoid any 'unused variable' warning.

Thanks,
Lijo

> +	limit &= 0xFFFF;
> +	limit |= 0 << 24;
> +	limit |= (power_src) << 16;
> +	ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit, NULL);
>   	if (ret) {
>   		dev_err(smu->adev->dev, "[%s] Set power limit Failed!\n", __func__);
>   		return ret;
>   	}
>   
> -	smu->current_power_limit = n;
> +	smu->current_power_limit = limit;
>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> index f6ef0ce6e9e2..eba516428f1b 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> @@ -2144,11 +2144,12 @@ static int vangogh_get_ppt_limit(struct smu_context *smu,
>   	return 0;
>   }
>   
> -static int vangogh_set_power_limit(struct smu_context *smu, uint32_t ppt_limit)
> +static int vangogh_set_power_limit(struct smu_context *smu,
> +				   enum smu_ppt_limit_type limit_type,
> +				   uint32_t ppt_limit)
>   {
>   	struct smu_11_5_power_context *power_context =
> -							smu->smu_power.power_context;
> -	uint32_t limit_type = ppt_limit >> 24;
> +			smu->smu_power.power_context;
>   	int ret = 0;
>   
>   	if (!smu_cmn_feature_is_enabled(smu, SMU_FEATURE_PPT_BIT)) {
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> index 5019903db492..59a7d276541d 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> @@ -1241,11 +1241,13 @@ static int aldebaran_get_power_limit(struct smu_context *smu,
>   	return 0;
>   }
>   
> -static int aldebaran_set_power_limit(struct smu_context *smu, uint32_t n)
> +static int aldebaran_set_power_limit(struct smu_context *smu,
> +				     enum smu_ppt_limit_type limit_type,
> +				     uint32_t limit)
>   {
>   	/* Power limit can be set only through primary die */
>   	if (aldebaran_is_primary(smu))
> -		return smu_v13_0_set_power_limit(smu, n);
> +		return smu_v13_0_set_power_limit(smu, limit_type, limit);
>   
>   	return -EINVAL;
>   }
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> index 05c5e61f3506..58d837d9a414 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> @@ -945,7 +945,9 @@ int smu_v13_0_get_current_power_limit(struct smu_context *smu,
>   	return ret;
>   }
>   
> -int smu_v13_0_set_power_limit(struct smu_context *smu, uint32_t n)
> +int smu_v13_0_set_power_limit(struct smu_context *smu,
> +			      enum smu_ppt_limit_type limit_type,
> +			      uint32_t limit)
>   {
>   	int ret = 0;
>   
> @@ -954,13 +956,13 @@ int smu_v13_0_set_power_limit(struct smu_context *smu, uint32_t n)
>   		return -EOPNOTSUPP;
>   	}
>   
> -	ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, n, NULL);
> +	ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit, NULL);
>   	if (ret) {
>   		dev_err(smu->adev->dev, "[%s] Set power limit Failed!\n", __func__);
>   		return ret;
>   	}
>   
> -	smu->current_power_limit = n;
> +	smu->current_power_limit = limit;
>   
>   	return 0;
>   }
> 

Re: [PATCH 3/3] drm/amd/pm: explicitly initialize cached power limits in smu struct

[Lazar, Lijo]

On 10/3/2021 10:16 AM, Darren Powell wrote:
> Code appears to initialize values but macro will exit without error
> or initializing value if function is not implmented
> 
> Signed-off-by: Darren Powell <darren.powell@amd.com>
> ---
>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index faa78a048b1f..210f047e136d 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -712,6 +712,10 @@ static int smu_late_init(void *handle)
>   		return ret;
>   	}
>   
> +	smu->current_power_limit = 0;
> +	smu->default_power_limit = 0;
> +	smu->max_power_limit = 0;
> +

If this is only about first-time init - smu_context is part of adev, it 
will be zero initialized when adev is allocated.


Thanks,
Lijo

>   	ret = smu_get_asic_power_limits(smu,
>   					&smu->current_power_limit,
>   					&smu->default_power_limit,
> 

Re: [PATCH 3/3] drm/amd/pm: explicitly initialize cached power limits in smu struct

[Powell, Darren]

[-- Attachment #1: Type: text/plain, Size: 2453 bytes --]

[AMD Official Use Only]

I'm just looking to clarify this code. The macro eventually expands to look like this

   if ((smu)->ppt_funcs)
   {
      if ((smu)->ppt_funcs->get_power_limit)
          (smu)->ppt_funcs->get_power_limit(smu,
                                            &smu->current_power_limit,                                                                     &smu->default_power_limit,
                                            &smu->max_power_limit);
      else
         return 0;
   }
   else
      return -EINVAL;

But you have to dig to realize that it's a macro, and that it makes no modification if the function is not defined. It's not clear without then searching and following the function pointers which platforms are using the saved value. I thought of inserting the following comment or should I just drop this altogether?
 /* seed the cached smu power limit values iff get_power_limit is defined, otherwise they remain 0 */

Thanks
Darren

________________________________
From: Lazar, Lijo <Lijo.Lazar@amd.com>
Sent: Monday, October 4, 2021 6:43 AM
To: Powell, Darren <Darren.Powell@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 3/3] drm/amd/pm: explicitly initialize cached power limits in smu struct



On 10/3/2021 10:16 AM, Darren Powell wrote:
> Code appears to initialize values but macro will exit without error
> or initializing value if function is not implmented
>
> Signed-off-by: Darren Powell <darren.powell@amd.com>
> ---
>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index faa78a048b1f..210f047e136d 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -712,6 +712,10 @@ static int smu_late_init(void *handle)
>                return ret;
>        }
>
> +     smu->current_power_limit = 0;
> +     smu->default_power_limit = 0;
> +     smu->max_power_limit = 0;
> +

If this is only about first-time init - smu_context is part of adev, it
will be zero initialized when adev is allocated.


Thanks,
Lijo

>        ret = smu_get_asic_power_limits(smu,
>                                        &smu->current_power_limit,
>                                        &smu->default_power_limit,
>

[-- Attachment #2: Type: text/html, Size: 7433 bytes --]

Re: [PATCH 3/3] drm/amd/pm: explicitly initialize cached power limits in smu struct

[Lazar, Lijo]

On 10/5/2021 10:34 AM, Powell, Darren wrote:
> [AMD Official Use Only]
> 
> 
> I'm just looking to clarify this code. The macro eventually expands to 
> look like this
> 
>     if ((smu)->ppt_funcs)
>     {
>        if ((smu)->ppt_funcs->get_power_limit)
>            (smu)->ppt_funcs->get_power_limit(smu,
>                                              &smu->current_power_limit, 
> &smu->default_power_limit,
> &smu->max_power_limit);
>        else
>           return 0;
>     }
>     else
>        return -EINVAL;
> 
> But you have to dig to realize that it's a macro, and that it makes no 
> modification if the function is not defined. It's not clear without then 
> searching and following the function pointers which platforms are using 
> the saved value. I thought of inserting the following comment or should 
> I just drop this altogether?

Yes. The limit values are by default initialized to 0. If the function 
is not supported, it's not considered an error to fail late_init (it 
only reports 0 values in hwmon).

late_init also gets called on other occasions like resume or after a 
GPU-only reset. If you want to change the invalid limit value in hwmon 
from 0 to something else (to differentiate unsupported vs API returning 
0 value), then better to do in sw_init.

Thanks,
Lijo

>   /* seed the cached smu power limit values iff get_power_limit is 
> defined, otherwise they remain 0 */
> 
> Thanks
> Darren
> 
> ------------------------------------------------------------------------
> *From:* Lazar, Lijo <Lijo.Lazar@amd.com>
> *Sent:* Monday, October 4, 2021 6:43 AM
> *To:* Powell, Darren <Darren.Powell@amd.com>; 
> amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> *Subject:* Re: [PATCH 3/3] drm/amd/pm: explicitly initialize cached 
> power limits in smu struct
> 
> 
> On 10/3/2021 10:16 AM, Darren Powell wrote:
>> Code appears to initialize values but macro will exit without error
>> or initializing value if function is not implmented
>> 
>> Signed-off-by: Darren Powell <darren.powell@amd.com>
>> ---
>>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> index faa78a048b1f..210f047e136d 100644
>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> @@ -712,6 +712,10 @@ static int smu_late_init(void *handle)
>>                return ret;
>>        }
>>   
>> +     smu->current_power_limit = 0;
>> +     smu->default_power_limit = 0;
>> +     smu->max_power_limit = 0;
>> +
> 
> If this is only about first-time init - smu_context is part of adev, it
> will be zero initialized when adev is allocated.
> 
> 
> Thanks,
> Lijo
> 
>>        ret = smu_get_asic_power_limits(smu,
>>                                        &smu->current_power_limit,
>>                                        &smu->default_power_limit,
>>