On 02/12/2025 07:34, Thomas Zimmermann wrote:
Hi
Am 01.12.25 um 16:43 schrieb Tvrtko Ursulin:
On 01/12/2025 15:00, Ard Biesheuvel wrote:
On Mon, 1 Dec 2025 at 11:33, Tvrtko Ursulin
<[email protected]> wrote:
On 01/12/2025 10:18, Ard Biesheuvel wrote:
On Mon, 1 Dec 2025 at 11:03, Tvrtko Ursulin
<[email protected]> wrote:
On 01/12/2025 09:39, Thomas Zimmermann wrote:
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 and
infers 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#L493
I 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).png
That'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?
That would create effectively the same situation as without my patch
because the panel-orientation quirk does not trigger unless detected
screen is 800x1280. Result is corrupted console since fbcon thinks
it is
a landscape 1280x800 screen.
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.
You mean declare it a firmware bug and live with the corrupt
console
until the final fb driver takes over?
I only mean that we should not use more video memory than
provided by EFI.
Right, but that information is not available in the GOP, right?
Ie. as I
wrote above it appears assumed:
si->lfb_size = si->lfb_linelength * si->lfb_height;
Do we have any other options apart from corruption or assume firmware
configured GOP screen info incorrectly?
How does it make sense to recalculate the line length? Those invisible
pixels at the end of the scanline are not going to be transposed to
the other dimension, right?
Not sure what you meant here. The line above is from gop.c and the
context is that GOP screen info appears to not carry the frame buffer
size in bytes so it is implied.
Elsewhere in the patch I quirk the pitch to the correct value so
rotated
rendering is correct.
But the corrected pitch also means that in principle we need to adjust
the frame buffer size, since it is larger than the size implied with
the
incorrect pitch.
OK, so if I understand all of the above correctly, you have a 800x1280
panel with 832 pixels per scanline, right? And the 5120 pitch is
simply bogus, but needed to maintain the fiction that the panel is
1280 pixels wide, and so the resulting lfb_size is bogus too?
Since we know that the PixelsPerScanline value is incorrect, I don't
think there is any point in attempting to cross reference this against
other firmware provided data. But it would make sense imho to apply
the quirk only if the exact combination of incorrect values (i.e.,
1280x800/5120) is encountered.
Right, the whole 1280x800 mode I *think* could be "bogus", that is,
some kind of a software rotated mode implemented by the firmware.
Default mode is 800x1280 (pitch 832), while this second native
resolution mode is 1280x800 (pitch 1280).
If default mode is left then both simpledrmfb and efidrmfb work fine.
The existing panel orientation quirk will trigger on 800x1280 and tell
fbcon to rotate.
But if someone, like for example grub2, changed the mode to this
software rotated one then the existing DRM quirk will not work.
So this is a bug in grub? Should it supply the original mode?
Apologies for only asking dump questions here. I find this very confusing.
Not at all, it is complicated and open whether it is worth improving.
I don't think it is a grub bug. To me it seems like an unfortunate
consequence of protocol limitations.
In the correct mode 800x1280, the first native pixel should be on the
lower left corner. and the second pixel should be 'up form it'. And
because it's marked as rotated CCW, fbcon adapts correctly.
If the display is in the bogus mode 1280x800, in which direction does it
draw by default? The framebuffer's first pixel should still be in one
of the corners. And the second pixel is nearby. In which direction does
it advance?
I am not a grub expert, but I had a brief look at its codebase, and
AFAICT it draws by software rendering into a shadow frame buffer and
calls GOP->Blit to update the screen.
As such my assumption is that with the fake 1280x800 mode, firmware
implements the blit to rotate under the hood.
So for grub everything is fine. It sets 1280x800, sees 1280x800, renders
the menu in there, and courtesy of firmware blit the menu is presented
in the human friendly orientation.
Note this is all when grub is asked to change the mode to 1280x800. This
does not happen on the normal SteamOS boot, but only if grub menu was
entered. Otherwise mode stays at the default 800x1280 and the DRM quirk
triggers to set the orientation both for the simple DRM fb, efidrmfb and
later the real amdgpu drm fd.
In other words without this patch fbcon is correctly rotated when the
mode is default 800x1280, but broken if grub menu is entered which sets
the mode to 1280x800. Because the DRM panel orientation quirk only
triggers for 800x1280.
Hence this patch/quirk to fudge the screen info back into the native
mode and allow the orientation quirk to always work and fbcon always be
right.
Questionable part is that it only fixes the user experience for people
who enter the boot menu, so probably only developers. And the broken
fbcon is only until amdgpu takes it over, which correctly probes the
mode, I guess from the display state, and always sees it as 800x1280 and
so panel orientation quirk matches.
I suppose for DRM panic before amdgpu loads, or before it fully
initializes, it would be worth having an readable fbcon, but again, I
accept it is probably a border line case so if people do not want to
take this quirk then it is what it is.
Regards,
Tvrtko
The quirk in this patch therefore proposes to correct back the mode to
the default native.
You are indeed right that the criteria needs to be tweaked. In v2 I've
fixed and it now looks like this:
...
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;
if (!data ||
(data->width == screen_info.lfb_width &&
data->height == screen_info.lfb_height)) {
screen_info.lfb_width = screen_info.lfb_height;
screen_info.lfb_height = temp;
There's a swap() macro BTW. [1]
[1] https://elixir.bootlin.com/linux/v6.18/source/include/linux/
minmax.h#L307
Best regards
Thomas
if (data && data->pitch) {
screen_info.lfb_linelength = data->pitch;
screen_info.lfb_size = data->pitch * data->width;
} else {
screen_info.lfb_linelength = 4 * screen_info.lfb_width;
}
}
}
...
Ie. only swap width<->height for the pre-existing quirks and the new
quirk *if* it is in 1280x800.
Regards,
Tvrtko
