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@xxxxxxxxxxxxxxxxxx>
> >>> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> >>> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> >>> Cc: Matthew Auld <matthew.william.auld@xxxxxxxxx>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >>> ---
> >>> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
