[PATCH 1/5] drm/i915: Use fence_write() from rpm resume

[PATCH 1/5] drm/i915: Use fence_write() from rpm resume

[Chris Wilson]

During rpm resume we restore the fences, but we do not have the
protection of struct_mutex. This rules out updating the activity
tracking on the fences, and requires us to rely on the rpm as the
serialisation barrier instead.

[  350.298052] [drm:intel_runtime_resume [i915]] Resuming device
[  350.308606]
[  350.310520] ===============================
[  350.315560] [ INFO: suspicious RCU usage. ]
[  350.320554] 4.8.0-rc8-bsw-rapl+ #3133 Tainted: G     U  W
[  350.327208] -------------------------------
[  350.331977] ../drivers/gpu/drm/i915/i915_gem_request.h:371 suspicious rcu_dereference_protected() usage!
[  350.342619]
[  350.342619] other info that might help us debug this:
[  350.342619]
[  350.351593]
[  350.351593] rcu_scheduler_active = 1, debug_locks = 0
[  350.358952] 3 locks held by Xorg/320:
[  350.363077]  #0:  (&dev->mode_config.mutex){+.+.+.}, at: [<ffffffffa030589c>] drm_modeset_lock_all+0x3c/0xd0 [drm]
[  350.375162]  #1:  (crtc_ww_class_acquire){+.+.+.}, at: [<ffffffffa03058a6>] drm_modeset_lock_all+0x46/0xd0 [drm]
[  350.387022]  #2:  (crtc_ww_class_mutex){+.+.+.}, at: [<ffffffffa0305056>] drm_modeset_lock+0x36/0x110 [drm]
[  350.398236]
[  350.398236] stack backtrace:
[  350.403196] CPU: 1 PID: 320 Comm: Xorg Tainted: G     U  W       4.8.0-rc8-bsw-rapl+ #3133
[  350.412457] Hardware name: Intel Corporation CHERRYVIEW C0 PLATFORM/Braswell CRB, BIOS BRAS.X64.X088.R00.1510270350 10/27/2015
[  350.425212]  0000000000000000 ffff8801680a78c8 ffffffff81332187 ffff88016c5c5000
[  350.433611]  0000000000000001 ffff8801680a78f8 ffffffff810ca6da ffff88016cc8b0f0
[  350.442012]  ffff88016cc80000 ffff88016cc80000 ffff880177ad0000 ffff8801680a7948
[  350.450409] Call Trace:
[  350.453165]  [<ffffffff81332187>] dump_stack+0x67/0x90
[  350.458931]  [<ffffffff810ca6da>] lockdep_rcu_suspicious+0xea/0x120
[  350.466002]  [<ffffffffa039e8dd>] fence_update+0xbd/0x670 [i915]
[  350.472766]  [<ffffffffa039efe2>] i915_gem_restore_fences+0x52/0x70 [i915]
[  350.480496]  [<ffffffffa0368f42>] vlv_resume_prepare+0x72/0x570 [i915]
[  350.487839]  [<ffffffffa0369802>] intel_runtime_resume+0x102/0x210 [i915]
[  350.495442]  [<ffffffff8137f26f>] pci_pm_runtime_resume+0x7f/0xb0
[  350.502274]  [<ffffffff8137f1f0>] ? pci_restore_standard_config+0x40/0x40
[  350.509883]  [<ffffffff814401c5>] __rpm_callback+0x35/0x70
[  350.516037]  [<ffffffff8137f1f0>] ? pci_restore_standard_config+0x40/0x40
[  350.523646]  [<ffffffff81440224>] rpm_callback+0x24/0x80
[  350.529604]  [<ffffffff8137f1f0>] ? pci_restore_standard_config+0x40/0x40
[  350.537212]  [<ffffffff814417bd>] rpm_resume+0x4ad/0x740
[  350.543161]  [<ffffffff81441aa1>] __pm_runtime_resume+0x51/0x80
[  350.549824]  [<ffffffffa03889c8>] intel_runtime_pm_get+0x28/0x90 [i915]
[  350.557265]  [<ffffffffa0388a53>] intel_display_power_get+0x23/0x50 [i915]
[  350.565001]  [<ffffffffa03ef23d>] intel_atomic_commit_tail+0xdfd/0x10b0 [i915]
[  350.573106]  [<ffffffffa034b2e9>] ? drm_atomic_helper_swap_state+0x159/0x300 [drm_kms_helper]
[  350.582659]  [<ffffffff81615091>] ? _raw_spin_unlock+0x31/0x50
[  350.589205]  [<ffffffffa034b2e9>] ? drm_atomic_helper_swap_state+0x159/0x300 [drm_kms_helper]
[  350.598787]  [<ffffffffa03ef8a5>] intel_atomic_commit+0x3b5/0x500 [i915]
[  350.606319]  [<ffffffffa03061dc>] ? drm_atomic_set_crtc_for_connector+0xcc/0x100 [drm]
[  350.615209]  [<ffffffffa0306b49>] drm_atomic_commit+0x49/0x50 [drm]
[  350.622242]  [<ffffffffa034dee8>] drm_atomic_helper_set_config+0x88/0xc0 [drm_kms_helper]
[  350.631419]  [<ffffffffa02f94ac>] drm_mode_set_config_internal+0x6c/0x120 [drm]
[  350.639623]  [<ffffffffa02fa94c>] drm_mode_setcrtc+0x22c/0x4d0 [drm]
[  350.646760]  [<ffffffffa02f0f19>] drm_ioctl+0x209/0x460 [drm]
[  350.653217]  [<ffffffffa02fa720>] ? drm_mode_getcrtc+0x150/0x150 [drm]
[  350.660536]  [<ffffffff810c984a>] ? __lock_is_held+0x4a/0x70
[  350.666885]  [<ffffffff81202303>] do_vfs_ioctl+0x93/0x6b0
[  350.672939]  [<ffffffff8120f843>] ? __fget+0x113/0x200
[  350.678797]  [<ffffffff8120f735>] ? __fget+0x5/0x200
[  350.684361]  [<ffffffff81202964>] SyS_ioctl+0x44/0x80
[  350.690030]  [<ffffffff81001deb>] do_syscall_64+0x5b/0x120
[  350.696184]  [<ffffffff81615ada>] entry_SYSCALL64_slow_path+0x25/0x25

Note we also have to remember the lesson from commit 4fc788f5ee3d
("drm/i915: Flush delayed fence releases after reset") where we have to
flush any changes to the fence on restore.

v2: Replace call to release user mmaps with an assertion that they have
already been zapped.

Fixes: 49ef5294cda2 ("drm/i915: Move fence tracking from object to vma")
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_fence.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_fence.c b/drivers/gpu/drm/i915/i915_gem_fence.c
index 8df1fa7234e8..f7081f4b5d22 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence.c
@@ -290,6 +290,8 @@ i915_vma_put_fence(struct i915_vma *vma)
 {
 	struct drm_i915_fence_reg *fence = vma->fence;
 
+	assert_rpm_wakelock_held(to_i915(vma->vm->dev));
+
 	if (!fence)
 		return 0;
 
@@ -341,6 +343,8 @@ i915_vma_get_fence(struct i915_vma *vma)
 	struct drm_i915_fence_reg *fence;
 	struct i915_vma *set = i915_gem_object_is_tiled(vma->obj) ? vma : NULL;
 
+	assert_rpm_wakelock_held(to_i915(vma->vm->dev));
+
 	/* Just update our place in the LRU if our fence is getting reused. */
 	if (vma->fence) {
 		fence = vma->fence;
@@ -371,6 +375,12 @@ void i915_gem_restore_fences(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	int i;
 
+	/* Note that this may be called outside of struct_mutex, by
+	 * runtime suspend/resume. The barrier we require is enforced by
+	 * rpm itself - all access to fences/GTT are only within an rpm
+	 * wakeref, and to acquire that wakeref you must pass through here.
+	 */
+
 	for (i = 0; i < dev_priv->num_fence_regs; i++) {
 		struct drm_i915_fence_reg *reg = &dev_priv->fence_regs[i];
 		struct i915_vma *vma = reg->vma;
@@ -379,10 +389,17 @@ void i915_gem_restore_fences(struct drm_device *dev)
 		 * Commit delayed tiling changes if we have an object still
 		 * attached to the fence, otherwise just clear the fence.
 		 */
-		if (vma && !i915_gem_object_is_tiled(vma->obj))
+		if (vma && !i915_gem_object_is_tiled(vma->obj)) {
+			GEM_BUG_ON(!reg->dirty);
+			GEM_BUG_ON(!list_empty(&vma->obj->userfault_link));
+
+			list_move(&reg->link, &dev_priv->mm.fence_list);
+			vma->fence = NULL;
 			vma = NULL;
+		}
 
-		fence_update(reg, vma);
+		fence_write(reg, vma);
+		reg->vma = vma;
 	}
 }
 
-- 
2.9.3

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

[PATCH 2/5] drm/i915: Update debugfs describe_obj() to show fault-mappable

[Chris Wilson]

The current meaning of whether an object has a GGTT vma is very
ill-defined (and note we don't check for any partials either), it just
means that at some point it was in the GGTT but it may not be now. The
information we really care about here is whether it is taking up
precious mappable aperture space. This is the obj->fault_mappable flag.
We have a redundant long form reprinting of this information, so remove
that in favour of the compact flag.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 358663e833d6..2e312e0f2670 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -107,7 +107,7 @@ static char get_tiling_flag(struct drm_i915_gem_object *obj)
 
 static char get_global_flag(struct drm_i915_gem_object *obj)
 {
-	return i915_gem_object_to_ggtt(obj, NULL) ?  'g' : ' ';
+	return obj->fault_mappable ? 'g' : ' ';
 }
 
 static char get_pin_mapped_flag(struct drm_i915_gem_object *obj)
@@ -186,15 +186,6 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 	}
 	if (obj->stolen)
 		seq_printf(m, " (stolen: %08llx)", obj->stolen->start);
-	if (obj->pin_display || obj->fault_mappable) {
-		char s[3], *t = s;
-		if (obj->pin_display)
-			*t++ = 'p';
-		if (obj->fault_mappable)
-			*t++ = 'f';
-		*t = '\0';
-		seq_printf(m, " (%s mappable)", s);
-	}
 
 	engine = i915_gem_active_get_engine(&obj->last_write,
 					    &dev_priv->drm.struct_mutex);
-- 
2.9.3

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

[PATCH 3/5] drm/i915: Move user fault tracking to a separate list

[Chris Wilson]

We want to decouple RPM and struct_mutex, but currently RPM has to walk
the list of bound objects and remove userspace mmapping before we
suspend (otherwise userspace may continue to access the GTT whilst it is
powered down). This currently requires the struct_mutex to walk the
bound_list, but if we move that to a separate list and lock we can take
the first step towards removing the struct_mutex.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c   |  2 +-
 drivers/gpu/drm/i915/i915_drv.h       | 20 +++++++++++-------
 drivers/gpu/drm/i915/i915_gem.c       | 39 ++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/i915_gem_evict.c |  2 +-
 4 files changed, 47 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 2e312e0f2670..0ce135dd80fb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -107,7 +107,7 @@ static char get_tiling_flag(struct drm_i915_gem_object *obj)
 
 static char get_global_flag(struct drm_i915_gem_object *obj)
 {
-	return obj->fault_mappable ? 'g' : ' ';
+	return !list_empty(&obj->userfault_link) ? 'g' : ' ';
 }
 
 static char get_pin_mapped_flag(struct drm_i915_gem_object *obj)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bf397b643cc0..72b3126c6c74 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1359,6 +1359,14 @@ struct i915_gem_mm {
 	 */
 	struct list_head unbound_list;
 
+	/** Protects access to the userfault_list */
+	spinlock_t userfault_lock;
+
+	/** List of all objects in gtt_space, currently mmaped by userspace.
+	 * All objects within this list must also be on bound_list.
+	 */
+	struct list_head userfault_list;
+
 	/** Usable portion of the GTT for GEM */
 	unsigned long stolen_base; /* limited to low memory (32-bit) */
 
@@ -2215,6 +2223,11 @@ struct drm_i915_gem_object {
 	struct drm_mm_node *stolen;
 	struct list_head global_list;
 
+	/**
+	 * Whether the object is currently in the GGTT mmap.
+	 */
+	struct list_head userfault_link;
+
 	/** Used in execbuf to temporarily hold a ref */
 	struct list_head obj_exec_link;
 
@@ -2242,13 +2255,6 @@ struct drm_i915_gem_object {
 	 */
 	unsigned int madv:2;
 
-	/**
-	 * Whether the current gtt mapping needs to be mappable (and isn't just
-	 * mappable by accident). Track pin and fault separate for a more
-	 * accurate mappable working set.
-	 */
-	unsigned int fault_mappable:1;
-
 	/*
 	 * Is the object to be mapped as read-only to the GPU
 	 * Only honoured if hardware has relevant pte bit
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fdd496e6c081..82e05ff6295d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1850,7 +1850,11 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf)
 	if (ret)
 		goto err_unpin;
 
-	obj->fault_mappable = true;
+	spin_lock(&dev_priv->mm.userfault_lock);
+	if (list_empty(&obj->userfault_link))
+		list_add(&obj->userfault_link, &dev_priv->mm.userfault_list);
+	spin_unlock(&dev_priv->mm.userfault_lock);
+
 err_unpin:
 	__i915_vma_unpin(vma);
 err_unlock:
@@ -1918,13 +1922,22 @@ err:
 void
 i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 {
+	struct drm_i915_private *i915 = to_i915(obj->base.dev);
+	bool zap = false;
+
 	/* Serialisation between user GTT access and our code depends upon
 	 * revoking the CPU's PTE whilst the mutex is held. The next user
 	 * pagefault then has to wait until we release the mutex.
 	 */
-	lockdep_assert_held(&obj->base.dev->struct_mutex);
+	lockdep_assert_held(&i915->drm.struct_mutex);
 
-	if (!obj->fault_mappable)
+	spin_lock(&i915->mm.userfault_lock);
+	if (!list_empty(&obj->userfault_link)) {
+		list_del_init(&obj->userfault_link);
+		zap = true;
+	}
+	spin_unlock(&i915->mm.userfault_lock);
+	if (!zap)
 		return;
 
 	drm_vma_node_unmap(&obj->base.vma_node,
@@ -1938,8 +1951,6 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 	 * memory writes before touching registers / GSM.
 	 */
 	wmb();
-
-	obj->fault_mappable = false;
 }
 
 void
@@ -1947,8 +1958,19 @@ i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv)
 {
 	struct drm_i915_gem_object *obj;
 
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
-		i915_gem_release_mmap(obj);
+	spin_lock(&dev_priv->mm.userfault_lock);
+	while ((obj = list_first_entry_or_null(&dev_priv->mm.userfault_list,
+					       struct drm_i915_gem_object,
+					       userfault_link))) {
+		list_del_init(&obj->userfault_link);
+		spin_unlock(&dev_priv->mm.userfault_lock);
+
+		drm_vma_node_unmap(&obj->base.vma_node,
+				   obj->base.dev->anon_inode->i_mapping);
+
+		spin_lock(&dev_priv->mm.userfault_lock);
+	}
+	spin_unlock(&dev_priv->mm.userfault_lock);
 }
 
 /**
@@ -4066,6 +4088,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
 	int i;
 
 	INIT_LIST_HEAD(&obj->global_list);
+	INIT_LIST_HEAD(&obj->userfault_link);
 	for (i = 0; i < I915_NUM_ENGINES; i++)
 		init_request_active(&obj->last_read[i],
 				    i915_gem_object_retire__read);
@@ -4441,6 +4464,7 @@ int i915_gem_init(struct drm_device *dev)
 	int ret;
 
 	mutex_lock(&dev->struct_mutex);
+	spin_lock_init(&dev_priv->mm.userfault_lock);
 
 	if (!i915.enable_execlists) {
 		dev_priv->gt.resume = intel_legacy_submission_resume;
@@ -4566,6 +4590,7 @@ i915_gem_load_init(struct drm_device *dev)
 	INIT_LIST_HEAD(&dev_priv->mm.unbound_list);
 	INIT_LIST_HEAD(&dev_priv->mm.bound_list);
 	INIT_LIST_HEAD(&dev_priv->mm.fence_list);
+	INIT_LIST_HEAD(&dev_priv->mm.userfault_list);
 	for (i = 0; i < I915_NUM_ENGINES; i++)
 		init_engine_lists(&dev_priv->engine[i]);
 	INIT_DELAYED_WORK(&dev_priv->gt.retire_work,
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index 5b6f81c1dbca..ca175c3063ed 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -55,7 +55,7 @@ mark_free(struct i915_vma *vma, unsigned int flags, struct list_head *unwind)
 	if (WARN_ON(!list_empty(&vma->exec_list)))
 		return false;
 
-	if (flags & PIN_NONFAULT && vma->obj->fault_mappable)
+	if (flags & PIN_NONFAULT && !list_empty(&vma->obj->userfault_link))
 		return false;
 
 	list_add(&vma->exec_list, unwind);
-- 
2.9.3

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

[PATCH 4/5] drm/i915: Use RPM as the barrier for controlling user mmap access

[Chris Wilson]

We can remove the false coupling between RPM and struct mutex by the
observation that we can use the RPM wakeref as the barrier around user
mmap access. That is as we tear down the user's PTE atomically from
within rpm suspend and then to fault in new PTE requires the rpm
wakeref, means that no user access is possible through those PTE without
RPM being awake. Having made that observation, we can then remove the
presumption of having to take rpm outside of struct_mutex and so allow
fine grained acquisition of a wakeref around hw access rather than
having to remember to acquire the wakeref early on.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c    | 22 ----------------------
 drivers/gpu/drm/i915/i915_drv.c        | 19 -------------------
 drivers/gpu/drm/i915/i915_gem.c        | 22 +++++++++++++---------
 drivers/gpu/drm/i915/i915_gem_gtt.c    | 17 +++++++++++++----
 drivers/gpu/drm/i915/i915_gem_tiling.c |  4 ----
 drivers/gpu/drm/i915/intel_uncore.c    |  6 +++---
 6 files changed, 29 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0ce135dd80fb..503d56b6faef 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1391,14 +1391,9 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
 static int ironlake_drpc_info(struct seq_file *m)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
-	struct drm_device *dev = &dev_priv->drm;
 	u32 rgvmodectl, rstdbyctl;
 	u16 crstandvid;
-	int ret;
 
-	ret = mutex_lock_interruptible(&dev->struct_mutex);
-	if (ret)
-		return ret;
 	intel_runtime_pm_get(dev_priv);
 
 	rgvmodectl = I915_READ(MEMMODECTL);
@@ -1406,7 +1401,6 @@ static int ironlake_drpc_info(struct seq_file *m)
 	crstandvid = I915_READ16(CRSTANDVID);
 
 	intel_runtime_pm_put(dev_priv);
-	mutex_unlock(&dev->struct_mutex);
 
 	seq_printf(m, "HD boost: %s\n", yesno(rgvmodectl & MEMMODE_BOOST_EN));
 	seq_printf(m, "Boost freq: %d\n",
@@ -2084,12 +2078,7 @@ static const char *swizzle_string(unsigned swizzle)
 static int i915_swizzle_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;
-	int ret;
 
-	ret = mutex_lock_interruptible(&dev->struct_mutex);
-	if (ret)
-		return ret;
 	intel_runtime_pm_get(dev_priv);
 
 	seq_printf(m, "bit6 swizzle for X-tiling = %s\n",
@@ -2129,7 +2118,6 @@ static int i915_swizzle_info(struct seq_file *m, void *data)
 		seq_puts(m, "L-shaped memory detected\n");
 
 	intel_runtime_pm_put(dev_priv);
-	mutex_unlock(&dev->struct_mutex);
 
 	return 0;
 }
@@ -4788,13 +4776,9 @@ i915_wedged_set(void *data, u64 val)
 	if (i915_reset_in_progress(&dev_priv->gpu_error))
 		return -EAGAIN;
 
-	intel_runtime_pm_get(dev_priv);
-
 	i915_handle_error(dev_priv, val,
 			  "Manually setting wedged to %llu", val);
 
-	intel_runtime_pm_put(dev_priv);
-
 	return 0;
 }
 
@@ -5029,22 +5013,16 @@ static int
 i915_cache_sharing_get(void *data, u64 *val)
 {
 	struct drm_i915_private *dev_priv = data;
-	struct drm_device *dev = &dev_priv->drm;
 	u32 snpcr;
-	int ret;
 
 	if (!(IS_GEN6(dev_priv) || IS_GEN7(dev_priv)))
 		return -ENODEV;
 
-	ret = mutex_lock_interruptible(&dev->struct_mutex);
-	if (ret)
-		return ret;
 	intel_runtime_pm_get(dev_priv);
 
 	snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
 
 	intel_runtime_pm_put(dev_priv);
-	mutex_unlock(&dev->struct_mutex);
 
 	*val = (snpcr & GEN6_MBC_SNPCR_MASK) >> GEN6_MBC_SNPCR_SHIFT;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 89d322215c84..31eee32fcf6d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2313,24 +2313,6 @@ static int intel_runtime_suspend(struct device *kdev)
 
 	DRM_DEBUG_KMS("Suspending device\n");
 
-	/*
-	 * We could deadlock here in case another thread holding struct_mutex
-	 * calls RPM suspend concurrently, since the RPM suspend will wait
-	 * first for this RPM suspend to finish. In this case the concurrent
-	 * RPM resume will be followed by its RPM suspend counterpart. Still
-	 * for consistency return -EAGAIN, which will reschedule this suspend.
-	 */
-	if (!mutex_trylock(&dev->struct_mutex)) {
-		DRM_DEBUG_KMS("device lock contention, deffering suspend\n");
-		/*
-		 * Bump the expiration timestamp, otherwise the suspend won't
-		 * be rescheduled.
-		 */
-		pm_runtime_mark_last_busy(kdev);
-
-		return -EAGAIN;
-	}
-
 	disable_rpm_wakeref_asserts(dev_priv);
 
 	/*
@@ -2338,7 +2320,6 @@ static int intel_runtime_suspend(struct device *kdev)
 	 * an RPM reference.
 	 */
 	i915_gem_release_all_mmaps(dev_priv);
-	mutex_unlock(&dev->struct_mutex);
 
 	intel_guc_suspend(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 82e05ff6295d..318501bb268c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1469,7 +1469,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 	 */
 	if (!i915_gem_object_has_struct_page(obj) ||
 	    cpu_write_needs_clflush(obj)) {
+		intel_runtime_pm_get(dev_priv);
 		ret = i915_gem_gtt_pwrite_fast(dev_priv, obj, args, file);
+		intel_runtime_pm_put(dev_priv);
 		/* Note that the gtt paths might fail with non-page-backed user
 		 * pointers (e.g. gtt mappings when moving data between
 		 * textures). Fallback to the shmem path in that case. */
@@ -1850,6 +1852,7 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf)
 	if (ret)
 		goto err_unpin;
 
+	assert_rpm_wakelock_held(dev_priv);
 	spin_lock(&dev_priv->mm.userfault_lock);
 	if (list_empty(&obj->userfault_link))
 		list_add(&obj->userfault_link, &dev_priv->mm.userfault_list);
@@ -1928,8 +1931,13 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 	/* Serialisation between user GTT access and our code depends upon
 	 * revoking the CPU's PTE whilst the mutex is held. The next user
 	 * pagefault then has to wait until we release the mutex.
+	 *
+	 * Note that RPM complicates somewhat by adding an additional
+	 * requirement that operations to the GGTT be made holding the RPM
+	 * wakeref.
 	 */
 	lockdep_assert_held(&i915->drm.struct_mutex);
+	intel_runtime_pm_get(i915);
 
 	spin_lock(&i915->mm.userfault_lock);
 	if (!list_empty(&obj->userfault_link)) {
@@ -1938,7 +1946,7 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 	}
 	spin_unlock(&i915->mm.userfault_lock);
 	if (!zap)
-		return;
+		goto out;
 
 	drm_vma_node_unmap(&obj->base.vma_node,
 			   obj->base.dev->anon_inode->i_mapping);
@@ -1951,6 +1959,9 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 	 * memory writes before touching registers / GSM.
 	 */
 	wmb();
+
+out:
+	intel_runtime_pm_put(i915);
 }
 
 void
@@ -3472,7 +3483,6 @@ int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data,
 int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
 			       struct drm_file *file)
 {
-	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_i915_gem_caching *args = data;
 	struct drm_i915_gem_object *obj;
 	enum i915_cache_level level;
@@ -3501,11 +3511,9 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	intel_runtime_pm_get(dev_priv);
-
 	ret = i915_mutex_lock_interruptible(dev);
 	if (ret)
-		goto rpm_put;
+		return ret;
 
 	obj = i915_gem_object_lookup(file, args->handle);
 	if (!obj) {
@@ -3514,13 +3522,9 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
 	}
 
 	ret = i915_gem_object_set_cache_level(obj, level);
-
 	i915_gem_object_put(obj);
 unlock:
 	mutex_unlock(&dev->struct_mutex);
-rpm_put:
-	intel_runtime_pm_put(dev_priv);
-
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 2d846aa39ca5..c0a4ba478309 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2596,6 +2596,7 @@ static int ggtt_bind_vma(struct i915_vma *vma,
 			 enum i915_cache_level cache_level,
 			 u32 flags)
 {
+	struct drm_i915_private *i915 = to_i915(vma->vm->dev);
 	struct drm_i915_gem_object *obj = vma->obj;
 	u32 pte_flags = 0;
 	int ret;
@@ -2608,8 +2609,10 @@ static int ggtt_bind_vma(struct i915_vma *vma,
 	if (obj->gt_ro)
 		pte_flags |= PTE_READ_ONLY;
 
+	intel_runtime_pm_get(i915);
 	vma->vm->insert_entries(vma->vm, vma->pages, vma->node.start,
 				cache_level, pte_flags);
+	intel_runtime_pm_put(i915);
 
 	/*
 	 * Without aliasing PPGTT there's no difference between
@@ -2625,6 +2628,7 @@ static int aliasing_gtt_bind_vma(struct i915_vma *vma,
 				 enum i915_cache_level cache_level,
 				 u32 flags)
 {
+	struct drm_i915_private *i915 = to_i915(vma->vm->dev);
 	u32 pte_flags;
 	int ret;
 
@@ -2639,14 +2643,15 @@ static int aliasing_gtt_bind_vma(struct i915_vma *vma,
 
 
 	if (flags & I915_VMA_GLOBAL_BIND) {
+		intel_runtime_pm_get(i915);
 		vma->vm->insert_entries(vma->vm,
 					vma->pages, vma->node.start,
 					cache_level, pte_flags);
+		intel_runtime_pm_put(i915);
 	}
 
 	if (flags & I915_VMA_LOCAL_BIND) {
-		struct i915_hw_ppgtt *appgtt =
-			to_i915(vma->vm->dev)->mm.aliasing_ppgtt;
+		struct i915_hw_ppgtt *appgtt = i915->mm.aliasing_ppgtt;
 		appgtt->base.insert_entries(&appgtt->base,
 					    vma->pages, vma->node.start,
 					    cache_level, pte_flags);
@@ -2657,13 +2662,17 @@ static int aliasing_gtt_bind_vma(struct i915_vma *vma,
 
 static void ggtt_unbind_vma(struct i915_vma *vma)
 {
-	struct i915_hw_ppgtt *appgtt = to_i915(vma->vm->dev)->mm.aliasing_ppgtt;
+	struct drm_i915_private *i915 = to_i915(vma->vm->dev);
+	struct i915_hw_ppgtt *appgtt = i915->mm.aliasing_ppgtt;
 	const u64 size = min(vma->size, vma->node.size);
 
-	if (vma->flags & I915_VMA_GLOBAL_BIND)
+	if (vma->flags & I915_VMA_GLOBAL_BIND) {
+		intel_runtime_pm_get(i915);
 		vma->vm->clear_range(vma->vm,
 				     vma->node.start, size,
 				     true);
+		intel_runtime_pm_put(i915);
+	}
 
 	if (vma->flags & I915_VMA_LOCAL_BIND && appgtt)
 		appgtt->base.clear_range(&appgtt->base,
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index a14b1e3d4c78..08f796a4f5f6 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -204,8 +204,6 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	intel_runtime_pm_get(dev_priv);
-
 	mutex_lock(&dev->struct_mutex);
 	if (obj->pin_display || obj->framebuffer_references) {
 		err = -EBUSY;
@@ -301,8 +299,6 @@ err:
 	i915_gem_object_put(obj);
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_runtime_pm_put(dev_priv);
-
 	return err;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index e2b188dcf908..e0c3bd941e38 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1364,7 +1364,7 @@ int i915_reg_read_ioctl(struct drm_device *dev,
 	struct register_whitelist const *entry = whitelist;
 	unsigned size;
 	i915_reg_t offset_ldw, offset_udw;
-	int i, ret = 0;
+	int i, ret;
 
 	for (i = 0; i < ARRAY_SIZE(whitelist); i++, entry++) {
 		if (i915_mmio_reg_offset(entry->offset_ldw) == (reg->offset & -entry->size) &&
@@ -1386,6 +1386,7 @@ int i915_reg_read_ioctl(struct drm_device *dev,
 
 	intel_runtime_pm_get(dev_priv);
 
+	ret = 0;
 	switch (size) {
 	case 8 | 1:
 		reg->val = I915_READ64_2x32(offset_ldw, offset_udw);
@@ -1404,10 +1405,9 @@ int i915_reg_read_ioctl(struct drm_device *dev,
 		break;
 	default:
 		ret = -EINVAL;
-		goto out;
+		break;
 	}
 
-out:
 	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
-- 
2.9.3

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

[PATCH 5/5] drm/i915: Remove superfluous locking around userfault_list

[Chris Wilson]

Now that we have reduced the access to the list to either (a) under the
struct_mutex whilst holding the RPM wakeref (so that concurrent writers to
the list are serialised by struct_mutex) and (b) under the atomic
runtime suspend (which cannot run concurrently with any other accessor due
to the atomic nature of the runtime suspend) we can remove the extra
locking around the list itself.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h |  3 ---
 drivers/gpu/drm/i915/i915_gem.c | 33 ++++++++++++---------------------
 2 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 72b3126c6c74..13ca968a760a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1359,9 +1359,6 @@ struct i915_gem_mm {
 	 */
 	struct list_head unbound_list;
 
-	/** Protects access to the userfault_list */
-	spinlock_t userfault_lock;
-
 	/** List of all objects in gtt_space, currently mmaped by userspace.
 	 * All objects within this list must also be on bound_list.
 	 */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 318501bb268c..b573a50148cb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1853,10 +1853,8 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf)
 		goto err_unpin;
 
 	assert_rpm_wakelock_held(dev_priv);
-	spin_lock(&dev_priv->mm.userfault_lock);
 	if (list_empty(&obj->userfault_link))
 		list_add(&obj->userfault_link, &dev_priv->mm.userfault_list);
-	spin_unlock(&dev_priv->mm.userfault_lock);
 
 err_unpin:
 	__i915_vma_unpin(vma);
@@ -1926,7 +1924,6 @@ void
 i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 {
 	struct drm_i915_private *i915 = to_i915(obj->base.dev);
-	bool zap = false;
 
 	/* Serialisation between user GTT access and our code depends upon
 	 * revoking the CPU's PTE whilst the mutex is held. The next user
@@ -1939,15 +1936,10 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 	lockdep_assert_held(&i915->drm.struct_mutex);
 	intel_runtime_pm_get(i915);
 
-	spin_lock(&i915->mm.userfault_lock);
-	if (!list_empty(&obj->userfault_link)) {
-		list_del_init(&obj->userfault_link);
-		zap = true;
-	}
-	spin_unlock(&i915->mm.userfault_lock);
-	if (!zap)
+	if (list_empty(&obj->userfault_link))
 		goto out;
 
+	list_del_init(&obj->userfault_link);
 	drm_vma_node_unmap(&obj->base.vma_node,
 			   obj->base.dev->anon_inode->i_mapping);
 
@@ -1967,21 +1959,21 @@ out:
 void
 i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_gem_object *obj;
+	struct drm_i915_gem_object *obj, *on;
 
-	spin_lock(&dev_priv->mm.userfault_lock);
-	while ((obj = list_first_entry_or_null(&dev_priv->mm.userfault_list,
-					       struct drm_i915_gem_object,
-					       userfault_link))) {
-		list_del_init(&obj->userfault_link);
-		spin_unlock(&dev_priv->mm.userfault_lock);
+	/*
+	 * Only called during RPM suspend. All users of the userfault_list
+	 * must be holding an RPM wakeref to ensure that this can not
+	 * run concurrently with themselves (and use the struct_mutex for
+	 * protection between themselves).
+	 */
 
+	list_for_each_entry_safe(obj, on,
+				 &dev_priv->mm.userfault_list, userfault_link) {
+		list_del_init(&obj->userfault_link);
 		drm_vma_node_unmap(&obj->base.vma_node,
 				   obj->base.dev->anon_inode->i_mapping);
-
-		spin_lock(&dev_priv->mm.userfault_lock);
 	}
-	spin_unlock(&dev_priv->mm.userfault_lock);
 }
 
 /**
@@ -4468,7 +4460,6 @@ int i915_gem_init(struct drm_device *dev)
 	int ret;
 
 	mutex_lock(&dev->struct_mutex);
-	spin_lock_init(&dev_priv->mm.userfault_lock);
 
 	if (!i915.enable_execlists) {
 		dev_priv->gt.resume = intel_legacy_submission_resume;
-- 
2.9.3

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

Re: [PATCH 1/5] drm/i915: Use fence_write() from rpm resume

[Joonas Lahtinen]

This was already;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Update debugfs describe_obj() to show fault-mappable

[Joonas Lahtinen]

On ke, 2016-10-12 at 12:16 +0100, Chris Wilson wrote:
> The current meaning of whether an object has a GGTT vma is very
> ill-defined (and note we don't check for any partials either), it just
> means that at some point it was in the GGTT but it may not be now. The
> information we really care about here is whether it is taking up
> precious mappable aperture space. This is the obj->fault_mappable flag.
> We have a redundant long form reprinting of this information, so remove
> that in favour of the compact flag.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

I'm sure I already reviewed this as a part of another patch, but better
as a separate one!

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for series starting with [1/5] drm/i915: Use fence_write() from rpm resume

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915: Use fence_write() from rpm resume
URL   : https://patchwork.freedesktop.org/series/13636/
State : warning

== Summary ==

Series 13636v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/13636/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-skl-6700k)
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-byt-j1900)
Test vgem_basic:
        Subgroup unload:
                skip       -> PASS       (fi-hsw-4770)

fi-bdw-5557u     total:248  pass:232  dwarn:0   dfail:0   fail:0   skip:16 
fi-bsw-n3050     total:248  pass:205  dwarn:0   dfail:0   fail:0   skip:43 
fi-bxt-t5700     total:248  pass:217  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-j1900     total:248  pass:214  dwarn:1   dfail:0   fail:1   skip:32 
fi-byt-n2820     total:248  pass:211  dwarn:0   dfail:0   fail:1   skip:36 
fi-hsw-4770      total:248  pass:225  dwarn:0   dfail:0   fail:0   skip:23 
fi-hsw-4770r     total:248  pass:225  dwarn:0   dfail:0   fail:0   skip:23 
fi-ilk-650       total:248  pass:185  dwarn:0   dfail:0   fail:2   skip:61 
fi-ivb-3520m     total:248  pass:222  dwarn:0   dfail:0   fail:0   skip:26 
fi-ivb-3770      total:248  pass:222  dwarn:0   dfail:0   fail:0   skip:26 
fi-kbl-7200u     total:248  pass:223  dwarn:0   dfail:0   fail:0   skip:25 
fi-skl-6260u     total:248  pass:233  dwarn:0   dfail:0   fail:0   skip:15 
fi-skl-6700hq    total:248  pass:225  dwarn:0   dfail:0   fail:0   skip:23 
fi-skl-6700k     total:248  pass:221  dwarn:2   dfail:0   fail:0   skip:25 
fi-skl-6770hq    total:248  pass:231  dwarn:1   dfail:0   fail:1   skip:15 
fi-snb-2520m     total:248  pass:211  dwarn:0   dfail:0   fail:0   skip:37 
fi-snb-2600      total:248  pass:210  dwarn:0   dfail:0   fail:0   skip:38 

Results at /archive/results/CI_IGT_test/Patchwork_2682/

46271d41e30090d7fc996e8f5abde6a59f51038b drm-intel-nightly: 2016y-10m-12d-11h-06m-41s UTC integration manifest
e4a3f83 drm/i915: Remove superfluous locking around userfault_list
1cacb0e drm/i915: Use RPM as the barrier for controlling user mmap access
8092cc0 drm/i915: Move user fault tracking to a separate list
9b29980 drm/i915: Update debugfs describe_obj() to show fault-mappable
42b8e6b drm/i915: Use fence_write() from rpm resume

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

Re: [PATCH 1/5] drm/i915: Use fence_write() from rpm resume

[Daniel Vetter]

On Wed, Oct 12, 2016 at 12:16:33PM +0100, Chris Wilson wrote:
> During rpm resume we restore the fences, but we do not have the
> protection of struct_mutex. This rules out updating the activity
> tracking on the fences, and requires us to rely on the rpm as the
> serialisation barrier instead.
> 
> [  350.298052] [drm:intel_runtime_resume [i915]] Resuming device
> [  350.308606]
> [  350.310520] ===============================
> [  350.315560] [ INFO: suspicious RCU usage. ]
> [  350.320554] 4.8.0-rc8-bsw-rapl+ #3133 Tainted: G     U  W
> [  350.327208] -------------------------------
> [  350.331977] ../drivers/gpu/drm/i915/i915_gem_request.h:371 suspicious rcu_dereference_protected() usage!
> [  350.342619]
> [  350.342619] other info that might help us debug this:
> [  350.342619]
> [  350.351593]
> [  350.351593] rcu_scheduler_active = 1, debug_locks = 0
> [  350.358952] 3 locks held by Xorg/320:
> [  350.363077]  #0:  (&dev->mode_config.mutex){+.+.+.}, at: [<ffffffffa030589c>] drm_modeset_lock_all+0x3c/0xd0 [drm]
> [  350.375162]  #1:  (crtc_ww_class_acquire){+.+.+.}, at: [<ffffffffa03058a6>] drm_modeset_lock_all+0x46/0xd0 [drm]
> [  350.387022]  #2:  (crtc_ww_class_mutex){+.+.+.}, at: [<ffffffffa0305056>] drm_modeset_lock+0x36/0x110 [drm]
> [  350.398236]
> [  350.398236] stack backtrace:
> [  350.403196] CPU: 1 PID: 320 Comm: Xorg Tainted: G     U  W       4.8.0-rc8-bsw-rapl+ #3133
> [  350.412457] Hardware name: Intel Corporation CHERRYVIEW C0 PLATFORM/Braswell CRB, BIOS BRAS.X64.X088.R00.1510270350 10/27/2015
> [  350.425212]  0000000000000000 ffff8801680a78c8 ffffffff81332187 ffff88016c5c5000
> [  350.433611]  0000000000000001 ffff8801680a78f8 ffffffff810ca6da ffff88016cc8b0f0
> [  350.442012]  ffff88016cc80000 ffff88016cc80000 ffff880177ad0000 ffff8801680a7948
> [  350.450409] Call Trace:
> [  350.453165]  [<ffffffff81332187>] dump_stack+0x67/0x90
> [  350.458931]  [<ffffffff810ca6da>] lockdep_rcu_suspicious+0xea/0x120
> [  350.466002]  [<ffffffffa039e8dd>] fence_update+0xbd/0x670 [i915]
> [  350.472766]  [<ffffffffa039efe2>] i915_gem_restore_fences+0x52/0x70 [i915]
> [  350.480496]  [<ffffffffa0368f42>] vlv_resume_prepare+0x72/0x570 [i915]
> [  350.487839]  [<ffffffffa0369802>] intel_runtime_resume+0x102/0x210 [i915]
> [  350.495442]  [<ffffffff8137f26f>] pci_pm_runtime_resume+0x7f/0xb0
> [  350.502274]  [<ffffffff8137f1f0>] ? pci_restore_standard_config+0x40/0x40
> [  350.509883]  [<ffffffff814401c5>] __rpm_callback+0x35/0x70
> [  350.516037]  [<ffffffff8137f1f0>] ? pci_restore_standard_config+0x40/0x40
> [  350.523646]  [<ffffffff81440224>] rpm_callback+0x24/0x80
> [  350.529604]  [<ffffffff8137f1f0>] ? pci_restore_standard_config+0x40/0x40
> [  350.537212]  [<ffffffff814417bd>] rpm_resume+0x4ad/0x740
> [  350.543161]  [<ffffffff81441aa1>] __pm_runtime_resume+0x51/0x80
> [  350.549824]  [<ffffffffa03889c8>] intel_runtime_pm_get+0x28/0x90 [i915]
> [  350.557265]  [<ffffffffa0388a53>] intel_display_power_get+0x23/0x50 [i915]
> [  350.565001]  [<ffffffffa03ef23d>] intel_atomic_commit_tail+0xdfd/0x10b0 [i915]
> [  350.573106]  [<ffffffffa034b2e9>] ? drm_atomic_helper_swap_state+0x159/0x300 [drm_kms_helper]
> [  350.582659]  [<ffffffff81615091>] ? _raw_spin_unlock+0x31/0x50
> [  350.589205]  [<ffffffffa034b2e9>] ? drm_atomic_helper_swap_state+0x159/0x300 [drm_kms_helper]
> [  350.598787]  [<ffffffffa03ef8a5>] intel_atomic_commit+0x3b5/0x500 [i915]
> [  350.606319]  [<ffffffffa03061dc>] ? drm_atomic_set_crtc_for_connector+0xcc/0x100 [drm]
> [  350.615209]  [<ffffffffa0306b49>] drm_atomic_commit+0x49/0x50 [drm]
> [  350.622242]  [<ffffffffa034dee8>] drm_atomic_helper_set_config+0x88/0xc0 [drm_kms_helper]
> [  350.631419]  [<ffffffffa02f94ac>] drm_mode_set_config_internal+0x6c/0x120 [drm]
> [  350.639623]  [<ffffffffa02fa94c>] drm_mode_setcrtc+0x22c/0x4d0 [drm]
> [  350.646760]  [<ffffffffa02f0f19>] drm_ioctl+0x209/0x460 [drm]
> [  350.653217]  [<ffffffffa02fa720>] ? drm_mode_getcrtc+0x150/0x150 [drm]
> [  350.660536]  [<ffffffff810c984a>] ? __lock_is_held+0x4a/0x70
> [  350.666885]  [<ffffffff81202303>] do_vfs_ioctl+0x93/0x6b0
> [  350.672939]  [<ffffffff8120f843>] ? __fget+0x113/0x200
> [  350.678797]  [<ffffffff8120f735>] ? __fget+0x5/0x200
> [  350.684361]  [<ffffffff81202964>] SyS_ioctl+0x44/0x80
> [  350.690030]  [<ffffffff81001deb>] do_syscall_64+0x5b/0x120
> [  350.696184]  [<ffffffff81615ada>] entry_SYSCALL64_slow_path+0x25/0x25
> 
> Note we also have to remember the lesson from commit 4fc788f5ee3d
> ("drm/i915: Flush delayed fence releases after reset") where we have to
> flush any changes to the fence on restore.
> 
> v2: Replace call to release user mmaps with an assertion that they have
> already been zapped.
> 
> Fixes: 49ef5294cda2 ("drm/i915: Move fence tracking from object to vma")
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_fence.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_fence.c b/drivers/gpu/drm/i915/i915_gem_fence.c
> index 8df1fa7234e8..f7081f4b5d22 100644
> --- a/drivers/gpu/drm/i915/i915_gem_fence.c
> +++ b/drivers/gpu/drm/i915/i915_gem_fence.c
> @@ -290,6 +290,8 @@ i915_vma_put_fence(struct i915_vma *vma)
>  {
>  	struct drm_i915_fence_reg *fence = vma->fence;
>  
> +	assert_rpm_wakelock_held(to_i915(vma->vm->dev));
> +
>  	if (!fence)
>  		return 0;
>  
> @@ -341,6 +343,8 @@ i915_vma_get_fence(struct i915_vma *vma)
>  	struct drm_i915_fence_reg *fence;
>  	struct i915_vma *set = i915_gem_object_is_tiled(vma->obj) ? vma : NULL;
>  
> +	assert_rpm_wakelock_held(to_i915(vma->vm->dev));
> +
>  	/* Just update our place in the LRU if our fence is getting reused. */
>  	if (vma->fence) {
>  		fence = vma->fence;
> @@ -371,6 +375,12 @@ void i915_gem_restore_fences(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	int i;
>  
> +	/* Note that this may be called outside of struct_mutex, by
> +	 * runtime suspend/resume. The barrier we require is enforced by
> +	 * rpm itself - all access to fences/GTT are only within an rpm
> +	 * wakeref, and to acquire that wakeref you must pass through here.
> +	 */
> +
>  	for (i = 0; i < dev_priv->num_fence_regs; i++) {
>  		struct drm_i915_fence_reg *reg = &dev_priv->fence_regs[i];
>  		struct i915_vma *vma = reg->vma;
> @@ -379,10 +389,17 @@ void i915_gem_restore_fences(struct drm_device *dev)
>  		 * Commit delayed tiling changes if we have an object still
>  		 * attached to the fence, otherwise just clear the fence.
>  		 */
> -		if (vma && !i915_gem_object_is_tiled(vma->obj))
> +		if (vma && !i915_gem_object_is_tiled(vma->obj)) {
> +			GEM_BUG_ON(!reg->dirty);
> +			GEM_BUG_ON(!list_empty(&vma->obj->userfault_link));
> +
> +			list_move(&reg->link, &dev_priv->mm.fence_list);
> +			vma->fence = NULL;
>  			vma = NULL;
> +		}
>  
> -		fence_update(reg, vma);
> +		fence_write(reg, vma);
> +		reg->vma = vma;

Same comments as with the userfault_list: Using rpm ordering to enforce
consistency causes mild panic attacks here with me ;-)

Is the above (delayed tiling change commit) even possible here, at least
for rpm resume? Same for system s/r (both s3 and s4) since the pagetables
won't survive anyway. Can't we simply make this an impossibility?
-Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Use fence_write() from rpm resume

[Chris Wilson]

On Thu, Oct 13, 2016 at 05:10:21PM +0200, Daniel Vetter wrote:
> On Wed, Oct 12, 2016 at 12:16:33PM +0100, Chris Wilson wrote:
> > @@ -379,10 +389,17 @@ void i915_gem_restore_fences(struct drm_device *dev)
> >  		 * Commit delayed tiling changes if we have an object still
> >  		 * attached to the fence, otherwise just clear the fence.
> >  		 */
> > -		if (vma && !i915_gem_object_is_tiled(vma->obj))
> > +		if (vma && !i915_gem_object_is_tiled(vma->obj)) {
> > +			GEM_BUG_ON(!reg->dirty);
> > +			GEM_BUG_ON(!list_empty(&vma->obj->userfault_link));
> > +
> > +			list_move(&reg->link, &dev_priv->mm.fence_list);
> > +			vma->fence = NULL;
> >  			vma = NULL;
> > +		}
> >  
> > -		fence_update(reg, vma);
> > +		fence_write(reg, vma);
> > +		reg->vma = vma;
> 
> Same comments as with the userfault_list: Using rpm ordering to enforce
> consistency causes mild panic attacks here with me ;-)
> 
> Is the above (delayed tiling change commit) even possible here, at least
> for rpm resume? Same for system s/r (both s3 and s4) since the pagetables
> won't survive anyway. Can't we simply make this an impossibility?

We also use this from reset to rewrite the old fences, and we know there
we can hit the delayed fence write [4fc788f5ee3d]. It would also be
possible to hit it on suspend as well.

I've been thinking about whether we should be bothering to write the
fence registers with the correct value or just cancel the fences. But we
have to restore anything that is pinned, and we have to write something
into the fences (just to be safe), and if we have to write something we
may as well use the most recent information we have as that has a good
chance of being used again.

Long story short, I don't have a better idea for restoring or avoiding
the restore of fences.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Use fence_write() from rpm resume

[Daniel Vetter]

On Thu, Oct 13, 2016 at 04:25:18PM +0100, Chris Wilson wrote:
> On Thu, Oct 13, 2016 at 05:10:21PM +0200, Daniel Vetter wrote:
> > On Wed, Oct 12, 2016 at 12:16:33PM +0100, Chris Wilson wrote:
> > > @@ -379,10 +389,17 @@ void i915_gem_restore_fences(struct drm_device *dev)
> > >  		 * Commit delayed tiling changes if we have an object still
> > >  		 * attached to the fence, otherwise just clear the fence.
> > >  		 */
> > > -		if (vma && !i915_gem_object_is_tiled(vma->obj))
> > > +		if (vma && !i915_gem_object_is_tiled(vma->obj)) {
> > > +			GEM_BUG_ON(!reg->dirty);
> > > +			GEM_BUG_ON(!list_empty(&vma->obj->userfault_link));
> > > +
> > > +			list_move(&reg->link, &dev_priv->mm.fence_list);
> > > +			vma->fence = NULL;
> > >  			vma = NULL;
> > > +		}
> > >  
> > > -		fence_update(reg, vma);
> > > +		fence_write(reg, vma);
> > > +		reg->vma = vma;
> > 
> > Same comments as with the userfault_list: Using rpm ordering to enforce
> > consistency causes mild panic attacks here with me ;-)
> > 
> > Is the above (delayed tiling change commit) even possible here, at least
> > for rpm resume? Same for system s/r (both s3 and s4) since the pagetables
> > won't survive anyway. Can't we simply make this an impossibility?
> 
> We also use this from reset to rewrite the old fences, and we know there
> we can hit the delayed fence write [4fc788f5ee3d]. It would also be
> possible to hit it on suspend as well.
> 
> I've been thinking about whether we should be bothering to write the
> fence registers with the correct value or just cancel the fences. But we
> have to restore anything that is pinned, and we have to write something
> into the fences (just to be safe), and if we have to write something we
> may as well use the most recent information we have as that has a good
> chance of being used again.
> 
> Long story short, I don't have a better idea for restoring or avoiding
> the restore of fences.

What about a rpm_resume only version that just does a blind fence_write?
It is something, and we can update the book-keeping once we do get to one
of the real synchronization points again.

With that we can leave the versions for reset and system s/r alone ... Or
is there trickery even with rpm going on?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Use fence_write() from rpm resume

[Chris Wilson]

On Thu, Oct 13, 2016 at 05:28:13PM +0200, Daniel Vetter wrote:
> On Thu, Oct 13, 2016 at 04:25:18PM +0100, Chris Wilson wrote:
> > On Thu, Oct 13, 2016 at 05:10:21PM +0200, Daniel Vetter wrote:
> > > On Wed, Oct 12, 2016 at 12:16:33PM +0100, Chris Wilson wrote:
> > > > @@ -379,10 +389,17 @@ void i915_gem_restore_fences(struct drm_device *dev)
> > > >  		 * Commit delayed tiling changes if we have an object still
> > > >  		 * attached to the fence, otherwise just clear the fence.
> > > >  		 */
> > > > -		if (vma && !i915_gem_object_is_tiled(vma->obj))
> > > > +		if (vma && !i915_gem_object_is_tiled(vma->obj)) {
> > > > +			GEM_BUG_ON(!reg->dirty);
> > > > +			GEM_BUG_ON(!list_empty(&vma->obj->userfault_link));
> > > > +
> > > > +			list_move(&reg->link, &dev_priv->mm.fence_list);
> > > > +			vma->fence = NULL;
> > > >  			vma = NULL;
> > > > +		}
> > > >  
> > > > -		fence_update(reg, vma);
> > > > +		fence_write(reg, vma);
> > > > +		reg->vma = vma;
> > > 
> > > Same comments as with the userfault_list: Using rpm ordering to enforce
> > > consistency causes mild panic attacks here with me ;-)
> > > 
> > > Is the above (delayed tiling change commit) even possible here, at least
> > > for rpm resume? Same for system s/r (both s3 and s4) since the pagetables
> > > won't survive anyway. Can't we simply make this an impossibility?
> > 
> > We also use this from reset to rewrite the old fences, and we know there
> > we can hit the delayed fence write [4fc788f5ee3d]. It would also be
> > possible to hit it on suspend as well.
> > 
> > I've been thinking about whether we should be bothering to write the
> > fence registers with the correct value or just cancel the fences. But we
> > have to restore anything that is pinned, and we have to write something
> > into the fences (just to be safe), and if we have to write something we
> > may as well use the most recent information we have as that has a good
> > chance of being used again.
> > 
> > Long story short, I don't have a better idea for restoring or avoiding
> > the restore of fences.
> 
> What about a rpm_resume only version that just does a blind fence_write?
> It is something, and we can update the book-keeping once we do get to one
> of the real synchronization points again.
> 
> With that we can leave the versions for reset and system s/r alone ... Or
> is there trickery even with rpm going on?

For rpm suspend, we only zap the user's mmap and not mark the fence as
lost. I think that's the missing piece as to why this is not as simple as
it could be for rpm-resume. On rpm-resume we only need to restore pinned
fences, and fences should only be pinned for hw access, and so there
should never be any if we were rpm-suspended. (Assuming that all pinned
fences are active, which on the surface seems a reasonable assumption.)

If that holds true, we do not need this at all for runtime pm (we still
need it across full system suspend/reset) and just need to doctor the
existing scary i915_gem_release_all_mmaps() (aka
i915_gem_runtime_suspend()!) to both release the mmap and throw away the
fence tracing. At least then we only have one dragon nest.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Use fence_write() from rpm resume

[Daniel Vetter]

On Thu, Oct 13, 2016 at 04:41:01PM +0100, Chris Wilson wrote:
> On Thu, Oct 13, 2016 at 05:28:13PM +0200, Daniel Vetter wrote:
> > On Thu, Oct 13, 2016 at 04:25:18PM +0100, Chris Wilson wrote:
> > > On Thu, Oct 13, 2016 at 05:10:21PM +0200, Daniel Vetter wrote:
> > > > On Wed, Oct 12, 2016 at 12:16:33PM +0100, Chris Wilson wrote:
> > > > > @@ -379,10 +389,17 @@ void i915_gem_restore_fences(struct drm_device *dev)
> > > > >  		 * Commit delayed tiling changes if we have an object still
> > > > >  		 * attached to the fence, otherwise just clear the fence.
> > > > >  		 */
> > > > > -		if (vma && !i915_gem_object_is_tiled(vma->obj))
> > > > > +		if (vma && !i915_gem_object_is_tiled(vma->obj)) {
> > > > > +			GEM_BUG_ON(!reg->dirty);
> > > > > +			GEM_BUG_ON(!list_empty(&vma->obj->userfault_link));
> > > > > +
> > > > > +			list_move(&reg->link, &dev_priv->mm.fence_list);
> > > > > +			vma->fence = NULL;
> > > > >  			vma = NULL;
> > > > > +		}
> > > > >  
> > > > > -		fence_update(reg, vma);
> > > > > +		fence_write(reg, vma);
> > > > > +		reg->vma = vma;
> > > > 
> > > > Same comments as with the userfault_list: Using rpm ordering to enforce
> > > > consistency causes mild panic attacks here with me ;-)
> > > > 
> > > > Is the above (delayed tiling change commit) even possible here, at least
> > > > for rpm resume? Same for system s/r (both s3 and s4) since the pagetables
> > > > won't survive anyway. Can't we simply make this an impossibility?
> > > 
> > > We also use this from reset to rewrite the old fences, and we know there
> > > we can hit the delayed fence write [4fc788f5ee3d]. It would also be
> > > possible to hit it on suspend as well.
> > > 
> > > I've been thinking about whether we should be bothering to write the
> > > fence registers with the correct value or just cancel the fences. But we
> > > have to restore anything that is pinned, and we have to write something
> > > into the fences (just to be safe), and if we have to write something we
> > > may as well use the most recent information we have as that has a good
> > > chance of being used again.
> > > 
> > > Long story short, I don't have a better idea for restoring or avoiding
> > > the restore of fences.
> > 
> > What about a rpm_resume only version that just does a blind fence_write?
> > It is something, and we can update the book-keeping once we do get to one
> > of the real synchronization points again.
> > 
> > With that we can leave the versions for reset and system s/r alone ... Or
> > is there trickery even with rpm going on?
> 
> For rpm suspend, we only zap the user's mmap and not mark the fence as
> lost. I think that's the missing piece as to why this is not as simple as
> it could be for rpm-resume. On rpm-resume we only need to restore pinned
> fences, and fences should only be pinned for hw access, and so there
> should never be any if we were rpm-suspended. (Assuming that all pinned
> fences are active, which on the surface seems a reasonable assumption.)
> 
> If that holds true, we do not need this at all for runtime pm (we still
> need it across full system suspend/reset) and just need to doctor the
> existing scary i915_gem_release_all_mmaps() (aka
> i915_gem_runtime_suspend()!) to both release the mmap and throw away the
> fence tracing. At least then we only have one dragon nest.

Yeah, would be great to unify this stuff a bit ... Making fence semantics
the same for rpm suspend as for normal suspend would definitely be nice,
and the hw will forget about the registers anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx