[PATCH 6/6] doorbell-cleanup
Michał Winiarski
michal.winiarski at intel.com
Tue Dec 12 21:13:06 UTC 2017
---
drivers/gpu/drm/i915/intel_guc_submission.c | 193 +++++++++-------------------
drivers/gpu/drm/i915/selftests/intel_guc.c | 110 +++++++---------
2 files changed, 110 insertions(+), 193 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 8f4b274d66a7..2f01839a6cb2 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -88,7 +88,7 @@ static inline bool is_high_priority(struct intel_guc_client *client)
client->priority == GUC_CLIENT_PRIORITY_HIGH);
}
-static int __reserve_doorbell(struct intel_guc_client *client)
+static int reserve_doorbell(struct intel_guc_client *client)
{
unsigned long offset;
unsigned long end;
@@ -120,7 +120,7 @@ static int __reserve_doorbell(struct intel_guc_client *client)
return 0;
}
-static void __unreserve_doorbell(struct intel_guc_client *client)
+static void unreserve_doorbell(struct intel_guc_client *client)
{
GEM_BUG_ON(client->doorbell_id == GUC_DOORBELL_INVALID);
@@ -188,32 +188,21 @@ static bool has_doorbell(struct intel_guc_client *client)
return test_bit(client->doorbell_id, client->guc->doorbell_bitmap);
}
-static int __create_doorbell(struct intel_guc_client *client)
+static void __create_doorbell(struct intel_guc_client *client)
{
struct guc_doorbell_info *doorbell;
- int err;
doorbell = __get_doorbell(client);
doorbell->db_status = GUC_DOORBELL_ENABLED;
doorbell->cookie = 0;
-
- err = __guc_allocate_doorbell(client->guc, client->stage_id);
- if (err) {
- doorbell->db_status = GUC_DOORBELL_DISABLED;
- DRM_ERROR("Couldn't create client %u doorbell: %d\n",
- client->stage_id, err);
- }
-
- return err;
}
-static int __destroy_doorbell(struct intel_guc_client *client)
+static void __destroy_doorbell(struct intel_guc_client *client)
{
struct drm_i915_private *dev_priv = guc_to_i915(client->guc);
struct guc_doorbell_info *doorbell;
u16 db_id = client->doorbell_id;
- GEM_BUG_ON(db_id >= GUC_DOORBELL_INVALID);
doorbell = __get_doorbell(client);
doorbell->db_status = GUC_DOORBELL_DISABLED;
@@ -226,47 +215,42 @@ static int __destroy_doorbell(struct intel_guc_client *client)
if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID), 10))
WARN_ONCE(true, "Doorbell never became invalid after disable\n");
- return __guc_deallocate_doorbell(client->guc, client->stage_id);
}
static int create_doorbell(struct intel_guc_client *client)
{
int ret;
- ret = __reserve_doorbell(client);
- if (ret)
- return ret;
-
__update_doorbell_desc(client, client->doorbell_id);
+ __create_doorbell(client);
- ret = __create_doorbell(client);
- if (ret)
- goto err;
+ ret = __guc_allocate_doorbell(client->guc, client->stage_id);
+ if (ret) {
+ __destroy_doorbell(client);
+ __update_doorbell_desc(client, GUC_DOORBELL_INVALID);
+ DRM_ERROR("Couldn't create client %u doorbell: %d\n",
+ client->stage_id, ret);
+ return ret;
+ }
return 0;
-
-err:
- __update_doorbell_desc(client, GUC_DOORBELL_INVALID);
- __unreserve_doorbell(client);
- return ret;
}
static int destroy_doorbell(struct intel_guc_client *client)
{
- int err;
+ int ret;
GEM_BUG_ON(!has_doorbell(client));
- /* XXX: wait for any interrupts */
- /* XXX: wait for workqueue to drain */
-
- err = __destroy_doorbell(client);
- if (err)
- return err;
-
+ __destroy_doorbell(client);
__update_doorbell_desc(client, GUC_DOORBELL_INVALID);
- __unreserve_doorbell(client);
+ ret = __guc_deallocate_doorbell(client->guc, client->stage_id);
+ if (ret) {
+ DRM_ERROR("Couldn't destroy client %u doorbell: %d\n",
+ client->stage_id, ret);
+ return ret;
+ }
return 0;
}
@@ -839,83 +823,50 @@ static bool doorbell_ok(struct intel_guc *guc, u16 db_id)
return false;
}
-/*
- * If the GuC thinks that the doorbell is unassigned (e.g. because we reset and
- * reloaded the GuC FW) we can use this function to tell the GuC to reassign the
- * doorbell to the rightful owner.
- */
-static int __reset_doorbell(struct intel_guc_client *client, u16 db_id)
+static bool guc_verify_doorbells(struct intel_guc *guc)
{
- int err;
+ u16 db_id;
- __update_doorbell_desc(client, db_id);
- err = __create_doorbell(client);
- if (!err)
- err = __destroy_doorbell(client);
+ /* Read back & verify all (used & unused) doorbell registers */
+ for (db_id = 0; db_id < GUC_NUM_DOORBELLS; ++db_id)
+ if (!doorbell_ok(guc, db_id))
+ return false;
- return err;
+ return true;
}
-/*
- * 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)
+static int guc_clients_doorbell_init(struct intel_guc *guc)
{
- struct intel_guc_client *client = guc->execbuf_client;
- bool recreate_first_client = false;
- u16 db_id;
int ret;
- /* 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;
-
- 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);
- }
-
- 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;
- }
-
- __update_doorbell_desc(client, client->doorbell_id);
- }
-
- /* Now for every client (and not only execbuf_client) make sure their
- * doorbells are known by the GuC
- */
- ret = __create_doorbell(guc->execbuf_client);
+ ret = create_doorbell(guc->execbuf_client);
if (ret)
return ret;
- ret = __create_doorbell(guc->preempt_client);
+ ret = create_doorbell(guc->preempt_client);
if (ret) {
- __destroy_doorbell(guc->execbuf_client);
+ destroy_doorbell(guc->execbuf_client);
return ret;
}
- /* 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;
}
+static void guc_clients_doorbell_fini(struct intel_guc *guc)
+{
+ GEM_WARN_ON(!guc_verify_doorbells(guc));
+
+ /*
+ * By the time we're here, GuC has already been reset.
+ * Instead of trying (in vain) to communicate with it, let's just
+ * cleanup the doorbell HW and our internal state.
+ */
+ __destroy_doorbell(guc->preempt_client);
+ __update_doorbell_desc(guc->preempt_client, GUC_DOORBELL_INVALID);
+ __destroy_doorbell(guc->execbuf_client);
+ __update_doorbell_desc(guc->execbuf_client, GUC_DOORBELL_INVALID);
+}
+
/**
* guc_client_alloc() - Allocate an intel_guc_client
* @dev_priv: driver private data structure
@@ -991,7 +942,7 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
guc_proc_desc_init(guc, client);
guc_stage_desc_init(guc, client);
- ret = create_doorbell(client);
+ ret = reserve_doorbell(client);
if (ret)
goto err_vaddr;
@@ -1015,16 +966,7 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
static void guc_client_free(struct intel_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);
+ unreserve_doorbell(client);
guc_stage_desc_fini(client->guc, client);
i915_gem_object_unpin_map(client->vma->obj);
i915_vma_unpin_and_release(&client->vma);
@@ -1219,6 +1161,11 @@ int intel_guc_submission_init(struct intel_guc *guc)
goto err_log;
GEM_BUG_ON(!guc->ads_vma);
+ GEM_WARN_ON(!guc_verify_doorbells(guc));
+ ret = guc_clients_create(guc);
+ if (ret)
+ return ret;
+
for_each_engine(engine, dev_priv, id) {
guc->preempt_work[id].engine = engine;
INIT_WORK(&guc->preempt_work[id].work, inject_preempt_context);
@@ -1242,6 +1189,7 @@ void intel_guc_submission_fini(struct intel_guc *guc)
for_each_engine(engine, dev_priv, id)
cancel_work_sync(&guc->preempt_work[id].work);
+ guc_clients_destroy(guc);
guc_ads_destroy(guc);
intel_guc_log_destroy(guc);
guc_stage_desc_pool_destroy(guc);
@@ -1347,28 +1295,18 @@ int intel_guc_submission_enable(struct intel_guc *guc)
sizeof(struct guc_wq_item) *
I915_NUM_ENGINES > GUC_WQ_SIZE);
- /*
- * We're being called on both module initialization and on reset,
- * until this flow is changed, we're using regular client presence to
- * determine which case are we in, and whether we should allocate new
- * clients or just reset their workqueues.
- */
- if (!guc->execbuf_client) {
- err = guc_clients_create(guc);
- if (err)
- return err;
- } else {
- guc_reset_wq(guc->execbuf_client);
- guc_reset_wq(guc->preempt_client);
- }
+ GEM_BUG_ON(!guc->execbuf_client);
+
+ guc_reset_wq(guc->execbuf_client);
+ guc_reset_wq(guc->preempt_client);
err = intel_guc_sample_forcewake(guc);
if (err)
- goto err_free_clients;
+ return err;
- err = guc_init_doorbell_hw(guc);
+ err = guc_clients_doorbell_init(guc);
if (err)
- goto err_free_clients;
+ return err;
/* Take over from manual control of ELSP (execlists) */
guc_interrupts_capture(dev_priv);
@@ -1385,10 +1323,6 @@ int intel_guc_submission_enable(struct intel_guc *guc)
}
return 0;
-
-err_free_clients:
- guc_clients_destroy(guc);
- return err;
}
void intel_guc_submission_disable(struct intel_guc *guc)
@@ -1398,11 +1332,10 @@ void intel_guc_submission_disable(struct intel_guc *guc)
GEM_BUG_ON(dev_priv->gt.awake); /* GT should be parked first */
guc_interrupts_release(dev_priv);
+ guc_clients_doorbell_fini(guc);
/* Revert back to manual ELSP submission */
intel_engines_reset_default_submission(dev_priv);
-
- guc_clients_destroy(guc);
}
#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c b/drivers/gpu/drm/i915/selftests/intel_guc.c
index 68d6a69c738f..8900a6c06cac 100644
--- a/drivers/gpu/drm/i915/selftests/intel_guc.c
+++ b/drivers/gpu/drm/i915/selftests/intel_guc.c
@@ -85,21 +85,26 @@ static int validate_client(struct intel_guc_client *client,
return 0;
}
+static bool doorbell_sync(struct intel_guc_client *client)
+{
+ return doorbell_ok(client->guc, client->doorbell_id);
+}
+
/*
- * Check that guc_init_doorbell_hw is doing what it should.
+ * Check that we're able to synchronize guc_clients with their doorbells
*
- * During GuC submission enable, we create GuC clients and their doorbells,
- * but after resetting the microcontroller (resume & gpu reset), these
- * GuC clients are still around, but the status of their doorbells may be
- * incorrect. This is the reason behind validating that the doorbells status
- * expected by the driver matches what the GuC/HW have.
+ * We're creating clients and reserving doorbells once, at module load. During
+ * module lifetime, GuC, doorbell HW, and i915 state may go out of sync due to
+ * GuC being reset. In other words - GuC clients are still around, but the
+ * status of their doorbells may be incorrect. This is the reason behind
+ * validating that the doorbells status expected by the driver matches what the
+ * GuC/HW have.
*/
-static int igt_guc_init_doorbell_hw(void *args)
+static int igt_guc_clients(void *args)
{
struct drm_i915_private *dev_priv = args;
struct intel_guc *guc;
- DECLARE_BITMAP(db_bitmap_bk, GUC_NUM_DOORBELLS);
- int i, err = 0;
+ int err = 0;
GEM_BUG_ON(!HAS_GUC(dev_priv));
mutex_lock(&dev_priv->drm.struct_mutex);
@@ -148,10 +153,21 @@ static int igt_guc_init_doorbell_hw(void *args)
goto out;
}
- /* each client should have received a doorbell during alloc */
+ /* each client should now have reserved a doorbell */
if (!has_doorbell(guc->execbuf_client) ||
!has_doorbell(guc->preempt_client)) {
- pr_err("guc_clients_create didn't create doorbells\n");
+ pr_err("guc_clients_create didn't reserve doorbells\n");
+ err = -EINVAL;
+ goto out;
+ }
+
+ /* Now create the doorbells */
+ guc_clients_doorbell_init(guc);
+
+ /* each client should now have received a doorbell */
+ if (!doorbell_sync(guc->execbuf_client) ||
+ !doorbell_sync(guc->preempt_client)) {
+ pr_err("failed to initialize the doorbells\n");
err = -EINVAL;
goto out;
}
@@ -160,25 +176,26 @@ static int igt_guc_init_doorbell_hw(void *args)
* Basic test - an attempt to reallocate a valid doorbell to the
* client it is currently assigned should not cause a failure.
*/
- err = guc_init_doorbell_hw(guc);
+ err = guc_clients_doorbell_init(guc);
if (err)
goto out;
/*
* Negative test - a client with no doorbell (invalid db id).
- * Each client gets a doorbell when it is created, after destroying
- * the doorbell, the db id is changed to GUC_DOORBELL_INVALID and the
- * firmware will reject any attempt to allocate a doorbell with an
- * invalid id (db has to be reserved before allocation).
+ * After destroying the doorbell, the db id is changed to
+ * GUC_DOORBELL_INVALID and the firmware will reject any attempt to
+ * allocate a doorbell with an invalid id (db has to be reserved before
+ * allocation).
*/
destroy_doorbell(guc->execbuf_client);
- if (has_doorbell(guc->execbuf_client)) {
+ if (doorbell_sync(guc->execbuf_client)) {
pr_err("destroy db did not work\n");
err = -EINVAL;
goto out;
}
- err = guc_init_doorbell_hw(guc);
+ unreserve_doorbell(guc->execbuf_client);
+ err = guc_clients_doorbell_init(guc);
if (err != -EIO) {
pr_err("unexpected (err = %d)", err);
goto out;
@@ -191,33 +208,13 @@ static int igt_guc_init_doorbell_hw(void *args)
}
/* clean after test */
- err = create_doorbell(guc->execbuf_client);
- if (err) {
- pr_err("recreate doorbell failed\n");
- goto out;
- }
-
- /*
- * Negative test - doorbell_bitmap out of sync, will trigger a few of
- * WARN_ON(!doorbell_ok(guc, db_id)) but that's ok as long as the
- * doorbells from our clients don't fail.
- */
- bitmap_copy(db_bitmap_bk, guc->doorbell_bitmap, GUC_NUM_DOORBELLS);
- for (i = 0; i < GUC_NUM_DOORBELLS; i++)
- if (i % 2)
- test_and_change_bit(i, guc->doorbell_bitmap);
-
- err = guc_init_doorbell_hw(guc);
+ err = reserve_doorbell(guc->execbuf_client);
if (err) {
- pr_err("out of sync doorbell caused an error\n");
- goto out;
+ pr_err("failed to reserve back the doorbel back\n");
}
-
- /* restore 'correct' db bitmap */
- bitmap_copy(guc->doorbell_bitmap, db_bitmap_bk, GUC_NUM_DOORBELLS);
- err = guc_init_doorbell_hw(guc);
+ err = create_doorbell(guc->execbuf_client);
if (err) {
- pr_err("restored doorbell caused an error\n");
+ pr_err("recreate doorbell failed\n");
goto out;
}
@@ -226,8 +223,11 @@ static int igt_guc_init_doorbell_hw(void *args)
* Leave clean state for other test, plus the driver always destroy the
* clients during unload.
*/
+ destroy_doorbell(guc->execbuf_client);
+ destroy_doorbell(guc->preempt_client);
guc_clients_destroy(guc);
guc_clients_create(guc);
+ guc_clients_doorbell_init(guc);
unlock:
mutex_unlock(&dev_priv->drm.struct_mutex);
return err;
@@ -309,25 +309,7 @@ static int igt_guc_doorbells(void *arg)
db_id = clients[i]->doorbell_id;
- /*
- * Client alloc gives us a doorbell, but we want to exercise
- * this ourselves (this resembles guc_init_doorbell_hw)
- */
- destroy_doorbell(clients[i]);
- if (clients[i]->doorbell_id != GUC_DOORBELL_INVALID) {
- pr_err("[%d] destroy db did not work!\n", i);
- err = -EINVAL;
- goto out;
- }
-
- err = __reserve_doorbell(clients[i]);
- if (err) {
- pr_err("[%d] Failed to reserve a doorbell\n", i);
- goto out;
- }
-
- __update_doorbell_desc(clients[i], clients[i]->doorbell_id);
- err = __create_doorbell(clients[i]);
+ err = create_doorbell(clients[i]);
if (err) {
pr_err("[%d] Failed to create a doorbell\n", i);
goto out;
@@ -348,8 +330,10 @@ static int igt_guc_doorbells(void *arg)
out:
for (i = 0; i < ATTEMPTS; i++)
- if (!IS_ERR_OR_NULL(clients[i]))
+ if (!IS_ERR_OR_NULL(clients[i])) {
+ destroy_doorbell(clients[i]);
guc_client_free(clients[i]);
+ }
unlock:
mutex_unlock(&dev_priv->drm.struct_mutex);
return err;
@@ -358,7 +342,7 @@ static int igt_guc_doorbells(void *arg)
int intel_guc_live_selftest(struct drm_i915_private *dev_priv)
{
static const struct i915_subtest tests[] = {
- SUBTEST(igt_guc_init_doorbell_hw),
+ SUBTEST(igt_guc_clients),
SUBTEST(igt_guc_doorbells),
};
--
2.14.3
More information about the Intel-gfx-trybot
mailing list