[PATCH 0/5] OvmfPkg/QemuVideoDxe: cleanups and tweaks

[PATCH 0/5] OvmfPkg/QemuVideoDxe: cleanups and tweaks

[Laszlo Ersek]

Reviewing (part of) Ray's recent FrameBufferBltLib changes gave me the
idea for this. The patches are really small.

Public branch: <https://github.com/lersek/edk2/commits/qemu_video_2>.

Cc: Jordan Justen <jordan.l.justen@intel.com>

Thanks
Laszlo

Laszlo Ersek (5):
  OvmfPkg/QemuVideoDxe: drop useless QEMU_VIDEO_MODE_DATA.RefreshRate
  OvmfPkg/QemuVideoDxe: drop QEMU_VIDEO_CIRRUS_MODES.RefreshRate
  OvmfPkg/QemuVideoDxe: remove useless
    QEMU_VIDEO_PRIVATE_DATA.CurrentMode
  OvmfPkg/QemuVideoDxe: remove useless
    QEMU_VIDEO_PRIVATE_DATA.LineBuffer
  OvmfPkg/QemuVideoDxe: upgrade VERBOSE debug messages to INFO

 OvmfPkg/QemuVideoDxe/Qemu.h       |  9 ++----
 OvmfPkg/QemuVideoDxe/Gop.c        | 16 -----------
 OvmfPkg/QemuVideoDxe/Initialize.c | 30 +++++++++-----------
 OvmfPkg/QemuVideoDxe/VbeShim.c    |  4 +--
 4 files changed, 17 insertions(+), 42 deletions(-)

-- 
2.9.2

[PATCH 1/5] OvmfPkg/QemuVideoDxe: drop useless QEMU_VIDEO_MODE_DATA.RefreshRate

[Laszlo Ersek]

This field is never used beyond assignment and debug-logging. Remove it.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/QemuVideoDxe/Qemu.h       |  1 -
 OvmfPkg/QemuVideoDxe/Initialize.c | 12 ++++--------
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
index 5844bc404cef..7c1f80c772e3 100644
--- a/OvmfPkg/QemuVideoDxe/Qemu.h
+++ b/OvmfPkg/QemuVideoDxe/Qemu.h
@@ -54,11 +54,10 @@
 typedef struct {
   UINT32  InternalModeIndex; // points into card-specific mode table
   UINT32  HorizontalResolution;
   UINT32  VerticalResolution;
   UINT32  ColorDepth;
-  UINT32  RefreshRate;
 } QEMU_VIDEO_MODE_DATA;
 
 #define PIXEL_RED_SHIFT   0
 #define PIXEL_GREEN_SHIFT 3
 #define PIXEL_BLUE_SHIFT  6
diff --git a/OvmfPkg/QemuVideoDxe/Initialize.c b/OvmfPkg/QemuVideoDxe/Initialize.c
index 9e0c3aa09fd6..b12253ba5892 100644
--- a/OvmfPkg/QemuVideoDxe/Initialize.c
+++ b/OvmfPkg/QemuVideoDxe/Initialize.c
@@ -184,19 +184,17 @@ QemuVideoCirrusModeSetup (
   for (Index = 0; Index < QEMU_VIDEO_CIRRUS_MODE_COUNT; Index ++) {
     ModeData->InternalModeIndex = Index;
     ModeData->HorizontalResolution          = VideoMode->Width;
     ModeData->VerticalResolution            = VideoMode->Height;
     ModeData->ColorDepth                    = VideoMode->ColorDepth;
-    ModeData->RefreshRate                   = VideoMode->RefreshRate;
     DEBUG ((EFI_D_INFO,
-      "Adding Mode %d as Cirrus Internal Mode %d: %dx%d, %d-bit, %d Hz\n",
+      "Adding Mode %d as Cirrus Internal Mode %d: %dx%d, %d-bit\n",
       (INT32) (ModeData - Private->ModeData),
       ModeData->InternalModeIndex,
       ModeData->HorizontalResolution,
       ModeData->VerticalResolution,
-      ModeData->ColorDepth,
-      ModeData->RefreshRate
+      ModeData->ColorDepth
       ));
 
     ModeData ++ ;
     VideoMode ++;
   }
@@ -328,19 +326,17 @@ QemuVideoBochsModeSetup (
     if (RequiredFbSize <= AvailableFbSize) {
       ModeData->InternalModeIndex    = Index;
       ModeData->HorizontalResolution = VideoMode->Width;
       ModeData->VerticalResolution   = VideoMode->Height;
       ModeData->ColorDepth           = VideoMode->ColorDepth;
-      ModeData->RefreshRate          = 60;
       DEBUG ((EFI_D_INFO,
-        "Adding Mode %d as Bochs Internal Mode %d: %dx%d, %d-bit, %d Hz\n",
+        "Adding Mode %d as Bochs Internal Mode %d: %dx%d, %d-bit\n",
         (INT32) (ModeData - Private->ModeData),
         ModeData->InternalModeIndex,
         ModeData->HorizontalResolution,
         ModeData->VerticalResolution,
-        ModeData->ColorDepth,
-        ModeData->RefreshRate
+        ModeData->ColorDepth
         ));
 
       ModeData ++ ;
     }
     VideoMode ++;
-- 
2.9.2

[PATCH 2/5] OvmfPkg/QemuVideoDxe: drop QEMU_VIDEO_CIRRUS_MODES.RefreshRate

[Laszlo Ersek]

Thanks to the previous patch, this field is also unnecessary now. Remove
it.

The patch is best reviewed with "git show --word-diff".

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/QemuVideoDxe/Qemu.h       |  1 -
 OvmfPkg/QemuVideoDxe/Initialize.c | 16 ++++++++--------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
index 7c1f80c772e3..1246b74d4955 100644
--- a/OvmfPkg/QemuVideoDxe/Qemu.h
+++ b/OvmfPkg/QemuVideoDxe/Qemu.h
@@ -129,11 +129,10 @@ typedef struct {
 ///
 typedef struct {
   UINT32  Width;
   UINT32  Height;
   UINT32  ColorDepth;
-  UINT32  RefreshRate;
   UINT8   *CrtcSettings;
   UINT16  *SeqSettings;
   UINT8   MiscSetting;
 } QEMU_VIDEO_CIRRUS_MODES;
 
diff --git a/OvmfPkg/QemuVideoDxe/Initialize.c b/OvmfPkg/QemuVideoDxe/Initialize.c
index b12253ba5892..de74fd65ec67 100644
--- a/OvmfPkg/QemuVideoDxe/Initialize.c
+++ b/OvmfPkg/QemuVideoDxe/Initialize.c
@@ -142,18 +142,18 @@ UINT16 Seq_1024_768_32bpp_60[15] = {
 
 ///
 /// Table of supported video modes
 ///
 QEMU_VIDEO_CIRRUS_MODES  QemuVideoCirrusModes[] = {
-//  {  640, 480, 8, 60, Crtc_640_480_256_60,  Seq_640_480_256_60,  0xe3 },
-//  {  800, 600, 8, 60, Crtc_800_600_256_60,  Seq_800_600_256_60,  0xef },
-  {  640, 480, 32, 60, Crtc_640_480_32bpp_60,  Seq_640_480_32bpp_60,  0xef },
-  {  800, 600, 32, 60, Crtc_800_600_32bpp_60,  Seq_800_600_32bpp_60,  0xef },
-//  { 1024, 768, 8, 60, Crtc_1024_768_256_60, Seq_1024_768_256_60, 0xef }
-  { 1024, 768, 24, 60, Crtc_1024_768_24bpp_60, Seq_1024_768_24bpp_60, 0xef }
-//  { 1024, 768, 32, 60, Crtc_1024_768_32bpp_60, Seq_1024_768_32bpp_60, 0xef }
-//  { 960, 720, 32, 60, Crtc_960_720_32bpp_60, Seq_1024_768_32bpp_60, 0xef }
+//  {  640, 480, 8, Crtc_640_480_256_60,  Seq_640_480_256_60,  0xe3 },
+//  {  800, 600, 8, Crtc_800_600_256_60,  Seq_800_600_256_60,  0xef },
+  {  640, 480, 32, Crtc_640_480_32bpp_60,  Seq_640_480_32bpp_60,  0xef },
+  {  800, 600, 32, Crtc_800_600_32bpp_60,  Seq_800_600_32bpp_60,  0xef },
+//  { 1024, 768, 8, Crtc_1024_768_256_60, Seq_1024_768_256_60, 0xef }
+  { 1024, 768, 24, Crtc_1024_768_24bpp_60, Seq_1024_768_24bpp_60, 0xef }
+//  { 1024, 768, 32, Crtc_1024_768_32bpp_60, Seq_1024_768_32bpp_60, 0xef }
+//  { 960, 720, 32, Crtc_960_720_32bpp_60, Seq_1024_768_32bpp_60, 0xef }
 };
 
 #define QEMU_VIDEO_CIRRUS_MODE_COUNT \
   (sizeof (QemuVideoCirrusModes) / sizeof (QemuVideoCirrusModes[0]))
 
-- 
2.9.2

[PATCH 3/5] OvmfPkg/QemuVideoDxe: remove useless QEMU_VIDEO_PRIVATE_DATA.CurrentMode

[Laszlo Ersek]

This field is never used.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/QemuVideoDxe/Qemu.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
index 1246b74d4955..f3358d8a7ce0 100644
--- a/OvmfPkg/QemuVideoDxe/Qemu.h
+++ b/OvmfPkg/QemuVideoDxe/Qemu.h
@@ -108,15 +108,13 @@ typedef struct {
   UINT64                                OriginalPciAttributes;
   EFI_GRAPHICS_OUTPUT_PROTOCOL          GraphicsOutput;
   EFI_DEVICE_PATH_PROTOCOL              *GopDevicePath;
 
   //
-  // The next three fields match the client-visible
-  // EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE.Mode and
-  // EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE.MaxMode fields.
+  // The next two fields match the client-visible
+  // EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE.MaxMode field.
   //
-  UINTN                                 CurrentMode;
   UINTN                                 MaxMode;
   QEMU_VIDEO_MODE_DATA                  *ModeData;
 
   UINT8                                 *LineBuffer;
   QEMU_VIDEO_VARIANT                    Variant;
-- 
2.9.2

[PATCH 4/5] OvmfPkg/QemuVideoDxe: remove useless QEMU_VIDEO_PRIVATE_DATA.LineBuffer

[Laszlo Ersek]

This field is (re)allocated in QemuVideoGraphicsOutputSetMode(), released
in QemuVideoGraphicsOutputDestructor(), and used for nothing else. Remove
it.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/QemuVideoDxe/Qemu.h |  1 -
 OvmfPkg/QemuVideoDxe/Gop.c  | 16 ----------------
 2 files changed, 17 deletions(-)

diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
index f3358d8a7ce0..2ce37defc5b8 100644
--- a/OvmfPkg/QemuVideoDxe/Qemu.h
+++ b/OvmfPkg/QemuVideoDxe/Qemu.h
@@ -114,11 +114,10 @@ typedef struct {
   // EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE.MaxMode field.
   //
   UINTN                                 MaxMode;
   QEMU_VIDEO_MODE_DATA                  *ModeData;
 
-  UINT8                                 *LineBuffer;
   QEMU_VIDEO_VARIANT                    Variant;
   FRAME_BUFFER_CONFIGURE                *FrameBufferBltConfigure;
   UINTN                                 FrameBufferBltConfigureSize;
 } QEMU_VIDEO_PRIVATE_DATA;
 
diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
index 532f20e6446c..5485ba3c2871 100644
--- a/OvmfPkg/QemuVideoDxe/Gop.c
+++ b/OvmfPkg/QemuVideoDxe/Gop.c
@@ -165,19 +165,10 @@ Routine Description:
     return EFI_UNSUPPORTED;
   }
 
   ModeData = &Private->ModeData[ModeNumber];
 
-  if (Private->LineBuffer) {
-    gBS->FreePool (Private->LineBuffer);
-  }
-
-  Private->LineBuffer = AllocatePool (4 * ModeData->HorizontalResolution);
-  if (Private->LineBuffer == NULL) {
-    return EFI_OUT_OF_RESOURCES;
-  }
-
   switch (Private->Variant) {
   case QEMU_VIDEO_CIRRUS_5430:
   case QEMU_VIDEO_CIRRUS_5446:
     InitializeCirrusGraphicsMode (Private, &QemuVideoCirrusModes[ModeData->InternalModeIndex]);
     break;
@@ -185,12 +176,10 @@ Routine Description:
   case QEMU_VIDEO_BOCHS:
     InitializeBochsGraphicsMode (Private, &QemuVideoBochsModes[ModeData->InternalModeIndex]);
     break;
   default:
     ASSERT (FALSE);
-    gBS->FreePool (Private->LineBuffer);
-    Private->LineBuffer = NULL;
     return EFI_DEVICE_ERROR;
   }
 
   This->Mode->Mode = ModeNumber;
   This->Mode->Info->HorizontalResolution = ModeData->HorizontalResolution;
@@ -347,11 +336,10 @@ QemuVideoGraphicsOutputConstructor (
   if (EFI_ERROR (Status)) {
     goto FreeMode;
   }
   Private->GraphicsOutput.Mode->MaxMode = (UINT32) Private->MaxMode;
   Private->GraphicsOutput.Mode->Mode    = GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER;
-  Private->LineBuffer                   = NULL;
   Private->FrameBufferBltConfigure      = NULL;
   Private->FrameBufferBltConfigureSize  = 0;
 
   //
   // Initialize the hardware
@@ -393,14 +381,10 @@ Returns:
 
   None
 
 --*/
 {
-  if (Private->LineBuffer != NULL) {
-    FreePool (Private->LineBuffer);
-  }
-
   if (Private->FrameBufferBltConfigure != NULL) {
     FreePool (Private->FrameBufferBltConfigure);
   }
 
   if (Private->GraphicsOutput.Mode != NULL) {
-- 
2.9.2

[PATCH 5/5] OvmfPkg/QemuVideoDxe: upgrade VERBOSE debug messages to INFO

[Laszlo Ersek]

In commit 5b2291f9567a ("OvmfPkg: QemuVideoDxe uses
MdeModulePkg/FrameBufferLib"), QemuVideoDxe was rebased to
FrameBufferBltLib.

The FrameBufferBltLib instance added in commit b1ca386074bd
("MdeModulePkg: Add FrameBufferBltLib library instance") logs many
messages on the VERBOSE level; for example, a normal boot with OVMF can
produce 500+ "VideoFill" messages, dependent on the progress bar, when the
VERBOSE bit is set in PcdDebugPrintErrorLevel. While FrameBufferBltLib is
certainly allowed to log such messages on the VERBOSE level, we should
separate those frequent messages from the (infrequent) ones produced by
QemuVideoDxe itself.

QemuVideoDxe logs VERBOSE messages in three locations (in two functions)
at the moment. All of them are infrequent: both QemuVideoBochsModeSetup()
and InstallVbeShim() are called from QemuVideoControllerDriverStart(),
that is, when a device is bound. Upgrade these messages to INFO level, so
that VERBOSE can be disabled in PcdDebugPrintErrorLevel -- perhaps
selectively for OvmfPkg/QemuVideoDxe -- without hiding these infrequent
messages.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/QemuVideoDxe/Initialize.c | 2 +-
 OvmfPkg/QemuVideoDxe/VbeShim.c    | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/QemuVideoDxe/Initialize.c b/OvmfPkg/QemuVideoDxe/Initialize.c
index de74fd65ec67..98ca4f8488fe 100644
--- a/OvmfPkg/QemuVideoDxe/Initialize.c
+++ b/OvmfPkg/QemuVideoDxe/Initialize.c
@@ -301,11 +301,11 @@ QemuVideoBochsModeSetup (
     }
   } else {
     AvailableFbSize  = BochsRead (Private, VBE_DISPI_INDEX_VIDEO_MEMORY_64K);
     AvailableFbSize *= SIZE_64KB;
   }
-  DEBUG ((EFI_D_VERBOSE, "%a: AvailableFbSize=0x%x\n", __FUNCTION__,
+  DEBUG ((EFI_D_INFO, "%a: AvailableFbSize=0x%x\n", __FUNCTION__,
     AvailableFbSize));
 
   //
   // Setup Video Modes
   //
diff --git a/OvmfPkg/QemuVideoDxe/VbeShim.c b/OvmfPkg/QemuVideoDxe/VbeShim.c
index 6a569f627449..4c4517e9da27 100644
--- a/OvmfPkg/QemuVideoDxe/VbeShim.c
+++ b/OvmfPkg/QemuVideoDxe/VbeShim.c
@@ -98,20 +98,20 @@ InstallVbeShim (
     // shouldn't override a real video BIOS with our shim, nor our own shim if
     // it's already present.
     //
     Handler = (Int0x10->Segment << 4) + Int0x10->Offset;
     if (Handler >= SegmentC && Handler < SegmentF) {
-      DEBUG ((EFI_D_VERBOSE, "%a: Video BIOS handler found at %04x:%04x\n",
+      DEBUG ((EFI_D_INFO, "%a: Video BIOS handler found at %04x:%04x\n",
         __FUNCTION__, Int0x10->Segment, Int0x10->Offset));
       return;
     }
 
     //
     // Otherwise we'll overwrite the Int10h vector, even though we may not own
     // the page at zero.
     //
-    DEBUG ((EFI_D_VERBOSE, "%a: failed to allocate page at zero: %r\n",
+    DEBUG ((EFI_D_INFO, "%a: failed to allocate page at zero: %r\n",
       __FUNCTION__, Status));
   } else {
     //
     // We managed to allocate the page at zero. SVN r14218 guarantees that it
     // is NUL-filled.
-- 
2.9.2

Re: [PATCH 0/5] OvmfPkg/QemuVideoDxe: cleanups and tweaks

[Jordan Justen]

Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

On 2016-10-12 03:09:00, Laszlo Ersek wrote:
> Reviewing (part of) Ray's recent FrameBufferBltLib changes gave me the
> idea for this. The patches are really small.
> 
> Public branch: <https://github.com/lersek/edk2/commits/qemu_video_2>.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (5):
>   OvmfPkg/QemuVideoDxe: drop useless QEMU_VIDEO_MODE_DATA.RefreshRate
>   OvmfPkg/QemuVideoDxe: drop QEMU_VIDEO_CIRRUS_MODES.RefreshRate
>   OvmfPkg/QemuVideoDxe: remove useless
>     QEMU_VIDEO_PRIVATE_DATA.CurrentMode
>   OvmfPkg/QemuVideoDxe: remove useless
>     QEMU_VIDEO_PRIVATE_DATA.LineBuffer
>   OvmfPkg/QemuVideoDxe: upgrade VERBOSE debug messages to INFO
> 
>  OvmfPkg/QemuVideoDxe/Qemu.h       |  9 ++----
>  OvmfPkg/QemuVideoDxe/Gop.c        | 16 -----------
>  OvmfPkg/QemuVideoDxe/Initialize.c | 30 +++++++++-----------
>  OvmfPkg/QemuVideoDxe/VbeShim.c    |  4 +--
>  4 files changed, 17 insertions(+), 42 deletions(-)
> 
> -- 
> 2.9.2
> 

Re: [PATCH 0/5] OvmfPkg/QemuVideoDxe: cleanups and tweaks

[Laszlo Ersek]

On 10/15/16 22:25, Jordan Justen wrote:
> Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

Commit range 5b54c92a6537..4dd8787a20e2.

Thank you!
Laszlo

> On 2016-10-12 03:09:00, Laszlo Ersek wrote:
>> Reviewing (part of) Ray's recent FrameBufferBltLib changes gave me the
>> idea for this. The patches are really small.
>>
>> Public branch: <https://github.com/lersek/edk2/commits/qemu_video_2>.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (5):
>>   OvmfPkg/QemuVideoDxe: drop useless QEMU_VIDEO_MODE_DATA.RefreshRate
>>   OvmfPkg/QemuVideoDxe: drop QEMU_VIDEO_CIRRUS_MODES.RefreshRate
>>   OvmfPkg/QemuVideoDxe: remove useless
>>     QEMU_VIDEO_PRIVATE_DATA.CurrentMode
>>   OvmfPkg/QemuVideoDxe: remove useless
>>     QEMU_VIDEO_PRIVATE_DATA.LineBuffer
>>   OvmfPkg/QemuVideoDxe: upgrade VERBOSE debug messages to INFO
>>
>>  OvmfPkg/QemuVideoDxe/Qemu.h       |  9 ++----
>>  OvmfPkg/QemuVideoDxe/Gop.c        | 16 -----------
>>  OvmfPkg/QemuVideoDxe/Initialize.c | 30 +++++++++-----------
>>  OvmfPkg/QemuVideoDxe/VbeShim.c    |  4 +--
>>  4 files changed, 17 insertions(+), 42 deletions(-)
>>
>> -- 
>> 2.9.2
>>