[PATCH 1/3] drm/i915: Stop holding a ref to the ppgtt from each vma

[PATCH 1/3] drm/i915: Stop holding a ref to the ppgtt from each vma

[Chris Wilson]

The context owns both the ppgtt and the vma within it, and our activity
tracking on the context ensures that we do not release active ppgtt. As
the context fulfils our obligations for active memory tracking, we can
relinquish the reference from the vma.

This fixes a silly transient refleak from closed vma being kept alive
until the entire system was idle, keeping all vm alive as well.

Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Testcase: igt/gem_ctx_create/files
Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_vma.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 274fd2a7bcb6..31efc971a3a8 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -199,7 +199,6 @@ vma_create(struct drm_i915_gem_object *obj,
 		vma->flags |= I915_VMA_GGTT;
 		list_add(&vma->obj_link, &obj->vma_list);
 	} else {
-		i915_ppgtt_get(i915_vm_to_ppgtt(vm));
 		list_add_tail(&vma->obj_link, &obj->vma_list);
 	}
 
@@ -810,9 +809,6 @@ static void __i915_vma_destroy(struct i915_vma *vma)
 	if (vma->obj)
 		rb_erase(&vma->obj_node, &vma->obj->vma_tree);
 
-	if (!i915_vma_is_ggtt(vma))
-		i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm));
-
 	rbtree_postorder_for_each_entry_safe(iter, n, &vma->active, node) {
 		GEM_BUG_ON(i915_gem_active_isset(&iter->base));
 		kfree(iter);
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: Report the number of closed vma held by each context in debugfs

[Chris Wilson]

Include a count of closed vma when reporting the per_ctx_stats of
debugfs/i915_gem_objects.

Whilst adjusting the context tracking, note that we can simply use our
list of contexts in i915->contexts rather than circumlocute via
dev->filelist and the per-file context idr.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 113 +++++++++++-----------------
 1 file changed, 42 insertions(+), 71 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 26b7e5276b15..a05d4bbc0318 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -302,6 +302,7 @@ struct file_stats {
 	u64 total, unbound;
 	u64 global, shared;
 	u64 active, inactive;
+	u64 closed;
 };
 
 static int per_file_stats(int id, void *ptr, void *data)
@@ -336,6 +337,9 @@ static int per_file_stats(int id, void *ptr, void *data)
 			stats->active += vma->node.size;
 		else
 			stats->inactive += vma->node.size;
+
+		if (i915_vma_is_closed(vma))
+			stats->closed += vma->node.size;
 	}
 
 	return 0;
@@ -343,7 +347,7 @@ static int per_file_stats(int id, void *ptr, void *data)
 
 #define print_file_stats(m, name, stats) do { \
 	if (stats.count) \
-		seq_printf(m, "%s: %lu objects, %llu bytes (%llu active, %llu inactive, %llu global, %llu shared, %llu unbound)\n", \
+		seq_printf(m, "%s: %lu objects, %llu bytes (%llu active, %llu inactive, %llu global, %llu shared, %llu unbound, %llu closed)\n", \
 			   name, \
 			   stats.count, \
 			   stats.total, \
@@ -351,7 +355,8 @@ static int per_file_stats(int id, void *ptr, void *data)
 			   stats.inactive, \
 			   stats.global, \
 			   stats.shared, \
-			   stats.unbound); \
+			   stats.unbound, \
+			   stats.closed); \
 } while (0)
 
 static void print_batch_pool_stats(struct seq_file *m,
@@ -377,44 +382,44 @@ static void print_batch_pool_stats(struct seq_file *m,
 	print_file_stats(m, "[k]batch pool", stats);
 }
 
-static int per_file_ctx_stats(int idx, void *ptr, void *data)
+static void print_context_stats(struct seq_file *m,
+				struct drm_i915_private *i915)
 {
-	struct i915_gem_context *ctx = ptr;
-	struct intel_engine_cs *engine;
-	enum intel_engine_id id;
-
-	for_each_engine(engine, ctx->i915, id) {
-		struct intel_context *ce = to_intel_context(ctx, engine);
+	struct file_stats kstats = {};
+	struct i915_gem_context *ctx;
 
-		if (ce->state)
-			per_file_stats(0, ce->state->obj, data);
-		if (ce->ring)
-			per_file_stats(0, ce->ring->vma->obj, data);
-	}
+	list_for_each_entry(ctx, &i915->contexts.list, link) {
+		struct file_stats stats = { .file_priv = ctx->file_priv };
+		struct intel_engine_cs *engine;
+		enum intel_engine_id id;
 
-	return 0;
-}
+		for_each_engine(engine, i915, id) {
+			struct intel_context *ce = to_intel_context(ctx, engine);
 
-static void print_context_stats(struct seq_file *m,
-				struct drm_i915_private *dev_priv)
-{
-	struct drm_device *dev = &dev_priv->drm;
-	struct file_stats stats;
-	struct drm_file *file;
+			if (ce->state)
+				per_file_stats(0, ce->state->obj, &kstats);
+			if (ce->ring)
+				per_file_stats(0, ce->ring->vma->obj, &kstats);
+		}
 
-	memset(&stats, 0, sizeof(stats));
+		if (!IS_ERR_OR_NULL(stats.file_priv)) {
+			struct drm_file *file = stats.file_priv->file;
+			struct task_struct *task;
 
-	mutex_lock(&dev->struct_mutex);
-	if (dev_priv->kernel_context)
-		per_file_ctx_stats(0, dev_priv->kernel_context, &stats);
+			spin_lock(&file->table_lock);
+			idr_for_each(&file->object_idr, per_file_stats, &stats);
+			spin_unlock(&file->table_lock);
 
-	list_for_each_entry(file, &dev->filelist, lhead) {
-		struct drm_i915_file_private *fpriv = file->driver_priv;
-		idr_for_each(&fpriv->context_idr, per_file_ctx_stats, &stats);
+			rcu_read_lock();
+			task = pid_task(ctx->pid ?: file->pid, PIDTYPE_PID);
+			print_file_stats(m,
+					 task ? task->comm : "<unknown>",
+					 stats);
+			rcu_read_unlock();
+		}
 	}
-	mutex_unlock(&dev->struct_mutex);
 
-	print_file_stats(m, "[k]contexts", stats);
+	print_file_stats(m, "[k]contexts", kstats);
 }
 
 static int i915_gem_object_info(struct seq_file *m, void *data)
@@ -426,14 +431,9 @@ static int i915_gem_object_info(struct seq_file *m, void *data)
 	u64 size, mapped_size, purgeable_size, dpy_size, huge_size;
 	struct drm_i915_gem_object *obj;
 	unsigned int page_sizes = 0;
-	struct drm_file *file;
 	char buf[80];
 	int ret;
 
-	ret = mutex_lock_interruptible(&dev->struct_mutex);
-	if (ret)
-		return ret;
-
 	seq_printf(m, "%u objects, %llu bytes\n",
 		   dev_priv->mm.object_count,
 		   dev_priv->mm.object_memory);
@@ -514,43 +514,14 @@ static int i915_gem_object_info(struct seq_file *m, void *data)
 					buf, sizeof(buf)));
 
 	seq_putc(m, '\n');
-	print_batch_pool_stats(m, dev_priv);
-	mutex_unlock(&dev->struct_mutex);
 
-	mutex_lock(&dev->filelist_mutex);
-	print_context_stats(m, dev_priv);
-	list_for_each_entry_reverse(file, &dev->filelist, lhead) {
-		struct file_stats stats;
-		struct drm_i915_file_private *file_priv = file->driver_priv;
-		struct i915_request *request;
-		struct task_struct *task;
-
-		mutex_lock(&dev->struct_mutex);
-
-		memset(&stats, 0, sizeof(stats));
-		stats.file_priv = file->driver_priv;
-		spin_lock(&file->table_lock);
-		idr_for_each(&file->object_idr, per_file_stats, &stats);
-		spin_unlock(&file->table_lock);
-		/*
-		 * Although we have a valid reference on file->pid, that does
-		 * not guarantee that the task_struct who called get_pid() is
-		 * still alive (e.g. get_pid(current) => fork() => exit()).
-		 * Therefore, we need to protect this ->comm access using RCU.
-		 */
-		request = list_first_entry_or_null(&file_priv->mm.request_list,
-						   struct i915_request,
-						   client_link);
-		rcu_read_lock();
-		task = pid_task(request && request->gem_context->pid ?
-				request->gem_context->pid : file->pid,
-				PIDTYPE_PID);
-		print_file_stats(m, task ? task->comm : "<unknown>", stats);
-		rcu_read_unlock();
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
 
-		mutex_unlock(&dev->struct_mutex);
-	}
-	mutex_unlock(&dev->filelist_mutex);
+	print_batch_pool_stats(m, dev_priv);
+	print_context_stats(m, dev_priv);
+	mutex_unlock(&dev->struct_mutex);
 
 	return 0;
 }
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Remove debugfs/i915_ppgtt_info

[Chris Wilson]

The information presented here is not relevant to current development.
We can either use the context information, but more often we want to
inspect the active gpu state.

The ulterior motive is to eradicate dev->filelist.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 119 ----------------------------
 1 file changed, 119 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a05d4bbc0318..bff95abd1b6a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2034,124 +2034,6 @@ static int i915_swizzle_info(struct seq_file *m, void *data)
 	return 0;
 }
 
-static int per_file_ctx(int id, void *ptr, void *data)
-{
-	struct i915_gem_context *ctx = ptr;
-	struct seq_file *m = data;
-	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
-
-	if (!ppgtt) {
-		seq_printf(m, "  no ppgtt for context %d\n",
-			   ctx->user_handle);
-		return 0;
-	}
-
-	if (i915_gem_context_is_default(ctx))
-		seq_puts(m, "  default context:\n");
-	else
-		seq_printf(m, "  context %d:\n", ctx->user_handle);
-	ppgtt->debug_dump(ppgtt, m);
-
-	return 0;
-}
-
-static void gen8_ppgtt_info(struct seq_file *m,
-			    struct drm_i915_private *dev_priv)
-{
-	struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt;
-	struct intel_engine_cs *engine;
-	enum intel_engine_id id;
-	int i;
-
-	if (!ppgtt)
-		return;
-
-	for_each_engine(engine, dev_priv, id) {
-		seq_printf(m, "%s\n", engine->name);
-		for (i = 0; i < 4; i++) {
-			u64 pdp = I915_READ(GEN8_RING_PDP_UDW(engine, i));
-			pdp <<= 32;
-			pdp |= I915_READ(GEN8_RING_PDP_LDW(engine, i));
-			seq_printf(m, "\tPDP%d 0x%016llx\n", i, pdp);
-		}
-	}
-}
-
-static void gen6_ppgtt_info(struct seq_file *m,
-			    struct drm_i915_private *dev_priv)
-{
-	struct intel_engine_cs *engine;
-	enum intel_engine_id id;
-
-	if (IS_GEN6(dev_priv))
-		seq_printf(m, "GFX_MODE: 0x%08x\n", I915_READ(GFX_MODE));
-
-	for_each_engine(engine, dev_priv, id) {
-		seq_printf(m, "%s\n", engine->name);
-		if (IS_GEN7(dev_priv))
-			seq_printf(m, "GFX_MODE: 0x%08x\n",
-				   I915_READ(RING_MODE_GEN7(engine)));
-		seq_printf(m, "PP_DIR_BASE: 0x%08x\n",
-			   I915_READ(RING_PP_DIR_BASE(engine)));
-		seq_printf(m, "PP_DIR_BASE_READ: 0x%08x\n",
-			   I915_READ(RING_PP_DIR_BASE_READ(engine)));
-		seq_printf(m, "PP_DIR_DCLV: 0x%08x\n",
-			   I915_READ(RING_PP_DIR_DCLV(engine)));
-	}
-	if (dev_priv->mm.aliasing_ppgtt) {
-		struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt;
-
-		seq_puts(m, "aliasing PPGTT:\n");
-		seq_printf(m, "pd gtt offset: 0x%08x\n", ppgtt->pd.base.ggtt_offset);
-
-		ppgtt->debug_dump(ppgtt, m);
-	}
-
-	seq_printf(m, "ECOCHK: 0x%08x\n", I915_READ(GAM_ECOCHK));
-}
-
-static int i915_ppgtt_info(struct seq_file *m, void *data)
-{
-	struct drm_i915_private *dev_priv = node_to_i915(m->private);
-	struct drm_device *dev = &dev_priv->drm;
-	struct drm_file *file;
-	int ret;
-
-	mutex_lock(&dev->filelist_mutex);
-	ret = mutex_lock_interruptible(&dev->struct_mutex);
-	if (ret)
-		goto out_unlock;
-
-	intel_runtime_pm_get(dev_priv);
-
-	if (INTEL_GEN(dev_priv) >= 8)
-		gen8_ppgtt_info(m, dev_priv);
-	else if (INTEL_GEN(dev_priv) >= 6)
-		gen6_ppgtt_info(m, dev_priv);
-
-	list_for_each_entry_reverse(file, &dev->filelist, lhead) {
-		struct drm_i915_file_private *file_priv = file->driver_priv;
-		struct task_struct *task;
-
-		task = get_pid_task(file->pid, PIDTYPE_PID);
-		if (!task) {
-			ret = -ESRCH;
-			goto out_rpm;
-		}
-		seq_printf(m, "\nproc: %s\n", task->comm);
-		put_task_struct(task);
-		idr_for_each(&file_priv->context_idr, per_file_ctx,
-			     (void *)(unsigned long)m);
-	}
-
-out_rpm:
-	intel_runtime_pm_put(dev_priv);
-	mutex_unlock(&dev->struct_mutex);
-out_unlock:
-	mutex_unlock(&dev->filelist_mutex);
-	return ret;
-}
-
 static int count_irq_waiters(struct drm_i915_private *i915)
 {
 	struct intel_engine_cs *engine;
@@ -4707,7 +4589,6 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_context_status", i915_context_status, 0},
 	{"i915_forcewake_domains", i915_forcewake_domains, 0},
 	{"i915_swizzle_info", i915_swizzle_info, 0},
-	{"i915_ppgtt_info", i915_ppgtt_info, 0},
 	{"i915_llc", i915_llc, 0},
 	{"i915_edp_psr_status", i915_edp_psr_status, 0},
 	{"i915_energy_uJ", i915_energy_uJ, 0},
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915: Stop holding a ref to the ppgtt from each vma

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Stop holding a ref to the ppgtt from each vma
URL   : https://patchwork.freedesktop.org/series/48297/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
179bc3e01013 drm/i915: Stop holding a ref to the ppgtt from each vma
05f97b88f2b5 drm/i915: Report the number of closed vma held by each context in debugfs
-:43: WARNING:LONG_LINE: line over 100 characters
#43: FILE: drivers/gpu/drm/i915/i915_debugfs.c:350:
+		seq_printf(m, "%s: %lu objects, %llu bytes (%llu active, %llu inactive, %llu global, %llu shared, %llu unbound, %llu closed)\n", \

total: 0 errors, 1 warnings, 0 checks, 169 lines checked
589db09a6999 drm/i915: Remove debugfs/i915_ppgtt_info

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Stop holding a ref to the ppgtt from each vma

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Stop holding a ref to the ppgtt from each vma
URL   : https://patchwork.freedesktop.org/series/48297/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4676 -> Patchwork_9960 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48297/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9960 that come from known issues:

  === IGT changes ===

    ==== Possible fixes ====

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         DMESG-FAIL (fdo#107164) -> PASS

    igt@drv_selftest@live_hangcheck:
      fi-skl-guc:         DMESG-FAIL (fdo#107174) -> PASS
      fi-kbl-7567u:       DMESG-FAIL (fdo#106560, fdo#106947) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    
    ==== Warnings ====

    {igt@kms_psr@primary_page_flip}:
      fi-cnl-psr:         DMESG-WARN (fdo#107372) -> DMESG-FAIL (fdo#107372)

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372


== Participating hosts (53 -> 47) ==

  Missing    (6): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


== Build changes ==

    * Linux: CI_DRM_4676 -> Patchwork_9960

  CI_DRM_4676: 8171ee8227a2633ffb5808841f08cc1a3bfaffbb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4599: 940cb5f46433a8ae48d21c6672e4d8ecd1358bbf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9960: 589db09a699919150858461c3e2d6bf3e0f2bb42 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

589db09a6999 drm/i915: Remove debugfs/i915_ppgtt_info
05f97b88f2b5 drm/i915: Report the number of closed vma held by each context in debugfs
179bc3e01013 drm/i915: Stop holding a ref to the ppgtt from each vma

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9960/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915: Stop holding a ref to the ppgtt from each vma

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Stop holding a ref to the ppgtt from each vma
URL   : https://patchwork.freedesktop.org/series/48297/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4676_full -> Patchwork_9960_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_9960_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)

    
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4676 -> Patchwork_9960

  CI_DRM_4676: 8171ee8227a2633ffb5808841f08cc1a3bfaffbb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4599: 940cb5f46433a8ae48d21c6672e4d8ecd1358bbf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9960: 589db09a699919150858461c3e2d6bf3e0f2bb42 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9960/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Stop holding a ref to the ppgtt from each vma

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> The context owns both the ppgtt and the vma within it, and our activity
> tracking on the context ensures that we do not release active ppgtt. As
> the context fulfils our obligations for active memory tracking, we can
> relinquish the reference from the vma.
>

The part of owning the vma within it escapes me. The vma is tied
to the object. Is it about that active objects, with their vmas
hold the refs to the context they are executing on?

> This fixes a silly transient refleak from closed vma being kept alive
> until the entire system was idle, keeping all vm alive as well.
>
> Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Testcase: igt/gem_ctx_create/files
> Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_vma.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 274fd2a7bcb6..31efc971a3a8 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -199,7 +199,6 @@ vma_create(struct drm_i915_gem_object *obj,
>  		vma->flags |= I915_VMA_GGTT;
>  		list_add(&vma->obj_link, &obj->vma_list);
>  	} else {
> -		i915_ppgtt_get(i915_vm_to_ppgtt(vm));

It seems this is the sole user of i915_ppgtt_get so you can
remove that too.

-Mika


>  		list_add_tail(&vma->obj_link, &obj->vma_list);
>  	}
>  
> @@ -810,9 +809,6 @@ static void __i915_vma_destroy(struct i915_vma *vma)
>  	if (vma->obj)
>  		rb_erase(&vma->obj_node, &vma->obj->vma_tree);
>  
> -	if (!i915_vma_is_ggtt(vma))
> -		i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm));
> -
>  	rbtree_postorder_for_each_entry_safe(iter, n, &vma->active, node) {
>  		GEM_BUG_ON(i915_gem_active_isset(&iter->base));
>  		kfree(iter);
> -- 
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Stop holding a ref to the ppgtt from each vma

[Chris Wilson]

Quoting Mika Kuoppala (2018-08-16 12:42:08)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > The context owns both the ppgtt and the vma within it, and our activity
> > tracking on the context ensures that we do not release active ppgtt. As
> > the context fulfils our obligations for active memory tracking, we can
> > relinquish the reference from the vma.
> >
> 
> The part of owning the vma within it escapes me. The vma is tied
> to the object. Is it about that active objects, with their vmas
> hold the refs to the context they are executing on?

The vma belongs to the (object, context). If the context is closed (or
on close(/dev/dri/card0)), so is the ppgtt. Closing the object
(gem_close(fd, obj)) closes the vma that match the contexts for the fd,
and if it was the last handle, then frees the object.
 
> > This fixes a silly transient refleak from closed vma being kept alive
> > until the entire system was idle, keeping all vm alive as well.
> >
> > Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Testcase: igt/gem_ctx_create/files
> > Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_vma.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> > index 274fd2a7bcb6..31efc971a3a8 100644
> > --- a/drivers/gpu/drm/i915/i915_vma.c
> > +++ b/drivers/gpu/drm/i915/i915_vma.c
> > @@ -199,7 +199,6 @@ vma_create(struct drm_i915_gem_object *obj,
> >               vma->flags |= I915_VMA_GGTT;
> >               list_add(&vma->obj_link, &obj->vma_list);
> >       } else {
> > -             i915_ppgtt_get(i915_vm_to_ppgtt(vm));
> 
> It seems this is the sole user of i915_ppgtt_get so you can
> remove that too.

Such shortsightedness; it will reappear shortly. See the patches to
share vm between contexts.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Stop holding a ref to the ppgtt from each vma

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2018-08-16 12:42:08)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > The context owns both the ppgtt and the vma within it, and our activity
>> > tracking on the context ensures that we do not release active ppgtt. As
>> > the context fulfils our obligations for active memory tracking, we can
>> > relinquish the reference from the vma.
>> >
>> 
>> The part of owning the vma within it escapes me. The vma is tied
>> to the object. Is it about that active objects, with their vmas
>> hold the refs to the context they are executing on?
>
> The vma belongs to the (object, context). If the context is closed (or
> on close(/dev/dri/card0)), so is the ppgtt. Closing the object
> (gem_close(fd, obj)) closes the vma that match the contexts for the fd,
> and if it was the last handle, then frees the object.
>  
>> > This fixes a silly transient refleak from closed vma being kept alive
>> > until the entire system was idle, keeping all vm alive as well.
>> >
>> > Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > Testcase: igt/gem_ctx_create/files
>> > Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close")
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_vma.c | 4 ----
>> >  1 file changed, 4 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
>> > index 274fd2a7bcb6..31efc971a3a8 100644
>> > --- a/drivers/gpu/drm/i915/i915_vma.c
>> > +++ b/drivers/gpu/drm/i915/i915_vma.c
>> > @@ -199,7 +199,6 @@ vma_create(struct drm_i915_gem_object *obj,
>> >               vma->flags |= I915_VMA_GGTT;
>> >               list_add(&vma->obj_link, &obj->vma_list);
>> >       } else {
>> > -             i915_ppgtt_get(i915_vm_to_ppgtt(vm));
>> 
>> It seems this is the sole user of i915_ppgtt_get so you can
>> remove that too.
>
> Such shortsightedness; it will reappear shortly. See the patches to
> share vm between contexts.

Ok let it float in there for a while.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Also

Tested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

I also been after this one previously and managed to bring
up a testcase where we create a mapping to an object and
then create context, do execbuf with mapped object as relocatable,
and destroy context. That would also end up keeping ppgtts
alive until client closes.

Would it be worthwhile to include such test or do you think
gem_ctx_create@files covers enough?

-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Stop holding a ref to the ppgtt from each vma

[Chris Wilson]

Quoting Mika Kuoppala (2018-08-16 13:14:37)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Quoting Mika Kuoppala (2018-08-16 12:42:08)
> >> Chris Wilson <chris@chris-wilson.co.uk> writes:
> >> 
> >> > The context owns both the ppgtt and the vma within it, and our activity
> >> > tracking on the context ensures that we do not release active ppgtt. As
> >> > the context fulfils our obligations for active memory tracking, we can
> >> > relinquish the reference from the vma.
> >> >
> >> 
> >> The part of owning the vma within it escapes me. The vma is tied
> >> to the object. Is it about that active objects, with their vmas
> >> hold the refs to the context they are executing on?
> >
> > The vma belongs to the (object, context). If the context is closed (or
> > on close(/dev/dri/card0)), so is the ppgtt. Closing the object
> > (gem_close(fd, obj)) closes the vma that match the contexts for the fd,
> > and if it was the last handle, then frees the object.
> >  
> >> > This fixes a silly transient refleak from closed vma being kept alive
> >> > until the entire system was idle, keeping all vm alive as well.
> >> >
> >> > Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> > Testcase: igt/gem_ctx_create/files
> >> > Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close")
> >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_vma.c | 4 ----
> >> >  1 file changed, 4 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> >> > index 274fd2a7bcb6..31efc971a3a8 100644
> >> > --- a/drivers/gpu/drm/i915/i915_vma.c
> >> > +++ b/drivers/gpu/drm/i915/i915_vma.c
> >> > @@ -199,7 +199,6 @@ vma_create(struct drm_i915_gem_object *obj,
> >> >               vma->flags |= I915_VMA_GGTT;
> >> >               list_add(&vma->obj_link, &obj->vma_list);
> >> >       } else {
> >> > -             i915_ppgtt_get(i915_vm_to_ppgtt(vm));
> >> 
> >> It seems this is the sole user of i915_ppgtt_get so you can
> >> remove that too.
> >
> > Such shortsightedness; it will reappear shortly. See the patches to
> > share vm between contexts.
> 
> Ok let it float in there for a while.
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> 
> Also
> 
> Tested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> 
> I also been after this one previously and managed to bring
> up a testcase where we create a mapping to an object and
> then create context, do execbuf with mapped object as relocatable,
> and destroy context. That would also end up keeping ppgtts
> alive until client closes.
> 
> Would it be worthwhile to include such test or do you think
> gem_ctx_create@files covers enough?

There's no linkage between the mmap and context, so it doesn't strike me
as novel. gem_exec_reloc (and friends) include passing mmap(obj) as the
execbuf, objects and/or relocations, so that should be covered; and
gem_ctx_create covers using lots of temporary and long lived contexts.

We put previous bugs in gem_ppgtt, e.g. the vma-leak, but part of the
challenge there is that to stabilise the counters you need an idle
system.

However, bug discovery is serendipitous and minor differences between
similar tests (even doing the same setup in a different order) can be
all the difference between finding a bug and not. So don't let me stop
you...

You can use the igt_kcov tools to tell if a test case is similar to any
existing one.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Stop holding a ref to the ppgtt from each vma

[Chris Wilson]

Quoting Mika Kuoppala (2018-08-16 13:14:37)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Quoting Mika Kuoppala (2018-08-16 12:42:08)
> >> Chris Wilson <chris@chris-wilson.co.uk> writes:
> >> 
> >> > The context owns both the ppgtt and the vma within it, and our activity
> >> > tracking on the context ensures that we do not release active ppgtt. As
> >> > the context fulfils our obligations for active memory tracking, we can
> >> > relinquish the reference from the vma.
> >> >
> >> 
> >> The part of owning the vma within it escapes me. The vma is tied
> >> to the object. Is it about that active objects, with their vmas
> >> hold the refs to the context they are executing on?
> >
> > The vma belongs to the (object, context). If the context is closed (or
> > on close(/dev/dri/card0)), so is the ppgtt. Closing the object
> > (gem_close(fd, obj)) closes the vma that match the contexts for the fd,
> > and if it was the last handle, then frees the object.
> >  
> >> > This fixes a silly transient refleak from closed vma being kept alive
> >> > until the entire system was idle, keeping all vm alive as well.
> >> >
> >> > Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> > Testcase: igt/gem_ctx_create/files
> >> > Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close")
> >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_vma.c | 4 ----
> >> >  1 file changed, 4 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> >> > index 274fd2a7bcb6..31efc971a3a8 100644
> >> > --- a/drivers/gpu/drm/i915/i915_vma.c
> >> > +++ b/drivers/gpu/drm/i915/i915_vma.c
> >> > @@ -199,7 +199,6 @@ vma_create(struct drm_i915_gem_object *obj,
> >> >               vma->flags |= I915_VMA_GGTT;
> >> >               list_add(&vma->obj_link, &obj->vma_list);
> >> >       } else {
> >> > -             i915_ppgtt_get(i915_vm_to_ppgtt(vm));
> >> 
> >> It seems this is the sole user of i915_ppgtt_get so you can
> >> remove that too.
> >
> > Such shortsightedness; it will reappear shortly. See the patches to
> > share vm between contexts.
> 
> Ok let it float in there for a while.
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> 
> Also
> 
> Tested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Applied the bug fix, thank you for the review.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Report the number of closed vma held by each context in debugfs

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Include a count of closed vma when reporting the per_ctx_stats of
> debugfs/i915_gem_objects.
>

The code corresponds for you to report a count of bytes of a closed vmas
and I am not sure which one do you want.

-Mika


> Whilst adjusting the context tracking, note that we can simply use our
> list of contexts in i915->contexts rather than circumlocute via
> dev->filelist and the per-file context idr.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 113 +++++++++++-----------------
>  1 file changed, 42 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 26b7e5276b15..a05d4bbc0318 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -302,6 +302,7 @@ struct file_stats {
>  	u64 total, unbound;
>  	u64 global, shared;
>  	u64 active, inactive;
> +	u64 closed;
>  };
>  
>  static int per_file_stats(int id, void *ptr, void *data)
> @@ -336,6 +337,9 @@ static int per_file_stats(int id, void *ptr, void *data)
>  			stats->active += vma->node.size;
>  		else
>  			stats->inactive += vma->node.size;
> +
> +		if (i915_vma_is_closed(vma))
> +			stats->closed += vma->node.size;
>  	}
>  
>  	return 0;
> @@ -343,7 +347,7 @@ static int per_file_stats(int id, void *ptr, void *data)
>  
>  #define print_file_stats(m, name, stats) do { \
>  	if (stats.count) \
> -		seq_printf(m, "%s: %lu objects, %llu bytes (%llu active, %llu inactive, %llu global, %llu shared, %llu unbound)\n", \
> +		seq_printf(m, "%s: %lu objects, %llu bytes (%llu active, %llu inactive, %llu global, %llu shared, %llu unbound, %llu closed)\n", \
>  			   name, \
>  			   stats.count, \
>  			   stats.total, \
> @@ -351,7 +355,8 @@ static int per_file_stats(int id, void *ptr, void *data)
>  			   stats.inactive, \
>  			   stats.global, \
>  			   stats.shared, \
> -			   stats.unbound); \
> +			   stats.unbound, \
> +			   stats.closed); \
>  } while (0)
>  
>  static void print_batch_pool_stats(struct seq_file *m,
> @@ -377,44 +382,44 @@ static void print_batch_pool_stats(struct seq_file *m,
>  	print_file_stats(m, "[k]batch pool", stats);
>  }
>  
> -static int per_file_ctx_stats(int idx, void *ptr, void *data)
> +static void print_context_stats(struct seq_file *m,
> +				struct drm_i915_private *i915)
>  {
> -	struct i915_gem_context *ctx = ptr;
> -	struct intel_engine_cs *engine;
> -	enum intel_engine_id id;
> -
> -	for_each_engine(engine, ctx->i915, id) {
> -		struct intel_context *ce = to_intel_context(ctx, engine);
> +	struct file_stats kstats = {};
> +	struct i915_gem_context *ctx;
>  
> -		if (ce->state)
> -			per_file_stats(0, ce->state->obj, data);
> -		if (ce->ring)
> -			per_file_stats(0, ce->ring->vma->obj, data);
> -	}
> +	list_for_each_entry(ctx, &i915->contexts.list, link) {
> +		struct file_stats stats = { .file_priv = ctx->file_priv };
> +		struct intel_engine_cs *engine;
> +		enum intel_engine_id id;
>  
> -	return 0;
> -}
> +		for_each_engine(engine, i915, id) {
> +			struct intel_context *ce = to_intel_context(ctx, engine);
>  
> -static void print_context_stats(struct seq_file *m,
> -				struct drm_i915_private *dev_priv)
> -{
> -	struct drm_device *dev = &dev_priv->drm;
> -	struct file_stats stats;
> -	struct drm_file *file;
> +			if (ce->state)
> +				per_file_stats(0, ce->state->obj, &kstats);
> +			if (ce->ring)
> +				per_file_stats(0, ce->ring->vma->obj, &kstats);
> +		}
>  
> -	memset(&stats, 0, sizeof(stats));
> +		if (!IS_ERR_OR_NULL(stats.file_priv)) {
> +			struct drm_file *file = stats.file_priv->file;
> +			struct task_struct *task;
>  
> -	mutex_lock(&dev->struct_mutex);
> -	if (dev_priv->kernel_context)
> -		per_file_ctx_stats(0, dev_priv->kernel_context, &stats);
> +			spin_lock(&file->table_lock);
> +			idr_for_each(&file->object_idr, per_file_stats, &stats);
> +			spin_unlock(&file->table_lock);
>  
> -	list_for_each_entry(file, &dev->filelist, lhead) {
> -		struct drm_i915_file_private *fpriv = file->driver_priv;
> -		idr_for_each(&fpriv->context_idr, per_file_ctx_stats, &stats);
> +			rcu_read_lock();
> +			task = pid_task(ctx->pid ?: file->pid, PIDTYPE_PID);
> +			print_file_stats(m,
> +					 task ? task->comm : "<unknown>",
> +					 stats);
> +			rcu_read_unlock();
> +		}
>  	}
> -	mutex_unlock(&dev->struct_mutex);
>  
> -	print_file_stats(m, "[k]contexts", stats);
> +	print_file_stats(m, "[k]contexts", kstats);
>  }
>  
>  static int i915_gem_object_info(struct seq_file *m, void *data)
> @@ -426,14 +431,9 @@ static int i915_gem_object_info(struct seq_file *m, void *data)
>  	u64 size, mapped_size, purgeable_size, dpy_size, huge_size;
>  	struct drm_i915_gem_object *obj;
>  	unsigned int page_sizes = 0;
> -	struct drm_file *file;
>  	char buf[80];
>  	int ret;
>  
> -	ret = mutex_lock_interruptible(&dev->struct_mutex);
> -	if (ret)
> -		return ret;
> -
>  	seq_printf(m, "%u objects, %llu bytes\n",
>  		   dev_priv->mm.object_count,
>  		   dev_priv->mm.object_memory);
> @@ -514,43 +514,14 @@ static int i915_gem_object_info(struct seq_file *m, void *data)
>  					buf, sizeof(buf)));
>  
>  	seq_putc(m, '\n');
> -	print_batch_pool_stats(m, dev_priv);
> -	mutex_unlock(&dev->struct_mutex);
>  
> -	mutex_lock(&dev->filelist_mutex);
> -	print_context_stats(m, dev_priv);
> -	list_for_each_entry_reverse(file, &dev->filelist, lhead) {
> -		struct file_stats stats;
> -		struct drm_i915_file_private *file_priv = file->driver_priv;
> -		struct i915_request *request;
> -		struct task_struct *task;
> -
> -		mutex_lock(&dev->struct_mutex);
> -
> -		memset(&stats, 0, sizeof(stats));
> -		stats.file_priv = file->driver_priv;
> -		spin_lock(&file->table_lock);
> -		idr_for_each(&file->object_idr, per_file_stats, &stats);
> -		spin_unlock(&file->table_lock);
> -		/*
> -		 * Although we have a valid reference on file->pid, that does
> -		 * not guarantee that the task_struct who called get_pid() is
> -		 * still alive (e.g. get_pid(current) => fork() => exit()).
> -		 * Therefore, we need to protect this ->comm access using RCU.
> -		 */
> -		request = list_first_entry_or_null(&file_priv->mm.request_list,
> -						   struct i915_request,
> -						   client_link);
> -		rcu_read_lock();
> -		task = pid_task(request && request->gem_context->pid ?
> -				request->gem_context->pid : file->pid,
> -				PIDTYPE_PID);
> -		print_file_stats(m, task ? task->comm : "<unknown>", stats);
> -		rcu_read_unlock();
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
>  
> -		mutex_unlock(&dev->struct_mutex);
> -	}
> -	mutex_unlock(&dev->filelist_mutex);
> +	print_batch_pool_stats(m, dev_priv);
> +	print_context_stats(m, dev_priv);
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	return 0;
>  }
> -- 
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Report the number of closed vma held by each context in debugfs

[Chris Wilson]

Quoting Mika Kuoppala (2018-08-16 14:29:33)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Include a count of closed vma when reporting the per_ctx_stats of
> > debugfs/i915_gem_objects.
> >
> 
> The code corresponds for you to report a count of bytes of a closed vmas
> and I am not sure which one do you want.

Bytes, since its neighbours on that line are expressed in bytes and you
do want to know at a glance how much memory is being held.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx