On Mon, 29 Nov 2021 at 13:57, Maarten Lankhorst
<maarten.lankhorst@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> ---
> 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);
> }
