Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941058AbcJXOwI (ORCPT ); Mon, 24 Oct 2016 10:52:08 -0400 Received: from foss.arm.com ([217.140.101.70]:54372 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753513AbcJXOwG (ORCPT ); Mon, 24 Oct 2016 10:52:06 -0400 Date: Mon, 24 Oct 2016 15:52:03 +0100 From: Brian Starkey To: Daniel Vetter Cc: rmk+kernel@armlinux.org.uk, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration Message-ID: <20161024145202.GB1988@e106950-lin.cambridge.arm.com> References: <20161024142312.GA1988@e106950-lin.cambridge.arm.com> <1477319279-21726-1-git-send-email-brian.starkey@arm.com> <20161024143627.GT20761@phenom.ffwll.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20161024143627.GT20761@phenom.ffwll.local> 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 Content-Length: 3939 Lines: 114 Hi Daniel, On Mon, Oct 24, 2016 at 04:36:27PM +0200, Daniel Vetter wrote: >On Mon, Oct 24, 2016 at 03:27:59PM +0100, Brian Starkey wrote: >> Connectors shouldn't be registered until the rest of the whole device >> is set up, so that consistent state is presented to userspace. >> >> As such, remove the calls to drm_connector_register() and >> drm_connector_unregister() from tda998x, as these are now handled by >> drm_dev_(un)register() itself. >> >> To work with this change, the mali-dp and hdlcd bind and unbind >> sequences have to be reordered, to ensure that the componentised >> encoder/connector is bound before drm_dev_register() registers all >> connectors. Similarly, the device must be unregistered before the >> component is unbound. >> >> Altogether, this allows other drivers using tda998x to be >> de-midlayered, and to have less racy initialisation of their components. >> >> Splitting this commit into three (one per driver) isn't possible without >> intermediate breakage, so it is all squashed together here. >> >> Suggested-by: Russell King >> Signed-off-by: Brian Starkey >> Reviewed-by: Liviu Dudau > >> 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. Cheers, -Brian >-Daniel > >> drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder); >> >> return 0; >> >> -err_sysfs: >> - drm_connector_cleanup(&priv->connector); >> err_connector: >> drm_encoder_cleanup(&priv->encoder); >> err_encoder: >> @@ -1463,7 +1456,6 @@ static void tda998x_unbind(struct device *dev, struct device *master, >> { >> struct tda998x_priv *priv = dev_get_drvdata(dev); >> >> - drm_connector_unregister(&priv->connector); >> drm_connector_cleanup(&priv->connector); >> drm_encoder_cleanup(&priv->encoder); >> tda998x_destroy(priv); >> -- >> 1.7.9.5 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > >-- >Daniel Vetter >Software Engineer, Intel Corporation >http://blog.ffwll.ch >