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

[Alex Deucher <alexdeucher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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