Re: [PATCH 1/2] drm/i915: Clean up pinned bo capture

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Dec 04, 2014 at 09:11:10AM +0000, Chris Wilson wrote:
> 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).

But I've thought the point of patch 2 was to long longer have that
disdinction? Or why did you want to remove the separate pinned_bo list?
Throughroughly confused over here now ..
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx