<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <style type="text/css" style="display:none;"></style> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr"> your patch looks good, do you think we should also do this: <div> void amd_sched_fence_scheduled(struct amd_sched_fence *fence) { - int ret = fence_signal(&fence->scheduled); + int ret; + + fence_get(&fence->scheduled;) + ret = fence_signal(&fence->scheduled); if (!ret) FENCE_TRACE(&fence->scheduled, "signaled from irq context\n"); else FENCE_TRACE(&fence->scheduled, "was already signaled\n"); + fence_put(&fence->scheduled); }</div> </div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Koenig, Christian Sent: Friday, October 13, 2017 5:00:27 PM To: Liu, Monk; Nicolai Hähnle; amd-gfx@lists.freedesktop.org Subject: Re: regression with d6c650c0a8f6f671e49553725e1db541376d95f2 <div> </div> </div> <div> <div class="moz-cite-prefix"> <blockquote type="cite"> There is chance that free_job() called before that “trace_amd_sched_process_job”, correct? </blockquote> Correct, but that is harmless. Take a look what trace_amd_sched_process_job actually does, it just prints the pointer of the fence structure (but the pointer might be stale at this point). Nevertheless you are right that this is really ugly. How about the attached patch to fix the issue? Regards, Christian. Am 13.10.2017 um 10:51 schrieb Liu, Monk: </div> <blockquote type="cite" cite="mid:BLUPR12MB044908B9AECE89D7E942EC7F84480@BLUPR12MB0449.namprd12.prod.outlook.com"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> <div class="WordSection1"> The free_job() is called in sched_job_finish() which is queued on a WORK and scheduled from that “amd_sched_fence_finished()”<o:p></o:p> So the finishing timing of free_job() is asynchronized with sched_process_job()<o:p></o:p> <o:p> </o:p> There is chance that free_job() called before that “trace_amd_sched_process_job”, correct?<o:p></o:p> And if so the s_fence referred by it maybe a wild pointer <o:p></o:p> <o:p> </o:p> <o:p> </o:p> BR Monk<o:p></o:p> <o:p> </o:p> <o:p> </o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm"> From: Liu, Monk Sent: 2017年10月13日 16:49 To: Koenig, Christian <a class="moz-txt-link-rfc2396E" href="mailto:Christian.Koenig@amd.com"> <Christian.Koenig@amd.com></a>; Nicolai Hähnle <a class="moz-txt-link-rfc2396E" href="mailto:nhaehnle@gmail.com"> <nhaehnle@gmail.com></a>; <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org"> amd-gfx@lists.freedesktop.org</a> Subject: RE: regression with d6c650c0a8f6f671e49553725e1db541376d95f2<o:p></o:p> </div> </div> <o:p> </o:p> No that’s not true<o:p></o:p> <o:p> </o:p> The free_job() is called in sched_job_finish() which is queued on a WORK and scheduled from that “amd_sched_fence_finished()”<o:p></o:p> So the finishing timing of free_job() is asynchronized with sched_process_job()<o:p></o:p> <o:p> </o:p> How can you sure free_job() must before “trace_amd_sched_process_job” ?<o:p></o:p> <o:p> </o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm"> From: Koenig, Christian Sent: 2017年10月13日 16:43 To: Liu, Monk <<a href="mailto:Monk.Liu@amd.com" moz-do-not-send="true">Monk.Liu@amd.com</a>>; Nicolai Hähnle <<a href="mailto:nhaehnle@gmail.com" moz-do-not-send="true">nhaehnle@gmail.com</a>>; <a href="mailto:amd-gfx@lists.freedesktop.org" moz-do-not-send="true">amd-gfx@lists.freedesktop.org</a> Subject: Re: regression with d6c650c0a8f6f671e49553725e1db541376d95f2<o:p></o:p> </div> </div> <o:p> </o:p> <div> The free_job() callback is called only way after the job has finished. That is one change actually made by you to the code :) Christian. Am 13.10.2017 um 10:39 schrieb Liu, Monk:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> I doubt it would always work fine…<o:p></o:p> <o:p></o:p> First, we have FENCE_TRACE reference s_fence->finished after “fence_signal(&fence->finished)”<o:p></o:p> Second, we have trace_amd_sched_proess_job(s_fence) after “amd_sched_fence_finished()”,<o:p></o:p> <o:p></o:p> If you put the finished before free_job() and by coincidence the job_finish() get very soon executed you’ll have odds to hit wild pointer on above two cases<o:p></o:p> <o:p></o:p> BR Monk<o:p></o:p> <o:p></o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm"> From: Koenig, Christian Sent: 2017年10月13日 16:17 To: Liu, Monk <a href="mailto:Monk.Liu@amd.com" moz-do-not-send="true"><Monk.Liu@amd.com></a>; Nicolai Hähnle <a href="mailto:nhaehnle@gmail.com" moz-do-not-send="true"><nhaehnle@gmail.com></a>; <a href="mailto:amd-gfx@lists.freedesktop.org" moz-do-not-send="true">amd-gfx@lists.freedesktop.org</a> Subject: Re: regression with d6c650c0a8f6f671e49553725e1db541376d95f2<o:p></o:p> </div> </div> <o:p></o:p> <div> Yeah, that change is actually incorrect and should be reverted. What we really need to do is remove dropping sched_job->s_fence from amd_sched_process_job() into amd_sched_job_finish() directly before the call to free_job(). Regards, Christian. Am 13.10.2017 um 09:24 schrieb Liu, Monk:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <div id="divtagdefaultwrapper"> <div> commit d6c650c0a8f6f671e49553725e1db541376d95f2 Author: Nicolai Hähnle <a href="mailto:nicolai.haehnle@amd.com" moz-do-not-send="true"> <nicolai.haehnle@amd.com></a> @@ -611,6 +611,10 @@ static int amd_sched_main(void *param) fence = sched->ops->run_job(sched_job); amd_sched_fence_scheduled(s_fence); + + /* amd_sched_process_job drops the job's reference of the fence. */ + sched_job->s_fence = NULL; + if (fence) { s_fence->parent = dma_fence_get(fence); r = dma_fence_add_callback(fence, &s_fence->cb,<o:p></o:p> </div> Hi Nicolai<o:p></o:p> <o:p></o:p> with this patch, you will break "amdgpu_sched_hw_job_reset()"routine:<o:p></o:p> <div> <o:p></o:p> <div> void <o:p></o:p> amd_sched_hw_job_reset(struct amd_gpu_scheduler<o:p></o:p> *sched)<o:p></o:p> </div> <o:p></o:p> <div> {<o:p></o:p> </div> <o:p></o:p> <div> struct amd_sched_job<o:p></o:p> *s_job;<o:p></o:p> </div> <o:p></o:p> <o:p></o:p> <o:p></o:p> <div> spin_lock(&sched->job_list_lock);<o:p></o:p> </div> <o:p></o:p> <div> list_for_each_entry_reverse(s_job,<o:p></o:p> &sched->ring_mirror_list, node) {<o:p></o:p> </div> <o:p></o:p> <div> if (s_job->s_fence->parent<o:p></o:p> &&<o:p></o:p> </div> <o:p></o:p> <div> fence_remove_callback(s_job->s_fence->parent,<o:p></o:p> </div> <o:p></o:p> <div> &s_job->s_fence->cb))<o:p></o:p> {<o:p></o:p> </div> <o:p></o:p> <div> fence_put(s_job->s_fence->parent);<o:p></o:p> </div> <o:p></o:p> <div> s_job->s_fence->parent<o:p></o:p> = <o:p></o:p> NULL;<o:p></o:p> </div> <o:p></o:p> <div> atomic_dec(&sched->hw_rq_count);<o:p></o:p> </div> <o:p></o:p> <div> }<o:p></o:p> </div> <o:p></o:p> <div> }<o:p></o:p> </div> <o:p></o:p> <div> spin_unlock(&sched->job_list_lock);<o:p></o:p> </div> <o:p></o:p> <div> }<o:p></o:p> </div> <o:p></o:p> </div> <o:p></o:p> see that without sched_job->s_fence, you cannot remove the callback from its hw fence,<o:p></o:p> <o:p></o:p> any idea??<o:p></o:p> <o:p></o:p> BR Monk<o:p></o:p> <o:p></o:p> <o:p></o:p> </div> </blockquote> <o:p></o:p> </blockquote> <o:p> </o:p> </div> </blockquote> </div> </body> </html>