Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1763415AbcJaI64 (ORCPT ); Mon, 31 Oct 2016 04:58:56 -0400 Received: from pandora.armlinux.org.uk ([78.32.30.218]:55458 "EHLO pandora.armlinux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753319AbcJaI6y (ORCPT ); Mon, 31 Oct 2016 04:58:54 -0400 Date: Mon, 31 Oct 2016 08:58:43 +0000 From: Russell King - ARM Linux To: Daniel Vetter Cc: Brian Starkey , dri-devel , Linux Kernel Mailing List Subject: Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration Message-ID: <20161031085843.GU1041@n2100.armlinux.org.uk> References: <20161024142312.GA1988@e106950-lin.cambridge.arm.com> <1477319279-21726-1-git-send-email-brian.starkey@arm.com> <20161024143627.GT20761@phenom.ffwll.local> <20161024145202.GB1988@e106950-lin.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3772 Lines: 94 On Mon, Oct 24, 2016 at 10:24:42PM +0200, Daniel Vetter wrote: > On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey wrote: > >> > >>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c > >>> b/drivers/gpu/drm/i2c/tda998x_drv.c > >>> index f4315bc..6e6fca2 100644 > >>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c > >>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > >>> @@ -1369,7 +1369,6 @@ const struct drm_connector_helper_funcs > >>> tda998x_connector_helper_funcs = { > >>> > >>> static void tda998x_connector_destroy(struct drm_connector *connector) > >>> { > >>> - drm_connector_unregister(connector); > >>> drm_connector_cleanup(connector); > >>> } > >>> > >>> @@ -1441,16 +1440,10 @@ static int tda998x_bind(struct device *dev, > >>> struct device *master, void *data) > >>> if (ret) > >>> goto err_connector; > >>> > >>> - ret = drm_connector_register(&priv->connector); > >>> - if (ret) > >>> - goto err_sysfs; > >>> - > >> > >> > >> Instead of smashing all these patches into one, what about checking here > >> for midlayer driver set with: > >> > >> /* register here for drivers still using midlayer load/unload */ > >> if (dev->driver->load) > >> drm_connector_register(connector), > >> > >> Similar in other places. That way we wouldn't need to switch the world in > >> one patch. > > > > > > I don't think that helps. If we do that in isolation (first), then > > mali-dp and hdlcd won't get their connectors registered because their > > bind order is: > > > > drm_dev_register(); > > component_bind_all(); > > > > If we change the mali-dp/hdlcd bind order first, then tda998x will > > explode on drm_connector_register() until it's patched to remove that. > > > > As I mentioned in my mail to Russell, the only way I can see to avoid > > patching all three drivers in one go is: > > 1) Add (probably open-coded) drm_connector_register_all() to the end > > of bind in hdlcd and mali-dp > > 2) Patch tda998x to remove drm_connector_register() > > 3) Reorder hdlcd/mali-dp bind and remove the connector registration > > added in 1) > > > > We can do that, but it's extra churn for the same result, and none of > > the 5 patches will really make sense in isolation anyway. > > I thought there's also armada to take care of, which this patch would > break? NO NO NO NO NO. I've said this several times. Let's try it again, and see if it sticks. Because Armada has not been converted from a mid-layered driver, it is _IMMUNE_ from any patch removing the drm_connector_register() call in TDA998x. It does _NOT_ break in any way. Only those drivers which are de-mid-layered, and worked around the drm_connector_register() call inside TDA998x (eg, mali) break, because of the order in which they are _forced_ to call stuff. In a de-mid-layered driver, with the drm_connector_register() call in place in TDA998x, drm_dev_register() _MUST_ be called prior to component_bind_all(), otherwise you get a WARN_ON() dump from the kobject code. With the drm_connector_register() call removed, drm_dev_register() _MUST_ be called after component_bind_all() so that the connector is registered. It's the de-mid-layered drivers which are the problem here, not the mid-layered ones like Armada. > Maybe even another driver, so the hack would still be useful > for those other drivers. And it would have been useful if malidp/hdlcd > wouldn't have started out with the wrong init ordering ;-) It's forced into the "wrong init ordering" due to the kobject WARN_ON. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.