Re: [PATCH v3] drm/i915: Fix recursive calls to unmap

[Ben Widawsky <ben@bwidawsk.net>]

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