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

From: "Thomas Hellström (Intel)" <thomas_os@shipmail.org>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 04/26] drm/i915: Add an implementation for i915_gem_ww_ctx locking, v2.
Date: Wed, 24 Jun 2020 09:10:43 +0200	[thread overview]
Message-ID: <a6c4e18e-7ba3-8ce7-caaf-78b3946441e6@shipmail.org> (raw)
In-Reply-To: <20200623142843.423594-4-maarten.lankhorst@linux.intel.com>

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@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.

Reviewed-by: Thomas Hellström <thomas.hellstrom@intel.com>

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