Re: [RFC][PATCH 2/2] drm/i915: Populate PATH prop for every connector

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 14023 bytes --]

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);
>  	} 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:


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel