[PATCH 1/2] drm/i915: Pull wait-for-idle into i915_gem_switch_to_kernel_context()

[PATCH 1/2] drm/i915: Pull wait-for-idle into i915_gem_switch_to_kernel_context()

[Chris Wilson]

All callers do want a synchronous switch to the kernel context, that is
by the time the call returns, the GPU has evicted all user contexts and
now has the kernel context pinned. As all callers want this behaviour,
refactor the common wait-for-idle into the switch.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |  6 ------
 drivers/gpu/drm/i915/i915_gem_context.c |  4 +++-
 drivers/gpu/drm/i915/i915_gem_evict.c   | 14 +-------------
 3 files changed, 4 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 890fe2802973..18ba74be286c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4564,12 +4564,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_unlock;
 
-	ret = i915_gem_wait_for_idle(dev_priv,
-				     I915_WAIT_INTERRUPTIBLE |
-				     I915_WAIT_LOCKED);
-	if (ret)
-		goto err_unlock;
-
 	assert_kernel_context_is_current(dev_priv);
 	i915_gem_contexts_lost(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 58a2a44f88bd..f70b05e682ac 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -924,7 +924,9 @@ int i915_gem_switch_to_kernel_context(struct drm_i915_private *dev_priv)
 			return ret;
 	}
 
-	return 0;
+	return i915_gem_wait_for_idle(dev_priv,
+				     I915_WAIT_INTERRUPTIBLE |
+				     I915_WAIT_LOCKED);
 }
 
 static bool client_is_banned(struct drm_i915_file_private *file_priv)
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index 4df039ef2ce3..5cf73ad4801a 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -52,25 +52,13 @@ static bool ggtt_is_idle(struct drm_i915_private *dev_priv)
 
 static int ggtt_flush(struct drm_i915_private *i915)
 {
-	int err;
-
 	/* Not everything in the GGTT is tracked via vma (otherwise we
 	 * could evict as required with minimal stalling) so we are forced
 	 * to idle the GPU and explicitly retire outstanding requests in
 	 * the hopes that we can then remove contexts and the like only
 	 * bound by their active reference.
 	 */
-	err = i915_gem_switch_to_kernel_context(i915);
-	if (err)
-		return err;
-
-	err = i915_gem_wait_for_idle(i915,
-				     I915_WAIT_INTERRUPTIBLE |
-				     I915_WAIT_LOCKED);
-	if (err)
-		return err;
-
-	return 0;
+	return i915_gem_switch_to_kernel_context(i915);
 }
 
 static bool
-- 
2.14.1

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

[PATCH 2/2] drm/i915/perf: Remove open-coding of i915_gem_switch_to_kernel_context()

[Chris Wilson]

The kernel context does not need to be updated for the oa config, since
it is *never* used for anything but idling the device; it should never
be required to emit OA samples. As such we can forgo tweaking the
context image, and just do a plain switch to enforce the GPU barrier so
that we can then update all other context images.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com> \o/
---
 drivers/gpu/drm/i915/i915_perf.c | 113 +--------------------------------------
 1 file changed, 1 insertion(+), 112 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 94185d610673..b44199726897 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1623,112 +1623,6 @@ static void gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
 	}
 }
 
-/*
- * Same as gen8_update_reg_state_unlocked only through the batchbuffer. This
- * is only used by the kernel context.
- */
-static int gen8_emit_oa_config(struct drm_i915_gem_request *req,
-			       const struct i915_oa_config *oa_config)
-{
-	struct drm_i915_private *dev_priv = req->i915;
-	/* The MMIO offsets for Flex EU registers aren't contiguous */
-	u32 flex_mmio[] = {
-		i915_mmio_reg_offset(EU_PERF_CNTL0),
-		i915_mmio_reg_offset(EU_PERF_CNTL1),
-		i915_mmio_reg_offset(EU_PERF_CNTL2),
-		i915_mmio_reg_offset(EU_PERF_CNTL3),
-		i915_mmio_reg_offset(EU_PERF_CNTL4),
-		i915_mmio_reg_offset(EU_PERF_CNTL5),
-		i915_mmio_reg_offset(EU_PERF_CNTL6),
-	};
-	u32 *cs;
-	int i;
-
-	cs = intel_ring_begin(req, ARRAY_SIZE(flex_mmio) * 2 + 4);
-	if (IS_ERR(cs))
-		return PTR_ERR(cs);
-
-	*cs++ = MI_LOAD_REGISTER_IMM(ARRAY_SIZE(flex_mmio) + 1);
-
-	*cs++ = i915_mmio_reg_offset(GEN8_OACTXCONTROL);
-	*cs++ = (dev_priv->perf.oa.period_exponent << GEN8_OA_TIMER_PERIOD_SHIFT) |
-		(dev_priv->perf.oa.periodic ? GEN8_OA_TIMER_ENABLE : 0) |
-		GEN8_OA_COUNTER_RESUME;
-
-	for (i = 0; i < ARRAY_SIZE(flex_mmio); i++) {
-		u32 mmio = flex_mmio[i];
-
-		/*
-		 * This arbitrary default will select the 'EU FPU0 Pipeline
-		 * Active' event. In the future it's anticipated that there
-		 * will be an explicit 'No Event' we can select, but not
-		 * yet...
-		 */
-		u32 value = 0;
-
-		if (oa_config) {
-			u32 j;
-
-			for (j = 0; j < oa_config->flex_regs_len; j++) {
-				if (i915_mmio_reg_offset(oa_config->flex_regs[j].addr) == mmio) {
-					value = oa_config->flex_regs[j].value;
-					break;
-				}
-			}
-		}
-
-		*cs++ = mmio;
-		*cs++ = value;
-	}
-
-	*cs++ = MI_NOOP;
-	intel_ring_advance(req, cs);
-
-	return 0;
-}
-
-static int gen8_switch_to_updated_kernel_context(struct drm_i915_private *dev_priv,
-						 const struct i915_oa_config *oa_config)
-{
-	struct intel_engine_cs *engine = dev_priv->engine[RCS];
-	struct i915_gem_timeline *timeline;
-	struct drm_i915_gem_request *req;
-	int ret;
-
-	lockdep_assert_held(&dev_priv->drm.struct_mutex);
-
-	i915_gem_retire_requests(dev_priv);
-
-	req = i915_gem_request_alloc(engine, dev_priv->kernel_context);
-	if (IS_ERR(req))
-		return PTR_ERR(req);
-
-	ret = gen8_emit_oa_config(req, oa_config);
-	if (ret) {
-		i915_add_request(req);
-		return ret;
-	}
-
-	/* Queue this switch after all other activity */
-	list_for_each_entry(timeline, &dev_priv->gt.timelines, link) {
-		struct drm_i915_gem_request *prev;
-		struct intel_timeline *tl;
-
-		tl = &timeline->engine[engine->id];
-		prev = i915_gem_active_raw(&tl->last_request,
-					   &dev_priv->drm.struct_mutex);
-		if (prev)
-			i915_sw_fence_await_sw_fence_gfp(&req->submit,
-							 &prev->submit,
-							 GFP_KERNEL);
-	}
-
-	ret = i915_switch_context(req);
-	i915_add_request(req);
-
-	return ret;
-}
-
 /*
  * Manages updating the per-context aspects of the OA stream
  * configuration across all contexts.
@@ -1771,11 +1665,6 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
 		mutex_lock(&dev_priv->drm.struct_mutex);
 	}
 
-	/* Switch away from any user context. */
-	ret = gen8_switch_to_updated_kernel_context(dev_priv, oa_config);
-	if (ret)
-		goto out;
-
 	/*
 	 * The OA register config is setup through the context image. This image
 	 * might be written to by the GPU on context switch (in particular on
@@ -1789,7 +1678,7 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
 	 * So far the best way to work around this issue seems to be draining
 	 * the GPU from any submitted work.
 	 */
-	ret = i915_gem_wait_for_idle(dev_priv, wait_flags);
+	ret = i915_gem_switch_to_kernel_context(dev_priv);
 	if (ret)
 		goto out;
 
-- 
2.14.1

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Pull wait-for-idle into i915_gem_switch_to_kernel_context()

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Pull wait-for-idle into i915_gem_switch_to_kernel_context()
URL   : https://patchwork.freedesktop.org/series/29478/
State : success

== Summary ==

Series 29478v1 series starting with [1/2] drm/i915: Pull wait-for-idle into i915_gem_switch_to_kernel_context()
https://patchwork.freedesktop.org/api/1.0/series/29478/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                fail       -> PASS       (fi-snb-2600) fdo#100215

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:455s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:439s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:358s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:551s
fi-bwr-2160      total:279  pass:184  dwarn:0   dfail:0   fail:0   skip:95  time:253s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:524s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:522s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:518s
fi-elk-e7500     total:279  pass:230  dwarn:0   dfail:0   fail:0   skip:49  time:430s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:610s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:448s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:424s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:421s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:497s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:474s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:475s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:598s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:599s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:522s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:481s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:489s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:441s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:478s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:547s
fi-snb-2600      total:279  pass:249  dwarn:0   dfail:0   fail:1   skip:29  time:414s

627598734e5ed449b1173ff8158126c57c361a40 drm-tip: 2017y-08m-29d-09h-52m-12s UTC integration manifest
a4f2c16aa596 drm/i915/perf: Remove open-coding of i915_gem_switch_to_kernel_context()
0046c657b34c drm/i915: Pull wait-for-idle into i915_gem_switch_to_kernel_context()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5520/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [1/2] drm/i915: Pull wait-for-idle into i915_gem_switch_to_kernel_context()

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Pull wait-for-idle into i915_gem_switch_to_kernel_context()
URL   : https://patchwork.freedesktop.org/series/29478/
State : failure

== Summary ==

Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-B-frame-sequence:
                pass       -> FAIL       (shard-hsw)
Test kms_plane:
        Subgroup plane-panning-bottom-right-suspend-pipe-C-planes:
                skip       -> PASS       (shard-hsw)
        Subgroup plane-position-hole-dpms-pipe-C-planes:
                skip       -> PASS       (shard-hsw)
Test kms_atomic_transition:
        Subgroup plane-all-transition-fencing:
                skip       -> PASS       (shard-hsw)
Test kms_properties:
        Subgroup plane-properties-legacy:
                skip       -> PASS       (shard-hsw)

shard-hsw        total:2230 pass:1229 dwarn:0   dfail:0   fail:19  skip:982 time:9626s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5520/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Pull wait-for-idle into i915_gem_switch_to_kernel_context()

[Mika Kuoppala]

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

> All callers do want a synchronous switch to the kernel context, that is
> by the time the call returns, the GPU has evicted all user contexts and
> now has the kernel context pinned. As all callers want this behaviour,
> refactor the common wait-for-idle into the switch.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         |  6 ------
>  drivers/gpu/drm/i915/i915_gem_context.c |  4 +++-
>  drivers/gpu/drm/i915/i915_gem_evict.c   | 14 +-------------
>  3 files changed, 4 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 890fe2802973..18ba74be286c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4564,12 +4564,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>  	if (ret)
>  		goto err_unlock;
>  
> -	ret = i915_gem_wait_for_idle(dev_priv,
> -				     I915_WAIT_INTERRUPTIBLE |
> -				     I915_WAIT_LOCKED);
> -	if (ret)
> -		goto err_unlock;
> -
>  	assert_kernel_context_is_current(dev_priv);
>  	i915_gem_contexts_lost(dev_priv);
>  	mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 58a2a44f88bd..f70b05e682ac 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -924,7 +924,9 @@ int i915_gem_switch_to_kernel_context(struct drm_i915_private *dev_priv)
>  			return ret;
>  	}
>  
> -	return 0;
> +	return i915_gem_wait_for_idle(dev_priv,
> +				     I915_WAIT_INTERRUPTIBLE |
> +				     I915_WAIT_LOCKED);

This wont apply due to special case EIO handling the previous
suspend hardening patches introduced. Please explain why the EIO
needs to passthrough and not return.

-Mika

>  }
>  
>  static bool client_is_banned(struct drm_i915_file_private *file_priv)
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index 4df039ef2ce3..5cf73ad4801a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -52,25 +52,13 @@ static bool ggtt_is_idle(struct drm_i915_private *dev_priv)
>  
>  static int ggtt_flush(struct drm_i915_private *i915)
>  {
> -	int err;
> -
>  	/* Not everything in the GGTT is tracked via vma (otherwise we
>  	 * could evict as required with minimal stalling) so we are forced
>  	 * to idle the GPU and explicitly retire outstanding requests in
>  	 * the hopes that we can then remove contexts and the like only
>  	 * bound by their active reference.
>  	 */
> -	err = i915_gem_switch_to_kernel_context(i915);
> -	if (err)
> -		return err;
> -
> -	err = i915_gem_wait_for_idle(i915,
> -				     I915_WAIT_INTERRUPTIBLE |
> -				     I915_WAIT_LOCKED);
> -	if (err)
> -		return err;
> -
> -	return 0;
> +	return i915_gem_switch_to_kernel_context(i915);
>  }
>  
>  static bool
> -- 
> 2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Pull wait-for-idle into i915_gem_switch_to_kernel_context()

[Chris Wilson]

Quoting Mika Kuoppala (2017-08-30 12:16:08)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > All callers do want a synchronous switch to the kernel context, that is
> > by the time the call returns, the GPU has evicted all user contexts and
> > now has the kernel context pinned. As all callers want this behaviour,
> > refactor the common wait-for-idle into the switch.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c         |  6 ------
> >  drivers/gpu/drm/i915/i915_gem_context.c |  4 +++-
> >  drivers/gpu/drm/i915/i915_gem_evict.c   | 14 +-------------
> >  3 files changed, 4 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 890fe2802973..18ba74be286c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4564,12 +4564,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
> >       if (ret)
> >               goto err_unlock;
> >  
> > -     ret = i915_gem_wait_for_idle(dev_priv,
> > -                                  I915_WAIT_INTERRUPTIBLE |
> > -                                  I915_WAIT_LOCKED);
> > -     if (ret)
> > -             goto err_unlock;
> > -
> >       assert_kernel_context_is_current(dev_priv);
> >       i915_gem_contexts_lost(dev_priv);
> >       mutex_unlock(&dev->struct_mutex);
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 58a2a44f88bd..f70b05e682ac 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -924,7 +924,9 @@ int i915_gem_switch_to_kernel_context(struct drm_i915_private *dev_priv)
> >                       return ret;
> >       }
> >  
> > -     return 0;
> > +     return i915_gem_wait_for_idle(dev_priv,
> > +                                  I915_WAIT_INTERRUPTIBLE |
> > +                                  I915_WAIT_LOCKED);
> 
> This wont apply due to special case EIO handling the previous
> suspend hardening patches introduced. Please explain why the EIO
> needs to passthrough and not return.

Hmm, hopefully my earlier reply turns up...

But as I was writing up an assert for add_request, I realise that we
know allow set-wedged to be unlocked and so we do not have the strict
control over requests-vs-wedging anymore. The idea still remains, if we
set-wedged as we build the request, we do want to cancel it and report
the -EIO.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx