[PATCH v2 1/1] OvmfPkg/QemuVideoDxe: fix bochs mode init

[PATCH v2 1/1] OvmfPkg/QemuVideoDxe: fix bochs mode init

[Gerd Hoffmann]

Add VgaInb() helper function to read vga registers.  With that in place
fix the unblanking.  We need to put the ATT_ADDRESS_REGISTER flip flop
into a known state, which is done by reading the
INPUT_STATUS_1_REGISTER.  Reading the INPUT_STATUS_1_REGISTER only works
when the device is in color mode, so make sure that bit (0x01) is set in
MISC_OUTPUT_REGISTER.

Currently the mode setting works more by luck because
ATT_ADDRESS_REGISTER flip flip happens to be in the state we need.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/QemuVideoDxe/Driver.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
index b91909a14e59..a15e54d38fd2 100644
--- a/OvmfPkg/QemuVideoDxe/Driver.c
+++ b/OvmfPkg/QemuVideoDxe/Driver.c
@@ -984,6 +984,31 @@ VgaOutb (
   }
 }
 
+UINT8
+VgaInb (
+  QEMU_VIDEO_PRIVATE_DATA  *Private,
+  UINTN                    Reg
+  )
+{
+  EFI_STATUS  Status;
+  UINT8 Data = 0;
+
+  if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO) {
+    Status = Private->PciIo->Mem.Read (
+                                   Private->PciIo,
+                                   EfiPciIoWidthUint8,
+                                   PCI_BAR_IDX2,
+                                   0x400 - 0x3c0 + Reg,
+                                   1,
+                                   &Data
+                                   );
+    ASSERT_EFI_ERROR (Status);
+  } else {
+    Data = inb (Private, Reg);
+  }
+  return Data;
+}
+
 VOID
 InitializeBochsGraphicsMode (
   QEMU_VIDEO_PRIVATE_DATA  *Private,
@@ -998,7 +1023,11 @@ InitializeBochsGraphicsMode (
     ModeData->ColorDepth
     ));
 
-  /* unblank */
+  /* set color mode */
+  VgaOutb (Private, MISC_OUTPUT_REGISTER, 0x01);
+
+  /* reset flip flop + unblank */
+  VgaInb (Private, INPUT_STATUS_1_REGISTER);
   VgaOutb (Private, ATT_ADDRESS_REGISTER, 0x20);
 
   BochsWrite (Private, VBE_DISPI_INDEX_ENABLE, 0);
-- 
2.37.3

Re: [PATCH v2 1/1] OvmfPkg/QemuVideoDxe: fix bochs mode init

[Ard Biesheuvel]

On Wed, 7 Sept 2022 at 07:17, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Add VgaInb() helper function to read vga registers.  With that in place
> fix the unblanking.  We need to put the ATT_ADDRESS_REGISTER flip flop
> into a known state, which is done by reading the
> INPUT_STATUS_1_REGISTER.  Reading the INPUT_STATUS_1_REGISTER only works
> when the device is in color mode, so make sure that bit (0x01) is set in
> MISC_OUTPUT_REGISTER.
>
> Currently the mode setting works more by luck because
> ATT_ADDRESS_REGISTER flip flip happens to be in the state we need.
>

flip flip

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Unfortunately, it appears your patch is terminally flawed, and needs
the following change applied to fix it:

Files with formatting errors:
 Formatting errors in QemuVideoDxe\Driver.c
 --- D:\a\1\s\OvmfPkg\QemuVideoDxe\Driver.c
 +++ D:\a\1\s\OvmfPkg\QemuVideoDxe\Driver.c.uncrustify_plugin
 @@ -991,7 +991,7 @@
 )
 {
 EFI_STATUS  Status;
 -  UINT8 Data = 0;
 +  UINT8       Data = 0;

 if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO) {
 Status = Private->PciIo->Mem.Read (
 @@ -1006,6 +1006,7 @@
 } else {
 Data = inb (Private, Reg);
 }
 +
 return Data;
 }

Interestingly, uncrustify happily accepts the = 0 initializer for
Data, even though the coding style does not permit this, so you might
hit a EccCheck error as well.





> ---
>  OvmfPkg/QemuVideoDxe/Driver.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> index b91909a14e59..a15e54d38fd2 100644
> --- a/OvmfPkg/QemuVideoDxe/Driver.c
> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
> @@ -984,6 +984,31 @@ VgaOutb (
>    }
>  }
>
> +UINT8
> +VgaInb (
> +  QEMU_VIDEO_PRIVATE_DATA  *Private,
> +  UINTN                    Reg
> +  )
> +{
> +  EFI_STATUS  Status;
> +  UINT8 Data = 0;
> +
> +  if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO) {
> +    Status = Private->PciIo->Mem.Read (
> +                                   Private->PciIo,
> +                                   EfiPciIoWidthUint8,
> +                                   PCI_BAR_IDX2,
> +                                   0x400 - 0x3c0 + Reg,
> +                                   1,
> +                                   &Data
> +                                   );
> +    ASSERT_EFI_ERROR (Status);
> +  } else {
> +    Data = inb (Private, Reg);
> +  }
> +  return Data;
> +}
> +
>  VOID
>  InitializeBochsGraphicsMode (
>    QEMU_VIDEO_PRIVATE_DATA  *Private,
> @@ -998,7 +1023,11 @@ InitializeBochsGraphicsMode (
>      ModeData->ColorDepth
>      ));
>
> -  /* unblank */
> +  /* set color mode */
> +  VgaOutb (Private, MISC_OUTPUT_REGISTER, 0x01);
> +
> +  /* reset flip flop + unblank */
> +  VgaInb (Private, INPUT_STATUS_1_REGISTER);
>    VgaOutb (Private, ATT_ADDRESS_REGISTER, 0x20);
>
>    BochsWrite (Private, VBE_DISPI_INDEX_ENABLE, 0);
> --
> 2.37.3
>