Re: [PATCH v2 1/3] efi: try to use the currently set GOP mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.03.2023 11:59, Roger Pau Monne wrote:
> Modify efi_find_gop_mode() so that passing cols or rows as 0 is
> interpreted as a request to attempt to keep the currently set mode,
> and do so if the mode query for information is successful and the depth
> is supported.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Changes since v1:
>  - Only update cols or rows if the value is 0.
>  - Leave depth alone.
> ---
>  xen/common/efi/boot.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)

While I realize that docs/misc/efi.pandoc currently doesn't describe
what rows/cols being zero means, I think that file needs updating in
the course of what you're doing. Irrespective of this I'm uncertain
about the change in behavior: Presently both values being 0 means
"find biggest resolution mode", in an attempt to have as much info on
the screen at a time as possible (in particular in case of problems).
I certainly appreciate the desire to have a way to say "keep the
current mode", but I don't think this should alter behavior of what's
presently considered valid config settings.

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -930,6 +930,27 @@ static UINTN __init 
> efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>      UINTN gop_mode = ~0, info_size, size;
>      unsigned int i;
>  
> +    if ( (!cols || !rows) && gop->Mode->Mode < gop->Mode->MaxMode )
> +    {
> +        /* If no (valid) resolution suggested, try to use the current mode. 
> */
> +        status = gop->QueryMode(gop, gop->Mode->Mode, &info_size, 
> &mode_info);
> +        if ( EFI_ERROR(status) )
> +            PrintErr(L"Invalid current graphics mode\r\n");
> +        else if ( mode_info->PixelFormat < PixelBltOnly )
> +            return gop->Mode->Mode;

What if one of cols/rows was non-zero? You then wouldn't fulfill the request.
"depth", if non-zero, is also entirely ignored. (We don't fulfill such a
request right now either, but not doing so becomes more odd now imo. In fact
right now in this case we leave the screen alone, if I'm not mistaken, so
there already looks to be a way to achieve what you're after.)

> +        else
> +        {
> +            /*
> +             * Try to find a mode with the same resolution and a valid pixel
> +             * format.
> +             */

Is "valid" the right word here? I think you mean "usable for us" or some
such?

> +            if ( !cols )
> +                cols = mode_info->HorizontalResolution;
> +            if ( !rows )
> +                rows = mode_info->VerticalResolution;
> +        }
> +    }

Overall with the resulting behavior I'm not sure the title really describes
what's being done. You "try" only in certain cases.

Jan