Re: [PATCH] drm/dp_mst: Add ddc i2c device links for DP MST connectors

[Sam McNally <sammc@chromium.org>]

Thanks for the feedback.

On Sat, 15 Aug 2020 at 01:00, Imre Deak <imre.deak@intel.com> wrote:
>
> On Wed, Jul 29, 2020 at 04:15:28PM +1000, Sam McNally wrote:
> > As of commit d8bd15b37d32 ("drm/dp_mst: Fix the DDC I2C device
> > registration of an MST port"), DP MST DDC I2C devices are consistently
> > parented to the underlying DRM device, making it challenging to
> > associate the ddc i2c device with its connector from userspace.
>
> I can't see how was it less challenging before the commit. There is no
> guarantee for a CSN message which was the only way for the i2c device to
> get reparented to the connector.
>

Yes, that's true - the state before and after are both unable to
support ddc reliable i2c device discovery, and consistency is better.
The challenging part certainly is an ongoing affair - I wasn't
intending to blame it on that commit, though it has come out that way,
unfortunately. Looking at it now, that paragraph doesn't need to
reference any commits in particular; I'll rewrite it for the next
version.

> > Given the need for further refactoring before the i2c devices can be
> > parented to their connectors, in the meantime follow the pattern of
> > commit e1a29c6c5955 ("drm: Add ddc link in sysfs created by
> > drm_connector"), creating sysfs ddc links to the associated i2c device
> > for MST DP connectors.
> >
> > If the connector is created and registered before the i2c device, create
> > the link when registering the i2c device; otherwise, create the link
> > during late connector registration.
> >
> > Signed-off-by: Sam McNally <sammc@chromium.org>
> > ---
> >
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 29 +++++++++++++++++++++++++--
> >  1 file changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 1ac874e4e7a1..73a2299c2faa 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2161,11 +2161,23 @@ static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
> >  int drm_dp_mst_connector_late_register(struct drm_connector *connector,
> >                                      struct drm_dp_mst_port *port)
> >  {
> > +     int ret;
> >       DRM_DEBUG_KMS("registering %s remote bus for %s\n",
> >                     port->aux.name, connector->kdev->kobj.name);
> >
> >       port->aux.dev = connector->kdev;
> > -     return drm_dp_aux_register_devnode(&port->aux);
> > +     ret = drm_dp_aux_register_devnode(&port->aux);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (port->pdt != DP_PEER_DEVICE_NONE &&
> > +         drm_dp_mst_is_end_device(port->pdt, port->mcs)) {
>
> How can we get here when drm_dp_mst_is_end_device(port) is not true?
> AFAICS that's only case where we should create a connector and an i2c
> device. (IOW we don't create them for branch ports.)
>

I'm not sure what you mean. Wouldn't this condition be checked during
the registration of any MST connector? This follows the pattern used
in drm_dp_mst_port_add_connector() [0], which seems like it's invoked
in the same cases as drm_dp_mst_connector_late_register(), modulo
early outs for errors.

[0] https://cgit.freedesktop.org/drm-tip/tree/drivers/gpu/drm/drm_dp_mst_topology.c?id=1939e049a8ec6cef03a098f7cc99cb0bbcff21c6#n2188



> > +             ret = sysfs_create_link(&port->connector->kdev->kobj,
> > +                                     &port->aux.ddc.dev.kobj, "ddc");
> > +             if (ret)
> > +                     drm_dp_aux_unregister_devnode(&port->aux);
> > +     }
> > +     return ret;
> >  }
> >  EXPORT_SYMBOL(drm_dp_mst_connector_late_register);
> >
> > @@ -5490,6 +5502,7 @@ static int drm_dp_mst_register_i2c_bus(struct drm_dp_mst_port *port)
> >  {
> >       struct drm_dp_aux *aux = &port->aux;
> >       struct device *parent_dev = port->mgr->dev->dev;
> > +     int ret;
> >
> >       aux->ddc.algo = &drm_dp_mst_i2c_algo;
> >       aux->ddc.algo_data = aux;
> > @@ -5504,7 +5517,17 @@ static int drm_dp_mst_register_i2c_bus(struct drm_dp_mst_port *port)
> >       strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(parent_dev),
> >               sizeof(aux->ddc.name));
> >
> > -     return i2c_add_adapter(&aux->ddc);
> > +     ret = i2c_add_adapter(&aux->ddc);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (port->connector && port->connector->kdev) {
> > +             ret = sysfs_create_link(&port->connector->kdev->kobj,
> > +                                     &port->aux.ddc.dev.kobj, "ddc");
> > +             if (ret)
> > +                     i2c_del_adapter(&port->aux.ddc);
> > +     }
> > +     return ret;
> >  }
> >
> >  /**
> > @@ -5513,6 +5536,8 @@ static int drm_dp_mst_register_i2c_bus(struct drm_dp_mst_port *port)
> >   */
> >  static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_mst_port *port)
> >  {
> > +     if (port->connector && port->connector->kdev)
> > +             sysfs_remove_link(&port->connector->kdev->kobj, "ddc");
> >       i2c_del_adapter(&port->aux.ddc);
> >  }
> >
> > --
> > 2.28.0.rc0.142.g3c755180ce-goog
> >