[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: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