From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: [PATCH 35/50] drm: Rip out totally bogus vga_switcheroo->can_switch locking Date: Wed, 11 Dec 2013 11:34:56 +0100 Message-ID: <1386758111-3446-36-git-send-email-daniel.vetter@ffwll.ch> References: <1386758111-3446-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f178.google.com (mail-ea0-f178.google.com [209.85.215.178]) by gabe.freedesktop.org (Postfix) with ESMTP id 8A251FB581 for ; Wed, 11 Dec 2013 02:35:16 -0800 (PST) Received: by mail-ea0-f178.google.com with SMTP id d10so2774023eaj.37 for ; Wed, 11 Dec 2013 02:35:15 -0800 (PST) In-Reply-To: <1386758111-3446-1-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: DRI Development Cc: Daniel Vetter List-Id: dri-devel@lists.freedesktop.org So I just wanted to add a new field to struct drm_device and accidentally stumbled over something. According to comments dev->open_count is protected by dev->count_lock, but that's totally not the case. It's protected by drm_global_mutex. Unfortunately the vga switcheroo callbacks took this comment at face value. The problem is that we can't just take the drm_global_mutex because: - It would lead to a locking inversion with the driver load/unload paths. - It wouldn't actually protect anything, for that we'd need to wrap the entire vga switcheroo code in the drm_global_mutex. And I'm not sure whether that would actually solve anything. What we probably want is a try_to_grab_switcheroo reference kind of thing which is used in the driver's ->open callback. Then we could move all that ->can_switch madness into the vga switcheroo core where it really belongs. But since that would amount to real work take the easy way out and just add a comment. It's definitely not going to make anything worse since doing switcheroo state changes while restarting X just isn't recommended. Even though the delayed switching code does exactly that. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_dma.c | 7 +++++-- drivers/gpu/drm/nouveau/nouveau_vga.c | 7 +++++-- drivers/gpu/drm/radeon/radeon_device.c | 7 +++++-- include/drm/drmP.h | 2 +- 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 7f653ab59db4..2babb597d23c 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1269,9 +1269,12 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev) struct drm_device *dev = pci_get_drvdata(pdev); bool can_switch; - spin_lock(&dev->count_lock); + /* + * FIXME: open_count is protected by drm_global_mutex but that would lead to + * locking inversion with the driver load path. And the access here is + * completely racy anyway. So don't bother with locking for now. + */ can_switch = (dev->open_count == 0); - spin_unlock(&dev->count_lock); return can_switch; } diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c index 81638d7f2eff..db721b4d778b 100644 --- a/drivers/gpu/drm/nouveau/nouveau_vga.c +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c @@ -64,9 +64,12 @@ nouveau_switcheroo_can_switch(struct pci_dev *pdev) struct drm_device *dev = pci_get_drvdata(pdev); bool can_switch; - spin_lock(&dev->count_lock); + /* + * FIXME: open_count is protected by drm_global_mutex but that would lead to + * locking inversion with the driver load path. And the access here is + * completely racy anyway. So don't bother with locking for now. + */ can_switch = (dev->open_count == 0); - spin_unlock(&dev->count_lock); return can_switch; } diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index 39b033b441d2..d67140811ca5 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -1119,9 +1119,12 @@ static bool radeon_switcheroo_can_switch(struct pci_dev *pdev) struct drm_device *dev = pci_get_drvdata(pdev); bool can_switch; - spin_lock(&dev->count_lock); + /* + * FIXME: open_count is protected by drm_global_mutex but that would lead to + * locking inversion with the driver load path. And the access here is + * completely racy anyway. So don't bother with locking for now. + */ can_switch = (dev->open_count == 0); - spin_unlock(&dev->count_lock); return can_switch; } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 1016ecc8de95..ee65c25e25d3 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1085,7 +1085,7 @@ struct drm_device { /** \name Usage Counters */ /*@{ */ - int open_count; /**< Outstanding files open */ + int open_count; /**< Outstanding files open, protected by drm_global_lock. */ atomic_t ioctl_count; /**< Outstanding IOCTLs pending */ atomic_t vma_count; /**< Outstanding vma areas open */ int buf_use; /**< Buffers in use -- cannot alloc */ -- 1.8.4.3