Second iteration of my endeavour to rid nouveau, radeon and amdgpu of runtime pm ref leaks. Patches 1 to 8 are identical to v1. Patch 9 of v1 modified the DRM core to turn off all CRTCs on driver unload. Based on feedback by Daniel Vetter, I've replaced this with a helper to turn off all CRTCs, which is called by nouveau, radeon and amdgpu on unload. In other words, this is now opt-in. So patch 9 of v1 is replaced with new patches 9 to 12. A by-product of patch 9 is a helper which turns off a *single* CRTC. This is open coded in three other places in the DRM tree and patches 13 to 15 refactor those to use the new helper. To ease reviewing, I've pushed this series to GitHub: https://github.com/l1k/linux/commits/drm_runpm_fixes_v2 The discussion on v1 is archived here: https://lists.freedesktop.org/archives/dri-devel/2016-May/thread.html#108278 Thanks, Lukas Lukas Wunner (15): 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: Add helpers to turn off CRTCs drm/nouveau: Turn off CRTCs on driver unload drm/radeon: Turn off CRTCs on driver unload drm/amdgpu: Turn off CRTCs on driver unload drm: Use helper to turn off CRTC drm/i2c/ch7006: Use helper to turn off CRTC drm/nouveau/dispnv04: Use helper to turn off CRTC drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 12 +++++-- drivers/gpu/drm/drm_crtc.c | 53 ++++++++++++++++++++++++++---- drivers/gpu/drm/i2c/ch7006_drv.c | 9 ++--- drivers/gpu/drm/nouveau/dispnv04/disp.c | 10 ------ drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 9 ++--- drivers/gpu/drm/nouveau/nouveau_display.c | 1 + drivers/gpu/drm/nouveau/nouveau_drm.c | 6 +++- drivers/gpu/drm/radeon/radeon_device.c | 4 +++ drivers/gpu/drm/radeon/radeon_display.c | 1 + drivers/gpu/drm/radeon/radeon_kms.c | 5 ++- include/drm/drm_crtc.h | 2 ++ 12 files changed, 77 insertions(+), 36 deletions(-) -- 2.8.1
Lukas Wunner
2016-Jun-08 16:47 UTC
[Nouveau] [PATCH v2 01/15] 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> Cc: Ben Skeggs <bskeggs at redhat.com> Reported-by: Karol Herbst <karolherbst at gmail.com> Tested-by: Karol Herbst <karolherbst at gmail.com> Tested-by: Peter Wu <peter at lekensteyn.nl> 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 5c4d4df..2b0f441 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -490,7 +490,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
Lukas Wunner
2016-Jun-08 16:47 UTC
[Nouveau] [PATCH v2 02/15] 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. Cc: Ben Skeggs <bskeggs at redhat.com> Tested-by: Karol Herbst <karolherbst at gmail.com> Tested-by: Peter Wu <peter at lekensteyn.nl> 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 2b0f441..e1d1d9c 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -492,6 +492,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-Jun-08 16:47 UTC
[Nouveau] [PATCH v2 09/15] drm: Add helpers to turn off CRTCs
Turning off a single CRTC or all active CRTCs of a DRM device is a fairly common pattern. Add helpers to avoid open coding this everywhere. The name was chosen to be consistent with drm_plane_force_disable(). Cc: Daniel Vetter <daniel at ffwll.ch> Signed-off-by: Lukas Wunner <lukas at wunner.de> --- drivers/gpu/drm/drm_crtc.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 2 ++ 2 files changed, 47 insertions(+) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 37427b2..1edc1f4 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -426,6 +426,51 @@ void drm_mode_object_reference(struct drm_mode_object *obj) } EXPORT_SYMBOL(drm_mode_object_reference); +/** + * drm_crtc_force_disable - Forcibly turn off a CRTC + * @crtc: CRTC to turn off + * + * Returns: + * Zero on success, error code on failure. + */ +int drm_crtc_force_disable(struct drm_crtc *crtc) +{ + struct drm_mode_set set = { + .crtc = crtc, + }; + + return drm_mode_set_config_internal(&set); +} +EXPORT_SYMBOL(drm_crtc_force_disable); + +/** + * drm_crtc_force_disable_all - Forcibly turn off all enabled CRTCs + * @dev: DRM device whose CRTCs to turn off + * + * Drivers may want to call this on unload to ensure that all displays are + * unlit and the GPU is in a consistent, low power state. Takes modeset locks. + * + * Returns: + * Zero on success, error code on failure. + */ +int drm_crtc_force_disable_all(struct drm_device *dev) +{ + struct drm_crtc *crtc; + int ret = 0; + + drm_modeset_lock_all(dev); + drm_for_each_crtc(crtc, dev) + if (crtc->enabled) { + ret = drm_crtc_force_disable(crtc); + if (ret) + goto out; + } +out: + drm_modeset_unlock_all(dev); + return ret; +} +EXPORT_SYMBOL(drm_crtc_force_disable_all); + static void drm_framebuffer_free(struct kref *kref) { struct drm_framebuffer *fb diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index d1559cd..c192f2c 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -2326,6 +2326,8 @@ extern int drm_crtc_check_viewport(const struct drm_crtc *crtc, int x, int y, const struct drm_display_mode *mode, const struct drm_framebuffer *fb); +extern int drm_crtc_force_disable(struct drm_crtc *crtc); +extern int drm_crtc_force_disable_all(struct drm_device *dev); extern void drm_encoder_cleanup(struct drm_encoder *encoder); -- 2.8.1
Lukas Wunner
2016-Jun-08 16:47 UTC
[Nouveau] [PATCH v2 10/15] drm/nouveau: Turn off CRTCs on driver unload
nouveau leaks a runtime pm ref if at least one CRTC is enabled on unload. The ref is taken by nouveau_crtc_set_config() and held as long as a CRTC is in use. nv04_display_destroy() should solve this by turning off all CRTCs, but (1) nv50_display_destroy() doesn't do the same and (2) it's broken since commit d6bf2f370703 ("drm/nouveau: run mode_config destructor before destroying internal display state") because the crtc structs are torn down by drm_mode_config_cleanup() before being turned off. Also, there's no locking. Move the code to turn off all CRTCs from nv04_display_destroy() to nouveau_display_destroy() so that it's called for both nv04 and nv50 and before drm_mode_config_cleanup(). Use drm_crtc_force_disable_all() helper to save on code and have proper locking. Cc: Ben Skeggs <bskeggs at redhat.com> Signed-off-by: Lukas Wunner <lukas at wunner.de> --- drivers/gpu/drm/nouveau/dispnv04/disp.c | 10 ---------- drivers/gpu/drm/nouveau/nouveau_display.c | 1 + 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/drivers/gpu/drm/nouveau/dispnv04/disp.c b/drivers/gpu/drm/nouveau/dispnv04/disp.c index aea81a5..34c0f2f6 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv04/disp.c @@ -125,18 +125,8 @@ nv04_display_destroy(struct drm_device *dev) struct nv04_display *disp = nv04_display(dev); struct nouveau_drm *drm = nouveau_drm(dev); struct nouveau_encoder *encoder; - struct drm_crtc *crtc; struct nouveau_crtc *nv_crtc; - /* Turn every CRTC off. */ - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { - struct drm_mode_set modeset = { - .crtc = crtc, - }; - - drm_mode_set_config_internal(&modeset); - } - /* Restore state */ list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.base.head) encoder->enc_restore(&encoder->base.base); diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index 7c77f96..cbdb3d4 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -554,6 +554,7 @@ nouveau_display_destroy(struct drm_device *dev) nouveau_display_vblank_fini(dev); drm_kms_helper_poll_fini(dev); + drm_crtc_force_disable_all(dev); drm_mode_config_cleanup(dev); if (disp->dtor) -- 2.8.1
Lukas Wunner
2016-Jun-08 16:47 UTC
[Nouveau] [PATCH v2 15/15] drm/nouveau/dispnv04: Use helper to turn off CRTC
Use shiny new drm_crtc_force_disable() instead of open coding the same. No functional change intended. Cc: Ben Skeggs <bskeggs at redhat.com> Signed-off-by: Lukas Wunner <lukas at wunner.de> --- drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c b/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c index a665b78..434d1e2 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c +++ b/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c @@ -749,13 +749,8 @@ static int nv17_tv_set_property(struct drm_encoder *encoder, /* Disable the crtc to ensure a full modeset is * performed whenever it's turned on again. */ - if (crtc) { - struct drm_mode_set modeset = { - .crtc = crtc, - }; - - drm_mode_set_config_internal(&modeset); - } + if (crtc) + drm_crtc_force_disable(crtc); } return 0; -- 2.8.1
Daniel Vetter
2016-Jun-09 06:50 UTC
[Nouveau] [PATCH v2 00/15] Runtime pm ref leak bonanza
On Wed, Jun 08, 2016 at 06:47:27PM +0200, Lukas Wunner wrote:> Second iteration of my endeavour to rid nouveau, radeon and amdgpu of > runtime pm ref leaks. > > Patches 1 to 8 are identical to v1. > > Patch 9 of v1 modified the DRM core to turn off all CRTCs on driver > unload. Based on feedback by Daniel Vetter, I've replaced this with > a helper to turn off all CRTCs, which is called by nouveau, radeon > and amdgpu on unload. In other words, this is now opt-in. > So patch 9 of v1 is replaced with new patches 9 to 12. > > A by-product of patch 9 is a helper which turns off a *single* CRTC. > This is open coded in three other places in the DRM tree and patches > 13 to 15 refactor those to use the new helper.Yeah I think this makes much more sense. Please poke amd/nouveau folks for reviews/acks, then I can merge. -Daniel> > To ease reviewing, I've pushed this series to GitHub: > https://github.com/l1k/linux/commits/drm_runpm_fixes_v2 > > The discussion on v1 is archived here: > https://lists.freedesktop.org/archives/dri-devel/2016-May/thread.html#108278 > > Thanks, > > Lukas > > Lukas Wunner (15): > 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: Add helpers to turn off CRTCs > drm/nouveau: Turn off CRTCs on driver unload > drm/radeon: Turn off CRTCs on driver unload > drm/amdgpu: Turn off CRTCs on driver unload > drm: Use helper to turn off CRTC > drm/i2c/ch7006: Use helper to turn off CRTC > drm/nouveau/dispnv04: Use helper to turn off CRTC > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 + > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 12 +++++-- > drivers/gpu/drm/drm_crtc.c | 53 ++++++++++++++++++++++++++---- > drivers/gpu/drm/i2c/ch7006_drv.c | 9 ++--- > drivers/gpu/drm/nouveau/dispnv04/disp.c | 10 ------ > drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 9 ++--- > drivers/gpu/drm/nouveau/nouveau_display.c | 1 + > drivers/gpu/drm/nouveau/nouveau_drm.c | 6 +++- > drivers/gpu/drm/radeon/radeon_device.c | 4 +++ > drivers/gpu/drm/radeon/radeon_display.c | 1 + > drivers/gpu/drm/radeon/radeon_kms.c | 5 ++- > include/drm/drm_crtc.h | 2 ++ > 12 files changed, 77 insertions(+), 36 deletions(-) > > -- > 2.8.1 >-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On 06/09/2016 04:50 PM, Daniel Vetter wrote:> On Wed, Jun 08, 2016 at 06:47:27PM +0200, Lukas Wunner wrote: >> Second iteration of my endeavour to rid nouveau, radeon and amdgpu of >> runtime pm ref leaks. >> >> Patches 1 to 8 are identical to v1. >> >> Patch 9 of v1 modified the DRM core to turn off all CRTCs on driver >> unload. Based on feedback by Daniel Vetter, I've replaced this with >> a helper to turn off all CRTCs, which is called by nouveau, radeon >> and amdgpu on unload. In other words, this is now opt-in. >> So patch 9 of v1 is replaced with new patches 9 to 12. >> >> A by-product of patch 9 is a helper which turns off a *single* CRTC. >> This is open coded in three other places in the DRM tree and patches >> 13 to 15 refactor those to use the new helper. > > Yeah I think this makes much more sense. Please poke amd/nouveau folks for > reviews/acks, then I can merge.Looks fine to me. Ben.> -Daniel > >> >> To ease reviewing, I've pushed this series to GitHub: >> https://github.com/l1k/linux/commits/drm_runpm_fixes_v2 >> >> The discussion on v1 is archived here: >> https://lists.freedesktop.org/archives/dri-devel/2016-May/thread.html#108278 >> >> Thanks, >> >> Lukas >> >> Lukas Wunner (15): >> 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: Add helpers to turn off CRTCs >> drm/nouveau: Turn off CRTCs on driver unload >> drm/radeon: Turn off CRTCs on driver unload >> drm/amdgpu: Turn off CRTCs on driver unload >> drm: Use helper to turn off CRTC >> drm/i2c/ch7006: Use helper to turn off CRTC >> drm/nouveau/dispnv04: Use helper to turn off CRTC >> >> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 + >> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 12 +++++-- >> drivers/gpu/drm/drm_crtc.c | 53 ++++++++++++++++++++++++++---- >> drivers/gpu/drm/i2c/ch7006_drv.c | 9 ++--- >> drivers/gpu/drm/nouveau/dispnv04/disp.c | 10 ------ >> drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 9 ++--- >> drivers/gpu/drm/nouveau/nouveau_display.c | 1 + >> drivers/gpu/drm/nouveau/nouveau_drm.c | 6 +++- >> drivers/gpu/drm/radeon/radeon_device.c | 4 +++ >> drivers/gpu/drm/radeon/radeon_display.c | 1 + >> drivers/gpu/drm/radeon/radeon_kms.c | 5 ++- >> include/drm/drm_crtc.h | 2 ++ >> 12 files changed, 77 insertions(+), 36 deletions(-) >> >> -- >> 2.8.1 >> >-------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20160609/336dc7c4/attachment.sig>
On Thu, Jun 9, 2016 at 2:50 AM, Daniel Vetter <daniel at ffwll.ch> wrote:> On Wed, Jun 08, 2016 at 06:47:27PM +0200, Lukas Wunner wrote: >> Second iteration of my endeavour to rid nouveau, radeon and amdgpu of >> runtime pm ref leaks. >> >> Patches 1 to 8 are identical to v1. >> >> Patch 9 of v1 modified the DRM core to turn off all CRTCs on driver >> unload. Based on feedback by Daniel Vetter, I've replaced this with >> a helper to turn off all CRTCs, which is called by nouveau, radeon >> and amdgpu on unload. In other words, this is now opt-in. >> So patch 9 of v1 is replaced with new patches 9 to 12. >> >> A by-product of patch 9 is a helper which turns off a *single* CRTC. >> This is open coded in three other places in the DRM tree and patches >> 13 to 15 refactor those to use the new helper. > > Yeah I think this makes much more sense. Please poke amd/nouveau folks for > reviews/acks, then I can merge.The amdgpu, radeon, and drm patches are: Acked-by: Alex Deucher <alexander.deucher at amd.com>> -Daniel > >> >> To ease reviewing, I've pushed this series to GitHub: >> https://github.com/l1k/linux/commits/drm_runpm_fixes_v2 >> >> The discussion on v1 is archived here: >> https://lists.freedesktop.org/archives/dri-devel/2016-May/thread.html#108278 >> >> Thanks, >> >> Lukas >> >> Lukas Wunner (15): >> 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: Add helpers to turn off CRTCs >> drm/nouveau: Turn off CRTCs on driver unload >> drm/radeon: Turn off CRTCs on driver unload >> drm/amdgpu: Turn off CRTCs on driver unload >> drm: Use helper to turn off CRTC >> drm/i2c/ch7006: Use helper to turn off CRTC >> drm/nouveau/dispnv04: Use helper to turn off CRTC >> >> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 + >> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 12 +++++-- >> drivers/gpu/drm/drm_crtc.c | 53 ++++++++++++++++++++++++++---- >> drivers/gpu/drm/i2c/ch7006_drv.c | 9 ++--- >> drivers/gpu/drm/nouveau/dispnv04/disp.c | 10 ------ >> drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 9 ++--- >> drivers/gpu/drm/nouveau/nouveau_display.c | 1 + >> drivers/gpu/drm/nouveau/nouveau_drm.c | 6 +++- >> drivers/gpu/drm/radeon/radeon_device.c | 4 +++ >> drivers/gpu/drm/radeon/radeon_display.c | 1 + >> drivers/gpu/drm/radeon/radeon_kms.c | 5 ++- >> include/drm/drm_crtc.h | 2 ++ >> 12 files changed, 77 insertions(+), 36 deletions(-) >> >> -- >> 2.8.1 >> > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel