| Message ID | 1416854990-1920-12-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 24, 2014 at 06:49:33PM +0000, John.C.Harrison@Intel.com wrote: > 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> > Reviewed-by: Thomas Daniel <Thomas.Daniel@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 | 2 ++ > drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++ > 5 files changed, 46 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 149532e6..e9b01e1 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2025,6 +2025,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); > @@ -2050,9 +2054,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 8e2c530..b62b9d9 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2732,6 +2732,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. > */ > @@ -2806,6 +2819,25 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring) > ring->trace_irq_seqno = 0; > } > > + while (!list_empty(&ring->delayed_free_list)) { This needs locking. It might work as-is but it's definitely too tricky to be worth it. -Daniel > + 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)); > } > > @@ -5007,6 +5039,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 6177179..09f4588 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1381,7 +1381,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); > INIT_LIST_HEAD(&ring->execlist_retired_req_list); > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index b9d0d29..e3082af 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -1807,6 +1807,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; > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 2a84bd9..cd1a9f9 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -263,6 +263,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 26/11/2014 09:19, Daniel Vetter wrote: > On Mon, Nov 24, 2014 at 06:49:33PM +0000, John.C.Harrison@Intel.com wrote: >> 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> >> Reviewed-by: Thomas Daniel <Thomas.Daniel@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 | 2 ++ >> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++ >> 5 files changed, 46 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 149532e6..e9b01e1 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2025,6 +2025,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); >> @@ -2050,9 +2054,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 8e2c530..b62b9d9 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -2732,6 +2732,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. >> */ >> @@ -2806,6 +2819,25 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring) >> ring->trace_irq_seqno = 0; >> } >> >> + while (!list_empty(&ring->delayed_free_list)) { > This needs locking. It might work as-is but it's definitely too tricky to > be worth it. > -Daniel The manipulation of the list is locked, only this test is unlocked. Do you really need to lock when testing for empty? Nothing can be removing items from the list although something could be asynchronously adding them. If the 'head->next == head' test goes wrong because head or next is being written to, it can't deference a dodgy pointer. It can only return an incorrect 'empty' in which case the clean up is delayed until later. An incorrect 'not-empty' is not possible. >> + 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)); >> } >> >> @@ -5007,6 +5039,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 6177179..09f4588 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -1381,7 +1381,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); >> INIT_LIST_HEAD(&ring->execlist_retired_req_list); >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c >> index b9d0d29..e3082af 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >> @@ -1807,6 +1807,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; >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h >> index 2a84bd9..cd1a9f9 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h >> @@ -263,6 +263,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 Wed, Nov 26, 2014 at 1:23 PM, John Harrison <John.C.Harrison@intel.com> wrote: >> This needs locking. It might work as-is but it's definitely too tricky to >> be worth it. > > The manipulation of the list is locked, only this test is unlocked. Do you > really need to lock when testing for empty? Nothing can be removing items > from the list although something could be asynchronously adding them. If the > 'head->next == head' test goes wrong because head or next is being written > to, it can't deference a dodgy pointer. It can only return an incorrect > 'empty' in which case the clean up is delayed until later. An incorrect > 'not-empty' is not possible. The problem is not that it's broken but too tricky. And nothing in either the commit message nor a code commit explains and justifies this. Of course every time I stumble over this I can think hard for a while and convince myself that it works, but GEM is already littered with such inherent complexity, so adding accidental complexity for locking tricks when they're not needed is a bad move. Since we don't actually need this in current code I'll drop it. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 149532e6..e9b01e1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2025,6 +2025,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); @@ -2050,9 +2054,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 8e2c530..b62b9d9 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2732,6 +2732,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. */ @@ -2806,6 +2819,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)); } @@ -5007,6 +5039,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 6177179..09f4588 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1381,7 +1381,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); INIT_LIST_HEAD(&ring->execlist_retired_req_list); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index b9d0d29..e3082af 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1807,6 +1807,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; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 2a84bd9..cd1a9f9 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -263,6 +263,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?