From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 24 Apr 2019 03:48:25 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6F9AE3093418; Wed, 24 Apr 2019 10:48:24 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-123.rdu2.redhat.com [10.10.120.123]) by smtp.corp.redhat.com (Postfix) with ESMTP id AD00D5DC1E; Wed, 24 Apr 2019 10:48:21 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH V2 2/2] MdeModulePkg/GraphicsConsoleDxe: Do not clean the screen From: "Laszlo Ersek" To: "Gao, Zhichao" , "devel@edk2.groups.io" Cc: Aaron Antone , "Wang, Jian J" , "Wu, Hao A" , "Ni, Ray" , "Zeng, Star" , "Gao, Liming" , Sean Brogan , Michael Turner , Bret Barkelew References: <20190423070450.1892-1-zhichao.gao@intel.com> <20190423070450.1892-3-zhichao.gao@intel.com> <1e606f64-af5b-494b-e497-eef0b2f0fe7a@redhat.com> <3CE959C139B4C44DBEA1810E3AA6F9000B7C3D2E@SHSMSX101.ccr.corp.intel.com> <68fcdf22-a6d8-85ba-8992-9aca4b5293b9@redhat.com> Message-ID: Date: Wed, 24 Apr 2019 12:48:20 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <68fcdf22-a6d8-85ba-8992-9aca4b5293b9@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Wed, 24 Apr 2019 10:48:24 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/24/19 12:24, Laszlo Ersek wrote: > ... Actually, you mention "the phase immediately after the > PlatformBootManagerAfterConsole() is the best choice". So how about the > following approach: > > (1) Introduce a new protocol (an edk2 extension). The UEFI protocol > database is supposed to contain *at most* one instance of this protocol, > at any time. > > (2) The protocol should consist of a BOOLEAN field only. (You can add a > "revision" field at the beginning too, if you like.) > > (3) In GraphicsConsoleDxe, which is a UEFI driver, you try to locate the > protocol at the right spot -- when SetMode is about to clear the screen. > If the protocol instance is missing, there is no change in behavior (the > screen is cleared). If the protocol instance is available, but the > BOOLEAN field is , there is no change in behavior (the screen > is cleared). If the protocol instance is available, and the BOOLEAN > field is , then the screen is *not* cleared. > > (4) Platforms that need the anti-flicker tweak are expected to do the > following two steps: > > (4a) add a platform DXE driver (or modify one) to install the protocol, > with the BOOLEAN field set to . > > (4b) append new logic to PlatformBootManagerAfterConsole(), for locating > the protocol, and setting the BOOLEAN field in it to . Two more thoughts: * regarding (4a), it's not necessary to modify (or introduce) a platform DXE driver for installing the protocol, with . I think it can be done in PlatformBootManagerBeforeConsole() as well. * the new protocol could be replced with a new event group: - GraphicsConsoleDxe would get a new global variable (a BOOLEAN), with the default value meaning "clear screen on SetMode". - GraphicsConsoleDxe would also install an event group notification function, for *inverting* the boolean variable (and doing nothing else). - In the affected platforms, BeforeConsole would signal the new event group once (to disable screen clearing temporarily). Then AfterConsole would signal the new event group once more, to restore the default (standards conformant) behavior. - Unaffected platforms would never signal the new event group; they'd ignore it. Personally, I like the protocol approach a lot more, because at any point, you can query the state of the "system" (= the protocol DB) with external utilities. The new event group allows for much less introspection, not to mention that the meaning of signaling the event group depends on the current state. (That could be avoided by introducing two event groups, one for setting the variable and another for clearing it, but that's getting baroque in my opinion.) I'd go with the new protocol -- you don't need GraphicsConsoleDxe to react to PlatformBootManagerLib with a callback. Thanks Laszlo > > > A variant could be to introduce a tri-state DynamicEx PCD -- , > the DEC default, means no change in behavior (= the platform doesn't > need the anti-flicker tweak); while and work like > above. The question is whether we are OK adding an EFI_PCD_PROTOCOL > dependency to GraphicsConsoleDxe, which is a UEFI driver > (EFI_PCD_PROTOCOL comes from the PI spec, not from the UEFI spec). > Inventing a new edk2 extension protocol (with a fresh protocol GUID) > would avoid that problem -- it could be implemented purely in terms of > the UEFI spec. > > Thanks > Laszlo > >>> I guess you have already investigated adding a boolean field to >>> GRAPHICS_CONSOLE_DEV, and found it unsuitable for those platforms that >>> need this anti-flicker tweak. So I'm not going to suggest such a boolean field >>> now. >>> >>> Instead, I propose a PCD (feature PCD or dynamic boolean PCD). If you add a >>> PCD, I won't care about the particulars of this patch, as long as platforms >>> continue observing the std conformant behavior, under the default value of >>> the PCD (i.e., from "MdeModulePkg.dec"). >>> >>> Thanks, >>> Laszlo >>> >>>> Cc: Jian J Wang >>>> Cc: Hao Wu >>>> Cc: Ray Ni >>>> Cc: Star Zeng >>>> Cc: Liming Gao >>>> Cc: Sean Brogan >>>> Cc: Michael Turner >>>> Cc: Bret Barkelew >>>> Cc: Laszlo Ersek >>>> Signed-off-by: Zhichao Gao >>>> --- >>>> .../GraphicsConsoleDxe/GraphicsConsole.c | 82 +++++++++++++------ >>>> .../GraphicsConsoleDxe/GraphicsConsoleDxe.inf | 3 + >>>> 2 files changed, 62 insertions(+), 23 deletions(-) >>>> >>>> diff --git >>>> >>> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole >>> .c >>>> >>> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol >>> e.c >>>> index 26ea19f300..39a999838c 100644 >>>> --- >>>> >>> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole >>> .c >>>> +++ >>> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol >>>> +++ e.c >>>> @@ -1,7 +1,7 @@ >>>> /** @file >>>> This is the main routine for initializing the Graphics Console support >>> routines. >>>> >>>> -Copyright (c) 2006 - 2018, Intel Corporation. All rights >>>> reserved.
>>>> +Copyright (c) 2006 - 2019, Intel Corporation. All rights >>>> +reserved.
>>>> SPDX-License-Identifier: BSD-2-Clause-Patent >>>> >>>> **/ >>>> @@ -96,6 +96,12 @@ EFI_DRIVER_BINDING_PROTOCOL >>> gGraphicsConsoleDriverBinding = { >>>> NULL >>>> }; >>>> >>>> +// >>>> +// Event and variable to indicate the boot phase. >>>> +// >>>> +EFI_EVENT mGraphicsConsoleReadyToBootEvent; >>>> +BOOLEAN mGraphicsConsoleReadyToBoot = FALSE; >>>> + >>>> /** >>>> Test to see if Graphics Console could be supported on the Controller. >>>> >>>> @@ -567,16 +573,7 @@ GraphicsConsoleControllerDriverStart ( >>>> // >>>> Private->SimpleTextOutputMode.MaxMode = (INT32) MaxMode; >>>> >>>> - DEBUG_CODE_BEGIN (); >>>> - Status = GraphicsConsoleConOutSetMode (&Private- >>>> SimpleTextOutput, 0); >>>> - if (EFI_ERROR (Status)) { >>>> - goto Error; >>>> - } >>>> - Status = GraphicsConsoleConOutOutputString (&Private- >>>> SimpleTextOutput, (CHAR16 *)L"Graphics Console Started\n\r"); >>>> - if (EFI_ERROR (Status)) { >>>> - goto Error; >>>> - } >>>> - DEBUG_CODE_END (); >>>> + DEBUG ((DEBUG_INFO, "Graphics Console Started!\n\r")); >>>> >>>> // >>>> // Install protocol interfaces for the Graphics Console device. >>>> @@ -1366,18 +1363,26 @@ GraphicsConsoleConOutSetMode ( >>>> // >>>> // The current graphics mode is correct, so simply clear the entire >>> display >>>> // >>>> - Status = GraphicsOutput->Blt ( >>>> - GraphicsOutput, >>>> - &mGraphicsEfiColors[0], >>>> - EfiBltVideoFill, >>>> - 0, >>>> - 0, >>>> - 0, >>>> - 0, >>>> - ModeData->GopWidth, >>>> - ModeData->GopHeight, >>>> - 0 >>>> - ); >>>> + // >>>> + // For the first time boot system, do not clear the display. >>>> + // Some platforms would show logo at PEIM and this would clear >>>> + // the whole screen. So for first boot set mode, do not clear >>>> + // the screen. >>>> + // >>>> + if (This->Mode->Mode != -1 || mGraphicsConsoleReadyToBoot) { >>>> + Status = GraphicsOutput->Blt ( >>>> + GraphicsOutput, >>>> + &mGraphicsEfiColors[0], >>>> + EfiBltVideoFill, >>>> + 0, >>>> + 0, >>>> + 0, >>>> + 0, >>>> + ModeData->GopWidth, >>>> + ModeData->GopHeight, >>>> + 0 >>>> + ); >>>> + } >>>> } >>>> } else if (FeaturePcdGet (PcdUgaConsumeSupport)) { >>>> // >>>> @@ -2065,6 +2070,24 @@ RegisterFontPackage ( >>>> FreePool (Package); >>>> } >>>> >>>> +/** >>>> + Indicate the Boot phase is at ReadyToBoot. >>>> + >>>> + @param[in] Event The Event that is being processed. >>>> + @param[in] Context The Event Context. >>>> + >>>> +**/ >>>> +VOID >>>> +GraphicsConsoleReadyToBootEvent ( >>>> + IN EFI_EVENT Event, >>>> + IN VOID *Context >>>> + ) >>>> +{ >>>> + mGraphicsConsoleReadyToBoot = TRUE; >>>> + >>>> + gBS->CloseEvent (mGraphicsConsoleReadyToBootEvent); >>>> +} >>>> + >>>> /** >>>> The user Entry Point for module GraphicsConsole. The user code starts >>> with this function. >>>> >>>> @@ -2095,6 +2118,19 @@ InitializeGraphicsConsole ( >>>> &mHiiRegistration >>>> ); >>>> >>>> + // >>>> + // Create a event function of ReadyToBoot to avoid clearing screen >>>> + before boot // Status = gBS->CreateEventEx ( >>>> + EVT_NOTIFY_SIGNAL, >>>> + TPL_NOTIFY, >>>> + GraphicsConsoleReadyToBootEvent, >>>> + NULL, >>>> + &gEfiEventReadyToBootGuid, >>>> + &mGraphicsConsoleReadyToBootEvent >>>> + ); >>>> + ASSERT_EFI_ERROR (Status); >>>> + >>>> // >>>> // Install driver model protocol(s). >>>> // >>>> diff --git >>>> >>> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole >>> Dxe >>>> .inf >>>> >>> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol >>> eDxe >>>> .inf >>>> index f7caa65aa9..59751893f6 100644 >>>> --- >>>> >>> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole >>> Dxe >>>> .inf >>>> +++ >>> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol >>>> +++ eDxe.inf >>>> @@ -49,6 +49,9 @@ >>>> HiiLib >>>> PcdLib >>>> >>>> +[Guids] >>>> + gEfiEventReadyToBootGuid ## CONSUMES >>>> + >>>> [Protocols] >>>> gEfiDevicePathProtocolGuid ## TO_START >>>> gEfiSimpleTextOutProtocolGuid ## BY_START >>>> >>> >>> >>> >> >