[PATCH v3] firmware/sysfb: Fix VESA format selection

[PATCH v3] firmware/sysfb: Fix VESA format selection

[Pierre Asselin]

Some legacy BIOSes report no reserved bits in their 32-bit rgb mode,
breaking the calculation of bits_per_pixel in commit f35cd3fa7729
("firmware/sysfb: Fix EFI/VESA format selection").  However they report
lfb_depth correctly for those modes.  Keep the computation but
set bits_per_pixel to lfb_depth if the latter is larger.

v2 fixes the warnings from a max3() macro with arguments of different
types;  split the bits_per_pixel assignment to avoid uglyfing the code
with too many casts.

v3 fixes space and formatting blips pointed out by Javier, and change
the bit_per_pixel assignment back to a single statement using two casts.

Link: https://lore.kernel.org/r/4Psm6B6Lqkz1QXM@panix3.panix.com
Link: https://lore.kernel.org/r/20230412150225.3757223-1-javierm@redhat.com
Link: https://lore.kernel.org/dri-devel/20230418183325.2327-1-pa@panix.com/T/#u
Link: https://lore.kernel.org/dri-devel/20230419044834.10816-1-pa@panix.com/T/#u
Fixes: f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
Signed-off-by: Pierre Asselin <pa@panix.com>
---
 drivers/firmware/sysfb_simplefb.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
index 82c64cb9f531..6f7c5d0c5090 100644
--- a/drivers/firmware/sysfb_simplefb.c
+++ b/drivers/firmware/sysfb_simplefb.c
@@ -51,15 +51,18 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
 	 *
 	 * It's not easily possible to fix this in struct screen_info,
 	 * as this could break UAPI. The best solution is to compute
-	 * bits_per_pixel here and ignore lfb_depth. In the loop below,
+	 * bits_per_pixel from the color bits, reserved bits and
+	 * reported lfb_depth, whichever is highest.  In the loop below,
 	 * ignore simplefb formats with alpha bits, as EFI and VESA
 	 * don't specify alpha channels.
 	 */
 	if (si->lfb_depth > 8) {
-		bits_per_pixel = max(max3(si->red_size + si->red_pos,
-					  si->green_size + si->green_pos,
-					  si->blue_size + si->blue_pos),
-				     si->rsvd_size + si->rsvd_pos);
+		/* max() macros args should be of the same type */
+		bits_per_pixel = max3((u16)max3(si->red_size + si->red_pos,
+					si->green_size + si->green_pos,
+					si->blue_size + si->blue_pos),
+				(u16)(si->rsvd_size + si->rsvd_pos),
+				si->lfb_depth);
 	} else {
 		bits_per_pixel = si->lfb_depth;
 	}

base-commit: 6a8f57ae2eb07ab39a6f0ccad60c760743051026
-- 
2.39.2

Re: [PATCH v3] firmware/sysfb: Fix VESA format selection

[Pierre Asselin]

I went back to nested max3() after all as Thomas asked.  My first cut
had casts in the innermost max3() and the code looked truly awful.  I
decided that two casts are tolerable.  I added a comment to explain
the casts.

Against clamp_t(u8,lfb_depth,1,32): the clamp_t() macro does no
typechecking;  might as well just cast lfb_depth to u8, but that assumes
the value would fit (positively crazy if it doesn't, but still.)
Instead, I widen the other two args of the outer max3().

--PA

Re: [PATCH v3] firmware/sysfb: Fix VESA format selection

[Linux regression tracking (Thorsten Leemhuis)]

On 20.04.23 17:57, Pierre Asselin wrote:
> Some legacy BIOSes report no reserved bits in their 32-bit rgb mode,
> breaking the calculation of bits_per_pixel in commit f35cd3fa7729
> ("firmware/sysfb: Fix EFI/VESA format selection").  However they report
> lfb_depth correctly for those modes.  Keep the computation but
> set bits_per_pixel to lfb_depth if the latter is larger.
> 
> v2 fixes the warnings from a max3() macro with arguments of different
> types;  split the bits_per_pixel assignment to avoid uglyfing the code
> with too many casts.
> 
> v3 fixes space and formatting blips pointed out by Javier, and change
> the bit_per_pixel assignment back to a single statement using two casts.
> 
> Link: https://lore.kernel.org/r/4Psm6B6Lqkz1QXM@panix3.panix.com
> Link: https://lore.kernel.org/r/20230412150225.3757223-1-javierm@redhat.com
> Link: https://lore.kernel.org/dri-devel/20230418183325.2327-1-pa@panix.com/T/#u
> Link: https://lore.kernel.org/dri-devel/20230419044834.10816-1-pa@panix.com/T/#u
> Fixes: f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
> Signed-off-by: Pierre Asselin <pa@panix.com>

Linus might release the final this weekend and this is among the last
few 6.3 regressions I track. Hence please allow me to ask:

Pierre, Tomas, Javier, et. al: how many "legacy BIOSes" do we suspect
are affected by this? So many that it might be worth delaying the
release by one week? And in case everybody involved might agree that
this patch is ready by today or tomorrow: might it be worth asking Linus
to merge this patch directly[1]?

[FWIW, I highly suspect the answer to the last two questions is "no,
that's definitely not worth is", just wanted to confirm]

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

[1] yes, that's a thing we do:
https://lore.kernel.org/all/CAHk-=wis_qQy4oDNynNKi5b7Qhosmxtoj1jxo5wmB6SRUwQUBQ@mail.gmail.com/

Re: [PATCH v3] firmware/sysfb: Fix VESA format selection

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2666 bytes --]

Hi

Am 21.04.23 um 13:32 schrieb Linux regression tracking (Thorsten Leemhuis):
> On 20.04.23 17:57, Pierre Asselin wrote:
>> Some legacy BIOSes report no reserved bits in their 32-bit rgb mode,
>> breaking the calculation of bits_per_pixel in commit f35cd3fa7729
>> ("firmware/sysfb: Fix EFI/VESA format selection").  However they report
>> lfb_depth correctly for those modes.  Keep the computation but
>> set bits_per_pixel to lfb_depth if the latter is larger.
>>
>> v2 fixes the warnings from a max3() macro with arguments of different
>> types;  split the bits_per_pixel assignment to avoid uglyfing the code
>> with too many casts.
>>
>> v3 fixes space and formatting blips pointed out by Javier, and change
>> the bit_per_pixel assignment back to a single statement using two casts.
>>
>> Link: https://lore.kernel.org/r/4Psm6B6Lqkz1QXM@panix3.panix.com
>> Link: https://lore.kernel.org/r/20230412150225.3757223-1-javierm@redhat.com
>> Link: https://lore.kernel.org/dri-devel/20230418183325.2327-1-pa@panix.com/T/#u
>> Link: https://lore.kernel.org/dri-devel/20230419044834.10816-1-pa@panix.com/T/#u
>> Fixes: f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
>> Signed-off-by: Pierre Asselin <pa@panix.com>
> 
> Linus might release the final this weekend and this is among the last
> few 6.3 regressions I track. Hence please allow me to ask:
> 
> Pierre, Tomas, Javier, et. al: how many "legacy BIOSes" do we suspect
> are affected by this? So many that it might be worth delaying the
> release by one week? And in case everybody involved might agree that
> this patch is ready by today or tomorrow: might it be worth asking Linus
> to merge this patch directly[1]?
> 
> [FWIW, I highly suspect the answer to the last two questions is "no,
> that's definitely not worth is", just wanted to confirm]

IMHO it's a fairly obscure bug and certainly not a release blocker. I'll 
send it through the regular channels of the DRM subsystem.

Best regards
Thomas

> 
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
> 
> [1] yes, that's a thing we do:
> https://lore.kernel.org/all/CAHk-=wis_qQy4oDNynNKi5b7Qhosmxtoj1jxo5wmB6SRUwQUBQ@mail.gmail.com/

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v3] firmware/sysfb: Fix VESA format selection

[Pierre Asselin]

Thomas Zimmerman writes:
>
> Am 21.04.23 um 13:32 schrieb Linux regression tracking (Thorsten
> Leemhuis):
>>
>> Pierre, Tomas, Javier, et. al: how many "legacy BIOSes" do we suspect
>> are affected by this?

So far, two:
1) my Gateway laptop (Intel(R) Core(TM) Duo CPU,
   Phoenix BIOS 83.08 Revision: 131.8 Release Date: 03/06/07)
2) my Dell Precision T3610 (Intel(R) Xeon(R) CPU E5-1650,
   Dell BIOS A19 Revision: 65.19 Release Date: 09/11/2019)

I don't know how to give a more precise description.

>> might it be worth asking Linus to merge this patch directly[1]?
>
> IMHO it's a fairly obscure bug and certainly not a release blocker. I'll
> send it through the regular channels of the DRM subsystem.

I agree.  Even when the regression bites, it's not a blocker.  The screen
is strange but readable.

--PA

Re: [PATCH v3] firmware/sysfb: Fix VESA format selection

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3189 bytes --]

Hi

Am 20.04.23 um 17:57 schrieb Pierre Asselin:
> Some legacy BIOSes report no reserved bits in their 32-bit rgb mode,
> breaking the calculation of bits_per_pixel in commit f35cd3fa7729
> ("firmware/sysfb: Fix EFI/VESA format selection").  However they report
> lfb_depth correctly for those modes.  Keep the computation but
> set bits_per_pixel to lfb_depth if the latter is larger.
> 
> v2 fixes the warnings from a max3() macro with arguments of different
> types;  split the bits_per_pixel assignment to avoid uglyfing the code
> with too many casts.
> 
> v3 fixes space and formatting blips pointed out by Javier, and change
> the bit_per_pixel assignment back to a single statement using two casts.
> 
> Link: https://lore.kernel.org/r/4Psm6B6Lqkz1QXM@panix3.panix.com
> Link: https://lore.kernel.org/r/20230412150225.3757223-1-javierm@redhat.com
> Link: https://lore.kernel.org/dri-devel/20230418183325.2327-1-pa@panix.com/T/#u
> Link: https://lore.kernel.org/dri-devel/20230419044834.10816-1-pa@panix.com/T/#u
> Fixes: f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
> Signed-off-by: Pierre Asselin <pa@panix.com>
> ---
>   drivers/firmware/sysfb_simplefb.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
> index 82c64cb9f531..6f7c5d0c5090 100644
> --- a/drivers/firmware/sysfb_simplefb.c
> +++ b/drivers/firmware/sysfb_simplefb.c
> @@ -51,15 +51,18 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
>   	 *
>   	 * It's not easily possible to fix this in struct screen_info,
>   	 * as this could break UAPI. The best solution is to compute
> -	 * bits_per_pixel here and ignore lfb_depth. In the loop below,
> +	 * bits_per_pixel from the color bits, reserved bits and
> +	 * reported lfb_depth, whichever is highest.  In the loop below,
>   	 * ignore simplefb formats with alpha bits, as EFI and VESA
>   	 * don't specify alpha channels.
>   	 */
>   	if (si->lfb_depth > 8) {
> -		bits_per_pixel = max(max3(si->red_size + si->red_pos,
> -					  si->green_size + si->green_pos,
> -					  si->blue_size + si->blue_pos),
> -				     si->rsvd_size + si->rsvd_pos);
> +		/* max() macros args should be of the same type */
> +		bits_per_pixel = max3((u16)max3(si->red_size + si->red_pos,
> +					si->green_size + si->green_pos,
> +					si->blue_size + si->blue_pos),
> +				(u16)(si->rsvd_size + si->rsvd_pos),
> +				si->lfb_depth);

I found this casting mess even more unreadable. I went back to v2, fixed 
the style issues and committed the patch as v4 (still under your name).

 
https://cgit.freedesktop.org/drm/drm-tip/commit/?id=1b617bc93178912fa36f87a957c15d1f1708c299

Thanks a lot for the bugfix.

Best regard
Thomas

>   	} else {
>   		bits_per_pixel = si->lfb_depth;
>   	}
> 
> base-commit: 6a8f57ae2eb07ab39a6f0ccad60c760743051026

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v3] firmware/sysfb: Fix VESA format selection

[Pierre Asselin]

Thomas Zimmerman <tzimmermann@suse.de> writes:
>
> I found this casting mess even more unreadable. I went back to v2, fixed
> the style issues and committed the patch as v4 (still under your name).
>
> https://cgit.freedesktop.org/drm/drm-tip/commit?id=1b617bc93178912fa36f87a957c15d1f1708c299

Will this patch make it into Linux 6.4 ?

--PA

Re: [PATCH v3] firmware/sysfb: Fix VESA format selection

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 853 bytes --]

Hi

Am 10.05.23 um 19:25 schrieb Pierre Asselin:
> Thomas Zimmerman <tzimmermann@suse.de> writes:
>>
>> I found this casting mess even more unreadable. I went back to v2, fixed
>> the style issues and committed the patch as v4 (still under your name).
>>
>> https://cgit.freedesktop.org/drm/drm-tip/commit?id=1b617bc93178912fa36f87a957c15d1f1708c299
> 
> Will this patch make it into Linux 6.4 ?

It appears to be stuck in the drm-misc-fixes tree. The time around the 
merge window is always complicated. I've send out a reminder to the 
maintainers to fetch the patch.

Best regards
Thomas

> 
> --PA
> 

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]