[3/7] drm/amdgpu: move PT validation back into VM code

Submitted by Christian König on Sept. 29, 2016, 7:52 a.m.

Details

Message ID 1475135557-11537-3-git-send-email-deathsimple@vodafone.de
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Christian König Sept. 29, 2016, 7:52 a.m.
From: Christian König <christian.koenig@amd.com>

Saves a bunch of CPU cycles when swapping things back in and
allows us to split the VM headers into a separate file.

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

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 840fcdb..0c2d32b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -917,8 +917,9 @@  void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
 void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
 			 struct list_head *validated,
 			 struct amdgpu_bo_list_entry *entry);
-void amdgpu_vm_get_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
-			  struct list_head *duplicates);
+int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
+			      int (*callback)(void *p, struct amdgpu_bo *bo),
+			      void *param);
 void amdgpu_vm_move_pt_bos_in_lru(struct amdgpu_device *adev,
 				  struct amdgpu_vm *vm);
 int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 5beab71..2adea17 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -388,9 +388,9 @@  retry:
 
 /* Last resort, try to evict something from the current working set */
 static bool amdgpu_cs_try_evict(struct amdgpu_cs_parser *p,
-				struct amdgpu_bo_list_entry *lobj)
+				struct amdgpu_bo *validated)
 {
-	uint32_t domain = lobj->robj->allowed_domains;
+	uint32_t domain = validated->allowed_domains;
 	int r;
 
 	if (!p->evictable)
@@ -406,7 +406,7 @@  static bool amdgpu_cs_try_evict(struct amdgpu_cs_parser *p,
 		uint32_t other;
 
 		/* If we reached our current BO we can forget it */
-		if (candidate == lobj)
+		if (candidate->robj == validated)
 			break;
 
 		other = amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type);
@@ -439,6 +439,23 @@  static bool amdgpu_cs_try_evict(struct amdgpu_cs_parser *p,
 	return false;
 }
 
+static int amdgpu_cs_validate(void *param, struct amdgpu_bo *bo)
+{
+	struct amdgpu_cs_parser *p = param;
+	int r;
+
+	do {
+		r = amdgpu_cs_bo_validate(p, bo);
+	} while (r == -ENOMEM && amdgpu_cs_try_evict(p, bo));
+	if (r)
+		return r;
+
+	if (bo->shadow)
+		r = amdgpu_cs_bo_validate(p, bo);
+
+	return r;
+}
+
 static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
 			    struct list_head *validated)
 {
@@ -466,18 +483,10 @@  static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
 		if (p->evictable == lobj)
 			p->evictable = NULL;
 
-		do {
-			r = amdgpu_cs_bo_validate(p, bo);
-		} while (r == -ENOMEM && amdgpu_cs_try_evict(p, lobj));
+		r = amdgpu_cs_validate(p, bo);
 		if (r)
 			return r;
 
-		if (bo->shadow) {
-			r = amdgpu_cs_bo_validate(p, bo);
-			if (r)
-				return r;
-		}
-
 		if (binding_userptr) {
 			drm_free_large(lobj->user_pages);
 			lobj->user_pages = NULL;
@@ -595,14 +604,19 @@  static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 		list_splice(&need_pages, &p->validated);
 	}
 
-	amdgpu_vm_get_pt_bos(p->adev, &fpriv->vm, &duplicates);
-
 	p->bytes_moved_threshold = amdgpu_cs_get_threshold_for_moves(p->adev);
 	p->bytes_moved = 0;
 	p->evictable = list_last_entry(&p->validated,
 				       struct amdgpu_bo_list_entry,
 				       tv.head);
 
+	r = amdgpu_vm_validate_pt_bos(p->adev, &fpriv->vm,
+				      amdgpu_cs_validate, p);
+	if (r) {
+		DRM_ERROR("amdgpu_vm_validate_pt_bos() failed.\n");
+		goto error_validate;
+	}
+
 	r = amdgpu_cs_list_validate(p, &duplicates);
 	if (r) {
 		DRM_ERROR("amdgpu_cs_list_validate(duplicates) failed.\n");
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index f2fb72d..8ff90fe 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -471,6 +471,16 @@  out:
 	return r;
 }
 
+static int amdgpu_gem_va_check(void *p, struct amdgpu_bo *bo)
+{
+	unsigned domain = amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type);
+
+	/* if anything is swapped out don't swap it in here,
+	   just abort and wait for the next CS */
+
+	return domain == AMDGPU_GEM_DOMAIN_CPU ? -ERESTARTSYS : 0;
+}
+
 /**
  * amdgpu_gem_va_update_vm -update the bo_va in its VM
  *
@@ -481,7 +491,8 @@  out:
  * vital here, so they are not reported back to userspace.
  */
 static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
-				    struct amdgpu_bo_va *bo_va, uint32_t operation)
+				    struct amdgpu_bo_va *bo_va,
+				    uint32_t operation)
 {
 	struct ttm_validate_buffer tv, *entry;
 	struct amdgpu_bo_list_entry vm_pd;
@@ -504,7 +515,6 @@  static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
 	if (r)
 		goto error_print;
 
-	amdgpu_vm_get_pt_bos(adev, bo_va->vm, &duplicates);
 	list_for_each_entry(entry, &list, head) {
 		domain = amdgpu_mem_type_to_domain(entry->bo->mem.mem_type);
 		/* if anything is swapped out don't swap it in here,
@@ -512,13 +522,10 @@  static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
 		if (domain == AMDGPU_GEM_DOMAIN_CPU)
 			goto error_unreserve;
 	}
-	list_for_each_entry(entry, &duplicates, head) {
-		domain = amdgpu_mem_type_to_domain(entry->bo->mem.mem_type);
-		/* if anything is swapped out don't swap it in here,
-		   just abort and wait for the next CS */
-		if (domain == AMDGPU_GEM_DOMAIN_CPU)
-			goto error_unreserve;
-	}
+	r = amdgpu_vm_validate_pt_bos(adev, bo_va->vm, amdgpu_gem_va_check,
+				      NULL);
+	if (r)
+		goto error_unreserve;
 
 	r = amdgpu_vm_update_page_directory(adev, bo_va->vm);
 	if (r)
@@ -539,8 +546,6 @@  error_print:
 		DRM_ERROR("Couldn't update BO_VA (%d)\n", r);
 }
 
-
-
 int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 			  struct drm_file *filp)
 {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 8777394..4c5e79a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -116,27 +116,28 @@  void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
 }
 
 /**
- * amdgpu_vm_get_bos - add the vm BOs to a duplicates list
+ * amdgpu_vm_validate_pt_bos - validate the page table BOs
  *
  * @adev: amdgpu device pointer
  * @vm: vm providing the BOs
- * @duplicates: head of duplicates list
+ * @callback: callback to do the validation
  *
- * Add the page directory to the BO duplicates list
- * for command submission.
+ * Validate the page table BOs on command submission if neccessary.
  */
-void amdgpu_vm_get_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
-			  struct list_head *duplicates)
+int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
+			      int (*callback)(void *p, struct amdgpu_bo *bo),
+			      void *param)
 {
 	uint64_t num_evictions;
 	unsigned i;
+	int r;
 
 	/* We only need to validate the page tables
 	 * if they aren't already valid.
 	 */
 	num_evictions = atomic64_read(&adev->num_evictions);
 	if (num_evictions == vm->last_eviction_counter)
-		return;
+		return 0;
 
 	/* add the vm page table to the list */
 	for (i = 0; i <= vm->max_pde_used; ++i) {
@@ -145,9 +146,12 @@  void amdgpu_vm_get_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		if (!entry->robj)
 			continue;
 
-		list_add(&entry->tv.head, duplicates);
+		r = callback(param, entry->robj);
+		if (r)
+			return r;
 	}
 
+	return 0;
 }
 
 /**

Comments

On Thu, Sep 29, 2016 at 3:52 AM, Christian König
<deathsimple@vodafone.de> wrote:
> From: Christian König <christian.koenig@amd.com>
>
> Saves a bunch of CPU cycles when swapping things back in and
> allows us to split the VM headers into a separate file.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  5 ++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 42 ++++++++++++++++++++++-----------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 27 ++++++++++++---------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 20 +++++++++-------
>  4 files changed, 59 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 840fcdb..0c2d32b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -917,8 +917,9 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
>  void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>                          struct list_head *validated,
>                          struct amdgpu_bo_list_entry *entry);
> -void amdgpu_vm_get_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> -                         struct list_head *duplicates);
> +int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> +                             int (*callback)(void *p, struct amdgpu_bo *bo),
> +                             void *param);
>  void amdgpu_vm_move_pt_bos_in_lru(struct amdgpu_device *adev,
>                                   struct amdgpu_vm *vm);
>  int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 5beab71..2adea17 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -388,9 +388,9 @@ retry:
>
>  /* Last resort, try to evict something from the current working set */
>  static bool amdgpu_cs_try_evict(struct amdgpu_cs_parser *p,
> -                               struct amdgpu_bo_list_entry *lobj)
> +                               struct amdgpu_bo *validated)
>  {
> -       uint32_t domain = lobj->robj->allowed_domains;
> +       uint32_t domain = validated->allowed_domains;
>         int r;
>
>         if (!p->evictable)
> @@ -406,7 +406,7 @@ static bool amdgpu_cs_try_evict(struct amdgpu_cs_parser *p,
>                 uint32_t other;
>
>                 /* If we reached our current BO we can forget it */
> -               if (candidate == lobj)
> +               if (candidate->robj == validated)
>                         break;
>
>                 other = amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type);
> @@ -439,6 +439,23 @@ static bool amdgpu_cs_try_evict(struct amdgpu_cs_parser *p,
>         return false;
>  }
>
> +static int amdgpu_cs_validate(void *param, struct amdgpu_bo *bo)
> +{
> +       struct amdgpu_cs_parser *p = param;
> +       int r;
> +
> +       do {
> +               r = amdgpu_cs_bo_validate(p, bo);
> +       } while (r == -ENOMEM && amdgpu_cs_try_evict(p, bo));
> +       if (r)
> +               return r;
> +
> +       if (bo->shadow)
> +               r = amdgpu_cs_bo_validate(p, bo);
> +
> +       return r;
> +}
> +
>  static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
>                             struct list_head *validated)
>  {
> @@ -466,18 +483,10 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
>                 if (p->evictable == lobj)
>                         p->evictable = NULL;
>
> -               do {
> -                       r = amdgpu_cs_bo_validate(p, bo);
> -               } while (r == -ENOMEM && amdgpu_cs_try_evict(p, lobj));
> +               r = amdgpu_cs_validate(p, bo);
>                 if (r)
>                         return r;
>
> -               if (bo->shadow) {
> -                       r = amdgpu_cs_bo_validate(p, bo);
> -                       if (r)
> -                               return r;
> -               }
> -
>                 if (binding_userptr) {
>                         drm_free_large(lobj->user_pages);
>                         lobj->user_pages = NULL;
> @@ -595,14 +604,19 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>                 list_splice(&need_pages, &p->validated);
>         }
>
> -       amdgpu_vm_get_pt_bos(p->adev, &fpriv->vm, &duplicates);
> -
>         p->bytes_moved_threshold = amdgpu_cs_get_threshold_for_moves(p->adev);
>         p->bytes_moved = 0;
>         p->evictable = list_last_entry(&p->validated,
>                                        struct amdgpu_bo_list_entry,
>                                        tv.head);
>
> +       r = amdgpu_vm_validate_pt_bos(p->adev, &fpriv->vm,
> +                                     amdgpu_cs_validate, p);
> +       if (r) {
> +               DRM_ERROR("amdgpu_vm_validate_pt_bos() failed.\n");
> +               goto error_validate;
> +       }
> +
>         r = amdgpu_cs_list_validate(p, &duplicates);
>         if (r) {
>                 DRM_ERROR("amdgpu_cs_list_validate(duplicates) failed.\n");
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index f2fb72d..8ff90fe 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -471,6 +471,16 @@ out:
>         return r;
>  }
>
> +static int amdgpu_gem_va_check(void *p, struct amdgpu_bo *bo)

maybe change the first parameter to param for consistency with
amdgpu_cs_validate()?

> +{
> +       unsigned domain = amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type);
> +
> +       /* if anything is swapped out don't swap it in here,
> +          just abort and wait for the next CS */
> +
> +       return domain == AMDGPU_GEM_DOMAIN_CPU ? -ERESTARTSYS : 0;
> +}
> +
>  /**
>   * amdgpu_gem_va_update_vm -update the bo_va in its VM
>   *
> @@ -481,7 +491,8 @@ out:
>   * vital here, so they are not reported back to userspace.
>   */
>  static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
> -                                   struct amdgpu_bo_va *bo_va, uint32_t operation)
> +                                   struct amdgpu_bo_va *bo_va,
> +                                   uint32_t operation)
>  {
>         struct ttm_validate_buffer tv, *entry;
>         struct amdgpu_bo_list_entry vm_pd;
> @@ -504,7 +515,6 @@ static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
>         if (r)
>                 goto error_print;
>
> -       amdgpu_vm_get_pt_bos(adev, bo_va->vm, &duplicates);
>         list_for_each_entry(entry, &list, head) {
>                 domain = amdgpu_mem_type_to_domain(entry->bo->mem.mem_type);
>                 /* if anything is swapped out don't swap it in here,
> @@ -512,13 +522,10 @@ static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
>                 if (domain == AMDGPU_GEM_DOMAIN_CPU)
>                         goto error_unreserve;
>         }
> -       list_for_each_entry(entry, &duplicates, head) {
> -               domain = amdgpu_mem_type_to_domain(entry->bo->mem.mem_type);
> -               /* if anything is swapped out don't swap it in here,
> -                  just abort and wait for the next CS */
> -               if (domain == AMDGPU_GEM_DOMAIN_CPU)
> -                       goto error_unreserve;
> -       }
> +       r = amdgpu_vm_validate_pt_bos(adev, bo_va->vm, amdgpu_gem_va_check,
> +                                     NULL);
> +       if (r)
> +               goto error_unreserve;
>
>         r = amdgpu_vm_update_page_directory(adev, bo_va->vm);
>         if (r)
> @@ -539,8 +546,6 @@ error_print:
>                 DRM_ERROR("Couldn't update BO_VA (%d)\n", r);
>  }
>
> -
> -
>  int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>                           struct drm_file *filp)
>  {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 8777394..4c5e79a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -116,27 +116,28 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>  }
>
>  /**
> - * amdgpu_vm_get_bos - add the vm BOs to a duplicates list
> + * amdgpu_vm_validate_pt_bos - validate the page table BOs
>   *
>   * @adev: amdgpu device pointer
>   * @vm: vm providing the BOs
> - * @duplicates: head of duplicates list
> + * @callback: callback to do the validation

How about changing the name to validation_callback or validation_cb so
it's clear what it's for in the code.

>   *
> - * Add the page directory to the BO duplicates list
> - * for command submission.
> + * Validate the page table BOs on command submission if neccessary.
>   */
> -void amdgpu_vm_get_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> -                         struct list_head *duplicates)
> +int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> +                             int (*callback)(void *p, struct amdgpu_bo *bo),
> +                             void *param)
>  {
>         uint64_t num_evictions;
>         unsigned i;
> +       int r;
>
>         /* We only need to validate the page tables
>          * if they aren't already valid.
>          */
>         num_evictions = atomic64_read(&adev->num_evictions);
>         if (num_evictions == vm->last_eviction_counter)
> -               return;
> +               return 0;
>
>         /* add the vm page table to the list */
>         for (i = 0; i <= vm->max_pde_used; ++i) {
> @@ -145,9 +146,12 @@ void amdgpu_vm_get_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>                 if (!entry->robj)
>                         continue;
>
> -               list_add(&entry->tv.head, duplicates);
> +               r = callback(param, entry->robj);
> +               if (r)
> +                       return r;
>         }
>
> +       return 0;
>  }
>
>  /**
> --
> 2.5.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx