[Intel-gfx] [PATCH 04/22] drm/i915: Hold a ref to the ring while retiring

[Tvrtko Ursulin]

On 18/03/2019 09:51, Chris Wilson wrote:
> As the final request on a ring may hold the reference to this ring (via
> retiring the last pinned context), we may find ourselves chasing a
> dangling pointer on completion of the list.
> 
> A quick solution is to hold a reference to the ring itself as we retire
> along it so that we only free it after we stop dereferencing it.

Is there a guilty commit to reference as Fixes: ?


> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_request.c          |  6 +++++-
>   drivers/gpu/drm/i915/intel_engine_types.h    |  2 ++
>   drivers/gpu/drm/i915/intel_lrc.c             |  4 ++--
>   drivers/gpu/drm/i915/intel_ringbuffer.c      |  9 +++++----
>   drivers/gpu/drm/i915/intel_ringbuffer.h      | 13 ++++++++++++-
>   drivers/gpu/drm/i915/selftests/mock_engine.c |  1 +
>   6 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 9533a85cb0b3..0a3d94517d0a 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1332,8 +1332,12 @@ void i915_retire_requests(struct drm_i915_private *i915)
>   	if (!i915->gt.active_requests)
>   		return;
>   
> -	list_for_each_entry_safe(ring, tmp, &i915->gt.active_rings, active_link)
> +	list_for_each_entry_safe(ring, tmp,
> +				 &i915->gt.active_rings, active_link) {
> +		intel_ring_get(ring); /* last rq holds reference! */
>   		ring_retire_requests(ring);
> +		intel_ring_put(ring);
> +	}

Where does it chase a dangling pointer? It used the safe iterator already.

>   }
>   
>   #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> diff --git a/drivers/gpu/drm/i915/intel_engine_types.h b/drivers/gpu/drm/i915/intel_engine_types.h
> index 79a166b9a81b..549fdfca17aa 100644
> --- a/drivers/gpu/drm/i915/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/intel_engine_types.h
> @@ -9,6 +9,7 @@
>   
>   #include <linux/hashtable.h>
>   #include <linux/irq_work.h>
> +#include <linux/kref.h>
>   #include <linux/list.h>
>   #include <linux/types.h>
>   
> @@ -58,6 +59,7 @@ struct intel_engine_hangcheck {
>   };
>   
>   struct intel_ring {
> +	struct kref ref;
>   	struct i915_vma *vma;
>   	void *vaddr;
>   
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index aa50f03ba812..d3f1fe06d013 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1236,7 +1236,7 @@ static void execlists_submit_request(struct i915_request *request)
>   
>   static void __execlists_context_fini(struct intel_context *ce)
>   {
> -	intel_ring_free(ce->ring);
> +	intel_ring_put(ce->ring);
>   
>   	GEM_BUG_ON(i915_gem_object_is_active(ce->state->obj));
>   	i915_gem_object_put(ce->state->obj);
> @@ -2867,7 +2867,7 @@ static int execlists_context_deferred_alloc(struct intel_context *ce,
>   	return 0;
>   
>   error_ring_free:
> -	intel_ring_free(ring);
> +	intel_ring_put(ring);
>   error_deref_obj:
>   	i915_gem_object_put(ctx_obj);
>   	return ret;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 746fe570466c..45a54fadc482 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1302,6 +1302,7 @@ intel_engine_create_ring(struct intel_engine_cs *engine,
>   	if (!ring)
>   		return ERR_PTR(-ENOMEM);
>   
> +	kref_init(&ring->ref);
>   	INIT_LIST_HEAD(&ring->request_list);
>   	ring->timeline = i915_timeline_get(timeline);
>   
> @@ -1326,9 +1327,9 @@ intel_engine_create_ring(struct intel_engine_cs *engine,
>   	return ring;
>   }
>   
> -void
> -intel_ring_free(struct intel_ring *ring)
> +void intel_ring_free(struct kref *ref)
>   {
> +	struct intel_ring *ring = container_of(ref, typeof(*ring), ref);
>   	struct drm_i915_gem_object *obj = ring->vma->obj;
>   
>   	i915_vma_close(ring->vma);
> @@ -1571,7 +1572,7 @@ static int intel_init_ring_buffer(struct intel_engine_cs *engine)
>   err_unpin:
>   	intel_ring_unpin(ring);
>   err_ring:
> -	intel_ring_free(ring);
> +	intel_ring_put(ring);
>   err:
>   	intel_engine_cleanup_common(engine);
>   	return err;
> @@ -1585,7 +1586,7 @@ void intel_engine_cleanup(struct intel_engine_cs *engine)
>   		(I915_READ_MODE(engine) & MODE_IDLE) == 0);
>   
>   	intel_ring_unpin(engine->buffer);
> -	intel_ring_free(engine->buffer);
> +	intel_ring_put(engine->buffer);
>   
>   	if (engine->cleanup)
>   		engine->cleanup(engine);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index e612bdca9fd9..a57489fcb302 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -231,7 +231,18 @@ int intel_ring_pin(struct intel_ring *ring);
>   void intel_ring_reset(struct intel_ring *ring, u32 tail);
>   unsigned int intel_ring_update_space(struct intel_ring *ring);
>   void intel_ring_unpin(struct intel_ring *ring);
> -void intel_ring_free(struct intel_ring *ring);
> +void intel_ring_free(struct kref *ref);
> +
> +static inline struct intel_ring *intel_ring_get(struct intel_ring *ring)
> +{
> +	kref_get(&ring->ref);
> +	return ring;
> +}
> +
> +static inline void intel_ring_put(struct intel_ring *ring)
> +{
> +	kref_put(&ring->ref, intel_ring_free);
> +}
>   
>   void intel_engine_stop(struct intel_engine_cs *engine);
>   void intel_engine_cleanup(struct intel_engine_cs *engine);
> diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c
> index f6d120e05ee4..881450c694e9 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_engine.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
> @@ -57,6 +57,7 @@ static struct intel_ring *mock_ring(struct intel_engine_cs *engine)
>   		return NULL;
>   	}
>   
> +	kref_init(&ring->base.ref);
>   	ring->base.size = sz;
>   	ring->base.effective_size = sz;
>   	ring->base.vaddr = (void *)(ring + 1);
> 

Regards,

Tvrtko