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

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-25 16:59:26)
> 
> 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 at 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?

vm->bind_alloc_flags it is.
-Chris