* Re: [PATCH 9/9] drm: Turn off crtc before tearing down its data structure [not found] ` <CAKMK7uGFb9ihRtjeK7s0ezPPv-C6S9GKbE4h9MLoPyHyN=9W5Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2016-06-01 12:36 ` Lukas Wunner [not found] ` <20160601123641.GA15243-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> 0 siblings, 1 reply; 5+ messages in thread From: Lukas Wunner @ 2016-06-01 12:36 UTC (permalink / raw) To: Daniel Vetter Cc: Alex Deucher, Nouveau Dev, intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, dri-devel, Dave Airlie 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) 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 _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <20160601123641.GA15243-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>]
* Re: [PATCH 9/9] drm: Turn off crtc before tearing down its data structure [not found] ` <20160601123641.GA15243-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> @ 2016-06-01 14:40 ` Daniel Vetter 2016-06-03 7:30 ` [Nouveau] " Lukas Wunner 0 siblings, 1 reply; 5+ messages in thread From: Daniel Vetter @ 2016-06-01 14:40 UTC (permalink / raw) To: Lukas Wunner Cc: Nouveau Dev, intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, dri-devel, Daniel Vetter, Alex Deucher, Dave Airlie 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Nouveau] [PATCH 9/9] drm: Turn off crtc before tearing down its data structure 2016-06-01 14:40 ` Daniel Vetter @ 2016-06-03 7:30 ` Lukas Wunner 2016-06-03 18:21 ` Daniel Vetter 0 siblings, 1 reply; 5+ messages in thread From: Lukas Wunner @ 2016-06-03 7:30 UTC (permalink / raw) To: Daniel Vetter Cc: Alex Deucher, Nouveau Dev, intel-gfx, dri-devel, Dave Airlie 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@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> > > > > > > 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? Thanks, Lukas _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Nouveau] [PATCH 9/9] drm: Turn off crtc before tearing down its data structure 2016-06-03 7:30 ` [Nouveau] " Lukas Wunner @ 2016-06-03 18:21 ` Daniel Vetter 2016-06-08 16:55 ` Lukas Wunner 0 siblings, 1 reply; 5+ messages in thread From: Daniel Vetter @ 2016-06-03 18:21 UTC (permalink / raw) To: Lukas Wunner; +Cc: Nouveau Dev, intel-gfx, dri-devel, Alex Deucher, Dave Airlie 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@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> > > > > > > > > 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. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Nouveau] [PATCH 9/9] drm: Turn off crtc before tearing down its data structure 2016-06-03 18:21 ` Daniel Vetter @ 2016-06-08 16:55 ` Lukas Wunner 0 siblings, 0 replies; 5+ messages in thread From: Lukas Wunner @ 2016-06-08 16:55 UTC (permalink / raw) To: Daniel Vetter Cc: Alex Deucher, Nouveau Dev, intel-gfx, dri-devel, Dave Airlie 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@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> > > > > > > > > > > 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-06-08 16:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1464103767.git.lukas@wunner.de>
[not found] ` <e83cf2e628a8e0299e029e7a1c3d4f183ce1a2af.1464103767.git.lukas@wunner.de>
[not found] ` <20160524213042.GC27098@phenom.ffwll.local>
[not found] ` <20160525105136.GA6235@wunner.de>
[not found] ` <CAKMK7uGFb9ihRtjeK7s0ezPPv-C6S9GKbE4h9MLoPyHyN=9W5Q@mail.gmail.com>
[not found] ` <CAKMK7uGFb9ihRtjeK7s0ezPPv-C6S9GKbE4h9MLoPyHyN=9W5Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-06-01 12:36 ` [PATCH 9/9] drm: Turn off crtc before tearing down its data structure Lukas Wunner
[not found] ` <20160601123641.GA15243-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
2016-06-01 14:40 ` Daniel Vetter
2016-06-03 7:30 ` [Nouveau] " Lukas Wunner
2016-06-03 18:21 ` Daniel Vetter
2016-06-08 16:55 ` Lukas Wunner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox