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(-)
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@redhat.com Cc: Ben Skeggs bskeggs@redhat.com Reported-by: Karol Herbst karolherbst@gmail.com Tested-by: Karol Herbst karolherbst@gmail.com Tested-by: Peter Wu peter@lekensteyn.nl Signed-off-by: Lukas Wunner lukas@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);
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@redhat.com Tested-by: Karol Herbst karolherbst@gmail.com Tested-by: Peter Wu peter@lekensteyn.nl Signed-off-by: Lukas Wunner lukas@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);
radeon_driver_load_kms() calls pm_runtime_put_autosuspend() if radeon_is_px(dev), but radeon_driver_unload_kms() calls pm_runtime_get_sync() unconditionally. We therefore leak a runtime pm ref whenever radeon is unloaded on a non-PX machine or if runpm=0. The GPU will subsequently never runtime suspend after loading radeon again.
Fix by taking the runtime pm ref under the same condition that it was released on driver load.
Fixes: 10ebc0bc0934 ("drm/radeon: add runtime PM support (v2)") Cc: Dave Airlie airlied@redhat.com Cc: Alex Deucher alexander.deucher@amd.com Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/radeon/radeon_kms.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index 414953c..51998a4 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -63,7 +63,9 @@ int radeon_driver_unload_kms(struct drm_device *dev) if (rdev->rmmio == NULL) goto done_free;
- pm_runtime_get_sync(dev->dev); + if (radeon_is_px(dev)) { + pm_runtime_get_sync(dev->dev); + }
radeon_kfd_device_fini(rdev);
radeon_device_init() returns an error if either of the two calls to radeon_init() fail. One level up in the call stack, radeon_driver_load_kms() will then skip runtime pm initialization and call radeon_driver_unload_kms(), which acquires a runtime pm ref that is leaked.
Balance by releasing a runtime pm ref in the error path of radeon_device_init().
Fixes: 10ebc0bc0934 ("drm/radeon: add runtime PM support (v2)") Cc: Dave Airlie airlied@redhat.com Cc: Alex Deucher alexander.deucher@amd.com Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/radeon/radeon_device.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index e721e6b..e0bf778 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -30,6 +30,7 @@ #include <drm/drmP.h> #include <drm/drm_crtc_helper.h> #include <drm/radeon_drm.h> +#include <linux/pm_runtime.h> #include <linux/vgaarb.h> #include <linux/vga_switcheroo.h> #include <linux/efi.h> @@ -1505,6 +1506,9 @@ int radeon_device_init(struct radeon_device *rdev, return 0;
failed: + /* balance pm_runtime_get_sync() in radeon_driver_unload_kms() */ + if (radeon_is_px(ddev)) + pm_runtime_put_noidle(ddev->dev); if (runtime) vga_switcheroo_fini_domain_pm_ops(rdev->dev); return r;
The PCI core calls pm_runtime_forbid() on device probe in pci_pm_init(), making this the default state when radeon is loaded. radeon_driver_load_kms() therefore calls pm_runtime_allow(), but there's no pm_runtime_forbid() in radeon_driver_unload_kms() 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 radeon is first loaded with runpm=1, then unloaded and loaded again with runpm=0, pm_runtime_forbid() will be called from radeon_pmops_runtime_idle() or radeon_pmops_runtime_suspend(), so the behaviour is correct. If there ever is a third party driver for AMD cards, this commit avoids that it has to clean up behind radeon.
Cc: Alex Deucher alexander.deucher@amd.com Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/radeon/radeon_kms.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index 51998a4..835563c 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -65,6 +65,7 @@ int radeon_driver_unload_kms(struct drm_device *dev)
if (radeon_is_px(dev)) { pm_runtime_get_sync(dev->dev); + pm_runtime_forbid(dev->dev); }
radeon_kfd_device_fini(rdev);
amdgpu_driver_load_kms() calls pm_runtime_put_autosuspend() if amdgpu_device_is_px(dev), but amdgpu_driver_unload_kms() calls pm_runtime_get_sync() unconditionally. We therefore leak a runtime pm ref whenever amdgpu is unloaded on a non-PX machine or if runpm=0. The GPU will subsequently never runtime suspend after loading amdgpu again.
Fix by taking the runtime pm ref under the same condition that it was released on driver load.
Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") Cc: Alex Deucher alexander.deucher@amd.com Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index 40a2370..9b1f979 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -60,7 +60,9 @@ int amdgpu_driver_unload_kms(struct drm_device *dev) if (adev->rmmio == NULL) goto done_free;
- pm_runtime_get_sync(dev->dev); + if (amdgpu_device_is_px(dev)) { + pm_runtime_get_sync(dev->dev); + }
amdgpu_amdkfd_device_fini(adev);
If an error occurs in amdgpu_device_init() after adev->rmmio has been set, its caller amdgpu_driver_load_kms() will skip runtime pm initialization and call amdgpu_driver_unload_kms(), which acquires a runtime pm ref that is leaked.
Balance by releasing a runtime pm ref in the error path of amdgpu_driver_load_kms().
Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") Cc: Alex Deucher alexander.deucher@amd.com Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index 9b1f979..0db692e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -137,9 +137,12 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags) }
out: - if (r) + if (r) { + /* balance pm_runtime_get_sync in amdgpu_driver_unload_kms */ + if (adev->rmmio && amdgpu_device_is_px(dev)) + pm_runtime_put_noidle(dev->dev); amdgpu_driver_unload_kms(dev); - + }
return r; }
The PCI core calls pm_runtime_forbid() on device probe in pci_pm_init(), making this the default state when amdgpu is loaded. amdgpu_driver_load_kms() therefore calls pm_runtime_allow(), but there's no pm_runtime_forbid() in amdgpu_driver_unload_kms() 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 amdgpu is first loaded with runpm=1, then unloaded and loaded again with runpm=0, pm_runtime_forbid() will be called from amdgpu_pmops_runtime_idle() or amdgpu_pmops_runtime_suspend(), so the behaviour is correct. If there ever is a third party driver for AMD cards, this commit avoids that it has to clean up behind amdgpu.
Cc: Alex Deucher alexander.deucher@amd.com Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index 0db692e..38a28d1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -62,6 +62,7 @@ int amdgpu_driver_unload_kms(struct drm_device *dev)
if (amdgpu_device_is_px(dev)) { pm_runtime_get_sync(dev->dev); + pm_runtime_forbid(dev->dev); }
amdgpu_amdkfd_device_fini(adev);
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@ffwll.ch Signed-off-by: Lukas Wunner lukas@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);
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@redhat.com Signed-off-by: Lukas Wunner lukas@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)
radeon leaks a runtime pm ref if at least one CRTC is enabled on unload. The ref is taken by radeon_crtc_set_config() and held as long as a CRTC is in use. Fix by turning off all CRTCs on unload.
Cc: Alex Deucher alexander.deucher@amd.com Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/radeon/radeon_display.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index 6a41b49..1922c96 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -1708,6 +1708,7 @@ void radeon_modeset_fini(struct radeon_device *rdev) radeon_afmt_fini(rdev); drm_kms_helper_poll_fini(rdev->ddev); radeon_hpd_fini(rdev); + drm_crtc_force_disable_all(rdev->ddev); drm_mode_config_cleanup(rdev->ddev); rdev->mode_info.mode_config_initialized = false; }
amdgpu leaks a runtime pm ref if at least one CRTC is enabled on unload. The ref is taken by amdgpu_crtc_set_config() and held as long as a CRTC is in use. Fix by turning off all CRTCs on unload.
Cc: Alex Deucher alexander.deucher@amd.com Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index bb8b149..2ab5e0b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -1624,6 +1624,7 @@ void amdgpu_device_fini(struct amdgpu_device *adev) amdgpu_bo_evict_vram(adev); amdgpu_ib_pool_fini(adev); amdgpu_fence_driver_fini(adev); + drm_crtc_force_disable_all(adev->ddev); amdgpu_fbdev_fini(adev); r = amdgpu_fini(adev); kfree(adev->ip_block_status);
Use shiny new drm_crtc_force_disable() instead of open coding the same. No functional change intended.
Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/drm_crtc.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 1edc1f4..3bd32a1 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -619,8 +619,6 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) struct drm_device *dev; struct drm_crtc *crtc; struct drm_plane *plane; - struct drm_mode_set set; - int ret;
if (!fb) return; @@ -650,11 +648,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) drm_for_each_crtc(crtc, dev) { if (crtc->primary->fb == fb) { /* should turn off the crtc */ - memset(&set, 0, sizeof(struct drm_mode_set)); - set.crtc = crtc; - set.fb = NULL; - ret = drm_mode_set_config_internal(&set); - if (ret) + if (drm_crtc_force_disable(crtc)) DRM_ERROR("failed to reset crtc %p when fb was deleted\n", crtc); } }
Use shiny new drm_crtc_force_disable() instead of open coding the same. No functional change intended.
Cc: Francisco Jerez currojerez@riseup.net Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/i2c/ch7006_drv.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i2c/ch7006_drv.c b/drivers/gpu/drm/i2c/ch7006_drv.c index 0594c45..e9e8ae2 100644 --- a/drivers/gpu/drm/i2c/ch7006_drv.c +++ b/drivers/gpu/drm/i2c/ch7006_drv.c @@ -361,13 +361,8 @@ static int ch7006_encoder_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;
Lukas Wunner lukas@wunner.de writes:
Use shiny new drm_crtc_force_disable() instead of open coding the same. No functional change intended.
Cc: Francisco Jerez currojerez@riseup.net Signed-off-by: Lukas Wunner lukas@wunner.de
Reviewed-by: Francisco Jerez currojerez@riseup.net
drivers/gpu/drm/i2c/ch7006_drv.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i2c/ch7006_drv.c b/drivers/gpu/drm/i2c/ch7006_drv.c index 0594c45..e9e8ae2 100644 --- a/drivers/gpu/drm/i2c/ch7006_drv.c +++ b/drivers/gpu/drm/i2c/ch7006_drv.c @@ -361,13 +361,8 @@ static int ch7006_encoder_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
Use shiny new drm_crtc_force_disable() instead of open coding the same. No functional change intended.
Cc: Ben Skeggs bskeggs@redhat.com Signed-off-by: Lukas Wunner lukas@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;
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
On Thu, Jun 9, 2016 at 2:50 AM, Daniel Vetter daniel@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@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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Jun 14, 2016 at 04:18:00PM -0400, Alex Deucher wrote:
On Thu, Jun 9, 2016 at 2:50 AM, Daniel Vetter daniel@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@amd.com
Great, thank you. That means all patches are either acked or reviewed:
* Patches 1, 2, 10 and 15 are Acked-by: Ben Skeggs bskeggs@redhat.com Message-ID: b4e3a8a6-cb44-80bf-8834-419afc2d1cb4@gmail.com Link: https://lists.freedesktop.org/archives/nouveau/2016-June/025350.html
* Patches 3 - 9 and 11 - 13 are Acked-by: Alex Deucher alexander.deucher@amd.com Message-ID: CADnq5_NENZukwJg-CACQ-djWsP4e299gnr4aWrEJkdcVMeaZWA@mail.gmail.com Link: https://lists.freedesktop.org/archives/dri-devel/2016-June/110876.html
* Patch 14 is Reviewed-by: Francisco Jerez currojerez@riseup.net Message-ID: 87ziqrtml5.fsf@riseup.net Link: https://lists.freedesktop.org/archives/dri-devel/2016-June/110588.html
Dave, Daniel, could one of you pick this up?
I've pushed another branch to GitHub which is amended with all the Acked-by and Reviewed-by tags. It's also rebased on latest drm-next. Otherwise this is identical to what I've posted. So in case you don't want to apply all tags manually, you can cherry-pick from or merge this branch (barring any objections of course): https://github.com/l1k/linux/commits/drm_runpm_fixes_v2_acked
Thanks everyone!
Lukas
On Wed, Jun 15, 2016 at 01:37:35PM +0200, Lukas Wunner wrote:
On Tue, Jun 14, 2016 at 04:18:00PM -0400, Alex Deucher wrote:
On Thu, Jun 9, 2016 at 2:50 AM, Daniel Vetter daniel@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@amd.com
Great, thank you. That means all patches are either acked or reviewed:
Patches 1, 2, 10 and 15 are Acked-by: Ben Skeggs bskeggs@redhat.com Message-ID: b4e3a8a6-cb44-80bf-8834-419afc2d1cb4@gmail.com Link: https://lists.freedesktop.org/archives/nouveau/2016-June/025350.html
Patches 3 - 9 and 11 - 13 are Acked-by: Alex Deucher alexander.deucher@amd.com Message-ID: CADnq5_NENZukwJg-CACQ-djWsP4e299gnr4aWrEJkdcVMeaZWA@mail.gmail.com Link: https://lists.freedesktop.org/archives/dri-devel/2016-June/110876.html
Patch 14 is Reviewed-by: Francisco Jerez currojerez@riseup.net Message-ID: 87ziqrtml5.fsf@riseup.net Link: https://lists.freedesktop.org/archives/dri-devel/2016-June/110588.html
Dave, Daniel, could one of you pick this up?
I've pushed another branch to GitHub which is amended with all the Acked-by and Reviewed-by tags. It's also rebased on latest drm-next. Otherwise this is identical to what I've posted. So in case you don't want to apply all tags manually, you can cherry-pick from or merge this branch (barring any objections of course): https://github.com/l1k/linux/commits/drm_runpm_fixes_v2_acked
Thanks everyone!
Yeah will pick up later this week, right now I have a big drm-misc pull that's pending. Would like to get that landed first. Please ping me if your patches haven't landed in drm-misc by next week.
Cheers, Daniel
On Wed, Jun 15, 2016 at 05:11:54PM +0200, Daniel Vetter wrote:
On Wed, Jun 15, 2016 at 01:37:35PM +0200, Lukas Wunner wrote:
On Tue, Jun 14, 2016 at 04:18:00PM -0400, Alex Deucher wrote:
On Thu, Jun 9, 2016 at 2:50 AM, Daniel Vetter daniel@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@amd.com
Great, thank you. That means all patches are either acked or reviewed:
Patches 1, 2, 10 and 15 are Acked-by: Ben Skeggs bskeggs@redhat.com Message-ID: b4e3a8a6-cb44-80bf-8834-419afc2d1cb4@gmail.com Link: https://lists.freedesktop.org/archives/nouveau/2016-June/025350.html
Patches 3 - 9 and 11 - 13 are Acked-by: Alex Deucher alexander.deucher@amd.com Message-ID: CADnq5_NENZukwJg-CACQ-djWsP4e299gnr4aWrEJkdcVMeaZWA@mail.gmail.com Link: https://lists.freedesktop.org/archives/dri-devel/2016-June/110876.html
Patch 14 is Reviewed-by: Francisco Jerez currojerez@riseup.net Message-ID: 87ziqrtml5.fsf@riseup.net Link: https://lists.freedesktop.org/archives/dri-devel/2016-June/110588.html
Dave, Daniel, could one of you pick this up?
I've pushed another branch to GitHub which is amended with all the Acked-by and Reviewed-by tags. It's also rebased on latest drm-next. Otherwise this is identical to what I've posted. So in case you don't want to apply all tags manually, you can cherry-pick from or merge this branch (barring any objections of course): https://github.com/l1k/linux/commits/drm_runpm_fixes_v2_acked
Thanks everyone!
Yeah will pick up later this week, right now I have a big drm-misc pull that's pending. Would like to get that landed first. Please ping me if your patches haven't landed in drm-misc by next week.
If you can be bothered to update your drm-misc pull: Ping! (Otherwise I think Dave or Thierry/Sumit could pick it up instead.)
Thanks and have a pleasant vacation,
Lukas
On Wed, Jun 22, 2016 at 2:44 PM, Lukas Wunner lukas@wunner.de wrote:
Yeah will pick up later this week, right now I have a big drm-misc pull that's pending. Would like to get that landed first. Please ping me if your patches haven't landed in drm-misc by next week.
If you can be bothered to update your drm-misc pull: Ping! (Otherwise I think Dave or Thierry/Sumit could pick it up instead.)
So much for my 5 second attention span :( I'll merge them right now, but I think it's better to let them soak a bit more. Maybe Thierry can send a pull request next week (if more patches show up). Note I'll wait with pushing out until Dave merged the previous pull, to avoid a mess in case another fixup is needed.
Thanks for reminding me. -Daniel
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Daniel Vetter -
Francisco Jerez -
Lukas Wunner