Chris Wilson <[email protected]> writes: > We only have to prevent the RING_TAIL from catching the RING_HEAD > cacheline and do not need to enforce a whole cacheline separation, and in > the process we can remove one branch from the computation. > > Signed-off-by: Chris Wilson <[email protected]> > --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 20 ++++++++++---------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 24 +++++++++++++----------- > 2 files changed, 23 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 1ec98851a621..48d4b5b24b21 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -39,12 +39,15 @@ > */ > #define LEGACY_REQUEST_SIZE 200 > > -static int __intel_ring_space(int head, int tail, int size) > +static unsigned int __intel_ring_space(unsigned int head, > + unsigned int tail, > + unsigned int size) > { > - int space = head - tail; > - if (space <= 0) > - space += size; > - return space - I915_RING_FREE_SPACE; > + /* "If the Ring Buffer Head Pointer and the Tail Pointer are on the > + * same cacheline, the Head Pointer must not be greater than the Tail > + * Pointer." > + */ > + return (round_down(head, CACHELINE_BYTES) - tail - 8) & (size - 1); > } > > void intel_ring_update_space(struct intel_ring *ring) > @@ -1618,12 +1621,9 @@ static int wait_for_space(struct drm_i915_gem_request > *req, int bytes) > GEM_BUG_ON(!req->reserved_space); > > list_for_each_entry(target, &ring->request_list, ring_link) { > - unsigned space; > - > /* Would completion of this request free enough space? */ > - space = __intel_ring_space(target->postfix, ring->emit, > - ring->size); > - if (space >= bytes) > + if (bytes <= __intel_ring_space(target->postfix, > + ring->emit, ring->size)) > break; > } > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h > b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 31998ea46fb8..e28ffc98c520 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -17,17 +17,6 @@ > #define CACHELINE_BYTES 64 > #define CACHELINE_DWORDS (CACHELINE_BYTES / sizeof(uint32_t)) > > -/* > - * Gen2 BSpec "1. Programming Environment" / 1.4.4.6 "Ring Buffer Use" > - * Gen3 BSpec "vol1c Memory Interface Functions" / 2.3.4.5 "Ring Buffer Use" > - * Gen4+ BSpec "vol1c Memory Interface and Command Stream" / 5.3.4.5 "Ring > Buffer Use" > - * > - * "If the Ring Buffer Head Pointer and the Tail Pointer are on the same > - * cacheline, the Head Pointer must not be greater than the Tail > - * Pointer." > - */ > -#define I915_RING_FREE_SPACE 64 > - > struct intel_hw_status_page { > struct i915_vma *vma; > u32 *page_addr; > @@ -545,6 +534,19 @@ assert_ring_tail_valid(const struct intel_ring *ring, > unsigned int tail) > */ > GEM_BUG_ON(!IS_ALIGNED(tail, 8)); > GEM_BUG_ON(tail >= ring->size); > + > + /* "Ring Buffer Use" > + * Gen2 BSpec "1. Programming Environment" / 1.4.4.6 > + * Gen3 BSpec "1c Memory Interface Functions" / 2.3.4.5 > + * Gen4+ BSpec "1c Memory Interface and Command Stream" / 5.3.4.5 > + * "If the Ring Buffer Head Pointer and the Tail Pointer are on the > + * same cacheline, the Head Pointer must not be greater than the Tail > + * Pointer." > + */ > +#define cacheline(a) round_down(a, CACHELINE_BYTES) > + GEM_BUG_ON(cacheline(tail) == cacheline(ring->head) &&
We are about to set the hw tail. But we are using our outdated bookkeepping value of ring head to do the check. I am confused. Is this something that can happen only when it wraps. And why we dont need to check the HW head? -Mika > + tail < ring->head); > +#undef cacheline > } > > static inline unsigned int > -- > 2.11.0 > > _______________________________________________ > Intel-gfx mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
