From patchwork Wed Mar 22 17:39:52 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: oscar.mateo@intel.com X-Patchwork-Id: 9640343 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 85FC46020B for ; Thu, 23 Mar 2017 00:40:03 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 761B62842C for ; Thu, 23 Mar 2017 00:40:03 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 6AFD3284ED; Thu, 23 Mar 2017 00:40:03 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.5 required=2.0 tests=BAYES_00, DATE_IN_PAST_06_12, DKIM_SIGNED, RCVD_IN_DNSWL_MED, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id A87A22842C for ; Thu, 23 Mar 2017 00:40:02 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 5EE136E9EA; Thu, 23 Mar 2017 00:40:01 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by gabe.freedesktop.org (Postfix) with ESMTPS id 421056E9DF for ; Thu, 23 Mar 2017 00:39:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=intel.com; i=@intel.com; q=dns/txt; s=intel; t=1490229594; x=1521765594; h=from:to:cc:subject:date:message-id:in-reply-to: references; bh=4mX6RrWHZnRys0iKYiZKE98EQ7JjKN3j6fZuMUpSx4s=; b=kKbjYNso2GgAA8BVqoKFLHDuQc2dg2dYV0UDYda9ck4E4MTFmCNvHDqr y51cTc2tUyQrs/UmrM2+3TsE2Nl6yA==; Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Mar 2017 17:39:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos; i="5.36,207,1486454400"; d="scan'208"; a="1145835216" Received: from omateolo-linux.fm.intel.com ([10.1.27.55]) by fmsmga002.fm.intel.com with ESMTP; 22 Mar 2017 17:39:52 -0700 From: Oscar Mateo To: intel-gfx@lists.freedesktop.org Date: Wed, 22 Mar 2017 10:39:52 -0700 Message-Id: <1490204396-3395-10-git-send-email-oscar.mateo@intel.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1490204396-3395-1-git-send-email-oscar.mateo@intel.com> References: <1490204396-3395-1-git-send-email-oscar.mateo@intel.com> Subject: [Intel-gfx] [PATCH v2 09/13] drm/i915/guc: A little bit more of doorbell sanitization 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-Virus-Scanned: ClamAV using ClamSMTP Some recent refactoring patches have left the doorbell creation outside the GuC client allocation, which does not make a lot of sense (a client without a doorbell is something useless). Move it back there, and refactor the init_doorbell_hw consequently. Thanks to this, we can do some other improvements, like hoisting the check for GuC submission enabled out of the enable function. v2: Rebased. Cc: Michal Wajdeczko Cc: Arkadiusz Hiler Cc: Tvrtko Ursulin Cc: Chris Wilson Cc: Daniele Ceraolo Spurio Signed-off-by: Oscar Mateo Reviewed-by: Joonas Lahtinen --- drivers/gpu/drm/i915/i915_guc_submission.c | 231 ++++++++++++++++------------- drivers/gpu/drm/i915/intel_uc.c | 21 +-- 2 files changed, 138 insertions(+), 114 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 2d4d854..c9d324f 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -163,15 +163,13 @@ static struct guc_context_desc *__get_context_desc(struct i915_guc_client *clien * client object which contains the page being used for the doorbell */ -static int __update_doorbell_desc(struct i915_guc_client *client, u16 new_id) +static void __update_doorbell_desc(struct i915_guc_client *client, u16 new_id) { struct guc_context_desc *desc; /* Update the GuC's idea of the doorbell ID */ desc = __get_context_desc(client); desc->db_id = new_id; - - return 0; } static struct guc_doorbell_info *__get_doorbell(struct i915_guc_client *client) @@ -225,6 +223,28 @@ static int __destroy_doorbell(struct i915_guc_client *client) return __guc_deallocate_doorbell(client->guc, client->ctx_index); } +static int create_doorbell(struct i915_guc_client *client) +{ + int ret; + + ret = __reserve_doorbell(client); + if (ret) + return ret; + + __update_doorbell_desc(client, client->doorbell_id); + + ret = __create_doorbell(client); + if (ret) + goto err; + + return 0; + +err: + __update_doorbell_desc(client, GUC_DOORBELL_INVALID); + __unreserve_doorbell(client); + return ret; +} + static int destroy_doorbell(struct i915_guc_client *client) { int err; @@ -494,6 +514,17 @@ static void guc_wq_item_append(struct i915_guc_client *client, wqi->fence_id = rq->global_seqno; } +static void guc_reset_wq(struct i915_guc_client *client) +{ + struct guc_process_desc *desc = client->vaddr + + client->proc_desc_offset; + + desc->head = 0; + desc->tail = 0; + + client->wq_tail = 0; +} + static int guc_ring_doorbell(struct i915_guc_client *client) { struct guc_process_desc *desc; @@ -749,19 +780,6 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size) return vma; } -static void guc_client_free(struct i915_guc_client *client) -{ - /* - * XXX: wait for any outstanding submissions before freeing memory. - * Be sure to drop any locks - */ - guc_ctx_desc_fini(client->guc, client); - i915_gem_object_unpin_map(client->vma->obj); - i915_vma_unpin_and_release(&client->vma); - ida_simple_remove(&client->guc->ctx_ids, client->ctx_index); - kfree(client); -} - /* Check that a doorbell register is in the expected state */ static bool doorbell_ok(struct intel_guc *guc, u16 db_id) { @@ -792,9 +810,8 @@ static int __reset_doorbell(struct i915_guc_client* client, u16 db_id) { int err; - err = __update_doorbell_desc(client, db_id); - if (!err) - err = __create_doorbell(client); + __update_doorbell_desc(client, db_id); + err = __create_doorbell(client); if (!err) err = __destroy_doorbell(client); @@ -802,48 +819,61 @@ static int __reset_doorbell(struct i915_guc_client* client, u16 db_id) } /* - * Borrow the first client to set up & tear down each unused doorbell - * in turn, to ensure that all doorbell h/w is (re)initialised. + * Set up & tear down each unused doorbell in turn, to ensure that all doorbell + * HW is (re)initialised. For that end, we might have to borrow the first + * client. Also, tell GuC about all the doorbells in use by all clients. + * We do this because the KMD, the GuC and the doorbell HW can easily go out of + * sync (e.g. we can reset the GuC, but not the doorbel HW). */ static int guc_init_doorbell_hw(struct intel_guc *guc) { struct i915_guc_client *client = guc->execbuf_client; - int err; - int i; - - if (has_doorbell(client)) - destroy_doorbell(client); + bool recreate_first_client = false; + u16 db_id; + int ret; - for (i = 0; i < GUC_NUM_DOORBELLS; ++i) { - if (doorbell_ok(guc, i)) + /* For unused doorbells, make sure they are disabled */ + for_each_clear_bit(db_id, guc->doorbell_bitmap, GUC_NUM_DOORBELLS) { + if (doorbell_ok(guc, db_id)) continue; - err = __reset_doorbell(client, i); - WARN(err, "Doorbell %d reset failed, err %d\n", i, err); + if (has_doorbell(client)) { + /* Borrow execbuf_client (we will recreate it later) */ + destroy_doorbell(client); + recreate_first_client = true; + } + + ret = __reset_doorbell(client, db_id); + WARN(ret, "Doorbell %u reset failed, err %d\n", db_id, ret); } - /* Read back & verify all doorbell registers */ - for (i = 0; i < GUC_NUM_DOORBELLS; ++i) - WARN_ON(!doorbell_ok(guc, i)); + if (recreate_first_client) { + ret = __reserve_doorbell(client); + if (unlikely(ret)) { + DRM_ERROR("Couldn't re-reserve first client db: %d\n", ret); + return ret; + } - err = __reserve_doorbell(client); - if (err) - return err; + __update_doorbell_desc(client, client->doorbell_id); + } - err = __update_doorbell_desc(client, client->doorbell_id); - if (err) - goto err_reserve; + /* Now for every client (and not only execbuf_client) make sure their + * doorbells are known by the GuC */ + //for (client = client_list; client != NULL; client = client->next) + { + ret = __create_doorbell(client); + if (ret) { + DRM_ERROR("Couldn't recreate client %u doorbell: %d\n", + client->ctx_index, ret); + return ret; + } + } - err = __create_doorbell(client); - if (err) - goto err_update; + /* Read back & verify all (used & unused) doorbell registers */ + for (db_id = 0; db_id < GUC_NUM_DOORBELLS; ++db_id) + WARN_ON(!doorbell_ok(guc, db_id)); return 0; -err_reserve: - __unreserve_doorbell(client); -err_update: - __update_doorbell_desc(client, GUC_DOORBELL_INVALID); - return err; } /** @@ -923,11 +953,9 @@ static int guc_init_doorbell_hw(struct intel_guc *guc) guc_proc_desc_init(guc, client); guc_ctx_desc_init(guc, client); - /* FIXME: Runtime client allocation (which currently we don't do) will - * require that the doorbell gets created now. The static execbuf_client - * is now getting its doorbell later (on submission enable) but maybe we - * also want to reorder things in the future so that we don't have to - * special case the doorbell creation */ + ret = create_doorbell(client); + if (ret) + goto err_vaddr; DRM_DEBUG_DRIVER("new priority %u client %p for engine(s) 0x%x: ctx_index %u\n", priority, client, client->engines, client->ctx_index); @@ -935,6 +963,9 @@ static int guc_init_doorbell_hw(struct intel_guc *guc) client->doorbell_id, client->doorbell_offset); return client; + +err_vaddr: + i915_gem_object_unpin_map(client->vma->obj); err_vma: i915_vma_unpin_and_release(&client->vma); err_id: @@ -944,6 +975,24 @@ static int guc_init_doorbell_hw(struct intel_guc *guc) return ERR_PTR(ret); } +static void guc_client_free(struct i915_guc_client *client) +{ + /* + * XXX: wait for any outstanding submissions before freeing memory. + * Be sure to drop any locks + */ + + /* FIXME: in many cases, by the time we get here the GuC has been + * reset, so we cannot destroy the doorbell properly. Ignore the + * error message for now */ + destroy_doorbell(client); + guc_ctx_desc_fini(client->guc, client); + i915_gem_object_unpin_map(client->vma->obj); + i915_vma_unpin_and_release(&client->vma); + ida_simple_remove(&client->guc->ctx_ids, client->ctx_index); + kfree(client); +} + static void guc_policies_init(struct guc_policies *policies) { struct guc_policy *policy; @@ -1035,8 +1084,8 @@ static void guc_ads_destroy(struct intel_guc *guc) } /* - * Set up the memory resources to be shared with the GuC. At this point, - * we require just one object that can be mapped through the GGTT. + * Set up the memory resources to be shared with the GuC (via the GGTT) + * at firmware loading time. */ int i915_guc_submission_init(struct drm_i915_private *dev_priv) { @@ -1048,16 +1097,6 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv) void *vaddr; int ret; - if (!HAS_GUC_SCHED(dev_priv)) - return 0; - - /* Wipe bitmap & delete client in case of reinitialisation */ - bitmap_clear(guc->doorbell_bitmap, 0, GUC_NUM_DOORBELLS); - i915_guc_submission_disable(dev_priv); - - if (!i915.enable_guc_submission) - return 0; - if (guc->ctx_pool) return 0; @@ -1085,20 +1124,8 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv) ida_init(&guc->ctx_ids); - guc->execbuf_client = guc_client_alloc(dev_priv, - INTEL_INFO(dev_priv)->ring_mask, - GUC_CTX_PRIORITY_KMD_NORMAL, - dev_priv->kernel_context); - if (IS_ERR(guc->execbuf_client)) { - DRM_ERROR("Failed to create GuC client for execbuf!\n"); - ret = PTR_ERR(guc->execbuf_client); - goto err_ads; - } - return 0; -err_ads: - guc_ads_destroy(guc); err_log: intel_guc_log_destroy(guc); err_vaddr: @@ -1112,11 +1139,6 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv) { struct intel_guc *guc = &dev_priv->guc; - if (!i915.enable_guc_submission) - return 0; - - guc_client_free(guc->execbuf_client); - guc->execbuf_client = NULL; ida_destroy(&guc->ctx_ids); guc_ads_destroy(guc); intel_guc_log_destroy(guc); @@ -1124,17 +1146,6 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv) i915_vma_unpin_and_release(&guc->ctx_pool); } -static void guc_reset_wq(struct i915_guc_client *client) -{ - struct guc_process_desc *desc = client->vaddr + - client->proc_desc_offset; - - desc->head = 0; - desc->tail = 0; - - client->wq_tail = 0; -} - static void guc_interrupts_capture(struct drm_i915_private *dev_priv) { struct intel_engine_cs *engine; @@ -1185,17 +1196,28 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) enum intel_engine_id id; int err; - if (!client) - return -ENODEV; + if (!client) { + client = guc_client_alloc(dev_priv, + INTEL_INFO(dev_priv)->ring_mask, + GUC_CTX_PRIORITY_KMD_NORMAL, + dev_priv->kernel_context); + if (IS_ERR(client)) { + DRM_ERROR("Failed to create GuC client for execbuf!\n"); + return PTR_ERR(client); + } + + guc->execbuf_client = client; + } err = intel_guc_sample_forcewake(guc); if (err) - return err; + goto err_execbuf_client; guc_reset_wq(client); + err = guc_init_doorbell_hw(guc); if (err) - return err; + goto err_execbuf_client; /* Take over from manual control of ELSP (execlists) */ guc_interrupts_capture(dev_priv); @@ -1222,6 +1244,11 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv) } return 0; + +err_execbuf_client: + guc_client_free(guc->execbuf_client); + guc->execbuf_client = NULL; + return err; } static void guc_interrupts_release(struct drm_i915_private *dev_priv) @@ -1254,16 +1281,11 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv) guc_interrupts_release(dev_priv); - if (!guc->execbuf_client) - return; - - /* FIXME: in many cases, by the time we get here the GuC has been - * reset, so we cannot destroy the doorbell properly. Ignore the - * error message for now */ - destroy_doorbell(guc->execbuf_client); - /* Revert back to manual ELSP submission */ intel_engines_reset_default_submission(dev_priv); + + guc_client_free(guc->execbuf_client); + guc->execbuf_client = NULL; } /** @@ -1292,7 +1314,6 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv) return intel_guc_send(guc, data, ARRAY_SIZE(data)); } - /** * intel_guc_resume() - notify GuC resuming from suspend state * @dev_priv: i915 device private diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index ea99c89..db52594 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -126,13 +126,15 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) /* We need to notify the guc whenever we change the GGTT */ i915_ggtt_enable_guc(dev_priv); - /* - * This is stuff we need to have available at fw load time - * if we are planning to enable submission later - */ - ret = i915_guc_submission_init(dev_priv); - if (ret) - goto err_guc; + if (i915.enable_guc_submission) { + /* + * This is stuff we need to have available at fw load time + * if we are planning to enable submission later + */ + ret = i915_guc_submission_init(dev_priv); + if (ret) + goto err_guc; + } /* WaEnableuKernelHeaderValidFix:skl */ /* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */ @@ -187,7 +189,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) err_interrupts: gen9_disable_guc_interrupts(dev_priv); err_submission: - i915_guc_submission_fini(dev_priv); + if (i915.enable_guc_submission) + i915_guc_submission_fini(dev_priv); err_guc: i915_ggtt_disable_guc(dev_priv); @@ -210,8 +213,8 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) if (i915.enable_guc_submission) { i915_guc_submission_disable(dev_priv); gen9_disable_guc_interrupts(dev_priv); + i915_guc_submission_fini(dev_priv); } - i915_guc_submission_fini(dev_priv); i915_ggtt_disable_guc(dev_priv); }