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
