| Message ID | 1415967559-17074-12-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf > Of John.C.Harrison@Intel.com > Sent: Friday, November 14, 2014 12:19 PM > To: Intel-GFX@Lists.FreeDesktop.Org > Subject: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly request > deference facility > > From: John Harrison <John.C.Harrison@Intel.com> > > The next patches in the series convert some display related seqno usage to > request structure usage. However, the request dereference introduced > must be done from interrupt context. As the dereference potentially > involves freeing the request structure and thus calling lots of non-interrupt > friendly code, this poses a problem. > > The solution is to add an IRQ friendly version of the dereference function. All > this does is to add the request structure to a 'delayed free' list and return. > The retire code, which is run periodically, then processes this list and does > the actual dereferencing of the request structures. > > v2: Added a count to allow for multiple IRQ dereferences of the same > request at a time. > > For: VIZ-4377 > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 7 +++++++ > drivers/gpu/drm/i915/i915_gem.c | 33 > +++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_lrc.c | 2 ++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +++ > drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++ > 5 files changed, 47 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h index 180b674..87cb355 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2019,6 +2019,10 @@ struct drm_i915_gem_request { > struct drm_i915_file_private *file_priv; > /** file_priv list entry for this request */ > struct list_head client_list; > + > + /** deferred free list for dereferencing from IRQ context */ > + struct list_head delay_free_list; > + uint32_t delay_free_count; > }; > > void i915_gem_request_free(struct kref *req_ref); @@ -2044,9 +2048,12 > @@ i915_gem_request_reference(struct drm_i915_gem_request *req) > static inline void i915_gem_request_unreference(struct > drm_i915_gem_request *req) { > + WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex)); > kref_put(&req->ref, i915_gem_request_free); } > > +void i915_gem_request_unreference_irq(struct drm_i915_gem_request > +*req); > + > static inline void i915_gem_request_assign(struct drm_i915_gem_request > **pdst, > struct drm_i915_gem_request *src) > { diff --git a/drivers/gpu/drm/i915/i915_gem.c > b/drivers/gpu/drm/i915/i915_gem.c index 60e5eec..8453bbd 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2722,6 +2722,19 @@ void i915_gem_reset(struct drm_device *dev) > i915_gem_restore_fences(dev); > } > > +void i915_gem_request_unreference_irq(struct drm_i915_gem_request > *req) > +{ > + struct intel_engine_cs *ring = req->ring; > + unsigned long flags; > + > + /* Need to add the req to a deferred dereference list to be > processed > + * outside of interrupt time */ > + spin_lock_irqsave(&ring->reqlist_lock, flags); > + if (req->delay_free_count++ == 0) > + list_add_tail(&req->delay_free_list, &ring- > >delayed_free_list); > + spin_unlock_irqrestore(&ring->reqlist_lock, flags); } > + > /** > * This function clears the request list as sequence numbers are passed. > */ > @@ -2796,6 +2809,25 @@ i915_gem_retire_requests_ring(struct > intel_engine_cs *ring) > ring->trace_irq_seqno = 0; > } > > + while (!list_empty(&ring->delayed_free_list)) { > + struct drm_i915_gem_request *request; > + unsigned long flags; > + uint32_t count; > + > + request = list_first_entry(&ring->delayed_free_list, > + struct drm_i915_gem_request, > + delay_free_list); > + > + spin_lock_irqsave(&request->ring->reqlist_lock, flags); > + list_del(&request->delay_free_list); > + count = request->delay_free_count; > + request->delay_free_count = 0; > + spin_unlock_irqrestore(&request->ring->reqlist_lock, flags); > + > + while (count-- > 0) > + i915_gem_request_unreference(request); > + } > + > WARN_ON(i915_verify_lists(ring->dev)); > } > > @@ -5055,6 +5087,7 @@ init_ring_lists(struct intel_engine_cs *ring) { > INIT_LIST_HEAD(&ring->active_list); > INIT_LIST_HEAD(&ring->request_list); > + INIT_LIST_HEAD(&ring->delayed_free_list); Same comment as before - multiple init points for this list. Thomas. > } > > void i915_init_vm(struct drm_i915_private *dev_priv, diff --git > a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index eba0acd..db8efaa 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1287,7 +1287,9 @@ static int logical_ring_init(struct drm_device *dev, > struct intel_engine_cs *rin > ring->dev = dev; > INIT_LIST_HEAD(&ring->active_list); > INIT_LIST_HEAD(&ring->request_list); > + spin_lock_init(&ring->reqlist_lock); > init_waitqueue_head(&ring->irq_queue); > + INIT_LIST_HEAD(&ring->delayed_free_list); > > INIT_LIST_HEAD(&ring->execlist_queue); > spin_lock_init(&ring->execlist_lock); > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 74c48ed..4338132 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -1808,6 +1808,8 @@ static int intel_init_ring_buffer(struct drm_device > *dev, > ring->dev = dev; > INIT_LIST_HEAD(&ring->active_list); > INIT_LIST_HEAD(&ring->request_list); > + spin_lock_init(&ring->reqlist_lock); > + INIT_LIST_HEAD(&ring->delayed_free_list); > INIT_LIST_HEAD(&ring->execlist_queue); > ringbuf->size = 32 * PAGE_SIZE; > ringbuf->ring = ring; > @@ -2510,6 +2512,7 @@ int intel_render_ring_init_dri(struct drm_device > *dev, u64 start, u32 size) > ring->dev = dev; > INIT_LIST_HEAD(&ring->active_list); > INIT_LIST_HEAD(&ring->request_list); > + INIT_LIST_HEAD(&ring->delayed_free_list); > > ringbuf->size = size; > ringbuf->effective_size = ringbuf->size; diff --git > a/drivers/gpu/drm/i915/intel_ringbuffer.h > b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 824f5884..3af7b7c 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -262,6 +262,8 @@ struct intel_engine_cs { > * outstanding. > */ > struct list_head request_list; > + spinlock_t reqlist_lock; > + struct list_head delayed_free_list; > > /** > * Do we have some not yet emitted requests outstanding? > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 18/11/2014 09:34, Daniel, Thomas wrote: >> -----Original Message----- >> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf >> Of John.C.Harrison@Intel.com >> Sent: Friday, November 14, 2014 12:19 PM >> To: Intel-GFX@Lists.FreeDesktop.Org >> Subject: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly request >> deference facility >> >> From: John Harrison <John.C.Harrison@Intel.com> >> >> The next patches in the series convert some display related seqno usage to >> request structure usage. However, the request dereference introduced >> must be done from interrupt context. As the dereference potentially >> involves freeing the request structure and thus calling lots of non-interrupt >> friendly code, this poses a problem. >> >> The solution is to add an IRQ friendly version of the dereference function. All >> this does is to add the request structure to a 'delayed free' list and return. >> The retire code, which is run periodically, then processes this list and does >> the actual dereferencing of the request structures. >> >> v2: Added a count to allow for multiple IRQ dereferences of the same >> request at a time. >> >> For: VIZ-4377 >> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 7 +++++++ >> drivers/gpu/drm/i915/i915_gem.c | 33 >> +++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_lrc.c | 2 ++ >> drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +++ >> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++ >> 5 files changed, 47 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h index 180b674..87cb355 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2019,6 +2019,10 @@ struct drm_i915_gem_request { >> struct drm_i915_file_private *file_priv; >> /** file_priv list entry for this request */ >> struct list_head client_list; >> + >> + /** deferred free list for dereferencing from IRQ context */ >> + struct list_head delay_free_list; >> + uint32_t delay_free_count; >> }; >> >> void i915_gem_request_free(struct kref *req_ref); @@ -2044,9 +2048,12 >> @@ i915_gem_request_reference(struct drm_i915_gem_request *req) >> static inline void i915_gem_request_unreference(struct >> drm_i915_gem_request *req) { >> + WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex)); >> kref_put(&req->ref, i915_gem_request_free); } >> >> +void i915_gem_request_unreference_irq(struct drm_i915_gem_request >> +*req); >> + >> static inline void i915_gem_request_assign(struct drm_i915_gem_request >> **pdst, >> struct drm_i915_gem_request *src) >> { diff --git a/drivers/gpu/drm/i915/i915_gem.c >> b/drivers/gpu/drm/i915/i915_gem.c index 60e5eec..8453bbd 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -2722,6 +2722,19 @@ void i915_gem_reset(struct drm_device *dev) >> i915_gem_restore_fences(dev); >> } >> >> +void i915_gem_request_unreference_irq(struct drm_i915_gem_request >> *req) >> +{ >> + struct intel_engine_cs *ring = req->ring; >> + unsigned long flags; >> + >> + /* Need to add the req to a deferred dereference list to be >> processed >> + * outside of interrupt time */ >> + spin_lock_irqsave(&ring->reqlist_lock, flags); >> + if (req->delay_free_count++ == 0) >> + list_add_tail(&req->delay_free_list, &ring- >>> delayed_free_list); >> + spin_unlock_irqrestore(&ring->reqlist_lock, flags); } >> + >> /** >> * This function clears the request list as sequence numbers are passed. >> */ >> @@ -2796,6 +2809,25 @@ i915_gem_retire_requests_ring(struct >> intel_engine_cs *ring) >> ring->trace_irq_seqno = 0; >> } >> >> + while (!list_empty(&ring->delayed_free_list)) { >> + struct drm_i915_gem_request *request; >> + unsigned long flags; >> + uint32_t count; >> + >> + request = list_first_entry(&ring->delayed_free_list, >> + struct drm_i915_gem_request, >> + delay_free_list); >> + >> + spin_lock_irqsave(&request->ring->reqlist_lock, flags); >> + list_del(&request->delay_free_list); >> + count = request->delay_free_count; >> + request->delay_free_count = 0; >> + spin_unlock_irqrestore(&request->ring->reqlist_lock, flags); >> + >> + while (count-- > 0) >> + i915_gem_request_unreference(request); >> + } >> + >> WARN_ON(i915_verify_lists(ring->dev)); >> } >> >> @@ -5055,6 +5087,7 @@ init_ring_lists(struct intel_engine_cs *ring) { >> INIT_LIST_HEAD(&ring->active_list); >> INIT_LIST_HEAD(&ring->request_list); >> + INIT_LIST_HEAD(&ring->delayed_free_list); > Same comment as before - multiple init points for this list. > > Thomas. Same reply as before: the function already exists and is already initialising other lists therefore it seems sensible to initialise the new list as well. Whether the function as a whole is redundant or not is unclear. The same duplicated initialisation also happens in intel_render_ring_init_dri(). If someone thinks these can all be simplified and wants to only do the initialisation once in one place then that should be another patch. >> } >> >> void i915_init_vm(struct drm_i915_private *dev_priv, diff --git >> a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >> index eba0acd..db8efaa 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -1287,7 +1287,9 @@ static int logical_ring_init(struct drm_device *dev, >> struct intel_engine_cs *rin >> ring->dev = dev; >> INIT_LIST_HEAD(&ring->active_list); >> INIT_LIST_HEAD(&ring->request_list); >> + spin_lock_init(&ring->reqlist_lock); >> init_waitqueue_head(&ring->irq_queue); >> + INIT_LIST_HEAD(&ring->delayed_free_list); >> >> INIT_LIST_HEAD(&ring->execlist_queue); >> spin_lock_init(&ring->execlist_lock); >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c >> b/drivers/gpu/drm/i915/intel_ringbuffer.c >> index 74c48ed..4338132 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >> @@ -1808,6 +1808,8 @@ static int intel_init_ring_buffer(struct drm_device >> *dev, >> ring->dev = dev; >> INIT_LIST_HEAD(&ring->active_list); >> INIT_LIST_HEAD(&ring->request_list); >> + spin_lock_init(&ring->reqlist_lock); >> + INIT_LIST_HEAD(&ring->delayed_free_list); >> INIT_LIST_HEAD(&ring->execlist_queue); >> ringbuf->size = 32 * PAGE_SIZE; >> ringbuf->ring = ring; >> @@ -2510,6 +2512,7 @@ int intel_render_ring_init_dri(struct drm_device >> *dev, u64 start, u32 size) >> ring->dev = dev; >> INIT_LIST_HEAD(&ring->active_list); >> INIT_LIST_HEAD(&ring->request_list); >> + INIT_LIST_HEAD(&ring->delayed_free_list); >> >> ringbuf->size = size; >> ringbuf->effective_size = ringbuf->size; diff --git >> a/drivers/gpu/drm/i915/intel_ringbuffer.h >> b/drivers/gpu/drm/i915/intel_ringbuffer.h >> index 824f5884..3af7b7c 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h >> @@ -262,6 +262,8 @@ struct intel_engine_cs { >> * outstanding. >> */ >> struct list_head request_list; >> + spinlock_t reqlist_lock; >> + struct list_head delayed_free_list; >> >> /** >> * Do we have some not yet emitted requests outstanding? >> -- >> 1.7.9.5 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Harrison, John C > Sent: Wednesday, November 19, 2014 12:26 PM > To: Daniel, Thomas; Intel-GFX@Lists.FreeDesktop.Org > Subject: Re: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly request > deference facility > > On 18/11/2014 09:34, Daniel, Thomas wrote: > >> -----Original Message----- > >> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On > >> Behalf Of John.C.Harrison@Intel.com > >> Sent: Friday, November 14, 2014 12:19 PM > >> To: Intel-GFX@Lists.FreeDesktop.Org > >> Subject: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly > >> request deference facility > >> > >> From: John Harrison <John.C.Harrison@Intel.com> > >> > >> The next patches in the series convert some display related seqno > >> usage to request structure usage. However, the request dereference > >> introduced must be done from interrupt context. As the dereference > >> potentially involves freeing the request structure and thus calling > >> lots of non-interrupt friendly code, this poses a problem. > >> > >> The solution is to add an IRQ friendly version of the dereference > >> function. All this does is to add the request structure to a 'delayed free' > list and return. > >> The retire code, which is run periodically, then processes this list > >> and does the actual dereferencing of the request structures. > >> > >> v2: Added a count to allow for multiple IRQ dereferences of the same > >> request at a time. > >> > >> For: VIZ-4377 > >> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_drv.h | 7 +++++++ > >> drivers/gpu/drm/i915/i915_gem.c | 33 > >> +++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/i915/intel_lrc.c | 2 ++ > >> drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +++ > >> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++ > >> 5 files changed, 47 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h > >> b/drivers/gpu/drm/i915/i915_drv.h index 180b674..87cb355 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> @@ -2019,6 +2019,10 @@ struct drm_i915_gem_request { > >> struct drm_i915_file_private *file_priv; > >> /** file_priv list entry for this request */ > >> struct list_head client_list; > >> + > >> + /** deferred free list for dereferencing from IRQ context */ > >> + struct list_head delay_free_list; > >> + uint32_t delay_free_count; > >> }; > >> > >> void i915_gem_request_free(struct kref *req_ref); @@ -2044,9 > >> +2048,12 @@ i915_gem_request_reference(struct > drm_i915_gem_request > >> *req) static inline void i915_gem_request_unreference(struct > >> drm_i915_gem_request *req) { > >> + WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex)); > >> kref_put(&req->ref, i915_gem_request_free); } > >> > >> +void i915_gem_request_unreference_irq(struct > drm_i915_gem_request > >> +*req); > >> + > >> static inline void i915_gem_request_assign(struct > >> drm_i915_gem_request **pdst, > >> struct drm_i915_gem_request *src) > { diff --git > >> a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >> index 60e5eec..8453bbd 100644 > >> --- a/drivers/gpu/drm/i915/i915_gem.c > >> +++ b/drivers/gpu/drm/i915/i915_gem.c > >> @@ -2722,6 +2722,19 @@ void i915_gem_reset(struct drm_device *dev) > >> i915_gem_restore_fences(dev); > >> } > >> > >> +void i915_gem_request_unreference_irq(struct > drm_i915_gem_request > >> *req) > >> +{ > >> + struct intel_engine_cs *ring = req->ring; > >> + unsigned long flags; > >> + > >> + /* Need to add the req to a deferred dereference list to be > >> processed > >> + * outside of interrupt time */ > >> + spin_lock_irqsave(&ring->reqlist_lock, flags); > >> + if (req->delay_free_count++ == 0) > >> + list_add_tail(&req->delay_free_list, &ring- > >>> delayed_free_list); > >> + spin_unlock_irqrestore(&ring->reqlist_lock, flags); } > >> + > >> /** > >> * This function clears the request list as sequence numbers are passed. > >> */ > >> @@ -2796,6 +2809,25 @@ i915_gem_retire_requests_ring(struct > >> intel_engine_cs *ring) > >> ring->trace_irq_seqno = 0; > >> } > >> > >> + while (!list_empty(&ring->delayed_free_list)) { > >> + struct drm_i915_gem_request *request; > >> + unsigned long flags; > >> + uint32_t count; > >> + > >> + request = list_first_entry(&ring->delayed_free_list, > >> + struct drm_i915_gem_request, > >> + delay_free_list); > >> + > >> + spin_lock_irqsave(&request->ring->reqlist_lock, flags); > >> + list_del(&request->delay_free_list); > >> + count = request->delay_free_count; > >> + request->delay_free_count = 0; > >> + spin_unlock_irqrestore(&request->ring->reqlist_lock, flags); > >> + > >> + while (count-- > 0) > >> + i915_gem_request_unreference(request); > >> + } > >> + > >> WARN_ON(i915_verify_lists(ring->dev)); > >> } > >> > >> @@ -5055,6 +5087,7 @@ init_ring_lists(struct intel_engine_cs *ring) { > >> INIT_LIST_HEAD(&ring->active_list); > >> INIT_LIST_HEAD(&ring->request_list); > >> + INIT_LIST_HEAD(&ring->delayed_free_list); > > Same comment as before - multiple init points for this list. > > > > Thomas. > Same reply as before: the function already exists and is already initialising You forgot to reply to the mailing list last time. > other lists therefore it seems sensible to initialise the new list as well. > Whether the function as a whole is redundant or not is unclear. The same You've introduced a new list - surely it's your responsibility to know where it should be initialised? > duplicated initialisation also happens in intel_render_ring_init_dri(). If > someone thinks these can all be simplified and wants to only do the > initialisation once in one place then that should be another patch. Agree that the other duplicate inits should also be fixed up in another patch but that's no reason to add another dupe. Thomas. > >> } > >> > >> void i915_init_vm(struct drm_i915_private *dev_priv, diff --git > >> a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > >> index eba0acd..db8efaa 100644 > >> --- a/drivers/gpu/drm/i915/intel_lrc.c > >> +++ b/drivers/gpu/drm/i915/intel_lrc.c > >> @@ -1287,7 +1287,9 @@ static int logical_ring_init(struct drm_device > >> *dev, struct intel_engine_cs *rin > >> ring->dev = dev; > >> INIT_LIST_HEAD(&ring->active_list); > >> INIT_LIST_HEAD(&ring->request_list); > >> + spin_lock_init(&ring->reqlist_lock); > >> init_waitqueue_head(&ring->irq_queue); > >> + INIT_LIST_HEAD(&ring->delayed_free_list); > >> > >> INIT_LIST_HEAD(&ring->execlist_queue); > >> spin_lock_init(&ring->execlist_lock); > >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > >> b/drivers/gpu/drm/i915/intel_ringbuffer.c > >> index 74c48ed..4338132 100644 > >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > >> @@ -1808,6 +1808,8 @@ static int intel_init_ring_buffer(struct > >> drm_device *dev, > >> ring->dev = dev; > >> INIT_LIST_HEAD(&ring->active_list); > >> INIT_LIST_HEAD(&ring->request_list); > >> + spin_lock_init(&ring->reqlist_lock); > >> + INIT_LIST_HEAD(&ring->delayed_free_list); > >> INIT_LIST_HEAD(&ring->execlist_queue); > >> ringbuf->size = 32 * PAGE_SIZE; > >> ringbuf->ring = ring; > >> @@ -2510,6 +2512,7 @@ int intel_render_ring_init_dri(struct > >> drm_device *dev, u64 start, u32 size) > >> ring->dev = dev; > >> INIT_LIST_HEAD(&ring->active_list); > >> INIT_LIST_HEAD(&ring->request_list); > >> + INIT_LIST_HEAD(&ring->delayed_free_list); > >> > >> ringbuf->size = size; > >> ringbuf->effective_size = ringbuf->size; diff --git > >> a/drivers/gpu/drm/i915/intel_ringbuffer.h > >> b/drivers/gpu/drm/i915/intel_ringbuffer.h > >> index 824f5884..3af7b7c 100644 > >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > >> @@ -262,6 +262,8 @@ struct intel_engine_cs { > >> * outstanding. > >> */ > >> struct list_head request_list; > >> + spinlock_t reqlist_lock; > >> + struct list_head delayed_free_list; > >> > >> /** > >> * Do we have some not yet emitted requests outstanding? > >> -- > >> 1.7.9.5 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 19/11/2014 13:28, Daniel, Thomas wrote: >> -----Original Message----- >> From: Harrison, John C >> Sent: Wednesday, November 19, 2014 12:26 PM >> To: Daniel, Thomas; Intel-GFX@Lists.FreeDesktop.Org >> Subject: Re: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly request >> deference facility >> >> On 18/11/2014 09:34, Daniel, Thomas wrote: >>>> -----Original Message----- >>>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On >>>> Behalf Of John.C.Harrison@Intel.com >>>> Sent: Friday, November 14, 2014 12:19 PM >>>> To: Intel-GFX@Lists.FreeDesktop.Org >>>> Subject: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly >>>> request deference facility >>>> >>>> From: John Harrison <John.C.Harrison@Intel.com> >>>> >>>> The next patches in the series convert some display related seqno >>>> usage to request structure usage. However, the request dereference >>>> introduced must be done from interrupt context. As the dereference >>>> potentially involves freeing the request structure and thus calling >>>> lots of non-interrupt friendly code, this poses a problem. >>>> >>>> The solution is to add an IRQ friendly version of the dereference >>>> function. All this does is to add the request structure to a 'delayed free' >> list and return. >>>> The retire code, which is run periodically, then processes this list >>>> and does the actual dereferencing of the request structures. >>>> >>>> v2: Added a count to allow for multiple IRQ dereferences of the same >>>> request at a time. >>>> >>>> For: VIZ-4377 >>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_drv.h | 7 +++++++ >>>> drivers/gpu/drm/i915/i915_gem.c | 33 >>>> +++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/i915/intel_lrc.c | 2 ++ >>>> drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +++ >>>> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++ >>>> 5 files changed, 47 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h >>>> b/drivers/gpu/drm/i915/i915_drv.h index 180b674..87cb355 100644 >>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>> @@ -2019,6 +2019,10 @@ struct drm_i915_gem_request { >>>> struct drm_i915_file_private *file_priv; >>>> /** file_priv list entry for this request */ >>>> struct list_head client_list; >>>> + >>>> + /** deferred free list for dereferencing from IRQ context */ >>>> + struct list_head delay_free_list; >>>> + uint32_t delay_free_count; >>>> }; >>>> >>>> void i915_gem_request_free(struct kref *req_ref); @@ -2044,9 >>>> +2048,12 @@ i915_gem_request_reference(struct >> drm_i915_gem_request >>>> *req) static inline void i915_gem_request_unreference(struct >>>> drm_i915_gem_request *req) { >>>> + WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex)); >>>> kref_put(&req->ref, i915_gem_request_free); } >>>> >>>> +void i915_gem_request_unreference_irq(struct >> drm_i915_gem_request >>>> +*req); >>>> + >>>> static inline void i915_gem_request_assign(struct >>>> drm_i915_gem_request **pdst, >>>> struct drm_i915_gem_request *src) >> { diff --git >>>> a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >>>> index 60e5eec..8453bbd 100644 >>>> --- a/drivers/gpu/drm/i915/i915_gem.c >>>> +++ b/drivers/gpu/drm/i915/i915_gem.c >>>> @@ -2722,6 +2722,19 @@ void i915_gem_reset(struct drm_device *dev) >>>> i915_gem_restore_fences(dev); >>>> } >>>> >>>> +void i915_gem_request_unreference_irq(struct >> drm_i915_gem_request >>>> *req) >>>> +{ >>>> + struct intel_engine_cs *ring = req->ring; >>>> + unsigned long flags; >>>> + >>>> + /* Need to add the req to a deferred dereference list to be >>>> processed >>>> + * outside of interrupt time */ >>>> + spin_lock_irqsave(&ring->reqlist_lock, flags); >>>> + if (req->delay_free_count++ == 0) >>>> + list_add_tail(&req->delay_free_list, &ring- >>>>> delayed_free_list); >>>> + spin_unlock_irqrestore(&ring->reqlist_lock, flags); } >>>> + >>>> /** >>>> * This function clears the request list as sequence numbers are passed. >>>> */ >>>> @@ -2796,6 +2809,25 @@ i915_gem_retire_requests_ring(struct >>>> intel_engine_cs *ring) >>>> ring->trace_irq_seqno = 0; >>>> } >>>> >>>> + while (!list_empty(&ring->delayed_free_list)) { >>>> + struct drm_i915_gem_request *request; >>>> + unsigned long flags; >>>> + uint32_t count; >>>> + >>>> + request = list_first_entry(&ring->delayed_free_list, >>>> + struct drm_i915_gem_request, >>>> + delay_free_list); >>>> + >>>> + spin_lock_irqsave(&request->ring->reqlist_lock, flags); >>>> + list_del(&request->delay_free_list); >>>> + count = request->delay_free_count; >>>> + request->delay_free_count = 0; >>>> + spin_unlock_irqrestore(&request->ring->reqlist_lock, flags); >>>> + >>>> + while (count-- > 0) >>>> + i915_gem_request_unreference(request); >>>> + } >>>> + >>>> WARN_ON(i915_verify_lists(ring->dev)); >>>> } >>>> >>>> @@ -5055,6 +5087,7 @@ init_ring_lists(struct intel_engine_cs *ring) { >>>> INIT_LIST_HEAD(&ring->active_list); >>>> INIT_LIST_HEAD(&ring->request_list); >>>> + INIT_LIST_HEAD(&ring->delayed_free_list); >>> Same comment as before - multiple init points for this list. >>> >>> Thomas. >> Same reply as before: the function already exists and is already initialising > You forgot to reply to the mailing list last time. > >> other lists therefore it seems sensible to initialise the new list as well. >> Whether the function as a whole is redundant or not is unclear. The same > You've introduced a new list - surely it's your responsibility to know where it should be initialised? My list is an extension of the existing 'request_list'. Therefore is seems prudent to initialise it wherever better minds than me have deemed it necessary to initialise request_list itself. >> duplicated initialisation also happens in intel_render_ring_init_dri(). If >> someone thinks these can all be simplified and wants to only do the >> initialisation once in one place then that should be another patch. > Agree that the other duplicate inits should also be fixed up in another patch but that's no reason to add another dupe. > > Thomas. Only if it is definitely a redundant duplication. Currently, it is not obvious that it is therefore I am being safe/sensible by following the existing convention. >>>> } >>>> >>>> void i915_init_vm(struct drm_i915_private *dev_priv, diff --git >>>> a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >>>> index eba0acd..db8efaa 100644 >>>> --- a/drivers/gpu/drm/i915/intel_lrc.c >>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c >>>> @@ -1287,7 +1287,9 @@ static int logical_ring_init(struct drm_device >>>> *dev, struct intel_engine_cs *rin >>>> ring->dev = dev; >>>> INIT_LIST_HEAD(&ring->active_list); >>>> INIT_LIST_HEAD(&ring->request_list); >>>> + spin_lock_init(&ring->reqlist_lock); >>>> init_waitqueue_head(&ring->irq_queue); >>>> + INIT_LIST_HEAD(&ring->delayed_free_list); >>>> >>>> INIT_LIST_HEAD(&ring->execlist_queue); >>>> spin_lock_init(&ring->execlist_lock); >>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c >>>> b/drivers/gpu/drm/i915/intel_ringbuffer.c >>>> index 74c48ed..4338132 100644 >>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >>>> @@ -1808,6 +1808,8 @@ static int intel_init_ring_buffer(struct >>>> drm_device *dev, >>>> ring->dev = dev; >>>> INIT_LIST_HEAD(&ring->active_list); >>>> INIT_LIST_HEAD(&ring->request_list); >>>> + spin_lock_init(&ring->reqlist_lock); >>>> + INIT_LIST_HEAD(&ring->delayed_free_list); >>>> INIT_LIST_HEAD(&ring->execlist_queue); >>>> ringbuf->size = 32 * PAGE_SIZE; >>>> ringbuf->ring = ring; >>>> @@ -2510,6 +2512,7 @@ int intel_render_ring_init_dri(struct >>>> drm_device *dev, u64 start, u32 size) >>>> ring->dev = dev; >>>> INIT_LIST_HEAD(&ring->active_list); >>>> INIT_LIST_HEAD(&ring->request_list); >>>> + INIT_LIST_HEAD(&ring->delayed_free_list); >>>> >>>> ringbuf->size = size; >>>> ringbuf->effective_size = ringbuf->size; diff --git >>>> a/drivers/gpu/drm/i915/intel_ringbuffer.h >>>> b/drivers/gpu/drm/i915/intel_ringbuffer.h >>>> index 824f5884..3af7b7c 100644 >>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h >>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h >>>> @@ -262,6 +262,8 @@ struct intel_engine_cs { >>>> * outstanding. >>>> */ >>>> struct list_head request_list; >>>> + spinlock_t reqlist_lock; >>>> + struct list_head delayed_free_list; >>>> >>>> /** >>>> * Do we have some not yet emitted requests outstanding? >>>> -- >>>> 1.7.9.5 >>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Harrison, John C > Sent: Wednesday, November 19, 2014 5:05 PM > To: Daniel, Thomas; Intel-GFX@Lists.FreeDesktop.Org > Subject: Re: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly request > deference facility > > On 19/11/2014 13:28, Daniel, Thomas wrote: > >> -----Original Message----- > >> From: Harrison, John C > >> Sent: Wednesday, November 19, 2014 12:26 PM > >> To: Daniel, Thomas; Intel-GFX@Lists.FreeDesktop.Org > >> Subject: Re: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly > >> request deference facility > >> > >> On 18/11/2014 09:34, Daniel, Thomas wrote: > >>>> -----Original Message----- > >>>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On > >>>> Behalf Of John.C.Harrison@Intel.com > >>>> Sent: Friday, November 14, 2014 12:19 PM > >>>> To: Intel-GFX@Lists.FreeDesktop.Org > >>>> Subject: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly > >>>> request deference facility > >>>> > >>>> From: John Harrison <John.C.Harrison@Intel.com> > >>>> > >>>> @@ -2796,6 +2809,25 @@ i915_gem_retire_requests_ring(struct > >>>> intel_engine_cs *ring) > >>>> ring->trace_irq_seqno = 0; > >>>> } > >>>> > >>>> + while (!list_empty(&ring->delayed_free_list)) { > >>>> + struct drm_i915_gem_request *request; > >>>> + unsigned long flags; > >>>> + uint32_t count; > >>>> + > >>>> + request = list_first_entry(&ring->delayed_free_list, > >>>> + struct drm_i915_gem_request, > >>>> + delay_free_list); > >>>> + > >>>> + spin_lock_irqsave(&request->ring->reqlist_lock, flags); > >>>> + list_del(&request->delay_free_list); > >>>> + count = request->delay_free_count; > >>>> + request->delay_free_count = 0; > >>>> + spin_unlock_irqrestore(&request->ring->reqlist_lock, flags); > >>>> + > >>>> + while (count-- > 0) > >>>> + i915_gem_request_unreference(request); > >>>> + } > >>>> + > >>>> WARN_ON(i915_verify_lists(ring->dev)); > >>>> } > >>>> > >>>> @@ -5055,6 +5087,7 @@ init_ring_lists(struct intel_engine_cs *ring) { > >>>> INIT_LIST_HEAD(&ring->active_list); > >>>> INIT_LIST_HEAD(&ring->request_list); > >>>> + INIT_LIST_HEAD(&ring->delayed_free_list); > >>> Same comment as before - multiple init points for this list. > >>> > >>> Thomas. > >> Same reply as before: the function already exists and is already > >> initialising > > You forgot to reply to the mailing list last time. > > > >> other lists therefore it seems sensible to initialise the new list as well. > >> Whether the function as a whole is redundant or not is unclear. The > >> same > > You've introduced a new list - surely it's your responsibility to know where > it should be initialised? > My list is an extension of the existing 'request_list'. Therefore is seems > prudent to initialise it wherever better minds than me have deemed it > necessary to initialise request_list itself. > > >> duplicated initialisation also happens in > >> intel_render_ring_init_dri(). If someone thinks these can all be > >> simplified and wants to only do the initialisation once in one place then > that should be another patch. > > Agree that the other duplicate inits should also be fixed up in another patch > but that's no reason to add another dupe. > > > > Thomas. > > Only if it is definitely a redundant duplication. Currently, it is not obvious that > it is therefore I am being safe/sensible by following the existing convention. Git blame suggests that init_ring_lists is a vestigial init from before intel_ringbuffers existed. Don’t you agree that either init_ring_buffer or logical_ring_init must be called before any requests can be put into the request_list? Thomas. > > >>>> } > >>>> > >>>> void i915_init_vm(struct drm_i915_private *dev_priv, diff --git > >>>> a/drivers/gpu/drm/i915/intel_lrc.c > >>>> b/drivers/gpu/drm/i915/intel_lrc.c > >>>> index eba0acd..db8efaa 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_lrc.c > >>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c > >>>> @@ -1287,7 +1287,9 @@ static int logical_ring_init(struct > >>>> drm_device *dev, struct intel_engine_cs *rin > >>>> ring->dev = dev; > >>>> INIT_LIST_HEAD(&ring->active_list); > >>>> INIT_LIST_HEAD(&ring->request_list); > >>>> + spin_lock_init(&ring->reqlist_lock); > >>>> init_waitqueue_head(&ring->irq_queue); > >>>> + INIT_LIST_HEAD(&ring->delayed_free_list); > >>>> > >>>> INIT_LIST_HEAD(&ring->execlist_queue); > >>>> spin_lock_init(&ring->execlist_lock); > >>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > >>>> b/drivers/gpu/drm/i915/intel_ringbuffer.c > >>>> index 74c48ed..4338132 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > >>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > >>>> @@ -1808,6 +1808,8 @@ static int intel_init_ring_buffer(struct > >>>> drm_device *dev, > >>>> ring->dev = dev; > >>>> INIT_LIST_HEAD(&ring->active_list); > >>>> INIT_LIST_HEAD(&ring->request_list); > >>>> + spin_lock_init(&ring->reqlist_lock); > >>>> + INIT_LIST_HEAD(&ring->delayed_free_list); > >>>> INIT_LIST_HEAD(&ring->execlist_queue); > >>>> ringbuf->size = 32 * PAGE_SIZE; > >>>> ringbuf->ring = ring; > >>>> @@ -2510,6 +2512,7 @@ int intel_render_ring_init_dri(struct > >>>> drm_device *dev, u64 start, u32 size) > >>>> ring->dev = dev; > >>>> INIT_LIST_HEAD(&ring->active_list); > >>>> INIT_LIST_HEAD(&ring->request_list); > >>>> + INIT_LIST_HEAD(&ring->delayed_free_list); > >>>> > >>>> ringbuf->size = size; > >>>> ringbuf->effective_size = ringbuf->size; diff --git > >>>> a/drivers/gpu/drm/i915/intel_ringbuffer.h > >>>> b/drivers/gpu/drm/i915/intel_ringbuffer.h > >>>> index 824f5884..3af7b7c 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > >>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > >>>> @@ -262,6 +262,8 @@ struct intel_engine_cs { > >>>> * outstanding. > >>>> */ > >>>> struct list_head request_list; > >>>> + spinlock_t reqlist_lock; > >>>> + struct list_head delayed_free_list; > >>>> > >>>> /** > >>>> * Do we have some not yet emitted requests outstanding? > >>>> -- > >>>> 1.7.9.5 > >>>> > >>>> _______________________________________________ > >>>> Intel-gfx mailing list > >>>> Intel-gfx@lists.freedesktop.org > >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 19/11/14 17:05, John Harrison wrote: > On 19/11/2014 13:28, Daniel, Thomas wrote: >>> -----Original Message----- >>> From: Harrison, John C >>> Sent: Wednesday, November 19, 2014 12:26 PM >>> To: Daniel, Thomas; Intel-GFX@Lists.FreeDesktop.Org >>> Subject: Re: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly >>> request >>> deference facility >>> >>> On 18/11/2014 09:34, Daniel, Thomas wrote: >>>>> -----Original Message----- >>>>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On >>>>> Behalf Of John.C.Harrison@Intel.com >>>>> Sent: Friday, November 14, 2014 12:19 PM >>>>> To: Intel-GFX@Lists.FreeDesktop.Org >>>>> Subject: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly >>>>> request deference facility >>>>> >>>>> From: John Harrison <John.C.Harrison@Intel.com> >>>>> >>>>> The next patches in the series convert some display related seqno >>>>> usage to request structure usage. However, the request dereference >>>>> introduced must be done from interrupt context. As the dereference >>>>> potentially involves freeing the request structure and thus calling >>>>> lots of non-interrupt friendly code, this poses a problem. >>>>> >>>>> The solution is to add an IRQ friendly version of the dereference >>>>> function. All this does is to add the request structure to a >>>>> 'delayed free' >>> list and return. >>>>> The retire code, which is run periodically, then processes this list >>>>> and does the actual dereferencing of the request structures. >>>>> >>>>> v2: Added a count to allow for multiple IRQ dereferences of the same >>>>> request at a time. >>>>> >>>>> For: VIZ-4377 >>>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/i915_drv.h | 7 +++++++ >>>>> drivers/gpu/drm/i915/i915_gem.c | 33 >>>>> +++++++++++++++++++++++++++++++ >>>>> drivers/gpu/drm/i915/intel_lrc.c | 2 ++ >>>>> drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +++ >>>>> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++ >>>>> 5 files changed, 47 insertions(+) >>>>> [snip] >>>>> >>>>> @@ -5055,6 +5087,7 @@ init_ring_lists(struct intel_engine_cs *ring) { >>>>> INIT_LIST_HEAD(&ring->active_list); >>>>> INIT_LIST_HEAD(&ring->request_list); >>>>> + INIT_LIST_HEAD(&ring->delayed_free_list); >>>> Same comment as before - multiple init points for this list. >>>> >>>> Thomas. >>> Same reply as before: the function already exists and is already >>> initialising >> You forgot to reply to the mailing list last time. >> >>> other lists therefore it seems sensible to initialise the new list as >>> well. >>> Whether the function as a whole is redundant or not is unclear. The same >> You've introduced a new list - surely it's your responsibility to know >> where it should be initialised? > My list is an extension of the existing 'request_list'. Therefore is > seems prudent to initialise it wherever better minds than me have deemed > it necessary to initialise request_list itself. > >>> duplicated initialisation also happens in >>> intel_render_ring_init_dri(). If >>> someone thinks these can all be simplified and wants to only do the >>> initialisation once in one place then that should be another patch. >> Agree that the other duplicate inits should also be fixed up in >> another patch but that's no reason to add another dupe. >> >> Thomas. > > Only if it is definitely a redundant duplication. Currently, it is not > obvious that it is therefore I am being safe/sensible by following the > existing convention. One initialisation of (ring->request_list) is in init_ring_lists() which does nothing other than what it's name implies. It's called only from i915_gem_load(), in turn called (early) from i915_driver_load(). The same list is also (re)initialised in i915_gem_init_rings() in legacy mode or logical_ring_init() in LRC mode; these are called (indirectly) from i915_gem_init_hw(). But this is called not only from i915_gem_init() (which is called via i915_load_modeset_init() later in i915_driver_load()), but also from i915_reset(), i915_drm_resume(), and i915_gem_entervt_ioctl! It is therefore entirely likely that it is necessary to REinitialise these lists in various situations after driver loading is complete; but conversely quite possible that they have to be set up very early, before the modeset call -- and in any case there's the possibility that the modeset may not be executed, on sufficiently ancient h/w :( It should be possible to test whether the early initialisation is necessary by poisoning the list {head,tail} values so that it triggers an OOPS if they're dereferenced before they're reinitialised. There's one more initialisation in intel_render_ring_init_dri(), but this code is not reachable on post-gen5 h/w and therefore by definition not when using execlists. .Dave.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 180b674..87cb355 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2019,6 +2019,10 @@ struct drm_i915_gem_request { struct drm_i915_file_private *file_priv; /** file_priv list entry for this request */ struct list_head client_list; + + /** deferred free list for dereferencing from IRQ context */ + struct list_head delay_free_list; + uint32_t delay_free_count; }; void i915_gem_request_free(struct kref *req_ref); @@ -2044,9 +2048,12 @@ i915_gem_request_reference(struct drm_i915_gem_request *req) static inline void i915_gem_request_unreference(struct drm_i915_gem_request *req) { + WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex)); kref_put(&req->ref, i915_gem_request_free); } +void i915_gem_request_unreference_irq(struct drm_i915_gem_request *req); + static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst, struct drm_i915_gem_request *src) { diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 60e5eec..8453bbd 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2722,6 +2722,19 @@ void i915_gem_reset(struct drm_device *dev) i915_gem_restore_fences(dev); } +void i915_gem_request_unreference_irq(struct drm_i915_gem_request *req) +{ + struct intel_engine_cs *ring = req->ring; + unsigned long flags; + + /* Need to add the req to a deferred dereference list to be processed + * outside of interrupt time */ + spin_lock_irqsave(&ring->reqlist_lock, flags); + if (req->delay_free_count++ == 0) + list_add_tail(&req->delay_free_list, &ring->delayed_free_list); + spin_unlock_irqrestore(&ring->reqlist_lock, flags); +} + /** * This function clears the request list as sequence numbers are passed. */ @@ -2796,6 +2809,25 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring) ring->trace_irq_seqno = 0; } + while (!list_empty(&ring->delayed_free_list)) { + struct drm_i915_gem_request *request; + unsigned long flags; + uint32_t count; + + request = list_first_entry(&ring->delayed_free_list, + struct drm_i915_gem_request, + delay_free_list); + + spin_lock_irqsave(&request->ring->reqlist_lock, flags); + list_del(&request->delay_free_list); + count = request->delay_free_count; + request->delay_free_count = 0; + spin_unlock_irqrestore(&request->ring->reqlist_lock, flags); + + while (count-- > 0) + i915_gem_request_unreference(request); + } + WARN_ON(i915_verify_lists(ring->dev)); } @@ -5055,6 +5087,7 @@ init_ring_lists(struct intel_engine_cs *ring) { INIT_LIST_HEAD(&ring->active_list); INIT_LIST_HEAD(&ring->request_list); + INIT_LIST_HEAD(&ring->delayed_free_list); } void i915_init_vm(struct drm_i915_private *dev_priv, diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index eba0acd..db8efaa 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1287,7 +1287,9 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin ring->dev = dev; INIT_LIST_HEAD(&ring->active_list); INIT_LIST_HEAD(&ring->request_list); + spin_lock_init(&ring->reqlist_lock); init_waitqueue_head(&ring->irq_queue); + INIT_LIST_HEAD(&ring->delayed_free_list); INIT_LIST_HEAD(&ring->execlist_queue); spin_lock_init(&ring->execlist_lock); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 74c48ed..4338132 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1808,6 +1808,8 @@ static int intel_init_ring_buffer(struct drm_device *dev, ring->dev = dev; INIT_LIST_HEAD(&ring->active_list); INIT_LIST_HEAD(&ring->request_list); + spin_lock_init(&ring->reqlist_lock); + INIT_LIST_HEAD(&ring->delayed_free_list); INIT_LIST_HEAD(&ring->execlist_queue); ringbuf->size = 32 * PAGE_SIZE; ringbuf->ring = ring; @@ -2510,6 +2512,7 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size) ring->dev = dev; INIT_LIST_HEAD(&ring->active_list); INIT_LIST_HEAD(&ring->request_list); + INIT_LIST_HEAD(&ring->delayed_free_list); ringbuf->size = size; ringbuf->effective_size = ringbuf->size; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 824f5884..3af7b7c 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -262,6 +262,8 @@ struct intel_engine_cs { * outstanding. */ struct list_head request_list; + spinlock_t reqlist_lock; + struct list_head delayed_free_list; /** * Do we have some not yet emitted requests outstanding?