Ravishankar Sarawadi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 Cstate Demotion ......................................................................
soc/intel/tigerlake: Disable C1 Cstate Demotion
Disable C1 state auto demotion to improve SoC power.
When set, processor will conditionally demote C3/C6/C7 requests to C1 based on uncore auto-demote information.
BUG=b:161215906 TEST=Measure and confirm power usage reduction for key use cases eg 'Google Meets video call' Signed-off-by: Ravi Sarawadi ravishankar.sarawadi@intel.com Signed-off-by: Shweta Malik shweta.malik@intel.com Change-Id: I649cafbaf03917d76521aa5f76ec58d218e1a1b1 --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/46438/1
diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c index 38f444b..f9f17f8 100644 --- a/src/soc/intel/tigerlake/fsp_params.c +++ b/src/soc/intel/tigerlake/fsp_params.c @@ -357,6 +357,10 @@
/* EnableMultiPhaseSiliconInit for running MultiPhaseSiInit */ params->EnableMultiPhaseSiliconInit = 1; + + /* Disable C1 Cstate Demotion */ + params->C1StateAutoDemotion = 0; + mainboard_silicon_init_params(params); }
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 Cstate Demotion ......................................................................
Patch Set 1: Code-Review+2
Do we need to check that auto-demotion is enabled first?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 Cstate Demotion ......................................................................
Patch Set 1: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 Cstate Demotion ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+2
Do we need to check that auto-demotion is enabled first?
or will FSP take care of the check first?
Ravishankar Sarawadi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 Cstate Demotion ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1: Code-Review+2
Do we need to check that auto-demotion is enabled first?
or will FSP take care of the check first?
We have to override unconditionally.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 Cstate Demotion ......................................................................
Patch Set 1: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 Cstate Demotion ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/46438/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46438/1//COMMIT_MSG@7 PS1, Line 7: Cstate C-State
https://review.coreboot.org/c/coreboot/+/46438/1//COMMIT_MSG@9 PS1, Line 9: state C-State
https://review.coreboot.org/c/coreboot/+/46438/1//COMMIT_MSG@9 PS1, Line 9: improve SoC power decrease SoC power usage
https://review.coreboot.org/c/coreboot/+/46438/1//COMMIT_MSG@12 PS1, Line 12: to C1 based on uncore auto-demote information. I wonder, what the autodemotion is useful for.
https://review.coreboot.org/c/coreboot/+/46438/1//COMMIT_MSG@16 PS1, Line 16: eg 'Google Meets video call' Please give concrete numbers, and how that was measured.
https://review.coreboot.org/c/coreboot/+/46438/1/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/46438/1/src/soc/intel/tigerlake/fsp... PS1, Line 361: Cstate C-State
Hello build bot (Jenkins), Tim Wawrzynczak, Nick Vaccaro, Tim Wawrzynczak, Raj Astekar, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46438
to look at the new patch set (#2).
Change subject: soc/intel/tigerlake: Disable C1 C-state Demotion ......................................................................
soc/intel/tigerlake: Disable C1 C-state Demotion
Disable C1 C-state auto demotion to decrease SoC power usage.
When set, processor will conditionally demote C3/C6/C7 requests to C1 based on uncore auto-demote information.
BUG=b:161215906 TEST=Measure and confirm power usage reduction for key use cases eg 'Google Meets video call' Signed-off-by: Ravi Sarawadi ravishankar.sarawadi@intel.com Signed-off-by: Shweta Malik shweta.malik@intel.com Change-Id: I649cafbaf03917d76521aa5f76ec58d218e1a1b1 --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/46438/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 C-state Demotion ......................................................................
Patch Set 2:
Ravi, you'll need to mark the comments as 'Resolved' in order for this to get merged
Ravishankar Sarawadi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 C-state Demotion ......................................................................
Patch Set 2:
(3 comments)
Yes Tim, I am waiting for PnP Intel team to provide the Test methods and data to be included in Commit msg. Will do that asap.
https://review.coreboot.org/c/coreboot/+/46438/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46438/1//COMMIT_MSG@7 PS1, Line 7: Cstate
C-State
Done
https://review.coreboot.org/c/coreboot/+/46438/1//COMMIT_MSG@9 PS1, Line 9: improve SoC power
decrease SoC power usage
Done
https://review.coreboot.org/c/coreboot/+/46438/1//COMMIT_MSG@9 PS1, Line 9: state
C-State
Done
Ravishankar Sarawadi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 C-state Demotion ......................................................................
Patch Set 2:
Team is working on gathering the test info, I will update when available.
Hello build bot (Jenkins), Tim Wawrzynczak, Nick Vaccaro, Tim Wawrzynczak, Raj Astekar, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46438
to look at the new patch set (#4).
Change subject: soc/intel/tigerlake: Disable C1 C-state Demotion ......................................................................
soc/intel/tigerlake: Disable C1 C-state Demotion
Disable C1 C-state auto demotion to decrease SoC power usage.
When set, processor will conditionally demote C3/C6/C7 requests to C1 based on uncore auto-demote information.
BUG=b:161215906 TEST=Measure and confirm SoC power usage reduction for key use cases eg 'Google Meets video call'
Measured on instrumented boards for Volteer EVT and Delbin.
Below measurements for Volteer:
Google meets with 720p w/ auto-demotion w/o auto-demotion System Power 13.14W 9.4W SOC Power 7.9W 5.4W
Signed-off-by: Ravi Sarawadi ravishankar.sarawadi@intel.com Signed-off-by: Shweta Malik shweta.malik@intel.com Change-Id: I649cafbaf03917d76521aa5f76ec58d218e1a1b1 --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/46438/4
Ravishankar Sarawadi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 C-state Demotion ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46438/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46438/1//COMMIT_MSG@12 PS1, Line 12: to C1 based on uncore auto-demote information.
I wonder, what the autodemotion is useful for.
Auto-demotion conditionally demote the C7 logic to C1. This is specifically a performance feature. Currently, by enabling this bit, we are seeing no effect on performance, but power for google meets KPI is impacted. Hence, we disabled this bit as an interim solution.
https://review.coreboot.org/c/coreboot/+/46438/1//COMMIT_MSG@16 PS1, Line 16: eg 'Google Meets video call'
Please give concrete numbers, and how that was measured.
Done
Ravishankar Sarawadi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 C-state Demotion ......................................................................
Patch Set 4:
Could we get it merged if no more review comments? Also needs to be cherrypicked into firmware-volteer-13521.B
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 C-state Demotion ......................................................................
Patch Set 4:
Still an outstanding comment to resolve: change the comment (now on line 369) to ``` /* Disable C1 C-state Demotion */ ```
then mark the comment as resolved
Hello build bot (Jenkins), Tim Wawrzynczak, Nick Vaccaro, Tim Wawrzynczak, Raj Astekar, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46438
to look at the new patch set (#5).
Change subject: soc/intel/tigerlake: Disable C1 C-state Demotion ......................................................................
soc/intel/tigerlake: Disable C1 C-state Demotion
Disable C1 C-state auto demotion to decrease SoC power usage.
When set, processor will conditionally demote C3/C6/C7 requests to C1 based on uncore auto-demote information.
BUG=b:161215906 TEST=Measure and confirm SoC power usage reduction for key use cases eg 'Google Meets video call'
Measured on instrumented boards for Volteer EVT and Delbin.
Below measurements for Volteer:
Google meets with 720p w/ auto-demotion w/o auto-demotion System Power 13.14W 9.4W SOC Power 7.9W 5.4W
Signed-off-by: Ravi Sarawadi ravishankar.sarawadi@intel.com Signed-off-by: Shweta Malik shweta.malik@intel.com Change-Id: I649cafbaf03917d76521aa5f76ec58d218e1a1b1 --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 4 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/46438/5
Ravishankar Sarawadi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 C-state Demotion ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46438/1/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/46438/1/src/soc/intel/tigerlake/fsp... PS1, Line 361: Cstate
C-State
Done
Hello build bot (Jenkins), Tim Wawrzynczak, Nick Vaccaro, Tim Wawrzynczak, Raj Astekar, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46438
to look at the new patch set (#6).
Change subject: soc/intel/tigerlake: Disable C1 C-state Demotion ......................................................................
soc/intel/tigerlake: Disable C1 C-state Demotion
Disable C1 C-state auto demotion to decrease SoC power usage.
When set, processor will conditionally demote C3/C6/C7 requests to C1 based on uncore auto-demote information.
BUG=b:161215906 TEST=Measure and confirm SoC power usage reduction for key use cases eg 'Google Meets video call'
Measured on instrumented boards for Volteer EVT and Delbin.
Below measurements for Volteer:
Google meets with 720p w/ auto-demotion w/o auto-demotion System Power 13.14W 9.4W SOC Power 7.9W 5.4W
Signed-off-by: Ravi Sarawadi ravishankar.sarawadi@intel.com Signed-off-by: Shweta Malik shweta.malik@intel.com Change-Id: I649cafbaf03917d76521aa5f76ec58d218e1a1b1 --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/46438/6
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 C-state Demotion ......................................................................
Patch Set 6: Code-Review+2
Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46438 )
Change subject: soc/intel/tigerlake: Disable C1 C-state Demotion ......................................................................
soc/intel/tigerlake: Disable C1 C-state Demotion
Disable C1 C-state auto demotion to decrease SoC power usage.
When set, processor will conditionally demote C3/C6/C7 requests to C1 based on uncore auto-demote information.
BUG=b:161215906 TEST=Measure and confirm SoC power usage reduction for key use cases eg 'Google Meets video call'
Measured on instrumented boards for Volteer EVT and Delbin.
Below measurements for Volteer:
Google meets with 720p w/ auto-demotion w/o auto-demotion System Power 13.14W 9.4W SOC Power 7.9W 5.4W
Signed-off-by: Ravi Sarawadi ravishankar.sarawadi@intel.com Signed-off-by: Shweta Malik shweta.malik@intel.com Change-Id: I649cafbaf03917d76521aa5f76ec58d218e1a1b1 Reviewed-on: https://review.coreboot.org/c/coreboot/+/46438 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 4 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c index 887241b..3427a61 100644 --- a/src/soc/intel/tigerlake/fsp_params.c +++ b/src/soc/intel/tigerlake/fsp_params.c @@ -365,6 +365,10 @@
/* EnableMultiPhaseSiliconInit for running MultiPhaseSiInit */ params->EnableMultiPhaseSiliconInit = 1; + + /* Disable C1 C-state Demotion */ + params->C1StateAutoDemotion = 0; + mainboard_silicon_init_params(params); }
-
Paul Menzel (Code Review) -
Ravishankar Sarawadi (Code Review)
-
Tim Wawrzynczak (Code Review)