Re: [PATCH v2 3/3] OvmfPkg/QemuVideoDxe: VMWare SVGA II device support.

[Laszlo Ersek <lersek@redhat.com>]

On 04/03/17 00:44, 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 SVGA2 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 SVGA2 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.
> 
>  OvmfPkg/QemuVideoDxe/Qemu.h       |  37 +++++++
>  OvmfPkg/QemuVideoDxe/Driver.c     |  76 ++++++++++++++
>  OvmfPkg/QemuVideoDxe/Gop.c        |  74 +++++++++++++-
>  OvmfPkg/QemuVideoDxe/Initialize.c | 107 ++++++++++++++++++++
>  4 files changed, 293 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
> index 2ce37defc5b8..b5c84c9e695e 100644
> --- a/OvmfPkg/QemuVideoDxe/Qemu.h
> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h
> @@ -56,6 +56,10 @@ typedef struct {
>    UINT32  HorizontalResolution;
>    UINT32  VerticalResolution;
>    UINT32  ColorDepth;
> +  //
> +  // VMWare specific:
> +  //
> +  UINT32  PixelsPerLine; // includes any dead space

I suggest to call this VmwPixelsPerLine.

>  } QEMU_VIDEO_MODE_DATA;
>  
>  #define PIXEL_RED_SHIFT   0
> @@ -92,6 +96,7 @@ typedef enum {
>    QEMU_VIDEO_CIRRUS_5446,
>    QEMU_VIDEO_BOCHS,
>    QEMU_VIDEO_BOCHS_MMIO,
> +  QEMU_VIDEO_VMWARE_SVGA2,

QEMU_VIDEO_VMW_SVGA? In the IndustryStandard header file, we just use
VMW_SVGA.

>  } QEMU_VIDEO_VARIANT;
>  
>  typedef struct {
> @@ -119,6 +124,8 @@ typedef struct {
>    QEMU_VIDEO_VARIANT                    Variant;
>    FRAME_BUFFER_CONFIGURE                *FrameBufferBltConfigure;
>    UINTN                                 FrameBufferBltConfigureSize;
> +
> +  UINT16                                VMWareSVGA2_BasePort;

I suggest to drop the empty line, and to call this VmwSvgaBasePort.

>  } QEMU_VIDEO_PRIVATE_DATA;
>  
>  ///
> @@ -502,9 +509,39 @@ QemuVideoBochsModeSetup (
>    BOOLEAN                  IsQxl
>    );
>  
> +EFI_STATUS
> +QemuVideoVmwareModeSetup (
> +  QEMU_VIDEO_PRIVATE_DATA *Private
> +  );

Call this QemuVideoVmwSvgaModeSetup?

> +
>  VOID
>  InstallVbeShim (
>    IN CONST CHAR16         *CardName,
>    IN EFI_PHYSICAL_ADDRESS FrameBufferBase
>    );
> +
> +VOID
> +QemuVideoVMWSVGA2RegisterWrite (
> +  QEMU_VIDEO_PRIVATE_DATA *Private,
> +  UINT16                  reg,
> +  UINT32                  value
> +  );

VmwSvgaWrite?

> +
> +UINT32
> +QemuVideoVMWSVGA2RegisterRead (
> +  QEMU_VIDEO_PRIVATE_DATA *Private,
> +  UINT16                  reg
> +  );

VmwSvgaRead?

> +
> +EFI_STATUS
> +QemuVideoVMWSVGA2CompleteModeData (
> +  IN  QEMU_VIDEO_PRIVATE_DATA           *Private,
> +  OUT EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode
> +  );

QemuVideoVmwSvgaCompleteModeData?

> +
> +void InitializeVMWSVGA2GraphicsMode (
> +  QEMU_VIDEO_PRIVATE_DATA  *Private,
> +  QEMU_VIDEO_BOCHS_MODES   *ModeData
> +  );
> +

InitializeVmwSvga2GraphicsMode?

>  #endif
> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> index fc8025ec46de..f01aec6f7b0e 100644
> --- a/OvmfPkg/QemuVideoDxe/Driver.c
> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
> @@ -15,6 +15,7 @@
>  **/
>  
>  #include "Qemu.h"
> +#include <IndustryStandard/VMWareSVGA2.h>

Aha! So we did use "2" in the header file name. Haven't noticed that
until this point. I suggest we stay consistent in all spots, so either
please drop the "2" from the header file name in patch #1 (and the
include guard macro too), or else please use it everywhere.

Also, we should put <> includes before "" includes. (I guess some of the
current code doesn't conform to that, just below...)

>  #include <IndustryStandard/Acpi.h>
>  
>  EFI_DRIVER_BINDING_PROTOCOL gQemuVideoDriverBinding = {
> @@ -58,6 +59,16 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = {
>          QEMU_VIDEO_BOCHS_MMIO,
>          L"QEMU VirtIO VGA"
>      },{
> +#if defined MDE_CPU_IA32 || defined MDE_CPU_X64
> +        //
> +        // Support only platforms which can do unaligned port I/O
> +        //
> +        PCI_VENDOR_ID_VMWARE,
> +        PCI_DEVICE_ID_VMWARE_SVGA2,
> +        QEMU_VIDEO_VMWARE_SVGA2,
> +        L"QEMU VMWare SVGA2"
> +    },{
> +#endif

Can you please drop the #if?

>          0 /* end of list */
>      }
>  };
> @@ -317,6 +328,45 @@ QemuVideoControllerDriverStart (
>    }
>  
>    //
> +  // Check if accessing VMWARE_SVGA2 interface works
> +  //
> +  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA2) {
> +    EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *iodesc;

Please call this IoDesc.

> +    UINT32 TargetId;
> +    UINT32 Svga2IDRead;

Please call this Svga2IdRead.

Also, please aligh these two variable names with IoDesc horizontally.

> +
> +    Private->PciIo->GetBarAttributes (
> +                      Private->PciIo,
> +                      PCI_BAR_IDX0,
> +                      NULL,
> +                      (VOID**) &iodesc
> +                      );

Please check the return status of this protocol member call.

If
- the call fails, or
- IoDesc->ResType differs from ACPI_ADDRESS_SPACE_TYPE_IO, or
- IoDesc->AddrRangeMin is larger than or equal to MAX_UINT16,
then set Status to EFI_DEVICE_ERROR, and jump to RestoreAttributes.

> +    Private->VMWareSVGA2_BasePort = iodesc->AddrRangeMin;

I think you should do an explicit cast here, to UINT16; the AddrRangeMin
field has type UINT64, and VS will whine if you don't cast the RHS of
the assignment explicitly.

> +
> +    TargetId = SVGA_ID_2;
> +    while (1) {

Please use "while (TRUE)" or "for (;;)".

"while (1)" is not unused in edk2, but the two other forms are more
idiomatic.

> +      QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_ID, TargetId);
> +      Svga2IDRead = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_ID);
> +      if ((Svga2IDRead == TargetId) || (TargetId <= SVGA_ID_0)) {
> +        break;
> +      }
> +      --TargetId;
> +    }
> +
> +    if (Svga2IDRead != TargetId) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "QemuVideo: QEMU_VIDEO_VMWARE_SVGA2 ID mismatch "
> +        "(got 0x%x, base address 0x%x)\n",
> +        Svga2IDRead,
> +        Private->VMWareSVGA2_BasePort
> +        ));
> +      Status = EFI_DEVICE_ERROR;
> +      goto RestoreAttributes;
> +    }
> +  }
> +
> +  //
>    // Get ParentDevicePath
>    //
>    Status = gBS->HandleProtocol (
> @@ -371,6 +421,9 @@ QemuVideoControllerDriverStart (
>    case QEMU_VIDEO_BOCHS:
>      Status = QemuVideoBochsModeSetup (Private, IsQxl);
>      break;
> +  case QEMU_VIDEO_VMWARE_SVGA2:
> +    Status = QemuVideoVmwareModeSetup (Private);
> +    break;
>    default:
>      ASSERT (FALSE);
>      Status = EFI_DEVICE_ERROR;
> @@ -975,3 +1028,26 @@ InitializeQemuVideo (
>  
>    return Status;
>  }
> +
> +void InitializeVMWSVGA2GraphicsMode (

Can you please move this function higher up, so that it follows
InitializeBochsGraphicsMode() directly?


Also, it should start like:

VOID
InitializeVMWSVGA2GraphicsMode (
  ...

Please update the function declaration in "Qemu.h" as well.

> +  QEMU_VIDEO_PRIVATE_DATA   *Private,
> +  QEMU_VIDEO_BOCHS_MODES    *ModeData
> +  )
> +{
> +  QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_WIDTH, ModeData->Width);
> +  QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_HEIGHT, ModeData->Height);
> +
> +  UINT32 capabilities = QemuVideoVMWSVGA2RegisterRead (
> +                          Private,
> +                          SVGA_REG_CAPABILITIES
> +                          );

Please
- call this variable Capabilities,
- move its definition to the top of the containing scope,
- use a separate assignment -- initialization of automatic storage
duration variables ("locals") is forbidden in edk2, even if the
initializer were a constant.

> +  if ((capabilities & SVGA_CAP_8BIT_EMULATION) != 0) {
> +    QemuVideoVMWSVGA2RegisterWrite(
> +      Private,
> +      SVGA_REG_BITS_PER_PIXEL,
> +      ModeData->ColorDepth
> +      );
> +  }
> +  SetDefaultPalette (Private);
> +  ClearScreen (Private);
> +}

I don't understand how ClearScreen() can work here. ClearScreen() writes
MMIO BAR 0. But, on this video card, BAR 0 is expected to be an IO BAR,
which contains the register selector and register value ports.

> diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
> index 359e9217d3d1..59918676e764 100644
> --- a/OvmfPkg/QemuVideoDxe/Gop.c
> +++ b/OvmfPkg/QemuVideoDxe/Gop.c
> @@ -14,6 +14,7 @@
>  **/
>  
>  #include "Qemu.h"
> +#include <IndustryStandard/VMWareSVGA2.h>

<> should go before ""

>  
>  STATIC
>  VOID
> @@ -76,6 +77,63 @@ QemuVideoCompleteModeData (
>  }
>  
>  
> +EFI_STATUS
> +QemuVMSVGA2VideoCompleteModeData (
> +  IN  QEMU_VIDEO_PRIVATE_DATA           *Private,
> +  OUT EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode
> +  )
> +{
> +  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  *Info;
> +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR     *FrameBufDesc;
> +  UINT32                                RedMask, GreenMask, BlueMask;
> +  UINT32                                BitsPerPixel, BytesPerLine, FBOffset;
> +
> +  Info = Mode->Info;
> +  Info->Version = 0;
> +  Info->PixelFormat = PixelBitMask;
> +
> +  RedMask = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_RED_MASK);
> +  Info->PixelInformation.RedMask = RedMask;
> +
> +  GreenMask = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_GREEN_MASK);
> +  Info->PixelInformation.GreenMask = GreenMask;
> +
> +  BlueMask = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_BLUE_MASK);
> +  Info->PixelInformation.BlueMask = BlueMask;
> +
> +  QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_ENABLE, 1);

I don't think this belongs here. Can you put it in
InitializeVMWSVGA2GraphicsMode() instead?

> +
> +  FBOffset = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_FB_OFFSET);
> +  BytesPerLine = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_BYTES_PER_LINE);
> +  BitsPerPixel = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_BITS_PER_PIXEL);
> +
> +  if (BitsPerPixel == 32) {
> +    if (BlueMask == 0xff && GreenMask == 0xff00 && RedMask == 0xff0000) {
> +      Info->PixelFormat = PixelBlueGreenRedReserved8BitPerColor;
> +    } else if (BlueMask == 0xff0000 && GreenMask == 0xff00 && RedMask == 0xff) {
> +      Info->PixelFormat = PixelRedGreenBlueReserved8BitPerColor;
> +    }
> +  }
> +
> +  Info->PixelInformation.ReservedMask = ((0x2u << (BitsPerPixel - 1u)) - 1u)
> +                                         & ~(RedMask | GreenMask | BlueMask);

I have no idea what this does. Please add a comment that explains it in
detail.

> +  Info->PixelsPerScanLine = BytesPerLine / (BitsPerPixel / 8);
> +
> +  Private->PciIo->GetBarAttributes (
> +                        Private->PciIo,
> +                        PCI_BAR_IDX1,
> +                        NULL,
> +                        (VOID**) &FrameBufDesc
> +                        );

So, indeed the video memory is in BAR1. ClearScreen() doesn't match
that. You might want to introduce another field in
QEMU_VIDEO_PRIVATE_DATA, to carry the BAR number that corresponds to the
video memory.

> +
> +  Mode->FrameBufferBase = FrameBufDesc->AddrRangeMin + FBOffset;
> +  Mode->FrameBufferSize = BytesPerLine * Info->VerticalResolution;
> +
> +  FreePool (FrameBufDesc);
> +  return EFI_SUCCESS;
> +}
> +
> +
>  //
>  // Graphics Output Protocol Member Functions
>  //
> @@ -129,6 +187,10 @@ Routine Description:
>    (*Info)->VerticalResolution   = ModeData->VerticalResolution;
>    QemuVideoCompleteModeInfo (ModeData, *Info);
>  
> +  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA2) {
> +    (*Info)->PixelsPerScanLine = ModeData->PixelsPerLine;
> +  }
> +
>    return EFI_SUCCESS;
>  }

This doesn't seem right. QemuVideoCompleteModeInfo() is called on two paths:

  QemuVideoGraphicsOutputSetMode()
    //
    // for non-vmware
    //
    QemuVideoCompleteModeData()
      QemuVideoCompleteModeInfo()

and

  QemuVideoGraphicsOutputQueryMode()
    QemuVideoCompleteModeInfo()

The first call path is for the mode being *selected*, the second call
path is for querying *any* mode.

Now, on the first call path, you found QemuVideoCompleteModeInfo()
inappropriate for vmw, because you introduced the following in its place:

  QemuVideoGraphicsOutputSetMode()
    //
    // for vmw
    //
    QemuVMSVGA2VideoCompleteModeData()
      //
      // manual setting of pixel mask
      // no call to QemuVideoCompleteModeInfo()
      //

But in that case, I don't see how calling QemuVideoCompleteModeInfo() in
QemuVideoGraphicsOutputQueryMode() -- that is, on the second path --,
and only overriding PixelsPerScanLine, could be correct for vmw. The
pixel mask values filled in by QemuVideoCompleteModeInfo() will be
incorrect.

The mode information should be populated uniformly on both code paths,
regardless of whether we are selecting a mode for actual use (==
changing hardware state), or we are just querying some random mode (==
not changing hardware state).

Are you saying that the pixel mask values cannot be fetched & returned
without actually selecting those graphics modes?

In that case, I think you should pre-compute the pixel mask values in
QemuVideoVmwareModeSetup(), where you already iterate through all the
modes. (I.e., similarly to PixelsPerLine.)

Then both the pixel format setting and the PixelsPerScanLine setting,
for vmw, should be extracted to a function similar to
QemuVideoCompleteModeInfo(). That new function should be called on both
paths above; that is, from QemuVMSVGA2VideoCompleteModeData(), and also
from QemuVideoGraphicsOutputQueryMode(). This new function should
populate Info from the precomputed, cached values, including all the
pixel format fields and the PixelsPerScanLine field.

(Even with PixelsPerScanLine you have an inconsistency now: although in
QueryMode, you use the PixelsPerLine value precomputed in
QemuVideoVmwareModeSetup(), in QemuVMSVGA2VideoCompleteModeData(), you
still read the SVGA_REG_BYTES_PER_LINE register directly again. I think
that shouldn't be necessary.)

>  
> @@ -176,6 +238,12 @@ Routine Description:
>    case QEMU_VIDEO_BOCHS:
>      InitializeBochsGraphicsMode (Private, &QemuVideoBochsModes[ModeData->InternalModeIndex]);
>      break;
> +  case QEMU_VIDEO_VMWARE_SVGA2:
> +    InitializeVMWSVGA2GraphicsMode (
> +      Private,
> +      &QemuVideoBochsModes[ModeData->InternalModeIndex]
> +      );
> +    break;
>    default:
>      ASSERT (FALSE);
>      return EFI_DEVICE_ERROR;
> @@ -186,7 +254,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_SVGA2) {
> +    QemuVMSVGA2VideoCompleteModeData(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..8a6db566ab05 100644
> --- a/OvmfPkg/QemuVideoDxe/Initialize.c
> +++ b/OvmfPkg/QemuVideoDxe/Initialize.c
> @@ -14,6 +14,8 @@
>  **/
>  
>  #include "Qemu.h"
> +#include "UnalignedIoInternal.h"
> +#include <IndustryStandard/VMWareSVGA2.h>

<> includes should go before "" includes.

>  
>  
>  ///
> @@ -346,3 +348,108 @@ QemuVideoBochsModeSetup (
>    return EFI_SUCCESS;
>  }
>  
> +VOID
> +QemuVideoVMWSVGA2RegisterWrite (
> +  QEMU_VIDEO_PRIVATE_DATA   *Private,
> +  UINT16                    reg,
> +  UINT32                    value
> +  )
> +{
> +  UnalignedIoWrite32 (Private->VMWareSVGA2_BasePort + SVGA_INDEX_PORT, reg);
> +  UnalignedIoWrite32 (Private->VMWareSVGA2_BasePort + SVGA_VALUE_PORT, value);
> +}
> +
> +UINT32
> +QemuVideoVMWSVGA2RegisterRead (
> +  QEMU_VIDEO_PRIVATE_DATA   *Private,
> +  UINT16                    reg
> +  )
> +{
> +  UnalignedIoWrite32 (Private->VMWareSVGA2_BasePort + SVGA_INDEX_PORT, reg);
> +  return UnalignedIoRead32 (Private->VMWareSVGA2_BasePort + SVGA_VALUE_PORT);
> +}

Please call "reg" and "value" Register and Value. (Don't forget to
update "Qemu.h" too.)

Also, I think these two functions should be moved to "Driver.c", just
under BochsWrite() / BochsRead().

> +
> +EFI_STATUS
> +QemuVideoVmwareModeSetup (
> +  QEMU_VIDEO_PRIVATE_DATA *Private
> +  )
> +{
> +  UINT32                 FBSize;

Please call this FbSize.

> +  UINT32                 MaxWidth, MaxHeight;
> +  UINT32                 Capabilities;
> +  UINT32                 HostBitsPerPixel;
> +  UINT32                 Index;
> +  QEMU_VIDEO_MODE_DATA   *ModeData;
> +  QEMU_VIDEO_BOCHS_MODES *VideoMode;
> +
> +  QemuVideoVMWSVGA2RegisterWrite (Private, SVGA_REG_ENABLE, 0);
> +
> +  Private->ModeData =
> +    AllocatePool (sizeof (Private->ModeData[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT);
> +  if (Private->ModeData == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  FBSize =       QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_FB_SIZE);
> +  MaxWidth =     QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_MAX_WIDTH);
> +  MaxHeight =    QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_MAX_HEIGHT);
> +  Capabilities = QemuVideoVMWSVGA2RegisterRead (Private, SVGA_REG_CAPABILITIES);
> +  if ((Capabilities & SVGA_CAP_8BIT_EMULATION) != 0) {
> +    HostBitsPerPixel = QemuVideoVMWSVGA2RegisterRead(

Missing space right after "QemuVideoVMWSVGA2RegisterRead".

> +                         Private,
> +                         SVGA_REG_HOST_BITS_PER_PIXEL
> +                         );
> +    QemuVideoVMWSVGA2RegisterWrite (
> +      Private,
> +      SVGA_REG_BITS_PER_PIXEL,
> +      HostBitsPerPixel
> +      );
> +  } else {
> +    HostBitsPerPixel = QemuVideoVMWSVGA2RegisterRead(

Missing space right after "QemuVideoVMWSVGA2RegisterRead".

> +                         Private,
> +                         SVGA_REG_BITS_PER_PIXEL
> +                         );
> +  }
> +
> +  ModeData = Private->ModeData;
> +  VideoMode = &QemuVideoBochsModes[0];
> +  for (Index = 0; Index < QEMU_VIDEO_BOCHS_MODE_COUNT; Index ++) {
> +    UINTN RequiredFbSize;
> +
> +    ASSERT (HostBitsPerPixel % 8 == 0);
> +    RequiredFbSize = (UINTN) VideoMode->Width * VideoMode->Height *
> +                     (HostBitsPerPixel / 8);
> +    if (RequiredFbSize <= FBSize
> +     && VideoMode->Width <= MaxWidth
> +     && VideoMode->Height <= MaxHeight)
> +    {

This should be:

  if (Expression1 &&
      Expression2 &&
      Expression3) {
    //
    // dependent code
    //
  }


> +      UINT32 BytesPerLine;
> +
> +      QemuVideoVMWSVGA2RegisterWrite (
> +        Private,
> +        SVGA_REG_WIDTH,
> +        VideoMode->Width
> +        );
> +      QemuVideoVMWSVGA2RegisterWrite (
> +        Private,
> +        SVGA_REG_HEIGHT,
> +        VideoMode->Height
> +        );
> +      BytesPerLine = QemuVideoVMWSVGA2RegisterRead (
> +                       Private,
> +                       SVGA_REG_BYTES_PER_LINE
> +                       );
> +      ModeData->PixelsPerLine = BytesPerLine / (HostBitsPerPixel / 8);
> +
> +      ModeData->InternalModeIndex    = Index;
> +      ModeData->HorizontalResolution = VideoMode->Width;
> +      ModeData->VerticalResolution   = VideoMode->Height;
> +      ModeData->ColorDepth           = HostBitsPerPixel;
> +
> +      ModeData ++;
> +    }
> +    VideoMode ++;
> +  }
> +  Private->MaxMode = ModeData - Private->ModeData;
> +  return EFI_SUCCESS;
> +}
> 

Thanks
Laszlo