[Patch] apple-gmux: lock iGP IO to protect from vgaarb changes

[Patch] apple-gmux: lock iGP IO to protect from vgaarb changes

[Bruno Prémont]

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>
---
 drivers/platform/x86/apple-gmux.c | 43 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index b9429fb..22da6a3 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,22 @@ static int gmux_resume(struct device *dev)
 	return 0;
 }
 
+static struct pci_dev *gmux_find_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 +443,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 +494,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 +502,22 @@ 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_find_pdev();
+	if (pdev && vga_tryget(pdev,
+			       VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM)) {
+		pr_err("gmux boot_vga IO+MEM vgaarb-locking failed\n");
+		ret = -EBUSY;
+		goto err_release;
+	} else if (pdev)
+		pr_info("gmux: 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 +609,7 @@ err_enable_gpe:
 err_notify:
 	backlight_device_unregister(bdev);
 err_release:
+	pci_dev_put(pdev);
 	release_region(gmux_data->iostart, gmux_data->iolen);
 err_free:
 	kfree(gmux_data);
@@ -593,6 +629,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);

Re: [Patch] apple-gmux: lock iGP IO to protect from vgaarb changes

[Darren Hart]

On Mon, Feb 23, 2015 at 09:51:55PM +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>

Hi Bruno,

Only a minor nit below.

> ---
>  drivers/platform/x86/apple-gmux.c | 43 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index b9429fb..22da6a3 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c

...

> @@ -475,7 +494,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 +502,22 @@ 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_find_pdev();
> +	if (pdev && vga_tryget(pdev,
> +			       VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM)) {
> +		pr_err("gmux boot_vga IO+MEM vgaarb-locking failed\n");

On this and the above, keep in mind the pr_fmt will already prefix this with
KBUILD_MODNAME, the "gmux" is probably redundant. 

> +		ret = -EBUSY;
> +		goto err_release;
> +	} else if (pdev)
> +		pr_info("gmux: locked IO for PCI:%s\n", pci_name(pdev));

The driver uses pr_fmt, so no need to prefix with "gmux:"
-- 
Darren Hart
Intel Open Source Technology Center

[Patch v2] apple-gmux: lock iGP IO to protect from vgaarb changes

[Bruno Prémont]

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>
---
Respinning, fixing Darren's nit.

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 | 43 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index b9429fb..22da6a3 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,22 @@ static int gmux_resume(struct device *dev)
 	return 0;
 }
 
+static struct pci_dev *gmux_find_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 +443,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 +494,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 +502,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_find_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 +609,7 @@ err_enable_gpe:
 err_notify:
 	backlight_device_unregister(bdev);
 err_release:
+	pci_dev_put(pdev);
 	release_region(gmux_data->iostart, gmux_data->iolen);
 err_free:
 	kfree(gmux_data);
@@ -593,6 +629,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);

Re: [Patch v2] apple-gmux: lock iGP IO to protect from vgaarb changes

[Darren Hart]

On Thu, Mar 05, 2015 at 11:20:38PM +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>
> ---
> Respinning, fixing Darren's nit.
> 
> 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

Hi Bruno,

I don't know if this is on your end or mine (I've not seen this before). Saving
off your patch (through mutt like I do everything else) to a file and applying
works, build works, and git show and visual inspection look correct.

However, checkpatch sees a lot of =3D instead of just =, and complains bitterly.

Can you check this patch (from the list) and let me know what you find?


ERROR: patch seems to be corrupt (line wrapped?)
#93: FILE: drivers/platform/x86/apple-gmux.c:27:
=20

ERROR: spaces required around that '=' (ctx:WxV)
#108: FILE: drivers/platform/x86/apple-gmux.c:421:
+	struct pci_dev *pdev =3D NULL;
 	                     ^

WARNING: Missing a blank line after declarations
#109: FILE: drivers/platform/x86/apple-gmux.c:422:
+	struct pci_dev *pdev =3D NULL;
+	while ((pdev =3D pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev))) {

ERROR: spaces required around that '=' (ctx:WxV)
#109: FILE: drivers/platform/x86/apple-gmux.c:422:
+	while ((pdev =3D pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev))) {
 	             ^

ERROR: spaces required around that '=' (ctx:WxV)
#130: FILE: drivers/platform/x86/apple-gmux.c:447:
+	struct pci_dev *pdev =3D NULL;
 	                     ^

ERROR: spaces required around that '=' (ctx:WxV)
#155: FILE: drivers/platform/x86/apple-gmux.c:510:
+	pdev =3D gmux_find_pdev();
 	     ^

ERROR: spaces required around that '=' (ctx:WxV)
#160: FILE: drivers/platform/x86/apple-gmux.c:515:
+		ret =3D -EBUSY;
 		    ^

ERROR: need consistent spacing around '-' (ctx:WxV)
#160: FILE: drivers/platform/x86/apple-gmux.c:515:
+		ret =3D -EBUSY;
 		        ^

ERROR: spaces required around that '=' (ctx:WxV)
#164: FILE: drivers/platform/x86/apple-gmux.c:519:
+	gmux_data->pdev =3D pdev;
 	                ^

total: 8 errors, 1 warnings, 96 lines checked

/home/dvhart/apple.patch has style problems, please review.

If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [Patch v2] apple-gmux: lock iGP IO to protect from vgaarb changes

[Bruno Prémont]

On Fri, 06 March 2015 Darren Hart <dvhart@infradead.org> wrote:
> On Thu, Mar 05, 2015 at 11:20:38PM +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>
> > ---
> > Respinning, fixing Darren's nit.
> > 
> > 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

Hi Darren,

> Hi Bruno,
> 
> I don't know if this is on your end or mine (I've not seen this before). Saving
> off your patch (through mutt like I do everything else) to a file and applying
> works, build works, and git show and visual inspection look correct.
> 
> However, checkpatch sees a lot of =3D instead of just =, and complains bitterly.
> 
> Can you check this patch (from the list) and let me know what you find?

That smells like `Content-Transfer-Encoding: quoted-printable` not being
understood by check-patch when operating on a raw mail.
No idea if mutt can recode the mail to plain 8-bit/UTF-8 when saving...
patch from patch-utils might have the same issues with it while git-am is
fine (as it handles mail-patches including charset and other headers).

I'm sending the mail via claws-mail which might encode it more
conservatively than e.g. git-send-email. I can see if there's an option
to prefer 8-bit over quoted-printable though.

Bruno


> ERROR: patch seems to be corrupt (line wrapped?)
> #93: FILE: drivers/platform/x86/apple-gmux.c:27:
> =20
> 
> ERROR: spaces required around that '=' (ctx:WxV)
> #108: FILE: drivers/platform/x86/apple-gmux.c:421:
> +	struct pci_dev *pdev =3D NULL;
>  	                     ^
> 
> WARNING: Missing a blank line after declarations
> #109: FILE: drivers/platform/x86/apple-gmux.c:422:
> +	struct pci_dev *pdev =3D NULL;
> +	while ((pdev =3D pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev))) {
> 
> ERROR: spaces required around that '=' (ctx:WxV)
> #109: FILE: drivers/platform/x86/apple-gmux.c:422:
> +	while ((pdev =3D pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev))) {
>  	             ^
> 
> ERROR: spaces required around that '=' (ctx:WxV)
> #130: FILE: drivers/platform/x86/apple-gmux.c:447:
> +	struct pci_dev *pdev =3D NULL;
>  	                     ^
> 
> ERROR: spaces required around that '=' (ctx:WxV)
> #155: FILE: drivers/platform/x86/apple-gmux.c:510:
> +	pdev =3D gmux_find_pdev();
>  	     ^
> 
> ERROR: spaces required around that '=' (ctx:WxV)
> #160: FILE: drivers/platform/x86/apple-gmux.c:515:
> +		ret =3D -EBUSY;
>  		    ^
> 
> ERROR: need consistent spacing around '-' (ctx:WxV)
> #160: FILE: drivers/platform/x86/apple-gmux.c:515:
> +		ret =3D -EBUSY;
>  		        ^
> 
> ERROR: spaces required around that '=' (ctx:WxV)
> #164: FILE: drivers/platform/x86/apple-gmux.c:519:
> +	gmux_data->pdev =3D pdev;
>  	                ^
> 
> total: 8 errors, 1 warnings, 96 lines checked
> 
> /home/dvhart/apple.patch has style problems, please review.
> 
> If any of these errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
> 

Re: [Patch v2 resend] apple-gmux: lock iGP IO to protect from vgaarb changes

[Bruno Prémont]

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 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 | 43 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index b9429fb..22da6a3 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,22 @@ static int gmux_resume(struct device *dev)
 	return 0;
 }
 
+static struct pci_dev *gmux_find_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 +443,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 +494,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 +502,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_find_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 +609,7 @@ err_enable_gpe:
 err_notify:
 	backlight_device_unregister(bdev);
 err_release:
+	pci_dev_put(pdev);
 	release_region(gmux_data->iostart, gmux_data->iolen);
 err_free:
 	kfree(gmux_data);
@@ -593,6 +629,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);

Re: [Patch v2 resend] apple-gmux: lock iGP IO to protect from vgaarb changes

[Bjorn Helgaas]

On Mon, Mar 9, 2015 at 4:52 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.

The bugzilla is assigned to PCI, and I was just trying to resolve it.
But it looks like Darren might be the logical person to apply this, so
I'll ignore it unless poked again.

A few minor comments below.

> 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 | 43 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index b9429fb..22da6a3 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,22 @@ static int gmux_resume(struct device *dev)
>         return 0;
>  }
>
> +static struct pci_dev *gmux_find_pdev(void)
> +{
> +       struct pci_dev *pdev = NULL;

Typical to have a blank line here (between local variables and code).

> +       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;

This returns a device with reference count incremented.  The
convention in PCI, at least, is to name functions that acquire a
reference with "_get_", and functions that don't acquire a reference
with "_find_".  So maybe this function should be renamed?

> +       }
> +
> +       return NULL;
> +}
> +
>  static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  {
>         struct apple_gmux_data *gmux_data;
> @@ -425,6 +443,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 +494,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 +502,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_find_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));

It'd be nice to use dev_info/dev_err() for all the messages here, but
I see the existing style is to just use pr_info()/pr_err().  So a
change to use the dev_ functions would be out of scope for the bug fix
itself, but possibly a useful follow-on.

> +               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 +609,7 @@ err_enable_gpe:
>  err_notify:
>         backlight_device_unregister(bdev);
>  err_release:
> +       pci_dev_put(pdev);
>         release_region(gmux_data->iostart, gmux_data->iolen);
>  err_free:
>         kfree(gmux_data);
> @@ -593,6 +629,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);

[Patch v3] apple-gmux: lock iGP IO to protect from vgaarb changes

[Bruno Prémont]

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);

Re: [Patch v3] apple-gmux: lock iGP IO to protect from vgaarb changes

[Darren Hart]

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.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [Patch v3] apple-gmux: lock iGP IO to protect from vgaarb changes

[Michael Marineau]

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/

Re: [Patch v3] apple-gmux: lock iGP IO to protect from vgaarb changes

[Darren Hart]

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?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [Patch v3] apple-gmux: lock iGP IO to protect from vgaarb changes

[Michael Marineau]

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

Re: [Patch v3] apple-gmux: lock iGP IO to protect from vgaarb changes

[Bruno Prémont]

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

Re: [Patch v3] apple-gmux: lock iGP IO to protect from vgaarb changes

[Michael Marineau]

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. :(

Re: [Patch v3] apple-gmux: lock iGP IO to protect from vgaarb changes

[Darren Hart]

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?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [Patch v3] apple-gmux: lock iGP IO to protect from vgaarb changes

[Bruno Prémont]

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

Re: [Patch v3] apple-gmux: lock iGP IO to protect from vgaarb changes

[Darren Hart]

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

-- 
Darren Hart
Intel Open Source Technology Center

Re: [Patch v3] apple-gmux: lock iGP IO to protect from vgaarb changes

[Bruno Prémont]

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

Re: [Patch v3] apple-gmux: lock iGP IO to protect from vgaarb changes

[Michael Marineau]

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.