[PATCH 1/4] drm/amdgpu:don't invoke srio-gpu-reset in gpu-reset
Christian König
deathsimple at vodafone.de
Mon May 8 09:33:45 UTC 2017
Sounds good, but what do we do with the amdgpu_irq_reset_work_func?
Please note that I find that calling amdgpu_gpu_reset() here is a bad
idea in the first place.
Instead we should consider the scheduler as faulting and let the
scheduler handle that as in the same way as a job timeout.
But I'm not sure if those interrupts are actually send under SRIOV or if
the hypervisor handles them somehow.
Christian.
Am 08.05.2017 um 11:24 schrieb Liu, Monk:
> I agree with disabling debugfs for amdgpu_reset when SRIOV detected.
>
> -----Original Message-----
> From: Christian König [mailto:deathsimple at vodafone.de]
> Sent: Monday, May 08, 2017 5:20 PM
> To: Liu, Monk <Monk.Liu at amd.com>; amd-gfx at lists.freedesktop.org
> Subject: Re: [PATCH 1/4] drm/amdgpu:don't invoke srio-gpu-reset in gpu-reset
>
>> You know that gpu reset under SR-IOV will have very big impact on all other VFs ...
> Mhm, good argument. But in this case we need to give at least some warning message instead of doing nothing.
>
> Or even better disable creating the amdgpu_reste debugfs file altogether. This way nobody will wonder why using it doesn't trigger anything.
>
> Christian.
>
> Am 08.05.2017 um 11:10 schrieb Liu, Monk:
>> For SR-IOV use case, we call gpu reset under the case we have no choice ...
>>
>> So many places like debug fs shouldn't a good reason to trigger gpu
>> reset
>>
>> You know that gpu reset under SR-IOV will have very big impact on all other VFs ...
>>
>> BR Monk
>>
>> -----Original Message-----
>> From: Christian König [mailto:deathsimple at vodafone.de]
>> Sent: Monday, May 08, 2017 5:08 PM
>> To: Liu, Monk <Monk.Liu at amd.com>; amd-gfx at lists.freedesktop.org
>> Subject: Re: [PATCH 1/4] drm/amdgpu:don't invoke srio-gpu-reset in
>> gpu-reset
>>
>> Am 08.05.2017 um 08:51 schrieb Monk Liu:
>>> because we don't want to do sriov-gpu-reset under certain cases, so
>>> just split those two funtion and don't invoke sr-iov one from
>>> bare-metal one.
>>>
>>> Change-Id: I641126c241e2ee2dfd54e6d16c389b159f99cfe0
>>> Signed-off-by: Monk Liu <Monk.Liu at amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 3 ++-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 +-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 3 ++-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 6 +++++-
>>> 5 files changed, 10 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 45a60a6..4985a7e 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -2652,9 +2652,6 @@ int amdgpu_gpu_reset(struct amdgpu_device *adev)
>>> int resched;
>>> bool need_full_reset;
>>>
>>> - if (amdgpu_sriov_vf(adev))
>>> - return amdgpu_sriov_gpu_reset(adev, true);
>>> -
>>> if (!amdgpu_check_soft_reset(adev)) {
>>> DRM_INFO("No hardware hang detected. Did some blocks stall?\n");
>>> return 0;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> index 5772ef2..d7523d1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> @@ -651,7 +651,8 @@ static int amdgpu_debugfs_gpu_reset(struct seq_file *m, void *data)
>>> struct amdgpu_device *adev = dev->dev_private;
>>>
>>> seq_printf(m, "gpu reset\n");
>>> - amdgpu_gpu_reset(adev);
>>> + if (!amdgpu_sriov_vf(adev))
>>> + amdgpu_gpu_reset(adev);
>> Well that is clearly not a good idea. Why do you want to disable the reset here?
>>
>>>
>>> return 0;
>>> }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index 67be795..5bcbea0 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -221,7 +221,7 @@ void amdgpu_gem_object_close(struct
>>> drm_gem_object *obj,
>>>
>>> static int amdgpu_gem_handle_lockup(struct amdgpu_device *adev, int r)
>>> {
>>> - if (r == -EDEADLK) {
>>> + if (r == -EDEADLK && !amdgpu_sriov_vf(adev)) {
>> Not a problem of your patch, but that stuff is outdated and should have been removed completely years ago. Going to take care of that.
>>
>>> r = amdgpu_gpu_reset(adev);
>>> if (!r)
>>> r = -EAGAIN;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>>> index f8a6c95..49c6e6e 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>>> @@ -89,7 +89,8 @@ static void amdgpu_irq_reset_work_func(struct work_struct *work)
>>> struct amdgpu_device *adev = container_of(work, struct amdgpu_device,
>>> reset_work);
>>>
>>> - amdgpu_gpu_reset(adev);
>>> + if (!amdgpu_sriov_vf(adev))
>>> + amdgpu_gpu_reset(adev);
>> Mhm, that disables the reset on an invalid register access or invalid command stream. Is that really what we want?
>>
>> Christian.
>>
>>> }
>>>
>>> /* Disable *all* interrupts */
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> index 690ef3d..c7718af 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> @@ -36,7 +36,11 @@ static void amdgpu_job_timedout(struct amd_sched_job *s_job)
>>> job->base.sched->name,
>>> atomic_read(&job->ring->fence_drv.last_seq),
>>> job->ring->fence_drv.sync_seq);
>>> - amdgpu_gpu_reset(job->adev);
>>> +
>>> + if (amdgpu_sriov_vf(job->adev))
>>> + amdgpu_sriov_gpu_reset(job->adev, true);
>>> + else
>>> + amdgpu_gpu_reset(job->adev);
>>> }
>>>
>>> int amdgpu_job_alloc(struct amdgpu_device *adev, unsigned num_ibs,
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
More information about the amd-gfx
mailing list