diff options
author | Ben Widawsky <benjamin.widawsky@intel.com> | 2014-01-01 13:15:13 -0500 |
---|---|---|
committer | Daniel Vetter <daniel.vetter@ffwll.ch> | 2014-01-22 04:34:37 -0500 |
commit | 1d62beeaebf109e1b2b18f1b24ab3279f31aa649 (patch) | |
tree | 4eb87ae27ca89c0d2a4223c9dbf5cf93dbaa9211 /drivers/gpu | |
parent | 1fb2362b43371f35b98a55e615d990f96d8ac966 (diff) |
drm/i915/ppgtt: Defer request freeing on reset
We need to defer the free request until the object/vma is capable of
being freed - or else we have a problem when we try to destroy the
context.
The exact same issue is described and fixed here:
commit e20780439b26ba95aeb29d3e27cd8cc32bc82a4c
Author: Ben Widawsky <ben@bwidawsk.net>
Date: Fri Dec 6 14:11:22 2013 -0800
drm/i915: Defer request freeing
I had this fix previously, but decided not to keep it for some reason I
can no longer remember.
gem_reset_stats is a really good test at hitting the problem.
For the inquisitive:
[ 170.516392] ------------[ cut here ]------------
[ 170.517227] WARNING: CPU: 1 PID: 105 at drivers/gpu/drm/drm_mm.c:578 drm_mm_takedown+0x2e/0x30 [drm]()
[ 170.518064] Memory manager not clean during takedown.
[ 170.518941] CPU: 1 PID: 105 Comm: kworker/1:1 Not tainted 3.13.0-rc4-BEN+ #28
[ 170.519787] Hardware name: Hewlett-Packard HP EliteBook 8470p/179B, BIOS 68ICF Ver. F.02 04/27/2012
[ 170.520662] Call Trace:
[ 170.521517] [<ffffffff814f0589>] dump_stack+0x4e/0x7a
[ 170.522373] [<ffffffff81049e6d>] warn_slowpath_common+0x7d/0xa0
[ 170.523227] [<ffffffff81049edc>] warn_slowpath_fmt+0x4c/0x50
[ 170.524079] [<ffffffffa06c414e>] drm_mm_takedown+0x2e/0x30 [drm]
[ 170.524934] [<ffffffffa07213f3>] gen6_ppgtt_cleanup+0x23/0x110
[i915]
[ 170.525777] [<ffffffffa07837ed>] ppgtt_release.part.5+0x24/0x29
[i915]
[ 170.526603] [<ffffffffa071aaa5>] i915_gem_context_free+0x195/0x1a0
[i915]
[ 170.527423] [<ffffffffa071189d>] i915_gem_free_request+0x9d/0xb0
[i915]
[ 170.528247] [<ffffffffa0718af9>] i915_gem_reset+0x1f9/0x3f0 [i915]
[ 170.529065] [<ffffffffa0700cce>] i915_reset+0x4e/0x180 [i915]
[ 170.529870] [<ffffffffa070829d>] i915_error_work_func+0xcd/0x120
[i915]
[ 170.530666] [<ffffffff8106c13a>] process_one_work+0x1fa/0x6d0
[ 170.531453] [<ffffffff8106c0d8>] ? process_one_work+0x198/0x6d0
[ 170.532230] [<ffffffff8106c72b>] worker_thread+0x11b/0x3a0
[ 170.532996] [<ffffffff8106c610>] ? process_one_work+0x6d0/0x6d0
[ 170.533771] [<ffffffff810743ef>] kthread+0xff/0x120
[ 170.534548] [<ffffffff810742f0>] ? insert_kthread_work+0x80/0x80
[ 170.535322] [<ffffffff814f97ac>] ret_from_fork+0x7c/0xb0
[ 170.536089] [<ffffffff810742f0>] ? insert_kthread_work+0x80/0x80
[ 170.536847] ---[ end trace 3d4c12892e42d58f ]---
v2: Whitespace fix. (Chris)
Note: This is a bug that only hits the ppgtt topic branch but I've
figured that doing the request cleanup in this order is generally the
right thing to do.
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
[danvet: Add a code comment to clarify what's actually going on since
the lifetime rules aroung ppgtt cleanup are ... fuzzy a best atm. Also
add a note about why we need this.]
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Diffstat (limited to 'drivers/gpu')
-rw-r--r-- | drivers/gpu/drm/i915/i915_gem.c | 27 |
1 files changed, 17 insertions, 10 deletions
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 51d4c1a0f544..4461336205ed 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c | |||
@@ -2388,16 +2388,6 @@ static void i915_gem_reset_ring_status(struct drm_i915_private *dev_priv, | |||
2388 | static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, | 2388 | static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, |
2389 | struct intel_ring_buffer *ring) | 2389 | struct intel_ring_buffer *ring) |
2390 | { | 2390 | { |
2391 | while (!list_empty(&ring->request_list)) { | ||
2392 | struct drm_i915_gem_request *request; | ||
2393 | |||
2394 | request = list_first_entry(&ring->request_list, | ||
2395 | struct drm_i915_gem_request, | ||
2396 | list); | ||
2397 | |||
2398 | i915_gem_free_request(request); | ||
2399 | } | ||
2400 | |||
2401 | while (!list_empty(&ring->active_list)) { | 2391 | while (!list_empty(&ring->active_list)) { |
2402 | struct drm_i915_gem_object *obj; | 2392 | struct drm_i915_gem_object *obj; |
2403 | 2393 | ||
@@ -2407,6 +2397,23 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, | |||
2407 | 2397 | ||
2408 | i915_gem_object_move_to_inactive(obj); | 2398 | i915_gem_object_move_to_inactive(obj); |
2409 | } | 2399 | } |
2400 | |||
2401 | /* | ||
2402 | * We must free the requests after all the corresponding objects have | ||
2403 | * been moved off active lists. Which is the same order as the normal | ||
2404 | * retire_requests function does. This is important if object hold | ||
2405 | * implicit references on things like e.g. ppgtt address spaces through | ||
2406 | * the request. | ||
2407 | */ | ||
2408 | while (!list_empty(&ring->request_list)) { | ||
2409 | struct drm_i915_gem_request *request; | ||
2410 | |||
2411 | request = list_first_entry(&ring->request_list, | ||
2412 | struct drm_i915_gem_request, | ||
2413 | list); | ||
2414 | |||
2415 | i915_gem_free_request(request); | ||
2416 | } | ||
2410 | } | 2417 | } |
2411 | 2418 | ||
2412 | void i915_gem_restore_fences(struct drm_device *dev) | 2419 | void i915_gem_restore_fences(struct drm_device *dev) |