Re: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

From: Daniel Vetter <daniel@ffwll.ch>
To: Lukas Wunner <lukas@wunner.de>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't
Date: Tue, 9 Feb 2016 10:04:01 +0100	[thread overview]
Message-ID: <20160209090401.GA11240@phenom.ffwll.local> (raw)
In-Reply-To: <f56ee6a0600a3e1bb5bed4d0db4ed9ade7445c47.1452525860.git.lukas@wunner.de>

On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
> gmux is a microcontroller built into dual GPU MacBook Pros.
> On pre-retina MBPs, if we're the inactive GPU, we need apple-gmux
> to temporarily switch DDC so that we can probe the panel's EDID.
> 
> The checks for CONFIG_VGA_ARB and CONFIG_VGA_SWITCHEROO are necessary
> because if either of them is disabled but gmux is present, the driver
> would never load, even if we're the active GPU. (vga_default_device()
> would evaluate to NULL and vga_switcheroo_handler_flags() would
> evaluate to 0.)
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> Tested-by: Lukas Wunner <lukas@wunner.de>
>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3ac616d..4a5fc5d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -35,9 +35,12 @@
>  #include "i915_trace.h"
>  #include "intel_drv.h"
>  
> +#include <linux/apple-gmux.h>
>  #include <linux/console.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/vgaarb.h>
> +#include <linux/vga_switcheroo.h>
>  #include <drm/drm_crtc_helper.h>
>  
>  static struct drm_driver driver;
> @@ -967,6 +970,15 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (PCI_FUNC(pdev->devfn))
>  		return -ENODEV;
>  
> +	/*
> +	 * apple-gmux is needed on dual GPU MacBook Pro
> +	 * to probe the panel if we're the inactive GPU.
> +	 */
> +	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
> +	    apple_gmux_present() && pdev != vga_default_device() &&
> +	    !vga_switcheroo_handler_flags())
> +		return -EPROBE_DEFER;

I pulled in all patches to drm-misc, but this here is imo ugly and needs
to be polished a bit. What about adding a vga_switcheroo_ready() function
which contains this check (and might in the future contain even more
checks)? Then i915/radeon/nouveau would just have a simple

	if (!vga_switcheroo_ready())
		return -EPROBE_DEFER;

and instead of duplicating the same comment 3 times we could have it once
in one place. Plus some neat kerneldoc for this new helper to describe how
it's supposed to be used. Plus better encapsulation of concepts.

Can you pls follow up with a patch/series to do that?

Thanks, Daniel

> +
>  	return drm_get_pci_dev(pdev, ent, &driver);
>  }
>  
> -- 
> 1.8.5.2 (Apple Git-48)
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel