drm/i915/guc: A little bit more of doorbell sanitization
authorOscar Mateo <oscar.mateo@intel.com>
Wed, 22 Mar 2017 17:39:52 +0000 (10:39 -0700)
committerJoonas Lahtinen <joonas.lahtinen@linux.intel.com>
Thu, 23 Mar 2017 12:58:29 +0000 (14:58 +0200)
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.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
drivers/gpu/drm/i915/i915_guc_submission.c
drivers/gpu/drm/i915/intel_uc.c

index 60add049b53129f3d14b8025f9268fdf243f2237..e2ad365679c3291f9c617cd65f55c244646de337 100644 (file)
@@ -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;
@@ -748,19 +779,6 @@ err:
        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)
 {
@@ -791,9 +809,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);
 
@@ -801,48 +818,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;
 }
 
 /**
@@ -922,11 +952,9 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
        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);
@@ -934,6 +962,9 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
                         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:
@@ -943,6 +974,24 @@ err_client:
        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;
@@ -1034,8 +1083,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)
 {
@@ -1047,16 +1096,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;
 
@@ -1084,20 +1123,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:
@@ -1111,11 +1138,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);
@@ -1123,17 +1145,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;
@@ -1184,17 +1195,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);
@@ -1221,6 +1243,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)
@@ -1253,16 +1280,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;
 }
 
 /**
@@ -1291,7 +1313,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
index 0b8b2505a44a3ea1b0af6d1e5a0139541c57a6f8..4a872cdf57e8181f0f1103824dfa5d3ceb117aa9 100644 (file)
@@ -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);
 }