[PATCH 1/7] drm/i915: Include engine->last_submitted_seqno in GPU error state

[PATCH 1/7] drm/i915: Include engine->last_submitted_seqno in GPU error state

[Chris Wilson]

It's useful to look at the last seqno submitted on a particular engine
and compare it against the HWS value to check for irregularities.

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>
---
 drivers/gpu/drm/i915/i915_debugfs.c   | 6 ++++--
 drivers/gpu/drm/i915/i915_drv.h       | 1 +
 drivers/gpu/drm/i915/i915_gpu_error.c | 2 ++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a2e3af02c292..906e5424e488 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1363,8 +1363,10 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
 
 	for_each_engine_id(engine, dev_priv, id) {
 		seq_printf(m, "%s:\n", engine->name);
-		seq_printf(m, "\tseqno = %x [current %x]\n",
-			   engine->hangcheck.seqno, seqno[id]);
+		seq_printf(m, "\tseqno = %x [current %x, last %x]\n",
+			   engine->hangcheck.seqno,
+			   seqno[id],
+			   engine->last_submitted_seqno);
 		seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
 			   (long long)engine->hangcheck.acthd,
 			   (long long)acthd[id]);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 313bc3576d87..fad8e0d5a216 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -495,6 +495,7 @@ struct drm_i915_error_state {
 		u32 cpu_ring_head;
 		u32 cpu_ring_tail;
 
+		u32 last_seqno;
 		u32 semaphore_seqno[I915_NUM_ENGINES - 1];
 
 		/* Register state */
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 9b55409d91b3..9463cb4e9323 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -296,6 +296,7 @@ static void i915_ring_error_state(struct drm_i915_error_state_buf *m,
 		}
 	}
 	err_printf(m, "  seqno: 0x%08x\n", ring->seqno);
+	err_printf(m, "  last_seqno: 0x%08x\n", ring->last_seqno);
 	err_printf(m, "  waiting: %s\n", yesno(ring->waiting));
 	err_printf(m, "  ring->head: 0x%08x\n", ring->cpu_ring_head);
 	err_printf(m, "  ring->tail: 0x%08x\n", ring->cpu_ring_tail);
@@ -931,6 +932,7 @@ static void i915_record_ring_state(struct drm_device *dev,
 	ering->instpm = I915_READ(RING_INSTPM(engine->mmio_base));
 	ering->seqno = engine->get_seqno(engine, false);
 	ering->acthd = intel_ring_get_active_head(engine);
+	ering->last_seqno = engine->last_submitted_seqno;
 	ering->start = I915_READ_START(engine);
 	ering->head = I915_READ_HEAD(engine);
 	ering->tail = I915_READ_TAIL(engine);
-- 
2.8.0.rc3

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

[PATCH 2/7] drm/i915: On GPU reset, set the HWS breadcrumb to the last seqno

[Chris Wilson]

After the GPU reset and we discard all of the incomplete requests, mark
the GPU as having advanced to the last_submitted_seqno (as having
completed the requests and ready for fresh work). The impact of this is
negligible, as all the requests will be considered completed by this
point, it just brings the HWS into line with expectations for external
viewers.

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>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 40f90c7e718a..3e9e6f9b66f5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2882,6 +2882,8 @@ static void i915_gem_reset_engine_cleanup(struct drm_i915_private *dev_priv,
 		buffer->last_retired_head = buffer->tail;
 		intel_ring_update_space(buffer);
 	}
+
+	intel_ring_init_seqno(engine, engine->last_submitted_seqno);
 }
 
 void i915_gem_reset(struct drm_device *dev)
-- 
2.8.0.rc3

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

[PATCH 3/7] drm/i915: Remove unneeded drm_device pointer from intel_ring_init_seqno()

[Chris Wilson]

We only use drm_i915_private within the function, so delete the unneeded
drm_device local.

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>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2e864b7a528b..371f4c1fc33c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2550,13 +2550,12 @@ int intel_ring_cacheline_align(struct drm_i915_gem_request *req)
 
 void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
 {
-	struct drm_device *dev = engine->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(engine->dev);
 
-	if (INTEL_INFO(dev)->gen == 6 || INTEL_INFO(dev)->gen == 7) {
+	if (INTEL_INFO(dev_priv)->gen == 6 || INTEL_INFO(dev_priv)->gen == 7) {
 		I915_WRITE(RING_SYNC_0(engine->mmio_base), 0);
 		I915_WRITE(RING_SYNC_1(engine->mmio_base), 0);
-		if (HAS_VEBOX(dev))
+		if (HAS_VEBOX(dev_priv))
 			I915_WRITE(RING_SYNC_2(engine->mmio_base), 0);
 	}
 
-- 
2.8.0.rc3

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

[PATCH 4/7] drm/i915: Move the hw semaphore initialisation from GEM to the engine

[Chris Wilson]

Since we are setting engine local values that are tied to the hardware,
move it out of i915_gem_init_seqno() into the intel_ring_init_seqno()
backend, next to where the other hw semaphore registers are written.

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>
---
 drivers/gpu/drm/i915/i915_gem.c         | 8 ++------
 drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++++++
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3e9e6f9b66f5..65f18f583ae1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2468,7 +2468,7 @@ i915_gem_init_seqno(struct drm_device *dev, u32 seqno)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int ret, j;
+	int ret;
 
 	/* Carefully retire all requests without writing to the rings */
 	for_each_engine(engine, dev_priv) {
@@ -2479,13 +2479,9 @@ i915_gem_init_seqno(struct drm_device *dev, u32 seqno)
 	i915_gem_retire_requests(dev);
 
 	/* Finally reset hw state */
-	for_each_engine(engine, dev_priv) {
+	for_each_engine(engine, dev_priv)
 		intel_ring_init_seqno(engine, seqno);
 
-		for (j = 0; j < ARRAY_SIZE(engine->semaphore.sync_seqno); j++)
-			engine->semaphore.sync_seqno[j] = 0;
-	}
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 371f4c1fc33c..fb304df8085d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2552,14 +2552,21 @@ void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
 {
 	struct drm_i915_private *dev_priv = to_i915(engine->dev);
 
+	/* Semaphores are strictly monotonic, so whenever we reset the seqno,
+	 * so long as we reset the tracking semaphore value to 0, it will
+	 * always be before the next request's seqno.
+	 */
 	if (INTEL_INFO(dev_priv)->gen == 6 || INTEL_INFO(dev_priv)->gen == 7) {
 		I915_WRITE(RING_SYNC_0(engine->mmio_base), 0);
 		I915_WRITE(RING_SYNC_1(engine->mmio_base), 0);
 		if (HAS_VEBOX(dev_priv))
 			I915_WRITE(RING_SYNC_2(engine->mmio_base), 0);
 	}
+	memset(engine->semaphore.sync_seqno, 0,
+	       sizeof(engine->semaphore.sync_seqno));
 
 	engine->set_seqno(engine, seqno);
+
 	engine->hangcheck.seqno = seqno;
 }
 
-- 
2.8.0.rc3

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

[PATCH 5/7] drm/i915: Reset semaphore page for gen8

[Chris Wilson]

An oversight is that when we wrap the seqno, we need to reset the hw
semaphore counters to 0. We did this for gen6 and gen7 and forgot to do
so for the new implementation required for gen8 (legacy).

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>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index fb304df8085d..c7023d6ca0b7 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2564,6 +2564,14 @@ void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
 	}
 	memset(engine->semaphore.sync_seqno, 0,
 	       sizeof(engine->semaphore.sync_seqno));
+	if (dev_priv->semaphore_obj) {
+		struct drm_i915_gem_object *obj = dev_priv->semaphore_obj;
+		struct page *page = i915_gem_object_get_dirty_page(obj, 0);
+		uint64_t *semaphores = kmap(page);
+		memset(semaphores + engine->id * I915_NUM_ENGINES, 0,
+		       sizeof(*semaphores) * I915_NUM_ENGINES);
+		kunmap(page);
+	}
 
 	engine->set_seqno(engine, seqno);
 
-- 
2.8.0.rc3

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

[PATCH 6/7] drm/i915: Reset engine->last_submitted_seqno

[Chris Wilson]

When we change the current seqno, we also need to remember to reset the
last_submitted_seqno for the engine.

Testcase: igt/gem_exec_whisper
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>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c7023d6ca0b7..be73c1b415e2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2574,6 +2574,7 @@ void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
 	}
 
 	engine->set_seqno(engine, seqno);
+	engine->last_submitted_seqno = seqno;
 
 	engine->hangcheck.seqno = seqno;
 }
-- 
2.8.0.rc3

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

[PATCH 7/7] drm/i915: Simplify check for idleness in hangcheck

[Chris Wilson]

Having fixed the tracking of the engine's last_submitted_seqno, we can
now rely on it for detecting when the engine is idle (and not have to
touch the requests pointer).

Testcase: igt/gem_exec_whisper
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>
---
 drivers/gpu/drm/i915/i915_irq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index c85eb8dec2dc..a56be4f92264 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2805,8 +2805,7 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
 static bool
 ring_idle(struct intel_engine_cs *engine, u32 seqno)
 {
-	return (list_empty(&engine->request_list) ||
-		i915_seqno_passed(seqno, engine->last_submitted_seqno));
+	return i915_seqno_passed(seqno, engine->last_submitted_seqno);
 }
 
 static bool
-- 
2.8.0.rc3

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

Re: [PATCH 1/7] drm/i915: Include engine->last_submitted_seqno in GPU error state

[Joonas Lahtinen]

On ke, 2016-04-06 at 00:57 +0100, Chris Wilson wrote:
> It's useful to look at the last seqno submitted on a particular engine
> and compare it against the HWS value to check for irregularities.
> 
> 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>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c   | 6 ++++--
>  drivers/gpu/drm/i915/i915_drv.h       | 1 +
>  drivers/gpu/drm/i915/i915_gpu_error.c | 2 ++
>  3 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index a2e3af02c292..906e5424e488 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1363,8 +1363,10 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
>  
>  	for_each_engine_id(engine, dev_priv, id) {
>  		seq_printf(m, "%s:\n", engine->name);
> -		seq_printf(m, "\tseqno = %x [current %x]\n",
> -			   engine->hangcheck.seqno, seqno[id]);
> +		seq_printf(m, "\tseqno = %x [current %x, last %x]\n",
> +			   engine->hangcheck.seqno,
> +			   seqno[id],
> +			   engine->last_submitted_seqno);
>  		seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
>  			   (long long)engine->hangcheck.acthd,
>  			   (long long)acthd[id]);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 313bc3576d87..fad8e0d5a216 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -495,6 +495,7 @@ struct drm_i915_error_state {
>  		u32 cpu_ring_head;
>  		u32 cpu_ring_tail;
>  
> +		u32 last_seqno;
>  		u32 semaphore_seqno[I915_NUM_ENGINES - 1];
>  
>  		/* Register state */
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 9b55409d91b3..9463cb4e9323 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -296,6 +296,7 @@ static void i915_ring_error_state(struct drm_i915_error_state_buf *m,
>  		}
>  	}
>  	err_printf(m, "  seqno: 0x%08x\n", ring->seqno);
> +	err_printf(m, "  last_seqno: 0x%08x\n", ring->last_seqno);
>  	err_printf(m, "  waiting: %s\n", yesno(ring->waiting));
>  	err_printf(m, "  ring->head: 0x%08x\n", ring->cpu_ring_head);
>  	err_printf(m, "  ring->tail: 0x%08x\n", ring->cpu_ring_tail);
> @@ -931,6 +932,7 @@ static void i915_record_ring_state(struct drm_device *dev,
>  	ering->instpm = I915_READ(RING_INSTPM(engine->mmio_base));
>  	ering->seqno = engine->get_seqno(engine, false);
>  	ering->acthd = intel_ring_get_active_head(engine);
> +	ering->last_seqno = engine->last_submitted_seqno;
>  	ering->start = I915_READ_START(engine);
>  	ering->head = I915_READ_HEAD(engine);
>  	ering->tail = I915_READ_TAIL(engine);
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/7] drm/i915: Include engine->last_submitted_seqno in GPU error state

[Patchwork]

== Series Details ==

Series: series starting with [1/7] drm/i915: Include engine->last_submitted_seqno in GPU error state
URL   : https://patchwork.freedesktop.org/series/5351/
State : failure

== Summary ==

Series 5351v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5351/revisions/1/mbox/

Test gem_basic:
        Subgroup create-fd-close:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test gem_exec_basic:
        Subgroup basic-default:
                pass       -> SKIP       (bsw-nuc-2)
        Subgroup readonly-blt:
                pass       -> SKIP       (bsw-nuc-2)
Test gem_mmap_gtt:
        Subgroup basic-read-write-distinct:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic-write-read:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test gem_pwrite:
        Subgroup basic:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test gem_ringfill:
        Subgroup basic-default:
                pass       -> INCOMPLETE (bsw-nuc-2)
        Subgroup basic-default-hang:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test gem_sync:
        Subgroup basic-all:
                dmesg-fail -> PASS       (bsw-nuc-2)
        Subgroup basic-vebox:
                pass       -> DMESG-FAIL (bsw-nuc-2)
Test kms_addfb_basic:
        Subgroup basic:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup unused-pitches:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> SKIP       (ivb-t430s)
        Subgroup force-load-detect:
                pass       -> SKIP       (ilk-hp8440p)
        Subgroup prune-stale-modes:
                skip       -> PASS       (ivb-t430s)
Test kms_pipe_crc_basic:
        Subgroup bad-nb-words-3:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test kms_setmode:
        Subgroup basic-clone-single-crtc:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test pm_rps:
        Subgroup basic-api:
                pass       -> DMESG-WARN (bsw-nuc-2)

bdw-nuci7        total:196  pass:184  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:196  pass:175  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:148  pass:110  dwarn:10  dfail:1   fail:0   skip:26 
byt-nuc          total:196  pass:161  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:196  pass:174  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:196  pass:179  dwarn:0   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:196  pass:130  dwarn:1   dfail:0   fail:0   skip:65 
ivb-t430s        total:196  pass:170  dwarn:0   dfail:0   fail:0   skip:26 
skl-i7k-2        total:196  pass:173  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:196  pass:185  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:196  pass:162  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:196  pass:162  dwarn:0   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1812/

12899f13b8ee9a4944f167a08e4db0526a3f3855 drm-intel-nightly: 2016y-04m-05d-19h-09m-25s UTC integration manifest
dce9e0d57ba434cde841ecc3b9ae58cb46e3e4b9 drm/i915: Simplify check for idleness in hangcheck
5edecb0b29f511367669f38d38399042ce2252ab drm/i915: Reset engine->last_submitted_seqno
0647a579d81612c2181a8331f47466819274e6a8 drm/i915: Reset semaphore page for gen8
69965ead9fa84032d63a63428234b8136a5b2f36 drm/i915: Move the hw semaphore initialisation from GEM to the engine
267eef5a3224d69f17e0f7b905e135698ab91b0a drm/i915: Remove unneeded drm_device pointer from intel_ring_init_seqno()
43d1f675567938739ef4712566d67b3e222e1f5d drm/i915: On GPU reset, set the HWS breadcrumb to the last seqno
da9bf3b735ccaf0405d318fc4d5d778693f0dfec drm/i915: Include engine->last_submitted_seqno in GPU error state

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

Re: [PATCH 7/7] drm/i915: Simplify check for idleness in hangcheck

[Joonas Lahtinen]

On ke, 2016-04-06 at 00:57 +0100, Chris Wilson wrote:
> Having fixed the tracking of the engine's last_submitted_seqno, we can
> now rely on it for detecting when the engine is idle (and not have to
> touch the requests pointer).
> 
> Testcase: igt/gem_exec_whisper
> 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>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index c85eb8dec2dc..a56be4f92264 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2805,8 +2805,7 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
>  static bool
>  ring_idle(struct intel_engine_cs *engine, u32 seqno)
>  {
> -	return (list_empty(&engine->request_list) ||
> -		i915_seqno_passed(seqno, engine->last_submitted_seqno));
> +	return i915_seqno_passed(seqno, engine->last_submitted_seqno);
>  }
>  
>  static bool
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: Remove unneeded drm_device pointer from intel_ring_init_seqno()

[Joonas Lahtinen]

On ke, 2016-04-06 at 00:57 +0100, Chris Wilson wrote:
> We only use drm_i915_private within the function, so delete the unneeded
> drm_device local.
> 
> 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>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 2e864b7a528b..371f4c1fc33c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2550,13 +2550,12 @@ int intel_ring_cacheline_align(struct drm_i915_gem_request *req)
>  
>  void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
>  {
> -	struct drm_device *dev = engine->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_private *dev_priv = to_i915(engine->dev);
>  
> -	if (INTEL_INFO(dev)->gen == 6 || INTEL_INFO(dev)->gen == 7) {
> +	if (INTEL_INFO(dev_priv)->gen == 6 || INTEL_INFO(dev_priv)->gen == 7) {
>  		I915_WRITE(RING_SYNC_0(engine->mmio_base), 0);
>  		I915_WRITE(RING_SYNC_1(engine->mmio_base), 0);
> -		if (HAS_VEBOX(dev))
> +		if (HAS_VEBOX(dev_priv))
>  			I915_WRITE(RING_SYNC_2(engine->mmio_base), 0);
>  	}
>  
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915: Include engine->last_submitted_seqno in GPU error state

[Mika Kuoppala]

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

> [ text/plain ]
> It's useful to look at the last seqno submitted on a particular engine
> and compare it against the HWS value to check for irregularities.
>
> 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>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c   | 6 ++++--
>  drivers/gpu/drm/i915/i915_drv.h       | 1 +
>  drivers/gpu/drm/i915/i915_gpu_error.c | 2 ++
>  3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index a2e3af02c292..906e5424e488 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1363,8 +1363,10 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
>  
>  	for_each_engine_id(engine, dev_priv, id) {
>  		seq_printf(m, "%s:\n", engine->name);
> -		seq_printf(m, "\tseqno = %x [current %x]\n",
> -			   engine->hangcheck.seqno, seqno[id]);
> +		seq_printf(m, "\tseqno = %x [current %x, last %x]\n",
> +			   engine->hangcheck.seqno,
> +			   seqno[id],
> +			   engine->last_submitted_seqno);
>  		seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
>  			   (long long)engine->hangcheck.acthd,
>  			   (long long)acthd[id]);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 313bc3576d87..fad8e0d5a216 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -495,6 +495,7 @@ struct drm_i915_error_state {
>  		u32 cpu_ring_head;
>  		u32 cpu_ring_tail;
>  
> +		u32 last_seqno;
>  		u32 semaphore_seqno[I915_NUM_ENGINES - 1];
>  
>  		/* Register state */
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 9b55409d91b3..9463cb4e9323 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -296,6 +296,7 @@ static void i915_ring_error_state(struct drm_i915_error_state_buf *m,
>  		}
>  	}
>  	err_printf(m, "  seqno: 0x%08x\n", ring->seqno);
> +	err_printf(m, "  last_seqno: 0x%08x\n", ring->last_seqno);
>  	err_printf(m, "  waiting: %s\n", yesno(ring->waiting));
>  	err_printf(m, "  ring->head: 0x%08x\n", ring->cpu_ring_head);
>  	err_printf(m, "  ring->tail: 0x%08x\n", ring->cpu_ring_tail);
> @@ -931,6 +932,7 @@ static void i915_record_ring_state(struct drm_device *dev,
>  	ering->instpm = I915_READ(RING_INSTPM(engine->mmio_base));
>  	ering->seqno = engine->get_seqno(engine, false);
>  	ering->acthd = intel_ring_get_active_head(engine);
> +	ering->last_seqno = engine->last_submitted_seqno;
>  	ering->start = I915_READ_START(engine);
>  	ering->head = I915_READ_HEAD(engine);
>  	ering->tail = I915_READ_TAIL(engine);
> -- 
> 2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: On GPU reset, set the HWS breadcrumb to the last seqno

[Mika Kuoppala]

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

> [ text/plain ]
> After the GPU reset and we discard all of the incomplete requests, mark
> the GPU as having advanced to the last_submitted_seqno (as having
> completed the requests and ready for fresh work). The impact of this is
> negligible, as all the requests will be considered completed by this
> point, it just brings the HWS into line with expectations for external
> viewers.
>
> 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>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 40f90c7e718a..3e9e6f9b66f5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2882,6 +2882,8 @@ static void i915_gem_reset_engine_cleanup(struct drm_i915_private *dev_priv,
>  		buffer->last_retired_head = buffer->tail;
>  		intel_ring_update_space(buffer);
>  	}
> +
> +	intel_ring_init_seqno(engine, engine->last_submitted_seqno);
>  }
>  
>  void i915_gem_reset(struct drm_device *dev)
> -- 
> 2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: Move the hw semaphore initialisation from GEM to the engine

[Mika Kuoppala]

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

> [ text/plain ]
> Since we are setting engine local values that are tied to the hardware,
> move it out of i915_gem_init_seqno() into the intel_ring_init_seqno()
> backend, next to where the other hw semaphore registers are written.
>
> 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>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem.c         | 8 ++------
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++++++
>  2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3e9e6f9b66f5..65f18f583ae1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2468,7 +2468,7 @@ i915_gem_init_seqno(struct drm_device *dev, u32 seqno)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_engine_cs *engine;
> -	int ret, j;
> +	int ret;
>  
>  	/* Carefully retire all requests without writing to the rings */
>  	for_each_engine(engine, dev_priv) {
> @@ -2479,13 +2479,9 @@ i915_gem_init_seqno(struct drm_device *dev, u32 seqno)
>  	i915_gem_retire_requests(dev);
>  
>  	/* Finally reset hw state */
> -	for_each_engine(engine, dev_priv) {
> +	for_each_engine(engine, dev_priv)
>  		intel_ring_init_seqno(engine, seqno);
>  
> -		for (j = 0; j < ARRAY_SIZE(engine->semaphore.sync_seqno); j++)
> -			engine->semaphore.sync_seqno[j] = 0;
> -	}
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 371f4c1fc33c..fb304df8085d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2552,14 +2552,21 @@ void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(engine->dev);
>  
> +	/* Semaphores are strictly monotonic, so whenever we reset the seqno,
> +	 * so long as we reset the tracking semaphore value to 0, it will
> +	 * always be before the next request's seqno.
> +	 */
>  	if (INTEL_INFO(dev_priv)->gen == 6 || INTEL_INFO(dev_priv)->gen == 7) {
>  		I915_WRITE(RING_SYNC_0(engine->mmio_base), 0);
>  		I915_WRITE(RING_SYNC_1(engine->mmio_base), 0);
>  		if (HAS_VEBOX(dev_priv))
>  			I915_WRITE(RING_SYNC_2(engine->mmio_base), 0);
>  	}
> +	memset(engine->semaphore.sync_seqno, 0,
> +	       sizeof(engine->semaphore.sync_seqno));
>  
>  	engine->set_seqno(engine, seqno);
> +
>  	engine->hangcheck.seqno = seqno;
>  }
>  
> -- 
> 2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915: Reset engine->last_submitted_seqno

[Mika Kuoppala]

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

> [ text/plain ]
> When we change the current seqno, we also need to remember to reset the
> last_submitted_seqno for the engine.
>
> Testcase: igt/gem_exec_whisper
> 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>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c7023d6ca0b7..be73c1b415e2 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2574,6 +2574,7 @@ void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
>  	}
>  
>  	engine->set_seqno(engine, seqno);
> +	engine->last_submitted_seqno = seqno;
>  
>  	engine->hangcheck.seqno = seqno;
>  }
> -- 
> 2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915: Simplify check for idleness in hangcheck

[Mika Kuoppala]

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

> [ text/plain ]
> Having fixed the tracking of the engine's last_submitted_seqno, we can
> now rely on it for detecting when the engine is idle (and not have to
> touch the requests pointer).
>
> Testcase: igt/gem_exec_whisper
> 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>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index c85eb8dec2dc..a56be4f92264 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2805,8 +2805,7 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
>  static bool
>  ring_idle(struct intel_engine_cs *engine, u32 seqno)
>  {
> -	return (list_empty(&engine->request_list) ||
> -		i915_seqno_passed(seqno, engine->last_submitted_seqno));
> +	return i915_seqno_passed(seqno, engine->last_submitted_seqno);

My concern here is that we should move the write for the
last_submitted_seqno before we push the work into ring.

To guarantee that the driver never sees a seqno > last_submitted_seqno
on particular ring.

-Mika

>  }
>  
>  static bool
> -- 
> 2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915: Simplify check for idleness in hangcheck

[Chris Wilson]

On Wed, Apr 06, 2016 at 12:32:29PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > [ text/plain ]
> > Having fixed the tracking of the engine's last_submitted_seqno, we can
> > now rely on it for detecting when the engine is idle (and not have to
> > touch the requests pointer).
> >
> > Testcase: igt/gem_exec_whisper
> > 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>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index c85eb8dec2dc..a56be4f92264 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2805,8 +2805,7 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
> >  static bool
> >  ring_idle(struct intel_engine_cs *engine, u32 seqno)
> >  {
> > -	return (list_empty(&engine->request_list) ||
> > -		i915_seqno_passed(seqno, engine->last_submitted_seqno));
> > +	return i915_seqno_passed(seqno, engine->last_submitted_seqno);
> 
> My concern here is that we should move the write for the
> last_submitted_seqno before we push the work into ring.
> 
> To guarantee that the driver never sees a seqno > last_submitted_seqno
> on particular ring.

Reasonable request. You'll want a smp_store_mb() on the
last_submitted_seqno as well with a comment
/* barrier for hangcheck */

In return what's up with the Braswell?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915: Reset semaphore page for gen8

[Joonas Lahtinen]

On ke, 2016-04-06 at 00:57 +0100, Chris Wilson wrote:
> An oversight is that when we wrap the seqno, we need to reset the hw
> semaphore counters to 0. We did this for gen6 and gen7 and forgot to do
> so for the new implementation required for gen8 (legacy).
> 
> 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>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index fb304df8085d..c7023d6ca0b7 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2564,6 +2564,14 @@ void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
>  	}
>  	memset(engine->semaphore.sync_seqno, 0,
>  	       sizeof(engine->semaphore.sync_seqno));
> +	if (dev_priv->semaphore_obj) {
> +		struct drm_i915_gem_object *obj = dev_priv->semaphore_obj;
> +		struct page *page = i915_gem_object_get_dirty_page(obj, 0);
> +		uint64_t *semaphores = kmap(page);
> +		memset(semaphores + engine->id * I915_NUM_ENGINES, 0,
> +		       sizeof(*semaphores) * I915_NUM_ENGINES);

There is i915_semaphore_seqno_size define which the GEN8_WAIT_OFFSET
and GEN8_SIGNAL_OFFSET use. So rather use that to stay consistent.

Other than that,

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

PS. The existing semaphore code could use some cleanup, adding to
backlog.

> +		kunmap(page);
> +	}
>  
>  	engine->set_seqno(engine, seqno);
>  
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915: Reset semaphore page for gen8

[Chris Wilson]

On Wed, Apr 06, 2016 at 12:58:43PM +0300, Joonas Lahtinen wrote:
> On ke, 2016-04-06 at 00:57 +0100, Chris Wilson wrote:
> > An oversight is that when we wrap the seqno, we need to reset the hw
> > semaphore counters to 0. We did this for gen6 and gen7 and forgot to do
> > so for the new implementation required for gen8 (legacy).
> > 
> > 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>
> > ---
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index fb304df8085d..c7023d6ca0b7 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -2564,6 +2564,14 @@ void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
> >  	}
> >  	memset(engine->semaphore.sync_seqno, 0,
> >  	       sizeof(engine->semaphore.sync_seqno));
> > +	if (dev_priv->semaphore_obj) {
> > +		struct drm_i915_gem_object *obj = dev_priv->semaphore_obj;
> > +		struct page *page = i915_gem_object_get_dirty_page(obj, 0);
> > +		uint64_t *semaphores = kmap(page);
> > +		memset(semaphores + engine->id * I915_NUM_ENGINES, 0,
> > +		       sizeof(*semaphores) * I915_NUM_ENGINES);
> 
> There is i915_semaphore_seqno_size define which the GEN8_WAIT_OFFSET
> and GEN8_SIGNAL_OFFSET use. So rather use that to stay consistent.

I didn't want the size, I wanted the type. What I really wanted were
sensible macros...

I can change this to i915_semaphore_seqno_size (even though that is not
the right name) and void *.
 
> Other than that,
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> PS. The existing semaphore code could use some cleanup, adding to
> backlog.

The existing semaphore code is 3 patches from working...
1. fix hw-id
2. fix signaling
3. reset pd after wait
4. enable/profit
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: Move the hw semaphore initialisation from GEM to the engine

[Joonas Lahtinen]

On ke, 2016-04-06 at 00:57 +0100, Chris Wilson wrote:
> Since we are setting engine local values that are tied to the hardware,
> move it out of i915_gem_init_seqno() into the intel_ring_init_seqno()
> backend, next to where the other hw semaphore registers are written.
> 
> 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>

Comment below, but anyway;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem.c         | 8 ++------
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++++++
>  2 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3e9e6f9b66f5..65f18f583ae1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2468,7 +2468,7 @@ i915_gem_init_seqno(struct drm_device *dev, u32 seqno)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_engine_cs *engine;
> -	int ret, j;
> +	int ret;
>  
>  	/* Carefully retire all requests without writing to the rings */
>  	for_each_engine(engine, dev_priv) {
> @@ -2479,13 +2479,9 @@ i915_gem_init_seqno(struct drm_device *dev, u32 seqno)
>  	i915_gem_retire_requests(dev);
>  
>  	/* Finally reset hw state */
> -	for_each_engine(engine, dev_priv) {
> +	for_each_engine(engine, dev_priv)
>  		intel_ring_init_seqno(engine, seqno);
>  
> -		for (j = 0; j < ARRAY_SIZE(engine->semaphore.sync_seqno); j++)
> -			engine->semaphore.sync_seqno[j] = 0;
> -	}
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 371f4c1fc33c..fb304df8085d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2552,14 +2552,21 @@ void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(engine->dev);
>  
> +	/* Semaphores are strictly monotonic, so whenever we reset the seqno,
> +	 * so long as we reset the tracking semaphore value to 0, it will
> +	 * always be before the next request's seqno.
> +	 */

I'd start the comment with; "Our semaphore implementation" to make it
clear it's deliberately engineered to be so, not by hardware.

Regards, Joonas

>  	if (INTEL_INFO(dev_priv)->gen == 6 || INTEL_INFO(dev_priv)->gen == 7) {
>  		I915_WRITE(RING_SYNC_0(engine->mmio_base), 0);
>  		I915_WRITE(RING_SYNC_1(engine->mmio_base), 0);
>  		if (HAS_VEBOX(dev_priv))
>  			I915_WRITE(RING_SYNC_2(engine->mmio_base), 0);
>  	}
> +	memset(engine->semaphore.sync_seqno, 0,
> +	       sizeof(engine->semaphore.sync_seqno));
>  
>  	engine->set_seqno(engine, seqno);
> +
>  	engine->hangcheck.seqno = seqno;
>  }
>  
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915: Reset engine->last_submitted_seqno

[Joonas Lahtinen]

On ke, 2016-04-06 at 00:57 +0100, Chris Wilson wrote:
> When we change the current seqno, we also need to remember to reset the
> last_submitted_seqno for the engine.
> 
> Testcase: igt/gem_exec_whisper
> 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>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c7023d6ca0b7..be73c1b415e2 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2574,6 +2574,7 @@ void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
>  	}
>  
>  	engine->set_seqno(engine, seqno);
> +	engine->last_submitted_seqno = seqno;
>  
>  	engine->hangcheck.seqno = seqno;
>  }
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: On GPU reset, set the HWS breadcrumb to the last seqno

[Joonas Lahtinen]

On ke, 2016-04-06 at 00:57 +0100, Chris Wilson wrote:
> After the GPU reset and we discard all of the incomplete requests, mark
> the GPU as having advanced to the last_submitted_seqno (as having
> completed the requests and ready for fresh work). The impact of this is
> negligible, as all the requests will be considered completed by this
> point, it just brings the HWS into line with expectations for external
> viewers.
> 
> 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>

Makes sense to me.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 40f90c7e718a..3e9e6f9b66f5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2882,6 +2882,8 @@ static void i915_gem_reset_engine_cleanup(struct drm_i915_private *dev_priv,
>  		buffer->last_retired_head = buffer->tail;
>  		intel_ring_update_space(buffer);
>  	}
> +
> +	intel_ring_init_seqno(engine, engine->last_submitted_seqno);
>  }
>  
>  void i915_gem_reset(struct drm_device *dev)
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915: Reset semaphore page for gen8

[Joonas Lahtinen]

On ke, 2016-04-06 at 11:10 +0100, Chris Wilson wrote:
> On Wed, Apr 06, 2016 at 12:58:43PM +0300, Joonas Lahtinen wrote:
> > 
> > On ke, 2016-04-06 at 00:57 +0100, Chris Wilson wrote:
> > > 
> > > An oversight is that when we wrap the seqno, we need to reset the hw
> > > semaphore counters to 0. We did this for gen6 and gen7 and forgot to do
> > > so for the new implementation required for gen8 (legacy).
> > > 
> > > 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>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > index fb304df8085d..c7023d6ca0b7 100644
> > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > @@ -2564,6 +2564,14 @@ void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
> > >  	}
> > >  	memset(engine->semaphore.sync_seqno, 0,
> > >  	       sizeof(engine->semaphore.sync_seqno));
> > > +	if (dev_priv->semaphore_obj) {
> > > +		struct drm_i915_gem_object *obj = dev_priv->semaphore_obj;
> > > +		struct page *page = i915_gem_object_get_dirty_page(obj, 0);
> > > +		uint64_t *semaphores = kmap(page);
> > > +		memset(semaphores + engine->id * I915_NUM_ENGINES, 0,
> > > +		       sizeof(*semaphores) * I915_NUM_ENGINES);
> > There is i915_semaphore_seqno_size define which the GEN8_WAIT_OFFSET
> > and GEN8_SIGNAL_OFFSET use. So rather use that to stay consistent.
> I didn't want the size, I wanted the type. What I really wanted were
> sensible macros...

The macros could use some cleanup is what I meant.

> 
> I can change this to i915_semaphore_seqno_size (even though that is not
> the right name) and void *.
>  
> > 
> > Other than that,
> > 
> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > 
> > PS. The existing semaphore code could use some cleanup, adding to
> > backlog.
> The existing semaphore code is 3 patches from working...
> 1. fix hw-id
> 2. fix signaling
> 3. reset pd after wait
> 4. enable/profit
> -Chris
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/9] drm/i915: Include engine->last_submitted_seqno in GPU error state

[Chris Wilson]

It's useful to look at the last seqno submitted on a particular engine
and compare it against the HWS value to check for irregularities.

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>
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c   | 6 ++++--
 drivers/gpu/drm/i915/i915_drv.h       | 1 +
 drivers/gpu/drm/i915/i915_gpu_error.c | 2 ++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a2e3af02c292..906e5424e488 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1363,8 +1363,10 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
 
 	for_each_engine_id(engine, dev_priv, id) {
 		seq_printf(m, "%s:\n", engine->name);
-		seq_printf(m, "\tseqno = %x [current %x]\n",
-			   engine->hangcheck.seqno, seqno[id]);
+		seq_printf(m, "\tseqno = %x [current %x, last %x]\n",
+			   engine->hangcheck.seqno,
+			   seqno[id],
+			   engine->last_submitted_seqno);
 		seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
 			   (long long)engine->hangcheck.acthd,
 			   (long long)acthd[id]);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a1f78f275c55..c7ce452d35ad 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -495,6 +495,7 @@ struct drm_i915_error_state {
 		u32 cpu_ring_head;
 		u32 cpu_ring_tail;
 
+		u32 last_seqno;
 		u32 semaphore_seqno[I915_NUM_ENGINES - 1];
 
 		/* Register state */
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 9b55409d91b3..9463cb4e9323 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -296,6 +296,7 @@ static void i915_ring_error_state(struct drm_i915_error_state_buf *m,
 		}
 	}
 	err_printf(m, "  seqno: 0x%08x\n", ring->seqno);
+	err_printf(m, "  last_seqno: 0x%08x\n", ring->last_seqno);
 	err_printf(m, "  waiting: %s\n", yesno(ring->waiting));
 	err_printf(m, "  ring->head: 0x%08x\n", ring->cpu_ring_head);
 	err_printf(m, "  ring->tail: 0x%08x\n", ring->cpu_ring_tail);
@@ -931,6 +932,7 @@ static void i915_record_ring_state(struct drm_device *dev,
 	ering->instpm = I915_READ(RING_INSTPM(engine->mmio_base));
 	ering->seqno = engine->get_seqno(engine, false);
 	ering->acthd = intel_ring_get_active_head(engine);
+	ering->last_seqno = engine->last_submitted_seqno;
 	ering->start = I915_READ_START(engine);
 	ering->head = I915_READ_HEAD(engine);
 	ering->tail = I915_READ_TAIL(engine);
-- 
2.8.0.rc3

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

[PATCH v2 2/9] drm/i915: On GPU reset, set the HWS breadcrumb to the last seqno

[Chris Wilson]

After the GPU reset and we discard all of the incomplete requests, mark
the GPU as having advanced to the last_submitted_seqno (as having
completed the requests and ready for fresh work). The impact of this is
negligible, as all the requests will be considered completed by this
point, it just brings the HWS into line with expectations for external
viewers.

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>
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b342f672f391..05711e7f9fb8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2882,6 +2882,8 @@ static void i915_gem_reset_engine_cleanup(struct drm_i915_private *dev_priv,
 		buffer->last_retired_head = buffer->tail;
 		intel_ring_update_space(buffer);
 	}
+
+	intel_ring_init_seqno(engine, engine->last_submitted_seqno);
 }
 
 void i915_gem_reset(struct drm_device *dev)
-- 
2.8.0.rc3

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

[PATCH v2 3/9] drm/i915: Remove unneeded drm_device pointer from intel_ring_init_seqno()

[Chris Wilson]

We only use drm_i915_private within the function, so delete the unneeded
drm_device local.

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>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2e864b7a528b..371f4c1fc33c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2550,13 +2550,12 @@ int intel_ring_cacheline_align(struct drm_i915_gem_request *req)
 
 void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
 {
-	struct drm_device *dev = engine->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(engine->dev);
 
-	if (INTEL_INFO(dev)->gen == 6 || INTEL_INFO(dev)->gen == 7) {
+	if (INTEL_INFO(dev_priv)->gen == 6 || INTEL_INFO(dev_priv)->gen == 7) {
 		I915_WRITE(RING_SYNC_0(engine->mmio_base), 0);
 		I915_WRITE(RING_SYNC_1(engine->mmio_base), 0);
-		if (HAS_VEBOX(dev))
+		if (HAS_VEBOX(dev_priv))
 			I915_WRITE(RING_SYNC_2(engine->mmio_base), 0);
 	}
 
-- 
2.8.0.rc3

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

[PATCH v2 4/9] drm/i915: Move the hw semaphore initialisation from GEM to the engine

[Chris Wilson]

Since we are setting engine local values that are tied to the hardware,
move it out of i915_gem_init_seqno() into the intel_ring_init_seqno()
backend, next to where the other hw semaphore registers are written.

v2: Make the explanatory comment about always resetting the semaphores to
0 irrespective of the value of the reset seqno.

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>
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |  8 ++------
 drivers/gpu/drm/i915/intel_ringbuffer.c | 11 +++++++++++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 05711e7f9fb8..7506e850913e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2468,7 +2468,7 @@ i915_gem_init_seqno(struct drm_device *dev, u32 seqno)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int ret, j;
+	int ret;
 
 	/* Carefully retire all requests without writing to the rings */
 	for_each_engine(engine, dev_priv) {
@@ -2479,13 +2479,9 @@ i915_gem_init_seqno(struct drm_device *dev, u32 seqno)
 	i915_gem_retire_requests(dev);
 
 	/* Finally reset hw state */
-	for_each_engine(engine, dev_priv) {
+	for_each_engine(engine, dev_priv)
 		intel_ring_init_seqno(engine, seqno);
 
-		for (j = 0; j < ARRAY_SIZE(engine->semaphore.sync_seqno); j++)
-			engine->semaphore.sync_seqno[j] = 0;
-	}
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 371f4c1fc33c..69852ac4018d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2552,14 +2552,25 @@ void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
 {
 	struct drm_i915_private *dev_priv = to_i915(engine->dev);
 
+	/* Our semaphore implementation is strictly monotonic (i.e. we proceed
+	 * so long as the semaphore value in the register/page is greater
+	 * than the sync value), so whenever we reset the seqno,
+	 * so long as we reset the tracking semaphore value to 0, it will
+	 * always be before the next request's seqno. If we don't reset
+	 * the semaphore value, then when the seqno moves backwards all
+	 * future waits will complete instantly (causing rendering corruption).
+	 */
 	if (INTEL_INFO(dev_priv)->gen == 6 || INTEL_INFO(dev_priv)->gen == 7) {
 		I915_WRITE(RING_SYNC_0(engine->mmio_base), 0);
 		I915_WRITE(RING_SYNC_1(engine->mmio_base), 0);
 		if (HAS_VEBOX(dev_priv))
 			I915_WRITE(RING_SYNC_2(engine->mmio_base), 0);
 	}
+	memset(engine->semaphore.sync_seqno, 0,
+	       sizeof(engine->semaphore.sync_seqno));
 
 	engine->set_seqno(engine, seqno);
+
 	engine->hangcheck.seqno = seqno;
 }
 
-- 
2.8.0.rc3

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

[PATCH v2 5/9] drm/i915: Refactor gen8 semaphore offset calculation

[Chris Wilson]

We reuse the same calculation into two macros, and I want to add a third
user. Time to refactor.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_ringbuffer.h | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 18074ab55f61..98eadfa79116 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -52,16 +52,15 @@ struct  intel_hw_status_page {
 /* seqno size is actually only a uint32, but since we plan to use MI_FLUSH_DW to
  * do the writes, and that must have qw aligned offsets, simply pretend it's 8b.
  */
-#define i915_semaphore_seqno_size sizeof(uint64_t)
+#define gen8_semaphore_seqno_size sizeof(uint64_t)
+#define GEN8_SEMAPHORE_OFFSET(__from, __to)			     \
+	(((__from) * I915_NUM_ENGINES  + (__to)) * gen8_semaphore_seqno_size)
 #define GEN8_SIGNAL_OFFSET(__ring, to)			     \
 	(i915_gem_obj_ggtt_offset(dev_priv->semaphore_obj) + \
-	((__ring)->id * I915_NUM_ENGINES * i915_semaphore_seqno_size) +	\
-	(i915_semaphore_seqno_size * (to)))
-
+	 GEN8_SEMAPHORE_OFFSET((__ring)->id, (to)))
 #define GEN8_WAIT_OFFSET(__ring, from)			     \
 	(i915_gem_obj_ggtt_offset(dev_priv->semaphore_obj) + \
-	((from) * I915_NUM_ENGINES * i915_semaphore_seqno_size) + \
-	(i915_semaphore_seqno_size * (__ring)->id))
+	 GEN8_SEMAPHORE_OFFSET(from, (__ring)->id))
 
 #define GEN8_RING_SEMAPHORE_INIT(e) do { \
 	if (!dev_priv->semaphore_obj) { \
-- 
2.8.0.rc3

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

[PATCH v2 6/9] drm/i915: Reset semaphore page for gen8

[Chris Wilson]

An oversight is that when we wrap the seqno, we need to reset the hw
semaphore counters to 0. We did this for gen6 and gen7 and forgot to do
so for the new implementation required for gen8 (legacy).

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>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 69852ac4018d..0e6490074011 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2566,6 +2566,14 @@ void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
 		if (HAS_VEBOX(dev_priv))
 			I915_WRITE(RING_SYNC_2(engine->mmio_base), 0);
 	}
+	if (dev_priv->semaphore_obj) {
+		struct drm_i915_gem_object *obj = dev_priv->semaphore_obj;
+		struct page *page = i915_gem_object_get_dirty_page(obj, 0);
+		void *semaphores = kmap(page);
+		memset(semaphores + GEN8_SEMAPHORE_OFFSET(engine->id, 0),
+		       0, I915_NUM_ENGINES * gen8_semaphore_seqno_size);
+		kunmap(page);
+	}
 	memset(engine->semaphore.sync_seqno, 0,
 	       sizeof(engine->semaphore.sync_seqno));
 
-- 
2.8.0.rc3

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

[PATCH v2 7/9] drm/i915: Reset engine->last_submitted_seqno

[Chris Wilson]

When we change the current seqno, we also need to remember to reset the
last_submitted_seqno for the engine.

Testcase: igt/gem_exec_whisper
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>
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 0e6490074011..6b4952031e30 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2578,6 +2578,7 @@ void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
 	       sizeof(engine->semaphore.sync_seqno));
 
 	engine->set_seqno(engine, seqno);
+	engine->last_submitted_seqno = seqno;
 
 	engine->hangcheck.seqno = seqno;
 }
-- 
2.8.0.rc3

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

[PATCH v2 8/9] drm/i915: Apply a mb between emitting the request and hangcheck

[Chris Wilson]

Seal the request and mark it as pending execution before we submit it to
hardware. We assume that the actual submission cannot fail (that
guarantee is provided by preallocating space in the request for the
submission). As we may inspect this state without holding any locks
during hangcheck we should apply a barrier to ensure that we do
not see a more recent value in the HWS than we are tracking.

Based on a patch by Mika Kuoppala.

Suggested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
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>
---
 drivers/gpu/drm/i915/i915_gem.c | 39 ++++++++++++++++++++++-----------------
 drivers/gpu/drm/i915/i915_irq.c |  3 ++-
 2 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7506e850913e..5a65a7663b88 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2575,6 +2575,28 @@ void __i915_add_request(struct drm_i915_gem_request *request,
 		WARN(ret, "*_ring_flush_all_caches failed: %d!\n", ret);
 	}
 
+	trace_i915_gem_request_add(request);
+
+	request->head = request_start;
+
+	/* Whilst this request exists, batch_obj will be on the
+	 * active_list, and so will hold the active reference. Only when this
+	 * request is retired will the the batch_obj be moved onto the
+	 * inactive_list and lose its active reference. Hence we do not need
+	 * to explicitly hold another reference here.
+	 */
+	request->batch_obj = obj;
+
+	/* Seal the request and mark it as pending execution. Note that
+	 * we may inspect this state, without holding any locks, during
+	 * hangcheck. Hence we apply the barrier to ensure that we do not
+	 * see a more recent value in the hws than we are tracking.
+	 */
+	request->emitted_jiffies = jiffies;
+	request->previous_seqno = engine->last_submitted_seqno;
+	smp_store_mb(engine->last_submitted_seqno, request->seqno);
+	list_add_tail(&request->list, &engine->request_list);
+
 	/* Record the position of the start of the request so that
 	 * should we detect the updated seqno part-way through the
 	 * GPU processing the request, we never over-estimate the
@@ -2592,23 +2614,6 @@ void __i915_add_request(struct drm_i915_gem_request *request,
 	/* Not allowed to fail! */
 	WARN(ret, "emit|add_request failed: %d!\n", ret);
 
-	request->head = request_start;
-
-	/* Whilst this request exists, batch_obj will be on the
-	 * active_list, and so will hold the active reference. Only when this
-	 * request is retired will the the batch_obj be moved onto the
-	 * inactive_list and lose its active reference. Hence we do not need
-	 * to explicitly hold another reference here.
-	 */
-	request->batch_obj = obj;
-
-	request->emitted_jiffies = jiffies;
-	request->previous_seqno = engine->last_submitted_seqno;
-	engine->last_submitted_seqno = request->seqno;
-	list_add_tail(&request->list, &engine->request_list);
-
-	trace_i915_gem_request_add(request);
-
 	i915_queue_hangcheck(engine->dev);
 
 	queue_delayed_work(dev_priv->wq,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5bec13844800..a5eb77d1f8cb 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2806,7 +2806,8 @@ static bool
 ring_idle(struct intel_engine_cs *engine, u32 seqno)
 {
 	return (list_empty(&engine->request_list) ||
-		i915_seqno_passed(seqno, engine->last_submitted_seqno));
+		i915_seqno_passed(seqno,
+				  READ_ONCE(engine->last_submitted_seqno)));
 }
 
 static bool
-- 
2.8.0.rc3

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

[PATCH v2 9/9] drm/i915: Simplify check for idleness in hangcheck

[Chris Wilson]

Having fixed the tracking of the engine's last_submitted_seqno, we can
now rely on it for detecting when the engine is idle (and not have to
touch the requests pointer).

Testcase: igt/gem_exec_whisper
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>
---
 drivers/gpu/drm/i915/i915_irq.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a5eb77d1f8cb..5007cde8d4c3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2805,9 +2805,8 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
 static bool
 ring_idle(struct intel_engine_cs *engine, u32 seqno)
 {
-	return (list_empty(&engine->request_list) ||
-		i915_seqno_passed(seqno,
-				  READ_ONCE(engine->last_submitted_seqno)));
+	return i915_seqno_passed(seqno,
+				 READ_ONCE(engine->last_submitted_seqno));
 }
 
 static bool
-- 
2.8.0.rc3

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

Re: [PATCH v2 8/9] drm/i915: Apply a mb between emitting the request and hangcheck

[Mika Kuoppala]

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

> [ text/plain ]
> Seal the request and mark it as pending execution before we submit it to
> hardware. We assume that the actual submission cannot fail (that
> guarantee is provided by preallocating space in the request for the
> submission). As we may inspect this state without holding any locks
> during hangcheck we should apply a barrier to ensure that we do
> not see a more recent value in the HWS than we are tracking.
>
> Based on a patch by Mika Kuoppala.
>
> Suggested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> 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>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 39 ++++++++++++++++++++++-----------------
>  drivers/gpu/drm/i915/i915_irq.c |  3 ++-
>  2 files changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7506e850913e..5a65a7663b88 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2575,6 +2575,28 @@ void __i915_add_request(struct drm_i915_gem_request *request,
>  		WARN(ret, "*_ring_flush_all_caches failed: %d!\n", ret);
>  	}
>  
> +	trace_i915_gem_request_add(request);
> +
> +	request->head = request_start;
> +
> +	/* Whilst this request exists, batch_obj will be on the
> +	 * active_list, and so will hold the active reference. Only when this
> +	 * request is retired will the the batch_obj be moved onto the
> +	 * inactive_list and lose its active reference. Hence we do not need
> +	 * to explicitly hold another reference here.
> +	 */
> +	request->batch_obj = obj;
> +
> +	/* Seal the request and mark it as pending execution. Note that
> +	 * we may inspect this state, without holding any locks, during
> +	 * hangcheck. Hence we apply the barrier to ensure that we do not
> +	 * see a more recent value in the hws than we are tracking.
> +	 */
> +	request->emitted_jiffies = jiffies;
> +	request->previous_seqno = engine->last_submitted_seqno;
> +	smp_store_mb(engine->last_submitted_seqno, request->seqno);
> +	list_add_tail(&request->list, &engine->request_list);
> +
>  	/* Record the position of the start of the request so that
>  	 * should we detect the updated seqno part-way through the
>  	 * GPU processing the request, we never over-estimate the
> @@ -2592,23 +2614,6 @@ void __i915_add_request(struct drm_i915_gem_request *request,
>  	/* Not allowed to fail! */
>  	WARN(ret, "emit|add_request failed: %d!\n", ret);
>  
> -	request->head = request_start;
> -
> -	/* Whilst this request exists, batch_obj will be on the
> -	 * active_list, and so will hold the active reference. Only when this
> -	 * request is retired will the the batch_obj be moved onto the
> -	 * inactive_list and lose its active reference. Hence we do not need
> -	 * to explicitly hold another reference here.
> -	 */
> -	request->batch_obj = obj;
> -
> -	request->emitted_jiffies = jiffies;
> -	request->previous_seqno = engine->last_submitted_seqno;
> -	engine->last_submitted_seqno = request->seqno;
> -	list_add_tail(&request->list, &engine->request_list);
> -
> -	trace_i915_gem_request_add(request);
> -
>  	i915_queue_hangcheck(engine->dev);
>  
>  	queue_delayed_work(dev_priv->wq,
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 5bec13844800..a5eb77d1f8cb 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2806,7 +2806,8 @@ static bool
>  ring_idle(struct intel_engine_cs *engine, u32 seqno)
>  {
>  	return (list_empty(&engine->request_list) ||
> -		i915_seqno_passed(seqno, engine->last_submitted_seqno));
> +		i915_seqno_passed(seqno,
> +				  READ_ONCE(engine->last_submitted_seqno)));
>  }
>  
>  static bool
> -- 
> 2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 9/9] drm/i915: Simplify check for idleness in hangcheck

[Mika Kuoppala]

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

> [ text/plain ]
> Having fixed the tracking of the engine's last_submitted_seqno, we can
> now rely on it for detecting when the engine is idle (and not have to
> touch the requests pointer).
>
> Testcase: igt/gem_exec_whisper
> 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>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a5eb77d1f8cb..5007cde8d4c3 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2805,9 +2805,8 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
>  static bool
>  ring_idle(struct intel_engine_cs *engine, u32 seqno)
>  {
> -	return (list_empty(&engine->request_list) ||
> -		i915_seqno_passed(seqno,
> -				  READ_ONCE(engine->last_submitted_seqno)));
> +	return i915_seqno_passed(seqno,
> +				 READ_ONCE(engine->last_submitted_seqno));
>  }
>  
>  static bool
> -- 
> 2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 6/9] drm/i915: Reset semaphore page for gen8

[Joonas Lahtinen]

On ke, 2016-04-06 at 13:33 +0100, Chris Wilson wrote:
> An oversight is that when we wrap the seqno, we need to reset the hw
> semaphore counters to 0. We did this for gen6 and gen7 and forgot to do
> so for the new implementation required for gen8 (legacy).
> 
> 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>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 69852ac4018d..0e6490074011 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2566,6 +2566,14 @@ void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
>  		if (HAS_VEBOX(dev_priv))
>  			I915_WRITE(RING_SYNC_2(engine->mmio_base), 0);
>  	}
> +	if (dev_priv->semaphore_obj) {
> +		struct drm_i915_gem_object *obj = dev_priv->semaphore_obj;
> +		struct page *page = i915_gem_object_get_dirty_page(obj, 0);
> +		void *semaphores = kmap(page);
> +		memset(semaphores + GEN8_SEMAPHORE_OFFSET(engine->id, 0),
> +		       0, I915_NUM_ENGINES * gen8_semaphore_seqno_size);
> +		kunmap(page);
> +	}
>  	memset(engine->semaphore.sync_seqno, 0,
>  	       sizeof(engine->semaphore.sync_seqno));
>  
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 5/9] drm/i915: Refactor gen8 semaphore offset calculation

[Joonas Lahtinen]

On ke, 2016-04-06 at 13:33 +0100, Chris Wilson wrote:
> We reuse the same calculation into two macros, and I want to add a third
> user. Time to refactor.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 18074ab55f61..98eadfa79116 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -52,16 +52,15 @@ struct  intel_hw_status_page {
>  /* seqno size is actually only a uint32, but since we plan to use MI_FLUSH_DW to
>   * do the writes, and that must have qw aligned offsets, simply pretend it's 8b.
>   */
> -#define i915_semaphore_seqno_size sizeof(uint64_t)
> +#define gen8_semaphore_seqno_size sizeof(uint64_t)
> +#define GEN8_SEMAPHORE_OFFSET(__from, __to)			     \
> +	(((__from) * I915_NUM_ENGINES  + (__to)) * gen8_semaphore_seqno_size)
>  #define GEN8_SIGNAL_OFFSET(__ring, to)			     \
>  	(i915_gem_obj_ggtt_offset(dev_priv->semaphore_obj) + \
> -	((__ring)->id * I915_NUM_ENGINES * i915_semaphore_seqno_size) +	\
> -	(i915_semaphore_seqno_size * (to)))
> -
> +	 GEN8_SEMAPHORE_OFFSET((__ring)->id, (to)))
>  #define GEN8_WAIT_OFFSET(__ring, from)			     \
>  	(i915_gem_obj_ggtt_offset(dev_priv->semaphore_obj) + \
> -	((from) * I915_NUM_ENGINES * i915_semaphore_seqno_size) + \
> -	(i915_semaphore_seqno_size * (__ring)->id))
> +	 GEN8_SEMAPHORE_OFFSET(from, (__ring)->id))
>  
>  #define GEN8_RING_SEMAPHORE_INIT(e) do { \
>  	if (!dev_priv->semaphore_obj) { \
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/9] drm/i915: Move the hw semaphore initialisation from GEM to the engine

[Joonas Lahtinen]

On ke, 2016-04-06 at 13:33 +0100, Chris Wilson wrote:
> Since we are setting engine local values that are tied to the hardware,
> move it out of i915_gem_init_seqno() into the intel_ring_init_seqno()
> backend, next to where the other hw semaphore registers are written.
> 
> v2: Make the explanatory comment about always resetting the semaphores to
> 0 irrespective of the value of the reset seqno.
> 
> 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>
> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         |  8 ++------
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 11 +++++++++++
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 05711e7f9fb8..7506e850913e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2468,7 +2468,7 @@ i915_gem_init_seqno(struct drm_device *dev, u32 seqno)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_engine_cs *engine;
> -	int ret, j;
> +	int ret;
>  
>  	/* Carefully retire all requests without writing to the rings */
>  	for_each_engine(engine, dev_priv) {
> @@ -2479,13 +2479,9 @@ i915_gem_init_seqno(struct drm_device *dev, u32 seqno)
>  	i915_gem_retire_requests(dev);
>  
>  	/* Finally reset hw state */
> -	for_each_engine(engine, dev_priv) {
> +	for_each_engine(engine, dev_priv)
>  		intel_ring_init_seqno(engine, seqno);
>  
> -		for (j = 0; j < ARRAY_SIZE(engine->semaphore.sync_seqno); j++)
> -			engine->semaphore.sync_seqno[j] = 0;
> -	}
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 371f4c1fc33c..69852ac4018d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2552,14 +2552,25 @@ void intel_ring_init_seqno(struct intel_engine_cs *engine, u32 seqno)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(engine->dev);
>  
> +	/* Our semaphore implementation is strictly monotonic (i.e. we proceed
> +	 * so long as the semaphore value in the register/page is greater
> +	 * than the sync value), so whenever we reset the seqno,
> +	 * so long as we reset the tracking semaphore value to 0, it will
> +	 * always be before the next request's seqno. If we don't reset
> +	 * the semaphore value, then when the seqno moves backwards all
> +	 * future waits will complete instantly (causing rendering corruption).
> +	 */

Yes, this is better comment.

>  	if (INTEL_INFO(dev_priv)->gen == 6 || INTEL_INFO(dev_priv)->gen == 7) {
>  		I915_WRITE(RING_SYNC_0(engine->mmio_base), 0);
>  		I915_WRITE(RING_SYNC_1(engine->mmio_base), 0);
>  		if (HAS_VEBOX(dev_priv))
>  			I915_WRITE(RING_SYNC_2(engine->mmio_base), 0);
>  	}
> +	memset(engine->semaphore.sync_seqno, 0,
> +	       sizeof(engine->semaphore.sync_seqno));
>  
>  	engine->set_seqno(engine, seqno);
> +
>  	engine->hangcheck.seqno = seqno;
>  }
>  
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx