| Message ID | 20210622095843.132549-1-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/ttm: consider all placements for the page alignment | expand |
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 > >
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;
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(-)