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

2017-11-08 Thread 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(>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 2/4] drm/ttm: consistently use reservation_object_unlock

2017-11-08 Thread 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(>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
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:  [#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   O4.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: 880233de6c80 task.stack: 880225ce
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:880225ce6d60 EFLAGS: 00010206
Nov  8 17:54:48 kaveri kernel: [ 1084.244783] RAX: dc00 RBX: 88038de24cd8 RCX: 
Nov  8 17:54:48 kaveri kernel: [ 1084.244784] RDX: 110044b9cdb4 RSI:  RDI: 88038de24ce0
Nov  8 17:54:48 kaveri kernel: [ 1084.244786] RBP: 110044b9cdb0 R08: 07cbd06c R09: 
Nov  8 17:54:48 kaveri kernel: [ 1084.244788] R10: 93daac8e R11: 00057de4 R12: 110044b9cdb4
Nov  8 17:54:48 kaveri kernel: [ 1084.244791] R13: 880388033c88 R14: 88038de24d88 R15: 88038a452718
Nov  8 17:54:48 kaveri kernel: [ 1084.244794] FS:  7fa01e650300() GS:8803ae74() knlGS:
Nov  8 17:54:48 kaveri kernel: [ 1084.244797] CS:  0010 DS:  ES:  CR0: 80050033
Nov  8 17:54:48 kaveri kernel: [ 1084.244799] CR2: 7faecd0f06f0 CR3: 00038b7d3000 CR4: 003406e0
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]

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

2017-11-08 Thread 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(>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.

___
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

2017-11-08 Thread 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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2017-11-08 Thread Christian König 
Instead of having a pointless wrapper or call the underlying ww_mutex
function directly.

Signed-off-by: Christian König 
---
 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 != >ttm_resv)
reservation_object_unlock(>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(>resv->lock);
+
+   reservation_object_unlock(bo->resv);
spin_unlock(>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(>ddestroy))) {
-   __ttm_bo_unreserve(bo);
+   reservation_object_unlock(bo->resv);
spin_unlock(>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(>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(>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(>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(>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(>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(>resv->lock);
-}
-
-/**
  * ttm_bo_unreserve
  *
  * @bo: A pointer to a struct ttm_buffer_object.
@@ -966,7 +954,7