Message ID | 1466198965-13821-1-git-send-email-imre.deak@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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? (I believe that LVDS/eDP doesn't coexist...) 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.) -Chris
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. --Imre
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) 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? Either that or we have missed a fuse to override the DP detected bit. -Chris
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> Regardless of the discussion we are having about whether there may be more complications, or if we have missed something, this patch seems a sensible guard. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
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
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 8c6f4e2..b6bb438 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14708,6 +14708,11 @@ static void intel_setup_outputs(struct drm_device *dev) struct intel_encoder *encoder; bool dpd_is_edp = false; + /* + * intel_edp_init_connector() depends on this completing first, to + * prevent the registeration of both eDP and LVDS and the incorrect + * sharing of the PPS. + */ intel_lvds_init(dev); if (intel_crt_present(dev)) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index be08351..5f7d731 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -5303,9 +5303,9 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, { struct drm_connector *connector = &intel_connector->base; struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); - struct intel_encoder *intel_encoder = &intel_dig_port->base; - struct drm_device *dev = intel_encoder->base.dev; + struct drm_device *dev = intel_dig_port->base.base.dev; struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_encoder *intel_encoder; struct drm_display_mode *fixed_mode = NULL; struct drm_display_mode *downclock_mode = NULL; bool has_dpcd; @@ -5316,6 +5316,19 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, if (!is_edp(intel_dp)) return true; + /* + * On ILK we may get here with LVDS already registered. Since the + * driver uses the only internal power sequencer available for both + * eDP and LVDS bail out early in this case to prevent interfering + * with an already powered-on LVDS power sequencer. + */ + for_each_intel_encoder(dev, intel_encoder) { + if (intel_encoder->type == INTEL_OUTPUT_LVDS) { + DRM_INFO("LVDS was detected, not registering eDP\n"); + return false; + } + } + pps_lock(intel_dp); intel_edp_panel_vdd_sanitize(intel_dp); pps_unlock(intel_dp);
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> --- drivers/gpu/drm/i915/intel_display.c | 5 +++++ drivers/gpu/drm/i915/intel_dp.c | 17 +++++++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-)