Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Brian Starkey]

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 <rmk+kernel@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Brian Starkey <brian.starkey@xxxxxxx>
> > Reviewed-by: Liviu Dudau <Liviu.Dudau@xxxxxxx>
>
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch