Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
I would clean them up further, but that's only moving code around so
feel free to add my rb to those.
Christian.
Am 29.04.19 um 16:14 schrieb Grodzovsky, Andrey:
Thanks David, with that only patches 5 and 6 are left for the series
to be reviewed.
Christian, any more comments on those patches ?
Andrey
On 4/27/19 10:56 PM, Zhou, David(ChunMing) wrote:
Sorry, I only can put my Acked-by: Chunming Zhou
on patch#3.
I cannot fully judge patch #4, #5, #6.
-David
*From:*amd-gfx *On Behalf Of
*Grodzovsky, Andrey
*Sent:* Friday, April 26, 2019 10:09 PM
*To:* Koenig, Christian ; Zhou,
David(ChunMing) ;
dri-de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org;
e...@anholt.net; etna...@lists.freedesktop.org
*Cc:* Kazlauskas, Nicholas ; Liu, Monk
*Subject:* Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty
job already signaled.
Ping (mostly David and Monk).
Andrey
On 4/24/19 3:09 AM, Christian König wrote:
Am 24.04.19 um 05:02 schrieb Zhou, David(ChunMing):
>> - drm_sched_stop(>sched, >base);
>> -
>> /* after all hw jobs are reset, hw fence is
meaningless, so force_completion */
>> amdgpu_fence_driver_force_completion(ring);
>> }
HW fence are already forced completion, then we can just
disable irq fence process and ignore hw fence signal when we
are trying to do GPU reset, I think. Otherwise which will
make the logic much more complex.
If this situation happens because of long time execution, we
can increase timeout of reset detection.
You are not thinking widely enough, forcing the hw fence to
complete can trigger other to start other activity in the system.
We first need to stop everything and make sure that we don't do
any processing any more and then start with our reset procedure
including forcing all hw fences to complete.
Christian.
-David
*From:*amd-gfx
<mailto:amd-gfx-boun...@lists.freedesktop.org> *On Behalf Of
*Grodzovsky, Andrey
*Sent:* Wednesday, April 24, 2019 12:00 AM
*To:* Zhou, David(ChunMing)
<mailto:david1.z...@amd.com>; dri-de...@lists.freedesktop.org
<mailto:dri-de...@lists.freedesktop.org>;
amd-gfx@lists.freedesktop.org
<mailto:amd-gfx@lists.freedesktop.org>; e...@anholt.net
<mailto:e...@anholt.net>; etna...@lists.freedesktop.org
<mailto:etna...@lists.freedesktop.org>;
ckoenig.leichtzumer...@gmail.com
<mailto:ckoenig.leichtzumer...@gmail.com>
*Cc:* Kazlauskas, Nicholas
<mailto:nicholas.kazlaus...@amd.com>; Liu, Monk
<mailto:monk....@amd.com>
*Subject:* Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if
guilty job already signaled.
No, i mean the actual HW fence which signals when the job
finished execution on the HW.
Andrey
On 4/23/19 11:19 AM, Zhou, David(ChunMing) wrote:
do you mean fence timer? why not stop it as well when
stopping sched for the reason of hw reset?
-------- Original Message ----
Subject: Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if
guilty job already signaled.
From: "Grodzovsky, Andrey"
To: "Zhou, David(ChunMing)"
,dri-de...@lists.freedesktop.org,amd-gfx@lists.freedesktop.org,e...@anholt.net,etna...@lists.freedesktop.org,ckoenig.leichtzumer...@gmail.com
<mailto:dri-de...@lists.freedesktop.org,amd-gfx@lists.freedesktop.org,e...@anholt.net,etna...@lists.freedesktop.org,ckoenig.leichtzumer...@gmail.com>
CC: "Kazlauskas, Nicholas" ,"Liu, Monk"
On 4/22/19 9:09 AM, Zhou, David(ChunMing) wrote:
> +Monk.
>
> GPU reset is used widely in SRIOV, so need
virtulizatino guy take a look.
>
> But out of curious, why guilty job can signal more if
the job is already
> set to guilty? set it wrongly?
>
>
> -David
It's possible that the job does completes at a later time
then it's
timeout handler started processing so in this patch we
try to protect
against this by rechecking the HW fence after stopping
all SW
schedulers. We do it BEFORE marking guilty on the job's
sched_entity so
at the point we check the guilty flag is not set yet.
Andrey
>
> 在 2019/4/18 23:00, Andrey Grodzovsky 写道:
>> Also reject TDRs if another one already running.
>>
>> v2:
>> Stop al
Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
Thanks David, with that only patches 5 and 6 are left for the series to be reviewed. Christian, any more comments on those patches ? Andrey On 4/27/19 10:56 PM, Zhou, David(ChunMing) wrote: Sorry, I only can put my Acked-by: Chunming Zhou <mailto:david1.z...@amd.com> on patch#3. I cannot fully judge patch #4, #5, #6. -David From: amd-gfx <mailto:amd-gfx-boun...@lists.freedesktop.org> On Behalf Of Grodzovsky, Andrey Sent: Friday, April 26, 2019 10:09 PM To: Koenig, Christian <mailto:christian.koe...@amd.com>; Zhou, David(ChunMing) <mailto:david1.z...@amd.com>; dri-de...@lists.freedesktop.org<mailto:dri-de...@lists.freedesktop.org>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>; e...@anholt.net<mailto:e...@anholt.net>; etna...@lists.freedesktop.org<mailto:etna...@lists.freedesktop.org> Cc: Kazlauskas, Nicholas <mailto:nicholas.kazlaus...@amd.com>; Liu, Monk <mailto:monk....@amd.com> Subject: Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled. Ping (mostly David and Monk). Andrey On 4/24/19 3:09 AM, Christian König wrote: Am 24.04.19 um 05:02 schrieb Zhou, David(ChunMing): >> -drm_sched_stop(>sched, >base); >> - >> /* after all hw jobs are reset, hw fence is meaningless, so >> force_completion */ >> amdgpu_fence_driver_force_completion(ring); >> } HW fence are already forced completion, then we can just disable irq fence process and ignore hw fence signal when we are trying to do GPU reset, I think. Otherwise which will make the logic much more complex. If this situation happens because of long time execution, we can increase timeout of reset detection. You are not thinking widely enough, forcing the hw fence to complete can trigger other to start other activity in the system. We first need to stop everything and make sure that we don't do any processing any more and then start with our reset procedure including forcing all hw fences to complete. Christian. -David From: amd-gfx <mailto:amd-gfx-boun...@lists.freedesktop.org> On Behalf Of Grodzovsky, Andrey Sent: Wednesday, April 24, 2019 12:00 AM To: Zhou, David(ChunMing) <mailto:david1.z...@amd.com>; dri-de...@lists.freedesktop.org<mailto:dri-de...@lists.freedesktop.org>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>; e...@anholt.net<mailto:e...@anholt.net>; etna...@lists.freedesktop.org<mailto:etna...@lists.freedesktop.org>; ckoenig.leichtzumer...@gmail.com<mailto:ckoenig.leichtzumer...@gmail.com> Cc: Kazlauskas, Nicholas <mailto:nicholas.kazlaus...@amd.com>; Liu, Monk <mailto:monk@amd.com> Subject: Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled. No, i mean the actual HW fence which signals when the job finished execution on the HW. Andrey On 4/23/19 11:19 AM, Zhou, David(ChunMing) wrote: do you mean fence timer? why not stop it as well when stopping sched for the reason of hw reset? Original Message Subject: Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled. From: "Grodzovsky, Andrey" To: "Zhou, David(ChunMing)" ,dri-de...@lists.freedesktop.org,amd-gfx@lists.freedesktop.org,e...@anholt.net,etna...@lists.freedesktop.org,ckoenig.leichtzumer...@gmail.com<mailto:dri-de...@lists.freedesktop.org,amd-gfx@lists.freedesktop.org,e...@anholt.net,etna...@lists.freedesktop.org,ckoenig.leichtzumer...@gmail.com> CC: "Kazlauskas, Nicholas" ,"Liu, Monk" On 4/22/19 9:09 AM, Zhou, David(ChunMing) wrote: > +Monk. > > GPU reset is used widely in SRIOV, so need virtulizatino guy take a look. > > But out of curious, why guilty job can signal more if the job is already > set to guilty? set it wrongly? > > > -David It's possible that the job does completes at a later time then it's timeout handler started processing so in this patch we try to protect against this by rechecking the HW fence after stopping all SW schedulers. We do it BEFORE marking guilty on the job's sched_entity so at the point we check the guilty flag is not set yet. Andrey > > 在 2019/4/18 23:00, Andrey Grodzovsky 写道: >> Also reject TDRs if another one already running. >> >> v2: >> Stop all schedulers across device and entire XGMI hive before >> force signaling HW fences. >> Avoid passing job_signaled to helper fnctions to keep all the decision >> making about skipping HW reset in one place. >> >> v3: >> Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced >> against it's decrement in drm_sched_stop in non HW reset case. >> v4: rebase >> v5: Revert v3 as we do it now in sceduler code. >> >> Signed-off-by: Andrey Grod
RE: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
Sorry, I only can put my Acked-by: Chunming Zhou on patch#3. I cannot fully judge patch #4, #5, #6. -David From: amd-gfx On Behalf Of Grodzovsky, Andrey Sent: Friday, April 26, 2019 10:09 PM To: Koenig, Christian ; Zhou, David(ChunMing) ; dri-de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; e...@anholt.net; etna...@lists.freedesktop.org Cc: Kazlauskas, Nicholas ; Liu, Monk Subject: Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled. Ping (mostly David and Monk). Andrey On 4/24/19 3:09 AM, Christian König wrote: Am 24.04.19 um 05:02 schrieb Zhou, David(ChunMing): >> -drm_sched_stop(>sched, >base); >> - >> /* after all hw jobs are reset, hw fence is meaningless, so >> force_completion */ >> amdgpu_fence_driver_force_completion(ring); >> } HW fence are already forced completion, then we can just disable irq fence process and ignore hw fence signal when we are trying to do GPU reset, I think. Otherwise which will make the logic much more complex. If this situation happens because of long time execution, we can increase timeout of reset detection. You are not thinking widely enough, forcing the hw fence to complete can trigger other to start other activity in the system. We first need to stop everything and make sure that we don't do any processing any more and then start with our reset procedure including forcing all hw fences to complete. Christian. -David From: amd-gfx <mailto:amd-gfx-boun...@lists.freedesktop.org> On Behalf Of Grodzovsky, Andrey Sent: Wednesday, April 24, 2019 12:00 AM To: Zhou, David(ChunMing) <mailto:david1.z...@amd.com>; dri-de...@lists.freedesktop.org<mailto:dri-de...@lists.freedesktop.org>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>; e...@anholt.net<mailto:e...@anholt.net>; etna...@lists.freedesktop.org<mailto:etna...@lists.freedesktop.org>; ckoenig.leichtzumer...@gmail.com<mailto:ckoenig.leichtzumer...@gmail.com> Cc: Kazlauskas, Nicholas <mailto:nicholas.kazlaus...@amd.com>; Liu, Monk <mailto:monk@amd.com> Subject: Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled. No, i mean the actual HW fence which signals when the job finished execution on the HW. Andrey On 4/23/19 11:19 AM, Zhou, David(ChunMing) wrote: do you mean fence timer? why not stop it as well when stopping sched for the reason of hw reset? -------- Original Message Subject: Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled. From: "Grodzovsky, Andrey" To: "Zhou, David(ChunMing)" ,dri-de...@lists.freedesktop.org,amd-gfx@lists.freedesktop.org,e...@anholt.net,etna...@lists.freedesktop.org,ckoenig.leichtzumer...@gmail.com<mailto:dri-de...@lists.freedesktop.org,amd-gfx@lists.freedesktop.org,e...@anholt.net,etna...@lists.freedesktop.org,ckoenig.leichtzumer...@gmail.com> CC: "Kazlauskas, Nicholas" ,"Liu, Monk" On 4/22/19 9:09 AM, Zhou, David(ChunMing) wrote: > +Monk. > > GPU reset is used widely in SRIOV, so need virtulizatino guy take a look. > > But out of curious, why guilty job can signal more if the job is already > set to guilty? set it wrongly? > > > -David It's possible that the job does completes at a later time then it's timeout handler started processing so in this patch we try to protect against this by rechecking the HW fence after stopping all SW schedulers. We do it BEFORE marking guilty on the job's sched_entity so at the point we check the guilty flag is not set yet. Andrey > > 在 2019/4/18 23:00, Andrey Grodzovsky 写道: >> Also reject TDRs if another one already running. >> >> v2: >> Stop all schedulers across device and entire XGMI hive before >> force signaling HW fences. >> Avoid passing job_signaled to helper fnctions to keep all the decision >> making about skipping HW reset in one place. >> >> v3: >> Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced >> against it's decrement in drm_sched_stop in non HW reset case. >> v4: rebase >> v5: Revert v3 as we do it now in sceduler code. >> >> Signed-off-by: Andrey Grodzovsky >> <mailto:andrey.grodzov...@amd.com> >> --- >>drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143 >> +++-- >>1 file changed, 95 insertions(+), 48 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> index a0e165c..85f8792 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> @@ -3334,8 +3334,6 @@ static int amdgpu_device_pre_
Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
Ping (mostly David and Monk). Andrey On 4/24/19 3:09 AM, Christian König wrote: Am 24.04.19 um 05:02 schrieb Zhou, David(ChunMing): >> -drm_sched_stop(>sched, >base); >> - >> /* after all hw jobs are reset, hw fence is meaningless, so >> force_completion */ >> amdgpu_fence_driver_force_completion(ring); >> } HW fence are already forced completion, then we can just disable irq fence process and ignore hw fence signal when we are trying to do GPU reset, I think. Otherwise which will make the logic much more complex. If this situation happens because of long time execution, we can increase timeout of reset detection. You are not thinking widely enough, forcing the hw fence to complete can trigger other to start other activity in the system. We first need to stop everything and make sure that we don't do any processing any more and then start with our reset procedure including forcing all hw fences to complete. Christian. -David From: amd-gfx <mailto:amd-gfx-boun...@lists.freedesktop.org> On Behalf Of Grodzovsky, Andrey Sent: Wednesday, April 24, 2019 12:00 AM To: Zhou, David(ChunMing) <mailto:david1.z...@amd.com>; dri-de...@lists.freedesktop.org<mailto:dri-de...@lists.freedesktop.org>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>; e...@anholt.net<mailto:e...@anholt.net>; etna...@lists.freedesktop.org<mailto:etna...@lists.freedesktop.org>; ckoenig.leichtzumer...@gmail.com<mailto:ckoenig.leichtzumer...@gmail.com> Cc: Kazlauskas, Nicholas <mailto:nicholas.kazlaus...@amd.com>; Liu, Monk <mailto:monk@amd.com> Subject: Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled. No, i mean the actual HW fence which signals when the job finished execution on the HW. Andrey On 4/23/19 11:19 AM, Zhou, David(ChunMing) wrote: do you mean fence timer? why not stop it as well when stopping sched for the reason of hw reset? -------- Original Message Subject: Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled. From: "Grodzovsky, Andrey" To: "Zhou, David(ChunMing)" ,dri-de...@lists.freedesktop.org,amd-gfx@lists.freedesktop.org,e...@anholt.net,etna...@lists.freedesktop.org,ckoenig.leichtzumer...@gmail.com<mailto:dri-de...@lists.freedesktop.org,amd-gfx@lists.freedesktop.org,e...@anholt.net,etna...@lists.freedesktop.org,ckoenig.leichtzumer...@gmail.com> CC: "Kazlauskas, Nicholas" ,"Liu, Monk" On 4/22/19 9:09 AM, Zhou, David(ChunMing) wrote: > +Monk. > > GPU reset is used widely in SRIOV, so need virtulizatino guy take a look. > > But out of curious, why guilty job can signal more if the job is already > set to guilty? set it wrongly? > > > -David It's possible that the job does completes at a later time then it's timeout handler started processing so in this patch we try to protect against this by rechecking the HW fence after stopping all SW schedulers. We do it BEFORE marking guilty on the job's sched_entity so at the point we check the guilty flag is not set yet. Andrey > > 在 2019/4/18 23:00, Andrey Grodzovsky 写道: >> Also reject TDRs if another one already running. >> >> v2: >> Stop all schedulers across device and entire XGMI hive before >> force signaling HW fences. >> Avoid passing job_signaled to helper fnctions to keep all the decision >> making about skipping HW reset in one place. >> >> v3: >> Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced >> against it's decrement in drm_sched_stop in non HW reset case. >> v4: rebase >> v5: Revert v3 as we do it now in sceduler code. >> >> Signed-off-by: Andrey Grodzovsky >> <mailto:andrey.grodzov...@amd.com> >> --- >>drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143 >> +++-- >>1 file changed, 95 insertions(+), 48 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> index a0e165c..85f8792 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> @@ -3334,8 +3334,6 @@ static int amdgpu_device_pre_asic_reset(struct >> amdgpu_device *adev, >> if (!ring || !ring->sched.thread) >> continue; >> >> -drm_sched_stop(>sched, >base); >> - >> /* after all hw jobs are reset, hw fence is meaningless, so >> force_completion */ >> amdgpu_fence_driver_force_completion(ring); >> } >> @@ -3343,6 +3341,7 @@ static int amdgpu_device_pre_as
Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
Am 24.04.19 um 05:02 schrieb Zhou, David(ChunMing):
>> - drm_sched_stop(>sched, >base);
>> -
>> /* after all hw jobs are reset, hw fence is
meaningless, so force_completion */
>> amdgpu_fence_driver_force_completion(ring);
>> }
HW fence are already forced completion, then we can just disable irq
fence process and ignore hw fence signal when we are trying to do GPU
reset, I think. Otherwise which will make the logic much more complex.
If this situation happens because of long time execution, we can
increase timeout of reset detection.
You are not thinking widely enough, forcing the hw fence to complete can
trigger other to start other activity in the system.
We first need to stop everything and make sure that we don't do any
processing any more and then start with our reset procedure including
forcing all hw fences to complete.
Christian.
-David
*From:*amd-gfx *On Behalf Of
*Grodzovsky, Andrey
*Sent:* Wednesday, April 24, 2019 12:00 AM
*To:* Zhou, David(ChunMing) ;
dri-de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org;
e...@anholt.net; etna...@lists.freedesktop.org;
ckoenig.leichtzumer...@gmail.com
*Cc:* Kazlauskas, Nicholas ; Liu, Monk
*Subject:* Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job
already signaled.
No, i mean the actual HW fence which signals when the job finished
execution on the HW.
Andrey
On 4/23/19 11:19 AM, Zhou, David(ChunMing) wrote:
do you mean fence timer? why not stop it as well when stopping
sched for the reason of hw reset?
Original Message --------
Subject: Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty
job already signaled.
From: "Grodzovsky, Andrey"
To: "Zhou, David(ChunMing)"
,dri-de...@lists.freedesktop.org,amd-gfx@lists.freedesktop.org,e...@anholt.net,etna...@lists.freedesktop.org,ckoenig.leichtzumer...@gmail.com
<mailto:dri-de...@lists.freedesktop.org,amd-gfx@lists.freedesktop.org,e...@anholt.net,etna...@lists.freedesktop.org,ckoenig.leichtzumer...@gmail.com>
CC: "Kazlauskas, Nicholas" ,"Liu, Monk"
On 4/22/19 9:09 AM, Zhou, David(ChunMing) wrote:
> +Monk.
>
> GPU reset is used widely in SRIOV, so need virtulizatino guy
take a look.
>
> But out of curious, why guilty job can signal more if the job is
already
> set to guilty? set it wrongly?
>
>
> -David
It's possible that the job does completes at a later time then it's
timeout handler started processing so in this patch we try to protect
against this by rechecking the HW fence after stopping all SW
schedulers. We do it BEFORE marking guilty on the job's
sched_entity so
at the point we check the guilty flag is not set yet.
Andrey
>
> 在 2019/4/18 23:00, Andrey Grodzovsky 写道:
>> Also reject TDRs if another one already running.
>>
>> v2:
>> Stop all schedulers across device and entire XGMI hive before
>> force signaling HW fences.
>> Avoid passing job_signaled to helper fnctions to keep all the
decision
>> making about skipping HW reset in one place.
>>
>> v3:
>> Fix SW sched. hang after non HW reset. sched.hw_rq_count has to
be balanced
>> against it's decrement in drm_sched_stop in non HW reset case.
>> v4: rebase
>> v5: Revert v3 as we do it now in sceduler code.
>>
>> Signed-off-by: Andrey Grodzovsky
<mailto:andrey.grodzov...@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143
+++--
>> 1 file changed, 95 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index a0e165c..85f8792 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3334,8 +3334,6 @@ static int
amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>> if (!ring || !ring->sched.thread)
>> continue;
>>
>> - drm_sched_stop(>sched, >base);
>> -
>> /* after all hw jobs are reset, hw fence is
meaningless, so force_completion */
>> amdgpu_fence_driver_force_completion(ring);
>> }
>> @@ -3343,6 +3341,7 @@ static int
amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>> if(job)
>> drm_sched_increase_karma(>base);
>>
>> + /* Don't suspend on bare metal if we are not going to HW
reset the ASIC */
>> if (!amdgpu_sriov_vf(adev)) {
RE: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
>> -drm_sched_stop(>sched, >base);
>> -
>> /* after all hw jobs are reset, hw fence is meaningless, so
>> force_completion */
>> amdgpu_fence_driver_force_completion(ring);
>> }
HW fence are already forced completion, then we can just disable irq fence
process and ignore hw fence signal when we are trying to do GPU reset, I think.
Otherwise which will make the logic much more complex.
If this situation happens because of long time execution, we can increase
timeout of reset detection.
-David
From: amd-gfx On Behalf Of Grodzovsky,
Andrey
Sent: Wednesday, April 24, 2019 12:00 AM
To: Zhou, David(ChunMing) ;
dri-de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org;
e...@anholt.net; etna...@lists.freedesktop.org; ckoenig.leichtzumer...@gmail.com
Cc: Kazlauskas, Nicholas ; Liu, Monk
Subject: Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already
signaled.
No, i mean the actual HW fence which signals when the job finished execution on
the HW.
Andrey
On 4/23/19 11:19 AM, Zhou, David(ChunMing) wrote:
do you mean fence timer? why not stop it as well when stopping sched for the
reason of hw reset?
---- Original Message ----
Subject: Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already
signaled.
From: "Grodzovsky, Andrey"
To: "Zhou, David(ChunMing)"
,dri-de...@lists.freedesktop.org,amd-gfx@lists.freedesktop.org,e...@anholt.net,etna...@lists.freedesktop.org,ckoenig.leichtzumer...@gmail.com<mailto:dri-de...@lists.freedesktop.org,amd-gfx@lists.freedesktop.org,e...@anholt.net,etna...@lists.freedesktop.org,ckoenig.leichtzumer...@gmail.com>
CC: "Kazlauskas, Nicholas" ,"Liu, Monk"
On 4/22/19 9:09 AM, Zhou, David(ChunMing) wrote:
> +Monk.
>
> GPU reset is used widely in SRIOV, so need virtulizatino guy take a look.
>
> But out of curious, why guilty job can signal more if the job is already
> set to guilty? set it wrongly?
>
>
> -David
It's possible that the job does completes at a later time then it's
timeout handler started processing so in this patch we try to protect
against this by rechecking the HW fence after stopping all SW
schedulers. We do it BEFORE marking guilty on the job's sched_entity so
at the point we check the guilty flag is not set yet.
Andrey
>
> 在 2019/4/18 23:00, Andrey Grodzovsky 写道:
>> Also reject TDRs if another one already running.
>>
>> v2:
>> Stop all schedulers across device and entire XGMI hive before
>> force signaling HW fences.
>> Avoid passing job_signaled to helper fnctions to keep all the decision
>> making about skipping HW reset in one place.
>>
>> v3:
>> Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced
>> against it's decrement in drm_sched_stop in non HW reset case.
>> v4: rebase
>> v5: Revert v3 as we do it now in sceduler code.
>>
>> Signed-off-by: Andrey Grodzovsky
>> <mailto:andrey.grodzov...@amd.com>
>> ---
>>drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143
>> +++--
>>1 file changed, 95 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index a0e165c..85f8792 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3334,8 +3334,6 @@ static int amdgpu_device_pre_asic_reset(struct
>> amdgpu_device *adev,
>> if (!ring || !ring->sched.thread)
>> continue;
>>
>> -drm_sched_stop(>sched, >base);
>> -
>> /* after all hw jobs are reset, hw fence is meaningless, so
>> force_completion */
>> amdgpu_fence_driver_force_completion(ring);
>> }
>> @@ -3343,6 +3341,7 @@ static int amdgpu_device_pre_asic_reset(struct
>> amdgpu_device *adev,
>> if(job)
>> drm_sched_increase_karma(>base);
>>
>> +/* Don't suspend on bare metal if we are not going to HW reset the ASIC
>> */
>> if (!amdgpu_sriov_vf(adev)) {
>>
>> if (!need_full_reset)
>> @@ -3480,37 +3479,21 @@ static int amdgpu_do_asic_reset(struct
>> amdgpu_hive_info *hive,
>> return r;
>>}
>>
>> -static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev)
>> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, bool
>> trylock)
>>{
>> -int i;
>> -
>> -for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>> -struct amdgpu_ring *ring = ad
Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
No, i mean the actual HW fence which signals when the job finished execution on
the HW.
Andrey
On 4/23/19 11:19 AM, Zhou, David(ChunMing) wrote:
do you mean fence timer? why not stop it as well when stopping sched for the
reason of hw reset?
Original Message
Subject: Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already
signaled.
From: "Grodzovsky, Andrey"
To: "Zhou, David(ChunMing)"
,dri-de...@lists.freedesktop.org,amd-gfx@lists.freedesktop.org,e...@anholt.net,etna...@lists.freedesktop.org,ckoenig.leichtzumer...@gmail.com<mailto:dri-de...@lists.freedesktop.org,amd-gfx@lists.freedesktop.org,e...@anholt.net,etna...@lists.freedesktop.org,ckoenig.leichtzumer...@gmail.com>
CC: "Kazlauskas, Nicholas" ,"Liu, Monk"
On 4/22/19 9:09 AM, Zhou, David(ChunMing) wrote:
> +Monk.
>
> GPU reset is used widely in SRIOV, so need virtulizatino guy take a look.
>
> But out of curious, why guilty job can signal more if the job is already
> set to guilty? set it wrongly?
>
>
> -David
It's possible that the job does completes at a later time then it's
timeout handler started processing so in this patch we try to protect
against this by rechecking the HW fence after stopping all SW
schedulers. We do it BEFORE marking guilty on the job's sched_entity so
at the point we check the guilty flag is not set yet.
Andrey
>
> 在 2019/4/18 23:00, Andrey Grodzovsky 写道:
>> Also reject TDRs if another one already running.
>>
>> v2:
>> Stop all schedulers across device and entire XGMI hive before
>> force signaling HW fences.
>> Avoid passing job_signaled to helper fnctions to keep all the decision
>> making about skipping HW reset in one place.
>>
>> v3:
>> Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced
>> against it's decrement in drm_sched_stop in non HW reset case.
>> v4: rebase
>> v5: Revert v3 as we do it now in sceduler code.
>>
>> Signed-off-by: Andrey Grodzovsky
>> <mailto:andrey.grodzov...@amd.com>
>> ---
>>drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143
>> +++--
>>1 file changed, 95 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index a0e165c..85f8792 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3334,8 +3334,6 @@ static int amdgpu_device_pre_asic_reset(struct
>> amdgpu_device *adev,
>> if (!ring || !ring->sched.thread)
>> continue;
>>
>> -drm_sched_stop(>sched, >base);
>> -
>> /* after all hw jobs are reset, hw fence is meaningless, so
>> force_completion */
>> amdgpu_fence_driver_force_completion(ring);
>> }
>> @@ -3343,6 +3341,7 @@ static int amdgpu_device_pre_asic_reset(struct
>> amdgpu_device *adev,
>> if(job)
>> drm_sched_increase_karma(>base);
>>
>> +/* Don't suspend on bare metal if we are not going to HW reset the ASIC
>> */
>> if (!amdgpu_sriov_vf(adev)) {
>>
>> if (!need_full_reset)
>> @@ -3480,37 +3479,21 @@ static int amdgpu_do_asic_reset(struct
>> amdgpu_hive_info *hive,
>> return r;
>>}
>>
>> -static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev)
>> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, bool
>> trylock)
>>{
>> -int i;
>> -
>> -for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>> -struct amdgpu_ring *ring = adev->rings[i];
>> -
>> -if (!ring || !ring->sched.thread)
>> -continue;
>> -
>> -if (!adev->asic_reset_res)
>> -drm_sched_resubmit_jobs(>sched);
>> +if (trylock) {
>> +if (!mutex_trylock(>lock_reset))
>> +return false;
>> +} else
>> +mutex_lock(>lock_reset);
>>
>> -drm_sched_start(>sched, !adev->asic_reset_res);
>> -}
>> -
>> -if (!amdgpu_device_has_dc_support(adev)) {
>> -drm_helper_resume_force_mode(adev->ddev);
>> -}
>> -
>> -adev->asic_reset_res = 0;
>> -}
>> -
>> -static void amdgpu_device_lock_adev(struct amdgpu_device *adev)
>> -{
>> -mutex_lock(>lock_reset);
>> atomic_inc(>gpu_reset_counter);
>> adev->in_gpu
Re:[PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
do you mean fence timer? why not stop it as well when stopping sched for the
reason of hw reset?
Original Message
Subject: Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already
signaled.
From: "Grodzovsky, Andrey"
To: "Zhou, David(ChunMing)"
,dri-de...@lists.freedesktop.org,amd-gfx@lists.freedesktop.org,e...@anholt.net,etna...@lists.freedesktop.org,ckoenig.leichtzumer...@gmail.com
CC: "Kazlauskas, Nicholas" ,"Liu, Monk"
On 4/22/19 9:09 AM, Zhou, David(ChunMing) wrote:
> +Monk.
>
> GPU reset is used widely in SRIOV, so need virtulizatino guy take a look.
>
> But out of curious, why guilty job can signal more if the job is already
> set to guilty? set it wrongly?
>
>
> -David
It's possible that the job does completes at a later time then it's
timeout handler started processing so in this patch we try to protect
against this by rechecking the HW fence after stopping all SW
schedulers. We do it BEFORE marking guilty on the job's sched_entity so
at the point we check the guilty flag is not set yet.
Andrey
>
> 在 2019/4/18 23:00, Andrey Grodzovsky 写道:
>> Also reject TDRs if another one already running.
>>
>> v2:
>> Stop all schedulers across device and entire XGMI hive before
>> force signaling HW fences.
>> Avoid passing job_signaled to helper fnctions to keep all the decision
>> making about skipping HW reset in one place.
>>
>> v3:
>> Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced
>> against it's decrement in drm_sched_stop in non HW reset case.
>> v4: rebase
>> v5: Revert v3 as we do it now in sceduler code.
>>
>> Signed-off-by: Andrey Grodzovsky
>> ---
>>drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143
>> +++--
>>1 file changed, 95 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index a0e165c..85f8792 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3334,8 +3334,6 @@ static int amdgpu_device_pre_asic_reset(struct
>> amdgpu_device *adev,
>> if (!ring || !ring->sched.thread)
>> continue;
>>
>> -drm_sched_stop(>sched, >base);
>> -
>> /* after all hw jobs are reset, hw fence is meaningless, so
>> force_completion */
>> amdgpu_fence_driver_force_completion(ring);
>> }
>> @@ -3343,6 +3341,7 @@ static int amdgpu_device_pre_asic_reset(struct
>> amdgpu_device *adev,
>> if(job)
>> drm_sched_increase_karma(>base);
>>
>> +/* Don't suspend on bare metal if we are not going to HW reset the ASIC
>> */
>> if (!amdgpu_sriov_vf(adev)) {
>>
>> if (!need_full_reset)
>> @@ -3480,37 +3479,21 @@ static int amdgpu_do_asic_reset(struct
>> amdgpu_hive_info *hive,
>> return r;
>>}
>>
>> -static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev)
>> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, bool
>> trylock)
>>{
>> -int i;
>> -
>> -for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>> -struct amdgpu_ring *ring = adev->rings[i];
>> -
>> -if (!ring || !ring->sched.thread)
>> -continue;
>> -
>> -if (!adev->asic_reset_res)
>> -drm_sched_resubmit_jobs(>sched);
>> +if (trylock) {
>> +if (!mutex_trylock(>lock_reset))
>> +return false;
>> +} else
>> +mutex_lock(>lock_reset);
>>
>> -drm_sched_start(>sched, !adev->asic_reset_res);
>> -}
>> -
>> -if (!amdgpu_device_has_dc_support(adev)) {
>> -drm_helper_resume_force_mode(adev->ddev);
>> -}
>> -
>> -adev->asic_reset_res = 0;
>> -}
>> -
>> -static void amdgpu_device_lock_adev(struct amdgpu_device *adev)
>> -{
>> -mutex_lock(>lock_reset);
>> atomic_inc(>gpu_reset_counter);
>> adev->in_gpu_reset = 1;
>> /* Block kfd: SRIOV would do it separately */
>> if (!amdgpu_sriov_vf(adev))
>>amdgpu_amdkfd_pre_reset(adev);
>> +
>> +return true;
>>}
>>
>>static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
>> @@ -3538,40 +3521,42 @@ s
Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
On 4/22/19 9:09 AM, Zhou, David(ChunMing) wrote:
> +Monk.
>
> GPU reset is used widely in SRIOV, so need virtulizatino guy take a look.
>
> But out of curious, why guilty job can signal more if the job is already
> set to guilty? set it wrongly?
>
>
> -David
It's possible that the job does completes at a later time then it's
timeout handler started processing so in this patch we try to protect
against this by rechecking the HW fence after stopping all SW
schedulers. We do it BEFORE marking guilty on the job's sched_entity so
at the point we check the guilty flag is not set yet.
Andrey
>
> 在 2019/4/18 23:00, Andrey Grodzovsky 写道:
>> Also reject TDRs if another one already running.
>>
>> v2:
>> Stop all schedulers across device and entire XGMI hive before
>> force signaling HW fences.
>> Avoid passing job_signaled to helper fnctions to keep all the decision
>> making about skipping HW reset in one place.
>>
>> v3:
>> Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced
>> against it's decrement in drm_sched_stop in non HW reset case.
>> v4: rebase
>> v5: Revert v3 as we do it now in sceduler code.
>>
>> Signed-off-by: Andrey Grodzovsky
>> ---
>>drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143
>> +++--
>>1 file changed, 95 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index a0e165c..85f8792 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3334,8 +3334,6 @@ static int amdgpu_device_pre_asic_reset(struct
>> amdgpu_device *adev,
>> if (!ring || !ring->sched.thread)
>> continue;
>>
>> -drm_sched_stop(>sched, >base);
>> -
>> /* after all hw jobs are reset, hw fence is meaningless, so
>> force_completion */
>> amdgpu_fence_driver_force_completion(ring);
>> }
>> @@ -3343,6 +3341,7 @@ static int amdgpu_device_pre_asic_reset(struct
>> amdgpu_device *adev,
>> if(job)
>> drm_sched_increase_karma(>base);
>>
>> +/* Don't suspend on bare metal if we are not going to HW reset the ASIC
>> */
>> if (!amdgpu_sriov_vf(adev)) {
>>
>> if (!need_full_reset)
>> @@ -3480,37 +3479,21 @@ static int amdgpu_do_asic_reset(struct
>> amdgpu_hive_info *hive,
>> return r;
>>}
>>
>> -static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev)
>> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, bool
>> trylock)
>>{
>> -int i;
>> -
>> -for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>> -struct amdgpu_ring *ring = adev->rings[i];
>> -
>> -if (!ring || !ring->sched.thread)
>> -continue;
>> -
>> -if (!adev->asic_reset_res)
>> -drm_sched_resubmit_jobs(>sched);
>> +if (trylock) {
>> +if (!mutex_trylock(>lock_reset))
>> +return false;
>> +} else
>> +mutex_lock(>lock_reset);
>>
>> -drm_sched_start(>sched, !adev->asic_reset_res);
>> -}
>> -
>> -if (!amdgpu_device_has_dc_support(adev)) {
>> -drm_helper_resume_force_mode(adev->ddev);
>> -}
>> -
>> -adev->asic_reset_res = 0;
>> -}
>> -
>> -static void amdgpu_device_lock_adev(struct amdgpu_device *adev)
>> -{
>> -mutex_lock(>lock_reset);
>> atomic_inc(>gpu_reset_counter);
>> adev->in_gpu_reset = 1;
>> /* Block kfd: SRIOV would do it separately */
>> if (!amdgpu_sriov_vf(adev))
>>amdgpu_amdkfd_pre_reset(adev);
>> +
>> +return true;
>>}
>>
>>static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
>> @@ -3538,40 +3521,42 @@ static void amdgpu_device_unlock_adev(struct
>> amdgpu_device *adev)
>>int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>>struct amdgpu_job *job)
>>{
>> -int r;
>> +struct list_head device_list, *device_list_handle = NULL;
>> +bool need_full_reset, job_signaled;
>> struct amdgpu_hive_info *hive = NULL;
>> -bool need_full_reset = false;
>> struct amdgpu_device *tmp_adev = NULL;
>> -struct list_head device_list, *device_list_handle = NULL;
>> +int i, r = 0;
>>
>> +need_full_reset = job_signaled = false;
>> INIT_LIST_HEAD(_list);
>>
>> dev_info(adev->dev, "GPU reset begin!\n");
>>
>> +hive = amdgpu_get_xgmi_hive(adev, false);
>> +
>> /*
>> - * In case of XGMI hive disallow concurrent resets to be triggered
>> - * by different nodes. No point also since the one node already
>> executing
>> - * reset will also reset all the other nodes in the hive.
>> + * Here we trylock to avoid chain of resets executing from
>> + * either trigger by jobs on different adevs in XGMI hive or jobs on
>> + * different schedulers
Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
Am 23.04.19 um 16:12 schrieb Grodzovsky, Andrey:
On 4/23/19 8:32 AM, Koenig, Christian wrote:
Well you at least have to give me time till after the holidays to get
going again :)
Not sure exactly jet why we need patch number 5.
Probably you missed the mail where I pointed out a bug I found during
testing - I am reattaching the mail and the KASAN dump.
Ah, so the job is actually resubmitted and we race with finishing and
destroying it.
Well that is a really ugly problem we have here, but your solution
should work.
Christian.
Andrey
And we should probably commit patch #1 and #2.
Christian.
Am 22.04.19 um 13:54 schrieb Grodzovsky, Andrey:
Ping for patches 3, new patch 5 and patch 6.
Andrey
On 4/18/19 11:00 AM, Andrey Grodzovsky wrote:
Also reject TDRs if another one already running.
v2:
Stop all schedulers across device and entire XGMI hive before
force signaling HW fences.
Avoid passing job_signaled to helper fnctions to keep all the decision
making about skipping HW reset in one place.
v3:
Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced
against it's decrement in drm_sched_stop in non HW reset case.
v4: rebase
v5: Revert v3 as we do it now in sceduler code.
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143
+++--
1 file changed, 95 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index a0e165c..85f8792 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3334,8 +3334,6 @@ static int amdgpu_device_pre_asic_reset(struct
amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- drm_sched_stop(>sched, >base);
-
/* after all hw jobs are reset, hw fence is meaningless, so
force_completion */
amdgpu_fence_driver_force_completion(ring);
}
@@ -3343,6 +3341,7 @@ static int amdgpu_device_pre_asic_reset(struct
amdgpu_device *adev,
if(job)
drm_sched_increase_karma(>base);
+ /* Don't suspend on bare metal if we are not going to HW reset the ASIC */
if (!amdgpu_sriov_vf(adev)) {
if (!need_full_reset)
@@ -3480,37 +3479,21 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info
*hive,
return r;
}
-static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev)
+static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, bool trylock)
{
- int i;
-
- for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
- struct amdgpu_ring *ring = adev->rings[i];
-
- if (!ring || !ring->sched.thread)
- continue;
-
- if (!adev->asic_reset_res)
- drm_sched_resubmit_jobs(>sched);
+ if (trylock) {
+ if (!mutex_trylock(>lock_reset))
+ return false;
+ } else
+ mutex_lock(>lock_reset);
- drm_sched_start(>sched, !adev->asic_reset_res);
- }
-
- if (!amdgpu_device_has_dc_support(adev)) {
- drm_helper_resume_force_mode(adev->ddev);
- }
-
- adev->asic_reset_res = 0;
-}
-
-static void amdgpu_device_lock_adev(struct amdgpu_device *adev)
-{
- mutex_lock(>lock_reset);
atomic_inc(>gpu_reset_counter);
adev->in_gpu_reset = 1;
/* Block kfd: SRIOV would do it separately */
if (!amdgpu_sriov_vf(adev))
amdgpu_amdkfd_pre_reset(adev);
+
+ return true;
}
static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
@@ -3538,40 +3521,42 @@ static void amdgpu_device_unlock_adev(struct
amdgpu_device *adev)
int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
struct amdgpu_job *job)
{
- int r;
+ struct list_head device_list, *device_list_handle = NULL;
+ bool need_full_reset, job_signaled;
struct amdgpu_hive_info *hive = NULL;
- bool need_full_reset = false;
struct amdgpu_device *tmp_adev = NULL;
- struct list_head device_list, *device_list_handle = NULL;
+ int i, r = 0;
+ need_full_reset = job_signaled = false;
INIT_LIST_HEAD(_list);
dev_info(adev->dev, "GPU reset begin!\n");
+ hive = amdgpu_get_xgmi_hive(adev, false);
+
/*
-* In case of XGMI hive disallow concurrent resets to be triggered
-* by different nodes. No point also since the one node already
executing
-* reset will also reset all the other nodes in the hive.
+* Here we trylock to avoid chain of resets executing from
+* either trigger by jobs on different adevs in XGMI hive or jobs on
+* different schedulers for same device while this TO handler is
running.
+* We always reset all
Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
On 4/23/19 8:32 AM, Koenig, Christian wrote:
> Well you at least have to give me time till after the holidays to get
> going again :)
>
> Not sure exactly jet why we need patch number 5.
Probably you missed the mail where I pointed out a bug I found during
testing - I am reattaching the mail and the KASAN dump.
Andrey
>
> And we should probably commit patch #1 and #2.
>
> Christian.
>
> Am 22.04.19 um 13:54 schrieb Grodzovsky, Andrey:
>> Ping for patches 3, new patch 5 and patch 6.
>>
>> Andrey
>>
>> On 4/18/19 11:00 AM, Andrey Grodzovsky wrote:
>>> Also reject TDRs if another one already running.
>>>
>>> v2:
>>> Stop all schedulers across device and entire XGMI hive before
>>> force signaling HW fences.
>>> Avoid passing job_signaled to helper fnctions to keep all the decision
>>> making about skipping HW reset in one place.
>>>
>>> v3:
>>> Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced
>>> against it's decrement in drm_sched_stop in non HW reset case.
>>> v4: rebase
>>> v5: Revert v3 as we do it now in sceduler code.
>>>
>>> Signed-off-by: Andrey Grodzovsky
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143
>>> +++--
>>> 1 file changed, 95 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index a0e165c..85f8792 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -3334,8 +3334,6 @@ static int amdgpu_device_pre_asic_reset(struct
>>> amdgpu_device *adev,
>>> if (!ring || !ring->sched.thread)
>>> continue;
>>>
>>> - drm_sched_stop(>sched, >base);
>>> -
>>> /* after all hw jobs are reset, hw fence is
>>> meaningless, so force_completion */
>>> amdgpu_fence_driver_force_completion(ring);
>>> }
>>> @@ -3343,6 +3341,7 @@ static int amdgpu_device_pre_asic_reset(struct
>>> amdgpu_device *adev,
>>> if(job)
>>> drm_sched_increase_karma(>base);
>>>
>>> + /* Don't suspend on bare metal if we are not going to HW reset the ASIC
>>> */
>>> if (!amdgpu_sriov_vf(adev)) {
>>>
>>> if (!need_full_reset)
>>> @@ -3480,37 +3479,21 @@ static int amdgpu_do_asic_reset(struct
>>> amdgpu_hive_info *hive,
>>> return r;
>>> }
>>>
>>> -static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev)
>>> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, bool
>>> trylock)
>>> {
>>> - int i;
>>> -
>>> - for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>>> - struct amdgpu_ring *ring = adev->rings[i];
>>> -
>>> - if (!ring || !ring->sched.thread)
>>> - continue;
>>> -
>>> - if (!adev->asic_reset_res)
>>> - drm_sched_resubmit_jobs(>sched);
>>> + if (trylock) {
>>> + if (!mutex_trylock(>lock_reset))
>>> + return false;
>>> + } else
>>> + mutex_lock(>lock_reset);
>>>
>>> - drm_sched_start(>sched, !adev->asic_reset_res);
>>> - }
>>> -
>>> - if (!amdgpu_device_has_dc_support(adev)) {
>>> - drm_helper_resume_force_mode(adev->ddev);
>>> - }
>>> -
>>> - adev->asic_reset_res = 0;
>>> -}
>>> -
>>> -static void amdgpu_device_lock_adev(struct amdgpu_device *adev)
>>> -{
>>> - mutex_lock(>lock_reset);
>>> atomic_inc(>gpu_reset_counter);
>>> adev->in_gpu_reset = 1;
>>> /* Block kfd: SRIOV would do it separately */
>>> if (!amdgpu_sriov_vf(adev))
>>> amdgpu_amdkfd_pre_reset(adev);
>>> +
>>> + return true;
>>> }
>>>
>>> static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
>>> @@ -3538,40 +3521,42 @@ static void amdgpu_device_unlock_adev(struct
>>> amdgpu_device *adev)
>>> int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>>> struct amdgpu_job *job)
>>> {
>>> - int r;
>>> + struct list_head device_list, *device_list_handle = NULL;
>>> + bool need_full_reset, job_signaled;
>>> struct amdgpu_hive_info *hive = NULL;
>>> - bool need_full_reset = false;
>>> struct amdgpu_device *tmp_adev = NULL;
>>> - struct list_head device_list, *device_list_handle = NULL;
>>> + int i, r = 0;
>>>
>>> + need_full_reset = job_signaled = false;
>>> INIT_LIST_HEAD(_list);
>>>
>>> dev_info(adev->dev, "GPU reset begin!\n");
>>>
>>> + hive = amdgpu_get_xgmi_hive(adev, false);
>>> +
>>> /*
>>> -* In case of XGMI hive disallow concurrent resets to be triggered
>>> -* by different nodes. No point also since the one node already
>>> executing
>>> -* reset will also reset all the other nodes in the hive.
>>> +* Here we
Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
OK, i will merge them into amd-staging drm-next.
Andrey
On 4/23/19 9:14 AM, Kazlauskas, Nicholas wrote:
> Feel free to merge 1+2 since they don't really depend on any other work
> in the series and they were previously reviewed.
>
> Nicholas Kazlauskas
>
> On 4/23/19 8:32 AM, Koenig, Christian wrote:
>> Well you at least have to give me time till after the holidays to get
>> going again :)
>>
>> Not sure exactly jet why we need patch number 5.
>>
>> And we should probably commit patch #1 and #2.
>>
>> Christian.
>>
>> Am 22.04.19 um 13:54 schrieb Grodzovsky, Andrey:
>>> Ping for patches 3, new patch 5 and patch 6.
>>>
>>> Andrey
>>>
>>> On 4/18/19 11:00 AM, Andrey Grodzovsky wrote:
Also reject TDRs if another one already running.
v2:
Stop all schedulers across device and entire XGMI hive before
force signaling HW fences.
Avoid passing job_signaled to helper fnctions to keep all the decision
making about skipping HW reset in one place.
v3:
Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced
against it's decrement in drm_sched_stop in non HW reset case.
v4: rebase
v5: Revert v3 as we do it now in sceduler code.
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143
+++--
1 file changed, 95 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index a0e165c..85f8792 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3334,8 +3334,6 @@ static int amdgpu_device_pre_asic_reset(struct
amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- drm_sched_stop(>sched, >base);
-
/* after all hw jobs are reset, hw fence is
meaningless, so force_completion */
amdgpu_fence_driver_force_completion(ring);
}
@@ -3343,6 +3341,7 @@ static int amdgpu_device_pre_asic_reset(struct
amdgpu_device *adev,
if(job)
drm_sched_increase_karma(>base);
+ /* Don't suspend on bare metal if we are not going to HW reset the ASIC
*/
if (!amdgpu_sriov_vf(adev)) {
if (!need_full_reset)
@@ -3480,37 +3479,21 @@ static int amdgpu_do_asic_reset(struct
amdgpu_hive_info *hive,
return r;
}
-static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev)
+static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, bool
trylock)
{
- int i;
-
- for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
- struct amdgpu_ring *ring = adev->rings[i];
-
- if (!ring || !ring->sched.thread)
- continue;
-
- if (!adev->asic_reset_res)
- drm_sched_resubmit_jobs(>sched);
+ if (trylock) {
+ if (!mutex_trylock(>lock_reset))
+ return false;
+ } else
+ mutex_lock(>lock_reset);
- drm_sched_start(>sched, !adev->asic_reset_res);
- }
-
- if (!amdgpu_device_has_dc_support(adev)) {
- drm_helper_resume_force_mode(adev->ddev);
- }
-
- adev->asic_reset_res = 0;
-}
-
-static void amdgpu_device_lock_adev(struct amdgpu_device *adev)
-{
- mutex_lock(>lock_reset);
atomic_inc(>gpu_reset_counter);
adev->in_gpu_reset = 1;
/* Block kfd: SRIOV would do it separately */
if (!amdgpu_sriov_vf(adev))
amdgpu_amdkfd_pre_reset(adev);
+
+ return true;
}
static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
@@ -3538,40 +3521,42 @@ static void amdgpu_device_unlock_adev(struct
amdgpu_device *adev)
int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
struct amdgpu_job *job)
{
- int r;
+ struct list_head device_list, *device_list_handle = NULL;
+ bool need_full_reset, job_signaled;
struct amdgpu_hive_info *hive = NULL;
- bool need_full_reset = false;
struct amdgpu_device *tmp_adev = NULL;
- struct list_head device_list, *device_list_handle = NULL;
+ int i, r = 0;
+ need_full_reset = job_signaled = false;
INIT_LIST_HEAD(_list);
dev_info(adev->dev, "GPU reset begin!\n");
+ hive = amdgpu_get_xgmi_hive(adev, false);
+
/*
- * In case of XGMI
Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
Feel free to merge 1+2 since they don't really depend on any other work
in the series and they were previously reviewed.
Nicholas Kazlauskas
On 4/23/19 8:32 AM, Koenig, Christian wrote:
> Well you at least have to give me time till after the holidays to get
> going again :)
>
> Not sure exactly jet why we need patch number 5.
>
> And we should probably commit patch #1 and #2.
>
> Christian.
>
> Am 22.04.19 um 13:54 schrieb Grodzovsky, Andrey:
>> Ping for patches 3, new patch 5 and patch 6.
>>
>> Andrey
>>
>> On 4/18/19 11:00 AM, Andrey Grodzovsky wrote:
>>> Also reject TDRs if another one already running.
>>>
>>> v2:
>>> Stop all schedulers across device and entire XGMI hive before
>>> force signaling HW fences.
>>> Avoid passing job_signaled to helper fnctions to keep all the decision
>>> making about skipping HW reset in one place.
>>>
>>> v3:
>>> Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced
>>> against it's decrement in drm_sched_stop in non HW reset case.
>>> v4: rebase
>>> v5: Revert v3 as we do it now in sceduler code.
>>>
>>> Signed-off-by: Andrey Grodzovsky
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143
>>> +++--
>>> 1 file changed, 95 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index a0e165c..85f8792 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -3334,8 +3334,6 @@ static int amdgpu_device_pre_asic_reset(struct
>>> amdgpu_device *adev,
>>> if (!ring || !ring->sched.thread)
>>> continue;
>>>
>>> - drm_sched_stop(>sched, >base);
>>> -
>>> /* after all hw jobs are reset, hw fence is
>>> meaningless, so force_completion */
>>> amdgpu_fence_driver_force_completion(ring);
>>> }
>>> @@ -3343,6 +3341,7 @@ static int amdgpu_device_pre_asic_reset(struct
>>> amdgpu_device *adev,
>>> if(job)
>>> drm_sched_increase_karma(>base);
>>>
>>> + /* Don't suspend on bare metal if we are not going to HW reset the ASIC
>>> */
>>> if (!amdgpu_sriov_vf(adev)) {
>>>
>>> if (!need_full_reset)
>>> @@ -3480,37 +3479,21 @@ static int amdgpu_do_asic_reset(struct
>>> amdgpu_hive_info *hive,
>>> return r;
>>> }
>>>
>>> -static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev)
>>> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, bool
>>> trylock)
>>> {
>>> - int i;
>>> -
>>> - for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>>> - struct amdgpu_ring *ring = adev->rings[i];
>>> -
>>> - if (!ring || !ring->sched.thread)
>>> - continue;
>>> -
>>> - if (!adev->asic_reset_res)
>>> - drm_sched_resubmit_jobs(>sched);
>>> + if (trylock) {
>>> + if (!mutex_trylock(>lock_reset))
>>> + return false;
>>> + } else
>>> + mutex_lock(>lock_reset);
>>>
>>> - drm_sched_start(>sched, !adev->asic_reset_res);
>>> - }
>>> -
>>> - if (!amdgpu_device_has_dc_support(adev)) {
>>> - drm_helper_resume_force_mode(adev->ddev);
>>> - }
>>> -
>>> - adev->asic_reset_res = 0;
>>> -}
>>> -
>>> -static void amdgpu_device_lock_adev(struct amdgpu_device *adev)
>>> -{
>>> - mutex_lock(>lock_reset);
>>> atomic_inc(>gpu_reset_counter);
>>> adev->in_gpu_reset = 1;
>>> /* Block kfd: SRIOV would do it separately */
>>> if (!amdgpu_sriov_vf(adev))
>>> amdgpu_amdkfd_pre_reset(adev);
>>> +
>>> + return true;
>>> }
>>>
>>> static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
>>> @@ -3538,40 +3521,42 @@ static void amdgpu_device_unlock_adev(struct
>>> amdgpu_device *adev)
>>> int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>>> struct amdgpu_job *job)
>>> {
>>> - int r;
>>> + struct list_head device_list, *device_list_handle = NULL;
>>> + bool need_full_reset, job_signaled;
>>> struct amdgpu_hive_info *hive = NULL;
>>> - bool need_full_reset = false;
>>> struct amdgpu_device *tmp_adev = NULL;
>>> - struct list_head device_list, *device_list_handle = NULL;
>>> + int i, r = 0;
>>>
>>> + need_full_reset = job_signaled = false;
>>> INIT_LIST_HEAD(_list);
>>>
>>> dev_info(adev->dev, "GPU reset begin!\n");
>>>
>>> + hive = amdgpu_get_xgmi_hive(adev, false);
>>> +
>>> /*
>>> -* In case of XGMI hive disallow concurrent resets to be triggered
>>> -* by different nodes. No point also since the one node already
>>> executing
>>> -* reset will also reset all the other nodes in the hive.
>>> +*
Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
Well you at least have to give me time till after the holidays to get
going again :)
Not sure exactly jet why we need patch number 5.
And we should probably commit patch #1 and #2.
Christian.
Am 22.04.19 um 13:54 schrieb Grodzovsky, Andrey:
> Ping for patches 3, new patch 5 and patch 6.
>
> Andrey
>
> On 4/18/19 11:00 AM, Andrey Grodzovsky wrote:
>> Also reject TDRs if another one already running.
>>
>> v2:
>> Stop all schedulers across device and entire XGMI hive before
>> force signaling HW fences.
>> Avoid passing job_signaled to helper fnctions to keep all the decision
>> making about skipping HW reset in one place.
>>
>> v3:
>> Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced
>> against it's decrement in drm_sched_stop in non HW reset case.
>> v4: rebase
>> v5: Revert v3 as we do it now in sceduler code.
>>
>> Signed-off-by: Andrey Grodzovsky
>> ---
>>drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143
>> +++--
>>1 file changed, 95 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index a0e165c..85f8792 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3334,8 +3334,6 @@ static int amdgpu_device_pre_asic_reset(struct
>> amdgpu_device *adev,
>> if (!ring || !ring->sched.thread)
>> continue;
>>
>> -drm_sched_stop(>sched, >base);
>> -
>> /* after all hw jobs are reset, hw fence is meaningless, so
>> force_completion */
>> amdgpu_fence_driver_force_completion(ring);
>> }
>> @@ -3343,6 +3341,7 @@ static int amdgpu_device_pre_asic_reset(struct
>> amdgpu_device *adev,
>> if(job)
>> drm_sched_increase_karma(>base);
>>
>> +/* Don't suspend on bare metal if we are not going to HW reset the ASIC
>> */
>> if (!amdgpu_sriov_vf(adev)) {
>>
>> if (!need_full_reset)
>> @@ -3480,37 +3479,21 @@ static int amdgpu_do_asic_reset(struct
>> amdgpu_hive_info *hive,
>> return r;
>>}
>>
>> -static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev)
>> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, bool
>> trylock)
>>{
>> -int i;
>> -
>> -for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>> -struct amdgpu_ring *ring = adev->rings[i];
>> -
>> -if (!ring || !ring->sched.thread)
>> -continue;
>> -
>> -if (!adev->asic_reset_res)
>> -drm_sched_resubmit_jobs(>sched);
>> +if (trylock) {
>> +if (!mutex_trylock(>lock_reset))
>> +return false;
>> +} else
>> +mutex_lock(>lock_reset);
>>
>> -drm_sched_start(>sched, !adev->asic_reset_res);
>> -}
>> -
>> -if (!amdgpu_device_has_dc_support(adev)) {
>> -drm_helper_resume_force_mode(adev->ddev);
>> -}
>> -
>> -adev->asic_reset_res = 0;
>> -}
>> -
>> -static void amdgpu_device_lock_adev(struct amdgpu_device *adev)
>> -{
>> -mutex_lock(>lock_reset);
>> atomic_inc(>gpu_reset_counter);
>> adev->in_gpu_reset = 1;
>> /* Block kfd: SRIOV would do it separately */
>> if (!amdgpu_sriov_vf(adev))
>>amdgpu_amdkfd_pre_reset(adev);
>> +
>> +return true;
>>}
>>
>>static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
>> @@ -3538,40 +3521,42 @@ static void amdgpu_device_unlock_adev(struct
>> amdgpu_device *adev)
>>int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>>struct amdgpu_job *job)
>>{
>> -int r;
>> +struct list_head device_list, *device_list_handle = NULL;
>> +bool need_full_reset, job_signaled;
>> struct amdgpu_hive_info *hive = NULL;
>> -bool need_full_reset = false;
>> struct amdgpu_device *tmp_adev = NULL;
>> -struct list_head device_list, *device_list_handle = NULL;
>> +int i, r = 0;
>>
>> +need_full_reset = job_signaled = false;
>> INIT_LIST_HEAD(_list);
>>
>> dev_info(adev->dev, "GPU reset begin!\n");
>>
>> +hive = amdgpu_get_xgmi_hive(adev, false);
>> +
>> /*
>> - * In case of XGMI hive disallow concurrent resets to be triggered
>> - * by different nodes. No point also since the one node already
>> executing
>> - * reset will also reset all the other nodes in the hive.
>> + * Here we trylock to avoid chain of resets executing from
>> + * either trigger by jobs on different adevs in XGMI hive or jobs on
>> + * different schedulers for same device while this TO handler is
>> running.
>> + * We always reset all schedulers for device and all devices for XGMI
>> + * hive so that should take care of them too.
>> */
>> -hive = amdgpu_get_xgmi_hive(adev, 0);
>> -if (hive &&
Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
+Monk.
GPU reset is used widely in SRIOV, so need virtulizatino guy take a look.
But out of curious, why guilty job can signal more if the job is already
set to guilty? set it wrongly?
-David
在 2019/4/18 23:00, Andrey Grodzovsky 写道:
> Also reject TDRs if another one already running.
>
> v2:
> Stop all schedulers across device and entire XGMI hive before
> force signaling HW fences.
> Avoid passing job_signaled to helper fnctions to keep all the decision
> making about skipping HW reset in one place.
>
> v3:
> Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced
> against it's decrement in drm_sched_stop in non HW reset case.
> v4: rebase
> v5: Revert v3 as we do it now in sceduler code.
>
> Signed-off-by: Andrey Grodzovsky
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143
> +++--
> 1 file changed, 95 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index a0e165c..85f8792 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3334,8 +3334,6 @@ static int amdgpu_device_pre_asic_reset(struct
> amdgpu_device *adev,
> if (!ring || !ring->sched.thread)
> continue;
>
> - drm_sched_stop(>sched, >base);
> -
> /* after all hw jobs are reset, hw fence is meaningless, so
> force_completion */
> amdgpu_fence_driver_force_completion(ring);
> }
> @@ -3343,6 +3341,7 @@ static int amdgpu_device_pre_asic_reset(struct
> amdgpu_device *adev,
> if(job)
> drm_sched_increase_karma(>base);
>
> + /* Don't suspend on bare metal if we are not going to HW reset the ASIC
> */
> if (!amdgpu_sriov_vf(adev)) {
>
> if (!need_full_reset)
> @@ -3480,37 +3479,21 @@ static int amdgpu_do_asic_reset(struct
> amdgpu_hive_info *hive,
> return r;
> }
>
> -static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev)
> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, bool trylock)
> {
> - int i;
> -
> - for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> - struct amdgpu_ring *ring = adev->rings[i];
> -
> - if (!ring || !ring->sched.thread)
> - continue;
> -
> - if (!adev->asic_reset_res)
> - drm_sched_resubmit_jobs(>sched);
> + if (trylock) {
> + if (!mutex_trylock(>lock_reset))
> + return false;
> + } else
> + mutex_lock(>lock_reset);
>
> - drm_sched_start(>sched, !adev->asic_reset_res);
> - }
> -
> - if (!amdgpu_device_has_dc_support(adev)) {
> - drm_helper_resume_force_mode(adev->ddev);
> - }
> -
> - adev->asic_reset_res = 0;
> -}
> -
> -static void amdgpu_device_lock_adev(struct amdgpu_device *adev)
> -{
> - mutex_lock(>lock_reset);
> atomic_inc(>gpu_reset_counter);
> adev->in_gpu_reset = 1;
> /* Block kfd: SRIOV would do it separately */
> if (!amdgpu_sriov_vf(adev))
> amdgpu_amdkfd_pre_reset(adev);
> +
> + return true;
> }
>
> static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
> @@ -3538,40 +3521,42 @@ static void amdgpu_device_unlock_adev(struct
> amdgpu_device *adev)
> int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
> struct amdgpu_job *job)
> {
> - int r;
> + struct list_head device_list, *device_list_handle = NULL;
> + bool need_full_reset, job_signaled;
> struct amdgpu_hive_info *hive = NULL;
> - bool need_full_reset = false;
> struct amdgpu_device *tmp_adev = NULL;
> - struct list_head device_list, *device_list_handle = NULL;
> + int i, r = 0;
>
> + need_full_reset = job_signaled = false;
> INIT_LIST_HEAD(_list);
>
> dev_info(adev->dev, "GPU reset begin!\n");
>
> + hive = amdgpu_get_xgmi_hive(adev, false);
> +
> /*
> - * In case of XGMI hive disallow concurrent resets to be triggered
> - * by different nodes. No point also since the one node already
> executing
> - * reset will also reset all the other nodes in the hive.
> + * Here we trylock to avoid chain of resets executing from
> + * either trigger by jobs on different adevs in XGMI hive or jobs on
> + * different schedulers for same device while this TO handler is
> running.
> + * We always reset all schedulers for device and all devices for XGMI
> + * hive so that should take care of them too.
>*/
> - hive = amdgpu_get_xgmi_hive(adev, 0);
> - if (hive && adev->gmc.xgmi.num_physical_nodes > 1 &&
> - !mutex_trylock(>reset_lock))
> +
> + if (hive && !mutex_trylock(>reset_lock)) {
> + DRM_INFO("Bailing on TDR for s_job:%llx, hive: %llx as another
> already in
Re: [PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
Ping for patches 3, new patch 5 and patch 6.
Andrey
On 4/18/19 11:00 AM, Andrey Grodzovsky wrote:
> Also reject TDRs if another one already running.
>
> v2:
> Stop all schedulers across device and entire XGMI hive before
> force signaling HW fences.
> Avoid passing job_signaled to helper fnctions to keep all the decision
> making about skipping HW reset in one place.
>
> v3:
> Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced
> against it's decrement in drm_sched_stop in non HW reset case.
> v4: rebase
> v5: Revert v3 as we do it now in sceduler code.
>
> Signed-off-by: Andrey Grodzovsky
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143
> +++--
> 1 file changed, 95 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index a0e165c..85f8792 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3334,8 +3334,6 @@ static int amdgpu_device_pre_asic_reset(struct
> amdgpu_device *adev,
> if (!ring || !ring->sched.thread)
> continue;
>
> - drm_sched_stop(>sched, >base);
> -
> /* after all hw jobs are reset, hw fence is meaningless, so
> force_completion */
> amdgpu_fence_driver_force_completion(ring);
> }
> @@ -3343,6 +3341,7 @@ static int amdgpu_device_pre_asic_reset(struct
> amdgpu_device *adev,
> if(job)
> drm_sched_increase_karma(>base);
>
> + /* Don't suspend on bare metal if we are not going to HW reset the ASIC
> */
> if (!amdgpu_sriov_vf(adev)) {
>
> if (!need_full_reset)
> @@ -3480,37 +3479,21 @@ static int amdgpu_do_asic_reset(struct
> amdgpu_hive_info *hive,
> return r;
> }
>
> -static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev)
> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, bool trylock)
> {
> - int i;
> -
> - for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> - struct amdgpu_ring *ring = adev->rings[i];
> -
> - if (!ring || !ring->sched.thread)
> - continue;
> -
> - if (!adev->asic_reset_res)
> - drm_sched_resubmit_jobs(>sched);
> + if (trylock) {
> + if (!mutex_trylock(>lock_reset))
> + return false;
> + } else
> + mutex_lock(>lock_reset);
>
> - drm_sched_start(>sched, !adev->asic_reset_res);
> - }
> -
> - if (!amdgpu_device_has_dc_support(adev)) {
> - drm_helper_resume_force_mode(adev->ddev);
> - }
> -
> - adev->asic_reset_res = 0;
> -}
> -
> -static void amdgpu_device_lock_adev(struct amdgpu_device *adev)
> -{
> - mutex_lock(>lock_reset);
> atomic_inc(>gpu_reset_counter);
> adev->in_gpu_reset = 1;
> /* Block kfd: SRIOV would do it separately */
> if (!amdgpu_sriov_vf(adev))
> amdgpu_amdkfd_pre_reset(adev);
> +
> + return true;
> }
>
> static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
> @@ -3538,40 +3521,42 @@ static void amdgpu_device_unlock_adev(struct
> amdgpu_device *adev)
> int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
> struct amdgpu_job *job)
> {
> - int r;
> + struct list_head device_list, *device_list_handle = NULL;
> + bool need_full_reset, job_signaled;
> struct amdgpu_hive_info *hive = NULL;
> - bool need_full_reset = false;
> struct amdgpu_device *tmp_adev = NULL;
> - struct list_head device_list, *device_list_handle = NULL;
> + int i, r = 0;
>
> + need_full_reset = job_signaled = false;
> INIT_LIST_HEAD(_list);
>
> dev_info(adev->dev, "GPU reset begin!\n");
>
> + hive = amdgpu_get_xgmi_hive(adev, false);
> +
> /*
> - * In case of XGMI hive disallow concurrent resets to be triggered
> - * by different nodes. No point also since the one node already
> executing
> - * reset will also reset all the other nodes in the hive.
> + * Here we trylock to avoid chain of resets executing from
> + * either trigger by jobs on different adevs in XGMI hive or jobs on
> + * different schedulers for same device while this TO handler is
> running.
> + * We always reset all schedulers for device and all devices for XGMI
> + * hive so that should take care of them too.
>*/
> - hive = amdgpu_get_xgmi_hive(adev, 0);
> - if (hive && adev->gmc.xgmi.num_physical_nodes > 1 &&
> - !mutex_trylock(>reset_lock))
> +
> + if (hive && !mutex_trylock(>reset_lock)) {
> + DRM_INFO("Bailing on TDR for s_job:%llx, hive: %llx as another
> already in progress",
> + job->base.id, hive->hive_id);
> return 0;
> + }
>
> /* Start with adev pre asic
[PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
Also reject TDRs if another one already running.
v2:
Stop all schedulers across device and entire XGMI hive before
force signaling HW fences.
Avoid passing job_signaled to helper fnctions to keep all the decision
making about skipping HW reset in one place.
v3:
Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced
against it's decrement in drm_sched_stop in non HW reset case.
v4: rebase
v5: Revert v3 as we do it now in sceduler code.
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143 +++--
1 file changed, 95 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index a0e165c..85f8792 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3334,8 +3334,6 @@ static int amdgpu_device_pre_asic_reset(struct
amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- drm_sched_stop(>sched, >base);
-
/* after all hw jobs are reset, hw fence is meaningless, so
force_completion */
amdgpu_fence_driver_force_completion(ring);
}
@@ -3343,6 +3341,7 @@ static int amdgpu_device_pre_asic_reset(struct
amdgpu_device *adev,
if(job)
drm_sched_increase_karma(>base);
+ /* Don't suspend on bare metal if we are not going to HW reset the ASIC
*/
if (!amdgpu_sriov_vf(adev)) {
if (!need_full_reset)
@@ -3480,37 +3479,21 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info
*hive,
return r;
}
-static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev)
+static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, bool trylock)
{
- int i;
-
- for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
- struct amdgpu_ring *ring = adev->rings[i];
-
- if (!ring || !ring->sched.thread)
- continue;
-
- if (!adev->asic_reset_res)
- drm_sched_resubmit_jobs(>sched);
+ if (trylock) {
+ if (!mutex_trylock(>lock_reset))
+ return false;
+ } else
+ mutex_lock(>lock_reset);
- drm_sched_start(>sched, !adev->asic_reset_res);
- }
-
- if (!amdgpu_device_has_dc_support(adev)) {
- drm_helper_resume_force_mode(adev->ddev);
- }
-
- adev->asic_reset_res = 0;
-}
-
-static void amdgpu_device_lock_adev(struct amdgpu_device *adev)
-{
- mutex_lock(>lock_reset);
atomic_inc(>gpu_reset_counter);
adev->in_gpu_reset = 1;
/* Block kfd: SRIOV would do it separately */
if (!amdgpu_sriov_vf(adev))
amdgpu_amdkfd_pre_reset(adev);
+
+ return true;
}
static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
@@ -3538,40 +3521,42 @@ static void amdgpu_device_unlock_adev(struct
amdgpu_device *adev)
int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
struct amdgpu_job *job)
{
- int r;
+ struct list_head device_list, *device_list_handle = NULL;
+ bool need_full_reset, job_signaled;
struct amdgpu_hive_info *hive = NULL;
- bool need_full_reset = false;
struct amdgpu_device *tmp_adev = NULL;
- struct list_head device_list, *device_list_handle = NULL;
+ int i, r = 0;
+ need_full_reset = job_signaled = false;
INIT_LIST_HEAD(_list);
dev_info(adev->dev, "GPU reset begin!\n");
+ hive = amdgpu_get_xgmi_hive(adev, false);
+
/*
-* In case of XGMI hive disallow concurrent resets to be triggered
-* by different nodes. No point also since the one node already
executing
-* reset will also reset all the other nodes in the hive.
+* Here we trylock to avoid chain of resets executing from
+* either trigger by jobs on different adevs in XGMI hive or jobs on
+* different schedulers for same device while this TO handler is
running.
+* We always reset all schedulers for device and all devices for XGMI
+* hive so that should take care of them too.
*/
- hive = amdgpu_get_xgmi_hive(adev, 0);
- if (hive && adev->gmc.xgmi.num_physical_nodes > 1 &&
- !mutex_trylock(>reset_lock))
+
+ if (hive && !mutex_trylock(>reset_lock)) {
+ DRM_INFO("Bailing on TDR for s_job:%llx, hive: %llx as another
already in progress",
+job->base.id, hive->hive_id);
return 0;
+ }
/* Start with adev pre asic reset first for soft reset check.*/
- amdgpu_device_lock_adev(adev);
- r = amdgpu_device_pre_asic_reset(adev,
-job,
-_full_reset);
- if (r) {
-
