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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.11.2022 16:45, Roger Pau Monne wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -864,6 +864,26 @@ 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.
> +             */
> +            cols = mode_info->HorizontalResolution;
> +            rows = mode_info->VerticalResolution;

For these I think you want to replace cols and rows individually, i.e.

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

whereas ...

> +            depth = 0;

... this case looks more complicated, as 0 is also already legitimate
as a value to come in. By zapping a non-zero incoming value you may end
up switching modes _just_ to alter depth, and then you may not fulfill
what was requested. For now I think depth simply wants leaving alone
here (and perhaps using the current mode's value if the incoming value
was zero, thus eliminating the need for a mode change in certain cases).

In a separate change we might then enhance this, e.g. by finding a
supported mode matching geometry but only coming close for "depth".

Jan