[AMD Official Use Only] Hi Christian
How about this way: #define AMDGPU_DMA_FENCE_FLAG_INSIDE_JOB DMA_FENCE_FLAG_USER_BITS And we can use AMDGPU_DMA_FENCE_FLAG_INSIDE_JOB instead of DMA_FENCE_FLAG_USER_BITS everywhere Thanks ------------------------------------------ Monk Liu | Cloud-GPU Core team ------------------------------------------ -----Original Message----- From: Jingwen Chen <jingwen.ch...@amd.com> Sent: Friday, July 23, 2021 3:07 PM To: Christian König <ckoenig.leichtzumer...@gmail.com>; Grodzovsky, Andrey <andrey.grodzov...@amd.com>; amd-gfx@lists.freedesktop.org Cc: Chen, Horace <horace.c...@amd.com>; Liu, Monk <monk....@amd.com>; Zhang, Jack (Jian) <jack.zha...@amd.com> Subject: Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence On Fri Jul 23, 2021 at 08:33:02AM +0200, Christian König wrote: > Am 22.07.21 um 18:47 schrieb Jingwen Chen: > > On Thu Jul 22, 2021 at 06:24:28PM +0200, Christian König wrote: > > > Am 22.07.21 um 16:45 schrieb Andrey Grodzovsky: > > > > On 2021-07-22 6:45 a.m., Jingwen Chen wrote: > > > > > On Wed Jul 21, 2021 at 12:53:51PM -0400, Andrey Grodzovsky wrote: > > > > > > On 2021-07-20 11:13 p.m., Jingwen Chen wrote: > > > > > > > [Why] > > > > > > > After embeded hw_fence to amdgpu_job, we need to add tdr > > > > > > > support for this feature. > > > > > > > > > > > > > > [How] > > > > > > > 1. Add a resubmit_flag for resubmit jobs. > > > > > > > 2. Clear job fence from RCU and force complete vm flush > > > > > > > fences in > > > > > > > pre_asic_reset > > > > > > > 3. skip dma_fence_get for resubmit jobs and add a > > > > > > > dma_fence_put > > > > > > > for guilty jobs. > > > > > > > > > > > > > > Signed-off-by: Jack Zhang <jack.zha...@amd.com> > > > > > > > Signed-off-by: Jingwen Chen <jingwen.ch...@amd.com> > > > > > > > --- > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 > > > > > > > +++++++++++- > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 16 > > > > > > > +++++++++++----- > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 4 +++- > > > > > > > drivers/gpu/drm/scheduler/sched_main.c | 1 + > > > > > > > include/drm/gpu_scheduler.h | 1 + > > > > > > > 5 files changed, 27 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > > > > > index 40461547701a..fe0237f72a09 100644 > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > > > > > @@ -4382,7 +4382,7 @@ int amdgpu_device_mode1_reset(struct > > > > > > > amdgpu_device *adev) > > > > > > > int amdgpu_device_pre_asic_reset(struct amdgpu_device > > > > > > > *adev, > > > > > > > struct amdgpu_reset_context > > > > > > > *reset_context) > > > > > > > { > > > > > > > - int i, r = 0; > > > > > > > + int i, j, r = 0; > > > > > > > struct amdgpu_job *job = NULL; > > > > > > > bool need_full_reset = > > > > > > > test_bit(AMDGPU_NEED_FULL_RESET, > > > > > > > &reset_context->flags); @@ -4406,6 +4406,16 @@ int > > > > > > > amdgpu_device_pre_asic_reset(struct amdgpu_device *adev, > > > > > > > if (!ring || !ring->sched.thread) > > > > > > > continue; > > > > > > > + /*clear job fence from fence drv to avoid > > > > > > > +force_completion > > > > > > > + *leave NULL and vm flush fence in fence drv */ > > > > > > > + for (j = 0; j <= ring->fence_drv.num_fences_mask; > > > > > > > +j ++) { > > > > > > > + struct dma_fence *old,**ptr; > > > > > > > + ptr = &ring->fence_drv.fences[j]; > > > > > > > + old = rcu_dereference_protected(*ptr, 1); > > > > > > > + if (old && test_bit(DMA_FENCE_FLAG_USER_BITS, > > > > > > > &old->flags))) { > > > > > > > + RCU_INIT_POINTER(*ptr, NULL); > > > > > > > + } > > > > > > Is this to avoid premature job free because of dma_fence_put > > > > > > inside amdgpu_fence_process ? > > > > > > I can't currently remember why but we probably want all the > > > > > > HW fences currently in the ring to be forced signaled - > > > > > > maybe better to test for DMA_FENCE_FLAG_USER_BITS inside > > > > > > amdgpu_fence_process and still do the signaling but not the > > > > > > dma_fence_put part > > > > > > > > > > > > Andrey > > > > > Hi Andrey, > > > > > > > > > > This is to avoid signaling the same fence twice. If we still > > > > > do the signaling, then the job in the pending list will be > > > > > signaled first in force_completion, and later be signaled in > > > > > resubmit. This will go to > > > > > BUG() in amdgpu_fence_process. > > > > > > > > Oh, i see, how about just adding 'skip' flag to amdgpu_ring and > > > > setting it before calling amdgpu_fence_driver_force_completion > > > > and resetting it after, then inside > > > > amdgpu_fence_driver_force_completion > > > > you can just skip the signaling part with this flag for fences > > > > with DMA_FENCE_FLAG_USER_BITS set Less lines of code at least. > > > Still sounds quite a bit hacky. > > > > > > I would rather suggest to completely drop the approach with > > > amdgpu_fence_driver_force_completion(). I could never see why we > > > would want that in the first place. > > > > > > Regards, > > > Christian. > > > > > Hi Christian, > > > > I keep the amdgpu_fence_driver_force_completion here to make sure > > the vm flush fence is signaled and put. > > So the key question is whether the fence of ib test should be the > > first fence to be signaled after reset. > > If it should be, it means not only fences with > > DMA_FENCE_FLAG_USER_BITS but also vm flush fences should be removed > > from RCU fence array before ib_test. Then we must do the > > force_completion here for vm flush fence, otherwise there will be a > > memory leak here as no one will signal and put it after we remove it from > > RCU. > > If not, then the first fence to signal could be vm flush fence. And > > I'm OK with drop amdgpu_fence_driver_force_completion here. > > The key problem is that amdgpu_fence_driver_force_completion() goes > over the RCU array and signals everything there. > > What we should do instead is to go over the jobs and cleanup the ones > we don't want to re-submit and keep the rest. > Hi Christian, The thing is vm flush fence has no job passed to amdgpu_fence_emit, so go through the jobs cannot help find the vm flush fence. And keep the rest fences in the RCU array will lead to signaling them in the ib_test right after ASIC reset. While they will be signaled again during resubmission. What I'm doning here is just trying to cleanup the fences without a parent job and make sure the rest fences won't be signaled twice. > And please drop any usage of DMA_FENCE_FLAG_USER_BITS. That is not > something which should be abused for reset handling. > The DMA_FENCE_FLAG_USER_BITS here is to mark this fence has a parent job. If this is not a proper usage here, do you have any suggestions about how to identify whether the fence has a parent job? Best Regards, JingWen Chen > Regards, > Christian. > > > > > > > Best Regards, > > JingWen Chen > > > > Andrey > > > > > > > > > > > > > > > + } > > > > > > > /* after all hw jobs are reset, hw fence is > > > > > > > meaningless, so force_completion */ > > > > > > > amdgpu_fence_driver_force_completion(ring); > > > > > > > } > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > > > > > > > index eecf21d8ec33..815776c9a013 100644 > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c > > > > > > > @@ -156,11 +156,17 @@ int amdgpu_fence_emit(struct > > > > > > > amdgpu_ring *ring, struct dma_fence **f, struct amd > > > > > > > job->ring = ring; > > > > > > > } > > > > > > > - seq = ++ring->fence_drv.sync_seq; > > > > > > > - dma_fence_init(fence, &amdgpu_fence_ops, > > > > > > > - &ring->fence_drv.lock, > > > > > > > - adev->fence_context + ring->idx, > > > > > > > - seq); > > > > > > > + if (job != NULL && job->base.resubmit_flag == 1) { > > > > > > > + /* reinit seq for resubmitted jobs */ > > > > > > > + seq = ++ring->fence_drv.sync_seq; > > > > > > > + fence->seqno = seq; > > > > > > > + } else { > > > > > > > + seq = ++ring->fence_drv.sync_seq; > > > > > > Seems like you could do the above line only once above > > > > > > if-else as it was before > > > > > Sure, I will modify this. > > > > > > > > > > > > > > > Best Regards, > > > > > JingWen Chen > > > > > > > + dma_fence_init(fence, &amdgpu_fence_ops, > > > > > > > + &ring->fence_drv.lock, > > > > > > > + adev->fence_context + ring->idx, > > > > > > > + seq); > > > > > > > + } > > > > > > > if (job != NULL) { > > > > > > > /* mark this fence has a parent job */ > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c > > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c > > > > > > > index 7c426e225b24..d6f848adc3f4 100644 > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c > > > > > > > @@ -241,6 +241,7 @@ static struct dma_fence > > > > > > > *amdgpu_job_run(struct drm_sched_job *sched_job) > > > > > > > dma_fence_set_error(finished, -ECANCELED);/* skip > > > > > > > IB as well if VRAM lost */ > > > > > > > if (finished->error < 0) { > > > > > > > + dma_fence_put(&job->hw_fence); > > > > > > > DRM_INFO("Skip scheduling IBs!\n"); > > > > > > > } else { > > > > > > > r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, > > > > > > > job, > > > > > > > @@ -249,7 +250,8 @@ static struct dma_fence > > > > > > > *amdgpu_job_run(struct drm_sched_job *sched_job) > > > > > > > DRM_ERROR("Error scheduling IBs (%d)\n", r); > > > > > > > } > > > > > > > - dma_fence_get(fence); > > > > > > > + if (!job->base.resubmit_flag) > > > > > > > + dma_fence_get(fence); > > > > > > > amdgpu_job_free_resources(job); > > > > > > > fence = r ? ERR_PTR(r) : fence; > > > > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > b/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > index f4f474944169..5a36ab5aea2d 100644 > > > > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c > > > > > > > @@ -544,6 +544,7 @@ void drm_sched_resubmit_jobs_ext(struct > > > > > > > drm_gpu_scheduler *sched, int max) > > > > > > > dma_fence_set_error(&s_fence->finished, -ECANCELED); > > > > > > > dma_fence_put(s_job->s_fence->parent); > > > > > > > + s_job->resubmit_flag = 1; > > > > > > > fence = sched->ops->run_job(s_job); > > > > > > > i++; > > > > > > > diff --git a/include/drm/gpu_scheduler.h > > > > > > > b/include/drm/gpu_scheduler.h > > > > > > > index 4ea8606d91fe..06c101af1f71 100644 > > > > > > > --- a/include/drm/gpu_scheduler.h > > > > > > > +++ b/include/drm/gpu_scheduler.h > > > > > > > @@ -198,6 +198,7 @@ struct drm_sched_job { > > > > > > > enum drm_sched_priority s_priority; > > > > > > > struct drm_sched_entity *entity; > > > > > > > struct dma_fence_cb cb; > > > > > > > + int resubmit_flag; > > > > > > > }; > > > > > > > static inline bool drm_sched_invalidate_job(struct > > > > > > > drm_sched_job *s_job, > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx