<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=Windows-1252"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr"> <div style="color: rgb(0, 0, 0);"> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Alex Deucher <alexdeucher@gmail.com> Sent: Saturday, July 20, 2019 1:53 AM 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 <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">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</div> <div class="PlainText"> </div> <div class="PlainText">[kevin]:</div> <div class="PlainText"> <div>I will improve the logic of this part of code according to your suggestion. thanks.</div> > > 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 > >>> <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx" id="LPlnk641870" class="OWAAutoLink" previewremoved="true"> https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a> </div> </div> </div> </div> </body> </html>