diff mbox

Backlight control does not work on an Apple dual-GPU (intel/nvidia) system using nouveau

Message ID 20160214132401.GA15948@wunner.de (mailing list archive)
State Deferred, archived
Headers show

Commit Message

Lukas Wunner Feb. 14, 2016, 1:24 p.m. UTC 
Hi Gaele,

On Sun, Feb 14, 2016 at 12:39:12AM +0100, Lukas Wunner wrote:
> On dual GPU MacBook Pros, brightness is controlled by gmux.
> It is changed by writing to gmux' I/O port range, 0x700 - 0x7FF.
> gmux is located on the LPC bus, i.e. behind the southbridge.
> 
> For communication with gmux to work, the above-mentioned I/O port
> range needs to be routed to bus 00, where the LPC bus bridge is
> located. Incidentally the integrated GPU is also located on that
> same bus, whereas the discrete GPU is on a different bus. (It is
> directly connected to the PCI Root Complex in the CPU.)

FWIW, I've cobbled together an experimental patch (included below)
which locks I/O to the LPC bridge instead of the integrated GPU.

This requires a modification to vgaarb.c to deal with not just
PCI_CLASS_DISPLAY_VGA, but also PCI_CLASS_BRIDGE_ISA devices.
As to the justification, well it *could* be argued that back in
the 1990s we had mainboards with PCI + ISA slots and for backward
compatibility we need to support VGA devices in ISA slots located
behind a PCI/ISA bridge. ;-)

Seriously though, maybe you want to give this a try and see if
it solves the issue for you (apply with git am --scissors).

Best regards,

Lukas

-- >8 --
Subject: [PATCH] apple-gmux: Fix IO locking

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/vga/vgaarb.c          |  3 ++-
 drivers/platform/x86/apple-gmux.c | 51 ++++++++++++++-------------------------
 2 files changed, 20 insertions(+), 34 deletions(-)

Comments

Gaele Strootman Feb. 20, 2016, 11:06 p.m. UTC | #1 
Hello Lukas,

Yes, your patch solves the issue for me.

Thanks,

Gaele

On 14-02-16 14:24, Lukas Wunner wrote:
> Hi Gaele,
>
> On Sun, Feb 14, 2016 at 12:39:12AM +0100, Lukas Wunner wrote:
>> On dual GPU MacBook Pros, brightness is controlled by gmux.
>> It is changed by writing to gmux' I/O port range, 0x700 - 0x7FF.
>> gmux is located on the LPC bus, i.e. behind the southbridge.
>>
>> For communication with gmux to work, the above-mentioned I/O port
>> range needs to be routed to bus 00, where the LPC bus bridge is
>> located. Incidentally the integrated GPU is also located on that
>> same bus, whereas the discrete GPU is on a different bus. (It is
>> directly connected to the PCI Root Complex in the CPU.)
> FWIW, I've cobbled together an experimental patch (included below)
> which locks I/O to the LPC bridge instead of the integrated GPU.
>
> This requires a modification to vgaarb.c to deal with not just
> PCI_CLASS_DISPLAY_VGA, but also PCI_CLASS_BRIDGE_ISA devices.
> As to the justification, well it *could* be argued that back in
> the 1990s we had mainboards with PCI + ISA slots and for backward
> compatibility we need to support VGA devices in ISA slots located
> behind a PCI/ISA bridge. ;-)
>
> Seriously though, maybe you want to give this a try and see if
> it solves the issue for you (apply with git am --scissors).
>
> Best regards,
>
> Lukas
>
> -- >8 --
> Subject: [PATCH] apple-gmux: Fix IO locking
>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/gpu/vga/vgaarb.c          |  3 ++-
>  drivers/platform/x86/apple-gmux.c | 51 ++++++++++++++-------------------------
>  2 files changed, 20 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index f17cb04..b23b471 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -529,7 +529,8 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
>  	u16 cmd;
>  
>  	/* Only deal with VGA class devices */
> -	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> +	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA &&
> +	    (pdev->class >> 8) != PCI_CLASS_BRIDGE_ISA)
>  		return false;
>  
>  	/* Allocate structure */
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index f236250..999daad 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -48,10 +48,10 @@
>  struct apple_gmux_data {
>  	unsigned long iostart;
>  	unsigned long iolen;
> +	struct pci_dev *lpc_bridge;
>  	bool indexed;
>  	struct mutex index_lock;
>  
> -	struct pci_dev *pdev;
>  	struct backlight_device *bdev;
>  
>  	/* switcheroo data */
> @@ -530,34 +530,17 @@ 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;
>  	struct resource *res;
> +	struct pci_dev *lpc_bridge;
>  	struct backlight_properties props;
>  	struct backlight_device *bdev;
>  	u8 ver_major, ver_minor, ver_release;
>  	int ret = -ENXIO;
>  	acpi_status status;
>  	unsigned long long gpe;
> -	struct pci_dev *pdev = NULL;
>  
>  	if (apple_gmux_data)
>  		return -EBUSY;
> @@ -622,16 +605,17 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  	 * 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));
> +	lpc_bridge = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
> +	if (lpc_bridge &&
> +	    !vga_tryget(lpc_bridge, VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM)) {
> +		pr_debug("Locked IO to %s\n", pci_name(lpc_bridge));
> +		gmux_data->lpc_bridge = lpc_bridge;
> +	} else {
> +		pr_err("Failed to lock IO to %s\n", pci_name(lpc_bridge));
> +		pci_dev_put(lpc_bridge);
>  		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;
> @@ -725,10 +709,11 @@ err_enable_gpe:
>  err_notify:
>  	backlight_device_unregister(bdev);
>  err_release:
> -	if (gmux_data->pdev)
> -		vga_put(gmux_data->pdev,
> +	if (gmux_data->lpc_bridge) {
> +		vga_put(gmux_data->lpc_bridge,
>  			VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM);
> -	pci_dev_put(pdev);
> +		pci_dev_put(gmux_data->lpc_bridge);
> +	}
>  	release_region(gmux_data->iostart, gmux_data->iolen);
>  err_free:
>  	kfree(gmux_data);
> @@ -748,10 +733,10 @@ static void gmux_remove(struct pnp_dev *pnp)
>  					   &gmux_notify_handler);
>  	}
>  
> -	if (gmux_data->pdev) {
> -		vga_put(gmux_data->pdev,
> +	if (gmux_data->lpc_bridge) {
> +		vga_put(gmux_data->lpc_bridge,
>  			VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM);
> -		pci_dev_put(gmux_data->pdev);
> +		pci_dev_put(gmux_data->lpc_bridge);
>  	}
>  	backlight_device_unregister(gmux_data->bdev);
>  

--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index f17cb04..b23b471 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -529,7 +529,8 @@  static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
 	u16 cmd;
 
 	/* Only deal with VGA class devices */
-	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
+	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA &&
+	    (pdev->class >> 8) != PCI_CLASS_BRIDGE_ISA)
 		return false;
 
 	/* Allocate structure */
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index f236250..999daad 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -48,10 +48,10 @@ 
 struct apple_gmux_data {
 	unsigned long iostart;
 	unsigned long iolen;
+	struct pci_dev *lpc_bridge;
 	bool indexed;
 	struct mutex index_lock;
 
-	struct pci_dev *pdev;
 	struct backlight_device *bdev;
 
 	/* switcheroo data */
@@ -530,34 +530,17 @@  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;
 	struct resource *res;
+	struct pci_dev *lpc_bridge;
 	struct backlight_properties props;
 	struct backlight_device *bdev;
 	u8 ver_major, ver_minor, ver_release;
 	int ret = -ENXIO;
 	acpi_status status;
 	unsigned long long gpe;
-	struct pci_dev *pdev = NULL;
 
 	if (apple_gmux_data)
 		return -EBUSY;
@@ -622,16 +605,17 @@  static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	 * 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));
+	lpc_bridge = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
+	if (lpc_bridge &&
+	    !vga_tryget(lpc_bridge, VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM)) {
+		pr_debug("Locked IO to %s\n", pci_name(lpc_bridge));
+		gmux_data->lpc_bridge = lpc_bridge;
+	} else {
+		pr_err("Failed to lock IO to %s\n", pci_name(lpc_bridge));
+		pci_dev_put(lpc_bridge);
 		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;
@@ -725,10 +709,11 @@  err_enable_gpe:
 err_notify:
 	backlight_device_unregister(bdev);
 err_release:
-	if (gmux_data->pdev)
-		vga_put(gmux_data->pdev,
+	if (gmux_data->lpc_bridge) {
+		vga_put(gmux_data->lpc_bridge,
 			VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM);
-	pci_dev_put(pdev);
+		pci_dev_put(gmux_data->lpc_bridge);
+	}
 	release_region(gmux_data->iostart, gmux_data->iolen);
 err_free:
 	kfree(gmux_data);
@@ -748,10 +733,10 @@  static void gmux_remove(struct pnp_dev *pnp)
 					   &gmux_notify_handler);
 	}
 
-	if (gmux_data->pdev) {
-		vga_put(gmux_data->pdev,
+	if (gmux_data->lpc_bridge) {
+		vga_put(gmux_data->lpc_bridge,
 			VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM);
-		pci_dev_put(gmux_data->pdev);
+		pci_dev_put(gmux_data->lpc_bridge);
 	}
 	backlight_device_unregister(gmux_data->bdev);