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@xxxxxxxxx> wrote:
>
>> On Fri, Jul 19, 2019 at 12:01 PM Wang, Kevin(Yang) <Kevin1.Wang@xxxxxxx> wrote:
>>
>>
>> ________________________________
>> From: Alex Deucher <alexdeucher@xxxxxxxxx>
>> Sent: Friday, July 19, 2019 11:17 PM
>> To: Wang, Kevin(Yang) <Kevin1.Wang@xxxxxxx>
>> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>; Huang, Ray <Ray.Huang@xxxxxxx>; Feng, Kenneth <Kenneth.Feng@xxxxxxx>
>> 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@xxxxxxx> 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@xxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxxxxxx
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
