[Intel-gfx] [PATCH 08/16] drm/i915/error: Do a better job of disambiguating VMAs

[Ben Widawsky]

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 <ben at bwidawsk.net>
> > > > ---
> > > >  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