The VM fault interrupt or illegal instruction will be delivered to GPU no matter it's SR-IOV or bare-metal case, And I removed them from invoking GPU reset is due to the same reason: Don't trigger gpu reset for sriov case if possible, always beware that trigger GPU reset under SR-IOV is a heavy cost (need take full access mode on GPU, so all Other VFs will be paused for a while) Because we can always rely on TDR and HYPERVISOR to detect GPU hang and resubmit malicious jobs or even kick them out later, and the gpu reset will eventually be invoked, so there is no reason to manually and voluntarily call gpu reset under SRIOV case. BR Monk -----Original Message----- From: Christian König [mailto:deathsimple@xxxxxxxxxxx] Sent: Monday, May 08, 2017 5:34 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 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 >
