<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style type="text/css" style="display:none;"><!-- P {margin-top:0;margin-bottom:0;} --></style>
</head>
<body dir="ltr">
<div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr">
<p style="margin-top:0;margin-bottom:0">Hi Christian</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">I use blow patch to capture the incorrect case :</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0"></p>
<div>@@ -267,12 +267,21 @@ void reservation_object_add_excl_fence(struct reservation_object *obj,</div>
<div> write_seqcount_end(&obj->seq);</div>
<div> preempt_enable();</div>
<div> </div>
<div>- /* inplace update, no shared fences */</div>
<div>- while (i--)</div>
<div>- dma_fence_put(rcu_dereference_protected(old->shared[i],</div>
<div>- reservation_object_held(obj)));</div>
<div>+ /* inplace update, no shared fences continue after all shared signaled */</div>
<div>+ while (i--) {</div>
<div>+ struct dma_fence *f = rcu_dereference_protected(old->shared[i],</div>
<div>+ reservation_object_held(obj));</div>
<div>+ if (!dma_fence_is_signaled(f))</div>
<div>+ BUG();</div>
<div>+</div>
<div>+ dma_fence_put(f);</div>
<div>+ /* better assign shared[i] with NULL for sure */</div>
<div>+ rcu_assign_pointer(old->shared[i], NULL);</div>
<div>+ }</div>
<div> </div>
<div> dma_fence_put(old_fence);</div>
<div>+</div>
<div>+</div>
<div> }</div>
<div> EXPORT_SYMBOL(reservation_object_add_excl_fence);</div>
<div><br>
</div>
<div>and I hit this BUG() during test:</div>
<div><br>
</div>
<div>
<div>[ 105.244816] [drm] Initialized amdgpu 3.24.0 20150101 for 0000:00:08.0 on minor 0</div>
<div>[ 105.623332] ------------[ cut here ]------------</div>
<div>[ 105.623335] kernel BUG at drivers/dma-buf/reservation.c:275!</div>
<div>[ 105.624470] invalid opcode: 0000 [#1] SMP</div>
<div>[ 105.624915] Modules linked in: amdgpu chash gpu_sched ttm drm_kms_helper drm i2c_algo_bit fb_sys_fops syscopyarea sysfillrect sysimgblt snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hda_core snd_hwdep crct10dif_pclmul crc32_pclmul snd_pcm ghash_clmulni_intel
pcbc snd_seq_midi snd_seq_midi_event snd_rawmidi aesni_intel aes_x86_64 crypto_simd glue_helper cryptd snd_seq snd_seq_device snd_timer serio_raw snd soundcore i2c_piix4 mac_hid parport_pc ppdev lp parport autofs4 8139too psmouse 8139cp mii floppy pata_acpi</div>
<div>[ 105.630547] CPU: 3 PID: 1216 Comm: 3dmark Not tainted 4.13.0-debug #1</div>
<div>[ 105.631762] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014</div>
<div>[ 105.633528] task: ffff8f8a6a165a00 task.stack: ffffb1204159c000</div>
<div>[ 105.634676] RIP: 0010:reservation_object_add_excl_fence+0x9c/0xf0</div>
<div>[ 105.635824] RSP: 0018:ffffb1204159f9f0 EFLAGS: 00010246</div>
<div>[ 105.636805] RAX: 0000000000000000 RBX: ffff8f8a64bee760 RCX: ffff8f8a6bfa2f50</div>
<div>[ 105.638123] RDX: ffff8f8a6bfa6770 RSI: ffff8f8a64bee660 RDI: ffff8f8a6635f628</div>
<div>[ 105.639440] RBP: ffffb1204159fa18 R08: 0000000000000000 R09: 0000000000000001</div>
<div>[ 105.640702] R10: ffffb1204159f808 R11: 0000000000000003 R12: 0000000000000000</div>
<div>[ 105.641947] R13: 0000000000000000 R14: ffff8f8a6d0f0200 R15: ffff8f8a64beee60</div>
<div>[ 105.643165] FS: 00007fd13c73d940(0000) GS:ffff8f8a76d80000(0000) knlGS:0000000000000000</div>
<div>[ 105.644573] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033</div>
<div>[ 105.646482] CR2: 00007fd13c6fd000 CR3: 00000001a2a58000 CR4: 00000000001406e0</div>
<div>[ 105.648467] Call Trace:</div>
<div>[ 105.652480] amdgpu_bo_do_create+0x3a1/0x540 [amdgpu]</div>
<div>[ 105.654233] amdgpu_bo_create+0x3a/0x220 [amdgpu]</div>
<div>[ 105.655956] amdgpu_vm_alloc_levels.isra.14+0x1dc/0x370 [amdgpu]</div>
<div>[ 105.657641] amdgpu_vm_alloc_pts+0x49/0x70 [amdgpu]</div>
<div>[ 105.659155] amdgpu_gem_va_ioctl+0x365/0x520 [amdgpu]</div>
<div>[ 105.660698] ? amdgpu_gem_create_ioctl+0x19a/0x280 [amdgpu]</div>
<div>[ 105.662515] ? amdgpu_gem_metadata_ioctl+0x1c0/0x1c0 [amdgpu]</div>
<div>[ 105.664203] drm_ioctl_kernel+0x69/0xb0 [drm]</div>
<div>[ 105.665491] ? drm_ioctl_kernel+0x69/0xb0 [drm]</div>
<div>[ 105.666959] drm_ioctl+0x2d2/0x390 [drm]</div>
<div>[ 105.668373] ? amdgpu_gem_metadata_ioctl+0x1c0/0x1c0 [amdgpu]</div>
<div>[ 105.670056] ? call_rcu_sched+0x1d/0x20</div>
<div>[ 105.671516] ? put_object+0x26/0x30</div>
<div>[ 105.672741] ? __delete_object+0x39/0x50</div>
<div>[ 105.674048] amdgpu_drm_ioctl+0x4c/0x80 [amdgpu]</div>
<div>[ 105.675551] do_vfs_ioctl+0x92/0x5a0</div>
<div>[ 105.676874] ? kvm_sched_clock_read+0x1e/0x30</div>
<div>[ 105.678276] ? sched_clock+0x9/0x10</div>
<div>[ 105.679553] ? get_vtime_delta+0x99/0xc0</div>
<div>[ 105.681007] SyS_ioctl+0x79/0x90</div>
<div>[ 105.684574] do_syscall_64+0x6e/0x150</div>
<div>[ 105.685910] entry_SYSCALL64_slow_path+0x25/0x25</div>
<div>[ 105.687354] RIP: 0033:0x7fd13b25ff47</div>
<div>[ 105.688666] RSP: 002b:00007fff5422b2c8 EFLAGS: 00000202 ORIG_RAX: 0000000000000010</div>
<div>[ 105.691268] RAX: ffffffffffffffda RBX: 0000000001886130 RCX: 00007fd13b25ff47</div>
<div>[ 105.693148] RDX: 00007fff5422b390 RSI: 00000000c0286448 RDI: 0000000000000007</div>
<div>[ 105.695003] RBP: 00007fff5422b300 R08: 0000000300000000 R09: 000000000000000e</div>
<div>[ 105.696774] R10: 0000000001887c28 R11: 0000000000000202 R12: 000000000188a430</div>
<div>[ 105.698459] R13: 0000000001886130 R14: 00007fff5422b638 R15: 0000000000000000</div>
<div>[ 105.700168] Code: 74 3f 41 89 c4 45 89 e5 4b 8b 5c ee 18 48 8b 43 48 a8 01 75 cc 48 8b 43 08 48 8b 40 18 48 85 c0 74 09 48 89 df ff d0 84 c0 75 0c <0f> 0b 48 89 df e8 2a ed ff ff eb b4 48 89 df e8 80 ef ff ff eb </div>
<div>[ 105.704982] RIP: reservation_object_add_excl_fence+0x9c/0xf0 RSP: ffffb1204159f9f0</div>
<div><br>
</div>
<br>
</div>
<div>the assumption that all shared fences should be signaled before adding excl fence looks not 100% guaranteed in LKG, </div>
<p></p>
<p style="margin-top:0;margin-bottom:0">Going to take a deep look ...</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">/Monk</p>
<br>
<br>
<div style="color: rgb(0, 0, 0);">
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> Liu, Monk<br>
<b>Sent:</b> Tuesday, March 6, 2018 6:47 PM<br>
<b>To:</b> Koenig, Christian; Chris Wilson; dri-devel@lists.freedesktop.org<br>
<b>Subject:</b> Re: reservation questions</font>
<div> </div>
</div>
<div dir="ltr">
<div id="x_divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:#000000; font-family:Calibri,Helvetica,sans-serif">
<p style="margin-top:0; margin-bottom:0">ok, that's good point ... </p>
</div>
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="x_divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>From:</b> Koenig, Christian<br>
<b>Sent:</b> Tuesday, March 6, 2018 6:42:44 PM<br>
<b>To:</b> Liu, Monk; Chris Wilson; dri-devel@lists.freedesktop.org<br>
<b>Subject:</b> Re: reservation questions</font>
<div> </div>
</div>
<div style="background-color:#FFFFFF">
<div class="x_x_moz-cite-prefix">Hi Monk,<br>
<br>
that is to remove the problem that allocating memory could fail.<br>
<br>
E.g. we only add the fence after sending the command to the hardware, so there is now way back and we need to add the fence or break memory management.<br>
<br>
So reservation_object_reserve_shared() makes sure there is a free fence slot *before* we start to prepare things for the hardware.<br>
<br>
Regards,<br>
Christian.<br>
<br>
Am 06.03.2018 um 11:19 schrieb Liu, Monk:<br>
</div>
<blockquote type="cite"><style type="text/css" style="display:none">
<!--
p
{margin-top:0;
margin-bottom:0}
-->
</style>
<div id="x_x_divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:#000000; font-family:Calibri,Helvetica,sans-serif">
<p style="margin-top:0; margin-bottom:0">Hi Chris<span style="font-size:12pt"> </span></p>
<p style="margin-top:0; margin-bottom:0"><span style="font-size:12pt"><br>
</span></p>
<p style="margin-top:0; margin-bottom:0">another question is why we not just call "<span style="color:rgb(220,220,170); background-color:rgb(30,30,30); font-family:"Droid Sans Mono",monospace,monospace,"Droid Sans Fallback"; font-size:14px; white-space:pre">reservation_object_reserve_shared"</span></p>
<p style="margin-top:0; margin-bottom:0"><span style="color:rgb(0,0,0); background-color:rgb(255,255,255); font-family:"Droid Sans Mono",monospace,monospace,"Droid Sans Fallback"; font-size:14px; white-space:pre">during below add_shared_fence function, so the
BUG_ON() could be avoided and caller won't need</span></p>
<p style="margin-top:0; margin-bottom:0"><span style="color:rgb(0,0,0); background-color:rgb(255,255,255); font-family:"Droid Sans Mono",monospace,monospace,"Droid Sans Fallback"; font-size:14px; white-space:pre">to worry when and how much time it should call
reserve_shared() ?</span></p>
<p style="margin-top:0; margin-bottom:0"><span style="color:rgb(0,0,0); background-color:rgb(255,255,255); font-family:"Droid Sans Mono",monospace,monospace,"Droid Sans Fallback"; font-size:14px; white-space:pre"></span></p>
<p style="margin-top:0; margin-bottom:0"><span style="color:rgb(0,0,0); background-color:rgb(255,255,255); font-family:"Droid Sans Mono",monospace,monospace,"Droid Sans Fallback"; font-size:14px; white-space:pre">thanks !</span></p>
<p style="margin-top:0; margin-bottom:0"><span style="font-size:12pt"><br>
</span></p>
<p style="margin-top:0; margin-bottom:0"><span style="font-size:12pt"></span></p>
<div style="color:rgb(212,212,212); background-color:rgb(30,30,30); font-family:"Droid Sans Mono",monospace,monospace,"Droid Sans Fallback"; font-size:14px; line-height:19px; white-space:pre">
<div><span style="color:#569cd6">void</span> <span style="color:#dcdcaa">reservation_object_add_shared_fence</span>(<span style="color:#569cd6">struct</span> reservation_object *obj,</div>
<div> <span style="color:#569cd6">struct</span> dma_fence *fence)</div>
<div>{</div>
<div> <span style="color:#569cd6">struct</span> reservation_object_list *old, *fobj = obj-><span style="color:#9cdcfe">staged</span>;</div>
<div> old = <span style="color:#dcdcaa">reservation_object_get_list</span>(obj);</div>
<div> obj-><span style="color:#9cdcfe">staged</span> = <span style="color:#569cd6">
NULL</span>;</div>
<div> <span style="color:#c586c0">if</span> (!fobj) {</div>
<div> <span style="color:#dcdcaa">BUG_ON</span>(old-><span style="color:#9cdcfe">shared_count</span> >= old-><span style="color:#9cdcfe">shared_max</span>);</div>
<div> <span style="color:#dcdcaa">reservation_object_add_shared_inplace</span>(obj, old, fence);</div>
<div> } <span style="color:#c586c0">else</span></div>
<div> <span style="color:#dcdcaa">reservation_object_add_shared_replace</span>(obj, old, fobj, fence);</div>
<div>}</div>
<div><span style="color:#dcdcaa">EXPORT_SYMBOL</span>(reservation_object_add_shared_fence);</div>
<div></div>
<div></div>
</div>
</div>
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="x_x_divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>From:</b> Chris Wilson
<a class="x_x_moz-txt-link-rfc2396E" href="mailto:chris@chris-wilson.co.uk"><chris@chris-wilson.co.uk></a><br>
<b>Sent:</b> Tuesday, March 6, 2018 6:10:21 PM<br>
<b>To:</b> Liu, Monk; <a class="x_x_moz-txt-link-abbreviated" href="mailto:dri-devel@lists.freedesktop.org">
dri-devel@lists.freedesktop.org</a>; Koenig, Christian<br>
<b>Subject:</b> Re: reservation questions</font>
<div> </div>
</div>
<div class="x_x_BodyFragment"><font size="2"><span style="font-size:11pt">
<div class="x_x_PlainText">Quoting Liu, Monk (2018-03-06 09:45:19)<br>
> call reservation_object_add_excl_fence,<br>
> it set obj->fence->shared_count to 0, and put all shared fence from obj->fence<br>
> without waiting signaling.<br>
> (this action looks inappropriate, I think at least before put all those shared<br>
> fences<br>
> we should dma_wait_fence() on them to make sure they are signaled)<br>
<br>
No. Serialisation of resv updates are handled by the caller, the fences<br>
are ordered asynchronously so the wait is implicit in the construction.<br>
(I.e. before the excl fence can be signaled, all of the earlier shared<br>
fences must be signaled. You can even say before the operation that the<br>
excl fence signals completion of can begin, all the shared fences must<br>
have been signaled. But that is all implicit so that we can do it<br>
asynchronously.)<br>
<br>
> call reservation_object_reserve_shared,<br>
> this time obj->staged isn't NULL, and it is freed (nothing bad now<br>
> since obj->fence points to other place),<br>
> and obj->staged set to NULL,<br>
> <br>
> call reservation_object_add_shared_fence,<br>
> this time should going through reservation_object_add_shared_inplace,<br>
> But BUG_ON(old->shared_count >= old->shared_max) will hit !<br>
<br>
How? You only free staged iff shared_count < shared_max.<br>
<br>
You've reminded me that we should cover all this with a bunch of<br>
selftests.<br>
-Chris<br>
</div>
</span></font></div>
</blockquote>
<br>
</div>
</div>
</div>
</div>
</body>
</html>