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

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

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