In preparation for runtime pm on muxed dual GPU laptops, I've fixed all runtime pm ref leaks I could find in nouveau, radeon and amdgpu. To ease reviewing, I've pushed this series to GitHub: https://github.com/l1k/linux/commits/drm_runpm_fixes_v1 @Alex Deucher: I do not have an AMD GPU so couldn't test this beyond verifying that it compiles. Please double-check the patches and test them internally at AMD. By the way, I've noticed that nouveau takes a runtime pm ref in ->preclose and releases it in ->postclose. This is missing in radeon and amdgpu. Please check if it is needed. Thanks, Lukas Lukas Wunner (9): drm/nouveau: Don't leak runtime pm ref on driver unload drm/nouveau: Forbid runtime pm on driver unload drm/radeon: Don't leak runtime pm ref on driver unload drm/radeon: Don't leak runtime pm ref on driver load drm/radeon: Forbid runtime pm on driver unload drm/amdgpu: Don't leak runtime pm ref on driver unload drm/amdgpu: Don't leak runtime pm ref on driver load drm/amdgpu: Forbid runtime pm on driver unload drm: Turn off crtc before tearing down its data structure drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 12 +++++++++--- drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++- drivers/gpu/drm/nouveau/nouveau_drm.c | 6 +++++- drivers/gpu/drm/radeon/radeon_device.c | 4 ++++ drivers/gpu/drm/radeon/radeon_kms.c | 5 ++++- 5 files changed, 34 insertions(+), 6 deletions(-) -- 2.8.1
Lukas Wunner
2016-May-24 16:03 UTC
[Nouveau] [PATCH 2/9] drm/nouveau: Forbid runtime pm on driver unload
The PCI core calls pm_runtime_forbid() on device probe in pci_pm_init(), making this the default state when nouveau is loaded. nouveau_drm_load() therefore calls pm_runtime_allow(), but there's no pm_runtime_forbid() in nouveau_drm_unload() to balance it. Add it so that we leave the device in the same state that we found it. This isn't a bug, it's just good housekeeping. When nouveau is first loaded with runpm=1, then unloaded and loaded again with runpm=0, pm_runtime_forbid() will be called from nouveau_pmops_runtime_idle() or nouveau_pmops_runtime_suspend(), so the behaviour is correct. The nvidia blob doesn't use runtime pm, but if it ever does, this commit avoids that it has to clean up behind nouveau. Tested-by: Karol Herbst <nouveau at karolherbst.de> Signed-off-by: Lukas Wunner <lukas at wunner.de> --- drivers/gpu/drm/nouveau/nouveau_drm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index faf7438..ef784b7 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -500,6 +500,7 @@ nouveau_drm_unload(struct drm_device *dev) if (nouveau_runtime_pm != 0) { pm_runtime_get_sync(dev->dev); + pm_runtime_forbid(dev->dev); } nouveau_fbcon_fini(dev); -- 2.8.1
Lukas Wunner
2016-May-24 16:03 UTC
[Nouveau] [PATCH 9/9] drm: Turn off crtc before tearing down its data structure
When a drm_crtc structure is destroyed with drm_crtc_cleanup(), the DRM core does not turn off the crtc first and neither do the drivers. With nouveau, radeon and amdgpu, this causes a runtime pm ref to be leaked on driver unload if at least one crtc was enabled. (See usage of have_disp_power_ref in nouveau_crtc_set_config(), radeon_crtc_set_config() and amdgpu_crtc_set_config()). Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)") Cc: Dave Airlie <airlied at redhat.com> Tested-by: Karol Herbst <nouveau at karolherbst.de> Signed-off-by: Lukas Wunner <lukas at wunner.de> --- drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d2a6d95..0cd6f00 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -716,12 +716,23 @@ EXPORT_SYMBOL(drm_crtc_init_with_planes); * * This function cleans up @crtc and removes it from the DRM mode setting * core. Note that the function does *not* free the crtc structure itself, - * this is the responsibility of the caller. + * this is the responsibility of the caller. If @crtc is currently enabled, + * it is turned off first. */ void drm_crtc_cleanup(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; + if (crtc->enabled) { + struct drm_mode_set modeset = { + .crtc = crtc, + }; + + drm_modeset_lock_all(dev); + drm_mode_set_config_internal(&modeset); + drm_modeset_unlock_all(dev); + } + kfree(crtc->gamma_store); crtc->gamma_store = NULL; -- 2.8.1
Lukas Wunner
2016-May-24 16:03 UTC
[Nouveau] [PATCH 1/9] drm/nouveau: Don't leak runtime pm ref on driver unload
nouveau_drm_load() calls pm_runtime_put() if nouveau_runtime_pm != 0, but nouveau_drm_unload() calls pm_runtime_get_sync() unconditionally. We therefore leak a runtime pm ref whenever nouveau is loaded with runpm=0 and then unloaded. The GPU will subsequently never runtime suspend even if nouveau is loaded again with runpm=1. Fix by taking the runtime pm ref under the same condition that it was released on driver load. Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)") Cc: Dave Airlie <airlied at redhat.com> Reported-by: Karol Herbst <nouveau at karolherbst.de> Tested-by: Karol Herbst <nouveau at karolherbst.de> Signed-off-by: Lukas Wunner <lukas at wunner.de> --- drivers/gpu/drm/nouveau/nouveau_drm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 11f8dd9..faf7438 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -498,7 +498,10 @@ nouveau_drm_unload(struct drm_device *dev) { struct nouveau_drm *drm = nouveau_drm(dev); - pm_runtime_get_sync(dev->dev); + if (nouveau_runtime_pm != 0) { + pm_runtime_get_sync(dev->dev); + } + nouveau_fbcon_fini(dev); nouveau_accel_fini(drm); nouveau_hwmon_fini(dev); -- 2.8.1
Daniel Vetter
2016-May-24 21:30 UTC
[Nouveau] [PATCH 9/9] drm: Turn off crtc before tearing down its data structure
On Tue, May 24, 2016 at 06:03:27PM +0200, Lukas Wunner wrote:> When a drm_crtc structure is destroyed with drm_crtc_cleanup(), the DRM > core does not turn off the crtc first and neither do the drivers. With > nouveau, radeon and amdgpu, this causes a runtime pm ref to be leaked on > driver unload if at least one crtc was enabled. > > (See usage of have_disp_power_ref in nouveau_crtc_set_config(), > radeon_crtc_set_config() and amdgpu_crtc_set_config()). > > Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)") > Cc: Dave Airlie <airlied at redhat.com> > Tested-by: Karol Herbst <nouveau at karolherbst.de> > Signed-off-by: Lukas Wunner <lukas at wunner.de>This is a core regression, we fixed it again. Previously when unreference drm_planes the core made sure that it's not longer in use, which had the side effect of shutting everything off in module unload. For a bunch of reasons we've stopped doing that, but that turned out to be a mistake. It's fixed since commit f2d580b9a8149735cbc4b59c4a8df60173658140 Author: Maarten Lankhorst <maarten.lankhorst at linux.intel.com> Date: Wed May 4 14:38:26 2016 +0200 drm/core: Do not preserve framebuffer on rmfb, v4. Your patch shouldn't be needed with that any more. If it still is it's most likely the fbdev cleanup done too late, but you /should/ get a big WARNING splat in that case from drm_mode_config_cleanup(). -Daniel> --- > drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index d2a6d95..0cd6f00 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -716,12 +716,23 @@ EXPORT_SYMBOL(drm_crtc_init_with_planes); > * > * This function cleans up @crtc and removes it from the DRM mode setting > * core. Note that the function does *not* free the crtc structure itself, > - * this is the responsibility of the caller. > + * this is the responsibility of the caller. If @crtc is currently enabled, > + * it is turned off first. > */ > void drm_crtc_cleanup(struct drm_crtc *crtc) > { > struct drm_device *dev = crtc->dev; > > + if (crtc->enabled) { > + struct drm_mode_set modeset = { > + .crtc = crtc, > + }; > + > + drm_modeset_lock_all(dev); > + drm_mode_set_config_internal(&modeset); > + drm_modeset_unlock_all(dev); > + } > + > kfree(crtc->gamma_store); > crtc->gamma_store = NULL; > > -- > 2.8.1 > > _______________________________________________ > Nouveau mailing list > Nouveau at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Peter Wu
2016-May-27 01:07 UTC
[Nouveau] [PATCH 1/9] drm/nouveau: Don't leak runtime pm ref on driver unload
On Tue, May 24, 2016 at 06:03:27PM +0200, Lukas Wunner wrote:> nouveau_drm_load() calls pm_runtime_put() if nouveau_runtime_pm != 0, > but nouveau_drm_unload() calls pm_runtime_get_sync() unconditionally. > We therefore leak a runtime pm ref whenever nouveau is loaded with > runpm=0 and then unloaded. The GPU will subsequently never runtime > suspend even if nouveau is loaded again with runpm=1. > > Fix by taking the runtime pm ref under the same condition that it was > released on driver load. > > Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)") > Cc: Dave Airlie <airlied at redhat.com> > Reported-by: Karol Herbst <nouveau at karolherbst.de> > Tested-by: Karol Herbst <nouveau at karolherbst.de> > Signed-off-by: Lukas Wunner <lukas at wunner.de>Looks good, I tested this scenario: ru(){ cat /sys/bus/pci/devices/0000\:01:00.0/power/runtime_usage;} ru # reports 1 modprobe nouveau runpm=0 ru # reports 2 rmmod nouveau ru # reports 1 Without runpm=0 the count drops to 0 in the second step and stays 0 in the third step. After applying patch 2/9, this correctly reports 1 as expected (this is the same as manually setting power/control to on). Peter> --- > drivers/gpu/drm/nouveau/nouveau_drm.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c > index 11f8dd9..faf7438 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c > @@ -498,7 +498,10 @@ nouveau_drm_unload(struct drm_device *dev) > { > struct nouveau_drm *drm = nouveau_drm(dev); > > - pm_runtime_get_sync(dev->dev); > + if (nouveau_runtime_pm != 0) { > + pm_runtime_get_sync(dev->dev); > + } > + > nouveau_fbcon_fini(dev); > nouveau_accel_fini(drm); > nouveau_hwmon_fini(dev); > -- > 2.8.1 > > _______________________________________________ > Nouveau mailing list > Nouveau at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau-- Kind regards, Peter Wu https://lekensteyn.nl
Apparently Analagous Threads
- [PATCH 1/9] drm/nouveau: Don't leak runtime pm ref on driver unload
- [PATCH 1/9] drm/nouveau: Don't leak runtime pm ref on driver unload
- [PATCH 1/9] drm/nouveau: Don't leak runtime pm ref on driver unload
- [PATCH 0/9] Fix runtime pm ref leaks
- [PATCH] drm: fix issue by messing up runpm usage_counter