Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1490059

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

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: [PATCH] drm/i2c: tda998x: don't register the connector Brian Starkey <brian.starkey@arm.com> - 2016-09-21 11:00 +0200
  Re: [PATCH] drm/i2c: tda998x: don't register the connector Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-09-21 18:30 +0200
    Re: [PATCH] drm/i2c: tda998x: don't register the connector Brian Starkey <brian.starkey@arm.com> - 2016-09-22 12:40 +0200
      Re: [PATCH] drm/i2c: tda998x: don't register the connector Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-09-22 13:00 +0200
        Re: [PATCH] drm/i2c: tda998x: don't register the connector Sean Paul <seanpaul@chromium.org> - 2016-09-22 13:30 +0200
          Re: [PATCH] drm/i2c: tda998x: don't register the connector Daniel Vetter <daniel@ffwll.ch> - 2016-09-22 13:30 +0200
            Re: [PATCH] drm/i2c: tda998x: don't register the connector Brian Starkey <brian.starkey@arm.com> - 2016-09-22 15:40 +0200
              Re: [PATCH] drm/i2c: tda998x: don't register the connector Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-09-22 16:30 +0200
          Re: [PATCH] drm/i2c: tda998x: don't register the connector Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-09-22 14:20 +0200
            Re: [PATCH] drm/i2c: tda998x: don't register the connector Sean Paul <seanpaul@chromium.org> - 2016-09-22 14:40 +0200
              Re: [PATCH] drm/i2c: tda998x: don't register the connector Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-09-22 15:00 +0200
                Re: [PATCH] drm/i2c: tda998x: don't register the connector Daniel Vetter <daniel@ffwll.ch> - 2016-09-23 09:10 +0200
          Re: [PATCH] drm/i2c: tda998x: don't register the connector Brian Starkey <brian.starkey@arm.com> - 2016-09-22 16:20 +0200
            Re: [PATCH] drm/i2c: tda998x: don't register the connector Daniel Vetter <daniel@ffwll.ch> - 2016-09-23 09:10 +0200
              Re: [PATCH] drm/i2c: tda998x: don't register the connector Liviu Dudau <Liviu.Dudau@arm.com> - 2016-09-23 11:40 +0200
                Re: [PATCH] drm/i2c: tda998x: don't register the connector Daniel Vetter <daniel@ffwll.ch> - 2016-09-23 13:00 +0200
                Re: [PATCH] drm/i2c: tda998x: don't register the connector Lucas Stach <l.stach@pengutronix.de> - 2016-09-23 14:30 +0200
                Re: [PATCH] drm/i2c: tda998x: don't register the connector Brian Starkey <brian.starkey@arm.com> - 2016-09-23 15:00 +0200
                Re: [PATCH] drm/i2c: tda998x: don't register the connector Daniel Vetter <daniel@ffwll.ch> - 2016-09-23 15:20 +0200
                Re: [PATCH] drm/i2c: tda998x: don't register the connector Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-09-23 16:10 +0200
                Re: [PATCH] drm/i2c: tda998x: don't register the connector Daniel Vetter <daniel@ffwll.ch> - 2016-09-25 22:40 +0200
                Re: [PATCH] drm/i2c: tda998x: don't register the connector Brian Starkey <brian.starkey@arm.com> - 2016-09-23 16:50 +0200
                Re: [PATCH] drm/i2c: tda998x: don't register the connector Daniel Vetter <daniel@ffwll.ch> - 2016-09-25 22:40 +0200

csiph-web