> -----Original Message----- > From: Harrison, John C > Sent: Wednesday, November 19, 2014 5:05 PM > To: Daniel, Thomas; Intel-GFX@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [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@xxxxxxxxxxxxxxxxxxxxx > >> Subject: Re: [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@xxxxxxxxxxxxxxxxxxxxx] On > >>>> Behalf Of John.C.Harrison@xxxxxxxxx > >>>> Sent: Friday, November 14, 2014 12:19 PM > >>>> To: Intel-GFX@xxxxxxxxxxxxxxxxxxxxx > >>>> Subject: [PATCH v2 11/28] drm/i915: Add IRQ friendly > >>>> request deference facility > >>>> > >>>> From: John Harrison <John.C.Harrison@xxxxxxxxx> > >>>> > >>>> @@ -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@xxxxxxxxxxxxxxxxxxxxx > >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
