| Message ID | 1457688402-10411-1-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 11, 2016 at 02:56:42PM +0530, akash.goel@intel.com wrote: > From: Akash Goel <akash.goel@intel.com> > > Currently for the case where there is enough space at the end of Ring > buffer for accommodating only the base request, the wrapround is done > immediately and as a result the base request gets added at the start > of Ring buffer. But there may not be enough free space at the beginning > to accommodate the base request, as before the wraparound, the wait was > effectively done for the reserved_size free space from the start of > Ring buffer. In such a case there is a potential of Ring buffer overflow, > the instructions at the head of Ring (ACTHD) can get overwritten. > > Since the base request can fit in the remaining space, there is no need > to wraparound immediately. The wraparound will anyway happen later when > the reserved part starts getting used. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Akash Goel <akash.goel@intel.com> From an earlier review: > The benefit (other than that bug fix which could affect ilk in extreme > circumstances, though I guess the timing is a little too short for > igt/gem_ringfill to hit it - we can fill the ring with ease, but we would > have to overwrite after wrap whilst the ACTHD was still in the zone in > order for it to actualy fallover. Hmm, if we added a BUG for tail overflow > that might have a chance of detecting the issue) should be that we > squeeze in perhaps even another whole request before the ring is > exhausted and so reserved-space is less wasteful on gen that do not need > the full reservation. > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > In theory this could be impacting upon Ironlakes in the real-world, so a > candidate for stable@ -Chris
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index f60d5eb..eae882b 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -853,11 +853,11 @@ static int logical_ring_prepare(struct drm_i915_gem_request *req, int bytes) if (unlikely(total_bytes > remain_usable)) { /* * The base request will fit but the reserved space - * falls off the end. So only need to to wait for the - * reserved size after flushing out the remainder. + * falls off the end. So don't need an immediate wrap + * and only need to effectively wait for the reserved + * size space from the start of ringbuffer. */ wait_bytes = remain_actual + ringbuf->reserved_size; - need_wrap = true; } else if (total_bytes > ringbuf->space) { /* No wrapping required, just waiting. */ wait_bytes = total_bytes; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 45ce45a..c2ea2cd 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2454,11 +2454,11 @@ static int __intel_ring_prepare(struct intel_engine_cs *ring, int bytes) if (unlikely(total_bytes > remain_usable)) { /* * The base request will fit but the reserved space - * falls off the end. So only need to to wait for the - * reserved size after flushing out the remainder. + * falls off the end. So don't need an immediate wrap + * and only need to effectively wait for the reserved + * size space from the start of ringbuffer. */ wait_bytes = remain_actual + ringbuf->reserved_size; - need_wrap = true; } else if (total_bytes > ringbuf->space) { /* No wrapping required, just waiting. */ wait_bytes = total_bytes;