RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
[AMD Official Use Only] Thanks for reminder. As I confirmed, the OD settings will be reset to default after gpu reset. As smu->adev->in_suspend is 0 during gpu reset. And that will reset OD settings to default. BR Evan > -Original Message- > From: Matt Coffin > Sent: Saturday, July 24, 2021 4:00 AM > To: Quan, Evan > Cc: amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings > properly for NV1x > > On 7/21/21 9:20 PM, Evan Quan wrote: > > The customized OD settings can be divided into two parts: those > > committed ones and non-committed ones. > >- For those changes which had been fed to SMU before S3/S4/Runpm > > suspend kicked, they are committed changes. They should be properly > > restored and fed to SMU on S3/S4/Runpm resume. > >- For those non-committed changes, they are restored only without > feeding > > to SMU. > > > > Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440 > > Signed-off-by: Evan Quan > > --- > > It's been a while since I've been through the SMU code so I don't know all > the places that restore_uesr_profile is used, but, just to confirm, these > would still go back to default (boot) values on a GPU reset, yes? > > I ask because when pushing OD settings too far, GPU resets are often > encountered, and you might be able to create a state where it would > continually reset if your OD settings are remembered even through a full > reset. > > Very nice feature other than that, though, as I actually keep `radeontop` > open in a tmux session to prevent my secondary card from resetting my OD > settings, so this will be nice. > > Thanks for listening, and for the driver in general, Matt ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
On 7/21/21 9:20 PM, Evan Quan wrote: The customized OD settings can be divided into two parts: those committed ones and non-committed ones. - For those changes which had been fed to SMU before S3/S4/Runpm suspend kicked, they are committed changes. They should be properly restored and fed to SMU on S3/S4/Runpm resume. - For those non-committed changes, they are restored only without feeding to SMU. Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440 Signed-off-by: Evan Quan --- It's been a while since I've been through the SMU code so I don't know all the places that restore_uesr_profile is used, but, just to confirm, these would still go back to default (boot) values on a GPU reset, yes? I ask because when pushing OD settings too far, GPU resets are often encountered, and you might be able to create a state where it would continually reset if your OD settings are remembered even through a full reset. Very nice feature other than that, though, as I actually keep `radeontop` open in a tmux session to prevent my secondary card from resetting my OD settings, so this will be nice. Thanks for listening, and for the driver in general, Matt ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
[Public]
I probably get your point. Please check the update V2.
BR
Evan
> -Original Message-
> From: Lazar, Lijo
> Sent: Friday, July 23, 2021 3:39 PM
> To: Quan, Evan ; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander
> Subject: RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings
> properly for NV1x
>
> [Public]
>
> Hi Evan,
>
> In that case, using "od_edit" table for the intermediate table
>
> During commit command, what if it's done like
> 1) Copy and update table if od_edit table is different than user_od
> table
> 2) Set the flag to false if od_edit table and boot_od table are
> different
>
> Then current od settings may always be picked from user_od table and the
> flag may be used to distinguish if it's boot od settings or custom settings
> (for
> restore or other purposes).
>
> Thanks,
> Lijo
>
> -Original Message-
> From: Quan, Evan
> Sent: Friday, July 23, 2021 12:51 PM
> To: Lazar, Lijo ; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander
> Subject: RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings
> properly for NV1x
>
> [AMD Official Use Only]
>
> Hi Lijo,
>
> Sorry, I doubled checked. The implementation of Navi1x is right.
> The original design for "restores to default settings" is a two-step process.
> That means for "case PP_OD_COMMIT_DPM_TABLE:" we must distinguish
> whether it's an usual customized od setting update or just restoring to
> defaults.
> And my current implementation for that is as below. Any comments?
>
> + if (memcmp(table_context->overdrive_table, table_context-
> >boot_overdrive_table,
> + sizeof(OverDriveTable_t))) {
> + smu->user_dpm_profile.committed_od = true;
> + memcpy(table_context->committed_overdrive_table,
> table_context->overdrive_table,
> + sizeof(OverDriveTable_t));
> + } else {
> + smu->user_dpm_profile.committed_od = false;
> + }
>
> BR
> Evan
> > -Original Message-----
> > From: Quan, Evan
> > Sent: Friday, July 23, 2021 2:48 PM
> > To: Lazar, Lijo ; amd-gfx@lists.freedesktop.org
> > Cc: Deucher, Alexander
> > Subject: RE: [PATCH 1/2] drm/amd/pm: restore user customized OD
> > settings properly for NV1x
> >
> > [AMD Official Use Only]
> >
> >
> >
> > > -Original Message-
> > > From: Lazar, Lijo
> > > Sent: Thursday, July 22, 2021 5:03 PM
> > > To: Quan, Evan ; amd-gfx@lists.freedesktop.org
> > > Cc: Deucher, Alexander
> > > Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD
> > > settings properly for NV1x
> > >
> > >
> > >
> > > On 7/22/2021 2:03 PM, Quan, Evan wrote:
> > > > [AMD Official Use Only]
> > > >
> > > >
> > > >
> > > >> -Original Message-
> > > >> From: Lazar, Lijo
> > > >> Sent: Thursday, July 22, 2021 4:10 PM
> > > >> To: Quan, Evan ; amd-
> > g...@lists.freedesktop.org
> > > >> Cc: Deucher, Alexander
> > > >> Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD
> > > >> settings properly for NV1x
> > > >>
> > > >>
> > > >>
> > > >> On 7/22/2021 8:50 AM, Evan Quan wrote:
> > > >>> The customized OD settings can be divided into two parts: those
> > > >>> committed ones and non-committed ones.
> > > >>> - For those changes which had been fed to SMU before
> > S3/S4/Runpm
> > > >>> suspend kicked, they are committed changes. They should be
> > > properly
> > > >>> restored and fed to SMU on S3/S4/Runpm resume.
> > > >>> - For those non-committed changes, they are restored only
> > > >>> without
> > > >> feeding
> > > >>> to SMU.
> > > >>>
> > > >>> Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
> > > >>> Signed-off-by: Evan Quan
> > > >>> ---
> > > >>>drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
> > > >>>drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9
> > > >>>.../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 52
> > > >> ++-
> > > &
RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
[Public]
Hi Evan,
In that case, using "od_edit" table for the intermediate table
During commit command, what if it's done like
1) Copy and update table if od_edit table is different than user_od
table
2) Set the flag to false if od_edit table and boot_od table are
different
Then current od settings may always be picked from user_od table and the flag
may be used to distinguish if it's boot od settings or custom settings (for
restore or other purposes).
Thanks,
Lijo
-Original Message-
From: Quan, Evan
Sent: Friday, July 23, 2021 12:51 PM
To: Lazar, Lijo ; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander
Subject: RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings
properly for NV1x
[AMD Official Use Only]
Hi Lijo,
Sorry, I doubled checked. The implementation of Navi1x is right.
The original design for "restores to default settings" is a two-step process.
That means for "case PP_OD_COMMIT_DPM_TABLE:" we must distinguish whether it's
an usual customized od setting update or just restoring to defaults.
And my current implementation for that is as below. Any comments?
+ if (memcmp(table_context->overdrive_table,
table_context->boot_overdrive_table,
+ sizeof(OverDriveTable_t))) {
+ smu->user_dpm_profile.committed_od = true;
+ memcpy(table_context->committed_overdrive_table,
table_context->overdrive_table,
+ sizeof(OverDriveTable_t));
+ } else {
+ smu->user_dpm_profile.committed_od = false;
+ }
BR
Evan
> -Original Message-
> From: Quan, Evan
> Sent: Friday, July 23, 2021 2:48 PM
> To: Lazar, Lijo ; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander
> Subject: RE: [PATCH 1/2] drm/amd/pm: restore user customized OD
> settings properly for NV1x
>
> [AMD Official Use Only]
>
>
>
> > -Original Message-
> > From: Lazar, Lijo
> > Sent: Thursday, July 22, 2021 5:03 PM
> > To: Quan, Evan ; amd-gfx@lists.freedesktop.org
> > Cc: Deucher, Alexander
> > Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD
> > settings properly for NV1x
> >
> >
> >
> > On 7/22/2021 2:03 PM, Quan, Evan wrote:
> > > [AMD Official Use Only]
> > >
> > >
> > >
> > >> -Original Message-----
> > >> From: Lazar, Lijo
> > >> Sent: Thursday, July 22, 2021 4:10 PM
> > >> To: Quan, Evan ; amd-
> g...@lists.freedesktop.org
> > >> Cc: Deucher, Alexander
> > >> Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD
> > >> settings properly for NV1x
> > >>
> > >>
> > >>
> > >> On 7/22/2021 8:50 AM, Evan Quan wrote:
> > >>> The customized OD settings can be divided into two parts: those
> > >>> committed ones and non-committed ones.
> > >>> - For those changes which had been fed to SMU before
> S3/S4/Runpm
> > >>> suspend kicked, they are committed changes. They should be
> > properly
> > >>> restored and fed to SMU on S3/S4/Runpm resume.
> > >>> - For those non-committed changes, they are restored only
> > >>> without
> > >> feeding
> > >>> to SMU.
> > >>>
> > >>> Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
> > >>> Signed-off-by: Evan Quan
> > >>> ---
> > >>>drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
> > >>>drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9
> > >>>.../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 52
> > >> ++-
> > >>>.../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c| 12 +
> > >>>4 files changed, 69 insertions(+), 12 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> index 3e89852e4820..8ba53f16d2d9 100644
> > >>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> @@ -231,6 +231,7 @@ struct smu_user_dpm_profile {
> > >>> uint32_t power_limit;
> > >>> uint32_t fan_speed_percent;
> > >>> uint32_t flags;
> > >>> + uint32_t committed_od;
> > >>>
> > >>> /* user clock state information */
> > >
RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
[AMD Official Use Only]
Hi Lijo,
Sorry, I doubled checked. The implementation of Navi1x is right.
The original design for "restores to default settings" is a two-step process.
That means for "case PP_OD_COMMIT_DPM_TABLE:" we must distinguish whether it's
an usual customized od setting update or just restoring to defaults.
And my current implementation for that is as below. Any comments?
+ if (memcmp(table_context->overdrive_table,
table_context->boot_overdrive_table,
+ sizeof(OverDriveTable_t))) {
+ smu->user_dpm_profile.committed_od = true;
+ memcpy(table_context->committed_overdrive_table,
table_context->overdrive_table,
+ sizeof(OverDriveTable_t));
+ } else {
+ smu->user_dpm_profile.committed_od = false;
+ }
BR
Evan
> -Original Message-
> From: Quan, Evan
> Sent: Friday, July 23, 2021 2:48 PM
> To: Lazar, Lijo ; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander
> Subject: RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings
> properly for NV1x
>
> [AMD Official Use Only]
>
>
>
> > -Original Message-
> > From: Lazar, Lijo
> > Sent: Thursday, July 22, 2021 5:03 PM
> > To: Quan, Evan ; amd-gfx@lists.freedesktop.org
> > Cc: Deucher, Alexander
> > Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD
> > settings properly for NV1x
> >
> >
> >
> > On 7/22/2021 2:03 PM, Quan, Evan wrote:
> > > [AMD Official Use Only]
> > >
> > >
> > >
> > >> -Original Message-
> > >> From: Lazar, Lijo
> > >> Sent: Thursday, July 22, 2021 4:10 PM
> > >> To: Quan, Evan ; amd-
> g...@lists.freedesktop.org
> > >> Cc: Deucher, Alexander
> > >> Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD
> > >> settings properly for NV1x
> > >>
> > >>
> > >>
> > >> On 7/22/2021 8:50 AM, Evan Quan wrote:
> > >>> The customized OD settings can be divided into two parts: those
> > >>> committed ones and non-committed ones.
> > >>> - For those changes which had been fed to SMU before
> S3/S4/Runpm
> > >>> suspend kicked, they are committed changes. They should be
> > properly
> > >>> restored and fed to SMU on S3/S4/Runpm resume.
> > >>> - For those non-committed changes, they are restored only
> > >>> without
> > >> feeding
> > >>> to SMU.
> > >>>
> > >>> Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
> > >>> Signed-off-by: Evan Quan
> > >>> ---
> > >>>drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
> > >>>drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9
> > >>>.../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 52
> > >> ++-
> > >>>.../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c| 12 +
> > >>>4 files changed, 69 insertions(+), 12 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> index 3e89852e4820..8ba53f16d2d9 100644
> > >>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> @@ -231,6 +231,7 @@ struct smu_user_dpm_profile {
> > >>> uint32_t power_limit;
> > >>> uint32_t fan_speed_percent;
> > >>> uint32_t flags;
> > >>> + uint32_t committed_od;
> > >>>
> > >>> /* user clock state information */
> > >>> uint32_t clk_mask[SMU_CLK_COUNT]; @@ -352,6 +353,7
> @@ struct
> > >>> smu_table_context
> > >>>
> > >>> void*overdrive_table;
> > >>> void*boot_overdrive_table;
> > >>> + void*committed_overdrive_table;
> > >>
> > >> May be rename it to user_overdrive_table - OD table with user settings?
> > > [Quan, Evan] Actually "overdrive_table " is the
> > > user_overdrive_table. It
> > contains all the modification including those not committed( the
> > commit is performed by echo "c" >
>
RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
[AMD Official Use Only]
> -Original Message-
> From: Lazar, Lijo
> Sent: Thursday, July 22, 2021 5:03 PM
> To: Quan, Evan ; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander
> Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings
> properly for NV1x
>
>
>
> On 7/22/2021 2:03 PM, Quan, Evan wrote:
> > [AMD Official Use Only]
> >
> >
> >
> >> -Original Message-
> >> From: Lazar, Lijo
> >> Sent: Thursday, July 22, 2021 4:10 PM
> >> To: Quan, Evan ; amd-gfx@lists.freedesktop.org
> >> Cc: Deucher, Alexander
> >> Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD
> >> settings properly for NV1x
> >>
> >>
> >>
> >> On 7/22/2021 8:50 AM, Evan Quan wrote:
> >>> The customized OD settings can be divided into two parts: those
> >>> committed ones and non-committed ones.
> >>> - For those changes which had been fed to SMU before S3/S4/Runpm
> >>> suspend kicked, they are committed changes. They should be
> properly
> >>> restored and fed to SMU on S3/S4/Runpm resume.
> >>> - For those non-committed changes, they are restored only
> >>> without
> >> feeding
> >>> to SMU.
> >>>
> >>> Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
> >>> Signed-off-by: Evan Quan
> >>> ---
> >>>drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
> >>>drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9
> >>>.../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 52
> >> ++-
> >>>.../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c| 12 +
> >>>4 files changed, 69 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> >>> b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> >>> index 3e89852e4820..8ba53f16d2d9 100644
> >>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> >>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> >>> @@ -231,6 +231,7 @@ struct smu_user_dpm_profile {
> >>> uint32_t power_limit;
> >>> uint32_t fan_speed_percent;
> >>> uint32_t flags;
> >>> + uint32_t committed_od;
> >>>
> >>> /* user clock state information */
> >>> uint32_t clk_mask[SMU_CLK_COUNT]; @@ -352,6 +353,7 @@ struct
> >>> smu_table_context
> >>>
> >>> void*overdrive_table;
> >>> void*boot_overdrive_table;
> >>> + void*committed_overdrive_table;
> >>
> >> May be rename it to user_overdrive_table - OD table with user settings?
> > [Quan, Evan] Actually "overdrive_table " is the user_overdrive_table. It
> contains all the modification including those not committed( the commit is
> performed by echo "c" > /sys/class/drm/card1/device/pp_od_clk_voltage).
> > The new member added refers to those user customized but committed
> only settings. That's why it was named as " committed_overdrive_table".
> Any good suggestion for the naming?
>
> As far as I understand, the problem is overdrive_table can have intemediary
> settings as the edit/commit is a two-step process. At any point we can have
> user_od_table keep the latest user mode settings. That is true when user
> restores to default settings also; that time we can just copy the boot
> settings
> back to user_od table and keep the flag as false indicating that there is no
> custom settings.
[Quan, Evan] For now, on Navi1x the "restores to default settings" is also a
two-step process. That will make "copy the boot settings
back to user_od table and keep the flag as false" tricky. However, it seems
that does not fit original design. As per original design,
the "restores to default settings" should be a one-step process. Let me correct
them with new patch series and proceed further discussion then.
BR
Evan
>
> >>
> >>> uint32_tgpu_metrics_table_size;
> >>> void*gpu_metrics_table;
> >>> @@ -623,6 +625,12 @@ struct pptable_funcs {
> >>>enum PP_OD_DPM_TABLE_COMMAND
> >> type,
> >>>long *input, uint32
Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
On 7/22/2021 2:03 PM, Quan, Evan wrote:
[AMD Official Use Only]
-Original Message-
From: Lazar, Lijo
Sent: Thursday, July 22, 2021 4:10 PM
To: Quan, Evan ; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander
Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings
properly for NV1x
On 7/22/2021 8:50 AM, Evan Quan wrote:
The customized OD settings can be divided into two parts: those
committed ones and non-committed ones.
- For those changes which had been fed to SMU before S3/S4/Runpm
suspend kicked, they are committed changes. They should be properly
restored and fed to SMU on S3/S4/Runpm resume.
- For those non-committed changes, they are restored only without
feeding
to SMU.
Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
Signed-off-by: Evan Quan
---
drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9
.../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 52
++-
.../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c| 12 +
4 files changed, 69 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
index 3e89852e4820..8ba53f16d2d9 100644
--- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
@@ -231,6 +231,7 @@ struct smu_user_dpm_profile {
uint32_t power_limit;
uint32_t fan_speed_percent;
uint32_t flags;
+ uint32_t committed_od;
/* user clock state information */
uint32_t clk_mask[SMU_CLK_COUNT];
@@ -352,6 +353,7 @@ struct smu_table_context
void*overdrive_table;
void*boot_overdrive_table;
+ void*committed_overdrive_table;
May be rename it to user_overdrive_table - OD table with user settings?
[Quan, Evan] Actually "overdrive_table " is the user_overdrive_table. It contains all the
modification including those not committed( the commit is performed by echo "c" >
/sys/class/drm/card1/device/pp_od_clk_voltage).
The new member added refers to those user customized but committed only settings. That's
why it was named as " committed_overdrive_table". Any good suggestion for the
naming?
As far as I understand, the problem is overdrive_table can have
intemediary settings as the edit/commit is a two-step process. At any
point we can have user_od_table keep the latest user mode settings. That
is true when user restores to default settings also; that time we can
just copy the boot settings back to user_od table and keep the flag as
false indicating that there is no custom settings.
uint32_tgpu_metrics_table_size;
void*gpu_metrics_table;
@@ -623,6 +625,12 @@ struct pptable_funcs {
enum PP_OD_DPM_TABLE_COMMAND
type,
long *input, uint32_t size);
+ /**
+* @restore_committed_od_settings: Restore the customized and
committed
+* OD settings on S3/S4/Runpm resume.
+*/
+ int (*restore_committed_od_settings)(struct smu_context *smu);
+
/**
* @get_clock_by_type_with_latency: Get the speed and latency of
a clock
* domain.
diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index ebe672142808..5f7d98e99f76 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -416,6 +416,15 @@ static void smu_restore_dpm_user_profile(struct
smu_context *smu)
}
}
+ /* Restore customized and committed OD settings */
+ if (smu->user_dpm_profile.committed_od) {
+ if (smu->ppt_funcs->restore_committed_od_settings) {
+ ret = smu->ppt_funcs-
restore_committed_od_settings(smu);
+ if (ret)
+ dev_err(smu->adev->dev, "Failed to upload
customized OD settings\n");
+ }
+ }
+
Just thinking if there is a need to handle it ASIC specific. The flags and table
pointer are maintained in common structure. So can't we do the restore of
user OD settings directly in this common flow instead of having each ASIC to
implement the callback?
[Quan, Evan] The OD settings restoring is ASIC specific as it performed on OD
table and that(OD table) is ASIC specific.
I was thinking in terms of final logic that is being done. The below
structures are available in common table context and we have a common
logic to update the table. If there is no custom OD settings available,
we could handle it with the flag.
+ struct smu_table_context *table_context = >smu_table;
+
RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
[AMD Official Use Only]
> -Original Message-
> From: Lazar, Lijo
> Sent: Thursday, July 22, 2021 4:10 PM
> To: Quan, Evan ; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander
> Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings
> properly for NV1x
>
>
>
> On 7/22/2021 8:50 AM, Evan Quan wrote:
> > The customized OD settings can be divided into two parts: those
> > committed ones and non-committed ones.
> >- For those changes which had been fed to SMU before S3/S4/Runpm
> > suspend kicked, they are committed changes. They should be properly
> > restored and fed to SMU on S3/S4/Runpm resume.
> >- For those non-committed changes, they are restored only without
> feeding
> > to SMU.
> >
> > Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
> > Signed-off-by: Evan Quan
> > ---
> > drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
> > drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9
> > .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 52
> ++-
> > .../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c| 12 +
> > 4 files changed, 69 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > index 3e89852e4820..8ba53f16d2d9 100644
> > --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > @@ -231,6 +231,7 @@ struct smu_user_dpm_profile {
> > uint32_t power_limit;
> > uint32_t fan_speed_percent;
> > uint32_t flags;
> > + uint32_t committed_od;
> >
> > /* user clock state information */
> > uint32_t clk_mask[SMU_CLK_COUNT];
> > @@ -352,6 +353,7 @@ struct smu_table_context
> >
> > void*overdrive_table;
> > void*boot_overdrive_table;
> > + void*committed_overdrive_table;
>
> May be rename it to user_overdrive_table - OD table with user settings?
[Quan, Evan] Actually "overdrive_table " is the user_overdrive_table. It
contains all the modification including those not committed( the commit is
performed by echo "c" > /sys/class/drm/card1/device/pp_od_clk_voltage).
The new member added refers to those user customized but committed only
settings. That's why it was named as " committed_overdrive_table". Any good
suggestion for the naming?
>
> > uint32_tgpu_metrics_table_size;
> > void*gpu_metrics_table;
> > @@ -623,6 +625,12 @@ struct pptable_funcs {
> > enum PP_OD_DPM_TABLE_COMMAND
> type,
> > long *input, uint32_t size);
> >
> > + /**
> > +* @restore_committed_od_settings: Restore the customized and
> committed
> > +* OD settings on S3/S4/Runpm resume.
> > +*/
> > + int (*restore_committed_od_settings)(struct smu_context *smu);
> > +
> > /**
> > * @get_clock_by_type_with_latency: Get the speed and latency of
> a clock
> > * domain.
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > index ebe672142808..5f7d98e99f76 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > @@ -416,6 +416,15 @@ static void smu_restore_dpm_user_profile(struct
> smu_context *smu)
> > }
> > }
> >
> > + /* Restore customized and committed OD settings */
> > + if (smu->user_dpm_profile.committed_od) {
> > + if (smu->ppt_funcs->restore_committed_od_settings) {
> > + ret = smu->ppt_funcs-
> >restore_committed_od_settings(smu);
> > + if (ret)
> > + dev_err(smu->adev->dev, "Failed to upload
> customized OD settings\n");
> > + }
> > + }
> > +
>
> Just thinking if there is a need to handle it ASIC specific. The flags and
> table
> pointer are maintained in common structure. So can't we do the restore of
> user OD settings directly in this common flow instead of having each ASIC to
> implement the callback?
[Quan, Evan] The OD settings restoring is ASIC specific as it performed on OD
table and that(OD table) is ASIC specific.
>
> > /* Disable restore flag */
> > smu->user_dpm_profile.flags &=
> ~SM
Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
On 7/22/2021 8:50 AM, Evan Quan wrote:
The customized OD settings can be divided into two parts: those
committed ones and non-committed ones.
- For those changes which had been fed to SMU before S3/S4/Runpm
suspend kicked, they are committed changes. They should be properly
restored and fed to SMU on S3/S4/Runpm resume.
- For those non-committed changes, they are restored only without feeding
to SMU.
Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
Signed-off-by: Evan Quan
---
drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9
.../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 52 ++-
.../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c| 12 +
4 files changed, 69 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
index 3e89852e4820..8ba53f16d2d9 100644
--- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
@@ -231,6 +231,7 @@ struct smu_user_dpm_profile {
uint32_t power_limit;
uint32_t fan_speed_percent;
uint32_t flags;
+ uint32_t committed_od;
/* user clock state information */
uint32_t clk_mask[SMU_CLK_COUNT];
@@ -352,6 +353,7 @@ struct smu_table_context
void*overdrive_table;
void*boot_overdrive_table;
+ void*committed_overdrive_table;
May be rename it to user_overdrive_table - OD table with user settings?
uint32_tgpu_metrics_table_size;
void*gpu_metrics_table;
@@ -623,6 +625,12 @@ struct pptable_funcs {
enum PP_OD_DPM_TABLE_COMMAND type,
long *input, uint32_t size);
+ /**
+* @restore_committed_od_settings: Restore the customized and committed
+* OD settings on S3/S4/Runpm resume.
+*/
+ int (*restore_committed_od_settings)(struct smu_context *smu);
+
/**
* @get_clock_by_type_with_latency: Get the speed and latency of a clock
* domain.
diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index ebe672142808..5f7d98e99f76 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -416,6 +416,15 @@ static void smu_restore_dpm_user_profile(struct
smu_context *smu)
}
}
+ /* Restore customized and committed OD settings */
+ if (smu->user_dpm_profile.committed_od) {
+ if (smu->ppt_funcs->restore_committed_od_settings) {
+ ret =
smu->ppt_funcs->restore_committed_od_settings(smu);
+ if (ret)
+ dev_err(smu->adev->dev, "Failed to upload customized
OD settings\n");
+ }
+ }
+
Just thinking if there is a need to handle it ASIC specific. The flags
and table pointer are maintained in common structure. So can't we do the
restore of user OD settings directly in this common flow instead of
having each ASIC to implement the callback?
/* Disable restore flag */
smu->user_dpm_profile.flags &= ~SMU_DPM_USER_PROFILE_RESTORE;
}
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
index 59ea59acfb00..4752299d7f91 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
@@ -2296,39 +2296,45 @@ static int navi10_set_default_od_settings(struct
smu_context *smu)
(OverDriveTable_t *)smu->smu_table.boot_overdrive_table;
int ret = 0;
- ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0, (void *)od_table, false);
+ ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0, (void
*)boot_od_table, false);
if (ret) {
dev_err(smu->adev->dev, "Failed to get overdrive table!\n");
return ret;
}
- if (!od_table->GfxclkVolt1) {
+ if (!boot_od_table->GfxclkVolt1) {
ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
-
_table->GfxclkVolt1,
-
od_table->GfxclkFreq1);
+
_od_table->GfxclkVolt1,
+
boot_od_table->GfxclkFreq1);
if (ret)
return ret;
}
- if (!od_table->GfxclkVolt2) {
+ if (!boot_od_table->GfxclkVolt2) {
ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
-
