Re: [PATCH v4 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA device support

[Laszlo Ersek <lersek@redhat.com>]

On 04/05/17 14:55, 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.
>     
>     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 <lersek@redhat.com>

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;
> +}
>