[Intel-gfx] [PATCH] drm/i915/gt: Acquire a GT wakeref for the breadcrumb interrupt

Fri Dec 6 11:18:50 UTC 2019

On 06/12/2019 11:09, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-12-06 11:02:15)
>>
>> On 05/12/2019 21:58, Chris Wilson wrote:
>>> Take a wakeref on the intel_gt specifically for the enabled breadcrumb
>>> interrupt so that we can safely process the mmio. If the intel_gt is
>>> already asleep by the time we try and setup the breadcrumb interrupt, by
>>> a process of elimination we know the request must have been completed
>>> and we can skip its enablement!
>>>
>>> <4> [1518.350005] Unclaimed write to register 0x220a8
>>> <4> [1518.350323] WARNING: CPU: 2 PID: 3685 at drivers/gpu/drm/i915/intel_uncore.c:1163 __unclaimed_reg_debug+0x40/0x50 [i915]
>>> <4> [1518.350393] Modules linked in: vgem snd_hda_codec_hdmi x86_pkg_temp_thermal i915 coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_hda_intel snd_intel_dspcfg snd_hda_codec snd_hwdep snd_hda_core btusb cdc_ether btrtl usbnet btbcm btintel r8152 snd_pcm mii bluetooth ecdh_generic ecc i2c_hid pinctrl_sunrisepoint pinctrl_intel intel_lpss_pci prime_numbers [last unloaded: vgem]
>>> <4> [1518.350646] CPU: 2 PID: 3685 Comm: gem_exec_parse_ Tainted: G     U            5.4.0-rc8-CI-CI_DRM_7490+ #1
>>> <4> [1518.350708] Hardware name: Google Caroline/Caroline, BIOS MrChromebox 08/27/2018
>>> <4> [1518.350946] RIP: 0010:__unclaimed_reg_debug+0x40/0x50 [i915]
>>> <4> [1518.350992] Code: 74 05 5b 5d 41 5c c3 45 84 e4 48 c7 c0 95 8d 47 a0 48 c7 c6 8b 8d 47 a0 48 0f 44 f0 89 ea 48 c7 c7 9e 8d 47 a0 e8 40 45 e3 e0 <0f> 0b 83 2d 27 4f 2a 00 01 5b 5d 41 5c c3 66 90 41 55 41 54 55 53
>>> <4> [1518.351100] RSP: 0018:ffffc900007f39c8 EFLAGS: 00010086
>>> <4> [1518.351140] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000006
>>> <4> [1518.351202] RDX: 0000000080000006 RSI: 0000000000000000 RDI: 00000000ffffffff
>>> <4> [1518.351249] RBP: 00000000000220a8 R08: 0000000000000000 R09: 0000000000000000
>>> <4> [1518.351296] R10: ffffc900007f3990 R11: ffffc900007f3868 R12: 0000000000000000
>>> <4> [1518.351342] R13: 00000000fefeffff R14: 0000000000000092 R15: ffff888155fea000
>>> <4> [1518.351391] FS:  00007fc255abfe40(0000) GS:ffff88817ab00000(0000) knlGS:0000000000000000
>>> <4> [1518.351445] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> <4> [1518.351485] CR2: 00007fc2554882d0 CR3: 0000000168ca2005 CR4: 00000000003606e0
>>> <4> [1518.351529] Call Trace:
>>> <4> [1518.351746]  fwtable_write32+0x114/0x1d0 [i915]
>>> <4> [1518.351795]  ? sync_file_alloc+0x80/0x80
>>> <4> [1518.352039]  gen8_logical_ring_enable_irq+0x30/0x50 [i915]
>>> <4> [1518.352295]  irq_enable.part.10+0x23/0x40 [i915]
>>> <4> [1518.352523]  i915_request_enable_breadcrumb+0xb5/0x330 [i915]
>>> <4> [1518.352575]  ? sync_file_alloc+0x80/0x80
>>> <4> [1518.352612]  __dma_fence_enable_signaling+0x60/0x160
>>> <4> [1518.352653]  ? sync_file_alloc+0x80/0x80
>>> <4> [1518.352685]  dma_fence_add_callback+0x44/0xd0
>>> <4> [1518.352726]  sync_file_poll+0x95/0xc0
>>> <4> [1518.352767]  do_sys_poll+0x24d/0x570
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 22 +++++++++++++++------
>>>    1 file changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
>>> index 55317081d48b..8a9facf4f3b6 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
>>> @@ -28,6 +28,7 @@
>>>    
>>>    #include "i915_drv.h"
>>>    #include "i915_trace.h"
>>> +#include "intel_gt_pm.h"
>>>    
>>>    static void irq_enable(struct intel_engine_cs *engine)
>>>    {
>>> @@ -53,15 +54,17 @@ static void irq_disable(struct intel_engine_cs *engine)
>>>    
>>>    static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
>>>    {
>>> +     struct intel_engine_cs *engine =
>>> +             container_of(b, struct intel_engine_cs, breadcrumbs);
>>> +
>>>        lockdep_assert_held(&b->irq_lock);
>>>    
>>>        GEM_BUG_ON(!b->irq_enabled);
>>>        if (!--b->irq_enabled)
>>> -             irq_disable(container_of(b,
>>> -                                      struct intel_engine_cs,
>>> -                                      breadcrumbs));
>>> +             irq_disable(engine);
>>>    
>>>        b->irq_armed = false;
>>> +     intel_gt_pm_put_async(engine->gt);
>>>    }
>>>    
>>>    void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>>> @@ -207,14 +210,17 @@ static void signal_irq_work(struct irq_work *work)
>>>        intel_engine_breadcrumbs_irq(engine);
>>>    }
>>>    
>>> -static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
>>> +static bool __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
>>>    {
>>>        struct intel_engine_cs *engine =
>>>                container_of(b, struct intel_engine_cs, breadcrumbs);
>>>    
>>>        lockdep_assert_held(&b->irq_lock);
>>>        if (b->irq_armed)
>>> -             return;
>>> +             return true;
>>> +
>>> +     if (!intel_gt_pm_get_if_awake(engine->gt))
>>> +             return false;
>>>    
>>>        /*
>>>         * The breadcrumb irq will be disarmed on the interrupt after the
>>> @@ -234,6 +240,8 @@ static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
>>>    
>>>        if (!b->irq_enabled++)
>>>                irq_enable(engine);
>>> +
>>> +     return true;
>>>    }
>>>    
>>>    void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
>>> @@ -277,7 +285,8 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
>>>                spin_lock(&b->irq_lock);
>>>                GEM_BUG_ON(test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags));
>>>    
>>> -             __intel_breadcrumbs_arm_irq(b);
>>> +             if (!__intel_breadcrumbs_arm_irq(b))
>>> +                     goto unlock;
>>>    
>>>                /*
>>>                 * We keep the seqno in retirement order, so we can break
>>> @@ -306,6 +315,7 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
>>>                GEM_BUG_ON(!check_signal_order(ce, rq));
>>>    
>>>                set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
>>> +unlock:
>>>                spin_unlock(&b->irq_lock);
>>>        }
>>>    
>>>
>>
>> Any special reason not to do PM management on the engine rather than GT?
> 
> The nice result of using GT here is that as soon as we park the engine,
> we'll drop the irq wakeref. Whereas if we kept it on the engine, we
> would have to introduce a new mechanism to drop the irq+wakeref when the
> last context is removed, i.e. cancel_breadcrumb and signal_irq_work
> (currently the mechanism is only on the interrupt after the last, which
> seems worth keeping, but we need to cleanup on parking then.)

Didn't really follow to be honest. I was thinking along the lines how 
engine pm get is just gt pm get plus more. But granted we don't need 
that "more" for this patch at all since it's not going to use engines. 
So after all I think it's even more correct to to it on GT.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko