[RFC PATCH v1 00/12] drm,usb/typec: uABI for USB-C DisplayPort connectors

[RFC PATCH v1 00/12] drm,usb/typec: uABI for USB-C DisplayPort connectors

[Dmitry Baryshkov]

During the discussion regarding DisplayPort wrapped in the USB-C
connectors (via the USB-C altmode) it was pointed out that currently
there is no good way to let userspace know if the DRM connector in
question is the 'native' DP connector or if it is the USB-C connector.

An attempt to use DRM_MODE_CONNECTOR_USB for such connectors was
declined, as existing DP drivers (i915, AMD) use
DRM_MODE_CONNECTOR_DisplayPort. New drivers should behave in the same
way.

An attempt to use subconnector property was also declined. It is defined
to the type of the DP dongle connector rather than the host connector.

This attempt targets reusing the connector's PATH property. Currently
this property is only used for the DP MST connectors. This patchset
reuses it to point out to the corresponding registered typec port
device.

Dmitry Baryshkov (12):
  Revert "drm/sysfs: Link DRM connectors to corresponding Type-C
    connectors"
  drm/sysfs: link DRM connector device to the connector's fw nodes
  drm/connector: extend PATH property to covert Type-C case
  drm/bridge-connector: set the PATH property for the connector
  drm/bridge: remove conditionals around devicetree pointers
  soc: qcom: pmic_glink_altmode: fix DRM connector type
  soc: qcom: pmic_glink_altmode: report that this is a Type-C connector
  usb: typec: support generating Type-C port names for userspace
  usb: typec: tcpm: support generating Type-C port names for userspace
  usb: typec: qcom: implement proper error path in probe()
  usb: typec: qcom: extract DRM bridge functionality to separate file
  usb: typec: qcom: define the bridge's path

 drivers/gpu/drm/bridge/panel.c                |  2 -
 drivers/gpu/drm/drm_bridge_connector.c        | 14 ++++-
 drivers/gpu/drm/drm_connector.c               | 10 +++-
 drivers/gpu/drm/drm_sysfs.c                   | 42 +-------------
 drivers/soc/qcom/pmic_glink_altmode.c         |  3 +-
 drivers/usb/typec/class.c                     | 14 +++++
 drivers/usb/typec/tcpm/qcom/Makefile          |  4 ++
 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 56 ++++++-------------
 .../usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c | 40 +++++++++++++
 .../usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h | 22 ++++++++
 drivers/usb/typec/tcpm/tcpm.c                 | 14 +++++
 include/drm/drm_bridge.h                      |  9 ++-
 include/linux/usb/tcpm.h                      |  2 +
 include/linux/usb/typec.h                     |  2 +
 14 files changed, 146 insertions(+), 88 deletions(-)
 create mode 100644 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c
 create mode 100644 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h

-- 
2.39.2

[RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Dmitry Baryshkov]

The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
dev_fwnode() checks never succeed, making the respective commit NOP.

And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
breaks drivers already using components (as it was pointed at [1]),
resulting in a deadlock. Lockdep trace is provided below.

Granted these two issues, it seems impractical to fix this commit in any
sane way. Revert it instead.

[1] https://lore.kernel.org/dri-devel/Y24bcYJKGy%2Fgd5fV@phenom.ffwll.local/

============================================
WARNING: possible recursive locking detected
6.5.0-rc6-next-20230816-10542-g090e2ca9feae-dirty #713 Tainted: G        W
--------------------------------------------
kworker/u16:0/11 is trying to acquire lock:
ffffce0f54bea490 (component_mutex){+.+.}-{3:3}, at: __component_add+0x64/0x170

but task is already holding lock:
ffffce0f54bea490 (component_mutex){+.+.}-{3:3}, at: __component_add+0x64/0x170

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(component_mutex);
  lock(component_mutex);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

6 locks held by kworker/u16:0/11:
 #0: ffff5b7680008d38 ((wq_completion)events_unbound){+.+.}-{0:0}, at: process_one_work+0x14c/0x51c
 #1: ffff8000800abde0 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x14c/0x51c
 #2: ffff5b76837a2908 (&dev->mutex){....}-{3:3}, at: __device_attach+0x38/0x188
 #3: ffffce0f54bea490 (component_mutex){+.+.}-{3:3}, at: __component_add+0x64/0x170
 #4: ffffce0f54bdeb40 (drm_connector_list_iter){.+.+}-{0:0}, at: drm_modeset_register_all+0x80/0x9c
 #5: ffff5b76866ad0d0 (&connector->mutex){+.+.}-{3:3}, at: drm_connector_register.part.0+0x28/0x104

stack backtrace:
CPU: 6 PID: 11 Comm: kworker/u16:0 Tainted: G        W          6.5.0-rc6-next-20230816-10542-g090e2ca9feae-dirty #713
Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
Workqueue: events_unbound deferred_probe_work_func
Call trace:
 dump_backtrace+0x98/0xf0
 show_stack+0x18/0x24
 dump_stack_lvl+0x60/0xac
 dump_stack+0x18/0x24
 print_deadlock_bug+0x254/0x340
 __lock_acquire+0x105c/0x1ebc
 lock_acquire+0x1ec/0x314
 __lock_acquire+0x105c/0x1ebc
 lock_acquire+0x1ec/0x314
 __mutex_lock+0xa0/0x77c
 mutex_lock_nested+0x24/0x30
 __component_add+0x64/0x170
 component_add+0x14/0x20
 drm_sysfs_connector_add+0x144/0x1a0
 drm_connector_register.part.0+0x5c/0x104
 drm_connector_register_all+0x84/0x160
 drm_modeset_register_all+0x80/0x9c
 drm_dev_register+0x120/0x238
 msm_drm_bind+0x550/0x6e0
 try_to_bring_up_aggregate_device+0x164/0x1d0
 __component_add+0xa8/0x170
 component_add+0x14/0x20
 dsi_dev_attach+0x20/0x2c
 dsi_host_attach+0x9c/0x144
 devm_mipi_dsi_attach+0x34/0xb4
 lt9611uxc_attach_dsi.isra.0+0x84/0xfc
 lt9611uxc_probe+0x5ac/0x66c
 i2c_device_probe+0x148/0x290
 really_probe+0x148/0x2ac
 __driver_probe_device+0x78/0x12c
 driver_probe_device+0x3c/0x160
 __device_attach_driver+0xb8/0x138
 bus_for_each_drv+0x80/0xdc
 __device_attach+0x9c/0x188
 device_initial_probe+0x14/0x20
 bus_probe_device+0xac/0xb0
 deferred_probe_work_func+0x8c/0xc8
 process_one_work+0x1ec/0x51c
 worker_thread+0x1ec/0x3e4
 kthread+0x120/0x124
 ret_from_fork+0x10/0x20

Fixes: c5c51b242062 ("drm/sysfs: Link DRM connectors to corresponding Type-C connectors")
Cc: Won Chung <wonchung@google.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_sysfs.c | 40 -------------------------------------
 1 file changed, 40 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index b169b3e44a92..06662cc8d3f4 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -11,14 +11,12 @@
  */
 
 #include <linux/acpi.h>
-#include <linux/component.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/export.h>
 #include <linux/gfp.h>
 #include <linux/i2c.h>
 #include <linux/kdev_t.h>
-#include <linux/property.h>
 #include <linux/slab.h>
 
 #include <drm/drm_accel.h>
@@ -98,34 +96,6 @@ static char *drm_devnode(const struct device *dev, umode_t *mode)
 	return kasprintf(GFP_KERNEL, "dri/%s", dev_name(dev));
 }
 
-static int typec_connector_bind(struct device *dev,
-				struct device *typec_connector, void *data)
-{
-	int ret;
-
-	ret = sysfs_create_link(&dev->kobj, &typec_connector->kobj, "typec_connector");
-	if (ret)
-		return ret;
-
-	ret = sysfs_create_link(&typec_connector->kobj, &dev->kobj, "drm_connector");
-	if (ret)
-		sysfs_remove_link(&dev->kobj, "typec_connector");
-
-	return ret;
-}
-
-static void typec_connector_unbind(struct device *dev,
-				   struct device *typec_connector, void *data)
-{
-	sysfs_remove_link(&typec_connector->kobj, "drm_connector");
-	sysfs_remove_link(&dev->kobj, "typec_connector");
-}
-
-static const struct component_ops typec_connector_ops = {
-	.bind = typec_connector_bind,
-	.unbind = typec_connector_unbind,
-};
-
 static CLASS_ATTR_STRING(version, S_IRUGO, "drm 1.1.0 20060810");
 
 /**
@@ -394,16 +364,9 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
 
 	connector->kdev = kdev;
 
-	if (dev_fwnode(kdev)) {
-		r = component_add(kdev, &typec_connector_ops);
-		if (r)
-			drm_err(dev, "failed to add component to create link to typec connector\n");
-	}
-
 	if (connector->ddc)
 		return sysfs_create_link(&connector->kdev->kobj,
 				 &connector->ddc->dev.kobj, "ddc");
-
 	return 0;
 
 err_free:
@@ -419,9 +382,6 @@ void drm_sysfs_connector_remove(struct drm_connector *connector)
 	if (connector->ddc)
 		sysfs_remove_link(&connector->kdev->kobj, "ddc");
 
-	if (dev_fwnode(connector->kdev))
-		component_del(connector->kdev, &typec_connector_ops);
-
 	DRM_DEBUG("removing \"%s\" from sysfs\n",
 		  connector->name);
 
-- 
2.39.2

[RFC PATCH v1 02/12] drm/sysfs: link DRM connector device to the connector's fw nodes

[Dmitry Baryshkov]

Setup the of_node and fwnode fields for the DRM connector device,
making respective links to appear in /sys.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 06662cc8d3f4..cb926d95ef0e 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -345,6 +345,8 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
 	kdev->class = drm_class;
 	kdev->type = &drm_sysfs_device_connector;
 	kdev->parent = dev->primary->kdev;
+	kdev->of_node = to_of_node(connector->fwnode);
+	kdev->fwnode = connector->fwnode;
 	kdev->groups = connector_dev_groups;
 	kdev->release = drm_sysfs_release;
 	dev_set_drvdata(kdev, connector);
-- 
2.39.2

[RFC PATCH v1 03/12] drm/connector: extend PATH property to covert Type-C case

[Dmitry Baryshkov]

Userspace needs to identify whether the DisplayPort connector is a
native one or it is wrapped into the USB-C port. Moreover the userspace
might need to point user to the exact location of this Type-C port on
the laptop case.

Existing USB-C altmode implementations (i915, amdgpu) have used the
DRM_MODE_CONNECTOR_DisplayPort even for such ports. To keep backwards
compatibility we can not change this to DRM_MODE_CONNECTOR_USB.
Therefore the kernel needs some other way to represent this information.

To facilitate this, reuse the 'PATH' property, which was used to
describe the DP port in the DP MST configuration. Use either
'typec:portN' to point out to the Type-C port class device, or just
'typec:' if the corresponding port can not be identified.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_connector.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 05fc29a54801..a326782e936e 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1185,10 +1185,14 @@ static const u32 dp_colorspaces =
  * 	never read back the value of "DPMS" because it can be incorrect.
  * PATH:
  * 	Connector path property to identify how this sink is physically
- * 	connected. Used by DP MST. This should be set by calling
- * 	drm_connector_set_path_property(), in the case of DP MST with the
- * 	path property the MST manager created. Userspace cannot change this
+ * 	connected. This should be set by calling
+ * 	drm_connector_set_path_property(). Userspace cannot change this
  * 	property.
+ * 	In the case of DP MST this is equal to the path property the MST
+ * 	manager created. It is equal to 'mst:baseID-port-port...'.
+ * 	In the case of DP USB-C connector this is equal to the 'typec:portN',
+ * 	pointing to the corresponding Type-C port device or just 'typec' if the
+ * 	corresponding Type-C port can not be identified.
  * TILE:
  * 	Connector tile group property to indicate how a set of DRM connector
  * 	compose together into one logical screen. This is used by both high-res
-- 
2.39.2

[RFC PATCH v1 04/12] drm/bridge-connector: set the PATH property for the connector

[Dmitry Baryshkov]

In order to properly identify connectors (in particular, DisplayPort
connectors wrapped into USB-C) allow bridge drivers to specify the value
to be used for connector's PATH property.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_bridge_connector.c | 12 ++++++++++++
 include/drm/drm_bridge.h               |  7 +++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
index bf73960c2c2a..008d730e1c2f 100644
--- a/drivers/gpu/drm/drm_bridge_connector.c
+++ b/drivers/gpu/drm/drm_bridge_connector.c
@@ -331,6 +331,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 	struct drm_connector *connector;
 	struct i2c_adapter *ddc = NULL;
 	struct drm_bridge *bridge, *panel_bridge = NULL;
+	const char *path = NULL;
 	int connector_type;
 	int ret;
 
@@ -377,6 +378,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			connector->fwnode = fwnode_handle_get(of_fwnode_handle(bridge->of_node));
 #endif
 
+		if (bridge->path)
+			path = bridge->path;
+
 		if (bridge->ddc)
 			ddc = bridge->ddc;
 
@@ -405,6 +409,14 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 		connector->polled = DRM_CONNECTOR_POLL_CONNECT
 				  | DRM_CONNECTOR_POLL_DISCONNECT;
 
+	if (path) {
+		drm_object_attach_property(&connector->base,
+					   drm->mode_config.path_property,
+					   0);
+
+		drm_connector_set_path_property(connector, path);
+	}
+
 	if (panel_bridge)
 		drm_panel_bridge_set_orientation(connector, panel_bridge);
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index c339fc85fd07..98e9d76474f4 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -753,6 +753,13 @@ struct drm_bridge {
 	 * before the peripheral.
 	 */
 	bool pre_enable_prev_first;
+	/**
+	 * @path: the 'path' of the bridge. For bridges at the end of this
+	 * chain this is used to set the 'PATH' property of the connector.
+	 * This string is not freed manually, so one either should use a static
+	 * string here or a devres-allocated one.
+	 */
+	const char *path;
 	/**
 	 * @ddc: Associated I2C adapter for DDC access, if any.
 	 */
-- 
2.39.2

[RFC PATCH v1 05/12] drm/bridge: remove conditionals around devicetree pointers

[Dmitry Baryshkov]

Remove ifdef CONFIG_OF around the drm_bridge::of_node field. This follow
the correponding change to struct device we had since 2.6.39. Having
conditional around the of_node pointers turns out to make driver code
use ugly #ifdef blocks. Drop the conditionals and remove the #ifdef
blocks from the affected drivers.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/bridge/panel.c         | 2 --
 drivers/gpu/drm/drm_bridge_connector.c | 2 --
 include/drm/drm_bridge.h               | 2 --
 3 files changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 9316384b4474..7f41525f7a6e 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -302,9 +302,7 @@ struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
 	panel_bridge->panel = panel;
 
 	panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs;
-#ifdef CONFIG_OF
 	panel_bridge->bridge.of_node = panel->dev->of_node;
-#endif
 	panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES;
 	panel_bridge->bridge.type = connector_type;
 
diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
index 008d730e1c2f..ca255609fb08 100644
--- a/drivers/gpu/drm/drm_bridge_connector.c
+++ b/drivers/gpu/drm/drm_bridge_connector.c
@@ -372,11 +372,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 		if (!drm_bridge_get_next_bridge(bridge))
 			connector_type = bridge->type;
 
-#ifdef CONFIG_OF
 		if (!drm_bridge_get_next_bridge(bridge) &&
 		    bridge->of_node)
 			connector->fwnode = fwnode_handle_get(of_fwnode_handle(bridge->of_node));
-#endif
 
 		if (bridge->path)
 			path = bridge->path;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 98e9d76474f4..afa1de791075 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -716,10 +716,8 @@ struct drm_bridge {
 	struct drm_encoder *encoder;
 	/** @chain_node: used to form a bridge chain */
 	struct list_head chain_node;
-#ifdef CONFIG_OF
 	/** @of_node: device node pointer to the bridge */
 	struct device_node *of_node;
-#endif
 	/** @list: to keep track of all added bridges */
 	struct list_head list;
 	/**
-- 
2.39.2

[RFC PATCH v1 06/12] soc: qcom: pmic_glink_altmode: fix DRM connector type

[Dmitry Baryshkov]

During discussions regarding USB-C vs native DisplayPort it was pointed
out that other implementations (i915, AMD) are using
DRM_MODE_CONNECTOR_DisplayPort for both native and USB-C-wrapped DP
output. Follow this example and make the pmic_glink_altmode driver also
report DipslayPort connector rather than the USB one.

Cc: Simon Ser <contact@emersion.fr
Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/soc/qcom/pmic_glink_altmode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c
index d05e0d6edf49..974c14d1e0bf 100644
--- a/drivers/soc/qcom/pmic_glink_altmode.c
+++ b/drivers/soc/qcom/pmic_glink_altmode.c
@@ -465,7 +465,7 @@ static int pmic_glink_altmode_probe(struct auxiliary_device *adev,
 		alt_port->bridge.funcs = &pmic_glink_altmode_bridge_funcs;
 		alt_port->bridge.of_node = to_of_node(fwnode);
 		alt_port->bridge.ops = DRM_BRIDGE_OP_HPD;
-		alt_port->bridge.type = DRM_MODE_CONNECTOR_USB;
+		alt_port->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
 
 		ret = devm_drm_bridge_add(dev, &alt_port->bridge);
 		if (ret)
-- 
2.39.2

[RFC PATCH v1 07/12] soc: qcom: pmic_glink_altmode: report that this is a Type-C connector

[Dmitry Baryshkov]

Set the bridge's path property to point out that this connector is
wrapped into the Type-C port.

We can not really identify the exact Type-C port because it is
registered separately, by another driver, which is not mandatory and the
corresponding device is not even present on some of platforms, like
sc8180x or sm8350. Thus we use the shortened version of the PATH, which
includes just the 'typec:' part.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/soc/qcom/pmic_glink_altmode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c
index 974c14d1e0bf..a5b72046caaa 100644
--- a/drivers/soc/qcom/pmic_glink_altmode.c
+++ b/drivers/soc/qcom/pmic_glink_altmode.c
@@ -466,6 +466,7 @@ static int pmic_glink_altmode_probe(struct auxiliary_device *adev,
 		alt_port->bridge.of_node = to_of_node(fwnode);
 		alt_port->bridge.ops = DRM_BRIDGE_OP_HPD;
 		alt_port->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
+		alt_port->bridge.path = "typec:";
 
 		ret = devm_drm_bridge_add(dev, &alt_port->bridge);
 		if (ret)
-- 
2.39.2

[RFC PATCH v1 08/12] usb: typec: support generating Type-C port names for userspace

[Dmitry Baryshkov]

We need a way to generate and return the Type-C port device names. For
example, it is going to be used by the DRM to provide PATH property for
DisplayPort connectors.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/class.c | 14 ++++++++++++++
 include/linux/usb/typec.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 9c1dbf3c00e0..7394a2ecef6f 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -2327,6 +2327,20 @@ void typec_unregister_port(struct typec_port *port)
 }
 EXPORT_SYMBOL_GPL(typec_unregister_port);
 
+/**
+ * typec_port_get_name - Get USB Type-C Port name
+ * @port: The port to describe
+ *
+ * Returns a name of the passed USB Type-C port on success or NULL when the
+ * port has not been enumerated yet. The resulting string should be freed by
+ * the caller.
+ */
+char *typec_port_get_name(struct typec_port *port)
+{
+	return kasprintf(GFP_KERNEL, "typec:%s", dev_name(&port->dev));
+}
+EXPORT_SYMBOL_GPL(typec_port_get_name);
+
 static int __init typec_init(void)
 {
 	int ret;
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 8fa781207970..4aa9c9378383 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -303,6 +303,8 @@ struct typec_plug *typec_register_plug(struct typec_cable *cable,
 				       struct typec_plug_desc *desc);
 void typec_unregister_plug(struct typec_plug *plug);
 
+char *typec_port_get_name(struct typec_port *port);
+
 void typec_set_data_role(struct typec_port *port, enum typec_data_role role);
 void typec_set_pwr_role(struct typec_port *port, enum typec_role role);
 void typec_set_vconn_role(struct typec_port *port, enum typec_role role);
-- 
2.39.2

[RFC PATCH v1 09/12] usb: typec: tcpm: support generating Type-C port names for userspace

[Dmitry Baryshkov]

We need a way to generate and return the Type-C port device names. For
example, it is going to be used by the DRM to provide PATH property for
DisplayPort connectors.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/tcpm/tcpm.c | 14 ++++++++++++++
 include/linux/usb/tcpm.h      |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index d962f67c95ae..9709b56a3046 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -5539,6 +5539,20 @@ bool tcpm_port_is_toggling(struct tcpm_port *port)
 }
 EXPORT_SYMBOL_GPL(tcpm_port_is_toggling);
 
+/**
+ * tcpm_port_get_name - get the name of the corresponding USB Type-C port
+ * @port: TCPM port instance
+ *
+ * Returns a name of the USB Type-C port correponding to the passed TCPM port
+ * instance on success or NULL when the port has not been enumerated yet. The
+ * resulting string should be freed by the caller.
+ */
+char *tcpm_port_get_name(struct tcpm_port *port)
+{
+	return typec_port_get_name(port->typec_port);
+}
+EXPORT_SYMBOL_GPL(tcpm_port_get_name);
+
 static void tcpm_enable_frs_work(struct kthread_work *work)
 {
 	struct tcpm_port *port = container_of(work, struct tcpm_port, enable_frs);
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index ab7ca872950b..623c34788680 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -161,6 +161,8 @@ struct tcpm_port;
 struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc);
 void tcpm_unregister_port(struct tcpm_port *port);
 
+char *tcpm_port_get_name(struct tcpm_port *port);
+
 void tcpm_vbus_change(struct tcpm_port *port);
 void tcpm_cc_change(struct tcpm_port *port);
 void tcpm_sink_frs(struct tcpm_port *port);
-- 
2.39.2

[RFC PATCH v1 10/12] usb: typec: qcom: implement proper error path in probe()

[Dmitry Baryshkov]

Implement proper error path in the qcom_pmic_typec_probe(). This makes
sure that we properly disable hardware blocks and do not leak memory.

Fixes: a4422ff22142 ("usb: typec: qcom: Add Qualcomm PMIC Type-C driver")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
index 581199d37b49..ceda594a8d56 100644
--- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
+++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
@@ -258,15 +258,22 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
 	ret = qcom_pmic_typec_port_start(tcpm->pmic_typec_port,
 					 tcpm->tcpm_port);
 	if (ret)
-		goto fwnode_remove;
+		goto tcpm_unregister_port;
 
 	ret = qcom_pmic_typec_pdphy_start(tcpm->pmic_typec_pdphy,
 					  tcpm->tcpm_port);
 	if (ret)
-		goto fwnode_remove;
+		goto typec_stop;
 
 	return 0;
 
+
+typec_stop:
+	qcom_pmic_typec_port_stop(tcpm->pmic_typec_port);
+
+tcpm_unregister_port:
+	tcpm_unregister_port(tcpm->tcpm_port);
+
 fwnode_remove:
 	fwnode_remove_software_node(tcpm->tcpc.fwnode);
 
-- 
2.39.2

[RFC PATCH v1 11/12] usb: typec: qcom: extract DRM bridge functionality to separate file

[Dmitry Baryshkov]

In order to simplify source code and to reduce the amount of ifdefs in
the C files, extract the DRM bridge functionality to the separate file.

Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Suggested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/tcpm/qcom/Makefile          |  4 ++
 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 40 +++----------------
 .../usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c | 38 ++++++++++++++++++
 .../usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h | 20 ++++++++++
 4 files changed, 67 insertions(+), 35 deletions(-)
 create mode 100644 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c
 create mode 100644 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h

diff --git a/drivers/usb/typec/tcpm/qcom/Makefile b/drivers/usb/typec/tcpm/qcom/Makefile
index dc1e8832e197..6d01ec97c9fd 100644
--- a/drivers/usb/typec/tcpm/qcom/Makefile
+++ b/drivers/usb/typec/tcpm/qcom/Makefile
@@ -4,3 +4,7 @@ obj-$(CONFIG_TYPEC_QCOM_PMIC)		+= qcom_pmic_tcpm.o
 qcom_pmic_tcpm-y			+= qcom_pmic_typec.o \
 					   qcom_pmic_typec_port.o \
 					   qcom_pmic_typec_pdphy.o
+
+ifneq ($(CONFIG_DRM),)
+	qcom_pmic_tcpm-y	+= qcom_pmic_typec_drm.o
+endif
diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
index ceda594a8d56..b9d4856101c7 100644
--- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
+++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
@@ -18,8 +18,7 @@
 #include <linux/usb/tcpm.h>
 #include <linux/usb/typec_mux.h>
 
-#include <drm/drm_bridge.h>
-
+#include "qcom_pmic_typec_drm.h"
 #include "qcom_pmic_typec_pdphy.h"
 #include "qcom_pmic_typec_port.h"
 
@@ -34,9 +33,9 @@ struct pmic_typec {
 	struct tcpc_dev		tcpc;
 	struct pmic_typec_pdphy	*pmic_typec_pdphy;
 	struct pmic_typec_port	*pmic_typec_port;
+	struct pmic_typec_drm	*pmic_typec_drm;
 	bool			vbus_enabled;
 	struct mutex		lock;		/* VBUS state serialization */
-	struct drm_bridge	bridge;
 };
 
 #define tcpc_to_tcpm(_tcpc_) container_of(_tcpc_, struct pmic_typec, tcpc)
@@ -150,35 +149,6 @@ static int qcom_pmic_typec_init(struct tcpc_dev *tcpc)
 	return 0;
 }
 
-#if IS_ENABLED(CONFIG_DRM)
-static int qcom_pmic_typec_attach(struct drm_bridge *bridge,
-				     enum drm_bridge_attach_flags flags)
-{
-	return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL;
-}
-
-static const struct drm_bridge_funcs qcom_pmic_typec_bridge_funcs = {
-	.attach = qcom_pmic_typec_attach,
-};
-
-static int qcom_pmic_typec_init_drm(struct pmic_typec *tcpm)
-{
-	tcpm->bridge.funcs = &qcom_pmic_typec_bridge_funcs;
-#ifdef CONFIG_OF
-	tcpm->bridge.of_node = of_get_child_by_name(tcpm->dev->of_node, "connector");
-#endif
-	tcpm->bridge.ops = DRM_BRIDGE_OP_HPD;
-	tcpm->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
-
-	return devm_drm_bridge_add(tcpm->dev, &tcpm->bridge);
-}
-#else
-static int qcom_pmic_typec_init_drm(struct pmic_typec *tcpm)
-{
-	return 0;
-}
-#endif
-
 static int qcom_pmic_typec_probe(struct platform_device *pdev)
 {
 	struct pmic_typec *tcpm;
@@ -241,9 +211,9 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
 	mutex_init(&tcpm->lock);
 	platform_set_drvdata(pdev, tcpm);
 
-	ret = qcom_pmic_typec_init_drm(tcpm);
-	if (ret)
-		return ret;
+	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
+	if (IS_ERR(tcpm->pmic_typec_drm))
+		return PTR_ERR(tcpm->pmic_typec_drm);
 
 	tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
 	if (!tcpm->tcpc.fwnode)
diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c
new file mode 100644
index 000000000000..e202eb7b52db
--- /dev/null
+++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2023, Linaro Ltd. All rights reserved.
+ */
+
+#include <linux/of.h>
+
+#include <drm/drm_bridge.h>
+
+struct pmic_typec_drm {
+	struct drm_bridge bridge;
+};
+
+static int qcom_pmic_typec_attach(struct drm_bridge *bridge,
+				     enum drm_bridge_attach_flags flags)
+{
+	return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL;
+}
+
+static const struct drm_bridge_funcs qcom_pmic_typec_bridge_funcs = {
+	.attach = qcom_pmic_typec_attach,
+};
+
+struct pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev)
+{
+	struct pmic_typec_drm *tcpm_drm;
+
+	tcpm_drm = devm_kzalloc(dev, sizeof(*tcpm_drm), GFP_KERNEL);
+	if (!tcpm_drm)
+		return ERR_PTR(-ENOMEM);
+
+	tcpm_drm->bridge.funcs = &qcom_pmic_typec_bridge_funcs;
+	tcpm_drm->bridge.of_node = of_get_child_by_name(dev->of_node, "connector");
+	tcpm_drm->bridge.ops = DRM_BRIDGE_OP_HPD;
+	tcpm_drm->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
+
+	return ERR_PTR(devm_drm_bridge_add(dev, &tcpm_drm->bridge));
+}
diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h
new file mode 100644
index 000000000000..01f4bb71346b
--- /dev/null
+++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2023, Linaro Ltd. All rights reserved.
+ */
+
+#ifndef __QCOM_PMIC_DRM_H__
+#define __QCOM_PMIC_DRM_H__
+
+struct pmic_typec_drm;
+
+#if IS_ENABLED(CONFIG_DRM)
+struct pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev);
+#else
+static inline pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
+#endif /* __QCOM_PMIC_DRM_H__ */
-- 
2.39.2

[RFC PATCH v1 12/12] usb: typec: qcom: define the bridge's path

[Dmitry Baryshkov]

In order to notify the userspace about the DRM connector's USB-C port,
export the corresponding port's name as the bridge's path field.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c     | 11 +++++++----
 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c |  4 +++-
 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h |  6 ++++--
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
index b9d4856101c7..452dc6437861 100644
--- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
+++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
@@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
 	struct device_node *np = dev->of_node;
 	const struct pmic_typec_resources *res;
 	struct regmap *regmap;
+	char *tcpm_name;
 	u32 base[2];
 	int ret;
 
@@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
 	mutex_init(&tcpm->lock);
 	platform_set_drvdata(pdev, tcpm);
 
-	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
-	if (IS_ERR(tcpm->pmic_typec_drm))
-		return PTR_ERR(tcpm->pmic_typec_drm);
-
 	tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
 	if (!tcpm->tcpc.fwnode)
 		return -EINVAL;
@@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
 		goto fwnode_remove;
 	}
 
+	tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
+	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
+	kfree(tcpm_name);
+	if (IS_ERR(tcpm->pmic_typec_drm))
+		return PTR_ERR(tcpm->pmic_typec_drm);
+
 	ret = qcom_pmic_typec_port_start(tcpm->pmic_typec_port,
 					 tcpm->tcpm_port);
 	if (ret)
diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c
index e202eb7b52db..7bd7f4bf3539 100644
--- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c
+++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c
@@ -21,7 +21,8 @@ static const struct drm_bridge_funcs qcom_pmic_typec_bridge_funcs = {
 	.attach = qcom_pmic_typec_attach,
 };
 
-struct pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev)
+struct pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev,
+						const char *path)
 {
 	struct pmic_typec_drm *tcpm_drm;
 
@@ -33,6 +34,7 @@ struct pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev)
 	tcpm_drm->bridge.of_node = of_get_child_by_name(dev->of_node, "connector");
 	tcpm_drm->bridge.ops = DRM_BRIDGE_OP_HPD;
 	tcpm_drm->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
+	tcpm_drm->bridge.path = devm_kstrdup(dev, path, GFP_KERNEL);
 
 	return ERR_PTR(devm_drm_bridge_add(dev, &tcpm_drm->bridge));
 }
diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h
index 01f4bb71346b..259c047265f7 100644
--- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h
+++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h
@@ -9,9 +9,11 @@
 struct pmic_typec_drm;
 
 #if IS_ENABLED(CONFIG_DRM)
-struct pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev);
+struct pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev,
+						const char *path);
 #else
-static inline pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev)
+static inline pmic_typec_drm *qcom_pmic_typec_init_drm(struct device *dev,
+						       const char *path)
 {
 	return NULL;
 }
-- 
2.39.2

Re: [RFC PATCH v1 06/12] soc: qcom: pmic_glink_altmode: fix DRM connector type

[Bjorn Andersson]

On Mon, Sep 04, 2023 at 12:41:44AM +0300, Dmitry Baryshkov wrote:
> During discussions regarding USB-C vs native DisplayPort it was pointed
> out that other implementations (i915, AMD) are using
> DRM_MODE_CONNECTOR_DisplayPort for both native and USB-C-wrapped DP
> output. Follow this example and make the pmic_glink_altmode driver also
> report DipslayPort connector rather than the USB one.

I started off with this, but on devices with both USB Type-C and
(native) DisplayPort, it seemed much more reasonable to make the
distinction; and thereby get the outputs named "USB-n".

Similarly, it looks quite reasonable that the output on my laptop are
eDP-1, USB-1 and USB-2.


But, if this is not the way its intended to be used, feel free to pick
this together with the other patches.

Acked-by: Bjorn Andersson <andersson@kernel.org>

Regards,
Bjorn

Re: [RFC PATCH v1 07/12] soc: qcom: pmic_glink_altmode: report that this is a Type-C connector

[Bjorn Andersson]

On Mon, Sep 04, 2023 at 12:41:45AM +0300, Dmitry Baryshkov wrote:
> Set the bridge's path property to point out that this connector is
> wrapped into the Type-C port.
> 
> We can not really identify the exact Type-C port because it is
> registered separately, by another driver, which is not mandatory and the
> corresponding device is not even present on some of platforms, like
> sc8180x or sm8350. Thus we use the shortened version of the PATH, which
> includes just the 'typec:' part.

How would a properly resolved path look like?

As with the other patch, I'm okay with this going through the USB tree.

Acked-by: Bjorn Andersson <andersson@kernel.org>

Regards,
Bjorn

Re: [RFC PATCH v1 07/12] soc: qcom: pmic_glink_altmode: report that this is a Type-C connector

[Dmitry Baryshkov]

On 04/09/2023 18:43, Bjorn Andersson wrote:
> On Mon, Sep 04, 2023 at 12:41:45AM +0300, Dmitry Baryshkov wrote:
>> Set the bridge's path property to point out that this connector is
>> wrapped into the Type-C port.
>>
>> We can not really identify the exact Type-C port because it is
>> registered separately, by another driver, which is not mandatory and the
>> corresponding device is not even present on some of platforms, like
>> sc8180x or sm8350. Thus we use the shortened version of the PATH, which
>> includes just the 'typec:' part.
> 
> How would a properly resolved path look like?

On RB5 it is 'typec:port0', as the USB-C port is registered as 
/sys/class/typec/port0

> 
> As with the other patch, I'm okay with this going through the USB tree.
> 
> Acked-by: Bjorn Andersson <andersson@kernel.org>
> 
> Regards,
> Bjorn

-- 
With best wishes
Dmitry

Re: [RFC PATCH v1 00/12] drm,usb/typec: uABI for USB-C DisplayPort connectors

[Bjorn Andersson]

On Mon, Sep 04, 2023 at 12:41:38AM +0300, Dmitry Baryshkov wrote:
> During the discussion regarding DisplayPort wrapped in the USB-C
> connectors (via the USB-C altmode) it was pointed out that currently
> there is no good way to let userspace know if the DRM connector in
> question is the 'native' DP connector or if it is the USB-C connector.
> 
> An attempt to use DRM_MODE_CONNECTOR_USB for such connectors was
> declined, as existing DP drivers (i915, AMD) use
> DRM_MODE_CONNECTOR_DisplayPort. New drivers should behave in the same
> way.
> 

Sorry, didn't see the commit message before posting my complaint about
USB -> DisplayPort.

> An attempt to use subconnector property was also declined. It is defined
> to the type of the DP dongle connector rather than the host connector.
> 
> This attempt targets reusing the connector's PATH property. Currently
> this property is only used for the DP MST connectors. This patchset
> reuses it to point out to the corresponding registered typec port
> device.
> 

Still interested in understanding how the path string should look like.

Is the path expected to be consumed by machine, or is it only there for
human convenience?

Regards,
Bjorn

Re: [RFC PATCH v1 00/12] drm,usb/typec: uABI for USB-C DisplayPort connectors

[Dmitry Baryshkov]

On 04/09/2023 18:46, Bjorn Andersson wrote:
> On Mon, Sep 04, 2023 at 12:41:38AM +0300, Dmitry Baryshkov wrote:
>> During the discussion regarding DisplayPort wrapped in the USB-C
>> connectors (via the USB-C altmode) it was pointed out that currently
>> there is no good way to let userspace know if the DRM connector in
>> question is the 'native' DP connector or if it is the USB-C connector.
>>
>> An attempt to use DRM_MODE_CONNECTOR_USB for such connectors was
>> declined, as existing DP drivers (i915, AMD) use
>> DRM_MODE_CONNECTOR_DisplayPort. New drivers should behave in the same
>> way.
>>
> 
> Sorry, didn't see the commit message before posting my complaint about
> USB -> DisplayPort.
> 
>> An attempt to use subconnector property was also declined. It is defined
>> to the type of the DP dongle connector rather than the host connector.
>>
>> This attempt targets reusing the connector's PATH property. Currently
>> this property is only used for the DP MST connectors. This patchset
>> reuses it to point out to the corresponding registered typec port
>> device.
>>
> 
> Still interested in understanding how the path string should look like.

As wrote in the other letter, on RB5 it is 'typec:port0'. If the machine 
has two Type-C ports and two connected DP blocks, one of them will have 
'typec:port0', another one 'typec:port1'. This way one can further look 
under /sys/class/typec/portN/physical_localtion/ and find corresponding 
location, etc.

> Is the path expected to be consumed by machine, or is it only there for
> human convenience?

As with DP MST it is expected that userspace will consume this 
information, possibly renaming the connector. For example, on my laptop 
I have DP-1, ... DP-5 connectors (with DP-2 -- DP-5 being DP MST ones). 
Xorg renames them to DP-1, DP-2, DP-1-1, DP-1-2, DP-1-3, because the MST 
ones are branches for the DP-1.

-- 
With best wishes
Dmitry

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Heikki Krogerus]

Hi Dmitry,

On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> dev_fwnode() checks never succeed, making the respective commit NOP.

That's not true. The dev->fwnode is assigned when the device is
created on ACPI platforms automatically. If the drm_connector fwnode
member is assigned before the device is registered, then that fwnode
is assigned also to the device - see drm_connector_acpi_find_companion().

But please note that even if drm_connector does not have anything in
its fwnode member, the device may still be assigned fwnode, just based
on some other logic (maybe in drivers/acpi/acpi_video.c?).

> And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> breaks drivers already using components (as it was pointed at [1]),
> resulting in a deadlock. Lockdep trace is provided below.
> 
> Granted these two issues, it seems impractical to fix this commit in any
> sane way. Revert it instead.

I think there is already user space stuff that relies on these links,
so I'm not sure you can just remove them like that. If the component
framework is not the correct tool here, then I think you need to
suggest some other way of creating them.

Side note. The problem you are describing here is a limitation in the
component framework - right now it's made with the idea that a device
can represent a single component, but it really should allow a device
to represent multiple components. I'm not saying that you should try
to fix the component framework, but I just wanted to make a note about
this (and this is not the only problem with the component framework).

I like the component framework as a concept, but I think it needs a
lot of improvements - possibly rewrite.

thanks,

-- 
heikki

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Dmitry Baryshkov]

Hi Heikki,

On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Dmitry,
>
> On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> > dev_fwnode() checks never succeed, making the respective commit NOP.
>
> That's not true. The dev->fwnode is assigned when the device is
> created on ACPI platforms automatically. If the drm_connector fwnode
> member is assigned before the device is registered, then that fwnode
> is assigned also to the device - see drm_connector_acpi_find_companion().
>
> But please note that even if drm_connector does not have anything in
> its fwnode member, the device may still be assigned fwnode, just based
> on some other logic (maybe in drivers/acpi/acpi_video.c?).
>
> > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> > breaks drivers already using components (as it was pointed at [1]),
> > resulting in a deadlock. Lockdep trace is provided below.
> >
> > Granted these two issues, it seems impractical to fix this commit in any
> > sane way. Revert it instead.
>
> I think there is already user space stuff that relies on these links,
> so I'm not sure you can just remove them like that. If the component
> framework is not the correct tool here, then I think you need to
> suggest some other way of creating them.

The issue (that was pointed out during review) is that having a
component code in the framework code can lead to lockups. With the
patch #2 in place (which is the only logical way to set kdev->fwnode
for non-ACPI systems) probing of drivers which use components and set
drm_connector::fwnode breaks immediately.

Can we move the component part to the respective drivers? With the
patch 2 in place, connector->fwnode will be copied to the created
kdev's fwnode pointer.

Another option might be to make this drm_sysfs component registration optional.

> Side note. The problem you are describing here is a limitation in the
> component framework - right now it's made with the idea that a device
> can represent a single component, but it really should allow a device
> to represent multiple components. I'm not saying that you should try
> to fix the component framework, but I just wanted to make a note about
> this (and this is not the only problem with the component framework).
>
> I like the component framework as a concept, but I think it needs a
> lot of improvements - possibly rewrite.

Yes. There were several attempts to rewrite the component framework,
but none succeeded up to now. Anyway, I consider rewriting components
framework to be a bigger topic compared to drm connector fwnode setup.

--
With best wishes
Dmitry

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Heikki Krogerus]

On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
> Hi Heikki,
> 
> On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Hi Dmitry,
> >
> > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> > > dev_fwnode() checks never succeed, making the respective commit NOP.
> >
> > That's not true. The dev->fwnode is assigned when the device is
> > created on ACPI platforms automatically. If the drm_connector fwnode
> > member is assigned before the device is registered, then that fwnode
> > is assigned also to the device - see drm_connector_acpi_find_companion().
> >
> > But please note that even if drm_connector does not have anything in
> > its fwnode member, the device may still be assigned fwnode, just based
> > on some other logic (maybe in drivers/acpi/acpi_video.c?).
> >
> > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> > > breaks drivers already using components (as it was pointed at [1]),
> > > resulting in a deadlock. Lockdep trace is provided below.
> > >
> > > Granted these two issues, it seems impractical to fix this commit in any
> > > sane way. Revert it instead.
> >
> > I think there is already user space stuff that relies on these links,
> > so I'm not sure you can just remove them like that. If the component
> > framework is not the correct tool here, then I think you need to
> > suggest some other way of creating them.
> 
> The issue (that was pointed out during review) is that having a
> component code in the framework code can lead to lockups. With the
> patch #2 in place (which is the only logical way to set kdev->fwnode
> for non-ACPI systems) probing of drivers which use components and set
> drm_connector::fwnode breaks immediately.
> 
> Can we move the component part to the respective drivers? With the
> patch 2 in place, connector->fwnode will be copied to the created
> kdev's fwnode pointer.
> 
> Another option might be to make this drm_sysfs component registration optional.

You don't need to use the component framework at all if there is
a better way of determining the connection between the DP and its
Type-C connector (I'm assuming that that's what this series is about).
You just need the symlinks, not the component.

thanks,

-- 
heikki

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Dmitry Baryshkov]

On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
> > Hi Heikki,
> >
> > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > Hi Dmitry,
> > >
> > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> > > > dev_fwnode() checks never succeed, making the respective commit NOP.
> > >
> > > That's not true. The dev->fwnode is assigned when the device is
> > > created on ACPI platforms automatically. If the drm_connector fwnode
> > > member is assigned before the device is registered, then that fwnode
> > > is assigned also to the device - see drm_connector_acpi_find_companion().
> > >
> > > But please note that even if drm_connector does not have anything in
> > > its fwnode member, the device may still be assigned fwnode, just based
> > > on some other logic (maybe in drivers/acpi/acpi_video.c?).
> > >
> > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> > > > breaks drivers already using components (as it was pointed at [1]),
> > > > resulting in a deadlock. Lockdep trace is provided below.
> > > >
> > > > Granted these two issues, it seems impractical to fix this commit in any
> > > > sane way. Revert it instead.
> > >
> > > I think there is already user space stuff that relies on these links,
> > > so I'm not sure you can just remove them like that. If the component
> > > framework is not the correct tool here, then I think you need to
> > > suggest some other way of creating them.
> >
> > The issue (that was pointed out during review) is that having a
> > component code in the framework code can lead to lockups. With the
> > patch #2 in place (which is the only logical way to set kdev->fwnode
> > for non-ACPI systems) probing of drivers which use components and set
> > drm_connector::fwnode breaks immediately.
> >
> > Can we move the component part to the respective drivers? With the
> > patch 2 in place, connector->fwnode will be copied to the created
> > kdev's fwnode pointer.
> >
> > Another option might be to make this drm_sysfs component registration optional.
>
> You don't need to use the component framework at all if there is
> a better way of determining the connection between the DP and its
> Type-C connector (I'm assuming that that's what this series is about).
> You just need the symlinks, not the component.

The problem is that right now this component registration has become
mandatory. And if I set the kdev->fwnode manually (like in the patch
2), the kernel hangs inside the component code.
That's why I proposed to move the components to the place where they
are really necessary, e.g. i915 and amd drivers.

-- 
With best wishes
Dmitry

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Laurent Pinchart]

On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
> On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus wrote:
> > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
> > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus wrote:
> > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> > > > > dev_fwnode() checks never succeed, making the respective commit NOP.
> > > >
> > > > That's not true. The dev->fwnode is assigned when the device is
> > > > created on ACPI platforms automatically. If the drm_connector fwnode
> > > > member is assigned before the device is registered, then that fwnode
> > > > is assigned also to the device - see drm_connector_acpi_find_companion().
> > > >
> > > > But please note that even if drm_connector does not have anything in
> > > > its fwnode member, the device may still be assigned fwnode, just based
> > > > on some other logic (maybe in drivers/acpi/acpi_video.c?).
> > > >
> > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> > > > > breaks drivers already using components (as it was pointed at [1]),
> > > > > resulting in a deadlock. Lockdep trace is provided below.
> > > > >
> > > > > Granted these two issues, it seems impractical to fix this commit in any
> > > > > sane way. Revert it instead.
> > > >
> > > > I think there is already user space stuff that relies on these links,
> > > > so I'm not sure you can just remove them like that. If the component
> > > > framework is not the correct tool here, then I think you need to
> > > > suggest some other way of creating them.
> > >
> > > The issue (that was pointed out during review) is that having a
> > > component code in the framework code can lead to lockups. With the
> > > patch #2 in place (which is the only logical way to set kdev->fwnode
> > > for non-ACPI systems) probing of drivers which use components and set
> > > drm_connector::fwnode breaks immediately.
> > >
> > > Can we move the component part to the respective drivers? With the
> > > patch 2 in place, connector->fwnode will be copied to the created
> > > kdev's fwnode pointer.
> > >
> > > Another option might be to make this drm_sysfs component registration optional.
> >
> > You don't need to use the component framework at all if there is
> > a better way of determining the connection between the DP and its
> > Type-C connector (I'm assuming that that's what this series is about).
> > You just need the symlinks, not the component.
> 
> The problem is that right now this component registration has become
> mandatory. And if I set the kdev->fwnode manually (like in the patch
> 2), the kernel hangs inside the component code.
> That's why I proposed to move the components to the place where they
> are really necessary, e.g. i915 and amd drivers.

I'm all for keeping the component framework out of common code. I
dislike that framework with passion, and still haven't lost all hopes of
replacing it with something better.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Heikki Krogerus]

On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
> On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
> > > Hi Heikki,
> > >
> > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > Hi Dmitry,
> > > >
> > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> > > > > dev_fwnode() checks never succeed, making the respective commit NOP.
> > > >
> > > > That's not true. The dev->fwnode is assigned when the device is
> > > > created on ACPI platforms automatically. If the drm_connector fwnode
> > > > member is assigned before the device is registered, then that fwnode
> > > > is assigned also to the device - see drm_connector_acpi_find_companion().
> > > >
> > > > But please note that even if drm_connector does not have anything in
> > > > its fwnode member, the device may still be assigned fwnode, just based
> > > > on some other logic (maybe in drivers/acpi/acpi_video.c?).
> > > >
> > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> > > > > breaks drivers already using components (as it was pointed at [1]),
> > > > > resulting in a deadlock. Lockdep trace is provided below.
> > > > >
> > > > > Granted these two issues, it seems impractical to fix this commit in any
> > > > > sane way. Revert it instead.
> > > >
> > > > I think there is already user space stuff that relies on these links,
> > > > so I'm not sure you can just remove them like that. If the component
> > > > framework is not the correct tool here, then I think you need to
> > > > suggest some other way of creating them.
> > >
> > > The issue (that was pointed out during review) is that having a
> > > component code in the framework code can lead to lockups. With the
> > > patch #2 in place (which is the only logical way to set kdev->fwnode
> > > for non-ACPI systems) probing of drivers which use components and set
> > > drm_connector::fwnode breaks immediately.
> > >
> > > Can we move the component part to the respective drivers? With the
> > > patch 2 in place, connector->fwnode will be copied to the created
> > > kdev's fwnode pointer.
> > >
> > > Another option might be to make this drm_sysfs component registration optional.
> >
> > You don't need to use the component framework at all if there is
> > a better way of determining the connection between the DP and its
> > Type-C connector (I'm assuming that that's what this series is about).
> > You just need the symlinks, not the component.
> 
> The problem is that right now this component registration has become
> mandatory. And if I set the kdev->fwnode manually (like in the patch
> 2), the kernel hangs inside the component code.
> That's why I proposed to move the components to the place where they
> are really necessary, e.g. i915 and amd drivers.

So why can't we replace the component with the method you are
proposing in this series of finding out the Type-C port also with
i915, AMD, or whatever driver and platform (that's the only thing that
component is used for)?

Determining the connection between a DP and its Type-C connector is
starting to get really important, so ideally we have a common solution
for that.

thanks,

-- 
heikki

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3514 bytes --]

On Wed, Sep 06, 2023 at 03:53:14PM +0300, Laurent Pinchart wrote:
> On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
> > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus wrote:
> > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
> > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus wrote:
> > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> > > > > > dev_fwnode() checks never succeed, making the respective commit NOP.
> > > > >
> > > > > That's not true. The dev->fwnode is assigned when the device is
> > > > > created on ACPI platforms automatically. If the drm_connector fwnode
> > > > > member is assigned before the device is registered, then that fwnode
> > > > > is assigned also to the device - see drm_connector_acpi_find_companion().
> > > > >
> > > > > But please note that even if drm_connector does not have anything in
> > > > > its fwnode member, the device may still be assigned fwnode, just based
> > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?).
> > > > >
> > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> > > > > > breaks drivers already using components (as it was pointed at [1]),
> > > > > > resulting in a deadlock. Lockdep trace is provided below.
> > > > > >
> > > > > > Granted these two issues, it seems impractical to fix this commit in any
> > > > > > sane way. Revert it instead.
> > > > >
> > > > > I think there is already user space stuff that relies on these links,
> > > > > so I'm not sure you can just remove them like that. If the component
> > > > > framework is not the correct tool here, then I think you need to
> > > > > suggest some other way of creating them.
> > > >
> > > > The issue (that was pointed out during review) is that having a
> > > > component code in the framework code can lead to lockups. With the
> > > > patch #2 in place (which is the only logical way to set kdev->fwnode
> > > > for non-ACPI systems) probing of drivers which use components and set
> > > > drm_connector::fwnode breaks immediately.
> > > >
> > > > Can we move the component part to the respective drivers? With the
> > > > patch 2 in place, connector->fwnode will be copied to the created
> > > > kdev's fwnode pointer.
> > > >
> > > > Another option might be to make this drm_sysfs component registration optional.
> > >
> > > You don't need to use the component framework at all if there is
> > > a better way of determining the connection between the DP and its
> > > Type-C connector (I'm assuming that that's what this series is about).
> > > You just need the symlinks, not the component.
> > 
> > The problem is that right now this component registration has become
> > mandatory. And if I set the kdev->fwnode manually (like in the patch
> > 2), the kernel hangs inside the component code.
> > That's why I proposed to move the components to the place where they
> > are really necessary, e.g. i915 and amd drivers.
> 
> I'm all for keeping the component framework out of common code. I
> dislike that framework with passion, and still haven't lost all hopes of
> replacing it with something better.

I'm not sure I share the same hate for the component framework, but I
agree. It's optional anyway, so we should provide a solution that works
for drivers working with and without the component framework.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Dmitry Baryshkov]

On 06/09/2023 16:38, Heikki Krogerus wrote:
> On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
>> On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
>> <heikki.krogerus@linux.intel.com> wrote:
>>>
>>> On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
>>>> Hi Heikki,
>>>>
>>>> On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
>>>> <heikki.krogerus@linux.intel.com> wrote:
>>>>>
>>>>> Hi Dmitry,
>>>>>
>>>>> On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
>>>>>> The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
>>>>>> dev_fwnode() checks never succeed, making the respective commit NOP.
>>>>>
>>>>> That's not true. The dev->fwnode is assigned when the device is
>>>>> created on ACPI platforms automatically. If the drm_connector fwnode
>>>>> member is assigned before the device is registered, then that fwnode
>>>>> is assigned also to the device - see drm_connector_acpi_find_companion().
>>>>>
>>>>> But please note that even if drm_connector does not have anything in
>>>>> its fwnode member, the device may still be assigned fwnode, just based
>>>>> on some other logic (maybe in drivers/acpi/acpi_video.c?).
>>>>>
>>>>>> And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
>>>>>> breaks drivers already using components (as it was pointed at [1]),
>>>>>> resulting in a deadlock. Lockdep trace is provided below.
>>>>>>
>>>>>> Granted these two issues, it seems impractical to fix this commit in any
>>>>>> sane way. Revert it instead.
>>>>>
>>>>> I think there is already user space stuff that relies on these links,
>>>>> so I'm not sure you can just remove them like that. If the component
>>>>> framework is not the correct tool here, then I think you need to
>>>>> suggest some other way of creating them.
>>>>
>>>> The issue (that was pointed out during review) is that having a
>>>> component code in the framework code can lead to lockups. With the
>>>> patch #2 in place (which is the only logical way to set kdev->fwnode
>>>> for non-ACPI systems) probing of drivers which use components and set
>>>> drm_connector::fwnode breaks immediately.
>>>>
>>>> Can we move the component part to the respective drivers? With the
>>>> patch 2 in place, connector->fwnode will be copied to the created
>>>> kdev's fwnode pointer.
>>>>
>>>> Another option might be to make this drm_sysfs component registration optional.
>>>
>>> You don't need to use the component framework at all if there is
>>> a better way of determining the connection between the DP and its
>>> Type-C connector (I'm assuming that that's what this series is about).
>>> You just need the symlinks, not the component.
>>
>> The problem is that right now this component registration has become
>> mandatory. And if I set the kdev->fwnode manually (like in the patch
>> 2), the kernel hangs inside the component code.
>> That's why I proposed to move the components to the place where they
>> are really necessary, e.g. i915 and amd drivers.
> 
> So why can't we replace the component with the method you are
> proposing in this series of finding out the Type-C port also with
> i915, AMD, or whatever driver and platform (that's the only thing that
> component is used for)?

The drm/msm driver uses drm_bridge for the pipeline (including the last 
DP entry) and the drm_bridge_connector to create the connector. I think 
that enabling i915 and AMD drivers to use drm_bridge fells out of scope 
for this series.


> Determining the connection between a DP and its Type-C connector is
> starting to get really important, so ideally we have a common solution
> for that.

Yes. This is what we have been discussing with Simon for quite some time 
on #dri-devel.

Unfortunately I think the solution that got merged was pretty much 
hastened in instead of being well-thought. For example, it is also not 
always possible to provide the drm_connector / typec_connector links (as 
you can see from the patch7. Sometimes we can only express that this is 
a Type-C DP connector, but we can not easily point it to the particular 
USB-C port.

So, I'm not sure, how can we proceed here. Currently merged patch breaks 
drm/msm if we even try to use it by setting kdef->fwnode to 
drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` 
is an ACPI-only thing, which is not expected to work in a non-ACPI cases.

-- 
With best wishes
Dmitry

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Heikki Krogerus]

On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote:
> On 06/09/2023 16:38, Heikki Krogerus wrote:
> > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
> > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > > 
> > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
> > > > > Hi Heikki,
> > > > > 
> > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
> > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > 
> > > > > > Hi Dmitry,
> > > > > > 
> > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP.
> > > > > > 
> > > > > > That's not true. The dev->fwnode is assigned when the device is
> > > > > > created on ACPI platforms automatically. If the drm_connector fwnode
> > > > > > member is assigned before the device is registered, then that fwnode
> > > > > > is assigned also to the device - see drm_connector_acpi_find_companion().
> > > > > > 
> > > > > > But please note that even if drm_connector does not have anything in
> > > > > > its fwnode member, the device may still be assigned fwnode, just based
> > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?).
> > > > > > 
> > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> > > > > > > breaks drivers already using components (as it was pointed at [1]),
> > > > > > > resulting in a deadlock. Lockdep trace is provided below.
> > > > > > > 
> > > > > > > Granted these two issues, it seems impractical to fix this commit in any
> > > > > > > sane way. Revert it instead.
> > > > > > 
> > > > > > I think there is already user space stuff that relies on these links,
> > > > > > so I'm not sure you can just remove them like that. If the component
> > > > > > framework is not the correct tool here, then I think you need to
> > > > > > suggest some other way of creating them.
> > > > > 
> > > > > The issue (that was pointed out during review) is that having a
> > > > > component code in the framework code can lead to lockups. With the
> > > > > patch #2 in place (which is the only logical way to set kdev->fwnode
> > > > > for non-ACPI systems) probing of drivers which use components and set
> > > > > drm_connector::fwnode breaks immediately.
> > > > > 
> > > > > Can we move the component part to the respective drivers? With the
> > > > > patch 2 in place, connector->fwnode will be copied to the created
> > > > > kdev's fwnode pointer.
> > > > > 
> > > > > Another option might be to make this drm_sysfs component registration optional.
> > > > 
> > > > You don't need to use the component framework at all if there is
> > > > a better way of determining the connection between the DP and its
> > > > Type-C connector (I'm assuming that that's what this series is about).
> > > > You just need the symlinks, not the component.
> > > 
> > > The problem is that right now this component registration has become
> > > mandatory. And if I set the kdev->fwnode manually (like in the patch
> > > 2), the kernel hangs inside the component code.
> > > That's why I proposed to move the components to the place where they
> > > are really necessary, e.g. i915 and amd drivers.
> > 
> > So why can't we replace the component with the method you are
> > proposing in this series of finding out the Type-C port also with
> > i915, AMD, or whatever driver and platform (that's the only thing that
> > component is used for)?
> 
> The drm/msm driver uses drm_bridge for the pipeline (including the last DP
> entry) and the drm_bridge_connector to create the connector. I think that
> enabling i915 and AMD drivers to use drm_bridge fells out of scope for this
> series.
> 
> 
> > Determining the connection between a DP and its Type-C connector is
> > starting to get really important, so ideally we have a common solution
> > for that.
> 
> Yes. This is what we have been discussing with Simon for quite some time on
> #dri-devel.
> 
> Unfortunately I think the solution that got merged was pretty much hastened
> in instead of being well-thought. For example, it is also not always
> possible to provide the drm_connector / typec_connector links (as you can
> see from the patch7. Sometimes we can only express that this is a Type-C DP
> connector, but we can not easily point it to the particular USB-C port.
> 
> So, I'm not sure, how can we proceed here. Currently merged patch breaks
> drm/msm if we even try to use it by setting kdef->fwnode to
> drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is
> an ACPI-only thing, which is not expected to work in a non-ACPI cases.

You really have to always supply not only the Type-C ports and partners,
but also the alt modes. You need them, firstly to keep things sane
inside kernel, but more importantly, so they are always exposed to the
user space, AND, always the same way. We have ABIs for all this stuff,
including the DP alt mode. Use them. No shortcuts.

So here's what you need to do. UCSI does not seem to bring you
anything useful, so just disable it for now. You don't need it. Your
port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so
that's where you need to register all these components - the ports,
partners and alt modes. You have all the needed information there.

Only after you've done that we can start to look at how should the
connection between the DPs and their USB Type-C connectors be handled.

thanks,

-- 
heikki

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Dmitry Baryshkov]

On 12/09/2023 14:05, Heikki Krogerus wrote:
> On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote:
>> On 06/09/2023 16:38, Heikki Krogerus wrote:
>>> On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
>>>> On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
>>>> <heikki.krogerus@linux.intel.com> wrote:
>>>>>
>>>>> On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
>>>>>> Hi Heikki,
>>>>>>
>>>>>> On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
>>>>>> <heikki.krogerus@linux.intel.com> wrote:
>>>>>>>
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
>>>>>>>> The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
>>>>>>>> dev_fwnode() checks never succeed, making the respective commit NOP.
>>>>>>>
>>>>>>> That's not true. The dev->fwnode is assigned when the device is
>>>>>>> created on ACPI platforms automatically. If the drm_connector fwnode
>>>>>>> member is assigned before the device is registered, then that fwnode
>>>>>>> is assigned also to the device - see drm_connector_acpi_find_companion().
>>>>>>>
>>>>>>> But please note that even if drm_connector does not have anything in
>>>>>>> its fwnode member, the device may still be assigned fwnode, just based
>>>>>>> on some other logic (maybe in drivers/acpi/acpi_video.c?).
>>>>>>>
>>>>>>>> And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
>>>>>>>> breaks drivers already using components (as it was pointed at [1]),
>>>>>>>> resulting in a deadlock. Lockdep trace is provided below.
>>>>>>>>
>>>>>>>> Granted these two issues, it seems impractical to fix this commit in any
>>>>>>>> sane way. Revert it instead.
>>>>>>>
>>>>>>> I think there is already user space stuff that relies on these links,
>>>>>>> so I'm not sure you can just remove them like that. If the component
>>>>>>> framework is not the correct tool here, then I think you need to
>>>>>>> suggest some other way of creating them.
>>>>>>
>>>>>> The issue (that was pointed out during review) is that having a
>>>>>> component code in the framework code can lead to lockups. With the
>>>>>> patch #2 in place (which is the only logical way to set kdev->fwnode
>>>>>> for non-ACPI systems) probing of drivers which use components and set
>>>>>> drm_connector::fwnode breaks immediately.
>>>>>>
>>>>>> Can we move the component part to the respective drivers? With the
>>>>>> patch 2 in place, connector->fwnode will be copied to the created
>>>>>> kdev's fwnode pointer.
>>>>>>
>>>>>> Another option might be to make this drm_sysfs component registration optional.
>>>>>
>>>>> You don't need to use the component framework at all if there is
>>>>> a better way of determining the connection between the DP and its
>>>>> Type-C connector (I'm assuming that that's what this series is about).
>>>>> You just need the symlinks, not the component.
>>>>
>>>> The problem is that right now this component registration has become
>>>> mandatory. And if I set the kdev->fwnode manually (like in the patch
>>>> 2), the kernel hangs inside the component code.
>>>> That's why I proposed to move the components to the place where they
>>>> are really necessary, e.g. i915 and amd drivers.
>>>
>>> So why can't we replace the component with the method you are
>>> proposing in this series of finding out the Type-C port also with
>>> i915, AMD, or whatever driver and platform (that's the only thing that
>>> component is used for)?
>>
>> The drm/msm driver uses drm_bridge for the pipeline (including the last DP
>> entry) and the drm_bridge_connector to create the connector. I think that
>> enabling i915 and AMD drivers to use drm_bridge fells out of scope for this
>> series.
>>
>>
>>> Determining the connection between a DP and its Type-C connector is
>>> starting to get really important, so ideally we have a common solution
>>> for that.
>>
>> Yes. This is what we have been discussing with Simon for quite some time on
>> #dri-devel.
>>
>> Unfortunately I think the solution that got merged was pretty much hastened
>> in instead of being well-thought. For example, it is also not always
>> possible to provide the drm_connector / typec_connector links (as you can
>> see from the patch7. Sometimes we can only express that this is a Type-C DP
>> connector, but we can not easily point it to the particular USB-C port.
>>
>> So, I'm not sure, how can we proceed here. Currently merged patch breaks
>> drm/msm if we even try to use it by setting kdef->fwnode to
>> drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is
>> an ACPI-only thing, which is not expected to work in a non-ACPI cases.
> 
> You really have to always supply not only the Type-C ports and partners,
> but also the alt modes. You need them, firstly to keep things sane
> inside kernel, but more importantly, so they are always exposed to the
> user space, AND, always the same way. We have ABIs for all this stuff,
> including the DP alt mode. Use them. No shortcuts.
> 
> So here's what you need to do. UCSI does not seem to bring you
> anything useful, so just disable it for now. You don't need it. Your
> port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so
> that's where you need to register all these components - the ports,
> partners and alt modes. You have all the needed information there.

To make things even more complicate, UCSI is necessary for the USB part 
of the story. It handles vbus and direction.

> Only after you've done that we can start to look at how should the
> connection between the DPs and their USB Type-C connectors be handled.

But sure enough, I can add typec port registration to the altmode 
driver. This will solve the 'port not existing' part of the story.

I'd like to hear your opinion on:

- components. Using them breaks drm/msm. How can we proceed?

- PATH property usage. This way we make USB-C DisplayPort behave like 
the MST ports.

-- 
With best wishes
Dmitry

Re: [Freedreno] [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Rob Clark]

On Mon, Sep 11, 2023 at 2:15 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 06/09/2023 16:38, Heikki Krogerus wrote:
> > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
> >> On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
> >> <heikki.krogerus@linux.intel.com> wrote:
> >>>
> >>> On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
> >>>> Hi Heikki,
> >>>>
> >>>> On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
> >>>> <heikki.krogerus@linux.intel.com> wrote:
> >>>>>
> >>>>> Hi Dmitry,
> >>>>>
> >>>>> On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> >>>>>> The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> >>>>>> dev_fwnode() checks never succeed, making the respective commit NOP.
> >>>>>
> >>>>> That's not true. The dev->fwnode is assigned when the device is
> >>>>> created on ACPI platforms automatically. If the drm_connector fwnode
> >>>>> member is assigned before the device is registered, then that fwnode
> >>>>> is assigned also to the device - see drm_connector_acpi_find_companion().
> >>>>>
> >>>>> But please note that even if drm_connector does not have anything in
> >>>>> its fwnode member, the device may still be assigned fwnode, just based
> >>>>> on some other logic (maybe in drivers/acpi/acpi_video.c?).
> >>>>>
> >>>>>> And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> >>>>>> breaks drivers already using components (as it was pointed at [1]),
> >>>>>> resulting in a deadlock. Lockdep trace is provided below.
> >>>>>>
> >>>>>> Granted these two issues, it seems impractical to fix this commit in any
> >>>>>> sane way. Revert it instead.
> >>>>>
> >>>>> I think there is already user space stuff that relies on these links,
> >>>>> so I'm not sure you can just remove them like that. If the component
> >>>>> framework is not the correct tool here, then I think you need to
> >>>>> suggest some other way of creating them.
> >>>>
> >>>> The issue (that was pointed out during review) is that having a
> >>>> component code in the framework code can lead to lockups. With the
> >>>> patch #2 in place (which is the only logical way to set kdev->fwnode
> >>>> for non-ACPI systems) probing of drivers which use components and set
> >>>> drm_connector::fwnode breaks immediately.
> >>>>
> >>>> Can we move the component part to the respective drivers? With the
> >>>> patch 2 in place, connector->fwnode will be copied to the created
> >>>> kdev's fwnode pointer.
> >>>>
> >>>> Another option might be to make this drm_sysfs component registration optional.
> >>>
> >>> You don't need to use the component framework at all if there is
> >>> a better way of determining the connection between the DP and its
> >>> Type-C connector (I'm assuming that that's what this series is about).
> >>> You just need the symlinks, not the component.
> >>
> >> The problem is that right now this component registration has become
> >> mandatory. And if I set the kdev->fwnode manually (like in the patch
> >> 2), the kernel hangs inside the component code.
> >> That's why I proposed to move the components to the place where they
> >> are really necessary, e.g. i915 and amd drivers.
> >
> > So why can't we replace the component with the method you are
> > proposing in this series of finding out the Type-C port also with
> > i915, AMD, or whatever driver and platform (that's the only thing that
> > component is used for)?
>
> The drm/msm driver uses drm_bridge for the pipeline (including the last
> DP entry) and the drm_bridge_connector to create the connector. I think
> that enabling i915 and AMD drivers to use drm_bridge fells out of scope
> for this series.
>
>
> > Determining the connection between a DP and its Type-C connector is
> > starting to get really important, so ideally we have a common solution
> > for that.
>
> Yes. This is what we have been discussing with Simon for quite some time
> on #dri-devel.
>
> Unfortunately I think the solution that got merged was pretty much
> hastened in instead of being well-thought. For example, it is also not
> always possible to provide the drm_connector / typec_connector links (as
> you can see from the patch7. Sometimes we can only express that this is
> a Type-C DP connector, but we can not easily point it to the particular
> USB-C port.
>
> So, I'm not sure, how can we proceed here. Currently merged patch breaks
> drm/msm if we even try to use it by setting kdef->fwnode to
> drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c`
> is an ACPI-only thing, which is not expected to work in a non-ACPI cases.

In these cases we revert and try again next cycle

BR,
-R

>
> --
> With best wishes
> Dmitry
>

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Heikki Krogerus]

Hi Dmitry,

On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote:
> On 12/09/2023 14:05, Heikki Krogerus wrote:
> > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote:
> > > On 06/09/2023 16:38, Heikki Krogerus wrote:
> > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
> > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
> > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > 
> > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
> > > > > > > Hi Heikki,
> > > > > > > 
> > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
> > > > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > > > 
> > > > > > > > Hi Dmitry,
> > > > > > > > 
> > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> > > > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP.
> > > > > > > > 
> > > > > > > > That's not true. The dev->fwnode is assigned when the device is
> > > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode
> > > > > > > > member is assigned before the device is registered, then that fwnode
> > > > > > > > is assigned also to the device - see drm_connector_acpi_find_companion().
> > > > > > > > 
> > > > > > > > But please note that even if drm_connector does not have anything in
> > > > > > > > its fwnode member, the device may still be assigned fwnode, just based
> > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?).
> > > > > > > > 
> > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> > > > > > > > > breaks drivers already using components (as it was pointed at [1]),
> > > > > > > > > resulting in a deadlock. Lockdep trace is provided below.
> > > > > > > > > 
> > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any
> > > > > > > > > sane way. Revert it instead.
> > > > > > > > 
> > > > > > > > I think there is already user space stuff that relies on these links,
> > > > > > > > so I'm not sure you can just remove them like that. If the component
> > > > > > > > framework is not the correct tool here, then I think you need to
> > > > > > > > suggest some other way of creating them.
> > > > > > > 
> > > > > > > The issue (that was pointed out during review) is that having a
> > > > > > > component code in the framework code can lead to lockups. With the
> > > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode
> > > > > > > for non-ACPI systems) probing of drivers which use components and set
> > > > > > > drm_connector::fwnode breaks immediately.
> > > > > > > 
> > > > > > > Can we move the component part to the respective drivers? With the
> > > > > > > patch 2 in place, connector->fwnode will be copied to the created
> > > > > > > kdev's fwnode pointer.
> > > > > > > 
> > > > > > > Another option might be to make this drm_sysfs component registration optional.
> > > > > > 
> > > > > > You don't need to use the component framework at all if there is
> > > > > > a better way of determining the connection between the DP and its
> > > > > > Type-C connector (I'm assuming that that's what this series is about).
> > > > > > You just need the symlinks, not the component.
> > > > > 
> > > > > The problem is that right now this component registration has become
> > > > > mandatory. And if I set the kdev->fwnode manually (like in the patch
> > > > > 2), the kernel hangs inside the component code.
> > > > > That's why I proposed to move the components to the place where they
> > > > > are really necessary, e.g. i915 and amd drivers.
> > > > 
> > > > So why can't we replace the component with the method you are
> > > > proposing in this series of finding out the Type-C port also with
> > > > i915, AMD, or whatever driver and platform (that's the only thing that
> > > > component is used for)?
> > > 
> > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP
> > > entry) and the drm_bridge_connector to create the connector. I think that
> > > enabling i915 and AMD drivers to use drm_bridge fells out of scope for this
> > > series.
> > > 
> > > 
> > > > Determining the connection between a DP and its Type-C connector is
> > > > starting to get really important, so ideally we have a common solution
> > > > for that.
> > > 
> > > Yes. This is what we have been discussing with Simon for quite some time on
> > > #dri-devel.
> > > 
> > > Unfortunately I think the solution that got merged was pretty much hastened
> > > in instead of being well-thought. For example, it is also not always
> > > possible to provide the drm_connector / typec_connector links (as you can
> > > see from the patch7. Sometimes we can only express that this is a Type-C DP
> > > connector, but we can not easily point it to the particular USB-C port.
> > > 
> > > So, I'm not sure, how can we proceed here. Currently merged patch breaks
> > > drm/msm if we even try to use it by setting kdef->fwnode to
> > > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is
> > > an ACPI-only thing, which is not expected to work in a non-ACPI cases.
> > 
> > You really have to always supply not only the Type-C ports and partners,
> > but also the alt modes. You need them, firstly to keep things sane
> > inside kernel, but more importantly, so they are always exposed to the
> > user space, AND, always the same way. We have ABIs for all this stuff,
> > including the DP alt mode. Use them. No shortcuts.
> > 
> > So here's what you need to do. UCSI does not seem to bring you
> > anything useful, so just disable it for now. You don't need it. Your
> > port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so
> > that's where you need to register all these components - the ports,
> > partners and alt modes. You have all the needed information there.
> 
> To make things even more complicate, UCSI is necessary for the USB part of
> the story. It handles vbus and direction.
> 
> > Only after you've done that we can start to look at how should the
> > connection between the DPs and their USB Type-C connectors be handled.
> 
> But sure enough, I can add typec port registration to the altmode driver.
> This will solve the 'port not existing' part of the story.
> 
> I'd like to hear your opinion on:
> 
> - components. Using them breaks drm/msm. How can we proceed?

I don't think replacing the components is going to be a problem once
you have described everything properly in you DT. I'm fairly certain now
that that is the main problem here. You don't have this connection
described in your DT as it should.

> - PATH property usage. This way we make USB-C DisplayPort behave like the
> MST ports.

That looks to me like an attempt to exploit a feature that is not
designed for this purposes at all. Just drop all that.

The connection has to be first described in your DT, and the way you
usually describe connections in DT is by using the device graph (OF
graph). It seems that you have everything needed for that - the USB
Type-C connectors have their own OF nodes (what you register as
drm_bridges are in fact USB Type-C connectors), and presumable you
also have OF nodes for all your video ports (DisplayPorts) - so
applying the graph between the two really should not be a problem. The
DP is endpoint for the USB Type-C connector, and vice versa.

After you have everything needed in your DT, the problem here isn't
actually much of a problem at all. We will have options how to move
forward after that.

Br,

-- 
heikki

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Neil Armstrong]

On 12/09/2023 19:39, Dmitry Baryshkov wrote:
> On 12/09/2023 14:05, Heikki Krogerus wrote:
>> On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote:
>>> On 06/09/2023 16:38, Heikki Krogerus wrote:
>>>> On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
>>>>> On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
>>>>> <heikki.krogerus@linux.intel.com> wrote:
>>>>>>
>>>>>> On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
>>>>>>> Hi Heikki,
>>>>>>>
>>>>>>> On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
>>>>>>> <heikki.krogerus@linux.intel.com> wrote:
>>>>>>>>
>>>>>>>> Hi Dmitry,
>>>>>>>>
>>>>>>>> On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
>>>>>>>>> The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
>>>>>>>>> dev_fwnode() checks never succeed, making the respective commit NOP.
>>>>>>>>
>>>>>>>> That's not true. The dev->fwnode is assigned when the device is
>>>>>>>> created on ACPI platforms automatically. If the drm_connector fwnode
>>>>>>>> member is assigned before the device is registered, then that fwnode
>>>>>>>> is assigned also to the device - see drm_connector_acpi_find_companion().
>>>>>>>>
>>>>>>>> But please note that even if drm_connector does not have anything in
>>>>>>>> its fwnode member, the device may still be assigned fwnode, just based
>>>>>>>> on some other logic (maybe in drivers/acpi/acpi_video.c?).
>>>>>>>>
>>>>>>>>> And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
>>>>>>>>> breaks drivers already using components (as it was pointed at [1]),
>>>>>>>>> resulting in a deadlock. Lockdep trace is provided below.
>>>>>>>>>
>>>>>>>>> Granted these two issues, it seems impractical to fix this commit in any
>>>>>>>>> sane way. Revert it instead.
>>>>>>>>
>>>>>>>> I think there is already user space stuff that relies on these links,
>>>>>>>> so I'm not sure you can just remove them like that. If the component
>>>>>>>> framework is not the correct tool here, then I think you need to
>>>>>>>> suggest some other way of creating them.
>>>>>>>
>>>>>>> The issue (that was pointed out during review) is that having a
>>>>>>> component code in the framework code can lead to lockups. With the
>>>>>>> patch #2 in place (which is the only logical way to set kdev->fwnode
>>>>>>> for non-ACPI systems) probing of drivers which use components and set
>>>>>>> drm_connector::fwnode breaks immediately.
>>>>>>>
>>>>>>> Can we move the component part to the respective drivers? With the
>>>>>>> patch 2 in place, connector->fwnode will be copied to the created
>>>>>>> kdev's fwnode pointer.
>>>>>>>
>>>>>>> Another option might be to make this drm_sysfs component registration optional.
>>>>>>
>>>>>> You don't need to use the component framework at all if there is
>>>>>> a better way of determining the connection between the DP and its
>>>>>> Type-C connector (I'm assuming that that's what this series is about).
>>>>>> You just need the symlinks, not the component.
>>>>>
>>>>> The problem is that right now this component registration has become
>>>>> mandatory. And if I set the kdev->fwnode manually (like in the patch
>>>>> 2), the kernel hangs inside the component code.
>>>>> That's why I proposed to move the components to the place where they
>>>>> are really necessary, e.g. i915 and amd drivers.
>>>>
>>>> So why can't we replace the component with the method you are
>>>> proposing in this series of finding out the Type-C port also with
>>>> i915, AMD, or whatever driver and platform (that's the only thing that
>>>> component is used for)?
>>>
>>> The drm/msm driver uses drm_bridge for the pipeline (including the last DP
>>> entry) and the drm_bridge_connector to create the connector. I think that
>>> enabling i915 and AMD drivers to use drm_bridge fells out of scope for this
>>> series.
>>>
>>>
>>>> Determining the connection between a DP and its Type-C connector is
>>>> starting to get really important, so ideally we have a common solution
>>>> for that.
>>>
>>> Yes. This is what we have been discussing with Simon for quite some time on
>>> #dri-devel.
>>>
>>> Unfortunately I think the solution that got merged was pretty much hastened
>>> in instead of being well-thought. For example, it is also not always
>>> possible to provide the drm_connector / typec_connector links (as you can
>>> see from the patch7. Sometimes we can only express that this is a Type-C DP
>>> connector, but we can not easily point it to the particular USB-C port.
>>>
>>> So, I'm not sure, how can we proceed here. Currently merged patch breaks
>>> drm/msm if we even try to use it by setting kdef->fwnode to
>>> drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is
>>> an ACPI-only thing, which is not expected to work in a non-ACPI cases.
>>
>> You really have to always supply not only the Type-C ports and partners,
>> but also the alt modes. You need them, firstly to keep things sane
>> inside kernel, but more importantly, so they are always exposed to the
>> user space, AND, always the same way. We have ABIs for all this stuff,
>> including the DP alt mode. Use them. No shortcuts.
>>
>> So here's what you need to do. UCSI does not seem to bring you
>> anything useful, so just disable it for now. You don't need it. Your
>> port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so
>> that's where you need to register all these components - the ports,
>> partners and alt modes. You have all the needed information there.
> 
> To make things even more complicate, UCSI is necessary for the USB part of the story. It handles vbus and direction.

On new platforms (starting from SM8450) UCSI is mandatory to have pmic_glink_altmode events triggering.

Neil

> 
>> Only after you've done that we can start to look at how should the
>> connection between the DPs and their USB Type-C connectors be handled.
> 
> But sure enough, I can add typec port registration to the altmode driver. This will solve the 'port not existing' part of the story.
> 
> I'd like to hear your opinion on:
> 
> - components. Using them breaks drm/msm. How can we proceed?
> 
> - PATH property usage. This way we make USB-C DisplayPort behave like the MST ports.
> 

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Dmitry Baryshkov]

Hi Heikki,

On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote:
> > On 12/09/2023 14:05, Heikki Krogerus wrote:
> > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote:
> > > > On 06/09/2023 16:38, Heikki Krogerus wrote:
> > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
> > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
> > > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > >
> > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
> > > > > > > > Hi Heikki,
> > > > > > > >
> > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
> > > > > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Dmitry,
> > > > > > > > >
> > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> > > > > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP.
> > > > > > > > >
> > > > > > > > > That's not true. The dev->fwnode is assigned when the device is
> > > > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode
> > > > > > > > > member is assigned before the device is registered, then that fwnode
> > > > > > > > > is assigned also to the device - see drm_connector_acpi_find_companion().
> > > > > > > > >
> > > > > > > > > But please note that even if drm_connector does not have anything in
> > > > > > > > > its fwnode member, the device may still be assigned fwnode, just based
> > > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?).
> > > > > > > > >
> > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> > > > > > > > > > breaks drivers already using components (as it was pointed at [1]),
> > > > > > > > > > resulting in a deadlock. Lockdep trace is provided below.
> > > > > > > > > >
> > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any
> > > > > > > > > > sane way. Revert it instead.
> > > > > > > > >
> > > > > > > > > I think there is already user space stuff that relies on these links,
> > > > > > > > > so I'm not sure you can just remove them like that. If the component
> > > > > > > > > framework is not the correct tool here, then I think you need to
> > > > > > > > > suggest some other way of creating them.
> > > > > > > >
> > > > > > > > The issue (that was pointed out during review) is that having a
> > > > > > > > component code in the framework code can lead to lockups. With the
> > > > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode
> > > > > > > > for non-ACPI systems) probing of drivers which use components and set
> > > > > > > > drm_connector::fwnode breaks immediately.
> > > > > > > >
> > > > > > > > Can we move the component part to the respective drivers? With the
> > > > > > > > patch 2 in place, connector->fwnode will be copied to the created
> > > > > > > > kdev's fwnode pointer.
> > > > > > > >
> > > > > > > > Another option might be to make this drm_sysfs component registration optional.
> > > > > > >
> > > > > > > You don't need to use the component framework at all if there is
> > > > > > > a better way of determining the connection between the DP and its
> > > > > > > Type-C connector (I'm assuming that that's what this series is about).
> > > > > > > You just need the symlinks, not the component.
> > > > > >
> > > > > > The problem is that right now this component registration has become
> > > > > > mandatory. And if I set the kdev->fwnode manually (like in the patch
> > > > > > 2), the kernel hangs inside the component code.
> > > > > > That's why I proposed to move the components to the place where they
> > > > > > are really necessary, e.g. i915 and amd drivers.
> > > > >
> > > > > So why can't we replace the component with the method you are
> > > > > proposing in this series of finding out the Type-C port also with
> > > > > i915, AMD, or whatever driver and platform (that's the only thing that
> > > > > component is used for)?
> > > >
> > > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP
> > > > entry) and the drm_bridge_connector to create the connector. I think that
> > > > enabling i915 and AMD drivers to use drm_bridge fells out of scope for this
> > > > series.
> > > >
> > > >
> > > > > Determining the connection between a DP and its Type-C connector is
> > > > > starting to get really important, so ideally we have a common solution
> > > > > for that.
> > > >
> > > > Yes. This is what we have been discussing with Simon for quite some time on
> > > > #dri-devel.
> > > >
> > > > Unfortunately I think the solution that got merged was pretty much hastened
> > > > in instead of being well-thought. For example, it is also not always
> > > > possible to provide the drm_connector / typec_connector links (as you can
> > > > see from the patch7. Sometimes we can only express that this is a Type-C DP
> > > > connector, but we can not easily point it to the particular USB-C port.
> > > >
> > > > So, I'm not sure, how can we proceed here. Currently merged patch breaks
> > > > drm/msm if we even try to use it by setting kdef->fwnode to
> > > > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is
> > > > an ACPI-only thing, which is not expected to work in a non-ACPI cases.
> > >
> > > You really have to always supply not only the Type-C ports and partners,
> > > but also the alt modes. You need them, firstly to keep things sane
> > > inside kernel, but more importantly, so they are always exposed to the
> > > user space, AND, always the same way. We have ABIs for all this stuff,
> > > including the DP alt mode. Use them. No shortcuts.
> > >
> > > So here's what you need to do. UCSI does not seem to bring you
> > > anything useful, so just disable it for now. You don't need it. Your
> > > port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so
> > > that's where you need to register all these components - the ports,
> > > partners and alt modes. You have all the needed information there.
> >
> > To make things even more complicate, UCSI is necessary for the USB part of
> > the story. It handles vbus and direction.
> >
> > > Only after you've done that we can start to look at how should the
> > > connection between the DPs and their USB Type-C connectors be handled.
> >
> > But sure enough, I can add typec port registration to the altmode driver.
> > This will solve the 'port not existing' part of the story.
> >
> > I'd like to hear your opinion on:
> >
> > - components. Using them breaks drm/msm. How can we proceed?
>
> I don't think replacing the components is going to be a problem once
> you have described everything properly in you DT. I'm fairly certain now
> that that is the main problem here. You don't have this connection
> described in your DT as it should.

We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/
(for non-PMIC-GLINK platform)
Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full
description of USB-C connector and signal flow.

In fact, thanks to this representation I can properly set
'connector->fwnode' to point to the OF node corresponding to the
connector's drm_bridge. I can even propagate it to the kdef->fwnode /
kdev->of_node in drm_sysfs_connector_add(). But then a component_add()
call looks the kernel up.

And to add on top of that, here is another reason why I think that
this sysfs links ABI/implementation was not well thought. The
typec_connector_ops are added to all fwnode-enabled connector devices.
It doesn't even bother checking that the device is really the DP
connector and that the device on the other side of fwnode link is a
typec port device. The symlink is named 'typec_connector', so one can
not easily extend this ABI to support SlimPort aka MyDP (which uses
micro-USB-B connectors instead of USB-C). Neither can we extend it to
represent MHL connections (again, micro-USB-B).

> > - PATH property usage. This way we make USB-C DisplayPort behave like the
> > MST ports.
>
> That looks to me like an attempt to exploit a feature that is not
> designed for this purposes at all. Just drop all that.

But why? From the docs: 'Connector path property to identify how this
sink is physically connected.'

So far we have been using it for MST only. But the description above
also suits properly for the 'connected to the Type-C port0 device'
kind of data. Then the userspace can use this property to change the
representation of the controller. Or to rename it as it does for
DP-MST connectors. Or just add the USB-C icon in the UI.

Having this data in sysfs only requires userspace first to map the
connector to the device under sysfs (which is not trivial since Xorg
renames DP-MST connectors), then to look for the symlink value. Quite
complicated compared to checking the DRM property.

Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the
schema to support 'microusb:something' values for this property.

> The connection has to be first described in your DT, and the way you
> usually describe connections in DT is by using the device graph (OF
> graph). It seems that you have everything needed for that - the USB
> Type-C connectors have their own OF nodes (what you register as
> drm_bridges are in fact USB Type-C connectors), and presumable you
> also have OF nodes for all your video ports (DisplayPorts) - so
> applying the graph between the two really should not be a problem. The
> DP is endpoint for the USB Type-C connector, and vice versa.

Not quite. There is no direct connection between the USB Type-C
connector and DP controller. The USB-C connector has three ports.

port@0 goes to theHS-USB controller. This is simple.

port@1 goes to the USB+DP PHY. All retimers and SS line muxes are
included in between. And it is the USB+DP PHY that is connected to the
DP and USB-SS controllers.

port@2 goes to SBU lines mux (e.g. fsa4480).

> After you have everything needed in your DT, the problem here isn't
> actually much of a problem at all. We will have options how to move
> forward after that.

Could you please describe what is missing there?

-- 
With best wishes
Dmitry

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Heikki Krogerus]

Hi Neil,

On Wed, Sep 13, 2023 at 11:38:19AM +0200, Neil Armstrong wrote:
> On new platforms (starting from SM8450) UCSI is mandatory to have
> pmic_glink_altmode events triggering.

You can also populate the typec devices conditionally, only if UCSI is
not supported.

However, I took a peek at drivers/soc/qcom/pmic_glink_altmode.c, and
it seems to be mostly is dealing with the muxes and retimer, and
sending the HPD notifications to the DRM side. All that is already
done in typec drivers, so there is actually a potential race here when
UCSI is used.

On top of that, it is sending two commands to the PMIC (ALTMODE_PAN_EN
and ALTMODE_PAN_ACK). I'm pretty sure both could be handled in the UCSI
glue driver (drivers/usb/typec/ucsi/ucsi_glink.c) if they are even
needed when UCSI is supported.

So why do you need that pmic_glibk_altmode driver at all when UCSI is
supported?

I don't know the hardware, so I may be missing something.

thanks,

-- 
heikki

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Heikki Krogerus]

On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote:
> Hi Heikki,
> 
> On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> > On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote:
> > > On 12/09/2023 14:05, Heikki Krogerus wrote:
> > > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote:
> > > > > On 06/09/2023 16:38, Heikki Krogerus wrote:
> > > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
> > > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
> > > > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > > >
> > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > Hi Heikki,
> > > > > > > > >
> > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
> > > > > > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Dmitry,
> > > > > > > > > >
> > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> > > > > > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP.
> > > > > > > > > >
> > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is
> > > > > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode
> > > > > > > > > > member is assigned before the device is registered, then that fwnode
> > > > > > > > > > is assigned also to the device - see drm_connector_acpi_find_companion().
> > > > > > > > > >
> > > > > > > > > > But please note that even if drm_connector does not have anything in
> > > > > > > > > > its fwnode member, the device may still be assigned fwnode, just based
> > > > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?).
> > > > > > > > > >
> > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> > > > > > > > > > > breaks drivers already using components (as it was pointed at [1]),
> > > > > > > > > > > resulting in a deadlock. Lockdep trace is provided below.
> > > > > > > > > > >
> > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any
> > > > > > > > > > > sane way. Revert it instead.
> > > > > > > > > >
> > > > > > > > > > I think there is already user space stuff that relies on these links,
> > > > > > > > > > so I'm not sure you can just remove them like that. If the component
> > > > > > > > > > framework is not the correct tool here, then I think you need to
> > > > > > > > > > suggest some other way of creating them.
> > > > > > > > >
> > > > > > > > > The issue (that was pointed out during review) is that having a
> > > > > > > > > component code in the framework code can lead to lockups. With the
> > > > > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode
> > > > > > > > > for non-ACPI systems) probing of drivers which use components and set
> > > > > > > > > drm_connector::fwnode breaks immediately.
> > > > > > > > >
> > > > > > > > > Can we move the component part to the respective drivers? With the
> > > > > > > > > patch 2 in place, connector->fwnode will be copied to the created
> > > > > > > > > kdev's fwnode pointer.
> > > > > > > > >
> > > > > > > > > Another option might be to make this drm_sysfs component registration optional.
> > > > > > > >
> > > > > > > > You don't need to use the component framework at all if there is
> > > > > > > > a better way of determining the connection between the DP and its
> > > > > > > > Type-C connector (I'm assuming that that's what this series is about).
> > > > > > > > You just need the symlinks, not the component.
> > > > > > >
> > > > > > > The problem is that right now this component registration has become
> > > > > > > mandatory. And if I set the kdev->fwnode manually (like in the patch
> > > > > > > 2), the kernel hangs inside the component code.
> > > > > > > That's why I proposed to move the components to the place where they
> > > > > > > are really necessary, e.g. i915 and amd drivers.
> > > > > >
> > > > > > So why can't we replace the component with the method you are
> > > > > > proposing in this series of finding out the Type-C port also with
> > > > > > i915, AMD, or whatever driver and platform (that's the only thing that
> > > > > > component is used for)?
> > > > >
> > > > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP
> > > > > entry) and the drm_bridge_connector to create the connector. I think that
> > > > > enabling i915 and AMD drivers to use drm_bridge fells out of scope for this
> > > > > series.
> > > > >
> > > > >
> > > > > > Determining the connection between a DP and its Type-C connector is
> > > > > > starting to get really important, so ideally we have a common solution
> > > > > > for that.
> > > > >
> > > > > Yes. This is what we have been discussing with Simon for quite some time on
> > > > > #dri-devel.
> > > > >
> > > > > Unfortunately I think the solution that got merged was pretty much hastened
> > > > > in instead of being well-thought. For example, it is also not always
> > > > > possible to provide the drm_connector / typec_connector links (as you can
> > > > > see from the patch7. Sometimes we can only express that this is a Type-C DP
> > > > > connector, but we can not easily point it to the particular USB-C port.
> > > > >
> > > > > So, I'm not sure, how can we proceed here. Currently merged patch breaks
> > > > > drm/msm if we even try to use it by setting kdef->fwnode to
> > > > > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is
> > > > > an ACPI-only thing, which is not expected to work in a non-ACPI cases.
> > > >
> > > > You really have to always supply not only the Type-C ports and partners,
> > > > but also the alt modes. You need them, firstly to keep things sane
> > > > inside kernel, but more importantly, so they are always exposed to the
> > > > user space, AND, always the same way. We have ABIs for all this stuff,
> > > > including the DP alt mode. Use them. No shortcuts.
> > > >
> > > > So here's what you need to do. UCSI does not seem to bring you
> > > > anything useful, so just disable it for now. You don't need it. Your
> > > > port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so
> > > > that's where you need to register all these components - the ports,
> > > > partners and alt modes. You have all the needed information there.
> > >
> > > To make things even more complicate, UCSI is necessary for the USB part of
> > > the story. It handles vbus and direction.
> > >
> > > > Only after you've done that we can start to look at how should the
> > > > connection between the DPs and their USB Type-C connectors be handled.
> > >
> > > But sure enough, I can add typec port registration to the altmode driver.
> > > This will solve the 'port not existing' part of the story.
> > >
> > > I'd like to hear your opinion on:
> > >
> > > - components. Using them breaks drm/msm. How can we proceed?
> >
> > I don't think replacing the components is going to be a problem once
> > you have described everything properly in you DT. I'm fairly certain now
> > that that is the main problem here. You don't have this connection
> > described in your DT as it should.
> 
> We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/
> (for non-PMIC-GLINK platform)
> Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full
> description of USB-C connector and signal flow.
> 
> In fact, thanks to this representation I can properly set
> 'connector->fwnode' to point to the OF node corresponding to the
> connector's drm_bridge. I can even propagate it to the kdef->fwnode /
> kdev->of_node in drm_sysfs_connector_add(). But then a component_add()
> call looks the kernel up.
> 
> And to add on top of that, here is another reason why I think that
> this sysfs links ABI/implementation was not well thought. The
> typec_connector_ops are added to all fwnode-enabled connector devices.
> It doesn't even bother checking that the device is really the DP
> connector and that the device on the other side of fwnode link is a
> typec port device. The symlink is named 'typec_connector', so one can
> not easily extend this ABI to support SlimPort aka MyDP (which uses
> micro-USB-B connectors instead of USB-C). Neither can we extend it to
> represent MHL connections (again, micro-USB-B).
> 
> > > - PATH property usage. This way we make USB-C DisplayPort behave like the
> > > MST ports.
> >
> > That looks to me like an attempt to exploit a feature that is not
> > designed for this purposes at all. Just drop all that.
> 
> But why? From the docs: 'Connector path property to identify how this
> sink is physically connected.'
> 
> So far we have been using it for MST only. But the description above
> also suits properly for the 'connected to the Type-C port0 device'
> kind of data. Then the userspace can use this property to change the
> representation of the controller. Or to rename it as it does for
> DP-MST connectors. Or just add the USB-C icon in the UI.
> 
> Having this data in sysfs only requires userspace first to map the
> connector to the device under sysfs (which is not trivial since Xorg
> renames DP-MST connectors), then to look for the symlink value. Quite
> complicated compared to checking the DRM property.
> 
> Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the
> schema to support 'microusb:something' values for this property.
> 
> > The connection has to be first described in your DT, and the way you
> > usually describe connections in DT is by using the device graph (OF
> > graph). It seems that you have everything needed for that - the USB
> > Type-C connectors have their own OF nodes (what you register as
> > drm_bridges are in fact USB Type-C connectors), and presumable you
> > also have OF nodes for all your video ports (DisplayPorts) - so
> > applying the graph between the two really should not be a problem. The
> > DP is endpoint for the USB Type-C connector, and vice versa.
> 
> Not quite. There is no direct connection between the USB Type-C
> connector and DP controller. The USB-C connector has three ports.
> 
> port@0 goes to theHS-USB controller. This is simple.
> 
> port@1 goes to the USB+DP PHY. All retimers and SS line muxes are
> included in between. And it is the USB+DP PHY that is connected to the
> DP and USB-SS controllers.
> 
> port@2 goes to SBU lines mux (e.g. fsa4480).
> 
> > After you have everything needed in your DT, the problem here isn't
> > actually much of a problem at all. We will have options how to move
> > forward after that.
> 
> Could you please describe what is missing there?

We are not after the direct connections here, we are after the final
endpoints. So you are missing description of the logical connection
between your DP and Type-C connector.

I understand that the idea is to build the graph to describe only the
physical connections, but with just the physical connections you are
doomed to write separate software solution for almost every single
platform, even though the final endpoints are always the same (DP to
Type-C). You just can not generalise the components (muxes, phys,
retimers, etc.) behind USB Type-C connectors (or anything else for
that matter), it's not possible. The components and their order vary
on almost every single platform. On some platforms the stack of parts
after the connector is also incredibly complex.

Having the logical final endpoint connection described in your DT/ACPI
on top of the physical connections costs very little, but at the same
time it's usually the only thing that the software needs (like in this
case).

So, either you add one more port to your graph for the DP to Type-C
connection, or, if that's not an option, then you need to describe
that connection in some other way. Named references work also quite
well in my experience.

Br,

-- 
heikki

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Dmitry Baryshkov]

On Wed, 13 Sept 2023 at 16:15, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote:
> > Hi Heikki,
> >
> > On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > > On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote:
> > > > On 12/09/2023 14:05, Heikki Krogerus wrote:
> > > > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote:
> > > > > > On 06/09/2023 16:38, Heikki Krogerus wrote:
> > > > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
> > > > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
> > > > > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > > Hi Heikki,
> > > > > > > > > >
> > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
> > > > > > > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Dmitry,
> > > > > > > > > > >
> > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> > > > > > > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP.
> > > > > > > > > > >
> > > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is
> > > > > > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode
> > > > > > > > > > > member is assigned before the device is registered, then that fwnode
> > > > > > > > > > > is assigned also to the device - see drm_connector_acpi_find_companion().
> > > > > > > > > > >
> > > > > > > > > > > But please note that even if drm_connector does not have anything in
> > > > > > > > > > > its fwnode member, the device may still be assigned fwnode, just based
> > > > > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?).
> > > > > > > > > > >
> > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> > > > > > > > > > > > breaks drivers already using components (as it was pointed at [1]),
> > > > > > > > > > > > resulting in a deadlock. Lockdep trace is provided below.
> > > > > > > > > > > >
> > > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any
> > > > > > > > > > > > sane way. Revert it instead.
> > > > > > > > > > >
> > > > > > > > > > > I think there is already user space stuff that relies on these links,
> > > > > > > > > > > so I'm not sure you can just remove them like that. If the component
> > > > > > > > > > > framework is not the correct tool here, then I think you need to
> > > > > > > > > > > suggest some other way of creating them.
> > > > > > > > > >
> > > > > > > > > > The issue (that was pointed out during review) is that having a
> > > > > > > > > > component code in the framework code can lead to lockups. With the
> > > > > > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode
> > > > > > > > > > for non-ACPI systems) probing of drivers which use components and set
> > > > > > > > > > drm_connector::fwnode breaks immediately.
> > > > > > > > > >
> > > > > > > > > > Can we move the component part to the respective drivers? With the
> > > > > > > > > > patch 2 in place, connector->fwnode will be copied to the created
> > > > > > > > > > kdev's fwnode pointer.
> > > > > > > > > >
> > > > > > > > > > Another option might be to make this drm_sysfs component registration optional.
> > > > > > > > >
> > > > > > > > > You don't need to use the component framework at all if there is
> > > > > > > > > a better way of determining the connection between the DP and its
> > > > > > > > > Type-C connector (I'm assuming that that's what this series is about).
> > > > > > > > > You just need the symlinks, not the component.
> > > > > > > >
> > > > > > > > The problem is that right now this component registration has become
> > > > > > > > mandatory. And if I set the kdev->fwnode manually (like in the patch
> > > > > > > > 2), the kernel hangs inside the component code.
> > > > > > > > That's why I proposed to move the components to the place where they
> > > > > > > > are really necessary, e.g. i915 and amd drivers.
> > > > > > >
> > > > > > > So why can't we replace the component with the method you are
> > > > > > > proposing in this series of finding out the Type-C port also with
> > > > > > > i915, AMD, or whatever driver and platform (that's the only thing that
> > > > > > > component is used for)?
> > > > > >
> > > > > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP
> > > > > > entry) and the drm_bridge_connector to create the connector. I think that
> > > > > > enabling i915 and AMD drivers to use drm_bridge fells out of scope for this
> > > > > > series.
> > > > > >
> > > > > >
> > > > > > > Determining the connection between a DP and its Type-C connector is
> > > > > > > starting to get really important, so ideally we have a common solution
> > > > > > > for that.
> > > > > >
> > > > > > Yes. This is what we have been discussing with Simon for quite some time on
> > > > > > #dri-devel.
> > > > > >
> > > > > > Unfortunately I think the solution that got merged was pretty much hastened
> > > > > > in instead of being well-thought. For example, it is also not always
> > > > > > possible to provide the drm_connector / typec_connector links (as you can
> > > > > > see from the patch7. Sometimes we can only express that this is a Type-C DP
> > > > > > connector, but we can not easily point it to the particular USB-C port.
> > > > > >
> > > > > > So, I'm not sure, how can we proceed here. Currently merged patch breaks
> > > > > > drm/msm if we even try to use it by setting kdef->fwnode to
> > > > > > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is
> > > > > > an ACPI-only thing, which is not expected to work in a non-ACPI cases.
> > > > >
> > > > > You really have to always supply not only the Type-C ports and partners,
> > > > > but also the alt modes. You need them, firstly to keep things sane
> > > > > inside kernel, but more importantly, so they are always exposed to the
> > > > > user space, AND, always the same way. We have ABIs for all this stuff,
> > > > > including the DP alt mode. Use them. No shortcuts.
> > > > >
> > > > > So here's what you need to do. UCSI does not seem to bring you
> > > > > anything useful, so just disable it for now. You don't need it. Your
> > > > > port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so
> > > > > that's where you need to register all these components - the ports,
> > > > > partners and alt modes. You have all the needed information there.
> > > >
> > > > To make things even more complicate, UCSI is necessary for the USB part of
> > > > the story. It handles vbus and direction.
> > > >
> > > > > Only after you've done that we can start to look at how should the
> > > > > connection between the DPs and their USB Type-C connectors be handled.
> > > >
> > > > But sure enough, I can add typec port registration to the altmode driver.
> > > > This will solve the 'port not existing' part of the story.
> > > >
> > > > I'd like to hear your opinion on:
> > > >
> > > > - components. Using them breaks drm/msm. How can we proceed?
> > >
> > > I don't think replacing the components is going to be a problem once
> > > you have described everything properly in you DT. I'm fairly certain now
> > > that that is the main problem here. You don't have this connection
> > > described in your DT as it should.
> >
> > We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/
> > (for non-PMIC-GLINK platform)
> > Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full
> > description of USB-C connector and signal flow.
> >
> > In fact, thanks to this representation I can properly set
> > 'connector->fwnode' to point to the OF node corresponding to the
> > connector's drm_bridge. I can even propagate it to the kdef->fwnode /
> > kdev->of_node in drm_sysfs_connector_add(). But then a component_add()
> > call looks the kernel up.
> >
> > And to add on top of that, here is another reason why I think that
> > this sysfs links ABI/implementation was not well thought. The
> > typec_connector_ops are added to all fwnode-enabled connector devices.
> > It doesn't even bother checking that the device is really the DP
> > connector and that the device on the other side of fwnode link is a
> > typec port device. The symlink is named 'typec_connector', so one can
> > not easily extend this ABI to support SlimPort aka MyDP (which uses
> > micro-USB-B connectors instead of USB-C). Neither can we extend it to
> > represent MHL connections (again, micro-USB-B).
> >
> > > > - PATH property usage. This way we make USB-C DisplayPort behave like the
> > > > MST ports.
> > >
> > > That looks to me like an attempt to exploit a feature that is not
> > > designed for this purposes at all. Just drop all that.
> >
> > But why? From the docs: 'Connector path property to identify how this
> > sink is physically connected.'
> >
> > So far we have been using it for MST only. But the description above
> > also suits properly for the 'connected to the Type-C port0 device'
> > kind of data. Then the userspace can use this property to change the
> > representation of the controller. Or to rename it as it does for
> > DP-MST connectors. Or just add the USB-C icon in the UI.
> >
> > Having this data in sysfs only requires userspace first to map the
> > connector to the device under sysfs (which is not trivial since Xorg
> > renames DP-MST connectors), then to look for the symlink value. Quite
> > complicated compared to checking the DRM property.
> >
> > Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the
> > schema to support 'microusb:something' values for this property.
> >
> > > The connection has to be first described in your DT, and the way you
> > > usually describe connections in DT is by using the device graph (OF
> > > graph). It seems that you have everything needed for that - the USB
> > > Type-C connectors have their own OF nodes (what you register as
> > > drm_bridges are in fact USB Type-C connectors), and presumable you
> > > also have OF nodes for all your video ports (DisplayPorts) - so
> > > applying the graph between the two really should not be a problem. The
> > > DP is endpoint for the USB Type-C connector, and vice versa.
> >
> > Not quite. There is no direct connection between the USB Type-C
> > connector and DP controller. The USB-C connector has three ports.
> >
> > port@0 goes to theHS-USB controller. This is simple.
> >
> > port@1 goes to the USB+DP PHY. All retimers and SS line muxes are
> > included in between. And it is the USB+DP PHY that is connected to the
> > DP and USB-SS controllers.
> >
> > port@2 goes to SBU lines mux (e.g. fsa4480).
> >
> > > After you have everything needed in your DT, the problem here isn't
> > > actually much of a problem at all. We will have options how to move
> > > forward after that.
> >
> > Could you please describe what is missing there?
>
> We are not after the direct connections here, we are after the final
> endpoints. So you are missing description of the logical connection
> between your DP and Type-C connector.

Adding Krzysztof, as one of DT maintainers, to the CC list.

From what I understand, DT describes hardware. There is no description
for the 'logical' connections.

>
> I understand that the idea is to build the graph to describe only the
> physical connections, but with just the physical connections you are
> doomed to write separate software solution for almost every single
> platform, even though the final endpoints are always the same (DP to
> Type-C). You just can not generalise the components (muxes, phys,
> retimers, etc.) behind USB Type-C connectors (or anything else for
> that matter), it's not possible. The components and their order vary
> on almost every single platform. On some platforms the stack of parts
> after the connector is also incredibly complex.

Yes. And this is why we have a chain of DRM bridges, going from the DP
controller to the final drm_bridge at the Type-C port manager. When
there is the altmode event, it gets sent via this chain using the
normal DRM HPD event.

> Having the logical final endpoint connection described in your DT/ACPI
> on top of the physical connections costs very little, but at the same
> time it's usually the only thing that the software needs (like in this
> case).

Maybe there is some misunderstanding here. We have this connection. We
have connector->fwnode and connector->of_node pointing to the correct
device - the last bridge in the chain. Each TCPM driver knows the
relationship between the in-built drm_bridge and the Type-C port. The
DP host controller port can be terminated with other endpoints, e.g.
eDP panel. Or there can be a non-DP host, which is then connected
through a series of bridges to the eDP or external DP port. This is
what anx78xx bridge does: it converts the HDMI link into an external
DP (SlimPort) connection. Bridge chains permit this to be handled in a
seamless way.

What you are asking for looks like a step backwards to me: it requires
the host to know that there is a USB-C connector.

>
> So, either you add one more port to your graph for the DP to Type-C
> connection, or, if that's not an option, then you need to describe
> that connection in some other way. Named references work also quite
> well in my experience.

Named references were considered and frowned upon by DT maintainers.

-- 
With best wishes
Dmitry

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Heikki Krogerus]

Hi Dmitry,

On Wed, Sep 13, 2023 at 04:47:12PM +0300, Dmitry Baryshkov wrote:
> On Wed, 13 Sept 2023 at 16:15, Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote:
> > > Hi Heikki,
> > >
> > > On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > > On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote:
> > > > > On 12/09/2023 14:05, Heikki Krogerus wrote:
> > > > > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote:
> > > > > > > On 06/09/2023 16:38, Heikki Krogerus wrote:
> > > > > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
> > > > > > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > > > Hi Heikki,
> > > > > > > > > > >
> > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
> > > > > > > > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Dmitry,
> > > > > > > > > > > >
> > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> > > > > > > > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP.
> > > > > > > > > > > >
> > > > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is
> > > > > > > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode
> > > > > > > > > > > > member is assigned before the device is registered, then that fwnode
> > > > > > > > > > > > is assigned also to the device - see drm_connector_acpi_find_companion().
> > > > > > > > > > > >
> > > > > > > > > > > > But please note that even if drm_connector does not have anything in
> > > > > > > > > > > > its fwnode member, the device may still be assigned fwnode, just based
> > > > > > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?).
> > > > > > > > > > > >
> > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> > > > > > > > > > > > > breaks drivers already using components (as it was pointed at [1]),
> > > > > > > > > > > > > resulting in a deadlock. Lockdep trace is provided below.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any
> > > > > > > > > > > > > sane way. Revert it instead.
> > > > > > > > > > > >
> > > > > > > > > > > > I think there is already user space stuff that relies on these links,
> > > > > > > > > > > > so I'm not sure you can just remove them like that. If the component
> > > > > > > > > > > > framework is not the correct tool here, then I think you need to
> > > > > > > > > > > > suggest some other way of creating them.
> > > > > > > > > > >
> > > > > > > > > > > The issue (that was pointed out during review) is that having a
> > > > > > > > > > > component code in the framework code can lead to lockups. With the
> > > > > > > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode
> > > > > > > > > > > for non-ACPI systems) probing of drivers which use components and set
> > > > > > > > > > > drm_connector::fwnode breaks immediately.
> > > > > > > > > > >
> > > > > > > > > > > Can we move the component part to the respective drivers? With the
> > > > > > > > > > > patch 2 in place, connector->fwnode will be copied to the created
> > > > > > > > > > > kdev's fwnode pointer.
> > > > > > > > > > >
> > > > > > > > > > > Another option might be to make this drm_sysfs component registration optional.
> > > > > > > > > >
> > > > > > > > > > You don't need to use the component framework at all if there is
> > > > > > > > > > a better way of determining the connection between the DP and its
> > > > > > > > > > Type-C connector (I'm assuming that that's what this series is about).
> > > > > > > > > > You just need the symlinks, not the component.
> > > > > > > > >
> > > > > > > > > The problem is that right now this component registration has become
> > > > > > > > > mandatory. And if I set the kdev->fwnode manually (like in the patch
> > > > > > > > > 2), the kernel hangs inside the component code.
> > > > > > > > > That's why I proposed to move the components to the place where they
> > > > > > > > > are really necessary, e.g. i915 and amd drivers.
> > > > > > > >
> > > > > > > > So why can't we replace the component with the method you are
> > > > > > > > proposing in this series of finding out the Type-C port also with
> > > > > > > > i915, AMD, or whatever driver and platform (that's the only thing that
> > > > > > > > component is used for)?
> > > > > > >
> > > > > > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP
> > > > > > > entry) and the drm_bridge_connector to create the connector. I think that
> > > > > > > enabling i915 and AMD drivers to use drm_bridge fells out of scope for this
> > > > > > > series.
> > > > > > >
> > > > > > >
> > > > > > > > Determining the connection between a DP and its Type-C connector is
> > > > > > > > starting to get really important, so ideally we have a common solution
> > > > > > > > for that.
> > > > > > >
> > > > > > > Yes. This is what we have been discussing with Simon for quite some time on
> > > > > > > #dri-devel.
> > > > > > >
> > > > > > > Unfortunately I think the solution that got merged was pretty much hastened
> > > > > > > in instead of being well-thought. For example, it is also not always
> > > > > > > possible to provide the drm_connector / typec_connector links (as you can
> > > > > > > see from the patch7. Sometimes we can only express that this is a Type-C DP
> > > > > > > connector, but we can not easily point it to the particular USB-C port.
> > > > > > >
> > > > > > > So, I'm not sure, how can we proceed here. Currently merged patch breaks
> > > > > > > drm/msm if we even try to use it by setting kdef->fwnode to
> > > > > > > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is
> > > > > > > an ACPI-only thing, which is not expected to work in a non-ACPI cases.
> > > > > >
> > > > > > You really have to always supply not only the Type-C ports and partners,
> > > > > > but also the alt modes. You need them, firstly to keep things sane
> > > > > > inside kernel, but more importantly, so they are always exposed to the
> > > > > > user space, AND, always the same way. We have ABIs for all this stuff,
> > > > > > including the DP alt mode. Use them. No shortcuts.
> > > > > >
> > > > > > So here's what you need to do. UCSI does not seem to bring you
> > > > > > anything useful, so just disable it for now. You don't need it. Your
> > > > > > port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so
> > > > > > that's where you need to register all these components - the ports,
> > > > > > partners and alt modes. You have all the needed information there.
> > > > >
> > > > > To make things even more complicate, UCSI is necessary for the USB part of
> > > > > the story. It handles vbus and direction.
> > > > >
> > > > > > Only after you've done that we can start to look at how should the
> > > > > > connection between the DPs and their USB Type-C connectors be handled.
> > > > >
> > > > > But sure enough, I can add typec port registration to the altmode driver.
> > > > > This will solve the 'port not existing' part of the story.
> > > > >
> > > > > I'd like to hear your opinion on:
> > > > >
> > > > > - components. Using them breaks drm/msm. How can we proceed?
> > > >
> > > > I don't think replacing the components is going to be a problem once
> > > > you have described everything properly in you DT. I'm fairly certain now
> > > > that that is the main problem here. You don't have this connection
> > > > described in your DT as it should.
> > >
> > > We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/
> > > (for non-PMIC-GLINK platform)
> > > Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full
> > > description of USB-C connector and signal flow.
> > >
> > > In fact, thanks to this representation I can properly set
> > > 'connector->fwnode' to point to the OF node corresponding to the
> > > connector's drm_bridge. I can even propagate it to the kdef->fwnode /
> > > kdev->of_node in drm_sysfs_connector_add(). But then a component_add()
> > > call looks the kernel up.
> > >
> > > And to add on top of that, here is another reason why I think that
> > > this sysfs links ABI/implementation was not well thought. The
> > > typec_connector_ops are added to all fwnode-enabled connector devices.
> > > It doesn't even bother checking that the device is really the DP
> > > connector and that the device on the other side of fwnode link is a
> > > typec port device. The symlink is named 'typec_connector', so one can
> > > not easily extend this ABI to support SlimPort aka MyDP (which uses
> > > micro-USB-B connectors instead of USB-C). Neither can we extend it to
> > > represent MHL connections (again, micro-USB-B).
> > >
> > > > > - PATH property usage. This way we make USB-C DisplayPort behave like the
> > > > > MST ports.
> > > >
> > > > That looks to me like an attempt to exploit a feature that is not
> > > > designed for this purposes at all. Just drop all that.
> > >
> > > But why? From the docs: 'Connector path property to identify how this
> > > sink is physically connected.'
> > >
> > > So far we have been using it for MST only. But the description above
> > > also suits properly for the 'connected to the Type-C port0 device'
> > > kind of data. Then the userspace can use this property to change the
> > > representation of the controller. Or to rename it as it does for
> > > DP-MST connectors. Or just add the USB-C icon in the UI.
> > >
> > > Having this data in sysfs only requires userspace first to map the
> > > connector to the device under sysfs (which is not trivial since Xorg
> > > renames DP-MST connectors), then to look for the symlink value. Quite
> > > complicated compared to checking the DRM property.
> > >
> > > Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the
> > > schema to support 'microusb:something' values for this property.
> > >
> > > > The connection has to be first described in your DT, and the way you
> > > > usually describe connections in DT is by using the device graph (OF
> > > > graph). It seems that you have everything needed for that - the USB
> > > > Type-C connectors have their own OF nodes (what you register as
> > > > drm_bridges are in fact USB Type-C connectors), and presumable you
> > > > also have OF nodes for all your video ports (DisplayPorts) - so
> > > > applying the graph between the two really should not be a problem. The
> > > > DP is endpoint for the USB Type-C connector, and vice versa.
> > >
> > > Not quite. There is no direct connection between the USB Type-C
> > > connector and DP controller. The USB-C connector has three ports.
> > >
> > > port@0 goes to theHS-USB controller. This is simple.
> > >
> > > port@1 goes to the USB+DP PHY. All retimers and SS line muxes are
> > > included in between. And it is the USB+DP PHY that is connected to the
> > > DP and USB-SS controllers.
> > >
> > > port@2 goes to SBU lines mux (e.g. fsa4480).
> > >
> > > > After you have everything needed in your DT, the problem here isn't
> > > > actually much of a problem at all. We will have options how to move
> > > > forward after that.
> > >
> > > Could you please describe what is missing there?
> >
> > We are not after the direct connections here, we are after the final
> > endpoints. So you are missing description of the logical connection
> > between your DP and Type-C connector.
> 
> Adding Krzysztof, as one of DT maintainers, to the CC list.
> 
> >From what I understand, DT describes hardware. There is no description
> for the 'logical' connections.
> 
> >
> > I understand that the idea is to build the graph to describe only the
> > physical connections, but with just the physical connections you are
> > doomed to write separate software solution for almost every single
> > platform, even though the final endpoints are always the same (DP to
> > Type-C). You just can not generalise the components (muxes, phys,
> > retimers, etc.) behind USB Type-C connectors (or anything else for
> > that matter), it's not possible. The components and their order vary
> > on almost every single platform. On some platforms the stack of parts
> > after the connector is also incredibly complex.
> 
> Yes. And this is why we have a chain of DRM bridges, going from the DP
> controller to the final drm_bridge at the Type-C port manager. When
> there is the altmode event, it gets sent via this chain using the
> normal DRM HPD event.

We will not have drm bridges between the thunderbolt controller and
the connector, but you still need to be able to show the connector to
the user when DisplayPort is tunneled over thunderbolt. DP alt mode is
only one way of getting DisplayPort through USB Type-C. You just can't
make any assumptions with USB Type-C.

The drm bridge chain could only solve the port/connector relationship
problem from a single angle, but we need a common solution. The
problem is after all completely generic. It is not DisplayPort
specific or even USB Type-C specific problem. Those are just two of
the many possible last endpoints for these connections that need to be
aware of each other.

So we really have to have a common way of getting this straight from
the hardware description somehow.

To me the obvious solution would be to just have a port in the graph
that points directly the last endpoint regardless of what you have in
between. But if that's not an option, then so be it. Then there just
needs to be some other way of getting that information from DT.

Maybe DT could use similar physical location object/attribute like
ACPI - the DP would have matching physical location with its connector?

> > Having the logical final endpoint connection described in your DT/ACPI
> > on top of the physical connections costs very little, but at the same
> > time it's usually the only thing that the software needs (like in this
> > case).
> 
> Maybe there is some misunderstanding here. We have this connection. We
> have connector->fwnode and connector->of_node pointing to the correct
> device - the last bridge in the chain. Each TCPM driver knows the
> relationship between the in-built drm_bridge and the Type-C port. The
> DP host controller port can be terminated with other endpoints, e.g.
> eDP panel. Or there can be a non-DP host, which is then connected
> through a series of bridges to the eDP or external DP port. This is
> what anx78xx bridge does: it converts the HDMI link into an external
> DP (SlimPort) connection. Bridge chains permit this to be handled in a
> seamless way.
> 
> What you are asking for looks like a step backwards to me: it requires
> the host to know that there is a USB-C connector.
> 
> > So, either you add one more port to your graph for the DP to Type-C
> > connection, or, if that's not an option, then you need to describe
> > that connection in some other way. Named references work also quite
> > well in my experience.
> 
> Named references were considered and frowned upon by DT maintainers.

thanks,

-- 
heikki

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Neil Armstrong]

On 14/09/2023 11:26, Heikki Krogerus wrote:
> Hi Dmitry,
> 
> On Wed, Sep 13, 2023 at 04:47:12PM +0300, Dmitry Baryshkov wrote:
>> On Wed, 13 Sept 2023 at 16:15, Heikki Krogerus
>> <heikki.krogerus@linux.intel.com> wrote:
>>>
>>> On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote:
>>>> Hi Heikki,
>>>>
>>>> On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus
>>>> <heikki.krogerus@linux.intel.com> wrote:
>>>>> On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote:
>>>>>> On 12/09/2023 14:05, Heikki Krogerus wrote:
>>>>>>> On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote:
>>>>>>>> On 06/09/2023 16:38, Heikki Krogerus wrote:
>>>>>>>>> On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
>>>>>>>>>> On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
>>>>>>>>>> <heikki.krogerus@linux.intel.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
>>>>>>>>>>>> Hi Heikki,
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
>>>>>>>>>>>> <heikki.krogerus@linux.intel.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Dmitry,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
>>>>>>>>>>>>>> The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
>>>>>>>>>>>>>> dev_fwnode() checks never succeed, making the respective commit NOP.
>>>>>>>>>>>>>
>>>>>>>>>>>>> That's not true. The dev->fwnode is assigned when the device is
>>>>>>>>>>>>> created on ACPI platforms automatically. If the drm_connector fwnode
>>>>>>>>>>>>> member is assigned before the device is registered, then that fwnode
>>>>>>>>>>>>> is assigned also to the device - see drm_connector_acpi_find_companion().
>>>>>>>>>>>>>
>>>>>>>>>>>>> But please note that even if drm_connector does not have anything in
>>>>>>>>>>>>> its fwnode member, the device may still be assigned fwnode, just based
>>>>>>>>>>>>> on some other logic (maybe in drivers/acpi/acpi_video.c?).
>>>>>>>>>>>>>
>>>>>>>>>>>>>> And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
>>>>>>>>>>>>>> breaks drivers already using components (as it was pointed at [1]),
>>>>>>>>>>>>>> resulting in a deadlock. Lockdep trace is provided below.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Granted these two issues, it seems impractical to fix this commit in any
>>>>>>>>>>>>>> sane way. Revert it instead.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I think there is already user space stuff that relies on these links,
>>>>>>>>>>>>> so I'm not sure you can just remove them like that. If the component
>>>>>>>>>>>>> framework is not the correct tool here, then I think you need to
>>>>>>>>>>>>> suggest some other way of creating them.
>>>>>>>>>>>>
>>>>>>>>>>>> The issue (that was pointed out during review) is that having a
>>>>>>>>>>>> component code in the framework code can lead to lockups. With the
>>>>>>>>>>>> patch #2 in place (which is the only logical way to set kdev->fwnode
>>>>>>>>>>>> for non-ACPI systems) probing of drivers which use components and set
>>>>>>>>>>>> drm_connector::fwnode breaks immediately.
>>>>>>>>>>>>
>>>>>>>>>>>> Can we move the component part to the respective drivers? With the
>>>>>>>>>>>> patch 2 in place, connector->fwnode will be copied to the created
>>>>>>>>>>>> kdev's fwnode pointer.
>>>>>>>>>>>>
>>>>>>>>>>>> Another option might be to make this drm_sysfs component registration optional.
>>>>>>>>>>>
>>>>>>>>>>> You don't need to use the component framework at all if there is
>>>>>>>>>>> a better way of determining the connection between the DP and its
>>>>>>>>>>> Type-C connector (I'm assuming that that's what this series is about).
>>>>>>>>>>> You just need the symlinks, not the component.
>>>>>>>>>>
>>>>>>>>>> The problem is that right now this component registration has become
>>>>>>>>>> mandatory. And if I set the kdev->fwnode manually (like in the patch
>>>>>>>>>> 2), the kernel hangs inside the component code.
>>>>>>>>>> That's why I proposed to move the components to the place where they
>>>>>>>>>> are really necessary, e.g. i915 and amd drivers.
>>>>>>>>>
>>>>>>>>> So why can't we replace the component with the method you are
>>>>>>>>> proposing in this series of finding out the Type-C port also with
>>>>>>>>> i915, AMD, or whatever driver and platform (that's the only thing that
>>>>>>>>> component is used for)?
>>>>>>>>
>>>>>>>> The drm/msm driver uses drm_bridge for the pipeline (including the last DP
>>>>>>>> entry) and the drm_bridge_connector to create the connector. I think that
>>>>>>>> enabling i915 and AMD drivers to use drm_bridge fells out of scope for this
>>>>>>>> series.
>>>>>>>>
>>>>>>>>
>>>>>>>>> Determining the connection between a DP and its Type-C connector is
>>>>>>>>> starting to get really important, so ideally we have a common solution
>>>>>>>>> for that.
>>>>>>>>
>>>>>>>> Yes. This is what we have been discussing with Simon for quite some time on
>>>>>>>> #dri-devel.
>>>>>>>>
>>>>>>>> Unfortunately I think the solution that got merged was pretty much hastened
>>>>>>>> in instead of being well-thought. For example, it is also not always
>>>>>>>> possible to provide the drm_connector / typec_connector links (as you can
>>>>>>>> see from the patch7. Sometimes we can only express that this is a Type-C DP
>>>>>>>> connector, but we can not easily point it to the particular USB-C port.
>>>>>>>>
>>>>>>>> So, I'm not sure, how can we proceed here. Currently merged patch breaks
>>>>>>>> drm/msm if we even try to use it by setting kdef->fwnode to
>>>>>>>> drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is
>>>>>>>> an ACPI-only thing, which is not expected to work in a non-ACPI cases.
>>>>>>>
>>>>>>> You really have to always supply not only the Type-C ports and partners,
>>>>>>> but also the alt modes. You need them, firstly to keep things sane
>>>>>>> inside kernel, but more importantly, so they are always exposed to the
>>>>>>> user space, AND, always the same way. We have ABIs for all this stuff,
>>>>>>> including the DP alt mode. Use them. No shortcuts.
>>>>>>>
>>>>>>> So here's what you need to do. UCSI does not seem to bring you
>>>>>>> anything useful, so just disable it for now. You don't need it. Your
>>>>>>> port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so
>>>>>>> that's where you need to register all these components - the ports,
>>>>>>> partners and alt modes. You have all the needed information there.
>>>>>>
>>>>>> To make things even more complicate, UCSI is necessary for the USB part of
>>>>>> the story. It handles vbus and direction.
>>>>>>
>>>>>>> Only after you've done that we can start to look at how should the
>>>>>>> connection between the DPs and their USB Type-C connectors be handled.
>>>>>>
>>>>>> But sure enough, I can add typec port registration to the altmode driver.
>>>>>> This will solve the 'port not existing' part of the story.
>>>>>>
>>>>>> I'd like to hear your opinion on:
>>>>>>
>>>>>> - components. Using them breaks drm/msm. How can we proceed?
>>>>>
>>>>> I don't think replacing the components is going to be a problem once
>>>>> you have described everything properly in you DT. I'm fairly certain now
>>>>> that that is the main problem here. You don't have this connection
>>>>> described in your DT as it should.
>>>>
>>>> We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/
>>>> (for non-PMIC-GLINK platform)
>>>> Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full
>>>> description of USB-C connector and signal flow.
>>>>
>>>> In fact, thanks to this representation I can properly set
>>>> 'connector->fwnode' to point to the OF node corresponding to the
>>>> connector's drm_bridge. I can even propagate it to the kdef->fwnode /
>>>> kdev->of_node in drm_sysfs_connector_add(). But then a component_add()
>>>> call looks the kernel up.
>>>>
>>>> And to add on top of that, here is another reason why I think that
>>>> this sysfs links ABI/implementation was not well thought. The
>>>> typec_connector_ops are added to all fwnode-enabled connector devices.
>>>> It doesn't even bother checking that the device is really the DP
>>>> connector and that the device on the other side of fwnode link is a
>>>> typec port device. The symlink is named 'typec_connector', so one can
>>>> not easily extend this ABI to support SlimPort aka MyDP (which uses
>>>> micro-USB-B connectors instead of USB-C). Neither can we extend it to
>>>> represent MHL connections (again, micro-USB-B).
>>>>
>>>>>> - PATH property usage. This way we make USB-C DisplayPort behave like the
>>>>>> MST ports.
>>>>>
>>>>> That looks to me like an attempt to exploit a feature that is not
>>>>> designed for this purposes at all. Just drop all that.
>>>>
>>>> But why? From the docs: 'Connector path property to identify how this
>>>> sink is physically connected.'
>>>>
>>>> So far we have been using it for MST only. But the description above
>>>> also suits properly for the 'connected to the Type-C port0 device'
>>>> kind of data. Then the userspace can use this property to change the
>>>> representation of the controller. Or to rename it as it does for
>>>> DP-MST connectors. Or just add the USB-C icon in the UI.
>>>>
>>>> Having this data in sysfs only requires userspace first to map the
>>>> connector to the device under sysfs (which is not trivial since Xorg
>>>> renames DP-MST connectors), then to look for the symlink value. Quite
>>>> complicated compared to checking the DRM property.
>>>>
>>>> Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the
>>>> schema to support 'microusb:something' values for this property.
>>>>
>>>>> The connection has to be first described in your DT, and the way you
>>>>> usually describe connections in DT is by using the device graph (OF
>>>>> graph). It seems that you have everything needed for that - the USB
>>>>> Type-C connectors have their own OF nodes (what you register as
>>>>> drm_bridges are in fact USB Type-C connectors), and presumable you
>>>>> also have OF nodes for all your video ports (DisplayPorts) - so
>>>>> applying the graph between the two really should not be a problem. The
>>>>> DP is endpoint for the USB Type-C connector, and vice versa.
>>>>
>>>> Not quite. There is no direct connection between the USB Type-C
>>>> connector and DP controller. The USB-C connector has three ports.
>>>>
>>>> port@0 goes to theHS-USB controller. This is simple.
>>>>
>>>> port@1 goes to the USB+DP PHY. All retimers and SS line muxes are
>>>> included in between. And it is the USB+DP PHY that is connected to the
>>>> DP and USB-SS controllers.
>>>>
>>>> port@2 goes to SBU lines mux (e.g. fsa4480).
>>>>
>>>>> After you have everything needed in your DT, the problem here isn't
>>>>> actually much of a problem at all. We will have options how to move
>>>>> forward after that.
>>>>
>>>> Could you please describe what is missing there?
>>>
>>> We are not after the direct connections here, we are after the final
>>> endpoints. So you are missing description of the logical connection
>>> between your DP and Type-C connector.
>>
>> Adding Krzysztof, as one of DT maintainers, to the CC list.
>>
>> >From what I understand, DT describes hardware. There is no description
>> for the 'logical' connections.
>>
>>>
>>> I understand that the idea is to build the graph to describe only the
>>> physical connections, but with just the physical connections you are
>>> doomed to write separate software solution for almost every single
>>> platform, even though the final endpoints are always the same (DP to
>>> Type-C). You just can not generalise the components (muxes, phys,
>>> retimers, etc.) behind USB Type-C connectors (or anything else for
>>> that matter), it's not possible. The components and their order vary
>>> on almost every single platform. On some platforms the stack of parts
>>> after the connector is also incredibly complex.
>>
>> Yes. And this is why we have a chain of DRM bridges, going from the DP
>> controller to the final drm_bridge at the Type-C port manager. When
>> there is the altmode event, it gets sent via this chain using the
>> normal DRM HPD event.
> 
> We will not have drm bridges between the thunderbolt controller and
> the connector, but you still need to be able to show the connector to
> the user when DisplayPort is tunneled over thunderbolt. DP alt mode is
> only one way of getting DisplayPort through USB Type-C. You just can't
> make any assumptions with USB Type-C.
> 
> The drm bridge chain could only solve the port/connector relationship
> problem from a single angle, but we need a common solution. The
> problem is after all completely generic. It is not DisplayPort
> specific or even USB Type-C specific problem. Those are just two of
> the many possible last endpoints for these connections that need to be
> aware of each other.
> 
> So we really have to have a common way of getting this straight from
> the hardware description somehow.
> 
> To me the obvious solution would be to just have a port in the graph
> that points directly the last endpoint regardless of what you have in
> between. But if that's not an option, then so be it. Then there just
> needs to be some other way of getting that information from DT.

The DT must describe the HW interconnection, this is what we do, but
we're allowed to parse it as we want and ignore the bridges/endpoint
between the connector node and the display node, all this is implementation.

We added intermediate bridge because they are part the displayport signal
chain, and they may need to react to the display enable/disable/check
if there's some limitations or init sequence in the retimer for example.

Neil

> 
> Maybe DT could use similar physical location object/attribute like
> ACPI - the DP would have matching physical location with its connector?
> 
>>> Having the logical final endpoint connection described in your DT/ACPI
>>> on top of the physical connections costs very little, but at the same
>>> time it's usually the only thing that the software needs (like in this
>>> case).
>>
>> Maybe there is some misunderstanding here. We have this connection. We
>> have connector->fwnode and connector->of_node pointing to the correct
>> device - the last bridge in the chain. Each TCPM driver knows the
>> relationship between the in-built drm_bridge and the Type-C port. The
>> DP host controller port can be terminated with other endpoints, e.g.
>> eDP panel. Or there can be a non-DP host, which is then connected
>> through a series of bridges to the eDP or external DP port. This is
>> what anx78xx bridge does: it converts the HDMI link into an external
>> DP (SlimPort) connection. Bridge chains permit this to be handled in a
>> seamless way.
>>
>> What you are asking for looks like a step backwards to me: it requires
>> the host to know that there is a USB-C connector.
>>
>>> So, either you add one more port to your graph for the DP to Type-C
>>> connection, or, if that's not an option, then you need to describe
>>> that connection in some other way. Named references work also quite
>>> well in my experience.
>>
>> Named references were considered and frowned upon by DT maintainers.
> 
> thanks,
> 

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Dmitry Baryshkov]

On Thu, 14 Sept 2023 at 12:35, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> On 14/09/2023 11:26, Heikki Krogerus wrote:
> > Hi Dmitry,
> >
> > On Wed, Sep 13, 2023 at 04:47:12PM +0300, Dmitry Baryshkov wrote:
> >> On Wed, 13 Sept 2023 at 16:15, Heikki Krogerus
> >> <heikki.krogerus@linux.intel.com> wrote:
> >>>
> >>> On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote:
> >>>> Hi Heikki,
> >>>>
> >>>> On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus
> >>>> <heikki.krogerus@linux.intel.com> wrote:
> >>>>> On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote:
> >>>>>> On 12/09/2023 14:05, Heikki Krogerus wrote:
> >>>>>>> On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote:
> >>>>>>>> On 06/09/2023 16:38, Heikki Krogerus wrote:
> >>>>>>>>> On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
> >>>>>>>>>> On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
> >>>>>>>>>> <heikki.krogerus@linux.intel.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
> >>>>>>>>>>>> Hi Heikki,
> >>>>>>>>>>>>
> >>>>>>>>>>>> On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
> >>>>>>>>>>>> <heikki.krogerus@linux.intel.com> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Hi Dmitry,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> >>>>>>>>>>>>>> The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> >>>>>>>>>>>>>> dev_fwnode() checks never succeed, making the respective commit NOP.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> That's not true. The dev->fwnode is assigned when the device is
> >>>>>>>>>>>>> created on ACPI platforms automatically. If the drm_connector fwnode
> >>>>>>>>>>>>> member is assigned before the device is registered, then that fwnode
> >>>>>>>>>>>>> is assigned also to the device - see drm_connector_acpi_find_companion().
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> But please note that even if drm_connector does not have anything in
> >>>>>>>>>>>>> its fwnode member, the device may still be assigned fwnode, just based
> >>>>>>>>>>>>> on some other logic (maybe in drivers/acpi/acpi_video.c?).
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> >>>>>>>>>>>>>> breaks drivers already using components (as it was pointed at [1]),
> >>>>>>>>>>>>>> resulting in a deadlock. Lockdep trace is provided below.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Granted these two issues, it seems impractical to fix this commit in any
> >>>>>>>>>>>>>> sane way. Revert it instead.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I think there is already user space stuff that relies on these links,
> >>>>>>>>>>>>> so I'm not sure you can just remove them like that. If the component
> >>>>>>>>>>>>> framework is not the correct tool here, then I think you need to
> >>>>>>>>>>>>> suggest some other way of creating them.
> >>>>>>>>>>>>
> >>>>>>>>>>>> The issue (that was pointed out during review) is that having a
> >>>>>>>>>>>> component code in the framework code can lead to lockups. With the
> >>>>>>>>>>>> patch #2 in place (which is the only logical way to set kdev->fwnode
> >>>>>>>>>>>> for non-ACPI systems) probing of drivers which use components and set
> >>>>>>>>>>>> drm_connector::fwnode breaks immediately.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Can we move the component part to the respective drivers? With the
> >>>>>>>>>>>> patch 2 in place, connector->fwnode will be copied to the created
> >>>>>>>>>>>> kdev's fwnode pointer.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Another option might be to make this drm_sysfs component registration optional.
> >>>>>>>>>>>
> >>>>>>>>>>> You don't need to use the component framework at all if there is
> >>>>>>>>>>> a better way of determining the connection between the DP and its
> >>>>>>>>>>> Type-C connector (I'm assuming that that's what this series is about).
> >>>>>>>>>>> You just need the symlinks, not the component.
> >>>>>>>>>>
> >>>>>>>>>> The problem is that right now this component registration has become
> >>>>>>>>>> mandatory. And if I set the kdev->fwnode manually (like in the patch
> >>>>>>>>>> 2), the kernel hangs inside the component code.
> >>>>>>>>>> That's why I proposed to move the components to the place where they
> >>>>>>>>>> are really necessary, e.g. i915 and amd drivers.
> >>>>>>>>>
> >>>>>>>>> So why can't we replace the component with the method you are
> >>>>>>>>> proposing in this series of finding out the Type-C port also with
> >>>>>>>>> i915, AMD, or whatever driver and platform (that's the only thing that
> >>>>>>>>> component is used for)?
> >>>>>>>>
> >>>>>>>> The drm/msm driver uses drm_bridge for the pipeline (including the last DP
> >>>>>>>> entry) and the drm_bridge_connector to create the connector. I think that
> >>>>>>>> enabling i915 and AMD drivers to use drm_bridge fells out of scope for this
> >>>>>>>> series.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> Determining the connection between a DP and its Type-C connector is
> >>>>>>>>> starting to get really important, so ideally we have a common solution
> >>>>>>>>> for that.
> >>>>>>>>
> >>>>>>>> Yes. This is what we have been discussing with Simon for quite some time on
> >>>>>>>> #dri-devel.
> >>>>>>>>
> >>>>>>>> Unfortunately I think the solution that got merged was pretty much hastened
> >>>>>>>> in instead of being well-thought. For example, it is also not always
> >>>>>>>> possible to provide the drm_connector / typec_connector links (as you can
> >>>>>>>> see from the patch7. Sometimes we can only express that this is a Type-C DP
> >>>>>>>> connector, but we can not easily point it to the particular USB-C port.
> >>>>>>>>
> >>>>>>>> So, I'm not sure, how can we proceed here. Currently merged patch breaks
> >>>>>>>> drm/msm if we even try to use it by setting kdef->fwnode to
> >>>>>>>> drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is
> >>>>>>>> an ACPI-only thing, which is not expected to work in a non-ACPI cases.
> >>>>>>>
> >>>>>>> You really have to always supply not only the Type-C ports and partners,
> >>>>>>> but also the alt modes. You need them, firstly to keep things sane
> >>>>>>> inside kernel, but more importantly, so they are always exposed to the
> >>>>>>> user space, AND, always the same way. We have ABIs for all this stuff,
> >>>>>>> including the DP alt mode. Use them. No shortcuts.
> >>>>>>>
> >>>>>>> So here's what you need to do. UCSI does not seem to bring you
> >>>>>>> anything useful, so just disable it for now. You don't need it. Your
> >>>>>>> port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so
> >>>>>>> that's where you need to register all these components - the ports,
> >>>>>>> partners and alt modes. You have all the needed information there.
> >>>>>>
> >>>>>> To make things even more complicate, UCSI is necessary for the USB part of
> >>>>>> the story. It handles vbus and direction.
> >>>>>>
> >>>>>>> Only after you've done that we can start to look at how should the
> >>>>>>> connection between the DPs and their USB Type-C connectors be handled.
> >>>>>>
> >>>>>> But sure enough, I can add typec port registration to the altmode driver.
> >>>>>> This will solve the 'port not existing' part of the story.
> >>>>>>
> >>>>>> I'd like to hear your opinion on:
> >>>>>>
> >>>>>> - components. Using them breaks drm/msm. How can we proceed?
> >>>>>
> >>>>> I don't think replacing the components is going to be a problem once
> >>>>> you have described everything properly in you DT. I'm fairly certain now
> >>>>> that that is the main problem here. You don't have this connection
> >>>>> described in your DT as it should.
> >>>>
> >>>> We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/
> >>>> (for non-PMIC-GLINK platform)
> >>>> Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full
> >>>> description of USB-C connector and signal flow.
> >>>>
> >>>> In fact, thanks to this representation I can properly set
> >>>> 'connector->fwnode' to point to the OF node corresponding to the
> >>>> connector's drm_bridge. I can even propagate it to the kdef->fwnode /
> >>>> kdev->of_node in drm_sysfs_connector_add(). But then a component_add()
> >>>> call looks the kernel up.
> >>>>
> >>>> And to add on top of that, here is another reason why I think that
> >>>> this sysfs links ABI/implementation was not well thought. The
> >>>> typec_connector_ops are added to all fwnode-enabled connector devices.
> >>>> It doesn't even bother checking that the device is really the DP
> >>>> connector and that the device on the other side of fwnode link is a
> >>>> typec port device. The symlink is named 'typec_connector', so one can
> >>>> not easily extend this ABI to support SlimPort aka MyDP (which uses
> >>>> micro-USB-B connectors instead of USB-C). Neither can we extend it to
> >>>> represent MHL connections (again, micro-USB-B).
> >>>>
> >>>>>> - PATH property usage. This way we make USB-C DisplayPort behave like the
> >>>>>> MST ports.
> >>>>>
> >>>>> That looks to me like an attempt to exploit a feature that is not
> >>>>> designed for this purposes at all. Just drop all that.
> >>>>
> >>>> But why? From the docs: 'Connector path property to identify how this
> >>>> sink is physically connected.'
> >>>>
> >>>> So far we have been using it for MST only. But the description above
> >>>> also suits properly for the 'connected to the Type-C port0 device'
> >>>> kind of data. Then the userspace can use this property to change the
> >>>> representation of the controller. Or to rename it as it does for
> >>>> DP-MST connectors. Or just add the USB-C icon in the UI.
> >>>>
> >>>> Having this data in sysfs only requires userspace first to map the
> >>>> connector to the device under sysfs (which is not trivial since Xorg
> >>>> renames DP-MST connectors), then to look for the symlink value. Quite
> >>>> complicated compared to checking the DRM property.
> >>>>
> >>>> Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the
> >>>> schema to support 'microusb:something' values for this property.
> >>>>
> >>>>> The connection has to be first described in your DT, and the way you
> >>>>> usually describe connections in DT is by using the device graph (OF
> >>>>> graph). It seems that you have everything needed for that - the USB
> >>>>> Type-C connectors have their own OF nodes (what you register as
> >>>>> drm_bridges are in fact USB Type-C connectors), and presumable you
> >>>>> also have OF nodes for all your video ports (DisplayPorts) - so
> >>>>> applying the graph between the two really should not be a problem. The
> >>>>> DP is endpoint for the USB Type-C connector, and vice versa.
> >>>>
> >>>> Not quite. There is no direct connection between the USB Type-C
> >>>> connector and DP controller. The USB-C connector has three ports.
> >>>>
> >>>> port@0 goes to theHS-USB controller. This is simple.
> >>>>
> >>>> port@1 goes to the USB+DP PHY. All retimers and SS line muxes are
> >>>> included in between. And it is the USB+DP PHY that is connected to the
> >>>> DP and USB-SS controllers.
> >>>>
> >>>> port@2 goes to SBU lines mux (e.g. fsa4480).
> >>>>
> >>>>> After you have everything needed in your DT, the problem here isn't
> >>>>> actually much of a problem at all. We will have options how to move
> >>>>> forward after that.
> >>>>
> >>>> Could you please describe what is missing there?
> >>>
> >>> We are not after the direct connections here, we are after the final
> >>> endpoints. So you are missing description of the logical connection
> >>> between your DP and Type-C connector.
> >>
> >> Adding Krzysztof, as one of DT maintainers, to the CC list.
> >>
> >> >From what I understand, DT describes hardware. There is no description
> >> for the 'logical' connections.
> >>
> >>>
> >>> I understand that the idea is to build the graph to describe only the
> >>> physical connections, but with just the physical connections you are
> >>> doomed to write separate software solution for almost every single
> >>> platform, even though the final endpoints are always the same (DP to
> >>> Type-C). You just can not generalise the components (muxes, phys,
> >>> retimers, etc.) behind USB Type-C connectors (or anything else for
> >>> that matter), it's not possible. The components and their order vary
> >>> on almost every single platform. On some platforms the stack of parts
> >>> after the connector is also incredibly complex.
> >>
> >> Yes. And this is why we have a chain of DRM bridges, going from the DP
> >> controller to the final drm_bridge at the Type-C port manager. When
> >> there is the altmode event, it gets sent via this chain using the
> >> normal DRM HPD event.
> >
> > We will not have drm bridges between the thunderbolt controller and
> > the connector, but you still need to be able to show the connector to
> > the user when DisplayPort is tunneled over thunderbolt. DP alt mode is
> > only one way of getting DisplayPort through USB Type-C. You just can't
> > make any assumptions with USB Type-C.
> >
> > The drm bridge chain could only solve the port/connector relationship
> > problem from a single angle, but we need a common solution. The
> > problem is after all completely generic. It is not DisplayPort
> > specific or even USB Type-C specific problem. Those are just two of
> > the many possible last endpoints for these connections that need to be
> > aware of each other.
> >
> > So we really have to have a common way of getting this straight from
> > the hardware description somehow.
> >
> > To me the obvious solution would be to just have a port in the graph
> > that points directly the last endpoint regardless of what you have in
> > between. But if that's not an option, then so be it. Then there just
> > needs to be some other way of getting that information from DT.
>
> The DT must describe the HW interconnection, this is what we do, but
> we're allowed to parse it as we want and ignore the bridges/endpoint
> between the connector node and the display node, all this is implementation.
>
> We added intermediate bridge because they are part the displayport signal
> chain, and they may need to react to the display enable/disable/check
> if there's some limitations or init sequence in the retimer for example.

Moreover, this way we do not have to care about exact binding of the
interim devices. They can have different connections and order ports
as they do see fit. Parsing the whole chain from DP controller would
require having standard bindings for all retimers / muxes / switches
in the chain.

>
> Neil
>
> >
> > Maybe DT could use similar physical location object/attribute like
> > ACPI - the DP would have matching physical location with its connector?
> >
> >>> Having the logical final endpoint connection described in your DT/ACPI
> >>> on top of the physical connections costs very little, but at the same
> >>> time it's usually the only thing that the software needs (like in this
> >>> case).
> >>
> >> Maybe there is some misunderstanding here. We have this connection. We
> >> have connector->fwnode and connector->of_node pointing to the correct
> >> device - the last bridge in the chain. Each TCPM driver knows the
> >> relationship between the in-built drm_bridge and the Type-C port. The
> >> DP host controller port can be terminated with other endpoints, e.g.
> >> eDP panel. Or there can be a non-DP host, which is then connected
> >> through a series of bridges to the eDP or external DP port. This is
> >> what anx78xx bridge does: it converts the HDMI link into an external
> >> DP (SlimPort) connection. Bridge chains permit this to be handled in a
> >> seamless way.
> >>
> >> What you are asking for looks like a step backwards to me: it requires
> >> the host to know that there is a USB-C connector.
> >>
> >>> So, either you add one more port to your graph for the DP to Type-C
> >>> connection, or, if that's not an option, then you need to describe
> >>> that connection in some other way. Named references work also quite
> >>> well in my experience.
> >>
> >> Named references were considered and frowned upon by DT maintainers.
> >
> > thanks,
> >
>


-- 
With best wishes
Dmitry

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Dmitry Baryshkov]

On Thu, 14 Sept 2023 at 12:26, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Dmitry,
>
> On Wed, Sep 13, 2023 at 04:47:12PM +0300, Dmitry Baryshkov wrote:
> > On Wed, 13 Sept 2023 at 16:15, Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote:
> > > > Hi Heikki,
> > > >
> > > > On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus
> > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote:
> > > > > > On 12/09/2023 14:05, Heikki Krogerus wrote:
> > > > > > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote:
> > > > > > > > On 06/09/2023 16:38, Heikki Krogerus wrote:
> > > > > > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
> > > > > > > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > > > > Hi Heikki,
> > > > > > > > > > > >
> > > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
> > > > > > > > > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hi Dmitry,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> > > > > > > > > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP.
> > > > > > > > > > > > >
> > > > > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is
> > > > > > > > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode
> > > > > > > > > > > > > member is assigned before the device is registered, then that fwnode
> > > > > > > > > > > > > is assigned also to the device - see drm_connector_acpi_find_companion().
> > > > > > > > > > > > >
> > > > > > > > > > > > > But please note that even if drm_connector does not have anything in
> > > > > > > > > > > > > its fwnode member, the device may still be assigned fwnode, just based
> > > > > > > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?).
> > > > > > > > > > > > >
> > > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> > > > > > > > > > > > > > breaks drivers already using components (as it was pointed at [1]),
> > > > > > > > > > > > > > resulting in a deadlock. Lockdep trace is provided below.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any
> > > > > > > > > > > > > > sane way. Revert it instead.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I think there is already user space stuff that relies on these links,
> > > > > > > > > > > > > so I'm not sure you can just remove them like that. If the component
> > > > > > > > > > > > > framework is not the correct tool here, then I think you need to
> > > > > > > > > > > > > suggest some other way of creating them.
> > > > > > > > > > > >
> > > > > > > > > > > > The issue (that was pointed out during review) is that having a
> > > > > > > > > > > > component code in the framework code can lead to lockups. With the
> > > > > > > > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode
> > > > > > > > > > > > for non-ACPI systems) probing of drivers which use components and set
> > > > > > > > > > > > drm_connector::fwnode breaks immediately.
> > > > > > > > > > > >
> > > > > > > > > > > > Can we move the component part to the respective drivers? With the
> > > > > > > > > > > > patch 2 in place, connector->fwnode will be copied to the created
> > > > > > > > > > > > kdev's fwnode pointer.
> > > > > > > > > > > >
> > > > > > > > > > > > Another option might be to make this drm_sysfs component registration optional.
> > > > > > > > > > >
> > > > > > > > > > > You don't need to use the component framework at all if there is
> > > > > > > > > > > a better way of determining the connection between the DP and its
> > > > > > > > > > > Type-C connector (I'm assuming that that's what this series is about).
> > > > > > > > > > > You just need the symlinks, not the component.
> > > > > > > > > >
> > > > > > > > > > The problem is that right now this component registration has become
> > > > > > > > > > mandatory. And if I set the kdev->fwnode manually (like in the patch
> > > > > > > > > > 2), the kernel hangs inside the component code.
> > > > > > > > > > That's why I proposed to move the components to the place where they
> > > > > > > > > > are really necessary, e.g. i915 and amd drivers.
> > > > > > > > >
> > > > > > > > > So why can't we replace the component with the method you are
> > > > > > > > > proposing in this series of finding out the Type-C port also with
> > > > > > > > > i915, AMD, or whatever driver and platform (that's the only thing that
> > > > > > > > > component is used for)?
> > > > > > > >
> > > > > > > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP
> > > > > > > > entry) and the drm_bridge_connector to create the connector. I think that
> > > > > > > > enabling i915 and AMD drivers to use drm_bridge fells out of scope for this
> > > > > > > > series.
> > > > > > > >
> > > > > > > >
> > > > > > > > > Determining the connection between a DP and its Type-C connector is
> > > > > > > > > starting to get really important, so ideally we have a common solution
> > > > > > > > > for that.
> > > > > > > >
> > > > > > > > Yes. This is what we have been discussing with Simon for quite some time on
> > > > > > > > #dri-devel.
> > > > > > > >
> > > > > > > > Unfortunately I think the solution that got merged was pretty much hastened
> > > > > > > > in instead of being well-thought. For example, it is also not always
> > > > > > > > possible to provide the drm_connector / typec_connector links (as you can
> > > > > > > > see from the patch7. Sometimes we can only express that this is a Type-C DP
> > > > > > > > connector, but we can not easily point it to the particular USB-C port.
> > > > > > > >
> > > > > > > > So, I'm not sure, how can we proceed here. Currently merged patch breaks
> > > > > > > > drm/msm if we even try to use it by setting kdef->fwnode to
> > > > > > > > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is
> > > > > > > > an ACPI-only thing, which is not expected to work in a non-ACPI cases.
> > > > > > >
> > > > > > > You really have to always supply not only the Type-C ports and partners,
> > > > > > > but also the alt modes. You need them, firstly to keep things sane
> > > > > > > inside kernel, but more importantly, so they are always exposed to the
> > > > > > > user space, AND, always the same way. We have ABIs for all this stuff,
> > > > > > > including the DP alt mode. Use them. No shortcuts.
> > > > > > >
> > > > > > > So here's what you need to do. UCSI does not seem to bring you
> > > > > > > anything useful, so just disable it for now. You don't need it. Your
> > > > > > > port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so
> > > > > > > that's where you need to register all these components - the ports,
> > > > > > > partners and alt modes. You have all the needed information there.
> > > > > >
> > > > > > To make things even more complicate, UCSI is necessary for the USB part of
> > > > > > the story. It handles vbus and direction.
> > > > > >
> > > > > > > Only after you've done that we can start to look at how should the
> > > > > > > connection between the DPs and their USB Type-C connectors be handled.
> > > > > >
> > > > > > But sure enough, I can add typec port registration to the altmode driver.
> > > > > > This will solve the 'port not existing' part of the story.
> > > > > >
> > > > > > I'd like to hear your opinion on:
> > > > > >
> > > > > > - components. Using them breaks drm/msm. How can we proceed?
> > > > >
> > > > > I don't think replacing the components is going to be a problem once
> > > > > you have described everything properly in you DT. I'm fairly certain now
> > > > > that that is the main problem here. You don't have this connection
> > > > > described in your DT as it should.
> > > >
> > > > We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/
> > > > (for non-PMIC-GLINK platform)
> > > > Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full
> > > > description of USB-C connector and signal flow.
> > > >
> > > > In fact, thanks to this representation I can properly set
> > > > 'connector->fwnode' to point to the OF node corresponding to the
> > > > connector's drm_bridge. I can even propagate it to the kdef->fwnode /
> > > > kdev->of_node in drm_sysfs_connector_add(). But then a component_add()
> > > > call looks the kernel up.
> > > >
> > > > And to add on top of that, here is another reason why I think that
> > > > this sysfs links ABI/implementation was not well thought. The
> > > > typec_connector_ops are added to all fwnode-enabled connector devices.
> > > > It doesn't even bother checking that the device is really the DP
> > > > connector and that the device on the other side of fwnode link is a
> > > > typec port device. The symlink is named 'typec_connector', so one can
> > > > not easily extend this ABI to support SlimPort aka MyDP (which uses
> > > > micro-USB-B connectors instead of USB-C). Neither can we extend it to
> > > > represent MHL connections (again, micro-USB-B).
> > > >
> > > > > > - PATH property usage. This way we make USB-C DisplayPort behave like the
> > > > > > MST ports.
> > > > >
> > > > > That looks to me like an attempt to exploit a feature that is not
> > > > > designed for this purposes at all. Just drop all that.
> > > >
> > > > But why? From the docs: 'Connector path property to identify how this
> > > > sink is physically connected.'
> > > >
> > > > So far we have been using it for MST only. But the description above
> > > > also suits properly for the 'connected to the Type-C port0 device'
> > > > kind of data. Then the userspace can use this property to change the
> > > > representation of the controller. Or to rename it as it does for
> > > > DP-MST connectors. Or just add the USB-C icon in the UI.
> > > >
> > > > Having this data in sysfs only requires userspace first to map the
> > > > connector to the device under sysfs (which is not trivial since Xorg
> > > > renames DP-MST connectors), then to look for the symlink value. Quite
> > > > complicated compared to checking the DRM property.
> > > >
> > > > Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the
> > > > schema to support 'microusb:something' values for this property.
> > > >
> > > > > The connection has to be first described in your DT, and the way you
> > > > > usually describe connections in DT is by using the device graph (OF
> > > > > graph). It seems that you have everything needed for that - the USB
> > > > > Type-C connectors have their own OF nodes (what you register as
> > > > > drm_bridges are in fact USB Type-C connectors), and presumable you
> > > > > also have OF nodes for all your video ports (DisplayPorts) - so
> > > > > applying the graph between the two really should not be a problem. The
> > > > > DP is endpoint for the USB Type-C connector, and vice versa.
> > > >
> > > > Not quite. There is no direct connection between the USB Type-C
> > > > connector and DP controller. The USB-C connector has three ports.
> > > >
> > > > port@0 goes to theHS-USB controller. This is simple.
> > > >
> > > > port@1 goes to the USB+DP PHY. All retimers and SS line muxes are
> > > > included in between. And it is the USB+DP PHY that is connected to the
> > > > DP and USB-SS controllers.
> > > >
> > > > port@2 goes to SBU lines mux (e.g. fsa4480).
> > > >
> > > > > After you have everything needed in your DT, the problem here isn't
> > > > > actually much of a problem at all. We will have options how to move
> > > > > forward after that.
> > > >
> > > > Could you please describe what is missing there?
> > >
> > > We are not after the direct connections here, we are after the final
> > > endpoints. So you are missing description of the logical connection
> > > between your DP and Type-C connector.
> >
> > Adding Krzysztof, as one of DT maintainers, to the CC list.
> >
> > >From what I understand, DT describes hardware. There is no description
> > for the 'logical' connections.
> >
> > >
> > > I understand that the idea is to build the graph to describe only the
> > > physical connections, but with just the physical connections you are
> > > doomed to write separate software solution for almost every single
> > > platform, even though the final endpoints are always the same (DP to
> > > Type-C). You just can not generalise the components (muxes, phys,
> > > retimers, etc.) behind USB Type-C connectors (or anything else for
> > > that matter), it's not possible. The components and their order vary
> > > on almost every single platform. On some platforms the stack of parts
> > > after the connector is also incredibly complex.
> >
> > Yes. And this is why we have a chain of DRM bridges, going from the DP
> > controller to the final drm_bridge at the Type-C port manager. When
> > there is the altmode event, it gets sent via this chain using the
> > normal DRM HPD event.
>
> We will not have drm bridges between the thunderbolt controller and
> the connector, but you still need to be able to show the connector to
> the user when DisplayPort is tunneled over thunderbolt. DP alt mode is
> only one way of getting DisplayPort through USB Type-C. You just can't
> make any assumptions with USB Type-C.

In the end the drm bridge chain is just a funny way to create a
drm_connector. The rest of the system sees just drm_connector, no
matter if internally it is intel_drm_connecttor or
drm_bridge_connector.

>
> The drm bridge chain could only solve the port/connector relationship
> problem from a single angle, but we need a common solution. The
> problem is after all completely generic. It is not DisplayPort
> specific or even USB Type-C specific problem. Those are just two of
> the many possible last endpoints for these connections that need to be
> aware of each other.
>
> So we really have to have a common way of getting this straight from
> the hardware description somehow.

I'd quite disagree with you here. This works in the x86 world, where
the hardware is more or less standard. In the embedded world it is not
that easy. This is why we opted for the software transcribing the
hardware description. In case of ACPI the software part can be common
to all the drivers. But in the embedded systems... I fear is is just
not possible.

> To me the obvious solution would be to just have a port in the graph
> that points directly the last endpoint regardless of what you have in
> between. But if that's not an option, then so be it. Then there just
> needs to be some other way of getting that information from DT.

Could you please point out what is wrong with generating this
information on the fly? We are going to work on fixing the
pmic_glink_altmode in the next couple of weeks, so that the typec port
device will be always present, as you suggested.
Are there any other obstacles?

>
> Maybe DT could use similar physical location object/attribute like
> ACPI - the DP would have matching physical location with its connector?
>
> > > Having the logical final endpoint connection described in your DT/ACPI
> > > on top of the physical connections costs very little, but at the same
> > > time it's usually the only thing that the software needs (like in this
> > > case).
> >
> > Maybe there is some misunderstanding here. We have this connection. We
> > have connector->fwnode and connector->of_node pointing to the correct
> > device - the last bridge in the chain. Each TCPM driver knows the
> > relationship between the in-built drm_bridge and the Type-C port. The
> > DP host controller port can be terminated with other endpoints, e.g.
> > eDP panel. Or there can be a non-DP host, which is then connected
> > through a series of bridges to the eDP or external DP port. This is
> > what anx78xx bridge does: it converts the HDMI link into an external
> > DP (SlimPort) connection. Bridge chains permit this to be handled in a
> > seamless way.
> >
> > What you are asking for looks like a step backwards to me: it requires
> > the host to know that there is a USB-C connector.
> >
> > > So, either you add one more port to your graph for the DP to Type-C
> > > connection, or, if that's not an option, then you need to describe
> > > that connection in some other way. Named references work also quite
> > > well in my experience.
> >
> > Named references were considered and frowned upon by DT maintainers.
>
> thanks,
>
> --
> heikki



-- 
With best wishes
Dmitry

Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"

[Heikki Krogerus]

Hi guys,

On Thu, Sep 14, 2023 at 01:40:49PM +0300, Dmitry Baryshkov wrote:
> On Thu, 14 Sept 2023 at 12:26, Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Hi Dmitry,
> >
> > On Wed, Sep 13, 2023 at 04:47:12PM +0300, Dmitry Baryshkov wrote:
> > > On Wed, 13 Sept 2023 at 16:15, Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote:
> > > > > Hi Heikki,
> > > > >
> > > > > On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus
> > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote:
> > > > > > > On 12/09/2023 14:05, Heikki Krogerus wrote:
> > > > > > > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > On 06/09/2023 16:38, Heikki Krogerus wrote:
> > > > > > > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus
> > > > > > > > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > > > > > Hi Heikki,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus
> > > > > > > > > > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Hi Dmitry,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > > > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so
> > > > > > > > > > > > > > > dev_fwnode() checks never succeed, making the respective commit NOP.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is
> > > > > > > > > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode
> > > > > > > > > > > > > > member is assigned before the device is registered, then that fwnode
> > > > > > > > > > > > > > is assigned also to the device - see drm_connector_acpi_find_companion().
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > But please note that even if drm_connector does not have anything in
> > > > > > > > > > > > > > its fwnode member, the device may still be assigned fwnode, just based
> > > > > > > > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?).
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it
> > > > > > > > > > > > > > > breaks drivers already using components (as it was pointed at [1]),
> > > > > > > > > > > > > > > resulting in a deadlock. Lockdep trace is provided below.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in any
> > > > > > > > > > > > > > > sane way. Revert it instead.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I think there is already user space stuff that relies on these links,
> > > > > > > > > > > > > > so I'm not sure you can just remove them like that. If the component
> > > > > > > > > > > > > > framework is not the correct tool here, then I think you need to
> > > > > > > > > > > > > > suggest some other way of creating them.
> > > > > > > > > > > > >
> > > > > > > > > > > > > The issue (that was pointed out during review) is that having a
> > > > > > > > > > > > > component code in the framework code can lead to lockups. With the
> > > > > > > > > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode
> > > > > > > > > > > > > for non-ACPI systems) probing of drivers which use components and set
> > > > > > > > > > > > > drm_connector::fwnode breaks immediately.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Can we move the component part to the respective drivers? With the
> > > > > > > > > > > > > patch 2 in place, connector->fwnode will be copied to the created
> > > > > > > > > > > > > kdev's fwnode pointer.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Another option might be to make this drm_sysfs component registration optional.
> > > > > > > > > > > >
> > > > > > > > > > > > You don't need to use the component framework at all if there is
> > > > > > > > > > > > a better way of determining the connection between the DP and its
> > > > > > > > > > > > Type-C connector (I'm assuming that that's what this series is about).
> > > > > > > > > > > > You just need the symlinks, not the component.
> > > > > > > > > > >
> > > > > > > > > > > The problem is that right now this component registration has become
> > > > > > > > > > > mandatory. And if I set the kdev->fwnode manually (like in the patch
> > > > > > > > > > > 2), the kernel hangs inside the component code.
> > > > > > > > > > > That's why I proposed to move the components to the place where they
> > > > > > > > > > > are really necessary, e.g. i915 and amd drivers.
> > > > > > > > > >
> > > > > > > > > > So why can't we replace the component with the method you are
> > > > > > > > > > proposing in this series of finding out the Type-C port also with
> > > > > > > > > > i915, AMD, or whatever driver and platform (that's the only thing that
> > > > > > > > > > component is used for)?
> > > > > > > > >
> > > > > > > > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP
> > > > > > > > > entry) and the drm_bridge_connector to create the connector. I think that
> > > > > > > > > enabling i915 and AMD drivers to use drm_bridge fells out of scope for this
> > > > > > > > > series.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > Determining the connection between a DP and its Type-C connector is
> > > > > > > > > > starting to get really important, so ideally we have a common solution
> > > > > > > > > > for that.
> > > > > > > > >
> > > > > > > > > Yes. This is what we have been discussing with Simon for quite some time on
> > > > > > > > > #dri-devel.
> > > > > > > > >
> > > > > > > > > Unfortunately I think the solution that got merged was pretty much hastened
> > > > > > > > > in instead of being well-thought. For example, it is also not always
> > > > > > > > > possible to provide the drm_connector / typec_connector links (as you can
> > > > > > > > > see from the patch7. Sometimes we can only express that this is a Type-C DP
> > > > > > > > > connector, but we can not easily point it to the particular USB-C port.
> > > > > > > > >
> > > > > > > > > So, I'm not sure, how can we proceed here. Currently merged patch breaks
> > > > > > > > > drm/msm if we even try to use it by setting kdef->fwnode to
> > > > > > > > > drm_connector->fwnode. The pointed out `drivers/usb/typec/port-mapper.c` is
> > > > > > > > > an ACPI-only thing, which is not expected to work in a non-ACPI cases.
> > > > > > > >
> > > > > > > > You really have to always supply not only the Type-C ports and partners,
> > > > > > > > but also the alt modes. You need them, firstly to keep things sane
> > > > > > > > inside kernel, but more importantly, so they are always exposed to the
> > > > > > > > user space, AND, always the same way. We have ABIs for all this stuff,
> > > > > > > > including the DP alt mode. Use them. No shortcuts.
> > > > > > > >
> > > > > > > > So here's what you need to do. UCSI does not seem to bring you
> > > > > > > > anything useful, so just disable it for now. You don't need it. Your
> > > > > > > > port driver is clearly drivers/soc/qcom/pmic_glink_altmode.c, so
> > > > > > > > that's where you need to register all these components - the ports,
> > > > > > > > partners and alt modes. You have all the needed information there.
> > > > > > >
> > > > > > > To make things even more complicate, UCSI is necessary for the USB part of
> > > > > > > the story. It handles vbus and direction.
> > > > > > >
> > > > > > > > Only after you've done that we can start to look at how should the
> > > > > > > > connection between the DPs and their USB Type-C connectors be handled.
> > > > > > >
> > > > > > > But sure enough, I can add typec port registration to the altmode driver.
> > > > > > > This will solve the 'port not existing' part of the story.
> > > > > > >
> > > > > > > I'd like to hear your opinion on:
> > > > > > >
> > > > > > > - components. Using them breaks drm/msm. How can we proceed?
> > > > > >
> > > > > > I don't think replacing the components is going to be a problem once
> > > > > > you have described everything properly in you DT. I'm fairly certain now
> > > > > > that that is the main problem here. You don't have this connection
> > > > > > described in your DT as it should.
> > > > >
> > > > > We have. See https://lore.kernel.org/linux-arm-msm/20230817145940.9887-1-dmitry.baryshkov@linaro.org/
> > > > > (for non-PMIC-GLINK platform)
> > > > > Or arch/arm64/boot/dts/qcom/sm8350-hdk.dts, which already has a full
> > > > > description of USB-C connector and signal flow.
> > > > >
> > > > > In fact, thanks to this representation I can properly set
> > > > > 'connector->fwnode' to point to the OF node corresponding to the
> > > > > connector's drm_bridge. I can even propagate it to the kdef->fwnode /
> > > > > kdev->of_node in drm_sysfs_connector_add(). But then a component_add()
> > > > > call looks the kernel up.
> > > > >
> > > > > And to add on top of that, here is another reason why I think that
> > > > > this sysfs links ABI/implementation was not well thought. The
> > > > > typec_connector_ops are added to all fwnode-enabled connector devices.
> > > > > It doesn't even bother checking that the device is really the DP
> > > > > connector and that the device on the other side of fwnode link is a
> > > > > typec port device. The symlink is named 'typec_connector', so one can
> > > > > not easily extend this ABI to support SlimPort aka MyDP (which uses
> > > > > micro-USB-B connectors instead of USB-C). Neither can we extend it to
> > > > > represent MHL connections (again, micro-USB-B).
> > > > >
> > > > > > > - PATH property usage. This way we make USB-C DisplayPort behave like the
> > > > > > > MST ports.
> > > > > >
> > > > > > That looks to me like an attempt to exploit a feature that is not
> > > > > > designed for this purposes at all. Just drop all that.
> > > > >
> > > > > But why? From the docs: 'Connector path property to identify how this
> > > > > sink is physically connected.'
> > > > >
> > > > > So far we have been using it for MST only. But the description above
> > > > > also suits properly for the 'connected to the Type-C port0 device'
> > > > > kind of data. Then the userspace can use this property to change the
> > > > > representation of the controller. Or to rename it as it does for
> > > > > DP-MST connectors. Or just add the USB-C icon in the UI.
> > > > >
> > > > > Having this data in sysfs only requires userspace first to map the
> > > > > connector to the device under sysfs (which is not trivial since Xorg
> > > > > renames DP-MST connectors), then to look for the symlink value. Quite
> > > > > complicated compared to checking the DRM property.
> > > > >
> > > > > Moreover, once we get to the SlimPort / MyDP / MHL, we can extend the
> > > > > schema to support 'microusb:something' values for this property.
> > > > >
> > > > > > The connection has to be first described in your DT, and the way you
> > > > > > usually describe connections in DT is by using the device graph (OF
> > > > > > graph). It seems that you have everything needed for that - the USB
> > > > > > Type-C connectors have their own OF nodes (what you register as
> > > > > > drm_bridges are in fact USB Type-C connectors), and presumable you
> > > > > > also have OF nodes for all your video ports (DisplayPorts) - so
> > > > > > applying the graph between the two really should not be a problem. The
> > > > > > DP is endpoint for the USB Type-C connector, and vice versa.
> > > > >
> > > > > Not quite. There is no direct connection between the USB Type-C
> > > > > connector and DP controller. The USB-C connector has three ports.
> > > > >
> > > > > port@0 goes to theHS-USB controller. This is simple.
> > > > >
> > > > > port@1 goes to the USB+DP PHY. All retimers and SS line muxes are
> > > > > included in between. And it is the USB+DP PHY that is connected to the
> > > > > DP and USB-SS controllers.
> > > > >
> > > > > port@2 goes to SBU lines mux (e.g. fsa4480).
> > > > >
> > > > > > After you have everything needed in your DT, the problem here isn't
> > > > > > actually much of a problem at all. We will have options how to move
> > > > > > forward after that.
> > > > >
> > > > > Could you please describe what is missing there?
> > > >
> > > > We are not after the direct connections here, we are after the final
> > > > endpoints. So you are missing description of the logical connection
> > > > between your DP and Type-C connector.
> > >
> > > Adding Krzysztof, as one of DT maintainers, to the CC list.
> > >
> > > >From what I understand, DT describes hardware. There is no description
> > > for the 'logical' connections.
> > >
> > > >
> > > > I understand that the idea is to build the graph to describe only the
> > > > physical connections, but with just the physical connections you are
> > > > doomed to write separate software solution for almost every single
> > > > platform, even though the final endpoints are always the same (DP to
> > > > Type-C). You just can not generalise the components (muxes, phys,
> > > > retimers, etc.) behind USB Type-C connectors (or anything else for
> > > > that matter), it's not possible. The components and their order vary
> > > > on almost every single platform. On some platforms the stack of parts
> > > > after the connector is also incredibly complex.
> > >
> > > Yes. And this is why we have a chain of DRM bridges, going from the DP
> > > controller to the final drm_bridge at the Type-C port manager. When
> > > there is the altmode event, it gets sent via this chain using the
> > > normal DRM HPD event.
> >
> > We will not have drm bridges between the thunderbolt controller and
> > the connector, but you still need to be able to show the connector to
> > the user when DisplayPort is tunneled over thunderbolt. DP alt mode is
> > only one way of getting DisplayPort through USB Type-C. You just can't
> > make any assumptions with USB Type-C.
> 
> In the end the drm bridge chain is just a funny way to create a
> drm_connector. The rest of the system sees just drm_connector, no
> matter if internally it is intel_drm_connecttor or
> drm_bridge_connector.
> 
> >
> > The drm bridge chain could only solve the port/connector relationship
> > problem from a single angle, but we need a common solution. The
> > problem is after all completely generic. It is not DisplayPort
> > specific or even USB Type-C specific problem. Those are just two of
> > the many possible last endpoints for these connections that need to be
> > aware of each other.
> >
> > So we really have to have a common way of getting this straight from
> > the hardware description somehow.
> 
> I'd quite disagree with you here. This works in the x86 world, where
> the hardware is more or less standard. In the embedded world it is not
> that easy. This is why we opted for the software transcribing the
> hardware description. In case of ACPI the software part can be common
> to all the drivers. But in the embedded systems... I fear is is just
> not possible.
> 
> > To me the obvious solution would be to just have a port in the graph
> > that points directly the last endpoint regardless of what you have in
> > between. But if that's not an option, then so be it. Then there just
> > needs to be some other way of getting that information from DT.
> 
> Could you please point out what is wrong with generating this
> information on the fly? We are going to work on fixing the
> pmic_glink_altmode in the next couple of weeks, so that the typec port
> device will be always present, as you suggested.
> Are there any other obstacles?
> 
> > Maybe DT could use similar physical location object/attribute like
> > ACPI - the DP would have matching physical location with its connector?
> >
> > > > Having the logical final endpoint connection described in your DT/ACPI
> > > > on top of the physical connections costs very little, but at the same
> > > > time it's usually the only thing that the software needs (like in this
> > > > case).
> > >
> > > Maybe there is some misunderstanding here. We have this connection. We
> > > have connector->fwnode and connector->of_node pointing to the correct
> > > device - the last bridge in the chain. Each TCPM driver knows the
> > > relationship between the in-built drm_bridge and the Type-C port. The
> > > DP host controller port can be terminated with other endpoints, e.g.
> > > eDP panel. Or there can be a non-DP host, which is then connected
> > > through a series of bridges to the eDP or external DP port. This is
> > > what anx78xx bridge does: it converts the HDMI link into an external
> > > DP (SlimPort) connection. Bridge chains permit this to be handled in a
> > > seamless way.

I'm sorry, I did not read this comment carefully enough earlier. I
probable have misunderstood something related to the drm bridges.

I'm still not sure why would it be a problem to try to hook up the
port and connector device nodes - you seem to have them ready in any
case, no?
But maybe it's best that you guys just prepare the next version at
this point. Let's continue then.

> > > What you are asking for looks like a step backwards to me: it requires
> > > the host to know that there is a USB-C connector.
> > >
> > > > So, either you add one more port to your graph for the DP to Type-C
> > > > connection, or, if that's not an option, then you need to describe
> > > > that connection in some other way. Named references work also quite
> > > > well in my experience.
> > >
> > > Named references were considered and frowned upon by DT maintainers.

thanks,

-- 
heikki

Re: [RFC PATCH v1 12/12] usb: typec: qcom: define the bridge's path

[Heikki Krogerus]

Hi Dmitry,

On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
> In order to notify the userspace about the DRM connector's USB-C port,
> export the corresponding port's name as the bridge's path field.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c     | 11 +++++++----
>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c |  4 +++-
>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h |  6 ++++--
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> index b9d4856101c7..452dc6437861 100644
> --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>  	struct device_node *np = dev->of_node;
>  	const struct pmic_typec_resources *res;
>  	struct regmap *regmap;
> +	char *tcpm_name;
>  	u32 base[2];
>  	int ret;
>  
> @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>  	mutex_init(&tcpm->lock);
>  	platform_set_drvdata(pdev, tcpm);
>  
> -	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
> -	if (IS_ERR(tcpm->pmic_typec_drm))
> -		return PTR_ERR(tcpm->pmic_typec_drm);
> -
>  	tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
>  	if (!tcpm->tcpc.fwnode)
>  		return -EINVAL;
> @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>  		goto fwnode_remove;
>  	}
>  
> +	tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
> +	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);

So I got some questions and concerns off-list. This was one of the
concerns. That tcpm_name is now the actual port device name, so I'm
afraid this is not acceptable.

You can't use device name as a reference, ever. There is no way to
guarantee that a device with a specific name is what you meant it to
be by the time it's accessed.

If you need to deal with a device, then you have to get an actual
reference to it (class_find_device_by_fwnode() should work in this
case).

Ideally you would get the reference in the place where you actually
use it (so drm_connector.c or more likely drm_sysfs.c) but that would
mean a dependency on typec in there, if the component framework or
something like that (device links?) is not an option. You could of
course try to confine the dependency somehow. drm_class does not have
implementation for dev_uevent, so you could take over that as a
temporary solution.

The only way to avoid the dependency completely would be to pass that
device reference from here through your drm bridge chain somehow.
But that's also really fragile. But it could be acceptable as a
temporary solution perhaps, if it's even possible.

Br,

-- 
heikki

Re: [RFC PATCH v1 03/12] drm/connector: extend PATH property to covert Type-C case

[Simon Ser]

On Sunday, September 3rd, 2023 at 23:41, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> To facilitate this, reuse the 'PATH' property, which was used to
> describe the DP port in the DP MST configuration. Use either
> 'typec:portN' to point out to the Type-C port class device, or just
> 'typec:' if the corresponding port can not be identified.

Typo: should be "typec" without the colon I think?

What are the situations where the port cannot be identified? It seems
weird to use the PATH property in that case.

Re: [RFC PATCH v1 12/12] usb: typec: qcom: define the bridge's path

[Dmitry Baryshkov]

On 15 September 2023 15:14:35 EEST, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
>Hi Dmitry,
>
>On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
>> In order to notify the userspace about the DRM connector's USB-C port,
>> export the corresponding port's name as the bridge's path field.
>> 
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c     | 11 +++++++----
>>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c |  4 +++-
>>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h |  6 ++++--
>>  3 files changed, 14 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
>> index b9d4856101c7..452dc6437861 100644
>> --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
>> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
>> @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>>  	struct device_node *np = dev->of_node;
>>  	const struct pmic_typec_resources *res;
>>  	struct regmap *regmap;
>> +	char *tcpm_name;
>>  	u32 base[2];
>>  	int ret;
>>  
>> @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>>  	mutex_init(&tcpm->lock);
>>  	platform_set_drvdata(pdev, tcpm);
>>  
>> -	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
>> -	if (IS_ERR(tcpm->pmic_typec_drm))
>> -		return PTR_ERR(tcpm->pmic_typec_drm);
>> -
>>  	tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
>>  	if (!tcpm->tcpc.fwnode)
>>  		return -EINVAL;
>> @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
>>  		goto fwnode_remove;
>>  	}
>>  
>> +	tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
>> +	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
>
>So I got some questions and concerns off-list. This was one of the
>concerns. That tcpm_name is now the actual port device name, so I'm
>afraid this is not acceptable.
>
>You can't use device name as a reference, ever. There is no way to
>guarantee that a device with a specific name is what you meant it to
>be by the time it's accessed.

Hmm, could you please be more specific, why? I mean, class devices are not that easy to be renamed in sysfs, are they? Or are you concerned about the device being destroyed behind userspace's back? At least for MSM this will be a huge problem already, with the bridge driver suddenly being removed.

>
>If you need to deal with a device, then you have to get an actual
>reference to it (class_find_device_by_fwnode() should work in this
>case).
>
>Ideally you would get the reference in the place where you actually
>use it (so drm_connector.c or more likely drm_sysfs.c) but that would
>mean a dependency on typec in there, if the component framework or
>something like that (device links?) is not an option. You could of
>course try to confine the dependency somehow. drm_class does not have
>implementation for dev_uevent, so you could take over that as a
>temporary solution.
>
>The only way to avoid the dependency completely would be to pass that
>device reference from here through your drm bridge chain somehow.
>But that's also really fragile. But it could be acceptable as a
>temporary solution perhaps, if it's even possible.
>
>Br,
>

Re: [RFC PATCH v1 12/12] usb: typec: qcom: define the bridge's path

[Heikki Krogerus]

On Mon, Oct 23, 2023 at 09:24:33PM +0300, Dmitry Baryshkov wrote:
> On 15 September 2023 15:14:35 EEST, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> >Hi Dmitry,
> >
> >On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
> >> In order to notify the userspace about the DRM connector's USB-C port,
> >> export the corresponding port's name as the bridge's path field.
> >> 
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> ---
> >>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c     | 11 +++++++----
> >>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c |  4 +++-
> >>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h |  6 ++++--
> >>  3 files changed, 14 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> >> index b9d4856101c7..452dc6437861 100644
> >> --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> >> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> >> @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> >>  	struct device_node *np = dev->of_node;
> >>  	const struct pmic_typec_resources *res;
> >>  	struct regmap *regmap;
> >> +	char *tcpm_name;
> >>  	u32 base[2];
> >>  	int ret;
> >>  
> >> @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> >>  	mutex_init(&tcpm->lock);
> >>  	platform_set_drvdata(pdev, tcpm);
> >>  
> >> -	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
> >> -	if (IS_ERR(tcpm->pmic_typec_drm))
> >> -		return PTR_ERR(tcpm->pmic_typec_drm);
> >> -
> >>  	tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
> >>  	if (!tcpm->tcpc.fwnode)
> >>  		return -EINVAL;
> >> @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> >>  		goto fwnode_remove;
> >>  	}
> >>  
> >> +	tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
> >> +	tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
> >
> >So I got some questions and concerns off-list. This was one of the
> >concerns. That tcpm_name is now the actual port device name, so I'm
> >afraid this is not acceptable.
> >
> >You can't use device name as a reference, ever. There is no way to
> >guarantee that a device with a specific name is what you meant it to
> >be by the time it's accessed.
> 
> Hmm, could you please be more specific, why? I mean, class devices are not
> that easy to be renamed in sysfs, are they? Or are you concerned about the
> device being destroyed behind userspace's back? At least for MSM this will be
> a huge problem already, with the bridge driver suddenly being removed.

The race exists even in your case, but please do not look at this as a
solution for only your platform.

This is about showing the user space a link between two device
instances (struct device), and the way you do that is by creating a
symlink. That way the kernel can take care of reference counting and
guarantee that the link always points to the correct device. That way
the link will also be always visible in user space without requirement
for any specific ABI like it should.

thanks,

-- 
heikki

Re: [RFC PATCH v1 12/12] usb: typec: qcom: define the bridge's path

[Dmitry Baryshkov]

On Mon, 30 Oct 2023 at 10:19, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Mon, Oct 23, 2023 at 09:24:33PM +0300, Dmitry Baryshkov wrote:
> > On 15 September 2023 15:14:35 EEST, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> > >Hi Dmitry,
> > >
> > >On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
> > >> In order to notify the userspace about the DRM connector's USB-C port,
> > >> export the corresponding port's name as the bridge's path field.
> > >>
> > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > >> ---
> > >>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c     | 11 +++++++----
> > >>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c |  4 +++-
> > >>  drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h |  6 ++++--
> > >>  3 files changed, 14 insertions(+), 7 deletions(-)
> > >>
> > >> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > >> index b9d4856101c7..452dc6437861 100644
> > >> --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > >> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > >> @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > >>    struct device_node *np = dev->of_node;
> > >>    const struct pmic_typec_resources *res;
> > >>    struct regmap *regmap;
> > >> +  char *tcpm_name;
> > >>    u32 base[2];
> > >>    int ret;
> > >>
> > >> @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > >>    mutex_init(&tcpm->lock);
> > >>    platform_set_drvdata(pdev, tcpm);
> > >>
> > >> -  tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
> > >> -  if (IS_ERR(tcpm->pmic_typec_drm))
> > >> -          return PTR_ERR(tcpm->pmic_typec_drm);
> > >> -
> > >>    tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
> > >>    if (!tcpm->tcpc.fwnode)
> > >>            return -EINVAL;
> > >> @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > >>            goto fwnode_remove;
> > >>    }
> > >>
> > >> +  tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
> > >> +  tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
> > >
> > >So I got some questions and concerns off-list. This was one of the
> > >concerns. That tcpm_name is now the actual port device name, so I'm
> > >afraid this is not acceptable.
> > >
> > >You can't use device name as a reference, ever. There is no way to
> > >guarantee that a device with a specific name is what you meant it to
> > >be by the time it's accessed.
> >
> > Hmm, could you please be more specific, why? I mean, class devices are not
> > that easy to be renamed in sysfs, are they? Or are you concerned about the
> > device being destroyed behind userspace's back? At least for MSM this will be
> > a huge problem already, with the bridge driver suddenly being removed.
>
> The race exists even in your case, but please do not look at this as a
> solution for only your platform.

Yes!

>
> This is about showing the user space a link between two device
> instances (struct device), and the way you do that is by creating a
> symlink. That way the kernel can take care of reference counting and
> guarantee that the link always points to the correct device. That way
> the link will also be always visible in user space without requirement
> for any specific ABI like it should.

I'm fine with the symlink approach (and I'll follow that up after
finishing the UCSI glue driver rework). However I feel several
deficiencies there:

1) It creates asymmetry with the DP MST case. Do we want to have
symlinks in each of the MST connectors? Or do we follow the PATH
properties in the MST case until we find the root port, which has
symlink? Please note, that fine X11 renames DP MST connectors
internally, so in xrandr I see DP-2-1, which maps to
/sys/class/drm/card0-DP-2. Kind of hard to follow.

2) For the multi-card cases, one has to remap the connector to the
card index + connector path. And this needs to be done by all user
space applications, which would like to present this kind of
information for the user.

3) If we were to support non-USB-C connectors (e.g. MyDP / SlimPort
and MHL used simple micro-USB connectors) there would be a completely
new uABI. And any external port / wrapper will also require a
completely new symlink kind.

I understand your concerns regarding mentioning external device in the
PATH property. However I think we should make it easier for the
userspace app to determine the kind of the external connector. What
would you think about extending the PATH property in the following
way:

For the USB-C connectors the PATH property has the value of
`typec:cardN-DP-m` value. Userspace app can then look for the
typec_connector symlink at the /sys/class/drm/cardN-DP-m subdir to
find the information about the corresponding USB-C port.

In future this will allow us to define e.g.:

For the SlimPort / MyDP the PATH property has the value of
`micro_usb:cardN-HDMI-m` or `micro_usb:cardN-DP-m`. The symlink is
called /sys/class/drm/cardN-DP-m/micro_usb_connector.

Or:

For the SlimPort / MyDP the PATH property has the value of
`mydp:cardN-HDMI-m` or `mydp:cardN-DP-m`. The symlink is called
/sys/class/drm/cardN-DP-m/mydp_connector.


-- 
With best wishes
Dmitry

Re: [RFC PATCH v1 12/12] usb: typec: qcom: define the bridge's path

[Simon Ser]

On Monday, October 30th, 2023 at 10:47, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Mon, 30 Oct 2023 at 10:19, Heikki Krogerus
> heikki.krogerus@linux.intel.com wrote:
> 
> > On Mon, Oct 23, 2023 at 09:24:33PM +0300, Dmitry Baryshkov wrote:
> > 
> > > On 15 September 2023 15:14:35 EEST, Heikki Krogerus heikki.krogerus@linux.intel.com wrote:
> > > 
> > > > Hi Dmitry,
> > > > 
> > > > On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
> > > > 
> > > > > In order to notify the userspace about the DRM connector's USB-C port,
> > > > > export the corresponding port's name as the bridge's path field.
> > > > > 
> > > > > Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
> > > > > ---
> > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 11 +++++++----
> > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c | 4 +++-
> > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h | 6 ++++--
> > > > > 3 files changed, 14 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > index b9d4856101c7..452dc6437861 100644
> > > > > --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > struct device_node *np = dev->of_node;
> > > > > const struct pmic_typec_resources *res;
> > > > > struct regmap *regmap;
> > > > > + char *tcpm_name;
> > > > > u32 base[2];
> > > > > int ret;
> > > > > 
> > > > > @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > mutex_init(&tcpm->lock);
> > > > > platform_set_drvdata(pdev, tcpm);
> > > > > 
> > > > > - tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
> > > > > - if (IS_ERR(tcpm->pmic_typec_drm))
> > > > > - return PTR_ERR(tcpm->pmic_typec_drm);
> > > > > -
> > > > > tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
> > > > > if (!tcpm->tcpc.fwnode)
> > > > > return -EINVAL;
> > > > > @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > goto fwnode_remove;
> > > > > }
> > > > > 
> > > > > + tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
> > > > > + tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
> > > > 
> > > > So I got some questions and concerns off-list. This was one of the
> > > > concerns. That tcpm_name is now the actual port device name, so I'm
> > > > afraid this is not acceptable.
> > > > 
> > > > You can't use device name as a reference, ever. There is no way to
> > > > guarantee that a device with a specific name is what you meant it to
> > > > be by the time it's accessed.
> > > 
> > > Hmm, could you please be more specific, why? I mean, class devices are not
> > > that easy to be renamed in sysfs, are they? Or are you concerned about the
> > > device being destroyed behind userspace's back? At least for MSM this will be
> > > a huge problem already, with the bridge driver suddenly being removed.
> > 
> > The race exists even in your case, but please do not look at this as a
> > solution for only your platform.
> 
> Yes!
> 
> > This is about showing the user space a link between two device
> > instances (struct device), and the way you do that is by creating a
> > symlink. That way the kernel can take care of reference counting and
> > guarantee that the link always points to the correct device. That way
> > the link will also be always visible in user space without requirement
> > for any specific ABI like it should.
> 
> I'm fine with the symlink approach (and I'll follow that up after
> finishing the UCSI glue driver rework). However I feel several
> deficiencies there:
> 
> 1) It creates asymmetry with the DP MST case. Do we want to have
> symlinks in each of the MST connectors? Or do we follow the PATH
> properties in the MST case until we find the root port, which has
> symlink? Please note, that fine X11 renames DP MST connectors
> internally, so in xrandr I see DP-2-1, which maps to
> /sys/class/drm/card0-DP-2. Kind of hard to follow.
> 
> 2) For the multi-card cases, one has to remap the connector to the
> card index + connector path. And this needs to be done by all user
> space applications, which would like to present this kind of
> information for the user.
> 
> 3) If we were to support non-USB-C connectors (e.g. MyDP / SlimPort
> and MHL used simple micro-USB connectors) there would be a completely
> new uABI. And any external port / wrapper will also require a
> completely new symlink kind.
> 
> I understand your concerns regarding mentioning external device in the
> PATH property. However I think we should make it easier for the
> userspace app to determine the kind of the external connector. What
> would you think about extending the PATH property in the following
> way:
> 
> For the USB-C connectors the PATH property has the value of
> `typec:cardN-DP-m` value. Userspace app can then look for the
> typec_connector symlink at the /sys/class/drm/cardN-DP-m subdir to
> find the information about the corresponding USB-C port.

This doesn't make sense to me. "cardN-DP-m" has nothing to do with the
physical path of the connector. All of the parts of this string are
exposed elsewhere in the KMS uAPI already.

> In future this will allow us to define e.g.:
> 
> For the SlimPort / MyDP the PATH property has the value of
> `micro_usb:cardN-HDMI-m` or `micro_usb:cardN-DP-m`. The symlink is
> called /sys/class/drm/cardN-DP-m/micro_usb_connector.
> 
> Or:
> 
> For the SlimPort / MyDP the PATH property has the value of
> `mydp:cardN-HDMI-m` or `mydp:cardN-DP-m`. The symlink is called
> /sys/class/drm/cardN-DP-m/mydp_connector.
> 
> 
> --
> With best wishes
> Dmitry

Re: [RFC PATCH v1 12/12] usb: typec: qcom: define the bridge's path

[Dmitry Baryshkov]

On Mon, 30 Oct 2023 at 12:13, Simon Ser <contact@emersion.fr> wrote:
>
> On Monday, October 30th, 2023 at 10:47, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>
> > On Mon, 30 Oct 2023 at 10:19, Heikki Krogerus
> > heikki.krogerus@linux.intel.com wrote:
> >
> > > On Mon, Oct 23, 2023 at 09:24:33PM +0300, Dmitry Baryshkov wrote:
> > >
> > > > On 15 September 2023 15:14:35 EEST, Heikki Krogerus heikki.krogerus@linux.intel.com wrote:
> > > >
> > > > > Hi Dmitry,
> > > > >
> > > > > On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
> > > > >
> > > > > > In order to notify the userspace about the DRM connector's USB-C port,
> > > > > > export the corresponding port's name as the bridge's path field.
> > > > > >
> > > > > > Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
> > > > > > ---
> > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 11 +++++++----
> > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c | 4 +++-
> > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h | 6 ++++--
> > > > > > 3 files changed, 14 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > index b9d4856101c7..452dc6437861 100644
> > > > > > --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > struct device_node *np = dev->of_node;
> > > > > > const struct pmic_typec_resources *res;
> > > > > > struct regmap *regmap;
> > > > > > + char *tcpm_name;
> > > > > > u32 base[2];
> > > > > > int ret;
> > > > > >
> > > > > > @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > mutex_init(&tcpm->lock);
> > > > > > platform_set_drvdata(pdev, tcpm);
> > > > > >
> > > > > > - tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
> > > > > > - if (IS_ERR(tcpm->pmic_typec_drm))
> > > > > > - return PTR_ERR(tcpm->pmic_typec_drm);
> > > > > > -
> > > > > > tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
> > > > > > if (!tcpm->tcpc.fwnode)
> > > > > > return -EINVAL;
> > > > > > @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > goto fwnode_remove;
> > > > > > }
> > > > > >
> > > > > > + tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
> > > > > > + tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
> > > > >
> > > > > So I got some questions and concerns off-list. This was one of the
> > > > > concerns. That tcpm_name is now the actual port device name, so I'm
> > > > > afraid this is not acceptable.
> > > > >
> > > > > You can't use device name as a reference, ever. There is no way to
> > > > > guarantee that a device with a specific name is what you meant it to
> > > > > be by the time it's accessed.
> > > >
> > > > Hmm, could you please be more specific, why? I mean, class devices are not
> > > > that easy to be renamed in sysfs, are they? Or are you concerned about the
> > > > device being destroyed behind userspace's back? At least for MSM this will be
> > > > a huge problem already, with the bridge driver suddenly being removed.
> > >
> > > The race exists even in your case, but please do not look at this as a
> > > solution for only your platform.
> >
> > Yes!
> >
> > > This is about showing the user space a link between two device
> > > instances (struct device), and the way you do that is by creating a
> > > symlink. That way the kernel can take care of reference counting and
> > > guarantee that the link always points to the correct device. That way
> > > the link will also be always visible in user space without requirement
> > > for any specific ABI like it should.
> >
> > I'm fine with the symlink approach (and I'll follow that up after
> > finishing the UCSI glue driver rework). However I feel several
> > deficiencies there:
> >
> > 1) It creates asymmetry with the DP MST case. Do we want to have
> > symlinks in each of the MST connectors? Or do we follow the PATH
> > properties in the MST case until we find the root port, which has
> > symlink? Please note, that fine X11 renames DP MST connectors
> > internally, so in xrandr I see DP-2-1, which maps to
> > /sys/class/drm/card0-DP-2. Kind of hard to follow.
> >
> > 2) For the multi-card cases, one has to remap the connector to the
> > card index + connector path. And this needs to be done by all user
> > space applications, which would like to present this kind of
> > information for the user.
> >
> > 3) If we were to support non-USB-C connectors (e.g. MyDP / SlimPort
> > and MHL used simple micro-USB connectors) there would be a completely
> > new uABI. And any external port / wrapper will also require a
> > completely new symlink kind.
> >
> > I understand your concerns regarding mentioning external device in the
> > PATH property. However I think we should make it easier for the
> > userspace app to determine the kind of the external connector. What
> > would you think about extending the PATH property in the following
> > way:
> >
> > For the USB-C connectors the PATH property has the value of
> > `typec:cardN-DP-m` value. Userspace app can then look for the
> > typec_connector symlink at the /sys/class/drm/cardN-DP-m subdir to
> > find the information about the corresponding USB-C port.
>
> This doesn't make sense to me. "cardN-DP-m" has nothing to do with the
> physical path of the connector. All of the parts of this string are
> exposed elsewhere in the KMS uAPI already.

True. It seems I mixed KMS and xrandr clients in my head.
Just 'typec:' then? This way userspace will still know that it is a
USB-C connector (and can stop there) and if it needs more information
(e.g. physical location) it can further look for the symlink in the
sysfs.

>
> > In future this will allow us to define e.g.:
> >
> > For the SlimPort / MyDP the PATH property has the value of
> > `micro_usb:cardN-HDMI-m` or `micro_usb:cardN-DP-m`. The symlink is
> > called /sys/class/drm/cardN-DP-m/micro_usb_connector.
> >
> > Or:
> >
> > For the SlimPort / MyDP the PATH property has the value of
> > `mydp:cardN-HDMI-m` or `mydp:cardN-DP-m`. The symlink is called
> > /sys/class/drm/cardN-DP-m/mydp_connector.
> >
> >
> > --
> > With best wishes
> > Dmitry



-- 
With best wishes
Dmitry

Re: [RFC PATCH v1 12/12] usb: typec: qcom: define the bridge's path

[Simon Ser]

On Monday, October 30th, 2023 at 11:22, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Mon, 30 Oct 2023 at 12:13, Simon Ser contact@emersion.fr wrote:
> 
> > On Monday, October 30th, 2023 at 10:47, Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
> > 
> > > On Mon, 30 Oct 2023 at 10:19, Heikki Krogerus
> > > heikki.krogerus@linux.intel.com wrote:
> > > 
> > > > On Mon, Oct 23, 2023 at 09:24:33PM +0300, Dmitry Baryshkov wrote:
> > > > 
> > > > > On 15 September 2023 15:14:35 EEST, Heikki Krogerus heikki.krogerus@linux.intel.com wrote:
> > > > > 
> > > > > > Hi Dmitry,
> > > > > > 
> > > > > > On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
> > > > > > 
> > > > > > > In order to notify the userspace about the DRM connector's USB-C port,
> > > > > > > export the corresponding port's name as the bridge's path field.
> > > > > > > 
> > > > > > > Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
> > > > > > > ---
> > > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 11 +++++++----
> > > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c | 4 +++-
> > > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h | 6 ++++--
> > > > > > > 3 files changed, 14 insertions(+), 7 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > > index b9d4856101c7..452dc6437861 100644
> > > > > > > --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > > +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > > @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > > struct device_node *np = dev->of_node;
> > > > > > > const struct pmic_typec_resources *res;
> > > > > > > struct regmap *regmap;
> > > > > > > + char *tcpm_name;
> > > > > > > u32 base[2];
> > > > > > > int ret;
> > > > > > > 
> > > > > > > @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > > mutex_init(&tcpm->lock);
> > > > > > > platform_set_drvdata(pdev, tcpm);
> > > > > > > 
> > > > > > > - tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
> > > > > > > - if (IS_ERR(tcpm->pmic_typec_drm))
> > > > > > > - return PTR_ERR(tcpm->pmic_typec_drm);
> > > > > > > -
> > > > > > > tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
> > > > > > > if (!tcpm->tcpc.fwnode)
> > > > > > > return -EINVAL;
> > > > > > > @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > > goto fwnode_remove;
> > > > > > > }
> > > > > > > 
> > > > > > > + tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
> > > > > > > + tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
> > > > > > 
> > > > > > So I got some questions and concerns off-list. This was one of the
> > > > > > concerns. That tcpm_name is now the actual port device name, so I'm
> > > > > > afraid this is not acceptable.
> > > > > > 
> > > > > > You can't use device name as a reference, ever. There is no way to
> > > > > > guarantee that a device with a specific name is what you meant it to
> > > > > > be by the time it's accessed.
> > > > > 
> > > > > Hmm, could you please be more specific, why? I mean, class devices are not
> > > > > that easy to be renamed in sysfs, are they? Or are you concerned about the
> > > > > device being destroyed behind userspace's back? At least for MSM this will be
> > > > > a huge problem already, with the bridge driver suddenly being removed.
> > > > 
> > > > The race exists even in your case, but please do not look at this as a
> > > > solution for only your platform.
> > > 
> > > Yes!
> > > 
> > > > This is about showing the user space a link between two device
> > > > instances (struct device), and the way you do that is by creating a
> > > > symlink. That way the kernel can take care of reference counting and
> > > > guarantee that the link always points to the correct device. That way
> > > > the link will also be always visible in user space without requirement
> > > > for any specific ABI like it should.
> > > 
> > > I'm fine with the symlink approach (and I'll follow that up after
> > > finishing the UCSI glue driver rework). However I feel several
> > > deficiencies there:
> > > 
> > > 1) It creates asymmetry with the DP MST case. Do we want to have
> > > symlinks in each of the MST connectors? Or do we follow the PATH
> > > properties in the MST case until we find the root port, which has
> > > symlink? Please note, that fine X11 renames DP MST connectors
> > > internally, so in xrandr I see DP-2-1, which maps to
> > > /sys/class/drm/card0-DP-2. Kind of hard to follow.
> > > 
> > > 2) For the multi-card cases, one has to remap the connector to the
> > > card index + connector path. And this needs to be done by all user
> > > space applications, which would like to present this kind of
> > > information for the user.
> > > 
> > > 3) If we were to support non-USB-C connectors (e.g. MyDP / SlimPort
> > > and MHL used simple micro-USB connectors) there would be a completely
> > > new uABI. And any external port / wrapper will also require a
> > > completely new symlink kind.
> > > 
> > > I understand your concerns regarding mentioning external device in the
> > > PATH property. However I think we should make it easier for the
> > > userspace app to determine the kind of the external connector. What
> > > would you think about extending the PATH property in the following
> > > way:
> > > 
> > > For the USB-C connectors the PATH property has the value of
> > > `typec:cardN-DP-m` value. Userspace app can then look for the
> > > typec_connector symlink at the /sys/class/drm/cardN-DP-m subdir to
> > > find the information about the corresponding USB-C port.
> > 
> > This doesn't make sense to me. "cardN-DP-m" has nothing to do with the
> > physical path of the connector. All of the parts of this string are
> > exposed elsewhere in the KMS uAPI already.
> 
> True. It seems I mixed KMS and xrandr clients in my head.
> Just 'typec:' then? This way userspace will still know that it is a
> USB-C connector (and can stop there) and if it needs more information
> (e.g. physical location) it can further look for the symlink in the
> sysfs.

It sounds like an abuse of the PATH property. PATH is supposed to
contain an actual path, not just some connector type.

User-space can directly look into sysfs if it wants to figure out
whether a connector is typec.

Re: [RFC PATCH v1 12/12] usb: typec: qcom: define the bridge's path

[Dmitry Baryshkov]

On Mon, 30 Oct 2023 at 12:27, Simon Ser <contact@emersion.fr> wrote:
>
> On Monday, October 30th, 2023 at 11:22, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>
> > On Mon, 30 Oct 2023 at 12:13, Simon Ser contact@emersion.fr wrote:
> >
> > > On Monday, October 30th, 2023 at 10:47, Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
> > >
> > > > On Mon, 30 Oct 2023 at 10:19, Heikki Krogerus
> > > > heikki.krogerus@linux.intel.com wrote:
> > > >
> > > > > On Mon, Oct 23, 2023 at 09:24:33PM +0300, Dmitry Baryshkov wrote:
> > > > >
> > > > > > On 15 September 2023 15:14:35 EEST, Heikki Krogerus heikki.krogerus@linux.intel.com wrote:
> > > > > >
> > > > > > > Hi Dmitry,
> > > > > > >
> > > > > > > On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote:
> > > > > > >
> > > > > > > > In order to notify the userspace about the DRM connector's USB-C port,
> > > > > > > > export the corresponding port's name as the bridge's path field.
> > > > > > > >
> > > > > > > > Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
> > > > > > > > ---
> > > > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 11 +++++++----
> > > > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c | 4 +++-
> > > > > > > > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h | 6 ++++--
> > > > > > > > 3 files changed, 14 insertions(+), 7 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > > > index b9d4856101c7..452dc6437861 100644
> > > > > > > > --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > > > +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> > > > > > > > @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > > > struct device_node *np = dev->of_node;
> > > > > > > > const struct pmic_typec_resources *res;
> > > > > > > > struct regmap *regmap;
> > > > > > > > + char *tcpm_name;
> > > > > > > > u32 base[2];
> > > > > > > > int ret;
> > > > > > > >
> > > > > > > > @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > > > mutex_init(&tcpm->lock);
> > > > > > > > platform_set_drvdata(pdev, tcpm);
> > > > > > > >
> > > > > > > > - tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev);
> > > > > > > > - if (IS_ERR(tcpm->pmic_typec_drm))
> > > > > > > > - return PTR_ERR(tcpm->pmic_typec_drm);
> > > > > > > > -
> > > > > > > > tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
> > > > > > > > if (!tcpm->tcpc.fwnode)
> > > > > > > > return -EINVAL;
> > > > > > > > @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
> > > > > > > > goto fwnode_remove;
> > > > > > > > }
> > > > > > > >
> > > > > > > > + tcpm_name = tcpm_port_get_name(tcpm->tcpm_port);
> > > > > > > > + tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name);
> > > > > > >
> > > > > > > So I got some questions and concerns off-list. This was one of the
> > > > > > > concerns. That tcpm_name is now the actual port device name, so I'm
> > > > > > > afraid this is not acceptable.
> > > > > > >
> > > > > > > You can't use device name as a reference, ever. There is no way to
> > > > > > > guarantee that a device with a specific name is what you meant it to
> > > > > > > be by the time it's accessed.
> > > > > >
> > > > > > Hmm, could you please be more specific, why? I mean, class devices are not
> > > > > > that easy to be renamed in sysfs, are they? Or are you concerned about the
> > > > > > device being destroyed behind userspace's back? At least for MSM this will be
> > > > > > a huge problem already, with the bridge driver suddenly being removed.
> > > > >
> > > > > The race exists even in your case, but please do not look at this as a
> > > > > solution for only your platform.
> > > >
> > > > Yes!
> > > >
> > > > > This is about showing the user space a link between two device
> > > > > instances (struct device), and the way you do that is by creating a
> > > > > symlink. That way the kernel can take care of reference counting and
> > > > > guarantee that the link always points to the correct device. That way
> > > > > the link will also be always visible in user space without requirement
> > > > > for any specific ABI like it should.
> > > >
> > > > I'm fine with the symlink approach (and I'll follow that up after
> > > > finishing the UCSI glue driver rework). However I feel several
> > > > deficiencies there:
> > > >
> > > > 1) It creates asymmetry with the DP MST case. Do we want to have
> > > > symlinks in each of the MST connectors? Or do we follow the PATH
> > > > properties in the MST case until we find the root port, which has
> > > > symlink? Please note, that fine X11 renames DP MST connectors
> > > > internally, so in xrandr I see DP-2-1, which maps to
> > > > /sys/class/drm/card0-DP-2. Kind of hard to follow.
> > > >
> > > > 2) For the multi-card cases, one has to remap the connector to the
> > > > card index + connector path. And this needs to be done by all user
> > > > space applications, which would like to present this kind of
> > > > information for the user.
> > > >
> > > > 3) If we were to support non-USB-C connectors (e.g. MyDP / SlimPort
> > > > and MHL used simple micro-USB connectors) there would be a completely
> > > > new uABI. And any external port / wrapper will also require a
> > > > completely new symlink kind.
> > > >
> > > > I understand your concerns regarding mentioning external device in the
> > > > PATH property. However I think we should make it easier for the
> > > > userspace app to determine the kind of the external connector. What
> > > > would you think about extending the PATH property in the following
> > > > way:
> > > >
> > > > For the USB-C connectors the PATH property has the value of
> > > > `typec:cardN-DP-m` value. Userspace app can then look for the
> > > > typec_connector symlink at the /sys/class/drm/cardN-DP-m subdir to
> > > > find the information about the corresponding USB-C port.
> > >
> > > This doesn't make sense to me. "cardN-DP-m" has nothing to do with the
> > > physical path of the connector. All of the parts of this string are
> > > exposed elsewhere in the KMS uAPI already.
> >
> > True. It seems I mixed KMS and xrandr clients in my head.
> > Just 'typec:' then? This way userspace will still know that it is a
> > USB-C connector (and can stop there) and if it needs more information
> > (e.g. physical location) it can further look for the symlink in the
> > sysfs.
>
> It sounds like an abuse of the PATH property. PATH is supposed to
> contain an actual path, not just some connector type.
>
> User-space can directly look into sysfs if it wants to figure out
> whether a connector is typec.

typec:N, where N is an id of the port?

-- 
With best wishes
Dmitry