From patchwork Mon Oct 6 14:15:07 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: John Harrison X-Patchwork-Id: 5037331 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 6F17AC11AB for ; Mon, 6 Oct 2014 14:15:42 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 8C4272015E for ; Mon, 6 Oct 2014 14:15:41 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 94ADF2016C for ; Mon, 6 Oct 2014 14:15:40 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id B05478995F; Mon, 6 Oct 2014 07:15:39 -0700 (PDT) X-Original-To: Intel-GFX@lists.freedesktop.org Delivered-To: Intel-GFX@lists.freedesktop.org Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 6272789939 for ; Mon, 6 Oct 2014 07:15:38 -0700 (PDT) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 06 Oct 2014 07:15:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,664,1406617200"; d="scan'208";a="610269981" Received: from johnharr-linux.isw.intel.com ([10.102.226.51]) by fmsmga002.fm.intel.com with ESMTP; 06 Oct 2014 07:15:29 -0700 From: John.C.Harrison@Intel.com To: Intel-GFX@Lists.FreeDesktop.Org Date: Mon, 6 Oct 2014 15:15:07 +0100 Message-Id: <1412604925-11290-4-git-send-email-John.C.Harrison@Intel.com> X-Mailer: git-send-email 1.7.9.5 In-Reply-To: <1412604925-11290-3-git-send-email-John.C.Harrison@Intel.com> References: <1412604925-11290-1-git-send-email-John.C.Harrison@Intel.com> <1412604925-11290-2-git-send-email-John.C.Harrison@Intel.com> <1412604925-11290-3-git-send-email-John.C.Harrison@Intel.com> Organization: Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ Subject: [Intel-gfx] [RFC 03/21] drm/i915: Ensure OLS & PLR are always in sync X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.15 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=-2.7 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY, URIBL_RHS_DOB 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 From: John Harrison The new seqno alloction code pre-allocates a 'lazy' request structure and then tries to allocate the 'lazy' seqno. The seqno allocation can potential wrap around zero and when doing so, tries to idle the ring by waiting for all oustanding work to complete. With a scheduler in place, this can mean first submitting extra work to the ring. However, at this point in time, the lazy request is valid but the lazy seqno is not. Some existing code was getting confused by this state and Bad Things would happen. The safest solution is to still allocate the lazy request in advance (to avoid having to roll back in an out of memory sitation) but to save the pointer in a local variable rather than immediately updating the lazy pointer. Only after a valid seqno has been acquired is the lazy request pointer actually updated. This guarantees that both lazy values are either invalid or both valid. There can no longer be an inconsistent state. For: VIZ-4377 Signed-off-by: John.C.Harrison@Intel.com --- drivers/gpu/drm/i915/intel_lrc.c | 42 ++++++++++++++++++++----------- drivers/gpu/drm/i915/intel_ringbuffer.c | 29 +++++++++++++++------ 2 files changed, 48 insertions(+), 23 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index bafd38b..3ac2622 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -796,27 +796,39 @@ void intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf) static int logical_ring_alloc_seqno(struct intel_engine_cs *ring, struct intel_context *ctx) { - if (ring->outstanding_lazy_seqno) - return 0; + struct drm_i915_gem_request *request; + int ret; - if (ring->preallocated_lazy_request == NULL) { - struct drm_i915_gem_request *request; + /* The aim is to replace seqno values with request structures. A step + * along the way is to switch to using the PLR in preference to the + * OLS. That requires the PLR to only be valid when the OLS is also + * valid. I.e., the two must be kept in step. */ - request = kmalloc(sizeof(*request), GFP_KERNEL); - if (request == NULL) - return -ENOMEM; + if (ring->outstanding_lazy_seqno) { + BUG_ON(ring->preallocated_lazy_request == NULL); + return 0; + } + BUG_ON(ring->preallocated_lazy_request != NULL); - /* Hold a reference to the context this request belongs to - * (we will need it when the time comes to emit/retire the - * request). - */ - request->ctx = ctx; - i915_gem_context_reference(request->ctx); + request = kmalloc(sizeof(*request), GFP_KERNEL); + if (request == NULL) + return -ENOMEM; - ring->preallocated_lazy_request = request; + ret = i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_seqno); + if (ret) { + kfree(request); + return ret; } - return i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_seqno); + /* Hold a reference to the context this request belongs to + * (we will need it when the time comes to emit/retire the + * request). + */ + request->ctx = ctx; + i915_gem_context_reference(request->ctx); + + ring->preallocated_lazy_request = request; + return 0; } static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf, diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 25795f2..cceac67 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2000,20 +2000,33 @@ int intel_ring_idle(struct intel_engine_cs *ring) static int intel_ring_alloc_seqno(struct intel_engine_cs *ring) { - if (ring->outstanding_lazy_seqno) + int ret; + struct drm_i915_gem_request *request; + + /* The aim is to replace seqno values with request structures. A step + * along the way is to switch to using the PLR in preference to the + * OLS. That requires the PLR to only be valid when the OLS is also + * valid. I.e., the two must be kept in step. */ + + if (ring->outstanding_lazy_seqno) { + BUG_ON(ring->preallocated_lazy_request == NULL); return 0; + } - if (ring->preallocated_lazy_request == NULL) { - struct drm_i915_gem_request *request; + BUG_ON(ring->preallocated_lazy_request != NULL); - request = kmalloc(sizeof(*request), GFP_KERNEL); - if (request == NULL) - return -ENOMEM; + request = kmalloc(sizeof(*request), GFP_KERNEL); + if (request == NULL) + return -ENOMEM; - ring->preallocated_lazy_request = request; + ret = i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_seqno); + if (ret) { + kfree(request); + return ret; } - return i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_seqno); + ring->preallocated_lazy_request = request; + return 0; } static int __intel_ring_prepare(struct intel_engine_cs *ring,