cbgfx: Prevent divide-by-zero edge case in Lanczos kernel
The existing lanczos_weight() implementation naively follows the purely mathematical definition for the `x == 0` special case. However, the point of defining that special case is obviously to prevent division by zero in the general case formula. Unfortunately we are still doing some multiplications with `x` before we get to the division step, and our fpmath library loses precision during multiplication. This can lead to edge cases where `x` is not zero but `x_times_pi` later ends up being 0, which causes the division to throw an exception after all. (I guess we've just been lucky to not see this case in practice for now... it requires the output pixel coordinate to be extremely close to but not quite on the next input pixel coordinate, which may be rare in practice with our scaling algorithms.) This patch fixes the issue by implementing the special case later and checking if `x_times_pi` is zero instead. Note that as long as we pass this check, we can be confident that the division cannot fail even though fpdiv() also truncates the divisor: this is because `x_times_pi` was calculated from an fpmul() call with the constant fppi(), which has 34 significant bits. Even if x is the smallest possible non-zero value after scaling for multiplication, the result `x_times_pi` must still have 18 significant bits. That means it can be scaled down a further 16 bits for division without becoming zero. Also add a simple unit test forcing exactly this condition to ensure the code will not regress. Change-Id: I2f212ee5df38252e97ec55aba3d2d25320c4b102 Signed-off-by: Julius Werner <jwerner@chromium.org> Reviewed-on: https://review.coreboot.org/c/coreboot/+/87532 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Yu-Ping Wu <yupingso@google.com> Reviewed-by: Jakub "Kuba" Czapiga <czapiga@google.com>
This commit is contained in:
parent
565c768c20
commit
c1bcb43f7c
3 changed files with 32 additions and 5 deletions
|
|
@ -715,19 +715,20 @@ static fpmath_t lanczos_weight(fpmath_t in, int off)
|
|||
*
|
||||
* So (off - S0) - (in - floor(in)) is the distance from the sample
|
||||
* pixel to S0 minus the distance from S0 to the output pixel, aka
|
||||
* the distance from the sample pixel to the output pixel.
|
||||
* the distance from the sample pixel to the output pixel. (Note that
|
||||
* this calculation using fpfloor() is only valid if |in| is not
|
||||
* negative, which is always the case for our current code.)
|
||||
*/
|
||||
fpmath_t x = fpisub(off - S0, fpsubi(in, fpfloor(in)));
|
||||
|
||||
if (fpequals(x, fp(0)))
|
||||
return fp(1);
|
||||
|
||||
/* x * 2 / a can save some instructions if a == 2 */
|
||||
fpmath_t x2a = x;
|
||||
if (LNCZ_A != 2)
|
||||
x2a = fpmul(x, fpfrac(2, LNCZ_A));
|
||||
|
||||
fpmath_t x_times_pi = fpmul(x, fppi());
|
||||
if (fpequals(x_times_pi, fp(0)))
|
||||
return fp(1);
|
||||
|
||||
/*
|
||||
* Rather than using sinr(pi*x), we leverage the "one-based" sine
|
||||
|
|
|
|||
|
|
@ -1,6 +1,9 @@
|
|||
# SPDX-License-Identifier: GPL-2.0-only
|
||||
|
||||
tests-y += speaker-test
|
||||
tests-y += graphics-test speaker-test
|
||||
|
||||
graphics-test-srcs += tests/drivers/graphics-test.c
|
||||
graphics-test-srcs += libc/fpmath.c
|
||||
|
||||
speaker-test-srcs += tests/drivers/speaker-test.c
|
||||
speaker-test-mocks += inb
|
||||
|
|
|
|||
23
payloads/libpayload/tests/drivers/graphics-test.c
Normal file
23
payloads/libpayload/tests/drivers/graphics-test.c
Normal file
|
|
@ -0,0 +1,23 @@
|
|||
/* SPDX-License-Identifier: GPL-2.0-only */
|
||||
|
||||
#include "../drivers/video/graphics.c"
|
||||
|
||||
#include <libpayload.h>
|
||||
#include <tests/test.h>
|
||||
|
||||
static void test_lanczos(void **state)
|
||||
{
|
||||
/* This makes sure the `x` in lanczos_weight() becomes very small. This
|
||||
test is mostly here to prove we won't end up dividing by zero. */
|
||||
fpmath_t x = lanczos_weight(fpisub(1, fpfrac(1, 1 << 30)), S0 + 1);
|
||||
assert_true(fpequals(x, fp(1)));
|
||||
}
|
||||
|
||||
int main(void)
|
||||
{
|
||||
const struct CMUnitTest tests[] = {
|
||||
cmocka_unit_test(test_lanczos),
|
||||
};
|
||||
|
||||
return lp_run_group_tests(tests, NULL, NULL);
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue