Re: [PATCH 1/4] drm/amdgpu:don't invoke srio-gpu-reset in gpu-reset

["Christian König" <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>]

> 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.
Well there is a rather good reason, we detect that something is wrong 
much faster than waiting for the timeout.

But I agree that it was broken before as well and we can fix that later. 
Please add a code comment that this needs more work.

With that fixed feel free to add my rb on it.

Christian.

Am 08.05.2017 um 11:42 schrieb Liu, Monk:
> 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@vodafone.de]
> Sent: Monday, May 08, 2017 5:34 PM
> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@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@vodafone.de]
>> Sent: Monday, May 08, 2017 5:20 PM
>> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@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@vodafone.de]
>>> Sent: Monday, May 08, 2017 5:08 PM
>>> To: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@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@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@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