Re: [PATCH 12/21] drm/i915: Mark up address spaces that may need to allocate

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 12/21] drm/i915: Mark up address spaces that may need to allocate
Date: Wed, 25 Sep 2019 16:59:26 +0100	[thread overview]
Message-ID: <cd7a61f5-a18f-63fa-1857-5ebd723f37df@linux.intel.com> (raw)
In-Reply-To: <156939979494.4979.4044550468298995712@skylake-alporthouse-com>

On 25/09/2019 09:23, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-09-23 09:10:26)
>>
>> On 20/09/2019 17:35, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-09-20 17:22:42)
>>>>
>>>> On 02/09/2019 05:02, Chris Wilson wrote:
>>>>> Since we cannot allocate underneath the vm->mutex (it is used in the
>>>>> direct-reclaim paths), we need to shift the allocations off into a
>>>>> mutexless worker with fence recursion prevention. To know when we need
>>>>> this protection, we mark up the address spaces that do allocate before
>>>>> insertion.
>>>>>
>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/i915_gem_gtt.c | 3 +++
>>>>>     drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++
>>>>>     2 files changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>> index 9095f017162e..56d27cf09a3d 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>> @@ -1500,6 +1500,7 @@ static struct i915_ppgtt *gen8_ppgtt_create(struct drm_i915_private *i915)
>>>>>                         goto err_free_pd;
>>>>>         }
>>>>>     
>>>>> +     ppgtt->vm.bind_alloc = I915_VMA_LOCAL_BIND;
>>>>
>>>> So this is re-using I915_VMA_LOCAL_BIND as a trick? Is it clear how that
>>>> works from these call sites? Should it be called bind_alloc*s*?
>>>> bind_allocates? Or be a boolean which is converted to a trick flag in
>>>> i915_vma_bind where a comment can be put explaining the trick?
>>>
>>> Is it a trick? We need to differentiate between requests for LOCAL_BIND,
>>> GLOBAL_BIND, LOCAL_BIND | GLOBAL_BIND, for different types of vm. Then I
>>> have a plan on using the worker for GLOBAL_BIND on bsw/bxt to defer the
>>> stop_machine().
>>
>> What's the connection between "mark up the address spaces that do
>> allocate before insertion" and I915_VMA_LOCAL_BIND?
> 
> Full-ppgtt is only accessible by PIN_USER.
> 
> Aliasing-ppgtt is accessible from global-gtt as PIN_USER. Only if we
> have an aliasing-gtt behind ggtt do we want to allocate for ggtt for
> local binds.
> 
> global-gtt by itself never allocates and is expected to be synchronous.
> However, we do use stop_machine() for bxt/bsw and that unfortunately is
> marked as an allocating mutex so one idea I had for avoiding that
> lockdep splat was to make bxt/bsw PIN_GLOBAL async.

I think we are not understanding each other from the very start.

My point was that "vm.bind_alloc = I915_VMA_LOCAL_BIND", at least my 
understanding, effectively means "use the worker when pinning/binding 
PIN_USER/I915_VMA_LOCAL_BIND". And that is I think non-obvious. Where 
you have in the code:

	if (flags & vma->vm->bind_alloc)

It is a shorter hacky way of saying:

	if (*flags & I915_VMA_LOCAL_BIND) &&
	    vma->vm->bind_allocates)

Or where you have:

	if (work && (bind_flags & ~vma_flags) & vma->vm->bind_alloc) {

This would be:

	if (work &&
	    vma->vm->bind_allocates &&
	    (bind_flags & I915_VMA_LOCAL_BIND) &&
	    !(vma_flags & I915_VMA_LOCAL_BIND)) {

But I think I see now what your code is actually saying, you are having 
vm->bind_alloc mean vm->bind_flags_which_allocate. Did I get your 
thinking right now? If so compromise with renaming to vm->bind_alloc_flags?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx