* [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail
@ 2018-06-08 17:25 Chris Wilson
2018-06-08 18:02 ` ✓ Fi.CI.BAT: success for " Patchwork
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Chris Wilson @ 2018-06-08 17:25 UTC (permalink / raw)
To: intel-gfx
The HW only accepts offsets within ring->size, and fails peculiarly if
the RING_HEAD or RING_TAIL is set to ring->size. Therefore whenever we
set ring->head/ring->tail we want to make sure it is within value (using
intel_ring_wrap()).
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++
drivers/gpu/drm/i915/intel_ringbuffer.h | 12 ++++++++++++
2 files changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 6ac3b65373fe..9fac0e0f078e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -496,6 +496,10 @@ static int init_ring_common(struct intel_engine_cs *engine)
DRM_DEBUG_DRIVER("%s initialization failed [head=%08x], fudging\n",
engine->name, I915_READ_HEAD(engine));
+ /* Check that the ring offsets point within the ring! */
+ GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head));
+ GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail));
+
intel_ring_update_space(ring);
I915_WRITE_HEAD(engine, ring->head);
I915_WRITE_TAIL(engine, ring->tail);
@@ -1064,6 +1068,7 @@ int intel_ring_pin(struct intel_ring *ring,
void intel_ring_reset(struct intel_ring *ring, u32 tail)
{
+ tail = intel_ring_wrap(ring, tail);
ring->tail = tail;
ring->head = tail;
ring->emit = tail;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index b44c67849749..1d8140ac2016 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -805,6 +805,18 @@ static inline u32 intel_ring_wrap(const struct intel_ring *ring, u32 pos)
return pos & (ring->size - 1);
}
+static inline bool
+intel_ring_offset_valid(const struct intel_ring *ring, u32 pos)
+{
+ if (pos & -ring->size) /* must be strictly within the ring */
+ return false;
+
+ if (!IS_ALIGNED(pos, 8)) /* must be qword aligned */
+ return false;
+
+ return true;
+}
+
static inline u32 intel_ring_offset(const struct i915_request *rq, void *addr)
{
/* Don't write ring->size (equivalent to 0) as that hangs some GPUs. */
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Wrap around the tail offset before setting ring->tail
2018-06-08 17:25 [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail Chris Wilson
@ 2018-06-08 18:02 ` Patchwork
2018-06-08 18:47 ` [PATCH v2] " Chris Wilson
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-06-08 18:02 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Wrap around the tail offset before setting ring->tail
URL : https://patchwork.freedesktop.org/series/44500/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4294 -> Patchwork_9246 =
== Summary - WARNING ==
Minor unknown changes coming with Patchwork_9246 need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_9246, 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/44500/revisions/1/mbox/
== Possible new issues ==
Here are the unknown changes that may have been introduced in Patchwork_9246:
=== IGT changes ===
==== Warnings ====
igt@gem_exec_gttfill@basic:
fi-pnv-d510: SKIP -> PASS
== Known issues ==
Here are the changes found in Patchwork_9246 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@kms_chamelium@hdmi-hpd-fast:
fi-kbl-7500u: SKIP -> FAIL (fdo#103841, fdo#102672)
igt@kms_frontbuffer_tracking@basic:
fi-hsw-peppy: PASS -> DMESG-FAIL (fdo#102614, fdo#106103)
fi-hsw-4200u: PASS -> DMESG-FAIL (fdo#102614, fdo#106103)
igt@kms_pipe_crc_basic@read-crc-pipe-c:
fi-glk-j4005: PASS -> DMESG-WARN (fdo#106097)
igt@prime_vgem@basic-fence-flip:
fi-ilk-650: PASS -> FAIL (fdo#104008)
==== Possible fixes ====
igt@gem_mmap_gtt@basic-small-bo-tiledx:
fi-gdg-551: FAIL (fdo#102575) -> PASS
igt@gem_sync@basic-many-each:
fi-cnl-y3: INCOMPLETE (fdo#105086) -> PASS
igt@kms_chamelium@dp-crc-fast:
fi-kbl-7500u: DMESG-FAIL (fdo#103841) -> PASS
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
fdo#102672 https://bugs.freedesktop.org/show_bug.cgi?id=102672
fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
fdo#105086 https://bugs.freedesktop.org/show_bug.cgi?id=105086
fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103
== Participating hosts (41 -> 38) ==
Additional (1): fi-bxt-dsi
Missing (4): fi-byt-j1900 fi-ilk-m540 fi-byt-squawks fi-skl-6700hq
== Build changes ==
* Linux: CI_DRM_4294 -> Patchwork_9246
CI_DRM_4294: af0889384edc6de2f91494325d571c66dffea83f @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4512: 093fa482371795c3aa246509994eb21907f501b9 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_9246: ab408babd7e5f765e2d0fe07653bcfdd10f9a859 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
ab408babd7e5 drm/i915: Wrap around the tail offset before setting ring->tail
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9246/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] drm/i915: Wrap around the tail offset before setting ring->tail
2018-06-08 17:25 [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail Chris Wilson
2018-06-08 18:02 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-06-08 18:47 ` Chris Wilson
2018-06-08 19:29 ` ✓ Fi.CI.BAT: success for drm/i915: Wrap around the tail offset before setting ring->tail (rev2) Patchwork
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2018-06-08 18:47 UTC (permalink / raw)
To: intel-gfx
The HW only accepts offsets within ring->size, and fails peculiarly if
the RING_HEAD or RING_TAIL is set to ring->size. Therefore whenever we
set ring->head/ring->tail we want to make sure it is within value (using
intel_ring_wrap()).
v2: Double check execlists as well
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/intel_lrc.c | 6 ++++--
drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++
drivers/gpu/drm/i915/intel_ringbuffer.h | 12 ++++++++++++
3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 091e28f0e024..3e008adf5a01 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1413,6 +1413,7 @@ __execlists_context_pin(struct intel_engine_cs *engine,
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->state->obj->pin_global++;
@@ -2001,9 +2002,10 @@ static void execlists_reset(struct intel_engine_cs *engine,
/* Move the RING_HEAD onto the breadcrumb, past the hanging batch */
regs[CTX_RING_BUFFER_START + 1] = i915_ggtt_offset(request->ring->vma);
- regs[CTX_RING_HEAD + 1] = request->postfix;
- request->ring->head = request->postfix;
+ request->ring->head = intel_ring_wrap(request->ring, request->postfix);
+ regs[CTX_RING_HEAD + 1] = request->ring->head;
+
intel_ring_update_space(request->ring);
/* Reset WaIdleLiteRestore:bdw,skl as well */
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 6ac3b65373fe..9fac0e0f078e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -496,6 +496,10 @@ static int init_ring_common(struct intel_engine_cs *engine)
DRM_DEBUG_DRIVER("%s initialization failed [head=%08x], fudging\n",
engine->name, I915_READ_HEAD(engine));
+ /* Check that the ring offsets point within the ring! */
+ GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head));
+ GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail));
+
intel_ring_update_space(ring);
I915_WRITE_HEAD(engine, ring->head);
I915_WRITE_TAIL(engine, ring->tail);
@@ -1064,6 +1068,7 @@ int intel_ring_pin(struct intel_ring *ring,
void intel_ring_reset(struct intel_ring *ring, u32 tail)
{
+ tail = intel_ring_wrap(ring, tail);
ring->tail = tail;
ring->head = tail;
ring->emit = tail;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index b44c67849749..1d8140ac2016 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -805,6 +805,18 @@ static inline u32 intel_ring_wrap(const struct intel_ring *ring, u32 pos)
return pos & (ring->size - 1);
}
+static inline bool
+intel_ring_offset_valid(const struct intel_ring *ring, u32 pos)
+{
+ if (pos & -ring->size) /* must be strictly within the ring */
+ return false;
+
+ if (!IS_ALIGNED(pos, 8)) /* must be qword aligned */
+ return false;
+
+ return true;
+}
+
static inline u32 intel_ring_offset(const struct i915_request *rq, void *addr)
{
/* Don't write ring->size (equivalent to 0) as that hangs some GPUs. */
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Wrap around the tail offset before setting ring->tail (rev2)
2018-06-08 17:25 [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail Chris Wilson
2018-06-08 18:02 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-06-08 18:47 ` [PATCH v2] " Chris Wilson
@ 2018-06-08 19:29 ` Patchwork
2018-06-09 1:33 ` ✓ Fi.CI.IGT: " Patchwork
2018-06-11 8:28 ` [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail Tvrtko Ursulin
4 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-06-08 19:29 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Wrap around the tail offset before setting ring->tail (rev2)
URL : https://patchwork.freedesktop.org/series/44500/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4296 -> Patchwork_9249 =
== Summary - WARNING ==
Minor unknown changes coming with Patchwork_9249 need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_9249, 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/44500/revisions/2/mbox/
== Possible new issues ==
Here are the unknown changes that may have been introduced in Patchwork_9249:
=== IGT changes ===
==== Warnings ====
igt@gem_exec_gttfill@basic:
fi-pnv-d510: SKIP -> PASS
== Known issues ==
Here are the changes found in Patchwork_9249 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
fi-skl-guc: PASS -> FAIL (fdo#104724, fdo#103191)
igt@prime_vgem@basic-fence-flip:
fi-ilk-650: PASS -> FAIL (fdo#104008)
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
== Participating hosts (41 -> 38) ==
Missing (3): fi-ilk-m540 fi-byt-squawks fi-skl-6700hq
== Build changes ==
* Linux: CI_DRM_4296 -> Patchwork_9249
CI_DRM_4296: 3639b8f5b23b8e777ba46501b4fd257e099bd13c @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4513: 7b6838781441cfbc7f6c18f421f127dfb02b44cf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_9249: f6a0dfb7aab764d47f615642bfd34dd436f85439 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
f6a0dfb7aab7 drm/i915: Wrap around the tail offset before setting ring->tail
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9249/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Wrap around the tail offset before setting ring->tail (rev2)
2018-06-08 17:25 [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail Chris Wilson
` (2 preceding siblings ...)
2018-06-08 19:29 ` ✓ Fi.CI.BAT: success for drm/i915: Wrap around the tail offset before setting ring->tail (rev2) Patchwork
@ 2018-06-09 1:33 ` Patchwork
2018-06-11 8:28 ` [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail Tvrtko Ursulin
4 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-06-09 1:33 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Wrap around the tail offset before setting ring->tail (rev2)
URL : https://patchwork.freedesktop.org/series/44500/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4296_full -> Patchwork_9249_full =
== Summary - WARNING ==
Minor unknown changes coming with Patchwork_9249_full need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_9249_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
== Possible new issues ==
Here are the unknown changes that may have been introduced in Patchwork_9249_full:
=== IGT changes ===
==== Warnings ====
igt@gem_mocs_settings@mocs-rc6-vebox:
shard-kbl: SKIP -> PASS +2
igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-xtiled:
shard-snb: SKIP -> PASS
igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-blt:
shard-snb: PASS -> SKIP +4
== Known issues ==
Here are the changes found in Patchwork_9249_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@drv_selftest@live_gtt:
shard-kbl: PASS -> FAIL (fdo#105347)
shard-glk: PASS -> INCOMPLETE (fdo#103359, k.org#198133) +1
igt@gem_ctx_switch@basic-all-light:
shard-hsw: PASS -> INCOMPLETE (fdo#103540)
igt@gem_eio@hibernate:
shard-snb: PASS -> INCOMPLETE (fdo#105411)
igt@kms_atomic_transition@1x-modeset-transitions-nonblocking:
shard-glk: PASS -> FAIL (fdo#105703)
igt@kms_flip@flip-vs-expired-vblank-interruptible:
shard-glk: PASS -> FAIL (fdo#105363, fdo#102887)
igt@kms_flip_tiling@flip-x-tiled:
shard-glk: PASS -> FAIL (fdo#104724, fdo#103822) +1
igt@kms_flip_tiling@flip-y-tiled:
shard-glk: PASS -> FAIL (fdo#104724)
igt@kms_setmode@basic:
shard-hsw: PASS -> FAIL (fdo#99912)
==== Possible fixes ====
igt@drv_selftest@live_hangcheck:
shard-kbl: DMESG-FAIL (fdo#106560) -> PASS
igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
shard-hsw: FAIL (fdo#105767) -> PASS
igt@kms_flip@2x-flip-vs-blocking-wf-vblank:
shard-glk: FAIL (fdo#100368) -> PASS
igt@kms_flip@flip-vs-expired-vblank:
shard-glk: FAIL (fdo#105189) -> PASS
igt@kms_setmode@basic:
shard-apl: FAIL (fdo#99912) -> PASS
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
fdo#105189 https://bugs.freedesktop.org/show_bug.cgi?id=105189
fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
fdo#105703 https://bugs.freedesktop.org/show_bug.cgi?id=105703
fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133
== Participating hosts (5 -> 5) ==
No changes in participating hosts
== Build changes ==
* Linux: CI_DRM_4296 -> Patchwork_9249
CI_DRM_4296: 3639b8f5b23b8e777ba46501b4fd257e099bd13c @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4513: 7b6838781441cfbc7f6c18f421f127dfb02b44cf @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_9249: f6a0dfb7aab764d47f615642bfd34dd436f85439 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9249/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail
2018-06-08 17:25 [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail Chris Wilson
` (3 preceding siblings ...)
2018-06-09 1:33 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-06-11 8:28 ` Tvrtko Ursulin
2018-06-11 8:32 ` Chris Wilson
4 siblings, 1 reply; 9+ messages in thread
From: Tvrtko Ursulin @ 2018-06-11 8:28 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 08/06/2018 18:25, Chris Wilson wrote:
> The HW only accepts offsets within ring->size, and fails peculiarly if
> the RING_HEAD or RING_TAIL is set to ring->size. Therefore whenever we
> set ring->head/ring->tail we want to make sure it is within value (using
> intel_ring_wrap()).
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++
> drivers/gpu/drm/i915/intel_ringbuffer.h | 12 ++++++++++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 6ac3b65373fe..9fac0e0f078e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -496,6 +496,10 @@ static int init_ring_common(struct intel_engine_cs *engine)
> DRM_DEBUG_DRIVER("%s initialization failed [head=%08x], fudging\n",
> engine->name, I915_READ_HEAD(engine));
>
> + /* Check that the ring offsets point within the ring! */
> + GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head));
> + GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail));
> +
> intel_ring_update_space(ring);
> I915_WRITE_HEAD(engine, ring->head);
> I915_WRITE_TAIL(engine, ring->tail);
> @@ -1064,6 +1068,7 @@ int intel_ring_pin(struct intel_ring *ring,
>
> void intel_ring_reset(struct intel_ring *ring, u32 tail)
> {
> + tail = intel_ring_wrap(ring, tail);
> ring->tail = tail;
> ring->head = tail;
> ring->emit = tail;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index b44c67849749..1d8140ac2016 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -805,6 +805,18 @@ static inline u32 intel_ring_wrap(const struct intel_ring *ring, u32 pos)
> return pos & (ring->size - 1);
> }
>
> +static inline bool
> +intel_ring_offset_valid(const struct intel_ring *ring, u32 pos)
> +{
> + if (pos & -ring->size) /* must be strictly within the ring */
> + return false;
> +
> + if (!IS_ALIGNED(pos, 8)) /* must be qword aligned */
> + return false;
> +
> + return true;
> +}
Looks like we have assert_ring_tail_valid and intel_ring_set_tail
already in the tree. So could just use the latter in intel_ring_reset if
needed. But also since intel_ring_reset is only setting the tail to
either zero or existing ring->tail it sounds like the check would be
better placed where the tail advances?
Regards,
Tvrtko
> +
> static inline u32 intel_ring_offset(const struct i915_request *rq, void *addr)
> {
> /* Don't write ring->size (equivalent to 0) as that hangs some GPUs. */
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail
2018-06-11 8:28 ` [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail Tvrtko Ursulin
@ 2018-06-11 8:32 ` Chris Wilson
2018-06-11 8:49 ` Tvrtko Ursulin
0 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2018-06-11 8:32 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2018-06-11 09:28:22)
>
> On 08/06/2018 18:25, Chris Wilson wrote:
> > The HW only accepts offsets within ring->size, and fails peculiarly if
> > the RING_HEAD or RING_TAIL is set to ring->size. Therefore whenever we
> > set ring->head/ring->tail we want to make sure it is within value (using
> > intel_ring_wrap()).
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Matthew Auld <matthew.william.auld@gmail.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++
> > drivers/gpu/drm/i915/intel_ringbuffer.h | 12 ++++++++++++
> > 2 files changed, 17 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 6ac3b65373fe..9fac0e0f078e 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -496,6 +496,10 @@ static int init_ring_common(struct intel_engine_cs *engine)
> > DRM_DEBUG_DRIVER("%s initialization failed [head=%08x], fudging\n",
> > engine->name, I915_READ_HEAD(engine));
> >
> > + /* Check that the ring offsets point within the ring! */
> > + GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head));
> > + GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail));
> > +
> > intel_ring_update_space(ring);
> > I915_WRITE_HEAD(engine, ring->head);
> > I915_WRITE_TAIL(engine, ring->tail);
> > @@ -1064,6 +1068,7 @@ int intel_ring_pin(struct intel_ring *ring,
> >
> > void intel_ring_reset(struct intel_ring *ring, u32 tail)
> > {
> > + tail = intel_ring_wrap(ring, tail);
> > ring->tail = tail;
> > ring->head = tail;
> > ring->emit = tail;
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > index b44c67849749..1d8140ac2016 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > @@ -805,6 +805,18 @@ static inline u32 intel_ring_wrap(const struct intel_ring *ring, u32 pos)
> > return pos & (ring->size - 1);
> > }
> >
> > +static inline bool
> > +intel_ring_offset_valid(const struct intel_ring *ring, u32 pos)
> > +{
> > + if (pos & -ring->size) /* must be strictly within the ring */
> > + return false;
> > +
> > + if (!IS_ALIGNED(pos, 8)) /* must be qword aligned */
> > + return false;
> > +
> > + return true;
> > +}
>
> Looks like we have assert_ring_tail_valid and intel_ring_set_tail
> already in the tree. So could just use the latter in intel_ring_reset if
> needed.
No, because that is only setting the tail. The problem also lies in that
we set RING_HEAD and so need to be careful about the value we assign to
ring->head.
> But also since intel_ring_reset is only setting the tail to
> either zero or existing ring->tail it sounds like the check would be
> better placed where the tail advances?
Which we do. The problem is not the tail...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail
2018-06-11 8:32 ` Chris Wilson
@ 2018-06-11 8:49 ` Tvrtko Ursulin
2018-06-11 8:54 ` Chris Wilson
0 siblings, 1 reply; 9+ messages in thread
From: Tvrtko Ursulin @ 2018-06-11 8:49 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 11/06/2018 09:32, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-06-11 09:28:22)
>>
>> On 08/06/2018 18:25, Chris Wilson wrote:
>>> The HW only accepts offsets within ring->size, and fails peculiarly if
>>> the RING_HEAD or RING_TAIL is set to ring->size. Therefore whenever we
>>> set ring->head/ring->tail we want to make sure it is within value (using
>>> intel_ring_wrap()).
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>>> Cc: Matthew Auld <matthew.william.auld@gmail.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++
>>> drivers/gpu/drm/i915/intel_ringbuffer.h | 12 ++++++++++++
>>> 2 files changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> index 6ac3b65373fe..9fac0e0f078e 100644
>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> @@ -496,6 +496,10 @@ static int init_ring_common(struct intel_engine_cs *engine)
>>> DRM_DEBUG_DRIVER("%s initialization failed [head=%08x], fudging\n",
>>> engine->name, I915_READ_HEAD(engine));
>>>
>>> + /* Check that the ring offsets point within the ring! */
>>> + GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head));
>>> + GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail));
>>> +
>>> intel_ring_update_space(ring);
>>> I915_WRITE_HEAD(engine, ring->head);
>>> I915_WRITE_TAIL(engine, ring->tail);
>>> @@ -1064,6 +1068,7 @@ int intel_ring_pin(struct intel_ring *ring,
>>>
>>> void intel_ring_reset(struct intel_ring *ring, u32 tail)
>>> {
>>> + tail = intel_ring_wrap(ring, tail);
>>> ring->tail = tail;
>>> ring->head = tail;
>>> ring->emit = tail;
>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
>>> index b44c67849749..1d8140ac2016 100644
>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>>> @@ -805,6 +805,18 @@ static inline u32 intel_ring_wrap(const struct intel_ring *ring, u32 pos)
>>> return pos & (ring->size - 1);
>>> }
>>>
>>> +static inline bool
>>> +intel_ring_offset_valid(const struct intel_ring *ring, u32 pos)
>>> +{
>>> + if (pos & -ring->size) /* must be strictly within the ring */
>>> + return false;
>>> +
>>> + if (!IS_ALIGNED(pos, 8)) /* must be qword aligned */
>>> + return false;
>>> +
>>> + return true;
>>> +}
>>
>> Looks like we have assert_ring_tail_valid and intel_ring_set_tail
>> already in the tree. So could just use the latter in intel_ring_reset if
>> needed.
>
> No, because that is only setting the tail. The problem also lies in that
> we set RING_HEAD and so need to be careful about the value we assign to
> ring->head.
>
>> But also since intel_ring_reset is only setting the tail to
>> either zero or existing ring->tail it sounds like the check would be
>> better placed where the tail advances?
>
> Which we do. The problem is not the tail...
Silly me. Can you just make use of intel_ring_offset_valid from
assert_ring_tail_valid so we don't duplicate the checks?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail
2018-06-11 8:49 ` Tvrtko Ursulin
@ 2018-06-11 8:54 ` Chris Wilson
0 siblings, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2018-06-11 8:54 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2018-06-11 09:49:59)
>
> On 11/06/2018 09:32, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-06-11 09:28:22)
> >>
> >> On 08/06/2018 18:25, Chris Wilson wrote:
> >>> The HW only accepts offsets within ring->size, and fails peculiarly if
> >>> the RING_HEAD or RING_TAIL is set to ring->size. Therefore whenever we
> >>> set ring->head/ring->tail we want to make sure it is within value (using
> >>> intel_ring_wrap()).
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >>> Cc: Matthew Auld <matthew.william.auld@gmail.com>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> ---
> >>> drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++
> >>> drivers/gpu/drm/i915/intel_ringbuffer.h | 12 ++++++++++++
> >>> 2 files changed, 17 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>> index 6ac3b65373fe..9fac0e0f078e 100644
> >>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>> @@ -496,6 +496,10 @@ static int init_ring_common(struct intel_engine_cs *engine)
> >>> DRM_DEBUG_DRIVER("%s initialization failed [head=%08x], fudging\n",
> >>> engine->name, I915_READ_HEAD(engine));
> >>>
> >>> + /* Check that the ring offsets point within the ring! */
> >>> + GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head));
> >>> + GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail));
> >>> +
> >>> intel_ring_update_space(ring);
> >>> I915_WRITE_HEAD(engine, ring->head);
> >>> I915_WRITE_TAIL(engine, ring->tail);
> >>> @@ -1064,6 +1068,7 @@ int intel_ring_pin(struct intel_ring *ring,
> >>>
> >>> void intel_ring_reset(struct intel_ring *ring, u32 tail)
> >>> {
> >>> + tail = intel_ring_wrap(ring, tail);
> >>> ring->tail = tail;
> >>> ring->head = tail;
> >>> ring->emit = tail;
> >>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>> index b44c67849749..1d8140ac2016 100644
> >>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>> @@ -805,6 +805,18 @@ static inline u32 intel_ring_wrap(const struct intel_ring *ring, u32 pos)
> >>> return pos & (ring->size - 1);
> >>> }
> >>>
> >>> +static inline bool
> >>> +intel_ring_offset_valid(const struct intel_ring *ring, u32 pos)
> >>> +{
> >>> + if (pos & -ring->size) /* must be strictly within the ring */
> >>> + return false;
> >>> +
> >>> + if (!IS_ALIGNED(pos, 8)) /* must be qword aligned */
> >>> + return false;
> >>> +
> >>> + return true;
> >>> +}
> >>
> >> Looks like we have assert_ring_tail_valid and intel_ring_set_tail
> >> already in the tree. So could just use the latter in intel_ring_reset if
> >> needed.
> >
> > No, because that is only setting the tail. The problem also lies in that
> > we set RING_HEAD and so need to be careful about the value we assign to
> > ring->head.
> >
> >> But also since intel_ring_reset is only setting the tail to
> >> either zero or existing ring->tail it sounds like the check would be
> >> better placed where the tail advances?
> >
> > Which we do. The problem is not the tail...
>
> Silly me. Can you just make use of intel_ring_offset_valid from
> assert_ring_tail_valid so we don't duplicate the checks?
Done. Fancy digging through the first patch to remove all the mmio from
gen6/gen7 reset? Pretty please? :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-06-11 8:54 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-08 17:25 [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail Chris Wilson
2018-06-08 18:02 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-06-08 18:47 ` [PATCH v2] " Chris Wilson
2018-06-08 19:29 ` ✓ Fi.CI.BAT: success for drm/i915: Wrap around the tail offset before setting ring->tail (rev2) Patchwork
2018-06-09 1:33 ` ✓ Fi.CI.IGT: " Patchwork
2018-06-11 8:28 ` [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail Tvrtko Ursulin
2018-06-11 8:32 ` Chris Wilson
2018-06-11 8:49 ` Tvrtko Ursulin
2018-06-11 8:54 ` Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.