From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B3622212DC5CD for ; Tue, 12 Jun 2018 10:50:59 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D10D5402242D for ; Tue, 12 Jun 2018 17:50:58 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-239.rdu2.redhat.com [10.10.120.239]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0D2BE2166BB2; Tue, 12 Jun 2018 17:50:57 +0000 (UTC) To: Gerd Hoffmann , edk2-devel@lists.01.org References: <20180612093117.27028-1-kraxel@redhat.com> <20180612093117.27028-3-kraxel@redhat.com> From: Laszlo Ersek Message-ID: Date: Tue, 12 Jun 2018 19:50:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180612093117.27028-3-kraxel@redhat.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 12 Jun 2018 17:50:58 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 12 Jun 2018 17:50:58 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH v2 2/4] OvmfPkg: add QemuRamfbDxe X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Jun 2018 17:51:00 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Gerd, On 06/12/18 11:31, Gerd Hoffmann wrote: > Add a driver for the qemu ramfb display device. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Gerd Hoffmann > --- > OvmfPkg/QemuRamfbDxe/QemuRamfb.c | 396 ++++++++++++++++++++++++++++++++++ > OvmfPkg/OvmfPkgIa32.dsc | 1 + > OvmfPkg/OvmfPkgIa32.fdf | 1 + > OvmfPkg/OvmfPkgIa32X64.dsc | 1 + > OvmfPkg/OvmfPkgIa32X64.fdf | 1 + > OvmfPkg/OvmfPkgX64.dsc | 1 + > OvmfPkg/OvmfPkgX64.fdf | 1 + > OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf | 52 +++++ > 8 files changed, 454 insertions(+) > create mode 100644 OvmfPkg/QemuRamfbDxe/QemuRamfb.c > create mode 100644 OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf > > diff --git a/OvmfPkg/QemuRamfbDxe/QemuRamfb.c b/OvmfPkg/QemuRamfbDxe/QemuRamfb.c > new file mode 100644 > index 0000000000..00a2875e99 > --- /dev/null > +++ b/OvmfPkg/QemuRamfbDxe/QemuRamfb.c > @@ -0,0 +1,396 @@ > +/** @file > + This driver is a implementation of the Graphics Output Protocol > + for the QEMU ramfb device. > + > + Copyright (c) 2018, Red Hat Inc. > + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD License > + which accompanies this distribution. The full text of the license may be found at (1) These lines are overlong, can you please wrap them to 80 chars? > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#define RAMFB_FORMAT 0x34325258 /* DRM_FORMAT_XRGB8888 */ > +#define RAMFB_BPP 4 > + > +#pragma pack (1) > +typedef struct RAMFB_CONFIG { > + UINT64 Address; > + UINT32 FourCC; > + UINT32 Flags; > + UINT32 Width; > + UINT32 Height; > + UINT32 Stride; > +} RAMFB_CONFIG; > +#pragma pack () > + > +static EFI_HANDLE mRamfbHandle; (2) So, when I mentioned "STATIC" in my last email, I meant "STATIC", not "static" :) I see you couldn't believe me! :) I apologize; yes, indeed in edk2 we spell "static" "STATIC". Can you please update all occurrences? > +static EFI_HANDLE mGopHandle; > +static FRAME_BUFFER_CONFIGURE *mQemuRamfbFrameBufferBltConfigure; > +static UINTN mQemuRamfbFrameBufferBltConfigureSize; > +static FIRMWARE_CONFIG_ITEM mRamFbFwCfgItem; (3) My bad, I suggested inconsistent spelling for "mRamFbFwCfgItem". You spelled it everywhere consistently "Ramfb", in identifiers. Can you please update this to "mRamfbFwCfgItem" too? > + > +static EFI_GRAPHICS_OUTPUT_MODE_INFORMATION mQemuRamfbModeInfo[] = { > + { > + 0, // Version > + 640, // HorizontalResolution > + 480, // VerticalResolution > + },{ > + 0, // Version > + 800, // HorizontalResolution > + 600, // VerticalResolution > + },{ > + 0, // Version > + 1024, // HorizontalResolution > + 768, // VerticalResolution > + } > +}; > +#define mQemuRamfbModeCount ARRAY_SIZE(mQemuRamfbModeInfo) (4) Sorry I was unclear about this in my last email. What I meant is to eliminate the "mQemuRamfbModeCount" macro entirely, and use the ARRAY_SIZE() in its place. I count two instances in the code. Alternatively, if you really like a dedicated macro for this, please spell it QEMU_RAMFB_MODE_COUNT. Either way: please insert a space character after ARRAY_SIZE and before "(". > + > +static EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE mQemuRamfbMode = { > + mQemuRamfbModeCount, // MaxMode > + 0, // Mode > + mQemuRamfbModeInfo, // Info > + sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION), // SizeOfInfo > +}; > + > +static > +EFI_STATUS > +EFIAPI > +QemuRamfbGraphicsOutputQueryMode ( > + IN EFI_GRAPHICS_OUTPUT_PROTOCOL *This, > + IN UINT32 ModeNumber, > + OUT UINTN *SizeOfInfo, > + OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION **Info > + ) > +{ > + EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *ModeInfo; > + > + if (Info == NULL || SizeOfInfo == NULL || ModeNumber >= mQemuRamfbMode.MaxMode) { (5) This line is too long; please use a 80 chars width. > + return EFI_INVALID_PARAMETER; > + } > + ModeInfo = &mQemuRamfbModeInfo[ModeNumber]; > + > + *Info = AllocateCopyPool (sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION), > + ModeInfo); > + if (*Info == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + *SizeOfInfo = sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION); > + > + return EFI_SUCCESS; > +} > + > +static > +EFI_STATUS > +EFIAPI > +QemuRamfbGraphicsOutputSetMode ( > + IN EFI_GRAPHICS_OUTPUT_PROTOCOL *This, > + IN UINT32 ModeNumber > + ) > +{ > + EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *ModeInfo; > + RAMFB_CONFIG Config; > + EFI_GRAPHICS_OUTPUT_BLT_PIXEL Black; > + RETURN_STATUS Status; > + > + if (ModeNumber >= mQemuRamfbMode.MaxMode) { > + return EFI_UNSUPPORTED; > + } > + ModeInfo = &mQemuRamfbModeInfo[ModeNumber]; > + > + DEBUG ((DEBUG_INFO, "Ramfb: SetMode %u (%ux%u)\n", ModeNumber, > + ModeInfo->HorizontalResolution, ModeInfo->VerticalResolution)); > + > + Config.Address = SwapBytes64 (mQemuRamfbMode.FrameBufferBase); > + Config.FourCC = SwapBytes32 (RAMFB_FORMAT); > + Config.Flags = SwapBytes32 (0); > + Config.Width = SwapBytes32 (ModeInfo->HorizontalResolution); > + Config.Height = SwapBytes32 (ModeInfo->VerticalResolution); > + Config.Stride = SwapBytes32 (ModeInfo->HorizontalResolution * RAMFB_BPP); > + > + Status = FrameBufferBltConfigure ( > + (VOID*)(UINTN)mQemuRamfbMode.FrameBufferBase, > + ModeInfo, > + mQemuRamfbFrameBufferBltConfigure, > + &mQemuRamfbFrameBufferBltConfigureSize > + ); > + > + if (Status == RETURN_BUFFER_TOO_SMALL) { > + if (mQemuRamfbFrameBufferBltConfigure != NULL) { > + FreePool (mQemuRamfbFrameBufferBltConfigure); > + } > + mQemuRamfbFrameBufferBltConfigure = > + AllocatePool (mQemuRamfbFrameBufferBltConfigureSize); > + if (mQemuRamfbFrameBufferBltConfigure == NULL) { > + return EFI_OUT_OF_RESOURCES; (6) Small logic issue here: at this point "mQemuRamfbFrameBufferBltConfigure" is NULL (which is good), but "mQemuRamfbFrameBufferBltConfigureSize" is nonzero (in fact it is "large enough"; it's just been returned to us with the correct size). As a result, at the next attempt to set the same mode, FrameBufferBltConfigure() will dereference NULL. So please, before returning, assign "mQemuRamfbFrameBufferBltConfigureSize" zero. > + } > + > + Status = FrameBufferBltConfigure ( > + (VOID*)(UINTN)mQemuRamfbMode.FrameBufferBase, > + ModeInfo, > + mQemuRamfbFrameBufferBltConfigure, > + &mQemuRamfbFrameBufferBltConfigureSize); (7) Please break off the ");" to a separate line, as discussed before. > + if (RETURN_ERROR (Status)) { > + ASSERT (Status == RETURN_UNSUPPORTED); > + return Status; > + } (8) I think I was unclear about this in my last email, sorry about that again. I meant for this RETURN_ERROR() check to be placed *after* (not inside) the RETURN_BUFFER_TOO_SMALL branch. With the idea being, after the end of the "too small" branch, regardless of us actually having taken the "too small" branch, Status must either be "success", or "unsupported". Currently the code, as-is, will not catch the first FrameBufferBltConfigure() call returning "unsupported". > + } > + > + mQemuRamfbMode.Mode = ModeNumber; > + mQemuRamfbMode.Info = ModeInfo; > + > + QemuFwCfgSelectItem (mRamFbFwCfgItem); > + QemuFwCfgWriteBytes (sizeof (Config), &Config); > + > + // > + // clear screen > + // > + ZeroMem (&Black, sizeof (Black)); > + Status = FrameBufferBlt ( > + mQemuRamfbFrameBufferBltConfigure, > + &Black, > + EfiBltVideoFill, > + 0, // SourceX -- ignored > + 0, // SourceY -- ignored > + 0, // DestinationX > + 0, // DestinationY > + ModeInfo->HorizontalResolution, // Width > + ModeInfo->VerticalResolution, // Height > + 0 // Delta -- ignored > + ); > + if (EFI_ERROR (Status)) { (9) For stylistic reasons, please use "RETURN_ERROR (Status)" here. (See the previous discussion on RETURN_STATUS vs. EFI_STATUS.) > + DEBUG ((DEBUG_WARN, "%a: clearing the screen failed: %r\n", > + __FUNCTION__, Status)); (10) Please fix the indentation. > + } > + > + return EFI_SUCCESS; > +} > + > +static > +EFI_STATUS > +EFIAPI > +QemuRamfbGraphicsOutputBlt ( > + IN EFI_GRAPHICS_OUTPUT_PROTOCOL *This, > + IN EFI_GRAPHICS_OUTPUT_BLT_PIXEL *BltBuffer, OPTIONAL > + IN EFI_GRAPHICS_OUTPUT_BLT_OPERATION BltOperation, > + IN UINTN SourceX, > + IN UINTN SourceY, > + IN UINTN DestinationX, > + IN UINTN DestinationY, > + IN UINTN Width, > + IN UINTN Height, > + IN UINTN Delta > + ) > +{ > + return FrameBufferBlt ( > + mQemuRamfbFrameBufferBltConfigure, > + BltBuffer, > + BltOperation, > + SourceX, > + SourceY, > + DestinationX, > + DestinationY, > + Width, > + Height, > + Delta); (11) Please break off the ");" to a separate line. > +} > + > +EFI_GRAPHICS_OUTPUT_PROTOCOL QemuRamfbGraphicsOutput = { (12) Please make this STATIC. (13) Please rename it to "mQemuRamfbGraphicsOutput". > + QemuRamfbGraphicsOutputQueryMode, > + QemuRamfbGraphicsOutputSetMode, > + QemuRamfbGraphicsOutputBlt, > + &mQemuRamfbMode, > +}; > + > +EFI_STATUS > +EFIAPI > +InitializeQemuRamfb ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_DEVICE_PATH_PROTOCOL *RamfbDevicePath; > + EFI_DEVICE_PATH_PROTOCOL *GopDevicePath; > + VOID *DevicePath; > + VENDOR_DEVICE_PATH VendorDeviceNode; > + ACPI_ADR_DEVICE_PATH AcpiDeviceNode; > + EFI_STATUS Status; > + EFI_PHYSICAL_ADDRESS FbBase; > + UINTN FbSize, MaxFbSize; > + UINTN Size, Pages, Index; (14) Can you rename "Size" to "FwCfgSize" as I requested earlier please? (15) I see that you are reluctant accepting that edk2 really prefers to break almost all variable definitions to separate lines :) I suggest the following compromise: UINTN FbSize, MaxFbSize, Pages; UINTN FwCfgSize; UINTN Index; Because, an argument can be made that FbSize, MaxFbSize, and Pages belong together. However, FwCfgSize and Index should each stand alone. > + > + if (!QemuFwCfgIsAvailable()) { (16) Oops, I missed this last time; please add a space between function identifier and (). > + DEBUG ((DEBUG_INFO, "Ramfb: no FwCfg\n")); > + return EFI_NOT_FOUND; > + } > + > + Status = QemuFwCfgFindFile("etc/ramfb", &mRamFbFwCfgItem, &Size); (17) Ditto. > + if (EFI_ERROR (Status)) { > + return EFI_NOT_FOUND; > + } > + if (Size != sizeof (RAMFB_CONFIG)) { > + DEBUG ((DEBUG_ERROR, "Ramfb: FwCfg size mismatch (expected %d, got %d)\n", > + sizeof (RAMFB_CONFIG), Size)); (18) Indentation. (19) Additionally, "sizeof" has type UINTN, and "Size" (FwCfgSize) has type UINTN too. Please cast them both to UINT64 and print them with %lu. > + return EFI_NOT_FOUND; (20) I generally return EFI_PROTOCOL_ERROR whenever fw_cfg produces garbage, but EFI_NOT_FOUND is perfectly acceptable as well. Up to you. > + } > + > + MaxFbSize = 0; > + for (Index = 0; Index < mQemuRamfbModeCount; Index++) { > + mQemuRamfbModeInfo[Index].PixelsPerScanLine = > + mQemuRamfbModeInfo[Index].HorizontalResolution; > + mQemuRamfbModeInfo[Index].PixelFormat = > + PixelBlueGreenRedReserved8BitPerColor; > + FbSize = RAMFB_BPP * > + mQemuRamfbModeInfo[Index].HorizontalResolution * > + mQemuRamfbModeInfo[Index].VerticalResolution; > + if (MaxFbSize < FbSize) { > + MaxFbSize = FbSize; > + } > + DEBUG ((DEBUG_INFO, "Ramfb: Mode %u: %ux%u, %lu kB\n", (UINT64)Index, (21) The cast is OK, but the conversion specifier should be %lu. > + mQemuRamfbModeInfo[Index].HorizontalResolution, > + mQemuRamfbModeInfo[Index].VerticalResolution, > + (UINT64)FbSize / 1024)); (22) Sorry, this division is no longer portable (between 64-bit and 32-bit edk2 builds) in this form. The reason is that UINT64 values can't *generally* be divided in 32-bit builds without compiler intrinsics; in order to avoid those, we use named helper functions for such divisions. However, using those helpers wasn't what I meant; I meant (UINT64)(FbSize / 1024) Because, FbSize is UINTN (i.e. UINT32 in 32-bit builds, UINT64 in 64-bit builds), and so it can be divided by 1024 (of type INT32) without helpers. It's the UINTN *quotient* that we should cast, for portable printing. I apologize if my previous comment on this was misleading. > + } > + > + Pages = EFI_SIZE_TO_PAGES (MaxFbSize); > + MaxFbSize = EFI_PAGES_TO_SIZE (Pages); > + FbBase = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateReservedPages (Pages); > + if (FbBase == 0) { > + DEBUG ((DEBUG_INFO, "Ramfb: memory allocation failed\n")); (23) Sorry, missed this last time, should be DEBUG_ERROR. > + return EFI_OUT_OF_RESOURCES; > + } > + DEBUG ((DEBUG_INFO, "Ramfb: Framebuffer at 0x%lx, %ld kB, %ld pages\n", > + (UINT64)FbBase, (UINT64)MaxFbSize / 1024, (UINT64)Pages)); (24) Indentation. (25) Regarding the MaxFbSize division, please see (22). (26) Please use %lu for printing both the MaxFbSize quotient (see above) and Pages. %ld is for INT64. > + mQemuRamfbMode.FrameBufferSize = MaxFbSize; > + mQemuRamfbMode.FrameBufferBase = FbBase; > + > + // > + // 800 x 600 > + // > + QemuRamfbGraphicsOutputSetMode (&QemuRamfbGraphicsOutput, 1); > + > + // > + // ramfb vendor devpath > + // > + VendorDeviceNode.Header.Type = HARDWARE_DEVICE_PATH; > + VendorDeviceNode.Header.SubType = HW_VENDOR_DP; > + CopyGuid(&VendorDeviceNode.Guid, &gQemuRamfbGuid); (27) Please add a space after "CopyGuid". > + SetDevicePathNodeLength (&VendorDeviceNode.Header, > + sizeof (VENDOR_DEVICE_PATH)); > + > + RamfbDevicePath = AppendDevicePathNode (NULL, > + (EFI_DEVICE_PATH_PROTOCOL *) &VendorDeviceNode); > + if (RamfbDevicePath == NULL) { > + Status = EFI_OUT_OF_RESOURCES; > + goto FreeFramebuffer; > + } > + > + Status = gBS->InstallMultipleProtocolInterfaces ( > + &mRamfbHandle, > + &gEfiDevicePathProtocolGuid, > + RamfbDevicePath, > + NULL > + ); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Ramfb: install Ramfb Vendor DevicePath failed: %r\n", > + Status)); > + goto FreeRamfbDevicePath; > + } > + > + // > + // gop devpath + protocol > + // > + ZeroMem (&AcpiDeviceNode, sizeof (ACPI_ADR_DEVICE_PATH)); (28) I think you can drop the ZeroMem(); all fields are set explicitly. > + AcpiDeviceNode.Header.Type = ACPI_DEVICE_PATH; > + AcpiDeviceNode.Header.SubType = ACPI_ADR_DP; > + AcpiDeviceNode.ADR = ACPI_DISPLAY_ADR ( > + 1, // DeviceIdScheme > + 0, // HeadId > + 0, // NonVgaOutput > + 1, // BiosCanDetect > + 0, // VendorInfo > + ACPI_ADR_DISPLAY_TYPE_EXTERNAL_DIGITAL, // Type > + 0, // Port > + 0 // Index > + ); Sigh, this is one of those situations when different coding style guidelines conflict. One style requirement would be to indent the arguments two columns to the right of the beginning of "ACPI_DISPLAY_ADR". However, in that case, we wouldn't fit in 80 characters; and in my book, that's a lot worse. So, just leave this as it is. :/ > + SetDevicePathNodeLength (&AcpiDeviceNode.Header, > + sizeof (ACPI_ADR_DEVICE_PATH)); > + > + GopDevicePath = AppendDevicePathNode (RamfbDevicePath, > + (EFI_DEVICE_PATH_PROTOCOL *) &AcpiDeviceNode); > + if (GopDevicePath == NULL) { > + Status = EFI_OUT_OF_RESOURCES; > + goto FreeRamfbHandle; > + } > + > + Status = gBS->InstallMultipleProtocolInterfaces ( > + &mGopHandle, > + &gEfiDevicePathProtocolGuid, > + GopDevicePath, > + &gEfiGraphicsOutputProtocolGuid, > + &QemuRamfbGraphicsOutput, > + NULL > + ); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Ramfb: install GOP DevicePath failed: %r\n", > + Status)); > + goto FreeGopDevicePath; > + } > + > + Status = gBS->OpenProtocol ( > + mRamfbHandle, > + &gEfiDevicePathProtocolGuid, > + &DevicePath, > + gImageHandle, > + mGopHandle, > + EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER > + ); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Ramfb: OpenProtocol failed: %r\n", Status)); > + goto FreeGopHandle; > + } > + > + return EFI_SUCCESS; > + > +FreeGopHandle: > + gBS->UninstallMultipleProtocolInterfaces ( > + mGopHandle, > + &gEfiDevicePathProtocolGuid, > + GopDevicePath, > + &gEfiGraphicsOutputProtocolGuid, > + &QemuRamfbGraphicsOutput, > + NULL > + ); > +FreeGopDevicePath: > + FreePool (GopDevicePath); > +FreeRamfbHandle: > + gBS->UninstallMultipleProtocolInterfaces ( > + mRamfbHandle, > + &gEfiDevicePathProtocolGuid, > + RamfbDevicePath, > + NULL > + ); > +FreeRamfbDevicePath: > + FreePool (RamfbDevicePath); > +FreeFramebuffer: > + FreePool ((VOID*)(UINTN)mQemuRamfbMode.FrameBufferBase); (29) Oops, completely missed this in the previous review, multiple times -- please use FreePages() for releasing FrameBufferBase, not FreePool()! > + return Status; > +} > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index a2c995b910..7ddda89999 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -745,6 +745,7 @@ > MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf > > OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > + OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf > OvmfPkg/VirtioGpuDxe/VirtioGpu.inf > > # > diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf > index b199713925..52b8b1fea1 100644 > --- a/OvmfPkg/OvmfPkgIa32.fdf > +++ b/OvmfPkg/OvmfPkgIa32.fdf > @@ -351,6 +351,7 @@ INF RuleOverride=CSM OvmfPkg/Csm/Csm16/Csm16.inf > !endif > > INF OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > +INF OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf > INF OvmfPkg/VirtioGpuDxe/VirtioGpu.inf > INF OvmfPkg/PlatformDxe/Platform.inf > INF OvmfPkg/IoMmuDxe/IoMmuDxe.inf > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index bc7db229d2..3481cdc36b 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -754,6 +754,7 @@ > MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf > > OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > + OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf > OvmfPkg/VirtioGpuDxe/VirtioGpu.inf > > # > diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf > index 4ebf64b2b9..70845d6972 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.fdf > +++ b/OvmfPkg/OvmfPkgIa32X64.fdf > @@ -357,6 +357,7 @@ INF RuleOverride=CSM OvmfPkg/Csm/Csm16/Csm16.inf > !endif > > INF OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > +INF OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf > INF OvmfPkg/VirtioGpuDxe/VirtioGpu.inf > INF OvmfPkg/PlatformDxe/Platform.inf > INF OvmfPkg/AmdSevDxe/AmdSevDxe.inf > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 0767b34d18..8b0895b0ff 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -752,6 +752,7 @@ > MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf > > OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > + OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf > OvmfPkg/VirtioGpuDxe/VirtioGpu.inf > > # > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > index 9ca96f9282..1eb46ac9a2 100644 > --- a/OvmfPkg/OvmfPkgX64.fdf > +++ b/OvmfPkg/OvmfPkgX64.fdf > @@ -357,6 +357,7 @@ INF RuleOverride=CSM OvmfPkg/Csm/Csm16/Csm16.inf > !endif > > INF OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > +INF OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf > INF OvmfPkg/VirtioGpuDxe/VirtioGpu.inf > INF OvmfPkg/PlatformDxe/Platform.inf > INF OvmfPkg/AmdSevDxe/AmdSevDxe.inf > diff --git a/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf b/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf > new file mode 100644 > index 0000000000..5a9fd42c32 > --- /dev/null > +++ b/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf > @@ -0,0 +1,52 @@ > +## @file > +# This driver is a implementation of the Graphics Output Protocol > +# for the QEMU ramfb device. > +# > +# Copyright (c) 2018, Red Hat Inc. > +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at (30) These two lines are too long, can you please rewrap them to 80 chars? > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = QemuRamfbDxe > + FILE_GUID = dce1b094-7dc6-45d0-9fdd-d7fc3cc3e4ef > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + > + ENTRY_POINT = InitializeQemuRamfb > + > +[Sources] > + QemuRamfb.c > + > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[LibraryClasses] > + BaseLib > + BaseMemoryLib > + DebugLib > + DevicePathLib > + FrameBufferBltLib > + MemoryAllocationLib > + UefiBootServicesTableLib > + UefiDriverEntryPoint > + QemuFwCfgLib > + > +[Protocols] > + gEfiGraphicsOutputProtocolGuid # BY_START (31) As requested previously, drop "# BY_START" and say ""## PRODUCES" please. > + > +[Guids] > + gQemuRamfbGuid > + > +[Depex] > + TRUE > Thanks! Laszlo