Re: [PATCH 01/12] drm/i915: Remove kref from i915_sw_fence

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> My original intention was for i915_sw_fence to be the base class and
> provide the reference count for the container. This was from starting
> with a design to handle async_work. In practice, for i915 we embed
> fences into structs which have their own independent reference counting,
> making the i915_sw_fence.kref duplicitous. If we remove the kref, we
> remove the i915_sw_fence's ability to free itself and its independence,
> it can only exist within a container and must be supplied with a
> callback to handle its release.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_sw_fence.c | 55 ++++++++----------------------------
>  drivers/gpu/drm/i915/i915_sw_fence.h |  1 -
>  2 files changed, 11 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
> index a277f8eb7beb..a0a690d6627e 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> @@ -120,34 +120,6 @@ void i915_sw_fence_fini(struct i915_sw_fence *fence)
>  }
>  #endif
>  
> -static void i915_sw_fence_release(struct kref *kref)
> -{
> -	struct i915_sw_fence *fence = container_of(kref, typeof(*fence), kref);
> -
> -	WARN_ON(atomic_read(&fence->pending) > 0);
> -	debug_fence_destroy(fence);
> -
> -	if (fence->flags & I915_SW_FENCE_MASK) {
> -		__i915_sw_fence_notify(fence, FENCE_FREE);

Is the current design so that the callback equals embeddding?

-Mika

> -	} else {
> -		i915_sw_fence_fini(fence);
> -		kfree(fence);
> -	}
> -}
> -
> -static void i915_sw_fence_put(struct i915_sw_fence *fence)
> -{
> -	debug_fence_assert(fence);
> -	kref_put(&fence->kref, i915_sw_fence_release);
> -}
> -
> -static struct i915_sw_fence *i915_sw_fence_get(struct i915_sw_fence *fence)
> -{
> -	debug_fence_assert(fence);
> -	kref_get(&fence->kref);
> -	return fence;
> -}
> -
>  static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence,
>  					struct list_head *continuation)
>  {
> @@ -202,13 +174,15 @@ static void __i915_sw_fence_complete(struct i915_sw_fence *fence,
>  
>  	debug_fence_set_state(fence, DEBUG_FENCE_IDLE, DEBUG_FENCE_NOTIFY);
>  
> -	if (fence->flags & I915_SW_FENCE_MASK &&
> -	    __i915_sw_fence_notify(fence, FENCE_COMPLETE) != NOTIFY_DONE)
> +	if (__i915_sw_fence_notify(fence, FENCE_COMPLETE) != NOTIFY_DONE)
>  		return;
>  
>  	debug_fence_set_state(fence, DEBUG_FENCE_NOTIFY, DEBUG_FENCE_IDLE);
>  
>  	__i915_sw_fence_wake_up_all(fence, continuation);
> +
> +	debug_fence_destroy(fence);
> +	__i915_sw_fence_notify(fence, FENCE_FREE);
>  }
>  
>  static void i915_sw_fence_complete(struct i915_sw_fence *fence)
> @@ -232,33 +206,26 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence,
>  			  const char *name,
>  			  struct lock_class_key *key)
>  {
> -	BUG_ON((unsigned long)fn & ~I915_SW_FENCE_MASK);
> +	BUG_ON(!fn || (unsigned long)fn & ~I915_SW_FENCE_MASK);
>  
>  	debug_fence_init(fence);
>  
>  	__init_waitqueue_head(&fence->wait, name, key);
> -	kref_init(&fence->kref);
>  	atomic_set(&fence->pending, 1);
>  	fence->flags = (unsigned long)fn;
>  }
>  
> -static void __i915_sw_fence_commit(struct i915_sw_fence *fence)
> -{
> -	i915_sw_fence_complete(fence);
> -	i915_sw_fence_put(fence);
> -}
> -
>  void i915_sw_fence_commit(struct i915_sw_fence *fence)
>  {
>  	debug_fence_activate(fence);
> -	__i915_sw_fence_commit(fence);
> +	i915_sw_fence_complete(fence);
>  }
>  
>  static int i915_sw_fence_wake(wait_queue_t *wq, unsigned mode, int flags, void *key)
>  {
>  	list_del(&wq->task_list);
>  	__i915_sw_fence_complete(wq->private, key);
> -	i915_sw_fence_put(wq->private);
> +
>  	if (wq->flags & I915_SW_FENCE_FLAG_ALLOC)
>  		kfree(wq);
>  	return 0;
> @@ -353,7 +320,7 @@ static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
>  	INIT_LIST_HEAD(&wq->task_list);
>  	wq->flags = pending;
>  	wq->func = i915_sw_fence_wake;
> -	wq->private = i915_sw_fence_get(fence);
> +	wq->private = fence;
>  
>  	i915_sw_fence_await(fence);
>  
> @@ -402,7 +369,7 @@ static void timer_i915_sw_fence_wake(unsigned long data)
>  	dma_fence_put(cb->dma);
>  	cb->dma = NULL;
>  
> -	__i915_sw_fence_commit(cb->fence);
> +	i915_sw_fence_complete(cb->fence);
>  	cb->timer.function = NULL;
>  }
>  
> @@ -413,7 +380,7 @@ static void dma_i915_sw_fence_wake(struct dma_fence *dma,
>  
>  	del_timer_sync(&cb->timer);
>  	if (cb->timer.function)
> -		__i915_sw_fence_commit(cb->fence);
> +		i915_sw_fence_complete(cb->fence);
>  	dma_fence_put(cb->dma);
>  
>  	kfree(cb);
> @@ -440,7 +407,7 @@ int i915_sw_fence_await_dma_fence(struct i915_sw_fence *fence,
>  		return dma_fence_wait(dma, false);
>  	}
>  
> -	cb->fence = i915_sw_fence_get(fence);
> +	cb->fence = fence;
>  	i915_sw_fence_await(fence);
>  
>  	cb->dma = NULL;
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h
> index d31cefbbcc04..1d3b6051daaf 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.h
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.h
> @@ -23,7 +23,6 @@ struct reservation_object;
>  struct i915_sw_fence {
>  	wait_queue_head_t wait;
>  	unsigned long flags;
> -	struct kref kref;
>  	atomic_t pending;
>  };
>  
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx