Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42505 )
Change subject: libpayload: Initialize video console conditionally ......................................................................
libpayload: Initialize video console conditionally
Initialize video console only if LP_VIDEO_CONSOLE is set.
BRANCH=none BUG=none TEST=USE="menu_ui" emerge-gale depthcharge
Change-Id: Ic45f9073330258cb77301003484ec525b2404180 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/gdb/stub.c 1 file changed, 4 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/42505/1
diff --git a/payloads/libpayload/gdb/stub.c b/payloads/libpayload/gdb/stub.c index 694577e..395579b4 100644 --- a/payloads/libpayload/gdb/stub.c +++ b/payloads/libpayload/gdb/stub.c @@ -73,9 +73,11 @@ if (!gdb_state.resumed) { /* Must be a die_if() in GDB (or a bug), so bail out and die. */ gdb_exit(-1); - video_console_init(); puts("GDB died, redirecting its last words to the screen:\n"); - console_write(buffer, count); + if (CONFIG(LP_VIDEO_CONSOLE)) { + video_console_init(); + console_write(buffer, count); + } } else { reply.used = 0; reply.buf[reply.used++] = 'O';
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42505 )
Change subject: libpayload: Initialize video console conditionally ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
LGTM, but might be good to have another coreboot.org contributor have a look.
https://review.coreboot.org/c/coreboot/+/42505/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42505/1//COMMIT_MSG@13 PS1, Line 13: TEST Just to confirm, does this let us compile depthcharge without defining video_console_init?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42505 )
Change subject: libpayload: Initialize video console conditionally ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42505/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42505/1//COMMIT_MSG@7 PS1, Line 7: libpayload: Initialize video console conditionally Maybe:
Condition video console init on LP_VIDEO_CONSOLE
https://review.coreboot.org/c/coreboot/+/42505/1//COMMIT_MSG@9 PS1, Line 9: Initialize video console only if LP_VIDEO_CONSOLE is set. What problem does this solve?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42505 )
Change subject: libpayload: Initialize video console conditionally ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42505/1/payloads/libpayload/gdb/stu... File payloads/libpayload/gdb/stub.c:
https://review.coreboot.org/c/coreboot/+/42505/1/payloads/libpayload/gdb/stu... PS1, Line 76: puts("GDB died, redirecting its last words to the screen:\n"); No, this is supposed to be visible on the screen, so it needs to come after video_console_init(). (It's okay to move it into that conditional.)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42505 )
Change subject: libpayload: Initialize video console conditionally ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42505/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42505/1//COMMIT_MSG@7 PS1, Line 7: libpayload: Initialize video console conditionally
Maybe: […]
While you guys are updating the subject it should probably say 'libpayload: gdb: ' or something to clarify that this only affects GDB.
Hello build bot (Jenkins), Joel Kitching, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42505
to look at the new patch set (#2).
Change subject: libpayload: gdb: Condition video console init on LP_VIDEO_CONSOLE ......................................................................
libpayload: gdb: Condition video console init on LP_VIDEO_CONSOLE
With the stub video_console_init() removed from depthcharge in CL:2241493, depthcharge will fail to compile:
payloads/libpayload/gdb/stub.c:76: undefined reference to `video_console_init'
Since video_console_init() is meant to be implemented in libpayload, libpayload should be consistent with itself by not calling this function when it's not implemented (i.e., when !LP_VIDEO_CONSOLE).
Therefore, initialize video console only if LP_VIDEO_CONSOLE is set.
BRANCH=none BUG=none TEST=USE="menu_ui" emerge-gale depthcharge
Change-Id: Ic45f9073330258cb77301003484ec525b2404180 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/gdb/stub.c 1 file changed, 5 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/42505/2
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42505 )
Change subject: libpayload: gdb: Condition video console init on LP_VIDEO_CONSOLE ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42505/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42505/1//COMMIT_MSG@7 PS1, Line 7: libpayload: Initialize video console conditionally
While you guys are updating the subject it should probably say 'libpayload: gdb: ' or something to c […]
Done
https://review.coreboot.org/c/coreboot/+/42505/1//COMMIT_MSG@9 PS1, Line 9: Initialize video console only if LP_VIDEO_CONSOLE is set.
What problem does this solve?
Done
https://review.coreboot.org/c/coreboot/+/42505/1//COMMIT_MSG@13 PS1, Line 13: TEST
Just to confirm, does this let us compile depthcharge without defining video_console_init?
Yes. If this is what you're asking, together with CL:2241493,
emerge-gale depthcharge
succeeded.
https://review.coreboot.org/c/coreboot/+/42505/1/payloads/libpayload/gdb/stu... File payloads/libpayload/gdb/stub.c:
https://review.coreboot.org/c/coreboot/+/42505/1/payloads/libpayload/gdb/stu... PS1, Line 76: puts("GDB died, redirecting its last words to the screen:\n");
No, this is supposed to be visible on the screen, so it needs to come after video_console_init(). […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42505 )
Change subject: libpayload: gdb: Condition video console init on LP_VIDEO_CONSOLE ......................................................................
Patch Set 2: Code-Review+1
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42505 )
Change subject: libpayload: gdb: Condition video console init on LP_VIDEO_CONSOLE ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42505/1/payloads/libpayload/gdb/stu... File payloads/libpayload/gdb/stub.c:
https://review.coreboot.org/c/coreboot/+/42505/1/payloads/libpayload/gdb/stu... PS1, Line 76: puts("GDB died, redirecting its last words to the screen:\n");
Done
Unless I'm mistaken, puts/console_write will write to all available output drivers, including video and serial. By putting the puts() and console_write() calls in the conditional, the strings will no longer be printed to the serial console (or any other connected output drivers). Is that okay? (Not even sure if the serial console will be able to output anything if GDB died anyways...)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42505 )
Change subject: libpayload: gdb: Condition video console init on LP_VIDEO_CONSOLE ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42505/1/payloads/libpayload/gdb/stu... File payloads/libpayload/gdb/stub.c:
https://review.coreboot.org/c/coreboot/+/42505/1/payloads/libpayload/gdb/stu... PS1, Line 76: puts("GDB died, redirecting its last words to the screen:\n");
Unless I'm mistaken, puts/console_write will write to all available output drivers, including video […]
Yes, but I don't think it matters. I don't recall exactly how all of this works but I think in practice, even though gdb_exit() tries to reenable the serial you're usually not gonna see it (I think because the UART output still goes to the gdb process on the host machine at this point, who thinks that's just some garbage data that doesn't match its usual remote protocol it is expecting and isn't going to display it to the user). At least the whole point of turning on the screen here is that you wouldn't see it on the UART if just printing it there, I think. So this should be fine.
I mean, then again, it should also be fine to just do
if (CONFIG(LP_VIDEO_CONSOLE)) video_console_init(); puts(...) console_write(...)
if you prefer to keep the old behavior as close as possible.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42505 )
Change subject: libpayload: gdb: Condition video console init on LP_VIDEO_CONSOLE ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42505/1/payloads/libpayload/gdb/stu... File payloads/libpayload/gdb/stub.c:
https://review.coreboot.org/c/coreboot/+/42505/1/payloads/libpayload/gdb/stu... PS1, Line 76: puts("GDB died, redirecting its last words to the screen:\n");
Yes, but I don't think it matters. […]
I have a slight preference for the way you've written it here, since it makes less assumptions about the output drivers. But you're right -- it shouldn't make a difference.
Hello build bot (Jenkins), Joel Kitching, Paul Menzel, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42505
to look at the new patch set (#3).
Change subject: libpayload: gdb: Condition video console init on LP_VIDEO_CONSOLE ......................................................................
libpayload: gdb: Condition video console init on LP_VIDEO_CONSOLE
With the stub video_console_init() removed from depthcharge in CL:2241493, depthcharge will fail to compile:
payloads/libpayload/gdb/stub.c:76: undefined reference to `video_console_init'
Since video_console_init() is meant to be implemented in libpayload, libpayload should be consistent with itself by not calling this function when it's not implemented (i.e., when !LP_VIDEO_CONSOLE).
Therefore, initialize video console only if LP_VIDEO_CONSOLE is set.
BRANCH=none BUG=none TEST=USE="menu_ui" emerge-gale depthcharge
Change-Id: Ic45f9073330258cb77301003484ec525b2404180 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/gdb/stub.c 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/42505/3
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42505 )
Change subject: libpayload: gdb: Condition video console init on LP_VIDEO_CONSOLE ......................................................................
Patch Set 3: Code-Review+2
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42505 )
Change subject: libpayload: gdb: Condition video console init on LP_VIDEO_CONSOLE ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42505/1/payloads/libpayload/gdb/stu... File payloads/libpayload/gdb/stub.c:
https://review.coreboot.org/c/coreboot/+/42505/1/payloads/libpayload/gdb/stu... PS1, Line 76: puts("GDB died, redirecting its last words to the screen:\n");
I have a slight preference for the way you've written it here, since it makes less assumptions about […]
Moved puts call out of the conditional.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42505 )
Change subject: libpayload: gdb: Condition video console init on LP_VIDEO_CONSOLE ......................................................................
libpayload: gdb: Condition video console init on LP_VIDEO_CONSOLE
With the stub video_console_init() removed from depthcharge in CL:2241493, depthcharge will fail to compile:
payloads/libpayload/gdb/stub.c:76: undefined reference to `video_console_init'
Since video_console_init() is meant to be implemented in libpayload, libpayload should be consistent with itself by not calling this function when it's not implemented (i.e., when !LP_VIDEO_CONSOLE).
Therefore, initialize video console only if LP_VIDEO_CONSOLE is set.
BRANCH=none BUG=none TEST=USE="menu_ui" emerge-gale depthcharge
Change-Id: Ic45f9073330258cb77301003484ec525b2404180 Signed-off-by: Yu-Ping Wu yupingso@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/42505 Reviewed-by: Joel Kitching kitching@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M payloads/libpayload/gdb/stub.c 1 file changed, 2 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Joel Kitching: Looks good to me, approved
diff --git a/payloads/libpayload/gdb/stub.c b/payloads/libpayload/gdb/stub.c index 694577e..019f27f 100644 --- a/payloads/libpayload/gdb/stub.c +++ b/payloads/libpayload/gdb/stub.c @@ -73,7 +73,8 @@ if (!gdb_state.resumed) { /* Must be a die_if() in GDB (or a bug), so bail out and die. */ gdb_exit(-1); - video_console_init(); + if (CONFIG(LP_VIDEO_CONSOLE)) + video_console_init(); puts("GDB died, redirecting its last words to the screen:\n"); console_write(buffer, count); } else {
-
Joel Kitching (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Yu-Ping Wu (Code Review)