| Message ID | 20230406164807.50969-3-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86/amd/pmf: Updates to AMD PMF driver | expand |
On 4/6/2023 11:48, Shyam Sundar S K wrote: > At times, when the mode transitions fail to happen, the current > driver does not give enough debug information on why the transition > failed or the default preset values did not load. Having an on-demand > logs guarded by CONFIG would be helpful in such cases. > > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > --- > drivers/platform/x86/amd/pmf/Kconfig | 10 ++++++++++ > drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++++++++++++++++++ > drivers/platform/x86/amd/pmf/cnqf.c | 19 +++++++++++++++++++ > 3 files changed, 51 insertions(+) > > diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig > index f4fd764e55a6..7129de0fb9fb 100644 > --- a/drivers/platform/x86/amd/pmf/Kconfig > +++ b/drivers/platform/x86/amd/pmf/Kconfig > @@ -26,3 +26,13 @@ config AMD_PMF_ACPI_DEBUG > in the PMF config store. > > Say Y here to enable more debug logs and Say N here if you are not sure. > + > +config AMD_PMF_DEBUG_FACILITIES > + bool "PMF debug facilities" > + depends on AMD_PMF > + help > + Enabling this option would give more debug information on the PMF interna > + counters such as time constants, power thresholds, target modes and > + power mode transitions of auto mode and CnQF features. With the availability of dynamic debugging is there a lot of benefit to guarding all the new dev_dbg statements behind a config option? Is it because of performance impact? > + > + Say Y here to enable logs and Say N here if you are not sure. > diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c > index 777490fcf8b9..560379b5cda7 100644 > --- a/drivers/platform/x86/amd/pmf/auto-mode.c > +++ b/drivers/platform/x86/amd/pmf/auto-mode.c > @@ -177,11 +177,33 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t > config_store.transition[i].applied = false; > update = true; > } > + > +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES > + dev_dbg(dev->dev, "[AUTO MODE] time_ms:%lld avg_power:%d mode:%s timer:%u tc:%d\n", > + time_elapsed_ms, avg_power, > + state_as_str(config_store.current_mode), > + config_store.transition[i].timer, > + config_store.transition[i].time_constant); > + > + dev_dbg(dev->dev, "[AUTO MODE] shiftup:%d pt:%d pf:%d pd:%u\n", > + config_store.transition[i].shifting_up, > + config_store.transition[i].power_threshold, > + config_store.mode_set[i].power_floor, > + config_store.transition[i].power_delta); > +#endif > } > > dev_dbg(dev->dev, "[AUTO_MODE] avg power: %u mW mode: %s\n", avg_power, > state_as_str(config_store.current_mode)); > > +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES > + dev_dbg(dev->dev, "[AUTO MODE] priority1: %u, priority2: %u, priority3: %u, priority4: %u", > + config_store.transition[0].applied, > + config_store.transition[1].applied, > + config_store.transition[2].applied, > + config_store. transition[3].applied); > +#endif > + > if (update) { > for (j = 0; j < AUTO_TRANSITION_MAX; j++) { > /* Apply the mode with highest priority indentified */ > diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c > index 4b9691cd592a..1f25016b3865 100644 > --- a/drivers/platform/x86/amd/pmf/cnqf.c > +++ b/drivers/platform/x86/amd/pmf/cnqf.c > @@ -174,6 +174,13 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_l > config_store.trans_param[src][i].count++; > > tp = &config_store.trans_param[src][i]; > + > +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES > + dev_dbg(dev->dev, "avg_power:%d total_power:%d count:%d timer:%d\n", avg_power, > + config_store.trans_param[src][i].total_power, > + config_store.trans_param[src][i].count, > + config_store.trans_param[src][i].timer); > +#endif > if (tp->timer >= tp->time_constant && tp->count) { > avg_power = tp->total_power / tp->count; > > @@ -194,6 +201,18 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_l > dev_dbg(dev->dev, "[CNQF] Avg power: %u mW socket power: %u mW mode:%s\n", > avg_power, socket_power, state_as_str(config_store.current_mode)); > > +#ifdef AMD_PMF_DEBUG_FACILITIES > + dev_dbg(dev->dev, "[CNQF] priority 1: %u, priority 2: %u, priority 3: %u", > + config_store.trans_param[src][0].priority, > + config_store.trans_param[src][1].priority, > + config_store.trans_param[src][2].priority); > + > + dev_dbg(dev->dev, "[CNQF] priority 4: %u, priority 5: %u, priority 6: %u", > + config_store.trans_param[src][3].priority, > + config_store.trans_param[src][4].priority, > + config_store.trans_param[src][5].priority); > +#endif > + > for (j = 0; j < CNQF_TRANSITION_MAX; j++) { > /* apply the highest priority */ > if (config_store.trans_param[src][j].priority) {
Hi Mario On 4/6/2023 10:21 PM, Limonciello, Mario wrote: > On 4/6/2023 11:48, Shyam Sundar S K wrote: >> At times, when the mode transitions fail to happen, the current >> driver does not give enough debug information on why the transition >> failed or the default preset values did not load. Having an on-demand >> logs guarded by CONFIG would be helpful in such cases. >> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >> --- >> drivers/platform/x86/amd/pmf/Kconfig | 10 ++++++++++ >> drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++++++++++++++++++ >> drivers/platform/x86/amd/pmf/cnqf.c | 19 +++++++++++++++++++ >> 3 files changed, 51 insertions(+) >> >> diff --git a/drivers/platform/x86/amd/pmf/Kconfig >> b/drivers/platform/x86/amd/pmf/Kconfig >> index f4fd764e55a6..7129de0fb9fb 100644 >> --- a/drivers/platform/x86/amd/pmf/Kconfig >> +++ b/drivers/platform/x86/amd/pmf/Kconfig >> @@ -26,3 +26,13 @@ config AMD_PMF_ACPI_DEBUG >> in the PMF config store. >> Say Y here to enable more debug logs and Say N here if you >> are not sure. >> + >> +config AMD_PMF_DEBUG_FACILITIES >> + bool "PMF debug facilities" >> + depends on AMD_PMF >> + help >> + Enabling this option would give more debug information on the >> PMF interna >> + counters such as time constants, power thresholds, target modes and >> + power mode transitions of auto mode and CnQF features. > > With the availability of dynamic debugging is there a lot of benefit to > guarding all the new dev_dbg statements behind a config option? > > Is it because of performance impact? Yes right. But not all these would be useful all the times when we turn on dyndbg. I borrowed some of these stuff actually from thinkpad_acpi driver. Thanks, Shyam > >> + >> + Say Y here to enable logs and Say N here if you are not sure. >> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c >> b/drivers/platform/x86/amd/pmf/auto-mode.c >> index 777490fcf8b9..560379b5cda7 100644 >> --- a/drivers/platform/x86/amd/pmf/auto-mode.c >> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c >> @@ -177,11 +177,33 @@ void amd_pmf_trans_automode(struct amd_pmf_dev >> *dev, int socket_power, ktime_t t >> config_store.transition[i].applied = false; >> update = true; >> } >> + >> +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES >> + dev_dbg(dev->dev, "[AUTO MODE] time_ms:%lld avg_power:%d >> mode:%s timer:%u tc:%d\n", >> + time_elapsed_ms, avg_power, >> + state_as_str(config_store.current_mode), >> + config_store.transition[i].timer, >> + config_store.transition[i].time_constant); >> + >> + dev_dbg(dev->dev, "[AUTO MODE] shiftup:%d pt:%d pf:%d pd:%u\n", >> + config_store.transition[i].shifting_up, >> + config_store.transition[i].power_threshold, >> + config_store.mode_set[i].power_floor, >> + config_store.transition[i].power_delta); >> +#endif >> } >> dev_dbg(dev->dev, "[AUTO_MODE] avg power: %u mW mode: %s\n", >> avg_power, >> state_as_str(config_store.current_mode)); >> +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES >> + dev_dbg(dev->dev, "[AUTO MODE] priority1: %u, priority2: %u, >> priority3: %u, priority4: %u", >> + config_store.transition[0].applied, >> + config_store.transition[1].applied, >> + config_store.transition[2].applied, >> + config_store. transition[3].applied); >> +#endif >> + >> if (update) { >> for (j = 0; j < AUTO_TRANSITION_MAX; j++) { >> /* Apply the mode with highest priority indentified */ >> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c >> b/drivers/platform/x86/amd/pmf/cnqf.c >> index 4b9691cd592a..1f25016b3865 100644 >> --- a/drivers/platform/x86/amd/pmf/cnqf.c >> +++ b/drivers/platform/x86/amd/pmf/cnqf.c >> @@ -174,6 +174,13 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, >> int socket_power, ktime_t time_l >> config_store.trans_param[src][i].count++; >> tp = &config_store.trans_param[src][i]; >> + >> +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES >> + dev_dbg(dev->dev, "avg_power:%d total_power:%d count:%d >> timer:%d\n", avg_power, >> + config_store.trans_param[src][i].total_power, >> + config_store.trans_param[src][i].count, >> + config_store.trans_param[src][i].timer); >> +#endif >> if (tp->timer >= tp->time_constant && tp->count) { >> avg_power = tp->total_power / tp->count; >> @@ -194,6 +201,18 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, >> int socket_power, ktime_t time_l >> dev_dbg(dev->dev, "[CNQF] Avg power: %u mW socket power: %u mW >> mode:%s\n", >> avg_power, socket_power, >> state_as_str(config_store.current_mode)); >> +#ifdef AMD_PMF_DEBUG_FACILITIES >> + dev_dbg(dev->dev, "[CNQF] priority 1: %u, priority 2: %u, >> priority 3: %u", >> + config_store.trans_param[src][0].priority, >> + config_store.trans_param[src][1].priority, >> + config_store.trans_param[src][2].priority); >> + >> + dev_dbg(dev->dev, "[CNQF] priority 4: %u, priority 5: %u, >> priority 6: %u", >> + config_store.trans_param[src][3].priority, >> + config_store.trans_param[src][4].priority, >> + config_store.trans_param[src][5].priority); >> +#endif >> + >> for (j = 0; j < CNQF_TRANSITION_MAX; j++) { >> /* apply the highest priority */ >> if (config_store.trans_param[src][j].priority) { >
Hi, On 4/6/23 18:48, Shyam Sundar S K wrote: > At times, when the mode transitions fail to happen, the current > driver does not give enough debug information on why the transition > failed or the default preset values did not load. Having an on-demand > logs guarded by CONFIG would be helpful in such cases. > > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > --- > drivers/platform/x86/amd/pmf/Kconfig | 10 ++++++++++ > drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++++++++++++++++++ > drivers/platform/x86/amd/pmf/cnqf.c | 19 +++++++++++++++++++ > 3 files changed, 51 insertions(+) > > diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig > index f4fd764e55a6..7129de0fb9fb 100644 > --- a/drivers/platform/x86/amd/pmf/Kconfig > +++ b/drivers/platform/x86/amd/pmf/Kconfig > @@ -26,3 +26,13 @@ config AMD_PMF_ACPI_DEBUG > in the PMF config store. > > Say Y here to enable more debug logs and Say N here if you are not sure. > + > +config AMD_PMF_DEBUG_FACILITIES > + bool "PMF debug facilities" > + depends on AMD_PMF > + help > + Enabling this option would give more debug information on the PMF interna > + counters such as time constants, power thresholds, target modes and > + power mode transitions of auto mode and CnQF features. > + > + Say Y here to enable logs and Say N here if you are not sure. Do we really need *both* CONFIG_AMD_PMF_ACPI_DEBUG and CONFIG_AMD_PMF_DEBUG_FACILITIES ? To me it seems that both could be covered by a generic CONFIG_AMD_PMF_DEBUG option ? especially since this patch only adds a few extra dev_dbg-s. > diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c > index 777490fcf8b9..560379b5cda7 100644 > --- a/drivers/platform/x86/amd/pmf/auto-mode.c > +++ b/drivers/platform/x86/amd/pmf/auto-mode.c > @@ -177,11 +177,33 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t > config_store.transition[i].applied = false; > update = true; > } > + > +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES > + dev_dbg(dev->dev, "[AUTO MODE] time_ms:%lld avg_power:%d mode:%s timer:%u tc:%d\n", > + time_elapsed_ms, avg_power, > + state_as_str(config_store.current_mode), > + config_store.transition[i].timer, > + config_store.transition[i].time_constant); > + > + dev_dbg(dev->dev, "[AUTO MODE] shiftup:%d pt:%d pf:%d pd:%u\n", > + config_store.transition[i].shifting_up, > + config_store.transition[i].power_threshold, > + config_store.mode_set[i].power_floor, > + config_store.transition[i].power_delta); > +#endif > } > > dev_dbg(dev->dev, "[AUTO_MODE] avg power: %u mW mode: %s\n", avg_power, > state_as_str(config_store.current_mode)); > > +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES > + dev_dbg(dev->dev, "[AUTO MODE] priority1: %u, priority2: %u, priority3: %u, priority4: %u", > + config_store.transition[0].applied, > + config_store.transition[1].applied, > + config_store.transition[2].applied, > + config_store. transition[3].applied); > +#endif > + > if (update) { > for (j = 0; j < AUTO_TRANSITION_MAX; j++) { > /* Apply the mode with highest priority indentified */ > diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c > index 4b9691cd592a..1f25016b3865 100644 > --- a/drivers/platform/x86/amd/pmf/cnqf.c > +++ b/drivers/platform/x86/amd/pmf/cnqf.c > @@ -174,6 +174,13 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_l > config_store.trans_param[src][i].count++; > > tp = &config_store.trans_param[src][i]; > + > +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES > + dev_dbg(dev->dev, "avg_power:%d total_power:%d count:%d timer:%d\n", avg_power, > + config_store.trans_param[src][i].total_power, > + config_store.trans_param[src][i].count, > + config_store.trans_param[src][i].timer); > +#endif > if (tp->timer >= tp->time_constant && tp->count) { > avg_power = tp->total_power / tp->count; > > @@ -194,6 +201,18 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_l > dev_dbg(dev->dev, "[CNQF] Avg power: %u mW socket power: %u mW mode:%s\n", > avg_power, socket_power, state_as_str(config_store.current_mode)); > > +#ifdef AMD_PMF_DEBUG_FACILITIES > + dev_dbg(dev->dev, "[CNQF] priority 1: %u, priority 2: %u, priority 3: %u", > + config_store.trans_param[src][0].priority, > + config_store.trans_param[src][1].priority, > + config_store.trans_param[src][2].priority); > + > + dev_dbg(dev->dev, "[CNQF] priority 4: %u, priority 5: %u, priority 6: %u", > + config_store.trans_param[src][3].priority, > + config_store.trans_param[src][4].priority, > + config_store.trans_param[src][5].priority); > +#endif > + > for (j = 0; j < CNQF_TRANSITION_MAX; j++) { > /* apply the highest priority */ > if (config_store.trans_param[src][j].priority) { Regards, Hans
diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig index f4fd764e55a6..7129de0fb9fb 100644 --- a/drivers/platform/x86/amd/pmf/Kconfig +++ b/drivers/platform/x86/amd/pmf/Kconfig @@ -26,3 +26,13 @@ config AMD_PMF_ACPI_DEBUG in the PMF config store. Say Y here to enable more debug logs and Say N here if you are not sure. + +config AMD_PMF_DEBUG_FACILITIES + bool "PMF debug facilities" + depends on AMD_PMF + help + Enabling this option would give more debug information on the PMF interna + counters such as time constants, power thresholds, target modes and + power mode transitions of auto mode and CnQF features. + + Say Y here to enable logs and Say N here if you are not sure. diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c index 777490fcf8b9..560379b5cda7 100644 --- a/drivers/platform/x86/amd/pmf/auto-mode.c +++ b/drivers/platform/x86/amd/pmf/auto-mode.c @@ -177,11 +177,33 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t config_store.transition[i].applied = false; update = true; } + +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES + dev_dbg(dev->dev, "[AUTO MODE] time_ms:%lld avg_power:%d mode:%s timer:%u tc:%d\n", + time_elapsed_ms, avg_power, + state_as_str(config_store.current_mode), + config_store.transition[i].timer, + config_store.transition[i].time_constant); + + dev_dbg(dev->dev, "[AUTO MODE] shiftup:%d pt:%d pf:%d pd:%u\n", + config_store.transition[i].shifting_up, + config_store.transition[i].power_threshold, + config_store.mode_set[i].power_floor, + config_store.transition[i].power_delta); +#endif } dev_dbg(dev->dev, "[AUTO_MODE] avg power: %u mW mode: %s\n", avg_power, state_as_str(config_store.current_mode)); +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES + dev_dbg(dev->dev, "[AUTO MODE] priority1: %u, priority2: %u, priority3: %u, priority4: %u", + config_store.transition[0].applied, + config_store.transition[1].applied, + config_store.transition[2].applied, + config_store. transition[3].applied); +#endif + if (update) { for (j = 0; j < AUTO_TRANSITION_MAX; j++) { /* Apply the mode with highest priority indentified */ diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c index 4b9691cd592a..1f25016b3865 100644 --- a/drivers/platform/x86/amd/pmf/cnqf.c +++ b/drivers/platform/x86/amd/pmf/cnqf.c @@ -174,6 +174,13 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_l config_store.trans_param[src][i].count++; tp = &config_store.trans_param[src][i]; + +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES + dev_dbg(dev->dev, "avg_power:%d total_power:%d count:%d timer:%d\n", avg_power, + config_store.trans_param[src][i].total_power, + config_store.trans_param[src][i].count, + config_store.trans_param[src][i].timer); +#endif if (tp->timer >= tp->time_constant && tp->count) { avg_power = tp->total_power / tp->count; @@ -194,6 +201,18 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_l dev_dbg(dev->dev, "[CNQF] Avg power: %u mW socket power: %u mW mode:%s\n", avg_power, socket_power, state_as_str(config_store.current_mode)); +#ifdef AMD_PMF_DEBUG_FACILITIES + dev_dbg(dev->dev, "[CNQF] priority 1: %u, priority 2: %u, priority 3: %u", + config_store.trans_param[src][0].priority, + config_store.trans_param[src][1].priority, + config_store.trans_param[src][2].priority); + + dev_dbg(dev->dev, "[CNQF] priority 4: %u, priority 5: %u, priority 6: %u", + config_store.trans_param[src][3].priority, + config_store.trans_param[src][4].priority, + config_store.trans_param[src][5].priority); +#endif + for (j = 0; j < CNQF_TRANSITION_MAX; j++) { /* apply the highest priority */ if (config_store.trans_param[src][j].priority) {
At times, when the mode transitions fail to happen, the current driver does not give enough debug information on why the transition failed or the default preset values did not load. Having an on-demand logs guarded by CONFIG would be helpful in such cases. Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> --- drivers/platform/x86/amd/pmf/Kconfig | 10 ++++++++++ drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++++++++++++++++++ drivers/platform/x86/amd/pmf/cnqf.c | 19 +++++++++++++++++++ 3 files changed, 51 insertions(+)