Re: [PATCH 03/11] drm/amdgpu: remove VM fault_credit handling

["Christian König" <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

This code was never used for retry faults, but only for non-retry faults 
and on HW generations older than GFX9.

Those handling where always protected by calls to "printk_ratelimit()" 
to prevent flooding the system log.

So the handling was never necessary to begin with,
Christian.

Am 30.11.18 um 22:07 schrieb Zeng, Oak:
> The credit was used to limit vm (retry) fault to be processed in each VM. If this is removed, it is possible that you get flooded interrupt storm.
>
> Even though you claimed from the commit message that, printk_ratelimit is a better solution, I didn't see you implement it in this patch. Are you planning a future patch?
>
> Thanks,
> Oak
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Christian König
> Sent: Friday, November 30, 2018 7:36 AM
> To: amd-gfx@lists.freedesktop.org
> Subject: [PATCH 03/11] drm/amdgpu: remove VM fault_credit handling
>
> printk_ratelimit() is much better suited to limit the number of reported VM faults.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 37 -------------------------  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  5 ----
>   drivers/gpu/drm/amd/amdgpu/cik_ih.c     | 18 +-----------
>   drivers/gpu/drm/amd/amdgpu/cz_ih.c      | 18 +-----------
>   drivers/gpu/drm/amd/amdgpu/iceland_ih.c | 18 +-----------
>   drivers/gpu/drm/amd/amdgpu/tonga_ih.c   | 18 +-----------
>   drivers/gpu/drm/amd/amdgpu/vega10_ih.c  |  7 ++---
>   7 files changed, 6 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 2acb9838913e..a2f149da83f8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -3057,7 +3057,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	}
>   
>   	INIT_KFIFO(vm->faults);
> -	vm->fault_credit = 16;
>   
>   	return 0;
>   
> @@ -3269,42 +3268,6 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   		amdgpu_vmid_free_reserved(adev, vm, i);  }
>   
> -/**
> - * amdgpu_vm_pasid_fault_credit - Check fault credit for given PASID
> - *
> - * @adev: amdgpu_device pointer
> - * @pasid: PASID do identify the VM
> - *
> - * This function is expected to be called in interrupt context.
> - *
> - * Returns:
> - * True if there was fault credit, false otherwise
> - */
> -bool amdgpu_vm_pasid_fault_credit(struct amdgpu_device *adev,
> -				  unsigned int pasid)
> -{
> -	struct amdgpu_vm *vm;
> -
> -	spin_lock(&adev->vm_manager.pasid_lock);
> -	vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
> -	if (!vm) {
> -		/* VM not found, can't track fault credit */
> -		spin_unlock(&adev->vm_manager.pasid_lock);
> -		return true;
> -	}
> -
> -	/* No lock needed. only accessed by IRQ handler */
> -	if (!vm->fault_credit) {
> -		/* Too many faults in this VM */
> -		spin_unlock(&adev->vm_manager.pasid_lock);
> -		return false;
> -	}
> -
> -	vm->fault_credit--;
> -	spin_unlock(&adev->vm_manager.pasid_lock);
> -	return true;
> -}
> -
>   /**
>    * amdgpu_vm_manager_init - init the VM manager
>    *
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 2a8898d19c8b..e8dcfd59fc93 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -229,9 +229,6 @@ struct amdgpu_vm {
>   	/* Up to 128 pending retry page faults */
>   	DECLARE_KFIFO(faults, u64, 128);
>   
> -	/* Limit non-retry fault storms */
> -	unsigned int		fault_credit;
> -
>   	/* Points to the KFD process VM info */
>   	struct amdkfd_process_info *process_info;
>   
> @@ -299,8 +296,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,  int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, unsigned int pasid);  void amdgpu_vm_release_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm);  void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm); -bool amdgpu_vm_pasid_fault_credit(struct amdgpu_device *adev,
> -				  unsigned int pasid);
>   void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>   			 struct list_head *validated,
>   			 struct amdgpu_bo_list_entry *entry); diff --git a/drivers/gpu/drm/amd/amdgpu/cik_ih.c b/drivers/gpu/drm/amd/amdgpu/cik_ih.c
> index b5775c6a857b..3e6c8c4067cb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/cik_ih.c
> +++ b/drivers/gpu/drm/amd/amdgpu/cik_ih.c
> @@ -237,23 +237,7 @@ static u32 cik_ih_get_wptr(struct amdgpu_device *adev)
>    */
>   static bool cik_ih_prescreen_iv(struct amdgpu_device *adev)  {
> -	u32 ring_index = adev->irq.ih.rptr >> 2;
> -	u16 pasid;
> -
> -	switch (le32_to_cpu(adev->irq.ih.ring[ring_index]) & 0xff) {
> -	case 146:
> -	case 147:
> -		pasid = le32_to_cpu(adev->irq.ih.ring[ring_index + 2]) >> 16;
> -		if (!pasid || amdgpu_vm_pasid_fault_credit(adev, pasid))
> -			return true;
> -		break;
> -	default:
> -		/* Not a VM fault */
> -		return true;
> -	}
> -
> -	adev->irq.ih.rptr += 16;
> -	return false;
> +	return true;
>   }
>   
>    /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/cz_ih.c b/drivers/gpu/drm/amd/amdgpu/cz_ih.c
> index df5ac4d85a00..447b3cbc47e5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/cz_ih.c
> +++ b/drivers/gpu/drm/amd/amdgpu/cz_ih.c
> @@ -216,23 +216,7 @@ static u32 cz_ih_get_wptr(struct amdgpu_device *adev)
>    */
>   static bool cz_ih_prescreen_iv(struct amdgpu_device *adev)  {
> -	u32 ring_index = adev->irq.ih.rptr >> 2;
> -	u16 pasid;
> -
> -	switch (le32_to_cpu(adev->irq.ih.ring[ring_index]) & 0xff) {
> -	case 146:
> -	case 147:
> -		pasid = le32_to_cpu(adev->irq.ih.ring[ring_index + 2]) >> 16;
> -		if (!pasid || amdgpu_vm_pasid_fault_credit(adev, pasid))
> -			return true;
> -		break;
> -	default:
> -		/* Not a VM fault */
> -		return true;
> -	}
> -
> -	adev->irq.ih.rptr += 16;
> -	return false;
> +	return true;
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/iceland_ih.c b/drivers/gpu/drm/amd/amdgpu/iceland_ih.c
> index cf0fc61aebe6..2b94a6d1550e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/iceland_ih.c
> +++ b/drivers/gpu/drm/amd/amdgpu/iceland_ih.c
> @@ -216,23 +216,7 @@ static u32 iceland_ih_get_wptr(struct amdgpu_device *adev)
>    */
>   static bool iceland_ih_prescreen_iv(struct amdgpu_device *adev)  {
> -	u32 ring_index = adev->irq.ih.rptr >> 2;
> -	u16 pasid;
> -
> -	switch (le32_to_cpu(adev->irq.ih.ring[ring_index]) & 0xff) {
> -	case 146:
> -	case 147:
> -		pasid = le32_to_cpu(adev->irq.ih.ring[ring_index + 2]) >> 16;
> -		if (!pasid || amdgpu_vm_pasid_fault_credit(adev, pasid))
> -			return true;
> -		break;
> -	default:
> -		/* Not a VM fault */
> -		return true;
> -	}
> -
> -	adev->irq.ih.rptr += 16;
> -	return false;
> +	return true;
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/tonga_ih.c b/drivers/gpu/drm/amd/amdgpu/tonga_ih.c
> index dcdbb4d72472..9d7b43da6acc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/tonga_ih.c
> +++ b/drivers/gpu/drm/amd/amdgpu/tonga_ih.c
> @@ -227,23 +227,7 @@ static u32 tonga_ih_get_wptr(struct amdgpu_device *adev)
>    */
>   static bool tonga_ih_prescreen_iv(struct amdgpu_device *adev)  {
> -	u32 ring_index = adev->irq.ih.rptr >> 2;
> -	u16 pasid;
> -
> -	switch (le32_to_cpu(adev->irq.ih.ring[ring_index]) & 0xff) {
> -	case 146:
> -	case 147:
> -		pasid = le32_to_cpu(adev->irq.ih.ring[ring_index + 2]) >> 16;
> -		if (!pasid || amdgpu_vm_pasid_fault_credit(adev, pasid))
> -			return true;
> -		break;
> -	default:
> -		/* Not a VM fault */
> -		return true;
> -	}
> -
> -	adev->irq.ih.rptr += 16;
> -	return false;
> +	return true;
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
> index d84b687240d1..b49290bcf109 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
> @@ -258,12 +258,9 @@ static bool vega10_ih_prescreen_iv(struct amdgpu_device *adev)
>   	if (!pasid)
>   		return true;
>   
> -	/* Not a retry fault, check fault credit */
> -	if (!(dw5 & 0x80)) {
> -		if (!amdgpu_vm_pasid_fault_credit(adev, pasid))
> -			goto ignore_iv;
> +	/* Not a retry fault */
> +	if (!(dw5 & 0x80))
>   		return true;
> -	}
>   
>   	/* Track retry faults in per-VM fault FIFO. */
>   	spin_lock(&adev->vm_manager.pasid_lock);
> --
> 2.17.1
>
> _______________________________________________
> 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