From patchwork Tue Feb 16 15:58:20 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lukas Wunner X-Patchwork-Id: 8328741 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id D9B0FC02AA for ; Tue, 16 Feb 2016 15:58:02 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 81CDA202EC for ; Tue, 16 Feb 2016 15:58:01 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id A5871202B8 for ; Tue, 16 Feb 2016 15:57:59 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 160F96E1EE; Tue, 16 Feb 2016 15:57:59 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mailout2.hostsharing.net (mailout2.hostsharing.net [83.223.90.233]) by gabe.freedesktop.org (Postfix) with ESMTPS id BC3A06E1EE; Tue, 16 Feb 2016 15:57:57 +0000 (UTC) Received: from h08.hostsharing.net (h08.hostsharing.net [83.223.95.28]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mailout2.hostsharing.net (Postfix) with ESMTPS id BDBF610189AC1; Tue, 16 Feb 2016 16:57:54 +0100 (CET) Received: from localhost (6-38-90-81.adsl.cmo.de [81.90.38.6]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by h08.hostsharing.net (Postfix) with ESMTPSA id 62F1D604201F; Tue, 16 Feb 2016 16:57:46 +0100 (CET) Date: Tue, 16 Feb 2016 16:58:20 +0100 From: Lukas Wunner To: Daniel Vetter Subject: Re: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't Message-ID: <20160216155820.GA17446@wunner.de> References: <20160209090401.GA11240@phenom.ffwll.local> <20160214121008.GA16847@wunner.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Cc: Alex Deucher , intel-gfx , Ben Skeggs , dri-devel , platform-driver-x86@vger.kernel.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi, On Sun, Feb 14, 2016 at 01:46:28PM +0100, Daniel Vetter wrote: > On Sun, Feb 14, 2016 at 1:10 PM, Lukas Wunner wrote: > > /** > > + * vga_switcheroo_client_probe_defer() - whether to defer probing a given GPU > > + * @pdev: pci device of GPU > > + * > > + * Determine whether any prerequisites are not fulfilled to probe a given GPU. > > I'd phrase this as "Determine whether the vgaswitcheroor support is > fully loaded" and drop the GPU part - it could be needed by snd > drivers eventually too. Ok, I've rephrased the kerneldoc to refer to "client" instead of "GPU" and moved the single existing check in an if block for PCI_CLASS_DISPLAY_VGA devices. > > + * DRM drivers should invoke this early on in their ->probe callback and return > > + * %-EPROBE_DEFER if it evaluates to %true. The GPU need not be registered with > > + * vga_switcheroo_register_client() beforehand. > > s/need not/must not/ ... is your native language German by any chance? In principle there's no harm in registering the client first, then checking if probing should be deferred, as long as the client is unregistered before deferring. Thus the language above is intentionally "need not" (muss nicht) rather than "must not" (darf nicht). I didn't want to mandate something that isn't actually required. The above sentence is merely an aid for driver developers who might be confused in which order to call what. > Aside from that ... should vga_switcheroo_register_client call this > helper instead directly and return the appropriate -EDEFER_PROBE > error? I realize for some drivers it might be way too late to unrol > from that point on, but e.g. i915 already uses -EDEFER_PROBE. And > calling it unconditionally will make sure that it's easier to notice > when people forget this. So maybe call it both from the register > functions, and export as a separate hook? And for i915 we should be > able to remove that early check entirely. I'm afraid that wouldn't be a good idea. The ->probe hook is potentially called dozens of times during boot until it finally succeeds and vga_switcheroo_register_client() is usually called in a fairly late driver load stage. In i915, we already have a working GEM at that point and an almost fully brought up GPU. Repeating bring up and tear down dozens of times is a nice stress test but nothing I'd inflict on production systems. I imagine it would also severely impact boot time and clutter the kernel log. So a separate helper seems to be the only sensible option. > > + * > > + * Return: %false unless one of the following applies: > > + * > > + * * On pre-retina MacBook Pros, the apple-gmux driver is needed to temporarily > > + * switch DDC to the inactive GPU so that it can probe the panel's EDID. > > + * Therefore return %true if gmux is built into the machine, @pdev is the > > + * inactive GPU and the apple-gmux driver has not registered its handler > > + * flags, signifying it has not yet loaded or has not finished initializing. > > I think the apple-specific comment here should be a separate comment > right above the if condition below - it doesn't explain the interface, > only one specific case. And we might grow more of those. Ok, I've moved that to a comment. Updated patch follows after the scissors & perforation. Thanks for the feedback! Lukas -- >8 -- Subject: [PATCH] vga_switcheroo: Allow clients to determine whether to defer probing So far we've got one condition when DRM drivers need to defer probing on a dual GPU system and it's coded separately into each of the relevant drivers. As suggested by Daniel Vetter, deduplicate that code in the drivers and move it to a new vga_switcheroo helper. This yields better encapsulation of concepts and lets us add further checks in a central place. (The existing check pertains to pre-retina MacBook Pros and an additional check is expected to be needed for retinas.) v2: This helper could eventually be used by audio clients as well, so rephrase kerneldoc to refer to "client" instead of "GPU" and move the single existing check in an if block specific to PCI_CLASS_DISPLAY_VGA devices. Move documentation on that check from kerneldoc to a comment. (Daniel Vetter) Cc: Daniel Vetter Cc: Ben Skeggs Cc: Alex Deucher Signed-off-by: Lukas Wunner --- drivers/gpu/drm/i915/i915_drv.c | 10 +--------- drivers/gpu/drm/nouveau/nouveau_drm.c | 10 +--------- drivers/gpu/drm/radeon/radeon_drv.c | 10 +--------- drivers/gpu/vga/vga_switcheroo.c | 28 ++++++++++++++++++++++++++++ include/linux/vga_switcheroo.h | 2 ++ 5 files changed, 33 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index e8d0f17..01ef24a 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -35,11 +35,9 @@ #include "i915_trace.h" #include "intel_drv.h" -#include #include #include #include -#include #include #include @@ -970,13 +968,7 @@ 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()) + if (vga_switcheroo_client_probe_defer(pdev)) return -EPROBE_DEFER; return drm_get_pci_dev(pdev, ent, &driver); diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index bb8498c..9141bcd 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -22,13 +22,11 @@ * Authors: Ben Skeggs */ -#include #include #include #include #include #include -#include #include #include "drmP.h" @@ -314,13 +312,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev, bool boot = false; int ret; - /* - * 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()) + if (vga_switcheroo_client_probe_defer(pdev)) return -EPROBE_DEFER; /* remove conflicting drivers (vesafb, efifb etc) */ diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index cad2555..7be0c38 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -34,11 +34,9 @@ #include "radeon_drv.h" #include -#include #include #include #include -#include #include #include @@ -321,13 +319,7 @@ static int radeon_pci_probe(struct pci_dev *pdev, { int ret; - /* - * 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()) + if (vga_switcheroo_client_probe_defer(pdev)) return -EPROBE_DEFER; /* Get rid of things like offb */ diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c index 56287ae..6108ebe 100644 --- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -30,6 +30,7 @@ #define pr_fmt(fmt) "vga_switcheroo: " fmt +#include #include #include #include @@ -375,6 +376,33 @@ find_active_client(struct list_head *head) } /** + * vga_switcheroo_client_probe_defer() - whether to defer probing a given client + * @pdev: client pci device + * + * Determine whether any prerequisites are not fulfilled to probe a given + * client. Drivers should invoke this early on in their ->probe callback + * and return %-EPROBE_DEFER if it evaluates to %true. The client need + * not be registered with vga_switcheroo_register_client() beforehand. + * + * Return: %true if probing should be deferred, otherwise %false. + */ +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev) +{ + if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) { + /* + * apple-gmux is needed on pre-retina MacBook Pro + * to probe the panel if pdev is the inactive GPU. + */ + if (apple_gmux_present() && pdev != vga_default_device() && + !vgasr_priv.handler_flags) + return true; + } + + return false; +} +EXPORT_SYMBOL(vga_switcheroo_client_probe_defer); + +/** * vga_switcheroo_get_client_state() - obtain power state of a given client * @pdev: client pci device * diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h index b39a5f3..960bedb 100644 --- a/include/linux/vga_switcheroo.h +++ b/include/linux/vga_switcheroo.h @@ -165,6 +165,7 @@ int vga_switcheroo_unlock_ddc(struct pci_dev *pdev); int vga_switcheroo_process_delayed_switch(void); +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev); enum vga_switcheroo_state vga_switcheroo_get_client_state(struct pci_dev *dev); void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic); @@ -188,6 +189,7 @@ static inline enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(v static inline int vga_switcheroo_lock_ddc(struct pci_dev *pdev) { return -ENODEV; } static inline int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) { return -ENODEV; } static inline int vga_switcheroo_process_delayed_switch(void) { return 0; } +static inline bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev) { return false; } static inline enum vga_switcheroo_state vga_switcheroo_get_client_state(struct pci_dev *dev) { return VGA_SWITCHEROO_ON; } static inline void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic) {}