[PATCH 1/4] drm/ttm: move unlocking out of ttm_bo_cleanup_memtype_use

[PATCH 1/4] drm/ttm: move unlocking out of ttm_bo_cleanup_memtype_use

[Christian König]

Needed for the next patch and makes the code quite a bit easier to
understand.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index c088703777e2..9905cf41cba6 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -390,8 +390,6 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
 	ttm_tt_destroy(bo->ttm);
 	bo->ttm = NULL;
 	ttm_bo_mem_put(bo, &bo->mem);
-
-	ww_mutex_unlock (&bo->resv->lock);
 }
 
 static int ttm_bo_individualize_resv(struct ttm_buffer_object *bo)
@@ -457,6 +455,7 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 				reservation_object_unlock(&bo->ttm_resv);
 
 			ttm_bo_cleanup_memtype_use(bo);
+			reservation_object_unlock(bo->resv);
 			return;
 		}
 
@@ -559,6 +558,7 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 
 	spin_unlock(&glob->lru_lock);
 	ttm_bo_cleanup_memtype_use(bo);
+	reservation_object_unlock(bo->resv);
 
 	return 0;
 }
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/ttm: consistently use reservation_object_unlock

[Christian König]

Instead of having a pointless wrapper or call the underlying ww_mutex
function directly.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c           | 13 +++++++------
 drivers/gpu/drm/ttm/ttm_execbuf_util.c |  8 ++++----
 include/drm/ttm/ttm_bo_driver.h        | 14 +-------------
 3 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 9905cf41cba6..6f55310a9d09 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -471,7 +471,7 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 			ttm_bo_add_to_lru(bo);
 		}
 
-		__ttm_bo_unreserve(bo);
+		reservation_object_unlock(bo->resv);
 	}
 	if (bo->resv != &bo->ttm_resv)
 		reservation_object_unlock(&bo->ttm_resv);
@@ -517,7 +517,8 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 
 	if (ret && !no_wait_gpu) {
 		long lret;
-		ww_mutex_unlock(&bo->resv->lock);
+
+		reservation_object_unlock(bo->resv);
 		spin_unlock(&glob->lru_lock);
 
 		lret = reservation_object_wait_timeout_rcu(resv, true,
@@ -547,7 +548,7 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 	}
 
 	if (ret || unlikely(list_empty(&bo->ddestroy))) {
-		__ttm_bo_unreserve(bo);
+		reservation_object_unlock(bo->resv);
 		spin_unlock(&glob->lru_lock);
 		return ret;
 	}
@@ -749,7 +750,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 
 			if (place && !bdev->driver->eviction_valuable(bo,
 								      place)) {
-				__ttm_bo_unreserve(bo);
+				reservation_object_unlock(bo->resv);
 				ret = -EBUSY;
 				continue;
 			}
@@ -1788,7 +1789,7 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
 	 * already swapped buffer.
 	 */
 
-	__ttm_bo_unreserve(bo);
+	reservation_object_unlock(bo->resv);
 	kref_put(&bo->list_kref, ttm_bo_release_list);
 	return ret;
 }
@@ -1825,7 +1826,7 @@ int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo)
 	ret = __ttm_bo_reserve(bo, true, false, NULL);
 	if (unlikely(ret != 0))
 		goto out_unlock;
-	__ttm_bo_unreserve(bo);
+	reservation_object_unlock(bo->resv);
 
 out_unlock:
 	mutex_unlock(&bo->wu_mutex);
diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c
index 5e1bcabffef5..373ced0b2fc2 100644
--- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c
+++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c
@@ -38,7 +38,7 @@ static void ttm_eu_backoff_reservation_reverse(struct list_head *list,
 	list_for_each_entry_continue_reverse(entry, list, head) {
 		struct ttm_buffer_object *bo = entry->bo;
 
-		__ttm_bo_unreserve(bo);
+		reservation_object_unlock(bo->resv);
 	}
 }
 
@@ -69,7 +69,7 @@ void ttm_eu_backoff_reservation(struct ww_acquire_ctx *ticket,
 		struct ttm_buffer_object *bo = entry->bo;
 
 		ttm_bo_add_to_lru(bo);
-		__ttm_bo_unreserve(bo);
+		reservation_object_unlock(bo->resv);
 	}
 	spin_unlock(&glob->lru_lock);
 
@@ -112,7 +112,7 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket,
 
 		ret = __ttm_bo_reserve(bo, intr, (ticket == NULL), ticket);
 		if (!ret && unlikely(atomic_read(&bo->cpu_writers) > 0)) {
-			__ttm_bo_unreserve(bo);
+			reservation_object_unlock(bo->resv);
 
 			ret = -EBUSY;
 
@@ -203,7 +203,7 @@ void ttm_eu_fence_buffer_objects(struct ww_acquire_ctx *ticket,
 		else
 			reservation_object_add_excl_fence(bo->resv, fence);
 		ttm_bo_add_to_lru(bo);
-		__ttm_bo_unreserve(bo);
+		reservation_object_unlock(bo->resv);
 	}
 	spin_unlock(&glob->lru_lock);
 	if (ticket)
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 5f821a9b3a1f..389359a0006b 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -941,18 +941,6 @@ static inline int ttm_bo_reserve_slowpath(struct ttm_buffer_object *bo,
 }
 
 /**
- * __ttm_bo_unreserve
- * @bo: A pointer to a struct ttm_buffer_object.
- *
- * Unreserve a previous reservation of @bo where the buffer object is
- * already on lru lists.
- */
-static inline void __ttm_bo_unreserve(struct ttm_buffer_object *bo)
-{
-	ww_mutex_unlock(&bo->resv->lock);
-}
-
-/**
  * ttm_bo_unreserve
  *
  * @bo: A pointer to a struct ttm_buffer_object.
@@ -966,7 +954,7 @@ static inline void ttm_bo_unreserve(struct ttm_buffer_object *bo)
 		ttm_bo_add_to_lru(bo);
 		spin_unlock(&bo->glob->lru_lock);
 	}
-	__ttm_bo_unreserve(bo);
+	reservation_object_unlock(bo->resv);
 }
 
 /**
-- 
2.11.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/4] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional

[Christian König]

Needed for the next patch.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 52 ++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 6f55310a9d09..d23592cfe42e 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -486,20 +486,21 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 }
 
 /**
- * function ttm_bo_cleanup_refs_and_unlock
+ * function ttm_bo_cleanup_refs
  * If bo idle, remove from delayed- and lru lists, and unref.
  * If not idle, do nothing.
  *
  * Must be called with lru_lock and reservation held, this function
- * will drop both before returning.
+ * will drop the lru lock and optionally the reservation lock before returning.
  *
  * @interruptible         Any sleeps should occur interruptibly.
  * @no_wait_gpu           Never wait for gpu. Return -EBUSY instead.
+ * @unlock_resv           Unlock the reservation lock as well.
  */
 
-static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
-					  bool interruptible,
-					  bool no_wait_gpu)
+static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
+			       bool interruptible, bool no_wait_gpu,
+			       bool unlock_resv)
 {
 	struct ttm_bo_global *glob = bo->glob;
 	struct reservation_object *resv;
@@ -518,7 +519,8 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 	if (ret && !no_wait_gpu) {
 		long lret;
 
-		reservation_object_unlock(bo->resv);
+		if (unlock_resv)
+			reservation_object_unlock(bo->resv);
 		spin_unlock(&glob->lru_lock);
 
 		lret = reservation_object_wait_timeout_rcu(resv, true,
@@ -531,19 +533,22 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 			return -EBUSY;
 
 		spin_lock(&glob->lru_lock);
-		ret = __ttm_bo_reserve(bo, false, true, NULL);
-
-		/*
-		 * We raced, and lost, someone else holds the reservation now,
-		 * and is probably busy in ttm_bo_cleanup_memtype_use.
-		 *
-		 * Even if it's not the case, because we finished waiting any
-		 * delayed destruction would succeed, so just return success
-		 * here.
-		 */
-		if (ret) {
-			spin_unlock(&glob->lru_lock);
-			return 0;
+		if (unlock_resv) {
+			ret = __ttm_bo_reserve(bo, false, true, NULL);
+			/*
+			 * We raced, and lost, someone else holds the reservation now,
+			 * and is probably busy in ttm_bo_cleanup_memtype_use.
+			 *
+			 * Even if it's not the case, because we finished waiting any
+			 * delayed destruction would succeed, so just return success
+			 * here.
+			 */
+			if (ret) {
+				spin_unlock(&glob->lru_lock);
+				return 0;
+			}
+		} else {
+			ret = 0;
 		}
 	}
 
@@ -600,8 +605,8 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all)
 		}
 
 		if (!ret)
-			ret = ttm_bo_cleanup_refs_and_unlock(entry, false,
-							     !remove_all);
+			ret = ttm_bo_cleanup_refs(entry, false, !remove_all,
+						  true);
 		else
 			spin_unlock(&glob->lru_lock);
 
@@ -770,8 +775,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 	kref_get(&bo->list_kref);
 
 	if (!list_empty(&bo->ddestroy)) {
-		ret = ttm_bo_cleanup_refs_and_unlock(bo, interruptible,
-						     no_wait_gpu);
+		ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu, true);
 		kref_put(&bo->list_kref, ttm_bo_release_list);
 		return ret;
 	}
@@ -1735,7 +1739,7 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
 	kref_get(&bo->list_kref);
 
 	if (!list_empty(&bo->ddestroy)) {
-		ret = ttm_bo_cleanup_refs_and_unlock(bo, false, false);
+		ret = ttm_bo_cleanup_refs(bo, false, false, true);
 		kref_put(&bo->list_kref, ttm_bo_release_list);
 		return ret;
 	}
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/ttm: optimize ttm_mem_evict_first

[Christian König]

Deleted BOs with the same reservation object can be reaped even if they
can't be reserved.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index d23592cfe42e..f486a484c70f 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -735,20 +735,35 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
 EXPORT_SYMBOL(ttm_bo_eviction_valuable);
 
 static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
-				uint32_t mem_type,
-				const struct ttm_place *place,
-				bool interruptible,
-				bool no_wait_gpu)
+			       struct reservation_object *resv,
+			       uint32_t mem_type,
+			       const struct ttm_place *place,
+			       bool interruptible,
+			       bool no_wait_gpu)
 {
 	struct ttm_bo_global *glob = bdev->glob;
 	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
 	struct ttm_buffer_object *bo;
 	int ret = -EBUSY;
+	bool locked;
 	unsigned i;
 
 	spin_lock(&glob->lru_lock);
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
 		list_for_each_entry(bo, &man->lru[i], lru) {
+			if (bo->resv == resv) {
+				if (list_empty(&bo->ddestroy))
+					continue;
+
+				if (place &&
+				    !bdev->driver->eviction_valuable(bo, place))
+					continue;
+
+				ret = 0;
+				locked = false;
+				break;
+			}
+
 			ret = __ttm_bo_reserve(bo, false, true, NULL);
 			if (ret)
 				continue;
@@ -760,6 +775,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 				continue;
 			}
 
+			locked = true;
 			break;
 		}
 
@@ -775,7 +791,8 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 	kref_get(&bo->list_kref);
 
 	if (!list_empty(&bo->ddestroy)) {
-		ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu, true);
+		ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu,
+					  locked);
 		kref_put(&bo->list_kref, ttm_bo_release_list);
 		return ret;
 	}
@@ -786,7 +803,8 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 	BUG_ON(ret != 0);
 
 	ret = ttm_bo_evict(bo, interruptible, no_wait_gpu);
-	ttm_bo_unreserve(bo);
+	if (locked)
+		ttm_bo_unreserve(bo);
 
 	kref_put(&bo->list_kref, ttm_bo_release_list);
 	return ret;
@@ -850,7 +868,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
 			return ret;
 		if (mem->mm_node)
 			break;
-		ret = ttm_mem_evict_first(bdev, mem_type, place,
+		ret = ttm_mem_evict_first(bdev, bo->resv, mem_type, place,
 					  interruptible, no_wait_gpu);
 		if (unlikely(ret != 0))
 			return ret;
@@ -1353,7 +1371,8 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
 		while (!list_empty(&man->lru[i])) {
 			spin_unlock(&glob->lru_lock);
-			ret = ttm_mem_evict_first(bdev, mem_type, NULL, false, false);
+			ret = ttm_mem_evict_first(bdev, NULL, mem_type, NULL,
+						  false, false);
 			if (ret)
 				return ret;
 			spin_lock(&glob->lru_lock);
-- 
2.11.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/4] drm/ttm: consistently use reservation_object_unlock

[Michel Dänzer]

On 08/11/17 03:59 PM, Christian König wrote:
> Instead of having a pointless wrapper or call the underlying ww_mutex
> function directly.

Not sure I can agree it's pointless, since it recently took me a while
to realize that unlocking bo->resv is essentially the same as
unreserving the BO.


Anyway, this breaks the qxl driver:

drivers/gpu/drm//qxl/qxl_release.c: In function ‘qxl_release_fence_buffer_objects’:
drivers/gpu/drm//qxl/qxl_release.c:472:3: error: implicit declaration of function ‘__ttm_bo_unreserve’; did you mean ‘ttm_bo_unreserve’? [-Werror=implicit-function-declaration]
   __ttm_bo_unreserve(bo);
   ^~~~~~~~~~~~~~~~~~
   ttm_bo_unreserve


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/ttm: consistently use reservation_object_unlock

[Christian König]

Am 08.11.2017 um 17:36 schrieb Michel Dänzer:
> On 08/11/17 03:59 PM, Christian König wrote:
>> Instead of having a pointless wrapper or call the underlying ww_mutex
>> function directly.
> Not sure I can agree it's pointless, since it recently took me a while
> to realize that unlocking bo->resv is essentially the same as
> unreserving the BO.

Ok in this case let's call this confusing. But yeah that 
__ttm_bo_unreserv(bo), reservation_object_unlock(bo->resv) and 
ww_mutex_unlock(&bo->resv->lock) are essentially the same thing is 
exactly what I want to fix here.

> Anyway, this breaks the qxl driver:
>
> drivers/gpu/drm//qxl/qxl_release.c: In function ‘qxl_release_fence_buffer_objects’:
> drivers/gpu/drm//qxl/qxl_release.c:472:3: error: implicit declaration of function ‘__ttm_bo_unreserve’; did you mean ‘ttm_bo_unreserve’? [-Werror=implicit-function-declaration]
>     __ttm_bo_unreserve(bo);
>     ^~~~~~~~~~~~~~~~~~
>     ttm_bo_unreserve

Thanks for pointing this out, going to respin.

Christian.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/ttm: consistently use reservation_object_unlock

[Michel Dänzer]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

On 08/11/17 05:41 PM, Christian König wrote:
> Am 08.11.2017 um 17:36 schrieb Michel Dänzer:
>> On 08/11/17 03:59 PM, Christian König wrote:
>>> Instead of having a pointless wrapper or call the underlying ww_mutex
>>> function directly.
>> Not sure I can agree it's pointless, since it recently took me a while
>> to realize that unlocking bo->resv is essentially the same as
>> unreserving the BO.
> 
> Ok in this case let's call this confusing. But yeah that
> __ttm_bo_unreserv(bo), reservation_object_unlock(bo->resv) and
> ww_mutex_unlock(&bo->resv->lock) are essentially the same thing is
> exactly what I want to fix here.

How about always using __ttm_bo_unreserve instead?


The first piglit run with this series applied hit the BUG_ON in
ttm_bo_add_to_lru, see the attached dmesg excerpt. The machine hung,
couldn't reboot cleanly. Haven't been able to reproduce it in three more
attempts though, so I'm not sure it's caused by this series, but I don't
remember seeing it before.


P.S. I just noticed I haven't had CONFIG_PROVE_LOCKING enabled in my
kernels for a while, I'll enable it again. Any other options that should
be enabled for testing?


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: kern.log --]
[-- Type: text/x-log; name="kern.log", Size: 9733 bytes --]

Nov  8 17:54:48 kaveri kernel: [ 1084.244694] ------------[ cut here ]------------
Nov  8 17:54:48 kaveri kernel: [ 1084.244697] kernel BUG at drivers/gpu/drm//ttm/ttm_bo.c:172!
Nov  8 17:54:48 kaveri kernel: [ 1084.244703] invalid opcode: 0000 [#1] SMP KASAN
Nov  8 17:54:48 kaveri kernel: [ 1084.244705] Modules linked in: lz4 lz4_compress cpufreq_powersave cpufreq_userspace cpufreq_conservative binfmt_misc nls_ascii nls_cp437 vfat fat edac_mce_amd amdgpu(O) amdkfd(O) radeon(O) kvm snd_hda_codec_realtek chash irqbypass snd_hda_codec_generic ttm(O) snd_hda_codec_hdmi crct10dif_pclmul crc32_pclmul drm_kms_helper(O) ghash_clmulni_intel efi_pstore snd_hda_intel pcbc snd_hda_codec drm(O) snd_hda_core snd_hwdep wmi_bmof snd_pcm snd_timer i2c_algo_bit fb_sys_fops snd aesni_intel syscopyarea ccp sysfillrect aes_x86_64 r8169 crypto_simd sp5100_tco ppdev glue_helper cryptd sg sysimgblt mfd_core mii soundcore rng_core pcspkr efivars i2c_piix4 parport_pc wmi parport i2c_designware_platform i2c_designware_core button acpi_cpufreq tcp_bbr sch_fq nct6775 hwmon_vid sunrpc efivarfs ip_tables x_tables
Nov  8 17:54:48 kaveri kernel: [ 1084.244743]  autofs4 ext4 crc16 mbcache jbd2 fscrypto dm_mod raid10 raid1 raid0 multipath linear md_mod sd_mod evdev hid_generic usbhid hid ahci xhci_pci libahci xhci_hcd libata crc32c_intel usbcore scsi_mod shpchp gpio_amdpt gpio_generic
Nov  8 17:54:48 kaveri kernel: [ 1084.244766] CPU: 13 PID: 4778 Comm: tex3d-maxsize Tainted: G           O    4.14.0-rc3+ #32
Nov  8 17:54:48 kaveri kernel: [ 1084.244768] Hardware name: Micro-Star International Co., Ltd. MS-7A34/B350 TOMAHAWK (MS-7A34), BIOS 1.80 09/13/2017
Nov  8 17:54:48 kaveri kernel: [ 1084.244770] task: ffff880233de6c80 task.stack: ffff880225ce0000
Nov  8 17:54:48 kaveri kernel: [ 1084.244779] RIP: 0010:ttm_bo_add_to_lru+0x4fe/0x640 [ttm]
Nov  8 17:54:48 kaveri kernel: [ 1084.244780] RSP: 0018:ffff880225ce6d60 EFLAGS: 00010206
Nov  8 17:54:48 kaveri kernel: [ 1084.244783] RAX: dffffc0000000000 RBX: ffff88038de24cd8 RCX: 0000000000000000
Nov  8 17:54:48 kaveri kernel: [ 1084.244784] RDX: 1ffff10044b9cdb4 RSI: 0000000000000000 RDI: ffff88038de24ce0
Nov  8 17:54:48 kaveri kernel: [ 1084.244786] RBP: 1ffff10044b9cdb0 R08: 0000000007cbd06c R09: 0000000000000000
Nov  8 17:54:48 kaveri kernel: [ 1084.244788] R10: ffffffff93daac8e R11: 0000000000057de4 R12: 1ffff10044b9cdb4
Nov  8 17:54:48 kaveri kernel: [ 1084.244791] R13: ffff880388033c88 R14: ffff88038de24d88 R15: ffff88038a452718
Nov  8 17:54:48 kaveri kernel: [ 1084.244794] FS:  00007fa01e650300(0000) GS:ffff8803ae740000(0000) knlGS:0000000000000000
Nov  8 17:54:48 kaveri kernel: [ 1084.244797] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Nov  8 17:54:48 kaveri kernel: [ 1084.244799] CR2: 00007faecd0f06f0 CR3: 000000038b7d3000 CR4: 00000000003406e0
Nov  8 17:54:48 kaveri kernel: [ 1084.244801] Call Trace:
Nov  8 17:54:48 kaveri kernel: [ 1084.244823]  ? drm_mm_init+0x4d0/0x4d0 [drm]
Nov  8 17:54:48 kaveri kernel: [ 1084.244832]  ? ttm_bo_device_init+0x620/0x620 [ttm]
Nov  8 17:54:48 kaveri kernel: [ 1084.244837]  ? __kernel_text_address+0xe/0x30
Nov  8 17:54:48 kaveri kernel: [ 1084.244841]  ? bpf_prog_alloc+0x2d0/0x2d0
Nov  8 17:54:48 kaveri kernel: [ 1084.244844]  ? deref_stack_reg+0x1f0/0x1f0
Nov  8 17:54:48 kaveri kernel: [ 1084.244851]  ttm_mem_evict_first+0x473/0x5c0 [ttm]
Nov  8 17:54:48 kaveri kernel: [ 1084.244857]  ? ttm_bo_evict+0xc70/0xc70 [ttm]
Nov  8 17:54:48 kaveri kernel: [ 1084.244863]  ttm_bo_mem_space+0x89d/0xed0 [ttm]
Nov  8 17:54:48 kaveri kernel: [ 1084.244869]  ? ttm_bo_mem_compat+0x6b/0x130 [ttm]
Nov  8 17:54:48 kaveri kernel: [ 1084.244874]  ttm_bo_validate+0x301/0x530 [ttm]
Nov  8 17:54:48 kaveri kernel: [ 1084.244879]  ? ttm_bo_evict_mm+0xa0/0xa0 [ttm]
Nov  8 17:54:48 kaveri kernel: [ 1084.244882]  ? SyS_membarrier+0x2b0/0x2b0
Nov  8 17:54:48 kaveri kernel: [ 1084.244896]  ? drm_vma_offset_add+0xb4/0x180 [drm]
Nov  8 17:54:48 kaveri kernel: [ 1084.244902]  ttm_bo_init_reserved+0xaf3/0x1200 [ttm]
Nov  8 17:54:48 kaveri kernel: [ 1084.244908]  ? ttm_bo_validate+0x530/0x530 [ttm]
Nov  8 17:54:48 kaveri kernel: [ 1084.244913]  ? get_empty_filp+0x27b/0x530
Nov  8 17:54:48 kaveri kernel: [ 1084.244914]  ? proc_nr_files+0x30/0x30
Nov  8 17:54:48 kaveri kernel: [ 1084.244919]  ? _copy_to_user+0x90/0x90
Nov  8 17:54:48 kaveri kernel: [ 1084.244922]  ? alloc_file+0x16d/0x440
Nov  8 17:54:48 kaveri kernel: [ 1084.244926]  ? __shmem_file_setup+0x2af/0x480
Nov  8 17:54:48 kaveri kernel: [ 1084.244981]  ? amdgpu_vm_clear_freed+0x28d/0x5d0 [amdgpu]
Nov  8 17:54:48 kaveri kernel: [ 1084.244993]  ? drm_gem_private_object_init+0x18a/0x2e0 [drm]
Nov  8 17:54:48 kaveri kernel: [ 1084.244997]  ? kasan_kmalloc+0xa0/0xd0
Nov  8 17:54:48 kaveri kernel: [ 1084.245038]  amdgpu_bo_do_create+0x549/0x1260 [amdgpu]
Nov  8 17:54:48 kaveri kernel: [ 1084.245079]  ? amdgpu_fill_buffer+0x9f0/0x9f0 [amdgpu]
Nov  8 17:54:48 kaveri kernel: [ 1084.245082]  ? show_initstate+0xb0/0xb0
Nov  8 17:54:48 kaveri kernel: [ 1084.245122]  ? amdgpu_ttm_placement_from_domain+0x8a0/0x8a0 [amdgpu]
Nov  8 17:54:48 kaveri kernel: [ 1084.245126]  ? reservation_object_test_signaled_rcu+0xb40/0xb40
Nov  8 17:54:48 kaveri kernel: [ 1084.245128]  ? deref_stack_reg+0x1f0/0x1f0
Nov  8 17:54:48 kaveri kernel: [ 1084.245132]  ? __free_insn_slot+0x4c0/0x4c0
Nov  8 17:54:48 kaveri kernel: [ 1084.245134]  ? reservation_object_add_excl_fence+0x380/0x380
Nov  8 17:54:48 kaveri kernel: [ 1084.245137]  ? is_bpf_text_address+0xa/0x20
Nov  8 17:54:48 kaveri kernel: [ 1084.245139]  ? __kernel_text_address+0xe/0x30
Nov  8 17:54:48 kaveri kernel: [ 1084.245141]  ? unwind_get_return_address+0x5f/0xa0
Nov  8 17:54:48 kaveri kernel: [ 1084.245182]  amdgpu_bo_create+0xba/0x880 [amdgpu]
Nov  8 17:54:48 kaveri kernel: [ 1084.245188]  ? ttm_bo_release_list+0x7d0/0x7d0 [ttm]
Nov  8 17:54:48 kaveri kernel: [ 1084.245228]  ? amdgpu_bo_do_create+0x1260/0x1260 [amdgpu]
Nov  8 17:54:48 kaveri kernel: [ 1084.245233]  ? ttm_bo_unref+0x493/0x9e0 [ttm]
Nov  8 17:54:48 kaveri kernel: [ 1084.245237]  ? __radix_tree_insert+0x7e0/0x7e0
Nov  8 17:54:48 kaveri kernel: [ 1084.245278]  amdgpu_gem_object_create+0x10d/0x210 [amdgpu]
Nov  8 17:54:48 kaveri kernel: [ 1084.245320]  ? amdgpu_gem_object_free+0x180/0x180 [amdgpu]
Nov  8 17:54:48 kaveri kernel: [ 1084.245323]  ? __ww_mutex_lock_interruptible_slowpath+0x20/0x20
Nov  8 17:54:48 kaveri kernel: [ 1084.245325]  ? __radix_tree_delete+0x13f/0x1c0
Nov  8 17:54:48 kaveri kernel: [ 1084.245367]  amdgpu_gem_create_ioctl+0x37d/0xa10 [amdgpu]
Nov  8 17:54:48 kaveri kernel: [ 1084.245408]  ? amdgpu_gem_object_close+0x790/0x790 [amdgpu]
Nov  8 17:54:48 kaveri kernel: [ 1084.245410]  ? radix_tree_lookup+0x10/0x10
Nov  8 17:54:48 kaveri kernel: [ 1084.245413]  ? memset+0x1f/0x40
Nov  8 17:54:48 kaveri kernel: [ 1084.245416]  ? __inode_attach_wb+0xb90/0xb90
Nov  8 17:54:48 kaveri kernel: [ 1084.245420]  ? __check_object_size+0x217/0x4a0
Nov  8 17:54:48 kaveri kernel: [ 1084.245461]  ? amdgpu_gem_object_close+0x790/0x790 [amdgpu]
Nov  8 17:54:48 kaveri kernel: [ 1084.245472]  drm_ioctl_kernel+0x1ba/0x2c0 [drm]
Nov  8 17:54:48 kaveri kernel: [ 1084.245484]  ? drm_ioctl_permit+0x2b0/0x2b0 [drm]
Nov  8 17:54:48 kaveri kernel: [ 1084.245486]  ? get_device_system_crosststamp+0x850/0x850
Nov  8 17:54:48 kaveri kernel: [ 1084.245498]  drm_ioctl+0x720/0xa30 [drm]
Nov  8 17:54:48 kaveri kernel: [ 1084.245540]  ? amdgpu_gem_object_close+0x790/0x790 [amdgpu]
Nov  8 17:54:48 kaveri kernel: [ 1084.245551]  ? drm_getstats+0x20/0x20 [drm]
Nov  8 17:54:48 kaveri kernel: [ 1084.245555]  ? pipe_write+0x633/0xe30
Nov  8 17:54:48 kaveri kernel: [ 1084.245594]  amdgpu_drm_ioctl+0xcc/0x1a0 [amdgpu]
Nov  8 17:54:48 kaveri kernel: [ 1084.245598]  do_vfs_ioctl+0x18f/0x1270
Nov  8 17:54:48 kaveri kernel: [ 1084.245600]  ? ioctl_preallocate+0x2a0/0x2a0
Nov  8 17:54:48 kaveri kernel: [ 1084.245603]  ? __fget_light+0x2b0/0x3b0
Nov  8 17:54:48 kaveri kernel: [ 1084.245605]  ? iterate_fd+0x2a0/0x2a0
Nov  8 17:54:48 kaveri kernel: [ 1084.245607]  ? vfs_write+0x32d/0x450
Nov  8 17:54:48 kaveri kernel: [ 1084.245610]  ? do_page_fault+0x93/0x320
Nov  8 17:54:48 kaveri kernel: [ 1084.245612]  ? SyS_write+0x128/0x1e0
Nov  8 17:54:48 kaveri kernel: [ 1084.245614]  ? SyS_read+0x1e0/0x1e0
Nov  8 17:54:48 kaveri kernel: [ 1084.245616]  SyS_ioctl+0x74/0x80
Nov  8 17:54:48 kaveri kernel: [ 1084.245619]  entry_SYSCALL_64_fastpath+0x1e/0xa9
Nov  8 17:54:48 kaveri kernel: [ 1084.245621] RIP: 0033:0x7fa01b2f4dc7
Nov  8 17:54:48 kaveri kernel: [ 1084.245622] RSP: 002b:00007ffc3e617fb8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
Nov  8 17:54:48 kaveri kernel: [ 1084.245625] RAX: ffffffffffffffda RBX: 0000562a26dd0d80 RCX: 00007fa01b2f4dc7
Nov  8 17:54:48 kaveri kernel: [ 1084.245626] RDX: 00007ffc3e618000 RSI: 00000000c0206440 RDI: 0000000000000006
Nov  8 17:54:48 kaveri kernel: [ 1084.245627] RBP: 00007ffc3e617ef0 R08: 00007fa01b5ac158 R09: 0000000000000060
Nov  8 17:54:48 kaveri kernel: [ 1084.245629] R10: 0000000000000004 R11: 0000000000000246 R12: 0000562a26dcfea0
Nov  8 17:54:48 kaveri kernel: [ 1084.245630] R13: 0000000000000001 R14: 00000000000000c0 R15: 0000000000000000
Nov  8 17:54:48 kaveri kernel: [ 1084.245632] Code: 03 80 3c 02 00 0f 85 f8 00 00 00 49 c1 ec 03 48 b8 00 00 00 00 00 fc ff df 4d 89 7d 00 41 c6 04 04 f8 f0 ff 43 34 e9 49 fe ff ff <0f> 0b e8 5b 06 cf d0 e9 99 fb ff ff e8 51 06 cf d0 e9 2b fe ff 
Nov  8 17:54:48 kaveri kernel: [ 1084.245661] RIP: ttm_bo_add_to_lru+0x4fe/0x640 [ttm] RSP: ffff880225ce6d60
Nov  8 17:54:48 kaveri kernel: [ 1084.245676] ---[ end trace da3cb1c26802a840 ]---


[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/ttm: consistently use reservation_object_unlock

[Christian König]

Am 08.11.2017 um 18:37 schrieb Michel Dänzer:
> On 08/11/17 05:41 PM, Christian König wrote:
>> Am 08.11.2017 um 17:36 schrieb Michel Dänzer:
>>> On 08/11/17 03:59 PM, Christian König wrote:
>>>> Instead of having a pointless wrapper or call the underlying ww_mutex
>>>> function directly.
>>> Not sure I can agree it's pointless, since it recently took me a while
>>> to realize that unlocking bo->resv is essentially the same as
>>> unreserving the BO.
>> Ok in this case let's call this confusing. But yeah that
>> __ttm_bo_unreserv(bo), reservation_object_unlock(bo->resv) and
>> ww_mutex_unlock(&bo->resv->lock) are essentially the same thing is
>> exactly what I want to fix here.
> How about always using __ttm_bo_unreserve instead?

I actually want to get rid of __ttm_bo_reserve as well. Cause what is 
easier to understand:

__ttm_bo_reserve(bo, false, false, NULL) vs. 
reservation_object_lock(bo->resv);

__ttm_bo_reserve(bo, false, true, NULL) vs. 
reservation_object_trylock(bo->resv);

__ttm_bo_reserve(bo, true, false, NULL) vs. 
reservation_object_lock_interruptible(bo->resv);

> The first piglit run with this series applied hit the BUG_ON in
> ttm_bo_add_to_lru, see the attached dmesg excerpt. The machine hung,
> couldn't reboot cleanly. Haven't been able to reproduce it in three more
> attempts though, so I'm not sure it's caused by this series, but I don't
> remember seeing it before.

It's most likely caused by this change, I will take another look tomorrow.

> P.S. I just noticed I haven't had CONFIG_PROVE_LOCKING enabled in my
> kernels for a while, I'll enable it again. Any other options that should
> be enabled for testing?

kmemleak is quite nice to have available as well, but doesn't need to 
run automatically all the time.

Christian.

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 3/4] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional

[He, Roger]

Please see comments inline

-----Original Message-----
From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] 
Sent: Wednesday, November 08, 2017 11:00 PM
To: He, Roger <Hongbo.He@amd.com>; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: [PATCH 3/4] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional

Needed for the next patch.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 52 ++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 6f55310a9d09..d23592cfe42e 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -486,20 +486,21 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)  }
 
 /**
- * function ttm_bo_cleanup_refs_and_unlock
+ * function ttm_bo_cleanup_refs
  * If bo idle, remove from delayed- and lru lists, and unref.
  * If not idle, do nothing.
  *
  * Must be called with lru_lock and reservation held, this function
- * will drop both before returning.
+ * will drop the lru lock and optionally the reservation lock before returning.
  *
  * @interruptible         Any sleeps should occur interruptibly.
  * @no_wait_gpu           Never wait for gpu. Return -EBUSY instead.
+ * @unlock_resv           Unlock the reservation lock as well.
  */
 
-static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
-					  bool interruptible,
-					  bool no_wait_gpu)
+static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
+			       bool interruptible, bool no_wait_gpu,
+			       bool unlock_resv)
 {
 	struct ttm_bo_global *glob = bo->glob;
 	struct reservation_object *resv;
@@ -518,7 +519,8 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 	if (ret && !no_wait_gpu) {
 		long lret;
 
-		reservation_object_unlock(bo->resv);
+		if (unlock_resv)
+			reservation_object_unlock(bo->resv);
 		spin_unlock(&glob->lru_lock);
 
 		lret = reservation_object_wait_timeout_rcu(resv, true, @@ -531,19 +533,22 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 			return -EBUSY;
 
 		spin_lock(&glob->lru_lock);
-		ret = __ttm_bo_reserve(bo, false, true, NULL);
-
-		/*
-		 * We raced, and lost, someone else holds the reservation now,
-		 * and is probably busy in ttm_bo_cleanup_memtype_use.
-		 *
-		 * Even if it's not the case, because we finished waiting any
-		 * delayed destruction would succeed, so just return success
-		 * here.
-		 */
-		if (ret) {
-			spin_unlock(&glob->lru_lock);
-			return 0;
+		if (unlock_resv) {
+			ret = __ttm_bo_reserve(bo, false, true, NULL);
+			/*
+			 * We raced, and lost, someone else holds the reservation now,
+			 * and is probably busy in ttm_bo_cleanup_memtype_use.
+			 *
+			 * Even if it's not the case, because we finished waiting any
+			 * delayed destruction would succeed, so just return success
+			 * here.
+			 */
+			if (ret) {
+				spin_unlock(&glob->lru_lock);
+				return 0;
+			}
+		} else {
+			ret = 0;
 		}
 	}

[Roger]: 
	//Looks like we also need the condition adjudge here.  Otherwise, it will unlock the vm root bo reservation that is not what we want here I think.
	if (unlock_resv)     //need this condition here
		reservation_object_unlock(bo->resv);

	return 0;
}

 
@@ -600,8 +605,8 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all)
 		}
 
 		if (!ret)
-			ret = ttm_bo_cleanup_refs_and_unlock(entry, false,
-							     !remove_all);
+			ret = ttm_bo_cleanup_refs(entry, false, !remove_all,
+						  true);
 		else
 			spin_unlock(&glob->lru_lock);
 
@@ -770,8 +775,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 	kref_get(&bo->list_kref);
 
 	if (!list_empty(&bo->ddestroy)) {
-		ret = ttm_bo_cleanup_refs_and_unlock(bo, interruptible,
-						     no_wait_gpu);
+		ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu, true);
 		kref_put(&bo->list_kref, ttm_bo_release_list);
 		return ret;
 	}
@@ -1735,7 +1739,7 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
 	kref_get(&bo->list_kref);
 
 	if (!list_empty(&bo->ddestroy)) {
-		ret = ttm_bo_cleanup_refs_and_unlock(bo, false, false);
+		ret = ttm_bo_cleanup_refs(bo, false, false, true);
 		kref_put(&bo->list_kref, ttm_bo_release_list);
 		return ret;
 	}
--
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel