intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] drm/i915/execlists: Delay updating ring register state after resume
@ 2018-09-14  9:42 Chris Wilson
  2018-09-14  9:42 ` [PATCH 2/4] drm/i915: Park the GPU on module load Chris Wilson
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Chris Wilson @ 2018-09-14  9:42 UTC (permalink / raw)
  To: intel-gfx

Now that we reload both RING_HEAD and RING_TAIL when rebinding the
context, we do not need to scrub those registers immediately on resume.

v2: Handle the perma-pinned contexts.
v3: Set RING_TAIL on context-pin so that we always have known state in
the context image for the ring registers and all parties have similar
code (ripe for refactoring).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 33 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 9b1f0e5211a0..d7fcbba8e982 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1338,11 +1338,13 @@ __execlists_context_pin(struct intel_engine_cs *engine,
 
 	intel_lr_context_descriptor_update(ctx, engine, ce);
 
+	GEM_BUG_ON(!intel_ring_offset_valid(ce->ring, ce->ring->head));
+
 	ce->lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
 	ce->lrc_reg_state[CTX_RING_BUFFER_START+1] =
 		i915_ggtt_offset(ce->ring->vma);
-	GEM_BUG_ON(!intel_ring_offset_valid(ce->ring, ce->ring->head));
-	ce->lrc_reg_state[CTX_RING_HEAD+1] = ce->ring->head;
+	ce->lrc_reg_state[CTX_RING_HEAD + 1] = ce->ring->head;
+	ce->lrc_reg_state[CTX_RING_TAIL + 1] = ce->ring->tail;
 
 	ce->state->obj->pin_global++;
 	i915_gem_context_get(ctx);
@@ -2841,13 +2843,14 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
 	return ret;
 }
 
-void intel_lr_context_resume(struct drm_i915_private *dev_priv)
+void intel_lr_context_resume(struct drm_i915_private *i915)
 {
 	struct intel_engine_cs *engine;
 	struct i915_gem_context *ctx;
 	enum intel_engine_id id;
 
-	/* Because we emit WA_TAIL_DWORDS there may be a disparity
+	/*
+	 * Because we emit WA_TAIL_DWORDS there may be a disparity
 	 * between our bookkeeping in ce->ring->head and ce->ring->tail and
 	 * that stored in context. As we only write new commands from
 	 * ce->ring->tail onwards, everything before that is junk. If the GPU
@@ -2857,28 +2860,22 @@ void intel_lr_context_resume(struct drm_i915_private *dev_priv)
 	 * So to avoid that we reset the context images upon resume. For
 	 * simplicity, we just zero everything out.
 	 */
-	list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
-		for_each_engine(engine, dev_priv, id) {
+	list_for_each_entry(ctx, &i915->contexts.list, link) {
+		for_each_engine(engine, i915, id) {
 			struct intel_context *ce =
 				to_intel_context(ctx, engine);
-			u32 *reg;
 
 			if (!ce->state)
 				continue;
 
-			reg = i915_gem_object_pin_map(ce->state->obj,
-						      I915_MAP_WB);
-			if (WARN_ON(IS_ERR(reg)))
-				continue;
-
-			reg += LRC_STATE_PN * PAGE_SIZE / sizeof(*reg);
-			reg[CTX_RING_HEAD+1] = 0;
-			reg[CTX_RING_TAIL+1] = 0;
+			intel_ring_reset(ce->ring, 0);
 
-			ce->state->obj->mm.dirty = true;
-			i915_gem_object_unpin_map(ce->state->obj);
+			if (ce->pin_count) { /* otherwise done in context_pin */
+				u32 *regs = ce->lrc_reg_state;
 
-			intel_ring_reset(ce->ring, 0);
+				regs[CTX_RING_HEAD + 1] = ce->ring->head;
+				regs[CTX_RING_TAIL + 1] = ce->ring->tail;
+			}
 		}
 	}
 }
-- 
2.19.0

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/4] drm/i915: Park the GPU on module load
  2018-09-14  9:42 [PATCH 1/4] drm/i915/execlists: Delay updating ring register state after resume Chris Wilson
@ 2018-09-14  9:42 ` Chris Wilson
  2018-09-14  9:42 ` [PATCH 3/4] drm/i915: Check engine->default_state mapping " Chris Wilson
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2018-09-14  9:42 UTC (permalink / raw)
  To: intel-gfx

Once we have flushed the first request through the system to both load a
context and record the default state; tell the GPU to park and idle
itself, putting itself immediately (hopefully at least) into a
powersaving state, and allowing ourselves to start from known state
after setting up all our bookkeeping.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 89834ce19acd..be9d012d851b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5414,6 +5414,14 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 
 	assert_kernel_context_is_current(i915);
 
+	/*
+	 * Immediately park the GPU so that we enable powersaving and
+	 * treat it as idle. The next time we issue a request, we will
+	 * unpark and start using the engine->pinned_default_state, otherwise
+	 * it is in limbo and an early reset may fail.
+	 */
+	__i915_gem_park(i915);
+
 	for_each_engine(engine, i915, id) {
 		struct i915_vma *state;
 
-- 
2.19.0

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/4] drm/i915: Check engine->default_state mapping on module load
  2018-09-14  9:42 [PATCH 1/4] drm/i915/execlists: Delay updating ring register state after resume Chris Wilson
  2018-09-14  9:42 ` [PATCH 2/4] drm/i915: Park the GPU on module load Chris Wilson
@ 2018-09-14  9:42 ` Chris Wilson
  2018-09-14 11:02   ` Tvrtko Ursulin
  2018-09-14  9:42 ` [PATCH 4/4] drm/i915/execlists: Use coherent writes into the context image Chris Wilson
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2018-09-14  9:42 UTC (permalink / raw)
  To: intel-gfx

Check we can indeed acquire a WB mapping of the context image on module
load. Later this will give us the opportunity to validate that we can
switch from WC to WB as required.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index be9d012d851b..37353afec66e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5424,6 +5424,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 
 	for_each_engine(engine, i915, id) {
 		struct i915_vma *state;
+		void *vaddr;
 
 		state = to_intel_context(ctx, engine)->state;
 		if (!state)
@@ -5446,6 +5447,16 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 			goto err_active;
 
 		engine->default_state = i915_gem_object_get(state->obj);
+
+		/* Check we can acquire the image of the context state */
+		vaddr = i915_gem_object_pin_map(engine->default_state,
+						I915_MAP_WB);
+		if (IS_ERR(vaddr)) {
+			err = PTR_ERR(vaddr);
+			goto err_active;
+		}
+
+		i915_gem_object_unpin_map(engine->default_state);
 	}
 
 	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) {
-- 
2.19.0

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/4] drm/i915/execlists: Use coherent writes into the context image
  2018-09-14  9:42 [PATCH 1/4] drm/i915/execlists: Delay updating ring register state after resume Chris Wilson
  2018-09-14  9:42 ` [PATCH 2/4] drm/i915: Park the GPU on module load Chris Wilson
  2018-09-14  9:42 ` [PATCH 3/4] drm/i915: Check engine->default_state mapping " Chris Wilson
@ 2018-09-14  9:42 ` Chris Wilson
  2018-09-14 13:03   ` Ville Syrjälä
  2018-09-14 12:29 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/execlists: Delay updating ring register state after resume Patchwork
  2018-09-14 12:47 ` ✗ Fi.CI.BAT: failure " Patchwork
  4 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2018-09-14  9:42 UTC (permalink / raw)
  To: intel-gfx

That we use a WB mapping for updating the RING_TAIL register inside the
context image even on !llc machines has been a source of consternation
for every reader. It appears to work on bsw+, but it may just have been
that we have been incredibly bad at detecting the errors.

v2: With extra enthusiasm.
v3: Drop force of map type for pinned default_state as by the time we
pin it, the map type is always WB and doesn't conflict with the earlier
use by ce->state.
v4: Transfer engine->default_state from MAP_WC to MAP_WB on creation so
we do not need the MAP_FORCE littered around the backends

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  | 6 ++++++
 drivers/gpu/drm/i915/i915_gem.c  | 4 +++-
 drivers/gpu/drm/i915/i915_perf.c | 3 ++-
 drivers/gpu/drm/i915/intel_lrc.c | 6 ++++--
 4 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bb43e56df197..7d4daa7412f1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3097,6 +3097,12 @@ enum i915_map_type {
 	I915_MAP_FORCE_WC = I915_MAP_WC | I915_MAP_OVERRIDE,
 };
 
+static inline enum i915_map_type
+i915_coherent_map_type(struct drm_i915_private *i915)
+{
+	return HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC;
+}
+
 /**
  * i915_gem_object_pin_map - return a contiguous mapping of the entire object
  * @obj: the object to map into kernel address space
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 37353afec66e..d9465bd1a00a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5426,6 +5426,8 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 		struct i915_vma *state;
 		void *vaddr;
 
+		GEM_BUG_ON(to_intel_context(ctx, engine)->pin_count);
+
 		state = to_intel_context(ctx, engine)->state;
 		if (!state)
 			continue;
@@ -5450,7 +5452,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 
 		/* Check we can acquire the image of the context state */
 		vaddr = i915_gem_object_pin_map(engine->default_state,
-						I915_MAP_WB);
+						I915_MAP_FORCE_WB);
 		if (IS_ERR(vaddr)) {
 			err = PTR_ERR(vaddr);
 			goto err_active;
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 3d7a052b4cca..664b96bb65a3 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1707,6 +1707,7 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
 				       const struct i915_oa_config *oa_config)
 {
 	struct intel_engine_cs *engine = dev_priv->engine[RCS];
+	unsigned int map_type = i915_coherent_map_type(dev_priv);
 	struct i915_gem_context *ctx;
 	struct i915_request *rq;
 	int ret;
@@ -1741,7 +1742,7 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
 		if (!ce->state)
 			continue;
 
-		regs = i915_gem_object_pin_map(ce->state->obj, I915_MAP_WB);
+		regs = i915_gem_object_pin_map(ce->state->obj, map_type);
 		if (IS_ERR(regs))
 			return PTR_ERR(regs);
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d7fcbba8e982..a51be16ddaac 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1294,7 +1294,7 @@ static int __context_pin(struct i915_gem_context *ctx, struct i915_vma *vma)
 	 * on an active context (which by nature is already on the GPU).
 	 */
 	if (!(vma->flags & I915_VMA_GLOBAL_BIND)) {
-		err = i915_gem_object_set_to_gtt_domain(vma->obj, true);
+		err = i915_gem_object_set_to_wc_domain(vma->obj, true);
 		if (err)
 			return err;
 	}
@@ -1322,7 +1322,9 @@ __execlists_context_pin(struct intel_engine_cs *engine,
 	if (ret)
 		goto err;
 
-	vaddr = i915_gem_object_pin_map(ce->state->obj, I915_MAP_WB);
+	vaddr = i915_gem_object_pin_map(ce->state->obj,
+					i915_coherent_map_type(ctx->i915) |
+					I915_MAP_OVERRIDE);
 	if (IS_ERR(vaddr)) {
 		ret = PTR_ERR(vaddr);
 		goto unpin_vma;
-- 
2.19.0

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/4] drm/i915: Check engine->default_state mapping on module load
  2018-09-14  9:42 ` [PATCH 3/4] drm/i915: Check engine->default_state mapping " Chris Wilson
@ 2018-09-14 11:02   ` Tvrtko Ursulin
  0 siblings, 0 replies; 10+ messages in thread
From: Tvrtko Ursulin @ 2018-09-14 11:02 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 14/09/2018 10:42, Chris Wilson wrote:
> Check we can indeed acquire a WB mapping of the context image on module
> load. Later this will give us the opportunity to validate that we can
> switch from WC to WB as required.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index be9d012d851b..37353afec66e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5424,6 +5424,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   
>   	for_each_engine(engine, i915, id) {
>   		struct i915_vma *state;
> +		void *vaddr;
>   
>   		state = to_intel_context(ctx, engine)->state;
>   		if (!state)
> @@ -5446,6 +5447,16 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   			goto err_active;
>   
>   		engine->default_state = i915_gem_object_get(state->obj);
> +
> +		/* Check we can acquire the image of the context state */
> +		vaddr = i915_gem_object_pin_map(engine->default_state,
> +						I915_MAP_WB);
> +		if (IS_ERR(vaddr)) {
> +			err = PTR_ERR(vaddr);
> +			goto err_active;
> +		}
> +
> +		i915_gem_object_unpin_map(engine->default_state);
>   	}
>   
>   	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) {
> 

I'm lost in the threads, caught in a trap.. :)

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* ✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/execlists: Delay updating ring register state after resume
  2018-09-14  9:42 [PATCH 1/4] drm/i915/execlists: Delay updating ring register state after resume Chris Wilson
                   ` (2 preceding siblings ...)
  2018-09-14  9:42 ` [PATCH 4/4] drm/i915/execlists: Use coherent writes into the context image Chris Wilson
@ 2018-09-14 12:29 ` Patchwork
  2018-09-14 12:47 ` ✗ Fi.CI.BAT: failure " Patchwork
  4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-09-14 12:29 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/4] drm/i915/execlists: Delay updating ring register state after resume
URL   : https://patchwork.freedesktop.org/series/49697/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/execlists: Delay updating ring register state after resume
Okay!

Commit: drm/i915: Park the GPU on module load
Okay!

Commit: drm/i915: Check engine->default_state mapping on module load
Okay!

Commit: drm/i915/execlists: Use coherent writes into the context image
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3712:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3718:16: warning: expression using sizeof(void)

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* ✗ Fi.CI.BAT: failure for series starting with [1/4] drm/i915/execlists: Delay updating ring register state after resume
  2018-09-14  9:42 [PATCH 1/4] drm/i915/execlists: Delay updating ring register state after resume Chris Wilson
                   ` (3 preceding siblings ...)
  2018-09-14 12:29 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/execlists: Delay updating ring register state after resume Patchwork
@ 2018-09-14 12:47 ` Patchwork
  4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-09-14 12:47 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/4] drm/i915/execlists: Delay updating ring register state after resume
URL   : https://patchwork.freedesktop.org/series/49697/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4825 -> Patchwork_10190 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_10190 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10190, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/49697/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10190:

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_selftest@live_coherency:
      fi-skl-6770hq:      PASS -> DMESG-WARN +3
      fi-snb-2600:        PASS -> DMESG-WARN +2
      fi-kbl-7500u:       PASS -> DMESG-WARN +3

    igt@drv_selftest@live_contexts:
      fi-cfl-s3:          PASS -> DMESG-WARN +4
      fi-snb-2520m:       PASS -> DMESG-WARN +1
      fi-pnv-d510:        PASS -> DMESG-WARN +1
      fi-glk-dsi:         PASS -> DMESG-WARN +4
      fi-gdg-551:         PASS -> DMESG-WARN +1
      fi-byt-clapper:     PASS -> DMESG-WARN +1
      fi-ivb-3770:        PASS -> DMESG-WARN +2

    igt@drv_selftest@live_execlists:
      fi-bdw-5557u:       PASS -> DMESG-WARN +1
      fi-skl-6700k2:      PASS -> DMESG-WARN +4
      fi-skl-6600u:       PASS -> DMESG-WARN +3

    igt@drv_selftest@live_requests:
      fi-whl-u:           PASS -> DMESG-WARN +3
      fi-skl-gvtdvm:      PASS -> DMESG-WARN +4
      fi-ilk-650:         PASS -> DMESG-WARN +2
      fi-ivb-3520m:       PASS -> DMESG-WARN
      fi-bdw-gvtdvm:      PASS -> DMESG-WARN +4
      fi-bxt-j4205:       PASS -> DMESG-WARN +4
      fi-cfl-guc:         PASS -> DMESG-WARN +4
      fi-skl-iommu:       PASS -> DMESG-WARN +3
      fi-skl-guc:         PASS -> DMESG-WARN +4
      fi-kbl-7567u:       PASS -> DMESG-WARN +2
      fi-glk-j4005:       PASS -> DMESG-WARN +3
      fi-hsw-peppy:       PASS -> DMESG-WARN +1
      fi-icl-u:           PASS -> DMESG-WARN +3
      fi-hsw-4770r:       NOTRUN -> DMESG-WARN +2
      fi-bwr-2160:        PASS -> DMESG-WARN +1
      fi-byt-n2820:       PASS -> DMESG-WARN +1
      fi-byt-j1900:       PASS -> DMESG-WARN +1
      fi-kbl-7560u:       PASS -> DMESG-WARN +1
      fi-hsw-4770:        PASS -> DMESG-WARN +2
      fi-blb-e6850:       PASS -> DMESG-WARN +2

    igt@drv_selftest@live_workarounds:
      fi-cfl-8700k:       PASS -> DMESG-WARN +3
      fi-kbl-r:           PASS -> DMESG-WARN +3
      fi-cfl-8109u:       PASS -> DMESG-WARN +4
      fi-kbl-guc:         PASS -> DMESG-WARN +3
      fi-bsw-kefka:       PASS -> DMESG-WARN +3
      fi-skl-6260u:       PASS -> DMESG-WARN +2
      fi-bsw-n3050:       PASS -> DMESG-WARN +3
      fi-bxt-dsi:         PASS -> DMESG-WARN +1
      fi-kbl-8809g:       PASS -> DMESG-WARN +3
      fi-kbl-x1275:       PASS -> DMESG-WARN +3

    
== Known issues ==

  Here are the changes found in Patchwork_10190 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload-inject:
      fi-hsw-4770r:       NOTRUN -> DMESG-WARN (fdo#107924, fdo#107425)

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         PASS -> DMESG-FAIL (fdo#107164)

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       PASS -> DMESG-WARN (fdo#102614)

    
    ==== Possible fixes ====

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     FAIL (fdo#103167) -> PASS

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:
      fi-byt-clapper:     FAIL (fdo#103191, fdo#107362) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-a:
      fi-byt-clapper:     FAIL (fdo#107362) -> PASS

    igt@kms_psr@primary_page_flip:
      fi-kbl-7560u:       FAIL (fdo#107336) -> PASS

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107425 https://bugs.freedesktop.org/show_bug.cgi?id=107425
  fdo#107924 https://bugs.freedesktop.org/show_bug.cgi?id=107924


== Participating hosts (47 -> 44) ==

  Additional (1): fi-hsw-4770r 
  Missing    (4): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4825 -> Patchwork_10190

  CI_DRM_4825: b42528aaa961c0d469f381b4a5c3830fe46aedfa @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4641: 468febc4c746f168e885e0d662ec3adb0cca60f6 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10190: 588829eb5ec63ccebdc09c9c075dcd5e786f5d5d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

588829eb5ec6 drm/i915/execlists: Use coherent writes into the context image
90d83f613d50 drm/i915: Check engine->default_state mapping on module load
95a895936424 drm/i915: Park the GPU on module load
dfcd2eaebd7b drm/i915/execlists: Delay updating ring register state after resume

== Logs ==

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] drm/i915/execlists: Use coherent writes into the context image
  2018-09-14  9:42 ` [PATCH 4/4] drm/i915/execlists: Use coherent writes into the context image Chris Wilson
@ 2018-09-14 13:03   ` Ville Syrjälä
  2018-09-14 13:16     ` Chris Wilson
  0 siblings, 1 reply; 10+ messages in thread
From: Ville Syrjälä @ 2018-09-14 13:03 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Sep 14, 2018 at 10:42:15AM +0100, Chris Wilson wrote:
> That we use a WB mapping for updating the RING_TAIL register inside the
> context image even on !llc machines has been a source of consternation
> for every reader. It appears to work on bsw+, but it may just have been
> that we have been incredibly bad at detecting the errors.

Presumably it's due to the "all ggtt accesses go through pat[0]" and
we make pat[0] snoop. So presumably the hw should snoop when loading
the context... maybe.

> 
> v2: With extra enthusiasm.
> v3: Drop force of map type for pinned default_state as by the time we
> pin it, the map type is always WB and doesn't conflict with the earlier
> use by ce->state.
> v4: Transfer engine->default_state from MAP_WC to MAP_WB on creation so
> we do not need the MAP_FORCE littered around the backends
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  | 6 ++++++
>  drivers/gpu/drm/i915/i915_gem.c  | 4 +++-
>  drivers/gpu/drm/i915/i915_perf.c | 3 ++-
>  drivers/gpu/drm/i915/intel_lrc.c | 6 ++++--
>  4 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bb43e56df197..7d4daa7412f1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3097,6 +3097,12 @@ enum i915_map_type {
>  	I915_MAP_FORCE_WC = I915_MAP_WC | I915_MAP_OVERRIDE,
>  };
>  
> +static inline enum i915_map_type
> +i915_coherent_map_type(struct drm_i915_private *i915)
> +{
> +	return HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC;
> +}
> +
>  /**
>   * i915_gem_object_pin_map - return a contiguous mapping of the entire object
>   * @obj: the object to map into kernel address space
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 37353afec66e..d9465bd1a00a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5426,6 +5426,8 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>  		struct i915_vma *state;
>  		void *vaddr;
>  
> +		GEM_BUG_ON(to_intel_context(ctx, engine)->pin_count);
> +
>  		state = to_intel_context(ctx, engine)->state;
>  		if (!state)
>  			continue;
> @@ -5450,7 +5452,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>  
>  		/* Check we can acquire the image of the context state */
>  		vaddr = i915_gem_object_pin_map(engine->default_state,
> -						I915_MAP_WB);
> +						I915_MAP_FORCE_WB);
>  		if (IS_ERR(vaddr)) {
>  			err = PTR_ERR(vaddr);
>  			goto err_active;
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 3d7a052b4cca..664b96bb65a3 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1707,6 +1707,7 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
>  				       const struct i915_oa_config *oa_config)
>  {
>  	struct intel_engine_cs *engine = dev_priv->engine[RCS];
> +	unsigned int map_type = i915_coherent_map_type(dev_priv);
>  	struct i915_gem_context *ctx;
>  	struct i915_request *rq;
>  	int ret;
> @@ -1741,7 +1742,7 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
>  		if (!ce->state)
>  			continue;
>  
> -		regs = i915_gem_object_pin_map(ce->state->obj, I915_MAP_WB);
> +		regs = i915_gem_object_pin_map(ce->state->obj, map_type);
>  		if (IS_ERR(regs))
>  			return PTR_ERR(regs);
>  
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index d7fcbba8e982..a51be16ddaac 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1294,7 +1294,7 @@ static int __context_pin(struct i915_gem_context *ctx, struct i915_vma *vma)
>  	 * on an active context (which by nature is already on the GPU).
>  	 */
>  	if (!(vma->flags & I915_VMA_GLOBAL_BIND)) {
> -		err = i915_gem_object_set_to_gtt_domain(vma->obj, true);
> +		err = i915_gem_object_set_to_wc_domain(vma->obj, true);
>  		if (err)
>  			return err;
>  	}
> @@ -1322,7 +1322,9 @@ __execlists_context_pin(struct intel_engine_cs *engine,
>  	if (ret)
>  		goto err;
>  
> -	vaddr = i915_gem_object_pin_map(ce->state->obj, I915_MAP_WB);
> +	vaddr = i915_gem_object_pin_map(ce->state->obj,
> +					i915_coherent_map_type(ctx->i915) |
> +					I915_MAP_OVERRIDE);
>  	if (IS_ERR(vaddr)) {
>  		ret = PTR_ERR(vaddr);
>  		goto unpin_vma;
> -- 
> 2.19.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] drm/i915/execlists: Use coherent writes into the context image
  2018-09-14 13:03   ` Ville Syrjälä
@ 2018-09-14 13:16     ` Chris Wilson
  2018-09-14 13:39       ` Ville Syrjälä
  0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2018-09-14 13:16 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

Quoting Ville Syrjälä (2018-09-14 14:03:35)
> On Fri, Sep 14, 2018 at 10:42:15AM +0100, Chris Wilson wrote:
> > That we use a WB mapping for updating the RING_TAIL register inside the
> > context image even on !llc machines has been a source of consternation
> > for every reader. It appears to work on bsw+, but it may just have been
> > that we have been incredibly bad at detecting the errors.
> 
> Presumably it's due to the "all ggtt accesses go through pat[0]" and
> we make pat[0] snoop. So presumably the hw should snoop when loading
> the context... maybe.

Shows how much attention I pay, I thought we made pat[0] uncached. Seems
strange to suggest that we should always be snooping when reading GGTT
from the GPU.

We still have the same PTE bits for GGTT as for ppGTT, do we not?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] drm/i915/execlists: Use coherent writes into the context image
  2018-09-14 13:16     ` Chris Wilson
@ 2018-09-14 13:39       ` Ville Syrjälä
  0 siblings, 0 replies; 10+ messages in thread
From: Ville Syrjälä @ 2018-09-14 13:39 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Sep 14, 2018 at 02:16:12PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjälä (2018-09-14 14:03:35)
> > On Fri, Sep 14, 2018 at 10:42:15AM +0100, Chris Wilson wrote:
> > > That we use a WB mapping for updating the RING_TAIL register inside the
> > > context image even on !llc machines has been a source of consternation
> > > for every reader. It appears to work on bsw+, but it may just have been
> > > that we have been incredibly bad at detecting the errors.
> > 
> > Presumably it's due to the "all ggtt accesses go through pat[0]" and
> > we make pat[0] snoop. So presumably the hw should snoop when loading
> > the context... maybe.
> 
> Shows how much attention I pay, I thought we made pat[0] uncached. Seems
> strange to suggest that we should always be snooping when reading GGTT
> from the GPU.

IIRC I did it originally that way for the status page. Sadly it's
either "all snooped" or "none snooped" due to the hw not having
wired up the bits for ggtt.

> 
> We still have the same PTE bits for GGTT as for ppGTT, do we not?

Same, except the pwt/pcd/pat bits do nothing for ggtt.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-09-14 13:40 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-14  9:42 [PATCH 1/4] drm/i915/execlists: Delay updating ring register state after resume Chris Wilson
2018-09-14  9:42 ` [PATCH 2/4] drm/i915: Park the GPU on module load Chris Wilson
2018-09-14  9:42 ` [PATCH 3/4] drm/i915: Check engine->default_state mapping " Chris Wilson
2018-09-14 11:02   ` Tvrtko Ursulin
2018-09-14  9:42 ` [PATCH 4/4] drm/i915/execlists: Use coherent writes into the context image Chris Wilson
2018-09-14 13:03   ` Ville Syrjälä
2018-09-14 13:16     ` Chris Wilson
2018-09-14 13:39       ` Ville Syrjälä
2018-09-14 12:29 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/execlists: Delay updating ring register state after resume Patchwork
2018-09-14 12:47 ` ✗ Fi.CI.BAT: failure " Patchwork

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).