Re: [PATCH 4/7] drm/i915/gem/ttm: Place new BOs in the requested region
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
Re: [PATCH 4/7] drm/i915/gem/ttm: Place new BOs in the requested region
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
Re: [PATCH 4/7] drm/i915/gem/ttm: Place new BOs in the requested region
On Fri, Jul 16, 2021 at 8:18 AM Matthew Auld
wrote:
>
> On Thu, 15 Jul 2021 at 23:39, 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().
>
> 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
> > Cc: Thomas Hellström
> > Cc: Matthew Auld
> > Cc: Maarten Lankhorst
> > ---
> > 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, , busy, );
> > +
> > /*
> > * 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(>bdev, i915_gem_to_ttm(obj), size,
> > - bo_type, _sys_placement,
> > - page_size >> PAGE_SHIFT,
> > + bo_type, , page_size >>
> > PAGE_SHIFT,
> >, NULL, NULL, i915_ttm_bo_destroy);
> > if (ret)
> > return i915_ttm_err_to_gem(ret);
> > --
> > 2.31.1
> >
Re: [PATCH 4/7] drm/i915/gem/ttm: Place new BOs in the requested region
On Thu, 15 Jul 2021 at 23:39, 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().
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
> Cc: Thomas Hellström
> Cc: Matthew Auld
> Cc: Maarten Lankhorst
> ---
> 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, , busy, );
> +
> /*
> * 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(>bdev, i915_gem_to_ttm(obj), size,
> - bo_type, _sys_placement,
> - page_size >> PAGE_SHIFT,
> + bo_type, , page_size >>
> PAGE_SHIFT,
>, NULL, NULL, i915_ttm_bo_destroy);
> if (ret)
> return i915_ttm_err_to_gem(ret);
> --
> 2.31.1
>
