Thanks for the feedback.
On Sat, 15 Aug 2020 at 01:00, Imre Deak <imre.deak@xxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> >
> > 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
> >
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
