| Message ID | 1439122373-16425-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
> -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of > Lukas Wunner > 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. Sorry to jump in. Could this help another use case as below ? We have some Bytrail machine (Bayley Bay), we applied HW rework to disable eDP connectivity to DDI1 and enable DP on DDI 1. But we found the i915 driver still take this DDI as eDP, not DP. we suspect it's because VBT still takes it as DP and so i915 driver just follows. If we don't want to revise VBT in BIOS after every rework, is there any other way to let i915 detect this is a DP link? Thanks Mengdong > > 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
hi Mengdong,
is there any reason why you cannot modify VBT ? unless it is
shipped version you
can just flash the modified VBT along with BIOS.
Chris,
i would be even more surprised if VBIOS/GOP can enable some display
when it is
configured incorrectly in VBT. Give me a day to check with someone if
this is even
possible in that level.
On 8/10/2015 8:00 AM, Lin, Mengdong wrote:
>> -----Original Message-----
>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>> Lukas Wunner
>> 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.
> Sorry to jump in. Could this help another use case as below ?
>
> We have some Bytrail machine (Bayley Bay), we applied HW rework to disable eDP connectivity to DDI1 and enable DP on DDI 1.
> But we found the i915 driver still take this DDI as eDP, not DP. we suspect it's because VBT still takes it as DP and so i915 driver just follows.
>
> If we don't want to revise VBT in BIOS after every rework, is there any other way to let i915 detect this is a DP link?
>
> Thanks
> Mengdong
>
>>> 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris,
After asking few people found that the simplest explanation and
plausible root cause
being that PORT D is set for both eDP and HDMI :). in which case the
proper fix is to
check in intel_dp_is_edp, if PORT D is again set for any other display
and return false
if that is the case
or
have a quirk for this config since this seems to be reported atleast for
now as suggested
by Lukas
i would recommend the first method, if we confirm the root cause is as
explained above.
On 8/10/2015 11:35 AM, Sivakumar Thulasimani wrote:
> hi Mengdong,
> is there any reason why you cannot modify VBT ? unless it is
> shipped version you
> can just flash the modified VBT along with BIOS.
>
> Chris,
> i would be even more surprised if VBIOS/GOP can enable some
> display when it is
> configured incorrectly in VBT. Give me a day to check with someone if
> this is even
> possible in that level.
>
> On 8/10/2015 8:00 AM, Lin, Mengdong wrote:
>>> -----Original Message-----
>>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
>>> Behalf Of
>>> Lukas Wunner
>>> 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.
>> Sorry to jump in. Could this help another use case as below ?
>>
>> We have some Bytrail machine (Bayley Bay), we applied HW rework to
>> disable eDP connectivity to DDI1 and enable DP on DDI 1.
>> But we found the i915 driver still take this DDI as eDP, not DP. we
>> suspect it's because VBT still takes it as DP and so i915 driver just
>> follows.
>>
>> If we don't want to revise VBT in BIOS after every rework, is there
>> any other way to let i915 detect this is a DP link?
>>
>> Thanks
>> Mengdong
>>
>>>> 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
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
On Mon, Aug 10, 2015 at 04:05:31PM +0530, Sivakumar Thulasimani wrote: > Chris, > After asking few people found that the simplest explanation and > plausible root cause > being that PORT D is set for both eDP and HDMI :). in which case the > proper fix is to > check in intel_dp_is_edp, if PORT D is again set for any other > display and return false > if that is the case The only difference between that and my patch is that you would not expose the DP link if the VBT flagged it as eDP. -Chris
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.
On Wed, Aug 12, 2015 at 04:45:39PM +0300, Ville Syrjälä wrote: > > - /* > > - * 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. Hmm, I was hoping that all the additional setup we do in init for edp would be enough for it to use any and all sidebands as necessary. The fallout will be that we simply cannot reuse the false eDP link as a plain DP and just establish one HDMI connector for the port. Or maybe it is still recoverable? -Chris
On Wed, Aug 12, 2015 at 03:14:53PM +0100, Chris Wilson wrote: > On Wed, Aug 12, 2015 at 04:45:39PM +0300, Ville Syrjälä wrote: > > > - /* > > > - * 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. > > Hmm, I was hoping that all the additional setup we do in init for edp > would be enough for it to use any and all sidebands as necessary. It might work by accident at least on some machines since BIOSen seem to leave the vdd force enabled for eDP panels even if the port isn't active. But if that's not the case, then we'd fail to power up the panel before attempting the initial aux communication, and the eDP probe would fail. > > The fallout will be that we simply cannot reuse the false eDP link as a > plain DP and just establish one HDMI connector for the port. Or maybe it > is still recoverable? If we can keep is_edp() returning true during the edp init, things should work OK. For vlv/chv we might need to poke at the pps (maybe just vlv_detach_power_sequencer() would be enough) to make normal DP work without the pps preventing the port from powering up. That should be testable by pretending the VBT is lying about a DP port being an eDP port.
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);
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(-)