[PATCH 1/6] drm/i915: Prevent signals from interrupting close()

[PATCH 1/6] drm/i915: Prevent signals from interrupting close()

[Ben Widawsky]

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

We neither report any unfinished operations during releasing GEM objects
associated with the file, and even if we did, it is bad form to report
-EINTR from a close().

The root cause of the bug that first showed itself during close is that
we do not do proper live tracking of vma and contexts under full-ppgtt,
but this is useful piece of defensive programming enforcing our
userspace API contract.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 5e583a1..f6aca71 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1959,9 +1959,18 @@ void i915_driver_lastclose(struct drm_device *dev)
 
 void i915_driver_preclose(struct drm_device *dev, struct drm_file *file_priv)
 {
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	bool was_interruptible;
+
 	mutex_lock(&dev->struct_mutex);
+	was_interruptible = dev_priv->mm.interruptible;
+	WARN_ON(!was_interruptible);
+	dev_priv->mm.interruptible = false;
+
 	i915_gem_context_close(dev, file_priv);
 	i915_gem_release(dev, file_priv);
+
+	dev_priv->mm.interruptible = was_interruptible;
 	mutex_unlock(&dev->struct_mutex);
 }
 
-- 
2.0.0

[PATCH 2/6] drm/i915: Only mark the ctx as initialised after a SET_CONTEXT operation

[Ben Widawsky]

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

Fallout from

commit 46470fc932ac8a0e8317a220b3f4ea4ed903338e
Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Date:   Wed May 21 19:01:06 2014 +0300

    drm/i915: Add null state batch to active list

undid the earlier fix of only marking the ctx as initialised after it is
saved by the hardware during a SET_CONTEXT operation.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 21eda88..0d2c75b 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -598,6 +598,7 @@ static int do_switch(struct intel_engine_cs *ring,
 	struct intel_context *from = ring->last_context;
 	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
 	u32 hw_flags = 0;
+	bool uninitialized = false;
 	int ret, i;
 
 	if (from != NULL && ring == &dev_priv->ring[RCS]) {
@@ -696,18 +697,19 @@ static int do_switch(struct intel_engine_cs *ring,
 		i915_gem_context_unreference(from);
 	}
 
+	uninitialized = !to->is_initialized && from == NULL;
+	to->is_initialized = true;
+
 done:
 	i915_gem_context_reference(to);
 	ring->last_context = to;
 
-	if (ring->id == RCS && !to->is_initialized && from == NULL) {
+	if (uninitialized) {
 		ret = i915_gem_render_state_init(ring);
 		if (ret)
 			DRM_ERROR("init render state: %d\n", ret);
 	}
 
-	to->is_initialized = true;
-
 	return 0;
 
 unpin_out:
-- 
2.0.0

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

[PATCH 3/6] drm/i915: Split up do_switch

[Ben Widawsky]

There are two important reasons for this patch. It should make the
existing code a lot more readable. It also makes the next patch much
easier to understand in my opinion. There are 2 main variables that
effect this function, leaving 4 permutations:
ring: RCS vs !RCS
PPGTT: full or not

I didn't find extracting the full PPGTT usage to be very beneficial at
this point, but it may be in the future.

This was originally recommended by Daniel Vetter, and in this case, I
agree. There was no intentional behavioral change.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 76 +++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 0d2c75b..35985ad 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -591,31 +591,57 @@ mi_set_context(struct intel_engine_cs *ring,
 	return ret;
 }
 
-static int do_switch(struct intel_engine_cs *ring,
-		     struct intel_context *to)
+static void do_switch_fini_common(struct intel_engine_cs *ring,
+				  struct intel_context *from,
+				  struct intel_context *to)
+{
+	if (likely(from))
+		i915_gem_context_unreference(from);
+	i915_gem_context_reference(to);
+	ring->last_context = to;
+}
+
+static int do_switch_xcs(struct intel_engine_cs *ring,
+			 struct intel_context *from,
+			 struct intel_context *to)
+{
+	struct drm_device *dev = ring->dev;
+	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
+	int ret;
+
+	BUG_ON(from && from->obj != NULL);
+
+	if (USES_FULL_PPGTT(dev)) {
+		ret = ppgtt->switch_mm(ppgtt, ring, false);
+		if (ret)
+			return ret;
+	}
+
+	if (from)
+		do_switch_fini_common(ring, from, to);
+
+	return 0;
+}
+
+static int do_switch_rcs(struct intel_engine_cs *ring,
+			 struct intel_context *from,
+			 struct intel_context *to)
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
-	struct intel_context *from = ring->last_context;
 	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
 	u32 hw_flags = 0;
 	bool uninitialized = false;
 	int ret, i;
 
-	if (from != NULL && ring == &dev_priv->ring[RCS]) {
+	if (from != NULL) {
 		BUG_ON(from->obj == NULL);
 		BUG_ON(!i915_gem_obj_is_pinned(from->obj));
 	}
 
-	if (from == to && !to->remap_slice)
-		return 0;
-
 	/* Trying to pin first makes error handling easier. */
-	if (ring == &dev_priv->ring[RCS]) {
-		ret = i915_gem_obj_ggtt_pin(to->obj,
-					    get_context_alignment(ring->dev), 0);
-		if (ret)
-			return ret;
-	}
+	ret = i915_gem_obj_ggtt_pin(to->obj, get_context_alignment(ring->dev), 0);
+	if (ret)
+		return ret;
 
 	/*
 	 * Pin can switch back to the default context if we end up calling into
@@ -630,12 +656,6 @@ static int do_switch(struct intel_engine_cs *ring,
 			goto unpin_out;
 	}
 
-	if (ring != &dev_priv->ring[RCS]) {
-		if (from)
-			i915_gem_context_unreference(from);
-		goto done;
-	}
-
 	/*
 	 * Clear this page out of any CPU caches for coherent swap-in/out. Note
 	 * that thanks to write = false in this call and us not setting any gpu
@@ -694,15 +714,11 @@ static int do_switch(struct intel_engine_cs *ring,
 
 		/* obj is kept alive until the next request by its active ref */
 		i915_gem_object_ggtt_unpin(from->obj);
-		i915_gem_context_unreference(from);
 	}
 
 	uninitialized = !to->is_initialized && from == NULL;
 	to->is_initialized = true;
-
-done:
-	i915_gem_context_reference(to);
-	ring->last_context = to;
+	do_switch_fini_common(ring, from, to);
 
 	if (uninitialized) {
 		ret = i915_gem_render_state_init(ring);
@@ -713,8 +729,7 @@ done:
 	return 0;
 
 unpin_out:
-	if (ring->id == RCS)
-		i915_gem_object_ggtt_unpin(to->obj);
+	i915_gem_object_ggtt_unpin(to->obj);
 	return ret;
 }
 
@@ -732,6 +747,7 @@ int i915_switch_context(struct intel_engine_cs *ring,
 			struct intel_context *to)
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct intel_context *from = ring->last_context;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
 
@@ -745,7 +761,13 @@ int i915_switch_context(struct intel_engine_cs *ring,
 		return 0;
 	}
 
-	return do_switch(ring, to);
+	if (from == to && !to->remap_slice)
+		return 0;
+
+	if (ring->id == RCS)
+		return do_switch_rcs(ring, from, to);
+	else
+		return do_switch_xcs(ring, from, to);
 }
 
 static bool hw_context_enabled(struct drm_device *dev)
-- 
2.0.0

[PATCH 4/6] drm/i915: Extract l3 remapping out of ctx switch

[Ben Widawsky]

This is just a cosmetic change to try to put do_switch_rcs on a diet. As
it stands, the function was quite complex, and error prone.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 35985ad..f2c4948 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -623,6 +623,24 @@ static int do_switch_xcs(struct intel_engine_cs *ring,
 	return 0;
 }
 
+static void remap_l3(struct intel_engine_cs *ring,
+		     struct intel_context *ctx)
+{
+	int ret, i;
+
+	for (i = 0; i < MAX_L3_SLICES; i++) {
+		if (!(ctx->remap_slice & (1<<i)))
+			continue;
+
+		ret = i915_gem_l3_remap(ring, i);
+		/* If it failed, try again next round */
+		if (ret)
+			DRM_DEBUG_DRIVER("L3 remapping failed\n");
+		else
+			ctx->remap_slice &= ~(1<<i);
+	}
+}
+
 static int do_switch_rcs(struct intel_engine_cs *ring,
 			 struct intel_context *from,
 			 struct intel_context *to)
@@ -631,7 +649,7 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
 	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
 	u32 hw_flags = 0;
 	bool uninitialized = false;
-	int ret, i;
+	int ret;
 
 	if (from != NULL) {
 		BUG_ON(from->obj == NULL);
@@ -681,17 +699,7 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
 	if (ret)
 		goto unpin_out;
 
-	for (i = 0; i < MAX_L3_SLICES; i++) {
-		if (!(to->remap_slice & (1<<i)))
-			continue;
-
-		ret = i915_gem_l3_remap(ring, i);
-		/* If it failed, try again next round */
-		if (ret)
-			DRM_DEBUG_DRIVER("L3 remapping failed\n");
-		else
-			to->remap_slice &= ~(1<<i);
-	}
+	remap_l3(ring, to);
 
 	/* The backing object for the context is done after switching to the
 	 * *next* context. Therefore we cannot retire the previous context until
-- 
2.0.0

[PATCH 5/6] drm/i915/ppgtt: Load address space after mi_set_context

[Ben Widawsky]

The simple explanation is, the docs say to do this for GEN8. Perhaps we
want to do this for GEN7 too, I am not certain.

PDPs are saved and restored with context. Contexts (without execlists)
only exist on the render ring. The docs say that PDPs are not power
context save/restored.  I've learned that this actually means something
which SW doesn't care about. So pretend the statement doesn't exist.
For non RCS, nothing changes.

All this patch now does is change the ordering of LRI vs MI_SET_CONTEXT
for the initialization of the context. I do this because the docs say to
do it, and frankly, I cannot reason why it is necessary. I've thought
about it a lot, and tried, without success, to get a reason from design.
The answer I got more or less says, "gen7 is different than gen8." I've
given up, and am adding this little bit of code to make it in sync with
the docs.

v2: Completely rewritten commit message that addresses the requests
Ville made for v1
Only load PDPs for initial context load (Ville)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index f2c4948..7c69738 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -649,6 +649,7 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
 	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
 	u32 hw_flags = 0;
 	bool uninitialized = false;
+	bool needs_pd_load = (INTEL_INFO(ring->dev)->gen < 8) && USES_FULL_PPGTT(ring->dev);
 	int ret;
 
 	if (from != NULL) {
@@ -668,7 +669,10 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
 	 */
 	from = ring->last_context;
 
-	if (USES_FULL_PPGTT(ring->dev)) {
+	if (needs_pd_load) {
+		/* Older GENs still want the load first, "PP_DCLV followed by
+		 * PP_DIR_BASE register through Load Register Immediate commands
+		 * in Ring Buffer before submitting a context."*/
 		ret = ppgtt->switch_mm(ppgtt, ring, false);
 		if (ret)
 			goto unpin_out;
@@ -692,13 +696,34 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
 		vma->bind_vma(vma, to->obj->cache_level, GLOBAL_BIND);
 	}
 
-	if (!to->is_initialized || i915_gem_context_is_default(to))
+	if (!to->is_initialized || i915_gem_context_is_default(to)) {
 		hw_flags |= MI_RESTORE_INHIBIT;
+		needs_pd_load = USES_FULL_PPGTT(ring->dev) && IS_GEN8(ring->dev);
+	}
 
 	ret = mi_set_context(ring, to, hw_flags);
 	if (ret)
 		goto unpin_out;
 
+	/* GEN8 does *not* require an explicit reload if the PDPs have been
+	 * setup, and we do not wish to move them.
+	 *
+	 * XXX: If we implemented page directory eviction code, this
+	 * optimization needs to be removed.
+	 */
+	if (needs_pd_load) {
+		ret = ppgtt->switch_mm(ppgtt, ring, false);
+		/* The hardware context switch is emitted, but we haven't
+		 * actually changed the state - so it's probably safe to bail
+		 * here. Still, let the user know something dangerous has
+		 * happened.
+		 */
+		if (ret) {
+			DRM_ERROR("Failed to change address space on context switch\n");
+			goto unpin_out;
+		}
+	}
+
 	remap_l3(ring, to);
 
 	/* The backing object for the context is done after switching to the
-- 
2.0.0

[PATCH 6/6] drm/i915/bdw: Enable full PPGTT

[Ben Widawsky]

Broadwell is perfectly capable of full PPGTT. I've been using it for
some time, and seen no especially ill effects.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Conflicts:
	drivers/gpu/drm/i915/i915_drv.h
---
 drivers/gpu/drm/i915/i915_drv.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8cea596..e91f27a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1992,7 +1992,7 @@ struct drm_i915_cmd_table {
 
 #define HAS_HW_CONTEXTS(dev)	(INTEL_INFO(dev)->gen >= 6)
 #define HAS_ALIASING_PPGTT(dev)	(INTEL_INFO(dev)->gen >= 6)
-#define HAS_PPGTT(dev)		(INTEL_INFO(dev)->gen >= 7 && !IS_GEN8(dev))
+#define HAS_PPGTT(dev)		(INTEL_INFO(dev)->gen >= 7 && !IS_CHERRYVIEW(dev))
 #define USES_PPGTT(dev)		intel_enable_ppgtt(dev, false)
 #define USES_FULL_PPGTT(dev)	intel_enable_ppgtt(dev, true)
 
-- 
2.0.0

Re: [PATCH 2/6] drm/i915: Only mark the ctx as initialised after a SET_CONTEXT operation

[Jani Nikula]

On Tue, 24 Jun 2014, Ben Widawsky <benjamin.widawsky@intel.com> wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
>
> Fallout from
>
> commit 46470fc932ac8a0e8317a220b3f4ea4ed903338e
> Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Date:   Wed May 21 19:01:06 2014 +0300
>
>     drm/i915: Add null state batch to active list
>
> undid the earlier fix of only marking the ctx as initialised after it is
> saved by the hardware during a SET_CONTEXT operation.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>

I've pushed this one patch to -fixes.

BR,
Jani.

> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 21eda88..0d2c75b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -598,6 +598,7 @@ static int do_switch(struct intel_engine_cs *ring,
>  	struct intel_context *from = ring->last_context;
>  	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
>  	u32 hw_flags = 0;
> +	bool uninitialized = false;
>  	int ret, i;
>  
>  	if (from != NULL && ring == &dev_priv->ring[RCS]) {
> @@ -696,18 +697,19 @@ static int do_switch(struct intel_engine_cs *ring,
>  		i915_gem_context_unreference(from);
>  	}
>  
> +	uninitialized = !to->is_initialized && from == NULL;
> +	to->is_initialized = true;
> +
>  done:
>  	i915_gem_context_reference(to);
>  	ring->last_context = to;
>  
> -	if (ring->id == RCS && !to->is_initialized && from == NULL) {
> +	if (uninitialized) {
>  		ret = i915_gem_render_state_init(ring);
>  		if (ret)
>  			DRM_ERROR("init render state: %d\n", ret);
>  	}
>  
> -	to->is_initialized = true;
> -
>  	return 0;
>  
>  unpin_out:
> -- 
> 2.0.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx