Plans for v1.0.0

[FlyGoat]

I’m excited to share that our project has just hit its 5-year milestone! As part of our ongoing development, I'm planning to release v1.0.0 soon, which will include significant improvements to the codebase. I'd love to get your feedback on the following potential updates:

• Migration to the Meson build system: This would help us avoid the complexities and frustrations often encountered with CMake. (A prototype is already available on the meson branch.)
• Logging callbacks for libryzenadj: This aims to eliminate reliance on stdout/stderr in the library, providing a cleaner and more efficient logging mechanism.
• Extensible API for adding new parameters and PMTable entries: This is still in the design phase, so I'm eager to hear your thoughts and suggestions!

Please use this issue as a function wish list as well.

Thanks

[Erruar]

Hello, FlyGoat!

Congratulations on reaching the 5-year milestone for our project! I’m excited about the upcoming release of v1.0.0 and the potential improvements you've proposed. Below, I've structured my feedback on each area, along with some additional suggestions that could enhance functionality further.

Feedback on Proposed Updates

1. Migration to the Meson Build System

   • Switching to Meson would indeed help streamline the build process and could simplify maintenance compared to CMake. Given that you’ve already started a prototype on the meson branch, I think it would be worthwhile to proceed with this transition to reduce the build complexity.

2. Logging Callbacks for libryzenadj

   • Introducing logging callbacks is a solid approach to make logging more efficient and remove the reliance on stdout/stderr. This would provide cleaner integration, especially in embedded systems or applications where logging needs to be controlled more granularly.

3. Extensible API for New Parameters and PM Table Entries

   • An extensible API for adding parameters and Power Management (PM) Table entries sounds promising. I’d love to see more details on the design as it develops, as I have some specific API-related suggestions below that might align well with this direction.

Suggested Enhancements to libryzenadj

To build on the extensible API you mentioned, here are a few API functions and modifications that could enhance libryzenadj for broader use cases and improved functionality:

1. Enhanced SMU Command Support

   • Add a command function:

     void SendSMUCommand(uint mailbox[], uint command, uint arguments[]);

     • Parameters:
       • mailbox[]: Contains three uint values – CMD, ARG, RSP – for structured command data.
       • command: A single uint command to execute.
       • arguments[]: Array of six uint values for additional arguments.
     • This addition would provide more flexible SMU (System Management Unit) command handling and extend compatibility with other functions that might rely on SMU commands.

2. PM Table Retrieval Functions

   • Add functions for Power Management Table retrieval:

     float[] GetPMTable(RyzenAccess ryzenAccess);
     float GetPMTableIndex(uint tableIndex, RyzenAccess ryzenAccess);

     • Description: These functions would allow more direct access to specific PM Table data (by index or as an entire set) for better monitoring, including desktop CPUs.

3. PM Table Metadata Access

   • Add metadata retrieval functions:

     int GetPMTableSize(RyzenAccess ryzenAccess);
     uint GetPMTableVersion(RyzenAccess ryzenAccess);

     • Purpose: Provides access to PM Table size and version for compatibility checks and adjustments based on specific PM Table versions.

4. Validation Check for RyzenAccess

   • Implement a check to ensure ryzenAccess is not zero before initializing the PM Table.
     • Reason: A null or zero ryzenAccess value is currently causing DLL crashes. Adding this validation would improve stability and avoid runtime errors.

5. Unified Adjustment Command

   • Add a universal function to adjust parameters:

     int ryzenadj_set(const char *param, int value);

     • Details: This command would streamline the parameter adjustment process by accepting a single string argument (param) and a value. It simplifies code by reducing multiple adjustment calls to a single, generalized function.

Additional Suggestions

6. Broader Platform Compatibility
   • Enable Desktop Support: Given that many commands used on laptops are similar or identical for desktop Ryzen CPUs, extending ryzenadj functionality to support desktop CPUs could broaden our user base.
   • Structured Command Lists by CPU Generation: To improve usability, it would be helpful to organize and present command lists based on CPU generation. This way, only relevant commands for the detected CPU model would be shown, simplifying the user experience.

---

Let me know what you think of these suggestions, and feel free to reach out if you'd like any clarification. Looking forward to seeing how the new release shapes up!

Best regards,
Serzhik Sakurajima

[kylon]

was going to open an issue but then noticed this
i wanted a cleaner implementation in my app so i reworked the library https://github.com/kylon/RyzenAdj

you can take ideas or pr if you want
ryzen_access is no longer a thing because apps should not be able to mess with internal objs, just api functions
features query functions are needed for gui apps, no longer duplicating checks to enable settings in apps
includes cleanup and removing unneeded windows stuff makes build instant and lib should be smaller in windows (didnt check size before)
as much as possible is done at init once, so everything is faster

i updated main for a quick test before implementing in app, you can see how the new api looks like

main

// SPDX-License-Identifier: GPL-2.0
/* Copyright (C) 2018-2019 Jiaxun Yang <jiaxun.yang@flygoat.com> */
/* Ryzen NB SMU Service Request Tool */

#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#include "lib/ryzenadj.h"
#include "argparse.h"

#define STRINGIFY2(X) #X
#define STRINGIFY(X) STRINGIFY2(X)

static const char *const usage[] = {
	"ryzenadj [options]",
	NULL,
};

static const char *family_name(const RYZEN_FAMILY fam)
{
	switch (fam)
	{
	case FAM_RAVEN: return "Raven";
	case FAM_PICASSO: return "Picasso";
	case FAM_RENOIR: return "Renoir";
	case FAM_CEZANNE: return "Cezanne";
	case FAM_DALI: return "Dali";
	case FAM_LUCIENNE: return "Lucienne";
	case FAM_VANGOGH: return "Vangogh";
	case FAM_REMBRANDT: return "Rembrandt";
	case FAM_PHOENIX: return "Phoenix Point";
	case FAM_HAWKPOINT: return "Hawk Point";
	case FAM_STRIXPOINT: return "Strix Point";
	default:
		break;
	}

	return "Unknown";
}

static void show_info_header()
{
	printf("CPU Family: %s\n", family_name(ryzenadj_get_cpu_family()));
	printf("SMU BIOS Interface Version: %d\n", ryzenadj_get_bios_if_ver());
	printf("Version: v" STRINGIFY(RYZENADJ_REVISION_VER) "." STRINGIFY(RYZENADJ_MAJOR_VER) "." STRINGIFY(RYZENADJ_MINIOR_VER) " \n");
}

static void show_info_table()
{
	printf("PM Table Version: %x\n", ryzenadj_get_table_ver());

	//get refresh table after adjust
	const ADJ_ERROR err = ryzenadj_refresh_table();
	if(err != ADJ_OK){
		printf("Unable to refresh power metric table: %d\n", err);
		return;
	}

	//print table in github markdown
	printf("|        Name         |   Value   |     Parameter      |\n");
	printf("|---------------------|-----------|--------------------|\n");
	char tableFormat[] = "| %-19s | %9.3lf | %-18s |\n";
	printf(tableFormat, "STAPM LIMIT", ryzenadj_get(ADJ_OPT_STAPM_LIMIT, NULL), "stapm-limit");
	printf(tableFormat, "STAPM VALUE", ryzenadj_get_value(ADJ_OPT_STAPM_LIMIT, NULL), "");
	printf(tableFormat, "PPT LIMIT FAST", ryzenadj_get(ADJ_OPT_FAST_LIMIT, NULL), "fast-limit");
	printf(tableFormat, "PPT VALUE FAST", ryzenadj_get_value(ADJ_OPT_FAST_LIMIT, NULL), "");
	printf(tableFormat, "PPT LIMIT SLOW", ryzenadj_get(ADJ_OPT_SLOW_LIMIT, NULL), "slow-limit");
	printf(tableFormat, "PPT VALUE SLOW", ryzenadj_get_value(ADJ_OPT_SLOW_LIMIT, NULL), "");
	printf(tableFormat, "StapmTimeConst", ryzenadj_get(ADJ_OPT_STAPM_TIME, NULL), "stapm-time");
	printf(tableFormat, "SlowPPTTimeConst", ryzenadj_get(ADJ_OPT_SLOW_TIME, NULL), "slow-time");
	printf(tableFormat, "PPT LIMIT APU", ryzenadj_get(ADJ_OPT_APU_SLOW_LIMIT, NULL), "apu-slow-limit");
	printf(tableFormat, "PPT VALUE APU", ryzenadj_get_value(ADJ_OPT_APU_SLOW_LIMIT, NULL), "");
	printf(tableFormat, "TDC LIMIT VDD", ryzenadj_get(ADJ_OPT_VRM_CURRENT, NULL), "vrm-current");
	printf(tableFormat, "TDC VALUE VDD", ryzenadj_get_value(ADJ_OPT_VRM_CURRENT, NULL), "");
	printf(tableFormat, "TDC LIMIT SOC", ryzenadj_get(ADJ_OPT_VRMSOC_CURRENT, NULL), "vrmsoc-current");
	printf(tableFormat, "TDC VALUE SOC", ryzenadj_get_value(ADJ_OPT_VRMSOC_CURRENT, NULL), "");
	printf(tableFormat, "EDC LIMIT VDD", ryzenadj_get(ADJ_OPT_VRMMAX_CURRENT, NULL), "vrmmax-current");
	printf(tableFormat, "EDC VALUE VDD", ryzenadj_get_value(ADJ_OPT_VRMMAX_CURRENT, NULL), "");
	printf(tableFormat, "EDC LIMIT SOC", ryzenadj_get(ADJ_OPT_VRMSOCMAX_CURRENT, NULL), "vrmsocmax-current");
	printf(tableFormat, "EDC VALUE SOC", ryzenadj_get_value(ADJ_OPT_VRMSOCMAX_CURRENT, NULL), "");
	printf(tableFormat, "THM LIMIT CORE", ryzenadj_get(ADJ_OPT_TCTL_TEMP, NULL), "tctl-temp");
	printf(tableFormat, "THM VALUE CORE", ryzenadj_get_value(ADJ_OPT_TCTL_TEMP, NULL), "");
	printf(tableFormat, "STT LIMIT APU", ryzenadj_get(ADJ_OPT_APU_SKIN_TEMP_LIMIT, NULL), "apu-skin-temp");
	printf(tableFormat, "STT VALUE APU", ryzenadj_get_value(ADJ_OPT_APU_SKIN_TEMP_LIMIT, NULL), "");
	printf(tableFormat, "STT LIMIT dGPU", ryzenadj_get(ADJ_OPT_DGPU_SKIN_TEMP_LIMIT, NULL), "dgpu-skin-temp");
	printf(tableFormat, "STT VALUE dGPU", ryzenadj_get_value(ADJ_OPT_DGPU_SKIN_TEMP_LIMIT, NULL), "");
	printf(tableFormat, "CCLK Boost SETPOINT", ryzenadj_get_value(ADJ_OPT_CCLK_SETPOINT, NULL), "power-saving /");
	printf(tableFormat, "CCLK BUSY VALUE", ryzenadj_get_value(ADJ_OPT_CCLK_BUSY, NULL), "max-performance");
}

static void show_table_dump(const int any_adjust_applied)
{
	size_t index, table_size;
	uint32_t *table_data_copy;
	float *current_table_values, *old_table_values;

	printf("PM Table Dump of Version: %x\n", ryzenadj_get_table_ver());
	table_size = ryzenadj_get_table_size();

	current_table_values = ryzenadj_get_table_values();
	table_data_copy = malloc(table_size);
	memcpy(table_data_copy, current_table_values, table_size);

	if(any_adjust_applied)
	{
		//copy old values before refresh
		old_table_values = malloc(table_size);
		memcpy(old_table_values, table_data_copy, table_size);

		const ADJ_ERROR err = ryzenadj_refresh_table();
		if(err != ADJ_OK)
			printf("Unable to refresh power metric table: %d\n", err);

		//print table in github markdown
		printf("| Offset |    Data    |   Value   | After Adjust |\n");
		printf("|--------|------------|-----------|--------------|\n");
		char tableFormat[] = "| 0x%04X | 0x%08X | %9.3lf | %12.3lf |\n";
		for(index = 0; index < table_size / 4; index++)
			printf(tableFormat, index * 4, table_data_copy[index], old_table_values[index], current_table_values[index]);

		free(old_table_values);
	}
	else
	{
		//print table in github markdown
		printf("| Offset |    Data    |   Value   |\n");
		printf("|--------|------------|-----------|\n");
		char tableFormat[] = "| 0x%04X | 0x%08X | %9.3lf |\n";
		for(index = 0; index < table_size / 4; index++)
			printf(tableFormat, index * 4, table_data_copy[index], current_table_values[index]);
	}

	free(table_data_copy);
	//don't free current_table_values because this would deinitialize our table
}

static void setOpt(const ADJ_OPT opt, const uint32_t val, ADJ_ERROR *err) {
	if (val == -1)
		return;

	*err = ryzenadj_set(opt, val);
}

static void setOptEnable(const ADJ_OPT opt, const uint32_t val, ADJ_ERROR *err) {
	if (!val)
		return;

	*err = ryzenadj_set(opt, 0);
}

int main(int argc, const char **argv)
{
	int info = 0, dump_table = 0;
	int power_saving = 0, max_performance = 0, enable_oc = 0x0, disable_oc = 0x0;
	//init unsigned types with max value because we treat max value as unset
	uint32_t stapm_limit = -1, fast_limit = -1, slow_limit = -1, slow_time = -1, stapm_time = -1, tctl_temp = -1;
	uint32_t vrm_current = -1, vrmsoc_current = -1, vrmmax_current = -1, vrmsocmax_current = -1, psi0_current = -1, psi0soc_current = -1;
	uint32_t vrmgfx_current = -1, vrmcvip_current = -1, vrmgfxmax_current = -1, psi3cpu_current = -1, psi3gfx_current = -1;
	uint32_t max_socclk_freq = -1, min_socclk_freq = -1, max_fclk_freq = -1, min_fclk_freq = -1, max_vcn = -1, min_vcn = -1, max_lclk = -1, min_lclk = -1;
	uint32_t max_gfxclk_freq = -1, min_gfxclk_freq = -1, prochot_deassertion_ramp = -1, apu_skin_temp_limit = -1, dgpu_skin_temp_limit = -1, apu_slow_limit = -1;
	uint32_t skin_temp_power_limit = -1;
	uint32_t gfx_clk = -1, oc_clk = -1, oc_volt = -1, coall = -1, coper = -1, cogfx = -1;
	ADJ_ERROR adjerr;

	//create structure for parseing
	struct argparse_option options[] = {
		OPT_HELP(),
		OPT_GROUP("Options"),
		OPT_BOOLEAN('i', "info", &info, "Show information and most important power metrics after adjustment"),
		OPT_BOOLEAN('\0', "dump-table", &dump_table, "Show whole power metric table before and after adjustment"),
		OPT_GROUP("Settings"),
		OPT_U32('a', "stapm-limit", &stapm_limit, "Sustained Power Limit         - STAPM LIMIT (mW)"),
		OPT_U32('b', "fast-limit", &fast_limit, "Actual Power Limit            - PPT LIMIT FAST (mW)"),
		OPT_U32('c', "slow-limit", &slow_limit, "Average Power Limit           - PPT LIMIT SLOW (mW)"),
		OPT_U32('d', "slow-time", &slow_time, "Slow PPT Constant Time (s)"),
		OPT_U32('e', "stapm-time", &stapm_time, "STAPM constant time (s)"),
		OPT_U32('f', "tctl-temp", &tctl_temp, "Tctl Temperature Limit (degree C)"),
		OPT_U32('g', "vrm-current", &vrm_current, "VRM Current Limit             - TDC LIMIT VDD (mA)"),
		OPT_U32('j', "vrmsoc-current", &vrmsoc_current, "VRM SoC Current Limit         - TDC LIMIT SoC (mA)"),
		OPT_U32('\0', "vrmgfx-current", &vrmgfx_current, "VRM GFX Current Limit - TDC LIMIT GFX (mA)"),
		OPT_U32('\0', "vrmcvip-current", &vrmcvip_current, "VRM CVIP Current Limit - TDC LIMIT CVIP (mA)"),
		OPT_U32('k', "vrmmax-current", &vrmmax_current, "VRM Maximum Current Limit     - EDC LIMIT VDD (mA)"),
		OPT_U32('l', "vrmsocmax-current", &vrmsocmax_current, "VRM SoC Maximum Current Limit - EDC LIMIT SoC (mA)"),
		OPT_U32('\0', "vrmgfxmax_current", &vrmgfxmax_current, "VRM GFX Maximum Current Limit - EDC LIMIT GFX (mA)"),
		OPT_U32('m', "psi0-current", &psi0_current, "PSI0 VDD Current Limit (mA)"),
		OPT_U32('\0', "psi3cpu_current", &psi3cpu_current, "PSI3 CPU Current Limit (mA)"),
		OPT_U32('n', "psi0soc-current", &psi0soc_current, "PSI0 SoC Current Limit (mA)"),
		OPT_U32('\0', "psi3gfx_current", &psi3gfx_current, "PSI3 GFX Current Limit (mA)"),
		OPT_U32('o', "max-socclk-frequency", &max_socclk_freq, "Maximum SoC Clock Frequency (MHz)"),
		OPT_U32('p', "min-socclk-frequency", &min_socclk_freq, "Minimum SoC Clock Frequency (MHz)"),
		OPT_U32('q', "max-fclk-frequency", &max_fclk_freq, "Maximum Transmission (CPU-GPU) Frequency (MHz)"),
		OPT_U32('r', "min-fclk-frequency", &min_fclk_freq, "Minimum Transmission (CPU-GPU) Frequency (MHz)"),
		OPT_U32('s', "max-vcn", &max_vcn, "Maximum Video Core Next (VCE - Video Coding Engine) (MHz)"),
		OPT_U32('t', "min-vcn", &min_vcn, "Minimum Video Core Next (VCE - Video Coding Engine) (MHz)"),
		OPT_U32('u', "max-lclk", &max_lclk, "Maximum Data Launch Clock (MHz)"),
		OPT_U32('v', "min-lclk", &min_lclk, "Minimum Data Launch Clock (MHz)"),
		OPT_U32('w', "max-gfxclk", &max_gfxclk_freq, "Maximum GFX Clock (MHz)"),
		OPT_U32('x', "min-gfxclk", &min_gfxclk_freq, "Minimum GFX Clock (MHz)"),
		OPT_U32('y', "prochot-deassertion-ramp", &prochot_deassertion_ramp, "Ramp Time After Prochot is Deasserted: limit power based on value, higher values does apply tighter limits after prochot is over"),
		OPT_U32('\0', "apu-skin-temp", &apu_skin_temp_limit, "APU Skin Temperature Limit    - STT LIMIT APU (degree C)"),
		OPT_U32('\0', "dgpu-skin-temp", &dgpu_skin_temp_limit, "dGPU Skin Temperature Limit   - STT LIMIT dGPU (degree C)"),
		OPT_U32('\0', "apu-slow-limit", &apu_slow_limit, "APU PPT Slow Power limit for A+A dGPU platform - PPT LIMIT APU (mW)"),
		OPT_U32('\0', "skin-temp-limit", &skin_temp_power_limit, "Skin Temperature Power Limit (mW)"),
		OPT_U32('\0', "gfx-clk", &gfx_clk, "Forced Clock Speed MHz (Renoir Only)"),
		OPT_U32('\0', "oc-clk", &oc_clk, "Forced Core Clock Speed MHz (Renoir and up Only)"),
		OPT_U32('\0', "oc-volt", &oc_volt, "Forced Core VID: Must follow this calcuation (1.55 - [VID you want to set e.g. 1.25 for 1.25v]) / 0.00625 (Renoir and up Only)"),
		OPT_BOOLEAN('\0', "enable-oc", &enable_oc, "Enable OC (Renoir and up Only)"),
		OPT_BOOLEAN('\0', "disable-oc", &disable_oc, "Disable OC (Renoir and up Only)"),
		OPT_U32('\0', "set-coall", &coall, "All core Curve Optimiser"),
		OPT_U32('\0', "set-coper", &coper, "Per core Curve Optimiser"),
		OPT_U32('\0', "set-cogfx", &cogfx, "iGPU Curve Optimiser"),
		OPT_BOOLEAN('\0', "power-saving", &power_saving, "Hidden options to improve power efficiency (is set when AC unplugged): behavior depends on CPU generation, Device and Manufacture"),
		OPT_BOOLEAN('\0', "max-performance", &max_performance, "Hidden options to improve performance (is set when AC plugged in): behavior depends on CPU generation, Device and Manufacture"),
		OPT_GROUP("P-State Functions"),
		OPT_END(),
	};

	struct argparse argparse;
	argparse_init(&argparse, options, usage, ARGPARSE_NON_OPTION_IS_INVALID);
	argparse_describe(&argparse, "\n Ryzen Power Management adjust tool.", "\nWARNING: Use at your own risk!\nBy Jiaxun Yang <jiaxun.yang@flygoat.com>, Under LGPL.\nVersion: v" STRINGIFY(RYZENADJ_REVISION_VER) "." STRINGIFY(RYZENADJ_MAJOR_VER) "." STRINGIFY(RYZENADJ_MINIOR_VER));
	argc = argparse_parse(&argparse, argc, argv);

	//init RyzenAdj and validate that it was able to
	adjerr = ryzenadj_init();
	if(adjerr != ADJ_OK){
		printf("Unable to init ryzenadj\n");
		return -1;
	}

	//shows info header before init_table
	if (info)
		show_info_header();

	if (info || dump_table) {
		//init before adjustment to get the default values
		adjerr = ryzenadj_refresh_table();
		if (adjerr != ADJ_OK) {
			char *er = ryzenadj_error_str(adjerr);

			printf("Unable to init power metric table, this does not affect adjustments because it is only needed for monitoring.\n");
			printf("%s\n",er);
			free(er);
		}
	}

	//adjust all the arguments sent to RyzenAdj.exe
	setOpt(ADJ_OPT_STAPM_LIMIT, stapm_limit, &adjerr);
	setOpt(ADJ_OPT_FAST_LIMIT, fast_limit, &adjerr);
	setOpt(ADJ_OPT_SLOW_LIMIT, slow_limit, &adjerr);
	setOpt(ADJ_OPT_SLOW_TIME, slow_time, &adjerr);
	setOpt(ADJ_OPT_STAPM_TIME, stapm_time, &adjerr);
	setOpt(ADJ_OPT_TCTL_TEMP, tctl_temp, &adjerr);
	setOpt(ADJ_OPT_VRM_CURRENT, vrm_current, &adjerr);
	setOpt(ADJ_OPT_VRMSOC_CURRENT, vrmsoc_current, &adjerr);
	setOpt(ADJ_OPT_VRMGFX_CURRENT, vrmgfx_current, &adjerr);
	setOpt(ADJ_OPT_VRMCVIP_CURRENT, vrmcvip_current, &adjerr);
	setOpt(ADJ_OPT_VRMMAX_CURRENT, vrmmax_current, &adjerr);
	setOpt(ADJ_OPT_VRMSOCMAX_CURRENT, vrmsocmax_current, &adjerr);
	setOpt(ADJ_OPT_VRMGFXMAX_CURRENT, vrmgfxmax_current, &adjerr);
	setOpt(ADJ_OPT_PSI0_CURRENT, psi0_current, &adjerr);
	setOpt(ADJ_OPT_PSI3_CPU_CURRENT, psi3cpu_current, &adjerr);
	setOpt(ADJ_OPT_PSI0_SOC_CURRENT, psi0soc_current, &adjerr);
	setOpt(ADJ_OPT_PSI3_GFX_CURRENT, psi3gfx_current, &adjerr);
	setOpt(ADJ_OPT_MAX_SOCCLK_FREQ, max_socclk_freq, &adjerr);
	setOpt(ADJ_OPT_MIN_SOCCLK_FREQ, min_socclk_freq, &adjerr);
	setOpt(ADJ_OPT_MAX_FCLK_FREQ, max_fclk_freq, &adjerr);
	setOpt(ADJ_OPT_MIN_FCLK_FREQ, min_fclk_freq, &adjerr);
	setOpt(ADJ_OPT_MAX_VCN, max_vcn, &adjerr);
	setOpt(ADJ_OPT_MIN_VCN, min_vcn, &adjerr);
	setOpt(ADJ_OPT_MAX_LCLK, max_lclk, &adjerr);
	setOpt(ADJ_OPT_MIN_LCLK, min_lclk, &adjerr);
	setOpt(ADJ_OPT_MAX_GFXCLK_FREQ, max_gfxclk_freq, &adjerr);
	setOpt(ADJ_OPT_MIN_GFXCLK_FREQ, min_gfxclk_freq, &adjerr);
	setOpt(ADJ_OPT_PROCHOT_DEASSERTION_RAMP, prochot_deassertion_ramp, &adjerr);
	setOpt(ADJ_OPT_APU_SKIN_TEMP_LIMIT, apu_skin_temp_limit, &adjerr);
	setOpt(ADJ_OPT_DGPU_SKIN_TEMP_LIMIT, dgpu_skin_temp_limit, &adjerr);
	setOpt(ADJ_OPT_APU_SLOW_LIMIT, apu_slow_limit, &adjerr);
	setOpt(ADJ_OPT_SKIN_TEMP_POWER_LIMIT, skin_temp_power_limit, &adjerr);
	setOpt(ADJ_OPT_GFX_CLK, gfx_clk, &adjerr);
	setOpt(ADJ_OPT_OC_CLK, oc_clk, &adjerr);
	setOpt(ADJ_OPT_OC_VOLT, oc_volt, &adjerr);
	setOptEnable(ADJ_OPT_CCLK_SETPOINT, power_saving, &adjerr);
	setOptEnable(ADJ_OPT_CCLK_BUSY, max_performance, &adjerr);
	setOptEnable(ADJ_OPT_ENABLE_OC, enable_oc, &adjerr);
	setOptEnable(ADJ_OPT_DISABLE_OC, disable_oc, &adjerr);
	setOpt(ADJ_OPT_COALL, coall, &adjerr);
	setOpt(ADJ_OPT_COPER, coper, &adjerr);
	setOpt(ADJ_OPT_COGFX, cogfx, &adjerr);

	if (adjerr == ADJ_OK) { // this is only checking last setopt error
		//call show table dump before anybody did call table refresh, because we want to copy the old values first
		if (dump_table)
			show_table_dump(1);

		//show power table after apply settings
		if (info)
			show_info_table();
	}

	ryzenadj_cleanup();

	return !(adjerr == ADJ_OK);

[Erruar]

@kylon Actually the idea of cleaning up the code and optimizing the library sounds great, but I am categorically against your idea of removing ryzen_access in code. I completely disagree with your opinion that only function calls should remain, although most likely I did not quite understand you. Why do I think so? For example, the use of libryzenadj.dll in apps with several pages, where each page is using Ryzenadj, its easier to use initialization only once, and save this pointer (ryzen_access) and use it across whole app instead of reinitialise it every time after one call. I guess I didn't quite understand your concept. I also have my own fork of Ryzenadj but it aimed only on providing additional information from power table for all supported CPU codenames

[FlyGoat]

@kylon, thanks, that's awesome! I was trying to implement a string based API but yours stand out.

Maybe ryzenadj_get family should be typed?

[FlyGoat]

@kylon Actually the idea of cleaning up the code and optimizing the library sounds great, but I am categorically against your idea of removing ryzen_access in code. I completely disagree with your opinion that only function calls should remain, although most likely I did not quite understand you. Why do I think so? For example, the use of libryzenadj.dll in apps with several pages, where each page is using Ryzenadj, its easier to use initialization only once, and save this pointer (ryzen_access) and use it across whole app instead of reinitialise it every time after one call. I guess I didn't quite understand your concept. I also have my own fork of Ryzenadj but it aimed only on providing additional information from power table for all supported CPU codenames

I think current RyzenAdj is indeed not working well in multi instance scenario. IMO we should keep ryzen_access and maintain a reference counter to handle multiple instances and locking.

[kylon]

@Erruar yea you were supposed to read the code, that was just the hint
ryzen_access is still there but private inside ryzenadj.c and only one at runtime, i dont want wasted memory ofc
the point is, apps should not be aware of ryzen_access and should use api calls instead, if they want something, they ask the library, they dont take smu obj and do their things for example

init is still one time only, just cleaner

[kylon]

@FlyGoat typed? can you explain?

[Erruar]

You see, for example, I used ryzen_access for checking is Ryzenadj already initialized, if it's not zero, then its already initialized. IDK what about your implementation. But that would be great to see auto initialization. Like you are using get or set command, and in dll code if it wasn't initialized before - its initializing and gave results to main app (results as usual get or set command), if its already initialized - just gave the result of call. I think it will be better realization if you hide ryzen_access from main application.

[FlyGoat]

@FlyGoat typed? can you explain?

Like maybe _float, _double, _int?

[kylon]

@Erruar thats not needed, i m not sure why you want to check if it is initialized, assuming you dinamically create/destroy pages, init should be in main, so when the app starts
anyway my init is a noop if already initialized, you can call it as many times as you want, only if access is null (first boot or after cleanup) it will execute
on top of this, i have ADJ_ERROR errors, that return ADJ_NOT_INITIALIZED if you call a function before init, or other errors, and you also can get the text of the error calling the error string function

auto-init is not good in c library, it must be clear that the lifetime is init-cleanup, if you remove init, it may confuse people or they forget about cleanup
if it was a c++ library then yeah, everything would be smooth and managed for users

@FlyGoat not sure, sounds like duplicating functions, i think the proper way is to return double, if the biggest value you can get from table is double, and let users cast to what they need or just use as is because things will be auto-casted anyway like double->int
seems reasonable and no duplicate fns to cast inside the lib

[FlyGoat]

@Erruar thats not needed, i m not sure why you want to check if it is initialized, assuming you dinamically create/destroy pages, init should be in main, so when the app starts anyway my init is a noop if already initialized, you can call it as many times as you want, only if access is null (first boot or after cleanup) it will execute on top of this, i have ADJ_ERROR errors, that return ADJ_NOT_INITIALIZED if you call a function before init, or other errors, and you also can get the text of the error calling the error string function

I don't think this is a proper way to do life-cycle management. If, for example, two libraries linked into the same binary are using ryzenadj, then we will run into double free on teardown. I guess we still need some kind of life-cycle management here, ryzen_access object was designed to do it in objective style.

We'd better still use object to expose life-cycle to API users, but internally we can just init everything once and use a reference counter to manage objects.

auto-init is not good in c library, it must be clear that the lifetime is init-cleanup, if you remove init, it may confuse people or they forget about cleanup if it was a c++ library then yeah, everything would be smooth and managed for users

@FlyGoat not sure, sounds like duplicating functions, i think the proper way is to return double, if the biggest value you can get from table is double, and let users cast to what they need or just use as is because things will be auto-casted anyway like double->int seems reasonable and no duplicate fns to cast inside the lib.

Converting between float and int brings a lot of issues like loosing precision, NaN representation... I think libryzenadj aims to be a slim shim so we shouldn't handle this.

[kylon]

I don't think this is a proper way to do life-cycle management. If, for example, two libraries linked into the same binary are using ryzenadj, then we will run into double free on teardown. I guess we still need some kind of life-cycle management here, ryzen_access object was designed to do it in objective style.

We'd better still use object to expose life-cycle to API users, but internally we can just init everything once and use a reference counter to manage objects.

i think i m not following here, if 2 libs are using ryzenadj, they have their own obj, they will manage it, not the app using the lib, you end up with 2 objs anyway with the exposed access obj and they will free their own, its not double because its not the same memory

dont get what the reference counter should do, sounds like a shared_ptr kinda implementation but my point is that access is unique

unless i m not understanding what you are saying

having to keep access around is a burdern with no clear advantage, for example pci_access (pciutils) makes sense because you need it for various things, but here, access, is just used as parameter for its own calls and i dont think it is good to make smu objs and other things in it public, i mean not directly but in a more structured way, like api to send cmds to smu but not get the smu pointer, if you think it this way, access is no longer valid as a public obj

not sure i m explaining this correctly

Converting between float and int brings a lot of issues like loosing precision, NaN representation... I think libryzenadj aims to be a slim shim so we shouldn't handle this.

yes, that was my point, imo ryzenadj should not do this kind of things or have typed functions, it reads the value and thats it, then the user will do what it want with it
in this case, lib job is to return the value, or do some work internally if the value really needs it

[FlyGoat]

I don't think this is a proper way to do life-cycle management. If, for example, two libraries linked into the same binary are using ryzenadj, then we will run into double free on teardown. I guess we still need some kind of life-cycle management here, ryzen_access object was designed to do it in objective style.
We'd better still use object to expose life-cycle to API users, but internally we can just init everything once and use a reference counter to manage objects.

i think i m not following here, if 2 libs are using ryzenadj, they have their own obj, they will manage it, not the app using the lib, you end up with 2 objs anyway with the exposed access obj and they will free their own, its not double because its not the same memory

For example, say there are two libraries say it's libfoo.so and libbar.so, both dynamically linked against libryzenadj.so, and your executable linked to both libfoo.so and libbar.so, then in your application's address space there will be only one copy of libryzenadj symbols. If libfoo called ryzenadj_cleanup(), then libbar is doomed.

[kylon]

ah yes, right
should be as simple as adding a counter field in access struct like you said, but we dont need to expose the obj even in this case, init and cleanup will handle this, and you can add get api for the ref count
i dont see how the exposed object can improve it, the app should not care and it should not check every time for ryzen_access, it should rely on errors returned by the api

[FlyGoat]

ah yes, right should be as simple as adding a counter field in access struct like you said, but we dont need to expose the obj even in this case, init and cleanup will handle this, and you can add get api for the ref count i dont see how the exposed object can improve it, the app should not care and it should not check every time for ryzen_access, it should rely on errors returned by the api

Mainly to avoid one instance's failure on life cycle management affect another. This is standard practice for thread-safe libraries.