[PATCH v5 1/2] drm/sched: Refactor ring mirror list handling.

[Christian König]

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 at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20190116/b6a6554e/attachment.html>