Re: [Intel-gfx] [PATCH 2/4] drm/i915: Fix POWER_DOMAIN_AUDIO refcounting.

[Daniel Vetter]

On Mon, Jan 16, 2017 at 12:00:46PM +0100, Maarten Lankhorst wrote:
> Op 11-01-17 om 17:13 schreef Daniel Vetter:
> > On Thu, Dec 15, 2016 at 03:29:43PM +0100, Maarten Lankhorst wrote:
> >> If the crtc was brought up with audio before the driver loads,
> >> then crtc_disable will remove a refcount to audio that doesn't exist
> >> before.
> >>
> >> Fortunately we already set power domains on readout, so we can just add
> >> the power domain handling to get_crtc_power_domains, which will update
> >> the power domains correctly in all cases.
> >>
> >> This was found when testing module reload on CI with the crtc enabled,
> >> which resulted in the following warn after module reload + modeset:
> >>
> >> [   24.197041] [ cut here ]
> >> [   24.197075] WARNING: CPU: 0 PID: 99 at 
> >> drivers/gpu/drm/i915/intel_runtime_pm.c:1790 
> >> intel_display_power_put+0x134/0x140 [i915]
> >> [   24.197076] Use count on domain AUDIO is already zero
> >> [   24.197098] CPU: 0 PID: 99 Comm: kworker/u8:2 Not tainted 
> >> 4.9.0-CI-Trybot_393+ #1
> >> [   24.197099] Hardware name:  /NUC6i5SYB, BIOS 
> >> SYSKLi35.86A.0042.2016.0409.1246 04/09/2016
> >> [   24.197102] Workqueue: events_unbound async_run_entry_fn
> >> [   24.197105]  c93c7688 81435b35 c93c76d8 
> >> 
> >> [   24.197107]  c93c76c8 8107e4d6 06fe5dc36f28 
> >> 88025dc30054
> >> [   24.197109]  88025dc36f28 88025dc3 88025dc3 
> >> 0015
> >> [   24.197110] Call Trace:
> >> [   24.197113]  [] dump_stack+0x67/0x92
> >> [   24.197116]  [] __warn+0xc6/0xe0
> >> [   24.197118]  [] warn_slowpath_fmt+0x4a/0x50
> >> [   24.197149]  [] intel_display_power_put+0x134/0x140 
> >> [i915]
> >> [   24.197187]  [] intel_disable_ddi+0x4d/0x80 [i915]
> >> [   24.197223]  [] 
> >> intel_encoders_disable.isra.74+0x7f/0x90 [i915]
> >> [   24.197257]  [] haswell_crtc_disable+0x55/0x170 [i915]
> >> [   24.197292]  [] intel_atomic_commit_tail+0x108/0xfd0 
> >> [i915]
> >> [   24.197295]  [] ? __lock_is_held+0x66/0x90
> >> [   24.197330]  [] intel_atomic_commit+0x429/0x560 [i915]
> >> [   24.197332]  [] 
> >> ?drm_atomic_add_affected_connectors+0x56/0xf0
> >> [   24.197334]  [] drm_atomic_commit+0x46/0x50
> >> [   24.197336]  [] restore_fbdev_mode+0x147/0x270
> >> [   24.197337]  [] 
> >> drm_fb_helper_restore_fbdev_mode_unlocked+0x2e/0x70
> >> [   24.197339]  [] drm_fb_helper_set_par+0x28/0x50
> >> [   24.197374]  [] intel_fbdev_set_par+0x13/0x70 [i915]
> >> [   24.197376]  [] fbcon_init+0x57a/0x600
> >> [   24.197379]  [] visual_init+0xd1/0x130
> >> [   24.197381]  [] do_bind_con_driver+0x1bc/0x3a0
> >> [   24.197384]  [] do_take_over_console+0x111/0x180
> >> [   24.197386]  [] do_fbcon_takeover+0x52/0xb0
> >> [   24.197387]  [] fbcon_event_notify+0x723/0x850
> >> [   24.197390]  [] 
> >> ?__blocking_notifier_call_chain+0x30/0x70
> >> [   24.197392]  [] notifier_call_chain+0x34/0xa0
> >> [   24.197394]  [] 
> >> __blocking_notifier_call_chain+0x48/0x70
> >> [   24.197397]  [] blocking_notifier_call_chain+0x11/0x20
> >> [   24.197398]  [] fb_notifier_call_chain+0x16/0x20
> >> [   24.197400]  [] register_framebuffer+0x24c/0x330
> >> [   24.197402]  [] 
> >> drm_fb_helper_initial_config+0x219/0x3c0
> >> [   24.197436]  [] intel_fbdev_initial_config+0x13/0x30 
> >> [i915]
> >> [   24.197438]  [] async_run_entry_fn+0x34/0x140
> >> [   24.197440]  [] process_one_work+0x1ec/0x6b0
> >> [   24.197442]  [] ? process_one_work+0x166/0x6b0
> >> [   24.197445]  [] worker_thread+0x49/0x490
> >> [   24.197447]  [] ? process_one_work+0x6b0/0x6b0
> >> [   24.197448]  [] kthread+0xeb/0x110
> >> [   24.197451]  [] ? kthread_park+0x60/0x60
> >> [   24.197453]  [] ret_from_fork+0x27/0x40
> >> [   24.197476] ---[ end trace bda64b683b8e8162 ]---
> >>
> >> Signed-off-by: Maarten Lankhorst 
> > Do we still need this with patch 3? I know it'd be nice if we could
> > faithfully restore any state we can also program, but then that's also a
> > lot of complexity ...
> >
> > Otoh patch 3 means we'll stop testing a lot of the fastboot code while
> > reloading the driver. But then that's been the thing in the past, and as
> > long as we still boot up we have at least some test coverage fo the
> > fastboot code (I'm mostly concerned about the plane/buffer readout code,
> > since that's not covered by the state checker).
> >
> > But for now I'd say let's just go with patch 3 only.
> We don't need this patch, but it makes the audio power domain tracked
> like all other power domains. It's a nice cleanup in general and I would
> like it even without the rest of the series. Could otherwise be merged
> through the intel tree through.

Hm, count me convinced.

Reviewed-by: Daniel Vetter 
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list

Re: [Intel-gfx] [PATCH 2/4] drm/i915: Fix POWER_DOMAIN_AUDIO refcounting.

[Maarten Lankhorst]

Op 11-01-17 om 17:13 schreef Daniel Vetter:
> On Thu, Dec 15, 2016 at 03:29:43PM +0100, Maarten Lankhorst wrote:
>> If the crtc was brought up with audio before the driver loads,
>> then crtc_disable will remove a refcount to audio that doesn't exist
>> before.
>>
>> Fortunately we already set power domains on readout, so we can just add
>> the power domain handling to get_crtc_power_domains, which will update
>> the power domains correctly in all cases.
>>
>> This was found when testing module reload on CI with the crtc enabled,
>> which resulted in the following warn after module reload + modeset:
>>
>> [   24.197041] [ cut here ]
>> [   24.197075] WARNING: CPU: 0 PID: 99 at 
>> drivers/gpu/drm/i915/intel_runtime_pm.c:1790 
>> intel_display_power_put+0x134/0x140 [i915]
>> [   24.197076] Use count on domain AUDIO is already zero
>> [   24.197098] CPU: 0 PID: 99 Comm: kworker/u8:2 Not tainted 
>> 4.9.0-CI-Trybot_393+ #1
>> [   24.197099] Hardware name:  /NUC6i5SYB, BIOS 
>> SYSKLi35.86A.0042.2016.0409.1246 04/09/2016
>> [   24.197102] Workqueue: events_unbound async_run_entry_fn
>> [   24.197105]  c93c7688 81435b35 c93c76d8 
>> 
>> [   24.197107]  c93c76c8 8107e4d6 06fe5dc36f28 
>> 88025dc30054
>> [   24.197109]  88025dc36f28 88025dc3 88025dc3 
>> 0015
>> [   24.197110] Call Trace:
>> [   24.197113]  [] dump_stack+0x67/0x92
>> [   24.197116]  [] __warn+0xc6/0xe0
>> [   24.197118]  [] warn_slowpath_fmt+0x4a/0x50
>> [   24.197149]  [] intel_display_power_put+0x134/0x140 
>> [i915]
>> [   24.197187]  [] intel_disable_ddi+0x4d/0x80 [i915]
>> [   24.197223]  [] 
>> intel_encoders_disable.isra.74+0x7f/0x90 [i915]
>> [   24.197257]  [] haswell_crtc_disable+0x55/0x170 [i915]
>> [   24.197292]  [] intel_atomic_commit_tail+0x108/0xfd0 
>> [i915]
>> [   24.197295]  [] ? __lock_is_held+0x66/0x90
>> [   24.197330]  [] intel_atomic_commit+0x429/0x560 [i915]
>> [   24.197332]  [] 
>> ?drm_atomic_add_affected_connectors+0x56/0xf0
>> [   24.197334]  [] drm_atomic_commit+0x46/0x50
>> [   24.197336]  [] restore_fbdev_mode+0x147/0x270
>> [   24.197337]  [] 
>> drm_fb_helper_restore_fbdev_mode_unlocked+0x2e/0x70
>> [   24.197339]  [] drm_fb_helper_set_par+0x28/0x50
>> [   24.197374]  [] intel_fbdev_set_par+0x13/0x70 [i915]
>> [   24.197376]  [] fbcon_init+0x57a/0x600
>> [   24.197379]  [] visual_init+0xd1/0x130
>> [   24.197381]  [] do_bind_con_driver+0x1bc/0x3a0
>> [   24.197384]  [] do_take_over_console+0x111/0x180
>> [   24.197386]  [] do_fbcon_takeover+0x52/0xb0
>> [   24.197387]  [] fbcon_event_notify+0x723/0x850
>> [   24.197390]  [] 
>> ?__blocking_notifier_call_chain+0x30/0x70
>> [   24.197392]  [] notifier_call_chain+0x34/0xa0
>> [   24.197394]  [] __blocking_notifier_call_chain+0x48/0x70
>> [   24.197397]  [] blocking_notifier_call_chain+0x11/0x20
>> [   24.197398]  [] fb_notifier_call_chain+0x16/0x20
>> [   24.197400]  [] register_framebuffer+0x24c/0x330
>> [   24.197402]  [] drm_fb_helper_initial_config+0x219/0x3c0
>> [   24.197436]  [] intel_fbdev_initial_config+0x13/0x30 
>> [i915]
>> [   24.197438]  [] async_run_entry_fn+0x34/0x140
>> [   24.197440]  [] process_one_work+0x1ec/0x6b0
>> [   24.197442]  [] ? process_one_work+0x166/0x6b0
>> [   24.197445]  [] worker_thread+0x49/0x490
>> [   24.197447]  [] ? process_one_work+0x6b0/0x6b0
>> [   24.197448]  [] kthread+0xeb/0x110
>> [   24.197451]  [] ? kthread_park+0x60/0x60
>> [   24.197453]  [] ret_from_fork+0x27/0x40
>> [   24.197476] ---[ end trace bda64b683b8e8162 ]---
>>
>> Signed-off-by: Maarten Lankhorst 
> Do we still need this with patch 3? I know it'd be nice if we could
> faithfully restore any state we can also program, but then that's also a
> lot of complexity ...
>
> Otoh patch 3 means we'll stop testing a lot of the fastboot code while
> reloading the driver. But then that's been the thing in the past, and as
> long as we still boot up we have at least some test coverage fo the
> fastboot code (I'm mostly concerned about the plane/buffer readout code,
> since that's not covered by the state checker).
>
> But for now I'd say let's just go with patch 3 only.
We don't need this patch, but it makes the audio power domain tracked like all 
other power domains. It's a nice cleanup in general and I would like it even 
without the rest of the series. Could otherwise be merged through the intel 
tree through.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel