| Message ID | 20210715223900.1840576-5-jason@jlekstrand.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Migrate memory to SMEM when imported cross-device | expand |
On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand <jason@jlekstrand.net> wrote: > > __i915_gem_ttm_object_init() was ignoring the placement requests coming > from the client and always placing all BOs in SMEM upon creation. > Instead, compute the requested placement set from the object and pass > that into ttm_bo_init_reserved(). AFAIK this is intentional, where we use SYS as a safe placeholder specifically for object creation, since that avoids allocating any actual pages. Later at get_pages() time we do the real allocation, where we need to consider the placements. If we set the real placements here, the ttm_bo_validate() call in init_reserved() might allocate the backing pages here for the lmem-only case, which is not what we want in i915. > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > index 6589411396d3f..d30f274c329c7 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > @@ -898,6 +898,8 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, > { > static struct lock_class_key lock_class; > struct drm_i915_private *i915 = mem->i915; > + struct ttm_place requested, busy[I915_TTM_MAX_PLACEMENTS]; > + struct ttm_placement placement; > struct ttm_operation_ctx ctx = { > .interruptible = true, > .no_wait_gpu = false, > @@ -919,6 +921,9 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, > /* Forcing the page size is kernel internal only */ > GEM_BUG_ON(page_size && obj->mm.n_placements); > > + GEM_BUG_ON(obj->mm.n_placements > I915_TTM_MAX_PLACEMENTS); > + i915_ttm_placement_from_obj(obj, &requested, busy, &placement); > + > /* > * If this function fails, it will call the destructor, but > * our caller still owns the object. So no freeing in the > @@ -927,8 +932,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, > * until successful initialization. > */ > ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj), size, > - bo_type, &i915_sys_placement, > - page_size >> PAGE_SHIFT, > + bo_type, &placement, page_size >> PAGE_SHIFT, > &ctx, NULL, NULL, i915_ttm_bo_destroy); > if (ret) > return i915_ttm_err_to_gem(ret); > -- > 2.31.1 >
On Fri, Jul 16, 2021 at 8:18 AM Matthew Auld <matthew.william.auld@gmail.com> wrote: > > On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand <jason@jlekstrand.net> wrote: > > > > __i915_gem_ttm_object_init() was ignoring the placement requests coming > > from the client and always placing all BOs in SMEM upon creation. > > Instead, compute the requested placement set from the object and pass > > that into ttm_bo_init_reserved(). > > AFAIK this is intentional, where we use SYS as a safe placeholder > specifically for object creation, since that avoids allocating any > actual pages. Later at get_pages() time we do the real allocation, > where we need to consider the placements. > > If we set the real placements here, the ttm_bo_validate() call in > init_reserved() might allocate the backing pages here for the > lmem-only case, which is not what we want in i915. I'm happy to drop this patch. It doesn't actually fix anything AFAIK. It makes the behavior more what I expected but my expectations are not to be trusted here. The other TTM patch does fix a real bug AFAIK. --Jason > > > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > Cc: Matthew Auld <matthew.auld@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > index 6589411396d3f..d30f274c329c7 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > @@ -898,6 +898,8 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, > > { > > static struct lock_class_key lock_class; > > struct drm_i915_private *i915 = mem->i915; > > + struct ttm_place requested, busy[I915_TTM_MAX_PLACEMENTS]; > > + struct ttm_placement placement; > > struct ttm_operation_ctx ctx = { > > .interruptible = true, > > .no_wait_gpu = false, > > @@ -919,6 +921,9 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, > > /* Forcing the page size is kernel internal only */ > > GEM_BUG_ON(page_size && obj->mm.n_placements); > > > > + GEM_BUG_ON(obj->mm.n_placements > I915_TTM_MAX_PLACEMENTS); > > + i915_ttm_placement_from_obj(obj, &requested, busy, &placement); > > + > > /* > > * If this function fails, it will call the destructor, but > > * our caller still owns the object. So no freeing in the > > @@ -927,8 +932,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, > > * until successful initialization. > > */ > > ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj), size, > > - bo_type, &i915_sys_placement, > > - page_size >> PAGE_SHIFT, > > + bo_type, &placement, page_size >> PAGE_SHIFT, > > &ctx, NULL, NULL, i915_ttm_bo_destroy); > > if (ret) > > return i915_ttm_err_to_gem(ret); > > -- > > 2.31.1 > >
Hi, Jason, On 7/16/21 12:38 AM, Jason Ekstrand wrote: > __i915_gem_ttm_object_init() was ignoring the placement requests coming > from the client and always placing all BOs in SMEM upon creation. > Instead, compute the requested placement set from the object and pass > that into ttm_bo_init_reserved(). This is done on purpose. When objects are initially created in SMEM, they are created in "Limbo", meaning they have no pages and costly allocation and clearing is deferred to first get_pages(). So we shouldn't be doing this. /Thomas
On 8/4/21 8:49 AM, Thomas Hellström wrote: > Hi, Jason, > > On 7/16/21 12:38 AM, Jason Ekstrand wrote: >> __i915_gem_ttm_object_init() was ignoring the placement requests coming >> from the client and always placing all BOs in SMEM upon creation. >> Instead, compute the requested placement set from the object and pass >> that into ttm_bo_init_reserved(). > > This is done on purpose. When objects are initially created in SMEM, > they are created in "Limbo", meaning they have no pages and costly > allocation and clearing is deferred to first get_pages(). > > So we shouldn't be doing this. Ah, I see Matthew already responded on this. Sorry for the noise. /Thomas
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 6589411396d3f..d30f274c329c7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -898,6 +898,8 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, { static struct lock_class_key lock_class; struct drm_i915_private *i915 = mem->i915; + struct ttm_place requested, busy[I915_TTM_MAX_PLACEMENTS]; + struct ttm_placement placement; struct ttm_operation_ctx ctx = { .interruptible = true, .no_wait_gpu = false, @@ -919,6 +921,9 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, /* Forcing the page size is kernel internal only */ GEM_BUG_ON(page_size && obj->mm.n_placements); + GEM_BUG_ON(obj->mm.n_placements > I915_TTM_MAX_PLACEMENTS); + i915_ttm_placement_from_obj(obj, &requested, busy, &placement); + /* * If this function fails, it will call the destructor, but * our caller still owns the object. So no freeing in the @@ -927,8 +932,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, * until successful initialization. */ ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj), size, - bo_type, &i915_sys_placement, - page_size >> PAGE_SHIFT, + bo_type, &placement, page_size >> PAGE_SHIFT, &ctx, NULL, NULL, i915_ttm_bo_destroy); if (ret) return i915_ttm_err_to_gem(ret);
__i915_gem_ttm_object_init() was ignoring the placement requests coming from the client and always placing all BOs in SMEM upon creation. Instead, compute the requested placement set from the object and pass that into ttm_bo_init_reserved(). Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> Cc: Matthew Auld <matthew.auld@intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)