[PATCH 4/4] drm/amdgpu/SRIOV:implement guilty job TDR (V2)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> You still avoid my question: whatâ??s the theoretical backend you that 
> you think check once instead of twice or even more is good**before** 
> hw_job_reset() ?
Because stopping all the scheduler threads takes a moment and it is 
entirely possible that the job finishes within that time.

> 1) if you check the fence and found it not signaled, then you will 
> call hw_job_reset(), but there is still chance that between your check 
> and the hw_job_reset() the
>
> Sched fence could signaled , isnâ??t it ? you still cannot avoid such 
> race condition
>
Crap, you're right. We would indeed need to check twice and that 
wouldn't be consistent anyway.

Once after stopping the schedulers and before hw_job_reset() because 
then we can just start the schedulers again and continue as if nothing 
has happened.

And another time after calling hw_job_reset() if we want to set the 
error code.

> Donâ??t forget your approach still have chance to hit the race 
> condition, and to me I donâ??t think the race condition matters thatâ??s 
> why I donâ??t even consider it
>
Yeah, you convinced me. Please go ahead with the current approach, but 
at least add a comment that we might want to improve that.

Regards,
Christian.

Am 10.05.2017 um 13:02 schrieb Liu, Monk:
>
> > Checking a second time is pointless since it can't signal any more 
> after calling amd_sched_hw_job_reset().
>
> [ML] you seems not response to me â?¦ I of cause know fence cannot 
> signal after hw_job_reset() â?¦.
>
> My question is , before you call hw_job_reset(), why you want to check 
> the fence ? why not check twice ?
>
> You still avoid my question: whatâ??s the theoretical backend you that 
> you think check once instead of twice or even more is good**before** 
> hw_job_reset() ?
>
> >No, the timeout is pretty meaningless. It's just the trigger that we 
> need to do something.
>
> 2) And even it signaled after entering gpu_reset(), it will 
> automatically done like normal cases, thatâ??s good. Why remove those 
> callback instead ?
>
> > No, that's absolutely not good. We don't know if it's the hardware 
> which results in the job being signaled or our reset code.
>
> [ML] you are wrong, for SR-IOV case, the timeout is all that matters, 
> because one VF can only have such time slice within timeout, and Iâ??m 
> doing the TDR on SR-IOV so donâ??t
>
> Always looks things with bare-metal mind
>
> >Otherwise we have a race condition here where we can't determine if 
> the reset finished the job or if it did just on it's own while we 
> stopped the scheduler.
>
> [ML] you are wrong :
> 1) if you check the fence and found it not signaled, then you will 
> call hw_job_reset(), but there is still chance that between your check 
> and the hw_job_reset() the
>
> Sched fence could signaled , isnâ??t it ? you still cannot avoid such 
> race condition
>
> 2) I donâ??t care if the fence is signaled due to its own finish or 
> because we force the hw fence signaled, for SR-IOV case, as long as 
> the job exceeds timeout, we consider
>
> It hang.
>
> 3) Even for bare-metal case, you still cannot sure if the fence is 
> signaled due to its own or hw_fence force signal, reason is in #1)
>
> >No, you are insist on a vague rules not strict, like I said, what is the 
> theoretic to backend your approach that only check once on the in 
> question job ? why not check again if not signaled ?
>
> >Because we have removed the connection between the job and the 
> hardware fence and because of this the job can never signal.
>
> [ML] like I said, before you call hw_job_reset(), you only check on 
> the job once, why not check again and again ?  I donâ??t see you have a 
> reason to only check once, and
>
> If you donâ??t have such reason I think you should not check at all.
>
> If you have such reason, prove me that only check once is good and enough.
>
> Donâ??t forget your approach still have chance to hit the race 
> condition, and to me I donâ??t think the race condition matters thatâ??s 
> why I donâ??t even consider it
>
> BR Monk
>
> *From:*amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] *On 
> Behalf Of *Christian König
> *Sent:* Wednesday, May 10, 2017 6:26 PM
> *To:* Liu, Monk <Monk.Liu at amd.com>; Koenig, Christian 
> <Christian.Koenig at amd.com>; amd-gfx at lists.freedesktop.org
> *Subject:* Re: [PATCH 4/4] drm/amdgpu/SRIOV:implement guilty job TDR (V2)
>
> Am 10.05.2017 um 12:05 schrieb Liu, Monk:
>
>     [ML] yes, but we cannot guarantee the job is 100% really hang when
>     entering gpu_reset(), we can only trust our amdgpu_job_timeout as
>     a deadline for each job.
>
>     You approach that check the fence first before charge it as
>     guilty/hang is incorrect looks to me because why you not check it
>     twice, triple, and even more loops ?
>
>     Because the job can't signal any more after calling
>     amd_sched_hw_job_reset().
>
>     [ML] No â?¦ thatâ??s where I think your approach is vague:
>
>     1) see that you check after scheduler stopped, see if job
>     signaled, my question is if the job is not signaled (like most
>     usual case)
>
>     Why you not check it again and again ?  maybe the second time you
>     will find it signaled â?¦
>
>
> Checking a second time is pointless since it can't signal any more 
> after calling amd_sched_hw_job_reset().
>
>
>     My point is the checking here is meaningless, we already have
>     timedout for the guard.
>
> No, the timeout is pretty meaningless. It's just the trigger that we 
> need to do something.
>
> But to determine what to do we first need to stop the scheduler, 
> remove the hardware fence and THEN check the current status.
>
> Otherwise we have a race condition here where we can't determine if 
> the reset finished the job or if it did just on it's own while we 
> stopped the scheduler.
>
>
>     2) And even it signaled after entering gpu_reset(), it will
>     automatically done like normal cases, thatâ??s good. Why remove
>     those callback instead ?
>
> No, that's absolutely not good. We don't know if it's the hardware 
> which results in the job being signaled or our reset code.
>
>
>
>
>         So I refuse to check if @job is just signaled in gpu_reset,
>         because this action is vague (and no one can guarantee the job
>         wonâ??t signal during gpu_reset, we should not argue on this
>         event â?¦), I prefer clean and restrict rules.
>
>     Yeah, completely agree that we need to have struct rules for that.
>     That's why I insists on doing this :)
>
>     No, you are insist on a vague rules not strict, like I said, what
>     is the theoretic to backend your approach that only check once on
>     the in question job ? why not check again if not signaled ?
>
> Because we have removed the connection between the job and the 
> hardware fence and because of this the job can never signal.
>
> Regards,
> Christian.
>
>
>     I donâ??t agree this approach is clean and strict. You are abuse
>     timedout parameter.
>
>
>
>     See I just want to avoid problems for the case that the job
>     signaled while we stop the scheduler (because stopping the
>     scheduler actually can take a moment).
>
>     Because when this happened the scheduler could already have pushed
>     the next job to the hardware and then we abort it with the GPU
>     reset and might create more problems than we solve.
>
>
>
>
>
>         [ML] I donâ??t see my approach will have chance to fence twiceâ?¦
>         on the contrast I think my approach is more clear: no matter
>         the in question job finally signaled or not, I just kick it
>         out from mirror-list
>
>         Without remove the callback from hw fence, that way even it
>         really signaled during the gpu_reset() period the logic is
>         still perfect and its sched fence will act like usual â?¦
>
>     We want to set an error code on the job before signaling it don't
>     we? So we need to be sure how and when the job is signaled as
>     finished.
>
>     I mean letting it signal when we force the hardware fence to
>     complete will work as well, but I still think that this isn't as
>     clean as signaling it manually.
>
>     Please also see the helper function the Intel guys introduced
>     drm_fence_set_error(), we will run into a BUG_ON if we can't
>     guarantee the order of execution here.
>
>     Regards,
>     Christian.
>
>     Am 10.05.2017 um 06:00 schrieb Liu, Monk:
>
>         Christian,
>
>         Looks like we need more discuss with itâ?¦
>
>         Here is your approach:
>
>         1. Stop the scheduler from feeding more jobs to the hardware
>         when a jobs completes.  //this is where I agree with you
>
>         2. Then call hw_job_reset to remove the connection between job
>         and hardware fence.
>
>         3. Test if job is now completed. It is entirely possible that
>         the job completed while we started the work to reset the hardware.
>
>         Removing the connection between the hardware fence and the job
>         is the deadline, if the job completes after that it is lost.
>
>         4. Check the karma and kick out the job if we found it guilty.
>
>         5. Get the whole stuff working again, e.g. reset the hardware,
>         restart the scheduler etc...
>
>         */[ML]: One thing I agree to change with your way: in
>         gpu_reset() we should first stop the in question ringâ??s
>         scheduler (not the all) before kick out the guilty job./*
>
>         > Indeed, but I still think that this is a bad approach cause
>         we then reset the hardware without a good reason.
>
>         [ML] yes, but we cannot guarantee the job is 100% really hang
>         when entering gpu_reset(), we can only trust our
>         amdgpu_job_timeout as a deadline for each job.
>
>         You approach that check the fence first before charge it as
>         guilty/hang is incorrect looks to me because why you not check
>         it twice, triple, and even more loops ?
>
>         You check it one time and you found it just signaled thatâ??s
>         great and lucky(really luckyâ?¦), But what if it didnâ??t signaled
>         (like most usual case) , why not check it again and again ? do
>         you have a theoretic to support on how much time you need to
>         check before finally consider it hang ? No I donâ??t think you
>         have so please just cut this unnecessary checking, we already
>         use amdgpu_job_timeout to give the deadline of each job.
>
>         So I refuse to check if @job is just signaled in gpu_reset,
>         because this action is vague (and no one can guarantee the job
>         wonâ??t signal during gpu_reset, we should not argue on this
>         event â?¦), I prefer clean and restrict rules.
>
>         >Force completion is not so much of the issue, but rather in
>         which order you do things.
>
>         >See the original code first stops the scheduler and removes
>         the connection between hardware fence and job in an atomic
>         manner. And THEN forces the hardware fence to complete.
>
>         >This way we could be sure that nothing happened in parallel,
>         e.g. that we don't try to signal the fence twice or something
>         like that.
>
>         [ML] I donâ??t see my approach will have chance to fence twiceâ?¦
>         on the contrast I think my approach is more clear: no matter
>         the in question job finally signaled or not, I just kick it
>         out from mirror-list
>
>         Without remove the callback from hw fence, that way even it
>         really signaled during the gpu_reset() period the logic is
>         still perfect and its sched fence will act like usual â?¦
>
>         Please point out where or how my approach will go wrong like
>         â??e.g. that we don't try to signal the fence twice or something
>         like that.â??, otherwise I cannot be persuaded and fix my way â?¦
>
>         At last. I run the TDR test and it ends up with Hypervisor
>         side error, the guest side is all perfect, here is what I ran:
>
>         The vk_example and vulkan CTS test under MCBP case, we have
>         all kinds of hang on compute ring, without TDR this test wonâ??t
>         suffer for 5 seconds, and with TDR although MCBP is buggy now
>         but we can
>
>         Finish this test (of cause test result is mismatch due to MCBP
>         issue), and there are tongs of job_timed_out in dmesg, but
>         guest driver didnâ??t have any error report.  I was also
>         surprised this behave really stable â?¦
>
>         The second test the using â??watchâ?? to trigger a gpu hang (bad
>         command stream) every 2 seconds, with amdgpu_job_timeout also
>         set to 2seconds, I can run it till hypervisor side hit VF FLR
>         error, and guest side
>
>         Still runs perfectly and nothing wrong happened, with hw fence
>         seq number I can tell there are 3000 loops of TDR finished
>         before Hypervisor hit error.
>
>         BR Monk
>
>         *From:*Koenig, Christian
>         *Sent:* Tuesday, May 09, 2017 8:52 PM
>         *To:* Liu, Monk <Monk.Liu at amd.com> <mailto:Monk.Liu at amd.com>;
>         Christian König <deathsimple at vodafone.de>
>         <mailto:deathsimple at vodafone.de>;
>         amd-gfx at lists.freedesktop.org
>         <mailto:amd-gfx at lists.freedesktop.org>
>         *Subject:* Re: [PATCH 4/4] drm/amdgpu/SRIOV:implement guilty
>         job TDR (V2)
>
>             [ML] if the job complete, the jobâ??s sched fence callback
>             will take this spin_lock and remove itself from
>             mirror_list, so we are still safe to call
>             amd_sched_job_kickout(), and it will do nothing if so
>
>         Indeed, but I still think that this is a bad approach cause we
>         then reset the hardware without a good reason.
>
>
>
>
>             Besides, original logic also force complete the hw fence,
>             and it works well â?¦
>
>         Force completion is not so much of the issue, but rather in
>         which order you do things.
>
>         See the original code first stops the scheduler and removes
>         the connection between hardware fence and job in an atomic
>         manner. And THEN forces the hardware fence to complete.
>
>         This way we could be sure that nothing happened in parallel,
>         e.g. that we don't try to signal the fence twice or something
>         like that.
>
>
>
>
>             State like â??You are missing that it is entirely possible
>             that the job will complete while we are trying to kick it
>             out.â??
>
>         Sorry I should have been more clear.
>
>
>
>
>             Is not a good reason to reject my approach, because that
>             is okay if the job just completed â?¦
>
>         We usually try to take a defensive approach, so stopping
>         everything, removing the hardware fence connection and then
>         explicitly kicking out the job in question sounds much better
>         than doing it implicitly with the hardware fence completion.
>
>         Even when this works (which I'm still not sure of) that is a
>         really awkward and hard to understand approach.
>
>         Regards,
>         Christian.
>
>         Am 09.05.2017 um 13:58 schrieb Liu, Monk:
>
>             You are missing that it is entirely possible that the job
>             will complete while we are trying to kick it out.
>
>             [ML] if the job complete, the jobâ??s sched fence callback
>             will take this spin_lock and remove itself from
>             mirror_list, so we are still safe to call
>             amd_sched_job_kickout(), and it will do nothing if so
>
>             Please go through the whole steps again,
>
>             Besides, original logic also force complete the hw fence,
>             and it works well â?¦
>
>             I donâ??t see the solid reason why you insist on your
>             approach, please go through the steps again  and give me
>             the details about where is incorrect than I can fix it
>
>             State like â??You are missing that it is entirely possible
>             that the job will complete while we are trying to kick it
>             out.â?? Is not a good reason to reject my approach, because
>             that is okay if the job just completed â?¦
>
>             BR Monk
>
>
>
>
>             *From:*Koenig, Christian
>             *Sent:* Tuesday, May 09, 2017 3:49 PM
>             *To:* Liu, Monk <Monk.Liu at amd.com>
>             <mailto:Monk.Liu at amd.com>; Christian König
>             <deathsimple at vodafone.de>
>             <mailto:deathsimple at vodafone.de>;
>             amd-gfx at lists.freedesktop.org
>             <mailto:amd-gfx at lists.freedesktop.org>
>             *Subject:* Re: [PATCH 4/4] drm/amdgpu/SRIOV:implement
>             guilty job TDR (V2)
>
>                 [ML] Really not necessary, we have spin_lock to
>                 protect the mirror-list, nothing will be messed up ...
>
>             You are missing that it is entirely possible that the job
>             will complete while we are trying to kick it out.
>
>
>
>
>
>                 [ML] why don't touch hardware fence at all ?  the
>                 original/bare-metal gpu reset also signal all ring's
>                 hardware fence first, I just follow the original logic ...
>                 Scheduler fence will be auto signaled after hw fence
>                 signaled, any problem with that ? what's the concern ?
>
>             The hardware runs async to the CPU which tries to reset
>             it, so we need to be careful in which order things are done.
>
>
>
>
>
>                 [ML] No I don't think so, the kickout must be prior to
>                 the hw_job_reset, otherwise the scheduler fence
>                 callback will be removed and you need manually install
>                 it later , which is not correct:
>                 For the guity job, we just kick it out before job
>                 reset, in job_reset we only reset other innocent jobs(
>                 and unbind the scheduler fence callback for them),
>                 after hw fence
>                 Forcely set to drv_seq, all hw fence are signaled
>                 (this is the way of original logic, I didn't change
>                 that).  When go to sched_recovery(), it will recover
>                 all innocent job and hook
>                 The scheduler fence with new hw fence.  That way only
>                 the guilty job is dropped forever.
>
>             Again same problem here.
>
>             To be absolutely sure that everything works as expected we
>             need to do it in the following order:
>
>             1. Stop the scheduler from feeding more jobs to the
>             hardware when a jobs completes.
>
>             2. Then call hw_job_reset to remove the connection between
>             job and hardware fence.
>
>             3. Test if job is now completed. It is entirely possible
>             that the job completed while we started the work to reset
>             the hardware.
>
>             Removing the connection between the hardware fence and the
>             job is the deadline, if the job completes after that it is
>             lost.
>
>             4. Check the karma and kick out the job if we found it guilty.
>
>             5. Get the whole stuff working again, e.g. reset the
>             hardware, restart the scheduler etc...
>
>             Regards,
>             Christian.
>
>             Am 09.05.2017 um 04:45 schrieb Liu, Monk:
>
>                 >
>                 > -     /* block scheduler */
>                 > -     for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>                 > -             ring = adev->rings[i];
>                 > +     /* we start from the ring trigger GPU hang */
>                 > +     j = job ? job->ring->idx : 0;
>                 > +
>                 > +     if (job)
>                 > +             if
>                 (amd_sched_invalidate_job(&job->base,
>                 amdgpu_job_hang_limit))
>                 > + amd_sched_job_kickout(&job->base);
>
>                 Well that looks like the wrong order to me. We should
>                 probably stop the scheduler before trying to mess
>                 anything with the job.
>
>                 [ML] Really not necessary, we have spin_lock to
>                 protect the mirror-list, nothing will be messed up ...
>
>
>                 >
>                 > +void
>                 amdgpu_fence_driver_force_completion_ring(struct
>                 amdgpu_ring
>                 > +*ring) {
>                 > +     if (ring)
>                 > +             amdgpu_fence_write(ring,
>                 ring->fence_drv.sync_seq); }
>                 > +
>
>                 The coding style is completely off.
>
>                 [ML] I don't know why at email side it looks wrong
>                 coding style, but I'm sure it is correct at my side,
>                 check here:
>                 void amdgpu_fence_driver_force_completion_ring(struct
>                 amdgpu_ring *ring)
>                 {
>                         if (ring)
>                                 amdgpu_fence_write(ring,
>                 ring->fence_drv.sync_seq);
>                 }
>
>                 Additional to that I don't think that this is a good
>                 idea. We should probably rather just signal all
>                 scheduler fences instead and don't touch the hardware
>                 fence at all.
>
>                 [ML] why don't touch hardware fence at all ?  the
>                 original/bare-metal gpu reset also signal all ring's
>                 hardware fence first, I just follow the original logic ...
>                 Scheduler fence will be auto signaled after hw fence
>                 signaled, any problem with that ? what's the concern ?
>
>
>                 > -     for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>                 > -             struct amdgpu_ring *ring = adev->rings[i];
>                 > +     for (i = j; i < j + AMDGPU_MAX_RINGS; ++i) {
>                 > +             ring = adev->rings[i % AMDGPU_MAX_RINGS];
>                 >                if (!ring || !ring->sched.thread)
>                 >                        continue;
>                 >
>                 > +             if (job && j != i) {
>                 > + kthread_unpark(ring->sched.thread);
>                 > +                     continue;
>                 > +             }
>                 > +
>
>                 Please split up that patch a bit further. E.g. first
>                 the handling to only hw_job_reset the ring in
>                 question, then the kickout handling.
>
>                 [ML] No I don't think so, the kickout must be prior to
>                 the hw_job_reset, otherwise the scheduler fence
>                 callback will be removed and you need manually install
>                 it later , which is not correct:
>                 For the guity job, we just kick it out before job
>                 reset, in job_reset we only reset other innocent jobs(
>                 and unbind the scheduler fence callback for them),
>                 after hw fence
>                 Forcely set to drv_seq, all hw fence are signaled
>                 (this is the way of original logic, I didn't change
>                 that).  When go to sched_recovery(), it will recover
>                 all innocent job and hook
>                 The scheduler fence with new hw fence.  That way only
>                 the guilty job is dropped forever.
>
>                 -----Original Message-----
>                 From: Christian König [mailto:deathsimple at vodafone.de]
>                 Sent: Monday, May 08, 2017 9:12 PM
>                 To: Liu, Monk <Monk.Liu at amd.com>
>                 <mailto:Monk.Liu at amd.com>;
>                 amd-gfx at lists.freedesktop.org
>                 <mailto:amd-gfx at lists.freedesktop.org>
>                 Cc: Koenig, Christian <Christian.Koenig at amd.com>
>                 <mailto:Christian.Koenig at amd.com>
>                 Subject: Re: [PATCH 4/4] drm/amdgpu/SRIOV:implement
>                 guilty job TDR (V2)
>
>                 Am 08.05.2017 um 09:01 schrieb Liu, Monk:
>                 > @Christian
>                 >
>                 > This one is changed to guilty job scheme accordingly
>                 with your
>                 > response
>                 >
>                 > BR Monk
>                 >
>                 > -----Original Message-----
>                 > From: Monk Liu [mailto:Monk.Liu at amd.com]
>                 > Sent: Monday, May 08, 2017 3:00 PM
>                 > To: amd-gfx at lists.freedesktop.org
>                 <mailto:amd-gfx at lists.freedesktop.org>
>                 > Cc: Liu, Monk <Monk.Liu at amd.com>
>                 <mailto:Monk.Liu at amd.com>
>                 > Subject: [PATCH] drm/amdgpu/SRIOV:implement guilty
>                 job TDR (V2)
>                 >
>                 > 1,TDR will kickout guilty job if it hang exceed the
>                 threshold of the given one from kernel paramter
>                 "job_hang_limit", that way a bad command stream will
>                 not infinitly cause GPU hang.
>                 >
>                 > by default this threshold is 1 so a job will be
>                 kicked out after it hang.
>                 >
>                 > 2,if a job timeout TDR routine will not reset all
>                 sched/ring, instead if will only reset on the givn one
>                 which is indicated by @job of amdgpu_sriov_gpu_reset,
>                 that way we don't need to reset and recover each
>                 sched/ring if we already know which job cause GPU hang.
>                 >
>                 > 3,unblock sriov_gpu_reset for AI family.
>                 > 4,don't init entity for KIQ
>                 >
>                 > TODO:
>                 > when a job is considered as guilty, we should mark
>                 some flag in its fence status flag, and let UMD side
>                 aware that this fence signaling is not due to job
>                 complete but job hang.
>                 >
>                 > Change-Id: I7b89c19a3de93249db570d0a80522176b1525a09
>                 > Signed-off-by: Monk Liu <Monk.Liu at amd.com>
>                 <mailto:Monk.Liu at amd.com>
>                 > ---
>                 > drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  1 +
>                 > drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c       |  4 +++
>                 > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    | 36
>                 ++++++++++++++++++++-------
>                 > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       |  4 +++
>                 > drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c     |  6 +++++
>                 > drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h      |  1 +
>                 > drivers/gpu/drm/amd/scheduler/gpu_scheduler.c | 11
>                 +++++++- drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>                 |  7 ++++++
>                 >   8 files changed, 60 insertions(+), 10 deletions(-)
>                 >
>                 > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>                 > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>                 > index 90a69bf..93bcea2 100644
>                 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>                 > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>                 > @@ -111,6 +111,7 @@ extern int
>                 amdgpu_prim_buf_per_se;  extern int
>                 > amdgpu_pos_buf_per_se;  extern int
>                 amdgpu_cntl_sb_buf_per_se;  extern
>                 > int amdgpu_param_buf_per_se;
>                 > +extern int amdgpu_job_hang_limit;
>                 >
>                 >   #define AMDGPU_DEFAULT_GTT_SIZE_MB 3072ULL /* 3GB
>                 by default */
>                 >   #define
>                 AMDGPU_WAIT_IDLE_TIMEOUT_IN_MS              3000
>                 > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>                 > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>                 > index b4bbbb3..23afc58 100644
>                 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>                 > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>                 > @@ -52,6 +52,10 @@ static int amdgpu_ctx_init(struct
>                 amdgpu_device *adev, struct amdgpu_ctx *ctx)
>                 >                struct amd_sched_rq *rq;
>                 >
>                 >                rq =
>                 &ring->sched.sched_rq[AMD_SCHED_PRIORITY_NORMAL];
>                 > +
>                 > +             if (ring == &adev->gfx.kiq.ring)
>                 > +                     continue;
>                 > +
>
>                 That looks like a bug fix and should probably go into
>                 a separate patch.
>
>                 >                r =
>                 amd_sched_entity_init(&ring->sched, &ctx->rings[i].entity,
>                 >                                          rq,
>                 amdgpu_sched_jobs);
>                 >                if (r)
>                 > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>                 > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>                 > index 0e5f314..f3990fe 100644
>                 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>                 > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>                 > @@ -2537,7 +2537,7 @@ static int
>                 amdgpu_recover_vram_from_shadow(struct amdgpu_device
>                 *adev,
>                 >    */
>                 >   int amdgpu_sriov_gpu_reset(struct amdgpu_device
>                 *adev, struct amdgpu_job *job)  {
>                 > -     int i, r = 0;
>                 > +     int i, j, r = 0;
>                 >        int resched;
>                 >        struct amdgpu_bo *bo, *tmp;
>                 >        struct amdgpu_ring *ring;
>                 > @@ -2550,19 +2550,30 @@ int
>                 amdgpu_sriov_gpu_reset(struct amdgpu_device *adev,
>                 struct amdgpu_job *job)
>                 >        /* block TTM */
>                 >        resched =
>                 ttm_bo_lock_delayed_workqueue(&adev->mman.bdev);
>                 >
>                 > -     /* block scheduler */
>                 > -     for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>                 > -             ring = adev->rings[i];
>                 > +     /* we start from the ring trigger GPU hang */
>                 > +     j = job ? job->ring->idx : 0;
>                 > +
>                 > +     if (job)
>                 > +             if
>                 (amd_sched_invalidate_job(&job->base,
>                 amdgpu_job_hang_limit))
>                 > + amd_sched_job_kickout(&job->base);
>
>                 Well that looks like the wrong order to me. We should
>                 probably stop the scheduler before trying to mess
>                 anything with the job.
>
>                 >
>                 > +     /* block scheduler */
>                 > +     for (i = j; i < j + AMDGPU_MAX_RINGS; ++i) {
>                 > +             ring = adev->rings[i % AMDGPU_MAX_RINGS];
>                 >                if (!ring || !ring->sched.thread)
>                 >                        continue;
>                 >
>                 > kthread_park(ring->sched.thread);
>                 > +
>                 > +             if (job && j != i)
>                 > +                     continue;
>                 > +
>                 > +             /* only do job_reset on the hang ring
>                 if @job not NULL */
>                 > amd_sched_hw_job_reset(&ring->sched);
>                 > -     }
>                 >
>                 > -     /* after all hw jobs are reset, hw fence is
>                 meaningless, so force_completion */
>                 > - amdgpu_fence_driver_force_completion(adev);
>                 > +             /* after all hw jobs are reset, hw
>                 fence is meaningless, so force_completion */
>                 > + amdgpu_fence_driver_force_completion_ring(ring);
>                 > +     }
>                 >
>                 >        /* request to take full control of GPU before
>                 re-initialization  */
>                 >        if (job)
>                 > @@ -2615,11 +2626,16 @@ int
>                 amdgpu_sriov_gpu_reset(struct amdgpu_device *adev,
>                 struct amdgpu_job *job)
>                 >        }
>                 >        fence_put(fence);
>                 >
>                 > -     for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>                 > -             struct amdgpu_ring *ring = adev->rings[i];
>                 > +     for (i = j; i < j + AMDGPU_MAX_RINGS; ++i) {
>                 > +             ring = adev->rings[i % AMDGPU_MAX_RINGS];
>                 >                if (!ring || !ring->sched.thread)
>                 >                        continue;
>                 >
>                 > +             if (job && j != i) {
>                 > + kthread_unpark(ring->sched.thread);
>                 > +                     continue;
>                 > +             }
>                 > +
>
>                 Please split up that patch a bit further. E.g. first
>                 the handling to only hw_job_reset the ring in
>                 question, then the kickout handling.
>
>                 > amd_sched_job_recovery(&ring->sched);
>                 > kthread_unpark(ring->sched.thread);
>                 >        }
>                 > @@ -2629,6 +2645,8 @@ int
>                 amdgpu_sriov_gpu_reset(struct amdgpu_device *adev,
>                 struct amdgpu_job *job)
>                 >        if (r) {
>                 >                /* bad news, how to tell it to
>                 userspace ? */
>                 >                dev_info(adev->dev, "GPU reset
>                 failed\n");
>                 > +     } else {
>                 > +             dev_info(adev->dev, "GPU reset
>                 successed!\n");
>                 >        }
>                 >
>                 >        adev->gfx.in_reset = false;
>                 > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>                 > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>                 > index 416908a..fd3691a8 100644
>                 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>                 > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>                 > @@ -112,6 +112,7 @@ int amdgpu_prim_buf_per_se = 0; 
>                 int
>                 > amdgpu_pos_buf_per_se = 0;  int
>                 amdgpu_cntl_sb_buf_per_se = 0;  int
>                 > amdgpu_param_buf_per_se = 0;
>                 > +int amdgpu_job_hang_limit = 0;
>                 >
>                 >   MODULE_PARM_DESC(vramlimit, "Restrict VRAM for
>                 testing, in
>                 > megabytes");  module_param_named(vramlimit,
>                 amdgpu_vram_limit, int,
>                 > 0600); @@ -237,6 +238,9 @@
>                 module_param_named(cntl_sb_buf_per_se,
>                 > amdgpu_cntl_sb_buf_per_se, int, 0444);
>                 > MODULE_PARM_DESC(param_buf_per_se, "the size of
>                 Off-Chip Pramater
>                 > Cache per Shader Engine (default depending on gfx)");
>                 > module_param_named(param_buf_per_se,
>                 amdgpu_param_buf_per_se, int,
>                 > 0444);
>                 >
>                 > +MODULE_PARM_DESC(job_hang_limit, "how much time
>                 allow a job hang and
>                 > +not drop it (default 0)");
>                 module_param_named(job_hang_limit,
>                 > +amdgpu_job_hang_limit, int ,0444);
>                 > +
>                 >
>                 >   static const struct pci_device_id pciidlist[] = { 
>                 #ifdef
>                 > CONFIG_DRM_AMDGPU_SI diff --git
>                 > a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>                 > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>                 > index d7523d1..8de3bd3 100644
>                 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>                 > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>                 > @@ -541,6 +541,12 @@ void
>                 amdgpu_fence_driver_force_completion(struct
>                 amdgpu_device *adev)
>                 >        }
>                 >   }
>                 >
>                 > +void
>                 amdgpu_fence_driver_force_completion_ring(struct
>                 amdgpu_ring
>                 > +*ring) {
>                 > +     if (ring)
>                 > +             amdgpu_fence_write(ring,
>                 ring->fence_drv.sync_seq); }
>                 > +
>
>                 The coding style is completely off.
>
>                 Additional to that I don't think that this is a good
>                 idea. We should probably rather just signal all
>                 scheduler fences instead and don't touch the hardware
>                 fence at all.
>
>                 >   /*
>                 >    * Common fence implementation
>                 >    */
>                 > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>                 > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>                 > index 981ef08..03e88c6 100644
>                 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>                 > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>                 > @@ -76,6 +76,7 @@ struct amdgpu_fence_driver {  int
>                 > amdgpu_fence_driver_init(struct amdgpu_device
>                 *adev);  void
>                 > amdgpu_fence_driver_fini(struct amdgpu_device
>                 *adev);  void
>                 > amdgpu_fence_driver_force_completion(struct
>                 amdgpu_device *adev);
>                 > +void
>                 amdgpu_fence_driver_force_completion_ring(struct
>                 amdgpu_ring
>                 > +*ring);
>                 >
>                 >   int amdgpu_fence_driver_init_ring(struct
>                 amdgpu_ring *ring,
>                 >                                  unsigned
>                 num_hw_submission);
>                 > diff --git
>                 a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>                 > b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>                 > index 6f4e31f..4e97e6d 100644
>                 > --- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>                 > +++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>                 > @@ -390,9 +390,18 @@ void
>                 amd_sched_hw_job_reset(struct amd_gpu_scheduler *sched)
>                 > &s_job->s_fence->cb)) {
>                 > fence_put(s_job->s_fence->parent);
>                 > s_job->s_fence->parent = NULL;
>                 > + atomic_dec(&sched->hw_rq_count);
>                 >                }
>                 >        }
>                 > -     atomic_set(&sched->hw_rq_count, 0);
>                 > + spin_unlock(&sched->job_list_lock);
>                 > +}
>                 > +
>                 > +void amd_sched_job_kickout(struct amd_sched_job
>                 *s_job) {
>                 > +     struct amd_gpu_scheduler *sched = s_job->sched;
>                 > +
>                 > + spin_lock(&sched->job_list_lock);
>                 > +     list_del_init(&s_job->node);
>                 > spin_unlock(&sched->job_list_lock);
>                 >   }
>                 >
>                 > diff --git
>                 a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>                 > b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>                 > index 8cb41d3..59694f3 100644
>                 > --- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>                 > +++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>                 > @@ -81,6 +81,7 @@ struct amd_sched_job {
>                 >        struct list_head                node;
>                 >        struct delayed_work work_tdr;
>                 >        uint64_t                        id;
>                 > +     atomic_t karma;
>                 >   };
>                 >
>                 >   extern const struct fence_ops
>                 amd_sched_fence_ops_scheduled; @@ -96,6 +97,11 @@
>                 static inline struct amd_sched_fence
>                 *to_amd_sched_fence(struct fence *f)
>                 >        return NULL;
>                 >   }
>                 >
>                 > +static inline bool amd_sched_invalidate_job(struct
>                 amd_sched_job
>                 > +*s_job, int threshold) {
>                 > +     return (s_job &&
>                 atomic_inc_return(&s_job->karma) > threshold); }
>                 > +
>
>                 Again coding style is completely off.
>
>                 Christian.
>
>                 >   /**
>                 >    * Define the backend operations called by the
>                 scheduler,
>                 >    * these functions should be implemented in driver
>                 side @@ -158,4 +164,5 @@ int amd_sched_job_init(struct
>                 amd_sched_job *job,
>                 >                       void *owner);
>                 >   void amd_sched_hw_job_reset(struct
>                 amd_gpu_scheduler *sched);  void
>                 > amd_sched_job_recovery(struct amd_gpu_scheduler *sched);
>                 > +void amd_sched_job_kickout(struct amd_sched_job
>                 *s_job);
>                 >   #endif
>                 > --
>                 > 2.7.4
>                 >
>                 > _______________________________________________
>                 > amd-gfx mailing list
>                 > amd-gfx at lists.freedesktop.org
>                 <mailto:amd-gfx at lists.freedesktop.org>
>                 > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
>
>
>
>
>
>         _______________________________________________
>
>         amd-gfx mailing list
>
>         amd-gfx at lists.freedesktop.org
>         <mailto:amd-gfx at lists.freedesktop.org>
>
>         https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20170510/512c3ddc/attachment-0001.html>


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux