| Message ID | 20230412150225.3757223-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | firmware/sysfb: Fix wrong stride when bits-per-pixel is calculated | expand |
> 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 <pa@panix.com> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > --- > > 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" <pa@panix.com> 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 <pa@panix.com> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >> --- >> >> 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 ?
> 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 <pa@panix.com> 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 ?
> 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" <pa@panix.com> 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.
Javier Martinez Canillas <javierm@redhat.com> 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; } }
> Javier Martinez Canillas <javierm@redhat.com> 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" <pa@panix.com> writes: >> Javier Martinez Canillas <javierm@redhat.com> 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 >
(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" <pa@panix.com> 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.
pa@panix.com (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 <javierm@redhat.com> 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 <pa@panix.com> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- 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
> pa@panix.com (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" <pa@panix.com> writes: >> pa@panix.com (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!
> 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" <pa@panix.com> 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...
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 <tzimmermann@suse.de> 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/dfb4f25ca8dfb0d81d778d6315f104ad.squirrel@mail.panix.com/ [3]: https://lists.freedesktop.org/archives/dri-devel/2023-April/400088.html
Hi Am 17.04.23 um 10:58 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> 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/dfb4f25ca8dfb0d81d778d6315f104ad.squirrel@mail.panix.com/ > [3]: https://lists.freedesktop.org/archives/dri-devel/2023-April/400088.html >
Thomas Zimmermann <tzimmermann@suse.de> 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.
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; } }
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 <pa@panix.com> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- drivers/firmware/sysfb_simplefb.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) base-commit: e62252bc55b6d4eddc6c2bdbf95a448180d6a08d