Commit 9192d4fb authored by Michał Winiarski's avatar Michał Winiarski Committed by Chris Wilson

drm/i915/guc: Extract doorbell creation from client allocation

Full GPU reset causes GuC to be reset. This means that every time we're
doing a reset, we need to talk to GuC and tell it about doorbells.
Let's separate the communication part (create_doorbell) from our
internal bookkeeping (reserve_doorbell) so that we can cleanly separate
the initialization done at module load from reinitialization done at
reset in the following patch.
While I'm here, let's also add a proper (although slightly asymetric)
cleanup that doesn't try to communicate with GuC after it's already
gone, getting rid of "expected" warnings caused by GuC action failures
on module unload.

Note that I've also removed one of the tests (bitmap out of sync), since
it doesn't make much sense anymore - bitmaps are now not expected to
change during the lifetime of a client.
Signed-off-by: default avatarMichał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Reviewed-by: default avatarMichel Thierry <michel.thierry@intel.com>
Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
Link: https://patchwork.freedesktop.org/patch/msgid/20171213221352.7173-5-michal.winiarski@intel.com
parent aeb950bd
...@@ -88,7 +88,7 @@ static inline bool is_high_priority(struct intel_guc_client *client) ...@@ -88,7 +88,7 @@ static inline bool is_high_priority(struct intel_guc_client *client)
client->priority == GUC_CLIENT_PRIORITY_HIGH); 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 offset;
unsigned long end; unsigned long end;
...@@ -120,7 +120,7 @@ static int __reserve_doorbell(struct intel_guc_client *client) ...@@ -120,7 +120,7 @@ static int __reserve_doorbell(struct intel_guc_client *client)
return 0; 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); GEM_BUG_ON(client->doorbell_id == GUC_DOORBELL_INVALID);
...@@ -188,32 +188,21 @@ static bool has_doorbell(struct intel_guc_client *client) ...@@ -188,32 +188,21 @@ static bool has_doorbell(struct intel_guc_client *client)
return test_bit(client->doorbell_id, client->guc->doorbell_bitmap); 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; struct guc_doorbell_info *doorbell;
int err;
doorbell = __get_doorbell(client); doorbell = __get_doorbell(client);
doorbell->db_status = GUC_DOORBELL_ENABLED; doorbell->db_status = GUC_DOORBELL_ENABLED;
doorbell->cookie = 0; 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 drm_i915_private *dev_priv = guc_to_i915(client->guc);
struct guc_doorbell_info *doorbell; struct guc_doorbell_info *doorbell;
u16 db_id = client->doorbell_id; u16 db_id = client->doorbell_id;
GEM_BUG_ON(db_id >= GUC_DOORBELL_INVALID);
doorbell = __get_doorbell(client); doorbell = __get_doorbell(client);
doorbell->db_status = GUC_DOORBELL_DISABLED; doorbell->db_status = GUC_DOORBELL_DISABLED;
...@@ -225,50 +214,42 @@ static int __destroy_doorbell(struct intel_guc_client *client) ...@@ -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)) if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID), 10))
WARN_ONCE(true, "Doorbell never became invalid after disable\n"); 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) static int create_doorbell(struct intel_guc_client *client)
{ {
int ret; int ret;
ret = __reserve_doorbell(client);
if (ret)
return ret;
__update_doorbell_desc(client, client->doorbell_id); __update_doorbell_desc(client, client->doorbell_id);
__create_doorbell(client);
ret = __create_doorbell(client); ret = __guc_allocate_doorbell(client->guc, client->stage_id);
if (ret) if (ret) {
goto err; __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; return 0;
err:
__update_doorbell_desc(client, GUC_DOORBELL_INVALID);
__unreserve_doorbell(client);
return ret;
} }
static int destroy_doorbell(struct intel_guc_client *client) static int destroy_doorbell(struct intel_guc_client *client)
{ {
int err; int ret;
GEM_BUG_ON(!has_doorbell(client)); GEM_BUG_ON(!has_doorbell(client));
/* XXX: wait for any interrupts */ __destroy_doorbell(client);
/* XXX: wait for workqueue to drain */ ret = __guc_deallocate_doorbell(client->guc, client->stage_id);
if (ret)
err = __destroy_doorbell(client); DRM_ERROR("Couldn't destroy client %u doorbell: %d\n",
if (err) client->stage_id, ret);
return err;
__update_doorbell_desc(client, GUC_DOORBELL_INVALID); __update_doorbell_desc(client, GUC_DOORBELL_INVALID);
__unreserve_doorbell(client); return ret;
return 0;
} }
static unsigned long __select_cacheline(struct intel_guc *guc) static unsigned long __select_cacheline(struct intel_guc *guc)
...@@ -839,73 +820,18 @@ static bool doorbell_ok(struct intel_guc *guc, u16 db_id) ...@@ -839,73 +820,18 @@ static bool doorbell_ok(struct intel_guc *guc, u16 db_id)
return false; return false;
} }
/* static int guc_clients_doorbell_init(struct intel_guc *guc)
* 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)
{ {
struct intel_guc_client *client = guc->execbuf_client;
bool recreate_first_client = false;
u16 db_id; u16 db_id;
int ret; int ret;
/* For unused doorbells, make sure they are disabled */ ret = create_doorbell(guc->execbuf_client);
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);
if (ret) if (ret)
return ret; return ret;
ret = __create_doorbell(guc->preempt_client); ret = create_doorbell(guc->preempt_client);
if (ret) { if (ret) {
__destroy_doorbell(guc->execbuf_client); destroy_doorbell(guc->execbuf_client);
return ret; return ret;
} }
...@@ -916,6 +842,19 @@ static int guc_init_doorbell_hw(struct intel_guc *guc) ...@@ -916,6 +842,19 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
return 0; 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 * guc_client_alloc() - Allocate an intel_guc_client
* @dev_priv: driver private data structure * @dev_priv: driver private data structure
...@@ -991,7 +930,7 @@ guc_client_alloc(struct drm_i915_private *dev_priv, ...@@ -991,7 +930,7 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
guc_proc_desc_init(guc, client); guc_proc_desc_init(guc, client);
guc_stage_desc_init(guc, client); guc_stage_desc_init(guc, client);
ret = create_doorbell(client); ret = reserve_doorbell(client);
if (ret) if (ret)
goto err_vaddr; goto err_vaddr;
...@@ -1015,16 +954,7 @@ guc_client_alloc(struct drm_i915_private *dev_priv, ...@@ -1015,16 +954,7 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
static void guc_client_free(struct intel_guc_client *client) static void guc_client_free(struct intel_guc_client *client)
{ {
/* unreserve_doorbell(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_stage_desc_fini(client->guc, client); guc_stage_desc_fini(client->guc, client);
i915_gem_object_unpin_map(client->vma->obj); i915_gem_object_unpin_map(client->vma->obj);
i915_vma_unpin_and_release(&client->vma); i915_vma_unpin_and_release(&client->vma);
...@@ -1366,7 +1296,7 @@ int intel_guc_submission_enable(struct intel_guc *guc) ...@@ -1366,7 +1296,7 @@ int intel_guc_submission_enable(struct intel_guc *guc)
if (err) if (err)
goto err_free_clients; goto err_free_clients;
err = guc_init_doorbell_hw(guc); err = guc_clients_doorbell_init(guc);
if (err) if (err)
goto err_free_clients; goto err_free_clients;
...@@ -1398,6 +1328,7 @@ void intel_guc_submission_disable(struct intel_guc *guc) ...@@ -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 */ GEM_BUG_ON(dev_priv->gt.awake); /* GT should be parked first */
guc_interrupts_release(dev_priv); guc_interrupts_release(dev_priv);
guc_clients_doorbell_fini(guc);
/* Revert back to manual ELSP submission */ /* Revert back to manual ELSP submission */
intel_engines_reset_default_submission(dev_priv); intel_engines_reset_default_submission(dev_priv);
......
...@@ -85,21 +85,26 @@ static int validate_client(struct intel_guc_client *client, ...@@ -85,21 +85,26 @@ static int validate_client(struct intel_guc_client *client,
return 0; 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, * We're creating clients and reserving doorbells once, at module load. During
* but after resetting the microcontroller (resume & gpu reset), these * module lifetime, GuC, doorbell HW, and i915 state may go out of sync due to
* GuC clients are still around, but the status of their doorbells may be * GuC being reset. In other words - GuC clients are still around, but the
* incorrect. This is the reason behind validating that the doorbells status * status of their doorbells may be incorrect. This is the reason behind
* expected by the driver matches what the GuC/HW have. * 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 drm_i915_private *dev_priv = args;
struct intel_guc *guc; struct intel_guc *guc;
DECLARE_BITMAP(db_bitmap_bk, GUC_NUM_DOORBELLS); int err = 0;
int i, err = 0;
GEM_BUG_ON(!HAS_GUC(dev_priv)); GEM_BUG_ON(!HAS_GUC(dev_priv));
mutex_lock(&dev_priv->drm.struct_mutex); mutex_lock(&dev_priv->drm.struct_mutex);
...@@ -148,10 +153,21 @@ static int igt_guc_init_doorbell_hw(void *args) ...@@ -148,10 +153,21 @@ static int igt_guc_init_doorbell_hw(void *args)
goto out; 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) || if (!has_doorbell(guc->execbuf_client) ||
!has_doorbell(guc->preempt_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; err = -EINVAL;
goto out; goto out;
} }
...@@ -160,25 +176,26 @@ static int igt_guc_init_doorbell_hw(void *args) ...@@ -160,25 +176,26 @@ static int igt_guc_init_doorbell_hw(void *args)
* Basic test - an attempt to reallocate a valid doorbell to the * Basic test - an attempt to reallocate a valid doorbell to the
* client it is currently assigned should not cause a failure. * client it is currently assigned should not cause a failure.
*/ */
err = guc_init_doorbell_hw(guc); err = guc_clients_doorbell_init(guc);
if (err) if (err)
goto out; goto out;
/* /*
* Negative test - a client with no doorbell (invalid db id). * Negative test - a client with no doorbell (invalid db id).
* Each client gets a doorbell when it is created, after destroying * After destroying the doorbell, the db id is changed to
* the doorbell, the db id is changed to GUC_DOORBELL_INVALID and the * GUC_DOORBELL_INVALID and the firmware will reject any attempt to
* firmware will reject any attempt to allocate a doorbell with an * allocate a doorbell with an invalid id (db has to be reserved before
* invalid id (db has to be reserved before allocation). * allocation).
*/ */
destroy_doorbell(guc->execbuf_client); 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"); pr_err("destroy db did not work\n");
err = -EINVAL; err = -EINVAL;
goto out; goto out;
} }
err = guc_init_doorbell_hw(guc); unreserve_doorbell(guc->execbuf_client);
err = guc_clients_doorbell_init(guc);
if (err != -EIO) { if (err != -EIO) {
pr_err("unexpected (err = %d)", err); pr_err("unexpected (err = %d)", err);
goto out; goto out;
...@@ -191,33 +208,13 @@ static int igt_guc_init_doorbell_hw(void *args) ...@@ -191,33 +208,13 @@ static int igt_guc_init_doorbell_hw(void *args)
} }
/* clean after test */ /* clean after test */
err = create_doorbell(guc->execbuf_client); err = reserve_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);
if (err) { if (err) {
pr_err("out of sync doorbell caused an error\n"); pr_err("failed to reserve back the doorbell back\n");
goto out;
} }
err = create_doorbell(guc->execbuf_client);
/* restore 'correct' db bitmap */
bitmap_copy(guc->doorbell_bitmap, db_bitmap_bk, GUC_NUM_DOORBELLS);
err = guc_init_doorbell_hw(guc);
if (err) { if (err) {
pr_err("restored doorbell caused an error\n"); pr_err("recreate doorbell failed\n");
goto out; goto out;
} }
...@@ -226,8 +223,11 @@ static int igt_guc_init_doorbell_hw(void *args) ...@@ -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 * Leave clean state for other test, plus the driver always destroy the
* clients during unload. * clients during unload.
*/ */
destroy_doorbell(guc->execbuf_client);
destroy_doorbell(guc->preempt_client);
guc_clients_destroy(guc); guc_clients_destroy(guc);
guc_clients_create(guc); guc_clients_create(guc);
guc_clients_doorbell_init(guc);
unlock: unlock:
mutex_unlock(&dev_priv->drm.struct_mutex); mutex_unlock(&dev_priv->drm.struct_mutex);
return err; return err;
...@@ -309,25 +309,7 @@ static int igt_guc_doorbells(void *arg) ...@@ -309,25 +309,7 @@ static int igt_guc_doorbells(void *arg)
db_id = clients[i]->doorbell_id; db_id = clients[i]->doorbell_id;
/* err = create_doorbell(clients[i]);
* 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]);
if (err) { if (err) {
pr_err("[%d] Failed to create a doorbell\n", i); pr_err("[%d] Failed to create a doorbell\n", i);
goto out; goto out;
...@@ -348,8 +330,10 @@ static int igt_guc_doorbells(void *arg) ...@@ -348,8 +330,10 @@ static int igt_guc_doorbells(void *arg)
out: out:
for (i = 0; i < ATTEMPTS; i++) 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]); guc_client_free(clients[i]);
}
unlock: unlock:
mutex_unlock(&dev_priv->drm.struct_mutex); mutex_unlock(&dev_priv->drm.struct_mutex);
return err; return err;
...@@ -358,7 +342,7 @@ static int igt_guc_doorbells(void *arg) ...@@ -358,7 +342,7 @@ static int igt_guc_doorbells(void *arg)
int intel_guc_live_selftest(struct drm_i915_private *dev_priv) int intel_guc_live_selftest(struct drm_i915_private *dev_priv)
{ {
static const struct i915_subtest tests[] = { static const struct i915_subtest tests[] = {
SUBTEST(igt_guc_init_doorbell_hw), SUBTEST(igt_guc_clients),
SUBTEST(igt_guc_doorbells), SUBTEST(igt_guc_doorbells),
}; };
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment