On Thu, Nov 21, 2013 at 9:11 AM, Maarten Lankhorst maarten.lankhorst@canonical.com wrote:
op 20-11-13 21:48, Rob Clark schreef:
At the moment, this doesn't do anything. But for atomic we will have an ww_acquire_ctx associated with the state, to simplify the locking and avoid potential deadlock when we cannot control the locking order.
Nack. :-)
Please don't split this out. ww_mutex may be backed by a mutex, but that's an implementation detail you must not rely on.
well, everywhere (but mutex_lock_nest_lock()) is using the ww_mutex fxns, so once the mutex_lock_nest_lock() thing is sorted, that shouldn't be a problem. The reason I was thinking about either squashing this, or re-juggling a bit is because it doesn't make much sense to change everything to ww_mutex and then a couple patches later to drm_modeset_{lock,unlock}_crtc().
BR, -R
drivers/gpu/drm/drm_crtc.c | 20 +++++++++++--------- drivers/gpu/drm/i915/intel_display.c | 16 ++++++++-------- drivers/gpu/drm/omapdrm/omap_crtc.c | 10 +++++----- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 ++++++++---- include/drm/drm_crtc.h | 3 ++- 5 files changed, 34 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 81ac351..55f37db 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -52,7 +52,7 @@ void drm_modeset_lock_all(struct drm_device *dev) mutex_lock(&dev->mode_config.mutex);
list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
mutex_lock_nest_lock(&crtc->mutex, &dev->mode_config.mutex);
mutex_lock_nest_lock(&crtc->mutex.base, &dev->mode_config.mutex);}
This breaks ww_mutex semantics, for example. What if someone holding a ww_ctx acquires has one mutex, and tries to acquire a second crtc mutex? If lockdep was smart it would notice that this lock is nested in different locks, but I don't think lockdep is that smart.
EXPORT_SYMBOL(drm_modeset_lock_all);
@@ -65,7 +65,7 @@ void drm_modeset_unlock_all(struct drm_device *dev) struct drm_crtc *crtc;
list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
mutex_unlock(&crtc->mutex);
ww_mutex_unlock(&crtc->mutex); mutex_unlock(&dev->mode_config.mutex);} @@ -84,7 +84,7 @@ void drm_warn_on_modeset_not_all_locked(struct drm_device *dev) return;
list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
WARN_ON(!mutex_is_locked(&crtc->mutex));
WARN_ON(!ww_mutex_is_locked(&crtc->mutex)); WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));} @@ -613,6 +613,8 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) } EXPORT_SYMBOL(drm_framebuffer_remove);
+static DEFINE_WW_CLASS(crtc_ww_class);
/**
- drm_crtc_init - Initialise a new CRTC object
- @dev: DRM device
@@ -634,8 +636,8 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, crtc->invert_dimensions = false;
drm_modeset_lock_all(dev);
mutex_init(&crtc->mutex);mutex_lock_nest_lock(&crtc->mutex, &dev->mode_config.mutex);
ww_mutex_init(&crtc->mutex, &crtc_ww_class);mutex_lock_nest_lock(&crtc->mutex.base, &dev->mode_config.mutex);In a later patch you keep this snippet, please make this a trylock instead. It removes the assumption that ww_mutex has mutex as base.
~Maarten