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

[Thomas Zimmermann <tzimmermann@suse.de>]

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

ping

Am 12.04.23 um 13:22 schrieb Javier Martinez Canillas:
> "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

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