| Message ID | 20150311223445.6af76f7c@neptune.home |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, Mar 11, 2015 at 10:34:45PM +0100, Bruno Prémont wrote: > As GMUX depends on IO for iGP to be enabled and active, lock the IO at > vgaarb level. This should prevent GPU driver for dGPU to disable IO for > iGP while it tries to own legacy VGA IO. > > This fixes usage of backlight control combined with closed nvidia > driver on some Apple dual-GPU (intel/nvidia) systems. > > On those systems loading nvidia driver disables intel IO decoding, > disabling the gmux backlight controls as a side effect. > Prior to commits moving boot_vga from (optional) efifb to less optional > vgaarb this mis-behavior could be avoided by using right kernel config > (efifb enabled but vgaarb disabled). > > This patch explicitly does not try to trigger vgaarb changes in order > to avoid confusing already running graphics drivers. If IO has been > mis-configured by vgaarb gmux will thus fail to probe. > It is expected to load/probe gmux prior to graphics drivers. > > Fixes: ce027dac592c0ada241ce0f95ae65856828ac450 # nvidia interaction > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=86121 > Reported-by: Petri Hodju <petrihodju@yahoo.com> > Tested-by: Petri Hodju <petrihodju@yahoo.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Matthew Garrett <matthew.garrett@nebula.com> > Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org> > --- > Resending v2 in the hope Darren won't hit quoted-printable. > Also adding linux-pci on CC by Bjorn's request in bugzilla. Much better, thanks. Queued.
FYI, this actually broke backlight controls on my MBP11,3 because the assumption the patch makes that gmux is always loaded before graphics drivers didn't hold true. At least for me dracut included the nouveau module in the initrd but not gmux, ensuring the ordering was wrong. No errors were reporting, and gmux still offered the backlight device, it just became inoperable. I worked around this for my kernel by building gmux into vmlinuz instead of as a module but that isn't going to in more general configs because there is an apple backlight driver which cannot be built at all in that configuration. Is there a way to make the ordering between nouveau and gmux more explicit/reliable? Can gmux complain loudly if the ordering is ever wrong? On Wed, Mar 11, 2015 at 2:34 PM, Bruno Prémont <bonbons@linux-vserver.org> wrote: > As GMUX depends on IO for iGP to be enabled and active, lock the IO at > vgaarb level. This should prevent GPU driver for dGPU to disable IO for > iGP while it tries to own legacy VGA IO. > > This fixes usage of backlight control combined with closed nvidia > driver on some Apple dual-GPU (intel/nvidia) systems. > > On those systems loading nvidia driver disables intel IO decoding, > disabling the gmux backlight controls as a side effect. > Prior to commits moving boot_vga from (optional) efifb to less optional > vgaarb this mis-behavior could be avoided by using right kernel config > (efifb enabled but vgaarb disabled). > > This patch explicitly does not try to trigger vgaarb changes in order > to avoid confusing already running graphics drivers. If IO has been > mis-configured by vgaarb gmux will thus fail to probe. > It is expected to load/probe gmux prior to graphics drivers. > > Fixes: ce027dac592c0ada241ce0f95ae65856828ac450 # nvidia interaction > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=86121 > Reported-by: Petri Hodju <petrihodju@yahoo.com> > Tested-by: Petri Hodju <petrihodju@yahoo.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Matthew Garrett <matthew.garrett@nebula.com> > Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org> > --- > Resending v2 in the hope Darren won't hit quoted-printable. > Also adding linux-pci on CC by Bjorn's request in bugzilla. > > Changes since v2: > - Renamed gmux_find_pdev to gmux_get_io_pdev > - Whitespace fix > - vga_put() on error path > > Changes since v1: > - Dropped repeat of gmux in pr_info/pr_err calls > - Mention PCI device we tried to lock IO for in case of error > > > drivers/platform/x86/apple-gmux.c | 48 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 47 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c > index b9429fb..e743b03 100644 > --- a/drivers/platform/x86/apple-gmux.c > +++ b/drivers/platform/x86/apple-gmux.c > @@ -22,6 +22,7 @@ > #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> > > @@ -31,6 +32,7 @@ struct apple_gmux_data { > bool indexed; > struct mutex index_lock; > > + struct pci_dev *pdev; > struct backlight_device *bdev; > > /* switcheroo data */ > @@ -415,6 +417,23 @@ 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 gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id) > { > struct apple_gmux_data *gmux_data; > @@ -425,6 +444,7 @@ 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; > @@ -475,7 +495,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\n"); > + pr_info("gmux device not present or IO disabled\n"); > ret = -ENODEV; > goto err_release; > } > @@ -483,6 +503,23 @@ 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); > @@ -574,6 +611,10 @@ 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); > @@ -593,6 +634,11 @@ 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); > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 26, 2015 at 12:10:48PM -0700, Michael Marineau wrote: > FYI, this actually broke backlight controls on my MBP11,3 because the > assumption the patch makes that gmux is always loaded before graphics > drivers didn't hold true. At least for me dracut included the nouveau > module in the initrd but not gmux, ensuring the ordering was wrong. No > errors were reporting, and gmux still offered the backlight device, it > just became inoperable. I worked around this for my kernel by building > gmux into vmlinuz instead of as a module but that isn't going to in > more general configs because there is an apple backlight driver which > cannot be built at all in that configuration. > Thank you for reporting this Michael, That is tough as nouveau doesn't have an explicit dependency on gmux, so we could do something like a passive request_module(), but if it isn't in the initrd image, it would still fail as you describe. > Is there a way to make the ordering between nouveau and gmux more > explicit/reliable? Can gmux complain loudly if the ordering is ever > wrong? It should print an error if the probe fails due to the IO already being in use or if it can't be allocated. The disabled IO case is only info level though, perhaps that should be higher priority. Printing something when failing to probe seems like a reasonable thing to do. Michael, which message do you get if you boot with "debug" or "loglevel=6" when apple-gmux is not built-in?
On Tue, May 26, 2015 at 9:47 PM, Darren Hart <dvhart@infradead.org> wrote: > On Tue, May 26, 2015 at 12:10:48PM -0700, Michael Marineau wrote: >> FYI, this actually broke backlight controls on my MBP11,3 because the >> assumption the patch makes that gmux is always loaded before graphics >> drivers didn't hold true. At least for me dracut included the nouveau >> module in the initrd but not gmux, ensuring the ordering was wrong. No >> errors were reporting, and gmux still offered the backlight device, it >> just became inoperable. I worked around this for my kernel by building >> gmux into vmlinuz instead of as a module but that isn't going to in >> more general configs because there is an apple backlight driver which >> cannot be built at all in that configuration. >> > > Thank you for reporting this Michael, > > That is tough as nouveau doesn't have an explicit dependency on gmux, so we > could do something like a passive request_module(), but if it isn't in the > initrd image, it would still fail as you describe. > >> Is there a way to make the ordering between nouveau and gmux more >> explicit/reliable? Can gmux complain loudly if the ordering is ever >> wrong? > > It should print an error if the probe fails due to the IO already being in use > or if it can't be allocated. The disabled IO case is only info level though, > perhaps that should be higher priority. Printing something when failing to probe > seems like a reasonable thing to do. > > Michael, which message do you get if you boot with "debug" or "loglevel=6" when > apple-gmux is not built-in? No error, gmux claims it worked: [ 13.693379] apple_gmux: Found gmux version 4.0.8 [indexed] [ 13.693400] vgaarb: device changed decodes: PCI:0000:01:00.0,olddecodes=io+mem,decodes=io+mem:owns=none [ 13.693404] apple_gmux: locked IO for PCI:0000:01:00.0 Full dmesg: https://gist.github.com/marineam/0e5a23548e8b3b2e1d50 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Michael, On Tue, 26 May 2015 21:47:49 -0700 Darren Hart wrote: > On Tue, May 26, 2015 at 12:10:48PM -0700, Michael Marineau wrote: > > FYI, this actually broke backlight controls on my MBP11,3 because the > > assumption the patch makes that gmux is always loaded before graphics > > drivers didn't hold true. At least for me dracut included the nouveau > > module in the initrd but not gmux, ensuring the ordering was wrong. No > > errors were reporting, and gmux still offered the backlight device, it > > just became inoperable. I worked around this for my kernel by building > > gmux into vmlinuz instead of as a module but that isn't going to in > > more general configs because there is an apple backlight driver which > > cannot be built at all in that configuration. > > Thank you for reporting this Michael, > > That is tough as nouveau doesn't have an explicit dependency on gmux, so we > could do something like a passive request_module(), but if it isn't in the > initrd image, it would still fail as you describe. > > > Is there a way to make the ordering between nouveau and gmux more > > explicit/reliable? Can gmux complain loudly if the ordering is ever > > wrong? > > It should print an error if the probe fails due to the IO already being in use > or if it can't be allocated. The disabled IO case is only info level though, > perhaps that should be higher priority. Printing something when failing to probe > seems like a reasonable thing to do. > > Michael, which message do you get if you boot with "debug" or "loglevel=6" when > apple-gmux is not built-in? A full kernel log up to including post-initrd loading of gmux would be useful. As far as I have seen nouveau should not be doing unneeded vgaarb operations by itself (though userspace might be) as opposed to closed nvidia driver. If your systems allows, try booting to init=/bin/bash, then check for backlight, load nouveau, check for backlight and finally load gmux and check backlight (putting i915 in the mix where initrd/userspace puts it would be nice). Thanks, Bruno -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 26 May 2015 22:35:46 -0700 Michael Marineau wrote: > On Tue, May 26, 2015 at 9:47 PM, Darren Hart <dvhart@infradead.org> wrote: > > On Tue, May 26, 2015 at 12:10:48PM -0700, Michael Marineau wrote: > >> FYI, this actually broke backlight controls on my MBP11,3 because the > >> assumption the patch makes that gmux is always loaded before graphics > >> drivers didn't hold true. At least for me dracut included the nouveau > >> module in the initrd but not gmux, ensuring the ordering was wrong. No > >> errors were reporting, and gmux still offered the backlight device, it > >> just became inoperable. I worked around this for my kernel by building > >> gmux into vmlinuz instead of as a module but that isn't going to in > >> more general configs because there is an apple backlight driver which > >> cannot be built at all in that configuration. > >> > > > > Thank you for reporting this Michael, > > > > That is tough as nouveau doesn't have an explicit dependency on gmux, so we > > could do something like a passive request_module(), but if it isn't in the > > initrd image, it would still fail as you describe. > > > >> Is there a way to make the ordering between nouveau and gmux more > >> explicit/reliable? Can gmux complain loudly if the ordering is ever > >> wrong? > > > > It should print an error if the probe fails due to the IO already being in use > > or if it can't be allocated. The disabled IO case is only info level though, > > perhaps that should be higher priority. Printing something when failing to probe > > seems like a reasonable thing to do. > > > > Michael, which message do you get if you boot with "debug" or "loglevel=6" when > > apple-gmux is not built-in? > > No error, gmux claims it worked: > [ 13.693379] apple_gmux: Found gmux version 4.0.8 [indexed] > [ 13.693400] vgaarb: device changed decodes: > PCI:0000:01:00.0,olddecodes=io+mem,decodes=io+mem:owns=none > [ 13.693404] apple_gmux: locked IO for PCI:0000:01:00.0 > > Full dmesg: https://gist.github.com/marineam/0e5a23548e8b3b2e1d50 What GPUs are there in your MBP11,3? From your dmesg it looks like all you have is NVIDIA GPU 0000:01:00.0 (lspci?). Is there a somehow hidden intel GPU around doing the backlight? If so my locking does the wrong thing for your system as: [ 0.393298] vgaarb: device added: PCI:0000:01:00.0,decodes=io+mem,owns=none,locks=none [ 0.393299] vgaarb: loaded [ 0.393300] vgaarb: setting as boot device: PCI:0000:01:00.0 [ 0.393301] vgaarb: bridge control possible 0000:01:00.0 ... [ 13.693379] apple_gmux: Found gmux version 4.0.8 [indexed] [ 13.693400] vgaarb: device changed decodes: PCI:0000:01:00.0,olddecodes=io+mem,decodes=io+mem:owns=none [ 13.693404] apple_gmux: locked IO for PCI:0000:01:00.0 Here it triggers "olddecodes=none -> io+mem". Making sure to lock only the intel GPU when present and especially protecting against nvidia driver will be hard if legacy-IO is being processed by a hidden device! Bruno -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 26, 2015 at 10:53 PM, Bruno Prémont <bonbons@linux-vserver.org> wrote: > Hi Michael, > > On Tue, 26 May 2015 21:47:49 -0700 Darren Hart wrote: >> On Tue, May 26, 2015 at 12:10:48PM -0700, Michael Marineau wrote: >> > FYI, this actually broke backlight controls on my MBP11,3 because the >> > assumption the patch makes that gmux is always loaded before graphics >> > drivers didn't hold true. At least for me dracut included the nouveau >> > module in the initrd but not gmux, ensuring the ordering was wrong. No >> > errors were reporting, and gmux still offered the backlight device, it >> > just became inoperable. I worked around this for my kernel by building >> > gmux into vmlinuz instead of as a module but that isn't going to in >> > more general configs because there is an apple backlight driver which >> > cannot be built at all in that configuration. >> >> Thank you for reporting this Michael, >> >> That is tough as nouveau doesn't have an explicit dependency on gmux, so we >> could do something like a passive request_module(), but if it isn't in the >> initrd image, it would still fail as you describe. >> >> > Is there a way to make the ordering between nouveau and gmux more >> > explicit/reliable? Can gmux complain loudly if the ordering is ever >> > wrong? >> >> It should print an error if the probe fails due to the IO already being in use >> or if it can't be allocated. The disabled IO case is only info level though, >> perhaps that should be higher priority. Printing something when failing to probe >> seems like a reasonable thing to do. >> >> Michael, which message do you get if you boot with "debug" or "loglevel=6" when >> apple-gmux is not built-in? > > A full kernel log up to including post-initrd loading of gmux would be > useful. > > As far as I have seen nouveau should not be doing unneeded vgaarb > operations by itself (though userspace might be) as opposed to closed > nvidia driver. > > If your systems allows, try booting to init=/bin/bash, then check for > backlight, load nouveau, check for backlight and finally load gmux and > check backlight (putting i915 in the mix where initrd/userspace puts > it would be nice). Both before and after loading nouveau there is a working acpi_video backlight control. Then after loading gmux it is the only backlight control and it doesn't do anything. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 26, 2015 at 11:13 PM, Bruno Prémont <bonbons@linux-vserver.org> wrote: > On Tue, 26 May 2015 22:35:46 -0700 Michael Marineau wrote: >> On Tue, May 26, 2015 at 9:47 PM, Darren Hart <dvhart@infradead.org> wrote: >> > On Tue, May 26, 2015 at 12:10:48PM -0700, Michael Marineau wrote: >> >> FYI, this actually broke backlight controls on my MBP11,3 because the >> >> assumption the patch makes that gmux is always loaded before graphics >> >> drivers didn't hold true. At least for me dracut included the nouveau >> >> module in the initrd but not gmux, ensuring the ordering was wrong. No >> >> errors were reporting, and gmux still offered the backlight device, it >> >> just became inoperable. I worked around this for my kernel by building >> >> gmux into vmlinuz instead of as a module but that isn't going to in >> >> more general configs because there is an apple backlight driver which >> >> cannot be built at all in that configuration. >> >> >> > >> > Thank you for reporting this Michael, >> > >> > That is tough as nouveau doesn't have an explicit dependency on gmux, so we >> > could do something like a passive request_module(), but if it isn't in the >> > initrd image, it would still fail as you describe. >> > >> >> Is there a way to make the ordering between nouveau and gmux more >> >> explicit/reliable? Can gmux complain loudly if the ordering is ever >> >> wrong? >> > >> > It should print an error if the probe fails due to the IO already being in use >> > or if it can't be allocated. The disabled IO case is only info level though, >> > perhaps that should be higher priority. Printing something when failing to probe >> > seems like a reasonable thing to do. >> > >> > Michael, which message do you get if you boot with "debug" or "loglevel=6" when >> > apple-gmux is not built-in? >> >> No error, gmux claims it worked: >> [ 13.693379] apple_gmux: Found gmux version 4.0.8 [indexed] >> [ 13.693400] vgaarb: device changed decodes: >> PCI:0000:01:00.0,olddecodes=io+mem,decodes=io+mem:owns=none >> [ 13.693404] apple_gmux: locked IO for PCI:0000:01:00.0 >> >> Full dmesg: https://gist.github.com/marineam/0e5a23548e8b3b2e1d50 > > What GPUs are there in your MBP11,3? From your dmesg it looks like all > you have is NVIDIA GPU 0000:01:00.0 (lspci?). > > Is there a somehow hidden intel GPU around doing the backlight? On this system if you don't futz with the firmware in just the right way it hides the Intel GPU: https://github.com/marineam/linux/commit/1f2fcbbca18176e2e6c862774428dad878bbac51 Exposing the Intel GPU isn't particularly great because power management gets wonky and I've never figured out how to switch between the GPUs, reconfiguring the eDP link never works right. Even when running with the Intel hidden the current nouveau driver cannot redo the eDP link so I have a hack to prevent modesetting from ever powering the link down: https://github.com/marineam/linux/commit/4b7e27ba268755963e24886e26e198531aa4da68 In short this machine is frickin weird. > If so my locking does the wrong thing for your system as: > > [ 0.393298] vgaarb: device added: PCI:0000:01:00.0,decodes=io+mem,owns=none,locks=none > [ 0.393299] vgaarb: loaded > [ 0.393300] vgaarb: setting as boot device: PCI:0000:01:00.0 > [ 0.393301] vgaarb: bridge control possible 0000:01:00.0 > ... > [ 13.693379] apple_gmux: Found gmux version 4.0.8 [indexed] > [ 13.693400] vgaarb: device changed decodes: PCI:0000:01:00.0,olddecodes=io+mem,decodes=io+mem:owns=none > [ 13.693404] apple_gmux: locked IO for PCI:0000:01:00.0 > > Here it triggers "olddecodes=none -> io+mem". > > Making sure to lock only the intel GPU when present and especially protecting > against nvidia driver will be hard if legacy-IO is being processed by a hidden > device! I had assumed that the Intel GPU was completely out of the game when hidden but if that isn't the case this sounds complicated. :( -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 27, 2015 at 08:13:23AM +0200, Bruno Prémont wrote: > On Tue, 26 May 2015 22:35:46 -0700 Michael Marineau wrote: > > On Tue, May 26, 2015 at 9:47 PM, Darren Hart <dvhart@infradead.org> wrote: > > > On Tue, May 26, 2015 at 12:10:48PM -0700, Michael Marineau wrote: > > >> FYI, this actually broke backlight controls on my MBP11,3 because the > > >> assumption the patch makes that gmux is always loaded before graphics > > >> drivers didn't hold true. At least for me dracut included the nouveau > > >> module in the initrd but not gmux, ensuring the ordering was wrong. No > > >> errors were reporting, and gmux still offered the backlight device, it > > >> just became inoperable. I worked around this for my kernel by building > > >> gmux into vmlinuz instead of as a module but that isn't going to in > > >> more general configs because there is an apple backlight driver which > > >> cannot be built at all in that configuration. > > >> > > > > > > Thank you for reporting this Michael, > > > > > > That is tough as nouveau doesn't have an explicit dependency on gmux, so we > > > could do something like a passive request_module(), but if it isn't in the > > > initrd image, it would still fail as you describe. > > > > > >> Is there a way to make the ordering between nouveau and gmux more > > >> explicit/reliable? Can gmux complain loudly if the ordering is ever > > >> wrong? > > > > > > It should print an error if the probe fails due to the IO already being in use > > > or if it can't be allocated. The disabled IO case is only info level though, > > > perhaps that should be higher priority. Printing something when failing to probe > > > seems like a reasonable thing to do. > > > > > > Michael, which message do you get if you boot with "debug" or "loglevel=6" when > > > apple-gmux is not built-in? > > > > No error, gmux claims it worked: > > [ 13.693379] apple_gmux: Found gmux version 4.0.8 [indexed] > > [ 13.693400] vgaarb: device changed decodes: > > PCI:0000:01:00.0,olddecodes=io+mem,decodes=io+mem:owns=none > > [ 13.693404] apple_gmux: locked IO for PCI:0000:01:00.0 > > > > Full dmesg: https://gist.github.com/marineam/0e5a23548e8b3b2e1d50 > > What GPUs are there in your MBP11,3? From your dmesg it looks like all > you have is NVIDIA GPU 0000:01:00.0 (lspci?). > > Is there a somehow hidden intel GPU around doing the backlight? > If so my locking does the wrong thing for your system as: > > [ 0.393298] vgaarb: device added: PCI:0000:01:00.0,decodes=io+mem,owns=none,locks=none > [ 0.393299] vgaarb: loaded > [ 0.393300] vgaarb: setting as boot device: PCI:0000:01:00.0 > [ 0.393301] vgaarb: bridge control possible 0000:01:00.0 > ... > [ 13.693379] apple_gmux: Found gmux version 4.0.8 [indexed] > [ 13.693400] vgaarb: device changed decodes: PCI:0000:01:00.0,olddecodes=io+mem,decodes=io+mem:owns=none > [ 13.693404] apple_gmux: locked IO for PCI:0000:01:00.0 > > Here it triggers "olddecodes=none -> io+mem". > > Making sure to lock only the intel GPU when present and especially protecting > against nvidia driver will be hard if legacy-IO is being processed by a hidden > device! Ugh indeed. Worst case we can special case via dmi strings. Is this Apple device significantly different from others? Bruno, what are you testing on?
On Fri, 29 May 2015 18:36:50 +0200 Darren Hart wrote: > > Making sure to lock only the intel GPU when present and especially protecting > > against nvidia driver will be hard if legacy-IO is being processed by a hidden > > device! > > Ugh indeed. Worst case we can special case via dmi strings. Is this Apple device > significantly different from others? Bruno, what are you testing on? I only own a pretty old MacBook Air with just NVIDIA IGP and had to rely on BUG reports and testing from affected users. Not doing anything on apple-gmux when only a single GPU is visible should be easy, but denying any vgaarb operation when Intel IGP is hidden and just discrete GPU present is much harder (if one does not want to risk opening the next can of worms). DMI based special-casing would work but will it uncover the next issue with the same device configured differently? Bruno -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 01, 2015 at 08:22:27AM +0200, Bruno Prémont wrote: > On Fri, 29 May 2015 18:36:50 +0200 Darren Hart wrote: > > > Making sure to lock only the intel GPU when present and especially protecting > > > against nvidia driver will be hard if legacy-IO is being processed by a hidden > > > device! > > > > Ugh indeed. Worst case we can special case via dmi strings. Is this Apple device > > significantly different from others? Bruno, what are you testing on? > > I only own a pretty old MacBook Air with just NVIDIA IGP and had to > rely on BUG reports and testing from affected users. > > Not doing anything on apple-gmux when only a single GPU is visible > should be easy, but denying any vgaarb operation when Intel IGP is > hidden and just discrete GPU present is much harder (if one does not > want to risk opening the next can of worms). > > DMI based special-casing would work but will it uncover the next issue > with the same device configured differently? No, we would need a combination. I presume "configured differently" would mean the Intel GPU present - which would be detectable. DMI + Nvidia = do A DMI + Intel = do B
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c index b9429fb..e743b03 100644 --- a/drivers/platform/x86/apple-gmux.c +++ b/drivers/platform/x86/apple-gmux.c @@ -22,6 +22,7 @@ #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> @@ -31,6 +32,7 @@ struct apple_gmux_data { bool indexed; struct mutex index_lock; + struct pci_dev *pdev; struct backlight_device *bdev; /* switcheroo data */ @@ -415,6 +417,23 @@ 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 gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id) { struct apple_gmux_data *gmux_data; @@ -425,6 +444,7 @@ 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; @@ -475,7 +495,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\n"); + pr_info("gmux device not present or IO disabled\n"); ret = -ENODEV; goto err_release; } @@ -483,6 +503,23 @@ 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); @@ -574,6 +611,10 @@ 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); @@ -593,6 +634,11 @@ 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);