Re: [PATCH 3/3] drm/i915: Track & check calls to intel(_logical)_ring_{begin, advance}

[Dave Gordon <david.s.gordon@intel.com>]

On 12/12/14 17:12, Chris Wilson wrote:
> On Fri, Dec 12, 2014 at 04:13:03PM +0000, Dave Gordon wrote:
>>  static inline void intel_ring_advance(struct intel_engine_cs *ring)
>>  {
>>  	struct intel_ringbuffer *ringbuf = ring->buffer;
>> -	ringbuf->tail &= ringbuf->size - 1;
>> +
>> +	__intel_ringbuffer_check(ringbuf);
>> +
>> +	/*
>> +	 * Tail == effecive_size is legitimate (buffer exactly full).
>> +	 * Tail > effective_size is not, and should give a warning,
>> +	 * but we'll reset tail in both cases to prevent further chaos
>> +	 */
>> +	if (ringbuf->tail >= ringbuf->effective_size)
>> +		ringbuf->tail -= ringbuf->effective_size;
> 
> Urm. No. If you never write into the reserved pair of cachelines at the
> end of the ringbuffer but the hw reads garbage from it, you lose.

This won't happen, because (with the first patch of this set applied)
the check for sufficient space uses effective_size, but the wrap code
uses the actual size. Thus, once the next chunk won't fit in the
available space up to the effective_size, we write NOPs all over
whatever is left up to the total size. The part between the effective
and the actual sizes is therefore always (and only) written with NOPs.

> tail &= size - 1; is a nice description of how the hw works that is
> suitable for inlining, with all the magic in begin().
> 
> The goal is to remove the duplicated logic from intel_lrc.c, use
> requests completely, and remove the dri1 hangover. To repeat what I said
> last time:
> http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/intel_ringbuffer.c?h=requests#n2447
> is where I want us to go, a single piece of logic for ring management.
> -Chris

I agree with the deduplication, which is why nearly all these changes
are in functions operating on ringbuffers (not rings), and located in
intel_ringbuffer.h, with only minimal changes in the execlist and legacy
paths to use these common functions rather than having two implementations.

So, what I think I'll do is rework this third patch to merge the
tail-masking into the common function; rename it to something other than
/check/; and then the legacy and LRC versions become trivial wrappers
round this.

Are you happy with the first two patches?

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx