| Message ID | 20230331095946.45024-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gfx: improvements when using multiboot2 and EFI | expand |
On 31.03.2023 11:59, Roger Pau Monne wrote: > @@ -887,6 +881,15 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable > > efi_arch_edid(gop_handle); > } > + else > + { > + /* If no GOP, init ConOut (StdOut) to the max supported size. */ > + efi_console_set_mode(); > + > + if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, > + &cols, &rows) == EFI_SUCCESS ) > + efi_arch_console_init(cols, rows); > + } Instead of making this an "else", wouldn't you better check that a valid gop_mode was found? efi_find_gop_mode() can return ~0 after all. Furthermore, what if the active mode doesn't support text output? (I consider the spec unclear in regard to whether this is possible, but maybe I simply didn't find the right place stating it.) Finally I think efi_arch_console_init() wants calling nevertheless. So altogether maybe if ( gop_mode == ~0 || StdOut->QueryMode(StdOut, StdOut->Mode->Mode, &cols, &rows) != EFI_SUCCESS ) /* If no usable GOP mode, init ConOut (StdOut) to the max supported size. */ efi_console_set_mode(); if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, &cols, &rows) == EFI_SUCCESS ) efi_arch_console_init(cols, rows); ? Jan
On Wed, Apr 05, 2023 at 12:36:55PM +0200, Jan Beulich wrote: > On 31.03.2023 11:59, Roger Pau Monne wrote: > > @@ -887,6 +881,15 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable > > > > efi_arch_edid(gop_handle); > > } > > + else > > + { > > + /* If no GOP, init ConOut (StdOut) to the max supported size. */ > > + efi_console_set_mode(); > > + > > + if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, > > + &cols, &rows) == EFI_SUCCESS ) > > + efi_arch_console_init(cols, rows); > > + } > > Instead of making this an "else", wouldn't you better check that a > valid gop_mode was found? efi_find_gop_mode() can return ~0 after all. When using vga=current gop_mode would also be ~0, in order for efi_set_gop_mode() to not change the current mode, I was trying to avoid exposing keep_current or similar extra variable to signal this. > Furthermore, what if the active mode doesn't support text output? (I > consider the spec unclear in regard to whether this is possible, but > maybe I simply didn't find the right place stating it.) > > Finally I think efi_arch_console_init() wants calling nevertheless. > > So altogether maybe > > if ( gop_mode == ~0 || > StdOut->QueryMode(StdOut, StdOut->Mode->Mode, > &cols, &rows) != EFI_SUCCESS ) I think it would make more sense to call efi_console_set_mode() only if the current StdOut mode is not valid, as anything different from vga=current will already force a GOP mode change. Thanks, Roger.
On 31.05.2023 12:57, Roger Pau Monné wrote: > On Wed, Apr 05, 2023 at 12:36:55PM +0200, Jan Beulich wrote: >> On 31.03.2023 11:59, Roger Pau Monne wrote: >>> @@ -887,6 +881,15 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable >>> >>> efi_arch_edid(gop_handle); >>> } >>> + else >>> + { >>> + /* If no GOP, init ConOut (StdOut) to the max supported size. */ >>> + efi_console_set_mode(); >>> + >>> + if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, >>> + &cols, &rows) == EFI_SUCCESS ) >>> + efi_arch_console_init(cols, rows); >>> + } >> >> Instead of making this an "else", wouldn't you better check that a >> valid gop_mode was found? efi_find_gop_mode() can return ~0 after all. > > When using vga=current gop_mode would also be ~0, in order for > efi_set_gop_mode() to not change the current mode, And then we'd skip efi_console_set_mode() here as well, which I think is what we want with "vga=current"? > I was trying to > avoid exposing keep_current or similar extra variable to signal this. > >> Furthermore, what if the active mode doesn't support text output? (I >> consider the spec unclear in regard to whether this is possible, but >> maybe I simply didn't find the right place stating it.) >> >> Finally I think efi_arch_console_init() wants calling nevertheless. >> >> So altogether maybe >> >> if ( gop_mode == ~0 || >> StdOut->QueryMode(StdOut, StdOut->Mode->Mode, >> &cols, &rows) != EFI_SUCCESS ) > > I think it would make more sense to call efi_console_set_mode() only > if the current StdOut mode is not valid, as anything different from > vga=current will already force a GOP mode change. Hmm, this may also make sense. I guess I'd like to see the combined result to be better able to judge. Jan
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index f46c1f021e..42aa135446 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -820,12 +820,6 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable efi_init(ImageHandle, SystemTable); - efi_console_set_mode(); - - if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, - &cols, &rows) == EFI_SUCCESS ) - efi_arch_console_init(cols, rows); - gop = efi_get_gop(&gop_handle); if ( gop ) @@ -887,6 +881,15 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable efi_arch_edid(gop_handle); } + else + { + /* If no GOP, init ConOut (StdOut) to the max supported size. */ + efi_console_set_mode(); + + if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, + &cols, &rows) == EFI_SUCCESS ) + efi_arch_console_init(cols, rows); + } efi_arch_edd(); efi_arch_cpu();
Only initialize StdOut if there's no GOP available. This avoids forcefully switching StdOut to the maximum supported resolution, and thus very likely changing the GOP mode without having first parsed the command line options. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - New in this version. --- xen/arch/x86/efi/efi-boot.h | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)