| Message ID | 1477319279-21726-1-git-send-email-brian.starkey@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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
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 >
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
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 >
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? 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.
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: > > 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? I need the patch against v4.8. I'm happy to pick it up and add it to my drm-tda998x-devel branch, which you can then merge into drm-misc if you wish.
On Mon, Oct 31, 2016 at 09:00:08AM +0000, Russell King - ARM Linux wrote: > I need the patch against v4.8. I'm happy to pick it up and add it > to my drm-tda998x-devel branch, which you can then merge into > drm-misc if you wish. ... or if Brian wants to send a git pull request to us with the patch based on v4.8, we can all merge that instead. Brian?
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 <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? > > 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
On Tue, Nov 08, 2016 at 10:25:52AM +0100, Daniel Vetter wrote: > 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). I think those who are interested in seeing the drm_connector_register() call disappear from tda998x only care about that happening, but not how it happens. We have agreement between myself, Brian and Liviu on this approach, and I think everyone else is waiting for me to push out the commit so it can be used as the basis for their work. I think everyone else is waiting for me to push something out which gets us past this log-jam. I don't understand the connectivity between drm-misc and David's drm tree - so I'm going to let you make the decision on whether to merge this into drm-misc. I normally send my pull requests for Armada and TDA998x changes to David, which means when I send my other TDA998x changes, the mali/tda998x commit will be included in that pull request too. So I'm wondering whether it would make more sense for me to send it to David instead, or whether I need to send my other changes through drm-misc instead. I find the whole drm vs drm-misc thing rather confusing. I think we should get this accepted into drm trees before anyone bases their work on this commit (which is why I've been holding off during the last week, waiting for DRM folk to get back from Santa Fe and readjust to the higher atmospheric pressure!) Anyway, here is my pull request for the mali/hdlcd/tda998x commit which I'd normally send to David - I don't mind which tree it goes into as long as things work out nicely. 8<=== David, Please incorporate the latest TDA998x I2C driver (drm-tda998x-mali branch), which can be found at: git://git.armlinux.org.uk/~rmk/linux-arm.git drm-tda998x-mali with SHA1 90731c24d2db7ec04df43ddbcee9605183d05187. This change removes the call to drm_connector_register() which has been blocking the proper de-midlayer conversion of other DRM drivers. Unfortunately, hdlcd and mali have intimate dependencies on this change, which is why these drivers need to be fixed up in the same commit - they can't be separate commits without these drivers breaking. All other DRM drivers which make use of tda998x (to my knowledge - armada, tilcdc) cope with this change. This will update the following files: 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(-) through these changes: Brian Starkey (1): drm/i2c: tda998x: mali-dp: hdlcd: refactor connector registration Many thanks.
On Tue, Nov 08, 2016 at 10:59:43AM +0000, Russell King - ARM Linux wrote: > On Tue, Nov 08, 2016 at 10:25:52AM +0100, Daniel Vetter wrote: > > 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). > > I think those who are interested in seeing the drm_connector_register() > call disappear from tda998x only care about that happening, but not how > it happens. > > We have agreement between myself, Brian and Liviu on this approach, and > I think everyone else is waiting for me to push out the commit so it can > be used as the basis for their work. I think everyone else is waiting > for me to push something out which gets us past this log-jam. > > I don't understand the connectivity between drm-misc and David's drm > tree - so I'm going to let you make the decision on whether to merge > this into drm-misc. I normally send my pull requests for Armada and > TDA998x changes to David, which means when I send my other TDA998x > changes, the mali/tda998x commit will be included in that pull > request too. So I'm wondering whether it would make more sense for > me to send it to David instead, or whether I need to send my other > changes through drm-misc instead. I find the whole drm vs drm-misc > thing rather confusing. > > I think we should get this accepted into drm trees before anyone bases > their work on this commit (which is why I've been holding off during > the last week, waiting for DRM folk to get back from Santa Fe and > readjust to the higher atmospheric pressure!) > > Anyway, here is my pull request for the mali/hdlcd/tda998x commit which > I'd normally send to David - I don't mind which tree it goes into as > long as things work out nicely. drm-misc is just the collector for when it doesn't make sense to have a driver or topic/feature pull request (or there isn't really a permanent driver tree). Pull to Dave directly makes sense. -Daniel > 8<=== > > David, > > Please incorporate the latest TDA998x I2C driver (drm-tda998x-mali > branch), which can be found at: > > git://git.armlinux.org.uk/~rmk/linux-arm.git drm-tda998x-mali > > with SHA1 90731c24d2db7ec04df43ddbcee9605183d05187. > > This change removes the call to drm_connector_register() which has been > blocking the proper de-midlayer conversion of other DRM drivers. > Unfortunately, hdlcd and mali have intimate dependencies on this change, > which is why these drivers need to be fixed up in the same commit - they > can't be separate commits without these drivers breaking. All other > DRM drivers which make use of tda998x (to my knowledge - armada, tilcdc) > cope with this change. > > This will update the following files: > > 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(-) > > through these changes: > > Brian Starkey (1): > drm/i2c: tda998x: mali-dp: hdlcd: refactor connector registration > > Many thanks. > > -- > 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.
I guess Dave must have missed this as I can't see it in drm-next, so I'm resending the pull request. On Tue, Nov 08, 2016 at 10:59:43AM +0000, Russell King - ARM Linux wrote: > On Tue, Nov 08, 2016 at 10:25:52AM +0100, Daniel Vetter wrote: > > 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). > > I think those who are interested in seeing the drm_connector_register() > call disappear from tda998x only care about that happening, but not how > it happens. > > We have agreement between myself, Brian and Liviu on this approach, and > I think everyone else is waiting for me to push out the commit so it can > be used as the basis for their work. I think everyone else is waiting > for me to push something out which gets us past this log-jam. > > I don't understand the connectivity between drm-misc and David's drm > tree - so I'm going to let you make the decision on whether to merge > this into drm-misc. I normally send my pull requests for Armada and > TDA998x changes to David, which means when I send my other TDA998x > changes, the mali/tda998x commit will be included in that pull > request too. So I'm wondering whether it would make more sense for > me to send it to David instead, or whether I need to send my other > changes through drm-misc instead. I find the whole drm vs drm-misc > thing rather confusing. > > I think we should get this accepted into drm trees before anyone bases > their work on this commit (which is why I've been holding off during > the last week, waiting for DRM folk to get back from Santa Fe and > readjust to the higher atmospheric pressure!) > > Anyway, here is my pull request for the mali/hdlcd/tda998x commit which > I'd normally send to David - I don't mind which tree it goes into as > long as things work out nicely. > > 8<=== > > David, > > Please incorporate the latest TDA998x I2C driver (drm-tda998x-mali > branch), which can be found at: > > git://git.armlinux.org.uk/~rmk/linux-arm.git drm-tda998x-mali > > with SHA1 90731c24d2db7ec04df43ddbcee9605183d05187. > > This change removes the call to drm_connector_register() which has been > blocking the proper de-midlayer conversion of other DRM drivers. > Unfortunately, hdlcd and mali have intimate dependencies on this change, > which is why these drivers need to be fixed up in the same commit - they > can't be separate commits without these drivers breaking. All other > DRM drivers which make use of tda998x (to my knowledge - armada, tilcdc) > cope with this change. > > This will update the following files: > > 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(-) > > through these changes: > > Brian Starkey (1): > drm/i2c: tda998x: mali-dp: hdlcd: refactor connector registration > > Many thanks. > > -- > 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.
Okay, I've queued this into my own for-next branch, along with the now reviewed and tested set of tda998x patches that I sent out for comment and testing. I'm still hopeful that Dave's going to pull the initial patch at some point... please? On Tue, Nov 15, 2016 at 09:46:31AM +0000, Russell King - ARM Linux wrote: > I guess Dave must have missed this as I can't see it in drm-next, so > I'm resending the pull request. > > On Tue, Nov 08, 2016 at 10:59:43AM +0000, Russell King - ARM Linux wrote: > > On Tue, Nov 08, 2016 at 10:25:52AM +0100, Daniel Vetter wrote: > > > 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). > > > > I think those who are interested in seeing the drm_connector_register() > > call disappear from tda998x only care about that happening, but not how > > it happens. > > > > We have agreement between myself, Brian and Liviu on this approach, and > > I think everyone else is waiting for me to push out the commit so it can > > be used as the basis for their work. I think everyone else is waiting > > for me to push something out which gets us past this log-jam. > > > > I don't understand the connectivity between drm-misc and David's drm > > tree - so I'm going to let you make the decision on whether to merge > > this into drm-misc. I normally send my pull requests for Armada and > > TDA998x changes to David, which means when I send my other TDA998x > > changes, the mali/tda998x commit will be included in that pull > > request too. So I'm wondering whether it would make more sense for > > me to send it to David instead, or whether I need to send my other > > changes through drm-misc instead. I find the whole drm vs drm-misc > > thing rather confusing. > > > > I think we should get this accepted into drm trees before anyone bases > > their work on this commit (which is why I've been holding off during > > the last week, waiting for DRM folk to get back from Santa Fe and > > readjust to the higher atmospheric pressure!) > > > > Anyway, here is my pull request for the mali/hdlcd/tda998x commit which > > I'd normally send to David - I don't mind which tree it goes into as > > long as things work out nicely. > > > > 8<=== > > > > David, > > > > Please incorporate the latest TDA998x I2C driver (drm-tda998x-mali > > branch), which can be found at: > > > > git://git.armlinux.org.uk/~rmk/linux-arm.git drm-tda998x-mali > > > > with SHA1 90731c24d2db7ec04df43ddbcee9605183d05187. > > > > This change removes the call to drm_connector_register() which has been > > blocking the proper de-midlayer conversion of other DRM drivers. > > Unfortunately, hdlcd and mali have intimate dependencies on this change, > > which is why these drivers need to be fixed up in the same commit - they > > can't be separate commits without these drivers breaking. All other > > DRM drivers which make use of tda998x (to my knowledge - armada, tilcdc) > > cope with this change. > > > > This will update the following files: > > > > 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(-) > > > > through these changes: > > > > Brian Starkey (1): > > drm/i2c: tda998x: mali-dp: hdlcd: refactor connector registration > > > > Many thanks. > > > > -- > > 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. > > -- > 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.
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);