[PATCH v5 6/6] drm/amdgpu: Avoid HW reset if guilty job already signaled.
Grodzovsky, Andrey
Andrey.Grodzovsky at amd.com
Tue Apr 23 14:12:34 UTC 2019
On 4/23/19 8:32 AM, Koenig, Christian wrote:
> Well you at least have to give me time till after the holidays to get
> going again :)
>
> Not sure exactly jet why we need patch number 5.
Probably you missed the mail where I pointed out a bug I found during
testing - I am reattaching the mail and the KASAN dump.
Andrey
>
> And we should probably commit patch #1 and #2.
>
> Christian.
>
> Am 22.04.19 um 13:54 schrieb Grodzovsky, Andrey:
>> Ping for patches 3, new patch 5 and patch 6.
>>
>> Andrey
>>
>> On 4/18/19 11:00 AM, Andrey Grodzovsky wrote:
>>> Also reject TDRs if another one already running.
>>>
>>> v2:
>>> Stop all schedulers across device and entire XGMI hive before
>>> force signaling HW fences.
>>> Avoid passing job_signaled to helper fnctions to keep all the decision
>>> making about skipping HW reset in one place.
>>>
>>> v3:
>>> Fix SW sched. hang after non HW reset. sched.hw_rq_count has to be balanced
>>> against it's decrement in drm_sched_stop in non HW reset case.
>>> v4: rebase
>>> v5: Revert v3 as we do it now in sceduler code.
>>>
>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 143 +++++++++++++++++++----------
>>> 1 file changed, 95 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index a0e165c..85f8792 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -3334,8 +3334,6 @@ static int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>> if (!ring || !ring->sched.thread)
>>> continue;
>>>
>>> - drm_sched_stop(&ring->sched, &job->base);
>>> -
>>> /* after all hw jobs are reset, hw fence is meaningless, so force_completion */
>>> amdgpu_fence_driver_force_completion(ring);
>>> }
>>> @@ -3343,6 +3341,7 @@ static int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
>>> if(job)
>>> drm_sched_increase_karma(&job->base);
>>>
>>> + /* Don't suspend on bare metal if we are not going to HW reset the ASIC */
>>> if (!amdgpu_sriov_vf(adev)) {
>>>
>>> if (!need_full_reset)
>>> @@ -3480,37 +3479,21 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>>> return r;
>>> }
>>>
>>> -static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev)
>>> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, bool trylock)
>>> {
>>> - int i;
>>> -
>>> - for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>>> - struct amdgpu_ring *ring = adev->rings[i];
>>> -
>>> - if (!ring || !ring->sched.thread)
>>> - continue;
>>> -
>>> - if (!adev->asic_reset_res)
>>> - drm_sched_resubmit_jobs(&ring->sched);
>>> + if (trylock) {
>>> + if (!mutex_trylock(&adev->lock_reset))
>>> + return false;
>>> + } else
>>> + mutex_lock(&adev->lock_reset);
>>>
>>> - drm_sched_start(&ring->sched, !adev->asic_reset_res);
>>> - }
>>> -
>>> - if (!amdgpu_device_has_dc_support(adev)) {
>>> - drm_helper_resume_force_mode(adev->ddev);
>>> - }
>>> -
>>> - adev->asic_reset_res = 0;
>>> -}
>>> -
>>> -static void amdgpu_device_lock_adev(struct amdgpu_device *adev)
>>> -{
>>> - mutex_lock(&adev->lock_reset);
>>> atomic_inc(&adev->gpu_reset_counter);
>>> adev->in_gpu_reset = 1;
>>> /* Block kfd: SRIOV would do it separately */
>>> if (!amdgpu_sriov_vf(adev))
>>> amdgpu_amdkfd_pre_reset(adev);
>>> +
>>> + return true;
>>> }
>>>
>>> static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
>>> @@ -3538,40 +3521,42 @@ static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
>>> int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>>> struct amdgpu_job *job)
>>> {
>>> - int r;
>>> + struct list_head device_list, *device_list_handle = NULL;
>>> + bool need_full_reset, job_signaled;
>>> struct amdgpu_hive_info *hive = NULL;
>>> - bool need_full_reset = false;
>>> struct amdgpu_device *tmp_adev = NULL;
>>> - struct list_head device_list, *device_list_handle = NULL;
>>> + int i, r = 0;
>>>
>>> + need_full_reset = job_signaled = false;
>>> INIT_LIST_HEAD(&device_list);
>>>
>>> dev_info(adev->dev, "GPU reset begin!\n");
>>>
>>> + hive = amdgpu_get_xgmi_hive(adev, false);
>>> +
>>> /*
>>> - * In case of XGMI hive disallow concurrent resets to be triggered
>>> - * by different nodes. No point also since the one node already executing
>>> - * reset will also reset all the other nodes in the hive.
>>> + * Here we trylock to avoid chain of resets executing from
>>> + * either trigger by jobs on different adevs in XGMI hive or jobs on
>>> + * different schedulers for same device while this TO handler is running.
>>> + * We always reset all schedulers for device and all devices for XGMI
>>> + * hive so that should take care of them too.
>>> */
>>> - hive = amdgpu_get_xgmi_hive(adev, 0);
>>> - if (hive && adev->gmc.xgmi.num_physical_nodes > 1 &&
>>> - !mutex_trylock(&hive->reset_lock))
>>> +
>>> + if (hive && !mutex_trylock(&hive->reset_lock)) {
>>> + DRM_INFO("Bailing on TDR for s_job:%llx, hive: %llx as another already in progress",
>>> + job->base.id, hive->hive_id);
>>> return 0;
>>> + }
>>>
>>> /* Start with adev pre asic reset first for soft reset check.*/
>>> - amdgpu_device_lock_adev(adev);
>>> - r = amdgpu_device_pre_asic_reset(adev,
>>> - job,
>>> - &need_full_reset);
>>> - if (r) {
>>> - /*TODO Should we stop ?*/
>>> - DRM_ERROR("GPU pre asic reset failed with err, %d for drm dev, %s ",
>>> - r, adev->ddev->unique);
>>> - adev->asic_reset_res = r;
>>> + if (!amdgpu_device_lock_adev(adev, !hive)) {
>>> + DRM_INFO("Bailing on TDR for s_job:%llx, as another already in progress",
>>> + job->base.id);
>>> + return 0;
>>> }
>>>
>>> /* Build list of devices to reset */
>>> - if (need_full_reset && adev->gmc.xgmi.num_physical_nodes > 1) {
>>> + if (adev->gmc.xgmi.num_physical_nodes > 1) {
>>> if (!hive) {
>>> amdgpu_device_unlock_adev(adev);
>>> return -ENODEV;
>>> @@ -3588,13 +3573,56 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>>> device_list_handle = &device_list;
>>> }
>>>
>>> + /* block all schedulers and reset given job's ring */
>>> + list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
>>> + for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>>> + struct amdgpu_ring *ring = tmp_adev->rings[i];
>>> +
>>> + if (!ring || !ring->sched.thread)
>>> + continue;
>>> +
>>> + drm_sched_stop(&ring->sched, &job->base);
>>> + }
>>> + }
>>> +
>>> +
>>> + /*
>>> + * Must check guilty signal here since after this point all old
>>> + * HW fences are force signaled.
>>> + *
>>> + * job->base holds a reference to parent fence
>>> + */
>>> + if (job && job->base.s_fence->parent &&
>>> + dma_fence_is_signaled(job->base.s_fence->parent))
>>> + job_signaled = true;
>>> +
>>> + if (!amdgpu_device_ip_need_full_reset(adev))
>>> + device_list_handle = &device_list;
>>> +
>>> + if (job_signaled) {
>>> + dev_info(adev->dev, "Guilty job already signaled, skipping HW reset");
>>> + goto skip_hw_reset;
>>> + }
>>> +
>>> +
>>> + /* Guilty job will be freed after this*/
>>> + r = amdgpu_device_pre_asic_reset(adev,
>>> + job,
>>> + &need_full_reset);
>>> + if (r) {
>>> + /*TODO Should we stop ?*/
>>> + DRM_ERROR("GPU pre asic reset failed with err, %d for drm dev, %s ",
>>> + r, adev->ddev->unique);
>>> + adev->asic_reset_res = r;
>>> + }
>>> +
>>> retry: /* Rest of adevs pre asic reset from XGMI hive. */
>>> list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
>>>
>>> if (tmp_adev == adev)
>>> continue;
>>>
>>> - amdgpu_device_lock_adev(tmp_adev);
>>> + amdgpu_device_lock_adev(tmp_adev, false);
>>> r = amdgpu_device_pre_asic_reset(tmp_adev,
>>> NULL,
>>> &need_full_reset);
>>> @@ -3618,9 +3646,28 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>>> goto retry;
>>> }
>>>
>>> +skip_hw_reset:
>>> +
>>> /* Post ASIC reset for all devs .*/
>>> list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
>>> - amdgpu_device_post_asic_reset(tmp_adev);
>>> + for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>>> + struct amdgpu_ring *ring = tmp_adev->rings[i];
>>> +
>>> + if (!ring || !ring->sched.thread)
>>> + continue;
>>> +
>>> + /* No point to resubmit jobs if we didn't HW reset*/
>>> + if (!tmp_adev->asic_reset_res && !job_signaled)
>>> + drm_sched_resubmit_jobs(&ring->sched);
>>> +
>>> + drm_sched_start(&ring->sched, !tmp_adev->asic_reset_res);
>>> + }
>>> +
>>> + if (!amdgpu_device_has_dc_support(tmp_adev) && !job_signaled) {
>>> + drm_helper_resume_force_mode(tmp_adev->ddev);
>>> + }
>>> +
>>> + tmp_adev->asic_reset_res = 0;
>>>
>>> if (r) {
>>> /* bad news, how to tell it to userspace ? */
>>> @@ -3633,7 +3680,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>>> amdgpu_device_unlock_adev(tmp_adev);
>>> }
>>>
>>> - if (hive && adev->gmc.xgmi.num_physical_nodes > 1)
>>> + if (hive)
>>> mutex_unlock(&hive->reset_lock);
>>>
>>> if (r)
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kasan.log
Type: text/x-log
Size: 3511 bytes
Desc: kasan.log
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20190423/77ad93b9/attachment-0001.bin>
-------------- next part --------------
An embedded message was scrubbed...
From: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
Subject: Re: [PATCH v3 1/5] drm/scheduler: rework job destruction
Date: Wed, 17 Apr 2019 17:06:31 -0400
Size: 12194
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20190423/77ad93b9/attachment-0001.mht>
More information about the amd-gfx
mailing list