Re: [PATCH v2 11/28] drm/i915: Add IRQ friendly request deference facility

[Dave Gordon <david.s.gordon@intel.com>]

On 19/11/14 17:05, John Harrison wrote:
> 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@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@lists.freedesktop.org] On
>>>>> Behalf Of John.C.Harrison@Intel.com
>>>>> Sent: Friday, November 14, 2014 12:19 PM
>>>>> To: Intel-GFX@Lists.FreeDesktop.Org
>>>>> Subject: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly
>>>>> request deference facility
>>>>>
>>>>> 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>
>>>>> ---
>>>>>    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(+)
>>>>>
[snip]
>>>>>
>>>>> @@ -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.

One initialisation of (ring->request_list) is in init_ring_lists()
which does nothing other than what it's name implies. It's called
only from i915_gem_load(), in turn called (early) from i915_driver_load().

The same list is also (re)initialised in i915_gem_init_rings() in legacy
mode or logical_ring_init() in LRC mode; these are called (indirectly)
from i915_gem_init_hw(). But this is called not only from
i915_gem_init() (which is called via i915_load_modeset_init() later in
i915_driver_load()), but also from i915_reset(), i915_drm_resume(),
and i915_gem_entervt_ioctl!

It is therefore entirely likely that it is necessary to REinitialise
these lists in various situations after driver loading is complete; but
conversely quite possible that they have to be set up very early, before
the modeset call -- and in any case there's the possibility that the
modeset may not be executed, on sufficiently ancient h/w :(

It should be possible to test whether the early initialisation is
necessary by poisoning the list {head,tail} values so that it triggers
an OOPS if they're dereferenced before they're reinitialised.

There's one more initialisation in intel_render_ring_init_dri(), but
this code is not reachable on post-gen5 h/w and therefore by definition
not when using execlists.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx