diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c index 8f4b274d66a77cccbf6dae7efa34549ed3cb08ed..c74e78b6ba4128e3b52d22cc0eb465b43a870e87 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; @@ -225,50 +214,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); + 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); __update_doorbell_desc(client, GUC_DOORBELL_INVALID); - __unreserve_doorbell(client); - - return 0; + return ret; } static unsigned long __select_cacheline(struct intel_guc *guc) @@ -839,73 +820,18 @@ 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) -{ - int err; - - __update_doorbell_desc(client, db_id); - err = __create_doorbell(client); - if (!err) - err = __destroy_doorbell(client); - - return err; -} - -/* - * 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; } @@ -916,6 +842,19 @@ static int guc_init_doorbell_hw(struct intel_guc *guc) return 0; } +static void guc_clients_doorbell_fini(struct intel_guc *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 +930,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 +954,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); @@ -1366,7 +1296,7 @@ int intel_guc_submission_enable(struct intel_guc *guc) if (err) goto err_free_clients; - err = guc_init_doorbell_hw(guc); + err = guc_clients_doorbell_init(guc); if (err) goto err_free_clients; @@ -1398,6 +1328,7 @@ 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); diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c b/drivers/gpu/drm/i915/selftests/intel_guc.c index 68d6a69c738fdc836542f068bbd5eb3fb2c784ea..3f9016466deaf74ebfb64587a5f6006a5202c249 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 client_doorbell_in_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 (!client_doorbell_in_sync(guc->execbuf_client) || + !client_doorbell_in_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 (client_doorbell_in_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 doorbell 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), };