From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1034947AbcIWOm4 (ORCPT ); Fri, 23 Sep 2016 10:42:56 -0400 Received: from foss.arm.com ([217.140.101.70]:33372 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1034918AbcIWOmz (ORCPT ); Fri, 23 Sep 2016 10:42:55 -0400 Date: Fri, 23 Sep 2016 15:42:52 +0100 From: Brian Starkey To: Liviu Dudau , Sean Paul , Russell King - ARM Linux , Linux Kernel Mailing List , dri-devel Subject: Re: [PATCH] drm/i2c: tda998x: don't register the connector Message-ID: <20160923144252.GB23321@e106950-lin.cambridge.arm.com> References: <20160921162803.GS1041@n2100.armlinux.org.uk> <20160922103917.GA5632@e106950-lin.cambridge.arm.com> <20160922105155.GT1041@n2100.armlinux.org.uk> <20160922141404.GA5222@e106950-lin.cambridge.arm.com> <20160923093416.GU8917@e106497-lin.cambridge.arm.com> <20160923125248.GA23321@e106950-lin.cambridge.arm.com> <20160923131315.GI3988@dvetter-linux.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20160923131315.GI3988@dvetter-linux.ger.corp.intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 >