[PATCH 1/2] drm/i915: Don't drop pinned fences

[PATCH 1/2] drm/i915: Don't drop pinned fences

[Daniel Vetter]

Userspace can currently provoke this when e.g. trying to use a pinned
scanout as a cursor or overlay target. Later on that might lead to
some fun fence pin count mayhem.

Spurred by Ville's report that something goes wrong here and
originally I've thought that this might slip through the pwrite gtt
fastpath. But that one checks of obj tiling, so should be ok.

But one thing that _does_ blow up is the vma unbinding with more than
one address space. The next patch will fix this.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 05e79fa02db2..9559d33cd94b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3058,6 +3058,9 @@ i915_gem_object_put_fence(struct drm_i915_gem_object *obj)
 
 	fence = &dev_priv->fence_regs[obj->fence_reg];
 
+	if (fence->pin_count)
+		return -EBUSY;
+
 	i915_gem_object_fence_lost(obj);
 	i915_gem_object_update_fence(obj, fence, false);
 
-- 
1.8.5.2

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

[PATCH 2/2] drm/i915: Only do gtt cleanup in vma_unbind for the global vma

[Daniel Vetter]

Otherwise we end up tearing down fences when e.g. the client quits
way too early. Might or might not fix a fence pin_count BUG Ville has
reported.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9559d33cd94b..1c118ade031b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2779,12 +2779,14 @@ int i915_vma_unbind(struct i915_vma *vma)
 	 * cause memory corruption through use-after-free.
 	 */
 
-	i915_gem_object_finish_gtt(obj);
+	if (i915_is_ggtt(vma->vm)) {
+		i915_gem_object_finish_gtt(obj);
 
-	/* release the fence reg _after_ flushing */
-	ret = i915_gem_object_put_fence(obj);
-	if (ret)
-		return ret;
+		/* release the fence reg _after_ flushing */
+		ret = i915_gem_object_put_fence(obj);
+		if (ret)
+			return ret;
+	}
 
 	trace_i915_vma_unbind(vma);
 
-- 
1.8.5.2

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

Re: [PATCH 1/2] drm/i915: Don't drop pinned fences

[Chris Wilson]

On Mon, Jan 27, 2014 at 09:49:47PM +0100, Daniel Vetter wrote:
> Userspace can currently provoke this when e.g. trying to use a pinned
> scanout as a cursor or overlay target. Later on that might lead to
> some fun fence pin count mayhem.
> 
> Spurred by Ville's report that something goes wrong here and
> originally I've thought that this might slip through the pwrite gtt
> fastpath. But that one checks of obj tiling, so should be ok.
> 
> But one thing that _does_ blow up is the vma unbinding with more than
> one address space. The next patch will fix this.

It's buggy behaviour, so if (WARN_ON(fence->pin_count) return -EBUSY;

I did worry that we would then have some unexpected failure, a new EBUSY
returning to userspace, but I think along the paths that can trigger it,
we have already returned EBUSY in the (dim and distant) past.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Don't drop pinned fences

[Daniel Vetter]

On Mon, Jan 27, 2014 at 10:26 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Jan 27, 2014 at 09:49:47PM +0100, Daniel Vetter wrote:
>> Userspace can currently provoke this when e.g. trying to use a pinned
>> scanout as a cursor or overlay target. Later on that might lead to
>> some fun fence pin count mayhem.
>>
>> Spurred by Ville's report that something goes wrong here and
>> originally I've thought that this might slip through the pwrite gtt
>> fastpath. But that one checks of obj tiling, so should be ok.
>>
>> But one thing that _does_ blow up is the vma unbinding with more than
>> one address space. The next patch will fix this.
>
> It's buggy behaviour, so if (WARN_ON(fence->pin_count) return -EBUSY;
>
> I did worry that we would then have some unexpected failure, a new EBUSY
> returning to userspace, but I think along the paths that can trigger it,
> we have already returned EBUSY in the (dim and distant) past.

Indeed we seem to have the required tiling checks everywhere. At first
I've had the WARN but then though userspace could sneak in through the
cursor/overlay code, but it can't. I'll update the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: Don't drop pinned fences

[Ville Syrjälä]

On Mon, Jan 27, 2014 at 10:41:57PM +0100, Daniel Vetter wrote:
> On Mon, Jan 27, 2014 at 10:26 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Mon, Jan 27, 2014 at 09:49:47PM +0100, Daniel Vetter wrote:
> >> Userspace can currently provoke this when e.g. trying to use a pinned
> >> scanout as a cursor or overlay target. Later on that might lead to
> >> some fun fence pin count mayhem.
> >>
> >> Spurred by Ville's report that something goes wrong here and
> >> originally I've thought that this might slip through the pwrite gtt
> >> fastpath. But that one checks of obj tiling, so should be ok.
> >>
> >> But one thing that _does_ blow up is the vma unbinding with more than
> >> one address space. The next patch will fix this.
> >
> > It's buggy behaviour, so if (WARN_ON(fence->pin_count) return -EBUSY;
> >
> > I did worry that we would then have some unexpected failure, a new EBUSY
> > returning to userspace, but I think along the paths that can trigger it,
> > we have already returned EBUSY in the (dim and distant) past.
> 
> Indeed we seem to have the required tiling checks everywhere. At first
> I've had the WARN but then though userspace could sneak in through the
> cursor/overlay code, but it can't. I'll update the patch.

I've tested these (+ Chris's WARN_ON) and the issue looks to be fixed.

Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

-- 
Ville Syrjälä
Intel OTC

[PATCH 1/3] drm/i915: Don't drop pinned fences

[Daniel Vetter]

Userspace can currently provoke this when e.g. trying to use a pinned
scanout as a cursor or overlay target. Later on that might lead to
some fun fence pin count mayhem.

Spurred by Ville's report that something goes wrong here and
originally I've thought that this might slip through the pwrite gtt
fastpath. But that one checks of obj tiling, so should be ok.

But one thing that _does_ blow up is the vma unbinding with more than
one address space. The next patch will fix this.

v2: Use a WARN_ON - Chris pointed out that we already catch all cases
so userspace can't provoke this like I've originally feared.

While reviewing relevant code I've noticed a pile of DRM_ERROR in the
overlay&cursor code which are all triggerable by userspace. Tune them
down while at it.

v3: Split out the DRM_ERROR->DRM_DEBUG_KMS change into a separate patch,
as requested by Chris.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3618bb0cda0a..675ad96a43e1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3010,6 +3010,9 @@ i915_gem_object_put_fence(struct drm_i915_gem_object *obj)
 
 	fence = &dev_priv->fence_regs[obj->fence_reg];
 
+	if (WARN_ON(fence->pin_count))
+		return -EBUSY;
+
 	i915_gem_object_fence_lost(obj);
 	i915_gem_object_update_fence(obj, fence, false);
 
-- 
1.8.5.2

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

[PATCH 2/3] drm/i915: tune down user-triggerable dmesg noise in the cursor/overlay code

[Daniel Vetter]

Spotted while auditing the code for fencing issues.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 10 +++++-----
 drivers/gpu/drm/i915/intel_overlay.c |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ac05da4cc188..8f54e779f09a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7549,7 +7549,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 		return -ENOENT;
 
 	if (obj->base.size < width * height * 4) {
-		DRM_ERROR("buffer is to small\n");
+		DRM_DEBUG_KMS("buffer is to small\n");
 		ret = -ENOMEM;
 		goto fail;
 	}
@@ -7560,7 +7560,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 		unsigned alignment;
 
 		if (obj->tiling_mode) {
-			DRM_ERROR("cursor cannot be tiled\n");
+			DRM_DEBUG_KMS("cursor cannot be tiled\n");
 			ret = -EINVAL;
 			goto fail_locked;
 		}
@@ -7576,13 +7576,13 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 
 		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
 		if (ret) {
-			DRM_ERROR("failed to move cursor bo into the GTT\n");
+			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
 			goto fail_locked;
 		}
 
 		ret = i915_gem_object_put_fence(obj);
 		if (ret) {
-			DRM_ERROR("failed to release fence for cursor");
+			DRM_DEBUG_KMS("failed to release fence for cursor");
 			goto fail_unpin;
 		}
 
@@ -7593,7 +7593,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 						  (intel_crtc->pipe == 0) ? I915_GEM_PHYS_CURSOR_0 : I915_GEM_PHYS_CURSOR_1,
 						  align);
 		if (ret) {
-			DRM_ERROR("failed to attach phys object\n");
+			DRM_DEBUG_KMS("failed to attach phys object\n");
 			goto fail_locked;
 		}
 		addr = obj->phys_obj->handle->busaddr;
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index ac519cb46f22..312961a8472e 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -1076,7 +1076,7 @@ int intel_overlay_put_image(struct drm_device *dev, void *data,
 	mutex_lock(&dev->struct_mutex);
 
 	if (new_bo->tiling_mode) {
-		DRM_ERROR("buffer used for overlay image can not be tiled\n");
+		DRM_DEBUG_KMS("buffer used for overlay image can not be tiled\n");
 		ret = -EINVAL;
 		goto out_unlock;
 	}
-- 
1.8.5.2

[PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma

[Daniel Vetter]

Otherwise we end up tearing down fences when e.g. the client quits
way too early. Might or might not fix a fence pin_count BUG Ville has
reported.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 675ad96a43e1..fa00b26a9cf7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2731,12 +2731,14 @@ int i915_vma_unbind(struct i915_vma *vma)
 	 * cause memory corruption through use-after-free.
 	 */
 
-	i915_gem_object_finish_gtt(obj);
+	if (i915_is_ggtt(vma->vm)) {
+		i915_gem_object_finish_gtt(obj);
 
-	/* release the fence reg _after_ flushing */
-	ret = i915_gem_object_put_fence(obj);
-	if (ret)
-		return ret;
+		/* release the fence reg _after_ flushing */
+		ret = i915_gem_object_put_fence(obj);
+		if (ret)
+			return ret;
+	}
 
 	trace_i915_vma_unbind(vma);
 
-- 
1.8.5.2

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

Re: [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma

[Ville Syrjälä]

On Fri, Feb 14, 2014 at 02:06:07PM +0100, Daniel Vetter wrote:
> Otherwise we end up tearing down fences when e.g. the client quits
> way too early. Might or might not fix a fence pin_count BUG Ville has
> reported.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

This one should also have:
Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 675ad96a43e1..fa00b26a9cf7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2731,12 +2731,14 @@ int i915_vma_unbind(struct i915_vma *vma)
>  	 * cause memory corruption through use-after-free.
>  	 */
>  
> -	i915_gem_object_finish_gtt(obj);
> +	if (i915_is_ggtt(vma->vm)) {
> +		i915_gem_object_finish_gtt(obj);
>  
> -	/* release the fence reg _after_ flushing */
> -	ret = i915_gem_object_put_fence(obj);
> -	if (ret)
> -		return ret;
> +		/* release the fence reg _after_ flushing */
> +		ret = i915_gem_object_put_fence(obj);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	trace_i915_vma_unbind(vma);
>  
> -- 
> 1.8.5.2

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma

[Chris Wilson]

On Fri, Feb 14, 2014 at 02:06:07PM +0100, Daniel Vetter wrote:
> Otherwise we end up tearing down fences when e.g. the client quits
> way too early. Might or might not fix a fence pin_count BUG Ville has
> reported.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Good catch. I am slightly worried that our management of GGTT is
a little haphazard, but given the slightly differing use-cases in
i915_vma_unbind and set_cache_level, this looks cleaner than any
alternative I quickly sketched.

This and the preceding patches are
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/3] drm/i915: tune down user-triggerable dmesg noise in the cursor/overlay code

[Damien Lespiau]

On Fri, Feb 14, 2014 at 02:06:06PM +0100, Daniel Vetter wrote:
> Spotted while auditing the code for fencing issues.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 +++++-----
>  drivers/gpu/drm/i915/intel_overlay.c |  2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ac05da4cc188..8f54e779f09a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7549,7 +7549,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  		return -ENOENT;
>  
>  	if (obj->base.size < width * height * 4) {
> -		DRM_ERROR("buffer is to small\n");
> +		DRM_DEBUG_KMS("buffer is to small\n");
>  		ret = -ENOMEM;
>  		goto fail;
>  	}
> @@ -7560,7 +7560,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  		unsigned alignment;
>  
>  		if (obj->tiling_mode) {
> -			DRM_ERROR("cursor cannot be tiled\n");
> +			DRM_DEBUG_KMS("cursor cannot be tiled\n");
>  			ret = -EINVAL;
>  			goto fail_locked;
>  		}
> @@ -7576,13 +7576,13 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  
>  		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
>  		if (ret) {
> -			DRM_ERROR("failed to move cursor bo into the GTT\n");
> +			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
>  			goto fail_locked;
>  		}
>  
>  		ret = i915_gem_object_put_fence(obj);
>  		if (ret) {
> -			DRM_ERROR("failed to release fence for cursor");
> +			DRM_DEBUG_KMS("failed to release fence for cursor");
>  			goto fail_unpin;
>  		}
>  
> @@ -7593,7 +7593,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  						  (intel_crtc->pipe == 0) ? I915_GEM_PHYS_CURSOR_0 : I915_GEM_PHYS_CURSOR_1,
>  						  align);
>  		if (ret) {
> -			DRM_ERROR("failed to attach phys object\n");
> +			DRM_DEBUG_KMS("failed to attach phys object\n");
>  			goto fail_locked;
>  		}
>  		addr = obj->phys_obj->handle->busaddr;
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index ac519cb46f22..312961a8472e 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -1076,7 +1076,7 @@ int intel_overlay_put_image(struct drm_device *dev, void *data,
>  	mutex_lock(&dev->struct_mutex);
>  
>  	if (new_bo->tiling_mode) {
> -		DRM_ERROR("buffer used for overlay image can not be tiled\n");
> +		DRM_DEBUG_KMS("buffer used for overlay image can not be tiled\n");
>  		ret = -EINVAL;
>  		goto out_unlock;
>  	}
> -- 
> 1.8.5.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: tune down user-triggerable dmesg noise in the cursor/overlay code

[Daniel Vetter]

On Fri, Feb 14, 2014 at 05:29:30PM +0000, Damien Lespiau wrote:
> On Fri, Feb 14, 2014 at 02:06:06PM +0100, Daniel Vetter wrote:
> > Spotted while auditing the code for fencing issues.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Queued for -next, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH igt] tests/kms_fence_pin_leak: Exercise full ppgtt fence pin_count leak in the kernel

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The kernel full ppgtt support has a bug where it can drop a pinned
fence to the floor, hence we leak the pin_count as the subsequent
fence unpin becomes a nop. We can trigger it easily by unbinding a
buffer from a ppgtt address space while the buffer is simultaneosly
being used for scanout.

Monitor i915_gem_fence_regs to detect the leak while performing
the required set of operations in a loop.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 tests/Makefile.sources     |   1 +
 tests/kms_fence_pin_leak.c | 224 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 225 insertions(+)
 create mode 100644 tests/kms_fence_pin_leak.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 5d5dc46..393c4a2 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -60,6 +60,7 @@ TESTS_progs_M = \
 	kms_addfb \
 	kms_cursor_crc \
 	kms_fbc_crc \
+	kms_fence_pin_leak \
 	kms_flip \
 	kms_flip_tiling \
 	kms_pipe_crc_basic \
diff --git a/tests/kms_fence_pin_leak.c b/tests/kms_fence_pin_leak.c
new file mode 100644
index 0000000..f299212
--- /dev/null
+++ b/tests/kms_fence_pin_leak.c
@@ -0,0 +1,224 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include <errno.h>
+#include <limits.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <string.h>
+
+#include "drmtest.h"
+#include "igt_debugfs.h"
+#include "igt_kms.h"
+#include "ioctl_wrappers.h"
+#include "intel_chipset.h"
+
+typedef struct {
+	int drm_fd;
+	uint32_t devid;
+	drm_intel_bufmgr *bufmgr;
+	igt_display_t display;
+} data_t;
+
+static void exec_nop(data_t *data, uint32_t handle, drm_intel_context *context)
+{
+	drm_intel_bo *dst;
+	struct intel_batchbuffer *batch;
+
+	dst = gem_handle_to_libdrm_bo(data->bufmgr, data->drm_fd, "", handle);
+	igt_assert(dst);
+
+	batch = intel_batchbuffer_alloc(data->bufmgr, data->devid);
+	igt_assert(batch);
+
+	/* add the reloc to make sure the kernel will think we write to dst */
+	BEGIN_BATCH(4);
+	OUT_BATCH(MI_BATCH_BUFFER_END);
+	OUT_BATCH(MI_NOOP);
+	OUT_RELOC(dst, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
+	OUT_BATCH(MI_NOOP);
+	ADVANCE_BATCH();
+
+	intel_batchbuffer_flush_with_context(batch, context);
+	intel_batchbuffer_free(batch);
+
+	drm_intel_bo_unreference(dst);
+}
+
+static unsigned int total_fence_pin_count(void)
+{
+	unsigned int pinned = 0;
+	char buf[128];
+	FILE *f;
+
+	f = igt_debugfs_fopen("i915_gem_fence_regs", "r");
+	igt_assert(f);
+
+	for (;;) {
+		unsigned int pin_count = 0;
+		char *ptr;
+
+		ptr = fgets(buf, sizeof(buf), f);
+		if (!ptr)
+			break;
+
+		if (sscanf(ptr, "Fence %*u, pin count = %u", &pin_count) == 1)
+			pinned += pin_count;
+	}
+
+	fclose(f);
+
+	return pinned;
+}
+
+static bool run_single_test(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+	igt_display_t *display = &data->display;
+	drmModeModeInfo *mode;
+	igt_plane_t *primary;
+	unsigned int pin_count;
+	struct igt_fb fb[2];
+	int i;
+
+	igt_output_set_pipe(output, pipe);
+	igt_display_commit(display);
+
+	if (!output->valid) {
+		igt_output_set_pipe(output, PIPE_ANY);
+		igt_display_commit(display);
+		return false;
+	}
+
+	mode = igt_output_get_mode(output);
+	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
+
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+			    DRM_FORMAT_XRGB8888,
+			    true, /* need a fence so must be tiled */
+			    0.0, 0.0, 0.0,
+			    &fb[0]);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+			    DRM_FORMAT_XRGB8888,
+			    true, /* need a fence so must be tiled */
+			    0.0, 0.0, 0.0,
+			    &fb[1]);
+
+	igt_plane_set_fb(primary, &fb[0]);
+	igt_display_commit(display);
+
+	/* sample the fence pin_counts now that we have one scanout buffer pinned */
+	pin_count = total_fence_pin_count();
+
+	for (i = 0; i < 64; i++) {
+		drm_intel_context *ctx;
+
+		/*
+		 * Link fb.gem_handle to the ppgtt vm of ctx so that the context
+		 * destruction will unbind the obj from the ppgtt vm in question.
+		 */
+		ctx = drm_intel_gem_context_create(data->bufmgr);
+		igt_assert(ctx);
+		exec_nop(data, fb[i&1].gem_handle, ctx);
+		drm_intel_gem_context_destroy(ctx);
+
+		/* Force a context switch to make sure ctx gets destroyed for real. */
+		exec_nop(data, fb[i&1].gem_handle, NULL);
+
+		gem_sync(data->drm_fd, fb[i&1].gem_handle);
+
+		/*
+		 * Pin the new buffer and unpin the old buffer from display. If
+		 * the kernel is buggy the ppgtt unbind will have dropped the
+		 * fence for the old buffer, and now the display code will try
+		 * to unpin only to find no fence there. So the pin_count will leak.
+		 */
+		igt_plane_set_fb(primary, &fb[!(i&1)]);
+		igt_display_commit(display);
+
+		/*
+		 * We always have exactly one scanout buffer, so the fence pin_count
+		 * should remain static. Fail the test if we detect a leak.
+		 */
+		igt_assert(pin_count == total_fence_pin_count());
+
+		printf(".");
+		fflush(stdout);
+	}
+
+	igt_plane_set_fb(primary, NULL);
+	igt_output_set_pipe(output, PIPE_ANY);
+	igt_display_commit(display);
+
+	igt_remove_fb(data->drm_fd, &fb[1]);
+	igt_remove_fb(data->drm_fd, &fb[0]);
+
+	printf("\n");
+
+	return true;
+}
+
+static void run_test(data_t *data)
+{
+	igt_display_t *display = &data->display;
+	igt_output_t *output;
+	enum pipe p;
+
+	for_each_connected_output(display, output) {
+		for (p = 0; p < igt_display_get_n_pipes(display); p++) {
+			if (run_single_test(data, p, output))
+				return; /* one time ought to be enough */
+		}
+	}
+
+	igt_skip("no valid crtc/connector combinations found\n");
+}
+
+igt_simple_main
+{
+	drm_intel_context *ctx;
+	data_t data = {};
+
+	igt_skip_on_simulation();
+
+	data.drm_fd = drm_open_any();
+
+	data.devid = intel_get_drm_devid(data.drm_fd);
+
+	igt_set_vt_graphics_mode();
+
+	data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096);
+	igt_assert(data.bufmgr);
+	drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
+
+	igt_display_init(&data.display, data.drm_fd);
+
+	ctx = drm_intel_gem_context_create(data.bufmgr);
+	igt_require(ctx);
+	drm_intel_gem_context_destroy(ctx);
+
+	run_test(&data);
+
+	drm_intel_bufmgr_destroy(data.bufmgr);
+	igt_display_fini(&data.display);
+}
-- 
1.8.3.2

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

Re: [PATCH igt] tests/kms_fence_pin_leak: Exercise full ppgtt fence pin_count leak in the kernel

[Daniel Vetter]

On Mon, May 12, 2014 at 08:46:24PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The kernel full ppgtt support has a bug where it can drop a pinned
> fence to the floor, hence we leak the pin_count as the subsequent
> fence unpin becomes a nop. We can trigger it easily by unbinding a
> buffer from a ppgtt address space while the buffer is simultaneosly
> being used for scanout.
> 
> Monitor i915_gem_fence_regs to detect the leak while performing
> the required set of operations in a loop.

do we really need that part? I've hoped just leaking enough fences until
the kernel blows up should be sufficient.

Generally I'm vary of using debugfs in new tests a bit since we
essentially then lock down that debugfs file as abi. And since no one
easily notices when a test stops catching a bug that's usually bad. Hence
also why I want basic testcases for debugfs infrastructure to make sure it
keeps on working.
-Daniel

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  tests/Makefile.sources     |   1 +
>  tests/kms_fence_pin_leak.c | 224 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 225 insertions(+)
>  create mode 100644 tests/kms_fence_pin_leak.c
> 
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 5d5dc46..393c4a2 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -60,6 +60,7 @@ TESTS_progs_M = \
>  	kms_addfb \
>  	kms_cursor_crc \
>  	kms_fbc_crc \
> +	kms_fence_pin_leak \
>  	kms_flip \
>  	kms_flip_tiling \
>  	kms_pipe_crc_basic \
> diff --git a/tests/kms_fence_pin_leak.c b/tests/kms_fence_pin_leak.c
> new file mode 100644
> index 0000000..f299212
> --- /dev/null
> +++ b/tests/kms_fence_pin_leak.c
> @@ -0,0 +1,224 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include <errno.h>
> +#include <limits.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include "drmtest.h"
> +#include "igt_debugfs.h"
> +#include "igt_kms.h"
> +#include "ioctl_wrappers.h"
> +#include "intel_chipset.h"
> +
> +typedef struct {
> +	int drm_fd;
> +	uint32_t devid;
> +	drm_intel_bufmgr *bufmgr;
> +	igt_display_t display;
> +} data_t;
> +
> +static void exec_nop(data_t *data, uint32_t handle, drm_intel_context *context)
> +{
> +	drm_intel_bo *dst;
> +	struct intel_batchbuffer *batch;
> +
> +	dst = gem_handle_to_libdrm_bo(data->bufmgr, data->drm_fd, "", handle);
> +	igt_assert(dst);
> +
> +	batch = intel_batchbuffer_alloc(data->bufmgr, data->devid);
> +	igt_assert(batch);
> +
> +	/* add the reloc to make sure the kernel will think we write to dst */
> +	BEGIN_BATCH(4);
> +	OUT_BATCH(MI_BATCH_BUFFER_END);
> +	OUT_BATCH(MI_NOOP);
> +	OUT_RELOC(dst, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
> +	OUT_BATCH(MI_NOOP);
> +	ADVANCE_BATCH();
> +
> +	intel_batchbuffer_flush_with_context(batch, context);
> +	intel_batchbuffer_free(batch);
> +
> +	drm_intel_bo_unreference(dst);
> +}
> +
> +static unsigned int total_fence_pin_count(void)
> +{
> +	unsigned int pinned = 0;
> +	char buf[128];
> +	FILE *f;
> +
> +	f = igt_debugfs_fopen("i915_gem_fence_regs", "r");
> +	igt_assert(f);
> +
> +	for (;;) {
> +		unsigned int pin_count = 0;
> +		char *ptr;
> +
> +		ptr = fgets(buf, sizeof(buf), f);
> +		if (!ptr)
> +			break;
> +
> +		if (sscanf(ptr, "Fence %*u, pin count = %u", &pin_count) == 1)
> +			pinned += pin_count;
> +	}
> +
> +	fclose(f);
> +
> +	return pinned;
> +}
> +
> +static bool run_single_test(data_t *data, enum pipe pipe, igt_output_t *output)
> +{
> +	igt_display_t *display = &data->display;
> +	drmModeModeInfo *mode;
> +	igt_plane_t *primary;
> +	unsigned int pin_count;
> +	struct igt_fb fb[2];
> +	int i;
> +
> +	igt_output_set_pipe(output, pipe);
> +	igt_display_commit(display);
> +
> +	if (!output->valid) {
> +		igt_output_set_pipe(output, PIPE_ANY);
> +		igt_display_commit(display);
> +		return false;
> +	}
> +
> +	mode = igt_output_get_mode(output);
> +	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
> +
> +	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> +			    DRM_FORMAT_XRGB8888,
> +			    true, /* need a fence so must be tiled */
> +			    0.0, 0.0, 0.0,
> +			    &fb[0]);
> +	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> +			    DRM_FORMAT_XRGB8888,
> +			    true, /* need a fence so must be tiled */
> +			    0.0, 0.0, 0.0,
> +			    &fb[1]);
> +
> +	igt_plane_set_fb(primary, &fb[0]);
> +	igt_display_commit(display);
> +
> +	/* sample the fence pin_counts now that we have one scanout buffer pinned */
> +	pin_count = total_fence_pin_count();
> +
> +	for (i = 0; i < 64; i++) {
> +		drm_intel_context *ctx;
> +
> +		/*
> +		 * Link fb.gem_handle to the ppgtt vm of ctx so that the context
> +		 * destruction will unbind the obj from the ppgtt vm in question.
> +		 */
> +		ctx = drm_intel_gem_context_create(data->bufmgr);
> +		igt_assert(ctx);
> +		exec_nop(data, fb[i&1].gem_handle, ctx);
> +		drm_intel_gem_context_destroy(ctx);
> +
> +		/* Force a context switch to make sure ctx gets destroyed for real. */
> +		exec_nop(data, fb[i&1].gem_handle, NULL);
> +
> +		gem_sync(data->drm_fd, fb[i&1].gem_handle);
> +
> +		/*
> +		 * Pin the new buffer and unpin the old buffer from display. If
> +		 * the kernel is buggy the ppgtt unbind will have dropped the
> +		 * fence for the old buffer, and now the display code will try
> +		 * to unpin only to find no fence there. So the pin_count will leak.
> +		 */
> +		igt_plane_set_fb(primary, &fb[!(i&1)]);
> +		igt_display_commit(display);
> +
> +		/*
> +		 * We always have exactly one scanout buffer, so the fence pin_count
> +		 * should remain static. Fail the test if we detect a leak.
> +		 */
> +		igt_assert(pin_count == total_fence_pin_count());
> +
> +		printf(".");
> +		fflush(stdout);
> +	}
> +
> +	igt_plane_set_fb(primary, NULL);
> +	igt_output_set_pipe(output, PIPE_ANY);
> +	igt_display_commit(display);
> +
> +	igt_remove_fb(data->drm_fd, &fb[1]);
> +	igt_remove_fb(data->drm_fd, &fb[0]);
> +
> +	printf("\n");
> +
> +	return true;
> +}
> +
> +static void run_test(data_t *data)
> +{
> +	igt_display_t *display = &data->display;
> +	igt_output_t *output;
> +	enum pipe p;
> +
> +	for_each_connected_output(display, output) {
> +		for (p = 0; p < igt_display_get_n_pipes(display); p++) {
> +			if (run_single_test(data, p, output))
> +				return; /* one time ought to be enough */
> +		}
> +	}
> +
> +	igt_skip("no valid crtc/connector combinations found\n");
> +}
> +
> +igt_simple_main
> +{
> +	drm_intel_context *ctx;
> +	data_t data = {};
> +
> +	igt_skip_on_simulation();
> +
> +	data.drm_fd = drm_open_any();
> +
> +	data.devid = intel_get_drm_devid(data.drm_fd);
> +
> +	igt_set_vt_graphics_mode();
> +
> +	data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096);
> +	igt_assert(data.bufmgr);
> +	drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
> +
> +	igt_display_init(&data.display, data.drm_fd);
> +
> +	ctx = drm_intel_gem_context_create(data.bufmgr);
> +	igt_require(ctx);
> +	drm_intel_gem_context_destroy(ctx);
> +
> +	run_test(&data);
> +
> +	drm_intel_bufmgr_destroy(data.bufmgr);
> +	igt_display_fini(&data.display);
> +}
> -- 
> 1.8.3.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH igt] tests/kms_fence_pin_leak: Exercise full ppgtt fence pin_count leak in the kernel

[Ville Syrjälä]

On Mon, May 12, 2014 at 08:34:07PM +0200, Daniel Vetter wrote:
> On Mon, May 12, 2014 at 08:46:24PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The kernel full ppgtt support has a bug where it can drop a pinned
> > fence to the floor, hence we leak the pin_count as the subsequent
> > fence unpin becomes a nop. We can trigger it easily by unbinding a
> > buffer from a ppgtt address space while the buffer is simultaneosly
> > being used for scanout.
> > 
> > Monitor i915_gem_fence_regs to detect the leak while performing
> > the required set of operations in a loop.
> 
> do we really need that part? I've hoped just leaking enough fences until
> the kernel blows up should be sufficient.

Sure if you want to let it run for >5 years. That's how long it would
take to make the pin_count overflow on my IVB machine, assuming you hit
the bug on every iteration of the loop, which for some reason doesn't
seem to be the case here.

Another option would be to trick the the pin_count to leak for every
fence registers. Then we wouldn't be able to find a new fence and
we'd get -EDEADLK from i915_find_fence_reg(). If I try to make sure
all  fences have an object associated with them, it should always
pick a fence with pin_count==0, so it seems like this approach should
work. I'll work on a it a bit and we'll see...

> Generally I'm vary of using debugfs in new tests a bit since we
> essentially then lock down that debugfs file as abi. And since no one
> easily notices when a test stops catching a bug that's usually bad. Hence
> also why I want basic testcases for debugfs infrastructure to make sure it
> keeps on working.
> -Daniel
> 
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  tests/Makefile.sources     |   1 +
> >  tests/kms_fence_pin_leak.c | 224 +++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 225 insertions(+)
> >  create mode 100644 tests/kms_fence_pin_leak.c
> > 
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > index 5d5dc46..393c4a2 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -60,6 +60,7 @@ TESTS_progs_M = \
> >  	kms_addfb \
> >  	kms_cursor_crc \
> >  	kms_fbc_crc \
> > +	kms_fence_pin_leak \
> >  	kms_flip \
> >  	kms_flip_tiling \
> >  	kms_pipe_crc_basic \
> > diff --git a/tests/kms_fence_pin_leak.c b/tests/kms_fence_pin_leak.c
> > new file mode 100644
> > index 0000000..f299212
> > --- /dev/null
> > +++ b/tests/kms_fence_pin_leak.c
> > @@ -0,0 +1,224 @@
> > +/*
> > + * Copyright © 2014 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + */
> > +
> > +#include <errno.h>
> > +#include <limits.h>
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +
> > +#include "drmtest.h"
> > +#include "igt_debugfs.h"
> > +#include "igt_kms.h"
> > +#include "ioctl_wrappers.h"
> > +#include "intel_chipset.h"
> > +
> > +typedef struct {
> > +	int drm_fd;
> > +	uint32_t devid;
> > +	drm_intel_bufmgr *bufmgr;
> > +	igt_display_t display;
> > +} data_t;
> > +
> > +static void exec_nop(data_t *data, uint32_t handle, drm_intel_context *context)
> > +{
> > +	drm_intel_bo *dst;
> > +	struct intel_batchbuffer *batch;
> > +
> > +	dst = gem_handle_to_libdrm_bo(data->bufmgr, data->drm_fd, "", handle);
> > +	igt_assert(dst);
> > +
> > +	batch = intel_batchbuffer_alloc(data->bufmgr, data->devid);
> > +	igt_assert(batch);
> > +
> > +	/* add the reloc to make sure the kernel will think we write to dst */
> > +	BEGIN_BATCH(4);
> > +	OUT_BATCH(MI_BATCH_BUFFER_END);
> > +	OUT_BATCH(MI_NOOP);
> > +	OUT_RELOC(dst, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
> > +	OUT_BATCH(MI_NOOP);
> > +	ADVANCE_BATCH();
> > +
> > +	intel_batchbuffer_flush_with_context(batch, context);
> > +	intel_batchbuffer_free(batch);
> > +
> > +	drm_intel_bo_unreference(dst);
> > +}
> > +
> > +static unsigned int total_fence_pin_count(void)
> > +{
> > +	unsigned int pinned = 0;
> > +	char buf[128];
> > +	FILE *f;
> > +
> > +	f = igt_debugfs_fopen("i915_gem_fence_regs", "r");
> > +	igt_assert(f);
> > +
> > +	for (;;) {
> > +		unsigned int pin_count = 0;
> > +		char *ptr;
> > +
> > +		ptr = fgets(buf, sizeof(buf), f);
> > +		if (!ptr)
> > +			break;
> > +
> > +		if (sscanf(ptr, "Fence %*u, pin count = %u", &pin_count) == 1)
> > +			pinned += pin_count;
> > +	}
> > +
> > +	fclose(f);
> > +
> > +	return pinned;
> > +}
> > +
> > +static bool run_single_test(data_t *data, enum pipe pipe, igt_output_t *output)
> > +{
> > +	igt_display_t *display = &data->display;
> > +	drmModeModeInfo *mode;
> > +	igt_plane_t *primary;
> > +	unsigned int pin_count;
> > +	struct igt_fb fb[2];
> > +	int i;
> > +
> > +	igt_output_set_pipe(output, pipe);
> > +	igt_display_commit(display);
> > +
> > +	if (!output->valid) {
> > +		igt_output_set_pipe(output, PIPE_ANY);
> > +		igt_display_commit(display);
> > +		return false;
> > +	}
> > +
> > +	mode = igt_output_get_mode(output);
> > +	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
> > +
> > +	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> > +			    DRM_FORMAT_XRGB8888,
> > +			    true, /* need a fence so must be tiled */
> > +			    0.0, 0.0, 0.0,
> > +			    &fb[0]);
> > +	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> > +			    DRM_FORMAT_XRGB8888,
> > +			    true, /* need a fence so must be tiled */
> > +			    0.0, 0.0, 0.0,
> > +			    &fb[1]);
> > +
> > +	igt_plane_set_fb(primary, &fb[0]);
> > +	igt_display_commit(display);
> > +
> > +	/* sample the fence pin_counts now that we have one scanout buffer pinned */
> > +	pin_count = total_fence_pin_count();
> > +
> > +	for (i = 0; i < 64; i++) {
> > +		drm_intel_context *ctx;
> > +
> > +		/*
> > +		 * Link fb.gem_handle to the ppgtt vm of ctx so that the context
> > +		 * destruction will unbind the obj from the ppgtt vm in question.
> > +		 */
> > +		ctx = drm_intel_gem_context_create(data->bufmgr);
> > +		igt_assert(ctx);
> > +		exec_nop(data, fb[i&1].gem_handle, ctx);
> > +		drm_intel_gem_context_destroy(ctx);
> > +
> > +		/* Force a context switch to make sure ctx gets destroyed for real. */
> > +		exec_nop(data, fb[i&1].gem_handle, NULL);
> > +
> > +		gem_sync(data->drm_fd, fb[i&1].gem_handle);
> > +
> > +		/*
> > +		 * Pin the new buffer and unpin the old buffer from display. If
> > +		 * the kernel is buggy the ppgtt unbind will have dropped the
> > +		 * fence for the old buffer, and now the display code will try
> > +		 * to unpin only to find no fence there. So the pin_count will leak.
> > +		 */
> > +		igt_plane_set_fb(primary, &fb[!(i&1)]);
> > +		igt_display_commit(display);
> > +
> > +		/*
> > +		 * We always have exactly one scanout buffer, so the fence pin_count
> > +		 * should remain static. Fail the test if we detect a leak.
> > +		 */
> > +		igt_assert(pin_count == total_fence_pin_count());
> > +
> > +		printf(".");
> > +		fflush(stdout);
> > +	}
> > +
> > +	igt_plane_set_fb(primary, NULL);
> > +	igt_output_set_pipe(output, PIPE_ANY);
> > +	igt_display_commit(display);
> > +
> > +	igt_remove_fb(data->drm_fd, &fb[1]);
> > +	igt_remove_fb(data->drm_fd, &fb[0]);
> > +
> > +	printf("\n");
> > +
> > +	return true;
> > +}
> > +
> > +static void run_test(data_t *data)
> > +{
> > +	igt_display_t *display = &data->display;
> > +	igt_output_t *output;
> > +	enum pipe p;
> > +
> > +	for_each_connected_output(display, output) {
> > +		for (p = 0; p < igt_display_get_n_pipes(display); p++) {
> > +			if (run_single_test(data, p, output))
> > +				return; /* one time ought to be enough */
> > +		}
> > +	}
> > +
> > +	igt_skip("no valid crtc/connector combinations found\n");
> > +}
> > +
> > +igt_simple_main
> > +{
> > +	drm_intel_context *ctx;
> > +	data_t data = {};
> > +
> > +	igt_skip_on_simulation();
> > +
> > +	data.drm_fd = drm_open_any();
> > +
> > +	data.devid = intel_get_drm_devid(data.drm_fd);
> > +
> > +	igt_set_vt_graphics_mode();
> > +
> > +	data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096);
> > +	igt_assert(data.bufmgr);
> > +	drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
> > +
> > +	igt_display_init(&data.display, data.drm_fd);
> > +
> > +	ctx = drm_intel_gem_context_create(data.bufmgr);
> > +	igt_require(ctx);
> > +	drm_intel_gem_context_destroy(ctx);
> > +
> > +	run_test(&data);
> > +
> > +	drm_intel_bufmgr_destroy(data.bufmgr);
> > +	igt_display_fini(&data.display);
> > +}
> > -- 
> > 1.8.3.2
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH igt] tests/kms_fence_pin_leak: Exercise full ppgtt fence pin_count leak in the kernel

[Daniel Vetter]

On Tue, May 13, 2014 at 11:24:48AM +0300, Ville Syrjälä wrote:
> On Mon, May 12, 2014 at 08:34:07PM +0200, Daniel Vetter wrote:
> > On Mon, May 12, 2014 at 08:46:24PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The kernel full ppgtt support has a bug where it can drop a pinned
> > > fence to the floor, hence we leak the pin_count as the subsequent
> > > fence unpin becomes a nop. We can trigger it easily by unbinding a
> > > buffer from a ppgtt address space while the buffer is simultaneosly
> > > being used for scanout.
> > > 
> > > Monitor i915_gem_fence_regs to detect the leak while performing
> > > the required set of operations in a loop.
> > 
> > do we really need that part? I've hoped just leaking enough fences until
> > the kernel blows up should be sufficient.
> 
> Sure if you want to let it run for >5 years. That's how long it would
> take to make the pin_count overflow on my IVB machine, assuming you hit
> the bug on every iteration of the loop, which for some reason doesn't
> seem to be the case here.

Yeah, that's only feasible if we restrict the pin count like we already do
with obj->pin_count.

> Another option would be to trick the the pin_count to leak for every
> fence registers. Then we wouldn't be able to find a new fence and
> we'd get -EDEADLK from i915_find_fence_reg(). If I try to make sure
> all  fences have an object associated with them, it should always
> pick a fence with pin_count==0, so it seems like this approach should
> work. I'll work on a it a bit and we'll see...

That's actually what I've had in mind ;-)

Aside: I'm of course not against using more of debugfs, so if it's too
much work I'm totally ok with this one. But generally I just prefer
black-box tests with as little introspection through debugfs as possible.

Thanks for doing this.
-Daniel

> 
> > Generally I'm vary of using debugfs in new tests a bit since we
> > essentially then lock down that debugfs file as abi. And since no one
> > easily notices when a test stops catching a bug that's usually bad. Hence
> > also why I want basic testcases for debugfs infrastructure to make sure it
> > keeps on working.
> > -Daniel
> > 
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  tests/Makefile.sources     |   1 +
> > >  tests/kms_fence_pin_leak.c | 224 +++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 225 insertions(+)
> > >  create mode 100644 tests/kms_fence_pin_leak.c
> > > 
> > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > > index 5d5dc46..393c4a2 100644
> > > --- a/tests/Makefile.sources
> > > +++ b/tests/Makefile.sources
> > > @@ -60,6 +60,7 @@ TESTS_progs_M = \
> > >  	kms_addfb \
> > >  	kms_cursor_crc \
> > >  	kms_fbc_crc \
> > > +	kms_fence_pin_leak \
> > >  	kms_flip \
> > >  	kms_flip_tiling \
> > >  	kms_pipe_crc_basic \
> > > diff --git a/tests/kms_fence_pin_leak.c b/tests/kms_fence_pin_leak.c
> > > new file mode 100644
> > > index 0000000..f299212
> > > --- /dev/null
> > > +++ b/tests/kms_fence_pin_leak.c
> > > @@ -0,0 +1,224 @@
> > > +/*
> > > + * Copyright © 2014 Intel Corporation
> > > + *
> > > + * Permission is hereby granted, free of charge, to any person obtaining a
> > > + * copy of this software and associated documentation files (the "Software"),
> > > + * to deal in the Software without restriction, including without limitation
> > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > > + * and/or sell copies of the Software, and to permit persons to whom the
> > > + * Software is furnished to do so, subject to the following conditions:
> > > + *
> > > + * The above copyright notice and this permission notice (including the next
> > > + * paragraph) shall be included in all copies or substantial portions of the
> > > + * Software.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > > + * IN THE SOFTWARE.
> > > + *
> > > + */
> > > +
> > > +#include <errno.h>
> > > +#include <limits.h>
> > > +#include <stdbool.h>
> > > +#include <stdio.h>
> > > +#include <string.h>
> > > +
> > > +#include "drmtest.h"
> > > +#include "igt_debugfs.h"
> > > +#include "igt_kms.h"
> > > +#include "ioctl_wrappers.h"
> > > +#include "intel_chipset.h"
> > > +
> > > +typedef struct {
> > > +	int drm_fd;
> > > +	uint32_t devid;
> > > +	drm_intel_bufmgr *bufmgr;
> > > +	igt_display_t display;
> > > +} data_t;
> > > +
> > > +static void exec_nop(data_t *data, uint32_t handle, drm_intel_context *context)
> > > +{
> > > +	drm_intel_bo *dst;
> > > +	struct intel_batchbuffer *batch;
> > > +
> > > +	dst = gem_handle_to_libdrm_bo(data->bufmgr, data->drm_fd, "", handle);
> > > +	igt_assert(dst);
> > > +
> > > +	batch = intel_batchbuffer_alloc(data->bufmgr, data->devid);
> > > +	igt_assert(batch);
> > > +
> > > +	/* add the reloc to make sure the kernel will think we write to dst */
> > > +	BEGIN_BATCH(4);
> > > +	OUT_BATCH(MI_BATCH_BUFFER_END);
> > > +	OUT_BATCH(MI_NOOP);
> > > +	OUT_RELOC(dst, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
> > > +	OUT_BATCH(MI_NOOP);
> > > +	ADVANCE_BATCH();
> > > +
> > > +	intel_batchbuffer_flush_with_context(batch, context);
> > > +	intel_batchbuffer_free(batch);
> > > +
> > > +	drm_intel_bo_unreference(dst);
> > > +}
> > > +
> > > +static unsigned int total_fence_pin_count(void)
> > > +{
> > > +	unsigned int pinned = 0;
> > > +	char buf[128];
> > > +	FILE *f;
> > > +
> > > +	f = igt_debugfs_fopen("i915_gem_fence_regs", "r");
> > > +	igt_assert(f);
> > > +
> > > +	for (;;) {
> > > +		unsigned int pin_count = 0;
> > > +		char *ptr;
> > > +
> > > +		ptr = fgets(buf, sizeof(buf), f);
> > > +		if (!ptr)
> > > +			break;
> > > +
> > > +		if (sscanf(ptr, "Fence %*u, pin count = %u", &pin_count) == 1)
> > > +			pinned += pin_count;
> > > +	}
> > > +
> > > +	fclose(f);
> > > +
> > > +	return pinned;
> > > +}
> > > +
> > > +static bool run_single_test(data_t *data, enum pipe pipe, igt_output_t *output)
> > > +{
> > > +	igt_display_t *display = &data->display;
> > > +	drmModeModeInfo *mode;
> > > +	igt_plane_t *primary;
> > > +	unsigned int pin_count;
> > > +	struct igt_fb fb[2];
> > > +	int i;
> > > +
> > > +	igt_output_set_pipe(output, pipe);
> > > +	igt_display_commit(display);
> > > +
> > > +	if (!output->valid) {
> > > +		igt_output_set_pipe(output, PIPE_ANY);
> > > +		igt_display_commit(display);
> > > +		return false;
> > > +	}
> > > +
> > > +	mode = igt_output_get_mode(output);
> > > +	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
> > > +
> > > +	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> > > +			    DRM_FORMAT_XRGB8888,
> > > +			    true, /* need a fence so must be tiled */
> > > +			    0.0, 0.0, 0.0,
> > > +			    &fb[0]);
> > > +	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> > > +			    DRM_FORMAT_XRGB8888,
> > > +			    true, /* need a fence so must be tiled */
> > > +			    0.0, 0.0, 0.0,
> > > +			    &fb[1]);
> > > +
> > > +	igt_plane_set_fb(primary, &fb[0]);
> > > +	igt_display_commit(display);
> > > +
> > > +	/* sample the fence pin_counts now that we have one scanout buffer pinned */
> > > +	pin_count = total_fence_pin_count();
> > > +
> > > +	for (i = 0; i < 64; i++) {
> > > +		drm_intel_context *ctx;
> > > +
> > > +		/*
> > > +		 * Link fb.gem_handle to the ppgtt vm of ctx so that the context
> > > +		 * destruction will unbind the obj from the ppgtt vm in question.
> > > +		 */
> > > +		ctx = drm_intel_gem_context_create(data->bufmgr);
> > > +		igt_assert(ctx);
> > > +		exec_nop(data, fb[i&1].gem_handle, ctx);
> > > +		drm_intel_gem_context_destroy(ctx);
> > > +
> > > +		/* Force a context switch to make sure ctx gets destroyed for real. */
> > > +		exec_nop(data, fb[i&1].gem_handle, NULL);
> > > +
> > > +		gem_sync(data->drm_fd, fb[i&1].gem_handle);
> > > +
> > > +		/*
> > > +		 * Pin the new buffer and unpin the old buffer from display. If
> > > +		 * the kernel is buggy the ppgtt unbind will have dropped the
> > > +		 * fence for the old buffer, and now the display code will try
> > > +		 * to unpin only to find no fence there. So the pin_count will leak.
> > > +		 */
> > > +		igt_plane_set_fb(primary, &fb[!(i&1)]);
> > > +		igt_display_commit(display);
> > > +
> > > +		/*
> > > +		 * We always have exactly one scanout buffer, so the fence pin_count
> > > +		 * should remain static. Fail the test if we detect a leak.
> > > +		 */
> > > +		igt_assert(pin_count == total_fence_pin_count());
> > > +
> > > +		printf(".");
> > > +		fflush(stdout);
> > > +	}
> > > +
> > > +	igt_plane_set_fb(primary, NULL);
> > > +	igt_output_set_pipe(output, PIPE_ANY);
> > > +	igt_display_commit(display);
> > > +
> > > +	igt_remove_fb(data->drm_fd, &fb[1]);
> > > +	igt_remove_fb(data->drm_fd, &fb[0]);
> > > +
> > > +	printf("\n");
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +static void run_test(data_t *data)
> > > +{
> > > +	igt_display_t *display = &data->display;
> > > +	igt_output_t *output;
> > > +	enum pipe p;
> > > +
> > > +	for_each_connected_output(display, output) {
> > > +		for (p = 0; p < igt_display_get_n_pipes(display); p++) {
> > > +			if (run_single_test(data, p, output))
> > > +				return; /* one time ought to be enough */
> > > +		}
> > > +	}
> > > +
> > > +	igt_skip("no valid crtc/connector combinations found\n");
> > > +}
> > > +
> > > +igt_simple_main
> > > +{
> > > +	drm_intel_context *ctx;
> > > +	data_t data = {};
> > > +
> > > +	igt_skip_on_simulation();
> > > +
> > > +	data.drm_fd = drm_open_any();
> > > +
> > > +	data.devid = intel_get_drm_devid(data.drm_fd);
> > > +
> > > +	igt_set_vt_graphics_mode();
> > > +
> > > +	data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096);
> > > +	igt_assert(data.bufmgr);
> > > +	drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
> > > +
> > > +	igt_display_init(&data.display, data.drm_fd);
> > > +
> > > +	ctx = drm_intel_gem_context_create(data.bufmgr);
> > > +	igt_require(ctx);
> > > +	drm_intel_gem_context_destroy(ctx);
> > > +
> > > +	run_test(&data);
> > > +
> > > +	drm_intel_bufmgr_destroy(data.bufmgr);
> > > +	igt_display_fini(&data.display);
> > > +}
> > > -- 
> > > 1.8.3.2
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v2 igt] tests/kms_fence_pin_leak: Exercise full ppgtt fence pin_count leak in the kernel

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The kernel full ppgtt support has a bug where it can drop a pinned
fence to the floor, hence we leak the pin_count as the subsequent
fence unpin becomes a nop. We can trigger it easily by unbinding a
buffer from a ppgtt address space while the buffer is simultaneosly
being used for scanout.

Make the kernel into leaking the fence pin_count and trick it into
picking a new fence register for the next scanout buffer. Looping like
this for a while we leak the pin_count for all fence registers after
which the kernel can no longer find a new fence register when it needs
one. As a result we get back a SIGBUS from the GTT mmap access.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 tests/Makefile.sources     |   1 +
 tests/kms_fence_pin_leak.c | 239 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 240 insertions(+)
 create mode 100644 tests/kms_fence_pin_leak.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 5d5dc46..393c4a2 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -60,6 +60,7 @@ TESTS_progs_M = \
 	kms_addfb \
 	kms_cursor_crc \
 	kms_fbc_crc \
+	kms_fence_pin_leak \
 	kms_flip \
 	kms_flip_tiling \
 	kms_pipe_crc_basic \
diff --git a/tests/kms_fence_pin_leak.c b/tests/kms_fence_pin_leak.c
new file mode 100644
index 0000000..d3ca132
--- /dev/null
+++ b/tests/kms_fence_pin_leak.c
@@ -0,0 +1,239 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include <errno.h>
+#include <limits.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <string.h>
+
+#include "drmtest.h"
+#include "igt_debugfs.h"
+#include "igt_kms.h"
+#include "ioctl_wrappers.h"
+#include "intel_chipset.h"
+
+typedef struct {
+	int drm_fd;
+	uint32_t devid;
+	drm_intel_bufmgr *bufmgr;
+	igt_display_t display;
+	drm_intel_bo *bos[64]; /* >= num fence registers */
+} data_t;
+
+static void exec_nop(data_t *data, uint32_t handle, drm_intel_context *context)
+{
+	drm_intel_bo *dst;
+	struct intel_batchbuffer *batch;
+
+	dst = gem_handle_to_libdrm_bo(data->bufmgr, data->drm_fd, "", handle);
+	igt_assert(dst);
+
+	batch = intel_batchbuffer_alloc(data->bufmgr, data->devid);
+	igt_assert(batch);
+
+	/* add the reloc to make sure the kernel will think we write to dst */
+	BEGIN_BATCH(4);
+	OUT_BATCH(MI_BATCH_BUFFER_END);
+	OUT_BATCH(MI_NOOP);
+	OUT_RELOC(dst, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
+	OUT_BATCH(MI_NOOP);
+	ADVANCE_BATCH();
+
+	intel_batchbuffer_flush_with_context(batch, context);
+	intel_batchbuffer_free(batch);
+
+	drm_intel_bo_unreference(dst);
+}
+
+static void alloc_fence_objs(data_t *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(data->bos); i++) {
+		drm_intel_bo *bo;
+
+		bo = drm_intel_bo_alloc(data->bufmgr, "fence bo", 4096, 4096);
+		igt_assert(bo);
+		gem_set_tiling(data->drm_fd, bo->handle, I915_TILING_X, 512);
+
+		data->bos[i] = bo;
+	}
+}
+
+static void touch_fences(data_t *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(data->bos); i++) {
+		uint32_t handle = data->bos[i]->handle;
+		void *ptr;
+
+		ptr = gem_mmap__gtt(data->drm_fd, handle, 4096, PROT_WRITE);
+		gem_set_domain(data->drm_fd, handle, I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
+		memset(ptr, 0, 4);
+		munmap(ptr, 4096);
+	}
+}
+
+static void free_fence_objs(data_t *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(data->bos); i++)
+		drm_intel_bo_unreference(data->bos[i]);
+}
+
+static bool run_single_test(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+	igt_display_t *display = &data->display;
+	drmModeModeInfo *mode;
+	igt_plane_t *primary;
+	struct igt_fb fb[2];
+	int i;
+
+	igt_output_set_pipe(output, pipe);
+	igt_display_commit(display);
+
+	if (!output->valid) {
+		igt_output_set_pipe(output, PIPE_ANY);
+		igt_display_commit(display);
+		return false;
+	}
+
+	mode = igt_output_get_mode(output);
+	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
+
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+			    DRM_FORMAT_XRGB8888,
+			    true, /* need a fence so must be tiled */
+			    0.0, 0.0, 0.0,
+			    &fb[0]);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+			    DRM_FORMAT_XRGB8888,
+			    true, /* need a fence so must be tiled */
+			    0.0, 0.0, 0.0,
+			    &fb[1]);
+
+	igt_plane_set_fb(primary, &fb[0]);
+	igt_display_commit(display);
+
+	for (i = 0; i < 64; i++) {
+		drm_intel_context *ctx;
+
+		/*
+		 * Link fb.gem_handle to the ppgtt vm of ctx so that the context
+		 * destruction will unbind the obj from the ppgtt vm in question.
+		 */
+		ctx = drm_intel_gem_context_create(data->bufmgr);
+		igt_assert(ctx);
+		exec_nop(data, fb[i&1].gem_handle, ctx);
+		drm_intel_gem_context_destroy(ctx);
+
+		/* Force a context switch to make sure ctx gets destroyed for real. */
+		exec_nop(data, fb[i&1].gem_handle, NULL);
+
+		gem_sync(data->drm_fd, fb[i&1].gem_handle);
+
+		/*
+		 * Make only the current fb has a fence and
+		 * the next fb will pick a new fence. Assuming
+		 * all fences are associated with an object, the
+		 * kernel will always pick a fence with pin_count==0.
+		 */
+		touch_fences(data);
+
+		/*
+		 * Pin the new buffer and unpin the old buffer from display. If
+		 * the kernel is buggy the ppgtt unbind will have dropped the
+		 * fence for the old buffer, and now the display code will try
+		 * to unpin only to find no fence there. So the pin_count will leak.
+		 */
+		igt_plane_set_fb(primary, &fb[!(i&1)]);
+		igt_display_commit(display);
+
+		printf(".");
+		fflush(stdout);
+	}
+
+	igt_plane_set_fb(primary, NULL);
+	igt_output_set_pipe(output, PIPE_ANY);
+	igt_display_commit(display);
+
+	igt_remove_fb(data->drm_fd, &fb[1]);
+	igt_remove_fb(data->drm_fd, &fb[0]);
+
+	printf("\n");
+
+	return true;
+}
+
+static void run_test(data_t *data)
+{
+	igt_display_t *display = &data->display;
+	igt_output_t *output;
+	enum pipe p;
+
+	for_each_connected_output(display, output) {
+		for (p = 0; p < igt_display_get_n_pipes(display); p++) {
+			if (run_single_test(data, p, output))
+				return; /* one time ought to be enough */
+		}
+	}
+
+	igt_skip("no valid crtc/connector combinations found\n");
+}
+
+igt_simple_main
+{
+	drm_intel_context *ctx;
+	data_t data = {};
+
+	igt_skip_on_simulation();
+
+	data.drm_fd = drm_open_any();
+
+	data.devid = intel_get_drm_devid(data.drm_fd);
+
+	igt_set_vt_graphics_mode();
+
+	data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096);
+	igt_assert(data.bufmgr);
+	drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
+
+	igt_display_init(&data.display, data.drm_fd);
+
+	ctx = drm_intel_gem_context_create(data.bufmgr);
+	igt_require(ctx);
+	drm_intel_gem_context_destroy(ctx);
+
+	alloc_fence_objs(&data);
+
+	run_test(&data);
+
+	free_fence_objs(&data);
+
+	drm_intel_bufmgr_destroy(data.bufmgr);
+	igt_display_fini(&data.display);
+}
-- 
1.8.3.2

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

Re: [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma

[Ville Syrjälä]

On Fri, Feb 14, 2014 at 02:06:07PM +0100, Daniel Vetter wrote:
> Otherwise we end up tearing down fences when e.g. the client quits
> way too early. Might or might not fix a fence pin_count BUG Ville has
> reported.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

For patches 1 and 3:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 675ad96a43e1..fa00b26a9cf7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2731,12 +2731,14 @@ int i915_vma_unbind(struct i915_vma *vma)
>  	 * cause memory corruption through use-after-free.
>  	 */
>  
> -	i915_gem_object_finish_gtt(obj);
> +	if (i915_is_ggtt(vma->vm)) {
> +		i915_gem_object_finish_gtt(obj);
>  
> -	/* release the fence reg _after_ flushing */
> -	ret = i915_gem_object_put_fence(obj);
> -	if (ret)
> -		return ret;
> +		/* release the fence reg _after_ flushing */
> +		ret = i915_gem_object_put_fence(obj);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	trace_i915_vma_unbind(vma);
>  
> -- 
> 1.8.5.2

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma

[Daniel Vetter]

On Wed, May 14, 2014 at 07:14:13PM +0300, Ville Syrjälä wrote:
> On Fri, Feb 14, 2014 at 02:06:07PM +0100, Daniel Vetter wrote:
> > Otherwise we end up tearing down fences when e.g. the client quits
> > way too early. Might or might not fix a fence pin_count BUG Ville has
> > reported.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> For patches 1 and 3:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Merged, thanks for the review and writing the testcase.
-Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 675ad96a43e1..fa00b26a9cf7 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2731,12 +2731,14 @@ int i915_vma_unbind(struct i915_vma *vma)
> >  	 * cause memory corruption through use-after-free.
> >  	 */
> >  
> > -	i915_gem_object_finish_gtt(obj);
> > +	if (i915_is_ggtt(vma->vm)) {
> > +		i915_gem_object_finish_gtt(obj);
> >  
> > -	/* release the fence reg _after_ flushing */
> > -	ret = i915_gem_object_put_fence(obj);
> > -	if (ret)
> > -		return ret;
> > +		/* release the fence reg _after_ flushing */
> > +		ret = i915_gem_object_put_fence(obj);
> > +		if (ret)
> > +			return ret;
> > +	}
> >  
> >  	trace_i915_vma_unbind(vma);
> >  
> > -- 
> > 1.8.5.2
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch