On 06-12-2021 18:10, Matthew Auld wrote:
> 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?
Correct! I missed it. Will fix it up when committing, or if a new version is needed.
