Re: [PATCH v2 1/2] drm/i915/ilk: Don't attempt to register eDP if LVDS was detected

[Imre Deak <imre.deak@intel.com>]

On ma, 2016-06-20 at 08:54 +0100, Chris Wilson wrote:
> On Sun, Jun 19, 2016 at 01:18:32PM +0300, Imre Deak wrote:
> > On Fri, 2016-06-17 at 23:35 +0100, Chris Wilson wrote:
> > > On Sat, Jun 18, 2016 at 12:29:24AM +0300, Imre Deak wrote:
> > > > Atm on ILK we attempt to detect if eDP is present even if LVDS was
> > > > already detected and an encoder for it was registered. This involves
> > > > trying to read out the eDP EDID, which in turn needs the same power
> > > > sequencer that LVDS uses. Poking at the VDD line at an unexpected time
> > > > may or may not interfere with the LVDS panel, but it's probably safer to
> > > > prevent this. Registering both an LVDS and an eDP connector would also
> > > > present a similar problem accessing the shared PPS at any point later in
> > > > an unexpected way.
> > > > 
> > > > We also need this to be able fix PPS initialization before its first use
> > > > in the next patch. For that we want to be sure that PPS is not in use
> > > > by LVDS.
> > > > 
> > > > v2:
> > > > - Split out the PPS init fix to a separate patch. (Chris)
> > > > - Add comment about eDP init depending on LVDS init. (Chris)
> > > > - Make the use of the intel_encoder ptr less error prone.
> > > > 
> > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > CC: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > 
> > > Main worry here is what if the LVDS detection was false?
> > 
> > That wouldn't really work anyway atm. We'd end up with both LVDS and
> > eDP registered and the subsequent LVDS modeset toggling the eDP panel
> > power out of order with respect to eDP's own pipe, PLL, port enabling
> > sequence. In the worst case we'd violate panel specs, for instance with
> > an LVDS panel off->eDP forced VDD sequence.
> > 
> > > (I believe that LVDS/eDP doesn't coexist...)
> > 
> > Right, they both use the single PPS we have which can't be shared.
> > 
> > > I'm just wondering if calling lvds_encoder->post_disable() to force the
> > > LVDS off in this circumstance is viable. Worst case (false eDP, real
> > > LVDS), we lose the output on the console until a mode is restored by fbdev.
> > > Best case (false LVDS, real eDP) we don't regress detection of eDP.
> > > 
> > > (Or knowing the internals, we could just do a save restore of LVDS
> > > PP_CONTROL around the eDP detection.)
> > 
> > The proper way to implement that kind of workaround would be to
> > unregister (or permanently disable) the LVDS encoder/connector if eDP
> > detection succeeds. We would also have to disable LVDS unconditionally
> > on ILK before eDP detect, even on a correctly detected LVDS output,
> > since we run the eDP detection also unconditionally. I think we should
> > only add support for this if we know that such broken systems exist.
> 
> Ah, but we don't do the eDP detection unconditionally. We only try and
> register the eDP ports if the hardware tells us it is present...
> 
> has_edp_a() (DP on port D invokes trust in the VBT)

Ah, yes, missed that check.

> So we are in a situation where the hw claims there is both a LVDS and
> eDP connection. You have already demonstrated that such broken HW
> exists, have you not?

I didn't, the issue this and the next patch fixes was on SKL. But yes

commit f30d26e468322b50d5e376bec40658683aff8ece
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Wed Jan 16 10:53:40 2013 +0200

    drm/i915/eDP: do not write power sequence registers for ghost eDP

show that there is such systems out there, although in the above case
the LVDS output wasn't a ghost. In any case I agree now that we should
consider this case and add proper support for this. However I think if
BIOS has enabled the LVDS output that should already give enough
confidence that the LVDS is real. In that way we could avoid disabling
a properly functioning LVDS output (leading to flicker even with
fastboot). I've put something together for this (top three commits):
https://github.com/ideak/linux/commits/lvds_edp_init

This still misses disabling the LVDS connector/encoder, I could still
add that on top.

> Either that or we have missed a fuse to override the DP detected bit.

We do check the strap bit on ILK, but we have the same issue on SNB/IVB
as pointed out by Ville, where we don't. The machine in the above
commit was an IVB/port A, so it's possible we could've avoided the
issue by checking a strap bit if it exists. I will check the docs.

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