Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
OK, I will update patches 1 and 2 and given your RBs push them since they fix some races. I will then update and test patch 3 on some basic scenarios and will send it for separate review where I might put a TODO comment in code with my objections regarding long jobs form our discussion so you can see and reply on that. Andrey On 01/24/2019 06:34 AM, Koenig, Christian wrote: > I see a few cleanups on Patch #3 which actually belong in patch #1: > >> +void drm_sched_stop(struct drm_gpu_scheduler *sched, struct >> drm_sched_job *bad) > The "bad" job parameter actually isn't used any more, isn't it? > >> +retry_wait: > Not used any more. > > But apart from that at least patch #1 and #2 look like they can have my > rb now. > > Patch #3 looks also like it should work after a bit of polishing. > > Thanks, > Christian. > > Am 18.01.19 um 20:15 schrieb Grodzovsky, Andrey: >> Attached series is the first 2 patches we already discussed about ring >> mirror list handling racing with all your comments fixed (still not >> committed). The third patch is a prototype based on the first 2 patches >> and on our discussion. >> >> Please take a look. >> >> Andrey >> >> >> On 01/18/2019 01:32 PM, Koenig, Christian wrote: >>> Am 18.01.19 um 18:34 schrieb Grodzovsky, Andrey: On 01/18/2019 12:10 PM, Koenig, Christian wrote: > Am 18.01.19 um 16:21 schrieb Grodzovsky, Andrey: >> On 01/18/2019 04:25 AM, Koenig, Christian wrote: >>> [SNIP] >>> Re-arming the timeout should probably have a much reduced value >>> when the job hasn't changed. E.g. something like a few ms. Now i got thinking about non hanged job in progress (job A) and let's say it's a long job , it just started executing but due to time out of another job (job B) on another (or this scheduler) it's parent cb got disconnected, we disarmed the tdr timer for the job's scheduler, meanwhile the timed out job did manage to complete before HW reset check and hence we skip HW reset, attach back the cb and rearm job's A tdr timer with a future value of few ms only - aren't we going to get false tdr triggered on job B now because we didn't let it enough time to run and complete ? I would prefer the other extreme of longer time for time out to trigger then false TDR. Optimally we would have per job timer and rearm to exactly the reminder of it's time out value - but we gave up on per job tdr work long ago. >>> Well we only re-arm the timeout with a shorter period if it already >>> triggered once. If we just suspend the timeout then we should still use >>> the longer period. >> Can you explain more on this ? I don't get it. > See drm_sched_job_timedout(), we re-arm the timeout at the end of the > procedure. > > We should change that and re-arm the timer with a much lower timeout if > the job is still not finished. > > Christian. I still don't see how this can fix the problem of of long job in progress triggering false tdr if no HW reset was done, but maybe I am missing other pieces you have in mind, I will finish the patch and send it and then we can be more specific based on the code. >>> Ok sounds good. We should probably discuss less on details and prototype >>> a bit more. >>> >>> Might be that I'm missing something here as well, so probably good to >>> have some code to talk about things more directly. >>> >>> Christian. >>> Andrey >> Andrey >> In general the more i think about it (correct me if I am wrong) I am less sure how much the optimization feature is useful - if job's time out did trigger what are the chances that the little more time we give it between beginning of tdr function and the time we do start the actual HW reset will be exactly what it needed to complete. Also, this is still not water proof as the job might complete and signal it's HW fence exactly after we checked for completion but before starting the HW reset code. >>> I don't see this as an optimization, but rather as mandatory for correct >>> operation. >>> >>> See without this we can run into issues because we execute jobs multiple >>> times. That can still happen with this clean handling, but it is much >>> more unlikely. >>> >>> Christian. >>> Andrey >> By unchanged you mean when we didn't resubmit the job because of the >> optimized non HW reset, right ? > Correct, yes. > About flushing tdr jobs in progress from .free_job cb - looks like drm_sched_job_finish->cancel_delayed_work_sync is not enough, we still need to take care of flushing all sced->work_tdr for a device and for all devices in hive for XGMI. What do you think ? >>> Why should that be
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
I see a few cleanups on Patch #3 which actually belong in patch #1: > +void drm_sched_stop(struct drm_gpu_scheduler *sched, struct > drm_sched_job *bad) The "bad" job parameter actually isn't used any more, isn't it? > +retry_wait: Not used any more. But apart from that at least patch #1 and #2 look like they can have my rb now. Patch #3 looks also like it should work after a bit of polishing. Thanks, Christian. Am 18.01.19 um 20:15 schrieb Grodzovsky, Andrey: > Attached series is the first 2 patches we already discussed about ring > mirror list handling racing with all your comments fixed (still not > committed). The third patch is a prototype based on the first 2 patches > and on our discussion. > > Please take a look. > > Andrey > > > On 01/18/2019 01:32 PM, Koenig, Christian wrote: >> Am 18.01.19 um 18:34 schrieb Grodzovsky, Andrey: >>> On 01/18/2019 12:10 PM, Koenig, Christian wrote: Am 18.01.19 um 16:21 schrieb Grodzovsky, Andrey: > On 01/18/2019 04:25 AM, Koenig, Christian wrote: >> [SNIP] >> Re-arming the timeout should probably have a much reduced value >> when the job hasn't changed. E.g. something like a few ms. >>> Now i got thinking about non hanged job in progress (job A) and let's >>> say it's a long job , it just started executing but due to time out of >>> another job (job B) on another (or this scheduler) it's parent cb got >>> disconnected, we disarmed the tdr timer for the job's scheduler, >>> meanwhile the timed out job did manage to complete before HW reset >>> check and hence we skip HW reset, attach back the cb and rearm job's A >>> tdr timer with a future value of few ms only - aren't we going to get >>> false tdr triggered on job B now because we didn't let it enough time >>> to run and complete ? I would prefer the other extreme of longer time >>> for time out to trigger then false TDR. Optimally we would have per >>> job timer and rearm to exactly the reminder of it's time out value - >>> but we gave up on per job tdr work long ago. >> Well we only re-arm the timeout with a shorter period if it already >> triggered once. If we just suspend the timeout then we should still use >> the longer period. > Can you explain more on this ? I don't get it. See drm_sched_job_timedout(), we re-arm the timeout at the end of the procedure. We should change that and re-arm the timer with a much lower timeout if the job is still not finished. Christian. >>> I still don't see how this can fix the problem of of long job in >>> progress triggering false tdr if no HW reset was done, but maybe I am >>> missing other pieces you have in mind, I will finish the patch and send >>> it and then we can be more specific based on the code. >> Ok sounds good. We should probably discuss less on details and prototype >> a bit more. >> >> Might be that I'm missing something here as well, so probably good to >> have some code to talk about things more directly. >> >> Christian. >> >>> Andrey >>> > Andrey > >>> In general the more i think about it (correct me if I am wrong) I am >>> less sure how much the optimization feature is useful - if job's time >>> out did trigger what are the chances that the little more time we give >>> it between beginning of tdr function and the time we do start the >>> actual HW reset will be exactly what it needed to complete. Also, this >>> is still not water proof as the job might complete and signal it's HW >>> fence exactly after we checked for completion but before starting the >>> HW reset code. >> I don't see this as an optimization, but rather as mandatory for correct >> operation. >> >> See without this we can run into issues because we execute jobs multiple >> times. That can still happen with this clean handling, but it is much >> more unlikely. >> >> Christian. >> >>> Andrey >>> > By unchanged you mean when we didn't resubmit the job because of the > optimized non HW reset, right ? Correct, yes. >>> About flushing tdr jobs in progress from .free_job cb - looks like >>> drm_sched_job_finish->cancel_delayed_work_sync is not enough, we >>> still need to take care of flushing all sced->work_tdr for a >>> device and for all devices in hive for XGMI. >>> What do you think ? >> Why should that be necessary? We only wait for the delayed work to >> make sure that the job is not destroyed while dealing with it. >> >> Christian. > But we might not be waiting for the correct sched->work_tdr, we do > the reset routine for all schedulers in a device accessing their > jobs too and not only for the scheduler to which the job belongs. > For XGMI not only that, we reset all the devices in the hive. That is harmless you only need
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
Attached series is the first 2 patches we already discussed about ring mirror list handling racing with all your comments fixed (still not committed). The third patch is a prototype based on the first 2 patches and on our discussion. Please take a look. Andrey On 01/18/2019 01:32 PM, Koenig, Christian wrote: > Am 18.01.19 um 18:34 schrieb Grodzovsky, Andrey: >> On 01/18/2019 12:10 PM, Koenig, Christian wrote: >>> Am 18.01.19 um 16:21 schrieb Grodzovsky, Andrey: On 01/18/2019 04:25 AM, Koenig, Christian wrote: > [SNIP] > Re-arming the timeout should probably have a much reduced value > when the job hasn't changed. E.g. something like a few ms. >> Now i got thinking about non hanged job in progress (job A) and let's >> say it's a long job , it just started executing but due to time out of >> another job (job B) on another (or this scheduler) it's parent cb got >> disconnected, we disarmed the tdr timer for the job's scheduler, >> meanwhile the timed out job did manage to complete before HW reset >> check and hence we skip HW reset, attach back the cb and rearm job's A >> tdr timer with a future value of few ms only - aren't we going to get >> false tdr triggered on job B now because we didn't let it enough time >> to run and complete ? I would prefer the other extreme of longer time >> for time out to trigger then false TDR. Optimally we would have per >> job timer and rearm to exactly the reminder of it's time out value - >> but we gave up on per job tdr work long ago. > Well we only re-arm the timeout with a shorter period if it already > triggered once. If we just suspend the timeout then we should still use > the longer period. Can you explain more on this ? I don't get it. >>> See drm_sched_job_timedout(), we re-arm the timeout at the end of the >>> procedure. >>> >>> We should change that and re-arm the timer with a much lower timeout if >>> the job is still not finished. >>> >>> Christian. >> I still don't see how this can fix the problem of of long job in >> progress triggering false tdr if no HW reset was done, but maybe I am >> missing other pieces you have in mind, I will finish the patch and send >> it and then we can be more specific based on the code. > Ok sounds good. We should probably discuss less on details and prototype > a bit more. > > Might be that I'm missing something here as well, so probably good to > have some code to talk about things more directly. > > Christian. > >> Andrey >> Andrey >> In general the more i think about it (correct me if I am wrong) I am >> less sure how much the optimization feature is useful - if job's time >> out did trigger what are the chances that the little more time we give >> it between beginning of tdr function and the time we do start the >> actual HW reset will be exactly what it needed to complete. Also, this >> is still not water proof as the job might complete and signal it's HW >> fence exactly after we checked for completion but before starting the >> HW reset code. > I don't see this as an optimization, but rather as mandatory for correct > operation. > > See without this we can run into issues because we execute jobs multiple > times. That can still happen with this clean handling, but it is much > more unlikely. > > Christian. > >> Andrey >> By unchanged you mean when we didn't resubmit the job because of the optimized non HW reset, right ? >>> Correct, yes. >>> >> About flushing tdr jobs in progress from .free_job cb - looks like >> drm_sched_job_finish->cancel_delayed_work_sync is not enough, we >> still need to take care of flushing all sced->work_tdr for a >> device and for all devices in hive for XGMI. >> What do you think ? > Why should that be necessary? We only wait for the delayed work to > make sure that the job is not destroyed while dealing with it. > > Christian. But we might not be waiting for the correct sched->work_tdr, we do the reset routine for all schedulers in a device accessing their jobs too and not only for the scheduler to which the job belongs. For XGMI not only that, we reset all the devices in the hive. >>> That is harmless you only need to wait for the work_tdr of the >>> current scheduler, not for all of them. >>> I was thinking, amdgpu driver is not even interested in allowing multiple sced->tdr to execute together - we have to serialize all of them anyway with the trylock mutex (even without XGMI), v3d in v3d_job_timedout seems also to reset all of his schedulers from the tdr work. Would it make sense to provide the sched->work_td as init parameter to scheduler (same one for all schedulers) so we can enforce serialization
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
Am 18.01.19 um 18:34 schrieb Grodzovsky, Andrey: > > On 01/18/2019 12:10 PM, Koenig, Christian wrote: >> Am 18.01.19 um 16:21 schrieb Grodzovsky, Andrey: >>> On 01/18/2019 04:25 AM, Koenig, Christian wrote: [SNIP] Re-arming the timeout should probably have a much reduced value when the job hasn't changed. E.g. something like a few ms. > Now i got thinking about non hanged job in progress (job A) and let's > say it's a long job , it just started executing but due to time out of > another job (job B) on another (or this scheduler) it's parent cb got > disconnected, we disarmed the tdr timer for the job's scheduler, > meanwhile the timed out job did manage to complete before HW reset > check and hence we skip HW reset, attach back the cb and rearm job's A > tdr timer with a future value of few ms only - aren't we going to get > false tdr triggered on job B now because we didn't let it enough time > to run and complete ? I would prefer the other extreme of longer time > for time out to trigger then false TDR. Optimally we would have per > job timer and rearm to exactly the reminder of it's time out value - > but we gave up on per job tdr work long ago. Well we only re-arm the timeout with a shorter period if it already triggered once. If we just suspend the timeout then we should still use the longer period. >>> Can you explain more on this ? I don't get it. >> See drm_sched_job_timedout(), we re-arm the timeout at the end of the >> procedure. >> >> We should change that and re-arm the timer with a much lower timeout if >> the job is still not finished. >> >> Christian. > I still don't see how this can fix the problem of of long job in > progress triggering false tdr if no HW reset was done, but maybe I am > missing other pieces you have in mind, I will finish the patch and send > it and then we can be more specific based on the code. Ok sounds good. We should probably discuss less on details and prototype a bit more. Might be that I'm missing something here as well, so probably good to have some code to talk about things more directly. Christian. > > Andrey > >>> Andrey >>> > In general the more i think about it (correct me if I am wrong) I am > less sure how much the optimization feature is useful - if job's time > out did trigger what are the chances that the little more time we give > it between beginning of tdr function and the time we do start the > actual HW reset will be exactly what it needed to complete. Also, this > is still not water proof as the job might complete and signal it's HW > fence exactly after we checked for completion but before starting the > HW reset code. I don't see this as an optimization, but rather as mandatory for correct operation. See without this we can run into issues because we execute jobs multiple times. That can still happen with this clean handling, but it is much more unlikely. Christian. > Andrey > >>> By unchanged you mean when we didn't resubmit the job because of the >>> optimized non HW reset, right ? >> Correct, yes. >> > About flushing tdr jobs in progress from .free_job cb - looks like > drm_sched_job_finish->cancel_delayed_work_sync is not enough, we > still need to take care of flushing all sced->work_tdr for a > device and for all devices in hive for XGMI. > What do you think ? Why should that be necessary? We only wait for the delayed work to make sure that the job is not destroyed while dealing with it. Christian. >>> But we might not be waiting for the correct sched->work_tdr, we do >>> the reset routine for all schedulers in a device accessing their >>> jobs too and not only for the scheduler to which the job belongs. >>> For XGMI not only that, we reset all the devices in the hive. >> That is harmless you only need to wait for the work_tdr of the >> current scheduler, not for all of them. >> >>> I was thinking, amdgpu driver is not even interested in allowing >>> multiple sced->tdr to execute together - we have to serialize all of >>> them anyway with the trylock mutex (even without XGMI), v3d in >>> v3d_job_timedout seems also to reset all of his schedulers from the >>> tdr work. Would it make sense to provide the sched->work_td as init >>> parameter to scheduler (same one for all schedulers) so we can >>> enforce serialization by disallowing more then 1 tdr work to execute >>> in the same time ? Other drivers interested to do in parallel can >>> provide unique sched->work_tdr per scheduler. This does imply >>> drm_sched_job_timedout has to removed and delegated to specific >>> driver implementation as probably other code dealing with >>> sched->work_tdr... Maybe even move tdr handling to the driver all
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
On 01/18/2019 12:10 PM, Koenig, Christian wrote: > Am 18.01.19 um 16:21 schrieb Grodzovsky, Andrey: >> On 01/18/2019 04:25 AM, Koenig, Christian wrote: >>> [SNIP] >>> Re-arming the timeout should probably have a much reduced value >>> when the job hasn't changed. E.g. something like a few ms. Now i got thinking about non hanged job in progress (job A) and let's say it's a long job , it just started executing but due to time out of another job (job B) on another (or this scheduler) it's parent cb got disconnected, we disarmed the tdr timer for the job's scheduler, meanwhile the timed out job did manage to complete before HW reset check and hence we skip HW reset, attach back the cb and rearm job's A tdr timer with a future value of few ms only - aren't we going to get false tdr triggered on job B now because we didn't let it enough time to run and complete ? I would prefer the other extreme of longer time for time out to trigger then false TDR. Optimally we would have per job timer and rearm to exactly the reminder of it's time out value - but we gave up on per job tdr work long ago. >>> Well we only re-arm the timeout with a shorter period if it already >>> triggered once. If we just suspend the timeout then we should still use >>> the longer period. >> Can you explain more on this ? I don't get it. > See drm_sched_job_timedout(), we re-arm the timeout at the end of the > procedure. > > We should change that and re-arm the timer with a much lower timeout if > the job is still not finished. > > Christian. I still don't see how this can fix the problem of of long job in progress triggering false tdr if no HW reset was done, but maybe I am missing other pieces you have in mind, I will finish the patch and send it and then we can be more specific based on the code. Andrey > >> Andrey >> In general the more i think about it (correct me if I am wrong) I am less sure how much the optimization feature is useful - if job's time out did trigger what are the chances that the little more time we give it between beginning of tdr function and the time we do start the actual HW reset will be exactly what it needed to complete. Also, this is still not water proof as the job might complete and signal it's HW fence exactly after we checked for completion but before starting the HW reset code. >>> I don't see this as an optimization, but rather as mandatory for correct >>> operation. >>> >>> See without this we can run into issues because we execute jobs multiple >>> times. That can still happen with this clean handling, but it is much >>> more unlikely. >>> >>> Christian. >>> Andrey >> By unchanged you mean when we didn't resubmit the job because of the >> optimized non HW reset, right ? > Correct, yes. > About flushing tdr jobs in progress from .free_job cb - looks like drm_sched_job_finish->cancel_delayed_work_sync is not enough, we still need to take care of flushing all sced->work_tdr for a device and for all devices in hive for XGMI. What do you think ? >>> Why should that be necessary? We only wait for the delayed work to >>> make sure that the job is not destroyed while dealing with it. >>> >>> Christian. >> But we might not be waiting for the correct sched->work_tdr, we do >> the reset routine for all schedulers in a device accessing their >> jobs too and not only for the scheduler to which the job belongs. >> For XGMI not only that, we reset all the devices in the hive. > That is harmless you only need to wait for the work_tdr of the > current scheduler, not for all of them. > >> I was thinking, amdgpu driver is not even interested in allowing >> multiple sced->tdr to execute together - we have to serialize all of >> them anyway with the trylock mutex (even without XGMI), v3d in >> v3d_job_timedout seems also to reset all of his schedulers from the >> tdr work. Would it make sense to provide the sched->work_td as init >> parameter to scheduler (same one for all schedulers) so we can >> enforce serialization by disallowing more then 1 tdr work to execute >> in the same time ? Other drivers interested to do in parallel can >> provide unique sched->work_tdr per scheduler. This does imply >> drm_sched_job_timedout has to removed and delegated to specific >> driver implementation as probably other code dealing with >> sched->work_tdr... Maybe even move tdr handling to the driver all >> together ? > Yeah, I was thinking something similar. The problem with this > approach is that a delayed work item can have only one delay, but for > multiple engines we need multiple delays. > > What we could do is to make it a timer instead and raise the work > item from the device specific callback. > > But that doesn't
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
Am 18.01.19 um 16:21 schrieb Grodzovsky, Andrey: > > On 01/18/2019 04:25 AM, Koenig, Christian wrote: >> [SNIP] >> Re-arming the timeout should probably have a much reduced value >> when the job hasn't changed. E.g. something like a few ms. >>> Now i got thinking about non hanged job in progress (job A) and let's >>> say it's a long job , it just started executing but due to time out of >>> another job (job B) on another (or this scheduler) it's parent cb got >>> disconnected, we disarmed the tdr timer for the job's scheduler, >>> meanwhile the timed out job did manage to complete before HW reset >>> check and hence we skip HW reset, attach back the cb and rearm job's A >>> tdr timer with a future value of few ms only - aren't we going to get >>> false tdr triggered on job B now because we didn't let it enough time >>> to run and complete ? I would prefer the other extreme of longer time >>> for time out to trigger then false TDR. Optimally we would have per >>> job timer and rearm to exactly the reminder of it's time out value - >>> but we gave up on per job tdr work long ago. >> Well we only re-arm the timeout with a shorter period if it already >> triggered once. If we just suspend the timeout then we should still use >> the longer period. > Can you explain more on this ? I don't get it. See drm_sched_job_timedout(), we re-arm the timeout at the end of the procedure. We should change that and re-arm the timer with a much lower timeout if the job is still not finished. Christian. > > Andrey > >>> In general the more i think about it (correct me if I am wrong) I am >>> less sure how much the optimization feature is useful - if job's time >>> out did trigger what are the chances that the little more time we give >>> it between beginning of tdr function and the time we do start the >>> actual HW reset will be exactly what it needed to complete. Also, this >>> is still not water proof as the job might complete and signal it's HW >>> fence exactly after we checked for completion but before starting the >>> HW reset code. >> I don't see this as an optimization, but rather as mandatory for correct >> operation. >> >> See without this we can run into issues because we execute jobs multiple >> times. That can still happen with this clean handling, but it is much >> more unlikely. >> >> Christian. >> >>> Andrey >>> > By unchanged you mean when we didn't resubmit the job because of the > optimized non HW reset, right ? Correct, yes. >>> About flushing tdr jobs in progress from .free_job cb - looks like >>> drm_sched_job_finish->cancel_delayed_work_sync is not enough, we >>> still need to take care of flushing all sced->work_tdr for a >>> device and for all devices in hive for XGMI. >>> What do you think ? >> Why should that be necessary? We only wait for the delayed work to >> make sure that the job is not destroyed while dealing with it. >> >> Christian. > But we might not be waiting for the correct sched->work_tdr, we do > the reset routine for all schedulers in a device accessing their > jobs too and not only for the scheduler to which the job belongs. > For XGMI not only that, we reset all the devices in the hive. That is harmless you only need to wait for the work_tdr of the current scheduler, not for all of them. > I was thinking, amdgpu driver is not even interested in allowing > multiple sced->tdr to execute together - we have to serialize all of > them anyway with the trylock mutex (even without XGMI), v3d in > v3d_job_timedout seems also to reset all of his schedulers from the > tdr work. Would it make sense to provide the sched->work_td as init > parameter to scheduler (same one for all schedulers) so we can > enforce serialization by disallowing more then 1 tdr work to execute > in the same time ? Other drivers interested to do in parallel can > provide unique sched->work_tdr per scheduler. This does imply > drm_sched_job_timedout has to removed and delegated to specific > driver implementation as probably other code dealing with > sched->work_tdr... Maybe even move tdr handling to the driver all > together ? Yeah, I was thinking something similar. The problem with this approach is that a delayed work item can have only one delay, but for multiple engines we need multiple delays. What we could do is to make it a timer instead and raise the work item from the device specific callback. But that doesn't really saves us the stop all schedulers trouble, so it doesn't buy us much in the end if I see this correctly. Christian. >> ___ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
On 01/18/2019 04:25 AM, Koenig, Christian wrote: > [SNIP] > Re-arming the timeout should probably have a much reduced value > when the job hasn't changed. E.g. something like a few ms. >> Now i got thinking about non hanged job in progress (job A) and let's >> say it's a long job , it just started executing but due to time out of >> another job (job B) on another (or this scheduler) it's parent cb got >> disconnected, we disarmed the tdr timer for the job's scheduler, >> meanwhile the timed out job did manage to complete before HW reset >> check and hence we skip HW reset, attach back the cb and rearm job's A >> tdr timer with a future value of few ms only - aren't we going to get >> false tdr triggered on job B now because we didn't let it enough time >> to run and complete ? I would prefer the other extreme of longer time >> for time out to trigger then false TDR. Optimally we would have per >> job timer and rearm to exactly the reminder of it's time out value - >> but we gave up on per job tdr work long ago. > Well we only re-arm the timeout with a shorter period if it already > triggered once. If we just suspend the timeout then we should still use > the longer period. Can you explain more on this ? I don't get it. Andrey > >> In general the more i think about it (correct me if I am wrong) I am >> less sure how much the optimization feature is useful - if job's time >> out did trigger what are the chances that the little more time we give >> it between beginning of tdr function and the time we do start the >> actual HW reset will be exactly what it needed to complete. Also, this >> is still not water proof as the job might complete and signal it's HW >> fence exactly after we checked for completion but before starting the >> HW reset code. > I don't see this as an optimization, but rather as mandatory for correct > operation. > > See without this we can run into issues because we execute jobs multiple > times. That can still happen with this clean handling, but it is much > more unlikely. > > Christian. > >> Andrey >> By unchanged you mean when we didn't resubmit the job because of the optimized non HW reset, right ? >>> Correct, yes. >>> >> About flushing tdr jobs in progress from .free_job cb - looks like >> drm_sched_job_finish->cancel_delayed_work_sync is not enough, we >> still need to take care of flushing all sced->work_tdr for a >> device and for all devices in hive for XGMI. >> What do you think ? > Why should that be necessary? We only wait for the delayed work to > make sure that the job is not destroyed while dealing with it. > > Christian. But we might not be waiting for the correct sched->work_tdr, we do the reset routine for all schedulers in a device accessing their jobs too and not only for the scheduler to which the job belongs. For XGMI not only that, we reset all the devices in the hive. >>> That is harmless you only need to wait for the work_tdr of the >>> current scheduler, not for all of them. >>> I was thinking, amdgpu driver is not even interested in allowing multiple sced->tdr to execute together - we have to serialize all of them anyway with the trylock mutex (even without XGMI), v3d in v3d_job_timedout seems also to reset all of his schedulers from the tdr work. Would it make sense to provide the sched->work_td as init parameter to scheduler (same one for all schedulers) so we can enforce serialization by disallowing more then 1 tdr work to execute in the same time ? Other drivers interested to do in parallel can provide unique sched->work_tdr per scheduler. This does imply drm_sched_job_timedout has to removed and delegated to specific driver implementation as probably other code dealing with sched->work_tdr... Maybe even move tdr handling to the driver all together ? >>> Yeah, I was thinking something similar. The problem with this >>> approach is that a delayed work item can have only one delay, but for >>> multiple engines we need multiple delays. >>> >>> What we could do is to make it a timer instead and raise the work >>> item from the device specific callback. >>> >>> But that doesn't really saves us the stop all schedulers trouble, so >>> it doesn't buy us much in the end if I see this correctly. >>> >>> Christian. > ___ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
[SNIP] Re-arming the timeout should probably have a much reduced value when the job hasn't changed. E.g. something like a few ms. > > Now i got thinking about non hanged job in progress (job A) and let's > say it's a long job , it just started executing but due to time out of > another job (job B) on another (or this scheduler) it's parent cb got > disconnected, we disarmed the tdr timer for the job's scheduler, > meanwhile the timed out job did manage to complete before HW reset > check and hence we skip HW reset, attach back the cb and rearm job's A > tdr timer with a future value of few ms only - aren't we going to get > false tdr triggered on job B now because we didn't let it enough time > to run and complete ? I would prefer the other extreme of longer time > for time out to trigger then false TDR. Optimally we would have per > job timer and rearm to exactly the reminder of it's time out value - > but we gave up on per job tdr work long ago. Well we only re-arm the timeout with a shorter period if it already triggered once. If we just suspend the timeout then we should still use the longer period. > In general the more i think about it (correct me if I am wrong) I am > less sure how much the optimization feature is useful - if job's time > out did trigger what are the chances that the little more time we give > it between beginning of tdr function and the time we do start the > actual HW reset will be exactly what it needed to complete. Also, this > is still not water proof as the job might complete and signal it's HW > fence exactly after we checked for completion but before starting the > HW reset code. I don't see this as an optimization, but rather as mandatory for correct operation. See without this we can run into issues because we execute jobs multiple times. That can still happen with this clean handling, but it is much more unlikely. Christian. > > Andrey > >>> >>> By unchanged you mean when we didn't resubmit the job because of the >>> optimized non HW reset, right ? >> >> Correct, yes. >> >>> > About flushing tdr jobs in progress from .free_job cb - looks like > drm_sched_job_finish->cancel_delayed_work_sync is not enough, we > still need to take care of flushing all sced->work_tdr for a > device and for all devices in hive for XGMI. > What do you think ? Why should that be necessary? We only wait for the delayed work to make sure that the job is not destroyed while dealing with it. Christian. >>> >>> But we might not be waiting for the correct sched->work_tdr, we do >>> the reset routine for all schedulers in a device accessing their >>> jobs too and not only for the scheduler to which the job belongs. >>> For XGMI not only that, we reset all the devices in the hive. >> >> That is harmless you only need to wait for the work_tdr of the >> current scheduler, not for all of them. >> >>> >>> I was thinking, amdgpu driver is not even interested in allowing >>> multiple sced->tdr to execute together - we have to serialize all of >>> them anyway with the trylock mutex (even without XGMI), v3d in >>> v3d_job_timedout seems also to reset all of his schedulers from the >>> tdr work. Would it make sense to provide the sched->work_td as init >>> parameter to scheduler (same one for all schedulers) so we can >>> enforce serialization by disallowing more then 1 tdr work to execute >>> in the same time ? Other drivers interested to do in parallel can >>> provide unique sched->work_tdr per scheduler. This does imply >>> drm_sched_job_timedout has to removed and delegated to specific >>> driver implementation as probably other code dealing with >>> sched->work_tdr... Maybe even move tdr handling to the driver all >>> together ? >> >> Yeah, I was thinking something similar. The problem with this >> approach is that a delayed work item can have only one delay, but for >> multiple engines we need multiple delays. >> >> What we could do is to make it a timer instead and raise the work >> item from the device specific callback. >> >> But that doesn't really saves us the stop all schedulers trouble, so >> it doesn't buy us much in the end if I see this correctly. >> >> Christian. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
On 01/17/2019 10:29 AM, Koenig, Christian wrote: Am 17.01.19 um 16:22 schrieb Grodzovsky, Andrey: On 01/17/2019 02:45 AM, Christian König wrote: Am 16.01.19 um 18:17 schrieb Grodzovsky, Andrey: On 01/16/2019 11:02 AM, Koenig, Christian wrote: Am 16.01.19 um 16:45 schrieb Grodzovsky, Andrey: On 01/16/2019 02:46 AM, Christian König wrote: Am 15.01.19 um 23:01 schrieb Grodzovsky, Andrey: On 01/11/2019 05:03 PM, Andrey Grodzovsky wrote: On 01/11/2019 02:11 PM, Koenig, Christian wrote: Am 11.01.19 um 16:37 schrieb Grodzovsky, Andrey: On 01/11/2019 04:42 AM, Koenig, Christian wrote: Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey: [SNIP] But we will not be adding the cb back in drm_sched_stop anymore, now we are only going to add back the cb in drm_sched_startr after rerunning those jobs in drm_sched_resubmit_jobs and assign them a new parent there anyway. Yeah, but when we find that we don't need to reset anything anymore then adding the callbacks again won't be possible any more. Christian. I am not sure I understand it, can u point me to example of how this will happen ? I am attaching my latest patches with waiting only for the last job's fence here just so we are on same page regarding the code. Well the whole idea is to prepare all schedulers, then check once more if the offending job hasn't completed in the meantime. If the job completed we need to be able to rollback everything and continue as if nothing had happened. Christian. Oh, but this piece of functionality - skipping HW ASIC reset in case the guilty job done is totally missing form this patch series and so needs to be added. So what you say actually is that for the case were we skip HW asic reset because the guilty job did complete we also need to skip resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the old parent fence pointer for reuse ? If so I would like to add all this functionality as a third patch since the first 2 patches are more about resolving race condition with jobs in flight while doing reset - what do you think ? Yeah, sounds perfectly fine to me. Christian. I realized there is another complication now for XGMI hive use case, we currently skip gpu recover for adev in case another gpu recover for different adev in same hive is running, under the assumption that we are going to reset all devices in hive anyway because that should cover our own dev too. But if we chose to skip now HW asic reset if our guilty job did finish we will aslo not HW reset any other devices in the hive even if one of them might actually had a bad job, wanted to do gpu recover but skipped it because our recover was in progress in that time. My general idea on that is to keep a list of guilty jobs per hive, when you start gpu recover you first add you guilty job to the hive and trylock hive->reset_lock. Any owner of hive->reset_lock (gpu recovery in progress) once he finished his recovery and released hive->reset_lock should go over hive->bad_jobs_list and if at least one of them is still not signaled (not done) trigger another gpu recovery and so on. If you do manage to trylock you also go over the list, clean it and perform recovery. This list probably needs to be protected with per hive lock. I also think we can for now at least finish reviewing the first 2 patches and submit them since as I said before they are not dealing with this topic and fixing existing race conditions. If you are OK with that I can send for review the last iteration of the first 2 patches where I wait for the last fence in ring mirror list. Andrey I implemented HW reset avoidance including XGMI use case according to the plan i specified. Patch is attached but I can't test it yet due to XGMI regression in PSP which is supposed to be fixed soon. Please take a look. Looks a bit too complicated on first glance. In general we should probably get away from handling a hive in any special way. Yes, I guess i can do it the same way as the generic handling in amdgpu_device_gpu_recover - there is a list of devices to process which is of size 1 for non xgmi use case or more then 1 for XGMI. Multiple timeout jobs in a hive are identical to multiple timeout jobs on different engines on a single device. How about the following handling: 1. Add the timeout job to a list. 2. Try to grab a lock to handle the reset, if that doesn't work because there is already a reset underway return immediately. 3. Stop all schedulers on all affected devices including stopping the timeouts and detaching all callbacks. 4. Double check the list of timed out jobs, if all hw fences of all jobs are completed now we actually don't need to do anything. What if all the jobs on the timed out list did complete but other job (or jobs) for which we removed the time out timer became hanged ? Wouldn't we miss a required reset in this case and wouldn't even have any indication of their hang ? If the timeout triggers before we disable it we will have the
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
On 01/17/2019 10:29 AM, Koenig, Christian wrote: Am 17.01.19 um 16:22 schrieb Grodzovsky, Andrey: On 01/17/2019 02:45 AM, Christian König wrote: Am 16.01.19 um 18:17 schrieb Grodzovsky, Andrey: On 01/16/2019 11:02 AM, Koenig, Christian wrote: Am 16.01.19 um 16:45 schrieb Grodzovsky, Andrey: On 01/16/2019 02:46 AM, Christian König wrote: Am 15.01.19 um 23:01 schrieb Grodzovsky, Andrey: On 01/11/2019 05:03 PM, Andrey Grodzovsky wrote: On 01/11/2019 02:11 PM, Koenig, Christian wrote: Am 11.01.19 um 16:37 schrieb Grodzovsky, Andrey: On 01/11/2019 04:42 AM, Koenig, Christian wrote: Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey: [SNIP] But we will not be adding the cb back in drm_sched_stop anymore, now we are only going to add back the cb in drm_sched_startr after rerunning those jobs in drm_sched_resubmit_jobs and assign them a new parent there anyway. Yeah, but when we find that we don't need to reset anything anymore then adding the callbacks again won't be possible any more. Christian. I am not sure I understand it, can u point me to example of how this will happen ? I am attaching my latest patches with waiting only for the last job's fence here just so we are on same page regarding the code. Well the whole idea is to prepare all schedulers, then check once more if the offending job hasn't completed in the meantime. If the job completed we need to be able to rollback everything and continue as if nothing had happened. Christian. Oh, but this piece of functionality - skipping HW ASIC reset in case the guilty job done is totally missing form this patch series and so needs to be added. So what you say actually is that for the case were we skip HW asic reset because the guilty job did complete we also need to skip resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the old parent fence pointer for reuse ? If so I would like to add all this functionality as a third patch since the first 2 patches are more about resolving race condition with jobs in flight while doing reset - what do you think ? Yeah, sounds perfectly fine to me. Christian. I realized there is another complication now for XGMI hive use case, we currently skip gpu recover for adev in case another gpu recover for different adev in same hive is running, under the assumption that we are going to reset all devices in hive anyway because that should cover our own dev too. But if we chose to skip now HW asic reset if our guilty job did finish we will aslo not HW reset any other devices in the hive even if one of them might actually had a bad job, wanted to do gpu recover but skipped it because our recover was in progress in that time. My general idea on that is to keep a list of guilty jobs per hive, when you start gpu recover you first add you guilty job to the hive and trylock hive->reset_lock. Any owner of hive->reset_lock (gpu recovery in progress) once he finished his recovery and released hive->reset_lock should go over hive->bad_jobs_list and if at least one of them is still not signaled (not done) trigger another gpu recovery and so on. If you do manage to trylock you also go over the list, clean it and perform recovery. This list probably needs to be protected with per hive lock. I also think we can for now at least finish reviewing the first 2 patches and submit them since as I said before they are not dealing with this topic and fixing existing race conditions. If you are OK with that I can send for review the last iteration of the first 2 patches where I wait for the last fence in ring mirror list. Andrey I implemented HW reset avoidance including XGMI use case according to the plan i specified. Patch is attached but I can't test it yet due to XGMI regression in PSP which is supposed to be fixed soon. Please take a look. Looks a bit too complicated on first glance. In general we should probably get away from handling a hive in any special way. Yes, I guess i can do it the same way as the generic handling in amdgpu_device_gpu_recover - there is a list of devices to process which is of size 1 for non xgmi use case or more then 1 for XGMI. Multiple timeout jobs in a hive are identical to multiple timeout jobs on different engines on a single device. How about the following handling: 1. Add the timeout job to a list. 2. Try to grab a lock to handle the reset, if that doesn't work because there is already a reset underway return immediately. 3. Stop all schedulers on all affected devices including stopping the timeouts and detaching all callbacks. 4. Double check the list of timed out jobs, if all hw fences of all jobs are completed now we actually don't need to do anything. What if all the jobs on the timed out list did complete but other job (or jobs) for which we removed the time out timer became hanged ? Wouldn't we miss a required reset in this case and wouldn't even have any indication of their hang ? If the timeout triggers before we disable it we will have the
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
Am 17.01.19 um 16:22 schrieb Grodzovsky, Andrey: On 01/17/2019 02:45 AM, Christian König wrote: Am 16.01.19 um 18:17 schrieb Grodzovsky, Andrey: On 01/16/2019 11:02 AM, Koenig, Christian wrote: Am 16.01.19 um 16:45 schrieb Grodzovsky, Andrey: On 01/16/2019 02:46 AM, Christian König wrote: Am 15.01.19 um 23:01 schrieb Grodzovsky, Andrey: On 01/11/2019 05:03 PM, Andrey Grodzovsky wrote: On 01/11/2019 02:11 PM, Koenig, Christian wrote: Am 11.01.19 um 16:37 schrieb Grodzovsky, Andrey: On 01/11/2019 04:42 AM, Koenig, Christian wrote: Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey: [SNIP] But we will not be adding the cb back in drm_sched_stop anymore, now we are only going to add back the cb in drm_sched_startr after rerunning those jobs in drm_sched_resubmit_jobs and assign them a new parent there anyway. Yeah, but when we find that we don't need to reset anything anymore then adding the callbacks again won't be possible any more. Christian. I am not sure I understand it, can u point me to example of how this will happen ? I am attaching my latest patches with waiting only for the last job's fence here just so we are on same page regarding the code. Well the whole idea is to prepare all schedulers, then check once more if the offending job hasn't completed in the meantime. If the job completed we need to be able to rollback everything and continue as if nothing had happened. Christian. Oh, but this piece of functionality - skipping HW ASIC reset in case the guilty job done is totally missing form this patch series and so needs to be added. So what you say actually is that for the case were we skip HW asic reset because the guilty job did complete we also need to skip resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the old parent fence pointer for reuse ? If so I would like to add all this functionality as a third patch since the first 2 patches are more about resolving race condition with jobs in flight while doing reset - what do you think ? Yeah, sounds perfectly fine to me. Christian. I realized there is another complication now for XGMI hive use case, we currently skip gpu recover for adev in case another gpu recover for different adev in same hive is running, under the assumption that we are going to reset all devices in hive anyway because that should cover our own dev too. But if we chose to skip now HW asic reset if our guilty job did finish we will aslo not HW reset any other devices in the hive even if one of them might actually had a bad job, wanted to do gpu recover but skipped it because our recover was in progress in that time. My general idea on that is to keep a list of guilty jobs per hive, when you start gpu recover you first add you guilty job to the hive and trylock hive->reset_lock. Any owner of hive->reset_lock (gpu recovery in progress) once he finished his recovery and released hive->reset_lock should go over hive->bad_jobs_list and if at least one of them is still not signaled (not done) trigger another gpu recovery and so on. If you do manage to trylock you also go over the list, clean it and perform recovery. This list probably needs to be protected with per hive lock. I also think we can for now at least finish reviewing the first 2 patches and submit them since as I said before they are not dealing with this topic and fixing existing race conditions. If you are OK with that I can send for review the last iteration of the first 2 patches where I wait for the last fence in ring mirror list. Andrey I implemented HW reset avoidance including XGMI use case according to the plan i specified. Patch is attached but I can't test it yet due to XGMI regression in PSP which is supposed to be fixed soon. Please take a look. Looks a bit too complicated on first glance. In general we should probably get away from handling a hive in any special way. Yes, I guess i can do it the same way as the generic handling in amdgpu_device_gpu_recover - there is a list of devices to process which is of size 1 for non xgmi use case or more then 1 for XGMI. Multiple timeout jobs in a hive are identical to multiple timeout jobs on different engines on a single device. How about the following handling: 1. Add the timeout job to a list. 2. Try to grab a lock to handle the reset, if that doesn't work because there is already a reset underway return immediately. 3. Stop all schedulers on all affected devices including stopping the timeouts and detaching all callbacks. 4. Double check the list of timed out jobs, if all hw fences of all jobs are completed now we actually don't need to do anything. What if all the jobs on the timed out list did complete but other job (or jobs) for which we removed the time out timer became hanged ? Wouldn't we miss a required reset in this case and wouldn't even have any indication of their hang ? If the timeout triggers before we disable it we will have the job on the list of jobs which are hanging. If we
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
On 01/17/2019 02:45 AM, Christian König wrote: Am 16.01.19 um 18:17 schrieb Grodzovsky, Andrey: On 01/16/2019 11:02 AM, Koenig, Christian wrote: Am 16.01.19 um 16:45 schrieb Grodzovsky, Andrey: On 01/16/2019 02:46 AM, Christian König wrote: Am 15.01.19 um 23:01 schrieb Grodzovsky, Andrey: On 01/11/2019 05:03 PM, Andrey Grodzovsky wrote: On 01/11/2019 02:11 PM, Koenig, Christian wrote: Am 11.01.19 um 16:37 schrieb Grodzovsky, Andrey: On 01/11/2019 04:42 AM, Koenig, Christian wrote: Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey: [SNIP] But we will not be adding the cb back in drm_sched_stop anymore, now we are only going to add back the cb in drm_sched_startr after rerunning those jobs in drm_sched_resubmit_jobs and assign them a new parent there anyway. Yeah, but when we find that we don't need to reset anything anymore then adding the callbacks again won't be possible any more. Christian. I am not sure I understand it, can u point me to example of how this will happen ? I am attaching my latest patches with waiting only for the last job's fence here just so we are on same page regarding the code. Well the whole idea is to prepare all schedulers, then check once more if the offending job hasn't completed in the meantime. If the job completed we need to be able to rollback everything and continue as if nothing had happened. Christian. Oh, but this piece of functionality - skipping HW ASIC reset in case the guilty job done is totally missing form this patch series and so needs to be added. So what you say actually is that for the case were we skip HW asic reset because the guilty job did complete we also need to skip resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the old parent fence pointer for reuse ? If so I would like to add all this functionality as a third patch since the first 2 patches are more about resolving race condition with jobs in flight while doing reset - what do you think ? Yeah, sounds perfectly fine to me. Christian. I realized there is another complication now for XGMI hive use case, we currently skip gpu recover for adev in case another gpu recover for different adev in same hive is running, under the assumption that we are going to reset all devices in hive anyway because that should cover our own dev too. But if we chose to skip now HW asic reset if our guilty job did finish we will aslo not HW reset any other devices in the hive even if one of them might actually had a bad job, wanted to do gpu recover but skipped it because our recover was in progress in that time. My general idea on that is to keep a list of guilty jobs per hive, when you start gpu recover you first add you guilty job to the hive and trylock hive->reset_lock. Any owner of hive->reset_lock (gpu recovery in progress) once he finished his recovery and released hive->reset_lock should go over hive->bad_jobs_list and if at least one of them is still not signaled (not done) trigger another gpu recovery and so on. If you do manage to trylock you also go over the list, clean it and perform recovery. This list probably needs to be protected with per hive lock. I also think we can for now at least finish reviewing the first 2 patches and submit them since as I said before they are not dealing with this topic and fixing existing race conditions. If you are OK with that I can send for review the last iteration of the first 2 patches where I wait for the last fence in ring mirror list. Andrey I implemented HW reset avoidance including XGMI use case according to the plan i specified. Patch is attached but I can't test it yet due to XGMI regression in PSP which is supposed to be fixed soon. Please take a look. Looks a bit too complicated on first glance. In general we should probably get away from handling a hive in any special way. Yes, I guess i can do it the same way as the generic handling in amdgpu_device_gpu_recover - there is a list of devices to process which is of size 1 for non xgmi use case or more then 1 for XGMI. Multiple timeout jobs in a hive are identical to multiple timeout jobs on different engines on a single device. How about the following handling: 1. Add the timeout job to a list. 2. Try to grab a lock to handle the reset, if that doesn't work because there is already a reset underway return immediately. 3. Stop all schedulers on all affected devices including stopping the timeouts and detaching all callbacks. 4. Double check the list of timed out jobs, if all hw fences of all jobs are completed now we actually don't need to do anything. What if all the jobs on the timed out list did complete but other job (or jobs) for which we removed the time out timer became hanged ? Wouldn't we miss a required reset in this case and wouldn't even have any indication of their hang ? If the timeout triggers before we disable it we will have the job on the list of jobs which are hanging. If we found that we don't reset and re-enable the
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
Am 16.01.19 um 18:17 schrieb Grodzovsky, Andrey: On 01/16/2019 11:02 AM, Koenig, Christian wrote: Am 16.01.19 um 16:45 schrieb Grodzovsky, Andrey: On 01/16/2019 02:46 AM, Christian König wrote: Am 15.01.19 um 23:01 schrieb Grodzovsky, Andrey: On 01/11/2019 05:03 PM, Andrey Grodzovsky wrote: On 01/11/2019 02:11 PM, Koenig, Christian wrote: Am 11.01.19 um 16:37 schrieb Grodzovsky, Andrey: On 01/11/2019 04:42 AM, Koenig, Christian wrote: Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey: [SNIP] But we will not be adding the cb back in drm_sched_stop anymore, now we are only going to add back the cb in drm_sched_startr after rerunning those jobs in drm_sched_resubmit_jobs and assign them a new parent there anyway. Yeah, but when we find that we don't need to reset anything anymore then adding the callbacks again won't be possible any more. Christian. I am not sure I understand it, can u point me to example of how this will happen ? I am attaching my latest patches with waiting only for the last job's fence here just so we are on same page regarding the code. Well the whole idea is to prepare all schedulers, then check once more if the offending job hasn't completed in the meantime. If the job completed we need to be able to rollback everything and continue as if nothing had happened. Christian. Oh, but this piece of functionality - skipping HW ASIC reset in case the guilty job done is totally missing form this patch series and so needs to be added. So what you say actually is that for the case were we skip HW asic reset because the guilty job did complete we also need to skip resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the old parent fence pointer for reuse ? If so I would like to add all this functionality as a third patch since the first 2 patches are more about resolving race condition with jobs in flight while doing reset - what do you think ? Yeah, sounds perfectly fine to me. Christian. I realized there is another complication now for XGMI hive use case, we currently skip gpu recover for adev in case another gpu recover for different adev in same hive is running, under the assumption that we are going to reset all devices in hive anyway because that should cover our own dev too. But if we chose to skip now HW asic reset if our guilty job did finish we will aslo not HW reset any other devices in the hive even if one of them might actually had a bad job, wanted to do gpu recover but skipped it because our recover was in progress in that time. My general idea on that is to keep a list of guilty jobs per hive, when you start gpu recover you first add you guilty job to the hive and trylock hive->reset_lock. Any owner of hive->reset_lock (gpu recovery in progress) once he finished his recovery and released hive->reset_lock should go over hive->bad_jobs_list and if at least one of them is still not signaled (not done) trigger another gpu recovery and so on. If you do manage to trylock you also go over the list, clean it and perform recovery. This list probably needs to be protected with per hive lock. I also think we can for now at least finish reviewing the first 2 patches and submit them since as I said before they are not dealing with this topic and fixing existing race conditions. If you are OK with that I can send for review the last iteration of the first 2 patches where I wait for the last fence in ring mirror list. Andrey I implemented HW reset avoidance including XGMI use case according to the plan i specified. Patch is attached but I can't test it yet due to XGMI regression in PSP which is supposed to be fixed soon. Please take a look. Looks a bit too complicated on first glance. In general we should probably get away from handling a hive in any special way. Yes, I guess i can do it the same way as the generic handling in amdgpu_device_gpu_recover - there is a list of devices to process which is of size 1 for non xgmi use case or more then 1 for XGMI. Multiple timeout jobs in a hive are identical to multiple timeout jobs on different engines on a single device. How about the following handling: 1. Add the timeout job to a list. 2. Try to grab a lock to handle the reset, if that doesn't work because there is already a reset underway return immediately. 3. Stop all schedulers on all affected devices including stopping the timeouts and detaching all callbacks. 4. Double check the list of timed out jobs, if all hw fences of all jobs are completed now we actually don't need to do anything. What if all the jobs on the timed out list did complete but other job (or jobs) for which we removed the time out timer became hanged ? Wouldn't we miss a required reset in this case and wouldn't even have any indication of their hang ? If the timeout triggers before we disable it we will have the job on the list of jobs which are hanging. If we found that we don't reset and re-enable the timeout it will trigger a bit later and we can do
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
On 01/16/2019 11:02 AM, Koenig, Christian wrote: Am 16.01.19 um 16:45 schrieb Grodzovsky, Andrey: On 01/16/2019 02:46 AM, Christian König wrote: Am 15.01.19 um 23:01 schrieb Grodzovsky, Andrey: On 01/11/2019 05:03 PM, Andrey Grodzovsky wrote: On 01/11/2019 02:11 PM, Koenig, Christian wrote: Am 11.01.19 um 16:37 schrieb Grodzovsky, Andrey: On 01/11/2019 04:42 AM, Koenig, Christian wrote: Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey: [SNIP] But we will not be adding the cb back in drm_sched_stop anymore, now we are only going to add back the cb in drm_sched_startr after rerunning those jobs in drm_sched_resubmit_jobs and assign them a new parent there anyway. Yeah, but when we find that we don't need to reset anything anymore then adding the callbacks again won't be possible any more. Christian. I am not sure I understand it, can u point me to example of how this will happen ? I am attaching my latest patches with waiting only for the last job's fence here just so we are on same page regarding the code. Well the whole idea is to prepare all schedulers, then check once more if the offending job hasn't completed in the meantime. If the job completed we need to be able to rollback everything and continue as if nothing had happened. Christian. Oh, but this piece of functionality - skipping HW ASIC reset in case the guilty job done is totally missing form this patch series and so needs to be added. So what you say actually is that for the case were we skip HW asic reset because the guilty job did complete we also need to skip resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the old parent fence pointer for reuse ? If so I would like to add all this functionality as a third patch since the first 2 patches are more about resolving race condition with jobs in flight while doing reset - what do you think ? Yeah, sounds perfectly fine to me. Christian. I realized there is another complication now for XGMI hive use case, we currently skip gpu recover for adev in case another gpu recover for different adev in same hive is running, under the assumption that we are going to reset all devices in hive anyway because that should cover our own dev too. But if we chose to skip now HW asic reset if our guilty job did finish we will aslo not HW reset any other devices in the hive even if one of them might actually had a bad job, wanted to do gpu recover but skipped it because our recover was in progress in that time. My general idea on that is to keep a list of guilty jobs per hive, when you start gpu recover you first add you guilty job to the hive and trylock hive->reset_lock. Any owner of hive->reset_lock (gpu recovery in progress) once he finished his recovery and released hive->reset_lock should go over hive->bad_jobs_list and if at least one of them is still not signaled (not done) trigger another gpu recovery and so on. If you do manage to trylock you also go over the list, clean it and perform recovery. This list probably needs to be protected with per hive lock. I also think we can for now at least finish reviewing the first 2 patches and submit them since as I said before they are not dealing with this topic and fixing existing race conditions. If you are OK with that I can send for review the last iteration of the first 2 patches where I wait for the last fence in ring mirror list. Andrey I implemented HW reset avoidance including XGMI use case according to the plan i specified. Patch is attached but I can't test it yet due to XGMI regression in PSP which is supposed to be fixed soon. Please take a look. Looks a bit too complicated on first glance. In general we should probably get away from handling a hive in any special way. Yes, I guess i can do it the same way as the generic handling in amdgpu_device_gpu_recover - there is a list of devices to process which is of size 1 for non xgmi use case or more then 1 for XGMI. Multiple timeout jobs in a hive are identical to multiple timeout jobs on different engines on a single device. How about the following handling: 1. Add the timeout job to a list. 2. Try to grab a lock to handle the reset, if that doesn't work because there is already a reset underway return immediately. 3. Stop all schedulers on all affected devices including stopping the timeouts and detaching all callbacks. 4. Double check the list of timed out jobs, if all hw fences of all jobs are completed now we actually don't need to do anything. What if all the jobs on the timed out list did complete but other job (or jobs) for which we removed the time out timer became hanged ? Wouldn't we miss a required reset in this case and wouldn't even have any indication of their hang ? If the timeout triggers before we disable it we will have the job on the list of jobs which are hanging. If we found that we don't reset and re-enable the timeout it will trigger a bit later and we can do the check again. Thinking about this a bit more we
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
Am 16.01.19 um 16:45 schrieb Grodzovsky, Andrey: On 01/16/2019 02:46 AM, Christian König wrote: Am 15.01.19 um 23:01 schrieb Grodzovsky, Andrey: On 01/11/2019 05:03 PM, Andrey Grodzovsky wrote: On 01/11/2019 02:11 PM, Koenig, Christian wrote: Am 11.01.19 um 16:37 schrieb Grodzovsky, Andrey: On 01/11/2019 04:42 AM, Koenig, Christian wrote: Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey: [SNIP] But we will not be adding the cb back in drm_sched_stop anymore, now we are only going to add back the cb in drm_sched_startr after rerunning those jobs in drm_sched_resubmit_jobs and assign them a new parent there anyway. Yeah, but when we find that we don't need to reset anything anymore then adding the callbacks again won't be possible any more. Christian. I am not sure I understand it, can u point me to example of how this will happen ? I am attaching my latest patches with waiting only for the last job's fence here just so we are on same page regarding the code. Well the whole idea is to prepare all schedulers, then check once more if the offending job hasn't completed in the meantime. If the job completed we need to be able to rollback everything and continue as if nothing had happened. Christian. Oh, but this piece of functionality - skipping HW ASIC reset in case the guilty job done is totally missing form this patch series and so needs to be added. So what you say actually is that for the case were we skip HW asic reset because the guilty job did complete we also need to skip resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the old parent fence pointer for reuse ? If so I would like to add all this functionality as a third patch since the first 2 patches are more about resolving race condition with jobs in flight while doing reset - what do you think ? Yeah, sounds perfectly fine to me. Christian. I realized there is another complication now for XGMI hive use case, we currently skip gpu recover for adev in case another gpu recover for different adev in same hive is running, under the assumption that we are going to reset all devices in hive anyway because that should cover our own dev too. But if we chose to skip now HW asic reset if our guilty job did finish we will aslo not HW reset any other devices in the hive even if one of them might actually had a bad job, wanted to do gpu recover but skipped it because our recover was in progress in that time. My general idea on that is to keep a list of guilty jobs per hive, when you start gpu recover you first add you guilty job to the hive and trylock hive->reset_lock. Any owner of hive->reset_lock (gpu recovery in progress) once he finished his recovery and released hive->reset_lock should go over hive->bad_jobs_list and if at least one of them is still not signaled (not done) trigger another gpu recovery and so on. If you do manage to trylock you also go over the list, clean it and perform recovery. This list probably needs to be protected with per hive lock. I also think we can for now at least finish reviewing the first 2 patches and submit them since as I said before they are not dealing with this topic and fixing existing race conditions. If you are OK with that I can send for review the last iteration of the first 2 patches where I wait for the last fence in ring mirror list. Andrey I implemented HW reset avoidance including XGMI use case according to the plan i specified. Patch is attached but I can't test it yet due to XGMI regression in PSP which is supposed to be fixed soon. Please take a look. Looks a bit too complicated on first glance. In general we should probably get away from handling a hive in any special way. Yes, I guess i can do it the same way as the generic handling in amdgpu_device_gpu_recover - there is a list of devices to process which is of size 1 for non xgmi use case or more then 1 for XGMI. Multiple timeout jobs in a hive are identical to multiple timeout jobs on different engines on a single device. How about the following handling: 1. Add the timeout job to a list. 2. Try to grab a lock to handle the reset, if that doesn't work because there is already a reset underway return immediately. 3. Stop all schedulers on all affected devices including stopping the timeouts and detaching all callbacks. 4. Double check the list of timed out jobs, if all hw fences of all jobs are completed now we actually don't need to do anything. What if all the jobs on the timed out list did complete but other job (or jobs) for which we removed the time out timer became hanged ? Wouldn't we miss a required reset in this case and wouldn't even have any indication of their hang ? If the timeout triggers before we disable it we will have the job on the list of jobs which are hanging. If we found that we don't reset and re-enable the timeout it will trigger a bit later and we can do the check again. Thinking about this a bit more we actually don't need to change the handling in any
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
On 01/16/2019 02:46 AM, Christian König wrote: Am 15.01.19 um 23:01 schrieb Grodzovsky, Andrey: On 01/11/2019 05:03 PM, Andrey Grodzovsky wrote: On 01/11/2019 02:11 PM, Koenig, Christian wrote: Am 11.01.19 um 16:37 schrieb Grodzovsky, Andrey: On 01/11/2019 04:42 AM, Koenig, Christian wrote: Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey: [SNIP] But we will not be adding the cb back in drm_sched_stop anymore, now we are only going to add back the cb in drm_sched_startr after rerunning those jobs in drm_sched_resubmit_jobs and assign them a new parent there anyway. Yeah, but when we find that we don't need to reset anything anymore then adding the callbacks again won't be possible any more. Christian. I am not sure I understand it, can u point me to example of how this will happen ? I am attaching my latest patches with waiting only for the last job's fence here just so we are on same page regarding the code. Well the whole idea is to prepare all schedulers, then check once more if the offending job hasn't completed in the meantime. If the job completed we need to be able to rollback everything and continue as if nothing had happened. Christian. Oh, but this piece of functionality - skipping HW ASIC reset in case the guilty job done is totally missing form this patch series and so needs to be added. So what you say actually is that for the case were we skip HW asic reset because the guilty job did complete we also need to skip resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the old parent fence pointer for reuse ? If so I would like to add all this functionality as a third patch since the first 2 patches are more about resolving race condition with jobs in flight while doing reset - what do you think ? Yeah, sounds perfectly fine to me. Christian. I realized there is another complication now for XGMI hive use case, we currently skip gpu recover for adev in case another gpu recover for different adev in same hive is running, under the assumption that we are going to reset all devices in hive anyway because that should cover our own dev too. But if we chose to skip now HW asic reset if our guilty job did finish we will aslo not HW reset any other devices in the hive even if one of them might actually had a bad job, wanted to do gpu recover but skipped it because our recover was in progress in that time. My general idea on that is to keep a list of guilty jobs per hive, when you start gpu recover you first add you guilty job to the hive and trylock hive->reset_lock. Any owner of hive->reset_lock (gpu recovery in progress) once he finished his recovery and released hive->reset_lock should go over hive->bad_jobs_list and if at least one of them is still not signaled (not done) trigger another gpu recovery and so on. If you do manage to trylock you also go over the list, clean it and perform recovery. This list probably needs to be protected with per hive lock. I also think we can for now at least finish reviewing the first 2 patches and submit them since as I said before they are not dealing with this topic and fixing existing race conditions. If you are OK with that I can send for review the last iteration of the first 2 patches where I wait for the last fence in ring mirror list. Andrey I implemented HW reset avoidance including XGMI use case according to the plan i specified. Patch is attached but I can't test it yet due to XGMI regression in PSP which is supposed to be fixed soon. Please take a look. Looks a bit too complicated on first glance. In general we should probably get away from handling a hive in any special way. Yes, I guess i can do it the same way as the generic handling in amdgpu_device_gpu_recover - there is a list of devices to process which is of size 1 for non xgmi use case or more then 1 for XGMI. Multiple timeout jobs in a hive are identical to multiple timeout jobs on different engines on a single device. How about the following handling: 1. Add the timeout job to a list. 2. Try to grab a lock to handle the reset, if that doesn't work because there is already a reset underway return immediately. 3. Stop all schedulers on all affected devices including stopping the timeouts and detaching all callbacks. 4. Double check the list of timed out jobs, if all hw fences of all jobs are completed now we actually don't need to do anything. What if all the jobs on the timed out list did complete but other job (or jobs) for which we removed the time out timer became hanged ? Wouldn't we miss a required reset in this case and wouldn't even have any indication of their hang ? 5. Do the reset on all affected devices. 6. Drop the lock. 7. Add callbacks again and restart the schedulers. I see your steps don't include flushing any tdr in progress from drm_sched_job_finish (or as I did it from amdgpu_job_free_cb) does it mean you don't think we need to flush in order to avoid freeing job while it still might be accessed from time
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
Am 15.01.19 um 23:01 schrieb Grodzovsky, Andrey: On 01/11/2019 05:03 PM, Andrey Grodzovsky wrote: On 01/11/2019 02:11 PM, Koenig, Christian wrote: Am 11.01.19 um 16:37 schrieb Grodzovsky, Andrey: On 01/11/2019 04:42 AM, Koenig, Christian wrote: Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey: [SNIP] But we will not be adding the cb back in drm_sched_stop anymore, now we are only going to add back the cb in drm_sched_startr after rerunning those jobs in drm_sched_resubmit_jobs and assign them a new parent there anyway. Yeah, but when we find that we don't need to reset anything anymore then adding the callbacks again won't be possible any more. Christian. I am not sure I understand it, can u point me to example of how this will happen ? I am attaching my latest patches with waiting only for the last job's fence here just so we are on same page regarding the code. Well the whole idea is to prepare all schedulers, then check once more if the offending job hasn't completed in the meantime. If the job completed we need to be able to rollback everything and continue as if nothing had happened. Christian. Oh, but this piece of functionality - skipping HW ASIC reset in case the guilty job done is totally missing form this patch series and so needs to be added. So what you say actually is that for the case were we skip HW asic reset because the guilty job did complete we also need to skip resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the old parent fence pointer for reuse ? If so I would like to add all this functionality as a third patch since the first 2 patches are more about resolving race condition with jobs in flight while doing reset - what do you think ? Yeah, sounds perfectly fine to me. Christian. I realized there is another complication now for XGMI hive use case, we currently skip gpu recover for adev in case another gpu recover for different adev in same hive is running, under the assumption that we are going to reset all devices in hive anyway because that should cover our own dev too. But if we chose to skip now HW asic reset if our guilty job did finish we will aslo not HW reset any other devices in the hive even if one of them might actually had a bad job, wanted to do gpu recover but skipped it because our recover was in progress in that time. My general idea on that is to keep a list of guilty jobs per hive, when you start gpu recover you first add you guilty job to the hive and trylock hive->reset_lock. Any owner of hive->reset_lock (gpu recovery in progress) once he finished his recovery and released hive->reset_lock should go over hive->bad_jobs_list and if at least one of them is still not signaled (not done) trigger another gpu recovery and so on. If you do manage to trylock you also go over the list, clean it and perform recovery. This list probably needs to be protected with per hive lock. I also think we can for now at least finish reviewing the first 2 patches and submit them since as I said before they are not dealing with this topic and fixing existing race conditions. If you are OK with that I can send for review the last iteration of the first 2 patches where I wait for the last fence in ring mirror list. Andrey I implemented HW reset avoidance including XGMI use case according to the plan i specified. Patch is attached but I can't test it yet due to XGMI regression in PSP which is supposed to be fixed soon. Please take a look. Looks a bit too complicated on first glance. In general we should probably get away from handling a hive in any special way. Multiple timeout jobs in a hive are identical to multiple timeout jobs on different engines on a single device. How about the following handling: 1. Add the timeout job to a list. 2. Try to grab a lock to handle the reset, if that doesn't work because there is already a reset underway return immediately. 3. Stop all schedulers on all affected devices including stopping the timeouts and detaching all callbacks. 4. Double check the list of timed out jobs, if all hw fences of all jobs are completed now we actually don't need to do anything. 5. Do the reset on all affected devices. 6. Drop the lock. 7. Add callbacks again and restart the schedulers. Regards, Christian. Andrey Andrey Andrey ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
On 01/11/2019 05:03 PM, Andrey Grodzovsky wrote: > > > On 01/11/2019 02:11 PM, Koenig, Christian wrote: >> Am 11.01.19 um 16:37 schrieb Grodzovsky, Andrey: >>> On 01/11/2019 04:42 AM, Koenig, Christian wrote: Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey: > [SNIP] But we will not be adding the cb back in drm_sched_stop anymore, now we are only going to add back the cb in drm_sched_startr after rerunning those jobs in drm_sched_resubmit_jobs and assign them a new parent there anyway. >>> Yeah, but when we find that we don't need to reset anything anymore >>> then adding the callbacks again won't be possible any more. >>> >>> Christian. >> I am not sure I understand it, can u point me to example of how this >> will happen ? I am attaching my latest patches with waiting only for >> the last job's fence here just so we are on same page regarding >> the code. Well the whole idea is to prepare all schedulers, then check once more if the offending job hasn't completed in the meantime. If the job completed we need to be able to rollback everything and continue as if nothing had happened. Christian. >>> Oh, but this piece of functionality - skipping HW ASIC reset in case >>> the >>> guilty job done is totally missing form this patch series and so needs >>> to be added. So what you say actually is that for the case were we skip >>> HW asic reset because the guilty job did complete we also need to skip >>> resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the >>> old parent fence pointer for reuse ? If so I would like to add all this >>> functionality as a third patch since the first 2 patches are more about >>> resolving race condition with jobs in flight while doing reset - >>> what do >>> you think ? >> Yeah, sounds perfectly fine to me. >> >> Christian. > > I realized there is another complication now for XGMI hive use case, > we currently skip gpu recover for adev in case another gpu recover for > different adev in same hive is running, under the assumption that we > are going to reset all devices in hive anyway because that should > cover our own dev too. But if we chose to skip now HW asic reset if > our guilty job did finish we will aslo not HW reset any other devices > in the hive even if one of them might actually had a bad job, wanted > to do gpu recover but skipped it because our recover was in progress > in that time. > My general idea on that is to keep a list of guilty jobs per hive, > when you start gpu recover you first add you guilty job to the hive > and trylock hive->reset_lock. Any owner of hive->reset_lock (gpu > recovery in progress) once he finished his recovery and released > hive->reset_lock should go over hive->bad_jobs_list and if at least > one of them is still not signaled (not done) trigger another gpu > recovery and so on. If you do manage to trylock you also go over the > list, clean it and perform recovery. This list probably needs to be > protected with per hive lock. > I also think we can for now at least finish reviewing the first 2 > patches and submit them since as I said before they are not dealing > with this topic and fixing existing race conditions. If you are OK > with that I can send for review the last iteration of the first 2 > patches where I wait for the last fence in ring mirror list. > > Andrey I implemented HW reset avoidance including XGMI use case according to the plan i specified. Patch is attached but I can't test it yet due to XGMI regression in PSP which is supposed to be fixed soon. Please take a look. Andrey > >> >>> Andrey >> Andrey >> ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> ___ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > From 60085547eec4002fe447783be68e7f14342944ff Mon Sep 17 00:00:00 2001 From: Andrey Grodzovsky Date: Tue, 15 Jan 2019 15:39:38 -0500 Subject: drm/sched: Skip HW reset if guilty job is signaled. Also take care of XGMI use case - special care needs to be taken of other devs in hive who had jobs TO but delegated their reset to current reset instance assuming it will do HW reset for them. In such case the reset in progress after finishing (and if HW reset was optimized) will inspect a perhive list of jobs for any jobs which still not finished and will issue another reset for them. Signed-off-by: Andrey Grodzovsky --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 100 ++--- drivers/gpu/drm/amd/amdgpu/amdgpu_job.c| 29 + drivers/gpu/drm/amd/amdgpu/amdgpu_job.h| 2 + drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 3 +
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
On 01/11/2019 02:11 PM, Koenig, Christian wrote: > Am 11.01.19 um 16:37 schrieb Grodzovsky, Andrey: >> On 01/11/2019 04:42 AM, Koenig, Christian wrote: >>> Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey: [SNIP] >>> But we will not be adding the cb back in drm_sched_stop anymore, now we >>> are only going to add back the cb in drm_sched_startr after rerunning >>> those jobs in drm_sched_resubmit_jobs and assign them a new parent >>> there >>> anyway. >> Yeah, but when we find that we don't need to reset anything anymore >> then adding the callbacks again won't be possible any more. >> >> Christian. > I am not sure I understand it, can u point me to example of how this > will happen ? I am attaching my latest patches with waiting only for > the last job's fence here just so we are on same page regarding the code. >>> Well the whole idea is to prepare all schedulers, then check once more >>> if the offending job hasn't completed in the meantime. >>> >>> If the job completed we need to be able to rollback everything and >>> continue as if nothing had happened. >>> >>> Christian. >> Oh, but this piece of functionality - skipping HW ASIC reset in case the >> guilty job done is totally missing form this patch series and so needs >> to be added. So what you say actually is that for the case were we skip >> HW asic reset because the guilty job did complete we also need to skip >> resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the >> old parent fence pointer for reuse ? If so I would like to add all this >> functionality as a third patch since the first 2 patches are more about >> resolving race condition with jobs in flight while doing reset - what do >> you think ? > Yeah, sounds perfectly fine to me. > > Christian. I realized there is another complication now for XGMI hive use case, we currently skip gpu recover for adev in case another gpu recover for different adev in same hive is running, under the assumption that we are going to reset all devices in hive anyway because that should cover our own dev too. But if we chose to skip now HW asic reset if our guilty job did finish we will aslo not HW reset any other devices in the hive even if one of them might actually had a bad job, wanted to do gpu recover but skipped it because our recover was in progress in that time. My general idea on that is to keep a list of guilty jobs per hive, when you start gpu recover you first add you guilty job to the hive and trylock hive->reset_lock. Any owner of hive->reset_lock (gpu recovery in progress) once he finished his recovery and released hive->reset_lock should go over hive->bad_jobs_list and if at least one of them is still not signaled (not done) trigger another gpu recovery and so on. If you do manage to trylock you also go over the list, clean it and perform recovery. This list probably needs to be protected with per hive lock. I also think we can for now at least finish reviewing the first 2 patches and submit them since as I said before they are not dealing with this topic and fixing existing race conditions. If you are OK with that I can send for review the last iteration of the first 2 patches where I wait for the last fence in ring mirror list. Andrey > >> Andrey > Andrey > >>> ___ >>> amd-gfx mailing list >>> amd-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > ___ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
Am 11.01.19 um 16:37 schrieb Grodzovsky, Andrey: > > On 01/11/2019 04:42 AM, Koenig, Christian wrote: >> Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey: >>> [SNIP] >> But we will not be adding the cb back in drm_sched_stop anymore, now we >> are only going to add back the cb in drm_sched_startr after rerunning >> those jobs in drm_sched_resubmit_jobs and assign them a new parent >> there >> anyway. > Yeah, but when we find that we don't need to reset anything anymore > then adding the callbacks again won't be possible any more. > > Christian. I am not sure I understand it, can u point me to example of how this will happen ? I am attaching my latest patches with waiting only for the last job's fence here just so we are on same page regarding the code. >> Well the whole idea is to prepare all schedulers, then check once more >> if the offending job hasn't completed in the meantime. >> >> If the job completed we need to be able to rollback everything and >> continue as if nothing had happened. >> >> Christian. > Oh, but this piece of functionality - skipping HW ASIC reset in case the > guilty job done is totally missing form this patch series and so needs > to be added. So what you say actually is that for the case were we skip > HW asic reset because the guilty job did complete we also need to skip > resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the > old parent fence pointer for reuse ? If so I would like to add all this > functionality as a third patch since the first 2 patches are more about > resolving race condition with jobs in flight while doing reset - what do > you think ? Yeah, sounds perfectly fine to me. Christian. > > Andrey Andrey >> ___ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
On 01/11/2019 04:42 AM, Koenig, Christian wrote: > Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey: >> [SNIP] > But we will not be adding the cb back in drm_sched_stop anymore, now we > are only going to add back the cb in drm_sched_startr after rerunning > those jobs in drm_sched_resubmit_jobs and assign them a new parent > there > anyway. Yeah, but when we find that we don't need to reset anything anymore then adding the callbacks again won't be possible any more. Christian. >>> I am not sure I understand it, can u point me to example of how this >>> will happen ? I am attaching my latest patches with waiting only for >>> the last job's fence here just so we are on same page regarding the code. > Well the whole idea is to prepare all schedulers, then check once more > if the offending job hasn't completed in the meantime. > > If the job completed we need to be able to rollback everything and > continue as if nothing had happened. > > Christian. Oh, but this piece of functionality - skipping HW ASIC reset in case the guilty job done is totally missing form this patch series and so needs to be added. So what you say actually is that for the case were we skip HW asic reset because the guilty job did complete we also need to skip resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the old parent fence pointer for reuse ? If so I would like to add all this functionality as a third patch since the first 2 patches are more about resolving race condition with jobs in flight while doing reset - what do you think ? Andrey > >>> Andrey >>> > ___ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey: > [SNIP] But we will not be adding the cb back in drm_sched_stop anymore, now we are only going to add back the cb in drm_sched_startr after rerunning those jobs in drm_sched_resubmit_jobs and assign them a new parent there anyway. >>> Yeah, but when we find that we don't need to reset anything anymore >>> then adding the callbacks again won't be possible any more. >>> >>> Christian. >> I am not sure I understand it, can u point me to example of how this >> will happen ? I am attaching my latest patches with waiting only for >> the last job's fence here just so we are on same page regarding the code. Well the whole idea is to prepare all schedulers, then check once more if the offending job hasn't completed in the meantime. If the job completed we need to be able to rollback everything and continue as if nothing had happened. Christian. >> >> Andrey >> ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
Just a ping.
Andrey
On 01/09/2019 10:18 AM, Andrey Grodzovsky wrote:
>
>
> On 01/09/2019 05:22 AM, Christian König wrote:
>> Am 07.01.19 um 20:47 schrieb Grodzovsky, Andrey:
>>>
>>> On 01/07/2019 09:13 AM, Christian König wrote:
Am 03.01.19 um 18:42 schrieb Grodzovsky, Andrey:
> On 01/03/2019 11:20 AM, Grodzovsky, Andrey wrote:
>> On 01/03/2019 03:54 AM, Koenig, Christian wrote:
>>> Am 21.12.18 um 21:36 schrieb Grodzovsky, Andrey:
On 12/21/2018 01:37 PM, Christian König wrote:
> Am 20.12.18 um 20:23 schrieb Andrey Grodzovsky:
>> Decauple sched threads stop and start and ring mirror
>> list handling from the policy of what to do about the
>> guilty jobs.
>> When stoppping the sched thread and detaching sched fences
>> from non signaled HW fenes wait for all signaled HW fences
>> to complete before rerunning the jobs.
>>
>> v2: Fix resubmission of guilty job into HW after refactoring.
>>
>> v4:
>> Full restart for all the jobs, not only from guilty ring.
>> Extract karma increase into standalone function.
>>
>> v5:
>> Rework waiting for signaled jobs without relying on the job
>> struct itself as those might already be freed for non 'guilty'
>> job's schedulers.
>> Expose karma increase to drivers.
>>
>> Suggested-by: Christian Koenig
>> Signed-off-by: Andrey Grodzovsky
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +--
>> drivers/gpu/drm/etnaviv/etnaviv_sched.c | 11 +-
>> drivers/gpu/drm/scheduler/sched_main.c | 188
>> +++--
>> drivers/gpu/drm/v3d/v3d_sched.c | 12 +-
>> include/drm/gpu_scheduler.h | 10 +-
>> 5 files changed, 151 insertions(+), 88 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 8a078f4..a4bd2d3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3298,12 +3298,10 @@ static int
>> amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>> if (!ring || !ring->sched.thread)
>> continue;
>> - kthread_park(ring->sched.thread);
>> + drm_sched_stop(>sched, job ? >base : NULL);
>> - if (job && job->base.sched != >sched)
>> - continue;
>> -
>> - drm_sched_hw_job_reset(>sched, job ? >base :
>> NULL);
>> + if(job)
>> + drm_sched_increase_karma(>base);
> Since we dropped the "job && job->base.sched != >sched"
> check
> above this will now increase the jobs karma multiple times.
>
> Maybe just move that outside of the loop.
>
>> /* after all hw jobs are reset, hw fence is
>> meaningless,
>> so force_completion */
>> amdgpu_fence_driver_force_completion(ring);
>> @@ -3454,14 +3452,10 @@ static void
>> amdgpu_device_post_asic_reset(struct amdgpu_device *adev,
>> if (!ring || !ring->sched.thread)
>> continue;
>> - /* only need recovery sched of the given
>> job's ring
>> - * or all rings (in the case @job is NULL)
>> - * after above amdgpu_reset accomplished
>> - */
>> - if ((!job || job->base.sched == >sched) &&
>> !adev->asic_reset_res)
>> - drm_sched_job_recovery(>sched);
>> + if (!adev->asic_reset_res)
>> + drm_sched_resubmit_jobs(>sched);
>> - kthread_unpark(ring->sched.thread);
>> + drm_sched_start(>sched, !adev->asic_reset_res);
>> }
>> if (!amdgpu_device_has_dc_support(adev)) {
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> index 49a6763..6f1268f 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> @@ -109,16 +109,19 @@ static void
>> etnaviv_sched_timedout_job(struct
>> drm_sched_job *sched_job)
>> }
>> /* block scheduler */
>> - kthread_park(gpu->sched.thread);
>> - drm_sched_hw_job_reset(>sched, sched_job);
>> + drm_sched_stop(>sched, sched_job);
>> +
>> + if(sched_job)
>> + drm_sched_increase_karma(sched_job);
>> /* get the GPU back into the init state */
>> etnaviv_core_dump(gpu);
>>
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
On 01/09/2019 05:22 AM, Christian König wrote:
> Am 07.01.19 um 20:47 schrieb Grodzovsky, Andrey:
>>
>> On 01/07/2019 09:13 AM, Christian König wrote:
>>> Am 03.01.19 um 18:42 schrieb Grodzovsky, Andrey:
On 01/03/2019 11:20 AM, Grodzovsky, Andrey wrote:
> On 01/03/2019 03:54 AM, Koenig, Christian wrote:
>> Am 21.12.18 um 21:36 schrieb Grodzovsky, Andrey:
>>> On 12/21/2018 01:37 PM, Christian König wrote:
Am 20.12.18 um 20:23 schrieb Andrey Grodzovsky:
> Decauple sched threads stop and start and ring mirror
> list handling from the policy of what to do about the
> guilty jobs.
> When stoppping the sched thread and detaching sched fences
> from non signaled HW fenes wait for all signaled HW fences
> to complete before rerunning the jobs.
>
> v2: Fix resubmission of guilty job into HW after refactoring.
>
> v4:
> Full restart for all the jobs, not only from guilty ring.
> Extract karma increase into standalone function.
>
> v5:
> Rework waiting for signaled jobs without relying on the job
> struct itself as those might already be freed for non 'guilty'
> job's schedulers.
> Expose karma increase to drivers.
>
> Suggested-by: Christian Koenig
> Signed-off-by: Andrey Grodzovsky
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +--
> drivers/gpu/drm/etnaviv/etnaviv_sched.c | 11 +-
> drivers/gpu/drm/scheduler/sched_main.c | 188
> +++--
> drivers/gpu/drm/v3d/v3d_sched.c | 12 +-
> include/drm/gpu_scheduler.h | 10 +-
> 5 files changed, 151 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 8a078f4..a4bd2d3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3298,12 +3298,10 @@ static int
> amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> if (!ring || !ring->sched.thread)
> continue;
> - kthread_park(ring->sched.thread);
> + drm_sched_stop(>sched, job ? >base : NULL);
> - if (job && job->base.sched != >sched)
> - continue;
> -
> - drm_sched_hw_job_reset(>sched, job ? >base :
> NULL);
> + if(job)
> + drm_sched_increase_karma(>base);
Since we dropped the "job && job->base.sched != >sched"
check
above this will now increase the jobs karma multiple times.
Maybe just move that outside of the loop.
> /* after all hw jobs are reset, hw fence is
> meaningless,
> so force_completion */
> amdgpu_fence_driver_force_completion(ring);
> @@ -3454,14 +3452,10 @@ static void
> amdgpu_device_post_asic_reset(struct amdgpu_device *adev,
> if (!ring || !ring->sched.thread)
> continue;
> - /* only need recovery sched of the given job's
> ring
> - * or all rings (in the case @job is NULL)
> - * after above amdgpu_reset accomplished
> - */
> - if ((!job || job->base.sched == >sched) &&
> !adev->asic_reset_res)
> - drm_sched_job_recovery(>sched);
> + if (!adev->asic_reset_res)
> + drm_sched_resubmit_jobs(>sched);
> - kthread_unpark(ring->sched.thread);
> + drm_sched_start(>sched, !adev->asic_reset_res);
> }
> if (!amdgpu_device_has_dc_support(adev)) {
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> index 49a6763..6f1268f 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> @@ -109,16 +109,19 @@ static void
> etnaviv_sched_timedout_job(struct
> drm_sched_job *sched_job)
> }
> /* block scheduler */
> - kthread_park(gpu->sched.thread);
> - drm_sched_hw_job_reset(>sched, sched_job);
> + drm_sched_stop(>sched, sched_job);
> +
> + if(sched_job)
> + drm_sched_increase_karma(sched_job);
> /* get the GPU back into the init state */
> etnaviv_core_dump(gpu);
> etnaviv_gpu_recover_hang(gpu);
> + drm_sched_resubmit_jobs(>sched);
> +
> /* restart scheduler after GPU is usable again */
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
Am 07.01.19 um 20:47 schrieb Grodzovsky, Andrey:
On 01/07/2019 09:13 AM, Christian König wrote:
Am 03.01.19 um 18:42 schrieb Grodzovsky, Andrey:
On 01/03/2019 11:20 AM, Grodzovsky, Andrey wrote:
On 01/03/2019 03:54 AM, Koenig, Christian wrote:
Am 21.12.18 um 21:36 schrieb Grodzovsky, Andrey:
On 12/21/2018 01:37 PM, Christian König wrote:
Am 20.12.18 um 20:23 schrieb Andrey Grodzovsky:
Decauple sched threads stop and start and ring mirror
list handling from the policy of what to do about the
guilty jobs.
When stoppping the sched thread and detaching sched fences
from non signaled HW fenes wait for all signaled HW fences
to complete before rerunning the jobs.
v2: Fix resubmission of guilty job into HW after refactoring.
v4:
Full restart for all the jobs, not only from guilty ring.
Extract karma increase into standalone function.
v5:
Rework waiting for signaled jobs without relying on the job
struct itself as those might already be freed for non 'guilty'
job's schedulers.
Expose karma increase to drivers.
Suggested-by: Christian Koenig
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +--
drivers/gpu/drm/etnaviv/etnaviv_sched.c | 11 +-
drivers/gpu/drm/scheduler/sched_main.c | 188
+++--
drivers/gpu/drm/v3d/v3d_sched.c | 12 +-
include/drm/gpu_scheduler.h | 10 +-
5 files changed, 151 insertions(+), 88 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 8a078f4..a4bd2d3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3298,12 +3298,10 @@ static int
amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- kthread_park(ring->sched.thread);
+ drm_sched_stop(>sched, job ? >base : NULL);
- if (job && job->base.sched != >sched)
- continue;
-
- drm_sched_hw_job_reset(>sched, job ? >base :
NULL);
+ if(job)
+ drm_sched_increase_karma(>base);
Since we dropped the "job && job->base.sched != >sched" check
above this will now increase the jobs karma multiple times.
Maybe just move that outside of the loop.
/* after all hw jobs are reset, hw fence is
meaningless,
so force_completion */
amdgpu_fence_driver_force_completion(ring);
@@ -3454,14 +3452,10 @@ static void
amdgpu_device_post_asic_reset(struct amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- /* only need recovery sched of the given job's ring
- * or all rings (in the case @job is NULL)
- * after above amdgpu_reset accomplished
- */
- if ((!job || job->base.sched == >sched) &&
!adev->asic_reset_res)
- drm_sched_job_recovery(>sched);
+ if (!adev->asic_reset_res)
+ drm_sched_resubmit_jobs(>sched);
- kthread_unpark(ring->sched.thread);
+ drm_sched_start(>sched, !adev->asic_reset_res);
}
if (!amdgpu_device_has_dc_support(adev)) {
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
index 49a6763..6f1268f 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
@@ -109,16 +109,19 @@ static void etnaviv_sched_timedout_job(struct
drm_sched_job *sched_job)
}
/* block scheduler */
- kthread_park(gpu->sched.thread);
- drm_sched_hw_job_reset(>sched, sched_job);
+ drm_sched_stop(>sched, sched_job);
+
+ if(sched_job)
+ drm_sched_increase_karma(sched_job);
/* get the GPU back into the init state */
etnaviv_core_dump(gpu);
etnaviv_gpu_recover_hang(gpu);
+ drm_sched_resubmit_jobs(>sched);
+
/* restart scheduler after GPU is usable again */
- drm_sched_job_recovery(>sched);
- kthread_unpark(gpu->sched.thread);
+ drm_sched_start(>sched, true);
}
static void etnaviv_sched_free_job(struct drm_sched_job
*sched_job)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index dbb6906..b5c5bee 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -60,8 +60,6 @@
static void drm_sched_process_job(struct dma_fence *f,
struct
dma_fence_cb *cb);
-static void drm_sched_expel_job_unlocked(struct
drm_sched_job
*s_job);
-
/**
* drm_sched_rq_init - initialize a given run queue struct
*
@@ -335,6 +333,42 @@ static void drm_sched_job_timedout(struct
work_struct *work)
spin_unlock_irqrestore(>job_list_lock, flags);
}
Kernel doc here would be nice to have.
+void drm_sched_increase_karma(struct drm_sched_job *bad)
+{
+ int i;
+ struct drm_sched_entity *tmp;
+ struct drm_sched_entity
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
On 01/07/2019 09:13 AM, Christian König wrote:
> Am 03.01.19 um 18:42 schrieb Grodzovsky, Andrey:
>>
>> On 01/03/2019 11:20 AM, Grodzovsky, Andrey wrote:
>>> On 01/03/2019 03:54 AM, Koenig, Christian wrote:
Am 21.12.18 um 21:36 schrieb Grodzovsky, Andrey:
> On 12/21/2018 01:37 PM, Christian König wrote:
>> Am 20.12.18 um 20:23 schrieb Andrey Grodzovsky:
>>> Decauple sched threads stop and start and ring mirror
>>> list handling from the policy of what to do about the
>>> guilty jobs.
>>> When stoppping the sched thread and detaching sched fences
>>> from non signaled HW fenes wait for all signaled HW fences
>>> to complete before rerunning the jobs.
>>>
>>> v2: Fix resubmission of guilty job into HW after refactoring.
>>>
>>> v4:
>>> Full restart for all the jobs, not only from guilty ring.
>>> Extract karma increase into standalone function.
>>>
>>> v5:
>>> Rework waiting for signaled jobs without relying on the job
>>> struct itself as those might already be freed for non 'guilty'
>>> job's schedulers.
>>> Expose karma increase to drivers.
>>>
>>> Suggested-by: Christian Koenig
>>> Signed-off-by: Andrey Grodzovsky
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +--
>>> drivers/gpu/drm/etnaviv/etnaviv_sched.c | 11 +-
>>> drivers/gpu/drm/scheduler/sched_main.c | 188
>>> +++--
>>> drivers/gpu/drm/v3d/v3d_sched.c | 12 +-
>>> include/drm/gpu_scheduler.h | 10 +-
>>> 5 files changed, 151 insertions(+), 88 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 8a078f4..a4bd2d3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -3298,12 +3298,10 @@ static int
>>> amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>> if (!ring || !ring->sched.thread)
>>> continue;
>>> - kthread_park(ring->sched.thread);
>>> + drm_sched_stop(>sched, job ? >base : NULL);
>>> - if (job && job->base.sched != >sched)
>>> - continue;
>>> -
>>> - drm_sched_hw_job_reset(>sched, job ? >base :
>>> NULL);
>>> + if(job)
>>> + drm_sched_increase_karma(>base);
>> Since we dropped the "job && job->base.sched != >sched" check
>> above this will now increase the jobs karma multiple times.
>>
>> Maybe just move that outside of the loop.
>>
>>> /* after all hw jobs are reset, hw fence is
>>> meaningless,
>>> so force_completion */
>>> amdgpu_fence_driver_force_completion(ring);
>>> @@ -3454,14 +3452,10 @@ static void
>>> amdgpu_device_post_asic_reset(struct amdgpu_device *adev,
>>> if (!ring || !ring->sched.thread)
>>> continue;
>>> - /* only need recovery sched of the given job's ring
>>> - * or all rings (in the case @job is NULL)
>>> - * after above amdgpu_reset accomplished
>>> - */
>>> - if ((!job || job->base.sched == >sched) &&
>>> !adev->asic_reset_res)
>>> - drm_sched_job_recovery(>sched);
>>> + if (!adev->asic_reset_res)
>>> + drm_sched_resubmit_jobs(>sched);
>>> - kthread_unpark(ring->sched.thread);
>>> + drm_sched_start(>sched, !adev->asic_reset_res);
>>> }
>>> if (!amdgpu_device_has_dc_support(adev)) {
>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>>> b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>>> index 49a6763..6f1268f 100644
>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>>> @@ -109,16 +109,19 @@ static void etnaviv_sched_timedout_job(struct
>>> drm_sched_job *sched_job)
>>> }
>>> /* block scheduler */
>>> - kthread_park(gpu->sched.thread);
>>> - drm_sched_hw_job_reset(>sched, sched_job);
>>> + drm_sched_stop(>sched, sched_job);
>>> +
>>> + if(sched_job)
>>> + drm_sched_increase_karma(sched_job);
>>> /* get the GPU back into the init state */
>>> etnaviv_core_dump(gpu);
>>> etnaviv_gpu_recover_hang(gpu);
>>> + drm_sched_resubmit_jobs(>sched);
>>> +
>>> /* restart scheduler after GPU is usable again */
>>> - drm_sched_job_recovery(>sched);
>>> - kthread_unpark(gpu->sched.thread);
>>> + drm_sched_start(>sched, true);
>>> }
>>> static void etnaviv_sched_free_job(struct drm_sched_job
>>> *sched_job)
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
Am 03.01.19 um 18:42 schrieb Grodzovsky, Andrey:
On 01/03/2019 11:20 AM, Grodzovsky, Andrey wrote:
On 01/03/2019 03:54 AM, Koenig, Christian wrote:
Am 21.12.18 um 21:36 schrieb Grodzovsky, Andrey:
On 12/21/2018 01:37 PM, Christian König wrote:
Am 20.12.18 um 20:23 schrieb Andrey Grodzovsky:
Decauple sched threads stop and start and ring mirror
list handling from the policy of what to do about the
guilty jobs.
When stoppping the sched thread and detaching sched fences
from non signaled HW fenes wait for all signaled HW fences
to complete before rerunning the jobs.
v2: Fix resubmission of guilty job into HW after refactoring.
v4:
Full restart for all the jobs, not only from guilty ring.
Extract karma increase into standalone function.
v5:
Rework waiting for signaled jobs without relying on the job
struct itself as those might already be freed for non 'guilty'
job's schedulers.
Expose karma increase to drivers.
Suggested-by: Christian Koenig
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +--
drivers/gpu/drm/etnaviv/etnaviv_sched.c | 11 +-
drivers/gpu/drm/scheduler/sched_main.c | 188
+++--
drivers/gpu/drm/v3d/v3d_sched.c | 12 +-
include/drm/gpu_scheduler.h | 10 +-
5 files changed, 151 insertions(+), 88 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 8a078f4..a4bd2d3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3298,12 +3298,10 @@ static int
amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- kthread_park(ring->sched.thread);
+ drm_sched_stop(>sched, job ? >base : NULL);
- if (job && job->base.sched != >sched)
- continue;
-
- drm_sched_hw_job_reset(>sched, job ? >base : NULL);
+ if(job)
+ drm_sched_increase_karma(>base);
Since we dropped the "job && job->base.sched != >sched" check
above this will now increase the jobs karma multiple times.
Maybe just move that outside of the loop.
/* after all hw jobs are reset, hw fence is meaningless,
so force_completion */
amdgpu_fence_driver_force_completion(ring);
@@ -3454,14 +3452,10 @@ static void
amdgpu_device_post_asic_reset(struct amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- /* only need recovery sched of the given job's ring
- * or all rings (in the case @job is NULL)
- * after above amdgpu_reset accomplished
- */
- if ((!job || job->base.sched == >sched) &&
!adev->asic_reset_res)
- drm_sched_job_recovery(>sched);
+ if (!adev->asic_reset_res)
+ drm_sched_resubmit_jobs(>sched);
- kthread_unpark(ring->sched.thread);
+ drm_sched_start(>sched, !adev->asic_reset_res);
}
if (!amdgpu_device_has_dc_support(adev)) {
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
index 49a6763..6f1268f 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
@@ -109,16 +109,19 @@ static void etnaviv_sched_timedout_job(struct
drm_sched_job *sched_job)
}
/* block scheduler */
- kthread_park(gpu->sched.thread);
- drm_sched_hw_job_reset(>sched, sched_job);
+ drm_sched_stop(>sched, sched_job);
+
+ if(sched_job)
+ drm_sched_increase_karma(sched_job);
/* get the GPU back into the init state */
etnaviv_core_dump(gpu);
etnaviv_gpu_recover_hang(gpu);
+ drm_sched_resubmit_jobs(>sched);
+
/* restart scheduler after GPU is usable again */
- drm_sched_job_recovery(>sched);
- kthread_unpark(gpu->sched.thread);
+ drm_sched_start(>sched, true);
}
static void etnaviv_sched_free_job(struct drm_sched_job *sched_job)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index dbb6906..b5c5bee 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -60,8 +60,6 @@
static void drm_sched_process_job(struct dma_fence *f, struct
dma_fence_cb *cb);
-static void drm_sched_expel_job_unlocked(struct drm_sched_job
*s_job);
-
/**
* drm_sched_rq_init - initialize a given run queue struct
*
@@ -335,6 +333,42 @@ static void drm_sched_job_timedout(struct
work_struct *work)
spin_unlock_irqrestore(>job_list_lock, flags);
}
Kernel doc here would be nice to have.
+void drm_sched_increase_karma(struct drm_sched_job *bad)
+{
+ int i;
+ struct drm_sched_entity *tmp;
+ struct drm_sched_entity *entity;
+ struct drm_gpu_scheduler *sched = bad->sched;
+
+ /*
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
On 01/03/2019 11:20 AM, Grodzovsky, Andrey wrote:
>
> On 01/03/2019 03:54 AM, Koenig, Christian wrote:
>> Am 21.12.18 um 21:36 schrieb Grodzovsky, Andrey:
>>> On 12/21/2018 01:37 PM, Christian König wrote:
Am 20.12.18 um 20:23 schrieb Andrey Grodzovsky:
> Decauple sched threads stop and start and ring mirror
> list handling from the policy of what to do about the
> guilty jobs.
> When stoppping the sched thread and detaching sched fences
> from non signaled HW fenes wait for all signaled HW fences
> to complete before rerunning the jobs.
>
> v2: Fix resubmission of guilty job into HW after refactoring.
>
> v4:
> Full restart for all the jobs, not only from guilty ring.
> Extract karma increase into standalone function.
>
> v5:
> Rework waiting for signaled jobs without relying on the job
> struct itself as those might already be freed for non 'guilty'
> job's schedulers.
> Expose karma increase to drivers.
>
> Suggested-by: Christian Koenig
> Signed-off-by: Andrey Grodzovsky
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +--
> drivers/gpu/drm/etnaviv/etnaviv_sched.c | 11 +-
> drivers/gpu/drm/scheduler/sched_main.c | 188
> +++--
> drivers/gpu/drm/v3d/v3d_sched.c | 12 +-
> include/drm/gpu_scheduler.h | 10 +-
> 5 files changed, 151 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 8a078f4..a4bd2d3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3298,12 +3298,10 @@ static int
> amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> if (!ring || !ring->sched.thread)
> continue;
> - kthread_park(ring->sched.thread);
> + drm_sched_stop(>sched, job ? >base : NULL);
> - if (job && job->base.sched != >sched)
> - continue;
> -
> - drm_sched_hw_job_reset(>sched, job ? >base : NULL);
> + if(job)
> + drm_sched_increase_karma(>base);
Since we dropped the "job && job->base.sched != >sched" check
above this will now increase the jobs karma multiple times.
Maybe just move that outside of the loop.
> /* after all hw jobs are reset, hw fence is meaningless,
> so force_completion */
> amdgpu_fence_driver_force_completion(ring);
> @@ -3454,14 +3452,10 @@ static void
> amdgpu_device_post_asic_reset(struct amdgpu_device *adev,
> if (!ring || !ring->sched.thread)
> continue;
> - /* only need recovery sched of the given job's ring
> - * or all rings (in the case @job is NULL)
> - * after above amdgpu_reset accomplished
> - */
> - if ((!job || job->base.sched == >sched) &&
> !adev->asic_reset_res)
> - drm_sched_job_recovery(>sched);
> + if (!adev->asic_reset_res)
> + drm_sched_resubmit_jobs(>sched);
> - kthread_unpark(ring->sched.thread);
> + drm_sched_start(>sched, !adev->asic_reset_res);
> }
> if (!amdgpu_device_has_dc_support(adev)) {
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> index 49a6763..6f1268f 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> @@ -109,16 +109,19 @@ static void etnaviv_sched_timedout_job(struct
> drm_sched_job *sched_job)
> }
> /* block scheduler */
> - kthread_park(gpu->sched.thread);
> - drm_sched_hw_job_reset(>sched, sched_job);
> + drm_sched_stop(>sched, sched_job);
> +
> + if(sched_job)
> + drm_sched_increase_karma(sched_job);
> /* get the GPU back into the init state */
> etnaviv_core_dump(gpu);
> etnaviv_gpu_recover_hang(gpu);
> + drm_sched_resubmit_jobs(>sched);
> +
> /* restart scheduler after GPU is usable again */
> - drm_sched_job_recovery(>sched);
> - kthread_unpark(gpu->sched.thread);
> + drm_sched_start(>sched, true);
> }
> static void etnaviv_sched_free_job(struct drm_sched_job *sched_job)
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index dbb6906..b5c5bee 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -60,8 +60,6 @@
> static void drm_sched_process_job(struct dma_fence *f, struct
> dma_fence_cb *cb);
>
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
On 01/03/2019 03:54 AM, Koenig, Christian wrote:
> Am 21.12.18 um 21:36 schrieb Grodzovsky, Andrey:
>> On 12/21/2018 01:37 PM, Christian König wrote:
>>> Am 20.12.18 um 20:23 schrieb Andrey Grodzovsky:
Decauple sched threads stop and start and ring mirror
list handling from the policy of what to do about the
guilty jobs.
When stoppping the sched thread and detaching sched fences
from non signaled HW fenes wait for all signaled HW fences
to complete before rerunning the jobs.
v2: Fix resubmission of guilty job into HW after refactoring.
v4:
Full restart for all the jobs, not only from guilty ring.
Extract karma increase into standalone function.
v5:
Rework waiting for signaled jobs without relying on the job
struct itself as those might already be freed for non 'guilty'
job's schedulers.
Expose karma increase to drivers.
Suggested-by: Christian Koenig
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +--
drivers/gpu/drm/etnaviv/etnaviv_sched.c | 11 +-
drivers/gpu/drm/scheduler/sched_main.c | 188
+++--
drivers/gpu/drm/v3d/v3d_sched.c | 12 +-
include/drm/gpu_scheduler.h | 10 +-
5 files changed, 151 insertions(+), 88 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 8a078f4..a4bd2d3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3298,12 +3298,10 @@ static int
amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- kthread_park(ring->sched.thread);
+ drm_sched_stop(>sched, job ? >base : NULL);
- if (job && job->base.sched != >sched)
- continue;
-
- drm_sched_hw_job_reset(>sched, job ? >base : NULL);
+ if(job)
+ drm_sched_increase_karma(>base);
>>> Since we dropped the "job && job->base.sched != >sched" check
>>> above this will now increase the jobs karma multiple times.
>>>
>>> Maybe just move that outside of the loop.
>>>
/* after all hw jobs are reset, hw fence is meaningless,
so force_completion */
amdgpu_fence_driver_force_completion(ring);
@@ -3454,14 +3452,10 @@ static void
amdgpu_device_post_asic_reset(struct amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- /* only need recovery sched of the given job's ring
- * or all rings (in the case @job is NULL)
- * after above amdgpu_reset accomplished
- */
- if ((!job || job->base.sched == >sched) &&
!adev->asic_reset_res)
- drm_sched_job_recovery(>sched);
+ if (!adev->asic_reset_res)
+ drm_sched_resubmit_jobs(>sched);
- kthread_unpark(ring->sched.thread);
+ drm_sched_start(>sched, !adev->asic_reset_res);
}
if (!amdgpu_device_has_dc_support(adev)) {
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
index 49a6763..6f1268f 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
@@ -109,16 +109,19 @@ static void etnaviv_sched_timedout_job(struct
drm_sched_job *sched_job)
}
/* block scheduler */
- kthread_park(gpu->sched.thread);
- drm_sched_hw_job_reset(>sched, sched_job);
+ drm_sched_stop(>sched, sched_job);
+
+ if(sched_job)
+ drm_sched_increase_karma(sched_job);
/* get the GPU back into the init state */
etnaviv_core_dump(gpu);
etnaviv_gpu_recover_hang(gpu);
+ drm_sched_resubmit_jobs(>sched);
+
/* restart scheduler after GPU is usable again */
- drm_sched_job_recovery(>sched);
- kthread_unpark(gpu->sched.thread);
+ drm_sched_start(>sched, true);
}
static void etnaviv_sched_free_job(struct drm_sched_job *sched_job)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index dbb6906..b5c5bee 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -60,8 +60,6 @@
static void drm_sched_process_job(struct dma_fence *f, struct
dma_fence_cb *cb);
-static void drm_sched_expel_job_unlocked(struct drm_sched_job
*s_job);
-
/**
* drm_sched_rq_init - initialize a given run queue struct
*
@@
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
Am 21.12.18 um 21:36 schrieb Grodzovsky, Andrey:
>
> On 12/21/2018 01:37 PM, Christian König wrote:
>> Am 20.12.18 um 20:23 schrieb Andrey Grodzovsky:
>>> Decauple sched threads stop and start and ring mirror
>>> list handling from the policy of what to do about the
>>> guilty jobs.
>>> When stoppping the sched thread and detaching sched fences
>>> from non signaled HW fenes wait for all signaled HW fences
>>> to complete before rerunning the jobs.
>>>
>>> v2: Fix resubmission of guilty job into HW after refactoring.
>>>
>>> v4:
>>> Full restart for all the jobs, not only from guilty ring.
>>> Extract karma increase into standalone function.
>>>
>>> v5:
>>> Rework waiting for signaled jobs without relying on the job
>>> struct itself as those might already be freed for non 'guilty'
>>> job's schedulers.
>>> Expose karma increase to drivers.
>>>
>>> Suggested-by: Christian Koenig
>>> Signed-off-by: Andrey Grodzovsky
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +--
>>> drivers/gpu/drm/etnaviv/etnaviv_sched.c | 11 +-
>>> drivers/gpu/drm/scheduler/sched_main.c | 188
>>> +++--
>>> drivers/gpu/drm/v3d/v3d_sched.c | 12 +-
>>> include/drm/gpu_scheduler.h | 10 +-
>>> 5 files changed, 151 insertions(+), 88 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 8a078f4..a4bd2d3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -3298,12 +3298,10 @@ static int
>>> amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>> if (!ring || !ring->sched.thread)
>>> continue;
>>> - kthread_park(ring->sched.thread);
>>> + drm_sched_stop(>sched, job ? >base : NULL);
>>> - if (job && job->base.sched != >sched)
>>> - continue;
>>> -
>>> - drm_sched_hw_job_reset(>sched, job ? >base : NULL);
>>> + if(job)
>>> + drm_sched_increase_karma(>base);
>> Since we dropped the "job && job->base.sched != >sched" check
>> above this will now increase the jobs karma multiple times.
>>
>> Maybe just move that outside of the loop.
>>
>>> /* after all hw jobs are reset, hw fence is meaningless,
>>> so force_completion */
>>> amdgpu_fence_driver_force_completion(ring);
>>> @@ -3454,14 +3452,10 @@ static void
>>> amdgpu_device_post_asic_reset(struct amdgpu_device *adev,
>>> if (!ring || !ring->sched.thread)
>>> continue;
>>> - /* only need recovery sched of the given job's ring
>>> - * or all rings (in the case @job is NULL)
>>> - * after above amdgpu_reset accomplished
>>> - */
>>> - if ((!job || job->base.sched == >sched) &&
>>> !adev->asic_reset_res)
>>> - drm_sched_job_recovery(>sched);
>>> + if (!adev->asic_reset_res)
>>> + drm_sched_resubmit_jobs(>sched);
>>> - kthread_unpark(ring->sched.thread);
>>> + drm_sched_start(>sched, !adev->asic_reset_res);
>>> }
>>> if (!amdgpu_device_has_dc_support(adev)) {
>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>>> b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>>> index 49a6763..6f1268f 100644
>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>>> @@ -109,16 +109,19 @@ static void etnaviv_sched_timedout_job(struct
>>> drm_sched_job *sched_job)
>>> }
>>> /* block scheduler */
>>> - kthread_park(gpu->sched.thread);
>>> - drm_sched_hw_job_reset(>sched, sched_job);
>>> + drm_sched_stop(>sched, sched_job);
>>> +
>>> + if(sched_job)
>>> + drm_sched_increase_karma(sched_job);
>>> /* get the GPU back into the init state */
>>> etnaviv_core_dump(gpu);
>>> etnaviv_gpu_recover_hang(gpu);
>>> + drm_sched_resubmit_jobs(>sched);
>>> +
>>> /* restart scheduler after GPU is usable again */
>>> - drm_sched_job_recovery(>sched);
>>> - kthread_unpark(gpu->sched.thread);
>>> + drm_sched_start(>sched, true);
>>> }
>>> static void etnaviv_sched_free_job(struct drm_sched_job *sched_job)
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index dbb6906..b5c5bee 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -60,8 +60,6 @@
>>> static void drm_sched_process_job(struct dma_fence *f, struct
>>> dma_fence_cb *cb);
>>> -static void drm_sched_expel_job_unlocked(struct drm_sched_job
>>> *s_job);
>>> -
>>> /**
>>> * drm_sched_rq_init - initialize a given run queue struct
>>> *
>>> @@ -335,6 +333,42 @@ static void drm_sched_job_timedout(struct
>>> work_struct *work)
>>> spin_unlock_irqrestore(>job_list_lock, flags);
>>> }
>> Kernel doc here would be nice to have.
>>
>>>
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
On 12/21/2018 01:37 PM, Christian König wrote:
> Am 20.12.18 um 20:23 schrieb Andrey Grodzovsky:
>> Decauple sched threads stop and start and ring mirror
>> list handling from the policy of what to do about the
>> guilty jobs.
>> When stoppping the sched thread and detaching sched fences
>> from non signaled HW fenes wait for all signaled HW fences
>> to complete before rerunning the jobs.
>>
>> v2: Fix resubmission of guilty job into HW after refactoring.
>>
>> v4:
>> Full restart for all the jobs, not only from guilty ring.
>> Extract karma increase into standalone function.
>>
>> v5:
>> Rework waiting for signaled jobs without relying on the job
>> struct itself as those might already be freed for non 'guilty'
>> job's schedulers.
>> Expose karma increase to drivers.
>>
>> Suggested-by: Christian Koenig
>> Signed-off-by: Andrey Grodzovsky
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +--
>> drivers/gpu/drm/etnaviv/etnaviv_sched.c | 11 +-
>> drivers/gpu/drm/scheduler/sched_main.c | 188
>> +++--
>> drivers/gpu/drm/v3d/v3d_sched.c | 12 +-
>> include/drm/gpu_scheduler.h | 10 +-
>> 5 files changed, 151 insertions(+), 88 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 8a078f4..a4bd2d3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3298,12 +3298,10 @@ static int
>> amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>> if (!ring || !ring->sched.thread)
>> continue;
>> - kthread_park(ring->sched.thread);
>> + drm_sched_stop(>sched, job ? >base : NULL);
>> - if (job && job->base.sched != >sched)
>> - continue;
>> -
>> - drm_sched_hw_job_reset(>sched, job ? >base : NULL);
>> + if(job)
>> + drm_sched_increase_karma(>base);
>
> Since we dropped the "job && job->base.sched != >sched" check
> above this will now increase the jobs karma multiple times.
>
> Maybe just move that outside of the loop.
>
>> /* after all hw jobs are reset, hw fence is meaningless,
>> so force_completion */
>> amdgpu_fence_driver_force_completion(ring);
>> @@ -3454,14 +3452,10 @@ static void
>> amdgpu_device_post_asic_reset(struct amdgpu_device *adev,
>> if (!ring || !ring->sched.thread)
>> continue;
>> - /* only need recovery sched of the given job's ring
>> - * or all rings (in the case @job is NULL)
>> - * after above amdgpu_reset accomplished
>> - */
>> - if ((!job || job->base.sched == >sched) &&
>> !adev->asic_reset_res)
>> - drm_sched_job_recovery(>sched);
>> + if (!adev->asic_reset_res)
>> + drm_sched_resubmit_jobs(>sched);
>> - kthread_unpark(ring->sched.thread);
>> + drm_sched_start(>sched, !adev->asic_reset_res);
>> }
>> if (!amdgpu_device_has_dc_support(adev)) {
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> index 49a6763..6f1268f 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> @@ -109,16 +109,19 @@ static void etnaviv_sched_timedout_job(struct
>> drm_sched_job *sched_job)
>> }
>> /* block scheduler */
>> - kthread_park(gpu->sched.thread);
>> - drm_sched_hw_job_reset(>sched, sched_job);
>> + drm_sched_stop(>sched, sched_job);
>> +
>> + if(sched_job)
>> + drm_sched_increase_karma(sched_job);
>> /* get the GPU back into the init state */
>> etnaviv_core_dump(gpu);
>> etnaviv_gpu_recover_hang(gpu);
>> + drm_sched_resubmit_jobs(>sched);
>> +
>> /* restart scheduler after GPU is usable again */
>> - drm_sched_job_recovery(>sched);
>> - kthread_unpark(gpu->sched.thread);
>> + drm_sched_start(>sched, true);
>> }
>> static void etnaviv_sched_free_job(struct drm_sched_job *sched_job)
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index dbb6906..b5c5bee 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -60,8 +60,6 @@
>> static void drm_sched_process_job(struct dma_fence *f, struct
>> dma_fence_cb *cb);
>> -static void drm_sched_expel_job_unlocked(struct drm_sched_job
>> *s_job);
>> -
>> /**
>> * drm_sched_rq_init - initialize a given run queue struct
>> *
>> @@ -335,6 +333,42 @@ static void drm_sched_job_timedout(struct
>> work_struct *work)
>> spin_unlock_irqrestore(>job_list_lock, flags);
>> }
>
> Kernel doc here would be nice to have.
>
>> +void drm_sched_increase_karma(struct drm_sched_job *bad)
>> +{
>> + int i;
>> + struct drm_sched_entity *tmp;
>> + struct drm_sched_entity *entity;
>> + struct drm_gpu_scheduler
Re: [PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
Am 20.12.18 um 20:23 schrieb Andrey Grodzovsky:
Decauple sched threads stop and start and ring mirror
list handling from the policy of what to do about the
guilty jobs.
When stoppping the sched thread and detaching sched fences
from non signaled HW fenes wait for all signaled HW fences
to complete before rerunning the jobs.
v2: Fix resubmission of guilty job into HW after refactoring.
v4:
Full restart for all the jobs, not only from guilty ring.
Extract karma increase into standalone function.
v5:
Rework waiting for signaled jobs without relying on the job
struct itself as those might already be freed for non 'guilty'
job's schedulers.
Expose karma increase to drivers.
Suggested-by: Christian Koenig
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +--
drivers/gpu/drm/etnaviv/etnaviv_sched.c| 11 +-
drivers/gpu/drm/scheduler/sched_main.c | 188 +++--
drivers/gpu/drm/v3d/v3d_sched.c| 12 +-
include/drm/gpu_scheduler.h| 10 +-
5 files changed, 151 insertions(+), 88 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 8a078f4..a4bd2d3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3298,12 +3298,10 @@ static int amdgpu_device_pre_asic_reset(struct
amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- kthread_park(ring->sched.thread);
+ drm_sched_stop(>sched, job ? >base : NULL);
- if (job && job->base.sched != >sched)
- continue;
-
- drm_sched_hw_job_reset(>sched, job ? >base : NULL);
+ if(job)
+ drm_sched_increase_karma(>base);
Since we dropped the "job && job->base.sched != >sched" check
above this will now increase the jobs karma multiple times.
Maybe just move that outside of the loop.
/* after all hw jobs are reset, hw fence is meaningless, so force_completion */
amdgpu_fence_driver_force_completion(ring);
@@ -3454,14 +3452,10 @@ static void amdgpu_device_post_asic_reset(struct
amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- /* only need recovery sched of the given job's ring
-* or all rings (in the case @job is NULL)
-* after above amdgpu_reset accomplished
-*/
- if ((!job || job->base.sched == >sched) &&
!adev->asic_reset_res)
- drm_sched_job_recovery(>sched);
+ if (!adev->asic_reset_res)
+ drm_sched_resubmit_jobs(>sched);
- kthread_unpark(ring->sched.thread);
+ drm_sched_start(>sched, !adev->asic_reset_res);
}
if (!amdgpu_device_has_dc_support(adev)) {
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
index 49a6763..6f1268f 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
@@ -109,16 +109,19 @@ static void etnaviv_sched_timedout_job(struct
drm_sched_job *sched_job)
}
/* block scheduler */
- kthread_park(gpu->sched.thread);
- drm_sched_hw_job_reset(>sched, sched_job);
+ drm_sched_stop(>sched, sched_job);
+
+ if(sched_job)
+ drm_sched_increase_karma(sched_job);
/* get the GPU back into the init state */
etnaviv_core_dump(gpu);
etnaviv_gpu_recover_hang(gpu);
+ drm_sched_resubmit_jobs(>sched);
+
/* restart scheduler after GPU is usable again */
- drm_sched_job_recovery(>sched);
- kthread_unpark(gpu->sched.thread);
+ drm_sched_start(>sched, true);
}
static void etnaviv_sched_free_job(struct drm_sched_job *sched_job)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index dbb6906..b5c5bee 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -60,8 +60,6 @@
static void drm_sched_process_job(struct dma_fence *f, struct dma_fence_cb *cb);
-static void drm_sched_expel_job_unlocked(struct drm_sched_job *s_job);
-
/**
* drm_sched_rq_init - initialize a given run queue struct
*
@@ -335,6 +333,42 @@ static void drm_sched_job_timedout(struct work_struct
*work)
spin_unlock_irqrestore(>job_list_lock, flags);
}
Kernel doc here would be nice to have.
+void drm_sched_increase_karma(struct drm_sched_job *bad)
+{
+ int i;
+ struct drm_sched_entity *tmp;
+ struct drm_sched_entity *entity;
+ struct drm_gpu_scheduler *sched = bad->sched;
+
+ /* don't increase @bad's karma if it's from KERNEL RQ,
+* because sometimes GPU hang would cause kernel jobs (like VM updating
jobs)
+* corrupt but keep in mind that kernel jobs
[PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.
Decauple sched threads stop and start and ring mirror
list handling from the policy of what to do about the
guilty jobs.
When stoppping the sched thread and detaching sched fences
from non signaled HW fenes wait for all signaled HW fences
to complete before rerunning the jobs.
v2: Fix resubmission of guilty job into HW after refactoring.
v4:
Full restart for all the jobs, not only from guilty ring.
Extract karma increase into standalone function.
v5:
Rework waiting for signaled jobs without relying on the job
struct itself as those might already be freed for non 'guilty'
job's schedulers.
Expose karma increase to drivers.
Suggested-by: Christian Koenig
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +--
drivers/gpu/drm/etnaviv/etnaviv_sched.c| 11 +-
drivers/gpu/drm/scheduler/sched_main.c | 188 +++--
drivers/gpu/drm/v3d/v3d_sched.c| 12 +-
include/drm/gpu_scheduler.h| 10 +-
5 files changed, 151 insertions(+), 88 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 8a078f4..a4bd2d3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3298,12 +3298,10 @@ static int amdgpu_device_pre_asic_reset(struct
amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- kthread_park(ring->sched.thread);
+ drm_sched_stop(>sched, job ? >base : NULL);
- if (job && job->base.sched != >sched)
- continue;
-
- drm_sched_hw_job_reset(>sched, job ? >base : NULL);
+ if(job)
+ drm_sched_increase_karma(>base);
/* after all hw jobs are reset, hw fence is meaningless, so
force_completion */
amdgpu_fence_driver_force_completion(ring);
@@ -3454,14 +3452,10 @@ static void amdgpu_device_post_asic_reset(struct
amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- /* only need recovery sched of the given job's ring
-* or all rings (in the case @job is NULL)
-* after above amdgpu_reset accomplished
-*/
- if ((!job || job->base.sched == >sched) &&
!adev->asic_reset_res)
- drm_sched_job_recovery(>sched);
+ if (!adev->asic_reset_res)
+ drm_sched_resubmit_jobs(>sched);
- kthread_unpark(ring->sched.thread);
+ drm_sched_start(>sched, !adev->asic_reset_res);
}
if (!amdgpu_device_has_dc_support(adev)) {
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
index 49a6763..6f1268f 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
@@ -109,16 +109,19 @@ static void etnaviv_sched_timedout_job(struct
drm_sched_job *sched_job)
}
/* block scheduler */
- kthread_park(gpu->sched.thread);
- drm_sched_hw_job_reset(>sched, sched_job);
+ drm_sched_stop(>sched, sched_job);
+
+ if(sched_job)
+ drm_sched_increase_karma(sched_job);
/* get the GPU back into the init state */
etnaviv_core_dump(gpu);
etnaviv_gpu_recover_hang(gpu);
+ drm_sched_resubmit_jobs(>sched);
+
/* restart scheduler after GPU is usable again */
- drm_sched_job_recovery(>sched);
- kthread_unpark(gpu->sched.thread);
+ drm_sched_start(>sched, true);
}
static void etnaviv_sched_free_job(struct drm_sched_job *sched_job)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index dbb6906..b5c5bee 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -60,8 +60,6 @@
static void drm_sched_process_job(struct dma_fence *f, struct dma_fence_cb
*cb);
-static void drm_sched_expel_job_unlocked(struct drm_sched_job *s_job);
-
/**
* drm_sched_rq_init - initialize a given run queue struct
*
@@ -335,6 +333,42 @@ static void drm_sched_job_timedout(struct work_struct
*work)
spin_unlock_irqrestore(>job_list_lock, flags);
}
+void drm_sched_increase_karma(struct drm_sched_job *bad)
+{
+ int i;
+ struct drm_sched_entity *tmp;
+ struct drm_sched_entity *entity;
+ struct drm_gpu_scheduler *sched = bad->sched;
+
+ /* don't increase @bad's karma if it's from KERNEL RQ,
+* because sometimes GPU hang would cause kernel jobs (like VM updating
jobs)
+* corrupt but keep in mind that kernel jobs always considered good.
+*/
+ if (bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) {
+ atomic_inc(>karma);
+ for (i = DRM_SCHED_PRIORITY_MIN; i < DRM_SCHED_PRIORITY_KERNEL;
+
