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

[Lukas Wunner]

On Fri, Jun 03, 2016 at 08:21:50PM +0200, Daniel Vetter wrote:
> On Fri, Jun 03, 2016 at 09:30:06AM +0200, Lukas Wunner wrote:
> > On Wed, Jun 01, 2016 at 04:40:12PM +0200, Daniel Vetter wrote:
> > > 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 at 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 at redhat.com>
> > > > > > > > Tested-by: Karol Herbst <nouveau at karolherbst.de>
> > > > > > > > Signed-off-by: Lukas Wunner <lukas at wunner.de>
> > > > > 
> > > > > 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.
> > 
> > Yes but drm_framebuffer_free() calls drm_mode_object_unregister()
> > and is invoked by drm_framebuffer_remove(), so the additional call to
> > drm_framebuffer_unregister_private() in intel_fbdev_destroy() seems
> > superfluous. Or is there some reason I'm missing that this needs to
> > be called before intel_unpin_fb_obj()?
> > 
> > 
> > > 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.
> > 
> > So the options seem to be:
> > 
> > (1) Refactor nouveau, radeon and amdgpu to not embed their framebuffer
> >     struct in their fbdev struct, so that drm_framebuffer_remove() can
> >     be used.
> > 
> > (2) Amend each of them to turn off crtcs which are using the fbdev
> >     framebuffer, duplicating the code in drm_framebuffer_remove().
> > 
> > (3) Split drm_framebuffer_remove(), move the portion to turn off crtcs
> >     into a separate helper, say, drm_framebuffer_deactivate(), call that
> >     from nouveau, radeon and amdgpu.
> > 
> > (4) Go back to square one and use patch [9/9] of this series.
> > 
> > Which one would be most preferred? Is there another solution I've missed?
> 
> I think a dedicated turn_off_everything helper would be best. We'd need an
> atomic and a legacy version (because hooray), but that would work in all
> cases. Relying on the implicit behaviour to turn off everything (strictly
> speaking you only need to turn off all the planes, you can leave crtcs on,
> and that's what most atomic drivers want really under normal
> circumstances) is a bit fragile, and it's also possible to disable fbdev
> emulation. If you driver needs everything to be off in module unload, then
> it's imo best to explicitly enforce that.
> 
> So "(5) Write dedicated helper to turn off everything" is imo the right
> fix.

Okay I did that and just posted it as v2. Hope I've understood correctly
what you suggested, if not please let me know and I'll rectify in a v3.

Thanks,

Lukas