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

[PATCH v2] 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 typecasts.

Link: https://lore.kernel.org/r/4Psm6B6Lqkz1QXM@panix3.panix.com
Link: https://lore.kernel.org/r/20230412150225.3757223-1-javierm@redhat.com
Fixes: f35cd3fa7729 [firmware/sysfb: Fix EFI/VESA format selection]
Signed-off-by: Pierre Asselin <pa@panix.com>
---
 drivers/firmware/sysfb_simplefb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
index 82c64cb9f531..358b792a8845 100644
--- a/drivers/firmware/sysfb_simplefb.c
+++ b/drivers/firmware/sysfb_simplefb.c
@@ -51,7 +51,8 @@ __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.
 	 */
@@ -60,6 +61,7 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
 					  si->green_size + si->green_pos,
 					  si->blue_size + si->blue_pos),
 				     si->rsvd_size + si->rsvd_pos);
+		bits_per_pixel= max(bits_per_pixel, (u32)si->lfb_depth);
 	} else {
 		bits_per_pixel = si->lfb_depth;
 	}

base-commit: 6a8f57ae2eb07ab39a6f0ccad60c760743051026
-- 
2.39.2

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

[Thomas Zimmermann]

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

Hi

Am 19.04.23 um 06:48 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 typecasts.

What exactly was that warning?

> 
> Link: https://lore.kernel.org/r/4Psm6B6Lqkz1QXM@panix3.panix.com
> Link: https://lore.kernel.org/r/20230412150225.3757223-1-javierm@redhat.com
> Fixes: f35cd3fa7729 [firmware/sysfb: Fix EFI/VESA format selection]
> Signed-off-by: Pierre Asselin <pa@panix.com>
> ---
>   drivers/firmware/sysfb_simplefb.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
> index 82c64cb9f531..358b792a8845 100644
> --- a/drivers/firmware/sysfb_simplefb.c
> +++ b/drivers/firmware/sysfb_simplefb.c
> @@ -51,7 +51,8 @@ __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.
>   	 */
> @@ -60,6 +61,7 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
>   					  si->green_size + si->green_pos,
>   					  si->blue_size + si->blue_pos),
>   				     si->rsvd_size + si->rsvd_pos);
> +		bits_per_pixel= max(bits_per_pixel, (u32)si->lfb_depth);

I liked the all-in-one assignment of the original patch. So I'd rather 
go back to v1 and copy si->lfb_depth to the correct type, like this:

   u32 depth = si->lfb_depth;

   bits_per_pixel = max3(max3(colors),
   		        rsvd,
                         depth);

Or, if you want to get fancy, you could add max3_t() to <linux/minmax.h>

   #define max3_t(type, x, y, z)   max_t(type, max_t(type, x, y), z)

and do

   bits_per_pixel = max3_t(u32,
                           max3(colors),
                           rsvd,
                           si->lfb_depth)

You could also add a max4_t(type, x, y, z, w) to <linux/minmax.h> and 
compare all values with max4_t().

Best regards
Thomas


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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

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

[Pierre Asselin]

Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 19.04.23 um 06:48 schrieb Pierre Asselin:
>>
>> 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 typecasts.
>
> What exactly was that warning?

A friendly note from a robot; make W=1 sysfb_simplefb.o .
https://lore.kernel.org/dri-devel/20230418183325.2327-1-pa@panix.com/T/#m38e859354329ab9f756da91e99b546e3b140fa91


> I liked the all-in-one assignment of the original patch. So I'd rather
> go back to v1 and copy si->lfb_depth to the correct type, like this:
>
>    u32 depth = si->lfb_depth;
>    bits_per_pixel = max3(max3(colors),
>    		        rsvd,
>                          depth);

Would that work?  If I understand correctly max3() checks that all args
have the same type.  {red,green,blue,rsvd}.{size,pos} are all u8 while
lfb_depth is u16.  The best I can do is

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

That compiles quietly with W=1 but those two casts are ugly.
If I do that, would K&R-on-parentheses read better ?

    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 think it's clearer, but not kernel style and still ugly.

> Or, if you want to get fancy, you could add max3_t() to <linux/minmax.h>
>
>    #define max3_t(type, x, y, z)   max_t(type, max_t(type, x, y), z)
>
> and do
>
>    bits_per_pixel = max3_t(u32,
>                            max3(colors),
>                            rsvd,
>                            si->lfb_depth)
>
> You could also add a max4_t(type, x, y, z, w) to <linux/minmax.h> and
> compare all values with max4_t().

That would be a two-patch series.  I'd rather keep it to the strict
minimum that fixes the regression.  (You trust me to even *look* at a
kernel header and not break it ?  Dangerous assumption!)

I'm new at this.  Two months ago I didn't know what to type a the
command line after "git".

Incidentally, should I send v3 as a new email or reply to the chain?

--PA

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

[Thomas Zimmermann]

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

Hi

Am 19.04.23 um 22:27 schrieb Pierre Asselin:
> Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 19.04.23 um 06:48 schrieb Pierre Asselin:
>>>
>>> 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 typecasts.
>>
>> What exactly was that warning?
> 
> A friendly note from a robot; make W=1 sysfb_simplefb.o .
> https://lore.kernel.org/dri-devel/20230418183325.2327-1-pa@panix.com/T/#m38e859354329ab9f756da91e99b546e3b140fa91
> 
> 
>> I liked the all-in-one assignment of the original patch. So I'd rather
>> go back to v1 and copy si->lfb_depth to the correct type, like this:
>>
>>     u32 depth = si->lfb_depth;
>>     bits_per_pixel = max3(max3(colors),
>>     		        rsvd,
>>                           depth);
> 
> Would that work?  If I understand correctly max3() checks that all args
> have the same type.  {red,green,blue,rsvd}.{size,pos} are all u8 while
> lfb_depth is u16.  The best I can do is

Maybe make the depth variable a u8 then with a clamp_t()-based cast there:

   u8 depth = clamp_t(u8, si->lfb_depth, 1, 32);

There's currently no way that lfb_depth would be outside the [1, 32] range.

> 
>      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);
> 
> That compiles quietly with W=1 but those two casts are ugly.
> If I do that, would K&R-on-parentheses read better ?
> 
>      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 think it's clearer, but not kernel style and still ugly.
> 
>> Or, if you want to get fancy, you could add max3_t() to <linux/minmax.h>
>>
>>     #define max3_t(type, x, y, z)   max_t(type, max_t(type, x, y), z)
>>
>> and do
>>
>>     bits_per_pixel = max3_t(u32,
>>                             max3(colors),
>>                             rsvd,
>>                             si->lfb_depth)
>>
>> You could also add a max4_t(type, x, y, z, w) to <linux/minmax.h> and
>> compare all values with max4_t().
> 
> That would be a two-patch series.  I'd rather keep it to the strict
> minimum that fixes the regression.  (You trust me to even *look* at a
> kernel header and not break it ?  Dangerous assumption!)
> 
> I'm new at this.  Two months ago I didn't know what to type a the
> command line after "git".

Welcome to the kernel community. :)

> 
> Incidentally, should I send v3 as a new email or reply to the chain?

As a new mail, please. It's easier for readers and tools.

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

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

[Javier Martinez Canillas]

Pierre Asselin <pa@panix.com> writes:

Hello Pierre,

> 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 typecasts.
>
> Link: https://lore.kernel.org/r/4Psm6B6Lqkz1QXM@panix3.panix.com
> Link: https://lore.kernel.org/r/20230412150225.3757223-1-javierm@redhat.com
> Fixes: f35cd3fa7729 [firmware/sysfb: Fix EFI/VESA format selection]

The convention is f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")

> Signed-off-by: Pierre Asselin <pa@panix.com>
> ---

[...]

> +		bits_per_pixel= max(bits_per_pixel, (u32)si->lfb_depth);

You are missing a space here.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Pierre Asselin]

Javier Martinez Canillas <javierm@redhat.com> writes:
> Pierre Asselin <pa@panix.com> writes:

>> Fixes: f35cd3fa7729 [firmware/sysfb: Fix EFI/VESA format selection]
>
> The convention is f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format
> selection")

>> +		bits_per_pixel= max(bits_per_pixel, (u32)si->lfb_depth);
>
> You are missing a space here.

OK.  I'll fix that.  Thanks.

--PA