Nouveau - May 2016 - [PATCH 9/9] drm: Turn off crtc before tearing down its data structure

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

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

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

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

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

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