AMD-GFX Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 1/2] amdgpu/pm: Fix incorrect variable for size of clocks array
@ 2022-05-09  3:58 Darren Powell
  2022-05-09  3:58 ` [PATCH v1 2/2] amdgpu/pm: Fix possible array out-of-bounds if SCLK levels != 2 Darren Powell
  2022-05-09  4:47 ` [PATCH v1 1/2] amdgpu/pm: Fix incorrect variable for size of clocks array Lazar, Lijo
  0 siblings, 2 replies; 5+ messages in thread
From: Darren Powell @ 2022-05-09  3:58 UTC (permalink / raw)
  To: amd-gfx
  Cc: kevin1.wang, lijo.lazar, Le.Ma, Darren Powell, evan.quan,
	kenneth.feng

Size of pp_clock_levels_with_latency is PP_MAX_CLOCK_LEVELS, not MAX_NUM_CLOCKS.
Both are currently defined as 16, modifying in case one value is modified in future
Changed code in both arcturus and aldabaran.

Also removed unneeded var count, and used min_t function

Signed-off-by: Darren Powell <darren.powell@amd.com>
---
 drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c  | 9 +++++----
 drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 9 +++++----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
index 201563072189..bdd1e1a35a12 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
@@ -573,12 +573,13 @@ static int arcturus_get_clk_table(struct smu_context *smu,
 			struct pp_clock_levels_with_latency *clocks,
 			struct smu_11_0_dpm_table *dpm_table)
 {
-	int i, count;
+	uint32_t i;
 
-	count = (dpm_table->count > MAX_NUM_CLOCKS) ? MAX_NUM_CLOCKS : dpm_table->count;
-	clocks->num_levels = count;
+	clocks->num_levels = min_t(uint32_t,
+				   dpm_table->count,
+				   (uint32_t)PP_MAX_CLOCK_LEVELS);
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; i < clocks->num_levels; i++) {
 		clocks->data[i].clocks_in_khz =
 			dpm_table->dpm_levels[i].value * 1000;
 		clocks->data[i].latency_in_us = 0;
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 38af648cb857..6a4fca47ae53 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
@@ -549,12 +549,13 @@ static int aldebaran_get_clk_table(struct smu_context *smu,
 				   struct pp_clock_levels_with_latency *clocks,
 				   struct smu_13_0_dpm_table *dpm_table)
 {
-	int i, count;
+	uint32_t i;
 
-	count = (dpm_table->count > MAX_NUM_CLOCKS) ? MAX_NUM_CLOCKS : dpm_table->count;
-	clocks->num_levels = count;
+	clocks->num_levels = min_t(uint32_t,
+				   dpm_table->count,
+				   (uint32_t)PP_MAX_CLOCK_LEVELS);
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; i < clocks->num_levels; i++) {
 		clocks->data[i].clocks_in_khz =
 			dpm_table->dpm_levels[i].value * 1000;
 		clocks->data[i].latency_in_us = 0;

base-commit: 8bb14fbec5ae45c31cbefe217db2418cc683612f
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v1 2/2] amdgpu/pm: Fix possible array out-of-bounds if SCLK levels != 2
  2022-05-09  3:58 [PATCH v1 1/2] amdgpu/pm: Fix incorrect variable for size of clocks array Darren Powell
@ 2022-05-09  3:58 ` Darren Powell
  2022-05-09  4:50   ` Lazar, Lijo
  2022-05-09  4:47 ` [PATCH v1 1/2] amdgpu/pm: Fix incorrect variable for size of clocks array Lazar, Lijo
  1 sibling, 1 reply; 5+ messages in thread
From: Darren Powell @ 2022-05-09  3:58 UTC (permalink / raw)
  To: amd-gfx
  Cc: kevin1.wang, lijo.lazar, Le.Ma, Darren Powell, evan.quan,
	kenneth.feng

 added a check to populate and use SCLK shim table freq_values only
   if using dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL or
                         AMD_DPM_FORCED_LEVEL_PERF_DETERMINISM
 removed clocks.num_levels from calculation of shim table size
 removed unsafe accesses to shim table freq_values
   output gfx_table values if using other dpm levels
 added check for freq_match when using freq_values for when now == min_clk

== Test ==
LOGFILE=aldebaran-sclk.test.log
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}

lspci -nn | grep "VGA\|Display"  > $LOGFILE
FILES="pp_od_clk_voltage
pp_dpm_sclk"

for f in $FILES
do
  echo === $f === >> $LOGFILE
  cat $HWMON_DIR/device/$f >> $LOGFILE
done
cat $LOGFILE

Signed-off-by: Darren Powell <darren.powell@amd.com>
---
 .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    | 60 +++++++++----------
 1 file changed, 29 insertions(+), 31 deletions(-)

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 6a4fca47ae53..a653668e8402 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
@@ -740,9 +740,8 @@ static int aldebaran_print_clk_levels(struct smu_context *smu,
 	struct smu_13_0_dpm_table *single_dpm_table;
 	struct smu_dpm_context *smu_dpm = &smu->smu_dpm;
 	struct smu_13_0_dpm_context *dpm_context = NULL;
-	uint32_t display_levels;
 	uint32_t freq_values[3] = {0};
-	uint32_t min_clk, max_clk;
+	uint32_t min_clk, max_clk, display_levels = 0;
 
 	smu_cmn_get_sysfs_buf(&buf, &size);
 
@@ -765,46 +764,45 @@ static int aldebaran_print_clk_levels(struct smu_context *smu,
 			return ret;
 		}
 
-		single_dpm_table = &(dpm_context->dpm_tables.gfx_table);
-		ret = aldebaran_get_clk_table(smu, &clocks, single_dpm_table);
-		if (ret) {
-			dev_err(smu->adev->dev, "Attempt to get gfx clk levels Failed!");
-			return ret;
-		}
-
-		display_levels = clocks.num_levels;
-
-		min_clk = pstate_table->gfxclk_pstate.curr.min;
-		max_clk = pstate_table->gfxclk_pstate.curr.max;
-
-		freq_values[0] = min_clk;
-		freq_values[1] = max_clk;
-
-		/* fine-grained dpm has only 2 levels */
-		if (now > min_clk && now < max_clk) {
-			display_levels = clocks.num_levels + 1;
-			freq_values[2] = max_clk;
-			freq_values[1] = now;
-		}
+		if ((smu_dpm->dpm_level != AMD_DPM_FORCED_LEVEL_MANUAL &&
+		     smu_dpm->dpm_level != AMD_DPM_FORCED_LEVEL_PERF_DETERMINISM)) {
+			single_dpm_table = &(dpm_context->dpm_tables.gfx_table);
+			ret = aldebaran_get_clk_table(smu, &clocks, single_dpm_table);
+			if (ret) {
+				dev_err(smu->adev->dev, "Attempt to get gfx clk levels Failed!");
+				return ret;
+			}
 
-		/*
-		 * For DPM disabled case, there will be only one clock level.
-		 * And it's safe to assume that is always the current clock.
-		 */
-		if (display_levels == clocks.num_levels) {
 			for (i = 0; i < clocks.num_levels; i++)
 				size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", i,
-					freq_values[i],
+					clocks.data[i].clocks_in_khz / 1000,
 					(clocks.num_levels == 1) ?
 						"*" :
 						(aldebaran_freqs_in_same_level(
-							 freq_values[i], now) ?
+							 clocks.data[i].clocks_in_khz / 1000, now) ?
 							 "*" :
 							 ""));
 		} else {
+			/* fine-grained dpm has only 2 levels */
+			display_levels = 2;
+
+			min_clk = pstate_table->gfxclk_pstate.curr.min;
+			max_clk = pstate_table->gfxclk_pstate.curr.max;
+
+			freq_values[0] = min_clk;
+			freq_values[1] = max_clk;
+
+			if (now > min_clk && now < max_clk) {
+				display_levels++;
+				freq_values[2] = max_clk;
+				freq_values[1] = now;
+			}
+
 			for (i = 0; i < display_levels; i++)
 				size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", i,
-						freq_values[i], i == 1 ? "*" : "");
+						freq_values[i],
+						aldebaran_freqs_in_same_level(freq_values[i], now) ?
+							"*" : "");
 		}
 
 		break;
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v1 1/2] amdgpu/pm: Fix incorrect variable for size of clocks array
  2022-05-09  3:58 [PATCH v1 1/2] amdgpu/pm: Fix incorrect variable for size of clocks array Darren Powell
  2022-05-09  3:58 ` [PATCH v1 2/2] amdgpu/pm: Fix possible array out-of-bounds if SCLK levels != 2 Darren Powell
@ 2022-05-09  4:47 ` Lazar, Lijo
  1 sibling, 0 replies; 5+ messages in thread
From: Lazar, Lijo @ 2022-05-09  4:47 UTC (permalink / raw)
  To: Darren Powell, amd-gfx; +Cc: Le.Ma, kevin1.wang, kenneth.feng, evan.quan



On 5/9/2022 9:28 AM, Darren Powell wrote:
> Size of pp_clock_levels_with_latency is PP_MAX_CLOCK_LEVELS, not MAX_NUM_CLOCKS.
> Both are currently defined as 16, modifying in case one value is modified in future
> Changed code in both arcturus and aldabaran.
> 
> Also removed unneeded var count, and used min_t function
> 
> Signed-off-by: Darren Powell <darren.powell@amd.com>
Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>

Thanks,
Lijo

> ---
>   drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c  | 9 +++++----
>   drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 9 +++++----
>   2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> index 201563072189..bdd1e1a35a12 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> @@ -573,12 +573,13 @@ static int arcturus_get_clk_table(struct smu_context *smu,
>   			struct pp_clock_levels_with_latency *clocks,
>   			struct smu_11_0_dpm_table *dpm_table)
>   {
> -	int i, count;
> +	uint32_t i;
>   
> -	count = (dpm_table->count > MAX_NUM_CLOCKS) ? MAX_NUM_CLOCKS : dpm_table->count;
> -	clocks->num_levels = count;
> +	clocks->num_levels = min_t(uint32_t,
> +				   dpm_table->count,
> +				   (uint32_t)PP_MAX_CLOCK_LEVELS);
>   
> -	for (i = 0; i < count; i++) {
> +	for (i = 0; i < clocks->num_levels; i++) {
>   		clocks->data[i].clocks_in_khz =
>   			dpm_table->dpm_levels[i].value * 1000;
>   		clocks->data[i].latency_in_us = 0;
> 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 38af648cb857..6a4fca47ae53 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> @@ -549,12 +549,13 @@ static int aldebaran_get_clk_table(struct smu_context *smu,
>   				   struct pp_clock_levels_with_latency *clocks,
>   				   struct smu_13_0_dpm_table *dpm_table)
>   {
> -	int i, count;
> +	uint32_t i;
>   
> -	count = (dpm_table->count > MAX_NUM_CLOCKS) ? MAX_NUM_CLOCKS : dpm_table->count;
> -	clocks->num_levels = count;
> +	clocks->num_levels = min_t(uint32_t,
> +				   dpm_table->count,
> +				   (uint32_t)PP_MAX_CLOCK_LEVELS);
>   
> -	for (i = 0; i < count; i++) {
> +	for (i = 0; i < clocks->num_levels; i++) {
>   		clocks->data[i].clocks_in_khz =
>   			dpm_table->dpm_levels[i].value * 1000;
>   		clocks->data[i].latency_in_us = 0;
> 
> base-commit: 8bb14fbec5ae45c31cbefe217db2418cc683612f
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v1 2/2] amdgpu/pm: Fix possible array out-of-bounds if SCLK levels != 2
  2022-05-09  3:58 ` [PATCH v1 2/2] amdgpu/pm: Fix possible array out-of-bounds if SCLK levels != 2 Darren Powell
@ 2022-05-09  4:50   ` Lazar, Lijo
  2022-05-10 22:07     ` Powell, Darren
  0 siblings, 1 reply; 5+ messages in thread
From: Lazar, Lijo @ 2022-05-09  4:50 UTC (permalink / raw)
  To: Darren Powell, amd-gfx; +Cc: Le.Ma, kevin1.wang, kenneth.feng, evan.quan



On 5/9/2022 9:28 AM, Darren Powell wrote:
>   added a check to populate and use SCLK shim table freq_values only
>     if using dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL or
>                           AMD_DPM_FORCED_LEVEL_PERF_DETERMINISM
>   removed clocks.num_levels from calculation of shim table size
>   removed unsafe accesses to shim table freq_values
>     output gfx_table values if using other dpm levels
>   added check for freq_match when using freq_values for when now == min_clk
> 
> == Test ==
> LOGFILE=aldebaran-sclk.test.log
> 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}
> 
> lspci -nn | grep "VGA\|Display"  > $LOGFILE
> FILES="pp_od_clk_voltage
> pp_dpm_sclk"
> 
> for f in $FILES
> do
>    echo === $f === >> $LOGFILE
>    cat $HWMON_DIR/device/$f >> $LOGFILE
> done
> cat $LOGFILE
> 
> Signed-off-by: Darren Powell <darren.powell@amd.com>
> ---
>   .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    | 60 +++++++++----------
>   1 file changed, 29 insertions(+), 31 deletions(-)
> 
> 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 6a4fca47ae53..a653668e8402 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> @@ -740,9 +740,8 @@ static int aldebaran_print_clk_levels(struct smu_context *smu,
>   	struct smu_13_0_dpm_table *single_dpm_table;
>   	struct smu_dpm_context *smu_dpm = &smu->smu_dpm;
>   	struct smu_13_0_dpm_context *dpm_context = NULL;
> -	uint32_t display_levels;
>   	uint32_t freq_values[3] = {0};
> -	uint32_t min_clk, max_clk;
> +	uint32_t min_clk, max_clk, display_levels = 0;
>   
>   	smu_cmn_get_sysfs_buf(&buf, &size);
>   
> @@ -765,46 +764,45 @@ static int aldebaran_print_clk_levels(struct smu_context *smu,
>   			return ret;
>   		}
>   
> -		single_dpm_table = &(dpm_context->dpm_tables.gfx_table);
> -		ret = aldebaran_get_clk_table(smu, &clocks, single_dpm_table);
> -		if (ret) {
> -			dev_err(smu->adev->dev, "Attempt to get gfx clk levels Failed!");
> -			return ret;
> -		}
> -
> -		display_levels = clocks.num_levels;
> -
> -		min_clk = pstate_table->gfxclk_pstate.curr.min;
> -		max_clk = pstate_table->gfxclk_pstate.curr.max;
> -
> -		freq_values[0] = min_clk;
> -		freq_values[1] = max_clk;
> -
> -		/* fine-grained dpm has only 2 levels */
> -		if (now > min_clk && now < max_clk) {
> -			display_levels = clocks.num_levels + 1;
> -			freq_values[2] = max_clk;
> -			freq_values[1] = now;
> -		}
> +		if ((smu_dpm->dpm_level != AMD_DPM_FORCED_LEVEL_MANUAL &&
> +		     smu_dpm->dpm_level != AMD_DPM_FORCED_LEVEL_PERF_DETERMINISM)) {
> +			single_dpm_table = &(dpm_context->dpm_tables.gfx_table);
> +			ret = aldebaran_get_clk_table(smu, &clocks, single_dpm_table);
> +			if (ret) {
> +				dev_err(smu->adev->dev, "Attempt to get gfx clk levels Failed!");
> +				return ret;
> +			}

There are only two levels for GFX clock in aldebaran - min and max. 
Regardless of the mode, gfxclk_pstate.curr.min/max should reflect the 
current min/max level.

Could you explain the issue you are seeing? It's not so clear from the 
commit message.

Thanks,
Lijo

>   
> -		/*
> -		 * For DPM disabled case, there will be only one clock level.
> -		 * And it's safe to assume that is always the current clock.
> -		 */
> -		if (display_levels == clocks.num_levels) {
>   			for (i = 0; i < clocks.num_levels; i++)
>   				size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", i,
> -					freq_values[i],
> +					clocks.data[i].clocks_in_khz / 1000,
>   					(clocks.num_levels == 1) ?
>   						"*" :
>   						(aldebaran_freqs_in_same_level(
> -							 freq_values[i], now) ?
> +							 clocks.data[i].clocks_in_khz / 1000, now) ?
>   							 "*" :
>   							 ""));
>   		} else {
> +			/* fine-grained dpm has only 2 levels */
> +			display_levels = 2;
> +
> +			min_clk = pstate_table->gfxclk_pstate.curr.min;
> +			max_clk = pstate_table->gfxclk_pstate.curr.max;
> +
> +			freq_values[0] = min_clk;
> +			freq_values[1] = max_clk;
> +
> +			if (now > min_clk && now < max_clk) {
> +				display_levels++;
> +				freq_values[2] = max_clk;
> +				freq_values[1] = now;
> +			}
> +
>   			for (i = 0; i < display_levels; i++)
>   				size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", i,
> -						freq_values[i], i == 1 ? "*" : "");
> +						freq_values[i],
> +						aldebaran_freqs_in_same_level(freq_values[i], now) ?
> +							"*" : "");
>   		}
>   
>   		break;
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v1 2/2] amdgpu/pm: Fix possible array out-of-bounds if SCLK levels != 2
  2022-05-09  4:50   ` Lazar, Lijo
@ 2022-05-10 22:07     ` Powell, Darren
  0 siblings, 0 replies; 5+ messages in thread
From: Powell, Darren @ 2022-05-10 22:07 UTC (permalink / raw)
  To: Lazar, Lijo, amd-gfx@lists.freedesktop.org
  Cc: Ma, Le, Quan, Evan, Feng, Kenneth, Wang, Yang(Kevin)

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

[AMD Official Use Only - General]



________________________________
From: Lazar, Lijo <Lijo.Lazar@amd.com>
Sent: Monday, May 9, 2022 12:50 AM
To: Powell, Darren <Darren.Powell@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; Feng, Kenneth <Kenneth.Feng@amd.com>; Quan, Evan <Evan.Quan@amd.com>; Ma, Le <Le.Ma@amd.com>
Subject: Re: [PATCH v1 2/2] amdgpu/pm: Fix possible array out-of-bounds if SCLK levels != 2



On 5/9/2022 9:28 AM, Darren Powell wrote:
>   added a check to populate and use SCLK shim table freq_values only
>     if using dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL or
>                           AMD_DPM_FORCED_LEVEL_PERF_DETERMINISM
>   removed clocks.num_levels from calculation of shim table size
>   removed unsafe accesses to shim table freq_values
>     output gfx_table values if using other dpm levels
>   added check for freq_match when using freq_values for when now == min_clk
>
> == Test ==
> LOGFILE=aldebaran-sclk.test.log
> 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}
>
> lspci -nn | grep "VGA\|Display"  > $LOGFILE
> FILES="pp_od_clk_voltage
> pp_dpm_sclk"
>
> for f in $FILES
> do
>    echo === $f === >> $LOGFILE
>    cat $HWMON_DIR/device/$f >> $LOGFILE
> done
> cat $LOGFILE
>
> Signed-off-by: Darren Powell <darren.powell@amd.com>
> ---
>   .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    | 60 +++++++++----------
>   1 file changed, 29 insertions(+), 31 deletions(-)
>
> 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 6a4fca47ae53..a653668e8402 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> @@ -740,9 +740,8 @@ static int aldebaran_print_clk_levels(struct smu_context *smu,
>        struct smu_13_0_dpm_table *single_dpm_table;
>        struct smu_dpm_context *smu_dpm = &smu->smu_dpm;
>        struct smu_13_0_dpm_context *dpm_context = NULL;
> -     uint32_t display_levels;
>        uint32_t freq_values[3] = {0};
> -     uint32_t min_clk, max_clk;
> +     uint32_t min_clk, max_clk, display_levels = 0;
>
>        smu_cmn_get_sysfs_buf(&buf, &size);
>
> @@ -765,46 +764,45 @@ static int aldebaran_print_clk_levels(struct smu_context *smu,
>                        return ret;
>                }
>
> -             single_dpm_table = &(dpm_context->dpm_tables.gfx_table);
> -             ret = aldebaran_get_clk_table(smu, &clocks, single_dpm_table);
> -             if (ret) {
> -                     dev_err(smu->adev->dev, "Attempt to get gfx clk levels Failed!");
> -                     return ret;
> -             }
> -
> -             display_levels = clocks.num_levels;
> -
> -             min_clk = pstate_table->gfxclk_pstate.curr.min;
> -             max_clk = pstate_table->gfxclk_pstate.curr.max;
> -
> -             freq_values[0] = min_clk;
> -             freq_values[1] = max_clk;
> -
> -             /* fine-grained dpm has only 2 levels */
> -             if (now > min_clk && now < max_clk) {
> -                     display_levels = clocks.num_levels + 1;
> -                     freq_values[2] = max_clk;
> -                     freq_values[1] = now;
> -             }
> +             if ((smu_dpm->dpm_level != AMD_DPM_FORCED_LEVEL_MANUAL &&
> +                  smu_dpm->dpm_level != AMD_DPM_FORCED_LEVEL_PERF_DETERMINISM)) {
> +                     single_dpm_table = &(dpm_context->dpm_tables.gfx_table);
> +                     ret = aldebaran_get_clk_table(smu, &clocks, single_dpm_table);
> +                     if (ret) {
> +                             dev_err(smu->adev->dev, "Attempt to get gfx clk levels Failed!");
> +                             return ret;
> +                     }

There are only two levels for GFX clock in aldebaran - min and max.
Regardless of the mode, gfxclk_pstate.curr.min/max should reflect the
current min/max level.

Could you explain the issue you are seeing? It's not so clear from the
commit message.

Thanks,
Lijo
[DP] I was concerned by the initialization of display_levels from clocks.num_levels and how it is used as the loop iterator
while accessing the freq_values array. My mistake was that I assumed that meant you intended to access clocks.data array.
If aldebaran only uses the curr.min and curr.max values that simplifies this greatly,
I'll respin this to initialize display_levels to 2 , and then output from freq_values array.

>
> -             /*
> -              * For DPM disabled case, there will be only one clock level.
> -              * And it's safe to assume that is always the current clock.
> -              */
> -             if (display_levels == clocks.num_levels) {
>                        for (i = 0; i < clocks.num_levels; i++)
>                                size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", i,
> -                                     freq_values[i],
> +                                     clocks.data[i].clocks_in_khz / 1000,
>                                        (clocks.num_levels == 1) ?
>                                                "*" :
>                                                (aldebaran_freqs_in_same_level(
> -                                                      freq_values[i], now) ?
> +                                                      clocks.data[i].clocks_in_khz / 1000, now) ?
>                                                         "*" :
>                                                         ""));
>                } else {
> +                     /* fine-grained dpm has only 2 levels */
> +                     display_levels = 2;
> +
> +                     min_clk = pstate_table->gfxclk_pstate.curr.min;
> +                     max_clk = pstate_table->gfxclk_pstate.curr.max;
> +
> +                     freq_values[0] = min_clk;
> +                     freq_values[1] = max_clk;
> +
> +                     if (now > min_clk && now < max_clk) {
> +                             display_levels++;
> +                             freq_values[2] = max_clk;
> +                             freq_values[1] = now;
> +                     }
> +
>                        for (i = 0; i < display_levels; i++)
>                                size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", i,
> -                                             freq_values[i], i == 1 ? "*" : "");
> +                                             freq_values[i],
> +                                             aldebaran_freqs_in_same_level(freq_values[i], now) ?
> +                                                     "*" : "");
>                }
>
>                break;
>

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-05-10 22:07 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-09  3:58 [PATCH v1 1/2] amdgpu/pm: Fix incorrect variable for size of clocks array Darren Powell
2022-05-09  3:58 ` [PATCH v1 2/2] amdgpu/pm: Fix possible array out-of-bounds if SCLK levels != 2 Darren Powell
2022-05-09  4:50   ` Lazar, Lijo
2022-05-10 22:07     ` Powell, Darren
2022-05-09  4:47 ` [PATCH v1 1/2] amdgpu/pm: Fix incorrect variable for size of clocks array Lazar, Lijo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox