| Message ID | 1397252175-14227-9-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 11, 2014 at 11:36:05PM +0200, Daniel Vetter wrote: [...] > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 4dff829c2d89..dcb67c6564dd 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1286,9 +1286,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; > } Couldn't this now be shortened to: return (dev->open_count == 0); ? Similarily for the other drivers. > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 9f1fb8d36b67..bd16a4c8ece4 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_lock. */ s/drm_global_lock/drm_global_mutex/? Thierry
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 4dff829c2d89..dcb67c6564dd 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1286,9 +1286,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 fb84da3cb50d..7b10c5a9196b 100644 --- a/drivers/gpu/drm/nouveau/nouveau_vga.c +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c @@ -66,9 +66,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 835516d2d257..c9811d01a3c7 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -1124,9 +1124,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 9f1fb8d36b67..bd16a4c8ece4 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_lock. */ int buf_use; /**< Buffers in use -- cannot alloc */ atomic_t buf_alloc; /**< Buffer allocation in progress */ /*@} */
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 <daniel.vetter@ffwll.ch> --- 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(-)