Bug 95634, take 2

Bug 95634, take 2

[Chris Wilson]

As Mika finally expressed his disquiet about not forcing the context
enabling during init, we grow a few patches to ensure that we have a
valid context image loaded into HW prior to enabling rc6. (And we keep
the current behaviour of doing some busy work in case userspace doesn't
so that we can enable powersaving when idle.)
-Chris

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

[PATCH v3 01/10] 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 v3 02/10] 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 v3 03/10] 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 v3 04/10] 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>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 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 v3 05/10] 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 v3 06/10] 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

[PATCH v3 07/10] 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

[PATCH v3 08/10] drm/i915: Preserve current RPS frequency

[Chris Wilson]

Select idle frequency during initialisation, then reset the last known
frequency when re-enabling. This allows us to preserve the user selected
frequency across resets.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b6dfd0264950..b85b7840a2ca 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)
@@ -4970,6 +4971,15 @@ static void gen6_init_rps_frequencies(struct drm_i915_private *dev_priv)
 	}
 }
 
+static void reset_rps(struct drm_i915_private *dev_priv,
+		      void (*set)(struct drm_i915_private *, u8))
+{
+	u8 freq = dev_priv->rps.cur_freq; /* force a reset */
+	dev_priv->rps.power = -1;
+	dev_priv->rps.cur_freq = -1;
+	set(dev_priv, freq);
+}
+
 /* See the Gen9_GT_PM_Programming_Guide doc for the below */
 static void gen9_enable_rps(struct drm_i915_private *dev_priv)
 {
@@ -5006,8 +5016,7 @@ 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);
+	reset_rps(dev_priv, gen6_set_rps);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
@@ -5153,8 +5162,7 @@ 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);
+	reset_rps(dev_priv, gen6_set_rps);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
@@ -5247,8 +5255,7 @@ 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);
+	reset_rps(dev_priv, gen6_set_rps);
 
 	rc6vids = 0;
 	ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
@@ -5612,6 +5619,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 +5684,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)
@@ -5784,7 +5793,7 @@ 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);
+	reset_rps(dev_priv, valleyview_set_rps);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
@@ -5864,16 +5873,7 @@ 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);
+	reset_rps(dev_priv, valleyview_set_rps);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
-- 
2.8.1

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

[PATCH v3 09/10] drm/i915: Remove superfluous powersave work flushing

[Chris Wilson]

Instead of flushing the outstanding enabling, remember the requested
frequency to apply when the powersave work runs.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 30 ++------------------------
 drivers/gpu/drm/i915/i915_sysfs.c   | 42 +++++++------------------------------
 2 files changed, 10 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 615cef736356..a49c7a0019b7 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;
@@ -4970,20 +4966,11 @@ i915_max_freq_get(void *data, u64 *val)
 {
 	struct drm_device *dev = data;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int ret;
 
 	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;
-
 	*val = intel_gpu_freq(dev_priv, dev_priv->rps.max_freq_softlimit);
-	mutex_unlock(&dev_priv->rps.hw_lock);
-
 	return 0;
 }
 
@@ -4998,8 +4985,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);
@@ -5037,20 +5022,11 @@ i915_min_freq_get(void *data, u64 *val)
 {
 	struct drm_device *dev = data;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int ret;
 
-	if (INTEL_INFO(dev)->gen < 6)
+	if (INTEL_GEN(dev_priv) < 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;
-
 	*val = intel_gpu_freq(dev_priv, dev_priv->rps.min_freq_softlimit);
-	mutex_unlock(&dev_priv->rps.hw_lock);
-
 	return 0;
 }
 
@@ -5062,11 +5038,9 @@ i915_min_freq_set(void *data, u64 val)
 	u32 hw_max, hw_min;
 	int ret;
 
-	if (INTEL_INFO(dev)->gen < 6)
+	if (INTEL_GEN(dev_priv) < 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_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index a6e90fe05a1e..915e97cdc4d5 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);
@@ -303,19 +301,9 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
 	struct drm_minor *minor = dev_to_drm_minor(kdev);
 	struct drm_device *dev = minor->dev;
 	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);
-	ret = intel_gpu_freq(dev_priv, dev_priv->rps.cur_freq);
-	mutex_unlock(&dev_priv->rps.hw_lock);
-
-	intel_runtime_pm_put(dev_priv);
-
-	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+			intel_gpu_freq(dev_priv, dev_priv->rps.cur_freq));
 }
 
 static ssize_t vlv_rpe_freq_mhz_show(struct device *kdev,
@@ -335,15 +323,10 @@ static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute
 	struct drm_minor *minor = dev_to_drm_minor(kdev);
 	struct drm_device *dev = minor->dev;
 	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);
-
-	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+			intel_gpu_freq(dev_priv,
+				       dev_priv->rps.max_freq_softlimit));
 }
 
 static ssize_t gt_max_freq_mhz_store(struct device *kdev,
@@ -360,8 +343,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);
@@ -403,15 +384,10 @@ static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute
 	struct drm_minor *minor = dev_to_drm_minor(kdev);
 	struct drm_device *dev = minor->dev;
 	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);
-
-	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+			intel_gpu_freq(dev_priv,
+				       dev_priv->rps.min_freq_softlimit));
 }
 
 static ssize_t gt_min_freq_mhz_store(struct device *kdev,
@@ -428,8 +404,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);
-- 
2.8.1

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

[PATCH v3 10/10] 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. As we do not forcibly load the kernel context at resume,
we have to hook into the batch submission to be sure that the render
state is setup before enabling rc6.

However, since we don't enable powersaving until that first batch, we
queued a delayed task in order to guarantee that the batch is indeed
submitted.

v2: Rearrange intel_disable_gt_powersave() to match.
v3: Apply user specified cur_freq (or idle_freq if not set).
v4: Give in, and supply a delayed work to autoenable 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_drv.c      |   7 +-
 drivers/gpu/drm/i915/intel_display.c |   3 +-
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_pm.c      | 151 +++++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_uncore.c  |   2 +-
 5 files changed, 98 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 943d7b222fd4..f441e9d8e48e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -804,6 +804,7 @@ static int i915_drm_resume(struct drm_device *dev)
 
 	intel_opregion_notify_adapter(dev_priv, PCI_D0);
 
+	intel_autoenable_gt_powersave(dev_priv);
 	drm_kms_helper_poll_enable(dev);
 
 	enable_rpm_wakeref_asserts(dev_priv);
@@ -1000,8 +1001,7 @@ int i915_reset(struct drm_i915_private *dev_priv)
 	 * previous concerns that it doesn't respond well to some forms
 	 * of re-init after reset.
 	 */
-	if (INTEL_INFO(dev)->gen > 5)
-		intel_enable_gt_powersave(dev_priv);
+	intel_autoenable_gt_powersave(dev_priv);
 
 	return 0;
 
@@ -1663,7 +1663,6 @@ static int intel_runtime_resume(struct device *device)
 	 * we can do is to hope that things will still work (and disable RPM).
 	 */
 	i915_gem_init_swizzling(dev);
-	gen6_update_ring_freq(dev_priv);
 
 	intel_runtime_pm_enable_interrupts(dev_priv);
 
@@ -1675,7 +1674,7 @@ 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);
+	intel_autoenable_gt_powersave(dev_priv);
 
 	enable_rpm_wakeref_asserts(dev_priv);
 
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..93a80e13f64d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1641,7 +1641,9 @@ 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_autoenable_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);
 void intel_reset_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 b85b7840a2ca..435815f35f22 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6387,13 +6387,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 +6400,65 @@ void intel_suspend_gt_powersave(struct drm_i915_private *dev_priv)
 	if (INTEL_GEN(dev_priv) < 6)
 		return;
 
-	gen6_suspend_rps(dev_priv);
+	cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
+
+	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 +6466,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 +6480,61 @@ 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);
 
+	if (INTEL_GEN(dev_priv) >= 6)
+		gen6_enable_rps_interrupts(dev_priv);
+
 	dev_priv->rps.enabled = true;
+	mutex_unlock(&dev_priv->rps.hw_lock);
+}
 
-	gen6_enable_rps_interrupts(dev_priv);
+static void __intel_autoenable_gt_powersave(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, typeof(*dev_priv), rps.delayed_resume_work.work);
+	struct intel_engine_cs *rcs;
+	struct drm_i915_gem_request *req;
+	int ret;
 
-	mutex_unlock(&dev_priv->rps.hw_lock);
+	if (READ_ONCE(dev_priv->rps.enabled))
+		return;
+
+	rcs = &dev_priv->engine[RCS];
+	if (rcs->last_context)
+		return;
+
+	if (!rcs->init_context)
+		return;
+
+	intel_runtime_pm_get(dev_priv);
+	mutex_lock(&dev_priv->dev->struct_mutex);
+
+	req = i915_gem_request_alloc(rcs, dev_priv->kernel_context);
+	if (IS_ERR(req))
+		goto unlock;
+
+	ret = 0;
+	if (!i915.enable_execlists)
+		ret = i915_switch_context(req);
+	if (ret == 0)
+		rcs->init_context(req);
+	i915_add_request_no_flush(req);
 
+unlock:
+	mutex_unlock(&dev_priv->dev->struct_mutex);
 	intel_runtime_pm_put(dev_priv);
 }
 
-void intel_enable_gt_powersave(struct drm_i915_private *dev_priv)
+void intel_autoenable_gt_powersave(struct drm_i915_private *dev_priv)
 {
-	/* Powersaving is controlled by the host when inside a VM */
-	if (intel_vgpu_active(dev_priv))
+	if (READ_ONCE(dev_priv->rps.enabled))
 		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);
-	}
+	if (INTEL_GEN(dev_priv) < 6)
+		return;
+
+	queue_delayed_work(dev_priv->wq,
+			   &dev_priv->rps.delayed_resume_work,
+			   round_jiffies_up_relative(2*HZ));
 }
 
 void intel_reset_gt_powersave(struct drm_i915_private *dev_priv)
@@ -6513,7 +6542,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;
 }
 
@@ -7580,7 +7609,7 @@ void intel_pm_setup(struct drm_device *dev)
 	spin_lock_init(&dev_priv->rps.client_lock);
 
 	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
-			  intel_gen6_powersave_work);
+			  __intel_autoenable_gt_powersave);
 	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 [v3,01/10] drm/i915: Skip idling an idle engine

[Patchwork]

== Series Details ==

Series: series starting with [v3,01/10] drm/i915: Skip idling an idle engine
URL   : https://patchwork.freedesktop.org/series/7792/
State : failure

== Summary ==

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

Test gem_busy:
        Subgroup basic-blt:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-cmd:
                pass       -> FAIL       (ro-byt-n2820)
Test gem_mmap_gtt:
        Subgroup basic-read:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (ro-bdw-i7-5600u)
                skip       -> PASS       (fi-skl-i5-6260u)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)

fi-bdw-i7-5557u  total:209  pass:197  dwarn:0   dfail:0   fail:0   skip:12 
fi-bsw-n3050     total:209  pass:167  dwarn:0   dfail:0   fail:2   skip:40 
fi-byt-n2820     total:209  pass:168  dwarn:0   dfail:0   fail:3   skip:38 
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:168  dwarn:0   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-skl-i7-6700hq total:204  pass:181  dwarn:2   dfail:0   fail:0   skip:21 
ro-snb-i7-2620M  total:209  pass:170  dwarn:0   dfail:0   fail:1   skip:38 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1018/

fc9d741 drm-intel-nightly: 2016y-05m-25d-07h-45m-48s UTC integration manifest
74f34c4 drm/i915: Defer enabling rc6 til after we submit the first batch/context
1b32d08 drm/i915: Remove superfluous powersave work flushing
1ceeaf8f drm/i915: Preserve current RPS frequency
fd62102 drm/i915: Only switch to default context when evicting from GGTT
ad05e18 drm/i915: Split idling from forcing context switch
abdffae drm/i915: No need to wait for idle on L3 remap
33ef68f drm/i915: Mark all default contexts as uninitialised after context loss
97c1019 drm/i915: Treat kernel context as initialised
95c103e drm/i915: Move legacy kernel context pinning to intel_ringbuffer.c
a3add04 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 v3 07/10] drm/i915: Only switch to default context when evicting from GGTT

[Joonas Lahtinen]

On to, 2016-05-26 at 09:52 +0100, Chris Wilson wrote:
> 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.
> 

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_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)
-- 
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 v3 06/10] drm/i915: Split idling from forcing context switch

[Joonas Lahtinen]

On to, 2016-05-26 at 09:52 +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.
> 
> v2: Tweak an error message if the wait fails for the ilk vtd w/a
> 

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

> 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;
>  
-- 
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 v3 09/10] drm/i915: Remove superfluous powersave work flushing

[Ville Syrjälä]

On Thu, May 26, 2016 at 09:52:39AM +0100, Chris Wilson wrote:
> Instead of flushing the outstanding enabling, remember the requested
> frequency to apply when the powersave work runs.

I didn't see a patch to move the frequency init to happen before
debugfs init. So methinks we still need the flush.

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 30 ++------------------------
>  drivers/gpu/drm/i915/i915_sysfs.c   | 42 +++++++------------------------------
>  2 files changed, 10 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 615cef736356..a49c7a0019b7 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;
> @@ -4970,20 +4966,11 @@ i915_max_freq_get(void *data, u64 *val)
>  {
>  	struct drm_device *dev = data;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int ret;
>  
>  	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;
> -
>  	*val = intel_gpu_freq(dev_priv, dev_priv->rps.max_freq_softlimit);
> -	mutex_unlock(&dev_priv->rps.hw_lock);
> -
>  	return 0;
>  }
>  
> @@ -4998,8 +4985,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);
> @@ -5037,20 +5022,11 @@ i915_min_freq_get(void *data, u64 *val)
>  {
>  	struct drm_device *dev = data;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int ret;
>  
> -	if (INTEL_INFO(dev)->gen < 6)
> +	if (INTEL_GEN(dev_priv) < 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;
> -
>  	*val = intel_gpu_freq(dev_priv, dev_priv->rps.min_freq_softlimit);
> -	mutex_unlock(&dev_priv->rps.hw_lock);
> -
>  	return 0;
>  }
>  
> @@ -5062,11 +5038,9 @@ i915_min_freq_set(void *data, u64 val)
>  	u32 hw_max, hw_min;
>  	int ret;
>  
> -	if (INTEL_INFO(dev)->gen < 6)
> +	if (INTEL_GEN(dev_priv) < 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_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index a6e90fe05a1e..915e97cdc4d5 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);
> @@ -303,19 +301,9 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
>  	struct drm_minor *minor = dev_to_drm_minor(kdev);
>  	struct drm_device *dev = minor->dev;
>  	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);
> -	ret = intel_gpu_freq(dev_priv, dev_priv->rps.cur_freq);
> -	mutex_unlock(&dev_priv->rps.hw_lock);
> -
> -	intel_runtime_pm_put(dev_priv);
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> +	return snprintf(buf, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(dev_priv, dev_priv->rps.cur_freq));
>  }
>  
>  static ssize_t vlv_rpe_freq_mhz_show(struct device *kdev,
> @@ -335,15 +323,10 @@ static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute
>  	struct drm_minor *minor = dev_to_drm_minor(kdev);
>  	struct drm_device *dev = minor->dev;
>  	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);
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> +	return snprintf(buf, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(dev_priv,
> +				       dev_priv->rps.max_freq_softlimit));
>  }
>  
>  static ssize_t gt_max_freq_mhz_store(struct device *kdev,
> @@ -360,8 +343,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);
> @@ -403,15 +384,10 @@ static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute
>  	struct drm_minor *minor = dev_to_drm_minor(kdev);
>  	struct drm_device *dev = minor->dev;
>  	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);
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> +	return snprintf(buf, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(dev_priv,
> +				       dev_priv->rps.min_freq_softlimit));
>  }
>  
>  static ssize_t gt_min_freq_mhz_store(struct device *kdev,
> @@ -428,8 +404,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);
> -- 
> 2.8.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 03/10] drm/i915: Treat kernel context as initialised

[Chris Wilson]

On Thu, May 26, 2016 at 09:52:33AM +0100, Chris Wilson wrote:
> 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;

This is really too much of a hack to live. So long as we avoid the
switch during suspend, we can let this patch drop.
-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 v3 09/10] drm/i915: Remove superfluous powersave work flushing

[Chris Wilson]

On Thu, May 26, 2016 at 02:45:09PM +0300, Ville Syrjälä wrote:
> On Thu, May 26, 2016 at 09:52:39AM +0100, Chris Wilson wrote:
> > Instead of flushing the outstanding enabling, remember the requested
> > frequency to apply when the powersave work runs.
> 
> I didn't see a patch to move the frequency init to happen before
> debugfs init. So methinks we still need the flush.

Considering debugfs_init is exposing us to userspace, it should be part
of the registration phase (i.e. last thing) after we have all the hw
probed / minimally prepared.
-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 v3 02/10] 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.
>
> 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;
> +

If you fail here, you will unpin regardless. Is that a problem?

> +		ce->initialised = false;

Just an observation that we seem to use both initialized and
initialised.

-Mika


> +	}
> +
>  	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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 05/10] 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 v3 05/10] drm/i915: No need to wait for idle on L3 remap

[Joonas Lahtinen]

On to, 2016-05-26 at 09:52 +0100, Chris Wilson wrote:
> 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: Joonas Lahtinen <joonas.lahtinen@linux.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.
-- 
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 v3 02/10] drm/i915: Move legacy kernel context pinning to intel_ringbuffer.c

[Chris Wilson]

On Thu, May 26, 2016 at 03:04:11PM +0300, Mika Kuoppala wrote:
> > +	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;
> > +
> 
> If you fail here, you will unpin regardless. Is that a problem?

To be here, ce->pin_count == 1 (because it is locked and we only init if
ce->pin_count == 0 on entry). So on error we restore it back to 0.
-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 v3 03/10] drm/i915: Treat kernel context as initialised

[Chris Wilson]

On Thu, May 26, 2016 at 12:51:45PM +0100, Chris Wilson wrote:
> On Thu, May 26, 2016 at 09:52:33AM +0100, Chris Wilson wrote:
> > 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;
> 
> This is really too much of a hack to live. So long as we avoid the
> switch during suspend, we can let this patch drop.

Hmm, but marking it as initialised stops us from having to allocate
the renderstate during eviction. :|

So far, this seems to be the least clumsy approach.
-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: Register the debugfs interfaces after loading the driver

[Chris Wilson]

In order to give the driver the chance to initialise the data structures
that will be exposed through debugfs, perform driver->load() before
registering the debugfs entries. (Otherwise it may be possible for
userspace to cause an oops through the debugfs interfaces.) As the
driver load is now before debugfs registration, make the registration
non-fatal (as it simply prevents us exposing an optional debug facility
and not hard ABI).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_debugfs.c | 13 ++++++---
 drivers/gpu/drm/drm_drv.c     | 62 +++++++++++++++++++++++++++++++------------
 2 files changed, 54 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 3bcf8e6a85b3..8f36e014fbd2 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -160,10 +160,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 	ret = drm_debugfs_create_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES,
 				       minor->debugfs_root, minor);
 	if (ret) {
-		debugfs_remove(minor->debugfs_root);
-		minor->debugfs_root = NULL;
 		DRM_ERROR("Failed to create core drm debugfs files\n");
-		return ret;
+		goto err_root;
 	}
 
 	if (dev->driver->debugfs_init) {
@@ -171,10 +169,17 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 		if (ret) {
 			DRM_ERROR("DRM: Driver failed to initialize "
 				  "/sys/kernel/debug/dri.\n");
-			return ret;
+			goto err_core;
 		}
 	}
 	return 0;
+
+err_core:
+	drm_debugfs_remove_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES, minor);
+err_root:
+	debugfs_remove(minor->debugfs_root);
+	minor->debugfs_root = NULL;
+	return ret;
 }
 
 
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index bff89226a344..82d1e80c2bf4 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -307,15 +307,9 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
 	if (!minor)
 		return 0;
 
-	ret = drm_debugfs_init(minor, minor->index, drm_debugfs_root);
-	if (ret) {
-		DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
-		return ret;
-	}
-
 	ret = device_add(minor->kdev);
 	if (ret)
-		goto err_debugfs;
+		return ret;
 
 	/* replace NULL with @minor so lookups will succeed from now on */
 	spin_lock_irqsave(&drm_minor_lock, flags);
@@ -324,10 +318,36 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
 
 	DRM_DEBUG("new minor registered %d\n", minor->index);
 	return 0;
+}
+
+static void drm_debugfs_register(struct drm_device *dev, unsigned int type)
+{
+	struct drm_minor *minor;
+
+	DRM_DEBUG("\n");
+	if (!drm_debugfs_root)
+		return;
+
+	minor = *drm_minor_get_slot(dev, type);
+	if (!minor)
+		return;
+
+	if (drm_debugfs_init(minor, minor->index, drm_debugfs_root))
+		DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
+}
+
+static void drm_debugfs_unregister(struct drm_device *dev, unsigned int type)
+{
+	struct drm_minor *minor;
+
+	if (!drm_debugfs_root)
+		return;
+
+	minor = *drm_minor_get_slot(dev, type);
+	if (!minor || !device_is_registered(minor->kdev))
+		return;
 
-err_debugfs:
 	drm_debugfs_cleanup(minor);
-	return ret;
 }
 
 static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
@@ -346,7 +366,6 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
 
 	device_del(minor->kdev);
 	dev_set_drvdata(minor->kdev, NULL); /* safety belt */
-	drm_debugfs_cleanup(minor);
 }
 
 /**
@@ -460,6 +479,10 @@ EXPORT_SYMBOL(drm_put_dev);
 
 void drm_unplug_dev(struct drm_device *dev)
 {
+	drm_debugfs_unregister(dev, DRM_MINOR_LEGACY);
+	drm_debugfs_unregister(dev, DRM_MINOR_RENDER);
+	drm_debugfs_unregister(dev, DRM_MINOR_CONTROL);
+
 	/* for a USB device */
 	drm_minor_unregister(dev, DRM_MINOR_LEGACY);
 	drm_minor_unregister(dev, DRM_MINOR_RENDER);
@@ -759,6 +782,10 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 			goto err_minors;
 	}
 
+	drm_debugfs_register(dev, DRM_MINOR_CONTROL);
+	drm_debugfs_register(dev, DRM_MINOR_RENDER);
+	drm_debugfs_register(dev, DRM_MINOR_LEGACY);
+
 	ret = 0;
 	goto out_unlock;
 
@@ -800,6 +827,10 @@ void drm_dev_unregister(struct drm_device *dev)
 	list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head)
 		drm_legacy_rmmap(dev, r_list->map);
 
+	drm_debugfs_unregister(dev, DRM_MINOR_LEGACY);
+	drm_debugfs_unregister(dev, DRM_MINOR_RENDER);
+	drm_debugfs_unregister(dev, DRM_MINOR_CONTROL);
+
 	drm_minor_unregister(dev, DRM_MINOR_LEGACY);
 	drm_minor_unregister(dev, DRM_MINOR_RENDER);
 	drm_minor_unregister(dev, DRM_MINOR_CONTROL);
@@ -904,17 +935,13 @@ static int __init drm_core_init(void)
 	}
 
 	drm_debugfs_root = debugfs_create_dir("dri", NULL);
-	if (!drm_debugfs_root) {
+	if (!drm_debugfs_root)
 		DRM_ERROR("Cannot create /sys/kernel/debug/dri\n");
-		ret = -1;
-		goto err_p3;
-	}
 
 	DRM_INFO("Initialized %s %d.%d.%d %s\n",
 		 CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE);
 	return 0;
-err_p3:
-	drm_sysfs_destroy();
+
 err_p2:
 	unregister_chrdev(DRM_MAJOR, "drm");
 
@@ -925,7 +952,8 @@ err_p1:
 
 static void __exit drm_core_exit(void)
 {
-	debugfs_remove(drm_debugfs_root);
+	if (drm_debugfs_root)
+		debugfs_remove(drm_debugfs_root);
 	drm_sysfs_destroy();
 
 	unregister_chrdev(DRM_MAJOR, "drm");
-- 
2.8.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Register the debugfs interfaces after loading the driver

[Chris Wilson]

On Thu, May 26, 2016 at 01:35:18PM +0100, Chris Wilson wrote:
> In order to give the driver the chance to initialise the data structures
> that will be exposed through debugfs, perform driver->load() before
> registering the debugfs entries. (Otherwise it may be possible for
> userspace to cause an oops through the debugfs interfaces.) As the
> driver load is now before debugfs registration, make the registration
> non-fatal (as it simply prevents us exposing an optional debug facility
> and not hard ABI).

The alternative here would be for i915.ko to stop registering a
driver->debugfs_init and do it as part of its registration phase (like
sysfs).
-Chris

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

✗ Ro.CI.BAT: failure for series starting with [v3,01/10] drm/i915: Skip idling an idle engine (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v3,01/10] drm/i915: Skip idling an idle engine (rev2)
URL   : https://patchwork.freedesktop.org/series/7792/
State : failure

== Summary ==

Series 7792v2 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/7792/revisions/2/mbox

Test core_auth:
        Subgroup basic-auth:
                pass       -> INCOMPLETE (ro-bdw-i7-5557U)
                pass       -> INCOMPLETE (ro-ilk1-i5-650)
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
                pass       -> INCOMPLETE (ro-bdw-i7-5600u)
                pass       -> INCOMPLETE (ro-bdw-i5-5250u)
                pass       -> INCOMPLETE (ro-snb-i7-2620M)
                pass       -> INCOMPLETE (ro-skl-i7-6700hq)
                pass       -> INCOMPLETE (ro-ivb2-i7-3770)
                pass       -> INCOMPLETE (ro-byt-n2820)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup basic-flip-vs-modeset:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup basic-plain-flip:
                pass       -> FAIL       (fi-hsw-i7-4770k)
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup force-edid:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup force-load-detect:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup prune-stale-modes:
                pass       -> FAIL       (fi-hsw-i7-4770k)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> FAIL       (fi-hsw-i7-4770k)
Test kms_pipe_crc_basic:
        Subgroup bad-nb-words-1:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup bad-nb-words-3:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup bad-pipe:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup bad-source:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup hang-read-crc-pipe-a:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup hang-read-crc-pipe-b:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup hang-read-crc-pipe-c:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup nonblocking-crc-pipe-a:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup nonblocking-crc-pipe-b:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup nonblocking-crc-pipe-c:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup nonblocking-crc-pipe-c-frame-sequence:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup read-crc-pipe-a:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup read-crc-pipe-a-frame-sequence:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup read-crc-pipe-b:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup read-crc-pipe-c:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup read-crc-pipe-c-frame-sequence:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup suspend-read-crc-pipe-a:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> FAIL       (fi-hsw-i7-4770k)
Test kms_setmode:
        Subgroup basic-clone-single-crtc:
                pass       -> FAIL       (fi-hsw-i7-4770k)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup basic-rte:
                pass       -> FAIL       (fi-hsw-i7-4770k)

fi-hsw-i7-4770k  total:209  pass:156  dwarn:0   dfail:0   fail:34  skip:19 
ro-bdw-i5-5250u  total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
ro-bdw-i7-5557U  total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
ro-bdw-i7-5600u  total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
ro-bsw-n3050     total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
ro-byt-n2820     total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
ro-hsw-i3-4010u  total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
ro-ilk-i7-620lm  total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
ro-ilk1-i5-650   total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
ro-ivb2-i7-3770  total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
ro-skl-i7-6700hq total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
WARNING: Long output truncated
fi-bdw-i7-5557u failed to connect after reboot
fi-bsw-n3050 failed to connect after reboot
fi-byt-n2820 failed to connect after reboot
fi-hsw-i7-4770r failed to connect after reboot
fi-skl-i7-6700k failed to connect after reboot
fi-snb-i7-2600 failed to connect after reboot
ro-hsw-i7-4770r failed to connect after reboot
ro-ivb-i7-3770 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1023/

fc9d741 drm-intel-nightly: 2016y-05m-25d-07h-45m-48s UTC integration manifest
f3b4c19 drm/i915: Defer enabling rc6 til after we submit the first batch/context
ae82b98 drm: Register the debugfs interfaces after loading the driver
1695450 drm/i915: Preserve current RPS frequency
f7fa651 drm/i915: Only switch to default context when evicting from GGTT
8c19d560 drm/i915: Split idling from forcing context switch
e4d542d drm/i915: No need to wait for idle on L3 remap
1e8f15f drm/i915: Mark all default contexts as uninitialised after context loss
c3a1753 drm/i915: Treat kernel context as initialised
29aeef4 drm/i915: Move legacy kernel context pinning to intel_ringbuffer.c
e2c5530 drm/i915: Skip idling an idle engine

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

[PATCH] drm/i915: Register debugfs interface last

[Chris Wilson]

Currently debugfs files are created before the driver is even loads.
This gives the opportunity for userspace to open that interface and poke
around before the backing data structures are initialised - with the
possibility of oopsing or worse.

Move the creation of the debugfs files to our registration phase, where
we announce our presence to the world when we are ready.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 6 ++++--
 drivers/gpu/drm/i915/i915_dma.c     | 2 ++
 drivers/gpu/drm/i915/i915_drv.c     | 4 ----
 drivers/gpu/drm/i915/i915_drv.h     | 6 ++++--
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 615cef736356..4a3546f114c6 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -5498,8 +5498,9 @@ void intel_display_crc_init(struct drm_device *dev)
 	}
 }
 
-int i915_debugfs_init(struct drm_minor *minor)
+int i915_debugfs_register(struct drm_i915_private *dev_priv)
 {
+	struct drm_minor *minor = dev_priv->dev->primary;
 	int ret, i;
 
 	ret = i915_forcewake_create(minor->debugfs_root, minor);
@@ -5525,8 +5526,9 @@ int i915_debugfs_init(struct drm_minor *minor)
 					minor->debugfs_root, minor);
 }
 
-void i915_debugfs_cleanup(struct drm_minor *minor)
+void i915_debugfs_unregister(struct drm_i915_private *dev_priv)
 {
+	struct drm_minor *minor = dev_priv->dev->primary;
 	int i;
 
 	drm_debugfs_remove_files(i915_debugfs_list,
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 07edaed9d5a2..af0a6ce6f0b9 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1372,6 +1372,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
 	if (intel_vgpu_active(dev_priv))
 		I915_WRITE(vgtif_reg(display_ready), VGT_DRV_DISPLAY_READY);
 
+	i915_debugfs_register(dev_priv);
 	i915_setup_sysfs(dev);
 
 	if (INTEL_INFO(dev_priv)->num_pipes) {
@@ -1397,6 +1398,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 	acpi_video_unregister();
 	intel_opregion_unregister(dev_priv);
 	i915_teardown_sysfs(dev_priv->dev);
+	i915_debugfs_unregister(dev_priv);
 	i915_gem_shrinker_cleanup(dev_priv);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 943d7b222fd4..fa9e16b2247c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1761,10 +1761,6 @@ static struct drm_driver driver = {
 	.postclose = i915_driver_postclose,
 	.set_busid = drm_pci_set_busid,
 
-#if defined(CONFIG_DEBUG_FS)
-	.debugfs_init = i915_debugfs_init,
-	.debugfs_cleanup = i915_debugfs_cleanup,
-#endif
 	.gem_free_object = i915_gem_free_object,
 	.gem_vm_ops = &i915_gem_vm_ops,
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b9d9a4205992..8f2994ef4386 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3543,12 +3543,14 @@ int i915_verify_lists(struct drm_device *dev);
 #endif
 
 /* i915_debugfs.c */
-int i915_debugfs_init(struct drm_minor *minor);
-void i915_debugfs_cleanup(struct drm_minor *minor);
 #ifdef CONFIG_DEBUG_FS
+int i915_debugfs_register(struct drm_i915_private *dev_priv);
+void i915_debugfs_unregister(struct drm_i915_private *dev_priv);
 int i915_debugfs_connector_add(struct drm_connector *connector);
 void intel_display_crc_init(struct drm_device *dev);
 #else
+static inline int i915_debugfs_register(struct drm_i915_private *) {return 0;}
+static inline void i915_debugfs_unregister(struct drm_i915_private *) {}
 static inline int i915_debugfs_connector_add(struct drm_connector *connector)
 { return 0; }
 static inline void intel_display_crc_init(struct drm_device *dev) {}
-- 
2.8.1

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

✗ Ro.CI.BAT: warning for series starting with [v3,01/10] drm/i915: Skip idling an idle engine (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [v3,01/10] drm/i915: Skip idling an idle engine (rev3)
URL   : https://patchwork.freedesktop.org/series/7792/
State : warning

== Summary ==

Series 7792v3 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/7792/revisions/3/mbox

Test gem_basic:
        Subgroup bad-close:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
Test gem_busy:
        Subgroup basic-blt:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test gem_exec_basic:
        Subgroup basic-vebox:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
Test gem_exec_store:
        Subgroup basic-default:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
Test gem_mmap_gtt:
        Subgroup basic-read:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test gem_ringfill:
        Subgroup basic-default-forked:
                pass       -> DMESG-WARN (ro-byt-n2820)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (ro-bdw-i7-5600u)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)

fi-bdw-i7-5557u  total:209  pass:197  dwarn:0   dfail:0   fail:0   skip:12 
fi-bsw-n3050     total:209  pass:167  dwarn:0   dfail:0   fail:2   skip:40 
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-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:168  dwarn:0   dfail:0   fail:2   skip:39 
ro-byt-n2820     total:209  pass:169  dwarn:1   dfail:0   fail:2   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-skl-i7-6700hq total:204  pass:178  dwarn:5   dfail:0   fail:0   skip:21 
ro-snb-i7-2620M  total:209  pass:170  dwarn:0   dfail:0   fail:1   skip:38 
fi-byt-n2820 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1025/

fc9d741 drm-intel-nightly: 2016y-05m-25d-07h-45m-48s UTC integration manifest
c654dd5 drm/i915: Defer enabling rc6 til after we submit the first batch/context
9e6501a drm/i915: Register debugfs interface last
e9d58a4 drm/i915: Preserve current RPS frequency
6b991ad drm/i915: Only switch to default context when evicting from GGTT
e67b6dc drm/i915: Split idling from forcing context switch
0b8ae7f drm/i915: No need to wait for idle on L3 remap
0ac86ee drm/i915: Mark all default contexts as uninitialised after context loss
87cba42 drm/i915: Treat kernel context as initialised
16ea248 drm/i915: Move legacy kernel context pinning to intel_ringbuffer.c
b828f51 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] drm/i915: Register debugfs interface last

[Ville Syrjälä]

On Thu, May 26, 2016 at 02:17:27PM +0100, Chris Wilson wrote:
> Currently debugfs files are created before the driver is even loads.
> This gives the opportunity for userspace to open that interface and poke
> around before the backing data structures are initialised - with the
> possibility of oopsing or worse.
> 
> Move the creation of the debugfs files to our registration phase, where
> we announce our presence to the world when we are ready.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

Makes sense to me. I never understood why these kinds of hooks existed
in the first place.

So the sequence changes from

drm_dev_register()
 -> drm_minor_register()
   -> drm_debugfs_init()
     -> i915_debugfs_init()
 -> i915_driver_load()

to something like

drm_dev_register()
 -> drm_minor_register()
   -> drm_debugfs_init()
 -> i915_driver_load()
   -> i915_debugfs_register()

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 6 ++++--
>  drivers/gpu/drm/i915/i915_dma.c     | 2 ++
>  drivers/gpu/drm/i915/i915_drv.c     | 4 ----
>  drivers/gpu/drm/i915/i915_drv.h     | 6 ++++--
>  4 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 615cef736356..4a3546f114c6 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -5498,8 +5498,9 @@ void intel_display_crc_init(struct drm_device *dev)
>  	}
>  }
>  
> -int i915_debugfs_init(struct drm_minor *minor)
> +int i915_debugfs_register(struct drm_i915_private *dev_priv)
>  {
> +	struct drm_minor *minor = dev_priv->dev->primary;
>  	int ret, i;
>  
>  	ret = i915_forcewake_create(minor->debugfs_root, minor);
> @@ -5525,8 +5526,9 @@ int i915_debugfs_init(struct drm_minor *minor)
>  					minor->debugfs_root, minor);
>  }
>  
> -void i915_debugfs_cleanup(struct drm_minor *minor)
> +void i915_debugfs_unregister(struct drm_i915_private *dev_priv)
>  {
> +	struct drm_minor *minor = dev_priv->dev->primary;
>  	int i;
>  
>  	drm_debugfs_remove_files(i915_debugfs_list,
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 07edaed9d5a2..af0a6ce6f0b9 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1372,6 +1372,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>  	if (intel_vgpu_active(dev_priv))
>  		I915_WRITE(vgtif_reg(display_ready), VGT_DRV_DISPLAY_READY);
>  
> +	i915_debugfs_register(dev_priv);
>  	i915_setup_sysfs(dev);
>  
>  	if (INTEL_INFO(dev_priv)->num_pipes) {
> @@ -1397,6 +1398,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>  	acpi_video_unregister();
>  	intel_opregion_unregister(dev_priv);
>  	i915_teardown_sysfs(dev_priv->dev);
> +	i915_debugfs_unregister(dev_priv);
>  	i915_gem_shrinker_cleanup(dev_priv);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 943d7b222fd4..fa9e16b2247c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1761,10 +1761,6 @@ static struct drm_driver driver = {
>  	.postclose = i915_driver_postclose,
>  	.set_busid = drm_pci_set_busid,
>  
> -#if defined(CONFIG_DEBUG_FS)
> -	.debugfs_init = i915_debugfs_init,
> -	.debugfs_cleanup = i915_debugfs_cleanup,
> -#endif
>  	.gem_free_object = i915_gem_free_object,
>  	.gem_vm_ops = &i915_gem_vm_ops,
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b9d9a4205992..8f2994ef4386 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3543,12 +3543,14 @@ int i915_verify_lists(struct drm_device *dev);
>  #endif
>  
>  /* i915_debugfs.c */
> -int i915_debugfs_init(struct drm_minor *minor);
> -void i915_debugfs_cleanup(struct drm_minor *minor);
>  #ifdef CONFIG_DEBUG_FS
> +int i915_debugfs_register(struct drm_i915_private *dev_priv);
> +void i915_debugfs_unregister(struct drm_i915_private *dev_priv);
>  int i915_debugfs_connector_add(struct drm_connector *connector);
>  void intel_display_crc_init(struct drm_device *dev);
>  #else
> +static inline int i915_debugfs_register(struct drm_i915_private *) {return 0;}
> +static inline void i915_debugfs_unregister(struct drm_i915_private *) {}
>  static inline int i915_debugfs_connector_add(struct drm_connector *connector)
>  { return 0; }
>  static inline void intel_display_crc_init(struct drm_device *dev) {}
> -- 
> 2.8.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Register the debugfs interfaces after loading the driver

[Daniel Vetter]

On Thu, May 26, 2016 at 02:06:13PM +0100, Chris Wilson wrote:
> On Thu, May 26, 2016 at 01:35:18PM +0100, Chris Wilson wrote:
> > In order to give the driver the chance to initialise the data structures
> > that will be exposed through debugfs, perform driver->load() before
> > registering the debugfs entries. (Otherwise it may be possible for
> > userspace to cause an oops through the debugfs interfaces.) As the
> > driver load is now before debugfs registration, make the registration
> > non-fatal (as it simply prevents us exposing an optional debug facility
> > and not hard ABI).
> 
> The alternative here would be for i915.ko to stop registering a
> driver->debugfs_init and do it as part of its registration phase (like
> sysfs).

I think the right fix would be to demidlayer i915 and stop using the
->load callback. Then we do have the right ordering, since debugfs setup
is done as part of the register phase.

It's just that for historical raisins load is called after the devnodes
are registered (because some dri1 drivers want to look at master/sarea
stuff iirc).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Register the debugfs interfaces after loading the driver

[Chris Wilson]

On Fri, May 27, 2016 at 08:47:48AM +0200, Daniel Vetter wrote:
> On Thu, May 26, 2016 at 02:06:13PM +0100, Chris Wilson wrote:
> > On Thu, May 26, 2016 at 01:35:18PM +0100, Chris Wilson wrote:
> > > In order to give the driver the chance to initialise the data structures
> > > that will be exposed through debugfs, perform driver->load() before
> > > registering the debugfs entries. (Otherwise it may be possible for
> > > userspace to cause an oops through the debugfs interfaces.) As the
> > > driver load is now before debugfs registration, make the registration
> > > non-fatal (as it simply prevents us exposing an optional debug facility
> > > and not hard ABI).
> > 
> > The alternative here would be for i915.ko to stop registering a
> > driver->debugfs_init and do it as part of its registration phase (like
> > sysfs).
> 
> I think the right fix would be to demidlayer i915 and stop using the
> ->load callback. Then we do have the right ordering, since debugfs setup
> is done as part of the register phase.

We would need to take control from drm_get_pci_dev() if we want to
publish the device nodes during our registration phase (which we do).
One way would be to opencode drm_get_pci_dev() and do the driver setup
entirely ourselves from i915_pci_probe() then register.

Seems reasonable, biggest challenge will be to make sure we aren't
dependent on the minors before registration (but that shouldn't be too
much of a challenge, at worst it means we have bad ordering to fixup).
-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