Hi Am 01.12.25 um 10:20 schrieb Tvrtko Ursulin:
On 01/12/2025 07:32, Thomas Zimmermann wrote:Hi Am 29.11.25 um 11:44 schrieb Tvrtko Ursulin:On 28/11/2025 17:07, Thomas Zimmermann wrote:Hi, thanks for the bug report Am 28.11.25 um 16:04 schrieb Tvrtko Ursulin:I am not sure how is simpledrmfb on top of EFI supposed to work, but at least at the moment it appears there is a missing link in the "discovery"of frame buffer parameters.What I can see is that EFI GOP reads some parameters from the firmware andinfers the other, such as in this case problematic pitch, or stride.The pitch/stride value comes from the firmware via pixels_per_scanline [1].Can you verify that this value is really 800 instead of 832 (eq 3328 bytes) ?[1] https://elixir.bootlin.com/linux/v6.17.9/source/drivers/firmware/ efi/libstub/gop.c#L493I actually got confused a bit in following the flow so thank you for asking me to double check.GOP actually reports 1280x800 with a stride of 5120. So it kind of reports a rotated view already, kind of.These are correct values.But the stream deck is this device: [1], right? It uses landscape-mode orientation. Why does it require rotation at all?[1] https://de.wikipedia.org/wiki/Steam_Deck#/media/ Datei:Steam_Deck_(front).pngThat's the device yes. For the user the screen is landscape, but the actual panel is 800x1280 portrait. Left edge is top of the display. (Hence the pre-existing entry in drm_get_panel_orientation_quirk.)
I see. So the EFI display settings are configured as if this was a landscape panel.
What happens if you leave the EFI settings as-is and simply remove the panel-orientation quirk?
You mean declare it a firmware bug and live with the corrupt console until the final fb driver takes over?Only when the rotation quirk from efifb_dmi_swap_width_height triggers the stride gets incorrectly recalculated:u16 temp = screen_info.lfb_width; screen_info.lfb_width = screen_info.lfb_height; screen_info.lfb_height = temp; screen_info.lfb_linelength = 4 * screen_info.lfb_width;So this is where things go wrong, well, they actually go wrong a little bit even earlier, in gop.c:si->lfb_size = si->lfb_linelength * si->lfb_height;Which potentially underestimates the fb size. If GOP was forward looking enough to give us the size we could derive the pitch based on size..Anyway, as it stands it looks a quirk in sysfb_apply_efi_quirks looks it is required to fix it all up.I am a bit uneasy about declaring the fb size larger than what was implied by firmware provided pitch * height * depth but limited to a specific DMI match and if it looks visually okay I think it is a safe assumption the quirked size is actually correct and safe.Yeah, we better not do that.
I only mean that we should not use more video memory than provided by EFI. Best regards Thomas
Regards, TvrtkoOne could be easily excused in thinking that pitch cannot be reliably inferred, since different display hardware has differing alignmentrequirements, so it is unclear how is hardware agnostic solution supposedto work.In the specific case of the Steam Deck hardware we have a 800x1280 nativepanel which is also installed rotated 90 degrees counter clockwise. Firmware appears to set up the pitch as 3328, while GOP assumes 3200, based of a width * bpp calculation.832 is a multiple of 64, while 800 is not. I've seen this over- allocation in other context as well.When this incorrect pitch propagates through (rather complicated) fbcon and DRM call paths, the end result is corrupted rendering all until theamdgpu takes over the fbdev. Simplistic solution in this patch is to add a DMI quirk to the EFI frame buffer setup code.Apart from the incorrect pitch, the quirk also does the swapping of the width and height. Apart from setting the correct fbcon dimensions thisone also allows the quirk from drm_get_panel_orientation_quirk() to report the correct orientation.NiceSigned-off-by: Tvrtko Ursulin <[email protected]> Cc: Thomas Zimmermann <[email protected]> Cc: Javier Martinez Canillas <[email protected]> Cc: Ard Biesheuvel <[email protected]> Cc: Alex Deucher <[email protected]> Cc: "Christian König" <[email protected]> Cc: Melissa Wen <[email protected]> Cc: Rodrigo Siqueira <[email protected]> Cc: Mario Limonciello <[email protected]> Cc: [email protected] ---drivers/firmware/efi/sysfb_efi.c | 48 ++++++++++++++++++++++++++++ ++--1 file changed, 46 insertions(+), 2 deletions(-)diff --git a/drivers/firmware/efi/sysfb_efi.c b/drivers/firmware/ efi/ sysfb_efi.cindex 1e509595ac03..84d9049bb2cb 100644 --- a/drivers/firmware/efi/sysfb_efi.c +++ b/drivers/firmware/efi/sysfb_efi.c@@ -231,6 +231,18 @@ static const struct dmi_system_id efifb_dmi_system_table[] __initconst = {{}, }; +struct efifb_mode_fixup { + unsigned int width; + unsigned int height; + unsigned int pitch; +}; + +static const struct efifb_mode_fixup efifb_steamdeck_mode_fixup = { + .width = 1280, + .height = 800, + .pitch = 3328, +}; + /** Some devices have a portrait LCD but advertise a landscape resolution (and * pitch). We simply swap width and height for these devices so that we can @@ -281,6 +293,24 @@ static const struct dmi_system_id efifb_dmi_swap_width_height[] __initconst = {DMI_MATCH(DMI_PRODUCT_NAME, "Lenovo YB1-X91"), }, }, + { + /* Valve Steam Deck (Jupiter) */ + .matches = { + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Valve"), + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Jupiter"), + DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "1"), + }, + .driver_data = (void *)&efifb_steamdeck_mode_fixup, + }, + { + /* Valve Steam Deck (Galileo) */ + .matches = { + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Valve"), + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Galileo"), + DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "1"), + }, + .driver_data = (void *)&efifb_steamdeck_mode_fixup, + }, {}, }; @@ -351,17 +381,31 @@ static struct fwnode_handle efifb_fwnode; __init void sysfb_apply_efi_quirks(void) { + const struct dmi_system_id *match; + if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI || !(screen_info.capabilities & VIDEO_CAPABILITY_SKIP_QUIRKS)) dmi_check_system(efifb_dmi_system_table); - if (screen_info.orig_video_isVGA == VIDEO_TYPE_EFI && - dmi_check_system(efifb_dmi_swap_width_height)) {Could we keep swap_width_height a separate list? The for loop would then be a full override-display list.+ if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI) + return; + + for (match = dmi_first_match(efifb_dmi_swap_width_height); + match; + match = dmi_first_match(match + 1)) { + const struct efifb_mode_fixup *data = match->driver_data; u16 temp = screen_info.lfb_width; screen_info.lfb_width = screen_info.lfb_height; screen_info.lfb_height = temp; screen_info.lfb_linelength = 4 * screen_info.lfb_width;FTR: This calculation has always been wrong. We need to take the lfb_depth into account. No need to fix this here; just so you know.I can add a patch to replace with screen_info.lfb_depth / BITS_PER_BYTE?+ + if (data && data->pitch && + data->width == screen_info.lfb_height && + data->height == screen_info.lfb_width) { + screen_info.lfb_linelength = data->pitch; + screen_info.lfb_size = data->pitch * data->width; + }If we have a separate fix-up list, we can make this much nicer: if (data->width) // assign data->width if (data->height) // assign data->width if (data->data) // assign data->pitch and then recompute linelength and sizeI thought the approach of looking at the multiple parameters at the same time, similar how drm_get_panel_orientation_quirk() does it, is safer. Ie. we don't want to edit unless the whole mode matches the expectation.Regards, Tvrtko
-- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
