* [PATCH 01/12] drm/i915/guc: Sanitize GuC client initialization
2017-03-21 9:02 [PATCH 00/12] Various improvements around the GuC topic Oscar Mateo
@ 2017-03-21 9:02 ` Oscar Mateo
2017-03-22 9:28 ` Joonas Lahtinen
2017-03-21 9:02 ` [PATCH 02/12] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access Oscar Mateo
` (11 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Oscar Mateo @ 2017-03-21 9:02 UTC (permalink / raw)
To: intel-gfx
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)
v3:
- "Select" a cacheline is a more accurate verb than "reserve" (Daniele).
- We cannot update & create the doorbell without reserving it first, so
move the whole doorbell creation for execbuf_client to the submission
enable (Oscar).i
- Add a fixme for ignoring possible doorbell destroy errors.
v4:
- Remove comment about is_high_priority (Daniele)
- Debug message typo (Daniele)
- Reuse __get_doorbell in more places (Daniele)
- Do not do arithmetic on void pointers (Daniele)
- Add comment to __reset_doorbell (Daniele)
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
drivers/gpu/drm/i915/i915_guc_submission.c | 399 ++++++++++++++++-------------
drivers/gpu/drm/i915/intel_guc_fwif.h | 4 +-
drivers/gpu/drm/i915/intel_uc.h | 11 +-
4 files changed, 234 insertions(+), 184 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 47e707d..061f8a5 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2476,7 +2476,7 @@ static void i915_guc_client_info(struct seq_file *m,
seq_printf(m, "\tPriority %d, GuC ctx index: %u, PD offset 0x%x\n",
client->priority, client->ctx_index, client->proc_desc_offset);
- seq_printf(m, "\tDoorbell id %d, offset: 0x%x, cookie 0x%x\n",
+ seq_printf(m, "\tDoorbell id %d, offset: 0x%lx, cookie 0x%x\n",
client->doorbell_id, client->doorbell_offset, client->doorbell_cookie);
seq_printf(m, "\tWQ size %d, offset: 0x%x, tail %d\n",
client->wq_size, client->wq_offset, client->wq_tail);
@@ -2511,7 +2511,7 @@ static int i915_guc_info(struct seq_file *m, void *data)
}
seq_printf(m, "Doorbell map:\n");
- seq_printf(m, "\t%*pb\n", GUC_MAX_DOORBELLS, guc->doorbell_bitmap);
+ seq_printf(m, "\t%*pb\n", GUC_NUM_DOORBELLS, guc->doorbell_bitmap);
seq_printf(m, "Doorbell next cacheline: 0x%x\n\n", guc->db_cacheline);
seq_printf(m, "GuC total action count: %llu\n", guc->action_count);
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 832ac9e..21dadc1 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -64,27 +64,70 @@
*
*/
+static inline bool is_high_priority(struct i915_guc_client* client)
+{
+ return client->priority <= GUC_CTX_PRIORITY_HIGH;
+}
+
+static int __reserve_doorbell(struct i915_guc_client *client)
+{
+ unsigned long offset;
+ unsigned long end;
+ u16 id;
+
+ GEM_BUG_ON(client->doorbell_id != GUC_DOORBELL_INVALID);
+
+ /*
+ * The bitmap tracks which doorbell registers are currently in use.
+ * It is split into two halves; the first half is used for normal
+ * priority contexts, the second half for high-priority ones.
+ */
+ offset = 0;
+ end = GUC_NUM_DOORBELLS/2;
+ if (is_high_priority(client)) {
+ offset = end;
+ end += offset;
+ }
+
+ id = find_next_zero_bit(client->guc->doorbell_bitmap, offset, end);
+ if (id == end)
+ return -ENOSPC;
+
+ __set_bit(id, client->guc->doorbell_bitmap);
+ client->doorbell_id = id;
+ DRM_DEBUG_DRIVER("client %u (high prio=%s) reserved doorbell: %d\n",
+ client->ctx_index, yesno(is_high_priority(client)),
+ id);
+ return 0;
+}
+
+static void __unreserve_doorbell(struct i915_guc_client *client)
+{
+ GEM_BUG_ON(client->doorbell_id == GUC_DOORBELL_INVALID);
+
+ __clear_bit(client->doorbell_id, client->guc->doorbell_bitmap);
+ client->doorbell_id = GUC_DOORBELL_INVALID;
+}
+
/*
* Tell the GuC to allocate or deallocate a specific doorbell
*/
-static int guc_allocate_doorbell(struct intel_guc *guc,
- struct i915_guc_client *client)
+static int __guc_allocate_doorbell(struct intel_guc *guc, u32 ctx_index)
{
u32 action[] = {
INTEL_GUC_ACTION_ALLOCATE_DOORBELL,
- client->ctx_index
+ ctx_index
};
return intel_guc_send(guc, action, ARRAY_SIZE(action));
}
-static int guc_release_doorbell(struct intel_guc *guc,
- struct i915_guc_client *client)
+static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
{
u32 action[] = {
INTEL_GUC_ACTION_DEALLOCATE_DOORBELL,
- client->ctx_index
+ ctx_index
};
return intel_guc_send(guc, action, ARRAY_SIZE(action));
@@ -97,104 +140,101 @@ static int guc_release_doorbell(struct intel_guc *guc,
* client object which contains the page being used for the doorbell
*/
-static int guc_update_doorbell_id(struct intel_guc *guc,
- struct i915_guc_client *client,
- u16 new_id)
+static int __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)
{
- struct sg_table *sg = guc->ctx_pool_vma->pages;
- void *doorbell_bitmap = guc->doorbell_bitmap;
- struct guc_doorbell_info *doorbell;
+ struct sg_table *sg = client->guc->ctx_pool_vma->pages;
struct guc_context_desc desc;
size_t len;
- doorbell = client->vaddr + client->doorbell_offset;
-
- 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)guc_release_doorbell(guc, client);
- __clear_bit(client->doorbell_id, doorbell_bitmap);
- }
-
/* 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);
+ 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);
+ 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;
+ return 0;
+}
- /* Activate the new doorbell */
- __set_bit(new_id, doorbell_bitmap);
+static struct guc_doorbell_info *__get_doorbell(struct i915_guc_client *client)
+{
+ return (struct guc_doorbell_info *)
+ ((char *)client->vaddr + client->doorbell_offset);
+}
+
+static bool has_doorbell(struct i915_guc_client *client)
+{
+ if (client->doorbell_id == GUC_DOORBELL_INVALID)
+ return false;
+
+ return test_bit(client->doorbell_id, client->guc->doorbell_bitmap);
+}
+
+static int __create_doorbell(struct i915_guc_client *client)
+{
+ struct guc_doorbell_info *doorbell;
+ int err;
+
+ doorbell = __get_doorbell(client);
doorbell->db_status = GUC_DOORBELL_ENABLED;
doorbell->cookie = client->doorbell_cookie;
- return guc_allocate_doorbell(guc, client);
+
+ err = __guc_allocate_doorbell(client->guc, client->ctx_index);
+ if (err) {
+ doorbell->db_status = GUC_DOORBELL_DISABLED;
+ doorbell->cookie = 0;
+ }
+ return err;
}
-static void guc_disable_doorbell(struct intel_guc *guc,
- struct i915_guc_client *client)
+static int __destroy_doorbell(struct i915_guc_client *client)
{
- (void)guc_update_doorbell_id(guc, client, GUC_INVALID_DOORBELL_ID);
+ struct guc_doorbell_info *doorbell;
- /* XXX: wait for any interrupts */
- /* XXX: wait for workqueue to drain */
+ doorbell = __get_doorbell(client);
+ doorbell->db_status = GUC_DOORBELL_DISABLED;
+ doorbell->cookie = 0;
+
+ return __guc_deallocate_doorbell(client->guc, client->ctx_index);
}
-static uint16_t
-select_doorbell_register(struct intel_guc *guc, uint32_t priority)
+static int destroy_doorbell(struct i915_guc_client *client)
{
- /*
- * The bitmap tracks which doorbell registers are currently in use.
- * It is split into two halves; the first half is used for normal
- * priority contexts, the second half for high-priority ones.
- * Note that logically higher priorities are numerically less than
- * normal ones, so the test below means "is it high-priority?"
- */
- const bool hi_pri = (priority <= GUC_CTX_PRIORITY_HIGH);
- const uint16_t half = GUC_MAX_DOORBELLS / 2;
- const uint16_t start = hi_pri ? half : 0;
- const uint16_t end = start + half;
- uint16_t id;
+ int err;
- id = find_next_zero_bit(guc->doorbell_bitmap, end, start);
- if (id == end)
- id = GUC_INVALID_DOORBELL_ID;
+ GEM_BUG_ON(!has_doorbell(client));
+
+ /* XXX: wait for any interrupts */
+ /* XXX: wait for workqueue to drain */
- DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
- hi_pri ? "high" : "normal", id);
+ err = __destroy_doorbell(client);
+ if (err)
+ return err;
- return id;
-}
+ __update_doorbell_desc(client, GUC_DOORBELL_INVALID);
-/*
- * Select, assign and relase doorbell cachelines
- *
- * These functions track which doorbell cachelines are in use.
- * The data they manipulate is protected by the intel_guc_send lock.
- */
+ __unreserve_doorbell(client);
+
+ return 0;
+}
-static uint32_t select_doorbell_cacheline(struct intel_guc *guc)
+static unsigned long __select_cacheline(struct intel_guc* guc)
{
- const uint32_t cacheline_size = cache_line_size();
- uint32_t offset;
+ unsigned long offset;
/* Doorbell uses a single cache line within a page */
offset = offset_in_page(guc->db_cacheline);
/* Moving to next cache line to reduce contention */
- guc->db_cacheline += cacheline_size;
-
- DRM_DEBUG_DRIVER("selected doorbell cacheline 0x%x, next 0x%x, linesize %u\n",
- offset, guc->db_cacheline, cacheline_size);
+ guc->db_cacheline += cache_line_size();
+ DRM_DEBUG_DRIVER("reserved cacheline 0x%lx, next 0x%x, linesize %u\n",
+ offset, guc->db_cacheline, cache_line_size());
return offset;
}
@@ -293,8 +333,7 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
gfx_addr = guc_ggtt_offset(client->vma);
desc.db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
client->doorbell_offset;
- desc.db_trigger_cpu =
- (uintptr_t)client->vaddr + client->doorbell_offset;
+ desc.db_trigger_cpu = (uintptr_t)__get_doorbell(client);
desc.db_trigger_uk = gfx_addr + client->doorbell_offset;
desc.process_desc = gfx_addr + client->proc_desc_offset;
desc.wq_addr = gfx_addr + client->wq_offset;
@@ -463,7 +502,7 @@ static int guc_ring_doorbell(struct i915_guc_client *client)
db_exc.cookie = 1;
/* pointer of current doorbell cacheline */
- db = client->vaddr + client->doorbell_offset;
+ db = (union guc_doorbell_qw *)__get_doorbell(client);
while (attempt--) {
/* lets ring the doorbell */
@@ -695,93 +734,101 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
return vma;
}
-static void
-guc_client_free(struct drm_i915_private *dev_priv,
- struct i915_guc_client *client)
+static void guc_client_free(struct i915_guc_client *client)
{
- struct intel_guc *guc = &dev_priv->guc;
-
- if (!client)
- return;
-
/*
* XXX: wait for any outstanding submissions before freeing memory.
* Be sure to drop any locks
*/
-
- if (client->vaddr) {
- /*
- * If we got as far as setting up a doorbell, make sure we
- * shut it down before unmapping & deallocating the memory.
- */
- guc_disable_doorbell(guc, client);
-
- i915_gem_object_unpin_map(client->vma->obj);
- }
-
+ guc_ctx_desc_fini(client->guc, client);
+ i915_gem_object_unpin_map(client->vma->obj);
i915_vma_unpin_and_release(&client->vma);
-
- if (client->ctx_index != GUC_INVALID_CTX_ID) {
- guc_ctx_desc_fini(guc, client);
- ida_simple_remove(&guc->ctx_ids, client->ctx_index);
- }
-
+ ida_simple_remove(&client->guc->ctx_ids, client->ctx_index);
kfree(client);
}
/* Check that a doorbell register is in the expected state */
-static bool guc_doorbell_check(struct intel_guc *guc, uint16_t db_id)
+static bool doorbell_ok(struct intel_guc *guc, u16 db_id)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
- i915_reg_t drbreg = GEN8_DRBREGL(db_id);
- uint32_t value = I915_READ(drbreg);
- bool enabled = (value & GUC_DOORBELL_ENABLED) != 0;
- bool expected = test_bit(db_id, guc->doorbell_bitmap);
+ u32 drbregl;
+ bool valid;
+
+ GEM_BUG_ON(db_id >= GUC_DOORBELL_INVALID);
+
+ drbregl = I915_READ(GEN8_DRBREGL(db_id));
+ valid = drbregl & GEN8_DRB_VALID;
- if (enabled == expected)
+ if (test_bit(db_id, guc->doorbell_bitmap) == valid)
return true;
- DRM_DEBUG_DRIVER("Doorbell %d (reg 0x%x) 0x%x, should be %s\n",
- db_id, drbreg.reg, value,
- expected ? "active" : "inactive");
+ DRM_DEBUG_DRIVER("Doorbell %d has unexpected state (0x%x): valid=%s\n",
+ db_id, drbregl, yesno(valid));
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 i915_guc_client* client, u16 db_id)
+{
+ int err;
+
+ err = __update_doorbell_desc(client, db_id);
+ if (!err)
+ err = __create_doorbell(client);
+ if (!err)
+ err = __destroy_doorbell(client);
+
+ return err;
+}
+
+/*
* Borrow the first client to set up & tear down each unused doorbell
* in turn, to ensure that all doorbell h/w is (re)initialised.
*/
-static void guc_init_doorbell_hw(struct intel_guc *guc)
+static int guc_init_doorbell_hw(struct intel_guc *guc)
{
struct i915_guc_client *client = guc->execbuf_client;
- uint16_t db_id;
- int i, err;
+ int err;
+ int i;
- guc_disable_doorbell(guc, client);
+ if (has_doorbell(client))
+ destroy_doorbell(client);
- for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
- /* Skip if doorbell is OK */
- if (guc_doorbell_check(guc, i))
+ for (i = 0; i < GUC_NUM_DOORBELLS; ++i) {
+ if (doorbell_ok(guc, i))
continue;
- err = guc_update_doorbell_id(guc, client, i);
- if (err)
- DRM_DEBUG_DRIVER("Doorbell %d update failed, err %d\n",
- i, err);
+ err = __reset_doorbell(client, i);
+ WARN(err, "Doorbell %d reset failed, err %d\n", i, err);
}
- db_id = select_doorbell_register(guc, client->priority);
- WARN_ON(db_id == GUC_INVALID_DOORBELL_ID);
+ /* Read back & verify all doorbell registers */
+ for (i = 0; i < GUC_NUM_DOORBELLS; ++i)
+ WARN_ON(!doorbell_ok(guc, i));
- err = guc_update_doorbell_id(guc, client, db_id);
+ err = __reserve_doorbell(client);
if (err)
- DRM_WARN("Failed to restore doorbell to %d, err %d\n",
- db_id, err);
+ return err;
- /* Read back & verify all doorbell registers */
- for (i = 0; i < GUC_MAX_DOORBELLS; ++i)
- (void)guc_doorbell_check(guc, i);
+ err = __update_doorbell_desc(client, client->doorbell_id);
+ if (err)
+ goto err_reserve;
+
+ err = __create_doorbell(client);
+ if (err)
+ goto err_update;
+
+ return 0;
+err_reserve:
+ __unreserve_doorbell(client);
+err_update:
+ __update_doorbell_desc(client, GUC_DOORBELL_INVALID);
+ return err;
}
/**
@@ -807,49 +854,46 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
struct intel_guc *guc = &dev_priv->guc;
struct i915_vma *vma;
void *vaddr;
- uint16_t db_id;
+ int ret;
client = kzalloc(sizeof(*client), GFP_KERNEL);
if (!client)
- return NULL;
+ return ERR_PTR(-ENOMEM);
- client->owner = ctx;
client->guc = guc;
+ client->owner = ctx;
client->engines = engines;
client->priority = priority;
- client->doorbell_id = GUC_INVALID_DOORBELL_ID;
+ client->doorbell_id = GUC_DOORBELL_INVALID;
+ client->wq_offset = GUC_DB_SIZE;
+ client->wq_size = GUC_WQ_SIZE;
+ spin_lock_init(&client->wq_lock);
- client->ctx_index = (uint32_t)ida_simple_get(&guc->ctx_ids, 0,
- GUC_MAX_GPU_CONTEXTS, GFP_KERNEL);
- if (client->ctx_index >= GUC_MAX_GPU_CONTEXTS) {
- client->ctx_index = GUC_INVALID_CTX_ID;
- goto err;
- }
+ ret = ida_simple_get(&guc->ctx_ids, 0, GUC_MAX_GPU_CONTEXTS,
+ GFP_KERNEL);
+ if (ret < 0)
+ goto err_client;
+
+ client->ctx_index = ret;
/* The first page is doorbell/proc_desc. Two followed pages are wq. */
vma = intel_guc_allocate_vma(guc, GUC_DB_SIZE + GUC_WQ_SIZE);
- if (IS_ERR(vma))
- goto err;
+ if (IS_ERR(vma)) {
+ ret = PTR_ERR(vma);
+ goto err_id;
+ }
/* We'll keep just the first (doorbell/proc) page permanently kmap'd. */
client->vma = vma;
vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
- if (IS_ERR(vaddr))
- goto err;
-
+ if (IS_ERR(vaddr)) {
+ ret = PTR_ERR(vaddr);
+ goto err_vma;
+ }
client->vaddr = vaddr;
- spin_lock_init(&client->wq_lock);
- client->wq_offset = GUC_DB_SIZE;
- client->wq_size = GUC_WQ_SIZE;
-
- db_id = select_doorbell_register(guc, client->priority);
- if (db_id == GUC_INVALID_DOORBELL_ID)
- /* XXX: evict a doorbell instead? */
- goto err;
-
- client->doorbell_offset = select_doorbell_cacheline(guc);
+ client->doorbell_offset = __select_cacheline(guc);
/*
* Since the doorbell only requires a single cacheline, we can save
@@ -864,27 +908,28 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
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);
- */
+ /* FIXME: Runtime client allocation (which currently we don't do) will
+ * require that the doorbell gets created now. The static execbuf_client
+ * is now getting its doorbell later (on submission enable) but maybe we
+ * also want to reorder things in the future so that we don't have to
+ * special case the doorbell creation */
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_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);
}
-
-
static void guc_policies_init(struct guc_policies *policies)
{
struct guc_policy *policy;
@@ -985,7 +1030,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
return 0;
/* Wipe bitmap & delete client in case of reinitialisation */
- bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS);
+ bitmap_clear(guc->doorbell_bitmap, 0, GUC_NUM_DOORBELLS);
i915_guc_submission_disable(dev_priv);
if (!i915.enable_guc_submission)
@@ -1007,7 +1052,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;
}
@@ -1078,14 +1123,19 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
struct i915_guc_client *client = guc->execbuf_client;
struct intel_engine_cs *engine;
enum intel_engine_id id;
+ int err;
if (!client)
return -ENODEV;
- intel_guc_sample_forcewake(guc);
+ err = intel_guc_sample_forcewake(guc);
+ if (err)
+ return err;
guc_reset_wq(client);
- guc_init_doorbell_hw(guc);
+ err = guc_init_doorbell_hw(guc);
+ if (err)
+ return err;
/* Take over from manual control of ELSP (execlists) */
guc_interrupts_capture(dev_priv);
@@ -1147,6 +1197,11 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
if (!guc->execbuf_client)
return;
+ /* 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(guc->execbuf_client);
+
/* Revert back to manual ELSP submission */
intel_engines_reset_default_submission(dev_priv);
}
@@ -1157,10 +1212,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(dev_priv, client);
+ if (client && !IS_ERR(client))
+ guc_client_free(client);
i915_vma_unpin_and_release(&guc->ads_vma);
i915_vma_unpin_and_release(&guc->log.vma);
@@ -1222,5 +1275,3 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
return intel_guc_send(guc, data, ARRAY_SIZE(data));
}
-
-
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 25691f0..3ae8cef 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -241,8 +241,8 @@ struct guc_doorbell_info {
u64 value_qw;
} __packed;
-#define GUC_MAX_DOORBELLS 256
-#define GUC_INVALID_DOORBELL_ID (GUC_MAX_DOORBELLS)
+#define GUC_NUM_DOORBELLS 256
+#define GUC_DOORBELL_INVALID (GUC_NUM_DOORBELLS)
#define GUC_DB_SIZE (PAGE_SIZE)
#define GUC_WQ_SIZE (PAGE_SIZE * 2)
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index a35eded..c3a3843 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -72,13 +72,12 @@ struct i915_guc_client {
uint32_t engines; /* bitmap of (host) engine ids */
uint32_t priority;
- uint32_t ctx_index;
+ u32 ctx_index;
uint32_t proc_desc_offset;
- uint32_t doorbell_offset;
- uint32_t doorbell_cookie;
- uint16_t doorbell_id;
- uint16_t padding[3]; /* Maintain alignment */
+ u16 doorbell_id;
+ unsigned long doorbell_offset;
+ u32 doorbell_cookie;
spinlock_t wq_lock;
uint32_t wq_offset;
@@ -159,7 +158,7 @@ struct intel_guc {
struct i915_guc_client *execbuf_client;
- DECLARE_BITMAP(doorbell_bitmap, GUC_MAX_DOORBELLS);
+ DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
uint32_t db_cacheline; /* Cyclic counter mod pagesize */
/* Action status & statistics */
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 01/12] drm/i915/guc: Sanitize GuC client initialization
2017-03-21 9:02 ` [PATCH 01/12] drm/i915/guc: Sanitize GuC client initialization Oscar Mateo
@ 2017-03-22 9:28 ` Joonas Lahtinen
2017-03-22 9:46 ` Oscar Mateo
0 siblings, 1 reply; 28+ messages in thread
From: Joonas Lahtinen @ 2017-03-22 9:28 UTC (permalink / raw)
To: Oscar Mateo, intel-gfx
It's a good practice to use "git am" to apply the original patch and
then "git commit --amend" to it, so that the "From: " field stays
intact. Or use git commit --amend --author="John Doe <foo@bar.org>" but
that is more prone to typos.
On ti, 2017-03-21 at 02:02 -0700, Oscar Mateo 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)
>
> v3:
> - "Select" a cacheline is a more accurate verb than "reserve" (Daniele).
> - We cannot update & create the doorbell without reserving it first, so
> move the whole doorbell creation for execbuf_client to the submission
> enable (Oscar).i
> - Add a fixme for ignoring possible doorbell destroy errors.
>
> v4:
> - Remove comment about is_high_priority (Daniele)
> - Debug message typo (Daniele)
> - Reuse __get_doorbell in more places (Daniele)
> - Do not do arithmetic on void pointers (Daniele)
> - Add comment to __reset_doorbell (Daniele)
>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
The R-b should probably be after S-o-b if it was to last version.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 01/12] drm/i915/guc: Sanitize GuC client initialization
2017-03-22 9:28 ` Joonas Lahtinen
@ 2017-03-22 9:46 ` Oscar Mateo
0 siblings, 0 replies; 28+ messages in thread
From: Oscar Mateo @ 2017-03-22 9:46 UTC (permalink / raw)
To: Joonas Lahtinen, intel-gfx
On 03/22/2017 02:28 AM, Joonas Lahtinen wrote:
> It's a good practice to use "git am" to apply the original patch and
> then "git commit --amend" to it, so that the "From: " field stays
> intact. Or use git commit --amend --author="John Doe <foo@bar.org>" but
> that is more prone to typos.
>
> On ti, 2017-03-21 at 02:02 -0700, Oscar Mateo 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)
>>
>> v3:
>> - "Select" a cacheline is a more accurate verb than "reserve" (Daniele).
>> - We cannot update & create the doorbell without reserving it first, so
>> move the whole doorbell creation for execbuf_client to the submission
>> enable (Oscar).i
>> - Add a fixme for ignoring possible doorbell destroy errors.
>>
>> v4:
>> - Remove comment about is_high_priority (Daniele)
>> - Debug message typo (Daniele)
>> - Reuse __get_doorbell in more places (Daniele)
>> - Do not do arithmetic on void pointers (Daniele)
>> - Add comment to __reset_doorbell (Daniele)
>>
>> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> The R-b should probably be after S-o-b if it was to last version.
>
> Regards, Joonas
My bad. I have spent too much time without using Git I'm like wicked
rusty (you have probably noticed other weird things I've done, like
sending the last series without the word "PATCH" or sending this one
without proper versions in the subject). I'll fix those things and resubmit.
Thanks,
Oscar
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 02/12] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
2017-03-21 9:02 [PATCH 00/12] Various improvements around the GuC topic Oscar Mateo
2017-03-21 9:02 ` [PATCH 01/12] drm/i915/guc: Sanitize GuC client initialization Oscar Mateo
@ 2017-03-21 9:02 ` Oscar Mateo
2017-03-22 9:45 ` Chris Wilson
2017-03-21 9:02 ` [PATCH 03/12] drm/i915/guc: Add onion teardown to the GuC setup Oscar Mateo
` (10 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Oscar Mateo @ 2017-03-21 9:02 UTC (permalink / raw)
To: intel-gfx
The GuC descriptor is big in size. If we use a local definition of
guc_desc we have a chance to overflow stack, so avoid it.
Also, Chris abhors scatterlists :)
v2: Rebased, helper function to retrieve the context descriptor,
s/ctx_pool_vma/ctx_pool/
v3: Zero out guc_context_desc before initialization
v4: Do not do arithmetic on void pointers
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 93 +++++++++++++++---------------
drivers/gpu/drm/i915/intel_guc_loader.c | 2 +-
drivers/gpu/drm/i915/intel_uc.h | 3 +-
3 files changed, 48 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 21dadc1..5870cec 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -133,6 +133,12 @@ static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
return intel_guc_send(guc, action, ARRAY_SIZE(action));
}
+static struct guc_context_desc *__get_context_desc(struct i915_guc_client *client)
+{
+ return (struct guc_context_desc *)((char *)client->guc->ctx_pool_vaddr +
+ sizeof(struct guc_context_desc) * client->ctx_index);
+}
+
/*
* Initialise, update, or clear doorbell data shared with the GuC
*
@@ -142,21 +148,11 @@ static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
static int __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)
{
- struct sg_table *sg = client->guc->ctx_pool_vma->pages;
- struct guc_context_desc desc;
- size_t len;
+ struct guc_context_desc *desc;
/* 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;
+ desc = __get_context_desc(client);
+ desc->db_id = new_id;
return 0;
}
@@ -272,29 +268,28 @@ static void guc_proc_desc_init(struct intel_guc *guc,
* data structures relating to this client (doorbell, process descriptor,
* write queue, etc).
*/
-
static void guc_ctx_desc_init(struct intel_guc *guc,
struct i915_guc_client *client)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
struct intel_engine_cs *engine;
struct i915_gem_context *ctx = client->owner;
- struct guc_context_desc desc;
- struct sg_table *sg;
+ struct guc_context_desc *desc;
unsigned int tmp;
u32 gfx_addr;
- memset(&desc, 0, sizeof(desc));
+ desc = __get_context_desc(client);
+ memset(desc, 0, sizeof(*desc));
- desc.attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
- desc.context_id = client->ctx_index;
- desc.priority = client->priority;
- desc.db_id = client->doorbell_id;
+ desc->attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
+ desc->context_id = client->ctx_index;
+ desc->priority = client->priority;
+ desc->db_id = client->doorbell_id;
for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
struct intel_context *ce = &ctx->engine[engine->id];
uint32_t guc_engine_id = engine->guc_id;
- struct guc_execlist_context *lrc = &desc.lrc[guc_engine_id];
+ struct guc_execlist_context *lrc = &desc->lrc[guc_engine_id];
/* TODO: We have a design issue to be solved here. Only when we
* receive the first batch, we know which engine is used by the
@@ -319,49 +314,40 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
lrc->ring_next_free_location = lrc->ring_begin;
lrc->ring_current_tail_pointer_value = 0;
- desc.engines_used |= (1 << guc_engine_id);
+ desc->engines_used |= (1 << guc_engine_id);
}
DRM_DEBUG_DRIVER("Host engines 0x%x => GuC engines used 0x%x\n",
- client->engines, desc.engines_used);
- WARN_ON(desc.engines_used == 0);
+ client->engines, desc->engines_used);
+ WARN_ON(desc->engines_used == 0);
/*
* The doorbell, process descriptor, and workqueue are all parts
* of the client object, which the GuC will reference via the GGTT
*/
gfx_addr = guc_ggtt_offset(client->vma);
- desc.db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
+ desc->db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
client->doorbell_offset;
- desc.db_trigger_cpu = (uintptr_t)__get_doorbell(client);
- desc.db_trigger_uk = gfx_addr + client->doorbell_offset;
- desc.process_desc = gfx_addr + client->proc_desc_offset;
- desc.wq_addr = gfx_addr + client->wq_offset;
- desc.wq_size = client->wq_size;
+ desc->db_trigger_cpu = (uintptr_t)__get_doorbell(client);
+ desc->db_trigger_uk = gfx_addr + client->doorbell_offset;
+ desc->process_desc = gfx_addr + client->proc_desc_offset;
+ desc->wq_addr = gfx_addr + client->wq_offset;
+ desc->wq_size = client->wq_size;
/*
* XXX: Take LRCs from an existing context if this is not an
* IsKMDCreatedContext client
*/
- desc.desc_private = (uintptr_t)client;
-
- /* Pool context is pinned already */
- sg = guc->ctx_pool_vma->pages;
- sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
- sizeof(desc) * client->ctx_index);
+ desc->desc_private = (uintptr_t)client;
}
static void guc_ctx_desc_fini(struct intel_guc *guc,
struct i915_guc_client *client)
{
- struct guc_context_desc desc;
- struct sg_table *sg;
-
- memset(&desc, 0, sizeof(desc));
+ struct guc_context_desc *desc;
- sg = guc->ctx_pool_vma->pages;
- sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
- sizeof(desc) * client->ctx_index);
+ desc = __get_context_desc(client);
+ memset(desc, 0, sizeof(*desc));
}
/**
@@ -1025,6 +1011,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
const size_t gemsize = round_up(poolsize, PAGE_SIZE);
struct intel_guc *guc = &dev_priv->guc;
struct i915_vma *vma;
+ void *vaddr;
if (!HAS_GUC_SCHED(dev_priv))
return 0;
@@ -1036,14 +1023,21 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
if (!i915.enable_guc_submission)
return 0; /* not enabled */
- if (guc->ctx_pool_vma)
+ if (guc->ctx_pool)
return 0; /* already allocated */
vma = intel_guc_allocate_vma(guc, gemsize);
if (IS_ERR(vma))
return PTR_ERR(vma);
- guc->ctx_pool_vma = vma;
+ guc->ctx_pool = vma;
+
+ vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
+ if (IS_ERR(vaddr))
+ goto err;
+
+ guc->ctx_pool_vaddr = vaddr;
+
ida_init(&guc->ctx_ids);
intel_guc_log_create(guc);
guc_addon_create(guc);
@@ -1218,9 +1212,12 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
i915_vma_unpin_and_release(&guc->ads_vma);
i915_vma_unpin_and_release(&guc->log.vma);
- if (guc->ctx_pool_vma)
+ if (guc->ctx_pool_vaddr) {
ida_destroy(&guc->ctx_ids);
- i915_vma_unpin_and_release(&guc->ctx_pool_vma);
+ i915_gem_object_unpin_map(guc->ctx_pool->obj);
+ }
+
+ i915_vma_unpin_and_release(&guc->ctx_pool);
}
/**
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 2f270d0..1a6e478 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -156,7 +156,7 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
/* If GuC submission is enabled, set up additional parameters here */
if (i915.enable_guc_submission) {
- u32 pgs = guc_ggtt_offset(dev_priv->guc.ctx_pool_vma);
+ u32 pgs = guc_ggtt_offset(dev_priv->guc.ctx_pool);
u32 ctx_in_16 = GUC_MAX_GPU_CONTEXTS / 16;
pgs >>= PAGE_SHIFT;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index c3a3843..77f7153 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -153,7 +153,8 @@ struct intel_guc {
bool interrupts_enabled;
struct i915_vma *ads_vma;
- struct i915_vma *ctx_pool_vma;
+ struct i915_vma *ctx_pool;
+ void *ctx_pool_vaddr;
struct ida ctx_ids;
struct i915_guc_client *execbuf_client;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 02/12] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
2017-03-21 9:02 ` [PATCH 02/12] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access Oscar Mateo
@ 2017-03-22 9:45 ` Chris Wilson
2017-03-22 9:42 ` Oscar Mateo
0 siblings, 1 reply; 28+ messages in thread
From: Chris Wilson @ 2017-03-22 9:45 UTC (permalink / raw)
To: Oscar Mateo; +Cc: intel-gfx
On Tue, Mar 21, 2017 at 02:02:47AM -0700, Oscar Mateo wrote:
> The GuC descriptor is big in size. If we use a local definition of
> guc_desc we have a chance to overflow stack, so avoid it.
>
> Also, Chris abhors scatterlists :)
>
> v2: Rebased, helper function to retrieve the context descriptor,
> s/ctx_pool_vma/ctx_pool/
>
> v3: Zero out guc_context_desc before initialization
>
> v4: Do not do arithmetic on void pointers
>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
> drivers/gpu/drm/i915/i915_guc_submission.c | 93 +++++++++++++++---------------
> drivers/gpu/drm/i915/intel_guc_loader.c | 2 +-
> drivers/gpu/drm/i915/intel_uc.h | 3 +-
> 3 files changed, 48 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 21dadc1..5870cec 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -133,6 +133,12 @@ static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
> return intel_guc_send(guc, action, ARRAY_SIZE(action));
> }
>
> +static struct guc_context_desc *__get_context_desc(struct i915_guc_client *client)
> +{
> + return (struct guc_context_desc *)((char *)client->guc->ctx_pool_vaddr +
We can use gccisms like using void * for arithmetic computations in the
kernel, i.e.
void *base = client->guc->ctx_pool_vaddr;
return base + sizeof(struct guc_context_desc) * client->ctx_index;
is just fine.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 02/12] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
2017-03-22 9:45 ` Chris Wilson
@ 2017-03-22 9:42 ` Oscar Mateo
2017-03-22 16:57 ` Chris Wilson
0 siblings, 1 reply; 28+ messages in thread
From: Oscar Mateo @ 2017-03-22 9:42 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 03/22/2017 02:45 AM, Chris Wilson wrote:
> On Tue, Mar 21, 2017 at 02:02:47AM -0700, Oscar Mateo wrote:
>> The GuC descriptor is big in size. If we use a local definition of
>> guc_desc we have a chance to overflow stack, so avoid it.
>>
>> Also, Chris abhors scatterlists :)
>>
>> v2: Rebased, helper function to retrieve the context descriptor,
>> s/ctx_pool_vma/ctx_pool/
>>
>> v3: Zero out guc_context_desc before initialization
>>
>> v4: Do not do arithmetic on void pointers
>>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_guc_submission.c | 93 +++++++++++++++---------------
>> drivers/gpu/drm/i915/intel_guc_loader.c | 2 +-
>> drivers/gpu/drm/i915/intel_uc.h | 3 +-
>> 3 files changed, 48 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 21dadc1..5870cec 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -133,6 +133,12 @@ static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
>> return intel_guc_send(guc, action, ARRAY_SIZE(action));
>> }
>>
>> +static struct guc_context_desc *__get_context_desc(struct i915_guc_client *client)
>> +{
>> + return (struct guc_context_desc *)((char *)client->guc->ctx_pool_vaddr +
> We can use gccisms like using void * for arithmetic computations in the
> kernel, i.e.
> void *base = client->guc->ctx_pool_vaddr;
> return base + sizeof(struct guc_context_desc) * client->ctx_index;
> is just fine.
> -Chris
:_(
I got convinced by a previous review comment that arithmetic with void
pointers was frowned upon.
Ok, thanks!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 02/12] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
2017-03-22 9:42 ` Oscar Mateo
@ 2017-03-22 16:57 ` Chris Wilson
0 siblings, 0 replies; 28+ messages in thread
From: Chris Wilson @ 2017-03-22 16:57 UTC (permalink / raw)
To: Oscar Mateo; +Cc: intel-gfx
On Wed, Mar 22, 2017 at 02:42:27AM -0700, Oscar Mateo wrote:
>
>
> On 03/22/2017 02:45 AM, Chris Wilson wrote:
> >On Tue, Mar 21, 2017 at 02:02:47AM -0700, Oscar Mateo wrote:
> >>The GuC descriptor is big in size. If we use a local definition of
> >>guc_desc we have a chance to overflow stack, so avoid it.
> >>
> >>Also, Chris abhors scatterlists :)
> >>
> >>v2: Rebased, helper function to retrieve the context descriptor,
> >>s/ctx_pool_vma/ctx_pool/
> >>
> >>v3: Zero out guc_context_desc before initialization
> >>
> >>v4: Do not do arithmetic on void pointers
> >>
> >>Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >>Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >>Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> >>---
> >> drivers/gpu/drm/i915/i915_guc_submission.c | 93 +++++++++++++++---------------
> >> drivers/gpu/drm/i915/intel_guc_loader.c | 2 +-
> >> drivers/gpu/drm/i915/intel_uc.h | 3 +-
> >> 3 files changed, 48 insertions(+), 50 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> >>index 21dadc1..5870cec 100644
> >>--- a/drivers/gpu/drm/i915/i915_guc_submission.c
> >>+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> >>@@ -133,6 +133,12 @@ static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
> >> return intel_guc_send(guc, action, ARRAY_SIZE(action));
> >> }
> >>+static struct guc_context_desc *__get_context_desc(struct i915_guc_client *client)
> >>+{
> >>+ return (struct guc_context_desc *)((char *)client->guc->ctx_pool_vaddr +
> >We can use gccisms like using void * for arithmetic computations in the
> >kernel, i.e.
> > void *base = client->guc->ctx_pool_vaddr;
> > return base + sizeof(struct guc_context_desc) * client->ctx_index;
> >is just fine.
> >-Chris
>
> :_(
> I got convinced by a previous review comment that arithmetic with
> void pointers was frowned upon.
> Ok, thanks!
Joonas mentioned trying:
struct guc_context_desc *base = client->guc->ctx_pool_vaddr;
return base + client->ctx_index;
or
return &base[client->ctx_index];
for even more idiomatic C.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 03/12] drm/i915/guc: Add onion teardown to the GuC setup
2017-03-21 9:02 [PATCH 00/12] Various improvements around the GuC topic Oscar Mateo
2017-03-21 9:02 ` [PATCH 01/12] drm/i915/guc: Sanitize GuC client initialization Oscar Mateo
2017-03-21 9:02 ` [PATCH 02/12] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access Oscar Mateo
@ 2017-03-21 9:02 ` Oscar Mateo
2017-03-21 9:02 ` [PATCH 04/12] drm/i915/guc: The Additional Data Struct (ADS) should get enabled together with GuC submission Oscar Mateo
` (9 subsequent siblings)
12 siblings, 0 replies; 28+ messages in thread
From: Oscar Mateo @ 2017-03-21 9:02 UTC (permalink / raw)
To: intel-gfx
Starting with intel_guc_loader, down to intel_guc_submission
and finally to intel_guc_log.
v2:
- Null execbuf client outside guc_client_free (Daniele)
- Assert if things try to get allocated twice (Daniele/Joonas)
- Null guc->log.buf_addr when destroyed (Daniele)
- Newline between returning success and error labels (Joonas)
- Remove some unnecessary comments (Joonas)
- Keep guc_log_create_extras naming convention (Joonas)
- Helper function guc_log_has_extras (Joonas)
- No need for separate relay_channel create/destroy. It's just another extra.
- No need to nullify guc->log.flush_wq when destroyed (Joonas)
- Hoist the check for has_extras out of guc_log_create_extras (Joonas)
- Try to do i915_guc_log_register/unregister calls (kind of) symmetric (Daniele)
- Make sure initel_guc_fini is not called before init is ever called (Daniele)
v3:
- Remove unnecessary parenthesis (Joonas)
- Check for logs enabled on debugfs registration
- Rebase on top of Tvrtko's "Fix request re-submission after reset"
v4:
- Rebased
- Comment around enabling/disabling interrupts inside GuC logging (Joonas)
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 11 +-
drivers/gpu/drm/i915/i915_gem.c | 10 +-
drivers/gpu/drm/i915/i915_guc_submission.c | 97 ++++----
drivers/gpu/drm/i915/intel_guc_loader.c | 21 --
drivers/gpu/drm/i915/intel_guc_log.c | 364 ++++++++++++++---------------
drivers/gpu/drm/i915/intel_uc.c | 55 +++--
drivers/gpu/drm/i915/intel_uc.h | 8 +-
7 files changed, 297 insertions(+), 269 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 03d9e45..6d9944a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -549,6 +549,8 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
static void i915_gem_fini(struct drm_i915_private *dev_priv)
{
mutex_lock(&dev_priv->drm.struct_mutex);
+ if (i915.enable_guc_loading)
+ intel_uc_fini_hw(dev_priv);
i915_gem_cleanup_engines(dev_priv);
i915_gem_context_fini(dev_priv);
mutex_unlock(&dev_priv->drm.struct_mutex);
@@ -609,7 +611,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
ret = i915_gem_init(dev_priv);
if (ret)
- goto cleanup_irq;
+ goto cleanup_uc;
intel_modeset_gem_init(dev);
@@ -631,9 +633,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
if (i915_gem_suspend(dev_priv))
DRM_ERROR("failed to idle hardware; continuing to unload!\n");
i915_gem_fini(dev_priv);
+cleanup_uc:
+ intel_uc_fini_fw(dev_priv);
cleanup_irq:
- intel_guc_fini(dev_priv);
- intel_huc_fini(dev_priv);
drm_irq_uninstall(dev);
intel_teardown_gmbus(dev_priv);
cleanup_csr:
@@ -1369,9 +1371,8 @@ void i915_driver_unload(struct drm_device *dev)
/* Flush any outstanding unpin_work. */
drain_workqueue(dev_priv->wq);
- intel_guc_fini(dev_priv);
- intel_huc_fini(dev_priv);
i915_gem_fini(dev_priv);
+ intel_uc_fini_fw(dev_priv);
intel_fbc_cleanup_cfb(dev_priv);
intel_power_domains_fini(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fd611b4..4eb46e4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4596,10 +4596,12 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
intel_mocs_init_l3cc_table(dev_priv);
- /* We can't enable contexts until all firmware is loaded */
- ret = intel_uc_init_hw(dev_priv);
- if (ret)
- goto out;
+ if (i915.enable_guc_loading) {
+ /* We can't enable contexts until all firmware is loaded */
+ ret = intel_uc_init_hw(dev_priv);
+ if (ret)
+ goto out;
+ }
out:
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 5870cec..b24602b 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -912,7 +912,6 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
ida_simple_remove(&guc->ctx_ids, client->ctx_index);
err_client:
kfree(client);
-
return ERR_PTR(ret);
}
@@ -938,7 +937,7 @@ static void guc_policies_init(struct guc_policies *policies)
policies->is_valid = 1;
}
-static void guc_addon_create(struct intel_guc *guc)
+static int guc_addon_create(struct intel_guc *guc)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
struct i915_vma *vma;
@@ -954,14 +953,13 @@ static void guc_addon_create(struct intel_guc *guc)
enum intel_engine_id id;
u32 base;
- vma = guc->ads_vma;
- if (!vma) {
- vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
- if (IS_ERR(vma))
- return;
+ GEM_BUG_ON(guc->ads_vma);
- guc->ads_vma = vma;
- }
+ vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
+ if (IS_ERR(vma))
+ return PTR_ERR(vma);
+
+ guc->ads_vma = vma;
page = i915_vma_first_page(vma);
blob = kmap(page);
@@ -998,6 +996,13 @@ static void guc_addon_create(struct intel_guc *guc)
blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state);
kunmap(page);
+
+ return 0;
+}
+
+static void guc_addon_destroy(struct intel_guc *guc)
+{
+ i915_vma_unpin_and_release(&guc->ads_vma);
}
/*
@@ -1012,6 +1017,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
struct intel_guc *guc = &dev_priv->guc;
struct i915_vma *vma;
void *vaddr;
+ int ret;
if (!HAS_GUC_SCHED(dev_priv))
return 0;
@@ -1021,10 +1027,10 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
i915_guc_submission_disable(dev_priv);
if (!i915.enable_guc_submission)
- return 0; /* not enabled */
+ return 0;
if (guc->ctx_pool)
- return 0; /* already allocated */
+ return 0;
vma = intel_guc_allocate_vma(guc, gemsize);
if (IS_ERR(vma))
@@ -1032,15 +1038,23 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
guc->ctx_pool = vma;
- vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
- if (IS_ERR(vaddr))
- goto err;
+ vaddr = i915_gem_object_pin_map(guc->ctx_pool->obj, I915_MAP_WB);
+ if (IS_ERR(vaddr)) {
+ ret = PTR_ERR(vaddr);
+ goto err_vma;
+ }
guc->ctx_pool_vaddr = vaddr;
+ ret = intel_guc_log_create(guc);
+ if (ret < 0)
+ goto err_vaddr;
+
+ ret = guc_addon_create(guc);
+ if (ret < 0)
+ goto err_log;
+
ida_init(&guc->ctx_ids);
- intel_guc_log_create(guc);
- guc_addon_create(guc);
guc->execbuf_client = guc_client_alloc(dev_priv,
INTEL_INFO(dev_priv)->ring_mask,
@@ -1048,14 +1062,37 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
dev_priv->kernel_context);
if (IS_ERR(guc->execbuf_client)) {
DRM_ERROR("Failed to create GuC client for execbuf!\n");
- goto err;
+ ret = PTR_ERR(guc->execbuf_client);
+ goto err_ads;
}
return 0;
-err:
- i915_guc_submission_fini(dev_priv);
- return -ENOMEM;
+err_ads:
+ guc_addon_destroy(guc);
+err_log:
+ intel_guc_log_destroy(guc);
+err_vaddr:
+ i915_gem_object_unpin_map(guc->ctx_pool->obj);
+err_vma:
+ i915_vma_unpin_and_release(&guc->ctx_pool);
+ return ret;
+}
+
+void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
+{
+ struct intel_guc *guc = &dev_priv->guc;
+
+ if (!i915.enable_guc_submission)
+ return 0;
+
+ guc_client_free(guc->execbuf_client);
+ guc->execbuf_client = NULL;
+ ida_destroy(&guc->ctx_ids);
+ guc_addon_destroy(guc);
+ intel_guc_log_destroy(guc);
+ i915_gem_object_unpin_map(guc->ctx_pool->obj);
+ i915_vma_unpin_and_release(&guc->ctx_pool);
}
static void guc_reset_wq(struct i915_guc_client *client)
@@ -1200,26 +1237,6 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
intel_engines_reset_default_submission(dev_priv);
}
-void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
-{
- struct intel_guc *guc = &dev_priv->guc;
- struct i915_guc_client *client;
-
- client = fetch_and_zero(&guc->execbuf_client);
- if (client && !IS_ERR(client))
- guc_client_free(client);
-
- i915_vma_unpin_and_release(&guc->ads_vma);
- i915_vma_unpin_and_release(&guc->log.vma);
-
- if (guc->ctx_pool_vaddr) {
- ida_destroy(&guc->ctx_ids);
- i915_gem_object_unpin_map(guc->ctx_pool->obj);
- }
-
- i915_vma_unpin_and_release(&guc->ctx_pool);
-}
-
/**
* intel_guc_suspend() - notify GuC entering suspend state
* @dev_priv: i915 device private
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 1a6e478..b8ba28d 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -430,24 +430,3 @@ int intel_guc_select_fw(struct intel_guc *guc)
return 0;
}
-
-/**
- * intel_guc_fini() - clean up all allocated resources
- * @dev_priv: i915 device private
- */
-void intel_guc_fini(struct drm_i915_private *dev_priv)
-{
- struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
- struct drm_i915_gem_object *obj;
-
- mutex_lock(&dev_priv->drm.struct_mutex);
- i915_guc_submission_disable(dev_priv);
- i915_guc_submission_fini(dev_priv);
- mutex_unlock(&dev_priv->drm.struct_mutex);
-
- obj = fetch_and_zero(&guc_fw->obj);
- if (obj)
- i915_gem_object_put(obj);
-
- guc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
-}
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 5c0f9a4..b39cdd6 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -66,7 +66,6 @@ static int guc_log_control(struct intel_guc *guc, u32 control_val)
return intel_guc_send(guc, action, ARRAY_SIZE(action));
}
-
/*
* Sub buffer switch callback. Called whenever relay has to switch to a new
* sub buffer, relay stays on the same sub buffer if 0 is returned.
@@ -139,45 +138,15 @@ static int remove_buf_file_callback(struct dentry *dentry)
.remove_buf_file = remove_buf_file_callback,
};
-static void guc_log_remove_relay_file(struct intel_guc *guc)
-{
- relay_close(guc->log.relay_chan);
-}
-
-static int guc_log_create_relay_channel(struct intel_guc *guc)
-{
- struct drm_i915_private *dev_priv = guc_to_i915(guc);
- struct rchan *guc_log_relay_chan;
- size_t n_subbufs, subbuf_size;
-
- /* Keep the size of sub buffers same as shared log buffer */
- subbuf_size = guc->log.vma->obj->base.size;
-
- /* Store up to 8 snapshots, which is large enough to buffer sufficient
- * boot time logs and provides enough leeway to User, in terms of
- * latency, for consuming the logs from relay. Also doesn't take
- * up too much memory.
- */
- n_subbufs = 8;
-
- guc_log_relay_chan = relay_open(NULL, NULL, subbuf_size,
- n_subbufs, &relay_callbacks, dev_priv);
- if (!guc_log_relay_chan) {
- DRM_ERROR("Couldn't create relay chan for GuC logging\n");
- return -ENOMEM;
- }
-
- GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
- guc->log.relay_chan = guc_log_relay_chan;
- return 0;
-}
-
-static int guc_log_create_relay_file(struct intel_guc *guc)
+static int guc_log_relay_file_create(struct intel_guc *guc)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
struct dentry *log_dir;
int ret;
+ if (i915.guc_log_level < 0)
+ return 0;
+
/* For now create the log file in /sys/kernel/debug/dri/0 dir */
log_dir = dev_priv->drm.primary->debugfs_root;
@@ -198,7 +167,7 @@ static int guc_log_create_relay_file(struct intel_guc *guc)
}
ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir);
- if (ret) {
+ if (ret < 0 && ret != -EEXIST) {
DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
return ret;
}
@@ -371,31 +340,6 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
}
}
-static void guc_log_cleanup(struct intel_guc *guc)
-{
- struct drm_i915_private *dev_priv = guc_to_i915(guc);
-
- lockdep_assert_held(&dev_priv->drm.struct_mutex);
-
- /* First disable the flush interrupt */
- gen9_disable_guc_interrupts(dev_priv);
-
- if (guc->log.flush_wq)
- destroy_workqueue(guc->log.flush_wq);
-
- guc->log.flush_wq = NULL;
-
- if (guc->log.relay_chan)
- guc_log_remove_relay_file(guc);
-
- guc->log.relay_chan = NULL;
-
- if (guc->log.buf_addr)
- i915_gem_object_unpin_map(guc->log.vma->obj);
-
- guc->log.buf_addr = NULL;
-}
-
static void capture_logs_work(struct work_struct *work)
{
struct intel_guc *guc =
@@ -404,120 +348,105 @@ static void capture_logs_work(struct work_struct *work)
guc_log_capture_logs(guc);
}
+static bool guc_log_has_extras(struct intel_guc *guc)
+{
+ return guc->log.buf_addr != NULL;
+}
+
static int guc_log_create_extras(struct intel_guc *guc)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
void *vaddr;
- int ret;
+ struct rchan *guc_log_relay_chan;
+ size_t n_subbufs, subbuf_size;
+ int ret = 0;
lockdep_assert_held(&dev_priv->drm.struct_mutex);
- /* Nothing to do */
- if (i915.guc_log_level < 0)
- return 0;
-
- if (!guc->log.buf_addr) {
- /* Create a WC (Uncached for read) vmalloc mapping of log
- * buffer pages, so that we can directly get the data
- * (up-to-date) from memory.
- */
- vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
- if (IS_ERR(vaddr)) {
- ret = PTR_ERR(vaddr);
- DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
- return ret;
- }
-
- guc->log.buf_addr = vaddr;
- }
+ GEM_BUG_ON(guc_log_has_extras(guc));
- if (!guc->log.relay_chan) {
- /* Create a relay channel, so that we have buffers for storing
- * the GuC firmware logs, the channel will be linked with a file
- * later on when debugfs is registered.
- */
- ret = guc_log_create_relay_channel(guc);
- if (ret)
- return ret;
+ /* Create a WC (Uncached for read) vmalloc mapping of log
+ * buffer pages, so that we can directly get the data
+ * (up-to-date) from memory.
+ */
+ vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
+ if (IS_ERR(vaddr)) {
+ DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
+ return PTR_ERR(vaddr);
}
- if (!guc->log.flush_wq) {
- INIT_WORK(&guc->log.flush_work, capture_logs_work);
-
- /*
- * GuC log buffer flush work item has to do register access to
- * send the ack to GuC and this work item, if not synced before
- * suspend, can potentially get executed after the GFX device is
- * suspended.
- * By marking the WQ as freezable, we don't have to bother about
- * flushing of this work item from the suspend hooks, the pending
- * work item if any will be either executed before the suspend
- * or scheduled later on resume. This way the handling of work
- * item can be kept same between system suspend & rpm suspend.
- */
- guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
- WQ_HIGHPRI | WQ_FREEZABLE);
- if (guc->log.flush_wq == NULL) {
- DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
- return -ENOMEM;
- }
- }
+ guc->log.buf_addr = vaddr;
- return 0;
-}
+ /* Keep the size of sub buffers same as shared log buffer */
+ subbuf_size = guc->log.vma->obj->base.size;
-void intel_guc_log_create(struct intel_guc *guc)
-{
- struct i915_vma *vma;
- unsigned long offset;
- uint32_t size, flags;
+ /* Store up to 8 snapshots, which is large enough to buffer sufficient
+ * boot time logs and provides enough leeway to User, in terms of
+ * latency, for consuming the logs from relay. Also doesn't take
+ * up too much memory.
+ */
+ n_subbufs = 8;
- if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
- i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
+ /* Create a relay channel, so that we have buffers for storing
+ * the GuC firmware logs, the channel will be linked with a file
+ * later on when debugfs is registered.
+ */
+ guc_log_relay_chan = relay_open(NULL, NULL, subbuf_size,
+ n_subbufs, &relay_callbacks, dev_priv);
+ if (!guc_log_relay_chan) {
+ DRM_ERROR("Couldn't create relay chan for GuC logging\n");
- /* The first page is to save log buffer state. Allocate one
- * extra page for others in case for overlap */
- size = (1 + GUC_LOG_DPC_PAGES + 1 +
- GUC_LOG_ISR_PAGES + 1 +
- GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
+ ret = -ENOMEM;
+ goto err_vaddr;
+ }
- vma = guc->log.vma;
- if (!vma) {
- /* We require SSE 4.1 for fast reads from the GuC log buffer and
- * it should be present on the chipsets supporting GuC based
- * submisssions.
- */
- if (WARN_ON(!i915_has_memcpy_from_wc())) {
- /* logging will not be enabled */
- i915.guc_log_level = -1;
- return;
- }
+ GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
+ guc->log.relay_chan = guc_log_relay_chan;
- vma = intel_guc_allocate_vma(guc, size);
- if (IS_ERR(vma)) {
- /* logging will be off */
- i915.guc_log_level = -1;
- return;
- }
+ INIT_WORK(&guc->log.flush_work, capture_logs_work);
+
+ /*
+ * GuC log buffer flush work item has to do register access to
+ * send the ack to GuC and this work item, if not synced before
+ * suspend, can potentially get executed after the GFX device is
+ * suspended.
+ * By marking the WQ as freezable, we don't have to bother about
+ * flushing of this work item from the suspend hooks, the pending
+ * work item if any will be either executed before the suspend
+ * or scheduled later on resume. This way the handling of work
+ * item can be kept same between system suspend & rpm suspend.
+ */
+ guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
+ WQ_HIGHPRI | WQ_FREEZABLE);
+ if (!guc->log.flush_wq) {
+ DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
+ ret = -ENOMEM;
+ goto err_relaychan;
+ }
- guc->log.vma = vma;
+ return 0;
- if (guc_log_create_extras(guc)) {
- guc_log_cleanup(guc);
- i915_vma_unpin_and_release(&guc->log.vma);
- i915.guc_log_level = -1;
- return;
- }
- }
+err_relaychan:
+ relay_close(guc->log.relay_chan);
+err_vaddr:
+ i915_gem_object_unpin_map(guc->log.vma->obj);
+ guc->log.buf_addr = NULL;
+ return ret;
+}
- /* each allocated unit is a page */
- flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
- (GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
- (GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
- (GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
+static void guc_log_destroy_extras(struct intel_guc *guc)
+{
+ /*
+ * It's possible that extras were never allocated because guc_log_level
+ * was < 0 at the time
+ **/
+ if (!guc_log_has_extras(guc))
+ return;
- offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
- guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
+ destroy_workqueue(guc->log.flush_wq);
+ relay_close(guc->log.relay_chan);
+ i915_gem_object_unpin_map(guc->log.vma->obj);
+ guc->log.buf_addr = NULL;
}
static int guc_log_late_setup(struct intel_guc *guc)
@@ -527,24 +456,25 @@ static int guc_log_late_setup(struct intel_guc *guc)
lockdep_assert_held(&dev_priv->drm.struct_mutex);
- if (i915.guc_log_level < 0)
- return -EINVAL;
+ if (!guc_log_has_extras(guc)) {
+ /* If log_level was set as -1 at boot time, then setup needed to
+ * handle log buffer flush interrupts would not have been done yet,
+ * so do that now.
+ */
+ ret = guc_log_create_extras(guc);
+ if (ret)
+ goto err;
+ }
- /* If log_level was set as -1 at boot time, then setup needed to
- * handle log buffer flush interrupts would not have been done yet,
- * so do that now.
- */
- ret = guc_log_create_extras(guc);
+ ret = guc_log_relay_file_create(guc);
if (ret)
- goto err;
-
- ret = guc_log_create_relay_file(guc);
- if (ret)
- goto err;
+ goto err_extras;
return 0;
+
+err_extras:
+ guc_log_destroy_extras(guc);
err:
- guc_log_cleanup(guc);
/* logging will remain off */
i915.guc_log_level = -1;
return ret;
@@ -586,6 +516,72 @@ static void guc_flush_logs(struct intel_guc *guc)
guc_log_capture_logs(guc);
}
+int intel_guc_log_create(struct intel_guc *guc)
+{
+ struct i915_vma *vma;
+ unsigned long offset;
+ uint32_t size, flags;
+ int ret;
+
+ GEM_BUG_ON(guc->log.vma);
+
+ if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
+ i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
+
+ /* The first page is to save log buffer state. Allocate one
+ * extra page for others in case for overlap */
+ size = (1 + GUC_LOG_DPC_PAGES + 1 +
+ GUC_LOG_ISR_PAGES + 1 +
+ GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
+
+ /* We require SSE 4.1 for fast reads from the GuC log buffer and
+ * it should be present on the chipsets supporting GuC based
+ * submisssions.
+ */
+ if (WARN_ON(!i915_has_memcpy_from_wc())) {
+ ret = -EINVAL;
+ goto err;
+ }
+
+ vma = intel_guc_allocate_vma(guc, size);
+ if (IS_ERR(vma)) {
+ ret = PTR_ERR(vma);
+ goto err;
+ }
+
+ guc->log.vma = vma;
+
+ if (i915.guc_log_level >= 0) {
+ ret = guc_log_create_extras(guc);
+ if (ret < 0)
+ goto err_vma;
+ }
+
+ /* each allocated unit is a page */
+ flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
+ (GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
+ (GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
+ (GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
+
+ offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
+ guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
+
+ return 0;
+
+err_vma:
+ i915_vma_unpin_and_release(&guc->log.vma);
+err:
+ /* logging will be off */
+ i915.guc_log_level = -1;
+ return ret;
+}
+
+void intel_guc_log_destroy(struct intel_guc *guc)
+{
+ guc_log_destroy_extras(guc);
+ i915_vma_unpin_and_release(&guc->log.vma);
+}
+
int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
{
struct intel_guc *guc = &dev_priv->guc;
@@ -609,17 +605,22 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
return ret;
}
- i915.guc_log_level = log_param.verbosity;
+ if (log_param.logging_enabled) {
+ i915.guc_log_level = log_param.verbosity;
- /* If log_level was set as -1 at boot time, then the relay channel file
- * wouldn't have been created by now and interrupts also would not have
- * been enabled.
- */
- if (!dev_priv->guc.log.relay_chan) {
+ /* If log_level was set as -1 at boot time, then the relay channel file
+ * wouldn't have been created by now and interrupts also would not have
+ * been enabled. Try again now, just in case.
+ */
ret = guc_log_late_setup(guc);
- if (!ret)
- gen9_enable_guc_interrupts(dev_priv);
- } else if (!log_param.logging_enabled) {
+ if (ret < 0) {
+ DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
+ return ret;
+ }
+
+ /* GuC logging is currently the only user of Guc2Host interrupts */
+ gen9_enable_guc_interrupts(dev_priv);
+ } else {
/* Once logging is disabled, GuC won't generate logs & send an
* interrupt. But there could be some data in the log buffer
* which is yet to be captured. So request GuC to update the log
@@ -629,9 +630,6 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
/* As logging is disabled, update log level to reflect that */
i915.guc_log_level = -1;
- } else {
- /* In case interrupts were disabled, enable them now */
- gen9_enable_guc_interrupts(dev_priv);
}
return ret;
@@ -639,7 +637,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
void i915_guc_log_register(struct drm_i915_private *dev_priv)
{
- if (!i915.enable_guc_submission)
+ if (!i915.enable_guc_submission || i915.guc_log_level < 0)
return;
mutex_lock(&dev_priv->drm.struct_mutex);
@@ -653,6 +651,8 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
return;
mutex_lock(&dev_priv->drm.struct_mutex);
- guc_log_cleanup(&dev_priv->guc);
+ /* GuC logging is currently the only user of Guc2Host interrupts */
+ gen9_disable_guc_interrupts(dev_priv);
+ guc_log_destroy_extras(&dev_priv->guc);
mutex_unlock(&dev_priv->drm.struct_mutex);
}
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 21f6d82..516e57d 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -95,24 +95,41 @@ void intel_uc_init_fw(struct drm_i915_private *dev_priv)
intel_uc_prepare_fw(dev_priv, &dev_priv->guc.fw);
}
+void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
+{
+ struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
+ struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
+ struct drm_i915_gem_object *obj;
+
+ obj = fetch_and_zero(&guc_fw->obj);
+ if (obj)
+ i915_gem_object_put(obj);
+
+ guc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
+
+ obj = fetch_and_zero(&huc_fw->obj);
+ if (obj)
+ i915_gem_object_put(obj);
+
+ huc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
+}
+
int intel_uc_init_hw(struct drm_i915_private *dev_priv)
{
int ret, attempts;
- /* GuC not enabled, nothing to do */
- if (!i915.enable_guc_loading)
- return 0;
-
gen9_reset_guc_interrupts(dev_priv);
/* We need to notify the guc whenever we change the GGTT */
i915_ggtt_enable_guc(dev_priv);
- if (i915.enable_guc_submission) {
- ret = i915_guc_submission_init(dev_priv);
- if (ret)
- goto err;
- }
+ /*
+ * This is stuff we need to have available at fw load time
+ * if we are planning to enable submission later
+ */
+ ret = i915_guc_submission_init(dev_priv);
+ if (ret)
+ goto err_guc;
/* WaEnableuKernelHeaderValidFix:skl */
/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
@@ -150,7 +167,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
ret = i915_guc_submission_enable(dev_priv);
if (ret)
- goto err_submission;
+ goto err_interrupts;
}
return 0;
@@ -164,11 +181,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
* nonfatal error (i.e. it doesn't prevent driver load, but
* marks the GPU as wedged until reset).
*/
+err_interrupts:
+ gen9_disable_guc_interrupts(dev_priv);
err_submission:
- if (i915.enable_guc_submission)
- i915_guc_submission_fini(dev_priv);
-
-err:
+ i915_guc_submission_fini(dev_priv);
+err_guc:
i915_ggtt_disable_guc(dev_priv);
DRM_ERROR("GuC init failed\n");
@@ -185,6 +202,16 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
return ret;
}
+void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
+{
+ if (i915.enable_guc_submission) {
+ i915_guc_submission_disable(dev_priv);
+ gen9_disable_guc_interrupts(dev_priv);
+ }
+ i915_guc_submission_fini(dev_priv);
+ i915_ggtt_disable_guc(dev_priv);
+}
+
/*
* Read GuC command/status register (SOFT_SCRATCH_0)
* Return true if it contains a response rather than a command
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 77f7153..eb30e7a 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -187,7 +187,9 @@ struct intel_huc {
void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
void intel_uc_init_early(struct drm_i915_private *dev_priv);
void intel_uc_init_fw(struct drm_i915_private *dev_priv);
+void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
int intel_uc_init_hw(struct drm_i915_private *dev_priv);
+void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
struct intel_uc_fw *uc_fw);
int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
@@ -196,7 +198,6 @@ void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
/* intel_guc_loader.c */
int intel_guc_select_fw(struct intel_guc *guc);
int intel_guc_init_hw(struct intel_guc *guc);
-void intel_guc_fini(struct drm_i915_private *dev_priv);
const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
int intel_guc_suspend(struct drm_i915_private *dev_priv);
int intel_guc_resume(struct drm_i915_private *dev_priv);
@@ -212,10 +213,11 @@ void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
/* intel_guc_log.c */
-void intel_guc_log_create(struct intel_guc *guc);
+int intel_guc_log_create(struct intel_guc *guc);
+void intel_guc_log_destroy(struct intel_guc *guc);
+int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
void i915_guc_log_register(struct drm_i915_private *dev_priv);
void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
-int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
static inline u32 guc_ggtt_offset(struct i915_vma *vma)
{
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 04/12] drm/i915/guc: The Additional Data Struct (ADS) should get enabled together with GuC submission
2017-03-21 9:02 [PATCH 00/12] Various improvements around the GuC topic Oscar Mateo
` (2 preceding siblings ...)
2017-03-21 9:02 ` [PATCH 03/12] drm/i915/guc: Add onion teardown to the GuC setup Oscar Mateo
@ 2017-03-21 9:02 ` Oscar Mateo
2017-03-21 17:51 ` Ceraolo Spurio, Daniele
2017-03-21 9:02 ` [PATCH 05/12] drm/i915/guc: Break out the GuC log extras into their own "runtime" struct Oscar Mateo
` (8 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Oscar Mateo @ 2017-03-21 9:02 UTC (permalink / raw)
To: intel-gfx
It's mandatory and it gets created if and only if GuC submission is enabled, so that should be
the condition for informing the GuC about it.
Also s/guc_addon_create/guc_ads_create and s/guc_addon_destroy/guc_ads_destroy and, while
at it, add an explanation of what things go inside the ADS object.
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 17 ++++++++++++-----
drivers/gpu/drm/i915/intel_guc_loader.c | 10 ++++------
2 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index b24602b..ff8235a1 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -62,6 +62,13 @@
* ELSP context descriptor dword into Work Item.
* See guc_wq_item_append()
*
+ * ADS:
+ * The Additional Data Struct (ADS) has pointers for different buffers used by
+ * the GuC. One single gem object contains the ADS struct itself (guc_ads), the
+ * scheduling policies (guc_policies), a structure describing a collection of
+ * register sets (guc_mmio_reg_state) and some extra pages for the GuC to save
+ * its internal state for sleep.
+ *
*/
static inline bool is_high_priority(struct i915_guc_client* client)
@@ -937,7 +944,7 @@ static void guc_policies_init(struct guc_policies *policies)
policies->is_valid = 1;
}
-static int guc_addon_create(struct intel_guc *guc)
+static int guc_ads_create(struct intel_guc *guc)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
struct i915_vma *vma;
@@ -1000,7 +1007,7 @@ static int guc_addon_create(struct intel_guc *guc)
return 0;
}
-static void guc_addon_destroy(struct intel_guc *guc)
+static void guc_ads_destroy(struct intel_guc *guc)
{
i915_vma_unpin_and_release(&guc->ads_vma);
}
@@ -1050,7 +1057,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
if (ret < 0)
goto err_vaddr;
- ret = guc_addon_create(guc);
+ ret = guc_ads_create(guc);
if (ret < 0)
goto err_log;
@@ -1069,7 +1076,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
return 0;
err_ads:
- guc_addon_destroy(guc);
+ guc_ads_destroy(guc);
err_log:
intel_guc_log_destroy(guc);
err_vaddr:
@@ -1089,7 +1096,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
guc_client_free(guc->execbuf_client);
guc->execbuf_client = NULL;
ida_destroy(&guc->ctx_ids);
- guc_addon_destroy(guc);
+ guc_ads_destroy(guc);
intel_guc_log_destroy(guc);
i915_gem_object_unpin_map(guc->ctx_pool->obj);
i915_vma_unpin_and_release(&guc->ctx_pool);
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index b8ba28d..62ebf56 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -148,17 +148,15 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
} else
params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
- if (guc->ads_vma) {
- u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
- params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
- params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED;
- }
-
/* If GuC submission is enabled, set up additional parameters here */
if (i915.enable_guc_submission) {
+ u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
u32 pgs = guc_ggtt_offset(dev_priv->guc.ctx_pool);
u32 ctx_in_16 = GUC_MAX_GPU_CONTEXTS / 16;
+ params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
+ params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED;
+
pgs >>= PAGE_SHIFT;
params[GUC_CTL_CTXINFO] = (pgs << GUC_CTL_BASE_ADDR_SHIFT) |
(ctx_in_16 << GUC_CTL_CTXNUM_IN16_SHIFT);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 04/12] drm/i915/guc: The Additional Data Struct (ADS) should get enabled together with GuC submission
2017-03-21 9:02 ` [PATCH 04/12] drm/i915/guc: The Additional Data Struct (ADS) should get enabled together with GuC submission Oscar Mateo
@ 2017-03-21 17:51 ` Ceraolo Spurio, Daniele
0 siblings, 0 replies; 28+ messages in thread
From: Ceraolo Spurio, Daniele @ 2017-03-21 17:51 UTC (permalink / raw)
To: Oscar Mateo, intel-gfx
On 3/21/2017 2:02 AM, Oscar Mateo wrote:
> It's mandatory and it gets created if and only if GuC submission is enabled, so that should be
> the condition for informing the GuC about it.
This is true now, but GuC might actually require the ADS even if GuC
submission is disabled, depending on the features being used. In the
future we might therefore want to gate ADS logic based on the value of
guc_loading instead of guc_submission.
This change is coherent with the current flow and makes things cleaner, so:
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>
> Also s/guc_addon_create/guc_ads_create and s/guc_addon_destroy/guc_ads_destroy and, while
> at it, add an explanation of what things go inside the ADS object.
>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 05/12] drm/i915/guc: Break out the GuC log extras into their own "runtime" struct
2017-03-21 9:02 [PATCH 00/12] Various improvements around the GuC topic Oscar Mateo
` (3 preceding siblings ...)
2017-03-21 9:02 ` [PATCH 04/12] drm/i915/guc: The Additional Data Struct (ADS) should get enabled together with GuC submission Oscar Mateo
@ 2017-03-21 9:02 ` Oscar Mateo
2017-03-21 9:02 ` [PATCH 06/12] drm/i915/guc: Make intel_guc_send a function pointer Oscar Mateo
` (7 subsequent siblings)
12 siblings, 0 replies; 28+ messages in thread
From: Oscar Mateo @ 2017-03-21 9:02 UTC (permalink / raw)
To: intel-gfx
When initializing the GuC log struct, there is an object we need to
allocate always, since the GuC needs its address at fw load time.
The rest is only needed during runtime, in the sense that we only
create if we actually enable GuC logging. Make that distinction
explicit by subdividing further the intel_guc_log struct.
v2: Call the new struct "runtime", instead of "extras" (Joonas)
v3: Check indent (Joonas)
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_irq.c | 4 +-
drivers/gpu/drm/i915/intel_guc_log.c | 72 ++++++++++++++++++------------------
drivers/gpu/drm/i915/intel_uc.h | 12 +++---
3 files changed, 45 insertions(+), 43 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index cb20c94..3d2e623 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1741,8 +1741,8 @@ static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
I915_WRITE(SOFT_SCRATCH(15), msg & ~flush);
/* Handle flush interrupt in bottom half */
- queue_work(dev_priv->guc.log.flush_wq,
- &dev_priv->guc.log.flush_work);
+ queue_work(dev_priv->guc.log.runtime.flush_wq,
+ &dev_priv->guc.log.runtime.flush_work);
dev_priv->guc.log.flush_interrupt_count++;
} else {
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index b39cdd6..6fb63a3 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -166,7 +166,7 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
return -ENODEV;
}
- ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir);
+ ret = relay_late_setup_files(guc->log.runtime.relay_chan, "guc_log", log_dir);
if (ret < 0 && ret != -EEXIST) {
DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
return ret;
@@ -183,15 +183,15 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
smp_wmb();
/* All data has been written, so now move the offset of sub buffer. */
- relay_reserve(guc->log.relay_chan, guc->log.vma->obj->base.size);
+ relay_reserve(guc->log.runtime.relay_chan, guc->log.vma->obj->base.size);
/* Switch to the next sub buffer */
- relay_flush(guc->log.relay_chan);
+ relay_flush(guc->log.runtime.relay_chan);
}
static void *guc_get_write_buffer(struct intel_guc *guc)
{
- if (!guc->log.relay_chan)
+ if (!guc->log.runtime.relay_chan)
return NULL;
/* Just get the base address of a new sub buffer and copy data into it
@@ -202,7 +202,7 @@ static void *guc_get_write_buffer(struct intel_guc *guc)
* done without using relay_reserve() along with relay_write(). So its
* better to use relay_reserve() alone.
*/
- return relay_reserve(guc->log.relay_chan, 0);
+ return relay_reserve(guc->log.runtime.relay_chan, 0);
}
static bool guc_check_log_buf_overflow(struct intel_guc *guc,
@@ -253,11 +253,11 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
void *src_data, *dst_data;
bool new_overflow;
- if (WARN_ON(!guc->log.buf_addr))
+ if (WARN_ON(!guc->log.runtime.buf_addr))
return;
/* Get the pointer to shared GuC log buffer */
- log_buf_state = src_data = guc->log.buf_addr;
+ log_buf_state = src_data = guc->log.runtime.buf_addr;
/* Get the pointer to local buffer to store the logs */
log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
@@ -343,17 +343,17 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
static void capture_logs_work(struct work_struct *work)
{
struct intel_guc *guc =
- container_of(work, struct intel_guc, log.flush_work);
+ container_of(work, struct intel_guc, log.runtime.flush_work);
guc_log_capture_logs(guc);
}
-static bool guc_log_has_extras(struct intel_guc *guc)
+static bool guc_log_has_runtime(struct intel_guc *guc)
{
- return guc->log.buf_addr != NULL;
+ return guc->log.runtime.buf_addr != NULL;
}
-static int guc_log_create_extras(struct intel_guc *guc)
+static int guc_log_runtime_create(struct intel_guc *guc)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
void *vaddr;
@@ -363,7 +363,7 @@ static int guc_log_create_extras(struct intel_guc *guc)
lockdep_assert_held(&dev_priv->drm.struct_mutex);
- GEM_BUG_ON(guc_log_has_extras(guc));
+ GEM_BUG_ON(guc_log_has_runtime(guc));
/* Create a WC (Uncached for read) vmalloc mapping of log
* buffer pages, so that we can directly get the data
@@ -375,7 +375,7 @@ static int guc_log_create_extras(struct intel_guc *guc)
return PTR_ERR(vaddr);
}
- guc->log.buf_addr = vaddr;
+ guc->log.runtime.buf_addr = vaddr;
/* Keep the size of sub buffers same as shared log buffer */
subbuf_size = guc->log.vma->obj->base.size;
@@ -401,9 +401,9 @@ static int guc_log_create_extras(struct intel_guc *guc)
}
GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
- guc->log.relay_chan = guc_log_relay_chan;
+ guc->log.runtime.relay_chan = guc_log_relay_chan;
- INIT_WORK(&guc->log.flush_work, capture_logs_work);
+ INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
/*
* GuC log buffer flush work item has to do register access to
@@ -416,9 +416,9 @@ static int guc_log_create_extras(struct intel_guc *guc)
* or scheduled later on resume. This way the handling of work
* item can be kept same between system suspend & rpm suspend.
*/
- guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
- WQ_HIGHPRI | WQ_FREEZABLE);
- if (!guc->log.flush_wq) {
+ guc->log.runtime.flush_wq = alloc_ordered_workqueue("i915-guc_log",
+ WQ_HIGHPRI | WQ_FREEZABLE);
+ if (!guc->log.runtime.flush_wq) {
DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
ret = -ENOMEM;
goto err_relaychan;
@@ -427,26 +427,26 @@ static int guc_log_create_extras(struct intel_guc *guc)
return 0;
err_relaychan:
- relay_close(guc->log.relay_chan);
+ relay_close(guc->log.runtime.relay_chan);
err_vaddr:
i915_gem_object_unpin_map(guc->log.vma->obj);
- guc->log.buf_addr = NULL;
+ guc->log.runtime.buf_addr = NULL;
return ret;
}
-static void guc_log_destroy_extras(struct intel_guc *guc)
+static void guc_log_runtime_destroy(struct intel_guc *guc)
{
/*
- * It's possible that extras were never allocated because guc_log_level
- * was < 0 at the time
+ * It's possible that the runtime stuff was never allocated because
+ * guc_log_level was < 0 at the time
**/
- if (!guc_log_has_extras(guc))
+ if (!guc_log_has_runtime(guc))
return;
- destroy_workqueue(guc->log.flush_wq);
- relay_close(guc->log.relay_chan);
+ destroy_workqueue(guc->log.runtime.flush_wq);
+ relay_close(guc->log.runtime.relay_chan);
i915_gem_object_unpin_map(guc->log.vma->obj);
- guc->log.buf_addr = NULL;
+ guc->log.runtime.buf_addr = NULL;
}
static int guc_log_late_setup(struct intel_guc *guc)
@@ -456,24 +456,24 @@ static int guc_log_late_setup(struct intel_guc *guc)
lockdep_assert_held(&dev_priv->drm.struct_mutex);
- if (!guc_log_has_extras(guc)) {
+ if (!guc_log_has_runtime(guc)) {
/* If log_level was set as -1 at boot time, then setup needed to
* handle log buffer flush interrupts would not have been done yet,
* so do that now.
*/
- ret = guc_log_create_extras(guc);
+ ret = guc_log_runtime_create(guc);
if (ret)
goto err;
}
ret = guc_log_relay_file_create(guc);
if (ret)
- goto err_extras;
+ goto err_runtime;
return 0;
-err_extras:
- guc_log_destroy_extras(guc);
+err_runtime:
+ guc_log_runtime_destroy(guc);
err:
/* logging will remain off */
i915.guc_log_level = -1;
@@ -507,7 +507,7 @@ static void guc_flush_logs(struct intel_guc *guc)
/* Before initiating the forceful flush, wait for any pending/ongoing
* flush to complete otherwise forceful flush may not actually happen.
*/
- flush_work(&guc->log.flush_work);
+ flush_work(&guc->log.runtime.flush_work);
/* Ask GuC to update the log buffer state */
guc_log_flush(guc);
@@ -552,7 +552,7 @@ int intel_guc_log_create(struct intel_guc *guc)
guc->log.vma = vma;
if (i915.guc_log_level >= 0) {
- ret = guc_log_create_extras(guc);
+ ret = guc_log_runtime_create(guc);
if (ret < 0)
goto err_vma;
}
@@ -578,7 +578,7 @@ int intel_guc_log_create(struct intel_guc *guc)
void intel_guc_log_destroy(struct intel_guc *guc)
{
- guc_log_destroy_extras(guc);
+ guc_log_runtime_destroy(guc);
i915_vma_unpin_and_release(&guc->log.vma);
}
@@ -653,6 +653,6 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
mutex_lock(&dev_priv->drm.struct_mutex);
/* GuC logging is currently the only user of Guc2Host interrupts */
gen9_disable_guc_interrupts(dev_priv);
- guc_log_destroy_extras(&dev_priv->guc);
+ guc_log_runtime_destroy(&dev_priv->guc);
mutex_unlock(&dev_priv->drm.struct_mutex);
}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index eb30e7a..c6f880c 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -132,11 +132,13 @@ struct intel_uc_fw {
struct intel_guc_log {
uint32_t flags;
struct i915_vma *vma;
- void *buf_addr;
- struct workqueue_struct *flush_wq;
- struct work_struct flush_work;
- struct rchan *relay_chan;
-
+ /* The runtime stuff gets created only when GuC logging gets enabled */
+ struct {
+ void *buf_addr;
+ struct workqueue_struct *flush_wq;
+ struct work_struct flush_work;
+ struct rchan *relay_chan;
+ } runtime;
/* logging related stats */
u32 capture_miss_count;
u32 flush_interrupt_count;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 06/12] drm/i915/guc: Make intel_guc_send a function pointer
2017-03-21 9:02 [PATCH 00/12] Various improvements around the GuC topic Oscar Mateo
` (4 preceding siblings ...)
2017-03-21 9:02 ` [PATCH 05/12] drm/i915/guc: Break out the GuC log extras into their own "runtime" struct Oscar Mateo
@ 2017-03-21 9:02 ` Oscar Mateo
2017-03-21 9:02 ` [PATCH 07/12] drm/i915/guc: Improve the GuC documentation & comments about proxy submissions Oscar Mateo
` (6 subsequent siblings)
12 siblings, 0 replies; 28+ messages in thread
From: Oscar Mateo @ 2017-03-21 9:02 UTC (permalink / raw)
To: intel-gfx
Prepare for an alternate GuC communication interface.
v2: Make a few functions static and name them correctly while we are at it (Oscar), but
leave an intel_guc_send_mmio interface for users that require old-style communication.
v3: Send intel_uc_init_early back to the top (Michal).
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/intel_uc.c | 16 +++++++++++-----
drivers/gpu/drm/i915/intel_uc.h | 9 ++++++++-
2 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 516e57d..ea99c89 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -83,7 +83,10 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
void intel_uc_init_early(struct drm_i915_private *dev_priv)
{
- mutex_init(&dev_priv->guc.send_mutex);
+ struct intel_guc *guc = &dev_priv->guc;
+
+ mutex_init(&guc->send_mutex);
+ guc->send = intel_guc_send_mmio;
}
void intel_uc_init_fw(struct drm_i915_private *dev_priv)
@@ -216,7 +219,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
* Read GuC command/status register (SOFT_SCRATCH_0)
* Return true if it contains a response rather than a command
*/
-static bool intel_guc_recv(struct intel_guc *guc, u32 *status)
+static bool guc_recv(struct intel_guc *guc, u32 *status)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -225,7 +228,10 @@ static bool intel_guc_recv(struct intel_guc *guc, u32 *status)
return INTEL_GUC_RECV_IS_RESPONSE(val);
}
-int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
+/*
+ * This function implements the MMIO based host to GuC interface.
+ */
+int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
u32 status;
@@ -253,9 +259,9 @@ int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
* up to that length of time, then switch to a slower sleep-wait loop.
* No inte_guc_send command should ever take longer than 10ms.
*/
- ret = wait_for_us(intel_guc_recv(guc, &status), 10);
+ ret = wait_for_us(guc_recv(guc, &status), 10);
if (ret)
- ret = wait_for(intel_guc_recv(guc, &status), 10);
+ ret = wait_for(guc_recv(guc, &status), 10);
if (status != INTEL_GUC_STATUS_SUCCESS) {
/*
* Either the GuC explicitly returned an error (which
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index c6f880c..3ec54a1 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -176,6 +176,9 @@ struct intel_guc {
/* To serialize the intel_guc_send actions */
struct mutex send_mutex;
+
+ /* GuC's FW specific send function */
+ int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
};
struct intel_huc {
@@ -194,8 +197,12 @@ struct intel_huc {
void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
void intel_uc_prepare_fw(struct drm_i915_private *dev_priv,
struct intel_uc_fw *uc_fw);
-int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
int intel_guc_sample_forcewake(struct intel_guc *guc);
+int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
+static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
+{
+ return guc->send(guc, action, len);
+}
/* intel_guc_loader.c */
int intel_guc_select_fw(struct intel_guc *guc);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 07/12] drm/i915/guc: Improve the GuC documentation & comments about proxy submissions
2017-03-21 9:02 [PATCH 00/12] Various improvements around the GuC topic Oscar Mateo
` (5 preceding siblings ...)
2017-03-21 9:02 ` [PATCH 06/12] drm/i915/guc: Make intel_guc_send a function pointer Oscar Mateo
@ 2017-03-21 9:02 ` Oscar Mateo
2017-03-21 17:54 ` Ceraolo Spurio, Daniele
2017-03-21 9:02 ` [PATCH 08/12] drm/i915/guc: Wait for doorbell to be inactive before deallocating Oscar Mateo
` (5 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Oscar Mateo @ 2017-03-21 9:02 UTC (permalink / raw)
To: intel-gfx
While at it, fix a typo (s/ring_lcra/ring_lrca) and improve the naming of one
firware interface field (s/ring_tail/submit_element_info, since it can contain
more than just the ring tail).
No change in functionality.
v2:
- Remove reference to "unique user" of the GuC (Daniele)
- Keep mention to renaming from "GuC context" to "client" (Daniele)
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 42 +++++++++++++++++++-----------
drivers/gpu/drm/i915/intel_guc_fwif.h | 4 +--
drivers/gpu/drm/i915/intel_uc.h | 6 +++--
3 files changed, 33 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index ff8235a1..855112d 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -30,16 +30,25 @@
/**
* DOC: GuC-based command submission
*
- * i915_guc_client:
- * We use the term client to avoid confusion with contexts. A i915_guc_client is
- * equivalent to GuC object guc_context_desc. This context descriptor is
- * allocated from a pool of 1024 entries. Kernel driver will allocate doorbell
- * and workqueue for it. Also the process descriptor (guc_process_desc), which
- * is mapped to client space. So the client can write Work Item then ring the
- * doorbell.
+ * GuC client:
+ * A i915_guc_client refers to a submission path through GuC. Currently, there
+ * is only one of these (the execbuf_client) and this one is charged with all
+ * submissions to the GuC. This struct is the owner of a doorbell, a process
+ * descriptor and a workqueue (all of them inside a single gem object that
+ * contains all required pages for these elements).
*
- * To simplify the implementation, we allocate one gem object that contains all
- * pages for doorbell, process descriptor and workqueue.
+ * GuC context descriptor:
+ * During initialization, the driver allocates a static pool of 1024 such
+ * descriptors, and shares them with the GuC.
+ * Currently, there exists a 1:1 mapping between a i915_guc_client and a
+ * guc_context_desc (via the client's context_index), so effectively only
+ * one guc_context_desc gets used. This context descriptor lets the GuC know
+ * about the doorbell, workqueue and process descriptor. Theoretically, it also
+ * lets the GuC know about our HW contexts (Context ID, etc...), but we actually
+ * employ a kind of submission where the GuC uses the LRCA sent via the work
+ * item instead (the single guc_context_desc associated to execbuf client
+ * contains information about the default kernel context only, but this is
+ * essentially unused). This is called a "proxy" submission.
*
* The Scratch registers:
* There are 16 MMIO-based registers start from 0xC180. The kernel driver writes
@@ -308,10 +317,17 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
if (!ce->state)
break; /* XXX: continue? */
+ /*
+ * XXX: When this is a GUC_CTX_DESC_ATTR_KERNEL client (proxy
+ * submission or, in other words, not using a direct submission
+ * model) the KMD's LRCA is not used for any work submission.
+ * Instead, the GuC uses the LRCA of the user mode context (see
+ * guc_wq_item_append below).
+ */
lrc->context_desc = lower_32_bits(ce->lrc_desc);
/* The state page is after PPHWSP */
- lrc->ring_lcra =
+ lrc->ring_lrca =
guc_ggtt_offset(ce->state) + LRC_STATE_PN * PAGE_SIZE;
lrc->context_id = (client->ctx_index << GUC_ELC_CTXID_OFFSET) |
(guc_engine_id << GUC_ELC_ENGINE_OFFSET);
@@ -341,10 +357,6 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
desc->wq_addr = gfx_addr + client->wq_offset;
desc->wq_size = client->wq_size;
- /*
- * XXX: Take LRCs from an existing context if this is not an
- * IsKMDCreatedContext client
- */
desc->desc_private = (uintptr_t)client;
}
@@ -468,7 +480,7 @@ static void guc_wq_item_append(struct i915_guc_client *client,
/* The GuC wants only the low-order word of the context descriptor */
wqi->context_desc = (u32)intel_lr_context_descriptor(rq->ctx, engine);
- wqi->ring_tail = tail << WQ_RING_TAIL_SHIFT;
+ wqi->submit_element_info = tail << WQ_RING_TAIL_SHIFT;
wqi->fence_id = rq->global_seqno;
}
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 3ae8cef..4edf40f 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -251,7 +251,7 @@ struct guc_doorbell_info {
struct guc_wq_item {
u32 header;
u32 context_desc;
- u32 ring_tail;
+ u32 submit_element_info;
u32 fence_id;
} __packed;
@@ -278,7 +278,7 @@ struct guc_execlist_context {
u32 context_desc;
u32 context_id;
u32 ring_status;
- u32 ring_lcra;
+ u32 ring_lrca;
u32 ring_begin;
u32 ring_end;
u32 ring_next_free_location;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 3ec54a1..65d0b59 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -34,7 +34,9 @@
/*
* This structure primarily describes the GEM object shared with the GuC.
- * The GEM object is held for the entire lifetime of our interaction with
+ * The specs sometimes refer to this object as a "GuC context", but we use
+ * the term "client" to avoid confusion with hardware contexts. This
+ * GEM object is held for the entire lifetime of our interaction with
* the GuC, being allocated before the GuC is loaded with its firmware.
* Because there's no way to update the address used by the GuC after
* initialisation, the shared object must stay pinned into the GGTT as
@@ -44,7 +46,7 @@
*
* The single GEM object described here is actually made up of several
* separate areas, as far as the GuC is concerned. The first page (kept
- * kmap'd) includes the "process decriptor" which holds sequence data for
+ * kmap'd) includes the "process descriptor" which holds sequence data for
* the doorbell, and one cacheline which actually *is* the doorbell; a
* write to this will "ring the doorbell" (i.e. send an interrupt to the
* GuC). The subsequent pages of the client object constitute the work
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 08/12] drm/i915/guc: Wait for doorbell to be inactive before deallocating
2017-03-21 9:02 [PATCH 00/12] Various improvements around the GuC topic Oscar Mateo
` (6 preceding siblings ...)
2017-03-21 9:02 ` [PATCH 07/12] drm/i915/guc: Improve the GuC documentation & comments about proxy submissions Oscar Mateo
@ 2017-03-21 9:02 ` Oscar Mateo
2017-03-21 18:20 ` Ceraolo Spurio, Daniele
2017-03-21 9:02 ` [PATCH 09/12] drm/i915/guc: A little bit more of doorbell sanitization Oscar Mateo
` (4 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Oscar Mateo @ 2017-03-21 9:02 UTC (permalink / raw)
To: intel-gfx
Doorbell release flow requires that we wait for GEN8_DRB_VALID bit to go
to zero after updating db_status before we call the GuC to release the
doorbell.
Kudos to Daniele for finding this out.
v2: WARN instead of DRM_ERROR (Joonas)
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 855112d..143ef26 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -206,12 +206,22 @@ static int __create_doorbell(struct i915_guc_client *client)
static int __destroy_doorbell(struct i915_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;
doorbell->cookie = 0;
+ /* Doorbell release flow requires that we wait for GEN8_DRB_VALID bit
+ * to go to zero after updating db_status before we call the GuC to
+ * release the doorbell */
+ 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->ctx_index);
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 08/12] drm/i915/guc: Wait for doorbell to be inactive before deallocating
2017-03-21 9:02 ` [PATCH 08/12] drm/i915/guc: Wait for doorbell to be inactive before deallocating Oscar Mateo
@ 2017-03-21 18:20 ` Ceraolo Spurio, Daniele
2017-03-21 11:24 ` Oscar Mateo
0 siblings, 1 reply; 28+ messages in thread
From: Ceraolo Spurio, Daniele @ 2017-03-21 18:20 UTC (permalink / raw)
To: Oscar Mateo, intel-gfx
On 3/21/2017 2:02 AM, Oscar Mateo wrote:
> Doorbell release flow requires that we wait for GEN8_DRB_VALID bit to go
> to zero after updating db_status before we call the GuC to release the
> doorbell.
>
> Kudos to Daniele for finding this out.
>
> v2: WARN instead of DRM_ERROR (Joonas)
>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
I'd like Joonas to give his ack on not returning after the wait since he
commented on it in the previous version. I personally prefer it this way
because we gain a bit more time for the doorbell to disable itself
(there is no spec in regards to how long this should take, but it should
be quick) while the GuC processes the H2G. If we ever see a case where
the warn fires but the H2G is successful it would mean that we have to
tune our wait time.
Thanks,
Daniele
> ---
> drivers/gpu/drm/i915/i915_guc_submission.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 855112d..143ef26 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -206,12 +206,22 @@ static int __create_doorbell(struct i915_guc_client *client)
>
> static int __destroy_doorbell(struct i915_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;
> doorbell->cookie = 0;
>
> + /* Doorbell release flow requires that we wait for GEN8_DRB_VALID bit
> + * to go to zero after updating db_status before we call the GuC to
> + * release the doorbell */
> + 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->ctx_index);
> }
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 08/12] drm/i915/guc: Wait for doorbell to be inactive before deallocating
2017-03-21 18:20 ` Ceraolo Spurio, Daniele
@ 2017-03-21 11:24 ` Oscar Mateo
2017-03-22 9:33 ` Joonas Lahtinen
0 siblings, 1 reply; 28+ messages in thread
From: Oscar Mateo @ 2017-03-21 11:24 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele, intel-gfx
On 03/21/2017 11:20 AM, Ceraolo Spurio, Daniele wrote:
>
>
> On 3/21/2017 2:02 AM, Oscar Mateo wrote:
>> Doorbell release flow requires that we wait for GEN8_DRB_VALID bit to go
>> to zero after updating db_status before we call the GuC to release the
>> doorbell.
>>
>> Kudos to Daniele for finding this out.
>>
>> v2: WARN instead of DRM_ERROR (Joonas)
>>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>
> I'd like Joonas to give his ack on not returning after the wait since
> he commented on it in the previous version. I personally prefer it
> this way because we gain a bit more time for the doorbell to disable
> itself (there is no spec in regards to how long this should take, but
> it should be quick) while the GuC processes the H2G. If we ever see a
> case where the warn fires but the H2G is successful it would mean that
> we have to tune our wait time.
>
> Thanks,
> Daniele
True. I went with his second option (WARN instead of DRM_ERROR) without
asking him first...
Thanks,
-- Oscar
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 08/12] drm/i915/guc: Wait for doorbell to be inactive before deallocating
2017-03-21 11:24 ` Oscar Mateo
@ 2017-03-22 9:33 ` Joonas Lahtinen
0 siblings, 0 replies; 28+ messages in thread
From: Joonas Lahtinen @ 2017-03-22 9:33 UTC (permalink / raw)
To: Oscar Mateo, Ceraolo Spurio, Daniele, intel-gfx
On ti, 2017-03-21 at 04:24 -0700, Oscar Mateo wrote:
>
> True. I went with his second option (WARN instead of DRM_ERROR) without
> asking him first...
Without knowing more of the innards of GuC, it's hard to say which will
be more appropriate action. WARN should be enough for now.
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 09/12] drm/i915/guc: A little bit more of doorbell sanitization
2017-03-21 9:02 [PATCH 00/12] Various improvements around the GuC topic Oscar Mateo
` (7 preceding siblings ...)
2017-03-21 9:02 ` [PATCH 08/12] drm/i915/guc: Wait for doorbell to be inactive before deallocating Oscar Mateo
@ 2017-03-21 9:02 ` Oscar Mateo
2017-03-21 9:02 ` [PATCH 10/12] drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor" Oscar Mateo
` (3 subsequent siblings)
12 siblings, 0 replies; 28+ messages in thread
From: Oscar Mateo @ 2017-03-21 9:02 UTC (permalink / raw)
To: intel-gfx
Some recent refactoring patches have left the doorbell creation outside
the GuC client allocation, which does not make a lot of sense (a client
without a doorbell is something useless). Move it back there, and
refactor the init_doorbell_hw consequently.
Thanks to this, we can do some other improvements, like hoisting the
check for GuC submission enabled out of the enable function.
v2: Rebased.
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 231 ++++++++++++++++-------------
drivers/gpu/drm/i915/intel_uc.c | 21 +--
2 files changed, 138 insertions(+), 114 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 143ef26..67084e5 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -162,15 +162,13 @@ static struct guc_context_desc *__get_context_desc(struct i915_guc_client *clien
* client object which contains the page being used for the doorbell
*/
-static int __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)
+static void __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)
{
struct guc_context_desc *desc;
/* Update the GuC's idea of the doorbell ID */
desc = __get_context_desc(client);
desc->db_id = new_id;
-
- return 0;
}
static struct guc_doorbell_info *__get_doorbell(struct i915_guc_client *client)
@@ -225,6 +223,28 @@ static int __destroy_doorbell(struct i915_guc_client *client)
return __guc_deallocate_doorbell(client->guc, client->ctx_index);
}
+static int create_doorbell(struct i915_guc_client *client)
+{
+ int ret;
+
+ ret = __reserve_doorbell(client);
+ if (ret)
+ return ret;
+
+ __update_doorbell_desc(client, client->doorbell_id);
+
+ ret = __create_doorbell(client);
+ if (ret)
+ goto err;
+
+ return 0;
+
+err:
+ __update_doorbell_desc(client, GUC_DOORBELL_INVALID);
+ __unreserve_doorbell(client);
+ return ret;
+}
+
static int destroy_doorbell(struct i915_guc_client *client)
{
int err;
@@ -494,6 +514,17 @@ static void guc_wq_item_append(struct i915_guc_client *client,
wqi->fence_id = rq->global_seqno;
}
+static void guc_reset_wq(struct i915_guc_client *client)
+{
+ struct guc_process_desc *desc = client->vaddr +
+ client->proc_desc_offset;
+
+ desc->head = 0;
+ desc->tail = 0;
+
+ client->wq_tail = 0;
+}
+
static int guc_ring_doorbell(struct i915_guc_client *client)
{
struct guc_process_desc *desc;
@@ -749,19 +780,6 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
return vma;
}
-static void guc_client_free(struct i915_guc_client *client)
-{
- /*
- * XXX: wait for any outstanding submissions before freeing memory.
- * Be sure to drop any locks
- */
- guc_ctx_desc_fini(client->guc, client);
- i915_gem_object_unpin_map(client->vma->obj);
- i915_vma_unpin_and_release(&client->vma);
- ida_simple_remove(&client->guc->ctx_ids, client->ctx_index);
- kfree(client);
-}
-
/* Check that a doorbell register is in the expected state */
static bool doorbell_ok(struct intel_guc *guc, u16 db_id)
{
@@ -792,9 +810,8 @@ static int __reset_doorbell(struct i915_guc_client* client, u16 db_id)
{
int err;
- err = __update_doorbell_desc(client, db_id);
- if (!err)
- err = __create_doorbell(client);
+ __update_doorbell_desc(client, db_id);
+ err = __create_doorbell(client);
if (!err)
err = __destroy_doorbell(client);
@@ -802,48 +819,61 @@ static int __reset_doorbell(struct i915_guc_client* client, u16 db_id)
}
/*
- * Borrow the first client to set up & tear down each unused doorbell
- * in turn, to ensure that all doorbell h/w is (re)initialised.
+ * 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 i915_guc_client *client = guc->execbuf_client;
- int err;
- int i;
-
- if (has_doorbell(client))
- destroy_doorbell(client);
+ bool recreate_first_client = false;
+ u16 db_id;
+ int ret;
- for (i = 0; i < GUC_NUM_DOORBELLS; ++i) {
- if (doorbell_ok(guc, i))
+ /* 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;
- err = __reset_doorbell(client, i);
- WARN(err, "Doorbell %d reset failed, err %d\n", i, err);
+ 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);
}
- /* Read back & verify all doorbell registers */
- for (i = 0; i < GUC_NUM_DOORBELLS; ++i)
- WARN_ON(!doorbell_ok(guc, i));
+ 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;
+ }
- err = __reserve_doorbell(client);
- if (err)
- return err;
+ __update_doorbell_desc(client, client->doorbell_id);
+ }
- err = __update_doorbell_desc(client, client->doorbell_id);
- if (err)
- goto err_reserve;
+ /* Now for every client (and not only execbuf_client) make sure their
+ * doorbells are known by the GuC */
+ //for (client = client_list; client != NULL; client = client->next)
+ {
+ ret = __create_doorbell(client);
+ if (ret) {
+ DRM_ERROR("Couldn't recreate client %u doorbell: %d\n",
+ client->ctx_index, ret);
+ return ret;
+ }
+ }
- err = __create_doorbell(client);
- if (err)
- goto err_update;
+ /* 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;
-err_reserve:
- __unreserve_doorbell(client);
-err_update:
- __update_doorbell_desc(client, GUC_DOORBELL_INVALID);
- return err;
}
/**
@@ -923,11 +953,9 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
guc_proc_desc_init(guc, client);
guc_ctx_desc_init(guc, client);
- /* FIXME: Runtime client allocation (which currently we don't do) will
- * require that the doorbell gets created now. The static execbuf_client
- * is now getting its doorbell later (on submission enable) but maybe we
- * also want to reorder things in the future so that we don't have to
- * special case the doorbell creation */
+ ret = create_doorbell(client);
+ if (ret)
+ goto err_vaddr;
DRM_DEBUG_DRIVER("new priority %u client %p for engine(s) 0x%x: ctx_index %u\n",
priority, client, client->engines, client->ctx_index);
@@ -935,6 +963,9 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
client->doorbell_id, client->doorbell_offset);
return client;
+
+err_vaddr:
+ i915_gem_object_unpin_map(client->vma->obj);
err_vma:
i915_vma_unpin_and_release(&client->vma);
err_id:
@@ -944,6 +975,24 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
return ERR_PTR(ret);
}
+static void guc_client_free(struct i915_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);
+ guc_ctx_desc_fini(client->guc, client);
+ i915_gem_object_unpin_map(client->vma->obj);
+ i915_vma_unpin_and_release(&client->vma);
+ ida_simple_remove(&client->guc->ctx_ids, client->ctx_index);
+ kfree(client);
+}
+
static void guc_policies_init(struct guc_policies *policies)
{
struct guc_policy *policy;
@@ -1035,8 +1084,8 @@ static void guc_ads_destroy(struct intel_guc *guc)
}
/*
- * Set up the memory resources to be shared with the GuC. At this point,
- * we require just one object that can be mapped through the GGTT.
+ * Set up the memory resources to be shared with the GuC (via the GGTT)
+ * at firmware loading time.
*/
int i915_guc_submission_init(struct drm_i915_private *dev_priv)
{
@@ -1048,16 +1097,6 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
void *vaddr;
int ret;
- if (!HAS_GUC_SCHED(dev_priv))
- return 0;
-
- /* Wipe bitmap & delete client in case of reinitialisation */
- bitmap_clear(guc->doorbell_bitmap, 0, GUC_NUM_DOORBELLS);
- i915_guc_submission_disable(dev_priv);
-
- if (!i915.enable_guc_submission)
- return 0;
-
if (guc->ctx_pool)
return 0;
@@ -1085,20 +1124,8 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
ida_init(&guc->ctx_ids);
- guc->execbuf_client = guc_client_alloc(dev_priv,
- INTEL_INFO(dev_priv)->ring_mask,
- GUC_CTX_PRIORITY_KMD_NORMAL,
- dev_priv->kernel_context);
- if (IS_ERR(guc->execbuf_client)) {
- DRM_ERROR("Failed to create GuC client for execbuf!\n");
- ret = PTR_ERR(guc->execbuf_client);
- goto err_ads;
- }
-
return 0;
-err_ads:
- guc_ads_destroy(guc);
err_log:
intel_guc_log_destroy(guc);
err_vaddr:
@@ -1112,11 +1139,6 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
{
struct intel_guc *guc = &dev_priv->guc;
- if (!i915.enable_guc_submission)
- return 0;
-
- guc_client_free(guc->execbuf_client);
- guc->execbuf_client = NULL;
ida_destroy(&guc->ctx_ids);
guc_ads_destroy(guc);
intel_guc_log_destroy(guc);
@@ -1124,17 +1146,6 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
i915_vma_unpin_and_release(&guc->ctx_pool);
}
-static void guc_reset_wq(struct i915_guc_client *client)
-{
- struct guc_process_desc *desc = client->vaddr +
- client->proc_desc_offset;
-
- desc->head = 0;
- desc->tail = 0;
-
- client->wq_tail = 0;
-}
-
static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
{
struct intel_engine_cs *engine;
@@ -1185,17 +1196,28 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
enum intel_engine_id id;
int err;
- if (!client)
- return -ENODEV;
+ if (!client) {
+ client = guc_client_alloc(dev_priv,
+ INTEL_INFO(dev_priv)->ring_mask,
+ GUC_CTX_PRIORITY_KMD_NORMAL,
+ dev_priv->kernel_context);
+ if (IS_ERR(client)) {
+ DRM_ERROR("Failed to create GuC client for execbuf!\n");
+ return PTR_ERR(client);
+ }
+
+ guc->execbuf_client = client;
+ }
err = intel_guc_sample_forcewake(guc);
if (err)
- return err;
+ goto err_execbuf_client;
guc_reset_wq(client);
+
err = guc_init_doorbell_hw(guc);
if (err)
- return err;
+ goto err_execbuf_client;
/* Take over from manual control of ELSP (execlists) */
guc_interrupts_capture(dev_priv);
@@ -1222,6 +1244,11 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
}
return 0;
+
+err_execbuf_client:
+ guc_client_free(guc->execbuf_client);
+ guc->execbuf_client = NULL;
+ return err;
}
static void guc_interrupts_release(struct drm_i915_private *dev_priv)
@@ -1254,16 +1281,11 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
guc_interrupts_release(dev_priv);
- if (!guc->execbuf_client)
- return;
-
- /* 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(guc->execbuf_client);
-
/* Revert back to manual ELSP submission */
intel_engines_reset_default_submission(dev_priv);
+
+ guc_client_free(guc->execbuf_client);
+ guc->execbuf_client = NULL;
}
/**
@@ -1292,7 +1314,6 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
return intel_guc_send(guc, data, ARRAY_SIZE(data));
}
-
/**
* intel_guc_resume() - notify GuC resuming from suspend state
* @dev_priv: i915 device private
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index ea99c89..db52594 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -126,13 +126,15 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
/* We need to notify the guc whenever we change the GGTT */
i915_ggtt_enable_guc(dev_priv);
- /*
- * This is stuff we need to have available at fw load time
- * if we are planning to enable submission later
- */
- ret = i915_guc_submission_init(dev_priv);
- if (ret)
- goto err_guc;
+ if (i915.enable_guc_submission) {
+ /*
+ * This is stuff we need to have available at fw load time
+ * if we are planning to enable submission later
+ */
+ ret = i915_guc_submission_init(dev_priv);
+ if (ret)
+ goto err_guc;
+ }
/* WaEnableuKernelHeaderValidFix:skl */
/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
@@ -187,7 +189,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
err_interrupts:
gen9_disable_guc_interrupts(dev_priv);
err_submission:
- i915_guc_submission_fini(dev_priv);
+ if (i915.enable_guc_submission)
+ i915_guc_submission_fini(dev_priv);
err_guc:
i915_ggtt_disable_guc(dev_priv);
@@ -210,8 +213,8 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
if (i915.enable_guc_submission) {
i915_guc_submission_disable(dev_priv);
gen9_disable_guc_interrupts(dev_priv);
+ i915_guc_submission_fini(dev_priv);
}
- i915_guc_submission_fini(dev_priv);
i915_ggtt_disable_guc(dev_priv);
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 10/12] drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor"
2017-03-21 9:02 [PATCH 00/12] Various improvements around the GuC topic Oscar Mateo
` (8 preceding siblings ...)
2017-03-21 9:02 ` [PATCH 09/12] drm/i915/guc: A little bit more of doorbell sanitization Oscar Mateo
@ 2017-03-21 9:02 ` Oscar Mateo
2017-03-22 10:05 ` Joonas Lahtinen
2017-03-21 9:02 ` [PATCH 11/12] drm/i915/guc: Split out the mmio_white_list struct Oscar Mateo
` (2 subsequent siblings)
12 siblings, 1 reply; 28+ messages in thread
From: Oscar Mateo @ 2017-03-21 9:02 UTC (permalink / raw)
To: intel-gfx
A GuC context and a HW context are in no way related, so the name "GuC context descriptor"
is very unfortunate, because a new reader of the code gets overwhelmed very quickly with
a lot of things called "context" that refer to different things. We can improve legibility
a lot by simply renaming a few objects in the GuC code.
v2:
- Rebased
- s/ctx_desc_pool/stage_desc_pool
- Move some explanations to the definition of the guc_stage_desc struct (Chris)
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
drivers/gpu/drm/i915/i915_guc_submission.c | 115 +++++++++++++++--------------
drivers/gpu/drm/i915/intel_guc_fwif.h | 48 +++++++-----
drivers/gpu/drm/i915/intel_guc_loader.c | 4 +-
drivers/gpu/drm/i915/intel_uc.h | 8 +-
5 files changed, 95 insertions(+), 84 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 061f8a5..29eefe2 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2474,8 +2474,8 @@ static void i915_guc_client_info(struct seq_file *m,
enum intel_engine_id id;
uint64_t tot = 0;
- seq_printf(m, "\tPriority %d, GuC ctx index: %u, PD offset 0x%x\n",
- client->priority, client->ctx_index, client->proc_desc_offset);
+ seq_printf(m, "\tPriority %d, GuC stage index: %u, PD offset 0x%x\n",
+ client->priority, client->stage_id, client->proc_desc_offset);
seq_printf(m, "\tDoorbell id %d, offset: 0x%lx, cookie 0x%x\n",
client->doorbell_id, client->doorbell_offset, client->doorbell_cookie);
seq_printf(m, "\tWQ size %d, offset: 0x%x, tail %d\n",
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 67084e5..5990939 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -37,16 +37,16 @@
* descriptor and a workqueue (all of them inside a single gem object that
* contains all required pages for these elements).
*
- * GuC context descriptor:
+ * GuC stage descriptor:
* During initialization, the driver allocates a static pool of 1024 such
* descriptors, and shares them with the GuC.
* Currently, there exists a 1:1 mapping between a i915_guc_client and a
- * guc_context_desc (via the client's context_index), so effectively only
- * one guc_context_desc gets used. This context descriptor lets the GuC know
- * about the doorbell, workqueue and process descriptor. Theoretically, it also
- * lets the GuC know about our HW contexts (Context ID, etc...), but we actually
+ * guc_stage_desc (via the client's stage_id), so effectively only one
+ * gets used. This stage descriptor lets the GuC know about the doorbell,
+ * workqueue and process descriptor. Theoretically, it also lets the GuC
+ * know about our HW contexts (context ID, etc...), but we actually
* employ a kind of submission where the GuC uses the LRCA sent via the work
- * item instead (the single guc_context_desc associated to execbuf client
+ * item instead (the single guc_stage_desc associated to execbuf client
* contains information about the default kernel context only, but this is
* essentially unused). This is called a "proxy" submission.
*
@@ -82,7 +82,7 @@
static inline bool is_high_priority(struct i915_guc_client* client)
{
- return client->priority <= GUC_CTX_PRIORITY_HIGH;
+ return client->priority <= GUC_CLIENT_PRIORITY_HIGH;
}
static int __reserve_doorbell(struct i915_guc_client *client)
@@ -112,7 +112,7 @@ static int __reserve_doorbell(struct i915_guc_client *client)
__set_bit(id, client->guc->doorbell_bitmap);
client->doorbell_id = id;
DRM_DEBUG_DRIVER("client %u (high prio=%s) reserved doorbell: %d\n",
- client->ctx_index, yesno(is_high_priority(client)),
+ client->stage_id, yesno(is_high_priority(client)),
id);
return 0;
}
@@ -129,30 +129,30 @@ static void __unreserve_doorbell(struct i915_guc_client *client)
* Tell the GuC to allocate or deallocate a specific doorbell
*/
-static int __guc_allocate_doorbell(struct intel_guc *guc, u32 ctx_index)
+static int __guc_allocate_doorbell(struct intel_guc *guc, u32 stage_id)
{
u32 action[] = {
INTEL_GUC_ACTION_ALLOCATE_DOORBELL,
- ctx_index
+ stage_id
};
return intel_guc_send(guc, action, ARRAY_SIZE(action));
}
-static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
+static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 stage_id)
{
u32 action[] = {
INTEL_GUC_ACTION_DEALLOCATE_DOORBELL,
- ctx_index
+ stage_id
};
return intel_guc_send(guc, action, ARRAY_SIZE(action));
}
-static struct guc_context_desc *__get_context_desc(struct i915_guc_client *client)
+static struct guc_stage_desc *__get_stage_desc(struct i915_guc_client *client)
{
- return (struct guc_context_desc *)((char *)client->guc->ctx_pool_vaddr +
- sizeof(struct guc_context_desc) * client->ctx_index);
+ return (struct guc_stage_desc *)((char *)client->guc->stage_desc_pool_vaddr +
+ sizeof(struct guc_stage_desc) * client->stage_id);
}
/*
@@ -164,10 +164,10 @@ static struct guc_context_desc *__get_context_desc(struct i915_guc_client *clien
static void __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)
{
- struct guc_context_desc *desc;
+ struct guc_stage_desc *desc;
/* Update the GuC's idea of the doorbell ID */
- desc = __get_context_desc(client);
+ desc = __get_stage_desc(client);
desc->db_id = new_id;
}
@@ -194,7 +194,7 @@ static int __create_doorbell(struct i915_guc_client *client)
doorbell->db_status = GUC_DOORBELL_ENABLED;
doorbell->cookie = client->doorbell_cookie;
- err = __guc_allocate_doorbell(client->guc, client->ctx_index);
+ err = __guc_allocate_doorbell(client->guc, client->stage_id);
if (err) {
doorbell->db_status = GUC_DOORBELL_DISABLED;
doorbell->cookie = 0;
@@ -220,7 +220,7 @@ static int __destroy_doorbell(struct i915_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->ctx_index);
+ return __guc_deallocate_doorbell(client->guc, client->stage_id);
}
static int create_doorbell(struct i915_guc_client *client)
@@ -301,34 +301,34 @@ static void guc_proc_desc_init(struct intel_guc *guc,
desc->wq_base_addr = 0;
desc->db_base_addr = 0;
- desc->context_id = client->ctx_index;
+ desc->stage_id = client->stage_id;
desc->wq_size_bytes = client->wq_size;
desc->wq_status = WQ_STATUS_ACTIVE;
desc->priority = client->priority;
}
/*
- * Initialise/clear the context descriptor shared with the GuC firmware.
+ * Initialise/clear the stage descriptor shared with the GuC firmware.
*
* This descriptor tells the GuC where (in GGTT space) to find the important
* data structures relating to this client (doorbell, process descriptor,
* write queue, etc).
*/
-static void guc_ctx_desc_init(struct intel_guc *guc,
- struct i915_guc_client *client)
+static void guc_stage_desc_init(struct intel_guc *guc,
+ struct i915_guc_client *client)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
struct intel_engine_cs *engine;
struct i915_gem_context *ctx = client->owner;
- struct guc_context_desc *desc;
+ struct guc_stage_desc *desc;
unsigned int tmp;
u32 gfx_addr;
- desc = __get_context_desc(client);
+ desc = __get_stage_desc(client);
memset(desc, 0, sizeof(*desc));
- desc->attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
- desc->context_id = client->ctx_index;
+ desc->attribute = GUC_STAGE_DESC_ATTR_ACTIVE | GUC_STAGE_DESC_ATTR_KERNEL;
+ desc->stage_id = client->stage_id;
desc->priority = client->priority;
desc->db_id = client->doorbell_id;
@@ -348,7 +348,7 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
break; /* XXX: continue? */
/*
- * XXX: When this is a GUC_CTX_DESC_ATTR_KERNEL client (proxy
+ * XXX: When this is a GUC_STAGE_DESC_ATTR_KERNEL client (proxy
* submission or, in other words, not using a direct submission
* model) the KMD's LRCA is not used for any work submission.
* Instead, the GuC uses the LRCA of the user mode context (see
@@ -359,7 +359,10 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
/* The state page is after PPHWSP */
lrc->ring_lrca =
guc_ggtt_offset(ce->state) + LRC_STATE_PN * PAGE_SIZE;
- lrc->context_id = (client->ctx_index << GUC_ELC_CTXID_OFFSET) |
+
+ /* XXX: In direct submission, the GuC wants the HW context id
+ * here. In proxy submission, it wants the stage id */
+ lrc->context_id = (client->stage_id << GUC_ELC_CTXID_OFFSET) |
(guc_engine_id << GUC_ELC_ENGINE_OFFSET);
lrc->ring_begin = guc_ggtt_offset(ce->ring->vma);
@@ -390,12 +393,12 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
desc->desc_private = (uintptr_t)client;
}
-static void guc_ctx_desc_fini(struct intel_guc *guc,
- struct i915_guc_client *client)
+static void guc_stage_desc_fini(struct intel_guc *guc,
+ struct i915_guc_client *client)
{
- struct guc_context_desc *desc;
+ struct guc_stage_desc *desc;
- desc = __get_context_desc(client);
+ desc = __get_stage_desc(client);
memset(desc, 0, sizeof(*desc));
}
@@ -864,7 +867,7 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
ret = __create_doorbell(client);
if (ret) {
DRM_ERROR("Couldn't recreate client %u doorbell: %d\n",
- client->ctx_index, ret);
+ client->stage_id, ret);
return ret;
}
}
@@ -914,12 +917,12 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
client->wq_size = GUC_WQ_SIZE;
spin_lock_init(&client->wq_lock);
- ret = ida_simple_get(&guc->ctx_ids, 0, GUC_MAX_GPU_CONTEXTS,
+ ret = ida_simple_get(&guc->stage_ids, 0, GUC_MAX_STAGE_DESCRIPTORS,
GFP_KERNEL);
if (ret < 0)
goto err_client;
- client->ctx_index = ret;
+ client->stage_id = ret;
/* The first page is doorbell/proc_desc. Two followed pages are wq. */
vma = intel_guc_allocate_vma(guc, GUC_DB_SIZE + GUC_WQ_SIZE);
@@ -951,14 +954,14 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
client->proc_desc_offset = (GUC_DB_SIZE / 2);
guc_proc_desc_init(guc, client);
- guc_ctx_desc_init(guc, client);
+ guc_stage_desc_init(guc, client);
ret = create_doorbell(client);
if (ret)
goto err_vaddr;
- 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("new priority %u client %p for engine(s) 0x%x: stage_id %u\n",
+ priority, client, client->engines, client->stage_id);
DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%lx\n",
client->doorbell_id, client->doorbell_offset);
@@ -969,7 +972,7 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
err_vma:
i915_vma_unpin_and_release(&client->vma);
err_id:
- ida_simple_remove(&guc->ctx_ids, client->ctx_index);
+ ida_simple_remove(&guc->stage_ids, client->stage_id);
err_client:
kfree(client);
return ERR_PTR(ret);
@@ -986,10 +989,10 @@ static void guc_client_free(struct i915_guc_client *client)
* reset, so we cannot destroy the doorbell properly. Ignore the
* error message for now */
destroy_doorbell(client);
- guc_ctx_desc_fini(client->guc, client);
+ guc_stage_desc_fini(client->guc, client);
i915_gem_object_unpin_map(client->vma->obj);
i915_vma_unpin_and_release(&client->vma);
- ida_simple_remove(&client->guc->ctx_ids, client->ctx_index);
+ ida_simple_remove(&client->guc->stage_ids, client->stage_id);
kfree(client);
}
@@ -1001,7 +1004,7 @@ static void guc_policies_init(struct guc_policies *policies)
policies->dpc_promote_time = 500000;
policies->max_num_work_items = POLICY_MAX_NUM_WI;
- for (p = 0; p < GUC_CTX_PRIORITY_NUM; p++) {
+ for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
policy = &policies->policy[p][i];
@@ -1089,30 +1092,30 @@ static void guc_ads_destroy(struct intel_guc *guc)
*/
int i915_guc_submission_init(struct drm_i915_private *dev_priv)
{
- const size_t ctxsize = sizeof(struct guc_context_desc);
- const size_t poolsize = GUC_MAX_GPU_CONTEXTS * ctxsize;
+ const size_t ctxsize = sizeof(struct guc_stage_desc);
+ const size_t poolsize = GUC_MAX_STAGE_DESCRIPTORS * ctxsize;
const size_t gemsize = round_up(poolsize, PAGE_SIZE);
struct intel_guc *guc = &dev_priv->guc;
struct i915_vma *vma;
void *vaddr;
int ret;
- if (guc->ctx_pool)
+ if (guc->stage_desc_pool)
return 0;
vma = intel_guc_allocate_vma(guc, gemsize);
if (IS_ERR(vma))
return PTR_ERR(vma);
- guc->ctx_pool = vma;
+ guc->stage_desc_pool = vma;
- vaddr = i915_gem_object_pin_map(guc->ctx_pool->obj, I915_MAP_WB);
+ vaddr = i915_gem_object_pin_map(guc->stage_desc_pool->obj, I915_MAP_WB);
if (IS_ERR(vaddr)) {
ret = PTR_ERR(vaddr);
goto err_vma;
}
- guc->ctx_pool_vaddr = vaddr;
+ guc->stage_desc_pool_vaddr = vaddr;
ret = intel_guc_log_create(guc);
if (ret < 0)
@@ -1122,16 +1125,16 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
if (ret < 0)
goto err_log;
- ida_init(&guc->ctx_ids);
+ ida_init(&guc->stage_ids);
return 0;
err_log:
intel_guc_log_destroy(guc);
err_vaddr:
- i915_gem_object_unpin_map(guc->ctx_pool->obj);
+ i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
err_vma:
- i915_vma_unpin_and_release(&guc->ctx_pool);
+ i915_vma_unpin_and_release(&guc->stage_desc_pool);
return ret;
}
@@ -1139,11 +1142,11 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
{
struct intel_guc *guc = &dev_priv->guc;
- ida_destroy(&guc->ctx_ids);
+ ida_destroy(&guc->stage_ids);
guc_ads_destroy(guc);
intel_guc_log_destroy(guc);
- i915_gem_object_unpin_map(guc->ctx_pool->obj);
- i915_vma_unpin_and_release(&guc->ctx_pool);
+ i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
+ i915_vma_unpin_and_release(&guc->stage_desc_pool);
}
static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
@@ -1199,7 +1202,7 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
if (!client) {
client = guc_client_alloc(dev_priv,
INTEL_INFO(dev_priv)->ring_mask,
- GUC_CTX_PRIORITY_KMD_NORMAL,
+ GUC_CLIENT_PRIORITY_KMD_NORMAL,
dev_priv->kernel_context);
if (IS_ERR(client)) {
DRM_ERROR("Failed to create GuC client for execbuf!\n");
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 4edf40f..ec2420d 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -26,14 +26,14 @@
#define GFXCORE_FAMILY_GEN9 12
#define GFXCORE_FAMILY_UNKNOWN 0x7fffffff
-#define GUC_CTX_PRIORITY_KMD_HIGH 0
-#define GUC_CTX_PRIORITY_HIGH 1
-#define GUC_CTX_PRIORITY_KMD_NORMAL 2
-#define GUC_CTX_PRIORITY_NORMAL 3
-#define GUC_CTX_PRIORITY_NUM 4
+#define GUC_CLIENT_PRIORITY_KMD_HIGH 0
+#define GUC_CLIENT_PRIORITY_HIGH 1
+#define GUC_CLIENT_PRIORITY_KMD_NORMAL 2
+#define GUC_CLIENT_PRIORITY_NORMAL 3
+#define GUC_CLIENT_PRIORITY_NUM 4
-#define GUC_MAX_GPU_CONTEXTS 1024
-#define GUC_INVALID_CTX_ID GUC_MAX_GPU_CONTEXTS
+#define GUC_MAX_STAGE_DESCRIPTORS 1024
+#define GUC_INVALID_STAGE_ID GUC_MAX_STAGE_DESCRIPTORS
#define GUC_RENDER_ENGINE 0
#define GUC_VIDEO_ENGINE 1
@@ -68,14 +68,14 @@
#define GUC_DOORBELL_ENABLED 1
#define GUC_DOORBELL_DISABLED 0
-#define GUC_CTX_DESC_ATTR_ACTIVE (1 << 0)
-#define GUC_CTX_DESC_ATTR_PENDING_DB (1 << 1)
-#define GUC_CTX_DESC_ATTR_KERNEL (1 << 2)
-#define GUC_CTX_DESC_ATTR_PREEMPT (1 << 3)
-#define GUC_CTX_DESC_ATTR_RESET (1 << 4)
-#define GUC_CTX_DESC_ATTR_WQLOCKED (1 << 5)
-#define GUC_CTX_DESC_ATTR_PCH (1 << 6)
-#define GUC_CTX_DESC_ATTR_TERMINATED (1 << 7)
+#define GUC_STAGE_DESC_ATTR_ACTIVE (1 << 0)
+#define GUC_STAGE_DESC_ATTR_PENDING_DB (1 << 1)
+#define GUC_STAGE_DESC_ATTR_KERNEL (1 << 2)
+#define GUC_STAGE_DESC_ATTR_PREEMPT (1 << 3)
+#define GUC_STAGE_DESC_ATTR_RESET (1 << 4)
+#define GUC_STAGE_DESC_ATTR_WQLOCKED (1 << 5)
+#define GUC_STAGE_DESC_ATTR_PCH (1 << 6)
+#define GUC_STAGE_DESC_ATTR_TERMINATED (1 << 7)
/* The guc control data is 10 DWORDs */
#define GUC_CTL_CTXINFO 0
@@ -256,7 +256,7 @@ struct guc_wq_item {
} __packed;
struct guc_process_desc {
- u32 context_id;
+ u32 stage_id;
u64 db_base_addr;
u32 head;
u32 tail;
@@ -289,10 +289,18 @@ struct guc_execlist_context {
u16 engine_submit_queue_count;
} __packed;
-/*Context descriptor for communicating between uKernel and Driver*/
-struct guc_context_desc {
+/*
+ * This structure describes a stage set arranged for a particular communication
+ * between uKernel (GuC) and Driver (KMD). Technically, this is known as a
+ * "GuC Context descriptor" in the specs, but we use the term "stage descriptor"
+ * to avoid confusion with all the other things already named "context" in the
+ * driver. A static pool of these descriptors are stored inside a GEM object
+ * (stage_desc_pool) which is held for the entire lifetime of our interaction
+ * with the GuC, being allocated before the GuC is loaded with its firmware.
+ */
+struct guc_stage_desc {
u32 sched_common_area;
- u32 context_id;
+ u32 stage_id;
u32 pas_id;
u8 engines_used;
u64 db_trigger_cpu;
@@ -359,7 +367,7 @@ struct guc_policy {
} __packed;
struct guc_policies {
- struct guc_policy policy[GUC_CTX_PRIORITY_NUM][GUC_MAX_ENGINES_NUM];
+ struct guc_policy policy[GUC_CLIENT_PRIORITY_NUM][GUC_MAX_ENGINES_NUM];
/* In micro seconds. How much time to allow before DPC processing is
* called back via interrupt (to prevent DPC queue drain starving).
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 62ebf56..7d92321 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -151,8 +151,8 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
/* If GuC submission is enabled, set up additional parameters here */
if (i915.enable_guc_submission) {
u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
- u32 pgs = guc_ggtt_offset(dev_priv->guc.ctx_pool);
- u32 ctx_in_16 = GUC_MAX_GPU_CONTEXTS / 16;
+ u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
+ u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 65d0b59..6cf2d14 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -74,7 +74,7 @@ struct i915_guc_client {
uint32_t engines; /* bitmap of (host) engine ids */
uint32_t priority;
- u32 ctx_index;
+ u32 stage_id;
uint32_t proc_desc_offset;
u16 doorbell_id;
@@ -157,9 +157,9 @@ struct intel_guc {
bool interrupts_enabled;
struct i915_vma *ads_vma;
- struct i915_vma *ctx_pool;
- void *ctx_pool_vaddr;
- struct ida ctx_ids;
+ struct i915_vma *stage_desc_pool;
+ void *stage_desc_pool_vaddr;
+ struct ida stage_ids;
struct i915_guc_client *execbuf_client;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 10/12] drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor"
2017-03-21 9:02 ` [PATCH 10/12] drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor" Oscar Mateo
@ 2017-03-22 10:05 ` Joonas Lahtinen
0 siblings, 0 replies; 28+ messages in thread
From: Joonas Lahtinen @ 2017-03-22 10:05 UTC (permalink / raw)
To: Oscar Mateo, intel-gfx
On ti, 2017-03-21 at 02:02 -0700, Oscar Mateo wrote:
> A GuC context and a HW context are in no way related, so the name "GuC context descriptor"
> is very unfortunate, because a new reader of the code gets overwhelmed very quickly with
> a lot of things called "context" that refer to different things. We can improve legibility
> a lot by simply renaming a few objects in the GuC code.
>
> v2:
> - Rebased
> - s/ctx_desc_pool/stage_desc_pool
> - Move some explanations to the definition of the guc_stage_desc struct (Chris)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
<SNIP>
> -static struct guc_context_desc *__get_context_desc(struct i915_guc_client *client)
> +static struct guc_stage_desc *__get_stage_desc(struct i915_guc_client *client)
> {
> - return (struct guc_context_desc *)((char *)client->guc->ctx_pool_vaddr +
> - sizeof(struct guc_context_desc) * client->ctx_index);
> + return (struct guc_stage_desc *)((char *)client->guc->stage_desc_pool_vaddr +
> + sizeof(struct guc_stage_desc) * client->stage_id);
Am I missing something or isn't this just a hard way of doing;
struct guc_stage_desc *stage_descs = client->guc->stage_desc_pool_vaddr;
return &stage_descs[client->stage_id];
(+/- fixing sparse warnings, if any)
> }
>
<SNIP>
> @@ -1089,30 +1092,30 @@ static void guc_ads_destroy(struct intel_guc *guc)
> */
> int i915_guc_submission_init(struct drm_i915_private *dev_priv)
> {
> - const size_t ctxsize = sizeof(struct guc_context_desc);
> - const size_t poolsize = GUC_MAX_GPU_CONTEXTS * ctxsize;
> + const size_t ctxsize = sizeof(struct guc_stage_desc);
I was about to comment to rename ctxsize -> stagesize, but;
> + const size_t poolsize = GUC_MAX_STAGE_DESCRIPTORS * ctxsize;
> const size_t gemsize = round_up(poolsize, PAGE_SIZE);
The above are only used once, so instead:
> struct intel_guc *guc = &dev_priv->guc;
> struct i915_vma *vma;
> void *vaddr;
> int ret;
>
> - if (guc->ctx_pool)
> + if (guc->stage_desc_pool)
> return 0;
>
> vma = intel_guc_allocate_vma(guc, gemsize);
Do
vma = intel_guc_allocate_vma(guc,
PAGE_ALIGN(sizeof(struct guc_stage_desc) *
GUC_MAX_GPU_CONTEXTS));
> if (IS_ERR(vma))
> return PTR_ERR(vma);
<SNIP>
> -#define GUC_CTX_DESC_ATTR_ACTIVE (1 << 0)
> -#define GUC_CTX_DESC_ATTR_PENDING_DB (1 << 1)
> -#define GUC_CTX_DESC_ATTR_KERNEL (1 << 2)
> -#define GUC_CTX_DESC_ATTR_PREEMPT (1 << 3)
> -#define GUC_CTX_DESC_ATTR_RESET (1 << 4)
> -#define GUC_CTX_DESC_ATTR_WQLOCKED (1 << 5)
> -#define GUC_CTX_DESC_ATTR_PCH (1 << 6)
> -#define GUC_CTX_DESC_ATTR_TERMINATED (1 << 7)
> +#define GUC_STAGE_DESC_ATTR_ACTIVE (1 << 0)
> +#define GUC_STAGE_DESC_ATTR_PENDING_DB (1 << 1)
> +#define GUC_STAGE_DESC_ATTR_KERNEL (1 << 2)
> +#define GUC_STAGE_DESC_ATTR_PREEMPT (1 << 3)
> +#define GUC_STAGE_DESC_ATTR_RESET (1 << 4)
> +#define GUC_STAGE_DESC_ATTR_WQLOCKED (1 << 5)
> +#define GUC_STAGE_DESC_ATTR_PCH (1 << 6)
> +#define GUC_STAGE_DESC_ATTR_TERMINATED (1 << 7)
While here, make 'em BIT()
With above, this is;
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 11/12] drm/i915/guc: Split out the mmio_white_list struct
2017-03-21 9:02 [PATCH 00/12] Various improvements around the GuC topic Oscar Mateo
` (9 preceding siblings ...)
2017-03-21 9:02 ` [PATCH 10/12] drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor" Oscar Mateo
@ 2017-03-21 9:02 ` Oscar Mateo
2017-03-21 18:04 ` Ceraolo Spurio, Daniele
2017-03-21 9:02 ` [PATCH 12/12] drm/i915/guc: Move guc_interrupts_release next to guc_interrupts_capture Oscar Mateo
2017-03-21 17:42 ` ✗ Fi.CI.BAT: warning for Various improvements around the GuC topic Patchwork
12 siblings, 1 reply; 28+ messages in thread
From: Oscar Mateo @ 2017-03-21 9:02 UTC (permalink / raw)
To: intel-gfx
We are going to need it for future platforms.
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 4 ++--
drivers/gpu/drm/i915/intel_guc_fwif.h | 15 ++++++++-------
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 5990939..95825f2 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1050,11 +1050,11 @@ static int guc_ads_create(struct intel_guc *guc)
/* MMIO reg state */
for_each_engine(engine, dev_priv, id) {
- blob->reg_state.mmio_white_list[engine->guc_id].mmio_start =
+ blob->reg_state.white_list[engine->guc_id].mmio_start =
engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
/* Nothing to be saved or restored for now. */
- blob->reg_state.mmio_white_list[engine->guc_id].count = 0;
+ blob->reg_state.white_list[engine->guc_id].count = 0;
}
/*
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index ec2420d..462e022 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -409,16 +409,17 @@ struct guc_mmio_regset {
u32 number_of_registers;
} __packed;
+/* MMIO registers that are set as non privileged */
+struct mmio_white_list {
+ u32 mmio_start;
+ u32 offsets[GUC_MMIO_WHITE_LIST_MAX];
+ u32 count;
+} __packed;
+
struct guc_mmio_reg_state {
struct guc_mmio_regset global_reg;
struct guc_mmio_regset engine_reg[GUC_MAX_ENGINES_NUM];
-
- /* MMIO registers that are set as non privileged */
- struct __packed {
- u32 mmio_start;
- u32 offsets[GUC_MMIO_WHITE_LIST_MAX];
- u32 count;
- } mmio_white_list[GUC_MAX_ENGINES_NUM];
+ struct mmio_white_list white_list[GUC_MAX_ENGINES_NUM];
} __packed;
/* GuC Additional Data Struct */
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 12/12] drm/i915/guc: Move guc_interrupts_release next to guc_interrupts_capture
2017-03-21 9:02 [PATCH 00/12] Various improvements around the GuC topic Oscar Mateo
` (10 preceding siblings ...)
2017-03-21 9:02 ` [PATCH 11/12] drm/i915/guc: Split out the mmio_white_list struct Oscar Mateo
@ 2017-03-21 9:02 ` Oscar Mateo
2017-03-21 18:08 ` Ceraolo Spurio, Daniele
2017-03-21 17:42 ` ✗ Fi.CI.BAT: warning for Various improvements around the GuC topic Patchwork
12 siblings, 1 reply; 28+ messages in thread
From: Oscar Mateo @ 2017-03-21 9:02 UTC (permalink / raw)
To: intel-gfx
They go better together.
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 48 +++++++++++++++---------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 95825f2..15a7495 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1191,6 +1191,30 @@ static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
dev_priv->rps.pm_intrmsk_mbz &= ~GEN8_PMINTR_DISABLE_REDIRECT_TO_GUC;
}
+static void guc_interrupts_release(struct drm_i915_private *dev_priv)
+{
+ struct intel_engine_cs *engine;
+ enum intel_engine_id id;
+ int irqs;
+
+ /*
+ * tell all command streamers NOT to forward interrupts or vblank
+ * to GuC.
+ */
+ irqs = _MASKED_FIELD(GFX_FORWARD_VBLANK_MASK, GFX_FORWARD_VBLANK_NEVER);
+ irqs |= _MASKED_BIT_DISABLE(GFX_INTERRUPT_STEERING);
+ for_each_engine(engine, dev_priv, id)
+ I915_WRITE(RING_MODE_GEN7(engine), irqs);
+
+ /* route all GT interrupts to the host */
+ I915_WRITE(GUC_BCS_RCS_IER, 0);
+ I915_WRITE(GUC_VCS2_VCS1_IER, 0);
+ I915_WRITE(GUC_WD_VECS_IER, 0);
+
+ dev_priv->rps.pm_intrmsk_mbz |= GEN8_PMINTR_DISABLE_REDIRECT_TO_GUC;
+ dev_priv->rps.pm_intrmsk_mbz &= ~ARAT_EXPIRED_INTRMSK;
+}
+
int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
{
struct intel_guc *guc = &dev_priv->guc;
@@ -1254,30 +1278,6 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
return err;
}
-static void guc_interrupts_release(struct drm_i915_private *dev_priv)
-{
- struct intel_engine_cs *engine;
- enum intel_engine_id id;
- int irqs;
-
- /*
- * tell all command streamers NOT to forward interrupts or vblank
- * to GuC.
- */
- irqs = _MASKED_FIELD(GFX_FORWARD_VBLANK_MASK, GFX_FORWARD_VBLANK_NEVER);
- irqs |= _MASKED_BIT_DISABLE(GFX_INTERRUPT_STEERING);
- for_each_engine(engine, dev_priv, id)
- I915_WRITE(RING_MODE_GEN7(engine), irqs);
-
- /* route all GT interrupts to the host */
- I915_WRITE(GUC_BCS_RCS_IER, 0);
- I915_WRITE(GUC_VCS2_VCS1_IER, 0);
- I915_WRITE(GUC_WD_VECS_IER, 0);
-
- dev_priv->rps.pm_intrmsk_mbz |= GEN8_PMINTR_DISABLE_REDIRECT_TO_GUC;
- dev_priv->rps.pm_intrmsk_mbz &= ~ARAT_EXPIRED_INTRMSK;
-}
-
void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
{
struct intel_guc *guc = &dev_priv->guc;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 28+ messages in thread* ✗ Fi.CI.BAT: warning for Various improvements around the GuC topic
2017-03-21 9:02 [PATCH 00/12] Various improvements around the GuC topic Oscar Mateo
` (11 preceding siblings ...)
2017-03-21 9:02 ` [PATCH 12/12] drm/i915/guc: Move guc_interrupts_release next to guc_interrupts_capture Oscar Mateo
@ 2017-03-21 17:42 ` Patchwork
2017-03-22 10:08 ` Joonas Lahtinen
12 siblings, 1 reply; 28+ messages in thread
From: Patchwork @ 2017-03-21 17:42 UTC (permalink / raw)
To: Oscar Mateo; +Cc: intel-gfx
== Series Details ==
Series: Various improvements around the GuC topic
URL : https://patchwork.freedesktop.org/series/21624/
State : warning
== Summary ==
Series 21624v1 Various improvements around the GuC topic
https://patchwork.freedesktop.org/api/1.0/series/21624/revisions/1/mbox/
Test gem_exec_flush:
Subgroup basic-batch-kernel-default-uc:
fail -> PASS (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
Subgroup basic-s4-devices:
dmesg-warn -> PASS (fi-bxt-t5700) fdo#100125
Test pm_rpm:
Subgroup basic-pci-d3-state:
pass -> DMESG-WARN (fi-bxt-j4205)
fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fi-bdw-5557u total:278 pass:263 dwarn:0 dfail:0 fail:4 skip:11 time: 453s
fi-bsw-n3050 total:278 pass:235 dwarn:0 dfail:0 fail:4 skip:39 time: 554s
fi-bxt-j4205 total:278 pass:254 dwarn:1 dfail:0 fail:4 skip:19 time: 522s
fi-bxt-t5700 total:278 pass:254 dwarn:0 dfail:0 fail:4 skip:20 time: 558s
fi-byt-j1900 total:278 pass:247 dwarn:0 dfail:0 fail:4 skip:27 time: 494s
fi-byt-n2820 total:278 pass:243 dwarn:0 dfail:0 fail:4 skip:31 time: 485s
fi-hsw-4770 total:278 pass:258 dwarn:0 dfail:0 fail:4 skip:16 time: 432s
fi-hsw-4770r total:278 pass:258 dwarn:0 dfail:0 fail:4 skip:16 time: 430s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:4 skip:46 time: 437s
fi-ivb-3520m total:278 pass:256 dwarn:0 dfail:0 fail:4 skip:18 time: 516s
fi-ivb-3770 total:278 pass:256 dwarn:0 dfail:0 fail:4 skip:18 time: 489s
fi-kbl-7500u total:278 pass:256 dwarn:0 dfail:0 fail:4 skip:18 time: 481s
fi-skl-6260u total:278 pass:264 dwarn:0 dfail:0 fail:4 skip:10 time: 469s
fi-skl-6700hq total:278 pass:257 dwarn:0 dfail:0 fail:4 skip:17 time: 590s
fi-skl-6700k total:278 pass:252 dwarn:4 dfail:0 fail:4 skip:18 time: 494s
fi-skl-6770hq total:278 pass:264 dwarn:0 dfail:0 fail:4 skip:10 time: 513s
fi-snb-2520m total:278 pass:246 dwarn:0 dfail:0 fail:4 skip:28 time: 542s
fi-snb-2600 total:278 pass:245 dwarn:0 dfail:0 fail:4 skip:29 time: 412s
7035813558b7d9601c8fdda50498cf9f8d7f0f2b drm-tip: 2017y-03m-21d-15h-58m-46s UTC integration manifest
1b4ed14 drm/i915/guc: Move guc_interrupts_release next to guc_interrupts_capture
b952000 drm/i915/guc: Split out the mmio_white_list struct
b8b653e drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor"
8ffda10 drm/i915/guc: A little bit more of doorbell sanitization
8f22f65 drm/i915/guc: Wait for doorbell to be inactive before deallocating
1362e1f drm/i915/guc: Improve the GuC documentation & comments about proxy submissions
f3b3a7b drm/i915/guc: Make intel_guc_send a function pointer
68aaede drm/i915/guc: Break out the GuC log extras into their own "runtime" struct
deb97d4 drm/i915/guc: The Additional Data Struct (ADS) should get enabled together with GuC submission
c821db7 drm/i915/guc: Add onion teardown to the GuC setup
3abdc29 drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
96f0f61 drm/i915/guc: Sanitize GuC client initialization
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4250/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 28+ messages in thread