On 02/14/2017 05:53 AM, Joonas Lahtinen wrote:
Started adding proper teardown to guc_client_alloc, ended up removing
quite a few dead ends where errors communicating with the GuC were
silently ignored. There also seemed to be quite a few erronous
teardown actions performed in case of an error (ordering wrong).
v2:
- Increase function symmetry/proximity (Michal/Daniele)
- Fix __reserve_doorbell accounting for high priority (Daniele)
- Call __update_doorbell_desc! (Daniele)
- Isolate __guc_{,de}allocate_doorbell (Michal/Daniele)
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx>
Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
drivers/gpu/drm/i915/i915_guc_submission.c | 371 ++++++++++++++++-------------
drivers/gpu/drm/i915/intel_guc_fwif.h | 4 +-
drivers/gpu/drm/i915/intel_uc.h | 11 +-
4 files changed, 215 insertions(+), 175 deletions(-)
<SNIP>
/*
* Since the doorbell only requires a single cacheline, we can save
@@ -753,27 +781,35 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
guc_proc_desc_init(guc, client);
guc_ctx_desc_init(guc, client);
- /* For runtime client allocation we need to enable the doorbell. Not
- * required yet for the static execbuf_client as this special kernel
- * client is enabled from i915_guc_submission_enable().
- *
- * guc_update_doorbell_id(guc, client, db_id);
- */
+ /* For runtime client allocation we need to enable the doorbell. */
+ ret = __update_doorbell_desc(client, client->doorbell_id);
+ if (ret)
+ goto err_vaddr;
+
+ ret = __create_doorbell(client);
+ if (ret)
+ goto err_db;
At this point, client->doorbell_id is still invalid (__reserve_doorbell
is not called until later from guc_init_doorbell_hw), so the
__create_doorbell fails (and from there, the whole thing falls over: see
my next comment below). CI.BAT didn't catch it because GuC is disabled
by default.
DRM_DEBUG_DRIVER("new priority %u client %p for engine(s) 0x%x: ctx_index %u\n",
- priority, client, client->engines, client->ctx_index);
- DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%x\n",
- client->doorbell_id, client->doorbell_offset);
+ priority, client, client->engines, client->ctx_index);
+ DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%lx\n",
+ client->doorbell_id, client->doorbell_offset);
return client;
+err_db:
+ __update_doorbell_desc(client, GUC_DOORBELL_INVALID);
+err_vaddr:
+ i915_gem_object_unpin_map(client->vma->obj);
+err_vma:
+ i915_vma_unpin_and_release(&client->vma);
+err_id:
+ ida_simple_remove(&guc->ctx_ids, client->ctx_index);
+err_client:
+ kfree(client);
-err:
- guc_client_free(dev_priv, client);
- return NULL;
+ return ERR_PTR(ret);
}
I know you are leaving i915_guc_submission_init to me, but this patch
should as a minimum check the return code from guc_client_alloc,
otherwise we might end up with an invalid guc->execbuf_client without
noticing.
Something like this should suffice, and I can take it from there:
@@ -939,7 +939,7 @@ int i915_guc_submission_init(struct drm_i915_private
*dev_priv)
INTEL_INFO(dev_priv)->ring_mask,
GUC_CTX_PRIORITY_KMD_NORMAL,
dev_priv->kernel_context);
- if (!guc->execbuf_client) {
+ if (IS_ERR(guc->execbuf_client)) {
DRM_ERROR("Failed to create GuC client for execbuf!\n");
goto err;
}
@@ -1016,10 +1016,8 @@ void i915_guc_submission_fini(struct
drm_i915_private *dev_priv)
struct i915_guc_client *client;
client = fetch_and_zero(&guc->execbuf_client);
- if (!client)
- return;
-
- guc_client_free(client);
+ if (client && !IS_ERR(client))
+ guc_client_free(client);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
