* [PATCH v5 01/13] drm/i915/guc: Sanitize GuC client initialization
2017-03-22 17:39 [PATCH 00/13] Various improvements around the GuC topic Oscar Mateo
@ 2017-03-22 17:39 ` Oscar Mateo
2017-03-22 17:39 ` [PATCH v5 02/13] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access Oscar Mateo
` (12 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Oscar Mateo @ 2017-03-22 17:39 UTC (permalink / raw)
To: intel-gfx
From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
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)
v5:
- gccisms like arithmetic on void pointers are not frowned upon (Oscar)
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 | 398 ++++++++++++++++-------------
drivers/gpu/drm/i915/intel_guc_fwif.h | 4 +-
drivers/gpu/drm/i915/intel_uc.h | 11 +-
4 files changed, 233 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..b19753c 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,100 @@ 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 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 +332,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 +501,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 +733,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 +853,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 +907,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 +1029,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 +1051,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 +1122,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 +1196,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 +1211,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 +1274,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] 22+ messages in thread* [PATCH v5 02/13] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
2017-03-22 17:39 [PATCH 00/13] Various improvements around the GuC topic Oscar Mateo
2017-03-22 17:39 ` [PATCH v5 01/13] drm/i915/guc: Sanitize GuC client initialization Oscar Mateo
@ 2017-03-22 17:39 ` Oscar Mateo
2017-03-22 17:39 ` [PATCH v4 03/13] drm/i915/guc: Add onion teardown to the GuC setup Oscar Mateo
` (11 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Oscar Mateo @ 2017-03-22 17:39 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 (Daniele)
v5: Nicer than arithmetic on pointers (Chris, Joonas)
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 | 94 +++++++++++++++---------------
drivers/gpu/drm/i915/intel_guc_loader.c | 2 +-
drivers/gpu/drm/i915/intel_uc.h | 3 +-
3 files changed, 49 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index b19753c..d4b2a9b 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -133,6 +133,13 @@ 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)
+{
+ struct guc_context_desc *base = client->guc->ctx_pool_vaddr;
+
+ return &base[client->ctx_index];
+}
+
/*
* Initialise, update, or clear doorbell data shared with the GuC
*
@@ -142,21 +149,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;
}
@@ -271,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
@@ -318,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;
+ struct guc_context_desc *desc;
- memset(&desc, 0, sizeof(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));
}
/**
@@ -1024,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;
@@ -1035,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);
@@ -1217,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] 22+ messages in thread* [PATCH v4 03/13] drm/i915/guc: Add onion teardown to the GuC setup
2017-03-22 17:39 [PATCH 00/13] Various improvements around the GuC topic Oscar Mateo
2017-03-22 17:39 ` [PATCH v5 01/13] drm/i915/guc: Sanitize GuC client initialization Oscar Mateo
2017-03-22 17:39 ` [PATCH v5 02/13] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access Oscar Mateo
@ 2017-03-22 17:39 ` Oscar Mateo
2017-03-23 22:57 ` Chris Wilson
2017-03-22 17:39 ` [PATCH 04/13] drm/i915/guc: The Additional Data Struct (ADS) should get enabled together with GuC submission Oscar Mateo
` (10 subsequent siblings)
13 siblings, 1 reply; 22+ messages in thread
From: Oscar Mateo @ 2017-03-22 17:39 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 d4b2a9b..08287f7 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] 22+ messages in thread* Re: [PATCH v4 03/13] drm/i915/guc: Add onion teardown to the GuC setup
2017-03-22 17:39 ` [PATCH v4 03/13] drm/i915/guc: Add onion teardown to the GuC setup Oscar Mateo
@ 2017-03-23 22:57 ` Chris Wilson
2017-03-23 16:36 ` Oscar Mateo
0 siblings, 1 reply; 22+ messages in thread
From: Chris Wilson @ 2017-03-23 22:57 UTC (permalink / raw)
To: Oscar Mateo; +Cc: intel-gfx
On Wed, Mar 22, 2017 at 10:39:46AM -0700, Oscar Mateo wrote:
> 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);
> 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:
I'm not happy with moving subfeature detection logic into the core GEM
code. if (i915.enable_guc_loading) firstly should never be a module
parameter (it's derived state!) and secondly it should reside next to
the dependent logic and not be interrupting the central control flow.
-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] 22+ messages in thread* Re: [PATCH v4 03/13] drm/i915/guc: Add onion teardown to the GuC setup
2017-03-23 22:57 ` Chris Wilson
@ 2017-03-23 16:36 ` Oscar Mateo
2017-03-24 8:59 ` Chris Wilson
0 siblings, 1 reply; 22+ messages in thread
From: Oscar Mateo @ 2017-03-23 16:36 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On 03/23/2017 03:57 PM, Chris Wilson wrote:
> On Wed, Mar 22, 2017 at 10:39:46AM -0700, Oscar Mateo wrote:
>> 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);
>> 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:
> I'm not happy with moving subfeature detection logic into the core GEM
> code. if (i915.enable_guc_loading) firstly should never be a module
> parameter (it's derived state!) and secondly it should reside next to
> the dependent logic and not be interrupting the central control flow.
> -Chris
What do you mean it's derived state? from what?
As for interrupting the central control flow, you are probably right, I
can change it back (the code was refactored so many times that I cannot
remember my logic behind that change anymore)
- Oscar
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v4 03/13] drm/i915/guc: Add onion teardown to the GuC setup
2017-03-23 16:36 ` Oscar Mateo
@ 2017-03-24 8:59 ` Chris Wilson
2017-03-27 13:06 ` Joonas Lahtinen
2017-03-27 17:33 ` Oscar Mateo
0 siblings, 2 replies; 22+ messages in thread
From: Chris Wilson @ 2017-03-24 8:59 UTC (permalink / raw)
To: Oscar Mateo; +Cc: intel-gfx
On Thu, Mar 23, 2017 at 09:36:10AM -0700, Oscar Mateo wrote:
> On 03/23/2017 03:57 PM, Chris Wilson wrote:
> >I'm not happy with moving subfeature detection logic into the core GEM
> >code. if (i915.enable_guc_loading) firstly should never be a module
> >parameter (it's derived state!) and secondly it should reside next to
> >the dependent logic and not be interrupting the central control flow.
> What do you mean it's derived state? from what?
The set of features, whether to use guc submission, huc, or whether
there is a platform requirement to load the firmware, define whether or
not we need to request and upload a particular firmware. Every module
option should be a quirk to alter driver behaviour (i.e. a debugging
crutch), few and strongly justified (we have too many) and necessarily
global in scope. Device specific options should ideally use a more
specific interface (most clear examples are the panel specific quirks).
-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] 22+ messages in thread
* Re: [PATCH v4 03/13] drm/i915/guc: Add onion teardown to the GuC setup
2017-03-24 8:59 ` Chris Wilson
@ 2017-03-27 13:06 ` Joonas Lahtinen
2017-03-27 17:33 ` Oscar Mateo
1 sibling, 0 replies; 22+ messages in thread
From: Joonas Lahtinen @ 2017-03-27 13:06 UTC (permalink / raw)
To: Oscar Mateo; +Cc: intel-gfx
On pe, 2017-03-24 at 08:59 +0000, Chris Wilson wrote:
> On Thu, Mar 23, 2017 at 09:36:10AM -0700, Oscar Mateo wrote:
> >
> > On 03/23/2017 03:57 PM, Chris Wilson wrote:
> > >
> > > I'm not happy with moving subfeature detection logic into the core GEM
> > > code. if (i915.enable_guc_loading) firstly should never be a module
> > > parameter (it's derived state!) and secondly it should reside next to
> > > the dependent logic and not be interrupting the central control flow.
> > What do you mean it's derived state? from what?
>
> The set of features, whether to use guc submission, huc, or whether
> there is a platform requirement to load the firmware, define whether or
> not we need to request and upload a particular firmware. Every module
> option should be a quirk to alter driver behaviour (i.e. a debugging
> crutch), few and strongly justified (we have too many) and necessarily
> global in scope. Device specific options should ideally use a more
> specific interface (most clear examples are the panel specific quirks).
Misguidance here was probably me enforcing the view that by hoisting
and unifying the checks, it'll be easier to comprehend the code when a
check covers greater amount of lines. I have to agree that the top-
level driver control flow should not be having the checks, but each
code path existing only within a .c file should have the "skip this
branch" check earlier than later.
Sorry for misguidance :P
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] 22+ messages in thread
* Re: [PATCH v4 03/13] drm/i915/guc: Add onion teardown to the GuC setup
2017-03-24 8:59 ` Chris Wilson
2017-03-27 13:06 ` Joonas Lahtinen
@ 2017-03-27 17:33 ` Oscar Mateo
1 sibling, 0 replies; 22+ messages in thread
From: Oscar Mateo @ 2017-03-27 17:33 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 03/24/2017 01:59 AM, Chris Wilson wrote:
> On Thu, Mar 23, 2017 at 09:36:10AM -0700, Oscar Mateo wrote:
>> On 03/23/2017 03:57 PM, Chris Wilson wrote:
>>> I'm not happy with moving subfeature detection logic into the core GEM
>>> code. if (i915.enable_guc_loading) firstly should never be a module
>>> parameter (it's derived state!) and secondly it should reside next to
>>> the dependent logic and not be interrupting the central control flow.
>> What do you mean it's derived state? from what?
> The set of features, whether to use guc submission, huc, or whether
> there is a platform requirement to load the firmware, define whether or
> not we need to request and upload a particular firmware. Every module
> option should be a quirk to alter driver behaviour (i.e. a debugging
> crutch), few and strongly justified (we have too many) and necessarily
> global in scope. Device specific options should ideally use a more
> specific interface (most clear examples are the panel specific quirks).
> -Chris
>
Ok, I see what you mean now. I didn't follow the GuC development, so I
don't know how we got here (separate enable_guc_loading and
enable_guc_submission module parameters, a lot of different
HAS_GUC_UCODE/HAS_GUC_SCHED/HAS_HUC_UCODE that test the same thing, etc...).
I'll create a patch with a cleanup proposal and send it as an RFC.
-- Oscar
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 04/13] drm/i915/guc: The Additional Data Struct (ADS) should get enabled together with GuC submission
2017-03-22 17:39 [PATCH 00/13] Various improvements around the GuC topic Oscar Mateo
` (2 preceding siblings ...)
2017-03-22 17:39 ` [PATCH v4 03/13] drm/i915/guc: Add onion teardown to the GuC setup Oscar Mateo
@ 2017-03-22 17:39 ` Oscar Mateo
2017-03-22 17:39 ` [PATCH v3 05/13] drm/i915/guc: Break out the GuC log extras into their own "runtime" struct Oscar Mateo
` (9 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Oscar Mateo @ 2017-03-22 17:39 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: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@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 08287f7..b3d1d5c 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] 22+ messages in thread* [PATCH v3 05/13] drm/i915/guc: Break out the GuC log extras into their own "runtime" struct
2017-03-22 17:39 [PATCH 00/13] Various improvements around the GuC topic Oscar Mateo
` (3 preceding siblings ...)
2017-03-22 17:39 ` [PATCH 04/13] drm/i915/guc: The Additional Data Struct (ADS) should get enabled together with GuC submission Oscar Mateo
@ 2017-03-22 17:39 ` Oscar Mateo
2017-03-22 17:39 ` [PATCH v3 06/13] drm/i915/guc: Make intel_guc_send a function pointer Oscar Mateo
` (8 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Oscar Mateo @ 2017-03-22 17:39 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] 22+ messages in thread* [PATCH v3 06/13] drm/i915/guc: Make intel_guc_send a function pointer
2017-03-22 17:39 [PATCH 00/13] Various improvements around the GuC topic Oscar Mateo
` (4 preceding siblings ...)
2017-03-22 17:39 ` [PATCH v3 05/13] drm/i915/guc: Break out the GuC log extras into their own "runtime" struct Oscar Mateo
@ 2017-03-22 17:39 ` Oscar Mateo
2017-03-22 17:39 ` [PATCH v2 07/13] drm/i915/guc: Improve the GuC documentation & comments about proxy submissions Oscar Mateo
` (7 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Oscar Mateo @ 2017-03-22 17:39 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] 22+ messages in thread* [PATCH v2 07/13] drm/i915/guc: Improve the GuC documentation & comments about proxy submissions
2017-03-22 17:39 [PATCH 00/13] Various improvements around the GuC topic Oscar Mateo
` (5 preceding siblings ...)
2017-03-22 17:39 ` [PATCH v3 06/13] drm/i915/guc: Make intel_guc_send a function pointer Oscar Mateo
@ 2017-03-22 17:39 ` Oscar Mateo
2017-03-22 17:39 ` [PATCH v2 08/13] drm/i915/guc: Wait for doorbell to be inactive before deallocating Oscar Mateo
` (6 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Oscar Mateo @ 2017-03-22 17:39 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)
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@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 b3d1d5c..c02167c 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] 22+ messages in thread* [PATCH v2 08/13] drm/i915/guc: Wait for doorbell to be inactive before deallocating
2017-03-22 17:39 [PATCH 00/13] Various improvements around the GuC topic Oscar Mateo
` (6 preceding siblings ...)
2017-03-22 17:39 ` [PATCH v2 07/13] drm/i915/guc: Improve the GuC documentation & comments about proxy submissions Oscar Mateo
@ 2017-03-22 17:39 ` Oscar Mateo
2017-03-22 17:39 ` [PATCH v2 09/13] drm/i915/guc: A little bit more of doorbell sanitization Oscar Mateo
` (5 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Oscar Mateo @ 2017-03-22 17:39 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)
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.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 c02167c..2d4d854 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] 22+ messages in thread* [PATCH v2 09/13] drm/i915/guc: A little bit more of doorbell sanitization
2017-03-22 17:39 [PATCH 00/13] Various improvements around the GuC topic Oscar Mateo
` (7 preceding siblings ...)
2017-03-22 17:39 ` [PATCH v2 08/13] drm/i915/guc: Wait for doorbell to be inactive before deallocating Oscar Mateo
@ 2017-03-22 17:39 ` Oscar Mateo
2017-03-22 17:39 ` [PATCH v3 10/13] drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor" Oscar Mateo
` (4 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Oscar Mateo @ 2017-03-22 17:39 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>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.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 2d4d854..c9d324f 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -163,15 +163,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] 22+ messages in thread* [PATCH v3 10/13] drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor"
2017-03-22 17:39 [PATCH 00/13] Various improvements around the GuC topic Oscar Mateo
` (8 preceding siblings ...)
2017-03-22 17:39 ` [PATCH v2 09/13] drm/i915/guc: A little bit more of doorbell sanitization Oscar Mateo
@ 2017-03-22 17:39 ` Oscar Mateo
2017-03-22 17:39 ` [PATCH 11/13] drm/i915/guc: Split out the mmio_white_list struct Oscar Mateo
` (3 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Oscar Mateo @ 2017-03-22 17:39 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)
v3:
- Calculate gemsize with less intermediate steps (Joonas)
- Use BIT() macro (Joonas)
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
drivers/gpu/drm/i915/i915_guc_submission.c | 118 +++++++++++++++--------------
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, 96 insertions(+), 86 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 c9d324f..735dcba 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,31 +129,31 @@ 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)
{
- struct guc_context_desc *base = client->guc->ctx_pool_vaddr;
+ struct guc_stage_desc *base = client->guc->stage_desc_pool_vaddr;
- return &base[client->ctx_index];
+ return &base[client->stage_id];
}
/*
@@ -165,10 +165,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,29 @@ 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 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);
+ vma = intel_guc_allocate_vma(guc,
+ PAGE_ALIGN(sizeof(struct guc_stage_desc) *
+ GUC_MAX_STAGE_DESCRIPTORS));
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 +1124,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 +1141,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 +1201,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..18131b7 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 BIT(0)
+#define GUC_STAGE_DESC_ATTR_PENDING_DB BIT(1)
+#define GUC_STAGE_DESC_ATTR_KERNEL BIT(2)
+#define GUC_STAGE_DESC_ATTR_PREEMPT BIT(3)
+#define GUC_STAGE_DESC_ATTR_RESET BIT(4)
+#define GUC_STAGE_DESC_ATTR_WQLOCKED BIT(5)
+#define GUC_STAGE_DESC_ATTR_PCH BIT(6)
+#define GUC_STAGE_DESC_ATTR_TERMINATED BIT(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] 22+ messages in thread* [PATCH 11/13] drm/i915/guc: Split out the mmio_white_list struct
2017-03-22 17:39 [PATCH 00/13] Various improvements around the GuC topic Oscar Mateo
` (9 preceding siblings ...)
2017-03-22 17:39 ` [PATCH v3 10/13] drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor" Oscar Mateo
@ 2017-03-22 17:39 ` Oscar Mateo
2017-03-22 17:39 ` [PATCH 12/13] drm/i915/guc: Move guc_interrupts_release next to guc_interrupts_capture Oscar Mateo
` (2 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Oscar Mateo @ 2017-03-22 17:39 UTC (permalink / raw)
To: intel-gfx
We are going to need it for future platforms.
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@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 735dcba..3d78a6b 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 18131b7..cb36cbf 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] 22+ messages in thread* [PATCH 12/13] drm/i915/guc: Move guc_interrupts_release next to guc_interrupts_capture
2017-03-22 17:39 [PATCH 00/13] Various improvements around the GuC topic Oscar Mateo
` (10 preceding siblings ...)
2017-03-22 17:39 ` [PATCH 11/13] drm/i915/guc: Split out the mmio_white_list struct Oscar Mateo
@ 2017-03-22 17:39 ` Oscar Mateo
2017-03-22 17:39 ` [PATCH 13/13] HAX Enable GuC loading & submission Oscar Mateo
2017-03-23 11:06 ` ✓ Fi.CI.BAT: success for Various improvements around the GuC topic Patchwork
13 siblings, 0 replies; 22+ messages in thread
From: Oscar Mateo @ 2017-03-22 17:39 UTC (permalink / raw)
To: intel-gfx
They go better together.
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@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 3d78a6b..3398204 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1190,6 +1190,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;
@@ -1253,30 +1277,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] 22+ messages in thread* [PATCH 13/13] HAX Enable GuC loading & submission
2017-03-22 17:39 [PATCH 00/13] Various improvements around the GuC topic Oscar Mateo
` (11 preceding siblings ...)
2017-03-22 17:39 ` [PATCH 12/13] drm/i915/guc: Move guc_interrupts_release next to guc_interrupts_capture Oscar Mateo
@ 2017-03-22 17:39 ` Oscar Mateo
2017-03-23 11:06 ` ✓ Fi.CI.BAT: success for Various improvements around the GuC topic Patchwork
13 siblings, 0 replies; 22+ messages in thread
From: Oscar Mateo @ 2017-03-22 17:39 UTC (permalink / raw)
To: intel-gfx
This is just for CI testing, do not merge.
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_params.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b6a7e36..abd2894 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -56,8 +56,8 @@ struct i915_params i915 __read_mostly = {
.verbose_state_checks = 1,
.nuclear_pageflip = 0,
.edp_vswing = 0,
- .enable_guc_loading = 0,
- .enable_guc_submission = 0,
+ .enable_guc_loading = 1,
+ .enable_guc_submission = 1,
.guc_log_level = -1,
.guc_firmware_path = NULL,
.huc_firmware_path = NULL,
@@ -221,12 +221,12 @@ struct i915_params i915 __read_mostly = {
module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int, 0400);
MODULE_PARM_DESC(enable_guc_loading,
"Enable GuC firmware loading "
- "(-1=auto, 0=never [default], 1=if available, 2=required)");
+ "(-1=auto, 0=never, 1=if available [default], 2=required)");
module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400);
MODULE_PARM_DESC(enable_guc_submission,
"Enable GuC submission "
- "(-1=auto, 0=never [default], 1=if available, 2=required)");
+ "(-1=auto, 0=never, 1=if available [default], 2=required)");
module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
MODULE_PARM_DESC(guc_log_level,
--
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] 22+ messages in thread* ✓ Fi.CI.BAT: success for Various improvements around the GuC topic
2017-03-22 17:39 [PATCH 00/13] Various improvements around the GuC topic Oscar Mateo
` (12 preceding siblings ...)
2017-03-22 17:39 ` [PATCH 13/13] HAX Enable GuC loading & submission Oscar Mateo
@ 2017-03-23 11:06 ` Patchwork
2017-03-23 13:20 ` Joonas Lahtinen
13 siblings, 1 reply; 22+ messages in thread
From: Patchwork @ 2017-03-23 11:06 UTC (permalink / raw)
To: Oscar Mateo; +Cc: intel-gfx
== Series Details ==
Series: Various improvements around the GuC topic
URL : https://patchwork.freedesktop.org/series/21726/
State : success
== Summary ==
Series 21726v1 Various improvements around the GuC topic
https://patchwork.freedesktop.org/api/1.0/series/21726/revisions/1/mbox/
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 460s
fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time: 461s
fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 time: 593s
fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 523s
fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 time: 511s
fi-byt-n2820 total:278 pass:247 dwarn:0 dfail:0 fail:0 skip:31 time: 502s
fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 441s
fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 431s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 436s
fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 509s
fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 496s
fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 478s
fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 468s
fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time: 588s
fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time: 480s
fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 513s
fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time: 446s
fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 558s
fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time: 416s
8229a8c712c22ff8e94e3244d4fd942a7dcd89af drm-tip: 2017y-03m-23d-09h-57m-34s UTC integration manifest
f003ba2 HAX Enable GuC loading & submission
43d51b6 drm/i915/guc: Move guc_interrupts_release next to guc_interrupts_capture
19b8f8a drm/i915/guc: Split out the mmio_white_list struct
22675ac drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor"
98c3280 drm/i915/guc: A little bit more of doorbell sanitization
a5d1810 drm/i915/guc: Wait for doorbell to be inactive before deallocating
cf3850d drm/i915/guc: Improve the GuC documentation & comments about proxy submissions
7112b1b drm/i915/guc: Make intel_guc_send a function pointer
cdbbc12 drm/i915/guc: Break out the GuC log extras into their own "runtime" struct
0009b27 drm/i915/guc: The Additional Data Struct (ADS) should get enabled together with GuC submission
c11c121 drm/i915/guc: Add onion teardown to the GuC setup
d1a1ced drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
5956a8b drm/i915/guc: Sanitize GuC client initialization
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4273/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: ✓ Fi.CI.BAT: success for Various improvements around the GuC topic
2017-03-23 11:06 ` ✓ Fi.CI.BAT: success for Various improvements around the GuC topic Patchwork
@ 2017-03-23 13:20 ` Joonas Lahtinen
2017-03-23 15:15 ` Oscar Mateo
0 siblings, 1 reply; 22+ messages in thread
From: Joonas Lahtinen @ 2017-03-23 13:20 UTC (permalink / raw)
To: intel-gfx, Oscar Mateo, Ceraolo Spurio, Daniele, Anusha Srivatsa,
Wajdeczko, Michal, Winiarski, Michal, Ursulin, Tvrtko
Merged this series except the HAX patch (also, reordered the S-o-b, R-b
and Cc lines to canonical form), so do rebase your work.
Regards, Joonas
On to, 2017-03-23 at 11:06 +0000, Patchwork wrote:
> == Series Details ==
>
> Series: Various improvements around the GuC topic
> URL : https://patchwork.freedesktop.org/series/21726/
> State : success
>
> == Summary ==
>
> Series 21726v1 Various improvements around the GuC topic
> https://patchwork.freedesktop.org/api/1.0/series/21726/revisions/1/mbox/
>
> fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 460s
> fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time: 461s
> fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 time: 593s
> fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 523s
> fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 time: 511s
> fi-byt-n2820 total:278 pass:247 dwarn:0 dfail:0 fail:0 skip:31 time: 502s
> fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 441s
> fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 431s
> fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 436s
> fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 509s
> fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 496s
> fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 478s
> fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 468s
> fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time: 588s
> fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time: 480s
> fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 513s
> fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time: 446s
> fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 558s
> fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time: 416s
>
> 8229a8c712c22ff8e94e3244d4fd942a7dcd89af drm-tip: 2017y-03m-23d-09h-57m-34s UTC integration manifest
> f003ba2 HAX Enable GuC loading & submission
> 43d51b6 drm/i915/guc: Move guc_interrupts_release next to guc_interrupts_capture
> 19b8f8a drm/i915/guc: Split out the mmio_white_list struct
> 22675ac drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor"
> 98c3280 drm/i915/guc: A little bit more of doorbell sanitization
> a5d1810 drm/i915/guc: Wait for doorbell to be inactive before deallocating
> cf3850d drm/i915/guc: Improve the GuC documentation & comments about proxy submissions
> 7112b1b drm/i915/guc: Make intel_guc_send a function pointer
> cdbbc12 drm/i915/guc: Break out the GuC log extras into their own "runtime" struct
> 0009b27 drm/i915/guc: The Additional Data Struct (ADS) should get enabled together with GuC submission
> c11c121 drm/i915/guc: Add onion teardown to the GuC setup
> d1a1ced drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
> 5956a8b drm/i915/guc: Sanitize GuC client initialization
>
> == Logs ==
>
> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4273/
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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] 22+ messages in thread
* Re: ✓ Fi.CI.BAT: success for Various improvements around the GuC topic
2017-03-23 13:20 ` Joonas Lahtinen
@ 2017-03-23 15:15 ` Oscar Mateo
0 siblings, 0 replies; 22+ messages in thread
From: Oscar Mateo @ 2017-03-23 15:15 UTC (permalink / raw)
To: Joonas Lahtinen, intel-gfx, Ceraolo Spurio, Daniele,
Anusha Srivatsa, Wajdeczko, Michal, Winiarski, Michal, Ursulin,
Tvrtko
On 03/23/2017 06:20 AM, Joonas Lahtinen wrote:
> Merged this series except the HAX patch (also, reordered the S-o-b, R-b
> and Cc lines to canonical form), so do rebase your work.
>
> Regards, Joonas
Thanks!
> On to, 2017-03-23 at 11:06 +0000, Patchwork wrote:
>> == Series Details ==
>>
>> Series: Various improvements around the GuC topic
>> URL : https://patchwork.freedesktop.org/series/21726/
>> State : success
>>
>> == Summary ==
>>
>> Series 21726v1 Various improvements around the GuC topic
>> https://patchwork.freedesktop.org/api/1.0/series/21726/revisions/1/mbox/
>>
>> fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 460s
>> fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time: 461s
>> fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 time: 593s
>> fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 523s
>> fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 time: 511s
>> fi-byt-n2820 total:278 pass:247 dwarn:0 dfail:0 fail:0 skip:31 time: 502s
>> fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 441s
>> fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 431s
>> fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 436s
>> fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 509s
>> fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 496s
>> fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 478s
>> fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 468s
>> fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time: 588s
>> fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time: 480s
>> fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 513s
>> fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time: 446s
>> fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 558s
>> fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time: 416s
>>
>> 8229a8c712c22ff8e94e3244d4fd942a7dcd89af drm-tip: 2017y-03m-23d-09h-57m-34s UTC integration manifest
>> f003ba2 HAX Enable GuC loading & submission
>> 43d51b6 drm/i915/guc: Move guc_interrupts_release next to guc_interrupts_capture
>> 19b8f8a drm/i915/guc: Split out the mmio_white_list struct
>> 22675ac drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor"
>> 98c3280 drm/i915/guc: A little bit more of doorbell sanitization
>> a5d1810 drm/i915/guc: Wait for doorbell to be inactive before deallocating
>> cf3850d drm/i915/guc: Improve the GuC documentation & comments about proxy submissions
>> 7112b1b drm/i915/guc: Make intel_guc_send a function pointer
>> cdbbc12 drm/i915/guc: Break out the GuC log extras into their own "runtime" struct
>> 0009b27 drm/i915/guc: The Additional Data Struct (ADS) should get enabled together with GuC submission
>> c11c121 drm/i915/guc: Add onion teardown to the GuC setup
>> d1a1ced drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
>> 5956a8b drm/i915/guc: Sanitize GuC client initialization
>>
>> == Logs ==
>>
>> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4273/
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread