[PATCH 0/4] drm/bridge-connector: simplify handling of USB-C DP

[PATCH 0/4] drm/bridge-connector: simplify handling of USB-C DP

[Dmitry Baryshkov]

During the discussion of DP connetors, it was pointed out that existing
DP drivers supporting USB-C altmode (AMDGPU, Intel) use
DRM_MODE_CONNECTOR_DisplayPort for such connectors rather than
DRM_MODE_CONNECTOR_USB.

This patchset attempts to solve this issue. It adds USB to the enum
drm_dp_subconnector and then provides a simple yet efficient way to
define DP-in-USB subconnector type for the drivers that use
drm_bridge_connector.

Dmitry Baryshkov (4):
  drm: allow specifying default subtype for the DP subconnector property
  drm/bridge-connector: handle subconnector types
  drm/uapi: document the USB subconnector type
  soc: qcom: pmic_glink: properly describe the DP connector

 .../gpu/drm/amd/amdgpu/amdgpu_connectors.c    |  3 +-
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  3 +-
 drivers/gpu/drm/drm_bridge_connector.c        | 33 ++++++++++++++++++-
 drivers/gpu/drm/drm_connector.c               |  7 ++--
 drivers/gpu/drm/i915/display/intel_dp.c       |  3 +-
 drivers/soc/qcom/pmic_glink_altmode.c         |  3 +-
 include/drm/drm_bridge.h                      |  4 +++
 include/drm/drm_connector.h                   |  3 +-
 include/uapi/drm/drm_mode.h                   |  1 +
 9 files changed, 52 insertions(+), 8 deletions(-)

-- 
2.39.2

[PATCH 1/4] drm: allow specifying default subtype for the DP subconnector property

[Dmitry Baryshkov]

In the embedded usecases the default subtype depends on the bridge
chain, so it is easier to specify the subtype at the proprety attachment
type rather than specifying it later.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c              | 3 ++-
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 3 ++-
 drivers/gpu/drm/drm_connector.c                             | 6 ++++--
 drivers/gpu/drm/i915/display/intel_dp.c                     | 3 ++-
 include/drm/drm_connector.h                                 | 3 ++-
 5 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index d34037b85cf8..c18459ecd4be 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -2022,7 +2022,8 @@ amdgpu_connector_add(struct amdgpu_device *adev,
 
 	if (connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
 	    connector_type == DRM_MODE_CONNECTOR_eDP) {
-		drm_connector_attach_dp_subconnector_property(&amdgpu_connector->base);
+		drm_connector_attach_dp_subconnector_property(&amdgpu_connector->base,
+							      DRM_MODE_SUBCONNECTOR_Unknown);
 	}
 
 	return;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 943959012d04..297321f0199e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -759,7 +759,8 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
 	drm_dp_mst_topology_mgr_init(&aconnector->mst_mgr, adev_to_drm(dm->adev),
 				     &aconnector->dm_dp_aux.aux, 16, 4, aconnector->connector_id);
 
-	drm_connector_attach_dp_subconnector_property(&aconnector->base);
+	drm_connector_attach_dp_subconnector_property(&aconnector->base,
+						      DRM_MODE_SUBCONNECTOR_Unknown);
 }
 
 int dm_mst_get_pbn_divider(struct dc_link *link)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index a3d3e7dc08b2..a6066e4a5e9a 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1577,10 +1577,12 @@ EXPORT_SYMBOL(drm_mode_create_dvi_i_properties);
 /**
  * drm_connector_attach_dp_subconnector_property - create subconnector property for DP
  * @connector: drm_connector to attach property
+ * @subtype: initial value for the subconnector type
  *
  * Called by a driver when DP connector is created.
  */
-void drm_connector_attach_dp_subconnector_property(struct drm_connector *connector)
+void drm_connector_attach_dp_subconnector_property(struct drm_connector *connector,
+						   enum drm_mode_subconnector subtype)
 {
 	struct drm_mode_config *mode_config = &connector->dev->mode_config;
 
@@ -1594,7 +1596,7 @@ void drm_connector_attach_dp_subconnector_property(struct drm_connector *connect
 
 	drm_object_attach_property(&connector->base,
 				   mode_config->dp_subconnector_property,
-				   DRM_MODE_SUBCONNECTOR_Unknown);
+				   subtype);
 }
 EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 474785110662..5819105187f6 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5391,7 +5391,8 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
 	enum port port = dp_to_dig_port(intel_dp)->base.port;
 
 	if (!intel_dp_is_edp(intel_dp))
-		drm_connector_attach_dp_subconnector_property(connector);
+		drm_connector_attach_dp_subconnector_property(connector,
+							      DRM_MODE_SUBCONNECTOR_Unknown);
 
 	if (!IS_G4X(dev_priv) && port != PORT_A)
 		intel_attach_force_audio_property(connector);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 5a8115dca359..a130a78f6e0f 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1990,7 +1990,8 @@ const char *drm_get_hdcp_content_type_name(int val);
 int drm_get_tv_mode_from_name(const char *name, size_t len);
 
 int drm_mode_create_dvi_i_properties(struct drm_device *dev);
-void drm_connector_attach_dp_subconnector_property(struct drm_connector *connector);
+void drm_connector_attach_dp_subconnector_property(struct drm_connector *connector,
+						   enum drm_mode_subconnector subtype);
 
 int drm_mode_create_tv_margin_properties(struct drm_device *dev);
 int drm_mode_create_tv_properties_legacy(struct drm_device *dev,
-- 
2.39.2

[PATCH 2/4] drm/bridge-connector: handle subconnector types

[Dmitry Baryshkov]

If the created connector type supports subconnector type property,
create and attach corresponding it. The default subtype value is 0,
which maps to the DRM_MODE_SUBCONNECTOR_Unknown type.

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

diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
index 07b5930b1282..a7b92f0d2430 100644
--- a/drivers/gpu/drm/drm_bridge_connector.c
+++ b/drivers/gpu/drm/drm_bridge_connector.c
@@ -329,7 +329,9 @@ 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;
+	enum drm_mode_subconnector subconnector;
 	int connector_type;
+	int ret;
 
 	bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL);
 	if (!bridge_connector)
@@ -365,8 +367,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 		if (bridge->ops & DRM_BRIDGE_OP_MODES)
 			bridge_connector->bridge_modes = bridge;
 
-		if (!drm_bridge_get_next_bridge(bridge))
+		if (!drm_bridge_get_next_bridge(bridge)) {
 			connector_type = bridge->type;
+			subconnector = bridge->subtype;
+		}
 
 #ifdef CONFIG_OF
 		if (!drm_bridge_get_next_bridge(bridge) &&
@@ -399,6 +403,33 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 	if (panel_bridge)
 		drm_panel_bridge_set_orientation(connector, panel_bridge);
 
+	if (connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
+		drm_connector_attach_dp_subconnector_property(connector, subconnector);
+	} else if (connector_type == DRM_MODE_CONNECTOR_DVII) {
+		ret = drm_mode_create_dvi_i_properties(drm);
+		if (ret)
+			return ERR_PTR(ret);
+
+		drm_object_attach_property(&connector->base,
+					   drm->mode_config.dvi_i_subconnector_property,
+					   subconnector);
+	} else if (connector_type == DRM_MODE_CONNECTOR_TV) {
+		ret = drm_mode_create_tv_properties(drm,
+						    BIT(DRM_MODE_TV_MODE_NTSC) |
+						    BIT(DRM_MODE_TV_MODE_NTSC_443) |
+						    BIT(DRM_MODE_TV_MODE_NTSC_J) |
+						    BIT(DRM_MODE_TV_MODE_PAL) |
+						    BIT(DRM_MODE_TV_MODE_PAL_M) |
+						    BIT(DRM_MODE_TV_MODE_PAL_N) |
+						    BIT(DRM_MODE_TV_MODE_SECAM));
+		if (ret)
+			return ERR_PTR(ret);
+
+		drm_object_attach_property(&connector->base,
+					   drm->mode_config.tv_subconnector_property,
+					   subconnector);
+	}
+
 	return connector;
 }
 EXPORT_SYMBOL_GPL(drm_bridge_connector_init);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index bf964cdfb330..68b14ac5ac0d 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -739,6 +739,10 @@ struct drm_bridge {
 	 * identifies the type of connected display.
 	 */
 	int type;
+	/**
+	 * @subtype: the subtype of the connector for the DP/TV/DVI-I cases.
+	 */
+	enum drm_mode_subconnector subtype;
 	/**
 	 * @interlace_allowed: Indicate that the bridge can handle interlaced
 	 * modes.
-- 
2.39.2

[PATCH 3/4] drm/uapi: document the USB subconnector type

[Dmitry Baryshkov]

To properly define the USB-C DP altmode connectors, add the USB
subconnector type.

Suggested-by: Simon Ser <contact@emersion.fr>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_connector.c | 1 +
 include/uapi/drm/drm_mode.h     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index a6066e4a5e9a..9e96b038f5d0 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1050,6 +1050,7 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
 	{ DRM_MODE_SUBCONNECTOR_DisplayPort, "DP"        }, /* DP */
 	{ DRM_MODE_SUBCONNECTOR_Wireless,    "Wireless"  }, /* DP */
 	{ DRM_MODE_SUBCONNECTOR_Native,	     "Native"    }, /* DP */
+	{ DRM_MODE_SUBCONNECTOR_USB,	     "USB"       }, /* DP */
 };
 
 DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 92d96a2b6763..0f74918b011c 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -398,6 +398,7 @@ enum drm_mode_subconnector {
 	DRM_MODE_SUBCONNECTOR_HDMIA       = 11, /*            DP */
 	DRM_MODE_SUBCONNECTOR_Native      = 15, /*            DP */
 	DRM_MODE_SUBCONNECTOR_Wireless    = 18, /*            DP */
+	DRM_MODE_SUBCONNECTOR_USB         = 20, /*            DP */
 };
 
 #define DRM_MODE_CONNECTOR_Unknown	0
-- 
2.39.2

[PATCH 4/4] soc: qcom: pmic_glink: properly describe the DP connector

[Dmitry Baryshkov]

During the discussion of the DP connectors, it was suggested that USB-C
DisplayPort connectors should have DRM_MODE_CONNECTOR_DisplayPort
connector type. This follows the example provided by other drivers
(AMDGPU, Intel). To distinguish them from native DP ports, they should
have the freshly defined USB as a subconnector type.

Suggested-by: Simon Ser <contact@emersion.fr>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/soc/qcom/pmic_glink_altmode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c
index 1dedacc52aea..9094944c6cc0 100644
--- a/drivers/soc/qcom/pmic_glink_altmode.c
+++ b/drivers/soc/qcom/pmic_glink_altmode.c
@@ -417,7 +417,8 @@ 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;
+		alt_port->bridge.subtype = DRM_MODE_SUBCONNECTOR_USB;
 
 		ret = devm_drm_bridge_add(dev, &alt_port->bridge);
 		if (ret)
-- 
2.39.2

Re: [PATCH 2/4] drm/bridge-connector: handle subconnector types

[Neil Armstrong]

On 29/07/2023 02:49, Dmitry Baryshkov wrote:
> If the created connector type supports subconnector type property,
> create and attach corresponding it. The default subtype value is 0,
> which maps to the DRM_MODE_SUBCONNECTOR_Unknown type.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/drm_bridge_connector.c | 33 +++++++++++++++++++++++++-
>   include/drm/drm_bridge.h               |  4 ++++
>   2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> index 07b5930b1282..a7b92f0d2430 100644
> --- a/drivers/gpu/drm/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/drm_bridge_connector.c
> @@ -329,7 +329,9 @@ 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;
> +	enum drm_mode_subconnector subconnector;
>   	int connector_type;
> +	int ret;
>   
>   	bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL);
>   	if (!bridge_connector)
> @@ -365,8 +367,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>   		if (bridge->ops & DRM_BRIDGE_OP_MODES)
>   			bridge_connector->bridge_modes = bridge;
>   
> -		if (!drm_bridge_get_next_bridge(bridge))
> +		if (!drm_bridge_get_next_bridge(bridge)) {
>   			connector_type = bridge->type;
> +			subconnector = bridge->subtype;
> +		}
>   
>   #ifdef CONFIG_OF
>   		if (!drm_bridge_get_next_bridge(bridge) &&
> @@ -399,6 +403,33 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>   	if (panel_bridge)
>   		drm_panel_bridge_set_orientation(connector, panel_bridge);
>   
> +	if (connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
> +		drm_connector_attach_dp_subconnector_property(connector, subconnector);
> +	} else if (connector_type == DRM_MODE_CONNECTOR_DVII) {
> +		ret = drm_mode_create_dvi_i_properties(drm);
> +		if (ret)
> +			return ERR_PTR(ret);
> +
> +		drm_object_attach_property(&connector->base,
> +					   drm->mode_config.dvi_i_subconnector_property,
> +					   subconnector);
> +	} else if (connector_type == DRM_MODE_CONNECTOR_TV) {
> +		ret = drm_mode_create_tv_properties(drm,
> +						    BIT(DRM_MODE_TV_MODE_NTSC) |
> +						    BIT(DRM_MODE_TV_MODE_NTSC_443) |
> +						    BIT(DRM_MODE_TV_MODE_NTSC_J) |
> +						    BIT(DRM_MODE_TV_MODE_PAL) |
> +						    BIT(DRM_MODE_TV_MODE_PAL_M) |
> +						    BIT(DRM_MODE_TV_MODE_PAL_N) |
> +						    BIT(DRM_MODE_TV_MODE_SECAM));
> +		if (ret)
> +			return ERR_PTR(ret);

I don't think this is right, this should be called from the appropriate encoder
device depending on the analog tv mode capabilities.


> +
> +		drm_object_attach_property(&connector->base,
> +					   drm->mode_config.tv_subconnector_property,
> +					   subconnector);

Here, only add the property if drm->mode_config.tv_subconnector_property exists,
and perhaps add a warning if not.

AFAIK same for DRM_MODE_CONNECTOR_DVII.

> +	}
> +
>   	return connector;
>   }
>   EXPORT_SYMBOL_GPL(drm_bridge_connector_init);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index bf964cdfb330..68b14ac5ac0d 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -739,6 +739,10 @@ struct drm_bridge {
>   	 * identifies the type of connected display.
>   	 */
>   	int type;
> +	/**
> +	 * @subtype: the subtype of the connector for the DP/TV/DVI-I cases.
> +	 */
> +	enum drm_mode_subconnector subtype;
>   	/**
>   	 * @interlace_allowed: Indicate that the bridge can handle interlaced
>   	 * modes.

Re: [PATCH 2/4] drm/bridge-connector: handle subconnector types

[Dmitry Baryshkov]

On Wed, 2 Aug 2023 at 11:35, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> On 29/07/2023 02:49, Dmitry Baryshkov wrote:
> > If the created connector type supports subconnector type property,
> > create and attach corresponding it. The default subtype value is 0,
> > which maps to the DRM_MODE_SUBCONNECTOR_Unknown type.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/drm_bridge_connector.c | 33 +++++++++++++++++++++++++-
> >   include/drm/drm_bridge.h               |  4 ++++
> >   2 files changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> > index 07b5930b1282..a7b92f0d2430 100644
> > --- a/drivers/gpu/drm/drm_bridge_connector.c
> > +++ b/drivers/gpu/drm/drm_bridge_connector.c
> > @@ -329,7 +329,9 @@ 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;
> > +     enum drm_mode_subconnector subconnector;
> >       int connector_type;
> > +     int ret;
> >
> >       bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL);
> >       if (!bridge_connector)
> > @@ -365,8 +367,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> >               if (bridge->ops & DRM_BRIDGE_OP_MODES)
> >                       bridge_connector->bridge_modes = bridge;
> >
> > -             if (!drm_bridge_get_next_bridge(bridge))
> > +             if (!drm_bridge_get_next_bridge(bridge)) {
> >                       connector_type = bridge->type;
> > +                     subconnector = bridge->subtype;
> > +             }
> >
> >   #ifdef CONFIG_OF
> >               if (!drm_bridge_get_next_bridge(bridge) &&
> > @@ -399,6 +403,33 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> >       if (panel_bridge)
> >               drm_panel_bridge_set_orientation(connector, panel_bridge);
> >
> > +     if (connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
> > +             drm_connector_attach_dp_subconnector_property(connector, subconnector);
> > +     } else if (connector_type == DRM_MODE_CONNECTOR_DVII) {
> > +             ret = drm_mode_create_dvi_i_properties(drm);
> > +             if (ret)
> > +                     return ERR_PTR(ret);
> > +
> > +             drm_object_attach_property(&connector->base,
> > +                                        drm->mode_config.dvi_i_subconnector_property,
> > +                                        subconnector);
> > +     } else if (connector_type == DRM_MODE_CONNECTOR_TV) {
> > +             ret = drm_mode_create_tv_properties(drm,
> > +                                                 BIT(DRM_MODE_TV_MODE_NTSC) |
> > +                                                 BIT(DRM_MODE_TV_MODE_NTSC_443) |
> > +                                                 BIT(DRM_MODE_TV_MODE_NTSC_J) |
> > +                                                 BIT(DRM_MODE_TV_MODE_PAL) |
> > +                                                 BIT(DRM_MODE_TV_MODE_PAL_M) |
> > +                                                 BIT(DRM_MODE_TV_MODE_PAL_N) |
> > +                                                 BIT(DRM_MODE_TV_MODE_SECAM));
> > +             if (ret)
> > +                     return ERR_PTR(ret);
>
> I don't think this is right, this should be called from the appropriate encoder
> device depending on the analog tv mode capabilities.

Good question. My logic was the following: the DRM device can have
different TV out ports with different capabilities (yeah, pure
theoretical construct). In this case it might be impossible to create
a single subset of values. Thus it is more correct to create the
property listing all possible values. The property is immutable anyway
(and so the user doesn't have control over the value).


> > +
> > +             drm_object_attach_property(&connector->base,
> > +                                        drm->mode_config.tv_subconnector_property,
> > +                                        subconnector);
>
> Here, only add the property if drm->mode_config.tv_subconnector_property exists,
> and perhaps add a warning if not.

This property is created in the previous call,
drm_mode_create_tv_properties() ->
drm_mode_create_tv_properties_legacy().

>
> AFAIK same for DRM_MODE_CONNECTOR_DVII.
>
> > +     }
> > +
> >       return connector;
> >   }
> >   EXPORT_SYMBOL_GPL(drm_bridge_connector_init);
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index bf964cdfb330..68b14ac5ac0d 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -739,6 +739,10 @@ struct drm_bridge {
> >        * identifies the type of connected display.
> >        */
> >       int type;
> > +     /**
> > +      * @subtype: the subtype of the connector for the DP/TV/DVI-I cases.
> > +      */
> > +     enum drm_mode_subconnector subtype;
> >       /**
> >        * @interlace_allowed: Indicate that the bridge can handle interlaced
> >        * modes.
>


-- 
With best wishes
Dmitry

Re: [PATCH 2/4] drm/bridge-connector: handle subconnector types

[Laurent Pinchart]

On Wed, Aug 02, 2023 at 12:05:50PM +0300, Dmitry Baryshkov wrote:
> On Wed, 2 Aug 2023 at 11:35, Neil Armstrong wrote:
> > On 29/07/2023 02:49, Dmitry Baryshkov wrote:
> > > If the created connector type supports subconnector type property,
> > > create and attach corresponding it. The default subtype value is 0,
> > > which maps to the DRM_MODE_SUBCONNECTOR_Unknown type.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >   drivers/gpu/drm/drm_bridge_connector.c | 33 +++++++++++++++++++++++++-
> > >   include/drm/drm_bridge.h               |  4 ++++
> > >   2 files changed, 36 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> > > index 07b5930b1282..a7b92f0d2430 100644
> > > --- a/drivers/gpu/drm/drm_bridge_connector.c
> > > +++ b/drivers/gpu/drm/drm_bridge_connector.c
> > > @@ -329,7 +329,9 @@ 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;
> > > +     enum drm_mode_subconnector subconnector;
> > >       int connector_type;
> > > +     int ret;
> > >
> > >       bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL);
> > >       if (!bridge_connector)
> > > @@ -365,8 +367,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > >               if (bridge->ops & DRM_BRIDGE_OP_MODES)
> > >                       bridge_connector->bridge_modes = bridge;
> > >
> > > -             if (!drm_bridge_get_next_bridge(bridge))
> > > +             if (!drm_bridge_get_next_bridge(bridge)) {
> > >                       connector_type = bridge->type;
> > > +                     subconnector = bridge->subtype;
> > > +             }
> > >
> > >   #ifdef CONFIG_OF
> > >               if (!drm_bridge_get_next_bridge(bridge) &&
> > > @@ -399,6 +403,33 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > >       if (panel_bridge)
> > >               drm_panel_bridge_set_orientation(connector, panel_bridge);
> > >
> > > +     if (connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
> > > +             drm_connector_attach_dp_subconnector_property(connector, subconnector);
> > > +     } else if (connector_type == DRM_MODE_CONNECTOR_DVII) {
> > > +             ret = drm_mode_create_dvi_i_properties(drm);
> > > +             if (ret)
> > > +                     return ERR_PTR(ret);
> > > +
> > > +             drm_object_attach_property(&connector->base,
> > > +                                        drm->mode_config.dvi_i_subconnector_property,
> > > +                                        subconnector);
> > > +     } else if (connector_type == DRM_MODE_CONNECTOR_TV) {
> > > +             ret = drm_mode_create_tv_properties(drm,
> > > +                                                 BIT(DRM_MODE_TV_MODE_NTSC) |
> > > +                                                 BIT(DRM_MODE_TV_MODE_NTSC_443) |
> > > +                                                 BIT(DRM_MODE_TV_MODE_NTSC_J) |
> > > +                                                 BIT(DRM_MODE_TV_MODE_PAL) |
> > > +                                                 BIT(DRM_MODE_TV_MODE_PAL_M) |
> > > +                                                 BIT(DRM_MODE_TV_MODE_PAL_N) |
> > > +                                                 BIT(DRM_MODE_TV_MODE_SECAM));
> > > +             if (ret)
> > > +                     return ERR_PTR(ret);
> >
> > I don't think this is right, this should be called from the appropriate encoder
> > device depending on the analog tv mode capabilities.
> 
> Good question. My logic was the following: the DRM device can have
> different TV out ports with different capabilities (yeah, pure
> theoretical construct). In this case it might be impossible to create
> a single subset of values. Thus it is more correct to create the
> property listing all possible values. The property is immutable anyway
> (and so the user doesn't have control over the value).

Those ports would correspond to different connectors, so I agree with
Neil, I don't think it's right to create a single property with all
modes and attach it to all analog output connectors.

If you want to support multiple analog outputs that have different
capabilities, this will need changes to drm_mode_create_tv_properties()
to allow creating multiple properties. If you don't want to do so now,
and prefer limiting support to devices where all ports support the same
modes (which includes devices with a single analog output), then the
modes should reflect what the device supports.

> > > +
> > > +             drm_object_attach_property(&connector->base,
> > > +                                        drm->mode_config.tv_subconnector_property,
> > > +                                        subconnector);
> >
> > Here, only add the property if drm->mode_config.tv_subconnector_property exists,
> > and perhaps add a warning if not.
> 
> This property is created in the previous call,
> drm_mode_create_tv_properties() ->
> drm_mode_create_tv_properties_legacy().
> 
> > AFAIK same for DRM_MODE_CONNECTOR_DVII.
> >
> > > +     }
> > > +
> > >       return connector;
> > >   }
> > >   EXPORT_SYMBOL_GPL(drm_bridge_connector_init);
> > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > > index bf964cdfb330..68b14ac5ac0d 100644
> > > --- a/include/drm/drm_bridge.h
> > > +++ b/include/drm/drm_bridge.h
> > > @@ -739,6 +739,10 @@ struct drm_bridge {
> > >        * identifies the type of connected display.
> > >        */
> > >       int type;
> > > +     /**
> > > +      * @subtype: the subtype of the connector for the DP/TV/DVI-I cases.
> > > +      */
> > > +     enum drm_mode_subconnector subtype;
> > >       /**
> > >        * @interlace_allowed: Indicate that the bridge can handle interlaced
> > >        * modes.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] drm/bridge-connector: handle subconnector types

[Dmitry Baryshkov]

On 02/08/2023 21:46, Laurent Pinchart wrote:
> On Wed, Aug 02, 2023 at 12:05:50PM +0300, Dmitry Baryshkov wrote:
>> On Wed, 2 Aug 2023 at 11:35, Neil Armstrong wrote:
>>> On 29/07/2023 02:49, Dmitry Baryshkov wrote:
>>>> If the created connector type supports subconnector type property,
>>>> create and attach corresponding it. The default subtype value is 0,
>>>> which maps to the DRM_MODE_SUBCONNECTOR_Unknown type.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>    drivers/gpu/drm/drm_bridge_connector.c | 33 +++++++++++++++++++++++++-
>>>>    include/drm/drm_bridge.h               |  4 ++++
>>>>    2 files changed, 36 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
>>>> index 07b5930b1282..a7b92f0d2430 100644
>>>> --- a/drivers/gpu/drm/drm_bridge_connector.c
>>>> +++ b/drivers/gpu/drm/drm_bridge_connector.c
>>>> @@ -329,7 +329,9 @@ 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;
>>>> +     enum drm_mode_subconnector subconnector;
>>>>        int connector_type;
>>>> +     int ret;
>>>>
>>>>        bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL);
>>>>        if (!bridge_connector)
>>>> @@ -365,8 +367,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>>>>                if (bridge->ops & DRM_BRIDGE_OP_MODES)
>>>>                        bridge_connector->bridge_modes = bridge;
>>>>
>>>> -             if (!drm_bridge_get_next_bridge(bridge))
>>>> +             if (!drm_bridge_get_next_bridge(bridge)) {
>>>>                        connector_type = bridge->type;
>>>> +                     subconnector = bridge->subtype;
>>>> +             }
>>>>
>>>>    #ifdef CONFIG_OF
>>>>                if (!drm_bridge_get_next_bridge(bridge) &&
>>>> @@ -399,6 +403,33 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>>>>        if (panel_bridge)
>>>>                drm_panel_bridge_set_orientation(connector, panel_bridge);
>>>>
>>>> +     if (connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
>>>> +             drm_connector_attach_dp_subconnector_property(connector, subconnector);
>>>> +     } else if (connector_type == DRM_MODE_CONNECTOR_DVII) {
>>>> +             ret = drm_mode_create_dvi_i_properties(drm);
>>>> +             if (ret)
>>>> +                     return ERR_PTR(ret);
>>>> +
>>>> +             drm_object_attach_property(&connector->base,
>>>> +                                        drm->mode_config.dvi_i_subconnector_property,
>>>> +                                        subconnector);
>>>> +     } else if (connector_type == DRM_MODE_CONNECTOR_TV) {
>>>> +             ret = drm_mode_create_tv_properties(drm,
>>>> +                                                 BIT(DRM_MODE_TV_MODE_NTSC) |
>>>> +                                                 BIT(DRM_MODE_TV_MODE_NTSC_443) |
>>>> +                                                 BIT(DRM_MODE_TV_MODE_NTSC_J) |
>>>> +                                                 BIT(DRM_MODE_TV_MODE_PAL) |
>>>> +                                                 BIT(DRM_MODE_TV_MODE_PAL_M) |
>>>> +                                                 BIT(DRM_MODE_TV_MODE_PAL_N) |
>>>> +                                                 BIT(DRM_MODE_TV_MODE_SECAM));
>>>> +             if (ret)
>>>> +                     return ERR_PTR(ret);
>>>
>>> I don't think this is right, this should be called from the appropriate encoder
>>> device depending on the analog tv mode capabilities.
>>
>> Good question. My logic was the following: the DRM device can have
>> different TV out ports with different capabilities (yeah, pure
>> theoretical construct). In this case it might be impossible to create
>> a single subset of values. Thus it is more correct to create the
>> property listing all possible values. The property is immutable anyway
>> (and so the user doesn't have control over the value).
> 
> Those ports would correspond to different connectors, so I agree with
> Neil, I don't think it's right to create a single property with all
> modes and attach it to all analog output connectors.

I agree that this case is mishandled currently (as current design 
assumes a single property that fits for the complete device).

> 
> If you want to support multiple analog outputs that have different
> capabilities, this will need changes to drm_mode_create_tv_properties()
> to allow creating multiple properties. If you don't want to do so now,
> and prefer limiting support to devices where all ports support the same
> modes (which includes devices with a single analog output), then the
> modes should reflect what the device supports.

Ack, I'll drop the create call and check for the property existence 
before attaching it.

> 
>>>> +
>>>> +             drm_object_attach_property(&connector->base,
>>>> +                                        drm->mode_config.tv_subconnector_property,
>>>> +                                        subconnector);
>>>
>>> Here, only add the property if drm->mode_config.tv_subconnector_property exists,
>>> and perhaps add a warning if not.
>>
>> This property is created in the previous call,
>> drm_mode_create_tv_properties() ->
>> drm_mode_create_tv_properties_legacy().
>>
>>> AFAIK same for DRM_MODE_CONNECTOR_DVII.
>>>
>>>> +     }
>>>> +
>>>>        return connector;
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(drm_bridge_connector_init);
>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>>> index bf964cdfb330..68b14ac5ac0d 100644
>>>> --- a/include/drm/drm_bridge.h
>>>> +++ b/include/drm/drm_bridge.h
>>>> @@ -739,6 +739,10 @@ struct drm_bridge {
>>>>         * identifies the type of connected display.
>>>>         */
>>>>        int type;
>>>> +     /**
>>>> +      * @subtype: the subtype of the connector for the DP/TV/DVI-I cases.
>>>> +      */
>>>> +     enum drm_mode_subconnector subtype;
>>>>        /**
>>>>         * @interlace_allowed: Indicate that the bridge can handle interlaced
>>>>         * modes.
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/4] drm: allow specifying default subtype for the DP subconnector property

[Laurent Pinchart]

Hi Dmitry,

Thank you for the patch.

On Sat, Jul 29, 2023 at 03:49:10AM +0300, Dmitry Baryshkov wrote:
> In the embedded usecases the default subtype depends on the bridge
> chain, so it is easier to specify the subtype at the proprety attachment

s/proprety/property/

> type rather than specifying it later.

Did you mean s/type/time/ ?

I think I understand why you need this, looking at patch 2/4, but the
commit message isn't very clear. It would benefit from being reworded.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c              | 3 ++-
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 3 ++-
>  drivers/gpu/drm/drm_connector.c                             | 6 ++++--
>  drivers/gpu/drm/i915/display/intel_dp.c                     | 3 ++-
>  include/drm/drm_connector.h                                 | 3 ++-
>  5 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> index d34037b85cf8..c18459ecd4be 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> @@ -2022,7 +2022,8 @@ amdgpu_connector_add(struct amdgpu_device *adev,
>  
>  	if (connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
>  	    connector_type == DRM_MODE_CONNECTOR_eDP) {
> -		drm_connector_attach_dp_subconnector_property(&amdgpu_connector->base);
> +		drm_connector_attach_dp_subconnector_property(&amdgpu_connector->base,
> +							      DRM_MODE_SUBCONNECTOR_Unknown);
>  	}
>  
>  	return;
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 943959012d04..297321f0199e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -759,7 +759,8 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
>  	drm_dp_mst_topology_mgr_init(&aconnector->mst_mgr, adev_to_drm(dm->adev),
>  				     &aconnector->dm_dp_aux.aux, 16, 4, aconnector->connector_id);
>  
> -	drm_connector_attach_dp_subconnector_property(&aconnector->base);
> +	drm_connector_attach_dp_subconnector_property(&aconnector->base,
> +						      DRM_MODE_SUBCONNECTOR_Unknown);
>  }
>  
>  int dm_mst_get_pbn_divider(struct dc_link *link)
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index a3d3e7dc08b2..a6066e4a5e9a 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1577,10 +1577,12 @@ EXPORT_SYMBOL(drm_mode_create_dvi_i_properties);
>  /**
>   * drm_connector_attach_dp_subconnector_property - create subconnector property for DP
>   * @connector: drm_connector to attach property
> + * @subtype: initial value for the subconnector type
>   *
>   * Called by a driver when DP connector is created.
>   */
> -void drm_connector_attach_dp_subconnector_property(struct drm_connector *connector)
> +void drm_connector_attach_dp_subconnector_property(struct drm_connector *connector,
> +						   enum drm_mode_subconnector subtype)
>  {
>  	struct drm_mode_config *mode_config = &connector->dev->mode_config;
>  
> @@ -1594,7 +1596,7 @@ void drm_connector_attach_dp_subconnector_property(struct drm_connector *connect
>  
>  	drm_object_attach_property(&connector->base,
>  				   mode_config->dp_subconnector_property,
> -				   DRM_MODE_SUBCONNECTOR_Unknown);
> +				   subtype);
>  }
>  EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 474785110662..5819105187f6 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5391,7 +5391,8 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
>  	enum port port = dp_to_dig_port(intel_dp)->base.port;
>  
>  	if (!intel_dp_is_edp(intel_dp))
> -		drm_connector_attach_dp_subconnector_property(connector);
> +		drm_connector_attach_dp_subconnector_property(connector,
> +							      DRM_MODE_SUBCONNECTOR_Unknown);
>  
>  	if (!IS_G4X(dev_priv) && port != PORT_A)
>  		intel_attach_force_audio_property(connector);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 5a8115dca359..a130a78f6e0f 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1990,7 +1990,8 @@ const char *drm_get_hdcp_content_type_name(int val);
>  int drm_get_tv_mode_from_name(const char *name, size_t len);
>  
>  int drm_mode_create_dvi_i_properties(struct drm_device *dev);
> -void drm_connector_attach_dp_subconnector_property(struct drm_connector *connector);
> +void drm_connector_attach_dp_subconnector_property(struct drm_connector *connector,
> +						   enum drm_mode_subconnector subtype);
>  
>  int drm_mode_create_tv_margin_properties(struct drm_device *dev);
>  int drm_mode_create_tv_properties_legacy(struct drm_device *dev,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] drm/uapi: document the USB subconnector type

[Laurent Pinchart]

Hi Dmitry,

Thank you for the patch.

On Sat, Jul 29, 2023 at 03:49:12AM +0300, Dmitry Baryshkov wrote:
> To properly define the USB-C DP altmode connectors, add the USB
> subconnector type.
> 
> Suggested-by: Simon Ser <contact@emersion.fr>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/drm_connector.c | 1 +
>  include/uapi/drm/drm_mode.h     | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index a6066e4a5e9a..9e96b038f5d0 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1050,6 +1050,7 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
>  	{ DRM_MODE_SUBCONNECTOR_DisplayPort, "DP"        }, /* DP */
>  	{ DRM_MODE_SUBCONNECTOR_Wireless,    "Wireless"  }, /* DP */
>  	{ DRM_MODE_SUBCONNECTOR_Native,	     "Native"    }, /* DP */
> +	{ DRM_MODE_SUBCONNECTOR_USB,	     "USB"       }, /* DP */

Should this be DRM_MODE_SUBCONNECTOR_USB_C and "USB-C", in case we get
another USB type later ?

>  };
>  
>  DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 92d96a2b6763..0f74918b011c 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -398,6 +398,7 @@ enum drm_mode_subconnector {
>  	DRM_MODE_SUBCONNECTOR_HDMIA       = 11, /*            DP */
>  	DRM_MODE_SUBCONNECTOR_Native      = 15, /*            DP */
>  	DRM_MODE_SUBCONNECTOR_Wireless    = 18, /*            DP */
> +	DRM_MODE_SUBCONNECTOR_USB         = 20, /*            DP */
>  };
>  
>  #define DRM_MODE_CONNECTOR_Unknown	0

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] drm/uapi: document the USB subconnector type

[Dmitry Baryshkov]

On 02/08/2023 21:55, Laurent Pinchart wrote:
> Hi Dmitry,
> 
> Thank you for the patch.
> 
> On Sat, Jul 29, 2023 at 03:49:12AM +0300, Dmitry Baryshkov wrote:
>> To properly define the USB-C DP altmode connectors, add the USB
>> subconnector type.
>>
>> Suggested-by: Simon Ser <contact@emersion.fr>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/drm_connector.c | 1 +
>>   include/uapi/drm/drm_mode.h     | 1 +
>>   2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index a6066e4a5e9a..9e96b038f5d0 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1050,6 +1050,7 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
>>   	{ DRM_MODE_SUBCONNECTOR_DisplayPort, "DP"        }, /* DP */
>>   	{ DRM_MODE_SUBCONNECTOR_Wireless,    "Wireless"  }, /* DP */
>>   	{ DRM_MODE_SUBCONNECTOR_Native,	     "Native"    }, /* DP */
>> +	{ DRM_MODE_SUBCONNECTOR_USB,	     "USB"       }, /* DP */
> 
> Should this be DRM_MODE_SUBCONNECTOR_USB_C and "USB-C", in case we get
> another USB type later ?

Hmm, which id should I use for micro-USB then? (consider anx7808, 
SlimPort). I thought about using DRM_MODE_SUBCONNECTOR_USB for both of 
them. But maybe I should add another subtype for SlimPort.

> 
>>   };
>>   
>>   DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index 92d96a2b6763..0f74918b011c 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -398,6 +398,7 @@ enum drm_mode_subconnector {
>>   	DRM_MODE_SUBCONNECTOR_HDMIA       = 11, /*            DP */
>>   	DRM_MODE_SUBCONNECTOR_Native      = 15, /*            DP */
>>   	DRM_MODE_SUBCONNECTOR_Wireless    = 18, /*            DP */
>> +	DRM_MODE_SUBCONNECTOR_USB         = 20, /*            DP */
>>   };
>>   
>>   #define DRM_MODE_CONNECTOR_Unknown	0
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/4] drm: allow specifying default subtype for the DP subconnector property

[Dmitry Baryshkov]

On 02/08/2023 21:54, Laurent Pinchart wrote:
> Hi Dmitry,
> 
> Thank you for the patch.
> 
> On Sat, Jul 29, 2023 at 03:49:10AM +0300, Dmitry Baryshkov wrote:
>> In the embedded usecases the default subtype depends on the bridge
>> chain, so it is easier to specify the subtype at the proprety attachment
> 
> s/proprety/property/
> 
>> type rather than specifying it later.
> 
> Did you mean s/type/time/ ?
> 
> I think I understand why you need this, looking at patch 2/4, but the
> commit message isn't very clear. It would benefit from being reworded.

Ack, thanks for the feedback.

> 
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c              | 3 ++-
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 3 ++-
>>   drivers/gpu/drm/drm_connector.c                             | 6 ++++--
>>   drivers/gpu/drm/i915/display/intel_dp.c                     | 3 ++-
>>   include/drm/drm_connector.h                                 | 3 ++-
>>   5 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>> index d34037b85cf8..c18459ecd4be 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>> @@ -2022,7 +2022,8 @@ amdgpu_connector_add(struct amdgpu_device *adev,
>>   
>>   	if (connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
>>   	    connector_type == DRM_MODE_CONNECTOR_eDP) {
>> -		drm_connector_attach_dp_subconnector_property(&amdgpu_connector->base);
>> +		drm_connector_attach_dp_subconnector_property(&amdgpu_connector->base,
>> +							      DRM_MODE_SUBCONNECTOR_Unknown);
>>   	}
>>   
>>   	return;
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> index 943959012d04..297321f0199e 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> @@ -759,7 +759,8 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
>>   	drm_dp_mst_topology_mgr_init(&aconnector->mst_mgr, adev_to_drm(dm->adev),
>>   				     &aconnector->dm_dp_aux.aux, 16, 4, aconnector->connector_id);
>>   
>> -	drm_connector_attach_dp_subconnector_property(&aconnector->base);
>> +	drm_connector_attach_dp_subconnector_property(&aconnector->base,
>> +						      DRM_MODE_SUBCONNECTOR_Unknown);
>>   }
>>   
>>   int dm_mst_get_pbn_divider(struct dc_link *link)
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index a3d3e7dc08b2..a6066e4a5e9a 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1577,10 +1577,12 @@ EXPORT_SYMBOL(drm_mode_create_dvi_i_properties);
>>   /**
>>    * drm_connector_attach_dp_subconnector_property - create subconnector property for DP
>>    * @connector: drm_connector to attach property
>> + * @subtype: initial value for the subconnector type
>>    *
>>    * Called by a driver when DP connector is created.
>>    */
>> -void drm_connector_attach_dp_subconnector_property(struct drm_connector *connector)
>> +void drm_connector_attach_dp_subconnector_property(struct drm_connector *connector,
>> +						   enum drm_mode_subconnector subtype)
>>   {
>>   	struct drm_mode_config *mode_config = &connector->dev->mode_config;
>>   
>> @@ -1594,7 +1596,7 @@ void drm_connector_attach_dp_subconnector_property(struct drm_connector *connect
>>   
>>   	drm_object_attach_property(&connector->base,
>>   				   mode_config->dp_subconnector_property,
>> -				   DRM_MODE_SUBCONNECTOR_Unknown);
>> +				   subtype);
>>   }
>>   EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property);
>>   
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 474785110662..5819105187f6 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -5391,7 +5391,8 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
>>   	enum port port = dp_to_dig_port(intel_dp)->base.port;
>>   
>>   	if (!intel_dp_is_edp(intel_dp))
>> -		drm_connector_attach_dp_subconnector_property(connector);
>> +		drm_connector_attach_dp_subconnector_property(connector,
>> +							      DRM_MODE_SUBCONNECTOR_Unknown);
>>   
>>   	if (!IS_G4X(dev_priv) && port != PORT_A)
>>   		intel_attach_force_audio_property(connector);
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 5a8115dca359..a130a78f6e0f 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -1990,7 +1990,8 @@ const char *drm_get_hdcp_content_type_name(int val);
>>   int drm_get_tv_mode_from_name(const char *name, size_t len);
>>   
>>   int drm_mode_create_dvi_i_properties(struct drm_device *dev);
>> -void drm_connector_attach_dp_subconnector_property(struct drm_connector *connector);
>> +void drm_connector_attach_dp_subconnector_property(struct drm_connector *connector,
>> +						   enum drm_mode_subconnector subtype);
>>   
>>   int drm_mode_create_tv_margin_properties(struct drm_device *dev);
>>   int drm_mode_create_tv_properties_legacy(struct drm_device *dev,
> 

-- 
With best wishes
Dmitry

Re: [PATCH 3/4] drm/uapi: document the USB subconnector type

[Laurent Pinchart]

On Wed, Aug 02, 2023 at 10:01:19PM +0300, Dmitry Baryshkov wrote:
> On 02/08/2023 21:55, Laurent Pinchart wrote:
> > Hi Dmitry,
> > 
> > Thank you for the patch.
> > 
> > On Sat, Jul 29, 2023 at 03:49:12AM +0300, Dmitry Baryshkov wrote:
> >> To properly define the USB-C DP altmode connectors, add the USB
> >> subconnector type.
> >>
> >> Suggested-by: Simon Ser <contact@emersion.fr>
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> ---
> >>   drivers/gpu/drm/drm_connector.c | 1 +
> >>   include/uapi/drm/drm_mode.h     | 1 +
> >>   2 files changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> >> index a6066e4a5e9a..9e96b038f5d0 100644
> >> --- a/drivers/gpu/drm/drm_connector.c
> >> +++ b/drivers/gpu/drm/drm_connector.c
> >> @@ -1050,6 +1050,7 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
> >>   	{ DRM_MODE_SUBCONNECTOR_DisplayPort, "DP"        }, /* DP */
> >>   	{ DRM_MODE_SUBCONNECTOR_Wireless,    "Wireless"  }, /* DP */
> >>   	{ DRM_MODE_SUBCONNECTOR_Native,	     "Native"    }, /* DP */
> >> +	{ DRM_MODE_SUBCONNECTOR_USB,	     "USB"       }, /* DP */
> > 
> > Should this be DRM_MODE_SUBCONNECTOR_USB_C and "USB-C", in case we get
> > another USB type later ?
> 
> Hmm, which id should I use for micro-USB then? (consider anx7808, 
> SlimPort). I thought about using DRM_MODE_SUBCONNECTOR_USB for both of 
> them. But maybe I should add another subtype for SlimPort.

I suppose it depends on whether userspace needs a way to differentiate
those. Do you have a good visibility on the userspace use cases ?

> >>   };
> >>   
> >>   DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >> index 92d96a2b6763..0f74918b011c 100644
> >> --- a/include/uapi/drm/drm_mode.h
> >> +++ b/include/uapi/drm/drm_mode.h
> >> @@ -398,6 +398,7 @@ enum drm_mode_subconnector {
> >>   	DRM_MODE_SUBCONNECTOR_HDMIA       = 11, /*            DP */
> >>   	DRM_MODE_SUBCONNECTOR_Native      = 15, /*            DP */
> >>   	DRM_MODE_SUBCONNECTOR_Wireless    = 18, /*            DP */
> >> +	DRM_MODE_SUBCONNECTOR_USB         = 20, /*            DP */
> >>   };
> >>   
> >>   #define DRM_MODE_CONNECTOR_Unknown	0

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] drm/uapi: document the USB subconnector type

[Dmitry Baryshkov]

2 августа 2023 г. 22:13:51 GMT+03:00, Laurent Pinchart <laurent.pinchart@ideasonboard.com> пишет:
>On Wed, Aug 02, 2023 at 10:01:19PM +0300, Dmitry Baryshkov wrote:
>> On 02/08/2023 21:55, Laurent Pinchart wrote:
>> > Hi Dmitry,
>> > 
>> > Thank you for the patch.
>> > 
>> > On Sat, Jul 29, 2023 at 03:49:12AM +0300, Dmitry Baryshkov wrote:
>> >> To properly define the USB-C DP altmode connectors, add the USB
>> >> subconnector type.
>> >>
>> >> Suggested-by: Simon Ser <contact@emersion.fr>
>> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> >> ---
>> >>   drivers/gpu/drm/drm_connector.c | 1 +
>> >>   include/uapi/drm/drm_mode.h     | 1 +
>> >>   2 files changed, 2 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> >> index a6066e4a5e9a..9e96b038f5d0 100644
>> >> --- a/drivers/gpu/drm/drm_connector.c
>> >> +++ b/drivers/gpu/drm/drm_connector.c
>> >> @@ -1050,6 +1050,7 @@ static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
>> >>   	{ DRM_MODE_SUBCONNECTOR_DisplayPort, "DP"        }, /* DP */
>> >>   	{ DRM_MODE_SUBCONNECTOR_Wireless,    "Wireless"  }, /* DP */
>> >>   	{ DRM_MODE_SUBCONNECTOR_Native,	     "Native"    }, /* DP */
>> >> +	{ DRM_MODE_SUBCONNECTOR_USB,	     "USB"       }, /* DP */
>> > 
>> > Should this be DRM_MODE_SUBCONNECTOR_USB_C and "USB-C", in case we get
>> > another USB type later ?
>> 
>> Hmm, which id should I use for micro-USB then? (consider anx7808, 
>> SlimPort). I thought about using DRM_MODE_SUBCONNECTOR_USB for both of 
>> them. But maybe I should add another subtype for SlimPort.
>
>I suppose it depends on whether userspace needs a way to differentiate
>those. Do you have a good visibility on the userspace use cases ?

No. I'm not even sure, which userspace handles subtypes properly.

For the reference, SlimPort is mostly legacy hardware, think about Nexus 4, 5, 6, 7 (2013)


>
>> >>   };
>> >>   
>> >>   DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
>> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> >> index 92d96a2b6763..0f74918b011c 100644
>> >> --- a/include/uapi/drm/drm_mode.h
>> >> +++ b/include/uapi/drm/drm_mode.h
>> >> @@ -398,6 +398,7 @@ enum drm_mode_subconnector {
>> >>   	DRM_MODE_SUBCONNECTOR_HDMIA       = 11, /*            DP */
>> >>   	DRM_MODE_SUBCONNECTOR_Native      = 15, /*            DP */
>> >>   	DRM_MODE_SUBCONNECTOR_Wireless    = 18, /*            DP */
>> >> +	DRM_MODE_SUBCONNECTOR_USB         = 20, /*            DP */
>> >>   };
>> >>   
>> >>   #define DRM_MODE_CONNECTOR_Unknown	0
>

Re: [PATCH 3/4] drm/uapi: document the USB subconnector type

[Simon Ser]

On Wednesday, August 2nd, 2023 at 21:23, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> >> >> +	{ DRM_MODE_SUBCONNECTOR_USB,	     "USB"       }, /* DP */
> >> >
> >> > Should this be DRM_MODE_SUBCONNECTOR_USB_C and "USB-C", in case we get
> >> > another USB type later ?
> >>
> >> Hmm, which id should I use for micro-USB then? (consider anx7808,
> >> SlimPort). I thought about using DRM_MODE_SUBCONNECTOR_USB for both of
> >> them. But maybe I should add another subtype for SlimPort.
> >
> >I suppose it depends on whether userspace needs a way to differentiate
> >those. Do you have a good visibility on the userspace use cases ?
> 
> No. I'm not even sure, which userspace handles subtypes properly.

wlroots uses it for human-readable output descriptions, e.g.

    > wayland-info
    interface: 'wl_output',                                  version:  4, name: 49
    	name: DP-3
    	description: Samsung Electric Company SyncMaster HS3P505873 (DP-3 via DVI-D)

The "via DVI-D" bit comes from subconnector.

The description is displayed to the user when picking an output to screen
capture, among other things. It is helpful to users because they can better
understand why their output connected via DVI shows up as "DP".

The KMS docs describe "subconnector" to be defined as "downstream port" for DP.
Can USB-C (or USB) be seen as a DP downstream port?

Re: [PATCH 3/4] drm/uapi: document the USB subconnector type

[Simon Ser]

On Thursday, August 3rd, 2023 at 17:22, Simon Ser <contact@emersion.fr> wrote:

> The KMS docs describe "subconnector" to be defined as "downstream port" for DP.
> Can USB-C (or USB) be seen as a DP downstream port?

To expand on this a bit: I'm wondering if we're mixing apples and
oranges here. The current values of "subconnector" typically describe
the lower-level protocol tunneled inside DP. For instance, VGA can be
tunneled inside the DP cable when using DP → VGA adapter.

However, in the USB-C case, DP itself is tunneled inside USB-C. And you
might use a USB-C → DP adapter. So it's not really *sub*connector, it's
more of a *super*connector, right?

I think [1] is somewhat related, since it also allows user-space to
discover whether a connector uses USB-C. But relying on sysfs to figure
this out isn't super optimal perhaps.

[1]: https://lore.kernel.org/dri-devel/20221108185004.2263578-1-wonchung@google.com/

Re: [PATCH 3/4] drm/uapi: document the USB subconnector type

[Dmitry Baryshkov]

On Thu, 3 Aug 2023 at 18:31, Simon Ser <contact@emersion.fr> wrote:
>
> On Thursday, August 3rd, 2023 at 17:22, Simon Ser <contact@emersion.fr> wrote:
>
> > The KMS docs describe "subconnector" to be defined as "downstream port" for DP.
> > Can USB-C (or USB) be seen as a DP downstream port?
>
> To expand on this a bit: I'm wondering if we're mixing apples and
> oranges here. The current values of "subconnector" typically describe
> the lower-level protocol tunneled inside DP. For instance, VGA can be
> tunneled inside the DP cable when using DP → VGA adapter.

My opinion hasn't changed: I think this should be the USB connector
with proper DP / DVI / HDMI / etc. subconnector type (or lack of it).
In the end, the physical connector on the side of laptop is USB-C.

If we want to make it different from GUD, we might want to define a
USB-DP connector type (which would also include SlimPort).

>
> However, in the USB-C case, DP itself is tunneled inside USB-C. And you
> might use a USB-C → DP adapter. So it's not really *sub*connector, it's
> more of a *super*connector, right?
>
> I think [1] is somewhat related, since it also allows user-space to
> discover whether a connector uses USB-C. But relying on sysfs to figure
> this out isn't super optimal perhaps.
>
> [1]: https://lore.kernel.org/dri-devel/20221108185004.2263578-1-wonchung@google.com/



-- 
With best wishes
Dmitry

Re: [PATCH 3/4] drm/uapi: document the USB subconnector type

[Simon Ser]

On Thursday, August 3rd, 2023 at 17:36, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Thu, 3 Aug 2023 at 18:31, Simon Ser contact@emersion.fr wrote:
> 
> > On Thursday, August 3rd, 2023 at 17:22, Simon Ser contact@emersion.fr wrote:
> > 
> > > The KMS docs describe "subconnector" to be defined as "downstream port" for DP.
> > > Can USB-C (or USB) be seen as a DP downstream port?
> > 
> > To expand on this a bit: I'm wondering if we're mixing apples and
> > oranges here. The current values of "subconnector" typically describe
> > the lower-level protocol tunneled inside DP. For instance, VGA can be
> > tunneled inside the DP cable when using DP → VGA adapter.
> 
> My opinion hasn't changed: I think this should be the USB connector
> with proper DP / DVI / HDMI / etc. subconnector type (or lack of it).
> In the end, the physical connector on the side of laptop is USB-C.

- Even if the connector is USB-C, the protocol used for display is
  still DP. There's also the case of Thunderbolt.
- This is inconsistent with existing drivers. i915 and amdgpu expose
  DP ports for their USB-C ports. Changing that isn't possible without
  causing user-space regressions (compositor config files use the
  connector type).

Re: [PATCH 3/4] drm/uapi: document the USB subconnector type

[Dmitry Baryshkov]

On Thu, 3 Aug 2023 at 18:43, Simon Ser <contact@emersion.fr> wrote:
>
> On Thursday, August 3rd, 2023 at 17:36, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>
> > On Thu, 3 Aug 2023 at 18:31, Simon Ser contact@emersion.fr wrote:
> >
> > > On Thursday, August 3rd, 2023 at 17:22, Simon Ser contact@emersion.fr wrote:
> > >
> > > > The KMS docs describe "subconnector" to be defined as "downstream port" for DP.
> > > > Can USB-C (or USB) be seen as a DP downstream port?
> > >
> > > To expand on this a bit: I'm wondering if we're mixing apples and
> > > oranges here. The current values of "subconnector" typically describe
> > > the lower-level protocol tunneled inside DP. For instance, VGA can be
> > > tunneled inside the DP cable when using DP → VGA adapter.
> >
> > My opinion hasn't changed: I think this should be the USB connector
> > with proper DP / DVI / HDMI / etc. subconnector type (or lack of it).
> > In the end, the physical connector on the side of laptop is USB-C.
>
> - Even if the connector is USB-C, the protocol used for display is
>   still DP. There's also the case of Thunderbolt.

Yes. But the connector type is not about the protocol.

> - This is inconsistent with existing drivers. i915 and amdgpu expose
>   DP ports for their USB-C ports. Changing that isn't possible without
>   causing user-space regressions (compositor config files use the
>   connector type).

Yes, I know. Consider my phrase as a personal opinion or minority report.

I think that using DisplayPort for USB-C connectors was a mistake,
which we now have to cope with somehow.

-- 
With best wishes
Dmitry

Re: [PATCH 3/4] drm/uapi: document the USB subconnector type

[Laurent Pinchart]

On Thu, Aug 03, 2023 at 03:31:16PM +0000, Simon Ser wrote:
> On Thursday, August 3rd, 2023 at 17:22, Simon Ser <contact@emersion.fr> wrote:
> 
> > The KMS docs describe "subconnector" to be defined as "downstream port" for DP.
> > Can USB-C (or USB) be seen as a DP downstream port?
> 
> To expand on this a bit: I'm wondering if we're mixing apples and
> oranges here. The current values of "subconnector" typically describe
> the lower-level protocol tunneled inside DP. For instance, VGA can be
> tunneled inside the DP cable when using DP → VGA adapter.

Doesn't this contradict the example use case you gave in your previous
e-mail, with wlroots stating "DP-3 via DVI-D" ? I understand that as DP
carried over a DVI-D physical connector, did I get it wrong ?

> However, in the USB-C case, DP itself is tunneled inside USB-C. And you
> might use a USB-C → DP adapter. So it's not really *sub*connector, it's
> more of a *super*connector, right?
> 
> I think [1] is somewhat related, since it also allows user-space to
> discover whether a connector uses USB-C. But relying on sysfs to figure
> this out isn't super optimal perhaps.
> 
> [1]: https://lore.kernel.org/dri-devel/20221108185004.2263578-1-wonchung@google.com/

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] drm/uapi: document the USB subconnector type

[Simon Ser]

On Thursday, August 3rd, 2023 at 22:44, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> On Thu, Aug 03, 2023 at 03:31:16PM +0000, Simon Ser wrote:
> 
> > On Thursday, August 3rd, 2023 at 17:22, Simon Ser contact@emersion.fr wrote:
> > 
> > > The KMS docs describe "subconnector" to be defined as "downstream port" for DP.
> > > Can USB-C (or USB) be seen as a DP downstream port?
> > 
> > To expand on this a bit: I'm wondering if we're mixing apples and
> > oranges here. The current values of "subconnector" typically describe
> > the lower-level protocol tunneled inside DP. For instance, VGA can be
> > tunneled inside the DP cable when using DP → VGA adapter.
> 
> Doesn't this contradict the example use case you gave in your previous
> e-mail, with wlroots stating "DP-3 via DVI-D" ? I understand that as DP
> carried over a DVI-D physical connector, did I get it wrong ?

No, this is DVI carried over DP. DP cannot be carried over VGA/DVI/HDMI,
but VGA/DVI/HDMI can be carried over DP.

Re: [PATCH 3/4] drm/uapi: document the USB subconnector type

[Dmitry Baryshkov]

On Thu, 3 Aug 2023 at 23:47, Simon Ser <contact@emersion.fr> wrote:
>
> On Thursday, August 3rd, 2023 at 22:44, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
>
> > On Thu, Aug 03, 2023 at 03:31:16PM +0000, Simon Ser wrote:
> >
> > > On Thursday, August 3rd, 2023 at 17:22, Simon Ser contact@emersion.fr wrote:
> > >
> > > > The KMS docs describe "subconnector" to be defined as "downstream port" for DP.
> > > > Can USB-C (or USB) be seen as a DP downstream port?
> > >
> > > To expand on this a bit: I'm wondering if we're mixing apples and
> > > oranges here. The current values of "subconnector" typically describe
> > > the lower-level protocol tunneled inside DP. For instance, VGA can be
> > > tunneled inside the DP cable when using DP → VGA adapter.
> >
> > Doesn't this contradict the example use case you gave in your previous
> > e-mail, with wlroots stating "DP-3 via DVI-D" ? I understand that as DP
> > carried over a DVI-D physical connector, did I get it wrong ?
>
> No, this is DVI carried over DP. DP cannot be carried over VGA/DVI/HDMI,
> but VGA/DVI/HDMI can be carried over DP.

Well, not quite. It means that the sink (display) connected to this
port identifies itself as an VGA / DVI / HDMI monitor.
E.g. on if connect HDMI/DVI monitor through the DP-DVI dongle (native
DP connector), AMD driver still identifies it as 'subconnector HDMI',
despite dongle a DVI connector on the other side.

-- 
With best wishes
Dmitry

Re: [PATCH 3/4] drm/uapi: document the USB subconnector type

[Dmitry Baryshkov]

Simon, Laurent,

On 03/08/2023 23:46, Simon Ser wrote:
> On Thursday, August 3rd, 2023 at 22:44, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> 
>> On Thu, Aug 03, 2023 at 03:31:16PM +0000, Simon Ser wrote:
>>
>>> On Thursday, August 3rd, 2023 at 17:22, Simon Ser contact@emersion.fr wrote:
>>>
>>>> The KMS docs describe "subconnector" to be defined as "downstream port" for DP.
>>>> Can USB-C (or USB) be seen as a DP downstream port?
>>>
>>> To expand on this a bit: I'm wondering if we're mixing apples and
>>> oranges here. The current values of "subconnector" typically describe
>>> the lower-level protocol tunneled inside DP. For instance, VGA can be
>>> tunneled inside the DP cable when using DP → VGA adapter.
>>
>> Doesn't this contradict the example use case you gave in your previous
>> e-mail, with wlroots stating "DP-3 via DVI-D" ? I understand that as DP
>> carried over a DVI-D physical connector, did I get it wrong ?
> 
> No, this is DVI carried over DP. DP cannot be carried over VGA/DVI/HDMI,
> but VGA/DVI/HDMI can be carried over DP.

Please excuse me for the long delay, I was on vacation.

Several notes on the subconnector topic.

For TV and DVI-I we are really identifying a connector (or a part of the 
connector pins) present on the device.

So, we can have e.g. following combinations (type / subtype):

DVI-I / DVI-D (digital part of DVI connector)
DVI-I / DVI-A (analog part of DVI connector)

TV / S-Video (full S-Video connector)
TV / Composite (either a separate Composite connector, or shared with 
S-Video)
etc.

For DP unfortunately we have mixed everything together.
The physical connector present on the device can be DP / miniDP or USB-C 
(or micro-USB for SlimPort).

The physical protocol can be DP or DVI / HDMI (but only for dual-mode DP 
ports). Over USB-C link the DP can be transferred using DP or USB signal 
levels.

And last, but not least, we have the dongle / display connector type, 
which can be VGA (for active DP -> VGA converters), HDMI, DVI, DP, etc.

If we were designing this from the scratch, I'd say that we should 
encode physical connector type to DRM connector type and the dongle type 
to subconnector. However AMD and Intel drivers have already reused 
DisplayPort connector type for USB-C connections. Subconnector type 
represents (if known) the type of downstream / dongle port. I'm not 
going to judge whether this was correct or not. We have to live with 
this and behave in a similar way.

We have been looking for a way to document that the corresponding DP 
port is represented by the USB connector on the device.

Consequently, I believe the best way to document it, would be to use 
DisplayPort / USB, when there is no dongle connected, switching to 
DisplayPort / HDMI, DisplayPort / VGA, DisplayPort / DisplayPort, etc. 
when the actual dongle / display is connected and then switching back to 
the DisplayPort / USB when it gets disconnected.

If this sounds good to all parties, I'll post v2, adding this 
explanation to the cover letter.

-- 
With best wishes
Dmitry

Re: [PATCH 3/4] drm/uapi: document the USB subconnector type

[Simon Ser]

On Thursday, August 17th, 2023 at 21:33, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> We have been looking for a way to document that the corresponding DP
> port is represented by the USB connector on the device.
> 
> Consequently, I believe the best way to document it, would be to use
> DisplayPort / USB, when there is no dongle connected, switching to
> DisplayPort / HDMI, DisplayPort / VGA, DisplayPort / DisplayPort, etc.
> when the actual dongle / display is connected and then switching back to
> the DisplayPort / USB when it gets disconnected.
> 
> If this sounds good to all parties, I'll post v2, adding this
> explanation to the cover letter.

But how can user-space discover that the port is USB-C when it's
connected? That information is lost at this point.

(In addition, this clashes with the existing semantics of the
subconnector prop as discussed before: USB-C is not sub-, it's super-.)

Re: [PATCH 3/4] drm/uapi: document the USB subconnector type

[Dmitry Baryshkov]

On 18/08/2023 09:24, Simon Ser wrote:
> On Thursday, August 17th, 2023 at 21:33, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> 
>> We have been looking for a way to document that the corresponding DP
>> port is represented by the USB connector on the device.
>>
>> Consequently, I believe the best way to document it, would be to use
>> DisplayPort / USB, when there is no dongle connected, switching to
>> DisplayPort / HDMI, DisplayPort / VGA, DisplayPort / DisplayPort, etc.
>> when the actual dongle / display is connected and then switching back to
>> the DisplayPort / USB when it gets disconnected.
>>
>> If this sounds good to all parties, I'll post v2, adding this
>> explanation to the cover letter.
> 
> But how can user-space discover that the port is USB-C when it's
> connected? That information is lost at this point.

Yes, unfortunately.

> (In addition, this clashes with the existing semantics of the
> subconnector prop as discussed before: USB-C is not sub-, it's super-.)

Ok. How do we proceed then? Is it fine to add another property for DP 
case? Do you have any particular property name in mind? I will follow 
with addition of this property then.

-- 
With best wishes
Dmitry