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

[Thomas Hellström (Intel)]

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?

/Thomas