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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





From: Alex Deucher <alexdeucher@xxxxxxxxx>
Sent: Saturday, July 20, 2019 1:53 AM
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 1:03 PM Wang, Kevin(Yang) <Kevin1.Wang@xxxxxxx> 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@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 = ""> > >>> 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 = ""> > >>> --
> >>> 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

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux