diff mbox series

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

Message ID 20240613090240.7107-1-tzimmermann@suse.de
State New
Headers show
Series fbdev: vesafb: Detect VGA compatibility from screen info's VESA attributes | expand

Commit Message

Thomas Zimmermann June 13, 2024, 9:02 a.m. UTC
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.

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(-)

Comments

Thomas Zimmermann June 13, 2024, 9:53 a.m. UTC | #1
Hi Javier

Am 13.06.24 um 11:35 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
>
> Hello Thomas,
>
>> 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.
>>
> How did you find this ?

I was reading through vesafb and found that [1] and [2] look 
surprisingly similar, which makes no sense. So I started looking where 
bit 1 came from. The flag signals a 64-bit framebuffer address for EFI 
(see VIDEO_CAPABILITY_64BIT_BASE 
<https://elixir.bootlin.com/linux/latest/C/ident/VIDEO_CAPABILITY_64BIT_BASE>). 
But old VESA framebuffers are usually located within the first 32-bit 
range. So the bit is mostly 0 and vesafb works as expected.

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/video/fbdev/vesafb.c#L274
[2] 
https://elixir.bootlin.com/linux/latest/source/include/linux/screen_info.h#L26

>
>> Besides fixing the bug in vesafb, this commit introduces a helper that
>> reads the correct bit from screen_info.
>>
>> 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+
>> ---
> The patch looks correct to me after your explanation in the commit message
> and looking at the mentioned commits.
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks a lot.

Best regards
Thomas

>
Helge Deller June 13, 2024, 9:30 p.m. UTC | #2
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?


>
> 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+.

Helge

>   	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..95f2a339de329 100644
> --- a/include/linux/screen_info.h
> +++ b/include/linux/screen_info.h
> @@ -49,6 +49,11 @@ 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)
> +{
> +	return si->vesa_attributes & BIT(5); // VGA if _not_ set
> +}
> +
>   static inline unsigned int __screen_info_video_type(unsigned int type)
>   {
>   	switch (type) {
Javier Martinez Canillas June 13, 2024, 9:50 p.m. UTC | #3
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?
>
>
>>
>> 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.
Helge Deller June 13, 2024, 10:24 p.m. UTC | #4
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?
>>
>>
>>>
>>> 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?

+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.

Helge
Thomas Zimmermann June 14, 2024, 7:23 a.m. UTC | #5
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.

Best regards
Thomas

>
> Helge
Helge Deller June 14, 2024, 8:09 a.m. UTC | #6
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
diff mbox series

Patch

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..95f2a339de329 100644
--- a/include/linux/screen_info.h
+++ b/include/linux/screen_info.h
@@ -49,6 +49,11 @@  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)
+{
+	return si->vesa_attributes & BIT(5); // VGA if _not_ set
+}
+
 static inline unsigned int __screen_info_video_type(unsigned int type)
 {
 	switch (type) {