[PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu
Alex Deucher
alexdeucher at gmail.com
Fri Jul 19 17:53:17 UTC 2019
On Fri, Jul 19, 2019 at 1:03 PM Wang, Kevin(Yang) <Kevin1.Wang at 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 at gmail.com> wrote:
> >
> >> On Fri, Jul 19, 2019 at 12:01 PM Wang, Kevin(Yang) <Kevin1.Wang at amd.com> wrote:
> >>
> >>
> >> ________________________________
> >> From: Alex Deucher <alexdeucher at gmail.com>
> >> Sent: Friday, July 19, 2019 11:17 PM
> >> To: Wang, Kevin(Yang) <Kevin1.Wang at amd.com>
> >> Cc: amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>; Huang, Ray <Ray.Huang at amd.com>; Feng, Kenneth <Kenneth.Feng at 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 at 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 at 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 at lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
More information about the amd-gfx
mailing list