| Message ID | 1423500207-1271-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5733
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 282/283 282/283
ILK +3-1 308/319 310/319
SNB +1-3 340/346 338/346
IVB -1 378/384 377/384
BYT 296/296 296/296
HSW +2 421/428 423/428
BDW 318/333 318/333
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
ILK igt_gem_unfence_active_buffers DMESG_WARN(1, M37)PASS(1, M37) DMESG_WARN(1, M37)
ILK igt_kms_flip_busy-flip-interruptible TIMEOUT(2, M37)PASS(1, M37) PASS(1, M37)
*ILK igt_kms_flip_flip-vs-panning TIMEOUT(2, M37) PASS(1, M37)
*ILK igt_kms_flip_plain-flip-ts-check-interruptible TIMEOUT(1, M37) PASS(1, M37)
*SNB igt_kms_flip_dpms-vs-vblank-race PASS(4, M22) DMESG_WARN(1, M22)
SNB igt_kms_flip_dpms-vs-vblank-race-interruptible DMESG_WARN(3, M22)PASS(2, M22) DMESG_WARN(1, M22)
SNB igt_kms_flip_modeset-vs-vblank-race-interruptible DMESG_WARN(1, M22)PASS(2, M22) DMESG_WARN(1, M22)
SNB igt_kms_pipe_crc_basic_read-crc-pipe-A DMESG_WARN(1, M22)PASS(6, M22) PASS(1, M22)
IVB igt_gem_pwrite_pread_snooped-copy-performance DMESG_WARN(1, M34)PASS(3, M34) DMESG_WARN(1, M34)
HSW igt_gem_storedw_loop_blt DMESG_WARN(2, M20)PASS(3, M20) PASS(1, M20)
HSW igt_gem_storedw_loop_vebox DMESG_WARN(2, M20)PASS(2, M20) PASS(1, M20)
Note: You need to pay more attention to line start with '*'
On Mon, 09 Feb 2015, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > From: Chris Wilson <chris@chris-wilson.co.uk> > > This (partially) reverts > > commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Tue Mar 25 13:23:06 2014 +0000 > > drm/i915: Invalidate our pages under memory pressure > > It appears given the right workload, that pages which are swapped out > more than once are incorrectly invalidated and discarded. I had presumed > that the swapin would mark the pages dirty again and so preserve them > against the next cycle of invalidation - that appears to be false, and > leads to memory corruption (even leak of stale pages to userspace). > > v2: Do a more throughrought revert and als get rid of the hunk in > gem_free_objects which we've tried to patch up already in > > commit 340fbd8ca1c7d6006a6b6afe716c10007bbfde85 > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Thu May 22 09:16:52 2014 +0100 > > drm/i915: Only discard backing storage on releasing the last ref > > This means this patch also fully reverts this fixup. Apparently this > is just too tricky. > > Reported-by: Sean V Kelley <sean.v.kelley@intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Sean V Kelley <sean.v.kelley@intel.com> > Cc: stable@vger.kernel.org > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> (v2) Pushed this one to drm-intel-next-fixes, thanks for the patch, and v2 of the patch. BR, Jani. > --- > drivers/gpu/drm/i915/i915_gem.c | 49 ++--------------------------------------- > 1 file changed, 2 insertions(+), 47 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 36f1093e3c63..39e2af9b5fef 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1946,26 +1946,6 @@ i915_gem_object_truncate(struct drm_i915_gem_object *obj) > obj->madv = __I915_MADV_PURGED; > } > > -/* Try to discard unwanted pages */ > -static void > -i915_gem_object_invalidate(struct drm_i915_gem_object *obj) > -{ > - struct address_space *mapping; > - > - switch (obj->madv) { > - case I915_MADV_DONTNEED: > - i915_gem_object_truncate(obj); > - case __I915_MADV_PURGED: > - return; > - } > - > - if (obj->base.filp == NULL) > - return; > - > - mapping = file_inode(obj->base.filp)->i_mapping, > - invalidate_mapping_pages(mapping, 0, (loff_t)-1); > -} > - > static void > i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj) > { > @@ -2028,7 +2008,8 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj) > ops->put_pages(obj); > obj->pages = NULL; > > - i915_gem_object_invalidate(obj); > + if (i915_gem_object_is_purgeable(obj)) > + i915_gem_object_truncate(obj); > > return 0; > } > @@ -4458,30 +4439,6 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > return obj; > } > > -static bool discard_backing_storage(struct drm_i915_gem_object *obj) > -{ > - /* If we are the last user of the backing storage (be it shmemfs > - * pages or stolen etc), we know that the pages are going to be > - * immediately released. In this case, we can then skip copying > - * back the contents from the GPU. > - */ > - > - if (obj->madv != I915_MADV_WILLNEED) > - return false; > - > - if (obj->base.filp == NULL) > - return true; > - > - /* At first glance, this looks racy, but then again so would be > - * userspace racing mmap against close. However, the first external > - * reference to the filp can only be obtained through the > - * i915_gem_mmap_ioctl() which safeguards us against the user > - * acquiring such a reference whilst we are in the middle of > - * freeing the object. > - */ > - return atomic_long_read(&obj->base.filp->f_count) == 1; > -} > - > void i915_gem_free_object(struct drm_gem_object *gem_obj) > { > struct drm_i915_gem_object *obj = to_intel_bo(gem_obj); > @@ -4524,8 +4481,6 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) > > if (WARN_ON(obj->pages_pin_count)) > obj->pages_pin_count = 0; > - if (discard_backing_storage(obj)) > - obj->madv = I915_MADV_DONTNEED; > i915_gem_object_put_pages(obj); > i915_gem_object_free_mmap_offset(obj); > > -- > 2.1.4 >
On Tue, 10 Feb 2015, Jani Nikula <jani.nikula@intel.com> wrote: > On Mon, 09 Feb 2015, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> From: Chris Wilson <chris@chris-wilson.co.uk> >> >> This (partially) reverts >> >> commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd >> Author: Chris Wilson <chris@chris-wilson.co.uk> >> Date: Tue Mar 25 13:23:06 2014 +0000 >> >> drm/i915: Invalidate our pages under memory pressure >> >> It appears given the right workload, that pages which are swapped out >> more than once are incorrectly invalidated and discarded. I had presumed >> that the swapin would mark the pages dirty again and so preserve them >> against the next cycle of invalidation - that appears to be false, and >> leads to memory corruption (even leak of stale pages to userspace). >> >> v2: Do a more throughrought revert and als get rid of the hunk in >> gem_free_objects which we've tried to patch up already in >> >> commit 340fbd8ca1c7d6006a6b6afe716c10007bbfde85 >> Author: Chris Wilson <chris@chris-wilson.co.uk> >> Date: Thu May 22 09:16:52 2014 +0100 >> >> drm/i915: Only discard backing storage on releasing the last ref >> >> This means this patch also fully reverts this fixup. Apparently this >> is just too tricky. >> >> Reported-by: Sean V Kelley <sean.v.kelley@intel.com> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Sean V Kelley <sean.v.kelley@intel.com> >> Cc: stable@vger.kernel.org >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> (v2) > > Pushed this one to drm-intel-next-fixes, thanks for the patch, and v2 of > the patch. For completeness, this one was dropped again on Feb 11. Jani. > > BR, > Jani. > >> --- >> drivers/gpu/drm/i915/i915_gem.c | 49 ++--------------------------------------- >> 1 file changed, 2 insertions(+), 47 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index 36f1093e3c63..39e2af9b5fef 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -1946,26 +1946,6 @@ i915_gem_object_truncate(struct drm_i915_gem_object *obj) >> obj->madv = __I915_MADV_PURGED; >> } >> >> -/* Try to discard unwanted pages */ >> -static void >> -i915_gem_object_invalidate(struct drm_i915_gem_object *obj) >> -{ >> - struct address_space *mapping; >> - >> - switch (obj->madv) { >> - case I915_MADV_DONTNEED: >> - i915_gem_object_truncate(obj); >> - case __I915_MADV_PURGED: >> - return; >> - } >> - >> - if (obj->base.filp == NULL) >> - return; >> - >> - mapping = file_inode(obj->base.filp)->i_mapping, >> - invalidate_mapping_pages(mapping, 0, (loff_t)-1); >> -} >> - >> static void >> i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj) >> { >> @@ -2028,7 +2008,8 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj) >> ops->put_pages(obj); >> obj->pages = NULL; >> >> - i915_gem_object_invalidate(obj); >> + if (i915_gem_object_is_purgeable(obj)) >> + i915_gem_object_truncate(obj); >> >> return 0; >> } >> @@ -4458,30 +4439,6 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, >> return obj; >> } >> >> -static bool discard_backing_storage(struct drm_i915_gem_object *obj) >> -{ >> - /* If we are the last user of the backing storage (be it shmemfs >> - * pages or stolen etc), we know that the pages are going to be >> - * immediately released. In this case, we can then skip copying >> - * back the contents from the GPU. >> - */ >> - >> - if (obj->madv != I915_MADV_WILLNEED) >> - return false; >> - >> - if (obj->base.filp == NULL) >> - return true; >> - >> - /* At first glance, this looks racy, but then again so would be >> - * userspace racing mmap against close. However, the first external >> - * reference to the filp can only be obtained through the >> - * i915_gem_mmap_ioctl() which safeguards us against the user >> - * acquiring such a reference whilst we are in the middle of >> - * freeing the object. >> - */ >> - return atomic_long_read(&obj->base.filp->f_count) == 1; >> -} >> - >> void i915_gem_free_object(struct drm_gem_object *gem_obj) >> { >> struct drm_i915_gem_object *obj = to_intel_bo(gem_obj); >> @@ -4524,8 +4481,6 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) >> >> if (WARN_ON(obj->pages_pin_count)) >> obj->pages_pin_count = 0; >> - if (discard_backing_storage(obj)) >> - obj->madv = I915_MADV_DONTNEED; >> i915_gem_object_put_pages(obj); >> i915_gem_object_free_mmap_offset(obj); >> >> -- >> 2.1.4 >> > > -- > Jani Nikula, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 36f1093e3c63..39e2af9b5fef 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1946,26 +1946,6 @@ i915_gem_object_truncate(struct drm_i915_gem_object *obj) obj->madv = __I915_MADV_PURGED; } -/* Try to discard unwanted pages */ -static void -i915_gem_object_invalidate(struct drm_i915_gem_object *obj) -{ - struct address_space *mapping; - - switch (obj->madv) { - case I915_MADV_DONTNEED: - i915_gem_object_truncate(obj); - case __I915_MADV_PURGED: - return; - } - - if (obj->base.filp == NULL) - return; - - mapping = file_inode(obj->base.filp)->i_mapping, - invalidate_mapping_pages(mapping, 0, (loff_t)-1); -} - static void i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj) { @@ -2028,7 +2008,8 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj) ops->put_pages(obj); obj->pages = NULL; - i915_gem_object_invalidate(obj); + if (i915_gem_object_is_purgeable(obj)) + i915_gem_object_truncate(obj); return 0; } @@ -4458,30 +4439,6 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, return obj; } -static bool discard_backing_storage(struct drm_i915_gem_object *obj) -{ - /* If we are the last user of the backing storage (be it shmemfs - * pages or stolen etc), we know that the pages are going to be - * immediately released. In this case, we can then skip copying - * back the contents from the GPU. - */ - - if (obj->madv != I915_MADV_WILLNEED) - return false; - - if (obj->base.filp == NULL) - return true; - - /* At first glance, this looks racy, but then again so would be - * userspace racing mmap against close. However, the first external - * reference to the filp can only be obtained through the - * i915_gem_mmap_ioctl() which safeguards us against the user - * acquiring such a reference whilst we are in the middle of - * freeing the object. - */ - return atomic_long_read(&obj->base.filp->f_count) == 1; -} - void i915_gem_free_object(struct drm_gem_object *gem_obj) { struct drm_i915_gem_object *obj = to_intel_bo(gem_obj); @@ -4524,8 +4481,6 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) if (WARN_ON(obj->pages_pin_count)) obj->pages_pin_count = 0; - if (discard_backing_storage(obj)) - obj->madv = I915_MADV_DONTNEED; i915_gem_object_put_pages(obj); i915_gem_object_free_mmap_offset(obj);