Re: [PATCH] drm/i915: Retry port as HDMI if dp_is_edp turns out to be false

[Lukas Wunner <lukas@wunner.de>]

Hi,

On Sun, Aug 09, 2015 at 01:12:53PM +0100, Chris Wilson wrote:
> We follow the VBT as to whether a DDI port is used for eDP and if so, do
> not attach a HDMI encoder to it. However there are machines for which
> the VBT eDP flag is a lie (shocking!) and we fail to detect a eDP link.
> Furthermore, on those machines the HDMI is connected to that DDI port
> but we ignore it.
> 
> If we reorder our port initialisation to try the eDP setup first and
> if that fails we can treat it as a normal DP port along with a HDMI
> output. To accomplish this, we have to delay registering the DP
> connector/encoder until after we establish its final type.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88331

The existing code seems very carefully crafted, taking into account
the differences betweem various GPU generations etc, shuffling that
around might risk breakage. FWIW, I'm wondering if just adding a quirk
for this single Dell workstation might be justified?

Best regards,

Lukas


> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  27 ++++----
>  drivers/gpu/drm/i915/intel_dp.c      | 127 +++++++++++++++++------------------
>  drivers/gpu/drm/i915/intel_drv.h     |   6 +-
>  3 files changed, 79 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0bcd1b1aba4f..aab8dfd1f8a5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13941,7 +13941,6 @@ static void intel_setup_outputs(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_encoder *encoder;
> -	bool dpd_is_edp = false;
>  
>  	intel_lvds_init(dev);
>  
> @@ -13983,31 +13982,33 @@ static void intel_setup_outputs(struct drm_device *dev)
>  			intel_ddi_init(dev, PORT_D);
>  	} else if (HAS_PCH_SPLIT(dev)) {
>  		int found;
> -		dpd_is_edp = intel_dp_is_edp(dev, PORT_D);
>  
>  		if (has_edp_a(dev))
>  			intel_dp_init(dev, DP_A, PORT_A);
>  
> +		found = 0;
> +		/* PCH SDVOB multiplex with HDMIB */
>  		if (I915_READ(PCH_HDMIB) & SDVO_DETECTED) {
> -			/* PCH SDVOB multiplex with HDMIB */
>  			found = intel_sdvo_init(dev, PCH_SDVOB, true);
>  			if (!found)
>  				intel_hdmi_init(dev, PCH_HDMIB, PORT_B);
> -			if (!found && (I915_READ(PCH_DP_B) & DP_DETECTED))
> -				intel_dp_init(dev, PCH_DP_B, PORT_B);
>  		}
> +		if (!found && I915_READ(PCH_DP_B) & DP_DETECTED)
> +			intel_dp_init(dev, PCH_DP_B, PORT_B);
>  
> -		if (I915_READ(PCH_HDMIC) & SDVO_DETECTED)
> -			intel_hdmi_init(dev, PCH_HDMIC, PORT_C);
> -
> -		if (!dpd_is_edp && I915_READ(PCH_HDMID) & SDVO_DETECTED)
> -			intel_hdmi_init(dev, PCH_HDMID, PORT_D);
> -
> +		found = 0;
>  		if (I915_READ(PCH_DP_C) & DP_DETECTED)
> -			intel_dp_init(dev, PCH_DP_C, PORT_C);
> +			found = intel_dp_init(dev, PCH_DP_C, PORT_C);
> +		if (found != DRM_MODE_CONNECTOR_eDP &&
> +		    I915_READ(PCH_HDMIC) & SDVO_DETECTED)
> +			intel_hdmi_init(dev, PCH_HDMIC, PORT_C);
>  
> +		found = 0;
>  		if (I915_READ(PCH_DP_D) & DP_DETECTED)
> -			intel_dp_init(dev, PCH_DP_D, PORT_D);
> +			found = intel_dp_init(dev, PCH_DP_D, PORT_D);
> +		if (found != DRM_MODE_CONNECTOR_eDP &&
> +		    I915_READ(PCH_HDMID) & SDVO_DETECTED)
> +			intel_hdmi_init(dev, PCH_HDMID, PORT_D);
>  	} else if (IS_VALLEYVIEW(dev)) {
>  		/*
>  		 * The DP_DETECTED bit is the latched state of the DDC
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1e64a8c1e7cb..8adf500f3989 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1031,14 +1031,13 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  	return ret;
>  }
>  
> -static void
> +static int
>  intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	enum port port = intel_dig_port->port;
>  	const char *name = NULL;
> -	int ret;
>  
>  	switch (port) {
>  	case PORT_A:
> @@ -1080,20 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
>  	DRM_DEBUG_KMS("registering %s bus for %s\n", name,
>  		      connector->base.kdev->kobj.name);
>  
> -	ret = drm_dp_aux_register(&intel_dp->aux);
> -	if (ret < 0) {
> -		DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n",
> -			  name, ret);
> -		return;
> -	}
> -
> -	ret = sysfs_create_link(&connector->base.kdev->kobj,
> -				&intel_dp->aux.ddc.dev.kobj,
> -				intel_dp->aux.ddc.dev.kobj.name);
> -	if (ret < 0) {
> -		DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name, ret);
> -		drm_dp_aux_unregister(&intel_dp->aux);
> -	}
> +	return drm_dp_aux_register(&intel_dp->aux);
>  }
>  
>  static void
> @@ -5019,6 +5005,10 @@ bool intel_dp_is_edp(struct drm_device *dev, enum port port)
>  		[PORT_D] = PORT_IDPD,
>  	};
>  
> +	/* eDP only on port B and/or C on vlv/chv */
> +	if (IS_VALLEYVIEW(dev_priv) && !(port == PORT_B || port == PORT_C))
> +		return false;
> +
>  	if (port == PORT_A)
>  		return true;
>  
> @@ -5670,9 +5660,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	struct edid *edid;
>  	enum pipe pipe = INVALID_PIPE;
>  
> -	if (!is_edp(intel_dp))
> -		return true;
> -
>  	pps_lock(intel_dp);
>  	intel_edp_panel_vdd_sanitize(intel_dp);
>  	pps_unlock(intel_dp);
> @@ -5762,7 +5749,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	return true;
>  }
>  
> -bool
> +int
>  intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  			struct intel_connector *intel_connector)
>  {
> @@ -5772,6 +5759,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	struct drm_device *dev = intel_encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum port port = intel_dig_port->port;
> +	int ret;
>  	int type;
>  
>  	intel_dp->pps_pipe = INVALID_PIPE;
> @@ -5802,34 +5790,14 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	else
>  		type = DRM_MODE_CONNECTOR_DisplayPort;
>  
> -	/*
> -	 * For eDP we always set the encoder type to INTEL_OUTPUT_EDP, but
> -	 * for DP the encoder type can be set by the caller to
> -	 * INTEL_OUTPUT_UNKNOWN for DDI, so don't rewrite it.
> -	 */
> -	if (type == DRM_MODE_CONNECTOR_eDP)
> -		intel_encoder->type = INTEL_OUTPUT_EDP;
> -
> -	/* eDP only on port B and/or C on vlv/chv */
> -	if (WARN_ON(IS_VALLEYVIEW(dev) && is_edp(intel_dp) &&
> -		    port != PORT_B && port != PORT_C))
> -		return false;
> -
>  	DRM_DEBUG_KMS("Adding %s connector on port %c\n",
>  			type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP",
>  			port_name(port));
>  
> -	drm_connector_init(dev, connector, &intel_dp_connector_funcs, type);
> -	drm_connector_helper_add(connector, &intel_dp_connector_helper_funcs);
> -
>  	connector->interlace_allowed = true;
>  	connector->doublescan_allowed = 0;
>  
> -	INIT_DELAYED_WORK(&intel_dp->panel_vdd_work,
> -			  edp_panel_vdd_work);
> -
> -	intel_connector_attach_encoder(intel_connector, intel_encoder);
> -	drm_connector_register(connector);
> +	INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, edp_panel_vdd_work);
>  
>  	if (HAS_DDI(dev))
>  		intel_connector->get_hw_state = intel_ddi_connector_get_hw_state;
> @@ -5855,7 +5823,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  		BUG();
>  	}
>  
> -	if (is_edp(intel_dp)) {
> +	if (type == DRM_MODE_CONNECTOR_eDP) {
>  		pps_lock(intel_dp);
>  		intel_dp_init_panel_power_timestamps(intel_dp);
>  		if (IS_VALLEYVIEW(dev))
> @@ -5865,7 +5833,49 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  		pps_unlock(intel_dp);
>  	}
>  
> -	intel_dp_aux_init(intel_dp, intel_connector);
> +	ret = intel_dp_aux_init(intel_dp, intel_connector);
> +	if (ret < 0) {
> +		DRM_ERROR("registering DP-AUX channel for %s failed (%d)\n",
> +			  intel_dp->aux.name, ret);
> +		return 0;
> +	}
> +
> +	if (type == DRM_MODE_CONNECTOR_eDP &&
> +	    !intel_edp_init_connector(intel_dp, intel_connector)) {
> +		drm_dp_aux_unregister(&intel_dp->aux);
> +		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> +		/*
> +		 * vdd might still be enabled do to the delayed vdd off.
> +		 * Make sure vdd is actually turned off here.
> +		 */
> +		pps_lock(intel_dp);
> +		edp_panel_vdd_off_sync(intel_dp);
> +		pps_unlock(intel_dp);
> +
> +		/* Downgrade to a normal DP link */
> +		type = DRM_MODE_CONNECTOR_DisplayPort;
> +	}
> +
> +	/*
> +	 * For eDP we always set the encoder type to INTEL_OUTPUT_EDP, but
> +	 * for DP the encoder type can be set by the caller to
> +	 * INTEL_OUTPUT_UNKNOWN for DDI, so don't rewrite it.
> +	 */
> +	if (type == DRM_MODE_CONNECTOR_eDP)
> +		intel_encoder->type = INTEL_OUTPUT_EDP;
> +
> +	drm_connector_init(dev, connector, &intel_dp_connector_funcs, type);
> +	drm_connector_helper_add(connector, &intel_dp_connector_helper_funcs);
> +
> +	intel_connector_attach_encoder(intel_connector, intel_encoder);
> +	drm_connector_register(connector);
> +
> +	ret = sysfs_create_link(&connector->kdev->kobj,
> +				&intel_dp->aux.ddc.dev.kobj,
> +				intel_dp->aux.ddc.dev.kobj.name);
> +	if (ret < 0)
> +		DRM_ERROR("sysfs_create_link() for %s failed (%d)\n",
> +			  intel_dp->aux.name, ret);
>  
>  	/* init MST on ports that can support it */
>  	if (HAS_DP_MST(dev) &&
> @@ -5873,23 +5883,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  		intel_dp_mst_encoder_init(intel_dig_port,
>  					  intel_connector->base.base.id);
>  
> -	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
> -		drm_dp_aux_unregister(&intel_dp->aux);
> -		if (is_edp(intel_dp)) {
> -			cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> -			/*
> -			 * vdd might still be enabled do to the delayed vdd off.
> -			 * Make sure vdd is actually turned off here.
> -			 */
> -			pps_lock(intel_dp);
> -			edp_panel_vdd_off_sync(intel_dp);
> -			pps_unlock(intel_dp);
> -		}
> -		drm_connector_unregister(connector);
> -		drm_connector_cleanup(connector);
> -		return false;
> -	}
> -
>  	intel_dp_add_properties(intel_dp, connector);
>  
>  	/* For G4X desktop chip, PEG_BAND_GAP_DATA 3:0 must first be written
> @@ -5903,10 +5896,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  
>  	i915_debugfs_connector_add(connector);
>  
> -	return true;
> +	return type;
>  }
>  
> -void
> +int
>  intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -5914,15 +5907,16 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>  	struct intel_encoder *intel_encoder;
>  	struct drm_encoder *encoder;
>  	struct intel_connector *intel_connector;
> +	int ret;
>  
>  	intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL);
>  	if (!intel_dig_port)
> -		return;
> +		return 0;
>  
>  	intel_connector = intel_connector_alloc();
>  	if (!intel_connector) {
>  		kfree(intel_dig_port);
> -		return;
> +		return 0;
>  	}
>  
>  	intel_encoder = &intel_dig_port->base;
> @@ -5970,11 +5964,14 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>  	intel_dig_port->hpd_pulse = intel_dp_hpd_pulse;
>  	dev_priv->hotplug.irq_port[port] = intel_dig_port;
>  
> -	if (!intel_dp_init_connector(intel_dig_port, intel_connector)) {
> +	ret = intel_dp_init_connector(intel_dig_port, intel_connector);
> +	if (ret == 0) {
>  		drm_encoder_cleanup(encoder);
>  		kfree(intel_dig_port);
>  		kfree(intel_connector);
>  	}
> +
> +	return ret;
>  }
>  
>  void intel_dp_mst_suspend(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 18fe6d8b7d93..ade1ce853583 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1179,9 +1179,9 @@ void intel_csr_ucode_fini(struct drm_device *dev);
>  void assert_csr_loaded(struct drm_i915_private *dev_priv);
>  
>  /* intel_dp.c */
> -void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
> -bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> -			     struct intel_connector *intel_connector);
> +int intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
> +int intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> +			    struct intel_connector *intel_connector);
>  void intel_dp_start_link_train(struct intel_dp *intel_dp);
>  void intel_dp_complete_link_train(struct intel_dp *intel_dp);
>  void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx