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

Groups > linux.kernel > #1512413 > unrolled thread

Re: [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

  Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector  registration Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-10-31 10:00 +0100
    Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector  registration Daniel Vetter <daniel@ffwll.ch> - 2016-11-08 10:30 +0100
      Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector  registration Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-11-08 12:10 +0100
        Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector  registration Daniel Vetter <daniel@ffwll.ch> - 2016-11-08 12:30 +0100

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

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.

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

[toc] | [next] | [standalone]

#1516994

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

#1517083

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.

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

#1517096

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.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web