diff mbox series

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

Message ID 20240617110725.23330-1-tzimmermann@suse.de (mailing list archive)
State Accepted, archived
Headers show
Series [v2] fbdev: vesafb: Detect VGA compatibility from screen info's VESA attributes | expand

Commit Message

Thomas Zimmermann June 17, 2024, 11:06 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.

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

Comments

Thomas Zimmermann June 17, 2024, 11:30 a.m. UTC | #1
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'

> +	 * is not the case. Drivers can then not make use of VGA resources. See
> +	 * Sec 4.4 of the VBE 2.0 spec.
> +	 */
> +	return si->vesa_attributes & BIT(5);
> +}
> +
>   static inline unsigned int __screen_info_video_type(unsigned int type)
>   {
>   	switch (type) {
Helge Deller June 17, 2024, 12:42 p.m. UTC | #2
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...

Thanks!
Helge
Thomas Zimmermann June 17, 2024, 12:44 p.m. UTC | #3
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
>
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..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
+	 * is not the case. Drivers can then not make use of VGA resources. See
+	 * Sec 4.4 of the VBE 2.0 spec.
+	 */
+	return si->vesa_attributes & BIT(5);
+}
+
 static inline unsigned int __screen_info_video_type(unsigned int type)
 {
 	switch (type) {