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 > */ > > #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:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
