Re: [Intel-gfx] [PATCH v5] vga_switcheroo: Add helper for deferred probing

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

 



On Mon, 23 May 2016, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Sat, May 21, 2016 at 03:59:16PM +0100, Lukas Wunner wrote:
>> 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)
>> 
>> v3: Mandate in kerneldoc that registration of client shall only
>>     happen after calling this helper. (Daniel Vetter)
>> 
>> v4: Rebase on 412c8f7de011 ("drm/radeon: Return -EPROBE_DEFER when
>>     amdkfd not loaded")
>> 
>> v5: Some Optimus GPUs use PCI_CLASS_DISPLAY_3D, make sure those are
>>     matched as well. (Emil Velikov)
>> 
>> 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      | 34 ++++++++++++++++++++++++++++++++--
>>  include/linux/vga_switcheroo.h        |  2 ++
>>  5 files changed, 37 insertions(+), 29 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index d37c0a6..20d5898 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>
>>  
>> @@ -1025,13 +1023,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 11f8dd9..5c4d4df 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"
>> @@ -315,13 +313,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 b55aa74..a455dc7 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>
>>  
>> @@ -340,13 +338,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
>>  	if (ret == -EPROBE_DEFER)
>>  		return 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 cbd7c98..2df216b3 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>
>> @@ -308,7 +309,8 @@ static int register_client(struct pci_dev *pdev,
>>   *
>>   * Register vga client (GPU). Enable vga_switcheroo if another GPU and a
>>   * handler have already registered. The power state of the client is assumed
>> - * to be ON.
>> + * to be ON. Beforehand, vga_switcheroo_client_probe_defer() shall be called
>> + * to ensure that all prerequisites are met.
>
> "shall" or "must"? It thought it's the latter ...

The meaning of must... RFC 2119 says MUST is equivalent to REQUIRED or
SHALL, and I think for our purposes that SHOULD matter more than
dictionaries. ;)

J.


> -Daniel
>>   *
>>   * Return: 0 on success, -ENOMEM on memory allocation error.
>>   */
>> @@ -329,7 +331,8 @@ EXPORT_SYMBOL(vga_switcheroo_register_client);
>>   * @id: client identifier
>>   *
>>   * Register audio client (audio device on a GPU). The power state of the
>> - * client is assumed to be ON.
>> + * client is assumed to be ON. Beforehand, vga_switcheroo_client_probe_defer()
>> + * shall be called to ensure that all prerequisites are met.
>>   *
>>   * Return: 0 on success, -ENOMEM on memory allocation error.
>>   */
>> @@ -376,6 +379,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 shall invoke this early on in their ->probe callback
>> + * and return %-EPROBE_DEFER if it evaluates to %true. Thou shalt not
>> + * register the client ere thou hast called this.
>
> I still think we should just smash this into the relevant _register
> functions(), for safety. But I forgot the actual result of the previous
> discussion ...
>
> Imo if you can replace a bit of documentation with code to enforce an
> invariant, always do it.
> -Daniel
>
>> + *
>> + * Return: %true if probing should be deferred, otherwise %false.
>> + */
>> +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev)
>> +{
>> +	if ((pdev->class >> 16) == PCI_BASE_CLASS_DISPLAY) {
>> +		/*
>> +		 * 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) {}
>> -- 
>> 2.8.1
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
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