* [PATCH V3 0/2] MdeModulePkg: Make the screen seamless @ 2019-05-05 2:17 Gao, Zhichao 2019-05-05 2:17 ` [PATCH V3 1/2] MdeModulePkg/ConSplitterDxe: Optimize the ConSplitterTextOutSetMode Gao, Zhichao 2019-05-05 2:17 ` [PATCH V3 2/2] MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode Gao, Zhichao 0 siblings, 2 replies; 5+ messages in thread From: Gao, Zhichao @ 2019-05-05 2:17 UTC (permalink / raw) To: devel Cc: Jian J Wang, Hao Wu, Ray Ni, Star Zeng, Liming Gao, Sean Brogan, Michael Turner, Bret Barkelew, Laszlo Ersek For now most platforms support display function at PEI phase. But the conspliter and graphics console driver would clear the screen at BDS connect console phase. Maybe some platforms would show logo in the next or maybe not. For consumers, it looks like the screen flashed. So change the behavior of graphics console devices while connect console devices to maintain seamless screen from PEI. Test has done on MinPlatform Kabylake-RVP3 which support PEI display. V2: Make the SetMode not clear the screen only at the first boot during the first conncettion of graphics device. V3: Abandon V2. Directly set the output mode without clear the screen while initialize the graphics console device. Add clear screen operation in stop function of graphics console device. Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Hao Wu <hao.a.wu@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Star Zeng <star.zeng@intel.com> Cc: Liming Gao <liming.gao@intel.com> Cc: Sean Brogan <sean.brogan@microsoft.com> Cc: Michael Turner <Michael.Turner@microsoft.com> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> Cc: Laszlo Ersek <lersek@redhat.com> Aaron Antone (1): MdeModulePkg/ConSplitterDxe: Optimize the ConSplitterTextOutSetMode Zhichao Gao (1): MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode .../Console/ConSplitterDxe/ConSplitter.c | 33 +++++++++----- .../Console/ConSplitterDxe/ConSplitter.h | 4 +- .../GraphicsConsoleDxe/GraphicsConsole.c | 44 ++++++++++++++----- .../GraphicsConsoleDxe/GraphicsConsoleDxe.inf | 2 + 4 files changed, 61 insertions(+), 22 deletions(-) -- 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH V3 1/2] MdeModulePkg/ConSplitterDxe: Optimize the ConSplitterTextOutSetMode 2019-05-05 2:17 [PATCH V3 0/2] MdeModulePkg: Make the screen seamless Gao, Zhichao @ 2019-05-05 2:17 ` Gao, Zhichao 2019-05-05 2:17 ` [PATCH V3 2/2] MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode Gao, Zhichao 1 sibling, 0 replies; 5+ messages in thread From: Gao, Zhichao @ 2019-05-05 2:17 UTC (permalink / raw) To: devel Cc: Aaron Antone, Jian J Wang, Hao Wu, Ray Ni, Star Zeng, Liming Gao, Sean Brogan, Michael Turner, Bret Barkelew From: Aaron Antone <aanton@microsoft.com> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1412 For Console Out device, it would always set all present devices' text out mode again through ConSplitterTextOutSetMode while adding devices. That may cause the screen cleared for serval times. So add a BOOLEAN to judge if it is adding device then we will not set the same text mode again for same console out device. Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Hao Wu <hao.a.wu@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Star Zeng <star.zeng@intel.com> Cc: Liming Gao <liming.gao@intel.com> Cc: Sean Brogan <sean.brogan@microsoft.com> Cc: Michael Turner <Michael.Turner@microsoft.com> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> --- .../Console/ConSplitterDxe/ConSplitter.c | 33 ++++++++++++------- .../Console/ConSplitterDxe/ConSplitter.h | 4 ++- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c index 6fc0e4796f..5de022b02a 100644 --- a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c +++ b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c @@ -16,7 +16,7 @@ never removed. Such design ensures sytem function well during none console device situation. -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR> (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR> SPDX-License-Identifier: BSD-2-Clause-Patent @@ -180,7 +180,8 @@ GLOBAL_REMOVE_IF_UNREFERENCED TEXT_OUT_SPLITTER_PRIVATE_DATA mConOut = { 0, (TEXT_OUT_SPLITTER_QUERY_DATA *) NULL, 0, - (INT32 *) NULL + (INT32 *) NULL, + FALSE }; // @@ -235,7 +236,8 @@ GLOBAL_REMOVE_IF_UNREFERENCED TEXT_OUT_SPLITTER_PRIVATE_DATA mStdErr = { 0, (TEXT_OUT_SPLITTER_QUERY_DATA *) NULL, 0, - (INT32 *) NULL + (INT32 *) NULL, + FALSE }; // @@ -3132,8 +3134,9 @@ ConSplitterTextOutAddDevice ( EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info; EFI_STATUS DeviceStatus; - Status = EFI_SUCCESS; - CurrentNumOfConsoles = Private->CurrentNumberOfConsoles; + Status = EFI_SUCCESS; + CurrentNumOfConsoles = Private->CurrentNumberOfConsoles; + Private->AddingConOutDevice = TRUE; // // If the Text Out List is full, enlarge it by calling ConSplitterGrowBuffer(). @@ -3290,6 +3293,8 @@ ConSplitterTextOutAddDevice ( // ConsplitterSetConsoleOutMode (Private); + Private->AddingConOutDevice = FALSE; + return Status; } @@ -4849,12 +4854,18 @@ ConSplitterTextOutSetMode ( // TextOutModeMap = Private->TextOutModeMap + Private->TextOutListCount * ModeNumber; for (Index = 0, ReturnStatus = EFI_SUCCESS; Index < Private->CurrentNumberOfConsoles; Index++) { - Status = Private->TextOutList[Index].TextOut->SetMode ( - Private->TextOutList[Index].TextOut, - TextOutModeMap[Index] - ); - if (EFI_ERROR (Status)) { - ReturnStatus = Status; + // + // While adding a console out device do not set same mode again for the same device. + // + if ((Private->AddingConOutDevice != TRUE) || + (TextOutModeMap[Index] != Private->TextOutList[Index].TextOut->Mode->Mode)) { + Status = Private->TextOutList[Index].TextOut->SetMode ( + Private->TextOutList[Index].TextOut, + TextOutModeMap[Index] + ); + if (EFI_ERROR (Status)) { + ReturnStatus = Status; + } } } diff --git a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.h b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.h index e9b68e58c6..419635c3f5 100644 --- a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.h +++ b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.h @@ -1,7 +1,7 @@ /** @file Private data structures for the Console Splitter driver -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR> SPDX-License-Identifier: BSD-2-Clause-Patent **/ @@ -218,6 +218,8 @@ typedef struct { UINTN TextOutQueryDataCount; INT32 *TextOutModeMap; + BOOLEAN AddingConOutDevice; + } TEXT_OUT_SPLITTER_PRIVATE_DATA; #define TEXT_OUT_SPLITTER_PRIVATE_DATA_FROM_THIS(a) \ -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH V3 2/2] MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode 2019-05-05 2:17 [PATCH V3 0/2] MdeModulePkg: Make the screen seamless Gao, Zhichao 2019-05-05 2:17 ` [PATCH V3 1/2] MdeModulePkg/ConSplitterDxe: Optimize the ConSplitterTextOutSetMode Gao, Zhichao @ 2019-05-05 2:17 ` Gao, Zhichao 2019-05-06 17:42 ` [edk2-devel] " Laszlo Ersek 1 sibling, 1 reply; 5+ messages in thread From: Gao, Zhichao @ 2019-05-05 2:17 UTC (permalink / raw) To: devel Cc: Jian J Wang, Hao Wu, Ray Ni, Star Zeng, Liming Gao, Sean Brogan, Michael Turner, Bret Barkelew, Laszlo Ersek REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1412 Original logic: Connect the graphics device -> connect it as graphics consoles and initialize its parameters(Mode = -1, invalid) -> connect it as console spliter and add the device to the list(use SetMode to set mode to the user defined mode or the best mode the devices supported if the mode is invalid. *clear the screen at this phase*) Changed logic: Connect the graphics device -> connect it as graphics consoles and initialize its parameters(initialize the mode to the user defined mode or the best mode. *directly set the mode value without using SetMode, that would not clear the screen) -> connect it as console spliter and add the device to the list(use SetMode to set mode to the user defined mode or the best mode the devices supported if the mode is invalid. *now the mode is already set, so it would not clear the screen*). Also remove the section of SetMode for debug version. But there would be no clear screen operation while reconnect the graphics device. Instead, clear the screen when stop the graphics console device. Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Hao Wu <hao.a.wu@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Star Zeng <star.zeng@intel.com> Cc: Liming Gao <liming.gao@intel.com> Cc: Sean Brogan <sean.brogan@microsoft.com> Cc: Michael Turner <Michael.Turner@microsoft.com> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> Cc: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> --- .../GraphicsConsoleDxe/GraphicsConsole.c | 44 ++++++++++++++----- .../GraphicsConsoleDxe/GraphicsConsoleDxe.inf | 2 + 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c index 26ea19f300..42386de3f5 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.<BR> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR> SPDX-License-Identifier: BSD-2-Clause-Patent **/ @@ -384,6 +384,12 @@ GraphicsConsoleControllerDriverStart ( EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode; UINTN SizeOfInfo; EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info; + UINTN PreferMode; + UINTN Index; + UINTN Column; + UINTN Row; + UINTN DefaultColumn; + UINTN DefaultRow; ModeNumber = 0; @@ -567,16 +573,32 @@ 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; + // + // Initialize the Mode of graphics console devices + // + PreferMode = 0; + DefaultColumn = PcdGet32 (PcdConOutColumn); + DefaultRow = PcdGet32 (PcdConOutRow); + Column = 0; + Row = 0; + for (Index = 0; Index < MaxMode; Index++) { + if (DefaultColumn != 0 && DefaultRow != 0) { + if ((Private->ModeData[Index].Columns == Column) && + (Private->ModeData[Index].Rows == Row)) { + PreferMode = Index; + break; + } + } else { + if ((Private->ModeData[Index].Columns > Column) && + (Private->ModeData[Index].Rows > Row)) { + Column = Private->ModeData[Index].Columns; + Row = Private->ModeData[Index].Rows; + PreferMode = Index; + } } - DEBUG_CODE_END (); + } + Private->SimpleTextOutput.Mode->Mode = (INT32)PreferMode; + DEBUG ((DEBUG_INFO, "Graphics Console Started, Mode: %x\n\r", PreferMode)); // // Install protocol interfaces for the Graphics Console device. @@ -669,6 +691,8 @@ GraphicsConsoleControllerDriverStop ( return EFI_NOT_STARTED; } + SimpleTextOutput->ClearScreen (SimpleTextOutput); + Private = GRAPHICS_CONSOLE_CON_OUT_DEV_FROM_THIS (SimpleTextOutput); Status = gBS->UninstallProtocolInterface ( diff --git a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf index f7caa65aa9..bcfd306eee 100644 --- a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf +++ b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf @@ -65,6 +65,8 @@ [Pcd] gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution ## SOMETIMES_CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution ## SOMETIMES_CONSUMES + gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow ## SOMETIMES_CONSUMES + gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn ## SOMETIMES_CONSUMES [UserExtensions.TianoCore."ExtraFiles"] GraphicsConsoleDxeExtra.uni -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH V3 2/2] MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode 2019-05-05 2:17 ` [PATCH V3 2/2] MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode Gao, Zhichao @ 2019-05-06 17:42 ` Laszlo Ersek 2019-05-07 7:11 ` Gao, Zhichao 0 siblings, 1 reply; 5+ messages in thread From: Laszlo Ersek @ 2019-05-06 17:42 UTC (permalink / raw) To: devel, zhichao.gao Cc: Jian J Wang, Hao Wu, Ray Ni, Star Zeng, Liming Gao, Sean Brogan, Michael Turner, Bret Barkelew On 05/05/19 04:17, Gao, Zhichao wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1412 > > Original logic: > Connect the graphics device -> connect it as graphics consoles > and initialize its parameters(Mode = -1, invalid) -> connect it > as console spliter and add the device to the list(use SetMode to > set mode to the user defined mode or the best mode the devices > supported if the mode is invalid. *clear the screen at this phase*) > > Changed logic: > Connect the graphics device -> connect it as graphics consoles > and initialize its parameters(initialize the mode to the user > defined mode or the best mode. *directly set the mode value without > using SetMode, that would not clear the screen) -> connect it as > console spliter and add the device to the list(use SetMode to set > mode to the user defined mode or the best mode the devices supported > if the mode is invalid. *now the mode is already set, so it would > not clear the screen*). > > Also remove the section of SetMode for debug version. > > But there would be no clear screen operation while reconnect the > graphics device. Instead, clear the screen when stop the graphics > console device. > > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Hao Wu <hao.a.wu@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Sean Brogan <sean.brogan@microsoft.com> > Cc: Michael Turner <Michael.Turner@microsoft.com> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > --- > .../GraphicsConsoleDxe/GraphicsConsole.c | 44 ++++++++++++++----- > .../GraphicsConsoleDxe/GraphicsConsoleDxe.inf | 2 + > 2 files changed, 36 insertions(+), 10 deletions(-) This driver is too complex for me to decide whether this patch is correct. The spec writes: > If the output device is not in a valid text mode at the time of the > EFI_BOOT_SERVICES.HandleProtocol() call, the device is to indicate > that its CurrentMode is -1. (1) Because the patch sets a value different from -1 to CurrentMode, can we guarantee that the console will be usable immediately? More comments below: On 05/05/19 04:17, Gao, Zhichao wrote: > diff --git a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c > index 26ea19f300..42386de3f5 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.<BR> > +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -384,6 +384,12 @@ GraphicsConsoleControllerDriverStart ( > EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode; > UINTN SizeOfInfo; > EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info; > + UINTN PreferMode; > + UINTN Index; > + UINTN Column; > + UINTN Row; > + UINTN DefaultColumn; > + UINTN DefaultRow; > > ModeNumber = 0; > > @@ -567,16 +573,32 @@ 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; > + // > + // Initialize the Mode of graphics console devices > + // > + PreferMode = 0; > + DefaultColumn = PcdGet32 (PcdConOutColumn); > + DefaultRow = PcdGet32 (PcdConOutRow); > + Column = 0; > + Row = 0; > + for (Index = 0; Index < MaxMode; Index++) { > + if (DefaultColumn != 0 && DefaultRow != 0) { > + if ((Private->ModeData[Index].Columns == Column) && > + (Private->ModeData[Index].Rows == Row)) { > + PreferMode = Index; > + break; > + } > + } else { > + if ((Private->ModeData[Index].Columns > Column) && > + (Private->ModeData[Index].Rows > Row)) { > + Column = Private->ModeData[Index].Columns; > + Row = Private->ModeData[Index].Rows; > + PreferMode = Index; > + } > } > - DEBUG_CODE_END (); > + } > + Private->SimpleTextOutput.Mode->Mode = (INT32)PreferMode; (2) The loop above can terminate without assigning Index to PreferMode. In that case, the assignment right after the loop will set the Mode field to 0, when overwriting the initial value (-1). Is this intentional? Because, if we can find no matching mode, it seems like we should stick with (-1) -- for "current text mode is not valid". In that case, the following passage from the spec would apply: > On connecting to the output device the caller is required to verify > the mode of the output device, and if it is not acceptable to set it > to something it can use. Should we perhaps change the type of PreferMode to INT32, and set it initially to (-1)? More comments: On 05/05/19 04:17, Gao, Zhichao wrote: > + DEBUG ((DEBUG_INFO, "Graphics Console Started, Mode: %x\n\r", PreferMode)); (3) DEBUG messages should not contain "\r" characters. My understanding is that PrintLib adds "\r" automatically. (4) UINTN values (which may be UINT64, dependent on build architecture) should not be printed with "%x". UINTN should be converted to UINT64 explicitly, for printing, and then printed with "%Lx" (or "%Lu"). (If you decide to switch PreferMode to INT32, then %d is the format specifier to use, of course.) (5) I applied the series, for testing, on top of commit fbb0ec7ea4c0 (current master). I tested the patches briefly with OVMF. With the patches applied, the firmware crashes when I run "reconnect -r", as the first command, in the built-in UEFI shell: > !!!! X64 Exception Type - 0D(#GP - General Protection) CPU Apic ID - 00000000 !!!! > ExceptionData - 0000000000000000 > RIP - 000000007EECB8F9, CS - 0000000000000038, RFLAGS - 0000000000010012 > RAX - 0000D0EC8148E589, RCX - 000000007EF06FE0, RDX - 000000000904A7A0 > RBX - 0000000000000013, RSP - 000000007EEC9110, RBP - 000000007EEC9140 > RSI - 000000007EEC92D8, RDI - 000000007EF07010 > R8 - 0000000000000007, R9 - 000000012E9C46D6, R10 - 0000000400020100 > R11 - 0000000000000008, R12 - 0000000000000000, R13 - 0000000000000000 > R14 - 0000000000000000, R15 - 0000000000000000 > DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030 > GS - 0000000000000030, SS - 0000000000000030 > CR0 - 0000000080010033, CR2 - 0000000000000000, CR3 - 000000007EC01000 > CR4 - 0000000000000668, CR8 - 0000000000000000 > DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 > DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 > GDTR - 000000007EBEDA98 0000000000000047, LDTR - 0000000000000000 > IDTR - 000000007E241018 0000000000000FFF, TR - 0000000000000000 > FXSAVE_STATE - 000000007EEC8D70 > !!!! Find image based on IP(0x7EECB8F9) > Build/Ovmf3264/NOOPT_GCC48/X64/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.dll > (ImageBase=000000007EECB000, EntryPoint=000000007EED00C3) !!!! The crash offset is 0x000000007EECB8F9-0x000000007EECB000 = 0x8F9. The crash occurs in the InternalBaseLibIsListValid() function: > // > // Count the total number of nodes in List. > // Exit early if the number of nodes in List >= PcdMaximumLinkedListLength > // > do { > Ptr = Ptr->ForwardLink; > 8f5: 48 8b 45 f0 mov -0x10(%rbp),%rax > 8f9: 48 8b 00 mov (%rax),%rax <------- here > 8fc: 48 89 45 f0 mov %rax,-0x10(%rbp) (RAX = 0xD0EC_8148_E589, which is approximately 208 TB, so obviously something corrupted a list). The crash does not occur ("reconnect -r" works fine) if I build OVMF at fbb0ec7ea4c0 (current master). Thanks, Laszlo > > // > // Install protocol interfaces for the Graphics Console device. > @@ -669,6 +691,8 @@ GraphicsConsoleControllerDriverStop ( > return EFI_NOT_STARTED; > } > > + SimpleTextOutput->ClearScreen (SimpleTextOutput); > + > Private = GRAPHICS_CONSOLE_CON_OUT_DEV_FROM_THIS (SimpleTextOutput); > > Status = gBS->UninstallProtocolInterface ( > diff --git a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf > index f7caa65aa9..bcfd306eee 100644 > --- a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf > +++ b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf > @@ -65,6 +65,8 @@ > [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution ## SOMETIMES_CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution ## SOMETIMES_CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow ## SOMETIMES_CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn ## SOMETIMES_CONSUMES > > [UserExtensions.TianoCore."ExtraFiles"] > GraphicsConsoleDxeExtra.uni > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH V3 2/2] MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode 2019-05-06 17:42 ` [edk2-devel] " Laszlo Ersek @ 2019-05-07 7:11 ` Gao, Zhichao 0 siblings, 0 replies; 5+ messages in thread From: Gao, Zhichao @ 2019-05-07 7:11 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, Zeng, Star, Gao, Liming, Sean Brogan, Michael Turner, Bret Barkelew > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Tuesday, May 7, 2019 1:42 AM > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; > Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > Michael Turner <Michael.Turner@microsoft.com>; Bret Barkelew > <Bret.Barkelew@microsoft.com> > Subject: Re: [edk2-devel] [PATCH V3 2/2] > MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode > > On 05/05/19 04:17, Gao, Zhichao wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1412 > > > > Original logic: > > Connect the graphics device -> connect it as graphics consoles and > > initialize its parameters(Mode = -1, invalid) -> connect it as console > > spliter and add the device to the list(use SetMode to set mode to the > > user defined mode or the best mode the devices supported if the mode > > is invalid. *clear the screen at this phase*) > > > > Changed logic: > > Connect the graphics device -> connect it as graphics consoles and > > initialize its parameters(initialize the mode to the user defined mode > > or the best mode. *directly set the mode value without using SetMode, > > that would not clear the screen) -> connect it as console spliter and > > add the device to the list(use SetMode to set mode to the user defined > > mode or the best mode the devices supported if the mode is invalid. > > *now the mode is already set, so it would not clear the screen*). > > > > Also remove the section of SetMode for debug version. > > > > But there would be no clear screen operation while reconnect the > > graphics device. Instead, clear the screen when stop the graphics > > console device. > > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > Cc: Hao Wu <hao.a.wu@intel.com> > > Cc: Ray Ni <ray.ni@intel.com> > > Cc: Star Zeng <star.zeng@intel.com> > > Cc: Liming Gao <liming.gao@intel.com> > > Cc: Sean Brogan <sean.brogan@microsoft.com> > > Cc: Michael Turner <Michael.Turner@microsoft.com> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > --- > > .../GraphicsConsoleDxe/GraphicsConsole.c | 44 ++++++++++++++----- > > .../GraphicsConsoleDxe/GraphicsConsoleDxe.inf | 2 + > > 2 files changed, 36 insertions(+), 10 deletions(-) > > This driver is too complex for me to decide whether this patch is correct. > > The spec writes: > > > If the output device is not in a valid text mode at the time of the > > EFI_BOOT_SERVICES.HandleProtocol() call, the device is to indicate > > that its CurrentMode is -1. > > (1) Because the patch sets a value different from -1 to CurrentMode, can we > guarantee that the console will be usable immediately? I think we can guarantee that the CurrentMode value is valid except the value is 0 or -1. Because the initialize mode section is same with the one in ConSplitterDxe. > > > More comments below: > > On 05/05/19 04:17, Gao, Zhichao wrote: > > diff --git > > > a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole > .c > > > b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol > e.c > > index 26ea19f300..42386de3f5 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.<BR> > > +Copyright (c) 2006 - 2019, Intel Corporation. All rights > > +reserved.<BR> > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > **/ > > @@ -384,6 +384,12 @@ GraphicsConsoleControllerDriverStart ( > > EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode; > > UINTN SizeOfInfo; > > EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info; > > + UINTN PreferMode; > > + UINTN Index; > > + UINTN Column; > > + UINTN Row; > > + UINTN DefaultColumn; > > + UINTN DefaultRow; > > > > ModeNumber = 0; > > > > @@ -567,16 +573,32 @@ 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; > > + // > > + // Initialize the Mode of graphics console devices // PreferMode > > + = 0; DefaultColumn = PcdGet32 (PcdConOutColumn); DefaultRow = > > + PcdGet32 (PcdConOutRow); Column = 0; Row = 0; for (Index = 0; > > + Index < MaxMode; Index++) { > > + if (DefaultColumn != 0 && DefaultRow != 0) { > > + if ((Private->ModeData[Index].Columns == Column) && > > + (Private->ModeData[Index].Rows == Row)) { > > + PreferMode = Index; > > + break; > > + } > > + } else { > > + if ((Private->ModeData[Index].Columns > Column) && > > + (Private->ModeData[Index].Rows > Row)) { > > + Column = Private->ModeData[Index].Columns; > > + Row = Private->ModeData[Index].Rows; > > + PreferMode = Index; > > + } > > } > > - DEBUG_CODE_END (); > > + } > > + Private->SimpleTextOutput.Mode->Mode = (INT32)PreferMode; > > (2) The loop above can terminate without assigning Index to PreferMode. > > In that case, the assignment right after the loop will set the Mode field to 0, > when overwriting the initial value (-1). > > Is this intentional? > > Because, if we can find no matching mode, it seems like we should stick with > (-1) -- for "current text mode is not valid". In that case, the following passage > from the spec would apply: > > > On connecting to the output device the caller is required to verify > > the mode of the output device, and if it is not acceptable to set it > > to something it can use. > > Should we perhaps change the type of PreferMode to INT32, and set it > initially to (-1)? I think you are right. If no mode is queried, then invalid mode should be returned. > > > More comments: > > On 05/05/19 04:17, Gao, Zhichao wrote: > > + DEBUG ((DEBUG_INFO, "Graphics Console Started, Mode: %x\n\r", > > + PreferMode)); > > (3) DEBUG messages should not contain "\r" characters. My understanding is > that PrintLib adds "\r" automatically. > > > (4) UINTN values (which may be UINT64, dependent on build architecture) > should not be printed with "%x". UINTN should be converted to UINT64 > explicitly, for printing, and then printed with "%Lx" (or "%Lu"). > > (If you decide to switch PreferMode to INT32, then %d is the format specifier > to use, of course.) Good catch. I will make the Prefermode to INT32 and adjust the debug code. > > > (5) I applied the series, for testing, on top of commit fbb0ec7ea4c0 (current > master). I tested the patches briefly with OVMF. > > With the patches applied, the firmware crashes when I run "reconnect -r", as > the first command, in the built-in UEFI shell: > > > !!!! X64 Exception Type - 0D(#GP - General Protection) CPU Apic ID - > 00000000 !!!! > > ExceptionData - 0000000000000000 > > RIP - 000000007EECB8F9, CS - 0000000000000038, RFLAGS - > > 0000000000010012 RAX - 0000D0EC8148E589, RCX - 000000007EF06FE0, RDX > > - 000000000904A7A0 RBX - 0000000000000013, RSP - 000000007EEC9110, > > RBP - 000000007EEC9140 RSI - 000000007EEC92D8, RDI - 000000007EF07010 > > R8 - 0000000000000007, R9 - 000000012E9C46D6, R10 - 0000000400020100 > > R11 - 0000000000000008, R12 - 0000000000000000, R13 - > > 0000000000000000 > > R14 - 0000000000000000, R15 - 0000000000000000 > > DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030 > > GS - 0000000000000030, SS - 0000000000000030 > > CR0 - 0000000080010033, CR2 - 0000000000000000, CR3 - > > 000000007EC01000 > > CR4 - 0000000000000668, CR8 - 0000000000000000 > > DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - > > 0000000000000000 > > DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - > > 0000000000000400 GDTR - 000000007EBEDA98 0000000000000047, LDTR - > 0000000000000000 > > IDTR - 000000007E241018 0000000000000FFF, TR - 0000000000000000 > > FXSAVE_STATE - 000000007EEC8D70 > > !!!! Find image based on IP(0x7EECB8F9) > > > Build/Ovmf3264/NOOPT_GCC48/X64/MdeModulePkg/Core/Dxe/DxeMain/ > DEBUG/Dxe > > Core.dll (ImageBase=000000007EECB000, EntryPoint=000000007EED00C3) > > !!!! > > The crash offset is 0x000000007EECB8F9-0x000000007EECB000 = 0x8F9. > > The crash occurs in the InternalBaseLibIsListValid() function: > > > // > > // Count the total number of nodes in List. > > // Exit early if the number of nodes in List >= > PcdMaximumLinkedListLength > > // > > do { > > Ptr = Ptr->ForwardLink; > > 8f5: 48 8b 45 f0 mov -0x10(%rbp),%rax > > 8f9: 48 8b 00 mov (%rax),%rax <------- here > > 8fc: 48 89 45 f0 mov %rax,-0x10(%rbp) > > (RAX = 0xD0EC_8148_E589, which is approximately 208 TB, so obviously > something corrupted a list). > > The crash does not occur ("reconnect -r" works fine) if I build OVMF at > fbb0ec7ea4c0 (current master). I have tried that. Something wrong with the ClearScreen function. I will investigate it later. Thanks, Zhichao > > Thanks, > Laszlo > > > > > // > > // Install protocol interfaces for the Graphics Console device. > > @@ -669,6 +691,8 @@ GraphicsConsoleControllerDriverStop ( > > return EFI_NOT_STARTED; > > } > > > > + SimpleTextOutput->ClearScreen (SimpleTextOutput); > > + > > Private = GRAPHICS_CONSOLE_CON_OUT_DEV_FROM_THIS > > (SimpleTextOutput); > > > > Status = gBS->UninstallProtocolInterface ( diff --git > > > a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole > Dxe > > .inf > > > b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol > eDxe > > .inf > > index f7caa65aa9..bcfd306eee 100644 > > --- > > > a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole > Dxe > > .inf > > +++ > b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol > > +++ eDxe.inf > > @@ -65,6 +65,8 @@ > > [Pcd] > > gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution ## > SOMETIMES_CONSUMES > > gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution ## > SOMETIMES_CONSUMES > > + gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow ## > SOMETIMES_CONSUMES > > + gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn ## > SOMETIMES_CONSUMES > > > > [UserExtensions.TianoCore."ExtraFiles"] > > GraphicsConsoleDxeExtra.uni > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-05-07 7:11 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-05-05 2:17 [PATCH V3 0/2] MdeModulePkg: Make the screen seamless Gao, Zhichao 2019-05-05 2:17 ` [PATCH V3 1/2] MdeModulePkg/ConSplitterDxe: Optimize the ConSplitterTextOutSetMode Gao, Zhichao 2019-05-05 2:17 ` [PATCH V3 2/2] MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode Gao, Zhichao 2019-05-06 17:42 ` [edk2-devel] " Laszlo Ersek 2019-05-07 7:11 ` Gao, Zhichao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox