Just checking the current region is not enough, if we later migrate the object somewhere else. For example if the placements are {SMEM, LMEM}, then we might get this wrong. Another idea might be to make the page_alignment part of the ttm_place, instead of the BO.
Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com --- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index c5deb8b7227c..5d894bba6430 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -753,6 +753,25 @@ void i915_ttm_bo_destroy(struct ttm_buffer_object *bo) call_rcu(&obj->rcu, __i915_gem_free_object_rcu); }
+static u64 i915_gem_object_page_size(struct drm_i915_gem_object *obj) +{ + u64 page_size; + int i; + + if (!obj->mm.n_placements) + return obj->mm.region->min_page_size; + + page_size = 0; + for (i = 0; i < obj->mm.n_placements; i++) { + struct intel_memory_region *mr = obj->mm.placements[i]; + + page_size = max_t(u64, mr->min_page_size, page_size); + } + + GEM_BUG_ON(!page_size); + return page_size; +} + /** * __i915_gem_ttm_object_init - Initialize a ttm-backed i915 gem object * @mem: The initial memory region for the object. @@ -793,7 +812,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, obj->base.vma_node.driver_private = i915_gem_to_ttm(obj); ret = ttm_bo_init(&i915->bdev, i915_gem_to_ttm(obj), size, bo_type, &i915_sys_placement, - mem->min_page_size >> PAGE_SHIFT, + i915_gem_object_page_size(obj) >> PAGE_SHIFT, true, NULL, NULL, i915_ttm_bo_destroy); if (!ret) obj->ttm.created = true;
On 6/22/21 11:58 AM, Matthew Auld wrote:
Just checking the current region is not enough, if we later migrate the object somewhere else. For example if the placements are {SMEM, LMEM}, then we might get this wrong. Another idea might be to make the page_alignment part of the ttm_place, instead of the BO.
Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index c5deb8b7227c..5d894bba6430 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -753,6 +753,25 @@ void i915_ttm_bo_destroy(struct ttm_buffer_object *bo) call_rcu(&obj->rcu, __i915_gem_free_object_rcu); }
+static u64 i915_gem_object_page_size(struct drm_i915_gem_object *obj) +{
- u64 page_size;
- int i;
- if (!obj->mm.n_placements)
return obj->mm.region->min_page_size;- page_size = 0;
- for (i = 0; i < obj->mm.n_placements; i++) {
struct intel_memory_region *mr = obj->mm.placements[i];page_size = max_t(u64, mr->min_page_size, page_size);- }
- GEM_BUG_ON(!page_size);
- return page_size;
+}
- /**
- __i915_gem_ttm_object_init - Initialize a ttm-backed i915 gem object
- @mem: The initial memory region for the object.
@@ -793,7 +812,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, obj->base.vma_node.driver_private = i915_gem_to_ttm(obj); ret = ttm_bo_init(&i915->bdev, i915_gem_to_ttm(obj), size, bo_type, &i915_sys_placement,
mem->min_page_size >> PAGE_SHIFT,
i915_gem_object_page_size(obj) >> PAGE_SHIFT,
Hmm, can't we just have the buddy manager silently enforce its min_page_size?
/Thomas
On Tue, 22 Jun 2021 at 11:11, Thomas Hellström thomas.hellstrom@linux.intel.com wrote:
On 6/22/21 11:58 AM, Matthew Auld wrote:
Just checking the current region is not enough, if we later migrate the object somewhere else. For example if the placements are {SMEM, LMEM}, then we might get this wrong. Another idea might be to make the page_alignment part of the ttm_place, instead of the BO.
Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index c5deb8b7227c..5d894bba6430 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -753,6 +753,25 @@ void i915_ttm_bo_destroy(struct ttm_buffer_object *bo) call_rcu(&obj->rcu, __i915_gem_free_object_rcu); }
+static u64 i915_gem_object_page_size(struct drm_i915_gem_object *obj) +{
u64 page_size;int i;if (!obj->mm.n_placements)return obj->mm.region->min_page_size;page_size = 0;for (i = 0; i < obj->mm.n_placements; i++) {struct intel_memory_region *mr = obj->mm.placements[i];page_size = max_t(u64, mr->min_page_size, page_size);}GEM_BUG_ON(!page_size);return page_size;+}
- /**
- __i915_gem_ttm_object_init - Initialize a ttm-backed i915 gem object
- @mem: The initial memory region for the object.
@@ -793,7 +812,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, obj->base.vma_node.driver_private = i915_gem_to_ttm(obj); ret = ttm_bo_init(&i915->bdev, i915_gem_to_ttm(obj), size, bo_type, &i915_sys_placement,
mem->min_page_size >> PAGE_SHIFT,
i915_gem_object_page_size(obj) >> PAGE_SHIFT,Hmm, can't we just have the buddy manager silently enforce its min_page_size?
Maybe, but we need some way of overriding it for all of our page-table allocations(and some other stuff also), so being able to control the page_alignment at the object level here seems reasonable? Could maybe pass it through with create_lmem_with_page_size(..., page_size)? Ok, it might be best to first type it and then see how it will all fit together.
/Thomas
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 6/22/21 2:15 PM, Matthew Auld wrote:
On Tue, 22 Jun 2021 at 11:11, Thomas Hellström thomas.hellstrom@linux.intel.com wrote:
On 6/22/21 11:58 AM, Matthew Auld wrote:
Just checking the current region is not enough, if we later migrate the object somewhere else. For example if the placements are {SMEM, LMEM}, then we might get this wrong. Another idea might be to make the page_alignment part of the ttm_place, instead of the BO.
Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index c5deb8b7227c..5d894bba6430 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -753,6 +753,25 @@ void i915_ttm_bo_destroy(struct ttm_buffer_object *bo) call_rcu(&obj->rcu, __i915_gem_free_object_rcu); }
+static u64 i915_gem_object_page_size(struct drm_i915_gem_object *obj) +{
u64 page_size;int i;if (!obj->mm.n_placements)return obj->mm.region->min_page_size;page_size = 0;for (i = 0; i < obj->mm.n_placements; i++) {struct intel_memory_region *mr = obj->mm.placements[i];page_size = max_t(u64, mr->min_page_size, page_size);}GEM_BUG_ON(!page_size);return page_size;+}
- /**
- __i915_gem_ttm_object_init - Initialize a ttm-backed i915 gem object
- @mem: The initial memory region for the object.
@@ -793,7 +812,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, obj->base.vma_node.driver_private = i915_gem_to_ttm(obj); ret = ttm_bo_init(&i915->bdev, i915_gem_to_ttm(obj), size, bo_type, &i915_sys_placement,
mem->min_page_size >> PAGE_SHIFT,
i915_gem_object_page_size(obj) >> PAGE_SHIFT,Hmm, can't we just have the buddy manager silently enforce its min_page_size?
Maybe, but we need some way of overriding it for all of our page-table allocations(and some other stuff also), so being able to control the page_alignment at the object level here seems reasonable? Could maybe pass it through with create_lmem_with_page_size(..., page_size)? Ok, it might be best to first type it and then see how it will all fit together.
Hmm, OK, I'm not 100% sure what the various requirements are here on the object level. But for region requirements, I think we've historically enforced that through the manager, taking also the bo->page_alignment into account and applying the larger of the two,
There is an example in vmw_thp_insert_aligned().
/Thomas
On 22/06/2021 13:29, Thomas Hellström wrote:
On 6/22/21 2:15 PM, Matthew Auld wrote:
On Tue, 22 Jun 2021 at 11:11, Thomas Hellström thomas.hellstrom@linux.intel.com wrote:
On 6/22/21 11:58 AM, Matthew Auld wrote:
Just checking the current region is not enough, if we later migrate the object somewhere else. For example if the placements are {SMEM, LMEM}, then we might get this wrong. Another idea might be to make the page_alignment part of the ttm_place, instead of the BO.
Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index c5deb8b7227c..5d894bba6430 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -753,6 +753,25 @@ void i915_ttm_bo_destroy(struct ttm_buffer_object *bo) call_rcu(&obj->rcu, __i915_gem_free_object_rcu); }
+static u64 i915_gem_object_page_size(struct drm_i915_gem_object *obj) +{ + u64 page_size; + int i;
+ if (!obj->mm.n_placements) + return obj->mm.region->min_page_size;
+ page_size = 0; + for (i = 0; i < obj->mm.n_placements; i++) { + struct intel_memory_region *mr = obj->mm.placements[i];
+ page_size = max_t(u64, mr->min_page_size, page_size); + }
+ GEM_BUG_ON(!page_size); + return page_size; +}
/** * __i915_gem_ttm_object_init - Initialize a ttm-backed i915 gem object * @mem: The initial memory region for the object. @@ -793,7 +812,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, obj->base.vma_node.driver_private = i915_gem_to_ttm(obj); ret = ttm_bo_init(&i915->bdev, i915_gem_to_ttm(obj), size, bo_type, &i915_sys_placement, - mem->min_page_size >> PAGE_SHIFT, + i915_gem_object_page_size(obj) >> PAGE_SHIFT,
Hmm, can't we just have the buddy manager silently enforce its min_page_size?
Maybe, but we need some way of overriding it for all of our page-table allocations(and some other stuff also), so being able to control the page_alignment at the object level here seems reasonable? Could maybe pass it through with create_lmem_with_page_size(..., page_size)? Ok, it might be best to first type it and then see how it will all fit together.
Hmm, OK, I'm not 100% sure what the various requirements are here on the object level. But for region requirements, I think we've historically enforced that through the manager, taking also the bo->page_alignment into account and applying the larger of the two,
There is an example in vmw_thp_insert_aligned().
Yeah, so for our use case we need to support page_alignment < min_page_size, for page-tables(4K). I guess pushing the min_page_size into buddy_man, and then letting page_alignment override that, with the added caveat that it can be smaller could work. Otherwise just using mm.chunk_size would already fit the bill quite nicely.
/Thomas
dri-devel@lists.freedesktop.org
-
Matthew Auld -
Matthew Auld -
Thomas Hellström