[PATCH] drm/amd/powerplay: added vega20 overdrive support

Paul Menzel pmenzel+amd-gfx at molgen.mpg.de
Wed Aug 29 12:23:18 UTC 2018


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 …?

> firmware will recalculate the frequency/voltage curve. Other
> intermediate levles will be stretched/shrunk accordingly.

levels

(A spell checker should detect simple typos.)

> 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?

> + *
> + * - 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*

> +			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?

> +{
> +	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?

> +
> +	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?

> +				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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5174 bytes
Desc: S/MIME Cryptographic Signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20180829/99424766/attachment.bin>


More information about the amd-gfx mailing list