Re: [PATCH 2/3] drm/amdgpu/pm: add new fields for Navi1x

[Lijo Lazar <lijo.lazar@xxxxxxx>]

On 5/18/2021 9:39 AM, David M Nieto wrote:
> Fill voltage fields in metrics table
>
> Signed-off-by: David M Nieto <david.nieto@xxxxxxx>
> ---
>   .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c   | 62 ++++++++++++++-----
>   1 file changed, 45 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> index ac13042672ea..9339fd24ae8c 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> @@ -505,7 +505,7 @@ static int navi10_tables_init(struct smu_context *smu)
>   		goto err0_out;
>   	smu_table->metrics_time = 0;
>   
> -	smu_table->gpu_metrics_table_size = sizeof(struct gpu_metrics_v1_1);
> +	smu_table->gpu_metrics_table_size = sizeof(struct gpu_metrics_v1_3);
>   	smu_table->gpu_metrics_table = kzalloc(smu_table->gpu_metrics_table_size, GFP_KERNEL);
>   	if (!smu_table->gpu_metrics_table)
>   		goto err1_out;
> @@ -2627,10 +2627,11 @@ static ssize_t navi10_get_legacy_gpu_metrics(struct smu_context *smu,
>   					     void **table)
>   {
>   	struct smu_table_context *smu_table = &smu->smu_table;
> -	struct gpu_metrics_v1_1 *gpu_metrics =
> -		(struct gpu_metrics_v1_1 *)smu_table->gpu_metrics_table;
> +	struct gpu_metrics_v1_3 *gpu_metrics =
> +		(struct gpu_metrics_v1_3 *)smu_table->gpu_metrics_table;
>   	SmuMetrics_legacy_t metrics;
>   	int ret = 0;
> +	int freq = 0, dpm = 0;

Variables added, seems unused in new code.

>   	mutex_lock(&smu->metrics_lock);
>   
> @@ -2646,7 +2647,7 @@ static ssize_t navi10_get_legacy_gpu_metrics(struct smu_context *smu,
>   
>   	mutex_unlock(&smu->metrics_lock);
>   
> -	smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 1);
> +	smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 3);
>   
>   	gpu_metrics->temperature_edge = metrics.TemperatureEdge;
>   	gpu_metrics->temperature_hotspot = metrics.TemperatureHotspot;
> @@ -2681,19 +2682,26 @@ static ssize_t navi10_get_legacy_gpu_metrics(struct smu_context *smu,
>   
>   	gpu_metrics->system_clock_counter = ktime_get_boottime_ns();
>   
> +	gpu_metrics->voltage_gfx = (155000 - 625 * metrics.CurrGfxVoltageOffset) / 100;
> +	gpu_metrics->voltage_mem = (155000 - 625 * metrics.CurrMemVidOffset) / 100;
> +	gpu_metrics->voltage_soc = (155000 - 625 * metrics.CurrSocVoltageOffset) / 100;
> +

It's better to add a non-zero check for offset values. Having 0 as 
offset value is unlikely, otherwise it could show the wrong voltage if 
FW is not passing the data.

Same comments for below functions also.

Thanks,
Lijo

>   	*table = (void *)gpu_metrics;
>   
> -	return sizeof(struct gpu_metrics_v1_1);
> +	return sizeof(struct gpu_metrics_v1_3);
> +out:
> +	return ret;
>   }
>   
>   static ssize_t navi10_get_gpu_metrics(struct smu_context *smu,
>   				      void **table)
>   {
>   	struct smu_table_context *smu_table = &smu->smu_table;
> -	struct gpu_metrics_v1_1 *gpu_metrics =
> -		(struct gpu_metrics_v1_1 *)smu_table->gpu_metrics_table;
> +	struct gpu_metrics_v1_3 *gpu_metrics =
> +		(struct gpu_metrics_v1_3 *)smu_table->gpu_metrics_table;
>   	SmuMetrics_t metrics;
>   	int ret = 0;
> +	int freq = 0, dpm = 0;
>   
>   	mutex_lock(&smu->metrics_lock);
>   
> @@ -2709,7 +2717,7 @@ static ssize_t navi10_get_gpu_metrics(struct smu_context *smu,
>   
>   	mutex_unlock(&smu->metrics_lock);
>   
> -	smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 1);
> +	smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 3);
>   
>   	gpu_metrics->temperature_edge = metrics.TemperatureEdge;
>   	gpu_metrics->temperature_hotspot = metrics.TemperatureHotspot;
> @@ -2746,19 +2754,26 @@ static ssize_t navi10_get_gpu_metrics(struct smu_context *smu,
>   
>   	gpu_metrics->system_clock_counter = ktime_get_boottime_ns();
>   
> +	gpu_metrics->voltage_gfx = (155000 - 625 * metrics.CurrGfxVoltageOffset) / 100;
> +	gpu_metrics->voltage_mem = (155000 - 625 * metrics.CurrMemVidOffset) / 100;
> +	gpu_metrics->voltage_soc = (155000 - 625 * metrics.CurrSocVoltageOffset) / 100;
> +
>   	*table = (void *)gpu_metrics;
>   
> -	return sizeof(struct gpu_metrics_v1_1);
> +	return sizeof(struct gpu_metrics_v1_3);
> +out:
> +	return ret;
>   }
>   
>   static ssize_t navi12_get_legacy_gpu_metrics(struct smu_context *smu,
>   					     void **table)
>   {
>   	struct smu_table_context *smu_table = &smu->smu_table;
> -	struct gpu_metrics_v1_1 *gpu_metrics =
> -		(struct gpu_metrics_v1_1 *)smu_table->gpu_metrics_table;
> +	struct gpu_metrics_v1_3 *gpu_metrics =
> +		(struct gpu_metrics_v1_3 *)smu_table->gpu_metrics_table;
>   	SmuMetrics_NV12_legacy_t metrics;
>   	int ret = 0;
> +	int freq = 0, dpm = 0;
>   
>   	mutex_lock(&smu->metrics_lock);
>   
> @@ -2774,7 +2789,7 @@ static ssize_t navi12_get_legacy_gpu_metrics(struct smu_context *smu,
>   
>   	mutex_unlock(&smu->metrics_lock);
>   
> -	smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 1);
> +	smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 3);
>   
>   	gpu_metrics->temperature_edge = metrics.TemperatureEdge;
>   	gpu_metrics->temperature_hotspot = metrics.TemperatureHotspot;
> @@ -2814,19 +2829,26 @@ static ssize_t navi12_get_legacy_gpu_metrics(struct smu_context *smu,
>   
>   	gpu_metrics->system_clock_counter = ktime_get_boottime_ns();
>   
> +	gpu_metrics->voltage_gfx = (155000 - 625 * metrics.CurrGfxVoltageOffset) / 100;
> +	gpu_metrics->voltage_mem = (155000 - 625 * metrics.CurrMemVidOffset) / 100;
> +	gpu_metrics->voltage_soc = (155000 - 625 * metrics.CurrSocVoltageOffset) / 100;
> +
>   	*table = (void *)gpu_metrics;
>   
> -	return sizeof(struct gpu_metrics_v1_1);
> +	return sizeof(struct gpu_metrics_v1_3);
> +out:
> +	return ret;
>   }
>   
>   static ssize_t navi12_get_gpu_metrics(struct smu_context *smu,
>   				      void **table)
>   {
>   	struct smu_table_context *smu_table = &smu->smu_table;
> -	struct gpu_metrics_v1_1 *gpu_metrics =
> -		(struct gpu_metrics_v1_1 *)smu_table->gpu_metrics_table;
> +	struct gpu_metrics_v1_3 *gpu_metrics =
> +		(struct gpu_metrics_v1_3 *)smu_table->gpu_metrics_table;
>   	SmuMetrics_NV12_t metrics;
>   	int ret = 0;
> +	int freq = 0, dpm = 0;
>   
>   	mutex_lock(&smu->metrics_lock);
>   
> @@ -2842,7 +2864,7 @@ static ssize_t navi12_get_gpu_metrics(struct smu_context *smu,
>   
>   	mutex_unlock(&smu->metrics_lock);
>   
> -	smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 1);
> +	smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 3);
>   
>   	gpu_metrics->temperature_edge = metrics.TemperatureEdge;
>   	gpu_metrics->temperature_hotspot = metrics.TemperatureHotspot;
> @@ -2884,9 +2906,15 @@ static ssize_t navi12_get_gpu_metrics(struct smu_context *smu,
>   
>   	gpu_metrics->system_clock_counter = ktime_get_boottime_ns();
>   
> +	gpu_metrics->voltage_gfx = (155000 - 625 * metrics.CurrGfxVoltageOffset) / 100;
> +	gpu_metrics->voltage_mem = (155000 - 625 * metrics.CurrMemVidOffset) / 100;
> +	gpu_metrics->voltage_soc = (155000 - 625 * metrics.CurrSocVoltageOffset) / 100;
> +
>   	*table = (void *)gpu_metrics;
>   
> -	return sizeof(struct gpu_metrics_v1_1);
> +	return sizeof(struct gpu_metrics_v1_3);
> +out:
> +	return ret;
>   }
>   
>   static ssize_t navi1x_get_gpu_metrics(struct smu_context *smu,

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