[PATCH] drm/i915/ttm: consider all placements for the page alignment

[PATCH] drm/i915/ttm: consider all placements for the page alignment

[Matthew Auld]

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;
-- 
2.26.3

Re: [PATCH] drm/i915/ttm: consider all placements for the page alignment

[Thomas Hellström]

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

Re: [Intel-gfx] [PATCH] drm/i915/ttm: consider all placements for the page alignment

[Matthew Auld]

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

Re: [Intel-gfx] [PATCH] drm/i915/ttm: consider all placements for the page alignment

[Thomas Hellström]

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

Re: [Intel-gfx] [PATCH] drm/i915/ttm: consider all placements for the page alignment

[Matthew Auld]

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
> 
>