[Intel-gfx] [PATCH 1/2] drm/i915: Clean up pinned bo capture

[Chris Wilson]

On Wed, Dec 03, 2014 at 03:16:09PM +0100, Daniel Vetter wrote:
> On Tue, Dec 02, 2014 at 04:46:38PM +0000, Chris Wilson wrote:
> > On Tue, Dec 02, 2014 at 04:19:43PM +0100, Daniel Vetter wrote:
> > >  /* Generate a semi-unique error code. The code is not meant to have meaning, The
> > > @@ -1085,7 +1083,6 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
> > >  				const int ndx)
> > >  {
> > >  	struct drm_i915_error_buffer *active_bo = NULL, *pinned_bo = NULL;
> > > -	struct drm_i915_gem_object *obj;
> > >  	struct i915_vma *vma;
> > >  	int i;
> > >  
> > > @@ -1094,12 +1091,12 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
> > >  		i++;
> > >  	error->active_bo_count[ndx] = i;
> > >  
> > > -	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> > > -		list_for_each_entry(vma, &obj->vma_list, vma_link)
> > > -			if (vma->vm == vm && vma->pin_count > 0) {
> > > -				i++;
> > > +	if (i915_is_ggtt(vm)) {
> > > +		list_for_each_entry(vma, &vm->inactive_list, mm_list) {
> > > +			if (vma->pin_count == 0)
> > >  				break;
> > > -			}
> > > +			i++;
> > > +		}
> > >  	}
> > >  	error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx];
> > >  
> > > @@ -1119,7 +1116,7 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
> > >  		error->pinned_bo_count[ndx] =
> > >  			capture_pinned_bo(pinned_bo,
> > >  					  error->pinned_bo_count[ndx],
> > > -					  &dev_priv->mm.bound_list, vm);
> > > +					  &vm->inactive_list);
> > 
> > I much prefer this to be an iteraction over the bound_list and check if
> > the object has a ggtt bound vma. Combined with the request to only print
> > out the pinned bo in the GGTT, it becomes much clearer and doesn't need
> > to result in the confusing "active + pinned for GGTT".
> 
> We do still want to dump active objects for the GGTT, e.g. shadow batch
> (well we probably should add the GGTT vm to the dumper ...) and definitely
> when full ppgtt isn't enabled (i.e. everything).
> 
> And if we dump the active list then I think we should scan only the
> inactive list for pinned bo to avoid duplicate listing.

No. The pinned list includes the pinned active bo because it makes it
easier for me when checking if the right objects are being pinned (I
only have to crosscheck a single list rather than multiple).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre