Re: [PATCH v3 1/6] vga_switcheroo: Add support for switching only the DDC

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

 



On Fri, Aug 14, 2015 at 06:50:15PM +0200, Lukas Wunner wrote:
> Originally by Seth Forshee <seth.forshee@xxxxxxxxxxxxx>, 2012-10-04:
>     During graphics driver initialization it's useful to be able to mux
>     only the DDC to the inactive client in order to read the EDID. Add
>     a switch_ddc callback to allow capable handlers to provide this
>     functionality, and add vga_switcheroo_switch_ddc() to allow DRM
>     to mux only the DDC.
> 
> Modified by Dave Airlie <airlied@xxxxxxxxx>, 2012-12-22:
>     I can't figure out why I didn't like this, but I rewrote this [...]
>     to lock/unlock the ddc lines [...]. I think I'd prefer something
>     like that otherwise the interface got really ugly.
> 
> Modified by Lukas Wunner <lukas@xxxxxxxxx>, 2015-08-14:
>     Don't lock vgasr_mutex in _lock_ddc() / _unlock_ddc(), it can cause
>     deadlocks because (a) during switch (with vgasr_mutex already held),
>     GPU is woken and probes its outputs, tries to re-acquire vgasr_mutex
>     to lock DDC lines; (b) Likewise during switch, GPU is suspended and
>     calls cancel_delayed_work_sync() to stop output polling, if poll
>     task is running at this moment we may wait forever for it to finish.
> 
>     Instead, lock ddc_lock when unregistering the handler because the
>     only reason why we'd want to lock vgasr_mutex in _lock_ddc() /
>     _unlock_ddc() is to block the handler from disappearing while DDC
>     lines are switched.

Shouldn't we also take the new look in register_handler, for consistency?
I know that if you look the mux provider too late it's fairly hopeless ...

Also while reading the patch I realized that the new lock really protects
hw state (instead of our software-side tracking and driver resume/suspend
code). And at least myself I have no idea what vgasr means, so what about
renaming it to hw_mutex? We have this pattern of low-level locks to avoid
concurrent hw access in a lot of places like i2c, dp_aux, and it's very
often called hw_lock or similar.

Otherwise patch looks good.

Wrt the overall patch series, can you pls haggle driver-maintainers (gmux,
radeon, nouveau) for acks/reviews? scripts/get_maintainers.pl should help.
Also, the revised docbook patch seems to be missing from this iteration,
please follow up with that one.

Thanks, Daniel

>     Also lock ddc_lock in stage2 to avoid race condition where reading
>     the EDID and switching happens simultaneously. Likewise on MIGD /
>     MDIS commands and on runtime suspend.
> 
>     Retain semantics of ->switchto handler callback to switch all pins,
>     including DDC. Change semantics of ->switch_ddc handler callback to
>     return previous DDC owner. Original version tried to determine
>     previous DDC owner with find_active_client() but this fails if the
>     inactive client registers before the active client.
> 
> v2.1: Overhaul locking, squash commits
>     (requested by Daniel Vetter <daniel.vetter@xxxxxxxx>)
> 
> v2.2: Readability improvements
>     (requested by Thierry Reding <thierry.reding@xxxxxxxxx>)
> 
> v2.3: Overhaul locking once more
> 
> v2.4: Retain semantics of ->switchto handler callback to switch all pins
>     (requested by Daniel Vetter <daniel.vetter@xxxxxxxx>)
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> Tested-by: Pierre Moreau <pierre.morrow@xxxxxxx>
>     [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]
> Tested-by: William Brown <william@xxxxxxxxxxxxxxxx>
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
> Tested-by: Lukas Wunner <lukas@xxxxxxxxx>
>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
> 
> Cc: Seth Forshee <seth.forshee@xxxxxxxxxxxxx>
> Cc: Dave Airlie <airlied@xxxxxxxxx>
> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> ---
>  drivers/gpu/vga/vga_switcheroo.c | 98 +++++++++++++++++++++++++++++++++++++++-
>  include/linux/vga_switcheroo.h   |  9 ++++
>  2 files changed, 105 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index aa077aa..9b6946e 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -73,9 +73,19 @@
>   * there can thus be up to three clients: Two vga clients (GPUs) and one audio
>   * client (on the discrete GPU). The code is mostly prepared to support
>   * machines with more than two GPUs should they become available.
> + *
>   * The GPU to which the outputs are currently switched is called the
>   * active client in vga_switcheroo parlance. The GPU not in use is the
> - * inactive client.
> + * inactive client. When the inactive client's DRM driver is loaded,
> + * it will be unable to probe the panel's EDID and hence depends on
> + * VBIOS to provide its display modes. If the VBIOS modes are bogus or
> + * if there is no VBIOS at all (which is common on the MacBook Pro),
> + * a client may alternatively request that the DDC lines are temporarily
> + * switched to it, provided that the handler supports this. Switching
> + * only the DDC lines and not the entire output avoids unnecessary
> + * flickering. On machines which are able to mux external connectors,
> + * VBIOS cannot know of attached displays so switching the DDC lines
> + * is the only option to retrieve the displays' EDID.
>   */
>  
>  /**
> @@ -125,6 +135,8 @@ static DEFINE_MUTEX(vgasr_mutex);
>   * 	(counting only vga clients, not audio clients)
>   * @clients: list of registered clients
>   * @handler: registered handler
> + * @ddc_lock: protects access to DDC lines while they are temporarily switched
> + * @old_ddc_owner: client to which DDC lines will be switched back on unlock
>   *
>   * vga_switcheroo private data. Currently only one vga_switcheroo instance
>   * per system is supported.
> @@ -141,6 +153,8 @@ struct vgasr_priv {
>  	struct list_head clients;
>  
>  	struct vga_switcheroo_handler *handler;
> +	struct mutex ddc_lock;
> +	int old_ddc_owner;
>  };
>  
>  #define ID_BIT_AUDIO		0x100
> @@ -155,6 +169,7 @@ static void vga_switcheroo_debugfs_fini(struct vgasr_priv *priv);
>  /* only one switcheroo per system */
>  static struct vgasr_priv vgasr_priv = {
>  	.clients = LIST_HEAD_INIT(vgasr_priv.clients),
> +	.ddc_lock = __MUTEX_INITIALIZER(vgasr_priv.ddc_lock),
>  };
>  
>  static bool vga_switcheroo_ready(void)
> @@ -221,12 +236,14 @@ EXPORT_SYMBOL(vga_switcheroo_register_handler);
>  void vga_switcheroo_unregister_handler(void)
>  {
>  	mutex_lock(&vgasr_mutex);
> +	mutex_lock(&vgasr_priv.ddc_lock);
>  	vgasr_priv.handler = NULL;
>  	if (vgasr_priv.active) {
>  		pr_info("disabled\n");
>  		vga_switcheroo_debugfs_fini(&vgasr_priv);
>  		vgasr_priv.active = false;
>  	}
> +	mutex_unlock(&vgasr_priv.ddc_lock);
>  	mutex_unlock(&vgasr_mutex);
>  }
>  EXPORT_SYMBOL(vga_switcheroo_unregister_handler);
> @@ -412,6 +429,76 @@ void vga_switcheroo_client_fb_set(struct pci_dev *pdev,
>  EXPORT_SYMBOL(vga_switcheroo_client_fb_set);
>  
>  /**
> + * vga_switcheroo_lock_ddc() - temporarily switch DDC lines to a given client
> + * @pdev: client pci device
> + *
> + * Temporarily switch DDC lines to the client identified by @pdev
> + * (but leave the outputs otherwise switched to where they are).
> + * This allows the inactive client to probe EDID. The DDC lines must
> + * afterwards be switched back by calling vga_switcheroo_unlock_ddc(),
> + * even if this function returns an error.
> + *
> + * Return: Previous DDC owner on success or a negative int on error.
> + * Specifically, -ENODEV if no handler has registered or if the handler
> + * does not support switching the DDC lines. Also, a negative value
> + * returned by the handler is propagated back to the caller.
> + * The return value has merely an informational purpose for any caller
> + * which might be interested in it. It is acceptable to ignore the return
> + * value and simply rely on the result of the subsequent EDID probe,
> + * which will be NULL if DDC switching failed.
> + */
> +int vga_switcheroo_lock_ddc(struct pci_dev *pdev)
> +{
> +	enum vga_switcheroo_client_id id;
> +
> +	mutex_lock(&vgasr_priv.ddc_lock);
> +	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) {
> +		vgasr_priv.old_ddc_owner = -ENODEV;
> +		return -ENODEV;
> +	}
> +
> +	id = vgasr_priv.handler->get_client_id(pdev);
> +	vgasr_priv.old_ddc_owner = vgasr_priv.handler->switch_ddc(id);
> +	return vgasr_priv.old_ddc_owner;
> +}
> +EXPORT_SYMBOL(vga_switcheroo_lock_ddc);
> +
> +/**
> + * vga_switcheroo_unlock_ddc() - switch DDC lines back to previous owner
> + * @pdev: client pci device
> + *
> + * Switch DDC lines back to the previous owner after calling
> + * vga_switcheroo_lock_ddc(). This must be called even if
> + * vga_switcheroo_lock_ddc() returned an error.
> + *
> + * Return: Previous DDC owner on success (i.e. the client identifier of @pdev)
> + * or a negative int on error.
> + * Specifically, -ENODEV if no handler has registered or if the handler
> + * does not support switching the DDC lines. Also, a negative value
> + * returned by the handler is propagated back to the caller.
> + * Finally, invoking this function without calling vga_switcheroo_lock_ddc()
> + * first is not allowed and will result in -EINVAL.
> + */
> +int vga_switcheroo_unlock_ddc(struct pci_dev *pdev)
> +{
> +	enum vga_switcheroo_client_id id;
> +	int ret = vgasr_priv.old_ddc_owner;
> +
> +	if (WARN_ON_ONCE(!mutex_is_locked(&vgasr_priv.ddc_lock)))
> +		return -EINVAL;
> +
> +	if (vgasr_priv.old_ddc_owner >= 0) {
> +		id = vgasr_priv.handler->get_client_id(pdev);
> +		if (vgasr_priv.old_ddc_owner != id)
> +			ret = vgasr_priv.handler->switch_ddc(
> +						     vgasr_priv.old_ddc_owner);
> +	}
> +	mutex_unlock(&vgasr_priv.ddc_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL(vga_switcheroo_unlock_ddc);
> +
> +/**
>   * DOC: Manual switching and manual power control
>   *
>   * In this mode of use, the file /sys/kernel/debug/vgaswitcheroo/switch
> @@ -548,7 +635,9 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
>  		console_unlock();
>  	}
>  
> +	mutex_lock(&vgasr_priv.ddc_lock);
>  	ret = vgasr_priv.handler->switchto(new_client->id);
> +	mutex_unlock(&vgasr_priv.ddc_lock);
>  	if (ret)
>  		return ret;
>  
> @@ -663,7 +752,9 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf,
>  	vgasr_priv.delayed_switch_active = false;
>  
>  	if (just_mux) {
> +		mutex_lock(&vgasr_priv.ddc_lock);
>  		ret = vgasr_priv.handler->switchto(client_id);
> +		mutex_unlock(&vgasr_priv.ddc_lock);
>  		goto out;
>  	}
>  
> @@ -875,8 +966,11 @@ static int vga_switcheroo_runtime_suspend(struct device *dev)
>  	if (ret)
>  		return ret;
>  	mutex_lock(&vgasr_mutex);
> -	if (vgasr_priv.handler->switchto)
> +	if (vgasr_priv.handler->switchto) {
> +		mutex_lock(&vgasr_priv.ddc_lock);
>  		vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD);
> +		mutex_unlock(&vgasr_priv.ddc_lock);
> +	}
>  	vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_OFF);
>  	mutex_unlock(&vgasr_mutex);
>  	return 0;
> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index b2a3dda..6edaacc 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -82,6 +82,9 @@ enum vga_switcheroo_client_id {
>   * 	Mandatory. For muxless machines this should be a no-op. Returning 0
>   * 	denotes success, anything else failure (in which case the switch is
>   * 	aborted)
> + * @switch_ddc: switch DDC lines to given client.
> + * 	Optional. Should return the previous DDC owner on success or a
> + * 	negative int on failure
>   * @power_state: cut or reinstate power of given client.
>   * 	Optional. The return value is ignored
>   * @get_client_id: determine if given pci device is integrated or discrete GPU.
> @@ -93,6 +96,7 @@ enum vga_switcheroo_client_id {
>  struct vga_switcheroo_handler {
>  	int (*init)(void);
>  	int (*switchto)(enum vga_switcheroo_client_id id);
> +	int (*switch_ddc)(enum vga_switcheroo_client_id id);
>  	int (*power_state)(enum vga_switcheroo_client_id id,
>  			   enum vga_switcheroo_state state);
>  	enum vga_switcheroo_client_id (*get_client_id)(struct pci_dev *pdev);
> @@ -132,6 +136,9 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
>  void vga_switcheroo_client_fb_set(struct pci_dev *dev,
>  				  struct fb_info *info);
>  
> +int vga_switcheroo_lock_ddc(struct pci_dev *pdev);
> +int vga_switcheroo_unlock_ddc(struct pci_dev *pdev);
> +
>  int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler);
>  void vga_switcheroo_unregister_handler(void);
>  
> @@ -150,6 +157,8 @@ static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}
>  static inline int vga_switcheroo_register_client(struct pci_dev *dev,
>  		const struct vga_switcheroo_client_ops *ops, bool driver_power_control) { return 0; }
>  static inline void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct fb_info *info) {}
> +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_register_handler(struct vga_switcheroo_handler *handler) { return 0; }
>  static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
>  	const struct vga_switcheroo_client_ops *ops,
> -- 
> 1.8.5.2 (Apple Git-48)
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://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