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

Darren Hart
2015-03-03T17:27:55Z

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