Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2 00/14] efi/gop: Refactoring + mode-setting feature
To:     Arvind Sankar <nivedita@alum.mit.edu>,
        Ard Biesheuvel <ardb@kernel.org>
Cc:     linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20200319192855.29876-1-nivedita@alum.mit.edu>
 <20200320020028.1936003-1-nivedita@alum.mit.edu>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <dcb1bfb1-9663-f149-a29c-87e5a6c6c2f0@redhat.com>
Date:   Wed, 25 Mar 2020 17:50:19 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <20200320020028.1936003-1-nivedita@alum.mit.edu>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On 3/20/20 3:00 AM, Arvind Sankar wrote:
> This series is against tip:efi/core.
> 
> Patches 1-9 are small cleanups and refactoring of the code in
> libstub/gop.c.
> 
> The rest of the patches add the ability to use a command-line option to
> switch the gop's display mode.
> 
> The options supported are:
> video=efifb:mode=n
>          Choose a specific mode number
> video=efifb:<xres>x<yres>[-(rgb|bgr|<bpp>)]
>          Specify mode by resolution and optionally color depth
> video=efifb:auto
>          Let the EFI stub choose the highest resolution mode available.
> 
> The mode-setting additions increase code size of gop.o by about 3k on
> x86-64 with EFI_MIXED enabled.

Thank you for adding me to the Cc. I will add these to my personal
tree, which I test semi-regular on various hardware.

I've only looked at patches 10 - 14 and a quick glance these look
good to me.

I was worried that you would maybe always enumerate the modes or
some such, but I see that you have structured things in such a way
that if the new kernel cmdline options are not used no extra EFI
calls are made, which make me very happy!

This way we do not need to worry about this patch-set tripping up
buggy firmware (which is quite likely to be out there somewhere)
by making new, previously unused, EFI calls.

Regards,

Hans


> 
> Changes in v2 (HT lkp@intel.com):
> - Fix __efistub_global attribute to be after the variable.
>    (NB: bunch of other places should ideally be fixed, those I guess
>    don't matter as they are scalars?)
> - Silence -Wmaybe-uninitialized warning in set_mode function.
> 
> Arvind Sankar (14):
>    efi/gop: Remove redundant current_fb_base
>    efi/gop: Move check for framebuffer before con_out
>    efi/gop: Get mode information outside the loop
>    efi/gop: Factor out locating the gop into a function
>    efi/gop: Slightly re-arrange logic of find_gop
>    efi/gop: Move variable declarations into loop block
>    efi/gop: Use helper macros for populating lfb_base
>    efi/gop: Use helper macros for find_bits
>    efi/gop: Remove unreachable code from setup_pixel_info
>    efi/gop: Add prototypes for query_mode and set_mode
>    efi/gop: Allow specifying mode number on command line
>    efi/gop: Allow specifying mode by <xres>x<yres>
>    efi/gop: Allow specifying depth as well as resolution
>    efi/gop: Allow automatically choosing the best mode
> 
>   Documentation/fb/efifb.rst                    |  33 +-
>   arch/x86/include/asm/efi.h                    |   4 +
>   .../firmware/efi/libstub/efi-stub-helper.c    |   3 +
>   drivers/firmware/efi/libstub/efistub.h        |   8 +-
>   drivers/firmware/efi/libstub/gop.c            | 489 ++++++++++++++----
>   5 files changed, 428 insertions(+), 109 deletions(-)
> 
> 
> base-commit: d5528d5e91041e68e8eab9792ce627705a0ed273
>