On Fri, Sep 23, 2016 at 03:13:15PM +0200, Daniel Vetter wrote:
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.du...@arm.com> 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).
I think this is predicated on first removing the drm_vblank_cleanup
call.
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.
To be clear (and in response to Russell's question) - you mean
only the call to ->lastclose in drm_dev_unregister, not in general?
But even with that gone you might still unload while fbdev is
enabled, so
this won't fix it all.
Yeah it will be tidier, but I don't think it actually fixes anything.
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.
OK so two things to do - remove drm_vblank_cleanup() from
drm_dev_unregister(), and then do the teardown like so:
drm_dev_unregister();
drm_crtc_force_disable_all(); // or atomic equivalent
fbdev_teardown();
...
Seems good to me. Are there any ordering constraints you're aware of
for drm_vblank_cleanup()? Or you think just putting it after
drm_dev_unregister() should be OK?
Thanks,
-Brian
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
