[Intel-gfx] [PATCH] Revert "drm/i915: Implement Link Rate fallback on Link training failure"

[Ville Syrjälä]

On Wed, Mar 01, 2017 at 08:18:18PM +0200, Jani Nikula wrote:
> On Wed, 01 Mar 2017, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> > On Wed, Mar 01, 2017 at 07:35:15PM +0200, Jani Nikula wrote:
> >> On Wed, 01 Mar 2017, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> >> > On Wed, Mar 01, 2017 at 06:17:49PM +0100, Daniel Vetter wrote:
> >> >> This reverts commit 233ce881dd91fb13eb6b09deefae33168e6ead4c.
> >> >> 
> >> >> I assumed it's ok, but really should have double-checked - CI caught
> >> >> tons of fail :(
> >> 
> >> Considering the velocity of drm-tip, I think any CI results for patches
> >> have a rather limited best before date. The patch should've been resent
> >> and gone through testing again before merging.
> >> 
> >> > For the record, the failure comes from the error message in
> >> > intel_dp_get_link_train_fallback_values() as take the fallback path. As
> >> > userspace is informed, we don't need an *ERROR* at that point.
> >> >
> >> > The really interesting question is why we are seeing link-training
> >> > failures in CI at all, and whether igt should be checking and reporting
> >> > link-status=BAD.
> >> 
> >> It's possible (I didn't check the logs) this pertains to the failure
> >> mode I've sometimes seen, where clock recovery fails, but as we continue
> >> with channel equalization anyway (without this patch), everything
> >> succeeds there. At worst we need to root cause and fix that issue
> >> first. :(
> >
> > The skl case seems pretty clear. We register DP for both port A and port
> > E even though we should register it only for port E (I think). They
> > both end up both using AUX A and so we think the same sink is connected
> > to both, and then we try to enable port A which fail for obvious reasons.
> >
> > The culprit is init_vbt_defaults() which always sets .supports_dp=true
> > for port A unless later overridden by the VBT. In this case the VBT has
> > no port A, so we leave the .supports_dp flag set. So presumably we
> > should just nuke this stuff from init_vbt_defaults().
> >
> > IIRC this was discussed at some point between Imre and Paulo, but I
> > can't remember what the conclusion was, or if in fact there was one.
> 
> I think one idea was simply
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index e144f033f4b5..0261c841756f 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1377,10 +1377,6 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
>  			&dev_priv->vbt.ddi_port_info[port];
>  
>  		info->hdmi_level_shift = HDMI_LEVEL_SHIFT_UNKNOWN;
> -
> -		info->supports_dvi = (port != PORT_A && port != PORT_E);
> -		info->supports_hdmi = info->supports_dvi;
> -		info->supports_dp = (port != PORT_E);
>  	}
>  }
>  
> ---
> 
> but apparently it's problematic for machines without VBT, specifically
> some Chromebooks [citation needed].

I guess the other option would to be split this out and do it only when
there's no VBT. Or we could make the VBT parsing code explicitly clear
anything not present in the VBT.

-- 
Ville Syrjälä
Intel OTC