From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 08/16] drm/i915/error: Do a better job of disambiguating VMAs Date: Sun, 20 Jul 2014 16:49:19 -0700 Message-ID: <20140720234919.GA10465@bwidawsk.net> References: <1404238671-18760-1-git-send-email-benjamin.widawsky@intel.com> <1404238671-18760-9-git-send-email-benjamin.widawsky@intel.com> <20140704075708.GG23812@nuc-i3427.alporthouse.com> <20140704165654.GB5903@bwidawsk.net> <20140717085123.GG15237@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail.bwidawsk.net (bwidawsk.net [166.78.191.112]) by gabe.freedesktop.org (Postfix) with ESMTP id A12EA89FA5 for ; Sun, 20 Jul 2014 16:49:36 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140717085123.GG15237@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: Intel GFX , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Thu, Jul 17, 2014 at 10:51:23AM +0200, Daniel Vetter wrote: > On Fri, Jul 04, 2014 at 09:56:54AM -0700, Ben Widawsky wrote: > > On Fri, Jul 04, 2014 at 08:57:08AM +0100, Chris Wilson wrote: > > > On Tue, Jul 01, 2014 at 11:17:43AM -0700, Ben Widawsky wrote: > > > > Some of the original PPGTT patches in this area where unmerged, and this > > > > left a lot of confusion in our error capture with regard to which vm/obj > > > > we want to capture. There have been at least a couple of patches from > > > > Chris, and myself to try to fix this up; so here is another shot. Nobody > > > > running without full PPGTT is effected by this, and that is probably why > > > > nobody has bothered to fix it yet. > > > > > > > > Instead of using any of the global lists to find the VMAs we want to > > > > capture, we use the union of the active, and the inactive list in the > > > > VM. This allows us to replace our capture_bo with capture_vma, and know > > > > all the VMAs we want to capture are valid. > > > > > > > > I could have probably figured out a way to reuse mm_list. As we've had > > > > bugs here before in the shrinker, I think the best way forward is to get > > > > it working, and then optimize it later. > > > > > > > > Signed-off-by: Ben Widawsky > > > > --- > > > > drivers/gpu/drm/i915/i915_gem_gtt.c | 1 + > > > > drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++ > > > > drivers/gpu/drm/i915/i915_gpu_error.c | 39 ++++++++++++++++++++++------------- > > > > 3 files changed, 28 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > index a4153ee..88451dc 100644 > > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > @@ -2114,6 +2114,7 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj, > > > > return ERR_PTR(-ENOMEM); > > > > > > > > INIT_LIST_HEAD(&vma->vma_link); > > > > + INIT_LIST_HEAD(&vma->pin_capture_link); > > > > INIT_LIST_HEAD(&vma->mm_list); > > > > INIT_LIST_HEAD(&vma->exec_list); > > > > vma->vm = vm; > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > > > > index 8d6f7c1..1d75801 100644 > > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > > > > @@ -126,6 +126,8 @@ struct i915_vma { > > > > > > > > struct list_head vma_link; /* Link in the object's VMA list */ > > > > > > > > + struct list_head pin_capture_link; /* Link in the error capture */ > > > > + > > > > /** This vma's place in the batchbuffer or on the eviction list */ > > > > struct list_head exec_list; > > > > > > We already have a slot for temporary lists... > > > -Chris > > > > > > > I did mention that in the commit message, if I caught your meaning. > > Chris is probably talking about exec_list which is our canonical temporary > list, mostly used by execbuf. But also in other places. > -Daniel I think that was a typo on my part, I meant exec_list. In either case, I think doing it this way and merging it later is the safest path. -- Ben Widawsky, Intel Open Source Technology Center