Approach 3 seems not workable since we do not know the default value.
I would prefer approach 1 if we have to fit into original APIs. There needs three pairs of <frequency, offset> to recalculate the voltage curve. Instead of silently calucating dpm_mid_leveâ??s offset in our driver, i would prefer to have it specified by user.
Also, we may need to update the description for the OD apis to tell user the different behaviors on vega20 and previous ASICs.
/**
* DOC: pp_od_clk_voltage
*
* The amdgpu driver provides a sysfs API for adjusting the clocks and voltages
* in each power level within a power state. The pp_od_clk_voltage is used for
* this.
*
* Reading the file will display:
*
* - a list of engine clock levels and voltages labeled OD_SCLK
*
* - a list of memory clock levels and voltages labeled OD_MCLK
*
* - a list of valid ranges for sclk, mclk, and voltage labeled OD_RANGE
*
* To manually adjust these settings, first select manual using
* power_dpm_force_performance_level. Enter a new value for each
* level by writing a string that contains "s/m level clock voltage" to
* the file. E.g., "s 1 500 820" will update sclk level 1 to be 500 MHz
* at 820 mV; "m 0 350 810" will update mclk level 0 to be 350 MHz at
* 810 mV. When you have edited all of the states as needed, write
* "c" (commit) to the file to commit your changes. If you want to reset to the
* default power levels, write "r" (reset) to the file to reset them.
*
*/
Regards,
Evan
________________________________
From: Alex Deucher <alexdeucher@xxxxxxxxx>
Sent: Wednesday, August 29, 2018 1:26:42 AM
To: Zhu, Rex
Cc: Deucher, Alexander; Quan, Evan; amd-gfx list; Xu, Feifei; Kuehling, Felix; Zhang, Hawking
Subject: Re: [PATCH] drm/amd/powerplay: expose vega20 OD features
On Tue, Aug 28, 2018 at 12:35 PM Zhu, Rex <Rex.Zhu at amd.com> wrote:
>
> Got it. Thanks Alex.
>
>
>
> I think we can simplify the od_clk_voltage on vega20.
>
>
>
> When user cat pp_od_clk_voltage
>
> The output can be simplified as:
>
>
>
> Sclk:
>
> 0 clock_value voltage_offset
>
> 7 clock_value voltage_offset
>
> Mclk
>
> 3 mclk_value
>
> Range:
>
> sclk {min, max}
>
> mclk {min, max}
>
> voltage{min, max}
>
>
>
> for sclk,
>
> user can only set the sclk in dpm level0 and level7, the clock in other levels will be recalculated by smu automatically based on userâ??s setting.
>
>
>
> For voltage, the avfs is enabled, so the voltage in each level is not a unique value.
>
> It is reasonable that export voltage offset to user.
>
> The voltage offset is 0 by default.
>
>
>
> The voltage offset in dpm_0, dpm_mid_level, dpm7_level can be configured.
>
> But in order to keep in consistent with sclkâ??s od,
>
> We only let user change the offset in dpm0 and dpm7, for dpm_mid_leveâ??s offset, we can calculate in driver by (offset0 + offset7)/2.
>
Seems pretty good. How about something like the following:
OD_SCLK:
min: clock_value
max: clock_value
OD_MCLK:
max: clock_value
OD_VDDC:
0: clock_value voltage_offset
1: clock_value voltage_offset
2: clock_value voltage_offset
OD_RANGE:
SCLK: clock_value clock_value
MCLK: clock_value clock_value
VDDC: voltage_value voltage_value
OD_SCLK:
min: 300Mhz
max: 1200Mhz
OD_MCLK:
max: 1500Mhz
OD_VDDC:
0: 300Mhz 0mV
1: 600Mhz 0mV
2: 1200Mhz 0mV
OD_RANGE:
SCLK: 300MHz 1200MHz
MCLK: 300MHz 1500MHz
VDDC: 700mV 1200mV
That would give you the fullest control. Alternatively, we could
simplify this a bit by deriving the mid point in the voltage curve.
E.g.,
OD_SCLK:
min: clock_value
max: clock_value
OD_MCLK:
max: clock_value
OD_VDDC:
min: clock_value voltage_offset
max: clock_value voltage_offset
OD_RANGE:
SCLK: clock_value clock_value
MCLK: clock_value clock_value
VDDC: voltage_value voltage_value
OD_SCLK:
min: 300Mhz
max: 1200Mhz
OD_MCLK:
max 1500Mhz
OD_VDDC:
min: 300Mhz 0mV
max: 1200Mhz 0mV
OD_RANGE:
SCLK: 300MHz 1200MHz
MCLK: 300MHz 1500MHz
VDDC: 700mV 1200mV
I think we could also make the interface backwards compatible. E.g.,
OD_SCLK:
0 300Mhz 900mV
7 1200Mhz 1000mV
OD_MCLK:
3 1500Mhz 900mV
OD_RANGE:
SCLK: 300MHz 1200MHz
MCLK: 300MHz 1500MHz
VDDC: 700mV 1200mV
We can ignore the voltage for OD_MCLK and then calculate the offsets
based on the absolute value specified in OD_SCLK (e.g., default value
- specified value = offset), then calculate the midpoint based on the
end points. It would be backwards compatible, but somewhat more
confusing for users. Probably one of the first two would be better.
Thoughts?
Alex
>
>
> For mclk,
>
> Smu just let us change the max clock in dpm3(high level). And the mclk in other dpm levels can not be changed.
>
>
>
> Any concerns?
>
>
>
>
>
> Best Regards
>
> Rex
>
>
>
> From: Deucher, Alexander
> Sent: Wednesday, August 29, 2018 12:03 AM
> To: Quan, Evan <Evan.Quan at amd.com>; Alex Deucher <alexdeucher at gmail.com>
> Cc: Zhu, Rex <Rex.Zhu at amd.com>; amd-gfx list <amd-gfx at lists.freedesktop.org>; Xu, Feifei <Feifei.Xu at amd.com>; Kuehling, Felix <Felix.Kuehling at amd.com>; Zhang, Hawking <Hawking.Zhang at amd.com>
> Subject: Re: [PATCH] drm/amd/powerplay: expose vega20 OD features
>
>
>
> Right. I'm saying use the same sysfs file (pp_od_clk_voltage) for all devices rather than adding a new one. The output of reading that file will be different on each asic that supports a different way of adjusting the clock and voltage parameters and the supported inputs will be different. We do the same thing today for pp_power_profile_mode. Vega10 and smu7 parts have different parameters, but we use the same files. See the attached patch to an example.
>
>
>
> Alex
>
> ________________________________
>
> From: Quan, Evan
> Sent: Tuesday, August 28, 2018 11:51:16 AM
> To: Alex Deucher
> Cc: Zhu, Rex; Deucher, Alexander; amd-gfx list; Xu, Feifei; Kuehling, Felix; Zhang, Hawking
> Subject: Re: [PATCH] drm/amd/powerplay: expose vega20 OD features
>
>
>
> Hi Alex,
>
>
>
> Sorry, i cannot get your point.
>
>
>
> The OD feature sysfs file used by previous ASICs is "pp_od_clk_voltage". And it binds to two APIs "amdgpu_get_pp_od_clk_voltage" and "amdgpu_set_pp_od_clk_voltage".
>
>
>
> static DEVICE_ATTR(pp_od_clk_voltage, S_IRUGO | S_IWUSR,
>
> amdgpu_get_pp_od_clk_voltage,
>
> amdgpu_set_pp_od_clk_voltage);
>
>
>
> I was saying we cannot reuse these two APIs for vega20 since we have no way to know the exact voltage value for each dpm level(that is what these two APIs required). Thus we cannot share the same sysfs file.
>
>
>
> Do you mean we can bind the sysfs file to other APIs? How?
>
>
>
> Regards,
>
> Evan
>
> ________________________________
>
> From: Alex Deucher <alexdeucher at gmail.com>
> Sent: Monday, August 27, 2018 10:01:49 PM
> To: Quan, Evan
> Cc: Zhu, Rex; Deucher, Alexander; amd-gfx list; Xu, Feifei; Kuehling, Felix; Zhang, Hawking
> Subject: Re: [PATCH] drm/amd/powerplay: expose vega20 OD features
>
>
>
> On Mon, Aug 27, 2018 at 12:04 AM Quan, Evan <Evan.Quan at amd.com> wrote:
> >
> > Hi @Deucher, Alexander & @Zhu, Rex,
> >
> >
> > I can remove the od setings for power limit and fan speed related since they seem redundant with other sysfs apis.
> >
> > But for the clock/voltage related, i can not see how to reuse old pp_od_clk_voltage APIs.
> >
> >
> > For the old pp_od_clk_voltage APIs,
> >
> > 1. they output/input the voltages in their exact values. E.g. "s 1 500 820" will update sclk level 1 to be 500 MHz at 820 mV
> >
> > But for vega20, we did not know its original voltage value. The output/input for voltage are just offest from its original value.
> >
> > So, if to reuse these APIs, we will make them have two different explanations(
> >
> > on vega20 or later, the voltage is offset. On previous ASICs, the voltage is exact value). I think that will confuse the users.
> >
> >
> > 2. they support voltage set for each level. But for vega20, it supports only three pairs of clock/voltage set. And per suggestion,
> >
> > these three pairs should be: voltage offset of minimum clock level, voltage offset for middle clock level and voltage offset
> >
> > for maximum clock level.
> >
> >
> > 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).
> >
> >
> > So, based on previous considerations, i decide to have new APIs instead of reusing existing ones.
>
> I'm not saying to use the same API. Use the new API, but expose the
> API using the same sysfs file rather than adding a new one. Same way
> we handle the power profiles; E.g., the API is different between smu7
> and vega10, but they both use the same sysfs file.
>
> Alex
>
>
> >
> >
> > Regards,
> >
> > Evan
> >
> > ________________________________
> > From: Zhu, Rex
> > Sent: Saturday, August 25, 2018 12:32 AM
> > To: Alex Deucher; Quan, Evan
> > Cc: amd-gfx list; Xu, Feifei; Kuehling, Felix; Deucher, Alexander; Zhang, Hawking
> > Subject: RE: [PATCH] drm/amd/powerplay: expose vega20 OD features
> >
> >
> >
> > > -----Original Message-----
> > > From: Alex Deucher <alexdeucher at gmail.com>
> > > Sent: Friday, August 24, 2018 11:48 PM
> > > To: Quan, Evan <Evan.Quan at amd.com>
> > > Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>; Xu, Feifei
> > > <Feifei.Xu at amd.com>; Kuehling, Felix <Felix.Kuehling at amd.com>; Deucher,
> > > Alexander <Alexander.Deucher at amd.com>; Zhu, Rex <Rex.Zhu at amd.com>;
> > > Zhang, Hawking <Hawking.Zhang at amd.com>
> > > Subject: Re: [PATCH] drm/amd/powerplay: expose vega20 OD features
> > >
> > > On Fri, Aug 24, 2018 at 3:45 AM Evan Quan <evan.quan at amd.com> wrote:
> > > >
> > > > Vega20 simplifies the OD logics and it can not fit old OD interfaces.
> > > > Thus we design new OD interfaces for vega20.
> > >
> > > Please split this into two patches, one to add the internal od8_settings API,
> > > and one to wire it up to sysfs. A few more comments below.
> > >
> > > >
> > > > Change-Id: I888faec46a81287ae24f452ce16b42c1f6d06d7d
> > > > Signed-off-by: Evan Quan <evan.quan at amd.com>
> > > > ---
> > > > drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h | 8 +
> > > > drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 125 ++++++++++++
> > > > .../gpu/drm/amd/include/kgd_pp_interface.h | 2 +
> > > > drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 37 ++++
> > > > .../drm/amd/powerplay/hwmgr/vega20_hwmgr.c | 191
> > > +++++++++++++++++-
> > > > drivers/gpu/drm/amd/powerplay/inc/hwmgr.h | 2 +
> > > > 6 files changed, 362 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
> > > > index ff24e1cc5b65..84b3e6f87abf 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
> > > > @@ -357,6 +357,14 @@ enum amdgpu_pcie_gen {
> > > > ((adev)->powerplay.pp_funcs->odn_edit_dpm_table(\
> > > > (adev)->powerplay.pp_handle, type, parameter,
> > > > size))
> > > >
> > > > +#define amdgpu_dpm_get_od8_settings(adev, buf) \
> > > > + ((adev)->powerplay.pp_funcs->get_od8_settings(\
> > > > + (adev)->powerplay.pp_handle, buf))
> > > > +
> > > > +#define amdgpu_dpm_set_od8_settings(adev, parameter, size) \
> > > > + ((adev)->powerplay.pp_funcs->set_od8_settings(\
> > > > + (adev)->powerplay.pp_handle, parameter, size))
> > > > +
> > > > struct amdgpu_dpm {
> > > > struct amdgpu_ps *ps;
> > > > /* number of valid power states */ diff --git
> > > > a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > > > index daa55fb06171..94cd7c503372 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > > > @@ -934,6 +934,121 @@ static ssize_t amdgpu_get_busy_percent(struct
> > > device *dev,
> > > > return snprintf(buf, PAGE_SIZE, "%d\n", value); }
> > > >
> > > > +/**
> > > > + * DOC: pp_od8_settings
> > > > + *
> > > > + * The amdgpu driver provides a sysfs API for adjusting the clocks,
> > > > +voltages,
> > > > + * power limit, fan speed and temperature. The pp_od8_settings is
> > > > +used for
> > > > + * this.
> > > > + *
> > > > + * Reading the file will display:
> > > > + *
> > > > + * - a name list of the features that are able to be adjusted
> > > > + *
> > > > + * - the mininum and maximum allowed value for each supported
> > > > + * feature labeled in format of "[mininum - maximum]"
> > > > + *
> > > > + * - the current value for each supported feature labeled after
> > > > + * ":"
> > > > + *
> > > > + * To manually adjust these settings:
> > > > + *
> > > > + * - write a string that contains the new value for each supported
> > > > + * feature. For those which do not need to be changed, just enter
> > > > + * their old value
> > > > + *
> > > > + * - make sure the new value is within the allowed ranges between
> > > > + * the minimum and maximum
> > >
> > > We should probably also make it a set and commit model like we do for
> > > previous asics for consistency. That way you can see the changes, and then
> > > apply them.
> > >
> > > > + *
> > > > + * All the possible supported features:
> > > > + *
> > > > + * - Gfx clock
> > > > + * The min and max frequency can be specified by GfxclkFmin
> > > > + * and GfxclkFmax(both in Mhz). Intermediate levels are
> > > > + * stretched/shrunk according to ratio in original table.
> > > > + *
> > > > + * - Gfx voltage
> > > > + * With three pairs of gfx clk(in Mhz) and offset voltage(in mV)
> > > > + * combinations specified, smu fw can calibrate the voltage
> > > > + * curve automatically.
> > > > + * - GfxclkFreq1 <-> GfxclkOffsetVolt1
> > > > + * - GfxclkFreq2 <-> GfxclkOffsetVolt2
> > > > + * - GfxclkFreq3 <-> GfxclkOffsetVolt3
> > > > + *
> > > > + * - Uclk
> > > > + * The max memory clock can be specified by UclkFmax(in Mhz).
> > > > + *
> > > > + * - Power limit
> > > > + * The power limit can be increased or descreased by
> > > > + * OverDrivePct(in percent).
> > >
> > > We already expose the power limit via hwmon. Do we still need it here?
> > >
> > > > + *
> > > > + * - Fan spped
> > >
> > > spped -> speed
> > >
> > > > + * The max fan speed can be set by FanMaxinumRpm and the
> > >
> > > FanMaxinumRpm -> FanMaximumRpm
> > >
> > > > + * min fan speed by FanMinimumPwm. Also zero rpm enablement
> > > > + * is specified by FanZeroRpmEnable.
> > > > + *
> > > > + * - Temperature
> > > > + * The fan target temperature can be set by FanTargetTemperature
> > > > + * and max Tj temperature by MaxOpTemp.
> > >
> > > The fan and temp stuff might be a better fit for the hwmon interfaces:
> > > https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface
> > >
> > > > + *
> > > > + */
> > > > +
> > > > +static ssize_t amdgpu_get_pp_od8_settings(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + char *buf)
> > > > +{
> > > > + struct drm_device *ddev = dev_get_drvdata(dev);
> > > > + struct amdgpu_device *adev = ddev->dev_private;
> > > > +
> > > > + if (adev->powerplay.pp_funcs->get_od8_settings)
> > > > + return amdgpu_dpm_get_od8_settings(adev, buf);
> > > > +
> > > > + return snprintf(buf, PAGE_SIZE, "\n"); }
> > > > +
> > > > +static ssize_t amdgpu_set_pp_od8_settings(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + const char *buf,
> > > > + size_t count)
> > > > +{
> > > > + struct drm_device *ddev = dev_get_drvdata(dev);
> > > > + struct amdgpu_device *adev = ddev->dev_private;
> > > > + char *sub_str, *tmp_str, buf_cpy[128];
> > > > + const char delimiter[3] = {' ', '\n', '\0'};
> > > > + uint32_t parameter_size = 0;
> > > > + long parameter[64];
> > > > + int ret = 0xff;
> > > > +
> > > > + if (count >= sizeof(buf_cpy) / sizeof(buf_cpy[0]))
> > > > + return -EINVAL;
> > > > +
> > > > + memcpy(buf_cpy, buf, count);
> > > > + buf_cpy[count] = '\0';
> > > > +
> > > > + tmp_str = buf_cpy;
> > > > +
> > > > + while (isspace(*tmp_str)) tmp_str++;
> > > > +
> > > > + while (tmp_str[0]) {
> > > > + sub_str = strsep(&tmp_str, delimiter);
> > > > + ret = kstrtol(sub_str, 0, ¶meter[parameter_size]);
> > > > + if (ret)
> > > > + return -EINVAL;
> > > > + parameter_size++;
> > > > +
> > > > + while (isspace(*tmp_str))
> > > > + tmp_str++;
> > > > + }
> > > > +
> > > > + if (adev->powerplay.pp_funcs->set_od8_settings)
> > > > + ret = amdgpu_dpm_set_od8_settings(adev, parameter,
> > > > + parameter_size);
> > > > +
> > > > + if (ret)
> > > > + return -EINVAL;
> > > > +
> > > > + return count;
> > > > +}
> > > > +
> > > > static DEVICE_ATTR(power_dpm_state, S_IRUGO | S_IWUSR,
> > > > amdgpu_get_dpm_state, amdgpu_set_dpm_state); static
> > > DEVICE_ATTR(power_dpm_force_performance_level, S_IRUGO | S_IWUSR,
> > > > amdgpu_get_dpm_forced_performance_level,
> > > > @@ -969,6 +1084,9 @@ static DEVICE_ATTR(pp_od_clk_voltage, S_IRUGO
> > > | S_IWUSR,
> > > > amdgpu_set_pp_od_clk_voltage); static
> > > > DEVICE_ATTR(gpu_busy_percent, S_IRUGO,
> > > > amdgpu_get_busy_percent, NULL);
> > > > +static DEVICE_ATTR(pp_od8_settings, S_IRUGO | S_IWUSR,
> > > > + amdgpu_get_pp_od8_settings,
> > > > + amdgpu_set_pp_od8_settings);
> > >
> > > Rather than adding a new file, would it make more sense to expose this new
> > > API on the same file we use for older asics? Otherwise, every time the
> > > interface changes, we'll need to add a new file. We already handle asic
> > > specific differences in the power profile sysfs files.
> > >
> > > >
> > > > static ssize_t amdgpu_hwmon_show_temp(struct device *dev,
> > > > struct device_attribute *attr,
> > > > @@ -1879,6 +1997,13 @@ int amdgpu_pm_sysfs_init(struct
> > > amdgpu_device *adev)
> > > > "gpu_busy_level\n");
> > > > return ret;
> > > > }
> > > > + ret = device_create_file(adev->dev,
> > > > + &dev_attr_pp_od8_settings);
> > > > + if (ret) {
> > > > + DRM_ERROR("failed to create device file "
> > > > + "pp_od8_settings\n");
> > > > + return ret;
> > > > + }
> > > > ret = amdgpu_debugfs_pm_init(adev);
> > > > if (ret) {
> > > > DRM_ERROR("Failed to register debugfs file for
> > > > dpm!\n"); diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > > > b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > > > index 6a41b81c7325..e23746ba53bf 100644
> > > > --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > > > +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > > > @@ -269,6 +269,8 @@ struct amd_pm_funcs {
> > > > int (*get_display_mode_validation_clocks)(void *handle,
> > > > struct amd_pp_simple_clock_info *clocks);
> > > > int (*notify_smu_enable_pwe)(void *handle);
> > > > + int (*get_od8_settings)(void *handle, char *buf);
> > > > + int (*set_od8_settings)(void *handle, long *input, uint32_t
> > > > + size);
> > > > };
> > > >
> > > > #endif
> > > > diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > > > b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > > > index da4ebff5b74d..4bcd06306698 100644
> > > > --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > > > +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > > > @@ -883,6 +883,41 @@ static int pp_odn_edit_dpm_table(void *handle,
> > > uint32_t type, long *input, uint3
> > > > return hwmgr->hwmgr_func->odn_edit_dpm_table(hwmgr, type,
> > > > input, size); }
> > > >
> > > > +static int pp_get_od8_settings(void *handle, char *buf) {
> > > > + struct pp_hwmgr *hwmgr = handle;
> > > > +
> > > > + if (!hwmgr || !hwmgr->pm_en || !buf)
> > > > + return -EINVAL;
> > > > +
> > > > + if (hwmgr->hwmgr_func->get_od8_settings == NULL) {
> > > > + pr_info("%s was not implemented.\n", __func__);
> > > > + return snprintf(buf, PAGE_SIZE, "\n");
> > > > + }
> > > > +
> > > > + return hwmgr->hwmgr_func->get_od8_settings(hwmgr, buf); }
> > > > +
> > > > +static int pp_set_od8_settings(void *handle, long *input, uint32_t
> > > > +size) {
> > > > + struct pp_hwmgr *hwmgr = handle;
> > > > + int ret = -EINVAL;
> > > > +
> > > > + if (!hwmgr || !hwmgr->pm_en)
> > > > + return ret;
> > > > +
> > > > + if (hwmgr->hwmgr_func->set_od8_settings == NULL) {
> > > > + pr_info("%s was not implemented.\n", __func__);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + mutex_lock(&hwmgr->smu_lock);
> > > > + ret = hwmgr->hwmgr_func->set_od8_settings(hwmgr, input, size);
> > > > + mutex_unlock(&hwmgr->smu_lock);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > static int pp_dpm_switch_power_profile(void *handle,
> > > > enum PP_SMC_POWER_PROFILE type, bool en) { @@ -1272,6
> > > > +1307,8 @@ static const struct amd_pm_funcs pp_dpm_funcs = {
> > > > .get_power_profile_mode = pp_get_power_profile_mode,
> > > > .set_power_profile_mode = pp_set_power_profile_mode,
> > > > .odn_edit_dpm_table = pp_odn_edit_dpm_table,
> > > > + .get_od8_settings = pp_get_od8_settings,
> > > > + .set_od8_settings = pp_set_od8_settings,
> > > > .set_power_limit = pp_set_power_limit,
> > > > .get_power_limit = pp_get_power_limit,
> > > > /* export to DC */
> > > > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c
> > > > b/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c
> > > > index fb32b28afa66..ececa2f7fe5f 100644
> > > > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c
> > > > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c
> > > > @@ -1099,7 +1099,188 @@ static int
> > > vega20_od8_initialize_default_settings(
> > > > return 0;
> > > > }
> > > >
> > > > -static int vega20_od8_set_settings(
> > > > +static int vega20_get_od8_settings(
> > > > + struct pp_hwmgr *hwmgr,
> > > > + char *buf)
> > > > +{
> > > > + struct vega20_hwmgr *data =
> > > > + (struct vega20_hwmgr *)(hwmgr->backend);
> > > > + struct vega20_od8_single_setting *od8_settings =
> > > > + data->od8_settings.od8_settings_array;
> > > > + OverDriveTable_t od_table;
> > > > + uint32_t size = 0;
> > > > + int ret = 0;
> > > > +
> > > > + if (!buf)
> > > > + return -EINVAL;
> > > > +
> > > > + ret = vega20_copy_table_from_smc(hwmgr,
> > > > + (uint8_t *)(&od_table),
> > > > + TABLE_OVERDRIVE);
> > > > + PP_ASSERT_WITH_CODE(!ret,
> > > > + "Failed to export over drive table!",
> > > > + return ret);
> > > > +
> > > > + if (od8_settings[OD8_SETTING_GFXCLK_FMIN].feature_id)
> > > > + size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "GfxclkFmin",
> > > > + od8_settings[OD8_SETTING_GFXCLK_FMIN].min_value,
> > > > + od8_settings[OD8_SETTING_GFXCLK_FMIN].max_value,
> > > > + od_table.GfxclkFmin);
> > > > +
> > > > + if (od8_settings[OD8_SETTING_GFXCLK_FMAX].feature_id)
> > > > + size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "GfxclkFmax",
> > > > + od8_settings[OD8_SETTING_GFXCLK_FMAX].min_value,
> > > > + od8_settings[OD8_SETTING_GFXCLK_FMAX].max_value,
> > > > + od_table.GfxclkFmax);
> > > > +
> > > > + if (od8_settings[OD8_SETTING_GFXCLK_FREQ1].feature_id)
> > > > + size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "GfxclkFreq1",
> > > > + od8_settings[OD8_SETTING_GFXCLK_FREQ1].min_value,
> > > > + od8_settings[OD8_SETTING_GFXCLK_FREQ1].max_value,
> > > > + od_table.GfxclkFreq1);
> > > > +
> > > > + if (od8_settings[OD8_SETTING_GFXCLK_VOLTAGE1].feature_id)
> > > > + size += sprintf(buf + size, "%22s[%d - %d]: %u\n",
> > > "GfxclkOffsetVolt1",
> > > > +
> > > od8_settings[OD8_SETTING_GFXCLK_VOLTAGE1].min_value,
> > > > +
> > > od8_settings[OD8_SETTING_GFXCLK_VOLTAGE1].max_value,
> > > > + od_table.GfxclkOffsetVolt1);
> > > > +
> > > > + if (od8_settings[OD8_SETTING_GFXCLK_FREQ2].feature_id)
> > > > + size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "GfxclkFreq2",
> > > > + od8_settings[OD8_SETTING_GFXCLK_FREQ2].min_value,
> > > > + od8_settings[OD8_SETTING_GFXCLK_FREQ2].max_value,
> > > > + od_table.GfxclkFreq2);
> > > > +
> > > > + if (od8_settings[OD8_SETTING_GFXCLK_VOLTAGE2].feature_id)
> > > > + size += sprintf(buf + size, "%22s[%d - %d]: %u\n",
> > > "GfxclkOffsetVolt2",
> > > > +
> > > od8_settings[OD8_SETTING_GFXCLK_VOLTAGE2].min_value,
> > > > +
> > > od8_settings[OD8_SETTING_GFXCLK_VOLTAGE2].max_value,
> > > > + od_table.GfxclkOffsetVolt2);
> > > > +
> > > > + if (od8_settings[OD8_SETTING_GFXCLK_FREQ3].feature_id)
> > > > + size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "GfxclkFreq3",
> > > > + od8_settings[OD8_SETTING_GFXCLK_FREQ3].min_value,
> > > > + od8_settings[OD8_SETTING_GFXCLK_FREQ3].max_value,
> > > > + od_table.GfxclkFreq3);
> > > > +
> > > > + if (od8_settings[OD8_SETTING_GFXCLK_VOLTAGE3].feature_id)
> > > > + size += sprintf(buf + size, "%22s[%d - %d]: %u\n",
> > > "GfxclkOffsetVolt3",
> > > > +
> > > od8_settings[OD8_SETTING_GFXCLK_VOLTAGE3].min_value,
> > > > +
> > > od8_settings[OD8_SETTING_GFXCLK_VOLTAGE3].max_value,
> > > > + od_table.GfxclkOffsetVolt3);
> > > > +
> > > > + if (od8_settings[OD8_SETTING_UCLK_FMAX].feature_id)
> > > > + size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "UclkFmax",
> > > > + od8_settings[OD8_SETTING_UCLK_FMAX].min_value,
> > > > + od8_settings[OD8_SETTING_UCLK_FMAX].max_value,
> > > > + od_table.UclkFmax);
> > > > +
> > > > + if (od8_settings[OD8_SETTING_POWER_PERCENTAGE].feature_id)
> > > > + size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "OverDrivePct",
> > > > +
> > > od8_settings[OD8_SETTING_POWER_PERCENTAGE].min_value,
> > > > +
> > > od8_settings[OD8_SETTING_POWER_PERCENTAGE].max_value,
> > > > + od_table.OverDrivePct);
> > > > +
> > > > + if (od8_settings[OD8_SETTING_FAN_ACOUSTIC_LIMIT].feature_id)
> > > > + size += sprintf(buf + size, "%22s[%d - %d]: %u\n",
> > > "FanMaximumRpm",
> > > > +
> > > od8_settings[OD8_SETTING_FAN_ACOUSTIC_LIMIT].min_value,
> > > > +
> > > od8_settings[OD8_SETTING_FAN_ACOUSTIC_LIMIT].max_value,
> > > > + od_table.FanMaximumRpm);
> > > > +
> > > > + if (od8_settings[OD8_SETTING_FAN_MIN_SPEED].feature_id)
> > > > + size += sprintf(buf + size, "%22s[%d - %d]: %u\n",
> > > "FanMinimumPwm",
> > > > +
> > > od8_settings[OD8_SETTING_FAN_MIN_SPEED].min_value,
> > > > +
> > > od8_settings[OD8_SETTING_FAN_MIN_SPEED].max_value,
> > > > + od_table.FanMinimumPwm);
> > > > +
> > > > + if (od8_settings[OD8_SETTING_FAN_TARGET_TEMP].feature_id)
> > > > + size += sprintf(buf + size, "%22s[%d - %d]: %u\n",
> > > "FanTargetTemperautre",
> > > > +
> > > od8_settings[OD8_SETTING_FAN_TARGET_TEMP].min_value,
> > > > +
> > > od8_settings[OD8_SETTING_FAN_TARGET_TEMP].max_value,
> > > > + od_table.FanTargetTemperature);
> > > > +
> > > > + if (od8_settings[OD8_SETTING_OPERATING_TEMP_MAX].feature_id)
> > > > + size += sprintf(buf + size, "%22s[%d - %d]: %u\n", "MaxOpTemp",
> > > > +
> > > od8_settings[OD8_SETTING_OPERATING_TEMP_MAX].min_value,
> > > > +
> > > od8_settings[OD8_SETTING_OPERATING_TEMP_MAX].max_value,
> > > > + od_table.MaxOpTemp);
> > > > +
> > > > + if (od8_settings[OD8_SETTING_FAN_ZERO_RPM_CONTROL].feature_id)
> > > > + size += sprintf(buf + size, "%22s[%d - %d]: %u\n",
> > > "FanZeroRpmEnable",
> > > > +
> > > od8_settings[OD8_SETTING_FAN_ZERO_RPM_CONTROL].min_value,
> > > > +
> > > od8_settings[OD8_SETTING_FAN_ZERO_RPM_CONTROL].max_value,
> > > > + od_table.FanZeroRpmEnable);
> > > > +
> > > > + return size;
> > > > +}
> > > > +
> > > > +static int vega20_set_od8_settings(
> > > > + struct pp_hwmgr *hwmgr,
> > > > + long *input,
> > > > + uint32_t size)
> > > > +{
> > > > + struct vega20_hwmgr *data =
> > > > + (struct vega20_hwmgr *)(hwmgr->backend);
> > > > + struct vega20_od8_single_setting *od8_settings =
> > > > + data->od8_settings.od8_settings_array;
> > > > + OverDriveTable_t od_table;
> > > > + uint16_t *buf = (uint16_t *)(&od_table);
> > > > + uint32_t i, j = 0, k;
> > > > + int ret = 0;
> > > > +
> > > > + /* retrieve current overdrive settings */
> > > > + ret = vega20_copy_table_from_smc(hwmgr,
> > > > + (uint8_t *)(&od_table),
> > > > + TABLE_OVERDRIVE);
> > > > + PP_ASSERT_WITH_CODE(!ret,
> > > > + "Failed to export over drive table!",
> > > > + return ret);
> > > > +
> > > > + for (i = 0;
> > > > + i < (sizeof(OverDriveTable_t) / sizeof(uint16_t)) - 1;
> > > > + i++) {
> > > > + /* overdrive table supports no dram timing setting */
> > > > + k = (i == OD8_SETTING_AC_TIMING) ?
> > > > + OD8_SETTING_FAN_ZERO_RPM_CONTROL : i;
> > > > + /*
> > > > + * Not all OD settings are supported and exported
> > > > + * to user.
> > > > + * So, updated only those which are supported and
> > > > + * exported to user.
> > > > + */
> > > > + if (od8_settings[k].feature_id) {
> > > > + if (input[j] < od8_settings[k].min_value ||
> > > > + input[j] > od8_settings[k].max_value)
> > > > + return -EINVAL;
> > > > +
> > > > + buf[i] = input[j];
> > > > + if (j++ >= size - 1)
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + /* enable the new overdrive settings */
> > > > + ret = vega20_copy_table_to_smc(hwmgr,
> > > > + (uint8_t *)(&od_table),
> > > > + TABLE_OVERDRIVE);
> > > > + PP_ASSERT_WITH_CODE(!ret,
> > > > + "Failed to import over drive table!",
> > > > + return ret);
> > > > +
> > > > + /* update the current_value of overdrive settings */
> > > > + for (i = 0;
> > > > + i < (sizeof(OverDriveTable_t) / sizeof(uint16_t)) - 1;
> > > > + i++) {
> > > > + k = (i == OD8_SETTING_AC_TIMING) ?
> > > > + OD8_SETTING_FAN_ZERO_RPM_CONTROL : i;
> > > > + if (od8_settings[k].feature_id)
> > > > + od8_settings[k].current_value = buf[i];
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int vega20_od8_set_single_setting(
> > > > struct pp_hwmgr *hwmgr,
> > > > uint32_t index,
> > > > uint32_t value)
> > > > @@ -1198,7 +1379,7 @@ static int vega20_set_sclk_od(
> > > > do_div(od_sclk, 100);
> > > > od_sclk +=
> > > > golden_sclk_table->dpm_levels[golden_sclk_table->count - 1].value;
> > > >
> > > > - ret = vega20_od8_set_settings(hwmgr, OD8_SETTING_GFXCLK_FMAX,
> > > od_sclk);
> > > > + ret = vega20_od8_set_single_setting(hwmgr,
> > > > + OD8_SETTING_GFXCLK_FMAX, od_sclk);
> > > > PP_ASSERT_WITH_CODE(!ret,
> > > > "[SetSclkOD] failed to set od gfxclk!",
> > > > return ret);
> > > > @@ -1245,7 +1426,7 @@ static int vega20_set_mclk_od(
> > > > do_div(od_mclk, 100);
> > > > od_mclk +=
> > > > golden_mclk_table->dpm_levels[golden_mclk_table->count - 1].value;
> > > >
> > > > - ret = vega20_od8_set_settings(hwmgr, OD8_SETTING_UCLK_FMAX,
> > > od_mclk);
> > > > + ret = vega20_od8_set_single_setting(hwmgr,
> > > > + OD8_SETTING_UCLK_FMAX, od_mclk);
> > > > PP_ASSERT_WITH_CODE(!ret,
> > > > "[SetMclkOD] failed to set od memclk!",
> > > > return ret);
> > > > @@ -2966,6 +3147,10 @@ static const struct pp_hwmgr_func
> > > vega20_hwmgr_funcs = {
> > > > vega20_get_power_profile_mode,
> > > > .set_power_profile_mode =
> > > > vega20_set_power_profile_mode,
> > > > + .get_od8_settings =
> > > > + vega20_get_od8_settings,
> > > > + .set_od8_settings =
> > > > + vega20_set_od8_settings,
> > > > /* od related */
> > > > .set_power_limit =
> > > > vega20_set_power_limit, diff --git
> > > > a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> > > > b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> > > > index a6d92128b19c..285af1728e2e 100644
> > > > --- a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> > > > +++ b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> > > > @@ -328,6 +328,8 @@ struct pp_hwmgr_func {
> > > > int (*set_power_limit)(struct pp_hwmgr *hwmgr, uint32_t n);
> > > > int (*powergate_mmhub)(struct pp_hwmgr *hwmgr);
> > > > int (*smus_notify_pwe)(struct pp_hwmgr *hwmgr);
> > > > + int (*get_od8_settings)(struct pp_hwmgr *hwmgr, char *buf);
> > > > + int (*set_od8_settings)(struct pp_hwmgr *hwmgr, long *input,
> > > > + uint32_t size);
> >
> > Those new added interfaces for OD parameters get/set seems have no difference with the old interface.
> >
> > int (*print_clock_levels)(struct pp_hwmgr *hwmgr, enum pp_clock_type type, char *buf);
> > int (*odn_edit_dpm_table)(struct pp_hwmgr *hwmgr, enum PP_OD_DPM_TABLE_COMMAND type, long *input, uint32_t size);
> >
> > Regards
> > Rex
> > > > };
> > > >
> > > > struct pp_table_func {
> > > > --
> > > > 2.18.0
> > > >
> > > > _______________________________________________
> > > > amd-gfx mailing list
> > > > amd-gfx at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20180829/22051863/attachment-0001.html>
