[Intel-gfx] [PATCH] drm/i915: Convert WARNs during userptr revoke to SIGBUS

[Tvrtko Ursulin]

On 12/10/15 11:10, Chris Wilson wrote:
> On Mon, Oct 12, 2015 at 10:31:35AM +0100, Chris Wilson wrote:
>> We basically need to clone the obj, move the pages and vma over and so
>> as the vma die the pages are unreferenced. All new use will be forced to
>> call gup and be fine.
>>
>> Ok, that smells ok, I'll see how doable that is.
>
> Hmm. If we take the vma-centric driver as granted (i.e. using the vma as the
> token when pinning, the vma holds fences etc), the tricky part if that we
> don't hold a reference from the pinned vma to the object. pin_display to
> the rescue!
>
> Initial sketch:
>
> static struct drm_i915_gem_object *steal_pages(struct drm_i915_gem_object *obj)
> {
>          struct drm_i915_gem_object *clone;
>          struct i915_vma *vma;
>          int i;
>
>          BUG_ON(obj->stolen);
>
>          clone = i915_gem_object_alloc(obj->base.dev);
>          if (clone == NULL)
>                  return clone;
>
>          drm_gem_private_object_init(obj->base.dev,
>                                      &clone->base,
>                                      obj->base.size);
>          i915_gem_object_init(clone, obj->ops);
>
>          list_splice_init(&obj->vma_list, &clone->vma_list);
>          list_for_each_entry(vma, &clone->vma_list, obj_link)
>                  vma->obj = clone;
>
>          if (obj->pin_display) {
>                  clone->pin_display = obj->pin_display;
>                  while (obj->pin_display--) {
>                          drm_gem_object_reference(&clone->base);
>                          drm_gem_object_unreference(&obj->base);
>                  }
>          }
>
>          clone->bind_count = obj->bind_count;
>          obj->bind_count = 0;
>          /* vma_ht / vma_hashed */
>
>          for (i = 0; i < I915_NUM_RINGS; i++) {
>                  if (obj->last_read[i].request == NULL)
>                          continue;
>
>                  clone->last_read[i].request = obj->last_read[i].request;
>                  list_replace_init(&obj->last_read[i].link,
>                                    &clone->last_read[i].link);
>                  clone->flags |= 1 << (i + I915_BO_ACTIVE_SHIFT);
>          }
>          if (obj->last_write.request) {
>                  clone->last_write.request = obj->last_write.request;
>                  list_replace_init(&obj->last_write.link,
>                                    &clone->last_write.link);
>          }
>
>          clone->dirty = obj->dirty;
>          obj->dirty = false;
>
>          clone->tiling_mode = obj->tiling_mode;
>          clone->stride = obj->stride;
>
>          clone->pin_display = obj->pin_display;
>          obj->pin_display = 0;
>
>          clone->madv = I915_MADV_DONTNEED;
>          clone->pages = obj->pages;
>          clone->pages_pin_count = obj->pages_pin_count;
>          clone->get_page = obj->get_page;
>          clone->vmapping = obj->vmapping;
>          obj->pages = NULL;
>          obj->pages_pin_count = 0;
>          obj->vmapping = NULL;
>
>          clone->bit_17 = obj->bit_17;
>          obj->bit_17 = NULL;
>
>          i915_gem_release_mmap(obj);
>
>          if (I915_BO_IS_ACTIVE(clone))
>                  clone->active_reference = true;
>          else
>                  drm_gem_object_unreference(&clone->base);
>
>          return clone;
> }
>
> It does have the presumption that we have either an active reference or a
> pinned reference.

I get the general idea but the base is too far from the current code to 
properly evaluate. But in principle it sounds promising.

Regards,

Tvrtko