[Intel-gfx] [PATCH v2 03/16] drm/i915: Remove pages_mutex and intel_gtt->vma_ops.set/clear_pages members, v2.

[Matthew Auld]

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