On 2018/8/23 10:53, Ruiyu Ni wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1109

Today's restriction of VGA device is to have only one VGA device
enabled per PCI segment. It's not correct because several segments
may share one IO / MMIO address space.
We should restrict to have one VGA per Host Bridge because each
Host Bridge has its only IO / MMIO address space.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <[email protected]>
Cc: Star Zeng <[email protected]>
---
  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 22 +++++++++++-----------
  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h | 10 +++++-----
  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c            |  4 ++--
  3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
index fdec0bcd53..fbcc53e41a 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
@@ -979,33 +979,33 @@ PciDeviceExisted (
  }
/**
-  Get the active VGA device on the same segment.
+  Get the active VGA device on the specified Host Bridge.
- @param VgaDevice PCI IO instance for the VGA device.
+  @param HostBridgeHandle    Host Bridge handle.
- @return The active VGA device on the same segment.
+  @return The active VGA device on the specified Host Bridge.
**/
  PCI_IO_DEVICE *
-ActiveVGADeviceOnTheSameSegment (
-  IN PCI_IO_DEVICE        *VgaDevice
+LocateVgaDeviceOnHostBridge (
+  IN EFI_HANDLE           HostBridgeHandle
    )
  {
    LIST_ENTRY      *CurrentLink;
-  PCI_IO_DEVICE   *Temp;
+  PCI_IO_DEVICE   *PciIo;

How about using PciIoDev as the local variable name? PciIo makes me regard it for PciIo protocol at the first glance. :)

There are still two minor comments for the code pieces below.

CurrentLink = mPciDevicePool.ForwardLink; while (CurrentLink != NULL && CurrentLink != &mPciDevicePool) { - Temp = PCI_IO_DEVICE_FROM_LINK (CurrentLink);
+    PciIo = PCI_IO_DEVICE_FROM_LINK (CurrentLink);
- if (Temp->PciRootBridgeIo->SegmentNumber == VgaDevice->PciRootBridgeIo->SegmentNumber) {
+    if (PciIo->PciRootBridgeIo->ParentHandle== HostBridgeHandle) {
- Temp = LocateVgaDevice (Temp);
+      PciIo = LocateVgaDevice (PciIo);
- if (Temp != NULL) {
-        return Temp;
+      if (PciIo != NULL) {
+        return PciIo;
        }
      }
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h
index 1ec2178a21..b45d2a5d77 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h
@@ -230,16 +230,16 @@ PciDeviceExisted (
    );
/**
-  Get the active VGA device on the same segment.
+  Get the active VGA device on the specified Host Bridge.
- @param VgaDevice PCI IO instance for the VGA device.
+  @param HostBridgeHandle    Host Bridge handle.
- @return The active VGA device on the same segment.
+  @return The active VGA device on the specified Host Bridge.
**/
  PCI_IO_DEVICE *
-ActiveVGADeviceOnTheSameSegment (
-  IN PCI_IO_DEVICE        *VgaDevice
+LocateVgaDeviceOnHostBridge (
+  IN EFI_HANDLE           HostBridgeHandle
    );
/**
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
index 291578c63c..333b875ff0 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
@@ -1415,7 +1415,7 @@ SupportPaletteSnoopAttributes (
    //
    // Get the boot VGA on the same segement

Please also update this code comment.

    //
-  Temp = ActiveVGADeviceOnTheSameSegment (PciIoDevice);
+  Temp = LocateVgaDeviceOnHostBridge 
(PciIoDevice->PciRootBridgeIo->ParentHandle);
if (Temp == NULL) {
      //
@@ -1670,7 +1670,7 @@ PciIoAttributes (
          //
          // Check if there have been an active VGA device on the same segment

Please also update this code comment.

With the new name and code comments updated,
Reviewed-by: Star Zeng <[email protected]>

Thanks,
Star

          //
-        Temp = ActiveVGADeviceOnTheSameSegment (PciIoDevice);
+        Temp = LocateVgaDeviceOnHostBridge 
(PciIoDevice->PciRootBridgeIo->ParentHandle);
          if (Temp != NULL && Temp != PciIoDevice) {
            //
            // An active VGA has been detected, so can not enable another


_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to