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

[Helge Deller <deller@gmx.de>]

On 6/14/24 09:23, Thomas Zimmermann wrote:
> Hi
>
> Am 14.06.24 um 00:24 schrieb Helge Deller:
>> On 6/13/24 23:50, Javier Martinez Canillas wrote:
>>> Helge Deller <deller@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@suse.de>
>>>>> Fixes: 5e8ddcbe8692 ("Video mode probing support for the new x86 setup code")
>>>>> Cc: <stable@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.

Ok, then I suggest to keep your patch as-is, with just the "//" comment
replaced by a /**/ comment.

Helge