Re: [PATCH v2.1 1/3] vga_switcheroo: Add support for switching only the DDC

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7643 bytes --]

On Fri, Aug 14, 2015 at 06:50:15PM +0200, Lukas Wunner wrote:
> Originally by Seth Forshee <seth.forshee@canonical.com>, 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@gmail.com>, 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@wunner.de>, 2015-08-14:
> Don't grab vgasr_mutex in lock_ddc/unlock_ddc to avoid the following
> deadlock scenarios: (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.
> If we don't grab vgasr_mutex the only bad thing that can happen is
> that the handler suddenly disappears. So block handler deregistration
> until DDC lines are unlocked again.
> 
> WARN_ON_ONCE if unlock_ddc is called without calling lock_ddc first.
> Lock ddc_lock in stage2 to avoid race condition where reading the
> EDID and switching happens simultaneously. Switch DDC lines on
> MIGD / MDIS commands and on runtime suspend. Fix bug in stage2
> where no client had the active attribute set if switching failed.
> Fix erroneous interface documentation.
> 
> If the inactive client registers before the active client then
> old_ddc_owner cannot be determined with find_active_client()
> (null pointer dereference). Therefore change semantics of the
> ->switch_ddc handler callback to return old_ddc_owner.
> 
> v2.1: Overhaul locking, squash commits (requested by Daniel Vetter)
> 
> 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]
> 
> Cc: Seth Forshee <seth.forshee@canonical.com>
> Cc: Dave Airlie <airlied@gmail.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/gpu/vga/vga_switcheroo.c | 73 +++++++++++++++++++++++++++++++++++++---
>  include/linux/vga_switcheroo.h   |  6 ++++
>  2 files changed, 74 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 37ac7b5..ac4ac12 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -9,12 +9,13 @@
>  
>   Switcher interface - methods require for ATPX and DCM
>   - switchto - this throws the output MUX switch
> - - discrete_set_power - sets the power state for the discrete card
> + - switch_ddc - switch only DDC lines, return old DDC owner
> + - power_state - sets the power state for either GPU
>  
>   GPU driver interface
>   - set_gpu_state - this should do the equiv of s/r for the card
>  		  - this should *not* set the discrete power state
> - - switch_check  - check if the device is in a position to switch now
> + - can_switch - check if the device is in a position to switch now
>   */
>  
>  #include <linux/module.h>
> @@ -57,6 +58,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
> @@ -70,6 +73,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)
> @@ -122,12 +126,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("vga_switcheroo: 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);
> @@ -256,6 +262,43 @@ void vga_switcheroo_client_fb_set(struct pci_dev *pdev,
>  }
>  EXPORT_SYMBOL(vga_switcheroo_client_fb_set);
>  
> +int vga_switcheroo_lock_ddc(struct pci_dev *pdev)
> +{
> +	int id;
> +
> +	mutex_lock(&vgasr_priv.ddc_lock);
> +
> +	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc)
> +		return vgasr_priv.old_ddc_owner = -ENODEV;

I find this very hard to read. Can this be split across two lines?

> +
> +	id = vgasr_priv.handler->get_client_id(pdev);
> +	return vgasr_priv.old_ddc_owner = vgasr_priv.handler->switch_ddc(id);

This too. I also notice that the only place you call this from doesn't
care about the return value, so why even bother returning one?

> +int vga_switcheroo_unlock_ddc(struct pci_dev *pdev)
> +{
> +	int ret, id;
> +
> +	if (WARN_ON_ONCE(!mutex_is_locked(&vgasr_priv.ddc_lock)))
> +		return -EINVAL;
> +
> +	if (vgasr_priv.old_ddc_owner < 0) {
> +		mutex_unlock(&vgasr_priv.ddc_lock);
> +		return -ENODEV;
> +	}
> +
> +	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);
> +	else
> +		ret = vgasr_priv.old_ddc_owner;
> +	mutex_unlock(&vgasr_priv.ddc_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(vga_switcheroo_unlock_ddc);

Same comment about the return value here.

> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index b483abd..62f303f 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -29,6 +29,7 @@ enum vga_switcheroo_client_id {
>  };
>  
>  struct vga_switcheroo_handler {
> +	int (*switch_ddc)(enum vga_switcheroo_client_id id);
>  	int (*switchto)(enum vga_switcheroo_client_id id);
>  	int (*power_state)(enum vga_switcheroo_client_id id,
>  			   enum vga_switcheroo_state state);
> @@ -54,6 +55,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);
>  
> @@ -72,6 +76,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; }

If you care about the return value you'll want to return 0 here to make
sure kernels without VGA switcheroo support will continue to work
properly.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel