Re: [PATCH 00/11] OvmfPkg, ArmVirtPkg: GOP driver for the VirtIo GPU (virtio-gpu-pci)

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 19 August 2016 at 14:49, Laszlo Ersek <lersek@redhat.com> wrote:
> This series solves
> <https://tianocore.acgmultimedia.com/show_bug.cgi?id=66>. In particular,
> it gives AARCH64 guests running on KVM a clean, uncorrupted graphical
> console.
>

Impressive! I suppose this means no direct frame buffer access for the
OS using the GOP? That is fine with me, btw, after finding out that
VGA is really the only problematic QEMU/KVM device (unlike the
reported USB issues, which were solved by making the PCI RC
dma-coherent in the DT), I think this approach is the best solution,
since OS accessing the GOP is a hack anyway (and breaks with the PCI
reconfiguration that occurs under ARM/Linux, even in the ACPI case,
which I expected would leave the firmware PCI setup alone. /me makes
mental note to revert the 'pci-probe-only' patch)

> Anatomy of the series:
>
> - Patches 01 and 02 fix small bugs in QemuVideoDxe and Virtio10Dxe, so
>   that virtio-vga is bound only by QemuVideoDxe, and virtio-gpu-pci is
>   bound only by Virtio10Dxe (and then, on top, by the driver being
>   posted).
>
> - Patch 03 makes OVMF' BDS pick up virtio-gpu-pci automatically as
>   ConOut. (ArmVirtPkg already has the necessary code.) A small tweak.
>
> - Patch 04 adds the VirtIo GPU stuff to IndustryStandard, using Gerd's
>   WIP additions to the VirtIo 1.0 spec, at
>   <https://www.kraxel.org/virtio>. (See the "GPU Device" section.) The
>   URLs are also captured in the code.
>
> - Patches 05 through 07 add the basic (= UEFI driver model) skeleton of
>   the driver, and include it in OvmfPkg and ArmVirtPkg.
>
>   In this series I managed to erect the driver in such a way that the
>   series doesn't only build at every stage, the driver even runs at
>   every stage (and the then-present functionality is testable). I used
>   this to test each layer of functionality in separation, during
>   development.
>
> - Patch 08 implements the basic VirtIo GPU initialization.
>
> - Patch 09 implements VirtIo GPU command primitives ("remote procedure
>   calls"), for performing graphics operations.
>
> - Patch 10 implements the Graphics Output Protocol.
>
> - Patch 11 drops PcdKludgeMapPciMmioAsCached from ArmVirtPkg.
>
> Tests done:
>
> - Virtio-vga regression testing on x86_64 KVM (OvmfPkgIa32X64 build).
>
> - Repeated "disconnect" and "connect -r" checks in the UEFI shell,
>   issued (also) from the serial console (ArmVirtQemu build on AARCH64
>   KVM, OvmfPkgIa32X64 build on x86_64 KVM).
>
> - Repeated mode switches with the "mode" command in the UEFI shell (same
>   platforms).
>
> - Changing the preferred resolution in the Device Manager, then
>   verifying with "mode" in the UEFI shell after reboot (same platforms).
>
> - Tested GRUB, using Fedora 23 / Fedora 24 installer ISOs (both
>   platforms).
>

OK, so grub drives the framebuffer using Blt(), also on ARM? That is lovely!

> - Also tested loading and launching Linux from GRUB. Here the results
>   differ of course: x86_64 Fedora 24 drives virtio-gpu-pci with its
>   native driver without problems, whereas AARCH64 Fedora 23 doesn't even
>   look for virtio-gpu-pci, apparently.
>
> - Standard VGA continues to work in AARCH64 guests with TCG (tested on
>   x86_64 host). In addition, I've verified that the display corruption
>   readily reproduces when using standard VGA in AARCH64/KVM guests, at
>   the end of the series.
>

Indeed. So on ARM, we should drive virtio-gpu in non-VGA mode, using
Blt() equivalent calls.
We should probably propose some patches for Linux then, to make this
the default (or simply the only supported mode)

> - Verified Driver Name and Controller Name in the UEFI shell, issuing
>   "devices", "drivers", and "dh" commands.
>
>   Interestingly, they work perfectly with OVMF, but in ArmVirtQemu, the
>   formatted names are not displayed for *any* driver or device. Likely a
>   general problem with the ArmVirtQemu (or more generally, AARCH64)
>   build of the shell.
>
>   I also ran into a couple of UEFI shell crashes with "devtree" and "dh
>   -d -v" (when listing all devices in the system). However, those
>   crashes reproduce identically when the series is not applied.
>
> Testing instructions:
>
> * OVMF guests: just add
>
>     -device virtio-gpu-pci
>
>   to the QEMU command line.
>
>   For the host display, I prefer SDL when I use the raw QEMU command
>   line:
>
>     -display sdl
>
>   but that's entirely up to the user.
>
> * ArmVirtQemu guests: again, add
>
>     -device virtio-gpu-pci
>
>   I ran my AARCH64/KVM tests on my Mustang, and for host display, there
>   I prefer VNC (when using the raw QEMU command line):
>
>     -display vnc=:0
>
>   Connect with "vncviewer" or another VNC client, optionally forwarded
>   over SSH.
>
>   If you'd even like to type on the graphical console, I recommend
>   adding a USB 3.0 keyboard:
>
>     -device nec-usb-xhci -device usb-kbd
>
> * On both guest arches, check out the nice graphical progress bar (five
>   seconds, and you can enter the Setup utility with F2 or ESC):
>
>     -boot menu=on,splash-time=5000
>
>
> ... This got to be one of my longest blurbs. Sorry about that :)
>
> Ard: I haven't forgotten about the beer you promised! ;)
>

Next time we meet face to face, beers are on me! In fact, there were
some rumors about having our Linaro conference in Budapest next
spring, so I will make sure to put you on the guest list if that
happens.

BTW, your patches look a bit white space impaired to me, i.e., I get
blank lines between each line of patch content.

-- 
Ard.


> Public branch: <https://github.com/lersek/edk2/commits/virtio_gpu>.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (11):
>   OvmfPkg/QemuVideoDxe: don't incorrectly bind virtio-gpu-pci
>   OvmfPkg/Virtio10Dxe: don't bind virtio-vga
>   OvmfPkg/PlatformBootManagerLib: relax device class requirement for
>     ConOut
>   OvmfPkg/IndustryStandard: add type definitions for the virtio GPU
>     device
>   OvmfPkg/VirtioGpuDxe: introduce with Component Name 2 and Driver
>     Binding
>   OvmfPkg: include VirtioGpuDxe in the platform DSC/FDF files
>   ArmVirtPkg/ArmVirtQemu: include VirtioGpuDxe in the platform DSC/FDF
>     files
>   OvmfPkg/VirtioGpuDxe: initialize and tear down VirtIo GPU device
>   OvmfPkg/VirtioGpuDxe: provide functions for sending VirtIo GPU
>     commands
>   OvmfPkg/VirtioGpuDxe: implement EFI_GRAPHICS_OUTPUT_PROTOCOL
>   ArmVirtPkg: remove PcdKludgeMapPciMmioAsCached
>
>  ArmVirtPkg/ArmVirtPkg.dec                            |  24 -
>  ArmVirtPkg/ArmVirtQemu.dsc                           |   4 +-
>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc                 |   1 +
>  ArmVirtPkg/ArmVirtQemuKernel.dsc                     |   4 +-
>  ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c          |   3 +-
>  ArmVirtPkg/PciHostBridgeDxe/PciHostBridgeDxe.inf     |   1 -
>  OvmfPkg/Include/IndustryStandard/Virtio10.h          |   5 +
>  OvmfPkg/Include/IndustryStandard/VirtioGpu.h         | 216 +++++
>  OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c |  10 +-
>  OvmfPkg/OvmfPkgIa32.dsc                              |   1 +
>  OvmfPkg/OvmfPkgIa32.fdf                              |   1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                           |   1 +
>  OvmfPkg/OvmfPkgIa32X64.fdf                           |   1 +
>  OvmfPkg/OvmfPkgX64.dsc                               |   1 +
>  OvmfPkg/OvmfPkgX64.fdf                               |   1 +
>  OvmfPkg/QemuVideoDxe/Driver.c                        |   3 +
>  OvmfPkg/Virtio10Dxe/Virtio10.c                       |  18 +-
>  OvmfPkg/VirtioGpuDxe/Commands.c                      | 559 +++++++++++++
>  OvmfPkg/VirtioGpuDxe/DriverBinding.c                 | 844 ++++++++++++++++++++
>  OvmfPkg/VirtioGpuDxe/Gop.c                           | 647 +++++++++++++++
>  OvmfPkg/VirtioGpuDxe/VirtioGpu.h                     | 327 ++++++++
>  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf                   |  51 ++
>  22 files changed, 2682 insertions(+), 41 deletions(-)
>  create mode 100644 OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
>  create mode 100644 OvmfPkg/Include/IndustryStandard/VirtioGpu.h
>  create mode 100644 OvmfPkg/VirtioGpuDxe/VirtioGpu.h
>  create mode 100644 OvmfPkg/VirtioGpuDxe/Commands.c
>  create mode 100644 OvmfPkg/VirtioGpuDxe/DriverBinding.c
>  create mode 100644 OvmfPkg/VirtioGpuDxe/Gop.c
>
> --
> 2.9.2
>