[PATCH v3 1/6] vga_switcheroo: Add support for switching only the DDC
Lukas Wunner
lukas at wunner.de
Tue Oct 6 03:10:48 PDT 2015
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 at ffwll.ch>)
> >
> > v2.2: Readability improvements
> > (requested by Thierry Reding <thierry.reding at 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 at 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 at free.fr>
> > [MBP 5,3 2009 nvidia MCP79 + G96 pre-retina 15"]
> > Tested-by: William Brown <william at blackhats.net.au>
> > [MBP 8,2 2011 intel SNB + amd turks pre-retina 15"]
> > Tested-by: Lukas Wunner <lukas at wunner.de>
> > [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"]
> >
> > Cc: Seth Forshee <seth.forshee at canonical.com>
> > Cc: Dave Airlie <airlied at gmail.com>
> > Signed-off-by: Lukas Wunner <lukas at 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
More information about the dri-devel
mailing list