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

Ville Syrjälä
2015-08-12T13:45:55Z

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
> 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;

aux (and IIRC pps setup on vlv/chv) needs is_edp() to work. So I think
this would break stuff.

--
Ville Syrjälä
Intel OTC