On Fri, Sep 23, 2016 at 01:52:49PM +0100, Brian Starkey wrote: > On Fri, Sep 23, 2016 at 12:58:46PM +0200, Daniel Vetter wrote: > > On Fri, Sep 23, 2016 at 11:34 AM, Liviu Dudau <Liviu.Dudau@xxxxxxx> wrote: > > > rmmod-ing the hdlcd module generates a WARN() splat as the vsync is still > > > enabled, but we never got the call to turn off the CRTC. Brian is still > > > tracking through the fbdev emulation to figure out the cause for that. > > > > fbdev emulation doesn't do that for you. If you need/want to shut down > > all the crtcs on driver unload, you need to do that yourself. There's > > atomic helpers to do that for you that for you. > > The problem is a sort-of circular dependency between ->lastclose (at > least the common implementation of it), unregister and disabling > fbdev. > > I want to move drm_dev_unregister() to be the first thing we do at > rmmod-time. However we need to disable fbdev first, otherwise > ->lastclose restores the fbdev mode, guaranteeing that vsync is turned > on for drm_vsync_cleanup() to then WARN on. > > There's a slightly different (perceived) problem - the one that Liviu > mentions - that drm_fbdev_cma_fini() doesn't disable the CRTC anyway. > You say it's not the fbdev helpers' responsibility to teardown their > modeset, but regardless I have nowhere to disable the CRTC if I want > to do drm_dev_unregister() first; and if the CRTC isn't disabled > there's always a chance of hitting the same vsync WARN even without > fbdev. Just disable all crtc in a suitable place (after drm_dev_unregister, before you tear down fbdev). > > We *could* add an ->unload and disable everything there, but as that's > deprecated I'm guessing there should be another way. > Perhaps we should track ->firstopen/->lastclose pairs so we can detect > that ->lastclose is being called from unregister and use it to > disable everything in that case. Hm, maybe we should simply not call ->lastclose for kms drivers. That is kinda only a hack for ums/dri1 drivers. But even with that gone you might still unload while fbdev is enabled, so this won't fix it all. > drm_vblank_cleanup() seems to have been carried over to unregister > from drm_put_dev(), but drm_dev_register() doesn't call > drm_vblank_init() so it seems a little strange to have it there. > I can see other drivers I'd expect to hit the same WARN but I don't > have HW to test it on. Oops. That call to drm_vblank_cleanup() really shouldn't be in there. We should push it into all callers instead I think. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
