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

2023-05-31 Thread Jan Beulich 
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,
>>> +   , ) == 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,
>>, ) != 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

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

2023-05-31 Thread Roger Pau Monné 
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,
> > +   , ) == 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,
>, ) != 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.

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

2023-04-05 Thread Jan Beulich 
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,
> +   , ) == 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,
   , ) != 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,
   , ) == EFI_SUCCESS )
efi_arch_console_init(cols, rows);

?

Jan