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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Feb 16, 2016 at 04:58:20PM +0100, Lukas Wunner wrote:
> 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 <lukas@xxxxxxxxx> 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.

I'd reject any driver that does this, registering, then checking, then
unregistering seems extermely unsafe. I'd really stick with mandatory
language here to make this clear.

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

Ok, makes sense. I still think adding the check to the client_register
function would be good, just as a safety measure. And I don't think you're
case of register(), check(), unregister() in case of failure is a valid
use-case. Let's not allow anyone to open-code that horror ;-)

Cheers, Daniel

> > > + *
> > > + * 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 <daniel.vetter@xxxxxxxx>
> Cc: Ben Skeggs <bskeggs@xxxxxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> ---
>  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 <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>
>  
> @@ -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 <linux/apple-gmux.h>
>  #include <linux/console.h>
>  #include <linux/delay.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  #include <linux/pm_runtime.h>
> -#include <linux/vgaarb.h>
>  #include <linux/vga_switcheroo.h>
>  
>  #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 <drm/drm_pciids.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_gem.h>
>  
> @@ -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 <linux/apple-gmux.h>
>  #include <linux/console.h>
>  #include <linux/debugfs.h>
>  #include <linux/fb.h>
> @@ -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) {}
> -- 
> 1.8.5.2 (Apple Git-48)
> 

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux