Re: [PATCH v3 1/3] drm/i915/ibx, cpt: Don't attempt to register eDP if LVDS was detected

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

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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx