[Intel-gfx] [PATCH 4/7] drm/i915/gem/ttm: Place new BOs in the requested region

[Jason Ekstrand]

On Fri, Jul 16, 2021 at 8:18 AM Matthew Auld
<matthew.william.auld at gmail.com> wrote:
>
> On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand <jason at 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 at jlekstrand.net>
> > Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> > Cc: Matthew Auld <matthew.auld at intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst at 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
> >