[Intel-gfx] [PATCH v3 3/7] drm: report dp downstream port type as a subconnector property

Ville Syrjälä ville.syrjala at linux.intel.com
Wed Aug 28 14:25:46 UTC 2019


On Mon, Aug 26, 2019 at 04:22:12PM +0300, Oleg Vasilev wrote:
> Currently, downstream port type is only reported in debugfs. This
> information should be considered important since it reflects the actual
> physical connector type. Some userspace (e.g. window compositors)
> may want to show this info to a user.
> 
> The 'subconnector' property is already utilized for DVI-I and TV-out for
> reporting connector subtype.
> 
> The initial motivation for this feature came from i2c test [1].
> It is supposed to be skipped on VGA connectors, but it cannot
> detect VGA over DP and fails instead.
> 
> v2:
>  - Ville: utilized drm_dp_is_branch()
>  - Ville: implement DP 1.0 downstream type info
>  - Replaced create_dp_properties with add_dp_subconnector_property
>  - Added dp_set_subconnector_property helper
> 
> [1]: https://bugs.freedesktop.org/show_bug.cgi?id=104097
> 
> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
> Signed-off-by: Oleg Vasilev <oleg.vasilev at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_connector.c | 49 +++++++++++++++++++++++-
>  drivers/gpu/drm/drm_dp_helper.c | 68 +++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h     |  3 ++
>  include/drm/drm_dp_helper.h     |  8 ++++
>  include/drm/drm_mode_config.h   |  6 +++
>  include/uapi/drm/drm_mode.h     | 22 +++++++----
>  6 files changed, 146 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 4c766624b20d..47e3466891db 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -829,7 +829,7 @@ static const struct drm_prop_enum_list drm_dvi_i_select_enum_list[] = {
>  DRM_ENUM_NAME_FN(drm_get_dvi_i_select_name, drm_dvi_i_select_enum_list)
>  
>  static const struct drm_prop_enum_list drm_dvi_i_subconnector_enum_list[] = {
> -	{ DRM_MODE_SUBCONNECTOR_Unknown,   "Unknown"   }, /* DVI-I and TV-out */
> +	{ DRM_MODE_SUBCONNECTOR_Unknown,   "Unknown"   }, /* DVI-I, TV-out and DP */
>  	{ DRM_MODE_SUBCONNECTOR_DVID,      "DVI-D"     }, /* DVI-I  */
>  	{ DRM_MODE_SUBCONNECTOR_DVIA,      "DVI-A"     }, /* DVI-I  */
>  };
> @@ -846,7 +846,7 @@ static const struct drm_prop_enum_list drm_tv_select_enum_list[] = {
>  DRM_ENUM_NAME_FN(drm_get_tv_select_name, drm_tv_select_enum_list)
>  
>  static const struct drm_prop_enum_list drm_tv_subconnector_enum_list[] = {
> -	{ DRM_MODE_SUBCONNECTOR_Unknown,   "Unknown"   }, /* DVI-I and TV-out */
> +	{ DRM_MODE_SUBCONNECTOR_Unknown,   "Unknown"   }, /* DVI-I, TV-out and DP */
>  	{ DRM_MODE_SUBCONNECTOR_Composite, "Composite" }, /* TV-out */
>  	{ DRM_MODE_SUBCONNECTOR_SVIDEO,    "SVIDEO"    }, /* TV-out */
>  	{ DRM_MODE_SUBCONNECTOR_Component, "Component" }, /* TV-out */
> @@ -855,6 +855,19 @@ static const struct drm_prop_enum_list drm_tv_subconnector_enum_list[] = {
>  DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>  		 drm_tv_subconnector_enum_list)
>  
> +static const struct drm_prop_enum_list drm_dp_subconnector_enum_list[] = {
> +	{ DRM_MODE_SUBCONNECTOR_Unknown,   "Unknown"   }, /* DVI-I, TV-out and DP */
> +	{ DRM_MODE_SUBCONNECTOR_VGA,	   "VGA"       }, /* DP */
> +	{ DRM_MODE_SUBCONNECTOR_DVI,	   "DVI"       }, /* DP */
> +	{ DRM_MODE_SUBCONNECTOR_HDMI,	   "HDMI"      }, /* DP */
> +	{ DRM_MODE_SUBCONNECTOR_DP,        "DP"        }, /* DP */
> +	{ DRM_MODE_SUBCONNECTOR_Wireless,  "Wireless"  }, /* DP */
> +	{ DRM_MODE_SUBCONNECTOR_Native,	   "Native"    }, /* DP */
> +};
> +
> +DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
> +		 drm_dp_subconnector_enum_list)
> +
>  static const struct drm_prop_enum_list hdmi_colorspaces[] = {
>  	/* For Default case, driver will set the colorspace */
>  	{ DRM_MODE_COLORIMETRY_DEFAULT, "Default" },
> @@ -1138,6 +1151,14 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {
>   *	can also expose this property to external outputs, in which case they
>   *	must support "None", which should be the default (since external screens
>   *	have a built-in scaler).
> + *
> + * subconnector:
> + *	This property is used by DVI-I, TVout and DisplayPort to indicate different
> + *	connector subtypes. Enum values more or less match with those from main
> + *	connector types.
> + *	For DVI-I and TVout there is also a matching property "select subconnector"
> + *	allowing to switch between signal types.
> + *	DP subconnector corresponds to a downstream port.
>   */
>  
>  int drm_connector_create_standard_properties(struct drm_device *dev)
> @@ -1226,6 +1247,30 @@ int drm_mode_create_dvi_i_properties(struct drm_device *dev)
>  }
>  EXPORT_SYMBOL(drm_mode_create_dvi_i_properties);
>  
> +/**
> + * drm_mode_add_dp_subconnector_property - create subconnector property for DP
> + * @connector: drm_connector to attach property
> + *
> + * Called by a driver when DP connector is created.
> + */
> +void drm_mode_add_dp_subconnector_property(struct drm_connector *connector)
> +{
> +	struct drm_mode_config *mode_config = &connector->dev->mode_config;
> +
> +	if (!mode_config->dp_subconnector_property)
> +		mode_config->dp_subconnector_property =
> +			drm_property_create_enum(connector->dev,
> +				DRM_MODE_PROP_IMMUTABLE,
> +				"subconnector",
> +				drm_dp_subconnector_enum_list,
> +				ARRAY_SIZE(drm_dp_subconnector_enum_list));
> +
> +	drm_object_attach_property(&connector->base,
> +				   mode_config->dp_subconnector_property,
> +				   DRM_MODE_SUBCONNECTOR_Unknown);
> +}
> +EXPORT_SYMBOL(drm_mode_add_dp_subconnector_property);
> +
>  /**
>   * DOC: HDMI connector properties
>   *
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 14320930091b..bca116cbc58f 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -638,6 +638,74 @@ void drm_dp_downstream_debug(struct seq_file *m,
>  }
>  EXPORT_SYMBOL(drm_dp_downstream_debug);
>  
> +/**
> + * drm_dp_subconnector_type() - get DP branch device type
> + * @dpcd: DisplayPort configuration data
> + * @port_cap: port capabilities
> + *
> + */
> +enum drm_mode_subconnector
> +drm_dp_subconnector_type(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> +			 const u8 port_cap[4])
> +{
> +	int type;
> +	bool branch_device_present = drm_dp_is_branch(dpcd);

Having a variable for this seems kinda pointless.

> +
> +	if (!branch_device_present)
> +		return DRM_MODE_SUBCONNECTOR_Native;
> +	/* DP 1.0 approach */
> +	if (dpcd[DP_DPCD_REV] == DP_DPCD_REV_10) {
> +		type = dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> +		       DP_DWN_STRM_PORT_TYPE_MASK;
> +		if (type == DP_DWN_STRM_PORT_TYPE_DP)
> +			return DRM_MODE_SUBCONNECTOR_DP;

TMDS -> DVI maybe?
analog -> VGA maybe?

> +		return DRM_MODE_SUBCONNECTOR_Unknown;
> +	}
> +	type = port_cap[0] & DP_DS_PORT_TYPE_MASK;
> +
> +	switch (type) {
> +	case DP_DS_PORT_TYPE_DP:
> +	case DP_DS_PORT_TYPE_DP_DUALMODE:
> +		return DRM_MODE_SUBCONNECTOR_DP;
> +	case DP_DS_PORT_TYPE_VGA:
> +		return DRM_MODE_SUBCONNECTOR_VGA;
> +	case DP_DS_PORT_TYPE_DVI:
> +		return DRM_MODE_SUBCONNECTOR_DVI;
> +	case DP_DS_PORT_TYPE_HDMI:
> +		return DRM_MODE_SUBCONNECTOR_HDMI;
> +	case DP_DS_PORT_TYPE_WIRELESS:
> +		return DRM_MODE_SUBCONNECTOR_Wireless;
> +	case DP_DS_PORT_TYPE_NON_EDID:
> +	default:
> +		return DRM_MODE_SUBCONNECTOR_Unknown;
> +	}
> +}
> +EXPORT_SYMBOL(drm_dp_subconnector_type);
> +
> +/**
> + * drm_mode_set_dp_subconnector_property - set subconnector for DP connector
> + * @connector: dp connector to attach property
> + * @status: connector status which is about to be set
> + * @dpcd: DisplayPort configuration data
> + * @port_cap: port capabilities
> + *
> + * Called by a driver on every detect event.
> + */
> +void drm_dp_set_subconnector_property(struct drm_connector *connector,
> +				      enum drm_connector_status status,
> +				      const u8 *dpcd,
> +				      const u8 port_cap[4])
> +{
> +	enum drm_mode_subconnector subconnector = DRM_MODE_SUBCONNECTOR_Unknown;
> +
> +	if (status == connector_status_connected)
> +		subconnector = drm_dp_subconnector_type(dpcd, port_cap);
> +	drm_object_property_set_value(&connector->base,
> +			connector->dev->mode_config.dp_subconnector_property,
> +			subconnector);
> +}
> +EXPORT_SYMBOL(drm_dp_set_subconnector_property);
> +
>  /*
>   * I2C-over-AUX implementation
>   */
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 681cb590f952..119606fcd59f 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1507,10 +1507,13 @@ const char *drm_get_dvi_i_subconnector_name(int val);
>  const char *drm_get_dvi_i_select_name(int val);
>  const char *drm_get_tv_subconnector_name(int val);
>  const char *drm_get_tv_select_name(int val);
> +const char *drm_get_dp_subconnector_name(int val);
>  const char *drm_get_content_protection_name(int val);
>  const char *drm_get_hdcp_content_type_name(int val);
>  
>  int drm_mode_create_dvi_i_properties(struct drm_device *dev);
> +void drm_mode_add_dp_subconnector_property(struct drm_connector *connector);
> +
>  int drm_mode_create_tv_margin_properties(struct drm_device *dev);
>  int drm_mode_create_tv_properties(struct drm_device *dev,
>  				  unsigned int num_modes,
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 1aea3e0810db..1fec3c731017 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -26,6 +26,7 @@
>  #include <linux/types.h>
>  #include <linux/i2c.h>
>  #include <linux/delay.h>
> +#include <drm/drm_connector.h>
>  
>  /*
>   * Unless otherwise noted, all values are from the DP 1.1a spec.  Note that
> @@ -1378,6 +1379,13 @@ int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
>  int drm_dp_downstream_id(struct drm_dp_aux *aux, char id[6]);
>  void drm_dp_downstream_debug(struct seq_file *m, const u8 dpcd[DP_RECEIVER_CAP_SIZE],
>  			     const u8 port_cap[4], struct drm_dp_aux *aux);
> +enum drm_mode_subconnector
> +drm_dp_subconnector_type(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> +			 const u8 port_cap[4]);
> +void drm_dp_set_subconnector_property(struct drm_connector *connector,
> +				      enum drm_connector_status status,
> +				      const u8 *dpcd,
> +				      const u8 port_cap[4]);
>  
>  void drm_dp_aux_init(struct drm_dp_aux *aux);
>  int drm_dp_aux_register(struct drm_dp_aux *aux);
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 3bcbe30339f0..732d51951bce 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -680,6 +680,12 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *dvi_i_select_subconnector_property;
>  
> +	/**
> +	 * @dp_subconnector_property: Optional DP property to differentiate
> +	 * between different DP downstream port types.
> +	 */
> +	struct drm_property *dp_subconnector_property;
> +
>  	/**
>  	 * @tv_subconnector_property: Optional TV property to differentiate
>  	 * between different TV connector types.
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 735c8cfdaaa1..80e3118fee13 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -332,14 +332,20 @@ struct drm_mode_get_encoder {
>  /* This is for connectors with multiple signal types. */
>  /* Try to match DRM_MODE_CONNECTOR_X as closely as possible. */
>  enum drm_mode_subconnector {
> -	DRM_MODE_SUBCONNECTOR_Automatic = 0,
> -	DRM_MODE_SUBCONNECTOR_Unknown = 0,
> -	DRM_MODE_SUBCONNECTOR_DVID = 3,
> -	DRM_MODE_SUBCONNECTOR_DVIA = 4,
> -	DRM_MODE_SUBCONNECTOR_Composite = 5,
> -	DRM_MODE_SUBCONNECTOR_SVIDEO = 6,
> -	DRM_MODE_SUBCONNECTOR_Component = 8,
> -	DRM_MODE_SUBCONNECTOR_SCART = 9,
> +	DRM_MODE_SUBCONNECTOR_Automatic  = 0,  /* DVI-I, TV     */
> +	DRM_MODE_SUBCONNECTOR_Unknown    = 0,  /* DVI-I, TV, DP */
> +	DRM_MODE_SUBCONNECTOR_VGA	 = 1,  /*            DP */
> +	DRM_MODE_SUBCONNECTOR_DVI	 = 2,  /*            DP */

I suspect the DP DFP TMDS/DVI stuff is always DVI-D. So maybe we
should just reuse that one?

> +	DRM_MODE_SUBCONNECTOR_DVID	 = 3,  /* DVI-I         */
> +	DRM_MODE_SUBCONNECTOR_DVIA	 = 4,  /* DVI-I         */
> +	DRM_MODE_SUBCONNECTOR_Composite  = 5,  /*        TV     */
> +	DRM_MODE_SUBCONNECTOR_SVIDEO	 = 6,  /*        TV     */
> +	DRM_MODE_SUBCONNECTOR_Component  = 8,  /*        TV     */
> +	DRM_MODE_SUBCONNECTOR_SCART	 = 9,  /*        TV     */
> +	DRM_MODE_SUBCONNECTOR_DP	 = 10, /*            DP */
> +	DRM_MODE_SUBCONNECTOR_HDMI       = 11, /*            DP */

Maybe we want to call it HDMIA just to match the connector counterpart?

> +	DRM_MODE_SUBCONNECTOR_Native     = 15, /*            DP */
> +	DRM_MODE_SUBCONNECTOR_Wireless   = 19, /*            DP */

Not sure what numbers we should assign these. The comment says
we're trying to match DRM_MODE_CONNECTOR_ here. Not sure why that's
needed though.

Reusing the WRITEBACK value for Wireless seems like a safe choice?

>  };
>  
>  #define DRM_MODE_CONNECTOR_Unknown	0
> -- 
> 2.23.0

-- 
Ville Syrjälä
Intel


More information about the Intel-gfx mailing list