[PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu

[PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu

[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

Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu

[Alex Deucher]

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

Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu

[Wang, Kevin(Yang)]

[-- 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

Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu

[Alex Deucher]

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

Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu

[Wang, Kevin(Yang)]

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

Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu

[Alex Deucher]

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

Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu

[Wang, Kevin(Yang)]

[-- 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

Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu

[Deucher, Alexander]

[-- 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

Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu

[Wang, Kevin(Yang)]

[-- 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

[PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu

[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