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