[v2,19/37] drm/i915: treat shmem as a region

Commit Message

Matthew Auld June 27, 2019, 8:56 p.m. UTC

Signed-off-by: Matthew Auld <[e-mail]>
Cc: Joonas Lahtinen <[e-mail]>
Cc: Abdiel Janulgue <[e-mail]>
---
 drivers/gpu/drm/i915/gem/i915_gem_shmem.c     | 67 ++++++++++++++-----
 drivers/gpu/drm/i915/i915_drv.c               |  5 +-
 drivers/gpu/drm/i915/i915_drv.h               |  4 +-
 drivers/gpu/drm/i915/i915_gem.c               | 13 +---
 drivers/gpu/drm/i915/i915_gem_gtt.c           | 11 ++-
 drivers/gpu/drm/i915/intel_memory_region.c    |  9 +++
 .../gpu/drm/i915/selftests/mock_gem_device.c  |  4 --
 7 files changed, 68 insertions(+), 45 deletions(-)

Comments

Chris Wilson June 27, 2019, 11:55 p.m. UTC | #1

Quoting Matthew Auld (2019-06-27 21:56:15)
>  int i915_gem_freeze(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index e4f811fdaedc..958c61e88200 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2941,7 +2941,8 @@ int i915_gem_init_memory_regions(struct drm_i915_private *i915)
>  
>                 type = MEMORY_TYPE_FROM_REGION(intel_region_map[i]);
>                 switch (type) {
> -               default:
> +               case INTEL_SMEM:
> +                       mem = i915_gem_shmem_setup(i915);
>                         break;
>                 }
>  
> @@ -2951,11 +2952,9 @@ int i915_gem_init_memory_regions(struct drm_i915_private *i915)
>                         goto out_cleanup;
>                 }
>  
> -               if (mem) {
> -                       mem->id = intel_region_map[i];
> -                       mem->type = type;
> -                       mem->instance = MEMORY_INSTANCE_FROM_REGION(intel_region_map[i]);
> -               }
> +               mem->id = intel_region_map[i];
> +               mem->type = type;
> +               mem->instance = MEMORY_INSTANCE_FROM_REGION(intel_region_map[i]);

Go back and adjust the stub function you just introduced to avoid
self-inflicted churn.

Meanwhile I'm left with this magic that isn't even defined in this patch
to try and figure out if this is equivalent to the code you just
removed.
-Chris

Patch

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 19d9ecdb2894..11c7ebda9e0e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -8,6 +8,7 @@ 
 #include <linux/swap.h>
 
 #include "i915_drv.h"
+#include "i915_gemfs.h"
 #include "i915_gem_object.h"
 #include "i915_scatterlist.h"
 
@@ -25,6 +26,7 @@  static void check_release_pagevec(struct pagevec *pvec)
 static int shmem_get_pages(struct drm_i915_gem_object *obj)
 {
 	struct drm_i915_private *i915 = to_i915(obj->base.dev);
+	struct intel_memory_region *mem = obj->memory_region;
 	const unsigned long page_count = obj->base.size / PAGE_SIZE;
 	unsigned long i;
 	struct address_space *mapping;
@@ -51,7 +53,7 @@  static int shmem_get_pages(struct drm_i915_gem_object *obj)
 	 * If there's no chance of allocating enough pages for the whole
 	 * object, bail early.
 	 */
-	if (page_count > totalram_pages())
+	if (obj->base.size > resource_size(&mem->region))
 		return -ENOMEM;
 
 	st = kmalloc(sizeof(*st), GFP_KERNEL);
@@ -424,9 +426,11 @@  const struct drm_i915_gem_object_ops i915_gem_shmem_ops = {
 	.writeback = shmem_writeback,
 
 	.pwrite = shmem_pwrite,
+
+	.release = i915_gem_object_release_memory_region,
 };
 
-static int create_shmem(struct drm_i915_private *i915,
+static int __create_shmem(struct drm_i915_private *i915,
 			struct drm_gem_object *obj,
 			size_t size)
 {
@@ -447,31 +451,23 @@  static int create_shmem(struct drm_i915_private *i915,
 	return 0;
 }
 
-struct drm_i915_gem_object *
-i915_gem_object_create_shmem(struct drm_i915_private *i915, u64 size)
+static struct drm_i915_gem_object *
+create_shmem(struct intel_memory_region *mem,
+	     resource_size_t size,
+	     unsigned flags)
 {
+	struct drm_i915_private *i915 = mem->i915;
 	struct drm_i915_gem_object *obj;
 	struct address_space *mapping;
 	unsigned int cache_level;
 	gfp_t mask;
 	int ret;
 
-	/* There is a prevalence of the assumption that we fit the object's
-	 * page count inside a 32bit _signed_ variable. Let's document this and
-	 * catch if we ever need to fix it. In the meantime, if you do spot
-	 * such a local variable, please consider fixing!
-	 */
-	if (size >> PAGE_SHIFT > INT_MAX)
-		return ERR_PTR(-E2BIG);
-
-	if (overflows_type(size, obj->base.size))
-		return ERR_PTR(-E2BIG);
-
 	obj = i915_gem_object_alloc();
 	if (!obj)
 		return ERR_PTR(-ENOMEM);
 
-	ret = create_shmem(i915, &obj->base, size);
+	ret = __create_shmem(i915, &obj->base, size);
 	if (ret)
 		goto fail;
 
@@ -510,8 +506,6 @@  i915_gem_object_create_shmem(struct drm_i915_private *i915, u64 size)
 
 	i915_gem_object_set_cache_coherency(obj, cache_level);
 
-	trace_i915_gem_object_create(obj);
-
 	return obj;
 
 fail:
@@ -519,6 +513,13 @@  i915_gem_object_create_shmem(struct drm_i915_private *i915, u64 size)
 	return ERR_PTR(ret);
 }
 
+struct drm_i915_gem_object *
+i915_gem_object_create_shmem(struct drm_i915_private *i915, u64 size)
+{
+	return i915_gem_object_create_region(i915->regions[INTEL_MEMORY_SMEM],
+					     size, 0);
+}
+
 /* Allocate a new GEM object and fill it with the supplied data */
 struct drm_i915_gem_object *
 i915_gem_object_create_shmem_from_data(struct drm_i915_private *dev_priv,
@@ -569,3 +570,33 @@  i915_gem_object_create_shmem_from_data(struct drm_i915_private *dev_priv,
 	i915_gem_object_put(obj);
 	return ERR_PTR(err);
 }
+
+static int init_shmem(struct intel_memory_region *mem)
+{
+	int err;
+
+	err = i915_gemfs_init(mem->i915);
+	if (err)
+		 DRM_NOTE("Unable to create a private tmpfs mount, hugepage support will be disabled(%d).\n", err);
+
+	return 0; /* Don't error, we can simply fallback to the kernel mnt */
+}
+
+static void release_shmem(struct intel_memory_region *mem)
+{
+	i915_gemfs_fini(mem->i915);
+}
+
+static const struct intel_memory_region_ops shmem_region_ops = {
+	.init = init_shmem,
+	.release = release_shmem,
+	.create_object = create_shmem,
+};
+
+struct intel_memory_region *i915_gem_shmem_setup(struct drm_i915_private *i915)
+{
+	return intel_memory_region_create(i915, 0,
+					  totalram_pages() << PAGE_SHIFT,
+					  I915_GTT_PAGE_SIZE_4K, 0,
+					  &shmem_region_ops);
+}
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 794c6814a6d0..ac8fbada0406 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -926,9 +926,7 @@  static int i915_driver_init_early(struct drm_i915_private *dev_priv)
 
 	intel_gt_init_early(&dev_priv->gt, dev_priv);
 
-	ret = i915_gem_init_early(dev_priv);
-	if (ret < 0)
-		goto err_workqueues;
+	i915_gem_init_early(dev_priv);
 
 	/* This must be called before any calls to HAS_PCH_* */
 	intel_detect_pch(dev_priv);
@@ -954,7 +952,6 @@  static int i915_driver_init_early(struct drm_i915_private *dev_priv)
 err_uc:
 	intel_uc_cleanup_early(dev_priv);
 	i915_gem_cleanup_early(dev_priv);
-err_workqueues:
 	i915_workqueues_cleanup(dev_priv);
 err_engines:
 	i915_engines_cleanup(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 42674a41e469..e8f41c235e70 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2447,11 +2447,13 @@  static inline bool intel_vgpu_active(struct drm_i915_private *dev_priv)
 int i915_gem_init_userptr(struct drm_i915_private *dev_priv);
 void i915_gem_cleanup_userptr(struct drm_i915_private *dev_priv);
 void i915_gem_sanitize(struct drm_i915_private *i915);
-int i915_gem_init_early(struct drm_i915_private *dev_priv);
+void i915_gem_init_early(struct drm_i915_private *dev_priv);
 void i915_gem_cleanup_early(struct drm_i915_private *dev_priv);
 int i915_gem_freeze(struct drm_i915_private *dev_priv);
 int i915_gem_freeze_late(struct drm_i915_private *dev_priv);
 
+struct intel_memory_region *i915_gem_shmem_setup(struct drm_i915_private *i915);
+
 static inline void i915_gem_drain_freed_objects(struct drm_i915_private *i915)
 {
 	if (!atomic_read(&i915->mm.free_count))
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 4ba386ab35e7..009e7199bea6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -45,7 +45,6 @@ 
 #include "gem/i915_gem_context.h"
 #include "gem/i915_gem_ioctls.h"
 #include "gem/i915_gem_pm.h"
-#include "gem/i915_gemfs.h"
 #include "gt/intel_gt.h"
 #include "gt/intel_gt_pm.h"
 #include "gt/intel_mocs.h"
@@ -1677,10 +1676,8 @@  static void i915_gem_init__mm(struct drm_i915_private *i915)
 	i915_gem_init__objects(i915);
 }
 
-int i915_gem_init_early(struct drm_i915_private *dev_priv)
+void i915_gem_init_early(struct drm_i915_private *dev_priv)
 {
-	int err;
-
 	i915_gem_init__mm(dev_priv);
 	i915_gem_init__pm(dev_priv);
 
@@ -1692,12 +1689,6 @@  int i915_gem_init_early(struct drm_i915_private *dev_priv)
 	atomic_set(&dev_priv->mm.bsd_engine_dispatch_index, 0);
 
 	spin_lock_init(&dev_priv->fb_tracking.lock);
-
-	err = i915_gemfs_init(dev_priv);
-	if (err)
-		DRM_NOTE("Unable to create a private tmpfs mount, hugepage support will be disabled(%d).\n", err);
-
-	return 0;
 }
 
 void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
@@ -1708,8 +1699,6 @@  void i915_gem_cleanup_early(struct drm_i915_private *dev_priv)
 	WARN_ON(dev_priv->mm.shrink_count);
 
 	cleanup_srcu_struct(&dev_priv->gpu_error.reset_backoff_srcu);
-
-	i915_gemfs_fini(dev_priv);
 }
 
 int i915_gem_freeze(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e4f811fdaedc..958c61e88200 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2941,7 +2941,8 @@  int i915_gem_init_memory_regions(struct drm_i915_private *i915)
 
 		type = MEMORY_TYPE_FROM_REGION(intel_region_map[i]);
 		switch (type) {
-		default:
+		case INTEL_SMEM:
+			mem = i915_gem_shmem_setup(i915);
 			break;
 		}
 
@@ -2951,11 +2952,9 @@  int i915_gem_init_memory_regions(struct drm_i915_private *i915)
 			goto out_cleanup;
 		}
 
-		if (mem) {
-			mem->id = intel_region_map[i];
-			mem->type = type;
-			mem->instance = MEMORY_INSTANCE_FROM_REGION(intel_region_map[i]);
-		}
+		mem->id = intel_region_map[i];
+		mem->type = type;
+		mem->instance = MEMORY_INSTANCE_FROM_REGION(intel_region_map[i]);
 
 		i915->regions[i] = mem;
 	}
diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
index cd41c212bc35..e6bf4bc122bd 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.c
+++ b/drivers/gpu/drm/i915/intel_memory_region.c
@@ -234,6 +234,13 @@  i915_gem_object_create_region(struct intel_memory_region *mem,
 	GEM_BUG_ON(!size);
 	GEM_BUG_ON(!IS_ALIGNED(size, I915_GTT_MIN_ALIGNMENT));
 
+	/*
+	 * There is a prevalence of the assumption that we fit the object's
+	 * page count inside a 32bit _signed_ variable. Let's document this and
+	 * catch if we ever need to fix it. In the meantime, if you do spot
+	 * such a local variable, please consider fixing!
+	 */
+
 	if (size >> PAGE_SHIFT > INT_MAX)
 		return ERR_PTR(-E2BIG);
 
@@ -257,6 +264,8 @@  i915_gem_object_create_region(struct intel_memory_region *mem,
 
 	mutex_unlock(&mem->obj_lock);
 
+	trace_i915_gem_object_create(obj);
+
 	return obj;
 }
 
diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
index df07a0bd089d..007108d26acb 100644
--- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
+++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
@@ -80,8 +80,6 @@  static void mock_device_release(struct drm_device *dev)
 
 	destroy_workqueue(i915->wq);
 
-	i915_gemfs_fini(i915);
-
 	i915_gem_cleanup_memory_regions(i915);
 
 	drm_mode_config_cleanup(&i915->drm);
@@ -225,8 +223,6 @@  struct drm_i915_private *mock_gem_device(void)
 
 	mutex_unlock(&i915->drm.struct_mutex);
 
-	WARN_ON(i915_gemfs_init(i915));
-
 	err = i915_gem_init_memory_regions(i915);
 	if (err)
 		goto err_context;