* [PATCH v3] drm/i915: Fix recursive calls to unmap
@ 2011-10-31 3:16 Ben Widawsky
2011-10-31 7:45 ` Daniel Vetter
2011-11-02 19:29 ` Dave Airlie
0 siblings, 2 replies; 7+ messages in thread
From: Ben Widawsky @ 2011-10-31 3:16 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky
After the ILK vt-d workaround patches it became clear that we had
introduced a bug. Chris tracked down the issue to recursive calls to
unmap. This happens because we try to optimize waiting on requests by
calling retire requests after the wait, which may drop the last
reference on an object and end up freeing the object (and then unmap the
object from the gtt).
The solution here is to add a new flag to the call chain which gives the
routines the information they need to possibly defer actions which may
cause us to recurse. A macro has been defined to replace i915_gpu_idle
which defaults to the old behavior.
Kudos to Chris for tracking this one down.
This fixes tests/gem_linear_blits in intel-gpu-tools.
v2: v1 used a global, v2 directly modified the call chain.
v3: As Keith pointed out, v2 changed retirement behavior. While it was
intentional, it wasn't related to the bugfix, and so has been removed.
To do this, changed i915_wait_request to be a macro to assure old
behavior is kept.
Cc: Keith Packard <keithp@keithp.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reported-by: guang.a.yang@intel.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42180
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/i915_drv.h | 9 ++++++---
drivers/gpu/drm/i915/i915_gem.c | 17 +++++++++--------
drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
3 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 06a37f4..fc3e00c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1180,13 +1180,16 @@ void i915_gem_do_init(struct drm_device *dev,
unsigned long start,
unsigned long mappable_end,
unsigned long end);
-int __must_check i915_gpu_idle(struct drm_device *dev);
+int __must_check i915_gem_gpu_idle(struct drm_device *dev, bool strictly_idle);
+#define i915_gpu_idle(dev) i915_gem_gpu_idle(dev, false)
int __must_check i915_gem_idle(struct drm_device *dev);
int __must_check i915_add_request(struct intel_ring_buffer *ring,
struct drm_file *file,
struct drm_i915_gem_request *request);
-int __must_check i915_wait_request(struct intel_ring_buffer *ring,
- uint32_t seqno);
+int __must_check i915_gem_wait_request(struct intel_ring_buffer *ring,
+ uint32_t seqno,
+ bool defer_retirement);
+#define i915_wait_request(ring, seqno) i915_gem_wait_request(ring, seqno, false)
int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
int __must_check
i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6651c36..3ea017a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1942,8 +1942,9 @@ i915_gem_retire_work_handler(struct work_struct *work)
* request and object lists appropriately for that event.
*/
int
-i915_wait_request(struct intel_ring_buffer *ring,
- uint32_t seqno)
+i915_gem_wait_request(struct intel_ring_buffer *ring,
+ uint32_t seqno,
+ bool defer_retirement)
{
drm_i915_private_t *dev_priv = ring->dev->dev_private;
u32 ier;
@@ -2027,7 +2028,7 @@ i915_wait_request(struct intel_ring_buffer *ring,
* buffer to have made it to the inactive list, and we would need
* a separate wait queue to handle that.
*/
- if (ret == 0)
+ if (ret == 0 && !defer_retirement)
i915_gem_retire_requests_ring(ring);
return ret;
@@ -2172,7 +2173,7 @@ i915_gem_flush_ring(struct intel_ring_buffer *ring,
return 0;
}
-static int i915_ring_idle(struct intel_ring_buffer *ring)
+static int i915_ring_idle(struct intel_ring_buffer *ring, bool defer_retirement)
{
int ret;
@@ -2186,18 +2187,18 @@ static int i915_ring_idle(struct intel_ring_buffer *ring)
return ret;
}
- return i915_wait_request(ring, i915_gem_next_request_seqno(ring));
+ return i915_gem_wait_request(ring, i915_gem_next_request_seqno(ring),
+ defer_retirement);
}
-int
-i915_gpu_idle(struct drm_device *dev)
+int i915_gem_gpu_idle(struct drm_device *dev, bool strictly_idle)
{
drm_i915_private_t *dev_priv = dev->dev_private;
int ret, i;
/* Flush everything onto the inactive list. */
for (i = 0; i < I915_NUM_RINGS; i++) {
- ret = i915_ring_idle(&dev_priv->ring[i]);
+ ret = i915_ring_idle(&dev_priv->ring[i], strictly_idle);
if (ret)
return ret;
}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 6042c5e..b90b547 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -55,7 +55,7 @@ static bool do_idling(struct drm_i915_private *dev_priv)
if (unlikely(dev_priv->mm.gtt->do_idle_maps)) {
dev_priv->mm.interruptible = false;
- if (i915_gpu_idle(dev_priv->dev)) {
+ if (i915_gem_gpu_idle(dev_priv->dev, true)) {
DRM_ERROR("Couldn't idle GPU\n");
/* Wait a bit, in hopes it avoids the hang */
udelay(10);
--
1.7.7.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] drm/i915: Fix recursive calls to unmap
2011-10-31 3:16 [PATCH v3] drm/i915: Fix recursive calls to unmap Ben Widawsky
@ 2011-10-31 7:45 ` Daniel Vetter
2011-11-02 19:29 ` Dave Airlie
1 sibling, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2011-10-31 7:45 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx
On Sun, Oct 30, 2011 at 08:16:45PM -0700, Ben Widawsky wrote:
[snip]
> -int
> -i915_gpu_idle(struct drm_device *dev)
> +int i915_gem_gpu_idle(struct drm_device *dev, bool strictly_idle)
> {
> drm_i915_private_t *dev_priv = dev->dev_private;
> int ret, i;
>
> /* Flush everything onto the inactive list. */
> for (i = 0; i < I915_NUM_RINGS; i++) {
> - ret = i915_ring_idle(&dev_priv->ring[i]);
> + ret = i915_ring_idle(&dev_priv->ring[i], strictly_idle);
One bikeshed: When introducing a new parametere all over the tree, I
prefere consistent naming for it, i.e. s/strictly_idle/defer_retirement/
...
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] drm/i915: Fix recursive calls to unmap
2011-10-31 3:16 [PATCH v3] drm/i915: Fix recursive calls to unmap Ben Widawsky
2011-10-31 7:45 ` Daniel Vetter
@ 2011-11-02 19:29 ` Dave Airlie
2011-11-03 4:47 ` Ben Widawsky
1 sibling, 1 reply; 7+ messages in thread
From: Dave Airlie @ 2011-11-02 19:29 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx
On Mon, Oct 31, 2011 at 3:16 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> After the ILK vt-d workaround patches it became clear that we had
> introduced a bug. Chris tracked down the issue to recursive calls to
> unmap. This happens because we try to optimize waiting on requests by
> calling retire requests after the wait, which may drop the last
> reference on an object and end up freeing the object (and then unmap the
> object from the gtt).
>
> The solution here is to add a new flag to the call chain which gives the
> routines the information they need to possibly defer actions which may
> cause us to recurse. A macro has been defined to replace i915_gpu_idle
> which defaults to the old behavior.
>
> Kudos to Chris for tracking this one down.
So this fixes the non-VTd case, the VT-d case still hits a recursion
here, for posterity its below.
Dave.
BUG: unable to handle kernel paging request at 959cba80
IP: [<c1032653>] cpuacct_charge+0x4e/0x7a
*pdpt = 0000000033426001 *pde = 0000000000000000
Thread overran stack, or stack corrupted
Oops: 0000 [#1] SMP
last sysfs file:
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:00/power_supply/BAT0/charge_full
Modules linked in: netconsole aes_i586 aes_generic fuse ipt_REJECT
nf_conntrack_ipv4 nf_defrag_ipv4 iptable_filter ip_tables ip6t_REJECT
nf_conntrack_ipv6 nf_defrag_ipv6 xt_state nf_conntrack ip6table_filter
ip6_tables ipv6 ext3 jbd dm_mirror dm_region_hash dm_log uinput
rtc_cmos tpm_infineon tpm_tis tpm tpm_bios ac battery sg joydev
serio_raw arc4 intel_ips ecb iTCO_wdt iTCO_vendor_support iwlagn
iwlcore mac80211 cfg80211 snd_hda_codec_hdmi snd_hda_codec_idt
snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm
snd_timer snd soundcore snd_page_alloc e1000e ext4 mbcache jbd2 crc16
sr_mod cdrom sd_mod crc_t10dif firewire_ohci firewire_core crc_itu_t
ahci libata usb_storage scsi_mod i915 drm_kms_helper drm i2c_algo_bit
button i2c_core video output [last unloaded: scsi_wait_scan]
Pid: 3129, comm: Xorg Not tainted 2.6.32-dael6 #23 Hewlett-Packard HP
EliteBook 2540p/7008
EIP: 0060:[<c1032653>] EFLAGS: 00213086 CPU: 2
EIP is at cpuacct_charge+0x4e/0x7a
EAX: c15905b8 EBX: 0122af07 ECX: f512a02c EDX: f743bcc0
ESI: 00000000 EDI: f512a000 EBP: f344e014 ESP: f344e004
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process Xorg (pid: 3129, ti=f344d000 task=f512a000 task.ti=f344e000)
Stack:
f512a02c 0122af07 00000000 f512a02c f344e058 c1032b38 3ae624ba 00000001
<0> f344e030 0000001d 00000002 f344e070 3aaacaad 0122af07 00000000 3ae624ba
<0> 00000001 f512a000 c5689ef4 f512a02c 00000002 f344e070 c103376e 00000000
Call Trace:
[<c1032b38>] update_curr+0x1a7/0x1eb
[<c103376e>] task_tick_fair+0x21/0xfd
[<c103bf09>] scheduler_tick+0xa3/0x1fa
[<c104bf78>] update_process_times+0x37/0x43
[<c1063b22>] tick_sched_timer+0x6d/0x9b
[<c105a704>] __run_hrtimer+0xb7/0x106
[<c1063ab5>] ? tick_sched_timer+0x0/0x9b
[<c105a940>] hrtimer_interrupt+0xde/0x1f3
[<c1021340>] smp_apic_timer_interrupt+0x69/0x7c
[<c13117e7>] apic_timer_interrupt+0x2f/0x34
[<c106007b>] ? second_overflow+0xd9/0x14a
[<c1310ea8>] ? _spin_unlock_irqrestore+0x40/0x48
[<c1192dd1>] __iommu_flush_iotlb+0x17d/0x198
[<c1193b20>] iommu_flush_iotlb_psi+0x7f/0xae
[<c1193db5>] intel_unmap_sg+0xc3/0xe1
[<c1193cf2>] ? intel_unmap_sg+0x0/0xe1
[<c11fbe44>] intel_gtt_unmap_memory+0x34/0x4f
[<c11fabb0>] ? intel_gtt_clear_range+0x26/0x42
[<f95e719f>] i915_gem_gtt_unbind_object+0x7f/0xc5 [i915]
[<f95e3b30>] i915_gem_object_unbind+0xac/0x119 [i915]
[<f95e3bb5>] i915_gem_free_object_tail+0x18/0xcb [i915]
[<f95e3ca0>] i915_gem_free_object+0x38/0x3c [i915]
[<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
[<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
[<c1175baf>] kref_put+0x39/0x42
[<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
[<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
[<f95e22cc>] i915_gem_wait_request+0x488/0x492 [i915]
[<c117dcab>] ? __sg_free_table+0x47/0x5e
[<f95e2352>] i915_gem_gpu_idle+0x7c/0x95 [i915]
[<f95e71be>] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915]
[<f95e3b30>] i915_gem_object_unbind+0xac/0x119 [i915]
[<f95e3bb5>] i915_gem_free_object_tail+0x18/0xcb [i915]
[<f95e3ca0>] i915_gem_free_object+0x38/0x3c [i915]
[<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
[<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
[<c1175baf>] kref_put+0x39/0x42
[<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
[<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
[<f95e22cc>] i915_gem_wait_request+0x488/0x492 [i915]
[<c117dcab>] ? __sg_free_table+0x47/0x5e
[<c1057751>] ? autoremove_wake_function+0x0/0x2f
[<f95e2352>] i915_gem_gpu_idle+0x7c/0x95 [i915]
[<f95e71be>] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915]
[<f95e3b30>] i915_gem_object_unbind+0xac/0x119 [i915]
[<f95e3bb5>] i915_gem_free_object_tail+0x18/0xcb [i915]
[<f95e3ca0>] i915_gem_free_object+0x38/0x3c [i915]
[<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
[<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
[<c1175baf>] kref_put+0x39/0x42
[<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
[<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
[<f95e22cc>] i915_gem_wait_request+0x488/0x492 [i915]
[<c117dcab>] ? __sg_free_table+0x47/0x5e
[<c1057751>] ? autoremove_wake_function+0x0/0x2f
[<f95e2352>] i915_gem_gpu_idle+0x7c/0x95 [i915]
[<f95e71be>] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915]
[<f95e3b30>] i915_gem_object_unbind+0xac/0x119 [i915]
[<f95e3bb5>] i915_gem_free_object_tail+0x18/0xcb [i915]
[<f95e3ca0>] i915_gem_free_object+0x38/0x3c [i915]
[<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
[<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
[<c1175baf>] kref_put+0x39/0x42
[<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
[<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
[<f95e22cc>] i915_gem_wait_request+0x488/0x492 [i915]
[<c117dcab>] ? __sg_free_table+0x47/0x5e
[<c1057751>] ? autoremove_wake_function+0x0/0x2f
[<f95e2352>] i915_gem_gpu_idle+0x7c/0x95 [i915]
[<f95e71be>] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915]
[<f95e3b30>] i915_gem_object_unbind+0xac/0x119 [i915]
[<f95e3bb5>] i915_gem_free_object_tail+0x18/0xcb [i915]
[<f95e3ca0>] i915_gem_free_object+0x38/0x3c [i915]
[<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
[<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
[<c1175baf>] kref_put+0x39/0x42
[<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
[<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
[<f95e22cc>] i915_gem_wait_request+0x488/0x492 [i915]
[<c117dcab>] ? __sg_free_table+0x47/0x5e
[<c1057751>] ? autoremove_wake_function+0x0/0x2f
[<f95e2352>] i915_gem_gpu_idle+0x7c/0x95 [i915]
[<f95e71be>] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915]
[<f95e3b30>] i915_gem_object_unbind+0xac/0x119 [i915]
[<f95e3bb5>] i915_gem_free_object_tail+0x18/0xcb [i915]
[<f95e3ca0>] i915_gem_free_object+0x38/0x3c [i915]
[<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
[<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
[<c1175baf>] kref_put+0x39/0x42
[<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
[<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
[<f95e22cc>] i915_gem_wait_request+0x488/0x492 [i915]
[<c117dcab>] ? __sg_free_table+0x47/0x5e
[<c1057751>] ? autoremove_wake_function+0x0/0x2f
[<f95e2352>] i915_gem_gpu_idle+0x7c/0x95 [i915]
[<f95e71be>] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915]
[<f95e3b30>] i915_gem_object_unbind+0xac/0x119 [i915]
[<f95e3bb5>] i915_gem_free_object_tail+0x18/0xcb [i915]
[<f95e3ca0>] i915_gem_free_object+0x38/0x3c [i915]
[<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
[<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
[<c1175baf>] kref_put+0x39/0x42
[<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
[<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
[<f95e22cc>] i915_gem_wait_request+0x488/0x492 [i915]
[<c117dcab>] ? __sg_free_table+0x47/0x5e
[<c1057751>] ? autoremove_wake_function+0x0/0x2f
[<f95e2352>] i915_gem_gpu_idle+0x7c/0x95 [i915]
[<f95e71be>] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915]
[<f95e3b30>] i915_gem_object_unbind+0xac/0x119 [i915]
[<f95e3bb5>] i915_gem_free_object_tail+0x18/0xcb [i915]
[<f95e3ca0>] i915_gem_free_object+0x38/0x3c [i915]
[<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
[<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
[<c1175baf>] kref_put+0x39/0x42
[<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
[<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
[<f95e22cc>] i915_gem_wait_request+0x488/0x492 [i915]
[<c117dcab>] ? __sg_free_table+0x47/0x5e
[<f95e2352>] i915_gem_gpu_idle+0x7c/0x95 [i915]
[<f95e71be>] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915]
[<f95e3b30>] i915_gem_object_unbind+0xac/0x119 [i915]
[<f95e3bb5>] i915_gem_free_object_tail+0x18/0xcb [i915]
[<f95e3ca0>] i915_gem_free_object+0x38/0x3c [i915]
[<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
[<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
[<c1175baf>] kref_put+0x39/0x42
[<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
[<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
[<f95e22cc>] i915_gem_wait_request+0x488/0x492 [i915]
[<c117dcab>] ? __sg_free_table+0x47/0x5e
[<c1057751>] ? autoremove_wake_function+0x0/0x2f
[<f95e2352>] i915_gem_gpu_idle+0x7c/0x95 [i915]
[<f95e71be>] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915]
[<f95e3b30>] i915_gem_object_unbind+0xac/0x119 [i915]
[<f95e3bb5>] i915_gem_free_object_tail+0x18/0xcb [i915]
[<f95e3ca0>] i915_gem_free_object+0x38/0x3c [i915]
[<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
[<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
[<c1175baf>] kref_put+0x39/0x42
[<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
[<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
[<f95e22cc>] i915_gem_wait_request+0x488/0x492 [i915]
[<c117dcab>] ? __sg_free_table+0x47/0x5e
[<c1057751>] ? autoremove_wake_function+0x0/0x2f
[<f95e2352>] i915_gem_gpu_idle+0x7c/0x95 [i915]
[<f95e71be>] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915]
[<f95e3b30>] i915_gem_object_unbind+0xac/0x119 [i915]
[<f95e3bb5>] i915_gem_free_object_tail+0x18/0xcb [i915]
[<f95e3ca0>] i915_gem_free_object+0x38/0x3c [i915]
[<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
[<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
[<c1175baf>] kref_put+0x39/0x42
[<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
[<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
[<f95e22cc>] i915_gem_wait_request+0x488/0x492 [i915]
[<c117dcab>] ? __sg_free_table+0x47/0x5e
[<c1057751>] ? autoremove_wake_function+0x0/0x2f
[<f95e2352>] i915_gem_gpu_idle+0x7c/0x95 [i915]
[<f95e71be>] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915]
[<f95e3b30>] i915_gem_object_unbind+0xac/0x119 [i915]
[<f95e3bb5>] i915_gem_free_object_tail+0x18/0xcb [i915]
[<f95e3ca0>] i915_gem_free_object+0x38/0x3c [i915]
[<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
[<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
[<c1175baf>] kref_put+0x39/0x42
[<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
[<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
[<f95e22cc>] i915_gem_wait_request+0x488/0x492 [i915]
[<c117dcab>] ? __sg_free_table+0x47/0x5e
[<c1057751>] ? autoremove_wake_function+0x0/0x2f
[<f95e2352>] i915_gem_gpu_idle+0x7c/0x95 [i915]
[<f95e71be>] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915]
[<f95e3b30>] i915_gem_object_unbind+0xac/0x119 [i915]
[<f95e3bb5>] i915_gem_free_object_tail+0x18/0xcb [i915]
[<f95e3ca0>] i915_gem_free_object+0x38/0x3c [i915]
[<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
[<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
[<c1175baf>] kref_put+0x39/0x42
[<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
[<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
[<f95e2fa4>] i915_gem_busy_ioctl+0xe4/0x111 [i915]
[<f83d3233>] drm_ioctl+0x26b/0x327 [drm]
[<f95e2ec0>] ? i915_gem_busy_ioctl+0x0/0x111 [i915]
[<c1032677>] ? cpuacct_charge+0x72/0x7a
[<c1141274>] ? file_has_perm+0x7f/0x88
[<f83d2fc8>] ? drm_ioctl+0x0/0x327 [drm]
[<c10f07b5>] vfs_ioctl+0x18/0x71
[<c10f0d3d>] do_vfs_ioctl+0x486/0x4c4
[<c1141432>] ? selinux_file_ioctl+0x3e/0x41
[<c10f0dbc>] sys_ioctl+0x41/0x61
[<c1007ff8>] sysenter_do_call+0x12/0x38
<IRQ>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] drm/i915: Fix recursive calls to unmap
2011-11-02 19:29 ` Dave Airlie
@ 2011-11-03 4:47 ` Ben Widawsky
2011-11-03 20:19 ` Dave Airlie
0 siblings, 1 reply; 7+ messages in thread
From: Ben Widawsky @ 2011-11-03 4:47 UTC (permalink / raw)
To: Dave Airlie; +Cc: Daniel Vetter, intel-gfx
On Wed, Nov 02, 2011 at 07:29:53PM +0000, Dave Airlie wrote:
> On Mon, Oct 31, 2011 at 3:16 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > After the ILK vt-d workaround patches it became clear that we had
> > introduced a bug. Chris tracked down the issue to recursive calls to
> > unmap. This happens because we try to optimize waiting on requests by
> > calling retire requests after the wait, which may drop the last
> > reference on an object and end up freeing the object (and then unmap the
> > object from the gtt).
> >
> > The solution here is to add a new flag to the call chain which gives the
> > routines the information they need to possibly defer actions which may
> > cause us to recurse. A macro has been defined to replace i915_gpu_idle
> > which defaults to the old behavior.
> >
> > Kudos to Chris for tracking this one down.
>
> So this fixes the non-VTd case, the VT-d case still hits a recursion
> here, for posterity its below.
>
> Dave.
I'm stumped. Chris, Daniel, did you see the mistake?
...
> [<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
> [<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
> [<c1175baf>] kref_put+0x39/0x42
> [<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
> [<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
> [<f95e22cc>] i915_gem_wait_request+0x488/0x492 [i915]v
> [<c117dcab>] ? __sg_free_table+0x47/0x5e
> [<c1057751>] ? autoremove_wake_function+0x0/0x2f
> [<f95e2352>] i915_gem_gpu_idle+0x7c/0x95 [i915]
> [<f95e71be>] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915]
> [<f95e3b30>] i915_gem_object_unbind+0xac/0x119 [i915]
> [<f95e3bb5>] i915_gem_free_object_tail+0x18/0xcb [i915]
> [<f95e3ca0>] i915_gem_free_object+0x38/0x3c [i915]
> [<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
> [<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
> [<c1175baf>] kref_put+0x39/0x42
> [<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
Below is the first part that doesn't make sense to me. Busy calls
retire, which moves to inactive which unbinds from the GTT... that makes
perfect sense. We then call do_idling, which should be calling
i915_gem_gpu_idle with strictly_idle (a.k.a defer retirement). So how
the heck do we end up back in i915_gem_retire_requests_ring when the
code is:
if (ret == 0 && !defer_retirement) // if (true && !true)
i915_gem_retire_requests_ring(ring);
> [<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
> [<f95e22cc>] i915_gem_wait_request+0x488/0x492 [i915]
> [<c117dcab>] ? __sg_free_table+0x47/0x5e
> [<c1057751>] ? autoremove_wake_function+0x0/0x2f
> [<f95e2352>] i915_gem_gpu_idle+0x7c/0x95 [i915]
> [<f95e71be>] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915]
> [<f95e3b30>] i915_gem_object_unbind+0xac/0x119 [i915]
> [<f95e3bb5>] i915_gem_free_object_tail+0x18/0xcb [i915]
> [<f95e3ca0>] i915_gem_free_object+0x38/0x3c [i915]
> [<f83d440f>] ? drm_gem_object_free+0x0/0x24 [drm]
> [<f83d4431>] drm_gem_object_free+0x22/0x24 [drm]
> [<c1175baf>] kref_put+0x39/0x42
> [<f95e12da>] i915_gem_object_move_to_inactive+0xad/0xb1 [i915]
> [<f95e1426>] i915_gem_retire_requests_ring+0x148/0x173 [i915]
> [<f95e2fa4>] i915_gem_busy_ioctl+0xe4/0x111 [i915]
> [<f83d3233>] drm_ioctl+0x26b/0x327 [drm]
> [<f95e2ec0>] ? i915_gem_busy_ioctl+0x0/0x111 [i915]
> [<c1032677>] ? cpuacct_charge+0x72/0x7a
> [<c1141274>] ? file_has_perm+0x7f/0x88
> [<f83d2fc8>] ? drm_ioctl+0x0/0x327 [drm]
> [<c10f07b5>] vfs_ioctl+0x18/0x71
> [<c10f0d3d>] do_vfs_ioctl+0x486/0x4c4
> [<c1141432>] ? selinux_file_ioctl+0x3e/0x41
> [<c10f0dbc>] sys_ioctl+0x41/0x61
> [<c1007ff8>] sysenter_do_call+0x12/0x38
> <IRQ>
Ben
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] drm/i915: Fix recursive calls to unmap
2011-11-03 4:47 ` Ben Widawsky
@ 2011-11-03 20:19 ` Dave Airlie
2011-11-03 22:23 ` Ben Widawsky
0 siblings, 1 reply; 7+ messages in thread
From: Dave Airlie @ 2011-11-03 20:19 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Daniel Vetter, intel-gfx
>> >
>> > The solution here is to add a new flag to the call chain which gives the
>> > routines the information they need to possibly defer actions which may
>> > cause us to recurse. A macro has been defined to replace i915_gpu_idle
>> > which defaults to the old behavior.
>> >
>> > Kudos to Chris for tracking this one down.
>>
>> So this fixes the non-VTd case, the VT-d case still hits a recursion
>> here, for posterity its below.
Okay I take that back, I got my EL6 kernel rock stable with the
correct blend of backported bits.
So ignore that backtrace, however I did get another IOMMU hang on my
upstream kernel with gem_linear_blits,
so this should be fine to merge but I'm guessing we have more
debugging to do on the VT-d cases.
Dave.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] drm/i915: Fix recursive calls to unmap
2011-11-03 20:19 ` Dave Airlie
@ 2011-11-03 22:23 ` Ben Widawsky
2012-01-17 11:15 ` Daniel Vetter
0 siblings, 1 reply; 7+ messages in thread
From: Ben Widawsky @ 2011-11-03 22:23 UTC (permalink / raw)
To: Dave Airlie; +Cc: Daniel Vetter, intel-gfx
On Thu, 3 Nov 2011 20:19:23 +0000
Dave Airlie <airlied@gmail.com> wrote:
> >> >
> >> > The solution here is to add a new flag to the call chain which gives the
> >> > routines the information they need to possibly defer actions which may
> >> > cause us to recurse. A macro has been defined to replace i915_gpu_idle
> >> > which defaults to the old behavior.
> >> >
> >> > Kudos to Chris for tracking this one down.
> >>
> >> So this fixes the non-VTd case, the VT-d case still hits a recursion
> >> here, for posterity its below.
>
> Okay I take that back, I got my EL6 kernel rock stable with the
> correct blend of backported bits.
>
> So ignore that backtrace, however I did get another IOMMU hang on my
> upstream kernel with gem_linear_blits,
>
> so this should be fine to merge but I'm guessing we have more
> debugging to do on the VT-d cases.
>
> Dave.
Does it pass your original failing case?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] drm/i915: Fix recursive calls to unmap
2011-11-03 22:23 ` Ben Widawsky
@ 2012-01-17 11:15 ` Daniel Vetter
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2012-01-17 11:15 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Daniel Vetter, intel-gfx
On Thu, Nov 03, 2011 at 03:23:39PM -0700, Ben Widawsky wrote:
> On Thu, 3 Nov 2011 20:19:23 +0000
> Dave Airlie <airlied@gmail.com> wrote:
>
> > >> >
> > >> > The solution here is to add a new flag to the call chain which gives the
> > >> > routines the information they need to possibly defer actions which may
> > >> > cause us to recurse. A macro has been defined to replace i915_gpu_idle
> > >> > which defaults to the old behavior.
> > >> >
> > >> > Kudos to Chris for tracking this one down.
> > >>
> > >> So this fixes the non-VTd case, the VT-d case still hits a recursion
> > >> here, for posterity its below.
> >
> > Okay I take that back, I got my EL6 kernel rock stable with the
> > correct blend of backported bits.
> >
> > So ignore that backtrace, however I did get another IOMMU hang on my
> > upstream kernel with gem_linear_blits,
> >
> > so this should be fine to merge but I'm guessing we have more
> > debugging to do on the VT-d cases.
> >
> > Dave.
>
> Does it pass your original failing case?
Hi Ben,
Is v3 of this patch the right one to merge or do we still have some
outstanding issues on this? Also, have you looked at the recent ilk dmar
fallout, I think strict dmar iotlb flushing doesn't sit too well with our
gpus. My snb dies almost immediately with that (even when I do not enable
rc6 nor semaphores).
Cheers, Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-01-17 11:15 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-31 3:16 [PATCH v3] drm/i915: Fix recursive calls to unmap Ben Widawsky
2011-10-31 7:45 ` Daniel Vetter
2011-11-02 19:29 ` Dave Airlie
2011-11-03 4:47 ` Ben Widawsky
2011-11-03 20:19 ` Dave Airlie
2011-11-03 22:23 ` Ben Widawsky
2012-01-17 11:15 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox