From: Brian Starkey <brian.starkey@arm.com> To: Liviu Dudau <Liviu.Dudau@arm.com>, Sean Paul <seanpaul@chromium.org>, Russell King - ARM Linux <linux@armlinux.org.uk>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org> Subject: Re: [PATCH] drm/i2c: tda998x: don't register the connector Date: Fri, 23 Sep 2016 15:42:52 +0100 [thread overview] Message-ID: <20160923144252.GB23321@e106950-lin.cambridge.arm.com> (raw) In-Reply-To: <20160923131315.GI3988@dvetter-linux.ger.corp.intel.com> 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@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 >
WARNING: multiple messages have this Message-ID (diff)
From: Brian Starkey <brian.starkey@arm.com> To: Liviu Dudau <Liviu.Dudau@arm.com>, Sean Paul <seanpaul@chromium.org>, Russell King - ARM Linux <linux@armlinux.org.uk>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org> Subject: Re: [PATCH] drm/i2c: tda998x: don't register the connector Date: Fri, 23 Sep 2016 15:42:52 +0100 [thread overview] Message-ID: <20160923144252.GB23321@e106950-lin.cambridge.arm.com> (raw) In-Reply-To: <20160923131315.GI3988@dvetter-linux.ger.corp.intel.com> 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@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 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2016-09-23 14:42 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-07-25 10:55 [PATCH] drm/i2c: tda998x: don't register the connector Brian Starkey 2016-07-25 10:55 ` Brian Starkey 2016-07-25 12:25 ` Russell King - ARM Linux 2016-07-25 12:54 ` Brian Starkey 2016-07-25 12:54 ` Brian Starkey 2016-07-25 15:08 ` Daniel Vetter 2016-07-25 15:08 ` Daniel Vetter 2016-08-08 16:04 ` Brian Starkey 2016-08-08 16:04 ` Brian Starkey 2016-08-09 6:07 ` Daniel Vetter 2016-08-09 6:07 ` Daniel Vetter 2016-08-09 22:48 ` Russell King - ARM Linux 2016-09-21 8:57 ` Brian Starkey 2016-09-21 16:28 ` Russell King - ARM Linux 2016-09-22 10:39 ` Brian Starkey 2016-09-22 10:51 ` Russell King - ARM Linux 2016-09-22 11:22 ` Sean Paul 2016-09-22 11:28 ` Daniel Vetter 2016-09-22 13:38 ` Brian Starkey 2016-09-22 13:38 ` Brian Starkey 2016-09-22 14:24 ` Russell King - ARM Linux 2016-09-22 12:09 ` Russell King - ARM Linux 2016-09-22 12:32 ` Sean Paul 2016-09-22 12:40 ` Russell King - ARM Linux 2016-09-23 7:07 ` Daniel Vetter 2016-09-23 7:07 ` Daniel Vetter 2016-09-22 14:14 ` Brian Starkey 2016-09-22 14:14 ` Brian Starkey 2016-09-23 7:05 ` Daniel Vetter 2016-09-23 7:05 ` Daniel Vetter 2016-09-23 7:18 ` [PATCH] Revert "drm/i2c: tda998x: don't register the connector" Sean Paul 2016-09-23 9:32 ` liviu.dudau 2016-09-23 11:05 ` Sean Paul 2016-09-23 9:34 ` [PATCH] drm/i2c: tda998x: don't register the connector Liviu Dudau 2016-09-23 10:58 ` Daniel Vetter 2016-09-23 10:58 ` Daniel Vetter 2016-09-23 12:22 ` Lucas Stach 2016-09-23 12:52 ` Brian Starkey 2016-09-23 12:52 ` Brian Starkey 2016-09-23 13:13 ` Daniel Vetter 2016-09-23 13:13 ` Daniel Vetter 2016-09-23 14:05 ` Russell King - ARM Linux 2016-09-25 20:34 ` Daniel Vetter 2016-09-25 20:34 ` Daniel Vetter 2016-09-23 14:42 ` Brian Starkey [this message] 2016-09-23 14:42 ` Brian Starkey 2016-09-25 20:38 ` Daniel Vetter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160923144252.GB23321@e106950-lin.cambridge.arm.com \ --to=brian.starkey@arm.com \ --cc=Liviu.Dudau@arm.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=seanpaul@chromium.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.