On Mon, 29 Nov 2021 at 13:57, Maarten Lankhorst maarten.lankhorst@linux.intel.com wrote:
Big delta, but boils down to moving set_pages to i915_vma.c, and removing the special handling, all callers use the defaults anyway. We only remap in ggtt, so default case will fall through.
Because we still don't require locking in i915_vma_unpin(), handle this by using xchg in get_pages(), as it's locked with obj->mutex, and cmpxchg in unpin, which only fails if we race a against a new pin.
Changes since v1:
- aliasing gtt sets ZERO_SIZE_PTR, not -ENODEV, remove special case from __i915_vma_get_pages(). (Matt)
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/i915/display/intel_dpt.c | 2 - drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 15 - drivers/gpu/drm/i915/gt/intel_ggtt.c | 403 ---------------- 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/i915_vma.c | 444 ++++++++++++++++-- drivers/gpu/drm/i915/i915_vma.h | 3 + drivers/gpu/drm/i915/i915_vma_types.h | 1 - drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 12 +- drivers/gpu/drm/i915/selftests/mock_gtt.c | 4 - 11 files changed, 424 insertions(+), 492 deletions(-)
<snip>
} @@ -854,18 +1233,22 @@ static int vma_get_pages(struct i915_vma *vma) static void __vma_put_pages(struct i915_vma *vma, unsigned int count) { /* We allocate under vma_get_pages, so beware the shrinker */
mutex_lock_nested(&vma->pages_mutex, SINGLE_DEPTH_NESTING);
struct sg_table *pages = READ_ONCE(vma->pages);GEM_BUG_ON(atomic_read(&vma->pages_count) < count);if (atomic_sub_return(count, &vma->pages_count) == 0) {
Does this emit a barrier? Or can the READ_ONCE(vma->pages) be moved past this, and does that matter?
vma->ops->clear_pages(vma);GEM_BUG_ON(vma->pages);
if (pages == cmpxchg(&vma->pages, pages, NULL) &&
try_cmpxchg? Also can pages be NULL here?
As an aside, is it somehow possible to re-order the series or something to avoid introducing the transient lockless trickery here? I know by the end of the series this all gets removed, but still just slightly worried here.
pages != vma->obj->mm.pages) {sg_free_table(pages);kfree(pages);} i915_gem_object_unpin_pages(vma->obj); }
mutex_unlock(&vma->pages_mutex);}