]> git.proxmox.com Git - mirror_ubuntu-bionic-kernel.git/commitdiff
Revert "apple-gmux: lock iGP IO to protect from vgaarb changes"
authorLukas Wunner <lukas@wunner.de>
Wed, 24 Jan 2018 18:35:45 +0000 (19:35 +0100)
committerSeth Forshee <seth.forshee@canonical.com>
Fri, 23 Feb 2018 14:27:18 +0000 (08:27 -0600)
BugLink: http://bugs.launchpad.net/bugs/1751131
commit d6fa7588fd7a8def4c747c0c574ce85d453e3788 upstream.

Commit 4eebd5a4e726 ("apple-gmux: lock iGP IO to protect from vgaarb
changes") amended this driver's ->probe hook to lock decoding of normal
(non-legacy) I/O space accesses to the integrated GPU on dual-GPU
MacBook Pros.  The lock stays in place until the driver is unbound.

The change was made to work around an issue with the out-of-tree nvidia
graphics driver (available at http://www.nvidia.com/object/unix.html).
It contains the following sequence in nvidia/nv.c:

#if defined(CONFIG_VGA_ARB) && !defined(NVCPU_PPC64LE)
#if defined(VGA_DEFAULT_DEVICE)
    vga_tryget(VGA_DEFAULT_DEVICE, VGA_RSRC_LEGACY_MASK);
#endif
    vga_set_legacy_decoding(dev, VGA_RSRC_NONE);
#endif

This code was reported to cause deadlocks with VFIO already in 2013:
https://devtalk.nvidia.com/default/topic/545560

I've reported the issue to Nvidia developers once more in 2017:
https://www.spinics.net/lists/dri-devel/msg138754.html

On the MacBookPro10,1, this code apparently breaks backlight control
(which is handled by apple-gmux via an I/O region starting at 0x700),
as reported by Petri Hodju:
https://bugzilla.kernel.org/show_bug.cgi?id=86121

I tried to replicate Petri's observations on my MacBook9,1, which uses
the same Intel Ivy Bridge + Nvidia GeForce GT 650M architecture, to no
avail.  On my machine apple-gmux' I/O region remains accessible even
with the nvidia driver loaded and commit 4eebd5a4e726 reverted.
Petri reported that apple-gmux becomes accessible again after a
suspend/resume cycle because the BIOS changed the VGA routing on the
root port to the Nvidia GPU.  Perhaps this is a BIOS issue after all
that can be fixed with an update?

In any case, the change made by commit 4eebd5a4e726 has turned out to
cause two new issues:

* Wilfried Klaebe reports a deadlock when launching Xorg because it
  opens /dev/vga_arbiter and calls vga_get(), but apple-gmux is holding
  a lock on I/O space indefinitely.  It looks like apple-gmux' current
  behavior is an abuse of the vgaarb API as locks are not meant to be
  held for longer periods:
  https://bugzilla.kernel.org/show_bug.cgi?id=88861#c11
  https://bugzilla.kernel.org/attachment.cgi?id=217541

* On dual GPU MacBook Pros introduced since 2013, the integrated GPU is
  powergated on boot und thus becomes invisible to Linux unless a custom
  EFI protocol is used to leave it powered on.  (A patch exists but is
  not in mainline yet due to several negative side effects.)  On these
  machines, locking I/O to the integrated GPU (as done by 4eebd5a4e726)
  fails and backlight control is therefore broken:
  https://bugzilla.kernel.org/show_bug.cgi?id=105051

So let's revert commit 4eebd5a4e726 please.  Users experiencing the
issue with the proprietary nvidia driver can comment out the above-
quoted problematic code as a workaround (or try updating the BIOS).

Cc: Petri Hodju <petrihodju@yahoo.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Bruno Prémont <bonbons@linux-vserver.org>
Cc: Andy Ritger <aritger@nvidia.com>
Cc: Ronald Tschalär <ronald@innovation.ch>
Tested-by: Wilfried Klaebe <linux-kernel@lebenslange-mailadresse.de>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
drivers/platform/x86/apple-gmux.c

index 623d322447a2192fcfba9e0041ddbe76080b30bb..7c4eb86c851ed9568cb882d32a9a5fdd3a0d13a0 100644 (file)
@@ -24,7 +24,6 @@
 #include <linux/delay.h>
 #include <linux/pci.h>
 #include <linux/vga_switcheroo.h>
-#include <linux/vgaarb.h>
 #include <acpi/video.h>
 #include <asm/io.h>
 
@@ -54,7 +53,6 @@ struct apple_gmux_data {
        bool indexed;
        struct mutex index_lock;
 
-       struct pci_dev *pdev;
        struct backlight_device *bdev;
 
        /* switcheroo data */
@@ -599,23 +597,6 @@ static int gmux_resume(struct device *dev)
        return 0;
 }
 
-static struct pci_dev *gmux_get_io_pdev(void)
-{
-       struct pci_dev *pdev = NULL;
-
-       while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev))) {
-               u16 cmd;
-
-               pci_read_config_word(pdev, PCI_COMMAND, &cmd);
-               if (!(cmd & PCI_COMMAND_IO))
-                       continue;
-
-               return pdev;
-       }
-
-       return NULL;
-}
-
 static int is_thunderbolt(struct device *dev, void *data)
 {
        return to_pci_dev(dev)->is_thunderbolt;
@@ -631,7 +612,6 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
        int ret = -ENXIO;
        acpi_status status;
        unsigned long long gpe;
-       struct pci_dev *pdev = NULL;
 
        if (apple_gmux_data)
                return -EBUSY;
@@ -682,7 +662,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
                        ver_minor = (version >> 16) & 0xff;
                        ver_release = (version >> 8) & 0xff;
                } else {
-                       pr_info("gmux device not present or IO disabled\n");
+                       pr_info("gmux device not present\n");
                        ret = -ENODEV;
                        goto err_release;
                }
@@ -690,23 +670,6 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
        pr_info("Found gmux version %d.%d.%d [%s]\n", ver_major, ver_minor,
                ver_release, (gmux_data->indexed ? "indexed" : "classic"));
 
-       /*
-        * Apple systems with gmux are EFI based and normally don't use
-        * VGA. In addition changing IO+MEM ownership between IGP and dGPU
-        * disables IO/MEM used for backlight control on some systems.
-        * Lock IO+MEM to GPU with active IO to prevent switch.
-        */
-       pdev = gmux_get_io_pdev();
-       if (pdev && vga_tryget(pdev,
-                              VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM)) {
-               pr_err("IO+MEM vgaarb-locking for PCI:%s failed\n",
-                       pci_name(pdev));
-               ret = -EBUSY;
-               goto err_release;
-       } else if (pdev)
-               pr_info("locked IO for PCI:%s\n", pci_name(pdev));
-       gmux_data->pdev = pdev;
-
        memset(&props, 0, sizeof(props));
        props.type = BACKLIGHT_PLATFORM;
        props.max_brightness = gmux_read32(gmux_data, GMUX_PORT_MAX_BRIGHTNESS);
@@ -822,10 +785,6 @@ err_enable_gpe:
 err_notify:
        backlight_device_unregister(bdev);
 err_release:
-       if (gmux_data->pdev)
-               vga_put(gmux_data->pdev,
-                       VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM);
-       pci_dev_put(pdev);
        release_region(gmux_data->iostart, gmux_data->iolen);
 err_free:
        kfree(gmux_data);
@@ -845,11 +804,6 @@ static void gmux_remove(struct pnp_dev *pnp)
                                           &gmux_notify_handler);
        }
 
-       if (gmux_data->pdev) {
-               vga_put(gmux_data->pdev,
-                       VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM);
-               pci_dev_put(gmux_data->pdev);
-       }
        backlight_device_unregister(gmux_data->bdev);
 
        release_region(gmux_data->iostart, gmux_data->iolen);