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> <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?