Bug 95634, avoid fresh work on shutdown

Bug 95634, avoid fresh work on shutdown

[Chris Wilson]

In https://bugs.freedesktop.org/show_bug.cgi?id=95634 the cause of the
warning is that we populate the GGTT without waking the device. For the
purposes of suspend, if the device is already idle we have accomplished
our goal (of making the device idle). This series tries to avoid waking
the device simply to idle it (during suspend and elsewhere).
-Chris

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

[PATCH 1/6] drm/i915: Skip idling an idle engine

[Chris Wilson]

During suspend (or module unload), if we have never accessed the engine
(i.e. userspace never submitted a batch to it), the engine is idle. Then
we attempt to idle the engine by forcing it to the default context,
which actually means we submit a render batch to setup the golden
context state and then wait for it to complete. We can skip this
entirely as we know the engine is idle.

v2: Drop incorrect comment.

References: https://bugs.freedesktop.org/show_bug.cgi?id=95634
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 89967f97c097..61812a53e56d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3426,8 +3426,10 @@ int i915_gpu_idle(struct drm_device *dev)
 	struct intel_engine_cs *engine;
 	int ret;
 
-	/* Flush everything onto the inactive list. */
 	for_each_engine(engine, dev_priv) {
+		if (engine->last_context == NULL)
+			continue;
+
 		if (!i915.enable_execlists) {
 			struct drm_i915_gem_request *req;
 
-- 
2.8.1

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

[PATCH 2/6] drm/i915: Treat kernel context as initialised

[Chris Wilson]

The kernel context exists simply as a placeholder and should never be
executed with a render context. It does not need the golden render
state, as that will always be applied to a user context. By skipping the
initialisation we can avoid issues in attempting to program the golden
render context when trying to make the hardware idle.

Testcase: igt/drm_module_reload_basic #byt
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95634
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 45 +++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index a3b11aac23a4..3e3acd054f05 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -413,7 +413,8 @@ int i915_gem_context_init(struct drm_device *dev)
 		return PTR_ERR(ctx);
 	}
 
-	if (!i915.enable_execlists && ctx->engine[RCS].state) {
+	if (!i915.enable_execlists) {
+		struct intel_engine_cs *engine;
 		int ret;
 
 		/* We may need to do things with the shrinker which
@@ -423,13 +424,28 @@ int i915_gem_context_init(struct drm_device *dev)
 		 * be available. To avoid this we always pin the default
 		 * context.
 		 */
-		ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state,
-					    get_context_alignment(dev_priv), 0);
-		if (ret) {
-			DRM_ERROR("Failed to pinned default global context (error %d)\n",
-				  ret);
-			i915_gem_context_unreference(ctx);
-			return ret;
+		for_each_engine(engine, dev_priv) {
+			struct intel_context *ce = &ctx->engine[engine->id];
+
+			ret = 0;
+			if (ce->state)
+				ret = i915_gem_obj_ggtt_pin(ce->state,
+							    get_context_alignment(dev_priv), 0);
+			if (ret) {
+				DRM_ERROR("Failed to pinned default global context (error %d)\n",
+					  ret);
+				i915_gem_context_unreference(ctx);
+				return ret;
+			}
+
+			/* The kernel context is only used as a placeholder
+			 * for flushing the active context. It is never used
+			 * for submitting rendering and as such never requires
+			 * the golden render context, and so we can skip
+			 * emitting it when we switch to the kernel context
+			 * (during eviction).
+			 */
+			ce->initialised = true;
 		}
 	}
 
@@ -452,13 +468,16 @@ void i915_gem_context_lost(struct drm_i915_private *dev_priv)
 			i915_gem_context_unpin(engine->last_context, engine);
 			engine->last_context = NULL;
 		}
-
-		/* Force the GPU state to be reinitialised on enabling */
-		dev_priv->kernel_context->engine[engine->id].initialised =
-			engine->init_context == NULL;
 	}
 
-	/* Force the GPU state to be reinitialised on enabling */
+	/* Force the GPU state to be restore on enabling */
+	for_each_engine(engine, dev_priv) {
+		struct intel_context *ce =
+			&dev_priv->kernel_context->engine[engine->id];
+
+		ce->initialised =
+			!i915.enable_execlists || engine->init_context == NULL;
+	}
 	dev_priv->kernel_context->remap_slice = ALL_L3_SLICES(dev_priv);
 }
 
-- 
2.8.1

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

[PATCH 3/6] drm/i915: Mark all default contexts as uninitialised after context loss

[Chris Wilson]

When the GPU is reset or state lost through suspend, every default
legacy context needs to reload their state - both the golden render
state and the L3 mapping. Only context images explicitly saved to memory
(i.e. all execlists and non-default legacy contexts) will retain their
state across the reset.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 3e3acd054f05..62de72a947e5 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -470,7 +470,21 @@ void i915_gem_context_lost(struct drm_i915_private *dev_priv)
 		}
 	}
 
-	/* Force the GPU state to be restore on enabling */
+	/* Force the GPU state to be restored on enabling */
+	if (!i915.enable_execlists) {
+		struct i915_gem_context *ctx;
+
+		list_for_each_entry(ctx, &dev_priv->context_list, link) {
+			if (!i915_gem_context_is_default(ctx))
+				continue;
+
+			for_each_engine(engine, dev_priv)
+				ctx->engine[engine->id].initialised = false;
+
+			ctx->remap_slice = ALL_L3_SLICES(dev_priv);
+		}
+	}
+
 	for_each_engine(engine, dev_priv) {
 		struct intel_context *ce =
 			&dev_priv->kernel_context->engine[engine->id];
@@ -478,7 +492,6 @@ void i915_gem_context_lost(struct drm_i915_private *dev_priv)
 		ce->initialised =
 			!i915.enable_execlists || engine->init_context == NULL;
 	}
-	dev_priv->kernel_context->remap_slice = ALL_L3_SLICES(dev_priv);
 }
 
 void i915_gem_context_fini(struct drm_device *dev)
-- 
2.8.1

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

[PATCH 4/6] drm/i915: No need to wait for idle on L3 remap

[Chris Wilson]

As the L3 remapping is applied before the next execution, there is no
need to wait until all previous uses are idle, the application will not
occur any sooner.

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

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 02507bfc8def..a6e90fe05a1e 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -227,13 +227,6 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
 		}
 	}
 
-	ret = i915_gpu_idle(drm_dev);
-	if (ret) {
-		kfree(temp);
-		mutex_unlock(&drm_dev->struct_mutex);
-		return ret;
-	}
-
 	/* TODO: Ideally we really want a GPU reset here to make sure errors
 	 * aren't propagated. Since I cannot find a stable way to reset the GPU
 	 * at this point it is left as a TODO.
-- 
2.8.1

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

[PATCH 5/6] drm/i915: Split idling from forcing context switch

[Chris Wilson]

We only need to force a switch to the kernel context placeholder during
eviction. All other uses of i915_gpu_idle() just want to wait until
existing work on the GPU is idle. Rename i915_gpu_idle() to
i915_gem_wait_for_idle() to avoid any implications about "parking" the
context first.

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          |  2 +-
 drivers/gpu/drm/i915/i915_gem.c          | 20 +++----------
 drivers/gpu/drm/i915/i915_gem_evict.c    | 51 +++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_gem_gtt.c      |  2 +-
 drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
 6 files changed, 55 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index ac7e5692496d..615cef736356 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4941,7 +4941,7 @@ i915_drop_caches_set(void *data, u64 val)
 		return ret;
 
 	if (val & DROP_ACTIVE) {
-		ret = i915_gpu_idle(dev);
+		ret = i915_gem_wait_for_idle(dev_priv);
 		if (ret)
 			goto unlock;
 	}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e4c8e341655c..d2719d96a274 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3283,7 +3283,7 @@ int i915_gem_init_engines(struct drm_device *dev);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
 void i915_gem_init_swizzling(struct drm_device *dev);
 void i915_gem_cleanup_engines(struct drm_device *dev);
-int __must_check i915_gpu_idle(struct drm_device *dev);
+int __must_check i915_gem_wait_for_idle(struct drm_i915_private *dev_priv);
 int __must_check i915_gem_suspend(struct drm_device *dev);
 void __i915_add_request(struct drm_i915_gem_request *req,
 			struct drm_i915_gem_object *batch_obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 61812a53e56d..e94758850c77 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3420,29 +3420,17 @@ int __i915_vma_unbind_no_wait(struct i915_vma *vma)
 	return __i915_vma_unbind(vma, false);
 }
 
-int i915_gpu_idle(struct drm_device *dev)
+int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	int ret;
 
+	lockdep_assert_held(&dev_priv->dev->struct_mutex);
+
 	for_each_engine(engine, dev_priv) {
 		if (engine->last_context == NULL)
 			continue;
 
-		if (!i915.enable_execlists) {
-			struct drm_i915_gem_request *req;
-
-			req = i915_gem_request_alloc(engine, NULL);
-			if (IS_ERR(req))
-				return PTR_ERR(req);
-
-			ret = i915_switch_context(req);
-			i915_add_request_no_flush(req);
-			if (ret)
-				return ret;
-		}
-
 		ret = intel_engine_idle(engine);
 		if (ret)
 			return ret;
@@ -4703,7 +4691,7 @@ i915_gem_suspend(struct drm_device *dev)
 	int ret = 0;
 
 	mutex_lock(&dev->struct_mutex);
-	ret = i915_gpu_idle(dev);
+	ret = i915_gem_wait_for_idle(dev_priv);
 	if (ret)
 		goto err;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index b144c3f5c650..5741b58d186c 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -33,6 +33,37 @@
 #include "intel_drv.h"
 #include "i915_trace.h"
 
+static int switch_to_pinned_context(struct drm_i915_private *dev_priv)
+{
+	struct intel_engine_cs *engine;
+
+	if (i915.enable_execlists)
+		return 0;
+
+	for_each_engine(engine, dev_priv) {
+		struct drm_i915_gem_request *req;
+		int ret;
+
+		if (engine->last_context == NULL)
+			continue;
+
+		if (engine->last_context == dev_priv->kernel_context)
+			continue;
+
+		req = i915_gem_request_alloc(engine, dev_priv->kernel_context);
+		if (IS_ERR(req))
+			return PTR_ERR(req);
+
+		ret = i915_switch_context(req);
+		i915_add_request_no_flush(req);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+
 static bool
 mark_free(struct i915_vma *vma, struct list_head *unwind)
 {
@@ -150,11 +181,17 @@ none:
 
 	/* Only idle the GPU and repeat the search once */
 	if (pass++ == 0) {
-		ret = i915_gpu_idle(dev);
+		struct drm_i915_private *dev_priv = to_i915(dev);
+
+		ret = switch_to_pinned_context(dev_priv);
 		if (ret)
 			return ret;
 
-		i915_gem_retire_requests(to_i915(dev));
+		ret = i915_gem_wait_for_idle(dev_priv);
+		if (ret)
+			return ret;
+
+		i915_gem_retire_requests(dev_priv);
 		goto search_again;
 	}
 
@@ -261,11 +298,17 @@ int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle)
 	trace_i915_gem_evict_vm(vm);
 
 	if (do_idle) {
-		ret = i915_gpu_idle(vm->dev);
+		struct drm_i915_private *dev_priv = to_i915(vm->dev);
+
+		ret = switch_to_pinned_context(dev_priv);
+		if (ret)
+			return ret;
+
+		ret = i915_gem_wait_for_idle(dev_priv);
 		if (ret)
 			return ret;
 
-		i915_gem_retire_requests(to_i915(vm->dev));
+		i915_gem_retire_requests(dev_priv);
 
 		WARN_ON(!list_empty(&vm->active_list));
 	}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 46684779d4d6..84f2ed78cf88 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2261,7 +2261,7 @@ static bool do_idling(struct drm_i915_private *dev_priv)
 
 	if (unlikely(ggtt->do_idle_maps)) {
 		dev_priv->mm.interruptible = false;
-		if (i915_gpu_idle(dev_priv->dev)) {
+		if (i915_gem_wait_for_idle(dev_priv)) {
 			DRM_ERROR("Couldn't idle GPU\n");
 			/* Wait a bit, in hopes it avoids the hang */
 			udelay(10);
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 538c30499848..886a8797566d 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -408,7 +408,7 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
 		return NOTIFY_DONE;
 
 	/* Force everything onto the inactive lists */
-	ret = i915_gpu_idle(dev_priv->dev);
+	ret = i915_gem_wait_for_idle(dev_priv);
 	if (ret)
 		goto out;
 
-- 
2.8.1

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

[PATCH 6/6] drm/i915: Only switch to default context when evicting from GGTT

[Chris Wilson]

The contexts only pin space within the global GTT. Therefore forcing the
switch to the perma-pinned kernel context only has an effect when trying
to evict from and find room within the global GTT. We can then restrict
the switch to only when operating on the default context. This is mostly
a no-op as full-ppgtt only exists with execlists at present which skips
the context switch anyway.

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

diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index 5741b58d186c..3c1280ec7ff6 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -183,9 +183,11 @@ none:
 	if (pass++ == 0) {
 		struct drm_i915_private *dev_priv = to_i915(dev);
 
-		ret = switch_to_pinned_context(dev_priv);
-		if (ret)
-			return ret;
+		if (i915_is_ggtt(vm)) {
+			ret = switch_to_pinned_context(dev_priv);
+			if (ret)
+				return ret;
+		}
 
 		ret = i915_gem_wait_for_idle(dev_priv);
 		if (ret)
@@ -300,9 +302,11 @@ int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle)
 	if (do_idle) {
 		struct drm_i915_private *dev_priv = to_i915(vm->dev);
 
-		ret = switch_to_pinned_context(dev_priv);
-		if (ret)
-			return ret;
+		if (i915_is_ggtt(vm)) {
+			ret = switch_to_pinned_context(dev_priv);
+			if (ret)
+				return ret;
+		}
 
 		ret = i915_gem_wait_for_idle(dev_priv);
 		if (ret)
-- 
2.8.1

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

✗ Ro.CI.BAT: failure for series starting with [1/6] drm/i915: Skip idling an idle engine

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915: Skip idling an idle engine
URL   : https://patchwork.freedesktop.org/series/7708/
State : failure

== Summary ==

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

Test drv_getparams_basic:
        Subgroup basic-eu-total:
                pass       -> SKIP       (ro-hsw-i3-4010u)
                pass       -> SKIP       (ro-bdw-i7-5600u)
                pass       -> SKIP       (fi-hsw-i7-4770r)
                pass       -> SKIP       (ro-snb-i7-2620M)
                pass       -> SKIP       (ro-hsw-i7-4770r)
                pass       -> SKIP       (fi-hsw-i7-4770k)
                pass       -> SKIP       (fi-snb-i7-2600)
                pass       -> SKIP       (ro-ivb-i7-3770)
                pass       -> SKIP       (ro-ivb2-i7-3770)
                pass       -> SKIP       (ro-byt-n2820)
                pass       -> SKIP       (fi-byt-n2820)
        Subgroup basic-subslice-total:
                pass       -> SKIP       (ro-hsw-i3-4010u)
                pass       -> SKIP       (ro-bdw-i7-5600u)
                pass       -> SKIP       (fi-hsw-i7-4770r)
                pass       -> SKIP       (ro-snb-i7-2620M)
                pass       -> SKIP       (ro-hsw-i7-4770r)
                pass       -> SKIP       (fi-hsw-i7-4770k)
                pass       -> SKIP       (fi-snb-i7-2600)
                pass       -> SKIP       (ro-ivb-i7-3770)
                pass       -> SKIP       (ro-ivb2-i7-3770)
                pass       -> SKIP       (ro-byt-n2820)
                pass       -> SKIP       (fi-byt-n2820)
Test drv_hangman:
        Subgroup error-state-basic:
                pass       -> SKIP       (ro-hsw-i3-4010u)
                pass       -> SKIP       (ro-bdw-i7-5600u)
                pass       -> SKIP       (fi-hsw-i7-4770r)
                pass       -> SKIP       (ro-snb-i7-2620M)
                pass       -> SKIP       (ro-hsw-i7-4770r)
                pass       -> SKIP       (fi-hsw-i7-4770k)
                pass       -> SKIP       (fi-snb-i7-2600)
                pass       -> SKIP       (ro-ivb-i7-3770)
                pass       -> SKIP       (ro-ivb2-i7-3770)
                pass       -> SKIP       (ro-byt-n2820)
                pass       -> SKIP       (fi-byt-n2820)
Test drv_module_reload_basic:
                pass       -> SKIP       (ro-hsw-i3-4010u)
                pass       -> SKIP       (ro-bdw-i7-5600u)
                pass       -> SKIP       (fi-hsw-i7-4770r)
                pass       -> SKIP       (ro-snb-i7-2620M)
                pass       -> SKIP       (ro-hsw-i7-4770r)
                pass       -> SKIP       (fi-hsw-i7-4770k)
                pass       -> SKIP       (fi-snb-i7-2600)
                pass       -> SKIP       (ro-ivb-i7-3770)
                pass       -> SKIP       (ro-ivb2-i7-3770)
                pass       -> SKIP       (ro-byt-n2820)
                pass       -> SKIP       (fi-byt-n2820)
Test gem_basic:
        Subgroup bad-close:
                pass       -> SKIP       (ro-hsw-i3-4010u)
                pass       -> SKIP       (ro-bdw-i7-5600u)
                pass       -> SKIP       (fi-hsw-i7-4770r)
                pass       -> SKIP       (ro-snb-i7-2620M)
                pass       -> SKIP       (ro-hsw-i7-4770r)
                pass       -> SKIP       (fi-hsw-i7-4770k)
                pass       -> SKIP       (fi-snb-i7-2600)
                pass       -> SKIP       (ro-ivb-i7-3770)
                pass       -> SKIP       (ro-ivb2-i7-3770)
                pass       -> SKIP       (ro-byt-n2820)
                pass       -> SKIP       (fi-byt-n2820)
        Subgroup create-close:
                pass       -> SKIP       (ro-hsw-i3-4010u)
                pass       -> SKIP       (ro-bdw-i7-5600u)
                pass       -> SKIP       (fi-hsw-i7-4770r)
                pass       -> SKIP       (ro-snb-i7-2620M)
                pass       -> SKIP       (ro-hsw-i7-4770r)
                pass       -> SKIP       (fi-hsw-i7-4770k)
                pass       -> SKIP       (fi-snb-i7-2600)
                pass       -> SKIP       (ro-ivb-i7-3770)
                pass       -> SKIP       (ro-ivb2-i7-3770)
                pass       -> SKIP       (ro-byt-n2820)
                pass       -> SKIP       (fi-byt-n2820)
        Subgroup create-fd-close:
                pass       -> SKIP       (ro-hsw-i3-4010u)
                pass       -> SKIP       (ro-bdw-i7-5600u)
                pass       -> SKIP       (fi-hsw-i7-4770r)
                pass       -> SKIP       (ro-snb-i7-2620M)
                pass       -> SKIP       (ro-hsw-i7-4770r)
                pass       -> SKIP       (fi-hsw-i7-4770k)
                pass       -> SKIP       (fi-snb-i7-2600)
                pass       -> SKIP       (ro-ivb-i7-3770)
                pass       -> SKIP       (ro-ivb2-i7-3770)
                pass       -> SKIP       (ro-byt-n2820)
                pass       -> SKIP       (fi-byt-n2820)
Test gem_busy:
        Subgroup basic-blt:
                pass       -> SKIP       (ro-hsw-i3-4010u)
                pass       -> SKIP       (ro-bdw-i7-5600u)
                pass       -> SKIP       (fi-hsw-i7-4770r)
                pass       -> SKIP       (ro-snb-i7-2620M)
                pass       -> SKIP       (ro-hsw-i7-4770r)
                pass       -> SKIP       (fi-hsw-i7-4770k)
                pass       -> SKIP       (fi-snb-i7-2600)
                pass       -> SKIP       (ro-ivb-i7-3770)
WARNING: Long output truncated
ro-bsw-n3050 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1005/

fc9d741 drm-intel-nightly: 2016y-05m-25d-07h-45m-48s UTC integration manifest
43ab6f3 drm/i915: Only switch to default context when evicting from GGTT
bf12017 drm/i915: Split idling from forcing context switch
cd73f4f drm/i915: No need to wait for idle on L3 remap
efb2f1e drm/i915: Mark all default contexts as uninitialised after context loss
d8e5fca drm/i915: Treat kernel context as initialised
4bdd0b9 drm/i915: Skip idling an idle engine

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

Re: [PATCH 2/6] drm/i915: Treat kernel context as initialised

[Joonas Lahtinen]

On ke, 2016-05-25 at 12:48 +0100, Chris Wilson wrote:
> The kernel context exists simply as a placeholder and should never be
> executed with a render context. It does not need the golden render
> state, as that will always be applied to a user context. By skipping the
> initialisation we can avoid issues in attempting to program the golden
> render context when trying to make the hardware idle.
> 
> Testcase: igt/drm_module_reload_basic #byt
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95634
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 45 +++++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index a3b11aac23a4..3e3acd054f05 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -413,7 +413,8 @@ int i915_gem_context_init(struct drm_device *dev)
>  		return PTR_ERR(ctx);
>  	}
>  
> -	if (!i915.enable_execlists && ctx->engine[RCS].state) {
> +	if (!i915.enable_execlists) {
> +		struct intel_engine_cs *engine;
>  		int ret;
>  
>  		/* We may need to do things with the shrinker which
> @@ -423,13 +424,28 @@ int i915_gem_context_init(struct drm_device *dev)
>  		 * be available. To avoid this we always pin the default
>  		 * context.
>  		 */
> -		ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state,
> -					    get_context_alignment(dev_priv), 0);
> -		if (ret) {
> -			DRM_ERROR("Failed to pinned default global context (error %d)\n",
> -				  ret);
> -			i915_gem_context_unreference(ctx);
> -			return ret;
> +		for_each_engine(engine, dev_priv) {
> +			struct intel_context *ce = &ctx->engine[engine->id];
> +
> +			ret = 0;
> +			if (ce->state)

Ugh, rather have one level of indent and teardown separated, it'll be
easier to read and extend yadda yadda...

> +				ret = i915_gem_obj_ggtt_pin(ce->state,
> +							    get_context_alignment(dev_priv), 0);
> +			if (ret) {
> +				DRM_ERROR("Failed to pinned default global context (error %d)\n",

s/pinned/pin/

> +					  ret);
> +				i915_gem_context_unreference(ctx);
> +				return ret;
> +			}
> +
> +			/* The kernel context is only used as a placeholder
> +			 * for flushing the active context. It is never used
> +			 * for submitting rendering and as such never requires
> +			 * the golden render context, and so we can skip
> +			 * emitting it when we switch to the kernel context
> +			 * (during eviction).
> +			 */
> +			ce->initialised = true;
>  		}
>  	}
>  
> @@ -452,13 +468,16 @@ void i915_gem_context_lost(struct drm_i915_private *dev_priv)
>  			i915_gem_context_unpin(engine->last_context, engine);
>  			engine->last_context = NULL;
>  		}
> -
> -		/* Force the GPU state to be reinitialised on enabling */
> -		dev_priv->kernel_context->engine[engine->id].initialised =
> -			engine->init_context == NULL;
>  	}
>  
> -	/* Force the GPU state to be reinitialised on enabling */
> +	/* Force the GPU state to be restore on enabling */

s/restore/restored/

> +	for_each_engine(engine, dev_priv) {
> +		struct intel_context *ce =
> +			&dev_priv->kernel_context->engine[engine->id];
> +
> +		ce->initialised =
> +			!i915.enable_execlists || engine->init_context == NULL;
> +	}

<NL> here?

>  	dev_priv->kernel_context->remap_slice = ALL_L3_SLICES(dev_priv);
>  }
>  
-- 
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/6] drm/i915: Treat kernel context as initialised

[Chris Wilson]

On Wed, May 25, 2016 at 12:48:49PM +0100, Chris Wilson wrote:
> The kernel context exists simply as a placeholder and should never be
> executed with a render context. It does not need the golden render
> state, as that will always be applied to a user context. By skipping the
> initialisation we can avoid issues in attempting to program the golden
> render context when trying to make the hardware idle.
> 
> Testcase: igt/drm_module_reload_basic #byt
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95634
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 45 +++++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index a3b11aac23a4..3e3acd054f05 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -413,7 +413,8 @@ int i915_gem_context_init(struct drm_device *dev)
>  		return PTR_ERR(ctx);
>  	}
>  
> -	if (!i915.enable_execlists && ctx->engine[RCS].state) {
> +	if (!i915.enable_execlists) {
> +		struct intel_engine_cs *engine;
>  		int ret;
>  
>  		/* We may need to do things with the shrinker which
> @@ -423,13 +424,28 @@ int i915_gem_context_init(struct drm_device *dev)
>  		 * be available. To avoid this we always pin the default
>  		 * context.
>  		 */
> -		ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state,
> -					    get_context_alignment(dev_priv), 0);
> -		if (ret) {
> -			DRM_ERROR("Failed to pinned default global context (error %d)\n",
> -				  ret);
> -			i915_gem_context_unreference(ctx);
> -			return ret;
> +		for_each_engine(engine, dev_priv) {

for_each_engine! Too early, we haven't yet initialised the engines.
The problem of trying to make it look neat.
-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 3/6] drm/i915: Mark all default contexts as uninitialised after context loss

[Joonas Lahtinen]

On ke, 2016-05-25 at 12:48 +0100, Chris Wilson wrote:
> When the GPU is reset or state lost through suspend, every default
> legacy context needs to reload their state - both the golden render
> state and the L3 mapping. Only context images explicitly saved to memory
> (i.e. all execlists and non-default legacy contexts) will retain their
> state across the reset.
> 

Should not hurt at least.

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

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 3e3acd054f05..62de72a947e5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -470,7 +470,21 @@ void i915_gem_context_lost(struct drm_i915_private *dev_priv)
>  		}
>  	}
>  
> -	/* Force the GPU state to be restore on enabling */
> +	/* Force the GPU state to be restored on enabling */
> +	if (!i915.enable_execlists) {
> +		struct i915_gem_context *ctx;
> +
> +		list_for_each_entry(ctx, &dev_priv->context_list, link) {
> +			if (!i915_gem_context_is_default(ctx))
> +				continue;
> +
> +			for_each_engine(engine, dev_priv)
> +				ctx->engine[engine->id].initialised = false;
> +
> +			ctx->remap_slice = ALL_L3_SLICES(dev_priv);
> +		}
> +	}
> +
>  	for_each_engine(engine, dev_priv) {
>  		struct intel_context *ce =
>  			&dev_priv->kernel_context->engine[engine->id];
> @@ -478,7 +492,6 @@ void i915_gem_context_lost(struct drm_i915_private *dev_priv)
>  		ce->initialised =
>  			!i915.enable_execlists || engine->init_context == NULL;
>  	}
> -	dev_priv->kernel_context->remap_slice = ALL_L3_SLICES(dev_priv);
>  }
>  
>  void i915_gem_context_fini(struct drm_device *dev)
-- 
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 5/6] drm/i915: Split idling from forcing context switch

[Joonas Lahtinen]

On ke, 2016-05-25 at 12:48 +0100, Chris Wilson wrote:
> We only need to force a switch to the kernel context placeholder during
> eviction. All other uses of i915_gpu_idle() just want to wait until
> existing work on the GPU is idle. Rename i915_gpu_idle() to
> i915_gem_wait_for_idle() to avoid any implications about "parking" the
> context first.
> 
> 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          |  2 +-
>  drivers/gpu/drm/i915/i915_gem.c          | 20 +++----------
>  drivers/gpu/drm/i915/i915_gem_evict.c    | 51 +++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/i915_gem_gtt.c      |  2 +-
>  drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
>  6 files changed, 55 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index ac7e5692496d..615cef736356 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4941,7 +4941,7 @@ i915_drop_caches_set(void *data, u64 val)
>  		return ret;
>  
>  	if (val & DROP_ACTIVE) {
> -		ret = i915_gpu_idle(dev);
> +		ret = i915_gem_wait_for_idle(dev_priv);
>  		if (ret)
>  			goto unlock;
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e4c8e341655c..d2719d96a274 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3283,7 +3283,7 @@ int i915_gem_init_engines(struct drm_device *dev);
>  int __must_check i915_gem_init_hw(struct drm_device *dev);
>  void i915_gem_init_swizzling(struct drm_device *dev);
>  void i915_gem_cleanup_engines(struct drm_device *dev);
> -int __must_check i915_gpu_idle(struct drm_device *dev);
> +int __must_check i915_gem_wait_for_idle(struct drm_i915_private *dev_priv);
>  int __must_check i915_gem_suspend(struct drm_device *dev);
>  void __i915_add_request(struct drm_i915_gem_request *req,
>  			struct drm_i915_gem_object *batch_obj,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 61812a53e56d..e94758850c77 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3420,29 +3420,17 @@ int __i915_vma_unbind_no_wait(struct i915_vma *vma)
>  	return __i915_vma_unbind(vma, false);
>  }
>  
> -int i915_gpu_idle(struct drm_device *dev)
> +int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_engine_cs *engine;
>  	int ret;
>  
> +	lockdep_assert_held(&dev_priv->dev->struct_mutex);
> +
>  	for_each_engine(engine, dev_priv) {
>  		if (engine->last_context == NULL)
>  			continue;
>  
> -		if (!i915.enable_execlists) {
> -			struct drm_i915_gem_request *req;
> -
> -			req = i915_gem_request_alloc(engine, NULL);
> -			if (IS_ERR(req))
> -				return PTR_ERR(req);
> -
> -			ret = i915_switch_context(req);
> -			i915_add_request_no_flush(req);
> -			if (ret)
> -				return ret;
> -		}
> -
>  		ret = intel_engine_idle(engine);
>  		if (ret)
>  			return ret;
> @@ -4703,7 +4691,7 @@ i915_gem_suspend(struct drm_device *dev)
>  	int ret = 0;
>  
>  	mutex_lock(&dev->struct_mutex);
> -	ret = i915_gpu_idle(dev);
> +	ret = i915_gem_wait_for_idle(dev_priv);
>  	if (ret)
>  		goto err;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index b144c3f5c650..5741b58d186c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -33,6 +33,37 @@
>  #include "intel_drv.h"
>  #include "i915_trace.h"
>  
> +static int switch_to_pinned_context(struct drm_i915_private *dev_priv)

Why not switch_to_kernel_context as it's effectively what this does (-
execlist behaviour)?

> +{
> +	struct intel_engine_cs *engine;
> +
> +	if (i915.enable_execlists)
> +		return 0;
> +
> +	for_each_engine(engine, dev_priv) {
> +		struct drm_i915_gem_request *req;
> +		int ret;
> +
> +		if (engine->last_context == NULL)
> +			continue;
> +
> +		if (engine->last_context == dev_priv->kernel_context)
> +			continue;
> +
> +		req = i915_gem_request_alloc(engine, dev_priv->kernel_context);
> +		if (IS_ERR(req))
> +			return PTR_ERR(req);
> +
> +		ret = i915_switch_context(req);
> +		i915_add_request_no_flush(req);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +
>  static bool
>  mark_free(struct i915_vma *vma, struct list_head *unwind)
>  {
> @@ -150,11 +181,17 @@ none:
>  
>  	/* Only idle the GPU and repeat the search once */
>  	if (pass++ == 0) {
> -		ret = i915_gpu_idle(dev);
> +		struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +		ret = switch_to_pinned_context(dev_priv);
>  		if (ret)
>  			return ret;
>  
> -		i915_gem_retire_requests(to_i915(dev));
> +		ret = i915_gem_wait_for_idle(dev_priv);
> +		if (ret)
> +			return ret;
> +
> +		i915_gem_retire_requests(dev_priv);
>  		goto search_again;
>  	}
>  
> @@ -261,11 +298,17 @@ int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle)
>  	trace_i915_gem_evict_vm(vm);
>  
>  	if (do_idle) {
> -		ret = i915_gpu_idle(vm->dev);
> +		struct drm_i915_private *dev_priv = to_i915(vm->dev);
> +
> +		ret = switch_to_pinned_context(dev_priv);
> +		if (ret)
> +			return ret;
> +
> +		ret = i915_gem_wait_for_idle(dev_priv);
>  		if (ret)
>  			return ret;
>  
> -		i915_gem_retire_requests(to_i915(vm->dev));
> +		i915_gem_retire_requests(dev_priv);
>  
>  		WARN_ON(!list_empty(&vm->active_list));
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 46684779d4d6..84f2ed78cf88 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2261,7 +2261,7 @@ static bool do_idling(struct drm_i915_private *dev_priv)
>  
>  	if (unlikely(ggtt->do_idle_maps)) {
>  		dev_priv->mm.interruptible = false;
> -		if (i915_gpu_idle(dev_priv->dev)) {
> +		if (i915_gem_wait_for_idle(dev_priv)) {
>  			DRM_ERROR("Couldn't idle GPU\n");

This wait_for scheme and the error message do not make sense together.

>  			/* Wait a bit, in hopes it avoids the hang */
>  			udelay(10);
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index 538c30499848..886a8797566d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -408,7 +408,7 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
>  		return NOTIFY_DONE;
>  
>  	/* Force everything onto the inactive lists */
> -	ret = i915_gpu_idle(dev_priv->dev);
> +	ret = i915_gem_wait_for_idle(dev_priv);
>  	if (ret)
>  		goto out;
>  
-- 
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/6] drm/i915: Treat kernel context as initialised

[Chris Wilson]

On Wed, May 25, 2016 at 03:38:44PM +0300, Joonas Lahtinen wrote:
> On ke, 2016-05-25 at 12:48 +0100, Chris Wilson wrote:
> > +	for_each_engine(engine, dev_priv) {
> > +		struct intel_context *ce =
> > +			&dev_priv->kernel_context->engine[engine->id];
> > +
> > +		ce->initialised =
> > +			!i915.enable_execlists || engine->init_context == NULL;
> > +	}
> 
> <NL> here?

Not sure.

For, keep it tight with the comment and similar to the code it replaced.

Against, whitespace is good.

It's moot as this line disappears in a couple of patches time. I
preferred trying to show the semantic similarity in the patch.
-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 5/6] drm/i915: Split idling from forcing context switch

[Chris Wilson]

On Wed, May 25, 2016 at 03:57:04PM +0300, Joonas Lahtinen wrote:
> On ke, 2016-05-25 at 12:48 +0100, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> > index b144c3f5c650..5741b58d186c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> > @@ -33,6 +33,37 @@
> >  #include "intel_drv.h"
> >  #include "i915_trace.h"
> >  
> > +static int switch_to_pinned_context(struct drm_i915_private *dev_priv)
> 
> Why not switch_to_kernel_context as it's effectively what this does (-
> execlist behaviour)?

Because "pinned context" tells us why we were are switching. The kernel
context is just the placeholder that is pinned and allows eviction of
the previous context.

> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 46684779d4d6..84f2ed78cf88 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -2261,7 +2261,7 @@ static bool do_idling(struct drm_i915_private *dev_priv)
> >  
> >  	if (unlikely(ggtt->do_idle_maps)) {
> >  		dev_priv->mm.interruptible = false;
> > -		if (i915_gpu_idle(dev_priv->dev)) {
> > +		if (i915_gem_wait_for_idle(dev_priv)) {
> >  			DRM_ERROR("Couldn't idle GPU\n");
> 
> This wait_for scheme and the error message do not make sense together.

Yes. Nothing about the HW bug this is for makes sense. Why let a message
you never wish to see make any difference ;)
-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

[PATCH v2 1/6] drm/i915: Skip idling an idle engine

[Chris Wilson]

During suspend (or module unload), if we have never accessed the engine
(i.e. userspace never submitted a batch to it), the engine is idle. Then
we attempt to idle the engine by forcing it to the default context,
which actually means we submit a render batch to setup the golden
context state and then wait for it to complete. We can skip this
entirely as we know the engine is idle.

v2: Drop incorrect comment.

References: https://bugs.freedesktop.org/show_bug.cgi?id=95634
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 89967f97c097..61812a53e56d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3426,8 +3426,10 @@ int i915_gpu_idle(struct drm_device *dev)
 	struct intel_engine_cs *engine;
 	int ret;
 
-	/* Flush everything onto the inactive list. */
 	for_each_engine(engine, dev_priv) {
+		if (engine->last_context == NULL)
+			continue;
+
 		if (!i915.enable_execlists) {
 			struct drm_i915_gem_request *req;
 
-- 
2.8.1

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

[PATCH v2 2/6] drm/i915: Move legacy kernel context pinning to intel_ringbuffer.c

[Chris Wilson]

This is so that we have symmetry with intel_lrc.c and avoid a source of
if (i915.enable_execlists) layering violation within i915_gem_context.c -
that is we move the specific handling of the dev_priv->kernel_context
for legacy submission into the legacy submission code.

This depends upon the init/fini ordering between contexts and engines
already defined by intel_lrc.c, and also exporting the context alignment
required for pinning the legacy context.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e4c8e341655c..19d0194c728f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -867,6 +867,8 @@ struct i915_gem_context {
 	u32 user_handle;
 #define CONTEXT_NO_ZEROMAP		(1<<0)
 
+	u32 ggtt_alignment;
+
 	struct intel_context {
 		struct drm_i915_gem_object *state;
 		struct intel_ringbuffer *ringbuf;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index a3b11aac23a4..c620fe6c9d96 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -268,6 +268,8 @@ __create_hw_context(struct drm_device *dev,
 	list_add_tail(&ctx->link, &dev_priv->context_list);
 	ctx->i915 = dev_priv;
 
+	ctx->ggtt_alignment = get_context_alignment(dev_priv);
+
 	if (dev_priv->hw_context_size) {
 		struct drm_i915_gem_object *obj =
 				i915_gem_alloc_context_obj(dev, dev_priv->hw_context_size);
@@ -413,26 +415,6 @@ int i915_gem_context_init(struct drm_device *dev)
 		return PTR_ERR(ctx);
 	}
 
-	if (!i915.enable_execlists && ctx->engine[RCS].state) {
-		int ret;
-
-		/* We may need to do things with the shrinker which
-		 * require us to immediately switch back to the default
-		 * context. This can cause a problem as pinning the
-		 * default context also requires GTT space which may not
-		 * be available. To avoid this we always pin the default
-		 * context.
-		 */
-		ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state,
-					    get_context_alignment(dev_priv), 0);
-		if (ret) {
-			DRM_ERROR("Failed to pinned default global context (error %d)\n",
-				  ret);
-			i915_gem_context_unreference(ctx);
-			return ret;
-		}
-	}
-
 	dev_priv->kernel_context = ctx;
 
 	DRM_DEBUG_DRIVER("%s context support initialized\n",
@@ -469,9 +451,6 @@ void i915_gem_context_fini(struct drm_device *dev)
 
 	lockdep_assert_held(&dev->struct_mutex);
 
-	if (!i915.enable_execlists && dctx->engine[RCS].state)
-		i915_gem_object_ggtt_unpin(dctx->engine[RCS].state);
-
 	i915_gem_context_unreference(dctx);
 	dev_priv->kernel_context = NULL;
 
@@ -721,7 +700,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
 
 	/* Trying to pin first makes error handling easier. */
 	ret = i915_gem_obj_ggtt_pin(to->engine[RCS].state,
-				    get_context_alignment(engine->i915),
+				    to->ggtt_alignment,
 				    0);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 8d35a3978f9b..4e0aa7e9d5da 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2244,6 +2244,8 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 				  struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct i915_gem_context *kctx = dev_priv->kernel_context;
+	struct intel_context *ce = &kctx->engine[engine->id];
 	struct intel_ringbuffer *ringbuf;
 	int ret;
 
@@ -2260,6 +2262,25 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 
 	init_waitqueue_head(&engine->irq_queue);
 
+	if (ce->state) {
+		i915_gem_context_reference(kctx);
+
+		/* We may need to do things with the shrinker which
+		 * require us to immediately switch back to the default
+		 * context. This can cause a problem as pinning the
+		 * default context also requires GTT space which may not
+		 * be available. To avoid this we always pin the default
+		 * context.
+		 */
+		ret = i915_gem_obj_ggtt_pin(ce->state,
+					    kctx->ggtt_alignment,
+					    0);
+		if (ret)
+			goto error;
+
+		ce->initialised = false;
+	}
+
 	ringbuf = intel_engine_create_ringbuffer(engine, 32 * PAGE_SIZE);
 	if (IS_ERR(ringbuf)) {
 		ret = PTR_ERR(ringbuf);
@@ -2300,6 +2321,8 @@ error:
 void intel_cleanup_engine(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv;
+	struct i915_gem_context *kctx;
+	struct intel_context *ce;
 
 	if (!intel_engine_initialized(engine))
 		return;
@@ -2327,6 +2350,14 @@ void intel_cleanup_engine(struct intel_engine_cs *engine)
 
 	i915_cmd_parser_fini_ring(engine);
 	i915_gem_batch_pool_fini(&engine->batch_pool);
+
+	kctx = dev_priv->kernel_context;
+	ce = &kctx->engine[engine->id];
+	if (ce->state) {
+		i915_gem_object_ggtt_unpin(ce->state);
+		i915_gem_context_unreference(kctx);
+	}
+
 	engine->i915 = NULL;
 }
 
-- 
2.8.1

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

[PATCH v2 3/6] drm/i915: Treat kernel context as initialised

[Chris Wilson]

The kernel context exists simply as a placeholder and should never be
executed with a render context. It does not need the golden render
state, as that will always be applied to a user context. By skipping the
initialisation we can avoid issues in attempting to program the golden
render context when trying to make the hardware idle.

Testcase: igt/drm_module_reload_basic #byt
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95634
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 13 ++++++++-----
 drivers/gpu/drm/i915/intel_ringbuffer.c |  9 ++++++++-
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index c620fe6c9d96..46cb2c7fd158 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -434,13 +434,16 @@ void i915_gem_context_lost(struct drm_i915_private *dev_priv)
 			i915_gem_context_unpin(engine->last_context, engine);
 			engine->last_context = NULL;
 		}
-
-		/* Force the GPU state to be reinitialised on enabling */
-		dev_priv->kernel_context->engine[engine->id].initialised =
-			engine->init_context == NULL;
 	}
 
-	/* Force the GPU state to be reinitialised on enabling */
+	/* Force the GPU state to be restored on enabling */
+	for_each_engine(engine, dev_priv) {
+		struct intel_context *ce =
+			&dev_priv->kernel_context->engine[engine->id];
+
+		ce->initialised =
+			!i915.enable_execlists || engine->init_context == NULL;
+	}
 	dev_priv->kernel_context->remap_slice = ALL_L3_SLICES(dev_priv);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 4e0aa7e9d5da..203b7952052a 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2278,7 +2278,14 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 		if (ret)
 			goto error;
 
-		ce->initialised = false;
+		/* The kernel context is only used as a placeholder
+		 * for flushing the active context. It is never used
+		 * for submitting rendering and as such never requires
+		 * the golden render context, and so we can skip
+		 * emitting it when we switch to the kernel context
+		 * (during eviction).
+		 */
+		ce->initialised = true;
 	}
 
 	ringbuf = intel_engine_create_ringbuffer(engine, 32 * PAGE_SIZE);
-- 
2.8.1

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

[PATCH v2 4/6] drm/i915: Mark all default contexts as uninitialised after context loss

[Chris Wilson]

When the GPU is reset or state lost through suspend, every default
legacy context needs to reload their state - both the golden render
state and the L3 mapping. Only context images explicitly saved to memory
(i.e. all execlists and non-default legacy contexts) will retain their
state across the reset.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 46cb2c7fd158..7f29c8eb750a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -437,6 +437,20 @@ void i915_gem_context_lost(struct drm_i915_private *dev_priv)
 	}
 
 	/* Force the GPU state to be restored on enabling */
+	if (!i915.enable_execlists) {
+		struct i915_gem_context *ctx;
+
+		list_for_each_entry(ctx, &dev_priv->context_list, link) {
+			if (!i915_gem_context_is_default(ctx))
+				continue;
+
+			for_each_engine(engine, dev_priv)
+				ctx->engine[engine->id].initialised = false;
+
+			ctx->remap_slice = ALL_L3_SLICES(dev_priv);
+		}
+	}
+
 	for_each_engine(engine, dev_priv) {
 		struct intel_context *ce =
 			&dev_priv->kernel_context->engine[engine->id];
@@ -444,7 +458,6 @@ void i915_gem_context_lost(struct drm_i915_private *dev_priv)
 		ce->initialised =
 			!i915.enable_execlists || engine->init_context == NULL;
 	}
-	dev_priv->kernel_context->remap_slice = ALL_L3_SLICES(dev_priv);
 }
 
 void i915_gem_context_fini(struct drm_device *dev)
-- 
2.8.1

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

[PATCH v2 5/6] drm/i915: No need to wait for idle on L3 remap

[Chris Wilson]

As the L3 remapping is applied before the next execution, there is no
need to wait until all previous uses are idle, the application will not
occur any sooner.

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

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 02507bfc8def..a6e90fe05a1e 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -227,13 +227,6 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
 		}
 	}
 
-	ret = i915_gpu_idle(drm_dev);
-	if (ret) {
-		kfree(temp);
-		mutex_unlock(&drm_dev->struct_mutex);
-		return ret;
-	}
-
 	/* TODO: Ideally we really want a GPU reset here to make sure errors
 	 * aren't propagated. Since I cannot find a stable way to reset the GPU
 	 * at this point it is left as a TODO.
-- 
2.8.1

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

[PATCH v2 6/6] drm/i915: Split idling from forcing context switch

[Chris Wilson]

We only need to force a switch to the kernel context placeholder during
eviction. All other uses of i915_gpu_idle() just want to wait until
existing work on the GPU is idle. Rename i915_gpu_idle() to
i915_gem_wait_for_idle() to avoid any implications about "parking" the
context first.

v2: Tweak an error message if the wait fails for the ilk vtd w/a

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          |  2 +-
 drivers/gpu/drm/i915/i915_gem.c          | 20 +++----------
 drivers/gpu/drm/i915/i915_gem_evict.c    | 51 +++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_gem_gtt.c      |  4 +--
 drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
 6 files changed, 56 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index ac7e5692496d..615cef736356 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4941,7 +4941,7 @@ i915_drop_caches_set(void *data, u64 val)
 		return ret;
 
 	if (val & DROP_ACTIVE) {
-		ret = i915_gpu_idle(dev);
+		ret = i915_gem_wait_for_idle(dev_priv);
 		if (ret)
 			goto unlock;
 	}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 19d0194c728f..b9d9a4205992 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3285,7 +3285,7 @@ int i915_gem_init_engines(struct drm_device *dev);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
 void i915_gem_init_swizzling(struct drm_device *dev);
 void i915_gem_cleanup_engines(struct drm_device *dev);
-int __must_check i915_gpu_idle(struct drm_device *dev);
+int __must_check i915_gem_wait_for_idle(struct drm_i915_private *dev_priv);
 int __must_check i915_gem_suspend(struct drm_device *dev);
 void __i915_add_request(struct drm_i915_gem_request *req,
 			struct drm_i915_gem_object *batch_obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 61812a53e56d..e94758850c77 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3420,29 +3420,17 @@ int __i915_vma_unbind_no_wait(struct i915_vma *vma)
 	return __i915_vma_unbind(vma, false);
 }
 
-int i915_gpu_idle(struct drm_device *dev)
+int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	int ret;
 
+	lockdep_assert_held(&dev_priv->dev->struct_mutex);
+
 	for_each_engine(engine, dev_priv) {
 		if (engine->last_context == NULL)
 			continue;
 
-		if (!i915.enable_execlists) {
-			struct drm_i915_gem_request *req;
-
-			req = i915_gem_request_alloc(engine, NULL);
-			if (IS_ERR(req))
-				return PTR_ERR(req);
-
-			ret = i915_switch_context(req);
-			i915_add_request_no_flush(req);
-			if (ret)
-				return ret;
-		}
-
 		ret = intel_engine_idle(engine);
 		if (ret)
 			return ret;
@@ -4703,7 +4691,7 @@ i915_gem_suspend(struct drm_device *dev)
 	int ret = 0;
 
 	mutex_lock(&dev->struct_mutex);
-	ret = i915_gpu_idle(dev);
+	ret = i915_gem_wait_for_idle(dev_priv);
 	if (ret)
 		goto err;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index b144c3f5c650..5741b58d186c 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -33,6 +33,37 @@
 #include "intel_drv.h"
 #include "i915_trace.h"
 
+static int switch_to_pinned_context(struct drm_i915_private *dev_priv)
+{
+	struct intel_engine_cs *engine;
+
+	if (i915.enable_execlists)
+		return 0;
+
+	for_each_engine(engine, dev_priv) {
+		struct drm_i915_gem_request *req;
+		int ret;
+
+		if (engine->last_context == NULL)
+			continue;
+
+		if (engine->last_context == dev_priv->kernel_context)
+			continue;
+
+		req = i915_gem_request_alloc(engine, dev_priv->kernel_context);
+		if (IS_ERR(req))
+			return PTR_ERR(req);
+
+		ret = i915_switch_context(req);
+		i915_add_request_no_flush(req);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+
 static bool
 mark_free(struct i915_vma *vma, struct list_head *unwind)
 {
@@ -150,11 +181,17 @@ none:
 
 	/* Only idle the GPU and repeat the search once */
 	if (pass++ == 0) {
-		ret = i915_gpu_idle(dev);
+		struct drm_i915_private *dev_priv = to_i915(dev);
+
+		ret = switch_to_pinned_context(dev_priv);
 		if (ret)
 			return ret;
 
-		i915_gem_retire_requests(to_i915(dev));
+		ret = i915_gem_wait_for_idle(dev_priv);
+		if (ret)
+			return ret;
+
+		i915_gem_retire_requests(dev_priv);
 		goto search_again;
 	}
 
@@ -261,11 +298,17 @@ int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle)
 	trace_i915_gem_evict_vm(vm);
 
 	if (do_idle) {
-		ret = i915_gpu_idle(vm->dev);
+		struct drm_i915_private *dev_priv = to_i915(vm->dev);
+
+		ret = switch_to_pinned_context(dev_priv);
+		if (ret)
+			return ret;
+
+		ret = i915_gem_wait_for_idle(dev_priv);
 		if (ret)
 			return ret;
 
-		i915_gem_retire_requests(to_i915(vm->dev));
+		i915_gem_retire_requests(dev_priv);
 
 		WARN_ON(!list_empty(&vm->active_list));
 	}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 46684779d4d6..5860fb73c0e3 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2261,8 +2261,8 @@ static bool do_idling(struct drm_i915_private *dev_priv)
 
 	if (unlikely(ggtt->do_idle_maps)) {
 		dev_priv->mm.interruptible = false;
-		if (i915_gpu_idle(dev_priv->dev)) {
-			DRM_ERROR("Couldn't idle GPU\n");
+		if (i915_gem_wait_for_idle(dev_priv)) {
+			DRM_ERROR("Failed to wait for idle; VT'd may hang.\n");
 			/* Wait a bit, in hopes it avoids the hang */
 			udelay(10);
 		}
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 538c30499848..886a8797566d 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -408,7 +408,7 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
 		return NOTIFY_DONE;
 
 	/* Force everything onto the inactive lists */
-	ret = i915_gpu_idle(dev_priv->dev);
+	ret = i915_gem_wait_for_idle(dev_priv);
 	if (ret)
 		goto out;
 
-- 
2.8.1

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

Re: [PATCH 2/6] drm/i915: Treat kernel context as initialised

[Mika Kuoppala]

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

> [ text/plain ]
> The kernel context exists simply as a placeholder and should never be
> executed with a render context. It does not need the golden render
> state, as that will always be applied to a user context. By skipping the
> initialisation we can avoid issues in attempting to program the golden
> render context when trying to make the hardware idle.
>
> Testcase: igt/drm_module_reload_basic #byt
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95634
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 45 +++++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index a3b11aac23a4..3e3acd054f05 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -413,7 +413,8 @@ int i915_gem_context_init(struct drm_device *dev)
>  		return PTR_ERR(ctx);
>  	}
>  
> -	if (!i915.enable_execlists && ctx->engine[RCS].state) {
> +	if (!i915.enable_execlists) {
> +		struct intel_engine_cs *engine;
>  		int ret;
>  
>  		/* We may need to do things with the shrinker which
> @@ -423,13 +424,28 @@ int i915_gem_context_init(struct drm_device *dev)
>  		 * be available. To avoid this we always pin the default
>  		 * context.
>  		 */
> -		ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state,
> -					    get_context_alignment(dev_priv), 0);
> -		if (ret) {
> -			DRM_ERROR("Failed to pinned default global context (error %d)\n",
> -				  ret);
> -			i915_gem_context_unreference(ctx);
> -			return ret;
> +		for_each_engine(engine, dev_priv) {
> +			struct intel_context *ce = &ctx->engine[engine->id];
> +
> +			ret = 0;
> +			if (ce->state)
> +				ret = i915_gem_obj_ggtt_pin(ce->state,
> +							    get_context_alignment(dev_priv), 0);
> +			if (ret) {
> +				DRM_ERROR("Failed to pinned default global context (error %d)\n",
> +					  ret);
> +				i915_gem_context_unreference(ctx);
> +				return ret;
> +			}
> +
> +			/* The kernel context is only used as a placeholder
> +			 * for flushing the active context. It is never used
> +			 * for submitting rendering and as such never requires
> +			 * the golden render context, and so we can skip
> +			 * emitting it when we switch to the kernel context
> +			 * (during eviction).
> +			 */
> +			ce->initialised = true;

My concern here is that the bdw had/has problem with entering and
exiting from rc6 with a vanilla hw state. And if you have not
submitted anything, you need a working context to sleep on.

Thus golden context was created to be that working one to sleep
and wakeup on.

-Mika



>  		}
>  	}
>  
> @@ -452,13 +468,16 @@ void i915_gem_context_lost(struct drm_i915_private *dev_priv)
>  			i915_gem_context_unpin(engine->last_context, engine);
>  			engine->last_context = NULL;
>  		}
> -
> -		/* Force the GPU state to be reinitialised on enabling */
> -		dev_priv->kernel_context->engine[engine->id].initialised =
> -			engine->init_context == NULL;
>  	}
>  
> -	/* Force the GPU state to be reinitialised on enabling */
> +	/* Force the GPU state to be restore on enabling */
> +	for_each_engine(engine, dev_priv) {
> +		struct intel_context *ce =
> +			&dev_priv->kernel_context->engine[engine->id];
> +
> +		ce->initialised =
> +			!i915.enable_execlists || engine->init_context == NULL;
> +	}
>  	dev_priv->kernel_context->remap_slice = ALL_L3_SLICES(dev_priv);
>  }
>  
> -- 
> 2.8.1
>
> _______________________________________________
> 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 4/6] drm/i915: No need to wait for idle on L3 remap

[Mika Kuoppala]

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

> [ text/plain ]
> As the L3 remapping is applied before the next execution, there is no
> need to wait until all previous uses are idle, the application will not
> occur any sooner.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

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


> ---
>  drivers/gpu/drm/i915/i915_sysfs.c | 7 -------
>  1 file changed, 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 02507bfc8def..a6e90fe05a1e 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -227,13 +227,6 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
>  		}
>  	}
>  
> -	ret = i915_gpu_idle(drm_dev);
> -	if (ret) {
> -		kfree(temp);
> -		mutex_unlock(&drm_dev->struct_mutex);
> -		return ret;
> -	}
> -
>  	/* TODO: Ideally we really want a GPU reset here to make sure errors
>  	 * aren't propagated. Since I cannot find a stable way to reset the GPU
>  	 * at this point it is left as a TODO.
> -- 
> 2.8.1
>
> _______________________________________________
> 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/6] drm/i915: Treat kernel context as initialised

[Chris Wilson]

On Wed, May 25, 2016 at 05:16:56PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > [ text/plain ]
> > The kernel context exists simply as a placeholder and should never be
> > executed with a render context. It does not need the golden render
> > state, as that will always be applied to a user context. By skipping the
> > initialisation we can avoid issues in attempting to program the golden
> > render context when trying to make the hardware idle.
> >
> > Testcase: igt/drm_module_reload_basic #byt
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95634
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 45 +++++++++++++++++++++++----------
> >  1 file changed, 32 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index a3b11aac23a4..3e3acd054f05 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -413,7 +413,8 @@ int i915_gem_context_init(struct drm_device *dev)
> >  		return PTR_ERR(ctx);
> >  	}
> >  
> > -	if (!i915.enable_execlists && ctx->engine[RCS].state) {
> > +	if (!i915.enable_execlists) {
> > +		struct intel_engine_cs *engine;
> >  		int ret;
> >  
> >  		/* We may need to do things with the shrinker which
> > @@ -423,13 +424,28 @@ int i915_gem_context_init(struct drm_device *dev)
> >  		 * be available. To avoid this we always pin the default
> >  		 * context.
> >  		 */
> > -		ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state,
> > -					    get_context_alignment(dev_priv), 0);
> > -		if (ret) {
> > -			DRM_ERROR("Failed to pinned default global context (error %d)\n",
> > -				  ret);
> > -			i915_gem_context_unreference(ctx);
> > -			return ret;
> > +		for_each_engine(engine, dev_priv) {
> > +			struct intel_context *ce = &ctx->engine[engine->id];
> > +
> > +			ret = 0;
> > +			if (ce->state)
> > +				ret = i915_gem_obj_ggtt_pin(ce->state,
> > +							    get_context_alignment(dev_priv), 0);
> > +			if (ret) {
> > +				DRM_ERROR("Failed to pinned default global context (error %d)\n",
> > +					  ret);
> > +				i915_gem_context_unreference(ctx);
> > +				return ret;
> > +			}
> > +
> > +			/* The kernel context is only used as a placeholder
> > +			 * for flushing the active context. It is never used
> > +			 * for submitting rendering and as such never requires
> > +			 * the golden render context, and so we can skip
> > +			 * emitting it when we switch to the kernel context
> > +			 * (during eviction).
> > +			 */
> > +			ce->initialised = true;
> 
> My concern here is that the bdw had/has problem with entering and
> exiting from rc6 with a vanilla hw state. And if you have not
> submitted anything, you need a working context to sleep on.

This patch doesn't alter the fact that no context is set prior to rc6
being enabled. Nor have we been setting one for bdw for almost 2 years.
-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

[PATCH] drm/i915: Defer enabling rc6 til after we submit the first batch/context

[Chris Wilson]

Some hardware requires a valid render context before it can initiate
rc6 power gating of the GPU; the default state of the GPU is not
sufficient and may lead to undefined behaviour. The first execution of
any batch will load the "golden render state", at which point it is safe
to enable rc6.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 12 ----------
 drivers/gpu/drm/i915/i915_drv.c      |  2 --
 drivers/gpu/drm/i915/i915_drv.h      |  1 -
 drivers/gpu/drm/i915/i915_sysfs.c    | 12 ----------
 drivers/gpu/drm/i915/intel_display.c |  2 +-
 drivers/gpu/drm/i915/intel_pm.c      | 46 +++++++++---------------------------
 6 files changed, 12 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 615cef736356..ae097c5d2b55 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1193,8 +1193,6 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
 
 	intel_runtime_pm_get(dev_priv);
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	if (IS_GEN5(dev)) {
 		u16 rgvswctl = I915_READ16(MEMSWCTL);
 		u16 rgvstat = I915_READ16(MEMSTAT_ILK);
@@ -1881,8 +1879,6 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
 
 	intel_runtime_pm_get(dev_priv);
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
 	if (ret)
 		goto out;
@@ -4975,8 +4971,6 @@ i915_max_freq_get(void *data, u64 *val)
 	if (INTEL_INFO(dev)->gen < 6)
 		return -ENODEV;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
 	if (ret)
 		return ret;
@@ -4998,8 +4992,6 @@ i915_max_freq_set(void *data, u64 val)
 	if (INTEL_INFO(dev)->gen < 6)
 		return -ENODEV;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	DRM_DEBUG_DRIVER("Manually setting max freq to %llu\n", val);
 
 	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
@@ -5042,8 +5034,6 @@ i915_min_freq_get(void *data, u64 *val)
 	if (INTEL_INFO(dev)->gen < 6)
 		return -ENODEV;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
 	if (ret)
 		return ret;
@@ -5065,8 +5055,6 @@ i915_min_freq_set(void *data, u64 val)
 	if (INTEL_INFO(dev)->gen < 6)
 		return -ENODEV;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	DRM_DEBUG_DRIVER("Manually setting min freq to %llu\n", val);
 
 	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 943d7b222fd4..6f685b665865 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1675,8 +1675,6 @@ static int intel_runtime_resume(struct device *device)
 	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
 		intel_hpd_init(dev_priv);
 
-	intel_enable_gt_powersave(dev_priv);
-
 	enable_rpm_wakeref_asserts(dev_priv);
 
 	if (ret)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b9d9a4205992..5fed037097c8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1161,7 +1161,6 @@ struct intel_gen6_power_mgmt {
 	bool client_boost;
 
 	bool enabled;
-	struct delayed_work delayed_resume_work;
 	unsigned boosts;
 
 	struct intel_rps_client semaphores, mmioflips;
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index a6e90fe05a1e..679c3d54bd8f 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -271,8 +271,6 @@ static ssize_t gt_act_freq_mhz_show(struct device *kdev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	intel_runtime_pm_get(dev_priv);
 
 	mutex_lock(&dev_priv->rps.hw_lock);
@@ -305,8 +303,6 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	intel_runtime_pm_get(dev_priv);
 
 	mutex_lock(&dev_priv->rps.hw_lock);
@@ -337,8 +333,6 @@ static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	mutex_lock(&dev_priv->rps.hw_lock);
 	ret = intel_gpu_freq(dev_priv, dev_priv->rps.max_freq_softlimit);
 	mutex_unlock(&dev_priv->rps.hw_lock);
@@ -360,8 +354,6 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev,
 	if (ret)
 		return ret;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	intel_runtime_pm_get(dev_priv);
 
 	mutex_lock(&dev_priv->rps.hw_lock);
@@ -405,8 +397,6 @@ static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	mutex_lock(&dev_priv->rps.hw_lock);
 	ret = intel_gpu_freq(dev_priv, dev_priv->rps.min_freq_softlimit);
 	mutex_unlock(&dev_priv->rps.hw_lock);
@@ -428,8 +418,6 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev,
 	if (ret)
 		return ret;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	intel_runtime_pm_get(dev_priv);
 
 	mutex_lock(&dev_priv->rps.hw_lock);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9ccd76699f48..dbb81c1b52a5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10910,6 +10910,7 @@ void intel_mark_busy(struct drm_i915_private *dev_priv)
 
 	intel_runtime_pm_get(dev_priv);
 	i915_update_gfx_val(dev_priv);
+	intel_enable_gt_powersave(dev_priv);
 	if (INTEL_GEN(dev_priv) >= 6)
 		gen6_rps_busy(dev_priv);
 	dev_priv->mm.busy = true;
@@ -15365,7 +15366,6 @@ void intel_modeset_init_hw(struct drm_device *dev)
 	dev_priv->atomic_cdclk_freq = dev_priv->cdclk_freq;
 
 	intel_init_clock_gating(dev);
-	intel_enable_gt_powersave(dev_priv);
 }
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b6dfd0264950..23d5e0218417 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6389,8 +6389,6 @@ void intel_cleanup_gt_powersave(struct drm_i915_private *dev_priv)
 
 static void gen6_suspend_rps(struct drm_i915_private *dev_priv)
 {
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	gen6_disable_rps_interrupts(dev_priv);
 }
 
@@ -6436,14 +6434,8 @@ void intel_disable_gt_powersave(struct drm_i915_private *dev_priv)
 	}
 }
 
-static void intel_gen6_powersave_work(struct work_struct *work)
+static void gen6_enable_gt_powersave(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv =
-		container_of(work, struct drm_i915_private,
-			     rps.delayed_resume_work.work);
-
-	mutex_lock(&dev_priv->rps.hw_lock);
-
 	gen6_reset_rps_interrupts(dev_priv);
 
 	if (IS_CHERRYVIEW(dev_priv)) {
@@ -6469,43 +6461,29 @@ static void intel_gen6_powersave_work(struct work_struct *work)
 	WARN_ON(dev_priv->rps.efficient_freq < dev_priv->rps.min_freq);
 	WARN_ON(dev_priv->rps.efficient_freq > dev_priv->rps.max_freq);
 
-	dev_priv->rps.enabled = true;
-
 	gen6_enable_rps_interrupts(dev_priv);
-
-	mutex_unlock(&dev_priv->rps.hw_lock);
-
-	intel_runtime_pm_put(dev_priv);
 }
 
 void intel_enable_gt_powersave(struct drm_i915_private *dev_priv)
 {
+	if (READ_ONCE(dev_priv->rps.enabled))
+		return;
+
 	/* Powersaving is controlled by the host when inside a VM */
 	if (intel_vgpu_active(dev_priv))
 		return;
 
+	mutex_lock(&dev_priv->rps.hw_lock);
+
 	if (IS_IRONLAKE_M(dev_priv)) {
 		ironlake_enable_drps(dev_priv);
-		mutex_lock(&dev_priv->dev->struct_mutex);
 		intel_init_emon(dev_priv);
-		mutex_unlock(&dev_priv->dev->struct_mutex);
-	} else if (INTEL_INFO(dev_priv)->gen >= 6) {
-		/*
-		 * PCU communication is slow and this doesn't need to be
-		 * done at any specific time, so do this out of our fast path
-		 * to make resume and init faster.
-		 *
-		 * We depend on the HW RC6 power context save/restore
-		 * mechanism when entering D3 through runtime PM suspend. So
-		 * disable RPM until RPS/RC6 is properly setup. We can only
-		 * get here via the driver load/system resume/runtime resume
-		 * paths, so the _noresume version is enough (and in case of
-		 * runtime resume it's necessary).
-		 */
-		if (schedule_delayed_work(&dev_priv->rps.delayed_resume_work,
-					   round_jiffies_up_relative(HZ)))
-			intel_runtime_pm_get_noresume(dev_priv);
+	} else if (INTEL_GEN(dev_priv) >= 6) {
+		gen6_enable_gt_powersave(dev_priv);
 	}
+
+	dev_priv->rps.enabled = true;
+	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
 void intel_reset_gt_powersave(struct drm_i915_private *dev_priv)
@@ -7579,8 +7557,6 @@ void intel_pm_setup(struct drm_device *dev)
 	mutex_init(&dev_priv->rps.hw_lock);
 	spin_lock_init(&dev_priv->rps.client_lock);
 
-	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
-			  intel_gen6_powersave_work);
 	INIT_LIST_HEAD(&dev_priv->rps.clients);
 	INIT_LIST_HEAD(&dev_priv->rps.semaphores.link);
 	INIT_LIST_HEAD(&dev_priv->rps.mmioflips.link);
-- 
2.8.1

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

✗ Ro.CI.BAT: failure for series starting with [1/6] drm/i915: Skip idling an idle engine (rev6)

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915: Skip idling an idle engine (rev6)
URL   : https://patchwork.freedesktop.org/series/7708/
State : failure

== Summary ==

Series 7708v6 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/7708/revisions/6/mbox

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-cmd:
                pass       -> FAIL       (ro-byt-n2820)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (ro-bdw-i7-5600u)
                skip       -> PASS       (fi-skl-i5-6260u)

fi-bdw-i7-5557u  total:209  pass:197  dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-n2820     total:209  pass:168  dwarn:0   dfail:0   fail:3   skip:38 
fi-hsw-i7-4770k  total:209  pass:190  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-i7-4770r  total:209  pass:186  dwarn:0   dfail:0   fail:0   skip:23 
fi-skl-i5-6260u  total:209  pass:198  dwarn:0   dfail:0   fail:0   skip:11 
fi-skl-i7-6700k  total:209  pass:184  dwarn:0   dfail:0   fail:0   skip:25 
fi-snb-i7-2600   total:209  pass:170  dwarn:0   dfail:0   fail:0   skip:39 
ro-bdw-i5-5250u  total:209  pass:172  dwarn:0   dfail:0   fail:0   skip:37 
ro-bdw-i7-5557U  total:209  pass:197  dwarn:0   dfail:0   fail:0   skip:12 
ro-bdw-i7-5600u  total:209  pass:181  dwarn:0   dfail:0   fail:0   skip:28 
ro-bsw-n3050     total:209  pass:167  dwarn:1   dfail:0   fail:2   skip:39 
ro-byt-n2820     total:209  pass:169  dwarn:0   dfail:0   fail:3   skip:37 
ro-hsw-i3-4010u  total:209  pass:186  dwarn:0   dfail:0   fail:0   skip:23 
ro-hsw-i7-4770r  total:209  pass:186  dwarn:0   dfail:0   fail:0   skip:23 
ro-ilk-i7-620lm  total:209  pass:146  dwarn:0   dfail:0   fail:1   skip:62 
ro-ilk1-i5-650   total:204  pass:146  dwarn:0   dfail:0   fail:1   skip:57 
ro-ivb-i7-3770   total:209  pass:177  dwarn:0   dfail:0   fail:0   skip:32 
ro-ivb2-i7-3770  total:209  pass:181  dwarn:0   dfail:0   fail:0   skip:28 
ro-snb-i7-2620M  total:209  pass:170  dwarn:0   dfail:0   fail:1   skip:38 
fi-bsw-n3050 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1010/

fc9d741 drm-intel-nightly: 2016y-05m-25d-07h-45m-48s UTC integration manifest
1808b12 drm/i915: Split idling from forcing context switch
fb05349 drm/i915: No need to wait for idle on L3 remap
4eadec1c drm/i915: Mark all default contexts as uninitialised after context loss
a2b8799 drm/i915: Treat kernel context as initialised
8fa7ef0 drm/i915: Move legacy kernel context pinning to intel_ringbuffer.c
b8fa169 drm/i915: Skip idling an idle engine

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

✗ Ro.CI.BAT: failure for series starting with [1/6] drm/i915: Skip idling an idle engine (rev7)

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915: Skip idling an idle engine (rev7)
URL   : https://patchwork.freedesktop.org/series/7708/
State : failure

== Summary ==

Applying: drm/i915: Skip idling an idle engine
Applying: drm/i915: Defer enabling rc6 til after we submit the first batch/context
Applying: drm/i915: Treat kernel context as initialised
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_gem_context.c
M	drivers/gpu/drm/i915/intel_ringbuffer.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_ringbuffer.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_ringbuffer.c
Auto-merging drivers/gpu/drm/i915/i915_gem_context.c
Patch failed at 0003 drm/i915: Treat kernel context as initialised
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

Re: [PATCH] drm/i915: Defer enabling rc6 til after we submit the first batch/context

[Chris Wilson]

On Wed, May 25, 2016 at 03:58:11PM +0100, Chris Wilson wrote:
> Some hardware requires a valid render context before it can initiate
> rc6 power gating of the GPU; the default state of the GPU is not
> sufficient and may lead to undefined behaviour. The first execution of
> any batch will load the "golden render state", at which point it is safe
> to enable rc6.

The big issue with first batch, is that it may not be RCS and so not
define the golden state and requires the user to do something for us to
save power.

The other approach is for the powersaving to send the batch itself if it
needs it.
-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

[PATCH] drm/i915: Defer enabling rc6 til after we submit the first batch/context

[Chris Wilson]

Some hardware requires a valid render context before it can initiate
rc6 power gating of the GPU; the default state of the GPU is not
sufficient and may lead to undefined behaviour. The first execution of
any batch will load the "golden render state", at which point it is safe
to enable rc6.

The major downside is that rc6 is then not enabled (the GPU is left at
low frequency) until userspace executes. The counter argument is that
without the driver, the hardware would be left out of rc6 (but this is a
lesser of two evils argument). Still, if someone notices...

The fix is as before to schedule a delayed batch that switches to kernel
context and emits the golden state. The submission will then enable rc6.

v2: Rearrange intel_disable_gt_powersave() to match.
v3: Apply user specified cur_freq (or idle_freq if not set).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  12 ---
 drivers/gpu/drm/i915/i915_drv.c      |   2 -
 drivers/gpu/drm/i915/i915_drv.h      |   1 -
 drivers/gpu/drm/i915/i915_sysfs.c    |  12 ---
 drivers/gpu/drm/i915/intel_display.c |   3 +-
 drivers/gpu/drm/i915/intel_drv.h     |   1 +
 drivers/gpu/drm/i915/intel_pm.c      | 161 ++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_uncore.c  |   2 +-
 8 files changed, 78 insertions(+), 116 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 615cef736356..ae097c5d2b55 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1193,8 +1193,6 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
 
 	intel_runtime_pm_get(dev_priv);
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	if (IS_GEN5(dev)) {
 		u16 rgvswctl = I915_READ16(MEMSWCTL);
 		u16 rgvstat = I915_READ16(MEMSTAT_ILK);
@@ -1881,8 +1879,6 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
 
 	intel_runtime_pm_get(dev_priv);
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
 	if (ret)
 		goto out;
@@ -4975,8 +4971,6 @@ i915_max_freq_get(void *data, u64 *val)
 	if (INTEL_INFO(dev)->gen < 6)
 		return -ENODEV;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
 	if (ret)
 		return ret;
@@ -4998,8 +4992,6 @@ i915_max_freq_set(void *data, u64 val)
 	if (INTEL_INFO(dev)->gen < 6)
 		return -ENODEV;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	DRM_DEBUG_DRIVER("Manually setting max freq to %llu\n", val);
 
 	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
@@ -5042,8 +5034,6 @@ i915_min_freq_get(void *data, u64 *val)
 	if (INTEL_INFO(dev)->gen < 6)
 		return -ENODEV;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
 	if (ret)
 		return ret;
@@ -5065,8 +5055,6 @@ i915_min_freq_set(void *data, u64 val)
 	if (INTEL_INFO(dev)->gen < 6)
 		return -ENODEV;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	DRM_DEBUG_DRIVER("Manually setting min freq to %llu\n", val);
 
 	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 943d7b222fd4..6f685b665865 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1675,8 +1675,6 @@ static int intel_runtime_resume(struct device *device)
 	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
 		intel_hpd_init(dev_priv);
 
-	intel_enable_gt_powersave(dev_priv);
-
 	enable_rpm_wakeref_asserts(dev_priv);
 
 	if (ret)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b9d9a4205992..5fed037097c8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1161,7 +1161,6 @@ struct intel_gen6_power_mgmt {
 	bool client_boost;
 
 	bool enabled;
-	struct delayed_work delayed_resume_work;
 	unsigned boosts;
 
 	struct intel_rps_client semaphores, mmioflips;
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index a6e90fe05a1e..679c3d54bd8f 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -271,8 +271,6 @@ static ssize_t gt_act_freq_mhz_show(struct device *kdev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	intel_runtime_pm_get(dev_priv);
 
 	mutex_lock(&dev_priv->rps.hw_lock);
@@ -305,8 +303,6 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	intel_runtime_pm_get(dev_priv);
 
 	mutex_lock(&dev_priv->rps.hw_lock);
@@ -337,8 +333,6 @@ static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	mutex_lock(&dev_priv->rps.hw_lock);
 	ret = intel_gpu_freq(dev_priv, dev_priv->rps.max_freq_softlimit);
 	mutex_unlock(&dev_priv->rps.hw_lock);
@@ -360,8 +354,6 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev,
 	if (ret)
 		return ret;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	intel_runtime_pm_get(dev_priv);
 
 	mutex_lock(&dev_priv->rps.hw_lock);
@@ -405,8 +397,6 @@ static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	mutex_lock(&dev_priv->rps.hw_lock);
 	ret = intel_gpu_freq(dev_priv, dev_priv->rps.min_freq_softlimit);
 	mutex_unlock(&dev_priv->rps.hw_lock);
@@ -428,8 +418,6 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev,
 	if (ret)
 		return ret;
 
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
 	intel_runtime_pm_get(dev_priv);
 
 	mutex_lock(&dev_priv->rps.hw_lock);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9ccd76699f48..e024ad422338 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10909,6 +10909,7 @@ void intel_mark_busy(struct drm_i915_private *dev_priv)
 		return;
 
 	intel_runtime_pm_get(dev_priv);
+	intel_enable_gt_powersave(dev_priv);
 	i915_update_gfx_val(dev_priv);
 	if (INTEL_GEN(dev_priv) >= 6)
 		gen6_rps_busy(dev_priv);
@@ -15365,7 +15366,6 @@ void intel_modeset_init_hw(struct drm_device *dev)
 	dev_priv->atomic_cdclk_freq = dev_priv->cdclk_freq;
 
 	intel_init_clock_gating(dev);
-	intel_enable_gt_powersave(dev_priv);
 }
 
 /*
@@ -16148,6 +16148,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_connector *connector;
 
+	intel_suspend_gt_powersave(dev_priv);
 	intel_disable_gt_powersave(dev_priv);
 
 	intel_backlight_unregister(dev);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9b5f6634c558..4d2cf2d7df14 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1641,6 +1641,7 @@ void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 void intel_gpu_ips_teardown(void);
 void intel_init_gt_powersave(struct drm_i915_private *dev_priv);
 void intel_cleanup_gt_powersave(struct drm_i915_private *dev_priv);
+void intel_sanitize_gt_powersave(struct drm_i915_private *dev_priv);
 void intel_enable_gt_powersave(struct drm_i915_private *dev_priv);
 void intel_disable_gt_powersave(struct drm_i915_private *dev_priv);
 void intel_suspend_gt_powersave(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b6dfd0264950..5734f2f2b05a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4954,6 +4954,7 @@ static void gen6_init_rps_frequencies(struct drm_i915_private *dev_priv)
 	}
 
 	dev_priv->rps.idle_freq = dev_priv->rps.min_freq;
+	dev_priv->rps.cur_freq = dev_priv->rps.idle_freq;
 
 	/* Preserve min/max settings in case of re-init */
 	if (dev_priv->rps.max_freq_softlimit == 0)
@@ -4973,6 +4974,8 @@ static void gen6_init_rps_frequencies(struct drm_i915_private *dev_priv)
 /* See the Gen9_GT_PM_Programming_Guide doc for the below */
 static void gen9_enable_rps(struct drm_i915_private *dev_priv)
 {
+	u8 freq;
+
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 
 	gen6_init_rps_frequencies(dev_priv);
@@ -5006,8 +5009,10 @@ static void gen9_enable_rps(struct drm_i915_private *dev_priv)
 	/* Leaning on the below call to gen6_set_rps to program/setup the
 	 * Up/Down EI & threshold registers, as well as the RP_CONTROL,
 	 * RP_INTERRUPT_LIMITS & RPNSWREQ registers */
-	dev_priv->rps.power = HIGH_POWER; /* force a reset */
-	gen6_set_rps(dev_priv, dev_priv->rps.idle_freq);
+	freq = dev_priv->rps.cur_freq; /* force a reset */
+	dev_priv->rps.power = -1;
+	dev_priv->rps.cur_freq = -1;
+	gen6_set_rps(dev_priv, freq);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
@@ -5083,6 +5088,7 @@ static void gen8_enable_rps(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	uint32_t rc6_mask = 0;
+	u8 freq;
 
 	/* 1a: Software RC state - RC0 */
 	I915_WRITE(GEN6_RC_STATE, 0);
@@ -5153,8 +5159,10 @@ static void gen8_enable_rps(struct drm_i915_private *dev_priv)
 
 	/* 6: Ring frequency + overclocking (our driver does this later */
 
-	dev_priv->rps.power = HIGH_POWER; /* force a reset */
-	gen6_set_rps(dev_priv, dev_priv->rps.idle_freq);
+	freq = dev_priv->rps.cur_freq; /* force a reset */
+	dev_priv->rps.power = -1;
+	dev_priv->rps.cur_freq = -1;
+	gen6_set_rps(dev_priv, freq);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
@@ -5165,6 +5173,7 @@ static void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	u32 rc6vids, pcu_mbox = 0, rc6_mask = 0;
 	u32 gtfifodbg;
 	int rc6_mode;
+	u8 freq;
 	int ret;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
@@ -5247,8 +5256,10 @@ static void gen6_enable_rps(struct drm_i915_private *dev_priv)
 		dev_priv->rps.max_freq = pcu_mbox & 0xff;
 	}
 
-	dev_priv->rps.power = HIGH_POWER; /* force a reset */
-	gen6_set_rps(dev_priv, dev_priv->rps.idle_freq);
+	freq = dev_priv->rps.cur_freq; /* force a reset */
+	dev_priv->rps.power = -1;
+	dev_priv->rps.cur_freq = -1;
+	gen6_set_rps(dev_priv, freq);
 
 	rc6vids = 0;
 	ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
@@ -5612,6 +5623,7 @@ static void valleyview_init_gt_powersave(struct drm_i915_private *dev_priv)
 			 dev_priv->rps.min_freq);
 
 	dev_priv->rps.idle_freq = dev_priv->rps.min_freq;
+	dev_priv->rps.cur_freq = dev_priv->rps.idle_freq;
 
 	/* Preserve min/max settings in case of re-init */
 	if (dev_priv->rps.max_freq_softlimit == 0)
@@ -5676,6 +5688,7 @@ static void cherryview_init_gt_powersave(struct drm_i915_private *dev_priv)
 		  "Odd GPU freq values\n");
 
 	dev_priv->rps.idle_freq = dev_priv->rps.min_freq;
+	dev_priv->rps.cur_freq = dev_priv->rps.idle_freq;
 
 	/* Preserve min/max settings in case of re-init */
 	if (dev_priv->rps.max_freq_softlimit == 0)
@@ -5696,6 +5709,7 @@ static void cherryview_enable_rps(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	u32 gtfifodbg, val, rc6_mode = 0, pcbr;
+	u8 freq;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
@@ -5784,7 +5798,10 @@ static void cherryview_enable_rps(struct drm_i915_private *dev_priv)
 			 intel_gpu_freq(dev_priv, dev_priv->rps.idle_freq),
 			 dev_priv->rps.idle_freq);
 
-	valleyview_set_rps(dev_priv, dev_priv->rps.idle_freq);
+	freq = dev_priv->rps.cur_freq; /* force a reset */
+	dev_priv->rps.power = -1;
+	dev_priv->rps.cur_freq = -1;
+	valleyview_set_rps(dev_priv, freq);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
@@ -5793,6 +5810,7 @@ static void valleyview_enable_rps(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	u32 gtfifodbg, val, rc6_mode = 0;
+	u8 freq;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
@@ -5864,16 +5882,10 @@ static void valleyview_enable_rps(struct drm_i915_private *dev_priv)
 	DRM_DEBUG_DRIVER("GPLL enabled? %s\n", yesno(val & GPLLENABLE));
 	DRM_DEBUG_DRIVER("GPU status: 0x%08x\n", val);
 
-	dev_priv->rps.cur_freq = (val >> 8) & 0xff;
-	DRM_DEBUG_DRIVER("current GPU freq: %d MHz (%u)\n",
-			 intel_gpu_freq(dev_priv, dev_priv->rps.cur_freq),
-			 dev_priv->rps.cur_freq);
-
-	DRM_DEBUG_DRIVER("setting GPU freq to %d MHz (%u)\n",
-			 intel_gpu_freq(dev_priv, dev_priv->rps.idle_freq),
-			 dev_priv->rps.idle_freq);
-
-	valleyview_set_rps(dev_priv, dev_priv->rps.idle_freq);
+	freq = dev_priv->rps.cur_freq; /* force a reset */
+	dev_priv->rps.power = -1;
+	dev_priv->rps.cur_freq = -1;
+	valleyview_set_rps(dev_priv, freq);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
@@ -6387,13 +6399,6 @@ void intel_cleanup_gt_powersave(struct drm_i915_private *dev_priv)
 		intel_runtime_pm_put(dev_priv);
 }
 
-static void gen6_suspend_rps(struct drm_i915_private *dev_priv)
-{
-	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
-
-	gen6_disable_rps_interrupts(dev_priv);
-}
-
 /**
  * intel_suspend_gt_powersave - suspend PM work and helper threads
  * @dev_priv: i915 device
@@ -6407,50 +6412,63 @@ void intel_suspend_gt_powersave(struct drm_i915_private *dev_priv)
 	if (INTEL_GEN(dev_priv) < 6)
 		return;
 
-	gen6_suspend_rps(dev_priv);
+	gen6_disable_rps_interrupts(dev_priv);
 
 	/* Force GPU to min freq during suspend */
 	gen6_rps_idle(dev_priv);
 }
 
+void intel_sanitize_gt_powersave(struct drm_i915_private *dev_priv)
+{
+	dev_priv->rps.enabled = true; /* force disabling */
+	intel_disable_gt_powersave(dev_priv);
+
+	gen6_reset_rps_interrupts(dev_priv);
+}
+
 void intel_disable_gt_powersave(struct drm_i915_private *dev_priv)
 {
-	if (IS_IRONLAKE_M(dev_priv)) {
-		ironlake_disable_drps(dev_priv);
-	} else if (INTEL_INFO(dev_priv)->gen >= 6) {
-		intel_suspend_gt_powersave(dev_priv);
+	if (!READ_ONCE(dev_priv->rps.enabled))
+		return;
 
-		mutex_lock(&dev_priv->rps.hw_lock);
-		if (INTEL_INFO(dev_priv)->gen >= 9) {
-			gen9_disable_rc6(dev_priv);
-			gen9_disable_rps(dev_priv);
-		} else if (IS_CHERRYVIEW(dev_priv))
-			cherryview_disable_rps(dev_priv);
-		else if (IS_VALLEYVIEW(dev_priv))
-			valleyview_disable_rps(dev_priv);
-		else
-			gen6_disable_rps(dev_priv);
+	mutex_lock(&dev_priv->rps.hw_lock);
 
-		dev_priv->rps.enabled = false;
-		mutex_unlock(&dev_priv->rps.hw_lock);
+	if (INTEL_GEN(dev_priv) >= 9) {
+		gen9_disable_rc6(dev_priv);
+		gen9_disable_rps(dev_priv);
+	} else if (IS_CHERRYVIEW(dev_priv)) {
+		cherryview_disable_rps(dev_priv);
+	} else if (IS_VALLEYVIEW(dev_priv)) {
+		valleyview_disable_rps(dev_priv);
+	} else if (INTEL_GEN(dev_priv) >= 6) {
+		gen6_disable_rps(dev_priv);
+	}  else if (IS_IRONLAKE_M(dev_priv)) {
+		ironlake_disable_drps(dev_priv);
 	}
+
+	dev_priv->rps.enabled = false;
+	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
-static void intel_gen6_powersave_work(struct work_struct *work)
+void intel_enable_gt_powersave(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv =
-		container_of(work, struct drm_i915_private,
-			     rps.delayed_resume_work.work);
+	/* We shouldn't be disabling as we submit, so this should be less
+	 * racy than it appears!
+	 */
+	if (READ_ONCE(dev_priv->rps.enabled))
+		return;
 
-	mutex_lock(&dev_priv->rps.hw_lock);
+	/* Powersaving is controlled by the host when inside a VM */
+	if (intel_vgpu_active(dev_priv))
+		return;
 
-	gen6_reset_rps_interrupts(dev_priv);
+	mutex_lock(&dev_priv->rps.hw_lock);
 
 	if (IS_CHERRYVIEW(dev_priv)) {
 		cherryview_enable_rps(dev_priv);
 	} else if (IS_VALLEYVIEW(dev_priv)) {
 		valleyview_enable_rps(dev_priv);
-	} else if (INTEL_INFO(dev_priv)->gen >= 9) {
+	} else if (INTEL_GEN(dev_priv) >= 9) {
 		gen9_enable_rc6(dev_priv);
 		gen9_enable_rps(dev_priv);
 		if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
@@ -6458,9 +6476,12 @@ static void intel_gen6_powersave_work(struct work_struct *work)
 	} else if (IS_BROADWELL(dev_priv)) {
 		gen8_enable_rps(dev_priv);
 		__gen6_update_ring_freq(dev_priv);
-	} else {
+	} else if (INTEL_GEN(dev_priv) >= 6) {
 		gen6_enable_rps(dev_priv);
 		__gen6_update_ring_freq(dev_priv);
+	} else if (IS_IRONLAKE_M(dev_priv)) {
+		ironlake_enable_drps(dev_priv);
+		intel_init_emon(dev_priv);
 	}
 
 	WARN_ON(dev_priv->rps.max_freq < dev_priv->rps.min_freq);
@@ -6469,43 +6490,11 @@ static void intel_gen6_powersave_work(struct work_struct *work)
 	WARN_ON(dev_priv->rps.efficient_freq < dev_priv->rps.min_freq);
 	WARN_ON(dev_priv->rps.efficient_freq > dev_priv->rps.max_freq);
 
-	dev_priv->rps.enabled = true;
-
-	gen6_enable_rps_interrupts(dev_priv);
+	if (INTEL_GEN(dev_priv) >= 6)
+		gen6_enable_rps_interrupts(dev_priv);
 
+	dev_priv->rps.enabled = true;
 	mutex_unlock(&dev_priv->rps.hw_lock);
-
-	intel_runtime_pm_put(dev_priv);
-}
-
-void intel_enable_gt_powersave(struct drm_i915_private *dev_priv)
-{
-	/* Powersaving is controlled by the host when inside a VM */
-	if (intel_vgpu_active(dev_priv))
-		return;
-
-	if (IS_IRONLAKE_M(dev_priv)) {
-		ironlake_enable_drps(dev_priv);
-		mutex_lock(&dev_priv->dev->struct_mutex);
-		intel_init_emon(dev_priv);
-		mutex_unlock(&dev_priv->dev->struct_mutex);
-	} else if (INTEL_INFO(dev_priv)->gen >= 6) {
-		/*
-		 * PCU communication is slow and this doesn't need to be
-		 * done at any specific time, so do this out of our fast path
-		 * to make resume and init faster.
-		 *
-		 * We depend on the HW RC6 power context save/restore
-		 * mechanism when entering D3 through runtime PM suspend. So
-		 * disable RPM until RPS/RC6 is properly setup. We can only
-		 * get here via the driver load/system resume/runtime resume
-		 * paths, so the _noresume version is enough (and in case of
-		 * runtime resume it's necessary).
-		 */
-		if (schedule_delayed_work(&dev_priv->rps.delayed_resume_work,
-					   round_jiffies_up_relative(HZ)))
-			intel_runtime_pm_get_noresume(dev_priv);
-	}
 }
 
 void intel_reset_gt_powersave(struct drm_i915_private *dev_priv)
@@ -6513,7 +6502,7 @@ void intel_reset_gt_powersave(struct drm_i915_private *dev_priv)
 	if (INTEL_INFO(dev_priv)->gen < 6)
 		return;
 
-	gen6_suspend_rps(dev_priv);
+	gen6_disable_rps_interrupts(dev_priv);
 	dev_priv->rps.enabled = false;
 }
 
@@ -7579,8 +7568,6 @@ void intel_pm_setup(struct drm_device *dev)
 	mutex_init(&dev_priv->rps.hw_lock);
 	spin_lock_init(&dev_priv->rps.client_lock);
 
-	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
-			  intel_gen6_powersave_work);
 	INIT_LIST_HEAD(&dev_priv->rps.clients);
 	INIT_LIST_HEAD(&dev_priv->rps.semaphores.link);
 	INIT_LIST_HEAD(&dev_priv->rps.mmioflips.link);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c1ca458d688e..1c7ae9dd8cae 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -435,7 +435,7 @@ void intel_uncore_sanitize(struct drm_i915_private *dev_priv)
 	i915.enable_rc6 = sanitize_rc6_option(dev_priv, i915.enable_rc6);
 
 	/* BIOS often leaves RC6 enabled, but disable it for hw init */
-	intel_disable_gt_powersave(dev_priv);
+	intel_sanitize_gt_powersave(dev_priv);
 }
 
 static void __intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
-- 
2.8.1

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

✗ Ro.CI.BAT: failure for series starting with [1/6] drm/i915: Skip idling an idle engine (rev8)

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915: Skip idling an idle engine (rev8)
URL   : https://patchwork.freedesktop.org/series/7708/
State : failure

== Summary ==

Applying: drm/i915: Skip idling an idle engine
Applying: drm/i915: Defer enabling rc6 til after we submit the first batch/context
Applying: drm/i915: Treat kernel context as initialised
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_gem_context.c
M	drivers/gpu/drm/i915/intel_ringbuffer.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_ringbuffer.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_ringbuffer.c
Auto-merging drivers/gpu/drm/i915/i915_gem_context.c
Patch failed at 0003 drm/i915: Treat kernel context as initialised
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

Re: [PATCH v2 2/6] drm/i915: Move legacy kernel context pinning to intel_ringbuffer.c

[Joonas Lahtinen]

On ke, 2016-05-25 at 15:04 +0100, Chris Wilson wrote:
> This is so that we have symmetry with intel_lrc.c and avoid a source of
> if (i915.enable_execlists) layering violation within i915_gem_context.c -
> that is we move the specific handling of the dev_priv->kernel_context
> for legacy submission into the legacy submission code.
> 
> This depends upon the init/fini ordering between contexts and engines
> already defined by intel_lrc.c, and also exporting the context alignment
> required for pinning the legacy context.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  2 ++
>  drivers/gpu/drm/i915/i915_gem_context.c | 27 +++------------------------
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 31 +++++++++++++++++++++++++++++++
>  3 files changed, 36 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e4c8e341655c..19d0194c728f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -867,6 +867,8 @@ struct i915_gem_context {
>  	u32 user_handle;
>  #define CONTEXT_NO_ZEROMAP		(1<<0)
>  
> +	u32 ggtt_alignment;
> +
>  	struct intel_context {
>  		struct drm_i915_gem_object *state;
>  		struct intel_ringbuffer *ringbuf;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index a3b11aac23a4..c620fe6c9d96 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -268,6 +268,8 @@ __create_hw_context(struct drm_device *dev,
>  	list_add_tail(&ctx->link, &dev_priv->context_list);
>  	ctx->i915 = dev_priv;
>  
> +	ctx->ggtt_alignment = get_context_alignment(dev_priv);
> +
>  	if (dev_priv->hw_context_size) {
>  		struct drm_i915_gem_object *obj =
>  				i915_gem_alloc_context_obj(dev, dev_priv->hw_context_size);
> @@ -413,26 +415,6 @@ int i915_gem_context_init(struct drm_device *dev)
>  		return PTR_ERR(ctx);
>  	}
>  
> -	if (!i915.enable_execlists && ctx->engine[RCS].state) {
> -		int ret;
> -
> -		/* We may need to do things with the shrinker which
> -		 * require us to immediately switch back to the default
> -		 * context. This can cause a problem as pinning the
> -		 * default context also requires GTT space which may not
> -		 * be available. To avoid this we always pin the default
> -		 * context.
> -		 */
> -		ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state,
> -					    get_context_alignment(dev_priv), 0);
> -		if (ret) {
> -			DRM_ERROR("Failed to pinned default global context (error %d)\n",
> -				  ret);
> -			i915_gem_context_unreference(ctx);
> -			return ret;
> -		}
> -	}
> -
>  	dev_priv->kernel_context = ctx;
>  
>  	DRM_DEBUG_DRIVER("%s context support initialized\n",
> @@ -469,9 +451,6 @@ void i915_gem_context_fini(struct drm_device *dev)
>  
>  	lockdep_assert_held(&dev->struct_mutex);
>  
> -	if (!i915.enable_execlists && dctx->engine[RCS].state)
> -		i915_gem_object_ggtt_unpin(dctx->engine[RCS].state);
> -
>  	i915_gem_context_unreference(dctx);
>  	dev_priv->kernel_context = NULL;
>  
> @@ -721,7 +700,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
>  
>  	/* Trying to pin first makes error handling easier. */
>  	ret = i915_gem_obj_ggtt_pin(to->engine[RCS].state,
> -				    get_context_alignment(engine->i915),
> +				    to->ggtt_alignment,
>  				    0);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 8d35a3978f9b..4e0aa7e9d5da 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2244,6 +2244,8 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  				  struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct i915_gem_context *kctx = dev_priv->kernel_context;
> +	struct intel_context *ce = &kctx->engine[engine->id];

I'd prefer kctxe name, just to make it clear they're tightly related.

>  	struct intel_ringbuffer *ringbuf;
>  	int ret;
>  
> @@ -2260,6 +2262,25 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  
>  	init_waitqueue_head(&engine->irq_queue);
>  
> +	if (ce->state) {
> +		i915_gem_context_reference(kctx);

Without rename this looks rather odd.

> +
> +		/* We may need to do things with the shrinker which
> +		 * require us to immediately switch back to the default
> +		 * context. This can cause a problem as pinning the
> +		 * default context also requires GTT space which may not
> +		 * be available. To avoid this we always pin the default
> +		 * context.
> +		 */
> +		ret = i915_gem_obj_ggtt_pin(ce->state,
> +					    kctx->ggtt_alignment,
> +					    0);
> +		if (ret)
> +			goto error;
> +
> +		ce->initialised = false;
> +	}
> +
>  	ringbuf = intel_engine_create_ringbuffer(engine, 32 * PAGE_SIZE);
>  	if (IS_ERR(ringbuf)) {
>  		ret = PTR_ERR(ringbuf);
> @@ -2300,6 +2321,8 @@ error:
>  void intel_cleanup_engine(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *dev_priv;
> +	struct i915_gem_context *kctx;
> +	struct intel_context *ce;
>  
>  	if (!intel_engine_initialized(engine))
>  		return;
> @@ -2327,6 +2350,14 @@ void intel_cleanup_engine(struct intel_engine_cs *engine)
>  
>  	i915_cmd_parser_fini_ring(engine);
>  	i915_gem_batch_pool_fini(&engine->batch_pool);
> +
> +	kctx = dev_priv->kernel_context;
> +	ce = &kctx->engine[engine->id];

I'd move these two upper in the function.

With those two,

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

> +	if (ce->state) {
> +		i915_gem_object_ggtt_unpin(ce->state);
> +		i915_gem_context_unreference(kctx);
> +	}
> +
>  	engine->i915 = NULL;
>  }
>  
-- 
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 v2 2/6] drm/i915: Move legacy kernel context pinning to intel_ringbuffer.c

[Chris Wilson]

On Thu, May 26, 2016 at 01:22:04PM +0300, Joonas Lahtinen wrote:
> > @@ -2327,6 +2350,14 @@ void intel_cleanup_engine(struct intel_engine_cs *engine)
> >  
> >  	i915_cmd_parser_fini_ring(engine);
> >  	i915_gem_batch_pool_fini(&engine->batch_pool);
> > +
> > +	kctx = dev_priv->kernel_context;
> > +	ce = &kctx->engine[engine->id];
> 
> I'd move these two upper in the function.

We can't just yet, because it is silly and has a dev_priv == NULL check.
-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

[PATCH] drm/i915: Move legacy kernel context pinning to intel_ringbuffer.c

[Chris Wilson]

This is so that we have symmetry with intel_lrc.c and avoid a source of
if (i915.enable_execlists) layering violation within i915_gem_context.c -
that is we move the specific handling of the dev_priv->kernel_context
for legacy submission into the legacy submission code.

This depends upon the init/fini ordering between contexts and engines
already defined by intel_lrc.c, and also exporting the context alignment
required for pinning the legacy context.

v2: Separate out pin/unpin context funcs for greater symmetry with
intel_lrc. One more step towards unifying behaviour between the two
classes of engines and towards fixing another bug in i915_switch_context
vs requests.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e4c8e341655c..19d0194c728f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -867,6 +867,8 @@ struct i915_gem_context {
 	u32 user_handle;
 #define CONTEXT_NO_ZEROMAP		(1<<0)
 
+	u32 ggtt_alignment;
+
 	struct intel_context {
 		struct drm_i915_gem_object *state;
 		struct intel_ringbuffer *ringbuf;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index a3b11aac23a4..c620fe6c9d96 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -268,6 +268,8 @@ __create_hw_context(struct drm_device *dev,
 	list_add_tail(&ctx->link, &dev_priv->context_list);
 	ctx->i915 = dev_priv;
 
+	ctx->ggtt_alignment = get_context_alignment(dev_priv);
+
 	if (dev_priv->hw_context_size) {
 		struct drm_i915_gem_object *obj =
 				i915_gem_alloc_context_obj(dev, dev_priv->hw_context_size);
@@ -413,26 +415,6 @@ int i915_gem_context_init(struct drm_device *dev)
 		return PTR_ERR(ctx);
 	}
 
-	if (!i915.enable_execlists && ctx->engine[RCS].state) {
-		int ret;
-
-		/* We may need to do things with the shrinker which
-		 * require us to immediately switch back to the default
-		 * context. This can cause a problem as pinning the
-		 * default context also requires GTT space which may not
-		 * be available. To avoid this we always pin the default
-		 * context.
-		 */
-		ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state,
-					    get_context_alignment(dev_priv), 0);
-		if (ret) {
-			DRM_ERROR("Failed to pinned default global context (error %d)\n",
-				  ret);
-			i915_gem_context_unreference(ctx);
-			return ret;
-		}
-	}
-
 	dev_priv->kernel_context = ctx;
 
 	DRM_DEBUG_DRIVER("%s context support initialized\n",
@@ -469,9 +451,6 @@ void i915_gem_context_fini(struct drm_device *dev)
 
 	lockdep_assert_held(&dev->struct_mutex);
 
-	if (!i915.enable_execlists && dctx->engine[RCS].state)
-		i915_gem_object_ggtt_unpin(dctx->engine[RCS].state);
-
 	i915_gem_context_unreference(dctx);
 	dev_priv->kernel_context = NULL;
 
@@ -721,7 +700,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
 
 	/* Trying to pin first makes error handling easier. */
 	ret = i915_gem_obj_ggtt_pin(to->engine[RCS].state,
-				    get_context_alignment(engine->i915),
+				    to->ggtt_alignment,
 				    0);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 8d35a3978f9b..92bb376e5039 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2240,6 +2240,47 @@ intel_ringbuffer_free(struct intel_ringbuffer *ring)
 	kfree(ring);
 }
 
+static int intel_ring_context_pin(struct i915_gem_context *ctx,
+				  struct intel_engine_cs *engine)
+{
+	struct intel_context *ce = &ctx->engine[engine->id];
+	int ret;
+
+	lockdep_assert_held(&ctx->i915->dev->struct_mutex);
+
+	if (ce->pin_count++)
+		return 0;
+
+	if (ce->state) {
+		ret = i915_gem_obj_ggtt_pin(ce->state, ctx->ggtt_alignment, 0);
+		if (ret)
+			goto error;
+	}
+
+	i915_gem_context_reference(ctx);
+	return 0;
+
+error:
+	ce->pin_count = 0;
+	return ret;
+}
+
+static void intel_ring_context_unpin(struct i915_gem_context *ctx,
+				     struct intel_engine_cs *engine)
+{
+	struct intel_context *ce = &ctx->engine[engine->id];
+
+	lockdep_assert_held(&ctx->i915->dev->struct_mutex);
+
+	if (--ce->pin_count)
+		return;
+
+	if (ce->state)
+		i915_gem_object_ggtt_unpin(ce->state);
+
+	i915_gem_context_unreference(ctx);
+}
+
 static int intel_init_ring_buffer(struct drm_device *dev,
 				  struct intel_engine_cs *engine)
 {
@@ -2260,6 +2301,17 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 
 	init_waitqueue_head(&engine->irq_queue);
 
+	/* We may need to do things with the shrinker which
+	 * require us to immediately switch back to the default
+	 * context. This can cause a problem as pinning the
+	 * default context also requires GTT space which may not
+	 * be available. To avoid this we always pin the default
+	 * context.
+	 */
+	ret = intel_ring_context_pin(dev_priv->kernel_context, engine);
+	if (ret)
+		goto error;
+
 	ringbuf = intel_engine_create_ringbuffer(engine, 32 * PAGE_SIZE);
 	if (IS_ERR(ringbuf)) {
 		ret = PTR_ERR(ringbuf);
@@ -2327,6 +2379,9 @@ void intel_cleanup_engine(struct intel_engine_cs *engine)
 
 	i915_cmd_parser_fini_ring(engine);
 	i915_gem_batch_pool_fini(&engine->batch_pool);
+
+	intel_ring_context_unpin(dev_priv->kernel_context, engine);
+
 	engine->i915 = NULL;
 }
 
-- 
2.8.1

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

✗ Ro.CI.BAT: failure for series starting with [1/6] drm/i915: Skip idling an idle engine (rev9)

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915: Skip idling an idle engine (rev9)
URL   : https://patchwork.freedesktop.org/series/7708/
State : failure

== Summary ==

Applying: drm/i915: Skip idling an idle engine
Applying: drm/i915: Move legacy kernel context pinning to intel_ringbuffer.c
Applying: drm/i915: Treat kernel context as initialised
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_ringbuffer.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_ringbuffer.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_ringbuffer.c
Patch failed at 0003 drm/i915: Treat kernel context as initialised
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

Re: [PATCH] drm/i915: Move legacy kernel context pinning to intel_ringbuffer.c

[Joonas Lahtinen]

On to, 2016-05-26 at 12:18 +0100, Chris Wilson wrote:
> This is so that we have symmetry with intel_lrc.c and avoid a source of
> if (i915.enable_execlists) layering violation within i915_gem_context.c -
> that is we move the specific handling of the dev_priv->kernel_context
> for legacy submission into the legacy submission code.
> 
> This depends upon the init/fini ordering between contexts and engines
> already defined by intel_lrc.c, and also exporting the context alignment
> required for pinning the legacy context.
> 
> v2: Separate out pin/unpin context funcs for greater symmetry with
> intel_lrc. One more step towards unifying behaviour between the two
> classes of engines and towards fixing another bug in i915_switch_context
> vs requests.
> 

Much better.

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

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  2 ++
>  drivers/gpu/drm/i915/i915_gem_context.c | 27 ++--------------
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 55 +++++++++++++++++++++++++++++++++
>  3 files changed, 60 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e4c8e341655c..19d0194c728f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -867,6 +867,8 @@ struct i915_gem_context {
>  	u32 user_handle;
>  #define CONTEXT_NO_ZEROMAP		(1<<0)
>  
> +	u32 ggtt_alignment;
> +
>  	struct intel_context {
>  		struct drm_i915_gem_object *state;
>  		struct intel_ringbuffer *ringbuf;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index a3b11aac23a4..c620fe6c9d96 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -268,6 +268,8 @@ __create_hw_context(struct drm_device *dev,
>  	list_add_tail(&ctx->link, &dev_priv->context_list);
>  	ctx->i915 = dev_priv;
>  
> +	ctx->ggtt_alignment = get_context_alignment(dev_priv);
> +
>  	if (dev_priv->hw_context_size) {
>  		struct drm_i915_gem_object *obj =
>  				i915_gem_alloc_context_obj(dev, dev_priv->hw_context_size);
> @@ -413,26 +415,6 @@ int i915_gem_context_init(struct drm_device *dev)
>  		return PTR_ERR(ctx);
>  	}
>  
> -	if (!i915.enable_execlists && ctx->engine[RCS].state) {
> -		int ret;
> -
> -		/* We may need to do things with the shrinker which
> -		 * require us to immediately switch back to the default
> -		 * context. This can cause a problem as pinning the
> -		 * default context also requires GTT space which may not
> -		 * be available. To avoid this we always pin the default
> -		 * context.
> -		 */
> -		ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state,
> -					    get_context_alignment(dev_priv), 0);
> -		if (ret) {
> -			DRM_ERROR("Failed to pinned default global context (error %d)\n",
> -				  ret);
> -			i915_gem_context_unreference(ctx);
> -			return ret;
> -		}
> -	}
> -
>  	dev_priv->kernel_context = ctx;
>  
>  	DRM_DEBUG_DRIVER("%s context support initialized\n",
> @@ -469,9 +451,6 @@ void i915_gem_context_fini(struct drm_device *dev)
>  
>  	lockdep_assert_held(&dev->struct_mutex);
>  
> -	if (!i915.enable_execlists && dctx->engine[RCS].state)
> -		i915_gem_object_ggtt_unpin(dctx->engine[RCS].state);
> -
>  	i915_gem_context_unreference(dctx);
>  	dev_priv->kernel_context = NULL;
>  
> @@ -721,7 +700,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
>  
>  	/* Trying to pin first makes error handling easier. */
>  	ret = i915_gem_obj_ggtt_pin(to->engine[RCS].state,
> -				    get_context_alignment(engine->i915),
> +				    to->ggtt_alignment,
>  				    0);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 8d35a3978f9b..92bb376e5039 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2240,6 +2240,47 @@ intel_ringbuffer_free(struct intel_ringbuffer *ring)
>  	kfree(ring);
>  }
>  
> +static int intel_ring_context_pin(struct i915_gem_context *ctx,
> +				  struct intel_engine_cs *engine)
> +{
> +	struct intel_context *ce = &ctx->engine[engine->id];
> +	int ret;
> +
> +	lockdep_assert_held(&ctx->i915->dev->struct_mutex);
> +
> +	if (ce->pin_count++)
> +		return 0;
> +
> +	if (ce->state) {
> +		ret = i915_gem_obj_ggtt_pin(ce->state, ctx->ggtt_alignment, 0);
> +		if (ret)
> +			goto error;
> +	}
> +
> +	i915_gem_context_reference(ctx);
> +	return 0;
> +
> +error:
> +	ce->pin_count = 0;
> +	return ret;
> +}
> +
> +static void intel_ring_context_unpin(struct i915_gem_context *ctx,
> +				     struct intel_engine_cs *engine)
> +{
> +	struct intel_context *ce = &ctx->engine[engine->id];
> +
> +	lockdep_assert_held(&ctx->i915->dev->struct_mutex);
> +
> +	if (--ce->pin_count)
> +		return;
> +
> +	if (ce->state)
> +		i915_gem_object_ggtt_unpin(ce->state);
> +
> +	i915_gem_context_unreference(ctx);
> +}
> +
>  static int intel_init_ring_buffer(struct drm_device *dev,
>  				  struct intel_engine_cs *engine)
>  {
> @@ -2260,6 +2301,17 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  
>  	init_waitqueue_head(&engine->irq_queue);
>  
> +	/* We may need to do things with the shrinker which
> +	 * require us to immediately switch back to the default
> +	 * context. This can cause a problem as pinning the
> +	 * default context also requires GTT space which may not
> +	 * be available. To avoid this we always pin the default
> +	 * context.
> +	 */
> +	ret = intel_ring_context_pin(dev_priv->kernel_context, engine);
> +	if (ret)
> +		goto error;
> +
>  	ringbuf = intel_engine_create_ringbuffer(engine, 32 * PAGE_SIZE);
>  	if (IS_ERR(ringbuf)) {
>  		ret = PTR_ERR(ringbuf);
> @@ -2327,6 +2379,9 @@ void intel_cleanup_engine(struct intel_engine_cs *engine)
>  
>  	i915_cmd_parser_fini_ring(engine);
>  	i915_gem_batch_pool_fini(&engine->batch_pool);
> +
> +	intel_ring_context_unpin(dev_priv->kernel_context, engine);
> +
>  	engine->i915 = NULL;
>  }
>  
-- 
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] drm/i915: Move legacy kernel context pinning to intel_ringbuffer.c

[Mika Kuoppala]

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

> [ text/plain ]
> This is so that we have symmetry with intel_lrc.c and avoid a source of
> if (i915.enable_execlists) layering violation within i915_gem_context.c -
> that is we move the specific handling of the dev_priv->kernel_context
> for legacy submission into the legacy submission code.
>
> This depends upon the init/fini ordering between contexts and engines
> already defined by intel_lrc.c, and also exporting the context alignment
> required for pinning the legacy context.
>
> v2: Separate out pin/unpin context funcs for greater symmetry with
> intel_lrc. One more step towards unifying behaviour between the two
> classes of engines and towards fixing another bug in i915_switch_context
> vs requests.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

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


> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  2 ++
>  drivers/gpu/drm/i915/i915_gem_context.c | 27 ++--------------
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 55 +++++++++++++++++++++++++++++++++
>  3 files changed, 60 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e4c8e341655c..19d0194c728f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -867,6 +867,8 @@ struct i915_gem_context {
>  	u32 user_handle;
>  #define CONTEXT_NO_ZEROMAP		(1<<0)
>  
> +	u32 ggtt_alignment;
> +
>  	struct intel_context {
>  		struct drm_i915_gem_object *state;
>  		struct intel_ringbuffer *ringbuf;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index a3b11aac23a4..c620fe6c9d96 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -268,6 +268,8 @@ __create_hw_context(struct drm_device *dev,
>  	list_add_tail(&ctx->link, &dev_priv->context_list);
>  	ctx->i915 = dev_priv;
>  
> +	ctx->ggtt_alignment = get_context_alignment(dev_priv);
> +
>  	if (dev_priv->hw_context_size) {
>  		struct drm_i915_gem_object *obj =
>  				i915_gem_alloc_context_obj(dev, dev_priv->hw_context_size);
> @@ -413,26 +415,6 @@ int i915_gem_context_init(struct drm_device *dev)
>  		return PTR_ERR(ctx);
>  	}
>  
> -	if (!i915.enable_execlists && ctx->engine[RCS].state) {
> -		int ret;
> -
> -		/* We may need to do things with the shrinker which
> -		 * require us to immediately switch back to the default
> -		 * context. This can cause a problem as pinning the
> -		 * default context also requires GTT space which may not
> -		 * be available. To avoid this we always pin the default
> -		 * context.
> -		 */
> -		ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state,
> -					    get_context_alignment(dev_priv), 0);
> -		if (ret) {
> -			DRM_ERROR("Failed to pinned default global context (error %d)\n",
> -				  ret);
> -			i915_gem_context_unreference(ctx);
> -			return ret;
> -		}
> -	}
> -
>  	dev_priv->kernel_context = ctx;
>  
>  	DRM_DEBUG_DRIVER("%s context support initialized\n",
> @@ -469,9 +451,6 @@ void i915_gem_context_fini(struct drm_device *dev)
>  
>  	lockdep_assert_held(&dev->struct_mutex);
>  
> -	if (!i915.enable_execlists && dctx->engine[RCS].state)
> -		i915_gem_object_ggtt_unpin(dctx->engine[RCS].state);
> -
>  	i915_gem_context_unreference(dctx);
>  	dev_priv->kernel_context = NULL;
>  
> @@ -721,7 +700,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
>  
>  	/* Trying to pin first makes error handling easier. */
>  	ret = i915_gem_obj_ggtt_pin(to->engine[RCS].state,
> -				    get_context_alignment(engine->i915),
> +				    to->ggtt_alignment,
>  				    0);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 8d35a3978f9b..92bb376e5039 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2240,6 +2240,47 @@ intel_ringbuffer_free(struct intel_ringbuffer *ring)
>  	kfree(ring);
>  }
>  
> +static int intel_ring_context_pin(struct i915_gem_context *ctx,
> +				  struct intel_engine_cs *engine)
> +{
> +	struct intel_context *ce = &ctx->engine[engine->id];
> +	int ret;
> +
> +	lockdep_assert_held(&ctx->i915->dev->struct_mutex);
> +
> +	if (ce->pin_count++)
> +		return 0;
> +
> +	if (ce->state) {
> +		ret = i915_gem_obj_ggtt_pin(ce->state, ctx->ggtt_alignment, 0);
> +		if (ret)
> +			goto error;
> +	}
> +
> +	i915_gem_context_reference(ctx);
> +	return 0;
> +
> +error:
> +	ce->pin_count = 0;
> +	return ret;
> +}
> +
> +static void intel_ring_context_unpin(struct i915_gem_context *ctx,
> +				     struct intel_engine_cs *engine)
> +{
> +	struct intel_context *ce = &ctx->engine[engine->id];
> +
> +	lockdep_assert_held(&ctx->i915->dev->struct_mutex);
> +
> +	if (--ce->pin_count)
> +		return;
> +
> +	if (ce->state)
> +		i915_gem_object_ggtt_unpin(ce->state);
> +
> +	i915_gem_context_unreference(ctx);
> +}
> +
>  static int intel_init_ring_buffer(struct drm_device *dev,
>  				  struct intel_engine_cs *engine)
>  {
> @@ -2260,6 +2301,17 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  
>  	init_waitqueue_head(&engine->irq_queue);
>  
> +	/* We may need to do things with the shrinker which
> +	 * require us to immediately switch back to the default
> +	 * context. This can cause a problem as pinning the
> +	 * default context also requires GTT space which may not
> +	 * be available. To avoid this we always pin the default
> +	 * context.
> +	 */
> +	ret = intel_ring_context_pin(dev_priv->kernel_context, engine);
> +	if (ret)
> +		goto error;
> +
>  	ringbuf = intel_engine_create_ringbuffer(engine, 32 * PAGE_SIZE);
>  	if (IS_ERR(ringbuf)) {
>  		ret = PTR_ERR(ringbuf);
> @@ -2327,6 +2379,9 @@ void intel_cleanup_engine(struct intel_engine_cs *engine)
>  
>  	i915_cmd_parser_fini_ring(engine);
>  	i915_gem_batch_pool_fini(&engine->batch_pool);
> +
> +	intel_ring_context_unpin(dev_priv->kernel_context, engine);
> +
>  	engine->i915 = NULL;
>  }
>  
> -- 
> 2.8.1
>
> _______________________________________________
> 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