Re: [PATCH] drm/i2c: tda998x: don't register the connector

[Brian Starkey]

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

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