* Premature unpinning
@ 2016-04-19 11:40 Chris Wilson
2016-04-19 11:40 ` [PATCH v2 01/12] drm/i915: Mark the current context as lost on suspend Chris Wilson
` (14 more replies)
0 siblings, 15 replies; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 11:40 UTC (permalink / raw)
To: intel-gfx
Rebased patches for CI and Tvrkto
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v2 01/12] drm/i915: Mark the current context as lost on suspend
2016-04-19 11:40 Premature unpinning Chris Wilson
@ 2016-04-19 11:40 ` Chris Wilson
2016-04-19 14:12 ` [PATCH v2] " Chris Wilson
2016-04-19 11:40 ` [PATCH v2 02/12] drm/i915: L3 cache remapping is part of context switching Chris Wilson
` (13 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 11:40 UTC (permalink / raw)
To: intel-gfx
In order to force a reload of the context image upon resume, we first
need to mark its absence on suspend. Currently we are failing to restore
the golden context state and any context w/a to the default context
after resume.
One oversight corrected, is that we had forgotten to reapply the L3
remapping when restoring the lost default context.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_gem.c | 1 +
drivers/gpu/drm/i915/i915_gem_context.c | 47 +++++++++++++++++----------------
3 files changed, 26 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 85102ad75962..595037bec2de 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3299,6 +3299,7 @@ void i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj);
/* i915_gem_context.c */
int __must_check i915_gem_context_init(struct drm_device *dev);
+void i915_gem_context_lost(struct drm_i915_private *dev_priv);
void i915_gem_context_fini(struct drm_device *dev);
void i915_gem_context_reset(struct drm_device *dev);
int i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6ce2c31b9a81..e7fe29857e23 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4711,6 +4711,7 @@ i915_gem_suspend(struct drm_device *dev)
i915_gem_retire_requests(dev);
i915_gem_stop_engines(dev);
+ i915_gem_context_lost(dev_priv);
mutex_unlock(&dev->struct_mutex);
cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index e5acc3916f75..c306c0b8435c 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -90,6 +90,8 @@
#include "i915_drv.h"
#include "i915_trace.h"
+#define ALL_L3_SLICES(dev) (1 << NUM_L3_SLICES(dev)) - 1
+
/* This is a HW constraint. The value below is the largest known requirement
* I've seen in a spec to date, and that was a workaround for a non-shipping
* part. It should be safe to decrease this, but it's more future proof as is.
@@ -249,7 +251,7 @@ __create_hw_context(struct drm_device *dev,
/* NB: Mark all slices as needing a remap so that when the context first
* loads it will restore whatever remap state already exists. If there
* is no remap info, it will be a NOP. */
- ctx->remap_slice = (1 << NUM_L3_SLICES(dev)) - 1;
+ ctx->remap_slice = ALL_L3_SLICES(dev_priv);
ctx->hang_stats.ban_period_seconds = DRM_I915_CTX_BAN_PERIOD;
@@ -336,7 +338,6 @@ static void i915_gem_context_unpin(struct intel_context *ctx,
void i915_gem_context_reset(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- int i;
if (i915.enable_execlists) {
struct intel_context *ctx;
@@ -345,17 +346,7 @@ void i915_gem_context_reset(struct drm_device *dev)
intel_lr_context_reset(dev_priv, ctx);
}
- for (i = 0; i < I915_NUM_ENGINES; i++) {
- struct intel_engine_cs *engine = &dev_priv->engine[i];
-
- if (engine->last_context) {
- i915_gem_context_unpin(engine->last_context, engine);
- engine->last_context = NULL;
- }
- }
-
- /* Force the GPU state to be reinitialised on enabling */
- dev_priv->kernel_context->legacy_hw_ctx.initialized = false;
+ i915_gem_context_lost(dev_priv);
}
int i915_gem_context_init(struct drm_device *dev)
@@ -403,11 +394,30 @@ int i915_gem_context_init(struct drm_device *dev)
return 0;
}
+void i915_gem_context_lost(struct drm_i915_private *dev_priv)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(dev_priv->engine); i++) {
+ struct intel_engine_cs *engine = &dev_priv->engine[i];
+
+ if (engine->last_context) {
+ i915_gem_context_unpin(engine->last_context, engine);
+ engine->last_context = NULL;
+ }
+ }
+
+ /* Force the GPU state to be reinitialised on enabling */
+ dev_priv->kernel_context->legacy_hw_ctx.initialized = false;
+ dev_priv->kernel_context->remap_slice = ALL_L3_SLICES(dev_priv);
+}
+
void i915_gem_context_fini(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_context *dctx = dev_priv->kernel_context;
- int i;
+
+ i915_gem_context_lost(dev_priv);
if (dctx->legacy_hw_ctx.rcs_state) {
/* The only known way to stop the gpu from accessing the hw context is
@@ -426,15 +436,6 @@ void i915_gem_context_fini(struct drm_device *dev)
i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
}
- for (i = I915_NUM_ENGINES; --i >= 0;) {
- struct intel_engine_cs *engine = &dev_priv->engine[i];
-
- if (engine->last_context) {
- i915_gem_context_unpin(engine->last_context, engine);
- engine->last_context = NULL;
- }
- }
-
i915_gem_context_unreference(dctx);
dev_priv->kernel_context = NULL;
}
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 02/12] drm/i915: L3 cache remapping is part of context switching
2016-04-19 11:40 Premature unpinning Chris Wilson
2016-04-19 11:40 ` [PATCH v2 01/12] drm/i915: Mark the current context as lost on suspend Chris Wilson
@ 2016-04-19 11:40 ` Chris Wilson
2016-04-19 11:40 ` [PATCH v2 03/12] drm/i915: Consolidate L3 remapping LRI Chris Wilson
` (12 subsequent siblings)
14 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 11:40 UTC (permalink / raw)
To: intel-gfx
Move the i915_gem_l3_remap function such that it next to the context
switching, which is where we perform the L3 remap.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 31 -------------------------------
drivers/gpu/drm/i915/i915_gem_context.c | 31 +++++++++++++++++++++++++++++++
2 files changed, 31 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e7fe29857e23..ec928c8701c3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4730,37 +4730,6 @@ err:
return ret;
}
-int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice)
-{
- struct intel_engine_cs *engine = req->engine;
- struct drm_device *dev = engine->dev;
- struct drm_i915_private *dev_priv = dev->dev_private;
- u32 *remap_info = dev_priv->l3_parity.remap_info[slice];
- int i, ret;
-
- if (!HAS_L3_DPF(dev) || !remap_info)
- return 0;
-
- ret = intel_ring_begin(req, GEN7_L3LOG_SIZE / 4 * 3);
- if (ret)
- return ret;
-
- /*
- * Note: We do not worry about the concurrent register cacheline hang
- * here because no other code should access these registers other than
- * at initialization time.
- */
- for (i = 0; i < GEN7_L3LOG_SIZE / 4; i++) {
- intel_ring_emit(engine, MI_LOAD_REGISTER_IMM(1));
- intel_ring_emit_reg(engine, GEN7_L3LOG(slice, i));
- intel_ring_emit(engine, remap_info[i]);
- }
-
- intel_ring_advance(engine);
-
- return ret;
-}
-
void i915_gem_init_swizzling(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index c306c0b8435c..a4b143d3981b 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -610,6 +610,37 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
return ret;
}
+int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice)
+{
+ struct intel_engine_cs *engine = req->engine;
+ struct drm_device *dev = engine->dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ u32 *remap_info = dev_priv->l3_parity.remap_info[slice];
+ int i, ret;
+
+ if (!HAS_L3_DPF(dev) || !remap_info)
+ return 0;
+
+ ret = intel_ring_begin(req, GEN7_L3LOG_SIZE / 4 * 3);
+ if (ret)
+ return ret;
+
+ /*
+ * Note: We do not worry about the concurrent register cacheline hang
+ * here because no other code should access these registers other than
+ * at initialization time.
+ */
+ for (i = 0; i < GEN7_L3LOG_SIZE / 4; i++) {
+ intel_ring_emit(engine, MI_LOAD_REGISTER_IMM(1));
+ intel_ring_emit_reg(engine, GEN7_L3LOG(slice, i));
+ intel_ring_emit(engine, remap_info[i]);
+ }
+
+ intel_ring_advance(engine);
+
+ return ret;
+}
+
static inline bool skip_rcs_switch(struct intel_engine_cs *engine,
struct intel_context *to)
{
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 03/12] drm/i915: Consolidate L3 remapping LRI
2016-04-19 11:40 Premature unpinning Chris Wilson
2016-04-19 11:40 ` [PATCH v2 01/12] drm/i915: Mark the current context as lost on suspend Chris Wilson
2016-04-19 11:40 ` [PATCH v2 02/12] drm/i915: L3 cache remapping is part of context switching Chris Wilson
@ 2016-04-19 11:40 ` Chris Wilson
2016-04-19 11:40 ` [PATCH v2 04/12] drm/i915: Remove early l3-remap Chris Wilson
` (11 subsequent siblings)
14 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 11:40 UTC (permalink / raw)
To: intel-gfx
We can use a single MI_LOAD_REGISTER_IMM command packet to write all the
L3 remapping registers, shrinking the number of bytes required to emit
the context switch.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_gem_context.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index a4b143d3981b..bde757f790d5 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -612,16 +612,14 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice)
{
+ u32 *remap_info = req->i915->l3_parity.remap_info[slice];
struct intel_engine_cs *engine = req->engine;
- struct drm_device *dev = engine->dev;
- struct drm_i915_private *dev_priv = dev->dev_private;
- u32 *remap_info = dev_priv->l3_parity.remap_info[slice];
int i, ret;
- if (!HAS_L3_DPF(dev) || !remap_info)
+ if (!remap_info)
return 0;
- ret = intel_ring_begin(req, GEN7_L3LOG_SIZE / 4 * 3);
+ ret = intel_ring_begin(req, GEN7_L3LOG_SIZE/4 * 2 + 2);
if (ret)
return ret;
@@ -630,15 +628,15 @@ int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice)
* here because no other code should access these registers other than
* at initialization time.
*/
- for (i = 0; i < GEN7_L3LOG_SIZE / 4; i++) {
- intel_ring_emit(engine, MI_LOAD_REGISTER_IMM(1));
+ intel_ring_emit(engine, MI_LOAD_REGISTER_IMM(GEN7_L3LOG_SIZE/4));
+ for (i = 0; i < GEN7_L3LOG_SIZE/4; i++) {
intel_ring_emit_reg(engine, GEN7_L3LOG(slice, i));
intel_ring_emit(engine, remap_info[i]);
}
-
+ intel_ring_emit(engine, MI_NOOP);
intel_ring_advance(engine);
- return ret;
+ return 0;
}
static inline bool skip_rcs_switch(struct intel_engine_cs *engine,
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 04/12] drm/i915: Remove early l3-remap
2016-04-19 11:40 Premature unpinning Chris Wilson
` (2 preceding siblings ...)
2016-04-19 11:40 ` [PATCH v2 03/12] drm/i915: Consolidate L3 remapping LRI Chris Wilson
@ 2016-04-19 11:40 ` Chris Wilson
2016-04-19 11:40 ` [PATCH v2 05/12] drm/i915: Rearrange switch_context to load the aliasing ppgtt on first use Chris Wilson
` (10 subsequent siblings)
14 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 11:40 UTC (permalink / raw)
To: intel-gfx
Since we do the l3-remap on context switch, we can remove the redundant
early call to set the mapping prior to performing the first context
switch.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 1 -
drivers/gpu/drm/i915/i915_gem.c | 10 +---------
drivers/gpu/drm/i915/i915_gem_context.c | 4 ++--
3 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 595037bec2de..7b347c20880d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3145,7 +3145,6 @@ bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
int __must_check i915_gem_init(struct drm_device *dev);
int i915_gem_init_engines(struct drm_device *dev);
int __must_check i915_gem_init_hw(struct drm_device *dev);
-int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice);
void i915_gem_init_swizzling(struct drm_device *dev);
void i915_gem_cleanup_engines(struct drm_device *dev);
int __must_check i915_gpu_idle(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ec928c8701c3..4874d265d528 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4834,7 +4834,7 @@ i915_gem_init_hw(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_engine_cs *engine;
- int ret, j;
+ int ret;
if (INTEL_INFO(dev)->gen < 6 && !intel_enable_gtt())
return -EIO;
@@ -4916,14 +4916,6 @@ i915_gem_init_hw(struct drm_device *dev)
break;
}
- if (engine->id == RCS) {
- for (j = 0; j < NUM_L3_SLICES(dev); j++) {
- ret = i915_gem_l3_remap(req, j);
- if (ret)
- goto err_request;
- }
- }
-
ret = i915_ppgtt_init_ring(req);
if (ret)
goto err_request;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index bde757f790d5..e44bf9ad8d96 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -610,7 +610,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
return ret;
}
-int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice)
+static int remap_l3(struct drm_i915_gem_request *req, int slice)
{
u32 *remap_info = req->i915->l3_parity.remap_info[slice];
struct intel_engine_cs *engine = req->engine;
@@ -808,7 +808,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
if (!(to->remap_slice & (1<<i)))
continue;
- ret = i915_gem_l3_remap(req, i);
+ ret = remap_l3(req, i);
if (ret)
return ret;
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 05/12] drm/i915: Rearrange switch_context to load the aliasing ppgtt on first use
2016-04-19 11:40 Premature unpinning Chris Wilson
` (3 preceding siblings ...)
2016-04-19 11:40 ` [PATCH v2 04/12] drm/i915: Remove early l3-remap Chris Wilson
@ 2016-04-19 11:40 ` Chris Wilson
2016-04-19 11:40 ` [PATCH v2 06/12] drm/i915: Assign every HW context a unique ID Chris Wilson
` (9 subsequent siblings)
14 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 11:40 UTC (permalink / raw)
To: intel-gfx
The code to switch_mm() is already handled by i915_switch_context(), the
only difference required to setup the aliasing ppgtt is that we need to
emit te switch_mm() on the first context, i.e. when transitioning from
engine->last_context == NULL. This allows us to defer the
initialisation of the GPU from early device initialisation to first use,
which should marginally speed up both. The caveat is that we then defer
the context initialisation until first use - i.e. we cannot assume that
the GPU engines are initialised. For example, this means that power
contexts for rc6 (Ironlake) need to explicitly loaded, as they are.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 1 -
drivers/gpu/drm/i915/i915_gem.c | 28 ---------------------
drivers/gpu/drm/i915/i915_gem_context.c | 43 +++++++++------------------------
drivers/gpu/drm/i915/i915_gem_gtt.c | 14 -----------
drivers/gpu/drm/i915/i915_gem_gtt.h | 1 -
5 files changed, 12 insertions(+), 75 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7b347c20880d..ed38e2ea76a5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3302,7 +3302,6 @@ void i915_gem_context_lost(struct drm_i915_private *dev_priv);
void i915_gem_context_fini(struct drm_device *dev);
void i915_gem_context_reset(struct drm_device *dev);
int i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
-int i915_gem_context_enable(struct drm_i915_gem_request *req);
void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
int i915_switch_context(struct drm_i915_gem_request *req);
struct intel_context *
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 4874d265d528..b95d5f83d3b0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4906,34 +4906,6 @@ i915_gem_init_hw(struct drm_device *dev)
if (ret)
goto out;
- /* Now it is safe to go back round and do everything else: */
- for_each_engine(engine, dev_priv) {
- struct drm_i915_gem_request *req;
-
- req = i915_gem_request_alloc(engine, NULL);
- if (IS_ERR(req)) {
- ret = PTR_ERR(req);
- break;
- }
-
- ret = i915_ppgtt_init_ring(req);
- if (ret)
- goto err_request;
-
- ret = i915_gem_context_enable(req);
- if (ret)
- goto err_request;
-
-err_request:
- i915_add_request_no_flush(req);
- if (ret) {
- DRM_ERROR("Failed to enable %s, error=%d\n",
- engine->name, ret);
- i915_gem_cleanup_engines(dev);
- break;
- }
- }
-
out:
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
return ret;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index e44bf9ad8d96..7557566516fe 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -440,27 +440,6 @@ void i915_gem_context_fini(struct drm_device *dev)
dev_priv->kernel_context = NULL;
}
-int i915_gem_context_enable(struct drm_i915_gem_request *req)
-{
- struct intel_engine_cs *engine = req->engine;
- int ret;
-
- if (i915.enable_execlists) {
- if (engine->init_context == NULL)
- return 0;
-
- ret = engine->init_context(req);
- } else
- ret = i915_switch_context(req);
-
- if (ret) {
- DRM_ERROR("ring init context: %d\n", ret);
- return ret;
- }
-
- return 0;
-}
-
static int context_idr_cleanup(int id, void *p, void *data)
{
struct intel_context *ctx = p;
@@ -659,7 +638,7 @@ static bool
needs_pd_load_pre(struct intel_engine_cs *engine, struct intel_context *to)
{
if (!to->ppgtt)
- return false;
+ return engine->last_context == NULL;
if (engine->last_context == to &&
!(intel_engine_flag(engine) & to->ppgtt->pd_dirty_rings))
@@ -693,6 +672,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
{
struct intel_context *to = req->ctx;
struct intel_engine_cs *engine = req->engine;
+ struct i915_hw_ppgtt *ppgtt = to->ppgtt ?: req->i915->mm.aliasing_ppgtt;
struct intel_context *from;
u32 hw_flags;
int ret, i;
@@ -734,7 +714,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
* Register Immediate commands in Ring Buffer before submitting
* a context."*/
trace_switch_mm(engine, to);
- ret = to->ppgtt->switch_mm(to->ppgtt, req);
+ ret = ppgtt->switch_mm(ppgtt, req);
if (ret)
goto unpin_out;
}
@@ -745,8 +725,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
* space. This means we must enforce that a page table load
* occur when this occurs. */
hw_flags = MI_RESTORE_INHIBIT;
- else if (to->ppgtt &&
- intel_engine_flag(engine) & to->ppgtt->pd_dirty_rings)
+ else if (ppgtt && intel_engine_flag(engine) & ppgtt->pd_dirty_rings)
hw_flags = MI_FORCE_RESTORE;
else
hw_flags = 0;
@@ -791,7 +770,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
*/
if (needs_pd_load_post(to, hw_flags)) {
trace_switch_mm(engine, to);
- ret = to->ppgtt->switch_mm(to->ppgtt, req);
+ ret = ppgtt->switch_mm(ppgtt, req);
/* The hardware context switch is emitted, but we haven't
* actually changed the state - so it's probably safe to bail
* here. Still, let the user know something dangerous has
@@ -801,8 +780,8 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
return ret;
}
- if (to->ppgtt)
- to->ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine);
+ if (ppgtt)
+ ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine);
for (i = 0; i < MAX_L3_SLICES; i++) {
if (!(to->remap_slice & (1<<i)))
@@ -857,15 +836,17 @@ int i915_switch_context(struct drm_i915_gem_request *req)
struct intel_context *to = req->ctx;
if (needs_pd_load_pre(engine, to)) {
+ struct i915_hw_ppgtt *ppgtt;
int ret;
+ ppgtt = to->ppgtt ?: req->i915->mm.aliasing_ppgtt;
+
trace_switch_mm(engine, to);
- ret = to->ppgtt->switch_mm(to->ppgtt, req);
+ ret = ppgtt->switch_mm(ppgtt, req);
if (ret)
return ret;
- /* Doing a PD load always reloads the page dirs */
- to->ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine);
+ ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine);
}
if (to != engine->last_context) {
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 9f165feb54ae..10de3c57ac8a 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2180,20 +2180,6 @@ int i915_ppgtt_init_hw(struct drm_device *dev)
return 0;
}
-int i915_ppgtt_init_ring(struct drm_i915_gem_request *req)
-{
- struct drm_i915_private *dev_priv = req->i915;
- struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt;
-
- if (i915.enable_execlists)
- return 0;
-
- if (!ppgtt)
- return 0;
-
- return ppgtt->switch_mm(ppgtt, req);
-}
-
struct i915_hw_ppgtt *
i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private *fpriv)
{
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index d7dd3d8a8758..333a2fc62b43 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -519,7 +519,6 @@ void i915_ggtt_cleanup_hw(struct drm_device *dev);
int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt);
int i915_ppgtt_init_hw(struct drm_device *dev);
-int i915_ppgtt_init_ring(struct drm_i915_gem_request *req);
void i915_ppgtt_release(struct kref *kref);
struct i915_hw_ppgtt *i915_ppgtt_create(struct drm_device *dev,
struct drm_i915_file_private *fpriv);
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 06/12] drm/i915: Assign every HW context a unique ID
2016-04-19 11:40 Premature unpinning Chris Wilson
` (4 preceding siblings ...)
2016-04-19 11:40 ` [PATCH v2 05/12] drm/i915: Rearrange switch_context to load the aliasing ppgtt on first use Chris Wilson
@ 2016-04-19 11:40 ` Chris Wilson
2016-04-19 11:40 ` [PATCH v2 07/12] drm/i915: Replace the pinned context address with its " Chris Wilson
` (8 subsequent siblings)
14 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 11:40 UTC (permalink / raw)
To: intel-gfx
The hardware tracks contexts and expects all live contexts (those active
on the hardware) to have a unique identifier. This is used by the
hardware to assign pagefaults and the like to a particular context.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
drivers/gpu/drm/i915/i915_drv.h | 10 ++++++++++
drivers/gpu/drm/i915/i915_gem_context.c | 34 +++++++++++++++++++++++++++++++++
3 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 931dc6086f3b..d46413969daa 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2001,7 +2001,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
ctx->legacy_hw_ctx.rcs_state == NULL)
continue;
- seq_puts(m, "HW context ");
+ seq_printf(m, "HW context %u ", ctx->hw_id);
describe_ctx(m, ctx);
if (ctx == dev_priv->kernel_context)
seq_printf(m, "(kernel context) ");
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ed38e2ea76a5..c71220f52bc7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -851,6 +851,9 @@ struct intel_context {
struct i915_ctx_hang_stats hang_stats;
struct i915_hw_ppgtt *ppgtt;
+ /* Unique identifier for this context, used by the hw for tracking */
+ unsigned hw_id;
+
/* Legacy ring buffer submission */
struct {
struct drm_i915_gem_object *rcs_state;
@@ -1838,6 +1841,13 @@ struct drm_i915_private {
DECLARE_HASHTABLE(mm_structs, 7);
struct mutex mm_lock;
+ /* The hw wants to have a stable context identifier for the lifetime
+ * of the context (for OA, PASID, faults, etc). This is limited
+ * in execlists to 20 bits.
+ */
+ struct ida context_hw_ida;
+#define MAX_CONTEXT_HW_ID (1<<20)
+
/* Kernel Modesetting */
struct drm_crtc *plane_to_crtc_mapping[I915_MAX_PIPES];
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 7557566516fe..eef1478ed128 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -171,6 +171,8 @@ void i915_gem_context_free(struct kref *ctx_ref)
if (ctx->legacy_hw_ctx.rcs_state)
drm_gem_object_unreference(&ctx->legacy_hw_ctx.rcs_state->base);
list_del(&ctx->link);
+
+ ida_simple_remove(&ctx->i915->context_hw_ida, ctx->hw_id);
kfree(ctx);
}
@@ -211,6 +213,28 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size)
return obj;
}
+static int assign_hw_id(struct drm_i915_private *dev_priv, unsigned *out)
+{
+ int ret;
+
+ ret = ida_simple_get(&dev_priv->context_hw_ida,
+ 0, MAX_CONTEXT_HW_ID, 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_gem_retire_requests(dev_priv->dev);
+ ret = ida_simple_get(&dev_priv->context_hw_ida,
+ 0, MAX_CONTEXT_HW_ID, GFP_KERNEL);
+ if (ret < 0)
+ return ret;
+ }
+
+ *out = ret;
+ return 0;
+}
+
static struct intel_context *
__create_hw_context(struct drm_device *dev,
struct drm_i915_file_private *file_priv)
@@ -227,6 +251,12 @@ __create_hw_context(struct drm_device *dev,
list_add_tail(&ctx->link, &dev_priv->context_list);
ctx->i915 = dev_priv;
+ ret = assign_hw_id(dev_priv, &ctx->hw_id);
+ if (ret) {
+ kfree(ctx);
+ return ERR_PTR(ret);
+ }
+
if (dev_priv->hw_context_size) {
struct drm_i915_gem_object *obj =
i915_gem_alloc_context_obj(dev, dev_priv->hw_context_size);
@@ -366,6 +396,10 @@ int i915_gem_context_init(struct drm_device *dev)
}
}
+ /* Using the simple ida interface, the max is limited by sizeof(int) */
+ BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
+ ida_init(&dev_priv->context_hw_ida);
+
if (i915.enable_execlists) {
/* NB: intentionally left blank. We will allocate our own
* backing objects as we need them, thank you very much */
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 07/12] drm/i915: Replace the pinned context address with its unique ID
2016-04-19 11:40 Premature unpinning Chris Wilson
` (5 preceding siblings ...)
2016-04-19 11:40 ` [PATCH v2 06/12] drm/i915: Assign every HW context a unique ID Chris Wilson
@ 2016-04-19 11:40 ` Chris Wilson
2016-04-19 11:40 ` [PATCH v2 08/12] drm/i915: Refactor execlists default context pinning Chris Wilson
` (7 subsequent siblings)
14 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 11:40 UTC (permalink / raw)
To: intel-gfx
Rather than reuse the current location of the context in the global GTT
for its hardware identifier, use the context's unique ID assigned to it
for its whole lifetime.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++-------
drivers/gpu/drm/i915/intel_lrc.c | 36 ++++++------------------------------
drivers/gpu/drm/i915/intel_lrc.h | 3 ---
3 files changed, 11 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d46413969daa..f775451bd0b6 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2043,15 +2043,13 @@ static void i915_dump_lrc_obj(struct seq_file *m,
struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
unsigned long ggtt_offset = 0;
+ seq_printf(m, "CONTEXT: %s %u\n", engine->name, ctx->hw_id);
+
if (ctx_obj == NULL) {
- seq_printf(m, "Context on %s with no gem object\n",
- engine->name);
+ seq_puts(m, "\tNot allocated\n");
return;
}
- seq_printf(m, "CONTEXT: %s %u\n", engine->name,
- intel_execlists_ctx_id(ctx, engine));
-
if (!i915_gem_obj_ggtt_bound(ctx_obj))
seq_puts(m, "\tNot bound in GGTT\n");
else
@@ -2170,8 +2168,8 @@ static int i915_execlists(struct seq_file *m, void *data)
seq_printf(m, "\t%d requests in queue\n", count);
if (head_req) {
- seq_printf(m, "\tHead request id: %u\n",
- intel_execlists_ctx_id(head_req->ctx, engine));
+ seq_printf(m, "\tHead request context: %u\n",
+ head_req->ctx->hw_id);
seq_printf(m, "\tHead request tail: %u\n",
head_req->tail);
}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 1562a75ac9d1..dedd82aea386 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -314,14 +314,12 @@ static void
intel_lr_context_descriptor_update(struct intel_context *ctx,
struct intel_engine_cs *engine)
{
- uint64_t lrca, desc;
+ u64 desc;
- lrca = ctx->engine[engine->id].lrc_vma->node.start +
- LRC_PPHWSP_PN * PAGE_SIZE;
-
- desc = engine->ctx_desc_template; /* bits 0-11 */
- desc |= lrca; /* bits 12-31 */
- desc |= (lrca >> PAGE_SHIFT) << GEN8_CTX_ID_SHIFT; /* bits 32-51 */
+ desc = engine->ctx_desc_template; /* bits 0-11 */
+ desc |= ctx->engine[engine->id].lrc_vma->node.start +
+ LRC_PPHWSP_PN * PAGE_SIZE; /* bits 12-31 */
+ desc |= (u64)ctx->hw_id << GEN8_CTX_ID_SHIFT; /* bits 32-51 */
ctx->engine[engine->id].lrc_desc = desc;
}
@@ -332,28 +330,6 @@ uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
return ctx->engine[engine->id].lrc_desc;
}
-/**
- * intel_execlists_ctx_id() - get the Execlists Context ID
- * @ctx: Context to get the ID for
- * @ring: Engine to get the ID for
- *
- * Do not confuse with ctx->id! Unfortunately we have a name overload
- * here: the old context ID we pass to userspace as a handler so that
- * they can refer to a context, and the new context ID we pass to the
- * ELSP so that the GPU can inform us of the context status via
- * interrupts.
- *
- * The context ID is a portion of the context descriptor, so we can
- * just extract the required part from the cached descriptor.
- *
- * Return: 20-bits globally unique context ID.
- */
-u32 intel_execlists_ctx_id(struct intel_context *ctx,
- struct intel_engine_cs *engine)
-{
- return intel_lr_context_descriptor(ctx, engine) >> GEN8_CTX_ID_SHIFT;
-}
-
static void execlists_elsp_write(struct drm_i915_gem_request *rq0,
struct drm_i915_gem_request *rq1)
{
@@ -499,7 +475,7 @@ execlists_check_remove_request(struct intel_engine_cs *engine, u32 request_id)
if (!head_req)
return 0;
- if (unlikely(intel_execlists_ctx_id(head_req->ctx, engine) != request_id))
+ if (unlikely(head_req->ctx->hw_id != request_id))
return 0;
WARN(head_req->elsp_submitted == 0, "Never submitted head request\n");
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 461f1ef9b5c1..b17ab79333aa 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -114,9 +114,6 @@ void intel_lr_context_reset(struct drm_i915_private *dev_priv,
uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
struct intel_engine_cs *engine);
-u32 intel_execlists_ctx_id(struct intel_context *ctx,
- struct intel_engine_cs *engine);
-
/* Execlists */
int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists);
struct i915_execbuffer_params;
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 08/12] drm/i915: Refactor execlists default context pinning
2016-04-19 11:40 Premature unpinning Chris Wilson
` (6 preceding siblings ...)
2016-04-19 11:40 ` [PATCH v2 07/12] drm/i915: Replace the pinned context address with its " Chris Wilson
@ 2016-04-19 11:40 ` Chris Wilson
2016-04-19 12:25 ` Tvrtko Ursulin
2016-04-20 14:07 ` Mika Kuoppala
2016-04-19 11:40 ` [PATCH v2 09/12] drm/i915: Move context initialisation to first-use Chris Wilson
` (6 subsequent siblings)
14 siblings, 2 replies; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 11:40 UTC (permalink / raw)
To: intel-gfx
Refactor pinning and unpinning of contexts, such that the default
context for an engine is pinned during initialisation and unpinned
during teardown (pinning of the context handles the reference counting).
Thus we can eliminate the special case handling of the default context
that was required to mask that it was not being pinned normally.
v2: Rebalance context_queue after rebasing.
v3: Rebase to -nightly (not 40 patches in)
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/intel_lrc.c | 107 ++++++++++++++----------------------
3 files changed, 43 insertions(+), 71 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f775451bd0b6..e81a7504656e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2095,9 +2095,8 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
return ret;
list_for_each_entry(ctx, &dev_priv->context_list, link)
- if (ctx != dev_priv->kernel_context)
- for_each_engine(engine, dev_priv)
- i915_dump_lrc_obj(m, ctx, engine);
+ for_each_engine(engine, dev_priv)
+ i915_dump_lrc_obj(m, ctx, engine);
mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b95d5f83d3b0..261185281e78 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2719,7 +2719,7 @@ void i915_gem_request_free(struct kref *req_ref)
i915_gem_request_remove_from_client(req);
if (ctx) {
- if (i915.enable_execlists && ctx != req->i915->kernel_context)
+ if (i915.enable_execlists)
intel_lr_context_unpin(ctx, req->engine);
i915_gem_context_unreference(ctx);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index dedd82aea386..e064a6ae2d97 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -588,9 +588,7 @@ static void execlists_context_queue(struct drm_i915_gem_request *request)
struct drm_i915_gem_request *cursor;
int num_elements = 0;
- if (request->ctx != request->i915->kernel_context)
- intel_lr_context_pin(request->ctx, engine);
-
+ intel_lr_context_pin(request->ctx, request->engine);
i915_gem_request_reference(request);
spin_lock_bh(&engine->execlist_lock);
@@ -691,10 +689,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
return ret;
}
- if (request->ctx != request->i915->kernel_context)
- ret = intel_lr_context_pin(request->ctx, request->engine);
-
- return ret;
+ return intel_lr_context_pin(request->ctx, request->engine);
}
static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
@@ -774,12 +769,8 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
if (engine->last_context != request->ctx) {
if (engine->last_context)
intel_lr_context_unpin(engine->last_context, engine);
- if (request->ctx != request->i915->kernel_context) {
- intel_lr_context_pin(request->ctx, engine);
- engine->last_context = request->ctx;
- } else {
- engine->last_context = NULL;
- }
+ intel_lr_context_pin(request->ctx, engine);
+ engine->last_context = request->ctx;
}
if (dev_priv->guc.execbuf_client)
@@ -1002,12 +993,7 @@ void intel_execlists_retire_requests(struct intel_engine_cs *engine)
spin_unlock_bh(&engine->execlist_lock);
list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) {
- struct intel_context *ctx = req->ctx;
- struct drm_i915_gem_object *ctx_obj =
- ctx->engine[engine->id].state;
-
- if (ctx_obj && (ctx != req->i915->kernel_context))
- intel_lr_context_unpin(ctx, engine);
+ intel_lr_context_unpin(req->ctx, engine);
list_del(&req->execlist_link);
i915_gem_request_unreference(req);
@@ -1052,23 +1038,26 @@ int logical_ring_flush_all_caches(struct drm_i915_gem_request *req)
return 0;
}
-static int intel_lr_context_do_pin(struct intel_context *ctx,
- struct intel_engine_cs *engine)
+static int intel_lr_context_pin(struct intel_context *ctx,
+ struct intel_engine_cs *engine)
{
- struct drm_device *dev = engine->dev;
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
- struct intel_ringbuffer *ringbuf = ctx->engine[engine->id].ringbuf;
+ struct drm_i915_private *dev_priv = ctx->i915;
+ struct drm_i915_gem_object *ctx_obj;
+ struct intel_ringbuffer *ringbuf;
void *vaddr;
u32 *lrc_reg_state;
int ret;
- WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
+ lockdep_assert_held(&ctx->i915->dev->struct_mutex);
+ if (ctx->engine[engine->id].pin_count++)
+ return 0;
+
+ ctx_obj = ctx->engine[engine->id].state;
ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN,
PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
if (ret)
- return ret;
+ goto err;
vaddr = i915_gem_object_pin_map(ctx_obj);
if (IS_ERR(vaddr)) {
@@ -1078,10 +1067,12 @@ static int intel_lr_context_do_pin(struct intel_context *ctx,
lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
+ ringbuf = ctx->engine[engine->id].ringbuf;
ret = intel_pin_and_map_ringbuffer_obj(engine->dev, ringbuf);
if (ret)
goto unpin_map;
+ i915_gem_context_reference(ctx);
ctx->engine[engine->id].lrc_vma = i915_gem_obj_to_ggtt(ctx_obj);
intel_lr_context_descriptor_update(ctx, engine);
lrc_reg_state[CTX_RING_BUFFER_START+1] = ringbuf->vma->node.start;
@@ -1092,51 +1083,39 @@ static int intel_lr_context_do_pin(struct intel_context *ctx,
if (i915.enable_guc_submission)
I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
- return ret;
+ return 0;
unpin_map:
i915_gem_object_unpin_map(ctx_obj);
unpin_ctx_obj:
i915_gem_object_ggtt_unpin(ctx_obj);
-
+err:
+ ctx->engine[engine->id].pin_count = 0;
return ret;
}
-static int intel_lr_context_pin(struct intel_context *ctx,
- struct intel_engine_cs *engine)
+void intel_lr_context_unpin(struct intel_context *ctx,
+ struct intel_engine_cs *engine)
{
- int ret = 0;
+ struct drm_i915_gem_object *ctx_obj;
- if (ctx->engine[engine->id].pin_count++ == 0) {
- ret = intel_lr_context_do_pin(ctx, engine);
- if (ret)
- goto reset_pin_count;
+ lockdep_assert_held(&ctx->i915->dev->struct_mutex);
+ GEM_BUG_ON(ctx->engine[engine->id].pin_count == 0);
- i915_gem_context_reference(ctx);
- }
- return ret;
+ if (--ctx->engine[engine->id].pin_count)
+ return;
-reset_pin_count:
- ctx->engine[engine->id].pin_count = 0;
- return ret;
-}
+ intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
-void intel_lr_context_unpin(struct intel_context *ctx,
- struct intel_engine_cs *engine)
-{
- struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
+ ctx_obj = ctx->engine[engine->id].state;
+ i915_gem_object_unpin_map(ctx_obj);
+ i915_gem_object_ggtt_unpin(ctx_obj);
- WARN_ON(!mutex_is_locked(&ctx->i915->dev->struct_mutex));
- if (--ctx->engine[engine->id].pin_count == 0) {
- i915_gem_object_unpin_map(ctx_obj);
- intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
- i915_gem_object_ggtt_unpin(ctx_obj);
- ctx->engine[engine->id].lrc_vma = NULL;
- ctx->engine[engine->id].lrc_desc = 0;
- ctx->engine[engine->id].lrc_reg_state = NULL;
+ ctx->engine[engine->id].lrc_vma = NULL;
+ ctx->engine[engine->id].lrc_desc = 0;
+ ctx->engine[engine->id].lrc_reg_state = NULL;
- i915_gem_context_unreference(ctx);
- }
+ i915_gem_context_unreference(ctx);
}
static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request *req)
@@ -2034,6 +2013,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
i915_gem_object_unpin_map(engine->status_page.obj);
engine->status_page.obj = NULL;
}
+ intel_lr_context_unpin(dev_priv->kernel_context, engine);
engine->idle_lite_restore_wa = 0;
engine->disable_lite_restore_wa = false;
@@ -2137,11 +2117,10 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
goto error;
/* As this is the default context, always pin it */
- ret = intel_lr_context_do_pin(dctx, engine);
+ ret = intel_lr_context_pin(dctx, engine);
if (ret) {
- DRM_ERROR(
- "Failed to pin and map ringbuffer %s: %d\n",
- engine->name, ret);
+ DRM_ERROR("Failed to pin context for %s: %d\n",
+ engine->name, ret);
goto error;
}
@@ -2562,12 +2541,6 @@ void intel_lr_context_free(struct intel_context *ctx)
if (!ctx_obj)
continue;
- if (ctx == ctx->i915->kernel_context) {
- intel_unpin_ringbuffer_obj(ringbuf);
- i915_gem_object_ggtt_unpin(ctx_obj);
- i915_gem_object_unpin_map(ctx_obj);
- }
-
WARN_ON(ctx->engine[i].pin_count);
intel_ringbuffer_free(ringbuf);
drm_gem_object_unreference(&ctx_obj->base);
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 09/12] drm/i915: Move context initialisation to first-use
2016-04-19 11:40 Premature unpinning Chris Wilson
` (7 preceding siblings ...)
2016-04-19 11:40 ` [PATCH v2 08/12] drm/i915: Refactor execlists default context pinning Chris Wilson
@ 2016-04-19 11:40 ` Chris Wilson
2016-04-19 11:40 ` [PATCH v2 10/12] drm/i915: Move the magical deferred context allocation into the request Chris Wilson
` (5 subsequent siblings)
14 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 11:40 UTC (permalink / raw)
To: intel-gfx
Instead of allocating a new request when allocating a context, use the
request that initiated the allocation to emit the context
initialisation. This serves two purposes, it makes the initialisation
atomic with first use (simplifying scheduling and our own error
handling). Secondly, it enables us to remove the explicit context
allocation required by higher levels of GEM and make that property of
execlists opaque (in the next patch). There is also a minor step
forwards towards convergence of legacy/execlist contexts.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_lrc.c | 39 ++++++++++++++++++---------------------
2 files changed, 19 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c71220f52bc7..c0a6fbff4678 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -868,6 +868,7 @@ struct intel_context {
struct i915_vma *lrc_vma;
u64 lrc_desc;
uint32_t *lrc_reg_state;
+ bool initialised;
} engine[I915_NUM_ENGINES];
struct list_head link;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index e064a6ae2d97..897f3d4c32c6 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -672,9 +672,10 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request)
{
- int ret = 0;
+ struct intel_engine_cs *engine = request->engine;
+ int ret;
- request->ringbuf = request->ctx->engine[request->engine->id].ringbuf;
+ request->ringbuf = request->ctx->engine[engine->id].ringbuf;
if (i915.enable_guc_submission) {
/*
@@ -689,7 +690,20 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
return ret;
}
- return intel_lr_context_pin(request->ctx, request->engine);
+ ret = intel_lr_context_pin(request->ctx, engine);
+ if (ret)
+ return ret;
+
+ if (!request->ctx->engine[engine->id].initialised) {
+ ret = engine->init_context(request);
+ if (ret) {
+ intel_lr_context_unpin(request->ctx, engine);
+ return ret;
+ }
+ request->ctx->engine[engine->id].initialised = true;
+ }
+
+ return 0;
}
static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
@@ -2636,25 +2650,8 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
ctx->engine[engine->id].ringbuf = ringbuf;
ctx->engine[engine->id].state = ctx_obj;
+ ctx->engine[engine->id].initialised = engine->init_context == NULL;
- if (ctx != ctx->i915->kernel_context && engine->init_context) {
- struct drm_i915_gem_request *req;
-
- req = i915_gem_request_alloc(engine, ctx);
- if (IS_ERR(req)) {
- ret = PTR_ERR(req);
- DRM_ERROR("ring create req: %d\n", ret);
- goto error_ringbuf;
- }
-
- ret = engine->init_context(req);
- i915_add_request_no_flush(req);
- if (ret) {
- DRM_ERROR("ring init context: %d\n",
- ret);
- goto error_ringbuf;
- }
- }
return 0;
error_ringbuf:
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 10/12] drm/i915: Move the magical deferred context allocation into the request
2016-04-19 11:40 Premature unpinning Chris Wilson
` (8 preceding siblings ...)
2016-04-19 11:40 ` [PATCH v2 09/12] drm/i915: Move context initialisation to first-use Chris Wilson
@ 2016-04-19 11:40 ` Chris Wilson
2016-04-19 12:40 ` kbuild test robot
2016-04-19 11:40 ` [PATCH v2 11/12] drm/i915: Track the previous pinned context inside " Chris Wilson
` (4 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 11:40 UTC (permalink / raw)
To: intel-gfx
We can hide more details of execlists from higher level code by removing
the explicit call to create an execlist context from execbuffer and
into its first use by execlists.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 --------
drivers/gpu/drm/i915/intel_lrc.c | 21 ++++++++++++++-------
drivers/gpu/drm/i915/intel_lrc.h | 2 --
3 files changed, 14 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 6f4f2a6cdf93..e0ee5d1ac372 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1085,14 +1085,6 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
return ERR_PTR(-EIO);
}
- if (i915.enable_execlists && !ctx->engine[engine->id].state) {
- int ret = intel_lr_context_deferred_alloc(ctx, engine);
- if (ret) {
- DRM_DEBUG("Could not create LRC %u: %d\n", ctx_id, ret);
- return ERR_PTR(ret);
- }
- }
-
return ctx;
}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 897f3d4c32c6..2910866611b2 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -227,6 +227,8 @@ enum {
#define GEN8_CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT 0x17
#define GEN9_CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT 0x26
+static int execlists_context_deferred_alloc(struct intel_context *ctx,
+ struct intel_engine_cs *engine);
static int intel_lr_context_pin(struct intel_context *ctx,
struct intel_engine_cs *engine);
@@ -675,8 +677,6 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
struct intel_engine_cs *engine = request->engine;
int ret;
- request->ringbuf = request->ctx->engine[engine->id].ringbuf;
-
if (i915.enable_guc_submission) {
/*
* Check that the GuC has space for the request before
@@ -690,6 +690,14 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
return ret;
}
+ if (request->ctx->engine[engine->id].state == NULL) {
+ ret = execlists_context_deferred_alloc(request->ctx, engine);
+ if (ret)
+ return ret;
+ }
+
+ request->ringbuf = request->ctx->engine[engine->id].ringbuf;
+
ret = intel_lr_context_pin(request->ctx, engine);
if (ret)
return ret;
@@ -2126,7 +2134,7 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
if (ret)
goto error;
- ret = intel_lr_context_deferred_alloc(dctx, engine);
+ ret = execlists_context_deferred_alloc(dctx, engine);
if (ret)
goto error;
@@ -2600,7 +2608,7 @@ uint32_t intel_lr_context_size(struct intel_engine_cs *engine)
}
/**
- * intel_lr_context_deferred_alloc() - create the LRC specific bits of a context
+ * execlists_context_deferred_alloc() - create the LRC specific bits of a context
* @ctx: LR context to create.
* @ring: engine to be used with the context.
*
@@ -2612,9 +2620,8 @@ uint32_t intel_lr_context_size(struct intel_engine_cs *engine)
*
* Return: non-zero on error.
*/
-
-int intel_lr_context_deferred_alloc(struct intel_context *ctx,
- struct intel_engine_cs *engine)
+static int execlists_context_deferred_alloc(struct intel_context *ctx,
+ struct intel_engine_cs *engine)
{
struct drm_device *dev = engine->dev;
struct drm_i915_gem_object *ctx_obj;
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index b17ab79333aa..8bea937973f6 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -102,8 +102,6 @@ static inline void intel_logical_ring_emit_reg(struct intel_ringbuffer *ringbuf,
void intel_lr_context_free(struct intel_context *ctx);
uint32_t intel_lr_context_size(struct intel_engine_cs *engine);
-int intel_lr_context_deferred_alloc(struct intel_context *ctx,
- struct intel_engine_cs *engine);
void intel_lr_context_unpin(struct intel_context *ctx,
struct intel_engine_cs *engine);
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 11/12] drm/i915: Track the previous pinned context inside the request
2016-04-19 11:40 Premature unpinning Chris Wilson
` (9 preceding siblings ...)
2016-04-19 11:40 ` [PATCH v2 10/12] drm/i915: Move the magical deferred context allocation into the request Chris Wilson
@ 2016-04-19 11:40 ` Chris Wilson
2016-04-19 11:40 ` [PATCH v2 12/12] drm/i915: Move releasing of the GEM request from free to retire/cancel Chris Wilson
` (3 subsequent siblings)
14 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 11:40 UTC (permalink / raw)
To: intel-gfx
As the contexts are accessed by the hardware until the switch is completed
to a new context, the hardware may still be writing to the context object
after the breadcrumb is visible. We must not unpin/unbind/prune that
object whilst still active and so we keep the previous context pinned until
the following request. If we move this tracking onto the request, we can
simplify the code and enable freeing of the request without the
struct_mutex in subsequent patches.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++++
drivers/gpu/drm/i915/intel_lrc.c | 17 ++++++++++-------
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c0a6fbff4678..ff1aa7363ab6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2302,6 +2302,17 @@ struct drm_i915_gem_request {
struct intel_context *ctx;
struct intel_ringbuffer *ringbuf;
+ /**
+ * Context related to the previous request.
+ * As the contexts are accessed by the hardware until the switch is
+ * completed to a new context, the hardware may still be writing
+ * to the context object after the breadcrumb is visible. We must
+ * not unpin/unbind/prune that object whilst still active and so
+ * we keep the previous context pinned until the following (this)
+ * request is retired.
+ */
+ struct intel_context *previous_context;
+
/** Batch buffer related to this request if any (used for
error state dump only) */
struct drm_i915_gem_object *batch_obj;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 2910866611b2..68382e05f720 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -788,12 +788,14 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
if (intel_engine_stopped(engine))
return 0;
- if (engine->last_context != request->ctx) {
- if (engine->last_context)
- intel_lr_context_unpin(engine->last_context, engine);
- intel_lr_context_pin(request->ctx, engine);
- engine->last_context = request->ctx;
- }
+ /* We keep the previous context alive until we retire the following
+ * request. This ensures that any the context object is still pinned
+ * for any residual writes the HW makes into it on the context switch
+ * into the next object following the breadcrumb. Otherwise, we may
+ * retire the context too early.
+ */
+ request->previous_context = engine->last_context;
+ engine->last_context = request->ctx;
if (dev_priv->guc.execbuf_client)
i915_guc_submit(dev_priv->guc.execbuf_client, request);
@@ -1015,7 +1017,8 @@ void intel_execlists_retire_requests(struct intel_engine_cs *engine)
spin_unlock_bh(&engine->execlist_lock);
list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) {
- intel_lr_context_unpin(req->ctx, engine);
+ if (req->previous_context)
+ intel_lr_context_unpin(req->previous_context, engine);
list_del(&req->execlist_link);
i915_gem_request_unreference(req);
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 12/12] drm/i915: Move releasing of the GEM request from free to retire/cancel
2016-04-19 11:40 Premature unpinning Chris Wilson
` (10 preceding siblings ...)
2016-04-19 11:40 ` [PATCH v2 11/12] drm/i915: Track the previous pinned context inside " Chris Wilson
@ 2016-04-19 11:40 ` Chris Wilson
2016-04-19 12:59 ` Update to patches 11 and 12 Chris Wilson
` (2 subsequent siblings)
14 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 11:40 UTC (permalink / raw)
To: intel-gfx
If we move the release of the GEM request (i.e. decoupling it from the
various lists used for client and context tracking) after it is complete
(either by the GPU retiring the request, or by the caller cancelling the
request), we can remove the requirement that the final unreference of
the GEM request need to be under the struct_mutex.
v2,v3: Rebalance execlists by moving the context unpinning.
v4: Rebase onto -nightly
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 14 --------------
drivers/gpu/drm/i915/i915_gem.c | 24 ++++++++++--------------
drivers/gpu/drm/i915/intel_display.c | 2 +-
drivers/gpu/drm/i915/intel_lrc.c | 4 ----
drivers/gpu/drm/i915/intel_pm.c | 2 +-
5 files changed, 12 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ff1aa7363ab6..be98e9643072 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2381,23 +2381,9 @@ i915_gem_request_reference(struct drm_i915_gem_request *req)
static inline void
i915_gem_request_unreference(struct drm_i915_gem_request *req)
{
- WARN_ON(!mutex_is_locked(&req->engine->dev->struct_mutex));
kref_put(&req->ref, i915_gem_request_free);
}
-static inline void
-i915_gem_request_unreference__unlocked(struct drm_i915_gem_request *req)
-{
- struct drm_device *dev;
-
- if (!req)
- return;
-
- dev = req->engine->dev;
- if (kref_put_mutex(&req->ref, i915_gem_request_free, &dev->struct_mutex))
- mutex_unlock(&dev->struct_mutex);
-}
-
static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst,
struct drm_i915_gem_request *src)
{
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 261185281e78..416b6bf0f131 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1413,6 +1413,14 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
list_del_init(&request->list);
i915_gem_request_remove_from_client(request);
+ if (request->previous_context) {
+ if (i915.enable_execlists)
+ intel_lr_context_unpin(request->previous_context,
+ request->engine);
+
+ }
+
+ i915_gem_context_unreference(request->ctx);
i915_gem_request_unreference(request);
}
@@ -2713,18 +2721,6 @@ void i915_gem_request_free(struct kref *req_ref)
{
struct drm_i915_gem_request *req = container_of(req_ref,
typeof(*req), ref);
- struct intel_context *ctx = req->ctx;
-
- if (req->file_priv)
- i915_gem_request_remove_from_client(req);
-
- if (ctx) {
- if (i915.enable_execlists)
- intel_lr_context_unpin(ctx, req->engine);
-
- i915_gem_context_unreference(ctx);
- }
-
kmem_cache_free(req->i915->requests, req);
}
@@ -3186,7 +3182,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
ret = __i915_wait_request(req[i], true,
args->timeout_ns > 0 ? &args->timeout_ns : NULL,
to_rps_client(file));
- i915_gem_request_unreference__unlocked(req[i]);
+ i915_gem_request_unreference(req[i]);
}
return ret;
@@ -4199,7 +4195,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
if (ret == 0)
queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, 0);
- i915_gem_request_unreference__unlocked(target);
+ i915_gem_request_unreference(target);
return ret;
}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 68151271283c..c44360c813b2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11399,7 +11399,7 @@ static void intel_mmio_flip_work_func(struct work_struct *work)
WARN_ON(__i915_wait_request(mmio_flip->req,
false, NULL,
&mmio_flip->i915->rps.mmioflips));
- i915_gem_request_unreference__unlocked(mmio_flip->req);
+ i915_gem_request_unreference(mmio_flip->req);
}
/* For framebuffer backed by dmabuf, wait for fence */
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 68382e05f720..970ad026fd59 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -590,7 +590,6 @@ static void execlists_context_queue(struct drm_i915_gem_request *request)
struct drm_i915_gem_request *cursor;
int num_elements = 0;
- intel_lr_context_pin(request->ctx, request->engine);
i915_gem_request_reference(request);
spin_lock_bh(&engine->execlist_lock);
@@ -1017,9 +1016,6 @@ void intel_execlists_retire_requests(struct intel_engine_cs *engine)
spin_unlock_bh(&engine->execlist_lock);
list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) {
- if (req->previous_context)
- intel_lr_context_unpin(req->previous_context, engine);
-
list_del(&req->execlist_link);
i915_gem_request_unreference(req);
}
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b7c218602c6e..ed3797bf41aa 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7366,7 +7366,7 @@ static void __intel_rps_boost_work(struct work_struct *work)
gen6_rps_boost(to_i915(req->engine->dev), NULL,
req->emitted_jiffies);
- i915_gem_request_unreference__unlocked(req);
+ i915_gem_request_unreference(req);
kfree(boost);
}
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 08/12] drm/i915: Refactor execlists default context pinning
2016-04-19 11:40 ` [PATCH v2 08/12] drm/i915: Refactor execlists default context pinning Chris Wilson
@ 2016-04-19 12:25 ` Tvrtko Ursulin
2016-04-20 14:07 ` Mika Kuoppala
1 sibling, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2016-04-19 12:25 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 19/04/16 12:40, Chris Wilson wrote:
> Refactor pinning and unpinning of contexts, such that the default
> context for an engine is pinned during initialisation and unpinned
> during teardown (pinning of the context handles the reference counting).
> Thus we can eliminate the special case handling of the default context
> that was required to mask that it was not being pinned normally.
>
> v2: Rebalance context_queue after rebasing.
> v3: Rebase to -nightly (not 40 patches in)
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
> drivers/gpu/drm/i915/i915_gem.c | 2 +-
> drivers/gpu/drm/i915/intel_lrc.c | 107 ++++++++++++++----------------------
> 3 files changed, 43 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index f775451bd0b6..e81a7504656e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2095,9 +2095,8 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
> return ret;
>
> list_for_each_entry(ctx, &dev_priv->context_list, link)
> - if (ctx != dev_priv->kernel_context)
> - for_each_engine(engine, dev_priv)
> - i915_dump_lrc_obj(m, ctx, engine);
> + for_each_engine(engine, dev_priv)
> + i915_dump_lrc_obj(m, ctx, engine);
>
> mutex_unlock(&dev->struct_mutex);
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b95d5f83d3b0..261185281e78 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2719,7 +2719,7 @@ void i915_gem_request_free(struct kref *req_ref)
> i915_gem_request_remove_from_client(req);
>
> if (ctx) {
> - if (i915.enable_execlists && ctx != req->i915->kernel_context)
> + if (i915.enable_execlists)
> intel_lr_context_unpin(ctx, req->engine);
>
> i915_gem_context_unreference(ctx);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index dedd82aea386..e064a6ae2d97 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -588,9 +588,7 @@ static void execlists_context_queue(struct drm_i915_gem_request *request)
> struct drm_i915_gem_request *cursor;
> int num_elements = 0;
>
> - if (request->ctx != request->i915->kernel_context)
> - intel_lr_context_pin(request->ctx, engine);
> -
> + intel_lr_context_pin(request->ctx, request->engine);
> i915_gem_request_reference(request);
>
> spin_lock_bh(&engine->execlist_lock);
> @@ -691,10 +689,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
> return ret;
> }
>
> - if (request->ctx != request->i915->kernel_context)
> - ret = intel_lr_context_pin(request->ctx, request->engine);
> -
> - return ret;
> + return intel_lr_context_pin(request->ctx, request->engine);
> }
>
> static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
> @@ -774,12 +769,8 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
> if (engine->last_context != request->ctx) {
> if (engine->last_context)
> intel_lr_context_unpin(engine->last_context, engine);
> - if (request->ctx != request->i915->kernel_context) {
> - intel_lr_context_pin(request->ctx, engine);
> - engine->last_context = request->ctx;
> - } else {
> - engine->last_context = NULL;
> - }
> + intel_lr_context_pin(request->ctx, engine);
> + engine->last_context = request->ctx;
> }
>
> if (dev_priv->guc.execbuf_client)
> @@ -1002,12 +993,7 @@ void intel_execlists_retire_requests(struct intel_engine_cs *engine)
> spin_unlock_bh(&engine->execlist_lock);
>
> list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) {
> - struct intel_context *ctx = req->ctx;
> - struct drm_i915_gem_object *ctx_obj =
> - ctx->engine[engine->id].state;
> -
> - if (ctx_obj && (ctx != req->i915->kernel_context))
> - intel_lr_context_unpin(ctx, engine);
> + intel_lr_context_unpin(req->ctx, engine);
>
> list_del(&req->execlist_link);
> i915_gem_request_unreference(req);
> @@ -1052,23 +1038,26 @@ int logical_ring_flush_all_caches(struct drm_i915_gem_request *req)
> return 0;
> }
>
> -static int intel_lr_context_do_pin(struct intel_context *ctx,
> - struct intel_engine_cs *engine)
> +static int intel_lr_context_pin(struct intel_context *ctx,
> + struct intel_engine_cs *engine)
> {
> - struct drm_device *dev = engine->dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
> - struct intel_ringbuffer *ringbuf = ctx->engine[engine->id].ringbuf;
> + struct drm_i915_private *dev_priv = ctx->i915;
> + struct drm_i915_gem_object *ctx_obj;
> + struct intel_ringbuffer *ringbuf;
> void *vaddr;
> u32 *lrc_reg_state;
> int ret;
>
> - WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
> + lockdep_assert_held(&ctx->i915->dev->struct_mutex);
>
> + if (ctx->engine[engine->id].pin_count++)
> + return 0;
> +
> + ctx_obj = ctx->engine[engine->id].state;
> ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN,
> PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> if (ret)
> - return ret;
> + goto err;
>
> vaddr = i915_gem_object_pin_map(ctx_obj);
> if (IS_ERR(vaddr)) {
> @@ -1078,10 +1067,12 @@ static int intel_lr_context_do_pin(struct intel_context *ctx,
>
> lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
>
> + ringbuf = ctx->engine[engine->id].ringbuf;
> ret = intel_pin_and_map_ringbuffer_obj(engine->dev, ringbuf);
> if (ret)
> goto unpin_map;
>
> + i915_gem_context_reference(ctx);
> ctx->engine[engine->id].lrc_vma = i915_gem_obj_to_ggtt(ctx_obj);
> intel_lr_context_descriptor_update(ctx, engine);
> lrc_reg_state[CTX_RING_BUFFER_START+1] = ringbuf->vma->node.start;
> @@ -1092,51 +1083,39 @@ static int intel_lr_context_do_pin(struct intel_context *ctx,
> if (i915.enable_guc_submission)
> I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
>
> - return ret;
> + return 0;
>
> unpin_map:
> i915_gem_object_unpin_map(ctx_obj);
> unpin_ctx_obj:
> i915_gem_object_ggtt_unpin(ctx_obj);
> -
> +err:
> + ctx->engine[engine->id].pin_count = 0;
> return ret;
> }
>
> -static int intel_lr_context_pin(struct intel_context *ctx,
> - struct intel_engine_cs *engine)
> +void intel_lr_context_unpin(struct intel_context *ctx,
> + struct intel_engine_cs *engine)
> {
> - int ret = 0;
> + struct drm_i915_gem_object *ctx_obj;
>
> - if (ctx->engine[engine->id].pin_count++ == 0) {
> - ret = intel_lr_context_do_pin(ctx, engine);
> - if (ret)
> - goto reset_pin_count;
> + lockdep_assert_held(&ctx->i915->dev->struct_mutex);
> + GEM_BUG_ON(ctx->engine[engine->id].pin_count == 0);
>
> - i915_gem_context_reference(ctx);
> - }
> - return ret;
> + if (--ctx->engine[engine->id].pin_count)
> + return;
>
> -reset_pin_count:
> - ctx->engine[engine->id].pin_count = 0;
> - return ret;
> -}
> + intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
>
> -void intel_lr_context_unpin(struct intel_context *ctx,
> - struct intel_engine_cs *engine)
> -{
> - struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
> + ctx_obj = ctx->engine[engine->id].state;
> + i915_gem_object_unpin_map(ctx_obj);
> + i915_gem_object_ggtt_unpin(ctx_obj);
>
> - WARN_ON(!mutex_is_locked(&ctx->i915->dev->struct_mutex));
> - if (--ctx->engine[engine->id].pin_count == 0) {
> - i915_gem_object_unpin_map(ctx_obj);
> - intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
> - i915_gem_object_ggtt_unpin(ctx_obj);
> - ctx->engine[engine->id].lrc_vma = NULL;
> - ctx->engine[engine->id].lrc_desc = 0;
> - ctx->engine[engine->id].lrc_reg_state = NULL;
> + ctx->engine[engine->id].lrc_vma = NULL;
> + ctx->engine[engine->id].lrc_desc = 0;
> + ctx->engine[engine->id].lrc_reg_state = NULL;
>
> - i915_gem_context_unreference(ctx);
> - }
> + i915_gem_context_unreference(ctx);
> }
>
> static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request *req)
> @@ -2034,6 +2013,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
> i915_gem_object_unpin_map(engine->status_page.obj);
> engine->status_page.obj = NULL;
> }
> + intel_lr_context_unpin(dev_priv->kernel_context, engine);
>
> engine->idle_lite_restore_wa = 0;
> engine->disable_lite_restore_wa = false;
> @@ -2137,11 +2117,10 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
> goto error;
>
> /* As this is the default context, always pin it */
> - ret = intel_lr_context_do_pin(dctx, engine);
> + ret = intel_lr_context_pin(dctx, engine);
> if (ret) {
> - DRM_ERROR(
> - "Failed to pin and map ringbuffer %s: %d\n",
> - engine->name, ret);
> + DRM_ERROR("Failed to pin context for %s: %d\n",
> + engine->name, ret);
> goto error;
> }
>
> @@ -2562,12 +2541,6 @@ void intel_lr_context_free(struct intel_context *ctx)
> if (!ctx_obj)
> continue;
>
> - if (ctx == ctx->i915->kernel_context) {
> - intel_unpin_ringbuffer_obj(ringbuf);
> - i915_gem_object_ggtt_unpin(ctx_obj);
> - i915_gem_object_unpin_map(ctx_obj);
> - }
> -
> WARN_ON(ctx->engine[i].pin_count);
> intel_ringbuffer_free(ringbuf);
> drm_gem_object_unreference(&ctx_obj->base);
>
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] 31+ messages in thread
* Re: [PATCH v2 10/12] drm/i915: Move the magical deferred context allocation into the request
2016-04-19 11:40 ` [PATCH v2 10/12] drm/i915: Move the magical deferred context allocation into the request Chris Wilson
@ 2016-04-19 12:40 ` kbuild test robot
0 siblings, 0 replies; 31+ messages in thread
From: kbuild test robot @ 2016-04-19 12:40 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 16455 bytes --]
Hi,
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20160419]
[cannot apply to v4.6-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Mark-the-current-context-as-lost-on-suspend/20160419-194400
base: git://anongit.freedesktop.org/drm-intel for-linux-next
reproduce: make htmldocs
All warnings (new ones prefixed by >>):
include/drm/drmP.h:280: warning: No description found for parameter 'ioctl'
include/drm/drmP.h:280: warning: No description found for parameter '_func'
include/drm/drmP.h:280: warning: No description found for parameter '_flags'
include/drm/drmP.h:362: warning: cannot understand function prototype: 'struct drm_lock_data '
include/drm/drmP.h:415: warning: cannot understand function prototype: 'struct drm_driver '
include/drm/drmP.h:672: warning: cannot understand function prototype: 'struct drm_info_list '
include/drm/drmP.h:682: warning: cannot understand function prototype: 'struct drm_info_node '
include/drm/drmP.h:692: warning: cannot understand function prototype: 'struct drm_minor '
include/drm/drmP.h:740: warning: cannot understand function prototype: 'struct drm_device '
drivers/gpu/drm/i915/intel_runtime_pm.c:2356: warning: No description found for parameter 'resume'
drivers/gpu/drm/i915/intel_runtime_pm.c:2356: warning: No description found for parameter 'resume'
drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt'
drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt'
drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt'
drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt'
drivers/gpu/drm/i915/i915_gem.c:416: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:416: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/i915_gem.c:416: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:681: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:681: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/i915_gem.c:681: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:762: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:762: warning: No description found for parameter 'obj'
drivers/gpu/drm/i915/i915_gem.c:762: warning: No description found for parameter 'args'
drivers/gpu/drm/i915/i915_gem.c:762: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:1025: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:1025: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/i915_gem.c:1025: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:1231: warning: No description found for parameter 'rps'
drivers/gpu/drm/i915/i915_gem.c:1446: warning: No description found for parameter 'req'
drivers/gpu/drm/i915/i915_gem.c:1475: warning: No description found for parameter 'obj'
drivers/gpu/drm/i915/i915_gem.c:1475: warning: No description found for parameter 'readonly'
drivers/gpu/drm/i915/i915_gem.c:1591: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:1591: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/i915_gem.c:1591: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:1654: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:1654: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/i915_gem.c:1654: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:1699: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:1699: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/i915_gem.c:1699: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:2004: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:2004: warning: No description found for parameter 'size'
drivers/gpu/drm/i915/i915_gem.c:2004: warning: No description found for parameter 'tiling_mode'
drivers/gpu/drm/i915/i915_gem.c:2004: warning: No description found for parameter 'fenced'
drivers/gpu/drm/i915/i915_gem.c:2004: warning: Excess function parameter 'obj' description in 'i915_gem_get_gtt_alignment'
drivers/gpu/drm/i915/i915_gem.c:2961: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/i915_gem.c:3087: warning: No description found for parameter 'obj'
drivers/gpu/drm/i915/i915_gem.c:3137: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:3137: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/i915_gem.c:3137: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:3137: warning: Excess function parameter 'DRM_IOCTL_ARGS' description in 'i915_gem_wait_ioctl'
drivers/gpu/drm/i915/i915_gem.c:3499: warning: No description found for parameter 'obj'
drivers/gpu/drm/i915/i915_gem.c:3499: warning: No description found for parameter 'vm'
drivers/gpu/drm/i915/i915_gem.c:3499: warning: No description found for parameter 'ggtt_view'
drivers/gpu/drm/i915/i915_gem.c:3499: warning: No description found for parameter 'alignment'
drivers/gpu/drm/i915/i915_gem.c:3499: warning: No description found for parameter 'flags'
drivers/gpu/drm/i915/i915_gem.c:3755: warning: No description found for parameter 'obj'
drivers/gpu/drm/i915/i915_gem.c:3755: warning: No description found for parameter 'write'
drivers/gpu/drm/i915/i915_gem.c:3833: warning: No description found for parameter 'obj'
drivers/gpu/drm/i915/i915_gem.c:3833: warning: No description found for parameter 'cache_level'
drivers/gpu/drm/i915/i915_gem.c:4107: warning: No description found for parameter 'obj'
drivers/gpu/drm/i915/i915_gem.c:4107: warning: No description found for parameter 'write'
drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_init_ring'
drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_fini_ring'
drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: Excess function parameter 'ring' description in 'i915_needs_cmd_parser'
drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: Excess function parameter 'ring' description in 'i915_parse_cmds'
drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_init_ring'
drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_fini_ring'
drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: Excess function parameter 'ring' description in 'i915_needs_cmd_parser'
drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: Excess function parameter 'ring' description in 'i915_parse_cmds'
drivers/gpu/drm/i915/intel_lrc.c:318: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/intel_lrc.c:318: warning: Excess function parameter 'ring' description in 'intel_lr_context_descriptor_update'
drivers/gpu/drm/i915/intel_lrc.c:522: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/intel_lrc.c:522: warning: Excess function parameter 'engine' description in 'intel_lrc_irq_handler'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: No description found for parameter 'params'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: Excess function parameter 'dev' description in 'intel_execlists_submission'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: Excess function parameter 'file' description in 'intel_execlists_submission'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: Excess function parameter 'ring' description in 'intel_execlists_submission'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission'
drivers/gpu/drm/i915/intel_lrc.c:1302: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/intel_lrc.c:1302: warning: Excess function parameter 'ring' description in 'gen8_init_indirectctx_bb'
drivers/gpu/drm/i915/intel_lrc.c:1365: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/intel_lrc.c:1365: warning: Excess function parameter 'ring' description in 'gen8_init_perctx_bb'
drivers/gpu/drm/i915/intel_lrc.c:2008: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/intel_lrc.c:2008: warning: Excess function parameter 'ring' description in 'intel_logical_ring_cleanup'
drivers/gpu/drm/i915/intel_lrc.c:2587: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/intel_lrc.c:2587: warning: Excess function parameter 'ring' description in 'intel_lr_context_size'
drivers/gpu/drm/i915/intel_lrc.c:2625: warning: No description found for parameter 'engine'
>> drivers/gpu/drm/i915/intel_lrc.c:2625: warning: Excess function parameter 'ring' description in 'execlists_context_deferred_alloc'
drivers/gpu/drm/i915/intel_lrc.c:318: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/intel_lrc.c:318: warning: Excess function parameter 'ring' description in 'intel_lr_context_descriptor_update'
drivers/gpu/drm/i915/intel_lrc.c:522: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/intel_lrc.c:522: warning: Excess function parameter 'engine' description in 'intel_lrc_irq_handler'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: No description found for parameter 'params'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: Excess function parameter 'dev' description in 'intel_execlists_submission'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: Excess function parameter 'file' description in 'intel_execlists_submission'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: Excess function parameter 'ring' description in 'intel_execlists_submission'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission'
drivers/gpu/drm/i915/intel_lrc.c:929: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission'
drivers/gpu/drm/i915/intel_lrc.c:1302: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/intel_lrc.c:1302: warning: Excess function parameter 'ring' description in 'gen8_init_indirectctx_bb'
drivers/gpu/drm/i915/intel_lrc.c:1365: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/intel_lrc.c:1365: warning: Excess function parameter 'ring' description in 'gen8_init_perctx_bb'
drivers/gpu/drm/i915/intel_lrc.c:2008: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/intel_lrc.c:2008: warning: Excess function parameter 'ring' description in 'intel_logical_ring_cleanup'
drivers/gpu/drm/i915/intel_lrc.c:2587: warning: No description found for parameter 'engine'
drivers/gpu/drm/i915/intel_lrc.c:2587: warning: Excess function parameter 'ring' description in 'intel_lr_context_size'
drivers/gpu/drm/i915/intel_lrc.c:2625: warning: No description found for parameter 'engine'
>> drivers/gpu/drm/i915/intel_lrc.c:2625: warning: Excess function parameter 'ring' description in 'execlists_context_deferred_alloc'
Warning: didn't use docs for i915_hotplug_interrupt_update
Warning: didn't use docs for ilk_update_display_irq
Warning: didn't use docs for ilk_update_gt_irq
Warning: didn't use docs for snb_update_pm_irq
Warning: didn't use docs for bdw_update_port_irq
Warning: didn't use docs for bdw_update_pipe_irq
Warning: didn't use docs for ibx_display_interrupt_update
Warning: didn't use docs for i915_enable_asle_pipestat
Warning: didn't use docs for ivybridge_parity_work
Warning: didn't use docs for i915_reset_and_wakeup
Warning: didn't use docs for i915_handle_error
Warning: didn't use docs for intel_irq_install
Warning: didn't use docs for intel_irq_uninstall
vim +2625 drivers/gpu/drm/i915/intel_lrc.c
ede7d42b Oscar Mateo 2014-07-24 2609
73e4d07f Oscar Mateo 2014-07-24 2610 /**
92c1989f Chris Wilson 2016-04-19 2611 * execlists_context_deferred_alloc() - create the LRC specific bits of a context
73e4d07f Oscar Mateo 2014-07-24 2612 * @ctx: LR context to create.
73e4d07f Oscar Mateo 2014-07-24 2613 * @ring: engine to be used with the context.
73e4d07f Oscar Mateo 2014-07-24 2614 *
73e4d07f Oscar Mateo 2014-07-24 2615 * This function can be called more than once, with different engines, if we plan
73e4d07f Oscar Mateo 2014-07-24 2616 * to use the context with them. The context backing objects and the ringbuffers
73e4d07f Oscar Mateo 2014-07-24 2617 * (specially the ringbuffer backing objects) suck a lot of memory up, and that's why
73e4d07f Oscar Mateo 2014-07-24 2618 * the creation is a deferred call: it's better to make sure first that we need to use
73e4d07f Oscar Mateo 2014-07-24 2619 * a given ring with the context.
73e4d07f Oscar Mateo 2014-07-24 2620 *
32197aab Masanari Iida 2014-10-20 2621 * Return: non-zero on error.
73e4d07f Oscar Mateo 2014-07-24 2622 */
92c1989f Chris Wilson 2016-04-19 2623 static int execlists_context_deferred_alloc(struct intel_context *ctx,
0bc40be8 Tvrtko Ursulin 2016-03-16 2624 struct intel_engine_cs *engine)
ede7d42b Oscar Mateo 2014-07-24 @2625 {
0bc40be8 Tvrtko Ursulin 2016-03-16 2626 struct drm_device *dev = engine->dev;
8c857917 Oscar Mateo 2014-07-24 2627 struct drm_i915_gem_object *ctx_obj;
8c857917 Oscar Mateo 2014-07-24 2628 uint32_t context_size;
84c2377f Oscar Mateo 2014-07-24 2629 struct intel_ringbuffer *ringbuf;
8c857917 Oscar Mateo 2014-07-24 2630 int ret;
8c857917 Oscar Mateo 2014-07-24 2631
ede7d42b Oscar Mateo 2014-07-24 2632 WARN_ON(ctx->legacy_hw_ctx.rcs_state != NULL);
0bc40be8 Tvrtko Ursulin 2016-03-16 2633 WARN_ON(ctx->engine[engine->id].state);
:::::: The code at line 2625 was first introduced by commit
:::::: ede7d42baeece583c864badb6f9081f4cded6c32 drm/i915/bdw: Initialization for Logical Ring Contexts
:::::: TO: Oscar Mateo <oscar.mateo@intel.com>
:::::: CC: Daniel Vetter <daniel.vetter@ffwll.ch>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6302 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 31+ messages in thread
* Update to patches 11 and 12
2016-04-19 11:40 Premature unpinning Chris Wilson
` (11 preceding siblings ...)
2016-04-19 11:40 ` [PATCH v2 12/12] drm/i915: Move releasing of the GEM request from free to retire/cancel Chris Wilson
@ 2016-04-19 12:59 ` Chris Wilson
2016-04-19 12:59 ` [PATCH 1/2] drm/i915: Move releasing of the GEM request from free to retire/cancel Chris Wilson
2016-04-19 12:59 ` [PATCH 2/2] drm/i915: Track the previous pinned context inside the request Chris Wilson
2016-04-19 13:24 ` ✗ Fi.CI.BAT: failure for series starting with [v2,01/12] drm/i915: Mark the current context as lost on suspend Patchwork
2016-04-19 14:58 ` ✗ Fi.CI.BAT: failure for series starting with [v2] drm/i915: Mark the current context as lost on suspend (rev2) Patchwork
14 siblings, 2 replies; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 12:59 UTC (permalink / raw)
To: intel-gfx
Flipped the last pair to make sure GuC isn't broken in between.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 1/2] drm/i915: Move releasing of the GEM request from free to retire/cancel
2016-04-19 12:59 ` Update to patches 11 and 12 Chris Wilson
@ 2016-04-19 12:59 ` Chris Wilson
2016-04-20 13:55 ` Tvrtko Ursulin
2016-04-19 12:59 ` [PATCH 2/2] drm/i915: Track the previous pinned context inside the request Chris Wilson
1 sibling, 1 reply; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 12:59 UTC (permalink / raw)
To: intel-gfx
If we move the release of the GEM request (i.e. decoupling it from the
various lists used for client and context tracking) after it is complete
(either by the GPU retiring the request, or by the caller cancelling the
request), we can remove the requirement that the final unreference of
the GEM request need to be under the struct_mutex.
v2,v3: Rebalance execlists by moving the context unpinning.
v4: Rebase onto -nightly
v5: Avoid trying to rebalance execlist/GuC context pinning, leave that
to the next step
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 14 --------------
drivers/gpu/drm/i915/i915_gem.c | 23 +++++++++--------------
drivers/gpu/drm/i915/intel_display.c | 2 +-
drivers/gpu/drm/i915/intel_pm.c | 2 +-
4 files changed, 11 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c0a6fbff4678..c59b2670cc36 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2370,23 +2370,9 @@ i915_gem_request_reference(struct drm_i915_gem_request *req)
static inline void
i915_gem_request_unreference(struct drm_i915_gem_request *req)
{
- WARN_ON(!mutex_is_locked(&req->engine->dev->struct_mutex));
kref_put(&req->ref, i915_gem_request_free);
}
-static inline void
-i915_gem_request_unreference__unlocked(struct drm_i915_gem_request *req)
-{
- struct drm_device *dev;
-
- if (!req)
- return;
-
- dev = req->engine->dev;
- if (kref_put_mutex(&req->ref, i915_gem_request_free, &dev->struct_mutex))
- mutex_unlock(&dev->struct_mutex);
-}
-
static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst,
struct drm_i915_gem_request *src)
{
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 261185281e78..9b4854a17264 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1413,6 +1413,13 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
list_del_init(&request->list);
i915_gem_request_remove_from_client(request);
+ if (request->ctx) {
+ if (i915.enable_execlists)
+ intel_lr_context_unpin(request->ctx, request->engine);
+
+ i915_gem_context_unreference(request->ctx);
+ }
+
i915_gem_request_unreference(request);
}
@@ -2713,18 +2720,6 @@ void i915_gem_request_free(struct kref *req_ref)
{
struct drm_i915_gem_request *req = container_of(req_ref,
typeof(*req), ref);
- struct intel_context *ctx = req->ctx;
-
- if (req->file_priv)
- i915_gem_request_remove_from_client(req);
-
- if (ctx) {
- if (i915.enable_execlists)
- intel_lr_context_unpin(ctx, req->engine);
-
- i915_gem_context_unreference(ctx);
- }
-
kmem_cache_free(req->i915->requests, req);
}
@@ -3186,7 +3181,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
ret = __i915_wait_request(req[i], true,
args->timeout_ns > 0 ? &args->timeout_ns : NULL,
to_rps_client(file));
- i915_gem_request_unreference__unlocked(req[i]);
+ i915_gem_request_unreference(req[i]);
}
return ret;
@@ -4199,7 +4194,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
if (ret == 0)
queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, 0);
- i915_gem_request_unreference__unlocked(target);
+ i915_gem_request_unreference(target);
return ret;
}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ff60241b1f76..f5bf46f99cc2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11399,7 +11399,7 @@ static void intel_mmio_flip_work_func(struct work_struct *work)
WARN_ON(__i915_wait_request(mmio_flip->req,
false, NULL,
&mmio_flip->i915->rps.mmioflips));
- i915_gem_request_unreference__unlocked(mmio_flip->req);
+ i915_gem_request_unreference(mmio_flip->req);
}
/* For framebuffer backed by dmabuf, wait for fence */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b7c218602c6e..ed3797bf41aa 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7366,7 +7366,7 @@ static void __intel_rps_boost_work(struct work_struct *work)
gen6_rps_boost(to_i915(req->engine->dev), NULL,
req->emitted_jiffies);
- i915_gem_request_unreference__unlocked(req);
+ i915_gem_request_unreference(req);
kfree(boost);
}
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 2/2] drm/i915: Track the previous pinned context inside the request
2016-04-19 12:59 ` Update to patches 11 and 12 Chris Wilson
2016-04-19 12:59 ` [PATCH 1/2] drm/i915: Move releasing of the GEM request from free to retire/cancel Chris Wilson
@ 2016-04-19 12:59 ` Chris Wilson
2016-04-20 14:08 ` Tvrtko Ursulin
1 sibling, 1 reply; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 12:59 UTC (permalink / raw)
To: intel-gfx
As the contexts are accessed by the hardware until the switch is completed
to a new context, the hardware may still be writing to the context object
after the breadcrumb is visible. We must not unpin/unbind/prune that
object whilst still active and so we keep the previous context pinned until
the following request. If we move this tracking onto the request, we can
simplify the code and treat execlists/GuC dispatch identically.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++++
drivers/gpu/drm/i915/i915_gem.c | 8 ++++----
drivers/gpu/drm/i915/intel_lrc.c | 17 ++++++++---------
3 files changed, 23 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c59b2670cc36..be98e9643072 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2302,6 +2302,17 @@ struct drm_i915_gem_request {
struct intel_context *ctx;
struct intel_ringbuffer *ringbuf;
+ /**
+ * Context related to the previous request.
+ * As the contexts are accessed by the hardware until the switch is
+ * completed to a new context, the hardware may still be writing
+ * to the context object after the breadcrumb is visible. We must
+ * not unpin/unbind/prune that object whilst still active and so
+ * we keep the previous context pinned until the following (this)
+ * request is retired.
+ */
+ struct intel_context *previous_context;
+
/** Batch buffer related to this request if any (used for
error state dump only) */
struct drm_i915_gem_object *batch_obj;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9b4854a17264..537aacfda3eb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1413,13 +1413,13 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
list_del_init(&request->list);
i915_gem_request_remove_from_client(request);
- if (request->ctx) {
+ if (request->previous_context) {
if (i915.enable_execlists)
- intel_lr_context_unpin(request->ctx, request->engine);
-
- i915_gem_context_unreference(request->ctx);
+ intel_lr_context_unpin(request->previous_context,
+ request->engine);
}
+ i915_gem_context_unreference(request->ctx);
i915_gem_request_unreference(request);
}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index ee4e9bb80042..06e013293ec6 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -590,7 +590,6 @@ static void execlists_context_queue(struct drm_i915_gem_request *request)
struct drm_i915_gem_request *cursor;
int num_elements = 0;
- intel_lr_context_pin(request->ctx, request->engine);
i915_gem_request_reference(request);
spin_lock_bh(&engine->execlist_lock);
@@ -788,12 +787,14 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
if (intel_engine_stopped(engine))
return 0;
- if (engine->last_context != request->ctx) {
- if (engine->last_context)
- intel_lr_context_unpin(engine->last_context, engine);
- intel_lr_context_pin(request->ctx, engine);
- engine->last_context = request->ctx;
- }
+ /* We keep the previous context alive until we retire the following
+ * request. This ensures that any the context object is still pinned
+ * for any residual writes the HW makes into it on the context switch
+ * into the next object following the breadcrumb. Otherwise, we may
+ * retire the context too early.
+ */
+ request->previous_context = engine->last_context;
+ engine->last_context = request->ctx;
if (dev_priv->guc.execbuf_client)
i915_guc_submit(dev_priv->guc.execbuf_client, request);
@@ -1015,8 +1016,6 @@ void intel_execlists_retire_requests(struct intel_engine_cs *engine)
spin_unlock_bh(&engine->execlist_lock);
list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) {
- intel_lr_context_unpin(req->ctx, engine);
-
list_del(&req->execlist_link);
i915_gem_request_unreference(req);
}
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [v2,01/12] drm/i915: Mark the current context as lost on suspend
2016-04-19 11:40 Premature unpinning Chris Wilson
` (12 preceding siblings ...)
2016-04-19 12:59 ` Update to patches 11 and 12 Chris Wilson
@ 2016-04-19 13:24 ` Patchwork
2016-04-19 14:58 ` ✗ Fi.CI.BAT: failure for series starting with [v2] drm/i915: Mark the current context as lost on suspend (rev2) Patchwork
14 siblings, 0 replies; 31+ messages in thread
From: Patchwork @ 2016-04-19 13:24 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,01/12] drm/i915: Mark the current context as lost on suspend
URL : https://patchwork.freedesktop.org/series/5926/
State : failure
== Summary ==
Series 5926v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5926/revisions/1/mbox/
Test drv_module_reload_basic:
pass -> DMESG-WARN (skl-nuci5)
pass -> DMESG-WARN (hsw-gt2)
pass -> DMESG-WARN (bdw-ultra)
pass -> DMESG-WARN (skl-i7k-2)
pass -> DMESG-WARN (ivb-t430s)
pass -> DMESG-WARN (byt-nuc)
pass -> DMESG-WARN (snb-x220t)
pass -> DMESG-WARN (snb-dellxps)
pass -> DMESG-WARN (hsw-brixbox)
pass -> DMESG-WARN (bdw-nuci7)
Test gem_mmap_gtt:
Subgroup basic-copy:
pass -> INCOMPLETE (ilk-hp8440p)
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
pass -> DMESG-WARN (ivb-t430s)
bdw-nuci7 total:192 pass:179 dwarn:1 dfail:0 fail:0 skip:12
bdw-ultra total:127 pass:104 dwarn:1 dfail:0 fail:0 skip:22
bsw-nuc-2 total:191 pass:152 dwarn:0 dfail:0 fail:0 skip:39
byt-nuc total:191 pass:152 dwarn:1 dfail:0 fail:0 skip:38
hsw-brixbox total:192 pass:167 dwarn:1 dfail:0 fail:0 skip:24
hsw-gt2 total:192 pass:172 dwarn:1 dfail:0 fail:0 skip:19
ilk-hp8440p total:1 pass:0 dwarn:0 dfail:0 fail:0 skip:0
ivb-t430s total:192 pass:162 dwarn:2 dfail:0 fail:0 skip:28
skl-i7k-2 total:192 pass:166 dwarn:1 dfail:0 fail:0 skip:25
skl-nuci5 total:192 pass:180 dwarn:1 dfail:0 fail:0 skip:11
snb-dellxps total:192 pass:153 dwarn:1 dfail:0 fail:0 skip:38
snb-x220t total:192 pass:153 dwarn:1 dfail:0 fail:1 skip:37
Results at /archive/results/CI_IGT_test/Patchwork_1940/
83dde235b9d8bbe1cabf7ad002a6c48ff5a699fc drm-intel-nightly: 2016y-04m-19d-11h-58m-43s UTC integration manifest
742cfa7 drm/i915: Move releasing of the GEM request from free to retire/cancel
bca9c3e drm/i915: Track the previous pinned context inside the request
0359cdc drm/i915: Move the magical deferred context allocation into the request
efead2c drm/i915: Move context initialisation to first-use
b2320a2 drm/i915: Refactor execlists default context pinning
60a484f drm/i915: Replace the pinned context address with its unique ID
5ad9b92 drm/i915: Assign every HW context a unique ID
51c5bfa drm/i915: Rearrange switch_context to load the aliasing ppgtt on first use
3d4bfa5 drm/i915: Remove early l3-remap
bae315c drm/i915: Consolidate L3 remapping LRI
f308724 drm/i915: L3 cache remapping is part of context switching
b4f4efb drm/i915: Mark the current context as lost on suspend
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v2] drm/i915: Mark the current context as lost on suspend
2016-04-19 11:40 ` [PATCH v2 01/12] drm/i915: Mark the current context as lost on suspend Chris Wilson
@ 2016-04-19 14:12 ` Chris Wilson
2016-04-19 14:20 ` Mika Kuoppala
0 siblings, 1 reply; 31+ messages in thread
From: Chris Wilson @ 2016-04-19 14:12 UTC (permalink / raw)
To: intel-gfx
In order to force a reload of the context image upon resume, we first
need to mark its absence on suspend. Currently we are failing to restore
the golden context state and any context w/a to the default context
after resume.
One oversight corrected, is that we had forgotten to reapply the L3
remapping when restoring the lost default context.
v2: Remove deprecated WARN.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_gem.c | 1 +
drivers/gpu/drm/i915/i915_gem_context.c | 54 ++++++++++++++-------------------
3 files changed, 25 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 85102ad75962..595037bec2de 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3299,6 +3299,7 @@ void i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj);
/* i915_gem_context.c */
int __must_check i915_gem_context_init(struct drm_device *dev);
+void i915_gem_context_lost(struct drm_i915_private *dev_priv);
void i915_gem_context_fini(struct drm_device *dev);
void i915_gem_context_reset(struct drm_device *dev);
int i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6ce2c31b9a81..e7fe29857e23 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4711,6 +4711,7 @@ i915_gem_suspend(struct drm_device *dev)
i915_gem_retire_requests(dev);
i915_gem_stop_engines(dev);
+ i915_gem_context_lost(dev_priv);
mutex_unlock(&dev->struct_mutex);
cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index e5acc3916f75..bf31ee1ed914 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -90,6 +90,8 @@
#include "i915_drv.h"
#include "i915_trace.h"
+#define ALL_L3_SLICES(dev) (1 << NUM_L3_SLICES(dev)) - 1
+
/* This is a HW constraint. The value below is the largest known requirement
* I've seen in a spec to date, and that was a workaround for a non-shipping
* part. It should be safe to decrease this, but it's more future proof as is.
@@ -249,7 +251,7 @@ __create_hw_context(struct drm_device *dev,
/* NB: Mark all slices as needing a remap so that when the context first
* loads it will restore whatever remap state already exists. If there
* is no remap info, it will be a NOP. */
- ctx->remap_slice = (1 << NUM_L3_SLICES(dev)) - 1;
+ ctx->remap_slice = ALL_L3_SLICES(dev_priv);
ctx->hang_stats.ban_period_seconds = DRM_I915_CTX_BAN_PERIOD;
@@ -336,7 +338,6 @@ static void i915_gem_context_unpin(struct intel_context *ctx,
void i915_gem_context_reset(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- int i;
if (i915.enable_execlists) {
struct intel_context *ctx;
@@ -345,17 +346,7 @@ void i915_gem_context_reset(struct drm_device *dev)
intel_lr_context_reset(dev_priv, ctx);
}
- for (i = 0; i < I915_NUM_ENGINES; i++) {
- struct intel_engine_cs *engine = &dev_priv->engine[i];
-
- if (engine->last_context) {
- i915_gem_context_unpin(engine->last_context, engine);
- engine->last_context = NULL;
- }
- }
-
- /* Force the GPU state to be reinitialised on enabling */
- dev_priv->kernel_context->legacy_hw_ctx.initialized = false;
+ i915_gem_context_lost(dev_priv);
}
int i915_gem_context_init(struct drm_device *dev)
@@ -403,11 +394,29 @@ int i915_gem_context_init(struct drm_device *dev)
return 0;
}
+void i915_gem_context_lost(struct drm_i915_private *dev_priv)
+{
+ struct intel_engine_cs *engine;
+
+ for_each_engine(engine, dev_priv) {
+ if (engine->last_context == NULL)
+ continue;
+
+ i915_gem_context_unpin(engine->last_context, engine);
+ engine->last_context = NULL;
+ }
+
+ /* Force the GPU state to be reinitialised on enabling */
+ dev_priv->kernel_context->legacy_hw_ctx.initialized = false;
+ dev_priv->kernel_context->remap_slice = ALL_L3_SLICES(dev_priv);
+}
+
void i915_gem_context_fini(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_context *dctx = dev_priv->kernel_context;
- int i;
+
+ i915_gem_context_lost(dev_priv);
if (dctx->legacy_hw_ctx.rcs_state) {
/* The only known way to stop the gpu from accessing the hw context is
@@ -415,26 +424,9 @@ void i915_gem_context_fini(struct drm_device *dev)
* other code, leading to spurious errors. */
intel_gpu_reset(dev, ALL_ENGINES);
- /* When default context is created and switched to, base object refcount
- * will be 2 (+1 from object creation and +1 from do_switch()).
- * i915_gem_context_fini() will be called after gpu_idle() has switched
- * to default context. So we need to unreference the base object once
- * to offset the do_switch part, so that i915_gem_context_unreference()
- * can then free the base object correctly. */
- WARN_ON(!dev_priv->engine[RCS].last_context);
-
i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
}
- for (i = I915_NUM_ENGINES; --i >= 0;) {
- struct intel_engine_cs *engine = &dev_priv->engine[i];
-
- if (engine->last_context) {
- i915_gem_context_unpin(engine->last_context, engine);
- engine->last_context = NULL;
- }
- }
-
i915_gem_context_unreference(dctx);
dev_priv->kernel_context = NULL;
}
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2] drm/i915: Mark the current context as lost on suspend
2016-04-19 14:12 ` [PATCH v2] " Chris Wilson
@ 2016-04-19 14:20 ` Mika Kuoppala
0 siblings, 0 replies; 31+ messages in thread
From: Mika Kuoppala @ 2016-04-19 14:20 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> [ text/plain ]
> In order to force a reload of the context image upon resume, we first
> need to mark its absence on suspend. Currently we are failing to restore
> the golden context state and any context w/a to the default context
> after resume.
>
> One oversight corrected, is that we had forgotten to reapply the L3
> remapping when restoring the lost default context.
>
> v2: Remove deprecated WARN.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_gem.c | 1 +
> drivers/gpu/drm/i915/i915_gem_context.c | 54 ++++++++++++++-------------------
> 3 files changed, 25 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 85102ad75962..595037bec2de 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3299,6 +3299,7 @@ void i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj);
>
> /* i915_gem_context.c */
> int __must_check i915_gem_context_init(struct drm_device *dev);
> +void i915_gem_context_lost(struct drm_i915_private *dev_priv);
> void i915_gem_context_fini(struct drm_device *dev);
> void i915_gem_context_reset(struct drm_device *dev);
> int i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6ce2c31b9a81..e7fe29857e23 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4711,6 +4711,7 @@ i915_gem_suspend(struct drm_device *dev)
> i915_gem_retire_requests(dev);
>
> i915_gem_stop_engines(dev);
> + i915_gem_context_lost(dev_priv);
> mutex_unlock(&dev->struct_mutex);
>
> cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index e5acc3916f75..bf31ee1ed914 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -90,6 +90,8 @@
> #include "i915_drv.h"
> #include "i915_trace.h"
>
> +#define ALL_L3_SLICES(dev) (1 << NUM_L3_SLICES(dev)) - 1
> +
> /* This is a HW constraint. The value below is the largest known requirement
> * I've seen in a spec to date, and that was a workaround for a non-shipping
> * part. It should be safe to decrease this, but it's more future proof as is.
> @@ -249,7 +251,7 @@ __create_hw_context(struct drm_device *dev,
> /* NB: Mark all slices as needing a remap so that when the context first
> * loads it will restore whatever remap state already exists. If there
> * is no remap info, it will be a NOP. */
> - ctx->remap_slice = (1 << NUM_L3_SLICES(dev)) - 1;
> + ctx->remap_slice = ALL_L3_SLICES(dev_priv);
>
> ctx->hang_stats.ban_period_seconds = DRM_I915_CTX_BAN_PERIOD;
>
> @@ -336,7 +338,6 @@ static void i915_gem_context_unpin(struct intel_context *ctx,
> void i915_gem_context_reset(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - int i;
>
> if (i915.enable_execlists) {
> struct intel_context *ctx;
> @@ -345,17 +346,7 @@ void i915_gem_context_reset(struct drm_device *dev)
> intel_lr_context_reset(dev_priv, ctx);
> }
>
> - for (i = 0; i < I915_NUM_ENGINES; i++) {
> - struct intel_engine_cs *engine = &dev_priv->engine[i];
> -
> - if (engine->last_context) {
> - i915_gem_context_unpin(engine->last_context, engine);
> - engine->last_context = NULL;
> - }
> - }
> -
> - /* Force the GPU state to be reinitialised on enabling */
> - dev_priv->kernel_context->legacy_hw_ctx.initialized = false;
> + i915_gem_context_lost(dev_priv);
> }
>
> int i915_gem_context_init(struct drm_device *dev)
> @@ -403,11 +394,29 @@ int i915_gem_context_init(struct drm_device *dev)
> return 0;
> }
>
> +void i915_gem_context_lost(struct drm_i915_private *dev_priv)
> +{
> + struct intel_engine_cs *engine;
> +
> + for_each_engine(engine, dev_priv) {
> + if (engine->last_context == NULL)
> + continue;
> +
> + i915_gem_context_unpin(engine->last_context, engine);
> + engine->last_context = NULL;
> + }
> +
> + /* Force the GPU state to be reinitialised on enabling */
> + dev_priv->kernel_context->legacy_hw_ctx.initialized = false;
> + dev_priv->kernel_context->remap_slice = ALL_L3_SLICES(dev_priv);
> +}
> +
> void i915_gem_context_fini(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_context *dctx = dev_priv->kernel_context;
> - int i;
> +
> + i915_gem_context_lost(dev_priv);
>
> if (dctx->legacy_hw_ctx.rcs_state) {
> /* The only known way to stop the gpu from accessing the hw context is
> @@ -415,26 +424,9 @@ void i915_gem_context_fini(struct drm_device *dev)
> * other code, leading to spurious errors. */
> intel_gpu_reset(dev, ALL_ENGINES);
>
> - /* When default context is created and switched to, base object refcount
> - * will be 2 (+1 from object creation and +1 from do_switch()).
> - * i915_gem_context_fini() will be called after gpu_idle() has switched
> - * to default context. So we need to unreference the base object once
> - * to offset the do_switch part, so that i915_gem_context_unreference()
> - * can then free the base object correctly. */
> - WARN_ON(!dev_priv->engine[RCS].last_context);
> -
> i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
> }
>
> - for (i = I915_NUM_ENGINES; --i >= 0;) {
> - struct intel_engine_cs *engine = &dev_priv->engine[i];
> -
> - if (engine->last_context) {
> - i915_gem_context_unpin(engine->last_context, engine);
> - engine->last_context = NULL;
> - }
> - }
> -
> i915_gem_context_unreference(dctx);
> dev_priv->kernel_context = NULL;
> }
> --
> 2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 31+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [v2] drm/i915: Mark the current context as lost on suspend (rev2)
2016-04-19 11:40 Premature unpinning Chris Wilson
` (13 preceding siblings ...)
2016-04-19 13:24 ` ✗ Fi.CI.BAT: failure for series starting with [v2,01/12] drm/i915: Mark the current context as lost on suspend Patchwork
@ 2016-04-19 14:58 ` Patchwork
14 siblings, 0 replies; 31+ messages in thread
From: Patchwork @ 2016-04-19 14:58 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2] drm/i915: Mark the current context as lost on suspend (rev2)
URL : https://patchwork.freedesktop.org/series/5926/
State : failure
== Summary ==
Series 5926v2 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5926/revisions/2/mbox/
Test drv_module_reload_basic:
pass -> DMESG-WARN (hsw-gt2)
pass -> DMESG-WARN (bdw-ultra)
pass -> DMESG-WARN (skl-i7k-2)
Test gem_exec_basic:
Subgroup readonly-bsd2:
skip -> INCOMPLETE (ilk-hp8440p)
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
pass -> DMESG-WARN (snb-dellxps)
bdw-nuci7 total:192 pass:180 dwarn:0 dfail:0 fail:0 skip:12
bdw-ultra total:127 pass:104 dwarn:1 dfail:0 fail:0 skip:22
bsw-nuc-2 total:191 pass:152 dwarn:0 dfail:0 fail:0 skip:39
byt-nuc total:191 pass:153 dwarn:0 dfail:0 fail:0 skip:38
hsw-brixbox total:192 pass:168 dwarn:0 dfail:0 fail:0 skip:24
hsw-gt2 total:192 pass:172 dwarn:1 dfail:0 fail:0 skip:19
ilk-hp8440p total:1 pass:0 dwarn:0 dfail:0 fail:0 skip:0
ivb-t430s total:192 pass:164 dwarn:0 dfail:0 fail:0 skip:28
skl-i7k-2 total:192 pass:166 dwarn:1 dfail:0 fail:0 skip:25
skl-nuci5 total:192 pass:181 dwarn:0 dfail:0 fail:0 skip:11
snb-dellxps total:192 pass:153 dwarn:1 dfail:0 fail:0 skip:38
snb-x220t total:192 pass:154 dwarn:0 dfail:0 fail:1 skip:37
Results at /archive/results/CI_IGT_test/Patchwork_1944/
83dde235b9d8bbe1cabf7ad002a6c48ff5a699fc drm-intel-nightly: 2016y-04m-19d-11h-58m-43s UTC integration manifest
5cd3f27 drm/i915: Move releasing of the GEM request from free to retire/cancel
6f8e811 drm/i915: Track the previous pinned context inside the request
b1e76d1 drm/i915: Move the magical deferred context allocation into the request
1deeae2 drm/i915: Move context initialisation to first-use
698bcb6 drm/i915: Refactor execlists default context pinning
34e685b drm/i915: Replace the pinned context address with its unique ID
e5bcb62 drm/i915: Assign every HW context a unique ID
6776f91 drm/i915: Rearrange switch_context to load the aliasing ppgtt on first use
37506b6 drm/i915: Remove early l3-remap
3f477c5 drm/i915: Consolidate L3 remapping LRI
9286a09 drm/i915: L3 cache remapping is part of context switching
98b2409 drm/i915: Mark the current context as lost on suspend
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/2] drm/i915: Move releasing of the GEM request from free to retire/cancel
2016-04-19 12:59 ` [PATCH 1/2] drm/i915: Move releasing of the GEM request from free to retire/cancel Chris Wilson
@ 2016-04-20 13:55 ` Tvrtko Ursulin
0 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2016-04-20 13:55 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 19/04/16 13:59, Chris Wilson wrote:
> If we move the release of the GEM request (i.e. decoupling it from the
> various lists used for client and context tracking) after it is complete
> (either by the GPU retiring the request, or by the caller cancelling the
> request), we can remove the requirement that the final unreference of
> the GEM request need to be under the struct_mutex.
>
> v2,v3: Rebalance execlists by moving the context unpinning.
> v4: Rebase onto -nightly
> v5: Avoid trying to rebalance execlist/GuC context pinning, leave that
> to the next step
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 14 --------------
> drivers/gpu/drm/i915/i915_gem.c | 23 +++++++++--------------
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/i915/intel_pm.c | 2 +-
> 4 files changed, 11 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c0a6fbff4678..c59b2670cc36 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2370,23 +2370,9 @@ i915_gem_request_reference(struct drm_i915_gem_request *req)
> static inline void
> i915_gem_request_unreference(struct drm_i915_gem_request *req)
> {
> - WARN_ON(!mutex_is_locked(&req->engine->dev->struct_mutex));
> kref_put(&req->ref, i915_gem_request_free);
> }
>
> -static inline void
> -i915_gem_request_unreference__unlocked(struct drm_i915_gem_request *req)
> -{
> - struct drm_device *dev;
> -
> - if (!req)
> - return;
> -
> - dev = req->engine->dev;
> - if (kref_put_mutex(&req->ref, i915_gem_request_free, &dev->struct_mutex))
> - mutex_unlock(&dev->struct_mutex);
> -}
> -
> static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst,
> struct drm_i915_gem_request *src)
> {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 261185281e78..9b4854a17264 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1413,6 +1413,13 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
> list_del_init(&request->list);
> i915_gem_request_remove_from_client(request);
>
> + if (request->ctx) {
> + if (i915.enable_execlists)
> + intel_lr_context_unpin(request->ctx, request->engine);
> +
> + i915_gem_context_unreference(request->ctx);
> + }
> +
> i915_gem_request_unreference(request);
> }
>
> @@ -2713,18 +2720,6 @@ void i915_gem_request_free(struct kref *req_ref)
> {
> struct drm_i915_gem_request *req = container_of(req_ref,
> typeof(*req), ref);
> - struct intel_context *ctx = req->ctx;
> -
> - if (req->file_priv)
> - i915_gem_request_remove_from_client(req);
> -
> - if (ctx) {
> - if (i915.enable_execlists)
> - intel_lr_context_unpin(ctx, req->engine);
> -
> - i915_gem_context_unreference(ctx);
> - }
> -
> kmem_cache_free(req->i915->requests, req);
> }
>
> @@ -3186,7 +3181,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> ret = __i915_wait_request(req[i], true,
> args->timeout_ns > 0 ? &args->timeout_ns : NULL,
> to_rps_client(file));
> - i915_gem_request_unreference__unlocked(req[i]);
> + i915_gem_request_unreference(req[i]);
> }
> return ret;
>
> @@ -4199,7 +4194,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
> if (ret == 0)
> queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, 0);
>
> - i915_gem_request_unreference__unlocked(target);
> + i915_gem_request_unreference(target);
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ff60241b1f76..f5bf46f99cc2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11399,7 +11399,7 @@ static void intel_mmio_flip_work_func(struct work_struct *work)
> WARN_ON(__i915_wait_request(mmio_flip->req,
> false, NULL,
> &mmio_flip->i915->rps.mmioflips));
> - i915_gem_request_unreference__unlocked(mmio_flip->req);
> + i915_gem_request_unreference(mmio_flip->req);
> }
>
> /* For framebuffer backed by dmabuf, wait for fence */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b7c218602c6e..ed3797bf41aa 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7366,7 +7366,7 @@ static void __intel_rps_boost_work(struct work_struct *work)
> gen6_rps_boost(to_i915(req->engine->dev), NULL,
> req->emitted_jiffies);
>
> - i915_gem_request_unreference__unlocked(req);
> + i915_gem_request_unreference(req);
> kfree(boost);
> }
>
>
If I am not too close to this:
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] 31+ messages in thread
* Re: [PATCH v2 08/12] drm/i915: Refactor execlists default context pinning
2016-04-19 11:40 ` [PATCH v2 08/12] drm/i915: Refactor execlists default context pinning Chris Wilson
2016-04-19 12:25 ` Tvrtko Ursulin
@ 2016-04-20 14:07 ` Mika Kuoppala
1 sibling, 0 replies; 31+ messages in thread
From: Mika Kuoppala @ 2016-04-20 14:07 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> [ text/plain ]
> Refactor pinning and unpinning of contexts, such that the default
> context for an engine is pinned during initialisation and unpinned
> during teardown (pinning of the context handles the reference counting).
> Thus we can eliminate the special case handling of the default context
> that was required to mask that it was not being pinned normally.
>
> v2: Rebalance context_queue after rebasing.
> v3: Rebase to -nightly (not 40 patches in)
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
I have done this atleast once so I am fan,
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
> drivers/gpu/drm/i915/i915_gem.c | 2 +-
> drivers/gpu/drm/i915/intel_lrc.c | 107 ++++++++++++++----------------------
> 3 files changed, 43 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index f775451bd0b6..e81a7504656e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2095,9 +2095,8 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
> return ret;
>
> list_for_each_entry(ctx, &dev_priv->context_list, link)
> - if (ctx != dev_priv->kernel_context)
> - for_each_engine(engine, dev_priv)
> - i915_dump_lrc_obj(m, ctx, engine);
> + for_each_engine(engine, dev_priv)
> + i915_dump_lrc_obj(m, ctx, engine);
>
> mutex_unlock(&dev->struct_mutex);
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b95d5f83d3b0..261185281e78 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2719,7 +2719,7 @@ void i915_gem_request_free(struct kref *req_ref)
> i915_gem_request_remove_from_client(req);
>
> if (ctx) {
> - if (i915.enable_execlists && ctx != req->i915->kernel_context)
> + if (i915.enable_execlists)
> intel_lr_context_unpin(ctx, req->engine);
>
> i915_gem_context_unreference(ctx);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index dedd82aea386..e064a6ae2d97 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -588,9 +588,7 @@ static void execlists_context_queue(struct drm_i915_gem_request *request)
> struct drm_i915_gem_request *cursor;
> int num_elements = 0;
>
> - if (request->ctx != request->i915->kernel_context)
> - intel_lr_context_pin(request->ctx, engine);
> -
> + intel_lr_context_pin(request->ctx, request->engine);
> i915_gem_request_reference(request);
>
> spin_lock_bh(&engine->execlist_lock);
> @@ -691,10 +689,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
> return ret;
> }
>
> - if (request->ctx != request->i915->kernel_context)
> - ret = intel_lr_context_pin(request->ctx, request->engine);
> -
> - return ret;
> + return intel_lr_context_pin(request->ctx, request->engine);
> }
>
> static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
> @@ -774,12 +769,8 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
> if (engine->last_context != request->ctx) {
> if (engine->last_context)
> intel_lr_context_unpin(engine->last_context, engine);
> - if (request->ctx != request->i915->kernel_context) {
> - intel_lr_context_pin(request->ctx, engine);
> - engine->last_context = request->ctx;
> - } else {
> - engine->last_context = NULL;
> - }
> + intel_lr_context_pin(request->ctx, engine);
> + engine->last_context = request->ctx;
> }
>
> if (dev_priv->guc.execbuf_client)
> @@ -1002,12 +993,7 @@ void intel_execlists_retire_requests(struct intel_engine_cs *engine)
> spin_unlock_bh(&engine->execlist_lock);
>
> list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) {
> - struct intel_context *ctx = req->ctx;
> - struct drm_i915_gem_object *ctx_obj =
> - ctx->engine[engine->id].state;
> -
> - if (ctx_obj && (ctx != req->i915->kernel_context))
> - intel_lr_context_unpin(ctx, engine);
> + intel_lr_context_unpin(req->ctx, engine);
>
> list_del(&req->execlist_link);
> i915_gem_request_unreference(req);
> @@ -1052,23 +1038,26 @@ int logical_ring_flush_all_caches(struct drm_i915_gem_request *req)
> return 0;
> }
>
> -static int intel_lr_context_do_pin(struct intel_context *ctx,
> - struct intel_engine_cs *engine)
> +static int intel_lr_context_pin(struct intel_context *ctx,
> + struct intel_engine_cs *engine)
> {
> - struct drm_device *dev = engine->dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
> - struct intel_ringbuffer *ringbuf = ctx->engine[engine->id].ringbuf;
> + struct drm_i915_private *dev_priv = ctx->i915;
> + struct drm_i915_gem_object *ctx_obj;
> + struct intel_ringbuffer *ringbuf;
> void *vaddr;
> u32 *lrc_reg_state;
> int ret;
>
> - WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
> + lockdep_assert_held(&ctx->i915->dev->struct_mutex);
>
> + if (ctx->engine[engine->id].pin_count++)
> + return 0;
> +
> + ctx_obj = ctx->engine[engine->id].state;
> ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN,
> PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> if (ret)
> - return ret;
> + goto err;
>
> vaddr = i915_gem_object_pin_map(ctx_obj);
> if (IS_ERR(vaddr)) {
> @@ -1078,10 +1067,12 @@ static int intel_lr_context_do_pin(struct intel_context *ctx,
>
> lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
>
> + ringbuf = ctx->engine[engine->id].ringbuf;
> ret = intel_pin_and_map_ringbuffer_obj(engine->dev, ringbuf);
> if (ret)
> goto unpin_map;
>
> + i915_gem_context_reference(ctx);
> ctx->engine[engine->id].lrc_vma = i915_gem_obj_to_ggtt(ctx_obj);
> intel_lr_context_descriptor_update(ctx, engine);
> lrc_reg_state[CTX_RING_BUFFER_START+1] = ringbuf->vma->node.start;
> @@ -1092,51 +1083,39 @@ static int intel_lr_context_do_pin(struct intel_context *ctx,
> if (i915.enable_guc_submission)
> I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
>
> - return ret;
> + return 0;
>
> unpin_map:
> i915_gem_object_unpin_map(ctx_obj);
> unpin_ctx_obj:
> i915_gem_object_ggtt_unpin(ctx_obj);
> -
> +err:
> + ctx->engine[engine->id].pin_count = 0;
> return ret;
> }
>
> -static int intel_lr_context_pin(struct intel_context *ctx,
> - struct intel_engine_cs *engine)
> +void intel_lr_context_unpin(struct intel_context *ctx,
> + struct intel_engine_cs *engine)
> {
> - int ret = 0;
> + struct drm_i915_gem_object *ctx_obj;
>
> - if (ctx->engine[engine->id].pin_count++ == 0) {
> - ret = intel_lr_context_do_pin(ctx, engine);
> - if (ret)
> - goto reset_pin_count;
> + lockdep_assert_held(&ctx->i915->dev->struct_mutex);
> + GEM_BUG_ON(ctx->engine[engine->id].pin_count == 0);
>
> - i915_gem_context_reference(ctx);
> - }
> - return ret;
> + if (--ctx->engine[engine->id].pin_count)
> + return;
>
> -reset_pin_count:
> - ctx->engine[engine->id].pin_count = 0;
> - return ret;
> -}
> + intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
>
> -void intel_lr_context_unpin(struct intel_context *ctx,
> - struct intel_engine_cs *engine)
> -{
> - struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
> + ctx_obj = ctx->engine[engine->id].state;
> + i915_gem_object_unpin_map(ctx_obj);
> + i915_gem_object_ggtt_unpin(ctx_obj);
>
> - WARN_ON(!mutex_is_locked(&ctx->i915->dev->struct_mutex));
> - if (--ctx->engine[engine->id].pin_count == 0) {
> - i915_gem_object_unpin_map(ctx_obj);
> - intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
> - i915_gem_object_ggtt_unpin(ctx_obj);
> - ctx->engine[engine->id].lrc_vma = NULL;
> - ctx->engine[engine->id].lrc_desc = 0;
> - ctx->engine[engine->id].lrc_reg_state = NULL;
> + ctx->engine[engine->id].lrc_vma = NULL;
> + ctx->engine[engine->id].lrc_desc = 0;
> + ctx->engine[engine->id].lrc_reg_state = NULL;
>
> - i915_gem_context_unreference(ctx);
> - }
> + i915_gem_context_unreference(ctx);
> }
>
> static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request *req)
> @@ -2034,6 +2013,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
> i915_gem_object_unpin_map(engine->status_page.obj);
> engine->status_page.obj = NULL;
> }
> + intel_lr_context_unpin(dev_priv->kernel_context, engine);
>
> engine->idle_lite_restore_wa = 0;
> engine->disable_lite_restore_wa = false;
> @@ -2137,11 +2117,10 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
> goto error;
>
> /* As this is the default context, always pin it */
> - ret = intel_lr_context_do_pin(dctx, engine);
> + ret = intel_lr_context_pin(dctx, engine);
> if (ret) {
> - DRM_ERROR(
> - "Failed to pin and map ringbuffer %s: %d\n",
> - engine->name, ret);
> + DRM_ERROR("Failed to pin context for %s: %d\n",
> + engine->name, ret);
> goto error;
> }
>
> @@ -2562,12 +2541,6 @@ void intel_lr_context_free(struct intel_context *ctx)
> if (!ctx_obj)
> continue;
>
> - if (ctx == ctx->i915->kernel_context) {
> - intel_unpin_ringbuffer_obj(ringbuf);
> - i915_gem_object_ggtt_unpin(ctx_obj);
> - i915_gem_object_unpin_map(ctx_obj);
> - }
> -
> WARN_ON(ctx->engine[i].pin_count);
> intel_ringbuffer_free(ringbuf);
> drm_gem_object_unreference(&ctx_obj->base);
> --
> 2.8.0.rc3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/2] drm/i915: Track the previous pinned context inside the request
2016-04-19 12:59 ` [PATCH 2/2] drm/i915: Track the previous pinned context inside the request Chris Wilson
@ 2016-04-20 14:08 ` Tvrtko Ursulin
2016-04-20 14:18 ` Chris Wilson
` (2 more replies)
0 siblings, 3 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2016-04-20 14:08 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 19/04/16 13:59, Chris Wilson wrote:
> As the contexts are accessed by the hardware until the switch is completed
> to a new context, the hardware may still be writing to the context object
> after the breadcrumb is visible. We must not unpin/unbind/prune that
> object whilst still active and so we keep the previous context pinned until
> the following request. If we move this tracking onto the request, we can
> simplify the code and treat execlists/GuC dispatch identically.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++++
> drivers/gpu/drm/i915/i915_gem.c | 8 ++++----
> drivers/gpu/drm/i915/intel_lrc.c | 17 ++++++++---------
> 3 files changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c59b2670cc36..be98e9643072 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2302,6 +2302,17 @@ struct drm_i915_gem_request {
> struct intel_context *ctx;
> struct intel_ringbuffer *ringbuf;
>
> + /**
> + * Context related to the previous request.
> + * As the contexts are accessed by the hardware until the switch is
> + * completed to a new context, the hardware may still be writing
> + * to the context object after the breadcrumb is visible. We must
> + * not unpin/unbind/prune that object whilst still active and so
> + * we keep the previous context pinned until the following (this)
> + * request is retired.
> + */
> + struct intel_context *previous_context;
> +
> /** Batch buffer related to this request if any (used for
> error state dump only) */
> struct drm_i915_gem_object *batch_obj;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 9b4854a17264..537aacfda3eb 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1413,13 +1413,13 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
> list_del_init(&request->list);
> i915_gem_request_remove_from_client(request);
>
> - if (request->ctx) {
> + if (request->previous_context) {
> if (i915.enable_execlists)
> - intel_lr_context_unpin(request->ctx, request->engine);
> -
> - i915_gem_context_unreference(request->ctx);
> + intel_lr_context_unpin(request->previous_context,
> + request->engine);
> }
>
> + i915_gem_context_unreference(request->ctx);
> i915_gem_request_unreference(request);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index ee4e9bb80042..06e013293ec6 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -590,7 +590,6 @@ static void execlists_context_queue(struct drm_i915_gem_request *request)
> struct drm_i915_gem_request *cursor;
> int num_elements = 0;
>
> - intel_lr_context_pin(request->ctx, request->engine);
I really really think this must go in a separate, subsequent patch.
Both from the conceptual side, leaving this patch to just extend
pinning, not limit it; and from the POV that there is a bug unless a
patch like mine which I pasted in yesterday is inserted between them
("drm/i915: Store LRC hardware id in the context", note the summary is
wrong, it is storing in requests not contexts so I have to rename it).
Otherwise execlists_check_remove_request when accessing head_req->ctx is
use after free. And I can demonstrate that easily via gem-close-race.
Put a WARN_ON(atomic_read(&head_req->ctx->ref.refcount) == 0); and see. :)
What I think happens is that with two submission ports, we can get two
context completions aggregated in an interrupt which comes after the
seqno for both has been consumed by GEM and so LRCs unpinned.
But with your persistent ctx hw id patches, I think the course is fine
to do this including the complete elimination of the execlist retired queue.
You can just drop the two chunks for the patch and I will follow up with
two patches to finish it all off.
> i915_gem_request_reference(request);
>
> spin_lock_bh(&engine->execlist_lock);
> @@ -788,12 +787,14 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
> if (intel_engine_stopped(engine))
> return 0;
>
> - if (engine->last_context != request->ctx) {
> - if (engine->last_context)
> - intel_lr_context_unpin(engine->last_context, engine);
> - intel_lr_context_pin(request->ctx, engine);
> - engine->last_context = request->ctx;
> - }
> + /* We keep the previous context alive until we retire the following
> + * request. This ensures that any the context object is still pinned
> + * for any residual writes the HW makes into it on the context switch
> + * into the next object following the breadcrumb. Otherwise, we may
> + * retire the context too early.
> + */
> + request->previous_context = engine->last_context;
> + engine->last_context = request->ctx;
>
> if (dev_priv->guc.execbuf_client)
> i915_guc_submit(dev_priv->guc.execbuf_client, request);
> @@ -1015,8 +1016,6 @@ void intel_execlists_retire_requests(struct intel_engine_cs *engine)
> spin_unlock_bh(&engine->execlist_lock);
>
> list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) {
> - intel_lr_context_unpin(req->ctx, engine);
> -
> list_del(&req->execlist_link);
> i915_gem_request_unreference(req);
> }
>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/2] drm/i915: Track the previous pinned context inside the request
2016-04-20 14:08 ` Tvrtko Ursulin
@ 2016-04-20 14:18 ` Chris Wilson
2016-04-20 14:22 ` Chris Wilson
2016-04-20 14:24 ` [RFC 1/2] drm/i915: Store LRC hardware id in " Tvrtko Ursulin
2 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2016-04-20 14:18 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Wed, Apr 20, 2016 at 03:08:19PM +0100, Tvrtko Ursulin wrote:
>
> On 19/04/16 13:59, Chris Wilson wrote:
> >As the contexts are accessed by the hardware until the switch is completed
> >to a new context, the hardware may still be writing to the context object
> >after the breadcrumb is visible. We must not unpin/unbind/prune that
> >object whilst still active and so we keep the previous context pinned until
> >the following request. If we move this tracking onto the request, we can
> >simplify the code and treat execlists/GuC dispatch identically.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> > drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++++
> > drivers/gpu/drm/i915/i915_gem.c | 8 ++++----
> > drivers/gpu/drm/i915/intel_lrc.c | 17 ++++++++---------
> > 3 files changed, 23 insertions(+), 13 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index c59b2670cc36..be98e9643072 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -2302,6 +2302,17 @@ struct drm_i915_gem_request {
> > struct intel_context *ctx;
> > struct intel_ringbuffer *ringbuf;
> >
> >+ /**
> >+ * Context related to the previous request.
> >+ * As the contexts are accessed by the hardware until the switch is
> >+ * completed to a new context, the hardware may still be writing
> >+ * to the context object after the breadcrumb is visible. We must
> >+ * not unpin/unbind/prune that object whilst still active and so
> >+ * we keep the previous context pinned until the following (this)
> >+ * request is retired.
> >+ */
> >+ struct intel_context *previous_context;
> >+
> > /** Batch buffer related to this request if any (used for
> > error state dump only) */
> > struct drm_i915_gem_object *batch_obj;
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 9b4854a17264..537aacfda3eb 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -1413,13 +1413,13 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
> > list_del_init(&request->list);
> > i915_gem_request_remove_from_client(request);
> >
> >- if (request->ctx) {
> >+ if (request->previous_context) {
> > if (i915.enable_execlists)
> >- intel_lr_context_unpin(request->ctx, request->engine);
> >-
> >- i915_gem_context_unreference(request->ctx);
> >+ intel_lr_context_unpin(request->previous_context,
> >+ request->engine);
> > }
> >
> >+ i915_gem_context_unreference(request->ctx);
> > i915_gem_request_unreference(request);
> > }
> >
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index ee4e9bb80042..06e013293ec6 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -590,7 +590,6 @@ static void execlists_context_queue(struct drm_i915_gem_request *request)
> > struct drm_i915_gem_request *cursor;
> > int num_elements = 0;
> >
> >- intel_lr_context_pin(request->ctx, request->engine);
>
> I really really think this must go in a separate, subsequent patch.
>
> Both from the conceptual side, leaving this patch to just extend
> pinning, not limit it; and from the POV that there is a bug unless a
> patch like mine which I pasted in yesterday is inserted between them
> ("drm/i915: Store LRC hardware id in the context", note the summary
> is wrong, it is storing in requests not contexts so I have to rename
> it).
>
> Otherwise execlists_check_remove_request when accessing
> head_req->ctx is use after free. And I can demonstrate that easily
> via gem-close-race. Put a
> WARN_ON(atomic_read(&head_req->ctx->ref.refcount) == 0); and see. :)
Oh, I don't have those racy accesses in my tree.
> What I think happens is that with two submission ports, we can get
> two context completions aggregated in an interrupt which comes after
> the seqno for both has been consumed by GEM and so LRCs unpinned.
>
> But with your persistent ctx hw id patches, I think the course is
> fine to do this including the complete elimination of the execlist
> retired queue.
>
> You can just drop the two chunks for the patch and I will follow up
> with two patches to finish it all off.
Or I could bring some more patches forward ;)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/2] drm/i915: Track the previous pinned context inside the request
2016-04-20 14:08 ` Tvrtko Ursulin
2016-04-20 14:18 ` Chris Wilson
@ 2016-04-20 14:22 ` Chris Wilson
2016-04-20 16:34 ` Tvrtko Ursulin
2016-04-20 14:24 ` [RFC 1/2] drm/i915: Store LRC hardware id in " Tvrtko Ursulin
2 siblings, 1 reply; 31+ messages in thread
From: Chris Wilson @ 2016-04-20 14:22 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Wed, Apr 20, 2016 at 03:08:19PM +0100, Tvrtko Ursulin wrote:
> Otherwise execlists_check_remove_request when accessing
> head_req->ctx is use after free. And I can demonstrate that easily
> via gem-close-race. Put a
> WARN_ON(atomic_read(&head_req->ctx->ref.refcount) == 0); and see. :)
More to the point, could we do a 10s burst of close race for BAT. What's
the likelihood of that capturing such faults?
With lockdep/kmemcheck etc enabled?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 31+ messages in thread
* [RFC 1/2] drm/i915: Store LRC hardware id in the request
2016-04-20 14:08 ` Tvrtko Ursulin
2016-04-20 14:18 ` Chris Wilson
2016-04-20 14:22 ` Chris Wilson
@ 2016-04-20 14:24 ` Tvrtko Ursulin
2016-04-20 14:24 ` [RFC 2/2] drm/i915: Stop tracking execlists retired requests Tvrtko Ursulin
2016-04-20 18:15 ` [RFC 1/2] drm/i915: Store LRC hardware id in the request Chris Wilson
2 siblings, 2 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2016-04-20 14:24 UTC (permalink / raw)
To: Intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
This way in the following patch we can disconnect requests
from contexts.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 2 ++
drivers/gpu/drm/i915/intel_lrc.c | 3 ++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index be98e9643072..c680dcdad828 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2349,6 +2349,8 @@ struct drm_i915_gem_request {
/** Execlists no. of times this request has been sent to the ELSP */
int elsp_submitted;
+ /** Execlists context hardware id. */
+ unsigned ctx_hw_id;
};
struct drm_i915_gem_request * __must_check
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 67c369ae649b..833d8fd3343f 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -477,7 +477,7 @@ execlists_check_remove_request(struct intel_engine_cs *engine, u32 request_id)
if (!head_req)
return 0;
- if (unlikely(head_req->ctx->hw_id != request_id))
+ if (unlikely(head_req->ctx_hw_id != request_id))
return 0;
WARN(head_req->elsp_submitted == 0, "Never submitted head request\n");
@@ -615,6 +615,7 @@ static void execlists_context_queue(struct drm_i915_gem_request *request)
}
list_add_tail(&request->execlist_link, &engine->execlist_queue);
+ request->ctx_hw_id = request->ctx->hw_id;
if (num_elements == 0)
execlists_context_unqueue(engine);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [RFC 2/2] drm/i915: Stop tracking execlists retired requests
2016-04-20 14:24 ` [RFC 1/2] drm/i915: Store LRC hardware id in " Tvrtko Ursulin
@ 2016-04-20 14:24 ` Tvrtko Ursulin
2016-04-20 18:15 ` [RFC 1/2] drm/i915: Store LRC hardware id in the request Chris Wilson
1 sibling, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2016-04-20 14:24 UTC (permalink / raw)
To: Intel-gfx; +Cc: Tvrtko Ursulin
From: Tvrtko Ursulin <tvrtko@ursulin.net>
With the previous patch having extended the pinned lifetime of
contexts by referencing the previous context from the current
request until the latter is retired (completed by the GPU),
we can now remove usage of execlist retired queue entirely.
This is because the above now guarantees that all execlist
object access requirements are satisfied by this new tracking,
and we can stop taking additional references and stop keeping
request on the execlists retired queue.
The latter was a source of significant scalability issues in
the driver causing performance hits on some tests. Most
dramatical of which was igt/gem_close_race which had run time
in tens of minutes which is now reduced to tens of seconds.
Signed-off-by: Tvrtko Ursulin <tvrtko@ursulin.net>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_gem.c | 10 +--------
drivers/gpu/drm/i915/intel_lrc.c | 39 ++++++++++++---------------------
drivers/gpu/drm/i915/intel_lrc.h | 2 +-
drivers/gpu/drm/i915/intel_ringbuffer.h | 1 -
4 files changed, 16 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dbfc38f91f7d..045d8369e24a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2876,13 +2876,7 @@ static void i915_gem_reset_engine_cleanup(struct drm_i915_private *dev_priv,
/* Ensure irq handler finishes or is cancelled. */
tasklet_kill(&engine->irq_tasklet);
- spin_lock_bh(&engine->execlist_lock);
- /* list_splice_tail_init checks for empty lists */
- list_splice_tail_init(&engine->execlist_queue,
- &engine->execlist_retired_req_list);
- spin_unlock_bh(&engine->execlist_lock);
-
- intel_execlists_retire_requests(engine);
+ intel_execlists_cancel_requests(engine);
}
/*
@@ -3006,8 +3000,6 @@ i915_gem_retire_requests(struct drm_device *dev)
spin_lock_bh(&engine->execlist_lock);
idle &= list_empty(&engine->execlist_queue);
spin_unlock_bh(&engine->execlist_lock);
-
- intel_execlists_retire_requests(engine);
}
}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 833d8fd3343f..37c557d3fb4a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -431,8 +431,8 @@ static void execlists_context_unqueue(struct intel_engine_cs *engine)
/* Same ctx: ignore first request, as second request
* will update tail past first request's workload */
cursor->elsp_submitted = req0->elsp_submitted;
- list_move_tail(&req0->execlist_link,
- &engine->execlist_retired_req_list);
+ list_del(&req0->execlist_link);
+ i915_gem_request_unreference(req0);
req0 = cursor;
} else {
req1 = cursor;
@@ -464,7 +464,7 @@ static void execlists_context_unqueue(struct intel_engine_cs *engine)
}
static unsigned int
-execlists_check_remove_request(struct intel_engine_cs *engine, u32 request_id)
+execlists_check_remove_request(struct intel_engine_cs *engine, u32 ctx_id)
{
struct drm_i915_gem_request *head_req;
@@ -474,19 +474,16 @@ execlists_check_remove_request(struct intel_engine_cs *engine, u32 request_id)
struct drm_i915_gem_request,
execlist_link);
- if (!head_req)
- return 0;
-
- if (unlikely(head_req->ctx_hw_id != request_id))
- return 0;
+ if (WARN_ON(!head_req || (head_req->ctx_hw_id != ctx_id)))
+ return 0;
WARN(head_req->elsp_submitted == 0, "Never submitted head request\n");
if (--head_req->elsp_submitted > 0)
return 0;
- list_move_tail(&head_req->execlist_link,
- &engine->execlist_retired_req_list);
+ list_del(&head_req->execlist_link);
+ i915_gem_request_unreference(head_req);
return 1;
}
@@ -590,9 +587,6 @@ static void execlists_context_queue(struct drm_i915_gem_request *request)
struct drm_i915_gem_request *cursor;
int num_elements = 0;
- intel_lr_context_pin(request->ctx, request->engine);
- i915_gem_request_reference(request);
-
spin_lock_bh(&engine->execlist_lock);
list_for_each_entry(cursor, &engine->execlist_queue, execlist_link)
@@ -609,11 +603,12 @@ static void execlists_context_queue(struct drm_i915_gem_request *request)
if (request->ctx == tail_req->ctx) {
WARN(tail_req->elsp_submitted != 0,
"More than 2 already-submitted reqs queued\n");
- list_move_tail(&tail_req->execlist_link,
- &engine->execlist_retired_req_list);
+ list_del(&tail_req->execlist_link);
+ i915_gem_request_unreference(tail_req);
}
}
+ i915_gem_request_reference(request);
list_add_tail(&request->execlist_link, &engine->execlist_queue);
request->ctx_hw_id = request->ctx->hw_id;
if (num_elements == 0)
@@ -1001,23 +996,18 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
return 0;
}
-void intel_execlists_retire_requests(struct intel_engine_cs *engine)
+void intel_execlists_cancel_requests(struct intel_engine_cs *engine)
{
struct drm_i915_gem_request *req, *tmp;
- struct list_head retired_list;
+ LIST_HEAD(cancel_list);
WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
- if (list_empty(&engine->execlist_retired_req_list))
- return;
- INIT_LIST_HEAD(&retired_list);
spin_lock_bh(&engine->execlist_lock);
- list_replace_init(&engine->execlist_retired_req_list, &retired_list);
+ list_replace_init(&engine->execlist_queue, &cancel_list);
spin_unlock_bh(&engine->execlist_lock);
- list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) {
- intel_lr_context_unpin(req->ctx, engine);
-
+ list_for_each_entry_safe(req, tmp, &cancel_list, execlist_link) {
list_del(&req->execlist_link);
i915_gem_request_unreference(req);
}
@@ -2109,7 +2099,6 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
INIT_LIST_HEAD(&engine->buffers);
INIT_LIST_HEAD(&engine->execlist_queue);
- INIT_LIST_HEAD(&engine->execlist_retired_req_list);
spin_lock_init(&engine->execlist_lock);
tasklet_init(&engine->irq_tasklet,
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 8bea937973f6..4b1c896b5019 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -119,6 +119,6 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
struct drm_i915_gem_execbuffer2 *args,
struct list_head *vmas);
-void intel_execlists_retire_requests(struct intel_engine_cs *engine);
+void intel_execlists_cancel_requests(struct intel_engine_cs *engine);
#endif /* _INTEL_LRC_H_ */
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 2ade194bbea9..527549dbeb3c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -269,7 +269,6 @@ struct intel_engine_cs {
struct tasklet_struct irq_tasklet;
spinlock_t execlist_lock; /* used inside tasklet, use spin_lock_bh */
struct list_head execlist_queue;
- struct list_head execlist_retired_req_list;
unsigned int fw_domains;
unsigned int next_context_status_buffer;
unsigned int idle_lite_restore_wa;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 2/2] drm/i915: Track the previous pinned context inside the request
2016-04-20 14:22 ` Chris Wilson
@ 2016-04-20 16:34 ` Tvrtko Ursulin
0 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2016-04-20 16:34 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 20/04/16 15:22, Chris Wilson wrote:
> On Wed, Apr 20, 2016 at 03:08:19PM +0100, Tvrtko Ursulin wrote:
>> Otherwise execlists_check_remove_request when accessing
>> head_req->ctx is use after free. And I can demonstrate that easily
>> via gem-close-race. Put a
>> WARN_ON(atomic_read(&head_req->ctx->ref.refcount) == 0); and see. :)
>
> More to the point, could we do a 10s burst of close race for BAT. What's
> the likelihood of that capturing such faults?
> With lockdep/kmemcheck etc enabled?
No idea, but on a lean kernel it takes a lot less than 10s. So maybe
time limited gem-close-race-basic, resend your series and see if it
catches it?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC 1/2] drm/i915: Store LRC hardware id in the request
2016-04-20 14:24 ` [RFC 1/2] drm/i915: Store LRC hardware id in " Tvrtko Ursulin
2016-04-20 14:24 ` [RFC 2/2] drm/i915: Stop tracking execlists retired requests Tvrtko Ursulin
@ 2016-04-20 18:15 ` Chris Wilson
1 sibling, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2016-04-20 18:15 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Wed, Apr 20, 2016 at 03:24:56PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> This way in the following patch we can disconnect requests
> from contexts.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 ++
> drivers/gpu/drm/i915/intel_lrc.c | 3 ++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index be98e9643072..c680dcdad828 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2349,6 +2349,8 @@ struct drm_i915_gem_request {
> /** Execlists no. of times this request has been sent to the ELSP */
> int elsp_submitted;
>
> + /** Execlists context hardware id. */
> + unsigned ctx_hw_id;
I'm cringing because this add yet another execlists specific variable
that we can avoid... However, I have to admit that it is a simpler step
toward the same goal.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2016-04-20 18:15 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-19 11:40 Premature unpinning Chris Wilson
2016-04-19 11:40 ` [PATCH v2 01/12] drm/i915: Mark the current context as lost on suspend Chris Wilson
2016-04-19 14:12 ` [PATCH v2] " Chris Wilson
2016-04-19 14:20 ` Mika Kuoppala
2016-04-19 11:40 ` [PATCH v2 02/12] drm/i915: L3 cache remapping is part of context switching Chris Wilson
2016-04-19 11:40 ` [PATCH v2 03/12] drm/i915: Consolidate L3 remapping LRI Chris Wilson
2016-04-19 11:40 ` [PATCH v2 04/12] drm/i915: Remove early l3-remap Chris Wilson
2016-04-19 11:40 ` [PATCH v2 05/12] drm/i915: Rearrange switch_context to load the aliasing ppgtt on first use Chris Wilson
2016-04-19 11:40 ` [PATCH v2 06/12] drm/i915: Assign every HW context a unique ID Chris Wilson
2016-04-19 11:40 ` [PATCH v2 07/12] drm/i915: Replace the pinned context address with its " Chris Wilson
2016-04-19 11:40 ` [PATCH v2 08/12] drm/i915: Refactor execlists default context pinning Chris Wilson
2016-04-19 12:25 ` Tvrtko Ursulin
2016-04-20 14:07 ` Mika Kuoppala
2016-04-19 11:40 ` [PATCH v2 09/12] drm/i915: Move context initialisation to first-use Chris Wilson
2016-04-19 11:40 ` [PATCH v2 10/12] drm/i915: Move the magical deferred context allocation into the request Chris Wilson
2016-04-19 12:40 ` kbuild test robot
2016-04-19 11:40 ` [PATCH v2 11/12] drm/i915: Track the previous pinned context inside " Chris Wilson
2016-04-19 11:40 ` [PATCH v2 12/12] drm/i915: Move releasing of the GEM request from free to retire/cancel Chris Wilson
2016-04-19 12:59 ` Update to patches 11 and 12 Chris Wilson
2016-04-19 12:59 ` [PATCH 1/2] drm/i915: Move releasing of the GEM request from free to retire/cancel Chris Wilson
2016-04-20 13:55 ` Tvrtko Ursulin
2016-04-19 12:59 ` [PATCH 2/2] drm/i915: Track the previous pinned context inside the request Chris Wilson
2016-04-20 14:08 ` Tvrtko Ursulin
2016-04-20 14:18 ` Chris Wilson
2016-04-20 14:22 ` Chris Wilson
2016-04-20 16:34 ` Tvrtko Ursulin
2016-04-20 14:24 ` [RFC 1/2] drm/i915: Store LRC hardware id in " Tvrtko Ursulin
2016-04-20 14:24 ` [RFC 2/2] drm/i915: Stop tracking execlists retired requests Tvrtko Ursulin
2016-04-20 18:15 ` [RFC 1/2] drm/i915: Store LRC hardware id in the request Chris Wilson
2016-04-19 13:24 ` ✗ Fi.CI.BAT: failure for series starting with [v2,01/12] drm/i915: Mark the current context as lost on suspend Patchwork
2016-04-19 14:58 ` ✗ Fi.CI.BAT: failure for series starting with [v2] drm/i915: Mark the current context as lost on suspend (rev2) Patchwork
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.