From: Phil Dennis-Jordan <phil@philjordan.eu>
In addition to the QXL, Cirrus, etc. VGA adapters, Qemu also implements
a basic version of VMWare's SVGA display device. Drivers for this
device exist for some guest OSes which do not support Qemu's other
display adapters, so supporting it in OVMF is useful in conjunction
with those OSes.
This change adds support for the SVGA device's framebuffer to
QemuVideoDxe's graphics output protocol implementation, based on
VMWare's documentation. The most basic initialisation, framebuffer
layout query, and mode setting operations are implemented.
The device relies on port-based 32-bit I/O, unfortunately on misaligned
addresses. This limits the driver's support to the x86 family of
platforms.
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
Notes:
v2:
- Unaligned I/O helper functions moved to separate commit [Laszlo]
- Multi-line function call whitespace fixes.
v3:
- Dropped the "2" from "SVGA2" where appropriate. [Jordan, Laszlo]
- Renamed various struct fields and functions with consistent prefixes [Laszlo]
- #include orders fixed [Laszlo]
- Renamedi/moved lots of local variables to comply with convention. [Laszlo]
- Added error checking to PCI BAR queries. [Laszlo]
- Moved some function definitions around for better grouping. [Laszlo]
- Fixed ClearScreen() to use the correct VRAM BAR. [Laszlo]
- Changed modelist initialisation to fetch all mode data on startup, so mode
queries can return everything including channel masks without hitting the
device. Mask calculations hopefully make more sense now. [Laszlo]
- Whitespace fixes. [Laszlo]
- Fixed a memory leak in BAR query.
OvmfPkg/QemuVideoDxe/Qemu.h | 29 ++++
OvmfPkg/QemuVideoDxe/Driver.c | 127 ++++++++++++++-
OvmfPkg/QemuVideoDxe/Gop.c | 61 +++++++-
OvmfPkg/QemuVideoDxe/Initialize.c | 161 ++++++++++++++++++++
4 files changed, 371 insertions(+), 7 deletions(-)
diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
index 2ce37defc5b8..7fbb25b3efd3 100644
--- a/OvmfPkg/QemuVideoDxe/Qemu.h
+++ b/OvmfPkg/QemuVideoDxe/Qemu.h
@@ -92,6 +92,7 @@ typedef enum {
QEMU_VIDEO_CIRRUS_5446,
QEMU_VIDEO_BOCHS,
QEMU_VIDEO_BOCHS_MMIO,
+ QEMU_VIDEO_VMWARE_SVGA,
} QEMU_VIDEO_VARIANT;
typedef struct {
@@ -115,10 +116,13 @@ typedef struct {
//
UINTN MaxMode;
QEMU_VIDEO_MODE_DATA *ModeData;
+ EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *VmwareSvgaModeInfo;
QEMU_VIDEO_VARIANT Variant;
FRAME_BUFFER_CONFIGURE *FrameBufferBltConfigure;
UINTN FrameBufferBltConfigureSize;
+ UINT8 FrameBufferVramBarIndex;
+ UINT16 VmwareSvgaBasePort;
} QEMU_VIDEO_PRIVATE_DATA;
///
@@ -502,9 +506,34 @@ QemuVideoBochsModeSetup (
BOOLEAN IsQxl
);
+EFI_STATUS
+QemuVideoVmwareSvgaModeSetup (
+ QEMU_VIDEO_PRIVATE_DATA *Private
+ );
+
VOID
InstallVbeShim (
IN CONST CHAR16 *CardName,
IN EFI_PHYSICAL_ADDRESS FrameBufferBase
);
+
+VOID
+VmwareSvgaWrite (
+ QEMU_VIDEO_PRIVATE_DATA *Private,
+ UINT16 Register,
+ UINT32 Value
+ );
+
+UINT32
+VmwareSvgaRead (
+ QEMU_VIDEO_PRIVATE_DATA *Private,
+ UINT16 Register
+ );
+
+VOID
+InitializeVmwareSvgaGraphicsMode (
+ QEMU_VIDEO_PRIVATE_DATA *Private,
+ QEMU_VIDEO_BOCHS_MODES *ModeData
+ );
+
#endif
diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
index fc8025ec46de..79c430e920d7 100644
--- a/OvmfPkg/QemuVideoDxe/Driver.c
+++ b/OvmfPkg/QemuVideoDxe/Driver.c
@@ -14,8 +14,10 @@
**/
-#include "Qemu.h"
+#include <IndustryStandard/VmwareSvga.h>
#include <IndustryStandard/Acpi.h>
+#include "Qemu.h"
+#include "UnalignedIoInternal.h"
EFI_DRIVER_BINDING_PROTOCOL gQemuVideoDriverBinding = {
QemuVideoControllerDriverSupported,
@@ -58,6 +60,11 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = {
QEMU_VIDEO_BOCHS_MMIO,
L"QEMU VirtIO VGA"
},{
+ VMWARE_PCI_VENDOR_ID_VMWARE,
+ VMWARE_PCI_DEVICE_ID_VMWARE_SVGA2,
+ QEMU_VIDEO_VMWARE_SVGA,
+ L"QEMU VMWare SVGA"
+ },{
0 /* end of list */
}
};
@@ -242,6 +249,7 @@ QemuVideoControllerDriverStart (
goto ClosePciIo;
}
Private->Variant = Card->Variant;
+ Private->FrameBufferVramBarIndex = PCI_BAR_IDX0;
//
// IsQxl is based on the detected Card->Variant, which at a later point might
@@ -317,6 +325,59 @@ QemuVideoControllerDriverStart (
}
//
+ // Check if accessing Vmware SVGA interface works
+ //
+ if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *IoDesc;
+ UINT32 TargetId;
+ UINT32 SvgaIdRead;
+
+ IoDesc = NULL;
+ Status = Private->PciIo->GetBarAttributes (
+ Private->PciIo,
+ PCI_BAR_IDX0,
+ NULL,
+ (VOID**) &IoDesc
+ );
+ if (EFI_ERROR(Status) ||
+ IoDesc->ResType != ACPI_ADDRESS_SPACE_TYPE_IO ||
+ IoDesc->AddrRangeMin >= MAX_UINT16 ||
+ IoDesc->AddrRangeMin + VMWARE_SVGA_VALUE_PORT >= MAX_UINT16) {
+ if (IoDesc != NULL) {
+ FreePool (IoDesc);
+ }
+ Status = EFI_DEVICE_ERROR;
+ goto RestoreAttributes;
+ }
+ Private->VmwareSvgaBasePort = (UINT16) IoDesc->AddrRangeMin;
+ FreePool (IoDesc);
+
+ TargetId = VMWARE_SVGA_ID_2;
+ while (TRUE) {
+ VmwareSvgaWrite (Private, VmwareSvgaRegId, TargetId);
+ SvgaIdRead = VmwareSvgaRead (Private, VmwareSvgaRegId);
+ if ((SvgaIdRead == TargetId) || (TargetId <= VMWARE_SVGA_ID_0)) {
+ break;
+ }
+ TargetId --;
+ }
+
+ if (SvgaIdRead != TargetId) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "QemuVideo: QEMU_VIDEO_VMWARE_SVGA ID mismatch "
+ "(got 0x%x, base address 0x%x)\n",
+ SvgaIdRead,
+ Private->VmwareSvgaBasePort
+ ));
+ Status = EFI_DEVICE_ERROR;
+ goto RestoreAttributes;
+ }
+
+ Private->FrameBufferVramBarIndex = PCI_BAR_IDX1;
+ }
+
+ //
// Get ParentDevicePath
//
Status = gBS->HandleProtocol (
@@ -371,6 +432,9 @@ QemuVideoControllerDriverStart (
case QEMU_VIDEO_BOCHS:
Status = QemuVideoBochsModeSetup (Private, IsQxl);
break;
+ case QEMU_VIDEO_VMWARE_SVGA:
+ Status = QemuVideoVmwareSvgaModeSetup (Private);
+ break;
default:
ASSERT (FALSE);
Status = EFI_DEVICE_ERROR;
@@ -750,7 +814,7 @@ ClearScreen (
Private->PciIo->Mem.Write (
Private->PciIo,
EfiPciIoWidthFillUint32,
- 0,
+ Private->FrameBufferVramBarIndex,
0,
0x400000 >> 2,
&Color
@@ -888,6 +952,35 @@ BochsRead (
}
VOID
+VmwareSvgaWrite (
+ QEMU_VIDEO_PRIVATE_DATA *Private,
+ UINT16 Register,
+ UINT32 Value
+ )
+{
+ UnalignedIoWrite32 (
+ Private->VmwareSvgaBasePort + VMWARE_SVGA_INDEX_PORT,
+ Register
+ );
+ UnalignedIoWrite32 (
+ Private->VmwareSvgaBasePort + VMWARE_SVGA_VALUE_PORT,
+ Value);
+}
+
+UINT32
+VmwareSvgaRead (
+ QEMU_VIDEO_PRIVATE_DATA *Private,
+ UINT16 Register
+ )
+{
+ UnalignedIoWrite32 (
+ Private->VmwareSvgaBasePort + VMWARE_SVGA_INDEX_PORT,
+ Register);
+ return UnalignedIoRead32 (
+ Private->VmwareSvgaBasePort + VMWARE_SVGA_VALUE_PORT);
+}
+
+VOID
VgaOutb (
QEMU_VIDEO_PRIVATE_DATA *Private,
UINTN Reg,
@@ -941,6 +1034,35 @@ InitializeBochsGraphicsMode (
ClearScreen (Private);
}
+VOID
+InitializeVmwareSvgaGraphicsMode (
+ QEMU_VIDEO_PRIVATE_DATA *Private,
+ QEMU_VIDEO_BOCHS_MODES *ModeData
+ )
+{
+ UINT32 Capabilities;
+
+ VmwareSvgaWrite (Private, VmwareSvgaRegWidth, ModeData->Width);
+ VmwareSvgaWrite (Private, VmwareSvgaRegHeight, ModeData->Height);
+
+ Capabilities = VmwareSvgaRead (
+ Private,
+ VmwareSvgaRegCapabilities
+ );
+ if ((Capabilities & VMWARE_SVGA_CAP_8BIT_EMULATION) != 0) {
+ VmwareSvgaWrite (
+ Private,
+ VmwareSvgaRegBitsPerPixel,
+ ModeData->ColorDepth
+ );
+ }
+
+ VmwareSvgaWrite (Private, VmwareSvgaRegEnable, 1);
+
+ SetDefaultPalette (Private);
+ ClearScreen (Private);
+}
+
EFI_STATUS
EFIAPI
InitializeQemuVideo (
@@ -975,3 +1097,4 @@ InitializeQemuVideo (
return Status;
}
+
diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
index 359e9217d3d1..934c9f39b459 100644
--- a/OvmfPkg/QemuVideoDxe/Gop.c
+++ b/OvmfPkg/QemuVideoDxe/Gop.c
@@ -13,6 +13,7 @@
**/
+#include <IndustryStandard/VmwareSvga.h>
#include "Qemu.h"
STATIC
@@ -75,6 +76,42 @@ QemuVideoCompleteModeData (
return EFI_SUCCESS;
}
+STATIC
+EFI_STATUS
+QemuVideoVmwareSvgaCompleteModeData (
+ IN QEMU_VIDEO_PRIVATE_DATA *Private,
+ OUT EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode
+ )
+{
+ EFI_STATUS Status;
+ EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info;
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *FrameBufDesc;
+ UINT32 BytesPerLine, FbOffset, BytesPerPixel;
+
+ Info = Mode->Info;
+ CopyMem (Info, &Private->VmwareSvgaModeInfo[Mode->Mode], sizeof(*Info));
+ BytesPerPixel = Private->ModeData[Mode->Mode].ColorDepth / 8;
+ BytesPerLine = Info->PixelsPerScanLine * BytesPerPixel;
+
+ FbOffset = VmwareSvgaRead (Private, VmwareSvgaRegFbOffset);
+
+ Status = Private->PciIo->GetBarAttributes (
+ Private->PciIo,
+ PCI_BAR_IDX1,
+ NULL,
+ (VOID**) &FrameBufDesc
+ );
+ if (EFI_ERROR(Status)) {
+ return EFI_DEVICE_ERROR;
+ }
+
+ Mode->FrameBufferBase = FrameBufDesc->AddrRangeMin + FbOffset;
+ Mode->FrameBufferSize = BytesPerLine * Info->VerticalResolution;
+
+ FreePool (FrameBufDesc);
+ return Status;
+}
+
//
// Graphics Output Protocol Member Functions
@@ -124,10 +161,14 @@ Routine Description:
*SizeOfInfo = sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
- ModeData = &Private->ModeData[ModeNumber];
- (*Info)->HorizontalResolution = ModeData->HorizontalResolution;
- (*Info)->VerticalResolution = ModeData->VerticalResolution;
- QemuVideoCompleteModeInfo (ModeData, *Info);
+ if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
+ **Info = Private->VmwareSvgaModeInfo[ModeNumber];
+ } else {
+ ModeData = &Private->ModeData[ModeNumber];
+ (*Info)->HorizontalResolution = ModeData->HorizontalResolution;
+ (*Info)->VerticalResolution = ModeData->VerticalResolution;
+ QemuVideoCompleteModeInfo (ModeData, *Info);
+ }
return EFI_SUCCESS;
}
@@ -176,6 +217,12 @@ Routine Description:
case QEMU_VIDEO_BOCHS:
InitializeBochsGraphicsMode (Private, &QemuVideoBochsModes[ModeData->InternalModeIndex]);
break;
+ case QEMU_VIDEO_VMWARE_SVGA:
+ InitializeVmwareSvgaGraphicsMode (
+ Private,
+ &QemuVideoBochsModes[ModeData->InternalModeIndex]
+ );
+ break;
default:
ASSERT (FALSE);
return EFI_DEVICE_ERROR;
@@ -186,7 +233,11 @@ Routine Description:
This->Mode->Info->VerticalResolution = ModeData->VerticalResolution;
This->Mode->SizeOfInfo = sizeof(EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
- QemuVideoCompleteModeData (Private, This->Mode);
+ if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
+ QemuVideoVmwareSvgaCompleteModeData (Private, This->Mode);
+ } else {
+ QemuVideoCompleteModeData (Private, This->Mode);
+ }
//
// Re-initialize the frame buffer configure when mode changes.
diff --git a/OvmfPkg/QemuVideoDxe/Initialize.c b/OvmfPkg/QemuVideoDxe/Initialize.c
index d5d8cfef9661..8c5c993c7d0e 100644
--- a/OvmfPkg/QemuVideoDxe/Initialize.c
+++ b/OvmfPkg/QemuVideoDxe/Initialize.c
@@ -13,6 +13,7 @@
**/
+#include <IndustryStandard/VmwareSvga.h>
#include "Qemu.h"
@@ -346,3 +347,163 @@ QemuVideoBochsModeSetup (
return EFI_SUCCESS;
}
+EFI_STATUS
+QemuVideoVmwareSvgaModeSetup (
+ QEMU_VIDEO_PRIVATE_DATA *Private
+ )
+{
+ EFI_STATUS Status;
+ UINT32 FbSize;
+ UINT32 MaxWidth, MaxHeight;
+ UINT32 Capabilities;
+ UINT32 BitsPerPixel;
+ UINT32 Index;
+ QEMU_VIDEO_MODE_DATA *ModeData;
+ QEMU_VIDEO_BOCHS_MODES *VideoMode;
+ EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *ModeInfo;
+
+ VmwareSvgaWrite (Private, VmwareSvgaRegEnable, 0);
+
+ Private->ModeData =
+ AllocatePool (sizeof (Private->ModeData[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT);
+ if (Private->ModeData == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto ModeDataAllocError;
+ }
+
+ Private->VmwareSvgaModeInfo =
+ AllocatePool (
+ sizeof (Private->VmwareSvgaModeInfo[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT
+ );
+ if (Private->VmwareSvgaModeInfo == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto ModeInfoAllocError;
+ }
+
+ FbSize = VmwareSvgaRead (Private, VmwareSvgaRegFbSize);
+ MaxWidth = VmwareSvgaRead (Private, VmwareSvgaRegMaxWidth);
+ MaxHeight = VmwareSvgaRead (Private, VmwareSvgaRegMaxHeight);
+ Capabilities = VmwareSvgaRead (Private, VmwareSvgaRegCapabilities);
+ if ((Capabilities & VMWARE_SVGA_CAP_8BIT_EMULATION) != 0) {
+ BitsPerPixel = VmwareSvgaRead (
+ Private,
+ VmwareSvgaRegHostBitsPerPixel
+ );
+ VmwareSvgaWrite (
+ Private,
+ VmwareSvgaRegBitsPerPixel,
+ BitsPerPixel
+ );
+ } else {
+ BitsPerPixel = VmwareSvgaRead (
+ Private,
+ VmwareSvgaRegBitsPerPixel
+ );
+ }
+
+ if (FbSize == 0 ||
+ MaxWidth == 0 ||
+ MaxHeight == 0 ||
+ BitsPerPixel == 0 ||
+ BitsPerPixel % 8 != 0) {
+ Status = EFI_DEVICE_ERROR;
+ goto Rollback;
+ }
+
+ ModeData = Private->ModeData;
+ ModeInfo = Private->VmwareSvgaModeInfo;
+ VideoMode = &QemuVideoBochsModes[0];
+ for (Index = 0; Index < QEMU_VIDEO_BOCHS_MODE_COUNT; Index ++) {
+ UINTN RequiredFbSize;
+
+ RequiredFbSize = (UINTN) VideoMode->Width * VideoMode->Height *
+ (BitsPerPixel / 8);
+ if (RequiredFbSize <= FbSize &&
+ VideoMode->Width <= MaxWidth &&
+ VideoMode->Height <= MaxHeight) {
+ UINT32 BytesPerLine;
+ UINT32 RedMask, GreenMask, BlueMask, PixelMask;
+
+ VmwareSvgaWrite (
+ Private,
+ VmwareSvgaRegWidth,
+ VideoMode->Width
+ );
+ VmwareSvgaWrite (
+ Private,
+ VmwareSvgaRegHeight,
+ VideoMode->Height
+ );
+ BytesPerLine = VmwareSvgaRead (
+ Private,
+ VmwareSvgaRegBytesPerLine
+ );
+
+ ModeData->InternalModeIndex = Index;
+ ModeData->HorizontalResolution = VideoMode->Width;
+ ModeData->VerticalResolution = VideoMode->Height;
+ ModeData->ColorDepth = BitsPerPixel;
+
+ //
+ // Setting VmwareSvgaRegWidth/VmwareSvgaRegHeight actually changes
+ // the device's display mode, so we all properties of each mode up front
+ // to avoid inadvertent mode changes later.
+ //
+ ModeInfo->Version = 0;
+ ModeInfo->HorizontalResolution = ModeData->HorizontalResolution;
+ ModeInfo->VerticalResolution = ModeData->VerticalResolution;
+
+ ModeInfo->PixelFormat = PixelBitMask;
+
+ RedMask = VmwareSvgaRead (Private, VmwareSvgaRegRedMask);
+ ModeInfo->PixelInformation.RedMask = RedMask;
+
+ GreenMask = VmwareSvgaRead (Private, VmwareSvgaRegGreenMask);
+ ModeInfo->PixelInformation.GreenMask = GreenMask;
+
+ BlueMask = VmwareSvgaRead (Private, VmwareSvgaRegBlueMask);
+ ModeInfo->PixelInformation.BlueMask = BlueMask;
+
+ BytesPerLine = VmwareSvgaRead (Private, VmwareSvgaRegBytesPerLine);
+
+ if (BitsPerPixel == 32) {
+ if (BlueMask == 0xff && GreenMask == 0xff00 && RedMask == 0xff0000) {
+ ModeInfo->PixelFormat = PixelBlueGreenRedReserved8BitPerColor;
+ } else if (BlueMask == 0xff0000 &&
+ GreenMask == 0xff00 &&
+ RedMask == 0xff) {
+ ModeInfo->PixelFormat = PixelRedGreenBlueReserved8BitPerColor;
+ }
+ }
+
+ //
+ // Reserved mask is whatever bits in the pixel are not used for RGB.
+ // PixelMask is as many binary 1s as BitsPerPixel, but shifting UINT32 by
+ // 32 is undefined, so work around that. BitsPerPixel isn't 0 so
+ // (BitsPerPixel - 1u) is ok.
+ //
+ PixelMask = ((0x1u << 1u) << (BitsPerPixel - 1u)) - 1u;
+ ModeInfo->PixelInformation.ReservedMask =
+ PixelMask & ~(RedMask | GreenMask | BlueMask);
+
+ ModeInfo->PixelsPerScanLine = BytesPerLine / (BitsPerPixel / 8);
+
+ ModeData ++;
+ ModeInfo ++;
+ }
+ VideoMode ++;
+ }
+ Private->MaxMode = ModeData - Private->ModeData;
+ return EFI_SUCCESS;
+
+Rollback:
+ FreePool(Private->VmwareSvgaModeInfo);
+ Private->VmwareSvgaModeInfo = NULL;
+
+ModeInfoAllocError:
+ FreePool(Private->ModeData);
+ Private->ModeData = NULL;
+
+ModeDataAllocError:
+ return Status;
+}
--
2.3.2 (Apple Git-55)
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 04/05/17 11:58, Phil Dennis-Jordan wrote:
> From: Phil Dennis-Jordan <phil@philjordan.eu>
>
> In addition to the QXL, Cirrus, etc. VGA adapters, Qemu also implements
> a basic version of VMWare's SVGA display device. Drivers for this
> device exist for some guest OSes which do not support Qemu's other
> display adapters, so supporting it in OVMF is useful in conjunction
> with those OSes.
>
> This change adds support for the SVGA device's framebuffer to
> QemuVideoDxe's graphics output protocol implementation, based on
> VMWare's documentation. The most basic initialisation, framebuffer
> layout query, and mode setting operations are implemented.
>
> The device relies on port-based 32-bit I/O, unfortunately on misaligned
> addresses. This limits the driver's support to the x86 family of
> platforms.
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>
> Notes:
> v2:
> - Unaligned I/O helper functions moved to separate commit [Laszlo]
> - Multi-line function call whitespace fixes.
>
> v3:
> - Dropped the "2" from "SVGA2" where appropriate. [Jordan, Laszlo]
> - Renamed various struct fields and functions with consistent prefixes [Laszlo]
> - #include orders fixed [Laszlo]
> - Renamedi/moved lots of local variables to comply with convention. [Laszlo]
> - Added error checking to PCI BAR queries. [Laszlo]
> - Moved some function definitions around for better grouping. [Laszlo]
> - Fixed ClearScreen() to use the correct VRAM BAR. [Laszlo]
> - Changed modelist initialisation to fetch all mode data on startup, so mode
> queries can return everything including channel masks without hitting the
> device. Mask calculations hopefully make more sense now. [Laszlo]
> - Whitespace fixes. [Laszlo]
> - Fixed a memory leak in BAR query.
>
> OvmfPkg/QemuVideoDxe/Qemu.h | 29 ++++
> OvmfPkg/QemuVideoDxe/Driver.c | 127 ++++++++++++++-
> OvmfPkg/QemuVideoDxe/Gop.c | 61 +++++++-
> OvmfPkg/QemuVideoDxe/Initialize.c | 161 ++++++++++++++++++++
> 4 files changed, 371 insertions(+), 7 deletions(-)
>
> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
> index 2ce37defc5b8..7fbb25b3efd3 100644
> --- a/OvmfPkg/QemuVideoDxe/Qemu.h
> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h
> @@ -92,6 +92,7 @@ typedef enum {
> QEMU_VIDEO_CIRRUS_5446,
> QEMU_VIDEO_BOCHS,
> QEMU_VIDEO_BOCHS_MMIO,
> + QEMU_VIDEO_VMWARE_SVGA,
> } QEMU_VIDEO_VARIANT;
>
> typedef struct {
> @@ -115,10 +116,13 @@ typedef struct {
> //
> UINTN MaxMode;
> QEMU_VIDEO_MODE_DATA *ModeData;
> + EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *VmwareSvgaModeInfo;
>
> QEMU_VIDEO_VARIANT Variant;
> FRAME_BUFFER_CONFIGURE *FrameBufferBltConfigure;
> UINTN FrameBufferBltConfigureSize;
> + UINT8 FrameBufferVramBarIndex;
> + UINT16 VmwareSvgaBasePort;
> } QEMU_VIDEO_PRIVATE_DATA;
>
> ///
> @@ -502,9 +506,34 @@ QemuVideoBochsModeSetup (
> BOOLEAN IsQxl
> );
>
> +EFI_STATUS
> +QemuVideoVmwareSvgaModeSetup (
> + QEMU_VIDEO_PRIVATE_DATA *Private
> + );
> +
> VOID
> InstallVbeShim (
> IN CONST CHAR16 *CardName,
> IN EFI_PHYSICAL_ADDRESS FrameBufferBase
> );
> +
> +VOID
> +VmwareSvgaWrite (
> + QEMU_VIDEO_PRIVATE_DATA *Private,
> + UINT16 Register,
> + UINT32 Value
> + );
> +
> +UINT32
> +VmwareSvgaRead (
> + QEMU_VIDEO_PRIVATE_DATA *Private,
> + UINT16 Register
> + );
> +
> +VOID
> +InitializeVmwareSvgaGraphicsMode (
> + QEMU_VIDEO_PRIVATE_DATA *Private,
> + QEMU_VIDEO_BOCHS_MODES *ModeData
> + );
> +
> #endif
> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> index fc8025ec46de..79c430e920d7 100644
> --- a/OvmfPkg/QemuVideoDxe/Driver.c
> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
> @@ -14,8 +14,10 @@
>
> **/
>
> -#include "Qemu.h"
> +#include <IndustryStandard/VmwareSvga.h>
> #include <IndustryStandard/Acpi.h>
> +#include "Qemu.h"
> +#include "UnalignedIoInternal.h"
>
> EFI_DRIVER_BINDING_PROTOCOL gQemuVideoDriverBinding = {
> QemuVideoControllerDriverSupported,
> @@ -58,6 +60,11 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = {
> QEMU_VIDEO_BOCHS_MMIO,
> L"QEMU VirtIO VGA"
> },{
> + VMWARE_PCI_VENDOR_ID_VMWARE,
> + VMWARE_PCI_DEVICE_ID_VMWARE_SVGA2,
> + QEMU_VIDEO_VMWARE_SVGA,
> + L"QEMU VMWare SVGA"
> + },{
> 0 /* end of list */
> }
> };
> @@ -242,6 +249,7 @@ QemuVideoControllerDriverStart (
> goto ClosePciIo;
> }
> Private->Variant = Card->Variant;
> + Private->FrameBufferVramBarIndex = PCI_BAR_IDX0;
>
> //
> // IsQxl is based on the detected Card->Variant, which at a later point might
> @@ -317,6 +325,59 @@ QemuVideoControllerDriverStart (
> }
>
> //
> + // Check if accessing Vmware SVGA interface works
> + //
> + if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *IoDesc;
> + UINT32 TargetId;
> + UINT32 SvgaIdRead;
> +
> + IoDesc = NULL;
> + Status = Private->PciIo->GetBarAttributes (
> + Private->PciIo,
> + PCI_BAR_IDX0,
> + NULL,
> + (VOID**) &IoDesc
> + );
> + if (EFI_ERROR(Status) ||
Missing space between "EFI_ERROR" and "(".
> + IoDesc->ResType != ACPI_ADDRESS_SPACE_TYPE_IO ||
> + IoDesc->AddrRangeMin >= MAX_UINT16 ||
> + IoDesc->AddrRangeMin + VMWARE_SVGA_VALUE_PORT >= MAX_UINT16) {
If we wanted to be exact, we would likely express the last two
conditions with a single
IoDesc->AddrRangeMin > MAX_UINT16 + 1 - (VMWARE_SVGA_VALUE_PORT + 4)
but I don't necessarily want to obsess about this.
> + if (IoDesc != NULL) {
> + FreePool (IoDesc);
> + }
Good catch :)
> + Status = EFI_DEVICE_ERROR;
> + goto RestoreAttributes;
> + }
> + Private->VmwareSvgaBasePort = (UINT16) IoDesc->AddrRangeMin;
> + FreePool (IoDesc);
Here too :)
> +
> + TargetId = VMWARE_SVGA_ID_2;
> + while (TRUE) {
> + VmwareSvgaWrite (Private, VmwareSvgaRegId, TargetId);
> + SvgaIdRead = VmwareSvgaRead (Private, VmwareSvgaRegId);
> + if ((SvgaIdRead == TargetId) || (TargetId <= VMWARE_SVGA_ID_0)) {
> + break;
> + }
> + TargetId --;
Hm, this change wasn't called for.
3.2.3 Formatting: Horizontal spacing
* Never put space between unary operators and the operand. See
"Horizontal Spacing".
5.2.2.3 Do not put space between unary operators and their object
> + }
> +
> + if (SvgaIdRead != TargetId) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "QemuVideo: QEMU_VIDEO_VMWARE_SVGA ID mismatch "
> + "(got 0x%x, base address 0x%x)\n",
> + SvgaIdRead,
> + Private->VmwareSvgaBasePort
> + ));
> + Status = EFI_DEVICE_ERROR;
> + goto RestoreAttributes;
> + }
> +
> + Private->FrameBufferVramBarIndex = PCI_BAR_IDX1;
> + }
> +
> + //
> // Get ParentDevicePath
> //
> Status = gBS->HandleProtocol (
> @@ -371,6 +432,9 @@ QemuVideoControllerDriverStart (
> case QEMU_VIDEO_BOCHS:
> Status = QemuVideoBochsModeSetup (Private, IsQxl);
> break;
> + case QEMU_VIDEO_VMWARE_SVGA:
> + Status = QemuVideoVmwareSvgaModeSetup (Private);
> + break;
> default:
> ASSERT (FALSE);
> Status = EFI_DEVICE_ERROR;
> @@ -750,7 +814,7 @@ ClearScreen (
> Private->PciIo->Mem.Write (
> Private->PciIo,
> EfiPciIoWidthFillUint32,
> - 0,
> + Private->FrameBufferVramBarIndex,
> 0,
> 0x400000 >> 2,
> &Color
> @@ -888,6 +952,35 @@ BochsRead (
> }
>
> VOID
> +VmwareSvgaWrite (
> + QEMU_VIDEO_PRIVATE_DATA *Private,
> + UINT16 Register,
> + UINT32 Value
> + )
> +{
> + UnalignedIoWrite32 (
> + Private->VmwareSvgaBasePort + VMWARE_SVGA_INDEX_PORT,
> + Register
> + );
> + UnalignedIoWrite32 (
> + Private->VmwareSvgaBasePort + VMWARE_SVGA_VALUE_PORT,
> + Value);
Please break the closing paren off to a new line.
> +}
> +
> +UINT32
> +VmwareSvgaRead (
> + QEMU_VIDEO_PRIVATE_DATA *Private,
> + UINT16 Register
> + )
> +{
> + UnalignedIoWrite32 (
> + Private->VmwareSvgaBasePort + VMWARE_SVGA_INDEX_PORT,
> + Register);
Please break the closing paren off to a new line.
> + return UnalignedIoRead32 (
> + Private->VmwareSvgaBasePort + VMWARE_SVGA_VALUE_PORT);
Please break the closing paren off to a new line.
> +}
> +
> +VOID
> VgaOutb (
> QEMU_VIDEO_PRIVATE_DATA *Private,
> UINTN Reg,
> @@ -941,6 +1034,35 @@ InitializeBochsGraphicsMode (
> ClearScreen (Private);
> }
>
> +VOID
> +InitializeVmwareSvgaGraphicsMode (
> + QEMU_VIDEO_PRIVATE_DATA *Private,
> + QEMU_VIDEO_BOCHS_MODES *ModeData
> + )
> +{
> + UINT32 Capabilities;
> +
> + VmwareSvgaWrite (Private, VmwareSvgaRegWidth, ModeData->Width);
> + VmwareSvgaWrite (Private, VmwareSvgaRegHeight, ModeData->Height);
> +
> + Capabilities = VmwareSvgaRead (
> + Private,
> + VmwareSvgaRegCapabilities
> + );
> + if ((Capabilities & VMWARE_SVGA_CAP_8BIT_EMULATION) != 0) {
> + VmwareSvgaWrite (
> + Private,
> + VmwareSvgaRegBitsPerPixel,
> + ModeData->ColorDepth
> + );
> + }
> +
> + VmwareSvgaWrite (Private, VmwareSvgaRegEnable, 1);
> +
> + SetDefaultPalette (Private);
> + ClearScreen (Private);
> +}
> +
> EFI_STATUS
> EFIAPI
> InitializeQemuVideo (
> @@ -975,3 +1097,4 @@ InitializeQemuVideo (
>
> return Status;
> }
> +
No need to add an empty line here.
> diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
> index 359e9217d3d1..934c9f39b459 100644
> --- a/OvmfPkg/QemuVideoDxe/Gop.c
> +++ b/OvmfPkg/QemuVideoDxe/Gop.c
> @@ -13,6 +13,7 @@
>
> **/
>
> +#include <IndustryStandard/VmwareSvga.h>
> #include "Qemu.h"
>
> STATIC
> @@ -75,6 +76,42 @@ QemuVideoCompleteModeData (
> return EFI_SUCCESS;
> }
>
> +STATIC
> +EFI_STATUS
> +QemuVideoVmwareSvgaCompleteModeData (
Huh, thanks for making this STATIC :)
> + IN QEMU_VIDEO_PRIVATE_DATA *Private,
> + OUT EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode
> + )
> +{
> + EFI_STATUS Status;
> + EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info;
> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *FrameBufDesc;
> + UINT32 BytesPerLine, FbOffset, BytesPerPixel;
> +
> + Info = Mode->Info;
> + CopyMem (Info, &Private->VmwareSvgaModeInfo[Mode->Mode], sizeof(*Info));
Please write "sizeof (*Info)" or "sizeof *Info".
> + BytesPerPixel = Private->ModeData[Mode->Mode].ColorDepth / 8;
> + BytesPerLine = Info->PixelsPerScanLine * BytesPerPixel;
> +
> + FbOffset = VmwareSvgaRead (Private, VmwareSvgaRegFbOffset);
> +
> + Status = Private->PciIo->GetBarAttributes (
> + Private->PciIo,
> + PCI_BAR_IDX1,
> + NULL,
> + (VOID**) &FrameBufDesc
> + );
> + if (EFI_ERROR(Status)) {
Missing space between "EFI_ERROR" and "(".
> + return EFI_DEVICE_ERROR;
> + }
> +
> + Mode->FrameBufferBase = FrameBufDesc->AddrRangeMin + FbOffset;
> + Mode->FrameBufferSize = BytesPerLine * Info->VerticalResolution;
> +
> + FreePool (FrameBufDesc);
> + return Status;
> +}
> +
>
> //
> // Graphics Output Protocol Member Functions
> @@ -124,10 +161,14 @@ Routine Description:
>
> *SizeOfInfo = sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
>
> - ModeData = &Private->ModeData[ModeNumber];
> - (*Info)->HorizontalResolution = ModeData->HorizontalResolution;
> - (*Info)->VerticalResolution = ModeData->VerticalResolution;
> - QemuVideoCompleteModeInfo (ModeData, *Info);
> + if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
> + **Info = Private->VmwareSvgaModeInfo[ModeNumber];
In edk2, structure assignment is not allowed (because some compilers
cannot be prevented from generating intrinsics for them). Please use an
explicit CopyMem (), like you do above in
QemuVideoVmwareSvgaCompleteModeData ().
> + } else {
> + ModeData = &Private->ModeData[ModeNumber];
> + (*Info)->HorizontalResolution = ModeData->HorizontalResolution;
> + (*Info)->VerticalResolution = ModeData->VerticalResolution;
> + QemuVideoCompleteModeInfo (ModeData, *Info);
> + }
>
> return EFI_SUCCESS;
> }
> @@ -176,6 +217,12 @@ Routine Description:
> case QEMU_VIDEO_BOCHS:
> InitializeBochsGraphicsMode (Private, &QemuVideoBochsModes[ModeData->InternalModeIndex]);
> break;
> + case QEMU_VIDEO_VMWARE_SVGA:
> + InitializeVmwareSvgaGraphicsMode (
> + Private,
> + &QemuVideoBochsModes[ModeData->InternalModeIndex]
> + );
> + break;
> default:
> ASSERT (FALSE);
> return EFI_DEVICE_ERROR;
> @@ -186,7 +233,11 @@ Routine Description:
> This->Mode->Info->VerticalResolution = ModeData->VerticalResolution;
> This->Mode->SizeOfInfo = sizeof(EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
>
> - QemuVideoCompleteModeData (Private, This->Mode);
> + if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
> + QemuVideoVmwareSvgaCompleteModeData (Private, This->Mode);
> + } else {
> + QemuVideoCompleteModeData (Private, This->Mode);
> + }
>
> //
> // Re-initialize the frame buffer configure when mode changes.
> diff --git a/OvmfPkg/QemuVideoDxe/Initialize.c b/OvmfPkg/QemuVideoDxe/Initialize.c
> index d5d8cfef9661..8c5c993c7d0e 100644
> --- a/OvmfPkg/QemuVideoDxe/Initialize.c
> +++ b/OvmfPkg/QemuVideoDxe/Initialize.c
> @@ -13,6 +13,7 @@
>
> **/
>
> +#include <IndustryStandard/VmwareSvga.h>
> #include "Qemu.h"
>
>
> @@ -346,3 +347,163 @@ QemuVideoBochsModeSetup (
> return EFI_SUCCESS;
> }
>
> +EFI_STATUS
> +QemuVideoVmwareSvgaModeSetup (
> + QEMU_VIDEO_PRIVATE_DATA *Private
> + )
> +{
> + EFI_STATUS Status;
> + UINT32 FbSize;
> + UINT32 MaxWidth, MaxHeight;
> + UINT32 Capabilities;
> + UINT32 BitsPerPixel;
> + UINT32 Index;
> + QEMU_VIDEO_MODE_DATA *ModeData;
> + QEMU_VIDEO_BOCHS_MODES *VideoMode;
> + EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *ModeInfo;
> +
> + VmwareSvgaWrite (Private, VmwareSvgaRegEnable, 0);
> +
> + Private->ModeData =
> + AllocatePool (sizeof (Private->ModeData[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT);
> + if (Private->ModeData == NULL) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto ModeDataAllocError;
> + }
> +
> + Private->VmwareSvgaModeInfo =
> + AllocatePool (
> + sizeof (Private->VmwareSvgaModeInfo[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT
> + );
> + if (Private->VmwareSvgaModeInfo == NULL) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto ModeInfoAllocError;
> + }
> +
> + FbSize = VmwareSvgaRead (Private, VmwareSvgaRegFbSize);
> + MaxWidth = VmwareSvgaRead (Private, VmwareSvgaRegMaxWidth);
> + MaxHeight = VmwareSvgaRead (Private, VmwareSvgaRegMaxHeight);
> + Capabilities = VmwareSvgaRead (Private, VmwareSvgaRegCapabilities);
> + if ((Capabilities & VMWARE_SVGA_CAP_8BIT_EMULATION) != 0) {
> + BitsPerPixel = VmwareSvgaRead (
> + Private,
> + VmwareSvgaRegHostBitsPerPixel
> + );
> + VmwareSvgaWrite (
> + Private,
> + VmwareSvgaRegBitsPerPixel,
> + BitsPerPixel
> + );
> + } else {
> + BitsPerPixel = VmwareSvgaRead (
> + Private,
> + VmwareSvgaRegBitsPerPixel
> + );
> + }
> +
> + if (FbSize == 0 ||
> + MaxWidth == 0 ||
> + MaxHeight == 0 ||
> + BitsPerPixel == 0 ||
> + BitsPerPixel % 8 != 0) {
> + Status = EFI_DEVICE_ERROR;
> + goto Rollback;
> + }
> +
> + ModeData = Private->ModeData;
> + ModeInfo = Private->VmwareSvgaModeInfo;
> + VideoMode = &QemuVideoBochsModes[0];
> + for (Index = 0; Index < QEMU_VIDEO_BOCHS_MODE_COUNT; Index ++) {
Hm, yeah, I see this comes from existing code, but please drop the " "
in "Index ++".
> + UINTN RequiredFbSize;
> +
> + RequiredFbSize = (UINTN) VideoMode->Width * VideoMode->Height *
> + (BitsPerPixel / 8);
> + if (RequiredFbSize <= FbSize &&
> + VideoMode->Width <= MaxWidth &&
> + VideoMode->Height <= MaxHeight) {
> + UINT32 BytesPerLine;
> + UINT32 RedMask, GreenMask, BlueMask, PixelMask;
> +
> + VmwareSvgaWrite (
> + Private,
> + VmwareSvgaRegWidth,
> + VideoMode->Width
> + );
> + VmwareSvgaWrite (
> + Private,
> + VmwareSvgaRegHeight,
> + VideoMode->Height
> + );
> + BytesPerLine = VmwareSvgaRead (
> + Private,
> + VmwareSvgaRegBytesPerLine
> + );
> +
> + ModeData->InternalModeIndex = Index;
> + ModeData->HorizontalResolution = VideoMode->Width;
> + ModeData->VerticalResolution = VideoMode->Height;
> + ModeData->ColorDepth = BitsPerPixel;
> +
> + //
> + // Setting VmwareSvgaRegWidth/VmwareSvgaRegHeight actually changes
> + // the device's display mode, so we all properties of each mode up front
I think you accidentally the verb :)
> + // to avoid inadvertent mode changes later.
> + //
> + ModeInfo->Version = 0;
> + ModeInfo->HorizontalResolution = ModeData->HorizontalResolution;
> + ModeInfo->VerticalResolution = ModeData->VerticalResolution;
> +
> + ModeInfo->PixelFormat = PixelBitMask;
> +
> + RedMask = VmwareSvgaRead (Private, VmwareSvgaRegRedMask);
> + ModeInfo->PixelInformation.RedMask = RedMask;
> +
> + GreenMask = VmwareSvgaRead (Private, VmwareSvgaRegGreenMask);
> + ModeInfo->PixelInformation.GreenMask = GreenMask;
> +
> + BlueMask = VmwareSvgaRead (Private, VmwareSvgaRegBlueMask);
> + ModeInfo->PixelInformation.BlueMask = BlueMask;
> +
> + BytesPerLine = VmwareSvgaRead (Private, VmwareSvgaRegBytesPerLine);
> +
> + if (BitsPerPixel == 32) {
> + if (BlueMask == 0xff && GreenMask == 0xff00 && RedMask == 0xff0000) {
> + ModeInfo->PixelFormat = PixelBlueGreenRedReserved8BitPerColor;
> + } else if (BlueMask == 0xff0000 &&
> + GreenMask == 0xff00 &&
> + RedMask == 0xff) {
> + ModeInfo->PixelFormat = PixelRedGreenBlueReserved8BitPerColor;
> + }
> + }
> +
> + //
> + // Reserved mask is whatever bits in the pixel are not used for RGB.
> + // PixelMask is as many binary 1s as BitsPerPixel, but shifting UINT32 by
> + // 32 is undefined, so work around that. BitsPerPixel isn't 0 so
> + // (BitsPerPixel - 1u) is ok.
> + //
Ah, OK. I've used this elsewhere, just in reverse order (first shift by
variable count - 1, then by 1). However, Jordan disliked the trick, and
ultimately we replaced it with a simple "if".
In fact, above you already have a (BitsPerPixel == 32) check. At the end
of that block, you could set PixelMask to MAX_UINT32 explicitly. And,
you could add an "else" branch, simply with
PixelMask = (1u << (BitsPerPixel - 1)) - 1;
What do you think?
> + PixelMask = ((0x1u << 1u) << (BitsPerPixel - 1u)) - 1u;
> + ModeInfo->PixelInformation.ReservedMask =
> + PixelMask & ~(RedMask | GreenMask | BlueMask);
> +
> + ModeInfo->PixelsPerScanLine = BytesPerLine / (BitsPerPixel / 8);
> +
> + ModeData ++;
> + ModeInfo ++;
> + }
> + VideoMode ++;
Please drop all the spaces before "++".
> + }
> + Private->MaxMode = ModeData - Private->ModeData;
> + return EFI_SUCCESS;
> +
> +Rollback:
> + FreePool(Private->VmwareSvgaModeInfo);
Missing space between "FreePool" and "(".
> + Private->VmwareSvgaModeInfo = NULL;
> +
> +ModeInfoAllocError:
> + FreePool(Private->ModeData);
Missing space between "FreePool" and "(".
> + Private->ModeData = NULL;
> +
> +ModeDataAllocError:
> + return Status;
> +}
>
I think this is a very good update. All my fresh comments have been
cosmetic. Please submit v4; I expect it to be the final version.
Thank you,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On Wed, Apr 5, 2017 at 11:41 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/05/17 11:58, Phil Dennis-Jordan wrote:
>> From: Phil Dennis-Jordan <phil@philjordan.eu>
>>
>> In addition to the QXL, Cirrus, etc. VGA adapters, Qemu also implements
>> a basic version of VMWare's SVGA display device. Drivers for this
>> device exist for some guest OSes which do not support Qemu's other
>> display adapters, so supporting it in OVMF is useful in conjunction
>> with those OSes.
>>
>> This change adds support for the SVGA device's framebuffer to
>> QemuVideoDxe's graphics output protocol implementation, based on
>> VMWare's documentation. The most basic initialisation, framebuffer
>> layout query, and mode setting operations are implemented.
>>
>> The device relies on port-based 32-bit I/O, unfortunately on misaligned
>> addresses. This limits the driver's support to the x86 family of
>> platforms.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
>> ---
>>
>> Notes:
>> v2:
>> - Unaligned I/O helper functions moved to separate commit [Laszlo]
>> - Multi-line function call whitespace fixes.
>>
>> v3:
>> - Dropped the "2" from "SVGA2" where appropriate. [Jordan, Laszlo]
>> - Renamed various struct fields and functions with consistent prefixes [Laszlo]
>> - #include orders fixed [Laszlo]
>> - Renamedi/moved lots of local variables to comply with convention. [Laszlo]
>> - Added error checking to PCI BAR queries. [Laszlo]
>> - Moved some function definitions around for better grouping. [Laszlo]
>> - Fixed ClearScreen() to use the correct VRAM BAR. [Laszlo]
>> - Changed modelist initialisation to fetch all mode data on startup, so mode
>> queries can return everything including channel masks without hitting the
>> device. Mask calculations hopefully make more sense now. [Laszlo]
>> - Whitespace fixes. [Laszlo]
>> - Fixed a memory leak in BAR query.
>>
>> OvmfPkg/QemuVideoDxe/Qemu.h | 29 ++++
>> OvmfPkg/QemuVideoDxe/Driver.c | 127 ++++++++++++++-
>> OvmfPkg/QemuVideoDxe/Gop.c | 61 +++++++-
>> OvmfPkg/QemuVideoDxe/Initialize.c | 161 ++++++++++++++++++++
>> 4 files changed, 371 insertions(+), 7 deletions(-)
>>
>> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
>> index 2ce37defc5b8..7fbb25b3efd3 100644
>> --- a/OvmfPkg/QemuVideoDxe/Qemu.h
>> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h
>> @@ -92,6 +92,7 @@ typedef enum {
>> QEMU_VIDEO_CIRRUS_5446,
>> QEMU_VIDEO_BOCHS,
>> QEMU_VIDEO_BOCHS_MMIO,
>> + QEMU_VIDEO_VMWARE_SVGA,
>> } QEMU_VIDEO_VARIANT;
>>
>> typedef struct {
>> @@ -115,10 +116,13 @@ typedef struct {
>> //
>> UINTN MaxMode;
>> QEMU_VIDEO_MODE_DATA *ModeData;
>> + EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *VmwareSvgaModeInfo;
>>
>> QEMU_VIDEO_VARIANT Variant;
>> FRAME_BUFFER_CONFIGURE *FrameBufferBltConfigure;
>> UINTN FrameBufferBltConfigureSize;
>> + UINT8 FrameBufferVramBarIndex;
>> + UINT16 VmwareSvgaBasePort;
>> } QEMU_VIDEO_PRIVATE_DATA;
>>
>> ///
>> @@ -502,9 +506,34 @@ QemuVideoBochsModeSetup (
>> BOOLEAN IsQxl
>> );
>>
>> +EFI_STATUS
>> +QemuVideoVmwareSvgaModeSetup (
>> + QEMU_VIDEO_PRIVATE_DATA *Private
>> + );
>> +
>> VOID
>> InstallVbeShim (
>> IN CONST CHAR16 *CardName,
>> IN EFI_PHYSICAL_ADDRESS FrameBufferBase
>> );
>> +
>> +VOID
>> +VmwareSvgaWrite (
>> + QEMU_VIDEO_PRIVATE_DATA *Private,
>> + UINT16 Register,
>> + UINT32 Value
>> + );
>> +
>> +UINT32
>> +VmwareSvgaRead (
>> + QEMU_VIDEO_PRIVATE_DATA *Private,
>> + UINT16 Register
>> + );
>> +
>> +VOID
>> +InitializeVmwareSvgaGraphicsMode (
>> + QEMU_VIDEO_PRIVATE_DATA *Private,
>> + QEMU_VIDEO_BOCHS_MODES *ModeData
>> + );
>> +
>> #endif
>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
>> index fc8025ec46de..79c430e920d7 100644
>> --- a/OvmfPkg/QemuVideoDxe/Driver.c
>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
>> @@ -14,8 +14,10 @@
>>
>> **/
>>
>> -#include "Qemu.h"
>> +#include <IndustryStandard/VmwareSvga.h>
>> #include <IndustryStandard/Acpi.h>
>> +#include "Qemu.h"
>> +#include "UnalignedIoInternal.h"
>>
>> EFI_DRIVER_BINDING_PROTOCOL gQemuVideoDriverBinding = {
>> QemuVideoControllerDriverSupported,
>> @@ -58,6 +60,11 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = {
>> QEMU_VIDEO_BOCHS_MMIO,
>> L"QEMU VirtIO VGA"
>> },{
>> + VMWARE_PCI_VENDOR_ID_VMWARE,
>> + VMWARE_PCI_DEVICE_ID_VMWARE_SVGA2,
>> + QEMU_VIDEO_VMWARE_SVGA,
>> + L"QEMU VMWare SVGA"
>> + },{
>> 0 /* end of list */
>> }
>> };
>> @@ -242,6 +249,7 @@ QemuVideoControllerDriverStart (
>> goto ClosePciIo;
>> }
>> Private->Variant = Card->Variant;
>> + Private->FrameBufferVramBarIndex = PCI_BAR_IDX0;
>>
>> //
>> // IsQxl is based on the detected Card->Variant, which at a later point might
>> @@ -317,6 +325,59 @@ QemuVideoControllerDriverStart (
>> }
>>
>> //
>> + // Check if accessing Vmware SVGA interface works
>> + //
>> + if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
>> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *IoDesc;
>> + UINT32 TargetId;
>> + UINT32 SvgaIdRead;
>> +
>> + IoDesc = NULL;
>> + Status = Private->PciIo->GetBarAttributes (
>> + Private->PciIo,
>> + PCI_BAR_IDX0,
>> + NULL,
>> + (VOID**) &IoDesc
>> + );
>> + if (EFI_ERROR(Status) ||
>
> Missing space between "EFI_ERROR" and "(".
Augh, sorry about these, old habits die hard.
>> + IoDesc->ResType != ACPI_ADDRESS_SPACE_TYPE_IO ||
>> + IoDesc->AddrRangeMin >= MAX_UINT16 ||
>> + IoDesc->AddrRangeMin + VMWARE_SVGA_VALUE_PORT >= MAX_UINT16) {
>
> If we wanted to be exact, we would likely express the last two
> conditions with a single
>
> IoDesc->AddrRangeMin > MAX_UINT16 + 1 - (VMWARE_SVGA_VALUE_PORT + 4)
>
> but I don't necessarily want to obsess about this.
The details of this, >/+1 vs >=, and +4 vs not +4, etc. are probably
debatable, but yes, there's no need (chance of overflow) to make this
2 separate conditions. I've gone with your version.
>> + if (IoDesc != NULL) {
>> + FreePool (IoDesc);
>> + }
>
> Good catch :)
>
>> + Status = EFI_DEVICE_ERROR;
>> + goto RestoreAttributes;
>> + }
>> + Private->VmwareSvgaBasePort = (UINT16) IoDesc->AddrRangeMin;
>> + FreePool (IoDesc);
>
> Here too :)
>
>> +
>> + TargetId = VMWARE_SVGA_ID_2;
>> + while (TRUE) {
>> + VmwareSvgaWrite (Private, VmwareSvgaRegId, TargetId);
>> + SvgaIdRead = VmwareSvgaRead (Private, VmwareSvgaRegId);
>> + if ((SvgaIdRead == TargetId) || (TargetId <= VMWARE_SVGA_ID_0)) {
>> + break;
>> + }
>> + TargetId --;
>
> Hm, this change wasn't called for.
>
> 3.2.3 Formatting: Horizontal spacing
>
> * Never put space between unary operators and the operand. See
> "Horizontal Spacing".
>
> 5.2.2.3 Do not put space between unary operators and their object
>
>> + }
>> +
>> + if (SvgaIdRead != TargetId) {
>> + DEBUG ((
>> + DEBUG_ERROR,
>> + "QemuVideo: QEMU_VIDEO_VMWARE_SVGA ID mismatch "
>> + "(got 0x%x, base address 0x%x)\n",
>> + SvgaIdRead,
>> + Private->VmwareSvgaBasePort
>> + ));
>> + Status = EFI_DEVICE_ERROR;
>> + goto RestoreAttributes;
>> + }
>> +
>> + Private->FrameBufferVramBarIndex = PCI_BAR_IDX1;
>> + }
>> +
>> + //
>> // Get ParentDevicePath
>> //
>> Status = gBS->HandleProtocol (
>> @@ -371,6 +432,9 @@ QemuVideoControllerDriverStart (
>> case QEMU_VIDEO_BOCHS:
>> Status = QemuVideoBochsModeSetup (Private, IsQxl);
>> break;
>> + case QEMU_VIDEO_VMWARE_SVGA:
>> + Status = QemuVideoVmwareSvgaModeSetup (Private);
>> + break;
>> default:
>> ASSERT (FALSE);
>> Status = EFI_DEVICE_ERROR;
>> @@ -750,7 +814,7 @@ ClearScreen (
>> Private->PciIo->Mem.Write (
>> Private->PciIo,
>> EfiPciIoWidthFillUint32,
>> - 0,
>> + Private->FrameBufferVramBarIndex,
>> 0,
>> 0x400000 >> 2,
>> &Color
>> @@ -888,6 +952,35 @@ BochsRead (
>> }
>>
>> VOID
>> +VmwareSvgaWrite (
>> + QEMU_VIDEO_PRIVATE_DATA *Private,
>> + UINT16 Register,
>> + UINT32 Value
>> + )
>> +{
>> + UnalignedIoWrite32 (
>> + Private->VmwareSvgaBasePort + VMWARE_SVGA_INDEX_PORT,
>> + Register
>> + );
>> + UnalignedIoWrite32 (
>> + Private->VmwareSvgaBasePort + VMWARE_SVGA_VALUE_PORT,
>> + Value);
>
> Please break the closing paren off to a new line.
>
>> +}
>> +
>> +UINT32
>> +VmwareSvgaRead (
>> + QEMU_VIDEO_PRIVATE_DATA *Private,
>> + UINT16 Register
>> + )
>> +{
>> + UnalignedIoWrite32 (
>> + Private->VmwareSvgaBasePort + VMWARE_SVGA_INDEX_PORT,
>> + Register);
>
> Please break the closing paren off to a new line.
>
>> + return UnalignedIoRead32 (
>> + Private->VmwareSvgaBasePort + VMWARE_SVGA_VALUE_PORT);
>
> Please break the closing paren off to a new line.
>
>> +}
>> +
>> +VOID
>> VgaOutb (
>> QEMU_VIDEO_PRIVATE_DATA *Private,
>> UINTN Reg,
>> @@ -941,6 +1034,35 @@ InitializeBochsGraphicsMode (
>> ClearScreen (Private);
>> }
>>
>> +VOID
>> +InitializeVmwareSvgaGraphicsMode (
>> + QEMU_VIDEO_PRIVATE_DATA *Private,
>> + QEMU_VIDEO_BOCHS_MODES *ModeData
>> + )
>> +{
>> + UINT32 Capabilities;
>> +
>> + VmwareSvgaWrite (Private, VmwareSvgaRegWidth, ModeData->Width);
>> + VmwareSvgaWrite (Private, VmwareSvgaRegHeight, ModeData->Height);
>> +
>> + Capabilities = VmwareSvgaRead (
>> + Private,
>> + VmwareSvgaRegCapabilities
>> + );
>> + if ((Capabilities & VMWARE_SVGA_CAP_8BIT_EMULATION) != 0) {
>> + VmwareSvgaWrite (
>> + Private,
>> + VmwareSvgaRegBitsPerPixel,
>> + ModeData->ColorDepth
>> + );
>> + }
>> +
>> + VmwareSvgaWrite (Private, VmwareSvgaRegEnable, 1);
>> +
>> + SetDefaultPalette (Private);
>> + ClearScreen (Private);
>> +}
>> +
>> EFI_STATUS
>> EFIAPI
>> InitializeQemuVideo (
>> @@ -975,3 +1097,4 @@ InitializeQemuVideo (
>>
>> return Status;
>> }
>> +
>
> No need to add an empty line here.
Whoops, leftover from moved function.
>> diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
>> index 359e9217d3d1..934c9f39b459 100644
>> --- a/OvmfPkg/QemuVideoDxe/Gop.c
>> +++ b/OvmfPkg/QemuVideoDxe/Gop.c
>> @@ -13,6 +13,7 @@
>>
>> **/
>>
>> +#include <IndustryStandard/VmwareSvga.h>
>> #include "Qemu.h"
>>
>> STATIC
>> @@ -75,6 +76,42 @@ QemuVideoCompleteModeData (
>> return EFI_SUCCESS;
>> }
>>
>> +STATIC
>> +EFI_STATUS
>> +QemuVideoVmwareSvgaCompleteModeData (
>
> Huh, thanks for making this STATIC :)
>
>> + IN QEMU_VIDEO_PRIVATE_DATA *Private,
>> + OUT EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode
>> + )
>> +{
>> + EFI_STATUS Status;
>> + EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info;
>> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *FrameBufDesc;
>> + UINT32 BytesPerLine, FbOffset, BytesPerPixel;
>> +
>> + Info = Mode->Info;
>> + CopyMem (Info, &Private->VmwareSvgaModeInfo[Mode->Mode], sizeof(*Info));
>
> Please write "sizeof (*Info)" or "sizeof *Info".
>
>> + BytesPerPixel = Private->ModeData[Mode->Mode].ColorDepth / 8;
>> + BytesPerLine = Info->PixelsPerScanLine * BytesPerPixel;
>> +
>> + FbOffset = VmwareSvgaRead (Private, VmwareSvgaRegFbOffset);
>> +
>> + Status = Private->PciIo->GetBarAttributes (
>> + Private->PciIo,
>> + PCI_BAR_IDX1,
>> + NULL,
>> + (VOID**) &FrameBufDesc
>> + );
>> + if (EFI_ERROR(Status)) {
>
> Missing space between "EFI_ERROR" and "(".
>
>> + return EFI_DEVICE_ERROR;
>> + }
>> +
>> + Mode->FrameBufferBase = FrameBufDesc->AddrRangeMin + FbOffset;
>> + Mode->FrameBufferSize = BytesPerLine * Info->VerticalResolution;
>> +
>> + FreePool (FrameBufDesc);
>> + return Status;
>> +}
>> +
>>
>> //
>> // Graphics Output Protocol Member Functions
>> @@ -124,10 +161,14 @@ Routine Description:
>>
>> *SizeOfInfo = sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
>>
>> - ModeData = &Private->ModeData[ModeNumber];
>> - (*Info)->HorizontalResolution = ModeData->HorizontalResolution;
>> - (*Info)->VerticalResolution = ModeData->VerticalResolution;
>> - QemuVideoCompleteModeInfo (ModeData, *Info);
>> + if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
>> + **Info = Private->VmwareSvgaModeInfo[ModeNumber];
>
> In edk2, structure assignment is not allowed (because some compilers
> cannot be prevented from generating intrinsics for them). Please use an
> explicit CopyMem (), like you do above in
> QemuVideoVmwareSvgaCompleteModeData ().
>
>> + } else {
>> + ModeData = &Private->ModeData[ModeNumber];
>> + (*Info)->HorizontalResolution = ModeData->HorizontalResolution;
>> + (*Info)->VerticalResolution = ModeData->VerticalResolution;
>> + QemuVideoCompleteModeInfo (ModeData, *Info);
>> + }
>>
>> return EFI_SUCCESS;
>> }
>> @@ -176,6 +217,12 @@ Routine Description:
>> case QEMU_VIDEO_BOCHS:
>> InitializeBochsGraphicsMode (Private, &QemuVideoBochsModes[ModeData->InternalModeIndex]);
>> break;
>> + case QEMU_VIDEO_VMWARE_SVGA:
>> + InitializeVmwareSvgaGraphicsMode (
>> + Private,
>> + &QemuVideoBochsModes[ModeData->InternalModeIndex]
>> + );
>> + break;
>> default:
>> ASSERT (FALSE);
>> return EFI_DEVICE_ERROR;
>> @@ -186,7 +233,11 @@ Routine Description:
>> This->Mode->Info->VerticalResolution = ModeData->VerticalResolution;
>> This->Mode->SizeOfInfo = sizeof(EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
>>
>> - QemuVideoCompleteModeData (Private, This->Mode);
>> + if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
>> + QemuVideoVmwareSvgaCompleteModeData (Private, This->Mode);
>> + } else {
>> + QemuVideoCompleteModeData (Private, This->Mode);
>> + }
>>
>> //
>> // Re-initialize the frame buffer configure when mode changes.
>> diff --git a/OvmfPkg/QemuVideoDxe/Initialize.c b/OvmfPkg/QemuVideoDxe/Initialize.c
>> index d5d8cfef9661..8c5c993c7d0e 100644
>> --- a/OvmfPkg/QemuVideoDxe/Initialize.c
>> +++ b/OvmfPkg/QemuVideoDxe/Initialize.c
>> @@ -13,6 +13,7 @@
>>
>> **/
>>
>> +#include <IndustryStandard/VmwareSvga.h>
>> #include "Qemu.h"
>>
>>
>> @@ -346,3 +347,163 @@ QemuVideoBochsModeSetup (
>> return EFI_SUCCESS;
>> }
>>
>> +EFI_STATUS
>> +QemuVideoVmwareSvgaModeSetup (
>> + QEMU_VIDEO_PRIVATE_DATA *Private
>> + )
>> +{
>> + EFI_STATUS Status;
>> + UINT32 FbSize;
>> + UINT32 MaxWidth, MaxHeight;
>> + UINT32 Capabilities;
>> + UINT32 BitsPerPixel;
>> + UINT32 Index;
>> + QEMU_VIDEO_MODE_DATA *ModeData;
>> + QEMU_VIDEO_BOCHS_MODES *VideoMode;
>> + EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *ModeInfo;
>> +
>> + VmwareSvgaWrite (Private, VmwareSvgaRegEnable, 0);
>> +
>> + Private->ModeData =
>> + AllocatePool (sizeof (Private->ModeData[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT);
>> + if (Private->ModeData == NULL) {
>> + Status = EFI_OUT_OF_RESOURCES;
>> + goto ModeDataAllocError;
>> + }
>> +
>> + Private->VmwareSvgaModeInfo =
>> + AllocatePool (
>> + sizeof (Private->VmwareSvgaModeInfo[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT
>> + );
>> + if (Private->VmwareSvgaModeInfo == NULL) {
>> + Status = EFI_OUT_OF_RESOURCES;
>> + goto ModeInfoAllocError;
>> + }
>> +
>> + FbSize = VmwareSvgaRead (Private, VmwareSvgaRegFbSize);
>> + MaxWidth = VmwareSvgaRead (Private, VmwareSvgaRegMaxWidth);
>> + MaxHeight = VmwareSvgaRead (Private, VmwareSvgaRegMaxHeight);
>> + Capabilities = VmwareSvgaRead (Private, VmwareSvgaRegCapabilities);
>> + if ((Capabilities & VMWARE_SVGA_CAP_8BIT_EMULATION) != 0) {
>> + BitsPerPixel = VmwareSvgaRead (
>> + Private,
>> + VmwareSvgaRegHostBitsPerPixel
>> + );
>> + VmwareSvgaWrite (
>> + Private,
>> + VmwareSvgaRegBitsPerPixel,
>> + BitsPerPixel
>> + );
>> + } else {
>> + BitsPerPixel = VmwareSvgaRead (
>> + Private,
>> + VmwareSvgaRegBitsPerPixel
>> + );
>> + }
>> +
>> + if (FbSize == 0 ||
>> + MaxWidth == 0 ||
>> + MaxHeight == 0 ||
>> + BitsPerPixel == 0 ||
>> + BitsPerPixel % 8 != 0) {
>> + Status = EFI_DEVICE_ERROR;
>> + goto Rollback;
>> + }
>> +
>> + ModeData = Private->ModeData;
>> + ModeInfo = Private->VmwareSvgaModeInfo;
>> + VideoMode = &QemuVideoBochsModes[0];
>> + for (Index = 0; Index < QEMU_VIDEO_BOCHS_MODE_COUNT; Index ++) {
>
> Hm, yeah, I see this comes from existing code, but please drop the " "
> in "Index ++".
>
>> + UINTN RequiredFbSize;
>> +
>> + RequiredFbSize = (UINTN) VideoMode->Width * VideoMode->Height *
>> + (BitsPerPixel / 8);
>> + if (RequiredFbSize <= FbSize &&
>> + VideoMode->Width <= MaxWidth &&
>> + VideoMode->Height <= MaxHeight) {
>> + UINT32 BytesPerLine;
>> + UINT32 RedMask, GreenMask, BlueMask, PixelMask;
>> +
>> + VmwareSvgaWrite (
>> + Private,
>> + VmwareSvgaRegWidth,
>> + VideoMode->Width
>> + );
>> + VmwareSvgaWrite (
>> + Private,
>> + VmwareSvgaRegHeight,
>> + VideoMode->Height
>> + );
>> + BytesPerLine = VmwareSvgaRead (
>> + Private,
>> + VmwareSvgaRegBytesPerLine
>> + );
>> +
>> + ModeData->InternalModeIndex = Index;
>> + ModeData->HorizontalResolution = VideoMode->Width;
>> + ModeData->VerticalResolution = VideoMode->Height;
>> + ModeData->ColorDepth = BitsPerPixel;
>> +
>> + //
>> + // Setting VmwareSvgaRegWidth/VmwareSvgaRegHeight actually changes
>> + // the device's display mode, so we all properties of each mode up front
>
> I think you accidentally the verb :)
>
>> + // to avoid inadvertent mode changes later.
>> + //
>> + ModeInfo->Version = 0;
>> + ModeInfo->HorizontalResolution = ModeData->HorizontalResolution;
>> + ModeInfo->VerticalResolution = ModeData->VerticalResolution;
>> +
>> + ModeInfo->PixelFormat = PixelBitMask;
>> +
>> + RedMask = VmwareSvgaRead (Private, VmwareSvgaRegRedMask);
>> + ModeInfo->PixelInformation.RedMask = RedMask;
>> +
>> + GreenMask = VmwareSvgaRead (Private, VmwareSvgaRegGreenMask);
>> + ModeInfo->PixelInformation.GreenMask = GreenMask;
>> +
>> + BlueMask = VmwareSvgaRead (Private, VmwareSvgaRegBlueMask);
>> + ModeInfo->PixelInformation.BlueMask = BlueMask;
>> +
>> + BytesPerLine = VmwareSvgaRead (Private, VmwareSvgaRegBytesPerLine);
>> +
>> + if (BitsPerPixel == 32) {
>> + if (BlueMask == 0xff && GreenMask == 0xff00 && RedMask == 0xff0000) {
>> + ModeInfo->PixelFormat = PixelBlueGreenRedReserved8BitPerColor;
>> + } else if (BlueMask == 0xff0000 &&
>> + GreenMask == 0xff00 &&
>> + RedMask == 0xff) {
>> + ModeInfo->PixelFormat = PixelRedGreenBlueReserved8BitPerColor;
>> + }
>> + }
>> +
>> + //
>> + // Reserved mask is whatever bits in the pixel are not used for RGB.
>> + // PixelMask is as many binary 1s as BitsPerPixel, but shifting UINT32 by
>> + // 32 is undefined, so work around that. BitsPerPixel isn't 0 so
>> + // (BitsPerPixel - 1u) is ok.
>> + //
>
> Ah, OK. I've used this elsewhere, just in reverse order (first shift by
> variable count - 1, then by 1). However, Jordan disliked the trick, and
> ultimately we replaced it with a simple "if".
>
> In fact, above you already have a (BitsPerPixel == 32) check. At the end
> of that block, you could set PixelMask to MAX_UINT32 explicitly. And,
> you could add an "else" branch, simply with
>
> PixelMask = (1u << (BitsPerPixel - 1)) - 1;
>
> What do you think?
I'm pretty sure you mean
PixelMask = (1u << BitsPerPixel) - 1;
but yes, that's definitely more readable.
>> + PixelMask = ((0x1u << 1u) << (BitsPerPixel - 1u)) - 1u;
>> + ModeInfo->PixelInformation.ReservedMask =
>> + PixelMask & ~(RedMask | GreenMask | BlueMask);
>> +
>> + ModeInfo->PixelsPerScanLine = BytesPerLine / (BitsPerPixel / 8);
>> +
>> + ModeData ++;
>> + ModeInfo ++;
>> + }
>> + VideoMode ++;
>
> Please drop all the spaces before "++".
>
>> + }
>> + Private->MaxMode = ModeData - Private->ModeData;
>> + return EFI_SUCCESS;
>> +
>> +Rollback:
>> + FreePool(Private->VmwareSvgaModeInfo);
>
> Missing space between "FreePool" and "(".
>
>> + Private->VmwareSvgaModeInfo = NULL;
>> +
>> +ModeInfoAllocError:
>> + FreePool(Private->ModeData);
>
> Missing space between "FreePool" and "(".
>
>> + Private->ModeData = NULL;
>> +
>> +ModeDataAllocError:
>> + return Status;
>> +}
>>
>
> I think this is a very good update. All my fresh comments have been
> cosmetic. Please submit v4; I expect it to be the final version.
Thanks for your patience and perseverance with this patch! v4 coming
up, hopefully this one's a charm…
Phil
> Thank you,
> Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.