On 04/07/2016 10:21 AM, Dave Gordon wrote:
During a hibernate/resume cycle, the driver, the GuC, and the doorbell
hardware can end up in inconsistent states. This patch refactors the
driver's handling and tracking of doorbells, in preparation for a later
one which will resolve the issue.

Signed-off-by: Dave Gordon <david.s.gor...@intel.com>
---
  drivers/gpu/drm/i915/i915_guc_submission.c | 88 ++++++++++++++++++------------
  1 file changed, 53 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 2171759..2fc69f1 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -175,8 +175,48 @@ static int host2guc_sample_forcewake(struct intel_guc *guc,
   * client object which contains the page being used for the doorbell
   */
+static int guc_update_doorbell_id(struct i915_guc_client *client,
+                                 struct guc_doorbell_info *doorbell,
+                                 u16 new_id)
+{
+       struct sg_table *sg = client->guc->ctx_pool_obj->pages;
+       void *doorbell_bitmap = client->guc->doorbell_bitmap;
+       struct guc_context_desc desc;
+       size_t len;
+
+       if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
+           test_bit(client->doorbell_id, doorbell_bitmap)) {
+               /* Deactivate the old doorbell */
+               doorbell->db_status = GUC_DOORBELL_DISABLED;
+               (void)host2guc_release_doorbell(client->guc, client);
+               clear_bit(client->doorbell_id, doorbell_bitmap);
+       }
+
+       /* Update the GuC's idea of the doorbell ID */
+       len = sg_pcopy_to_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
+                            sizeof(desc) * client->ctx_index);
+       if (len != sizeof(desc))
+               return -EFAULT;
+       desc.db_id = new_id;
+       len = sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
+                            sizeof(desc) * client->ctx_index);
+       if (len != sizeof(desc))
+               return -EFAULT;
+

We may cache the vmap of context pool for its life cycle to avoid these copies. That is why a generic vmap helper function is really nice to have.

Alex
+       client->doorbell_id = new_id;
+       if (new_id == GUC_INVALID_DOORBELL_ID)
+               return 0;
+
+       /* Activate the new doorbell */
+       set_bit(client->doorbell_id, doorbell_bitmap);
+       doorbell->db_status = GUC_DOORBELL_ENABLED;
+       doorbell->cookie = 0;
+       return host2guc_allocate_doorbell(client->guc, client);
+}
+
  static void guc_init_doorbell(struct intel_guc *guc,
-                             struct i915_guc_client *client)
+                             struct i915_guc_client *client,
+                             uint16_t db_id)
  {
        struct guc_doorbell_info *doorbell;
        void *base;
@@ -184,8 +224,7 @@ static void guc_init_doorbell(struct intel_guc *guc,
        base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
        doorbell = base + client->doorbell_offset;
- doorbell->db_status = 1;
-       doorbell->cookie = 0;
+       guc_update_doorbell_id(client, doorbell, db_id);
kunmap_atomic(base);
  }
@@ -193,27 +232,16 @@ static void guc_init_doorbell(struct intel_guc *guc,
  static void guc_disable_doorbell(struct intel_guc *guc,
                                 struct i915_guc_client *client)
  {
-       struct drm_i915_private *dev_priv = guc_to_i915(guc);
        struct guc_doorbell_info *doorbell;
        void *base;
-       i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
-       int value;
base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
        doorbell = base + client->doorbell_offset;
- doorbell->db_status = 0;
+       guc_update_doorbell_id(client, doorbell, GUC_INVALID_DOORBELL_ID);
kunmap_atomic(base); - I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
-
-       value = I915_READ(drbreg);
-       WARN_ON((value & GEN8_DRB_VALID) != 0);
-
-       I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0);
-       I915_WRITE(drbreg, 0);
-
        /* XXX: wait for any interrupts */
        /* XXX: wait for workqueue to drain */
  }
@@ -260,7 +288,7 @@ static uint16_t assign_doorbell(struct intel_guc *guc, 
uint32_t priority)
        if (id == end)
                id = GUC_INVALID_DOORBELL_ID;
        else
-               bitmap_set(guc->doorbell_bitmap, id, 1);
+               set_bit(id, guc->doorbell_bitmap);
DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
                        hi_pri ? "high" : "normal", id);
@@ -268,11 +296,6 @@ static uint16_t assign_doorbell(struct intel_guc *guc, 
uint32_t priority)
        return id;
  }
-static void release_doorbell(struct intel_guc *guc, uint16_t id)
-{
-       bitmap_clear(guc->doorbell_bitmap, id, 1);
-}
-
  /*
   * Initialise the process descriptor shared with the GuC firmware.
   */
@@ -667,15 +690,7 @@ static void guc_client_free(struct drm_device *dev,
        if (!client)
                return;
- if (client->doorbell_id != GUC_INVALID_DOORBELL_ID) {
-               /*
-                * First disable the doorbell, then tell the GuC we've
-                * finished with it, finally deallocate it in our bitmap
-                */
-               guc_disable_doorbell(guc, client);
-               host2guc_release_doorbell(guc, client);
-               release_doorbell(guc, client->doorbell_id);
-       }
+       guc_disable_doorbell(guc, client);
/*
         * XXX: wait for any outstanding submissions before freeing memory.
@@ -712,6 +727,7 @@ static struct i915_guc_client *guc_client_alloc(struct 
drm_device *dev,
        struct drm_i915_private *dev_priv = dev->dev_private;
        struct intel_guc *guc = &dev_priv->guc;
        struct drm_i915_gem_object *obj;
+       uint16_t db_id;
client = kzalloc(sizeof(*client), GFP_KERNEL);
        if (!client)
@@ -750,22 +766,24 @@ static struct i915_guc_client *guc_client_alloc(struct 
drm_device *dev,
        else
                client->proc_desc_offset = (GUC_DB_SIZE / 2);
- client->doorbell_id = assign_doorbell(guc, client->priority);
-       if (client->doorbell_id == GUC_INVALID_DOORBELL_ID)
+       db_id = assign_doorbell(guc, client->priority);
+       if (db_id == GUC_INVALID_DOORBELL_ID)
                /* XXX: evict a doorbell instead */
                goto err;
guc_init_proc_desc(guc, client);
        guc_init_ctx_desc(guc, client);
-       guc_init_doorbell(guc, client);
+       guc_init_doorbell(guc, client, db_id);
/* XXX: Any cache flushes needed? General domain mgmt calls? */ if (host2guc_allocate_doorbell(guc, client))
                goto err;
- DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u db_id %u\n",
-               priority, client, client->ctx_index, client->doorbell_id);
+       DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u\n",
+               priority, client, client->ctx_index);
+       DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%x\n",
+               client->doorbell_id, client->doorbell_offset);
return client;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to