| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aFMpxWiADVXBwVgO@ddadap-lakeline.nvidia.com>
Date: Wed, 18 Jun 2025 16:04:05 -0500
From: Daniel Dadap <ddadap@...dia.com>
To: Mario Limonciello <superm1@...nel.org>
Cc: Thomas Zimmermann <tzimmermann@...e.de>,
Alex Williamson <alex.williamson@...hat.com>,
David Airlie <airlied@...il.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Alex Deucher <alexander.deucher@....com>,
Christian König <christian.koenig@....com>,
Simona Vetter <simona@...ll.ch>, Lukas Wunner <lukas@...ner.de>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
David Woodhouse <dwmw2@...radead.org>,
Lu Baolu <baolu.lu@...ux.intel.com>, Joerg Roedel <joro@...tes.org>,
Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
"open list:DRM DRIVERS" <dri-devel@...ts.freedesktop.org>,
open list <linux-kernel@...r.kernel.org>,
"open list:INTEL IOMMU (VT-d)" <iommu@...ts.linux.dev>,
"open list:PCI SUBSYSTEM" <linux-pci@...r.kernel.org>,
"open list:VFIO DRIVER" <kvm@...r.kernel.org>,
"open list:SOUND" <linux-sound@...r.kernel.org>,
Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH v2 6/6] vgaarb: Look at all PCI display devices in VGA
arbiter
On Wed, Jun 18, 2025 at 01:45:40PM -0500, Mario Limonciello wrote:
> On 6/18/2025 9:12 AM, Simona Vetter wrote:
> > On Wed, Jun 18, 2025 at 11:11:26AM +0200, Thomas Zimmermann wrote:
> > > Hi
> > >
> > > Am 17.06.25 um 22:22 schrieb Mario Limonciello:
> > > >
> > > >
> > > > On 6/17/25 2:22 PM, Alex Williamson wrote:
> > > > > On Tue, 17 Jun 2025 12:59:10 -0500
> > > > > Mario Limonciello <superm1@...nel.org> wrote:
> > > > >
> > > > > > From: Mario Limonciello <mario.limonciello@....com>
> > > > > >
> > > > > > On a mobile system with an AMD integrated GPU + NVIDIA discrete GPU the
> > > > > > AMD GPU is not being selected by some desktop environments for any
> > > > > > rendering tasks. This is because neither GPU is being treated as
> > > > > > "boot_vga" but that is what some environments use to select a GPU [1].
> > > > > >
> > > > > > The VGA arbiter driver only looks at devices that report as PCI display
> > > > > > VGA class devices. Neither GPU on the system is a PCI display VGA class
> > > > > > device:
> > > > > >
> > > > > > c5:00.0 3D controller: NVIDIA Corporation Device 2db9 (rev a1)
> > > > > > c6:00.0 Display controller: Advanced Micro Devices, Inc.
> > > > > > [AMD/ATI] Device 150e (rev d1)
> > > > > >
> > > > > > If the GPUs were looked at the vga_is_firmware_default()
> > > > > > function actually
> > > > > > does do a good job at recognizing the case from the device used for the
> > > > > > firmware framebuffer.
> > > > > >
> > > > > > Modify the VGA arbiter code and matching sysfs file entries to
> > > > > > examine all
> > > > > > PCI display class devices. The existing logic stays the same.
> > > > > >
> > > > > > This will cause all GPUs to gain a `boot_vga` file, but the
> > > > > > correct device
> > > > > > (AMD GPU in this case) will now show `1` and the incorrect
> > > > > > device shows `0`.
> > > > > > Userspace then picks the right device as well.
> > > > > >
> > > > > > Link: https://github.com/robherring/libpciaccess/commit/b2838fb61c3542f107014b285cbda097acae1e12
> > > > > > [1]
> > > > > > Suggested-by: Daniel Dadap <ddadap@...dia.com>
> > > > > > Acked-by: Thomas Zimmermann <tzimmermann@...e.de>
> > > > > > Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> > > > > > ---
> > > > > > drivers/pci/pci-sysfs.c | 2 +-
> > > > > > drivers/pci/vgaarb.c | 8 ++++----
> > > > > > 2 files changed, 5 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > > > > index 268c69daa4d57..c314ee1b3f9ac 100644
> > > > > > --- a/drivers/pci/pci-sysfs.c
> > > > > > +++ b/drivers/pci/pci-sysfs.c
> > > > > > @@ -1707,7 +1707,7 @@ static umode_t
> > > > > > pci_dev_attrs_are_visible(struct kobject *kobj,
> > > > > > struct device *dev = kobj_to_dev(kobj);
> > > > > > struct pci_dev *pdev = to_pci_dev(dev);
> > > > > > - if (a == &dev_attr_boot_vga.attr && pci_is_vga(pdev))
> > > > > > + if (a == &dev_attr_boot_vga.attr && pci_is_display(pdev))
> > > > > > return a->mode;
> > > > > > return 0;
> > > > > > diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> > > > > > index 78748e8d2dbae..63216e5787d73 100644
> > > > > > --- a/drivers/pci/vgaarb.c
> > > > > > +++ b/drivers/pci/vgaarb.c
> > > > > > @@ -1499,8 +1499,8 @@ 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))
> > > > > > + /* Only deal with PCI display class devices */
> > > > > > + if (!pci_is_display(pdev))
> > > > > > return 0;
> > > > > > /*
> > > > > > @@ -1546,12 +1546,12 @@ static int __init vga_arb_device_init(void)
> > > > > > bus_register_notifier(&pci_bus_type, &pci_notifier);
> > > > > > - /* Add all VGA class PCI devices by default */
> > > > > > + /* Add all PCI display class devices by default */
> > > > > > pdev = NULL;
> > > > > > 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);
> > > > > > }
> > > > >
> > > > > At the very least a non-VGA device should not mark that it decodes
> > > > > legacy resources, marking the boot VGA device is only a part of what
> > > > > the VGA arbiter does. It seems none of the actual VGA arbitration
> > > > > interfaces have been considered here though.
> > > > >
> > > > > I still think this is a bad idea and I'm not sure Thomas didn't
> > > > > withdraw his ack in the previous round[1]. Thanks,
> > > >
> > > > Ah; I didn't realize that was intended to be a withdrawl.
> > > > If there's another version of this I'll remove it.
> > >
> > > Then let me formally withdraw the A-b.
> > >
> > > I think this updated patch doesn't address the concerns raised in the
> > > previous reviews. AFAIU vgaarb is really only about VGA devices.
> >
> > I missed the earlier version, but wanted to chime in that I concur. vgaarb
> > is about vga decoding, and modern gpu drivers are trying pretty hard to
> > disable that since it can cause pain. If we mix in the meaning of "default
> > display device" into this, we have a mess.
> >
> > I guess what does make sense is if the kernel exposes its notion of
> > "default display device", since we do have that in some sense with
> > simpledrm. At least on systems where simpledrm is a thing, but I think you
> > need some really old machines for that to not be the case.
> >
> > Cheers, Sima
>
> Thanks guys. Let's discard patch 6. Here's a spin of an approach for
> userspace that does something similar to what the compositors are doing.
> We can iterate on that.
>
> https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/merge_requests/38
>
> I think patches 1-5 still are valuable though. So please add reviews to
> those and we can take those without patch 6 if there is agreement.
Sima already gave her Reviewed-by: for patches 1-5 in the thread for 5. I
don't expect my Reviewed-by: is worth much for any of those files, but if
you want it, you have mine as well. (In patch 4, the iommu/vt-d one, it is
a little weird that it's no longer symmetrical with the other class test
macros right next to it, but I still think I prefer using the helper.)
Powered by blists - more mailing lists