From: Matthew Auld <matthew.william.auld@gmail.com> To: Jason Ekstrand <jason@jlekstrand.net> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>, "Intel Graphics Development" <intel-gfx@lists.freedesktop.org>, "Matthew Auld" <matthew.auld@intel.com>, "ML dri-devel" <dri-devel@lists.freedesktop.org> Subject: Re: [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj Date: Fri, 16 Jul 2021 16:52:27 +0100 [thread overview] Message-ID: <CAM0jSHO4EU_gBXo-56GtDJffezfVHYoUhCeOnb97ZgBj5vyA7Q@mail.gmail.com> (raw) In-Reply-To: <CAOFGe95gEUNsjCh+30AXhrQLz8_OKbHwwxv=_OhaGKQxGpvcew@mail.gmail.com> On Fri, 16 Jul 2021 at 15:10, Jason Ekstrand <jason@jlekstrand.net> wrote: > > On Fri, Jul 16, 2021 at 8:54 AM Matthew Auld > <matthew.william.auld@gmail.com> wrote: > > > > On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand <jason@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. The mm.pages must be NULL here, otherwise it would just increment the pages_pin_count? > > 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? > > 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@jlekstrand.net> > > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > > Cc: Matthew Auld <matthew.auld@intel.com> > > > Cc: Maarten Lankhorst <maarten.lankhorst@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 > > >
WARNING: multiple messages have this Message-ID (diff)
From: Matthew Auld <matthew.william.auld@gmail.com> To: Jason Ekstrand <jason@jlekstrand.net> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>, "Intel Graphics Development" <intel-gfx@lists.freedesktop.org>, "Matthew Auld" <matthew.auld@intel.com>, "ML dri-devel" <dri-devel@lists.freedesktop.org> Subject: Re: [Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj Date: Fri, 16 Jul 2021 16:52:27 +0100 [thread overview] Message-ID: <CAM0jSHO4EU_gBXo-56GtDJffezfVHYoUhCeOnb97ZgBj5vyA7Q@mail.gmail.com> (raw) In-Reply-To: <CAOFGe95gEUNsjCh+30AXhrQLz8_OKbHwwxv=_OhaGKQxGpvcew@mail.gmail.com> On Fri, 16 Jul 2021 at 15:10, Jason Ekstrand <jason@jlekstrand.net> wrote: > > On Fri, Jul 16, 2021 at 8:54 AM Matthew Auld > <matthew.william.auld@gmail.com> wrote: > > > > On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand <jason@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. The mm.pages must be NULL here, otherwise it would just increment the pages_pin_count? > > 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? > > 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@jlekstrand.net> > > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > > Cc: Matthew Auld <matthew.auld@intel.com> > > > Cc: Maarten Lankhorst <maarten.lankhorst@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 > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2021-07-16 15:52 UTC|newest] Thread overview: 72+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-15 22:38 [PATCH 0/7] drm/i915: Migrate memory to SMEM when imported cross-device Jason Ekstrand 2021-07-15 22:38 ` [Intel-gfx] " Jason Ekstrand 2021-07-15 22:38 ` [PATCH 1/7] drm/i915/gem: Check object_can_migrate from object_migrate Jason Ekstrand 2021-07-15 22:38 ` [Intel-gfx] " Jason Ekstrand 2021-07-15 22:38 ` [PATCH 2/7] drm/i915/gem: Refactor placement setup for i915_gem_object_create* Jason Ekstrand 2021-07-15 22:38 ` [Intel-gfx] " Jason Ekstrand 2021-07-16 11:12 ` Matthew Auld 2021-07-16 11:12 ` Matthew Auld 2021-07-16 13:52 ` Jason Ekstrand 2021-07-16 13:52 ` Jason Ekstrand 2021-07-15 22:38 ` [PATCH 3/7] drm/i915/gem: Unify user object creation Jason Ekstrand 2021-07-15 22:38 ` [Intel-gfx] " Jason Ekstrand 2021-07-16 11:20 ` Matthew Auld 2021-07-16 11:20 ` [Intel-gfx] " Matthew Auld 2021-07-16 14:02 ` Jason Ekstrand 2021-07-16 14:02 ` [Intel-gfx] " Jason Ekstrand 2021-07-20 9:34 ` Matthew Auld 2021-07-20 9:34 ` [Intel-gfx] " Matthew Auld 2021-07-20 22:04 ` Jason Ekstrand 2021-07-20 22:04 ` [Intel-gfx] " Jason Ekstrand 2021-07-21 8:24 ` Matthew Auld 2021-07-21 8:24 ` [Intel-gfx] " Matthew Auld 2021-07-21 15:47 ` Jason Ekstrand 2021-07-21 15:47 ` [Intel-gfx] " Jason Ekstrand 2021-07-21 16:29 ` Matthew Auld 2021-07-21 16:29 ` [Intel-gfx] " Matthew Auld 2021-07-15 22:38 ` [PATCH 4/7] drm/i915/gem/ttm: Place new BOs in the requested region Jason Ekstrand 2021-07-15 22:38 ` [Intel-gfx] " Jason Ekstrand 2021-07-16 13:17 ` Matthew Auld 2021-07-16 13:17 ` [Intel-gfx] " Matthew Auld 2021-07-16 13:46 ` Jason Ekstrand 2021-07-16 13:46 ` [Intel-gfx] " Jason Ekstrand 2021-08-04 6:49 ` Thomas Hellström 2021-08-04 6:49 ` [Intel-gfx] " Thomas Hellström 2021-08-04 6:52 ` Thomas Hellström 2021-08-04 6:52 ` [Intel-gfx] " Thomas Hellström 2021-07-15 22:38 ` [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj Jason Ekstrand 2021-07-15 22:38 ` [Intel-gfx] " Jason Ekstrand 2021-07-16 13:54 ` Matthew Auld 2021-07-16 13:54 ` [Intel-gfx] " Matthew Auld 2021-07-16 14:10 ` Jason Ekstrand 2021-07-16 14:10 ` [Intel-gfx] " Jason Ekstrand 2021-07-16 15:52 ` Matthew Auld [this message] 2021-07-16 15:52 ` Matthew Auld 2021-07-16 16:00 ` Matthew Auld 2021-07-16 16:00 ` [Intel-gfx] " Matthew Auld 2021-07-16 17:38 ` Jason Ekstrand 2021-07-16 17:38 ` [Intel-gfx] " Jason Ekstrand 2021-07-16 18:44 ` Matthew Auld 2021-07-16 18:44 ` [Intel-gfx] " Matthew Auld 2021-07-16 19:49 ` Jason Ekstrand 2021-07-16 19:49 ` [Intel-gfx] " Jason Ekstrand 2021-07-19 13:34 ` Matthew Auld 2021-07-19 13:34 ` [Intel-gfx] " Matthew Auld 2021-07-21 20:11 ` Jason Ekstrand 2021-07-21 20:11 ` [Intel-gfx] " Jason Ekstrand 2021-07-21 20:32 ` Daniel Vetter 2021-07-21 20:32 ` Daniel Vetter 2021-07-22 9:49 ` Matthew Auld 2021-07-22 9:49 ` [Intel-gfx] " Matthew Auld 2021-07-22 9:59 ` Matthew Auld 2021-07-22 9:59 ` [Intel-gfx] " Matthew Auld 2021-08-04 8:00 ` Thomas Hellström 2021-08-04 8:00 ` [Intel-gfx] " Thomas Hellström 2021-08-04 14:35 ` Daniel Vetter 2021-08-04 14:35 ` [Intel-gfx] " Daniel Vetter 2021-07-15 22:38 ` [PATCH 6/7] drm/i915/gem: Correct the locking and pin pattern for dma-buf (v6) Jason Ekstrand 2021-07-15 22:38 ` [Intel-gfx] " Jason Ekstrand 2021-07-15 22:39 ` [PATCH 7/7] drm/i915/gem: Migrate to system at dma-buf attach time (v6) Jason Ekstrand 2021-07-15 22:39 ` [Intel-gfx] " Jason Ekstrand 2021-07-16 14:14 [PATCH 0/7] drm/i915: Migrate memory to SMEM when imported cross-device (v7) Jason Ekstrand 2021-07-16 14:14 ` [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj Jason Ekstrand 2021-07-21 20:13 [PATCH 0/7] drm/i915: Migrate memory to SMEM when imported cross-device (v8) Jason Ekstrand 2021-07-21 20:13 ` [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj Jason Ekstrand
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAM0jSHO4EU_gBXo-56GtDJffezfVHYoUhCeOnb97ZgBj5vyA7Q@mail.gmail.com \ --to=matthew.william.auld@gmail.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=jason@jlekstrand.net \ --cc=matthew.auld@intel.com \ --cc=thomas.hellstrom@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.