[Intel-gfx] [PATCH 04/26] drm/i915: Add an implementation for i915_gem_ww_ctx locking, v2.

[Chris Wilson]

Quoting Thomas Hellström (Intel) (2020-06-24 08:49:21)
> Hi, Chris,
> 
> On 6/24/20 9:43 AM, Chris Wilson wrote:
> > Quoting Thomas Hellström (Intel) (2020-06-24 08:10:43)
> >> Hi, Maarten,
> >>
> >>
> >> On 6/23/20 4:28 PM, Maarten Lankhorst wrote:
> >>> i915_gem_ww_ctx is used to lock all gem bo's for pinning and memory
> >>> eviction. We don't use it yet, but lets start adding the definition
> >>> first.
> >>>
> >>> To use it, we have to pass a non-NULL ww to gem_object_lock, and don't
> >>> unlock directly. It is done in i915_gem_ww_ctx_fini.
> >>>
> >>> Changes since v1:
> >>> - Change ww_ctx and obj order in locking functions (Jonas Lahtinen)
> >>>
> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/display/intel_display.c  |  4 +-
> >>>    .../gpu/drm/i915/gem/i915_gem_client_blt.c    |  2 +-
> >>>    drivers/gpu/drm/i915/gem/i915_gem_context.c   |  2 +-
> >>>    drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  4 +-
> >>>    drivers/gpu/drm/i915/gem/i915_gem_domain.c    | 10 ++--
> >>>    .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |  4 +-
> >>>    drivers/gpu/drm/i915/gem/i915_gem_object.c    |  2 +-
> >>>    drivers/gpu/drm/i915/gem/i915_gem_object.h    | 38 +++++++++++---
> >>>    .../gpu/drm/i915/gem/i915_gem_object_types.h  |  9 ++++
> >>>    drivers/gpu/drm/i915/gem/i915_gem_pm.c        |  2 +-
> >>>    drivers/gpu/drm/i915/gem/i915_gem_tiling.c    |  2 +-
> >>>    .../gpu/drm/i915/gem/selftests/huge_pages.c   |  2 +-
> >>>    .../i915/gem/selftests/i915_gem_client_blt.c  |  2 +-
> >>>    .../i915/gem/selftests/i915_gem_coherency.c   | 10 ++--
> >>>    .../drm/i915/gem/selftests/i915_gem_context.c |  4 +-
> >>>    .../drm/i915/gem/selftests/i915_gem_mman.c    |  4 +-
> >>>    .../drm/i915/gem/selftests/i915_gem_phys.c    |  2 +-
> >>>    .../gpu/drm/i915/gt/selftest_workarounds.c    |  2 +-
> >>>    drivers/gpu/drm/i915/gvt/cmd_parser.c         |  2 +-
> >>>    drivers/gpu/drm/i915/i915_gem.c               | 52 +++++++++++++++++--
> >>>    drivers/gpu/drm/i915/i915_gem.h               | 11 ++++
> >>>    drivers/gpu/drm/i915/selftests/i915_gem.c     | 41 +++++++++++++++
> >>>    drivers/gpu/drm/i915/selftests/i915_vma.c     |  2 +-
> >>>    .../drm/i915/selftests/intel_memory_region.c  |  2 +-
> >>>    24 files changed, 173 insertions(+), 42 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> >>> index b1f82a11aef2..3740c0080e38 100644
> >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> >>> @@ -122,6 +122,15 @@ struct drm_i915_gem_object {
> >>>         */
> >>>        struct list_head lut_list;
> >>>    
> >>> +     /**
> >>> +      * @obj_link: Link into @i915_gem_ww_ctx.obj_list
> >>> +      *
> >>> +      * When we lock this object through i915_gem_object_lock() with a
> >>> +      * context, we add it to the list to ensure we can unlock everything
> >>> +      * when i915_gem_ww_ctx_backoff() or i915_gem_ww_ctx_fini() are called.
> >>> +      */
> >>> +     struct list_head obj_link;
> >>> +
> >> Since we don't refcount objects on the list, (and we shouldn't need to),
> >> perhaps a debug warning if during object destruction, this isn't an
> >> empty list head?
> >>
> >> Other than that, this patch looks good to me.
> > Aside it from being in the wrong layer, as was also mentioned several
> > months ago.
> > -Chris
> 
> Could you send a pointer, or perhaps elaborate a bit?

We have been trying to extricate the GEM uAPI layer and objects from the
memory management of the backing store with a view to bypassing the
implicit rules imposed by GEM, and to remove the layering violation of
the HW layer calling back into the upper API layer.

For the moment the distinction is "should this be obj or obj->mm". Some
might have been arguing for obj->mm to become its backing store object,
[and had hoped ttm would have been usable for managing it] for we will
have i915_vma that do not [need to] refer to GEM objects. Aside from
first class user i915_vma, we also need to allocate PD as some sort of
highly restricted object (and often tiny, so suballocations).

And everything needs a severe diet.
-Chris