From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ua0-x242.google.com (mail-ua0-x242.google.com [IPv6:2607:f8b0:400c:c08::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id F37682008090B for ; Wed, 5 Apr 2017 05:46:30 -0700 (PDT) Received: by mail-ua0-x242.google.com with SMTP id 100so889795uaj.2 for ; Wed, 05 Apr 2017 05:46:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=philjordan-eu.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=eWc2842503ZoLtBzBdhS8j8g4xstRiwzYbnUSA0qE2o=; b=dcsw+2PUIx7mT5vA15oiM759TAuEojNj+8L2mxyGw3tzm4fxyis4nUeKc06c2qLVnd CPysy4xieR57Mj6PnxfVDRV0golLAaS1E4xRWZV6Dlj7JOTrRtLQztwns5eOZ/UsLzEm FtQ3KfuezPmm9jnmH2a1U3/6gBJV1zVKmkkzRZVeofcbRy8bLSncf2NVbF8OGydYxQuf rJk9cj6rp9uX8AK0FIMjip4E9BdlbYHlIPHC3Kafv9mWKlSYnFz9/Od6qZQ+n5ifCtsG cC0lfsUMKwulsG7WjEBN6IYrvK7jjUx73JbWAWBeXrnZBaz+8flHc0OVacmIT/as3Yfl hb9Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=eWc2842503ZoLtBzBdhS8j8g4xstRiwzYbnUSA0qE2o=; b=mKrv3+5mwshssnp6z2BLw/pP1s3X6m9YQl5+JyqAWj7Mhndi0nDGT3Z5oTtTNiMZQL 9sH09avRwQr93yd17F92bINt1xlSrxMgNhMD0dtHG1Xmb8Ekwotmywy9FHOuOgp61vN1 hMcRnZwG0E/5t8fGg1t5+eebZofVKb9RLMww3j3bkU6It84uNXwZVoNyk1R3YEWKBSUh vq1XosNSbG2C4n/uBBK7vMKZ8RtmA/KjrWGTdFySVnwgGNqpP6PLPEBwdF8jNJBafaj1 Moy2bQ8roNFMVo2MeNUIKP1l4vAXUGqaOsk+4vtb+MlG/clm7gy00lBX7RenSatWgDcp Pi4A== X-Gm-Message-State: AFeK/H3nuZ+4O25n7jMgWd1EFZ7YL0DLleHbGmNkOosltFimwMM1E+bma4HgI9N3joS7cf9rWV6f4QixzOT1vw== X-Received: by 10.176.16.83 with SMTP id g19mr913056uab.168.1491396389637; Wed, 05 Apr 2017 05:46:29 -0700 (PDT) MIME-Version: 1.0 Received: by 10.31.58.83 with HTTP; Wed, 5 Apr 2017 05:46:09 -0700 (PDT) In-Reply-To: <31192690-0c6a-ce61-8c8f-a19be1c3ccb4@redhat.com> References: <1491386280-50077-1-git-send-email-lists@philjordan.eu> <1491386280-50077-4-git-send-email-lists@philjordan.eu> <31192690-0c6a-ce61-8c8f-a19be1c3ccb4@redhat.com> From: Phil Dennis-Jordan Date: Thu, 6 Apr 2017 00:46:09 +1200 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 , Jordan Justen , Phil Dennis-Jordan Subject: Re: [PATCH v3 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA device support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Apr 2017 12:46:31 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Wed, Apr 5, 2017 at 11:41 PM, Laszlo Ersek wrote: > On 04/05/17 11:58, Phil Dennis-Jordan wrote: >> From: Phil Dennis-Jordan >> >> 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 >> Cc: Laszlo Ersek >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Phil Dennis-Jordan >> --- >> >> 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 prefix= es [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. [Laszl= o] >> - 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 hitt= ing 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 >> #include >> +#include "Qemu.h" >> +#include "UnalignedIoInternal.h" >> >> EFI_DRIVER_BINDING_PROTOCOL gQemuVideoDriverBinding =3D { >> QemuVideoControllerDriverSupported, >> @@ -58,6 +60,11 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] =3D { >> 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 =3D Card->Variant; >> + Private->FrameBufferVramBarIndex =3D PCI_BAR_IDX0; >> >> // >> // IsQxl is based on the detected Card->Variant, which at a later poi= nt might >> @@ -317,6 +325,59 @@ QemuVideoControllerDriverStart ( >> } >> >> // >> + // Check if accessing Vmware SVGA interface works >> + // >> + if (Private->Variant =3D=3D QEMU_VIDEO_VMWARE_SVGA) { >> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *IoDesc; >> + UINT32 TargetId; >> + UINT32 SvgaIdRead; >> + >> + IoDesc =3D NULL; >> + Status =3D 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 !=3D ACPI_ADDRESS_SPACE_TYPE_IO || >> + IoDesc->AddrRangeMin >=3D MAX_UINT16 || >> + IoDesc->AddrRangeMin + VMWARE_SVGA_VALUE_PORT >=3D 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 >=3D, 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 !=3D NULL) { >> + FreePool (IoDesc); >> + } > > Good catch :) > >> + Status =3D EFI_DEVICE_ERROR; >> + goto RestoreAttributes; >> + } >> + Private->VmwareSvgaBasePort =3D (UINT16) IoDesc->AddrRangeMin; >> + FreePool (IoDesc); > > Here too :) > >> + >> + TargetId =3D VMWARE_SVGA_ID_2; >> + while (TRUE) { >> + VmwareSvgaWrite (Private, VmwareSvgaRegId, TargetId); >> + SvgaIdRead =3D VmwareSvgaRead (Private, VmwareSvgaRegId); >> + if ((SvgaIdRead =3D=3D TargetId) || (TargetId <=3D 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 !=3D TargetId) { >> + DEBUG (( >> + DEBUG_ERROR, >> + "QemuVideo: QEMU_VIDEO_VMWARE_SVGA ID mismatch " >> + "(got 0x%x, base address 0x%x)\n", >> + SvgaIdRead, >> + Private->VmwareSvgaBasePort >> + )); >> + Status =3D EFI_DEVICE_ERROR; >> + goto RestoreAttributes; >> + } >> + >> + Private->FrameBufferVramBarIndex =3D PCI_BAR_IDX1; >> + } >> + >> + // >> // Get ParentDevicePath >> // >> Status =3D gBS->HandleProtocol ( >> @@ -371,6 +432,9 @@ QemuVideoControllerDriverStart ( >> case QEMU_VIDEO_BOCHS: >> Status =3D QemuVideoBochsModeSetup (Private, IsQxl); >> break; >> + case QEMU_VIDEO_VMWARE_SVGA: >> + Status =3D QemuVideoVmwareSvgaModeSetup (Private); >> + break; >> default: >> ASSERT (FALSE); >> Status =3D 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 =3D VmwareSvgaRead ( >> + Private, >> + VmwareSvgaRegCapabilities >> + ); >> + if ((Capabilities & VMWARE_SVGA_CAP_8BIT_EMULATION) !=3D 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 >> #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, BytesPe= rPixel; >> + >> + Info =3D Mode->Info; >> + CopyMem (Info, &Private->VmwareSvgaModeInfo[Mode->Mode], sizeof(*Info= )); > > Please write "sizeof (*Info)" or "sizeof *Info". > >> + BytesPerPixel =3D Private->ModeData[Mode->Mode].ColorDepth / 8; >> + BytesPerLine =3D Info->PixelsPerScanLine * BytesPerPixel; >> + >> + FbOffset =3D VmwareSvgaRead (Private, VmwareSvgaRegFbOffset); >> + >> + Status =3D 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 =3D FrameBufDesc->AddrRangeMin + FbOffset; >> + Mode->FrameBufferSize =3D BytesPerLine * Info->VerticalResolution; >> + >> + FreePool (FrameBufDesc); >> + return Status; >> +} >> + >> >> // >> // Graphics Output Protocol Member Functions >> @@ -124,10 +161,14 @@ Routine Description: >> >> *SizeOfInfo =3D sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION); >> >> - ModeData =3D &Private->ModeData[ModeNumber]; >> - (*Info)->HorizontalResolution =3D ModeData->HorizontalResolution; >> - (*Info)->VerticalResolution =3D ModeData->VerticalResolution; >> - QemuVideoCompleteModeInfo (ModeData, *Info); >> + if (Private->Variant =3D=3D QEMU_VIDEO_VMWARE_SVGA) { >> + **Info =3D 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 =3D &Private->ModeData[ModeNumber]; >> + (*Info)->HorizontalResolution =3D ModeData->HorizontalResolution; >> + (*Info)->VerticalResolution =3D 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 =3D ModeData->VerticalResolution= ; >> This->Mode->SizeOfInfo =3D sizeof(EFI_GRAPHICS_OUTPUT_MODE_INFORMATIO= N); >> >> - QemuVideoCompleteModeData (Private, This->Mode); >> + if (Private->Variant =3D=3D 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/In= itialize.c >> index d5d8cfef9661..8c5c993c7d0e 100644 >> --- a/OvmfPkg/QemuVideoDxe/Initialize.c >> +++ b/OvmfPkg/QemuVideoDxe/Initialize.c >> @@ -13,6 +13,7 @@ >> >> **/ >> >> +#include >> #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 =3D >> + AllocatePool (sizeof (Private->ModeData[0]) * QEMU_VIDEO_BOCHS_MODE= _COUNT); >> + if (Private->ModeData =3D=3D NULL) { >> + Status =3D EFI_OUT_OF_RESOURCES; >> + goto ModeDataAllocError; >> + } >> + >> + Private->VmwareSvgaModeInfo =3D >> + AllocatePool ( >> + sizeof (Private->VmwareSvgaModeInfo[0]) * QEMU_VIDEO_BOCHS_MODE_C= OUNT >> + ); >> + if (Private->VmwareSvgaModeInfo =3D=3D NULL) { >> + Status =3D EFI_OUT_OF_RESOURCES; >> + goto ModeInfoAllocError; >> + } >> + >> + FbSize =3D VmwareSvgaRead (Private, VmwareSvgaRegFbSize); >> + MaxWidth =3D VmwareSvgaRead (Private, VmwareSvgaRegMaxWidth); >> + MaxHeight =3D VmwareSvgaRead (Private, VmwareSvgaRegMaxHeight); >> + Capabilities =3D VmwareSvgaRead (Private, VmwareSvgaRegCapabilities); >> + if ((Capabilities & VMWARE_SVGA_CAP_8BIT_EMULATION) !=3D 0) { >> + BitsPerPixel =3D VmwareSvgaRead ( >> + Private, >> + VmwareSvgaRegHostBitsPerPixel >> + ); >> + VmwareSvgaWrite ( >> + Private, >> + VmwareSvgaRegBitsPerPixel, >> + BitsPerPixel >> + ); >> + } else { >> + BitsPerPixel =3D VmwareSvgaRead ( >> + Private, >> + VmwareSvgaRegBitsPerPixel >> + ); >> + } >> + >> + if (FbSize =3D=3D 0 || >> + MaxWidth =3D=3D 0 || >> + MaxHeight =3D=3D 0 || >> + BitsPerPixel =3D=3D 0 || >> + BitsPerPixel % 8 !=3D 0) { >> + Status =3D EFI_DEVICE_ERROR; >> + goto Rollback; >> + } >> + >> + ModeData =3D Private->ModeData; >> + ModeInfo =3D Private->VmwareSvgaModeInfo; >> + VideoMode =3D &QemuVideoBochsModes[0]; >> + for (Index =3D 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 =3D (UINTN) VideoMode->Width * VideoMode->Height * >> + (BitsPerPixel / 8); >> + if (RequiredFbSize <=3D FbSize && >> + VideoMode->Width <=3D MaxWidth && >> + VideoMode->Height <=3D MaxHeight) { >> + UINT32 BytesPerLine; >> + UINT32 RedMask, GreenMask, BlueMask, PixelMask; >> + >> + VmwareSvgaWrite ( >> + Private, >> + VmwareSvgaRegWidth, >> + VideoMode->Width >> + ); >> + VmwareSvgaWrite ( >> + Private, >> + VmwareSvgaRegHeight, >> + VideoMode->Height >> + ); >> + BytesPerLine =3D VmwareSvgaRead ( >> + Private, >> + VmwareSvgaRegBytesPerLine >> + ); >> + >> + ModeData->InternalModeIndex =3D Index; >> + ModeData->HorizontalResolution =3D VideoMode->Width; >> + ModeData->VerticalResolution =3D VideoMode->Height; >> + ModeData->ColorDepth =3D BitsPerPixel; >> + >> + // >> + // Setting VmwareSvgaRegWidth/VmwareSvgaRegHeight actually change= s >> + // the device's display mode, so we all properties of each mode u= p front > > I think you accidentally the verb :) > >> + // to avoid inadvertent mode changes later. >> + // >> + ModeInfo->Version =3D 0; >> + ModeInfo->HorizontalResolution =3D ModeData->HorizontalResolution= ; >> + ModeInfo->VerticalResolution =3D ModeData->VerticalResolution; >> + >> + ModeInfo->PixelFormat =3D PixelBitMask; >> + >> + RedMask =3D VmwareSvgaRead (Private, VmwareSvgaRegRedMask); >> + ModeInfo->PixelInformation.RedMask =3D RedMask; >> + >> + GreenMask =3D VmwareSvgaRead (Private, VmwareSvgaRegGreenMask); >> + ModeInfo->PixelInformation.GreenMask =3D GreenMask; >> + >> + BlueMask =3D VmwareSvgaRead (Private, VmwareSvgaRegBlueMask); >> + ModeInfo->PixelInformation.BlueMask =3D BlueMask; >> + >> + BytesPerLine =3D VmwareSvgaRead (Private, VmwareSvgaRegBytesPerLi= ne); >> + >> + if (BitsPerPixel =3D=3D 32) { >> + if (BlueMask =3D=3D 0xff && GreenMask =3D=3D 0xff00 && RedMask = =3D=3D 0xff0000) { >> + ModeInfo->PixelFormat =3D PixelBlueGreenRedReserved8BitPerCol= or; >> + } else if (BlueMask =3D=3D 0xff0000 && >> + GreenMask =3D=3D 0xff00 && >> + RedMask =3D=3D 0xff) { >> + ModeInfo->PixelFormat =3D PixelRedGreenBlueReserved8BitPerCol= or; >> + } >> + } >> + >> + // >> + // Reserved mask is whatever bits in the pixel are not used for R= GB. >> + // PixelMask is as many binary 1s as BitsPerPixel, but shifting U= INT32 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 =3D=3D 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 =3D (1u << (BitsPerPixel - 1)) - 1; > > What do you think? I'm pretty sure you mean PixelMask =3D (1u << BitsPerPixel) - 1; but yes, that's definitely more readable. >> + PixelMask =3D ((0x1u << 1u) << (BitsPerPixel - 1u)) - 1u; >> + ModeInfo->PixelInformation.ReservedMask =3D >> + PixelMask & ~(RedMask | GreenMask | BlueMask); >> + >> + ModeInfo->PixelsPerScanLine =3D BytesPerLine / (BitsPerPixel / 8)= ; >> + >> + ModeData ++; >> + ModeInfo ++; >> + } >> + VideoMode ++; > > Please drop all the spaces before "++". > >> + } >> + Private->MaxMode =3D ModeData - Private->ModeData; >> + return EFI_SUCCESS; >> + >> +Rollback: >> + FreePool(Private->VmwareSvgaModeInfo); > > Missing space between "FreePool" and "(". > >> + Private->VmwareSvgaModeInfo =3D NULL; >> + >> +ModeInfoAllocError: >> + FreePool(Private->ModeData); > > Missing space between "FreePool" and "(". > >> + Private->ModeData =3D 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=E2=80=A6 Phil > Thank you, > Laszlo