Commit abddffdf authored by Joonas Lahtinen's avatar Joonas Lahtinen

drm/i915/guc: Sanitize GuC client initialization

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: default avatarJoonas 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>
Signed-off-by: default avatarOscar Mateo <oscar.mateo@intel.com>
Reviewed-by: default avatarDaniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
parent d223760f
......@@ -2473,7 +2473,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);
......@@ -2508,7 +2508,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);
......
......@@ -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);
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;
client->doorbell_id = new_id;
if (new_id == GUC_INVALID_DOORBELL_ID)
return 0;
}
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;
/* Activate the new doorbell */
__set_bit(new_id, doorbell_bitmap);
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));
DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
hi_pri ? "high" : "normal", id);
/* XXX: wait for any interrupts */
/* XXX: wait for workqueue to drain */
return id;
}
err = __destroy_doorbell(client);
if (err)
return err;
/*
* 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.
*/
__update_doorbell_desc(client, GUC_DOORBELL_INVALID);
__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 */
......@@ -694,93 +732,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);
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);
if (enabled == expected)
drbregl = I915_READ(GEN8_DRBREGL(db_id));
valid = drbregl & GEN8_DRB_VALID;
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;
}
/**
......@@ -806,49 +852,46 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
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
......@@ -863,27 +906,28 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
guc_proc_desc_init(guc, client);
guc_ctx_desc_init(guc, client);
/* For runtime client allocation we need to enable the doorbell. Not
* required yet for the static execbuf_client as this special kernel
* client is enabled from i915_guc_submission_enable().
*
* guc_update_doorbell_id(guc, client, db_id);
*/
/* 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",
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;
......@@ -984,7 +1028,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)
......@@ -1006,7 +1050,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;
}
......@@ -1077,14 +1121,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);
......@@ -1146,6 +1195,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);
}
......@@ -1156,10 +1210,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);
......@@ -1221,5 +1273,3 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
return intel_guc_send(guc, data, ARRAY_SIZE(data));
}
......@@ -241,8 +241,8 @@ union guc_doorbell_qw {
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)
......
......@@ -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 */
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment