[PATCH 0/5] Some HW context fixes

[PATCH 0/5] Some HW context fixes

[Ben Widawsky]

Daniel, the first 3 patches I believe are all that's required to fix
https://bugs.freedesktop.org/show_bug.cgi?id=52429 The latter 2 are
things we've discussed sort of. I've managed to go back and forth
several times on whether or not we actually need them, so I'll just let
you decide if you want them. I'm not completely certain they aren't
required to fix #52429, but it seems likely.

Ben Widawsky (5):
  drm/i915: Cleanup instdone state
  drm/i915/contexts: fix list corruption
  drm/i915/contexts: Switch to default on resume
  drm/i915/contexts: Add forced switches
  drm/i915/contexts: Serialize default context init

 drivers/gpu/drm/i915/i915_gem.c         |  5 +++--
 drivers/gpu/drm/i915/i915_gem_context.c | 37 +++++++++++++++++++++++----------
 2 files changed, 29 insertions(+), 13 deletions(-)

-- 
1.7.11.4

[PATCH 1/5] drm/i915: Cleanup instdone state

[Ben Widawsky]

Clear the cached instdone state to match what we expect from hardware
and prevent us from comparing stale values.

Actually, clearing the state is not the same as setting idle state.
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.

By putting the clear into add_request we are essentially initializing
the cached instdone to a known state before we start the hangcheck
timer.

v2: clear instdone in more place (Chris)
Rewrote the commit message

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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0514593..0d992e6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1601,6 +1601,7 @@ i915_add_request(struct intel_ring_buffer *ring,
 
 	if (!dev_priv->mm.suspended) {
 		if (i915_enable_hangcheck) {
+			dev_priv->last_instdone = dev_priv->last_instdone1 = 0;
 			mod_timer(&dev_priv->hangcheck_timer,
 				  jiffies +
 				  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
-- 
1.7.11.4

Re: [PATCH 1/5] drm/i915: Cleanup instdone state

[Chris Wilson]

On Tue, 14 Aug 2012 14:35:13 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> Clear the cached instdone state to match what we expect from hardware
> and prevent us from comparing stale values.
> 
> Actually, clearing the state is not the same as setting idle state.
> 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.
> 
> By putting the clear into add_request we are essentially initializing
> the cached instdone to a known state before we start the hangcheck
> timer.
> 
> v2: clear instdone in more place (Chris)
> Rewrote the commit message
> 
> 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>

So if hangcheck is already running and we add a new request, we still
clear the "stale" state. This means that we may take an extra tick after
a new request for hangcheck to raise an error. Not a big deal and it
keeps the code clean. Though possibly deserves a comment?

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/5] drm/i915: Cleanup instdone state

[Ben Widawsky]

On 2012-08-15 02:14, Chris Wilson wrote:
> On Tue, 14 Aug 2012 14:35:13 -0700, Ben Widawsky <ben@bwidawsk.net> 
> wrote:
>> Clear the cached instdone state to match what we expect from 
>> hardware
>> and prevent us from comparing stale values.
>>
>> Actually, clearing the state is not the same as setting idle state.
>> 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.
>>
>> By putting the clear into add_request we are essentially 
>> initializing
>> the cached instdone to a known state before we start the hangcheck
>> timer.
>>
>> v2: clear instdone in more place (Chris)
>> Rewrote the commit message
>>
>> 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>
>
> So if hangcheck is already running and we add a new request, we still
> clear the "stale" state. This means that we may take an extra tick 
> after
> a new request for hangcheck to raise an error. Not a big deal and it
> keeps the code clean. Though possibly deserves a comment?
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris

I was thinking about this exactly. I rationalized it as comparing stale 
state and comparing 0 state should result in the same number of retries, 
unless of course stale state happens to be equal to the state at the 
first timeout. This thinking led me to start thinking that this patch 
wasn't even really necessary at all, so I started going lalala.

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 2/5] 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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 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 0d992e6..0edec12 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2274,11 +2274,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

Re: [PATCH 2/5] drm/i915/contexts: fix list corruption

[Daniel Vetter]

On Tue, Aug 14, 2012 at 02:35:14PM -0700, Ben Widawsky 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>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Picked up for -fixes (with a minor conflict resolved), thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 3/5] drm/i915/contexts: Switch to default on resume

[Ben Widawsky]

In order to make the HW state CCID match with what we think it should
be, we must order a switch to the default context.

The really sad thing here is that the switch can potentially fail, and
as such we have to assume contexts no longer work. There is likely room
for improvement but until we actually start seeing the case occur, I
think it should be fine.

This was accidentally left this out of the first series, noticed by
Chris Wilson.

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

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 5c2d354..3e884dc 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -244,15 +244,26 @@ void i915_gem_context_init(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t ctx_size;
 
+	if (dev_priv->hw_contexts_disabled)
+		return;
+
 	if (!HAS_HW_CONTEXTS(dev)) {
 		dev_priv->hw_contexts_disabled = true;
 		return;
 	}
 
-	/* If called from reset, or thaw... we've been here already */
-	if (dev_priv->hw_contexts_disabled ||
-	    dev_priv->ring[RCS].default_context)
+	/* If called from reset, or thaw we want to switch back to the default
+	 * context so our HW CCID matches what we think it should be in software.
+	 */
+	if (dev_priv->ring[RCS].default_context) {
+		int ret = do_switch(dev_priv->ring[RCS].default_context);
+		if (ret) {
+			DRM_ERROR("HW contexts were broken after resume\n");
+			i915_gem_context_fini(dev);
+			dev_priv->hw_contexts_disabled = true;
+		}
 		return;
+	}
 
 	ctx_size = get_context_size(dev);
 	dev_priv->hw_context_size = get_context_size(dev);
-- 
1.7.11.4

Re: [PATCH 3/5] drm/i915/contexts: Switch to default on resume

[Chris Wilson]

On Tue, 14 Aug 2012 14:35:15 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> In order to make the HW state CCID match with what we think it should
> be, we must order a switch to the default context.
> 
> The really sad thing here is that the switch can potentially fail, and
> as such we have to assume contexts no longer work. There is likely room
> for improvement but until we actually start seeing the case occur, I
> think it should be fine.
> 
> This was accidentally left this out of the first series, noticed by
> Chris Wilson.

However that do_switch() is a no-op as it spots that the
ring->last_ctx_obj is the DEFAULT_CONTEXT (saved across s&r) and we exit
early.

The only clean way I could see was to extract the actual set_switch
portion of do_switch() (i.e. discard the initial checks and pre-/post-op
pinning). Which brings you nicely into the next few patches...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/5] drm/i915/contexts: Switch to default on resume

[Ben Widawsky]

On 2012-08-15 02:18, Chris Wilson wrote:
> On Tue, 14 Aug 2012 14:35:15 -0700, Ben Widawsky <ben@bwidawsk.net> 
> wrote:
>> In order to make the HW state CCID match with what we think it 
>> should
>> be, we must order a switch to the default context.
>>
>> The really sad thing here is that the switch can potentially fail, 
>> and
>> as such we have to assume contexts no longer work. There is likely 
>> room
>> for improvement but until we actually start seeing the case occur, I
>> think it should be fine.
>>
>> This was accidentally left this out of the first series, noticed by
>> Chris Wilson.
>
> However that do_switch() is a no-op as it spots that the
> ring->last_ctx_obj is the DEFAULT_CONTEXT (saved across s&r) and we 
> exit
> early.
>
> The only clean way I could see was to extract the actual set_switch
> portion of do_switch() (i.e. discard the initial checks and 
> pre-/post-op
> pinning). Which brings you nicely into the next few patches...
> -Chris

Agreed, I was thinking re-ordering the series would just magically 
solve all such issues :-)

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 4/5] 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. The reason it's hand-wavy is because
the context we load is still somewhat random. Certain things may be
initialized, and certain other things may not. However it should
definitely be better than he random state that HW comes up in (and
better than all 0's).

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.

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 | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 3e884dc..3db84e2 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;
 
@@ -256,7 +256,7 @@ void i915_gem_context_init(struct drm_device *dev)
 	 * context so our HW CCID matches what we think it should be in software.
 	 */
 	if (dev_priv->ring[RCS].default_context) {
-		int ret = do_switch(dev_priv->ring[RCS].default_context);
+		int ret = do_switch(dev_priv->ring[RCS].default_context, false);
 		if (ret) {
 			DRM_ERROR("HW contexts were broken after resume\n");
 			i915_gem_context_fini(dev);
@@ -373,7 +373,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;
@@ -382,7 +382,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);
@@ -403,10 +403,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) {
@@ -482,7 +482,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 5/5] drm/i915/contexts: Serialize default context init

[Ben Widawsky]

This is possible with the new force parameter 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 3db84e2..eed00da 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