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

[Matthew Auld]

On Tue, 7 Dec 2021 at 10:06, Maarten Lankhorst
<maarten.lankhorst at linux.intel.com> wrote:
>
> On 06-12-2021 18:10, Matthew Auld wrote:
> > 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>
> > <snip>
> >
> >> +static int
> >> +__i915_vma_get_pages(struct i915_vma *vma)
> >> +{
> >> +       struct sg_table *pages;
> >> +       int ret;
> >> +
> >> +       /*
> >> +        * The vma->pages are only valid within the lifespan of the borrowed
> >> +        * obj->mm.pages. When the obj->mm.pages sg_table is regenerated, so
> >> +        * must be the vma->pages. A simple rule is that vma->pages must only
> >> +        * be accessed when the obj->mm.pages are pinned.
> >> +        */
> >> +       GEM_BUG_ON(!i915_gem_object_has_pinned_pages(vma->obj));
> >> +
> >> +       switch (vma->ggtt_view.type) {
> >> +       default:
> >> +               GEM_BUG_ON(vma->ggtt_view.type);
> >> +               fallthrough;
> >> +       case I915_GGTT_VIEW_NORMAL:
> >> +               pages = vma->obj->mm.pages;
> >> +               break;
> >> +
> >> +       case I915_GGTT_VIEW_ROTATED:
> >> +               pages =
> >> +                       intel_rotate_pages(&vma->ggtt_view.rotated, vma->obj);
> >> +               break;
> >> +
> >> +       case I915_GGTT_VIEW_REMAPPED:
> >> +               pages =
> >> +                       intel_remap_pages(&vma->ggtt_view.remapped, vma->obj);
> >> +               break;
> >> +
> >> +       case I915_GGTT_VIEW_PARTIAL:
> >> +               pages = intel_partial_pages(&vma->ggtt_view, vma->obj);
> >> +               break;
> >> +       }
> >> +
> >> +       ret = 0;
> >> +       if (IS_ERR(pages)) {
> >> +               ret = PTR_ERR(pages);
> >> +               pages = NULL;
> >> +               drm_err(&vma->vm->i915->drm,
> >> +                       "Failed to get pages for VMA view type %u (%d)!\n",
> >> +                       vma->ggtt_view.type, ret);
> >> +       }
> >> +
> >> +       pages = xchg(&vma->pages, pages);
> >> +
> >> +       /* did we race against a put_pages? */
> >> +       if (pages && pages != vma->obj->mm.pages) {
> >> +               sg_free_table(vma->pages);
> >> +               kfree(vma->pages);
> > So should this one rather be:
> >
> > sg_free_table(pages);
> > kfree(pages);
> >
> > Or am I missing something?
>
> Correct! I missed it. Will fix it up when committing, or if a new version is needed.
>

r-b with that.