<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"></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> Friday, July 19, 2019 11:17 PM<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 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</div> <div class="PlainText"><br> </div> <div class="PlainText">[kevin]:</div> <div class="PlainText"><br> </div> <div class="PlainText">thanks review, in current code, the read sensor related function is not very clear, so i want to refine them.</div> <div class="PlainText"></div> <div>but I'm not sure which way to write good code logic.</div> <div><br> </div> <div>way 1:</div> <div><br> </div> <div>provide a puiblic function named smu_read_sensor as public smu api for other kenel module, like this patch.</div> <div>this function will try to get value from asic or smu ip level or common, c<span style="font-size: 11pt;">all them in turn according to the rules.</span></div> <div><br> </div> <div>way 2:</div> <div><br> </div> <div>define a maco named smu_read_sensor as public api, implement it in xxx_ppt.c file, </div> <div>if can't handle sensor type in xxx_ppt.c, then call helper in smu_v11_0.c, then call amdgpu_smu.c helper.</div> <div><br> </div> <div>in this way, it means we must implement this callback function in xxx_ppt.c.</div> <div>if need to support new asic, we should add some dulicated code in xxx_ppt.c, <span style="font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols; font-size: 14.6667px;">if not the smu_read_sensor api is not work well.</span><span></span></div> <div><span style="font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols; font-size: 14.6667px;">in smu module, use many macros as module public api, it</span> is impossible to tell at what level these macros implement specific code logic.</div> <div>so i want to refine them.</div> <div><br> </div> <div>do you think which way is good for this case? </div> <div>thanks.</div> <div class="PlainText"><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="LPlnk239629" class="OWAAutoLink" previewremoved="true"> https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a><br> </div> </span></font></div> </div> </div> </body> </html>