Re: [PATCH v2 3/3] multiboot2: do not set StdOut mode unconditionally

["Roger Pau Monné" <roger.pau@citrix.com>]

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.