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

[Thomas Zimmermann]

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@xxxxxxxxxxxxxxxx
> Link: https://lore.kernel.org/r/20230412150225.3757223-1-javierm@xxxxxxxxxx
> Fixes: f35cd3fa7729 [firmware/sysfb: Fix EFI/VESA format selection]
> Signed-off-by: Pierre Asselin <pa@xxxxxxxxx>
> ---
>   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: OpenPGP_signature
Description: OpenPGP digital signature