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

[yuchenlin <yuchenlin@synology.com>]

On 2018-11-21 03:59, Laszlo Ersek wrote:
> On 11/07/18 18:06, Laszlo Ersek wrote:
>> On 11/07/18 04:47, yuchenlin@synology.com wrote:
>>> From: yuchenlin <yuchenlin@synology.com>
>>> 
>>> BAR  |    std vga     |  vmsvga
>>> ---------------------------------
>>> 0    |   Framebuffer  | I/O space
>>> 1    |   Reserved     | Framebuffer
>>> 2    |   MMIO         | FIFO
>>> 
>>> - We cannot recognize VMW SVGA as BOCHS because that would confuse 
>>> the
>>>   IsQxl setting in QemuVideoControllerDriverStart(),
>>> 
>>> - We cannot recognize VMW SVGA as BOCHS_MMIO because BAR2 on VMW SVGA 
>>> is
>>>   not the BOCHS MMIO BAR (we can only use port IO).
>>> 
>>> Therefore the list of reasons for which we should introduce
>>> QEMU_VIDEO_VMWARE_SVGA should name three reasons:
>>> 
>>>  (1) Get framebuffer from correct PCI BAR
>>>  (2) Prevent using BAR2 for MMIO
>>>  (3) Prevent mis-recognizing VMW SVGA as QXL
>>> 
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: yuchenlin <yuchenlin@synology.com>
>>> ---
>>> Changelog:
>>> 
>>> v1 -> v2
>>> * use 'else' clause (Thanks Philippe).
>>> * add more comment in revert patches (Thanks Philippe).
>>> * reorder the revert patches, we should revert the last commit first.
>>> * use correct framebuffer to ClearScreen.
>>> * revert VMWare svga definitions.
>>> 
>>> v2 -> v3
>>> * Update commit message (Thanks Laszlo)
>>> * Treat QEMU_VIDEO_VMWARE_SVGA as QEMU_VIDEO_BOCHS (Thanks Laszlo)
>>> 
>>> ---
>>>  OvmfPkg/QemuVideoDxe/Driver.c | 16 +++++++++++++++-
>>>  OvmfPkg/QemuVideoDxe/Gop.c    |  2 +-
>>>  OvmfPkg/QemuVideoDxe/Qemu.h   |  2 ++
>>>  3 files changed, 18 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c 
>>> b/OvmfPkg/QemuVideoDxe/Driver.c
>>> index 2304afd1e6..8e02700d39 100644
>>> --- a/OvmfPkg/QemuVideoDxe/Driver.c
>>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
>>> @@ -69,6 +69,12 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = {
>>>          0x1050,
>>>          QEMU_VIDEO_BOCHS_MMIO,
>>>          L"QEMU VirtIO VGA"
>>> +    },{
>>> +        PCI_CLASS_DISPLAY_VGA,
>>> +        0x15ad,
>>> +        0x0405,
>>> +        QEMU_VIDEO_VMWARE_SVGA,
>>> +        L"QEMU VMWare SVGA"
>>>      },{
>>>          0 /* end of list */
>>>      }
>>> @@ -316,6 +322,14 @@ QemuVideoControllerDriverStart (
>>>      }
>>>    }
>>> 
>>> +  //
>>> +  // VMWare SVGA is handled like Bochs (with port IO only).
>>> +  //
>>> +  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
>>> +    Private->Variant = QEMU_VIDEO_BOCHS;
>>> +    Private->FrameBufferVramBarIndex = PCI_BAR_IDX1;
>>> +  }
>>> +
>>>    //
>>>    // Check if accessing the bochs interface works.
>>>    //
>>> @@ -764,7 +778,7 @@ ClearScreen (
>>>    Private->PciIo->Mem.Write (
>>>                          Private->PciIo,
>>>                          EfiPciIoWidthFillUint32,
>>> -                        0,
>>> +                        Private->FrameBufferVramBarIndex,
>>>                          0,
>>>                          0x400000 >> 2,
>>>                          &Color
>>> diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
>>> index d490fa7a2e..6f542d9eac 100644
>>> --- a/OvmfPkg/QemuVideoDxe/Gop.c
>>> +++ b/OvmfPkg/QemuVideoDxe/Gop.c
>>> @@ -60,7 +60,7 @@ QemuVideoCompleteModeData (
>>> 
>>>    Private->PciIo->GetBarAttributes (
>>>                          Private->PciIo,
>>> -                        0,
>>> +                        Private->FrameBufferVramBarIndex,
>>>                          NULL,
>>>                          (VOID**) &FrameBufDesc
>>>                          );
>>> 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;
>>> 
>>>  ///
>>> 
>> 
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> 
>> I'm not pushing this patch at once, for two reasons:
>> 
>> - I should leave time for other reviewers to comment,
>> - we are now in the soft feature freeze period
>> <https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning>,
>> and this is not a bugfix, but a feature (re-)enablement that is only 
>> now
>> being reviewed.
>> 
>> I'll keep this tagged on my queue until after the edk2-stable201811 
>> tag
>> is pushed, and then I'll push this patch. Please do ping me, should I
>> forget.
> 
> I haven't forgotten. :)
> 
> * First, I've cherry-picked the first four patches (from the v2 series)
> from the git history. See
> <http://mid.mail-archive.com/3167b5fd-5514-5a79-e400-39d7c64e50ea@redhat.com>.
> 
> * Second, I've added PhilMD's R-b to those four patches. See
> <http://mid.mail-archive.com/a2bb9800-6e82-b552-a950-fa11377d24e1@redhat.com>.
> 
> * Third, PhilMD got busy with other stuff meanwhile, so I've tested the
> patch I'm replying to, myself. I have confirmed the following device
> models continue to work:
> - Cirrus 5446
> - QEMU Standard VGA
> - QEMU QXL VGA
> - QEMU VirtIO VGA
> 
> Therefore, for patch #5 (the present patch),
> 
> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
> 
> * Fourth, I've also determined that the patch enables OVMF to drive:
> - QEMU VMWare SVGA
> 
> therefore, for this patch,
> 
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> 
> * Fifth, the full series has been pushed as commit range
> 7f3b0bad4bbb..d021868ccf49.
> 
> 
> Thank you for the contribution, and also for your patience!
> Laszlo

Thank all for testing and all your effort.

YuChen