From: yuchenlin <yuchenlin@synology.com>
To: Laszlo Ersek <lersek@redhat.com>
Cc: edk2-devel@lists.01.org, phil@philjordan.eu,
jordan.l.justen@intel.com, anthony.perard@citrix.com,
"Philippe Mathieu-Daudé" <philmd@redhat.com>,
"Gerd Hoffmann" <kraxel@redhat.com>
Subject: Re: [PATCH v2 5/5] OvmfPkg: simply use the Bochs interface for vmsvga
Date: Wed, 07 Nov 2018 10:36:16 +0800 [thread overview]
Message-ID: <72357c4c380cfa491076c1baa8e8ce71@synology.com> (raw)
In-Reply-To: <4d3da28e-42ca-c54e-1efa-c6fc8c9b0fb2@redhat.com>
On 2018-11-06 19:47, Laszlo Ersek wrote:
> I suggest the following:
>
> On 11/02/18 04:24, yuchenlin via edk2-devel 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
>
> [a] The commit message should be udpated as follows:
>
> - 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: both of the currently
> listed reasons, plus "prevent mis-recognizing VMW SVGA as QXL" as the
> third one.
>
Will do.
Thanks,
yuchenlin
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: yuchenlin <yuchenlin@synology.com>
>> ---
>> OvmfPkg/QemuVideoDxe/Driver.c | 18 ++++++++++++++++--
>> OvmfPkg/QemuVideoDxe/Gop.c | 3 ++-
>> OvmfPkg/QemuVideoDxe/Qemu.h | 2 ++
>> 3 files changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c
>> b/OvmfPkg/QemuVideoDxe/Driver.c
>> index 2304afd1e6..76d4a2d27e 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 */
>> }
>> @@ -256,6 +262,12 @@ QemuVideoControllerDriverStart (
>> goto ClosePciIo;
>> }
>> Private->Variant = Card->Variant;
>> + if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
>> + Private->FrameBufferVramBarIndex = PCI_BAR_IDX1;
>> + } else {
>> + Private->FrameBufferVramBarIndex = PCI_BAR_IDX0;
>> + }
>> +
>>
>> //
>> // 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:
>> @@ -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..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,
>> 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;
>>
>> ///
>>
>
> [b] How about the following -- incremental, to be squashed -- patch:
>
>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c
>> b/OvmfPkg/QemuVideoDxe/Driver.c
>> index 76d4a2d27e7e..8e02700d3960 100644
>> --- a/OvmfPkg/QemuVideoDxe/Driver.c
>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
>> @@ -262,12 +262,6 @@ QemuVideoControllerDriverStart (
>> goto ClosePciIo;
>> }
>> Private->Variant = Card->Variant;
>> - if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
>> - Private->FrameBufferVramBarIndex = PCI_BAR_IDX1;
>> - } else {
>> - Private->FrameBufferVramBarIndex = PCI_BAR_IDX0;
>> - }
>> -
>>
>> //
>> // IsQxl is based on the detected Card->Variant, which at a later
>> point
>> might
>> @@ -328,12 +322,19 @@ 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.
>> //
>> if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO ||
>> - Private->Variant == QEMU_VIDEO_BOCHS ||
>> - Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
>> + Private->Variant == QEMU_VIDEO_BOCHS) {
>> UINT16 BochsId;
>> BochsId = BochsRead(Private, VBE_DISPI_INDEX_ID);
>> if ((BochsId & 0xFFF0) != VBE_DISPI_ID0) {
>> @@ -396,7 +397,6 @@ 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 3abc5eeb36a6..6f542d9eac05 100644
>> --- a/OvmfPkg/QemuVideoDxe/Gop.c
>> +++ b/OvmfPkg/QemuVideoDxe/Gop.c
>> @@ -177,7 +177,6 @@ Routine Description:
>> break;
>> case QEMU_VIDEO_BOCHS_MMIO:
>> case QEMU_VIDEO_BOCHS:
>> - case QEMU_VIDEO_VMWARE_SVGA:
>> InitializeBochsGraphicsMode (Private,
>> &QemuVideoBochsModes[ModeData->InternalModeIndex]);
>> break;
>> default:
>
> This would produce the following -- squashed -- patch, for v3:
>
>> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
>> index d7da761705a1..3aac9eeca687 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;
>>
>> ///
>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c
>> b/OvmfPkg/QemuVideoDxe/Driver.c
>> index 2304afd1e686..8e02700d3960 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 d490fa7a2e6f..6f542d9eac05 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
>> );
>
> For me this is much easier to understand.
I agree. Will do
Thanks,
yuchenlin
>
> ... While we discuss this, I'll go ahead and push the first four
> patches. The code being reverted is dead anyway. I'll report back about
> the commit hashes.
>
> Thanks,
> Laszlo
next prev parent reply other threads:[~2018-11-07 2:36 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-02 3:23 [PATCH v2 0/5] OvmfPkg: simply use the Bochs interface for vmsvga yuchenlin
2018-11-02 3:23 ` [PATCH v2 1/5] Revert "OvmfPkg/QemuVideoDxe: list "UnalignedIoInternal.h" in the INF file" yuchenlin
2018-11-06 10:03 ` Laszlo Ersek
2018-11-02 3:23 ` [PATCH v2 2/5] Revert "OvmfPkg/QemuVideoDxe: VMWare SVGA device support" yuchenlin
2018-11-06 10:36 ` Laszlo Ersek
2018-11-02 3:24 ` [PATCH v2 3/5] Revert "OvmfPkg/QemuVideoDxe: Helper functions for unaligned port I/O." yuchenlin
2018-11-06 10:44 ` Laszlo Ersek
2018-11-02 3:24 ` [PATCH v2 4/5] Revert "OvmfPkg: VMWare SVGA display device register definitions" yuchenlin
2018-11-06 10:48 ` Laszlo Ersek
2018-11-02 3:24 ` [PATCH v2 5/5] OvmfPkg: simply use the Bochs interface for vmsvga yuchenlin
2018-11-06 11:47 ` Laszlo Ersek
2018-11-06 13:36 ` Laszlo Ersek
2018-11-06 13:44 ` Philippe Mathieu-Daudé
2018-11-06 16:20 ` Laszlo Ersek
2018-11-07 2:37 ` yuchenlin
2018-11-12 11:19 ` Laszlo Ersek
2018-11-12 11:28 ` Philippe Mathieu-Daudé
2018-11-12 12:21 ` Laszlo Ersek
2018-11-07 2:36 ` yuchenlin [this message]
2018-11-05 22:52 ` [PATCH v2 0/5] " Laszlo Ersek
2018-11-12 14:16 ` Philippe Mathieu-Daudé
2018-11-12 15:40 ` Laszlo Ersek
2018-11-15 11:56 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=72357c4c380cfa491076c1baa8e8ce71@synology.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox