All of 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; +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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.