<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252">
<style type="text/css" style="display:none;"><!-- P {margin-top:0;margin-bottom:0;} --></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">
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<div style="color: rgb(0, 0, 0);">
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> Alex Deucher <alexdeucher@gmail.com><br>
<b>Sent:</b> Saturday, July 20, 2019 1:53 AM<br>
<b>To:</b> Wang, Kevin(Yang) <Kevin1.Wang@amd.com><br>
<b>Cc:</b> amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; Huang, Ray <Ray.Huang@amd.com>; Feng, Kenneth <Kenneth.Feng@amd.com><br>
<b>Subject:</b> Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText">On Fri, Jul 19, 2019 at 1:03 PM Wang, Kevin(Yang) <Kevin1.Wang@amd.com> wrote:<br>
><br>
> 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.<br>
><br>
> Eg: get gfx clk,<br>
> Only need send message to get value.<br>
> Because the message already mapped on each asic, so this sensor should be handled in smu_v11.c.<br>
><br>
> Eg: get gpu load,<br>
> this sensor should be get value from firmware table, so should must be handled in xxx_ppt.c<br>
><br>
> Eg: get pstate clock,<br>
> It is full software sensor, so it is handled in amdgpu_smu.c<br>
><br>
> 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.<br>
><br>
> I want to reduce duplicate code in smu, it will be let bring up new asic easy.<br>
<br>
I think it's still pretty straight forward.  E.g., in the asic<br>
specific read_sensor() callback you do something like this:<br>
<br>
switch (sensor) {<br>
case SENSOR1:<br>
case SENSOR2:<br>
case SENSOR3:<br>
      ret = smu_v11_read_sensor_helper(smu, sensor, value);<br>
      breakl<br>
case SENSOR4:<br>
     ret = vega20_get_sensor4(smu, value);<br>
     break;<br>
default:<br>
    ...<br>
}<br>
<br>
That way there is less confusion about following callbacks.<br>
<br>
Alex</div>
<div class="PlainText"><br>
</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>
<br>
><br>
> Thanks<br>
><br>
> > On Jul 20, 2019, at 12:14 AM, Alex Deucher <alexdeucher@gmail.com> wrote:<br>
> ><br>
> >> On Fri, Jul 19, 2019 at 12:01 PM Wang, Kevin(Yang) <Kevin1.Wang@amd.com> wrote:<br>
> >><br>
> >><br>
> >> ________________________________<br>
> >> From: Alex Deucher <alexdeucher@gmail.com><br>
> >> Sent: Friday, July 19, 2019 11:17 PM<br>
> >> To: Wang, Kevin(Yang) <Kevin1.Wang@amd.com><br>
> >> Cc: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; Huang, Ray <Ray.Huang@amd.com>; Feng, Kenneth <Kenneth.Feng@amd.com><br>
> >> Subject: Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu<br>
> >><br>
> >>> On Fri, Jul 19, 2019 at 7:23 AM Wang, Kevin(Yang) <Kevin1.Wang@amd.com> wrote:<br>
> >>><br>
> >>> each asic maybe has different read sensor method.<br>
> >>> so change read sensor sequence in smu.<br>
> >>><br>
> >>> read sensor sequence:<br>
> >>> asic sensor --> smc sensor (smu 11...) --> default_sensor (common)<br>
> >><br>
> >> I think this makes sense.  That said, the current swSMU callback<br>
> >> structures are really confusing.  I think we should switch to a single<br>
> >> set of per asic callbacks and then add common helpers.  Then for asics<br>
> >> where it makes sense we can just use the helper as the callback for<br>
> >> all relevant asics.  If they need something asic specific, use the<br>
> >> asic specific function.  That should avoid the current mix of<br>
> >> callbacks and make it clearer what code gets used when.<br>
> >><br>
> >> Alex<br>
> >><br>
> >> [kevin]:<br>
> >><br>
> >> thanks review, in current code, the read sensor related function is not very clear, so i want to refine them.<br>
> >> but I'm not sure which way to write good code logic.<br>
> >><br>
> >> way 1:<br>
> >><br>
> >> provide a puiblic function named smu_read_sensor as public smu api for other kenel module, like this patch.<br>
> >> this function will try to get value from asic or smu ip level or common, call them in turn according to the rules.<br>
> >><br>
> >> way 2:<br>
> >><br>
> >> define a maco named smu_read_sensor as public api, implement it in xxx_ppt.c file,<br>
> >> if can't handle sensor type in xxx_ppt.c, then call helper in smu_v11_0.c,  then call amdgpu_smu.c helper.<br>
> >><br>
> >> in this way, it means we must implement this callback function in xxx_ppt.c.<br>
> >> 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.<br>
> >> in smu module, use many macros as module public api, it is impossible to tell at what level these macros implement specific code logic.<br>
> >> so i want to refine them.<br>
> >><br>
> >> do you think which way is good for this case?<br>
> ><br>
> > I personally prefer way 2.  With way 1, the common functions would<br>
> > just be a wrapper around the asic specific callbacks.  The older<br>
> > powerplay code worked that way.  If there is something common that<br>
> > needs to be done for all asics, I think that would make sense, but I<br>
> > don't know that we have any cases like that.  If we do end up needing<br>
> > something like that, we can always revisit this.<br>
> ><br>
> > I was thinking something like the following:<br>
> ><br>
> > struct smu_asic_funcs {<br>
> >    int (*get_current_clock_freq)();<br>
> >    int (*get_fan_speed_rpm)();<br>
> >    ...<br>
> > }<br>
> ><br>
> > Then for cases where two asics use the same SMU interface, you can<br>
> > create a common function.  So for vega20, it might look like:<br>
> ><br>
> > static const struct smu_asic_funcs vega20_smu_asic_funcs =<br>
> > {<br>
> >    .get_current_clock_freq = smu_v11_0_get_current_clock_freq,<br>
> >    .get_fan_speed_rpm = vega20_get_fan_speed_rpm,<br>
> >    ...<br>
> > };<br>
> ><br>
> > and navi10 would look like:<br>
> ><br>
> > static const struct smu_asic_funcs navi10_smu_asic_funcs =<br>
> > {<br>
> >    .get_current_clock_freq = smu_v11_0_get_current_clock_freq,<br>
> >    .get_fan_speed_rpm = navi10_get_fan_speed_rpm,<br>
> >    ...<br>
> > };<br>
> ><br>
> > Alex<br>
> ><br>
> ><br>
> >> thanks.<br>
> >><br>
> >>><br>
> >>> Signed-off-by: Kevin Wang <kevin1.wang@amd.com><br>
> >>> ---<br>
> >>> drivers/gpu/drm/amd/powerplay/amdgpu_smu.c    | 26 +++++++++++++++++--<br>
> >>> .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h    |  9 ++++---<br>
> >>> drivers/gpu/drm/amd/powerplay/navi10_ppt.c    |  3 +++<br>
> >>> drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 10 +++----<br>
> >>> drivers/gpu/drm/amd/powerplay/vega20_ppt.c    |  3 +++<br>
> >>> 5 files changed, 40 insertions(+), 11 deletions(-)<br>
> >>><br>
> >>> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c<br>
> >>> index 05b91bc5054c..85269f86cae2 100644<br>
> >>> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c<br>
> >>> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c<br>
> >>> @@ -284,11 +284,14 @@ int smu_get_power_num_states(struct smu_context *smu,<br>
> >>>        return 0;<br>
> >>> }<br>
> >>><br>
> >>> -int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor,<br>
> >>> -                          void *data, uint32_t *size)<br>
> >>> +int smu_default_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor,<br>
> >>> +                           void *data, uint32_t *size)<br>
> >>> {<br>
> >>>        int ret = 0;<br>
> >>><br>
> >>> +       if (!data || !size)<br>
> >>> +               return -EINVAL;<br>
> >>> +<br>
> >>>        switch (sensor) {<br>
> >>>        case AMDGPU_PP_SENSOR_STABLE_PSTATE_SCLK:<br>
> >>>                *((uint32_t *)data) = smu->pstate_sclk;<br>
> >>> @@ -321,6 +324,25 @@ int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor,<br>
> >>>        return ret;<br>
> >>> }<br>
> >>><br>
> >>> +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor,<br>
> >>> +                   void *data, uint32_t *size)<br>
> >>> +{<br>
> >>> +       int ret = 0;<br>
> >>> +<br>
> >>> +       if (!data || !size)<br>
> >>> +               return -EINVAL;<br>
> >>> +<br>
> >>> +       /* handle sensor sequence: asic --> ip level -->  default */<br>
> >>> +       ret = smu_asic_read_sensor(smu, sensor, data, size);<br>
> >>> +       if (ret) {<br>
> >>> +               ret = smu_smc_read_sensor(smu, sensor, data, size);<br>
> >>> +               if (ret)<br>
> >>> +                       ret = smu_default_read_sensor(smu, sensor, data, size);<br>
> >>> +       }<br>
> >>> +<br>
> >>> +       return ret;<br>
> >>> +}<br>
> >>> +<br>
> >>> int smu_update_table(struct smu_context *smu, enum smu_table_id table_index, int argument,<br>
> >>>                     void *table_data, bool drv2smu)<br>
> >>> {<br>
> >>> diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h<br>
> >>> index 34093ddca105..462bae8d62aa 100644<br>
> >>> --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h<br>
> >>> +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h<br>
> >>> @@ -820,10 +820,10 @@ struct smu_funcs<br>
> >>>        ((smu)->ppt_funcs->set_thermal_fan_table ? (smu)->ppt_funcs->set_thermal_fan_table((smu)) : 0)<br>
> >>> #define smu_start_thermal_control(smu) \<br>
> >>>        ((smu)->funcs->start_thermal_control? (smu)->funcs->start_thermal_control((smu)) : 0)<br>
> >>> -#define smu_read_sensor(smu, sensor, data, size) \<br>
> >>> -       ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : 0)<br>
> >>> +#define smu_smc_read_sensor(smu, sensor, data, size) \<br>
> >>> +       ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL)<br>
> >>> #define smu_asic_read_sensor(smu, sensor, data, size) \<br>
> >>> -       ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : 0)<br>
> >>> +       ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (data), (size)) : -EINVAL)<br>
> >>> #define smu_get_power_profile_mode(smu, buf) \<br>
> >>>        ((smu)->ppt_funcs->get_power_profile_mode ? (smu)->ppt_funcs->get_power_profile_mode((smu), buf) : 0)<br>
> >>> #define smu_set_power_profile_mode(smu, param, param_size) \<br>
> >>> @@ -989,5 +989,6 @@ enum amd_dpm_forced_level smu_get_performance_level(struct smu_context *smu);<br>
> >>> int smu_force_performance_level(struct smu_context *smu, enum amd_dpm_forced_level level);<br>
> >>> int smu_set_display_count(struct smu_context *smu, uint32_t count);<br>
> >>> bool smu_clk_dpm_is_enabled(struct smu_context *smu, enum smu_clk_type clk_type);<br>
> >>> -<br>
> >>> +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sensor,<br>
> >>> +                   void *data, uint32_t *size);<br>
> >>> #endif<br>
> >>> diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c<br>
> >>> index 46e2913e4af4..0a53695785b6 100644<br>
> >>> --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c<br>
> >>> +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c<br>
> >>> @@ -1365,6 +1365,9 @@ static int navi10_read_sensor(struct smu_context *smu,<br>
> >>>        struct smu_table_context *table_context = &smu->smu_table;<br>
> >>>        PPTable_t *pptable = table_context->driver_pptable;<br>
> >>><br>
> >>> +       if (!data || !size)<br>
> >>> +               return -EINVAL;<br>
> >>> +<br>
> >>>        switch (sensor) {<br>
> >>>        case AMDGPU_PP_SENSOR_MAX_FAN_RPM:<br>
> >>>                *(uint32_t *)data = pptable->FanMaximumRpm;<br>
> >>> diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c<br>
> >>> index 76bc157525d0..2679b6ff6ca3 100644<br>
> >>> --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c<br>
> >>> +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c<br>
> >>> @@ -1267,6 +1267,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu,<br>
> >>>                                 void *data, uint32_t *size)<br>
> >>> {<br>
> >>>        int ret = 0;<br>
> >>> +<br>
> >>> +       if (!data || !size)<br>
> >>> +               return -EINVAL;<br>
> >>> +<br>
> >>>        switch (sensor) {<br>
> >>>        case AMDGPU_PP_SENSOR_GFX_MCLK:<br>
> >>>                ret = smu_get_current_clk_freq(smu, SMU_UCLK, (uint32_t *)data);<br>
> >>> @@ -1285,14 +1289,10 @@ static int smu_v11_0_read_sensor(struct smu_context *smu,<br>
> >>>                *size = 4;<br>
> >>>                break;<br>
> >>>        default:<br>
> >>> -               ret = smu_common_read_sensor(smu, sensor, data, size);<br>
> >>> +               ret = -EINVAL;<br>
> >>>                break;<br>
> >>>        }<br>
> >>><br>
> >>> -       /* try get sensor data by asic */<br>
> >>> -       if (ret)<br>
> >>> -               ret = smu_asic_read_sensor(smu, sensor, data, size);<br>
> >>> -<br>
> >>>        if (ret)<br>
> >>>                *size = 0;<br>
> >>><br>
> >>> diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c<br>
> >>> index bcd0efaf7bbd..b44ec7c670c5 100644<br>
> >>> --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c<br>
> >>> +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c<br>
> >>> @@ -3146,6 +3146,9 @@ static int vega20_read_sensor(struct smu_context *smu,<br>
> >>>        struct smu_table_context *table_context = &smu->smu_table;<br>
> >>>        PPTable_t *pptable = table_context->driver_pptable;<br>
> >>><br>
> >>> +       if (!data || !size)<br>
> >>> +               return -EINVAL;<br>
> >>> +<br>
> >>>        switch (sensor) {<br>
> >>>        case AMDGPU_PP_SENSOR_MAX_FAN_RPM:<br>
> >>>                *(uint32_t *)data = pptable->FanMaximumRpm;<br>
> >>> --<br>
> >>> 2.22.0<br>
> >>><br>
> >>> _______________________________________________<br>
> >>> amd-gfx mailing list<br>
> >>> amd-gfx@lists.freedesktop.org<br>
> >>> <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><br>
</div>
</span></font></div>
</div>
</div>
</body>
</html>