[Intel-gfx] [PATCH 4/4] drm/i915: Late request cancellations are harmful
Chris Wilson
chris at chris-wilson.co.uk
Sat Apr 9 09:27:39 UTC 2016
Conceptually, each request is a record of a hardware transaction - we
build up a list of pending commands and then either commit them to
hardware, or cancel them. However, whilst building up the list of
pending commands, we may modify state outside of the request and make
references to the pending request. If we do so and then cancel that
request, external objects then point to the deleted request leading to
both graphical and memory corruption.
The easiest example is to consider object/VMA tracking. When we mark an
object as active in a request, we store a pointer to this, the most
recent request, in the object. Then we want to free that object, we wait
for the most recent request to be idle before proceeding (otherwise the
hardware will write to pages now owned by the system, or we will attempt
to read from those pages before the hardware is finished writing). If
the request was cancelled instead, that wait completes immediately. As a
result, all requests must be committed and not cancelled if the external
state is unknown.
All that remains of i915_gem_request_cancel() users are just a couple of
extremely unlikely allocation failures, so remove the API entirely.
A consequence of committing all incomplete requests is that we generate
excess breadcrumbs and fill the ring much more often with dummy work. We
have completely undone the outstanding_last_seqno optimisation.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93907
Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
Cc: stable at vger.kernel.org
---
drivers/gpu/drm/i915/i915_drv.h | 2 --
drivers/gpu/drm/i915/i915_gem.c | 50 ++++++++++++------------------
drivers/gpu/drm/i915/i915_gem_context.c | 21 ++++++-------
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 15 +++------
drivers/gpu/drm/i915/intel_display.c | 2 +-
drivers/gpu/drm/i915/intel_lrc.c | 4 +--
drivers/gpu/drm/i915/intel_overlay.c | 8 ++---
7 files changed, 39 insertions(+), 63 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a93e5dd4fa9a..f374db8de673 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2320,7 +2320,6 @@ struct drm_i915_gem_request {
struct drm_i915_gem_request * __must_check
i915_gem_request_alloc(struct intel_engine_cs *engine,
struct intel_context *ctx);
-void i915_gem_request_cancel(struct drm_i915_gem_request *req);
void i915_gem_request_free(struct kref *req_ref);
int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
struct drm_file *file);
@@ -2872,7 +2871,6 @@ int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
void i915_gem_execbuffer_move_to_active(struct list_head *vmas,
struct drm_i915_gem_request *req);
-void i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params);
int i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
struct drm_i915_gem_execbuffer2 *args,
struct list_head *vmas);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1c3ff56594d6..42227495803f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2753,7 +2753,8 @@ __i915_gem_request_alloc(struct intel_engine_cs *engine,
* fully prepared. Thus it can be cleaned up using the proper
* free code.
*/
- i915_gem_request_cancel(req);
+ intel_ring_reserved_space_cancel(req->ringbuf);
+ i915_gem_request_unreference(req);
return ret;
}
@@ -2790,13 +2791,6 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
return err ? ERR_PTR(err) : req;
}
-void i915_gem_request_cancel(struct drm_i915_gem_request *req)
-{
- intel_ring_reserved_space_cancel(req->ringbuf);
-
- i915_gem_request_unreference(req);
-}
-
struct drm_i915_gem_request *
i915_gem_find_active_request(struct intel_engine_cs *engine)
{
@@ -3410,12 +3404,9 @@ int i915_gpu_idle(struct drm_device *dev)
return PTR_ERR(req);
ret = i915_switch_context(req);
- if (ret) {
- i915_gem_request_cancel(req);
- return ret;
- }
-
i915_add_request_no_flush(req);
+ if (ret)
+ return ret;
}
ret = intel_engine_idle(engine);
@@ -4917,34 +4908,33 @@ i915_gem_init_hw(struct drm_device *dev)
req = i915_gem_request_alloc(engine, NULL);
if (IS_ERR(req)) {
ret = PTR_ERR(req);
- i915_gem_cleanup_engines(dev);
- goto out;
+ break;
}
if (engine->id == RCS) {
- for (j = 0; j < NUM_L3_SLICES(dev); j++)
- i915_gem_l3_remap(req, j);
+ 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 && ret != -EIO) {
- DRM_ERROR("PPGTT enable %s failed %d\n",
- engine->name, ret);
- i915_gem_request_cancel(req);
- i915_gem_cleanup_engines(dev);
- goto out;
- }
+ if (ret)
+ goto err_request;
ret = i915_gem_context_enable(req);
- if (ret && ret != -EIO) {
- DRM_ERROR("Context enable %s failed %d\n",
+ 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_request_cancel(req);
i915_gem_cleanup_engines(dev);
- goto out;
+ break;
}
-
- i915_add_request_no_flush(req);
}
out:
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index e5ad7b21e356..583b373d41ab 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -668,7 +668,6 @@ static int do_switch(struct drm_i915_gem_request *req)
struct drm_i915_private *dev_priv = req->i915;
struct intel_context *from = engine->last_context;
u32 hw_flags = 0;
- bool uninitialized = false;
int ret, i;
if (from != NULL && engine == &dev_priv->engine[RCS]) {
@@ -776,6 +775,15 @@ static int do_switch(struct drm_i915_gem_request *req)
to->remap_slice &= ~(1<<i);
}
+ if (!to->legacy_hw_ctx.initialized) {
+ if (engine->init_context) {
+ ret = engine->init_context(req);
+ if (ret)
+ goto unpin_out;
+ }
+ to->legacy_hw_ctx.initialized = true;
+ }
+
/* The backing object for the context is done after switching to the
* *next* context. Therefore we cannot retire the previous context until
* the next context has already started running. In fact, the below code
@@ -799,21 +807,10 @@ static int do_switch(struct drm_i915_gem_request *req)
i915_gem_context_unreference(from);
}
- uninitialized = !to->legacy_hw_ctx.initialized;
- to->legacy_hw_ctx.initialized = true;
-
done:
i915_gem_context_reference(to);
engine->last_context = to;
- if (uninitialized) {
- if (engine->init_context) {
- ret = engine->init_context(req);
- if (ret)
- DRM_ERROR("ring init context: %d\n", ret);
- }
- }
-
return 0;
unpin_out:
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0ee61fd014df..b733f7bdafca 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1137,7 +1137,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
}
}
-void
+static void
i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params)
{
/* Unconditionally force add_request to emit a full flush. */
@@ -1322,7 +1322,6 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
i915_gem_execbuffer_move_to_active(vmas, params->request);
- i915_gem_execbuffer_retire_commands(params);
return 0;
}
@@ -1624,7 +1623,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
ret = i915_gem_request_add_to_client(req, file);
if (ret)
- goto err_batch_unpin;
+ goto err_request;
/*
* Save assorted stuff away to pass through to *_submission().
@@ -1641,6 +1640,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
params->request = req;
ret = dev_priv->gt.execbuf_submit(params, args, &eb->vmas);
+err_request:
+ i915_gem_execbuffer_retire_commands(params);
err_batch_unpin:
/*
@@ -1657,14 +1658,6 @@ err:
i915_gem_context_unreference(ctx);
eb_destroy(eb);
- /*
- * If the request was created but not successfully submitted then it
- * must be freed again. If it was submitted then it is being tracked
- * on the active request list and no clean up is required here.
- */
- if (ret && !IS_ERR_OR_NULL(req))
- i915_gem_request_cancel(req);
-
mutex_unlock(&dev->struct_mutex);
pre_mutex_err:
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index feb7028341b8..e3637a12126c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11654,7 +11654,7 @@ cleanup_unpin:
intel_unpin_fb_obj(fb, crtc->primary->state->rotation);
cleanup_pending:
if (!IS_ERR_OR_NULL(request))
- i915_gem_request_cancel(request);
+ i915_add_request_no_flush(request);
atomic_dec(&intel_crtc->unpin_work_count);
mutex_unlock(&dev->struct_mutex);
cleanup:
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index a1db6a02cf23..8784f0769de3 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1016,7 +1016,6 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
i915_gem_execbuffer_move_to_active(vmas, params->request);
- i915_gem_execbuffer_retire_commands(params);
return 0;
}
@@ -2657,13 +2656,12 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
}
ret = engine->init_context(req);
+ i915_add_request_no_flush(req);
if (ret) {
DRM_ERROR("ring init context: %d\n",
ret);
- i915_gem_request_cancel(req);
goto error_ringbuf;
}
- i915_add_request_no_flush(req);
}
return 0;
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 6694e9230cd5..bcc3b6a016d8 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -247,7 +247,7 @@ static int intel_overlay_on(struct intel_overlay *overlay)
ret = intel_ring_begin(req, 4);
if (ret) {
- i915_gem_request_cancel(req);
+ i915_add_request_no_flush(req);
return ret;
}
@@ -290,7 +290,7 @@ static int intel_overlay_continue(struct intel_overlay *overlay,
ret = intel_ring_begin(req, 2);
if (ret) {
- i915_gem_request_cancel(req);
+ i915_add_request_no_flush(req);
return ret;
}
@@ -356,7 +356,7 @@ static int intel_overlay_off(struct intel_overlay *overlay)
ret = intel_ring_begin(req, 6);
if (ret) {
- i915_gem_request_cancel(req);
+ i915_add_request_no_flush(req);
return ret;
}
@@ -431,7 +431,7 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
ret = intel_ring_begin(req, 2);
if (ret) {
- i915_gem_request_cancel(req);
+ i915_add_request_no_flush(req);
return ret;
}
--
2.8.0.rc3
More information about the Intel-gfx
mailing list