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

[Daniel Vetter]

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