* [PATCH] drm/i915: Hold a RPM reference during i915_driver_unload
@ 2015-12-29 10:55 Gabriel Feceoru
2015-12-29 12:32 ` ✗ warning: Fi.CI.BAT Patchwork
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Gabriel Feceoru @ 2015-12-29 10:55 UTC (permalink / raw)
To: intel-gfx
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@intel.com>
Signed-off-by: Gabriel Feceoru <gabriel.feceoru@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);
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);
+
kfree(dev_priv);
return 0;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread
* ✗ warning: Fi.CI.BAT
2015-12-29 10:55 [PATCH] drm/i915: Hold a RPM reference during i915_driver_unload Gabriel Feceoru
@ 2015-12-29 12:32 ` Patchwork
2015-12-29 12:40 ` Patchwork
2015-12-30 13:03 ` [PATCH] drm/i915: Hold a RPM reference during i915_driver_unload Joonas Lahtinen
2 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2015-12-29 12:32 UTC (permalink / raw)
To: Feceoru, Gabriel; +Cc: intel-gfx
== Summary ==
Built on ec0382c73cb1adc972bebdd94afad3f0ea117114 drm-intel-nightly: 2015y-12m-23d-22h-28m-25s UTC integration manifest
Test gem_storedw_loop:
Subgroup basic-render:
dmesg-warn -> PASS (skl-i5k-2)
Test kms_flip:
Subgroup basic-flip-vs-dpms:
pass -> DMESG-WARN (bsw-nuc-2)
dmesg-warn -> PASS (ilk-hp8440p)
Subgroup basic-flip-vs-modeset:
dmesg-warn -> PASS (skl-i5k-2)
pass -> DMESG-WARN (hsw-brixbox)
pass -> DMESG-WARN (bdw-nuci7)
Subgroup basic-plain-flip:
pass -> DMESG-WARN (bdw-ultra)
Test kms_pipe_crc_basic:
Subgroup read-crc-pipe-a:
dmesg-warn -> PASS (snb-x220t)
Subgroup read-crc-pipe-b:
pass -> DMESG-WARN (snb-dellxps)
Subgroup read-crc-pipe-c-frame-sequence:
dmesg-warn -> PASS (bsw-nuc-2)
Subgroup suspend-read-crc-pipe-a:
pass -> DMESG-WARN (snb-x220t)
Test kms_psr_sink_crc:
Subgroup psr_basic:
dmesg-warn -> PASS (bdw-ultra)
Test kms_setmode:
Subgroup basic-clone-single-crtc:
dmesg-warn -> PASS (snb-dellxps)
Test pm_rpm:
Subgroup basic-rte:
pass -> DMESG-WARN (byt-nuc)
bdw-nuci7 total:132 pass:120 dwarn:3 dfail:0 fail:0 skip:9
bdw-ultra total:132 pass:123 dwarn:2 dfail:0 fail:1 skip:6
bsw-nuc-2 total:135 pass:113 dwarn:2 dfail:0 fail:0 skip:20
byt-nuc total:135 pass:120 dwarn:2 dfail:0 fail:0 skip:13
hsw-brixbox total:135 pass:126 dwarn:2 dfail:0 fail:0 skip:7
hsw-gt2 total:135 pass:130 dwarn:1 dfail:0 fail:0 skip:4
ilk-hp8440p total:135 pass:100 dwarn:0 dfail:0 fail:0 skip:35
ivb-t430s total:135 pass:127 dwarn:2 dfail:0 fail:0 skip:6
skl-i5k-2 total:135 pass:125 dwarn:2 dfail:0 fail:0 skip:8
skl-i7k-2 total:135 pass:124 dwarn:3 dfail:0 fail:0 skip:8
snb-dellxps total:135 pass:121 dwarn:2 dfail:0 fail:0 skip:12
snb-x220t total:135 pass:121 dwarn:2 dfail:0 fail:1 skip:11
Results at /archive/results/CI_IGT_test/Patchwork_935/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* ✗ warning: Fi.CI.BAT
2015-12-29 10:55 [PATCH] drm/i915: Hold a RPM reference during i915_driver_unload Gabriel Feceoru
2015-12-29 12:32 ` ✗ warning: Fi.CI.BAT Patchwork
@ 2015-12-29 12:40 ` Patchwork
2015-12-30 13:03 ` [PATCH] drm/i915: Hold a RPM reference during i915_driver_unload Joonas Lahtinen
2 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2015-12-29 12:40 UTC (permalink / raw)
To: Feceoru, Gabriel; +Cc: intel-gfx
== Summary ==
Built on ec0382c73cb1adc972bebdd94afad3f0ea117114 drm-intel-nightly: 2015y-12m-23d-22h-28m-25s UTC integration manifest
Test gem_storedw_loop:
Subgroup basic-render:
dmesg-warn -> PASS (skl-i5k-2)
Test kms_flip:
Subgroup basic-flip-vs-dpms:
pass -> DMESG-WARN (bsw-nuc-2)
dmesg-warn -> PASS (ilk-hp8440p)
Subgroup basic-flip-vs-modeset:
dmesg-warn -> PASS (skl-i5k-2)
pass -> DMESG-WARN (hsw-xps12)
pass -> DMESG-WARN (hsw-brixbox)
pass -> DMESG-WARN (bdw-nuci7)
Subgroup basic-plain-flip:
pass -> DMESG-WARN (bdw-ultra)
Test kms_pipe_crc_basic:
Subgroup read-crc-pipe-a:
dmesg-warn -> PASS (snb-x220t)
Subgroup read-crc-pipe-b:
pass -> DMESG-WARN (snb-dellxps)
Subgroup read-crc-pipe-c-frame-sequence:
dmesg-warn -> PASS (bsw-nuc-2)
Subgroup suspend-read-crc-pipe-a:
pass -> DMESG-WARN (snb-x220t)
Test kms_psr_sink_crc:
Subgroup psr_basic:
dmesg-warn -> PASS (bdw-ultra)
Test kms_setmode:
Subgroup basic-clone-single-crtc:
dmesg-warn -> PASS (snb-dellxps)
Test pm_rpm:
Subgroup basic-rte:
dmesg-warn -> PASS (hsw-xps12)
pass -> DMESG-WARN (byt-nuc)
bdw-nuci7 total:132 pass:120 dwarn:3 dfail:0 fail:0 skip:9
bdw-ultra total:132 pass:123 dwarn:2 dfail:0 fail:1 skip:6
bsw-nuc-2 total:135 pass:113 dwarn:2 dfail:0 fail:0 skip:20
byt-nuc total:135 pass:120 dwarn:2 dfail:0 fail:0 skip:13
hsw-brixbox total:135 pass:126 dwarn:2 dfail:0 fail:0 skip:7
hsw-gt2 total:135 pass:130 dwarn:1 dfail:0 fail:0 skip:4
hsw-xps12 total:132 pass:125 dwarn:3 dfail:0 fail:0 skip:4
ilk-hp8440p total:135 pass:100 dwarn:0 dfail:0 fail:0 skip:35
ivb-t430s total:135 pass:127 dwarn:2 dfail:0 fail:0 skip:6
skl-i5k-2 total:135 pass:125 dwarn:2 dfail:0 fail:0 skip:8
skl-i7k-2 total:135 pass:124 dwarn:3 dfail:0 fail:0 skip:8
snb-dellxps total:135 pass:121 dwarn:2 dfail:0 fail:0 skip:12
snb-x220t total:135 pass:121 dwarn:2 dfail:0 fail:1 skip:11
Results at /archive/results/CI_IGT_test/Patchwork_935/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Hold a RPM reference during i915_driver_unload
2015-12-29 10:55 [PATCH] drm/i915: Hold a RPM reference during i915_driver_unload Gabriel Feceoru
2015-12-29 12:32 ` ✗ warning: Fi.CI.BAT Patchwork
2015-12-29 12:40 ` Patchwork
@ 2015-12-30 13:03 ` Joonas Lahtinen
2015-12-31 10:44 ` Gabriel Feceoru
2016-01-14 17:30 ` Imre Deak
2 siblings, 2 replies; 7+ messages in thread
From: Joonas Lahtinen @ 2015-12-30 13:03 UTC (permalink / raw)
To: Gabriel Feceoru, Imre Deak, intel-gfx
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@intel.com>
> Signed-off-by: Gabriel Feceoru <gabriel.feceoru@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.
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
>
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Hold a RPM reference during i915_driver_unload
2015-12-30 13:03 ` [PATCH] drm/i915: Hold a RPM reference during i915_driver_unload Joonas Lahtinen
@ 2015-12-31 10:44 ` Gabriel Feceoru
2015-12-31 11:27 ` Joonas Lahtinen
2016-01-14 17:30 ` Imre Deak
1 sibling, 1 reply; 7+ messages in thread
From: Gabriel Feceoru @ 2015-12-31 10:44 UTC (permalink / raw)
To: Joonas Lahtinen, Imre Deak, intel-gfx
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@intel.com>
>> Signed-off-by: Gabriel Feceoru <gabriel.feceoru@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
>
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Hold a RPM reference during i915_driver_unload
2015-12-31 10:44 ` Gabriel Feceoru
@ 2015-12-31 11:27 ` Joonas Lahtinen
0 siblings, 0 replies; 7+ messages in thread
From: Joonas Lahtinen @ 2015-12-31 11:27 UTC (permalink / raw)
To: Gabriel Feceoru, Imre Deak, intel-gfx
On to, 2015-12-31 at 12:44 +0200, Gabriel Feceoru wrote:
>
> On 30.12.2015 15:03, Joonas Lahtinen wrote:
> > Hi
<SNIP>
> > > +++ 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'
In that case a second path is made for erroring out of the function
(see i915_driver_load):
...
return 0;
label1:
...
labelN:
intel_runtime_pm_put(dev_priv)
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().
Except that i915_driver_load is the function during which autosuspend
is enabled by calling intel_runtime_pm_enable so it's quite a different
animal. But I think that function has it incorrectly too, the return
path for no errors, _put is correct, but the return path for errors,
the _put should really be _put_noidle because the whole RPM thing was
torn down already see below for explanation.
The load/unload sequences are really not in sync (compare the _unload
function to the _load function teardown path for example), and in
addition to that it is not exactly clear (without going through all the
functions one by one) which functions might end up altering the
hardware state and do require the wakeref and which are tearing down
pure software constructs, so I might be off here, but this is what it
looks like to me.
I feel uneasy for the fact we're calling runtime_pm_put after releasing
the PCI device which basically should be the last put and which should
end up suspending the device when there is no device being held
anymore.
Also, in i915_driver_unload function the PM system teardown has already
been performed by intel_power_domains_fini way earlier in the function,
after which I think we can call at most runtime_pm_put_noidle because
essentially the device has been shut down already.
I do not think those error paths have been stressed much.
Regards, Joonas
> Waiting for Imre's feedback on this.
> >
> > > kfree(dev_priv);
> > >
> > > return 0;
> >
> > Insert goto label around here and make it "return ret;".
> >
> > Regards, Joonas
> >
> > >
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Hold a RPM reference during i915_driver_unload
2015-12-30 13:03 ` [PATCH] drm/i915: Hold a RPM reference during i915_driver_unload Joonas Lahtinen
2015-12-31 10:44 ` Gabriel Feceoru
@ 2016-01-14 17:30 ` Imre Deak
1 sibling, 0 replies; 7+ messages in thread
From: Imre Deak @ 2016-01-14 17:30 UTC (permalink / raw)
To: Joonas Lahtinen, Gabriel Feceoru, intel-gfx
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@intel.com>
> > Signed-off-by: Gabriel Feceoru <gabriel.feceoru@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
>
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-01-14 17:31 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-29 10:55 [PATCH] drm/i915: Hold a RPM reference during i915_driver_unload Gabriel Feceoru
2015-12-29 12:32 ` ✗ warning: Fi.CI.BAT Patchwork
2015-12-29 12:40 ` Patchwork
2015-12-30 13:03 ` [PATCH] drm/i915: Hold a RPM reference during i915_driver_unload Joonas Lahtinen
2015-12-31 10:44 ` Gabriel Feceoru
2015-12-31 11:27 ` Joonas Lahtinen
2016-01-14 17:30 ` Imre Deak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.