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

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

[Sam McNally]

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.

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)) {
+		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

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

[Imre Deak]

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.

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

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

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

[Sam McNally]

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

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

[Imre Deak]

On Thu, Aug 20, 2020 at 12:27:03PM +1000, Sam McNally wrote:
> > > [...]
> > > 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.

Re-reading the code, a DRM connector is created whenever the MST port is
an output port, so even in the case of an output branch port.

I'm still not sure why we can't register/unregister the I2C bus whenever
creating/removing the DRM connector. That's also the scope of the AUX
bus, which is what I2C depends on, and if a port doesn't support I2C
messaging then the corresponding AUX messages would be NAKed.

--Imre

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

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

[Lyude Paul]

On Thu, 2020-08-20 at 21:03 +0300, Imre Deak wrote:
> On Thu, Aug 20, 2020 at 12:27:03PM +1000, Sam McNally wrote:
> > > > [...]
> > > > 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.
> 
> Re-reading the code, a DRM connector is created whenever the MST port is
> an output port, so even in the case of an output branch port.
> 
> I'm still not sure why we can't register/unregister the I2C bus whenever
> creating/removing the DRM connector. That's also the scope of the AUX
> bus, which is what I2C depends on, and if a port doesn't support I2C
> messaging then the corresponding AUX messages would be NAKed.

FWIW - I'm totally fine with this, as long as it works :)
> 
> --Imre
> 
> > [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
> > > > 
-- 
Sincerely,
      Lyude Paul (she/her)
      Software Engineer at Red Hat