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: 6246731 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 8567EBF4A6 for ; Tue, 21 Apr 2015 11:36:57 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 3292A2022D for ; Tue, 21 Apr 2015 11:36:56 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id EFA2120221 for ; Tue, 21 Apr 2015 11:36:54 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 00FC66E5A0; Tue, 21 Apr 2015 04:36:52 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mailout1.hostsharing.net (mailout1.hostsharing.net [83.223.95.204]) by gabe.freedesktop.org (Postfix) with ESMTP id A3D626E48F for ; Tue, 21 Apr 2015 04:36:49 -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 mailout1.hostsharing.net (Postfix) with ESMTPS id A483A103900C3; Tue, 21 Apr 2015 13:31:34 +0200 (CEST) Received: from localhost (5-38-90-81.adsl.cmo.de [81.90.38.5]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by h08.hostsharing.net (Postfix) with ESMTPSA id 444C2603E04D; Tue, 21 Apr 2015 13:31:27 +0200 (CEST) X-Mailbox-Line: From 82851f51fc7afd6d975a11331ba5ee17b7175af2 Mon Sep 17 00:00:00 2001 Message-Id: <82851f51fc7afd6d975a11331ba5ee17b7175af2.1429610300.git.lukas@wunner.de> In-Reply-To: References: From: Lukas Wunner Date: Fri, 27 Mar 2015 12:29:40 +0100 Subject: [PATCH 06/11] vga_switcheroo: Lock/unlock DDC lines harder To: dri-devel@lists.freedesktop.org Cc: Daniel Vetter , Seth Forshee , 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,T_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. intel_dp_detect_dpcd() calls drm_probe_ddc(), needs to lock DDC lines. Don't lock vgasr_mutex in vga_switcheroo_lock/unlock_ddc(), it's already locked in vga_switcheroo_debugfs_write(), leading to a deadlock upon reprobe in stage2 when the new client reads the EDID. Locking ddc_lock is sufficient. 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. 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. Signed-off-by: Lukas Wunner --- drivers/gpu/drm/drm_edid.c | 4 +- drivers/gpu/drm/i915/intel_dp.c | 6 ++- drivers/gpu/vga/vga_switcheroo.c | 82 ++++++++++++++++++++++----------------- drivers/platform/x86/apple-gmux.c | 15 ++++++- include/linux/vga_switcheroo.h | 3 +- 5 files changed, 71 insertions(+), 39 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index f91593b..8950722 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1329,8 +1329,10 @@ struct edid *drm_get_edid(struct drm_connector *connector, vga_switcheroo_lock_ddc(connector->dev->pdev); - if (!drm_probe_ddc(adapter)) + if (!drm_probe_ddc(adapter)) { + vga_switcheroo_unlock_ddc(connector->dev->pdev); return NULL; + } edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); if (edid) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index d023710..e9c40b1 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -4079,6 +4080,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) { uint8_t *dpcd = intel_dp->dpcd; uint8_t type; + struct pci_dev *pdev = intel_dp->attached_connector->base.dev->pdev; if (!intel_dp_get_dpcd(intel_dp)) return connector_status_disconnected; @@ -4101,7 +4103,9 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) } /* If no HPD, poke DDC gently */ - if (drm_probe_ddc(&intel_dp->aux.ddc)) + if (vga_switcheroo_lock_ddc(pdev) >= 0 && + drm_probe_ddc(&intel_dp->aux.ddc) && + vga_switcheroo_unlock_ddc(pdev) >= 0) return connector_status_connected; /* Well we tried, say unknown for unreliable port types */ diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c index 0223020..2534d84 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,12 +277,9 @@ 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; - mutex_lock(&vgasr_mutex); - if (!vgasr_priv.handler) { ret = -ENODEV; goto out; @@ -290,22 +288,17 @@ int vga_switcheroo_lock_ddc(struct pci_dev *pdev) if (vgasr_priv.handler->switch_ddc) { mutex_lock(&vgasr_priv.ddc_lock); - 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) { + mutex_unlock(&vgasr_priv.ddc_lock); + printk(KERN_ERR "vga_switcheroo: failed to switch DDC lines\n"); + } else + vgasr_priv.old_ddc_owner = ret; } out: - mutex_unlock(&vgasr_mutex); return ret; } EXPORT_SYMBOL(vga_switcheroo_lock_ddc); @@ -314,7 +307,6 @@ int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) { int ret = 0; int id; - mutex_lock(&vgasr_mutex); if (!vgasr_priv.handler) { ret = -ENODEV; @@ -322,15 +314,17 @@ int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) } 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; + 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) + printk(KERN_ERR "vga_switcheroo: failed to switch DDC lines\n"); + mutex_unlock(&vgasr_priv.ddc_lock); } + out: - mutex_unlock(&vgasr_mutex); return ret; } EXPORT_SYMBOL(vga_switcheroo_unlock_ddc); @@ -433,14 +427,24 @@ 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) { + printk(KERN_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) { + printk(KERN_ERR "vga_switcheroo: failed to switch to client %d\n", new_client->id); + /* 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 +456,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 +557,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) { + printk(KERN_ERR "vga_switcheroo: failed to switch DDC lines\n"); + goto out; + } + } ret = vgasr_priv.handler->switchto(client_id); goto out; } @@ -716,6 +721,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) + printk(KERN_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 1cf242c..9aafbde 100644 --- a/drivers/platform/x86/apple-gmux.c +++ b/drivers/platform/x86/apple-gmux.c @@ -273,11 +273,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..8963799 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 0; } +static inline int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) { return 0; } 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,