From: Laszlo Ersek <lersek@redhat.com>
To: yuchenlin@synology.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: Tue, 6 Nov 2018 12:47:46 +0100 [thread overview]
Message-ID: <4d3da28e-42ca-c54e-1efa-c6fc8c9b0fb2@redhat.com> (raw)
In-Reply-To: <20181102032402.19686-6-yuchenlin@synology.com>
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.
>
> 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.
... 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-06 11:48 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 [this message]
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
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=4d3da28e-42ca-c54e-1efa-c6fc8c9b0fb2@redhat.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