[PATCH] fbdev: vesafb: Detect VGA compatibility from screen info's VESA attributes

[Thomas Zimmermann]

Hi

Am 14.06.24 um 00:24 schrieb Helge Deller:
> On 6/13/24 23:50, Javier Martinez Canillas wrote:
>> Helge Deller <deller at gmx.de> writes:
>>
>>> On 6/13/24 11:02, Thomas Zimmermann wrote:
>>>> Test the vesa_attributes field in struct screen_info for compatibility
>>>> with VGA hardware. Vesafb currently tests bit 1 in screen_info's
>>>> capabilities field, It sets the framebuffer address size and is
>>>> unrelated to VGA.
>>>>
>>>> Section 4.4 of the Vesa VBE 2.0 specifications defines that bit 5 in
>>>> the mode's attributes field signals VGA compatibility. The mode is
>>>> compatible with VGA hardware if the bit is clear. In that case, the
>>>> driver can access VGA state of the VBE's underlying hardware. The
>>>> vesafb driver uses this feature to program the color LUT in palette
>>>> modes. Without, colors might be incorrect.
>>>>
>>>> The problem got introduced in commit 89ec4c238e7a ("[PATCH] vesafb: 
>>>> Fix
>>>> incorrect logo colors in x86_64"). It incorrectly stores the mode
>>>> attributes in the screen_info's capabilities field and updates vesafb
>>>> accordingly. Later, commit 5e8ddcbe8692 ("Video mode probing 
>>>> support for
>>>> the new x86 setup code") fixed the screen_info, but did not update 
>>>> vesafb.
>>>> Color output still tends to work, because bit 1 in capabilities is
>>>> usually 0.
>>>>
>>>> Besides fixing the bug in vesafb, this commit introduces a helper that
>>>> reads the correct bit from screen_info.
>>>
>>> Nice catch, Thomas!
>>>
>>> But do we really need this additional helper?

Yes, please. Decoding screen_info is a science on it's own. I've added 
several of these helpers to access because such code was open-coded and 
duplicated all over the graphics code. And several places got something 
wrong. The patch here is just one of many such cases.

>>>
>>>
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>>>> Fixes: 5e8ddcbe8692 ("Video mode probing support for the new x86 
>>>> setup code")
>>>> Cc: <stable at vger.kernel.org> # v2.6.23+
>>>
>>>> ---
>>>>    drivers/video/fbdev/vesafb.c | 2 +-
>>>>    include/linux/screen_info.h  | 5 +++++
>>>>    2 files changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/video/fbdev/vesafb.c 
>>>> b/drivers/video/fbdev/vesafb.c
>>>> index 8ab64ae4cad3e..5a161750a3aee 100644
>>>> --- a/drivers/video/fbdev/vesafb.c
>>>> +++ b/drivers/video/fbdev/vesafb.c
>>>> @@ -271,7 +271,7 @@ static int vesafb_probe(struct platform_device 
>>>> *dev)
>>>>        if (si->orig_video_isVGA != VIDEO_TYPE_VLFB)
>>>>            return -ENODEV;
>>>>
>>>> -    vga_compat = (si->capabilities & 2) ? 0 : 1;
>>>> +    vga_compat = !__screen_info_vbe_mode_nonvga(si);
>>>
>>> Instead maybe just this: ?
>>>    +    /* mode is VGA-compatible if BIT 5 is _NOT_ set */
>>>    +    vga_compat = (si->vesa_attributes & BIT(5)) == 0;
>>>
>>> I suggest to make patch small, esp. if you ask for backport to 
>>> v2.6.23+.
>>>
>>
>> I prefer the helper. It's a static inline anyways and having it as a
>> function makes it much easier to read / understand.
>
> Really?

Yep.

>
> +static inline bool __screen_info_vbe_mode_nonvga(const struct 
> screen_info *si)
> +{
> +    return si->vesa_attributes & BIT(5); // VGA if _not_ set
> +}
>
> At least the double negation "!nonvga()" breaks my head and the "//" 
> comment
> should be converted to /*..*/ IMHO.

The non-VGA bit is specified by VESA. So the helper does the correct 
thing. We can make a better comment though.

If we want to simplify usage of this helper, I'd say we should kick 
vga_compat from vesafb and rework that code.

Best regards
Thomas

>
> Helge

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)