[Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly request deference facility
Daniel, Thomas
thomas.daniel at intel.com
Wed Nov 19 18:28:54 CET 2014
> -----Original Message-----
> From: Harrison, John C
> Sent: Wednesday, November 19, 2014 5:05 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 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>
> >>>>
> >>>> @@ -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 at lists.freedesktop.org
> >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list