From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933047AbcKHJ0B (ORCPT ); Tue, 8 Nov 2016 04:26:01 -0500 Received: from mail-wm0-f66.google.com ([74.125.82.66]:36738 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932256AbcKHJZ4 (ORCPT ); Tue, 8 Nov 2016 04:25:56 -0500 Date: Tue, 8 Nov 2016 10:25:52 +0100 From: Daniel Vetter To: Russell King - ARM Linux Cc: Daniel Vetter , Brian Starkey , dri-devel , Linux Kernel Mailing List Subject: Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration Message-ID: <20161108092552.6qnum32uhndnsdou@phenom.ffwll.local> Mail-Followup-To: Russell King - ARM Linux , Brian Starkey , dri-devel , Linux Kernel Mailing List 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> <20161031085843.GU1041@n2100.armlinux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161031085843.GU1041@n2100.armlinux.org.uk> X-Operating-System: Linux phenom 4.6.0-1-amd64 User-Agent: NeoMutt/20161014 (1.7.1) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 31, 2016 at 08:58:43AM +0000, Russell King - ARM Linux wrote: > 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. Hm, I entirely missed that part of the troubles. Anyway, if you all agree on a patch I certainly won't block it, feel free to merge through suitable trees (or I can smash it into drm-misc if that's wanted). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch