<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<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:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr">
<p style="margin-top:0;margin-bottom:0">Hi <a class="mention ms-bgc-nlr ms-fcl-b" id="OWAAM369891" href="mailto:Alexander.Deucher@amd.com">
@Deucher, Alexander</a> & <a class="mention ms-bgc-nlr ms-fcl-b" id="OWAAM450498" href="mailto:Rex.Zhu@amd.com">
@Zhu, Rex</a>,</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">I can remove the od setings for power limit and fan speed related since they seem
<span>redundant </span>with other sysfs apis.</p>
<p style="margin-top:0;margin-bottom:0">But for the clock/voltage related, i can not see how to reuse old <span>pp_od_clk_voltage APIs.</span></p>
<p style="margin-top:0;margin-bottom:0"><span><br>
</span></p>
<p style="margin-top:0;margin-bottom:0"><span><i>For the old pp_od_clk_voltage APIs,</i></span></p>
<p style="margin-top:0;margin-bottom:0"><i><span> 1. they output/input the voltages in their exact values. E.g. </span><span style="font-size: 12pt;">"s 1 500 820" will update sclk level 1 to be 500 MHz</span><span style="font-size: 12pt;"> at 820 mV</span></i></p>
<p style="margin-top:0;margin-bottom:0"><span style="font-size: 12pt;"><i>     But for vega20, we did not know its original voltage value. The output/input for voltage are just offest from its original value.</i></span></p>
<p style="margin-top:0;margin-bottom:0"><i>     So, if  to reuse these APIs, we will make them have two different explanations(</i></p>
<p style="margin-top:0;margin-bottom:0"><i>     on vega20 or later, the voltage is offset. On previous ASICs, the voltage is exact value). I think that will confuse the users.</i></p>
<p style="margin-top:0;margin-bottom:0"><i><br>
</i></p>
<p style="margin-top:0;margin-bottom:0"><span><i> 2. they support voltage set for each level. But for vega20, it supports only three pairs of clock/voltage set. And per suggestion,</i></span></p>
<p style="margin-top:0;margin-bottom:0"><span><i>     these three pairs should be: voltage offset of minimum clock level, voltage offset for middle clock level  and voltage offset</i></span></p>
<p style="margin-top:0;margin-bottom:0"><span><i>     for maximum clock level.</i></span></p>
<p style="margin-top:0;margin-bottom:0"><span><br>
</span></p>
<p style="margin-top:0;margin-bottom:0"><span><i>Also, i believe the future ASICs will also take the vega20 OD ways(that is we need the offset, not the exact value for voltage).</i></span></p>
<p style="margin-top:0;margin-bottom:0"><span><br>
</span></p>
<p style="margin-top:0;margin-bottom:0"><span>So, based on previous considerations, i decide to have new APIs instead of reusing existing ones.</span></p>
<p style="margin-top:0;margin-bottom:0"><span><br>
</span></p>
<p style="margin-top:0;margin-bottom:0"><span>Regards,</span></p>
<p style="margin-top:0;margin-bottom:0"><span>Evan</span></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> Zhu, Rex<br>
<b>Sent:</b> Saturday, August 25, 2018 12:32 AM<br>
<b>To:</b> Alex Deucher; Quan, Evan<br>
<b>Cc:</b> amd-gfx list; Xu, Feifei; Kuehling, Felix; Deucher, Alexander; Zhang, Hawking<br>
<b>Subject:</b> RE: [PATCH] drm/amd/powerplay: expose vega20 OD features</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText"><br>
<br>
> -----Original Message-----<br>
> From: Alex Deucher <alexdeucher@gmail.com><br>
> Sent: Friday, August 24, 2018 11:48 PM<br>
> To: Quan, Evan <Evan.Quan@amd.com><br>
> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>; Xu, Feifei<br>
> <Feifei.Xu@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>; Deucher,<br>
> Alexander <Alexander.Deucher@amd.com>; Zhu, Rex <Rex.Zhu@amd.com>;<br>
> Zhang, Hawking <Hawking.Zhang@amd.com><br>
> Subject: Re: [PATCH] drm/amd/powerplay: expose vega20 OD features<br>
> <br>
> On Fri, Aug 24, 2018 at 3:45 AM Evan Quan <evan.quan@amd.com> wrote:<br>
> ><br>
> > Vega20 simplifies the OD logics and it can not fit old OD interfaces.<br>
> > Thus we design new OD interfaces for vega20.<br>
> <br>
> Please split this into two patches, one to add the internal od8_settings API,<br>
> and one to wire it up to sysfs.  A few more comments below.<br>
> <br>
> ><br>
> > Change-Id: I888faec46a81287ae24f452ce16b42c1f6d06d7d<br>
> > Signed-off-by: Evan Quan <evan.quan@amd.com><br>
> > ---<br>
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h       |   8 +<br>
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c        | 125 ++++++++++++<br>
> >  .../gpu/drm/amd/include/kgd_pp_interface.h    |   2 +<br>
> >  drivers/gpu/drm/amd/powerplay/amd_powerplay.c |  37 ++++<br>
> >  .../drm/amd/powerplay/hwmgr/vega20_hwmgr.c    | 191<br>
> +++++++++++++++++-<br>
> >  drivers/gpu/drm/amd/powerplay/inc/hwmgr.h     |   2 +<br>
> >  6 files changed, 362 insertions(+), 3 deletions(-)<br>
> ><br>
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h<br>
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h<br>
> > index ff24e1cc5b65..84b3e6f87abf 100644<br>
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h<br>
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h<br>
> > @@ -357,6 +357,14 @@ enum amdgpu_pcie_gen {<br>
> >                 ((adev)->powerplay.pp_funcs->odn_edit_dpm_table(\<br>
> >                         (adev)->powerplay.pp_handle, type, parameter,<br>
> > size))<br>
> ><br>
> > +#define amdgpu_dpm_get_od8_settings(adev, buf) \<br>
> > +               ((adev)->powerplay.pp_funcs->get_od8_settings(\<br>
> > +                       (adev)->powerplay.pp_handle, buf))<br>
> > +<br>
> > +#define amdgpu_dpm_set_od8_settings(adev, parameter, size) \<br>
> > +               ((adev)->powerplay.pp_funcs->set_od8_settings(\<br>
> > +                       (adev)->powerplay.pp_handle, parameter, size))<br>
> > +<br>
> >  struct amdgpu_dpm {<br>
> >         struct amdgpu_ps        *ps;<br>
> >         /* number of valid power states */ diff --git<br>
> > a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c<br>
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c<br>
> > index daa55fb06171..94cd7c503372 100644<br>
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c<br>
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c<br>
> > @@ -934,6 +934,121 @@ static ssize_t amdgpu_get_busy_percent(struct<br>
> device *dev,<br>
> >         return snprintf(buf, PAGE_SIZE, "%d\n", value);  }<br>
> ><br>
> > +/**<br>
> > + * DOC: pp_od8_settings<br>
> > + *<br>
> > + * The amdgpu driver provides a sysfs API for adjusting the clocks,<br>
> > +voltages,<br>
> > + * power limit, fan speed and temperature. The pp_od8_settings is<br>
> > +used for<br>
> > + * this.<br>
> > + *<br>
> > + * Reading the file will display:<br>
> > + *<br>
> > + * - a name list of the features that are able to be adjusted<br>
> > + *<br>
> > + * - the mininum and maximum allowed value for each supported<br>
> > + *   feature labeled in format of "[mininum - maximum]"<br>
> > + *<br>
> > + * - the current value for each supported feature labeled after<br>
> > + *   ":"<br>
> > + *<br>
> > + * To manually adjust these settings:<br>
> > + *<br>
> > + * - write a string that contains the new value for each supported<br>
> > + *   feature. For those which do not need to be changed, just enter<br>
> > + *   their old value<br>
> > + *<br>
> > + * - make sure the new value is within the allowed ranges between<br>
> > + *   the minimum and maximum<br>
> <br>
> We should probably also make it a set and commit model like we do for<br>
> previous asics for consistency.  That way you can see the changes, and then<br>
> apply them.<br>
> <br>
> > + *<br>
> > + * All the possible supported features:<br>
> > + *<br>
> > + * - Gfx clock<br>
> > + *   The min and max frequency can be specified by GfxclkFmin<br>
> > + *   and GfxclkFmax(both in Mhz). Intermediate levels are<br>
> > + *   stretched/shrunk according to ratio in original table.<br>
> > + *<br>
> > + * - Gfx voltage<br>
> > + *   With three pairs of gfx clk(in Mhz) and offset voltage(in mV)<br>
> > + *   combinations specified, smu fw can calibrate the voltage<br>
> > + *   curve automatically.<br>
> > + *     - GfxclkFreq1 <-> GfxclkOffsetVolt1<br>
> > + *     - GfxclkFreq2 <-> GfxclkOffsetVolt2<br>
> > + *     - GfxclkFreq3 <-> GfxclkOffsetVolt3<br>
> > + *<br>
> > + * - Uclk<br>
> > + *   The max memory clock can be specified by UclkFmax(in Mhz).<br>
> > + *<br>
> > + * - Power limit<br>
> > + *   The power limit can be increased or descreased by<br>
> > + *   OverDrivePct(in percent).<br>
> <br>
> We already expose the power limit via hwmon.  Do we still need it here?<br>
> <br>
> > + *<br>
> > + * - Fan spped<br>
> <br>
> spped -> speed<br>
> <br>
> > + *   The max fan speed can be set by FanMaxinumRpm and the<br>
> <br>
> FanMaxinumRpm -> FanMaximumRpm<br>
> <br>
> > + *   min fan speed by FanMinimumPwm. Also zero rpm enablement<br>
> > + *   is specified by FanZeroRpmEnable.<br>
> > + *<br>
> > + * - Temperature<br>
> > + *   The fan target temperature can be set by FanTargetTemperature<br>
> > + *   and max Tj temperature by MaxOpTemp.<br>
> <br>
> The fan and temp stuff might be a better fit for the hwmon interfaces:<br>
> <a href="https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface" id="LPlnk523014" class="OWAAutoLink" previewremoved="true">
https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface</a><br>
> <br>
> > + *<br>
> > + */<br>
> > +<br>
> > +static ssize_t amdgpu_get_pp_od8_settings(struct device *dev,<br>
> > +               struct device_attribute *attr,<br>
> > +               char *buf)<br>
> > +{<br>
> > +       struct drm_device *ddev = dev_get_drvdata(dev);<br>
> > +       struct amdgpu_device *adev = ddev->dev_private;<br>
> > +<br>
> > +       if (adev->powerplay.pp_funcs->get_od8_settings)<br>
> > +               return amdgpu_dpm_get_od8_settings(adev, buf);<br>
> > +<br>
> > +       return snprintf(buf, PAGE_SIZE, "\n"); }<br>
> > +<br>
> > +static ssize_t amdgpu_set_pp_od8_settings(struct device *dev,<br>
> > +               struct device_attribute *attr,<br>
> > +               const char *buf,<br>
> > +               size_t count)<br>
> > +{<br>
> > +       struct drm_device *ddev = dev_get_drvdata(dev);<br>
> > +       struct amdgpu_device *adev = ddev->dev_private;<br>
> > +       char *sub_str, *tmp_str, buf_cpy[128];<br>
> > +       const char delimiter[3] = {' ', '\n', '\0'};<br>
> > +       uint32_t parameter_size = 0;<br>
> > +       long parameter[64];<br>
> > +       int ret = 0xff;<br>
> > +<br>
> > +       if (count >= sizeof(buf_cpy) / sizeof(buf_cpy[0]))<br>
> > +               return -EINVAL;<br>
> > +<br>
> > +       memcpy(buf_cpy, buf, count);<br>
> > +       buf_cpy[count] = '\0';<br>
> > +<br>
> > +       tmp_str = buf_cpy;<br>
> > +<br>
> > +       while (isspace(*tmp_str)) tmp_str++;<br>
> > +<br>
> > +       while (tmp_str[0]) {<br>
> > +               sub_str = strsep(&tmp_str, delimiter);<br>
> > +               ret = kstrtol(sub_str, 0, &parameter[parameter_size]);<br>
> > +               if (ret)<br>
> > +                       return -EINVAL;<br>
> > +               parameter_size++;<br>
> > +<br>
> > +               while (isspace(*tmp_str))<br>
> > +                       tmp_str++;<br>
> > +       }<br>
> > +<br>
> > +       if (adev->powerplay.pp_funcs->set_od8_settings)<br>
> > +               ret = amdgpu_dpm_set_od8_settings(adev, parameter,<br>
> > + parameter_size);<br>
> > +<br>
> > +       if (ret)<br>
> > +               return -EINVAL;<br>
> > +<br>
> > +       return count;<br>
> > +}<br>
> > +<br>
> >  static DEVICE_ATTR(power_dpm_state, S_IRUGO | S_IWUSR,<br>
> > amdgpu_get_dpm_state, amdgpu_set_dpm_state);  static<br>
> DEVICE_ATTR(power_dpm_force_performance_level, S_IRUGO | S_IWUSR,<br>
> >                    amdgpu_get_dpm_forced_performance_level,<br>
> > @@ -969,6 +1084,9 @@ static DEVICE_ATTR(pp_od_clk_voltage, S_IRUGO<br>
> | S_IWUSR,<br>
> >                 amdgpu_set_pp_od_clk_voltage);  static<br>
> > DEVICE_ATTR(gpu_busy_percent, S_IRUGO,<br>
> >                 amdgpu_get_busy_percent, NULL);<br>
> > +static DEVICE_ATTR(pp_od8_settings, S_IRUGO | S_IWUSR,<br>
> > +               amdgpu_get_pp_od8_settings,<br>
> > +               amdgpu_set_pp_od8_settings);<br>
> <br>
> Rather than adding a new file, would it make more sense to expose this new<br>
> API on the same file we use for older asics?  Otherwise, every time the<br>
> interface changes, we'll need to add a new file.  We already handle asic<br>
> specific differences in the power profile sysfs files.<br>
> <br>
> ><br>
> >  static ssize_t amdgpu_hwmon_show_temp(struct device *dev,<br>
> >                                       struct device_attribute *attr,<br>
> > @@ -1879,6 +1997,13 @@ int amdgpu_pm_sysfs_init(struct<br>
> amdgpu_device *adev)<br>
> >                                 "gpu_busy_level\n");<br>
> >                 return ret;<br>
> >         }<br>
> > +       ret = device_create_file(adev->dev,<br>
> > +                       &dev_attr_pp_od8_settings);<br>
> > +       if (ret) {<br>
> > +               DRM_ERROR("failed to create device file "<br>
> > +                               "pp_od8_settings\n");<br>
> > +               return ret;<br>
> > +       }<br>
> >         ret = amdgpu_debugfs_pm_init(adev);<br>
> >         if (ret) {<br>
> >                 DRM_ERROR("Failed to register debugfs file for<br>
> > dpm!\n"); diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h<br>
> > b/drivers/gpu/drm/amd/include/kgd_pp_interface.h<br>
> > index 6a41b81c7325..e23746ba53bf 100644<br>
> > --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h<br>
> > +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h<br>
> > @@ -269,6 +269,8 @@ struct amd_pm_funcs {<br>
> >         int (*get_display_mode_validation_clocks)(void *handle,<br>
> >                 struct amd_pp_simple_clock_info *clocks);<br>
> >         int (*notify_smu_enable_pwe)(void *handle);<br>
> > +       int (*get_od8_settings)(void *handle, char *buf);<br>
> > +       int (*set_od8_settings)(void *handle, long *input, uint32_t<br>
> > + size);<br>
> >  };<br>
> ><br>
> >  #endif<br>
> > diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c<br>
> > b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c<br>
> > index da4ebff5b74d..4bcd06306698 100644<br>
> > --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c<br>
> > +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c<br>
> > @@ -883,6 +883,41 @@ static int pp_odn_edit_dpm_table(void *handle,<br>
> uint32_t type, long *input, uint3<br>
> >         return hwmgr->hwmgr_func->odn_edit_dpm_table(hwmgr, type,<br>
> > input, size);  }<br>
> ><br>
> > +static int pp_get_od8_settings(void *handle, char *buf) {<br>
> > +       struct pp_hwmgr *hwmgr = handle;<br>
> > +<br>
> > +       if (!hwmgr || !hwmgr->pm_en || !buf)<br>
> > +               return -EINVAL;<br>
> > +<br>
> > +       if (hwmgr->hwmgr_func->get_od8_settings == NULL) {<br>
> > +               pr_info("%s was not implemented.\n", __func__);<br>
> > +               return snprintf(buf, PAGE_SIZE, "\n");<br>
> > +       }<br>
> > +<br>
> > +       return hwmgr->hwmgr_func->get_od8_settings(hwmgr, buf); }<br>
> > +<br>
> > +static int pp_set_od8_settings(void *handle, long *input, uint32_t<br>
> > +size) {<br>
> > +       struct pp_hwmgr *hwmgr = handle;<br>
> > +       int ret = -EINVAL;<br>
> > +<br>
> > +       if (!hwmgr || !hwmgr->pm_en)<br>
> > +               return ret;<br>
> > +<br>
> > +       if (hwmgr->hwmgr_func->set_od8_settings == NULL) {<br>
> > +               pr_info("%s was not implemented.\n", __func__);<br>
> > +               return ret;<br>
> > +       }<br>
> > +<br>
> > +       mutex_lock(&hwmgr->smu_lock);<br>
> > +       ret = hwmgr->hwmgr_func->set_od8_settings(hwmgr, input, size);<br>
> > +       mutex_unlock(&hwmgr->smu_lock);<br>
> > +<br>
> > +       return ret;<br>
> > +}<br>
> > +<br>
> >  static int pp_dpm_switch_power_profile(void *handle,<br>
> >                 enum PP_SMC_POWER_PROFILE type, bool en)  { @@ -1272,6<br>
> > +1307,8 @@ static const struct amd_pm_funcs pp_dpm_funcs = {<br>
> >         .get_power_profile_mode = pp_get_power_profile_mode,<br>
> >         .set_power_profile_mode = pp_set_power_profile_mode,<br>
> >         .odn_edit_dpm_table = pp_odn_edit_dpm_table,<br>
> > +       .get_od8_settings = pp_get_od8_settings,<br>
> > +       .set_od8_settings = pp_set_od8_settings,<br>
> >         .set_power_limit = pp_set_power_limit,<br>
> >         .get_power_limit = pp_get_power_limit,<br>
> >  /* export to DC */<br>
> > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c<br>
> > b/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c<br>
> > index fb32b28afa66..ececa2f7fe5f 100644<br>
> > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c<br>
> > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c<br>
> > @@ -1099,7 +1099,188 @@ static int<br>
> vega20_od8_initialize_default_settings(<br>
> >         return 0;<br>
> >  }<br>
> ><br>
> > -static int vega20_od8_set_settings(<br>
> > +static int vega20_get_od8_settings(<br>
> > +               struct pp_hwmgr *hwmgr,<br>
> > +               char *buf)<br>
> > +{<br>
> > +       struct vega20_hwmgr *data =<br>
> > +                       (struct vega20_hwmgr *)(hwmgr->backend);<br>
> > +       struct vega20_od8_single_setting *od8_settings =<br>
> > +                       data->od8_settings.od8_settings_array;<br>
> > +       OverDriveTable_t od_table;<br>
> > +       uint32_t size = 0;<br>
> > +       int ret = 0;<br>
> > +<br>
> > +       if (!buf)<br>
> > +               return -EINVAL;<br>
> > +<br>
> > +       ret = vega20_copy_table_from_smc(hwmgr,<br>
> > +                       (uint8_t *)(&od_table),<br>
> > +                       TABLE_OVERDRIVE);<br>
> > +       PP_ASSERT_WITH_CODE(!ret,<br>
> > +                       "Failed to export over drive table!",<br>
> > +                       return ret);<br>
> > +<br>
> > +       if (od8_settings[OD8_SETTING_GFXCLK_FMIN].feature_id)<br>
> > +               size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "GfxclkFmin",<br>
> > +                               od8_settings[OD8_SETTING_GFXCLK_FMIN].min_value,<br>
> > +                               od8_settings[OD8_SETTING_GFXCLK_FMIN].max_value,<br>
> > +                               od_table.GfxclkFmin);<br>
> > +<br>
> > +       if (od8_settings[OD8_SETTING_GFXCLK_FMAX].feature_id)<br>
> > +               size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "GfxclkFmax",<br>
> > +                               od8_settings[OD8_SETTING_GFXCLK_FMAX].min_value,<br>
> > +                               od8_settings[OD8_SETTING_GFXCLK_FMAX].max_value,<br>
> > +                               od_table.GfxclkFmax);<br>
> > +<br>
> > +       if (od8_settings[OD8_SETTING_GFXCLK_FREQ1].feature_id)<br>
> > +               size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "GfxclkFreq1",<br>
> > +                               od8_settings[OD8_SETTING_GFXCLK_FREQ1].min_value,<br>
> > +                               od8_settings[OD8_SETTING_GFXCLK_FREQ1].max_value,<br>
> > +                               od_table.GfxclkFreq1);<br>
> > +<br>
> > +       if (od8_settings[OD8_SETTING_GFXCLK_VOLTAGE1].feature_id)<br>
> > +               size += sprintf(buf + size, "%22s[%d - %d]: %u\n",<br>
> "GfxclkOffsetVolt1",<br>
> > +<br>
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE1].min_value,<br>
> > +<br>
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE1].max_value,<br>
> > +                               od_table.GfxclkOffsetVolt1);<br>
> > +<br>
> > +       if (od8_settings[OD8_SETTING_GFXCLK_FREQ2].feature_id)<br>
> > +               size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "GfxclkFreq2",<br>
> > +                               od8_settings[OD8_SETTING_GFXCLK_FREQ2].min_value,<br>
> > +                               od8_settings[OD8_SETTING_GFXCLK_FREQ2].max_value,<br>
> > +                               od_table.GfxclkFreq2);<br>
> > +<br>
> > +       if (od8_settings[OD8_SETTING_GFXCLK_VOLTAGE2].feature_id)<br>
> > +               size += sprintf(buf + size, "%22s[%d - %d]: %u\n",<br>
> "GfxclkOffsetVolt2",<br>
> > +<br>
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE2].min_value,<br>
> > +<br>
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE2].max_value,<br>
> > +                               od_table.GfxclkOffsetVolt2);<br>
> > +<br>
> > +       if (od8_settings[OD8_SETTING_GFXCLK_FREQ3].feature_id)<br>
> > +               size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "GfxclkFreq3",<br>
> > +                               od8_settings[OD8_SETTING_GFXCLK_FREQ3].min_value,<br>
> > +                               od8_settings[OD8_SETTING_GFXCLK_FREQ3].max_value,<br>
> > +                               od_table.GfxclkFreq3);<br>
> > +<br>
> > +       if (od8_settings[OD8_SETTING_GFXCLK_VOLTAGE3].feature_id)<br>
> > +               size += sprintf(buf + size, "%22s[%d - %d]: %u\n",<br>
> "GfxclkOffsetVolt3",<br>
> > +<br>
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE3].min_value,<br>
> > +<br>
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE3].max_value,<br>
> > +                               od_table.GfxclkOffsetVolt3);<br>
> > +<br>
> > +       if (od8_settings[OD8_SETTING_UCLK_FMAX].feature_id)<br>
> > +               size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "UclkFmax",<br>
> > +                               od8_settings[OD8_SETTING_UCLK_FMAX].min_value,<br>
> > +                               od8_settings[OD8_SETTING_UCLK_FMAX].max_value,<br>
> > +                               od_table.UclkFmax);<br>
> > +<br>
> > +       if (od8_settings[OD8_SETTING_POWER_PERCENTAGE].feature_id)<br>
> > +               size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "OverDrivePct",<br>
> > +<br>
> od8_settings[OD8_SETTING_POWER_PERCENTAGE].min_value,<br>
> > +<br>
> od8_settings[OD8_SETTING_POWER_PERCENTAGE].max_value,<br>
> > +                               od_table.OverDrivePct);<br>
> > +<br>
> > +       if (od8_settings[OD8_SETTING_FAN_ACOUSTIC_LIMIT].feature_id)<br>
> > +               size += sprintf(buf + size, "%22s[%d - %d]: %u\n",<br>
> "FanMaximumRpm",<br>
> > +<br>
> od8_settings[OD8_SETTING_FAN_ACOUSTIC_LIMIT].min_value,<br>
> > +<br>
> od8_settings[OD8_SETTING_FAN_ACOUSTIC_LIMIT].max_value,<br>
> > +                               od_table.FanMaximumRpm);<br>
> > +<br>
> > +       if (od8_settings[OD8_SETTING_FAN_MIN_SPEED].feature_id)<br>
> > +               size += sprintf(buf + size, "%22s[%d - %d]: %u\n",<br>
> "FanMinimumPwm",<br>
> > +<br>
> od8_settings[OD8_SETTING_FAN_MIN_SPEED].min_value,<br>
> > +<br>
> od8_settings[OD8_SETTING_FAN_MIN_SPEED].max_value,<br>
> > +                               od_table.FanMinimumPwm);<br>
> > +<br>
> > +       if (od8_settings[OD8_SETTING_FAN_TARGET_TEMP].feature_id)<br>
> > +               size += sprintf(buf + size, "%22s[%d - %d]: %u\n",<br>
> "FanTargetTemperautre",<br>
> > +<br>
> od8_settings[OD8_SETTING_FAN_TARGET_TEMP].min_value,<br>
> > +<br>
> od8_settings[OD8_SETTING_FAN_TARGET_TEMP].max_value,<br>
> > +                               od_table.FanTargetTemperature);<br>
> > +<br>
> > +       if (od8_settings[OD8_SETTING_OPERATING_TEMP_MAX].feature_id)<br>
> > +               size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "MaxOpTemp",<br>
> > +<br>
> od8_settings[OD8_SETTING_OPERATING_TEMP_MAX].min_value,<br>
> > +<br>
> od8_settings[OD8_SETTING_OPERATING_TEMP_MAX].max_value,<br>
> > +                               od_table.MaxOpTemp);<br>
> > +<br>
> > +       if (od8_settings[OD8_SETTING_FAN_ZERO_RPM_CONTROL].feature_id)<br>
> > +               size += sprintf(buf + size, "%22s[%d - %d]: %u\n",<br>
> "FanZeroRpmEnable",<br>
> > +<br>
> od8_settings[OD8_SETTING_FAN_ZERO_RPM_CONTROL].min_value,<br>
> > +<br>
> od8_settings[OD8_SETTING_FAN_ZERO_RPM_CONTROL].max_value,<br>
> > +                               od_table.FanZeroRpmEnable);<br>
> > +<br>
> > +       return size;<br>
> > +}<br>
> > +<br>
> > +static int vega20_set_od8_settings(<br>
> > +               struct pp_hwmgr *hwmgr,<br>
> > +               long *input,<br>
> > +               uint32_t size)<br>
> > +{<br>
> > +       struct vega20_hwmgr *data =<br>
> > +                       (struct vega20_hwmgr *)(hwmgr->backend);<br>
> > +       struct vega20_od8_single_setting *od8_settings =<br>
> > +                       data->od8_settings.od8_settings_array;<br>
> > +       OverDriveTable_t od_table;<br>
> > +       uint16_t *buf = (uint16_t *)(&od_table);<br>
> > +       uint32_t i, j = 0, k;<br>
> > +       int ret = 0;<br>
> > +<br>
> > +       /* retrieve current overdrive settings */<br>
> > +       ret = vega20_copy_table_from_smc(hwmgr,<br>
> > +                       (uint8_t *)(&od_table),<br>
> > +                       TABLE_OVERDRIVE);<br>
> > +       PP_ASSERT_WITH_CODE(!ret,<br>
> > +                       "Failed to export over drive table!",<br>
> > +                       return ret);<br>
> > +<br>
> > +       for (i = 0;<br>
> > +            i < (sizeof(OverDriveTable_t) / sizeof(uint16_t)) - 1;<br>
> > +            i++) {<br>
> > +               /* overdrive table supports no dram timing setting */<br>
> > +               k = (i == OD8_SETTING_AC_TIMING) ?<br>
> > +                       OD8_SETTING_FAN_ZERO_RPM_CONTROL : i;<br>
> > +               /*<br>
> > +                * Not all OD settings are supported and exported<br>
> > +                * to user.<br>
> > +                * So, updated only those which are supported and<br>
> > +                * exported to user.<br>
> > +                */<br>
> > +               if (od8_settings[k].feature_id) {<br>
> > +                       if (input[j] < od8_settings[k].min_value ||<br>
> > +                           input[j] > od8_settings[k].max_value)<br>
> > +                               return -EINVAL;<br>
> > +<br>
> > +                       buf[i] = input[j];<br>
> > +                       if (j++ >= size - 1)<br>
> > +                               break;<br>
> > +               }<br>
> > +       }<br>
> > +<br>
> > +       /* enable the new overdrive settings */<br>
> > +       ret = vega20_copy_table_to_smc(hwmgr,<br>
> > +                       (uint8_t *)(&od_table),<br>
> > +                       TABLE_OVERDRIVE);<br>
> > +       PP_ASSERT_WITH_CODE(!ret,<br>
> > +                       "Failed to import over drive table!",<br>
> > +                       return ret);<br>
> > +<br>
> > +       /* update the current_value of overdrive settings */<br>
> > +       for (i = 0;<br>
> > +            i < (sizeof(OverDriveTable_t) / sizeof(uint16_t)) - 1;<br>
> > +            i++) {<br>
> > +               k = (i == OD8_SETTING_AC_TIMING) ?<br>
> > +                       OD8_SETTING_FAN_ZERO_RPM_CONTROL : i;<br>
> > +               if (od8_settings[k].feature_id)<br>
> > +                       od8_settings[k].current_value = buf[i];<br>
> > +       }<br>
> > +<br>
> > +       return 0;<br>
> > +}<br>
> > +<br>
> > +static int vega20_od8_set_single_setting(<br>
> >                 struct pp_hwmgr *hwmgr,<br>
> >                 uint32_t index,<br>
> >                 uint32_t value)<br>
> > @@ -1198,7 +1379,7 @@ static int vega20_set_sclk_od(<br>
> >         do_div(od_sclk, 100);<br>
> >         od_sclk +=<br>
> > golden_sclk_table->dpm_levels[golden_sclk_table->count - 1].value;<br>
> ><br>
> > -       ret = vega20_od8_set_settings(hwmgr, OD8_SETTING_GFXCLK_FMAX,<br>
> od_sclk);<br>
> > +       ret = vega20_od8_set_single_setting(hwmgr,<br>
> > + OD8_SETTING_GFXCLK_FMAX, od_sclk);<br>
> >         PP_ASSERT_WITH_CODE(!ret,<br>
> >                         "[SetSclkOD] failed to set od gfxclk!",<br>
> >                         return ret);<br>
> > @@ -1245,7 +1426,7 @@ static int vega20_set_mclk_od(<br>
> >         do_div(od_mclk, 100);<br>
> >         od_mclk +=<br>
> > golden_mclk_table->dpm_levels[golden_mclk_table->count - 1].value;<br>
> ><br>
> > -       ret = vega20_od8_set_settings(hwmgr, OD8_SETTING_UCLK_FMAX,<br>
> od_mclk);<br>
> > +       ret = vega20_od8_set_single_setting(hwmgr,<br>
> > + OD8_SETTING_UCLK_FMAX, od_mclk);<br>
> >         PP_ASSERT_WITH_CODE(!ret,<br>
> >                         "[SetMclkOD] failed to set od memclk!",<br>
> >                         return ret);<br>
> > @@ -2966,6 +3147,10 @@ static const struct pp_hwmgr_func<br>
> vega20_hwmgr_funcs = {<br>
> >                 vega20_get_power_profile_mode,<br>
> >         .set_power_profile_mode =<br>
> >                 vega20_set_power_profile_mode,<br>
> > +       .get_od8_settings =<br>
> > +               vega20_get_od8_settings,<br>
> > +       .set_od8_settings =<br>
> > +               vega20_set_od8_settings,<br>
> >         /* od related */<br>
> >         .set_power_limit =<br>
> >                 vega20_set_power_limit, diff --git<br>
> > a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h<br>
> > b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h<br>
> > index a6d92128b19c..285af1728e2e 100644<br>
> > --- a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h<br>
> > +++ b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h<br>
> > @@ -328,6 +328,8 @@ struct pp_hwmgr_func {<br>
> >         int (*set_power_limit)(struct pp_hwmgr *hwmgr, uint32_t n);<br>
> >         int (*powergate_mmhub)(struct pp_hwmgr *hwmgr);<br>
> >         int (*smus_notify_pwe)(struct pp_hwmgr *hwmgr);<br>
> > +       int (*get_od8_settings)(struct pp_hwmgr *hwmgr, char *buf);<br>
> > +       int (*set_od8_settings)(struct pp_hwmgr *hwmgr, long *input,<br>
> > + uint32_t size);<br>
<br>
Those new added interfaces for OD parameters get/set seems have no difference with the old interface.<br>
  <br>
        int (*print_clock_levels)(struct pp_hwmgr *hwmgr, enum pp_clock_type type, char *buf);<br>
        int (*odn_edit_dpm_table)(struct pp_hwmgr *hwmgr, enum PP_OD_DPM_TABLE_COMMAND type, long *input, uint32_t size);<br>
<br>
Regards<br>
Rex<br>
> >  };<br>
> ><br>
> >  struct pp_table_func {<br>
> > --<br>
> > 2.18.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="LPlnk84178" class="OWAAutoLink" previewremoved="true">
https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a><br>
</div>
</span></font></div>
</div>
</div>
</body>
</html>