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; Tue, 23 Apr 2019 06:50:43 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AFC3A81F35; Tue, 23 Apr 2019 13:50:42 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-235.rdu2.redhat.com [10.10.120.235]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2B2C45D6A9; Tue, 23 Apr 2019 13:50:37 +0000 (UTC) Subject: Re: [PATCH V2 2/2] MdeModulePkg/GraphicsConsoleDxe: Do not clean the screen To: Zhichao Gao , devel@edk2.groups.io Cc: Aaron Antone , Jian J Wang , Hao Wu , Ray Ni , Star Zeng , Liming Gao , Sean Brogan , Michael Turner , Bret Barkelew References: <20190423070450.1892-1-zhichao.gao@intel.com> <20190423070450.1892-3-zhichao.gao@intel.com> From: "Laszlo Ersek" Message-ID: <1e606f64-af5b-494b-e497-eef0b2f0fe7a@redhat.com> Date: Tue, 23 Apr 2019 15:50:36 +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: <20190423070450.1892-3-zhichao.gao@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Tue, 23 Apr 2019 13:50:42 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/23/19 09:04, Zhichao Gao wrote: > From: Aaron Antone > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1412 > > For now, most platform support to display during PEIM. It means the logo > can show at PEI phase. But the screen would be cleared at BDS connect > console phase. That may make the screen flush and turn into black screen. > So do not clear the screen while set the text mode for graphics console > device for the first time boot. > As the shell reconnect command would make the same reslut with the first > boot, use the gEfiEventReadyToBootGuid to distinguish them. > > Also replace the debug code in GraphicsConsoleControllerDriverStart. The > origin one would set a basic mode and then print the text info to graphic > console device. Then the conspliter would set a best mode for graphics > console device. If the best mode is different with the basic one, the > screen would be cleared. So use the debug output instead. > > This patch only affect the behavior of SetMode at the first boot during > the graphics console devices first connect operations. That means at > DXE phase before ReadyToBoot, the Graphics Simple Text Out SetMode would not > clear the screen during the first connecttion of the graphics devices. The UEFI spec requirement doesn't apply after ReadyToBoot only. What about SysPrep applications, for example: """ When launched, the platform is required to provide the application loaded by SysPrep####, with the same services such as console and network as are normally provided at launch to applications referenced by a Boot#### variable. [...] After all SysPrep#### variables have been launched and exited, the platform shall notify EFI_EVENT_GROUP_READY_TO_BOOT event group and begin to evaluate Boot#### variables [...] """ Thus a SysPrep application is permitted to expect, and to use, the console, but it is launched before ReadyToBoot; and so this patch could break the console's std conformance for SysPrep apps. 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/GraphicsConsole.c > index 26ea19f300..39a999838c 100644 > --- a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c > +++ b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.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/GraphicsConsoleDxe.inf b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf > index f7caa65aa9..59751893f6 100644 > --- a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf > +++ b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf > @@ -49,6 +49,9 @@ > HiiLib > PcdLib > > +[Guids] > + gEfiEventReadyToBootGuid ## CONSUMES > + > [Protocols] > gEfiDevicePathProtocolGuid ## TO_START > gEfiSimpleTextOutProtocolGuid ## BY_START >