[PATCH 2/2] drm: add tdr support for embeded hw_fence
Christian König
ckoenig.leichtzumerken at gmail.com
Thu Jul 22 16:24:28 UTC 2021
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.Zhang1 at amd.com>
>>>> Signed-off-by: Jingwen Chen <Jingwen.Chen2 at 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.
>
> 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,
More information about the amd-gfx
mailing list