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
