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

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>
>>
>> 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().
>
> 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

There's still no need to forcefully shut down crtc at cleanup time in
the core, this is still a driver bug. So yes your patch might be
needed, but it's not the right fix.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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>
> >>
> >> 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().
> >
> > 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)

Best regards,

Lukas
> 
> There's still no need to forcefully shut down crtc at cleanup time in
> the core, this is still a driver bug. So yes your patch might be
> needed, but it's not the right fix.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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>
> > >>
> > >> 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().
> > >
> > > 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch