| Message ID | 20180610194325.13467-5-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Chris Wilson <chris@chris-wilson.co.uk> writes: > 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 409f499c0a45..7970ecb199e2 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); I am pondering this wrap here and it's usefulness. Where could we ever get a tail which is not valid? From corrupted context? -Mika > 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
Quoting Mika Kuoppala (2018-06-11 11:16:14) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > > index 409f499c0a45..7970ecb199e2 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); > > I am pondering this wrap here and it's usefulness. Where > could we ever get a tail which is not valid? From corrupted > context? It's just being defensive. We could make this GEM_BUG_ON(!offset_valid()) instead? Your choice? -Chris
Quoting Chris Wilson (2018-06-10 22:43:12) > 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> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas
Chris Wilson <chris@chris-wilson.co.uk> writes: > Quoting Mika Kuoppala (2018-06-11 11:16:14) >> Chris Wilson <chris@chris-wilson.co.uk> writes: >> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c >> > index 409f499c0a45..7970ecb199e2 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); >> >> I am pondering this wrap here and it's usefulness. Where >> could we ever get a tail which is not valid? From corrupted >> context? > > It's just being defensive. We could make this > GEM_BUG_ON(!offset_valid()) instead? > > Your choice? That would be best to assert it. For fancyness, you could do a ring_offset_valid check from assert_ring_tail_valid as it is a subset and share some of the checks. -Mika
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 409f499c0a45..7970ecb199e2 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. */
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(-)