diff mbox

[RFCv3,02/14] drm: convert crtc to ww_mutex

Message ID 1384980493-25499-3-git-send-email-robdclark@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rob Clark Nov. 20, 2013, 8:48 p.m. UTC 
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.
---
 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(-)

Comments

Maarten Lankhorst Nov. 21, 2013, 2:11 p.m. UTC | #1 
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.
> ---
>  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
Rob Clark Nov. 21, 2013, 3:12 p.m. UTC | #2 
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
diff mbox

Patch

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);
 }
 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);
 
 	ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC);
 	if (ret)
@@ -2284,7 +2286,7 @@  static int drm_mode_cursor_common(struct drm_device *dev,
 	}
 	crtc = obj_to_crtc(obj);
 
-	mutex_lock(&crtc->mutex);
+	ww_mutex_lock(&crtc->mutex, NULL);
 	if (req->flags & DRM_MODE_CURSOR_BO) {
 		if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) {
 			ret = -ENXIO;
@@ -2308,7 +2310,7 @@  static int drm_mode_cursor_common(struct drm_device *dev,
 		}
 	}
 out:
-	mutex_unlock(&crtc->mutex);
+	ww_mutex_unlock(&crtc->mutex);
 
 	return ret;
 
@@ -3657,7 +3659,7 @@  int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		return -ENOENT;
 	crtc = obj_to_crtc(obj);
 
-	mutex_lock(&crtc->mutex);
+	ww_mutex_lock(&crtc->mutex, NULL);
 	if (crtc->fb == NULL) {
 		/* The framebuffer is currently unbound, presumably
 		 * due to a hotplug event, that userspace has not
@@ -3741,7 +3743,7 @@  out:
 		drm_framebuffer_unreference(fb);
 	if (old_fb)
 		drm_framebuffer_unreference(old_fb);
-	mutex_unlock(&crtc->mutex);
+	ww_mutex_unlock(&crtc->mutex);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3cddd50..741188f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2232,11 +2232,11 @@  void intel_display_handle_reset(struct drm_device *dev)
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-		mutex_lock(&crtc->mutex);
+		ww_mutex_lock(&crtc->mutex, NULL);
 		if (intel_crtc->active)
 			dev_priv->display.update_plane(crtc, crtc->fb,
 						       crtc->x, crtc->y);
-		mutex_unlock(&crtc->mutex);
+		ww_mutex_unlock(&crtc->mutex);
 	}
 }
 
@@ -7550,7 +7550,7 @@  bool intel_get_load_detect_pipe(struct drm_connector *connector,
 	if (encoder->crtc) {
 		crtc = encoder->crtc;
 
-		mutex_lock(&crtc->mutex);
+		ww_mutex_lock(&crtc->mutex, NULL);
 
 		old->dpms_mode = connector->dpms;
 		old->load_detect_temp = false;
@@ -7581,7 +7581,7 @@  bool intel_get_load_detect_pipe(struct drm_connector *connector,
 		return false;
 	}
 
-	mutex_lock(&crtc->mutex);
+	ww_mutex_lock(&crtc->mutex, NULL);
 	intel_encoder->new_crtc = to_intel_crtc(crtc);
 	to_intel_connector(connector)->new_encoder = intel_encoder;
 
@@ -7609,7 +7609,7 @@  bool intel_get_load_detect_pipe(struct drm_connector *connector,
 		DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
 	if (IS_ERR(fb)) {
 		DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n");
-		mutex_unlock(&crtc->mutex);
+		ww_mutex_unlock(&crtc->mutex);
 		return false;
 	}
 
@@ -7617,7 +7617,7 @@  bool intel_get_load_detect_pipe(struct drm_connector *connector,
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
 		if (old->release_fb)
 			old->release_fb->funcs->destroy(old->release_fb);
-		mutex_unlock(&crtc->mutex);
+		ww_mutex_unlock(&crtc->mutex);
 		return false;
 	}
 
@@ -7648,7 +7648,7 @@  void intel_release_load_detect_pipe(struct drm_connector *connector,
 			drm_framebuffer_unreference(old->release_fb);
 		}
 
-		mutex_unlock(&crtc->mutex);
+		ww_mutex_unlock(&crtc->mutex);
 		return;
 	}
 
@@ -7656,7 +7656,7 @@  void intel_release_load_detect_pipe(struct drm_connector *connector,
 	if (old->dpms_mode != DRM_MODE_DPMS_ON)
 		connector->funcs->dpms(connector, old->dpms_mode);
 
-	mutex_unlock(&crtc->mutex);
+	ww_mutex_unlock(&crtc->mutex);
 }
 
 static int i9xx_pll_refclk(struct drm_device *dev,
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 5dd22ab..c09d29f 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -307,13 +307,13 @@  static void page_flip_worker(struct work_struct *work)
 	struct drm_display_mode *mode = &crtc->mode;
 	struct drm_gem_object *bo;
 
-	mutex_lock(&crtc->mutex);
+	ww_mutex_lock(&crtc->mutex, NULL);
 	omap_plane_mode_set(omap_crtc->plane, crtc, crtc->fb,
 			0, 0, mode->hdisplay, mode->vdisplay,
 			crtc->x << 16, crtc->y << 16,
 			mode->hdisplay << 16, mode->vdisplay << 16,
 			vblank_cb, crtc);
-	mutex_unlock(&crtc->mutex);
+	ww_mutex_unlock(&crtc->mutex);
 
 	bo = omap_framebuffer_bo(crtc->fb, 0);
 	drm_gem_object_unreference_unlocked(bo);
@@ -447,7 +447,7 @@  static void apply_worker(struct work_struct *work)
 	 * the callbacks and list modification all serialized
 	 * with respect to modesetting ioctls from userspace.
 	 */
-	mutex_lock(&crtc->mutex);
+	ww_mutex_lock(&crtc->mutex, NULL);
 	dispc_runtime_get();
 
 	/*
@@ -492,7 +492,7 @@  static void apply_worker(struct work_struct *work)
 
 out:
 	dispc_runtime_put();
-	mutex_unlock(&crtc->mutex);
+	ww_mutex_unlock(&crtc->mutex);
 }
 
 int omap_crtc_apply(struct drm_crtc *crtc,
@@ -500,7 +500,7 @@  int omap_crtc_apply(struct drm_crtc *crtc,
 {
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
 
-	WARN_ON(!mutex_is_locked(&crtc->mutex));
+	WARN_ON(!ww_mutex_is_locked(&crtc->mutex));
 
 	/* no need to queue it again if it is already queued: */
 	if (apply->queued)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index f91447c..7b3bf18 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -186,7 +186,7 @@  int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 	 * can do this since the caller in the drm core doesn't check anything
 	 * which is protected by any looks.
 	 */
-	mutex_unlock(&crtc->mutex);
+	ww_mutex_unlock(&crtc->mutex);
 	drm_modeset_lock_all(dev_priv->dev);
 
 	/* A lot of the code assumes this */
@@ -251,7 +251,9 @@  int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 	ret = 0;
 out:
 	drm_modeset_unlock_all(dev_priv->dev);
-	mutex_lock(&crtc->mutex);
+// XXX umm, we probably need the state object here to properly
+// re-aquire the lock..
+	ww_mutex_lock(&crtc->mutex, NULL);
 
 	return ret;
 }
@@ -272,7 +274,7 @@  int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 	 * can do this since the caller in the drm core doesn't check anything
 	 * which is protected by any looks.
 	 */
-	mutex_unlock(&crtc->mutex);
+	ww_mutex_unlock(&crtc->mutex);
 	drm_modeset_lock_all(dev_priv->dev);
 
 	vmw_cursor_update_position(dev_priv, shown,
@@ -280,7 +282,9 @@  int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 				   du->cursor_y + du->hotspot_y);
 
 	drm_modeset_unlock_all(dev_priv->dev);
-	mutex_lock(&crtc->mutex);
+// XXX umm, we probably need the state object here to properly
+// re-aquire the lock..
+	ww_mutex_lock(&crtc->mutex, NULL);
 
 	return 0;
 }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 0ca684a..3650254 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -27,6 +27,7 @@ 
 
 #include <linux/i2c.h>
 #include <linux/spinlock.h>
+#include <linux/ww_mutex.h>
 #include <linux/types.h>
 #include <linux/idr.h>
 #include <linux/fb.h>
@@ -417,7 +418,7 @@  struct drm_crtc {
 	 * state, ...) and a write lock for everything which can be update
 	 * without a full modeset (fb, cursor data, ...)
 	 */
-	struct mutex mutex;
+	struct ww_mutex mutex;
 
 	struct drm_mode_object base;