The commit f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
fixed format selection, by calculating the bits-per-pixel instead of just
using the reported color depth.
But unfortunately this broke some modes because the stride is always set
to the reported line length (in bytes), which could not match the actual
stride if the calculated bits-per-pixel doesn't match the reported depth.
Fixes: f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
Reported-by: Pierre Asselin <[email protected]>
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
drivers/firmware/sysfb_simplefb.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
index 82c64cb9f531..5dc23e57089f 100644
--- a/drivers/firmware/sysfb_simplefb.c
+++ b/drivers/firmware/sysfb_simplefb.c
@@ -28,7 +28,7 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
struct simplefb_platform_data *mode)
{
__u8 type;
- u32 bits_per_pixel;
+ u32 bits_per_pixel, stride;
unsigned int i;
type = si->orig_video_isVGA;
@@ -54,14 +54,19 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
* bits_per_pixel here and ignore lfb_depth. In the loop below,
* ignore simplefb formats with alpha bits, as EFI and VESA
* don't specify alpha channels.
+ *
+ * If a calculated bits_per_pixel is used instead of lfb_depth,
+ * then also ignore lfb_linelength and calculate the stride.
*/
if (si->lfb_depth > 8) {
bits_per_pixel = max(max3(si->red_size + si->red_pos,
si->green_size + si->green_pos,
si->blue_size + si->blue_pos),
si->rsvd_size + si->rsvd_pos);
+ stride = DIV_ROUND_UP(si->lfb_width * bits_per_pixel, 8);
} else {
bits_per_pixel = si->lfb_depth;
+ stride = si->lfb_linelength;
}
for (i = 0; i < ARRAY_SIZE(formats); ++i) {
@@ -80,7 +85,7 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
mode->format = f->name;
mode->width = si->lfb_width;
mode->height = si->lfb_height;
- mode->stride = si->lfb_linelength;
+ mode->stride = stride;
return true;
}
}
base-commit: e62252bc55b6d4eddc6c2bdbf95a448180d6a08d
--
2.40.0
> The commit f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
> fixed format selection, by calculating the bits-per-pixel instead of just
> using the reported color depth.
>
> But unfortunately this broke some modes because the stride is always set
> to the reported line length (in bytes), which could not match the actual
> stride if the calculated bits-per-pixel doesn't match the reported depth.
>
> Fixes: f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
> Reported-by: Pierre Asselin <[email protected]>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
> ---
>
> drivers/firmware/sysfb_simplefb.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/sysfb_simplefb.c
> b/drivers/firmware/sysfb_simplefb.c
> index 82c64cb9f531..5dc23e57089f 100644
> --- a/drivers/firmware/sysfb_simplefb.c
> +++ b/drivers/firmware/sysfb_simplefb.c
>
> [patch elided]
NOOOOOO ! The 1024x768x32 screen is all garbled.
(gfxpayload=keep, gfxpayload=1024x768x32 or vga=0x318).
The other modes work as before (but the dmesg has less information;
I'll investigate.)
"Pierre Asselin" <[email protected]> writes:
>> The commit f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
>> fixed format selection, by calculating the bits-per-pixel instead of just
>> using the reported color depth.
>>
>> But unfortunately this broke some modes because the stride is always set
>> to the reported line length (in bytes), which could not match the actual
>> stride if the calculated bits-per-pixel doesn't match the reported depth.
>>
>> Fixes: f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
>> Reported-by: Pierre Asselin <[email protected]>
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>> ---
>>
>> drivers/firmware/sysfb_simplefb.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/sysfb_simplefb.c
>> b/drivers/firmware/sysfb_simplefb.c
>> index 82c64cb9f531..5dc23e57089f 100644
>> --- a/drivers/firmware/sysfb_simplefb.c
>> +++ b/drivers/firmware/sysfb_simplefb.c
>>
>> [patch elided]
>
> NOOOOOO ! The 1024x768x32 screen is all garbled.
> (gfxpayload=keep, gfxpayload=1024x768x32 or vga=0x318).
>
That's suprising... I tested the patch with vga=ask and picked
1024x768x15, 1024x768x16, 1024x768x24 and 1024x768x32. For all
cases the bits-per-pixel and line length values were correct.
But I don't have a machine with legacy BIOS so I testee using QEMU and
SeaBIOS.
> The other modes work as before (but the dmesg has less information;
> I'll investigate.)
>
Interesting. So you don't have the simplefb output that you had before ?
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
> Interesting. So you don't have the simplefb output that you had before ?
I do now, after a 'make clean' and "make bzImage'.
In between, I had tried CONFIG_SYSFB_SIMPLEFB=n . That "works", by
falling back to vesafb in every case. I restored the .config before
testing the patch, but there must have been leftover dregs in the
build tree. 1024x768x32 is garbled, others are good whether simplefb
or vesafb.
--PA
On Wed, Apr 12, 2023 at 8:12 PM Pierre Asselin <[email protected]> wrote:
>
>
> > Interesting. So you don't have the simplefb output that you had before ?
>
> I do now, after a 'make clean' and "make bzImage'.
>
> In between, I had tried CONFIG_SYSFB_SIMPLEFB=n . That "works", by
> falling back to vesafb in every case. I restored the .config before
> testing the patch, but there must have been leftover dregs in the
> build tree. 1024x768x32 is garbled, others are good whether simplefb
And can you share the "linelength=" print out from simplefb ?
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
> And can you share the "linelength=" print out from simplefb ?
Okay. Three cases, see below.
Your patch tries to fix the stride, but what if it's the _depth_
that's wrong ? Grub sets the mode, the pre-regression kernel picks this:
format=x8r8g8b8, mode=1024x768x32, linelength=4096
========== Good ======================================================
grub: gfxpayload=1024x768x24
[ 0.003333] Console: colour dummy device 128x48
[ 0.003333] printk: console [tty0] enabled
[ 0.417054] fbcon: Taking over console
[ 0.513399] pci 0000:01:05.0: vgaarb: setting as boot VGA device
[ 0.513431] pci 0000:01:05.0: vgaarb: bridge control possible
[ 0.513455] pci 0000:01:05.0: vgaarb: VGA device added:
decodes=io+mem,owns=io+mem,locks=none
[ 0.513490] vgaarb: loaded
[ 3.337529] simple-framebuffer simple-framebuffer.0: framebuffer at
0xd8000000, 0x240000 bytes
[ 3.337567] simple-framebuffer simple-framebuffer.0: format=r8g8b8,
mode=1024x768x24, linelength=3072
[ 3.338000] Console: switching to colour frame buffer device 128x48
[ 3.566490] simple-framebuffer simple-framebuffer.0: fb0: simplefb
registered!
========== Bad after patch, typing blind to log in !==================
grub: gfxpayload=keep
[ 0.003333] Console: colour dummy device 128x48
[ 0.003333] printk: console [tty0] enabled
[ 0.423925] fbcon: Taking over console
[ 0.520030] pci 0000:01:05.0: vgaarb: setting as boot VGA device
[ 0.520061] pci 0000:01:05.0: vgaarb: bridge control possible
[ 0.520085] pci 0000:01:05.0: vgaarb: VGA device added:
decodes=io+mem,owns=io+mem,locks=none
[ 0.520120] vgaarb: loaded
[ 3.290444] simple-framebuffer simple-framebuffer.0: framebuffer at
0xd8000000, 0x240000 bytes
[ 3.290483] simple-framebuffer simple-framebuffer.0: format=r8g8b8,
mode=1024x768x24, linelength=3072
[ 3.290916] Console: switching to colour frame buffer device 128x48
[ 3.519523] simple-framebuffer simple-framebuffer.0: fb0: simplefb
registered!
========== Good, earlier kernel before regression ====================
grub: gfxpayload=keep
[ 0.226675] Console: colour dummy device 128x48
[ 0.228643] printk: console [tty0] enabled
[ 0.429214] fbcon: Taking over console
[ 0.524994] pci 0000:01:05.0: vgaarb: setting as boot VGA device
[ 0.525025] pci 0000:01:05.0: vgaarb: bridge control possible
[ 0.525049] pci 0000:01:05.0: vgaarb: VGA device added:
decodes=io+mem,owns=io+mem,locks=none
[ 0.525082] vgaarb: loaded
[ 3.320474] simple-framebuffer simple-framebuffer.0: framebuffer at
0xd8000000, 0x300000 bytes
[ 3.320513] simple-framebuffer simple-framebuffer.0: format=x8r8g8b8,
mode=1024x768x32, linelength=4096
[ 3.320983] Console: switching to colour frame buffer device 128x48
[ 3.415643] simple-framebuffer simple-framebuffer.0: fb0: simplefb
registered!
"Pierre Asselin" <[email protected]> writes:
>> And can you share the "linelength=" print out from simplefb ?
>
> Okay. Three cases, see below.
>
> Your patch tries to fix the stride, but what if it's the _depth_
> that's wrong ? Grub sets the mode, the pre-regression kernel picks this:
> format=x8r8g8b8, mode=1024x768x32, linelength=4096
>
Yes, it seems the VBE mode set by GRUB is x8r8g8b8. And the line length
calculation is also correct: 1024 * (32 / 8) = 4096.
> ========== Good ======================================================
> grub: gfxpayload=1024x768x24
> [ 0.003333] Console: colour dummy device 128x48
> [ 0.003333] printk: console [tty0] enabled
> [ 0.417054] fbcon: Taking over console
> [ 0.513399] pci 0000:01:05.0: vgaarb: setting as boot VGA device
> [ 0.513431] pci 0000:01:05.0: vgaarb: bridge control possible
> [ 0.513455] pci 0000:01:05.0: vgaarb: VGA device added:
> decodes=io+mem,owns=io+mem,locks=none
> [ 0.513490] vgaarb: loaded
> [ 3.337529] simple-framebuffer simple-framebuffer.0: framebuffer at
> 0xd8000000, 0x240000 bytes
> [ 3.337567] simple-framebuffer simple-framebuffer.0: format=r8g8b8,
> mode=1024x768x24, linelength=3072
This is also correct when GRUB sets it to r8g8b8, since the line length
is: 1024 * (24 / 8) = 3072.
> [ 3.338000] Console: switching to colour frame buffer device 128x48
> [ 3.566490] simple-framebuffer simple-framebuffer.0: fb0: simplefb
> registered!
>
> ========== Bad after patch, typing blind to log in !==================
> grub: gfxpayload=keep
> [ 0.003333] Console: colour dummy device 128x48
> [ 0.003333] printk: console [tty0] enabled
> [ 0.423925] fbcon: Taking over console
> [ 0.520030] pci 0000:01:05.0: vgaarb: setting as boot VGA device
> [ 0.520061] pci 0000:01:05.0: vgaarb: bridge control possible
> [ 0.520085] pci 0000:01:05.0: vgaarb: VGA device added:
> decodes=io+mem,owns=io+mem,locks=none
> [ 0.520120] vgaarb: loaded
> [ 3.290444] simple-framebuffer simple-framebuffer.0: framebuffer at
> 0xd8000000, 0x240000 bytes
> [ 3.290483] simple-framebuffer simple-framebuffer.0: format=r8g8b8,
> mode=1024x768x24, linelength=3072
Now, this is the part where things start to break I believe. Because you
mentioned before that gfxpayload=keep used to set the format to xr8g8b8
but now after my patch (and also after the original commit f35cd3fa7729)
it is set to r8g8b8 instead.
That *shouldn't* be an issue because it only means that the alpha channel
is discarded but maybe it is an issue for your display controller?
By the way, in https://www.panix.com/~pa/linux-6.3-simplefb/selected-modes
that you shared before the gfxpayload=keep GRUB option used to also led to
the pixel format being set to r8g8b8 instead of xr8g8b8. The difference is
that in that output the line lenght didn't match the pixel format and size:
[ 3.290596] simple-framebuffer simple-framebuffer.0: format=r8g8b8, mode=1024x768x24, linelength=4096
but after my patch you mentioned that is:
[ 3.290483] simple-framebuffer simple-framebuffer.0: format=r8g8b8, mode=1024x768x24, linelength=3072
which at least matches, so in a way is an improvement (even when it still
doesn't work).
> [ 3.290916] Console: switching to colour frame buffer device 128x48
> [ 3.519523] simple-framebuffer simple-framebuffer.0: fb0: simplefb
> registered!
>
> ========== Good, earlier kernel before regression ====================
> grub: gfxpayload=keep
> [ 0.226675] Console: colour dummy device 128x48
> [ 0.228643] printk: console [tty0] enabled
> [ 0.429214] fbcon: Taking over console
> [ 0.524994] pci 0000:01:05.0: vgaarb: setting as boot VGA device
> [ 0.525025] pci 0000:01:05.0: vgaarb: bridge control possible
> [ 0.525049] pci 0000:01:05.0: vgaarb: VGA device added:
> decodes=io+mem,owns=io+mem,locks=none
> [ 0.525082] vgaarb: loaded
> [ 3.320474] simple-framebuffer simple-framebuffer.0: framebuffer at
> 0xd8000000, 0x300000 bytes
> [ 3.320513] simple-framebuffer simple-framebuffer.0: format=x8r8g8b8,
> mode=1024x768x32, linelength=4096
Yes, and it works because is the correct mode it seems but for some reason
after commit f35cd3fa7729 the pixel format is calculated incorrectly. We
can see that the total framebuffer size is 0x300000 bytes, which matches a
1024x768x34 mode framebuffer: 1024 * 768 * (34 / 8) = 3342336 = 0x300000.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Javier Martinez Canillas <[email protected]> writes:
[...]
>> ========== Bad after patch, typing blind to log in !==================
>> grub: gfxpayload=keep
>> [ 0.003333] Console: colour dummy device 128x48
>> [ 0.003333] printk: console [tty0] enabled
>> [ 0.423925] fbcon: Taking over console
>> [ 0.520030] pci 0000:01:05.0: vgaarb: setting as boot VGA device
>> [ 0.520061] pci 0000:01:05.0: vgaarb: bridge control possible
>> [ 0.520085] pci 0000:01:05.0: vgaarb: VGA device added:
>> decodes=io+mem,owns=io+mem,locks=none
>> [ 0.520120] vgaarb: loaded
>> [ 3.290444] simple-framebuffer simple-framebuffer.0: framebuffer at
>> 0xd8000000, 0x240000 bytes
>> [ 3.290483] simple-framebuffer simple-framebuffer.0: format=r8g8b8,
>> mode=1024x768x24, linelength=3072
>
> Now, this is the part where things start to break I believe. Because you
> mentioned before that gfxpayload=keep used to set the format to xr8g8b8
> but now after my patch (and also after the original commit f35cd3fa7729)
> it is set to r8g8b8 instead.
>
I still don't understand why this particular configuration didn't work...
The framebuffer starts at 0xd8000000 and has a size of 0x240000 bytes, so
a r8g8b8 pixel format with resolution 1024x768 should be correct. Since is
1024 * 768 * (24 / 8) = 2359296 = 0x240000.
In any case, it seems that there is something wrong on how the screen_info
is reported to sysfb since you mentioned that gfxpayload=1024x768x32 leads
to a format=r8g8b8 and mode=1024x768x24, instead of the format=xr8g8b8 and
mode=1024x768x32 that is expected.
Could you please apply the following diff that will print all the relevant
fields from the screen_info that are used to calculate the bpp and stride.
My guess is that the rsvd_size and rsvd_pos are not correct and that's why
the bpp is set to 24 instead of 32.
diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
index 5dc23e57089f..6678ac6ff5b1 100644
--- a/drivers/firmware/sysfb_simplefb.c
+++ b/drivers/firmware/sysfb_simplefb.c
@@ -58,6 +58,13 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
* If a calculated bits_per_pixel is used instead of lfb_depth,
* then also ignore lfb_linelength and calculate the stride.
*/
+
+ printk("sysfb: si->lfb_depth %u si->lfb_width %u\n", si->lfb_depth, si->lfb_width);
+ printk("sysfb: si->red_size %u si->red_pos %u\n", si->red_size, si->red_pos);
+ printk("sysfb: si->green_size %u si->green_pos %u\n", si->green_size, si->green_pos);
+ printk("sysfb: si->blue_size %u si->blue_pos %u\n", si->blue_size, si->blue_pos);
+ printk("sysfb: si->rsvd_size %u si->rsvd_pos %u\n", si->rsvd_size, si->rsvd_pos);
+
if (si->lfb_depth > 8) {
bits_per_pixel = max(max3(si->red_size + si->red_pos,
si->green_size + si->green_pos,
@@ -69,6 +76,9 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
stride = si->lfb_linelength;
}
+ printk("sysfb: bits_per_pixel %u si->lfb_linelength %u\n", bits_per_pixel, si->lfb_linelength);
+ printk("sysfb: stride %u\n", stride);
+
for (i = 0; i < ARRAY_SIZE(formats); ++i) {
const struct simplefb_format *f = &formats[i];
@@ -86,6 +96,7 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
mode->width = si->lfb_width;
mode->height = si->lfb_height;
mode->stride = stride;
+ printk("sysfb: format %s\n", f->name);
return true;
}
}
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
> Javier Martinez Canillas <[email protected]> writes:
>
> I still don't understand why this particular configuration didn't work...
>
> The framebuffer starts at 0xd8000000 and has a size of 0x240000 bytes, so
Says who ? It's the same grub, same video mode as before the regression,
so the size is probably 0x300000 like it always was.
> a r8g8b8 pixel format with resolution 1024x768 should be correct. Since is
> 1024 * 768 * (24 / 8) = 2359296 = 0x240000.
That is internally consistent, but at variance with the video mode
set up by grub.
It is better to sqeeze bits by 4:3 on each line (regression) than to
scatter 4 logical lines across 3 physical lines (regression, patched) !
> Could you please apply the following diff that will print all the relevant
> fields from the screen_info that are used to calculate the bpp and stride.
YES ! I can't peer into that struct screen_info and I don't know to
write the printk's. (Hm, doesn't look too hard, but trust me, I would
fumble it.)
I'll back out the original patch first.
Stand by.
--PA
"Pierre Asselin" <[email protected]> writes:
>> Javier Martinez Canillas <[email protected]> writes:
>>
>> I still don't understand why this particular configuration didn't work...
>>
>> The framebuffer starts at 0xd8000000 and has a size of 0x240000 bytes, so
>
> Says who ? It's the same grub, same video mode as before the regression,
> so the size is probably 0x300000 like it always was.
>
>> a r8g8b8 pixel format with resolution 1024x768 should be correct. Since is
>> 1024 * 768 * (24 / 8) = 2359296 = 0x240000.
>
> That is internally consistent, but at variance with the video mode
> set up by grub.
>
> It is better to sqeeze bits by 4:3 on each line (regression) than to
> scatter 4 logical lines across 3 physical lines (regression, patched) !
>
Indeed. I noticed now that the IORESOURCE_MEM is set-up in the function
sysfb_create_simplefb() so is likely that is internally consistent as you
said but wrong :)
>> Could you please apply the following diff that will print all the relevant
>> fields from the screen_info that are used to calculate the bpp and stride.
>
> YES ! I can't peer into that struct screen_info and I don't know to
> write the printk's. (Hm, doesn't look too hard, but trust me, I would
> fumble it.)
>
> I'll back out the original patch first.
> Stand by.
>
> --PA
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
(Okay, can't back out *all* of the first patch, just the assignment
to mode->stride.)
Anyway, here you go:
grub: gfxpayload=keep
[ 0.003333] Console: colour dummy device 128x48
[ 0.003333] printk: console [tty0] enabled
[ 0.419983] fbcon: Taking over console
[ 0.516198] pci 0000:01:05.0: vgaarb: setting as boot VGA device
[ 0.516229] pci 0000:01:05.0: vgaarb: bridge control possible
[ 0.516253] pci 0000:01:05.0: vgaarb: VGA device added:
decodes=io+mem,owns=io+mem,locks=none
[ 0.516288] vgaarb: loaded
[ 3.343649] simple-framebuffer simple-framebuffer.0: framebuffer at
0xd8000000, 0x300000 bytes
[ 3.343687] simple-framebuffer simple-framebuffer.0: format=r8g8b8,
mode=1024x768x24, linelength=4096
[ 3.344199] Console: switching to colour frame buffer device 128x48
[ 3.681177] simple-framebuffer simple-framebuffer.0: fb0: simplefb
registered!
[ 3.343345] sysfb: si->lfb_depth 32 si->lfb_width 1024
[ 3.343372] sysfb: si->red_size 8 si->red_pos 16
[ 3.343392] sysfb: si->green_size 8 si->green_pos 8
[ 3.343413] sysfb: si->blue_size 8 si->blue_pos 0
[ 3.343433] sysfb: si->rsvd_size 0 si->rsvd_pos 0
[ 3.343453] sysfb: bits_per_pixel 24 si->lfb_linelength 4096
[ 3.343476] sysfb: stride 3072
[ 3.343493] sysfb: format r8g8b8
So it's the rsvd_size and rsvd_pos that are bogus. The fix would be to:
1) believe si->lfb_depth
2) fill with ones a bitmask of size si->lfb_depth
3) clear chunks of bits based on si->{red,green,blue,rsvd}_{size,pos}
4) printk if the bitmask is not all zeros
5) override rsvd_{size,pos} based on the bitmask
That way you know where the 'x' goes in xrgb.
Hm. Could that fix my two boxes but cause a regression for someone else ?
What if _depth is low but the rsvd_ are right ?
Then _width and _linelength would be inconsistent with _depth but
consistent with the recomputed bits_per_pixel ? How many ways can the
firmware lie ?
We need more testers, don't we ?
After careful reading of the comments in f35cd3fa7729, would this
be an appropriate fix ? Does it still address all the issues raised
by Thomas ?
(not tested)
diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
index 82c64cb9f531..9f5299d54732 100644
--- a/drivers/firmware/sysfb_simplefb.c
+++ b/drivers/firmware/sysfb_simplefb.c
@@ -56,10 +56,11 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
* don't specify alpha channels.
*/
if (si->lfb_depth > 8) {
- bits_per_pixel = max(max3(si->red_size + si->red_pos,
+ bits_per_pixel = max3(max3(si->red_size + si->red_pos,
si->green_size + si->green_pos,
si->blue_size + si->blue_pos),
- si->rsvd_size + si->rsvd_pos);
+ si->rsvd_size + si->rsvd_pos,
+ si->lfb_depth);
} else {
bits_per_pixel = si->lfb_depth;
}
> (not tested)
Tested. It fixes the regression on my laptop.
> diff --git a/drivers/firmware/sysfb_simplefb.c
> b/drivers/firmware/sysfb_simplefb.c
> index 82c64cb9f531..9f5299d54732 100644
> --- a/drivers/firmware/sysfb_simplefb.c
> +++ b/drivers/firmware/sysfb_simplefb.c
> @@ -56,10 +56,11 @@ __init bool sysfb_parse_mode(const struct screen_info
> *si,
> * don't specify alpha channels.
> */
> if (si->lfb_depth > 8) {
> - bits_per_pixel = max(max3(si->red_size + si->red_pos,
> + bits_per_pixel = max3(max3(si->red_size + si->red_pos,
> si->green_size + si->green_pos,
> si->blue_size + si->blue_pos),
> - si->rsvd_size + si->rsvd_pos);
> + si->rsvd_size + si->rsvd_pos,
> + si->lfb_depth);
> } else {
> bits_per_pixel = si->lfb_depth;
> }
"Pierre Asselin" <[email protected]> writes:
[...]
> [ 3.343433] sysfb: si->rsvd_size 0 si->rsvd_pos 0
Thanks for confirming this. I was expected that as mentioned since it was
the only reasonable explanation for your problem.
[...]
> What if _depth is low but the rsvd_ are right ?
> Then _width and _linelength would be inconsistent with _depth but
> consistent with the recomputed bits_per_pixel ? How many ways can the
> firmware lie ?
>
I don't know. But in your case the firmware is not reporting the mode
correctly since it is setting a framebuffer of 1024x768 and xRGB but
is not reporting si->rsvd_size=8 and si->rsvd_pos=24 as it should.
One option is to have a DMI match table similar to what we already have
for EFI machines in drivers/firmware/efi/sysfb_efi.c but also for BIOS.
The question then is if we can trust other systems to report a proper
rsvd_size and rsvd_pos...
> We need more testers, don't we ?
>
It's tricky, yes.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
[email protected] (Pierre Asselin) writes:
> After careful reading of the comments in f35cd3fa7729, would this
> be an appropriate fix ? Does it still address all the issues raised
> by Thomas ?
>
> (not tested)
>
> diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
> index 82c64cb9f531..9f5299d54732 100644
> --- a/drivers/firmware/sysfb_simplefb.c
> +++ b/drivers/firmware/sysfb_simplefb.c
> @@ -56,10 +56,11 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
> * don't specify alpha channels.
> */
> if (si->lfb_depth > 8) {
> - bits_per_pixel = max(max3(si->red_size + si->red_pos,
> + bits_per_pixel = max3(max3(si->red_size + si->red_pos,
> si->green_size + si->green_pos,
> si->blue_size + si->blue_pos),
> - si->rsvd_size + si->rsvd_pos);
> + si->rsvd_size + si->rsvd_pos,
> + si->lfb_depth);
I would defer to Thomas but personally I don't like it. Seems to me that
this is getting too complicated just to workaround buggy BIOS that are not
reporting consistent information about their firmware-provided framebuffer.
I would either trust the pixel channel information (what Thomas patch did)
+ my patch to calculate the stride (since we can't trust the line lenght
which is based on the reported depth) + a DMI table for broken machines.
But that will only work if your systems are the exception and not a common
issue, otherwise then Thomas' approach won't work if there are too many
buggy BIOS out there.
Another option is to do the opposite, not calculate a BPP using the pixel
and then use that value to calculate a new stride, but instead assume that
the lfb_linelength is correct and use that to calculate the BPP.
Something like the following patch, which should also fix your regression
and may be enough to address Thomas' concerns of inconsistent depths too?
From e70d4df257f8a84deea82f75270b14e069729679 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <[email protected]>
Date: Thu, 13 Apr 2023 09:00:09 +0200
Subject: [PATCH v2] firmware/sysfb: Use reported line length to calculate the
bits-per-pixel
The commit f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
fixed format selection, by calculating the bits-per-pixel instead of just
using the reported color depth.
But unfortunately it broke some systems due the calculated bits-per-pixel
not taking into account the filler bits, for pixel formats that contained
padding bits.
For example some firmware-provided framebuffers with pixel format xRGB24
where wrongly reported as RGB24, causing the VT output to have glitches.
This seems to be caused by the rsvd_size and rsvd_pos fields not being
set correctly in some cases. So a different calculation has to be made.
Since the lfb_depth field can't be trusted, use the lfb_linelength field
to calculate the bits-per-pixel. This is already set to the stride so it
is already deemed to be correctly set by the firmware.
Fixes: f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
Reported-by: Pierre Asselin <[email protected]>
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
Changes in v2:
- Instead of calculating the stride from the bits-per-pixel, assume that
it is correct and use it to calculate the bits-per-pixel.
drivers/firmware/sysfb_simplefb.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
index 82c64cb9f531..0ab8c542b1f5 100644
--- a/drivers/firmware/sysfb_simplefb.c
+++ b/drivers/firmware/sysfb_simplefb.c
@@ -55,14 +55,10 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
* ignore simplefb formats with alpha bits, as EFI and VESA
* don't specify alpha channels.
*/
- if (si->lfb_depth > 8) {
- bits_per_pixel = max(max3(si->red_size + si->red_pos,
- si->green_size + si->green_pos,
- si->blue_size + si->blue_pos),
- si->rsvd_size + si->rsvd_pos);
- } else {
+ if (si->lfb_depth > 8)
+ bits_per_pixel = si->lfb_linelength * 8 / si->lfb_width;
+ else
bits_per_pixel = si->lfb_depth;
- }
for (i = 0; i < ARRAY_SIZE(formats); ++i) {
const struct simplefb_format *f = &formats[i];
base-commit: e62252bc55b6d4eddc6c2bdbf95a448180d6a08d
--
2.40.0
> [email protected] (Pierre Asselin) writes:
>
>> After careful reading of the comments in f35cd3fa7729, would this
>> be an appropriate fix ? Does it still address all the issues raised
>> by Thomas ?
>>
>> (not tested)
>>
>> diff --git a/drivers/firmware/sysfb_simplefb.c
>> b/drivers/firmware/sysfb_simplefb.c
>> index 82c64cb9f531..9f5299d54732 100644
>> --- a/drivers/firmware/sysfb_simplefb.c
>> +++ b/drivers/firmware/sysfb_simplefb.c
>> @@ -56,10 +56,11 @@ __init bool sysfb_parse_mode(const struct
>> screen_info *si,
>> * don't specify alpha channels.
>> */
>> if (si->lfb_depth > 8) {
>> - bits_per_pixel = max(max3(si->red_size + si->red_pos,
>> + bits_per_pixel = max3(max3(si->red_size + si->red_pos,
>> si->green_size + si->green_pos,
>> si->blue_size + si->blue_pos),
>> - si->rsvd_size + si->rsvd_pos);
>> + si->rsvd_size + si->rsvd_pos,
>> + si->lfb_depth);
> I would defer to Thomas but personally I don't like it. Seems to me that
> this is getting too complicated just to workaround buggy BIOS that are not
> reporting consistent information about their firmware-provided
> framebuffer.
Okay, but remember, this is a regression. The buggy BIOSes were there
the whole time and the old code that matched f->bits_per_pixel against
si->lfb_depth used to work against these buggy BIOSes.
And is it a bug, or is it an underspecified standard ? "These bits aren't
reserved, we just ignore them" or some such.
My reading of Thomas' comments in the code was that sometimes lfb_depth
was reported too small but never too large ? But I'm not sure. It's
true in my two cases.
> I would either trust the pixel channel information (what Thomas patch did)
> + my patch to calculate the stride (since we can't trust the line lenght
> which is based on the reported depth) + a DMI table for broken machines.
> But that will only work if your systems are the exception and not a common
> issue, otherwise then Thomas' approach won't work if there are too many
> buggy BIOS out there.
The laptop is ancient but the Dell tower is maybe 4 years old. The
BIOS is 09/11/2019 according to dmidecode, and the most recent for
this box.
> Another option is to do the opposite, not calculate a BPP using the pixel
> and then use that value to calculate a new stride, but instead assume that
> the lfb_linelength is correct and use that to calculate the BPP.
Or reject (some) inconsistencies in the struct screen_info and return
false, so the kernel falls back to e.g. vesa ?
> Something like the following patch, which should also fix your regression
> and may be enough to address Thomas' concerns of inconsistent depths too?
I'll try that patch.
"Pierre Asselin" <[email protected]> writes:
>> [email protected] (Pierre Asselin) writes:
[...]
>>> - bits_per_pixel = max(max3(si->red_size + si->red_pos,
>>> + bits_per_pixel = max3(max3(si->red_size + si->red_pos,
>>> si->green_size + si->green_pos,
>>> si->blue_size + si->blue_pos),
>>> - si->rsvd_size + si->rsvd_pos);
>>> + si->rsvd_size + si->rsvd_pos,
>>> + si->lfb_depth);
>
>
>> I would defer to Thomas but personally I don't like it. Seems to me that
>> this is getting too complicated just to workaround buggy BIOS that are not
>> reporting consistent information about their firmware-provided
>> framebuffer.
>
> Okay, but remember, this is a regression. The buggy BIOSes were there
Yes, I agree that is a regression and has to be fixed. I'm just arguing
against this particular fix.
> the whole time and the old code that matched f->bits_per_pixel against
> si->lfb_depth used to work against these buggy BIOSes.
>
> And is it a bug, or is it an underspecified standard ? "These bits aren't
> reserved, we just ignore them" or some such.
>
> My reading of Thomas' comments in the code was that sometimes lfb_depth
> was reported too small but never too large ? But I'm not sure. It's
> true in my two cases.
>
I (mis?)understood that it could be too small as well. But that's why I
prefer Thomas to chime in before agreeing on any path forward. But he is
in vacation this week I believe.
>> I would either trust the pixel channel information (what Thomas patch did)
>> + my patch to calculate the stride (since we can't trust the line lenght
>> which is based on the reported depth) + a DMI table for broken machines.
>
>> But that will only work if your systems are the exception and not a common
>> issue, otherwise then Thomas' approach won't work if there are too many
>> buggy BIOS out there.
>
> The laptop is ancient but the Dell tower is maybe 4 years old. The
> BIOS is 09/11/2019 according to dmidecode, and the most recent for
> this box.
>
I see.
>> Another option is to do the opposite, not calculate a BPP using the pixel
>> and then use that value to calculate a new stride, but instead assume that
>> the lfb_linelength is correct and use that to calculate the BPP.
>
> Or reject (some) inconsistencies in the struct screen_info and return
> false, so the kernel falls back to e.g. vesa ?
>
That a reasonable approach too. But better if we can make simpledrm work
too since many distros have dropped all the fbdev drivers in favor of it.
>> Something like the following patch, which should also fix your regression
>> and may be enough to address Thomas' concerns of inconsistent depths too?
>
> I'll try that patch.
>
Thanks for all your information and testing with this bug!
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
> diff --git a/drivers/firmware/sysfb_simplefb.c
> b/drivers/firmware/sysfb_simplefb.c
> index 82c64cb9f531..0ab8c542b1f5 100644
> --- a/drivers/firmware/sysfb_simplefb.c
> +++ b/drivers/firmware/sysfb_simplefb.c
> @@ -55,14 +55,10 @@ __init bool sysfb_parse_mode(const struct screen_info
> *si,
> * ignore simplefb formats with alpha bits, as EFI and VESA
> * don't specify alpha channels.
> */
> - if (si->lfb_depth > 8) {
> - bits_per_pixel = max(max3(si->red_size + si->red_pos,
> - si->green_size + si->green_pos,
> - si->blue_size + si->blue_pos),
> - si->rsvd_size + si->rsvd_pos);
> - } else {
> + if (si->lfb_depth > 8)
> + bits_per_pixel = si->lfb_linelength * 8 / si->lfb_width;
> + else
> bits_per_pixel = si->lfb_depth;
> - }
>
> for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> const struct simplefb_format *f = &formats[i];
>
> base-commit: e62252bc55b6d4eddc6c2bdbf95a448180d6a08d
> --
> 2.40.0
Patch is good on both boxes.
"Pierre Asselin" <[email protected]> writes:
>> diff --git a/drivers/firmware/sysfb_simplefb.c
>> b/drivers/firmware/sysfb_simplefb.c
>> index 82c64cb9f531..0ab8c542b1f5 100644
>> --- a/drivers/firmware/sysfb_simplefb.c
>> +++ b/drivers/firmware/sysfb_simplefb.c
>> @@ -55,14 +55,10 @@ __init bool sysfb_parse_mode(const struct screen_info
>> *si,
>> * ignore simplefb formats with alpha bits, as EFI and VESA
>> * don't specify alpha channels.
>> */
>> - if (si->lfb_depth > 8) {
>> - bits_per_pixel = max(max3(si->red_size + si->red_pos,
>> - si->green_size + si->green_pos,
>> - si->blue_size + si->blue_pos),
>> - si->rsvd_size + si->rsvd_pos);
>> - } else {
>> + if (si->lfb_depth > 8)
>> + bits_per_pixel = si->lfb_linelength * 8 / si->lfb_width;
>> + else
>> bits_per_pixel = si->lfb_depth;
>> - }
>>
>> for (i = 0; i < ARRAY_SIZE(formats); ++i) {
>> const struct simplefb_format *f = &formats[i];
>>
>> base-commit: e62252bc55b6d4eddc6c2bdbf95a448180d6a08d
>> --
>> 2.40.0
>
> Patch is good on both boxes.
>
Thanks for testing it! I'll wait for Thomas though before posting as a
proper patch. I'm sure whether we can rely on lfb_linelength or not...
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Hi,
thanks a lot to both of you for this bug fix.
Am 13.04.23 um 03:34 schrieb Pierre Asselin:
>> (not tested)
>
> Tested. It fixes the regression on my laptop.
>
>> diff --git a/drivers/firmware/sysfb_simplefb.c
>> b/drivers/firmware/sysfb_simplefb.c
>> index 82c64cb9f531..9f5299d54732 100644
>> --- a/drivers/firmware/sysfb_simplefb.c
>> +++ b/drivers/firmware/sysfb_simplefb.c
>> @@ -56,10 +56,11 @@ __init bool sysfb_parse_mode(const struct screen_info
>> *si,
>> * don't specify alpha channels.
>> */
>> if (si->lfb_depth > 8) {
>> - bits_per_pixel = max(max3(si->red_size + si->red_pos,
>> + bits_per_pixel = max3(max3(si->red_size + si->red_pos,
>> si->green_size + si->green_pos,
>> si->blue_size + si->blue_pos),
>> - si->rsvd_size + si->rsvd_pos);
>> + si->rsvd_size + si->rsvd_pos,
>> + si->lfb_depth);
I'm OK with this change. There's a comment
"The best solution is to compute bits_per_pixel here and ignore
lfb_depth."
I'd change this to
"The best solution is to compute bits_per_pixel here from the color
bits, the reserved bits and the reported color depth; whatever is highest."
That will hopefully clarify the meaning of these max3() statements. They
are not obvious at first.
Best regards
Thomas
>> } else {
>> bits_per_pixel = si->lfb_depth;
>> }
>
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Thomas Zimmermann <[email protected]> writes:
> Hi,
>
> thanks a lot to both of you for this bug fix.
>
> Am 13.04.23 um 03:34 schrieb Pierre Asselin:
>>> (not tested)
>>
>> Tested. It fixes the regression on my laptop.
>>
>>> diff --git a/drivers/firmware/sysfb_simplefb.c
>>> b/drivers/firmware/sysfb_simplefb.c
>>> index 82c64cb9f531..9f5299d54732 100644
>>> --- a/drivers/firmware/sysfb_simplefb.c
>>> +++ b/drivers/firmware/sysfb_simplefb.c
>>> @@ -56,10 +56,11 @@ __init bool sysfb_parse_mode(const struct screen_info
>>> *si,
>>> * don't specify alpha channels.
>>> */
>>> if (si->lfb_depth > 8) {
>>> - bits_per_pixel = max(max3(si->red_size + si->red_pos,
>>> + bits_per_pixel = max3(max3(si->red_size + si->red_pos,
>>> si->green_size + si->green_pos,
>>> si->blue_size + si->blue_pos),
>>> - si->rsvd_size + si->rsvd_pos);
>>> + si->rsvd_size + si->rsvd_pos,
>>> + si->lfb_depth);
>
> I'm OK with this change. There's a comment
>
> "The best solution is to compute bits_per_pixel here and ignore
> lfb_depth."
>
> I'd change this to
>
> "The best solution is to compute bits_per_pixel here from the color
> bits, the reserved bits and the reported color depth; whatever is highest."
>
> That will hopefully clarify the meaning of these max3() statements. They
> are not obvious at first.
>
I'm OK with this as well but then should probably also apply my patch [1]
that computed the stride too. Since if we don't trust the lfb_depth and
calculate the BPP, then we shouldn't trust the reported line length too.
As Pierre reported in the thread [2], when the wrong BPP was calculated (and
wrong pixel format chosen), the line lenght didn't match the BPP * lfb_width.
He mentioned that it was like this:
format=r8g8b8, mode=1024x768x24, linelength=4096
Instead of the expected:
format=r8g8b8, mode=1024x768x24, linelength=3072
My patch in [1], fixed the linelength calculation so it was internally
consistent (but still wrong since the pixel format was really xr8g8b8).
In other words, I think that we should either:
a) Trust the lfb_linelength and lfb_width (we are already doing that since
mode->stride and mode->width are set to those once the format matches).
If we decided to trust those, then the bits-per-pixel could just be
calculated as: bits_per_pixel = si->lfb_linelength * 8 / si->lfb_width
which is what I do on my v2 patch [3].
b) Not trust lfb_linelength, since that would need to be recalculated after
the BPP was calcualted. That's why I mentioned that we need Pierre's fix +
my patch [1] that did:
stride = DIV_ROUND_UP(si->lfb_width * bits_per_pixel, 8)
But calculating a BPP yet blindly using linelength doens't make sense to me.
[1]: https://lists.freedesktop.org/archives/dri-devel/2023-April/399963.html
[2]: https://lore.kernel.org/dri-devel/[email protected]/
[3]: https://lists.freedesktop.org/archives/dri-devel/2023-April/400088.html
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Hi
Am 17.04.23 um 10:58 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <[email protected]> writes:
>
>> Hi,
>>
>> thanks a lot to both of you for this bug fix.
>>
>> Am 13.04.23 um 03:34 schrieb Pierre Asselin:
>>>> (not tested)
>>>
>>> Tested. It fixes the regression on my laptop.
>>>
>>>> diff --git a/drivers/firmware/sysfb_simplefb.c
>>>> b/drivers/firmware/sysfb_simplefb.c
>>>> index 82c64cb9f531..9f5299d54732 100644
>>>> --- a/drivers/firmware/sysfb_simplefb.c
>>>> +++ b/drivers/firmware/sysfb_simplefb.c
>>>> @@ -56,10 +56,11 @@ __init bool sysfb_parse_mode(const struct screen_info
>>>> *si,
>>>> * don't specify alpha channels.
>>>> */
>>>> if (si->lfb_depth > 8) {
>>>> - bits_per_pixel = max(max3(si->red_size + si->red_pos,
>>>> + bits_per_pixel = max3(max3(si->red_size + si->red_pos,
>>>> si->green_size + si->green_pos,
>>>> si->blue_size + si->blue_pos),
>>>> - si->rsvd_size + si->rsvd_pos);
>>>> + si->rsvd_size + si->rsvd_pos,
>>>> + si->lfb_depth);
>>
>> I'm OK with this change. There's a comment
>>
>> "The best solution is to compute bits_per_pixel here and ignore
>> lfb_depth."
>>
>> I'd change this to
>>
>> "The best solution is to compute bits_per_pixel here from the color
>> bits, the reserved bits and the reported color depth; whatever is highest."
>>
>> That will hopefully clarify the meaning of these max3() statements. They
>> are not obvious at first.
>>
>
> I'm OK with this as well but then should probably also apply my patch [1]
> that computed the stride too. Since if we don't trust the lfb_depth and
> calculate the BPP, then we shouldn't trust the reported line length too.
>
> As Pierre reported in the thread [2], when the wrong BPP was calculated (and
> wrong pixel format chosen), the line lenght didn't match the BPP * lfb_width.
>
> He mentioned that it was like this:
>
> format=r8g8b8, mode=1024x768x24, linelength=4096
>
> Instead of the expected:
>
> format=r8g8b8, mode=1024x768x24, linelength=3072
>
> My patch in [1], fixed the linelength calculation so it was internally
> consistent (but still wrong since the pixel format was really xr8g8b8).
>
> In other words, I think that we should either:
>
> a) Trust the lfb_linelength and lfb_width (we are already doing that since
> mode->stride and mode->width are set to those once the format matches).
>
> If we decided to trust those, then the bits-per-pixel could just be
> calculated as: bits_per_pixel = si->lfb_linelength * 8 / si->lfb_width
>
> which is what I do on my v2 patch [3].
>
> b) Not trust lfb_linelength, since that would need to be recalculated after
> the BPP was calcualted. That's why I mentioned that we need Pierre's fix +
> my patch [1] that did:
>
> stride = DIV_ROUND_UP(si->lfb_width * bits_per_pixel, 8)
I'd rather keep the code as-is until we get an actual bug report.
For example, DRM framebuffer sizes are often multiples of 64. Creating a
framebuffer of 800x600 will create a framebuffer with
stride/pitch/linelength of 832. I can imagine that some BIOSes out
there do something similar with the system framebuffer. Messing with the
stride would break them.
Best regards
Thomas
>
> But calculating a BPP yet blindly using linelength doens't make sense to me.
>
> [1]: https://lists.freedesktop.org/archives/dri-devel/2023-April/399963.html
> [2]: https://lore.kernel.org/dri-devel/[email protected]/
> [3]: https://lists.freedesktop.org/archives/dri-devel/2023-April/400088.html
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Thomas Zimmermann <[email protected]> writes:
[...]
>>
>> stride = DIV_ROUND_UP(si->lfb_width * bits_per_pixel, 8)
>
> I'd rather keep the code as-is until we get an actual bug report.
>
Ok. After all if the pixel format is chosen correctly, the reported line
length should match anyways. So is really a corner case what Pierre had.
> For example, DRM framebuffer sizes are often multiples of 64. Creating a
> framebuffer of 800x600 will create a framebuffer with
> stride/pitch/linelength of 832. I can imagine that some BIOSes out
> there do something similar with the system framebuffer. Messing with the
> stride would break them.
>
I see, is not that simple then. Thanks a lot for the clarification.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat