From patchwork Fri Mar 27 11:29:40 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lukas Wunner X-Patchwork-Id: 7000721 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 74ABA9F373 for ; Wed, 12 Aug 2015 11:40:03 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 24E12206D4 for ; Wed, 12 Aug 2015 11:40:02 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id E7C9B20678 for ; Wed, 12 Aug 2015 11:40:00 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id DCC456EB38; Wed, 12 Aug 2015 04:39:53 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mailout2.hostsharing.net (mailout2.hostsharing.net [83.223.90.233]) by gabe.freedesktop.org (Postfix) with ESMTPS id 03F886EB2D for ; Wed, 12 Aug 2015 04:39:52 -0700 (PDT) Received: from h08.hostsharing.net (h08.hostsharing.net [83.223.95.28]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mailout2.hostsharing.net (Postfix) with ESMTPS id 38A271008C470; Wed, 12 Aug 2015 13:29:54 +0200 (CEST) Received: from localhost (6-38-90-81.adsl.cmo.de [81.90.38.6]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by h08.hostsharing.net (Postfix) with ESMTPSA id 278FE60423EA; Wed, 12 Aug 2015 13:29:41 +0200 (CEST) X-Mailbox-Line: From ec399430bb382373e8b4edaa5f773563ebc6aaa9 Mon Sep 17 00:00:00 2001 Message-Id: In-Reply-To: References: <29bed586baf62f6be77b7ab0ba1b8f5cb3be3aad.1439288957.git.lukas@wunner.de> <164b43588e80baaddb7a4d1081785c4d03a89c4b.1439288957.git.lukas@wunner.de> <27944adb13aa1ab246ee4a1ebb833e397324d073.1439288957.git.lukas@wunner.de> <2ac3eca0759cedd1009221cbef908605f8d29e1e.1439288957.git.lukas@wunner.de> From: Lukas Wunner Date: Fri, 27 Mar 2015 12:29:40 +0100 Subject: [PATCH v2 06/22] vga_switcheroo: Lock/unlock DDC lines harder To: dri-devel@lists.freedesktop.org Cc: Andreas Heider , Paul Hordiienko , William Brown , Bruno Bierbaumer , Matthew Garrett , Dave Airlie X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-0.8 required=5.0 tests=BAYES_00, DATE_IN_PAST_96_XX, RCVD_IN_DNSWL_MED,RP_MATCHES_RCVD,UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Unlock DDC lines if drm_probe_ddc() fails. 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 on success or a negative int on failure. Use mutex_trylock(&vgasr_mutex) in vga_switcheroo_unlock_ddc() to avoid locking inversion when one client locks vgasr_mutex on entering vga_switcheroo_lock_ddc() and tries to acquire ddc_lock while another client is holding ddc_lock and tries to acquire vgasr_mutex on entering vga_switcheroo_unlock_ddc(). 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. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115 Tested-by: Paul Hordiienko [MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina] Tested-by: William Brown [MBP 8,2 2011 intel SNB + amd turks pre-retina] Tested-by: Lukas Wunner [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina] Tested-by: Bruno Bierbaumer [MBP 11,3 2013 intel HSW + nvidia GK107 retina -- work in progress] Signed-off-by: Lukas Wunner --- drivers/gpu/drm/drm_edid.c | 9 ++-- drivers/gpu/vga/vga_switcheroo.c | 110 ++++++++++++++++++++++---------------- drivers/platform/x86/apple-gmux.c | 15 +++++- include/linux/vga_switcheroo.h | 3 +- 4 files changed, 87 insertions(+), 50 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 505eed1..cdb2fa1 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1378,17 +1378,20 @@ struct edid *drm_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { struct edid *edid; + struct pci_dev *pdev = connector->dev->pdev; - vga_switcheroo_lock_ddc(connector->dev->pdev); + vga_switcheroo_lock_ddc(pdev); - if (!drm_probe_ddc(adapter)) + if (!drm_probe_ddc(adapter)) { + vga_switcheroo_unlock_ddc(pdev); return NULL; + } edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); if (edid) drm_get_displayid(connector, edid); - vga_switcheroo_unlock_ddc(connector->dev->pdev); + vga_switcheroo_unlock_ddc(pdev); return edid; } diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c index 0223020..f02e7fc 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 (or < 0 on failure) + - 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 @@ -59,7 +60,7 @@ struct vgasr_priv { struct vga_switcheroo_handler *handler; struct mutex ddc_lock; - struct pci_dev *old_ddc_owner; + enum vga_switcheroo_client_id old_ddc_owner; }; #define ID_BIT_AUDIO 0x100 @@ -276,33 +277,24 @@ EXPORT_SYMBOL(vga_switcheroo_client_fb_set); int vga_switcheroo_lock_ddc(struct pci_dev *pdev) { - struct vga_switcheroo_client *client; - int ret = 0; - int id; + int ret, id; mutex_lock(&vgasr_mutex); - if (!vgasr_priv.handler) { + if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) { ret = -ENODEV; goto out; } - if (vgasr_priv.handler->switch_ddc) { - mutex_lock(&vgasr_priv.ddc_lock); + mutex_lock(&vgasr_priv.ddc_lock); + id = vgasr_priv.handler->get_client_id(pdev); + ret = vgasr_priv.handler->switch_ddc(id); - client = find_active_client(&vgasr_priv.clients); - if (!client) { - mutex_unlock(&vgasr_priv.ddc_lock); - ret = -ENODEV; - goto out; - } - vgasr_priv.old_ddc_owner = client->pdev; - if (client->pdev == pdev) - goto out; - - id = vgasr_priv.handler->get_client_id(pdev); - ret = vgasr_priv.handler->switch_ddc(id); - } + if (ret < 0) { + pr_err("vga_switcheroo: failed to switch DDC lines\n"); + mutex_unlock(&vgasr_priv.ddc_lock); + } else + vgasr_priv.old_ddc_owner = ret; out: mutex_unlock(&vgasr_mutex); @@ -312,25 +304,33 @@ EXPORT_SYMBOL(vga_switcheroo_lock_ddc); int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) { - int ret = 0; - int id; - mutex_lock(&vgasr_mutex); + int ret, id; + bool vgasr_mutex_acquired = mutex_trylock(&vgasr_mutex); - if (!vgasr_priv.handler) { - ret = -ENODEV; + if (!mutex_is_locked(&vgasr_priv.ddc_lock)) { + ret = -EINVAL; goto out; } - if (vgasr_priv.handler->switch_ddc) { - if (vgasr_priv.old_ddc_owner != pdev) { - id = vgasr_priv.handler->get_client_id(vgasr_priv.old_ddc_owner); - ret = vgasr_priv.handler->switch_ddc(id); - } - vgasr_priv.old_ddc_owner = NULL; + if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) { + pr_err("vga_switcheroo: handler disappeared\n"); mutex_unlock(&vgasr_priv.ddc_lock); + ret = -ENODEV; + goto out; } + + 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); + if (ret < 0) + pr_err("vga_switcheroo: failed to switch DDC lines\n"); + } else + ret = vgasr_priv.old_ddc_owner; + mutex_unlock(&vgasr_priv.ddc_lock); + out: - mutex_unlock(&vgasr_mutex); + if (vgasr_mutex_acquired) + mutex_unlock(&vgasr_mutex); return ret; } EXPORT_SYMBOL(vga_switcheroo_unlock_ddc); @@ -433,14 +433,26 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client) } if (vgasr_priv.handler->switch_ddc) { + mutex_lock(&vgasr_priv.ddc_lock); ret = vgasr_priv.handler->switch_ddc(new_client->id); - if (ret) + mutex_unlock(&vgasr_priv.ddc_lock); + if (ret < 0) { + pr_err("vga_switcheroo: failed to switch DDC lines\n"); return ret; + } } ret = vgasr_priv.handler->switchto(new_client->id); - if (ret) - goto restore_ddc; + if (ret) { + pr_err("vga_switcheroo: failed to switch to client %d\n", + new_client->id); + active->active = true; + /* restore DDC lines */ + mutex_lock(&vgasr_priv.ddc_lock); + vgasr_priv.handler->switch_ddc(active->id); + mutex_unlock(&vgasr_priv.ddc_lock); + return ret; + } if (new_client->ops->reprobe) new_client->ops->reprobe(new_client->pdev); @@ -452,14 +464,6 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client) new_client->active = true; return 0; - -restore_ddc: - if (vgasr_priv.handler->switch_ddc) { - int id = (new_client->id == VGA_SWITCHEROO_IGD) ? - VGA_SWITCHEROO_DIS : VGA_SWITCHEROO_IGD; - ret = vgasr_priv.handler->switch_ddc(id); - } - return ret; } static bool check_can_switch(void) @@ -561,6 +565,15 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf, vgasr_priv.delayed_switch_active = false; if (just_mux) { + if (vgasr_priv.handler->switch_ddc) { + mutex_lock(&vgasr_priv.ddc_lock); + ret = vgasr_priv.handler->switch_ddc(client_id); + mutex_unlock(&vgasr_priv.ddc_lock); + if (ret < 0) { + pr_err("vga_switcheroo: failed to switch DDC lines\n"); + goto out; + } + } ret = vgasr_priv.handler->switchto(client_id); goto out; } @@ -716,6 +729,13 @@ static int vga_switcheroo_runtime_suspend(struct device *dev) ret = dev->bus->pm->runtime_suspend(dev); if (ret) return ret; + if (vgasr_priv.handler->switch_ddc) { + mutex_lock(&vgasr_priv.ddc_lock); + ret = vgasr_priv.handler->switch_ddc(VGA_SWITCHEROO_IGD); + mutex_unlock(&vgasr_priv.ddc_lock); + if (ret < 0) + pr_err("vga_switcheroo: failed to switch DDC lines\n"); + } if (vgasr_priv.handler->switchto) vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD); vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_OFF); diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c index f0a55a4..08bdf1e 100644 --- a/drivers/platform/x86/apple-gmux.c +++ b/drivers/platform/x86/apple-gmux.c @@ -275,11 +275,24 @@ static const struct backlight_ops gmux_bl_ops = { static int gmux_switch_ddc(enum vga_switcheroo_client_id id) { + enum vga_switcheroo_client_id old_ddc_owner; + + if (gmux_read8(apple_gmux_data, GMUX_PORT_SWITCH_DDC) == 1) + old_ddc_owner = VGA_SWITCHEROO_IGD; + else + old_ddc_owner = VGA_SWITCHEROO_DIS; + + pr_debug("Switching gmux DDC from %d to %d\n", old_ddc_owner, id); + + if (id == old_ddc_owner) + return old_ddc_owner; + if (id == VGA_SWITCHEROO_IGD) gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1); else gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2); - return 0; + + return old_ddc_owner; } static int gmux_switchto(enum vga_switcheroo_client_id id) diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h index 352bef3..39b25b0 100644 --- a/include/linux/vga_switcheroo.h +++ b/include/linux/vga_switcheroo.h @@ -77,7 +77,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 void vga_switcheroo_switch_ddc(struct pci_dev *pdev) { return NULL; } +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,