[Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly request deference facility
John Harrison
John.C.Harrison at Intel.com
Wed Nov 19 13:25:36 CET 2014
On 18/11/2014 09:34, Daniel, Thomas wrote:
>> -----Original Message-----
>> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf
>> Of John.C.Harrison at Intel.com
>> Sent: Friday, November 14, 2014 12:19 PM
>> To: Intel-GFX at Lists.FreeDesktop.Org
>> Subject: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly request
>> deference facility
>>
>> From: John Harrison <John.C.Harrison at 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 at 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list