[PATCH 1/8] drm/amdgpu: fix ATC handling for Ryzen

[PATCH 1/8] drm/amdgpu: fix ATC handling for Ryzen

[Christian König]

Otherwise we don't correctly use translate further.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 3d221f044183..059d9802e713 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -767,14 +767,17 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
 
 	addr = 0;
 	if (ats_entries) {
-		uint64_t ats_value;
+		uint64_t value = 0, flags;
 
-		ats_value = AMDGPU_PTE_DEFAULT_ATC;
-		if (level != AMDGPU_VM_PTB)
-			ats_value |= AMDGPU_PDE_PTE;
+		flags = AMDGPU_PTE_DEFAULT_ATC;
+		if (level != AMDGPU_VM_PTB) {
+			/* Handle leaf PDEs as PTEs */
+			flags |= AMDGPU_PDE_PTE;
+			amdgpu_gmc_get_vm_pde(adev, level, &value, &flags);
+		}
 
 		r = vm->update_funcs->update(&params, bo, addr, 0, ats_entries,
-					     0, ats_value);
+					     value, flags);
 		if (r)
 			return r;
 
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/8] drm/amdgpu: handle leaf PDEs as PTEs on Vega

[Christian König]

This way we get retry faults for missing PDs.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 059d9802e713..c54181a713a8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -785,15 +785,22 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
 	}
 
 	if (entries) {
-		uint64_t value = 0;
-
-		/* Workaround for fault priority problem on GMC9 */
-		if (level == AMDGPU_VM_PTB &&
-		    adev->asic_type >= CHIP_VEGA10)
-			value = AMDGPU_PTE_EXECUTABLE;
+		uint64_t value = 0, flags = 0;
+
+		if (adev->asic_type >= CHIP_VEGA10) {
+			if (level != AMDGPU_VM_PTB) {
+				/* Handle leaf PDEs as PTEs */
+				flags |= AMDGPU_PDE_PTE;
+				amdgpu_gmc_get_vm_pde(adev, level,
+						      &value, &flags);
+			} else {
+				/* Workaround for fault priority problem on GMC9 */
+				flags = AMDGPU_PTE_EXECUTABLE;
+			}
+		}
 
 		r = vm->update_funcs->update(&params, bo, addr, 0, entries,
-					     0, value);
+					     value, flags);
 		if (r)
 			return r;
 	}
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/8] drm/amdgpu: provide the page fault queue to the VM code

[Christian König]

We are going to need that for recoverable page faults.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 1 +
 drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index beac15bca526..91baf95212a6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -303,6 +303,7 @@ struct amdgpu_vm_manager {
 	const struct amdgpu_vm_pte_funcs	*vm_pte_funcs;
 	struct drm_sched_rq			*vm_pte_rqs[AMDGPU_MAX_RINGS];
 	unsigned				vm_pte_num_rqs;
+	struct amdgpu_ring			*page_fault;
 
 	/* partial resident texture handling */
 	spinlock_t				prt_lock;
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
index 8691b621148e..44f4845dacf4 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
@@ -2292,6 +2292,7 @@ static void sdma_v4_0_set_vm_pte_funcs(struct amdgpu_device *adev)
 				&sched->sched_rq[DRM_SCHED_PRIORITY_KERNEL];
 		}
 		adev->vm_manager.vm_pte_num_rqs = adev->sdma.num_instances - 1;
+		adev->vm_manager.page_fault = &adev->sdma.instance[0].page;
 	} else {
 		for (i = 0; i < adev->sdma.num_instances; i++) {
 			sched = &adev->sdma.instance[i].ring.sched;
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/8] drm/amdgpu: provide the page fault queue to the VM code

[Alex Deucher]

On Fri, Mar 29, 2019 at 6:45 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> We are going to need that for recoverable page faults.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 1 +
>  drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index beac15bca526..91baf95212a6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -303,6 +303,7 @@ struct amdgpu_vm_manager {
>         const struct amdgpu_vm_pte_funcs        *vm_pte_funcs;
>         struct drm_sched_rq                     *vm_pte_rqs[AMDGPU_MAX_RINGS];
>         unsigned                                vm_pte_num_rqs;
> +       struct amdgpu_ring                      *page_fault;
>
>         /* partial resident texture handling */
>         spinlock_t                              prt_lock;
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> index 8691b621148e..44f4845dacf4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> @@ -2292,6 +2292,7 @@ static void sdma_v4_0_set_vm_pte_funcs(struct amdgpu_device *adev)
>                                 &sched->sched_rq[DRM_SCHED_PRIORITY_KERNEL];
>                 }
>                 adev->vm_manager.vm_pte_num_rqs = adev->sdma.num_instances - 1;
> +               adev->vm_manager.page_fault = &adev->sdma.instance[0].page;

What about asics that don't have the page queue?  Shouldn't we handle
them as well?

Alex

>         } else {
>                 for (i = 0; i < adev->sdma.num_instances; i++) {
>                         sched = &adev->sdma.instance[i].ring.sched;
> --
> 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

Re: [PATCH 3/8] drm/amdgpu: provide the page fault queue to the VM code

[Christian König]

Am 29.03.19 um 15:23 schrieb Alex Deucher:
> On Fri, Mar 29, 2019 at 6:45 AM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>> We are going to need that for recoverable page faults.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 1 +
>>   drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 1 +
>>   2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index beac15bca526..91baf95212a6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -303,6 +303,7 @@ struct amdgpu_vm_manager {
>>          const struct amdgpu_vm_pte_funcs        *vm_pte_funcs;
>>          struct drm_sched_rq                     *vm_pte_rqs[AMDGPU_MAX_RINGS];
>>          unsigned                                vm_pte_num_rqs;
>> +       struct amdgpu_ring                      *page_fault;
>>
>>          /* partial resident texture handling */
>>          spinlock_t                              prt_lock;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>> index 8691b621148e..44f4845dacf4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>> @@ -2292,6 +2292,7 @@ static void sdma_v4_0_set_vm_pte_funcs(struct amdgpu_device *adev)
>>                                  &sched->sched_rq[DRM_SCHED_PRIORITY_KERNEL];
>>                  }
>>                  adev->vm_manager.vm_pte_num_rqs = adev->sdma.num_instances - 1;
>> +               adev->vm_manager.page_fault = &adev->sdma.instance[0].page;
> What about asics that don't have the page queue?  Shouldn't we handle
> them as well?

In this case the entry should just be NULL indicating that we don't have 
a page fault queue.

I just didn't see the point to explicitly set it to NULL on older ASICs.

Christian.

>
> Alex
>
>>          } else {
>>                  for (i = 0; i < adev->sdma.num_instances; i++) {
>>                          sched = &adev->sdma.instance[i].ring.sched;
>> --
>> 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

[PATCH 4/8] drm/amdgpu: allow direct submission in the VM backends

[Christian König]

This allows us to update page tables directly while in a page fault.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  5 ++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c  |  4 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 29 +++++++++++++--------
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 91baf95212a6..66e27b941a55 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -191,6 +191,11 @@ struct amdgpu_vm_update_params {
 	 */
 	struct amdgpu_vm *vm;
 
+	/**
+	 * @direct: if changes should be made directly
+	 */
+	bool direct;
+
 	/**
 	 * @pages_addr:
 	 *
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
index 5222d165abfc..f94e4896079c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
@@ -49,6 +49,10 @@ static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p, void *owner,
 {
 	int r;
 
+	/* Don't wait for anything during page fault */
+	if (p->direct)
+		return 0;
+
 	/* Wait for PT BOs to be idle. PTs share the same resv. object
 	 * as the root PD BO
 	 */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
index ddd181f5ed37..891d597063cb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
@@ -68,17 +68,17 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
 	if (r)
 		return r;
 
-	r = amdgpu_sync_fence(p->adev, &p->job->sync, exclusive, false);
-	if (r)
-		return r;
+	p->num_dw_left = ndw;
+
+	if (p->direct)
+		return 0;
 
-	r = amdgpu_sync_resv(p->adev, &p->job->sync, root->tbo.resv,
-			     owner, false);
+	r = amdgpu_sync_fence(p->adev, &p->job->sync, exclusive, false);
 	if (r)
 		return r;
 
-	p->num_dw_left = ndw;
-	return 0;
+	return amdgpu_sync_resv(p->adev, &p->job->sync, root->tbo.resv,
+				owner, false);
 }
 
 /**
@@ -99,13 +99,21 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
 	struct dma_fence *f;
 	int r;
 
-	ring = container_of(p->vm->entity.rq->sched, struct amdgpu_ring, sched);
+	if (p->direct)
+		ring = p->adev->vm_manager.page_fault;
+	else
+		ring = container_of(p->vm->entity.rq->sched,
+				    struct amdgpu_ring, sched);
 
 	WARN_ON(ib->length_dw == 0);
 	amdgpu_ring_pad_ib(ring, ib);
 	WARN_ON(ib->length_dw > p->num_dw_left);
-	r = amdgpu_job_submit(p->job, &p->vm->entity,
-			      AMDGPU_FENCE_OWNER_VM, &f);
+
+	if (p->direct)
+		r = amdgpu_job_submit_direct(p->job, ring, &f);
+	else
+		r = amdgpu_job_submit(p->job, &p->vm->entity,
+				      AMDGPU_FENCE_OWNER_VM, &f);
 	if (r)
 		goto error;
 
@@ -120,7 +128,6 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
 	return r;
 }
 
-
 /**
  * amdgpu_vm_sdma_copy_ptes - copy the PTEs from mapping
  *
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 5/8] drm/amdgpu: allow direct submission of PDE updates.

[Christian König]

For handling PDE updates directly in the fault handler.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c           | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c          | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c           | 8 +++++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h           | 4 ++--
 5 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 047bba8c62d6..9b54b9edc020 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -347,7 +347,7 @@ static int vm_update_pds(struct amdgpu_vm *vm, struct amdgpu_sync *sync)
 	struct amdgpu_device *adev = amdgpu_ttm_adev(pd->tbo.bdev);
 	int ret;
 
-	ret = amdgpu_vm_update_directories(adev, vm);
+	ret = amdgpu_vm_update_pdes(adev, vm, false);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 545302d0955f..d61f6d80aae8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -913,7 +913,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 	if (r)
 		return r;
 
-	r = amdgpu_vm_update_directories(adev, vm);
+	r = amdgpu_vm_update_pdes(adev, vm, false);
 	if (r)
 		return r;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 9ee8d7a3c6d4..ed433bae412d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -521,7 +521,7 @@ static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
 			goto error;
 	}
 
-	r = amdgpu_vm_update_directories(adev, vm);
+	r = amdgpu_vm_update_pdes(adev, vm, false);
 
 error:
 	if (r && r != -ERESTARTSYS)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index c54181a713a8..b49be71ae65a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1219,18 +1219,19 @@ static void amdgpu_vm_invalidate_pds(struct amdgpu_device *adev,
 }
 
 /*
- * amdgpu_vm_update_directories - make sure that all directories are valid
+ * amdgpu_vm_update_ - make sure that all directories are valid
  *
  * @adev: amdgpu_device pointer
  * @vm: requested vm
+ * @direct: submit directly to the paging queue
  *
  * Makes sure all directories are up to date.
  *
  * Returns:
  * 0 for success, error for failure.
  */
-int amdgpu_vm_update_directories(struct amdgpu_device *adev,
-				 struct amdgpu_vm *vm)
+int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
+			  struct amdgpu_vm *vm, bool direct)
 {
 	struct amdgpu_vm_update_params params;
 	int r;
@@ -1241,6 +1242,7 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
 	memset(&params, 0, sizeof(params));
 	params.adev = adev;
 	params.vm = vm;
+	params.direct = direct;
 
 	r = vm->update_funcs->prepare(&params, AMDGPU_FENCE_OWNER_VM, NULL);
 	if (r)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 66e27b941a55..2c98f608a604 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -355,8 +355,8 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 			      int (*callback)(void *p, struct amdgpu_bo *bo),
 			      void *param);
 int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
-int amdgpu_vm_update_directories(struct amdgpu_device *adev,
-				 struct amdgpu_vm *vm);
+int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
+			  struct amdgpu_vm *vm, bool direct);
 int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
 			  struct amdgpu_vm *vm,
 			  struct dma_fence **fence);
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 6/8] drm/amdgpu: allow direct submission of PTE updates

[Christian König]

For handling PTE updates directly in the fault handler.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index b49be71ae65a..cca4a67b0147 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1484,13 +1484,14 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
  * amdgpu_vm_bo_update_mapping - update a mapping in the vm page table
  *
  * @adev: amdgpu_device pointer
- * @exclusive: fence we need to sync to
- * @pages_addr: DMA addresses to use for mapping
  * @vm: requested vm
+ * @direct: direct submission in a page fault
+ * @exclusive: fence we need to sync to
  * @start: start of mapped range
  * @last: last mapped entry
  * @flags: flags for the entries
  * @addr: addr to set the area to
+ * @pages_addr: DMA addresses to use for mapping
  * @fence: optional resulting fence
  *
  * Fill in the page table entries between @start and @last.
@@ -1499,11 +1500,11 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
  * 0 for success, -EINVAL for failure.
  */
 static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
+				       struct amdgpu_vm *vm, bool direct,
 				       struct dma_fence *exclusive,
-				       dma_addr_t *pages_addr,
-				       struct amdgpu_vm *vm,
 				       uint64_t start, uint64_t last,
 				       uint64_t flags, uint64_t addr,
+				       dma_addr_t *pages_addr,
 				       struct dma_fence **fence)
 {
 	struct amdgpu_vm_update_params params;
@@ -1513,6 +1514,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	memset(&params, 0, sizeof(params));
 	params.adev = adev;
 	params.vm = vm;
+	params.direct = direct;
 	params.pages_addr = pages_addr;
 
 	/* sync to everything except eviction fences on unmapping */
@@ -1634,9 +1636,9 @@ static int amdgpu_vm_bo_split_mapping(struct amdgpu_device *adev,
 		}
 
 		last = min((uint64_t)mapping->last, start + max_entries - 1);
-		r = amdgpu_vm_bo_update_mapping(adev, exclusive, dma_addr, vm,
+		r = amdgpu_vm_bo_update_mapping(adev, vm, false, exclusive,
 						start, last, flags, addr,
-						fence);
+						dma_addr, fence);
 		if (r)
 			return r;
 
@@ -1930,9 +1932,9 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
 		    mapping->start < AMDGPU_GMC_HOLE_START)
 			init_pte_value = AMDGPU_PTE_DEFAULT_ATC;
 
-		r = amdgpu_vm_bo_update_mapping(adev, NULL, NULL, vm,
+		r = amdgpu_vm_bo_update_mapping(adev, vm, false, NULL,
 						mapping->start, mapping->last,
-						init_pte_value, 0, &f);
+						init_pte_value, 0, NULL, &f);
 		amdgpu_vm_free_mapping(adev, vm, mapping, f);
 		if (r) {
 			dma_fence_put(f);
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 7/8] drm/amdgpu: allow direct submission of clears

[Christian König]

For handling PD/PT clears directly in the fault handler.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index cca4a67b0147..890fc2cd4a0f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -693,6 +693,7 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm)
  * @adev: amdgpu_device pointer
  * @vm: VM to clear BO from
  * @bo: BO to clear
+ * @direct: use a direct update
  *
  * Root PD needs to be reserved when calling this.
  *
@@ -701,7 +702,8 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm)
  */
 static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
 			      struct amdgpu_vm *vm,
-			      struct amdgpu_bo *bo)
+			      struct amdgpu_bo *bo,
+			      bool direct)
 {
 	struct ttm_operation_ctx ctx = { true, false };
 	unsigned level = adev->vm_manager.root_level;
@@ -760,6 +762,7 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
 	memset(&params, 0, sizeof(params));
 	params.adev = adev;
 	params.vm = vm;
+	params.direct = direct;
 
 	r = vm->update_funcs->prepare(&params, AMDGPU_FENCE_OWNER_KFD, NULL);
 	if (r)
@@ -850,7 +853,8 @@ static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
  */
 static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
 			       struct amdgpu_vm *vm,
-			       struct amdgpu_vm_pt_cursor *cursor)
+			       struct amdgpu_vm_pt_cursor *cursor,
+			       bool direct)
 {
 	struct amdgpu_vm_pt *entry = cursor->entry;
 	struct amdgpu_bo_param bp;
@@ -883,7 +887,7 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
 	pt->parent = amdgpu_bo_ref(cursor->parent->base.bo);
 	amdgpu_vm_bo_base_init(&entry->base, vm, pt);
 
-	r = amdgpu_vm_clear_bo(adev, vm, pt);
+	r = amdgpu_vm_clear_bo(adev, vm, pt, direct);
 	if (r)
 		goto error_free_pt;
 
@@ -1393,7 +1397,8 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 		uint64_t incr, entry_end, pe_start;
 		struct amdgpu_bo *pt;
 
-		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor);
+		r = amdgpu_vm_alloc_pts(params->adev, params->vm, &cursor,
+					params->direct);
 		if (r)
 			return r;
 
@@ -2720,7 +2725,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 
 	amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
 
-	r = amdgpu_vm_clear_bo(adev, vm, root);
+	r = amdgpu_vm_clear_bo(adev, vm, root, false);
 	if (r)
 		goto error_unreserve;
 
@@ -2810,7 +2815,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, uns
 	 */
 	if (pte_support_ats != vm->pte_support_ats) {
 		vm->pte_support_ats = pte_support_ats;
-		r = amdgpu_vm_clear_bo(adev, vm, vm->root.base.bo);
+		r = amdgpu_vm_clear_bo(adev, vm, vm->root.base.bo, false);
 		if (r)
 			goto free_idr;
 	}
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 8/8] drm/amdgpu: add graceful VM fault handling

[Christian König]

Next step towards HMM support. For now just silence the retry fault and
optionally redirect the request to the dummy page.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 59 ++++++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  2 +
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  |  4 ++
 3 files changed, 65 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 890fc2cd4a0f..c76c913b6249 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -3079,3 +3079,62 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
 		}
 	}
 }
+
+/**
+ * amdgpu_vm_handle_fault - graceful handling of VM faults.
+ * @adev: amdgpu device pointer
+ * @pasid: PASID of the VM
+ * @addr: Address of the fault
+ *
+ * Try to gracefully handle a VM fault. Return true if the fault was handled and
+ * shouldn't be reported any more.
+ */
+bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid,
+			    uint64_t addr)
+{
+	struct amdgpu_ring *ring = &adev->sdma.instance[0].page;
+	uint64_t value, flags;
+	struct amdgpu_vm *vm;
+	long r;
+
+	if (!ring->sched.ready)
+		return false;
+
+	spin_lock(&adev->vm_manager.pasid_lock);
+	vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
+	spin_unlock(&adev->vm_manager.pasid_lock);
+
+	if (!vm)
+		return false;
+
+	r = amdgpu_bo_reserve(vm->root.base.bo, true);
+	if (r)
+		return false;
+
+	addr /= AMDGPU_GPU_PAGE_SIZE;
+	flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SNOOPED |
+		AMDGPU_PTE_SYSTEM;
+
+	if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) {
+		/* Redirect the access to the dummy page */
+		value = adev->dummy_page_addr;
+		flags |= AMDGPU_PTE_EXECUTABLE | AMDGPU_PTE_READABLE |
+			AMDGPU_PTE_WRITEABLE;
+	} else {
+		value = 0;
+	}
+
+	r = amdgpu_vm_bo_update_mapping(adev, vm, true, NULL, addr, addr + 1,
+					flags, value, NULL, NULL);
+	if (r)
+		goto error;
+
+	r = amdgpu_vm_update_pdes(adev, vm, true);
+
+error:
+	amdgpu_bo_unreserve(vm->root.base.bo);
+	if (r < 0)
+		DRM_ERROR("Can't handle page fault (%ld)\n", r);
+
+	return false;
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 2c98f608a604..e9c022300d29 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -402,6 +402,8 @@ void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev);
 
 void amdgpu_vm_get_task_info(struct amdgpu_device *adev, unsigned int pasid,
 			     struct amdgpu_task_info *task_info);
+bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, unsigned int pasid,
+			    uint64_t addr);
 
 void amdgpu_vm_set_task_info(struct amdgpu_vm *vm);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index e00fef6962da..cc11c0efb586 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -318,6 +318,10 @@ static int gmc_v9_0_process_interrupt(struct amdgpu_device *adev,
 		return 1; /* This also prevents sending it to KFD */
 
 	/* If it's the first fault for this address, process it normally */
+	if (retry_fault && !in_interrupt() &&
+	    amdgpu_vm_handle_fault(adev, entry->pasid, addr))
+		return 1; /* This also prevents sending it to KFD */
+
 	if (!amdgpu_sriov_vf(adev)) {
 		status = RREG32(hub->vm_l2_pro_fault_status);
 		WREG32_P(hub->vm_l2_pro_fault_cntl, 1, ~1);
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/8] drm/amdgpu: fix ATC handling for Ryzen

[Kuehling, Felix]

Patches 1-3 are Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

About the direct mode, that removes a bunch of synchronization, so it 
must make some assumptions about the state of the page tables. What 
makes that safe? Is it safe to use direct-mode on a 
per-page-table-update basis? Or do all page table updates have to go 
through direct mode to avoid hazards? If yes, then maybe this should be 
a property of the VM rather than a parameter that gets passed to a bunch 
of function calls.

Regards,
   Felix

On 2019-03-29 6:45 a.m., Christian König wrote:
> Otherwise we don't correctly use translate further.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 3d221f044183..059d9802e713 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -767,14 +767,17 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>   
>   	addr = 0;
>   	if (ats_entries) {
> -		uint64_t ats_value;
> +		uint64_t value = 0, flags;
>   
> -		ats_value = AMDGPU_PTE_DEFAULT_ATC;
> -		if (level != AMDGPU_VM_PTB)
> -			ats_value |= AMDGPU_PDE_PTE;
> +		flags = AMDGPU_PTE_DEFAULT_ATC;
> +		if (level != AMDGPU_VM_PTB) {
> +			/* Handle leaf PDEs as PTEs */
> +			flags |= AMDGPU_PDE_PTE;
> +			amdgpu_gmc_get_vm_pde(adev, level, &value, &flags);
> +		}
>   
>   		r = vm->update_funcs->update(&params, bo, addr, 0, ats_entries,
> -					     0, ats_value);
> +					     value, flags);
>   		if (r)
>   			return r;
>   
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/8] drm/amdgpu: fix ATC handling for Ryzen

[Christian König]

Am 30.03.19 um 01:41 schrieb Kuehling, Felix:
> Patches 1-3 are Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

Thanks.

>
> About the direct mode, that removes a bunch of synchronization, so it
> must make some assumptions about the state of the page tables. What
> makes that safe?

Direct mode is only supposed to be used during page fault handling.

E.g. we know that the page tables are in the correct place in this 
situation because the hardware is hammering on a PTE and waiting for it 
to become valid.

Christian.

>   Is it safe to use direct-mode on a
> per-page-table-update basis? Or do all page table updates have to go
> through direct mode to avoid hazards? If yes, then maybe this should be
> a property of the VM rather than a parameter that gets passed to a bunch
> of function calls.
>
> Regards,
>     Felix
>
> On 2019-03-29 6:45 a.m., Christian König wrote:
>> Otherwise we don't correctly use translate further.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++-----
>>    1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 3d221f044183..059d9802e713 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -767,14 +767,17 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>>    
>>    	addr = 0;
>>    	if (ats_entries) {
>> -		uint64_t ats_value;
>> +		uint64_t value = 0, flags;
>>    
>> -		ats_value = AMDGPU_PTE_DEFAULT_ATC;
>> -		if (level != AMDGPU_VM_PTB)
>> -			ats_value |= AMDGPU_PDE_PTE;
>> +		flags = AMDGPU_PTE_DEFAULT_ATC;
>> +		if (level != AMDGPU_VM_PTB) {
>> +			/* Handle leaf PDEs as PTEs */
>> +			flags |= AMDGPU_PDE_PTE;
>> +			amdgpu_gmc_get_vm_pde(adev, level, &value, &flags);
>> +		}
>>    
>>    		r = vm->update_funcs->update(&params, bo, addr, 0, ats_entries,
>> -					     0, ats_value);
>> +					     value, flags);
>>    		if (r)
>>    			return r;
>>    

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/8] drm/amdgpu: fix ATC handling for Ryzen

[Kuehling, Felix]

On 2019-04-01 7:23 a.m., Christian König wrote:
> Am 30.03.19 um 01:41 schrieb Kuehling, Felix:
>> Patches 1-3 are Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>
> Thanks.
>
>>
>> About the direct mode, that removes a bunch of synchronization, so it
>> must make some assumptions about the state of the page tables. What
>> makes that safe?
>
> Direct mode is only supposed to be used during page fault handling.
>
> E.g. we know that the page tables are in the correct place in this 
> situation because the hardware is hammering on a PTE and waiting for 
> it to become valid.

A fence could also indicate a concurrent modification of the page table. 
For example a PTB may be allocated and initialized concurrently, not in 
direct mode. Would direct mode need to wait for a fence that indicates 
completion of the PTB initialization? Or do we have some way to ensure 
such concurrent allocation and initialization of a PTB cannot happen?

Regards,
   Felix


>
> Christian.
>
>>   Is it safe to use direct-mode on a
>> per-page-table-update basis? Or do all page table updates have to go
>> through direct mode to avoid hazards? If yes, then maybe this should be
>> a property of the VM rather than a parameter that gets passed to a bunch
>> of function calls.
>>
>> Regards,
>>     Felix
>>
>> On 2019-03-29 6:45 a.m., Christian König wrote:
>>> Otherwise we don't correctly use translate further.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++-----
>>>    1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 3d221f044183..059d9802e713 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -767,14 +767,17 @@ static int amdgpu_vm_clear_bo(struct 
>>> amdgpu_device *adev,
>>>           addr = 0;
>>>        if (ats_entries) {
>>> -        uint64_t ats_value;
>>> +        uint64_t value = 0, flags;
>>>    -        ats_value = AMDGPU_PTE_DEFAULT_ATC;
>>> -        if (level != AMDGPU_VM_PTB)
>>> -            ats_value |= AMDGPU_PDE_PTE;
>>> +        flags = AMDGPU_PTE_DEFAULT_ATC;
>>> +        if (level != AMDGPU_VM_PTB) {
>>> +            /* Handle leaf PDEs as PTEs */
>>> +            flags |= AMDGPU_PDE_PTE;
>>> +            amdgpu_gmc_get_vm_pde(adev, level, &value, &flags);
>>> +        }
>>>               r = vm->update_funcs->update(&params, bo, addr, 0, 
>>> ats_entries,
>>> -                         0, ats_value);
>>> +                         value, flags);
>>>            if (r)
>>>                return r;
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/8] drm/amdgpu: fix ATC handling for Ryzen

[Christian König]

Am 01.04.19 um 19:59 schrieb Kuehling, Felix:
> On 2019-04-01 7:23 a.m., Christian König wrote:
>> Am 30.03.19 um 01:41 schrieb Kuehling, Felix:
>>> Patches 1-3 are Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>> Thanks.
>>
>>> About the direct mode, that removes a bunch of synchronization, so it
>>> must make some assumptions about the state of the page tables. What
>>> makes that safe?
>> Direct mode is only supposed to be used during page fault handling.
>>
>> E.g. we know that the page tables are in the correct place in this
>> situation because the hardware is hammering on a PTE and waiting for
>> it to become valid.
> A fence could also indicate a concurrent modification of the page table.
> For example a PTB may be allocated and initialized concurrently, not in
> direct mode. Would direct mode need to wait for a fence that indicates
> completion of the PTB initialization? Or do we have some way to ensure
> such concurrent allocation and initialization of a PTB cannot happen?

Yeah, that is a very good question I haven't solved yet either.

My currently best idea is to separate the address space, e.g. use the 
lower address space for on demand paging and the higher with classic 
pre-filled page tables for the MM and display engines.

Christian.

>
> Regards,
>     Felix
>
>
>> Christian.
>>
>>>    Is it safe to use direct-mode on a
>>> per-page-table-update basis? Or do all page table updates have to go
>>> through direct mode to avoid hazards? If yes, then maybe this should be
>>> a property of the VM rather than a parameter that gets passed to a bunch
>>> of function calls.
>>>
>>> Regards,
>>>      Felix
>>>
>>> On 2019-03-29 6:45 a.m., Christian König wrote:
>>>> Otherwise we don't correctly use translate further.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++-----
>>>>     1 file changed, 8 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 3d221f044183..059d9802e713 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -767,14 +767,17 @@ static int amdgpu_vm_clear_bo(struct
>>>> amdgpu_device *adev,
>>>>            addr = 0;
>>>>         if (ats_entries) {
>>>> -        uint64_t ats_value;
>>>> +        uint64_t value = 0, flags;
>>>>     -        ats_value = AMDGPU_PTE_DEFAULT_ATC;
>>>> -        if (level != AMDGPU_VM_PTB)
>>>> -            ats_value |= AMDGPU_PDE_PTE;
>>>> +        flags = AMDGPU_PTE_DEFAULT_ATC;
>>>> +        if (level != AMDGPU_VM_PTB) {
>>>> +            /* Handle leaf PDEs as PTEs */
>>>> +            flags |= AMDGPU_PDE_PTE;
>>>> +            amdgpu_gmc_get_vm_pde(adev, level, &value, &flags);
>>>> +        }
>>>>                r = vm->update_funcs->update(&params, bo, addr, 0,
>>>> ats_entries,
>>>> -                         0, ats_value);
>>>> +                         value, flags);
>>>>             if (r)
>>>>                 return r;
> _______________________________________________
> 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

Re: [PATCH 1/8] drm/amdgpu: fix ATC handling for Ryzen

[Kuehling, Felix]

On 2019-04-01 2:03 p.m., Christian König wrote:
> Am 01.04.19 um 19:59 schrieb Kuehling, Felix:
>> On 2019-04-01 7:23 a.m., Christian König wrote:
>>> Am 30.03.19 um 01:41 schrieb Kuehling, Felix:
>>>> Patches 1-3 are Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>> Thanks.
>>>
>>>> About the direct mode, that removes a bunch of synchronization, so it
>>>> must make some assumptions about the state of the page tables. What
>>>> makes that safe?
>>> Direct mode is only supposed to be used during page fault handling.
>>>
>>> E.g. we know that the page tables are in the correct place in this
>>> situation because the hardware is hammering on a PTE and waiting for
>>> it to become valid.
>> A fence could also indicate a concurrent modification of the page table.
>> For example a PTB may be allocated and initialized concurrently, not in
>> direct mode. Would direct mode need to wait for a fence that indicates
>> completion of the PTB initialization? Or do we have some way to ensure
>> such concurrent allocation and initialization of a PTB cannot happen?
>
> Yeah, that is a very good question I haven't solved yet either.
>
> My currently best idea is to separate the address space, e.g. use the 
> lower address space for on demand paging and the higher with classic 
> pre-filled page tables for the MM and display engines.

That may work for graphics, but doesn't work for KFD. I need the ability 
to mix pre-filled page tables with HMM in the same SVM address space. 
That's why I was thinking that all page table updates for a given VM 
would need to use the same method.

Regards,
   Felix

>
> Christian.
>
>>
>> Regards,
>>     Felix
>>
>>
>>> Christian.
>>>
>>>>    Is it safe to use direct-mode on a
>>>> per-page-table-update basis? Or do all page table updates have to go
>>>> through direct mode to avoid hazards? If yes, then maybe this 
>>>> should be
>>>> a property of the VM rather than a parameter that gets passed to a 
>>>> bunch
>>>> of function calls.
>>>>
>>>> Regards,
>>>>      Felix
>>>>
>>>> On 2019-03-29 6:45 a.m., Christian König wrote:
>>>>> Otherwise we don't correctly use translate further.
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++-----
>>>>>     1 file changed, 8 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> index 3d221f044183..059d9802e713 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -767,14 +767,17 @@ static int amdgpu_vm_clear_bo(struct
>>>>> amdgpu_device *adev,
>>>>>            addr = 0;
>>>>>         if (ats_entries) {
>>>>> -        uint64_t ats_value;
>>>>> +        uint64_t value = 0, flags;
>>>>>     -        ats_value = AMDGPU_PTE_DEFAULT_ATC;
>>>>> -        if (level != AMDGPU_VM_PTB)
>>>>> -            ats_value |= AMDGPU_PDE_PTE;
>>>>> +        flags = AMDGPU_PTE_DEFAULT_ATC;
>>>>> +        if (level != AMDGPU_VM_PTB) {
>>>>> +            /* Handle leaf PDEs as PTEs */
>>>>> +            flags |= AMDGPU_PDE_PTE;
>>>>> +            amdgpu_gmc_get_vm_pde(adev, level, &value, &flags);
>>>>> +        }
>>>>>                r = vm->update_funcs->update(&params, bo, addr, 0,
>>>>> ats_entries,
>>>>> -                         0, ats_value);
>>>>> +                         value, flags);
>>>>>             if (r)
>>>>>                 return r;
>> _______________________________________________
>> 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

Re: [PATCH 1/8] drm/amdgpu: fix ATC handling for Ryzen

[Koenig, Christian]

Am 01.04.19 um 20:58 schrieb Kuehling, Felix:
> On 2019-04-01 2:03 p.m., Christian König wrote:
>> Am 01.04.19 um 19:59 schrieb Kuehling, Felix:
>>> On 2019-04-01 7:23 a.m., Christian König wrote:
>>>> Am 30.03.19 um 01:41 schrieb Kuehling, Felix:
>>>>> Patches 1-3 are Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>> Thanks.
>>>>
>>>>> About the direct mode, that removes a bunch of synchronization, so it
>>>>> must make some assumptions about the state of the page tables. What
>>>>> makes that safe?
>>>> Direct mode is only supposed to be used during page fault handling.
>>>>
>>>> E.g. we know that the page tables are in the correct place in this
>>>> situation because the hardware is hammering on a PTE and waiting for
>>>> it to become valid.
>>> A fence could also indicate a concurrent modification of the page table.
>>> For example a PTB may be allocated and initialized concurrently, not in
>>> direct mode. Would direct mode need to wait for a fence that indicates
>>> completion of the PTB initialization? Or do we have some way to ensure
>>> such concurrent allocation and initialization of a PTB cannot happen?
>> Yeah, that is a very good question I haven't solved yet either.
>>
>> My currently best idea is to separate the address space, e.g. use the
>> lower address space for on demand paging and the higher with classic
>> pre-filled page tables for the MM and display engines.
> That may work for graphics, but doesn't work for KFD. I need the ability
> to mix pre-filled page tables with HMM in the same SVM address space.

Even after thinking for multiple days about it I can't of hand find a 
way to make this work.

> That's why I was thinking that all page table updates for a given VM
> would need to use the same method.

Well what exactly do you mean with that? Essentially there are two methods:

1. Pre-fill the page tables before accessing them with the hardware.

2. Fill on demand with page faults.

I don't think we can mix those two methods together in the same address 
range.

E.g. we can say to use pre-fill for MM engines in the upper range and on 
demand filling in the lower range, but we can't mix them.

Regards,
Christian.

>
> Regards,
>     Felix
>
>> Christian.
>>
>>> Regards,
>>>      Felix
>>>
>>>
>>>> Christian.
>>>>
>>>>>     Is it safe to use direct-mode on a
>>>>> per-page-table-update basis? Or do all page table updates have to go
>>>>> through direct mode to avoid hazards? If yes, then maybe this
>>>>> should be
>>>>> a property of the VM rather than a parameter that gets passed to a
>>>>> bunch
>>>>> of function calls.
>>>>>
>>>>> Regards,
>>>>>       Felix
>>>>>
>>>>> On 2019-03-29 6:45 a.m., Christian König wrote:
>>>>>> Otherwise we don't correctly use translate further.
>>>>>>
>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>> ---
>>>>>>      drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++-----
>>>>>>      1 file changed, 8 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> index 3d221f044183..059d9802e713 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> @@ -767,14 +767,17 @@ static int amdgpu_vm_clear_bo(struct
>>>>>> amdgpu_device *adev,
>>>>>>             addr = 0;
>>>>>>          if (ats_entries) {
>>>>>> -        uint64_t ats_value;
>>>>>> +        uint64_t value = 0, flags;
>>>>>>      -        ats_value = AMDGPU_PTE_DEFAULT_ATC;
>>>>>> -        if (level != AMDGPU_VM_PTB)
>>>>>> -            ats_value |= AMDGPU_PDE_PTE;
>>>>>> +        flags = AMDGPU_PTE_DEFAULT_ATC;
>>>>>> +        if (level != AMDGPU_VM_PTB) {
>>>>>> +            /* Handle leaf PDEs as PTEs */
>>>>>> +            flags |= AMDGPU_PDE_PTE;
>>>>>> +            amdgpu_gmc_get_vm_pde(adev, level, &value, &flags);
>>>>>> +        }
>>>>>>                 r = vm->update_funcs->update(&params, bo, addr, 0,
>>>>>> ats_entries,
>>>>>> -                         0, ats_value);
>>>>>> +                         value, flags);
>>>>>>              if (r)
>>>>>>                  return r;
>>> _______________________________________________
>>> 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

Re: [PATCH 1/8] drm/amdgpu: fix ATC handling for Ryzen

[Kuehling, Felix]

On 2019-04-03 1:24 p.m., Koenig, Christian wrote:
> Am 01.04.19 um 20:58 schrieb Kuehling, Felix:
>> On 2019-04-01 2:03 p.m., Christian König wrote:
>>> Am 01.04.19 um 19:59 schrieb Kuehling, Felix:
>>>> On 2019-04-01 7:23 a.m., Christian König wrote:
>>>>> Am 30.03.19 um 01:41 schrieb Kuehling, Felix:
>>>>>> Patches 1-3 are Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>>> Thanks.
>>>>>
>>>>>> About the direct mode, that removes a bunch of synchronization, so it
>>>>>> must make some assumptions about the state of the page tables. What
>>>>>> makes that safe?
>>>>> Direct mode is only supposed to be used during page fault handling.
>>>>>
>>>>> E.g. we know that the page tables are in the correct place in this
>>>>> situation because the hardware is hammering on a PTE and waiting for
>>>>> it to become valid.
>>>> A fence could also indicate a concurrent modification of the page table.
>>>> For example a PTB may be allocated and initialized concurrently, not in
>>>> direct mode. Would direct mode need to wait for a fence that indicates
>>>> completion of the PTB initialization? Or do we have some way to ensure
>>>> such concurrent allocation and initialization of a PTB cannot happen?
>>> Yeah, that is a very good question I haven't solved yet either.
>>>
>>> My currently best idea is to separate the address space, e.g. use the
>>> lower address space for on demand paging and the higher with classic
>>> pre-filled page tables for the MM and display engines.
>> That may work for graphics, but doesn't work for KFD. I need the ability
>> to mix pre-filled page tables with HMM in the same SVM address space.
> Even after thinking for multiple days about it I can't of hand find a
> way to make this work.
>
>> That's why I was thinking that all page table updates for a given VM
>> would need to use the same method.
> Well what exactly do you mean with that? Essentially there are two methods:
>
> 1. Pre-fill the page tables before accessing them with the hardware.
>
> 2. Fill on demand with page faults.
>
> I don't think we can mix those two methods together in the same address
> range.

That's what I was hoping to do. For example an application could use 
"old" BO-based memory management APIs that pre-fill page tables with 
"new" HMM-based memory management APIs that rely on page faults. Those 
may be different libraries written in different languages running in the 
same application. E.g. a GPU BLAS implementation that's optimized and 
uses old-style memory allocations linked to an OpenMP application that 
relies on HMM.

If that's not possible, I'd need to emulate all the old memory APIs on 
top of HMM. I was hoping to avoid that.

Even when page faults are enabled, we want to be able to pre-fault stuff 
to avoid the performance it on the first access. Are you saying that 
won't be possible?

Regards,
   Felix


>
> E.g. we can say to use pre-fill for MM engines in the upper range and on
> demand filling in the lower range, but we can't mix them.
>
> Regards,
> Christian.
>
>> Regards,
>>      Felix
>>
>>> Christian.
>>>
>>>> Regards,
>>>>       Felix
>>>>
>>>>
>>>>> Christian.
>>>>>
>>>>>>      Is it safe to use direct-mode on a
>>>>>> per-page-table-update basis? Or do all page table updates have to go
>>>>>> through direct mode to avoid hazards? If yes, then maybe this
>>>>>> should be
>>>>>> a property of the VM rather than a parameter that gets passed to a
>>>>>> bunch
>>>>>> of function calls.
>>>>>>
>>>>>> Regards,
>>>>>>        Felix
>>>>>>
>>>>>> On 2019-03-29 6:45 a.m., Christian König wrote:
>>>>>>> Otherwise we don't correctly use translate further.
>>>>>>>
>>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>>> ---
>>>>>>>       drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++-----
>>>>>>>       1 file changed, 8 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> index 3d221f044183..059d9802e713 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> @@ -767,14 +767,17 @@ static int amdgpu_vm_clear_bo(struct
>>>>>>> amdgpu_device *adev,
>>>>>>>              addr = 0;
>>>>>>>           if (ats_entries) {
>>>>>>> -        uint64_t ats_value;
>>>>>>> +        uint64_t value = 0, flags;
>>>>>>>       -        ats_value = AMDGPU_PTE_DEFAULT_ATC;
>>>>>>> -        if (level != AMDGPU_VM_PTB)
>>>>>>> -            ats_value |= AMDGPU_PDE_PTE;
>>>>>>> +        flags = AMDGPU_PTE_DEFAULT_ATC;
>>>>>>> +        if (level != AMDGPU_VM_PTB) {
>>>>>>> +            /* Handle leaf PDEs as PTEs */
>>>>>>> +            flags |= AMDGPU_PDE_PTE;
>>>>>>> +            amdgpu_gmc_get_vm_pde(adev, level, &value, &flags);
>>>>>>> +        }
>>>>>>>                  r = vm->update_funcs->update(&params, bo, addr, 0,
>>>>>>> ats_entries,
>>>>>>> -                         0, ats_value);
>>>>>>> +                         value, flags);
>>>>>>>               if (r)
>>>>>>>                   return r;
>>>> _______________________________________________
>>>> 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