[v4,07/12] drm/i915: GEM operations need to be done under the big lock
diff mbox

Message ID 1454424379-11524-1-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin Feb. 2, 2016, 2:46 p.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

VMA creation and GEM list management need the big lock.

v2:

Mutex unlock ended on the wrong path somehow. (0-day, Julia Lawall)

Not to mention drm_gem_object_unreference was there in existing
code with no mutex held.

v3:

Some callers of i915_gem_object_create_stolen_for_preallocated
already hold the lock so move the mutex into the other caller
as well.

v4:

Changed to lockdep_assert_held. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 2 ++
 drivers/gpu/drm/i915/intel_display.c   | 8 ++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

Comments

Chris Wilson Feb. 11, 2016, 10:13 a.m. UTC | #1
On Tue, Feb 02, 2016 at 02:46:19PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> VMA creation and GEM list management need the big lock.
> 
> v2:
> 
> Mutex unlock ended on the wrong path somehow. (0-day, Julia Lawall)
> 
> Not to mention drm_gem_object_unreference was there in existing
> code with no mutex held.
> 
> v3:
> 
> Some callers of i915_gem_object_create_stolen_for_preallocated
> already hold the lock so move the mutex into the other caller
> as well.
> 
> v4:
> 
> Changed to lockdep_assert_held. (Chris Wilson)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

Also
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
:)

Daniel, what did you think of having a config option to turn
lockdep_assert_held into WARN_ON(!mutex_is_locked()) when lockdep is not
fully-enabled?
-Chris
Daniel Vetter Feb. 15, 2016, 4:09 p.m. UTC | #2
On Thu, Feb 11, 2016 at 10:13:30AM +0000, Chris Wilson wrote:
> On Tue, Feb 02, 2016 at 02:46:19PM +0000, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > VMA creation and GEM list management need the big lock.
> > 
> > v2:
> > 
> > Mutex unlock ended on the wrong path somehow. (0-day, Julia Lawall)
> > 
> > Not to mention drm_gem_object_unreference was there in existing
> > code with no mutex held.
> > 
> > v3:
> > 
> > Some callers of i915_gem_object_create_stolen_for_preallocated
> > already hold the lock so move the mutex into the other caller
> > as well.
> > 
> > v4:
> > 
> > Changed to lockdep_assert_held. (Chris Wilson)
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Also
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> :)
> 
> Daniel, what did you think of having a config option to turn
> lockdep_assert_held into WARN_ON(!mutex_is_locked()) when lockdep is not
> fully-enabled?

I like, and kinda would even go as far as making it the default. But that
might upset some performance concious folks, so Kconfig seems fine. Cany
you please submit that to a wider audience? Then we'd never have to have
this discussion again ;-)

Thanks, Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index c384dc9c8a63..8cb0371673c5 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -635,6 +635,8 @@  i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return NULL;
 
+	lockdep_assert_held(&dev->struct_mutex);
+
 	DRM_DEBUG_KMS("creating preallocated stolen object: stolen_offset=%x, gtt_offset=%x, size=%x\n",
 			stolen_offset, gtt_offset, size);
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f3c036d98b6c..b7ee2f3af856 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2551,12 +2551,16 @@  intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
 	if (size_aligned * 2 > dev_priv->gtt.stolen_usable_size)
 		return false;
 
+	mutex_lock(&dev->struct_mutex);
+
 	obj = i915_gem_object_create_stolen_for_preallocated(dev,
 							     base_aligned,
 							     base_aligned,
 							     size_aligned);
-	if (!obj)
+	if (!obj) {
+		mutex_unlock(&dev->struct_mutex);
 		return false;
+	}
 
 	obj->tiling_mode = plane_config->tiling;
 	if (obj->tiling_mode == I915_TILING_X)
@@ -2569,12 +2573,12 @@  intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
 	mode_cmd.modifier[0] = fb->modifier[0];
 	mode_cmd.flags = DRM_MODE_FB_MODIFIERS;
 
-	mutex_lock(&dev->struct_mutex);
 	if (intel_framebuffer_init(dev, to_intel_framebuffer(fb),
 				   &mode_cmd, obj)) {
 		DRM_DEBUG_KMS("intel fb init failed\n");
 		goto out_unref_obj;
 	}
+
 	mutex_unlock(&dev->struct_mutex);
 
 	DRM_DEBUG_KMS("initial plane fb obj %p\n", obj);