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

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx