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

From: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
To: Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org>
Cc: Alex Deucher <alexander.deucher-5C7GfCeVMHo@public.gmane.org>,
	nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
	Dave Airlie <airlied-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Subject: Re: [PATCH 9/9] drm: Turn off crtc before tearing down its data structure
Date: Wed, 25 May 2016 12:51:36 +0200	[thread overview]
Message-ID: <20160525105136.GA6235@wunner.de> (raw)
In-Reply-To: <20160524213042.GC27098-dv86pmgwkMBes7Z6vYuT8azUEOm+Xw19@public.gmane.org>

Hi Daniel,

On Tue, May 24, 2016 at 11:30:42PM +0200, Daniel Vetter wrote:
> 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@redhat.com>
> > Tested-by: Karol Herbst <nouveau@karolherbst.de>
> > Signed-off-by: Lukas Wunner <lukas@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@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().

I tested it and at least with nouveau, the above-mentioned commit does *not*
solve the issue, so patch [9/9] of this series is still needed. I do not get
a WARN splat when unloading nouveau.

Best regards,

Lukas

> 
> > ---
> >  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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/nouveau
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau