[Intel-gfx] [PATCH 1/3] drm/i915: Only update the current userptr worker

[Tvrtko Ursulin]

On 07/01/2015 12:09 PM, Chris Wilson wrote:
> On Wed, Jul 01, 2015 at 11:58:46AM +0100, Tvrtko Ursulin wrote:
>> On 07/01/2015 10:59 AM, Chris Wilson wrote:
>>> On Wed, Jul 01, 2015 at 10:48:59AM +0100, Tvrtko Ursulin wrote:
>>>> Previously the canceled worker would allow another worker to be
>>>> created in case it failed (obj->userptr.work != &work->work; ret =
>>>> 0;) and now it still does since obj->userptr.work remains at NULL
>>> >from cancellation.
>>>>
>>>> Both seem wrong, am I missing the change?
>>>
>>> No, the obj->userptr.work must remain NULL until a new get_pages()
>>> because we don't actually know if this worker's gup was before or after
>>> the cancellation  - mmap_sem vs struct_mutex ordering.
>>
>> No one is not wrong, or no I was not missing the change?
>
> The only change is that we don't change the value of userptr.work if it
> is set to something else. The only time it should be different was if it
> had been cancelled and so NULL. The patch just makes it so that a coding
> error is less damaging - and I think easier to read because of that.
>
>> I am thinking more and more that we should just mark it canceled
>> forever and not allow get_pages to succeed ever since.
>
> Yes, I toyed with that yesterday in response to you being able to alias
> a GTT mmap address with the userptr after munmap(userptr.ptr). The
> problem is that cancel_userptr() is caller for any change in the CPU
> PTE's, including mprotect() or cow after forking. Both of those are
> valid situations where we want to keep the userptr around, but with a
> new gup.

Why do we want that? I would be surprised if someone is using it like 
that. How would it be defined on the GEM handle level even?

Regards,

Tvrtko