On Fri, Jul 16, 2021 at 8:18 AM Matthew Auld
<matthew.william.auld@xxxxxxxxx> wrote:
>
> On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand <jason@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > Cc: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> > Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > ---
> > 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
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
