[Intel-gfx] [PATCH] drm/i915: Hold a RPM reference during i915_driver_unload

[Imre Deak]

On ke, 2015-12-30 at 15:03 +0200, Joonas Lahtinen wrote:
> Hi,
> 
> On ti, 2015-12-29 at 12:55 +0200, Gabriel Feceoru wrote:
> > This fixes an issue added with: "1f814da drm/i915: add support for
> > checking
> > if we hold an RPM reference", noticed while running
> > drv_module_reload_basic.
> > 
> > WARNING: CPU: 1 PID: 2032 at drivers/gpu/drm/i915/intel_drv.h:1446
> > gen6_read32+0x1ca/0x1e0 [i915]()
> > [  138.682686] RPM wakelock ref not held during HW access
> > [  138.682687] Modules linked in:
> > [  138.682688]  i915(-) drm_kms_helper drm snd_hda_codec_hdmi
> > snd_hda_codec_realtek snd_hda_codec_generic snd_hda_codec snd_hwdep
> > x86_pkg_temp_thermal snd_hda_core i2c_algo_bit syscopyarea
> > sysfillrect sysimgblt fb_sys_fops xhci_pci ehci_pci r8169 xhci_hcd
> > mii ehci_hcd video [last unloaded: snd_hda_intel]
> > [  138.682699] CPU: 1 PID: 2032 Comm: rmmod Tainted: G        W      
> >  4.4.0-rc4+ #44
> > [  138.682701] Hardware name: Dell Inc. Inspiron 3847/088DT1       ,
> > BIOS A06 01/15/2015
> > [  138.682702]  ffffffffc03b6358 ffff880210d8ba58 ffffffff813e0c0f
> > ffff880210d8baa0
> > [  138.682703]  ffff880210d8ba90 ffffffff8105f6a2 ffff8800daa40000
> > 0000000000064400
> > [  138.682705]  0000000000000004 ffff880210d8bb9c ffff8800daa40000
> > ffff880210d8baf0
> > [  138.682706] Call Trace:
> > [  138.682710]  [] dump_stack+0x44/0x55
> > [  138.682713]  [] warn_slowpath_common+0x82/0xc0
> > [  138.682715]  [] warn_slowpath_fmt+0x4c/0x50
> > [  138.682725]  [] ?
> > i915_gem_object_unpin_from_display_plane+0x1c/0x50 [i915]
> > [  138.682734]  [] gen6_read32+0x1ca/0x1e0 [i915]
> > [  138.682737]  [] ? mutex_lock+0x12/0x30
> > [  138.682747]  []
> > intel_ddi_get_hw_state+0x7a/0x180 [i915]
> > [  138.682758]  []
> > intel_connector_get_hw_state+0x28/0x30 [i915]
> > [  138.682767]  [] intel_atomic_commit+0xa9c/0x17e0
> > [i915]
> > [  138.682779]  [] ?
> > drm_atomic_check_only+0x18e/0x590 [drm]
> > [  138.682786]  [] ?
> > drm_atomic_add_affected_connectors+0x8c/0xf0 [drm]
> > [  138.682792]  [] drm_atomic_commit+0x37/0x60
> > [drm]
> > [  138.682797]  []
> > drm_atomic_helper_set_config+0x76/0xb0 [drm_kms_helper]
> > [  138.682804]  [] ?
> > drm_modeset_lock_all_ctx+0x9a/0xb0 [drm]
> > [  138.682809]  []
> > drm_mode_set_config_internal+0x62/0x100 [drm]
> > [  138.682814]  []
> > drm_framebuffer_remove+0xe8/0x120 [drm]
> > [  138.682826]  [] intel_fbdev_fini+0x6d/0x90
> > [i915]
> > [  138.682838]  [] i915_driver_unload+0x1a/0x290
> > [i915]
> > [  138.682844]  [] drm_dev_unregister+0x29/0xb0
> > [drm]
> > [  138.682848]  [] drm_put_dev+0x23/0x60 [drm]
> > [  138.682854]  [] i915_pci_remove+0x15/0x20 [i915]
> > [  138.682856]  [] pci_device_remove+0x39/0xc0
> > [  138.682859]  []
> > __device_release_driver+0xa1/0x150
> > [  138.682860]  [] driver_detach+0xa3/0xb0
> > [  138.682862]  [] bus_remove_driver+0x55/0xd0
> > [  138.682864]  [] driver_unregister+0x2c/0x50
> > [  138.682866]  [] pci_unregister_driver+0x21/0x90
> > [  138.682871]  [] drm_pci_exit+0x94/0xb0 [drm]
> > [  138.682883]  [] i915_exit+0x20/0xc1c [i915]
> > 
> > Reported-by: Marius Vlad <marius.c.vlad at intel.com>
> > Signed-off-by: Gabriel Feceoru <gabriel.feceoru at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > b/drivers/gpu/drm/i915/i915_dma.c
> > index 988a380..08ad01f0 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1136,6 +1136,8 @@ int i915_driver_unload(struct drm_device
> > *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int ret;
> >  
> > +	intel_runtime_pm_get(dev_priv);
> > +
> >  	intel_fbdev_fini(dev);
> >  
> >  	i915_audio_component_cleanup(dev_priv);
> > @@ -1143,6 +1145,7 @@ int i915_driver_unload(struct drm_device
> > *dev)
> >  	ret = i915_gem_suspend(dev);
> >  	if (ret) {
> >  		DRM_ERROR("failed to idle hardware: %d\n", ret);
> > +		intel_runtime_pm_put(dev_priv);
> 
> This should be made into goto construct.
> 
> >  		return ret;
> >  	}
> >  
> > @@ -1221,6 +1224,9 @@ int i915_driver_unload(struct drm_device
> > *dev)
> >  	kmem_cache_destroy(dev_priv->vmas);
> >  	kmem_cache_destroy(dev_priv->objects);
> >  	pci_dev_put(dev_priv->bridge_dev);
> > +
> > +	intel_runtime_pm_put(dev_priv);
> > +
> 
> Not sure if we should/can keep the runtime reference until this
> point.
> At worst this could lead into the runtime_pm_put function poking at
> the
> hardware registers after the pci_dev has been released.

Well if you meant the above bridge_dev PCI device it's not relevant as
far as power management goes, we only care about the main PCI device.
That one in turn is guaranteed to exist while we are in the load/unload
functions.

But since this looks like fixing the problem where the HW readout code
in the modeset state checker doesn't hold an RPM reference I would
prefer fixing it there, see the discussion with Chris about it. Also
note that intel_power_domains_fini() will already ensure that an RPM
reference is held for the rest of the function, if we really needed we
could bring that one earlier, but I think the state checker fix would
be enough.

> 
> Also if we change the hangcheck task to execute depending on the
> runtime_pm count, this will surely cause trouble. Added Imre as CC to
> comment on this.
> 
> >  	kfree(dev_priv);
> > 
> > 	return 0;
> 
> Insert goto label around here and make it "return ret;".
> 
> Regards, Joonas
> 
> >