Re: [PATCH] drm/i915: Convert WARNs during userptr revoke to SIGBUS

Tvrtko Ursulin
2015-10-12T12:59:39Z

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