Alternatively changing all callers to give MAX_INT as parameter when
they don't care is the preferred variant, but a bit more work.
Christian.
Am 11.03.21 um 04:24 schrieb Grodzovsky, Andrey:
> You can just rename drm_sched_resubmit_jobs to
> drm_sched_resubmit_jobs_imp and create a wrapper
> functiondrm_sched_resubmit_jobs which will call that function with
> MAX_INT and so no need for code change in other driver will be needed.
> For amdgpu you call directly drm_sched_resubmit_jobs_imp with 1 and
> MAX_INT.
>
> Andrey
>
> ------------------------------------------------------------------------
> *From:* Zhang, Jack (Jian) <Jack.Zhang1 at amd.com>
> *Sent:* 10 March 2021 22:05
> *To:* Grodzovsky, Andrey <Andrey.Grodzovsky at amd.com>;
> amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>; Koenig,
> Christian <Christian.Koenig at amd.com>; Liu, Monk <Monk.Liu at amd.com>;
> Deng, Emily <Emily.Deng at amd.com>
> *Subject:* RE: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode
> [AMD Official Use Only - Internal Distribution Only]
>
> Hi, Andrey,
>
> Thank you for your review.
>
> >> This is identical to drm_sched_resubmit_jobs in all but the 'int
> max' handling, can't you reuse drm_sched_resubmit_jobs by passing max
> argument==1 for the find guilty run and then, for the later normal run
> passing max==MAX_INT to avoid break the iteration to early ?
>
> Due to C language doesn't support function overloading, we couldn’t
> define function like the following(compiler error):
> void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched, int max
> = INT_MAX);
>
> We have to rewrite every vender's code where call the
> drm_sched_resubmit_jobs if we defined the function like this.
> void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched, int max);
>
> Meanwhile, I also explored the Variable Arguments in C, But it still
> cannot meet the needs very well.
>
> Do you know some other methods that could help us to make this happen?
> Otherwise, I'm afraid we have to define a separate ways as patch V6 did.
>
> Thanks,
> Jack
>
> -----Original Message-----
> From: Grodzovsky, Andrey <Andrey.Grodzovsky at amd.com>
> Sent: Thursday, March 11, 2021 12:19 AM
> To: Zhang, Jack (Jian) <Jack.Zhang1 at amd.com>;
> amd-gfx at lists.freedesktop.org; Koenig, Christian
> <Christian.Koenig at amd.com>; Liu, Monk <Monk.Liu at amd.com>; Deng, Emily
> <Emily.Deng at amd.com>
> Subject: Re: [PATCH v6] drm/amd/amdgpu implement tdr advanced mode
>
>
>
> On 2021-03-10 8:33 a.m., Jack Zhang wrote:
> > [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 <Jack.Zhang1 at amd.com>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 72 ++++++++++++++++++++-
> > 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(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index e247c3a2ec08..dac0a242e5f5 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4639,6 +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;
> >
> > /*
> > * Special case: RAS triggered and full reset isn't supported @@
> > -4689,9 +4690,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);
> > +}
>
> I think this piece should be in a standalone patch as it comes to fix
> a bug of leaking jobs and not directly related to find actual guilty
> job. Also, this is not the only place where the problem arises - it
> also arises in other drivers where they check that guilty job's fence
> already signaled and bail out before reinsertion of bad job to mirror
> list. Seems to me better fix would be to handle this within scheduler
> code by adding specific return value to
> drm_sched_backend_ops.timedout_job marking that the code terminated
> early - before calling drm_sched_stop and so drm_sched_job_timedout
> would know this and then reinsert the job back to mirror_list itself.
>
> > goto skip_recovery;
> > }
> >
> > @@ -4788,6 +4794,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 +4812,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..340a193b4fb9 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);
>
> This is identical to drm_sched_resubmit_jobs in all but the 'int max'
> handling, can't you reuse drm_sched_resubmit_jobs by passing max
> argument==1 for the find guilty run and then, for the later normal run
> passing max==MAX_INT to avoid break the iteration to early ?
>
> > +
> > +
> > +
> > +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);
>
> Same as above, very similar drm_sched_increase_karma, I would add a
> flag and just reuse code instead of duplucation.
>
> Andrey
>
> > +
> > /**
> > * 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);
> >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20210311/649feb9c/attachment-0001.htm>