| Message ID | 7984d8eacbb0386ca52e3fdf2ad2554dc90ff1fe.1439739853.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Hi Thierry, Thanks a lot for your comments! On Mon, Aug 17, 2015 at 12:36:55PM +0200, Thierry Reding wrote: > On Fri, Aug 14, 2015 at 06:50:15PM +0200, Lukas Wunner wrote: > > +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? Ok, will rectify in v2.2. > > + 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? I carried this over from Seth Forshee's and Dave Airlie's patches, I believe the rationale is that the ->switch_ddc handler callback might return an error and that is customarily passed up to the caller. While drm_get_edid() does indeed ignore that return value (it will just try to probe DDC anyway), some future function that invokes vga_switcheroo_lock_ddc() might want to do something useful with it. So either way has its merits and tbh I don't feel in a position to judge what's right or wrong here. I'd be grateful if you or some other maintainer would decide whether to make the return value void or not and I'll be happy to change the patch accordingly. > > +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. Maybe I'm mistaken but I believe -ENODEV is correct. If there's no error then vga_switcheroo_lock_ddc/unlock_ddc() return the old_ddc_owner which is numbered from 0. E.g. on the MacBook Pro, 0 is IGD and 1 is DIS. So returning 0 would mean "okay, successfully switched, was previously switched to the integrated GPU". Best regards, Lukas
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 > */ Since you're just diggin around in switcheroo code it would be awesome to write proper kerneldoc for all the functions exposed to drivers, move the above to a proper kerneldoc for the vfunc tables (there's new support for adding kerneldoc split up per member instead of one big pile squashed at the top). And then bind it together with the overview sections with DOC comments in Documentation/DocBook/drm.tmpl. That would also help reviewing ;-) > > #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; New locks imo need comments about what exactly they protect. > }; > > #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) Since this can fail please call it something like try_lock_ddc to make that clear to callers. Or make it return void. > +{ > + int id; > + > + mutex_lock(&vgasr_priv.ddc_lock); > + > + if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) > + return vgasr_priv.old_ddc_owner = -ENODEV; > + > + id = vgasr_priv.handler->get_client_id(pdev); > + return vgasr_priv.old_ddc_owner = vgasr_priv.handler->switch_ddc(id); Looking at patch 3 you don't seem to handle this error. Also if this function fails it should _not_ grab the lock so that we can do something like ret = try_lock_ddc(); if (ret) { printk("arg!"); return ret; } drm_get_edid(); unlock_ddc(); } > +} > +EXPORT_SYMBOL(vga_switcheroo_lock_ddc); > + > +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); > + > static int vga_switcheroo_show(struct seq_file *m, void *v) > { > struct vga_switcheroo_client *client; > @@ -341,8 +384,6 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client) > if (!active) > return 0; > > - active->active = false; > - > set_audio_state(active->id, VGA_SWITCHEROO_OFF); > > if (new_client->fb_info) { > @@ -353,9 +394,21 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client) > console_unlock(); > } > > + if (vgasr_priv.handler->switch_ddc) { > + mutex_lock(&vgasr_priv.ddc_lock); > + vgasr_priv.handler->switch_ddc(new_client->id); > + mutex_unlock(&vgasr_priv.ddc_lock); > + } > ret = vgasr_priv.handler->switchto(new_client->id); > - if (ret) > + if (ret) { > + if (vgasr_priv.handler->switch_ddc) { > + mutex_lock(&vgasr_priv.ddc_lock); > + vgasr_priv.handler->switch_ddc(active->id); > + mutex_unlock(&vgasr_priv.ddc_lock); > + } Just for safety I'd grab the ddc_lock around the entire sequence. Overall there seems to be a lot less scary in these 3 patches than the previous series. Is this really all due to squashing or is this series incomplete? -Daniel > return ret; > + } > + active->active = false; > > if (new_client->ops->reprobe) > new_client->ops->reprobe(new_client->pdev); > @@ -468,6 +521,11 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf, > vgasr_priv.delayed_switch_active = false; > > if (just_mux) { > + if (vgasr_priv.handler->switch_ddc) { > + mutex_lock(&vgasr_priv.ddc_lock); > + vgasr_priv.handler->switch_ddc(client_id); > + mutex_unlock(&vgasr_priv.ddc_lock); > + } > ret = vgasr_priv.handler->switchto(client_id); > goto out; > } > @@ -623,6 +681,11 @@ static int vga_switcheroo_runtime_suspend(struct device *dev) > ret = dev->bus->pm->runtime_suspend(dev); > if (ret) > return ret; > + if (vgasr_priv.handler->switch_ddc) { > + mutex_lock(&vgasr_priv.ddc_lock); > + vgasr_priv.handler->switch_ddc(VGA_SWITCHEROO_IGD); > + mutex_unlock(&vgasr_priv.ddc_lock); > + } > if (vgasr_priv.handler->switchto) > vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD); > vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_OFF); > 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; } > 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, > -- > 2.1.0 >
Hi Daniel, On Tue, Aug 25, 2015 at 10:12:50AM +0200, Daniel Vetter wrote: > Since you're just diggin around in switcheroo code it would be awesome to > write proper kerneldoc for all the functions exposed to drivers, move the > above to a proper kerneldoc for the vfunc tables Alright, a patch set with vga_switcheroo docs is coming up right after this e-mail. I also fixed some bugs and cosmetic issues while I was at it. To ease reviewing I've pushed this to GitHub: https://github.com/l1k/linux/commits/vga_switcheroo_docs Patch #2 requires Markdown support so you may want to postpone that single patch for now. The patches are based on today's topic/drm-misc. > (there's new support for adding kerneldoc split up per member instead of > one big pile squashed at the top). To be honest the data structures aren't that large so I felt that the traditional "docs above, struct below" way is clearer and more concise in this case. > And then bind it together with the overview sections with DOC > comments in Documentation/DocBook/drm.tmpl. vga_switcheroo is a subsystem of its own which interfaces not just with DRM but also with multiplexer drivers, ALSA and power management, so I put it in a separate .tmpl file. Thanks, Lukas
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; + + id = vgasr_priv.handler->get_client_id(pdev); + return vgasr_priv.old_ddc_owner = vgasr_priv.handler->switch_ddc(id); +} +EXPORT_SYMBOL(vga_switcheroo_lock_ddc); + +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); + static int vga_switcheroo_show(struct seq_file *m, void *v) { struct vga_switcheroo_client *client; @@ -341,8 +384,6 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client) if (!active) return 0; - active->active = false; - set_audio_state(active->id, VGA_SWITCHEROO_OFF); if (new_client->fb_info) { @@ -353,9 +394,21 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client) console_unlock(); } + if (vgasr_priv.handler->switch_ddc) { + mutex_lock(&vgasr_priv.ddc_lock); + vgasr_priv.handler->switch_ddc(new_client->id); + mutex_unlock(&vgasr_priv.ddc_lock); + } ret = vgasr_priv.handler->switchto(new_client->id); - if (ret) + if (ret) { + if (vgasr_priv.handler->switch_ddc) { + mutex_lock(&vgasr_priv.ddc_lock); + vgasr_priv.handler->switch_ddc(active->id); + mutex_unlock(&vgasr_priv.ddc_lock); + } return ret; + } + active->active = false; if (new_client->ops->reprobe) new_client->ops->reprobe(new_client->pdev); @@ -468,6 +521,11 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf, vgasr_priv.delayed_switch_active = false; if (just_mux) { + if (vgasr_priv.handler->switch_ddc) { + mutex_lock(&vgasr_priv.ddc_lock); + vgasr_priv.handler->switch_ddc(client_id); + mutex_unlock(&vgasr_priv.ddc_lock); + } ret = vgasr_priv.handler->switchto(client_id); goto out; } @@ -623,6 +681,11 @@ static int vga_switcheroo_runtime_suspend(struct device *dev) ret = dev->bus->pm->runtime_suspend(dev); if (ret) return ret; + if (vgasr_priv.handler->switch_ddc) { + mutex_lock(&vgasr_priv.ddc_lock); + vgasr_priv.handler->switch_ddc(VGA_SWITCHEROO_IGD); + mutex_unlock(&vgasr_priv.ddc_lock); + } if (vgasr_priv.handler->switchto) vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD); vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_OFF); 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; } 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,