* [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu
@ 2019-07-23 9:39 Kenneth Feng
[not found] ` <1563874746-6318-1-git-send-email-kenneth.feng-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Kenneth Feng @ 2019-07-23 9:39 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: Kenneth Feng
change the smu_read_sensor sequence to:
asic specific sensor read -> smu v11 specific sensor read -> smu v11 common sensor read
Signed-off-by: Kenneth Feng <kenneth.feng@amd.com>
---
drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 3 +++
drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 4 ++--
drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 5 ++++-
drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 8 ++++----
drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 5 ++++-
5 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
index 416f9a8..8ff18c8 100644
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
@@ -315,6 +315,9 @@ int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor,
{
int ret = 0;
+ if(!data || !size)
+ return -EINVAL;
+
switch (sensor) {
case AMDGPU_PP_SENSOR_STABLE_PSTATE_SCLK:
*((uint32_t *)data) = smu->pstate_sclk;
diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
index b702c9e..fabb373 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
@@ -819,9 +819,9 @@ struct smu_funcs
#define smu_start_thermal_control(smu) \
((smu)->funcs->start_thermal_control? (smu)->funcs->start_thermal_control((smu)) : 0)
#define smu_read_sensor(smu, sensor, data, size) \
- ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : 0)
-#define smu_asic_read_sensor(smu, sensor, data, size) \
((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : 0)
+#define smu_smc_read_sensor(smu, sensor, data, size) \
+ ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL)
#define smu_get_power_profile_mode(smu, buf) \
((smu)->ppt_funcs->get_power_profile_mode ? (smu)->ppt_funcs->get_power_profile_mode((smu), buf) : 0)
#define smu_set_power_profile_mode(smu, param, param_size) \
diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
index c8ce9bb..6409718 100644
--- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
@@ -1366,6 +1366,9 @@ static int navi10_read_sensor(struct smu_context *smu,
struct smu_table_context *table_context = &smu->smu_table;
PPTable_t *pptable = table_context->driver_pptable;
+ if(!data || !size)
+ return -EINVAL;
+
switch (sensor) {
case AMDGPU_PP_SENSOR_MAX_FAN_RPM:
*(uint32_t *)data = pptable->FanMaximumRpm;
@@ -1387,7 +1390,7 @@ static int navi10_read_sensor(struct smu_context *smu,
*size = 4;
break;
default:
- return -EINVAL;
+ ret = smu_smc_read_sensor(smu, sensor, data, size);
}
return ret;
diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
index e3a1784..5267b68 100644
--- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
+++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
@@ -1267,6 +1267,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu,
void *data, uint32_t *size)
{
int ret = 0;
+
+ if(!data || !size)
+ return -EINVAL;
+
switch (sensor) {
case AMDGPU_PP_SENSOR_GFX_MCLK:
ret = smu_get_current_clk_freq(smu, SMU_UCLK, (uint32_t *)data);
@@ -1289,10 +1293,6 @@ static int smu_v11_0_read_sensor(struct smu_context *smu,
break;
}
- /* try get sensor data by asic */
- if (ret)
- ret = smu_asic_read_sensor(smu, sensor, data, size);
-
if (ret)
*size = 0;
diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
index 9ead361..e864a54 100644
--- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
@@ -3163,6 +3163,9 @@ static int vega20_read_sensor(struct smu_context *smu,
struct smu_table_context *table_context = &smu->smu_table;
PPTable_t *pptable = table_context->driver_pptable;
+ if(!data || !size)
+ return -EINVAL;
+
switch (sensor) {
case AMDGPU_PP_SENSOR_MAX_FAN_RPM:
*(uint32_t *)data = pptable->FanMaximumRpm;
@@ -3186,7 +3189,7 @@ static int vega20_read_sensor(struct smu_context *smu,
*size = 4;
break;
default:
- return -EINVAL;
+ ret = smu_smc_read_sensor(smu, sensor, data, size);
}
return ret;
--
2.7.4
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread[parent not found: <1563874746-6318-1-git-send-email-kenneth.feng-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu [not found] ` <1563874746-6318-1-git-send-email-kenneth.feng-5C7GfCeVMHo@public.gmane.org> @ 2019-07-23 9:43 ` Wang, Kevin(Yang) [not found] ` <MN2PR12MB3296043F281A9E7345179AE1A2C70-rweVpJHSKTqAm9ToKNQgFgdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> 0 siblings, 1 reply; 10+ messages in thread From: Wang, Kevin(Yang) @ 2019-07-23 9:43 UTC (permalink / raw) To: Feng, Kenneth, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Deucher, Alexander [-- Attachment #1.1: Type: text/plain, Size: 6389 bytes --] it looks fine for me, please @Deucher, Alexander<mailto:Alexander.Deucher-5C7GfCeVMHo@public.gmane.org> double check confirm. Reviewed-by: Kevin Wang <kevin1.wang-5C7GfCeVMHo@public.gmane.org> Best Regards, Kevin ________________________________ From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of Kenneth Feng <kenneth.feng-5C7GfCeVMHo@public.gmane.org> Sent: Tuesday, July 23, 2019 5:39 PM To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> Cc: Feng, Kenneth <Kenneth.Feng-5C7GfCeVMHo@public.gmane.org> Subject: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu change the smu_read_sensor sequence to: asic specific sensor read -> smu v11 specific sensor read -> smu v11 common sensor read Signed-off-by: Kenneth Feng <kenneth.feng-5C7GfCeVMHo@public.gmane.org> --- drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 3 +++ drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 4 ++-- drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 5 ++++- drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 8 ++++---- drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 5 ++++- 5 files changed, 17 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c index 416f9a8..8ff18c8 100644 --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c @@ -315,6 +315,9 @@ int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, { int ret = 0; + if(!data || !size) + return -EINVAL; + switch (sensor) { case AMDGPU_PP_SENSOR_STABLE_PSTATE_SCLK: *((uint32_t *)data) = smu->pstate_sclk; diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h index b702c9e..fabb373 100644 --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h @@ -819,9 +819,9 @@ struct smu_funcs #define smu_start_thermal_control(smu) \ ((smu)->funcs->start_thermal_control? (smu)->funcs->start_thermal_control((smu)) : 0) #define smu_read_sensor(smu, sensor, data, size) \ - ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : 0) -#define smu_asic_read_sensor(smu, sensor, data, size) \ ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : 0) +#define smu_smc_read_sensor(smu, sensor, data, size) \ + ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL) #define smu_get_power_profile_mode(smu, buf) \ ((smu)->ppt_funcs->get_power_profile_mode ? (smu)->ppt_funcs->get_power_profile_mode((smu), buf) : 0) #define smu_set_power_profile_mode(smu, param, param_size) \ diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c index c8ce9bb..6409718 100644 --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c @@ -1366,6 +1366,9 @@ static int navi10_read_sensor(struct smu_context *smu, struct smu_table_context *table_context = &smu->smu_table; PPTable_t *pptable = table_context->driver_pptable; + if(!data || !size) + return -EINVAL; + switch (sensor) { case AMDGPU_PP_SENSOR_MAX_FAN_RPM: *(uint32_t *)data = pptable->FanMaximumRpm; @@ -1387,7 +1390,7 @@ static int navi10_read_sensor(struct smu_context *smu, *size = 4; break; default: - return -EINVAL; + ret = smu_smc_read_sensor(smu, sensor, data, size); } return ret; diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c index e3a1784..5267b68 100644 --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c @@ -1267,6 +1267,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, void *data, uint32_t *size) { int ret = 0; + + if(!data || !size) + return -EINVAL; + switch (sensor) { case AMDGPU_PP_SENSOR_GFX_MCLK: ret = smu_get_current_clk_freq(smu, SMU_UCLK, (uint32_t *)data); @@ -1289,10 +1293,6 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, break; } - /* try get sensor data by asic */ - if (ret) - ret = smu_asic_read_sensor(smu, sensor, data, size); - if (ret) *size = 0; diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c index 9ead361..e864a54 100644 --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c @@ -3163,6 +3163,9 @@ static int vega20_read_sensor(struct smu_context *smu, struct smu_table_context *table_context = &smu->smu_table; PPTable_t *pptable = table_context->driver_pptable; + if(!data || !size) + return -EINVAL; + switch (sensor) { case AMDGPU_PP_SENSOR_MAX_FAN_RPM: *(uint32_t *)data = pptable->FanMaximumRpm; @@ -3186,7 +3189,7 @@ static int vega20_read_sensor(struct smu_context *smu, *size = 4; break; default: - return -EINVAL; + ret = smu_smc_read_sensor(smu, sensor, data, size); } return ret; -- 2.7.4 _______________________________________________ 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 To see the collection of prior postings to the list, visit the amd-gfx Archives.. Using amd-gfx: To post a message to all the list members, send email to amd-gfx-PD4FTy7X32lNgt0PjOBp9xlNPtJONSTn@public.gmane.org You can subscribe to the list, or change your existing subscription, in the sections below. [-- Attachment #1.2: Type: text/html, Size: 13164 bytes --] [-- Attachment #2: Type: text/plain, Size: 153 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
[parent not found: <MN2PR12MB3296043F281A9E7345179AE1A2C70-rweVpJHSKTqAm9ToKNQgFgdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>]
* Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu [not found] ` <MN2PR12MB3296043F281A9E7345179AE1A2C70-rweVpJHSKTqAm9ToKNQgFgdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> @ 2019-07-23 15:06 ` Deucher, Alexander 0 siblings, 0 replies; 10+ messages in thread From: Deucher, Alexander @ 2019-07-23 15:06 UTC (permalink / raw) To: Wang, Kevin(Yang), Feng, Kenneth, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW [-- Attachment #1.1: Type: text/plain, Size: 6958 bytes --] Looks good to me. Acked-by: Alex Deucher <alexander.deucher-5C7GfCeVMHo@public.gmane.org> ________________________________ From: Wang, Kevin(Yang) <Kevin1.Wang-5C7GfCeVMHo@public.gmane.org> Sent: Tuesday, July 23, 2019 5:43 AM To: Feng, Kenneth <Kenneth.Feng-5C7GfCeVMHo@public.gmane.org>; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>; Deucher, Alexander <Alexander.Deucher-5C7GfCeVMHo@public.gmane.org> Subject: Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu it looks fine for me, please @Deucher, Alexander<mailto:Alexander.Deucher-5C7GfCeVMHo@public.gmane.org> double check confirm. Reviewed-by: Kevin Wang <kevin1.wang-5C7GfCeVMHo@public.gmane.org> Best Regards, Kevin ________________________________ From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of Kenneth Feng <kenneth.feng-5C7GfCeVMHo@public.gmane.org> Sent: Tuesday, July 23, 2019 5:39 PM To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> Cc: Feng, Kenneth <Kenneth.Feng-5C7GfCeVMHo@public.gmane.org> Subject: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu change the smu_read_sensor sequence to: asic specific sensor read -> smu v11 specific sensor read -> smu v11 common sensor read Signed-off-by: Kenneth Feng <kenneth.feng-5C7GfCeVMHo@public.gmane.org> --- drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 3 +++ drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 4 ++-- drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 5 ++++- drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 8 ++++---- drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 5 ++++- 5 files changed, 17 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c index 416f9a8..8ff18c8 100644 --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c @@ -315,6 +315,9 @@ int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, { int ret = 0; + if(!data || !size) + return -EINVAL; + switch (sensor) { case AMDGPU_PP_SENSOR_STABLE_PSTATE_SCLK: *((uint32_t *)data) = smu->pstate_sclk; diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h index b702c9e..fabb373 100644 --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h @@ -819,9 +819,9 @@ struct smu_funcs #define smu_start_thermal_control(smu) \ ((smu)->funcs->start_thermal_control? (smu)->funcs->start_thermal_control((smu)) : 0) #define smu_read_sensor(smu, sensor, data, size) \ - ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : 0) -#define smu_asic_read_sensor(smu, sensor, data, size) \ ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : 0) +#define smu_smc_read_sensor(smu, sensor, data, size) \ + ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL) #define smu_get_power_profile_mode(smu, buf) \ ((smu)->ppt_funcs->get_power_profile_mode ? (smu)->ppt_funcs->get_power_profile_mode((smu), buf) : 0) #define smu_set_power_profile_mode(smu, param, param_size) \ diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c index c8ce9bb..6409718 100644 --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c @@ -1366,6 +1366,9 @@ static int navi10_read_sensor(struct smu_context *smu, struct smu_table_context *table_context = &smu->smu_table; PPTable_t *pptable = table_context->driver_pptable; + if(!data || !size) + return -EINVAL; + switch (sensor) { case AMDGPU_PP_SENSOR_MAX_FAN_RPM: *(uint32_t *)data = pptable->FanMaximumRpm; @@ -1387,7 +1390,7 @@ static int navi10_read_sensor(struct smu_context *smu, *size = 4; break; default: - return -EINVAL; + ret = smu_smc_read_sensor(smu, sensor, data, size); } return ret; diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c index e3a1784..5267b68 100644 --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c @@ -1267,6 +1267,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, void *data, uint32_t *size) { int ret = 0; + + if(!data || !size) + return -EINVAL; + switch (sensor) { case AMDGPU_PP_SENSOR_GFX_MCLK: ret = smu_get_current_clk_freq(smu, SMU_UCLK, (uint32_t *)data); @@ -1289,10 +1293,6 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, break; } - /* try get sensor data by asic */ - if (ret) - ret = smu_asic_read_sensor(smu, sensor, data, size); - if (ret) *size = 0; diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c index 9ead361..e864a54 100644 --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c @@ -3163,6 +3163,9 @@ static int vega20_read_sensor(struct smu_context *smu, struct smu_table_context *table_context = &smu->smu_table; PPTable_t *pptable = table_context->driver_pptable; + if(!data || !size) + return -EINVAL; + switch (sensor) { case AMDGPU_PP_SENSOR_MAX_FAN_RPM: *(uint32_t *)data = pptable->FanMaximumRpm; @@ -3186,7 +3189,7 @@ static int vega20_read_sensor(struct smu_context *smu, *size = 4; break; default: - return -EINVAL; + ret = smu_smc_read_sensor(smu, sensor, data, size); } return ret; -- 2.7.4 _______________________________________________ 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 To see the collection of prior postings to the list, visit the amd-gfx Archives.. Using amd-gfx: To post a message to all the list members, send email to amd-gfx-PD4FTy7X32lNgt0PjOBp9xlNPtJONSTn@public.gmane.org You can subscribe to the list, or change your existing subscription, in the sections below. [-- Attachment #1.2: Type: text/html, Size: 14360 bytes --] [-- Attachment #2: Type: text/plain, Size: 153 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu
@ 2019-07-19 11:22 Wang, Kevin(Yang)
[not found] ` <20190719112232.28485-1-kevin1.wang-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Wang, Kevin(Yang) @ 2019-07-19 11:22 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Huang, Ray, Feng, Kenneth, Wang, Kevin(Yang)
each asic maybe has different read sensor method.
so change read sensor sequence in smu.
read sensor sequence:
asic sensor --> smc sensor (smu 11...) --> default_sensor (common)
Signed-off-by: Kevin Wang <kevin1.wang@amd.com>
---
drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 26 +++++++++++++++++--
.../gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 9 ++++---
drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 3 +++
drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 10 +++----
drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 3 +++
5 files changed, 40 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
index 05b91bc5054c..85269f86cae2 100644
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
@@ -284,11 +284,14 @@ int smu_get_power_num_states(struct smu_context *smu,
return 0;
}
-int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor,
- void *data, uint32_t *size)
+int smu_default_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor,
+ void *data, uint32_t *size)
{
int ret = 0;
+ if (!data || !size)
+ return -EINVAL;
+
switch (sensor) {
case AMDGPU_PP_SENSOR_STABLE_PSTATE_SCLK:
*((uint32_t *)data) = smu->pstate_sclk;
@@ -321,6 +324,25 @@ int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor,
return ret;
}
+int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor,
+ void *data, uint32_t *size)
+{
+ int ret = 0;
+
+ if (!data || !size)
+ return -EINVAL;
+
+ /* handle sensor sequence: asic --> ip level --> default */
+ ret = smu_asic_read_sensor(smu, sensor, data, size);
+ if (ret) {
+ ret = smu_smc_read_sensor(smu, sensor, data, size);
+ if (ret)
+ ret = smu_default_read_sensor(smu, sensor, data, size);
+ }
+
+ return ret;
+}
+
int smu_update_table(struct smu_context *smu, enum smu_table_id table_index, int argument,
void *table_data, bool drv2smu)
{
diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
index 34093ddca105..462bae8d62aa 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
@@ -820,10 +820,10 @@ struct smu_funcs
((smu)->ppt_funcs->set_thermal_fan_table ? (smu)->ppt_funcs->set_thermal_fan_table((smu)) : 0)
#define smu_start_thermal_control(smu) \
((smu)->funcs->start_thermal_control? (smu)->funcs->start_thermal_control((smu)) : 0)
-#define smu_read_sensor(smu, sensor, data, size) \
- ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : 0)
+#define smu_smc_read_sensor(smu, sensor, data, size) \
+ ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL)
#define smu_asic_read_sensor(smu, sensor, data, size) \
- ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : 0)
+ ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL)
#define smu_get_power_profile_mode(smu, buf) \
((smu)->ppt_funcs->get_power_profile_mode ? (smu)->ppt_funcs->get_power_profile_mode((smu), buf) : 0)
#define smu_set_power_profile_mode(smu, param, param_size) \
@@ -989,5 +989,6 @@ enum amd_dpm_forced_level smu_get_performance_level(struct smu_context *smu);
int smu_force_performance_level(struct smu_context *smu, enum amd_dpm_forced_level level);
int smu_set_display_count(struct smu_context *smu, uint32_t count);
bool smu_clk_dpm_is_enabled(struct smu_context *smu, enum smu_clk_type clk_type);
-
+int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor,
+ void *data, uint32_t *size);
#endif
diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
index 46e2913e4af4..0a53695785b6 100644
--- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
@@ -1365,6 +1365,9 @@ static int navi10_read_sensor(struct smu_context *smu,
struct smu_table_context *table_context = &smu->smu_table;
PPTable_t *pptable = table_context->driver_pptable;
+ if (!data || !size)
+ return -EINVAL;
+
switch (sensor) {
case AMDGPU_PP_SENSOR_MAX_FAN_RPM:
*(uint32_t *)data = pptable->FanMaximumRpm;
diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
index 76bc157525d0..2679b6ff6ca3 100644
--- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
+++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
@@ -1267,6 +1267,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu,
void *data, uint32_t *size)
{
int ret = 0;
+
+ if (!data || !size)
+ return -EINVAL;
+
switch (sensor) {
case AMDGPU_PP_SENSOR_GFX_MCLK:
ret = smu_get_current_clk_freq(smu, SMU_UCLK, (uint32_t *)data);
@@ -1285,14 +1289,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu,
*size = 4;
break;
default:
- ret = smu_common_read_sensor(smu, sensor, data, size);
+ ret = -EINVAL;
break;
}
- /* try get sensor data by asic */
- if (ret)
- ret = smu_asic_read_sensor(smu, sensor, data, size);
-
if (ret)
*size = 0;
diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
index bcd0efaf7bbd..b44ec7c670c5 100644
--- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
@@ -3146,6 +3146,9 @@ static int vega20_read_sensor(struct smu_context *smu,
struct smu_table_context *table_context = &smu->smu_table;
PPTable_t *pptable = table_context->driver_pptable;
+ if (!data || !size)
+ return -EINVAL;
+
switch (sensor) {
case AMDGPU_PP_SENSOR_MAX_FAN_RPM:
*(uint32_t *)data = pptable->FanMaximumRpm;
--
2.22.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread[parent not found: <20190719112232.28485-1-kevin1.wang-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu [not found] ` <20190719112232.28485-1-kevin1.wang-5C7GfCeVMHo@public.gmane.org> @ 2019-07-19 15:17 ` Alex Deucher [not found] ` <CADnq5_P2UVmm4P1myih0UOQ2nvLDB01zdaFN9v7p423QeLASKg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 10+ messages in thread From: Alex Deucher @ 2019-07-19 15:17 UTC (permalink / raw) To: Wang, Kevin(Yang) Cc: Huang, Ray, Feng, Kenneth, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On Fri, Jul 19, 2019 at 7:23 AM Wang, Kevin(Yang) <Kevin1.Wang@amd.com> wrote: > > each asic maybe has different read sensor method. > so change read sensor sequence in smu. > > read sensor sequence: > asic sensor --> smc sensor (smu 11...) --> default_sensor (common) I think this makes sense. That said, the current swSMU callback structures are really confusing. I think we should switch to a single set of per asic callbacks and then add common helpers. Then for asics where it makes sense we can just use the helper as the callback for all relevant asics. If they need something asic specific, use the asic specific function. That should avoid the current mix of callbacks and make it clearer what code gets used when. Alex > > Signed-off-by: Kevin Wang <kevin1.wang@amd.com> > --- > drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 26 +++++++++++++++++-- > .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 9 ++++--- > drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 3 +++ > drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 10 +++---- > drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 3 +++ > 5 files changed, 40 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > index 05b91bc5054c..85269f86cae2 100644 > --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > @@ -284,11 +284,14 @@ int smu_get_power_num_states(struct smu_context *smu, > return 0; > } > > -int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > - void *data, uint32_t *size) > +int smu_default_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > + void *data, uint32_t *size) > { > int ret = 0; > > + if (!data || !size) > + return -EINVAL; > + > switch (sensor) { > case AMDGPU_PP_SENSOR_STABLE_PSTATE_SCLK: > *((uint32_t *)data) = smu->pstate_sclk; > @@ -321,6 +324,25 @@ int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > return ret; > } > > +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > + void *data, uint32_t *size) > +{ > + int ret = 0; > + > + if (!data || !size) > + return -EINVAL; > + > + /* handle sensor sequence: asic --> ip level --> default */ > + ret = smu_asic_read_sensor(smu, sensor, data, size); > + if (ret) { > + ret = smu_smc_read_sensor(smu, sensor, data, size); > + if (ret) > + ret = smu_default_read_sensor(smu, sensor, data, size); > + } > + > + return ret; > +} > + > int smu_update_table(struct smu_context *smu, enum smu_table_id table_index, int argument, > void *table_data, bool drv2smu) > { > diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > index 34093ddca105..462bae8d62aa 100644 > --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > @@ -820,10 +820,10 @@ struct smu_funcs > ((smu)->ppt_funcs->set_thermal_fan_table ? (smu)->ppt_funcs->set_thermal_fan_table((smu)) : 0) > #define smu_start_thermal_control(smu) \ > ((smu)->funcs->start_thermal_control? (smu)->funcs->start_thermal_control((smu)) : 0) > -#define smu_read_sensor(smu, sensor, data, size) \ > - ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : 0) > +#define smu_smc_read_sensor(smu, sensor, data, size) \ > + ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL) > #define smu_asic_read_sensor(smu, sensor, data, size) \ > - ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : 0) > + ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL) > #define smu_get_power_profile_mode(smu, buf) \ > ((smu)->ppt_funcs->get_power_profile_mode ? (smu)->ppt_funcs->get_power_profile_mode((smu), buf) : 0) > #define smu_set_power_profile_mode(smu, param, param_size) \ > @@ -989,5 +989,6 @@ enum amd_dpm_forced_level smu_get_performance_level(struct smu_context *smu); > int smu_force_performance_level(struct smu_context *smu, enum amd_dpm_forced_level level); > int smu_set_display_count(struct smu_context *smu, uint32_t count); > bool smu_clk_dpm_is_enabled(struct smu_context *smu, enum smu_clk_type clk_type); > - > +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > + void *data, uint32_t *size); > #endif > diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > index 46e2913e4af4..0a53695785b6 100644 > --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > @@ -1365,6 +1365,9 @@ static int navi10_read_sensor(struct smu_context *smu, > struct smu_table_context *table_context = &smu->smu_table; > PPTable_t *pptable = table_context->driver_pptable; > > + if (!data || !size) > + return -EINVAL; > + > switch (sensor) { > case AMDGPU_PP_SENSOR_MAX_FAN_RPM: > *(uint32_t *)data = pptable->FanMaximumRpm; > diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > index 76bc157525d0..2679b6ff6ca3 100644 > --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > @@ -1267,6 +1267,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, > void *data, uint32_t *size) > { > int ret = 0; > + > + if (!data || !size) > + return -EINVAL; > + > switch (sensor) { > case AMDGPU_PP_SENSOR_GFX_MCLK: > ret = smu_get_current_clk_freq(smu, SMU_UCLK, (uint32_t *)data); > @@ -1285,14 +1289,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, > *size = 4; > break; > default: > - ret = smu_common_read_sensor(smu, sensor, data, size); > + ret = -EINVAL; > break; > } > > - /* try get sensor data by asic */ > - if (ret) > - ret = smu_asic_read_sensor(smu, sensor, data, size); > - > if (ret) > *size = 0; > > diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > index bcd0efaf7bbd..b44ec7c670c5 100644 > --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > @@ -3146,6 +3146,9 @@ static int vega20_read_sensor(struct smu_context *smu, > struct smu_table_context *table_context = &smu->smu_table; > PPTable_t *pptable = table_context->driver_pptable; > > + if (!data || !size) > + return -EINVAL; > + > switch (sensor) { > case AMDGPU_PP_SENSOR_MAX_FAN_RPM: > *(uint32_t *)data = pptable->FanMaximumRpm; > -- > 2.22.0 > > _______________________________________________ > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <CADnq5_P2UVmm4P1myih0UOQ2nvLDB01zdaFN9v7p423QeLASKg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu [not found] ` <CADnq5_P2UVmm4P1myih0UOQ2nvLDB01zdaFN9v7p423QeLASKg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2019-07-19 16:01 ` Wang, Kevin(Yang) [not found] ` <MN2PR12MB329684D6E227BE0132F67A8EA2CB0-rweVpJHSKTqAm9ToKNQgFgdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> 0 siblings, 1 reply; 10+ messages in thread From: Wang, Kevin(Yang) @ 2019-07-19 16:01 UTC (permalink / raw) To: Alex Deucher Cc: Huang, Ray, Feng, Kenneth, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW [-- Attachment #1.1: Type: text/plain, Size: 9252 bytes --] ________________________________ From: Alex Deucher <alexdeucher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Sent: Friday, July 19, 2019 11:17 PM To: Wang, Kevin(Yang) <Kevin1.Wang-5C7GfCeVMHo@public.gmane.org> Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>; Huang, Ray <Ray.Huang-5C7GfCeVMHo@public.gmane.org>; Feng, Kenneth <Kenneth.Feng-5C7GfCeVMHo@public.gmane.org> Subject: Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu On Fri, Jul 19, 2019 at 7:23 AM Wang, Kevin(Yang) <Kevin1.Wang-5C7GfCeVMHo@public.gmane.org> wrote: > > each asic maybe has different read sensor method. > so change read sensor sequence in smu. > > read sensor sequence: > asic sensor --> smc sensor (smu 11...) --> default_sensor (common) I think this makes sense. That said, the current swSMU callback structures are really confusing. I think we should switch to a single set of per asic callbacks and then add common helpers. Then for asics where it makes sense we can just use the helper as the callback for all relevant asics. If they need something asic specific, use the asic specific function. That should avoid the current mix of callbacks and make it clearer what code gets used when. Alex [kevin]: thanks review, in current code, the read sensor related function is not very clear, so i want to refine them. but I'm not sure which way to write good code logic. way 1: provide a puiblic function named smu_read_sensor as public smu api for other kenel module, like this patch. this function will try to get value from asic or smu ip level or common, call them in turn according to the rules. way 2: define a maco named smu_read_sensor as public api, implement it in xxx_ppt.c file, if can't handle sensor type in xxx_ppt.c, then call helper in smu_v11_0.c, then call amdgpu_smu.c helper. in this way, it means we must implement this callback function in xxx_ppt.c. if need to support new asic, we should add some dulicated code in xxx_ppt.c, if not the smu_read_sensor api is not work well. in smu module, use many macros as module public api, it is impossible to tell at what level these macros implement specific code logic. so i want to refine them. do you think which way is good for this case? thanks. > > Signed-off-by: Kevin Wang <kevin1.wang-5C7GfCeVMHo@public.gmane.org> > --- > drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 26 +++++++++++++++++-- > .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 9 ++++--- > drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 3 +++ > drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 10 +++---- > drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 3 +++ > 5 files changed, 40 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > index 05b91bc5054c..85269f86cae2 100644 > --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > @@ -284,11 +284,14 @@ int smu_get_power_num_states(struct smu_context *smu, > return 0; > } > > -int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > - void *data, uint32_t *size) > +int smu_default_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > + void *data, uint32_t *size) > { > int ret = 0; > > + if (!data || !size) > + return -EINVAL; > + > switch (sensor) { > case AMDGPU_PP_SENSOR_STABLE_PSTATE_SCLK: > *((uint32_t *)data) = smu->pstate_sclk; > @@ -321,6 +324,25 @@ int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > return ret; > } > > +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > + void *data, uint32_t *size) > +{ > + int ret = 0; > + > + if (!data || !size) > + return -EINVAL; > + > + /* handle sensor sequence: asic --> ip level --> default */ > + ret = smu_asic_read_sensor(smu, sensor, data, size); > + if (ret) { > + ret = smu_smc_read_sensor(smu, sensor, data, size); > + if (ret) > + ret = smu_default_read_sensor(smu, sensor, data, size); > + } > + > + return ret; > +} > + > int smu_update_table(struct smu_context *smu, enum smu_table_id table_index, int argument, > void *table_data, bool drv2smu) > { > diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > index 34093ddca105..462bae8d62aa 100644 > --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > @@ -820,10 +820,10 @@ struct smu_funcs > ((smu)->ppt_funcs->set_thermal_fan_table ? (smu)->ppt_funcs->set_thermal_fan_table((smu)) : 0) > #define smu_start_thermal_control(smu) \ > ((smu)->funcs->start_thermal_control? (smu)->funcs->start_thermal_control((smu)) : 0) > -#define smu_read_sensor(smu, sensor, data, size) \ > - ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : 0) > +#define smu_smc_read_sensor(smu, sensor, data, size) \ > + ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL) > #define smu_asic_read_sensor(smu, sensor, data, size) \ > - ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : 0) > + ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL) > #define smu_get_power_profile_mode(smu, buf) \ > ((smu)->ppt_funcs->get_power_profile_mode ? (smu)->ppt_funcs->get_power_profile_mode((smu), buf) : 0) > #define smu_set_power_profile_mode(smu, param, param_size) \ > @@ -989,5 +989,6 @@ enum amd_dpm_forced_level smu_get_performance_level(struct smu_context *smu); > int smu_force_performance_level(struct smu_context *smu, enum amd_dpm_forced_level level); > int smu_set_display_count(struct smu_context *smu, uint32_t count); > bool smu_clk_dpm_is_enabled(struct smu_context *smu, enum smu_clk_type clk_type); > - > +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > + void *data, uint32_t *size); > #endif > diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > index 46e2913e4af4..0a53695785b6 100644 > --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > @@ -1365,6 +1365,9 @@ static int navi10_read_sensor(struct smu_context *smu, > struct smu_table_context *table_context = &smu->smu_table; > PPTable_t *pptable = table_context->driver_pptable; > > + if (!data || !size) > + return -EINVAL; > + > switch (sensor) { > case AMDGPU_PP_SENSOR_MAX_FAN_RPM: > *(uint32_t *)data = pptable->FanMaximumRpm; > diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > index 76bc157525d0..2679b6ff6ca3 100644 > --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > @@ -1267,6 +1267,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, > void *data, uint32_t *size) > { > int ret = 0; > + > + if (!data || !size) > + return -EINVAL; > + > switch (sensor) { > case AMDGPU_PP_SENSOR_GFX_MCLK: > ret = smu_get_current_clk_freq(smu, SMU_UCLK, (uint32_t *)data); > @@ -1285,14 +1289,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, > *size = 4; > break; > default: > - ret = smu_common_read_sensor(smu, sensor, data, size); > + ret = -EINVAL; > break; > } > > - /* try get sensor data by asic */ > - if (ret) > - ret = smu_asic_read_sensor(smu, sensor, data, size); > - > if (ret) > *size = 0; > > diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > index bcd0efaf7bbd..b44ec7c670c5 100644 > --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > @@ -3146,6 +3146,9 @@ static int vega20_read_sensor(struct smu_context *smu, > struct smu_table_context *table_context = &smu->smu_table; > PPTable_t *pptable = table_context->driver_pptable; > > + if (!data || !size) > + return -EINVAL; > + > switch (sensor) { > case AMDGPU_PP_SENSOR_MAX_FAN_RPM: > *(uint32_t *)data = pptable->FanMaximumRpm; > -- > 2.22.0 > > _______________________________________________ > amd-gfx mailing list > amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx [-- Attachment #1.2: Type: text/html, Size: 16785 bytes --] [-- Attachment #2: Type: text/plain, Size: 153 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <MN2PR12MB329684D6E227BE0132F67A8EA2CB0-rweVpJHSKTqAm9ToKNQgFgdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>]
* Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu [not found] ` <MN2PR12MB329684D6E227BE0132F67A8EA2CB0-rweVpJHSKTqAm9ToKNQgFgdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> @ 2019-07-19 16:14 ` Alex Deucher [not found] ` <CADnq5_PDRyxDNn3TK40L0w2KGDUcahrZge7+q_Z06DX3q-xFWw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 10+ messages in thread From: Alex Deucher @ 2019-07-19 16:14 UTC (permalink / raw) To: Wang, Kevin(Yang) Cc: Huang, Ray, Feng, Kenneth, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On Fri, Jul 19, 2019 at 12:01 PM Wang, Kevin(Yang) <Kevin1.Wang@amd.com> wrote: > > > ________________________________ > From: Alex Deucher <alexdeucher@gmail.com> > Sent: Friday, July 19, 2019 11:17 PM > To: Wang, Kevin(Yang) <Kevin1.Wang@amd.com> > Cc: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; Huang, Ray <Ray.Huang@amd.com>; Feng, Kenneth <Kenneth.Feng@amd.com> > Subject: Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu > > On Fri, Jul 19, 2019 at 7:23 AM Wang, Kevin(Yang) <Kevin1.Wang@amd.com> wrote: > > > > each asic maybe has different read sensor method. > > so change read sensor sequence in smu. > > > > read sensor sequence: > > asic sensor --> smc sensor (smu 11...) --> default_sensor (common) > > I think this makes sense. That said, the current swSMU callback > structures are really confusing. I think we should switch to a single > set of per asic callbacks and then add common helpers. Then for asics > where it makes sense we can just use the helper as the callback for > all relevant asics. If they need something asic specific, use the > asic specific function. That should avoid the current mix of > callbacks and make it clearer what code gets used when. > > Alex > > [kevin]: > > thanks review, in current code, the read sensor related function is not very clear, so i want to refine them. > but I'm not sure which way to write good code logic. > > way 1: > > provide a puiblic function named smu_read_sensor as public smu api for other kenel module, like this patch. > this function will try to get value from asic or smu ip level or common, call them in turn according to the rules. > > way 2: > > define a maco named smu_read_sensor as public api, implement it in xxx_ppt.c file, > if can't handle sensor type in xxx_ppt.c, then call helper in smu_v11_0.c, then call amdgpu_smu.c helper. > > in this way, it means we must implement this callback function in xxx_ppt.c. > if need to support new asic, we should add some dulicated code in xxx_ppt.c, if not the smu_read_sensor api is not work well. > in smu module, use many macros as module public api, it is impossible to tell at what level these macros implement specific code logic. > so i want to refine them. > > do you think which way is good for this case? I personally prefer way 2. With way 1, the common functions would just be a wrapper around the asic specific callbacks. The older powerplay code worked that way. If there is something common that needs to be done for all asics, I think that would make sense, but I don't know that we have any cases like that. If we do end up needing something like that, we can always revisit this. I was thinking something like the following: struct smu_asic_funcs { int (*get_current_clock_freq)(); int (*get_fan_speed_rpm)(); ... } Then for cases where two asics use the same SMU interface, you can create a common function. So for vega20, it might look like: static const struct smu_asic_funcs vega20_smu_asic_funcs = { .get_current_clock_freq = smu_v11_0_get_current_clock_freq, .get_fan_speed_rpm = vega20_get_fan_speed_rpm, ... }; and navi10 would look like: static const struct smu_asic_funcs navi10_smu_asic_funcs = { .get_current_clock_freq = smu_v11_0_get_current_clock_freq, .get_fan_speed_rpm = navi10_get_fan_speed_rpm, ... }; Alex > thanks. > > > > > Signed-off-by: Kevin Wang <kevin1.wang@amd.com> > > --- > > drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 26 +++++++++++++++++-- > > .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 9 ++++--- > > drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 3 +++ > > drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 10 +++---- > > drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 3 +++ > > 5 files changed, 40 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > > index 05b91bc5054c..85269f86cae2 100644 > > --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > > +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > > @@ -284,11 +284,14 @@ int smu_get_power_num_states(struct smu_context *smu, > > return 0; > > } > > > > -int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > > - void *data, uint32_t *size) > > +int smu_default_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > > + void *data, uint32_t *size) > > { > > int ret = 0; > > > > + if (!data || !size) > > + return -EINVAL; > > + > > switch (sensor) { > > case AMDGPU_PP_SENSOR_STABLE_PSTATE_SCLK: > > *((uint32_t *)data) = smu->pstate_sclk; > > @@ -321,6 +324,25 @@ int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > > return ret; > > } > > > > +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > > + void *data, uint32_t *size) > > +{ > > + int ret = 0; > > + > > + if (!data || !size) > > + return -EINVAL; > > + > > + /* handle sensor sequence: asic --> ip level --> default */ > > + ret = smu_asic_read_sensor(smu, sensor, data, size); > > + if (ret) { > > + ret = smu_smc_read_sensor(smu, sensor, data, size); > > + if (ret) > > + ret = smu_default_read_sensor(smu, sensor, data, size); > > + } > > + > > + return ret; > > +} > > + > > int smu_update_table(struct smu_context *smu, enum smu_table_id table_index, int argument, > > void *table_data, bool drv2smu) > > { > > diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > > index 34093ddca105..462bae8d62aa 100644 > > --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > > +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > > @@ -820,10 +820,10 @@ struct smu_funcs > > ((smu)->ppt_funcs->set_thermal_fan_table ? (smu)->ppt_funcs->set_thermal_fan_table((smu)) : 0) > > #define smu_start_thermal_control(smu) \ > > ((smu)->funcs->start_thermal_control? (smu)->funcs->start_thermal_control((smu)) : 0) > > -#define smu_read_sensor(smu, sensor, data, size) \ > > - ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : 0) > > +#define smu_smc_read_sensor(smu, sensor, data, size) \ > > + ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL) > > #define smu_asic_read_sensor(smu, sensor, data, size) \ > > - ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : 0) > > + ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL) > > #define smu_get_power_profile_mode(smu, buf) \ > > ((smu)->ppt_funcs->get_power_profile_mode ? (smu)->ppt_funcs->get_power_profile_mode((smu), buf) : 0) > > #define smu_set_power_profile_mode(smu, param, param_size) \ > > @@ -989,5 +989,6 @@ enum amd_dpm_forced_level smu_get_performance_level(struct smu_context *smu); > > int smu_force_performance_level(struct smu_context *smu, enum amd_dpm_forced_level level); > > int smu_set_display_count(struct smu_context *smu, uint32_t count); > > bool smu_clk_dpm_is_enabled(struct smu_context *smu, enum smu_clk_type clk_type); > > - > > +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > > + void *data, uint32_t *size); > > #endif > > diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > > index 46e2913e4af4..0a53695785b6 100644 > > --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > > +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > > @@ -1365,6 +1365,9 @@ static int navi10_read_sensor(struct smu_context *smu, > > struct smu_table_context *table_context = &smu->smu_table; > > PPTable_t *pptable = table_context->driver_pptable; > > > > + if (!data || !size) > > + return -EINVAL; > > + > > switch (sensor) { > > case AMDGPU_PP_SENSOR_MAX_FAN_RPM: > > *(uint32_t *)data = pptable->FanMaximumRpm; > > diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > > index 76bc157525d0..2679b6ff6ca3 100644 > > --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > > +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > > @@ -1267,6 +1267,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, > > void *data, uint32_t *size) > > { > > int ret = 0; > > + > > + if (!data || !size) > > + return -EINVAL; > > + > > switch (sensor) { > > case AMDGPU_PP_SENSOR_GFX_MCLK: > > ret = smu_get_current_clk_freq(smu, SMU_UCLK, (uint32_t *)data); > > @@ -1285,14 +1289,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, > > *size = 4; > > break; > > default: > > - ret = smu_common_read_sensor(smu, sensor, data, size); > > + ret = -EINVAL; > > break; > > } > > > > - /* try get sensor data by asic */ > > - if (ret) > > - ret = smu_asic_read_sensor(smu, sensor, data, size); > > - > > if (ret) > > *size = 0; > > > > diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > > index bcd0efaf7bbd..b44ec7c670c5 100644 > > --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > > +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > > @@ -3146,6 +3146,9 @@ static int vega20_read_sensor(struct smu_context *smu, > > struct smu_table_context *table_context = &smu->smu_table; > > PPTable_t *pptable = table_context->driver_pptable; > > > > + if (!data || !size) > > + return -EINVAL; > > + > > switch (sensor) { > > case AMDGPU_PP_SENSOR_MAX_FAN_RPM: > > *(uint32_t *)data = pptable->FanMaximumRpm; > > -- > > 2.22.0 > > > > _______________________________________________ > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <CADnq5_PDRyxDNn3TK40L0w2KGDUcahrZge7+q_Z06DX3q-xFWw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu [not found] ` <CADnq5_PDRyxDNn3TK40L0w2KGDUcahrZge7+q_Z06DX3q-xFWw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2019-07-19 17:03 ` Wang, Kevin(Yang) [not found] ` <A2EC6125-E08F-4FD2-A56C-E9D48CD3F95F-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 10+ messages in thread From: Wang, Kevin(Yang) @ 2019-07-19 17:03 UTC (permalink / raw) To: Alex Deucher Cc: Huang, Ray, Feng, Kenneth, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW The read sensor function is not same as other one, this function should be handle many different sensor type, I don’t want to handle all sensor type in asic file, because some sensor is very simple, only should be send a message to smc, and some sensor should be parse Table with firmware. Eg: get gfx clk, Only need send message to get value. Because the message already mapped on each asic, so this sensor should be handled in smu_v11.c. Eg: get gpu load, this sensor should be get value from firmware table, so should must be handled in xxx_ppt.c Eg: get pstate clock, It is full software sensor, so it is handled in amdgpu_smu.c In this patch, the smu only want to public one api of smu_read_sensor, and the other sensor macro is helper in smu internally. I want to reduce duplicate code in smu, it will be let bring up new asic easy. Thanks > On Jul 20, 2019, at 12:14 AM, Alex Deucher <alexdeucher@gmail.com> wrote: > >> On Fri, Jul 19, 2019 at 12:01 PM Wang, Kevin(Yang) <Kevin1.Wang@amd.com> wrote: >> >> >> ________________________________ >> From: Alex Deucher <alexdeucher@gmail.com> >> Sent: Friday, July 19, 2019 11:17 PM >> To: Wang, Kevin(Yang) <Kevin1.Wang@amd.com> >> Cc: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; Huang, Ray <Ray.Huang@amd.com>; Feng, Kenneth <Kenneth.Feng@amd.com> >> Subject: Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu >> >>> On Fri, Jul 19, 2019 at 7:23 AM Wang, Kevin(Yang) <Kevin1.Wang@amd.com> wrote: >>> >>> each asic maybe has different read sensor method. >>> so change read sensor sequence in smu. >>> >>> read sensor sequence: >>> asic sensor --> smc sensor (smu 11...) --> default_sensor (common) >> >> I think this makes sense. That said, the current swSMU callback >> structures are really confusing. I think we should switch to a single >> set of per asic callbacks and then add common helpers. Then for asics >> where it makes sense we can just use the helper as the callback for >> all relevant asics. If they need something asic specific, use the >> asic specific function. That should avoid the current mix of >> callbacks and make it clearer what code gets used when. >> >> Alex >> >> [kevin]: >> >> thanks review, in current code, the read sensor related function is not very clear, so i want to refine them. >> but I'm not sure which way to write good code logic. >> >> way 1: >> >> provide a puiblic function named smu_read_sensor as public smu api for other kenel module, like this patch. >> this function will try to get value from asic or smu ip level or common, call them in turn according to the rules. >> >> way 2: >> >> define a maco named smu_read_sensor as public api, implement it in xxx_ppt.c file, >> if can't handle sensor type in xxx_ppt.c, then call helper in smu_v11_0.c, then call amdgpu_smu.c helper. >> >> in this way, it means we must implement this callback function in xxx_ppt.c. >> if need to support new asic, we should add some dulicated code in xxx_ppt.c, if not the smu_read_sensor api is not work well. >> in smu module, use many macros as module public api, it is impossible to tell at what level these macros implement specific code logic. >> so i want to refine them. >> >> do you think which way is good for this case? > > I personally prefer way 2. With way 1, the common functions would > just be a wrapper around the asic specific callbacks. The older > powerplay code worked that way. If there is something common that > needs to be done for all asics, I think that would make sense, but I > don't know that we have any cases like that. If we do end up needing > something like that, we can always revisit this. > > I was thinking something like the following: > > struct smu_asic_funcs { > int (*get_current_clock_freq)(); > int (*get_fan_speed_rpm)(); > ... > } > > Then for cases where two asics use the same SMU interface, you can > create a common function. So for vega20, it might look like: > > static const struct smu_asic_funcs vega20_smu_asic_funcs = > { > .get_current_clock_freq = smu_v11_0_get_current_clock_freq, > .get_fan_speed_rpm = vega20_get_fan_speed_rpm, > ... > }; > > and navi10 would look like: > > static const struct smu_asic_funcs navi10_smu_asic_funcs = > { > .get_current_clock_freq = smu_v11_0_get_current_clock_freq, > .get_fan_speed_rpm = navi10_get_fan_speed_rpm, > ... > }; > > Alex > > >> thanks. >> >>> >>> Signed-off-by: Kevin Wang <kevin1.wang@amd.com> >>> --- >>> drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 26 +++++++++++++++++-- >>> .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 9 ++++--- >>> drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 3 +++ >>> drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 10 +++---- >>> drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 3 +++ >>> 5 files changed, 40 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c >>> index 05b91bc5054c..85269f86cae2 100644 >>> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c >>> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c >>> @@ -284,11 +284,14 @@ int smu_get_power_num_states(struct smu_context *smu, >>> return 0; >>> } >>> >>> -int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, >>> - void *data, uint32_t *size) >>> +int smu_default_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, >>> + void *data, uint32_t *size) >>> { >>> int ret = 0; >>> >>> + if (!data || !size) >>> + return -EINVAL; >>> + >>> switch (sensor) { >>> case AMDGPU_PP_SENSOR_STABLE_PSTATE_SCLK: >>> *((uint32_t *)data) = smu->pstate_sclk; >>> @@ -321,6 +324,25 @@ int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, >>> return ret; >>> } >>> >>> +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, >>> + void *data, uint32_t *size) >>> +{ >>> + int ret = 0; >>> + >>> + if (!data || !size) >>> + return -EINVAL; >>> + >>> + /* handle sensor sequence: asic --> ip level --> default */ >>> + ret = smu_asic_read_sensor(smu, sensor, data, size); >>> + if (ret) { >>> + ret = smu_smc_read_sensor(smu, sensor, data, size); >>> + if (ret) >>> + ret = smu_default_read_sensor(smu, sensor, data, size); >>> + } >>> + >>> + return ret; >>> +} >>> + >>> int smu_update_table(struct smu_context *smu, enum smu_table_id table_index, int argument, >>> void *table_data, bool drv2smu) >>> { >>> diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h >>> index 34093ddca105..462bae8d62aa 100644 >>> --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h >>> +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h >>> @@ -820,10 +820,10 @@ struct smu_funcs >>> ((smu)->ppt_funcs->set_thermal_fan_table ? (smu)->ppt_funcs->set_thermal_fan_table((smu)) : 0) >>> #define smu_start_thermal_control(smu) \ >>> ((smu)->funcs->start_thermal_control? (smu)->funcs->start_thermal_control((smu)) : 0) >>> -#define smu_read_sensor(smu, sensor, data, size) \ >>> - ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : 0) >>> +#define smu_smc_read_sensor(smu, sensor, data, size) \ >>> + ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL) >>> #define smu_asic_read_sensor(smu, sensor, data, size) \ >>> - ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : 0) >>> + ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL) >>> #define smu_get_power_profile_mode(smu, buf) \ >>> ((smu)->ppt_funcs->get_power_profile_mode ? (smu)->ppt_funcs->get_power_profile_mode((smu), buf) : 0) >>> #define smu_set_power_profile_mode(smu, param, param_size) \ >>> @@ -989,5 +989,6 @@ enum amd_dpm_forced_level smu_get_performance_level(struct smu_context *smu); >>> int smu_force_performance_level(struct smu_context *smu, enum amd_dpm_forced_level level); >>> int smu_set_display_count(struct smu_context *smu, uint32_t count); >>> bool smu_clk_dpm_is_enabled(struct smu_context *smu, enum smu_clk_type clk_type); >>> - >>> +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, >>> + void *data, uint32_t *size); >>> #endif >>> diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c >>> index 46e2913e4af4..0a53695785b6 100644 >>> --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c >>> +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c >>> @@ -1365,6 +1365,9 @@ static int navi10_read_sensor(struct smu_context *smu, >>> struct smu_table_context *table_context = &smu->smu_table; >>> PPTable_t *pptable = table_context->driver_pptable; >>> >>> + if (!data || !size) >>> + return -EINVAL; >>> + >>> switch (sensor) { >>> case AMDGPU_PP_SENSOR_MAX_FAN_RPM: >>> *(uint32_t *)data = pptable->FanMaximumRpm; >>> diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c >>> index 76bc157525d0..2679b6ff6ca3 100644 >>> --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c >>> +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c >>> @@ -1267,6 +1267,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, >>> void *data, uint32_t *size) >>> { >>> int ret = 0; >>> + >>> + if (!data || !size) >>> + return -EINVAL; >>> + >>> switch (sensor) { >>> case AMDGPU_PP_SENSOR_GFX_MCLK: >>> ret = smu_get_current_clk_freq(smu, SMU_UCLK, (uint32_t *)data); >>> @@ -1285,14 +1289,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, >>> *size = 4; >>> break; >>> default: >>> - ret = smu_common_read_sensor(smu, sensor, data, size); >>> + ret = -EINVAL; >>> break; >>> } >>> >>> - /* try get sensor data by asic */ >>> - if (ret) >>> - ret = smu_asic_read_sensor(smu, sensor, data, size); >>> - >>> if (ret) >>> *size = 0; >>> >>> diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c >>> index bcd0efaf7bbd..b44ec7c670c5 100644 >>> --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c >>> +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c >>> @@ -3146,6 +3146,9 @@ static int vega20_read_sensor(struct smu_context *smu, >>> struct smu_table_context *table_context = &smu->smu_table; >>> PPTable_t *pptable = table_context->driver_pptable; >>> >>> + if (!data || !size) >>> + return -EINVAL; >>> + >>> switch (sensor) { >>> case AMDGPU_PP_SENSOR_MAX_FAN_RPM: >>> *(uint32_t *)data = pptable->FanMaximumRpm; >>> -- >>> 2.22.0 >>> >>> _______________________________________________ >>> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <A2EC6125-E08F-4FD2-A56C-E9D48CD3F95F-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu [not found] ` <A2EC6125-E08F-4FD2-A56C-E9D48CD3F95F-5C7GfCeVMHo@public.gmane.org> @ 2019-07-19 17:53 ` Alex Deucher [not found] ` <CADnq5_ODiK_pyMZsUmw0T2ivG=_-j5h3ky9vouE-ACEJV+fddQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 10+ messages in thread From: Alex Deucher @ 2019-07-19 17:53 UTC (permalink / raw) To: Wang, Kevin(Yang) Cc: Huang, Ray, Feng, Kenneth, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On Fri, Jul 19, 2019 at 1:03 PM Wang, Kevin(Yang) <Kevin1.Wang@amd.com> wrote: > > The read sensor function is not same as other one, this function should be handle many different sensor type, I don’t want to handle all sensor type in asic file, because some sensor is very simple, only should be send a message to smc, and some sensor should be parse Table with firmware. > > Eg: get gfx clk, > Only need send message to get value. > Because the message already mapped on each asic, so this sensor should be handled in smu_v11.c. > > Eg: get gpu load, > this sensor should be get value from firmware table, so should must be handled in xxx_ppt.c > > Eg: get pstate clock, > It is full software sensor, so it is handled in amdgpu_smu.c > > In this patch, the smu only want to public one api of smu_read_sensor, and the other sensor macro is helper in smu internally. > > I want to reduce duplicate code in smu, it will be let bring up new asic easy. I think it's still pretty straight forward. E.g., in the asic specific read_sensor() callback you do something like this: switch (sensor) { case SENSOR1: case SENSOR2: case SENSOR3: ret = smu_v11_read_sensor_helper(smu, sensor, value); breakl case SENSOR4: ret = vega20_get_sensor4(smu, value); break; default: ... } That way there is less confusion about following callbacks. Alex > > Thanks > > > On Jul 20, 2019, at 12:14 AM, Alex Deucher <alexdeucher@gmail.com> wrote: > > > >> On Fri, Jul 19, 2019 at 12:01 PM Wang, Kevin(Yang) <Kevin1.Wang@amd.com> wrote: > >> > >> > >> ________________________________ > >> From: Alex Deucher <alexdeucher@gmail.com> > >> Sent: Friday, July 19, 2019 11:17 PM > >> To: Wang, Kevin(Yang) <Kevin1.Wang@amd.com> > >> Cc: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; Huang, Ray <Ray.Huang@amd.com>; Feng, Kenneth <Kenneth.Feng@amd.com> > >> Subject: Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu > >> > >>> On Fri, Jul 19, 2019 at 7:23 AM Wang, Kevin(Yang) <Kevin1.Wang@amd.com> wrote: > >>> > >>> each asic maybe has different read sensor method. > >>> so change read sensor sequence in smu. > >>> > >>> read sensor sequence: > >>> asic sensor --> smc sensor (smu 11...) --> default_sensor (common) > >> > >> I think this makes sense. That said, the current swSMU callback > >> structures are really confusing. I think we should switch to a single > >> set of per asic callbacks and then add common helpers. Then for asics > >> where it makes sense we can just use the helper as the callback for > >> all relevant asics. If they need something asic specific, use the > >> asic specific function. That should avoid the current mix of > >> callbacks and make it clearer what code gets used when. > >> > >> Alex > >> > >> [kevin]: > >> > >> thanks review, in current code, the read sensor related function is not very clear, so i want to refine them. > >> but I'm not sure which way to write good code logic. > >> > >> way 1: > >> > >> provide a puiblic function named smu_read_sensor as public smu api for other kenel module, like this patch. > >> this function will try to get value from asic or smu ip level or common, call them in turn according to the rules. > >> > >> way 2: > >> > >> define a maco named smu_read_sensor as public api, implement it in xxx_ppt.c file, > >> if can't handle sensor type in xxx_ppt.c, then call helper in smu_v11_0.c, then call amdgpu_smu.c helper. > >> > >> in this way, it means we must implement this callback function in xxx_ppt.c. > >> if need to support new asic, we should add some dulicated code in xxx_ppt.c, if not the smu_read_sensor api is not work well. > >> in smu module, use many macros as module public api, it is impossible to tell at what level these macros implement specific code logic. > >> so i want to refine them. > >> > >> do you think which way is good for this case? > > > > I personally prefer way 2. With way 1, the common functions would > > just be a wrapper around the asic specific callbacks. The older > > powerplay code worked that way. If there is something common that > > needs to be done for all asics, I think that would make sense, but I > > don't know that we have any cases like that. If we do end up needing > > something like that, we can always revisit this. > > > > I was thinking something like the following: > > > > struct smu_asic_funcs { > > int (*get_current_clock_freq)(); > > int (*get_fan_speed_rpm)(); > > ... > > } > > > > Then for cases where two asics use the same SMU interface, you can > > create a common function. So for vega20, it might look like: > > > > static const struct smu_asic_funcs vega20_smu_asic_funcs = > > { > > .get_current_clock_freq = smu_v11_0_get_current_clock_freq, > > .get_fan_speed_rpm = vega20_get_fan_speed_rpm, > > ... > > }; > > > > and navi10 would look like: > > > > static const struct smu_asic_funcs navi10_smu_asic_funcs = > > { > > .get_current_clock_freq = smu_v11_0_get_current_clock_freq, > > .get_fan_speed_rpm = navi10_get_fan_speed_rpm, > > ... > > }; > > > > Alex > > > > > >> thanks. > >> > >>> > >>> Signed-off-by: Kevin Wang <kevin1.wang@amd.com> > >>> --- > >>> drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 26 +++++++++++++++++-- > >>> .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 9 ++++--- > >>> drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 3 +++ > >>> drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 10 +++---- > >>> drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 3 +++ > >>> 5 files changed, 40 insertions(+), 11 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > >>> index 05b91bc5054c..85269f86cae2 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > >>> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > >>> @@ -284,11 +284,14 @@ int smu_get_power_num_states(struct smu_context *smu, > >>> return 0; > >>> } > >>> > >>> -int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > >>> - void *data, uint32_t *size) > >>> +int smu_default_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > >>> + void *data, uint32_t *size) > >>> { > >>> int ret = 0; > >>> > >>> + if (!data || !size) > >>> + return -EINVAL; > >>> + > >>> switch (sensor) { > >>> case AMDGPU_PP_SENSOR_STABLE_PSTATE_SCLK: > >>> *((uint32_t *)data) = smu->pstate_sclk; > >>> @@ -321,6 +324,25 @@ int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > >>> return ret; > >>> } > >>> > >>> +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > >>> + void *data, uint32_t *size) > >>> +{ > >>> + int ret = 0; > >>> + > >>> + if (!data || !size) > >>> + return -EINVAL; > >>> + > >>> + /* handle sensor sequence: asic --> ip level --> default */ > >>> + ret = smu_asic_read_sensor(smu, sensor, data, size); > >>> + if (ret) { > >>> + ret = smu_smc_read_sensor(smu, sensor, data, size); > >>> + if (ret) > >>> + ret = smu_default_read_sensor(smu, sensor, data, size); > >>> + } > >>> + > >>> + return ret; > >>> +} > >>> + > >>> int smu_update_table(struct smu_context *smu, enum smu_table_id table_index, int argument, > >>> void *table_data, bool drv2smu) > >>> { > >>> diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > >>> index 34093ddca105..462bae8d62aa 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > >>> +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > >>> @@ -820,10 +820,10 @@ struct smu_funcs > >>> ((smu)->ppt_funcs->set_thermal_fan_table ? (smu)->ppt_funcs->set_thermal_fan_table((smu)) : 0) > >>> #define smu_start_thermal_control(smu) \ > >>> ((smu)->funcs->start_thermal_control? (smu)->funcs->start_thermal_control((smu)) : 0) > >>> -#define smu_read_sensor(smu, sensor, data, size) \ > >>> - ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : 0) > >>> +#define smu_smc_read_sensor(smu, sensor, data, size) \ > >>> + ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL) > >>> #define smu_asic_read_sensor(smu, sensor, data, size) \ > >>> - ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : 0) > >>> + ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL) > >>> #define smu_get_power_profile_mode(smu, buf) \ > >>> ((smu)->ppt_funcs->get_power_profile_mode ? (smu)->ppt_funcs->get_power_profile_mode((smu), buf) : 0) > >>> #define smu_set_power_profile_mode(smu, param, param_size) \ > >>> @@ -989,5 +989,6 @@ enum amd_dpm_forced_level smu_get_performance_level(struct smu_context *smu); > >>> int smu_force_performance_level(struct smu_context *smu, enum amd_dpm_forced_level level); > >>> int smu_set_display_count(struct smu_context *smu, uint32_t count); > >>> bool smu_clk_dpm_is_enabled(struct smu_context *smu, enum smu_clk_type clk_type); > >>> - > >>> +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > >>> + void *data, uint32_t *size); > >>> #endif > >>> diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > >>> index 46e2913e4af4..0a53695785b6 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > >>> +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > >>> @@ -1365,6 +1365,9 @@ static int navi10_read_sensor(struct smu_context *smu, > >>> struct smu_table_context *table_context = &smu->smu_table; > >>> PPTable_t *pptable = table_context->driver_pptable; > >>> > >>> + if (!data || !size) > >>> + return -EINVAL; > >>> + > >>> switch (sensor) { > >>> case AMDGPU_PP_SENSOR_MAX_FAN_RPM: > >>> *(uint32_t *)data = pptable->FanMaximumRpm; > >>> diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > >>> index 76bc157525d0..2679b6ff6ca3 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > >>> +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > >>> @@ -1267,6 +1267,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, > >>> void *data, uint32_t *size) > >>> { > >>> int ret = 0; > >>> + > >>> + if (!data || !size) > >>> + return -EINVAL; > >>> + > >>> switch (sensor) { > >>> case AMDGPU_PP_SENSOR_GFX_MCLK: > >>> ret = smu_get_current_clk_freq(smu, SMU_UCLK, (uint32_t *)data); > >>> @@ -1285,14 +1289,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, > >>> *size = 4; > >>> break; > >>> default: > >>> - ret = smu_common_read_sensor(smu, sensor, data, size); > >>> + ret = -EINVAL; > >>> break; > >>> } > >>> > >>> - /* try get sensor data by asic */ > >>> - if (ret) > >>> - ret = smu_asic_read_sensor(smu, sensor, data, size); > >>> - > >>> if (ret) > >>> *size = 0; > >>> > >>> diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > >>> index bcd0efaf7bbd..b44ec7c670c5 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > >>> +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > >>> @@ -3146,6 +3146,9 @@ static int vega20_read_sensor(struct smu_context *smu, > >>> struct smu_table_context *table_context = &smu->smu_table; > >>> PPTable_t *pptable = table_context->driver_pptable; > >>> > >>> + if (!data || !size) > >>> + return -EINVAL; > >>> + > >>> switch (sensor) { > >>> case AMDGPU_PP_SENSOR_MAX_FAN_RPM: > >>> *(uint32_t *)data = pptable->FanMaximumRpm; > >>> -- > >>> 2.22.0 > >>> > >>> _______________________________________________ > >>> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <CADnq5_ODiK_pyMZsUmw0T2ivG=_-j5h3ky9vouE-ACEJV+fddQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu [not found] ` <CADnq5_ODiK_pyMZsUmw0T2ivG=_-j5h3ky9vouE-ACEJV+fddQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2019-07-20 2:58 ` Wang, Kevin(Yang) 0 siblings, 0 replies; 10+ messages in thread From: Wang, Kevin(Yang) @ 2019-07-20 2:58 UTC (permalink / raw) To: Alex Deucher Cc: Huang, Ray, Feng, Kenneth, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW [-- Attachment #1.1: Type: text/plain, Size: 13570 bytes --] ________________________________ From: Alex Deucher <alexdeucher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Sent: Saturday, July 20, 2019 1:53 AM To: Wang, Kevin(Yang) <Kevin1.Wang-5C7GfCeVMHo@public.gmane.org> Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>; Huang, Ray <Ray.Huang-5C7GfCeVMHo@public.gmane.org>; Feng, Kenneth <Kenneth.Feng-5C7GfCeVMHo@public.gmane.org> Subject: Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu On Fri, Jul 19, 2019 at 1:03 PM Wang, Kevin(Yang) <Kevin1.Wang-5C7GfCeVMHo@public.gmane.org> wrote: > > The read sensor function is not same as other one, this function should be handle many different sensor type, I don’t want to handle all sensor type in asic file, because some sensor is very simple, only should be send a message to smc, and some sensor should be parse Table with firmware. > > Eg: get gfx clk, > Only need send message to get value. > Because the message already mapped on each asic, so this sensor should be handled in smu_v11.c. > > Eg: get gpu load, > this sensor should be get value from firmware table, so should must be handled in xxx_ppt.c > > Eg: get pstate clock, > It is full software sensor, so it is handled in amdgpu_smu.c > > In this patch, the smu only want to public one api of smu_read_sensor, and the other sensor macro is helper in smu internally. > > I want to reduce duplicate code in smu, it will be let bring up new asic easy. I think it's still pretty straight forward. E.g., in the asic specific read_sensor() callback you do something like this: switch (sensor) { case SENSOR1: case SENSOR2: case SENSOR3: ret = smu_v11_read_sensor_helper(smu, sensor, value); breakl case SENSOR4: ret = vega20_get_sensor4(smu, value); break; default: ... } That way there is less confusion about following callbacks. Alex [kevin]: I will improve the logic of this part of code according to your suggestion. thanks. > > Thanks > > > On Jul 20, 2019, at 12:14 AM, Alex Deucher <alexdeucher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > > > >> On Fri, Jul 19, 2019 at 12:01 PM Wang, Kevin(Yang) <Kevin1.Wang-urvtwAKJhsc@public.gmane.orgm> wrote: > >> > >> > >> ________________________________ > >> From: Alex Deucher <alexdeucher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > >> Sent: Friday, July 19, 2019 11:17 PM > >> To: Wang, Kevin(Yang) <Kevin1.Wang-5C7GfCeVMHo@public.gmane.org> > >> Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>; Huang, Ray <Ray.Huang-5C7GfCeVMHo@public.gmane.org>; Feng, Kenneth <Kenneth.Feng-5C7GfCeVMHo@public.gmane.org> > >> Subject: Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu > >> > >>> On Fri, Jul 19, 2019 at 7:23 AM Wang, Kevin(Yang) <Kevin1.Wang-urvtwAKJhsc@public.gmane.orgm> wrote: > >>> > >>> each asic maybe has different read sensor method. > >>> so change read sensor sequence in smu. > >>> > >>> read sensor sequence: > >>> asic sensor --> smc sensor (smu 11...) --> default_sensor (common) > >> > >> I think this makes sense. That said, the current swSMU callback > >> structures are really confusing. I think we should switch to a single > >> set of per asic callbacks and then add common helpers. Then for asics > >> where it makes sense we can just use the helper as the callback for > >> all relevant asics. If they need something asic specific, use the > >> asic specific function. That should avoid the current mix of > >> callbacks and make it clearer what code gets used when. > >> > >> Alex > >> > >> [kevin]: > >> > >> thanks review, in current code, the read sensor related function is not very clear, so i want to refine them. > >> but I'm not sure which way to write good code logic. > >> > >> way 1: > >> > >> provide a puiblic function named smu_read_sensor as public smu api for other kenel module, like this patch. > >> this function will try to get value from asic or smu ip level or common, call them in turn according to the rules. > >> > >> way 2: > >> > >> define a maco named smu_read_sensor as public api, implement it in xxx_ppt.c file, > >> if can't handle sensor type in xxx_ppt.c, then call helper in smu_v11_0.c, then call amdgpu_smu.c helper. > >> > >> in this way, it means we must implement this callback function in xxx_ppt.c. > >> if need to support new asic, we should add some dulicated code in xxx_ppt.c, if not the smu_read_sensor api is not work well. > >> in smu module, use many macros as module public api, it is impossible to tell at what level these macros implement specific code logic. > >> so i want to refine them. > >> > >> do you think which way is good for this case? > > > > I personally prefer way 2. With way 1, the common functions would > > just be a wrapper around the asic specific callbacks. The older > > powerplay code worked that way. If there is something common that > > needs to be done for all asics, I think that would make sense, but I > > don't know that we have any cases like that. If we do end up needing > > something like that, we can always revisit this. > > > > I was thinking something like the following: > > > > struct smu_asic_funcs { > > int (*get_current_clock_freq)(); > > int (*get_fan_speed_rpm)(); > > ... > > } > > > > Then for cases where two asics use the same SMU interface, you can > > create a common function. So for vega20, it might look like: > > > > static const struct smu_asic_funcs vega20_smu_asic_funcs = > > { > > .get_current_clock_freq = smu_v11_0_get_current_clock_freq, > > .get_fan_speed_rpm = vega20_get_fan_speed_rpm, > > ... > > }; > > > > and navi10 would look like: > > > > static const struct smu_asic_funcs navi10_smu_asic_funcs = > > { > > .get_current_clock_freq = smu_v11_0_get_current_clock_freq, > > .get_fan_speed_rpm = navi10_get_fan_speed_rpm, > > ... > > }; > > > > Alex > > > > > >> thanks. > >> > >>> > >>> Signed-off-by: Kevin Wang <kevin1.wang-5C7GfCeVMHo@public.gmane.org> > >>> --- > >>> drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 26 +++++++++++++++++-- > >>> .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 9 ++++--- > >>> drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 3 +++ > >>> drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 10 +++---- > >>> drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 3 +++ > >>> 5 files changed, 40 insertions(+), 11 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > >>> index 05b91bc5054c..85269f86cae2 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > >>> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > >>> @@ -284,11 +284,14 @@ int smu_get_power_num_states(struct smu_context *smu, > >>> return 0; > >>> } > >>> > >>> -int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > >>> - void *data, uint32_t *size) > >>> +int smu_default_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > >>> + void *data, uint32_t *size) > >>> { > >>> int ret = 0; > >>> > >>> + if (!data || !size) > >>> + return -EINVAL; > >>> + > >>> switch (sensor) { > >>> case AMDGPU_PP_SENSOR_STABLE_PSTATE_SCLK: > >>> *((uint32_t *)data) = smu->pstate_sclk; > >>> @@ -321,6 +324,25 @@ int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > >>> return ret; > >>> } > >>> > >>> +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > >>> + void *data, uint32_t *size) > >>> +{ > >>> + int ret = 0; > >>> + > >>> + if (!data || !size) > >>> + return -EINVAL; > >>> + > >>> + /* handle sensor sequence: asic --> ip level --> default */ > >>> + ret = smu_asic_read_sensor(smu, sensor, data, size); > >>> + if (ret) { > >>> + ret = smu_smc_read_sensor(smu, sensor, data, size); > >>> + if (ret) > >>> + ret = smu_default_read_sensor(smu, sensor, data, size); > >>> + } > >>> + > >>> + return ret; > >>> +} > >>> + > >>> int smu_update_table(struct smu_context *smu, enum smu_table_id table_index, int argument, > >>> void *table_data, bool drv2smu) > >>> { > >>> diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > >>> index 34093ddca105..462bae8d62aa 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > >>> +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > >>> @@ -820,10 +820,10 @@ struct smu_funcs > >>> ((smu)->ppt_funcs->set_thermal_fan_table ? (smu)->ppt_funcs->set_thermal_fan_table((smu)) : 0) > >>> #define smu_start_thermal_control(smu) \ > >>> ((smu)->funcs->start_thermal_control? (smu)->funcs->start_thermal_control((smu)) : 0) > >>> -#define smu_read_sensor(smu, sensor, data, size) \ > >>> - ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : 0) > >>> +#define smu_smc_read_sensor(smu, sensor, data, size) \ > >>> + ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL) > >>> #define smu_asic_read_sensor(smu, sensor, data, size) \ > >>> - ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : 0) > >>> + ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL) > >>> #define smu_get_power_profile_mode(smu, buf) \ > >>> ((smu)->ppt_funcs->get_power_profile_mode ? (smu)->ppt_funcs->get_power_profile_mode((smu), buf) : 0) > >>> #define smu_set_power_profile_mode(smu, param, param_size) \ > >>> @@ -989,5 +989,6 @@ enum amd_dpm_forced_level smu_get_performance_level(struct smu_context *smu); > >>> int smu_force_performance_level(struct smu_context *smu, enum amd_dpm_forced_level level); > >>> int smu_set_display_count(struct smu_context *smu, uint32_t count); > >>> bool smu_clk_dpm_is_enabled(struct smu_context *smu, enum smu_clk_type clk_type); > >>> - > >>> +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor, > >>> + void *data, uint32_t *size); > >>> #endif > >>> diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > >>> index 46e2913e4af4..0a53695785b6 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > >>> +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > >>> @@ -1365,6 +1365,9 @@ static int navi10_read_sensor(struct smu_context *smu, > >>> struct smu_table_context *table_context = &smu->smu_table; > >>> PPTable_t *pptable = table_context->driver_pptable; > >>> > >>> + if (!data || !size) > >>> + return -EINVAL; > >>> + > >>> switch (sensor) { > >>> case AMDGPU_PP_SENSOR_MAX_FAN_RPM: > >>> *(uint32_t *)data = pptable->FanMaximumRpm; > >>> diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > >>> index 76bc157525d0..2679b6ff6ca3 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > >>> +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > >>> @@ -1267,6 +1267,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, > >>> void *data, uint32_t *size) > >>> { > >>> int ret = 0; > >>> + > >>> + if (!data || !size) > >>> + return -EINVAL; > >>> + > >>> switch (sensor) { > >>> case AMDGPU_PP_SENSOR_GFX_MCLK: > >>> ret = smu_get_current_clk_freq(smu, SMU_UCLK, (uint32_t *)data); > >>> @@ -1285,14 +1289,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu, > >>> *size = 4; > >>> break; > >>> default: > >>> - ret = smu_common_read_sensor(smu, sensor, data, size); > >>> + ret = -EINVAL; > >>> break; > >>> } > >>> > >>> - /* try get sensor data by asic */ > >>> - if (ret) > >>> - ret = smu_asic_read_sensor(smu, sensor, data, size); > >>> - > >>> if (ret) > >>> *size = 0; > >>> > >>> diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > >>> index bcd0efaf7bbd..b44ec7c670c5 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > >>> +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > >>> @@ -3146,6 +3146,9 @@ static int vega20_read_sensor(struct smu_context *smu, > >>> struct smu_table_context *table_context = &smu->smu_table; > >>> PPTable_t *pptable = table_context->driver_pptable; > >>> > >>> + if (!data || !size) > >>> + return -EINVAL; > >>> + > >>> switch (sensor) { > >>> case AMDGPU_PP_SENSOR_MAX_FAN_RPM: > >>> *(uint32_t *)data = pptable->FanMaximumRpm; > >>> -- > >>> 2.22.0 > >>> > >>> _______________________________________________ > >>> amd-gfx mailing list > >>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx [-- Attachment #1.2: Type: text/html, Size: 22768 bytes --] [-- Attachment #2: Type: text/plain, Size: 153 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-07-23 15:06 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-23 9:39 [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu Kenneth Feng
[not found] ` <1563874746-6318-1-git-send-email-kenneth.feng-5C7GfCeVMHo@public.gmane.org>
2019-07-23 9:43 ` Wang, Kevin(Yang)
[not found] ` <MN2PR12MB3296043F281A9E7345179AE1A2C70-rweVpJHSKTqAm9ToKNQgFgdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2019-07-23 15:06 ` Deucher, Alexander
-- strict thread matches above, loose matches on Subject: below --
2019-07-19 11:22 Wang, Kevin(Yang)
[not found] ` <20190719112232.28485-1-kevin1.wang-5C7GfCeVMHo@public.gmane.org>
2019-07-19 15:17 ` Alex Deucher
[not found] ` <CADnq5_P2UVmm4P1myih0UOQ2nvLDB01zdaFN9v7p423QeLASKg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-07-19 16:01 ` Wang, Kevin(Yang)
[not found] ` <MN2PR12MB329684D6E227BE0132F67A8EA2CB0-rweVpJHSKTqAm9ToKNQgFgdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2019-07-19 16:14 ` Alex Deucher
[not found] ` <CADnq5_PDRyxDNn3TK40L0w2KGDUcahrZge7+q_Z06DX3q-xFWw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-07-19 17:03 ` Wang, Kevin(Yang)
[not found] ` <A2EC6125-E08F-4FD2-A56C-E9D48CD3F95F-5C7GfCeVMHo@public.gmane.org>
2019-07-19 17:53 ` Alex Deucher
[not found] ` <CADnq5_ODiK_pyMZsUmw0T2ivG=_-j5h3ky9vouE-ACEJV+fddQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-07-20 2:58 ` Wang, Kevin(Yang)
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.