All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 0/6] drm/i915: Remove short term pins from execbuf by requiring lock to unbind.
@ 2022-01-14 13:23 ` Maarten Lankhorst
  0 siblings, 0 replies; 33+ messages in thread
From: Maarten Lankhorst @ 2022-01-14 13:23 UTC (permalink / raw)
  To: intel-gfx; +Cc: dri-devel

Previously, short term pinning in execbuf was required because i915_vma was
effectively independent from objects, and has its own refcount, locking,
lifetime rules and pinning.

This series removes the separate locking, by requiring vma->obj->resv to be
held when pinning and unbinding. This will also be required for VM_BIND work.
Some patches have already been merged, but this contains the mremainder of
the conversion.

With pinning required for pinning and unbinding, the lock is enough to prevent
unbinding when trying to pin with the lock held, for example in execbuf.

This makes binding/unbinding similar to ttm_bo_validate()'s use, which just
cares that an object is in a certain place, without pinning it in place.

Having the VMA part of gem bo removes a lot of the vma refcounting, and makes
i915_vma more a part of the bo, instead of its own floating object that just
happens to be part of a bo. This is also required to make it more compatible
with TTM, and migration in general.

For future work, it makes things a lot simpler and clear. We want to end up
with i915_vma just being a specific mapping of the BO, just like is the
case in other drivers. i915_vma->active removal is the next step there,
and makes it when object is destroyed, the bindings are destroyed (after idle),
instead of object being destroyed when bindings are idle. 

Changes since last version:
- Fix list corruption in i915_gem_evict_vm()
- Small fixes and assorted canges.

Maarten Lankhorst (6):
  drm/i915: Call i915_gem_evict_vm in vm_fault_gtt to prevent new ENOSPC
    errors, v2.
  drm/i915: Add locking to i915_gem_evict_vm(), v2.
  drm/i915: Add object locking to i915_gem_evict_for_node and
    i915_gem_evict_something, v2.
  drm/i915: Add i915_vma_unbind_unlocked, and take obj lock for
    i915_vma_unbind, v2.
  drm/i915: Remove support for unlocked i915_vma unbind
  drm/i915: Remove short-term pins from execbuf, v6.

 drivers/gpu/drm/i915/display/intel_fb_pin.c   |   2 +-
 .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 220 +++++++++---------
 drivers/gpu/drm/i915/gem/i915_gem_mman.c      |  18 +-
 .../gpu/drm/i915/gem/selftests/huge_pages.c   |   2 +-
 .../i915/gem/selftests/i915_gem_client_blt.c  |   2 +-
 .../drm/i915/gem/selftests/i915_gem_mman.c    |   6 +
 drivers/gpu/drm/i915/gt/intel_ggtt.c          |  47 +++-
 drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c  |   1 -
 drivers/gpu/drm/i915/gt/selftest_hangcheck.c  |   2 +-
 drivers/gpu/drm/i915/gvt/aperture_gm.c        |   2 +-
 drivers/gpu/drm/i915/i915_gem.c               |   2 +
 drivers/gpu/drm/i915/i915_gem_evict.c         |  97 +++++++-
 drivers/gpu/drm/i915/i915_gem_evict.h         |   6 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c           |   8 +-
 drivers/gpu/drm/i915/i915_gem_gtt.h           |   4 +
 drivers/gpu/drm/i915/i915_vgpu.c              |   2 +-
 drivers/gpu/drm/i915/i915_vma.c               | 122 +++++-----
 drivers/gpu/drm/i915/i915_vma.h               |   1 +
 .../gpu/drm/i915/selftests/i915_gem_evict.c   |  26 ++-
 drivers/gpu/drm/i915/selftests/i915_gem_gtt.c |  28 +--
 drivers/gpu/drm/i915/selftests/i915_vma.c     |   8 +-
 21 files changed, 382 insertions(+), 224 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 33+ messages in thread
* [PATCH v5 0/6] drm/i915: Remove short term pins from execbuf by requiring lock to unbind.
@ 2022-01-13 11:44 Maarten Lankhorst
  2022-01-13 12:53 ` [Intel-gfx] ✗ Fi.CI.DOCS: warning for " Patchwork
  0 siblings, 1 reply; 33+ messages in thread
From: Maarten Lankhorst @ 2022-01-13 11:44 UTC (permalink / raw)
  To: intel-gfx; +Cc: dri-devel

Previously, short term pinning in execbuf was required because i915_vma was
effectively independent from objects, and has its own refcount, locking,
lifetime rules and pinning.

This series removes the separate locking, by requiring vma->obj->resv to be
held when pinning and unbinding. This will also be required for VM_BIND work.
Some patches have already been merged, but this contains the mremainder of
the conversion.

With pinning required for pinning and unbinding, the lock is enough to prevent
unbinding when trying to pin with the lock held, for example in execbuf.

This makes binding/unbinding similar to ttm_bo_validate()'s use, which just
cares that an object is in a certain place, without pinning it in place.

Having the VMA part of gem bo removes a lot of the vma refcounting, and makes
i915_vma more a part of the bo, instead of its own floating object that just
happens to be part of a bo. This is also required to make it more compatible
with TTM, and migration in general.

For future work, it makes things a lot simpler and clear. We want to end up
with i915_vma just being a specific mapping of the BO, just like is the
case in other drivers. i915_vma->active removal is the next step there,
and makes it when object is destroyed, the bindings are destroyed (after idle),
instead of object being destroyed when bindings are idle. 

Changes since previous version:
* As a special case, we allow unbinding pages when the object is dead.
  This will allow us to free objects without complications, which is
  important because many IGT tests free and then immediately put a new
  object in the same address. This is a shortcoming of softpin, as
  there is no way to tell when it's safe to re-use the address.

  As a potential performance optimization, i915 should use the random
  allocator by default, instead of the high to low allocator,
  which causes the unintentional address re-use.

Maarten Lankhorst (6):
  drm/i915: Call i915_gem_evict_vm in vm_fault_gtt to prevent new ENOSPC
    errors, v2.
  drm/i915: Add locking to i915_gem_evict_vm()
  drm/i915: Add object locking to i915_gem_evict_for_node and
    i915_gem_evict_something, v2.
  drm/i915: Add i915_vma_unbind_unlocked, and take obj lock for
    i915_vma_unbind, v2.
  drm/i915: Remove support for unlocked i915_vma unbind
  drm/i915: Remove short-term pins from execbuf, v6.

 drivers/gpu/drm/i915/display/intel_fb_pin.c   |   2 +-
 .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 220 +++++++++---------
 drivers/gpu/drm/i915/gem/i915_gem_mman.c      |  18 +-
 .../gpu/drm/i915/gem/selftests/huge_pages.c   |   2 +-
 .../i915/gem/selftests/i915_gem_client_blt.c  |   2 +-
 .../drm/i915/gem/selftests/i915_gem_mman.c    |   6 +
 drivers/gpu/drm/i915/gt/intel_ggtt.c          |  47 +++-
 drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c  |   1 -
 drivers/gpu/drm/i915/gt/selftest_hangcheck.c  |   2 +-
 drivers/gpu/drm/i915/gvt/aperture_gm.c        |   2 +-
 drivers/gpu/drm/i915/i915_gem.c               |   2 +
 drivers/gpu/drm/i915/i915_gem_evict.c         | 107 ++++++++-
 drivers/gpu/drm/i915/i915_gem_evict.h         |   6 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c           |   8 +-
 drivers/gpu/drm/i915/i915_gem_gtt.h           |   4 +
 drivers/gpu/drm/i915/i915_vgpu.c              |   2 +-
 drivers/gpu/drm/i915/i915_vma.c               | 122 +++++-----
 drivers/gpu/drm/i915/i915_vma.h               |   1 +
 .../gpu/drm/i915/selftests/i915_gem_evict.c   |  27 ++-
 drivers/gpu/drm/i915/selftests/i915_gem_gtt.c |  28 +--
 drivers/gpu/drm/i915/selftests/i915_vma.c     |   8 +-
 21 files changed, 393 insertions(+), 224 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 33+ messages in thread
* [PATCH v3 00/17] drm/i915: Remove short term pins from execbuf by requiring lock to unbind.
@ 2021-12-16 14:27 Maarten Lankhorst
  2021-12-17  8:51 ` [Intel-gfx] ✗ Fi.CI.DOCS: warning for " Patchwork
  0 siblings, 1 reply; 33+ messages in thread
From: Maarten Lankhorst @ 2021-12-16 14:27 UTC (permalink / raw)
  To: intel-gfx; +Cc: dri-devel

Previously, short term pinning in execbuf was required because i915_vma was
effectively independent from objects, and has its own refcount, locking,
lifetime rules and pinning.

This series removes the separate locking, by requiring vma->obj->resv to be
held when pinning and unbinding. This will also be required for VM_BIND work.

With pinning required for pinning and unbinding, the lock is enough to prevent
unbinding when trying to pin with the lock held, for example in execbuf.

This makes binding/unbinding similar to ttm_bo_validate()'s use, which just
cares that an object is in a certain place, without pinning it in place.

Having the VMA part of gem bo removes a lot of the vma refcounting, and makes
i915_vma more a part of the bo, instead of its own floating object that just
happens to be part of a bo. This is also required to make it more compatible
with TTM, and migration in general.

For future work, it makes things a lot simpler and clear. We want to end up
with i915_vma just being a specific mapping of the BO, just like is the
case in other drivers. i915_vma->active removal is the next step there,
and makes it when object is destroyed, the bindings are destroyed (after idle),
instead of object being destroyed when bindings are idle. 

---
This is version 3, I changed the ordering of the patches slightly to be more
natural, and divided up some commits in different ways.

In particular, I added the ww ctx to trylock first, then converted
i915_gem_evict_vm(), the shrinker, and the other eviction functions all
in separate commits. Functionally the same, but easier to review and bisect.

In theory, it was already reviewed, but leaving those r-b's out as they're
technically different patches now.

The first patches prepare for vma->obj->resv requirement, by ensuring that we
unbind as needed, and take the ww lock in some places where they're required,
including when freeing the object.

After that we convert the shrinker, eviction functions, and then we can start
requiring the object lock for i915_vma_unbind.

Now that the conversion and requirement is added, we can remove ther special
case of assert_object_held_shared, which was added because we didn't always
require the object lock.

As last step we can remove the short term pins from execbuf. We still pin when
evicting the entire VM, but if it turns out to be an issue, it can be removed,
and replaced with unpinning the entire VM.

Maarten Lankhorst (17):
  drm/i915: Remove unused bits of i915_vma/active api
  drm/i915: Change shrink ordering to use locking around unbinding.
  drm/i915: Remove pages_mutex and intel_gtt->vma_ops.set/clear_pages
    members, v3.
  drm/i915: Take object lock in i915_ggtt_pin if ww is not set
  drm/i915: Force ww lock for i915_gem_object_ggtt_pin_ww, v2.
  drm/i915: Ensure gem_contexts selftests work with unbind changes, v2.
  drm/i915: Ensure i915_vma tests do not get -ENOSPC with the locking
    changes.
  drm/i915: Call i915_gem_evict_vm in vm_fault_gtt to prevent new ENOSPC
    errors
  drm/i915: Trylock the object when shrinking
  drm/i915: Require object lock when freeing pages during destruction
  drm/i915: Add ww ctx to i915_gem_object_trylock
  drm/i915: Add locking to i915_gem_evict_vm()
  drm/i915: Add object locking to i915_gem_evict_for_node and
    i915_gem_evict_something
  drm/i915: Add i915_vma_unbind_unlocked, and take obj lock for
    i915_vma_unbind, v2.
  drm/i915: Remove assert_object_held_shared
  drm/i915: Remove support for unlocked i915_vma unbind
  drm/i915: Remove short-term pins from execbuf, v5.

 drivers/gpu/drm/i915/display/intel_dpt.c      |   2 -
 drivers/gpu/drm/i915/display/intel_fb_pin.c   |   2 +-
 .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 250 ++++----
 drivers/gpu/drm/i915/gem/i915_gem_mman.c      |  18 +-
 drivers/gpu/drm/i915/gem/i915_gem_object.c    |   9 +-
 drivers/gpu/drm/i915/gem/i915_gem_object.h    |  22 +-
 drivers/gpu/drm/i915/gem/i915_gem_pages.c     |  12 +-
 drivers/gpu/drm/i915/gem/i915_gem_shrinker.c  |  44 +-
 drivers/gpu/drm/i915/gem/i915_gem_stolen.c    |   2 +-
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c   |   2 +-
 .../gpu/drm/i915/gem/selftests/huge_pages.c   |   2 +-
 .../i915/gem/selftests/i915_gem_client_blt.c  |   2 +-
 .../drm/i915/gem/selftests/i915_gem_context.c |  59 +-
 .../drm/i915/gem/selftests/i915_gem_mman.c    |   6 +
 drivers/gpu/drm/i915/gt/gen6_ppgtt.c          |  15 -
 drivers/gpu/drm/i915/gt/intel_engine_pm.c     |   2 +-
 drivers/gpu/drm/i915/gt/intel_ggtt.c          | 454 ++------------
 drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c  |   1 -
 drivers/gpu/drm/i915/gt/intel_gtt.c           |  13 -
 drivers/gpu/drm/i915/gt/intel_gtt.h           |   7 -
 drivers/gpu/drm/i915/gt/intel_ppgtt.c         |  12 -
 drivers/gpu/drm/i915/gt/mock_engine.c         |   2 +-
 drivers/gpu/drm/i915/gt/selftest_hangcheck.c  |   2 +-
 drivers/gpu/drm/i915/gt/selftest_migrate.c    |   2 +-
 drivers/gpu/drm/i915/gvt/aperture_gm.c        |   2 +-
 drivers/gpu/drm/i915/i915_active.c            |  28 +-
 drivers/gpu/drm/i915/i915_active.h            |  17 +-
 drivers/gpu/drm/i915/i915_drv.h               |  12 +-
 drivers/gpu/drm/i915/i915_gem.c               |  32 +-
 drivers/gpu/drm/i915/i915_gem_evict.c         |  64 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c           |   8 +-
 drivers/gpu/drm/i915/i915_gem_gtt.h           |   4 +
 drivers/gpu/drm/i915/i915_vgpu.c              |   2 +-
 drivers/gpu/drm/i915/i915_vma.c               | 579 +++++++++++++++---
 drivers/gpu/drm/i915/i915_vma.h               |   6 +-
 drivers/gpu/drm/i915/i915_vma_types.h         |   1 -
 .../gpu/drm/i915/selftests/i915_gem_evict.c   |  27 +-
 drivers/gpu/drm/i915/selftests/i915_gem_gtt.c |  50 +-
 drivers/gpu/drm/i915/selftests/i915_vma.c     |  19 +-
 drivers/gpu/drm/i915/selftests/mock_gtt.c     |   4 -
 40 files changed, 951 insertions(+), 846 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, other threads:[~2022-01-18 14:50 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-14 13:23 [PATCH v6 0/6] drm/i915: Remove short term pins from execbuf by requiring lock to unbind Maarten Lankhorst
2022-01-14 13:23 ` [Intel-gfx] " Maarten Lankhorst
2022-01-14 13:23 ` [PATCH v6 1/6] drm/i915: Call i915_gem_evict_vm in vm_fault_gtt to prevent new ENOSPC errors, v2 Maarten Lankhorst
2022-01-14 13:23   ` [Intel-gfx] " Maarten Lankhorst
2022-01-14 13:23 ` [PATCH v6 2/6] drm/i915: Add locking to i915_gem_evict_vm(), v2 Maarten Lankhorst
2022-01-14 13:23   ` [Intel-gfx] " Maarten Lankhorst
2022-01-14 13:57   ` Thomas Hellström (Intel)
2022-01-17  7:56     ` [PATCH] drm/i915: Add locking to i915_gem_evict_vm(), v3 Maarten Lankhorst
2022-01-17  7:56       ` [Intel-gfx] " Maarten Lankhorst
2022-01-17 14:08       ` Thomas Hellström
2022-01-17 14:08         ` [Intel-gfx] " Thomas Hellström
2022-01-18 14:50         ` Maarten Lankhorst
2022-01-18 14:50           ` [Intel-gfx] " Maarten Lankhorst
2022-01-14 13:23 ` [Intel-gfx] [PATCH v6 3/6] drm/i915: Add object locking to i915_gem_evict_for_node and i915_gem_evict_something, v2 Maarten Lankhorst
2022-01-14 13:23   ` Maarten Lankhorst
2022-01-14 13:23 ` [Intel-gfx] [PATCH v6 4/6] drm/i915: Add i915_vma_unbind_unlocked, and take obj lock for i915_vma_unbind, v2 Maarten Lankhorst
2022-01-14 13:23   ` Maarten Lankhorst
2022-01-14 13:23 ` [PATCH v6 5/6] drm/i915: Remove support for unlocked i915_vma unbind Maarten Lankhorst
2022-01-14 13:23   ` [Intel-gfx] " Maarten Lankhorst
2022-01-14 13:23 ` [PATCH v6 6/6] drm/i915: Remove short-term pins from execbuf, v6 Maarten Lankhorst
2022-01-14 13:23   ` [Intel-gfx] " Maarten Lankhorst
2022-01-14 15:05 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Remove short term pins from execbuf by requiring lock to unbind Patchwork
2022-01-14 15:06 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2022-01-14 15:10 ` [Intel-gfx] ✗ Fi.CI.DOCS: " Patchwork
2022-01-14 15:37 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2022-01-17  8:08 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Remove short term pins from execbuf by requiring lock to unbind. (rev2) Patchwork
2022-01-17  8:09 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2022-01-17  8:13 ` [Intel-gfx] ✗ Fi.CI.DOCS: " Patchwork
2022-01-17  8:43 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2022-01-17 10:43 ` [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915: Remove short term pins from execbuf by requiring lock to unbind Patchwork
2022-01-17 14:03 ` [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915: Remove short term pins from execbuf by requiring lock to unbind. (rev2) Patchwork
  -- strict thread matches above, loose matches on Subject: below --
2022-01-13 11:44 [PATCH v5 0/6] drm/i915: Remove short term pins from execbuf by requiring lock to unbind Maarten Lankhorst
2022-01-13 12:53 ` [Intel-gfx] ✗ Fi.CI.DOCS: warning for " Patchwork
2021-12-16 14:27 [PATCH v3 00/17] " Maarten Lankhorst
2021-12-17  8:51 ` [Intel-gfx] ✗ Fi.CI.DOCS: warning for " Patchwork

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.