From: Ville Syrjälä ville.syrjala@linux.intel.com
Here's a possible apporoach for providing userspace with some stable connector identifiers. Combine with the bus name of the GPU and you should have some kind of real physical path description. Unfortunately the ship has sailed for MST connectors because userspace is already parsing the property and expects to find certain things there. So if we want stable names for those we'd probably have introduce another PATH prop (PHYS_PATH?).
I suppose one alternative would to make the connector type_id stable. Currently that is being populated by drm core and it's just a global counter. Not sure how badly things would turn out if we'd allow each driver to set that. It could result in conflicting xrandr connector names between different GPUs which I suppose would confuse existing userspace?
Cc: Daniel Vetter daniel@ffwll.ch Cc: Pekka Paalanen ppaalanen@gmail.com Cc: Ilia Mirkin imirkin@alum.mit.edu
Ville Syrjälä (2): drm: Improve PATH prop docs drm/i915: Populate PATH prop for every connector
drivers/gpu/drm/drm_connector.c | 13 ++++++++-- drivers/gpu/drm/i915/icl_dsi.c | 3 +++ drivers/gpu/drm/i915/intel_connector.c | 20 +++++++++++++++ drivers/gpu/drm/i915/intel_connector.h | 3 +++ drivers/gpu/drm/i915/intel_crt.c | 2 ++ drivers/gpu/drm/i915/intel_dp.c | 6 ++++- drivers/gpu/drm/i915/intel_dp_mst.c | 3 +-- drivers/gpu/drm/i915/intel_dvo.c | 3 +++ drivers/gpu/drm/i915/intel_hdmi.c | 4 +++ drivers/gpu/drm/i915/intel_lvds.c | 2 ++ drivers/gpu/drm/i915/intel_sdvo.c | 35 ++++++++++++++++++-------- drivers/gpu/drm/i915/intel_tv.c | 2 ++ drivers/gpu/drm/i915/vlv_dsi.c | 3 +++ 13 files changed, 83 insertions(+), 16 deletions(-)
From: Ville Syrjälä ville.syrjala@linux.intel.com
The PATH blob is already being parsed by userspace for MST connectors so the layout of the blob is now uabi. Let's document what it should look like.
Also add a clear note saying non-MST connectors can have a PATH prop too.
Cc: Daniel Vetter daniel@ffwll.ch Cc: Pekka Paalanen ppaalanen@gmail.com Cc: Ilia Mirkin imirkin@alum.mit.edu Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_connector.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index e17586aaa80f..ce3926e9ad11 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -899,7 +899,16 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = { * 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 - * property. + * property. The value must be an ASCII string. + * + * For DP MST connectors the path string follows the pattern + * "mst:<base connector ID>[-<mst port>]...", where the base connector ID + * identifies the DP connector on the source device, and the mst ports + * are the port numbers in the DP MST topology. + * + * For non-DP MST connectors the format is freeform, as long as it + * uniquely identifies the physical path, remains stable across + * kernel releases, and does not start with "mst:". * 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 @@ -1678,7 +1687,7 @@ int drm_mode_create_suggested_offset_properties(struct drm_device *dev) EXPORT_SYMBOL(drm_mode_create_suggested_offset_properties);
/** - * drm_connector_set_path_property - set tile property on connector + * drm_connector_set_path_property - set path property on connector * @connector: connector to set property on. * @path: path to use for property; must not be NULL. *
On Thu, 13 Jun 2019 21:43:34 +0300 Ville Syrjala ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
The PATH blob is already being parsed by userspace for MST connectors so the layout of the blob is now uabi. Let's document what it should look like.
Also add a clear note saying non-MST connectors can have a PATH prop too.
Cc: Daniel Vetter daniel@ffwll.ch Cc: Pekka Paalanen ppaalanen@gmail.com Cc: Ilia Mirkin imirkin@alum.mit.edu Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_connector.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index e17586aaa80f..ce3926e9ad11 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -899,7 +899,16 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {
- 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
- property.
- property. The value must be an ASCII string.
- For DP MST connectors the path string follows the pattern
- "mst:<base connector ID>[-<mst port>]...", where the base connector ID
- identifies the DP connector on the source device, and the mst ports
- are the port numbers in the DP MST topology.
Hi,
what exactly is the connector ID as already used here for MST? Is it not persistent?
I assume the MST port numbers are persistent as long as the physical topology does not change.
- For non-DP MST connectors the format is freeform, as long as it
- uniquely identifies the physical path, remains stable across
- kernel releases, and does not start with "mst:".
Maybe the requirements for "persistent name" should be clarified more: - remains stable across kernel releases - is immune to driver loading/initialisation order changes - is immune to adding or removing other hardware (e.g. graphics cards)
Maybe also some explicit words about what to do with dynamic non-MST connectors?
Thanks, pq
From: Ville Syrjälä ville.syrjala@linux.intel.com
Userspace may want stable identifiers for connectors. Let's try to provide that via the PATH prop. I tried to make these somewhat abstract by using just "port_type:index" type of approach, where we derive the index from the physical instance of that hw block, so it should remain stable even if we reorder things in the driver.
Cc: Daniel Vetter daniel@ffwll.ch Cc: Pekka Paalanen ppaalanen@gmail.com Cc: Ilia Mirkin imirkin@alum.mit.edu Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/icl_dsi.c | 3 +++ drivers/gpu/drm/i915/intel_connector.c | 20 +++++++++++++++ drivers/gpu/drm/i915/intel_connector.h | 3 +++ drivers/gpu/drm/i915/intel_crt.c | 2 ++ drivers/gpu/drm/i915/intel_dp.c | 6 ++++- drivers/gpu/drm/i915/intel_dp_mst.c | 3 +-- drivers/gpu/drm/i915/intel_dvo.c | 3 +++ drivers/gpu/drm/i915/intel_hdmi.c | 4 +++ drivers/gpu/drm/i915/intel_lvds.c | 2 ++ drivers/gpu/drm/i915/intel_sdvo.c | 35 ++++++++++++++++++-------- drivers/gpu/drm/i915/intel_tv.c | 2 ++ drivers/gpu/drm/i915/vlv_dsi.c | 3 +++ 12 files changed, 72 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c index 74448e6bf749..54ccc69aa60a 100644 --- a/drivers/gpu/drm/i915/icl_dsi.c +++ b/drivers/gpu/drm/i915/icl_dsi.c @@ -1544,6 +1544,9 @@ void icl_dsi_init(struct drm_i915_private *dev_priv) /* attach connector to encoder */ intel_connector_attach_encoder(intel_connector, encoder);
+ intel_connector_set_path_property(connector, "dsi:%d", + port - PORT_A); + mutex_lock(&dev->mode_config.mutex); fixed_mode = intel_panel_vbt_fixed_mode(intel_connector); mutex_unlock(&dev->mode_config.mutex); diff --git a/drivers/gpu/drm/i915/intel_connector.c b/drivers/gpu/drm/i915/intel_connector.c index 073b6c3ab7cc..6c1b027fdb11 100644 --- a/drivers/gpu/drm/i915/intel_connector.c +++ b/drivers/gpu/drm/i915/intel_connector.c @@ -280,3 +280,23 @@ intel_attach_colorspace_property(struct drm_connector *connector) drm_object_attach_property(&connector->base, connector->colorspace_property, 0); } + +int intel_connector_set_path_property(struct drm_connector *connector, + const char *fmt, ...) +{ + char path[64]; + va_list ap; + int ret; + + va_start(ap, fmt); + ret = vsnprintf(path, sizeof(path), fmt, ap); + va_end(ap); + + if (WARN_ON(ret >= sizeof(path))) + return -EINVAL; + + drm_object_attach_property(&connector->base, + connector->dev->mode_config.path_property, 0); + + return drm_connector_set_path_property(connector, path); +} diff --git a/drivers/gpu/drm/i915/intel_connector.h b/drivers/gpu/drm/i915/intel_connector.h index 93a7375c8196..108777bc9545 100644 --- a/drivers/gpu/drm/i915/intel_connector.h +++ b/drivers/gpu/drm/i915/intel_connector.h @@ -31,5 +31,8 @@ void intel_attach_force_audio_property(struct drm_connector *connector); void intel_attach_broadcast_rgb_property(struct drm_connector *connector); void intel_attach_aspect_ratio_property(struct drm_connector *connector); void intel_attach_colorspace_property(struct drm_connector *connector); +__printf(2, 3) +int intel_connector_set_path_property(struct drm_connector *connector, + const char *fmt, ...);
#endif /* __INTEL_CONNECTOR_H__ */ diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 3fcf2f84bcce..1383db646986 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -1048,6 +1048,8 @@ void intel_crt_init(struct drm_i915_private *dev_priv) if (!I915_HAS_HOTPLUG(dev_priv)) intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
+ intel_connector_set_path_property(connector, "crt:0"); + /* * Configure the automatic hotplug detection stuff */ diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 4336df46fe78..c9071d25bd37 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -6527,7 +6527,11 @@ static void intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connector) { struct drm_i915_private *dev_priv = to_i915(connector->dev); - enum port port = dp_to_dig_port(intel_dp)->base.port; + struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base; + enum port port = encoder->port; + + intel_connector_set_path_property(connector, "ddi:%d\n", + port - PORT_A);
if (!IS_G4X(dev_priv) && port != PORT_A) intel_attach_force_audio_property(connector); diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 0caf645fbbb8..3bc0de2ff5af 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -529,10 +529,9 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo goto err; }
- drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0); drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
- ret = drm_connector_set_path_property(connector, pathprop); + ret = intel_connector_set_path_property(connector, "%s", pathprop); if (ret) goto err;
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c index 22666d28f4aa..4e7ea0f4c5d5 100644 --- a/drivers/gpu/drm/i915/intel_dvo.c +++ b/drivers/gpu/drm/i915/intel_dvo.c @@ -531,6 +531,9 @@ void intel_dvo_init(struct drm_i915_private *dev_priv) connector->interlace_allowed = false; connector->doublescan_allowed = false;
+ intel_connector_set_path_property(connector, "dvo:%d", + port - PORT_A); + intel_connector_attach_encoder(intel_connector, intel_encoder); if (dvo->type == INTEL_DVO_CHIP_LVDS) { /* diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 187a2b828b97..38a0e423420a 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -2794,6 +2794,10 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c struct drm_i915_private *dev_priv = to_i915(connector->dev); struct intel_digital_port *intel_dig_port = hdmi_to_dig_port(intel_hdmi); + enum port port = intel_dig_port->base.port; + + intel_connector_set_path_property(connector, "ddi:%d", + port - PORT_A);
intel_attach_force_audio_property(connector); intel_attach_broadcast_rgb_property(connector); diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index efefed62a7f8..463665f0ecbd 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -915,6 +915,8 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
lvds_encoder->reg = lvds_reg;
+ intel_connector_set_path_property(connector, "lvds:0"); + /* create the scaling mode property */ allowed_scalers = BIT(DRM_MODE_SCALE_ASPECT); allowed_scalers |= BIT(DRM_MODE_SCALE_FULLSCREEN); diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 0860ae36bb87..c16cdde849cc 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -2650,9 +2650,8 @@ static struct intel_sdvo_connector *intel_sdvo_connector_alloc(void) static bool intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device) { - struct drm_encoder *encoder = &intel_sdvo->base.base; + struct intel_encoder *encoder = &intel_sdvo->base; struct drm_connector *connector; - struct intel_encoder *intel_encoder = to_intel_encoder(encoder); struct intel_connector *intel_connector; struct intel_sdvo_connector *intel_sdvo_connector;
@@ -2679,12 +2678,12 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device) * Some SDVO devices have one-shot hotplug interrupts. * Ensure that they get re-enabled when an interrupt happens. */ - intel_encoder->hotplug = intel_sdvo_hotplug; - intel_sdvo_enable_hotplug(intel_encoder); + encoder->hotplug = intel_sdvo_hotplug; + intel_sdvo_enable_hotplug(encoder); } else { intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT; } - encoder->encoder_type = DRM_MODE_ENCODER_TMDS; + encoder->base.encoder_type = DRM_MODE_ENCODER_TMDS; connector->connector_type = DRM_MODE_CONNECTOR_DVID;
if (intel_sdvo_is_hdmi_connector(intel_sdvo, device)) { @@ -2700,13 +2699,18 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device) if (intel_sdvo_connector->is_hdmi) intel_sdvo_add_hdmi_properties(intel_sdvo, intel_sdvo_connector);
+ intel_connector_set_path_property(connector, "sdvo:%d:%s:%d", + encoder->port - PORT_A, + intel_sdvo_connector->is_hdmi ? + "hdmi" : "dvi", device); + return true; }
static bool intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type) { - struct drm_encoder *encoder = &intel_sdvo->base.base; + struct intel_encoder *encoder = &intel_sdvo->base; struct drm_connector *connector; struct intel_connector *intel_connector; struct intel_sdvo_connector *intel_sdvo_connector; @@ -2719,7 +2723,7 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
intel_connector = &intel_sdvo_connector->base; connector = &intel_connector->base; - encoder->encoder_type = DRM_MODE_ENCODER_TVDAC; + encoder->base.encoder_type = DRM_MODE_ENCODER_TVDAC; connector->connector_type = DRM_MODE_CONNECTOR_SVIDEO;
intel_sdvo->controlled_output |= type; @@ -2736,6 +2740,9 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type) if (!intel_sdvo_create_enhance_property(intel_sdvo, intel_sdvo_connector)) goto err;
+ intel_connector_set_path_property(connector, "sdvo:%d:tv:%d", + encoder->port - PORT_A, type); + return true;
err: @@ -2746,7 +2753,7 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type) static bool intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device) { - struct drm_encoder *encoder = &intel_sdvo->base.base; + struct intel_encoder *encoder = &intel_sdvo->base; struct drm_connector *connector; struct intel_connector *intel_connector; struct intel_sdvo_connector *intel_sdvo_connector; @@ -2760,7 +2767,7 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device) intel_connector = &intel_sdvo_connector->base; connector = &intel_connector->base; intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT; - encoder->encoder_type = DRM_MODE_ENCODER_DAC; + encoder->base.encoder_type = DRM_MODE_ENCODER_DAC; connector->connector_type = DRM_MODE_CONNECTOR_VGA;
if (device == 0) { @@ -2776,13 +2783,16 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device) return false; }
+ intel_connector_set_path_property(connector, "sdvo:%d:crt:%d", + encoder->port - PORT_A, device); + return true; }
static bool intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device) { - struct drm_encoder *encoder = &intel_sdvo->base.base; + struct intel_encoder *encoder = &intel_sdvo->base; struct drm_connector *connector; struct intel_connector *intel_connector; struct intel_sdvo_connector *intel_sdvo_connector; @@ -2796,7 +2806,7 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
intel_connector = &intel_sdvo_connector->base; connector = &intel_connector->base; - encoder->encoder_type = DRM_MODE_ENCODER_LVDS; + encoder->base.encoder_type = DRM_MODE_ENCODER_LVDS; connector->connector_type = DRM_MODE_CONNECTOR_LVDS;
if (device == 0) { @@ -2831,6 +2841,9 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device) if (!intel_connector->panel.fixed_mode) goto err;
+ intel_connector_set_path_property(connector, "sdvo:%d:lvds:%d", + encoder->port - PORT_A, device); + return true;
err: diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c index 5dc594eafaf2..f9481404f642 100644 --- a/drivers/gpu/drm/i915/intel_tv.c +++ b/drivers/gpu/drm/i915/intel_tv.c @@ -1988,4 +1988,6 @@ intel_tv_init(struct drm_i915_private *dev_priv) drm_object_attach_property(&connector->base, dev->mode_config.tv_bottom_margin_property, state->tv.margins.bottom); + + intel_connector_set_path_property(connector, "tv:0"); } diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c index e272d826210a..e97e689c6021 100644 --- a/drivers/gpu/drm/i915/vlv_dsi.c +++ b/drivers/gpu/drm/i915/vlv_dsi.c @@ -1985,6 +1985,9 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
intel_dsi_add_properties(intel_connector);
+ intel_connector_set_path_property(connector, "dsi:%d", + port - PORT_A); + return;
err_cleanup_connector:
On Thu, 13 Jun 2019 21:43:35 +0300 Ville Syrjala ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Userspace may want stable identifiers for connectors. Let's try to provide that via the PATH prop. I tried to make these somewhat abstract by using just "port_type:index" type of approach, where we derive the index from the physical instance of that hw block, so it should remain stable even if we reorder things in the driver.
Cc: Daniel Vetter daniel@ffwll.ch Cc: Pekka Paalanen ppaalanen@gmail.com Cc: Ilia Mirkin imirkin@alum.mit.edu Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Hi,
I don't know intel driver nor hardware, but the described idea and the names in the code look good to me.
The only open question is if it should be the existing PATH property or a new property in case PATH for MST is not persistent.
Thanks, pq
drivers/gpu/drm/i915/icl_dsi.c | 3 +++ drivers/gpu/drm/i915/intel_connector.c | 20 +++++++++++++++ drivers/gpu/drm/i915/intel_connector.h | 3 +++ drivers/gpu/drm/i915/intel_crt.c | 2 ++ drivers/gpu/drm/i915/intel_dp.c | 6 ++++- drivers/gpu/drm/i915/intel_dp_mst.c | 3 +-- drivers/gpu/drm/i915/intel_dvo.c | 3 +++ drivers/gpu/drm/i915/intel_hdmi.c | 4 +++ drivers/gpu/drm/i915/intel_lvds.c | 2 ++ drivers/gpu/drm/i915/intel_sdvo.c | 35 ++++++++++++++++++-------- drivers/gpu/drm/i915/intel_tv.c | 2 ++ drivers/gpu/drm/i915/vlv_dsi.c | 3 +++ 12 files changed, 72 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c index 74448e6bf749..54ccc69aa60a 100644 --- a/drivers/gpu/drm/i915/icl_dsi.c +++ b/drivers/gpu/drm/i915/icl_dsi.c @@ -1544,6 +1544,9 @@ void icl_dsi_init(struct drm_i915_private *dev_priv) /* attach connector to encoder */ intel_connector_attach_encoder(intel_connector, encoder);
- intel_connector_set_path_property(connector, "dsi:%d",
port - PORT_A);- mutex_lock(&dev->mode_config.mutex); fixed_mode = intel_panel_vbt_fixed_mode(intel_connector); mutex_unlock(&dev->mode_config.mutex);
diff --git a/drivers/gpu/drm/i915/intel_connector.c b/drivers/gpu/drm/i915/intel_connector.c index 073b6c3ab7cc..6c1b027fdb11 100644 --- a/drivers/gpu/drm/i915/intel_connector.c +++ b/drivers/gpu/drm/i915/intel_connector.c @@ -280,3 +280,23 @@ intel_attach_colorspace_property(struct drm_connector *connector) drm_object_attach_property(&connector->base, connector->colorspace_property, 0); }
+int intel_connector_set_path_property(struct drm_connector *connector,
const char *fmt, ...)+{
- char path[64];
- va_list ap;
- int ret;
- va_start(ap, fmt);
- ret = vsnprintf(path, sizeof(path), fmt, ap);
- va_end(ap);
- if (WARN_ON(ret >= sizeof(path)))
return -EINVAL;- drm_object_attach_property(&connector->base,
connector->dev->mode_config.path_property, 0);- return drm_connector_set_path_property(connector, path);
+} diff --git a/drivers/gpu/drm/i915/intel_connector.h b/drivers/gpu/drm/i915/intel_connector.h index 93a7375c8196..108777bc9545 100644 --- a/drivers/gpu/drm/i915/intel_connector.h +++ b/drivers/gpu/drm/i915/intel_connector.h @@ -31,5 +31,8 @@ void intel_attach_force_audio_property(struct drm_connector *connector); void intel_attach_broadcast_rgb_property(struct drm_connector *connector); void intel_attach_aspect_ratio_property(struct drm_connector *connector); void intel_attach_colorspace_property(struct drm_connector *connector); +__printf(2, 3) +int intel_connector_set_path_property(struct drm_connector *connector,
const char *fmt, ...);#endif /* __INTEL_CONNECTOR_H__ */ diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 3fcf2f84bcce..1383db646986 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -1048,6 +1048,8 @@ void intel_crt_init(struct drm_i915_private *dev_priv) if (!I915_HAS_HOTPLUG(dev_priv)) intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
- intel_connector_set_path_property(connector, "crt:0");
- /*
*/
- Configure the automatic hotplug detection stuff
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 4336df46fe78..c9071d25bd37 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -6527,7 +6527,11 @@ static void intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connector) { struct drm_i915_private *dev_priv = to_i915(connector->dev);
- enum port port = dp_to_dig_port(intel_dp)->base.port;
struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
enum port port = encoder->port;
intel_connector_set_path_property(connector, "ddi:%d\n",
port - PORT_A);if (!IS_G4X(dev_priv) && port != PORT_A) intel_attach_force_audio_property(connector);
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 0caf645fbbb8..3bc0de2ff5af 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -529,10 +529,9 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo goto err; }
drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0); drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
ret = drm_connector_set_path_property(connector, pathprop);
- ret = intel_connector_set_path_property(connector, "%s", pathprop); if (ret) goto err;
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c index 22666d28f4aa..4e7ea0f4c5d5 100644 --- a/drivers/gpu/drm/i915/intel_dvo.c +++ b/drivers/gpu/drm/i915/intel_dvo.c @@ -531,6 +531,9 @@ void intel_dvo_init(struct drm_i915_private *dev_priv) connector->interlace_allowed = false; connector->doublescan_allowed = false;
intel_connector_set_path_property(connector, "dvo:%d",port - PORT_A);- intel_connector_attach_encoder(intel_connector, intel_encoder); if (dvo->type == INTEL_DVO_CHIP_LVDS) { /*
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 187a2b828b97..38a0e423420a 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -2794,6 +2794,10 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c struct drm_i915_private *dev_priv = to_i915(connector->dev); struct intel_digital_port *intel_dig_port = hdmi_to_dig_port(intel_hdmi);
enum port port = intel_dig_port->base.port;
intel_connector_set_path_property(connector, "ddi:%d",
port - PORT_A);intel_attach_force_audio_property(connector); intel_attach_broadcast_rgb_property(connector);
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index efefed62a7f8..463665f0ecbd 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -915,6 +915,8 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
lvds_encoder->reg = lvds_reg;
- intel_connector_set_path_property(connector, "lvds:0");
- /* create the scaling mode property */ allowed_scalers = BIT(DRM_MODE_SCALE_ASPECT); allowed_scalers |= BIT(DRM_MODE_SCALE_FULLSCREEN);
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 0860ae36bb87..c16cdde849cc 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -2650,9 +2650,8 @@ static struct intel_sdvo_connector *intel_sdvo_connector_alloc(void) static bool intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device) {
- struct drm_encoder *encoder = &intel_sdvo->base.base;
- struct intel_encoder *encoder = &intel_sdvo->base; struct drm_connector *connector;
- struct intel_encoder *intel_encoder = to_intel_encoder(encoder); struct intel_connector *intel_connector; struct intel_sdvo_connector *intel_sdvo_connector;
@@ -2679,12 +2678,12 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device) * Some SDVO devices have one-shot hotplug interrupts. * Ensure that they get re-enabled when an interrupt happens. */
intel_encoder->hotplug = intel_sdvo_hotplug;intel_sdvo_enable_hotplug(intel_encoder);
encoder->hotplug = intel_sdvo_hotplug;intel_sdvo_enable_hotplug(encoder);
- encoder->encoder_type = DRM_MODE_ENCODER_TMDS;
encoder->base.encoder_type = DRM_MODE_ENCODER_TMDS; connector->connector_type = DRM_MODE_CONNECTOR_DVID;
if (intel_sdvo_is_hdmi_connector(intel_sdvo, device)) {
@@ -2700,13 +2699,18 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device) if (intel_sdvo_connector->is_hdmi) intel_sdvo_add_hdmi_properties(intel_sdvo, intel_sdvo_connector);
- intel_connector_set_path_property(connector, "sdvo:%d:%s:%d",
encoder->port - PORT_A,intel_sdvo_connector->is_hdmi ?"hdmi" : "dvi", device);- return true;
}
static bool intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type) {
- struct drm_encoder *encoder = &intel_sdvo->base.base;
- struct intel_encoder *encoder = &intel_sdvo->base; struct drm_connector *connector; struct intel_connector *intel_connector; struct intel_sdvo_connector *intel_sdvo_connector;
@@ -2719,7 +2723,7 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
intel_connector = &intel_sdvo_connector->base; connector = &intel_connector->base;
- encoder->encoder_type = DRM_MODE_ENCODER_TVDAC;
encoder->base.encoder_type = DRM_MODE_ENCODER_TVDAC; connector->connector_type = DRM_MODE_CONNECTOR_SVIDEO;
intel_sdvo->controlled_output |= type;
@@ -2736,6 +2740,9 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type) if (!intel_sdvo_create_enhance_property(intel_sdvo, intel_sdvo_connector)) goto err;
- intel_connector_set_path_property(connector, "sdvo:%d:tv:%d",
encoder->port - PORT_A, type);- return true;
err: @@ -2746,7 +2753,7 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type) static bool intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device) {
- struct drm_encoder *encoder = &intel_sdvo->base.base;
- struct intel_encoder *encoder = &intel_sdvo->base; struct drm_connector *connector; struct intel_connector *intel_connector; struct intel_sdvo_connector *intel_sdvo_connector;
@@ -2760,7 +2767,7 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device) intel_connector = &intel_sdvo_connector->base; connector = &intel_connector->base; intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
- encoder->encoder_type = DRM_MODE_ENCODER_DAC;
encoder->base.encoder_type = DRM_MODE_ENCODER_DAC; connector->connector_type = DRM_MODE_CONNECTOR_VGA;
if (device == 0) {
@@ -2776,13 +2783,16 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device) return false; }
- intel_connector_set_path_property(connector, "sdvo:%d:crt:%d",
encoder->port - PORT_A, device);- return true;
}
static bool intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device) {
- struct drm_encoder *encoder = &intel_sdvo->base.base;
- struct intel_encoder *encoder = &intel_sdvo->base; struct drm_connector *connector; struct intel_connector *intel_connector; struct intel_sdvo_connector *intel_sdvo_connector;
@@ -2796,7 +2806,7 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
intel_connector = &intel_sdvo_connector->base; connector = &intel_connector->base;
- encoder->encoder_type = DRM_MODE_ENCODER_LVDS;
encoder->base.encoder_type = DRM_MODE_ENCODER_LVDS; connector->connector_type = DRM_MODE_CONNECTOR_LVDS;
if (device == 0) {
@@ -2831,6 +2841,9 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device) if (!intel_connector->panel.fixed_mode) goto err;
- intel_connector_set_path_property(connector, "sdvo:%d:lvds:%d",
encoder->port - PORT_A, device);- return true;
err: diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c index 5dc594eafaf2..f9481404f642 100644 --- a/drivers/gpu/drm/i915/intel_tv.c +++ b/drivers/gpu/drm/i915/intel_tv.c @@ -1988,4 +1988,6 @@ intel_tv_init(struct drm_i915_private *dev_priv) drm_object_attach_property(&connector->base, dev->mode_config.tv_bottom_margin_property, state->tv.margins.bottom);
- intel_connector_set_path_property(connector, "tv:0");
} diff --git a/drivers/gpu/drm/i915/vlv_dsi.c b/drivers/gpu/drm/i915/vlv_dsi.c index e272d826210a..e97e689c6021 100644 --- a/drivers/gpu/drm/i915/vlv_dsi.c +++ b/drivers/gpu/drm/i915/vlv_dsi.c @@ -1985,6 +1985,9 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
intel_dsi_add_properties(intel_connector);
- intel_connector_set_path_property(connector, "dsi:%d",
port - PORT_A);- return;
err_cleanup_connector:
On Thu, Jun 13, 2019 at 09:43:33PM +0300, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Here's a possible apporoach for providing userspace with some stable connector identifiers. Combine with the bus name of the GPU and you should have some kind of real physical path description. Unfortunately the ship has sailed for MST connectors because userspace is already parsing the property and expects to find certain things there. So if we want stable names for those we'd probably have introduce another PATH prop (PHYS_PATH?).
I suppose one alternative would to make the connector type_id stable. Currently that is being populated by drm core and it's just a global counter. Not sure how badly things would turn out if we'd allow each driver to set that. It could result in conflicting xrandr connector names between different GPUs which I suppose would confuse existing userspace?
I think the only reason this global id stuff exists is because with original xrandr, that stuff was global. And then it got copypasted forever.
Would need to do a bunch of reviewing, but I'd expect we'll get away with just making all these allocators per-device. -Daniel
Cc: Daniel Vetter daniel@ffwll.ch Cc: Pekka Paalanen ppaalanen@gmail.com Cc: Ilia Mirkin imirkin@alum.mit.edu
Ville Syrjälä (2): drm: Improve PATH prop docs drm/i915: Populate PATH prop for every connector
drivers/gpu/drm/drm_connector.c | 13 ++++++++-- drivers/gpu/drm/i915/icl_dsi.c | 3 +++ drivers/gpu/drm/i915/intel_connector.c | 20 +++++++++++++++ drivers/gpu/drm/i915/intel_connector.h | 3 +++ drivers/gpu/drm/i915/intel_crt.c | 2 ++ drivers/gpu/drm/i915/intel_dp.c | 6 ++++- drivers/gpu/drm/i915/intel_dp_mst.c | 3 +-- drivers/gpu/drm/i915/intel_dvo.c | 3 +++ drivers/gpu/drm/i915/intel_hdmi.c | 4 +++ drivers/gpu/drm/i915/intel_lvds.c | 2 ++ drivers/gpu/drm/i915/intel_sdvo.c | 35 ++++++++++++++++++-------- drivers/gpu/drm/i915/intel_tv.c | 2 ++ drivers/gpu/drm/i915/vlv_dsi.c | 3 +++ 13 files changed, 83 insertions(+), 16 deletions(-)
-- 2.21.0
On Thu, 13 Jun 2019 22:42:08 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Thu, Jun 13, 2019 at 09:43:33PM +0300, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Here's a possible apporoach for providing userspace with some stable connector identifiers. Combine with the bus name of the GPU and you should have some kind of real physical path description. Unfortunately the ship has sailed for MST connectors because userspace is already parsing the property and expects to find certain things there. So if we want stable names for those we'd probably have introduce another PATH prop (PHYS_PATH?).
I suppose one alternative would to make the connector type_id stable. Currently that is being populated by drm core and it's just a global counter. Not sure how badly things would turn out if we'd allow each driver to set that. It could result in conflicting xrandr connector names between different GPUs which I suppose would confuse existing userspace?
I think the only reason this global id stuff exists is because with original xrandr, that stuff was global. And then it got copypasted forever.
Would need to do a bunch of reviewing, but I'd expect we'll get away with just making all these allocators per-device.
Hi,
I'm not sure I'm that optimistic... I assume most userspace uses type_id for naming already and might rely on uniqueness. Weston uses type_id, but does not rely on uniqueness yet, since it only handles one device so far.
The bigger problem to me however is changing the meaning of type_id, causing old kernels do one thing and new kernels do another thing. When userspace uses type_id for connector naming, it may use that name in configuration files. Weston does, but again is not affected because it doesn't support using multiple devices yet. If someone has two gfx cards in his machine, making type_id per-device changes the numbering, meaning the user's configuration does not apply anymore or applies wrong. I suppose it doesn't matter if the naming was already unreliable, since it is reliable if the drivers/devices happened to probe in the same order every boot.
Are connector names in xrandr still using type_id in their names? That would be a sure blocker, I think.
Thanks, pq
On 2019-06-27 9:35 a.m., Pekka Paalanen wrote:
Are connector names in xrandr still using type_id in their names?
Yes, they are.
(adding sunpeng.li@amd.com to the thread here, since this is relevant to the DP aux device work)
I mentioned this in IRC, but figured I should mention it on the ML as well so it can be discussed further. Honestly: I don't like the way we implement the path prop for MST. Mainly because
* It looks ugly: mst:<obj-id>-<port#1>-<port#2> is ambiguous looking. I didn't even realize the first number was actually supposed to be the object ID until I looked at the code * I strongly doubt object IDs are consistent enough for the path prop to actually be as meaningful as it looks * Obviously we can't just remove the path property, since it's being used in userspace. This has me somewhat convinced that I think it might be a better idea to just make a whole new path_v2 prop, and document that the path prop was a bad idea and is now deprecated (but still functional). If we did this, we could come up with a much nicer MST naming scheme as well! Consider:
For a connector with the RAD 0.1 living on the topology on DP-1 on card0:
mst:DP-1:0.1
I see multiple benefits to this: * Look how easy it is to read! * DP-1 isn't guaranteed to be consistent, but it is certainly far more likely to be consistent than an object ID. * This seems a lot easier to write udev rules for, imho
The only thing I'm not sure about whether or not we should also prepend the connector name with the device (e.g. card0, card1, etc.). I thought this might be necessary at first, but thinking about it - it shouldn't be hard to figure out the device in question from looking at sysfs since any userspace application will know which DRM fd the connector comes from.
Does this sound like a good idea? If so, I'd be happy to write up some patches for this
On Thu, 2019-06-13 at 21:43 +0300, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Here's a possible apporoach for providing userspace with some stable connector identifiers. Combine with the bus name of the GPU and you should have some kind of real physical path description. Unfortunately the ship has sailed for MST connectors because userspace is already parsing the property and expects to find certain things there. So if we want stable names for those we'd probably have introduce another PATH prop (PHYS_PATH?).
I suppose one alternative would to make the connector type_id stable. Currently that is being populated by drm core and it's just a global counter. Not sure how badly things would turn out if we'd allow each driver to set that. It could result in conflicting xrandr connector names between different GPUs which I suppose would confuse existing userspace?
Cc: Daniel Vetter daniel@ffwll.ch Cc: Pekka Paalanen ppaalanen@gmail.com Cc: Ilia Mirkin imirkin@alum.mit.edu
Ville Syrjälä (2): drm: Improve PATH prop docs drm/i915: Populate PATH prop for every connector
drivers/gpu/drm/drm_connector.c | 13 ++++++++-- drivers/gpu/drm/i915/icl_dsi.c | 3 +++ drivers/gpu/drm/i915/intel_connector.c | 20 +++++++++++++++ drivers/gpu/drm/i915/intel_connector.h | 3 +++ drivers/gpu/drm/i915/intel_crt.c | 2 ++ drivers/gpu/drm/i915/intel_dp.c | 6 ++++- drivers/gpu/drm/i915/intel_dp_mst.c | 3 +-- drivers/gpu/drm/i915/intel_dvo.c | 3 +++ drivers/gpu/drm/i915/intel_hdmi.c | 4 +++ drivers/gpu/drm/i915/intel_lvds.c | 2 ++ drivers/gpu/drm/i915/intel_sdvo.c | 35 ++++++++++++++++++-------- drivers/gpu/drm/i915/intel_tv.c | 2 ++ drivers/gpu/drm/i915/vlv_dsi.c | 3 +++ 13 files changed, 83 insertions(+), 16 deletions(-)
On Wed, Jul 10, 2019 at 06:43:53PM -0400, Lyude Paul wrote:
(adding sunpeng.li@amd.com to the thread here, since this is relevant to the DP aux device work)
I mentioned this in IRC, but figured I should mention it on the ML as well so it can be discussed further. Honestly: I don't like the way we implement the path prop for MST. Mainly because
- It looks ugly: mst:<obj-id>-<port#1>-<port#2> is ambiguous looking. I didn't even realize the first number was actually supposed to be the object ID until I looked at the code
- I strongly doubt object IDs are consistent enough for the path prop to actually be as meaningful as it looks
Obviously we can't just remove the path property, since it's being used in userspace. This has me somewhat convinced that I think it might be a better idea to just make a whole new path_v2 prop, and document that the path prop was a bad idea and is now deprecated (but still functional). If we did this, we could come up with a much nicer MST naming scheme as well! Consider:
For a connector with the RAD 0.1 living on the topology on DP-1 on card0:
mst:DP-1:0.1
I see multiple benefits to this:
- Look how easy it is to read!
- DP-1 isn't guaranteed to be consistent, but it is certainly far more likely to be consistent than an object ID.
- This seems a lot easier to write udev rules for, imho
We just had an epic discussion on irc about how connector names are _also_ not stable. At least not if you have more than drm_device (they're allocated from a global idr, because lolz and it's apparently uapi, xrandr will die if they're not globally unique). So actually worse than the object id, which at least on a given machine, and for any driver not doing something real funny with undefined load ordering, should be stable.
So maybe not so stable for soc drivers with lots of EPROBE_DEFER.
The only thing I'm not sure about whether or not we should also prepend the connector name with the device (e.g. card0, card1, etc.). I thought this might be necessary at first, but thinking about it - it shouldn't be hard to figure out the device in question from looking at sysfs since any userspace application will know which DRM fd the connector comes from.
Does this sound like a good idea? If so, I'd be happy to write up some patches for this
Stable naming is offiically hard. I think the most reasonable approach, and the one every DE implements, is to look at the serial/vendor in edid and just assume that cables/connectors/cards are fungible.
Anyway I'm not sure how useful any of this is ... There's also the hilarity that on older i915 cards, there's both hdmi and dp "connectors" for DP++ ports. -Daniel
On Thu, 2019-06-13 at 21:43 +0300, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Here's a possible apporoach for providing userspace with some stable connector identifiers. Combine with the bus name of the GPU and you should have some kind of real physical path description. Unfortunately the ship has sailed for MST connectors because userspace is already parsing the property and expects to find certain things there. So if we want stable names for those we'd probably have introduce another PATH prop (PHYS_PATH?).
I suppose one alternative would to make the connector type_id stable. Currently that is being populated by drm core and it's just a global counter. Not sure how badly things would turn out if we'd allow each driver to set that. It could result in conflicting xrandr connector names between different GPUs which I suppose would confuse existing userspace?
Cc: Daniel Vetter daniel@ffwll.ch Cc: Pekka Paalanen ppaalanen@gmail.com Cc: Ilia Mirkin imirkin@alum.mit.edu
Ville Syrjälä (2): drm: Improve PATH prop docs drm/i915: Populate PATH prop for every connector
drivers/gpu/drm/drm_connector.c | 13 ++++++++-- drivers/gpu/drm/i915/icl_dsi.c | 3 +++ drivers/gpu/drm/i915/intel_connector.c | 20 +++++++++++++++ drivers/gpu/drm/i915/intel_connector.h | 3 +++ drivers/gpu/drm/i915/intel_crt.c | 2 ++ drivers/gpu/drm/i915/intel_dp.c | 6 ++++- drivers/gpu/drm/i915/intel_dp_mst.c | 3 +-- drivers/gpu/drm/i915/intel_dvo.c | 3 +++ drivers/gpu/drm/i915/intel_hdmi.c | 4 +++ drivers/gpu/drm/i915/intel_lvds.c | 2 ++ drivers/gpu/drm/i915/intel_sdvo.c | 35 ++++++++++++++++++-------- drivers/gpu/drm/i915/intel_tv.c | 2 ++ drivers/gpu/drm/i915/vlv_dsi.c | 3 +++ 13 files changed, 83 insertions(+), 16 deletions(-)
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, 10 Jul 2019 18:43:53 -0400 Lyude Paul lyude@redhat.com wrote:
(adding sunpeng.li@amd.com to the thread here, since this is relevant to the DP aux device work)
I mentioned this in IRC, but figured I should mention it on the ML as well so it can be discussed further. Honestly: I don't like the way we implement the path prop for MST. Mainly because
- It looks ugly: mst:<obj-id>-<port#1>-<port#2> is ambiguous looking. I didn't even realize the first number was actually supposed to be the object ID until I looked at the code
- I strongly doubt object IDs are consistent enough for the path prop to actually be as meaningful as it looks
Obviously we can't just remove the path property, since it's being used in userspace. This has me somewhat convinced that I think it might be a better idea to just make a whole new path_v2 prop, and document that the path prop was a bad idea and is now deprecated (but still functional). If we did this, we could come up with a much nicer MST naming scheme as well! Consider:
For a connector with the RAD 0.1 living on the topology on DP-1 on card0:
mst:DP-1:0.1
I see multiple benefits to this:
- Look how easy it is to read!
- DP-1 isn't guaranteed to be consistent, but it is certainly far more likely to be consistent than an object ID.
Hi,
please, do not go with "more consistent but not fully". If the name is not all the way persistent, then it's useless because it will break things in rare cases. We already have a 80% or a 95% solutions, adding one more <100% solution does not help.
- This seems a lot easier to write udev rules for, imho
Wait, one can write udev rules for connectors and stuff? How? What can they do?
The only thing I'm not sure about whether or not we should also prepend the connector name with the device (e.g. card0, card1, etc.). I thought this might be necessary at first, but thinking about it - it shouldn't be hard to figure out the device in question from looking at sysfs since any userspace application will know which DRM fd the connector comes from.
From a display server perspective using the libdrm KMS API, yes, we always know from which device a connector comes from, and can make up a persistent name for the device ("card0" is not a persistent name).
Thanks, pq
Does this sound like a good idea? If so, I'd be happy to write up some patches for this
On Thu, 2019-06-13 at 21:43 +0300, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Here's a possible apporoach for providing userspace with some stable connector identifiers. Combine with the bus name of the GPU and you should have some kind of real physical path description. Unfortunately the ship has sailed for MST connectors because userspace is already parsing the property and expects to find certain things there. So if we want stable names for those we'd probably have introduce another PATH prop (PHYS_PATH?).
I suppose one alternative would to make the connector type_id stable. Currently that is being populated by drm core and it's just a global counter. Not sure how badly things would turn out if we'd allow each driver to set that. It could result in conflicting xrandr connector names between different GPUs which I suppose would confuse existing userspace?
Cc: Daniel Vetter daniel@ffwll.ch Cc: Pekka Paalanen ppaalanen@gmail.com Cc: Ilia Mirkin imirkin@alum.mit.edu
Ville Syrjälä (2): drm: Improve PATH prop docs drm/i915: Populate PATH prop for every connector
drivers/gpu/drm/drm_connector.c | 13 ++++++++-- drivers/gpu/drm/i915/icl_dsi.c | 3 +++ drivers/gpu/drm/i915/intel_connector.c | 20 +++++++++++++++ drivers/gpu/drm/i915/intel_connector.h | 3 +++ drivers/gpu/drm/i915/intel_crt.c | 2 ++ drivers/gpu/drm/i915/intel_dp.c | 6 ++++- drivers/gpu/drm/i915/intel_dp_mst.c | 3 +-- drivers/gpu/drm/i915/intel_dvo.c | 3 +++ drivers/gpu/drm/i915/intel_hdmi.c | 4 +++ drivers/gpu/drm/i915/intel_lvds.c | 2 ++ drivers/gpu/drm/i915/intel_sdvo.c | 35 ++++++++++++++++++-------- drivers/gpu/drm/i915/intel_tv.c | 2 ++ drivers/gpu/drm/i915/vlv_dsi.c | 3 +++ 13 files changed, 83 insertions(+), 16 deletions(-)
On 2019-07-11 3:29 a.m., Pekka Paalanen wrote:
Wait, one can write udev rules for connectors and stuff? How? What can they do?
I was using it to generate user-friendly device names for the mst aux implementation: https://patchwork.freedesktop.org/patch/315900/?series=63237&rev=2
Leo
On Tue, 16 Jul 2019 14:59:58 +0000 "Li, Sun peng (Leo)" Sunpeng.Li@amd.com wrote:
On 2019-07-11 3:29 a.m., Pekka Paalanen wrote:
Wait, one can write udev rules for connectors and stuff? How? What can they do?
I was using it to generate user-friendly device names for the mst aux implementation: https://patchwork.freedesktop.org/patch/315900/?series=63237&rev=2
Hi,
what is that device node used for?
Are the "by-path" symlinks to help a display server associate the right device node with the right DRM KMS connector resource?
The patch commit message did not explain what the names are actually used for.
Thanks, pq
On 2019-08-01 5:51 a.m., Pekka Paalanen wrote:
On Tue, 16 Jul 2019 14:59:58 +0000 "Li, Sun peng (Leo)" Sunpeng.Li@amd.com wrote:
On 2019-07-11 3:29 a.m., Pekka Paalanen wrote:
Wait, one can write udev rules for connectors and stuff? How? What can they do?
I was using it to generate user-friendly device names for the mst aux implementation: https://patchwork.freedesktop.org/patch/315900/?series=63237&rev=2
Hi,
what is that device node used for?
Are the "by-path" symlinks to help a display server associate the right device node with the right DRM KMS connector resource?
I intended it to be something more descriptive than the '/dev/drm_dp_aux0, drm_dp_aux1, drm_dp_aux2, ...' names, to help users identify the connector they're addressing in the mst topology. I guess it could also be used for the purpose you mention as well.
Of course, we'd need more reliable and persistent PATH props first. The patch was dropped until this happens.
Leo
The patch commit message did not explain what the names are actually used for.
Thanks, pq
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Li, Sun peng (Leo) -
Lyude Paul -
Michel Dänzer -
Pekka Paalanen -
Ville Syrjala