* [PATCH 0/4] Add QemuRamfbDxe driver
@ 2018-06-08 11:39 Gerd Hoffmann
2018-06-08 11:39 ` [PATCH 1/4] OvmfPkg: add QEMU_RAMFB_GUID Gerd Hoffmann
` (4 more replies)
0 siblings, 5 replies; 18+ messages in thread
From: Gerd Hoffmann @ 2018-06-08 11:39 UTC (permalink / raw)
To: edk2-devel
This is the efi driver for qemu ramfb, a simple boot framebuffer.
Qemu patches just have been posted to qemu-devel.
Gerd Hoffmann (4):
OvmfPkg: add QEMU_RAMFB_GUID
OvmfPkg: add QemuRamfbDxe
OvmfPkg: add QemuRamfb to platform console
ArmVirtPkg: add QemuRamfbDxe
OvmfPkg/Include/Guid/QemuRamfb.h | 25 ++
.../Library/PlatformBootManagerLib/PlatformData.c | 44 +++
OvmfPkg/QemuRamfbDxe/QemuRamfb.c | 308 +++++++++++++++++++++
ArmVirtPkg/ArmVirtQemu.dsc | 2 +
ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 +
OvmfPkg/OvmfPkg.dec | 1 +
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 | 34 +++
14 files changed, 423 insertions(+)
create mode 100644 OvmfPkg/Include/Guid/QemuRamfb.h
create mode 100644 OvmfPkg/QemuRamfbDxe/QemuRamfb.c
create mode 100644 OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
--
2.9.3
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/4] OvmfPkg: add QEMU_RAMFB_GUID
2018-06-08 11:39 [PATCH 0/4] Add QemuRamfbDxe driver Gerd Hoffmann
@ 2018-06-08 11:39 ` Gerd Hoffmann
2018-06-11 13:06 ` Laszlo Ersek
2018-06-08 11:39 ` [PATCH 2/4] OvmfPkg: add QemuRamfbDxe Gerd Hoffmann
` (3 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Gerd Hoffmann @ 2018-06-08 11:39 UTC (permalink / raw)
To: edk2-devel
Add GUID header file for the QemuRamfbDxe driver.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
OvmfPkg/Include/Guid/QemuRamfb.h | 25 +++++++++++++++++++++++++
OvmfPkg/OvmfPkg.dec | 1 +
2 files changed, 26 insertions(+)
create mode 100644 OvmfPkg/Include/Guid/QemuRamfb.h
diff --git a/OvmfPkg/Include/Guid/QemuRamfb.h b/OvmfPkg/Include/Guid/QemuRamfb.h
new file mode 100644
index 0000000000..878224599d
--- /dev/null
+++ b/OvmfPkg/Include/Guid/QemuRamfb.h
@@ -0,0 +1,25 @@
+/** @file
+ Recommended GUID to be used in the Vendor Hardware device path nodes that
+ identify qemu ramfb devices.
+
+ 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 that accompanies this
+ distribution. The full text of the license may be found at
+ 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.
+
+**/
+
+#ifndef __QEMU_RAMFB_H__
+#define __QEMU_RAMFB_H__
+
+#define QEMU_RAMFB_GUID \
+{0x837dca9e, 0xe874, 0x4d82, {0xb2, 0x9a, 0x23, 0xfe, 0x0e, 0x23, 0xd1, 0xe2}}
+
+extern EFI_GUID gQemuRamfbGuid;
+
+#endif
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index dc5597db41..a16c22a289 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -75,6 +75,7 @@
gEfiXenInfoGuid = {0xd3b46f3b, 0xd441, 0x1244, {0x9a, 0x12, 0x0, 0x12, 0x27, 0x3f, 0xc1, 0x4d}}
gOvmfPlatformConfigGuid = {0x7235c51c, 0x0c80, 0x4cab, {0x87, 0xac, 0x3b, 0x08, 0x4a, 0x63, 0x04, 0xb1}}
gVirtioMmioTransportGuid = {0x837dca9e, 0xe874, 0x4d82, {0xb2, 0x9a, 0x23, 0xfe, 0x0e, 0x23, 0xd1, 0xe2}}
+ gQemuRamfbGuid = {0x837dca9e, 0xe874, 0x4d82, {0xb2, 0x9a, 0x23, 0xfe, 0x0e, 0x23, 0xd1, 0xe2}}
gXenBusRootDeviceGuid = {0xa732241f, 0x383d, 0x4d9c, {0x8a, 0xe1, 0x8e, 0x09, 0x83, 0x75, 0x89, 0xd7}}
gRootBridgesConnectedEventGroupGuid = {0x24a2d66f, 0xeedd, 0x4086, {0x90, 0x42, 0xf2, 0x6e, 0x47, 0x97, 0xee, 0x69}}
--
2.9.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/4] OvmfPkg: add QemuRamfbDxe
2018-06-08 11:39 [PATCH 0/4] Add QemuRamfbDxe driver Gerd Hoffmann
2018-06-08 11:39 ` [PATCH 1/4] OvmfPkg: add QEMU_RAMFB_GUID Gerd Hoffmann
@ 2018-06-08 11:39 ` Gerd Hoffmann
2018-06-10 5:57 ` Ard Biesheuvel
2018-06-11 15:56 ` Laszlo Ersek
2018-06-08 11:39 ` [PATCH 3/4] OvmfPkg: add QemuRamfb to platform console Gerd Hoffmann
` (2 subsequent siblings)
4 siblings, 2 replies; 18+ messages in thread
From: Gerd Hoffmann @ 2018-06-08 11:39 UTC (permalink / raw)
To: edk2-devel
Add a driver for the qemu ramfb display device.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
OvmfPkg/QemuRamfbDxe/QemuRamfb.c | 308 ++++++++++++++++++++++++++++++++++
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 | 34 ++++
8 files changed, 348 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..f04a314c24
--- /dev/null
+++ b/OvmfPkg/QemuRamfbDxe/QemuRamfb.c
@@ -0,0 +1,308 @@
+#include <Uefi.h>
+#include <Protocol/GraphicsOutput.h>
+
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/DevicePathLib.h>
+#include <Library/FrameBufferBltLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/UefiBootManagerLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiDriverEntryPoint.h>
+#include <Library/UefiLib.h>
+#include <Library/QemuFwCfgLib.h>
+
+#include <Guid/QemuRamfb.h>
+
+#define RAMFB_FORMAT 0x34325258 /* DRM_FORMAT_XRGB8888 */
+#define RAMFB_BPP 4
+
+EFI_GUID gQemuRamfbGuid = QEMU_RAMFB_GUID;
+
+typedef struct RAMFB_CONFIG {
+ UINT64 Address;
+ UINT32 FourCC;
+ UINT32 Flags;
+ UINT32 Width;
+ UINT32 Height;
+ UINT32 Stride;
+} RAMFB_CONFIG;
+
+EFI_HANDLE RamfbHandle;
+EFI_HANDLE GopHandle;
+FRAME_BUFFER_CONFIGURE *QemuRamfbFrameBufferBltConfigure;
+UINTN QemuRamfbFrameBufferBltConfigureSize;
+
+EFI_GRAPHICS_OUTPUT_MODE_INFORMATION QemuRamfbModeInfo[] = {
+ {
+ .HorizontalResolution = 640,
+ .VerticalResolution = 480,
+ },{
+ .HorizontalResolution = 800,
+ .VerticalResolution = 600,
+ },{
+ .HorizontalResolution = 1024,
+ .VerticalResolution = 768,
+ }
+};
+#define QemuRamfbModeCount (sizeof(QemuRamfbModeInfo)/sizeof(QemuRamfbModeInfo[0]))
+
+EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE QemuRamfbMode = {
+ .MaxMode = QemuRamfbModeCount,
+ .Mode = 0,
+ .Info = QemuRamfbModeInfo,
+ .SizeOfInfo = sizeof(EFI_GRAPHICS_OUTPUT_MODE_INFORMATION),
+};
+
+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 > QemuRamfbMode.MaxMode) {
+ return EFI_INVALID_PARAMETER;
+ }
+ ModeInfo = &QemuRamfbModeInfo[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;
+}
+
+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 Ret;
+ FIRMWARE_CONFIG_ITEM Item;
+ UINTN Size;
+
+ if (ModeNumber > QemuRamfbMode.MaxMode) {
+ return EFI_UNSUPPORTED;
+ }
+ ModeInfo = &QemuRamfbModeInfo[ModeNumber];
+
+ DEBUG ((EFI_D_INFO, "Ramfb: SetMode %d (%dx%d)\n", ModeNumber,
+ ModeInfo->HorizontalResolution,
+ ModeInfo->VerticalResolution));
+
+ QemuRamfbMode.Mode = ModeNumber;
+ QemuRamfbMode.Info = ModeInfo;
+
+ Config.Address = SwapBytes64( QemuRamfbMode.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 );
+
+ QemuFwCfgFindFile("etc/ramfb", &Item, &Size);
+ QemuFwCfgSelectItem(Item);
+ QemuFwCfgWriteBytes(sizeof(Config), &Config);
+
+ Ret = FrameBufferBltConfigure (
+ (VOID*)(UINTN)QemuRamfbMode.FrameBufferBase,
+ ModeInfo,
+ QemuRamfbFrameBufferBltConfigure,
+ &QemuRamfbFrameBufferBltConfigureSize);
+
+ if (Ret == RETURN_BUFFER_TOO_SMALL) {
+ if (QemuRamfbFrameBufferBltConfigure != NULL) {
+ FreePool(QemuRamfbFrameBufferBltConfigure);
+ }
+ QemuRamfbFrameBufferBltConfigure =
+ AllocatePool(QemuRamfbFrameBufferBltConfigureSize);
+
+ Ret = FrameBufferBltConfigure (
+ (VOID*)(UINTN)QemuRamfbMode.FrameBufferBase,
+ ModeInfo,
+ QemuRamfbFrameBufferBltConfigure,
+ &QemuRamfbFrameBufferBltConfigureSize);
+ }
+
+ /* clear screen */
+ ZeroMem (&Black, sizeof (Black));
+ Ret = FrameBufferBlt (
+ QemuRamfbFrameBufferBltConfigure,
+ &Black,
+ EfiBltVideoFill,
+ 0, 0,
+ 0, 0,
+ ModeInfo->HorizontalResolution,
+ ModeInfo->VerticalResolution,
+ 0
+ );
+
+ return EFI_SUCCESS;
+}
+
+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 (
+ QemuRamfbFrameBufferBltConfigure,
+ BltBuffer,
+ BltOperation,
+ SourceX,
+ SourceY,
+ DestinationX,
+ DestinationY,
+ Width,
+ Height,
+ Delta);
+}
+
+EFI_GRAPHICS_OUTPUT_PROTOCOL QemuRamfbGraphicsOutput = {
+ .QueryMode = QemuRamfbGraphicsOutputQueryMode,
+ .SetMode = QemuRamfbGraphicsOutputSetMode,
+ .Blt = QemuRamfbGraphicsOutputBlt,
+ .Mode = &QemuRamfbMode,
+};
+
+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;
+ RETURN_STATUS Ret;
+ FIRMWARE_CONFIG_ITEM Item;
+ EFI_PHYSICAL_ADDRESS FbBase;
+ UINTN Size, FbSize, MaxFbSize, Pages, Index;
+
+ DEBUG ((EFI_D_INFO, "InitializeQemuRamfb\n"));
+
+ if (!QemuFwCfgIsAvailable()) {
+ DEBUG ((EFI_D_INFO, "Ramfb: no FwCfg\n"));
+ return EFI_NOT_FOUND;
+ }
+
+ Ret = QemuFwCfgFindFile("etc/ramfb", &Item, &Size);
+ if (Ret != RETURN_SUCCESS) {
+ DEBUG ((EFI_D_INFO, "Ramfb: no etc/ramfb in FwCfg\n"));
+ return EFI_NOT_FOUND;
+ }
+
+ MaxFbSize = 0;
+ for (Index = 0; Index < QemuRamfbModeCount; Index++) {
+ QemuRamfbModeInfo[Index].PixelsPerScanLine =
+ QemuRamfbModeInfo[Index].HorizontalResolution;
+ QemuRamfbModeInfo[Index].PixelFormat =
+ PixelBlueGreenRedReserved8BitPerColor,
+ FbSize = RAMFB_BPP *
+ QemuRamfbModeInfo[Index].HorizontalResolution *
+ QemuRamfbModeInfo[Index].VerticalResolution;
+ if (MaxFbSize < FbSize)
+ MaxFbSize = FbSize;
+ DEBUG ((EFI_D_INFO, "Ramfb: Mode %d: %dx%d, %d kB\n", Index,
+ QemuRamfbModeInfo[Index].HorizontalResolution,
+ QemuRamfbModeInfo[Index].VerticalResolution,
+ FbSize / 1024));
+ }
+
+ Pages = EFI_SIZE_TO_PAGES(MaxFbSize);
+ MaxFbSize = EFI_PAGES_TO_SIZE(Pages);
+ FbBase = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateRuntimePages(Pages);
+ if (!FbBase) {
+ DEBUG ((EFI_D_INFO, "Ramfb: memory allocation failed\n"));
+ return EFI_OUT_OF_RESOURCES;
+ }
+ DEBUG ((EFI_D_INFO, "Ramfb: Framebuffer at 0x%lx, %d kB, %d pages\n",
+ FbBase, MaxFbSize / 1024, Pages));
+ QemuRamfbMode.FrameBufferSize = MaxFbSize;
+ QemuRamfbMode.FrameBufferBase = FbBase;
+
+ /* 800 x 600 */
+ QemuRamfbGraphicsOutputSetMode (&QemuRamfbGraphicsOutput, 1);
+
+ /* ramfb vendor devpath */
+ ZeroMem (&VendorDeviceNode, sizeof (VENDOR_DEVICE_PATH));
+ VendorDeviceNode.Header.Type = HARDWARE_DEVICE_PATH;
+ VendorDeviceNode.Header.SubType = HW_VENDOR_DP;
+ VendorDeviceNode.Guid = gQemuRamfbGuid;
+ SetDevicePathNodeLength (&VendorDeviceNode.Header, sizeof (VENDOR_DEVICE_PATH));
+
+ RamfbDevicePath = AppendDevicePathNode (
+ NULL,
+ (EFI_DEVICE_PATH_PROTOCOL *) &VendorDeviceNode);
+
+ Status = gBS->InstallMultipleProtocolInterfaces (
+ &RamfbHandle,
+ &gEfiDevicePathProtocolGuid, RamfbDevicePath,
+ NULL);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((EFI_D_INFO, "Ramfb: install Ramfb Vendor DevicePath failed\n"));
+ FreePool((VOID*)(UINTN)QemuRamfbMode.FrameBufferBase);
+ return Status;
+ }
+
+ /* gop devpath + protocol */
+ ZeroMem (&AcpiDeviceNode, sizeof (ACPI_ADR_DEVICE_PATH));
+ AcpiDeviceNode.Header.Type = ACPI_DEVICE_PATH;
+ AcpiDeviceNode.Header.SubType = ACPI_ADR_DP;
+ AcpiDeviceNode.ADR = ACPI_DISPLAY_ADR (1, 0, 0, 1, 0,
+ ACPI_ADR_DISPLAY_TYPE_EXTERNAL_DIGITAL,
+ 0, 0);
+ SetDevicePathNodeLength (&AcpiDeviceNode.Header, sizeof (ACPI_ADR_DEVICE_PATH));
+
+ GopDevicePath = AppendDevicePathNode (
+ RamfbDevicePath,
+ (EFI_DEVICE_PATH_PROTOCOL *) &AcpiDeviceNode);
+
+ Status = gBS->InstallMultipleProtocolInterfaces (
+ &GopHandle,
+ &gEfiDevicePathProtocolGuid, GopDevicePath,
+ &gEfiGraphicsOutputProtocolGuid, &QemuRamfbGraphicsOutput,
+ NULL);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((EFI_D_INFO, "Ramfb: install GOP DevicePath failed\n"));
+ FreePool((VOID*)(UINTN)QemuRamfbMode.FrameBufferBase);
+ return Status;
+ }
+
+ gBS->OpenProtocol (
+ RamfbHandle,
+ &gEfiDevicePathProtocolGuid,
+ &DevicePath,
+ gImageHandle,
+ GopHandle,
+ EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER);
+
+ return EFI_SUCCESS;
+}
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..75a7d86832
--- /dev/null
+++ b/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
@@ -0,0 +1,34 @@
+[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.common]
+ QemuRamfb.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+ BaseMemoryLib
+ DebugLib
+ DevicePathLib
+ FrameBufferBltLib
+ MemoryAllocationLib
+ UefiBootManagerLib
+ UefiBootServicesTableLib
+ UefiDriverEntryPoint
+ UefiLib
+ QemuFwCfgLib
+
+[Protocols]
+ gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START
+
+[Depex]
+ TRUE
--
2.9.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/4] OvmfPkg: add QemuRamfb to platform console
2018-06-08 11:39 [PATCH 0/4] Add QemuRamfbDxe driver Gerd Hoffmann
2018-06-08 11:39 ` [PATCH 1/4] OvmfPkg: add QEMU_RAMFB_GUID Gerd Hoffmann
2018-06-08 11:39 ` [PATCH 2/4] OvmfPkg: add QemuRamfbDxe Gerd Hoffmann
@ 2018-06-08 11:39 ` Gerd Hoffmann
2018-06-11 16:24 ` Laszlo Ersek
2018-06-08 11:39 ` [PATCH 4/4] ArmVirtPkg: add QemuRamfbDxe Gerd Hoffmann
2018-06-11 16:35 ` [PATCH 0/4] Add QemuRamfbDxe driver Laszlo Ersek
4 siblings, 1 reply; 18+ messages in thread
From: Gerd Hoffmann @ 2018-06-08 11:39 UTC (permalink / raw)
To: edk2-devel
Add QemuRamfbDxe device path to the list of platform console devices,
so ConSplitter will pick up the device even though it isn't a PCI GPU.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
.../Library/PlatformBootManagerLib/PlatformData.c | 44 ++++++++++++++++++++++
1 file changed, 44 insertions(+)
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
index a50cd7bcaf..6202516971 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
@@ -14,6 +14,7 @@
**/
#include "BdsPlatform.h"
+#include <Guid/QemuRamfb.h>
//
// Debug Agent UART Device Path structure
@@ -37,6 +38,17 @@ typedef struct {
} USB_KEYBOARD_DEVICE_PATH;
#pragma pack ()
+//
+// QemuRamfb Device Path structure
+//
+#pragma pack(1)
+typedef struct {
+ VENDOR_DEVICE_PATH Vendor;
+ ACPI_ADR_DEVICE_PATH AcpiAdr;
+ EFI_DEVICE_PATH_PROTOCOL End;
+} VENDOR_RAMFB_DEVICE_PATH;
+#pragma pack()
+
ACPI_HID_DEVICE_PATH gPnpPs2KeyboardDeviceNode = gPnpPs2Keyboard;
ACPI_HID_DEVICE_PATH gPnp16550ComPortDeviceNode = gPnp16550ComPort;
UART_DEVICE_PATH gUartDeviceNode = gUart;
@@ -100,6 +112,34 @@ STATIC USB_KEYBOARD_DEVICE_PATH gUsbKeyboardDevicePath = {
gEndEntire
};
+STATIC VENDOR_RAMFB_DEVICE_PATH gQemuRamfbDevicePath = {
+ {
+ {
+ HARDWARE_DEVICE_PATH,
+ HW_VENDOR_DP,
+ {
+ (UINT8) (sizeof (VENDOR_DEVICE_PATH)),
+ (UINT8) ((sizeof (VENDOR_DEVICE_PATH)) >> 8)
+ }
+ },
+ QEMU_RAMFB_GUID,
+ },
+ {
+ {
+ ACPI_DEVICE_PATH,
+ ACPI_ADR_DP,
+ {
+ (UINT8) (sizeof (ACPI_ADR_DEVICE_PATH)),
+ (UINT8) ((sizeof (ACPI_ADR_DEVICE_PATH)) >> 8)
+ }
+ },
+ ACPI_DISPLAY_ADR (1, 0, 0, 1, 0,
+ ACPI_ADR_DISPLAY_TYPE_EXTERNAL_DIGITAL,
+ 0, 0)
+ },
+ gEndEntire
+};
+
//
// Predefined platform default console device path
//
@@ -113,6 +153,10 @@ PLATFORM_CONSOLE_CONNECT_ENTRY gPlatformConsole[] = {
CONSOLE_IN
},
{
+ (EFI_DEVICE_PATH_PROTOCOL *)&gQemuRamfbDevicePath,
+ CONSOLE_OUT
+ },
+ {
NULL,
0
}
--
2.9.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 4/4] ArmVirtPkg: add QemuRamfbDxe
2018-06-08 11:39 [PATCH 0/4] Add QemuRamfbDxe driver Gerd Hoffmann
` (2 preceding siblings ...)
2018-06-08 11:39 ` [PATCH 3/4] OvmfPkg: add QemuRamfb to platform console Gerd Hoffmann
@ 2018-06-08 11:39 ` Gerd Hoffmann
2018-06-11 16:29 ` Laszlo Ersek
2018-06-11 16:35 ` [PATCH 0/4] Add QemuRamfbDxe driver Laszlo Ersek
4 siblings, 1 reply; 18+ messages in thread
From: Gerd Hoffmann @ 2018-06-08 11:39 UTC (permalink / raw)
To: edk2-devel
Build wireup for ArmVirt.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
ArmVirtPkg/ArmVirtQemu.dsc | 2 ++
ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 ++
3 files changed, 5 insertions(+)
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index d74feb709c..744d127a10 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -57,6 +57,7 @@
BootLogoLib|MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf
PlatformBootManagerLib|ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
+ FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
QemuBootOrderLib|OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
PciPcdProducerLib|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
@@ -391,6 +392,7 @@
#
# Video support
#
+ OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
OvmfPkg/PlatformDxe/Platform.inf
diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
index 89f95b2d99..63a202c788 100644
--- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
+++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
@@ -175,6 +175,7 @@ READ_LOCK_STATUS = TRUE
#
# Video support
#
+ INF OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
INF OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
INF OvmfPkg/PlatformDxe/Platform.inf
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index 1e823aeab7..e59f53b728 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -57,6 +57,7 @@
BootLogoLib|MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf
PlatformBootManagerLib|ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
+ FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
QemuBootOrderLib|OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
PciPcdProducerLib|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
@@ -380,6 +381,7 @@
#
# Video support
#
+ OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
OvmfPkg/PlatformDxe/Platform.inf
--
2.9.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] OvmfPkg: add QemuRamfbDxe
2018-06-08 11:39 ` [PATCH 2/4] OvmfPkg: add QemuRamfbDxe Gerd Hoffmann
@ 2018-06-10 5:57 ` Ard Biesheuvel
2018-06-11 15:56 ` Laszlo Ersek
1 sibling, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2018-06-10 5:57 UTC (permalink / raw)
To: Gerd Hoffmann, Laszlo Ersek; +Cc: edk2-devel@lists.01.org
Hello Gerd,
Thanks a lot for contributing this code. I am quite pleased that we
finally have a solution for the EFI frame buffer for ARM systems
running under KVM.
One comment below.
On 8 June 2018 at 13:39, Gerd Hoffmann <kraxel@redhat.com> wrote:
> Add a driver for the qemu ramfb display device.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> OvmfPkg/QemuRamfbDxe/QemuRamfb.c | 308 ++++++++++++++++++++++++++++++++++
> 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 | 34 ++++
> 8 files changed, 348 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..f04a314c24
> --- /dev/null
> +++ b/OvmfPkg/QemuRamfbDxe/QemuRamfb.c
> @@ -0,0 +1,308 @@
> +#include <Uefi.h>
> +#include <Protocol/GraphicsOutput.h>
> +
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DevicePathLib.h>
> +#include <Library/FrameBufferBltLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/UefiBootManagerLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiDriverEntryPoint.h>
> +#include <Library/UefiLib.h>
> +#include <Library/QemuFwCfgLib.h>
> +
> +#include <Guid/QemuRamfb.h>
> +
> +#define RAMFB_FORMAT 0x34325258 /* DRM_FORMAT_XRGB8888 */
> +#define RAMFB_BPP 4
> +
> +EFI_GUID gQemuRamfbGuid = QEMU_RAMFB_GUID;
> +
> +typedef struct RAMFB_CONFIG {
> + UINT64 Address;
> + UINT32 FourCC;
> + UINT32 Flags;
> + UINT32 Width;
> + UINT32 Height;
> + UINT32 Stride;
> +} RAMFB_CONFIG;
> +
> +EFI_HANDLE RamfbHandle;
> +EFI_HANDLE GopHandle;
> +FRAME_BUFFER_CONFIGURE *QemuRamfbFrameBufferBltConfigure;
> +UINTN QemuRamfbFrameBufferBltConfigureSize;
> +
> +EFI_GRAPHICS_OUTPUT_MODE_INFORMATION QemuRamfbModeInfo[] = {
> + {
> + .HorizontalResolution = 640,
> + .VerticalResolution = 480,
> + },{
> + .HorizontalResolution = 800,
> + .VerticalResolution = 600,
> + },{
> + .HorizontalResolution = 1024,
> + .VerticalResolution = 768,
> + }
> +};
> +#define QemuRamfbModeCount (sizeof(QemuRamfbModeInfo)/sizeof(QemuRamfbModeInfo[0]))
> +
> +EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE QemuRamfbMode = {
> + .MaxMode = QemuRamfbModeCount,
> + .Mode = 0,
> + .Info = QemuRamfbModeInfo,
> + .SizeOfInfo = sizeof(EFI_GRAPHICS_OUTPUT_MODE_INFORMATION),
> +};
> +
> +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 > QemuRamfbMode.MaxMode) {
> + return EFI_INVALID_PARAMETER;
> + }
> + ModeInfo = &QemuRamfbModeInfo[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;
> +}
> +
> +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 Ret;
> + FIRMWARE_CONFIG_ITEM Item;
> + UINTN Size;
> +
> + if (ModeNumber > QemuRamfbMode.MaxMode) {
> + return EFI_UNSUPPORTED;
> + }
> + ModeInfo = &QemuRamfbModeInfo[ModeNumber];
> +
> + DEBUG ((EFI_D_INFO, "Ramfb: SetMode %d (%dx%d)\n", ModeNumber,
> + ModeInfo->HorizontalResolution,
> + ModeInfo->VerticalResolution));
> +
> + QemuRamfbMode.Mode = ModeNumber;
> + QemuRamfbMode.Info = ModeInfo;
> +
> + Config.Address = SwapBytes64( QemuRamfbMode.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 );
> +
> + QemuFwCfgFindFile("etc/ramfb", &Item, &Size);
> + QemuFwCfgSelectItem(Item);
> + QemuFwCfgWriteBytes(sizeof(Config), &Config);
> +
> + Ret = FrameBufferBltConfigure (
> + (VOID*)(UINTN)QemuRamfbMode.FrameBufferBase,
> + ModeInfo,
> + QemuRamfbFrameBufferBltConfigure,
> + &QemuRamfbFrameBufferBltConfigureSize);
> +
> + if (Ret == RETURN_BUFFER_TOO_SMALL) {
> + if (QemuRamfbFrameBufferBltConfigure != NULL) {
> + FreePool(QemuRamfbFrameBufferBltConfigure);
> + }
> + QemuRamfbFrameBufferBltConfigure =
> + AllocatePool(QemuRamfbFrameBufferBltConfigureSize);
> +
> + Ret = FrameBufferBltConfigure (
> + (VOID*)(UINTN)QemuRamfbMode.FrameBufferBase,
> + ModeInfo,
> + QemuRamfbFrameBufferBltConfigure,
> + &QemuRamfbFrameBufferBltConfigureSize);
> + }
> +
> + /* clear screen */
> + ZeroMem (&Black, sizeof (Black));
> + Ret = FrameBufferBlt (
> + QemuRamfbFrameBufferBltConfigure,
> + &Black,
> + EfiBltVideoFill,
> + 0, 0,
> + 0, 0,
> + ModeInfo->HorizontalResolution,
> + ModeInfo->VerticalResolution,
> + 0
> + );
> +
> + return EFI_SUCCESS;
> +}
> +
> +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 (
> + QemuRamfbFrameBufferBltConfigure,
> + BltBuffer,
> + BltOperation,
> + SourceX,
> + SourceY,
> + DestinationX,
> + DestinationY,
> + Width,
> + Height,
> + Delta);
> +}
> +
> +EFI_GRAPHICS_OUTPUT_PROTOCOL QemuRamfbGraphicsOutput = {
> + .QueryMode = QemuRamfbGraphicsOutputQueryMode,
> + .SetMode = QemuRamfbGraphicsOutputSetMode,
> + .Blt = QemuRamfbGraphicsOutputBlt,
> + .Mode = &QemuRamfbMode,
> +};
> +
> +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;
> + RETURN_STATUS Ret;
> + FIRMWARE_CONFIG_ITEM Item;
> + EFI_PHYSICAL_ADDRESS FbBase;
> + UINTN Size, FbSize, MaxFbSize, Pages, Index;
> +
> + DEBUG ((EFI_D_INFO, "InitializeQemuRamfb\n"));
> +
> + if (!QemuFwCfgIsAvailable()) {
> + DEBUG ((EFI_D_INFO, "Ramfb: no FwCfg\n"));
> + return EFI_NOT_FOUND;
> + }
> +
> + Ret = QemuFwCfgFindFile("etc/ramfb", &Item, &Size);
> + if (Ret != RETURN_SUCCESS) {
> + DEBUG ((EFI_D_INFO, "Ramfb: no etc/ramfb in FwCfg\n"));
> + return EFI_NOT_FOUND;
> + }
> +
> + MaxFbSize = 0;
> + for (Index = 0; Index < QemuRamfbModeCount; Index++) {
> + QemuRamfbModeInfo[Index].PixelsPerScanLine =
> + QemuRamfbModeInfo[Index].HorizontalResolution;
> + QemuRamfbModeInfo[Index].PixelFormat =
> + PixelBlueGreenRedReserved8BitPerColor,
> + FbSize = RAMFB_BPP *
> + QemuRamfbModeInfo[Index].HorizontalResolution *
> + QemuRamfbModeInfo[Index].VerticalResolution;
> + if (MaxFbSize < FbSize)
> + MaxFbSize = FbSize;
> + DEBUG ((EFI_D_INFO, "Ramfb: Mode %d: %dx%d, %d kB\n", Index,
> + QemuRamfbModeInfo[Index].HorizontalResolution,
> + QemuRamfbModeInfo[Index].VerticalResolution,
> + FbSize / 1024));
> + }
> +
> + Pages = EFI_SIZE_TO_PAGES(MaxFbSize);
> + MaxFbSize = EFI_PAGES_TO_SIZE(Pages);
> + FbBase = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateRuntimePages(Pages);
EfiRuntimeServicesMemory is special in the sense that it gets added to
the page tables that are installed while UEFI runtime services are
being invoked by the OS. Given that the runtime firmware does not care
about this frame buffer, this is unnecessary, and so I'd rather avoid
it. The proper fix /could/ be to use EfiRuntimeServicesMemory with the
EFI_MEMORY_RUNTIME attribute cleared, but sadly, edk2 does not
implement that at all. So instead, I recommend using
EfiReservedMemoryType here.
(The spec may argue that you should never use it, but it also
recommends that, e.g., SMBIOS tables are put in
EfiRuntimeServicesMemory with the EFI_MEMORY_RUNTIME attribute
cleared, so it is obviously not in sync with reality)
Thanks,
Ard.
> + if (!FbBase) {
> + DEBUG ((EFI_D_INFO, "Ramfb: memory allocation failed\n"));
> + return EFI_OUT_OF_RESOURCES;
> + }
> + DEBUG ((EFI_D_INFO, "Ramfb: Framebuffer at 0x%lx, %d kB, %d pages\n",
> + FbBase, MaxFbSize / 1024, Pages));
> + QemuRamfbMode.FrameBufferSize = MaxFbSize;
> + QemuRamfbMode.FrameBufferBase = FbBase;
> +
> + /* 800 x 600 */
> + QemuRamfbGraphicsOutputSetMode (&QemuRamfbGraphicsOutput, 1);
> +
> + /* ramfb vendor devpath */
> + ZeroMem (&VendorDeviceNode, sizeof (VENDOR_DEVICE_PATH));
> + VendorDeviceNode.Header.Type = HARDWARE_DEVICE_PATH;
> + VendorDeviceNode.Header.SubType = HW_VENDOR_DP;
> + VendorDeviceNode.Guid = gQemuRamfbGuid;
> + SetDevicePathNodeLength (&VendorDeviceNode.Header, sizeof (VENDOR_DEVICE_PATH));
> +
> + RamfbDevicePath = AppendDevicePathNode (
> + NULL,
> + (EFI_DEVICE_PATH_PROTOCOL *) &VendorDeviceNode);
> +
> + Status = gBS->InstallMultipleProtocolInterfaces (
> + &RamfbHandle,
> + &gEfiDevicePathProtocolGuid, RamfbDevicePath,
> + NULL);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((EFI_D_INFO, "Ramfb: install Ramfb Vendor DevicePath failed\n"));
> + FreePool((VOID*)(UINTN)QemuRamfbMode.FrameBufferBase);
> + return Status;
> + }
> +
> + /* gop devpath + protocol */
> + ZeroMem (&AcpiDeviceNode, sizeof (ACPI_ADR_DEVICE_PATH));
> + AcpiDeviceNode.Header.Type = ACPI_DEVICE_PATH;
> + AcpiDeviceNode.Header.SubType = ACPI_ADR_DP;
> + AcpiDeviceNode.ADR = ACPI_DISPLAY_ADR (1, 0, 0, 1, 0,
> + ACPI_ADR_DISPLAY_TYPE_EXTERNAL_DIGITAL,
> + 0, 0);
> + SetDevicePathNodeLength (&AcpiDeviceNode.Header, sizeof (ACPI_ADR_DEVICE_PATH));
> +
> + GopDevicePath = AppendDevicePathNode (
> + RamfbDevicePath,
> + (EFI_DEVICE_PATH_PROTOCOL *) &AcpiDeviceNode);
> +
> + Status = gBS->InstallMultipleProtocolInterfaces (
> + &GopHandle,
> + &gEfiDevicePathProtocolGuid, GopDevicePath,
> + &gEfiGraphicsOutputProtocolGuid, &QemuRamfbGraphicsOutput,
> + NULL);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((EFI_D_INFO, "Ramfb: install GOP DevicePath failed\n"));
> + FreePool((VOID*)(UINTN)QemuRamfbMode.FrameBufferBase);
> + return Status;
> + }
> +
> + gBS->OpenProtocol (
> + RamfbHandle,
> + &gEfiDevicePathProtocolGuid,
> + &DevicePath,
> + gImageHandle,
> + GopHandle,
> + EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER);
> +
> + return EFI_SUCCESS;
> +}
> 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..75a7d86832
> --- /dev/null
> +++ b/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
> @@ -0,0 +1,34 @@
> +[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.common]
> + QemuRamfb.c
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> + BaseMemoryLib
> + DebugLib
> + DevicePathLib
> + FrameBufferBltLib
> + MemoryAllocationLib
> + UefiBootManagerLib
> + UefiBootServicesTableLib
> + UefiDriverEntryPoint
> + UefiLib
> + QemuFwCfgLib
> +
> +[Protocols]
> + gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START
> +
> +[Depex]
> + TRUE
> --
> 2.9.3
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] OvmfPkg: add QEMU_RAMFB_GUID
2018-06-08 11:39 ` [PATCH 1/4] OvmfPkg: add QEMU_RAMFB_GUID Gerd Hoffmann
@ 2018-06-11 13:06 ` Laszlo Ersek
0 siblings, 0 replies; 18+ messages in thread
From: Laszlo Ersek @ 2018-06-11 13:06 UTC (permalink / raw)
To: Gerd Hoffmann, edk2-devel
Hi Gerd,
On 06/08/18 13:39, Gerd Hoffmann wrote:
> Add GUID header file for the QemuRamfbDxe driver.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> OvmfPkg/Include/Guid/QemuRamfb.h | 25 +++++++++++++++++++++++++
> OvmfPkg/OvmfPkg.dec | 1 +
> 2 files changed, 26 insertions(+)
> create mode 100644 OvmfPkg/Include/Guid/QemuRamfb.h
>
> diff --git a/OvmfPkg/Include/Guid/QemuRamfb.h b/OvmfPkg/Include/Guid/QemuRamfb.h
> new file mode 100644
> index 0000000000..878224599d
> --- /dev/null
> +++ b/OvmfPkg/Include/Guid/QemuRamfb.h
> @@ -0,0 +1,25 @@
> +/** @file
> + Recommended GUID to be used in the Vendor Hardware device path nodes that
> + identify qemu ramfb devices.
> +
> + 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 that accompanies this
> + distribution. The full text of the license may be found at
> + 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.
> +
> +**/
> +
> +#ifndef __QEMU_RAMFB_H__
> +#define __QEMU_RAMFB_H__
> +
> +#define QEMU_RAMFB_GUID \
> +{0x837dca9e, 0xe874, 0x4d82, {0xb2, 0x9a, 0x23, 0xfe, 0x0e, 0x23, 0xd1, 0xe2}}
> +
> +extern EFI_GUID gQemuRamfbGuid;
> +
> +#endif
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index dc5597db41..a16c22a289 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -75,6 +75,7 @@
> gEfiXenInfoGuid = {0xd3b46f3b, 0xd441, 0x1244, {0x9a, 0x12, 0x0, 0x12, 0x27, 0x3f, 0xc1, 0x4d}}
> gOvmfPlatformConfigGuid = {0x7235c51c, 0x0c80, 0x4cab, {0x87, 0xac, 0x3b, 0x08, 0x4a, 0x63, 0x04, 0xb1}}
> gVirtioMmioTransportGuid = {0x837dca9e, 0xe874, 0x4d82, {0xb2, 0x9a, 0x23, 0xfe, 0x0e, 0x23, 0xd1, 0xe2}}
> + gQemuRamfbGuid = {0x837dca9e, 0xe874, 0x4d82, {0xb2, 0x9a, 0x23, 0xfe, 0x0e, 0x23, 0xd1, 0xe2}}
> gXenBusRootDeviceGuid = {0xa732241f, 0x383d, 0x4d9c, {0x8a, 0xe1, 0x8e, 0x09, 0x83, 0x75, 0x89, 0xd7}}
> gRootBridgesConnectedEventGroupGuid = {0x24a2d66f, 0xeedd, 0x4086, {0x90, 0x42, 0xf2, 0x6e, 0x47, 0x97, 0xee, 0x69}}
>
>
this patch is all good, formally speaking, but you should please
generate a new GUID with "uuidgen", rather than using the one from
"VirtioMmioTransport.h" :)
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] OvmfPkg: add QemuRamfbDxe
2018-06-08 11:39 ` [PATCH 2/4] OvmfPkg: add QemuRamfbDxe Gerd Hoffmann
2018-06-10 5:57 ` Ard Biesheuvel
@ 2018-06-11 15:56 ` Laszlo Ersek
2018-06-12 9:15 ` Gerd Hoffmann
1 sibling, 1 reply; 18+ messages in thread
From: Laszlo Ersek @ 2018-06-11 15:56 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: edk2-devel
On 06/08/18 13:39, Gerd Hoffmann wrote:
> diff --git a/OvmfPkg/QemuRamfbDxe/QemuRamfb.c b/OvmfPkg/QemuRamfbDxe/QemuRamfb.c
> new file mode 100644
> index 0000000000..f04a314c24
> --- /dev/null
> +++ b/OvmfPkg/QemuRamfbDxe/QemuRamfb.c
> @@ -0,0 +1,308 @@
> +#include <Uefi.h>
(1) I think we should be able to drop this. See the leading comment in
"MdePkg/Include/Uefi.h" -- and this is not a UEFI_DRIVER but a platform
DXE_DRIVER module.
> +#include <Protocol/GraphicsOutput.h>
> +
> +#include <Library/BaseMemoryLib.h>
OK, ZeroMem() is from this one.
> +#include <Library/DebugLib.h>
OK, DEBUG() needs this.
> +#include <Library/DevicePathLib.h>
OK, AppendDevicePathNode().
> +#include <Library/FrameBufferBltLib.h>
OK, FrameBufferBltConfigure().
> +#include <Library/MemoryAllocationLib.h>
OK, AllocateCopyPool().
> +#include <Library/UefiBootManagerLib.h>
(2) I think we can drop this. (Please also remove it from the
[LibraryClasses] section in the INF file.)
> +#include <Library/UefiBootServicesTableLib.h>
OK, gBS->InstallMultipleProtocolInterfaces().
> +#include <Library/UefiDriverEntryPoint.h>
(3) Well, strictly speaking, this isn't necessary as an #include -- the
corresponding lib class under [LibraryClasses] *is* necessary though.
I've grepped the tree for this #include directive, and usage is
inconsistent. In new drivers we introduce, I prefer that we not add the
include. Can you please drop it? (Again, do keep it in the INF file.)
> +#include <Library/UefiLib.h>
(4) I tried to see if we use anything from UefiLib.h, and came up empty.
Can you please attempt to drop it both here and from [LibraryClasses] in
the INF file?
> +#include <Library/QemuFwCfgLib.h>
Yup, needed.
> +
> +#include <Guid/QemuRamfb.h>
> +
> +#define RAMFB_FORMAT 0x34325258 /* DRM_FORMAT_XRGB8888 */
> +#define RAMFB_BPP 4
> +
> +EFI_GUID gQemuRamfbGuid = QEMU_RAMFB_GUID;
(5) Please drop this -- we have the declaration of gQemuRamfbGuid from
<Guid/QemuRamfb.h>, and the definition comes from source code
auto-generated by the build tools.
(I'll comment more on the usage below.)
> +
> +typedef struct RAMFB_CONFIG {
> + UINT64 Address;
> + UINT32 FourCC;
> + UINT32 Flags;
> + UINT32 Width;
> + UINT32 Height;
> + UINT32 Stride;
> +} RAMFB_CONFIG;
(6) Please wrap this in:
#pragma pack (1)
...
#pragma pack ()
(7) We could also add this type definition in a new file
"OvmfPkg/Include/IndustryStandard/QemuRamfbConfig.h". However, I do
realize that's somewhat overkill. Up to you; I don't mind if we keep it
here.
> +
> +EFI_HANDLE RamfbHandle;
> +EFI_HANDLE GopHandle;
> +FRAME_BUFFER_CONFIGURE *QemuRamfbFrameBufferBltConfigure;
> +UINTN QemuRamfbFrameBufferBltConfigureSize;
(8) Please make all these variables STATIC, and also prepend an "m"
prefix to their names (it stands for "module"):
STATIC EFI_HANDLE mRamfbHandle;
> +
> +EFI_GRAPHICS_OUTPUT_MODE_INFORMATION QemuRamfbModeInfo[] = {
(9) Same comment as (8).
> + {
> + .HorizontalResolution = 640,
> + .VerticalResolution = 480,
> + },{
> + .HorizontalResolution = 800,
> + .VerticalResolution = 600,
> + },{
> + .HorizontalResolution = 1024,
> + .VerticalResolution = 768,
> + }
> +};
(10) In edk2 we cannot use designated initializers. I suggest (for
example) assigning these values in the entry point function.
Alternatively, please use the C90-style syntax for aggregate
initialization (i.e. just insert enough zeros):
STATIC EFI_GRAPHICS_OUTPUT_MODE_INFORMATION QemuRamfbModeInfo[] = {
{
0, // Version
640, // HorizontalResolution
400, // VerticalResolution
0, // PixelFormat -- filled in later
0, // PixelInformation -- will be ignored
0 // PixelsPerScanLine -- filled in later
},
...
};
> +#define QemuRamfbModeCount (sizeof(QemuRamfbModeInfo)/sizeof(QemuRamfbModeInfo[0]))
(11) Please use ARRAY_SIZE() instead, wherever necessary.
> +
> +EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE QemuRamfbMode = {
> + .MaxMode = QemuRamfbModeCount,
> + .Mode = 0,
> + .Info = QemuRamfbModeInfo,
> + .SizeOfInfo = sizeof(EFI_GRAPHICS_OUTPUT_MODE_INFORMATION),
> +};
(12) See (10).
(13) Please insert a space between "sizeof" and "(". In edk2, we put one
space between function identifier / function-like macro name, and the
opening paren, in function calls / macro invocations.
(Obviously, I know that "sizeof" is neither a function nor a macro; it
is an operator. In fact my preferred style is just "sizeof object", no
parens. However, if we use the parens, which is indeed required for
typenames, then we should use one space.)
> +
> +EFI_STATUS
> +EFIAPI
> +QemuRamfbGraphicsOutputQueryMode (
(14) Please also make all functions STATIC, except InitializeQemuRamfb().
> + 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 > QemuRamfbMode.MaxMode) {
(15) I think the ModeNumber comparison is off-by-one; equality should be
rejected too.
> + return EFI_INVALID_PARAMETER;
> + }
> + ModeInfo = &QemuRamfbModeInfo[ModeNumber];
> +
> + *Info = AllocateCopyPool (sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION),
> + ModeInfo);
(16) The indentation is incorrect; "ModeInfo" should start 2 colums to
the right of the "A" in AllocateCopyPool().
In general, in edk2 there are two accepted indentation styles for
function calls that extend to multiple lines:
variant #1: all arguments (including the first one) on separate lines,
with the closing paren also on a separate line:
Status = StructPointer->Function (
Arg1,
Arg2,
Arg3
);
variant #2: filling horizontal space with arguments up to column 80; and
whenever a line break is needed, stick with the same indentation as seen
in variant #1. The closing paren is not broken to a separate line.
Status = StructPointer->Function (LongArgumentOne, LongArgumentTwo,
LongArgumentThree, LongArgumentFour);
For both styles, if StructPointer doesn't exist, then the indentation
remains the same, relatively speaking, anchored to "Function".
> + if (*Info == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> + *SizeOfInfo = sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
> +
> + return EFI_SUCCESS;
> +}
> +
> +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 Ret;
(17) We generally call such variables "Status".
> + FIRMWARE_CONFIG_ITEM Item;
> + UINTN Size;
> +
> + if (ModeNumber > QemuRamfbMode.MaxMode) {
> + return EFI_UNSUPPORTED;
> + }
(18) See (15).
> + ModeInfo = &QemuRamfbModeInfo[ModeNumber];
> +
> + DEBUG ((EFI_D_INFO, "Ramfb: SetMode %d (%dx%d)\n", ModeNumber,
(19) We've stopped adding EFI_D_* macros; instead we use DEBUG_* macros
in new code.
(20) Please print UINT32 values with "%u" rather than "%d".
> + ModeInfo->HorizontalResolution,
> + ModeInfo->VerticalResolution));
(21) Please see (16).
> +
> + QemuRamfbMode.Mode = ModeNumber;
> + QemuRamfbMode.Info = ModeInfo;
> +
> + Config.Address = SwapBytes64( QemuRamfbMode.FrameBufferBase );
(22) Please #include <BaseLib.h> for SwapBytes*(); also please add it to
[LibraryClasses] in the INF file.
(23) The space characters near the parens are not idiomatic; we'd use
Config.Address = SwapBytes64 (QemuRamfbMode.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 );
> +
> + QemuFwCfgFindFile("etc/ramfb", &Item, &Size);
(24) You perform the same lookup in the entry point function; can you
please cache the selector value ("Item") in a new
STATIC FIRMWARE_CONFIG_ITEM mRamFbFwCfgItem;
global variable? And then use that here.
Furthermore, instead of Size, you can use "sizeof Config".
> + QemuFwCfgSelectItem(Item);
> + QemuFwCfgWriteBytes(sizeof(Config), &Config);
(25) (several counts:) please see (13).
> +
> + Ret = FrameBufferBltConfigure (
> + (VOID*)(UINTN)QemuRamfbMode.FrameBufferBase,
> + ModeInfo,
> + QemuRamfbFrameBufferBltConfigure,
> + &QemuRamfbFrameBufferBltConfigureSize);
(26) Please see (16).
> +
> + if (Ret == RETURN_BUFFER_TOO_SMALL) {
> + if (QemuRamfbFrameBufferBltConfigure != NULL) {
> + FreePool(QemuRamfbFrameBufferBltConfigure);
(27) Please see (13).
> + }
> + QemuRamfbFrameBufferBltConfigure =
> + AllocatePool(QemuRamfbFrameBufferBltConfigureSize);
(28) Please see (13).
(29) Please reorganize the function as follows: this AllocatePool() call
should be moved near the top, so that, if it fails, we can return
EFI_OUT_OF_RESOURCES without actually changing fw_cfg or any of the
GOP-related data structures. Then, please check the return value for NULL.
My understanding is that QemuRamfbMode.FrameBufferBase is invariant (=
independent of the mode requested), after the initial setup, so this
should be possible.
> +
> + Ret = FrameBufferBltConfigure (
> + (VOID*)(UINTN)QemuRamfbMode.FrameBufferBase,
> + ModeInfo,
> + QemuRamfbFrameBufferBltConfigure,
> + &QemuRamfbFrameBufferBltConfigureSize);
> + }
(30) Please see (16).
(31) At this point, "Ret" (well, "Status") must either be RETURN_SUCCESS
or RETURN_UNSUPPORTED. Please add:
if (RETURN_ERROR (Status)) {
ASSERT (Status == RETURN_UNSUPPORTED);
return Status;
}
> +
> + /* clear screen */
(32) The edk2 style for such comments is:
//
// clear screen
//
> + ZeroMem (&Black, sizeof (Black));
> + Ret = FrameBufferBlt (
> + QemuRamfbFrameBufferBltConfigure,
> + &Black,
> + EfiBltVideoFill,
> + 0, 0,
> + 0, 0,
> + ModeInfo->HorizontalResolution,
> + ModeInfo->VerticalResolution,
> + 0
> + );
(33) We've got a load of arguments here, can we please do:
Status = FrameBufferBlt (
QemuRamfbFrameBufferBltConfigure,
&Black,
EfiBltVideoFill,
0, // SourceX -- ignored
0, // SourceY -- ignored
0, // DestinationX
0, // DestinationY
ModeInfo->HorizontalResolution, // Width
ModeInfo->VerticalResolution, // Height
0 // Delta -- ignored
);
(34) I agree that this call should never fail, and even if it does, we
should still return EFI_SUCCESS about the mode change. But, it might be
nice to log a debug message on failure, such as:
DEBUG ((DEBUG_WARN, "%a: clearing the screen failed: %r\n",
__FUNCTION__, Status));
> +
> + return EFI_SUCCESS;
> +}
> +
> +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 (
> + QemuRamfbFrameBufferBltConfigure,
> + BltBuffer,
> + BltOperation,
> + SourceX,
> + SourceY,
> + DestinationX,
> + DestinationY,
> + Width,
> + Height,
> + Delta);
> +}
(35) Please see (16).
> +
> +EFI_GRAPHICS_OUTPUT_PROTOCOL QemuRamfbGraphicsOutput = {
> + .QueryMode = QemuRamfbGraphicsOutputQueryMode,
> + .SetMode = QemuRamfbGraphicsOutputSetMode,
> + .Blt = QemuRamfbGraphicsOutputBlt,
> + .Mode = &QemuRamfbMode,
> +};
(36) Please see (10).
> +
> +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;
> + RETURN_STATUS Ret;
Another background story... EFI_STATUS is for (a) anything defined in
the Platform Init and UEFI specifications, (b) library classes that are
meant to be used *solely* in modules defined by those specifications.
RETURN_STATUS is for libraries that are lower-level than those, at least
conceptually; in other words, if they provide functionality that is
independent of client module type ("BASE" libraries). In theory such
libraries are OK to use in SEC modules even.
Technically, there isn't any difference between these types and their
values, however. Thus, if you have a function in which you already have
an EFI_STATUS local variable (for storing EFI_STATUS values), you can
freely store RETURN_STATUS retvals to it as well. If you only assign
RETURN_STATUS values, then it's more idiomatic to use RETURN_STATUS.
(Which is what you do in QemuRamfbGraphicsOutputSetMode(), correctly.)
For RETURN_STATUS variables, we have macros such as:
- RETURN_ERROR (Status)
- ASSERT_RETURN_ERROR (Status)
For EFI_STATUS variables, we have the same, just replace RETURN with EFI.
(37) So, I suggest dropping "Ret", and just using Status.
> + FIRMWARE_CONFIG_ITEM Item;
(38) See (24).
> + EFI_PHYSICAL_ADDRESS FbBase;
> + UINTN Size, FbSize, MaxFbSize, Pages, Index;
(39) I know it's annoying, but please break at least *some* of those to
separate declarations. I guess keeping "FbSize" and "MaxFbSize" on the
same line makes sense.
Furthermore, "Size" should be called "FwCfgSize" (for clarity).
> +
> + DEBUG ((EFI_D_INFO, "InitializeQemuRamfb\n"));
(40) Please see (19). (There are some more occurrences below.)
(41) Also, I suggest using "%a" with __FUNCTION__.
> +
> + if (!QemuFwCfgIsAvailable()) {
> + DEBUG ((EFI_D_INFO, "Ramfb: no FwCfg\n"));
> + return EFI_NOT_FOUND;
> + }
> +
> + Ret = QemuFwCfgFindFile("etc/ramfb", &Item, &Size);
> + if (Ret != RETURN_SUCCESS) {
> + DEBUG ((EFI_D_INFO, "Ramfb: no etc/ramfb in FwCfg\n"));
> + return EFI_NOT_FOUND;
> + }
(42) For whatever reason, the idiomatic spelling for the comparison is
if (EFI_ERROR (Status)) {
...
}
(43) You might want to drop the debug message for this case, as in most
setups, I expect ramfb will not be present; plus, if the driver exits
with an error code, the DXE core will log that anyway. If you'd really
like to keep the message, I suggest DEBUG_VERBOSE.
(44) If the lookup succeeds, please compare "FwCfgSize" against sizeof
(RAMFB_CONFIG). In case of mismatch, log an error (DEBUG_ERROR) and exit.
> +
> + MaxFbSize = 0;
> + for (Index = 0; Index < QemuRamfbModeCount; Index++) {
> + QemuRamfbModeInfo[Index].PixelsPerScanLine =
> + QemuRamfbModeInfo[Index].HorizontalResolution;
> + QemuRamfbModeInfo[Index].PixelFormat =
> + PixelBlueGreenRedReserved8BitPerColor,
(45) Nice use of the comma operator :) but I think it may not be
intended. Please let's stick with the semicolon.
> + FbSize = RAMFB_BPP *
> + QemuRamfbModeInfo[Index].HorizontalResolution *
> + QemuRamfbModeInfo[Index].VerticalResolution;
> + if (MaxFbSize < FbSize)
> + MaxFbSize = FbSize;
(46) The edk2 coding style requires braces.
> + DEBUG ((EFI_D_INFO, "Ramfb: Mode %d: %dx%d, %d kB\n", Index,
> + QemuRamfbModeInfo[Index].HorizontalResolution,
> + QemuRamfbModeInfo[Index].VerticalResolution,
> + FbSize / 1024));
> + }
(47) indentation, please see (16).
(48) For printing UINTN values portably across 32-bit and 64-bit builds,
the safest way is to cast the variable to UINT64, and then print it with
"%Lu" (or, if you like hex, "%Lx").
This refers to both "Index" and "FbSize".
(Also, feel free to use lower-case "l" in place of my suggested
upper-case "L"; in edk2 they are interchangeable for printing integers.
I just dislike lower-case "l" because in ISO C, it stands for "long",
whereas "L" is not defined in ISO C for integers. I think that makes "L"
a better fit for a non-standard purpose such as "print UINT64". And, no,
edk2 does not have PRIx64.)
(49) EFI_D_*, please see (19)
(50) Please see (20) as well, for printing the resolutions.
> +
> + Pages = EFI_SIZE_TO_PAGES(MaxFbSize);
> + MaxFbSize = EFI_PAGES_TO_SIZE(Pages);
> + FbBase = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateRuntimePages(Pages);
(51) Multiple instances of (13).
(52) What Ard said about runtime allocations.
Please call AllocateReservedPages() instead.
> + if (!FbBase) {
(53) In edk2, we only use the logical negation operator (!) for
BOOLEANs. Please compare FbBase against 0 explicitly.
> + DEBUG ((EFI_D_INFO, "Ramfb: memory allocation failed\n"));
> + return EFI_OUT_OF_RESOURCES;
> + }
(54) please see (19).
> + DEBUG ((EFI_D_INFO, "Ramfb: Framebuffer at 0x%lx, %d kB, %d pages\n",
> + FbBase, MaxFbSize / 1024, Pages));
(55) Please see (48) for "MaxFbSize / 1024" and "Pages".
> + QemuRamfbMode.FrameBufferSize = MaxFbSize;
> + QemuRamfbMode.FrameBufferBase = FbBase;
> +
> + /* 800 x 600 */
> + QemuRamfbGraphicsOutputSetMode (&QemuRamfbGraphicsOutput, 1);
> +
> + /* ramfb vendor devpath */
(56) Please refer to (32); two instances above.
> + ZeroMem (&VendorDeviceNode, sizeof (VENDOR_DEVICE_PATH));
> + VendorDeviceNode.Header.Type = HARDWARE_DEVICE_PATH;
> + VendorDeviceNode.Header.SubType = HW_VENDOR_DP;
> + VendorDeviceNode.Guid = gQemuRamfbGuid;
(57) Unfortunately, structure assignment is forbidden in edk2; please
use the CopyGuid() function from BaseMemoryLib. See more below.
> + SetDevicePathNodeLength (&VendorDeviceNode.Header, sizeof (VENDOR_DEVICE_PATH));
(58) This line seems overlong (82 characters); please stick with 80 chars.
(59) I think you've filled in all members of VendorDeviceNode; can you
drop the ZeroMem()?
> +
> + RamfbDevicePath = AppendDevicePathNode (
> + NULL,
> + (EFI_DEVICE_PATH_PROTOCOL *) &VendorDeviceNode);
> +
(60) Please see (16).
(61) Please check the return value. AppendDevicePathNode() allocates
memory dynamically. If it fails, we should roll back earlier operations
(if any) and exit with EFI_OUT_OF_RESOURCES.
> + Status = gBS->InstallMultipleProtocolInterfaces (
> + &RamfbHandle,
> + &gEfiDevicePathProtocolGuid, RamfbDevicePath,
> + NULL);
(62) Please see (16).
> + if (EFI_ERROR (Status)) {
> + DEBUG ((EFI_D_INFO, "Ramfb: install Ramfb Vendor DevicePath failed\n"));
(63) We should use DEBUG_ERROR here.
(64) In such cases it is helpful to print Status too; please use the
"%r" format specifier for that.
> + FreePool((VOID*)(UINTN)QemuRamfbMode.FrameBufferBase);
(65) Please see (13).
> + return Status;
(65) This leaks two objects:
- the RamfbHandle handle created with
gBS->InstallMultipleProtocolInterfaces(),
- the RamfbDevicePath devpath created with AppendDevicePathNode().
In edk2 we like the "cascading error path", with several labels and goto
statements; I suggest we put it to use as well. On the error path, we
should tear down resources in reverse order of construction, as usual:
- in UEFI, a handle is created when the first protocol interface is
installed on an initially NULL handle; and a handle destroyed when the
last protocol interface is uninstalled from it.
Common pitfall: InstallMultipleProtocolInterfaces() takes a *pointer to*
a handle (because if the handle is NULL on input, the function wants to
store the new handle on output); however,
UninstallMultipleProtocolInterfaces() takes the handle itself! Be
careful with the address-of ("&") operator.
- "RamfbDevicePath" should be freed with FreePool().
> + }
> +
> + /* gop devpath + protocol */
(66) please see (32).
> + ZeroMem (&AcpiDeviceNode, sizeof (ACPI_ADR_DEVICE_PATH));
(67) Does (59) apply here?
> + AcpiDeviceNode.Header.Type = ACPI_DEVICE_PATH;
> + AcpiDeviceNode.Header.SubType = ACPI_ADR_DP;
> + AcpiDeviceNode.ADR = ACPI_DISPLAY_ADR (1, 0, 0, 1, 0,
> + ACPI_ADR_DISPLAY_TYPE_EXTERNAL_DIGITAL,
> + 0, 0);
(68) Please add comments to the individual arguments. Example:
"OvmfPkg/VirtioGpuDxe/DriverBinding.c", the initializer of "mAcpiAdr".
> + SetDevicePathNodeLength (&AcpiDeviceNode.Header, sizeof (ACPI_ADR_DEVICE_PATH));
> +
> + GopDevicePath = AppendDevicePathNode (
> + RamfbDevicePath,
> + (EFI_DEVICE_PATH_PROTOCOL *) &AcpiDeviceNode);
> +
> + Status = gBS->InstallMultipleProtocolInterfaces (
> + &GopHandle,
> + &gEfiDevicePathProtocolGuid, GopDevicePath,
> + &gEfiGraphicsOutputProtocolGuid, &QemuRamfbGraphicsOutput,
> + NULL);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((EFI_D_INFO, "Ramfb: install GOP DevicePath failed\n"));
> + FreePool((VOID*)(UINTN)QemuRamfbMode.FrameBufferBase);
> + return Status;
> + }
(69) Same comments as (60) through (65).
> +
> + gBS->OpenProtocol (
> + RamfbHandle,
> + &gEfiDevicePathProtocolGuid,
> + &DevicePath,
> + gImageHandle,
> + GopHandle,
> + EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER);
(70) Please see (16).
(71) Please check the return status. If the call fails, please roll back
the earlier actions and propagate the error out of the entry point function.
> +
> + return EFI_SUCCESS;
> +}
> diff --git a/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf b/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
> new file mode 100644
> index 0000000000..75a7d86832
> --- /dev/null
> +++ b/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
> @@ -0,0 +1,34 @@
(72) Here's a shared request for both new files ("QemuRamfb.c" and
"QemuRamfbDxe.inf"): please refer to the standard .c and .inf file
"banners" for example under OvmfPkg/VirtioGpuDxe. Files should start with:
- a @file comment that briefly states what the file is good for,
- a list of copyright notices,
- the copyright license / reference (2-clause BSDL usually).
> +[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.common]
> + QemuRamfb.c
(73) We can simply say [Sources] here.
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> + BaseMemoryLib
> + DebugLib
> + DevicePathLib
> + FrameBufferBltLib
> + MemoryAllocationLib
> + UefiBootManagerLib
> + UefiBootServicesTableLib
> + UefiDriverEntryPoint
> + UefiLib
> + QemuFwCfgLib
> +
> +[Protocols]
> + gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START
(74) The "BY_START" protocol usage comment is incorrect here, because
we're not producing instances of the protocol in a DriverBindingStart()
function. (In other words, the driver is not a UEFI_DRIVER that conforms
to the UEFI driver model, instead it's a platform DXE driver.) Please
use the comment "## PRODUCES" (you can also drop "PROTOCOL").
(75) In turn, is where I refer back to (5) and (57). Please add a
section like this:
[Guids]
gQemuRamfbGuid
This will ensure that the build utilities generate a "gQemuRamfbGuid"
*definition* (not just a declaration) for you.
> +
> +[Depex]
> + TRUE
>
I realize this review ended up quite long and tedious. I think (hope!)
that you're going to contribute to edk2 in the longer term, thus I
intended to use this opportunity to spell out idioms and such that might
apply to your future patches too.
Thank you!
Laszlo
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] OvmfPkg: add QemuRamfb to platform console
2018-06-08 11:39 ` [PATCH 3/4] OvmfPkg: add QemuRamfb to platform console Gerd Hoffmann
@ 2018-06-11 16:24 ` Laszlo Ersek
2018-06-11 16:58 ` Laszlo Ersek
0 siblings, 1 reply; 18+ messages in thread
From: Laszlo Ersek @ 2018-06-11 16:24 UTC (permalink / raw)
To: Gerd Hoffmann, edk2-devel
On 06/08/18 13:39, Gerd Hoffmann wrote:
> Add QemuRamfbDxe device path to the list of platform console devices,
> so ConSplitter will pick up the device even though it isn't a PCI GPU.
This explanation is not wrong, but I'll list more details, just in case.
By adding the devpath to "gPlatformConsole" with CONSOLE_OUT,
technically we push the devpath into the ConOut UEFI variable, as
follows:
BdsEntry() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c]
PlatformBootManagerBeforeConsole() [OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c]
PlatformInitializeConsole() [OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c]
EfiBootManagerUpdateConsoleVariable() [MdeModulePkg/Library/UefiBootManagerLib/BmConsole.c]
After this, BdsEntry goes on to actually connect the device (i.e., it
makes the QemuRamfbDxe driver bind the device):
BdsEntry() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c]
EfiBootManagerConnectAllDefaultConsoles() [MdeModulePkg/Library/UefiBootManagerLib/BmConsole.c]
Binding drivers to keyboard, serial and graphics devices, so that text
input, text output, and graphics output protocols are produced, isn't
per se sufficient for ConSplitterDxe to multiplex to/from those
protocols. For that, ConPlatformDxe needs to "tag" the handles with
EfiConsole(In|Out)DeviceGuid|EfiStandardErrorDeviceGuid --
ConSplitterDxe depends on those protocols. It's ConPlatformDxe that
needs the devpath to be in ConIn/ConOut/ErrOut, for the "tagging" to
occur.
In total, we add the ramfb devpath to "gPlatformConsole" so that our
PlatformInitializeConsole() function puts it in ConOut, despite ramfb
not being a PCI display device. Binding the device to QemuRamFbDxe, and
multiplexing from/to it happen "automatically" from there, thanks to
BdsDxe, and ConPlatformDxe/ConSplitterDxe respectively.
(1) I don't mind if you stick with the current commit message; I just
wanted to provide more details for this discussion.
More comments below:
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> .../Library/PlatformBootManagerLib/PlatformData.c | 44 ++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
>
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
> index a50cd7bcaf..6202516971 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
> @@ -14,6 +14,7 @@
> **/
>
> #include "BdsPlatform.h"
> +#include <Guid/QemuRamfb.h>
>
> //
> // Debug Agent UART Device Path structure
> @@ -37,6 +38,17 @@ typedef struct {
> } USB_KEYBOARD_DEVICE_PATH;
> #pragma pack ()
>
> +//
> +// QemuRamfb Device Path structure
> +//
> +#pragma pack(1)
> +typedef struct {
> + VENDOR_DEVICE_PATH Vendor;
> + ACPI_ADR_DEVICE_PATH AcpiAdr;
> + EFI_DEVICE_PATH_PROTOCOL End;
> +} VENDOR_RAMFB_DEVICE_PATH;
> +#pragma pack()
(2) Please add a space character between each "pack" and "(".
> +
> ACPI_HID_DEVICE_PATH gPnpPs2KeyboardDeviceNode = gPnpPs2Keyboard;
> ACPI_HID_DEVICE_PATH gPnp16550ComPortDeviceNode = gPnp16550ComPort;
> UART_DEVICE_PATH gUartDeviceNode = gUart;
> @@ -100,6 +112,34 @@ STATIC USB_KEYBOARD_DEVICE_PATH gUsbKeyboardDevicePath = {
> gEndEntire
> };
>
> +STATIC VENDOR_RAMFB_DEVICE_PATH gQemuRamfbDevicePath = {
> + {
> + {
> + HARDWARE_DEVICE_PATH,
> + HW_VENDOR_DP,
> + {
> + (UINT8) (sizeof (VENDOR_DEVICE_PATH)),
> + (UINT8) ((sizeof (VENDOR_DEVICE_PATH)) >> 8)
> + }
> + },
> + QEMU_RAMFB_GUID,
> + },
> + {
> + {
> + ACPI_DEVICE_PATH,
> + ACPI_ADR_DP,
> + {
> + (UINT8) (sizeof (ACPI_ADR_DEVICE_PATH)),
> + (UINT8) ((sizeof (ACPI_ADR_DEVICE_PATH)) >> 8)
> + }
> + },
> + ACPI_DISPLAY_ADR (1, 0, 0, 1, 0,
> + ACPI_ADR_DISPLAY_TYPE_EXTERNAL_DIGITAL,
> + 0, 0)
(3) Urgh. On one hand, I dislike this dump of constants; on the other
hand, I don't want to ask you to repeat all the parameter comments that
I asked for in the previous patch.
OK, let's stick with the dump of constants, just make sure the
indentation is idiomatic.
> + },
> + gEndEntire
> +};
> +
> //
> // Predefined platform default console device path
> //
> @@ -113,6 +153,10 @@ PLATFORM_CONSOLE_CONNECT_ENTRY gPlatformConsole[] = {
> CONSOLE_IN
> },
> {
> + (EFI_DEVICE_PATH_PROTOCOL *)&gQemuRamfbDevicePath,
> + CONSOLE_OUT
> + },
Right, this is consistent with PreparePciDisplayDevicePath().
Thanks,
Laszlo
> + {
> NULL,
> 0
> }
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] ArmVirtPkg: add QemuRamfbDxe
2018-06-08 11:39 ` [PATCH 4/4] ArmVirtPkg: add QemuRamfbDxe Gerd Hoffmann
@ 2018-06-11 16:29 ` Laszlo Ersek
0 siblings, 0 replies; 18+ messages in thread
From: Laszlo Ersek @ 2018-06-11 16:29 UTC (permalink / raw)
To: Gerd Hoffmann, edk2-devel
On 06/08/18 13:39, Gerd Hoffmann wrote:
> Build wireup for ArmVirt.
Ard requested earlier that we include the meaning of the subject line in
the body of the commit message as well, so that the commit message can
be sensibly read without looking at the subject line.
With a bit more elaboration like that:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> ArmVirtPkg/ArmVirtQemu.dsc | 2 ++
> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
> ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 ++
> 3 files changed, 5 insertions(+)
>
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index d74feb709c..744d127a10 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -57,6 +57,7 @@
> BootLogoLib|MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf
> PlatformBootManagerLib|ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
> + FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
> QemuBootOrderLib|OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
> FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
> PciPcdProducerLib|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
> @@ -391,6 +392,7 @@
> #
> # Video support
> #
> + OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
> OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
> OvmfPkg/PlatformDxe/Platform.inf
>
> diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> index 89f95b2d99..63a202c788 100644
> --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> @@ -175,6 +175,7 @@ READ_LOCK_STATUS = TRUE
> #
> # Video support
> #
> + INF OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
> INF OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
> INF OvmfPkg/PlatformDxe/Platform.inf
>
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 1e823aeab7..e59f53b728 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -57,6 +57,7 @@
> BootLogoLib|MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf
> PlatformBootManagerLib|ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
> + FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
> QemuBootOrderLib|OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
> FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
> PciPcdProducerLib|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
> @@ -380,6 +381,7 @@
> #
> # Video support
> #
> + OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
> OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
> OvmfPkg/PlatformDxe/Platform.inf
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] Add QemuRamfbDxe driver
2018-06-08 11:39 [PATCH 0/4] Add QemuRamfbDxe driver Gerd Hoffmann
` (3 preceding siblings ...)
2018-06-08 11:39 ` [PATCH 4/4] ArmVirtPkg: add QemuRamfbDxe Gerd Hoffmann
@ 2018-06-11 16:35 ` Laszlo Ersek
2018-06-12 9:21 ` Gerd Hoffmann
4 siblings, 1 reply; 18+ messages in thread
From: Laszlo Ersek @ 2018-06-11 16:35 UTC (permalink / raw)
To: Gerd Hoffmann, edk2-devel
On 06/08/18 13:39, Gerd Hoffmann wrote:
> This is the efi driver for qemu ramfb, a simple boot framebuffer.
> Qemu patches just have been posted to qemu-devel.
>
> Gerd Hoffmann (4):
> OvmfPkg: add QEMU_RAMFB_GUID
> OvmfPkg: add QemuRamfbDxe
> OvmfPkg: add QemuRamfb to platform console
> ArmVirtPkg: add QemuRamfbDxe
>
> OvmfPkg/Include/Guid/QemuRamfb.h | 25 ++
> .../Library/PlatformBootManagerLib/PlatformData.c | 44 +++
> OvmfPkg/QemuRamfbDxe/QemuRamfb.c | 308 +++++++++++++++++++++
> ArmVirtPkg/ArmVirtQemu.dsc | 2 +
> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
> ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 +
> OvmfPkg/OvmfPkg.dec | 1 +
> 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 | 34 +++
> 14 files changed, 423 insertions(+)
> create mode 100644 OvmfPkg/Include/Guid/QemuRamfb.h
> create mode 100644 OvmfPkg/QemuRamfbDxe/QemuRamfb.c
> create mode 100644 OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
>
Some testing related questions:
- What happens in the UEFI shell if you do recursive connect/disconnect
cycles for all handles in the system? (Preferably initiated from serial.)
- What happens if you locate the parent handle (with the VenHw node)
and/or the child handle (with the GOP on it), and try to disconnect them?
- Have you tested mode changes with the MODE command?
Expected results:
- recursive connect / disconnect should not break, as far as the entire
system is concerned; the procedure should simply skip ramfb.
- targeted connect / disconnect for ramfb should fail (produce an error
message), but nothing should crash or stop working.
- mode changes should work.
I expect the first two behaviors because the driver is a platform DXE
driver, not a UEFI driver that conforms to the UEFI driver model -- we
don't install an EFI_DRIVER_BINDING_PROTOCOL instance, hence the driver
should be "invisible" to the connect / disconnect UEFI shell commands
(they should fail gracefully).
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] OvmfPkg: add QemuRamfb to platform console
2018-06-11 16:24 ` Laszlo Ersek
@ 2018-06-11 16:58 ` Laszlo Ersek
0 siblings, 0 replies; 18+ messages in thread
From: Laszlo Ersek @ 2018-06-11 16:58 UTC (permalink / raw)
To: Gerd Hoffmann, edk2-devel
On 06/11/18 18:24, Laszlo Ersek wrote:
> On 06/08/18 13:39, Gerd Hoffmann wrote:
>> Add QemuRamfbDxe device path to the list of platform console devices,
>> so ConSplitter will pick up the device even though it isn't a PCI GPU.
>
> This explanation is not wrong, but I'll list more details, just in case.
>
> By adding the devpath to "gPlatformConsole" with CONSOLE_OUT,
> technically we push the devpath into the ConOut UEFI variable, as
> follows:
>
> BdsEntry() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c]
> PlatformBootManagerBeforeConsole() [OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c]
> PlatformInitializeConsole() [OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c]
> EfiBootManagerUpdateConsoleVariable() [MdeModulePkg/Library/UefiBootManagerLib/BmConsole.c]
>
> After this, BdsEntry goes on to actually connect the device (i.e., it
> makes the QemuRamfbDxe driver bind the device):
>
> BdsEntry() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c]
> EfiBootManagerConnectAllDefaultConsoles() [MdeModulePkg/Library/UefiBootManagerLib/BmConsole.c]
>
> Binding drivers to keyboard, serial and graphics devices, so that text
> input, text output, and graphics output protocols are produced, isn't
> per se sufficient for ConSplitterDxe to multiplex to/from those
> protocols. For that, ConPlatformDxe needs to "tag" the handles with
> EfiConsole(In|Out)DeviceGuid|EfiStandardErrorDeviceGuid --
> ConSplitterDxe depends on those protocols. It's ConPlatformDxe that
> needs the devpath to be in ConIn/ConOut/ErrOut, for the "tagging" to
> occur.
>
> In total, we add the ramfb devpath to "gPlatformConsole" so that our
> PlatformInitializeConsole() function puts it in ConOut, despite ramfb
> not being a PCI display device. Binding the device to QemuRamFbDxe, and
> multiplexing from/to it happen "automatically" from there, thanks to
> BdsDxe, and ConPlatformDxe/ConSplitterDxe respectively.
Whoops, managed to confuse myself a little here; some correction should
be in order:
The ramfb driver does not follow the UEFI driver model. The GOP is
produced right in the entry point of the driver, not when platform BDS
connects the device.
It remains true however that ConPlatformDxe / ConSplitterDxe, which do
follow the UEFI driver model, bind the GOP in
EfiBootManagerConnectAllDefaultConsoles(). I guess I would re-formulate,
"""
In total, we add the ramfb devpath to "gPlatformConsole" so that our
PlatformInitializeConsole() function puts it in ConOut, despite ramfb
not being a PCI display device. Multiplexing from/to the ramfb GOP
happens "automatically" from there, thanks to
ConPlatformDxe/ConSplitterDxe.
"""
Which is basically what the current commit message says. :)
Sorry for any confusion caused!
Laszlo
>
> (1) I don't mind if you stick with the current commit message; I just
> wanted to provide more details for this discussion.
>
> More comments below:
>
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>> .../Library/PlatformBootManagerLib/PlatformData.c | 44 ++++++++++++++++++++++
>> 1 file changed, 44 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
>> index a50cd7bcaf..6202516971 100644
>> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
>> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
>> @@ -14,6 +14,7 @@
>> **/
>>
>> #include "BdsPlatform.h"
>> +#include <Guid/QemuRamfb.h>
>>
>> //
>> // Debug Agent UART Device Path structure
>> @@ -37,6 +38,17 @@ typedef struct {
>> } USB_KEYBOARD_DEVICE_PATH;
>> #pragma pack ()
>>
>> +//
>> +// QemuRamfb Device Path structure
>> +//
>> +#pragma pack(1)
>> +typedef struct {
>> + VENDOR_DEVICE_PATH Vendor;
>> + ACPI_ADR_DEVICE_PATH AcpiAdr;
>> + EFI_DEVICE_PATH_PROTOCOL End;
>> +} VENDOR_RAMFB_DEVICE_PATH;
>> +#pragma pack()
>
> (2) Please add a space character between each "pack" and "(".
>
>> +
>> ACPI_HID_DEVICE_PATH gPnpPs2KeyboardDeviceNode = gPnpPs2Keyboard;
>> ACPI_HID_DEVICE_PATH gPnp16550ComPortDeviceNode = gPnp16550ComPort;
>> UART_DEVICE_PATH gUartDeviceNode = gUart;
>> @@ -100,6 +112,34 @@ STATIC USB_KEYBOARD_DEVICE_PATH gUsbKeyboardDevicePath = {
>> gEndEntire
>> };
>>
>> +STATIC VENDOR_RAMFB_DEVICE_PATH gQemuRamfbDevicePath = {
>> + {
>> + {
>> + HARDWARE_DEVICE_PATH,
>> + HW_VENDOR_DP,
>> + {
>> + (UINT8) (sizeof (VENDOR_DEVICE_PATH)),
>> + (UINT8) ((sizeof (VENDOR_DEVICE_PATH)) >> 8)
>> + }
>> + },
>> + QEMU_RAMFB_GUID,
>> + },
>> + {
>> + {
>> + ACPI_DEVICE_PATH,
>> + ACPI_ADR_DP,
>> + {
>> + (UINT8) (sizeof (ACPI_ADR_DEVICE_PATH)),
>> + (UINT8) ((sizeof (ACPI_ADR_DEVICE_PATH)) >> 8)
>> + }
>> + },
>> + ACPI_DISPLAY_ADR (1, 0, 0, 1, 0,
>> + ACPI_ADR_DISPLAY_TYPE_EXTERNAL_DIGITAL,
>> + 0, 0)
>
> (3) Urgh. On one hand, I dislike this dump of constants; on the other
> hand, I don't want to ask you to repeat all the parameter comments that
> I asked for in the previous patch.
>
> OK, let's stick with the dump of constants, just make sure the
> indentation is idiomatic.
>
>> + },
>> + gEndEntire
>> +};
>> +
>> //
>> // Predefined platform default console device path
>> //
>> @@ -113,6 +153,10 @@ PLATFORM_CONSOLE_CONNECT_ENTRY gPlatformConsole[] = {
>> CONSOLE_IN
>> },
>> {
>> + (EFI_DEVICE_PATH_PROTOCOL *)&gQemuRamfbDevicePath,
>> + CONSOLE_OUT
>> + },
>
> Right, this is consistent with PreparePciDisplayDevicePath().
>
> Thanks,
> Laszlo
>
>> + {
>> NULL,
>> 0
>> }
>>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] OvmfPkg: add QemuRamfbDxe
2018-06-11 15:56 ` Laszlo Ersek
@ 2018-06-12 9:15 ` Gerd Hoffmann
2018-06-12 13:01 ` Laszlo Ersek
0 siblings, 1 reply; 18+ messages in thread
From: Gerd Hoffmann @ 2018-06-12 9:15 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel
Hi,
> > + {
> > + .HorizontalResolution = 640,
> > + .VerticalResolution = 480,
> > + },{
> > + .HorizontalResolution = 800,
> > + .VerticalResolution = 600,
> > + },{
> > + .HorizontalResolution = 1024,
> > + .VerticalResolution = 768,
> > + }
> > +};
>
> (10) In edk2 we cannot use designated initializers. I suggest (for
> example) assigning these values in the entry point function.
Really? C99 is almost 20 years old now ...
Are there compilers left without C99 support which edk2 still supports?
> In general, in edk2 there are two accepted indentation styles for
> function calls that extend to multiple lines:
>
> variant #1: all arguments (including the first one) on separate lines,
> with the closing paren also on a separate line:
>
> Status = StructPointer->Function (
> Arg1,
> Arg2,
> Arg3
> );
Hmm, pretty unusual, which is bad for editor auto-indent support.
Anyone knows tricks to teack emacs that style?
cheers,
Gerd
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] Add QemuRamfbDxe driver
2018-06-11 16:35 ` [PATCH 0/4] Add QemuRamfbDxe driver Laszlo Ersek
@ 2018-06-12 9:21 ` Gerd Hoffmann
2018-06-12 12:53 ` Laszlo Ersek
0 siblings, 1 reply; 18+ messages in thread
From: Gerd Hoffmann @ 2018-06-12 9:21 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel
Hi,
> - What happens in the UEFI shell if you do recursive connect/disconnect
> cycles for all handles in the system? (Preferably initiated from serial.)
Seems to work fine (tried "disconnect -a" and "connect").
> - What happens if you locate the parent handle (with the VenHw node)
> and/or the child handle (with the GOP on it), and try to disconnect them?
How can I do that?
> - Have you tested mode changes with the MODE command?
Yes, works.
cheers,
Gerd
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] Add QemuRamfbDxe driver
2018-06-12 9:21 ` Gerd Hoffmann
@ 2018-06-12 12:53 ` Laszlo Ersek
2018-06-13 7:40 ` Gerd Hoffmann
0 siblings, 1 reply; 18+ messages in thread
From: Laszlo Ersek @ 2018-06-12 12:53 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: edk2-devel
On 06/12/18 11:21, Gerd Hoffmann wrote:
> Hi,
>
>> - What happens in the UEFI shell if you do recursive connect/disconnect
>> cycles for all handles in the system? (Preferably initiated from serial.)
>
> Seems to work fine (tried "disconnect -a" and "connect").
Cool.
>
>> - What happens if you locate the parent handle (with the VenHw node)
>> and/or the child handle (with the GOP on it), and try to disconnect them?
>
> How can I do that?
Run "dh -d -v -p GraphicsOutput". The listing will include all handles
(represented by hex identifiers) that carry a GOP. Each handle will also
have its device path protocol instance displayed, in textual
representation, so if there are multiple GOPs, you'll be able to locate
the one produced by QemuRamfbDxe. The listing should also reference the
parent controller handle.
Knowing the hex identifiers for parent and child, experiment with the
"disconnect" command. (See "help disconnect" for syntax.)
>> - Have you tested mode changes with the MODE command?
>
> Yes, works.
Nice!
Thansk
Laszlo
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] OvmfPkg: add QemuRamfbDxe
2018-06-12 9:15 ` Gerd Hoffmann
@ 2018-06-12 13:01 ` Laszlo Ersek
0 siblings, 0 replies; 18+ messages in thread
From: Laszlo Ersek @ 2018-06-12 13:01 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: edk2-devel
On 06/12/18 11:15, Gerd Hoffmann wrote:
> Hi,
>
>>> + {
>>> + .HorizontalResolution = 640,
>>> + .VerticalResolution = 480,
>>> + },{
>>> + .HorizontalResolution = 800,
>>> + .VerticalResolution = 600,
>>> + },{
>>> + .HorizontalResolution = 1024,
>>> + .VerticalResolution = 768,
>>> + }
>>> +};
>>
>> (10) In edk2 we cannot use designated initializers. I suggest (for
>> example) assigning these values in the entry point function.
>
> Really? C99 is almost 20 years old now ...
> Are there compilers left without C99 support which edk2 still supports?
Visual Studio has never committed to *full* C99 support, to my
knowledge. I don't know whether VS happens to support designated
initializers specifically; either way, we can't use them in edk2.
... After some googling, I see "signs" that VS >=2013 supports
designated initializers.
However, edk2 targets VS 2003, 2005, 2008, 2010, and 2012 too, before
2013. Refer to "Supported Tool Chains" in
"BaseTools/Conf/tools_def.template".
>
>> In general, in edk2 there are two accepted indentation styles for
>> function calls that extend to multiple lines:
>>
>> variant #1: all arguments (including the first one) on separate lines,
>> with the closing paren also on a separate line:
>>
>> Status = StructPointer->Function (
>> Arg1,
>> Arg2,
>> Arg3
>> );
>
> Hmm, pretty unusual,
Yes, very much. I believe this indentation style might originate from
Windows, but I'm not sure.
> which is bad for editor auto-indent support.
> Anyone knows tricks to teack emacs that style?
I don't use emacs, apologies!
Laszlo
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] Add QemuRamfbDxe driver
2018-06-12 12:53 ` Laszlo Ersek
@ 2018-06-13 7:40 ` Gerd Hoffmann
2018-06-13 16:16 ` Laszlo Ersek
0 siblings, 1 reply; 18+ messages in thread
From: Gerd Hoffmann @ 2018-06-13 7:40 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel
Hi,
> >> - What happens if you locate the parent handle (with the VenHw node)
> >> and/or the child handle (with the GOP on it), and try to disconnect them?
> >
> > How can I do that?
>
> Run "dh -d -v -p GraphicsOutput". The listing will include all handles
> (represented by hex identifiers) that carry a GOP. Each handle will also
> have its device path protocol instance displayed, in textual
> representation, so if there are multiple GOPs, you'll be able to locate
> the one produced by QemuRamfbDxe. The listing should also reference the
> parent controller handle.
>
> Knowing the hex identifiers for parent and child, experiment with the
> "disconnect" command. (See "help disconnect" for syntax.)
Shell> dh -p GraphicsOutput
Handle dump by protocol 'GraphicsOutput'
44: ConsoleOut SimpleTextOut GraphicsOutput(GraphicsOutput) DevicePath(..C08C457)/AcpiAdr(0x80010300))
6D: GraphicsOutput(GraphicsOutput) SimpleTextOut
Shell> disconnect 6d
Disconnect - (6D,3E643560,3E5C3A03) Result Success.
Shell> disconnect 44
Disconnect - (44,3E643560,3E5C3A03) Result Success.
Shell>
The second disconnect makes ovmf stop using ramfb as console (serial continues
to work).
cheers,
Gerd
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] Add QemuRamfbDxe driver
2018-06-13 7:40 ` Gerd Hoffmann
@ 2018-06-13 16:16 ` Laszlo Ersek
0 siblings, 0 replies; 18+ messages in thread
From: Laszlo Ersek @ 2018-06-13 16:16 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: edk2-devel
On 06/13/18 09:40, Gerd Hoffmann wrote:
> Hi,
>
>>>> - What happens if you locate the parent handle (with the VenHw node)
>>>> and/or the child handle (with the GOP on it), and try to disconnect them?
>>>
>>> How can I do that?
>>
>> Run "dh -d -v -p GraphicsOutput". The listing will include all handles
>> (represented by hex identifiers) that carry a GOP. Each handle will also
>> have its device path protocol instance displayed, in textual
>> representation, so if there are multiple GOPs, you'll be able to locate
>> the one produced by QemuRamfbDxe. The listing should also reference the
>> parent controller handle.
>>
>> Knowing the hex identifiers for parent and child, experiment with the
>> "disconnect" command. (See "help disconnect" for syntax.)
>
> Shell> dh -p GraphicsOutput
> Handle dump by protocol 'GraphicsOutput'
> 44: ConsoleOut SimpleTextOut GraphicsOutput(GraphicsOutput) DevicePath(..C08C457)/AcpiAdr(0x80010300))
> 6D: GraphicsOutput(GraphicsOutput) SimpleTextOut
> Shell> disconnect 6d
> Disconnect - (6D,3E643560,3E5C3A03) Result Success.
> Shell> disconnect 44
> Disconnect - (44,3E643560,3E5C3A03) Result Success.
> Shell>
>
> The second disconnect makes ovmf stop using ramfb as console (serial continues
> to work).
I'll follow up under your v3 series:
[edk2] [PATCH v3 0/4] Add QemuRamfbDxe driver
http://mid.mail-archive.com/20180613072936.12480-1-kraxel@redhat.com
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-06-13 16:16 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-08 11:39 [PATCH 0/4] Add QemuRamfbDxe driver Gerd Hoffmann
2018-06-08 11:39 ` [PATCH 1/4] OvmfPkg: add QEMU_RAMFB_GUID Gerd Hoffmann
2018-06-11 13:06 ` Laszlo Ersek
2018-06-08 11:39 ` [PATCH 2/4] OvmfPkg: add QemuRamfbDxe Gerd Hoffmann
2018-06-10 5:57 ` Ard Biesheuvel
2018-06-11 15:56 ` Laszlo Ersek
2018-06-12 9:15 ` Gerd Hoffmann
2018-06-12 13:01 ` Laszlo Ersek
2018-06-08 11:39 ` [PATCH 3/4] OvmfPkg: add QemuRamfb to platform console Gerd Hoffmann
2018-06-11 16:24 ` Laszlo Ersek
2018-06-11 16:58 ` Laszlo Ersek
2018-06-08 11:39 ` [PATCH 4/4] ArmVirtPkg: add QemuRamfbDxe Gerd Hoffmann
2018-06-11 16:29 ` Laszlo Ersek
2018-06-11 16:35 ` [PATCH 0/4] Add QemuRamfbDxe driver Laszlo Ersek
2018-06-12 9:21 ` Gerd Hoffmann
2018-06-12 12:53 ` Laszlo Ersek
2018-06-13 7:40 ` Gerd Hoffmann
2018-06-13 16:16 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox