Messages in this thread | | | From | Javier Martinez Canillas <> | Subject | Re: [PATCH] firmware/sysfb: Fix wrong stride when bits-per-pixel is calculated | Date | Thu, 13 Apr 2023 18:34:40 +0200 |
| |
"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!
-- Best regards,
Javier Martinez Canillas Core Platforms Red Hat
| |