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 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@xxxxxxxxx> > [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina] > > Cc: Seth Forshee <seth.forshee@xxxxxxxxxxxxx> > Cc: Dave Airlie <airlied@xxxxxxxxx> > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx> > --- > 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 > -- 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
