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

Commit Message

Roger Pau Monné March 31, 2023, 9:59 a.m. UTC

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é <[e-mail]>
---
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(+)

Comments

Jan Beulich April 4, 2023, 4:07 p.m. UTC | #1

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é <[e-mail]>
> ---
> 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

Patch

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index b69c83e354..766a2553be 100644
--- 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;
+        else
+        {
+            /*
+             * Try to find a mode with the same resolution and a valid pixel
+             * format.
+             */
+            if ( !cols )
+                cols = mode_info->HorizontalResolution;
+            if ( !rows )
+                rows = mode_info->VerticalResolution;
+        }
+    }
+
     for ( i = size = 0; i < gop->Mode->MaxMode; ++i )
     {
         unsigned int bpp = 0;