On Tue, 2 Dec 2025 at 12:17, Tvrtko Ursulin <[email protected]> wrote: > > > On 02/12/2025 10:56, Ard Biesheuvel wrote: > > On Tue, 2 Dec 2025 at 11:44, Tvrtko Ursulin <[email protected]> > > wrote: > >> > >> > >> 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. > >> > > > > Indeed - GRUB never draws into the EFI GOP linear frame buffer > > directly, and this seems to be what makes it work in this case. > > > > Could you boot with video=efifb:list on the kernel command line to see > > how the GOP modes are reported? I suppose the bug here is that the > > mode in question is not reported with PixelFormat==PixelBltOnly as it > > should be. > > No problem, I had this info already and the 1280x800 mode has the same > pixel format as all the rest - PIXEL_BGR_RESERVED_8BIT_PER_COLOR. > > If it was set to PIXEL_BLT_ONLY, there would be no screen info set up, > right? So neither simpledrmfb or efidrmfb, or even legacy efifb, would > probe ie. there would be no fbcon until amdgpu loads? >
No, but we could make the EFI stub switch to a different mode if the active mode is BltOnly.
