| Message ID | 1398199519-21821-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 22, 2014 at 10:45:19PM +0200, Daniel Vetter wrote: > 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. > > v2: > - Simplify the ->can_switch implementations more (Thierry) > - Fix comment about the dev->open_count locking (Thierry) > > Cc: Thierry Reding <treding@nvidia.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> (v1) > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_dma.c | 11 ++++++----- > drivers/gpu/drm/nouveau/nouveau_vga.c | 11 ++++++----- > drivers/gpu/drm/radeon/radeon_device.c | 11 ++++++----- > include/drm/drmP.h | 2 +- > 4 files changed, 19 insertions(+), 16 deletions(-) Looks good: Reviewed-by: Thierry Reding <treding@nvidia.com>
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 96177eec0a0e..283ff06001bc 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1277,12 +1277,13 @@ static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_ 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); - can_switch = (dev->open_count == 0); - spin_unlock(&dev->count_lock); - return can_switch; + /* + * 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. + */ + return dev->open_count == 0; } static const struct vga_switcheroo_client_ops i915_switcheroo_ops = { diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c index fb84da3cb50d..4f4c3fec6916 100644 --- a/drivers/gpu/drm/nouveau/nouveau_vga.c +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c @@ -64,12 +64,13 @@ static bool nouveau_switcheroo_can_switch(struct pci_dev *pdev) { struct drm_device *dev = pci_get_drvdata(pdev); - bool can_switch; - spin_lock(&dev->count_lock); - can_switch = (dev->open_count == 0); - spin_unlock(&dev->count_lock); - return can_switch; + /* + * 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. + */ + return dev->open_count == 0; } static const struct vga_switcheroo_client_ops diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index 511fe26198e4..9aa1afd1786e 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -1125,12 +1125,13 @@ static void radeon_switcheroo_set_state(struct pci_dev *pdev, enum vga_switchero 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); - can_switch = (dev->open_count == 0); - spin_unlock(&dev->count_lock); - return can_switch; + /* + * 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. + */ + return dev->open_count == 0; } static const struct vga_switcheroo_client_ops radeon_switcheroo_ops = { diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 9f1fb8d36b67..a20d882ca265 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1076,7 +1076,7 @@ struct drm_device { /** \name Usage Counters */ /*@{ */ - int open_count; /**< Outstanding files open */ + int open_count; /**< Outstanding files open, protected by drm_global_mutex. */ int buf_use; /**< Buffers in use -- cannot alloc */ atomic_t buf_alloc; /**< Buffer allocation in progress */ /*@} */