Re: [PATCH 2/2] drm: add tdr support for embeded hw_fence

[Andrey Grodzovsky <andrey.grodzovsky@amd.com>]

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@amd.com>
>>> Signed-off-by: Jingwen Chen <Jingwen.Chen2@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.

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