[PATCH 1/4] drm/i915/contexts: fix list corruption

[PATCH 1/4] drm/i915/contexts: fix list corruption

[Ben Widawsky]

After reset we unconditionally reinitialize lists. If the context switch
hasn't yet completed before the suspend the default context object will
end up on lists that are going to go away when we resume.

The patch forces the context switch to be synchronous before suspend
assuring that the active/inactive tracking is correct at the time of
resume.

References: https://bugs.freedesktop.org/show_bug.cgi?id=52429
Tested-by: Guang A Yang <guang.a.yang@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0514593..31054fa 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2273,11 +2273,11 @@ int i915_gpu_idle(struct drm_device *dev)
 
 	/* Flush everything onto the inactive list. */
 	for_each_ring(ring, dev_priv, i) {
-		ret = i915_ring_idle(ring);
+		ret = i915_switch_context(ring, NULL, DEFAULT_CONTEXT_ID);
 		if (ret)
 			return ret;
 
-		ret = i915_switch_context(ring, NULL, DEFAULT_CONTEXT_ID);
+		ret = i915_ring_idle(ring);
 		if (ret)
 			return ret;
 	}
-- 
1.7.11.4

[PATCH 2/4] drm/i915/contexts: Add forced switches

[Ben Widawsky]

A force parameter for switch currently only has one use, the first time
we load the default context.

Slightly hand-wavy explanation: We want to get the default context
actually loaded so that the GPU has some real state to load (instead of
garbage) after a reset or resume

Therefore, the benefit to adding a parameter instead of trying to
determine when to force is that we can strictly limit when such switches
occur.

As a +1 to me:
This existed in earlier versions of the patch series, but got removed as
part of the review process. The reason before was the same, and the same
person who convinced me to remove it before has convinced me to re-add
it. :-)

References: https://bugs.freedesktop.org/show_bug.cgi?id=52429
Tested-by: Guang A Yang <guang.a.yang@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 5c2d354..3945e79 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -97,7 +97,7 @@
 
 static struct i915_hw_context *
 i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
-static int do_switch(struct i915_hw_context *to);
+static int do_switch(struct i915_hw_context *to, bool force);
 
 static int get_context_size(struct drm_device *dev)
 {
@@ -225,7 +225,7 @@ static int create_default_context(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_destroy;
 
-	ret = do_switch(ctx);
+	ret = do_switch(ctx, false);
 	if (ret)
 		goto err_unpin;
 
@@ -362,7 +362,7 @@ mi_set_context(struct intel_ring_buffer *ring,
 	return ret;
 }
 
-static int do_switch(struct i915_hw_context *to)
+static int do_switch(struct i915_hw_context *to, bool force)
 {
 	struct intel_ring_buffer *ring = to->ring;
 	struct drm_i915_gem_object *from_obj = ring->last_context_obj;
@@ -371,7 +371,7 @@ static int do_switch(struct i915_hw_context *to)
 
 	BUG_ON(from_obj != NULL && from_obj->pin_count == 0);
 
-	if (from_obj == to->obj)
+	if (!force && (from_obj == to->obj))
 		return 0;
 
 	ret = i915_gem_object_pin(to->obj, CONTEXT_ALIGN, false);
@@ -392,10 +392,10 @@ static int do_switch(struct i915_hw_context *to)
 	if (!to->obj->has_global_gtt_mapping)
 		i915_gem_gtt_bind_object(to->obj, to->obj->cache_level);
 
-	if (!to->is_initialized || is_default_context(to))
-		hw_flags |= MI_RESTORE_INHIBIT;
-	else if (WARN_ON_ONCE(from_obj == to->obj)) /* not yet expected */
+	if (force) {
 		hw_flags |= MI_FORCE_RESTORE;
+	} else if (!to->is_initialized || is_default_context(to))
+		hw_flags |= MI_RESTORE_INHIBIT;
 
 	ret = mi_set_context(ring, to, hw_flags);
 	if (ret) {
@@ -471,7 +471,7 @@ int i915_switch_context(struct intel_ring_buffer *ring,
 			return -ENOENT;
 	}
 
-	return do_switch(to);
+	return do_switch(to, false);
 }
 
 int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
-- 
1.7.11.4

[PATCH 3/4] drm/i915/contexts: Serialize default context init

[Ben Widawsky]

This is possible with the new force paramter in do_switch. As stated in
that patch, the goal is to get a real context stored at the time of
initialization.

References: https://bugs.freedesktop.org/show_bug.cgi?id=52429
Tested-by: Guang A Yang <guang.a.yang@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 3945e79..c96d6f2 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -229,6 +229,10 @@ static int create_default_context(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_unpin;
 
+	ret = do_switch(ctx, true);
+	if (ret)
+		goto err_unpin;
+
 	DRM_DEBUG_DRIVER("Default HW context loaded\n");
 	return 0;
 
-- 
1.7.11.4

Re: [PATCH 3/4] drm/i915/contexts: Serialize default context init

[Chris Wilson]

On Mon, 13 Aug 2012 22:41:10 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> This is possible with the new force paramter in do_switch. As stated in
> that patch, the goal is to get a real context stored at the time of
> initialization.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=52429
> Tested-by: Guang A Yang <guang.a.yang@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I'm missing the rationalisation for this pair of patches... For
instance, I can't see how this closes the hole we have upon resume where
ring->context_obj == DEFAULT_CONTEXT but CCID is 0.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/4] drm/i915/contexts: Serialize default context init

[Ben Widawsky]

On 2012-08-14 00:41, Chris Wilson wrote:
> On Mon, 13 Aug 2012 22:41:10 -0700, Ben Widawsky <ben@bwidawsk.net> 
> wrote:
>> This is possible with the new force paramter in do_switch. As stated 
>> in
>> that patch, the goal is to get a real context stored at the time of
>> initialization.
>>
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=52429
>> Tested-by: Guang A Yang <guang.a.yang@intel.com>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>
> I'm missing the rationalisation for this pair of patches... For
> instance, I can't see how this closes the hole we have upon resume 
> where
> ring->context_obj == DEFAULT_CONTEXT but CCID is 0.
> -Chris

Yeah this doesn't fix that problem. The problem this is trying to solve 
is suspend/resume before any context switch actually occurs. Basically 
jam the default context obj in, and this allows us to force restore it 
on resume. However, as you point out, I guess that force restore is 
missing. Let me think a bit more/chat on IRC.

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 4/4] drm/i915: Cleanup instdone state when idle

[Ben Widawsky]

The previous state is bogus when we've gone into idle. Actually there
would be a known state of idle (ie. all units are done), but since it
differs for every platform, we can just set 0, and let the hangcheck
progress as normal.

References: https://bugs.freedesktop.org/show_bug.cgi?id=52429
Tested-by: Guang A Yang <guang.a.yang@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 31054fa..5f116a1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3624,6 +3624,9 @@ i915_gem_idle(struct drm_device *dev)
 	/* Cancel the retire work handler, which should be idle now. */
 	cancel_delayed_work_sync(&dev_priv->mm.retire_work);
 
+	dev_priv->last_instdone = 0;
+	dev_priv->last_instdone1 = 0;
+
 	return 0;
 }
 
-- 
1.7.11.4

Re: [PATCH 4/4] drm/i915: Cleanup instdone state when idle

[Chris Wilson]

On Mon, 13 Aug 2012 22:41:11 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> The previous state is bogus when we've gone into idle. Actually there
> would be a known state of idle (ie. all units are done), but since it
> differs for every platform, we can just set 0, and let the hangcheck
> progress as normal.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=52429
> Tested-by: Guang A Yang <guang.a.yang@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I think you have a point, but I don't think this should be just on idle.
Everytime we kick off the hangcheck, we should not be comparing to stale
state.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: Cleanup instdone state when idle

[Ben Widawsky]

On 2012-08-14 00:39, Chris Wilson wrote:
> On Mon, 13 Aug 2012 22:41:11 -0700, Ben Widawsky <ben@bwidawsk.net> 
> wrote:
>> The previous state is bogus when we've gone into idle. Actually 
>> there
>> would be a known state of idle (ie. all units are done), but since 
>> it
>> differs for every platform, we can just set 0, and let the hangcheck
>> progress as normal.
>>
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=52429
>> Tested-by: Guang A Yang <guang.a.yang@intel.com>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>
> I think you have a point, but I don't think this should be just on 
> idle.
> Everytime we kick off the hangcheck, we should not be comparing to 
> stale
> state.
> -Chris

Are you suggesting this is a patch which already exists somewhere?

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 4/4] drm/i915: Cleanup instdone state when idle

[Chris Wilson]

On Tue, 14 Aug 2012 09:42:07 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On 2012-08-14 00:39, Chris Wilson wrote:
> > On Mon, 13 Aug 2012 22:41:11 -0700, Ben Widawsky <ben@bwidawsk.net> 
> > wrote:
> >> The previous state is bogus when we've gone into idle. Actually 
> >> there
> >> would be a known state of idle (ie. all units are done), but since 
> >> it
> >> differs for every platform, we can just set 0, and let the hangcheck
> >> progress as normal.
> >>
> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=52429
> >> Tested-by: Guang A Yang <guang.a.yang@intel.com>
> >> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> >
> > I think you have a point, but I don't think this should be just on 
> > idle.
> > Everytime we kick off the hangcheck, we should not be comparing to 
> > stale
> > state.
> 
> Are you suggesting this is a patch which already exists somewhere?

Not yet. :) Just that I agree with you that this a problem, and it is a
bigger problem than first thought.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/4] drm/i915/contexts: fix list corruption

[Chris Wilson]

On Mon, 13 Aug 2012 22:41:08 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> After reset we unconditionally reinitialize lists. If the context switch
> hasn't yet completed before the suspend the default context object will
> end up on lists that are going to go away when we resume.
> 
> The patch forces the context switch to be synchronous before suspend
> assuring that the active/inactive tracking is correct at the time of
> resume.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=52429
> Tested-by: Guang A Yang <guang.a.yang@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

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

-- 
Chris Wilson, Intel Open Source Technology Centre