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

[Javier Martinez Canillas <javierm@redhat.com>]

"Pierre Asselin" <pa@panix.com> writes:

>> Can you please share you grub config file? It seems that is set to
>> GRUB_GFXMODE=1024x768x32 but the actual mode is set to 1024x768x24 ?
>
> Okay, but you'll be sorry...  The gfxmode is set to "keep" in all the
> entries.  https://www.panix.com/~pa/linux-6.3-simplefb/grub.cfg .
>
> The "TEST" entry was used to bisect.  The "PRE-TEST" was to set things
> up, to receive the bzImages compiled on a faster machine. Now I boot
> the "Linux 6.3.0-rc5-x86".
>
>
>> That is, it fails when the picked format is DRM_FORMAT_RGB8888 but
>> works when is DRM_FORMAT_XRGB888. I can't spot any error in Thomas'
>> patch so I wonder if the problem is with what grub is passing to the
>> kernel.
>>
>> The mentioned vga=0x318 workaround that you mentioned makes the mode
>> passed to match the selected DRM_FORMAT_RGB888 which I guess is why
>> that worked for you.
>
> All right, I did a series of reboots, editing the grub command line
> to change the "gfxpayload=" grub option or the "vga=" kernel option.
> In each case I captured the output of
>   "dmesg | egrep -i 'fbcon|console|fb0|frameb|simple|vga|vesa'
>
> https://www.panix.com/~pa/linux-6.3-simplefb/selected-modes
>
> (D'oh.  My script printed "vga=vga=" twice when that option was set.
> Good enough as is.)
>
> Note the difference in linelength= between the bad and good r8g8b8.
> Does it mean anything ?
>  (bad)> format=r8g8b8, mode=1024x768x24, linelength=4096
> (good)> format=r8g8b8, mode=1024x768x24, linelength=3072
>

Ah! That's a good data point and I believe that found a possible issue in
the sysfb format selection logic. Can you please try the following patch?

From 55b5375c528b4128350dfa2126277049f8821349 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javierm@redhat.com>
Date: Wed, 12 Apr 2023 13:20:48 +0200
Subject: [PATCH] firmware/sysfb: Fix wrong stride when bits-per-pixel is
 calculated

The commit f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
fixed format selection by calculating the bits-per-pixel instead of just
using the reported color depth.

But unfortunately this broke some modes because the stride is always set
to the reported line length (in bytes), which could not match the actual
stride if the calculated bits-per-pixel doesn't match the reported depth.

Fixes: commit f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
Reported-by: Pierre Asselin <pa@panix.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/firmware/sysfb_simplefb.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
index 82c64cb9f531..5dc23e57089f 100644
--- a/drivers/firmware/sysfb_simplefb.c
+++ b/drivers/firmware/sysfb_simplefb.c
@@ -28,7 +28,7 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
 			     struct simplefb_platform_data *mode)
 {
 	__u8 type;
-	u32 bits_per_pixel;
+	u32 bits_per_pixel, stride;
 	unsigned int i;
 
 	type = si->orig_video_isVGA;
@@ -54,14 +54,19 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
 	 * bits_per_pixel here and ignore lfb_depth. In the loop below,
 	 * ignore simplefb formats with alpha bits, as EFI and VESA
 	 * don't specify alpha channels.
+	 *
+	 * If a calculated bits_per_pixel is used instead of lfb_depth,
+	 * then also ignore lfb_linelength and calculate the stride.
 	 */
 	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);
+		stride = DIV_ROUND_UP(si->lfb_width * bits_per_pixel, 8);
 	} else {
 		bits_per_pixel = si->lfb_depth;
+		stride = si->lfb_linelength;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
@@ -80,7 +85,7 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
 			mode->format = f->name;
 			mode->width = si->lfb_width;
 			mode->height = si->lfb_height;
-			mode->stride = si->lfb_linelength;
+			mode->stride = stride;
 			return true;
 		}
 	}

base-commit: fd35174e13f98f9232c4aa66689816731d34ca28
-- 

Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat