On Wed, Jun 01, 2016 at 02:36:41PM +0200, Lukas Wunner wrote:
On Wed, May 25, 2016 at 03:43:42PM +0200, Daniel Vetter wrote:
On Wed, May 25, 2016 at 12:51 PM, Lukas Wunner lukas@wunner.de wrote:
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.
With legacy kms the only way to keep a crtc enabled is to display a drm_framebuffer on it. And drm_mode_config_cleanup has a WARN_ON if framebuffers are left behind. There's a bunch of options:
- nouveau somehow manages to keep the crtc on without a framebuffer
- nouveau somehow leaks a drm_framebuffer, but removes it from the fb_list
- something else
Found it. nouveau_fbcon_destroy() doesn't call drm_framebuffer_remove(). If I add that, the crtc gets properly disabled on unload.
It does call drm_framebuffer_cleanup(). That's why there was no WARN, drm_mode_config_cleanup() only WARNs if a framebuffer was left on the mode_config.fb_list.
radeon and amdgpu have the same problem. In fact there are very few drivers that call drm_framebuffer_remove(): tegra, msm, exynos, omapdrm and i915 (since Imre Deak's 9d6612516da0).
Should we add a WARN to prevent this? How about WARN_ON(crtc->enabled) in drm_crtc_cleanup()?
Also, i915 calls drm_framebuffer_unregister_private() before it calls drm_framebuffer_remove(). This ordering has the unfortunate side effect that the drm_framebuffer has ID 0 in log messages emitted by drm_framebuffer_remove():
[ 39.680874] [drm:drm_mode_object_unreference] OBJ ID: 0 (3) [ 39.680878] [drm:drm_mode_object_unreference] OBJ ID: 0 (2) [ 39.680884] [drm:drm_mode_object_unreference] OBJ ID: 0 (1)
Well we must first unregister it before we can remove it, so this is unavoidable.
Wrt switching from _cleanup to _remove, iirc there was troubles with the later calling into the fb->funcs->destroy hook. But many drivers have their fbdev fb embedded into some struct (instead of a pointer like i915), and then things go sideways badly. That's why you can't just blindly replace them. -Daniel