Re: [Intel-gfx] [PATCH 6/7] drm/i915: Check HPD live state during eDP probe

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

On Thu, 2023-03-02 at 18:10 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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@linux.intel.com>
> ---

Reviewed-by: Vinod Govindapillai <vinod.govindapillai@intel.com>

>  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) {