[PATCH 1/2] drm: add a flag to indicate job is resubmitted

[PATCH 1/2] drm: add a flag to indicate job is resubmitted

[Jingwen Chen]

Add a flag in drm_sched_job to indicate the job resubmit.

Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 2 ++
 include/drm/gpu_scheduler.h            | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index da24c4e8b9fb..d59ac90f5081 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -517,6 +517,7 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
 		if (found_guilty && s_job->s_fence->scheduled.context == guilty_context)
 			dma_fence_set_error(&s_fence->finished, -ECANCELED);
 
+		s_job->flags |= DRM_FLAG_RESUBMIT_JOB;
 		dma_fence_put(s_job->s_fence->parent);
 		fence = sched->ops->run_job(s_job);
 
@@ -565,6 +566,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
 	if (!job->s_fence)
 		return -ENOMEM;
 	job->id = atomic64_inc_return(&sched->job_id_count);
+	job->flags = 0;
 
 	INIT_LIST_HEAD(&job->node);
 
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 92436553fd6a..a22122f98c9c 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -29,6 +29,7 @@
 #include <linux/completion.h>
 
 #define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000)
+#define DRM_FLAG_RESUBMIT_JOB (1 << 0)
 
 struct drm_gpu_scheduler;
 struct drm_sched_rq;
@@ -198,6 +199,7 @@ struct drm_sched_job {
 	enum drm_sched_priority		s_priority;
 	struct drm_sched_entity  *entity;
 	struct dma_fence_cb		cb;
+	uint32_t			flags;
 };
 
 static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
-- 
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amd/amdgpu: force flush resubmit job

[Jingwen Chen]

[Why]
when a job is scheduled during TDR(after device reset count
increase and before drm_sched_stop), this job won't do vm_flush
when resubmit itself after GPU reset done. This can lead to
a page fault.

[How]
Always do vm_flush for resubmit job.

Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index fdbe7d4e8b8b..4af2c5d15950 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1088,7 +1088,8 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
 	if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
 		adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
 
-	if (amdgpu_vmid_had_gpu_reset(adev, id)) {
+	if (amdgpu_vmid_had_gpu_reset(adev, id)	||
+		(job->base.flags & DRM_FLAG_RESUBMIT_JOB)) {
 		gds_switch_needed = true;
 		vm_flush_needed = true;
 		pasid_mapping_needed = true;
-- 
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/2] drm: add a flag to indicate job is resubmitted

[Liu, Monk]

[AMD Official Use Only - Internal Distribution Only]

Patches are fine by me,  reviewed by: Monk Liu <monk.liu@amd.com>

Still need Christian to take a look

Thanks 

------------------------------------------
Monk Liu | Cloud-GPU Core team
------------------------------------------

-----Original Message-----
From: Jingwen Chen <Jingwen.Chen2@amd.com> 
Sent: Thursday, February 25, 2021 1:27 PM
To: amd-gfx@lists.freedesktop.org
Cc: Liu, Monk <Monk.Liu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Chen, JingWen <JingWen.Chen2@amd.com>
Subject: [PATCH 1/2] drm: add a flag to indicate job is resubmitted

Add a flag in drm_sched_job to indicate the job resubmit.

Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 2 ++
 include/drm/gpu_scheduler.h            | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index da24c4e8b9fb..d59ac90f5081 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -517,6 +517,7 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
 		if (found_guilty && s_job->s_fence->scheduled.context == guilty_context)
 			dma_fence_set_error(&s_fence->finished, -ECANCELED);
 
+		s_job->flags |= DRM_FLAG_RESUBMIT_JOB;
 		dma_fence_put(s_job->s_fence->parent);
 		fence = sched->ops->run_job(s_job);
 
@@ -565,6 +566,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
 	if (!job->s_fence)
 		return -ENOMEM;
 	job->id = atomic64_inc_return(&sched->job_id_count);
+	job->flags = 0;
 
 	INIT_LIST_HEAD(&job->node);
 
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index 92436553fd6a..a22122f98c9c 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -29,6 +29,7 @@
 #include <linux/completion.h>
 
 #define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000)
+#define DRM_FLAG_RESUBMIT_JOB (1 << 0)
 
 struct drm_gpu_scheduler;
 struct drm_sched_rq;
@@ -198,6 +199,7 @@ struct drm_sched_job {
 	enum drm_sched_priority		s_priority;
 	struct drm_sched_entity  *entity;
 	struct dma_fence_cb		cb;
+	uint32_t			flags;
 };
 
 static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
--
2.25.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amd/amdgpu: force flush resubmit job

[Christian König]

Am 25.02.21 um 06:27 schrieb Jingwen Chen:
> [Why]
> when a job is scheduled during TDR(after device reset count
> increase and before drm_sched_stop), this job won't do vm_flush
> when resubmit itself after GPU reset done. This can lead to
> a page fault.
>
> [How]
> Always do vm_flush for resubmit job.

NAK, what do you think amdgpu_vmid_had_gpu_reset already does?

Christian.

>
> Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index fdbe7d4e8b8b..4af2c5d15950 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1088,7 +1088,8 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>   	if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
>   		adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
>   
> -	if (amdgpu_vmid_had_gpu_reset(adev, id)) {
> +	if (amdgpu_vmid_had_gpu_reset(adev, id)	||
> +		(job->base.flags & DRM_FLAG_RESUBMIT_JOB)) {
>   		gds_switch_needed = true;
>   		vm_flush_needed = true;
>   		pasid_mapping_needed = true;

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 2/2] drm/amd/amdgpu: force flush resubmit job

[Chen, JingWen]

[AMD Official Use Only - Internal Distribution Only]

Consider this sequence:
1. GPU reset begin
2. device reset count + 1
3. job id 1 scheduled with vm_need_flush=false
4. When handling this job in vm_flush, amdgpu_vmid_had_gpu_reset will return true, thus this job will be flush and the vmid_reset_count will be set equals to device_reset_count
5. stop drm scheduler
6. GPU do real reset
7. resubmit job id 1 with vm_need_flush=false
8. Job id 1 is the first job to resubmit after reset. This time amdgpu_vmid_had_gpu_reset returns false and the vm_need_flush==false

Then no one will flush pd_addr and vmid for jobs after resubmit. In this sequence amdgpu_vmid_had_gpu_reset doesn't work.


Best Regards,
JingWen Chen

-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Thursday, February 25, 2021 3:46 PM
To: Chen, JingWen <JingWen.Chen2@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Liu, Monk <Monk.Liu@amd.com>
Subject: Re: [PATCH 2/2] drm/amd/amdgpu: force flush resubmit job



Am 25.02.21 um 06:27 schrieb Jingwen Chen:
> [Why]
> when a job is scheduled during TDR(after device reset count increase
> and before drm_sched_stop), this job won't do vm_flush when resubmit
> itself after GPU reset done. This can lead to a page fault.
>
> [How]
> Always do vm_flush for resubmit job.

NAK, what do you think amdgpu_vmid_had_gpu_reset already does?

Christian.

>
> Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index fdbe7d4e8b8b..4af2c5d15950 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1088,7 +1088,8 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>   if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
>   adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
>
> -if (amdgpu_vmid_had_gpu_reset(adev, id)) {
> +if (amdgpu_vmid_had_gpu_reset(adev, id)||
> +(job->base.flags & DRM_FLAG_RESUBMIT_JOB)) {
>   gds_switch_needed = true;
>   vm_flush_needed = true;
>   pasid_mapping_needed = true;

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amd/amdgpu: force flush resubmit job

[Christian König]

Good catch, but the approach for the fix is incorrect.

The device reset count can only be incremented after taking the reset 
lock and stopping the scheduler, otherwise a whole bunch of different 
race conditions can happen.

Christian.

Am 25.02.21 um 08:56 schrieb Chen, JingWen:
> [AMD Official Use Only - Internal Distribution Only]
>
> Consider this sequence:
> 1. GPU reset begin
> 2. device reset count + 1
> 3. job id 1 scheduled with vm_need_flush=false
> 4. When handling this job in vm_flush, amdgpu_vmid_had_gpu_reset will return true, thus this job will be flush and the vmid_reset_count will be set equals to device_reset_count
> 5. stop drm scheduler
> 6. GPU do real reset
> 7. resubmit job id 1 with vm_need_flush=false
> 8. Job id 1 is the first job to resubmit after reset. This time amdgpu_vmid_had_gpu_reset returns false and the vm_need_flush==false
>
> Then no one will flush pd_addr and vmid for jobs after resubmit. In this sequence amdgpu_vmid_had_gpu_reset doesn't work.
>
>
> Best Regards,
> JingWen Chen
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Thursday, February 25, 2021 3:46 PM
> To: Chen, JingWen <JingWen.Chen2@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Liu, Monk <Monk.Liu@amd.com>
> Subject: Re: [PATCH 2/2] drm/amd/amdgpu: force flush resubmit job
>
>
>
> Am 25.02.21 um 06:27 schrieb Jingwen Chen:
>> [Why]
>> when a job is scheduled during TDR(after device reset count increase
>> and before drm_sched_stop), this job won't do vm_flush when resubmit
>> itself after GPU reset done. This can lead to a page fault.
>>
>> [How]
>> Always do vm_flush for resubmit job.
> NAK, what do you think amdgpu_vmid_had_gpu_reset already does?
>
> Christian.
>
>> Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++-
>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index fdbe7d4e8b8b..4af2c5d15950 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1088,7 +1088,8 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>>    if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
>>    adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
>>
>> -if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>> +if (amdgpu_vmid_had_gpu_reset(adev, id)||
>> +(job->base.flags & DRM_FLAG_RESUBMIT_JOB)) {
>>    gds_switch_needed = true;
>>    vm_flush_needed = true;
>>    pasid_mapping_needed = true;

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 2/2] drm/amd/amdgpu: force flush resubmit job

[Liu, Monk]

[AMD Official Use Only - Internal Distribution Only]

Yeah, that sounds better than original fix 

Thanks Christian

------------------------------------------
Monk Liu | Cloud-GPU Core team
------------------------------------------

-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com> 
Sent: Thursday, February 25, 2021 4:08 PM
To: Chen, JingWen <JingWen.Chen2@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Liu, Monk <Monk.Liu@amd.com>
Subject: Re: [PATCH 2/2] drm/amd/amdgpu: force flush resubmit job

Good catch, but the approach for the fix is incorrect.

The device reset count can only be incremented after taking the reset lock and stopping the scheduler, otherwise a whole bunch of different race conditions can happen.

Christian.

Am 25.02.21 um 08:56 schrieb Chen, JingWen:
> [AMD Official Use Only - Internal Distribution Only]
>
> Consider this sequence:
> 1. GPU reset begin
> 2. device reset count + 1
> 3. job id 1 scheduled with vm_need_flush=false 4. When handling this 
> job in vm_flush, amdgpu_vmid_had_gpu_reset will return true, thus this 
> job will be flush and the vmid_reset_count will be set equals to 
> device_reset_count 5. stop drm scheduler 6. GPU do real reset 7. 
> resubmit job id 1 with vm_need_flush=false 8. Job id 1 is the first 
> job to resubmit after reset. This time amdgpu_vmid_had_gpu_reset 
> returns false and the vm_need_flush==false
>
> Then no one will flush pd_addr and vmid for jobs after resubmit. In this sequence amdgpu_vmid_had_gpu_reset doesn't work.
>
>
> Best Regards,
> JingWen Chen
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Thursday, February 25, 2021 3:46 PM
> To: Chen, JingWen <JingWen.Chen2@amd.com>; 
> amd-gfx@lists.freedesktop.org
> Cc: Liu, Monk <Monk.Liu@amd.com>
> Subject: Re: [PATCH 2/2] drm/amd/amdgpu: force flush resubmit job
>
>
>
> Am 25.02.21 um 06:27 schrieb Jingwen Chen:
>> [Why]
>> when a job is scheduled during TDR(after device reset count increase 
>> and before drm_sched_stop), this job won't do vm_flush when resubmit 
>> itself after GPU reset done. This can lead to a page fault.
>>
>> [How]
>> Always do vm_flush for resubmit job.
> NAK, what do you think amdgpu_vmid_had_gpu_reset already does?
>
> Christian.
>
>> Signed-off-by: Jingwen Chen <Jingwen.Chen2@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++-
>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index fdbe7d4e8b8b..4af2c5d15950 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1088,7 +1088,8 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>>    if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
>>    adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
>>
>> -if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>> +if (amdgpu_vmid_had_gpu_reset(adev, id)|| (job->base.flags & 
>> +DRM_FLAG_RESUBMIT_JOB)) {
>>    gds_switch_needed = true;
>>    vm_flush_needed = true;
>>    pasid_mapping_needed = true;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx