* [PATCH] drm/i915: Reduce context HW ID lifetime
@ 2018-08-30 10:24 Chris Wilson
2018-08-30 12:38 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Reduce context HW ID lifetime (rev2) Patchwork
` (9 more replies)
0 siblings, 10 replies; 19+ messages in thread
From: Chris Wilson @ 2018-08-30 10:24 UTC (permalink / raw)
To: intel-gfx; +Cc: Oscar Mateo, Mika Kuoppala
Future gen reduce the number of bits we will have available to
differentiate between contexts, so reduce the lifetime of the ID
assignment from that of the context to its current active cycle (i.e.
only while it is pinned for use by the HW, will it have a constant ID).
This means that instead of a max of 2k allocated contexts (worst case
before fun with bit twiddling), we instead have a limit of 2k in flight
contexts (minus a few that have been pinned by the kernel or by perf).
We cannot reduce the scope of an HW-ID to an engine (allowing the same
gem_context to have different ids on each engine) as in the future we
will need to preassign an id before we know which engine the
context is being executed on.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
drivers/gpu/drm/i915/i915_drv.h | 2 +
drivers/gpu/drm/i915/i915_gem_context.c | 207 +++++++++++++-----
drivers/gpu/drm/i915/i915_gem_context.h | 17 ++
drivers/gpu/drm/i915/intel_lrc.c | 8 +
drivers/gpu/drm/i915/selftests/mock_context.c | 11 +-
6 files changed, 181 insertions(+), 69 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a5265c236a33..bf3b6c6db51d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1953,7 +1953,10 @@ static int i915_context_status(struct seq_file *m, void *unused)
return ret;
list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
- seq_printf(m, "HW context %u ", ctx->hw_id);
+ seq_puts(m, "HW context ");
+ if (!list_empty(&ctx->hw_id_link))
+ seq_printf(m, "%x [pin %u]",
+ ctx->hw_id, atomic_read(&ctx->pin_hw_id));
if (ctx->pid) {
struct task_struct *task;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e5b9d3c77139..ca6b55d29d6a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1862,6 +1862,7 @@ struct drm_i915_private {
struct mutex av_mutex;
struct {
+ struct mutex mutex;
struct list_head list;
struct llist_head free_list;
struct work_struct free_work;
@@ -1874,6 +1875,7 @@ struct drm_i915_private {
#define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */
#define MAX_GUC_CONTEXT_HW_ID (1 << 20) /* exclusive */
#define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */
+ struct list_head hw_id_list;
} contexts;
u32 fdi_rx_config;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index f15a039772db..d3390942f37b 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -115,6 +115,85 @@ static void lut_close(struct i915_gem_context *ctx)
rcu_read_unlock();
}
+static inline int new_hw_id(struct drm_i915_private *i915, gfp_t gfp)
+{
+ unsigned int max;
+
+ lockdep_assert_held(&i915->contexts.mutex);
+
+ if (INTEL_GEN(i915) >= 11)
+ max = GEN11_MAX_CONTEXT_HW_ID;
+ else if (USES_GUC_SUBMISSION(i915))
+ /*
+ * When using GuC in proxy submission, GuC consumes the
+ * highest bit in the context id to indicate proxy submission.
+ */
+ max = MAX_GUC_CONTEXT_HW_ID;
+ else
+ max = MAX_CONTEXT_HW_ID;
+
+ return ida_simple_get(&i915->contexts.hw_ida, 0, max, gfp);
+}
+
+static int steal_hw_id(struct drm_i915_private *i915)
+{
+ struct i915_gem_context *ctx, *cn;
+ LIST_HEAD(pinned);
+ int id = -ENOSPC;
+
+ lockdep_assert_held(&i915->contexts.mutex);
+
+ list_for_each_entry_safe(ctx, cn,
+ &i915->contexts.hw_id_list, hw_id_link) {
+ if (atomic_read(&ctx->pin_hw_id)) {
+ list_move_tail(&ctx->hw_id_link, &pinned);
+ continue;
+ }
+
+ GEM_BUG_ON(!ctx->hw_id); /* perma-pinned kernel context */
+ list_del_init(&ctx->hw_id_link);
+ id = ctx->hw_id;
+ break;
+ }
+
+ list_splice_tail(&pinned, &i915->contexts.hw_id_list);
+ return id;
+}
+
+static int assign_hw_id(struct drm_i915_private *i915, unsigned int *out)
+{
+ int ret;
+
+ lockdep_assert_held(&i915->contexts.mutex);
+
+ ret = new_hw_id(i915, GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
+ if (unlikely(ret < 0)) {
+ ret = steal_hw_id(i915);
+ if (ret < 0) /* once again for the correct erro code */
+ ret = new_hw_id(i915, GFP_KERNEL);
+ if (ret < 0)
+ return ret;
+ }
+
+ *out = ret;
+ return 0;
+}
+
+static void release_hw_id(struct i915_gem_context *ctx)
+{
+ struct drm_i915_private *i915 = ctx->i915;
+
+ if (list_empty(&ctx->hw_id_link))
+ return;
+
+ mutex_lock(&i915->contexts.mutex);
+ if (!list_empty(&ctx->hw_id_link)) {
+ ida_simple_remove(&i915->contexts.hw_ida, ctx->hw_id);
+ list_del_init(&ctx->hw_id_link);
+ }
+ mutex_unlock(&i915->contexts.mutex);
+}
+
static void i915_gem_context_free(struct i915_gem_context *ctx)
{
unsigned int n;
@@ -122,6 +201,7 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
lockdep_assert_held(&ctx->i915->drm.struct_mutex);
GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
+ release_hw_id(ctx);
i915_ppgtt_put(ctx->ppgtt);
for (n = 0; n < ARRAY_SIZE(ctx->__engine); n++) {
@@ -136,7 +216,6 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
list_del(&ctx->link);
- ida_simple_remove(&ctx->i915->contexts.hw_ida, ctx->hw_id);
kfree_rcu(ctx, rcu);
}
@@ -190,6 +269,12 @@ static void context_close(struct i915_gem_context *ctx)
{
i915_gem_context_set_closed(ctx);
+ /*
+ * This context will never again be assinged to HW, so we can
+ * reuse its ID for the next context.
+ */
+ release_hw_id(ctx);
+
/*
* The LUT uses the VMA as a backpointer to unref the object,
* so we need to clear the LUT before we close all the VMA (inside
@@ -203,43 +288,6 @@ static void context_close(struct i915_gem_context *ctx)
i915_gem_context_put(ctx);
}
-static int assign_hw_id(struct drm_i915_private *dev_priv, unsigned *out)
-{
- int ret;
- unsigned int max;
-
- if (INTEL_GEN(dev_priv) >= 11) {
- max = GEN11_MAX_CONTEXT_HW_ID;
- } else {
- /*
- * When using GuC in proxy submission, GuC consumes the
- * highest bit in the context id to indicate proxy submission.
- */
- if (USES_GUC_SUBMISSION(dev_priv))
- max = MAX_GUC_CONTEXT_HW_ID;
- else
- max = MAX_CONTEXT_HW_ID;
- }
-
-
- ret = ida_simple_get(&dev_priv->contexts.hw_ida,
- 0, max, GFP_KERNEL);
- if (ret < 0) {
- /* Contexts are only released when no longer active.
- * Flush any pending retires to hopefully release some
- * stale contexts and try again.
- */
- i915_retire_requests(dev_priv);
- ret = ida_simple_get(&dev_priv->contexts.hw_ida,
- 0, max, GFP_KERNEL);
- if (ret < 0)
- return ret;
- }
-
- *out = ret;
- return 0;
-}
-
static u32 default_desc_template(const struct drm_i915_private *i915,
const struct i915_hw_ppgtt *ppgtt)
{
@@ -276,12 +324,6 @@ __create_hw_context(struct drm_i915_private *dev_priv,
if (ctx == NULL)
return ERR_PTR(-ENOMEM);
- ret = assign_hw_id(dev_priv, &ctx->hw_id);
- if (ret) {
- kfree(ctx);
- return ERR_PTR(ret);
- }
-
kref_init(&ctx->ref);
list_add_tail(&ctx->link, &dev_priv->contexts.list);
ctx->i915 = dev_priv;
@@ -295,6 +337,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
INIT_LIST_HEAD(&ctx->handles_list);
+ INIT_LIST_HEAD(&ctx->hw_id_link);
/* Default context will never have a file_priv */
ret = DEFAULT_CONTEXT_HANDLE;
@@ -421,15 +464,35 @@ i915_gem_context_create_gvt(struct drm_device *dev)
return ctx;
}
+static void
+destroy_kernel_context(struct i915_gem_context **ctxp)
+{
+ struct i915_gem_context *ctx;
+
+ /* Keep the context ref so that we can free it immediately ourselves */
+ ctx = i915_gem_context_get(fetch_and_zero(ctxp));
+ GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
+
+ context_close(ctx);
+ i915_gem_context_free(ctx);
+}
+
struct i915_gem_context *
i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
{
struct i915_gem_context *ctx;
+ int err;
ctx = i915_gem_create_context(i915, NULL);
if (IS_ERR(ctx))
return ctx;
+ err = i915_gem_context_pin_hw_id(ctx);
+ if (err) {
+ destroy_kernel_context(&ctx);
+ return ERR_PTR(err);
+ }
+
i915_gem_context_clear_bannable(ctx);
ctx->sched.priority = prio;
ctx->ring_size = PAGE_SIZE;
@@ -439,17 +502,19 @@ i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
return ctx;
}
-static void
-destroy_kernel_context(struct i915_gem_context **ctxp)
+static void init_contexts(struct drm_i915_private *i915)
{
- struct i915_gem_context *ctx;
+ mutex_init(&i915->contexts.mutex);
+ INIT_LIST_HEAD(&i915->contexts.list);
- /* Keep the context ref so that we can free it immediately ourselves */
- ctx = i915_gem_context_get(fetch_and_zero(ctxp));
- GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
+ /* Using the simple ida interface, the max is limited by sizeof(int) */
+ BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
+ BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
+ ida_init(&i915->contexts.hw_ida);
+ INIT_LIST_HEAD(&i915->contexts.hw_id_list);
- context_close(ctx);
- i915_gem_context_free(ctx);
+ INIT_WORK(&i915->contexts.free_work, contexts_free_worker);
+ init_llist_head(&i915->contexts.free_list);
}
static bool needs_preempt_context(struct drm_i915_private *i915)
@@ -470,14 +535,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
if (ret)
return ret;
- INIT_LIST_HEAD(&dev_priv->contexts.list);
- INIT_WORK(&dev_priv->contexts.free_work, contexts_free_worker);
- init_llist_head(&dev_priv->contexts.free_list);
-
- /* Using the simple ida interface, the max is limited by sizeof(int) */
- BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
- BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
- ida_init(&dev_priv->contexts.hw_ida);
+ init_contexts(dev_priv);
/* lowest priority; idle task */
ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_MIN);
@@ -490,6 +548,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
* all user contexts will have non-zero hw_id.
*/
GEM_BUG_ON(ctx->hw_id);
+ GEM_BUG_ON(!atomic_read(&ctx->pin_hw_id));
dev_priv->kernel_context = ctx;
/* highest priority; preempting task */
@@ -527,6 +586,7 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
destroy_kernel_context(&i915->kernel_context);
/* Must free all deferred contexts (via flush_workqueue) first */
+ GEM_BUG_ON(!list_empty(&i915->contexts.hw_id_list));
ida_destroy(&i915->contexts.hw_ida);
}
@@ -932,6 +992,33 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
return ret;
}
+int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
+{
+ struct drm_i915_private *i915 = ctx->i915;
+ int err = 0;
+
+ mutex_lock(&i915->contexts.mutex);
+
+ GEM_BUG_ON(i915_gem_context_is_closed(ctx));
+
+ if (list_empty(&ctx->hw_id_link)) {
+ GEM_BUG_ON(atomic_read(&ctx->pin_hw_id));
+
+ err = assign_hw_id(i915, &ctx->hw_id);
+ if (err)
+ goto out_unlock;
+
+ list_add_tail(&ctx->hw_id_link, &i915->contexts.hw_id_list);
+ }
+
+ GEM_BUG_ON(atomic_read(&ctx->pin_hw_id) == ~0u);
+ atomic_inc(&ctx->pin_hw_id);
+
+out_unlock:
+ mutex_unlock(&i915->contexts.mutex);
+ return err;
+}
+
#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
#include "selftests/mock_context.c"
#include "selftests/i915_gem_context.c"
diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
index 851dad6decd7..c73ac614f58c 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/i915_gem_context.h
@@ -136,6 +136,8 @@ struct i915_gem_context {
* id for the lifetime of the context.
*/
unsigned int hw_id;
+ atomic_t pin_hw_id;
+ struct list_head hw_id_link;
/**
* @user_handle: userspace identifier
@@ -254,6 +256,21 @@ static inline void i915_gem_context_set_force_single_submission(struct i915_gem_
__set_bit(CONTEXT_FORCE_SINGLE_SUBMISSION, &ctx->flags);
}
+int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx);
+static inline int i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
+{
+ if (atomic_inc_not_zero(&ctx->pin_hw_id))
+ return 0;
+
+ return __i915_gem_context_pin_hw_id(ctx);
+}
+
+static inline void i915_gem_context_unpin_hw_id(struct i915_gem_context *ctx)
+{
+ GEM_BUG_ON(atomic_read(&ctx->pin_hw_id) == 0u);
+ atomic_dec(&ctx->pin_hw_id);
+}
+
static inline bool i915_gem_context_is_default(const struct i915_gem_context *c)
{
return c->user_handle == DEFAULT_CONTEXT_HANDLE;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index f8ceb9c99dd6..2ea3aea3e342 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1272,6 +1272,8 @@ static void execlists_context_destroy(struct intel_context *ce)
static void execlists_context_unpin(struct intel_context *ce)
{
+ i915_gem_context_unpin_hw_id(ce->gem_context);
+
intel_ring_unpin(ce->ring);
ce->state->obj->pin_global--;
@@ -1330,6 +1332,10 @@ __execlists_context_pin(struct intel_engine_cs *engine,
if (ret)
goto unpin_map;
+ ret = i915_gem_context_pin_hw_id(ctx);
+ if (ret)
+ goto unpin_ring;
+
intel_lr_context_descriptor_update(ctx, engine, ce);
ce->lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
@@ -1342,6 +1348,8 @@ __execlists_context_pin(struct intel_engine_cs *engine,
i915_gem_context_get(ctx);
return ce;
+unpin_ring:
+ intel_ring_unpin(ce->ring);
unpin_map:
i915_gem_object_unpin_map(ce->state->obj);
unpin_vma:
diff --git a/drivers/gpu/drm/i915/selftests/mock_context.c b/drivers/gpu/drm/i915/selftests/mock_context.c
index 8904f1ce64e3..d937bdff26f9 100644
--- a/drivers/gpu/drm/i915/selftests/mock_context.c
+++ b/drivers/gpu/drm/i915/selftests/mock_context.c
@@ -43,6 +43,7 @@ mock_context(struct drm_i915_private *i915,
INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
INIT_LIST_HEAD(&ctx->handles_list);
+ INIT_LIST_HEAD(&ctx->hw_id_link);
for (n = 0; n < ARRAY_SIZE(ctx->__engine); n++) {
struct intel_context *ce = &ctx->__engine[n];
@@ -50,11 +51,9 @@ mock_context(struct drm_i915_private *i915,
ce->gem_context = ctx;
}
- ret = ida_simple_get(&i915->contexts.hw_ida,
- 0, MAX_CONTEXT_HW_ID, GFP_KERNEL);
+ ret = i915_gem_context_pin_hw_id(ctx);
if (ret < 0)
goto err_handles;
- ctx->hw_id = ret;
if (name) {
ctx->name = kstrdup(name, GFP_KERNEL);
@@ -85,11 +84,7 @@ void mock_context_close(struct i915_gem_context *ctx)
void mock_init_contexts(struct drm_i915_private *i915)
{
- INIT_LIST_HEAD(&i915->contexts.list);
- ida_init(&i915->contexts.hw_ida);
-
- INIT_WORK(&i915->contexts.free_work, contexts_free_worker);
- init_llist_head(&i915->contexts.free_list);
+ init_contexts(i915);
}
struct i915_gem_context *
--
2.19.0.rc1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Reduce context HW ID lifetime (rev2)
2018-08-30 10:24 [PATCH] drm/i915: Reduce context HW ID lifetime Chris Wilson
@ 2018-08-30 12:38 ` Patchwork
2018-08-30 12:39 ` ✗ Fi.CI.SPARSE: " Patchwork
` (8 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2018-08-30 12:38 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Reduce context HW ID lifetime (rev2)
URL : https://patchwork.freedesktop.org/series/44134/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
30bb0eb3b07e drm/i915: Reduce context HW ID lifetime
-:50: CHECK:UNCOMMENTED_DEFINITION: struct mutex definition without comment
#50: FILE: drivers/gpu/drm/i915/i915_drv.h:1865:
+ struct mutex mutex;
total: 0 errors, 0 warnings, 1 checks, 408 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* ✗ Fi.CI.SPARSE: warning for drm/i915: Reduce context HW ID lifetime (rev2)
2018-08-30 10:24 [PATCH] drm/i915: Reduce context HW ID lifetime Chris Wilson
2018-08-30 12:38 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Reduce context HW ID lifetime (rev2) Patchwork
@ 2018-08-30 12:39 ` Patchwork
2018-08-30 12:58 ` ✓ Fi.CI.BAT: success " Patchwork
` (7 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2018-08-30 12:39 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Reduce context HW ID lifetime (rev2)
URL : https://patchwork.freedesktop.org/series/44134/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Commit: drm/i915: Reduce context HW ID lifetime
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3685:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3687:16: warning: expression using sizeof(void)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Reduce context HW ID lifetime (rev2)
2018-08-30 10:24 [PATCH] drm/i915: Reduce context HW ID lifetime Chris Wilson
2018-08-30 12:38 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Reduce context HW ID lifetime (rev2) Patchwork
2018-08-30 12:39 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2018-08-30 12:58 ` Patchwork
2018-08-30 16:23 ` [PATCH] drm/i915: Reduce context HW ID lifetime Tvrtko Ursulin
` (6 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2018-08-30 12:58 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Reduce context HW ID lifetime (rev2)
URL : https://patchwork.freedesktop.org/series/44134/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4739 -> Patchwork_10046 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/44134/revisions/2/mbox/
== Known issues ==
Here are the changes found in Patchwork_10046 that come from known issues:
=== IGT changes ===
==== Issues hit ====
{igt@amdgpu/amd_basic@userptr}:
{fi-kbl-8809g}: PASS -> INCOMPLETE (fdo#107402)
igt@kms_chamelium@dp-crc-fast:
fi-kbl-7500u: PASS -> DMESG-FAIL (fdo#103841)
igt@kms_frontbuffer_tracking@basic:
{fi-byt-clapper}: PASS -> FAIL (fdo#103167)
igt@kms_pipe_crc_basic@read-crc-pipe-b:
{fi-byt-clapper}: PASS -> FAIL (fdo#107362)
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
fi-bxt-dsi: PASS -> INCOMPLETE (fdo#103927)
==== Possible fixes ====
{igt@pm_rpm@module-reload}:
fi-cnl-psr: WARN (fdo#107602, fdo#107708) -> PASS
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
fdo#107402 https://bugs.freedesktop.org/show_bug.cgi?id=107402
fdo#107602 https://bugs.freedesktop.org/show_bug.cgi?id=107602
fdo#107708 https://bugs.freedesktop.org/show_bug.cgi?id=107708
== Participating hosts (54 -> 48) ==
Missing (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-skl-6700hq
== Build changes ==
* Linux: CI_DRM_4739 -> Patchwork_10046
CI_DRM_4739: f65e436af74d73b095b211d5294f5d7cd5132882 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4612: e39e09910fc8e369e24f6a0cabaeb9356dbfae08 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10046: 30bb0eb3b07e55636c05510321556c28fa14c9fd @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
30bb0eb3b07e drm/i915: Reduce context HW ID lifetime
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10046/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reduce context HW ID lifetime
2018-08-30 10:24 [PATCH] drm/i915: Reduce context HW ID lifetime Chris Wilson
` (2 preceding siblings ...)
2018-08-30 12:58 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-08-30 16:23 ` Tvrtko Ursulin
2018-08-31 12:36 ` Chris Wilson
2018-08-30 17:10 ` ✓ Fi.CI.IGT: success for drm/i915: Reduce context HW ID lifetime (rev2) Patchwork
` (5 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Tvrtko Ursulin @ 2018-08-30 16:23 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Oscar Mateo, Mika Kuoppala
On 30/08/2018 11:24, Chris Wilson wrote:
> Future gen reduce the number of bits we will have available to
> differentiate between contexts, so reduce the lifetime of the ID
> assignment from that of the context to its current active cycle (i.e.
> only while it is pinned for use by the HW, will it have a constant ID).
> This means that instead of a max of 2k allocated contexts (worst case
> before fun with bit twiddling), we instead have a limit of 2k in flight
> contexts (minus a few that have been pinned by the kernel or by perf).
We need a paragraph outlining the implementation.
>
> We cannot reduce the scope of an HW-ID to an engine (allowing the same
> gem_context to have different ids on each engine) as in the future we
> will need to preassign an id before we know which engine the
> context is being executed on.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
You can drop Oscar since he left Intel.
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
> drivers/gpu/drm/i915/i915_drv.h | 2 +
> drivers/gpu/drm/i915/i915_gem_context.c | 207 +++++++++++++-----
> drivers/gpu/drm/i915/i915_gem_context.h | 17 ++
> drivers/gpu/drm/i915/intel_lrc.c | 8 +
> drivers/gpu/drm/i915/selftests/mock_context.c | 11 +-
> 6 files changed, 181 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index a5265c236a33..bf3b6c6db51d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1953,7 +1953,10 @@ static int i915_context_status(struct seq_file *m, void *unused)
> return ret;
>
> list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
> - seq_printf(m, "HW context %u ", ctx->hw_id);
> + seq_puts(m, "HW context ");
> + if (!list_empty(&ctx->hw_id_link))
> + seq_printf(m, "%x [pin %u]",
> + ctx->hw_id, atomic_read(&ctx->pin_hw_id));
> if (ctx->pid) {
> struct task_struct *task;
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e5b9d3c77139..ca6b55d29d6a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1862,6 +1862,7 @@ struct drm_i915_private {
> struct mutex av_mutex;
>
> struct {
> + struct mutex mutex;
> struct list_head list;
> struct llist_head free_list;
> struct work_struct free_work;
> @@ -1874,6 +1875,7 @@ struct drm_i915_private {
> #define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */
> #define MAX_GUC_CONTEXT_HW_ID (1 << 20) /* exclusive */
> #define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */
> + struct list_head hw_id_list;
> } contexts;
>
> u32 fdi_rx_config;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index f15a039772db..d3390942f37b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -115,6 +115,85 @@ static void lut_close(struct i915_gem_context *ctx)
> rcu_read_unlock();
> }
>
> +static inline int new_hw_id(struct drm_i915_private *i915, gfp_t gfp)
> +{
> + unsigned int max;
> +
> + lockdep_assert_held(&i915->contexts.mutex);
> +
> + if (INTEL_GEN(i915) >= 11)
> + max = GEN11_MAX_CONTEXT_HW_ID;
> + else if (USES_GUC_SUBMISSION(i915))
> + /*
> + * When using GuC in proxy submission, GuC consumes the
> + * highest bit in the context id to indicate proxy submission.
> + */
> + max = MAX_GUC_CONTEXT_HW_ID;
> + else
> + max = MAX_CONTEXT_HW_ID;
> +
> + return ida_simple_get(&i915->contexts.hw_ida, 0, max, gfp);
> +}
> +
> +static int steal_hw_id(struct drm_i915_private *i915)
> +{
> + struct i915_gem_context *ctx, *cn;
> + LIST_HEAD(pinned);
> + int id = -ENOSPC;
> +
> + lockdep_assert_held(&i915->contexts.mutex);
> +
> + list_for_each_entry_safe(ctx, cn,
> + &i915->contexts.hw_id_list, hw_id_link) {
> + if (atomic_read(&ctx->pin_hw_id)) {
> + list_move_tail(&ctx->hw_id_link, &pinned);
> + continue;
> + }
> +
> + GEM_BUG_ON(!ctx->hw_id); /* perma-pinned kernel context */
> + list_del_init(&ctx->hw_id_link);
> + id = ctx->hw_id;
> + break;
> + }
> +
> + list_splice_tail(&pinned, &i915->contexts.hw_id_list);
Put a comment what is this code doing please. Trying to create some sort
of LRU order?
> + return id;
> +}
> +
> +static int assign_hw_id(struct drm_i915_private *i915, unsigned int *out)
> +{
> + int ret;
> +
> + lockdep_assert_held(&i915->contexts.mutex);
> +
> + ret = new_hw_id(i915, GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
> + if (unlikely(ret < 0)) {
> + ret = steal_hw_id(i915);
> + if (ret < 0) /* once again for the correct erro code */
errno
> + ret = new_hw_id(i915, GFP_KERNEL);
Hmm.. shouldn't you try GFP_KERNEL before attempting to steal? Actually
I think you should branch based on -ENOSPC (steal) vs -ENOMEM (retry
with GFP_KERNEL). Which would actually mean something like:
flags = may fail;
func = new_hw_id
retry:
ret = func(flags);
if (ret == -ENOMEM && flags != GFP_KERNEL) {
flags = GFP_KERNEL;
goto retry;
} else if (ret == -ENOSPC && func != steal_hw_id) {
func = steal_hw_id;
goto retry;
} else {
no can do
}
> + if (ret < 0)
> + return ret;
> + }
> +
> + *out = ret;
> + return 0;
> +}
> +
> +static void release_hw_id(struct i915_gem_context *ctx)
> +{
> + struct drm_i915_private *i915 = ctx->i915;
> +
> + if (list_empty(&ctx->hw_id_link))
> + return;
> +
> + mutex_lock(&i915->contexts.mutex);
> + if (!list_empty(&ctx->hw_id_link)) {
> + ida_simple_remove(&i915->contexts.hw_ida, ctx->hw_id);
> + list_del_init(&ctx->hw_id_link);
> + }
> + mutex_unlock(&i915->contexts.mutex);
> +}
> +
> static void i915_gem_context_free(struct i915_gem_context *ctx)
> {
> unsigned int n;
> @@ -122,6 +201,7 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
> lockdep_assert_held(&ctx->i915->drm.struct_mutex);
> GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
>
> + release_hw_id(ctx);
> i915_ppgtt_put(ctx->ppgtt);
>
> for (n = 0; n < ARRAY_SIZE(ctx->__engine); n++) {
> @@ -136,7 +216,6 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
>
> list_del(&ctx->link);
>
> - ida_simple_remove(&ctx->i915->contexts.hw_ida, ctx->hw_id);
> kfree_rcu(ctx, rcu);
> }
>
> @@ -190,6 +269,12 @@ static void context_close(struct i915_gem_context *ctx)
> {
> i915_gem_context_set_closed(ctx);
>
> + /*
> + * This context will never again be assinged to HW, so we can
> + * reuse its ID for the next context.
> + */
> + release_hw_id(ctx);
> +
> /*
> * The LUT uses the VMA as a backpointer to unref the object,
> * so we need to clear the LUT before we close all the VMA (inside
> @@ -203,43 +288,6 @@ static void context_close(struct i915_gem_context *ctx)
> i915_gem_context_put(ctx);
> }
>
> -static int assign_hw_id(struct drm_i915_private *dev_priv, unsigned *out)
> -{
> - int ret;
> - unsigned int max;
> -
> - if (INTEL_GEN(dev_priv) >= 11) {
> - max = GEN11_MAX_CONTEXT_HW_ID;
> - } else {
> - /*
> - * When using GuC in proxy submission, GuC consumes the
> - * highest bit in the context id to indicate proxy submission.
> - */
> - if (USES_GUC_SUBMISSION(dev_priv))
> - max = MAX_GUC_CONTEXT_HW_ID;
> - else
> - max = MAX_CONTEXT_HW_ID;
> - }
> -
> -
> - ret = ida_simple_get(&dev_priv->contexts.hw_ida,
> - 0, max, GFP_KERNEL);
Although now that I see this I am struggling not to say the change to
try a lighter weight allocation strategy first (gfp may fail) needs to
be split out to a separate patch.
> - if (ret < 0) {
> - /* Contexts are only released when no longer active.
> - * Flush any pending retires to hopefully release some
> - * stale contexts and try again.
> - */
> - i915_retire_requests(dev_priv);
> - ret = ida_simple_get(&dev_priv->contexts.hw_ida,
> - 0, max, GFP_KERNEL);
> - if (ret < 0)
> - return ret;
> - }
> -
> - *out = ret;
> - return 0;
> -}
> -
> static u32 default_desc_template(const struct drm_i915_private *i915,
> const struct i915_hw_ppgtt *ppgtt)
> {
> @@ -276,12 +324,6 @@ __create_hw_context(struct drm_i915_private *dev_priv,
> if (ctx == NULL)
> return ERR_PTR(-ENOMEM);
>
> - ret = assign_hw_id(dev_priv, &ctx->hw_id);
> - if (ret) {
> - kfree(ctx);
> - return ERR_PTR(ret);
> - }
> -
> kref_init(&ctx->ref);
> list_add_tail(&ctx->link, &dev_priv->contexts.list);
> ctx->i915 = dev_priv;
> @@ -295,6 +337,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
>
> INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
> INIT_LIST_HEAD(&ctx->handles_list);
> + INIT_LIST_HEAD(&ctx->hw_id_link);
>
> /* Default context will never have a file_priv */
> ret = DEFAULT_CONTEXT_HANDLE;
> @@ -421,15 +464,35 @@ i915_gem_context_create_gvt(struct drm_device *dev)
> return ctx;
> }
>
> +static void
> +destroy_kernel_context(struct i915_gem_context **ctxp)
> +{
> + struct i915_gem_context *ctx;
> +
> + /* Keep the context ref so that we can free it immediately ourselves */
> + ctx = i915_gem_context_get(fetch_and_zero(ctxp));
> + GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> +
> + context_close(ctx);
> + i915_gem_context_free(ctx);
> +}
> +
> struct i915_gem_context *
> i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
> {
> struct i915_gem_context *ctx;
> + int err;
>
> ctx = i915_gem_create_context(i915, NULL);
> if (IS_ERR(ctx))
> return ctx;
>
> + err = i915_gem_context_pin_hw_id(ctx);
> + if (err) {
> + destroy_kernel_context(&ctx);
> + return ERR_PTR(err);
> + }
> +
> i915_gem_context_clear_bannable(ctx);
> ctx->sched.priority = prio;
> ctx->ring_size = PAGE_SIZE;
> @@ -439,17 +502,19 @@ i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
> return ctx;
> }
>
> -static void
> -destroy_kernel_context(struct i915_gem_context **ctxp)
> +static void init_contexts(struct drm_i915_private *i915)
> {
> - struct i915_gem_context *ctx;
> + mutex_init(&i915->contexts.mutex);
> + INIT_LIST_HEAD(&i915->contexts.list);
>
> - /* Keep the context ref so that we can free it immediately ourselves */
> - ctx = i915_gem_context_get(fetch_and_zero(ctxp));
> - GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> + /* Using the simple ida interface, the max is limited by sizeof(int) */
> + BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
> + BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
> + ida_init(&i915->contexts.hw_ida);
> + INIT_LIST_HEAD(&i915->contexts.hw_id_list);
>
> - context_close(ctx);
> - i915_gem_context_free(ctx);
> + INIT_WORK(&i915->contexts.free_work, contexts_free_worker);
> + init_llist_head(&i915->contexts.free_list);
ugh diff.. :) looks like pure movement from perspective of
destroy_kernel_context.
> }
>
> static bool needs_preempt_context(struct drm_i915_private *i915)
> @@ -470,14 +535,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> if (ret)
> return ret;
>
> - INIT_LIST_HEAD(&dev_priv->contexts.list);
> - INIT_WORK(&dev_priv->contexts.free_work, contexts_free_worker);
> - init_llist_head(&dev_priv->contexts.free_list);
> -
> - /* Using the simple ida interface, the max is limited by sizeof(int) */
> - BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
> - BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
> - ida_init(&dev_priv->contexts.hw_ida);
> + init_contexts(dev_priv);
>
> /* lowest priority; idle task */
> ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_MIN);
> @@ -490,6 +548,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> * all user contexts will have non-zero hw_id.
> */
> GEM_BUG_ON(ctx->hw_id);
> + GEM_BUG_ON(!atomic_read(&ctx->pin_hw_id));
/* Kernel context is perma-pinned */
> dev_priv->kernel_context = ctx;
>
> /* highest priority; preempting task */
> @@ -527,6 +586,7 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
> destroy_kernel_context(&i915->kernel_context);
>
> /* Must free all deferred contexts (via flush_workqueue) first */
> + GEM_BUG_ON(!list_empty(&i915->contexts.hw_id_list));
> ida_destroy(&i915->contexts.hw_ida);
> }
>
> @@ -932,6 +992,33 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
> return ret;
> }
>
> +int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
> +{
> + struct drm_i915_private *i915 = ctx->i915;
> + int err = 0;
> +
> + mutex_lock(&i915->contexts.mutex);
> +
> + GEM_BUG_ON(i915_gem_context_is_closed(ctx));
> +
> + if (list_empty(&ctx->hw_id_link)) {
> + GEM_BUG_ON(atomic_read(&ctx->pin_hw_id));
> +
> + err = assign_hw_id(i915, &ctx->hw_id);
> + if (err)
> + goto out_unlock;
> +
> + list_add_tail(&ctx->hw_id_link, &i915->contexts.hw_id_list);
> + }
> +
> + GEM_BUG_ON(atomic_read(&ctx->pin_hw_id) == ~0u);
> + atomic_inc(&ctx->pin_hw_id);
> +
> +out_unlock:
> + mutex_unlock(&i915->contexts.mutex);
> + return err;
> +}
> +
> #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> #include "selftests/mock_context.c"
> #include "selftests/i915_gem_context.c"
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> index 851dad6decd7..c73ac614f58c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> @@ -136,6 +136,8 @@ struct i915_gem_context {
> * id for the lifetime of the context.
> */
> unsigned int hw_id;
> + atomic_t pin_hw_id;
I think now we need short comments describing the difference between the
two.
> + struct list_head hw_id_link;
And for this one.
>
> /**
> * @user_handle: userspace identifier
> @@ -254,6 +256,21 @@ static inline void i915_gem_context_set_force_single_submission(struct i915_gem_
> __set_bit(CONTEXT_FORCE_SINGLE_SUBMISSION, &ctx->flags);
> }
>
> +int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx);
> +static inline int i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
> +{
> + if (atomic_inc_not_zero(&ctx->pin_hw_id))
> + return 0;
> +
> + return __i915_gem_context_pin_hw_id(ctx);
> +}
> +
> +static inline void i915_gem_context_unpin_hw_id(struct i915_gem_context *ctx)
> +{
> + GEM_BUG_ON(atomic_read(&ctx->pin_hw_id) == 0u);
> + atomic_dec(&ctx->pin_hw_id);
> +}
> +
> static inline bool i915_gem_context_is_default(const struct i915_gem_context *c)
> {
> return c->user_handle == DEFAULT_CONTEXT_HANDLE;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index f8ceb9c99dd6..2ea3aea3e342 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1272,6 +1272,8 @@ static void execlists_context_destroy(struct intel_context *ce)
>
> static void execlists_context_unpin(struct intel_context *ce)
> {
> + i915_gem_context_unpin_hw_id(ce->gem_context);
> +
> intel_ring_unpin(ce->ring);
>
> ce->state->obj->pin_global--;
> @@ -1330,6 +1332,10 @@ __execlists_context_pin(struct intel_engine_cs *engine,
> if (ret)
> goto unpin_map;
>
> + ret = i915_gem_context_pin_hw_id(ctx);
> + if (ret)
> + goto unpin_ring;
> +
> intel_lr_context_descriptor_update(ctx, engine, ce);
>
> ce->lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
> @@ -1342,6 +1348,8 @@ __execlists_context_pin(struct intel_engine_cs *engine,
> i915_gem_context_get(ctx);
> return ce;
>
> +unpin_ring:
> + intel_ring_unpin(ce->ring);
> unpin_map:
> i915_gem_object_unpin_map(ce->state->obj);
> unpin_vma:
> diff --git a/drivers/gpu/drm/i915/selftests/mock_context.c b/drivers/gpu/drm/i915/selftests/mock_context.c
> index 8904f1ce64e3..d937bdff26f9 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_context.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_context.c
> @@ -43,6 +43,7 @@ mock_context(struct drm_i915_private *i915,
>
> INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
> INIT_LIST_HEAD(&ctx->handles_list);
> + INIT_LIST_HEAD(&ctx->hw_id_link);
>
> for (n = 0; n < ARRAY_SIZE(ctx->__engine); n++) {
> struct intel_context *ce = &ctx->__engine[n];
> @@ -50,11 +51,9 @@ mock_context(struct drm_i915_private *i915,
> ce->gem_context = ctx;
> }
>
> - ret = ida_simple_get(&i915->contexts.hw_ida,
> - 0, MAX_CONTEXT_HW_ID, GFP_KERNEL);
> + ret = i915_gem_context_pin_hw_id(ctx);
> if (ret < 0)
> goto err_handles;
> - ctx->hw_id = ret;
>
> if (name) {
> ctx->name = kstrdup(name, GFP_KERNEL);
> @@ -85,11 +84,7 @@ void mock_context_close(struct i915_gem_context *ctx)
>
> void mock_init_contexts(struct drm_i915_private *i915)
> {
> - INIT_LIST_HEAD(&i915->contexts.list);
> - ida_init(&i915->contexts.hw_ida);
> -
> - INIT_WORK(&i915->contexts.free_work, contexts_free_worker);
> - init_llist_head(&i915->contexts.free_list);
> + init_contexts(i915);
> }
>
> struct i915_gem_context *
>
So in essence there will be a little bit more cost when pinning in the
normal case, or a bit bit more in the stealing/pathological case, but as
long as we stay below over-subscription the cost is only on first pin.
No complaints there. Debug also won't be confusing in the normal case
since numbers will be stable.
Does it have any negative connotations in the world of OA is the
question for Lionel?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Reduce context HW ID lifetime (rev2)
2018-08-30 10:24 [PATCH] drm/i915: Reduce context HW ID lifetime Chris Wilson
` (3 preceding siblings ...)
2018-08-30 16:23 ` [PATCH] drm/i915: Reduce context HW ID lifetime Tvrtko Ursulin
@ 2018-08-30 17:10 ` Patchwork
2018-09-04 15:31 ` [PATCH v2] drm/i915: Reduce context HW ID lifetime Chris Wilson
` (4 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2018-08-30 17:10 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Reduce context HW ID lifetime (rev2)
URL : https://patchwork.freedesktop.org/series/44134/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4739_full -> Patchwork_10046_full =
== Summary - SUCCESS ==
No regressions found.
== Known issues ==
Here are the changes found in Patchwork_10046_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@drv_suspend@shrink:
shard-hsw: PASS -> INCOMPLETE (fdo#106886, fdo#103540)
shard-glk: PASS -> FAIL (fdo#106886)
igt@gem_ppgtt@blt-vs-render-ctx0:
shard-kbl: PASS -> INCOMPLETE (fdo#106023, fdo#103665)
igt@kms_flip@flip-vs-expired-vblank:
shard-glk: PASS -> FAIL (fdo#105363)
igt@prime_vgem@basic-sync-default:
shard-apl: PASS -> DMESG-WARN (fdo#105602, fdo#103558) +1
==== Possible fixes ====
igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
shard-hsw: FAIL (fdo#105767) -> PASS
igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-gtt:
shard-glk: FAIL (fdo#103167) -> PASS
igt@kms_pipe_crc_basic@read-crc-pipe-c:
shard-hsw: DMESG-WARN (fdo#102614) -> PASS +1
igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
shard-apl: FAIL (fdo#103375) -> PASS
igt@kms_setmode@basic:
shard-apl: FAIL (fdo#99912) -> PASS
shard-kbl: FAIL (fdo#99912) -> PASS
fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
== Participating hosts (5 -> 5) ==
No changes in participating hosts
== Build changes ==
* Linux: CI_DRM_4739 -> Patchwork_10046
CI_DRM_4739: f65e436af74d73b095b211d5294f5d7cd5132882 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4612: e39e09910fc8e369e24f6a0cabaeb9356dbfae08 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10046: 30bb0eb3b07e55636c05510321556c28fa14c9fd @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10046/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reduce context HW ID lifetime
2018-08-30 16:23 ` [PATCH] drm/i915: Reduce context HW ID lifetime Tvrtko Ursulin
@ 2018-08-31 12:36 ` Chris Wilson
2018-09-03 9:59 ` Tvrtko Ursulin
0 siblings, 1 reply; 19+ messages in thread
From: Chris Wilson @ 2018-08-31 12:36 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx; +Cc: Oscar Mateo, Mika Kuoppala
Quoting Tvrtko Ursulin (2018-08-30 17:23:43)
>
> On 30/08/2018 11:24, Chris Wilson wrote:
> > +static int steal_hw_id(struct drm_i915_private *i915)
> > +{
> > + struct i915_gem_context *ctx, *cn;
> > + LIST_HEAD(pinned);
> > + int id = -ENOSPC;
> > +
> > + lockdep_assert_held(&i915->contexts.mutex);
> > +
> > + list_for_each_entry_safe(ctx, cn,
> > + &i915->contexts.hw_id_list, hw_id_link) {
> > + if (atomic_read(&ctx->pin_hw_id)) {
> > + list_move_tail(&ctx->hw_id_link, &pinned);
> > + continue;
> > + }
> > +
> > + GEM_BUG_ON(!ctx->hw_id); /* perma-pinned kernel context */
> > + list_del_init(&ctx->hw_id_link);
> > + id = ctx->hw_id;
> > + break;
> > + }
> > +
> > + list_splice_tail(&pinned, &i915->contexts.hw_id_list);
>
> Put a comment what is this code doing please. Trying to create some sort
> of LRU order?
LRSearched. Same as the shrinker, and eviction code if you would also
review that ;)
>
> > + return id;
> > +}
> > +
> > +static int assign_hw_id(struct drm_i915_private *i915, unsigned int *out)
> > +{
> > + int ret;
> > +
> > + lockdep_assert_held(&i915->contexts.mutex);
> > +
> > + ret = new_hw_id(i915, GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
> > + if (unlikely(ret < 0)) {
> > + ret = steal_hw_id(i915);
> > + if (ret < 0) /* once again for the correct erro code */
>
> errno
>
> > + ret = new_hw_id(i915, GFP_KERNEL);
>
> Hmm.. shouldn't you try GFP_KERNEL before attempting to steal? Actually
> I think you should branch based on -ENOSPC (steal) vs -ENOMEM (retry
> with GFP_KERNEL). Which would actually mean something like:
I was applying the same strategy as we use elsewhere. Penalise any
driver cache before hitting reclaim.
I think that is fair from an application of soft backpressure point of
view. (Lack of backpressure is probably a sore point for many.)
> > - ret = ida_simple_get(&dev_priv->contexts.hw_ida,
> > - 0, max, GFP_KERNEL);
>
> Although now that I see this I am struggling not to say the change to
> try a lighter weight allocation strategy first (gfp may fail) needs to
> be split out to a separate patch.
Pardon? I appear to suddenly be hard of hearing.
The patch was all about the steal_hw_id().
> > - if (ret < 0) {
> > - /* Contexts are only released when no longer active.
> > - * Flush any pending retires to hopefully release some
> > - * stale contexts and try again.
> > - */
> > - i915_retire_requests(dev_priv);
> > - ret = ida_simple_get(&dev_priv->contexts.hw_ida,
> > - 0, max, GFP_KERNEL);
> > - if (ret < 0)
> > - return ret;
> > - }
> > -
> > - *out = ret;
> > - return 0;
> > -}
> > -
> > static u32 default_desc_template(const struct drm_i915_private *i915,
> > const struct i915_hw_ppgtt *ppgtt)
> > {
> > @@ -276,12 +324,6 @@ __create_hw_context(struct drm_i915_private *dev_priv,
> > if (ctx == NULL)
> > return ERR_PTR(-ENOMEM);
> >
> > - ret = assign_hw_id(dev_priv, &ctx->hw_id);
> > - if (ret) {
> > - kfree(ctx);
> > - return ERR_PTR(ret);
> > - }
> > -
> > kref_init(&ctx->ref);
> > list_add_tail(&ctx->link, &dev_priv->contexts.list);
> > ctx->i915 = dev_priv;
> > @@ -295,6 +337,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
> >
> > INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
> > INIT_LIST_HEAD(&ctx->handles_list);
> > + INIT_LIST_HEAD(&ctx->hw_id_link);
> >
> > /* Default context will never have a file_priv */
> > ret = DEFAULT_CONTEXT_HANDLE;
> > @@ -421,15 +464,35 @@ i915_gem_context_create_gvt(struct drm_device *dev)
> > return ctx;
> > }
> >
> > +static void
> > +destroy_kernel_context(struct i915_gem_context **ctxp)
> > +{
> > + struct i915_gem_context *ctx;
> > +
> > + /* Keep the context ref so that we can free it immediately ourselves */
> > + ctx = i915_gem_context_get(fetch_and_zero(ctxp));
> > + GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> > +
> > + context_close(ctx);
> > + i915_gem_context_free(ctx);
> > +}
> > +
> > struct i915_gem_context *
> > i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
> > {
> > struct i915_gem_context *ctx;
> > + int err;
> >
> > ctx = i915_gem_create_context(i915, NULL);
> > if (IS_ERR(ctx))
> > return ctx;
> >
> > + err = i915_gem_context_pin_hw_id(ctx);
> > + if (err) {
> > + destroy_kernel_context(&ctx);
> > + return ERR_PTR(err);
> > + }
> > +
> > i915_gem_context_clear_bannable(ctx);
> > ctx->sched.priority = prio;
> > ctx->ring_size = PAGE_SIZE;
> > @@ -439,17 +502,19 @@ i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
> > return ctx;
> > }
> >
> > -static void
> > -destroy_kernel_context(struct i915_gem_context **ctxp)
> > +static void init_contexts(struct drm_i915_private *i915)
> > {
> > - struct i915_gem_context *ctx;
> > + mutex_init(&i915->contexts.mutex);
> > + INIT_LIST_HEAD(&i915->contexts.list);
> >
> > - /* Keep the context ref so that we can free it immediately ourselves */
> > - ctx = i915_gem_context_get(fetch_and_zero(ctxp));
> > - GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> > + /* Using the simple ida interface, the max is limited by sizeof(int) */
> > + BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
> > + BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
> > + ida_init(&i915->contexts.hw_ida);
> > + INIT_LIST_HEAD(&i915->contexts.hw_id_list);
> >
> > - context_close(ctx);
> > - i915_gem_context_free(ctx);
> > + INIT_WORK(&i915->contexts.free_work, contexts_free_worker);
> > + init_llist_head(&i915->contexts.free_list);
>
> ugh diff.. :) looks like pure movement from perspective of
> destroy_kernel_context.
>
> > }
> >
> > static bool needs_preempt_context(struct drm_i915_private *i915)
> > @@ -470,14 +535,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> > if (ret)
> > return ret;
> >
> > - INIT_LIST_HEAD(&dev_priv->contexts.list);
> > - INIT_WORK(&dev_priv->contexts.free_work, contexts_free_worker);
> > - init_llist_head(&dev_priv->contexts.free_list);
> > -
> > - /* Using the simple ida interface, the max is limited by sizeof(int) */
> > - BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
> > - BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
> > - ida_init(&dev_priv->contexts.hw_ida);
> > + init_contexts(dev_priv);
> >
> > /* lowest priority; idle task */
> > ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_MIN);
> > @@ -490,6 +548,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> > * all user contexts will have non-zero hw_id.
> > */
> > GEM_BUG_ON(ctx->hw_id);
> > + GEM_BUG_ON(!atomic_read(&ctx->pin_hw_id));
>
> /* Kernel context is perma-pinned */
>
> > dev_priv->kernel_context = ctx;
> >
> > /* highest priority; preempting task */
> > @@ -527,6 +586,7 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
> > destroy_kernel_context(&i915->kernel_context);
> >
> > /* Must free all deferred contexts (via flush_workqueue) first */
> > + GEM_BUG_ON(!list_empty(&i915->contexts.hw_id_list));
> > ida_destroy(&i915->contexts.hw_ida);
> > }
> >
> > @@ -932,6 +992,33 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
> > return ret;
> > }
> >
> > +int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
> > +{
> > + struct drm_i915_private *i915 = ctx->i915;
> > + int err = 0;
> > +
> > + mutex_lock(&i915->contexts.mutex);
> > +
> > + GEM_BUG_ON(i915_gem_context_is_closed(ctx));
> > +
> > + if (list_empty(&ctx->hw_id_link)) {
> > + GEM_BUG_ON(atomic_read(&ctx->pin_hw_id));
> > +
> > + err = assign_hw_id(i915, &ctx->hw_id);
> > + if (err)
> > + goto out_unlock;
> > +
> > + list_add_tail(&ctx->hw_id_link, &i915->contexts.hw_id_list);
> > + }
> > +
> > + GEM_BUG_ON(atomic_read(&ctx->pin_hw_id) == ~0u);
> > + atomic_inc(&ctx->pin_hw_id);
> > +
> > +out_unlock:
> > + mutex_unlock(&i915->contexts.mutex);
> > + return err;
> > +}
> > +
> > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> > #include "selftests/mock_context.c"
> > #include "selftests/i915_gem_context.c"
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> > index 851dad6decd7..c73ac614f58c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.h
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> > @@ -136,6 +136,8 @@ struct i915_gem_context {
> > * id for the lifetime of the context.
> > */
> > unsigned int hw_id;
> > + atomic_t pin_hw_id;
>
> I think now we need short comments describing the difference between the
> two.
One is 32bits unsigned, unserialised. The other is 32bits signed, and
very loosely serialised :)
> > + struct list_head hw_id_link;
>
> And for this one.
[snip]
> So in essence there will be a little bit more cost when pinning in the
> normal case, or a bit bit more in the stealing/pathological case, but as
> long as we stay below over-subscription the cost is only on first pin.
> No complaints there. Debug also won't be confusing in the normal case
> since numbers will be stable.
Yup. Nice addition to the changelog, thanks.
> Does it have any negative connotations in the world of OA is the
> question for Lionel?
Lionel kept promising me this was ok, that he/gputop was quite ready for
shorter lived ctx id, and reuse.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reduce context HW ID lifetime
2018-08-31 12:36 ` Chris Wilson
@ 2018-09-03 9:59 ` Tvrtko Ursulin
2018-09-04 13:48 ` Chris Wilson
0 siblings, 1 reply; 19+ messages in thread
From: Tvrtko Ursulin @ 2018-09-03 9:59 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Oscar Mateo, Mika Kuoppala
On 31/08/2018 13:36, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-08-30 17:23:43)
>>
>> On 30/08/2018 11:24, Chris Wilson wrote:
>>> +static int steal_hw_id(struct drm_i915_private *i915)
>>> +{
>>> + struct i915_gem_context *ctx, *cn;
>>> + LIST_HEAD(pinned);
>>> + int id = -ENOSPC;
>>> +
>>> + lockdep_assert_held(&i915->contexts.mutex);
>>> +
>>> + list_for_each_entry_safe(ctx, cn,
>>> + &i915->contexts.hw_id_list, hw_id_link) {
>>> + if (atomic_read(&ctx->pin_hw_id)) {
>>> + list_move_tail(&ctx->hw_id_link, &pinned);
>>> + continue;
>>> + }
>>> +
>>> + GEM_BUG_ON(!ctx->hw_id); /* perma-pinned kernel context */
>>> + list_del_init(&ctx->hw_id_link);
>>> + id = ctx->hw_id;
>>> + break;
>>> + }
>>> +
>>> + list_splice_tail(&pinned, &i915->contexts.hw_id_list);
>>
>> Put a comment what is this code doing please. Trying to create some sort
>> of LRU order?
>
> LRSearched. Same as the shrinker, and eviction code if you would also
> review that ;)
Two things are infinite, the universe and your stream of patches! :)
>>
>>> + return id;
>>> +}
>>> +
>>> +static int assign_hw_id(struct drm_i915_private *i915, unsigned int *out)
>>> +{
>>> + int ret;
>>> +
>>> + lockdep_assert_held(&i915->contexts.mutex);
>>> +
>>> + ret = new_hw_id(i915, GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
>>> + if (unlikely(ret < 0)) {
>>> + ret = steal_hw_id(i915);
>>> + if (ret < 0) /* once again for the correct erro code */
>>
>> errno
>>
>>> + ret = new_hw_id(i915, GFP_KERNEL);
>>
>> Hmm.. shouldn't you try GFP_KERNEL before attempting to steal? Actually
>> I think you should branch based on -ENOSPC (steal) vs -ENOMEM (retry
>> with GFP_KERNEL). Which would actually mean something like:
>
> I was applying the same strategy as we use elsewhere. Penalise any
> driver cache before hitting reclaim.
>
> I think that is fair from an application of soft backpressure point of
> view. (Lack of backpressure is probably a sore point for many.)
My concern was lack of a phase which avoids hw id stealing for loads
with few contexts but heavy memory pressure. Sounded like a thing worth
"robustifying" against - you don't think so?
>
>>> - ret = ida_simple_get(&dev_priv->contexts.hw_ida,
>>> - 0, max, GFP_KERNEL);
>>
>> Although now that I see this I am struggling not to say the change to
>> try a lighter weight allocation strategy first (gfp may fail) needs to
>> be split out to a separate patch.
>
> Pardon? I appear to suddenly be hard of hearing.
>
> The patch was all about the steal_hw_id().
Yes, but you could't have kept the GFP_KERNEL ida_simple_get and only
then fall back to stealing. Or as I said, GFP_MAYFAIL, then GFP_KERNEL,
then steal.
>
>>> - if (ret < 0) {
>>> - /* Contexts are only released when no longer active.
>>> - * Flush any pending retires to hopefully release some
>>> - * stale contexts and try again.
>>> - */
>>> - i915_retire_requests(dev_priv);
>>> - ret = ida_simple_get(&dev_priv->contexts.hw_ida,
>>> - 0, max, GFP_KERNEL);
>>> - if (ret < 0)
>>> - return ret;
>>> - }
>>> -
>>> - *out = ret;
>>> - return 0;
>>> -}
>>> -
>>> static u32 default_desc_template(const struct drm_i915_private *i915,
>>> const struct i915_hw_ppgtt *ppgtt)
>>> {
>>> @@ -276,12 +324,6 @@ __create_hw_context(struct drm_i915_private *dev_priv,
>>> if (ctx == NULL)
>>> return ERR_PTR(-ENOMEM);
>>>
>>> - ret = assign_hw_id(dev_priv, &ctx->hw_id);
>>> - if (ret) {
>>> - kfree(ctx);
>>> - return ERR_PTR(ret);
>>> - }
>>> -
>>> kref_init(&ctx->ref);
>>> list_add_tail(&ctx->link, &dev_priv->contexts.list);
>>> ctx->i915 = dev_priv;
>>> @@ -295,6 +337,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
>>>
>>> INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
>>> INIT_LIST_HEAD(&ctx->handles_list);
>>> + INIT_LIST_HEAD(&ctx->hw_id_link);
>>>
>>> /* Default context will never have a file_priv */
>>> ret = DEFAULT_CONTEXT_HANDLE;
>>> @@ -421,15 +464,35 @@ i915_gem_context_create_gvt(struct drm_device *dev)
>>> return ctx;
>>> }
>>>
>>> +static void
>>> +destroy_kernel_context(struct i915_gem_context **ctxp)
>>> +{
>>> + struct i915_gem_context *ctx;
>>> +
>>> + /* Keep the context ref so that we can free it immediately ourselves */
>>> + ctx = i915_gem_context_get(fetch_and_zero(ctxp));
>>> + GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
>>> +
>>> + context_close(ctx);
>>> + i915_gem_context_free(ctx);
>>> +}
>>> +
>>> struct i915_gem_context *
>>> i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
>>> {
>>> struct i915_gem_context *ctx;
>>> + int err;
>>>
>>> ctx = i915_gem_create_context(i915, NULL);
>>> if (IS_ERR(ctx))
>>> return ctx;
>>>
>>> + err = i915_gem_context_pin_hw_id(ctx);
>>> + if (err) {
>>> + destroy_kernel_context(&ctx);
>>> + return ERR_PTR(err);
>>> + }
>>> +
>>> i915_gem_context_clear_bannable(ctx);
>>> ctx->sched.priority = prio;
>>> ctx->ring_size = PAGE_SIZE;
>>> @@ -439,17 +502,19 @@ i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
>>> return ctx;
>>> }
>>>
>>> -static void
>>> -destroy_kernel_context(struct i915_gem_context **ctxp)
>>> +static void init_contexts(struct drm_i915_private *i915)
>>> {
>>> - struct i915_gem_context *ctx;
>>> + mutex_init(&i915->contexts.mutex);
>>> + INIT_LIST_HEAD(&i915->contexts.list);
>>>
>>> - /* Keep the context ref so that we can free it immediately ourselves */
>>> - ctx = i915_gem_context_get(fetch_and_zero(ctxp));
>>> - GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
>>> + /* Using the simple ida interface, the max is limited by sizeof(int) */
>>> + BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
>>> + BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
>>> + ida_init(&i915->contexts.hw_ida);
>>> + INIT_LIST_HEAD(&i915->contexts.hw_id_list);
>>>
>>> - context_close(ctx);
>>> - i915_gem_context_free(ctx);
>>> + INIT_WORK(&i915->contexts.free_work, contexts_free_worker);
>>> + init_llist_head(&i915->contexts.free_list);
>>
>> ugh diff.. :) looks like pure movement from perspective of
>> destroy_kernel_context.
>>
>>> }
>>>
>>> static bool needs_preempt_context(struct drm_i915_private *i915)
>>> @@ -470,14 +535,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
>>> if (ret)
>>> return ret;
>>>
>>> - INIT_LIST_HEAD(&dev_priv->contexts.list);
>>> - INIT_WORK(&dev_priv->contexts.free_work, contexts_free_worker);
>>> - init_llist_head(&dev_priv->contexts.free_list);
>>> -
>>> - /* Using the simple ida interface, the max is limited by sizeof(int) */
>>> - BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
>>> - BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
>>> - ida_init(&dev_priv->contexts.hw_ida);
>>> + init_contexts(dev_priv);
>>>
>>> /* lowest priority; idle task */
>>> ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_MIN);
>>> @@ -490,6 +548,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
>>> * all user contexts will have non-zero hw_id.
>>> */
>>> GEM_BUG_ON(ctx->hw_id);
>>> + GEM_BUG_ON(!atomic_read(&ctx->pin_hw_id));
>>
>> /* Kernel context is perma-pinned */
>>
>>> dev_priv->kernel_context = ctx;
>>>
>>> /* highest priority; preempting task */
>>> @@ -527,6 +586,7 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
>>> destroy_kernel_context(&i915->kernel_context);
>>>
>>> /* Must free all deferred contexts (via flush_workqueue) first */
>>> + GEM_BUG_ON(!list_empty(&i915->contexts.hw_id_list));
>>> ida_destroy(&i915->contexts.hw_ida);
>>> }
>>>
>>> @@ -932,6 +992,33 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
>>> return ret;
>>> }
>>>
>>> +int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
>>> +{
>>> + struct drm_i915_private *i915 = ctx->i915;
>>> + int err = 0;
>>> +
>>> + mutex_lock(&i915->contexts.mutex);
>>> +
>>> + GEM_BUG_ON(i915_gem_context_is_closed(ctx));
>>> +
>>> + if (list_empty(&ctx->hw_id_link)) {
>>> + GEM_BUG_ON(atomic_read(&ctx->pin_hw_id));
>>> +
>>> + err = assign_hw_id(i915, &ctx->hw_id);
>>> + if (err)
>>> + goto out_unlock;
>>> +
>>> + list_add_tail(&ctx->hw_id_link, &i915->contexts.hw_id_list);
>>> + }
>>> +
>>> + GEM_BUG_ON(atomic_read(&ctx->pin_hw_id) == ~0u);
>>> + atomic_inc(&ctx->pin_hw_id);
>>> +
>>> +out_unlock:
>>> + mutex_unlock(&i915->contexts.mutex);
>>> + return err;
>>> +}
>>> +
>>> #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>>> #include "selftests/mock_context.c"
>>> #include "selftests/i915_gem_context.c"
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
>>> index 851dad6decd7..c73ac614f58c 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_context.h
>>> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
>>> @@ -136,6 +136,8 @@ struct i915_gem_context {
>>> * id for the lifetime of the context.
>>> */
>>> unsigned int hw_id;
>>> + atomic_t pin_hw_id;
>>
>> I think now we need short comments describing the difference between the
>> two.
>
> One is 32bits unsigned, unserialised. The other is 32bits signed, and
> very loosely serialised :)
And pin_hw_id is really hw_id_pin_count, no? :)
>
>>> + struct list_head hw_id_link;
>>
>> And for this one.
>
> [snip]
>
>> So in essence there will be a little bit more cost when pinning in the
>> normal case, or a bit bit more in the stealing/pathological case, but as
>> long as we stay below over-subscription the cost is only on first pin.
>> No complaints there. Debug also won't be confusing in the normal case
>> since numbers will be stable.
>
> Yup. Nice addition to the changelog, thanks.
>
>> Does it have any negative connotations in the world of OA is the
>> question for Lionel?
>
> Lionel kept promising me this was ok, that he/gputop was quite ready for
> shorter lived ctx id, and reuse.
Cool.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reduce context HW ID lifetime
2018-09-03 9:59 ` Tvrtko Ursulin
@ 2018-09-04 13:48 ` Chris Wilson
0 siblings, 0 replies; 19+ messages in thread
From: Chris Wilson @ 2018-09-04 13:48 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx; +Cc: Oscar Mateo, Mika Kuoppala
Quoting Tvrtko Ursulin (2018-09-03 10:59:01)
>
> On 31/08/2018 13:36, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-08-30 17:23:43)
> >>
> >> On 30/08/2018 11:24, Chris Wilson wrote:
> >>> +static int assign_hw_id(struct drm_i915_private *i915, unsigned int *out)
> >>> +{
> >>> + int ret;
> >>> +
> >>> + lockdep_assert_held(&i915->contexts.mutex);
> >>> +
> >>> + ret = new_hw_id(i915, GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
> >>> + if (unlikely(ret < 0)) {
> >>> + ret = steal_hw_id(i915);
> >>> + if (ret < 0) /* once again for the correct erro code */
> >>
> >> errno
> >>
> >>> + ret = new_hw_id(i915, GFP_KERNEL);
> >>
> >> Hmm.. shouldn't you try GFP_KERNEL before attempting to steal? Actually
> >> I think you should branch based on -ENOSPC (steal) vs -ENOMEM (retry
> >> with GFP_KERNEL). Which would actually mean something like:
> >
> > I was applying the same strategy as we use elsewhere. Penalise any
> > driver cache before hitting reclaim.
> >
> > I think that is fair from an application of soft backpressure point of
> > view. (Lack of backpressure is probably a sore point for many.)
>
> My concern was lack of a phase which avoids hw id stealing for loads
> with few contexts but heavy memory pressure. Sounded like a thing worth
> "robustifying" against - you don't think so?
Do we care much at the point where we fail to direct reclaim a page for
the ida allocator?
It's a tough call, and I think erring on the side of the rest of the
system vs new requests is best overall in an enlightened self-interest
pov. I completely agree we can construct cases where giving up amounts
to priority-inversion and an unfortunate DoS of important clients, but
my gut feeling is that they typical desktop would remain more responsive
with i915 giving up first.
Thank goodness we are not RT.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] drm/i915: Reduce context HW ID lifetime
2018-08-30 10:24 [PATCH] drm/i915: Reduce context HW ID lifetime Chris Wilson
` (4 preceding siblings ...)
2018-08-30 17:10 ` ✓ Fi.CI.IGT: success for drm/i915: Reduce context HW ID lifetime (rev2) Patchwork
@ 2018-09-04 15:31 ` Chris Wilson
2018-09-05 9:49 ` Tvrtko Ursulin
2018-09-04 16:02 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Reduce context HW ID lifetime (rev3) Patchwork
` (3 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Chris Wilson @ 2018-09-04 15:31 UTC (permalink / raw)
To: intel-gfx; +Cc: Mika Kuoppala
Future gen reduce the number of bits we will have available to
differentiate between contexts, so reduce the lifetime of the ID
assignment from that of the context to its current active cycle (i.e.
only while it is pinned for use by the HW, will it have a constant ID).
This means that instead of a max of 2k allocated contexts (worst case
before fun with bit twiddling), we instead have a limit of 2k in flight
contexts (minus a few that have been pinned by the kernel or by perf).
To reduce the number of contexts id we require, we allocate a context id
on first and mark it as pinned for as long as the GEM context itself is,
that is we keep it pinned it while active on each engine. If we exhaust
our context id space, then we try to reclaim an id from an idle context.
In the extreme case where all context ids are pinned by active contexts,
we force the system to idle in order to recover ids.
We cannot reduce the scope of an HW-ID to an engine (allowing the same
gem_context to have different ids on each engine) as in the future we
will need to preassign an id before we know which engine the
context is being executed on.
v2: Improved commentary (Tvrtko) [I tried at least]
References: https://bugs.freedesktop.org/show_bug.cgi?id=107788
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
drivers/gpu/drm/i915/i915_drv.h | 2 +
drivers/gpu/drm/i915/i915_gem_context.c | 222 +++++++++++++-----
drivers/gpu/drm/i915/i915_gem_context.h | 23 ++
drivers/gpu/drm/i915/intel_lrc.c | 8 +
drivers/gpu/drm/i915/selftests/mock_context.c | 11 +-
6 files changed, 201 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 4ad0e2ed8610..1f7051e97afb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1953,7 +1953,10 @@ static int i915_context_status(struct seq_file *m, void *unused)
return ret;
list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
- seq_printf(m, "HW context %u ", ctx->hw_id);
+ seq_puts(m, "HW context ");
+ if (!list_empty(&ctx->hw_id_link))
+ seq_printf(m, "%x [pin %u]", ctx->hw_id,
+ atomic_read(&ctx->hw_id_pin_count));
if (ctx->pid) {
struct task_struct *task;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5a4da5b723fd..767615ecdea5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1861,6 +1861,7 @@ struct drm_i915_private {
struct mutex av_mutex;
struct {
+ struct mutex mutex;
struct list_head list;
struct llist_head free_list;
struct work_struct free_work;
@@ -1873,6 +1874,7 @@ struct drm_i915_private {
#define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */
#define MAX_GUC_CONTEXT_HW_ID (1 << 20) /* exclusive */
#define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */
+ struct list_head hw_id_list;
} contexts;
u32 fdi_rx_config;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index f15a039772db..747b8170a15a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -115,6 +115,95 @@ static void lut_close(struct i915_gem_context *ctx)
rcu_read_unlock();
}
+static inline int new_hw_id(struct drm_i915_private *i915, gfp_t gfp)
+{
+ unsigned int max;
+
+ lockdep_assert_held(&i915->contexts.mutex);
+
+ if (INTEL_GEN(i915) >= 11)
+ max = GEN11_MAX_CONTEXT_HW_ID;
+ else if (USES_GUC_SUBMISSION(i915))
+ /*
+ * When using GuC in proxy submission, GuC consumes the
+ * highest bit in the context id to indicate proxy submission.
+ */
+ max = MAX_GUC_CONTEXT_HW_ID;
+ else
+ max = MAX_CONTEXT_HW_ID;
+
+ return ida_simple_get(&i915->contexts.hw_ida, 0, max, gfp);
+}
+
+static int steal_hw_id(struct drm_i915_private *i915)
+{
+ struct i915_gem_context *ctx, *cn;
+ LIST_HEAD(pinned);
+ int id = -ENOSPC;
+
+ lockdep_assert_held(&i915->contexts.mutex);
+
+ list_for_each_entry_safe(ctx, cn,
+ &i915->contexts.hw_id_list, hw_id_link) {
+ if (atomic_read(&ctx->hw_id_pin_count)) {
+ list_move_tail(&ctx->hw_id_link, &pinned);
+ continue;
+ }
+
+ GEM_BUG_ON(!ctx->hw_id); /* perma-pinned kernel context */
+ list_del_init(&ctx->hw_id_link);
+ id = ctx->hw_id;
+ break;
+ }
+
+ /*
+ * Remember how far we got up on the last repossesion scan, so the
+ * list is kept in a "least recently scanned" order.
+ */
+ list_splice_tail(&pinned, &i915->contexts.hw_id_list);
+ return id;
+}
+
+static int assign_hw_id(struct drm_i915_private *i915, unsigned int *out)
+{
+ int ret;
+
+ lockdep_assert_held(&i915->contexts.mutex);
+
+ /*
+ * We prefer to steal/stall ourselves and our users over that of the
+ * entire system. That may be a little unfair to our users, and
+ * even hurt high priority clients. The choice is whether to oomkill
+ * something else, or steal a context id.
+ */
+ ret = new_hw_id(i915, GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
+ if (unlikely(ret < 0)) {
+ ret = steal_hw_id(i915);
+ if (ret < 0) /* once again for the correct errno code */
+ ret = new_hw_id(i915, GFP_KERNEL);
+ if (ret < 0)
+ return ret;
+ }
+
+ *out = ret;
+ return 0;
+}
+
+static void release_hw_id(struct i915_gem_context *ctx)
+{
+ struct drm_i915_private *i915 = ctx->i915;
+
+ if (list_empty(&ctx->hw_id_link))
+ return;
+
+ mutex_lock(&i915->contexts.mutex);
+ if (!list_empty(&ctx->hw_id_link)) {
+ ida_simple_remove(&i915->contexts.hw_ida, ctx->hw_id);
+ list_del_init(&ctx->hw_id_link);
+ }
+ mutex_unlock(&i915->contexts.mutex);
+}
+
static void i915_gem_context_free(struct i915_gem_context *ctx)
{
unsigned int n;
@@ -122,6 +211,7 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
lockdep_assert_held(&ctx->i915->drm.struct_mutex);
GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
+ release_hw_id(ctx);
i915_ppgtt_put(ctx->ppgtt);
for (n = 0; n < ARRAY_SIZE(ctx->__engine); n++) {
@@ -136,7 +226,6 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
list_del(&ctx->link);
- ida_simple_remove(&ctx->i915->contexts.hw_ida, ctx->hw_id);
kfree_rcu(ctx, rcu);
}
@@ -190,6 +279,12 @@ static void context_close(struct i915_gem_context *ctx)
{
i915_gem_context_set_closed(ctx);
+ /*
+ * This context will never again be assinged to HW, so we can
+ * reuse its ID for the next context.
+ */
+ release_hw_id(ctx);
+
/*
* The LUT uses the VMA as a backpointer to unref the object,
* so we need to clear the LUT before we close all the VMA (inside
@@ -203,43 +298,6 @@ static void context_close(struct i915_gem_context *ctx)
i915_gem_context_put(ctx);
}
-static int assign_hw_id(struct drm_i915_private *dev_priv, unsigned *out)
-{
- int ret;
- unsigned int max;
-
- if (INTEL_GEN(dev_priv) >= 11) {
- max = GEN11_MAX_CONTEXT_HW_ID;
- } else {
- /*
- * When using GuC in proxy submission, GuC consumes the
- * highest bit in the context id to indicate proxy submission.
- */
- if (USES_GUC_SUBMISSION(dev_priv))
- max = MAX_GUC_CONTEXT_HW_ID;
- else
- max = MAX_CONTEXT_HW_ID;
- }
-
-
- ret = ida_simple_get(&dev_priv->contexts.hw_ida,
- 0, max, GFP_KERNEL);
- if (ret < 0) {
- /* Contexts are only released when no longer active.
- * Flush any pending retires to hopefully release some
- * stale contexts and try again.
- */
- i915_retire_requests(dev_priv);
- ret = ida_simple_get(&dev_priv->contexts.hw_ida,
- 0, max, GFP_KERNEL);
- if (ret < 0)
- return ret;
- }
-
- *out = ret;
- return 0;
-}
-
static u32 default_desc_template(const struct drm_i915_private *i915,
const struct i915_hw_ppgtt *ppgtt)
{
@@ -276,12 +334,6 @@ __create_hw_context(struct drm_i915_private *dev_priv,
if (ctx == NULL)
return ERR_PTR(-ENOMEM);
- ret = assign_hw_id(dev_priv, &ctx->hw_id);
- if (ret) {
- kfree(ctx);
- return ERR_PTR(ret);
- }
-
kref_init(&ctx->ref);
list_add_tail(&ctx->link, &dev_priv->contexts.list);
ctx->i915 = dev_priv;
@@ -295,6 +347,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
INIT_LIST_HEAD(&ctx->handles_list);
+ INIT_LIST_HEAD(&ctx->hw_id_link);
/* Default context will never have a file_priv */
ret = DEFAULT_CONTEXT_HANDLE;
@@ -421,15 +474,35 @@ i915_gem_context_create_gvt(struct drm_device *dev)
return ctx;
}
+static void
+destroy_kernel_context(struct i915_gem_context **ctxp)
+{
+ struct i915_gem_context *ctx;
+
+ /* Keep the context ref so that we can free it immediately ourselves */
+ ctx = i915_gem_context_get(fetch_and_zero(ctxp));
+ GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
+
+ context_close(ctx);
+ i915_gem_context_free(ctx);
+}
+
struct i915_gem_context *
i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
{
struct i915_gem_context *ctx;
+ int err;
ctx = i915_gem_create_context(i915, NULL);
if (IS_ERR(ctx))
return ctx;
+ err = i915_gem_context_pin_hw_id(ctx);
+ if (err) {
+ destroy_kernel_context(&ctx);
+ return ERR_PTR(err);
+ }
+
i915_gem_context_clear_bannable(ctx);
ctx->sched.priority = prio;
ctx->ring_size = PAGE_SIZE;
@@ -439,17 +512,19 @@ i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
return ctx;
}
-static void
-destroy_kernel_context(struct i915_gem_context **ctxp)
+static void init_contexts(struct drm_i915_private *i915)
{
- struct i915_gem_context *ctx;
+ mutex_init(&i915->contexts.mutex);
+ INIT_LIST_HEAD(&i915->contexts.list);
- /* Keep the context ref so that we can free it immediately ourselves */
- ctx = i915_gem_context_get(fetch_and_zero(ctxp));
- GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
+ /* Using the simple ida interface, the max is limited by sizeof(int) */
+ BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
+ BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
+ ida_init(&i915->contexts.hw_ida);
+ INIT_LIST_HEAD(&i915->contexts.hw_id_list);
- context_close(ctx);
- i915_gem_context_free(ctx);
+ INIT_WORK(&i915->contexts.free_work, contexts_free_worker);
+ init_llist_head(&i915->contexts.free_list);
}
static bool needs_preempt_context(struct drm_i915_private *i915)
@@ -470,14 +545,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
if (ret)
return ret;
- INIT_LIST_HEAD(&dev_priv->contexts.list);
- INIT_WORK(&dev_priv->contexts.free_work, contexts_free_worker);
- init_llist_head(&dev_priv->contexts.free_list);
-
- /* Using the simple ida interface, the max is limited by sizeof(int) */
- BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
- BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
- ida_init(&dev_priv->contexts.hw_ida);
+ init_contexts(dev_priv);
/* lowest priority; idle task */
ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_MIN);
@@ -487,9 +555,13 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
}
/*
* For easy recognisablity, we want the kernel context to be 0 and then
- * all user contexts will have non-zero hw_id.
+ * all user contexts will have non-zero hw_id. Kernel contexts are
+ * permanently pinned, so that we never suffer a stall and can
+ * use them from any allocation context (e.g. for evicting other
+ * contexts and from inside the shrinker).
*/
GEM_BUG_ON(ctx->hw_id);
+ GEM_BUG_ON(!atomic_read(&ctx->hw_id_pin_count));
dev_priv->kernel_context = ctx;
/* highest priority; preempting task */
@@ -527,6 +599,7 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
destroy_kernel_context(&i915->kernel_context);
/* Must free all deferred contexts (via flush_workqueue) first */
+ GEM_BUG_ON(!list_empty(&i915->contexts.hw_id_list));
ida_destroy(&i915->contexts.hw_ida);
}
@@ -932,6 +1005,33 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
return ret;
}
+int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
+{
+ struct drm_i915_private *i915 = ctx->i915;
+ int err = 0;
+
+ mutex_lock(&i915->contexts.mutex);
+
+ GEM_BUG_ON(i915_gem_context_is_closed(ctx));
+
+ if (list_empty(&ctx->hw_id_link)) {
+ GEM_BUG_ON(atomic_read(&ctx->hw_id_pin_count));
+
+ err = assign_hw_id(i915, &ctx->hw_id);
+ if (err)
+ goto out_unlock;
+
+ list_add_tail(&ctx->hw_id_link, &i915->contexts.hw_id_list);
+ }
+
+ GEM_BUG_ON(atomic_read(&ctx->hw_id_pin_count) == ~0u);
+ atomic_inc(&ctx->hw_id_pin_count);
+
+out_unlock:
+ mutex_unlock(&i915->contexts.mutex);
+ return err;
+}
+
#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
#include "selftests/mock_context.c"
#include "selftests/i915_gem_context.c"
diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
index 851dad6decd7..e09673ca731d 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/i915_gem_context.h
@@ -134,8 +134,16 @@ struct i915_gem_context {
* functions like fault reporting, PASID, scheduling. The
* &drm_i915_private.context_hw_ida is used to assign a unqiue
* id for the lifetime of the context.
+ *
+ * @hw_id_pin_count: - number of times this context had been pinned
+ * for use (should be, at most, once per engine).
+ *
+ * @hw_id_link: - all contexts with an assigned id are tracked
+ * for possible repossession.
*/
unsigned int hw_id;
+ atomic_t hw_id_pin_count;
+ struct list_head hw_id_link;
/**
* @user_handle: userspace identifier
@@ -254,6 +262,21 @@ static inline void i915_gem_context_set_force_single_submission(struct i915_gem_
__set_bit(CONTEXT_FORCE_SINGLE_SUBMISSION, &ctx->flags);
}
+int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx);
+static inline int i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
+{
+ if (atomic_inc_not_zero(&ctx->hw_id_pin_count))
+ return 0;
+
+ return __i915_gem_context_pin_hw_id(ctx);
+}
+
+static inline void i915_gem_context_unpin_hw_id(struct i915_gem_context *ctx)
+{
+ GEM_BUG_ON(atomic_read(&ctx->hw_id_pin_count) == 0u);
+ atomic_dec(&ctx->hw_id_pin_count);
+}
+
static inline bool i915_gem_context_is_default(const struct i915_gem_context *c)
{
return c->user_handle == DEFAULT_CONTEXT_HANDLE;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index def467c2451b..9b1f0e5211a0 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1272,6 +1272,8 @@ static void execlists_context_destroy(struct intel_context *ce)
static void execlists_context_unpin(struct intel_context *ce)
{
+ i915_gem_context_unpin_hw_id(ce->gem_context);
+
intel_ring_unpin(ce->ring);
ce->state->obj->pin_global--;
@@ -1330,6 +1332,10 @@ __execlists_context_pin(struct intel_engine_cs *engine,
if (ret)
goto unpin_map;
+ ret = i915_gem_context_pin_hw_id(ctx);
+ if (ret)
+ goto unpin_ring;
+
intel_lr_context_descriptor_update(ctx, engine, ce);
ce->lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
@@ -1342,6 +1348,8 @@ __execlists_context_pin(struct intel_engine_cs *engine,
i915_gem_context_get(ctx);
return ce;
+unpin_ring:
+ intel_ring_unpin(ce->ring);
unpin_map:
i915_gem_object_unpin_map(ce->state->obj);
unpin_vma:
diff --git a/drivers/gpu/drm/i915/selftests/mock_context.c b/drivers/gpu/drm/i915/selftests/mock_context.c
index 8904f1ce64e3..d937bdff26f9 100644
--- a/drivers/gpu/drm/i915/selftests/mock_context.c
+++ b/drivers/gpu/drm/i915/selftests/mock_context.c
@@ -43,6 +43,7 @@ mock_context(struct drm_i915_private *i915,
INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
INIT_LIST_HEAD(&ctx->handles_list);
+ INIT_LIST_HEAD(&ctx->hw_id_link);
for (n = 0; n < ARRAY_SIZE(ctx->__engine); n++) {
struct intel_context *ce = &ctx->__engine[n];
@@ -50,11 +51,9 @@ mock_context(struct drm_i915_private *i915,
ce->gem_context = ctx;
}
- ret = ida_simple_get(&i915->contexts.hw_ida,
- 0, MAX_CONTEXT_HW_ID, GFP_KERNEL);
+ ret = i915_gem_context_pin_hw_id(ctx);
if (ret < 0)
goto err_handles;
- ctx->hw_id = ret;
if (name) {
ctx->name = kstrdup(name, GFP_KERNEL);
@@ -85,11 +84,7 @@ void mock_context_close(struct i915_gem_context *ctx)
void mock_init_contexts(struct drm_i915_private *i915)
{
- INIT_LIST_HEAD(&i915->contexts.list);
- ida_init(&i915->contexts.hw_ida);
-
- INIT_WORK(&i915->contexts.free_work, contexts_free_worker);
- init_llist_head(&i915->contexts.free_list);
+ init_contexts(i915);
}
struct i915_gem_context *
--
2.19.0.rc1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Reduce context HW ID lifetime (rev3)
2018-08-30 10:24 [PATCH] drm/i915: Reduce context HW ID lifetime Chris Wilson
` (5 preceding siblings ...)
2018-09-04 15:31 ` [PATCH v2] drm/i915: Reduce context HW ID lifetime Chris Wilson
@ 2018-09-04 16:02 ` Patchwork
2018-09-04 16:03 ` ✗ Fi.CI.SPARSE: " Patchwork
` (2 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2018-09-04 16:02 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Reduce context HW ID lifetime (rev3)
URL : https://patchwork.freedesktop.org/series/44134/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
9a6f6d0994b2 drm/i915: Reduce context HW ID lifetime
-:61: CHECK:UNCOMMENTED_DEFINITION: struct mutex definition without comment
#61: FILE: drivers/gpu/drm/i915/i915_drv.h:1864:
+ struct mutex mutex;
total: 0 errors, 0 warnings, 1 checks, 433 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* ✗ Fi.CI.SPARSE: warning for drm/i915: Reduce context HW ID lifetime (rev3)
2018-08-30 10:24 [PATCH] drm/i915: Reduce context HW ID lifetime Chris Wilson
` (6 preceding siblings ...)
2018-09-04 16:02 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Reduce context HW ID lifetime (rev3) Patchwork
@ 2018-09-04 16:03 ` Patchwork
2018-09-04 16:19 ` ✓ Fi.CI.BAT: success " Patchwork
2018-09-04 22:28 ` ✓ Fi.CI.IGT: " Patchwork
9 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2018-09-04 16:03 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Reduce context HW ID lifetime (rev3)
URL : https://patchwork.freedesktop.org/series/44134/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Commit: drm/i915: Reduce context HW ID lifetime
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3686:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3688:16: warning: expression using sizeof(void)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Reduce context HW ID lifetime (rev3)
2018-08-30 10:24 [PATCH] drm/i915: Reduce context HW ID lifetime Chris Wilson
` (7 preceding siblings ...)
2018-09-04 16:03 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2018-09-04 16:19 ` Patchwork
2018-09-04 22:28 ` ✓ Fi.CI.IGT: " Patchwork
9 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2018-09-04 16:19 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Reduce context HW ID lifetime (rev3)
URL : https://patchwork.freedesktop.org/series/44134/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4767 -> Patchwork_10083 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/44134/revisions/3/mbox/
== Known issues ==
Here are the changes found in Patchwork_10083 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_exec_suspend@basic-s3:
fi-kbl-soraka: NOTRUN -> INCOMPLETE (fdo#107774, fdo#107556)
==== Possible fixes ====
igt@gem_exec_suspend@basic-s3:
fi-cfl-8109u: DMESG-WARN (fdo#107345) -> PASS +1
igt@gem_exec_suspend@basic-s4-devices:
fi-kbl-7500u: DMESG-WARN (fdo#105128, fdo#107139) -> PASS
igt@kms_flip@basic-flip-vs-modeset:
fi-cfl-8109u: INCOMPLETE (fdo#107468) -> PASS
fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
fdo#107345 https://bugs.freedesktop.org/show_bug.cgi?id=107345
fdo#107468 https://bugs.freedesktop.org/show_bug.cgi?id=107468
fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
fdo#107774 https://bugs.freedesktop.org/show_bug.cgi?id=107774
== Participating hosts (50 -> 45) ==
Additional (1): fi-kbl-soraka
Missing (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-kbl-7560u
== Build changes ==
* Linux: CI_DRM_4767 -> Patchwork_10083
CI_DRM_4767: e9b69bafd3c2c13a8b9fa8e7a410f5d5ef32e328 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4626: bfce01d8c93dbd86e6ab04ca1afb844e0cbc8078 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10083: 9a6f6d0994b229642ebe29974173f89352efd764 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
9a6f6d0994b2 drm/i915: Reduce context HW ID lifetime
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10083/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Reduce context HW ID lifetime (rev3)
2018-08-30 10:24 [PATCH] drm/i915: Reduce context HW ID lifetime Chris Wilson
` (8 preceding siblings ...)
2018-09-04 16:19 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-09-04 22:28 ` Patchwork
9 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2018-09-04 22:28 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Reduce context HW ID lifetime (rev3)
URL : https://patchwork.freedesktop.org/series/44134/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4767_full -> Patchwork_10083_full =
== Summary - WARNING ==
Minor unknown changes coming with Patchwork_10083_full need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_10083_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
== Possible new issues ==
Here are the unknown changes that may have been introduced in Patchwork_10083_full:
=== IGT changes ===
==== Warnings ====
igt@pm_rc6_residency@rc6-accuracy:
shard-kbl: PASS -> SKIP
== Known issues ==
Here are the changes found in Patchwork_10083_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@kms_color@pipe-b-gamma:
shard-kbl: PASS -> DMESG-WARN (fdo#103558, fdo#105602) +3
igt@kms_flip@flip-vs-expired-vblank:
shard-glk: PASS -> FAIL (fdo#102887, fdo#105363)
igt@kms_flip@flip-vs-panning-vs-hang:
shard-kbl: PASS -> DMESG-WARN (fdo#103558, fdo#103313, fdo#105602) +6
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
shard-kbl: PASS -> DMESG-WARN (fdo#103558, fdo#105079, fdo#103841, fdo#105602)
igt@pm_rpm@gem-mmap-cpu:
shard-apl: PASS -> DMESG-WARN (fdo#103558, fdo#105602) +18
igt@pm_rpm@i2c:
shard-kbl: PASS -> DMESG-FAIL (fdo#103558, fdo#103313, fdo#105602)
==== Possible fixes ====
igt@kms_flip@flip-vs-expired-vblank-interruptible:
shard-glk: FAIL (fdo#102887, fdo#105363) -> PASS
igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-draw-pwrite:
shard-glk: FAIL (fdo#103167) -> PASS
igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
shard-glk: INCOMPLETE (fdo#103359, k.org#198133) -> PASS
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103313 https://bugs.freedesktop.org/show_bug.cgi?id=103313
fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
fdo#105079 https://bugs.freedesktop.org/show_bug.cgi?id=105079
fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133
== Participating hosts (5 -> 5) ==
No changes in participating hosts
== Build changes ==
* Linux: CI_DRM_4767 -> Patchwork_10083
CI_DRM_4767: e9b69bafd3c2c13a8b9fa8e7a410f5d5ef32e328 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4626: bfce01d8c93dbd86e6ab04ca1afb844e0cbc8078 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10083: 9a6f6d0994b229642ebe29974173f89352efd764 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10083/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] drm/i915: Reduce context HW ID lifetime
2018-09-04 15:31 ` [PATCH v2] drm/i915: Reduce context HW ID lifetime Chris Wilson
@ 2018-09-05 9:49 ` Tvrtko Ursulin
2018-09-05 10:33 ` Chris Wilson
2018-09-05 11:01 ` Chris Wilson
0 siblings, 2 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2018-09-05 9:49 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Mika Kuoppala
On 04/09/2018 16:31, Chris Wilson wrote:
> Future gen reduce the number of bits we will have available to
> differentiate between contexts, so reduce the lifetime of the ID
> assignment from that of the context to its current active cycle (i.e.
> only while it is pinned for use by the HW, will it have a constant ID).
> This means that instead of a max of 2k allocated contexts (worst case
> before fun with bit twiddling), we instead have a limit of 2k in flight
> contexts (minus a few that have been pinned by the kernel or by perf).
>
> To reduce the number of contexts id we require, we allocate a context id
> on first and mark it as pinned for as long as the GEM context itself is,
> that is we keep it pinned it while active on each engine. If we exhaust
> our context id space, then we try to reclaim an id from an idle context.
> In the extreme case where all context ids are pinned by active contexts,
> we force the system to idle in order to recover ids.
>
> We cannot reduce the scope of an HW-ID to an engine (allowing the same
> gem_context to have different ids on each engine) as in the future we
> will need to preassign an id before we know which engine the
> context is being executed on.
>
> v2: Improved commentary (Tvrtko) [I tried at least]
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=107788
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
> drivers/gpu/drm/i915/i915_drv.h | 2 +
> drivers/gpu/drm/i915/i915_gem_context.c | 222 +++++++++++++-----
> drivers/gpu/drm/i915/i915_gem_context.h | 23 ++
> drivers/gpu/drm/i915/intel_lrc.c | 8 +
> drivers/gpu/drm/i915/selftests/mock_context.c | 11 +-
> 6 files changed, 201 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 4ad0e2ed8610..1f7051e97afb 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1953,7 +1953,10 @@ static int i915_context_status(struct seq_file *m, void *unused)
> return ret;
>
> list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
> - seq_printf(m, "HW context %u ", ctx->hw_id);
> + seq_puts(m, "HW context ");
> + if (!list_empty(&ctx->hw_id_link))
> + seq_printf(m, "%x [pin %u]", ctx->hw_id,
> + atomic_read(&ctx->hw_id_pin_count));
Do you want to put some marker for the unallocated case here?
> if (ctx->pid) {
> struct task_struct *task;
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5a4da5b723fd..767615ecdea5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1861,6 +1861,7 @@ struct drm_i915_private {
> struct mutex av_mutex;
>
> struct {
> + struct mutex mutex;
> struct list_head list;
> struct llist_head free_list;
> struct work_struct free_work;
> @@ -1873,6 +1874,7 @@ struct drm_i915_private {
> #define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */
> #define MAX_GUC_CONTEXT_HW_ID (1 << 20) /* exclusive */
> #define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */
> + struct list_head hw_id_list;
> } contexts;
>
> u32 fdi_rx_config;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index f15a039772db..747b8170a15a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -115,6 +115,95 @@ static void lut_close(struct i915_gem_context *ctx)
> rcu_read_unlock();
> }
>
> +static inline int new_hw_id(struct drm_i915_private *i915, gfp_t gfp)
> +{
> + unsigned int max;
> +
> + lockdep_assert_held(&i915->contexts.mutex);
> +
> + if (INTEL_GEN(i915) >= 11)
> + max = GEN11_MAX_CONTEXT_HW_ID;
> + else if (USES_GUC_SUBMISSION(i915))
> + /*
> + * When using GuC in proxy submission, GuC consumes the
> + * highest bit in the context id to indicate proxy submission.
> + */
> + max = MAX_GUC_CONTEXT_HW_ID;
> + else
> + max = MAX_CONTEXT_HW_ID;
> +
> + return ida_simple_get(&i915->contexts.hw_ida, 0, max, gfp);
> +}
> +
> +static int steal_hw_id(struct drm_i915_private *i915)
> +{
> + struct i915_gem_context *ctx, *cn;
> + LIST_HEAD(pinned);
> + int id = -ENOSPC;
> +
> + lockdep_assert_held(&i915->contexts.mutex);
> +
> + list_for_each_entry_safe(ctx, cn,
> + &i915->contexts.hw_id_list, hw_id_link) {
> + if (atomic_read(&ctx->hw_id_pin_count)) {
> + list_move_tail(&ctx->hw_id_link, &pinned);
> + continue;
> + }
> +
> + GEM_BUG_ON(!ctx->hw_id); /* perma-pinned kernel context */
> + list_del_init(&ctx->hw_id_link);
> + id = ctx->hw_id;
> + break;
> + }
> +
> + /*
> + * Remember how far we got up on the last repossesion scan, so the
> + * list is kept in a "least recently scanned" order.
> + */
> + list_splice_tail(&pinned, &i915->contexts.hw_id_list);
> + return id;
> +}
> +
> +static int assign_hw_id(struct drm_i915_private *i915, unsigned int *out)
> +{
> + int ret;
> +
> + lockdep_assert_held(&i915->contexts.mutex);
> +
> + /*
> + * We prefer to steal/stall ourselves and our users over that of the
> + * entire system. That may be a little unfair to our users, and
> + * even hurt high priority clients. The choice is whether to oomkill
> + * something else, or steal a context id.
> + */
> + ret = new_hw_id(i915, GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
> + if (unlikely(ret < 0)) {
> + ret = steal_hw_id(i915);
> + if (ret < 0) /* once again for the correct errno code */
> + ret = new_hw_id(i915, GFP_KERNEL);
> + if (ret < 0)
> + return ret;
> + }
> +
> + *out = ret;
> + return 0;
> +}
> +
> +static void release_hw_id(struct i915_gem_context *ctx)
> +{
> + struct drm_i915_private *i915 = ctx->i915;
> +
> + if (list_empty(&ctx->hw_id_link))
> + return;
> +
> + mutex_lock(&i915->contexts.mutex);
> + if (!list_empty(&ctx->hw_id_link)) {
> + ida_simple_remove(&i915->contexts.hw_ida, ctx->hw_id);
> + list_del_init(&ctx->hw_id_link);
> + }
> + mutex_unlock(&i915->contexts.mutex);
> +}
> +
> static void i915_gem_context_free(struct i915_gem_context *ctx)
> {
> unsigned int n;
> @@ -122,6 +211,7 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
> lockdep_assert_held(&ctx->i915->drm.struct_mutex);
> GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
>
> + release_hw_id(ctx);
> i915_ppgtt_put(ctx->ppgtt);
>
> for (n = 0; n < ARRAY_SIZE(ctx->__engine); n++) {
> @@ -136,7 +226,6 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
>
> list_del(&ctx->link);
>
> - ida_simple_remove(&ctx->i915->contexts.hw_ida, ctx->hw_id);
> kfree_rcu(ctx, rcu);
> }
>
> @@ -190,6 +279,12 @@ static void context_close(struct i915_gem_context *ctx)
> {
> i915_gem_context_set_closed(ctx);
>
> + /*
> + * This context will never again be assinged to HW, so we can
> + * reuse its ID for the next context.
> + */
> + release_hw_id(ctx);
> +
> /*
> * The LUT uses the VMA as a backpointer to unref the object,
> * so we need to clear the LUT before we close all the VMA (inside
> @@ -203,43 +298,6 @@ static void context_close(struct i915_gem_context *ctx)
> i915_gem_context_put(ctx);
> }
>
> -static int assign_hw_id(struct drm_i915_private *dev_priv, unsigned *out)
> -{
> - int ret;
> - unsigned int max;
> -
> - if (INTEL_GEN(dev_priv) >= 11) {
> - max = GEN11_MAX_CONTEXT_HW_ID;
> - } else {
> - /*
> - * When using GuC in proxy submission, GuC consumes the
> - * highest bit in the context id to indicate proxy submission.
> - */
> - if (USES_GUC_SUBMISSION(dev_priv))
> - max = MAX_GUC_CONTEXT_HW_ID;
> - else
> - max = MAX_CONTEXT_HW_ID;
> - }
> -
> -
> - ret = ida_simple_get(&dev_priv->contexts.hw_ida,
> - 0, max, GFP_KERNEL);
> - if (ret < 0) {
> - /* Contexts are only released when no longer active.
> - * Flush any pending retires to hopefully release some
> - * stale contexts and try again.
> - */
> - i915_retire_requests(dev_priv);
> - ret = ida_simple_get(&dev_priv->contexts.hw_ida,
> - 0, max, GFP_KERNEL);
> - if (ret < 0)
> - return ret;
> - }
> -
> - *out = ret;
> - return 0;
> -}
> -
> static u32 default_desc_template(const struct drm_i915_private *i915,
> const struct i915_hw_ppgtt *ppgtt)
> {
> @@ -276,12 +334,6 @@ __create_hw_context(struct drm_i915_private *dev_priv,
> if (ctx == NULL)
> return ERR_PTR(-ENOMEM);
>
> - ret = assign_hw_id(dev_priv, &ctx->hw_id);
> - if (ret) {
> - kfree(ctx);
> - return ERR_PTR(ret);
> - }
> -
> kref_init(&ctx->ref);
> list_add_tail(&ctx->link, &dev_priv->contexts.list);
> ctx->i915 = dev_priv;
> @@ -295,6 +347,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
>
> INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
> INIT_LIST_HEAD(&ctx->handles_list);
> + INIT_LIST_HEAD(&ctx->hw_id_link);
>
> /* Default context will never have a file_priv */
> ret = DEFAULT_CONTEXT_HANDLE;
> @@ -421,15 +474,35 @@ i915_gem_context_create_gvt(struct drm_device *dev)
> return ctx;
> }
>
> +static void
> +destroy_kernel_context(struct i915_gem_context **ctxp)
> +{
> + struct i915_gem_context *ctx;
> +
> + /* Keep the context ref so that we can free it immediately ourselves */
> + ctx = i915_gem_context_get(fetch_and_zero(ctxp));
> + GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> +
> + context_close(ctx);
> + i915_gem_context_free(ctx);
> +}
> +
> struct i915_gem_context *
> i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
> {
> struct i915_gem_context *ctx;
> + int err;
>
> ctx = i915_gem_create_context(i915, NULL);
> if (IS_ERR(ctx))
> return ctx;
>
> + err = i915_gem_context_pin_hw_id(ctx);
> + if (err) {
> + destroy_kernel_context(&ctx);
> + return ERR_PTR(err);
> + }
> +
> i915_gem_context_clear_bannable(ctx);
> ctx->sched.priority = prio;
> ctx->ring_size = PAGE_SIZE;
> @@ -439,17 +512,19 @@ i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
> return ctx;
> }
>
> -static void
> -destroy_kernel_context(struct i915_gem_context **ctxp)
> +static void init_contexts(struct drm_i915_private *i915)
> {
> - struct i915_gem_context *ctx;
> + mutex_init(&i915->contexts.mutex);
> + INIT_LIST_HEAD(&i915->contexts.list);
>
> - /* Keep the context ref so that we can free it immediately ourselves */
> - ctx = i915_gem_context_get(fetch_and_zero(ctxp));
> - GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> + /* Using the simple ida interface, the max is limited by sizeof(int) */
> + BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
> + BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
> + ida_init(&i915->contexts.hw_ida);
> + INIT_LIST_HEAD(&i915->contexts.hw_id_list);
>
> - context_close(ctx);
> - i915_gem_context_free(ctx);
> + INIT_WORK(&i915->contexts.free_work, contexts_free_worker);
> + init_llist_head(&i915->contexts.free_list);
> }
>
> static bool needs_preempt_context(struct drm_i915_private *i915)
> @@ -470,14 +545,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> if (ret)
> return ret;
>
> - INIT_LIST_HEAD(&dev_priv->contexts.list);
> - INIT_WORK(&dev_priv->contexts.free_work, contexts_free_worker);
> - init_llist_head(&dev_priv->contexts.free_list);
> -
> - /* Using the simple ida interface, the max is limited by sizeof(int) */
> - BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
> - BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
> - ida_init(&dev_priv->contexts.hw_ida);
> + init_contexts(dev_priv);
>
> /* lowest priority; idle task */
> ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_MIN);
> @@ -487,9 +555,13 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> }
> /*
> * For easy recognisablity, we want the kernel context to be 0 and then
> - * all user contexts will have non-zero hw_id.
> + * all user contexts will have non-zero hw_id. Kernel contexts are
> + * permanently pinned, so that we never suffer a stall and can
> + * use them from any allocation context (e.g. for evicting other
> + * contexts and from inside the shrinker).
> */
> GEM_BUG_ON(ctx->hw_id);
> + GEM_BUG_ON(!atomic_read(&ctx->hw_id_pin_count));
> dev_priv->kernel_context = ctx;
>
> /* highest priority; preempting task */
> @@ -527,6 +599,7 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
> destroy_kernel_context(&i915->kernel_context);
>
> /* Must free all deferred contexts (via flush_workqueue) first */
> + GEM_BUG_ON(!list_empty(&i915->contexts.hw_id_list));
> ida_destroy(&i915->contexts.hw_ida);
> }
>
> @@ -932,6 +1005,33 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
> return ret;
> }
>
> +int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
> +{
> + struct drm_i915_private *i915 = ctx->i915;
> + int err = 0;
> +
> + mutex_lock(&i915->contexts.mutex);
> +
> + GEM_BUG_ON(i915_gem_context_is_closed(ctx));
> +
> + if (list_empty(&ctx->hw_id_link)) {
> + GEM_BUG_ON(atomic_read(&ctx->hw_id_pin_count));
> +
> + err = assign_hw_id(i915, &ctx->hw_id);
> + if (err)
> + goto out_unlock;
> +
> + list_add_tail(&ctx->hw_id_link, &i915->contexts.hw_id_list);
> + }
> +
> + GEM_BUG_ON(atomic_read(&ctx->hw_id_pin_count) == ~0u);
> + atomic_inc(&ctx->hw_id_pin_count);
> +
> +out_unlock:
> + mutex_unlock(&i915->contexts.mutex);
> + return err;
> +}
> +
> #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> #include "selftests/mock_context.c"
> #include "selftests/i915_gem_context.c"
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> index 851dad6decd7..e09673ca731d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> @@ -134,8 +134,16 @@ struct i915_gem_context {
> * functions like fault reporting, PASID, scheduling. The
> * &drm_i915_private.context_hw_ida is used to assign a unqiue
> * id for the lifetime of the context.
> + *
> + * @hw_id_pin_count: - number of times this context had been pinned
> + * for use (should be, at most, once per engine).
> + *
> + * @hw_id_link: - all contexts with an assigned id are tracked
> + * for possible repossession.
> */
> unsigned int hw_id;
> + atomic_t hw_id_pin_count;
> + struct list_head hw_id_link;
>
> /**
> * @user_handle: userspace identifier
> @@ -254,6 +262,21 @@ static inline void i915_gem_context_set_force_single_submission(struct i915_gem_
> __set_bit(CONTEXT_FORCE_SINGLE_SUBMISSION, &ctx->flags);
> }
>
> +int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx);
> +static inline int i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
> +{
> + if (atomic_inc_not_zero(&ctx->hw_id_pin_count))
> + return 0;
> +
> + return __i915_gem_context_pin_hw_id(ctx);
> +}
> +
> +static inline void i915_gem_context_unpin_hw_id(struct i915_gem_context *ctx)
> +{
> + GEM_BUG_ON(atomic_read(&ctx->hw_id_pin_count) == 0u);
> + atomic_dec(&ctx->hw_id_pin_count);
> +}
> +
> static inline bool i915_gem_context_is_default(const struct i915_gem_context *c)
> {
> return c->user_handle == DEFAULT_CONTEXT_HANDLE;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index def467c2451b..9b1f0e5211a0 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1272,6 +1272,8 @@ static void execlists_context_destroy(struct intel_context *ce)
>
> static void execlists_context_unpin(struct intel_context *ce)
> {
> + i915_gem_context_unpin_hw_id(ce->gem_context);
> +
> intel_ring_unpin(ce->ring);
>
> ce->state->obj->pin_global--;
> @@ -1330,6 +1332,10 @@ __execlists_context_pin(struct intel_engine_cs *engine,
> if (ret)
> goto unpin_map;
>
> + ret = i915_gem_context_pin_hw_id(ctx);
> + if (ret)
> + goto unpin_ring;
> +
> intel_lr_context_descriptor_update(ctx, engine, ce);
>
> ce->lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
> @@ -1342,6 +1348,8 @@ __execlists_context_pin(struct intel_engine_cs *engine,
> i915_gem_context_get(ctx);
> return ce;
>
> +unpin_ring:
> + intel_ring_unpin(ce->ring);
> unpin_map:
> i915_gem_object_unpin_map(ce->state->obj);
> unpin_vma:
> diff --git a/drivers/gpu/drm/i915/selftests/mock_context.c b/drivers/gpu/drm/i915/selftests/mock_context.c
> index 8904f1ce64e3..d937bdff26f9 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_context.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_context.c
> @@ -43,6 +43,7 @@ mock_context(struct drm_i915_private *i915,
>
> INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
> INIT_LIST_HEAD(&ctx->handles_list);
> + INIT_LIST_HEAD(&ctx->hw_id_link);
>
> for (n = 0; n < ARRAY_SIZE(ctx->__engine); n++) {
> struct intel_context *ce = &ctx->__engine[n];
> @@ -50,11 +51,9 @@ mock_context(struct drm_i915_private *i915,
> ce->gem_context = ctx;
> }
>
> - ret = ida_simple_get(&i915->contexts.hw_ida,
> - 0, MAX_CONTEXT_HW_ID, GFP_KERNEL);
> + ret = i915_gem_context_pin_hw_id(ctx);
> if (ret < 0)
> goto err_handles;
> - ctx->hw_id = ret;
>
> if (name) {
> ctx->name = kstrdup(name, GFP_KERNEL);
> @@ -85,11 +84,7 @@ void mock_context_close(struct i915_gem_context *ctx)
>
> void mock_init_contexts(struct drm_i915_private *i915)
> {
> - INIT_LIST_HEAD(&i915->contexts.list);
> - ida_init(&i915->contexts.hw_ida);
> -
> - INIT_WORK(&i915->contexts.free_work, contexts_free_worker);
> - init_llist_head(&i915->contexts.free_list);
> + init_contexts(i915);
> }
>
> struct i915_gem_context *
>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] drm/i915: Reduce context HW ID lifetime
2018-09-05 9:49 ` Tvrtko Ursulin
@ 2018-09-05 10:33 ` Chris Wilson
2018-09-05 10:55 ` Tvrtko Ursulin
2018-09-05 11:01 ` Chris Wilson
1 sibling, 1 reply; 19+ messages in thread
From: Chris Wilson @ 2018-09-05 10:33 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx; +Cc: Mika Kuoppala
Quoting Tvrtko Ursulin (2018-09-05 10:49:02)
>
> On 04/09/2018 16:31, Chris Wilson wrote:
> > Future gen reduce the number of bits we will have available to
> > differentiate between contexts, so reduce the lifetime of the ID
> > assignment from that of the context to its current active cycle (i.e.
> > only while it is pinned for use by the HW, will it have a constant ID).
> > This means that instead of a max of 2k allocated contexts (worst case
> > before fun with bit twiddling), we instead have a limit of 2k in flight
> > contexts (minus a few that have been pinned by the kernel or by perf).
> >
> > To reduce the number of contexts id we require, we allocate a context id
> > on first and mark it as pinned for as long as the GEM context itself is,
> > that is we keep it pinned it while active on each engine. If we exhaust
> > our context id space, then we try to reclaim an id from an idle context.
> > In the extreme case where all context ids are pinned by active contexts,
> > we force the system to idle in order to recover ids.
> >
> > We cannot reduce the scope of an HW-ID to an engine (allowing the same
> > gem_context to have different ids on each engine) as in the future we
> > will need to preassign an id before we know which engine the
> > context is being executed on.
> >
> > v2: Improved commentary (Tvrtko) [I tried at least]
> >
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=107788
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
> > drivers/gpu/drm/i915/i915_drv.h | 2 +
> > drivers/gpu/drm/i915/i915_gem_context.c | 222 +++++++++++++-----
> > drivers/gpu/drm/i915/i915_gem_context.h | 23 ++
> > drivers/gpu/drm/i915/intel_lrc.c | 8 +
> > drivers/gpu/drm/i915/selftests/mock_context.c | 11 +-
> > 6 files changed, 201 insertions(+), 70 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 4ad0e2ed8610..1f7051e97afb 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1953,7 +1953,10 @@ static int i915_context_status(struct seq_file *m, void *unused)
> > return ret;
> >
> > list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
> > - seq_printf(m, "HW context %u ", ctx->hw_id);
> > + seq_puts(m, "HW context ");
> > + if (!list_empty(&ctx->hw_id_link))
> > + seq_printf(m, "%x [pin %u]", ctx->hw_id,
> > + atomic_read(&ctx->hw_id_pin_count));
>
> Do you want to put some marker for the unallocated case here?
I was content with absence of marker as indicating it has never had an
id, or it had been revoked.
Who reads this file anyway? There's not one igt where I've thought this
would be useful debug info. Maybe I'm wrong...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] drm/i915: Reduce context HW ID lifetime
2018-09-05 10:33 ` Chris Wilson
@ 2018-09-05 10:55 ` Tvrtko Ursulin
0 siblings, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2018-09-05 10:55 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Mika Kuoppala
On 05/09/2018 11:33, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-09-05 10:49:02)
>>
>> On 04/09/2018 16:31, Chris Wilson wrote:
>>> Future gen reduce the number of bits we will have available to
>>> differentiate between contexts, so reduce the lifetime of the ID
>>> assignment from that of the context to its current active cycle (i.e.
>>> only while it is pinned for use by the HW, will it have a constant ID).
>>> This means that instead of a max of 2k allocated contexts (worst case
>>> before fun with bit twiddling), we instead have a limit of 2k in flight
>>> contexts (minus a few that have been pinned by the kernel or by perf).
>>>
>>> To reduce the number of contexts id we require, we allocate a context id
>>> on first and mark it as pinned for as long as the GEM context itself is,
>>> that is we keep it pinned it while active on each engine. If we exhaust
>>> our context id space, then we try to reclaim an id from an idle context.
>>> In the extreme case where all context ids are pinned by active contexts,
>>> we force the system to idle in order to recover ids.
>>>
>>> We cannot reduce the scope of an HW-ID to an engine (allowing the same
>>> gem_context to have different ids on each engine) as in the future we
>>> will need to preassign an id before we know which engine the
>>> context is being executed on.
>>>
>>> v2: Improved commentary (Tvrtko) [I tried at least]
>>>
>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=107788
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>>> Cc: Michel Thierry <michel.thierry@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
>>> drivers/gpu/drm/i915/i915_drv.h | 2 +
>>> drivers/gpu/drm/i915/i915_gem_context.c | 222 +++++++++++++-----
>>> drivers/gpu/drm/i915/i915_gem_context.h | 23 ++
>>> drivers/gpu/drm/i915/intel_lrc.c | 8 +
>>> drivers/gpu/drm/i915/selftests/mock_context.c | 11 +-
>>> 6 files changed, 201 insertions(+), 70 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 4ad0e2ed8610..1f7051e97afb 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -1953,7 +1953,10 @@ static int i915_context_status(struct seq_file *m, void *unused)
>>> return ret;
>>>
>>> list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
>>> - seq_printf(m, "HW context %u ", ctx->hw_id);
>>> + seq_puts(m, "HW context ");
>>> + if (!list_empty(&ctx->hw_id_link))
>>> + seq_printf(m, "%x [pin %u]", ctx->hw_id,
>>> + atomic_read(&ctx->hw_id_pin_count));
>>
>> Do you want to put some marker for the unallocated case here?
>
> I was content with absence of marker as indicating it has never had an
> id, or it had been revoked.
>
> Who reads this file anyway? There's not one igt where I've thought this
> would be useful debug info. Maybe I'm wrong...
True, the file as it is looks weak. It was only a question/suggestion
anyway. Perhaps we can later improve the file to list them in a more
modern/meaningful way.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] drm/i915: Reduce context HW ID lifetime
2018-09-05 9:49 ` Tvrtko Ursulin
2018-09-05 10:33 ` Chris Wilson
@ 2018-09-05 11:01 ` Chris Wilson
1 sibling, 0 replies; 19+ messages in thread
From: Chris Wilson @ 2018-09-05 11:01 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx; +Cc: Mika Kuoppala
Quoting Tvrtko Ursulin (2018-09-05 10:49:02)
>
> On 04/09/2018 16:31, Chris Wilson wrote:
> > Future gen reduce the number of bits we will have available to
> > differentiate between contexts, so reduce the lifetime of the ID
> > assignment from that of the context to its current active cycle (i.e.
> > only while it is pinned for use by the HW, will it have a constant ID).
> > This means that instead of a max of 2k allocated contexts (worst case
> > before fun with bit twiddling), we instead have a limit of 2k in flight
> > contexts (minus a few that have been pinned by the kernel or by perf).
> >
> > To reduce the number of contexts id we require, we allocate a context id
> > on first and mark it as pinned for as long as the GEM context itself is,
> > that is we keep it pinned it while active on each engine. If we exhaust
> > our context id space, then we try to reclaim an id from an idle context.
> > In the extreme case where all context ids are pinned by active contexts,
> > we force the system to idle in order to recover ids.
> >
> > We cannot reduce the scope of an HW-ID to an engine (allowing the same
> > gem_context to have different ids on each engine) as in the future we
> > will need to preassign an id before we know which engine the
> > context is being executed on.
> >
> > v2: Improved commentary (Tvrtko) [I tried at least]
> >
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=107788
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
[snip]
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Pushed, thanks for the review. I hope the silence is because it just
works and you guys foresee no problems with guc/hw integration! :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] drm/i915: Reduce context HW ID lifetime
@ 2018-06-02 8:47 Chris Wilson
0 siblings, 0 replies; 19+ messages in thread
From: Chris Wilson @ 2018-06-02 8:47 UTC (permalink / raw)
To: intel-gfx; +Cc: Mika Kuoppala
Future gen reduce the number of bits we will have available to
differentiate between contexts, so reduce the lifetime of the ID
assignment from that of the context to its current active cycle (i.e.
only while it is pinned for use by the HW, will it have a constant ID).
This means that instead of a max of 2k allocated contexts (worst case
before fun with bit twiddling), we instead have a limit of 2k in flight
contexts (minus a few that have been pinned by the kernel or by perf).
We cannot reduce the scope of an HW-ID to an engine (allowing the same
gem_context to have different ids on each engine) as in the future we
will need to preassign an id before we know which engine the
context is being executed on.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_gem_context.c | 201 +++++++++++++-----
drivers/gpu/drm/i915/i915_gem_context.h | 5 +
drivers/gpu/drm/i915/i915_perf.c | 35 ++-
drivers/gpu/drm/i915/i915_request.c | 6 +-
drivers/gpu/drm/i915/i915_request.h | 2 +-
drivers/gpu/drm/i915/i915_trace.h | 2 +-
drivers/gpu/drm/i915/intel_lrc.c | 8 +
drivers/gpu/drm/i915/selftests/mock_context.c | 11 +-
10 files changed, 190 insertions(+), 86 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 15e86d34a81c..ccecaf96cc91 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1955,7 +1955,10 @@ static int i915_context_status(struct seq_file *m, void *unused)
return ret;
list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
- seq_printf(m, "HW context %u ", ctx->hw_id);
+ seq_puts(m, "HW context ");
+ if (!list_empty(&ctx->hw_id_link))
+ seq_printf(m, "%x [pin %u]",
+ ctx->hw_id, ctx->pin_hw_id);
if (ctx->pid) {
struct task_struct *task;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 38157df6ff5c..21729d648269 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1842,6 +1842,7 @@ struct drm_i915_private {
struct ida hw_ida;
#define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */
#define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */
+ struct list_head hw_id_list;
} contexts;
u32 fdi_rx_config;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 94e4db1870aa..aa7ed5a9cb7b 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -136,11 +136,15 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
list_del(&ctx->link);
- ida_simple_remove(&ctx->i915->contexts.hw_ida, ctx->hw_id);
+ if (!list_empty(&ctx->hw_id_link)) {
+ ida_simple_remove(&ctx->i915->contexts.hw_ida, ctx->hw_id);
+ list_del(&ctx->hw_id_link);
+ }
+
kfree_rcu(ctx, rcu);
}
-static void contexts_free(struct drm_i915_private *i915)
+static bool contexts_free(struct drm_i915_private *i915)
{
struct llist_node *freed = llist_del_all(&i915->contexts.free_list);
struct i915_gem_context *ctx, *cn;
@@ -149,6 +153,8 @@ static void contexts_free(struct drm_i915_private *i915)
llist_for_each_entry_safe(ctx, cn, freed, free_link)
i915_gem_context_free(ctx);
+
+ return freed;
}
static void contexts_free_first(struct drm_i915_private *i915)
@@ -203,34 +209,6 @@ static void context_close(struct i915_gem_context *ctx)
i915_gem_context_put(ctx);
}
-static int assign_hw_id(struct drm_i915_private *dev_priv, unsigned *out)
-{
- int ret;
- unsigned int max;
-
- if (INTEL_GEN(dev_priv) >= 11)
- max = GEN11_MAX_CONTEXT_HW_ID;
- else
- max = MAX_CONTEXT_HW_ID;
-
- ret = ida_simple_get(&dev_priv->contexts.hw_ida,
- 0, max, GFP_KERNEL);
- if (ret < 0) {
- /* Contexts are only released when no longer active.
- * Flush any pending retires to hopefully release some
- * stale contexts and try again.
- */
- i915_retire_requests(dev_priv);
- ret = ida_simple_get(&dev_priv->contexts.hw_ida,
- 0, max, GFP_KERNEL);
- if (ret < 0)
- return ret;
- }
-
- *out = ret;
- return 0;
-}
-
static u32 default_desc_template(const struct drm_i915_private *i915,
const struct i915_hw_ppgtt *ppgtt)
{
@@ -267,12 +245,6 @@ __create_hw_context(struct drm_i915_private *dev_priv,
if (ctx == NULL)
return ERR_PTR(-ENOMEM);
- ret = assign_hw_id(dev_priv, &ctx->hw_id);
- if (ret) {
- kfree(ctx);
- return ERR_PTR(ret);
- }
-
kref_init(&ctx->ref);
list_add_tail(&ctx->link, &dev_priv->contexts.list);
ctx->i915 = dev_priv;
@@ -286,6 +258,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
INIT_LIST_HEAD(&ctx->handles_list);
+ INIT_LIST_HEAD(&ctx->hw_id_link);
/* Default context will never have a file_priv */
ret = DEFAULT_CONTEXT_HANDLE;
@@ -422,15 +395,35 @@ i915_gem_context_create_gvt(struct drm_device *dev)
return ctx;
}
+static void
+destroy_kernel_context(struct i915_gem_context **ctxp)
+{
+ struct i915_gem_context *ctx;
+
+ /* Keep the context ref so that we can free it immediately ourselves */
+ ctx = i915_gem_context_get(fetch_and_zero(ctxp));
+ GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
+
+ context_close(ctx);
+ i915_gem_context_free(ctx);
+}
+
struct i915_gem_context *
i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
{
struct i915_gem_context *ctx;
+ int err;
ctx = i915_gem_create_context(i915, NULL);
if (IS_ERR(ctx))
return ctx;
+ err = i915_gem_context_pin_hw_id(ctx);
+ if (err) {
+ destroy_kernel_context(&ctx);
+ return ERR_PTR(err);
+ }
+
i915_gem_context_clear_bannable(ctx);
ctx->sched.priority = prio;
ctx->ring_size = PAGE_SIZE;
@@ -440,17 +433,18 @@ i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
return ctx;
}
-static void
-destroy_kernel_context(struct i915_gem_context **ctxp)
+static void init_contexts(struct drm_i915_private *i915)
{
- struct i915_gem_context *ctx;
+ INIT_LIST_HEAD(&i915->contexts.list);
- /* Keep the context ref so that we can free it immediately ourselves */
- ctx = i915_gem_context_get(fetch_and_zero(ctxp));
- GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
+ /* Using the simple ida interface, the max is limited by sizeof(int) */
+ BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
+ BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
+ ida_init(&i915->contexts.hw_ida);
+ INIT_LIST_HEAD(&i915->contexts.hw_id_list);
- context_close(ctx);
- i915_gem_context_free(ctx);
+ INIT_WORK(&i915->contexts.free_work, contexts_free_worker);
+ init_llist_head(&i915->contexts.free_list);
}
static bool needs_preempt_context(struct drm_i915_private *i915)
@@ -471,14 +465,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
if (ret)
return ret;
- INIT_LIST_HEAD(&dev_priv->contexts.list);
- INIT_WORK(&dev_priv->contexts.free_work, contexts_free_worker);
- init_llist_head(&dev_priv->contexts.free_list);
-
- /* Using the simple ida interface, the max is limited by sizeof(int) */
- BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
- BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
- ida_init(&dev_priv->contexts.hw_ida);
+ init_contexts(dev_priv);
/* lowest priority; idle task */
ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_MIN);
@@ -491,6 +478,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
* all user contexts will have non-zero hw_id.
*/
GEM_BUG_ON(ctx->hw_id);
+ GEM_BUG_ON(!ctx->pin_hw_id);
dev_priv->kernel_context = ctx;
/* highest priority; preempting task */
@@ -528,6 +516,7 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
destroy_kernel_context(&i915->kernel_context);
/* Must free all deferred contexts (via flush_workqueue) first */
+ GEM_BUG_ON(!list_empty(&i915->contexts.hw_id_list));
ida_destroy(&i915->contexts.hw_ida);
}
@@ -940,6 +929,114 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
return ret;
}
+static inline int new_hw_id(struct drm_i915_private *i915)
+{
+ unsigned int max;
+
+ if (INTEL_GEN(i915) >= 11)
+ max = GEN11_MAX_CONTEXT_HW_ID;
+ else
+ max = MAX_CONTEXT_HW_ID;
+
+ return ida_simple_get(&i915->contexts.hw_ida, 0, max, GFP_KERNEL);
+}
+
+static int steal_hw_id(struct drm_i915_private *i915)
+{
+ struct i915_gem_context *ctx, *cn;
+ LIST_HEAD(pinned);
+ int id = 0;
+
+ list_for_each_entry_safe(ctx, cn,
+ &i915->contexts.hw_id_list, hw_id_link) {
+ if (ctx->pin_hw_id) {
+ list_move_tail(&ctx->hw_id_link, &pinned);
+ continue;
+ }
+
+ GEM_BUG_ON(!ctx->hw_id);
+ list_del_init(&ctx->hw_id_link);
+ id = ctx->hw_id;
+ break;
+ }
+
+ list_splice_tail(&pinned, &i915->contexts.hw_id_list);
+ return id;
+}
+
+static int assign_hw_id(struct drm_i915_private *i915, unsigned int *out)
+{
+ int ret;
+
+ ret = new_hw_id(i915);
+ if (unlikely(ret < 0)) {
+ ret = steal_hw_id(i915);
+ if (ret)
+ goto out;
+
+ /*
+ * Contexts are only released when no longer active.
+ * Flush any pending retires to hopefully release some
+ * stale contexts and try again.
+ */
+ if (i915_retire_requests(i915)) {
+ ret = steal_hw_id(i915);
+ if (ret)
+ goto out;
+
+ ret = i915_gem_wait_for_idle(i915,
+ I915_WAIT_INTERRUPTIBLE |
+ I915_WAIT_LOCKED);
+ if (ret)
+ return ret;
+
+ ret = steal_hw_id(i915);
+ if (ret)
+ goto out;
+ }
+
+ /* One last attempt, to determine the errno */
+ ret = -ENOSPC;
+ if (contexts_free(i915)) {
+ ret = new_hw_id(i915);
+ GEM_BUG_ON(ret == 0);
+ }
+ if (ret < 0)
+ return ret;
+ }
+
+out:
+ *out = ret;
+ return 0;
+}
+
+int i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
+{
+ int err;
+
+ lockdep_assert_held(&ctx->i915->drm.struct_mutex);
+
+ GEM_BUG_ON(ctx->pin_hw_id == ~0u);
+ if (!ctx->pin_hw_id++ && list_empty(&ctx->hw_id_link)) {
+ err = assign_hw_id(ctx->i915, &ctx->hw_id);
+ if (err)
+ return err;
+
+ list_add_tail(&ctx->hw_id_link,
+ &ctx->i915->contexts.hw_id_list);
+ }
+
+ return 0;
+}
+
+void i915_gem_context_unpin_hw_id(struct i915_gem_context *ctx)
+{
+ lockdep_assert_held(&ctx->i915->drm.struct_mutex);
+
+ GEM_BUG_ON(ctx->pin_hw_id == 0u);
+ --ctx->pin_hw_id;
+}
+
#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
#include "selftests/mock_context.c"
#include "selftests/i915_gem_context.c"
diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
index b116e4942c10..1e41d983bdd7 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/i915_gem_context.h
@@ -136,6 +136,8 @@ struct i915_gem_context {
* id for the lifetime of the context.
*/
unsigned int hw_id;
+ unsigned int pin_hw_id;
+ struct list_head hw_id_link;
/**
* @user_handle: userspace identifier
@@ -257,6 +259,9 @@ static inline void i915_gem_context_set_force_single_submission(struct i915_gem_
__set_bit(CONTEXT_FORCE_SINGLE_SUBMISSION, &ctx->flags);
}
+int i915_gem_context_pin_hw_id(struct i915_gem_context *ctx);
+void i915_gem_context_unpin_hw_id(struct i915_gem_context *ctx);
+
static inline bool i915_gem_context_is_default(const struct i915_gem_context *c)
{
return c->user_handle == DEFAULT_CONTEXT_HANDLE;
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 4f0eb84b3c00..f449e6a31f75 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1216,31 +1216,24 @@ static int i915_oa_read(struct i915_perf_stream *stream,
static int oa_get_render_ctx_id(struct i915_perf_stream *stream)
{
struct drm_i915_private *dev_priv = stream->dev_priv;
+ struct intel_engine_cs *engine = dev_priv->engine[RCS];
+ struct intel_context *ce;
+ int ret;
- if (HAS_LOGICAL_RING_CONTEXTS(dev_priv)) {
- dev_priv->perf.oa.specific_ctx_id = stream->ctx->hw_id;
- } else {
- struct intel_engine_cs *engine = dev_priv->engine[RCS];
- struct intel_context *ce;
- int ret;
-
- ret = i915_mutex_lock_interruptible(&dev_priv->drm);
- if (ret)
- return ret;
+ ret = i915_mutex_lock_interruptible(&dev_priv->drm);
+ if (ret)
+ return ret;
- /*
- * As the ID is the gtt offset of the context's vma we
- * pin the vma to ensure the ID remains fixed.
- *
- * NB: implied RCS engine...
- */
- ce = intel_context_pin(stream->ctx, engine);
- mutex_unlock(&dev_priv->drm.struct_mutex);
- if (IS_ERR(ce))
- return PTR_ERR(ce);
+ ce = intel_context_pin(stream->ctx, engine);
+ mutex_unlock(&dev_priv->drm.struct_mutex);
+ if (IS_ERR(ce))
+ return PTR_ERR(ce);
- dev_priv->perf.oa.pinned_ctx = ce;
+ dev_priv->perf.oa.pinned_ctx = ce;
+ if (HAS_LOGICAL_RING_CONTEXTS(dev_priv)) {
+ dev_priv->perf.oa.specific_ctx_id = stream->ctx->hw_id;
+ } else {
/*
* Explicitly track the ID (instead of calling
* i915_ggtt_offset() on the fly) considering the difference
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index f187250e60c6..dd8b1a273e8f 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1419,17 +1419,19 @@ static void ring_retire_requests(struct intel_ring *ring)
}
}
-void i915_retire_requests(struct drm_i915_private *i915)
+bool i915_retire_requests(struct drm_i915_private *i915)
{
struct intel_ring *ring, *tmp;
lockdep_assert_held(&i915->drm.struct_mutex);
if (!i915->gt.active_requests)
- return;
+ return false;
list_for_each_entry_safe(ring, tmp, &i915->gt.active_rings, active_link)
ring_retire_requests(ring);
+
+ return true;
}
#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index 491ff81d0fea..479c6afac002 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -318,7 +318,7 @@ static inline bool i915_sched_node_signaled(const struct i915_sched_node *node)
return i915_request_completed(rq);
}
-void i915_retire_requests(struct drm_i915_private *i915);
+bool i915_retire_requests(struct drm_i915_private *i915);
/*
* We treat requests as fences. This is not be to confused with our
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 5d4f78765083..883c90711a4c 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -935,7 +935,7 @@ DECLARE_EVENT_CLASS(i915_context,
TP_fast_assign(
__entry->dev = ctx->i915->drm.primary->index;
__entry->ctx = ctx;
- __entry->hw_id = ctx->hw_id;
+ __entry->hw_id = ctx->pin_hw_id ? ctx->hw_id : ~0u;
__entry->vm = ctx->ppgtt ? &ctx->ppgtt->base : NULL;
),
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 517e92c6a70b..f247b02dbd17 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1344,6 +1344,8 @@ static void execlists_context_destroy(struct intel_context *ce)
static void execlists_context_unpin(struct intel_context *ce)
{
+ i915_gem_context_unpin_hw_id(ce->gem_context);
+
intel_ring_unpin(ce->ring);
ce->state->obj->pin_global--;
@@ -1403,6 +1405,10 @@ __execlists_context_pin(struct intel_engine_cs *engine,
if (ret)
goto unpin_map;
+ ret = i915_gem_context_pin_hw_id(ctx);
+ if (ret)
+ goto unpin_ring;
+
intel_lr_context_descriptor_update(ctx, engine, ce);
ce->lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
@@ -1414,6 +1420,8 @@ __execlists_context_pin(struct intel_engine_cs *engine,
i915_gem_context_get(ctx);
return ce;
+unpin_ring:
+ intel_ring_unpin(ce->ring);
unpin_map:
i915_gem_object_unpin_map(ce->state->obj);
unpin_vma:
diff --git a/drivers/gpu/drm/i915/selftests/mock_context.c b/drivers/gpu/drm/i915/selftests/mock_context.c
index 8904f1ce64e3..d937bdff26f9 100644
--- a/drivers/gpu/drm/i915/selftests/mock_context.c
+++ b/drivers/gpu/drm/i915/selftests/mock_context.c
@@ -43,6 +43,7 @@ mock_context(struct drm_i915_private *i915,
INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
INIT_LIST_HEAD(&ctx->handles_list);
+ INIT_LIST_HEAD(&ctx->hw_id_link);
for (n = 0; n < ARRAY_SIZE(ctx->__engine); n++) {
struct intel_context *ce = &ctx->__engine[n];
@@ -50,11 +51,9 @@ mock_context(struct drm_i915_private *i915,
ce->gem_context = ctx;
}
- ret = ida_simple_get(&i915->contexts.hw_ida,
- 0, MAX_CONTEXT_HW_ID, GFP_KERNEL);
+ ret = i915_gem_context_pin_hw_id(ctx);
if (ret < 0)
goto err_handles;
- ctx->hw_id = ret;
if (name) {
ctx->name = kstrdup(name, GFP_KERNEL);
@@ -85,11 +84,7 @@ void mock_context_close(struct i915_gem_context *ctx)
void mock_init_contexts(struct drm_i915_private *i915)
{
- INIT_LIST_HEAD(&i915->contexts.list);
- ida_init(&i915->contexts.hw_ida);
-
- INIT_WORK(&i915->contexts.free_work, contexts_free_worker);
- init_llist_head(&i915->contexts.free_list);
+ init_contexts(i915);
}
struct i915_gem_context *
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-09-05 11:01 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-30 10:24 [PATCH] drm/i915: Reduce context HW ID lifetime Chris Wilson
2018-08-30 12:38 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Reduce context HW ID lifetime (rev2) Patchwork
2018-08-30 12:39 ` ✗ Fi.CI.SPARSE: " Patchwork
2018-08-30 12:58 ` ✓ Fi.CI.BAT: success " Patchwork
2018-08-30 16:23 ` [PATCH] drm/i915: Reduce context HW ID lifetime Tvrtko Ursulin
2018-08-31 12:36 ` Chris Wilson
2018-09-03 9:59 ` Tvrtko Ursulin
2018-09-04 13:48 ` Chris Wilson
2018-08-30 17:10 ` ✓ Fi.CI.IGT: success for drm/i915: Reduce context HW ID lifetime (rev2) Patchwork
2018-09-04 15:31 ` [PATCH v2] drm/i915: Reduce context HW ID lifetime Chris Wilson
2018-09-05 9:49 ` Tvrtko Ursulin
2018-09-05 10:33 ` Chris Wilson
2018-09-05 10:55 ` Tvrtko Ursulin
2018-09-05 11:01 ` Chris Wilson
2018-09-04 16:02 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Reduce context HW ID lifetime (rev3) Patchwork
2018-09-04 16:03 ` ✗ Fi.CI.SPARSE: " Patchwork
2018-09-04 16:19 ` ✓ Fi.CI.BAT: success " Patchwork
2018-09-04 22:28 ` ✓ Fi.CI.IGT: " Patchwork
-- strict thread matches above, loose matches on Subject: below --
2018-06-02 8:47 [PATCH] drm/i915: Reduce context HW ID lifetime Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.