| Message ID | 20221123154525.63068-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gfx: improvements when using multiboot2 and EFI + misc | expand |
On 23.11.2022 16:45, Roger Pau Monne wrote: > Do not unconditionally set a mode in efi_console_set_mode(), do so > only if the currently set mode is not valid. You don't say why you want to do so. Furthermore ... > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -799,6 +799,11 @@ static void __init efi_console_set_mode(void) > UINTN cols, rows, size; > unsigned int best, i; > > + /* Only set a mode if the current one is not valid. */ > + if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, &cols, &rows) == > + EFI_SUCCESS ) > + return; ... it might be okay if you put such a check in efi_multiboot2(), but the call here from efi_start() is specifically guarded by a check of whether "-basevideo" was passed to xen.efi. This _may_ not be as relevant anymore today, but it certainly was 20 years ago (recall that we've inherited this code from a much older project of ours) - at that time EFI usually started in 80x25 text mode. And I think that even today when you end up launching xen.efi from the EFI shell, you'd be stuck with 80x25 text mode on at least some implementations. Overall, looking at (for now) just the titles of subsequent patches, I'm not convinced the change here is needed at all. Or if anything it may want to go at the end, taking action only when "vga=current" was specified. Jan
On Mon, Dec 05, 2022 at 03:19:13PM +0100, Jan Beulich wrote: > On 23.11.2022 16:45, Roger Pau Monne wrote: > > Do not unconditionally set a mode in efi_console_set_mode(), do so > > only if the currently set mode is not valid. > > You don't say why you want to do so. Furthermore ... > > > --- a/xen/common/efi/boot.c > > +++ b/xen/common/efi/boot.c > > @@ -799,6 +799,11 @@ static void __init efi_console_set_mode(void) > > UINTN cols, rows, size; > > unsigned int best, i; > > > > + /* Only set a mode if the current one is not valid. */ > > + if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, &cols, &rows) == > > + EFI_SUCCESS ) > > + return; > > ... it might be okay if you put such a check in efi_multiboot2(), but > the call here from efi_start() is specifically guarded by a check of > whether "-basevideo" was passed to xen.efi. This _may_ not be as > relevant anymore today, but it certainly was 20 years ago (recall > that we've inherited this code from a much older project of ours) - > at that time EFI usually started in 80x25 text mode. And I think that > even today when you end up launching xen.efi from the EFI shell, > you'd be stuck with 80x25 text mode on at least some implementations. Won't you use console=vga vga=gfx-... To switch to a best mode? > Overall, looking at (for now) just the titles of subsequent patches, > I'm not convinced the change here is needed at all. Or if anything it > may want to go at the end, taking action only when "vga=current" was > specified. I guess I'm slightly confused by the usage of both GOP and StdOut, I would assume if we have a gop, and can correctly initialize it there's no need to fiddle with StdOut also? Thanks, Roger.
On 30.03.2023 17:44, Roger Pau Monné wrote: > On Mon, Dec 05, 2022 at 03:19:13PM +0100, Jan Beulich wrote: >> On 23.11.2022 16:45, Roger Pau Monne wrote: >>> Do not unconditionally set a mode in efi_console_set_mode(), do so >>> only if the currently set mode is not valid. >> >> You don't say why you want to do so. Furthermore ... >> >>> --- a/xen/common/efi/boot.c >>> +++ b/xen/common/efi/boot.c >>> @@ -799,6 +799,11 @@ static void __init efi_console_set_mode(void) >>> UINTN cols, rows, size; >>> unsigned int best, i; >>> >>> + /* Only set a mode if the current one is not valid. */ >>> + if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, &cols, &rows) == >>> + EFI_SUCCESS ) >>> + return; >> >> ... it might be okay if you put such a check in efi_multiboot2(), but >> the call here from efi_start() is specifically guarded by a check of >> whether "-basevideo" was passed to xen.efi. This _may_ not be as >> relevant anymore today, but it certainly was 20 years ago (recall >> that we've inherited this code from a much older project of ours) - >> at that time EFI usually started in 80x25 text mode. And I think that >> even today when you end up launching xen.efi from the EFI shell, >> you'd be stuck with 80x25 text mode on at least some implementations. > > Won't you use console=vga vga=gfx-... > > To switch to a best mode? I don't think "vga=gfx-..." is presently consumed in any way xen.efi. Doing so would require a similar hack of peeking at the (ordinary) command line options as in legacy booting, except that in xen.efi we read that command line from a file, which iirc is done only after fiddling with the video mode. >> Overall, looking at (for now) just the titles of subsequent patches, >> I'm not convinced the change here is needed at all. Or if anything it >> may want to go at the end, taking action only when "vga=current" was >> specified. > > I guess I'm slightly confused by the usage of both GOP and StdOut, I > would assume if we have a gop, and can correctly initialize it there's > no need to fiddle with StdOut also? Setting the GOP mode is done last before exiting boot services; this may be a graphics mode which doesn't support a text output protocol. Jan
On Thu, Mar 30, 2023 at 06:07:57PM +0200, Jan Beulich wrote: > On 30.03.2023 17:44, Roger Pau Monné wrote: > > On Mon, Dec 05, 2022 at 03:19:13PM +0100, Jan Beulich wrote: > >> On 23.11.2022 16:45, Roger Pau Monne wrote: > >>> Do not unconditionally set a mode in efi_console_set_mode(), do so > >>> only if the currently set mode is not valid. > >> > >> You don't say why you want to do so. Furthermore ... > >> > >>> --- a/xen/common/efi/boot.c > >>> +++ b/xen/common/efi/boot.c > >>> @@ -799,6 +799,11 @@ static void __init efi_console_set_mode(void) > >>> UINTN cols, rows, size; > >>> unsigned int best, i; > >>> > >>> + /* Only set a mode if the current one is not valid. */ > >>> + if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, &cols, &rows) == > >>> + EFI_SUCCESS ) > >>> + return; > >> > >> ... it might be okay if you put such a check in efi_multiboot2(), but > >> the call here from efi_start() is specifically guarded by a check of > >> whether "-basevideo" was passed to xen.efi. This _may_ not be as > >> relevant anymore today, but it certainly was 20 years ago (recall > >> that we've inherited this code from a much older project of ours) - > >> at that time EFI usually started in 80x25 text mode. And I think that > >> even today when you end up launching xen.efi from the EFI shell, > >> you'd be stuck with 80x25 text mode on at least some implementations. > > > > Won't you use console=vga vga=gfx-... > > > > To switch to a best mode? > > I don't think "vga=gfx-..." is presently consumed in any way xen.efi. > Doing so would require a similar hack of peeking at the (ordinary) > command line options as in legacy booting, except that in xen.efi we > read that command line from a file, which iirc is done only after > fiddling with the video mode. I will only take care of multiboot2, since I don't have a way to test xen.efi ATM. > >> Overall, looking at (for now) just the titles of subsequent patches, > >> I'm not convinced the change here is needed at all. Or if anything it > >> may want to go at the end, taking action only when "vga=current" was > >> specified. > > > > I guess I'm slightly confused by the usage of both GOP and StdOut, I > > would assume if we have a gop, and can correctly initialize it there's > > no need to fiddle with StdOut also? > > Setting the GOP mode is done last before exiting boot services; this > may be a graphics mode which doesn't support a text output protocol. Right, that's what I was missing. I assumed that all modes available in GOP would be compatible with the ConOut mode. Would you be OK with leaving StdOut as-is when booted from multiboot2, or there's a chance of things not being properly setup? IMO it's not very friendly to change the StdOut mode if not explicitly requested, as in the multiboot2 case that gets setup by the bootloader. Thanks, Roger.
On 30.03.2023 18:17, Roger Pau Monné wrote: > On Thu, Mar 30, 2023 at 06:07:57PM +0200, Jan Beulich wrote: >> On 30.03.2023 17:44, Roger Pau Monné wrote: >>> I guess I'm slightly confused by the usage of both GOP and StdOut, I >>> would assume if we have a gop, and can correctly initialize it there's >>> no need to fiddle with StdOut also? >> >> Setting the GOP mode is done last before exiting boot services; this >> may be a graphics mode which doesn't support a text output protocol. > > Right, that's what I was missing. I assumed that all modes available > in GOP would be compatible with the ConOut mode. > > Would you be OK with leaving StdOut as-is when booted from multiboot2, > or there's a chance of things not being properly setup? On modern UEFI it may be unlikely, but I think it's not impossible (see below). > IMO it's not very friendly to change the StdOut mode if not explicitly > requested, as in the multiboot2 case that gets setup by the > bootloader. May get set up, that is. If it was set up, then yes, we probably should leave it alone unless told to use another mode. I.e. no vga= or vga=current should minimally result in no further mode change. Aiui we can't easily honor vga=gfx-... in that case, so leaving the mode alone there may also be better than trying to guess a mode. The only time where I would think it would be nice to switch by default even in the xen.gz case is if the boot loader handed us the screen in some text mode. Jan
On Fri, Mar 31, 2023 at 08:51:46AM +0200, Jan Beulich wrote: > On 30.03.2023 18:17, Roger Pau Monné wrote: > > On Thu, Mar 30, 2023 at 06:07:57PM +0200, Jan Beulich wrote: > >> On 30.03.2023 17:44, Roger Pau Monné wrote: > >>> I guess I'm slightly confused by the usage of both GOP and StdOut, I > >>> would assume if we have a gop, and can correctly initialize it there's > >>> no need to fiddle with StdOut also? > >> > >> Setting the GOP mode is done last before exiting boot services; this > >> may be a graphics mode which doesn't support a text output protocol. > > > > Right, that's what I was missing. I assumed that all modes available > > in GOP would be compatible with the ConOut mode. > > > > Would you be OK with leaving StdOut as-is when booted from multiboot2, > > or there's a chance of things not being properly setup? > > On modern UEFI it may be unlikely, but I think it's not impossible (see > below). > > > IMO it's not very friendly to change the StdOut mode if not explicitly > > requested, as in the multiboot2 case that gets setup by the > > bootloader. > > May get set up, that is. If it was set up, then yes, we probably should > leave it alone unless told to use another mode. I.e. no vga= or > vga=current should minimally result in no further mode change. Aiui we > can't easily honor vga=gfx-... in that case, so leaving the mode alone > there may also be better than trying to guess a mode. The only time > where I would think it would be nice to switch by default even in the > xen.gz case is if the boot loader handed us the screen in some text > mode. How would you detect such case? ConOut is always text-mode like because it's a EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL interface. Would it be a matter of checking whether the current GOP mode is valid, and if so leave it as-is unless told otherwise by a command line parameter? I would also like to avoid the unconditional resizing of the ConOut interface that's done in efi_console_set_mode(), as that has the size effect of changing the GOP mode, so I would only call efi_console_set_mode() is there's no gop. Not sure it's meaningful to change the ConOut number of cols/rows if there's no GOP, maybe it's possible to have some kind of screen that's usable for EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL but not as a GOP? Thanks, Roger.
On 31.03.2023 09:37, Roger Pau Monné wrote: > On Fri, Mar 31, 2023 at 08:51:46AM +0200, Jan Beulich wrote: >> On 30.03.2023 18:17, Roger Pau Monné wrote: >>> On Thu, Mar 30, 2023 at 06:07:57PM +0200, Jan Beulich wrote: >>>> On 30.03.2023 17:44, Roger Pau Monné wrote: >>>>> I guess I'm slightly confused by the usage of both GOP and StdOut, I >>>>> would assume if we have a gop, and can correctly initialize it there's >>>>> no need to fiddle with StdOut also? >>>> >>>> Setting the GOP mode is done last before exiting boot services; this >>>> may be a graphics mode which doesn't support a text output protocol. >>> >>> Right, that's what I was missing. I assumed that all modes available >>> in GOP would be compatible with the ConOut mode. >>> >>> Would you be OK with leaving StdOut as-is when booted from multiboot2, >>> or there's a chance of things not being properly setup? >> >> On modern UEFI it may be unlikely, but I think it's not impossible (see >> below). >> >>> IMO it's not very friendly to change the StdOut mode if not explicitly >>> requested, as in the multiboot2 case that gets setup by the >>> bootloader. >> >> May get set up, that is. If it was set up, then yes, we probably should >> leave it alone unless told to use another mode. I.e. no vga= or >> vga=current should minimally result in no further mode change. Aiui we >> can't easily honor vga=gfx-... in that case, so leaving the mode alone >> there may also be better than trying to guess a mode. The only time >> where I would think it would be nice to switch by default even in the >> xen.gz case is if the boot loader handed us the screen in some text >> mode. > > How would you detect such case? > > ConOut is always text-mode like because it's a > EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL interface. > > Would it be a matter of checking whether the current GOP mode is > valid, and if so leave it as-is unless told otherwise by a command > line parameter? I think so, yes. > I would also like to avoid the unconditional resizing of the ConOut > interface that's done in efi_console_set_mode(), as that has the size > effect of changing the GOP mode, so I would only call > efi_console_set_mode() is there's no gop. Or maybe when the set mode isn't text-output capable. > Not sure it's meaningful to change the ConOut number of cols/rows if > there's no GOP, maybe it's possible to have some kind of screen that's > usable for EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL but not as a GOP? Of course there is. As said, earlier on screens started in 80x25 mode. Even going to 80x50 or 80x60 is already an improvement. Plus there are systems which support wider-than-80-cols text modes. Jan
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index db0340c8e2..7e8a8b7857 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -799,6 +799,11 @@ static void __init efi_console_set_mode(void) UINTN cols, rows, size; unsigned int best, i; + /* Only set a mode if the current one is not valid. */ + if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, &cols, &rows) == + EFI_SUCCESS ) + return; + for ( i = 0, size = 0, best = StdOut->Mode->Mode; i < StdOut->Mode->MaxMode; ++i ) {
Do not unconditionally set a mode in efi_console_set_mode(), do so only if the currently set mode is not valid. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/common/efi/boot.c | 5 +++++ 1 file changed, 5 insertions(+)