[PATCH 1/2] drm/amdgpu: remove amdgpu_bo_list_entry.robj

[PATCH 1/2] drm/amdgpu: remove amdgpu_bo_list_entry.robj

[Christian König]

We can get that just by casting tv.bo.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 42 ++++++++++++---------
 drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      | 58 ++++++++++++++++-------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      |  3 +-
 4 files changed, 58 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
index b80243d3972e..14d2982a47cc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
@@ -49,8 +49,11 @@ static void amdgpu_bo_list_free(struct kref *ref)
 						   refcount);
 	struct amdgpu_bo_list_entry *e;
 
-	amdgpu_bo_list_for_each_entry(e, list)
-		amdgpu_bo_unref(&e->robj);
+	amdgpu_bo_list_for_each_entry(e, list) {
+		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
+
+		amdgpu_bo_unref(&bo);
+	}
 
 	call_rcu(&list->rhead, amdgpu_bo_list_free_rcu);
 }
@@ -112,21 +115,20 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp,
 			entry = &array[last_entry++];
 		}
 
-		entry->robj = bo;
 		entry->priority = min(info[i].bo_priority,
 				      AMDGPU_BO_LIST_MAX_PRIORITY);
-		entry->tv.bo = &entry->robj->tbo;
-		entry->tv.shared = !entry->robj->prime_shared_count;
-
-		if (entry->robj->preferred_domains == AMDGPU_GEM_DOMAIN_GDS)
-			list->gds_obj = entry->robj;
-		if (entry->robj->preferred_domains == AMDGPU_GEM_DOMAIN_GWS)
-			list->gws_obj = entry->robj;
-		if (entry->robj->preferred_domains == AMDGPU_GEM_DOMAIN_OA)
-			list->oa_obj = entry->robj;
-
-		total_size += amdgpu_bo_size(entry->robj);
-		trace_amdgpu_bo_list_set(list, entry->robj);
+		entry->tv.bo = &bo->tbo;
+		entry->tv.shared = !bo->prime_shared_count;
+
+		if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_GDS)
+			list->gds_obj = bo;
+		if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_GWS)
+			list->gws_obj = bo;
+		if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_OA)
+			list->oa_obj = bo;
+
+		total_size += amdgpu_bo_size(bo);
+		trace_amdgpu_bo_list_set(list, bo);
 	}
 
 	list->first_userptr = first_userptr;
@@ -138,8 +140,11 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp,
 	return 0;
 
 error_free:
-	while (i--)
-		amdgpu_bo_unref(&array[i].robj);
+	while (i--) {
+		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(array[i].tv.bo);
+
+		amdgpu_bo_unref(&bo);
+	}
 	kvfree(list);
 	return r;
 
@@ -191,9 +196,10 @@ void amdgpu_bo_list_get_list(struct amdgpu_bo_list *list,
 	 * with the same priority, i.e. it must be stable.
 	 */
 	amdgpu_bo_list_for_each_entry(e, list) {
+		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
 		unsigned priority = e->priority;
 
-		if (!e->robj->parent)
+		if (!bo->parent)
 			list_add_tail(&e->tv.head, &bucket[priority]);
 
 		e->user_pages = NULL;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
index 61b089768e1c..7c5f5d1601e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
@@ -32,7 +32,6 @@ struct amdgpu_bo_va;
 struct amdgpu_fpriv;
 
 struct amdgpu_bo_list_entry {
-	struct amdgpu_bo		*robj;
 	struct ttm_validate_buffer	tv;
 	struct amdgpu_bo_va		*bo_va;
 	uint32_t			priority;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index c5cc648a1b4e..2e488c6f9562 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -39,6 +39,7 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p,
 				      uint32_t *offset)
 {
 	struct drm_gem_object *gobj;
+	struct amdgpu_bo *bo;
 	unsigned long size;
 	int r;
 
@@ -46,21 +47,21 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p,
 	if (gobj == NULL)
 		return -EINVAL;
 
-	p->uf_entry.robj = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj));
+	bo = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj));
 	p->uf_entry.priority = 0;
-	p->uf_entry.tv.bo = &p->uf_entry.robj->tbo;
+	p->uf_entry.tv.bo = &bo->tbo;
 	p->uf_entry.tv.shared = true;
 	p->uf_entry.user_pages = NULL;
 
 	drm_gem_object_put_unlocked(gobj);
 
-	size = amdgpu_bo_size(p->uf_entry.robj);
+	size = amdgpu_bo_size(bo);
 	if (size != PAGE_SIZE || (data->offset + 8) > size) {
 		r = -EINVAL;
 		goto error_unref;
 	}
 
-	if (amdgpu_ttm_tt_get_usermm(p->uf_entry.robj->tbo.ttm)) {
+	if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm)) {
 		r = -EINVAL;
 		goto error_unref;
 	}
@@ -70,7 +71,7 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p,
 	return 0;
 
 error_unref:
-	amdgpu_bo_unref(&p->uf_entry.robj);
+	amdgpu_bo_unref(&bo);
 	return r;
 }
 
@@ -229,7 +230,7 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, union drm_amdgpu_cs
 		goto free_all_kdata;
 	}
 
-	if (p->uf_entry.robj)
+	if (p->uf_entry.tv.bo)
 		p->job->uf_addr = uf_offset;
 	kfree(chunk_array);
 
@@ -458,13 +459,13 @@ static bool amdgpu_cs_try_evict(struct amdgpu_cs_parser *p,
 	     p->evictable = list_prev_entry(p->evictable, tv.head)) {
 
 		struct amdgpu_bo_list_entry *candidate = p->evictable;
-		struct amdgpu_bo *bo = candidate->robj;
+		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(candidate->tv.bo);
 		struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
 		bool update_bytes_moved_vis;
 		uint32_t other;
 
 		/* If we reached our current BO we can forget it */
-		if (candidate->robj == validated)
+		if (bo == validated)
 			break;
 
 		/* We can't move pinned BOs here */
@@ -529,7 +530,7 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
 	int r;
 
 	list_for_each_entry(lobj, validated, tv.head) {
-		struct amdgpu_bo *bo = lobj->robj;
+		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(lobj->tv.bo);
 		bool binding_userptr = false;
 		struct mm_struct *usermm;
 
@@ -604,7 +605,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 	INIT_LIST_HEAD(&duplicates);
 	amdgpu_vm_get_pd_bo(&fpriv->vm, &p->validated, &p->vm_pd);
 
-	if (p->uf_entry.robj && !p->uf_entry.robj->parent)
+	if (p->uf_entry.tv.bo && !ttm_to_amdgpu_bo(p->uf_entry.tv.bo)->parent)
 		list_add(&p->uf_entry.tv.head, &p->validated);
 
 	while (1) {
@@ -620,7 +621,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 
 		INIT_LIST_HEAD(&need_pages);
 		amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
-			struct amdgpu_bo *bo = e->robj;
+			struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
 
 			if (amdgpu_ttm_tt_userptr_invalidated(bo->tbo.ttm,
 				 &e->user_invalidated) && e->user_pages) {
@@ -639,7 +640,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 				list_del(&e->tv.head);
 				list_add(&e->tv.head, &need_pages);
 
-				amdgpu_bo_unreserve(e->robj);
+				amdgpu_bo_unreserve(bo);
 			}
 		}
 
@@ -658,7 +659,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 
 		/* Fill the page arrays for all userptrs. */
 		list_for_each_entry(e, &need_pages, tv.head) {
-			struct ttm_tt *ttm = e->robj->tbo.ttm;
+			struct ttm_tt *ttm = e->tv.bo->ttm;
 
 			e->user_pages = kvmalloc_array(ttm->num_pages,
 							 sizeof(struct page*),
@@ -717,7 +718,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 	oa = p->bo_list->oa_obj;
 
 	amdgpu_bo_list_for_each_entry(e, p->bo_list)
-		e->bo_va = amdgpu_vm_bo_find(vm, e->robj);
+		e->bo_va = amdgpu_vm_bo_find(vm, ttm_to_amdgpu_bo(e->tv.bo));
 
 	if (gds) {
 		p->job->gds_base = amdgpu_bo_gpu_offset(gds);
@@ -732,8 +733,8 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 		p->job->oa_size = amdgpu_bo_size(oa);
 	}
 
-	if (!r && p->uf_entry.robj) {
-		struct amdgpu_bo *uf = p->uf_entry.robj;
+	if (!r && p->uf_entry.tv.bo) {
+		struct amdgpu_bo *uf = ttm_to_amdgpu_bo(p->uf_entry.tv.bo);
 
 		r = amdgpu_ttm_alloc_gart(&uf->tbo);
 		p->job->uf_addr += amdgpu_bo_gpu_offset(uf);
@@ -749,8 +750,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 		if (!e->user_pages)
 			continue;
 
-		release_pages(e->user_pages,
-			      e->robj->tbo.ttm->num_pages);
+		release_pages(e->user_pages, e->tv.bo->ttm->num_pages);
 		kvfree(e->user_pages);
 	}
 
@@ -763,9 +763,11 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
 	int r;
 
 	list_for_each_entry(e, &p->validated, tv.head) {
-		struct reservation_object *resv = e->robj->tbo.resv;
+		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
+		struct reservation_object *resv = bo->tbo.resv;
+
 		r = amdgpu_sync_resv(p->adev, &p->job->sync, resv, p->filp,
-				     amdgpu_bo_explicit_sync(e->robj));
+				     amdgpu_bo_explicit_sync(bo));
 
 		if (r)
 			return r;
@@ -808,7 +810,11 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error,
 	kfree(parser->chunks);
 	if (parser->job)
 		amdgpu_job_free(parser->job);
-	amdgpu_bo_unref(&parser->uf_entry.robj);
+	if (parser->uf_entry.tv.bo) {
+		struct amdgpu_bo *uf = ttm_to_amdgpu_bo(parser->uf_entry.tv.bo);
+
+		amdgpu_bo_unref(&uf);
+	}
 }
 
 static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
@@ -919,7 +925,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 		struct dma_fence *f;
 
 		/* ignore duplicates */
-		bo = e->robj;
+		bo = ttm_to_amdgpu_bo(e->tv.bo);
 		if (!bo)
 			continue;
 
@@ -958,11 +964,13 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 	if (amdgpu_vm_debug) {
 		/* Invalidate all BOs to test for userspace bugs */
 		amdgpu_bo_list_for_each_entry(e, p->bo_list) {
+			struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
+
 			/* ignore duplicates */
-			if (!e->robj)
+			if (!bo)
 				continue;
 
-			amdgpu_vm_bo_invalidate(adev, e->robj, false);
+			amdgpu_vm_bo_invalidate(adev, bo, false);
 		}
 	}
 
@@ -1211,7 +1219,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
 	/* No memory allocation is allowed while holding the mn lock */
 	amdgpu_mn_lock(p->mn);
 	amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
-		struct amdgpu_bo *bo = e->robj;
+		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
 
 		if (amdgpu_ttm_tt_userptr_needs_pages(bo->tbo.ttm)) {
 			r = -ERESTARTSYS;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 3e37b119371d..cb6a5114128e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -593,9 +593,8 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
 			 struct list_head *validated,
 			 struct amdgpu_bo_list_entry *entry)
 {
-	entry->robj = vm->root.base.bo;
 	entry->priority = 0;
-	entry->tv.bo = &entry->robj->tbo;
+	entry->tv.bo = &vm->root.base.bo->tbo;
 	entry->tv.shared = true;
 	entry->user_pages = NULL;
 	list_add(&entry->tv.head, validated);
-- 
2.14.1

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

[PATCH 2/2] drm/amdgpu: use a single linked list for amdgpu_vm_bo_base

[Christian König]

Instead of the double linked list. Gets the size of amdgpu_vm_pt down to
64 bytes again.

We could even reduce it down to 32 bytes, but that would require some
rather extreme hacks.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index de990bdcdd6c..e6909252aefa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -448,7 +448,7 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
 		return -ENOMEM;
 	drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
 	INIT_LIST_HEAD(&bo->shadow_list);
-	INIT_LIST_HEAD(&bo->va);
+	bo->vm_bo = NULL;
 	bo->preferred_domains = bp->preferred_domain ? bp->preferred_domain :
 		bp->domain;
 	bo->allowed_domains = bo->preferred_domains;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index 907fdf46d895..64337ff2ad63 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -89,8 +89,8 @@ struct amdgpu_bo {
 	void				*metadata;
 	u32				metadata_size;
 	unsigned			prime_shared_count;
-	/* list of all virtual address to which this bo is associated to */
-	struct list_head		va;
+	/* per VM structure for page tables and with virtual addresses */
+	struct amdgpu_vm_bo_base	*vm_bo;
 	/* Constant after initialization */
 	struct drm_gem_object		gem_base;
 	struct amdgpu_bo		*parent;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index cb6a5114128e..fb6b16273c54 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -309,12 +309,13 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
 {
 	base->vm = vm;
 	base->bo = bo;
-	INIT_LIST_HEAD(&base->bo_list);
+	base->next = NULL;
 	INIT_LIST_HEAD(&base->vm_status);
 
 	if (!bo)
 		return;
-	list_add_tail(&base->bo_list, &bo->va);
+	base->next = bo->vm_bo;
+	bo->vm_bo = base;
 
 	if (bo->tbo.resv != vm->root.base.bo->tbo.resv)
 		return;
@@ -352,7 +353,7 @@ static struct amdgpu_vm_pt *amdgpu_vm_pt_parent(struct amdgpu_vm_pt *pt)
 	if (!parent)
 		return NULL;
 
-	return list_first_entry(&parent->va, struct amdgpu_vm_pt, base.bo_list);
+	return container_of(parent->vm_bo, struct amdgpu_vm_pt, base);
 }
 
 /**
@@ -954,7 +955,7 @@ static void amdgpu_vm_free_pts(struct amdgpu_device *adev,
 	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, cursor, entry) {
 
 		if (entry->base.bo) {
-			list_del(&entry->base.bo_list);
+			entry->base.bo->vm_bo = NULL;
 			list_del(&entry->base.vm_status);
 			amdgpu_bo_unref(&entry->base.bo->shadow);
 			amdgpu_bo_unref(&entry->base.bo);
@@ -1162,12 +1163,13 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_
 struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
 				       struct amdgpu_bo *bo)
 {
-	struct amdgpu_bo_va *bo_va;
+	struct amdgpu_vm_bo_base *base;
 
-	list_for_each_entry(bo_va, &bo->va, base.bo_list) {
-		if (bo_va->base.vm == vm) {
-			return bo_va;
-		}
+	for (base = bo->vm_bo; base; base = base->next) {
+		if (base->vm != vm)
+			continue;
+
+		return container_of(base, struct amdgpu_bo_va, base);
 	}
 	return NULL;
 }
@@ -2728,11 +2730,21 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
 	struct amdgpu_bo_va_mapping *mapping, *next;
 	struct amdgpu_bo *bo = bo_va->base.bo;
 	struct amdgpu_vm *vm = bo_va->base.vm;
+	struct amdgpu_vm_bo_base **base;
 
-	if (bo && bo->tbo.resv == vm->root.base.bo->tbo.resv)
-		vm->bulk_moveable = false;
+	if (bo) {
+		if (bo->tbo.resv == vm->root.base.bo->tbo.resv)
+			vm->bulk_moveable = false;
 
-	list_del(&bo_va->base.bo_list);
+		for (base = &bo_va->base.bo->vm_bo; *base;
+		     base = &(*base)->next) {
+			if (*base != &bo_va->base)
+				continue;
+
+			*base = bo_va->base.next;
+			break;
+		}
+	}
 
 	spin_lock(&vm->invalidated_lock);
 	list_del(&bo_va->base.vm_status);
@@ -2774,7 +2786,7 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
 	if (bo->parent && bo->parent->shadow == bo)
 		bo = bo->parent;
 
-	list_for_each_entry(bo_base, &bo->va, bo_list) {
+	for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
 		struct amdgpu_vm *vm = bo_base->vm;
 
 		if (evicted && bo->tbo.resv == vm->root.base.bo->tbo.resv) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index e275ee7c1bc1..8966e40767eb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -128,7 +128,7 @@ struct amdgpu_vm_bo_base {
 	struct amdgpu_bo		*bo;
 
 	/* protected by bo being reserved */
-	struct list_head		bo_list;
+	struct amdgpu_vm_bo_base	*next;
 
 	/* protected by spinlock */
 	struct list_head		vm_status;
-- 
2.14.1

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

Re: [PATCH 1/2] drm/amdgpu: remove amdgpu_bo_list_entry.robj

[zhoucm1]

Reviewed-by: Chunming Zhou <david1.zhou@amd.com>


On 2018年09月12日 16:55, Christian König wrote:
> We can get that just by casting tv.bo.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 42 ++++++++++++---------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h |  1 -
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      | 58 ++++++++++++++++-------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      |  3 +-
>   4 files changed, 58 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> index b80243d3972e..14d2982a47cc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> @@ -49,8 +49,11 @@ static void amdgpu_bo_list_free(struct kref *ref)
>   						   refcount);
>   	struct amdgpu_bo_list_entry *e;
>   
> -	amdgpu_bo_list_for_each_entry(e, list)
> -		amdgpu_bo_unref(&e->robj);
> +	amdgpu_bo_list_for_each_entry(e, list) {
> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
> +
> +		amdgpu_bo_unref(&bo);
> +	}
>   
>   	call_rcu(&list->rhead, amdgpu_bo_list_free_rcu);
>   }
> @@ -112,21 +115,20 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp,
>   			entry = &array[last_entry++];
>   		}
>   
> -		entry->robj = bo;
>   		entry->priority = min(info[i].bo_priority,
>   				      AMDGPU_BO_LIST_MAX_PRIORITY);
> -		entry->tv.bo = &entry->robj->tbo;
> -		entry->tv.shared = !entry->robj->prime_shared_count;
> -
> -		if (entry->robj->preferred_domains == AMDGPU_GEM_DOMAIN_GDS)
> -			list->gds_obj = entry->robj;
> -		if (entry->robj->preferred_domains == AMDGPU_GEM_DOMAIN_GWS)
> -			list->gws_obj = entry->robj;
> -		if (entry->robj->preferred_domains == AMDGPU_GEM_DOMAIN_OA)
> -			list->oa_obj = entry->robj;
> -
> -		total_size += amdgpu_bo_size(entry->robj);
> -		trace_amdgpu_bo_list_set(list, entry->robj);
> +		entry->tv.bo = &bo->tbo;
> +		entry->tv.shared = !bo->prime_shared_count;
> +
> +		if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_GDS)
> +			list->gds_obj = bo;
> +		if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_GWS)
> +			list->gws_obj = bo;
> +		if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_OA)
> +			list->oa_obj = bo;
> +
> +		total_size += amdgpu_bo_size(bo);
> +		trace_amdgpu_bo_list_set(list, bo);
>   	}
>   
>   	list->first_userptr = first_userptr;
> @@ -138,8 +140,11 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp,
>   	return 0;
>   
>   error_free:
> -	while (i--)
> -		amdgpu_bo_unref(&array[i].robj);
> +	while (i--) {
> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(array[i].tv.bo);
> +
> +		amdgpu_bo_unref(&bo);
> +	}
>   	kvfree(list);
>   	return r;
>   
> @@ -191,9 +196,10 @@ void amdgpu_bo_list_get_list(struct amdgpu_bo_list *list,
>   	 * with the same priority, i.e. it must be stable.
>   	 */
>   	amdgpu_bo_list_for_each_entry(e, list) {
> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>   		unsigned priority = e->priority;
>   
> -		if (!e->robj->parent)
> +		if (!bo->parent)
>   			list_add_tail(&e->tv.head, &bucket[priority]);
>   
>   		e->user_pages = NULL;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> index 61b089768e1c..7c5f5d1601e6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> @@ -32,7 +32,6 @@ struct amdgpu_bo_va;
>   struct amdgpu_fpriv;
>   
>   struct amdgpu_bo_list_entry {
> -	struct amdgpu_bo		*robj;
>   	struct ttm_validate_buffer	tv;
>   	struct amdgpu_bo_va		*bo_va;
>   	uint32_t			priority;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index c5cc648a1b4e..2e488c6f9562 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -39,6 +39,7 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p,
>   				      uint32_t *offset)
>   {
>   	struct drm_gem_object *gobj;
> +	struct amdgpu_bo *bo;
>   	unsigned long size;
>   	int r;
>   
> @@ -46,21 +47,21 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p,
>   	if (gobj == NULL)
>   		return -EINVAL;
>   
> -	p->uf_entry.robj = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj));
> +	bo = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj));
>   	p->uf_entry.priority = 0;
> -	p->uf_entry.tv.bo = &p->uf_entry.robj->tbo;
> +	p->uf_entry.tv.bo = &bo->tbo;
>   	p->uf_entry.tv.shared = true;
>   	p->uf_entry.user_pages = NULL;
>   
>   	drm_gem_object_put_unlocked(gobj);
>   
> -	size = amdgpu_bo_size(p->uf_entry.robj);
> +	size = amdgpu_bo_size(bo);
>   	if (size != PAGE_SIZE || (data->offset + 8) > size) {
>   		r = -EINVAL;
>   		goto error_unref;
>   	}
>   
> -	if (amdgpu_ttm_tt_get_usermm(p->uf_entry.robj->tbo.ttm)) {
> +	if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm)) {
>   		r = -EINVAL;
>   		goto error_unref;
>   	}
> @@ -70,7 +71,7 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p,
>   	return 0;
>   
>   error_unref:
> -	amdgpu_bo_unref(&p->uf_entry.robj);
> +	amdgpu_bo_unref(&bo);
>   	return r;
>   }
>   
> @@ -229,7 +230,7 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, union drm_amdgpu_cs
>   		goto free_all_kdata;
>   	}
>   
> -	if (p->uf_entry.robj)
> +	if (p->uf_entry.tv.bo)
>   		p->job->uf_addr = uf_offset;
>   	kfree(chunk_array);
>   
> @@ -458,13 +459,13 @@ static bool amdgpu_cs_try_evict(struct amdgpu_cs_parser *p,
>   	     p->evictable = list_prev_entry(p->evictable, tv.head)) {
>   
>   		struct amdgpu_bo_list_entry *candidate = p->evictable;
> -		struct amdgpu_bo *bo = candidate->robj;
> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(candidate->tv.bo);
>   		struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>   		bool update_bytes_moved_vis;
>   		uint32_t other;
>   
>   		/* If we reached our current BO we can forget it */
> -		if (candidate->robj == validated)
> +		if (bo == validated)
>   			break;
>   
>   		/* We can't move pinned BOs here */
> @@ -529,7 +530,7 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
>   	int r;
>   
>   	list_for_each_entry(lobj, validated, tv.head) {
> -		struct amdgpu_bo *bo = lobj->robj;
> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(lobj->tv.bo);
>   		bool binding_userptr = false;
>   		struct mm_struct *usermm;
>   
> @@ -604,7 +605,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>   	INIT_LIST_HEAD(&duplicates);
>   	amdgpu_vm_get_pd_bo(&fpriv->vm, &p->validated, &p->vm_pd);
>   
> -	if (p->uf_entry.robj && !p->uf_entry.robj->parent)
> +	if (p->uf_entry.tv.bo && !ttm_to_amdgpu_bo(p->uf_entry.tv.bo)->parent)
>   		list_add(&p->uf_entry.tv.head, &p->validated);
>   
>   	while (1) {
> @@ -620,7 +621,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>   
>   		INIT_LIST_HEAD(&need_pages);
>   		amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
> -			struct amdgpu_bo *bo = e->robj;
> +			struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>   
>   			if (amdgpu_ttm_tt_userptr_invalidated(bo->tbo.ttm,
>   				 &e->user_invalidated) && e->user_pages) {
> @@ -639,7 +640,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>   				list_del(&e->tv.head);
>   				list_add(&e->tv.head, &need_pages);
>   
> -				amdgpu_bo_unreserve(e->robj);
> +				amdgpu_bo_unreserve(bo);
>   			}
>   		}
>   
> @@ -658,7 +659,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>   
>   		/* Fill the page arrays for all userptrs. */
>   		list_for_each_entry(e, &need_pages, tv.head) {
> -			struct ttm_tt *ttm = e->robj->tbo.ttm;
> +			struct ttm_tt *ttm = e->tv.bo->ttm;
>   
>   			e->user_pages = kvmalloc_array(ttm->num_pages,
>   							 sizeof(struct page*),
> @@ -717,7 +718,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>   	oa = p->bo_list->oa_obj;
>   
>   	amdgpu_bo_list_for_each_entry(e, p->bo_list)
> -		e->bo_va = amdgpu_vm_bo_find(vm, e->robj);
> +		e->bo_va = amdgpu_vm_bo_find(vm, ttm_to_amdgpu_bo(e->tv.bo));
>   
>   	if (gds) {
>   		p->job->gds_base = amdgpu_bo_gpu_offset(gds);
> @@ -732,8 +733,8 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>   		p->job->oa_size = amdgpu_bo_size(oa);
>   	}
>   
> -	if (!r && p->uf_entry.robj) {
> -		struct amdgpu_bo *uf = p->uf_entry.robj;
> +	if (!r && p->uf_entry.tv.bo) {
> +		struct amdgpu_bo *uf = ttm_to_amdgpu_bo(p->uf_entry.tv.bo);
>   
>   		r = amdgpu_ttm_alloc_gart(&uf->tbo);
>   		p->job->uf_addr += amdgpu_bo_gpu_offset(uf);
> @@ -749,8 +750,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>   		if (!e->user_pages)
>   			continue;
>   
> -		release_pages(e->user_pages,
> -			      e->robj->tbo.ttm->num_pages);
> +		release_pages(e->user_pages, e->tv.bo->ttm->num_pages);
>   		kvfree(e->user_pages);
>   	}
>   
> @@ -763,9 +763,11 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
>   	int r;
>   
>   	list_for_each_entry(e, &p->validated, tv.head) {
> -		struct reservation_object *resv = e->robj->tbo.resv;
> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
> +		struct reservation_object *resv = bo->tbo.resv;
> +
>   		r = amdgpu_sync_resv(p->adev, &p->job->sync, resv, p->filp,
> -				     amdgpu_bo_explicit_sync(e->robj));
> +				     amdgpu_bo_explicit_sync(bo));
>   
>   		if (r)
>   			return r;
> @@ -808,7 +810,11 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error,
>   	kfree(parser->chunks);
>   	if (parser->job)
>   		amdgpu_job_free(parser->job);
> -	amdgpu_bo_unref(&parser->uf_entry.robj);
> +	if (parser->uf_entry.tv.bo) {
> +		struct amdgpu_bo *uf = ttm_to_amdgpu_bo(parser->uf_entry.tv.bo);
> +
> +		amdgpu_bo_unref(&uf);
> +	}
>   }
>   
>   static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
> @@ -919,7 +925,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>   		struct dma_fence *f;
>   
>   		/* ignore duplicates */
> -		bo = e->robj;
> +		bo = ttm_to_amdgpu_bo(e->tv.bo);
>   		if (!bo)
>   			continue;
>   
> @@ -958,11 +964,13 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>   	if (amdgpu_vm_debug) {
>   		/* Invalidate all BOs to test for userspace bugs */
>   		amdgpu_bo_list_for_each_entry(e, p->bo_list) {
> +			struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
> +
>   			/* ignore duplicates */
> -			if (!e->robj)
> +			if (!bo)
>   				continue;
>   
> -			amdgpu_vm_bo_invalidate(adev, e->robj, false);
> +			amdgpu_vm_bo_invalidate(adev, bo, false);
>   		}
>   	}
>   
> @@ -1211,7 +1219,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>   	/* No memory allocation is allowed while holding the mn lock */
>   	amdgpu_mn_lock(p->mn);
>   	amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
> -		struct amdgpu_bo *bo = e->robj;
> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>   
>   		if (amdgpu_ttm_tt_userptr_needs_pages(bo->tbo.ttm)) {
>   			r = -ERESTARTSYS;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 3e37b119371d..cb6a5114128e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -593,9 +593,8 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>   			 struct list_head *validated,
>   			 struct amdgpu_bo_list_entry *entry)
>   {
> -	entry->robj = vm->root.base.bo;
>   	entry->priority = 0;
> -	entry->tv.bo = &entry->robj->tbo;
> +	entry->tv.bo = &vm->root.base.bo->tbo;
>   	entry->tv.shared = true;
>   	entry->user_pages = NULL;
>   	list_add(&entry->tv.head, validated);

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

Re: [PATCH 2/2] drm/amdgpu: use a single linked list for amdgpu_vm_bo_base

[zhoucm1]

Reviewed-by: Chunming Zhou <david1.zhou@amd.com>


On 2018年09月12日 16:55, Christian König wrote:
> Instead of the double linked list. Gets the size of amdgpu_vm_pt down to
> 64 bytes again.
>
> We could even reduce it down to 32 bytes, but that would require some
> rather extreme hacks.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  4 ++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 38 ++++++++++++++++++++----------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  2 +-
>   4 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index de990bdcdd6c..e6909252aefa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -448,7 +448,7 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>   		return -ENOMEM;
>   	drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
>   	INIT_LIST_HEAD(&bo->shadow_list);
> -	INIT_LIST_HEAD(&bo->va);
> +	bo->vm_bo = NULL;
>   	bo->preferred_domains = bp->preferred_domain ? bp->preferred_domain :
>   		bp->domain;
>   	bo->allowed_domains = bo->preferred_domains;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index 907fdf46d895..64337ff2ad63 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -89,8 +89,8 @@ struct amdgpu_bo {
>   	void				*metadata;
>   	u32				metadata_size;
>   	unsigned			prime_shared_count;
> -	/* list of all virtual address to which this bo is associated to */
> -	struct list_head		va;
> +	/* per VM structure for page tables and with virtual addresses */
> +	struct amdgpu_vm_bo_base	*vm_bo;
>   	/* Constant after initialization */
>   	struct drm_gem_object		gem_base;
>   	struct amdgpu_bo		*parent;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index cb6a5114128e..fb6b16273c54 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -309,12 +309,13 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
>   {
>   	base->vm = vm;
>   	base->bo = bo;
> -	INIT_LIST_HEAD(&base->bo_list);
> +	base->next = NULL;
>   	INIT_LIST_HEAD(&base->vm_status);
>   
>   	if (!bo)
>   		return;
> -	list_add_tail(&base->bo_list, &bo->va);
> +	base->next = bo->vm_bo;
> +	bo->vm_bo = base;
>   
>   	if (bo->tbo.resv != vm->root.base.bo->tbo.resv)
>   		return;
> @@ -352,7 +353,7 @@ static struct amdgpu_vm_pt *amdgpu_vm_pt_parent(struct amdgpu_vm_pt *pt)
>   	if (!parent)
>   		return NULL;
>   
> -	return list_first_entry(&parent->va, struct amdgpu_vm_pt, base.bo_list);
> +	return container_of(parent->vm_bo, struct amdgpu_vm_pt, base);
>   }
>   
>   /**
> @@ -954,7 +955,7 @@ static void amdgpu_vm_free_pts(struct amdgpu_device *adev,
>   	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, cursor, entry) {
>   
>   		if (entry->base.bo) {
> -			list_del(&entry->base.bo_list);
> +			entry->base.bo->vm_bo = NULL;
>   			list_del(&entry->base.vm_status);
>   			amdgpu_bo_unref(&entry->base.bo->shadow);
>   			amdgpu_bo_unref(&entry->base.bo);
> @@ -1162,12 +1163,13 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_
>   struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
>   				       struct amdgpu_bo *bo)
>   {
> -	struct amdgpu_bo_va *bo_va;
> +	struct amdgpu_vm_bo_base *base;
>   
> -	list_for_each_entry(bo_va, &bo->va, base.bo_list) {
> -		if (bo_va->base.vm == vm) {
> -			return bo_va;
> -		}
> +	for (base = bo->vm_bo; base; base = base->next) {
> +		if (base->vm != vm)
> +			continue;
> +
> +		return container_of(base, struct amdgpu_bo_va, base);
>   	}
>   	return NULL;
>   }
> @@ -2728,11 +2730,21 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
>   	struct amdgpu_bo_va_mapping *mapping, *next;
>   	struct amdgpu_bo *bo = bo_va->base.bo;
>   	struct amdgpu_vm *vm = bo_va->base.vm;
> +	struct amdgpu_vm_bo_base **base;
>   
> -	if (bo && bo->tbo.resv == vm->root.base.bo->tbo.resv)
> -		vm->bulk_moveable = false;
> +	if (bo) {
> +		if (bo->tbo.resv == vm->root.base.bo->tbo.resv)
> +			vm->bulk_moveable = false;
>   
> -	list_del(&bo_va->base.bo_list);
> +		for (base = &bo_va->base.bo->vm_bo; *base;
> +		     base = &(*base)->next) {
> +			if (*base != &bo_va->base)
> +				continue;
> +
> +			*base = bo_va->base.next;
> +			break;
> +		}
> +	}
>   
>   	spin_lock(&vm->invalidated_lock);
>   	list_del(&bo_va->base.vm_status);
> @@ -2774,7 +2786,7 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>   	if (bo->parent && bo->parent->shadow == bo)
>   		bo = bo->parent;
>   
> -	list_for_each_entry(bo_base, &bo->va, bo_list) {
> +	for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>   		struct amdgpu_vm *vm = bo_base->vm;
>   
>   		if (evicted && bo->tbo.resv == vm->root.base.bo->tbo.resv) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index e275ee7c1bc1..8966e40767eb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -128,7 +128,7 @@ struct amdgpu_vm_bo_base {
>   	struct amdgpu_bo		*bo;
>   
>   	/* protected by bo being reserved */
> -	struct list_head		bo_list;
> +	struct amdgpu_vm_bo_base	*next;
>   
>   	/* protected by spinlock */
>   	struct list_head		vm_status;

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

Re: [PATCH 1/2] drm/amdgpu: remove amdgpu_bo_list_entry.robj

[Felix Kuehling]

On 2018-09-12 04:55 AM, Christian König wrote:
> We can get that just by casting tv.bo.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 42 ++++++++++++---------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h |  1 -
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      | 58 ++++++++++++++++-------------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      |  3 +-
>  4 files changed, 58 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> index b80243d3972e..14d2982a47cc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> @@ -49,8 +49,11 @@ static void amdgpu_bo_list_free(struct kref *ref)
>  						   refcount);
>  	struct amdgpu_bo_list_entry *e;
>  
> -	amdgpu_bo_list_for_each_entry(e, list)
> -		amdgpu_bo_unref(&e->robj);
> +	amdgpu_bo_list_for_each_entry(e, list) {
> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
> +
> +		amdgpu_bo_unref(&bo);
> +	}
>  
>  	call_rcu(&list->rhead, amdgpu_bo_list_free_rcu);
>  }
> @@ -112,21 +115,20 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp,
>  			entry = &array[last_entry++];
>  		}
>  
> -		entry->robj = bo;
>  		entry->priority = min(info[i].bo_priority,
>  				      AMDGPU_BO_LIST_MAX_PRIORITY);
> -		entry->tv.bo = &entry->robj->tbo;
> -		entry->tv.shared = !entry->robj->prime_shared_count;
> -
> -		if (entry->robj->preferred_domains == AMDGPU_GEM_DOMAIN_GDS)
> -			list->gds_obj = entry->robj;
> -		if (entry->robj->preferred_domains == AMDGPU_GEM_DOMAIN_GWS)
> -			list->gws_obj = entry->robj;
> -		if (entry->robj->preferred_domains == AMDGPU_GEM_DOMAIN_OA)
> -			list->oa_obj = entry->robj;
> -
> -		total_size += amdgpu_bo_size(entry->robj);
> -		trace_amdgpu_bo_list_set(list, entry->robj);
> +		entry->tv.bo = &bo->tbo;
> +		entry->tv.shared = !bo->prime_shared_count;

You're no longer initializing entry->priority here. Is that intentional?

Regards,
  Felix

> +
> +		if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_GDS)
> +			list->gds_obj = bo;
> +		if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_GWS)
> +			list->gws_obj = bo;
> +		if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_OA)
> +			list->oa_obj = bo;
> +
> +		total_size += amdgpu_bo_size(bo);
> +		trace_amdgpu_bo_list_set(list, bo);
>  	}
>  
>  	list->first_userptr = first_userptr;
> @@ -138,8 +140,11 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp,
>  	return 0;
>  
>  error_free:
> -	while (i--)
> -		amdgpu_bo_unref(&array[i].robj);
> +	while (i--) {
> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(array[i].tv.bo);
> +
> +		amdgpu_bo_unref(&bo);
> +	}
>  	kvfree(list);
>  	return r;
>  
> @@ -191,9 +196,10 @@ void amdgpu_bo_list_get_list(struct amdgpu_bo_list *list,
>  	 * with the same priority, i.e. it must be stable.
>  	 */
>  	amdgpu_bo_list_for_each_entry(e, list) {
> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>  		unsigned priority = e->priority;
>  
> -		if (!e->robj->parent)
> +		if (!bo->parent)
>  			list_add_tail(&e->tv.head, &bucket[priority]);
>  
>  		e->user_pages = NULL;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> index 61b089768e1c..7c5f5d1601e6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> @@ -32,7 +32,6 @@ struct amdgpu_bo_va;
>  struct amdgpu_fpriv;
>  
>  struct amdgpu_bo_list_entry {
> -	struct amdgpu_bo		*robj;
>  	struct ttm_validate_buffer	tv;
>  	struct amdgpu_bo_va		*bo_va;
>  	uint32_t			priority;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index c5cc648a1b4e..2e488c6f9562 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -39,6 +39,7 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p,
>  				      uint32_t *offset)
>  {
>  	struct drm_gem_object *gobj;
> +	struct amdgpu_bo *bo;
>  	unsigned long size;
>  	int r;
>  
> @@ -46,21 +47,21 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p,
>  	if (gobj == NULL)
>  		return -EINVAL;
>  
> -	p->uf_entry.robj = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj));
> +	bo = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj));
>  	p->uf_entry.priority = 0;
> -	p->uf_entry.tv.bo = &p->uf_entry.robj->tbo;
> +	p->uf_entry.tv.bo = &bo->tbo;
>  	p->uf_entry.tv.shared = true;
>  	p->uf_entry.user_pages = NULL;
>  
>  	drm_gem_object_put_unlocked(gobj);
>  
> -	size = amdgpu_bo_size(p->uf_entry.robj);
> +	size = amdgpu_bo_size(bo);
>  	if (size != PAGE_SIZE || (data->offset + 8) > size) {
>  		r = -EINVAL;
>  		goto error_unref;
>  	}
>  
> -	if (amdgpu_ttm_tt_get_usermm(p->uf_entry.robj->tbo.ttm)) {
> +	if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm)) {
>  		r = -EINVAL;
>  		goto error_unref;
>  	}
> @@ -70,7 +71,7 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p,
>  	return 0;
>  
>  error_unref:
> -	amdgpu_bo_unref(&p->uf_entry.robj);
> +	amdgpu_bo_unref(&bo);
>  	return r;
>  }
>  
> @@ -229,7 +230,7 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, union drm_amdgpu_cs
>  		goto free_all_kdata;
>  	}
>  
> -	if (p->uf_entry.robj)
> +	if (p->uf_entry.tv.bo)
>  		p->job->uf_addr = uf_offset;
>  	kfree(chunk_array);
>  
> @@ -458,13 +459,13 @@ static bool amdgpu_cs_try_evict(struct amdgpu_cs_parser *p,
>  	     p->evictable = list_prev_entry(p->evictable, tv.head)) {
>  
>  		struct amdgpu_bo_list_entry *candidate = p->evictable;
> -		struct amdgpu_bo *bo = candidate->robj;
> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(candidate->tv.bo);
>  		struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>  		bool update_bytes_moved_vis;
>  		uint32_t other;
>  
>  		/* If we reached our current BO we can forget it */
> -		if (candidate->robj == validated)
> +		if (bo == validated)
>  			break;
>  
>  		/* We can't move pinned BOs here */
> @@ -529,7 +530,7 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
>  	int r;
>  
>  	list_for_each_entry(lobj, validated, tv.head) {
> -		struct amdgpu_bo *bo = lobj->robj;
> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(lobj->tv.bo);
>  		bool binding_userptr = false;
>  		struct mm_struct *usermm;
>  
> @@ -604,7 +605,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  	INIT_LIST_HEAD(&duplicates);
>  	amdgpu_vm_get_pd_bo(&fpriv->vm, &p->validated, &p->vm_pd);
>  
> -	if (p->uf_entry.robj && !p->uf_entry.robj->parent)
> +	if (p->uf_entry.tv.bo && !ttm_to_amdgpu_bo(p->uf_entry.tv.bo)->parent)
>  		list_add(&p->uf_entry.tv.head, &p->validated);
>  
>  	while (1) {
> @@ -620,7 +621,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  
>  		INIT_LIST_HEAD(&need_pages);
>  		amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
> -			struct amdgpu_bo *bo = e->robj;
> +			struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>  
>  			if (amdgpu_ttm_tt_userptr_invalidated(bo->tbo.ttm,
>  				 &e->user_invalidated) && e->user_pages) {
> @@ -639,7 +640,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  				list_del(&e->tv.head);
>  				list_add(&e->tv.head, &need_pages);
>  
> -				amdgpu_bo_unreserve(e->robj);
> +				amdgpu_bo_unreserve(bo);
>  			}
>  		}
>  
> @@ -658,7 +659,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  
>  		/* Fill the page arrays for all userptrs. */
>  		list_for_each_entry(e, &need_pages, tv.head) {
> -			struct ttm_tt *ttm = e->robj->tbo.ttm;
> +			struct ttm_tt *ttm = e->tv.bo->ttm;
>  
>  			e->user_pages = kvmalloc_array(ttm->num_pages,
>  							 sizeof(struct page*),
> @@ -717,7 +718,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  	oa = p->bo_list->oa_obj;
>  
>  	amdgpu_bo_list_for_each_entry(e, p->bo_list)
> -		e->bo_va = amdgpu_vm_bo_find(vm, e->robj);
> +		e->bo_va = amdgpu_vm_bo_find(vm, ttm_to_amdgpu_bo(e->tv.bo));
>  
>  	if (gds) {
>  		p->job->gds_base = amdgpu_bo_gpu_offset(gds);
> @@ -732,8 +733,8 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  		p->job->oa_size = amdgpu_bo_size(oa);
>  	}
>  
> -	if (!r && p->uf_entry.robj) {
> -		struct amdgpu_bo *uf = p->uf_entry.robj;
> +	if (!r && p->uf_entry.tv.bo) {
> +		struct amdgpu_bo *uf = ttm_to_amdgpu_bo(p->uf_entry.tv.bo);
>  
>  		r = amdgpu_ttm_alloc_gart(&uf->tbo);
>  		p->job->uf_addr += amdgpu_bo_gpu_offset(uf);
> @@ -749,8 +750,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  		if (!e->user_pages)
>  			continue;
>  
> -		release_pages(e->user_pages,
> -			      e->robj->tbo.ttm->num_pages);
> +		release_pages(e->user_pages, e->tv.bo->ttm->num_pages);
>  		kvfree(e->user_pages);
>  	}
>  
> @@ -763,9 +763,11 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
>  	int r;
>  
>  	list_for_each_entry(e, &p->validated, tv.head) {
> -		struct reservation_object *resv = e->robj->tbo.resv;
> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
> +		struct reservation_object *resv = bo->tbo.resv;
> +
>  		r = amdgpu_sync_resv(p->adev, &p->job->sync, resv, p->filp,
> -				     amdgpu_bo_explicit_sync(e->robj));
> +				     amdgpu_bo_explicit_sync(bo));
>  
>  		if (r)
>  			return r;
> @@ -808,7 +810,11 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error,
>  	kfree(parser->chunks);
>  	if (parser->job)
>  		amdgpu_job_free(parser->job);
> -	amdgpu_bo_unref(&parser->uf_entry.robj);
> +	if (parser->uf_entry.tv.bo) {
> +		struct amdgpu_bo *uf = ttm_to_amdgpu_bo(parser->uf_entry.tv.bo);
> +
> +		amdgpu_bo_unref(&uf);
> +	}
>  }
>  
>  static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
> @@ -919,7 +925,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>  		struct dma_fence *f;
>  
>  		/* ignore duplicates */
> -		bo = e->robj;
> +		bo = ttm_to_amdgpu_bo(e->tv.bo);
>  		if (!bo)
>  			continue;
>  
> @@ -958,11 +964,13 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>  	if (amdgpu_vm_debug) {
>  		/* Invalidate all BOs to test for userspace bugs */
>  		amdgpu_bo_list_for_each_entry(e, p->bo_list) {
> +			struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
> +
>  			/* ignore duplicates */
> -			if (!e->robj)
> +			if (!bo)
>  				continue;
>  
> -			amdgpu_vm_bo_invalidate(adev, e->robj, false);
> +			amdgpu_vm_bo_invalidate(adev, bo, false);
>  		}
>  	}
>  
> @@ -1211,7 +1219,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>  	/* No memory allocation is allowed while holding the mn lock */
>  	amdgpu_mn_lock(p->mn);
>  	amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
> -		struct amdgpu_bo *bo = e->robj;
> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>  
>  		if (amdgpu_ttm_tt_userptr_needs_pages(bo->tbo.ttm)) {
>  			r = -ERESTARTSYS;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 3e37b119371d..cb6a5114128e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -593,9 +593,8 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>  			 struct list_head *validated,
>  			 struct amdgpu_bo_list_entry *entry)
>  {
> -	entry->robj = vm->root.base.bo;
>  	entry->priority = 0;
> -	entry->tv.bo = &entry->robj->tbo;
> +	entry->tv.bo = &vm->root.base.bo->tbo;
>  	entry->tv.shared = true;
>  	entry->user_pages = NULL;
>  	list_add(&entry->tv.head, validated);

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

Re: [PATCH 2/2] drm/amdgpu: use a single linked list for amdgpu_vm_bo_base

[Felix Kuehling]

Is the small reduction in memory footprint (8 bytes per page table on a
64-bit kernel) really worth the trouble of open-coding a single-linked
list implementation? I guess this change makes a bigger difference for
2-level page tables than it does for 4-level, because the amdgpu_vm_pt
array is allocated at the page directory level and includes page tables
that don't even exist yet and may never exist. The amount of memory you
save is the same as the size of the page directory.

I wonder if the overhead could be reduced more effectively by allocating
struct amdgpu_vm_pt with the page table, rather than with the page
directory. Then the amdgpu_vm_pt.entries array would be an array of
pointers instead. It could be an array[0] at the end of the structure
since the number of entries is know then the page directory is
allocated. The BO could also be embedded in the amdgpu_vm_pt structure
so it doesn't need to be a separate allocation from the amdgpu_vm_pt.

Acked-by: Felix Kuehling <Felix.Kuehling@amd.com>

Regards,
  Felix


On 2018-09-12 04:55 AM, Christian König wrote:
> Instead of the double linked list. Gets the size of amdgpu_vm_pt down to
> 64 bytes again.
>
> We could even reduce it down to 32 bytes, but that would require some
> rather extreme hacks.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  4 ++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 38 ++++++++++++++++++++----------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  2 +-
>  4 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index de990bdcdd6c..e6909252aefa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -448,7 +448,7 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>  		return -ENOMEM;
>  	drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
>  	INIT_LIST_HEAD(&bo->shadow_list);
> -	INIT_LIST_HEAD(&bo->va);
> +	bo->vm_bo = NULL;
>  	bo->preferred_domains = bp->preferred_domain ? bp->preferred_domain :
>  		bp->domain;
>  	bo->allowed_domains = bo->preferred_domains;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index 907fdf46d895..64337ff2ad63 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -89,8 +89,8 @@ struct amdgpu_bo {
>  	void				*metadata;
>  	u32				metadata_size;
>  	unsigned			prime_shared_count;
> -	/* list of all virtual address to which this bo is associated to */
> -	struct list_head		va;
> +	/* per VM structure for page tables and with virtual addresses */
> +	struct amdgpu_vm_bo_base	*vm_bo;
>  	/* Constant after initialization */
>  	struct drm_gem_object		gem_base;
>  	struct amdgpu_bo		*parent;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index cb6a5114128e..fb6b16273c54 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -309,12 +309,13 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
>  {
>  	base->vm = vm;
>  	base->bo = bo;
> -	INIT_LIST_HEAD(&base->bo_list);
> +	base->next = NULL;
>  	INIT_LIST_HEAD(&base->vm_status);
>  
>  	if (!bo)
>  		return;
> -	list_add_tail(&base->bo_list, &bo->va);
> +	base->next = bo->vm_bo;
> +	bo->vm_bo = base;
>  
>  	if (bo->tbo.resv != vm->root.base.bo->tbo.resv)
>  		return;
> @@ -352,7 +353,7 @@ static struct amdgpu_vm_pt *amdgpu_vm_pt_parent(struct amdgpu_vm_pt *pt)
>  	if (!parent)
>  		return NULL;
>  
> -	return list_first_entry(&parent->va, struct amdgpu_vm_pt, base.bo_list);
> +	return container_of(parent->vm_bo, struct amdgpu_vm_pt, base);
>  }
>  
>  /**
> @@ -954,7 +955,7 @@ static void amdgpu_vm_free_pts(struct amdgpu_device *adev,
>  	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, cursor, entry) {
>  
>  		if (entry->base.bo) {
> -			list_del(&entry->base.bo_list);
> +			entry->base.bo->vm_bo = NULL;
>  			list_del(&entry->base.vm_status);
>  			amdgpu_bo_unref(&entry->base.bo->shadow);
>  			amdgpu_bo_unref(&entry->base.bo);
> @@ -1162,12 +1163,13 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_
>  struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
>  				       struct amdgpu_bo *bo)
>  {
> -	struct amdgpu_bo_va *bo_va;
> +	struct amdgpu_vm_bo_base *base;
>  
> -	list_for_each_entry(bo_va, &bo->va, base.bo_list) {
> -		if (bo_va->base.vm == vm) {
> -			return bo_va;
> -		}
> +	for (base = bo->vm_bo; base; base = base->next) {
> +		if (base->vm != vm)
> +			continue;
> +
> +		return container_of(base, struct amdgpu_bo_va, base);
>  	}
>  	return NULL;
>  }
> @@ -2728,11 +2730,21 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
>  	struct amdgpu_bo_va_mapping *mapping, *next;
>  	struct amdgpu_bo *bo = bo_va->base.bo;
>  	struct amdgpu_vm *vm = bo_va->base.vm;
> +	struct amdgpu_vm_bo_base **base;
>  
> -	if (bo && bo->tbo.resv == vm->root.base.bo->tbo.resv)
> -		vm->bulk_moveable = false;
> +	if (bo) {
> +		if (bo->tbo.resv == vm->root.base.bo->tbo.resv)
> +			vm->bulk_moveable = false;
>  
> -	list_del(&bo_va->base.bo_list);
> +		for (base = &bo_va->base.bo->vm_bo; *base;
> +		     base = &(*base)->next) {
> +			if (*base != &bo_va->base)
> +				continue;
> +
> +			*base = bo_va->base.next;
> +			break;
> +		}
> +	}
>  
>  	spin_lock(&vm->invalidated_lock);
>  	list_del(&bo_va->base.vm_status);
> @@ -2774,7 +2786,7 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>  	if (bo->parent && bo->parent->shadow == bo)
>  		bo = bo->parent;
>  
> -	list_for_each_entry(bo_base, &bo->va, bo_list) {
> +	for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>  		struct amdgpu_vm *vm = bo_base->vm;
>  
>  		if (evicted && bo->tbo.resv == vm->root.base.bo->tbo.resv) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index e275ee7c1bc1..8966e40767eb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -128,7 +128,7 @@ struct amdgpu_vm_bo_base {
>  	struct amdgpu_bo		*bo;
>  
>  	/* protected by bo being reserved */
> -	struct list_head		bo_list;
> +	struct amdgpu_vm_bo_base	*next;
>  
>  	/* protected by spinlock */
>  	struct list_head		vm_status;

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

Re: [PATCH 2/2] drm/amdgpu: use a single linked list for amdgpu_vm_bo_base

[Christian König]

Am 13.09.2018 um 00:58 schrieb Felix Kuehling:
> Is the small reduction in memory footprint (8 bytes per page table on a
> 64-bit kernel) really worth the trouble of open-coding a single-linked
> list implementation?

Well the key point is it is now a power of two again. So we don't waste 
28KB per page table (on 4 levels) any more because it is rounded up to 
the next order size :)

> I guess this change makes a bigger difference for
> 2-level page tables than it does for 4-level, because the amdgpu_vm_pt
> array is allocated at the page directory level and includes page tables
> that don't even exist yet and may never exist. The amount of memory you
> save is the same as the size of the page directory.
>
> I wonder if the overhead could be reduced more effectively by allocating
> struct amdgpu_vm_pt with the page table, rather than with the page
> directory. Then the amdgpu_vm_pt.entries array would be an array of
> pointers instead. It could be an array[0] at the end of the structure
> since the number of entries is know then the page directory is
> allocated. The BO could also be embedded in the amdgpu_vm_pt structure
> so it doesn't need to be a separate allocation from the amdgpu_vm_pt.

Yeah, thought about that as well. But the change looked to invasive on 
first glance.

> Acked-by: Felix Kuehling <Felix.Kuehling@amd.com>

Thanks,
Christian.

>
> Regards,
>    Felix
>
>
> On 2018-09-12 04:55 AM, Christian König wrote:
>> Instead of the double linked list. Gets the size of amdgpu_vm_pt down to
>> 64 bytes again.
>>
>> We could even reduce it down to 32 bytes, but that would require some
>> rather extreme hacks.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  2 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  4 ++--
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 38 ++++++++++++++++++++----------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  2 +-
>>   4 files changed, 29 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index de990bdcdd6c..e6909252aefa 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -448,7 +448,7 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>>   		return -ENOMEM;
>>   	drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
>>   	INIT_LIST_HEAD(&bo->shadow_list);
>> -	INIT_LIST_HEAD(&bo->va);
>> +	bo->vm_bo = NULL;
>>   	bo->preferred_domains = bp->preferred_domain ? bp->preferred_domain :
>>   		bp->domain;
>>   	bo->allowed_domains = bo->preferred_domains;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> index 907fdf46d895..64337ff2ad63 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> @@ -89,8 +89,8 @@ struct amdgpu_bo {
>>   	void				*metadata;
>>   	u32				metadata_size;
>>   	unsigned			prime_shared_count;
>> -	/* list of all virtual address to which this bo is associated to */
>> -	struct list_head		va;
>> +	/* per VM structure for page tables and with virtual addresses */
>> +	struct amdgpu_vm_bo_base	*vm_bo;
>>   	/* Constant after initialization */
>>   	struct drm_gem_object		gem_base;
>>   	struct amdgpu_bo		*parent;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index cb6a5114128e..fb6b16273c54 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -309,12 +309,13 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
>>   {
>>   	base->vm = vm;
>>   	base->bo = bo;
>> -	INIT_LIST_HEAD(&base->bo_list);
>> +	base->next = NULL;
>>   	INIT_LIST_HEAD(&base->vm_status);
>>   
>>   	if (!bo)
>>   		return;
>> -	list_add_tail(&base->bo_list, &bo->va);
>> +	base->next = bo->vm_bo;
>> +	bo->vm_bo = base;
>>   
>>   	if (bo->tbo.resv != vm->root.base.bo->tbo.resv)
>>   		return;
>> @@ -352,7 +353,7 @@ static struct amdgpu_vm_pt *amdgpu_vm_pt_parent(struct amdgpu_vm_pt *pt)
>>   	if (!parent)
>>   		return NULL;
>>   
>> -	return list_first_entry(&parent->va, struct amdgpu_vm_pt, base.bo_list);
>> +	return container_of(parent->vm_bo, struct amdgpu_vm_pt, base);
>>   }
>>   
>>   /**
>> @@ -954,7 +955,7 @@ static void amdgpu_vm_free_pts(struct amdgpu_device *adev,
>>   	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, cursor, entry) {
>>   
>>   		if (entry->base.bo) {
>> -			list_del(&entry->base.bo_list);
>> +			entry->base.bo->vm_bo = NULL;
>>   			list_del(&entry->base.vm_status);
>>   			amdgpu_bo_unref(&entry->base.bo->shadow);
>>   			amdgpu_bo_unref(&entry->base.bo);
>> @@ -1162,12 +1163,13 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_
>>   struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
>>   				       struct amdgpu_bo *bo)
>>   {
>> -	struct amdgpu_bo_va *bo_va;
>> +	struct amdgpu_vm_bo_base *base;
>>   
>> -	list_for_each_entry(bo_va, &bo->va, base.bo_list) {
>> -		if (bo_va->base.vm == vm) {
>> -			return bo_va;
>> -		}
>> +	for (base = bo->vm_bo; base; base = base->next) {
>> +		if (base->vm != vm)
>> +			continue;
>> +
>> +		return container_of(base, struct amdgpu_bo_va, base);
>>   	}
>>   	return NULL;
>>   }
>> @@ -2728,11 +2730,21 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
>>   	struct amdgpu_bo_va_mapping *mapping, *next;
>>   	struct amdgpu_bo *bo = bo_va->base.bo;
>>   	struct amdgpu_vm *vm = bo_va->base.vm;
>> +	struct amdgpu_vm_bo_base **base;
>>   
>> -	if (bo && bo->tbo.resv == vm->root.base.bo->tbo.resv)
>> -		vm->bulk_moveable = false;
>> +	if (bo) {
>> +		if (bo->tbo.resv == vm->root.base.bo->tbo.resv)
>> +			vm->bulk_moveable = false;
>>   
>> -	list_del(&bo_va->base.bo_list);
>> +		for (base = &bo_va->base.bo->vm_bo; *base;
>> +		     base = &(*base)->next) {
>> +			if (*base != &bo_va->base)
>> +				continue;
>> +
>> +			*base = bo_va->base.next;
>> +			break;
>> +		}
>> +	}
>>   
>>   	spin_lock(&vm->invalidated_lock);
>>   	list_del(&bo_va->base.vm_status);
>> @@ -2774,7 +2786,7 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>>   	if (bo->parent && bo->parent->shadow == bo)
>>   		bo = bo->parent;
>>   
>> -	list_for_each_entry(bo_base, &bo->va, bo_list) {
>> +	for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>>   		struct amdgpu_vm *vm = bo_base->vm;
>>   
>>   		if (evicted && bo->tbo.resv == vm->root.base.bo->tbo.resv) {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index e275ee7c1bc1..8966e40767eb 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -128,7 +128,7 @@ struct amdgpu_vm_bo_base {
>>   	struct amdgpu_bo		*bo;
>>   
>>   	/* protected by bo being reserved */
>> -	struct list_head		bo_list;
>> +	struct amdgpu_vm_bo_base	*next;
>>   
>>   	/* protected by spinlock */
>>   	struct list_head		vm_status;

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

Re: [PATCH 1/2] drm/amdgpu: remove amdgpu_bo_list_entry.robj

[Christian König]

Am 12.09.2018 um 22:21 schrieb Felix Kuehling:
> On 2018-09-12 04:55 AM, Christian König wrote:
>> We can get that just by casting tv.bo.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 42 ++++++++++++---------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h |  1 -
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      | 58 ++++++++++++++++-------------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      |  3 +-
>>   4 files changed, 58 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>> index b80243d3972e..14d2982a47cc 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>> @@ -49,8 +49,11 @@ static void amdgpu_bo_list_free(struct kref *ref)
>>   						   refcount);
>>   	struct amdgpu_bo_list_entry *e;
>>   
>> -	amdgpu_bo_list_for_each_entry(e, list)
>> -		amdgpu_bo_unref(&e->robj);
>> +	amdgpu_bo_list_for_each_entry(e, list) {
>> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>> +
>> +		amdgpu_bo_unref(&bo);
>> +	}
>>   
>>   	call_rcu(&list->rhead, amdgpu_bo_list_free_rcu);
>>   }
>> @@ -112,21 +115,20 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp,
>>   			entry = &array[last_entry++];
>>   		}
>>   
>> -		entry->robj = bo;
>>   		entry->priority = min(info[i].bo_priority,
>>   				      AMDGPU_BO_LIST_MAX_PRIORITY);
>> -		entry->tv.bo = &entry->robj->tbo;
>> -		entry->tv.shared = !entry->robj->prime_shared_count;
>> -
>> -		if (entry->robj->preferred_domains == AMDGPU_GEM_DOMAIN_GDS)
>> -			list->gds_obj = entry->robj;
>> -		if (entry->robj->preferred_domains == AMDGPU_GEM_DOMAIN_GWS)
>> -			list->gws_obj = entry->robj;
>> -		if (entry->robj->preferred_domains == AMDGPU_GEM_DOMAIN_OA)
>> -			list->oa_obj = entry->robj;
>> -
>> -		total_size += amdgpu_bo_size(entry->robj);
>> -		trace_amdgpu_bo_list_set(list, entry->robj);
>> +		entry->tv.bo = &bo->tbo;
>> +		entry->tv.shared = !bo->prime_shared_count;
> You're no longer initializing entry->priority here. Is that intentional?

Hui? Please take another look, the initialization of entry->priority is 
not touched by this patch.

Christian.


>
> Regards,
>    Felix
>
>> +
>> +		if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_GDS)
>> +			list->gds_obj = bo;
>> +		if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_GWS)
>> +			list->gws_obj = bo;
>> +		if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_OA)
>> +			list->oa_obj = bo;
>> +
>> +		total_size += amdgpu_bo_size(bo);
>> +		trace_amdgpu_bo_list_set(list, bo);
>>   	}
>>   
>>   	list->first_userptr = first_userptr;
>> @@ -138,8 +140,11 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp,
>>   	return 0;
>>   
>>   error_free:
>> -	while (i--)
>> -		amdgpu_bo_unref(&array[i].robj);
>> +	while (i--) {
>> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(array[i].tv.bo);
>> +
>> +		amdgpu_bo_unref(&bo);
>> +	}
>>   	kvfree(list);
>>   	return r;
>>   
>> @@ -191,9 +196,10 @@ void amdgpu_bo_list_get_list(struct amdgpu_bo_list *list,
>>   	 * with the same priority, i.e. it must be stable.
>>   	 */
>>   	amdgpu_bo_list_for_each_entry(e, list) {
>> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>>   		unsigned priority = e->priority;
>>   
>> -		if (!e->robj->parent)
>> +		if (!bo->parent)
>>   			list_add_tail(&e->tv.head, &bucket[priority]);
>>   
>>   		e->user_pages = NULL;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
>> index 61b089768e1c..7c5f5d1601e6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
>> @@ -32,7 +32,6 @@ struct amdgpu_bo_va;
>>   struct amdgpu_fpriv;
>>   
>>   struct amdgpu_bo_list_entry {
>> -	struct amdgpu_bo		*robj;
>>   	struct ttm_validate_buffer	tv;
>>   	struct amdgpu_bo_va		*bo_va;
>>   	uint32_t			priority;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index c5cc648a1b4e..2e488c6f9562 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -39,6 +39,7 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p,
>>   				      uint32_t *offset)
>>   {
>>   	struct drm_gem_object *gobj;
>> +	struct amdgpu_bo *bo;
>>   	unsigned long size;
>>   	int r;
>>   
>> @@ -46,21 +47,21 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p,
>>   	if (gobj == NULL)
>>   		return -EINVAL;
>>   
>> -	p->uf_entry.robj = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj));
>> +	bo = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj));
>>   	p->uf_entry.priority = 0;
>> -	p->uf_entry.tv.bo = &p->uf_entry.robj->tbo;
>> +	p->uf_entry.tv.bo = &bo->tbo;
>>   	p->uf_entry.tv.shared = true;
>>   	p->uf_entry.user_pages = NULL;
>>   
>>   	drm_gem_object_put_unlocked(gobj);
>>   
>> -	size = amdgpu_bo_size(p->uf_entry.robj);
>> +	size = amdgpu_bo_size(bo);
>>   	if (size != PAGE_SIZE || (data->offset + 8) > size) {
>>   		r = -EINVAL;
>>   		goto error_unref;
>>   	}
>>   
>> -	if (amdgpu_ttm_tt_get_usermm(p->uf_entry.robj->tbo.ttm)) {
>> +	if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm)) {
>>   		r = -EINVAL;
>>   		goto error_unref;
>>   	}
>> @@ -70,7 +71,7 @@ static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p,
>>   	return 0;
>>   
>>   error_unref:
>> -	amdgpu_bo_unref(&p->uf_entry.robj);
>> +	amdgpu_bo_unref(&bo);
>>   	return r;
>>   }
>>   
>> @@ -229,7 +230,7 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, union drm_amdgpu_cs
>>   		goto free_all_kdata;
>>   	}
>>   
>> -	if (p->uf_entry.robj)
>> +	if (p->uf_entry.tv.bo)
>>   		p->job->uf_addr = uf_offset;
>>   	kfree(chunk_array);
>>   
>> @@ -458,13 +459,13 @@ static bool amdgpu_cs_try_evict(struct amdgpu_cs_parser *p,
>>   	     p->evictable = list_prev_entry(p->evictable, tv.head)) {
>>   
>>   		struct amdgpu_bo_list_entry *candidate = p->evictable;
>> -		struct amdgpu_bo *bo = candidate->robj;
>> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(candidate->tv.bo);
>>   		struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>>   		bool update_bytes_moved_vis;
>>   		uint32_t other;
>>   
>>   		/* If we reached our current BO we can forget it */
>> -		if (candidate->robj == validated)
>> +		if (bo == validated)
>>   			break;
>>   
>>   		/* We can't move pinned BOs here */
>> @@ -529,7 +530,7 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
>>   	int r;
>>   
>>   	list_for_each_entry(lobj, validated, tv.head) {
>> -		struct amdgpu_bo *bo = lobj->robj;
>> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(lobj->tv.bo);
>>   		bool binding_userptr = false;
>>   		struct mm_struct *usermm;
>>   
>> @@ -604,7 +605,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>>   	INIT_LIST_HEAD(&duplicates);
>>   	amdgpu_vm_get_pd_bo(&fpriv->vm, &p->validated, &p->vm_pd);
>>   
>> -	if (p->uf_entry.robj && !p->uf_entry.robj->parent)
>> +	if (p->uf_entry.tv.bo && !ttm_to_amdgpu_bo(p->uf_entry.tv.bo)->parent)
>>   		list_add(&p->uf_entry.tv.head, &p->validated);
>>   
>>   	while (1) {
>> @@ -620,7 +621,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>>   
>>   		INIT_LIST_HEAD(&need_pages);
>>   		amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
>> -			struct amdgpu_bo *bo = e->robj;
>> +			struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>>   
>>   			if (amdgpu_ttm_tt_userptr_invalidated(bo->tbo.ttm,
>>   				 &e->user_invalidated) && e->user_pages) {
>> @@ -639,7 +640,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>>   				list_del(&e->tv.head);
>>   				list_add(&e->tv.head, &need_pages);
>>   
>> -				amdgpu_bo_unreserve(e->robj);
>> +				amdgpu_bo_unreserve(bo);
>>   			}
>>   		}
>>   
>> @@ -658,7 +659,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>>   
>>   		/* Fill the page arrays for all userptrs. */
>>   		list_for_each_entry(e, &need_pages, tv.head) {
>> -			struct ttm_tt *ttm = e->robj->tbo.ttm;
>> +			struct ttm_tt *ttm = e->tv.bo->ttm;
>>   
>>   			e->user_pages = kvmalloc_array(ttm->num_pages,
>>   							 sizeof(struct page*),
>> @@ -717,7 +718,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>>   	oa = p->bo_list->oa_obj;
>>   
>>   	amdgpu_bo_list_for_each_entry(e, p->bo_list)
>> -		e->bo_va = amdgpu_vm_bo_find(vm, e->robj);
>> +		e->bo_va = amdgpu_vm_bo_find(vm, ttm_to_amdgpu_bo(e->tv.bo));
>>   
>>   	if (gds) {
>>   		p->job->gds_base = amdgpu_bo_gpu_offset(gds);
>> @@ -732,8 +733,8 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>>   		p->job->oa_size = amdgpu_bo_size(oa);
>>   	}
>>   
>> -	if (!r && p->uf_entry.robj) {
>> -		struct amdgpu_bo *uf = p->uf_entry.robj;
>> +	if (!r && p->uf_entry.tv.bo) {
>> +		struct amdgpu_bo *uf = ttm_to_amdgpu_bo(p->uf_entry.tv.bo);
>>   
>>   		r = amdgpu_ttm_alloc_gart(&uf->tbo);
>>   		p->job->uf_addr += amdgpu_bo_gpu_offset(uf);
>> @@ -749,8 +750,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>>   		if (!e->user_pages)
>>   			continue;
>>   
>> -		release_pages(e->user_pages,
>> -			      e->robj->tbo.ttm->num_pages);
>> +		release_pages(e->user_pages, e->tv.bo->ttm->num_pages);
>>   		kvfree(e->user_pages);
>>   	}
>>   
>> @@ -763,9 +763,11 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
>>   	int r;
>>   
>>   	list_for_each_entry(e, &p->validated, tv.head) {
>> -		struct reservation_object *resv = e->robj->tbo.resv;
>> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>> +		struct reservation_object *resv = bo->tbo.resv;
>> +
>>   		r = amdgpu_sync_resv(p->adev, &p->job->sync, resv, p->filp,
>> -				     amdgpu_bo_explicit_sync(e->robj));
>> +				     amdgpu_bo_explicit_sync(bo));
>>   
>>   		if (r)
>>   			return r;
>> @@ -808,7 +810,11 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error,
>>   	kfree(parser->chunks);
>>   	if (parser->job)
>>   		amdgpu_job_free(parser->job);
>> -	amdgpu_bo_unref(&parser->uf_entry.robj);
>> +	if (parser->uf_entry.tv.bo) {
>> +		struct amdgpu_bo *uf = ttm_to_amdgpu_bo(parser->uf_entry.tv.bo);
>> +
>> +		amdgpu_bo_unref(&uf);
>> +	}
>>   }
>>   
>>   static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>> @@ -919,7 +925,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>>   		struct dma_fence *f;
>>   
>>   		/* ignore duplicates */
>> -		bo = e->robj;
>> +		bo = ttm_to_amdgpu_bo(e->tv.bo);
>>   		if (!bo)
>>   			continue;
>>   
>> @@ -958,11 +964,13 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>>   	if (amdgpu_vm_debug) {
>>   		/* Invalidate all BOs to test for userspace bugs */
>>   		amdgpu_bo_list_for_each_entry(e, p->bo_list) {
>> +			struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>> +
>>   			/* ignore duplicates */
>> -			if (!e->robj)
>> +			if (!bo)
>>   				continue;
>>   
>> -			amdgpu_vm_bo_invalidate(adev, e->robj, false);
>> +			amdgpu_vm_bo_invalidate(adev, bo, false);
>>   		}
>>   	}
>>   
>> @@ -1211,7 +1219,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>>   	/* No memory allocation is allowed while holding the mn lock */
>>   	amdgpu_mn_lock(p->mn);
>>   	amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
>> -		struct amdgpu_bo *bo = e->robj;
>> +		struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>>   
>>   		if (amdgpu_ttm_tt_userptr_needs_pages(bo->tbo.ttm)) {
>>   			r = -ERESTARTSYS;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 3e37b119371d..cb6a5114128e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -593,9 +593,8 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>>   			 struct list_head *validated,
>>   			 struct amdgpu_bo_list_entry *entry)
>>   {
>> -	entry->robj = vm->root.base.bo;
>>   	entry->priority = 0;
>> -	entry->tv.bo = &entry->robj->tbo;
>> +	entry->tv.bo = &vm->root.base.bo->tbo;
>>   	entry->tv.shared = true;
>>   	entry->user_pages = NULL;
>>   	list_add(&entry->tv.head, validated);

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

Re: [PATCH 1/2] drm/amdgpu: remove amdgpu_bo_list_entry.robj

[Felix Kuehling]

On 2018-09-13 01:50 PM, Christian König wrote:
> Am 12.09.2018 um 22:21 schrieb Felix Kuehling:
>> On 2018-09-12 04:55 AM, Christian König wrote:
>>> We can get that just by casting tv.bo.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 42
>>> ++++++++++++---------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h |  1 -
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      | 58
>>> ++++++++++++++++-------------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      |  3 +-
>>>   4 files changed, 58 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>>> index b80243d3972e..14d2982a47cc 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>>> @@ -49,8 +49,11 @@ static void amdgpu_bo_list_free(struct kref *ref)
>>>                              refcount);
>>>       struct amdgpu_bo_list_entry *e;
>>>   -    amdgpu_bo_list_for_each_entry(e, list)
>>> -        amdgpu_bo_unref(&e->robj);
>>> +    amdgpu_bo_list_for_each_entry(e, list) {
>>> +        struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>>> +
>>> +        amdgpu_bo_unref(&bo);
>>> +    }
>>>         call_rcu(&list->rhead, amdgpu_bo_list_free_rcu);
>>>   }
>>> @@ -112,21 +115,20 @@ int amdgpu_bo_list_create(struct amdgpu_device
>>> *adev, struct drm_file *filp,
>>>               entry = &array[last_entry++];
>>>           }
>>>   -        entry->robj = bo;
>>>           entry->priority = min(info[i].bo_priority,
>>>                         AMDGPU_BO_LIST_MAX_PRIORITY);
>>> -        entry->tv.bo = &entry->robj->tbo;
>>> -        entry->tv.shared = !entry->robj->prime_shared_count;
>>> -
>>> -        if (entry->robj->preferred_domains == AMDGPU_GEM_DOMAIN_GDS)
>>> -            list->gds_obj = entry->robj;
>>> -        if (entry->robj->preferred_domains == AMDGPU_GEM_DOMAIN_GWS)
>>> -            list->gws_obj = entry->robj;
>>> -        if (entry->robj->preferred_domains == AMDGPU_GEM_DOMAIN_OA)
>>> -            list->oa_obj = entry->robj;
>>> -
>>> -        total_size += amdgpu_bo_size(entry->robj);
>>> -        trace_amdgpu_bo_list_set(list, entry->robj);
>>> +        entry->tv.bo = &bo->tbo;
>>> +        entry->tv.shared = !bo->prime_shared_count;
>> You're no longer initializing entry->priority here. Is that intentional?
>
> Hui? Please take another look, the initialization of entry->priority
> is not touched by this patch.

Sorry, I misread it. I saw a "-" where there was none between all the
other lines being removed/replaced.

Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

>
> Christian.
>
>
>>
>> Regards,
>>    Felix
>>
>>> +
>>> +        if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_GDS)
>>> +            list->gds_obj = bo;
>>> +        if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_GWS)
>>> +            list->gws_obj = bo;
>>> +        if (bo->preferred_domains == AMDGPU_GEM_DOMAIN_OA)
>>> +            list->oa_obj = bo;
>>> +
>>> +        total_size += amdgpu_bo_size(bo);
>>> +        trace_amdgpu_bo_list_set(list, bo);
>>>       }
>>>         list->first_userptr = first_userptr;
>>> @@ -138,8 +140,11 @@ int amdgpu_bo_list_create(struct amdgpu_device
>>> *adev, struct drm_file *filp,
>>>       return 0;
>>>     error_free:
>>> -    while (i--)
>>> -        amdgpu_bo_unref(&array[i].robj);
>>> +    while (i--) {
>>> +        struct amdgpu_bo *bo = ttm_to_amdgpu_bo(array[i].tv.bo);
>>> +
>>> +        amdgpu_bo_unref(&bo);
>>> +    }
>>>       kvfree(list);
>>>       return r;
>>>   @@ -191,9 +196,10 @@ void amdgpu_bo_list_get_list(struct
>>> amdgpu_bo_list *list,
>>>        * with the same priority, i.e. it must be stable.
>>>        */
>>>       amdgpu_bo_list_for_each_entry(e, list) {
>>> +        struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>>>           unsigned priority = e->priority;
>>>   -        if (!e->robj->parent)
>>> +        if (!bo->parent)
>>>               list_add_tail(&e->tv.head, &bucket[priority]);
>>>             e->user_pages = NULL;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
>>> index 61b089768e1c..7c5f5d1601e6 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
>>> @@ -32,7 +32,6 @@ struct amdgpu_bo_va;
>>>   struct amdgpu_fpriv;
>>>     struct amdgpu_bo_list_entry {
>>> -    struct amdgpu_bo        *robj;
>>>       struct ttm_validate_buffer    tv;
>>>       struct amdgpu_bo_va        *bo_va;
>>>       uint32_t            priority;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index c5cc648a1b4e..2e488c6f9562 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -39,6 +39,7 @@ static int amdgpu_cs_user_fence_chunk(struct
>>> amdgpu_cs_parser *p,
>>>                         uint32_t *offset)
>>>   {
>>>       struct drm_gem_object *gobj;
>>> +    struct amdgpu_bo *bo;
>>>       unsigned long size;
>>>       int r;
>>>   @@ -46,21 +47,21 @@ static int amdgpu_cs_user_fence_chunk(struct
>>> amdgpu_cs_parser *p,
>>>       if (gobj == NULL)
>>>           return -EINVAL;
>>>   -    p->uf_entry.robj = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj));
>>> +    bo = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj));
>>>       p->uf_entry.priority = 0;
>>> -    p->uf_entry.tv.bo = &p->uf_entry.robj->tbo;
>>> +    p->uf_entry.tv.bo = &bo->tbo;
>>>       p->uf_entry.tv.shared = true;
>>>       p->uf_entry.user_pages = NULL;
>>>         drm_gem_object_put_unlocked(gobj);
>>>   -    size = amdgpu_bo_size(p->uf_entry.robj);
>>> +    size = amdgpu_bo_size(bo);
>>>       if (size != PAGE_SIZE || (data->offset + 8) > size) {
>>>           r = -EINVAL;
>>>           goto error_unref;
>>>       }
>>>   -    if (amdgpu_ttm_tt_get_usermm(p->uf_entry.robj->tbo.ttm)) {
>>> +    if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm)) {
>>>           r = -EINVAL;
>>>           goto error_unref;
>>>       }
>>> @@ -70,7 +71,7 @@ static int amdgpu_cs_user_fence_chunk(struct
>>> amdgpu_cs_parser *p,
>>>       return 0;
>>>     error_unref:
>>> -    amdgpu_bo_unref(&p->uf_entry.robj);
>>> +    amdgpu_bo_unref(&bo);
>>>       return r;
>>>   }
>>>   @@ -229,7 +230,7 @@ static int amdgpu_cs_parser_init(struct
>>> amdgpu_cs_parser *p, union drm_amdgpu_cs
>>>           goto free_all_kdata;
>>>       }
>>>   -    if (p->uf_entry.robj)
>>> +    if (p->uf_entry.tv.bo)
>>>           p->job->uf_addr = uf_offset;
>>>       kfree(chunk_array);
>>>   @@ -458,13 +459,13 @@ static bool amdgpu_cs_try_evict(struct
>>> amdgpu_cs_parser *p,
>>>            p->evictable = list_prev_entry(p->evictable, tv.head)) {
>>>             struct amdgpu_bo_list_entry *candidate = p->evictable;
>>> -        struct amdgpu_bo *bo = candidate->robj;
>>> +        struct amdgpu_bo *bo = ttm_to_amdgpu_bo(candidate->tv.bo);
>>>           struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>>>           bool update_bytes_moved_vis;
>>>           uint32_t other;
>>>             /* If we reached our current BO we can forget it */
>>> -        if (candidate->robj == validated)
>>> +        if (bo == validated)
>>>               break;
>>>             /* We can't move pinned BOs here */
>>> @@ -529,7 +530,7 @@ static int amdgpu_cs_list_validate(struct
>>> amdgpu_cs_parser *p,
>>>       int r;
>>>         list_for_each_entry(lobj, validated, tv.head) {
>>> -        struct amdgpu_bo *bo = lobj->robj;
>>> +        struct amdgpu_bo *bo = ttm_to_amdgpu_bo(lobj->tv.bo);
>>>           bool binding_userptr = false;
>>>           struct mm_struct *usermm;
>>>   @@ -604,7 +605,7 @@ static int amdgpu_cs_parser_bos(struct
>>> amdgpu_cs_parser *p,
>>>       INIT_LIST_HEAD(&duplicates);
>>>       amdgpu_vm_get_pd_bo(&fpriv->vm, &p->validated, &p->vm_pd);
>>>   -    if (p->uf_entry.robj && !p->uf_entry.robj->parent)
>>> +    if (p->uf_entry.tv.bo &&
>>> !ttm_to_amdgpu_bo(p->uf_entry.tv.bo)->parent)
>>>           list_add(&p->uf_entry.tv.head, &p->validated);
>>>         while (1) {
>>> @@ -620,7 +621,7 @@ static int amdgpu_cs_parser_bos(struct
>>> amdgpu_cs_parser *p,
>>>             INIT_LIST_HEAD(&need_pages);
>>>           amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
>>> -            struct amdgpu_bo *bo = e->robj;
>>> +            struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>>>                 if (amdgpu_ttm_tt_userptr_invalidated(bo->tbo.ttm,
>>>                    &e->user_invalidated) && e->user_pages) {
>>> @@ -639,7 +640,7 @@ static int amdgpu_cs_parser_bos(struct
>>> amdgpu_cs_parser *p,
>>>                   list_del(&e->tv.head);
>>>                   list_add(&e->tv.head, &need_pages);
>>>   -                amdgpu_bo_unreserve(e->robj);
>>> +                amdgpu_bo_unreserve(bo);
>>>               }
>>>           }
>>>   @@ -658,7 +659,7 @@ static int amdgpu_cs_parser_bos(struct
>>> amdgpu_cs_parser *p,
>>>             /* Fill the page arrays for all userptrs. */
>>>           list_for_each_entry(e, &need_pages, tv.head) {
>>> -            struct ttm_tt *ttm = e->robj->tbo.ttm;
>>> +            struct ttm_tt *ttm = e->tv.bo->ttm;
>>>                 e->user_pages = kvmalloc_array(ttm->num_pages,
>>>                                sizeof(struct page*),
>>> @@ -717,7 +718,7 @@ static int amdgpu_cs_parser_bos(struct
>>> amdgpu_cs_parser *p,
>>>       oa = p->bo_list->oa_obj;
>>>         amdgpu_bo_list_for_each_entry(e, p->bo_list)
>>> -        e->bo_va = amdgpu_vm_bo_find(vm, e->robj);
>>> +        e->bo_va = amdgpu_vm_bo_find(vm, ttm_to_amdgpu_bo(e->tv.bo));
>>>         if (gds) {
>>>           p->job->gds_base = amdgpu_bo_gpu_offset(gds);
>>> @@ -732,8 +733,8 @@ static int amdgpu_cs_parser_bos(struct
>>> amdgpu_cs_parser *p,
>>>           p->job->oa_size = amdgpu_bo_size(oa);
>>>       }
>>>   -    if (!r && p->uf_entry.robj) {
>>> -        struct amdgpu_bo *uf = p->uf_entry.robj;
>>> +    if (!r && p->uf_entry.tv.bo) {
>>> +        struct amdgpu_bo *uf = ttm_to_amdgpu_bo(p->uf_entry.tv.bo);
>>>             r = amdgpu_ttm_alloc_gart(&uf->tbo);
>>>           p->job->uf_addr += amdgpu_bo_gpu_offset(uf);
>>> @@ -749,8 +750,7 @@ static int amdgpu_cs_parser_bos(struct
>>> amdgpu_cs_parser *p,
>>>           if (!e->user_pages)
>>>               continue;
>>>   -        release_pages(e->user_pages,
>>> -                  e->robj->tbo.ttm->num_pages);
>>> +        release_pages(e->user_pages, e->tv.bo->ttm->num_pages);
>>>           kvfree(e->user_pages);
>>>       }
>>>   @@ -763,9 +763,11 @@ static int amdgpu_cs_sync_rings(struct
>>> amdgpu_cs_parser *p)
>>>       int r;
>>>         list_for_each_entry(e, &p->validated, tv.head) {
>>> -        struct reservation_object *resv = e->robj->tbo.resv;
>>> +        struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>>> +        struct reservation_object *resv = bo->tbo.resv;
>>> +
>>>           r = amdgpu_sync_resv(p->adev, &p->job->sync, resv, p->filp,
>>> -                     amdgpu_bo_explicit_sync(e->robj));
>>> +                     amdgpu_bo_explicit_sync(bo));
>>>             if (r)
>>>               return r;
>>> @@ -808,7 +810,11 @@ static void amdgpu_cs_parser_fini(struct
>>> amdgpu_cs_parser *parser, int error,
>>>       kfree(parser->chunks);
>>>       if (parser->job)
>>>           amdgpu_job_free(parser->job);
>>> -    amdgpu_bo_unref(&parser->uf_entry.robj);
>>> +    if (parser->uf_entry.tv.bo) {
>>> +        struct amdgpu_bo *uf =
>>> ttm_to_amdgpu_bo(parser->uf_entry.tv.bo);
>>> +
>>> +        amdgpu_bo_unref(&uf);
>>> +    }
>>>   }
>>>     static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>>> @@ -919,7 +925,7 @@ static int amdgpu_cs_vm_handling(struct
>>> amdgpu_cs_parser *p)
>>>           struct dma_fence *f;
>>>             /* ignore duplicates */
>>> -        bo = e->robj;
>>> +        bo = ttm_to_amdgpu_bo(e->tv.bo);
>>>           if (!bo)
>>>               continue;
>>>   @@ -958,11 +964,13 @@ static int amdgpu_cs_vm_handling(struct
>>> amdgpu_cs_parser *p)
>>>       if (amdgpu_vm_debug) {
>>>           /* Invalidate all BOs to test for userspace bugs */
>>>           amdgpu_bo_list_for_each_entry(e, p->bo_list) {
>>> +            struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>>> +
>>>               /* ignore duplicates */
>>> -            if (!e->robj)
>>> +            if (!bo)
>>>                   continue;
>>>   -            amdgpu_vm_bo_invalidate(adev, e->robj, false);
>>> +            amdgpu_vm_bo_invalidate(adev, bo, false);
>>>           }
>>>       }
>>>   @@ -1211,7 +1219,7 @@ static int amdgpu_cs_submit(struct
>>> amdgpu_cs_parser *p,
>>>       /* No memory allocation is allowed while holding the mn lock */
>>>       amdgpu_mn_lock(p->mn);
>>>       amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
>>> -        struct amdgpu_bo *bo = e->robj;
>>> +        struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
>>>             if (amdgpu_ttm_tt_userptr_needs_pages(bo->tbo.ttm)) {
>>>               r = -ERESTARTSYS;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 3e37b119371d..cb6a5114128e 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -593,9 +593,8 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>>>                struct list_head *validated,
>>>                struct amdgpu_bo_list_entry *entry)
>>>   {
>>> -    entry->robj = vm->root.base.bo;
>>>       entry->priority = 0;
>>> -    entry->tv.bo = &entry->robj->tbo;
>>> +    entry->tv.bo = &vm->root.base.bo->tbo;
>>>       entry->tv.shared = true;
>>>       entry->user_pages = NULL;
>>>       list_add(&entry->tv.head, validated);
>

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