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@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> >>>>> >>>>> 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@xxxxxxxxx> >>>>> --- >>>>> 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
