From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH v3] drm/i915: Fix recursive calls to unmap Date: Thu, 3 Nov 2011 04:47:31 +0000 Message-ID: <20111103044730.GA19799@cloud01> References: <1320031005-8729-1-git-send-email-ben@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from cloud01.chad-versace.us (184-106-247-128.static.cloud-ips.com [184.106.247.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 627429E810 for ; Wed, 2 Nov 2011 21:47:30 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Dave Airlie Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, Nov 02, 2011 at 07:29:53PM +0000, Dave Airlie wrote: > On Mon, Oct 31, 2011 at 3:16 AM, Ben Widawsky wrote: > > After the ILK vt-d workaround patches it became clear that we had > > introduced a bug. =A0Chris 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? = ... > [] ? drm_gem_object_free+0x0/0x24 [drm] > [] drm_gem_object_free+0x22/0x24 [drm] > [] kref_put+0x39/0x42 > [] i915_gem_object_move_to_inactive+0xad/0xb1 [i915] > [] i915_gem_retire_requests_ring+0x148/0x173 [i915] > [] i915_gem_wait_request+0x488/0x492 [i915]v > [] ? __sg_free_table+0x47/0x5e > [] ? autoremove_wake_function+0x0/0x2f > [] i915_gem_gpu_idle+0x7c/0x95 [i915] > [] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915] > [] i915_gem_object_unbind+0xac/0x119 [i915] > [] i915_gem_free_object_tail+0x18/0xcb [i915] > [] i915_gem_free_object+0x38/0x3c [i915] > [] ? drm_gem_object_free+0x0/0x24 [drm] > [] drm_gem_object_free+0x22/0x24 [drm] > [] kref_put+0x39/0x42 > [] 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 =3D=3D 0 && !defer_retirement) // if (true && !true) i915_gem_retire_requests_ring(ring); > [] i915_gem_retire_requests_ring+0x148/0x173 [i915] > [] i915_gem_wait_request+0x488/0x492 [i915] > [] ? __sg_free_table+0x47/0x5e > [] ? autoremove_wake_function+0x0/0x2f > [] i915_gem_gpu_idle+0x7c/0x95 [i915] > [] i915_gem_gtt_unbind_object+0x9e/0xc5 [i915] > [] i915_gem_object_unbind+0xac/0x119 [i915] > [] i915_gem_free_object_tail+0x18/0xcb [i915] > [] i915_gem_free_object+0x38/0x3c [i915] > [] ? drm_gem_object_free+0x0/0x24 [drm] > [] drm_gem_object_free+0x22/0x24 [drm] > [] kref_put+0x39/0x42 > [] i915_gem_object_move_to_inactive+0xad/0xb1 [i915] > [] i915_gem_retire_requests_ring+0x148/0x173 [i915] > [] i915_gem_busy_ioctl+0xe4/0x111 [i915] > [] drm_ioctl+0x26b/0x327 [drm] > [] ? i915_gem_busy_ioctl+0x0/0x111 [i915] > [] ? cpuacct_charge+0x72/0x7a > [] ? file_has_perm+0x7f/0x88 > [] ? drm_ioctl+0x0/0x327 [drm] > [] vfs_ioctl+0x18/0x71 > [] do_vfs_ioctl+0x486/0x4c4 > [] ? selinux_file_ioctl+0x3e/0x41 > [] sys_ioctl+0x41/0x61 > [] sysenter_do_call+0x12/0x38 > Ben