[Intel-gfx] [PATCH] drm/i915: Retry port as HDMI if dp_is_edp turns out to be false
Sivakumar Thulasimani
sivakumar.thulasimani at intel.com
Sun Aug 9 23:05:40 PDT 2015
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 at 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 at chris-wilson.co.uk>
>>> Cc: Jesse Barnes <jbarnes at 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 at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
regards,
Sivakumar
More information about the Intel-gfx
mailing list