Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Javier Martinez Canillas <javierm@redhat.com>
To:     Pierre Asselin <pa@panix.com>
Cc:     Pierre Asselin <pa@panix.com>, tzimmermann@suse.de,
        linux-kernel@vger.kernel.org, jfalempe@redhat.com,
        hdegoede@redhat.com, dri-devel@lists.freedesktop.org,
        daniel.vetter@ffwll.ch, ardb@kernel.org
Subject: Re: [PATCH] firmware/sysfb: Fix wrong stride when bits-per-pixel is
 calculated
In-Reply-To: <ae5f6784c72e1b31cdf7766d3c6dbe0c.squirrel@mail.panix.com>
References: <20230412150225.3757223-1-javierm@redhat.com>
 <2e07f818ccdff7023a060e732d7c4ef6.squirrel@mail.panix.com>
 <87jzyhror0.fsf@minerva.mail-host-address-is-not-set>
 <beeff0335ab4cc244d214a7baadba371.squirrel@mail.panix.com>
 <CAFOAJEdKBUg91pDmNYYw5xigUxjifBgOLz2YgD+xQ+WyEy=V2w@mail.gmail.com>
 <1afd3044c2aca9322ecf304941c7df66.squirrel@mail.panix.com>
 <87fs94stgw.fsf@minerva.mail-host-address-is-not-set>
 <87cz48srs4.fsf@minerva.mail-host-address-is-not-set>
 <40edb0fdb0eaff434f4872dd677923a6.squirrel@mail.panix.com>
 <87a5zcsqg8.fsf@minerva.mail-host-address-is-not-set>
 <9e6fff69b09b36cbdd96499cd0015154.squirrel@mail.panix.com>
 <4PxhQn3zK1zcbc@panix1.panix.com>
 <87o7nsuumt.fsf@minerva.mail-host-address-is-not-set>
 <ae5f6784c72e1b31cdf7766d3c6dbe0c.squirrel@mail.panix.com>
Date:   Thu, 13 Apr 2023 18:34:40 +0200
Message-ID: <87jzyfrba7.fsf@minerva.mail-host-address-is-not-set>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

"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