On Sat, 09 Dec 2017 14:17:58 +0000, Stefan Schake wrote:

On Sat, Dec 9, 2017 at 1:34 PM, Marc Zyngier marc.zyngier@arm.com wrote:

...

On Fri, 08 Dec 2017 22:44:27 +0000, Stefan Wahren wrote:

...

Hi,

the commit 253696ccd613 ("drm/vc4: Account for interrupts in flight") triggers a warning during boot of Raspberry Pi 2 with multi_v7_defconfig:

[ 7.962699] vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <-> 3f902000.hdmi mapping ok [ 7.962732] vc4_hdmi 3f902000.hdmi: ASoC: no DMI vendor name! [ 7.973355] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4]) [ 7.973651] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4]) [ 7.973788] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4]) [ 7.974157] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4]) [ 7.974464] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4]) [ 7.974746] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4]) [ 8.018820] ------------[ cut here ]------------ [ 8.018861] WARNING: CPU: 2 PID: 172 at kernel/irq/chip.c:244 __irq_startup+0x9c/0xa8 [ 8.018868] Modules linked in: vc4(+) snd_soc_core ac97_bus snd_pcm_dmaengine snd_pcm snd_timer snd soundcore cec [ 8.018911] CPU: 2 PID: 172 Comm: systemd-udevd Not tainted 4.15.0-rc1-00291-g75f64f6 #10 [ 8.018914] Hardware name: BCM2835 [ 8.018950] [<c03115a4>] (unwind_backtrace) from [<c030c6b4>] (show_stack+0x10/0x14) [ 8.018970] [<c030c6b4>] (show_stack) from [<c0cfc3b0>] (dump_stack+0x88/0xa4) [ 8.018993] [<c0cfc3b0>] (dump_stack) from [<c03430cc>] (__warn+0xe4/0x110) [ 8.019012] [<c03430cc>] (__warn) from [<c03431c4>] (warn_slowpath_null+0x3c/0x48) [ 8.019029] [<c03431c4>] (warn_slowpath_null) from [<c0391370>] (__irq_startup+0x9c/0xa8) [ 8.019045] [<c0391370>] (__irq_startup) from [<c03913c0>] (irq_startup+0x44/0x134) [ 8.019061] [<c03913c0>] (irq_startup) from [<c038f360>] (enable_irq+0x34/0x6c) [ 8.019210] [<c038f360>] (enable_irq) from [<bf0a8380>] (vc4_irq_postinstall+0x14/0x34 [vc4]) [ 8.019338] [<bf0a8380>] (vc4_irq_postinstall [vc4]) from [<c0816a98>] (drm_irq_install+0xc0/0x134) [ 8.019456] [<c0816a98>] (drm_irq_install) from [<bf0ab0c8>] (vc4_v3d_bind+0x12c/0x238 [vc4])

The sequence "request_irq/enable" feels pretty odd, given that the interrupt wasn't requested with NOAUTOEN. What are the expected semantics of that patch?

Thanks,

    M.

-- Jazz is not dead, it just smell funny.

There is a second path to vc4_irq_postinstall when we return from power management disable. Maybe the enable would be better situated there?

I've no idea. But enabling an already enabled interrupts feels a bit wrong, to say the least

That said, some more study of the irq code is giving me more questions than answers. If disable/enable are depth-counted, why isn't it also warning about that (there is a check)? Depth must be 1 after request_irq. Similarly, the irq is activated in request_irq, but the warning reported here is complaining that it is not.

I'm afraid you'll have to trace why the irq is not flagged as activated. From what I can see, the irqchip driver of this HW doesn't even implement an activate callback, so the interrupt should be trivially flagged as activated. If it is not, it means something rather bad has happened. Do you see this splat on each enable_irq()?

M.

On Sun, 10 Dec 2017 03:05:07 +0000, Stefan Schake wrote:

On Fri, Dec 8, 2017 at 11:44 PM, Stefan Wahren stefan.wahren@i2se.com wrote:

...

Hi,

the commit 253696ccd613 ("drm/vc4: Account for interrupts in flight") triggers a warning during boot of Raspberry Pi 2 with multi_v7_defconfig:

[ 7.962699] vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <-> 3f902000.hdmi mapping ok [ 7.962732] vc4_hdmi 3f902000.hdmi: ASoC: no DMI vendor name! [ 7.973355] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4]) [ 7.973651] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4]) [ 7.973788] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4]) [ 7.974157] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4]) [ 7.974464] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4]) [ 7.974746] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4]) [ 8.018820] ------------[ cut here ]------------ [ 8.018861] WARNING: CPU: 2 PID: 172 at kernel/irq/chip.c:244 __irq_startup+0x9c/0xa8 [ 8.018868] Modules linked in: vc4(+) snd_soc_core ac97_bus snd_pcm_dmaengine snd_pcm snd_timer snd soundcore cec [ 8.018911] CPU: 2 PID: 172 Comm: systemd-udevd Not tainted 4.15.0-rc1-00291-g75f64f6 #10 [ 8.018914] Hardware name: BCM2835 [ 8.018950] [<c03115a4>] (unwind_backtrace) from [<c030c6b4>] (show_stack+0x10/0x14) [ 8.018970] [<c030c6b4>] (show_stack) from [<c0cfc3b0>] (dump_stack+0x88/0xa4) [ 8.018993] [<c0cfc3b0>] (dump_stack) from [<c03430cc>] (__warn+0xe4/0x110) [ 8.019012] [<c03430cc>] (__warn) from [<c03431c4>] (warn_slowpath_null+0x3c/0x48) [ 8.019029] [<c03431c4>] (warn_slowpath_null) from [<c0391370>] (__irq_startup+0x9c/0xa8) [ 8.019045] [<c0391370>] (__irq_startup) from [<c03913c0>] (irq_startup+0x44/0x134) [ 8.019061] [<c03913c0>] (irq_startup) from [<c038f360>] (enable_irq+0x34/0x6c) [ 8.019210] [<c038f360>] (enable_irq) from [<bf0a8380>] (vc4_irq_postinstall+0x14/0x34 [vc4]) [ 8.019338] [<bf0a8380>] (vc4_irq_postinstall [vc4]) from [<c0816a98>] (drm_irq_install+0xc0/0x134) [ 8.019456] [<c0816a98>] (drm_irq_install) from [<bf0ab0c8>] (vc4_v3d_bind+0x12c/0x238 [vc4]) [ 8.019550] [<bf0ab0c8>] (vc4_v3d_bind [vc4]) from [<c089f760>] (component_bind_all+0xe8/0x21c) [ 8.019635] [<c089f760>] (component_bind_all) from [<bf09efbc>] (vc4_drm_bind+0x78/0x130 [vc4]) [ 8.019721] [<bf09efbc>] (vc4_drm_bind [vc4]) from [<c089fbac>] (try_to_bring_up_master+0x13c/0x184) [ 8.019739] [<c089fbac>] (try_to_bring_up_master) from [<c089fdb0>] (component_master_add_with_match+0x80/0xb8) [ 8.019822] [<c089fdb0>] (component_master_add_with_match) from [<bf09f104>] (vc4_platform_drm_probe+0x90/0xa8 [vc4]) [ 8.019905] [<bf09f104>] (vc4_platform_drm_probe [vc4]) from [<c08a68f8>] (platform_drv_probe+0x4c/0xac) [ 8.019924] [<c08a68f8>] (platform_drv_probe) from [<c08a4ccc>] (driver_probe_device+0x234/0x338) [ 8.019939] [<c08a4ccc>] (driver_probe_device) from [<c08a4e7c>] (__driver_attach+0xac/0xb0) [ 8.019953] [<c08a4e7c>] (__driver_attach) from [<c08a317c>] (bus_for_each_dev+0x64/0x94) [ 8.019967] [<c08a317c>] (bus_for_each_dev) from [<c08a4250>] (bus_add_driver+0x184/0x20c) [ 8.019981] [<c08a4250>] (bus_add_driver) from [<c08a5920>] (driver_register+0x78/0xf8) [ 8.019997] [<c08a5920>] (driver_register) from [<c03021cc>] (do_one_initcall+0x3c/0x16c) [ 8.020018] [<c03021cc>] (do_one_initcall) from [<c03c293c>] (do_init_module+0x5c/0x1f0) [ 8.020037] [<c03c293c>] (do_init_module) from [<c03c1b00>] (load_module+0x1ba4/0x2248) [ 8.020058] [<c03c1b00>] (load_module) from [<c03c2370>] (SyS_finit_module+0x8c/0x9c) [ 8.020076] [<c03c2370>] (SyS_finit_module) from [<c0307edc>] (__sys_trace_return+0x0/0x10) [ 8.020085] ---[ end trace d5c350f831cb07d2 ]--- [ 8.020201] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4])

So I've done some testing on an RPi 3b that is also affected. It turns out that dev->irq is only initialized *after* postinstall by drm_irq_install, so we were calling irq_enable on IRQ 0 and that just happens to be not activated, hence the warning.

And IRQ 0 is definitely not a valid IRQ (I'd expect the DRM subsystem to should in that case...).

Having learned some more about the IRQ subsystem, my plan would be first to simply replace the disable/enable dance with one synchronize_irq. We do reenable the interrupt in the HW registers at the end of the binner work callback so this may not suffice. In that case, we could simply move the enable_irq to the power management handler that is calling postinstall.

I don't know anything about VC4 (or DRM), so I can't be of any advise here. But if you intend to perform some enable/disable on the IRQ, I wonder if you're actually ready to handle stuff by the time you perform the request_irq. You may want to consider controlling the interrupt at the device level instead.

Thanks,

M.