From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 3210121A04811 for ; Wed, 5 Apr 2017 06:58:37 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9A68FC05AA56; Wed, 5 Apr 2017 13:58:36 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9A68FC05AA56 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 9A68FC05AA56 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-13.phx2.redhat.com [10.3.116.13]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0FE0F7F39C; Wed, 5 Apr 2017 13:58:34 +0000 (UTC) To: Phil Dennis-Jordan , edk2-devel@lists.01.org References: <1491396916-53170-1-git-send-email-lists@philjordan.eu> <1491396916-53170-4-git-send-email-lists@philjordan.eu> Cc: Jordan Justen , Phil Dennis-Jordan From: Laszlo Ersek Message-ID: <9ccc975c-36ad-e3ba-d909-3863c09dbf57@redhat.com> Date: Wed, 5 Apr 2017 15:58:33 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1491396916-53170-4-git-send-email-lists@philjordan.eu> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 05 Apr 2017 13:58:36 +0000 (UTC) Subject: Re: [PATCH v4 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 13:58:37 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 04/05/17 14:55, 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 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. > > v4: > - Simplified mode info pixel mask calculation & PCI BAR OOB check. [Laszlo] > - Replaced struct assignment with CopyMem. [Laszlo] > - Whitespace & comment typo fixes. [Laszlo] Also, you moved around the "BytesPerLine" assignment. Then I noticed that you actually moved around the *second* instance of it, so you continue to have two identical assignments, one of which should be superfluous: [snip] > +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 > + ); This is #1. > + > + 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 save 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; > + > + // > + // Reserved mask is whatever bits in the pixel not containing RGB data, > + // so start with binary 1s for every bit in the pixel, then mask off > + // bits already used for RGB. Special case 32 to avoid undefined > + // behaviour in the shift. > + // > + if (BitsPerPixel == 32) { > + if (BlueMask == 0xff && GreenMask == 0xff00 && RedMask == 0xff0000) { > + ModeInfo->PixelFormat = PixelBlueGreenRedReserved8BitPerColor; > + } else if (BlueMask == 0xff0000 && > + GreenMask == 0xff00 && > + RedMask == 0xff) { > + ModeInfo->PixelFormat = PixelRedGreenBlueReserved8BitPerColor; > + } > + PixelMask = MAX_UINT32; > + } else { > + PixelMask = (1u << BitsPerPixel) - 1; > + } > + ModeInfo->PixelInformation.ReservedMask = > + PixelMask & ~(RedMask | GreenMask | BlueMask); > + > + BytesPerLine = VmwareSvgaRead (Private, VmwareSvgaRegBytesPerLine); This is #2. Which one do you want to keep? I think we should drop #1, and keep #2. Do you wish to submit v5, or are you okay if I remove #1 for you at commit time? With this tweak, Reviewed-by: Laszlo Ersek Before committing, I would like to give Jordan a chance to comment as well. I think Friday this week should be a good day to commit the series. Impressive work! Thank you, Laszlo > + 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; > +} >