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

[Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>]

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
>

-- 
regards,
Sivakumar

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx