diff mbox series

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

Message ID 20190318095204.9913-4-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series [01/22] drm/i915: Flush pages on acquisition | expand

Commit Message

Chris Wilson March 18, 2019, 9:51 a.m. UTC
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.

Signed-off-by: Chris Wilson <chris@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(-)

Comments

Tvrtko Ursulin March 18, 2019, 10:31 a.m. UTC | #1
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@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
Chris Wilson March 18, 2019, 10:37 a.m. UTC | #2
Quoting Tvrtko Ursulin (2019-03-18 10:31:57)
> 
> 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: ?

It only becomes a problem with veng as we gain an immediate free path,
whereas at the moment, context frees are deferred until they can acquire
the struct_mutex. We cannot hit this path at the moment, but that we had
to use the safe iterator implies that we were aware that the ring itself
could disappear. If you wanted to pin it on something,

References: b887d6154624 ("drm/i915: Retire requests along rings")

> > Signed-off-by: Chris Wilson <chris@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.

Inside ring_retire_requests(); the use of _safe here actually implies we
met this problem already :)
-Chris
Tvrtko Ursulin March 18, 2019, 10:46 a.m. UTC | #3
On 18/03/2019 10:37, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-03-18 10:31:57)
>>
>> 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: ?
> 
> It only becomes a problem with veng as we gain an immediate free path,
> whereas at the moment, context frees are deferred until they can acquire
> the struct_mutex. We cannot hit this path at the moment, but that we had
> to use the safe iterator implies that we were aware that the ring itself
> could disappear. If you wanted to pin it on something,
> 
> References: b887d6154624 ("drm/i915: Retire requests along rings")
> 
>>> Signed-off-by: Chris Wilson <chris@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.
> 
> Inside ring_retire_requests(); the use of _safe here actually implies we
> met this problem already :)

I get it, the issue is during ring->request_list iteration in 
ring_retire_requests. How about move ring pinning in there so it is clearer?

Regards,

Tvrtko
Chris Wilson March 18, 2019, 10:56 a.m. UTC | #4
Quoting Tvrtko Ursulin (2019-03-18 10:46:28)
> 
> On 18/03/2019 10:37, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-03-18 10:31:57)
> >>
> >> 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: ?
> > 
> > It only becomes a problem with veng as we gain an immediate free path,
> > whereas at the moment, context frees are deferred until they can acquire
> > the struct_mutex. We cannot hit this path at the moment, but that we had
> > to use the safe iterator implies that we were aware that the ring itself
> > could disappear. If you wanted to pin it on something,
> > 
> > References: b887d6154624 ("drm/i915: Retire requests along rings")
> > 
> >>> Signed-off-by: Chris Wilson <chris@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.
> > 
> > Inside ring_retire_requests(); the use of _safe here actually implies we
> > met this problem already :)
> 
> I get it, the issue is during ring->request_list iteration in 
> ring_retire_requests. How about move ring pinning in there so it is clearer?

It's only this path that is affected. That maye becomes clearer when we
retire along timelines and not rings, which is the patchset I wanted to
land to fix this, but figured this was a tiny fix for now.
-Chris
Tvrtko Ursulin March 18, 2019, 1:25 p.m. UTC | #5
On 18/03/2019 10:56, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-03-18 10:46:28)
>>
>> On 18/03/2019 10:37, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-03-18 10:31:57)
>>>>
>>>> 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: ?
>>>
>>> It only becomes a problem with veng as we gain an immediate free path,
>>> whereas at the moment, context frees are deferred until they can acquire
>>> the struct_mutex. We cannot hit this path at the moment, but that we had
>>> to use the safe iterator implies that we were aware that the ring itself
>>> could disappear. If you wanted to pin it on something,
>>>
>>> References: b887d6154624 ("drm/i915: Retire requests along rings")
>>>
>>>>> Signed-off-by: Chris Wilson <chris@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.
>>>
>>> Inside ring_retire_requests(); the use of _safe here actually implies we
>>> met this problem already :)
>>
>> I get it, the issue is during ring->request_list iteration in
>> ring_retire_requests. How about move ring pinning in there so it is clearer?
> 
> It's only this path that is affected. That maye becomes clearer when we
> retire along timelines and not rings, which is the patchset I wanted to
> land to fix this, but figured this was a tiny fix for now.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I failed to find a short and clear suggestion for making the comment 
more precise.

Regards,

Tvrtko
diff mbox series

Patch

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);
+	}
 }
 
 #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);