Re: [PATCH] firmware/sysfb: Fix wrong stride when bits-per-pixel is calculated

[Pierre Asselin]

> pa@xxxxxxxxx (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.