| Message ID | a46bc3667badb466475ed9ab6eac21c2b4814741.1443952353.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 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@ffwll.ch>) > > v2.2: Readability improvements > (requested by Thierry Reding <thierry.reding@gmail.com>) > > v2.3: Overhaul locking once more > > v2.4: Retain semantics of ->switchto handler callback to switch all pins > (requested by Daniel Vetter <daniel.vetter@ffwll.ch>) > > 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@free.fr> > [MBP 5,3 2009 nvidia MCP79 + G96 pre-retina 15"] > Tested-by: William Brown <william@blackhats.net.au> > [MBP 8,2 2011 intel SNB + amd turks pre-retina 15"] > Tested-by: Lukas Wunner <lukas@wunner.de> > [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"] > > 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 | 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel, thank you for taking a look at the patch set and shepherding this through the review process. On Tue, Oct 06, 2015 at 09:27:24AM +0200, Daniel Vetter wrote: > On Fri, Aug 14, 2015 at 06:50:15PM +0200, Lukas Wunner wrote: > > 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 ... With the chosen approach that's not necessary: vga_switcheroo_lock_ddc() records in vgasr_priv.old_ddc_owner if there's no mux: if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) { vgasr_priv.old_ddc_owner = -ENODEV; return -ENODEV; } And vga_switcheroo_unlock_ddc() does nothing if vgasr_priv.old_ddc_owner is negative, it just releases the lock and returns: 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); So it has no consequences if the mux registers between the calls to _lock_ddc() and _unlock_ddc(). > 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. Hm, hw_lock sounds a bit ambiguous. vgasr_mutex is really a catch-all that protects access to the vgasr_priv structure and also protects various hardware state. (Power state of each GPU, mux state.) Up until now this approach worked fine, it looks like there was no need for additional locks. We may need to move to more fine-grained locking as new features get added to vga_switcheroo. The newly introduced ddc_lock is a first step and I think is aptly named since it only protects the mux state for the DDC lines. > Wrt the overall patch series, can you pls haggle driver-maintainers (gmux, > radeon, nouveau) for acks/reviews? scripts/get_maintainers.pl should help. Will do. > Also, the revised docbook patch seems to be missing from this iteration, > please follow up with that one. The docbook patch posted September 17 automatically picks up the kernel-doc for the new functions through the !E directive. However I used markdown syntax for the unsorted lists in the DOC sections, so it will look a bit funny unless markdown gets merged, which as we all know is contentious. (Which is sad, though I can understand Jonathan Corbet's concerns.) By the way, Jani has kindly provided a Reviewed-By: for patch 6 of the vga_switcheroo docs series. The patch is not dependent on the preceding patch 5 which is awaiting an ack from Takashi, so could be merged now: [PATCH 06/15] vga_switcheroo: Use enum vga_switcheroo_state instead Patches 7 and 8 are similarly trivial cleanups: [PATCH 07/15] vga_switcheroo: Use VGA_SWITCHEROO_UNKNOWN_ID instead [PATCH 08/15] vga_switcheroo: Use enum vga_switcheroo_client_id And patch 10 has gotten a Reviewed-By: from Takashi: [PATCH 10/15] ALSA: hda - Spell vga_switcheroo consistently Kind regards, Lukas > > 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@ffwll.ch>) > > > > v2.2: Readability improvements > > (requested by Thierry Reding <thierry.reding@gmail.com>) > > > > v2.3: Overhaul locking once more > > > > v2.4: Retain semantics of ->switchto handler callback to switch all pins > > (requested by Daniel Vetter <daniel.vetter@ffwll.ch>) > > > > 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@free.fr> > > [MBP 5,3 2009 nvidia MCP79 + G96 pre-retina 15"] > > Tested-by: William Brown <william@blackhats.net.au> > > [MBP 8,2 2011 intel SNB + amd turks pre-retina 15"] > > Tested-by: Lukas Wunner <lukas@wunner.de> > > [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"] > > > > 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 | 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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Tue, Oct 06, 2015 at 12:10:48PM +0200, Lukas Wunner wrote: > Hi Daniel, > > thank you for taking a look at the patch set and shepherding this > through the review process. > > On Tue, Oct 06, 2015 at 09:27:24AM +0200, Daniel Vetter wrote: > > On Fri, Aug 14, 2015 at 06:50:15PM +0200, Lukas Wunner wrote: > > > 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 ... > > With the chosen approach that's not necessary: vga_switcheroo_lock_ddc() > records in vgasr_priv.old_ddc_owner if there's no mux: > > if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) { > vgasr_priv.old_ddc_owner = -ENODEV; > return -ENODEV; > } > > And vga_switcheroo_unlock_ddc() does nothing if vgasr_priv.old_ddc_owner > is negative, it just releases the lock and returns: > > 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); > > So it has no consequences if the mux registers between the calls to > _lock_ddc() and _unlock_ddc(). > > > > 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. > > Hm, hw_lock sounds a bit ambiguous. > > vgasr_mutex is really a catch-all that protects access to the vgasr_priv > structure and also protects various hardware state. (Power state of each > GPU, mux state.) Up until now this approach worked fine, it looks like > there was no need for additional locks. We may need to move to more > fine-grained locking as new features get added to vga_switcheroo. > The newly introduced ddc_lock is a first step and I think is aptly > named since it only protects the mux state for the DDC lines. Oops sorry mixed up the names. I meant rename the ddc_lock to hw_lock since it protects a bit more than just the ddc (it protects the entire hw switch state since we grab it both for dcc switching and for full-blown switching of everything). > > > > Wrt the overall patch series, can you pls haggle driver-maintainers (gmux, > > radeon, nouveau) for acks/reviews? scripts/get_maintainers.pl should help. > > Will do. > > > > Also, the revised docbook patch seems to be missing from this iteration, > > please follow up with that one. > > The docbook patch posted September 17 automatically picks up the > kernel-doc for the new functions through the !E directive. I asked for the inclusion to be moved to drm.tmpl instead of creating a new docbook. > However I used markdown syntax for the unsorted lists in the DOC > sections, so it will look a bit funny unless markdown gets merged, > which as we all know is contentious. (Which is sad, though I can > understand Jonathan Corbet's concerns.) > > > By the way, Jani has kindly provided a Reviewed-By: for patch 6 of > the vga_switcheroo docs series. The patch is not dependent on the > preceding patch 5 which is awaiting an ack from Takashi, so could > be merged now: > [PATCH 06/15] vga_switcheroo: Use enum vga_switcheroo_state instead > > Patches 7 and 8 are similarly trivial cleanups: > [PATCH 07/15] vga_switcheroo: Use VGA_SWITCHEROO_UNKNOWN_ID instead > [PATCH 08/15] vga_switcheroo: Use enum vga_switcheroo_client_id > > And patch 10 has gotten a Reviewed-By: from Takashi: > [PATCH 10/15] ALSA: hda - Spell vga_switcheroo consistently Hm those patches aren't in this series, that makes it really hard for me to know what's still pending. I think it would be best to resend _everything_, so including docs and cleanups and all that. Otherwise I think this will take a lot longer than necessary to pull in. Also when resending patches unchanged please directly add r-b tags you've collected already - replying top-level with them all again makes it a bit harder to sort everything our here for me. And one small nit: The usual style for patch bombing is flat threading, not nested. It should be the default for new-ish git, but if that's not the case please use git send-email --no-chain-reply-to. Note also if you generate patches first with git format-patch that has a separate knob for deep threading, you need to disable both iirc to avoid deep threading, there it's git format-patch --thread=shallow. -Daniel
Hi Daniel, On Tue, Oct 06, 2015 at 01:10:00PM +0200, Daniel Vetter wrote: > On Tue, Oct 06, 2015 at 12:10:48PM +0200, Lukas Wunner wrote: > > On Tue, Oct 06, 2015 at 09:27:24AM +0200, Daniel Vetter wrote: > > > 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. > > > > Hm, hw_lock sounds a bit ambiguous. > > > > vgasr_mutex is really a catch-all that protects access to the vgasr_priv > > structure and also protects various hardware state. (Power state of each > > GPU, mux state.) Up until now this approach worked fine, it looks like > > there was no need for additional locks. We may need to move to more > > fine-grained locking as new features get added to vga_switcheroo. > > The newly introduced ddc_lock is a first step and I think is aptly > > named since it only protects the mux state for the DDC lines. > > Oops sorry mixed up the names. I meant rename the ddc_lock to hw_lock > since it protects a bit more than just the ddc (it protects the entire hw > switch state since we grab it both for dcc switching and for full-blown > switching of everything). Interesting observation. Actually the intention is to use ddc_lock to only lock hardware state of the DDC switch, but you're right, it also locks hardware state of the panel switch. That's an unintended consequence of the ->switchto callback in apple-gmux also switching DDC, and the need to avoid races because of this. Now that you mention it I'm contemplating removing DDC switching from the ->switchto callback in apple-gmux. Then I don't need to take the ddc_lock when switching the full panel. > > > Also, the revised docbook patch seems to be missing from this iteration, > > > please follow up with that one. > > > > The docbook patch posted September 17 automatically picks up the > > kernel-doc for the new functions through the !E directive. > > I asked for the inclusion to be moved to drm.tmpl instead of creating a > new docbook. Your argument was that including it in drm.tmpl will increase the chances it's read. Quite honestly I think that reasoning is a bit thin. One might as well argue that people won't find the information in the juggernaut of 180 kByte XML text that is drm.tmpl. The (honest) question is this, vga_switcheroo is not located in the DRM tree, so it's a separate subsystem. Then why should someone be looking for its documentation in the DRM DocBook? > > By the way, Jani has kindly provided a Reviewed-By: for patch 6 of > > the vga_switcheroo docs series. The patch is not dependent on the > > preceding patch 5 which is awaiting an ack from Takashi, so could > > be merged now: > > [PATCH 06/15] vga_switcheroo: Use enum vga_switcheroo_state instead > > > > Patches 7 and 8 are similarly trivial cleanups: > > [PATCH 07/15] vga_switcheroo: Use VGA_SWITCHEROO_UNKNOWN_ID instead > > [PATCH 08/15] vga_switcheroo: Use enum vga_switcheroo_client_id > > > > And patch 10 has gotten a Reviewed-By: from Takashi: > > [PATCH 10/15] ALSA: hda - Spell vga_switcheroo consistently > > Hm those patches aren't in this series, that makes it really hard for me > to know what's still pending. I think it would be best to resend > _everything_, so including docs and cleanups and all that. Otherwise I > think this will take a lot longer than necessary to pull in. I updated my vga_switcheroo_docs branch on GitHub as the Reviewed-Bys came in and just rebased it to current topic/drm-misc, so you can pull from there if you're comfortable with that: https://github.com/l1k/linux/commits/vga_switcheroo_docs If on the other hand you prefer merging stuff via the mailing list, I'll be happy to resend. Thanks, Lukas
On Tue, Oct 06, 2015 at 08:27:44PM +0200, Lukas Wunner wrote: > Hi Daniel, > > On Tue, Oct 06, 2015 at 01:10:00PM +0200, Daniel Vetter wrote: > > On Tue, Oct 06, 2015 at 12:10:48PM +0200, Lukas Wunner wrote: > > > On Tue, Oct 06, 2015 at 09:27:24AM +0200, Daniel Vetter wrote: > > > > 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. > > > > > > Hm, hw_lock sounds a bit ambiguous. > > > > > > vgasr_mutex is really a catch-all that protects access to the vgasr_priv > > > structure and also protects various hardware state. (Power state of each > > > GPU, mux state.) Up until now this approach worked fine, it looks like > > > there was no need for additional locks. We may need to move to more > > > fine-grained locking as new features get added to vga_switcheroo. > > > The newly introduced ddc_lock is a first step and I think is aptly > > > named since it only protects the mux state for the DDC lines. > > > > Oops sorry mixed up the names. I meant rename the ddc_lock to hw_lock > > since it protects a bit more than just the ddc (it protects the entire hw > > switch state since we grab it both for dcc switching and for full-blown > > switching of everything). > > Interesting observation. Actually the intention is to use ddc_lock to > only lock hardware state of the DDC switch, but you're right, it also > locks hardware state of the panel switch. That's an unintended > consequence of the ->switchto callback in apple-gmux also switching > DDC, and the need to avoid races because of this. > > Now that you mention it I'm contemplating removing DDC switching from the > ->switchto callback in apple-gmux. Then I don't need to take the ddc_lock > when switching the full panel. I think switching everything together makes some sense, but either way should be find. > > > > Also, the revised docbook patch seems to be missing from this iteration, > > > > please follow up with that one. > > > > > > The docbook patch posted September 17 automatically picks up the > > > kernel-doc for the new functions through the !E directive. > > > > I asked for the inclusion to be moved to drm.tmpl instead of creating a > > new docbook. > > Your argument was that including it in drm.tmpl will increase the chances > it's read. Quite honestly I think that reasoning is a bit thin. One might > as well argue that people won't find the information in the juggernaut of > 180 kByte XML text that is drm.tmpl. > > The (honest) question is this, vga_switcheroo is not located in the DRM > tree, so it's a separate subsystem. Then why should someone be looking > for its documentation in the DRM DocBook? DRM has essentially become the gfx subsystem of the kernel, the name is purely historical accident. And DRM maintainers generally take care of everything under drivers/gpu/* so I think it makes sense to include it there. If you think the DRM in the docbook is confusing then we can fix that easily by renaming it to gpu.tmpl and GPU Developer's Guide. Actually that seems like a decent idea, so let me write that patch. > > > By the way, Jani has kindly provided a Reviewed-By: for patch 6 of > > > the vga_switcheroo docs series. The patch is not dependent on the > > > preceding patch 5 which is awaiting an ack from Takashi, so could > > > be merged now: > > > [PATCH 06/15] vga_switcheroo: Use enum vga_switcheroo_state instead > > > > > > Patches 7 and 8 are similarly trivial cleanups: > > > [PATCH 07/15] vga_switcheroo: Use VGA_SWITCHEROO_UNKNOWN_ID instead > > > [PATCH 08/15] vga_switcheroo: Use enum vga_switcheroo_client_id > > > > > > And patch 10 has gotten a Reviewed-By: from Takashi: > > > [PATCH 10/15] ALSA: hda - Spell vga_switcheroo consistently > > > > Hm those patches aren't in this series, that makes it really hard for me > > to know what's still pending. I think it would be best to resend > > _everything_, so including docs and cleanups and all that. Otherwise I > > think this will take a lot longer than necessary to pull in. > > I updated my vga_switcheroo_docs branch on GitHub as the Reviewed-Bys > came in and just rebased it to current topic/drm-misc, so you can pull > from there if you're comfortable with that: > https://github.com/l1k/linux/commits/vga_switcheroo_docs > > If on the other hand you prefer merging stuff via the mailing list, > I'll be happy to resend. Mailing list is highly preferred. Thanks, Daniel
Hi Daniel, as requested here's a resend of the 4 pending vga_switcheroo cleanup patches (originally posted Sep 17), 2 of them now with Reviewed-by tags. On top, a new one for i915 and 4 new ones for Alex. Also available on GitHub: https://github.com/l1k/linux/commits/vga_switcheroo_cleanup Thank you & best regards, Lukas Lukas Wunner (9): vga_switcheroo: Use enum vga_switcheroo_state instead of int vga_switcheroo: Use VGA_SWITCHEROO_UNKNOWN_ID instead of -1 vga_switcheroo: Use enum vga_switcheroo_client_id instead of int ALSA: hda - Spell vga_switcheroo consistently drm/i915: Drop unnecessary #include <linux/vga_switcheroo.h> drm/radeon: Drop unnecessary #include <linux/vga_switcheroo.h> drm/amdgpu: Drop unnecessary #include <linux/vga_switcheroo.h> drm/radeon: Fix kernel-doc copy/paste snafu drm/amdgpu: Fix kernel-doc copy/paste snafu drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 1 - drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 1 - drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 4 ++-- drivers/gpu/drm/i915/intel_acpi.c | 1 - drivers/gpu/drm/radeon/radeon_acpi.c | 1 - drivers/gpu/drm/radeon/radeon_asic.c | 1 - drivers/gpu/drm/radeon/radeon_bios.c | 1 - drivers/gpu/drm/radeon/radeon_kms.c | 4 ++-- drivers/gpu/vga/vga_switcheroo.c | 36 ++++++++++++++++++-------------- include/linux/vga_switcheroo.h | 14 ++++++++----- sound/pci/hda/hda_controller.h | 2 +- sound/pci/hda/hda_intel.c | 12 +++++------ sound/pci/hda/hda_intel.h | 2 +- 13 files changed, 41 insertions(+), 39 deletions(-)
Changes since v3:
* Previously when switching the display, the DDC lines were switched
as well. But actually this is not necessary: If a GPU probes EDID,
DDC must be switched and locked to the GPU anyway, so why switch
DDC preemptively? Also, previously the DDC lines were switched back
to the previous owner on unlock. This is unnecessary for the same
reason. So I did away with all that and it simplifies the code
considerably. This works fine as long as anything accessing DDC
calls vga_switcheroo_lock_ddc() first. If something in the kernel
or a VM or in user space accesses DDC and omits locking it first,
things may go awry. (However, if locking is omitted, things could
go awry with the previous behaviour as well.) I'm curious if this
new behaviour provokes objections.
* I now have conclusive evidence that only the pre-retina MacBook Pro
can switch DDC. The mux in the retina is incapable of that. So the
->switch_ddc callback is no longer advertised to vga_switcheroo
on retinas.
Also available on GitHub for easier reviewing:
https://github.com/l1k/linux/commits/mbp_switcheroo_v4
For those who haven't been following this series so far:
The pre-retina MacBook Pro uses an LVDS panel and a gmux controller
to switch the panel between its two GPUs. The panel mode in VBIOS
is notoriously bogus on these machines and some models have no
VBIOS at all, so the inactive GPU cannot set up its LVDS output.
Extend vga_switcheroo to support switching only the DDC lines.
Introduce a drm_get_edid_switcheroo() helper which uses this feature.
Amend i915, nouveau and radeon to call it for LVDS connectors.
This only enables EDID probing on the pre-retina MBP (2008 - 2013),
and only under the condition that apple-gmux loads before the DRM
drivers. Later patches will address reprobing of the DRM drivers
if apple-gmux loads late.
The retina MBP (2012 - present) uses eDP and is apparently not
capable of switching AUX separately from the main link.
This will also be addressed in later patches.
Previous installments:
v1: http://lists.freedesktop.org/archives/dri-devel/2015-April/081515.html
v2: http://lists.freedesktop.org/archives/dri-devel/2015-August/088156.html
v3: http://lists.freedesktop.org/archives/dri-devel/2015-October/091741.html
Lukas Wunner (6):
vga_switcheroo: Add support for switching only the DDC
apple-gmux: Add switch_ddc support
drm/edid: Switch DDC when reading the EDID
drm/i915: Switch DDC when reading the EDID
drm/nouveau: Switch DDC when reading the EDID
drm/radeon: Switch DDC when reading the EDID
drivers/gpu/drm/drm_edid.c | 26 ++++++++++++
drivers/gpu/drm/i915/intel_lvds.c | 3 +-
drivers/gpu/drm/nouveau/nouveau_connector.c | 13 +++++-
drivers/gpu/drm/radeon/radeon_connectors.c | 4 ++
drivers/gpu/vga/vga_switcheroo.c | 66 ++++++++++++++++++++++++++++-
drivers/platform/x86/apple-gmux.c | 21 ++++++++-
include/drm/drm_crtc.h | 2 +
include/linux/vga_switcheroo.h | 15 +++++--
8 files changed, 141 insertions(+), 9 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,
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 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. 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@ffwll.ch>) v2.2: Readability improvements (requested by Thierry Reding <thierry.reding@gmail.com>) v2.3: Overhaul locking once more v2.4: Retain semantics of ->switchto handler callback to switch all pins (requested by Daniel Vetter <daniel.vetter@ffwll.ch>) 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@free.fr> [MBP 5,3 2009 nvidia MCP79 + G96 pre-retina 15"] Tested-by: William Brown <william@blackhats.net.au> [MBP 8,2 2011 intel SNB + amd turks pre-retina 15"] Tested-by: Lukas Wunner <lukas@wunner.de> [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"] 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 | 98 +++++++++++++++++++++++++++++++++++++++- include/linux/vga_switcheroo.h | 9 ++++ 2 files changed, 105 insertions(+), 2 deletions(-)