Change in coreboot[master]: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45235 )

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling

The 'equals' counter stores the number of latest pixels that were
exactly equal. Since the sample array is updated in a column-based
manner, we should also initialize the 'equals' counter in the same
order.

BUG=b:167739127
TEST=emerge-puff libpayload
TEST=Character 'k' is rendered correctly on puff
BRANCH=zork

Change-Id: Ibc91ad1af85adcf093eff40797cd54f32f57111d
Signed-off-by: Yu-Ping Wu 
---
M payloads/libpayload/drivers/video/graphics.c
1 file changed, 22 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/45235/1

diff --git a/payloads/libpayload/drivers/video/graphics.c b/payloads/libpayload/drivers/video/graphics.c
index 2bf5e19..a8c9612 100644
--- a/payloads/libpayload/drivers/video/graphics.c
+++ b/payloads/libpayload/drivers/video/graphics.c
@@ -858,36 +858,38 @@
    	}

/*
-		 * Initialize the sample array for this line. For pixels to the
-		 * left of S0 there are no corresponding input pixels so just
-		 * copy the S0 values over.
-		 *
-		 * Also initialize the equals counter, which counts how many of
-		 * the latest pixels were exactly equal. We know the columns
-		 * left of S0 must be equal to S0, so start with that number.
+		 * Initialize the sample array for this line, and also
+		 * the equals counter, which counts how many of the latest
+		 * pixels were exactly equal.
    	 */
-		int equals = S0 * SSZ;
-		uint8_t last_equal = ypix[0][0];
-		for (sy = 0; sy < SSZ; sy++) {
-			for (sx = S0; sx < SSZ; sx++) {
+		int equals = 0;
+		uint8_t last_equal = -1;
+		struct rgb_color *last_sample = NULL;
+		for (sx = 0; sx < SSZ; sx++) {
+			for (sy = 0; sy < SSZ; sy++) {
    			if (sx >= dim_org->width) {
    				sample[sx][sy] = sample[sx - 1][sy];
    				equals++;
    				continue;
    			}
-				uint8_t i = ypix[sy][sx - S0];
+				/*
+				 * For pixels to the left of S0 there are no
+				 * corresponding input pixels so just use
+				 * ypix[sy][0].
+				 */
+				uint8_t i = ypix[sy][MAX(0, sx - S0)];
+				if (last_sample && i == last_equal) {
+					sample[sx][sy] = *last_sample;
+					equals++;
+					continue;
+				}
    			if (pal_to_rgb(i, pal, header->colors_used,
    				       &sample[sx][sy]))
    				goto bitmap_error;
-				if (i == last_equal) {
-					equals++;
-				} else {
-					last_equal = i;
-					equals = 1;
-				}
+				last_equal = i;
+				last_sample = &sample[sx][sy];
+				equals = 1;
    		}
-			for (sx = S0 - 1; sx >= 0; sx--)
-				sample[sx][sy] = sample[S0][sy];
    	}

ix = 0;

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45235 )

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/45235/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45235/1//COMMIT_MSG@9 
PS1, Line 9: The 'equals' counter stores the number of latest pixels that were
Please summarize the problem first.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45235 )

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

Patch Set 1:

(1 comment)

Can you explain exactly what the problem is (or did you just try around until it went away)? Because I'm not really seeing it yet or why your code is necessary to fix something. (Sorry, I know my code is hard to follow... but it's important we really understand what's going on here to make sure we can fix it in the most performant manner.)

I found one error in a different place and I guess your patch may be inadvertently working around that (because you're iterating over X before Y, which normally shouldn't make a difference, but it does because that bug can make it access uninitialized values in the array). Can you try to fix that instead and see if it has an effect on your 'k'?

https://review.coreboot.org/c/coreboot/+/45235/1/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/video/graphics.c:

https://review.coreboot.org/c/coreboot/+/45235/1/payloads/libpayload/drivers... 
PS1, Line 873: sx 
I think this needs to be (sx - S0 >= dim_org->width), btw. Would be nice if you could fix that too while you're here. (I guess if the font glyph bitmaps are very small, this could actually be the reason for your bug?)

[Yu-Ping Wu (Code Review)]

Hello build bot (Jenkins), Paul Menzel,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/45235

to look at the new patch set (#2).

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling

The current initialization of the 'equals' counter is incorrect, so that
when 'equals >= SSZ * SSZ', the pixels in the sample array might not be
all the same, leading to an wrong pixel value being set in the
framebuffer.

The 'equals' counter stores the number of latest pixels that were
exactly equal. Within the for loop of 'ox', the sample array is updated
in a column-based order, and the 'equals' counter is updated
accordingly. However, the 'equals' counter is initialized in a row-based
order, which causes it to be set too large than it should be. Consider
the example where sample[sx][sy] are initially:

[X X X A A A]  // sy = 0
 [X X X B B B]
 [X X X B B B]
 [X X X B B B]
 [X X X B B B]
 [X X X B B B]  // sy = SSZ

Then, the correct implementation will initialize 'equals' to be 15, with
last_equal being B. Suppose all of the remaining pixels are B. Then, at
the end of the 'while (fpfloor(ixfp) > ix)' loop when ix = 4, or
equivalently after 4 more columns of sample are updated, 'equals' will
be 15 + 6 * 4 = 39, which is greater than SSZ * SSZ = 36, but we can see
there are still 2 A's in the sample:

[B B B B A A]
 [B B B B B B]
 [B B B B B B]
 [B B B B B B]
 [B B B B B B]
 [B B B B B B]

Therefore, we must also initialize the 'equals' counter in a
column-based order.

BUG=b:167739127
TEST=emerge-puff libpayload
TEST=Character 'k' is rendered correctly on puff
BRANCH=zork

Change-Id: Ibc91ad1af85adcf093eff40797cd54f32f57111d
Signed-off-by: Yu-Ping Wu 
---
M payloads/libpayload/drivers/video/graphics.c
1 file changed, 23 insertions(+), 21 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/45235/2

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45235 )

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

Patch Set 2:

(2 comments)
> Patch Set 1:
>
> (1 comment)
>
> Can you explain exactly what the problem is (or did you just try around until it went away)?

Explained in more details in the commit message. Please check PS2.

https://review.coreboot.org/c/coreboot/+/45235/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45235/1//COMMIT_MSG@9 
PS1, Line 9: The 'equals' counter stores the number of latest pixels that were
> Please summarize the problem first.

Done

https://review.coreboot.org/c/coreboot/+/45235/1/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/video/graphics.c:

https://review.coreboot.org/c/coreboot/+/45235/1/payloads/libpayload/drivers... 
PS1, Line 873: sx
> I think this needs to be (sx - S0 >= dim_org->width), btw.

Yes, you're right. Corrected.
> I guess if the font glyph bitmaps are very small, this could actually be the reason for your bug?

Actually no. dim_org->width is 9 for puff and SSZ is 6, so this condition will never be reached.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45235 )

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/45235/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45235/2//COMMIT_MSG@11 
PS2, Line 11: an wrong
a wrong

[Yu-Ping Wu (Code Review)]

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/+/45235

to look at the new patch set (#3).

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling

The current initialization of the 'equals' counter is incorrect, so that
when 'equals >= SSZ * SSZ', the pixels in the sample array might not be
all the same, leading to a wrong pixel value being set in the
framebuffer.

The 'equals' counter stores the number of latest pixels that were
exactly equal. Within the for loop of 'ox', the sample array is updated
in a column-based order, and the 'equals' counter is updated
accordingly. However, the 'equals' counter is initialized in a row-based
order, which causes it to be set too large than it should be. Consider
the example where sample[sx][sy] are initially:

[X X X A A A]  // sy = 0
 [X X X B B B]
 [X X X B B B]
 [X X X B B B]
 [X X X B B B]
 [X X X B B B]  // sy = SSZ

Then, the correct implementation will initialize 'equals' to be 15, with
last_equal being B. Suppose all of the remaining pixels are B. Then, at
the end of the 'while (fpfloor(ixfp) > ix)' loop when ix = 4, or
equivalently after 4 more columns of sample are updated, 'equals' will
be 15 + 6 * 4 = 39, which is greater than SSZ * SSZ = 36, but we can see
there are still 2 A's in the sample:

[B B B B A A]
 [B B B B B B]
 [B B B B B B]
 [B B B B B B]
 [B B B B B B]
 [B B B B B B]

Therefore, we must also initialize the 'equals' counter in a
column-based order.

BUG=b:167739127
TEST=emerge-puff libpayload
TEST=Character 'k' is rendered correctly on puff
BRANCH=zork

Change-Id: Ibc91ad1af85adcf093eff40797cd54f32f57111d
Signed-off-by: Yu-Ping Wu 
---
M payloads/libpayload/drivers/video/graphics.c
1 file changed, 23 insertions(+), 21 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/45235/3

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45235 )

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45235/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45235/2//COMMIT_MSG@11 
PS2, Line 11: an wrong
> a wrong

Done

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45235 )

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

Patch Set 3:

(1 comment)

Thanks for the explanation, makes perfect sense now! Main patch LGTM, only the last_sample addtion still seems weird (and unrelated?) to me.

https://review.coreboot.org/c/coreboot/+/45235/3/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/video/graphics.c:

https://review.coreboot.org/c/coreboot/+/45235/3/payloads/libpayload/drivers... 
PS3, Line 882: 					sample[sx][sy] = *last_sample;
I'm confused what you're trying to add with the whole last_sample thing, it doesn't seem to be necessary? Are you just trying to save a call to pal_to_rgb()? I kinda doubt that's worth it... but if you want to add it you should also add it to the loop in line 922 for consistency (that's really the more important one). (I'm also not sure if making it a pointer or a normal struct rgb variable would be more efficient...)

[Yu-Ping Wu (Code Review)]

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/+/45235

to look at the new patch set (#4).

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling

The current initialization of the 'equals' counter is incorrect, so that
when 'equals >= SSZ * SSZ', the pixels in the sample array might not be
all the same, leading to a wrong pixel value being set in the
framebuffer.

The 'equals' counter stores the number of latest pixels that were
exactly equal. Within the for loop of 'ox', the sample array is updated
in a column-based order, and the 'equals' counter is updated
accordingly. However, the 'equals' counter is initialized in a row-based
order, which causes it to be set too large than it should be. Consider
the example where sample[sx][sy] are initially:

[X X X A A A]  // sy = 0
 [X X X B B B]
 [X X X B B B]
 [X X X B B B]
 [X X X B B B]
 [X X X B B B]  // sy = SSZ

Then, the correct implementation will initialize 'equals' to be 15, with
last_equal being B. Suppose all of the remaining pixels are B. Then, at
the end of the 'while (fpfloor(ixfp) > ix)' loop when ix = 4, or
equivalently after 4 more columns of sample are updated, 'equals' will
be 15 + 6 * 4 = 39, which is greater than SSZ * SSZ = 36, but we can see
there are still 2 A's in the sample:

[B B B B A A]
 [B B B B B B]
 [B B B B B B]
 [B B B B B B]
 [B B B B B B]
 [B B B B B B]

Therefore, we must also initialize the 'equals' counter in a
column-based order.

BUG=b:167739127
TEST=emerge-puff libpayload
TEST=Character 'k' is rendered correctly on puff
BRANCH=zork

Change-Id: Ibc91ad1af85adcf093eff40797cd54f32f57111d
Signed-off-by: Yu-Ping Wu 
---
M payloads/libpayload/drivers/video/graphics.c
1 file changed, 15 insertions(+), 16 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/45235/4

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45235 )

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45235/3/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/video/graphics.c:

https://review.coreboot.org/c/coreboot/+/45235/3/payloads/libpayload/drivers... 
PS3, Line 882: 					sample[sx][sy] = *last_sample;
> I'm confused what you're trying to add with the whole last_sample thing, it doesn't seem to be neces […]

This is not all about the speed. It's just not easy to initialize with the right order while keeping the code clean. Considering that

1. The sample array for 0 <= sx < S0 needs to be initialized first, since the case 'sx - S0 >= dim_org->width' will need these values (another bug in the original implementation).
2. I'd like to have only one occurrence of pal_to_rgb() in this block of code.
3. I'd also like to avoid too many calls to pal_to_rgb() for 0 <= sx < S0

this is the best way I can think of right now. Dropping condition #3, we could get rid of 'last_sample' (see PS4). Do you think it's better?

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45235 )

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45235/3/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/video/graphics.c:

https://review.coreboot.org/c/coreboot/+/45235/3/payloads/libpayload/drivers... 
PS3, Line 882: 					sample[sx][sy] = *last_sample;
> This is not all about the speed. […]

BTW, this makes me recall b/162775521, where the monospace texts are completely unrecognizable. Even if the screen resolution is pretty low in bmpblk, the texts shouldn't look that bad. I think that issue is related to the bug (sx - S0 >= dim_org->width) we found here.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45235 )

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

Patch Set 4: Code-Review+2

(2 comments)

https://review.coreboot.org/c/coreboot/+/45235/3/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/video/graphics.c:

https://review.coreboot.org/c/coreboot/+/45235/3/payloads/libpayload/drivers... 
PS3, Line 882: 					sample[sx][sy] = *last_sample;
> The sample array for 0 <= sx < S0 needs to be initialized first, since the case 'sx - S0 >= dim_org->width' will need these values (another bug in the original implementation).

I wrote this under the assumption that images are always at least one pixel wide and high (otherwise parse_bitmap_header_v3() exits earlier), so for sx == S0 the check for sx - S0 >= dim_org->width can never be true. So I don't think that was another bug... it can't reach over the end of the image on the left while trying to fill values beyond the end of the image on the right, there's at least one pixel actual image in between that it would use instead.

The way you wrote it now is the way I would've expected you to write it, so yeah, let's just stick with that.

https://review.coreboot.org/c/coreboot/+/45235/4/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/video/graphics.c:

https://review.coreboot.org/c/coreboot/+/45235/4/payloads/libpayload/drivers... 
PS4, Line 883: equals > 0 
nit: This extra check is unnecessary since i can never equal -1 (which actually ends up being 0xff because the type is unsigned). You could've also just left last_equal to get initialized to ypix[0][0] as it was before, since that's the right value for the first pixel (and then we'd just equals++ here).

But no big difference either way.

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45235 )

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45235/3/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/video/graphics.c:

https://review.coreboot.org/c/coreboot/+/45235/3/payloads/libpayload/drivers... 
PS3, Line 882: 					sample[sx][sy] = *last_sample;
> So I don't think that was another bug

Yes, you're right about that.

[Yu-Ping Wu (Code Review)]

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/+/45235

to look at the new patch set (#5).

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling

The current initialization of the 'equals' counter is incorrect, so that
when 'equals >= SSZ * SSZ', the pixels in the sample array might not be
all the same, leading to a wrong pixel value being set in the
framebuffer.

The 'equals' counter stores the number of latest pixels that were
exactly equal. Within the for loop of 'ox', the sample array is updated
in a column-based order, and the 'equals' counter is updated
accordingly. However, the 'equals' counter is initialized in a row-based
order, which causes it to be set too large than it should be. Consider
the example where sample[sx][sy] are initially:

[X X X A A A]  // sy = 0
 [X X X B B B]
 [X X X B B B]
 [X X X B B B]
 [X X X B B B]
 [X X X B B B]  // sy = SSZ

Then, the correct implementation will initialize 'equals' to be 15, with
last_equal being B. Suppose all of the remaining pixels are B. Then, at
the end of the 'while (fpfloor(ixfp) > ix)' loop when ix = 4, or
equivalently after 4 more columns of sample are updated, 'equals' will
be 15 + 6 * 4 = 39, which is greater than SSZ * SSZ = 36, but we can see
there are still 2 A's in the sample:

[B B B B A A]
 [B B B B B B]
 [B B B B B B]
 [B B B B B B]
 [B B B B B B]
 [B B B B B B]

Therefore, we must also initialize the 'equals' counter in a
column-based order.

BUG=b:167739127
TEST=emerge-puff libpayload
TEST=Character 'k' is rendered correctly on puff
BRANCH=zork

Change-Id: Ibc91ad1af85adcf093eff40797cd54f32f57111d
Signed-off-by: Yu-Ping Wu 
---
M payloads/libpayload/drivers/video/graphics.c
1 file changed, 13 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/45235/5

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45235 )

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45235/4/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/video/graphics.c:

https://review.coreboot.org/c/coreboot/+/45235/4/payloads/libpayload/drivers... 
PS4, Line 883: equals > 0
> nit: This extra check is unnecessary since i can never equal -1 (which actually ends up being 0xff b […]

Done

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45235 )

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

Patch Set 5: Code-Review+2

[Shelley Chen (Code Review)]

Shelley Chen has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45235 )

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling

The current initialization of the 'equals' counter is incorrect, so that
when 'equals >= SSZ * SSZ', the pixels in the sample array might not be
all the same, leading to a wrong pixel value being set in the
framebuffer.

The 'equals' counter stores the number of latest pixels that were
exactly equal. Within the for loop of 'ox', the sample array is updated
in a column-based order, and the 'equals' counter is updated
accordingly. However, the 'equals' counter is initialized in a row-based
order, which causes it to be set too large than it should be. Consider
the example where sample[sx][sy] are initially:

[X X X A A A]  // sy = 0
 [X X X B B B]
 [X X X B B B]
 [X X X B B B]
 [X X X B B B]
 [X X X B B B]  // sy = SSZ

Then, the correct implementation will initialize 'equals' to be 15, with
last_equal being B. Suppose all of the remaining pixels are B. Then, at
the end of the 'while (fpfloor(ixfp) > ix)' loop when ix = 4, or
equivalently after 4 more columns of sample are updated, 'equals' will
be 15 + 6 * 4 = 39, which is greater than SSZ * SSZ = 36, but we can see
there are still 2 A's in the sample:

[B B B B A A]
 [B B B B B B]
 [B B B B B B]
 [B B B B B B]
 [B B B B B B]
 [B B B B B B]

Therefore, we must also initialize the 'equals' counter in a
column-based order.

BUG=b:167739127
TEST=emerge-puff libpayload
TEST=Character 'k' is rendered correctly on puff
BRANCH=zork

Change-Id: Ibc91ad1af85adcf093eff40797cd54f32f57111d
Signed-off-by: Yu-Ping Wu 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/45235
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
---
M payloads/libpayload/drivers/video/graphics.c
1 file changed, 13 insertions(+), 14 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 2bf5e19..563f296 100644
--- a/payloads/libpayload/drivers/video/graphics.c
+++ b/payloads/libpayload/drivers/video/graphics.c
@@ -858,24 +858,25 @@
    	}

/*
-		 * Initialize the sample array for this line. For pixels to the
-		 * left of S0 there are no corresponding input pixels so just
-		 * copy the S0 values over.
-		 *
-		 * Also initialize the equals counter, which counts how many of
-		 * the latest pixels were exactly equal. We know the columns
-		 * left of S0 must be equal to S0, so start with that number.
+		 * Initialize the sample array for this line, and also
+		 * the equals counter, which counts how many of the latest
+		 * pixels were exactly equal.
    	 */
-		int equals = S0 * SSZ;
+		int equals = 0;
    	uint8_t last_equal = ypix[0][0];
-		for (sy = 0; sy < SSZ; sy++) {
-			for (sx = S0; sx < SSZ; sx++) {
-				if (sx >= dim_org->width) {
+		for (sx = 0; sx < SSZ; sx++) {
+			for (sy = 0; sy < SSZ; sy++) {
+				if (sx - S0 >= dim_org->width) {
    				sample[sx][sy] = sample[sx - 1][sy];
    				equals++;
    				continue;
    			}
-				uint8_t i = ypix[sy][sx - S0];
+				/*
+				 * For pixels to the left of S0 there are no
+				 * corresponding input pixels so just use
+				 * ypix[sy][0].
+				 */
+				uint8_t i = ypix[sy][MAX(0, sx - S0)];
    			if (pal_to_rgb(i, pal, header->colors_used,
    				       &sample[sx][sy]))
    				goto bitmap_error;
@@ -886,8 +887,6 @@
    				equals = 1;
    			}
    		}
-			for (sx = S0 - 1; sx >= 0; sx--)
-				sample[sx][sy] = sample[S0][sy];
    	}

ix = 0;

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45235 )

Change subject: libpayload: cbgfx: Fix 'equals' counter for Lanczos resampling
......................................................................

Patch Set 6:

Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9
"QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19905
"QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19904
"QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19903
"QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19902
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19901
"HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19909
"HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19908
"HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19907
"HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19906

Please note: This test is under development and might not be accurate at all!