[PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Jason Ekstrand]

On Fri, Jul 16, 2021 at 8:54 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:
> >
> > Whenever we had a user object (n_placements > 0), we were ignoring
> > obj->mm.region and always putting obj->placements[0] as the requested
> > region.  For LMEM+SMEM objects, this was causing them to get shoved into
> > LMEM on every i915_ttm_get_pages() even when SMEM was requested by, say,
> > i915_gem_object_migrate().
>
> i915_ttm_migrate calls i915_ttm_place_from_region() directly with the
> requested region, so there shouldn't be an issue with migration right?
> Do you have some more details?

With i915_ttm_migrate directly, no.  But, in the last patch in the
series, we're trying to migrate LMEM+SMEM buffers into SMEM on
attach() and pin it there.  This blows up in a very unexpected (IMO)
way.  The flow goes something like this:

 - Client attempts a dma-buf import from another device
 - In attach() we call i915_gem_object_migrate() which calls
i915_ttm_migrate() which migrates as requested.
 - Once the migration is complete, we call i915_gem_object_pin_pages()
which calls i915_ttm_get_pages() which depends on
i915_ttm_placement_from_obj() and so migrates it right back to LMEM.

Maybe the problem here is actually that our TTM code isn't respecting
obj->mm.pages_pin_count?

In case you can't tell, I really have no clue what I'm doing here.
I'm really stumbling around in the dark finding things that make my
bug go away.  I'm happy for the feedback.

--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 | 3 +--
> >  1 file changed, 1 insertion(+), 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 d30f274c329c7..5985e994d56cf 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > @@ -150,8 +150,7 @@ i915_ttm_placement_from_obj(const struct drm_i915_gem_object *obj,
> >         unsigned int i;
> >
> >         placement->num_placement = 1;
> > -       i915_ttm_place_from_region(num_allowed ? obj->mm.placements[0] :
> > -                                  obj->mm.region, requested, flags);
> > +       i915_ttm_place_from_region(obj->mm.region, requested, flags);
> >
> >         /* Cache this on object? */
> >         placement->num_busy_placement = num_allowed;
> > --
> > 2.31.1
> >