On Thu, 2023-03-02 at 18:10 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> We need to untangle the mess where some SKL machines (at least)
> declare both DDI A and DDI E to be present in their VBT, and
> both using AUX A. DDI A is a ghost eDP, wheres DDI E may be a
> real DP->VGA converter.
>
> Currently that is handled by checking the VBT child devices
> for conflicts before output probing. But that kind of solution
> will not work for the ADL phantom dual eDP VBTs. I think on
> those we just have to probe the eDP first. And would be nice
> to use the same probe scheme for everything.
>
> On these SKL systems if we probe DDI A first (which is only
> natural given it's declared by VBT first) we will get an answer
> via AUX, but it came from the DP->VGA converter hooked to the
> DDI E, not DDI A. Thus we mistakenly register eDP on DDI A
> and screw up the real DP device in DDI E.
>
> To fix this let's check the HPD live state during the eDP probe.
> If we got an answer via DPCD but HPD is still down let's assume
> we got the answer from someone else.
>
> Smoke tested on all my eDP machines (ilk,hsw-ult,tgl,adl) and
> I also tested turning off all HPD hardware prior to loading
> i915 to make sure it all comes up properly. And I simulated
> the failure path too by not turning on HPD sense and that
> correctly gave up on eDP.
>
> I *think* Windows might just fully depend on HPD here. I
> couldn't really find any other way they probe displays. And
> I did find code where they also check the live state prior
> to AUX transfers (something Imre and I have also talked
> about perhaps doing). That would also solve this as we'd
> not succeed in the eDP probe DPCD reads.
>
> Other solutions I've considered:
>
> - Reintrduce DDI strap checks on SKL. Unfortunately we just
> don't have any idea how reliable they are on real production
> hardware, and commit 5a2376d1360b ("drm/i915/skl: WaIgnoreDDIAStrap
> is forever, always init DDI A") does suggest that not very.
> Sadly that commit is very poor in details :/
>
> Also the systems (Asrock B250M-HDV at least) fixed by commit
> 41e35ffb380b ("drm/i915: Favor last VBT child device with
> conflicting AUX ch/DDC pin") might still not work since we
> don't know what their straps indicate. Stupid me for not
> asking the reporter to check those at the time :(
>
> We have currently two CI machines (fi-cfl-guc,fi-cfl-8700k
> both MS-7B54/Z370M) that also declare both DDI A and DDI E
> in VBT to use AUX A, and on these the DDI A strap is also
> set. There doesn't seem to be anything hooked up to either
> DDI however. But given the DDI A strap is wrong on these it
> might well be wrong on the Asrock too.
>
> Most other CI machines seem to have straps that generally
> match the VBT. fi-kbl-soraka is an exception though as DDI D
> strap is not set, but it is declared in VBT as a DP++ port.
> No idea if there's a real physical port to go with it or not.
>
> - Some kind of quirk just for the cases where both DDI A and DDI E
> are present in VBT. Might be feasible given we've ignored
> DDI A in these cases up to now successfully. But feels rather
> unsatisfactory, and not very future proof against funny VBTs.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=111966
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
Reviewed-by: Vinod Govindapillai <vinod.govindapillai@xxxxxxxxx>
> drivers/gpu/drm/i915/display/intel_dp.c | 28 +++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index aee93b0d810e..35b02278d840 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -46,6 +46,7 @@
> #include "g4x_dp.h"
> #include "i915_debugfs.h"
> #include "i915_drv.h"
> +#include "i915_irq.h"
> #include "i915_reg.h"
> #include "intel_atomic.h"
> #include "intel_audio.h"
> @@ -5308,6 +5309,15 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> goto out_vdd_off;
> }
>
> + /*
> + * Enable HPD sense for live status check.
> + * intel_hpd_irq_setup() will turn it off again
> + * if it's no longer needed later.
> + *
> + * The DPCD probe below will make sure VDD is on.
> + */
> + intel_hpd_enable_detection(encoder);
> +
> /* Cache DPCD and EDID for edp. */
> has_dpcd = intel_edp_init_dpcd(intel_dp);
>
> @@ -5319,6 +5329,24 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> goto out_vdd_off;
> }
>
> + /*
> + * VBT and straps are liars. Also check HPD as that seems
> + * to be the most reliable piece of information available.
> + */
> + if (!intel_digital_port_connected(encoder)) {
> + /*
> + * If this fails, presume the DPCD answer came
> + * from some other port using the same AUX CH.
> + *
> + * FIXME maybe cleaner to check this before the
> + * DPCD read? Would need sort out the VDD handling...
> + */
> + drm_info(&dev_priv->drm,
> + "[ENCODER:%d:%s] HPD is down, disabling eDP\n",
> + encoder->base.base.id, encoder->base.name);
> + goto out_vdd_off;
> + }
> +
> mutex_lock(&dev_priv->drm.mode_config.mutex);
> drm_edid = drm_edid_read_ddc(connector, &intel_dp->aux.ddc);
> if (!drm_edid) {
