From patchwork Tue Apr 12 20:03:08 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chris Wilson X-Patchwork-Id: 8814761 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 490CB9F3D1 for ; Tue, 12 Apr 2016 20:03:45 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 016092034C for ; Tue, 12 Apr 2016 20:03:44 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id AEE8420374 for ; Tue, 12 Apr 2016 20:03:42 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 99BA46E793; Tue, 12 Apr 2016 20:03:40 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-wm0-x242.google.com (mail-wm0-x242.google.com [IPv6:2a00:1450:400c:c09::242]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4DDCA6E78F for ; Tue, 12 Apr 2016 20:03:34 +0000 (UTC) Received: by mail-wm0-x242.google.com with SMTP id l6so8298366wml.3 for ; Tue, 12 Apr 2016 13:03:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id:in-reply-to:references; bh=BFQa8JDu++SKWuZzfRbS6eGejmbBENFi0F8zCuJcRi0=; b=GgQgKapEt0ytbAAYmMRNsohkWBX72VP4eA6QlmXR97IATUkim0AZmK9DYhaT7iv9j0 3HPebRHZ3Gmo+KRbw8dIcLV2SVNy8l3S6EgYaj+qevAbhi/7Q1VVfJL7xJy1iIOh8lNi eIi5ZohAp58cJ2YCw9IjZcHjmUAxuIYAl3H/ORY5AwPG5BMqNl02FE3dybKIM5jccwfZ xJ1DJAsTA6KjnS3ApnYHi6vaajZJkLBTOFV9geU42X9u4us0c/PuxJjPughw42EQfUqY NCdHl1dfIVuFBa/c0+32NJ3CHdfpx7lSuuwCU26HU96R3ItbT9eePzXfHboScAIDIQ4t fSpw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:from:to:cc:subject:date:message-id :in-reply-to:references; bh=BFQa8JDu++SKWuZzfRbS6eGejmbBENFi0F8zCuJcRi0=; b=Aj5jycfz8LBZc/emv/bvm+w3Amc1t1w6feg/nZB0v7UYUQiHOUGull6bF4J6KhchS9 s0W8+5D/P3HPrexKWmsHb+dj3m9XqZEBLtu1YWWX1MLtjaXZoHQtlvtT3PeWwNT/yDiI Dp/GTdHjsu0wVuFDxIzQbX1tWeCBpOKD1xRYFNe4RNJRr+oponkUpjqIE98s/n6RI4fV +s17UU6r9oVTRNJI7WcFtDL/mOHC/A1FPa8kXiUQ3MMZvcAy7q+ioEz2rI8n2Csee8Ml hW3K8QMHxkkqozv7G8VT6bGm8/jyHMTBFzgnRjSoL21LSHLPynJI7aR0CYpCNyoHH2ka X3xg== X-Gm-Message-State: AD7BkJJMEi3UTIOmfAnHgz8LgC2J0gRJ+/tAX7GZayLlT9YTGpXeo05ElEM7rGCHYC6nhg== X-Received: by 10.28.54.70 with SMTP id d67mr28317047wma.94.1460491412907; Tue, 12 Apr 2016 13:03:32 -0700 (PDT) Received: from haswell.alporthouse.com ([78.156.65.138]) by smtp.gmail.com with ESMTPSA id i201sm24406718wmf.23.2016.04.12.13.03.31 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 12 Apr 2016 13:03:31 -0700 (PDT) From: Chris Wilson To: intel-gfx@lists.freedesktop.org Date: Tue, 12 Apr 2016 21:03:08 +0100 Message-Id: <1460491389-8602-14-git-send-email-chris@chris-wilson.co.uk> X-Mailer: git-send-email 2.8.0.rc3 In-Reply-To: <1460491389-8602-1-git-send-email-chris@chris-wilson.co.uk> References: <1460491389-8602-1-git-send-email-chris@chris-wilson.co.uk> Cc: Daniel Vetter Subject: [Intel-gfx] [CI-ping 14/15] drm/i915: Reorganise legacy context switch to cope with late failure 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=-5.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=ham 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 After mi_set_context() succeeds, we need to update the state of the engine's last_context. This ensures that we hold a pin on the context whilst the hardware may write to it. However, since we didn't complete the post-switch setup of the context, we need to force the subsequent use of the same context to complete the setup (which means updating should_skip_switch()). Signed-off-by: Chris Wilson Cc: Daniel Vetter Cc: Tvrtko Ursulin --- drivers/gpu/drm/i915/i915_gem_context.c | 215 ++++++++++++++++---------------- 1 file changed, 109 insertions(+), 106 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index e5ad7b21e356..361959b1d53a 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -609,50 +609,40 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags) return ret; } -static inline bool should_skip_switch(struct intel_engine_cs *engine, - struct intel_context *from, +static inline bool should_skip_switch(struct intel_context *from, struct intel_context *to) { if (to->remap_slice) return false; - if (to->ppgtt && from == to && - !(intel_engine_flag(engine) & to->ppgtt->pd_dirty_rings)) - return true; + if (!to->legacy_hw_ctx.initialized) + return false; - return false; + if (to->ppgtt && to->ppgtt->pd_dirty_rings & (1 << RCS)) + return false; + + return to == from; } static bool -needs_pd_load_pre(struct intel_engine_cs *engine, struct intel_context *to) +needs_pd_load_pre(struct intel_context *to) { - struct drm_i915_private *dev_priv = engine->dev->dev_private; - if (!to->ppgtt) return false; - if (INTEL_INFO(engine->dev)->gen < 8) - return true; - - if (engine != &dev_priv->engine[RCS]) + if (INTEL_INFO(to->i915)->gen < 8) return true; return false; } static bool -needs_pd_load_post(struct intel_engine_cs *engine, struct intel_context *to, - u32 hw_flags) +needs_pd_load_post(struct intel_context *to, u32 hw_flags) { - struct drm_i915_private *dev_priv = engine->dev->dev_private; - if (!to->ppgtt) return false; - if (!IS_GEN8(engine->dev)) - return false; - - if (engine != &dev_priv->engine[RCS]) + if (!IS_GEN8(to->i915)) return false; if (hw_flags & MI_RESTORE_INHIBIT) @@ -661,60 +651,33 @@ needs_pd_load_post(struct intel_engine_cs *engine, struct intel_context *to, return false; } -static int do_switch(struct drm_i915_gem_request *req) +static int do_rcs_switch(struct drm_i915_gem_request *req) { struct intel_context *to = req->ctx; struct intel_engine_cs *engine = req->engine; - struct drm_i915_private *dev_priv = req->i915; - struct intel_context *from = engine->last_context; - u32 hw_flags = 0; - bool uninitialized = false; + struct intel_context *from; + u32 hw_flags; int ret, i; - if (from != NULL && engine == &dev_priv->engine[RCS]) { - BUG_ON(from->legacy_hw_ctx.rcs_state == NULL); - BUG_ON(!i915_gem_obj_is_pinned(from->legacy_hw_ctx.rcs_state)); - } - - if (should_skip_switch(engine, from, to)) + if (should_skip_switch(engine->last_context, to)) return 0; /* Trying to pin first makes error handling easier. */ - if (engine == &dev_priv->engine[RCS]) { - ret = i915_gem_obj_ggtt_pin(to->legacy_hw_ctx.rcs_state, - get_context_alignment(engine->dev), - 0); - if (ret) - return ret; - } + ret = i915_gem_obj_ggtt_pin(to->legacy_hw_ctx.rcs_state, + get_context_alignment(engine->dev), + 0); + if (ret) + return ret; /* * Pin can switch back to the default context if we end up calling into * evict_everything - as a last ditch gtt defrag effort that also * switches to the default context. Hence we need to reload from here. + * + * XXX: Doing so is painfully broken! */ from = engine->last_context; - if (needs_pd_load_pre(engine, to)) { - /* Older GENs and non render rings still want the load first, - * "PP_DCLV followed by PP_DIR_BASE register through Load - * Register Immediate commands in Ring Buffer before submitting - * a context."*/ - trace_switch_mm(engine, to); - ret = to->ppgtt->switch_mm(to->ppgtt, req); - if (ret) - goto unpin_out; - - /* Doing a PD load always reloads the page dirs */ - to->ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine); - } - - if (engine != &dev_priv->engine[RCS]) { - if (from) - i915_gem_context_unreference(from); - goto done; - } - /* * Clear this page out of any CPU caches for coherent swap-in/out. Note * that thanks to write = false in this call and us not setting any gpu @@ -727,55 +690,46 @@ static int do_switch(struct drm_i915_gem_request *req) if (ret) goto unpin_out; - if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) { - hw_flags |= MI_RESTORE_INHIBIT; + if (needs_pd_load_pre(to)) { + /* Older GENs and non render rings still want the load first, + * "PP_DCLV followed by PP_DIR_BASE register through Load + * Register Immediate commands in Ring Buffer before submitting + * a context."*/ + trace_switch_mm(engine, to); + ret = to->ppgtt->switch_mm(to->ppgtt, req); + if (ret) + goto unpin_out; + + /* Doing a PD load always reloads the page dirs */ + to->ppgtt->pd_dirty_rings &= ~(1 << RCS); + } + + if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) /* NB: If we inhibit the restore, the context is not allowed to * die because future work may end up depending on valid address * space. This means we must enforce that a page table load * occur when this occurs. */ - } else if (to->ppgtt && - (intel_engine_flag(engine) & to->ppgtt->pd_dirty_rings)) { - hw_flags |= MI_FORCE_RESTORE; - to->ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine); - } + hw_flags = MI_RESTORE_INHIBIT; + else if (to->ppgtt && to->ppgtt->pd_dirty_rings & (1 << RCS)) + hw_flags = MI_FORCE_RESTORE; + else + hw_flags = 0; /* We should never emit switch_mm more than once */ - WARN_ON(needs_pd_load_pre(engine, to) && - needs_pd_load_post(engine, to, hw_flags)); + WARN_ON(needs_pd_load_pre(to) && needs_pd_load_post(to, hw_flags)); - ret = mi_set_context(req, hw_flags); - if (ret) - goto unpin_out; - - /* GEN8 does *not* require an explicit reload if the PDPs have been - * setup, and we do not wish to move them. - */ - if (needs_pd_load_post(engine, to, hw_flags)) { - trace_switch_mm(engine, to); - ret = to->ppgtt->switch_mm(to->ppgtt, req); - /* The hardware context switch is emitted, but we haven't - * actually changed the state - so it's probably safe to bail - * here. Still, let the user know something dangerous has - * happened. - */ + if (to != from || (hw_flags & MI_FORCE_RESTORE)) { + ret = mi_set_context(req, hw_flags); if (ret) { - DRM_ERROR("Failed to change address space on context switch\n"); + /* Force the reload of this and the previous mm */ + if (needs_pd_load_pre(to)) + to->ppgtt->pd_dirty_rings |= 1 << RCS; + if (from && needs_pd_load_pre(from)) + from->ppgtt->pd_dirty_rings |= 1 << RCS; goto unpin_out; } } - for (i = 0; i < MAX_L3_SLICES; i++) { - if (!(to->remap_slice & (1<remap_slice &= ~(1<legacy_hw_ctx.rcs_state); i915_gem_context_unreference(from); } - - uninitialized = !to->legacy_hw_ctx.initialized; - to->legacy_hw_ctx.initialized = true; - -done: i915_gem_context_reference(to); engine->last_context = to; - if (uninitialized) { + /* GEN8 does *not* require an explicit reload if the PDPs have been + * setup, and we do not wish to move them. + */ + if (needs_pd_load_post(to, hw_flags)) { + trace_switch_mm(engine, to); + ret = to->ppgtt->switch_mm(to->ppgtt, req); + /* The hardware context switch is emitted, but we haven't + * actually changed the state - so it's probably safe to bail + * here. Still, let the user know something dangerous has + * happened. + */ + if (ret) + return ret; + + to->ppgtt->pd_dirty_rings &= ~(1 << RCS); + } + if (hw_flags & MI_FORCE_RESTORE) + to->ppgtt->pd_dirty_rings &= ~(1 << RCS); + + for (i = 0; i < MAX_L3_SLICES; i++) { + if (!(to->remap_slice & (1<remap_slice &= ~(1<legacy_hw_ctx.initialized) { if (engine->init_context) { ret = engine->init_context(req); if (ret) - DRM_ERROR("ring init context: %d\n", ret); + return ret; } + to->legacy_hw_ctx.initialized = true; } return 0; unpin_out: - if (engine->id == RCS) - i915_gem_object_ggtt_unpin(to->legacy_hw_ctx.rcs_state); + i915_gem_object_ggtt_unpin(to->legacy_hw_ctx.rcs_state); return ret; } @@ -853,7 +832,31 @@ int i915_switch_context(struct drm_i915_gem_request *req) return 0; } - return do_switch(req); + if (engine->id != RCS) { + struct intel_context *from = engine->last_context; + struct intel_context *to = req->ctx; + + if (to->ppgtt && + (from != to || + intel_engine_flag(engine) & to->ppgtt->pd_dirty_rings)) { + int ret; + + trace_switch_mm(engine, to); + ret = to->ppgtt->switch_mm(to->ppgtt, req); + if (ret) + return ret; + + to->ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine); + } + + if (from) + i915_gem_context_unreference(from); + i915_gem_context_reference(to); + engine->last_context = to; + return 0; + } + + return do_rcs_switch(req); } static bool contexts_enabled(struct drm_device *dev)