Change in ...coreboot[master]: libpayload: cbgfx: Run cbgfx_init() before we need it for draw_box()
Hello Daisuke Nojiri, Shelley Chen,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/33855
to review the following change.
Change subject: libpayload: cbgfx: Run cbgfx_init() before we need it for draw_box() ......................................................................
libpayload: cbgfx: Run cbgfx_init() before we need it for draw_box()
calculate_color() uses the 'fbinfo' global that is initialized by cbgfx_init(), so we need to run the latter before we can run the former or we get a null pointer access.
Change-Id: I73ca8e20ca36f64d699379d504fd41dc2084f157 Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/drivers/video/graphics.c 1 file changed, 4 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/33855/1
diff --git a/payloads/libpayload/drivers/video/graphics.c b/payloads/libpayload/drivers/video/graphics.c index 4956326..85a8642 100644 --- a/payloads/libpayload/drivers/video/graphics.c +++ b/payloads/libpayload/drivers/video/graphics.c @@ -183,6 +183,10 @@ struct vector top_left; struct vector size; struct vector p, t; + + if (cbgfx_init()) + return CBGFX_ERROR_INIT; + const uint32_t color = calculate_color(rgb, 0); const struct scale top_left_s = { .x = { .n = box->offset.x, .d = CANVAS_SCALE, }, @@ -193,9 +197,6 @@ .y = { .n = box->size.y, .d = CANVAS_SCALE, } };
- if (cbgfx_init()) - return CBGFX_ERROR_INIT; - transform_vector(&top_left, &canvas.size, &top_left_s, &canvas.offset); transform_vector(&size, &canvas.size, &size_s, &vzero); add_vectors(&t, &top_left, &size);
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33855 )
Change subject: libpayload: cbgfx: Run cbgfx_init() before we need it for draw_box() ......................................................................
Patch Set 1: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33855 )
Change subject: libpayload: cbgfx: Run cbgfx_init() before we need it for draw_box() ......................................................................
Patch Set 1: Code-Review+1
Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33855 )
Change subject: libpayload: cbgfx: Run cbgfx_init() before we need it for draw_box() ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/33855/1/payloads/libpayload/drivers/video/gr... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/#/c/33855/1/payloads/libpayload/drivers/video/gr... PS1, Line 190: const uint32_t color You don't get "declaration must be at beginning of a block" error?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33855 )
Change subject: libpayload: cbgfx: Run cbgfx_init() before we need it for draw_box() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33855/1/payloads/libpayload/drivers/video/gr... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/#/c/33855/1/payloads/libpayload/drivers/video/gr... PS1, Line 190: const uint32_t color
You don't get "declaration must be at beginning of a block" error?
libpayload is built with -std=gnu11, so no.
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33855 )
Change subject: libpayload: cbgfx: Run cbgfx_init() before we need it for draw_box() ......................................................................
libpayload: cbgfx: Run cbgfx_init() before we need it for draw_box()
calculate_color() uses the 'fbinfo' global that is initialized by cbgfx_init(), so we need to run the latter before we can run the former or we get a null pointer access.
Change-Id: I73ca8e20ca36f64d699379d504fd41dc2084f157 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/33855 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Daisuke Nojiri dnojiri@chromium.org --- M payloads/libpayload/drivers/video/graphics.c 1 file changed, 4 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Daisuke Nojiri: Looks good to me, approved Patrick Rudolph: Looks good to me, approved
diff --git a/payloads/libpayload/drivers/video/graphics.c b/payloads/libpayload/drivers/video/graphics.c index 4956326..85a8642 100644 --- a/payloads/libpayload/drivers/video/graphics.c +++ b/payloads/libpayload/drivers/video/graphics.c @@ -183,6 +183,10 @@ struct vector top_left; struct vector size; struct vector p, t; + + if (cbgfx_init()) + return CBGFX_ERROR_INIT; + const uint32_t color = calculate_color(rgb, 0); const struct scale top_left_s = { .x = { .n = box->offset.x, .d = CANVAS_SCALE, }, @@ -193,9 +197,6 @@ .y = { .n = box->size.y, .d = CANVAS_SCALE, } };
- if (cbgfx_init()) - return CBGFX_ERROR_INIT; - transform_vector(&top_left, &canvas.size, &top_left_s, &canvas.offset); transform_vector(&size, &canvas.size, &size_s, &vzero); add_vectors(&t, &top_left, &size);
-
Daisuke Nojiri (Code Review) -
Julius Werner (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)