On 08/06/16 11:55, 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.
Perhaps a few word on the goal, method and result of refactoring. Would
be good for documentation and easier review.
Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 87 ++++++++++++++++++------------
1 file changed, 53 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 7510841..eef67c8 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -174,36 +174,63 @@ static int host2guc_sample_forcewake(struct intel_guc *guc,
* client object which contains the page being used for the doorbell
*/
-static void guc_init_doorbell(struct intel_guc *guc,
- struct i915_guc_client *client)
+static int guc_update_doorbell_id(struct i915_guc_client *client,
+ struct guc_doorbell_info *doorbell,
+ u16 new_id)
{
- struct guc_doorbell_info *doorbell;
+ 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);
+ }
- doorbell = client->client_base + client->doorbell_offset;
+ /* 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;
+
+ 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);
A bunch of new code here which wasn't done on doorbell init before.
Populating the ctx_pool_obj and hust2guc call. I think the commit needs
to explain what is happening here.
Maybe it is mostly a matter of copying over some text from the cover
letter, but ideally it should explain what it is doing more precisely.
}
-static void guc_disable_doorbell(struct intel_guc *guc,
- struct i915_guc_client *client)
+static void guc_init_doorbell(struct intel_guc *guc,
+ struct i915_guc_client *client,
+ uint16_t db_id)
{
- struct drm_i915_private *dev_priv = guc_to_i915(guc);
struct guc_doorbell_info *doorbell;
- i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
- int value;
doorbell = client->client_base + client->doorbell_offset;
- doorbell->db_status = GUC_DOORBELL_DISABLED;
-
- I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
+ guc_update_doorbell_id(client, doorbell, db_id);
+}
This function does not end up doing anything now, just a pass-through to
guc_update_doorbell_id.
What happens to the write to drbreg ? It is completely gone and also
from the init doorbell path together with the write to another register.
- value = I915_READ(drbreg);
- WARN_ON((value & GEN8_DRB_VALID) != 0);
+static void guc_disable_doorbell(struct intel_guc *guc,
+ struct i915_guc_client *client)
+{
+ struct guc_doorbell_info *doorbell;
- I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0);
- I915_WRITE(drbreg, 0);
+ doorbell = client->client_base + client->doorbell_offset;
+ guc_update_doorbell_id(client, doorbell, GUC_INVALID_DOORBELL_ID);
/* XXX: wait for any interrupts */
/* XXX: wait for workqueue to drain */
@@ -251,7 +278,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);
set_bit is atomic - is there a reason it is required here?
Actually the same question for the clear_bit above, where it was
bitmap_clear before.
DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
hi_pri ? "high" : "normal", id);
@@ -259,11 +286,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.
*/
@@ -657,8 +679,6 @@ static void guc_client_free(struct drm_device *dev,
*/
if (client->client_base) {
- uint16_t db_id = client->doorbell_id;
-
/*
* If we got as far as setting up a doorbell, make sure
* we shut it down before unmapping & deallocating the
@@ -666,12 +686,8 @@ static void guc_client_free(struct drm_device *dev,
* GuC that we've finished with it, finally deallocate
* it in our bitmap
*/
- if (db_id != GUC_INVALID_DOORBELL_ID) {
+ if (client->doorbell_id != GUC_INVALID_DOORBELL_ID)
guc_disable_doorbell(guc, client);
- if (test_bit(db_id, guc->doorbell_bitmap))
- host2guc_release_doorbell(guc, client);
- release_doorbell(guc, db_id);
- }
kunmap(kmap_to_page(client->client_base));
}
@@ -706,6 +722,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)
@@ -746,22 +763,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;
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
