Re: [PATCH 4/4] OvmfPkg: simply use the Bochs interface for vmsvga

[yuchenlin <yuchenlin@synology.com>]

On 2018-11-02 02:54, Philippe Mathieu-Daudé wrote:
> Hi Yu-chen Lin,
> 
> On 24/10/18 8:40, yuchenlin@synology.com wrote:
>> From: yuchenlin <yuchenlin@synology.com>
>> 
>> BAR  |    std vga     |  vmsvga
>> ---------------------------------
>> 0    |   Framebuffer  | I/O space
>> 1    |   Reserved     | Framebuffer
>> 2    |   MMIO         | FIFO
>> 
>> Because of the PCI BARs difference between std vga and
>> vmsvga, we can not simply recognize the "QEMU VMWare SVGA"
>> as the QEMU_VIDEO_BOCHS_MMIO variant.
>> 
>> Instead, we remain variant QEMU_VIDEO_VMWARE_SVGA, and use
>> it for:
>> 
>> (1) Get framebuffer from correct PCI BAR
>> (2) Prevent using BAR2 for MMIO
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: yuchenlin <yuchenlin@synology.com>
>> ---
>>   OvmfPkg/QemuVideoDxe/Driver.c | 16 +++++++++++++++-
>>   OvmfPkg/QemuVideoDxe/Gop.c    |  3 ++-
>>   OvmfPkg/QemuVideoDxe/Qemu.h   |  2 ++
>>   3 files changed, 19 insertions(+), 2 deletions(-)
>> 
>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c 
>> b/OvmfPkg/QemuVideoDxe/Driver.c
>> index 2304afd1e6..a1785c2285 100644
>> --- a/OvmfPkg/QemuVideoDxe/Driver.c
>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
>> @@ -16,6 +16,7 @@
>>     #include "Qemu.h"
>>   #include <IndustryStandard/Acpi.h>
>> +#include <IndustryStandard/VmwareSvga.h>
>>     EFI_DRIVER_BINDING_PROTOCOL gQemuVideoDriverBinding = {
>>     QemuVideoControllerDriverSupported,
>> @@ -69,6 +70,12 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = {
>>           0x1050,
>>           QEMU_VIDEO_BOCHS_MMIO,
>>           L"QEMU VirtIO VGA"
>> +    },{
>> +        PCI_CLASS_DISPLAY_VGA,
>> +        VMWARE_PCI_VENDOR_ID_VMWARE,
>> +        VMWARE_PCI_DEVICE_ID_VMWARE_SVGA2,
>> +        QEMU_VIDEO_VMWARE_SVGA,
>> +        L"QEMU VMWare SVGA"
> 
> Looks OK.
> 
>>       },{
>>           0 /* end of list */
>>       }
>> @@ -256,6 +263,11 @@ QemuVideoControllerDriverStart (
>>       goto ClosePciIo;
>>     }
>>     Private->Variant = Card->Variant;
>> +  Private->FrameBufferVramBarIndex = PCI_BAR_IDX0;
>> +  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
>> +    Private->FrameBufferVramBarIndex = PCI_BAR_IDX1;
> 
> OK, but why not use an 'else' clause?

Will do

Thanks,
yuchenlin

> 
>> +  }
>> +
>>       //
>>     // IsQxl is based on the detected Card->Variant, which at a later 
>> point might
>> @@ -320,7 +332,8 @@ QemuVideoControllerDriverStart (
>>     // Check if accessing the bochs interface works.
>>     //
>>     if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO ||
>> -      Private->Variant == QEMU_VIDEO_BOCHS) {
>> +      Private->Variant == QEMU_VIDEO_BOCHS ||
>> +      Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
>>       UINT16 BochsId;
>>       BochsId = BochsRead(Private, VBE_DISPI_INDEX_ID);
>>       if ((BochsId & 0xFFF0) != VBE_DISPI_ID0) {
>> @@ -383,6 +396,7 @@ QemuVideoControllerDriverStart (
>>       break;
>>     case QEMU_VIDEO_BOCHS_MMIO:
>>     case QEMU_VIDEO_BOCHS:
>> +  case QEMU_VIDEO_VMWARE_SVGA:
>>       Status = QemuVideoBochsModeSetup (Private, IsQxl);
>>       break;
>>     default:
>> diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
>> index d490fa7a2e..3abc5eeb36 100644
>> --- a/OvmfPkg/QemuVideoDxe/Gop.c
>> +++ b/OvmfPkg/QemuVideoDxe/Gop.c
>> @@ -60,7 +60,7 @@ QemuVideoCompleteModeData (
>>       Private->PciIo->GetBarAttributes (
>>                           Private->PciIo,
>> -                        0,
>> +                        Private->FrameBufferVramBarIndex,
> 
> OK
> 
>>                           NULL,
>>                           (VOID**) &FrameBufDesc
>>                           );
>> @@ -177,6 +177,7 @@ Routine Description:
>>       break;
>>     case QEMU_VIDEO_BOCHS_MMIO:
>>     case QEMU_VIDEO_BOCHS:
>> +  case QEMU_VIDEO_VMWARE_SVGA:
>>       InitializeBochsGraphicsMode (Private, 
>> &QemuVideoBochsModes[ModeData->InternalModeIndex]);
>>       break;
>>     default:
>> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
>> index d7da761705..3aac9eeca6 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 {
>> @@ -120,6 +121,7 @@ typedef struct {
>>     QEMU_VIDEO_VARIANT                    Variant;
>>     FRAME_BUFFER_CONFIGURE                *FrameBufferBltConfigure;
>>     UINTN                                 FrameBufferBltConfigureSize;
>> +  UINT8                                 FrameBufferVramBarIndex;
>>   } QEMU_VIDEO_PRIVATE_DATA;
>>     ///
>>