All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/i915: Clean up pinned bo capture
@ 2014-12-02 15:19 Daniel Vetter
  2014-12-02 15:19 ` [PATCH 2/2] drm/i915: Don't capture pinned bo separately Daniel Vetter
  2014-12-02 16:46 ` [PATCH 1/2] drm/i915: Clean up pinned bo capture Chris Wilson
  0 siblings, 2 replies; 8+ messages in thread
From: Daniel Vetter @ 2014-12-02 15:19 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter, Daniel Vetter

Somewhere between removing the pinned bo list and adding full ppgtt
support we've started to no longer filter the active buffers out of
the captured pinned buffers. I've tried to dig out exactly where but
didn't spot it.

At first I've thought (checked, but without warning about it) array
overrun. But we have the same issue when counting the pinned bo.

Still not pretty, so switch over to walking the inactive per-vm list,
which will naturally filter out all active buffers.

v2: While at it only capture pinned bo for the global gtt, we don't
care about them for real ppgtt (since only execbuf pins those, and
only temporarily).

v3: Don't WARN_ON about overruns, this code is still racy. So just
debug output instead.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 43 ++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index c4536e185b75..b76443778e8e 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -696,35 +696,33 @@ static u32 capture_active_bo(struct drm_i915_error_buffer *err,
 
 	list_for_each_entry(vma, head, mm_list) {
 		capture_bo(err++, vma);
-		if (++i == count)
+		if (++i == count) {
+			DRM_DEBUG("Capture array overrun\n");
 			break;
+		}
 	}
 
 	return i;
 }
 
 static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
-			     int count, struct list_head *head,
-			     struct i915_address_space *vm)
+			     int count, struct list_head *head)
 {
-	struct drm_i915_gem_object *obj;
-	struct drm_i915_error_buffer * const first = err;
-	struct drm_i915_error_buffer * const last = err + count;
-
-	list_for_each_entry(obj, head, global_list) {
-		struct i915_vma *vma;
+	struct i915_vma *vma;
+	int i = 0;
 
-		if (err == last)
+	list_for_each_entry(vma, head, mm_list) {
+		if (vma->pin_count == 0)
 			break;
 
-		list_for_each_entry(vma, &obj->vma_list, vma_link)
-			if (vma->vm == vm && vma->pin_count > 0) {
-				capture_bo(err++, vma);
-				break;
-			}
+		capture_bo(err++, vma);
+		if (++i == count) {
+			DRM_DEBUG("Capture array overrun\n");
+			break;
+		}
 	}
 
-	return err - first;
+	return i;
 }
 
 /* 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);
 	error->active_bo[ndx] = active_bo;
 	error->pinned_bo[ndx] = pinned_bo;
 }
-- 
1.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] drm/i915: Don't capture pinned bo separately
  2014-12-02 15:19 [PATCH 1/2] drm/i915: Clean up pinned bo capture Daniel Vetter
@ 2014-12-02 15:19 ` Daniel Vetter
  2014-12-03  1:34   ` shuang.he
  2014-12-02 16:46 ` [PATCH 1/2] drm/i915: Clean up pinned bo capture Chris Wilson
  1 sibling, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2014-12-02 15:19 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter, Daniel Vetter

We only care about pinned bo in the global ggtt, so simplify the logic
a bit. Since we don't yet capture any per-vm state the pretty-printing
isn't too pretty.

v2: git add to make it build ...

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |  4 ++--
 drivers/gpu/drm/i915/i915_gpu_error.c | 23 ++++-------------------
 2 files changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ef1f00e0a7b3..f9fe9293290c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -450,9 +450,9 @@ struct drm_i915_error_state {
 		u32 userptr:1;
 		s32 ring:4;
 		u32 cache_level:3;
-	} **active_bo, **pinned_bo;
+	} **active_bo;
 
-	u32 *active_bo_count, *pinned_bo_count;
+	u32 *active_bo_count;
 	u32 vm_count;
 };
 
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index b76443778e8e..b1387343812d 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -398,13 +398,9 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 	for (i = 0; i < error->vm_count; i++) {
 		err_printf(m, "vm[%d]\n", i);
 
-		print_error_buffers(m, "Active",
+		print_error_buffers(m, "Active (+ pinned for ggtt)",
 				    error->active_bo[i],
 				    error->active_bo_count[i]);
-
-		print_error_buffers(m, "Pinned",
-				    error->pinned_bo[i],
-				    error->pinned_bo_count[i]);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(error->ring); i++) {
@@ -1098,7 +1094,6 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
 			i++;
 		}
 	}
-	error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx];
 
 	if (i) {
 		active_bo = kcalloc(i, sizeof(*active_bo), GFP_ATOMIC);
@@ -1113,12 +1108,11 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
 					  &vm->active_list);
 
 	if (pinned_bo)
-		error->pinned_bo_count[ndx] =
+		error->active_bo_count[ndx] +=
 			capture_pinned_bo(pinned_bo,
-					  error->pinned_bo_count[ndx],
+					  i - error->active_bo_count[ndx],
 					  &vm->inactive_list);
 	error->active_bo[ndx] = active_bo;
-	error->pinned_bo[ndx] = pinned_bo;
 }
 
 static void i915_gem_capture_buffers(struct drm_i915_private *dev_priv,
@@ -1131,25 +1125,16 @@ static void i915_gem_capture_buffers(struct drm_i915_private *dev_priv,
 		cnt++;
 
 	error->active_bo = kcalloc(cnt, sizeof(*error->active_bo), GFP_ATOMIC);
-	error->pinned_bo = kcalloc(cnt, sizeof(*error->pinned_bo), GFP_ATOMIC);
 	error->active_bo_count = kcalloc(cnt, sizeof(*error->active_bo_count),
 					 GFP_ATOMIC);
-	error->pinned_bo_count = kcalloc(cnt, sizeof(*error->pinned_bo_count),
-					 GFP_ATOMIC);
 
 	if (error->active_bo == NULL ||
-	    error->pinned_bo == NULL ||
-	    error->active_bo_count == NULL ||
-	    error->pinned_bo_count == NULL) {
+	    error->active_bo_count == NULL) {
 		kfree(error->active_bo);
 		kfree(error->active_bo_count);
-		kfree(error->pinned_bo);
-		kfree(error->pinned_bo_count);
 
 		error->active_bo = NULL;
 		error->active_bo_count = NULL;
-		error->pinned_bo = NULL;
-		error->pinned_bo_count = NULL;
 	} else {
 		list_for_each_entry(vm, &dev_priv->vm_list, global_link)
 			i915_gem_capture_vm(dev_priv, error, vm, i++);
-- 
1.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] drm/i915: Clean up pinned bo capture
  2014-12-02 15:19 [PATCH 1/2] drm/i915: Clean up pinned bo capture Daniel Vetter
  2014-12-02 15:19 ` [PATCH 2/2] drm/i915: Don't capture pinned bo separately Daniel Vetter
@ 2014-12-02 16:46 ` Chris Wilson
  2014-12-03 14:16   ` Daniel Vetter
  1 sibling, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2014-12-02 16:46 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development

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".
-Chris
-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] drm/i915: Don't capture pinned bo separately
  2014-12-02 15:19 ` [PATCH 2/2] drm/i915: Don't capture pinned bo separately Daniel Vetter
@ 2014-12-03  1:34   ` shuang.he
  0 siblings, 0 replies; 8+ messages in thread
From: shuang.he @ 2014-12-03  1:34 UTC (permalink / raw)
  To: shuang.he, intel-gfx, daniel.vetter

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -2              364/364              362/364
ILK              +1                 365/366              366/366
SNB                                  450/450              450/450
IVB                                  498/498              498/498
BYT                                  289/289              289/289
HSW                                  564/564              564/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gem_fence_thrash_bo-write-verify-x      PASS(2, M25M23)      NO_RESULT(1, M23)
*PNV  igt_gem_tiled_partial_pwrite_pread_writes-after-reads      PASS(2, M25M23)      NO_RESULT(1, M23)
 ILK  igt_kms_flip_wf_vblank-ts-check      DMESG_WARN(2, M26)PASS(11, M26M37)      PASS(1, M37)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] drm/i915: Clean up pinned bo capture
  2014-12-02 16:46 ` [PATCH 1/2] drm/i915: Clean up pinned bo capture Chris Wilson
@ 2014-12-03 14:16   ` Daniel Vetter
  2014-12-04  9:11     ` Chris Wilson
  0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2014-12-03 14:16 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, Intel Graphics Development, Daniel Vetter

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.
-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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] drm/i915: Clean up pinned bo capture
  2014-12-03 14:16   ` Daniel Vetter
@ 2014-12-04  9:11     ` Chris Wilson
  2014-12-04 10:47       ` Daniel Vetter
  0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2014-12-04  9:11 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] drm/i915: Clean up pinned bo capture
  2014-12-04  9:11     ` Chris Wilson
@ 2014-12-04 10:47       ` Daniel Vetter
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2014-12-04 10:47 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, Daniel Vetter,
	Intel Graphics Development, Daniel Vetter

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] drm/i915: Clean up pinned bo capture
@ 2014-12-02  9:56 Daniel Vetter
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2014-12-02  9:56 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter, Daniel Vetter

Somewhere between removing the pinned bo list and adding full ppgtt
support we've started to no longer filter the active buffers out of
the captured pinned buffers. I've tried to dig out exactly where but
didn't spot it.

At first I've thought (checked, but without warning about it) array
overrun. But we have the same issue when counting the pinned bo.

Still not pretty, so switch over to walking the inactive per-vm list,
which will naturally filter out all active buffers.

v2: While at it only capture pinned bo for the global gtt, we don't
care about them for real ppgtt (since only execbuf pins those, and
only temporarily).

v3: Don't WARN_ON about overruns, this code is still racy. So just
debug output instead.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 43 ++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index cdaee6ce05f8..644496690e8d 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -697,35 +697,33 @@ static u32 capture_active_bo(struct drm_i915_error_buffer *err,
 
 	list_for_each_entry(vma, head, mm_list) {
 		capture_bo(err++, vma);
-		if (++i == count)
+		if (++i == count) {
+			DRM_DEBUG("Capture array overrun\n");
 			break;
+		}
 	}
 
 	return i;
 }
 
 static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
-			     int count, struct list_head *head,
-			     struct i915_address_space *vm)
+			     int count, struct list_head *head)
 {
-	struct drm_i915_gem_object *obj;
-	struct drm_i915_error_buffer * const first = err;
-	struct drm_i915_error_buffer * const last = err + count;
-
-	list_for_each_entry(obj, head, global_list) {
-		struct i915_vma *vma;
+	struct i915_vma *vma;
+	int i = 0;
 
-		if (err == last)
+	list_for_each_entry(vma, head, mm_list) {
+		if (vma->pin_count == 0)
 			break;
 
-		list_for_each_entry(vma, &obj->vma_list, vma_link)
-			if (vma->vm == vm && vma->pin_count > 0) {
-				capture_bo(err++, vma);
-				break;
-			}
+		capture_bo(err++, vma);
+		if (++i == count) {
+			DRM_DEBUG("Capture array overrun\n");
+			break;
+		}
 	}
 
-	return err - first;
+	return i;
 }
 
 /* Generate a semi-unique error code. The code is not meant to have meaning, The
@@ -1086,7 +1084,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;
 
@@ -1095,12 +1092,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];
 
@@ -1120,7 +1117,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);
 	error->active_bo[ndx] = active_bo;
 	error->pinned_bo[ndx] = pinned_bo;
 }
-- 
2.1.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-12-04 10:46 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-02 15:19 [PATCH 1/2] drm/i915: Clean up pinned bo capture Daniel Vetter
2014-12-02 15:19 ` [PATCH 2/2] drm/i915: Don't capture pinned bo separately Daniel Vetter
2014-12-03  1:34   ` shuang.he
2014-12-02 16:46 ` [PATCH 1/2] drm/i915: Clean up pinned bo capture Chris Wilson
2014-12-03 14:16   ` Daniel Vetter
2014-12-04  9:11     ` Chris Wilson
2014-12-04 10:47       ` Daniel Vetter
  -- strict thread matches above, loose matches on Subject: below --
2014-12-02  9:56 Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.