From: Laszlo Ersek <lersek@redhat.com>
To: Phil Dennis-Jordan <lists@philjordan.eu>, edk2-devel@lists.01.org
Cc: Jordan Justen <jordan.l.justen@intel.com>,
Phil Dennis-Jordan <phil@philjordan.eu>
Subject: Re: [PATCH v3 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA device support
Date: Wed, 5 Apr 2017 13:41:35 +0200 [thread overview]
Message-ID: <31192690-0c6a-ce61-8c8f-a19be1c3ccb4@redhat.com> (raw)
In-Reply-To: <1491386280-50077-4-git-send-email-lists@philjordan.eu>
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
next prev parent reply other threads:[~2017-04-05 11:41 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-05 9:57 [PATCH v3 0/3] OvmfPkg/QemuVideoDxe: Add VMWare SVGA framebuffer support Phil Dennis-Jordan
2017-04-05 9:57 ` [PATCH v3 1/3] OvmfPkg: VMWare SVGA display device register definitions Phil Dennis-Jordan
2017-04-05 10:05 ` Laszlo Ersek
2017-04-05 9:57 ` [PATCH v3 2/3] OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O Phil Dennis-Jordan
2017-04-05 10:16 ` Laszlo Ersek
2017-04-05 9:58 ` [PATCH v3 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA device support Phil Dennis-Jordan
2017-04-05 11:41 ` Laszlo Ersek [this message]
2017-04-05 12:46 ` Phil Dennis-Jordan
[not found] ` <CAAibmn3sm+QGwp1QaBq+_m4UQy1L1rcG7Z3RRq=L4sY-1WmJyQ@mail.gmail.com>
2017-04-05 12:58 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=31192690-0c6a-ce61-8c8f-a19be1c3ccb4@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox