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

[Javier Martinez Canillas]

"Pierre Asselin" <pa@xxxxxxxxx> writes:

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