Re: [PATCH] drm/i915: Release ctx->syncobj on final put, not on ctx close

[Jason Ekstrand <jason@jlekstrand.net>]

[-- Attachment #1: Type: text/plain, Size: 2341 bytes --]

On August 6, 2021 15:18:59 Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> gem context refcounting is another exercise in least locking design it
> seems, where most things get destroyed upon context closure (which can
> race with anything really). Only the actual memory allocation and the
> locks survive while holding a reference.
>
> This tripped up Jason when reimplementing the single timeline feature
> in
>
> commit 00dae4d3d35d4f526929633b76e00b0ab4d3970d
> Author: Jason Ekstrand <jason@jlekstrand.net>
> Date:   Thu Jul 8 10:48:12 2021 -0500
>
>    drm/i915: Implement SINGLE_TIMELINE with a syncobj (v4)
>
> We could fix the bug by holding ctx->mutex, but it's cleaner to just

What bug is this fixing, exactly?

--Jason

>
> make the context object actually invariant over its _entire_ lifetime.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Fixes: 00dae4d3d35d ("drm/i915: Implement SINGLE_TIMELINE with a syncobj (v4)")
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: "Thomas Hellström" <thomas.hellstrom@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 754b9b8d4981..93ba0197d70a 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -940,6 +940,9 @@ void i915_gem_context_release(struct kref *ref)
>  trace_i915_context_free(ctx);
>  GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
>
> + if (ctx->syncobj)
> + drm_syncobj_put(ctx->syncobj);
> +
>  mutex_destroy(&ctx->engines_mutex);
>  mutex_destroy(&ctx->lut_mutex);
>
> @@ -1159,9 +1162,6 @@ static void context_close(struct i915_gem_context *ctx)
>  if (vm)
>  i915_vm_close(vm);
>
> - if (ctx->syncobj)
> - drm_syncobj_put(ctx->syncobj);
> -
>  ctx->file_priv = ERR_PTR(-EBADF);
>
>  /*
> --
> 2.32.0


[-- Attachment #2: Type: text/html, Size: 4658 bytes --]