tail

Tvrtko Ursulin Mon, 11 Jun 2018 01:50:25 -0700


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 <[email protected]>
Cc: Joonas Lahtinen <[email protected]>
Cc: Mika Kuoppala <[email protected]>
Cc: Matthew Auld <[email protected]>
Cc: Tvrtko Ursulin <[email protected]>
---
   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 fromassert_ring_tail_valid so we don't duplicate the checks?


Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Wrap around the tail offset before setting ring->tail

Reply via email to