* [PATCH 0/2] Track calls to intel(_logical)_ring_{begin, advance}
@ 2014-12-10 15:07 Dave Gordon
2014-12-10 15:07 ` [PATCH 1/2] drm/i915: Track & check " Dave Gordon
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Dave Gordon @ 2014-12-10 15:07 UTC (permalink / raw)
To: intel-gfx
When adding instructions to a legacy or LRC ringbuffer, the sequence of
emit() calls must be preceded by a call to intel(_logical)_ring_begin()
to reserve the required amount of space, and followed by a matching call
to intel(_logical)_ring_advance(). Historically some (display) code
didn't use begin/advance, but just inserted instructions ad hoc, which
would then be sent to the hardware along with the current or next batch,
but this is not supported and is now regarded as incorrect.
This commit therefore adds begin/advance tracking, with WARNings where
various forms of misuse are detected.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/2] drm/i915: Track & check calls to intel(_logical)_ring_{begin, advance} 2014-12-10 15:07 [PATCH 0/2] Track calls to intel(_logical)_ring_{begin, advance} Dave Gordon @ 2014-12-10 15:07 ` Dave Gordon 2014-12-10 15:44 ` Daniel Vetter 2014-12-10 15:07 ` [PATCH 2/2] drm/i915: Track nested " Dave Gordon ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: Dave Gordon @ 2014-12-10 15:07 UTC (permalink / raw) To: intel-gfx When adding instructions to a legacy or LRC ringbuffer, the sequence of emit() calls must be preceded by a call to intel(_logical)_ring_begin() to reserve the required amount of space, and followed by a matching call to intel(_logical)_ring_advance() (note that this used to trigger immediate submission to the h/w, but now actual submission is deferred until all the instructions for a single batch submission have been assembled). Historically some (display) code didn't use begin/advance, but just inserted instructions ad hoc, which would then be sent to the hardware along with the current or next batch, but this is not supported and is now regarded as incorrect. This commit therefore adds begin/advance tracking, with WARNings where various forms of misuse are detected. These include: * advance without begin * begin without advance before submission to h/w * multiple begins without an advance between * exceeding the space reserved by begin * leaving the ring misaligned * ring buffer overrun (negative freespace) Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/intel_lrc.c | 4 ++- drivers/gpu/drm/i915/intel_lrc.h | 11 ++++++- drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++-- drivers/gpu/drm/i915/intel_ringbuffer.h | 55 ++++++++++++++++++++++++++++++- 4 files changed, 75 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index a82020e..56e3636 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -825,6 +825,7 @@ void intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf) struct intel_context *ctx = ringbuf->FIXME_lrc_ctx; intel_logical_ring_advance(ringbuf); + WARN_ON(ringbuf->rsv_level != 0); if (intel_ring_stopped(ring)) return; @@ -1084,7 +1085,8 @@ int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf, int num_dwords) if (ret) return ret; - ringbuf->space -= num_dwords * sizeof(uint32_t); + __intel_ringbuffer_begin(ringbuf, num_dwords); + return 0; } diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h index 14b216b..9a0457e 100644 --- a/drivers/gpu/drm/i915/intel_lrc.h +++ b/drivers/gpu/drm/i915/intel_lrc.h @@ -48,8 +48,17 @@ void intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf); */ static inline void intel_logical_ring_advance(struct intel_ringbuffer *ringbuf) { - 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; } + /** * intel_logical_ring_emit() - write a DWORD to the ringbuffer. * @ringbuf: Ringbuffer to write to. diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 83accb7..5874eab 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -55,6 +55,7 @@ int __intel_ring_space(int head, int tail, int size) int space = head - tail; if (space <= 0) space += size; + WARN_ON(space < I915_RING_FREE_SPACE); return space - I915_RING_FREE_SPACE; } @@ -84,7 +85,10 @@ bool intel_ring_stopped(struct intel_engine_cs *ring) void __intel_ring_advance(struct intel_engine_cs *ring) { struct intel_ringbuffer *ringbuf = ring->buffer; - ringbuf->tail &= ringbuf->size - 1; + + intel_ring_advance(ring); + WARN_ON(ringbuf->rsv_level != 0); + if (intel_ring_stopped(ring)) return; ring->write_tail(ring, ringbuf->tail); @@ -1911,6 +1915,7 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n) return 0; list_for_each_entry(request, &ring->request_list, list) { + /* Would completion of this request free enough space? */ if (__intel_ring_space(request->tail, ringbuf->tail, ringbuf->size) >= n) { break; @@ -2096,7 +2101,8 @@ int intel_ring_begin(struct intel_engine_cs *ring, if (ret) return ret; - ring->buffer->space -= num_dwords * sizeof(uint32_t); + __intel_ringbuffer_begin(ring->buffer, num_dwords); + return 0; } diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 6dbb6f4..a10d271 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -112,6 +112,11 @@ struct intel_ringbuffer { int size; int effective_size; + /* these let advance() check for misuse */ + int rsv_level; /* reservation nesting level */ + int rsv_size; /* size passed to begin() */ + int rsv_start; /* tail when begin() last returned */ + /** We track the position of the requests in the ring buffer, and * when each is retired we increment last_retired_head as the GPU * must have finished processing the request and so we know we @@ -401,11 +406,59 @@ static inline void intel_ring_emit(struct intel_engine_cs *ring, iowrite32(data, ringbuf->virtual_start + ringbuf->tail); ringbuf->tail += 4; } + +static inline void __intel_ringbuffer_begin(struct intel_ringbuffer *ringbuf, + int num_dwords) +{ + int nbytes = num_dwords * sizeof(uint32_t); +#if 1 /* DEBUG CODE */ + WARN_ON(num_dwords & 1); + WARN_ON(nbytes <= 0); + + if (ringbuf->rsv_level++) { + /* begin() called twice or more without advance() */ + WARN_ON(1); + } else { + /* + * A new reservation; validate and record the start and + * size, then deduct the size from the remaining space + */ + WARN_ON(ringbuf->tail & 7); + WARN_ON(ringbuf->tail > ringbuf->effective_size-8); + WARN_ON(ringbuf->tail + nbytes > ringbuf->effective_size); + WARN_ON(ringbuf->space < nbytes); + } +#endif + ringbuf->rsv_start = ringbuf->tail; + ringbuf->rsv_size = nbytes; + ringbuf->space -= nbytes; +} + +static inline void __intel_ringbuffer_check(struct intel_ringbuffer *ringbuf) +{ +#if 1 + WARN_ON(ringbuf->rsv_level-- != 1); + WARN_ON(ringbuf->rsv_start < 0 || ringbuf->rsv_size < 0); + WARN_ON(ringbuf->tail & 7); + WARN_ON(ringbuf->tail > ringbuf->rsv_start + ringbuf->rsv_size); + WARN_ON(ringbuf->tail > ringbuf->effective_size); +#endif +} 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; } + int __intel_ring_space(int head, int tail, int size); void intel_ring_update_space(struct intel_ringbuffer *ringbuf); int intel_ring_space(struct intel_ringbuffer *ringbuf); -- 1.7.9.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: Track & check calls to intel(_logical)_ring_{begin, advance} 2014-12-10 15:07 ` [PATCH 1/2] drm/i915: Track & check " Dave Gordon @ 2014-12-10 15:44 ` Daniel Vetter 0 siblings, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2014-12-10 15:44 UTC (permalink / raw) To: Dave Gordon; +Cc: intel-gfx On Wed, Dec 10, 2014 at 03:07:08PM +0000, Dave Gordon wrote: > @@ -401,11 +406,59 @@ static inline void intel_ring_emit(struct intel_engine_cs *ring, > iowrite32(data, ringbuf->virtual_start + ringbuf->tail); > ringbuf->tail += 4; > } > + > +static inline void __intel_ringbuffer_begin(struct intel_ringbuffer *ringbuf, > + int num_dwords) > +{ > + int nbytes = num_dwords * sizeof(uint32_t); > +#if 1 /* DEBUG CODE */ No #ifdef's in code please, they tend to be either just go stale or just obfuscate the code. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] drm/i915: Track nested calls to intel(_logical)_ring_{begin, advance} 2014-12-10 15:07 [PATCH 0/2] Track calls to intel(_logical)_ring_{begin, advance} Dave Gordon 2014-12-10 15:07 ` [PATCH 1/2] drm/i915: Track & check " Dave Gordon @ 2014-12-10 15:07 ` Dave Gordon 2014-12-10 22:46 ` shuang.he 2014-12-10 16:12 ` [PATCH v2 0/2] Track " Dave Gordon 2014-12-10 16:23 ` [PATCH 0/2] Track " Chris Wilson 3 siblings, 1 reply; 10+ messages in thread From: Dave Gordon @ 2014-12-10 15:07 UTC (permalink / raw) To: intel-gfx With the current deferred-submission model, if a problem arises part-way through the insertion of instructions into the ringbuffer (e.g. due to one of the begin() calls finding there's not enough space), we avoid sending the incomplete sequence to the h/w; but currently have no means of undoing the work so far, which will lead to undefined behaviour when the next batch is submitted (probably TDR will trigger a reset first, though, and clean up the ring state). A future idea is to move to an atomic-submission model, where all the space required for a batch submission is reserved up front, and in the event of failure partway through, the work can be abandoned without side-effects. This will be required for the forthcoming GPU scheduler (specifically, for preemption). To support this, we allow nested begin/advance pairs. Specifically, the outermost pair defines the total space reservation; inner pairs can be nested ad lib, but all inner reservations at any level must fit entirely within the outermost one. Thus, this is permitted: begin(128) - guarantees that up to 128 dwords can now be emitted without waiting for more freespace begin(6) advance begin(10) advance begin(8) advance etc, as long as the total is no more than 128 dwords advance-and-submit The execbuffer code will later be enhanced to use this approach. In the mean time, the traditional single-level begin/advance mechanism remains fully supported. This commit changes only the begin/advance checking code, to permit (but not require) nested begin/advance pairs. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/intel_ringbuffer.h | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index a10d271..71cb3ef 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -416,8 +416,17 @@ static inline void __intel_ringbuffer_begin(struct intel_ringbuffer *ringbuf, WARN_ON(nbytes <= 0); if (ringbuf->rsv_level++) { - /* begin() called twice or more without advance() */ - WARN_ON(1); + /* + * A nested reservation; check that it falls entirely + * within the outer block. Don't adjust remaining space. + */ + WARN_ON(ringbuf->rsv_start < 0); + WARN_ON(ringbuf->rsv_start & 7); + WARN_ON(ringbuf->tail & 7); + WARN_ON(ringbuf->tail > ringbuf->effective_size); + WARN_ON(ringbuf->tail > ringbuf->rsv_start + ringbuf->rsv_size); + WARN_ON(ringbuf->tail + nbytes > ringbuf->effective_size); + WARN_ON(ringbuf->tail + nbytes > ringbuf->rsv_start + ringbuf->rsv_size); } else { /* * A new reservation; validate and record the start and @@ -437,7 +446,7 @@ static inline void __intel_ringbuffer_begin(struct intel_ringbuffer *ringbuf, static inline void __intel_ringbuffer_check(struct intel_ringbuffer *ringbuf) { #if 1 - WARN_ON(ringbuf->rsv_level-- != 1); + WARN_ON(ringbuf->rsv_level-- <= 0); WARN_ON(ringbuf->rsv_start < 0 || ringbuf->rsv_size < 0); WARN_ON(ringbuf->tail & 7); WARN_ON(ringbuf->tail > ringbuf->rsv_start + ringbuf->rsv_size); -- 1.7.9.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/i915: Track nested calls to intel(_logical)_ring_{begin, advance} 2014-12-10 15:07 ` [PATCH 2/2] drm/i915: Track nested " Dave Gordon @ 2014-12-10 22:46 ` shuang.he 0 siblings, 0 replies; 10+ messages in thread From: shuang.he @ 2014-12-10 22:46 UTC (permalink / raw) To: shuang.he, intel-gfx, david.s.gordon Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com) -------------------------------------Summary------------------------------------- Platform Delta drm-intel-nightly Series Applied PNV -2 364/364 362/364 ILK -11 364/366 353/366 SNB -13 448/450 435/450 IVB -11 497/498 486/498 BYT -5 289/289 284/289 HSW -10 563/564 553/564 BDW -7 417/417 410/417 -------------------------------------Detailed------------------------------------- Platform Test drm-intel-nightly Series Applied *PNV igt_gem_ringfill_blitter PASS(3, M23M25) DMESG_WARN(1, M25) *PNV igt_gem_ringfill_blitter-interruptible PASS(3, M23M25) DMESG_WARN(1, M25) *ILK igt_gem_concurrent_blit_gpu-bcs-gpu-read-after-write-interruptible PASS(2, M26) DMESG_WARN(1, M26) *ILK igt_gem_concurrent_blit_gpu-bcs-overwrite-source-interruptible PASS(2, M26) DMESG_WARN(1, M26) *ILK igt_gem_concurrent_blit_gpuX-bcs-gpu-read-after-write-interruptible PASS(2, M26) DMESG_WARN(1, M26) *ILK igt_gem_concurrent_blit_gpuX-bcs-overwrite-source-interruptible PASS(2, M26) DMESG_WARN(1, M26) *ILK igt_gem_concurrent_blit_prw-bcs-gpu-read-after-write-interruptible PASS(2, M26) DMESG_WARN(1, M26) *ILK igt_kms_flip_nonexisting-fb DMESG_WARN(1, M26)PASS(4, M26) NSPT(1, M26) *ILK igt_gem_concurrent_blit_gtt-bcs-gpu-read-after-write-interruptible PASS(2, M26) DMESG_WARN(1, M26) *ILK igt_gem_concurrent_blit_gttX-bcs-gpu-read-after-write-interruptible PASS(2, M26) DMESG_WARN(1, M26) *ILK igt_gem_ringfill_blitter PASS(2, M26) DMESG_WARN(1, M26) *ILK igt_gem_ringfill_blitter-interruptible PASS(2, M26) DMESG_WARN(1, M26) *ILK igt_kms_flip_bcs-flip-vs-modeset-interruptible PASS(5, M26) DMESG_WARN(1, M26) *SNB igt_gem_concurrent_blit_cpu-rcs-gpu-read-after-write-interruptible PASS(3, M35M22) DMESG_WARN(1, M22) *SNB igt_gem_concurrent_blit_gpu-rcs-gpu-read-after-write-forked PASS(3, M35M22) DMESG_WARN(1, M22) *SNB igt_gem_concurrent_blit_gpu-rcs-gpu-read-after-write-interruptible PASS(3, M35M22) DMESG_WARN(1, M22) *SNB igt_gem_concurrent_blit_gpuX-rcs-gpu-read-after-write-forked PASS(2, M35M22) DMESG_WARN(1, M22) *SNB igt_gem_concurrent_blit_gpuX-rcs-gpu-read-after-write-interruptible PASS(3, M35M22) DMESG_WARN(1, M22) *SNB igt_gem_concurrent_blit_prw-rcs-gpu-read-after-write-forked PASS(3, M35M22) DMESG_WARN(1, M22) *SNB igt_gem_concurrent_blit_prw-rcs-gpu-read-after-write-interruptible PASS(3, M35M22) DMESG_WARN(1, M22) *SNB igt_gem_ring_sync_copy_sync-render-blitter-read-write PASS(3, M35M22) DMESG_WARN(1, M22) *SNB igt_gem_ring_sync_copy_sync-render-blitter-write-read PASS(3, M35M22) DMESG_WARN(1, M22) *SNB igt_gem_ring_sync_copy_sync-render-blitter-write-write PASS(3, M35M22) DMESG_WARN(1, M22) *SNB igt_gem_userptr_blits_coherency-sync PASS(3, M35M22) DMESG_WARN(1, M22) *SNB igt_pm_rps_min-max-config-loaded PASS(3, M35M22) DMESG_WARN(1, M22) *SNB igt_gem_concurrent_blit_gtt-rcs-gpu-read-after-write-interruptible PASS(3, M35M22) DMESG_WARN(1, M22) *IVB igt_gem_linear_blits_interruptible PASS(3, M4M34) DMESG_WARN(1, M34) *IVB igt_gem_linear_blits_normal PASS(3, M4M34) DMESG_WARN(1, M34) *IVB igt_gem_render_linear_blits PASS(3, M4M34) DMESG_WARN(1, M34) *IVB igt_gem_render_tiled_blits PASS(3, M4M34) DMESG_WARN(1, M34) *IVB igt_gem_tiled_blits_interruptible PASS(3, M4M34) DMESG_WARN(1, M34) *IVB igt_gem_tiled_blits_normal PASS(3, M4M34) DMESG_WARN(1, M34) *IVB igt_gem_tiled_fence_blits PASS(3, M4M34) DMESG_WARN(1, M34) *IVB igt_gem_userptr_blits_coherency-sync PASS(2, M4M34) DMESG_WARN(1, M34) *IVB igt_gem_userptr_blits_coherency-unsync PASS(2, M4M34) DMESG_WARN(1, M34) *IVB igt_gem_ringfill_blitter PASS(3, M4M34) DMESG_WARN(1, M34) *IVB igt_gem_ringfill_blitter-interruptible PASS(3, M4M34) DMESG_WARN(1, M34) *BYT igt_gem_gtt_hog PASS(3, M48M51) DMESG_WARN(1, M51) *BYT igt_gem_ringfill_blitter PASS(3, M48M51) DMESG_WARN(1, M51) *BYT igt_gem_ringfill_blitter-interruptible PASS(3, M48M51) DMESG_WARN(1, M51) *BYT igt_gem_ringfill_render PASS(3, M48M51) DMESG_WARN(1, M51) *BYT igt_gem_ringfill_render-interruptible PASS(3, M48M51) DMESG_WARN(1, M51) *HSW igt_gem_linear_blits_interruptible PASS(3, M40M20) DMESG_WARN(1, M40) *HSW igt_gem_linear_blits_normal PASS(3, M40M20) DMESG_WARN(1, M40) *HSW igt_gem_render_linear_blits PASS(3, M40M20) DMESG_WARN(1, M40) *HSW igt_gem_render_tiled_blits PASS(3, M40M20) DMESG_WARN(1, M40) *HSW igt_gem_tiled_blits_interruptible PASS(3, M40M20) DMESG_WARN(1, M40) *HSW igt_gem_tiled_blits_normal PASS(3, M40M20) DMESG_WARN(1, M40) *HSW igt_gem_tiled_fence_blits PASS(3, M40M20) DMESG_WARN(1, M40) *HSW igt_pm_rps_min-max-config-loaded PASS(3, M40M20) DMESG_WARN(1, M40) *HSW igt_gem_ringfill_blitter PASS(3, M40M20) DMESG_WARN(1, M40) *HSW igt_gem_ringfill_blitter-interruptible PASS(3, M40M20) DMESG_WARN(1, M40) *BDW igt_gem_render_linear_blits TIMEOUT(1, M30)PASS(3, M30M28) DMESG_WARN(1, M28) *BDW igt_gem_render_tiled_blits TIMEOUT(1, M30)PASS(3, M30M28) DMESG_WARN(1, M28) *BDW igt_gem_gtt_hog PASS(3, M30M28) DMESG_WARN(1, M28) *BDW igt_gem_ringfill_blitter PASS(3, M30M28) DMESG_WARN(1, M28) *BDW igt_gem_ringfill_blitter-interruptible PASS(3, M30M28) DMESG_WARN(1, M28) *BDW igt_gem_ringfill_render PASS(3, M30M28) DMESG_WARN(1, M28) *BDW igt_gem_ringfill_render-interruptible PASS(3, M30M28) DMESG_WARN(1, M28) Note: You need to pay more attention to line start with '*' _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 0/2] Track calls to intel(_logical)_ring_{begin, advance} 2014-12-10 15:07 [PATCH 0/2] Track calls to intel(_logical)_ring_{begin, advance} Dave Gordon 2014-12-10 15:07 ` [PATCH 1/2] drm/i915: Track & check " Dave Gordon 2014-12-10 15:07 ` [PATCH 2/2] drm/i915: Track nested " Dave Gordon @ 2014-12-10 16:12 ` Dave Gordon 2014-12-10 16:12 ` [PATCH v2 1/2] drm/i915: Track & check " Dave Gordon 2014-12-10 16:12 ` [PATCH v2 2/2] drm/i915: Track nested " Dave Gordon 2014-12-10 16:23 ` [PATCH 0/2] Track " Chris Wilson 3 siblings, 2 replies; 10+ messages in thread From: Dave Gordon @ 2014-12-10 16:12 UTC (permalink / raw) To: intel-gfx When adding instructions to a legacy or LRC ringbuffer, the sequence of emit() calls must be preceded by a call to intel(_logical)_ring_begin() to reserve the required amount of space, and followed by a matching call to intel(_logical)_ring_advance(). Historically some (display) code didn't use begin/advance, but just inserted instructions ad hoc, which would then be sent to the hardware along with the current or next batch, but this is not supported and is now regarded as incorrect. This commit therefore adds begin/advance tracking, with WARNings where various forms of misuse are detected. v2: remove redundant "#if 1" markers _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] drm/i915: Track & check calls to intel(_logical)_ring_{begin, advance} 2014-12-10 16:12 ` [PATCH v2 0/2] Track " Dave Gordon @ 2014-12-10 16:12 ` Dave Gordon 2014-12-10 16:12 ` [PATCH v2 2/2] drm/i915: Track nested " Dave Gordon 1 sibling, 0 replies; 10+ messages in thread From: Dave Gordon @ 2014-12-10 16:12 UTC (permalink / raw) To: intel-gfx When adding instructions to a legacy or LRC ringbuffer, the sequence of emit() calls must be preceded by a call to intel(_logical)_ring_begin() to reserve the required amount of space, and followed by a matching call to intel(_logical)_ring_advance() (note that this used to trigger immediate submission to the h/w, but now actual submission is deferred until all the instructions for a single batch submission have been assembled). Historically some (display) code didn't use begin/advance, but just inserted instructions ad hoc, which would then be sent to the hardware along with the current or next batch, but this is not supported and is now regarded as incorrect. This commit therefore adds begin/advance tracking, with WARNings where various forms of misuse are detected. These include: * advance without begin * begin without advance before submission to h/w * multiple begins without an advance between * exceeding the space reserved by begin * leaving the ring misaligned * ring buffer overrun (negative freespace) Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/intel_lrc.c | 4 ++- drivers/gpu/drm/i915/intel_lrc.h | 11 ++++++- drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++-- drivers/gpu/drm/i915/intel_ringbuffer.h | 54 ++++++++++++++++++++++++++++++- 4 files changed, 74 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index a82020e..56e3636 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -825,6 +825,7 @@ void intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf) struct intel_context *ctx = ringbuf->FIXME_lrc_ctx; intel_logical_ring_advance(ringbuf); + WARN_ON(ringbuf->rsv_level != 0); if (intel_ring_stopped(ring)) return; @@ -1084,7 +1085,8 @@ int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf, int num_dwords) if (ret) return ret; - ringbuf->space -= num_dwords * sizeof(uint32_t); + __intel_ringbuffer_begin(ringbuf, num_dwords); + return 0; } diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h index 14b216b..9a0457e 100644 --- a/drivers/gpu/drm/i915/intel_lrc.h +++ b/drivers/gpu/drm/i915/intel_lrc.h @@ -48,8 +48,17 @@ void intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf); */ static inline void intel_logical_ring_advance(struct intel_ringbuffer *ringbuf) { - 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; } + /** * intel_logical_ring_emit() - write a DWORD to the ringbuffer. * @ringbuf: Ringbuffer to write to. diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 83accb7..5874eab 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -55,6 +55,7 @@ int __intel_ring_space(int head, int tail, int size) int space = head - tail; if (space <= 0) space += size; + WARN_ON(space < I915_RING_FREE_SPACE); return space - I915_RING_FREE_SPACE; } @@ -84,7 +85,10 @@ bool intel_ring_stopped(struct intel_engine_cs *ring) void __intel_ring_advance(struct intel_engine_cs *ring) { struct intel_ringbuffer *ringbuf = ring->buffer; - ringbuf->tail &= ringbuf->size - 1; + + intel_ring_advance(ring); + WARN_ON(ringbuf->rsv_level != 0); + if (intel_ring_stopped(ring)) return; ring->write_tail(ring, ringbuf->tail); @@ -1911,6 +1915,7 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n) return 0; list_for_each_entry(request, &ring->request_list, list) { + /* Would completion of this request free enough space? */ if (__intel_ring_space(request->tail, ringbuf->tail, ringbuf->size) >= n) { break; @@ -2096,7 +2101,8 @@ int intel_ring_begin(struct intel_engine_cs *ring, if (ret) return ret; - ring->buffer->space -= num_dwords * sizeof(uint32_t); + __intel_ringbuffer_begin(ring->buffer, num_dwords); + return 0; } diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 6dbb6f4..a6660c1 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -112,6 +112,11 @@ struct intel_ringbuffer { int size; int effective_size; + /* these let advance() check for misuse */ + int rsv_level; /* reservation nesting level */ + int rsv_size; /* size passed to begin() */ + int rsv_start; /* tail when begin() last returned */ + /** We track the position of the requests in the ring buffer, and * when each is retired we increment last_retired_head as the GPU * must have finished processing the request and so we know we @@ -401,11 +406,58 @@ static inline void intel_ring_emit(struct intel_engine_cs *ring, iowrite32(data, ringbuf->virtual_start + ringbuf->tail); ringbuf->tail += 4; } + +static inline void __intel_ringbuffer_begin(struct intel_ringbuffer *ringbuf, + int num_dwords) +{ + int nbytes = num_dwords * sizeof(uint32_t); + + WARN_ON(num_dwords & 1); + WARN_ON(nbytes <= 0); + + if (ringbuf->rsv_level++) { + /* begin() called twice or more without advance() */ + WARN_ON(1); + } else { + /* + * A new reservation; validate and record the start and + * size, then deduct the size from the remaining space + */ + WARN_ON(ringbuf->tail & 7); + WARN_ON(ringbuf->tail > ringbuf->effective_size-8); + WARN_ON(ringbuf->tail + nbytes > ringbuf->effective_size); + WARN_ON(ringbuf->space < nbytes); + } + + ringbuf->rsv_start = ringbuf->tail; + ringbuf->rsv_size = nbytes; + ringbuf->space -= nbytes; +} + +static inline void __intel_ringbuffer_check(struct intel_ringbuffer *ringbuf) +{ + WARN_ON(ringbuf->rsv_level-- != 1); + WARN_ON(ringbuf->rsv_start < 0 || ringbuf->rsv_size < 0); + WARN_ON(ringbuf->tail & 7); + WARN_ON(ringbuf->tail > ringbuf->rsv_start + ringbuf->rsv_size); + WARN_ON(ringbuf->tail > ringbuf->effective_size); +} + 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; } + int __intel_ring_space(int head, int tail, int size); void intel_ring_update_space(struct intel_ringbuffer *ringbuf); int intel_ring_space(struct intel_ringbuffer *ringbuf); -- 1.7.9.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] drm/i915: Track nested calls to intel(_logical)_ring_{begin, advance} 2014-12-10 16:12 ` [PATCH v2 0/2] Track " Dave Gordon 2014-12-10 16:12 ` [PATCH v2 1/2] drm/i915: Track & check " Dave Gordon @ 2014-12-10 16:12 ` Dave Gordon 2014-12-11 0:30 ` shuang.he 1 sibling, 1 reply; 10+ messages in thread From: Dave Gordon @ 2014-12-10 16:12 UTC (permalink / raw) To: intel-gfx With the current deferred-submission model, if a problem arises part-way through the insertion of instructions into the ringbuffer (e.g. due to one of the begin() calls finding there's not enough space), we avoid sending the incomplete sequence to the h/w; but currently have no means of undoing the work so far, which will lead to undefined behaviour when the next batch is submitted (probably TDR will trigger a reset first, though, and clean up the ring state). A future idea is to move to an atomic-submission model, where all the space required for a batch submission is reserved up front, and in the event of failure partway through, the work can be abandoned without side-effects. This will be required for the forthcoming GPU scheduler (specifically, for preemption). To support this, we allow nested begin/advance pairs. Specifically, the outermost pair defines the total space reservation; inner pairs can be nested ad lib, but all inner reservations at any level must fit entirely within the outermost one. Thus, this is permitted: begin(128) - guarantees that up to 128 dwords can now be emitted without waiting for more freespace begin(6) advance begin(10) advance begin(8) advance etc, as long as the total is no more than 128 dwords advance-and-submit The execbuffer code will later be enhanced to use this approach. In the mean time, the traditional single-level begin/advance mechanism remains fully supported. This commit changes only the begin/advance checking code, to permit (but not require) nested begin/advance pairs. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/intel_ringbuffer.h | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index a6660c1..68665c7 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -416,8 +416,17 @@ static inline void __intel_ringbuffer_begin(struct intel_ringbuffer *ringbuf, WARN_ON(nbytes <= 0); if (ringbuf->rsv_level++) { - /* begin() called twice or more without advance() */ - WARN_ON(1); + /* + * A nested reservation; check that it falls entirely + * within the outer block. Don't adjust remaining space. + */ + WARN_ON(ringbuf->rsv_start < 0); + WARN_ON(ringbuf->rsv_start & 7); + WARN_ON(ringbuf->tail & 7); + WARN_ON(ringbuf->tail > ringbuf->effective_size); + WARN_ON(ringbuf->tail > ringbuf->rsv_start + ringbuf->rsv_size); + WARN_ON(ringbuf->tail + nbytes > ringbuf->effective_size); + WARN_ON(ringbuf->tail + nbytes > ringbuf->rsv_start + ringbuf->rsv_size); } else { /* * A new reservation; validate and record the start and @@ -436,7 +445,7 @@ static inline void __intel_ringbuffer_begin(struct intel_ringbuffer *ringbuf, static inline void __intel_ringbuffer_check(struct intel_ringbuffer *ringbuf) { - WARN_ON(ringbuf->rsv_level-- != 1); + WARN_ON(ringbuf->rsv_level-- <= 0); WARN_ON(ringbuf->rsv_start < 0 || ringbuf->rsv_size < 0); WARN_ON(ringbuf->tail & 7); WARN_ON(ringbuf->tail > ringbuf->rsv_start + ringbuf->rsv_size); -- 1.7.9.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: Track nested calls to intel(_logical)_ring_{begin, advance} 2014-12-10 16:12 ` [PATCH v2 2/2] drm/i915: Track nested " Dave Gordon @ 2014-12-11 0:30 ` shuang.he 0 siblings, 0 replies; 10+ messages in thread From: shuang.he @ 2014-12-11 0:30 UTC (permalink / raw) To: shuang.he, intel-gfx, david.s.gordon Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com) -------------------------------------Summary------------------------------------- Platform Delta drm-intel-nightly Series Applied PNV -2 364/364 362/364 ILK +1-9 364/366 356/366 SNB -12 448/450 436/450 IVB -9 497/498 488/498 BYT -5 289/289 284/289 HSW -11 563/564 552/564 BDW -5 417/417 412/417 -------------------------------------Detailed------------------------------------- Platform Test drm-intel-nightly Series Applied *PNV igt_gem_ringfill_blitter PASS(3, M23M25) DMESG_WARN(1, M23) *PNV igt_gem_ringfill_blitter-interruptible PASS(3, M23M25) DMESG_WARN(1, M23) *ILK igt_gem_concurrent_blit_gpu-bcs-gpu-read-after-write-interruptible PASS(3, M26M37) DMESG_WARN(1, M37) *ILK igt_gem_concurrent_blit_gpu-bcs-overwrite-source PASS(2, M26M37) DMESG_WARN(1, M37) *ILK igt_gem_concurrent_blit_gpu-bcs-overwrite-source-interruptible PASS(3, M26M37) DMESG_WARN(1, M37) *ILK igt_gem_concurrent_blit_gpuX-bcs-gpu-read-after-write-interruptible PASS(3, M26M37) DMESG_WARN(1, M37) *ILK igt_gem_concurrent_blit_gpuX-bcs-overwrite-source-interruptible PASS(3, M26M37) DMESG_WARN(1, M37) *ILK igt_gem_concurrent_blit_gtt-bcs-gpu-read-after-write-interruptible PASS(3, M26M37) DMESG_WARN(1, M37) *ILK igt_gem_concurrent_blit_gttX-bcs-gpu-read-after-write-interruptible PASS(3, M26M37) DMESG_WARN(1, M37) *ILK igt_gem_ringfill_blitter PASS(3, M26M37) DMESG_WARN(1, M37) *ILK igt_gem_ringfill_blitter-interruptible PASS(3, M26M37) DMESG_WARN(1, M37) ILK igt_kms_flip_wf_vblank-ts-check DMESG_WARN(3, M26)PASS(19, M26M37) PASS(1, M37) *SNB igt_gem_concurrent_blit_cpu-rcs-gpu-read-after-write-interruptible PASS(3, M35M22) DMESG_WARN(1, M35) *SNB igt_gem_concurrent_blit_gpu-rcs-gpu-read-after-write-forked PASS(3, M35M22) DMESG_WARN(1, M35) *SNB igt_gem_concurrent_blit_gpu-rcs-gpu-read-after-write-interruptible PASS(3, M35M22) DMESG_WARN(1, M35) *SNB igt_gem_concurrent_blit_gpuX-rcs-gpu-read-after-write-interruptible PASS(3, M35M22) DMESG_WARN(1, M35) *SNB igt_gem_concurrent_blit_prw-rcs-gpu-read-after-write-forked PASS(3, M35M22) DMESG_WARN(1, M35) *SNB igt_gem_concurrent_blit_prw-rcs-gpu-read-after-write-interruptible PASS(3, M35M22) DMESG_WARN(1, M35) *SNB igt_gem_ring_sync_copy_sync-render-blitter-read-write PASS(3, M35M22) DMESG_WARN(1, M35) *SNB igt_gem_ring_sync_copy_sync-render-blitter-write-read PASS(3, M35M22) DMESG_WARN(1, M35) *SNB igt_gem_ring_sync_copy_sync-render-blitter-write-write PASS(3, M35M22) DMESG_WARN(1, M35) *SNB igt_gem_userptr_blits_coherency-sync PASS(3, M35M22) DMESG_WARN(1, M35) *SNB igt_pm_rps_min-max-config-loaded PASS(3, M35M22) DMESG_WARN(1, M35) *SNB igt_gem_concurrent_blit_gtt-rcs-gpu-read-after-write-interruptible PASS(3, M35M22) DMESG_WARN(1, M35) *IVB igt_gem_linear_blits_interruptible PASS(3, M4M34) DMESG_WARN(1, M4) *IVB igt_gem_linear_blits_normal PASS(3, M4M34) DMESG_WARN(1, M4) *IVB igt_gem_render_linear_blits PASS(3, M4M34) DMESG_WARN(1, M4) *IVB igt_gem_render_tiled_blits PASS(3, M4M34) DMESG_WARN(1, M4) *IVB igt_gem_tiled_blits_interruptible PASS(3, M4M34) DMESG_WARN(1, M4) *IVB igt_gem_tiled_blits_normal PASS(3, M4M34) DMESG_WARN(1, M4) *IVB igt_gem_tiled_fence_blits PASS(3, M4M34) DMESG_WARN(1, M4) *IVB igt_gem_ringfill_blitter PASS(3, M4M34) DMESG_WARN(1, M4) *IVB igt_gem_ringfill_blitter-interruptible PASS(3, M4M34) DMESG_WARN(1, M4) *BYT igt_gem_gtt_hog PASS(3, M48M51) DMESG_WARN(1, M48) *BYT igt_gem_ringfill_blitter PASS(3, M48M51) DMESG_WARN(1, M48) *BYT igt_gem_ringfill_blitter-interruptible PASS(3, M48M51) DMESG_WARN(1, M48) *BYT igt_gem_ringfill_render PASS(3, M48M51) DMESG_WARN(1, M48) *BYT igt_gem_ringfill_render-interruptible PASS(3, M48M51) DMESG_WARN(1, M48) *HSW igt_gem_linear_blits_interruptible PASS(3, M40M20) DMESG_WARN(1, M20) *HSW igt_gem_linear_blits_normal PASS(3, M40M20) DMESG_WARN(1, M20) *HSW igt_gem_render_linear_blits PASS(3, M40M20) DMESG_WARN(1, M20) *HSW igt_gem_render_tiled_blits PASS(3, M40M20) DMESG_WARN(1, M20) *HSW igt_gem_tiled_blits_interruptible PASS(3, M40M20) DMESG_WARN(1, M20) *HSW igt_gem_tiled_blits_normal PASS(3, M40M20) DMESG_WARN(1, M20) *HSW igt_gem_tiled_fence_blits PASS(3, M40M20) DMESG_WARN(1, M20) *HSW igt_gem_userptr_blits_coherency-unsync PASS(2, M40M20) DMESG_WARN(1, M20) *HSW igt_pm_rps_min-max-config-loaded PASS(3, M40M20) DMESG_WARN(1, M20) *HSW igt_gem_ringfill_blitter PASS(3, M40M20) DMESG_WARN(1, M20) *HSW igt_gem_ringfill_blitter-interruptible PASS(3, M40M20) DMESG_WARN(1, M20) *BDW igt_gem_gtt_hog PASS(3, M30M28) DMESG_WARN(1, M30) *BDW igt_gem_ringfill_blitter PASS(3, M30M28) DMESG_WARN(1, M30) *BDW igt_gem_ringfill_blitter-interruptible PASS(3, M30M28) DMESG_WARN(1, M30) *BDW igt_gem_ringfill_render PASS(3, M30M28) DMESG_WARN(1, M30) *BDW igt_gem_ringfill_render-interruptible PASS(3, M30M28) DMESG_WARN(1, M30) Note: You need to pay more attention to line start with '*' _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Track calls to intel(_logical)_ring_{begin, advance} 2014-12-10 15:07 [PATCH 0/2] Track calls to intel(_logical)_ring_{begin, advance} Dave Gordon ` (2 preceding siblings ...) 2014-12-10 16:12 ` [PATCH v2 0/2] Track " Dave Gordon @ 2014-12-10 16:23 ` Chris Wilson 3 siblings, 0 replies; 10+ messages in thread From: Chris Wilson @ 2014-12-10 16:23 UTC (permalink / raw) To: Dave Gordon; +Cc: intel-gfx On Wed, Dec 10, 2014 at 03:07:07PM +0000, Dave Gordon wrote: > When adding instructions to a legacy or LRC ringbuffer, the sequence of > emit() calls must be preceded by a call to intel(_logical)_ring_begin() > to reserve the required amount of space, and followed by a matching call > to intel(_logical)_ring_advance(). Historically some (display) code > didn't use begin/advance, but just inserted instructions ad hoc, which > would then be sent to the hardware along with the current or next batch, > but this is not supported and is now regarded as incorrect. > > This commit therefore adds begin/advance tracking, with WARNings where > various forms of misuse are detected. Please review the suggested approach I made months ago which avoid all this ad hoc hilarity. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-12-11 0:30 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-10 15:07 [PATCH 0/2] Track calls to intel(_logical)_ring_{begin, advance} Dave Gordon
2014-12-10 15:07 ` [PATCH 1/2] drm/i915: Track & check " Dave Gordon
2014-12-10 15:44 ` Daniel Vetter
2014-12-10 15:07 ` [PATCH 2/2] drm/i915: Track nested " Dave Gordon
2014-12-10 22:46 ` shuang.he
2014-12-10 16:12 ` [PATCH v2 0/2] Track " Dave Gordon
2014-12-10 16:12 ` [PATCH v2 1/2] drm/i915: Track & check " Dave Gordon
2014-12-10 16:12 ` [PATCH v2 2/2] drm/i915: Track nested " Dave Gordon
2014-12-11 0:30 ` shuang.he
2014-12-10 16:23 ` [PATCH 0/2] Track " Chris Wilson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.