Thanks Paul,
Comment inline
> -----Original Message-----
> From: Paul Menzel <pmenzel+amd-gfx at molgen.mpg.de>
> Sent: 2018å¹´8æ??29æ?¥ 20:23
> To: Quan, Evan <Evan.Quan at amd.com>
> Cc: amd-gfx at lists.freedesktop.org; Alex Deucher
> <alexdeucher at gmail.com>; Zhu, Rex <Rex.Zhu at amd.com>
> Subject: Re: [PATCH] drm/amd/powerplay: added vega20 overdrive support
>
> Dear Evan,
>
>
> On 08/29/18 11:12, Evan Quan wrote:
> > Vega20 supports only sclk voltage overdrive. And user can
> > only tell three groups of <frequency, voltage_offset>. SMU
>
> Do you mean: The user can only configure three groups of â?¦?
>
[Quan, Evan] Yes.
> > firmware will recalculate the frequency/voltage curve. Other
> > intermediate levles will be stretched/shrunk accordingly.
>
> levels
>
> (A spell checker should detect simple typos.)
>
[Quan, Evan] Thanks.
> > Change-Id: I403cb38a95863db664cf06d030ac42a19bff6b33
> > Signed-off-by: Evan Quan <evan.quan at amd.com>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 26 +++
> > .../drm/amd/powerplay/hwmgr/vega20_hwmgr.c | 165
> +++++++++++++++++-
> > .../drm/amd/powerplay/hwmgr/vega20_hwmgr.h | 2 +
> > 3 files changed, 192 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > index e2577518b9c6..6e0c8f583521 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > @@ -474,6 +474,8 @@ static ssize_t amdgpu_set_pp_table(struct device
> *dev,
> > * in each power level within a power state. The pp_od_clk_voltage is
> used for
> > * this.
> > *
> > + * < For Vega10 and previous ASICs >
> > + *
> > * Reading the file will display:
> > *
> > * - a list of engine clock levels and voltages labeled OD_SCLK
> > @@ -491,6 +493,30 @@ static ssize_t amdgpu_set_pp_table(struct device
> *dev,
> > * "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.
> > *
> > + *
> > + * < For Vega20 >
> > + *
> > + * Reading the file will display:
> > + *
> > + * - three groups of engine clock and voltage offset labeled OD_SCLK
> > + *
> > + * - a list of valid ranges for above three groups of sclk and voltage offset
> > + * labeled OD_RANGE
> > + *
> > + * To manually adjust these settings:
> > + *
> > + * - First select manual using power_dpm_force_performance_level
> > + *
> > + * - Enter a new value for each group by writing a string that contains
> > + * "s group_index clock voltage_offset" to the file. E.g., "s 0 500 20"
> > + * will update group1 sclk to be 500 MHz with voltage increased 20mV
>
> increased *by* 20mV? by or to?
>
[Quan, Evan] It should be 'by' since here is an offset.
> > + *
> > + * - 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
> > + *
> > */
> >
> > static ssize_t amdgpu_set_pp_od_clk_voltage(struct device *dev,
> > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c
> > index ececa2f7fe5f..9f6e070a76e0 100644
> > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c
> > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c
> > @@ -1096,6 +1096,13 @@ static int
> vega20_od8_initialize_default_settings(
> > }
> > }
> >
> > + ret = vega20_copy_table_from_smc(hwmgr,
> > + (uint8_t *)(&data->od_table),
> > + TABLE_OVERDRIVE);
> > + PP_ASSERT_WITH_CODE(!ret,
> > + "Failed to export over drive table!",
>
> *over drive* or *overdrive*
[Quan, Evan] Should be 'overdrive'. Will fix it.
>
> > + return ret);
> > +
> > return 0;
> > }
> >
> > @@ -2506,11 +2513,112 @@ static int
> vega20_set_watermarks_for_clocks_ranges(struct pp_hwmgr *hwmgr,
> > return 0;
> > }
> >
> > +static int vega20_odn_edit_dpm_table(struct pp_hwmgr *hwmgr,
> > + enum
> PP_OD_DPM_TABLE_COMMAND type,
> > + long *input, uint32_t size)
>
> What is size? Could it be `size_t` or just int?
[Quan, Evan] Number of elements in the input array.
>
> > +{
> > + 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;
>
> Is CamelCase wanted?
>
> > + int32_t input_index, input_clk, input_vol, i;
> > + int ret = 0;
>
> Is the initialization needed?
>
[Quan, Evan] Fine to have it uninitialized here since it will be initialized before use.
> > +
> > + PP_ASSERT_WITH_CODE(input, "NULL user input for clock and
> voltage",
> > + return -EINVAL);
> > +
> > + switch (type) {
> > + case PP_OD_EDIT_SCLK_VDDC_TABLE:
> > + if (!(od8_settings[OD8_SETTING_GFXCLK_FREQ1].feature_id
> &&
> > + od8_settings[OD8_SETTING_GFXCLK_FREQ2].feature_id
> &&
> > + od8_settings[OD8_SETTING_GFXCLK_FREQ3].feature_id
> &&
> > +
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE1].feature_id &&
> > +
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE2].feature_id &&
> > +
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE3].feature_id)) {
> > + pr_info("Sclk voltage overdrive not supported\n");
> > + return 0;
> > + }
> > +
> > + for (i = 0; i < size; i += 3) {
> > + if (i + 3 > size) {
> > + pr_info("invalid clock voltage input\n");
>
> As this is log level *Info* Iâ??d say it should be better understandable for
> the user. Also, why not print the values?
>
[Quan, Evan] Thanks. Let me refine the error output to be more friendly.
> > + return 0;
> > + }
> > +
> > + input_index = input[i];
> > + input_clk = input[i + 1];
> > + input_vol = input[i + 2];
> > + if (input_index > 2 ||
> > + input_clk <
> od8_settings[OD8_SETTING_GFXCLK_FREQ1 + input_index * 2].min_value
> ||
> > + input_clk >
> od8_settings[OD8_SETTING_GFXCLK_FREQ1 + input_index * 2].max_value
> ||
> > + input_vol <
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE1 + input_index *
> 2].min_value ||
> > + input_vol >
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE1 + input_index *
> 2].max_value) {
> > + pr_info("input argument is not within
> allowed range\n");
> > + return -EINVAL;
>
> Ditto. Maybe even print the values?
>
> > + }
> > +
> > + switch (input_index) {
> > + case 0:
> > + data->od_table.GfxclkFreq1 = input_clk;
> > + data->od_table.GfxclkOffsetVolt1 =
> input_vol;
> > + break;
> > + case 1:
> > + data->od_table.GfxclkFreq2 = input_clk;
> > + data->od_table.GfxclkOffsetVolt2 =
> input_vol;
> > + break;
> > + case 2:
> > + data->od_table.GfxclkFreq3 = input_clk;
> > + data->od_table.GfxclkOffsetVolt3 =
> input_vol;
> > + break;
> > + }
> > + }
> > + break;
> > +
> > + case PP_OD_EDIT_MCLK_VDDC_TABLE:
> > + pr_info("Mclk voltage overdrive not supported!\n");
>
> Ditto.
>
> > + break;
> > +
> > + case PP_OD_RESTORE_DEFAULT_TABLE:
> > + 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);
> > +
> > + memcpy(&data->od_table, &od_table, sizeof(od_table));
> > + break;
> > +
> > + case PP_OD_COMMIT_DPM_TABLE:
> > + memcpy(&od_table, &data->od_table, sizeof(od_table));
> > +
> > + 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);
> > +
> > + break;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int vega20_print_clock_levels(struct pp_hwmgr *hwmgr,
> > enum pp_clock_type type, char *buf)
> > {
> > - int i, now, size = 0;
> > + 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 = &data->od_table;
> > struct pp_clock_levels_with_latency clocks;
> > + int i, now, size = 0;
> > int ret = 0;
> >
> > switch (type) {
> > @@ -2551,6 +2659,59 @@ static int vega20_print_clock_levels(struct
> pp_hwmgr *hwmgr,
> > case PP_PCIE:
> > break;
> >
> > + case OD_SCLK:
> > + if (od8_settings[OD8_SETTING_GFXCLK_FREQ1].feature_id
> &&
> > + od8_settings[OD8_SETTING_GFXCLK_FREQ2].feature_id
> &&
> > + od8_settings[OD8_SETTING_GFXCLK_FREQ3].feature_id
> &&
> > +
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE1].feature_id &&
> > +
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE2].feature_id &&
> > +
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE3].feature_id) {
> > +
> > + size = sprintf(buf, "%s:\n", "OD_SCLK");
> > + size += sprintf(buf + size, "0: %10uMhz %10dmV\n",
> > + od_table->GfxclkFreq1,
> > + od_table->GfxclkOffsetVolt1);
> > + size += sprintf(buf + size, "1: %10uMhz %10dmV\n",
> > + od_table->GfxclkFreq2,
> > + od_table->GfxclkOffsetVolt2);
> > + size += sprintf(buf + size, "2: %10uMhz %10dmV\n",
> > + od_table->GfxclkFreq3,
> > + od_table->GfxclkOffsetVolt3);
> > + }
> > + break;
> > +
> > + case OD_MCLK:
> > + break;
> > +
> > + case OD_RANGE:
> > + if (od8_settings[OD8_SETTING_GFXCLK_FREQ1].feature_id
> &&
> > + od8_settings[OD8_SETTING_GFXCLK_FREQ2].feature_id
> &&
> > + od8_settings[OD8_SETTING_GFXCLK_FREQ3].feature_id
> &&
> > +
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE1].feature_id &&
> > +
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE2].feature_id &&
> > +
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE3].feature_id) {
> > +
> > + size = sprintf(buf, "%s:\n", "OD_RANGE");
> > + size += sprintf(buf + size,
> "SCLK[0]: %7uMhz %10uMhz\n",
> > +
> od8_settings[OD8_SETTING_GFXCLK_FREQ1].min_value,
> > +
> od8_settings[OD8_SETTING_GFXCLK_FREQ1].max_value);
> > + size += sprintf(buf + size,
> "VDDC[0]: %7dmV %11dmV\n",
> > +
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE1].min_value,
> > +
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE1].max_value);
> > + size += sprintf(buf + size,
> "SCLK[1]: %7uMhz %10uMhz\n",
> > +
> od8_settings[OD8_SETTING_GFXCLK_FREQ2].min_value,
> > +
> od8_settings[OD8_SETTING_GFXCLK_FREQ2].max_value);
> > + size += sprintf(buf + size,
> "VDDC[1]: %7dmV %11dmV\n",
> > +
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE2].min_value,
> > +
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE2].max_value);
> > + size += sprintf(buf + size,
> "SCLK[2]: %7uMhz %10uMhz\n",
> > +
> od8_settings[OD8_SETTING_GFXCLK_FREQ3].min_value,
> > +
> od8_settings[OD8_SETTING_GFXCLK_FREQ3].max_value);
> > + size += sprintf(buf + size,
> "VDDC[2]: %7dmV %11dmV\n",
> > +
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE3].min_value,
> > +
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE3].max_value);
> > + }
> > + break;
> > default:
> > break;
> > }
> > @@ -3162,6 +3323,8 @@ static const struct pp_hwmgr_func
> vega20_hwmgr_funcs = {
> > vega20_get_mclk_od,
> > .set_mclk_od =
> > vega20_set_mclk_od,
> > + .odn_edit_dpm_table =
> > + vega20_odn_edit_dpm_table,
> > /* for sysfs to retrive/set gfxclk/memclk */
> > .force_clock_level =
> > vega20_force_clock_level,
> > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.h
> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.h
> > index 72e4f2a55641..8bb90e22efb6 100644
> > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.h
> > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.h
> > @@ -513,6 +513,8 @@ struct vega20_hwmgr {
> > /* ---- Gfxoff ---- */
> > bool gfxoff_allowed;
> > uint32_t counter_gfxoff;
> > +
> > + OverDriveTable_t od_table;
> > };
> >
> > #define VEGA20_DPM2_NEAR_TDP_DEC 10
>
>
> Kind regards,
>
> Paul
Regards,
Evan
