Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41392 )
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
libpayload/cbgfx: Remove gap between adjacent boxes
When drawing two adjacent boxes with draw_box(), there will be a gap between them. This is due to the truncation in integer division when calculating the bottom right coordinate of the box.
In this patch, the relative bottom right coordinate is calculated before transforming to an absolute one. The same issue is also fixed for draw_rounded_box().
BRANCH=none BUG=chromium:1082593 TEST=emerge-nami libpayload
Change-Id: I073cf8ec6eb3952a0dcb417b4c3c3c7047567837 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/video/graphics.c 1 file changed, 29 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/41392/1
diff --git a/payloads/libpayload/drivers/video/graphics.c b/payloads/libpayload/drivers/video/graphics.c index ea10f13..8294097 100644 --- a/payloads/libpayload/drivers/video/graphics.c +++ b/payloads/libpayload/drivers/video/graphics.c @@ -72,6 +72,28 @@ return f->d != 0; }
+static void add_fractions(struct fraction *out, + const struct fraction *f1, const struct fraction *f2) +{ + int64_t n, d; + n = (int64_t)f1->n * f2->d + (int64_t)f2->n * f1->d; + d = (int64_t)f1->d * f2->d; + /* Simplest way to reduce the fraction until fitting in int32_t */ + while (d > INT32_MAX || d < INT32_MIN) { + n /= 2; + d /= 2; + } + out->n = n; + out->d = d; +} + +static void add_scales(struct scale *out, + const struct scale *s1, const struct scale *s2) +{ + add_fractions(&out->x, &s1->x, &s2->x); + add_fractions(&out->y, &s1->y, &s2->y); +} + /* * Transform a vector: * x' = x * a_x + offset_x @@ -211,7 +233,6 @@ int draw_box(const struct rect *box, const struct rgb_color *rgb) { struct vector top_left; - struct vector size; struct vector p, t;
if (cbgfx_init()) @@ -222,14 +243,13 @@ .x = { .n = box->offset.x, .d = CANVAS_SCALE, }, .y = { .n = box->offset.y, .d = CANVAS_SCALE, } }; - const struct scale size_s = { - .x = { .n = box->size.x, .d = CANVAS_SCALE, }, - .y = { .n = box->size.y, .d = CANVAS_SCALE, } + const struct scale bottom_right_s = { + .x = { .n = box->offset.x + box->size.x, .d = CANVAS_SCALE, }, + .y = { .n = box->offset.y + box->size.y, .d = CANVAS_SCALE, } };
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); + transform_vector(&t, &canvas.size, &bottom_right_s, &canvas.offset); if (within_box(&t, &canvas) < 0) { LOG("Box exceeds canvas boundary\n"); return CBGFX_ERROR_BOUNDARY; @@ -247,8 +267,8 @@ const struct fraction *thickness, const struct fraction *radius) { + struct scale pos_end_rel; struct vector top_left; - struct vector size; struct vector p, t;
if (cbgfx_init()) @@ -256,9 +276,9 @@
const uint32_t color = calculate_color(rgb, 0);
+ add_scales(&pos_end_rel, pos_rel, dim_rel); transform_vector(&top_left, &canvas.size, pos_rel, &canvas.offset); - transform_vector(&size, &canvas.size, dim_rel, &vzero); - add_vectors(&t, &top_left, &size); + transform_vector(&t, &canvas.size, &pos_end_rel, &canvas.offset); if (within_box(&t, &canvas) < 0) { LOG("Box exceeds canvas boundary\n"); return CBGFX_ERROR_BOUNDARY;
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41392 )
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... PS1, Line 82: while (d > INT32_MAX || d < INT32_MIN) { Faster:
int shift = log2(d >> 31); if (shift > 0) { n >>= shift; d >>= shift; }
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... PS1, Line 233: int draw_box(const struct rect *box, const struct rgb_color *rgb) off-topic, but I think we should try to get rid of this and just only use draw_rounded_box() everywhere at some point (maybe make a draw_box(pos_rel, dim_rel rgb) that's just a thin wrapper which sets thickness and radius to zero).
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41392 )
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... PS1, Line 82: while (d > INT32_MAX || d < INT32_MIN) {
Faster: […]
(Also, maybe make it MAX(n, d)? Is it documented anywhere that struct fraction is only valid for n < d?)
Hello build bot (Jenkins), Joel Kitching, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41392
to look at the new patch set (#2).
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
libpayload/cbgfx: Remove gap between adjacent boxes
When drawing two adjacent boxes with draw_box(), there will be a gap between them. This is due to the truncation in integer division when calculating the bottom right coordinate of the box.
In this patch, the relative bottom right coordinate is calculated before transforming to an absolute one. The same issue is also fixed for draw_rounded_box().
Also check validity of 'pos_rel' and 'dim_rel' arguments for draw_rounded_box().
BRANCH=none BUG=chromium:1082593 TEST=emerge-nami libpayload
Change-Id: I073cf8ec6eb3952a0dcb417b4c3c3c7047567837 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/video/graphics.c 1 file changed, 38 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/41392/2
Hello build bot (Jenkins), Joel Kitching, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41392
to look at the new patch set (#3).
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
libpayload/cbgfx: Remove gap between adjacent boxes
When drawing two adjacent boxes with draw_box(), there will be a gap between them. This is due to the truncation in integer division when calculating the bottom right coordinate of the box.
In this patch, the relative bottom right coordinate is calculated before transforming to an absolute one. The same issue is also fixed for draw_rounded_box().
Also check validity of 'pos_rel' and 'dim_rel' arguments for draw_rounded_box().
BRANCH=none BUG=chromium:1082593 TEST=emerge-nami libpayload
Change-Id: I073cf8ec6eb3952a0dcb417b4c3c3c7047567837 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/video/graphics.c 1 file changed, 43 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/41392/3
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41392 )
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... PS1, Line 82: while (d > INT32_MAX || d < INT32_MIN) {
log2 and MAX(n, d)
Done.
About the right shifting, if (x < 0), then the most significant bit of (x >> shift) seems to be implementation-defined [1].
[1] https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Bit-Shifting
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... PS1, Line 233: int draw_box(const struct rect *box, const struct rgb_color *rgb)
off-topic, but I think we should try to get rid of this and just only use draw_rounded_box() everywh […]
Yeah, I agree.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41392 )
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... PS1, Line 82: while (d > INT32_MAX || d < INT32_MIN) {
log2 and MAX(n, d) […]
I don't see where it says implementation defined on there? It's defined by whether the data type of x is signed or unsigned. In this case, it's signed, so you will shift in a 1 if x < 0 which is exactly what you want.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41392 )
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... PS1, Line 82: while (d > INT32_MAX || d < INT32_MIN) {
new bits added on the left side are usually 0, but if the first operand is a signed negative value, then the added bits will be either 0 or whatever value was previously in the leftmost bit position.
[2] also mentions that the result is implementation-dependent.
[2] https://stackoverflow.com/questions/7622/are-the-shift-operators-arithmetic-...
Hello build bot (Jenkins), Joel Kitching, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41392
to look at the new patch set (#4).
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
libpayload/cbgfx: Remove gap between adjacent boxes
When drawing two adjacent boxes with draw_box(), there will be a gap between them. This is due to the truncation in integer division when calculating the bottom right coordinate of the box.
In this patch, the relative bottom right coordinate is calculated before transforming to an absolute one. The same issue is also fixed for draw_rounded_box().
Also check validity of 'pos_rel' and 'dim_rel' arguments for draw_rounded_box().
BRANCH=none BUG=chromium:1082593 TEST=emerge-nami libpayload
Change-Id: I073cf8ec6eb3952a0dcb417b4c3c3c7047567837 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/video/graphics.c 1 file changed, 43 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/41392/4
Hello build bot (Jenkins), Joel Kitching, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41392
to look at the new patch set (#5).
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
libpayload/cbgfx: Remove gap between adjacent boxes
When drawing two adjacent boxes with draw_box(), there will be a gap between them. This is due to the truncation in integer division when calculating the bottom right coordinate of the box.
In this patch, the relative bottom right coordinate is calculated before transforming to an absolute one. The same issue is also fixed for draw_rounded_box().
Also check validity of 'pos_rel' and 'dim_rel' arguments for draw_rounded_box().
BRANCH=none BUG=chromium:1082593 TEST=emerge-nami libpayload
Change-Id: I073cf8ec6eb3952a0dcb417b4c3c3c7047567837 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/video/graphics.c 1 file changed, 43 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/41392/5
Hello build bot (Jenkins), Joel Kitching, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41392
to look at the new patch set (#6).
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
libpayload/cbgfx: Remove gap between adjacent boxes
When drawing two adjacent boxes with draw_box(), there will be a gap between them. This is due to the truncation in integer division when calculating the bottom right coordinate of the box.
In this patch, the relative bottom right coordinate is calculated before transforming to an absolute one. The same issue is also fixed for draw_rounded_box().
Also check validity of 'pos_rel' and 'dim_rel' arguments for draw_rounded_box().
BRANCH=none BUG=chromium:1082593 TEST=emerge-nami libpayload
Change-Id: I073cf8ec6eb3952a0dcb417b4c3c3c7047567837 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/video/graphics.c 1 file changed, 43 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/41392/6
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41392 )
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
Patch Set 6:
Do you have an example payload as a test case?
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41392 )
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
Patch Set 6:
Patch Set 6:
Do you have an example payload as a test case?
Yes, depthcharge. This bug was discovered when I was testing https://chromium-review.googlesource.com/c/chromiumos/platform/depthcharge/+.... You can see from the screenshot the gaps between colored blocks.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41392 )
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... PS1, Line 82: while (d > INT32_MAX || d < INT32_MIN) {
new bits added on the left side are usually 0, but if the first operand is a signed negative value […]
Fine. coreboot and libpayload are written for GCC (and maybe clang which follows GCC on these things, although I think we dropped that support again for other issues). GCC implements arithmetic shifts as expected: https://gcc.gnu.org/onlinedocs/gcc-8.3.0/gcc/Integers-implementation.html#In...
We have plenty of code relying on GCC-specific extensions already, let's please not add a bunch of unnecessary complication here for this.
Hello build bot (Jenkins), Joel Kitching, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41392
to look at the new patch set (#7).
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
libpayload/cbgfx: Remove gap between adjacent boxes
When drawing two adjacent boxes with draw_box(), there will be a gap between them. This is due to the truncation in integer division when calculating the bottom right coordinate of the box.
In this patch, the relative bottom right coordinate is calculated before transforming to an absolute one. The same issue is also fixed for draw_rounded_box().
Also check validity of 'pos_rel' and 'dim_rel' arguments for draw_rounded_box().
BRANCH=none BUG=chromium:1082593 TEST=emerge-nami libpayload
Change-Id: I073cf8ec6eb3952a0dcb417b4c3c3c7047567837 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/video/graphics.c 1 file changed, 40 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/41392/7
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41392 )
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/video/graphics.c:
https://review.coreboot.org/c/coreboot/+/41392/1/payloads/libpayload/drivers... PS1, Line 82: while (d > INT32_MAX || d < INT32_MIN) {
We have plenty of code relying on GCC-specific extensions already
I didn't know that before. Done.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41392 )
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
Patch Set 7: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41392 )
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
libpayload/cbgfx: Remove gap between adjacent boxes
When drawing two adjacent boxes with draw_box(), there will be a gap between them. This is due to the truncation in integer division when calculating the bottom right coordinate of the box.
In this patch, the relative bottom right coordinate is calculated before transforming to an absolute one. The same issue is also fixed for draw_rounded_box().
Also check validity of 'pos_rel' and 'dim_rel' arguments for draw_rounded_box().
BRANCH=none BUG=chromium:1082593 TEST=emerge-nami libpayload
Change-Id: I073cf8ec6eb3952a0dcb417b4c3c3c7047567837 Signed-off-by: Yu-Ping Wu yupingso@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/41392 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/drivers/video/graphics.c 1 file changed, 40 insertions(+), 10 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/payloads/libpayload/drivers/video/graphics.c b/payloads/libpayload/drivers/video/graphics.c index ea10f13..81d2bb9 100644 --- a/payloads/libpayload/drivers/video/graphics.c +++ b/payloads/libpayload/drivers/video/graphics.c @@ -72,6 +72,35 @@ return f->d != 0; }
+static int is_valid_scale(const struct scale *s) +{ + return is_valid_fraction(&s->x) && is_valid_fraction(&s->y); +} + +static void add_fractions(struct fraction *out, + const struct fraction *f1, const struct fraction *f2) +{ + int64_t n, d; + int shift; + n = (int64_t)f1->n * f2->d + (int64_t)f2->n * f1->d; + d = (int64_t)f1->d * f2->d; + /* Simplest way to reduce the fraction until fitting in int32_t */ + shift = log2(MAX(ABS(n), ABS(d)) >> 31); + if (shift > 0) { + n >>= shift; + d >>= shift; + } + out->n = n; + out->d = d; +} + +static void add_scales(struct scale *out, + const struct scale *s1, const struct scale *s2) +{ + add_fractions(&out->x, &s1->x, &s2->x); + add_fractions(&out->y, &s1->y, &s2->y); +} + /* * Transform a vector: * x' = x * a_x + offset_x @@ -82,7 +111,7 @@ const struct scale *a, const struct vector *offset) { - if (!is_valid_fraction(&a->x) || !is_valid_fraction(&a->y)) + if (!is_valid_scale(a)) return CBGFX_ERROR_INVALID_PARAMETER; out->x = a->x.n * in->x / a->x.d + offset->x; out->y = a->y.n * in->y / a->y.d + offset->y; @@ -211,7 +240,6 @@ int draw_box(const struct rect *box, const struct rgb_color *rgb) { struct vector top_left; - struct vector size; struct vector p, t;
if (cbgfx_init()) @@ -222,14 +250,13 @@ .x = { .n = box->offset.x, .d = CANVAS_SCALE, }, .y = { .n = box->offset.y, .d = CANVAS_SCALE, } }; - const struct scale size_s = { - .x = { .n = box->size.x, .d = CANVAS_SCALE, }, - .y = { .n = box->size.y, .d = CANVAS_SCALE, } + const struct scale bottom_right_s = { + .x = { .n = box->offset.x + box->size.x, .d = CANVAS_SCALE, }, + .y = { .n = box->offset.y + box->size.y, .d = CANVAS_SCALE, } };
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); + transform_vector(&t, &canvas.size, &bottom_right_s, &canvas.offset); if (within_box(&t, &canvas) < 0) { LOG("Box exceeds canvas boundary\n"); return CBGFX_ERROR_BOUNDARY; @@ -247,8 +274,8 @@ const struct fraction *thickness, const struct fraction *radius) { + struct scale pos_end_rel; struct vector top_left; - struct vector size; struct vector p, t;
if (cbgfx_init()) @@ -256,9 +283,12 @@
const uint32_t color = calculate_color(rgb, 0);
+ if (!is_valid_scale(pos_rel) || !is_valid_scale(dim_rel)) + return CBGFX_ERROR_INVALID_PARAMETER; + + add_scales(&pos_end_rel, pos_rel, dim_rel); transform_vector(&top_left, &canvas.size, pos_rel, &canvas.offset); - transform_vector(&size, &canvas.size, dim_rel, &vzero); - add_vectors(&t, &top_left, &size); + transform_vector(&t, &canvas.size, &pos_end_rel, &canvas.offset); if (within_box(&t, &canvas) < 0) { LOG("Box exceeds canvas boundary\n"); return CBGFX_ERROR_BOUNDARY;
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41392 )
Change subject: libpayload/cbgfx: Remove gap between adjacent boxes ......................................................................
Patch Set 8:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/4047 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4046 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4045 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4044
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Yu-Ping Wu (Code Review)