Re: [PATCH] PCI/VGA: Look at all PCI display devices in VGA arbiter

[Mario Limonciello <superm1@kernel.org>]

On 6/17/25 11:36 AM, Daniel Dadap wrote:
> On Tue, Jun 17, 2025 at 11:06:00AM -0500, Mario Limonciello wrote:
>> On 6/17/25 10:53 AM, Daniel Dadap wrote:
>>> On Mon, Jun 16, 2025 at 09:14:04PM +0200, Lukas Wunner wrote:
>>>> On Mon, Jun 16, 2025 at 10:05:48AM -0500, Daniel Dadap wrote:
>>>>> On Sat, Jun 14, 2025 at 09:04:52AM +0200, Lukas Wunner wrote:
>>>>>> On Fri, Jun 13, 2025 at 04:47:20PM -0500, Daniel Dadap wrote:
>>>>>>> Ideally we'd be able to actually query which GPU is connected to
>>>>>>> the panel at the time we're making this determination, but I don't
>>>>>>> think there's a uniform way to do this at the moment. Selecting the
>>>>>>> integrated GPU seems like a reasonable heuristic, since I'm not
>>>>>>> aware of any systems where the internal panel defaults to dGPU
>>>>>>> connection, since that would defeat the purpose of having a hybrid
>>>>>>> GPU system in the first place
>>>>>>
>>>>>> Intel-based dual-GPU MacBook Pros boot with the panel switched to the
>>>>>> dGPU by default.  This is done for Windows compatibility because Apple
>>>>>> never bothered to implement dynamic GPU switching on Windows.
>>>>>>
>>>>>> The boot firmware can be told to switch the panel to the iGPU via an
>>>>>> EFI variable, but that's not something the kernel can or should depend on.
>>>>>>
>>>>>> MacBook Pros introduced since 2013/2014 hide the iGPU if the panel is
>>>>>> switched to the dGPU on boot, but the kernel is now unhiding it since
>>>>>> 71e49eccdca6.
>>>>>
>>>>> This is good to know. Is vga_switcheroo initialized by the time the code
>>>>> in this patch runs? If so, maybe we should query switcheroo to determine
>>>>> the GPU which is connected to the panel and favor that one, then fall
>>>>> back to the "iGPU is probably right" heuristic otherwise.
>>>>
>>>> Right now vga_switcheroo doesn't seem to provide a function to query the
>>>> current mux state.
>>>>
>>>> The driver for the mux on MacBook Pros, apple_gmux.c, can be modular,
>>>> so may be loaded fairly late.
>>>
>>> Yeah, that's what I was afraid of.
>>>
>>>>
>>>> Personally I'm booting my MacBook Pro via EFI, so the GPU in use is
>>>> whatever efifb is talking to.  However it is possible to boot these
>>>> machines in a legacy CSM mode and I don't know what the situation
>>>> looks like in that case.
>>>>
>>>
>>> Skimming through the code, this seems like the sort of thing that the
>>> existing check in vga_is_firmware_default() is testing. I'm not familiar
>>> enough with the relevant code to intuitively know whether it is supposed
>>> to work for UEFI or legacy VGA or both (I think both?).
>>>
>>> Mario, on the affected system, what does vga_is_firmware_default() return
>>> on each of the GPUs? (I'd expect it to return true for the iGPU and false
>>> for the dGPU, but I think the (boot_vga && boot_vga->is_firmware_default)
>>> test would cause vga_is_boot_device() to return false if the vga_default
>>> is passed into it a second time. That made sense for the way that the
>>> function was originally called, to test if the passed-in vgadev is any
>>> "better" than vga_default, and from a quick skim it doesn't seem like it
>>> would get called multiple times in the new code either, but I worry that
>>> if someone else decides they need to call vga_is_boot_device() a second
>>> time in the future it might return a false result for vga_default.)
>>>
>>
>> Right "now" on an unpatched kernel it won't run 'at all' because
>> vga_arb_device_init() only matches VGA class devices.
>>
>> Both GPUs are not VGA compatible, which is what lead to the patch in this
>> thread starting to match display class devices too.
>>
>>
> 
> Sure, I was curious what vga_is_firmware_default() returns for each of
> the GPUs when run, regardless of whether or not it's currently being run
> in the existing code - I'm wondering if vga_is_firmware_default() can be
> a better tie breaker (or at least a first line tie breaker) than the one
> you have now which tests for iGPU. I kind of went off on a tangent about
> vga_is_boot_device() being possibly unreliable for a second time caller
> when I was checking the callers of vga_is_firmware_default().
> 

That's actually a really good point.  Here's the diff I tried.

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 78748e8d2dba..b4e085806544 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -816,6 +816,8 @@ static bool vga_arbiter_add_pci_device(struct 
pci_dev *pdev)
                 bus = bus->parent;
         }

+       pci_info(pdev, "is firmware default: %d\n", 
vga_is_firmware_default(pdev));
+
         if (vga_is_boot_device(vgadev)) {
                 vgaarb_info(&pdev->dev, "setting as boot VGA device%s\n",
                             vga_default_device() ?
@@ -1500,7 +1502,7 @@ static int pci_notify(struct notifier_block *nb, 
unsigned long action,
         vgaarb_dbg(dev, "%s\n", __func__);

         /* Only deal with VGA class devices */
-       if (!pci_is_vga(pdev))
+       if (!pci_is_display(pdev))
                 return 0;

         /*
@@ -1551,7 +1553,7 @@ static int __init vga_arb_device_init(void)
         while ((pdev =
                 pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
                                PCI_ANY_ID, pdev)) != NULL) {
-               if (pci_is_vga(pdev))
+               if (pci_is_display(pdev))
                         vga_arbiter_add_pci_device(pdev);
         }

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 05e68f35f392..077e90a2af37 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -744,6 +744,22 @@ static inline bool pci_is_vga(struct pci_dev *pdev)
         return false;
  }

+
+/**
+ * pci_is_display - Check if a PCI device is a display controller
+ * @pdev: Pointer to the PCI device structure
+ *
+ * This function determines whether the given PCI device corresponds
+ * to a display controller. Display controllers are typically used
+ * for graphical output and are identified based on their class code.
+ *
+ * Return: true if the PCI device is a display controller, false otherwise.
+ */
+static inline bool pci_is_display(struct pci_dev *pdev)
+{
+       return (pdev->class >> 16) == PCI_BASE_CLASS_DISPLAY;
+}
+
  #define for_each_pci_bridge(dev, bus)                          \
         list_for_each_entry(dev, &bus->devices, bus_list)       \
                 if (!pci_is_bridge(dev)) {} else

On 6.16-rc2 with that applied:

$ lspci | grep "\[03"
c6:00.0 3D controller [0302]: NVIDIA Corporation Device (rev a1)
c7:00.0 Display controller [0380]: Advanced Micro Devices, Inc. 
[AMD/ATI] Device (rev d1)

$ sudo dmesg | grep "firmware default"
[    0.379664] pci 0000:c6:00.0: is firmware default: 0
[    0.379664] pci 0000:c7:00.0: is firmware default: 1

Which actually means that the existing code when making a second pass 
the correct GPU *is* getting set using that as a tie breaker.

$ sudo dmesg | grep arb
[    0.379664] pci 0000:c6:00.0: vgaarb: setting as boot VGA device
[    0.379664] pci 0000:c6:00.0: vgaarb: bridge control possible
[    0.379664] pci 0000:c6:00.0: vgaarb: VGA device added: 
decodes=io+mem,owns=none,locks=none
[    0.379664] pci 0000:c7:00.0: vgaarb: setting as boot VGA device 
(overriding previous)
[    0.379664] pci 0000:c7:00.0: vgaarb: bridge control possible
[    0.379664] pci 0000:c7:00.0: vgaarb: VGA device added: 
decodes=io+mem,owns=none,locks=none
[    0.379664] vgaarb: loaded
[    4.108302] amdgpu 0000:c7:00.0: vgaarb: deactivate vga console

IE if changing pci_dev_attrs_are_visible() to show for all display 
devices it /should/ work without the logic changes I had.