From patchwork Wed Dec 10 16:12:14 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dave Gordon X-Patchwork-Id: 5470651 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 6A0D3BEEA8 for ; Wed, 10 Dec 2014 16:18:40 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 519422010B for ; Wed, 10 Dec 2014 16:18:39 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 76389200F3 for ; Wed, 10 Dec 2014 16:18:34 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id E8D9B6E69F; Wed, 10 Dec 2014 08:18:33 -0800 (PST) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id 09B516E69F for ; Wed, 10 Dec 2014 08:18:32 -0800 (PST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP; 10 Dec 2014 08:12:24 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,553,1413270000"; d="scan'208";a="635624560" Received: from dsgordon-linux.isw.intel.com ([10.102.226.149]) by fmsmga001.fm.intel.com with ESMTP; 10 Dec 2014 08:12:23 -0800 From: Dave Gordon To: intel-gfx@lists.freedesktop.org Date: Wed, 10 Dec 2014 16:12:14 +0000 Message-Id: <1418227935-21630-2-git-send-email-david.s.gordon@intel.com> X-Mailer: git-send-email 1.7.9.5 In-Reply-To: <1418227935-21630-1-git-send-email-david.s.gordon@intel.com> References: <1418224029-20055-1-git-send-email-david.s.gordon@intel.com> <1418227935-21630-1-git-send-email-david.s.gordon@intel.com> Subject: [Intel-gfx] [PATCH v2 1/2] drm/i915: Track & check calls to intel(_logical)_ring_{begin, advance} X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 --- 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);