* [PATCH 1/2] drm/i915: Don't drop pinned fences @ 2014-01-27 20:49 Daniel Vetter 2014-01-27 20:49 ` [PATCH 2/2] drm/i915: Only do gtt cleanup in vma_unbind for the global vma Daniel Vetter 2014-01-27 21:26 ` [PATCH 1/2] drm/i915: Don't drop pinned fences Chris Wilson 0 siblings, 2 replies; 19+ messages in thread From: Daniel Vetter @ 2014-01-27 20:49 UTC (permalink / raw) To: Intel Graphics Development; +Cc: 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/2] drm/i915: Only do gtt cleanup in vma_unbind for the global vma 2014-01-27 20:49 [PATCH 1/2] drm/i915: Don't drop pinned fences Daniel Vetter @ 2014-01-27 20:49 ` Daniel Vetter 2014-01-27 21:26 ` [PATCH 1/2] drm/i915: Don't drop pinned fences Chris Wilson 1 sibling, 0 replies; 19+ messages in thread From: Daniel Vetter @ 2014-01-27 20:49 UTC (permalink / raw) To: Intel Graphics Development; +Cc: 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't drop pinned fences 2014-01-27 20:49 [PATCH 1/2] drm/i915: Don't drop pinned fences Daniel Vetter 2014-01-27 20:49 ` [PATCH 2/2] drm/i915: Only do gtt cleanup in vma_unbind for the global vma Daniel Vetter @ 2014-01-27 21:26 ` Chris Wilson 2014-01-27 21:41 ` Daniel Vetter 1 sibling, 1 reply; 19+ messages in thread From: Chris Wilson @ 2014-01-27 21:26 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't drop pinned fences 2014-01-27 21:26 ` [PATCH 1/2] drm/i915: Don't drop pinned fences Chris Wilson @ 2014-01-27 21:41 ` Daniel Vetter 2014-01-28 12:40 ` Ville Syrjälä 0 siblings, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2014-01-27 21:41 UTC (permalink / raw) To: Chris Wilson, Daniel Vetter, Intel Graphics Development 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't drop pinned fences 2014-01-27 21:41 ` Daniel Vetter @ 2014-01-28 12:40 ` Ville Syrjälä 2014-02-14 13:06 ` [PATCH 1/3] " Daniel Vetter 0 siblings, 1 reply; 19+ messages in thread From: Ville Syrjälä @ 2014-01-28 12:40 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/3] drm/i915: Don't drop pinned fences 2014-01-28 12:40 ` Ville Syrjälä @ 2014-02-14 13:06 ` Daniel Vetter 2014-02-14 13:06 ` [PATCH 2/3] drm/i915: tune down user-triggerable dmesg noise in the cursor/overlay code Daniel Vetter 2014-02-14 13:06 ` [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma Daniel Vetter 0 siblings, 2 replies; 19+ messages in thread From: Daniel Vetter @ 2014-02-14 13:06 UTC (permalink / raw) To: Intel Graphics Development; +Cc: 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/3] drm/i915: tune down user-triggerable dmesg noise in the cursor/overlay code 2014-02-14 13:06 ` [PATCH 1/3] " Daniel Vetter @ 2014-02-14 13:06 ` Daniel Vetter 2014-02-14 17:29 ` Damien Lespiau 2014-02-14 13:06 ` [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma Daniel Vetter 1 sibling, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2014-02-14 13:06 UTC (permalink / raw) To: Intel Graphics Development; +Cc: 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] drm/i915: tune down user-triggerable dmesg noise in the cursor/overlay code 2014-02-14 13:06 ` [PATCH 2/3] drm/i915: tune down user-triggerable dmesg noise in the cursor/overlay code Daniel Vetter @ 2014-02-14 17:29 ` Damien Lespiau 2014-02-14 18:02 ` Daniel Vetter 0 siblings, 1 reply; 19+ messages in thread From: Damien Lespiau @ 2014-02-14 17:29 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] drm/i915: tune down user-triggerable dmesg noise in the cursor/overlay code 2014-02-14 17:29 ` Damien Lespiau @ 2014-02-14 18:02 ` Daniel Vetter 0 siblings, 0 replies; 19+ messages in thread From: Daniel Vetter @ 2014-02-14 18:02 UTC (permalink / raw) To: Damien Lespiau; +Cc: Daniel Vetter, Intel Graphics Development 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma 2014-02-14 13:06 ` [PATCH 1/3] " Daniel Vetter 2014-02-14 13:06 ` [PATCH 2/3] drm/i915: tune down user-triggerable dmesg noise in the cursor/overlay code Daniel Vetter @ 2014-02-14 13:06 ` Daniel Vetter 2014-02-14 13:23 ` Ville Syrjälä ` (3 more replies) 1 sibling, 4 replies; 19+ messages in thread From: Daniel Vetter @ 2014-02-14 13:06 UTC (permalink / raw) To: Intel Graphics Development; +Cc: 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma 2014-02-14 13:06 ` [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma Daniel Vetter @ 2014-02-14 13:23 ` Ville Syrjälä 2014-02-14 13:26 ` Chris Wilson ` (2 subsequent siblings) 3 siblings, 0 replies; 19+ messages in thread From: Ville Syrjälä @ 2014-02-14 13:23 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma 2014-02-14 13:06 ` [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma Daniel Vetter 2014-02-14 13:23 ` Ville Syrjälä @ 2014-02-14 13:26 ` Chris Wilson 2014-05-12 17:46 ` [PATCH igt] tests/kms_fence_pin_leak: Exercise full ppgtt fence pin_count leak in the kernel ville.syrjala 2014-05-14 16:14 ` [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma Ville Syrjälä 3 siblings, 0 replies; 19+ messages in thread From: Chris Wilson @ 2014-02-14 13:26 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH igt] tests/kms_fence_pin_leak: Exercise full ppgtt fence pin_count leak in the kernel 2014-02-14 13:06 ` [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma Daniel Vetter 2014-02-14 13:23 ` Ville Syrjälä 2014-02-14 13:26 ` Chris Wilson @ 2014-05-12 17:46 ` ville.syrjala 2014-05-12 18:34 ` Daniel Vetter 2014-05-14 16:14 ` [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma Ville Syrjälä 3 siblings, 1 reply; 19+ messages in thread From: ville.syrjala @ 2014-05-12 17:46 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH igt] tests/kms_fence_pin_leak: Exercise full ppgtt fence pin_count leak in the kernel 2014-05-12 17:46 ` [PATCH igt] tests/kms_fence_pin_leak: Exercise full ppgtt fence pin_count leak in the kernel ville.syrjala @ 2014-05-12 18:34 ` Daniel Vetter 2014-05-13 8:24 ` Ville Syrjälä 0 siblings, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2014-05-12 18:34 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH igt] tests/kms_fence_pin_leak: Exercise full ppgtt fence pin_count leak in the kernel 2014-05-12 18:34 ` Daniel Vetter @ 2014-05-13 8:24 ` Ville Syrjälä 2014-05-13 8:42 ` Daniel Vetter 0 siblings, 1 reply; 19+ messages in thread From: Ville Syrjälä @ 2014-05-13 8:24 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH igt] tests/kms_fence_pin_leak: Exercise full ppgtt fence pin_count leak in the kernel 2014-05-13 8:24 ` Ville Syrjälä @ 2014-05-13 8:42 ` Daniel Vetter 2014-05-13 8:56 ` [PATCH v2 " ville.syrjala 0 siblings, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2014-05-13 8:42 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 igt] tests/kms_fence_pin_leak: Exercise full ppgtt fence pin_count leak in the kernel 2014-05-13 8:42 ` Daniel Vetter @ 2014-05-13 8:56 ` ville.syrjala 0 siblings, 0 replies; 19+ messages in thread From: ville.syrjala @ 2014-05-13 8:56 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma 2014-02-14 13:06 ` [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma Daniel Vetter ` (2 preceding siblings ...) 2014-05-12 17:46 ` [PATCH igt] tests/kms_fence_pin_leak: Exercise full ppgtt fence pin_count leak in the kernel ville.syrjala @ 2014-05-14 16:14 ` Ville Syrjälä 2014-05-14 16:40 ` Daniel Vetter 3 siblings, 1 reply; 19+ messages in thread From: Ville Syrjälä @ 2014-05-14 16:14 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma 2014-05-14 16:14 ` [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma Ville Syrjälä @ 2014-05-14 16:40 ` Daniel Vetter 0 siblings, 0 replies; 19+ messages in thread From: Daniel Vetter @ 2014-05-14 16:40 UTC (permalink / raw) To: Ville Syrjälä; +Cc: Daniel Vetter, Intel Graphics Development 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2014-05-14 16:40 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-01-27 20:49 [PATCH 1/2] drm/i915: Don't drop pinned fences Daniel Vetter 2014-01-27 20:49 ` [PATCH 2/2] drm/i915: Only do gtt cleanup in vma_unbind for the global vma Daniel Vetter 2014-01-27 21:26 ` [PATCH 1/2] drm/i915: Don't drop pinned fences Chris Wilson 2014-01-27 21:41 ` Daniel Vetter 2014-01-28 12:40 ` Ville Syrjälä 2014-02-14 13:06 ` [PATCH 1/3] " Daniel Vetter 2014-02-14 13:06 ` [PATCH 2/3] drm/i915: tune down user-triggerable dmesg noise in the cursor/overlay code Daniel Vetter 2014-02-14 17:29 ` Damien Lespiau 2014-02-14 18:02 ` Daniel Vetter 2014-02-14 13:06 ` [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma Daniel Vetter 2014-02-14 13:23 ` Ville Syrjälä 2014-02-14 13:26 ` Chris Wilson 2014-05-12 17:46 ` [PATCH igt] tests/kms_fence_pin_leak: Exercise full ppgtt fence pin_count leak in the kernel ville.syrjala 2014-05-12 18:34 ` Daniel Vetter 2014-05-13 8:24 ` Ville Syrjälä 2014-05-13 8:42 ` Daniel Vetter 2014-05-13 8:56 ` [PATCH v2 " ville.syrjala 2014-05-14 16:14 ` [PATCH 3/3] drm/i915: Only do gtt cleanup in vma_unbind for the global vma Ville Syrjälä 2014-05-14 16:40 ` 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.