[Intel-gfx] [PATCH] drm/i915: Move atomic state free from out of fence release

[Chris Wilson]

On Tue, Jan 24, 2017 at 10:39:44AM +0200, Joonas Lahtinen wrote:
> On ma, 2017-01-23 at 21:29 +0000, Chris Wilson wrote:
> > Fences are required to support being released from under an atomic context.
> > The drm_atomic_state struct may take a mutex when being released and so
> > we cannot drop a reference to the drm_atomic_state from the fence release
> > path directly, and so we need to defer that unreference to a worker.
> > 
> > [  326.576697] WARNING: CPU: 2 PID: 366 at kernel/sched/core.c:7737 __might_sleep+0x5d/0x80
> > [  326.576816] do not call blocking ops when !TASK_RUNNING; state=1 set at [<ffffffffc0359549>] intel_breadcrumbs_signaler+0x59/0x270 [i915]
> > [  326.576818] Modules linked in: rfcomm fuse snd_hda_codec_hdmi bnep snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core snd_pcm snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq snd_seq_device snd_timer input_leds led_class snd punit_atom_debug btusb btrtl btbcm btintel intel_rapl bluetooth i915 drm_kms_helper syscopyarea sysfillrect iwlwifi sysimgblt soundcore fb_sys_fops mei_txe cfg80211 drm pwm_lpss_platform pwm_lpss pinctrl_cherryview fjes acpi_pad parport_pc ppdev parport autofs4
> > [  326.576899] CPU: 2 PID: 366 Comm: i915/signal:0 Tainted: G     U          4.10.0-rc3-patser+ #5030
> > [  326.576902] Hardware name:                  /NUC5PPYB, BIOS PYBSWCEL.86A.0031.2015.0601.1712 06/01/2015
> > [  326.576905] Call Trace:
> > [  326.576920]  dump_stack+0x4d/0x6d
> > [  326.576926]  __warn+0xc0/0xe0
> > [  326.576931]  warn_slowpath_fmt+0x5a/0x80
> > [  326.577004]  ? intel_breadcrumbs_signaler+0x59/0x270 [i915]
> > [  326.577075]  ? intel_breadcrumbs_signaler+0x59/0x270 [i915]
> > [  326.577079]  __might_sleep+0x5d/0x80
> > [  326.577087]  mutex_lock+0x1b/0x40
> > [  326.577133]  drm_property_free_blob+0x1e/0x80 [drm]
> > [  326.577167]  ? drm_property_destroy+0xe0/0xe0 [drm]
> > [  326.577200]  drm_mode_object_unreference+0x5c/0x70 [drm]
> > [  326.577233]  drm_property_unreference_blob+0xe/0x10 [drm]
> > [  326.577260]  __drm_atomic_helper_crtc_destroy_state+0x14/0x40 [drm_kms_helper]
> > [  326.577278]  drm_atomic_helper_crtc_destroy_state+0x10/0x20 [drm_kms_helper]
> > [  326.577352]  intel_crtc_destroy_state+0x9/0x10 [i915]
> > [  326.577388]  drm_atomic_state_default_clear+0xea/0x1d0 [drm]
> > [  326.577462]  intel_atomic_state_clear+0xd/0x20 [i915]
> > [  326.577497]  drm_atomic_state_clear+0x1a/0x30 [drm]
> > [  326.577532]  __drm_atomic_state_free+0x13/0x60 [drm]
> > [  326.577607]  intel_atomic_commit_ready+0x6f/0x78 [i915]
> > [  326.577670]  i915_sw_fence_release+0x3a/0x50 [i915]
> > [  326.577733]  dma_i915_sw_fence_wake+0x39/0x80 [i915]
> > [  326.577741]  dma_fence_signal+0xda/0x120
> > [  326.577812]  ? intel_breadcrumbs_signaler+0x59/0x270 [i915]
> > [  326.577884]  intel_breadcrumbs_signaler+0xb1/0x270 [i915]
> > [  326.577889]  kthread+0x127/0x130
> > [  326.577961]  ? intel_engine_remove_wait+0x1a0/0x1a0 [i915]
> > [  326.577964]  ? kthread_stop+0x120/0x120
> > [  326.577970]  ret_from_fork+0x22/0x30
> > 
> > Reported-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> 
> <SNIP>
> 
> > @@ -17329,6 +17350,9 @@ void intel_modeset_cleanup(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> > +	flush_work(&dev_priv->atomic_helper.free_work);
> > +	WARN_ON(!llist_empty(&dev_priv->atomic_helper.free_list));
> 
> Maybe make this while(!llist_empty) flush_work() to begin with? There's
> exactly no locking in place to prevent that from happening?

Right, there's no locking. But before modeset can be cleaned up, it must
have completed its final modeset to disable all crtc. That should be
sufficient to ensure that all pending atomic state have been flushed. If
not we need some wait_for(atomic_commits_finished()) ; first. Given
that, there's no expectation that the free_list is self-arming, nor do
we have a RCU grace period to worry about, so a while is currently
misleading.

Now I'm not totally convinced that all the atomic workers are
necessarily complete before we flush the freed work/list.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre