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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Wed, 2021-06-23 at 12:26 +0100, 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;
> +}
> +

I think if at all possible, we really should try to avoid the above.
Could we, just like in your next patch, perhaps set alignment to 0,
indicating that we don't care at the per-object level and something
else, indicating that we care.

Then the manager could use its default if we don't care and the
indicated alignment, even if it's less, if we care at the per object
level? 

/Thomas

>  /**
>   * __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;


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