[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 18:05:23 CET 2014
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 at 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 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
> 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 at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list