| Message ID | 20211104225226.5031-5-chang.seok.bae@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | None | expand |
Chang, On Thu, Nov 04 2021 at 15:52, Chang S. Bae wrote: > +/** > + * intel_idle_tile - Ask the processor to enter the given idle state. > + * @dev: cpuidle device of the target CPU. > + * @drv: cpuidle driver (assumed to point to intel_idle_driver). > + * > + * Ensure TILE registers in INIT-state before using intel_idle() to > + * enter the idle state. > + */ > +static __cpuidle int intel_idle_tile(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + fpu_idle_fpregs(); That's redundant because arch_cpu_idle_enter() is invoked before the actual idle mechanism. > +/** > + * intel_idle_s2idle_tile - Ask the processor to enter the given idle state. > + * @dev: cpuidle device of the target CPU. > + * @drv: cpuidle driver (assumed to point to intel_idle_driver). > + * @index: Target idle state index. > + * > + * Ensure TILE registers in INIT-state before using intel_idle_s2idle() to > + * enter the idle state. > + */ > +static __cpuidle int intel_idle_s2idle_tile(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + fpu_idle_fpregs(); Ditto Thanks, tglx
On Nov 5, 2021, at 07:33, Thomas Gleixner <tglx@linutronix.de> wrote: > > Chang, > > On Thu, Nov 04 2021 at 15:52, Chang S. Bae wrote: >> +/** >> + * intel_idle_tile - Ask the processor to enter the given idle state. >> + * @dev: cpuidle device of the target CPU. >> + * @drv: cpuidle driver (assumed to point to intel_idle_driver). >> + * >> + * Ensure TILE registers in INIT-state before using intel_idle() to >> + * enter the idle state. >> + */ >> +static __cpuidle int intel_idle_tile(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int index) >> +{ >> + fpu_idle_fpregs(); > > That's redundant because arch_cpu_idle_enter() is invoked before the > actual idle mechanism. I think the way this series is shaped makes confusion, sorry. Since PATCH3 and PATCH4 are in debate -- which approach should be chosen, it was decided to post both and let just one of them be selected. E.g., if PATCH3 is right, then PATCH4 should be abandoned. I think PATCH3 is better. Maybe PATCH4 should not be sent together to avoid such confusion. Thanks, Chang
On Fri, Nov 05 2021 at 16:03, Bae, Chang Seok wrote: > On Nov 5, 2021, at 07:33, Thomas Gleixner <tglx@linutronix.de> wrote: >> On Thu, Nov 04 2021 at 15:52, Chang S. Bae wrote: >>> +static __cpuidle int intel_idle_tile(struct cpuidle_device *dev, >>> + struct cpuidle_driver *drv, int index) >>> +{ >>> + fpu_idle_fpregs(); >> >> That's redundant because arch_cpu_idle_enter() is invoked before the >> actual idle mechanism. > > I think the way this series is shaped makes confusion, sorry. > > Since PATCH3 and PATCH4 are in debate -- which approach should be chosen, it > was decided to post both and let just one of them be selected. E.g., if PATCH3 > is right, then PATCH4 should be abandoned. My bad. I should have read the cover letter before complaining. > I think PATCH3 is better. Maybe PATCH4 should not be sent together to avoid > such confusion. Yes. patch 3 is way better than patch 4. Thanks, tglx
On 11/5/2021 10:26 AM, Thomas Gleixner wrote: > On Fri, Nov 05 2021 at 16:03, Bae, Chang Seok wrote: > > My bad. I should have read the cover letter before complaining. > >> I think PATCH3 is better. Maybe PATCH4 should not be sent together to avoid >> such confusion. > > Yes. patch 3 is way better than patch 4. At the time of this post, it was clueless whether this TILERELEASE need is architectural or implementation-specific. But the latter is the case as it turns out. So PATCH4 only appears in V2 here -- https://lore.kernel.org/lkml/20220309223431.26560-1-chang.seok.bae@intel.com/ Thanks, Chang
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index e6c543b5ee1d..0ac7dc9e6d51 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -54,6 +54,7 @@ #include <asm/intel-family.h> #include <asm/mwait.h> #include <asm/msr.h> +#include <asm/fpu/api.h> #define INTEL_IDLE_VERSION "0.5.1" @@ -155,6 +156,39 @@ static __cpuidle int intel_idle_s2idle(struct cpuidle_device *dev, return 0; } +/** + * intel_idle_tile - Ask the processor to enter the given idle state. + * @dev: cpuidle device of the target CPU. + * @drv: cpuidle driver (assumed to point to intel_idle_driver). + * + * Ensure TILE registers in INIT-state before using intel_idle() to + * enter the idle state. + */ +static __cpuidle int intel_idle_tile(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + fpu_idle_fpregs(); + + return intel_idle(dev, drv, index); +} + +/** + * intel_idle_s2idle_tile - Ask the processor to enter the given idle state. + * @dev: cpuidle device of the target CPU. + * @drv: cpuidle driver (assumed to point to intel_idle_driver). + * @index: Target idle state index. + * + * Ensure TILE registers in INIT-state before using intel_idle_s2idle() to + * enter the idle state. + */ +static __cpuidle int intel_idle_s2idle_tile(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + fpu_idle_fpregs(); + + return intel_idle_s2idle(dev, drv, index); +} + /* * States are indexed by the cstate number, * which is also the index into the MWAIT hint array. @@ -752,6 +786,27 @@ static struct cpuidle_state icx_cstates[] __initdata = { .enter = NULL } }; +static struct cpuidle_state spr_cstates[] __initdata = { + { + .name = "C1", + .desc = "MWAIT 0x00", + .flags = MWAIT2flg(0x00), + .exit_latency = 1, + .target_residency = 1, + .enter = &intel_idle, + .enter_s2idle = intel_idle_s2idle, }, + { + .name = "C6", + .desc = "MWAIT 0x20", + .flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED, + .exit_latency = 128, + .target_residency = 384, + .enter = &intel_idle_tile, + .enter_s2idle = intel_idle_s2idle_tile, }, + { + .enter = NULL } +}; + static struct cpuidle_state atom_cstates[] __initdata = { { .name = "C1E", @@ -1095,6 +1150,12 @@ static const struct idle_cpu idle_cpu_icx __initconst = { .use_acpi = true, }; +static const struct idle_cpu idle_cpu_spr __initconst = { + .state_table = spr_cstates, + .disable_promotion_to_c1e = true, + .use_acpi = true, +}; + static const struct idle_cpu idle_cpu_avn __initconst = { .state_table = avn_cstates, .disable_promotion_to_c1e = true, @@ -1157,6 +1218,7 @@ static const struct x86_cpu_id intel_idle_ids[] __initconst = { X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X, &idle_cpu_skx), X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X, &idle_cpu_icx), X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_D, &idle_cpu_icx), + X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X, &idle_cpu_spr), X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNL, &idle_cpu_knl), X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNM, &idle_cpu_knl), X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT, &idle_cpu_bxt),
Add a custom Sapphire Rapids C-state table to intel_idle driver. The parameters in the table are preferred over those supplied by ACPI. SPR supports AMX, and so this custom table uses idle entry points that know how to initialize AMX state if necessary. This guarantees that AMX state will never be the cause of hardware C-state demotion from C6 to C1E. Under some conditions, this may result in improved power savings and thus a higher available turbo frequency budget. [ Based on patch by Artem Bityutskiy <artem.bityutskiy@linux.intel.com>. ] Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com> Cc: x86@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-pm@vger.kernel.org --- drivers/idle/intel_idle.c | 62 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)