diff mbox

[3/7] drm/i915: add dev_priv->mm.stolen_lock

Message ID 1435781726-7282-4-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni July 1, 2015, 8:15 p.m. UTC 
From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Which should protect dev_priv->mm.stolen usage. This will allow us to
simplify the relationship between stolen memory, FBC and struct_mutex.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |  7 +++-
 drivers/gpu/drm/i915/i915_gem_stolen.c | 69 +++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_fbc.c       | 29 +++++++++++---
 3 files changed, 77 insertions(+), 28 deletions(-)

Comments

Chris Wilson July 1, 2015, 8:33 p.m. UTC | #1 
On Wed, Jul 01, 2015 at 05:15:22PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Which should protect dev_priv->mm.stolen usage. This will allow us to
> simplify the relationship between stolen memory, FBC and struct_mutex.

Too coarse. The locking need only be around the stolen drm_mm, i.e. just
insert/remove node. (And you don't need the lock around drm_mm_initialized,
similarly teardown since like the init, they are serialised through
other means.)
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c955037..0b908b1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1245,6 +1245,10 @@  struct intel_l3_parity {
 struct i915_gem_mm {
 	/** Memory allocator for GTT stolen memory */
 	struct drm_mm stolen;
+	/** Protects the usage of the GTT stolen memory allocator. This is
+	 * always the inner lock when overlapping with struct_mutex. */
+	struct mutex stolen_lock;
+
 	/** List of all objects in gtt_space. Used to restore gtt
 	 * mappings on resume */
 	struct list_head bound_list;
@@ -3112,7 +3116,8 @@  static inline void i915_gem_chipset_flush(struct drm_device *dev)
 int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
 				struct drm_mm_node *node, u64 size,
 				unsigned alignment);
-void i915_gem_stolen_remove_node(struct drm_mm_node *node);
+void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
+				 struct drm_mm_node *node);
 int i915_gem_init_stolen(struct drm_device *dev);
 void i915_gem_cleanup_stolen(struct drm_device *dev);
 struct drm_i915_gem_object *
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 0619786..b432085 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -46,6 +46,8 @@  int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
 				struct drm_mm_node *node, u64 size,
 				unsigned alignment)
 {
+	WARN_ON(!mutex_is_locked(&dev_priv->mm.stolen_lock));
+
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return -ENODEV;
 
@@ -53,8 +55,11 @@  int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
 				  DRM_MM_SEARCH_DEFAULT);
 }
 
-void i915_gem_stolen_remove_node(struct drm_mm_node *node)
+void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
+				 struct drm_mm_node *node)
 {
+	WARN_ON(!mutex_is_locked(&dev_priv->mm.stolen_lock));
+
 	drm_mm_remove_node(node);
 }
 
@@ -171,10 +176,15 @@  void i915_gem_cleanup_stolen(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	mutex_lock(&dev_priv->mm.stolen_lock);
+
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return;
+		goto out;
 
 	drm_mm_takedown(&dev_priv->mm.stolen);
+
+out:
+	mutex_unlock(&dev_priv->mm.stolen_lock);
 }
 
 int i915_gem_init_stolen(struct drm_device *dev)
@@ -183,6 +193,8 @@  int i915_gem_init_stolen(struct drm_device *dev)
 	u32 tmp;
 	int bios_reserved = 0;
 
+	mutex_init(&dev_priv->mm.stolen_lock);
+
 #ifdef CONFIG_INTEL_IOMMU
 	if (intel_iommu_gfx_mapped && INTEL_INFO(dev)->gen < 8) {
 		DRM_INFO("DMAR active, disabling use of stolen memory\n");
@@ -273,8 +285,10 @@  static void i915_gem_object_put_pages_stolen(struct drm_i915_gem_object *obj)
 static void
 i915_gem_object_release_stolen(struct drm_i915_gem_object *obj)
 {
+	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
+
 	if (obj->stolen) {
-		i915_gem_stolen_remove_node(obj->stolen);
+		i915_gem_stolen_remove_node(dev_priv, obj->stolen);
 		kfree(obj->stolen);
 		obj->stolen = NULL;
 	}
@@ -325,29 +339,36 @@  i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
 	struct drm_mm_node *stolen;
 	int ret;
 
+	mutex_lock(&dev_priv->mm.stolen_lock);
+
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return NULL;
+		goto out_unlock;
 
 	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
 	if (size == 0)
-		return NULL;
+		goto out_unlock;
 
 	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
 	if (!stolen)
-		return NULL;
+		goto out_unlock;
 
 	ret = i915_gem_stolen_insert_node(dev_priv, stolen, size, 4096);
-	if (ret) {
-		kfree(stolen);
-		return NULL;
-	}
+	if (ret)
+		goto out_free;
 
 	obj = _i915_gem_object_create_stolen(dev, stolen);
-	if (obj)
-		return obj;
+	if (!obj)
+		goto out_node;
 
-	i915_gem_stolen_remove_node(stolen);
+	mutex_unlock(&dev_priv->mm.stolen_lock);
+	return obj;
+
+out_node:
+	i915_gem_stolen_remove_node(dev_priv, stolen);
+out_free:
 	kfree(stolen);
+out_unlock:
+	mutex_unlock(&dev_priv->mm.stolen_lock);
 	return NULL;
 }
 
@@ -364,8 +385,10 @@  i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	struct i915_vma *vma;
 	int ret;
 
+	mutex_lock(&dev_priv->mm.stolen_lock);
+
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return NULL;
+		goto err_unlock;
 
 	DRM_DEBUG_KMS("creating preallocated stolen object: stolen_offset=%x, gtt_offset=%x, size=%x\n",
 			stolen_offset, gtt_offset, size);
@@ -373,11 +396,11 @@  i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	/* KISS and expect everything to be page-aligned */
 	if (WARN_ON(size == 0) || WARN_ON(size & 4095) ||
 	    WARN_ON(stolen_offset & 4095))
-		return NULL;
+		goto err_unlock;
 
 	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
 	if (!stolen)
-		return NULL;
+		goto err_unlock;
 
 	stolen->start = stolen_offset;
 	stolen->size = size;
@@ -385,20 +408,20 @@  i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	if (ret) {
 		DRM_DEBUG_KMS("failed to allocate stolen space\n");
 		kfree(stolen);
-		return NULL;
+		goto err_unlock;
 	}
 
 	obj = _i915_gem_object_create_stolen(dev, stolen);
 	if (obj == NULL) {
 		DRM_DEBUG_KMS("failed to allocate stolen object\n");
-		i915_gem_stolen_remove_node(stolen);
+		i915_gem_stolen_remove_node(dev_priv, stolen);
 		kfree(stolen);
-		return NULL;
+		goto err_unlock;
 	}
 
 	/* Some objects just need physical mem from stolen space */
 	if (gtt_offset == I915_GTT_OFFSET_NONE)
-		return obj;
+		goto success;
 
 	vma = i915_gem_obj_lookup_or_create_vma(obj, ggtt);
 	if (IS_ERR(vma)) {
@@ -427,13 +450,17 @@  i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	list_add_tail(&vma->mm_list, &ggtt->inactive_list);
 	i915_gem_object_pin_pages(obj);
 
+success:
+	mutex_unlock(&dev_priv->mm.stolen_lock);
 	return obj;
 
 err_vma:
 	i915_gem_vma_destroy(vma);
 err_out:
-	i915_gem_stolen_remove_node(stolen);
+	i915_gem_stolen_remove_node(dev_priv, stolen);
 	kfree(stolen);
 	drm_gem_object_unreference(&obj->base);
+err_unlock:
+	mutex_unlock(&dev_priv->mm.stolen_lock);
 	return NULL;
 }
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index a91bf82..dcd83ab 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -601,40 +601,57 @@  static int intel_fbc_alloc_cfb(struct drm_device *dev, int size, int fb_cpp)
 
 err_fb:
 	kfree(compressed_llb);
-	i915_gem_stolen_remove_node(&dev_priv->fbc.compressed_fb);
+	i915_gem_stolen_remove_node(dev_priv, &dev_priv->fbc.compressed_fb);
 err_llb:
 	pr_info_once("drm: not enough stolen space for compressed buffer (need %d more bytes), disabling. Hint: you may be able to increase stolen memory size in the BIOS to avoid this.\n", size);
 	return -ENOSPC;
 }
 
-void intel_fbc_cleanup_cfb(struct drm_device *dev)
+static void __intel_fbc_cleanup_cfb(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (dev_priv->fbc.uncompressed_size == 0)
 		return;
 
-	i915_gem_stolen_remove_node(&dev_priv->fbc.compressed_fb);
+	i915_gem_stolen_remove_node(dev_priv, &dev_priv->fbc.compressed_fb);
 
 	if (dev_priv->fbc.compressed_llb) {
-		i915_gem_stolen_remove_node(dev_priv->fbc.compressed_llb);
+		i915_gem_stolen_remove_node(dev_priv,
+					    dev_priv->fbc.compressed_llb);
 		kfree(dev_priv->fbc.compressed_llb);
 	}
 
 	dev_priv->fbc.uncompressed_size = 0;
 }
 
+void intel_fbc_cleanup_cfb(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->mm.stolen_lock);
+	__intel_fbc_cleanup_cfb(dev);
+	mutex_unlock(&dev_priv->mm.stolen_lock);
+}
+
 static int intel_fbc_setup_cfb(struct drm_device *dev, int size, int fb_cpp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
 
 	if (size <= dev_priv->fbc.uncompressed_size)
 		return 0;
 
+	mutex_lock(&dev_priv->mm.stolen_lock);
+
 	/* Release any current block */
-	intel_fbc_cleanup_cfb(dev);
+	__intel_fbc_cleanup_cfb(dev);
+
+	ret = intel_fbc_alloc_cfb(dev, size, fb_cpp);
+
+	mutex_unlock(&dev_priv->mm.stolen_lock);
 
-	return intel_fbc_alloc_cfb(dev, size, fb_cpp);
+	return ret;
 }
 
 /**