Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1507243 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration Brian Starkey <brian.starkey@arm.com> - 2016-10-24 16:30 +0200
    Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector  registration Daniel Vetter <daniel@ffwll.ch> - 2016-10-24 16:40 +0200
      Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector  registration Brian Starkey <brian.starkey@arm.com> - 2016-10-24 17:00 +0200
        Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration Daniel Vetter <daniel@ffwll.ch> - 2016-10-24 22:30 +0200
          Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector  registration Brian Starkey <brian.starkey@arm.com> - 2016-10-25 12:00 +0200
            Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector  registration Daniel Vetter <daniel@ffwll.ch> - 2016-10-25 12:20 +0200
              Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector  registration Brian Starkey <brian.starkey@arm.com> - 2016-10-25 12:50 +0200

#1507243 — [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

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@arm.linux.org.uk>
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/gpu/drm/arm/hdlcd_drv.c   |   19 +++++++++++--------
 drivers/gpu/drm/arm/malidp_drv.c  |   18 +++++++++++-------
 drivers/gpu/drm/i2c/tda998x_drv.c |    8 --------
 3 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index d83b46a..85aec8b 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -337,14 +337,10 @@ static int hdlcd_drm_bind(struct device *dev)
 	if (ret)
 		goto err_free;
 
-	ret = drm_dev_register(drm, 0);
-	if (ret)
-		goto err_unload;
-
 	ret = component_bind_all(dev, drm);
 	if (ret) {
 		DRM_ERROR("Failed to bind all components\n");
-		goto err_unregister;
+		goto err_unload;
 	}
 
 	ret = pm_runtime_set_active(dev);
@@ -371,8 +367,17 @@ static int hdlcd_drm_bind(struct device *dev)
 		goto err_fbdev;
 	}
 
+	ret = drm_dev_register(drm, 0);
+	if (ret)
+		goto err_register;
+
 	return 0;
 
+err_register:
+	if (hdlcd->fbdev) {
+		drm_fbdev_cma_fini(hdlcd->fbdev);
+		hdlcd->fbdev = NULL;
+	}
 err_fbdev:
 	drm_kms_helper_poll_fini(drm);
 	drm_mode_config_cleanup(drm);
@@ -381,8 +386,6 @@ err_vblank:
 	pm_runtime_disable(drm->dev);
 err_pm_active:
 	component_unbind_all(dev, drm);
-err_unregister:
-	drm_dev_unregister(drm);
 err_unload:
 	drm_irq_uninstall(drm);
 	of_reserved_mem_device_release(drm->dev);
@@ -398,6 +401,7 @@ static void hdlcd_drm_unbind(struct device *dev)
 	struct drm_device *drm = dev_get_drvdata(dev);
 	struct hdlcd_drm_private *hdlcd = drm->dev_private;
 
+	drm_dev_unregister(drm);
 	if (hdlcd->fbdev) {
 		drm_fbdev_cma_fini(hdlcd->fbdev);
 		hdlcd->fbdev = NULL;
@@ -411,7 +415,6 @@ static void hdlcd_drm_unbind(struct device *dev)
 	pm_runtime_disable(drm->dev);
 	of_reserved_mem_device_release(drm->dev);
 	drm_mode_config_cleanup(drm);
-	drm_dev_unregister(drm);
 	drm_dev_unref(drm);
 	drm->dev_private = NULL;
 	dev_set_drvdata(dev, NULL);
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 82171d2..79bfc13 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -354,10 +354,6 @@ static int malidp_bind(struct device *dev)
 	if (ret < 0)
 		goto init_fail;
 
-	ret = drm_dev_register(drm, 0);
-	if (ret)
-		goto register_fail;
-
 	/* Set the CRTC's port so that the encoder component can find it */
 	ep = of_graph_get_next_endpoint(dev->of_node, NULL);
 	if (!ep) {
@@ -394,8 +390,18 @@ static int malidp_bind(struct device *dev)
 	}
 
 	drm_kms_helper_poll_init(drm);
+
+	ret = drm_dev_register(drm, 0);
+	if (ret)
+		goto register_fail;
+
 	return 0;
 
+register_fail:
+	if (malidp->fbdev) {
+		drm_fbdev_cma_fini(malidp->fbdev);
+		malidp->fbdev = NULL;
+	}
 fbdev_fail:
 	drm_vblank_cleanup(drm);
 vblank_fail:
@@ -407,8 +413,6 @@ bind_fail:
 	of_node_put(malidp->crtc.port);
 	malidp->crtc.port = NULL;
 port_fail:
-	drm_dev_unregister(drm);
-register_fail:
 	malidp_de_planes_destroy(drm);
 	drm_mode_config_cleanup(drm);
 init_fail:
@@ -431,6 +435,7 @@ static void malidp_unbind(struct device *dev)
 	struct malidp_drm *malidp = drm->dev_private;
 	struct malidp_hw_device *hwdev = malidp->dev;
 
+	drm_dev_unregister(drm);
 	if (malidp->fbdev) {
 		drm_fbdev_cma_fini(malidp->fbdev);
 		malidp->fbdev = NULL;
@@ -442,7 +447,6 @@ static void malidp_unbind(struct device *dev)
 	component_unbind_all(dev, drm);
 	of_node_put(malidp->crtc.port);
 	malidp->crtc.port = NULL;
-	drm_dev_unregister(drm);
 	malidp_de_planes_destroy(drm);
 	drm_mode_config_cleanup(drm);
 	drm->dev_private = NULL;
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;
-
 	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

[toc] | [next] | [standalone]

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

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@arm.linux.org.uk>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>

> 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.
-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

[toc] | [prev] | [next] | [standalone]

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

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@arm.linux.org.uk>
>> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>> Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
>
>> 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
>

[toc] | [prev] | [next] | [standalone]

#1507693

On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey <brian.starkey@arm.com> 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? 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 ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

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

Hi Daniel,

On Mon, Oct 24, 2016 at 10:24:42PM +0200, Daniel Vetter wrote:
>On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey <brian.starkey@arm.com> 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? Maybe even another driver, so the hack would still be useful
>for those other drivers.

OK it seems like this situation has got very confused. In short, this
patch does not break armada. Russell previously tested the tda998x
patch against armada and reported no issues.
Drivers with a ->load() callback don't need to register the connector
anymore, because drm_dev_register() does it for them.

As far as I know, this patch touching these three drivers is
sufficient to allow all current users of tda998x to continue using it,
whilst also allowing armada and tilcdc to be de-midlayered.

>And it would have been useful if malidp/hdlcd
>wouldn't have started out with the wrong init ordering ;-)

Yeah, believe me I know -_-
Hindsight is always 20/20 something something

Thanks,
-Brian

>-Daniel
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>+41 (0) 79 365 57 48 - http://blog.ffwll.ch
>

[toc] | [prev] | [next] | [standalone]

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

On Tue, Oct 25, 2016 at 10:52:33AM +0100, Brian Starkey wrote:
> Hi Daniel,
> 
> On Mon, Oct 24, 2016 at 10:24:42PM +0200, Daniel Vetter wrote:
> > On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey <brian.starkey@arm.com> 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? Maybe even another driver, so the hack would still be useful
> > for those other drivers.
> 
> OK it seems like this situation has got very confused. In short, this
> patch does not break armada. Russell previously tested the tda998x
> patch against armada and reported no issues.
> Drivers with a ->load() callback don't need to register the connector
> anymore, because drm_dev_register() does it for them.
> 
> As far as I know, this patch touching these three drivers is
> sufficient to allow all current users of tda998x to continue using it,
> whilst also allowing armada and tilcdc to be de-midlayered.

Ah, makes sense. Should I apply this to drm-misc so it's in a shared tree?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

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

On Tue, Oct 25, 2016 at 12:19:19PM +0200, Daniel Vetter wrote:
>On Tue, Oct 25, 2016 at 10:52:33AM +0100, Brian Starkey wrote:
>
>Ah, makes sense. Should I apply this to drm-misc so it's in a shared tree?

Honestly, I don't know. I didn't entirely follow what it was Russell
wanted in terms of getting this merged:

>On Sat, Oct 22, 2016 at 02:40:22PM +0100, Russell King - ARM Linux wrote:
>>So, what I would like to see is a single patch against Linus' 4.8.0
>>commit fixing mali-dp, hdlcd and any other driver, together with
>>removing drm_connector_register() from TDA998x.  This is so the patch
>>can be shared between all interested parties without forcing everyone
>>to 4.9-rc1.  Looking at the diff between 4.8 and 4.9-rc1 for
>>drivers/gpu/drm/arm, that shouldn't result in any merge conflicts -
>>and if you want to follow on from that with 4.9-rc1 development, you
>>can always merge 4.9-rc1 on top of that commit.

Looks like Jyri's series which depends on this is also dependent on
Russell's tree, so might be best to wait for him to get back online.

-Brian

>-Daniel
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web