Bug 76671

> 
> The latest known good commit:4726e0b045b80c514377da35ca01467ef6a4de53
> The latest known bad commit: 57e00b2e1b348b3855f4a3b681d583f8c9cf39fb
> 

Retest commit 4726e0b045b80c514377da35ca01467ef6a4de53, it works well.
The latest known good commit: b2040f6fed736ccd2319768bc59833abe74148b8

Can you please bisect where this regression has been introduced?

I know that running the full piglit takes time, but its not a lot of commits in the good..bad range, at least the first one. Also: Why is the good..bad range much bigger in comment #1?

I'd say it was the request->ctx dereference. But that would suggest that ring->last_context was invalid?

I think:

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 021e21223c9c..91000ff1b626 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -506,9 +506,6 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
 	struct intel_ring_buffer *ring;
 	int ret, i;
 
-	if (!HAS_HW_CONTEXTS(dev_priv->dev))
-		return 0;
-
 	/* This is the only place the aliasing PPGTT gets enabled, which means
 	 * it has to happen before we bail on reset */
 	if (dev_priv->mm.aliasing_ppgtt) {
@@ -833,7 +830,13 @@ int i915_switch_context(struct intel_ring_buffer *ring,
 
 	/* We have the fake context */
 	if (!HAS_HW_CONTEXTS(ring->dev)) {
-		ring->last_context = to;
+		if (to != ring->last_context) {
+			if (to)
+				i915_gem_context_reference(to);
+			if (ring->last_context)
+				i915_gem_context_unreference(ring->last_context);
+			ring->last_context = to;
+		}
 		return 0;
 	}

(In reply to comment #4)
> I think:
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 021e21223c9c..91000ff1b626 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -506,9 +506,6 @@ int i915_gem_context_enable(struct drm_i915_private
> *dev_priv)
>  	struct intel_ring_buffer *ring;
>  	int ret, i;
>  
> -	if (!HAS_HW_CONTEXTS(dev_priv->dev))
> -		return 0;
> -
>  	/* This is the only place the aliasing PPGTT gets enabled, which means
>  	 * it has to happen before we bail on reset */
>  	if (dev_priv->mm.aliasing_ppgtt) {
> @@ -833,7 +830,13 @@ int i915_switch_context(struct intel_ring_buffer *ring,
>  
>  	/* We have the fake context */
>  	if (!HAS_HW_CONTEXTS(ring->dev)) {
> -		ring->last_context = to;
> +		if (to != ring->last_context) {
> +			if (to)
> +				i915_gem_context_reference(to);
> +			if (ring->last_context)
> +				i915_gem_context_unreference(ring->last_context);
> +			ring->last_context = to;
> +		}
>  		return 0;
>  	}


Test this patch, system boot fail.

Created attachment 96509 [details]
dmesg(patch)

Created attachment 96526 [details] [review]
Make the fake context less fake

Updated patch.

Created attachment 96636 [details]
boot log

(In reply to comment #7)
> Created attachment 96526 [details] [review] [review]
> Make the fake context less fake
> 
> Updated patch.

Apply this patch, system boot fails.

Created attachment 96685 [details] [review]
Make the fake context less fake

Ok, this time it actually booted here on a pnv. So let's see if it helps.

(In reply to comment #9)
> Created attachment 96685 [details] [review] [review]
> Make the fake context less fake
> 
> Ok, this time it actually booted here on a pnv. So let's see if it helps.

On latest -nightly kernel:
patching file drivers/gpu/drm/i915/i915_drv.h
Hunk #1 succeeded at 2277 (offset -49 lines).
patching file drivers/gpu/drm/i915/i915_gem.c
Hunk #1 succeeded at 2790 (offset -193 lines).
patching file drivers/gpu/drm/i915/i915_gem_context.c
Hunk #2 FAILED at 222.
Hunk #3 FAILED at 233.
Hunk #4 succeeded at 291 (offset -20 lines).
Hunk #5 succeeded at 339 (offset -20 lines).
Hunk #6 succeeded at 349 (offset -20 lines).
Hunk #7 succeeded at 372 (offset -20 lines).
Hunk #8 succeeded at 389 (offset -20 lines).
Hunk #9 succeeded at 427 (offset -20 lines).
Hunk #10 succeeded at 440 (offset -20 lines).
Hunk #11 succeeded at 450 (offset -20 lines).
Hunk #12 succeeded at 463 (offset -20 lines).
Hunk #13 succeeded at 470 (offset -20 lines).
Hunk #14 succeeded at 484 (offset -20 lines).
Hunk #15 FAILED at 527.
Hunk #16 FAILED at 561.
Hunk #17 succeeded at 555 (offset -19 lines).
Hunk #18 FAILED at 665.
Hunk #19 succeeded at 736 (offset -55 lines).
Hunk #20 succeeded at 744 (offset -55 lines).
Hunk #21 succeeded at 772 (offset -55 lines).
5 out of 21 hunks FAILED -- saving rejects to file drivers/gpu/drm/i915/i915_gem_context.c.rej
patching file drivers/gpu/drm/i915/i915_gem_execbuffer.c
Hunk #1 succeeded at 1221 with fuzz 2 (offset 13 lines).

*** Bug 76929 has been marked as a duplicate of this bug. ***

Created attachment 96762 [details] [review]
Always use kref tracking for all contexts

Now rebased to include an earlier bug fix as well.

(In reply to comment #12)
> Created attachment 96762 [details] [review] [review]
> Always use kref tracking for all contexts
> 
> Now rebased to include an earlier bug fix as well.


Apply this patch and run one cycle on ILK, it works well.

commit 349a1b45a80e5f1dcd8a94516c71118350f439e5
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Apr 9 09:07:36 2014 +0100

    drm/i915: Always use kref tracking for all contexts.
    
    If we always initialize kref for the context, even if we are using fake
    contexts for hangstats when there is no hw support, we can forgo the
    dance to dereference the ctx->obj and inspect whether we are permitted
    to use kref inside i915_gem_context_reference() and _unreference().
    
    My ulterior motive here is to improve the debugging of a use-after-free
    of ctx->obj. This patch avoids the dereference here and instead forces
    the assertion checks associated with kref.
    
    v2: Refactor the fake contexts to being even more like the real
    contexts, so that there is much less duplicated and special case code.
    
    v3: Tweaks.
    v4: Tweaks, minor.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76671
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Tested-by: lu hua <huax.lu@intel.com>
    Cc: Ben Widawsky <benjamin.widawsky@intel.com>
    Cc: Mika Kuoppala <mika.kuoppala@intel.com>
    Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
    [Jani: tiny change to backport to drm-intel-fixes.]
    Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Verified.Fixed.

Closing verified+fixed.