On Fri, 16 Jul 2021 at 18:39, Jason Ekstrand <jason@xxxxxxxxxxxxxx> wrote: > > On Fri, Jul 16, 2021 at 11:00 AM Matthew Auld > <matthew.william.auld@xxxxxxxxx> wrote: > > > > On Fri, 16 Jul 2021 at 16:52, Matthew Auld > > <matthew.william.auld@xxxxxxxxx> wrote: > > > > > > On Fri, 16 Jul 2021 at 15:10, Jason Ekstrand <jason@xxxxxxxxxxxxxx> wrote: > > > > > > > > On Fri, Jul 16, 2021 at 8:54 AM Matthew Auld > > > > <matthew.william.auld@xxxxxxxxx> wrote: > > > > > > > > > > On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand <jason@xxxxxxxxxxxxxx> 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. > > > > > > The mm.pages must be NULL here, otherwise it would just increment the > > > pages_pin_count? > > Given that the test is using the ____four_underscores version, it > doesn't have that check. However, this executes after we've done the > dma-buf import which pinned pages. So we should definitely have > pages. We shouldn't call ____four_underscores() if we might already have pages though. Under non-TTM that would leak the pages, and in TTM we might hit the WARN_ON(mm->pages) in __i915_ttm_get_pages(), if for example nothing was moved. I take it we can't just call pin_pages()? Four scary underscores usually means "don't call this in normal code". > > > > > > > > > Maybe the problem here is actually that our TTM code isn't respecting > > > > obj->mm.pages_pin_count? > > > > > > I think if the resource is moved, we always nuke the mm.pages after > > > being notified of the move. Also TTM is also not allowed to move > > > pinned buffers. > > > > > > I guess if we are evicted/swapped, so assuming we are not holding the > > > object lock, and it's not pinned, the future call to get_pages() will > > > see mm.pages = NULL, even though the ttm_resource is still there, and > > > because we prioritise the placements[0], instead of mm.region we end > > > up moving it for no good reason. But in your case you are holding the > > > lock, or it's pinned? Also is this just with the selftest, or > > > something real? > > > > Or at least in the selftest I see ____i915_gem_object_get_pages() > > which doesn't even consider the mm.pages AFAIK. > > The bogus migration is happening as part of the > __i915_gem_object_get_pages() (2 __underscores) call in > i915_gem_dmabuf_attach (see last patch). That code is attempting to > migrate the BO to SMEM and then pin it there using the obvious calls > to do so. However, in the pin_pages call, it gets implicitly migrated > back to LMEM thanks to i915_ttm_get_pages(). Why is _get_pages() > migrating things at all? Not sure yet, but __two_underscores() checks if i915_gem_object_has_pages() before actually calling into i915_ttm_get_pages(), so the mm.pages would have to be NULL here for some reason, so best guess is something to do with move_notify(). > > --Jason > > > > > > > > > > > > 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@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 | 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 > > > > > >
