| Message ID | 1466499109-20240-2-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 21, 2016 at 11:51:47AM +0300, Imre Deak wrote: > Atm on IBX/CPT 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 DPCD, 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. > v3: > - Use IBX/CPT reference instead of the incorrect ILK, add a WARN about > this. (Ville) > > 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> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v2) > --- > drivers/gpu/drm/i915/intel_display.c | 5 +++++ > drivers/gpu/drm/i915/intel_dp.c | 17 +++++++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 0b2cd66..1141b86 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14710,6 +14710,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 ffa43ec..9a1cef4 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -5303,6 +5303,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > 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 intel_encoder *tmp_encoder; > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_display_mode *fixed_mode = NULL; > struct drm_display_mode *downclock_mode = NULL; > @@ -5314,6 +5315,22 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > if (!is_edp(intel_dp)) > return true; > > + /* > + * On IBX/CPT 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, tmp_encoder) { > + if (tmp_encoder->type != INTEL_OUTPUT_LVDS) > + continue; > + > + WARN_ON(!(HAS_PCH_IBX(dev_priv) || HAS_PCH_CPT(dev_priv))); I think I usually prefer '!A && !B' for whatever reason. Either works. > + DRM_INFO("LVDS was detected, not registering eDP\n"); > + > + return false; > + } I would have probably stuffed the loop into a small helper (eg. has_lvds_encoder() or something along those lines). But this works too, so Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + > pps_lock(intel_dp); > intel_edp_panel_vdd_sanitize(intel_dp); > pps_unlock(intel_dp); > -- > 2.5.0
On ke, 2016-06-22 at 15:26 +0300, Ville Syrjälä wrote: > On Tue, Jun 21, 2016 at 11:51:47AM +0300, Imre Deak wrote: > > Atm on IBX/CPT 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 DPCD, 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. > > v3: > > - Use IBX/CPT reference instead of the incorrect ILK, add a WARN about > > this. (Ville) > > > > 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> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v2) > > --- > > drivers/gpu/drm/i915/intel_display.c | 5 +++++ > > drivers/gpu/drm/i915/intel_dp.c | 17 +++++++++++++++++ > > 2 files changed, 22 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 0b2cd66..1141b86 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -14710,6 +14710,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 ffa43ec..9a1cef4 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -5303,6 +5303,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > > 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 intel_encoder *tmp_encoder; > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct drm_display_mode *fixed_mode = NULL; > > struct drm_display_mode *downclock_mode = NULL; > > @@ -5314,6 +5315,22 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > > if (!is_edp(intel_dp)) > > return true; > > > > + /* > > + * On IBX/CPT 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, tmp_encoder) { > > + if (tmp_encoder->type != INTEL_OUTPUT_LVDS) > > + continue; > > + > > + WARN_ON(!(HAS_PCH_IBX(dev_priv) || HAS_PCH_CPT(dev_priv))); > > I think I usually prefer '!A && !B' for whatever reason. Either works. I couldn't decide either, we have it both ways in the driver. > > + DRM_INFO("LVDS was detected, not registering eDP\n"); > > + > > + return false; > > + } > > I would have probably stuffed the loop into a small helper > (eg. has_lvds_encoder() or something along those lines). But > this works too, so There was the same loop in intel_lvds.c, so I added a helper there and changed it here while applying. I pushed all 3 patches to -dinq, thanks for the reviews. > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > + > > pps_lock(intel_dp); > > intel_edp_panel_vdd_sanitize(intel_dp); > > pps_unlock(intel_dp); > > -- > > 2.5.0 >
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0b2cd66..1141b86 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14710,6 +14710,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 ffa43ec..9a1cef4 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -5303,6 +5303,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, 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 intel_encoder *tmp_encoder; struct drm_i915_private *dev_priv = dev->dev_private; struct drm_display_mode *fixed_mode = NULL; struct drm_display_mode *downclock_mode = NULL; @@ -5314,6 +5315,22 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, if (!is_edp(intel_dp)) return true; + /* + * On IBX/CPT 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, tmp_encoder) { + if (tmp_encoder->type != INTEL_OUTPUT_LVDS) + continue; + + WARN_ON(!(HAS_PCH_IBX(dev_priv) || HAS_PCH_CPT(dev_priv))); + 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);