drivers/firmware/sysfb_simplefb.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
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.2Pierre 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
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
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
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
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)
© 2016 - 2025 Red Hat, Inc.