| Message ID | 20180608172521.21417-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. */ >
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
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
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
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. */
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(+)