Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43508 )
Change subject: libpayload: cbgfx: Add draw_line() ......................................................................
libpayload: cbgfx: Add draw_line()
Add draw_line() to draw either a horizontal or vertical line segment.
Theoretically a horizontal line can also be drawn by calling draw_rounded_box() with dim_rel.x being the line length and dim_rel.y being the line width. However, due to the truncation in integer division when converting relative coordinates to absolute ones, this will potentially produce inconsistent line widths, depending on the value of pos_rel.y.
It is guaranteed that draw_line() will produce consistent line widths, regardless of the position of the line. Also, when the thickness argument is zero, this function is able to draw a line with 1-pixel width, which is not achievable by draw_rounded_box().
BRANCH=puff BUG=b:146399181, b:161424726 TEST=emerge-puff libpayload
Change-Id: I2d50414c4bfed343516197da9bb50791c89ba4c2 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/video/graphics.c M payloads/libpayload/include/cbgfx.h 2 files changed, 96 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43508/1
diff --git a/payloads/libpayload/drivers/video/graphics.c b/payloads/libpayload/drivers/video/graphics.c index 54d3dfa..b52bd99 100644 --- a/payloads/libpayload/drivers/video/graphics.c +++ b/payloads/libpayload/drivers/video/graphics.c @@ -99,6 +99,11 @@ out->y = v1->y + v2->y; }
+static int fraction_equal(const struct fraction *f1, const struct fraction *f2) +{ + return (int64_t)f1->n * f2->d == (int64_t)f2->n * f1->d; +} + static int is_valid_fraction(const struct fraction *f) { return f->d != 0; @@ -109,17 +114,31 @@ return is_valid_fraction(&s->x) && is_valid_fraction(&s->y); }
+static void reduce_fraction(struct fraction *out, int64_t n, int64_t d) +{ + /* Simplest way to reduce the fraction until fitting in int32_t */ + int shift = log2(MAX(ABS(n), ABS(d)) >> 31) + 1; + out->n = n >> shift; + out->d = d >> shift; +} + +/* out = f1 + f2 */ 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) + 1; - out->n = n >> shift; - out->d = d >> shift; + reduce_fraction(out, + (int64_t)f1->n * f2->d + (int64_t)f2->n * f1->d, + (int64_t)f1->d * f2->d); +} + +/* out = f1 - f2 */ +static void subtract_fractions(struct fraction *out, + const struct fraction *f1, + const struct fraction *f2) +{ + reduce_fraction(out, + (int64_t)f1->n * f2->d - (int64_t)f2->n * f1->d, + (int64_t)f1->d * f2->d); }
static void add_scales(struct scale *out, @@ -477,6 +496,59 @@ return CBGFX_SUCCESS; }
+int draw_line(const struct scale *pos1, const struct scale *pos2, + const struct fraction *thickness, const struct rgb_color *rgb) +{ + struct fraction len; + 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); + + if (!is_valid_fraction(thickness)) + return CBGFX_ERROR_INVALID_PARAMETER; + + transform_vector(&top_left, &canvas.size, pos1, &canvas.offset); + if (fraction_equal(&pos1->y, &pos2->y)) { + /* Horizontal line */ + subtract_fractions(&len, &pos2->x, &pos1->x); + struct scale dim = { + .x = { .n = len.n, .d = len.d }, + .y = { .n = thickness->n, .d = thickness->d }, + }; + transform_vector(&size, &canvas.size, &dim, &vzero); + size.y = MAX(size.y, 1); + } else if (fraction_equal(&pos1->x, &pos2->x)) { + /* Vertical line */ + subtract_fractions(&len, &pos2->y, &pos1->y); + struct scale dim = { + .x = { .n = thickness->n, .d = thickness->d }, + .y = { .n = len.n, .d = len.d }, + }; + transform_vector(&size, &canvas.size, &dim, &vzero); + size.x = MAX(size.x, 1); + } else { + LOG("Only support horizontal and vertical lines\n"); + return CBGFX_ERROR_INVALID_PARAMETER; + } + + add_vectors(&t, &top_left, &size); + if (within_box(&t, &canvas) < 0) { + LOG("Line exceeds canvas boundary\n"); + return CBGFX_ERROR_BOUNDARY; + } + + for (p.y = top_left.y; p.y < t.y; p.y++) + for (p.x = top_left.x; p.x < t.x; p.x++) + set_pixel(&p, color); + + return CBGFX_SUCCESS; +} + int clear_canvas(const struct rgb_color *rgb) { const struct rect box = { diff --git a/payloads/libpayload/include/cbgfx.h b/payloads/libpayload/include/cbgfx.h index 84e76f2..4397c28 100644 --- a/payloads/libpayload/include/cbgfx.h +++ b/payloads/libpayload/include/cbgfx.h @@ -133,6 +133,22 @@ const struct fraction *radius);
/** + * Draw a horizontal or vertical line segment on screen. If horizontal, pos1 + * must be the left endpoint. If vertical, pos1 must be the top endpoint. When + * the specified thickness is truncated to zero, a line with 1-pixel width will + * be drawn. + * + * @param[in] pos1 Start position of the line relative to the canvas. + * @param[in] pos2 End position of the line relative to the canvas. + * @param[in] thickness Thickness of the line relative to the canvas. + * @param[in] rgb Color of the line. + * + * @return CBGFX_* error codes + */ +int draw_line(const struct scale *pos1, const struct scale *pos2, + const struct fraction *thickness, const struct rgb_color *rgb); + +/** * Clear the canvas */ int clear_canvas(const struct rgb_color *rgb);
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43508 )
Change subject: libpayload: cbgfx: Add draw_line() ......................................................................
Patch Set 1: Code-Review+2
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43508 )
Change subject: libpayload: cbgfx: Add draw_line() ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43508/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43508/1//COMMIT_MSG@15 PS1, Line 15: depending on the value of : pos_rel.y Is there no good way of fixing draw_rounded_box()? I.e. calculating line widths independent of pos_rel.y.
https://review.coreboot.org/c/coreboot/+/43508/1/payloads/libpayload/include... File payloads/libpayload/include/cbgfx.h:
https://review.coreboot.org/c/coreboot/+/43508/1/payloads/libpayload/include... PS1, Line 138: truncated to zero To be strictly precise, maybe "is zero or is truncated to zero"?
Hello build bot (Jenkins), Joel Kitching, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43508
to look at the new patch set (#2).
Change subject: libpayload: cbgfx: Add draw_line() ......................................................................
libpayload: cbgfx: Add draw_line()
Add draw_line() to draw either a horizontal or vertical line segment.
Theoretically a horizontal line can also be drawn by calling draw_rounded_box() with dim_rel.x being the line length and dim_rel.y being the line width. However, due to the truncation in integer division when converting relative coordinates to absolute ones, this will potentially produce inconsistent line widths, depending on the value of pos_rel.y.
It is guaranteed that draw_line() will produce consistent line widths, regardless of the position of the line. Also, when the thickness argument is zero, this function is able to draw a line with 1-pixel width, which is not achievable by draw_rounded_box().
BRANCH=puff BUG=b:146399181, b:161424726 TEST=emerge-puff libpayload
Change-Id: I2d50414c4bfed343516197da9bb50791c89ba4c2 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/video/graphics.c M payloads/libpayload/include/cbgfx.h 2 files changed, 96 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43508/2
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43508 )
Change subject: libpayload: cbgfx: Add draw_line() ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43508/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43508/1//COMMIT_MSG@15 PS1, Line 15: depending on the value of : pos_rel.y
Is there no good way of fixing draw_rounded_box()? I.e. […]
By fixing draw_rounded_box(), we will have another issue: https://bugs.chromium.org/p/chromium/issues/detail?id=1082593.
https://review.coreboot.org/c/coreboot/+/43508/1/payloads/libpayload/include... File payloads/libpayload/include/cbgfx.h:
https://review.coreboot.org/c/coreboot/+/43508/1/payloads/libpayload/include... PS1, Line 138: truncated to zero
To be strictly precise, maybe "is zero or is truncated to zero"?
Done
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43508 )
Change subject: libpayload: cbgfx: Add draw_line() ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/43508/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43508/1//COMMIT_MSG@15 PS1, Line 15: depending on the value of : pos_rel.y
By fixing draw_rounded_box(), we will have another issue: https://bugs.chromium. […]
Right -- thanks for the reminder.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43508 )
Change subject: libpayload: cbgfx: Add draw_line() ......................................................................
Patch Set 2:
+Patrick - Same as Hung-Te's question in CB:43436, is there a way to re-trigger Jenkins build?
Hello build bot (Jenkins), Joel Kitching, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43508
to look at the new patch set (#3).
Change subject: libpayload: cbgfx: Add draw_line() ......................................................................
libpayload: cbgfx: Add draw_line()
Add a function draw_line() to draw either a horizontal or vertical line segment.
Theoretically a horizontal line can also be drawn by calling draw_rounded_box() with dim_rel.x being the line length and dim_rel.y being the line width. However, due to the truncation in integer division when converting relative coordinates to absolute ones, this will potentially produce inconsistent line widths, depending on the value of pos_rel.y.
It is guaranteed that draw_line() will produce consistent line widths, regardless of the position of the line. Also, when the thickness argument is zero, this function is able to draw a line with 1-pixel width, which is not achievable by draw_rounded_box().
BRANCH=puff BUG=b:146399181, b:161424726 TEST=emerge-puff libpayload
Change-Id: I2d50414c4bfed343516197da9bb50791c89ba4c2 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/video/graphics.c M payloads/libpayload/include/cbgfx.h 2 files changed, 96 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/43508/3
Joel Kitching has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43508 )
Change subject: libpayload: cbgfx: Add draw_line() ......................................................................
libpayload: cbgfx: Add draw_line()
Add a function draw_line() to draw either a horizontal or vertical line segment.
Theoretically a horizontal line can also be drawn by calling draw_rounded_box() with dim_rel.x being the line length and dim_rel.y being the line width. However, due to the truncation in integer division when converting relative coordinates to absolute ones, this will potentially produce inconsistent line widths, depending on the value of pos_rel.y.
It is guaranteed that draw_line() will produce consistent line widths, regardless of the position of the line. Also, when the thickness argument is zero, this function is able to draw a line with 1-pixel width, which is not achievable by draw_rounded_box().
BRANCH=puff BUG=b:146399181, b:161424726 TEST=emerge-puff libpayload
Change-Id: I2d50414c4bfed343516197da9bb50791c89ba4c2 Signed-off-by: Yu-Ping Wu yupingso@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/43508 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Joel Kitching kitching@google.com --- M payloads/libpayload/drivers/video/graphics.c M payloads/libpayload/include/cbgfx.h 2 files changed, 96 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Joel Kitching: Looks good to me, approved
diff --git a/payloads/libpayload/drivers/video/graphics.c b/payloads/libpayload/drivers/video/graphics.c index 54d3dfa..b52bd99 100644 --- a/payloads/libpayload/drivers/video/graphics.c +++ b/payloads/libpayload/drivers/video/graphics.c @@ -99,6 +99,11 @@ out->y = v1->y + v2->y; }
+static int fraction_equal(const struct fraction *f1, const struct fraction *f2) +{ + return (int64_t)f1->n * f2->d == (int64_t)f2->n * f1->d; +} + static int is_valid_fraction(const struct fraction *f) { return f->d != 0; @@ -109,17 +114,31 @@ return is_valid_fraction(&s->x) && is_valid_fraction(&s->y); }
+static void reduce_fraction(struct fraction *out, int64_t n, int64_t d) +{ + /* Simplest way to reduce the fraction until fitting in int32_t */ + int shift = log2(MAX(ABS(n), ABS(d)) >> 31) + 1; + out->n = n >> shift; + out->d = d >> shift; +} + +/* out = f1 + f2 */ 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) + 1; - out->n = n >> shift; - out->d = d >> shift; + reduce_fraction(out, + (int64_t)f1->n * f2->d + (int64_t)f2->n * f1->d, + (int64_t)f1->d * f2->d); +} + +/* out = f1 - f2 */ +static void subtract_fractions(struct fraction *out, + const struct fraction *f1, + const struct fraction *f2) +{ + reduce_fraction(out, + (int64_t)f1->n * f2->d - (int64_t)f2->n * f1->d, + (int64_t)f1->d * f2->d); }
static void add_scales(struct scale *out, @@ -477,6 +496,59 @@ return CBGFX_SUCCESS; }
+int draw_line(const struct scale *pos1, const struct scale *pos2, + const struct fraction *thickness, const struct rgb_color *rgb) +{ + struct fraction len; + 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); + + if (!is_valid_fraction(thickness)) + return CBGFX_ERROR_INVALID_PARAMETER; + + transform_vector(&top_left, &canvas.size, pos1, &canvas.offset); + if (fraction_equal(&pos1->y, &pos2->y)) { + /* Horizontal line */ + subtract_fractions(&len, &pos2->x, &pos1->x); + struct scale dim = { + .x = { .n = len.n, .d = len.d }, + .y = { .n = thickness->n, .d = thickness->d }, + }; + transform_vector(&size, &canvas.size, &dim, &vzero); + size.y = MAX(size.y, 1); + } else if (fraction_equal(&pos1->x, &pos2->x)) { + /* Vertical line */ + subtract_fractions(&len, &pos2->y, &pos1->y); + struct scale dim = { + .x = { .n = thickness->n, .d = thickness->d }, + .y = { .n = len.n, .d = len.d }, + }; + transform_vector(&size, &canvas.size, &dim, &vzero); + size.x = MAX(size.x, 1); + } else { + LOG("Only support horizontal and vertical lines\n"); + return CBGFX_ERROR_INVALID_PARAMETER; + } + + add_vectors(&t, &top_left, &size); + if (within_box(&t, &canvas) < 0) { + LOG("Line exceeds canvas boundary\n"); + return CBGFX_ERROR_BOUNDARY; + } + + for (p.y = top_left.y; p.y < t.y; p.y++) + for (p.x = top_left.x; p.x < t.x; p.x++) + set_pixel(&p, color); + + return CBGFX_SUCCESS; +} + int clear_canvas(const struct rgb_color *rgb) { const struct rect box = { diff --git a/payloads/libpayload/include/cbgfx.h b/payloads/libpayload/include/cbgfx.h index 84e76f2..f2883b0 100644 --- a/payloads/libpayload/include/cbgfx.h +++ b/payloads/libpayload/include/cbgfx.h @@ -133,6 +133,22 @@ const struct fraction *radius);
/** + * Draw a horizontal or vertical line segment on screen. If horizontal, pos1 + * must be the left endpoint. If vertical, pos1 must be the top endpoint. When + * the specified thickness is zero (or truncated to zero), a line with 1-pixel + * width will be drawn. + * + * @param[in] pos1 Start position of the line relative to the canvas. + * @param[in] pos2 End position of the line relative to the canvas. + * @param[in] thickness Thickness of the line relative to the canvas. + * @param[in] rgb Color of the line. + * + * @return CBGFX_* error codes + */ +int draw_line(const struct scale *pos1, const struct scale *pos2, + const struct fraction *thickness, const struct rgb_color *rgb); + +/** * Clear the canvas */ int clear_canvas(const struct rgb_color *rgb);
-
Joel Kitching (Code Review) -
Julius Werner (Code Review)
-
Yu-Ping Wu (Code Review)