Change in coreboot[master]: mb/ocp/deltalake: Disable ISOC vi UPD to improve MLC performance
Johnny Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48738 )
Change subject: mb/ocp/deltalake: Disable ISOC vi UPD to improve MLC performance ......................................................................
mb/ocp/deltalake: Disable ISOC vi UPD to improve MLC performance
UEFI disables ISOC, after disabling it MLC (memory latency checker) can have the same performance as UEFI.
Tested=On OCP Delta Lake, mlc performance is improved and on par with UEFI BIOS.
Change-Id: I08c22ee001b601e607452b3f23fad969ecb484b4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/mainboard/ocp/deltalake/romstage.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/48738/1
diff --git a/src/mainboard/ocp/deltalake/romstage.c b/src/mainboard/ocp/deltalake/romstage.c index f0cdd3d..9d83d1c 100644 --- a/src/mainboard/ocp/deltalake/romstage.c +++ b/src/mainboard/ocp/deltalake/romstage.c @@ -75,6 +75,7 @@ printk(BIOS_DEBUG, "Setting MemRefreshWatermark %d from VPD\n", val); mupd->FspmConfig.UnusedUpdSpace0[0] = val; } + mupd->FspmConfig.isocEn = 0; }
/* Update bifurcation settings according to different Configs */
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48738 )
Change subject: mb/ocp/deltalake: Disable ISOC vi UPD to improve MLC performance ......................................................................
Patch Set 1:
Should we make the setting in soc, instead of mb?
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48738 )
Change subject: soc/intel/xeon_sp/cpx: Disable ISOC via UPD to improve MLC performance ......................................................................
Patch Set 2:
Patch Set 1:
Should we make the setting in soc, instead of mb?
Yes, done.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48738 )
Change subject: soc/intel/xeon_sp/cpx: Disable ISOC via UPD to improve MLC performance ......................................................................
Patch Set 2: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48738 )
Change subject: soc/intel/xeon_sp/cpx: Disable ISOC via UPD to improve MLC performance ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48738/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48738/2//COMMIT_MSG@10 PS2, Line 10: can have the same performance as UEFI. Please add the concrete numbers.
https://review.coreboot.org/c/coreboot/+/48738/2//COMMIT_MSG@13 PS2, Line 13: with UEFI BIOS. What utility is used for the test?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48738 )
Change subject: soc/intel/xeon_sp/cpx: Disable ISOC via UPD to improve MLC performance ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/48738/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48738/2//COMMIT_MSG@7 PS2, Line 7: soc/intel/xeon_sp/cpx: Disable ISOC via UPD to improve MLC performance Even I find this commit message hard to understand. I would rewrite it as follows:
soc/intel/xeon_sp/cpx: Disable isoch operation for performance
Isochronous operation negatively impacts memory performance, as per Intel MLC (Memory Latency Checker) benchmark results. Thus, disable isochronous operation, like analogous UEFI firmware does.
TEST=On OCP Delta Lake, verify that MLC benchmark results have improved and are now on par with analogous UEFI firmware.
https://review.coreboot.org/c/coreboot/+/48738/2//COMMIT_MSG@13 PS2, Line 13: with UEFI BIOS.
What utility is used for the test?
MLC (memory latency checker) is the tool used for testing, but I had to look it up.
https://review.coreboot.org/c/coreboot/+/48738/2/src/soc/intel/xeon_sp/cpx/r... File src/soc/intel/xeon_sp/cpx/romstage.c:
https://review.coreboot.org/c/coreboot/+/48738/2/src/soc/intel/xeon_sp/cpx/r... PS2, Line 166: Disable ISOC Consider mentioning why, e.g.: `Disable isochronous operation to improve performance`
Hello build bot (Jenkins), Marc Jones, Anjaneya "Reddy" Chagam, Jonathan Zhang, Jingle Hsu, Angel Pons, Morgan Jang, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48738
to look at the new patch set (#3).
Change subject: soc/intel/xeon_sp/cpx: Disable isoch operation for performance ......................................................................
soc/intel/xeon_sp/cpx: Disable isoch operation for performance
Isochronous operation negatively impacts memory performance, as per Intel MLC (Memory Latency Checker) benchmark results. Thus, disable isochronous operation, like analogous UEFI firmware does. The MLC results after disabling isoch: "--max_bandwidth" ALL Reads : 106948.17 3:1 Reads-Writes : 101580.46 2:1 Reads-Writes : 100523.26 1:1 Reads-Writes : 99059.44 Stream-triad like : 97762.47
"--peak_injection_bandwidth" ALL Reads : 105724.3 3:1 Reads-Writes : 100655.8 2:1 Reads-Writes : 99463 1:1 Reads-Writes : 98708 Stream-triad like : 91515
TEST=On OCP Delta Lake, verify that MLC benchmark results have improved and are now on par with analogous UEFI firmware. Change-Id: I08c22ee001b601e607452b3f23fad969ecb484b4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/soc/intel/xeon_sp/cpx/romstage.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/48738/3
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48738 )
Change subject: soc/intel/xeon_sp/cpx: Disable isoch operation for performance ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/48738/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48738/2//COMMIT_MSG@7 PS2, Line 7: soc/intel/xeon_sp/cpx: Disable ISOC via UPD to improve MLC performance
Even I find this commit message hard to understand. I would rewrite it as follows: […]
Done
https://review.coreboot.org/c/coreboot/+/48738/2//COMMIT_MSG@10 PS2, Line 10: can have the same performance as UEFI.
Please add the concrete numbers.
Done
https://review.coreboot.org/c/coreboot/+/48738/2//COMMIT_MSG@13 PS2, Line 13: with UEFI BIOS.
MLC (memory latency checker) is the tool used for testing, but I had to look it up.
Yes, added to comment Intel memory latency checker.
https://review.coreboot.org/c/coreboot/+/48738/2/src/soc/intel/xeon_sp/cpx/r... File src/soc/intel/xeon_sp/cpx/romstage.c:
https://review.coreboot.org/c/coreboot/+/48738/2/src/soc/intel/xeon_sp/cpx/r... PS2, Line 166: Disable ISOC
Consider mentioning why, e.g. […]
Since our analogous UEFI firmware has better MLC performance, we just compare the CHA and IMC CSR and found this difference, after aligning to the same configuration the MLC performance is much improved and on par with our UEFI firmware.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48738 )
Change subject: soc/intel/xeon_sp/cpx: Disable isoch operation for performance ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/48738/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48738/3//COMMIT_MSG@10 PS3, Line 10: Intel MLC (Memory Latency Checker) benchmark Thank you. It’s even publically available.
https://software.intel.com/content/www/us/en/develop/articles/intelr-memory-...
https://review.coreboot.org/c/coreboot/+/48738/3//COMMIT_MSG@12 PS3, Line 12: The MLC results after disabling isoch: Please add a blank line above for better legibility.
https://review.coreboot.org/c/coreboot/+/48738/3//COMMIT_MSG@13 PS3, Line 13: "--max_bandwidth" : ALL Reads : 106948.17 : 3:1 Reads-Writes : 101580.46 : 2:1 Reads-Writes : 100523.26 : 1:1 Reads-Writes : 99059.44 : Stream-triad like : 97762.47 : : "--peak_injection_bandwidth" : ALL Reads : 105724.3 : 3:1 Reads-Writes : 100655.8 : 2:1 Reads-Writes : 99463 : 1:1 Reads-Writes : 98708 : Stream-triad like : 91515 Thank you for the giving the after results. Could you please also post the results before the change, so the improvement can be seen?
https://review.coreboot.org/c/coreboot/+/48738/3//COMMIT_MSG@29 PS3, Line 29: Stray character.
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48738 )
Change subject: soc/intel/xeon_sp/cpx: Disable isoch operation for performance ......................................................................
Patch Set 3: Code-Review+2
Hello Marc Jones, build bot (Jenkins), Anjaneya "Reddy" Chagam, Jonathan Zhang, Jingle Hsu, Angel Pons, Morgan Jang, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48738
to look at the new patch set (#4).
Change subject: soc/intel/xeon_sp/cpx: Disable isoch operation for performance ......................................................................
soc/intel/xeon_sp/cpx: Disable isoch operation for performance
Isochronous operation negatively impacts memory performance, as per Intel MLC (Memory Latency Checker) benchmark results. Thus, disable isochronous operation, like analogous UEFI firmware does. The MLC results after disabling isoch:
"--max_bandwidth" ALL Reads : 106948.17 3:1 Reads-Writes : 101580.46 2:1 Reads-Writes : 100523.26 1:1 Reads-Writes : 99059.44 Stream-triad like : 97762.47
"--peak_injection_bandwidth" ALL Reads : 105724.3 3:1 Reads-Writes : 100655.8 2:1 Reads-Writes : 99463 1:1 Reads-Writes : 98708 Stream-triad like : 91515
The MLC results before disabling isoch:
"--max_bandwidth" ALL Reads : 88824.96 3:1 Reads-Writes : 94820.81 2:1 Reads-Writes : 94867.53 1:1 Reads-Writes : 92567.36 Stream-triad like : 91900.43
"--peak_injection_bandwidth" ALL Reads : 88859.6 3:1 Reads-Writes : 94064 2:1 Reads-Writes : 94186.2 1:1 Reads-Writes : 92516.1 Stream-triad like : 85147.4
TEST=On OCP Delta Lake, verify that MLC benchmark results have improved and are now on par with analogous UEFI firmware.
Change-Id: I08c22ee001b601e607452b3f23fad969ecb484b4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/soc/intel/xeon_sp/cpx/romstage.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/48738/4
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48738 )
Change subject: soc/intel/xeon_sp/cpx: Disable isoch operation for performance ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/48738/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48738/3//COMMIT_MSG@12 PS3, Line 12: The MLC results after disabling isoch:
Please add a blank line above for better legibility.
Done
https://review.coreboot.org/c/coreboot/+/48738/3//COMMIT_MSG@13 PS3, Line 13: "--max_bandwidth" : ALL Reads : 106948.17 : 3:1 Reads-Writes : 101580.46 : 2:1 Reads-Writes : 100523.26 : 1:1 Reads-Writes : 99059.44 : Stream-triad like : 97762.47 : : "--peak_injection_bandwidth" : ALL Reads : 105724.3 : 3:1 Reads-Writes : 100655.8 : 2:1 Reads-Writes : 99463 : 1:1 Reads-Writes : 98708 : Stream-triad like : 91515
Thank you for the giving the after results. […]
Done
https://review.coreboot.org/c/coreboot/+/48738/3//COMMIT_MSG@29 PS3, Line 29:
Stray character.
Done
Hello build bot (Jenkins), Marc Jones, Anjaneya "Reddy" Chagam, Jonathan Zhang, Jingle Hsu, Angel Pons, Morgan Jang, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48738
to look at the new patch set (#5).
Change subject: soc/intel/xeon_sp/cpx: Disable isoch operation for performance ......................................................................
soc/intel/xeon_sp/cpx: Disable isoch operation for performance
Isochronous operation negatively impacts memory performance, as per Intel MLC (Memory Latency Checker) benchmark results. Thus, disable isochronous operation, like analogous UEFI firmware does. The MLC results after disabling isoch:
"--max_bandwidth" ALL Reads : 106948.17 3:1 Reads-Writes : 101580.46 2:1 Reads-Writes : 100523.26 1:1 Reads-Writes : 99059.44 Stream-triad like : 97762.47
"--peak_injection_bandwidth" ALL Reads : 105724.3 3:1 Reads-Writes : 100655.8 2:1 Reads-Writes : 99463 1:1 Reads-Writes : 98708 Stream-triad like : 91515
The MLC results before disabling isoch:
"--max_bandwidth" ALL Reads : 88824.96 3:1 Reads-Writes : 94820.81 2:1 Reads-Writes : 94867.53 1:1 Reads-Writes : 92567.36 Stream-triad like : 91900.43
"--peak_injection_bandwidth" ALL Reads : 88859.6 3:1 Reads-Writes : 94064 2:1 Reads-Writes : 94186.2 1:1 Reads-Writes : 92516.1 Stream-triad like : 85147.4
TEST=On OCP Delta Lake, verify that MLC benchmark results have improved.
Change-Id: I08c22ee001b601e607452b3f23fad969ecb484b4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/soc/intel/xeon_sp/cpx/romstage.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/48738/5
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48738 )
Change subject: soc/intel/xeon_sp/cpx: Disable isoch operation for performance ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48738/2/src/soc/intel/xeon_sp/cpx/r... File src/soc/intel/xeon_sp/cpx/romstage.c:
https://review.coreboot.org/c/coreboot/+/48738/2/src/soc/intel/xeon_sp/cpx/r... PS2, Line 166: Disable ISOC
Since our analogous UEFI firmware has better MLC performance, we just compare the CHA and IMC CSR an […]
Ack
Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48738 )
Change subject: soc/intel/xeon_sp/cpx: Disable isoch operation for performance ......................................................................
soc/intel/xeon_sp/cpx: Disable isoch operation for performance
Isochronous operation negatively impacts memory performance, as per Intel MLC (Memory Latency Checker) benchmark results. Thus, disable isochronous operation, like analogous UEFI firmware does. The MLC results after disabling isoch:
"--max_bandwidth" ALL Reads : 106948.17 3:1 Reads-Writes : 101580.46 2:1 Reads-Writes : 100523.26 1:1 Reads-Writes : 99059.44 Stream-triad like : 97762.47
"--peak_injection_bandwidth" ALL Reads : 105724.3 3:1 Reads-Writes : 100655.8 2:1 Reads-Writes : 99463 1:1 Reads-Writes : 98708 Stream-triad like : 91515
The MLC results before disabling isoch:
"--max_bandwidth" ALL Reads : 88824.96 3:1 Reads-Writes : 94820.81 2:1 Reads-Writes : 94867.53 1:1 Reads-Writes : 92567.36 Stream-triad like : 91900.43
"--peak_injection_bandwidth" ALL Reads : 88859.6 3:1 Reads-Writes : 94064 2:1 Reads-Writes : 94186.2 1:1 Reads-Writes : 92516.1 Stream-triad like : 85147.4
TEST=On OCP Delta Lake, verify that MLC benchmark results have improved.
Change-Id: I08c22ee001b601e607452b3f23fad969ecb484b4 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/48738 Reviewed-by: Marc Jones marc@marcjonesconsulting.com Reviewed-by: Jonathan Zhang jonzhang@fb.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/xeon_sp/cpx/romstage.c 1 file changed, 3 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Marc Jones: Looks good to me, approved Jonathan Zhang: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/xeon_sp/cpx/romstage.c b/src/soc/intel/xeon_sp/cpx/romstage.c index 6f47a30..e423d0a 100644 --- a/src/soc/intel/xeon_sp/cpx/romstage.c +++ b/src/soc/intel/xeon_sp/cpx/romstage.c @@ -163,5 +163,8 @@ m_cfg->VtdSupport = config->vtd_support; m_cfg->X2apic = config->x2apic;
+ /* Disable ISOC */ + m_cfg->isocEn = 0; + mainboard_memory_init_params(mupd); }
-
Angel Pons (Code Review) -
Hung-Te Lin (Code Review)
-
Johnny Lin (Code Review)
-
Jonathan Zhang (Code Review)
-
Marc Jones (Code Review)
-
Paul Menzel (Code Review)