<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body bgcolor="white" lang="ZH-CN" link="blue" vlink="purple"> <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 <Christian.Koenig@amd.com>; Nicolai Hähnle <nhaehnle@gmail.com>; amd-gfx@lists.freedesktop.org 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">Monk.Liu@amd.com</a>>; Nicolai Hähnle <<a href="mailto:nhaehnle@gmail.com">nhaehnle@gmail.com</a>>; <a 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> <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"><Monk.Liu@amd.com></a>; Nicolai Hähnle <a href="mailto:nhaehnle@gmail.com"><nhaehnle@gmail.com></a>; <a 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> <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"><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> </body> </html>