<html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> Yeah, but wouldn't it be feasible to extend the existing function with parameters what to do like Andrey suggested? (It's possible that I just missed the response for this). Christian. <div class="moz-cite-prefix">Am 10.03.21 um 15:05 schrieb Zhang, Jack (Jian): </div> <blockquote type="cite" cite="mid:DM5PR1201MB0204120DC4A44A5A32196901BB919@DM5PR1201MB0204.namprd12.prod.outlook.com"> [AMD Official Use Only - Internal Distribution Only] <div> <div dir="auto" style="direction: ltr; margin: 0; padding: 0; font-family: sans-serif; font-size: 11pt; color: black; "> </div> <div dir="auto" style="direction: ltr; margin: 0; padding: 0; font-family: sans-serif; font-size: 11pt; color: black; "> As Monk previously explained in another mail session. Because the drm_sched_resubmit_jobs submit all jobs which does not meet our needs. </div> <div dir="auto" style="direction: ltr; margin: 0; padding: 0; font-family: sans-serif; font-size: 11pt; color: black; "> </div> <div dir="auto" style="direction: ltr; margin: 0; padding: 0; font-family: sans-serif; font-size: 11pt; color: black; "> We need a new APi drm_sched_resubmit_jobs2() </div> <div dir="auto" style="direction: ltr; margin: 0; padding: 0; font-family: sans-serif; font-size: 11pt; color: black; "> </div> <div dir="auto" style="direction: ltr; margin: 0px; padding: 0px; font-family: sans-serif; font-size: 11pt; color: black; text-align: left;"> to submit the first job of a ring. (also we can leverage the error handling for Null or Error fence inside this function ). </div> <div dir="auto" style="direction: ltr; margin: 0px; padding: 0px; font-family: sans-serif; font-size: 11pt; color: black; text-align: left;"> </div> <div dir="auto" style="direction: ltr; margin: 0px; padding: 0px; font-family: sans-serif; font-size: 11pt; color: black; text-align: left;"> It seems clean to use this jobs2 function than writing the run_job + some error handling in the main logic. </div> <div dir="auto" style="direction: ltr; margin: 0px; padding: 0px; font-family: sans-serif; font-size: 11pt; color: black; text-align: left;"> </div> <div dir="auto" style="direction: ltr; margin: 0px; padding: 0px; font-family: sans-serif; font-size: 11pt; color: black; text-align: left;"> </div> <div dir="auto" style="direction: ltr; margin: 0px; padding: 0px; font-family: sans-serif; font-size: 11pt; color: black; text-align: left;"> Thanks， /Jack </div> <div dir="auto" style="direction: ltr; margin: 0px; padding: 0px; font-family: sans-serif; font-size: 11pt; color: black; text-align: left;"> </div> <div dir="auto" style="direction: ltr; margin: 0px; padding: 0px; font-family: sans-serif; font-size: 11pt; color: black; text-align: left;"> </div> <div dir="auto" style="direction: ltr; margin: 0px; padding: 0px; font-family: sans-serif; font-size: 11pt; color: black; text-align: left;"> </div> <div dir="auto" style="direction: ltr; margin: 0; padding: 0; font-family: sans-serif; font-size: 11pt; color: black; "> </div> <div id="id-f87222bd-c403-491c-81fb-bdf2e154f36d" class="ms-outlook-mobile-reference-message"> <div style="font-family: sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg">发件人: Koenig, Christian <a class="moz-txt-link-rfc2396E" href="mailto:Christian.Koenig@amd.com"><Christian.Koenig@amd.com></a> 发送时间: 2021年3月10日星期三下午9:49 收件人: Zhang, Jack (Jian); <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a>; Grodzovsky, Andrey; Liu, Monk; Deng, Emily 主题: Re: [PATCH v4] drm/amd/amdgpu implement tdr advanced mode </div> <meta content="text/html; charset=utf-8"> Andrey needs to review the reste, but essentially I don't see the reason why you need this drm_sched_resubmit_jobs2(). Christian. <div class="moz-cite-prefix">Am 10.03.21 um 14:36 schrieb Zhang, Jack (Jian): </div> <blockquote type="cite"> [AMD Official Use Only - Internal Distribution Only] <div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> Thanks Christian， just did the checkpatch scan. Please see the V6 patch </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> /Jack </div> <div dir="auto" style="direction:ltr; margin:0px; padding:0px; font-family:sans-serif; font-size:11pt; color:black; text-align:left"> </div> <div dir="auto" style="direction:ltr; margin:0; padding:0; font-family:sans-serif; font-size:11pt; color:black"> </div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="divRplyFwdMsg" dir="ltr">From: Koenig, Christian <a class="moz-txt-link-rfc2396E" href="mailto:Christian.Koenig@amd.com" moz-do-not-send="true"><Christian.Koenig@amd.com></a> Sent: Wednesday, March 10, 2021 8:54:53 PM To: Zhang, Jack (Jian) <a class="moz-txt-link-rfc2396E" href="mailto:Jack.Zhang1@amd.com" moz-do-not-send="true"> <Jack.Zhang1@amd.com></a>; <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org" moz-do-not-send="true"> amd-gfx@lists.freedesktop.org</a> <a class="moz-txt-link-rfc2396E" href="mailto:amd-gfx@lists.freedesktop.org" moz-do-not-send="true"> <amd-gfx@lists.freedesktop.org></a>; Grodzovsky, Andrey <a class="moz-txt-link-rfc2396E" href="mailto:Andrey.Grodzovsky@amd.com" moz-do-not-send="true"> <Andrey.Grodzovsky@amd.com></a>; Liu, Monk <a class="moz-txt-link-rfc2396E" href="mailto:Monk.Liu@amd.com" moz-do-not-send="true"> <Monk.Liu@amd.com></a>; Deng, Emily <a class="moz-txt-link-rfc2396E" href="mailto:Emily.Deng@amd.com" moz-do-not-send="true"> <Emily.Deng@amd.com></a> Subject: Re: [PATCH v4] drm/amd/amdgpu implement tdr advanced mode <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">Am 10.03.21 um 12:19 schrieb Jack Zhang: > [Why] > Previous tdr design treats the first job in job_timeout as the bad job. > But sometimes a later bad compute job can block a good gfx job and > cause an unexpected gfx job timeout because gfx and compute ring share > internal GC HW mutually. > > [How] > This patch implements an advanced tdr mode.It involves an additinal > synchronous pre-resubmit step(Step0 Resubmit) before normal resubmit > step in order to find the real bad job. > > 1. At Step0 Resubmit stage, it synchronously submits and pends for the > first job being signaled. If it gets timeout, we identify it as guilty > and do hw reset. After that, we would do the normal resubmit step to > resubmit left jobs. > > 2. Re-insert Bailing job to mirror_list, and leave it to be handled by > the main reset thread. > > 3. For whole gpu reset(vram lost), do resubmit as the old way. > > Signed-off-by: Jack Zhang <a class="moz-txt-link-rfc2396E" href="mailto:Jack.Zhang1@amd.com" moz-do-not-send="true"> <Jack.Zhang1@amd.com></a> > Change-Id: I408357f10b9034caaa1b83610e19e514c5fbaaf2 > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 73 ++++++++++++++++++++- > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- > drivers/gpu/drm/scheduler/sched_main.c | 75 ++++++++++++++++++++++ > include/drm/gpu_scheduler.h | 2 + > 4 files changed, 148 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index e247c3a2ec08..02056355a055 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -4639,7 +4639,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev, > int i, r = 0; > bool need_emergency_restart = false; > bool audio_suspended = false; > - > + int tmp_vram_lost_counter; Please keep the empoty line between declaration and code. In general give that patch a pass with checkpath.pl since there are a couple of other place which needs fixing at first glance. Christian. > /* > * Special case: RAS triggered and full reset isn't supported > */ > @@ -4689,9 +4689,14 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev, > dev_info(adev->dev, "Bailing on TDR for s_job:%llx, as another already in progress", > job ? job->base.id : -1); > > - /* even we skipped this reset, still need to set the job to guilty */ > - if (job) > + if (job) { > + /* re-insert node to avoid memory leak */ > + spin_lock(&job->base.sched->job_list_lock); > + list_add(&job->base.node, &job->base.sched->pending_list); > + spin_unlock(&job->base.sched->job_list_lock); > + /* even we skipped this reset, still need to set the job to guilty */ > drm_sched_increase_karma(&job->base); > + } > goto skip_recovery; > } > > @@ -4788,6 +4793,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev, > } > } > > + tmp_vram_lost_counter = atomic_read(&((adev)->vram_lost_counter)); > /* Actual ASIC resets if needed.*/ > /* TODO Implement XGMI hive reset logic for SRIOV */ > if (amdgpu_sriov_vf(adev)) { > @@ -4805,6 +4811,67 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev, > /* Post ASIC reset for all devs .*/ > list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) { > > + if (amdgpu_gpu_recovery == 2 && > + !(tmp_vram_lost_counter < atomic_read(&adev->vram_lost_counter))) { > + > + for (i = 0; i < AMDGPU_MAX_RINGS; ++i) { > + struct amdgpu_ring *ring = tmp_adev->rings[i]; > + int ret = 0; > + struct drm_sched_job *s_job; > + > + if (!ring || !ring->sched.thread) > + continue; > + > + /* No point to resubmit jobs if we didn't HW reset*/ > + if (!tmp_adev->asic_reset_res && !job_signaled) { > + > + s_job = list_first_entry_or_null(&ring->sched.pending_list, struct drm_sched_job, list); > + if (s_job == NULL) > + continue; > + > + /* clear job's guilty and depend the folowing step to decide the real one */ > + drm_sched_reset_karma(s_job); > + drm_sched_resubmit_jobs2(&ring->sched, 1); > + ret = dma_fence_wait_timeout(s_job->s_fence->parent, false, ring->sched.timeout); > + > + if (ret == 0) { /* timeout */ > + DRM_ERROR("Found the real bad job! ring:%s, job_id:%llx\n", ring->sched.name, s_job->id); > + /* set guilty */ > + drm_sched_increase_karma(s_job); > + > + /* do hw reset */ > + if (amdgpu_sriov_vf(adev)) { > + amdgpu_virt_fini_data_exchange(adev); > + r = amdgpu_device_reset_sriov(adev, false); > + if (r) > + adev->asic_reset_res = r; > + } else { > + r = amdgpu_do_asic_reset(hive, device_list_handle, &need_full_reset, false); > + if (r && r == -EAGAIN) > + goto retry; > + } > + > + /* add reset counter so that the following resubmitted job could flush vmid */ > + atomic_inc(&tmp_adev->gpu_reset_counter); > + continue; > + } > + > + /* got the hw fence, signal finished fence */ > + atomic_dec(&ring->sched.num_jobs); > + dma_fence_get(&s_job->s_fence->finished); > + dma_fence_signal(&s_job->s_fence->finished); > + dma_fence_put(&s_job->s_fence->finished); > + > + > + /* remove node from list and free the job */ > + spin_lock(&ring->sched.job_list_lock); > + list_del_init(&s_job->node); > + spin_unlock(&ring->sched.job_list_lock); > + ring->sched.ops->free_job(s_job); > + } > + } > + } > + > for (i = 0; i < AMDGPU_MAX_RINGS; ++i) { > struct amdgpu_ring *ring = tmp_adev->rings[i]; > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > index 865f924772b0..9c3f4edb7532 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > @@ -509,7 +509,7 @@ module_param_named(compute_multipipe, amdgpu_compute_multipipe, int, 0444); > * DOC: gpu_recovery (int) > * Set to enable GPU recovery mechanism (1 = enable, 0 = disable). The default is -1 (auto, disabled except SRIOV). > */ > -MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (1 = enable, 0 = disable, -1 = auto)"); > +MODULE_PARM_DESC(gpu_recovery, "Enable GPU recovery mechanism, (2 = advanced tdr mode, 1 = enable, 0 = disable, -1 = auto)"); > module_param_named(gpu_recovery, amdgpu_gpu_recovery, int, 0444); > > /** > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c > index d82a7ebf6099..99a6a8bddd6f 100644 > --- a/drivers/gpu/drm/scheduler/sched_main.c > +++ b/drivers/gpu/drm/scheduler/sched_main.c > @@ -395,6 +395,81 @@ void drm_sched_increase_karma(struct drm_sched_job *bad) > } > EXPORT_SYMBOL(drm_sched_increase_karma); > > + > +void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int max) > +{ > + struct drm_sched_job *s_job, *tmp; > + uint64_t guilty_context; > + bool found_guilty = false; > + struct dma_fence *fence; > + int i = 0; > + > + list_for_each_entry_safe(s_job, tmp, &sched->pending_list, list) { > + struct drm_sched_fence *s_fence = s_job->s_fence; > + > + if (i>=max) > + break; > + > + if (!found_guilty && atomic_read(&s_job->karma) > sched->hang_limit) { > + found_guilty = true; > + guilty_context = s_job->s_fence->scheduled.context; > + } > + > + if (found_guilty && s_job->s_fence->scheduled.context == guilty_context) > + dma_fence_set_error(&s_fence->finished, -ECANCELED); > + > + dma_fence_put(s_job->s_fence->parent); > + fence = sched->ops->run_job(s_job); > + i++; > + > + if (IS_ERR_OR_NULL(fence)) { > + if (IS_ERR(fence)) > + dma_fence_set_error(&s_fence->finished, PTR_ERR(fence)); > + > + s_job->s_fence->parent = NULL; > + } else { > + s_job->s_fence->parent = fence; > + } > + } > +} > +EXPORT_SYMBOL(drm_sched_resubmit_jobs2); > + > + > + > +void drm_sched_reset_karma(struct drm_sched_job *bad) > +{ > + int i; > + struct drm_sched_entity *tmp; > + struct drm_sched_entity *entity; > + struct drm_gpu_scheduler *sched = bad->sched; > + > + /* don't reset @bad's karma if it's from KERNEL RQ, > + * because sometimes GPU hang would cause kernel jobs (like VM updating jobs) > + * corrupt but keep in mind that kernel jobs always considered good. > + */ > + if (bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) { > + atomic_set(&bad->karma, 0); > + for (i = DRM_SCHED_PRIORITY_MIN; i < DRM_SCHED_PRIORITY_KERNEL; > + i++) { > + struct drm_sched_rq *rq = &sched->sched_rq[i]; > + > + spin_lock(&rq->lock); > + list_for_each_entry_safe(entity, tmp, &rq->entities, list) { > + if (bad->s_fence->scheduled.context == > + entity->fence_context) { > + if (entity->guilty) > + atomic_set(entity->guilty, 0); > + break; > + } > + } > + spin_unlock(&rq->lock); > + if (&entity->list != &rq->entities) > + break; > + } > + } > +} > +EXPORT_SYMBOL(drm_sched_reset_karma); > + > /** > * drm_sched_stop - stop the scheduler > * > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h > index 1c815e0a14ed..01c609149731 100644 > --- a/include/drm/gpu_scheduler.h > +++ b/include/drm/gpu_scheduler.h > @@ -321,7 +321,9 @@ void drm_sched_wakeup(struct drm_gpu_scheduler *sched); > void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad); > void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery); > void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched); > +void drm_sched_resubmit_jobs2(struct drm_gpu_scheduler *sched, int max); > void drm_sched_increase_karma(struct drm_sched_job *bad); > +void drm_sched_reset_karma(struct drm_sched_job *bad); > bool drm_sched_dependency_optimized(struct dma_fence* fence, > struct drm_sched_entity *entity); > void drm_sched_fault(struct drm_gpu_scheduler *sched); </div> </div> </div> </blockquote> </div> </div> </blockquote> </body> </html>