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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 17.06.24 um 14:42 schrieb Helge Deller:
> On 6/17/24 13:30, Thomas Zimmermann wrote:
>>
>>
>> Am 17.06.24 um 13:06 schrieb Thomas Zimmermann:
>>> 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.
>>>
>>> v2:
>>> - clarify comment on non-VGA modes (Helge)
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Fixes: 5e8ddcbe8692 ("Video mode probing support for the new x86 
>>> setup code")
>>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>>> Cc: <stable@vger.kernel.org> # v2.6.23+
>>> ---
>>>   drivers/video/fbdev/vesafb.c |  2 +-
>>>   include/linux/screen_info.h  | 10 ++++++++++
>>>   2 files changed, 11 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);
>>>       vesafb_fix.smem_start = si->lfb_base;
>>>       vesafb_defined.bits_per_pixel = si->lfb_depth;
>>>       if (15 == vesafb_defined.bits_per_pixel)
>>> diff --git a/include/linux/screen_info.h b/include/linux/screen_info.h
>>> index 75303c126285a..d21f8e4e9f4a4 100644
>>> --- a/include/linux/screen_info.h
>>> +++ b/include/linux/screen_info.h
>>> @@ -49,6 +49,16 @@ static inline u64 __screen_info_lfb_size(const 
>>> struct screen_info *si, unsigned
>>>       return lfb_size;
>>>   }
>>> +static inline bool __screen_info_vbe_mode_nonvga(const struct 
>>> screen_info *si)
>>> +{
>>> +    /*
>>> +     * VESA modes typically run on VGA hardware. Set bit 5 signal 
>>> that this
>>
>> 'signals'
>
> I've fixed this up in your patch and applied it to the fbdev git tree.
> No need to send new patch...

Great, thank you so much.

Best regards
Thomas

>
> Thanks!
> 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)