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

[Gabriel Feceoru]

On 30.12.2015 15:03, 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]  [<ffffffff813e0c0f>] dump_stack+0x44/0x55
>> [  138.682713]  [<ffffffff8105f6a2>] warn_slowpath_common+0x82/0xc0
>> [  138.682715]  [<ffffffff8105f72c>] warn_slowpath_fmt+0x4c/0x50
>> [  138.682725]  [<ffffffffc031aefc>] ?
>> i915_gem_object_unpin_from_display_plane+0x1c/0x50 [i915]
>> [  138.682734]  [<ffffffffc0333b9a>] gen6_read32+0x1ca/0x1e0 [i915]
>> [  138.682737]  [<ffffffff8172c562>] ? mutex_lock+0x12/0x30
>> [  138.682747]  [<ffffffffc03715ca>]
>> intel_ddi_get_hw_state+0x7a/0x180 [i915]
>> [  138.682758]  [<ffffffffc0355c88>]
>> intel_connector_get_hw_state+0x28/0x30 [i915]
>> [  138.682767]  [<ffffffffc03543fc>] intel_atomic_commit+0xa9c/0x17e0
>> [i915]
>> [  138.682779]  [<ffffffffc00a7e8e>] ?
>> drm_atomic_check_only+0x18e/0x590 [drm]
>> [  138.682786]  [<ffffffffc00a78cc>] ?
>> drm_atomic_add_affected_connectors+0x8c/0xf0 [drm]
>> [  138.682792]  [<ffffffffc00a82c7>] drm_atomic_commit+0x37/0x60
>> [drm]
>> [  138.682797]  [<ffffffffc0163356>]
>> drm_atomic_helper_set_config+0x76/0xb0 [drm_kms_helper]
>> [  138.682804]  [<ffffffffc00a696a>] ?
>> drm_modeset_lock_all_ctx+0x9a/0xb0 [drm]
>> [  138.682809]  [<ffffffffc00979c2>]
>> drm_mode_set_config_internal+0x62/0x100 [drm]
>> [  138.682814]  [<ffffffffc0097b48>]
>> drm_framebuffer_remove+0xe8/0x120 [drm]
>> [  138.682826]  [<ffffffffc036bb4d>] intel_fbdev_fini+0x6d/0x90
>> [i915]
>> [  138.682838]  [<ffffffffc0396b9a>] i915_driver_unload+0x1a/0x290
>> [i915]
>> [  138.682844]  [<ffffffffc0090ff9>] drm_dev_unregister+0x29/0xb0
>> [drm]
>> [  138.682848]  [<ffffffffc0091673>] drm_put_dev+0x23/0x60 [drm]
>> [  138.682854]  [<ffffffffc02dc315>] i915_pci_remove+0x15/0x20 [i915]
>> [  138.682856]  [<ffffffff8141f409>] pci_device_remove+0x39/0xc0
>> [  138.682859]  [<ffffffff814e3d61>]
>> __device_release_driver+0xa1/0x150
>> [  138.682860]  [<ffffffff814e4833>] driver_detach+0xa3/0xb0
>> [  138.682862]  [<ffffffff814e3825>] bus_remove_driver+0x55/0xd0
>> [  138.682864]  [<ffffffff814e4e2c>] driver_unregister+0x2c/0x50
>> [  138.682866]  [<ffffffff8141db31>] pci_unregister_driver+0x21/0x90
>> [  138.682871]  [<ffffffffc0092ec4>] drm_pci_exit+0x94/0xb0 [drm]
>> [  138.682883]  [<ffffffffc0397404>] 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.
I cannot have one instance only of intel_runtime_put() at the end of the 
function, since one exit branch uses kfree(dev_priv) and the other 
doesn't. Similar to i915_driver_load()
If this comment refers just to the 'return ret' - this is a legacy issue 
not related to this fix. Could be seen in many places (i915_driver_load 
again), just check i915_driver_open() where the last 3 lines of code 
could be replaced with 'return ret'
>
>>   		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.
>
> 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.
I'm not sure either, but again, the same is done in i915_driver_load().
Waiting for Imre's feedback on this.
>
>>   	kfree(dev_priv);
>>
>> 	return 0;
>
> Insert goto label around here and make it "return ret;".
>
> Regards, Joonas
>
>>