lkml.org 
[lkml]   [2016]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration
    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

    \
     
     \ /
      Last update: 2016-11-08 10:26    [W:4.774 / U:0.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site